UN-3405 [FEAT] Add full_access permission tier for Platform API DELETE#1926
Merged
athul-rs merged 9 commits intoMay 12, 2026
Merged
Conversation
Introduces a third ApiKeyPermission tier (full_access) that is a strict superset of read_write and additionally permits DELETE. The middleware previously blocked DELETE unconditionally for all bearer-auth keys; the check is now tier-aware, so keys holding full_access can reach DELETE routes while read_write keys remain restricted and read-only keys are unchanged. - backend/platform_api/models.py: add FULL_ACCESS TextChoices member; widen permission max_length 16 -> 20 - backend/platform_api/migrations/0002_add_full_access_permission.py: AlterField for updated choices + max_length - backend/account_v2/custom_auth_middleware.py: replace unconditional DELETE rejection with tier-aware gate (read_write blocked from DELETE, full_access allowed to fall through) - frontend/src/components/settings/platform-api-keys/PlatformApiKeys.jsx: add "Full Access" option in Create/Edit modals, refactor Tag render into a color/label config map
Contributor
|
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:
WalkthroughAdds a ChangesPlatform API key permission flow
Sequence DiagramsequenceDiagram
actor Client
participant Middleware as Authorization Middleware
participant DB as Database
participant App as Application
Client->>Middleware: HTTP request with Bearer API key
Middleware->>DB: Lookup PlatformApiKey by token
DB-->>Middleware: PlatformApiKey (permission)
Middleware->>Middleware: Validate permission value
alt permission valid and allows(method)
Middleware->>App: Forward request
App-->>Client: 2xx / resource response
else permission valid but disallows(method)
Middleware-->>Client: 403 Forbidden (method not allowed)
else permission invalid
Middleware-->>Client: 403 Forbidden (invalid permission)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Contributor
|
| Filename | Overview |
|---|---|
| backend/account_v2/custom_auth_middleware.py | Replaced blanket DELETE block with a per-tier allows() dispatch; adds unknown-permission guard and delegates permission logic cleanly to the model. |
| backend/platform_api/models.py | Adds FULL_ACCESS to ApiKeyPermission, implements allows() method on the enum, widens max_length to 20, and adds a CheckConstraint to enforce valid values at the DB level. |
| backend/platform_api/migrations/0002_add_full_access_permission.py | Safe AlterField migration adding the full_access choice and widening max_length from 16 to 20; no data backfill required. |
| backend/platform_api/migrations/0003_platformapikey_permission_check.py | Adds the platform_api_key_permission_valid CHECK constraint via AddConstraint; values are correctly hardcoded (static migration pattern) matching the model's ApiKeyPermission.values. |
| frontend/src/components/settings/platform-api-keys/PlatformApiKeys.jsx | Adds Full Access option to Create/Edit modals, refactors permission tag rendering into a config map with proper unknown-value fallback; tag color for full_access uses green but PR description specifies red. |
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
frontend/src/components/settings/platform-api-keys/PlatformApiKeys.jsx:42
The PR description specifies `full_access → red`, but the implementation uses `green`. Red better signals elevated privilege to admins (consistent with "danger" conventions), while green reads as "safe/allowed". An admin who quickly scans the key list might underestimate the blast radius of a `full_access` key if it renders with the same color used for successful/benign states.
```suggestion
{ value: "full_access", label: "Full Access", color: "red" },
```
Reviews (7): Last reviewed commit: "Merge branch 'main' into UN-3405-Impleme..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
frontend/src/components/settings/platform-api-keys/PlatformApiKeys.jsx (2)
426-436: Optional: extract the permission options to a shared constant.The same three
<Select.Option>s are duplicated between the Create (Lines 432-434) and Edit (Lines 478-480) modals, and the label strings also duplicate theconfigmap at Lines 267-271. Consider hoisting a singlePERMISSION_OPTIONSarray (or reusing theconfigmap) and rendering it in both modals so new tiers only need to be added in one place.♻️ Sketch of the refactor
+const PERMISSION_META = { + full_access: { color: "red", label: "Full Access" }, + read_write: { color: "blue", label: "Read/Write" }, + read: { color: "default", label: "Read" }, +}; +const PERMISSION_OPTIONS = [ + { value: "read_write", label: "Read/Write" }, + { value: "read", label: "Read" }, + { value: "full_access", label: "Full Access" }, +];Then use
PERMISSION_METAin the columnrenderand mapPERMISSION_OPTIONSinside both<Select>s.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/settings/platform-api-keys/PlatformApiKeys.jsx` around lines 426 - 436, Extract the duplicated permission options into a shared constant (e.g., PERMISSION_OPTIONS) and reuse the existing config map (or create PERMISSION_META) for display labels; replace the duplicated <Select.Option> lists in the Create and Edit modal Selects (the Form.Item with name="permission" / Select components) by mapping PERMISSION_OPTIONS, and update any column render that currently references the inline labels to use PERMISSION_META so labels stay consistent and new tiers are added in one place.
266-274: Minor: usegreencolor instead ofredfor thefull_accessTag.In Ant Design's semantic color conventions,
redis reserved for error/danger statuses, whilegreenis the standard for success and elevated privilege contexts. Usingredforfull_accesscontradicts these conventions and may mislead users into interpreting it as an error rather than an elevated permission tier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/settings/platform-api-keys/PlatformApiKeys.jsx` around lines 266 - 274, Update the Tag color mapping in the render function that builds the config object (the full_access key inside the config in PlatformApiKeys.jsx) to use "green" instead of "red"; locate the render method where config is defined (full_access/read_write/read) and change the color value for full_access so the Tag displays green for elevated permissions.backend/account_v2/custom_auth_middleware.py (1)
112-133: Consider a table-driven permission → allowed-methods mapping.The two sequential
ifblocks work, but each new tier will require another branch and the reviewer has to mentally union the gates to verify the matrix. A small mapping keeps the tier semantics in one place and makes future tiers (or per-endpoint refinements) a one-line change:♻️ Sketch
ALLOWED_METHODS_BY_PERMISSION = { ApiKeyPermission.READ: set(RequestMethod.SAFE_METHODS), ApiKeyPermission.READ_WRITE: set(RequestMethod.SAFE_METHODS) | {"POST", "PUT", "PATCH"}, ApiKeyPermission.FULL_ACCESS: set(RequestMethod.SAFE_METHODS) | {"POST", "PUT", "PATCH", "DELETE"}, } allowed = ALLOWED_METHODS_BY_PERMISSION.get(key.permission, set()) if request.method not in allowed: return JsonResponse( {"message": f"API key permission '{key.permission}' does not allow {request.method}"}, status=403, )Not a blocker — current code is correct — but worth considering before more tiers land.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/account_v2/custom_auth_middleware.py` around lines 112 - 133, Replace the two ad-hoc permission checks with a single table-driven mapping: create ALLOWED_METHODS_BY_PERMISSION mapping ApiKeyPermission -> allowed method set (use RequestMethod.SAFE_METHODS for safe methods and include "POST","PUT","PATCH","DELETE" where appropriate), then compute allowed = ALLOWED_METHODS_BY_PERMISSION.get(key.permission, set()) and return the existing JsonResponse error if request.method not in allowed; update the error message to include key.permission and request.method for clarity and keep references to key.permission, RequestMethod, ApiKeyPermission, request.method, JsonResponse and ALLOWED_METHODS_BY_PERMISSION when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/account_v2/custom_auth_middleware.py`:
- Around line 112-133: Replace the two ad-hoc permission checks with a single
table-driven mapping: create ALLOWED_METHODS_BY_PERMISSION mapping
ApiKeyPermission -> allowed method set (use RequestMethod.SAFE_METHODS for safe
methods and include "POST","PUT","PATCH","DELETE" where appropriate), then
compute allowed = ALLOWED_METHODS_BY_PERMISSION.get(key.permission, set()) and
return the existing JsonResponse error if request.method not in allowed; update
the error message to include key.permission and request.method for clarity and
keep references to key.permission, RequestMethod, ApiKeyPermission,
request.method, JsonResponse and ALLOWED_METHODS_BY_PERMISSION when making the
change.
In `@frontend/src/components/settings/platform-api-keys/PlatformApiKeys.jsx`:
- Around line 426-436: Extract the duplicated permission options into a shared
constant (e.g., PERMISSION_OPTIONS) and reuse the existing config map (or create
PERMISSION_META) for display labels; replace the duplicated <Select.Option>
lists in the Create and Edit modal Selects (the Form.Item with name="permission"
/ Select components) by mapping PERMISSION_OPTIONS, and update any column render
that currently references the inline labels to use PERMISSION_META so labels
stay consistent and new tiers are added in one place.
- Around line 266-274: Update the Tag color mapping in the render function that
builds the config object (the full_access key inside the config in
PlatformApiKeys.jsx) to use "green" instead of "red"; locate the render method
where config is defined (full_access/read_write/read) and change the color value
for full_access so the Tag displays green for elevated permissions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ef7bdb4-b157-4218-a4a3-0dd283af3688
📒 Files selected for processing (4)
backend/account_v2/custom_auth_middleware.pybackend/platform_api/migrations/0002_add_full_access_permission.pybackend/platform_api/models.pyfrontend/src/components/settings/platform-api-keys/PlatformApiKeys.jsx
- Middleware: add explicit allowlist guard so any PlatformApiKey row with a permission value outside the known tiers is rejected with 403 instead of silently falling through to full_access privileges. Logs an error so the bad row is discoverable. - Frontend: when the permission tag render encounters an unknown value, render the raw value with default color instead of masquerading as "Read" — avoids misleading admins about a key's actual privilege.
…-API-with-permission-tiers
jaseemjaskp
reviewed
May 11, 2026
Contributor
jaseemjaskp
left a comment
jaseemjaskp
left a comment
There was a problem hiding this comment.
Automated review (PR Review Toolkit) — focused on the new permission-tier logic. Highest-stakes finding: no test coverage exists for _authenticate_with_platform_key or the new permission gate (confirmed via grep — backend/account_v2/tests.py is a Django stub, backend/platform_api/ has no tests directory). A future refactor could silently grant or revoke DELETE for the wrong tier with no failing test.
Inline comments below are grouped High → Low. The two earlier @greptile-apps[bot] findings have been verified against the current code: the backend one (unknown-permission fallthrough) is fully addressed by lines 137-150; the frontend one ("silent fallback to Read") was code-changed and is now a different issue, re-raised below.
chandrasekharan-zipstack
approved these changes
May 11, 2026
AntD's semantic color convention reserves red for danger/error states and uses green for success/elevated contexts. Switch the full_access permission Tag from red to green so reviewers don't read it as an error indicator.
Address review feedback from @jaseemjaskp by moving the tier capability matrix onto ApiKeyPermission itself, validating the column at the database layer, and removing the now-redundant ad-hoc checks in middleware. Backend: - ApiKeyPermission gains an allows(method) method so any future caller (DRF permission classes, mgmt commands, internal tasks) can ask the enum directly instead of reimplementing the matrix. - Middleware collapses to two gates: (1) reject permission values outside ApiKeyPermission.values with a single source of truth, then (2) delegate to permission.allows(method). Error message uses the human label ("Read/Write", "Full Access") rather than the snake_case token. - Add CheckConstraint on platform_api_key.permission so invalid values cannot land via raw .create() / .update() — choices alone are advisory. - New migration 0003_platformapikey_permission_check. Frontend: - Hoist PERMISSION_OPTIONS / PERMISSION_CONFIG to module-level constants so the Create modal, Edit modal, and table tag share one source. Both Selects switch to AntD's options prop. - Unknown permission values now render as "Unknown: <value>" with the default color, so a future server-side tier doesn't silently masquerade as a known one.
Contributor
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 `@backend/platform_api/models.py`:
- Around line 14-20: The allows method on ApiKeyPermission is too permissive for
READ_WRITE because returning method != "DELETE" allows non-standard verbs;
update ApiKeyPermission.allows to use an explicit allowlist for READ_WRITE
(e.g., {"GET","HEAD","OPTIONS","POST","PUT","PATCH"}) rather than a negative
check, and normalize the incoming method (e.g., .upper()) before membership
testing; keep the existing FULL_ACCESS branch returning True and the default
read-only branch returning membership in {"GET","HEAD","OPTIONS"}.
🪄 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: CHILL
Plan: Pro
Run ID: 798e9f5c-4752-4e2c-8ff8-9c7e92f53843
📒 Files selected for processing (4)
backend/account_v2/custom_auth_middleware.pybackend/platform_api/migrations/0003_platformapikey_permission_check.pybackend/platform_api/models.pyfrontend/src/components/settings/platform-api-keys/PlatformApiKeys.jsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/settings/platform-api-keys/PlatformApiKeys.jsx
- backend/account_v2/custom_auth_middleware.py
…-API-with-permission-tiers
Address CodeRabbit follow-up: the previous `READ_WRITE` branch used `method != "DELETE"` which would have allowed non-standard verbs (TRACE, CONNECT, or custom methods) through the gate. Switch every tier to an explicit allowlist of the standard HTTP verbs and normalize the incoming method to uppercase so case variants don't slip past. `FULL_ACCESS` is unchanged in intent — it now lists every standard verb explicitly instead of returning True, keeping the allowlist pattern uniform across tiers and bounding the surface to the methods Django routes today.
The 403 response only reaches bearer-auth API callers (the gate is skipped on session-auth and the body is never rendered as UI text). Those callers already work with the snake_case identifier everywhere else: it's what GET /platform-api/keys/ returns, what they POST/PATCH, and what is stored in the DB. Surfacing 'Read/Write' forced them to mentally map back to 'read_write'. Switch the error message to use `permission.value` so the identifier matches the rest of the public API contract.
jaseemjaskp
reviewed
May 12, 2026
Contributor
jaseemjaskp
left a comment
jaseemjaskp
left a comment
There was a problem hiding this comment.
Second-pass review. One important finding remains — see inline. Most of the prior thread was addressed cleanly (capability matrix centralized on the enum, DB check constraint added, frontend drift fallback added, snake_case error token kept intentionally). Verified separately: migration 0002's varchar(16)→(20) is metadata-only on Postgres, AntD Select.options prop is supported in the pinned v5, the middleware not in values guard correctly prevents ValueError on the subsequent ApiKeyPermission(...) call, and no new StateStore leak is introduced.
jaseemjaskp
approved these changes
May 12, 2026
Contributor
jaseemjaskp
left a comment
jaseemjaskp
left a comment
There was a problem hiding this comment.
Re-reviewed after the latest round (262de1a, f3f1fed). Ran the PR Review Toolkit (Comment, Test, Silent-Failure, Type-Design, Code-Review, Simplifier) over the PR-scope files. Every material finding from the agents is either already in the existing comment thread (implicit-READ branch on ApiKeyPermission.allows(), missing tests on the new permission gate, snake_case in 403 message) or below the bar for a re-post (taste-level simplifications, minor docstring tightening). Nothing new on correctness, security, data loss, or contracts.
Resolving 7 of my prior threads that the current code addresses; leaving open the two that still represent live disagreements / deferrals (allows() exhaustiveness + missing tests).
LGTM.
…-API-with-permission-tiers
Contributor
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
Contributor
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
|
athul-rs
deleted the
UN-3405-Implement-DELETE-support-in-Platform-API-with-permission-tiers
branch
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
4 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.
What
full_accesspermission tier to Platform API keys (ApiKeyPermission) alongside the existingreadandread_writetiers.DELETErequests from bearer-auth keys that hold thefull_accesstier;read_writekeys can no longer DELETE;readkeys remain safe-methods only.Why
DELETEunconditionally for every Platform API key, so no key can ever delete a resource exposed to external callers.read_writeor removing the block outright.How
backend/platform_api/models.py— addFULL_ACCESS = "full_access", "Full Access"toApiKeyPermission; widenpermission.max_lengthfrom 16 to 20.backend/platform_api/migrations/0002_add_full_access_permission.py—AlterFieldreflecting the new choices and max_length.backend/account_v2/custom_auth_middleware.py— remove the pre-DB unconditional DELETE rejection; after the read-only gate, add a branch that returns 403 when aread_writekey attempts DELETE.full_accessfalls through to the normal request flow.frontend/src/components/settings/platform-api-keys/PlatformApiKeys.jsx— add a "Full Access"Select.Optionto both Create and Edit modals; refactor the permissionTagrender into a color/label config map (full_access→ red,read_write→ blue,read→ default).Tier matrix after this change:
readread_writefull_accessCan this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
readandread_writekeys keep their current behavior — non-DELETE verbs are unaffected, and DELETE was already forbidden for both tiers (previously via a blanket block, now via the tier-specific branch). The only behavior change is that keys newly issued withfull_accessare now permitted to issue DELETE requests.Database Migrations
backend/platform_api/migrations/0002_add_full_access_permission.pyadds thefull_accesschoice and widenspermission.max_lengthfrom 16 to 20. SafeAlterFieldon theplatform_api_keytable; no data backfill required since existing rows already holdreadorread_write.Env Config
Relevant Docs
full_accesstier can be noted inPLATFORM_API_REFERENCE.mdin a follow-up.)Related Issues or PRs
Dependencies Versions
Notes on Testing
python manage.py migrate platform_api.DELETEviacurl -X DELETE -H "Authorization: Bearer <key>" …against a route that supports DELETE:readkey → 403 "read-only permission".read_writekey → 403 "does not have DELETE permission; requires full_access".full_accesskey → request reaches the view (204 / resource-level response, not a middleware 403).read/read_writekeys are unchanged.Screenshots
Checklist
I have read and understood the Contribution Guidelines.