✨(entitlements) add Entitlements system with Deploy Center backend#519
Open
✨(entitlements) add Entitlements system with Deploy Center backend#519
Conversation
📝 WalkthroughWalkthroughThis PR introduces a pluggable entitlements system enabling access control and storage quota management. It includes a Django-backed caching layer, factory-based backend selection (Dummy for development, DeployCenter for production), REST API endpoint, OIDC login synchronization of mailbox access, and a React frontend quota widget with multi-language support. Changes
Sequence DiagramssequenceDiagram
participant User
participant OIDC as OIDC Backend
participant Ent as Entitlements Service
participant DC as DeployCenter API
participant DB as Database
User->>OIDC: Login
OIDC->>Ent: get_user_entitlements(force_refresh=true)
Ent->>DC: GET /api/v1.0/entitlements?account_type=user
DC-->>Ent: {can_access, can_admin_maildomains, operator}
Ent->>DB: Cache result
Ent-->>OIDC: Entitlements data
alt can_access = true
OIDC->>DB: _sync_entitlements(user)
OIDC->>DB: Create/Delete MailDomainAccess ADMIN records
DB-->>OIDC: Sync complete
OIDC-->>User: Login successful
else can_access = false
OIDC-->>User: SuspiciousOperation (403)
end
sequenceDiagram
participant FE as Frontend
participant Cache as React Query Cache
participant API as API Endpoint
participant Ent as Entitlements Service
participant Backend as Entitlements Backend
FE->>Cache: useEntitlements(mailboxId)?
alt Cache Hit
Cache-->>FE: Cached entitlements
else Cache Miss
Cache->>API: GET /api/v1.0/entitlements?mailbox_id=X
API->>Ent: get_mailbox_entitlements(mailbox_email)
Ent->>Ent: Check cache for mailbox
alt Cache Hit
Ent-->>API: Cached mailbox entitlements
else Cache Miss
Ent->>Backend: fetch mailbox entitlements
Backend-->>Ent: {max_storage, storage_used}
Ent->>Ent: Store in Django cache (300s)
Ent-->>API: Entitlements data
end
API-->>Cache: {can_access, can_admin_maildomains, mailbox}
Cache->>FE: Render QuotaWidget with storage bar
end
FE->>FE: Display storage: used / total with % fill
FE->>FE: Apply warning (80%) or critical (95%) styling
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~35 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 5
🤖 Fix all issues with AI agents
In `@src/backend/core/api/viewsets/entitlements.py`:
- Around line 51-62: Validate the incoming mailbox_id format and enforce access
before fetching entitlements: first check mailbox_id is a valid identifier
(return 400 for malformed ids), then try to load the Mailbox via
Mailbox.objects.select_related("domain").get(id=mailbox_id) and if it doesn't
exist return 404; after loading, verify the caller has access using the
MailboxAccess relationship (e.g.,
MailboxAccess.objects.filter(user=request.user, mailbox=mailbox).exists()) and
return 404/403 when access is missing; only then call
get_mailbox_entitlements(mailbox_email) and populate response_data["mailbox"]
(keep Mailbox.DoesNotExist handling to map to 404 and avoid raising 500).
In `@src/backend/core/authentication/backends.py`:
- Around line 121-136: The current loop uses MailDomainAccess.objects.create
which can raise IntegrityError if a non-admin record for (user, maildomain)
already exists; change the logic in the for domain in entitled_domains loop to
call MailDomainAccess.objects.update_or_create (keyed by user=user,
maildomain=domain) with defaults={'role': MailDomainAccessRoleChoices.ADMIN} so
existing non-admin accesses are upgraded safely and races are handled; keep
references to user, entitled_domains, MailDomainAccess, and
MailDomainAccessRoleChoices.ADMIN when making this change.
In `@src/backend/core/entitlements/__init__.py`:
- Around line 17-41: Wrap the backend call in get_user_entitlements (and the
other similar function around lines 44–66) in a try/except that catches
EntitlementsUnavailableError: call
get_entitlements_backend().get_user_entitlements(...) inside the try; on
exception, attempt to read the same cache_key via cache.get(cache_key) and
return cached value if present, otherwise re-raise the
EntitlementsUnavailableError; keep cache.set(cache_key, result,
settings.ENTITLEMENTS_CACHE_TIMEOUT) on success. This keeps force_refresh
semantics but provides the documented cache fallback and follows the guideline
to handle business-logic exceptions.
In `@src/backend/core/entitlements/backends/deploycenter.py`:
- Around line 22-53: The _make_request function logs raw account_id (PII) and
doesn't handle JSON decoding errors or report exceptions to Sentry; update
_make_request to avoid logging the raw email by redacting or hashing account_id
when composing log messages, catch ValueError from response.json() in the same
except block (or a separate except), include exception details in the
logger.warning call (without exposing the raw account_id) and call
sentry_sdk.capture_exception(e) for any caught exception; ensure the
Authorization header behavior and existing requests.RequestException handling
remain but add the ValueError handling and Sentry reporting while referencing
_make_request, response.json, requests.get, logger.warning, and
sentry_sdk.capture_exception to locate where to change.
In `@src/backend/core/tests/entitlements/test_middleware.py`:
- Around line 60-63: Remove the unused "response =" assignments in the tests
that call api_client.get(...) (e.g., the call preceding
mock_get.assert_not_called() and the other occurrences at the noted places);
either delete the "response =" portion and call api_client.get(...) directly or
replace "response" with "_" to discard the return value so Ruff no longer flags
the unused variable while leaving the test assertions like
mock_get.assert_not_called() intact.
🧹 Nitpick comments (9)
docs/entitlements.md (3)
7-26: Consider adding a language specifier to the fenced code block.The ASCII diagram should have a language specifier (e.g.,
textorplaintext) to satisfy markdownlint MD040.📝 Suggested fix
-``` +```text ┌─────────────────────────────────────────────┐
46-50: Table formatting can be improved for consistency.The table header row has compact pipe spacing that triggers markdownlint MD060. Adding spaces around the pipes improves readability.
📝 Suggested fix
-| Variable | Default | Description | -|---|---|---| +| Variable | Default | Description | +| -------- | ------- | ----------- |
98-100: Consider adding a language specifier to the API endpoint code block.Use
httportextas the language specifier to satisfy markdownlint MD040.📝 Suggested fix
-``` +```http GET {base_url}/api/v1.0/entitlements?service_id=X&account_type=X&account_id=Xsrc/backend/core/tests/entitlements/test_backends.py (1)
77-91: Remove unused variable assignment.The
resultvariable is assigned but never used. The test verifies request headers, so either remove the assignment or add an assertion on the result for completeness.♻️ Proposed fix
`@responses.activate` def test_get_user_entitlements_no_access_token(self): responses.add( responses.GET, "https://deploycenter.example.com/api/v1.0/entitlements", json={"can_access": True, "can_admin_maildomains": []}, status=200, ) backend = self._get_backend() - result = backend.get_user_entitlements("user-sub", "user@example.com") + backend.get_user_entitlements("user-sub", "user@example.com") request = responses.calls[0].request assert "Authorization" not in request.headerssrc/backend/core/tests/entitlements/test_api.py (1)
86-86: Remove extraneous f-string prefix.The f-string has no placeholders and should be a plain string.
♻️ Proposed fix
- mock_get_mailbox.assert_called_once_with(f"john@example.com") + mock_get_mailbox.assert_called_once_with("john@example.com")src/backend/core/authentication/backends.py (1)
101-110: Capture entitlements failures in Sentry for visibility.
[queue]Consider reporting the EntitlementsUnavailableError to Sentry (in addition to logging) to keep error rates observable during login flows.
🔧 Proposed fix
+from sentry_sdk import capture_exception @@ - except EntitlementsUnavailableError: + except EntitlementsUnavailableError as err: + capture_exception(err) logger.error( "Entitlements service unavailable during login for user %s", user.sub, ) returnAs per coding guidelines: Capture and report exceptions to Sentry; use capture_exception() for custom errors.
src/backend/core/api/viewsets/entitlements.py (1)
33-40: Log and report entitlements outages.The service-unavailable path returns 503 but drops observability. Please log it (using the existing logger) and capture the exception to Sentry.
🔧 Proposed fix
+from sentry_sdk import capture_exception @@ - except EntitlementsUnavailableError: + except EntitlementsUnavailableError as err: + capture_exception(err) + logger.warning("Entitlements service unavailable for user %s", request.user.sub) return Response( {"detail": "Entitlements service unavailable"}, status=503 ) @@ - except EntitlementsUnavailableError: + except EntitlementsUnavailableError as err: + capture_exception(err) + logger.warning("Entitlements service unavailable for user %s", request.user.sub) return Response( {"detail": "Entitlements service unavailable"}, status=503 )As per coding guidelines: Log expected and unexpected actions with appropriate log levels; Capture and report exceptions to Sentry; use capture_exception() for custom errors.
Also applies to: 63-66
src/backend/core/entitlements/middleware.py (1)
58-69: Report entitlements outages to Sentry.The warning log is good, but these outages should also be captured to Sentry for visibility.
🔧 Proposed fix
+from sentry_sdk import capture_exception @@ - except EntitlementsUnavailableError: + except EntitlementsUnavailableError as err: + capture_exception(err) logger.warning( "Entitlements service unavailable for user %s", request.user.sub, ) return JsonResponse( {"detail": "Entitlements service unavailable"}, status=503 )As per coding guidelines: Capture and report exceptions to Sentry; use capture_exception() for custom errors.
src/backend/core/entitlements/backends/base.py (1)
9-10: Drop the empty init (or mark it abstract).Ruff B027 flags empty init in abstract bases; removing it is cleaner unless you plan to enforce constructor requirements.
🔧 Proposed fix
- def __init__(self, **kwargs): - pass
Comment on lines
+51
to
+62
| mailbox_id = request.query_params.get("mailbox_id") | ||
| if mailbox_id: | ||
| try: | ||
| mailbox = Mailbox.objects.select_related("domain").get(id=mailbox_id) | ||
| mailbox_email = str(mailbox) | ||
| mailbox_data = get_mailbox_entitlements(mailbox_email) | ||
| response_data["mailbox"] = { | ||
| "max_storage": mailbox_data.get("max_storage"), | ||
| "storage_used": mailbox_data.get("storage_used"), | ||
| } | ||
| except Mailbox.DoesNotExist: | ||
| response_data["mailbox"] = None |
There was a problem hiding this comment.
Enforce mailbox access and validate mailbox_id.
Any authenticated user can query any mailbox id, and malformed ids can surface 500s. Gate mailbox lookups by user access (e.g., MailboxAccess) and handle invalid ids (400/404) before calling the backend.
🔧 Proposed fix
+from django.core.exceptions import ValidationError
@@
-from core.models import Mailbox
+from core.models import Mailbox, MailboxAccess
@@
- if mailbox_id:
- try:
- mailbox = Mailbox.objects.select_related("domain").get(id=mailbox_id)
- mailbox_email = str(mailbox)
- mailbox_data = get_mailbox_entitlements(mailbox_email)
- response_data["mailbox"] = {
- "max_storage": mailbox_data.get("max_storage"),
- "storage_used": mailbox_data.get("storage_used"),
- }
- except Mailbox.DoesNotExist:
- response_data["mailbox"] = None
- except EntitlementsUnavailableError:
- return Response(
- {"detail": "Entitlements service unavailable"}, status=503
- )
+ if mailbox_id:
+ try:
+ if not MailboxAccess.objects.filter(
+ user=request.user, mailbox_id=mailbox_id
+ ).exists():
+ return Response({"detail": "Mailbox not found"}, status=404)
+ mailbox = Mailbox.objects.select_related("domain").get(id=mailbox_id)
+ mailbox_email = str(mailbox)
+ mailbox_data = get_mailbox_entitlements(mailbox_email)
+ response_data["mailbox"] = {
+ "max_storage": mailbox_data.get("max_storage"),
+ "storage_used": mailbox_data.get("storage_used"),
+ }
+ except (Mailbox.DoesNotExist, ValidationError, ValueError):
+ return Response({"detail": "Invalid mailbox_id"}, status=400)
+ except EntitlementsUnavailableError:
+ return Response(
+ {"detail": "Entitlements service unavailable"}, status=503
+ )🤖 Prompt for AI Agents
In `@src/backend/core/api/viewsets/entitlements.py` around lines 51 - 62, Validate
the incoming mailbox_id format and enforce access before fetching entitlements:
first check mailbox_id is a valid identifier (return 400 for malformed ids),
then try to load the Mailbox via
Mailbox.objects.select_related("domain").get(id=mailbox_id) and if it doesn't
exist return 404; after loading, verify the caller has access using the
MailboxAccess relationship (e.g.,
MailboxAccess.objects.filter(user=request.user, mailbox=mailbox).exists()) and
return 404/403 when access is missing; only then call
get_mailbox_entitlements(mailbox_email) and populate response_data["mailbox"]
(keep Mailbox.DoesNotExist handling to map to 404 and avoid raising 500).
Comment on lines
+121
to
+136
| # Create missing MailDomainAccess ADMIN records | ||
| existing_accesses = MailDomainAccess.objects.filter( | ||
| user=user, role=MailDomainAccessRoleChoices.ADMIN | ||
| ) | ||
| existing_domain_ids = set( | ||
| existing_accesses.values_list("maildomain_id", flat=True) | ||
| ) | ||
|
|
||
| # Add new accesses | ||
| for domain in entitled_domains: | ||
| if domain.id not in existing_domain_ids: | ||
| MailDomainAccess.objects.create( | ||
| user=user, | ||
| maildomain=domain, | ||
| role=MailDomainAccessRoleChoices.ADMIN, | ||
| ) |
There was a problem hiding this comment.
Avoid IntegrityError when upgrading admin access.
If the user already has a MailDomainAccess for the domain with a non-admin role, .create will violate the (maildomain, user) uniqueness constraint and can fail login. Use update_or_create (or get_or_create + role update) to safely upgrade roles and avoid races.
🔧 Proposed fix
- for domain in entitled_domains:
- if domain.id not in existing_domain_ids:
- MailDomainAccess.objects.create(
- user=user,
- maildomain=domain,
- role=MailDomainAccessRoleChoices.ADMIN,
- )
+ for domain in entitled_domains:
+ MailDomainAccess.objects.update_or_create(
+ user=user,
+ maildomain=domain,
+ defaults={"role": MailDomainAccessRoleChoices.ADMIN},
+ )📝 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
| # Create missing MailDomainAccess ADMIN records | |
| existing_accesses = MailDomainAccess.objects.filter( | |
| user=user, role=MailDomainAccessRoleChoices.ADMIN | |
| ) | |
| existing_domain_ids = set( | |
| existing_accesses.values_list("maildomain_id", flat=True) | |
| ) | |
| # Add new accesses | |
| for domain in entitled_domains: | |
| if domain.id not in existing_domain_ids: | |
| MailDomainAccess.objects.create( | |
| user=user, | |
| maildomain=domain, | |
| role=MailDomainAccessRoleChoices.ADMIN, | |
| ) | |
| # Create missing MailDomainAccess ADMIN records | |
| existing_accesses = MailDomainAccess.objects.filter( | |
| user=user, role=MailDomainAccessRoleChoices.ADMIN | |
| ) | |
| existing_domain_ids = set( | |
| existing_accesses.values_list("maildomain_id", flat=True) | |
| ) | |
| # Add new accesses | |
| for domain in entitled_domains: | |
| MailDomainAccess.objects.update_or_create( | |
| user=user, | |
| maildomain=domain, | |
| defaults={"role": MailDomainAccessRoleChoices.ADMIN}, | |
| ) |
🤖 Prompt for AI Agents
In `@src/backend/core/authentication/backends.py` around lines 121 - 136, The
current loop uses MailDomainAccess.objects.create which can raise IntegrityError
if a non-admin record for (user, maildomain) already exists; change the logic in
the for domain in entitled_domains loop to call
MailDomainAccess.objects.update_or_create (keyed by user=user,
maildomain=domain) with defaults={'role': MailDomainAccessRoleChoices.ADMIN} so
existing non-admin accesses are upgraded safely and races are handled; keep
references to user, entitled_domains, MailDomainAccess, and
MailDomainAccessRoleChoices.ADMIN when making this change.
Comment on lines
+17
to
+41
| def get_user_entitlements( | ||
| user_sub, user_email, access_token=None, force_refresh=False | ||
| ): | ||
| """Get user entitlements, using Django cache. | ||
|
|
||
| Returns: | ||
| dict: {"can_access": bool, "can_admin_maildomains": [str], "operator": dict|None} | ||
|
|
||
| Raises: | ||
| EntitlementsUnavailableError: If the backend cannot be reached and no cache exists. | ||
| """ | ||
| cache_key = f"entitlements:user:{user_sub}" | ||
|
|
||
| if not force_refresh: | ||
| cached = cache.get(cache_key) | ||
| if cached is not None: | ||
| return cached | ||
|
|
||
| backend = get_entitlements_backend() | ||
| result = backend.get_user_entitlements( | ||
| user_sub, user_email, access_token=access_token | ||
| ) | ||
|
|
||
| cache.set(cache_key, result, settings.ENTITLEMENTS_CACHE_TIMEOUT) | ||
| return result |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/backend/core/entitlements/__init__.py | head -80Repository: suitenumerique/messages
Length of output: 2418
Resolve docstring-behavior mismatch: add cache fallback on backend failure.
The docstring states EntitlementsUnavailableError is raised only when the backend cannot be reached and no cache exists. However, when force_refresh=True, the code bypasses the cache check and raises the exception immediately if the backend fails, even if cached data is available. Additionally, the coding guidelines require try-except blocks for business logic exception handling.
Either update the docstring to reflect this behavior, or implement cache fallback on backend failure as suggested below. Both functions (lines 17–41 and 44–66) have this issue.
💡 Suggested implementation
def get_user_entitlements(
user_sub, user_email, access_token=None, force_refresh=False
):
- if not force_refresh:
- cached = cache.get(cache_key)
- if cached is not None:
- return cached
+ cached = None
+ if not force_refresh:
+ cached = cache.get(cache_key)
+ if cached is not None:
+ return cached
+
+ try:
+ result = backend.get_user_entitlements(
+ user_sub, user_email, access_token=access_token
+ )
+ except EntitlementsUnavailableError:
+ if cached is not None:
+ return cached
+ raise🤖 Prompt for AI Agents
In `@src/backend/core/entitlements/__init__.py` around lines 17 - 41, Wrap the
backend call in get_user_entitlements (and the other similar function around
lines 44–66) in a try/except that catches EntitlementsUnavailableError: call
get_entitlements_backend().get_user_entitlements(...) inside the try; on
exception, attempt to read the same cache_key via cache.get(cache_key) and
return cached value if present, otherwise re-raise the
EntitlementsUnavailableError; keep cache.set(cache_key, result,
settings.ENTITLEMENTS_CACHE_TIMEOUT) on success. This keeps force_refresh
semantics but provides the documented cache fallback and follows the guideline
to handle business-logic exceptions.
Comment on lines
+22
to
+53
| def _make_request(self, account_type, account_id, access_token=None): | ||
| """Make a request to the DeployCenter entitlements API. | ||
|
|
||
| Returns: | ||
| dict | None: The response data, or None on failure. | ||
| """ | ||
| url = f"{self.base_url}/api/v1.0/entitlements" | ||
| params = { | ||
| "service_id": self.service_id, | ||
| "account_type": account_type, | ||
| "account_id": account_id, | ||
| } | ||
| headers = { | ||
| "X-Service-Auth": f"ApiKey {self.api_key}", | ||
| } | ||
| if access_token: | ||
| headers["Authorization"] = f"Bearer {access_token}" | ||
|
|
||
| try: | ||
| response = requests.get( | ||
| url, params=params, headers=headers, timeout=self.timeout | ||
| ) | ||
| response.raise_for_status() | ||
| return response.json() | ||
| except requests.RequestException: | ||
| logger.warning( | ||
| "DeployCenter entitlements request failed for %s/%s", | ||
| account_type, | ||
| account_id, | ||
| exc_info=True, | ||
| ) | ||
| return None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd src/backend && find . -name "deploycenter.py" -type fRepository: suitenumerique/messages
Length of output: 111
🏁 Script executed:
cat -n src/backend/core/entitlements/backends/deploycenter.pyRepository: suitenumerique/messages
Length of output: 3817
🏁 Script executed:
# Check if sentry_sdk is imported elsewhere in the backend
rg "from sentry_sdk import" src/backend/ -A 2 | head -40Repository: suitenumerique/messages
Length of output: 49
🏁 Script executed:
# Check what account_id type/usage is in the codebase
rg "account_id" src/backend/core/entitlements/ -B 2 -A 2 | head -60Repository: suitenumerique/messages
Length of output: 1394
🏁 Script executed:
# Broader search for sentry_sdk usage in the entire project
rg "sentry_sdk|capture_exception" . --include="*.py" | head -30Repository: suitenumerique/messages
Length of output: 508
🏁 Script executed:
# Check the base class to understand entitlements backend pattern
cat -n src/backend/core/entitlements/backends/base.py | head -50Repository: suitenumerique/messages
Length of output: 1394
🏁 Script executed:
# Fix rg syntax - search for sentry usage
rg "sentry_sdk" . -t py | head -20Repository: suitenumerique/messages
Length of output: 371
🏁 Script executed:
# Also search for capture_exception pattern
rg "capture_exception" . -t py | head -20Repository: suitenumerique/messages
Length of output: 49
🏁 Script executed:
# Check if there are other backends that might show the pattern
fd "backends" src/backend --type dRepository: suitenumerique/messages
Length of output: 106
🏁 Script executed:
# Look for exception handling patterns in other backend files
find src/backend -name "*.py" -path "*/backends/*" -type fRepository: suitenumerique/messages
Length of output: 267
🏁 Script executed:
# Check other backend implementations
cat -n src/backend/core/entitlements/backends/dummy.pyRepository: suitenumerique/messages
Length of output: 840
🏁 Script executed:
# Search for logger.warning patterns in backend to see if PII is logged elsewhere
rg "logger\.warning\|logger\.error\|logger\.info" src/backend/core/entitlements/ -B 2 -A 2Repository: suitenumerique/messages
Length of output: 49
🏁 Script executed:
# Check if there's any logging of emails/user identifiers in the backend
rg "user_email\|mailbox_email" src/backend/ -B 2 -A 2 | head -40Repository: suitenumerique/messages
Length of output: 49
🏁 Script executed:
# Check if there are any other logging of user/email data in the backend
rg "logger\." src/backend/ -B 1 -A 1 | head -60Repository: suitenumerique/messages
Length of output: 3839
🏁 Script executed:
# Search for any documentation or tests that might show usage patterns
find src/backend -name "*test*" -o -name "*spec*" | grep entitlementRepository: suitenumerique/messages
Length of output: 326
🏁 Script executed:
# Check the imports and patterns in settings to understand sentry setup
cat -n src/backend/messages/settings.py | grep -A 10 "sentry_sdk"Repository: suitenumerique/messages
Length of output: 1245
Avoid logging raw account identifiers and harden error handling.
account_id is an email (PII), and response.json() can raise ValueError which currently bubbles out. Exceptions should also be reported to Sentry per guidelines.
🛡️ Suggested fix
-import logging
+import hashlib
+import logging
import requests
+from sentry_sdk import capture_exception
@@
- try:
- response = requests.get(
- url, params=params, headers=headers, timeout=self.timeout
- )
- response.raise_for_status()
- return response.json()
- except requests.RequestException:
- logger.warning(
- "DeployCenter entitlements request failed for %s/%s",
- account_type,
- account_id,
- exc_info=True,
- )
- return None
+ try:
+ response = requests.get(
+ url, params=params, headers=headers, timeout=self.timeout
+ )
+ response.raise_for_status()
+ return response.json()
+ except (requests.RequestException, ValueError) as exc:
+ safe_account_id = hashlib.sha256(
+ account_id.encode()
+ ).hexdigest()[:8]
+ logger.warning(
+ "DeployCenter entitlements request failed for %s/%s",
+ account_type,
+ safe_account_id,
+ exc_info=True,
+ )
+ capture_exception(exc)
+ return None🤖 Prompt for AI Agents
In `@src/backend/core/entitlements/backends/deploycenter.py` around lines 22 - 53,
The _make_request function logs raw account_id (PII) and doesn't handle JSON
decoding errors or report exceptions to Sentry; update _make_request to avoid
logging the raw email by redacting or hashing account_id when composing log
messages, catch ValueError from response.json() in the same except block (or a
separate except), include exception details in the logger.warning call (without
exposing the raw account_id) and call sentry_sdk.capture_exception(e) for any
caught exception; ensure the Authorization header behavior and existing
requests.RequestException handling remain but add the ValueError handling and
Sentry reporting while referencing _make_request, response.json, requests.get,
logger.warning, and sentry_sdk.capture_exception to locate where to change.
NathanVss
reviewed
Feb 9, 2026
| user_sub, user_email, access_token=access_token | ||
| ) | ||
|
|
||
| cache.set(cache_key, result, settings.ENTITLEMENTS_CACHE_TIMEOUT) |
There was a problem hiding this comment.
Maybe prefer referring a ENTITLEMENTS_BACKEND_PARAMETERS dict variable to store all the options?
https://github.com/suitenumerique/drive/blob/0c7c381eb74ed409cfb11935dbc2acdf446fbb34/src/backend/drive/settings.py#L1457-L1461
NathanVss
approved these changes
Feb 9, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/entitlements.md (2)
7-26: Add language identifiers to fenced code blocks.The architecture diagram (line 7) and the API endpoint example (line 98) lack language specifiers. Use
```textfor both to satisfy markdownlint MD040 and improve rendering.📝 Proposed fix
Line 7:
-``` +```text ┌─────────────────────────────────────────────┐Line 98:
-``` +```text GET {base_url}/api/v1.0/entitlements?service_id=X&account_type=X&account_id=X🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/entitlements.md` around lines 7 - 26, The two fenced code blocks containing the ASCII architecture diagram and the API endpoint example should include a language identifier to satisfy markdownlint MD040; update the fence that wraps the ASCII diagram (the block starting with the boxed "Django Cache Layer" diagram) and the block with the GET example ("GET {base_url}/api/v1.0/entitlements?...") to use ```text as the opening fence so both blocks become ```text ... ```; this is a simple edit to the fenced block markers in entitlements.md around the diagram and the API example.
132-134: Good deployment warning — consider making it more prominent.The note about existing admin accesses being removed if not present in the DeployCenter response is critical for production rollout. Consider wrapping it in a Markdown admonition (e.g.,
> ⚠️ **Warning**: ...) to increase visibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/entitlements.md` around lines 132 - 134, The deployment note under the "### Deployment Consideration" heading about entitlements sync removing admin accesses should be made more prominent: replace the plain paragraph that contains "The entitlements sync will **remove** admin accesses that are not in the DeployCenter response." with a Markdown warning admonition (e.g., start the paragraph with "> ⚠️ **Warning**:" and keep the same wording/terminology) so the critical removal behavior is visually emphasized for readers planning production rollouts.
🤖 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/tests/entitlements/test_backends.py`:
- Around line 80-93: The test function
test_get_user_entitlements_no_access_token assigns result =
backend.get_user_entitlements("user-sub", "user@example.com") but never uses it;
either remove the unused assignment or add an assertion about the returned
value. Update the test by deleting the result = ... line if the return value is
irrelevant, or replace it with an assertion (e.g., assert result is None or
assert result == <expected>) referencing the get_user_entitlements call to
ensure the variable is used.
---
Duplicate comments:
In `@src/backend/core/authentication/backends.py`:
- Around line 143-158: The current loop creates MailDomainAccess rows and can
raise IntegrityError because existing_accesses only included ADMIN roles;
replace the MailDomainAccess.objects.create(...) call in the loop over
entitled_domains with
MailDomainAccess.objects.update_or_create(defaults={"role":
MailDomainAccessRoleChoices.ADMIN}, user=user, maildomain=domain) so the (user,
maildomain) unique constraint is handled safely and existing non-ADMIN rows are
upgraded to ADMIN; keep the surrounding variables (user, entitled_domains,
existing_domain_ids) as needed or remove the pre-check if you rely on
update_or_create for idempotency.
In `@src/backend/core/entitlements/backends/deploycenter.py`:
- Around line 40-53: The current try/except around requests.get handles
requests.RequestException but not JSON decoding errors from response.json(), and
it logs PII (account_id); update the error handling in the block that calls
response.json() so it also catches JSONDecodeError
(requests.exceptions.JSONDecodeError or json.JSONDecodeError) and converts it
into the same EntitlementsUnavailableError path (or returns None consistent with
existing behavior) while including exc_info in the log; remove account_id from
the logger.warning call (log only account_type or a masked identifier) so PII is
not emitted, and ensure logger.warning still provides context and exception info
when response.json() fails.
---
Nitpick comments:
In `@docs/entitlements.md`:
- Around line 7-26: The two fenced code blocks containing the ASCII architecture
diagram and the API endpoint example should include a language identifier to
satisfy markdownlint MD040; update the fence that wraps the ASCII diagram (the
block starting with the boxed "Django Cache Layer" diagram) and the block with
the GET example ("GET {base_url}/api/v1.0/entitlements?...") to use ```text as
the opening fence so both blocks become ```text ... ```; this is a simple edit
to the fenced block markers in entitlements.md around the diagram and the API
example.
- Around line 132-134: The deployment note under the "### Deployment
Consideration" heading about entitlements sync removing admin accesses should be
made more prominent: replace the plain paragraph that contains "The entitlements
sync will **remove** admin accesses that are not in the DeployCenter response."
with a Markdown warning admonition (e.g., start the paragraph with "> ⚠️
**Warning**:" and keep the same wording/terminology) so the critical removal
behavior is visually emphasized for readers planning production rollouts.
Comment on lines
+80
to
+93
| @responses.activate | ||
| def test_get_user_entitlements_no_access_token(self): | ||
| responses.add( | ||
| responses.GET, | ||
| "https://deploycenter.example.com/api/v1.0/entitlements", | ||
| json={"entitlements": {"can_access": True, "can_admin_maildomains": []}}, | ||
| status=200, | ||
| ) | ||
|
|
||
| backend = self._get_backend() | ||
| result = backend.get_user_entitlements("user-sub", "user@example.com") | ||
|
|
||
| request = responses.calls[0].request | ||
| assert "Authorization" not in request.headers |
There was a problem hiding this comment.
Unused variable result on line 90.
The result variable is assigned but never used. Either remove the assignment or add an assertion on the result.
🔧 Proposed fix
backend = self._get_backend()
- result = backend.get_user_entitlements("user-sub", "user@example.com")
+ backend.get_user_entitlements("user-sub", "user@example.com")📝 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
| @responses.activate | |
| def test_get_user_entitlements_no_access_token(self): | |
| responses.add( | |
| responses.GET, | |
| "https://deploycenter.example.com/api/v1.0/entitlements", | |
| json={"entitlements": {"can_access": True, "can_admin_maildomains": []}}, | |
| status=200, | |
| ) | |
| backend = self._get_backend() | |
| result = backend.get_user_entitlements("user-sub", "user@example.com") | |
| request = responses.calls[0].request | |
| assert "Authorization" not in request.headers | |
| `@responses.activate` | |
| def test_get_user_entitlements_no_access_token(self): | |
| responses.add( | |
| responses.GET, | |
| "https://deploycenter.example.com/api/v1.0/entitlements", | |
| json={"entitlements": {"can_access": True, "can_admin_maildomains": []}}, | |
| status=200, | |
| ) | |
| backend = self._get_backend() | |
| backend.get_user_entitlements("user-sub", "user@example.com") | |
| request = responses.calls[0].request | |
| assert "Authorization" not in request.headers |
🧰 Tools
🪛 Ruff (0.15.1)
[error] 90-90: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/tests/entitlements/test_backends.py` around lines 80 - 93,
The test function test_get_user_entitlements_no_access_token assigns result =
backend.get_user_entitlements("user-sub", "user@example.com") but never uses it;
either remove the unused assignment or add an assertion about the returned
value. Update the test by deleting the result = ... line if the return value is
irrelevant, or replace it with an assertion (e.g., assert result is None or
assert result == <expected>) referencing the get_user_entitlements call to
ensure the variable is used.
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
2 participants
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.
First rough pass, feedbacks welcomed
Summary by CodeRabbit
Release Notes
New Features
Documentation
Localization