Feature/eo api entitlements - #8
Open
slegouffe wants to merge 11 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds configurable entitlements backends, OIDC claim persistence, authentication and draft-upload authorization, a protected entitlements API, development claim support, and a frontend access-denied page with supporting configuration and tests. ChangesEntitlements Access Control
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant OIDCProvider
participant OIDCAuthenticationBackend
participant EntitlementsBackend
participant Session
participant OIDCAuthenticationCallbackView
participant ErrorsPage
OIDCProvider->>OIDCAuthenticationBackend: submit authentication callback
OIDCAuthenticationBackend->>EntitlementsBackend: can_access(user)
EntitlementsBackend-->>OIDCAuthenticationBackend: denied entitlement result
OIDCAuthenticationBackend->>Session: store access-denied flag
OIDCAuthenticationCallbackView->>Session: read and remove flag
OIDCAuthenticationCallbackView->>ErrorsPage: redirect to /errors/
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
…ntitlement is also call in get_or_create_user()
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker/auth/realm.json`:
- Around line 750-752: The client entry with clientId "deploycenter" has an
invalid UUID value for "id" (contains a non-hex character 'g'); replace the "id"
string with a valid RFC 4122 v4 UUID (e.g., regenerate a new UUID) ensuring all
characters are 0-9 or a-f and the standard hyphenated format, then validate the
JSON to ensure Keycloak will accept the realm import for the deploycenter
client.
- Around line 1417-1431: The "email sub" mapper is overriding the OIDC subject
by writing the user's email into claim "sub"; remove or change that mapper so
the OIDC "sub" stays Keycloak's stable subject. Locate the mapper named "email
sub" (protocolMapper "oidc-usermodel-property-mapper") and either delete it or
update its config so "claim.name" is "email" (or another non-"sub" claim) while
keeping "user.attribute": "email"; ensure no custom mapper writes to "sub" so
the backend continues to receive a stable subject from Keycloak's built-in
Subject/pairwise mapper.
In `@src/backend/core/api/permissions.py`:
- Around line 39-45: The permission check in has_permission currently only
protects actions in _UPLOAD_ACTIONS (frozenset of
"add_file","sign_part","complete_upload") so the "finalize" action bypasses
enforce_upload_entitlement; update _UPLOAD_ACTIONS to include "finalize" (or
expand the set to cover all mutating draft actions) so that has_permission calls
enforce_upload_entitlement for finalize as well, ensuring
enforce_upload_entitlement(request.user) is invoked before returning True.
In `@src/backend/core/api/viewsets/entitlements.py`:
- Around line 17-22: The current loop uses dir(entitlements_backend) and
auto-invokes any callable starting with "can_", which is fragile; change to an
explicit export contract on the backend (e.g., a list attribute like
EXPORTED_ENTITLEMENTS or a method get_entitlement_checks()) and iterate that
instead. Update the logic in entitlements.py to retrieve the declared names from
entitlements_backend (e.g., entitlements_backend.EXPORTED_ENTITLEMENTS or call
entitlements_backend.get_entitlement_checks()) and for each declared name call
the corresponding callable with only the user (method =
getattr(entitlements_backend, name); entitlements[name] = method(request.user)),
preserving entitlements["context"] =
entitlements_backend.get_context(request.user); also validate existence and
callability of each declared symbol and raise a clear error if missing or
signature is incorrect.
In `@src/backend/core/authentication/backends.py`:
- Around line 71-77: claims_to_store currently includes keys with value None
because user_info.get(claim) returns None for missing claims; change the
comprehension in the claims_to_store assignment to only include claim: value
pairs where value is not None (e.g., iterate over getattr(settings,
"OIDC_STORE_CLAIMS", []) and include claim if (value := user_info.get(claim)) is
not None) so the stored "claims" dict excludes null entries; keep the
surrounding return structure (including compute_full_name(user_info)) unchanged.
In `@src/backend/core/authentication/dev_claims_middleware.py`:
- Around line 22-31: The middleware's check using hasattr(user, "claims") is
ineffective because User now defines a claims attribute; update the __call__
logic to detect empty or missing claims instead: retrieve existing_claims =
getattr(user, "claims", None) and if not existing_claims (e.g., None or empty
dict/falsey) then load claims from request.session using _DEV_CLAIMS_SESSION_KEY
and set via setattr(user, "claims", claims_or_empty_dict); keep the
isinstance(claims, dict) guard and return self.get_response(request) as before
so get_response remains unchanged.
In `@src/backend/core/entitlements/backends/deploycenter.py`:
- Around line 105-109: The try/except around fetch_entitlements currently
catches requests.RequestException and re-raises it directly, which bypasses our
app-level EntitlementsUnavailableError; update the except block in
get_entitlements (the block calling self.fetch_entitlements(user)) to log the
failure (preserve logger.exception call) and then raise
EntitlementsUnavailableError from the caught RequestException so the higher
layers get the controlled unavailability error instead of a raw requests
exception. Ensure you reference the original exception as the __cause__ (use
"raise EntitlementsUnavailableError(...) from e") and keep the same context
(include user.id in the log).
- Around line 53-58: fetch_entitlements is sending account_type="email" but
tests and the docstring expect "user"—change the params in
DeployCenterEntitlementsBackend.fetch_entitlements to use "account_type": "user"
(keep account_id and account_email as-is); additionally, instead of letting
requests.RequestException bubble, wrap the call to get_entitlements (or inside
get_entitlements) to catch requests.RequestException, log the original
exception, and raise EntitlementsUnavailableError (preserving context/message)
so upstream core.api.exception_handler will normalize DeployCenter outages;
reference DeployCenterEntitlementsBackend.fetch_entitlements, get_entitlements,
and EntitlementsUnavailableError when making the changes.
In `@src/backend/core/entitlements/backends/static.py`:
- Around line 9-15: The can_access method can raise KeyError if the provided
entitlements dict lacks "can_access"; update StaticEntitlementsBackend to
defensively handle this by either (a) normalizing/validating entitlements in
__init__ to ensure a "can_access" key exists (e.g., set a default {"can_access":
{"result": True}} when absent) or (b) change can_access(self, user) to return
self.entitlements.get("can_access", {"result": True}); reference the
StaticEntitlementsBackend class, its __init__ and can_access methods when
applying the fix.
In `@src/backend/core/entitlements/factory.py`:
- Around line 9-12: The function get_entitlements_backend is currently memoized
with functools.cache which freezes the first-resolved backend and parameters
(settings.ENTITLEMENTS_BACKEND and ENTITLEMENTS_BACKEND_PARAMETERS) for the
process; remove the `@functools.cache` decorator so the backend is instantiated on
each call, or alternatively implement a cache keyed by the resolved backend
import path plus an immutable serialization of ENTITLEMENTS_BACKEND_PARAMETERS,
but the simplest fix is to stop using functools.cache and return
import_string(settings.ENTITLEMENTS_BACKEND)(**settings.ENTITLEMENTS_BACKEND_PARAMETERS)
per invocation so overrides or config changes take effect.
In `@src/backend/core/tests/test_api_entitlements.py`:
- Around line 48-55: The test currently only sets
settings.ENTITLEMENTS_BACKEND_PARAMETERS, which leaves the chosen backend
dependent on project defaults; explicitly set the entitlements backend to the
static implementation (e.g. settings.ENTITLEMENTS_BACKEND = "static" or the
project’s static backend key) inside
test_api_entitlements_static_backend_reads_from_parameters before calling
get_entitlements_backend.cache_clear(), so the test always uses the static
backend when reading ENTITLEMENTS_BACKEND_PARAMETERS.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ea1e819d-2a3e-4f88-8fb4-0180d01a7213
📒 Files selected for processing (28)
Makefilecompose.yamldocker/auth/realm.jsondocker/files/development/nginx/conf.d/oidc.confenv.d/development/backend.defaultsenv.d/development/kc_postgresql.defaultssrc/backend/core/api/permissions.pysrc/backend/core/api/viewsets/draft.pysrc/backend/core/api/viewsets/entitlements.pysrc/backend/core/authentication/__init__.pysrc/backend/core/authentication/backends.pysrc/backend/core/authentication/dev_bypass.pysrc/backend/core/authentication/dev_claims_middleware.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/static.pysrc/backend/core/entitlements/factory.pysrc/backend/core/migrations/0003_user_claims.pysrc/backend/core/models.pysrc/backend/core/tests/test_api_drafts.pysrc/backend/core/tests/test_api_entitlements.pysrc/backend/core/tests/test_api_entitlements_deploycenter.pysrc/backend/core/urls.pysrc/backend/transferts/settings.pysrc/frontend/next.config.tssrc/frontend/src/pages/index.tsx
Comment on lines
+750
to
+752
| { | ||
| "id": "869481d0-5774-4e64-bc30-fedc7c58958g", | ||
| "clientId": "deploycenter", |
There was a problem hiding this comment.
Potentially invalid UUID for deploycenter client.
The id value "869481d0-5774-4e64-bc30-fedc7c58958g" ends with g, which is not a valid hexadecimal character (UUIDs use 0-9 and a-f). This may cause issues when Keycloak imports or validates this realm.
🐛 Suggested fix
{
- "id": "869481d0-5774-4e64-bc30-fedc7c58958g",
+ "id": "869481d0-5774-4e64-bc30-fedc7c58958f",
"clientId": "deploycenter",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docker/auth/realm.json` around lines 750 - 752, The client entry with
clientId "deploycenter" has an invalid UUID value for "id" (contains a non-hex
character 'g'); replace the "id" string with a valid RFC 4122 v4 UUID (e.g.,
regenerate a new UUID) ensuring all characters are 0-9 or a-f and the standard
hyphenated format, then validate the JSON to ensure Keycloak will accept the
realm import for the deploycenter client.
Comment on lines
+1417
to
+1431
| { | ||
| "id": "qb109597-e31e-46d7-7844-62e5fcf32ac8", | ||
| "name": "email sub", | ||
| "protocol": "openid-connect", | ||
| "protocolMapper": "oidc-usermodel-property-mapper", | ||
| "consentRequired": false, | ||
| "config": { | ||
| "userinfo.token.claim": "true", | ||
| "user.attribute": "email", | ||
| "id.token.claim": "true", | ||
| "access.token.claim": "true", | ||
| "claim.name": "sub", | ||
| "jsonType.label": "String" | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact mapper block around the reported lines
sed -n '1400,1450p' docker/auth/realm.json | nl -ba | sed -n '1,120p'
# Find other mappers that touch the "sub" claim
rg -n '"claim\.name"\s*:\s*"sub"' docker/auth/realm.json || true
rg -n '"id\.token\.claim"| "access\.token\.claim"| "userinfo\.token\.claim"' docker/auth/realm.json | head -n 50
# Search repo for where DeployCenter or this mapper might be referenced
rg -n 'DeployCenter|deploycenter|"email sub"|"oidc-usermodel-property-mapper"|claim\.name\"\s*:\s*\"sub\"' . || trueRepository: suitenumerique/st-transfers
Length of output: 114
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print numbered context without `nl`
awk 'NR>=1400 && NR<=1450 {printf "%6d\t%s\n", NR, $0}' docker/auth/realm.json
echo "---- mappers setting claim.name=sub ----"
rg -n '"claim\.name"\s*:\s*"sub"' docker/auth/realm.json || true
echo "---- mappers named \"email sub\" ----"
rg -n '"name"\s*:\s*"email sub"' docker/auth/realm.json || true
echo "---- repo references to DeployCenter/email-as-sub patterns ----"
rg -n 'DeployCenter|deploycenter|oidc-usermodel-property-mapper|"email sub"' . || trueRepository: suitenumerique/st-transfers
Length of output: 7922
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n '"sub"\s*|claim\.name"\s*:\s*"sub"|OIDC_STORE_CLAIMS|oidc_.*sub|user\.sub|\bsub\b claim' src || true
rg -n 'sub\s*:' src || true
rg -n 'OIDC_STORE_CLAIMS|STORE_CLAIMS' . || trueRepository: suitenumerique/st-transfers
Length of output: 2052
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- src/backend/core/authentication/backends.py (head 120) ----"
sed -n '1,140p' src/backend/core/authentication/backends.py | nl -ba | head -n 170
echo "---- src/backend/core/models.py (around sub) ----"
sed -n '70,190p' src/backend/core/models.py | nl -ba | head -n 160
echo "---- src/backend/transferts/settings.py (around OIDC_STORE_CLAIMS) ----"
sed -n '420,500p' src/backend/transferts/settings.py | nl -ba
echo "---- env.d/development/backend.defaults (around OIDC_STORE_CLAIMS) ----"
sed -n '50,90p' env.d/development/backend.defaults | nl -ba
echo "---- src/backend/core/entitlements/backends/deploycenter.py (around line ~70-110) ----"
sed -n '60,120p' src/backend/core/entitlements/backends/deploycenter.py | nl -baRepository: suitenumerique/st-transfers
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
show_range() {
local file="$1" start="$2" end="$3"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%6d\t%s\n", NR, $0}' "$file"
}
echo "---- src/backend/core/authentication/backends.py (around sub) ----"
show_range src/backend/core/authentication/backends.py 1 140
echo "---- src/backend/core/models.py (around sub) ----"
show_range src/backend/core/models.py 70 190
echo "---- src/backend/transferts/settings.py (around OIDC_STORE_CLAIMS) ----"
show_range src/backend/transferts/settings.py 430 490
echo "---- env.d/development/backend.defaults (around OIDC_STORE_CLAIMS) ----"
show_range env.d/development/backend.defaults 50 90
echo "---- src/backend/core/entitlements/backends/deploycenter.py (around line 70-110) ----"
show_range src/backend/core/entitlements/backends/deploycenter.py 60 130Repository: suitenumerique/st-transfers
Length of output: 15670
🌐 Web query:
Keycloak oidc-usermodel-property-mapper claim.name "sub" userinfo.token.claim behavior
💡 Result:
Key point: In Keycloak, the standard OIDC claim sub is primarily controlled by the built-in Subject (sub) protocol mapper (and for pairwise subject identifiers, by the Pairwise subject identifier mapper overriding it). So using oidc-usermodel-property-mapper with claim.name="sub" will not reliably “change what sub means” in issued tokens/userinfo unless you also account for/disable the built-in Subject (sub) mapper and mapper execution priority. 1) What oidc-usermodel-property-mapper does The oidc-usermodel-property-mapper (UserPropertyMapper) maps a Keycloak UserModel property to a token claim name, and it can add claims to ID tokens, access tokens, and the UserInfo response because it implements UserInfoTokenMapper [1]. 2) What claim.name="sub" means in this mapper For this mapper, claim.name is the claim name into which the configured user property value is written [1]. Therefore, configuring claim.name="sub" should cause the mapper to attempt to add a claim named sub with the chosen user attribute value. 3) Why your sub value may still not match expectations (default built-in mapper + priority) Keycloak includes a built-in Subject (sub) protocol mapper in the client scope basic. Keycloak’s docs state that for pairwise subject identifiers, the Subject (sub) mapper runs before the Pairwise subject identifier mapper, and “hence the pairwise value will override the value added by the Subject mapper” because of protocol-mapper priority [2]. Practical consequence: even if your oidc-usermodel-property-mapper writes a sub claim, the final value that clients see in tokens or UserInfo may be overwritten by the built-in Subject (sub) / pairwise logic due to mapper priority [2]. 4) userinfo.token.claim behavior Since UserPropertyMapper can contribute to the UserInfo endpoint [1], the same overwrite/priority behavior applies to the UserInfo “sub” value you receive: it can be overridden by the built-in Subject (sub) and/or Pairwise subject identifier mapper behavior [2]. 5) Recommended way to actually control sub If your goal is to control the sub claim value, you need to manage the Subject (sub) mapper (and possibly Pairwise subject identifier mapper) in addition to any oidc-usermodel-property-mapper configuration, because Keycloak documents that mapper priority determines which sub wins [2]. If you share your Keycloak version and whether you are using pairwise subject identifiers (and which token types: ID token, access token, and/or UserInfo), I can give the exact console mapper settings to achieve the desired sub value.
Citations:
- 1: https://www.keycloak.org/docs-api/26.1.5/javadocs/org/keycloak/protocol/oidc/mappers/UserPropertyMapper.html
- 2: https://github.com/keycloak/keycloak/blob/main/docs/documentation/server_admin/topics/clients/con-protocol-mappers.adoc
Avoid setting the OIDC sub claim to the user’s email in docker/auth/realm.json
docker/auth/realm.jsondefines the"email sub"mapper that writesuser.attribute: "email"intoclaim.name: "sub"(lines 1417-1431).- The backend treats
subas the stable unique user identifier: it readsuser_info["sub"]and stores it inUser.sub(unique=True) for user lookup/creation. Ifsubchanges when email changes, user association/session continuity can break (or create duplicates depending on your fallback settings). - DeployCenter entitlements are driven from OIDC userinfo persisted into
User.claimsviaOIDC_STORE_CLAIMS(not fromsub), so there’s no need to overridesubfor that integration. - Also, Keycloak’s built-in Subject (
sub) / pairwise-subject mapper priority can override custom mappers writingsub; either keep the standard stablesuband map email separately, or explicitly manage the Subject mapper/pairwise configuration accordingly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docker/auth/realm.json` around lines 1417 - 1431, The "email sub" mapper is
overriding the OIDC subject by writing the user's email into claim "sub"; remove
or change that mapper so the OIDC "sub" stays Keycloak's stable subject. Locate
the mapper named "email sub" (protocolMapper "oidc-usermodel-property-mapper")
and either delete it or update its config so "claim.name" is "email" (or another
non-"sub" claim) while keeping "user.attribute": "email"; ensure no custom
mapper writes to "sub" so the backend continues to receive a stable subject from
Keycloak's built-in Subject/pairwise mapper.
Comment on lines
+39
to
+45
| _UPLOAD_ACTIONS = frozenset({"add_file", "sign_part", "complete_upload"}) | ||
|
|
||
| def has_permission(self, request, view): | ||
| if getattr(view, "action", None) not in self._UPLOAD_ACTIONS: | ||
| return True | ||
| enforce_upload_entitlement(request.user) | ||
| return True |
There was a problem hiding this comment.
finalize still bypasses the entitlement gate.
Only add_file, sign_part, and complete_upload are checked here. A user with an existing draft can still call finalize after can_access starts returning false and create a transfer anyway, which defeats the server-side access gate the homepage now enforces. Add finalize to the protected action set, or gate all mutating draft actions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/core/api/permissions.py` around lines 39 - 45, The permission
check in has_permission currently only protects actions in _UPLOAD_ACTIONS
(frozenset of "add_file","sign_part","complete_upload") so the "finalize" action
bypasses enforce_upload_entitlement; update _UPLOAD_ACTIONS to include
"finalize" (or expand the set to cover all mutating draft actions) so that
has_permission calls enforce_upload_entitlement for finalize as well, ensuring
enforce_upload_entitlement(request.user) is invoked before returning True.
Comment on lines
+17
to
+22
| for method_name in dir(entitlements_backend): | ||
| if method_name.startswith("can_"): | ||
| method = getattr(entitlements_backend, method_name) | ||
| if callable(method): | ||
| entitlements[method_name] = method(request.user) | ||
| entitlements["context"] = entitlements_backend.get_context(request.user) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use an explicit entitlement export contract instead of dir().
The response schema currently depends on every callable named can_* on the backend instance. That is fragile: adding a helper like can_service(user, service_id) would now be auto-invoked here with the wrong signature and break GET /entitlements/. Prefer a dedicated method or declared list of exported entitlement checks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/core/api/viewsets/entitlements.py` around lines 17 - 22, The
current loop uses dir(entitlements_backend) and auto-invokes any callable
starting with "can_", which is fragile; change to an explicit export contract on
the backend (e.g., a list attribute like EXPORTED_ENTITLEMENTS or a method
get_entitlement_checks()) and iterate that instead. Update the logic in
entitlements.py to retrieve the declared names from entitlements_backend (e.g.,
entitlements_backend.EXPORTED_ENTITLEMENTS or call
entitlements_backend.get_entitlement_checks()) and for each declared name call
the corresponding callable with only the user (method =
getattr(entitlements_backend, name); entitlements[name] = method(request.user)),
preserving entitlements["context"] =
entitlements_backend.get_context(request.user); also validate existence and
callability of each declared symbol and raise a clear error if missing or
signature is incorrect.
Comment on lines
+71
to
+77
| claims_to_store = { | ||
| claim: user_info.get(claim) | ||
| for claim in getattr(settings, "OIDC_STORE_CLAIMS", []) | ||
| } | ||
| return { | ||
| "full_name": self.compute_full_name(user_info), | ||
| "claims": claims_to_store, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider filtering out None values from stored claims.
user_info.get(claim) returns None for missing claims, which will be stored in the database. This may clutter the claims field with explicit nulls for claims the IdP didn't provide.
♻️ Suggested improvement
def get_extra_claims(self, user_info):
"""Get extra claims from user info."""
claims_to_store = {
claim: user_info.get(claim)
for claim in getattr(settings, "OIDC_STORE_CLAIMS", [])
+ if user_info.get(claim) is not None
}
return {
"full_name": self.compute_full_name(user_info),
"claims": claims_to_store,
}📝 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
| claims_to_store = { | |
| claim: user_info.get(claim) | |
| for claim in getattr(settings, "OIDC_STORE_CLAIMS", []) | |
| } | |
| return { | |
| "full_name": self.compute_full_name(user_info), | |
| "claims": claims_to_store, | |
| claims_to_store = { | |
| claim: user_info.get(claim) | |
| for claim in getattr(settings, "OIDC_STORE_CLAIMS", []) | |
| if user_info.get(claim) is not None | |
| } | |
| return { | |
| "full_name": self.compute_full_name(user_info), | |
| "claims": claims_to_store, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/core/authentication/backends.py` around lines 71 - 77,
claims_to_store currently includes keys with value None because
user_info.get(claim) returns None for missing claims; change the comprehension
in the claims_to_store assignment to only include claim: value pairs where value
is not None (e.g., iterate over getattr(settings, "OIDC_STORE_CLAIMS", []) and
include claim if (value := user_info.get(claim)) is not None) so the stored
"claims" dict excludes null entries; keep the surrounding return structure
(including compute_full_name(user_info)) unchanged.
Comment on lines
+105
to
+109
| try: | ||
| entitlements = self.fetch_entitlements(user) | ||
| except requests.RequestException: | ||
| logger.exception("Failed to fetch entitlements for user %s", user.id) | ||
| raise |
There was a problem hiding this comment.
Wrap provider failures in EntitlementsUnavailableError.
get_entitlements() logs requests.RequestException and re-raises it unchanged. That bypasses the new app-level unavailability exception and will likely surface DeployCenter outages as 500s instead of a controlled entitlement-unavailable response.
Suggested fix
try:
entitlements = self.fetch_entitlements(user)
- except requests.RequestException:
+ except requests.RequestException as exc:
logger.exception("Failed to fetch entitlements for user %s", user.id)
- raise
+ raise EntitlementsUnavailableError(
+ "Failed to fetch entitlements from DeployCenter."
+ ) from exc📝 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
| try: | |
| entitlements = self.fetch_entitlements(user) | |
| except requests.RequestException: | |
| logger.exception("Failed to fetch entitlements for user %s", user.id) | |
| raise | |
| try: | |
| entitlements = self.fetch_entitlements(user) | |
| except requests.RequestException as exc: | |
| logger.exception("Failed to fetch entitlements for user %s", user.id) | |
| raise EntitlementsUnavailableError( | |
| "Failed to fetch entitlements from DeployCenter." | |
| ) from exc |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/core/entitlements/backends/deploycenter.py` around lines 105 -
109, The try/except around fetch_entitlements currently catches
requests.RequestException and re-raises it directly, which bypasses our
app-level EntitlementsUnavailableError; update the except block in
get_entitlements (the block calling self.fetch_entitlements(user)) to log the
failure (preserve logger.exception call) and then raise
EntitlementsUnavailableError from the caught RequestException so the higher
layers get the controlled unavailability error instead of a raw requests
exception. Ensure you reference the original exception as the __cause__ (use
"raise EntitlementsUnavailableError(...) from e") and keep the same context
(include user.id in the log).
Comment on lines
+9
to
+15
| def __init__(self, entitlements=None): | ||
| self.entitlements = entitlements or { | ||
| "can_access": {"result": True}, | ||
| } | ||
|
|
||
| def can_access(self, user): | ||
| return self.entitlements["can_access"] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider defensive access to the "can_access" key.
If entitlements is passed without a "can_access" key (e.g., StaticEntitlementsBackend(entitlements={})), can_access() will raise a KeyError. While the default handles the normal case, consider using .get() with a fallback or validating the input in __init__.
🛡️ Optional defensive approach
def can_access(self, user):
- return self.entitlements["can_access"]
+ return self.entitlements.get("can_access", {"result": False})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/core/entitlements/backends/static.py` around lines 9 - 15, The
can_access method can raise KeyError if the provided entitlements dict lacks
"can_access"; update StaticEntitlementsBackend to defensively handle this by
either (a) normalizing/validating entitlements in __init__ to ensure a
"can_access" key exists (e.g., set a default {"can_access": {"result": True}}
when absent) or (b) change can_access(self, user) to return
self.entitlements.get("can_access", {"result": True}); reference the
StaticEntitlementsBackend class, its __init__ and can_access methods when
applying the fix.
Comment on lines
+9
to
+12
| @functools.cache | ||
| def get_entitlements_backend(): | ||
| """Get the entitlements backend.""" | ||
| return import_string(settings.ENTITLEMENTS_BACKEND)(**settings.ENTITLEMENTS_BACKEND_PARAMETERS) |
There was a problem hiding this comment.
Avoid caching a settings-derived backend instance.
This memoizes the first backend class and parameters for the whole process. Any later override_settings() or config switch still returns the stale instance, so tests or long-lived workers can evaluate entitlements against the wrong backend. Instantiate per call, or cache by the resolved backend path + immutable parameters instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/core/entitlements/factory.py` around lines 9 - 12, The function
get_entitlements_backend is currently memoized with functools.cache which
freezes the first-resolved backend and parameters (settings.ENTITLEMENTS_BACKEND
and ENTITLEMENTS_BACKEND_PARAMETERS) for the process; remove the
`@functools.cache` decorator so the backend is instantiated on each call, or
alternatively implement a cache keyed by the resolved backend import path plus
an immutable serialization of ENTITLEMENTS_BACKEND_PARAMETERS, but the simplest
fix is to stop using functools.cache and return
import_string(settings.ENTITLEMENTS_BACKEND)(**settings.ENTITLEMENTS_BACKEND_PARAMETERS)
per invocation so overrides or config changes take effect.
Comment on lines
+48
to
+55
| def test_api_entitlements_static_backend_reads_from_parameters(settings): | ||
| """StaticEntitlementsBackend should return values from ENTITLEMENTS_BACKEND_PARAMETERS.""" | ||
| settings.ENTITLEMENTS_BACKEND_PARAMETERS = { | ||
| "entitlements": { | ||
| "can_access": {"result": False, "message": "Access denied for testing"}, | ||
| }, | ||
| } | ||
| get_entitlements_backend.cache_clear() |
There was a problem hiding this comment.
Make this test select the static backend explicitly.
Changing only ENTITLEMENTS_BACKEND_PARAMETERS makes the assertion depend on whichever backend is configured as the project default. If CI or a later settings change switches the default to DeployCenter, this test will fail for the wrong reason.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/core/tests/test_api_entitlements.py` around lines 48 - 55, The
test currently only sets settings.ENTITLEMENTS_BACKEND_PARAMETERS, which leaves
the chosen backend dependent on project defaults; explicitly set the
entitlements backend to the static implementation (e.g.
settings.ENTITLEMENTS_BACKEND = "static" or the project’s static backend key)
inside test_api_entitlements_static_backend_reads_from_parameters before calling
get_entitlements_backend.cache_clear(), so the test always uses the static
backend when reading ENTITLEMENTS_BACKEND_PARAMETERS.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/src/features/errors/components/Error.tsx`:
- Around line 3-4: The SUPPORT_LINK_URL constant in Error.tsx duplicates the
backend SUPPORT_URL (settings.py) and should be sourced from a single
configurable place; replace the hardcoded SUPPORT_LINK_URL with a read from a
runtime configuration or build-time env var (e.g.,
window.__RUNTIME_CONFIG__.SUPPORT_URL or process.env.REACT_APP_SUPPORT_URL) so
the front-end and back-end share the same value, update the Error component to
use that config value with a sensible fallback, and ensure the deployment
injects the runtime config or env var used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e7edfb22-0d2f-4cc8-a65d-8e9f5547f2be
⛔ Files ignored due to path filters (1)
src/frontend/public/images/main-error.svgis excluded by!**/*.svg
📒 Files selected for processing (11)
env.d/development/backend.defaultssrc/backend/core/authentication/__init__.pysrc/backend/core/authentication/backends.pysrc/backend/core/authentication/views.pysrc/backend/transferts/settings.pysrc/frontend/src/features/errors/components/Error.tsxsrc/frontend/src/features/errors/components/ErrorPageLayout.tsxsrc/frontend/src/features/errors/components/_errors.scsssrc/frontend/src/pages/errors/index.tsxsrc/frontend/src/pages/index.tsxsrc/frontend/src/styles/main.scss
Comment on lines
+3
to
+4
| const SUPPORT_LINK_URL = | ||
| "https://docs.suite.anct.gouv.fr/docs/281bc1f0-5911-4442-b4b7-af78d77f0e1e/"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Hardcoded support URL duplicates backend configuration.
This URL matches SUPPORT_URL in settings.py (line 181-185). If the support URL changes, both places need updating. Consider exposing this via a runtime config endpoint or environment variable at build time to keep them in sync.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/frontend/src/features/errors/components/Error.tsx` around lines 3 - 4,
The SUPPORT_LINK_URL constant in Error.tsx duplicates the backend SUPPORT_URL
(settings.py) and should be sourced from a single configurable place; replace
the hardcoded SUPPORT_LINK_URL with a read from a runtime configuration or
build-time env var (e.g., window.__RUNTIME_CONFIG__.SUPPORT_URL or
process.env.REACT_APP_SUPPORT_URL) so the front-end and back-end share the same
value, update the Error component to use that config value with a sensible
fallback, and ensure the deployment injects the runtime config or env var used.
Nastaliss
force-pushed
the
feature/eo-api-entitlements
branch
from
June 15, 2026 16:14
36bd903 to
46cd8da
Compare
|
Note The previously reviewed commits are no longer reachable (likely due to a force-push or rebase), so CodeRabbit is performing a full review instead of an incremental one. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
src/backend/core/entitlements/backends/static.py (1)
14-15: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider defensive access to the
"can_access"key.If
entitlementsis passed without a"can_access"key (e.g.,StaticEntitlementsBackend(entitlements={})),can_access()will raise aKeyError. While the default handles the normal case, consider using.get()with a fallback or validating the input in__init__.🛡️ Proposed fix
def can_access(self, user): - return self.entitlements["can_access"] + return self.entitlements.get("can_access", {"result": False})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/entitlements/backends/static.py` around lines 14 - 15, Update StaticEntitlementsBackend.can_access to handle entitlements missing the "can_access" key without raising KeyError, returning the appropriate false fallback for absent values while preserving configured values.src/backend/core/api/permissions.py (1)
39-45: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
finalizestill bypasses the entitlement gate.Only
add_file,sign_part, andcomplete_uploadare checked here. A user with an existing draft can still callfinalizeaftercan_accessstarts returning false and create a transfer anyway, which defeats the server-side access gate the homepage now enforces. Addfinalizeto the protected action set, or gate all mutating draft actions.🔒️ Proposed fix
- _UPLOAD_ACTIONS = frozenset({"add_file", "sign_part", "complete_upload"}) + _UPLOAD_ACTIONS = frozenset({"add_file", "sign_part", "complete_upload", "finalize"}) def has_permission(self, request, view):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/api/permissions.py` around lines 39 - 45, Update the _UPLOAD_ACTIONS set used by has_permission to include the finalize action, ensuring finalize requests invoke enforce_upload_entitlement(request.user) before proceeding while preserving the existing behavior for the other upload actions.src/backend/core/entitlements/backends/deploycenter.py (1)
105-109: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWrap provider failures in
EntitlementsUnavailableError.
get_entitlements()logsrequests.RequestExceptionand re-raises it unchanged. That bypasses the new app-level unavailability exception and will likely surface DeployCenter outages as 500s instead of a controlled entitlement-unavailable response.🛡️ Proposed fix
try: entitlements = self.fetch_entitlements(user) - except requests.RequestException: + except requests.RequestException as exc: logger.exception("Failed to fetch entitlements for user %s", user.id) - raise + raise EntitlementsUnavailableError( + "Failed to fetch entitlements from DeployCenter." + ) from exc🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/entitlements/backends/deploycenter.py` around lines 105 - 109, Update get_entitlements() so requests.RequestException from fetch_entitlements() is logged, then wrapped and raised as EntitlementsUnavailableError instead of being re-raised unchanged. Preserve the existing user context in the log and ensure the provider exception is retained as the cause of the app-level error.src/backend/core/api/viewsets/entitlements.py (1)
17-22: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse an explicit entitlement export contract instead of
dir().The response schema currently depends on every callable named
can_*on the backend instance. That is fragile: adding a helper likecan_service(user, service_id)would now be auto-invoked here with the wrong signature and breakGET /entitlements/. Prefer a dedicated method or declared list of exported entitlement checks.♻️ Proposed fix using a dedicated method on the backend
First, define the method in
EntitlementsBackend(inbase.py):def get_entitlement_checks(self, user): """Return all entitlement checks for the user.""" return {"can_access": self.can_access(user)}Then update
EntitlementsViewsetto use it:- for method_name in dir(entitlements_backend): - if method_name.startswith("can_"): - method = getattr(entitlements_backend, method_name) - if callable(method): - entitlements[method_name] = method(request.user) + entitlements.update(entitlements_backend.get_entitlement_checks(request.user)) entitlements["context"] = entitlements_backend.get_context(request.user) return drf.response.Response(entitlements)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/api/viewsets/entitlements.py` around lines 17 - 22, Replace the dynamic dir(entitlements_backend) scan in EntitlementsViewset with the explicit entitlement export contract provided by EntitlementsBackend.get_entitlement_checks(user). Add or use that method to return only the response-safe checks, while preserving the existing context assignment and response structure.src/backend/core/entitlements/factory.py (1)
9-12: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid caching a settings-derived backend instance.
This memoizes the first backend class and parameters for the whole process. Any later
override_settings()or config switch still returns the stale instance, so tests or long-lived workers can evaluate entitlements against the wrong backend. Instantiate per call, or cache by the resolved backend path + immutable parameters instead.♻️ Proposed fix
-@functools.cache def get_entitlements_backend(): """Get the entitlements backend.""" return import_string(settings.ENTITLEMENTS_BACKEND)(**settings.ENTITLEMENTS_BACKEND_PARAMETERS)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/entitlements/factory.py` around lines 9 - 12, Remove the functools.cache decorator from get_entitlements_backend so each call resolves settings.ENTITLEMENTS_BACKEND and settings.ENTITLEMENTS_BACKEND_PARAMETERS anew, ensuring override_settings or runtime configuration changes cannot reuse a stale backend instance.src/backend/core/authentication/backends.py (1)
79-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider filtering out
Nonevalues from stored claims.
user_info.get(claim)returnsNonefor missing claims, which will be stored in the database. This may clutter theclaimsfield with explicit nulls for claims the IdP didn't provide.♻️ Suggested improvement
- def get_extra_claims(self, user_info): - """Get extra claims from user info.""" - claims_to_store = { - claim: user_info.get(claim) - for claim in getattr(settings, "OIDC_STORE_CLAIMS", []) - } - return { - "full_name": self.compute_full_name(user_info), - "claims": claims_to_store, - } + def get_extra_claims(self, user_info): + """Get extra claims from user info.""" + claims_to_store = { + claim: user_info.get(claim) + for claim in getattr(settings, "OIDC_STORE_CLAIMS", []) + if user_info.get(claim) is not None + } + return { + "full_name": self.compute_full_name(user_info), + "claims": claims_to_store, + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/authentication/backends.py` around lines 79 - 88, Update get_extra_claims so the claims_to_store comprehension excludes configured claims whose user_info.get(claim) result is None, while continuing to store all provided claim values and preserving the existing full_name and claims response structure.src/backend/core/authentication/dev_claims_middleware.py (1)
22-31: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix logic bug and use direct attribute assignment.
hasattr(user, "claims")always returnsTruebecause theUsermodel now explicitly defines aclaimsfield. This prevents the middleware from ever populating development claims from the session. Check if the claims are empty instead.As per static analysis hints, avoid using
setattrfor constant attribute values.🐛 Proposed fix
- def __call__(self, request: HttpRequest) -> HttpResponse: - user = getattr(request, "user", None) - if user is not None and getattr(user, "is_authenticated", False): - if not hasattr(user, "claims"): - claims: Any = request.session.get(_DEV_CLAIMS_SESSION_KEY, {}) - if isinstance(claims, dict): - setattr(user, "claims", claims) - else: - setattr(user, "claims", {}) - return self.get_response(request) + def __call__(self, request: HttpRequest) -> HttpResponse: + user = getattr(request, "user", None) + if user is not None and getattr(user, "is_authenticated", False): + if not getattr(user, "claims", None): + claims: Any = request.session.get(_DEV_CLAIMS_SESSION_KEY, {}) + if isinstance(claims, dict): + user.claims = claims + else: + user.claims = {} + return self.get_response(request)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/authentication/dev_claims_middleware.py` around lines 22 - 31, Update the authenticated-user branch in __call__ to populate development claims when user.claims is empty, rather than checking hasattr, since the User model always defines the field. Preserve the dictionary validation and empty-dictionary fallback for session claims, and replace setattr calls with direct user.claims assignments.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/core/tests/test_api_drafts.py`:
- Around line 1105-1186: Refactor TestDraftUploadEntitlement to apply the
entitlement settings with a class-level override_settings decorator, remove each
test’s _no_access_entitlements context and try/finally blocks, and add an
autouse fixture that clears get_entitlements_backend’s cache before and after
each test. Preserve the existing test requests and assertions.
---
Duplicate comments:
In `@src/backend/core/api/permissions.py`:
- Around line 39-45: Update the _UPLOAD_ACTIONS set used by has_permission to
include the finalize action, ensuring finalize requests invoke
enforce_upload_entitlement(request.user) before proceeding while preserving the
existing behavior for the other upload actions.
In `@src/backend/core/api/viewsets/entitlements.py`:
- Around line 17-22: Replace the dynamic dir(entitlements_backend) scan in
EntitlementsViewset with the explicit entitlement export contract provided by
EntitlementsBackend.get_entitlement_checks(user). Add or use that method to
return only the response-safe checks, while preserving the existing context
assignment and response structure.
In `@src/backend/core/authentication/backends.py`:
- Around line 79-88: Update get_extra_claims so the claims_to_store
comprehension excludes configured claims whose user_info.get(claim) result is
None, while continuing to store all provided claim values and preserving the
existing full_name and claims response structure.
In `@src/backend/core/authentication/dev_claims_middleware.py`:
- Around line 22-31: Update the authenticated-user branch in __call__ to
populate development claims when user.claims is empty, rather than checking
hasattr, since the User model always defines the field. Preserve the dictionary
validation and empty-dictionary fallback for session claims, and replace setattr
calls with direct user.claims assignments.
In `@src/backend/core/entitlements/backends/deploycenter.py`:
- Around line 105-109: Update get_entitlements() so requests.RequestException
from fetch_entitlements() is logged, then wrapped and raised as
EntitlementsUnavailableError instead of being re-raised unchanged. Preserve the
existing user context in the log and ensure the provider exception is retained
as the cause of the app-level error.
In `@src/backend/core/entitlements/backends/static.py`:
- Around line 14-15: Update StaticEntitlementsBackend.can_access to handle
entitlements missing the "can_access" key without raising KeyError, returning
the appropriate false fallback for absent values while preserving configured
values.
In `@src/backend/core/entitlements/factory.py`:
- Around line 9-12: Remove the functools.cache decorator from
get_entitlements_backend so each call resolves settings.ENTITLEMENTS_BACKEND and
settings.ENTITLEMENTS_BACKEND_PARAMETERS anew, ensuring override_settings or
runtime configuration changes cannot reuse a stale backend instance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b05e2003-97bc-4ad9-91f3-26d748bc7c6d
⛔ Files ignored due to path filters (1)
src/frontend/public/images/main-error.svgis excluded by!**/*.svg
📒 Files selected for processing (28)
env.d/development/backend.defaultsenv.d/development/kc_postgresql.defaultssrc/backend/core/api/permissions.pysrc/backend/core/api/viewsets/draft.pysrc/backend/core/api/viewsets/entitlements.pysrc/backend/core/authentication/__init__.pysrc/backend/core/authentication/backends.pysrc/backend/core/authentication/dev_bypass.pysrc/backend/core/authentication/dev_claims_middleware.pysrc/backend/core/authentication/views.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/static.pysrc/backend/core/entitlements/factory.pysrc/backend/core/migrations/0006_user_claims.pysrc/backend/core/models.pysrc/backend/core/tests/test_api_drafts.pysrc/backend/core/tests/test_api_entitlements.pysrc/backend/core/tests/test_api_entitlements_deploycenter.pysrc/backend/core/urls.pysrc/backend/transferts/settings.pysrc/frontend/src/features/errors/components/Error.tsxsrc/frontend/src/features/errors/components/ErrorPageLayout.tsxsrc/frontend/src/features/errors/components/_errors.scsssrc/frontend/src/routes/errors/index.tsxsrc/frontend/src/styles/main.scss
Comment on lines
+1105
to
+1186
| @pytest.mark.django_db | ||
| class TestDraftUploadEntitlement: | ||
| """Draft multipart endpoints require ``can_access`` from the entitlements backend.""" | ||
|
|
||
| @staticmethod | ||
| def _no_access_entitlements(): | ||
| return override_settings( | ||
| ENTITLEMENTS_BACKEND="core.entitlements.backends.static.StaticEntitlementsBackend", | ||
| ENTITLEMENTS_BACKEND_PARAMETERS={ | ||
| "entitlements": { | ||
| "can_access": {"result": False, "message": "access_denied"}, | ||
| }, | ||
| }, | ||
| ) | ||
|
|
||
| def test_add_file_returns_403_when_can_access_false(self, authenticated_client): | ||
| with self._no_access_entitlements(): | ||
| get_entitlements_backend.cache_clear() | ||
| try: | ||
| resp = authenticated_client.post( | ||
| ADD_FILE_URL, | ||
| {"filename": "a.bin", "size": 100}, | ||
| format="json", | ||
| ) | ||
| assert resp.status_code == 403, resp.data | ||
| assert "access_denied" in str(resp.data) | ||
| finally: | ||
| get_entitlements_backend.cache_clear() | ||
|
|
||
| def test_sign_part_returns_403_when_can_access_false(self, authenticated_client, user): | ||
| draft = TransferDraftFactory(owner=user) | ||
| transfer_file = TransferFileFactory( | ||
| draft=draft, | ||
| transfer=None, | ||
| filename="a.bin", | ||
| size=100, | ||
| upload_id="mpu-test", | ||
| upload_completed_at=None, | ||
| ) | ||
| with self._no_access_entitlements(): | ||
| get_entitlements_backend.cache_clear() | ||
| try: | ||
| resp = authenticated_client.post( | ||
| f"{DRAFTS_URL}{draft.id}/sign-part/", | ||
| { | ||
| "transfer_file_id": str(transfer_file.id), | ||
| "part_number": 1, | ||
| }, | ||
| format="json", | ||
| ) | ||
| assert resp.status_code == 403, resp.data | ||
| finally: | ||
| get_entitlements_backend.cache_clear() | ||
|
|
||
| def test_complete_upload_returns_403_when_can_access_false( | ||
| self, authenticated_client, user | ||
| ): | ||
| draft = TransferDraftFactory(owner=user) | ||
| transfer_file = TransferFileFactory( | ||
| draft=draft, | ||
| transfer=None, | ||
| filename="a.bin", | ||
| size=100, | ||
| upload_id="mpu-test", | ||
| upload_completed_at=None, | ||
| ) | ||
| with self._no_access_entitlements(): | ||
| get_entitlements_backend.cache_clear() | ||
| try: | ||
| resp = authenticated_client.post( | ||
| f"{DRAFTS_URL}{draft.id}/complete-upload/", | ||
| { | ||
| "transfer_file_id": str(transfer_file.id), | ||
| "parts": [{"PartNumber": 1, "ETag": '"etag-1"'}], | ||
| }, | ||
| format="json", | ||
| ) | ||
| assert resp.status_code == 403, resp.data | ||
| finally: | ||
| get_entitlements_backend.cache_clear() | ||
|
|
||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Simplify test setup with class decorators and autouse fixtures.
The repeated with self._no_access_entitlements(): and try/finally blocks for clearing the cache can be eliminated by using a class-level @override_settings decorator and an autouse=True fixture for the cache cleanup. This significantly improves readability and reduces boilerplate.
♻️ Proposed refactor
-@pytest.mark.django_db
+@override_settings(
+ ENTITLEMENTS_BACKEND="core.entitlements.backends.static.StaticEntitlementsBackend",
+ ENTITLEMENTS_BACKEND_PARAMETERS={
+ "entitlements": {
+ "can_access": {"result": False, "message": "access_denied"},
+ },
+ },
+)
+@pytest.mark.django_db
class TestDraftUploadEntitlement:
"""Draft multipart endpoints require ``can_access`` from the entitlements backend."""
- `@staticmethod`
- def _no_access_entitlements():
- return override_settings(
- ENTITLEMENTS_BACKEND="core.entitlements.backends.static.StaticEntitlementsBackend",
- ENTITLEMENTS_BACKEND_PARAMETERS={
- "entitlements": {
- "can_access": {"result": False, "message": "access_denied"},
- },
- },
- )
+ `@pytest.fixture`(autouse=True)
+ def _clear_cache(self):
+ get_entitlements_backend.cache_clear()
+ yield
+ get_entitlements_backend.cache_clear()
def test_add_file_returns_403_when_can_access_false(self, authenticated_client):
- with self._no_access_entitlements():
- get_entitlements_backend.cache_clear()
- try:
- resp = authenticated_client.post(
- ADD_FILE_URL,
- {"filename": "a.bin", "size": 100},
- format="json",
- )
- assert resp.status_code == 403, resp.data
- assert "access_denied" in str(resp.data)
- finally:
- get_entitlements_backend.cache_clear()
+ resp = authenticated_client.post(
+ ADD_FILE_URL,
+ {"filename": "a.bin", "size": 100},
+ format="json",
+ )
+ assert resp.status_code == 403, resp.data
+ assert "access_denied" in str(resp.data)
def test_sign_part_returns_403_when_can_access_false(self, authenticated_client, user):
draft = TransferDraftFactory(owner=user)
transfer_file = TransferFileFactory(
draft=draft,
transfer=None,
filename="a.bin",
size=100,
upload_id="mpu-test",
upload_completed_at=None,
)
- with self._no_access_entitlements():
- get_entitlements_backend.cache_clear()
- try:
- resp = authenticated_client.post(
- f"{DRAFTS_URL}{draft.id}/sign-part/",
- {
- "transfer_file_id": str(transfer_file.id),
- "part_number": 1,
- },
- format="json",
- )
- assert resp.status_code == 403, resp.data
- finally:
- get_entitlements_backend.cache_clear()
+ resp = authenticated_client.post(
+ f"{DRAFTS_URL}{draft.id}/sign-part/",
+ {
+ "transfer_file_id": str(transfer_file.id),
+ "part_number": 1,
+ },
+ format="json",
+ )
+ assert resp.status_code == 403, resp.data
def test_complete_upload_returns_403_when_can_access_false(
self, authenticated_client, user
):
draft = TransferDraftFactory(owner=user)
transfer_file = TransferFileFactory(
draft=draft,
transfer=None,
filename="a.bin",
size=100,
upload_id="mpu-test",
upload_completed_at=None,
)
- with self._no_access_entitlements():
- get_entitlements_backend.cache_clear()
- try:
- resp = authenticated_client.post(
- f"{DRAFTS_URL}{draft.id}/complete-upload/",
- {
- "transfer_file_id": str(transfer_file.id),
- "parts": [{"PartNumber": 1, "ETag": '"etag-1"'}],
- },
- format="json",
- )
- assert resp.status_code == 403, resp.data
- finally:
- get_entitlements_backend.cache_clear()
+ resp = authenticated_client.post(
+ f"{DRAFTS_URL}{draft.id}/complete-upload/",
+ {
+ "transfer_file_id": str(transfer_file.id),
+ "parts": [{"PartNumber": 1, "ETag": '"etag-1"'}],
+ },
+ format="json",
+ )
+ assert resp.status_code == 403, resp.data📝 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.mark.django_db | |
| class TestDraftUploadEntitlement: | |
| """Draft multipart endpoints require ``can_access`` from the entitlements backend.""" | |
| @staticmethod | |
| def _no_access_entitlements(): | |
| return override_settings( | |
| ENTITLEMENTS_BACKEND="core.entitlements.backends.static.StaticEntitlementsBackend", | |
| ENTITLEMENTS_BACKEND_PARAMETERS={ | |
| "entitlements": { | |
| "can_access": {"result": False, "message": "access_denied"}, | |
| }, | |
| }, | |
| ) | |
| def test_add_file_returns_403_when_can_access_false(self, authenticated_client): | |
| with self._no_access_entitlements(): | |
| get_entitlements_backend.cache_clear() | |
| try: | |
| resp = authenticated_client.post( | |
| ADD_FILE_URL, | |
| {"filename": "a.bin", "size": 100}, | |
| format="json", | |
| ) | |
| assert resp.status_code == 403, resp.data | |
| assert "access_denied" in str(resp.data) | |
| finally: | |
| get_entitlements_backend.cache_clear() | |
| def test_sign_part_returns_403_when_can_access_false(self, authenticated_client, user): | |
| draft = TransferDraftFactory(owner=user) | |
| transfer_file = TransferFileFactory( | |
| draft=draft, | |
| transfer=None, | |
| filename="a.bin", | |
| size=100, | |
| upload_id="mpu-test", | |
| upload_completed_at=None, | |
| ) | |
| with self._no_access_entitlements(): | |
| get_entitlements_backend.cache_clear() | |
| try: | |
| resp = authenticated_client.post( | |
| f"{DRAFTS_URL}{draft.id}/sign-part/", | |
| { | |
| "transfer_file_id": str(transfer_file.id), | |
| "part_number": 1, | |
| }, | |
| format="json", | |
| ) | |
| assert resp.status_code == 403, resp.data | |
| finally: | |
| get_entitlements_backend.cache_clear() | |
| def test_complete_upload_returns_403_when_can_access_false( | |
| self, authenticated_client, user | |
| ): | |
| draft = TransferDraftFactory(owner=user) | |
| transfer_file = TransferFileFactory( | |
| draft=draft, | |
| transfer=None, | |
| filename="a.bin", | |
| size=100, | |
| upload_id="mpu-test", | |
| upload_completed_at=None, | |
| ) | |
| with self._no_access_entitlements(): | |
| get_entitlements_backend.cache_clear() | |
| try: | |
| resp = authenticated_client.post( | |
| f"{DRAFTS_URL}{draft.id}/complete-upload/", | |
| { | |
| "transfer_file_id": str(transfer_file.id), | |
| "parts": [{"PartNumber": 1, "ETag": '"etag-1"'}], | |
| }, | |
| format="json", | |
| ) | |
| assert resp.status_code == 403, resp.data | |
| finally: | |
| get_entitlements_backend.cache_clear() | |
| `@override_settings`( | |
| ENTITLEMENTS_BACKEND="core.entitlements.backends.static.StaticEntitlementsBackend", | |
| ENTITLEMENTS_BACKEND_PARAMETERS={ | |
| "entitlements": { | |
| "can_access": {"result": False, "message": "access_denied"}, | |
| }, | |
| }, | |
| ) | |
| `@pytest.mark.django_db` | |
| class TestDraftUploadEntitlement: | |
| """Draft multipart endpoints require ``can_access`` from the entitlements backend.""" | |
| `@pytest.fixture`(autouse=True) | |
| def _clear_cache(self): | |
| get_entitlements_backend.cache_clear() | |
| yield | |
| get_entitlements_backend.cache_clear() | |
| def test_add_file_returns_403_when_can_access_false(self, authenticated_client): | |
| resp = authenticated_client.post( | |
| ADD_FILE_URL, | |
| {"filename": "a.bin", "size": 100}, | |
| format="json", | |
| ) | |
| assert resp.status_code == 403, resp.data | |
| assert "access_denied" in str(resp.data) | |
| def test_sign_part_returns_403_when_can_access_false(self, authenticated_client, user): | |
| draft = TransferDraftFactory(owner=user) | |
| transfer_file = TransferFileFactory( | |
| draft=draft, | |
| transfer=None, | |
| filename="a.bin", | |
| size=100, | |
| upload_id="mpu-test", | |
| upload_completed_at=None, | |
| ) | |
| resp = authenticated_client.post( | |
| f"{DRAFTS_URL}{draft.id}/sign-part/", | |
| { | |
| "transfer_file_id": str(transfer_file.id), | |
| "part_number": 1, | |
| }, | |
| format="json", | |
| ) | |
| assert resp.status_code == 403, resp.data | |
| def test_complete_upload_returns_403_when_can_access_false( | |
| self, authenticated_client, user | |
| ): | |
| draft = TransferDraftFactory(owner=user) | |
| transfer_file = TransferFileFactory( | |
| draft=draft, | |
| transfer=None, | |
| filename="a.bin", | |
| size=100, | |
| upload_id="mpu-test", | |
| upload_completed_at=None, | |
| ) | |
| resp = authenticated_client.post( | |
| f"{DRAFTS_URL}{draft.id}/complete-upload/", | |
| { | |
| "transfer_file_id": str(transfer_file.id), | |
| "parts": [{"PartNumber": 1, "ETag": '"etag-1"'}], | |
| }, | |
| format="json", | |
| ) | |
| assert resp.status_code == 403, resp.data |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/core/tests/test_api_drafts.py` around lines 1105 - 1186, Refactor
TestDraftUploadEntitlement to apply the entitlement settings with a class-level
override_settings decorator, remove each test’s _no_access_entitlements context
and try/finally blocks, and add an autouse fixture that clears
get_entitlements_backend’s cache before and after each test. Preserve the
existing test requests and assertions.
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.
Add API Entitlement + Keycloack
Summary by CodeRabbit
New Features
Bug Fixes