UN-2407 [FEAT] Add global API deployment keys for grouped API deployments#1881
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.
|
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 global API deployment keys end to end: backend auth now distinguishes global keys, the API exposes CRUD and rotation for them, and the frontend adds an admin settings page to manage keys. Safe-text validation is moved into a shared utility. ChangesGlobal API deployment key feature
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
|
| Filename | Overview |
|---|---|
| backend/global_api_deployment_key/serializers.py | Adds scoped create and update serializers for global deployment key data. |
| backend/global_api_deployment_key/views.py | Adds org-scoped management endpoints for global deployment keys. |
| backend/api_v2/deployment_helper.py | Adds fallback authentication through global deployment keys. |
| backend/api_v2/serializers.py | Adds global-key-aware profile validation during execution requests. |
| frontend/src/routes/useMainAppRoutes.js | Adds the global deployment key settings route under the admin settings area. |
| frontend/src/components/settings/api-key-manager/ApiKeyManager.jsx | Adds a shared API key management UI for list, copy, rotate, edit, and delete flows. |
Reviews (9): Last reviewed commit: "UN-2407 [FIX] Merge implicitly concatena..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| }; | ||
|
|
||
| const columns = [ | ||
| { | ||
| title: "Name", | ||
| dataIndex: "name", | ||
| key: "name", |
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}
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
| ( | ||
| "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", | ||
| ), | ||
| ), |
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.
| allow_all_deployments = models.BooleanField( | ||
| default=True, | ||
| db_comment="If True, this key can authenticate any API deployment in the org", |
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.
| 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.
| <Route | ||
| path="settings/global-api-deployment-keys" | ||
| element={<GlobalApiDeploymentKeysPage />} | ||
| /> |
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.
| <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
| description = models.TextField(max_length=512) | ||
| key = models.UUIDField(default=uuid.uuid4, unique=True) | ||
| is_active = models.BooleanField(default=True) | ||
| allow_all_deployments = models.BooleanField( |
There was a problem hiding this comment.
@Deepak-Kesavan why is this field present if we anyway intend to support access to all API deployments? Is it because we provide the option to also authenticate a subset of the projects identified by the M2M field?
There was a problem hiding this comment.
Yes, exactly. allow_all_deployments is a mode switch:
True→ the key authenticates every API deployment in the org.False→ the key is restricted to the specific subset listed in theapi_deploymentsM2M field.
You can see it in has_access_to_deployment() — it short-circuits to True when allow_all_deployments is set, otherwise it checks membership in the M2M list. So the field lets an admin choose between org-wide access and a scoped set of deployments under a single key.
chandrasekharan-zipstack
left a comment
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
Please add screenshots for the FE changes. Should we include a link or a hook in the existing API deployment's creation flow to easily create or use an existing global API key?
With the current changes, we'll have to hope the user stumbles upon it or we need to explicitly educate a few users that won't scale well. I suggest adding a tool tip and link to this new page from the existing API deployment creation modal
|
Two concerns with the current approach: 1. Separate table for what could be an extension of
2. Not extensible beyond deployments
At minimum, could we either generalize this model to support both resource types from the start, or scope this as a first step with a clear path to pipeline support? |
…ields - permissions.py: subclass IsOrganizationAdmin with a clearer denial message - GlobalApiDeploymentKeys.jsx: move DeploymentScopeFields to module scope and add PropTypes
# Conflicts: # backend/uv.lock # frontend/src/routes/useMainAppRoutes.js
…ope global-key profiles - Dedupe validate_safe_text/SAFE_TEXT_PATTERN into utils.input_sanitizer (was duplicated in platform_api and global_api_deployment_key serializers); add a docstring explaining the safe-char allow-list and why (reviewer ask) - api_v2: for global API keys, verify the llm_profile_id belongs to the deployment's organization before use (ProfileManager.objects is not org-scoped, so a caller could otherwise reference another org's profile). Generic 'Profile not found' avoids cross-org info disclosure (CodeRabbit) - Drop unrelated django-celery-beat 2.5.0->2.6.0 bump (reviewer ask)
The global_api_deployment_key app was branched before commit 64b06a7 marked DefaultOrganizationMixin.organization as editable=False (server- managed, never client input). After merging main, makemigrations detected model drift ('changes not yet reflected in a migration'). Add the matching AlterField migration, mirroring the same change already applied to api_v2, connector_v2, etc.
chandrasekharan-zipstack
left a comment
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
Reviewed the backend auth path, the new app, and the settings page in depth, and traced the import graph, the DTO consumers, and the validate_safe_text move.
The security core holds up
Saying this first, because the rest is a list of problems:
- Cross-org isolation is sound.
GlobalApiDeploymentKey.objects.get(key=...)is deliberately not org-scoped, which makeshas_access_to_deployment()'sorganization_idcheck the entire tenant boundary — and it's there, and it fires beforeallow_all_deploymentsis consulted. organizationcan't be client-injected —editable=False, set server-side inDefaultOrganizationMixin.save().is_global_keyis derived server-side, never read from the request.- The
validate_llm_profile_idswap is correct.ProfileManager.objectsgenuinely is not org-scoped, so the new org check really is load-bearing, and the generic"Profile not found"correctly avoids confirming another org's profile exists. - Pipelines are correctly excluded —
pipeline_v2has its own same-namedDeploymentHelpercallingKeyHelper.validate_api_keydirectly. Easy to misread; worth knowing it was checked. - The fallback doesn't mask real errors.
validate_api_keyonly ever raisesUnauthorizedKey, so a DBOperationalErrorstill surfaces as a 500 rather than being swallowed into a 401. - Moving
validate_safe_textintoutils/input_sanitizer.pydid not change the exception type — that module importsValidationErrorfromrest_framework.serializers, soplatform_apistill returns 400, not 500.
Blockers
-
No tests. This PR introduces four independent cross-tenant boundaries —
has_access_to_deployment()'s org check,get_queryset()'s org filter, theapi_deploymentsqueryset scoping, andvalidate_llm_profile_id's org equality — and not one is pinned by a test. Also unverified: rotate-invalidates-old-key,is_active=Falserejection, non-admin → 403, explicit-M2M rejecting unlisted deployments, and — highest traffic of all — that a valid deployment-scoped key still authenticates after thevalidate_apirewrite. The pattern to copy ispluggable_apps/manual_review_v2/tests/test_manual_review_views.py(APITestCase+force_authenticate+patch("utils.user_context.UserContext.get_organization")). Suggested homes:global_api_deployment_key/tests/{test_models,test_views,test_serializers}.pyand a newapi_v2/tests/test_deployment_auth.py. -
The UI mints org-wide keys by default — the model defaults
allow_all_deployments=False, the create form defaults it toTrue. Inline comment on the checkbox. -
rufffails, so CI will be red:I001 global_api_deployment_key/serializers.py:1 Import block is un-sorted I001 platform_api/serializers.py:1 Import block is un-sortedBoth auto-fixable with
ruff check --fix.
Themes in the inline comments
- Guarantee logging on every auth-rejection path. Three distinct rejection reasons currently collapse to a bare
raise UnauthorizedKey()with no log line and no exception chaining. Funnel them so no path can silently reject; keep the client response generic. - Let the ORM do the UUID coercion.
keyis aUUIDField, so the hand-rolleduuid.UUID()parse duplicatesto_python— and mirroringvalidate_api_key'sexcept (DoesNotExist, ValidationError)removes the parse, the aliased import, and a latentTypeError-on-Nonehole at once. - Don't throw away the authenticated key's identity.
validate_global_api_deployment_keyreturns the key object andvalidate_apicollapses it to a bool, so nothing can answer "which global key ran this execution?". - Incoherent scopes are representable and unvalidated in both serializers.
- Squash the two migrations — new app, neither shipped, and
0002emits no SQL. - Plus: the
"shown once"docstring is false, a mislabelled "non-critical" silent catch, hardcoded colours instead of theme tokens, and aTextField(max_length=...)that Postgres ignores.
I've also left a note on the model suggesting we eventually cover ETL and TASK pipelines for consistency, with the one privilege-escalation trap that decision carries.
Two things I checked that are not bugs
Flagging these so nobody re-derives them:
- The one-time key is not lost if the clipboard write fails.
handleCopyKeyre-fetches the full plaintext key fromretrieve(). Annoying, not destructive. initialValue={true}does not clobber the edit form.setFieldsValueruns synchronously before the Modal mounts its children, and rc-field-form skipsinitialValuewhen the store already holds a value. A scoped key is not silently widened on edit. (The create default in blocker 2 is a separate, real problem.)
Nice work overall — the hard part (the tenant boundary) is right. The gaps are around it.
- serializers.py: assigning specific deployments to a global key (allow_all_deployments=False) failed with 'Invalid pk ... does not exist' for valid same-org deployments. DRF validates a many=True relation against child_relation.queryset; the code set .queryset on the ManyRelatedField wrapper (a no-op), leaving validation at APIDeployment.objects.none(). Set child_relation.queryset in both Create and Update serializers. Found via live API testing. - GlobalApiDeploymentKeys.jsx: restore apostrophe in SAFE_TEXT_REGEX/message to match backend allow-list; the copy dropped it, rejecting valid names client-side. - views.py get_serializer_class: remove unreachable create/partial_update branches.
…lidation, cleanups Frontend (GlobalApiDeploymentKeys.jsx): - BLOCKER: default 'Allow all API deployments' to unchecked and coerce to false (was checked / ?? true) so Create no longer silently mints an org-wide key; require >=1 deployment when not allow-all. Add .catch() to validateFields. Backend: - serializers: cross-field validate() rejecting incoherent scopes; fix DetailSerializer docstring (full key retrievable for lifetime, not once). - key_helper: hoist import (no circular dep), drop uuid alias, rely on UUIDField coercion instead of manual parse. - deployment_helper: log the deployment->global key auth fallback + which named global key authorized (audit visibility). - models: description TextField->CharField(512); type-annotate + note load-bearing org check on has_access_to_deployment. - migrations: squash 0002 into 0001 (app unreleased).
…urface picker load error - dto/deployment_helper: carry the resolved GlobalApiDeploymentKey through DeploymentExecutionDTO instead of a bool; is_global_key becomes a property. Preserves 'which named key authorized this execution?' for audit. - key_helper: log the reason on every global-key rejection while keeping the client 401 generic; drop the unneeded select_related(organization). - frontend: surface a failed deployments fetch (was silently swallowed).
…lobal key screens Extract the common key-management screen into a shared ApiKeyManager (table, create/edit/rotate/delete/copy, name/description + safe-text validation); the platform and global key screens become thin wrappers injecting only their feature-specific column/fields/payload shaping. Shared CSS uses antd theme tokens instead of hardcoded rgba (dark-mode aware). Kills the SonarCloud new-code duplication and the CSS theming comment.
ruff-format had collapsed the two-line error message into two adjacent string literals on one line, which reads like a forgotten comma. Merge into a single literal (same resulting message).
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
|
Unstract test resultsPer-group results
Critical paths
|
jaseemjaskp
left a comment
jaseemjaskp
left a comment
There was a problem hiding this comment.
PR Review Toolkit — consolidated findings
Ran the full toolkit (code-reviewer, silent-failure-hunter, type-design-analyzer, pr-test-analyzer, comment-analyzer, code-simplifier) over the global-API-deployment-key feature. Findings that duplicate already-posted threads (retrieve-returns-full-key, dto audit identity, cross-field scope validation, least-privilege default, serializers.py:428 profile authz, the RequireAdmin route thread, modified_at on rotate) were dropped.
The implementation itself is sound — the org-scoping guard, the UnauthorizedKey-only fallback, UUID coercion, and per-branch logging were all independently verified as correct. The one blocking item is not a defect in the code but its complete absence of tests on a security-critical, cross-tenant authorization path — shipping that untested is the merge risk. Everything else is low / nit.
Summary
- 🔴 1 blocking: no tests for the global-key auth path (cross-org rejection, authz matrix, fallback ordering, malformed key, profile org check)
- 🟡 4 low: model-layer scope invariant unenforced; partial-PATCH scope traps; DTO leaks bearer token via repr; inaccurate sanitizer comment + missing
validate_safe_texttests - ⚪ 5 nits: serializer duplication, removable
destroyoverride,validateFieldscatch, static-rules hoist / column widths, frontend transform merge
Verdict: not approving solely on the untested auth path — add the auth tests and this is good to go.
| # the key up by ``key`` + ``is_active`` with NO org filter, so this is | ||
| # the only thing preventing a key from authenticating another org's | ||
| # deployment. | ||
| if self.organization_id != api_deployment.organization_id: |
There was a problem hiding this comment.
[blocking] The security-critical global-key auth path ships with zero tests. This organization_id != api_deployment.organization_id check is (per the comment right above it) the only guard preventing a valid, active key from Org A from authenticating a deployment in Org B — validate_global_api_deployment_key (key_helper.py:68) looks the key up by key + is_active=True with no org filter. There is no test in the diff (the new app has none, and test_deployment_helper.py doesn't touch this path) proving that guard holds, so a future refactor that drops/reorders it opens a silent cross-tenant auth hole.
Please add tests covering:
- Cross-org rejection: active
allow_all_deployments=Truekey in Org A →UnauthorizedKeyfor a deployment in Org B (andhas_access_to_deploymentreturnsFalse). - Authz matrix:
allow_all=Truesame-org → authorizes;allow_all=False+ deployment in M2M → authorizes; not in M2M → reject;is_active=False→ reject. - Fallback ordering (
deployment_helper.validate_api): a valid deployment-specific key returnsNoneand must not hit the global lookup; only anUnauthorizedKeyfalls through to the global key; both failing →UnauthorizedKeypropagates. - Malformed key: a non-UUID bearer token →
UnauthorizedKey, not a 500 (relies onUUIDFieldcoercion raisingValidationError). - Global-key LLM-profile org check (serializers.py:419-428): global key + same-org profile accepted; + cross-org profile →
Profile not found.
test_deployment_helper.py already has a clean mock-collaborator pattern these can reuse.
| description = models.CharField(max_length=512) | ||
| key = models.UUIDField(default=uuid.uuid4, unique=True) | ||
| is_active = models.BooleanField(default=True) | ||
| allow_all_deployments = models.BooleanField( |
There was a problem hiding this comment.
[low] The scope invariant lives only in the serializer, so an incoherent state is persistable. Nothing at the model layer (clean() / DB constraint) stops the ORM, Django admin, a data migration, or bulk_create from saving allow_all_deployments=True with a non-empty api_deployments list. At runtime has_access_to_deployment short-circuits on allow_all (line 62) and silently ignores the list — so a key an admin believes is scoped to two deployments would actually authenticate every deployment in the org. A CheckConstraint can't span the M2M and clean() can't see M2M pre-save, which is the tell that (bool + M2M) makes the coherent state structurally unenforceable.
Pragmatic fix: add a clean() rejecting the incoherent pair (covers admin/forms) and a comment that has_access_to_deployment intentionally lets allow_all win; longer term consider a single scope source of truth.
| def validate_description(self, value): | ||
| return validate_safe_text(value) | ||
|
|
||
| def validate(self, attrs): |
There was a problem hiding this comment.
[low] Two partial-PATCH traps in this scope validation (the UI hides both by always sending a shaped api_deployments: [], but direct API callers hit them):
- Can't flip a scoped key to allow-all in one field. PATCH
{"allow_all_deployments": true}alone falls back to the stored non-empty list, so_validate_deployment_scope(True, stored)raises "Leave empty when 'allow all deployments' is enabled" — an error about a field the caller never sent. - A stranded key becomes un-editable. If the only
APIDeploymenta key is scoped to is deleted, the M2M through-rows cascade away, leavingallow_all=False+ empty list. The key now authenticates nothing (fails closed — good), but every subsequent PATCH (including theis_activetoggle to deactivate it) fails_validate_deployment_scopeuntil re-scoped.
Fix: when effective allow_all resolves to True, treat the deployment list as empty for validation and auto-clear the M2M in update(); and allow an empty scope as a valid inert state so the key stays editable/deactivatable.
| """DTO for deployment execution viewset.""" | ||
|
|
||
| api: APIDeployment | ||
| api_key: str |
There was a problem hiding this comment.
[low] DeploymentExecutionDTO's auto-generated __repr__ includes the raw bearer token. Because this is a plain @dataclass, any logger.debug("...%s", dto) or an exception repr on this object writes the live api_key credential into logs. Fix: api_key: str = field(repr=False) (import field from dataclasses), or drop api_key from the DTO entirely since downstream reads api/global_key. Marking the DTO frozen=True while you're here would make its single-construction, read-only nature explicit.
| # descriptions). Unlike ``validate_no_html_tags`` (which block-lists known | ||
| # dangerous constructs), this is a strict allow-list: alphanumerics, spaces and | ||
| # a small set of common punctuation. Everything else is rejected so no HTML | ||
| # angle brackets (``<``/``>``), quotes, ampersands, backticks or other |
There was a problem hiding this comment.
[low] This rationale is inaccurate and reassuring about a property the code doesn't have. The comment lists "quotes" among the rejected characters (and the docstring at line 69 repeats it), but SAFE_TEXT_PATTERN allows the apostrophe ' — a single quote that does break out of a single-quoted HTML attribute (title='...'). The SAFE_TEXT_ERROR string itself advertises apostrophes as allowed. A maintainer who trusts "no quotes reach storage" and embeds one of these values in a single-quoted attribute would be wrong. Reword to "double quotes and backticks are rejected; the apostrophe is allowed, so single-quoted attribute contexts must still escape it." The same self-contradictory wording is mirrored at ApiKeyManager.jsx:36.
Separately: validate_safe_text (line 64) has zero direct tests even though its siblings in this module are exhaustively covered in test_input_sanitizer.py. Add a TestValidateSafeText in the same style: whitespace-only → empty error, <script>/&/" → SAFE_TEXT_ERROR, and " x " → "x".
| model = GlobalApiDeploymentKey | ||
| fields = ["name", "description", "allow_all_deployments", "api_deployments"] | ||
|
|
||
| def __init__(self, *args, **kwargs): |
There was a problem hiding this comment.
[nit] The api_deployments field declaration, this entire __init__ (identical child_relation.queryset org-scoping plus its identical comment block), and validate_description are byte-for-byte duplicated in GlobalApiDeploymentKeyUpdateSerializer. Extract a shared _GlobalApiKeyWriteSerializer(AuditSerializer) base holding the field, __init__, and validate_description; each subclass keeps its own Meta/validate. DRF's metaclass still collects the declared field from the base, so behavior is unchanged and ~30 duplicated lines go away. (Nice work already extracting the shared ApiKeyManager and validate_safe_text.)
| response_serializer = GlobalApiDeploymentKeyListSerializer(updated_instance) | ||
| return Response(response_serializer.data) | ||
|
|
||
| def destroy(self, request, *args, **kwargs): |
There was a problem hiding this comment.
[nit] This destroy override reproduces ModelViewSet.destroy/perform_destroy exactly (get_object → delete() → 204). Deleting it is behavior-identical and drops a method. (retrieve/create/partial_update/rotate earn their overrides — they swap serializers.)
| .finally(() => setIsSaving(false)); | ||
| }) | ||
| .catch(() => { | ||
| /* Invalid form: antd renders inline field errors; swallow the reject. */ |
There was a problem hiding this comment.
[nit] This bare .catch is scoped to the whole outer chain, not just the antd validation reject. setIsSaving(true) runs before the axios call but setIsSaving(false) lives only in the inner chain's .finally(), so a non-validation error thrown synchronously in the .then body (e.g. from transformPayload) would be swallowed here and leave the modal stuck in confirmLoading with no feedback. Low likelihood today, but cheap to make robust: discriminate the reject — if (err?.errorFields) return; then surface via setAlertDetails(handleException(err, ...)) and reset isSaving. Applies to the matching handleEdit catch too.
| }, | ||
| ]; | ||
|
|
||
| const nameAndDescriptionRules = { |
There was a problem hiding this comment.
[nit] nameAndDescriptionRules is fully static (no props/state/closures) yet is rebuilt on every render. Hoist it to a module-level constant next to SAFE_TEXT_REGEX/SAFE_TEXT_MESSAGE. (Unlike columns, which closes over handlers and must stay in-render.) Minor related cosmetic: the base column width percentages already total ~88%, so injecting scopeColumn (14%) pushes the table to ~102% — trim a base width so base + one extra column lands at 100%.
| allow_all_deployments: record?.allow_all_deployments, | ||
| api_deployments: record?.api_deployments?.map((d) => d?.id) || [], | ||
| })} | ||
| transformCreatePayload={(values) => ({ |
There was a problem hiding this comment.
[nit] transformCreatePayload and transformEditPayload differ only in the create one defaulting allow_all_deployments ?? false. Since the edit form always supplies that field (initial value from the record), a single shared transform that includes the ?? false default is behavior-equivalent for both and removes a function.
3 participants
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
Checklist
I have read and understood the Contribution Guidelines.