Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis change introduces a new HTTP POST route handler for Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant DB
participant AuditLog
Client->>Handler: POST /v2/keys.deleteKey (with keyId, Permanent flag)
Handler->>DB: Authenticate root key
Handler->>DB: Fetch key by ID
alt Key not found or not in workspace
Handler-->>Client: 404 Not Found / 403 Forbidden
else Permission check fails
Handler-->>Client: 403 Forbidden
else Permanent flag is true
Handler->>DB: DeleteKeyByID (hard delete, cascade)
Handler->>AuditLog: Log permanent deletion
Handler-->>Client: 200 OK (empty data)
else Soft delete
Handler->>DB: SoftDeleteKeyByID (set deleted_at_m)
Handler->>AuditLog: Log soft deletion
Handler-->>Client: 200 OK (empty data)
end
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningsgo/pkg/db/querier_generated.go (1)🧬 Code Graph Analysis (1)go/pkg/db/querier_generated.go (1)
⏰ Context from checks skipped due to timeout of 90000ms (24)
🔇 Additional comments (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
go/apps/api/routes/v2_keys_delete_key/200_test.go(1 hunks)go/apps/api/routes/v2_keys_delete_key/400_test.go(1 hunks)go/apps/api/routes/v2_keys_delete_key/401_test.go(1 hunks)go/apps/api/routes/v2_keys_delete_key/403_test.go(1 hunks)go/apps/api/routes/v2_keys_delete_key/404_test.go(1 hunks)go/apps/api/routes/v2_keys_delete_key/handler.go(1 hunks)go/pkg/db/key_delete_by_id.sql_generated.go(1 hunks)go/pkg/db/key_soft_delete_by_id.sql_generated.go(1 hunks)go/pkg/db/querier_generated.go(2 hunks)go/pkg/db/queries/key_delete_by_id.sql(1 hunks)go/pkg/db/queries/key_soft_delete_by_id.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: chronark
PR: unkeyed/unkey#2693
File: apps/api/src/routes/v1_keys_updateKey.ts:350-368
Timestamp: 2024-11-29T15:15:47.308Z
Learning: In `apps/api/src/routes/v1_keys_updateKey.ts`, the code intentionally handles `externalId` and `ownerId` separately for clarity. The `ownerId` field will be removed in the future, simplifying the code.
go/apps/api/routes/v2_keys_delete_key/400_test.go (2)
Learnt from: MichaelUnkey
PR: unkeyed/unkey#2114
File: apps/api/src/routes/v1_keys_updateKey.error.test.ts:0-0
Timestamp: 2024-09-27T15:20:05.475Z
Learning: In the `v1/keys.updateKey` endpoint, the server validates the refill configuration before checking if the key exists. Therefore, tests can assert validation errors without needing to create the key first.
Learnt from: Flo4604
PR: unkeyed/unkey#2955
File: go/apps/api/routes/v2_identities_create_identity/handler.go:162-202
Timestamp: 2025-03-19T09:25:59.751Z
Learning: In the Unkey codebase, input validation for API endpoints is primarily handled through OpenAPI schema validation, which occurs before requests reach the handler code. For example, in the identities.createIdentity endpoint, minimum values for ratelimit duration and limit are defined in the OpenAPI schema rather than duplicating these checks in the handler.
go/pkg/db/queries/key_delete_by_id.sql (2)
Learnt from: chronark
PR: unkeyed/unkey#3420
File: go/pkg/hydra/store/gorm/gorm.go:486-498
Timestamp: 2025-07-02T11:51:58.572Z
Learning: The Hydra package (go/pkg/hydra) is planned to be migrated from GORM to sqlc for database operations, which explains why raw SQL queries are acceptable in the current implementation.
Learnt from: Flo4604
PR: unkeyed/unkey#3151
File: go/apps/api/openapi/gen.go:221-233
Timestamp: 2025-04-18T20:01:33.812Z
Learning: For identity deletion operations in the Unkey API, identityId takes precedence over externalId when both are provided in the request body.
go/apps/api/routes/v2_keys_delete_key/404_test.go (1)
Learnt from: MichaelUnkey
PR: unkeyed/unkey#2114
File: apps/api/src/routes/v1_keys_updateKey.error.test.ts:0-0
Timestamp: 2024-09-27T15:20:05.475Z
Learning: In the `v1/keys.updateKey` endpoint, the server validates the refill configuration before checking if the key exists. Therefore, tests can assert validation errors without needing to create the key first.
go/pkg/db/querier_generated.go (1)
Learnt from: Flo4604
PR: unkeyed/unkey#3151
File: go/apps/api/openapi/gen.go:221-233
Timestamp: 2025-04-18T20:01:33.812Z
Learning: For identity deletion operations in the Unkey API, identityId takes precedence over externalId when both are provided in the request body.
🧬 Code Graph Analysis (1)
go/pkg/db/querier_generated.go (1)
go/pkg/db/key_soft_delete_by_id.sql_generated.go (1)
SoftDeleteKeyByIDParams(17-20)
🔇 Additional comments (10)
go/pkg/db/queries/key_delete_by_id.sql (1)
1-8: LGTM! Well-structured multi-table DELETE query.The SQL query correctly implements permanent key deletion by:
- Using LEFT JOINs to include all related tables that reference the key
- Cleaning up associated data from
keys_permissions,keys_roles,ratelimits, andencrypted_keystables- Using parameterized queries for security
- Performing the deletion atomically in a single operation
go/pkg/db/queries/key_soft_delete_by_id.sql (1)
1-2: LGTM! Correct soft delete implementation.The query properly implements soft deletion by updating the
deleted_at_mtimestamp while preserving all related data. The parameterized approach ensures type safety and prevents SQL injection.go/pkg/db/key_delete_by_id.sql_generated.go (1)
22-34: LGTM! Generated code follows established patterns.The
DeleteKeyByIDmethod is properly generated with:
- Consistent method signature using context and DBTX interface
- Appropriate error handling
- Clear documentation including the SQL query
- Follows the same patterns as other sqlc-generated deletion methods
go/pkg/db/key_soft_delete_by_id.sql_generated.go (1)
17-28: LGTM! Proper parameter structure and method implementation.The generated code correctly implements soft deletion with:
- Well-defined
SoftDeleteKeyByIDParamsstruct using appropriate Go typessql.NullInt64for the timestamp field allows for proper null handling- Method signature consistent with other parameterized sqlc methods
- Proper error handling and context usage
go/pkg/db/querier_generated.go (1)
17-26: LGTM! Interface updates are consistent and well-documented.The new deletion methods are properly added to the
Querierinterface with:
- Method signatures that exactly match the generated implementations
- Comprehensive documentation including the underlying SQL queries
- Logical placement within the interface structure
- Consistent naming and parameter patterns with existing methods
Also applies to: 829-832
go/apps/api/routes/v2_keys_delete_key/401_test.go (1)
86-143: Additional context needed: please show the implementation ofVerifyRootKeyingo/internal/services/keys/verify_root_key.go, especially how it handles errors after extracting the bearer token and when the key does not exist. This will let me confirm whether malformed headers map to 400 and missing keys to 401.go/apps/api/routes/v2_keys_delete_key/handler.go (1)
46-171: Well-implemented handler with proper security and error handling.The handler correctly implements:
- Authentication via root key verification
- Authorization with granular permission checks
- Workspace validation to prevent cross-workspace access
- Transaction-based deletion with audit logging
- Proper error handling with appropriate fault codes
The use of
ptr.SafeDereffor the optional permanent flag is a good defensive programming practice.go/apps/api/routes/v2_keys_delete_key/403_test.go (3)
1-18: LGTM! Clean imports and package structure.The imports are well-organized and include all necessary dependencies for testing the delete key endpoint's permission validation.
131-189: Excellent test coverage for permission scenarios.The test cases comprehensively cover all forbidden access scenarios:
- No permissions
- Wrong permissions (create vs delete)
- Cross-workspace access
- Cross-API access
Each test properly validates both the HTTP status code and the presence of an error response body.
172-172: Cross-workspace access correctly returns 404The handler’s workspace-scoping intentionally treats keys in other workspaces as “not found” before any permission check:
- In handler.go (around line 33), you’ll see:
if key.WorkspaceID != auth.AuthorizedWorkspaceID { return fault.New("key not found", fault.Code(codes.Data.Key.NotFound.URN()), fault.Internal("key belongs to different workspace"), fault.Public("The specified key was not found."), ) }- The cross-workspace test in 403_test.go asserts a 404, matching this logic.
- Other forbidden scenarios (missing permissions) occur later in the flow and rightly return 403.
No changes required.
chronark
left a comment
chronark
left a comment
There was a problem hiding this comment.
a few code rabbit comments are good, let's fix those before merging
2 participants
What does this PR do?
For #3419
Type of change
How should this be tested?
Unit tests
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes
Tests