Fix prompt behaviour
#58
Conversation
WalkthroughThe pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant AuthService
participant GenerateAuthUrl
Client->>GenerateAuthUrl: Request Auth URL
alt Registration Type
GenerateAuthUrl-->>AuthService: Set prompt=create
else Login Type
GenerateAuthUrl-->>AuthService: Set prompt=login
else Default
GenerateAuthUrl-->>AuthService: Set prompt=none
end
AuthService-->>Client: Return Authentication URL
The sequence diagram illustrates how the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (2)
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)
lib/types.ts (1)
Breaking Change: The
offline_accessscope value has been changed toofflineThe verification confirms this is a breaking change:
- The enum value in
lib/types.tsshowsoffline_access = "offline", meaning the enum key isoffline_accessbut the actual value sent to the server is"offline"- Test files in
lib/utils/generateAuthUrl.test.tsuseScopes.offline_accessin their test cases- The
mapLoginMethodParamsForUrl.tsincludesofflinein the default scope string:"email profile openid offline"This inconsistency between the enum key (
offline_access) and its value (offline) could cause confusion and potential issues:
- Code using string literals expecting "offline_access" will break
- The enum key suggests a different value than what's actually being sent
- The default scope in
mapLoginMethodParamsForUrl.tsuses "offline" while the enum key is "offline_access"🔗 Analysis chain
Line range hint
4-4: Breaking Change: Scope value modified from "offline_access" to "offline"This change modifies an enum value which could break existing integrations that rely on the "offline_access" scope.
Let's verify the impact:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of offline_access scope rg -i "offline_access" --type tsLength of output: 281
Script:
#!/bin/bash # Let's check the actual content of lib/types.ts to understand the enum definition cat lib/types.tsLength of output: 2944
Script:
#!/bin/bash # Let's check the test file to understand the usage context cat lib/utils/generateAuthUrl.test.tsLength of output: 6990
Script:
#!/bin/bash # Let's check the implementation of generateAuthUrl to see how the scope is used cat lib/utils/generateAuthUrl.tsLength of output: 2534
Script:
#!/bin/bash # Let's check how the scope is mapped in the URL parameters cat lib/utils/mapLoginMethodParamsForUrl.tsLength of output: 1036
lib/utils/generateAuthUrl.test.ts (1)
Line range hint
95-150: Fix duplicate test descriptions.There are two test cases with identical descriptions: "should handle default responseType if not provided". This makes it unclear what specific scenario each test is validating. Consider updating the descriptions to clearly differentiate between the test cases, for example:
-it("should handle default responseType if not provided", async () => { +it("should handle default responseType with state parameter", async () => {and
-it("should handle default responseType if not provided", async () => { +it("should handle default responseType without state parameter", async () => {
🧹 Nitpick comments (1)
lib/utils/generateAuthUrl.test.ts (1)
39-64: Consider grouping related test scenarios.While the test case correctly validates the registration flow, consider organizing related test cases using
describeblocks. For example:describe('generateAuthUrl', () => { describe('authentication types', () => { it('should generate login URL...', async () => { // existing login test }); it('should generate register URL...', async () => { // your new registration test }); }); });This organization would make the test structure more maintainable as you add more authentication type scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/main.test.ts(1 hunks)lib/types.ts(3 hunks)lib/utils/generateAuthUrl.test.ts(5 hunks)lib/utils/generateAuthUrl.ts(3 hunks)
🔇 Additional comments (8)
lib/types.ts (3)
8-12: Well-structured enum following OAuth 2.0 standards
The PromptTypes enum values align with standard OAuth 2.0 prompt parameter values, providing type safety for authentication flows.
81-86: Improved type safety for prompt parameter
Good improvements:
- Clear documentation for each prompt value
- Making the property optional aligns with OAuth 2.0 spec
- Type safety through enum usage prevents invalid values
104-104: Documentation updated to reflect scope change
Documentation correctly reflects the new "offline" scope value, but please note this is related to the breaking change mentioned above.
lib/utils/generateAuthUrl.ts (2)
2-2: LGTM: Required import for prompt type handling
61-63: Good default prompt handling for registration
The logic appropriately sets prompt=create for registration flows when not explicitly provided. Consider adding a test case to verify this behavior.
Let's check if there's a test case for this:
✅ Verification successful
Test coverage exists for registration prompt behavior
The test file already includes a test case "should generate the register URL when type is 'registration'" that verifies the prompt=create parameter is correctly set for registration flows. The test explicitly checks that the generated URL includes prompt=create in the expected URL string when IssuerRouteTypes.register is used, even when no prompt is provided in the options.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related test cases
rg -A 5 "IssuerRouteTypes.register.*generateAuthUrl" --type ts
Length of output: 64
Script:
#!/bin/bash
# Let's search for test files related to generateAuthUrl
fd ".*\.test\.ts$" | xargs rg -l "generateAuthUrl"
# Also search for any test files containing IssuerRouteTypes.register
fd ".*\.test\.ts$" | xargs rg -l "IssuerRouteTypes.register"
# Let's also check the implementation to understand the test coverage needs
rg -A 5 "function generateAuthUrl" --type ts
Length of output: 243
Script:
#!/bin/bash
# Let's examine the test file content to check for registration prompt testing
cat lib/utils/generateAuthUrl.test.ts
Length of output: 6990
lib/main.test.ts (1)
32-32: LGTM: Export test updated correctly
The PromptTypes enum is correctly added to the expected exports list, maintaining alphabetical order.
lib/utils/generateAuthUrl.test.ts (2)
22-22: LGTM! Expected URL updated correctly.
The removal of the prompt parameter from the expected URL aligns with the new prompt behavior.
Also applies to: 27-27
76-76: Verify prompt type value against PromptTypes enum.
The test correctly uses "login" as the prompt value. However, let's verify that this matches the PromptTypes enum definition.
Also applies to: 79-79, 84-84
✅ Verification successful
The prompt value "login" is correctly aligned with PromptTypes enum
The test uses "login" which matches exactly with the PromptTypes.login enum value defined in lib/types.ts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for PromptTypes enum definition to verify the value
ast-grep --pattern 'enum PromptTypes {
$$$
}'
Length of output: 212
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
lib/utils/generateAuthUrl.test.ts (2)
Line range hint
92-150: Refactor duplicate test cases.There are two test cases with identical names testing similar behavior. Consider combining them or giving them more specific names to clarify their distinct purposes.
Suggestions:
- Rename the first test to indicate it validates state parameter
- Rename the second test to indicate it tests auto-generated state
- Or combine them into a single test case with multiple assertions
Line range hint
173-175: Fix incorrect storage key usage for code verifier.There's a bug in the test where
codeVerifieris being retrieved usingStorageKeys.stateinstead of the correct storage key.Apply this change:
- const codeVerifier = await store.getSessionItem(StorageKeys.state); + const codeVerifier = await store.getSessionItem(StorageKeys.codeVerifier);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/utils/generateAuthUrl.test.ts(5 hunks)lib/utils/generateAuthUrl.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/utils/generateAuthUrl.ts
🧰 Additional context used
🪛 GitHub Check: main (20.x)
lib/utils/generateAuthUrl.test.ts
[failure] 76-76:
Type '"login"' is not assignable to type 'PromptTypes | undefined'.
🔇 Additional comments (2)
lib/utils/generateAuthUrl.test.ts (2)
22-22: LGTM! URL structure aligns with PR objectives.
The expected URL correctly excludes the start_page parameter while maintaining all required security parameters.
39-64: Well-structured test case for registration flow.
The test case correctly validates that:
- Registration URLs include
prompt=create - Security parameters (nonce, code_challenge) are properly handled
- The URL structure aligns with the new registration flow
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/utils/generateAuthUrl.test.ts (1)
Line range hint
39-179: Consider adding test coverage for PromptTypes.noneWhile the tests cover
PromptTypes.loginandPromptTypes.create, consider adding a test case forPromptTypes.noneto ensure complete enum coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/utils/generateAuthUrl.test.ts(7 hunks)
🔇 Additional comments (6)
lib/utils/generateAuthUrl.test.ts (6)
2-2: LGTM: Import changes align with new PromptTypes implementation
The addition of PromptTypes to the imports is consistent with the PR's objective to use the prompt parameter instead of start_page.
22-22: LGTM: URL expectation correctly updated
The expected URL has been updated to remove the start_page parameter, which aligns with the PR's objective to deprecate this parameter.
39-64: LGTM: Well-structured test for registration flow
The new test case properly validates the registration URL generation with the correct prompt parameter, following the established testing patterns and verifying essential security parameters.
76-76: Type error resolved: Using PromptTypes enum
The change from string literal to PromptTypes.login resolves the previously identified type error.
100-100: LGTM: Consistent usage of PromptTypes enum
The test cases have been properly updated to use PromptTypes.create, maintaining consistency with the new prompt parameter implementation.
Also applies to: 129-129
167-167: LGTM: State management test properly updated
The test case has been updated to use PromptTypes.login while maintaining the essential state management validation logic.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #58 +/- ##
==========================================
+ Coverage 85.48% 85.58% +0.09%
==========================================
Files 32 32
Lines 875 881 +6
Branches 166 168 +2
==========================================
+ Hits 748 754 +6
Misses 127 127
|
Explain your changes
start_pagewas a Kinde implementation to allow theregisterpage to be requested as part of the auth url, prior to the spec addingcreateas an option for thepromptparameter.This change removes
start_pagein favour of usingprompt(this shouldn't cause breaking changes as it is already the way that the PKCE SDK operates)The inclusion of either
start_page=loginorprompt=loginwill always force Kinde to show the Login screen which often is not desired behaviour. Most of the time founders just wish the SSO session to continue. This PR removes the prompt being added by default and only adds when requested or if the a register auth url link has been requested.Also fixed a small typo in one of the types
Types and Tests updated
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.