fix: Missing plan check and ip whitelist parsing#2379
fix: Missing plan check and ip whitelist parsing#2379chronark merged 20 commits intounkeyed:mainfrom
Conversation
Flo4604
requested review from
MichaelUnkey,
chronark,
mcstepp and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
|
@Flo4604 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! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
apps/api/src/routes/v1_apis_getApi.happy.test.ts (1)
34-34: Approved: IP whitelist format change aligns with PR objectives.The change from
JSON.stringify(["127.0.0.1"])to["127.0.0.1"].join(",")addresses the IP whitelist parsing issue mentioned in the PR objectives and linked issue #2372. This should prevent the 500 error caused by attempting to JSON parse a CSV-formatted list.Consider adding the following test cases to improve coverage:
- A test with multiple IP addresses in the whitelist to ensure correct handling of multiple entries.
- A test that explicitly verifies the format of the stored IP whitelist in the database.
Example for multiple IP addresses:
const multipleIps = ["127.0.0.1", "192.168.0.1", "10.0.0.1"]; const api = { // ... other properties ... ipWhitelist: multipleIps.join(","), }; // ... rest of the test ... // Add an assertion to check if the stored ipWhitelist matches the expected format expect(storedApi.ipWhitelist).toBe(multipleIps.join(","));apps/api/src/routes/legacy_keys_verifyKey.test.ts (1)
180-180: LGTM: IP whitelist format consistently updated.The change to use a comma-separated string for the
ipWhitelistproperty is consistent with the previous change and aligns with the PR objectives.For improved code consistency and readability, consider extracting the IP whitelist into a constant at the top of the file:
const TEST_IP_WHITELIST = ["100.100.100.100"].join(",");Then use this constant in both test cases:
ipWhitelist: TEST_IP_WHITELIST,This would make it easier to maintain and update the test IP whitelist across all test cases.
apps/api/src/routes/v1_keys_verifyKey.test.ts (1)
Line range hint
352-385: LGTM! Consider uncommenting and fixing the remaining tokens expectation.The new test suite for rate limit override looks good. It correctly tests the deduction of tokens when a specific cost is applied to a request. However, there's a commented-out expectation for the remaining tokens:
// expect(res.body.ratelimit?.remaining).toEqual(6);Consider uncommenting this line and ensuring it's correct. If the initial limit is 10 and the cost is 4, the remaining should indeed be 6.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- apps/api/src/pkg/keys/service.ts (5 hunks)
- apps/api/src/routes/legacy_keys_verifyKey.test.ts (2 hunks)
- apps/api/src/routes/v1_apis_getApi.happy.test.ts (1 hunks)
- apps/api/src/routes/v1_keys_verifyKey.test.ts (3 hunks)
- apps/dashboard/lib/trpc/routers/api/updateIpWhitelist.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (10)
apps/api/src/routes/legacy_keys_verifyKey.test.ts (2)
Line range hint
1-214: Summary: IP whitelist parsing issue addressed successfully.The changes in this file successfully address the IP whitelist parsing issue mentioned in the PR objectives and linked issue #2372. The
ipWhitelistproperty is now consistently set as a comma-separated string, which should resolve the 500 error caused by incorrect parsing.The test cases cover various scenarios, including:
- Valid key verification
- Bad requests
- Temporary key handling
- IP whitelist validation (both valid and invalid IPs)
These comprehensive tests ensure that the key verification process behaves correctly under different conditions, particularly with the updated IP whitelist format.
To further improve the test suite, consider adding the following test cases:
- Multiple IP addresses in the whitelist
- IPv6 addresses in the whitelist
- Edge cases such as empty whitelist or whitelist with invalid IP formats
Would you like me to provide examples of these additional test cases?
132-132: LGTM: IP whitelist format updated correctly.The change from a JSON string to a comma-separated string for the
ipWhitelistproperty aligns with the PR objectives and addresses the issue described in #2372. This should resolve the 500 error caused by incorrect parsing.To ensure consistency across the codebase, let's verify that this change is applied uniformly:
apps/api/src/pkg/keys/service.ts (5)
60-64: LGTM: Improved readability ofInvalidResponsetypeThe reformatting of the
identityfield in theInvalidResponsetype enhances code readability without changing the underlying logic. This change aligns with TypeScript best practices for type definitions.
80-84: LGTM: Consistent improvement inValidResponsetypeThe reformatting of the
identityfield in theValidResponsetype mirrors the changes made to theInvalidResponsetype. This consistency improves overall code readability and maintains a uniform structure across related type definitions.
296-298: LGTM: Enhanced readability ofratelimitsobject declarationThe reformatting of the
ratelimitsobject declaration improves code readability while maintaining the original functionality. This change is consistent with the earlier modifications to type definitions, contributing to a more uniform and readable codebase.
410-410: LGTM: Fixed IP whitelist parsing and improved readabilityThe changes address the issue mentioned in PR #2372 regarding the 500 error caused by incorrect parsing of IP whitelist data. The new implementation correctly parses the CSV string into an array of IP addresses using
split(',').map((s) => s.trim()). This fix resolves the parsing error and ensures proper functionality of the IP whitelist feature.Additionally, the added empty line improves code readability by separating logical blocks.
To ensure the fix works as expected, we can run the following test:
Also applies to: 422-422
✅ Verification successful
Verified: IP whitelist parsing implemented correctly and readability improved
The IP whitelist parsing has been successfully verified, addressing the issue described in PR #2372. The implementation correctly parses the CSV string into an array of IP addresses using
split(',').map((s) => s.trim()), ensuring proper functionality of the IP whitelist feature.Additionally, the added empty line enhances code readability by separating logical blocks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Test IP whitelist parsing echo "Testing IP whitelist parsing..." ast-grep --lang typescript --pattern 'const ipWhitelist = $_.split(",").map((s) => s.trim());' if [ $? -eq 0 ]; then echo "IP whitelist parsing implemented correctly." else echo "IP whitelist parsing not found or implemented incorrectly." fiLength of output: 401
Line range hint
1-624: Overall improvements implemented, but clarification needed on missing plan checkThe changes in this PR successfully address the IP whitelist parsing issue mentioned in #2372, which should resolve the 500 error. The code readability has also been improved through consistent reformatting of type definitions and object declarations.
However, the PR title mentions a "Missing plan check," but there doesn't seem to be any explicit changes related to this in the reviewed code. Could you please clarify if the plan check has been implemented elsewhere or if it's still pending?
To confirm if any plan-related changes were made, we can run the following search:
apps/api/src/routes/v1_keys_verifyKey.test.ts (2)
521-521: LGTM! IP whitelist format updated correctly.The change from a JSON string to a comma-separated string for the
ipWhitelistis correct and aligns with the PR objectives. This update should resolve the 500 error mentioned in issue #2372 by ensuring proper parsing of the IP whitelist.
569-569: LGTM! IP whitelist format consistently updated.The change to use a comma-separated string for the
ipWhitelistis consistent with the update in the "with valid ip" test case. This ensures that both test cases use the same format for the IP whitelist, maintaining consistency across the test suite.apps/dashboard/lib/trpc/routers/api/updateIpWhitelist.ts (1)
84-84: No action neededThe addition of a blank line improves code readability and follows standard formatting conventions.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
chronark
left a comment
chronark
left a comment
There was a problem hiding this comment.
one issue, rest looks good
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
apps/dashboard/app/(app)/apis/[apiId]/settings/update-ip-whitelist.tsx (3)
33-33: Approved: Good shift to feature-based access control.The change from
plantofeaturesin theworkspaceprop type is a good move towards more granular feature-based access control. This aligns well with the PR objectives and allows for more flexible management of the IP whitelist feature.Consider using a more specific type for
featuresif possible, such as{ ipWhitelist: boolean }, to make the expected structure clearer and improve type safety.
Line range hint
82-121: Approved: Good implementation of conditional rendering based on feature flag.The updated rendering logic correctly uses the
isEnabledvariable to conditionally render the IP whitelist input or an upgrade alert. This change effectively implements the feature-based access control.Consider updating the alert message to reflect the new feature-based approach rather than mentioning the enterprise plan specifically. For example:
<AlertDescription> IP whitelists are not enabled for your workspace. Please contact support to enable this feature. </AlertDescription>
Line range hint
1-134: Summary: Successful implementation of feature-based IP whitelist control.The changes in this file effectively implement the feature-based control for IP whitelisting, addressing the issues mentioned in the PR objectives. The shift from plan-based to feature-based access control provides more flexibility and aligns well with the intended improvements.
Key points:
- The
workspaceprop now usesfeaturesinstead ofplan.- A new
isEnabledvariable clearly determines feature availability.- Conditional rendering based on the feature flag is correctly implemented.
These changes should resolve the issues related to IP whitelisting and improve the overall functionality of the component. The code is clean, well-structured, and follows React best practices.
As you continue to develop this feature, consider the following architectural advice:
- Implement a centralized feature flag management system if not already in place.
- Ensure that the backend API is also updated to respect these feature flags consistently.
- Consider adding analytics or logging to track usage of the IP whitelist feature across different workspaces.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- apps/api/src/pkg/keys/service.ts (5 hunks)
- apps/api/src/routes/v1_keys_verifyKey.ts (1 hunks)
- apps/dashboard/app/(app)/apis/[apiId]/settings/page.tsx (1 hunks)
- apps/dashboard/app/(app)/apis/[apiId]/settings/update-ip-whitelist.tsx (3 hunks)
- apps/dashboard/lib/trpc/routers/api/updateIpWhitelist.ts (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/dashboard/app/(app)/apis/[apiId]/settings/page.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/src/pkg/keys/service.ts
- apps/dashboard/lib/trpc/routers/api/updateIpWhitelist.ts
🧰 Additional context used
🔇 Additional comments (2)
apps/dashboard/app/(app)/apis/[apiId]/settings/update-ip-whitelist.tsx (1)
45-45: LGTM: Clear implementation of feature flag.The introduction of the
isEnabledvariable usingworkspace.features.ipWhitelistis a clear and effective implementation of the feature flag. This change directly addresses the PR objectives and simplifies the logic for enabling the IP whitelist feature.apps/api/src/routes/v1_keys_verifyKey.ts (1)
347-350: LGTM!The
identityobject is correctly constructed, defaultingmetato an empty object if undefined. This ensures that the response consistently includes themetafield as an object, which aligns with the response schema.
|
I will fix the merge conflict. |
… Tree Format (unkeyed#2238) * fix:changed the permission view * fixed issue comments * [autofix.ci] apply automated fixes * removed font --------- Co-authored-by: Andreas Thomas <dev@chronark.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
unkeyed#2273) * fix: show shallow permissions followed by nested in alphabetical order * fix: correct the sorting of nested permissions top level keys * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: Andreas Thomas <dev@chronark.com>
|
Awarding Flo4604: 300 points 🕹️ Well done! Check out your new contribution on oss.gg/Flo4604 |
7 participants
What does this PR do?
This checks whether the Workspace is allowed to have IP whitelisting.
It also fixes the parsing of the whitelisted IPs as which lead to a 500 error before.
Fixes #2372
Type of change
How should this be tested?
Manually send the updated whitelist topic call with an API and workspace ID with the pro or free plan. This should give an error and not go through.
Afterwards, test by doing a curl and overwriting the True-Client-IP to an invalid and then valid IP to see if the IP Whitelist works properly.
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes
ipWhitelistproperty formatting across various tests and services.Documentation
Tests
ipWhitelisthandling and added new tests for rate limit overrides.