fix: updating a key's action should not work when it has been deleted#2897
fix: updating a key's action should not work when it has been deleted#2897chronark merged 3 commits intounkeyed:mainfrom
Conversation
Nelwhix
requested review from
MichaelUnkey,
chronark,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@Nelwhix is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
|
Thank you for following the naming conventions for pull request titles! 🙏 |
📝 WalkthroughWalkthroughThis pull request updates multiple API routes that manage key operations by modifying database queries. The updated queries now include an additional condition to check that the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant A as API Endpoint
participant DB as Database
participant R as Response Handler
C->>A: Send request to update key
A->>DB: Query key with conditions (workspaceId, keyId, isNull(deletedAt))
DB-->>A: Return key record or null
alt Key Found
A->>R: Process update
R-->>C: Return success response
else Key Not Found
A->>R: Return 404 error with key ID not found
R-->>C: 404 Error Response
end
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
apps/api/src/routes/v1_keys_updateRemaining.ts (2)
164-166: 🛠️ Refactor suggestionAdd deletedAt check to final key lookup.
For consistency, the final key lookup should also include the
deletedAtcheck to ensure we don't retrieve a key that was deleted during the update operation.const keyAfterUpdate = await db.readonly.query.keys.findFirst({ - where: (table, { eq }) => eq(table.id, req.keyId), + where: (table, { eq, isNull, and }) => + and(eq(table.id, req.keyId), isNull(table.deletedAt)), });
164-166: 🛠️ Refactor suggestionAdd deletedAt check to verification query.
For consistency, the verification query after the update should also include the
deletedAtcheck.const keyAfterUpdate = await db.readonly.query.keys.findFirst({ - where: (table, { eq }) => eq(table.id, req.keyId), + where: (table, { eq, isNull, and }) => and(eq(table.id, req.keyId), isNull(table.deletedAt)), });
🧹 Nitpick comments (3)
apps/api/src/routes/v1_keys_updateKey.ts (3)
296-299: Consider improving the error message for deleted keys.The current error message doesn't distinguish between non-existent keys and deleted keys, which might confuse API consumers.
- message: `key ${req.keyId} not found`, + message: `key ${req.keyId} not found or has been deleted`,
328-339: Consider extracting validation logic into a separate function.The validation logic for refill settings could be more maintainable if extracted into a dedicated function.
+const validateRefillSettings = (req: V1KeysUpdateKeyRequest) => { + if (req.remaining === null && req.refill) { + throw new UnkeyApiError({ + code: "BAD_REQUEST", + message: "Cannot set refill on a key with unlimited requests", + }); + } + if (req.refill?.interval === "daily" && req.refill.refillDay) { + throw new UnkeyApiError({ + code: "BAD_REQUEST", + message: "Cannot set 'refillDay' if 'interval' is 'daily'", + }); + } +} + // In the route handler: -if (req.remaining === null && req.refill) { - throw new UnkeyApiError({ - code: "BAD_REQUEST", - message: "Cannot set refill on a key with unlimited requests", - }); -} -if (req.refill?.interval === "daily" && req.refill.refillDay) { - throw new UnkeyApiError({ - code: "BAD_REQUEST", - message: "Cannot set 'refillDay' if 'interval' is 'daily'", - }); -} +validateRefillSettings(req);
343-405: Consider using a more type-safe approach for tracking changes.The current implementation uses a generic
Partial<Key>type for tracking changes. Consider using a more specific type that only includes the fields that can be updated.+type UpdateableKeyFields = Pick<Key, + 'name' | 'meta' | 'identityId' | 'ownerId' | 'expires' | + 'remaining' | 'ratelimitAsync' | 'ratelimitLimit' | + 'ratelimitDuration' | 'refillAmount' | 'refillDay' | + 'lastRefillAt' | 'enabled' +>; -const changes: Partial<Key> = {}; +const changes: Partial<UpdateableKeyFields> = {};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/api/src/routes/v1_keys_addPermissions.ts(1 hunks)apps/api/src/routes/v1_keys_addRoles.ts(1 hunks)apps/api/src/routes/v1_keys_removePermissions.ts(1 hunks)apps/api/src/routes/v1_keys_removeRoles.ts(1 hunks)apps/api/src/routes/v1_keys_setPermissions.ts(1 hunks)apps/api/src/routes/v1_keys_setRoles.ts(1 hunks)apps/api/src/routes/v1_keys_updateKey.error.test.ts(1 hunks)apps/api/src/routes/v1_keys_updateKey.ts(1 hunks)apps/api/src/routes/v1_keys_updateRemaining.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
apps/api/src/routes/v1_keys_updateKey.error.test.ts (1)
Learnt from: MichaelUnkey
PR: unkeyed/unkey#2114
File: apps/api/src/routes/v1_keys_updateKey.error.test.ts:0-0
Timestamp: 2024-11-10T16:45:16.994Z
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.
🔇 Additional comments (17)
apps/api/src/routes/v1_keys_updateKey.error.test.ts (2)
82-117: LGTM! Well-structured test case.The test case effectively verifies that attempting to update a deleted key returns a 404 response, aligning with the PR objective.
82-117: LGTM! Well-structured test case.The test case effectively verifies that attempting to update a deleted key returns a 404 response, which aligns with the PR objective.
apps/api/src/routes/v1_keys_removeRoles.ts (2)
88-93: LGTM! Correctly prevents role removal from deleted keys.The added
isNull(table.deletedAt)check ensures that role removal operations cannot be performed on deleted keys.
88-93: LGTM! Correctly prevents role removal from deleted keys.The query now includes the
deletedAtcheck, ensuring that roles cannot be removed from soft-deleted keys.apps/api/src/routes/v1_keys_removePermissions.ts (2)
88-93: LGTM! Correctly prevents permission removal from deleted keys.The added
isNull(table.deletedAt)check ensures that permission removal operations cannot be performed on deleted keys.
88-93: LGTM! Correctly prevents permission removal from deleted keys.The query now includes the
deletedAtcheck, ensuring that permissions cannot be removed from soft-deleted keys.apps/api/src/routes/v1_keys_updateRemaining.ts (2)
70-71: LGTM! Correctly prevents updating remaining count for deleted keys.The added
isNull(table.deletedAt)check ensures that remaining count updates cannot be performed on deleted keys.
71-71: LGTM! Correctly prevents updating remaining count for deleted keys.The query now includes the
deletedAtcheck, ensuring that the remaining count cannot be updated for soft-deleted keys.apps/api/src/routes/v1_keys_addPermissions.ts (1)
102-107: LGTM! Properly prevents operations on deleted keys.The query now correctly checks for
deletedAtbeing null, ensuring that permissions cannot be added to soft-deleted keys.apps/api/src/routes/v1_keys_addRoles.ts (1)
115-120: LGTM! Properly prevents operations on deleted keys.The query now correctly checks for
deletedAtbeing null, ensuring that roles cannot be added to soft-deleted keys.apps/api/src/routes/v1_keys_setRoles.ts (1)
129-134: LGTM! Properly prevents operations on deleted keys.The query now correctly checks for
deletedAtbeing null, ensuring that roles cannot be set on soft-deleted keys.apps/api/src/routes/v1_keys_setPermissions.ts (1)
133-138: LGTM! Properly prevents operations on deleted keys.The query now correctly checks for
deletedAtbeing null, ensuring that permissions cannot be set on soft-deleted keys.apps/api/src/routes/v1_keys_updateKey.ts (5)
284-293: LGTM! Query now correctly handles soft-deleted keys.The addition of
isNull(table.deletedAt)ensures that soft-deleted keys cannot be updated, which aligns with the PR objective.
295-307: Verify if identical error messages are intentional.Both the "not found" and "wrong workspace" scenarios return the same error message. While this might be a security measure to prevent workspace enumeration, it could make debugging harder.
Please confirm if this is intentional. If not, consider using different error messages:
if (key.workspaceId !== auth.authorizedWorkspaceId) { throw new UnkeyApiError({ code: "NOT_FOUND", - message: `key ${req.keyId} not found`, + message: `key ${req.keyId} does not belong to this workspace`, }); }
309-326: LGTM! Robust RBAC implementation.The RBAC evaluation is thorough, with proper error handling and clear permission checks.
406-437: LGTM! Thorough audit logging and cache invalidation.The implementation properly handles:
- Audit logging with detailed metadata
- Cache invalidation for both key hash and ID
439-446: LGTM! Efficient role and permission updates.The implementation efficiently handles role and permission updates using Promise.all for concurrent execution.
2 participants
What does this PR do?
After a key has been soft-deleted, actions like:
still work. This PR eliminates that.
Fixes #2890
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Bug Fixes
Tests