UN-3405 [FEAT] Add full_access permission tier for Platform API DELETE#1926
Open
Deepak-Kesavan wants to merge 1 commit intomainfrom
Open
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
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
Contributor
WalkthroughThis pull request introduces a new "full_access" permission level for platform API keys. The authorization middleware transitions from unconditionally blocking DELETE requests to conditionally allowing them based on API key permission—permitting only full_access keys to perform DELETE operations. The backend model adds the new permission choice, a migration updates the database schema, and the frontend UI supports the new permission option in creation and editing modals. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Middleware as Authorization Middleware
participant DB as Database
Client->>Middleware: DELETE /api/resource<br/>(with API key)
Middleware->>DB: Lookup API key & permission
DB-->>Middleware: Return permission (read, read_write,<br/>or full_access)
alt permission == full_access
Middleware-->>Client: 200 OK (proceed with DELETE)
else permission != full_access
Middleware-->>Client: 403 Forbidden<br/>(full_access required)
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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
|
Contributor
|
| Filename | Overview |
|---|---|
| backend/account_v2/custom_auth_middleware.py | Replaces unconditional pre-DB DELETE block with post-auth tier-aware gate; logic is correct for all three tiers but unknown permission values silently fall through as full_access |
| backend/platform_api/models.py | Adds FULL_ACCESS TextChoices member and widens max_length from 16 to 20; clean and correct |
| backend/platform_api/migrations/0002_add_full_access_permission.py | Non-destructive AlterField migration updating choices list and max_length; dependency chain and field values are correct |
| frontend/src/components/settings/platform-api-keys/PlatformApiKeys.jsx | Adds Full Access option to Create/Edit modals and refactors Tag render into a config map; minor: unknown permission falls back to "Read" style silently |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Bearer token request] --> B{Valid UUID & active key?}
B -- No --> C[401 Invalid/inactive key]
B -- Yes --> D{Org match?}
D -- No --> E[403 Org mismatch]
D -- Yes --> F{api_user linked?}
F -- No --> G[401 Missing service account]
F -- Yes --> H{permission == READ AND method not in SAFE_METHODS?}
H -- Yes --> I[403 read-only permission]
H -- No --> J{permission == READ_WRITE AND method == DELETE?}
J -- Yes --> K[403 requires full_access]
J -- No --> L[Attach user & pass to view]
L --> M{permission tier}
M -- read --> N[GET/HEAD/OPTIONS only]
M -- read_write --> O[All methods except DELETE]
M -- full_access --> P[All methods incl. DELETE]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: backend/account_v2/custom_auth_middleware.py
Line: 122-133
Comment:
**Unknown permission values fall through as full_access**
Any `PlatformApiKey` row whose `permission` column holds a value outside the three known enum members (e.g., a legacy value or a corrupted write) will pass both permission guards and be treated with `full_access` privileges — allowing DELETE. Adding an explicit allowlist/denylist guard makes this future-proof:
```python
# After the existing read-only check:
if key.permission not in (
ApiKeyPermission.READ,
ApiKeyPermission.READ_WRITE,
ApiKeyPermission.FULL_ACCESS,
):
logger.error("API key %s has unknown permission %r", key.id, key.permission)
return JsonResponse({"message": "API key has an unrecognized permission tier"}, status=403)
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: frontend/src/components/settings/platform-api-keys/PlatformApiKeys.jsx
Line: 272
Comment:
**Silent fallback to "Read" for unknown permission values**
`config[text] ?? config.read` silently renders any unrecognised permission value (e.g. a future tier added server-side before the frontend is updated) with "Read" styling. This could mislead an admin into thinking a key is read-only when it actually holds broader permissions. Falling back to showing the raw value is safer:
```suggestion
const { color, label } = config[text] ?? { color: "default", label: text };
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "UN-3405 [FEAT] Add full_access permissio..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+122
to
+133
| # Only full_access keys may DELETE; read_write keys cannot. | ||
| if ( | ||
| key.permission == ApiKeyPermission.READ_WRITE | ||
| and request.method == RequestMethod.DELETE | ||
| ): | ||
| return JsonResponse( | ||
| { | ||
| "message": "API key does not have DELETE permission; " | ||
| "requires full_access" | ||
| }, | ||
| status=403, | ||
| ) |
Contributor
There was a problem hiding this comment.
Any PlatformApiKey row whose permission column holds a value outside the three known enum members (e.g., a legacy value or a corrupted write) will pass both permission guards and be treated with full_access privileges — allowing DELETE. Adding an explicit allowlist/denylist guard makes this future-proof:
# After the existing read-only check:
if key.permission not in (
ApiKeyPermission.READ,
ApiKeyPermission.READ_WRITE,
ApiKeyPermission.FULL_ACCESS,
):
logger.error("API key %s has unknown permission %r", key.id, key.permission)
return JsonResponse({"message": "API key has an unrecognized permission tier"}, status=403)Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/account_v2/custom_auth_middleware.py
Line: 122-133
Comment:
**Unknown permission values fall through as full_access**
Any `PlatformApiKey` row whose `permission` column holds a value outside the three known enum members (e.g., a legacy value or a corrupted write) will pass both permission guards and be treated with `full_access` privileges — allowing DELETE. Adding an explicit allowlist/denylist guard makes this future-proof:
```python
# After the existing read-only check:
if key.permission not in (
ApiKeyPermission.READ,
ApiKeyPermission.READ_WRITE,
ApiKeyPermission.FULL_ACCESS,
):
logger.error("API key %s has unknown permission %r", key.id, key.permission)
return JsonResponse({"message": "API key has an unrecognized permission tier"}, status=403)
```
How can I resolve this? If you propose a fix, please make it concise.
| read_write: { color: "blue", label: "Read/Write" }, | ||
| read: { color: "default", label: "Read" }, | ||
| }; | ||
| const { color, label } = config[text] ?? config.read; |
Contributor
There was a problem hiding this comment.
config[text] ?? config.read silently renders any unrecognised permission value (e.g. a future tier added server-side before the frontend is updated) with "Read" styling. This could mislead an admin into thinking a key is read-only when it actually holds broader permissions. Falling back to showing the raw value is safer:
Suggested change
| const { color, label } = config[text] ?? config.read; | |
| const { color, label } = config[text] ?? { color: "default", label: text }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/components/settings/platform-api-keys/PlatformApiKeys.jsx
Line: 272
Comment:
**Silent fallback to "Read" for unknown permission values**
`config[text] ?? config.read` silently renders any unrecognised permission value (e.g. a future tier added server-side before the frontend is updated) with "Read" styling. This could mislead an admin into thinking a key is read-only when it actually holds broader permissions. Falling back to showing the raw value is safer:
```suggestion
const { color, label } = config[text] ?? { color: "default", label: text };
```
How can I resolve this? If you propose a fix, please make it concise.![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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.