✨(entitlements) add Entitlements system with pluggable backends#1110
Draft
✨(entitlements) add Entitlements system with pluggable backends#1110
Conversation
This follows implementations in Drive, Messages and Calendars. This system allows Meet to gate some features for users depending on an authorization server. We provide 2 backends: a local one that always allows room creation, mimicking the current behaviour, and a DeployCenter backend, that fetches a "can_create" flag from a remote API. Future deployment contexts might add new backends, or reuse the API format of the DeployCenter one.
|
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request introduces a comprehensive entitlements system to control user access for creating meetings. The backend adds a new entitlements service module with an abstract base class, a DeployCenterEntitlementsBackend for fetching entitlements from an external API with caching and fallback behavior, a LocalEntitlementsBackend for development, and a factory for backend selection. The API layer integrates entitlements by enforcing can_create checks in room permissions (fail-closed on service unavailability), exposing a new UserMeSerializer with the can_create field, and warming the entitlements cache during authentication. The frontend conditionally renders the create menu only when can_create is true and displays localized access denial messages. Configuration settings enable backend selection, parameter passing, and cache timeout customization. Comprehensive tests cover all components. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/core/tests/test_api_users.py (1)
116-128: 🧹 Nitpick | 🔵 TrivialStub entitlements in this endpoint test.
can_createis now backend-driven, so assertingTruehere makes this test depend on global entitlements settings and service availability instead of only verifying/users/me/serialization. Patchcore.api.serializers.get_user_entitlements()or set the backend explicitly in the test.🔧 Suggested stabilization
+from unittest import mock ... - response = client.get( - "/api/v1.0/users/me/", - ) + with mock.patch( + "core.api.serializers.get_user_entitlements", + return_value={"can_create": True}, + ): + response = client.get( + "/api/v1.0/users/me/", + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/test_api_users.py` around lines 116 - 128, The test currently asserts a backend-driven entitlement value (can_create) making it flaky; stub or override get_user_entitlements in core.api.serializers for this test (or explicitly set the entitlements backend in the test) so the /api/v1.0/users/me/ response is deterministic—e.g., monkeypatch core.api.serializers.get_user_entitlements to return a fixed entitlement dict before calling client.get("/api/v1.0/users/me/") (or configure the test app to use a known entitlements backend) and then assert the exact expected serialized response.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/core/api/permissions.py`:
- Around line 61-68: The current create-permission branch swallows
EntitlementsUnavailableError and returns False without telemetry; update the
exception handler in the view.action == "create" block that calls
get_user_entitlements to emit an operator signal: log a warning via the app
logger (e.g., process_logger or the module logger) including
request.user.sub/email and the caught exception, and/or increment a metrics
counter (e.g., entitlements.unavailable or similar) so outages are visible; keep
the return False behavior but ensure the handler in permissions.py (around
get_user_entitlements and EntitlementsUnavailableError) records the
warning/metric before returning.
In `@src/backend/core/authentication/backends.py`:
- Around line 66-79: get_user_entitlements can raise
EntitlementsUnavailableError, but user.email is nullable so the current handler
may raise a TypeError when evaluating "@" in user.email; update the except block
in the login entitlements warm-up to guard user.email (e.g., check if user.email
is truthy before using "@" or .split, or use getattr/user.email or a conditional
to produce a safe fallback like "?" for email_domain) and then call
logger.warning("Entitlements unavailable for user@%s during login",
email_domain) with that safe email_domain; ensure changes are applied in the
same block that catches EntitlementsUnavailableError where get_user_entitlements
is invoked.
In `@src/backend/core/entitlements/backends/deploycenter.py`:
- Around line 44-45: _cache_key currently uses only user_sub which causes
collisions for email-only users and stale entries when account_email or
forwarded OIDC claims change; update the _cache_key implementation to
incorporate account_email and the forwarded OIDC claims (or their relevant
subset) alongside user_sub (and handle user_sub==None explicitly) into a
deterministic string (e.g., serialize a tuple of (user_sub, account_email,
sorted_claims) or compute a stable hash) so each unique identity/claim-set gets
a unique cache key; update the other cache key usages in the same file (the
similar logic around lines 96-103) to use the same composite-key function to
avoid leaking can_create decisions between users.
- Around line 114-119: Validate the incoming payload before reading can_create:
ensure data.get("entitlements") yields a dict and that
entitlements.get("can_create") is a boolean; if entitlements is None, not a
dict, or can_create is not a bool, try to read a previous value via
cache.get(cache_key) and use that fallback, otherwise raise
EntitlementsUnavailableError instead of proceeding; then construct result =
{"can_create": bool_value} and call cache.set(cache_key, result,
settings.ENTITLEMENTS_CACHE_TIMEOUT) as before. Ensure you reference the
existing variables/data structures entitlements, data, cache_key,
cache.set/cache.get, settings.ENTITLEMENTS_CACHE_TIMEOUT and
EntitlementsUnavailableError when implementing.
In `@src/backend/core/tests/test_entitlements.py`:
- Around line 149-289: Add unit tests in
src/backend/core/tests/test_entitlements.py exercising cache identity and
payload validation for DeployCenterEntitlementsBackend.get_user_entitlements:
(1) add a test (e.g., test_deploycenter_backend_cache_ignores_email_and_claims)
that calls get_user_entitlements twice with the same user_sub but different
email and/or oidc_claims and asserts the second call returns the cached
entitlements (no extra responses.calls) unless force_refresh=True; (2) add a
test (e.g., test_deploycenter_backend_user_sub_none_bypasses_cache_and_fails)
that passes user_sub=None to get_user_entitlements and asserts requests are not
cached and that an API failure raises EntitlementsUnavailableError; (3) add a
test (e.g., test_deploycenter_backend_handles_malformed_payload) that mocks a
response with {"entitlements": null} (or missing entitlements) and asserts
get_user_entitlements raises EntitlementsUnavailableError. Locate logic in
DeployCenterEntitlementsBackend.get_user_entitlements to ensure these behaviors
are covered by the tests.
- Around line 30-33: The autouse fixture _clear_cache currently clears the
Django cache but not the process-cached backend singleton; update this fixture
to also clear the entitlements backend cache by calling
get_entitlements_backend.cache_clear() (or, if the backend is a module-level
singleton, reset ENTITLEMENTS_BACKEND to None) so tests that change
ENTITLEMENTS_BACKEND won't leak state; locate the fixture and add the
cache-clear/reset call referencing get_entitlements_backend and
ENTITLEMENTS_BACKEND.
In `@src/backend/meet/settings.py`:
- Around line 715-719: ENTITLEMENTS_BACKEND_PARAMETERS currently permits putting
the DeployCenter API token in a plain DictValue; split secret-bearing params out
and stop passing secrets inside ENTITLEMENTS_BACKEND_PARAMETERS. Add a dedicated
secret-backed setting (e.g., ENTITLEMENTS_DEPLOYCENTER_API_KEY using
SecretFileValue) and update the code paths that construct or instantiate
DeployCenterEntitlementsBackend to read the bearer token from that new setting
instead of from ENTITLEMENTS_BACKEND_PARAMETERS["api_key"]; keep
ENTITLEMENTS_BACKEND_PARAMETERS for non-secret config only and ensure any
environment/environ_name usage matches the SecretFileValue convention for
production.
In `@src/frontend/src/features/home/routes/Home.tsx`:
- Around line 151-152: The code treats a missing user.can_create as false;
change the check in the Home component so an omitted can_create defaults to
allowed (server remains source of truth). Replace the strict equality check
(const canCreate = user?.can_create === true) with a negative check that only
denies create when can_create is explicitly false (e.g., useUser() result and
set canCreate using user?.can_create !== false or equivalent), keeping useUser
and the surrounding logic unchanged.
In `@src/frontend/src/features/sdk/routes/CreatePopup.tsx`:
- Around line 61-64: The effect watching isLoggedIn, canCreate, callbackId, and
createRoom can leave the popup stuck showing a spinner when isLoggedIn is true
but canCreate is false; update the logic in the effect around
createMeetingRoom/createRoom to explicitly handle the
authenticated-but-forbidden case by invoking the opener callback (using
callbackId) or returning an error result and closing the popup instead of
waiting: detect when isLoggedIn && !canCreate && callbackId and call the same
callback path used for errors (or dispatch the equivalent failure/forbidden
flow), ensuring createMeetingRoom is only called when canCreate is true and the
popup resolves in all branches.
---
Outside diff comments:
In `@src/backend/core/tests/test_api_users.py`:
- Around line 116-128: The test currently asserts a backend-driven entitlement
value (can_create) making it flaky; stub or override get_user_entitlements in
core.api.serializers for this test (or explicitly set the entitlements backend
in the test) so the /api/v1.0/users/me/ response is deterministic—e.g.,
monkeypatch core.api.serializers.get_user_entitlements to return a fixed
entitlement dict before calling client.get("/api/v1.0/users/me/") (or configure
the test app to use a known entitlements backend) and then assert the exact
expected serialized response.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 649d13d4-9eff-4a35-bd0f-7263ce6868bf
📒 Files selected for processing (20)
src/backend/core/api/permissions.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets.pysrc/backend/core/authentication/backends.pysrc/backend/core/entitlements/__init__.pysrc/backend/core/entitlements/backends/__init__.pysrc/backend/core/entitlements/backends/base.pysrc/backend/core/entitlements/backends/deploycenter.pysrc/backend/core/entitlements/backends/local.pysrc/backend/core/entitlements/factory.pysrc/backend/core/tests/test_api_users.pysrc/backend/core/tests/test_entitlements.pysrc/backend/meet/settings.pysrc/frontend/src/features/auth/api/ApiUser.tssrc/frontend/src/features/home/routes/Home.tsxsrc/frontend/src/features/sdk/routes/CreatePopup.tsxsrc/frontend/src/locales/de/home.jsonsrc/frontend/src/locales/en/home.jsonsrc/frontend/src/locales/fr/home.jsonsrc/frontend/src/locales/nl/home.json
Comment on lines
+61
to
+68
| if view.action == "create": | ||
| try: | ||
| entitlements = get_user_entitlements( | ||
| request.user.sub, request.user.email | ||
| ) | ||
| return entitlements.get("can_create", False) | ||
| except EntitlementsUnavailableError: | ||
| return False |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Emit telemetry when fail-closed blocks room creation.
This branch turns an entitlements outage into a normal False permission result, so operators get no signal that room creation is failing because the backend is unavailable. Please add at least a warning log and/or metric here.
Suggested change
+import logging
+
from rest_framework import permissions
from core.entitlements import EntitlementsUnavailableError, get_user_entitlements
+logger = logging.getLogger(__name__)
+
@@
if view.action == "create":
try:
entitlements = get_user_entitlements(
request.user.sub, request.user.email
)
return entitlements.get("can_create", False)
except EntitlementsUnavailableError:
+ logger.warning(
+ "entitlements_unavailable_during_room_create",
+ extra={"action": "create"},
+ )
return False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if view.action == "create": | |
| try: | |
| entitlements = get_user_entitlements( | |
| request.user.sub, request.user.email | |
| ) | |
| return entitlements.get("can_create", False) | |
| except EntitlementsUnavailableError: | |
| return False | |
| if view.action == "create": | |
| try: | |
| entitlements = get_user_entitlements( | |
| request.user.sub, request.user.email | |
| ) | |
| return entitlements.get("can_create", False) | |
| except EntitlementsUnavailableError: | |
| logger.warning( | |
| "entitlements_unavailable_during_room_create", | |
| extra={"action": "create"}, | |
| ) | |
| return False |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/api/permissions.py` around lines 61 - 68, The current
create-permission branch swallows EntitlementsUnavailableError and returns False
without telemetry; update the exception handler in the view.action == "create"
block that calls get_user_entitlements to emit an operator signal: log a warning
via the app logger (e.g., process_logger or the module logger) including
request.user.sub/email and the caught exception, and/or increment a metrics
counter (e.g., entitlements.unavailable or similar) so outages are visible; keep
the return False behavior but ensure the handler in permissions.py (around
get_user_entitlements and EntitlementsUnavailableError) records the
warning/metric before returning.
Comment on lines
+66
to
+79
| # Warm the entitlements cache on login (force_refresh) | ||
| try: | ||
| get_user_entitlements( | ||
| user_sub=user.sub, | ||
| user_email=user.email, | ||
| user_info=claims, | ||
| force_refresh=True, | ||
| ) | ||
| except EntitlementsUnavailableError: | ||
| email_domain = user.email.split("@")[-1] if "@" in user.email else "?" | ||
| logger.warning( | ||
| "Entitlements unavailable for user@%s during login", | ||
| email_domain, | ||
| ) |
There was a problem hiding this comment.
Guard the warning path against null emails.
core.models.User.email is nullable, so when entitlements are unavailable this handler can raise TypeError on @" in user.email and abort login instead of just warning.
🩹 Proposed fix
except EntitlementsUnavailableError:
- email_domain = user.email.split("@")[-1] if "@" in user.email else "?"
+ email_domain = email.split("@")[-1] if email and "@" in email else "?"
logger.warning(
"Entitlements unavailable for user@%s during login",
email_domain,
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/authentication/backends.py` around lines 66 - 79,
get_user_entitlements can raise EntitlementsUnavailableError, but user.email is
nullable so the current handler may raise a TypeError when evaluating "@" in
user.email; update the except block in the login entitlements warm-up to guard
user.email (e.g., check if user.email is truthy before using "@" or .split, or
use getattr/user.email or a conditional to produce a safe fallback like "?" for
email_domain) and then call logger.warning("Entitlements unavailable for user@%s
during login", email_domain) with that safe email_domain; ensure changes are
applied in the same block that catches EntitlementsUnavailableError where
get_user_entitlements is invoked.
Comment on lines
+44
to
+45
| def _cache_key(self, user_sub): | ||
| return f"entitlements:user:{user_sub}" |
There was a problem hiding this comment.
user_sub alone is not a safe cache key here.
The upstream lookup is driven by account_email plus forwarded OIDC claims, but the cache key ignores both. That returns stale entitlements after an email/claim change, and it collapses all email-only users onto entitlements:user:None, which can leak one user's can_create decision to another.
🔐 Proposed fix
+import hashlib
+import json
import logging
@@
- def _cache_key(self, user_sub):
- return f"entitlements:user:{user_sub}"
+ def _cache_key(self, user_sub, user_email, user_info=None):
+ forwarded_claims = {
+ claim: user_info.get(claim)
+ for claim in self.oidc_claims
+ if user_info and claim in user_info
+ }
+ identity = json.dumps(
+ {
+ "base_url": self.base_url,
+ "service_id": self.service_id,
+ "user_sub": user_sub,
+ "user_email": user_email,
+ "claims": forwarded_claims,
+ },
+ sort_keys=True,
+ default=str,
+ )
+ return f"entitlements:user:{hashlib.sha256(identity.encode()).hexdigest()}"
@@
- cache_key = self._cache_key(user_sub)
+ cache_key = self._cache_key(user_sub, user_email, user_info=user_info)Also applies to: 96-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/entitlements/backends/deploycenter.py` around lines 44 - 45,
_cache_key currently uses only user_sub which causes collisions for email-only
users and stale entries when account_email or forwarded OIDC claims change;
update the _cache_key implementation to incorporate account_email and the
forwarded OIDC claims (or their relevant subset) alongside user_sub (and handle
user_sub==None explicitly) into a deterministic string (e.g., serialize a tuple
of (user_sub, account_email, sorted_claims) or compute a stable hash) so each
unique identity/claim-set gets a unique cache key; update the other cache key
usages in the same file (the similar logic around lines 96-103) to use the same
composite-key function to avoid leaking can_create decisions between users.
Comment on lines
+114
to
+119
| entitlements = data.get("entitlements", {}) | ||
| result = { | ||
| "can_create": entitlements.get("can_create", False), | ||
| } | ||
|
|
||
| cache.set(cache_key, result, settings.ENTITLEMENTS_CACHE_TIMEOUT) |
There was a problem hiding this comment.
Validate the DeployCenter payload before reading can_create.
A list response, {"entitlements": null}, or a non-boolean can_create will either raise here or propagate a wrong value. This path sits on login and room-creation authorization, so malformed payloads should fall back to cache or raise EntitlementsUnavailableError instead of turning into a 500 or a truthy string.
🛡️ Proposed fix
- entitlements = data.get("entitlements", {})
- result = {
- "can_create": entitlements.get("can_create", False),
- }
+ if not isinstance(data, dict):
+ data = None
+
+ entitlements = data.get("entitlements", {}) if data else None
+ can_create = (
+ entitlements.get("can_create", False)
+ if isinstance(entitlements, dict)
+ else None
+ )
+ if not isinstance(can_create, bool):
+ cached = cache.get(cache_key)
+ if cached is not None:
+ return cached
+ raise EntitlementsUnavailableError(
+ "Invalid user entitlements payload from DeployCenter"
+ )
+
+ result = {"can_create": can_create}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| entitlements = data.get("entitlements", {}) | |
| result = { | |
| "can_create": entitlements.get("can_create", False), | |
| } | |
| cache.set(cache_key, result, settings.ENTITLEMENTS_CACHE_TIMEOUT) | |
| if not isinstance(data, dict): | |
| data = None | |
| entitlements = data.get("entitlements", {}) if data else None | |
| can_create = ( | |
| entitlements.get("can_create", False) | |
| if isinstance(entitlements, dict) | |
| else None | |
| ) | |
| if not isinstance(can_create, bool): | |
| cached = cache.get(cache_key) | |
| if cached is not None: | |
| return cached | |
| raise EntitlementsUnavailableError( | |
| "Invalid user entitlements payload from DeployCenter" | |
| ) | |
| result = {"can_create": can_create} | |
| cache.set(cache_key, result, settings.ENTITLEMENTS_CACHE_TIMEOUT) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/entitlements/backends/deploycenter.py` around lines 114 -
119, Validate the incoming payload before reading can_create: ensure
data.get("entitlements") yields a dict and that entitlements.get("can_create")
is a boolean; if entitlements is None, not a dict, or can_create is not a bool,
try to read a previous value via cache.get(cache_key) and use that fallback,
otherwise raise EntitlementsUnavailableError instead of proceeding; then
construct result = {"can_create": bool_value} and call cache.set(cache_key,
result, settings.ENTITLEMENTS_CACHE_TIMEOUT) as before. Ensure you reference the
existing variables/data structures entitlements, data, cache_key,
cache.set/cache.get, settings.ENTITLEMENTS_CACHE_TIMEOUT and
EntitlementsUnavailableError when implementing.
Comment on lines
+30
to
+33
| @pytest.fixture(autouse=True) | ||
| def _clear_cache(): | ||
| """Clear Django cache between tests to prevent entitlements cache bleed.""" | ||
| django_cache.clear() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clear the backend singleton in the autouse fixture too.
These tests reset Django's cache, but get_entitlements_backend() is also process-cached. Keeping cache_clear() in the shared fixture will prevent order-dependent failures when a test changes ENTITLEMENTS_BACKEND and forgets to reset the singleton.
♻️ Proposed fix
`@pytest.fixture`(autouse=True)
def _clear_cache():
- """Clear Django cache between tests to prevent entitlements cache bleed."""
- django_cache.clear()
+ """Clear Django and entitlements caches between tests."""
+ get_entitlements_backend.cache_clear()
+ django_cache.clear()
+ yield
+ django_cache.clear()
+ get_entitlements_backend.cache_clear()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @pytest.fixture(autouse=True) | |
| def _clear_cache(): | |
| """Clear Django cache between tests to prevent entitlements cache bleed.""" | |
| django_cache.clear() | |
| `@pytest.fixture`(autouse=True) | |
| def _clear_cache(): | |
| """Clear Django and entitlements caches between tests.""" | |
| get_entitlements_backend.cache_clear() | |
| django_cache.clear() | |
| yield | |
| django_cache.clear() | |
| get_entitlements_backend.cache_clear() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/tests/test_entitlements.py` around lines 30 - 33, The
autouse fixture _clear_cache currently clears the Django cache but not the
process-cached backend singleton; update this fixture to also clear the
entitlements backend cache by calling get_entitlements_backend.cache_clear()
(or, if the backend is a module-level singleton, reset ENTITLEMENTS_BACKEND to
None) so tests that change ENTITLEMENTS_BACKEND won't leak state; locate the
fixture and add the cache-clear/reset call referencing get_entitlements_backend
and ENTITLEMENTS_BACKEND.
Comment on lines
+149
to
+289
| @responses.activate | ||
| @override_settings(ENTITLEMENTS_CACHE_TIMEOUT=300) | ||
| def test_deploycenter_backend_uses_cache(): | ||
| """DeployCenter should use cached results when not force_refresh.""" | ||
| responses.add( | ||
| responses.GET, | ||
| DC_URL, | ||
| json={"entitlements": {"can_create": True}}, | ||
| status=200, | ||
| ) | ||
|
|
||
| backend = DeployCenterEntitlementsBackend( | ||
| base_url=DC_URL, | ||
| service_id="meet", | ||
| api_key="test-key", | ||
| ) | ||
|
|
||
| # First call hits the API | ||
| result1 = backend.get_user_entitlements("sub-123", "user@example.com") | ||
| assert result1 == {"can_create": True} | ||
| assert len(responses.calls) == 1 | ||
|
|
||
| # Second call should use cache | ||
| result2 = backend.get_user_entitlements("sub-123", "user@example.com") | ||
| assert result2 == {"can_create": True} | ||
| assert len(responses.calls) == 1 # No additional API call | ||
|
|
||
|
|
||
| @responses.activate | ||
| @override_settings(ENTITLEMENTS_CACHE_TIMEOUT=300) | ||
| def test_deploycenter_backend_force_refresh_bypasses_cache(): | ||
| """force_refresh=True should bypass cache and hit the API.""" | ||
| responses.add( | ||
| responses.GET, | ||
| DC_URL, | ||
| json={"entitlements": {"can_create": True}}, | ||
| status=200, | ||
| ) | ||
| responses.add( | ||
| responses.GET, | ||
| DC_URL, | ||
| json={"entitlements": {"can_create": False}}, | ||
| status=200, | ||
| ) | ||
|
|
||
| backend = DeployCenterEntitlementsBackend( | ||
| base_url=DC_URL, | ||
| service_id="meet", | ||
| api_key="test-key", | ||
| ) | ||
|
|
||
| result1 = backend.get_user_entitlements("sub-123", "user@example.com") | ||
| assert result1["can_create"] is True | ||
|
|
||
| result2 = backend.get_user_entitlements( | ||
| "sub-123", "user@example.com", force_refresh=True | ||
| ) | ||
| assert result2["can_create"] is False | ||
| assert len(responses.calls) == 2 | ||
|
|
||
|
|
||
| @responses.activate | ||
| @override_settings(ENTITLEMENTS_CACHE_TIMEOUT=300) | ||
| def test_deploycenter_backend_fallback_to_stale_cache(): | ||
| """When API fails, should return stale cached value if available.""" | ||
| responses.add( | ||
| responses.GET, | ||
| DC_URL, | ||
| json={"entitlements": {"can_create": True}}, | ||
| status=200, | ||
| ) | ||
|
|
||
| backend = DeployCenterEntitlementsBackend( | ||
| base_url=DC_URL, | ||
| service_id="meet", | ||
| api_key="test-key", | ||
| ) | ||
|
|
||
| # Populate cache | ||
| backend.get_user_entitlements("sub-123", "user@example.com") | ||
|
|
||
| # Now API fails | ||
| responses.replace( | ||
| responses.GET, | ||
| DC_URL, | ||
| body=requests.ConnectionError("Connection error"), | ||
| ) | ||
|
|
||
| # force_refresh to hit API, but should fall back to cache | ||
| result = backend.get_user_entitlements( | ||
| "sub-123", "user@example.com", force_refresh=True | ||
| ) | ||
| assert result == {"can_create": True} | ||
|
|
||
|
|
||
| @responses.activate | ||
| def test_deploycenter_backend_raises_when_no_cache(): | ||
| """When API fails and no cache exists, should raise.""" | ||
| responses.add( | ||
| responses.GET, | ||
| DC_URL, | ||
| body=requests.ConnectionError("Connection error"), | ||
| ) | ||
|
|
||
| backend = DeployCenterEntitlementsBackend( | ||
| base_url=DC_URL, | ||
| service_id="meet", | ||
| api_key="test-key", | ||
| ) | ||
|
|
||
| with pytest.raises(EntitlementsUnavailableError): | ||
| backend.get_user_entitlements("sub-123", "user@example.com") | ||
|
|
||
|
|
||
| @responses.activate | ||
| def test_deploycenter_backend_sends_oidc_claims(): | ||
| """DeployCenter should forward configured OIDC claims.""" | ||
| responses.add( | ||
| responses.GET, | ||
| DC_URL, | ||
| json={"entitlements": {"can_create": True}}, | ||
| status=200, | ||
| ) | ||
|
|
||
| backend = DeployCenterEntitlementsBackend( | ||
| base_url=DC_URL, | ||
| service_id="meet", | ||
| api_key="test-key", | ||
| oidc_claims=["organization"], | ||
| ) | ||
|
|
||
| backend.get_user_entitlements( | ||
| "sub-123", | ||
| "user@example.com", | ||
| user_info={"organization": "org-42", "other": "ignored"}, | ||
| ) | ||
|
|
||
| request = responses.calls[0].request | ||
| assert "organization=org-42" in request.url | ||
| assert "other" not in request.url | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Please cover the critical cache-identity and payload-validation cases.
The DeployCenter tests never exercise the same user_sub with changed email/claim inputs, user_sub=None, or malformed payloads such as {"entitlements": null}. Those are exactly the cases that currently break the backend, so adding them here would lock the fix in.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/tests/test_entitlements.py` around lines 149 - 289, Add unit
tests in src/backend/core/tests/test_entitlements.py exercising cache identity
and payload validation for
DeployCenterEntitlementsBackend.get_user_entitlements: (1) add a test (e.g.,
test_deploycenter_backend_cache_ignores_email_and_claims) that calls
get_user_entitlements twice with the same user_sub but different email and/or
oidc_claims and asserts the second call returns the cached entitlements (no
extra responses.calls) unless force_refresh=True; (2) add a test (e.g.,
test_deploycenter_backend_user_sub_none_bypasses_cache_and_fails) that passes
user_sub=None to get_user_entitlements and asserts requests are not cached and
that an API failure raises EntitlementsUnavailableError; (3) add a test (e.g.,
test_deploycenter_backend_handles_malformed_payload) that mocks a response with
{"entitlements": null} (or missing entitlements) and asserts
get_user_entitlements raises EntitlementsUnavailableError. Locate logic in
DeployCenterEntitlementsBackend.get_user_entitlements to ensure these behaviors
are covered by the tests.
Comment on lines
+715
to
+719
| ENTITLEMENTS_BACKEND_PARAMETERS = values.DictValue( | ||
| {}, | ||
| environ_name="ENTITLEMENTS_BACKEND_PARAMETERS", | ||
| environ_prefix=None, | ||
| ) |
There was a problem hiding this comment.
Don't put the DeployCenter API token inside ENTITLEMENTS_BACKEND_PARAMETERS.
DeployCenterEntitlementsBackend consumes api_key as a bearer secret, but this setting is a plain DictValue. That forces production deployments to carry the token in JSON config instead of using the SecretFileValue path used for the other credentials in this file. Please split secret-bearing parameters into dedicated secret-backed settings before enabling this backend in production.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/meet/settings.py` around lines 715 - 719,
ENTITLEMENTS_BACKEND_PARAMETERS currently permits putting the DeployCenter API
token in a plain DictValue; split secret-bearing params out and stop passing
secrets inside ENTITLEMENTS_BACKEND_PARAMETERS. Add a dedicated secret-backed
setting (e.g., ENTITLEMENTS_DEPLOYCENTER_API_KEY using SecretFileValue) and
update the code paths that construct or instantiate
DeployCenterEntitlementsBackend to read the bearer token from that new setting
instead of from ENTITLEMENTS_BACKEND_PARAMETERS["api_key"]; keep
ENTITLEMENTS_BACKEND_PARAMETERS for non-secret config only and ensure any
environment/environ_name usage matches the SecretFileValue convention for
production.
Comment on lines
+151
to
+152
| const { isLoggedIn, user } = useUser() | ||
| const canCreate = user?.can_create === true |
There was a problem hiding this comment.
Don't treat a missing can_create field as "no access".
src/frontend/src/features/auth/api/ApiUser.ts:3-11 makes can_create optional, and useUser returns that payload unchanged. With user?.can_create === true, any response that omits the field hides the create UI for logged-in users, which breaks the PR's “preserve current behavior” goal during mixed-version rollouts. Default undefined to allowed and keep the server-side permission as the source of truth.
Suggested fix
- const canCreate = user?.can_create === true
+ const canCreate = user?.can_create ?? true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const { isLoggedIn, user } = useUser() | |
| const canCreate = user?.can_create === true | |
| const { isLoggedIn, user } = useUser() | |
| const canCreate = user?.can_create ?? true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontend/src/features/home/routes/Home.tsx` around lines 151 - 152, The
code treats a missing user.can_create as false; change the check in the Home
component so an omitted can_create defaults to allowed (server remains source of
truth). Replace the strict equality check (const canCreate = user?.can_create
=== true) with a negative check that only denies create when can_create is
explicitly false (e.g., useUser() result and set canCreate using
user?.can_create !== false or equivalent), keeping useUser and the surrounding
logic unchanged.
Comment on lines
+61
to
+64
| if (isLoggedIn && canCreate && callbackId) { | ||
| createMeetingRoom() | ||
| } | ||
| }, [isLoggedIn, callbackId, createRoom]) | ||
| }, [isLoggedIn, canCreate, callbackId, createRoom]) |
There was a problem hiding this comment.
Handle the authenticated-but-forbidden popup path.
When isLoggedIn is true and canCreate is false, neither effect does anything and this route keeps rendering only the spinner. In the SDK flow that leaves the opener waiting indefinitely for a callback/result.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontend/src/features/sdk/routes/CreatePopup.tsx` around lines 61 - 64,
The effect watching isLoggedIn, canCreate, callbackId, and createRoom can leave
the popup stuck showing a spinner when isLoggedIn is true but canCreate is
false; update the logic in the effect around createMeetingRoom/createRoom to
explicitly handle the authenticated-but-forbidden case by invoking the opener
callback (using callbackId) or returning an error result and closing the popup
instead of waiting: detect when isLoggedIn && !canCreate && callbackId and call
the same callback path used for errors (or dispatch the equivalent
failure/forbidden flow), ensuring createMeetingRoom is only called when
canCreate is true and the popup resolves in all branches.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This follows implementations in Drive, Messages and Calendars. This system allows Meet to gate some features for users depending on an authorization server. We provide 2 backends: a local one that always allows room creation, mimicking the current behaviour, and a DeployCenter backend, that fetches a "can_create" flag from a remote API. Future deployment contexts might add new backends, or reuse the API format of the DeployCenter one.
Currenty being tested in ANCT's instance.