feat: Who Am I route #2159
feat: Who Am I route #2159
Conversation
🦋 Changeset detectedLatest commit: 02e91fc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@harshsbhat is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce two new API endpoints: Changes
Assessment against linked issues
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 (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
github-actions
bot
added
🕹️ 300 points
Feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
apps/api/src/worker.ts (1)
12-12: Overall impact: Positive addition with a minor suggestion.The introduction of the "Who Am I" route is well-integrated into the existing codebase structure. It enhances the API's capabilities without disrupting the current functionality, aligning perfectly with the PR objectives.
To improve code readability and maintainability, consider grouping related route registrations. You could add a comment above the key-related routes to clearly demarcate this section.
Consider adding a comment above the key-related routes:
// misc registerV1Liveness(app); +// Key-related routes // keys registerV1KeysGetKey(app); registerV1KeysWhoAmi(app); registerV1KeysDeleteKey(app); // ... (other key-related routes)Also applies to: 71-71
apps/api/src/routes/v1_keys_getWhoAmI.ts (2)
88-88: Correct function name for consistency and readabilityThe function is named
registerV1KeysWhoAmi, but the correct spelling should beregisterV1KeysWhoAmIto match the operation ID and route path. This improves consistency and readability.Apply this diff to fix the function name:
(Line 88):
-export const registerV1KeysWhoAmi = (app: App) => +export const registerV1KeysWhoAmI = (app: App) =>
152-155: Reuse parsedmetadata to avoid redundant parsingThe
key.metais being parsed twice. First in lines 152-155 and again in line 168. Consider reusing themetavariable to avoid redundant parsing operations.Apply this diff:
(Line 168):
- meta: key.meta ? JSON.parse(key.meta) : undefined, + meta: meta,Also applies to: 168-168
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/api/src/routes/v1_keys_getWhoAmI.ts (1 hunks)
- apps/api/src/worker.ts (2 hunks)
🔇 Additional comments (2)
apps/api/src/worker.ts (2)
12-12: LGTM: Import statement for new route handler.The import statement for
registerV1KeysWhoAmiis consistent with the existing pattern and naming convention used for other route handlers. This aligns well with the PR objective of introducing a new "Who Am I" route for keys.
71-71: LGTM: Route registration for "Who Am I" functionality.The
registerV1KeysWhoAmi(app)call is correctly placed within the keys section of route registrations, maintaining consistency with the existing codebase structure. This addition aligns with the PR objective of creating a new "Who Am I" route for keys.To ensure the implementation meets the requirements outlined in the PR objectives, let's verify the route handler:
|
Please add a tests |
|
I will get working on this again tomorrow. Had to do some college work today. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
I have resolved the requested changes. I will add the tests in sometime. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
apps/api/src/routes/v1_keys_whoAmI.ts (1)
145-150: Consider returning 'FORBIDDEN' instead of 'NOT_FOUND' for unauthorized accessWhen
key.workspaceIddoes not matchauth.authorizedWorkspaceId, an error with code'NOT_FOUND'is thrown. While this hides resource existence, returning'FORBIDDEN'may be more appropriate to indicate insufficient permissions.Evaluate the security policy and adjust the error code if necessary to accurately reflect the authorization failure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/api/src/routes/v1_keys_whoAmI.ts (1 hunks)
- apps/api/src/worker.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/worker.ts
🧰 Additional context used
📓 Learnings (1)
apps/api/src/routes/v1_keys_whoAmI.ts (1)
Learnt from: chronark PR: unkeyed/unkey#2159 File: apps/api/src/routes/v1_keys_getWhoAmI.ts:12-13 Timestamp: 2024-10-03T09:51:43.832Z Learning: When implementing endpoints that require a key, avoid sending the key as part of the URL. Use POST methods with the key in the request body instead, as URLs get logged and could leak sensitive information.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
apps/api/src/routes/v1_keys_whoAmI.error.test.ts (1)
8-34: LGTM: Well-structured test case with comprehensive assertions.The test case effectively covers the scenario where the queried key does not exist. It follows a clear setup-action-assertion pattern and uses the
IntegrationHarnessfor consistent testing. The assertions check both the status code and the detailed structure of the error response, which is a good practice.Consider adding a comment explaining the purpose of
apiId(line 10) as it's created but not directly used in the test. This would improve code clarity for future maintainers.apps/api/src/routes/v1_keys_whoAmI.security.test.ts (2)
84-84: Enhance type safety for roles mappingWhen roles are functions that depend on
apiId, consider adding explicit type annotations or using more descriptive variable names to improve code readability and maintainability.
98-98: Simplify assertion error message for clarityIncluding the entire response object in the assertion error message can make the test output verbose and harder to read. Simplify the assertion to make test failures easier to diagnose.
Update the assertion as follows:
-expect(res.status, `expected 200, received: ${JSON.stringify(res, null, 2)}`).toEqual(200); +expect(res.status).toEqual(200);If additional information is needed upon failure, consider logging the response body separately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- apps/api/src/routes/v1_keys_whoAmI.error.test.ts (1 hunks)
- apps/api/src/routes/v1_keys_whoAmI.security.test.ts (1 hunks)
- apps/api/src/routes/v1_keys_whoAmI.ts (1 hunks)
- apps/api/src/worker.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/worker.ts
🧰 Additional context used
📓 Learnings (1)
apps/api/src/routes/v1_keys_whoAmI.ts (1)
Learnt from: chronark PR: unkeyed/unkey#2159 File: apps/api/src/routes/v1_keys_getWhoAmI.ts:12-13 Timestamp: 2024-10-03T09:51:43.832Z Learning: When implementing endpoints that require a key, avoid sending the key as part of the URL. Use POST methods with the key in the request body instead, as URLs get logged and could leak sensitive information.
🔇 Additional comments (7)
apps/api/src/routes/v1_keys_whoAmI.error.test.ts (1)
1-6: LGTM: Imports are well-organized and relevant.The imports are appropriately structured, including necessary dependencies and types for the test case. The use of named imports enhances code readability.
apps/api/src/routes/v1_keys_whoAmI.ts (6)
1-89: Well-structured route definition with proper security considerations.The route definition is comprehensive and follows best practices:
- Uses POST method, aligning with the recommendation to avoid sending keys in URLs.
- Implements bearer authentication for security.
- Utilizes Zod for request and response schema validation, enhancing type safety and documentation.
91-97: Clear and consistent type exports.The exported types leverage the Zod schemas, ensuring consistency between runtime validation and TypeScript types. This approach enhances type safety and improves developer experience when working with this API route.
99-103: Solid initial setup for the route handler.The route handler function is well-structured:
- It correctly extracts the validated key from the request body.
- Computes the hash of the key, which is a good security practice for key handling.
104-153: Efficient key retrieval and robust authorization logic.The implementation demonstrates several good practices:
- Utilizes a cache with SWR strategy, reducing database load.
- Performs a well-structured database query when necessary.
- Implements proper authorization checks to ensure secure access to key information.
159-174: Well-structured response matching the defined schema.The response construction is thorough and aligns with the Zod schema:
- Includes all required fields from the key data.
- Properly handles optional fields with null checks and fallbacks.
- Utilizes the parsed metadata, maintaining consistency with the earlier parsing.
1-174: Excellent implementation of the "Who Am I" route, meeting PR objectives.This implementation successfully addresses the requirements outlined in issue #2140:
- Provides a secure POST endpoint for retrieving key information without requiring a
keyId.- Implements proper authentication and authorization checks.
- Returns a well-structured response with all required key details.
The code demonstrates several strengths:
- Efficient caching strategy to reduce database load.
- Strong type safety through consistent use of Zod schemas.
- Adherence to security best practices, such as using POST for sensitive data and hashing keys.
Minor optimization opportunities have been identified and suggested in previous comments. Overall, this is a high-quality implementation that meets the project's standards and objectives.
|
@chronark should be good for review |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
apps/api/src/routes/v1_keys_whoAmI.happy.test.ts (3)
11-53: LGTM! Comprehensive test case with minor improvement needed.The test case "returns 200" is well-structured and covers the essential aspects of the
/v1/keys.whoAmIendpoint. It properly sets up the test environment, creates necessary data, makes the API call, and verifies the response.There's a duplicate assertion for the 'name' field. Consider removing one of these lines:
expect(res.body.name).toEqual(keySchema.name); expect(res.body.remaining).toEqual(keySchema.remaining); - expect(res.body.name).toEqual(keySchema.name); expect(res.body.meta).toEqual(JSON.parse(keySchema.meta));
55-86: LGTM! Well-structured test for identity retrieval.The test case "returns identity" effectively verifies the
/v1/keys.whoAmIendpoint's ability to return identity information associated with a key. The setup, API call, and assertions are all appropriate for this specific test scenario.Consider adding assertions for other key fields (similar to the first test) if it's important to verify that all key information is correctly returned along with the identity. This would ensure that the presence of identity doesn't affect other key data. For example:
expect(res.body.id).toBeDefined(); expect(res.body.name).toBeDefined(); expect(res.body.remaining).toBeDefined(); // ... other key field assertionsHowever, if the focus of this test is specifically on identity retrieval, the current assertions are sufficient.
1-86: Great job on the happy path tests! Consider adding error scenarios.The test file provides comprehensive coverage for the happy path scenarios of the
/v1/keys.whoAmIendpoint. Both tests are well-structured, using theIntegrationHarnesseffectively for setup and API calls. They cover the main functionality of retrieving key information and associated identity data.To further improve the test suite, consider adding the following test cases:
Error scenario: Invalid key
- Test the API response when an invalid or non-existent key is provided.
- Verify that the appropriate error status code and message are returned.
Error scenario: Unauthorized access
- Test the API response when the root key lacks the necessary permissions.
- Verify that the appropriate error status code and message are returned.
Edge case: Key with all optional fields
- Create a key with all possible optional fields populated.
- Verify that all fields are correctly returned in the response.
These additional tests would help ensure the robustness of the endpoint under various conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/api/src/routes/v1_keys_whoAmI.happy.test.ts (1 hunks)
- apps/api/src/routes/v1_keys_whoAmI.security.test.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/routes/v1_keys_whoAmI.security.test.ts
🧰 Additional context used
📓 Learnings (1)
📓 Common learnings
Learnt from: chronark PR: unkeyed/unkey#2159 File: apps/api/src/routes/v1_keys_getWhoAmI.ts:12-13 Timestamp: 2024-10-03T09:51:43.832Z Learning: When implementing endpoints that require a key, avoid sending the key as part of the URL. Use POST methods with the key in the request body instead, as URLs get logged and could leak sensitive information.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
.changeset/late-colts-shout.md (1)
1-5: Approved with suggestions for improvement.The changeset correctly identifies this as a minor API change, which is appropriate for adding a new route. However, the description could be more informative.
Consider expanding the description to include:
- The purpose of the new route (e.g., "to retrieve current key configuration and limits").
- Any notable features or requirements (e.g., "uses bearer authentication").
- Reference to related issue or PR numbers for traceability.
Here's a suggested revision:
--- "api": minor --- Add /v1/keys.whoami route to retrieve current key configuration and limits - Allows users to get key information without knowing the keyId - Requires bearer authentication with a rootKey - Implements POST method for secure key handling - Related to issue #2140 and PR #2159This provides more context and helps with future reference and changelog generation.
apps/docs/api-reference/keys/whoami.mdx (1)
13-14: Expand the endpoint description with more details.While the current description provides a good rationale for the endpoint, it lacks crucial information about its usage and behavior.
Consider expanding the description to include:
- Specify that this is a POST request.
- Mention the authentication method (bearer token with rootKey).
- Describe the expected request body structure.
- Outline the response structure and possible status codes.
For example:
This POST endpoint offers an alternative to [`/v1/keys.getKey`](/api-reference/keys/get) when you don't have access to the `keyId`. Authentication is required using a bearer token with a valid rootKey. The request body should be a JSON object containing the `key` field: ```json { "key": "your_actual_key_here" }The response will include detailed information about the key, such as its ID, name, remaining usage, and more. Refer to the OpenAPI specification below for the full response structure.
Possible status codes:
- 200: Successful operation
- 401: Unauthorized
- 404: Key not found
This additional information will help users understand how to use the endpoint correctly. </blockquote></details> <details> <summary>apps/docs/mint.json (1)</summary><blockquote> `237-238`: **LGTM! Consider adding a comma for consistency.** The addition of the "whoami" page to the "Keys" group is appropriate and aligns with the PR's objective of introducing a new "Who Am I" route. This change enhances the documentation structure by including the new endpoint. For consistency with the JSON format used throughout the file, consider adding a trailing comma after the new line. Here's the suggested change: ```diff "api-reference/keys/set-roles", -"api-reference/keys/whoami" +"api-reference/keys/whoami",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- .changeset/late-colts-shout.md (1 hunks)
- apps/api/src/routes/v1_keys_whoAmI.error.test.ts (1 hunks)
- apps/api/src/routes/v1_keys_whoAmI.happy.test.ts (1 hunks)
- apps/api/src/routes/v1_keys_whoAmI.security.test.ts (1 hunks)
- apps/api/src/routes/v1_keys_whoAmI.ts (1 hunks)
- apps/api/src/worker.ts (2 hunks)
- apps/docs/api-reference/keys/whoami.mdx (1 hunks)
- apps/docs/mint.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/api/src/routes/v1_keys_whoAmI.error.test.ts
- apps/api/src/routes/v1_keys_whoAmI.happy.test.ts
- apps/api/src/routes/v1_keys_whoAmI.security.test.ts
- apps/api/src/worker.ts
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: chronark PR: unkeyed/unkey#2159 File: apps/api/src/routes/v1_keys_getWhoAmI.ts:12-13 Timestamp: 2024-10-03T09:51:43.832Z Learning: When implementing endpoints that require a key, avoid sending the key as part of the URL. Use POST methods with the key in the request body instead, as URLs get logged and could leak sensitive information.apps/api/src/routes/v1_keys_whoAmI.ts (1)
Learnt from: chronark PR: unkeyed/unkey#2159 File: apps/api/src/routes/v1_keys_getWhoAmI.ts:12-13 Timestamp: 2024-10-03T09:51:43.832Z Learning: When implementing endpoints that require a key, avoid sending the key as part of the URL. Use POST methods with the key in the request body instead, as URLs get logged and could leak sensitive information.
🔇 Additional comments (6)
apps/api/src/routes/v1_keys_whoAmI.ts (6)
1-89: Well-structured OpenAPI route definition with comprehensive schemas.The route definition follows OpenAPI standards and includes proper documentation. The request and response schemas are well-defined using Zod, ensuring proper validation and type safety. The comprehensive response schema covers all necessary fields for the key information.
91-97: Good practice: Exporting inferred types for route, request, and response.Exporting these types enhances type safety and promotes reusability across the codebase. The types are correctly inferred from the Zod schemas, ensuring consistency between the OpenAPI definition and the TypeScript types.
99-103: LGTM: Proper route registration and key handling.The
registerV1KeysWhoAmIfunction correctly registers the route with the app. The extraction of the key from the validated request body and the use of SHA-256 for hashing demonstrate good security practices.
104-147: Efficient key retrieval with proper authorization checks.The implementation demonstrates good practices:
- Using cache with a database fallback optimizes performance.
- Proper authorization checks ensure secure access to key information.
- Comprehensive error handling covers various scenarios.
These aspects contribute to a robust and secure implementation of the "Who Am I" functionality.
159-173: Well-structured response construction aligned with the schema.The response object is correctly constructed, aligning with the defined response schema. The use of optional chaining and nullish coalescing operators for handling potentially undefined values is a good practice, ensuring a consistent response structure.
1-174: Summary: Successful implementation of the "Who Am I" route for keysThis implementation successfully addresses the requirements outlined in issue #2140 and aligns well with the PR objectives. Key points:
- The route allows retrieval of key information without requiring a
keyId.- The use of a POST method with the key in the request body enhances security by avoiding key exposure in URLs.
- The response structure matches the specified requirements, providing comprehensive key information.
- The implementation follows good practices for OpenAPI route definition, type safety, and error handling.
Overall, this is a solid implementation that enhances the API functionality as intended.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Awarding harshsbhat: 300 points 🕹️ Well done! Check out your new contribution on oss.gg/harshsbhat |
|
Enjoyed every minute of writing this new route. Thanks @chronark for giving an opportunity for developers like me ( with no experience ) to actually see my code in the production. I would love to update the docs for this too |
What does this PR do?
Creates a new API route called Who AM I for keys.
Fixes #2140
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
/v1/keys.whoAmIfor retrieving key information using a POST request./v1/keys.whoAmIendpoint, detailing its usage and purpose.Bug Fixes
Tests
/v1/keys.whoAmIendpoint for non-existent keys and permissions based on user roles.