feat: add permissions for ratelimit overrides #2126
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several new API routes and corresponding functionalities for managing rate limit overrides in the application. The new routes include the ability to delete, list, set, and get rate limit overrides, all requiring bearer authentication. Additionally, several permissions related to these operations are added to the workspace permissions structure. The changes also include updates to audit logging and various test cases to ensure the proper functioning and security of the new features. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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! 🙏 |
…-custom-override-permissions
….com/unkeyed/unkey into eng-1336-custom-override-permissions
![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
🧹 Outside diff range and nitpick comments (15)
apps/api/src/pkg/analytics.ts (1)
Line range hint
13-23: Consider extracting the schema for reusability and adding constraints.The implementation looks good and follows best practices with proper schema validation. However, there are some potential improvements:
- Consider extracting the schema into a separate constant for reuse across the codebase
- Add constraints for field validation (e.g., max length for strings, time range validation)
- Consider making the schema extensible with optional fields for future telemetry data
Example refactor:
+const sdkTelemetrySchema = z.object({ + request_id: z.string().max(100), + time: z.number().int().min(0), + runtime: z.string().max(50), + platform: z.string().max(50), + versions: z.array(z.string().max(20)).max(10), + // Optional fields for future use + metadata: z.record(z.string(), z.unknown()).optional(), +}); public get insertSdkTelemetry() { return this.clickhouse.client.insert({ table: "telemetry.raw_sdks_v1", - schema: z.object({ - request_id: z.string(), - time: z.number().int(), - runtime: z.string(), - platform: z.string(), - versions: z.array(z.string()), - }), + schema: sdkTelemetrySchema, }); }🧰 Tools
🪛 Biome
[error] 41-41: expected
}but instead the file endsthe file ends here
(parse)
apps/api/src/routes/v1_ratelimit_listOverrides.happy.test.ts (5)
8-8: Test description could be more specific.Consider renaming the test to better describe the scenario being tested, e.g., "should return all overrides for given namespace and identifier".
16-33: Consider extracting test data setup.The namespace and override creation logic could be moved to a helper function for better reusability across tests.
async function createTestNamespaceWithOverride(h: IntegrationHarness) { const namespaceId = newId("test"); const namespaceName = "test.Name"; const overrideId = newId("test"); const identifier = randomUUID(); await h.db.primary.insert(schema.ratelimitNamespaces).values({ id: namespaceId, name: namespaceName, workspaceId: h.resources.userWorkspace.id, createdAt: new Date(), }); await h.db.primary.insert(schema.ratelimitOverrides).values({ id: overrideId, workspaceId: h.resources.userWorkspace.id, namespaceId: namespaceId, identifier: identifier, limit: 1, duration: 60_000, async: false, }); return { namespaceId, namespaceName, overrideId, identifier }; }
31-31: Consider using a named constant for duration.The magic number
60_000would be clearer as a named constant, e.g.,const ONE_MINUTE_MS = 60_000.
42-42: Improve error message specificity.The error message could be more specific about what failed in the request.
-expect(res.status, `expected 200, received: ${JSON.stringify(res, null, 2)}`).toBe(200); +expect(res.status, `List overrides request failed with status ${res.status}. Response: ${JSON.stringify(res, null, 2)}`).toBe(200);
43-45: Enhance assertion coverage.The test only validates the ID and identifier of the override. Consider adding assertions for other important properties like
limit,duration, andasync.expect(res.body.total).toBe(1); expect(res.body.overrides[0].id).toEqual(overrideId); expect(res.body.overrides[0].identifier).toEqual(identifier); +expect(res.body.overrides[0].limit).toEqual(1); +expect(res.body.overrides[0].duration).toEqual(60_000); +expect(res.body.overrides[0].async).toEqual(false);apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts (3)
25-25: Remove debug console.log statementDebug logging statements should be removed from test files to maintain clean test output.
- console.log(namespaceRes);
27-35: Consider extracting test constantsThe hardcoded values for limit, duration, and async flag could be extracted into named constants at the top of the file. This would make the test more maintainable and the values' purposes more explicit.
+const TEST_RATE_LIMIT = 1; +const TEST_DURATION_MS = 60_000; +const TEST_ASYNC = false; await h.db.primary.insert(schema.ratelimitOverrides).values({ id: overrideId, workspaceId: h.resources.userWorkspace.id, namespaceId: namespaceId, identifier: identifier, - limit: 1, - duration: 60_000, - async: false, + limit: TEST_RATE_LIMIT, + duration: TEST_DURATION_MS, + async: TEST_ASYNC, });
43-43: Remove debug console.log statementDebug logging statements should be removed from test files to maintain clean test output.
- console.log(res);apps/api/src/routes/v1_ratelimit_setOverride.happy.test.ts (2)
33-37: Enhance namespace verification coverage.The test only verifies the namespace ID. Consider verifying other fields like
workspaceId,name, andcreatedAtfor more comprehensive testing.expect(namespaceRes?.id).toBe(namespaceId); + expect(namespaceRes?.workspaceId).toBe(h.resources.userWorkspace.id); + expect(namespaceRes?.name).toBe(namespace.name); + expect(namespaceRes?.createdAt).toEqual(namespace.createdAt);
39-45: Use named constants for duration values.The test uses magic numbers (6500, 50000) for duration values. Consider using named constants to make the test more maintainable and self-documenting.
+ const INITIAL_DURATION_MS = 6500; + const UPDATED_DURATION_MS = 50000; const override = { namespaceId: namespaceId, identifier: identifier, limit: 10, - duration: 6500, + duration: INITIAL_DURATION_MS, async: true, }; // ... later in the code ... body: { namespaceId: namespaceId, identifier: identifier, limit: 10, - duration: 50000, + duration: UPDATED_DURATION_MS, async: true, },Also applies to: 74-80
apps/api/src/routes/v1_ratelimit_getOverride.ts (4)
25-29: Add validation for wildcard patterns in identifier.The description mentions support for wildcards (*) but there's no validation to ensure proper wildcard pattern format.
Consider adding a regex pattern validation:
identifier: z.string().openapi({ + pattern: "^[a-zA-Z0-9_*]+$", description: "Identifier of your user, this can be their userId, an email, an ip or anything else. Wildcards ( * ) can be used to match multiple identifiers, More info can be found at https://www.unkey.com/docs/ratelimiting/overrides#wildcard-rules", example: "user_123", }),
42-42: Document the purpose of the async field.The response schema includes an optional
asyncfield but its purpose and usage aren't documented.Add a description in the OpenAPI schema:
- async: z.boolean().nullable().optional(), + async: z.boolean().nullable().optional().openapi({ + description: "When true, indicates that rate limit checks are performed asynchronously", + }),
91-93: Improve error message clarity.The error message construction could be clearer about which identifier was not found.
- throw new Error(`Namespace ${namespaceId ? namespaceId : namespaceName} not found`); + throw new UnkeyApiError({ + code: "NOT_FOUND", + message: `Namespace not found: ${namespaceId ? `id=${namespaceId}` : `name=${namespaceName}`}`, + });
109-115: Consider adding telemetry for rate limit override access.For better observability, consider tracking access to rate limit overrides.
Add telemetry tracking before returning the response:
+ await c.get("services").analytics.track({ + event: "ratelimit.override.accessed", + workspaceId: authorizedWorkspaceId, + properties: { + overrideId: override.id, + namespaceId: override.namespaceId, + }, + }); + return c.json({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
apps/api/src/pkg/analytics.ts(1 hunks)apps/api/src/routes/v1_ratelimit_deleteOverride.happy.test.ts(1 hunks)apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts(1 hunks)apps/api/src/routes/v1_ratelimit_getOverride.ts(1 hunks)apps/api/src/routes/v1_ratelimit_listOverrides.happy.test.ts(1 hunks)apps/api/src/routes/v1_ratelimit_listOverrides.ts(1 hunks)apps/api/src/routes/v1_ratelimit_setOverride.happy.test.ts(1 hunks)apps/api/src/worker.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/api/src/routes/v1_ratelimit_deleteOverride.happy.test.ts
- apps/api/src/routes/v1_ratelimit_listOverrides.ts
- apps/api/src/worker.ts
🧰 Additional context used
🪛 Biome
apps/api/src/pkg/analytics.ts
[error] 41-41: expected } but instead the file ends
the file ends here
(parse)
🔇 Additional comments (7)
apps/api/src/routes/v1_ratelimit_listOverrides.happy.test.ts (2)
1-7: LGTM! Well-structured imports.
The imports are well-organized, using native Node.js modules and proper type imports.
10-10: Review permission scope.
The permission "ratelimit.*.read_override" uses a wildcard which might be too permissive. Consider if a more specific scope would be more appropriate for the test.
✅ Verification successful
Permission scope is appropriate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of this permission to understand the intended scope
rg -A 2 "ratelimit\.\*\.read_override"
Length of output: 2265
Script:
#!/bin/bash
# Search for the definition or creation of the "ratelimit.*.read_override" permission
rg '"ratelimit\.\*\.read_override"'
Length of output: 757
apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts (1)
1-7: LGTM: Imports are well-organized and complete
All necessary dependencies and types are properly imported.
apps/api/src/routes/v1_ratelimit_setOverride.happy.test.ts (2)
1-11: LGTM! Well-structured imports and type declarations.
The imports are properly organized and include all necessary dependencies for testing.
57-57: Rename unclear variable.
The variable name resInit is unclear. Consider a more descriptive name like initialOverride or createdOverride.
apps/api/src/routes/v1_ratelimit_getOverride.ts (2)
1-7: LGTM! All necessary imports are present and well-organized.
The imports cover all required functionality including authentication, error handling, OpenAPI/Zod integration, and database operations.
51-55: LGTM! Type exports are well-defined.
The types are properly inferred from Zod schemas and follow TypeScript best practices.
….com/unkeyed/unkey into eng-1336-custom-override-permissions
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (18)
apps/api/src/routes/v1_ratelimit_getOverride.error.test.ts (2)
8-33: Consider improving test maintainability with constants and helpers.While the test setup is thorough, consider these improvements:
- Extract the permission string into a constant
- Make the rate limit values more explicit
- Consider moving the test data setup into a reusable helper
+const RATELIMIT_READ_PERMISSION = "ratelimit.*.read_override"; +const DEFAULT_RATE_LIMIT = 1; +const ONE_MINUTE_MS = 60_000; + test("Missing Namespace", async (t) => { const h = await IntegrationHarness.init(t); - const root = await h.createRootKey(["ratelimit.*.read_override"]); + const root = await h.createRootKey([RATELIMIT_READ_PERMISSION]); // ... other setup code ... await h.db.primary.insert(schema.ratelimitOverrides).values({ id: overrideId, workspaceId: h.resources.userWorkspace.id, namespaceId: namespaceId, identifier: identifier, - limit: 1, - duration: 60_000, + limit: DEFAULT_RATE_LIMIT, + duration: ONE_MINUTE_MS, async: false, });
34-48: Consider adding test coverage for namespaceName parameter.Since the error message mentions "You must provide a namespaceId or a namespaceName", consider adding test cases for:
- Empty namespaceName
- Both namespaceId and namespaceName empty
- Both parameters present but invalid
Would you like me to help generate these additional test cases?
apps/api/src/routes/v1_ratelimit_listOverrides.error.test.ts (2)
8-8: Consider a more descriptive test nameThe test name could be more specific about what's being tested, e.g., "should return 400 when both namespaceId and namespaceName are missing".
42-49: Consider using a constant for the docs URLThe hardcoded docs URL "https://unkey.dev/docs/api-reference/errors/code/BAD_REQUEST" could be problematic if the documentation structure changes. Consider extracting this to a shared constant.
+ // In a shared constants file + export const ERROR_DOCS_BASE_URL = "https://unkey.dev/docs/api-reference/errors/code"; + expect(res.body).toMatchObject({ error: { code: "BAD_REQUEST", - docs: "https://unkey.dev/docs/api-reference/errors/code/BAD_REQUEST", + docs: `${ERROR_DOCS_BASE_URL}/BAD_REQUEST`, message: "You must provide a namespaceId or a namespaceName", }, });apps/api/src/routes/v1_ratelimit_deleteOverride.error.test.ts (1)
13-36: Consider improving test data setup for better maintainabilityWhile the setup is functional, consider these improvements:
- Extract magic numbers into named constants (e.g.,
DEFAULT_RATE_LIMIT = 1,ONE_MINUTE_MS = 60_000)- Consider moving the test data setup into a helper function for reuse
- Add a comment explaining the significance of the
asyncflagExample refactor:
+const DEFAULT_RATE_LIMIT = 1; +const ONE_MINUTE_MS = 60_000; + +async function setupTestOverride(h: IntegrationHarness) { + const overrideId = newId("test"); + const identifier = randomUUID(); + const namespaceId = newId("test"); + + // Setup namespace + const namespace = { + id: namespaceId, + workspaceId: h.resources.userWorkspace.id, + createdAt: new Date(), + name: "namespace", + }; + await h.db.primary.insert(schema.ratelimitNamespaces).values(namespace); + + // Setup override with default rate limiting + await h.db.primary.insert(schema.ratelimitOverrides).values({ + id: overrideId, + workspaceId: h.resources.userWorkspace.id, + namespaceId, + identifier, + limit: DEFAULT_RATE_LIMIT, + duration: ONE_MINUTE_MS, + async: false, // Synchronous rate limiting for predictable test behavior + }); + + return { overrideId, identifier, namespaceId }; +}apps/api/src/routes/v1_ratelimit_setOverride.error.test.ts (2)
13-47: Improve test robustness and permission naming consistency.
- The permission string has inconsistent casing. It should be
ratelimit.*.set_overrideto match the conventional snake_case format.- Consider adding a database check to verify the namespace doesn't exist before making the request.
const root = await h.createRootKey([ "*", - "ratelimit.*.set_Override", + "ratelimit.*.set_override", "ratelimit.*.create_namespace", "ratelimit.*.read_override", ]); + +// Verify namespace doesn't exist +const namespace = await h.db.primary.query.ratelimitNamespaces.findFirst({ + where: (table, { eq }) => eq(table.id, namespaceId), +}); +expect(namespace).toBeNull();
1-97: Consider adding more error test cases.The current test file only covers two error scenarios. Consider adding tests for:
- Invalid duration (negative or zero)
- Invalid limit (negative or zero)
- Missing or insufficient permissions
- Invalid async flag value
Would you like me to help generate these additional test cases?
apps/api/src/routes/v1_ratelimit_getOverride.ts (1)
8-49: Consider adding additional validation rules to the schema.While the schema is well-documented, consider adding these validations for better input handling:
namespaceId: Add pattern validation for therlns_prefixlimitandduration: Add minimum value constraints (e.g., > 0)identifier: Add maximum length constraintrequest: { query: z.object({ namespaceId: z.string() + .regex(/^rlns_[a-zA-Z0-9]+$/, 'Must start with rlns_') .optional() .openapi({ description: "The id of the namespace.", example: "rlns_1234", }), // ... other fields ... identifier: z.string() + .max(255, 'Identifier too long') .openapi({ description: // ... }), }), }, responses: { // ... schema: z.object({ // ... - limit: z.number().int(), - duration: z.number().int(), + limit: z.number().int().positive(), + duration: z.number().int().positive(), // ... }), }, },apps/api/src/routes/v1_ratelimit_listOverrides.security.test.ts (2)
12-42: Consider extracting test data setup to reduce duplication.The test data setup logic is duplicated across different test suites. Consider extracting it into a helper function.
Example refactor:
async function setupTestData(rh: IntegrationHarness) { const overrideId = newId("test"); const identifier = randomUUID(); const namespaceId = newId("test"); const namespace = { id: namespaceId, workspaceId: rh.resources.userWorkspace.id, createdAt: new Date(), name: "namespace", }; await rh.db.primary.insert(schema.ratelimitNamespaces).values(namespace); await rh.db.primary.insert(schema.ratelimitOverrides).values({ id: overrideId, workspaceId: rh.resources.userWorkspace.id, namespaceId, identifier, limit: 1, duration: 60_000, async: false, }); return { namespaceId, overrideId, identifier }; }
29-31: Consider using constants for rate limit values.Magic numbers are used for rate limit values. Consider extracting these into named constants for better maintainability.
Example:
const TEST_RATE_LIMIT = { LIMIT: 1, DURATION_MS: 60_000, };apps/api/src/routes/v1_ratelimit_getOverride.security.test.ts (3)
12-42: Consider extracting test data setup to reduce duplication.The test data setup logic is duplicated across different test sections. Consider extracting it into a helper function to improve maintainability.
async function setupTestData(h: IntegrationHarness) { const overrideId = newId("test"); const identifier = randomUUID(); const namespaceId = newId("test"); const namespace = { id: namespaceId, workspaceId: h.resources.userWorkspace.id, createdAt: new Date(), name: "namespace", }; await h.db.primary.insert(schema.ratelimitNamespaces).values(namespace); await h.db.primary.insert(schema.ratelimitOverrides).values({ id: overrideId, workspaceId: h.resources.userWorkspace.id, namespaceId, identifier, limit: 1, duration: 60_000, async: false, }); return { namespaceId, identifier }; }
30-30: Consider using a named constant for the duration value.The magic number
60_000would be more maintainable as a named constant that clearly indicates it represents one minute in milliseconds.const ONE_MINUTE_MS = 60_000;
1-11: Consider adding tests for invalid input scenarios.The test file would benefit from additional test cases covering:
- Invalid namespace IDs
- Malformed identifiers
- Missing query parameters
- Invalid content types
Also, consider adding JSDoc comments to describe the test suite's purpose and requirements.
/** * Security tests for the rate limit override endpoint. * * These tests verify: * - Role-based access control * - Input validation * - Error handling * * Required permissions: * - ratelimit.*.read_override */apps/api/src/routes/v1_ratelimit_setOverride.security.test.ts (2)
12-46: Consider enhancing test coverage for edge cases.While the basic setup is good, consider adding tests for:
- Different duration values (especially edge cases)
- The implications of the
asyncflag- Validation that the namespace was created correctly before proceeding
const override = { namespaceId: namespaceId, identifier: identifier, - limit: 10, - duration: 6500, + limit: 10, + duration: 60_000, // Use a more standard duration value async: true, }; + +// Add validation +const createdNamespace = await rh.db.primary.query.ratelimitNamespaces.findFirst({ + where: (table, { eq }) => eq(table.id, namespaceId), +}); +if (!createdNamespace) { + throw new Error("Failed to create test namespace"); +}
47-111: Enhance test assertions and use constants for better maintainability.The test coverage for successful operations is good, but consider these improvements:
- Use constants for magic numbers (limit: 1, limit: 7, duration: 60_000)
- Add assertions for all override properties, not just the limit
- Add test cases for concurrent updates to verify race conditions
+const TEST_CONSTANTS = { + INITIAL_LIMIT: 1, + UPDATED_LIMIT: 7, + DURATION: 60_000, +} as const; test("returns 200", async (t) => { // ... existing setup ... body: { namespaceId, identifier, - limit: 1, - duration: 60_000, + limit: TEST_CONSTANTS.INITIAL_LIMIT, + duration: TEST_CONSTANTS.DURATION, async: false, }, }); // Add more comprehensive assertions expect(found?.limit).toEqual(7); +expect(found?.duration).toEqual(TEST_CONSTANTS.DURATION); +expect(found?.async).toEqual(false); +expect(found?.namespaceId).toEqual(namespaceId); +expect(found?.identifier).toEqual(identifier);apps/api/src/routes/v1_ratelimit_deleteOverride.security.test.ts (3)
12-46: Consider enhancing test data setup.While the setup is functional, consider these improvements:
- Extract magic numbers into named constants (e.g.,
DEFAULT_DURATION_MS = 60_000)- Consider parameterizing the test data values (limit, duration, async) for better test coverage
- Ensure proper cleanup of test data after tests
+const DEFAULT_DURATION_MS = 60_000; +const DEFAULT_LIMIT = 1; + runCommonRouteTests<V1RatelimitDeleteOverrideRequest>({ prepareRequest: async (rh) => { const overrideId = newId("test"); const identifier = randomUUID(); const namespaceId = newId("test"); const namespace = { id: namespaceId, workspaceId: rh.resources.userWorkspace.id, createdAt: new Date(), name: "namespace", }; await rh.db.primary.insert(schema.ratelimitNamespaces).values(namespace); await rh.db.primary.insert(schema.ratelimitOverrides).values({ id: overrideId, workspaceId: rh.resources.userWorkspace.id, namespaceId, identifier, - limit: 1, - duration: 60_000, + limit: DEFAULT_LIMIT, + duration: DEFAULT_DURATION_MS, async: false, });
47-101: Consider extracting duplicated setup code.The test setup code is duplicated from the common tests. Consider extracting the setup into a shared helper function.
async function setupTestData(h: IntegrationHarness) { const overrideId = newId("test"); const identifier = randomUUID(); const namespaceId = newId("test"); // ... rest of the setup return { overrideId, identifier, namespaceId }; }The role-based permission check and database verification are well implemented.
102-157: LGTM! Comprehensive negative test case.The test properly verifies:
- 403 status code for unauthorized access
- Data persistence (override remains in database)
- Correct error handling for incorrect roles
Consider adding more test cases with different incorrect roles to ensure robust permission checking.
describe("incorrect roles", () => { describe.each([ - { name: "delete override", roles: ["ratelimit.*.create_override"] } + { name: "create override role", roles: ["ratelimit.*.create_override"] }, + { name: "read override role", roles: ["ratelimit.*.read_override"] }, + { name: "no roles", roles: [] } ])(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
apps/api/src/routes/v1_ratelimit_deleteOverride.error.test.ts(1 hunks)apps/api/src/routes/v1_ratelimit_deleteOverride.security.test.ts(1 hunks)apps/api/src/routes/v1_ratelimit_deleteOverride.ts(1 hunks)apps/api/src/routes/v1_ratelimit_getOverride.error.test.ts(1 hunks)apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts(1 hunks)apps/api/src/routes/v1_ratelimit_getOverride.security.test.ts(1 hunks)apps/api/src/routes/v1_ratelimit_getOverride.ts(1 hunks)apps/api/src/routes/v1_ratelimit_listOverrides.error.test.ts(1 hunks)apps/api/src/routes/v1_ratelimit_listOverrides.security.test.ts(1 hunks)apps/api/src/routes/v1_ratelimit_listOverrides.ts(1 hunks)apps/api/src/routes/v1_ratelimit_setOverride.error.test.ts(1 hunks)apps/api/src/routes/v1_ratelimit_setOverride.security.test.ts(1 hunks)apps/api/src/routes/v1_ratelimit_setOverride.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/api/src/routes/v1_ratelimit_deleteOverride.ts
- apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts
- apps/api/src/routes/v1_ratelimit_listOverrides.ts
- apps/api/src/routes/v1_ratelimit_setOverride.ts
🔇 Additional comments (17)
apps/api/src/routes/v1_ratelimit_getOverride.error.test.ts (3)
1-7: LGTM! Well-organized imports with clear dependencies.
The imports are properly structured, including all necessary dependencies for testing, database operations, and type safety.
34-48: LGTM! Clear and thorough error case testing.
The test properly verifies the error response structure and includes helpful error messages in the assertions.
1-48: Verify consistency with other error test patterns.
Let's ensure this error test follows the same patterns as other error tests in the codebase.
✅ Verification successful
Error test follows consistent patterns across the codebase
The test follows the established error testing patterns in the codebase:
- Uses consistent status code assertion format:
expect(res.status, 'expected 400...').toBe(400) - Follows similar error response structure with
toMatchObject - Belongs to the same family of rate limit error tests (found in
deleteOverride.error.test.ts,listOverrides.error.test.ts, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar error test patterns in other test files
rg -t ts "Missing.*test.*async.*expect.*BAD_REQUEST" apps/api/src/routes/
Length of output: 75
Script:
#!/bin/bash
# Let's search for error test patterns more broadly
rg -t ts "test.*BAD_REQUEST.*error" apps/api/src/routes/ -B 1 -A 1
# Also search for similar test structure
rg -t ts "\.error\.test\.ts" apps/api/src/routes/ --files-with-matches
# Look for test files with expect status checks
rg -t ts "expect\(.*status.*\).*toBe\(400\)" apps/api/src/routes/ -B 1 -A 1
Length of output: 1747
apps/api/src/routes/v1_ratelimit_listOverrides.error.test.ts (3)
1-7: LGTM! Clean and well-organized imports
The imports follow good practices with a clear separation between built-in Node.js modules, external dependencies, and internal imports.
35-40: LGTM! Well-structured API request setup
The request is properly configured to test the error case, with good type safety through the response type assertion.
10-10: Verify permission scope alignment
The test uses the "ratelimit.*.read_override" permission. Let's verify this matches the API implementation.
✅ Verification successful
The permission "ratelimit.*.read_override" is consistently used across the codebase:
- In the route handler
v1_ratelimit_listOverrides.tswhich is being tested - In a related route handler
v1_ratelimit_getOverride.ts - In the dashboard permissions configuration
The test's permission scope perfectly aligns with the API implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for permission validation in the route handler
ast-grep --pattern 'verifyPermission($_, "ratelimit.*.read_override")'
# Search for permission documentation
rg -g '!*.test.ts' "ratelimit\.\*\.read_override"
Length of output: 742
apps/api/src/routes/v1_ratelimit_deleteOverride.error.test.ts (2)
1-11: LGTM: Imports and type declarations are well-organized
The imports are properly structured, with clear separation between node built-ins, internal packages, and external dependencies.
50-57: Verify the error documentation URL
The test includes a hardcoded documentation URL. Let's ensure this URL is valid and matches our documentation structure.
✅ Verification successful
Documentation URL pattern is correctly implemented and consistent
The error documentation URL in the test is valid and matches the centralized error handling implementation in apps/api/src/pkg/errors/http.ts. The URL is consistently constructed using template literals across the codebase:
- The base URL pattern
https://unkey.dev/docs/api-reference/errors/code/is used in the error schema factory - Error responses are generated through centralized functions that maintain this URL pattern
- The error code is appended to create the complete documentation URL
- This pattern is used consistently across 35+ files in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the error documentation URL pattern across the codebase
# Check if this URL pattern is consistently used
rg -l "unkey\.dev/docs/api-reference/errors/code/" --type ts
# Check if there's a central constant or configuration for the docs URL
rg "const\s+.*DOC.*URL|const\s+.*ERROR.*URL" --type ts
Length of output: 2073
Script:
#!/bin/bash
# Let's check the error handling implementation to see if there's a centralized error creation
ast-grep --pattern 'class $_ extends Error {
$$$
docs: string;
$$$
}'
# Also check the specific file where errors are defined
cat apps/api/src/pkg/errors/http.ts
Length of output: 6463
apps/api/src/routes/v1_ratelimit_setOverride.error.test.ts (1)
1-11: LGTM! Imports are well-organized and necessary.
All required dependencies and types are properly imported.
apps/api/src/routes/v1_ratelimit_getOverride.ts (2)
1-7: LGTM! Dependencies are well organized.
The imports are clean, relevant, and properly typed.
51-55: LGTM! Type exports are well-defined.
The types are properly derived from the Zod schema and exported correctly.
apps/api/src/routes/v1_ratelimit_listOverrides.security.test.ts (2)
1-11: LGTM! Imports and type declarations are well-organized.
All necessary dependencies are properly imported, and the types are correctly referenced from the implementation file.
1-123: Verify complete security coverage.
While the basic RBAC testing is in place, let's verify coverage of additional security aspects:
- SQL injection via namespaceId parameter
- Rate limiting on the endpoint itself
- Workspace isolation (ensuring users can't access other workspaces' data)
apps/api/src/routes/v1_ratelimit_getOverride.security.test.ts (1)
43-85: Enhance test coverage for role combinations.
The test only verifies a single role combination. Consider adding more test cases with different role combinations to ensure comprehensive coverage.
describe.each([
{ name: "get override with specific permission", roles: ["ratelimit.*.read_override"] },
{ name: "get override with wildcard permission", roles: ["ratelimit.*"] },
{ name: "get override with admin role", roles: ["admin"] }
])Additionally, consider verifying the response body structure to ensure the API returns the expected data format.
apps/api/src/routes/v1_ratelimit_setOverride.security.test.ts (2)
1-11: LGTM! Imports are well-organized and appropriate for security testing.
The imports include all necessary testing utilities, database schema, and type definitions.
1-155: Add boundary tests for limit and duration values.
Consider adding test cases to verify the behavior with boundary values:
- Maximum allowed limit
- Maximum allowed duration
- Zero or negative values
- Very large numbers
✅ Verification successful
The implementation shows that there are input validations for both limit and duration:
limit: Must be a positive integer (z.number().int().positive())duration: Must be an integer with minimum value of 1000ms (z.number().int().min(1000))
Since the validation rules are clearly defined in the implementation, I can generate the final response:
Add test cases for limit and duration validation boundaries
Add test cases to verify the input validation rules:
limit: Test with 0 and negative values (should fail)duration: Test with values < 1000ms (should fail)
Example test cases to add:
test("fails with invalid limit", async (t) => {
// Test with limit: 0
// Test with limit: -1
});
test("fails with invalid duration", async (t) => {
// Test with duration: 999
// Test with duration: 0
// Test with duration: -1
});🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any validation of limit and duration values in the implementation
rg -A 5 "limit|duration" apps/api/src/routes/v1_ratelimit_setOverride.ts
Length of output: 4620
apps/api/src/routes/v1_ratelimit_deleteOverride.security.test.ts (1)
1-11: LGTM! Imports are well-organized and complete.
All necessary testing utilities and types are properly imported.
apps/api/src/routes/v1_ratelimit_listOverrides.security.test.ts
Outdated
Show resolved
Hide resolved
apps/api/src/routes/v1_ratelimit_listOverrides.security.test.ts
Outdated
Show resolved
Hide resolved
|
|
||
| expect(res.status, `expected 200, received: ${JSON.stringify(res, null, 2)}`).toBe(200); | ||
|
|
||
| const resInit = await h.db.primary.query.ratelimitOverrides.findFirst({ |
| where: (table, { eq, and }) => | ||
| and(eq(table.namespaceId, namespaceId), eq(table.identifier, identifier)), | ||
| }); | ||
| expect(resNew?.identifier).toEqual(identifier); |
There was a problem hiding this comment.
do a check that it's not undefined, and then assert it with !
apps/api/src/routes/v1_ratelimit_deleteOverride.security.test.ts
Outdated
Show resolved
Hide resolved
apps/api/src/routes/v1_ratelimit_listOverrides.security.test.ts
Outdated
Show resolved
Hide resolved
…-custom-override-permissions
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts (2)
31-31: Consider extracting magic numbers into named constantsThe duration value
60_000would be more maintainable and readable if extracted into a named constant.+const ONE_MINUTE_MS = 60_000; + test("return a single override using namespaceId", async (t) => { // ... - duration: 60_000, + duration: ONE_MINUTE_MS,
8-48: Consider reducing code duplicationBoth test cases share similar setup code. Consider extracting the common setup logic into a helper function to improve maintainability and reduce duplication.
async function setupTestOverride(h: IntegrationHarness) { const namespaceId = newId("test"); const namespaceName = randomUUID(); const overrideId = newId("test"); const identifier = randomUUID(); const namespace = { id: namespaceId, name: namespaceName, workspaceId: h.resources.userWorkspace.id, createdAt: new Date(), }; await h.db.primary.insert(schema.ratelimitNamespaces).values(namespace); await h.db.primary.insert(schema.ratelimitOverrides).values({ id: overrideId, workspaceId: h.resources.userWorkspace.id, namespaceId: namespaceId, identifier: identifier, limit: 1, duration: ONE_MINUTE_MS, async: false, }); return { namespaceId, namespaceName, overrideId, identifier }; }Also applies to: 50-89
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
apps/api/src/routes/v1_ratelimit_deleteOverride.error.test.ts(1 hunks)apps/api/src/routes/v1_ratelimit_deleteOverride.happy.test.ts(1 hunks)apps/api/src/routes/v1_ratelimit_deleteOverride.security.test.ts(1 hunks)apps/api/src/routes/v1_ratelimit_deleteOverride.ts(1 hunks)apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts(1 hunks)apps/api/src/routes/v1_ratelimit_getOverride.security.test.ts(1 hunks)apps/api/src/routes/v1_ratelimit_getOverride.ts(1 hunks)apps/api/src/routes/v1_ratelimit_listOverrides.happy.test.ts(1 hunks)apps/api/src/routes/v1_ratelimit_listOverrides.security.test.ts(1 hunks)apps/api/src/routes/v1_ratelimit_listOverrides.ts(1 hunks)apps/api/src/routes/v1_ratelimit_setOverride.error.test.ts(1 hunks)apps/api/src/routes/v1_ratelimit_setOverride.happy.test.ts(1 hunks)apps/api/src/routes/v1_ratelimit_setOverride.security.test.ts(1 hunks)apps/api/src/routes/v1_ratelimit_setOverride.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- apps/api/src/routes/v1_ratelimit_deleteOverride.error.test.ts
- apps/api/src/routes/v1_ratelimit_deleteOverride.happy.test.ts
- apps/api/src/routes/v1_ratelimit_deleteOverride.security.test.ts
- apps/api/src/routes/v1_ratelimit_deleteOverride.ts
- apps/api/src/routes/v1_ratelimit_getOverride.security.test.ts
- apps/api/src/routes/v1_ratelimit_getOverride.ts
- apps/api/src/routes/v1_ratelimit_listOverrides.happy.test.ts
- apps/api/src/routes/v1_ratelimit_listOverrides.security.test.ts
- apps/api/src/routes/v1_ratelimit_listOverrides.ts
- apps/api/src/routes/v1_ratelimit_setOverride.error.test.ts
- apps/api/src/routes/v1_ratelimit_setOverride.happy.test.ts
- apps/api/src/routes/v1_ratelimit_setOverride.security.test.ts
- apps/api/src/routes/v1_ratelimit_setOverride.ts
🧰 Additional context used
📓 Learnings (1)
apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts (1)
Learnt from: chronark
PR: unkeyed/unkey#2126
File: apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts:36-36
Timestamp: 2024-11-13T19:06:36.786Z
Learning: In the rate limit test files (e.g., `apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts`), URL parameters like `namespaceId` and `identifier` do not need to be URL-encoded in the test code because the values used are always considered safe within the test environment.
| test("return a single override using namespaceId", async (t) => { | ||
| const h = await IntegrationHarness.init(t); | ||
| const root = await h.createRootKey(["ratelimit.*.read_override"]); | ||
| const namespaceId = newId("test"); | ||
| const namespaceName = randomUUID(); | ||
| const overrideId = newId("test"); | ||
| const identifier = randomUUID(); | ||
|
|
||
| // Namespace | ||
| const namespace = { | ||
| id: namespaceId, | ||
| name: namespaceName, | ||
| workspaceId: h.resources.userWorkspace.id, | ||
| createdAt: new Date(), | ||
| }; | ||
| await h.db.primary.insert(schema.ratelimitNamespaces).values(namespace); | ||
| // Initial Override | ||
| await h.db.primary.insert(schema.ratelimitOverrides).values({ | ||
| id: overrideId, | ||
| workspaceId: h.resources.userWorkspace.id, | ||
| namespaceId: namespaceId, | ||
| identifier: identifier, | ||
| limit: 1, | ||
| duration: 60_000, | ||
| async: false, | ||
| }); | ||
|
|
||
| const res = await h.get<V1RatelimitGetOverrideResponse>({ | ||
| url: `/v1/ratelimit.getOverride?namespaceId=${namespaceId}&identifier=${identifier}`, | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| Authorization: `Bearer ${root.key}`, | ||
| }, | ||
| }); | ||
| expect(res.status, `expected 200, received: ${JSON.stringify(res, null, 2)}`).toBe(200); | ||
| expect(res.body.id).toBe(overrideId); | ||
| expect(res.body.identifier).toEqual(identifier); | ||
| expect(res.body.limit).toEqual(1); | ||
| expect(res.body.duration).toEqual(60_000); | ||
| expect(res.body.async).toEqual(false); | ||
| }); | ||
|
|
||
| test("return a single override using namespaceName", async (t) => { | ||
| const h = await IntegrationHarness.init(t); | ||
| const root = await h.createRootKey(["ratelimit.*.read_override"]); | ||
| const namespaceId = newId("test"); | ||
| const namespaceName = randomUUID(); | ||
| const overrideId = newId("test"); | ||
| const identifier = randomUUID(); | ||
|
|
||
| // Namespace | ||
| const namespace = { | ||
| id: namespaceId, | ||
| name: namespaceName, | ||
| workspaceId: h.resources.userWorkspace.id, | ||
| createdAt: new Date(), | ||
| }; | ||
| await h.db.primary.insert(schema.ratelimitNamespaces).values(namespace); | ||
| await h.db.primary.insert(schema.ratelimitOverrides).values({ | ||
| id: overrideId, | ||
| workspaceId: h.resources.userWorkspace.id, | ||
| namespaceId: namespaceId, | ||
| identifier: identifier, | ||
| limit: 1, | ||
| duration: 60_000, | ||
| async: false, | ||
| }); | ||
|
|
||
| const res = await h.get<V1RatelimitGetOverrideResponse>({ | ||
| url: `/v1/ratelimit.getOverride?namespaceName=${namespaceName}&identifier=${identifier}`, | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| Authorization: `Bearer ${root.key}`, | ||
| }, | ||
| }); | ||
| expect(res.status, `expected 200, received: ${JSON.stringify(res, null, 2)}`).toBe(200); | ||
| expect(res.body.id).toBe(overrideId); | ||
| expect(res.body.identifier).toEqual(identifier); | ||
| expect(res.body.limit).toEqual(1); | ||
| expect(res.body.duration).toEqual(60_000); | ||
| expect(res.body.async).toEqual(false); | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Review comment needs to be updated but is partially correct
The review comment is partially correct as it suggests adding error case tests, but there's already a dedicated error test file (v1_ratelimit_getOverride.error.test.ts) that covers the "Missing Namespace" scenario. However, other suggested error cases are still missing:
- Invalid permissions
- Non-existent overrides
- Missing identifier
- Invalid namespace ID/name format
🔗 Analysis chain
Add error case tests
The current tests only cover successful scenarios. Consider adding tests for error cases such as:
- Invalid namespace ID/name
- Missing identifier
- Invalid permissions
- Non-existent overrides
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for error case test coverage
# Look for error test files
fd "v1_ratelimit_getOverride.*error.*test.ts$" apps/api/src/routes/
# Search for error case tests in other test files
rg "test\(.*(error|fail|invalid|exception)" "apps/api/src/routes/v1_ratelimit_getOverride.*test.ts"
Length of output: 324
Script:
#!/bin/bash
# Let's check the content of the error test file to see what cases are already covered
cat apps/api/src/routes/v1_ratelimit_getOverride.error.test.ts
Length of output: 1683
…-custom-override-permissions
What does this PR do?
Fixes # (issue)
eng-1336
Type of change
How should this be tested?
Check if I understood the assignment properly.
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Release Notes
New Features
/v1/ratelimit.deleteOverride/v1/ratelimit.listOverrides/v1/ratelimit.setOverride/v1/ratelimit.getOverrideBug Fixes
Tests