✨(entitlements) add DeployCenter backend for syncing maildomain admins#572
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a pluggable Entitlements system (service façade, backend interface, local and DeployCenter backends with caching/fallback), integrates entitlement sync into OIDC login to reconcile MailDomainAccess ADMIN records, adds a provisioning API (API-key protected), feature flags for maildomain create/manage-access, settings, frontend gating, docs, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant OIDC as OIDC Provider
participant Auth as OIDCAuthenticationBackend
participant EntSvc as Entitlements Service
participant EntBackend as Entitlements Backend
participant Cache as Cache
participant DB as Database
User->>OIDC: Login (ID token / claims)
OIDC->>Auth: Callback with claims / user_info
Auth->>Auth: get_or_create_user() (store user_info)
Auth->>Auth: post_get_or_create_user()
Auth->>EntSvc: get_user_entitlements(user_sub, email, user_info, force_refresh)
EntSvc->>EntBackend: get_user_entitlements(...)
EntBackend->>Cache: check per-user cache
alt cache hit
Cache-->>EntBackend: cached entitlements
else cache miss
EntBackend->>EntBackend: _make_request -> DeployCenter (HTTP)
EntBackend->>Cache: store result (rgba(0,128,0,0.5))
end
EntBackend-->>EntSvc: {can_access, can_admin_maildomains}
EntSvc-->>Auth: entitlements
Auth->>DB: query existing ADMIN MailDomainAccess entries
Auth->>DB: create missing ADMIN accesses
Auth->>DB: delete stale ADMIN accesses
Auth-->>User: login complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 4
🧹 Nitpick comments (1)
src/backend/core/entitlements/backends/deploycenter.py (1)
70-77: Consider adding Sentry error capture for API failures.The exception is logged but not reported to Sentry. Per the coding guidelines, exceptions should be captured and reported. This would help with monitoring API reliability in production.
♻️ Proposed enhancement
+from sentry_sdk import capture_exception + ... except (requests.RequestException, ValueError): email_domain = user_email.split("@")[-1] if "@" in user_email else "?" logger.warning( "DeployCenter entitlements request failed for user@%s", email_domain, exc_info=True, ) + capture_exception() return NoneAs per coding guidelines: "Capture and report exceptions to Sentry; use capture_exception() for custom errors".
🤖 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 70 - 77, In the except block handling requests.RequestException/ValueError inside the DeployCenter entitlements call, add a Sentry capture of the exception in addition to the logger.warning; call capture_exception(err) (imported from sentry_sdk) with the caught exception so failures are reported to Sentry, and keep the existing email_domain logging and return None behavior intact. Ensure capture_exception is imported where other Sentry usages live and invoke it before returning None.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/entitlements.md`:
- Around line 7-30: Add explicit language identifiers to the two fenced code
blocks: mark the ASCII diagram block (the box diagram starting with
"┌─────────────────────────────────────────────┐" and containing "OIDC
Authentication Backend") as ```text and mark the HTTP example block (the GET
{base_url}?service_id=... line) as ```http so markdown-lint stops flagging them;
update the opening fences accordingly for the ASCII diagram and the GET example.
- Around line 82-84: The ENTITLEMENTS_BACKEND_PARAMETERS example uses raw JSON
which can be misinterpreted by shells; update the example by wrapping the entire
JSON value in single quotes for the ENTITLEMENTS_BACKEND_PARAMETERS environment
variable so special characters ({ } [ ] ") are not expanded or split when
copy/pasted into a shell (refer to the ENTITLEMENTS_BACKEND_PARAMETERS example
line and ENTITLEMENTS_BACKEND symbol in the diff).
In `@src/backend/core/authentication/backends.py`:
- Around line 114-121: The code uses entitlements.get("can_admin_maildomains")
directly in MailDomain.objects.filter(name__in=admin_domains) which can
misbehave if admin_domains is not a list/iterable (e.g., a string or dict);
update the admin_domains handling in this block to validate and normalize the
type before using it: check that admin_domains is an instance of list/tuple/set
(or coerce to a list of strings) and if it is not a valid sequence, skip the
sync (return) or convert safely; apply this change to the variables
admin_domains, entitled_domains (MailDomain.objects.filter), and
entitled_domain_ids so only a validated list is passed to name__in.
- Around line 103-112: The EntitlementsUnavailableError is currently swallowed
and the warning logs user.sub (PII); update the except block around
get_user_entitlements to stop logging sensitive identifiers and instead call
capture_exception(EntitlementsUnavailableError) to report the error to Sentry
while retaining the fail-open return behavior—replace
logger.warning("Entitlements service unavailable during login for user %s",
user.sub) with a non-PII log (e.g., logger.warning("Entitlements service
unavailable during login")) and add capture_exception(e) (or capture_exception()
with the caught exception variable) so the exception is recorded for
observability; keep returning after handling the exception.
---
Nitpick comments:
In `@src/backend/core/entitlements/backends/deploycenter.py`:
- Around line 70-77: In the except block handling
requests.RequestException/ValueError inside the DeployCenter entitlements call,
add a Sentry capture of the exception in addition to the logger.warning; call
capture_exception(err) (imported from sentry_sdk) with the caught exception so
failures are reported to Sentry, and keep the existing email_domain logging and
return None behavior intact. Ensure capture_exception is imported where other
Sentry usages live and invoke it before returning None.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
docs/entitlements.mdsrc/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/entitlements/__init__.pysrc/backend/core/tests/entitlements/test_backends.pysrc/backend/core/tests/entitlements/test_oidc_sync.pysrc/backend/messages/settings.py
NathanVss
approved these changes
Mar 2, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/backend/core/authentication/backends.py (1)
139-155: Sync logic looks correct.The implementation handles the sync well:
- Early return optimization when already in sync
update_or_createcorrectly handles the unique constraint- Stale access cleanup is properly scoped to ADMIN role only
Consider wrapping lines 140-155 in
transaction.atomic()if strict consistency is desired, though the current fail-open behavior is reasonable for this use case.🤖 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 139 - 155, The sync block that adds new MailDomainAccess entries and deletes stale ones (references: entitled_domains, existing_domain_ids, stale_domain_ids, MailDomainAccess, MailDomainAccessRoleChoices, user) should be executed inside a single database transaction to ensure strict consistency: wrap the for-loop that calls MailDomainAccess.objects.update_or_create(...) and the subsequent MailDomainAccess.objects.filter(...).delete() in a transaction.atomic() context so either all inserts and deletes succeed together or none are applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/backend/core/authentication/backends.py`:
- Around line 139-155: The sync block that adds new MailDomainAccess entries and
deletes stale ones (references: entitled_domains, existing_domain_ids,
stale_domain_ids, MailDomainAccess, MailDomainAccessRoleChoices, user) should be
executed inside a single database transaction to ensure strict consistency: wrap
the for-loop that calls MailDomainAccess.objects.update_or_create(...) and the
subsequent MailDomainAccess.objects.filter(...).delete() in a
transaction.atomic() context so either all inserts and deletes succeed together
or none are applied.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/entitlements.mdsrc/backend/core/authentication/backends.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/entitlements.md
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+50
to
+54
| { | ||
| "created": created, | ||
| "existing": existing, | ||
| "errors": errors, | ||
| }, |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the problem, avoid returning the raw exception message to the client. Instead, either provide a generic error description or a very controlled, minimal message that does not expose stack traces, internal field names, or database details. Any detailed information from the exception should be logged server-side (using logger or Sentry) rather than sent in the HTTP response.
The best minimal-change fix here is to:
- Catch
ValidationErroras before, but:- Optionally log the exception (e.g.,
logger.warning(...)). - Append a safe, generic error message to
errorsfor the client, such as “Invalid domain.” or “Invalid domain name.” instead ofstr(e).
- Optionally log the exception (e.g.,
This preserves existing behavior in terms of which domains are recorded as errors and how the overall response is structured (created, existing, errors), while no longer exposing the raw exception text. All changes are in src/backend/core/api/viewsets/provisioning.py, within the except ValidationError as e: block. No new imports are strictly required, since logger is already defined; we can just add a logging call if desired.
Suggested changeset
1
src/backend/core/api/viewsets/provisioning.py
| @@ -51,7 +51,17 @@ | ||
| domain.save() | ||
| existing.append(domain_name) | ||
| except ValidationError as e: | ||
| errors.append({"domain": domain_name, "error": str(e)}) | ||
| logger.warning( | ||
| "ValidationError while provisioning domain %s: %s", | ||
| domain_name, | ||
| e, | ||
| ) | ||
| errors.append( | ||
| { | ||
| "domain": domain_name, | ||
| "error": "Invalid domain.", | ||
| } | ||
| ) | ||
| except IntegrityError as exc: | ||
| capture_exception(exc) | ||
| logger.exception( |
Copilot is powered by AI and may make mistakes. Always verify output.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/frontend/src/features/providers/config.tsx (1)
30-31: Consider fail-closed defaults for rollout flags.With
truedefaults, a config fetch failure enables these UI features by default. For rollout control, defaulting tofalseis safer.Suggested change
- FEATURE_MAILDOMAIN_CREATE: true, - FEATURE_MAILDOMAIN_MANAGE_ACCESSES: true, + FEATURE_MAILDOMAIN_CREATE: false, + FEATURE_MAILDOMAIN_MANAGE_ACCESSES: false,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/providers/config.tsx` around lines 30 - 31, The rollout flags FEATURE_MAILDOMAIN_CREATE and FEATURE_MAILDOMAIN_MANAGE_ACCESSES currently default to true; change their defaults to false so the UI fails closed on config fetch failures. Locate the config object (where FEATURE_MAILDOMAIN_CREATE and FEATURE_MAILDOMAIN_MANAGE_ACCESSES are defined) and set both flag values to false, keeping any existing runtime override or fetch logic intact so remote config can still enable them when available.src/frontend/src/pages/domain/index.tsx (1)
29-31: Avoid duplicating the same access-gating expression.The
hasManageAbility && isManageAccessesEnabledlogic appears in two places. Consider extracting a small helper/hook to keep this policy centralized.Also applies to: 101-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/pages/domain/index.tsx` around lines 29 - 31, Extract the duplicated gating logic into a small reusable hook or helper named e.g. useCanManageMaildomainAccesses that internally calls useAbility(Abilities.CAN_MANAGE_SOME_MAILDOMAIN_ACCESSES) and useFeatureFlag(FEATURE_KEYS.MAILDOMAIN_MANAGE_ACCESSES) and returns the combined boolean; then replace the inline expressions using hasManageAbility && isManageAccessesEnabled with a single call to useCanManageMaildomainAccesses (update places that currently compute canManageMaildomainAccesses and the other occurrence that repeats the same check). Ensure the hook exports a clear name and is used wherever that policy is needed so the policy stays centralized.
🤖 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/viewsets/provisioning.py`:
- Around line 46-47: The except block that appends errors currently uses str(e)
for both ValidationError and IntegrityError which may leak DB/internal details;
update the except handling in the provisioning viewset so ValidationError keeps
its user-facing message but IntegrityError appends a generic message (e.g., "An
internal error occurred while creating the domain" or "Conflict creating
domain") to the errors list for domain_name, and separately log the full
IntegrityError stack trace using the module logger (e.g., call logger.exception
or logger.error with e) so internal details are recorded but not returned to
callers; adjust the except branch that references ValidationError,
IntegrityError, errors, and domain_name accordingly.
In `@src/backend/core/tests/api/test_provisioning_maildomains.py`:
- Around line 51-59: The test test_provisioning_no_key_configured_returns_403
must explicitly clear the PROVISIONING_API_KEY so it doesn't rely on external
environment; update the test to unset or override PROVISIONING_API_KEY (e.g.,
using monkeypatch.setenv("PROVISIONING_API_KEY", "") or
monkeypatch.delenv("PROVISIONING_API_KEY", raising=False)) before calling
client.post, then restore or leave it cleared for the remainder of the test so
the 403 assertion is deterministic.
---
Nitpick comments:
In `@src/frontend/src/features/providers/config.tsx`:
- Around line 30-31: The rollout flags FEATURE_MAILDOMAIN_CREATE and
FEATURE_MAILDOMAIN_MANAGE_ACCESSES currently default to true; change their
defaults to false so the UI fails closed on config fetch failures. Locate the
config object (where FEATURE_MAILDOMAIN_CREATE and
FEATURE_MAILDOMAIN_MANAGE_ACCESSES are defined) and set both flag values to
false, keeping any existing runtime override or fetch logic intact so remote
config can still enable them when available.
In `@src/frontend/src/pages/domain/index.tsx`:
- Around line 29-31: Extract the duplicated gating logic into a small reusable
hook or helper named e.g. useCanManageMaildomainAccesses that internally calls
useAbility(Abilities.CAN_MANAGE_SOME_MAILDOMAIN_ACCESSES) and
useFeatureFlag(FEATURE_KEYS.MAILDOMAIN_MANAGE_ACCESSES) and returns the combined
boolean; then replace the inline expressions using hasManageAbility &&
isManageAccessesEnabled with a single call to useCanManageMaildomainAccesses
(update places that currently compute canManageMaildomainAccesses and the other
occurrence that repeats the same check). Ensure the hook exports a clear name
and is used wherever that policy is needed so the policy stays centralized.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/frontend/src/features/api/gen/models/config_retrieve200.tsis excluded by!**/gen/**
📒 Files selected for processing (16)
src/backend/core/api/permissions.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/config.pysrc/backend/core/api/viewsets/maildomain.pysrc/backend/core/api/viewsets/maildomain_access.pysrc/backend/core/api/viewsets/provisioning.pysrc/backend/core/tests/api/test_admin_maildomains_create.pysrc/backend/core/tests/api/test_config.pysrc/backend/core/tests/api/test_maildomain_access.pysrc/backend/core/tests/api/test_provisioning_maildomains.pysrc/backend/core/urls.pysrc/backend/messages/settings.pysrc/frontend/src/features/layouts/components/admin/domains-view/create-domain-action.tsxsrc/frontend/src/features/providers/config.tsxsrc/frontend/src/hooks/use-feature.tssrc/frontend/src/pages/domain/index.tsx
Comment on lines
+51
to
+59
| def test_provisioning_no_key_configured_returns_403(client, url): | ||
| """When PROVISIONING_API_KEY is not configured, returns 403.""" | ||
| response = client.post( | ||
| url, | ||
| data={"domains": ["test.fr"]}, | ||
| content_type="application/json", | ||
| HTTP_AUTHORIZATION="Bearer some-key", | ||
| ) | ||
| assert response.status_code == 403 |
There was a problem hiding this comment.
Make the “no key configured” case deterministic.
This test currently depends on whatever default PROVISIONING_API_KEY exists in the test environment. Explicitly clearing it inside the test will avoid environment-dependent failures.
💡 Suggested patch
`@pytest.mark.django_db`
-def test_provisioning_no_key_configured_returns_403(client, url):
+def test_provisioning_no_key_configured_returns_403(client, url, settings):
"""When PROVISIONING_API_KEY is not configured, returns 403."""
+ settings.PROVISIONING_API_KEY = ""
response = client.post(
url,
data={"domains": ["test.fr"]},
content_type="application/json",
HTTP_AUTHORIZATION="Bearer some-key",
)
assert response.status_code == 403🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/tests/api/test_provisioning_maildomains.py` around lines 51
- 59, The test test_provisioning_no_key_configured_returns_403 must explicitly
clear the PROVISIONING_API_KEY so it doesn't rely on external environment;
update the test to unset or override PROVISIONING_API_KEY (e.g., using
monkeypatch.setenv("PROVISIONING_API_KEY", "") or
monkeypatch.delenv("PROVISIONING_API_KEY", raising=False)) before calling
client.post, then restore or leave it cleared for the remainder of the test so
the 403 assertion is deterministic.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/viewsets/provisioning.py`:
- Around line 52-60: The except IntegrityError block currently only calls
logger.exception and appends to errors; modify it to also report the exception
to Sentry by calling sentry_sdk.capture_exception (or capture_exception with the
caught exception) in the same except block so failures are visible in Sentry;
ensure sentry_sdk is imported at module level if not present and keep the
existing logger.exception and errors.append calls (identify the change in the
except IntegrityError block that contains logger.exception("IntegrityError while
provisioning domain %s", domain_name) and errors.append({...})).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/backend/core/api/viewsets/provisioning.pysrc/backend/core/tests/api/test_provisioning_maildomains.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/backend/core/api/viewsets/provisioning.py (1)
54-63:⚠️ Potential issue | 🟡 MinorReport
IntegrityErrorto Sentry for production visibility.The
IntegrityErroris logged but not sent to Sentry. Per coding guidelines, exceptions should be captured and reported to Sentry. This improves visibility for unexpected provisioning failures in production monitoring.🛡️ Proposed fix to add Sentry capture
import logging from django.core.exceptions import ValidationError from django.db import IntegrityError +from sentry_sdk import capture_exception from drf_spectacular.utils import extend_schema- except IntegrityError: + except IntegrityError as exc: logger.exception( "IntegrityError while provisioning domain %s", domain_name ) + capture_exception(exc) errors.append( { "domain": domain_name, "error": "Failed to provision domain.", } )Based on learnings: "Capture and report exceptions to Sentry; use capture_exception() for custom errors"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/viewsets/provisioning.py` around lines 54 - 63, The IntegrityError exception handler in the provisioning flow currently logs via logger.exception and appends to errors but does not report to Sentry; update the except IntegrityError block (the handler that references logger.exception, domain_name and errors) to call Sentry's capture_exception (e.g., sentry_sdk.capture_exception) with the caught exception and include context (domain_name as a tag or extra) so the error is recorded in Sentry for production visibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/backend/core/api/viewsets/provisioning.py`:
- Around line 54-63: The IntegrityError exception handler in the provisioning
flow currently logs via logger.exception and appends to errors but does not
report to Sentry; update the except IntegrityError block (the handler that
references logger.exception, domain_name and errors) to call Sentry's
capture_exception (e.g., sentry_sdk.capture_exception) with the caught exception
and include context (domain_name as a tag or extra) so the error is recorded in
Sentry for production visibility.
jbpenrath
added a commit
that referenced
this pull request
Mar 5, 2026
Update all version files and changelog for minor release. Added - Store thread read state per thread access #575⚠️ This migration requires a search reindex to be run after the upgrade. - Store and display the user who sent a message #574 - Display selected threads count in right panel #576 - Add skip navigation link for keyboard users #573 - Add DeployCenter backend for syncing maildomain admins #572 - Add management command to print all users of the instance Changed - Bump keycloak to 26.5.4 #571 - Add migrations-check Makefile command Fixed - Preserve scroll position across renders #578 - Convert newlines to `<br>` in styled text #577 - Scope labels and user_role to the requested mailbox
Merged
jbpenrath
added a commit
that referenced
this pull request
Mar 5, 2026
Update all version files and changelog for minor release. Added - Store thread read state per thread access #575⚠️ This migration requires a search reindex to be run after the upgrade. - Store and display the user who sent a message #574 - Display selected threads count in right panel #576 - Add skip navigation link for keyboard users #573 - Add DeployCenter backend for syncing maildomain admins #572 - Add management command to print all users of the instance Changed - Bump keycloak to 26.5.4 #571 - Add migrations-check Makefile command Fixed - Preserve scroll position across renders #578 - Convert newlines to `<br>` in styled text #577 - Scope labels and user_role to the requested mailbox
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
3 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.
This is a lighter version than #519, focused on maildomain admin permissions only. The DeployCenter side is in prod.
Summary by CodeRabbit
New Features
Documentation
Tests