UN-2407 [FEAT] Add global API deployment keys for grouped API deployments#1881
Open
Deepak-Kesavan wants to merge 2 commits intomainfrom
Open
Conversation
…ents Add ability to generate a common API key that works across a group of API deployments, allowing users to manage shared access without creating individual keys per deployment.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbitRelease Notes
WalkthroughThis PR adds a global API deployment key system enabling organization admins to create and manage API keys that authenticate requests across multiple API deployments. Changes include a new Django app with models/views/serializers, backend modifications to deployment execution logic to validate global keys, frontend settings UI for managing keys, app registration, and routing configuration. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant DeploymentExecution as Deployment<br/>Execution Handler
participant DeploymentHelper as Deployment<br/>Helper
participant KeyHelper as Key<br/>Helper
participant Serializer as ExecutionRequest<br/>Serializer
participant Database as Database
Client->>DeploymentExecution: POST /api/v2/deployments/.../execution<br/>(with api_key)
DeploymentExecution->>DeploymentHelper: validate_and_process(api_deployment, api_key)
DeploymentHelper->>KeyHelper: validate_api(api_deployment, api_key)
alt Deployment-level key exists
KeyHelper->>Database: Query APIDeploymentKey
Database-->>KeyHelper: Found ✓
KeyHelper-->>DeploymentHelper: return False<br/>(is_global_key=False)
else Deployment-level key not found
KeyHelper->>KeyHelper: Try parsing api_key as UUID
KeyHelper->>Database: Query GlobalApiDeploymentKey by UUID
Database-->>KeyHelper: Found
KeyHelper->>Database: Verify deployment access<br/>(has_access_to_deployment)
Database-->>KeyHelper: Access granted ✓
KeyHelper-->>DeploymentHelper: return True<br/>(is_global_key=True)
end
DeploymentHelper->>DeploymentHelper: Create DeploymentExecutionDTO<br/>(api, api_key, is_global_key)
DeploymentHelper-->>DeploymentExecution: DTO with is_global_key flag
DeploymentExecution->>Serializer: Initialize with context<br/>(api, api_key, is_global_key)
Serializer->>Serializer: validate_llm_profile_id()
alt is_global_key = True
Serializer->>Database: Verify profile exists
Database-->>Serializer: ✓
Serializer->>Serializer: Skip ownership validation<br/>Return llm_profile_id
else is_global_key = False
Serializer->>Database: Fetch active API key<br/>& compare created_by
Database-->>Serializer: Ownership verified ✓
Serializer->>Serializer: Return llm_profile_id
end
Serializer-->>DeploymentExecution: Validated data ✓
DeploymentExecution-->>Client: HTTP 200 / Execution result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate 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
|
| Filename | Overview |
|---|---|
| backend/global_api_deployment_key/models.py | New model with UUID PK, M2M to APIDeployment, org FK, and correct has_access_to_deployment guard checking org match and allow_all flag. |
| backend/global_api_deployment_key/views.py | Clean ModelViewSet with org-scoped queryset, admin-only permission, and correct serializer dispatch per action; rotate correctly returns full key via DetailSerializer. |
| backend/global_api_deployment_key/serializers.py | Org-scoped queryset on api_deployments field is correct; however, no cross-field validation clears api_deployments when allow_all_deployments=True, leaving stale M2M rows via direct API calls. |
| backend/api_v2/key_helper.py | New validate_global_api_deployment_key validates UUID format, key existence+active status, and delegates org+deployment-scope check to has_access_to_deployment; TYPE_CHECKING import avoids circular dependency correctly. |
| backend/api_v2/deployment_helper.py | Fallback validation tries deployment-specific key first, catches UnauthorizedKey, then tries global key; returns bool flag to distinguish key type for downstream LLM profile ownership bypass. |
| frontend/src/components/settings/global-api-deployment-keys/GlobalApiDeploymentKeys.jsx | Full-featured CRUD UI; DeploymentScopeFields is defined inside the parent component causing React to remount it on every parent re-render, which can cause form state loss during field interactions. |
| frontend/src/routes/useMainAppRoutes.js | New route is correctly placed inside the RequireAdmin guard block alongside platform-api-keys, so client-side protection is consistent. |
| backend/global_api_deployment_key/migrations/0001_initial.py | Migration correctly creates the table with UUID PK, unique key field, M2M join table, org FK with CASCADE, and unique constraint on (name, organization). |
| backend/backend/settings/base.py | global_api_deployment_key added to SHARED_APPS alongside api_v2 and platform_api, which is consistent given TENANT_APPS is empty. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Client sends Bearer token to deployment endpoint] --> B[DeploymentHelper.validate_api]
B --> C{Deployment-specific key valid?}
C -->|Yes| D[is_global_key = False]
C -->|UnauthorizedKey| E[KeyHelper.validate_global_api_deployment_key]
E --> F{Global key found and active?}
F -->|No| G[Raise UnauthorizedKey - 401]
F -->|Yes| H{has_access_to_deployment?}
H -->|org mismatch or not in M2M list| G
H -->|allow_all=True or in M2M list| I[is_global_key = True]
D --> J[Build DeploymentExecutionDTO]
I --> J
J --> K{is_global_key?}
K -->|False| L[Validate per-user LLM profile ownership]
K -->|True| M[Skip ownership check - org-level access]
L --> N[Execute workflow]
M --> N
Prompt To Fix All With AI
This is a comment left during a code review.
Path: frontend/src/components/settings/global-api-deployment-keys/GlobalApiDeploymentKeys.jsx
Line: 1162-1192
Comment:
**Component defined inside parent causes remounting on each render**
`DeploymentScopeFields` is declared inside `GlobalApiDeploymentKeys`, so a **new function reference** is created on every render of the parent. React uses referential equality to identify component types; when the parent re-renders (e.g. `isLoading` toggles, `keys` or `deployments` state updates), React sees a different component type in the tree and **unmounts the previous `DeploymentScopeFields` and mounts a fresh one**.
For a form component this means:
- Any in-progress selection in the multi-select dropdown is wiped
- `Form.useWatch` re-registers, potentially firing an extra render
- Ant Design's `initialValue={true}` on `allow_all_deployments` is re-applied on each remount
The fix is to lift `DeploymentScopeFields` outside the parent function and pass `deployments` as a prop:
```js
// Outside GlobalApiDeploymentKeys
function DeploymentScopeFields({ form, deployments }) {
const allowAll = Form.useWatch("allow_all_deployments", form);
// ...same JSX...
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: backend/global_api_deployment_key/serializers.py
Line: 479-544
Comment:
**No cross-field validation: `allow_all_deployments=True` + non-empty `api_deployments` persists stale M2M rows**
Neither `GlobalApiDeploymentKeyCreateSerializer` nor `GlobalApiDeploymentKeyUpdateSerializer` has a cross-field `validate()` method. A direct API call with `{"allow_all_deployments": true, "api_deployments": ["<uuid>"]}` will persist entries in the M2M join table even though they are semantically irrelevant when `allow_all_deployments=True`.
The frontend defensively sends `api_deployments: []` in this case, but the backend contract is not enforced. `has_access_to_deployment` correctly gates on `allow_all_deployments` first so there is no auth bypass, but the join table accumulates orphaned rows and the API response will include them in the `api_deployments` list, which is confusing.
Adding a `validate()` method to both serializers would keep the data clean:
```python
def validate(self, attrs):
if attrs.get("allow_all_deployments"):
attrs["api_deployments"] = []
return attrs
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "UN-2407 [FIX] Address PR review comments..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+233
to
+239
| }; | ||
|
|
||
| const columns = [ | ||
| { | ||
| title: "Name", | ||
| dataIndex: "name", | ||
| key: "name", |
Contributor
There was a problem hiding this comment.
handleCopyKey always copies the masked key, not the real key
The retrieve action in views.py returns data serialized by GlobalApiDeploymentKeyListSerializer, whose get_key() method masks the value as "****-{last4}". So res?.data?.key will always be something like ****-abcd — not the actual UUID — making the copy button non-functional.
The full key is only exposed by GlobalApiDeploymentKeyDetailSerializer, which is used by the create and rotate responses. Consider adding a dedicated /keys/<pk>/reveal/ POST action that returns GlobalApiDeploymentKeyDetailSerializer so that retrieval of the full key is an intentional, audited action.
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/components/settings/global-api-deployment-keys/GlobalApiDeploymentKeys.jsx
Line: 233-239
Comment:
**`handleCopyKey` always copies the masked key, not the real key**
The `retrieve` action in `views.py` returns data serialized by `GlobalApiDeploymentKeyListSerializer`, whose `get_key()` method masks the value as `"****-{last4}"`. So `res?.data?.key` will always be something like `****-abcd` — not the actual UUID — making the copy button non-functional.
The full key is only exposed by `GlobalApiDeploymentKeyDetailSerializer`, which is used by the `create` and `rotate` responses. Consider adding a dedicated `/keys/<pk>/reveal/` POST action that returns `GlobalApiDeploymentKeyDetailSerializer` so that retrieval of the full key is an intentional, audited action.
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.
Actionable comments posted: 8
🧹 Nitpick comments (5)
backend/global_api_deployment_key/permissions.py (1)
1-3: Use a feature-specific permission class message.Current re-export will return the upstream message mentioning “platform API keys,” which is misleading for this endpoint.
♻️ Suggested adjustment
-from platform_api.permissions import IsOrganizationAdmin +from platform_api.permissions import IsOrganizationAdmin as PlatformIsOrganizationAdmin + + +class IsOrganizationAdmin(PlatformIsOrganizationAdmin): + message = "Only organization admins can manage global API deployment keys." __all__ = ["IsOrganizationAdmin"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/global_api_deployment_key/permissions.py` around lines 1 - 3, The re-export of IsOrganizationAdmin exposes the upstream error message mentioning “platform API keys”; create a small feature-specific subclass (e.g., GlobalAPIDeploymentKeyIsOrganizationAdmin) that inherits from IsOrganizationAdmin and overrides the permission error message/attribute to mention global API deployment keys (or this endpoint) and then export that subclass in __all__ instead of the upstream name; update any references to use the new subclass name so the endpoint shows the correct permission message.backend/api_v2/key_helper.py (1)
96-97: Add explicit exception chaining inexceptblocks raisingUnauthorizedKey.Use
raise UnauthorizedKey() from None(orfrom err) to make exception intent explicit and satisfy Ruff B904. This applies to lines 39, 97, and 104.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/api_v2/key_helper.py` around lines 96 - 97, Update the except blocks that currently do "except (ValueError, AttributeError): raise UnauthorizedKey()" to use explicit exception chaining; replace the raise with "raise UnauthorizedKey() from None" (or "from err" if you capture the exception as "except (ValueError, AttributeError) as err:") for each place that raises UnauthorizedKey (the three except blocks that currently raise UnauthorizedKey).frontend/src/components/settings/global-api-deployment-keys/GlobalApiDeploymentKeys.jsx (2)
352-382: MoveDeploymentScopeFieldsoutside the parent component.Defining
DeploymentScopeFieldsinsideGlobalApiDeploymentKeyscauses it to be re-created on every render, which can lead to:
- Loss of component state and focus when parent re-renders
- Unnecessary re-mounts affecting form validation state
Since
DeploymentScopeFieldsusesdeploymentsfrom the parent scope, pass it as a prop.♻️ Proposed refactor
Define
DeploymentScopeFieldsoutside the component (above or in a separate file):// Outside GlobalApiDeploymentKeys function function DeploymentScopeFields({ form, deployments }) { const allowAll = Form.useWatch("allow_all_deployments", form); return ( <> <Form.Item name="allow_all_deployments" valuePropName="checked" initialValue={true} > <Checkbox>Allow all API deployments</Checkbox> </Form.Item> <Form.Item name="api_deployments" label="Select API Deployments"> <Select mode="multiple" placeholder="Search and select deployments" optionFilterProp="children" className="gadk__deployment-select" showSearch disabled={allowAll !== false} > {deployments?.map((d) => ( <Select.Option key={d?.id} value={d?.id}> {d?.display_name || d?.api_name} {!d?.is_active && " (inactive)"} </Select.Option> ))} </Select> </Form.Item> </> ); }Then update usages:
- <DeploymentScopeFields form={createForm} /> + <DeploymentScopeFields form={createForm} deployments={deployments} />- <DeploymentScopeFields form={editForm} /> + <DeploymentScopeFields form={editForm} deployments={deployments} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/settings/global-api-deployment-keys/GlobalApiDeploymentKeys.jsx` around lines 352 - 382, DeploymentScopeFields is currently defined inside GlobalApiDeploymentKeys causing it to be recreated on every render; move the DeploymentScopeFields function out of the GlobalApiDeploymentKeys component (define it above or in a separate file) and accept deployments as a prop (i.e., change signature to DeploymentScopeFields({ form, deployments })) so it uses Form.useWatch("allow_all_deployments", form) but no longer closes over parent scope; update all usages inside GlobalApiDeploymentKeys to pass the form and deployments props accordingly to preserve state, focus and avoid unnecessary remounts.
161-175: Consider disabling the switch during status update to prevent double-clicks.
handleToggleStatusdoesn't prevent concurrent updates. Rapid toggling could cause race conditions or confusing UI states.♻️ Optional: Track loading state per row
const [updatingIds, setUpdatingIds] = useState(new Set()); const handleToggleStatus = (record) => { if (updatingIds.has(record?.id)) return; setUpdatingIds((prev) => new Set(prev).add(record?.id)); axiosPrivate({ // ... existing code }) .then(() => fetchKeys()) .catch((err) => setAlertDetails(handleException(err, "Failed to update key status")) ) .finally(() => { setUpdatingIds((prev) => { const next = new Set(prev); next.delete(record?.id); return next; }); }); }; // In render: <Switch size="small" checked={record?.is_active} loading={updatingIds.has(record?.id)} onChange={() => handleToggleStatus(record)} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/settings/global-api-deployment-keys/GlobalApiDeploymentKeys.jsx` around lines 161 - 175, handleToggleStatus currently allows rapid concurrent requests which can cause race conditions; add a per-row loading guard using a state like updatingIds (useState(new Set())) to ignore toggles when the id is present, set the id into updatingIds before calling axiosPrivate and remove it in finally, and pass updatingIds.has(record?.id) to the Switch component's loading prop while keeping onChange tied to handleToggleStatus so the switch is disabled/indicates loading during the update; ensure you still call fetchKeys() on success and handleException on error.backend/global_api_deployment_key/views.py (1)
75-81: Consider pagination for thedeploymentsendpoint.For organizations with many API deployments, returning all records without pagination could impact performance. This is acceptable for admin-only usage with small datasets, but may need pagination if deployment counts grow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/global_api_deployment_key/views.py` around lines 75 - 81, The deployments action currently returns all APIDeployment objects unpaginated; update the deployments method to use DRF pagination by calling self.paginate_queryset(APIDeployment.objects.filter(organization=organization)) and, if a page is returned, serialize that page with ApiDeploymentMinimalSerializer and return self.get_paginated_response(serializer.data); otherwise serialize the full queryset and return Response(serializer.data). Keep references to the existing method name deployments, model APIDeployment and serializer ApiDeploymentMinimalSerializer so the change is localized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/api_v2/serializers.py`:
- Around line 395-397: The code currently returns value when is_global_key is
true without checking the profile's org; instead, fetch the profile referenced
by llm_profile_id and compare its organization/tenant to the caller's
organization (obtainable from the serializer context request or passed tenant
id), and if they don't match raise a generic validation error; keep the
early-return for valid matches but replace the unconditional return in the
is_global_key branch with an org equality check that raises a
serializers.ValidationError (or the existing error type) on mismatch.
In `@backend/global_api_deployment_key/migrations/0001_initial.py`:
- Around line 71-81: The organization FK on this model (the "organization"
ForeignKey added in the migration, inherited via DefaultOrganizationMixin) is
nullable which allows duplicate (name, NULL) rows because SQL treats NULLs as
distinct; to fix, make organization non-nullable at the model/migration level
(set null=False, blank=False and remove default=None on the organization
ForeignKey) so the existing UniqueConstraint on (name, organization) enforces
per-organization uniqueness, and update the model and migration for
GlobalAPIDeploymentKey (or the model defined in this migration) accordingly; if
you intentionally need nullable orgs instead, instead add a partial
UniqueConstraint that applies only when organization IS NOT NULL or document why
nullability is required.
In `@backend/global_api_deployment_key/models.py`:
- Around line 16-18: The BooleanField allow_all_deployments is set to
default=True which makes new keys org-wide by default; change the field
declaration to use default=False on the models.BooleanField for
allow_all_deployments (preserving db_comment), add the corresponding schema
migration to update the default, and adjust any factory/tests that relied on the
permissive default (or explicitly set allow_all_deployments=True where intended)
so behavior remains explicit.
In `@backend/global_api_deployment_key/serializers.py`:
- Around line 72-74: The api_deployments PrimaryKeyRelatedField on
GlobalApiDeploymentKeyCreateSerializer is using APIDeployment.objects.all(),
allowing cross-organization assignment; override the serializer's __init__ to
accept the request/user/org context and set
self.fields['api_deployments'].queryset =
APIDeployment.objects.filter(organization=...) (use the org from
self.context['request'].user or passed context) so only same-organization
deployments are selectable; apply the identical change to
GlobalApiDeploymentKeyUpdateSerializer and ensure both serializers still allow
required=False and many=True.
In `@backend/global_api_deployment_key/views.py`:
- Around line 67-73: In the rotate method, update_fields is preventing Django's
auto_now on modified_at from running; change the save call in
GlobalApiDeploymentKeyView.rotate (currently instance.save(update_fields=["key",
"modified_by"])) to include "modified_at" in the update_fields list
(["key","modified_by","modified_at"]) or simply call instance.save() without
update_fields so modified_at is updated automatically after rotating the key.
In `@backend/middleware/request_id.py`:
- Around line 14-17: The middleware currently forces response.status_code = 200
for any request where "/api/v1/socket" appears in request.path and thus masks
real errors and is too broad; remove the status code rewrite and tighten the
match to an explicit path or prefix check (e.g., use request.path ==
"/api/v1/socket" or request.path.startswith("/api/v1/socket/") as appropriate)
so the code simply returns the response without mutating response.status_code;
update the condition in request_id.py to use the narrowed match and delete the
assignment to response.status_code to preserve real error semantics.
In `@backend/utils/log_events.py`:
- Around line 136-138: The start_server function currently returns the raw
Django WSGIHandler which bypasses Engine.IO/Socket.IO; restore the Socket.IO
wrapper by replacing the commented line with a call that wraps django_app in
socketio.WSGIApp(sio, django_app, socketio_path=namespace) and return that
wrapper (keep WSGIHandler typing intact), removing the commented-out line so
socket routes are handled by socketio rather than Django.
In `@frontend/src/routes/useMainAppRoutes.js`:
- Around line 218-221: The new route rendering GlobalApiDeploymentKeysPage is
not protected by admin-only gating; wrap this Route (the one with path
"settings/global-api-deployment-keys" and element <GlobalApiDeploymentKeysPage
/>) with the RequireAdmin protection (or move it inside the existing
admin-protected block that uses RequireAdmin) so only admins can access it;
ensure you reference the same Route and component names and keep any existing
Route props intact while nesting it under RequireAdmin.
---
Nitpick comments:
In `@backend/api_v2/key_helper.py`:
- Around line 96-97: Update the except blocks that currently do "except
(ValueError, AttributeError): raise UnauthorizedKey()" to use explicit exception
chaining; replace the raise with "raise UnauthorizedKey() from None" (or "from
err" if you capture the exception as "except (ValueError, AttributeError) as
err:") for each place that raises UnauthorizedKey (the three except blocks that
currently raise UnauthorizedKey).
In `@backend/global_api_deployment_key/permissions.py`:
- Around line 1-3: The re-export of IsOrganizationAdmin exposes the upstream
error message mentioning “platform API keys”; create a small feature-specific
subclass (e.g., GlobalAPIDeploymentKeyIsOrganizationAdmin) that inherits from
IsOrganizationAdmin and overrides the permission error message/attribute to
mention global API deployment keys (or this endpoint) and then export that
subclass in __all__ instead of the upstream name; update any references to use
the new subclass name so the endpoint shows the correct permission message.
In `@backend/global_api_deployment_key/views.py`:
- Around line 75-81: The deployments action currently returns all APIDeployment
objects unpaginated; update the deployments method to use DRF pagination by
calling
self.paginate_queryset(APIDeployment.objects.filter(organization=organization))
and, if a page is returned, serialize that page with
ApiDeploymentMinimalSerializer and return
self.get_paginated_response(serializer.data); otherwise serialize the full
queryset and return Response(serializer.data). Keep references to the existing
method name deployments, model APIDeployment and serializer
ApiDeploymentMinimalSerializer so the change is localized.
In
`@frontend/src/components/settings/global-api-deployment-keys/GlobalApiDeploymentKeys.jsx`:
- Around line 352-382: DeploymentScopeFields is currently defined inside
GlobalApiDeploymentKeys causing it to be recreated on every render; move the
DeploymentScopeFields function out of the GlobalApiDeploymentKeys component
(define it above or in a separate file) and accept deployments as a prop (i.e.,
change signature to DeploymentScopeFields({ form, deployments })) so it uses
Form.useWatch("allow_all_deployments", form) but no longer closes over parent
scope; update all usages inside GlobalApiDeploymentKeys to pass the form and
deployments props accordingly to preserve state, focus and avoid unnecessary
remounts.
- Around line 161-175: handleToggleStatus currently allows rapid concurrent
requests which can cause race conditions; add a per-row loading guard using a
state like updatingIds (useState(new Set())) to ignore toggles when the id is
present, set the id into updatingIds before calling axiosPrivate and remove it
in finally, and pass updatingIds.has(record?.id) to the Switch component's
loading prop while keeping onChange tied to handleToggleStatus so the switch is
disabled/indicates loading during the update; ensure you still call fetchKeys()
on success and handleException on error.
🪄 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: 52eb7baa-a489-4324-99a6-11e09058028a
⛔ Files ignored due to path filters (1)
backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
backend/api_v2/api_deployment_views.pybackend/api_v2/deployment_helper.pybackend/api_v2/dto.pybackend/api_v2/key_helper.pybackend/api_v2/serializers.pybackend/backend/settings/base.pybackend/backend/urls_v2.pybackend/global_api_deployment_key/__init__.pybackend/global_api_deployment_key/apps.pybackend/global_api_deployment_key/migrations/0001_initial.pybackend/global_api_deployment_key/migrations/__init__.pybackend/global_api_deployment_key/models.pybackend/global_api_deployment_key/permissions.pybackend/global_api_deployment_key/serializers.pybackend/global_api_deployment_key/urls.pybackend/global_api_deployment_key/views.pybackend/middleware/request_id.pybackend/pyproject.tomlbackend/utils/log_events.pyfrontend/src/components/navigations/side-nav-bar/SideNavBar.jsxfrontend/src/components/settings/global-api-deployment-keys/GlobalApiDeploymentKeys.cssfrontend/src/components/settings/global-api-deployment-keys/GlobalApiDeploymentKeys.jsxfrontend/src/pages/GlobalApiDeploymentKeysPage.jsxfrontend/src/routes/useMainAppRoutes.js
Comment on lines
+395
to
+397
| # Global API Keys are org-level; skip per-user ownership check | ||
| if is_global_key: | ||
| return value |
Contributor
There was a problem hiding this comment.
Enforce tenant-scoped profile authorization for global keys.
On Lines 395-397, global-key requests skip ownership checks and only require profile existence. That weakens isolation: a caller can potentially use any known llm_profile_id, including from another org. Add an explicit org-scoped authorization check before returning (and keep a generic error on mismatch).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/api_v2/serializers.py` around lines 395 - 397, The code currently
returns value when is_global_key is true without checking the profile's org;
instead, fetch the profile referenced by llm_profile_id and compare its
organization/tenant to the caller's organization (obtainable from the serializer
context request or passed tenant id), and if they don't match raise a generic
validation error; keep the early-return for valid matches but replace the
unconditional return in the is_global_key branch with an org equality check that
raises a serializers.ValidationError (or the existing error type) on mismatch.
Comment on lines
+71
to
+81
| ( | ||
| "organization", | ||
| models.ForeignKey( | ||
| blank=True, | ||
| db_comment="Foreign key reference to the Organization model.", | ||
| default=None, | ||
| null=True, | ||
| on_delete=django.db.models.deletion.CASCADE, | ||
| to="account_v2.organization", | ||
| ), | ||
| ), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how organization FKs are typically defined in this codebase
rg -n "ForeignKey.*organization" --type py -A3 -B1 | head -80Repository: Zipstack/unstract
Length of output: 43
🏁 Script executed:
# First, let's find and read the migration file mentioned
find . -path "*global_api_deployment_key/migrations/0001_initial.py" -type fRepository: Zipstack/unstract
Length of output: 123
🏁 Script executed:
# Search for the GlobalApiDeploymentKey model definition
rg -n "class GlobalApiDeploymentKey" --type py -A 30Repository: Zipstack/unstract
Length of output: 13214
🏁 Script executed:
# Check for viewsets related to global_api_deployment_key
fd -t f "viewset" | xargs rg -l "GlobalApiDeploymentKey" --type pyRepository: Zipstack/unstract
Length of output: 370
🏁 Script executed:
# Look for other models with nullable organization FKs to understand the pattern
rg -n "organization.*ForeignKey" --type py -B 2 -A 5Repository: Zipstack/unstract
Length of output: 1884
🏁 Script executed:
cat -n backend/global_api_deployment_key/migrations/0001_initial.py | head -100Repository: Zipstack/unstract
Length of output: 4113
🏁 Script executed:
# Check the full Meta constraints in the model
cat -n backend/global_api_deployment_key/models.py | tail -20Repository: Zipstack/unstract
Length of output: 945
🏁 Script executed:
# Check if there are any data migrations or model updates after the initial migration
ls -la backend/global_api_deployment_key/migrations/Repository: Zipstack/unstract
Length of output: 297
Organization FK nullability follows codebase pattern but allows theoretical duplicate names at DB level.
The organization FK inherits null=True, blank=True from DefaultOrganizationMixin, which is used throughout the codebase. Application controls prevent the theoretical issue: the viewset always filters by organization=UserContext.get_organization() and the create serializer validates name uniqueness per organization. However, the unique constraint on (name, organization) would allow duplicate names when organization is NULL in the database (since NULL != NULL in SQL uniqueness checks).
If organization should always be present at the application level, consider whether the mixin pattern should be reconsidered for this model, or document why nullability is needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/global_api_deployment_key/migrations/0001_initial.py` around lines 71
- 81, The organization FK on this model (the "organization" ForeignKey added in
the migration, inherited via DefaultOrganizationMixin) is nullable which allows
duplicate (name, NULL) rows because SQL treats NULLs as distinct; to fix, make
organization non-nullable at the model/migration level (set null=False,
blank=False and remove default=None on the organization ForeignKey) so the
existing UniqueConstraint on (name, organization) enforces per-organization
uniqueness, and update the model and migration for GlobalAPIDeploymentKey (or
the model defined in this migration) accordingly; if you intentionally need
nullable orgs instead, instead add a partial UniqueConstraint that applies only
when organization IS NOT NULL or document why nullability is required.
Comment on lines
+16
to
+18
| allow_all_deployments = models.BooleanField( | ||
| default=True, | ||
| db_comment="If True, this key can authenticate any API deployment in the org", |
Contributor
There was a problem hiding this comment.
Default scope is overly permissive.
Line 16/17 sets allow_all_deployments=True, so newly created keys become org-wide unless callers explicitly override it. That is risky for least-privilege and can cause accidental overexposure.
🔐 Proposed fix
allow_all_deployments = models.BooleanField(
- default=True,
+ default=False,
db_comment="If True, this key can authenticate any API deployment in the org",
)📝 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
| allow_all_deployments = models.BooleanField( | |
| default=True, | |
| db_comment="If True, this key can authenticate any API deployment in the org", | |
| allow_all_deployments = models.BooleanField( | |
| default=False, | |
| db_comment="If True, this key can authenticate any API deployment in the org", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/global_api_deployment_key/models.py` around lines 16 - 18, The
BooleanField allow_all_deployments is set to default=True which makes new keys
org-wide by default; change the field declaration to use default=False on the
models.BooleanField for allow_all_deployments (preserving db_comment), add the
corresponding schema migration to update the default, and adjust any
factory/tests that relied on the permissive default (or explicitly set
allow_all_deployments=True where intended) so behavior remains explicit.
Comment on lines
+218
to
+221
| <Route | ||
| path="settings/global-api-deployment-keys" | ||
| element={<GlobalApiDeploymentKeysPage />} | ||
| /> |
Contributor
There was a problem hiding this comment.
Guard this settings route with RequireAdmin.
This new admin-management page is currently outside the admin-protected block, so non-admin users can directly open the route path.
🔒 Proposed fix
<Route element={<RequireAdmin />}>
<Route path="users" element={<UsersPage />} />
<Route path="users/invite" element={<InviteEditUserPage />} />
<Route path="users/edit" element={<InviteEditUserPage />} />
<Route
path="settings/platform-api-keys"
element={<PlatformApiKeysPage />}
/>
+ <Route
+ path="settings/global-api-deployment-keys"
+ element={<GlobalApiDeploymentKeysPage />}
+ />
</Route>
- <Route
- path="settings/global-api-deployment-keys"
- element={<GlobalApiDeploymentKeysPage />}
- />📝 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
| <Route | |
| path="settings/global-api-deployment-keys" | |
| element={<GlobalApiDeploymentKeysPage />} | |
| /> | |
| <Route element={<RequireAdmin />}> | |
| <Route path="users" element={<UsersPage />} /> | |
| <Route path="users/invite" element={<InviteEditUserPage />} /> | |
| <Route path="users/edit" element={<InviteEditUserPage />} /> | |
| <Route | |
| path="settings/platform-api-keys" | |
| element={<PlatformApiKeysPage />} | |
| /> | |
| <Route | |
| path="settings/global-api-deployment-keys" | |
| element={<GlobalApiDeploymentKeysPage />} | |
| /> | |
| </Route> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/routes/useMainAppRoutes.js` around lines 218 - 221, The new
route rendering GlobalApiDeploymentKeysPage is not protected by admin-only
gating; wrap this Route (the one with path "settings/global-api-deployment-keys"
and element <GlobalApiDeploymentKeysPage />) with the RequireAdmin protection
(or move it inside the existing admin-protected block that uses RequireAdmin) so
only admins can access it; ensure you reference the same Route and component
names and keep any existing Route props intact while nesting it under
RequireAdmin.
Deepak-Kesavan
requested review from
chandrasekharan-zipstack,
jaags-dev and
muhammad-ali-e
- Revert unrelated changes: restore socketio WSGI wrapper in log_events.py and remove socket path middleware override - Security: scope api_deployments queryset to user's organization in both Create and Update serializers to prevent cross-org assignment - Move global-api-deployment-keys route inside RequireAdmin guard - Change allow_all_deployments default to False (least privilege) - Fix retrieve endpoint to return full key via DetailSerializer so copy-to-clipboard works with the actual key value - Include modified_at in rotate update_fields so timestamp updates
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
|
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
Why
How
Backend:
global_api_deployment_keywith model, views, serializers, URLs, and permissionsGlobalApiDeploymentKeymodel supportsallow_all_deployments(org-wide access) or explicit M2M assignment to specificAPIDeploymentinstances/api/v1/unstract/<org_id>/global-api-deployment/KeyHelper.validate_global_api_deployment_key()checks key existence, active status, org match, and deployment accessFrontend:
GlobalApiDeploymentKeyscomponent with full CRUD UI (create, edit, delete, rotate, copy key)/:orgName/settings/global-api-deployment-keysCan 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)
KeyHelperonly gets a new static method; existing key validation methods are untouched/api/v1/socketpathsDatabase Migrations
0001_initial.pycreates:global_api_deployment_keytable with UUID PK, name, description, key, is_active, allow_all_deployments, org FK, created_by FK, modified_by FKapi_deploymentsrelationshipEnv Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
Screenshots
N/A
Checklist
I have read and understood the Contribution Guidelines.