Conversation
- Implemented GetAuthURLWithInvitation method to include invitation_code and is_invitation parameters in the authentication URL. - Updated UseKindeAuth function to utilize the new method for redirecting users with invitation codes. - Added comprehensive tests for invitation code handling in authorization code flow. This enhancement allows for better user onboarding through invitation codes.
- Added multiple test cases to validate the behavior of invitation codes in various scenarios, including precedence over options, handling of special characters, and whitespace-only codes. - Ensured that invitation codes are correctly included in the generated authentication URLs under different conditions. - Improved test coverage for the GetAuthURLWithInvitation method to ensure robustness and reliability.
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdds invitation-code support to the authorization code flow and middleware: new GetAuthURLWithInvitation(invitationCode string) and WithInvitationCode(option) add Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GinMiddleware as Gin Middleware
participant AuthFlow as AuthorizationCodeFlow
participant Kinde as Kinde Auth Service
Client->>GinMiddleware: Request (unauthenticated)
GinMiddleware->>GinMiddleware: Check auth
alt no auth
GinMiddleware->>GinMiddleware: Read `invitation_code` param
GinMiddleware->>AuthFlow: GetAuthURLWithInvitation(invitationCode)
AuthFlow->>AuthFlow: Build base auth URL (state, PKCE, etc.)
alt invitation present
AuthFlow->>AuthFlow: Append `invitation_code` & `is_invitation=true`
end
AuthFlow-->>GinMiddleware: auth URL
GinMiddleware-->>Client: 302 Redirect to auth URL
end
Client->>Kinde: Follow redirect (auth request)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (3)
oauth2/authorization_code/options.go (1)
146-155: Consider validating or trimming the invitation code input.The implementation correctly follows the existing pattern. However, whitespace-only invitation codes (e.g.,
" ","\t") will pass the!= ""check and be added to the URL. Consider trimming whitespace or validating the format to prevent potentially invalid codes from being sent.🔎 Suggested enhancement with whitespace trimming
func WithInvitationCode(invitationCode string) Option { return func(s *AuthorizationCodeFlow) { + invitationCode = strings.TrimSpace(invitationCode) if invitationCode != "" { WithAuthParameter("invitation_code", invitationCode)(s) WithAuthParameter("is_invitation", "true")(s) } } }You'll need to import
"strings"at the top of the file.oauth2/authorization_code/authorization_code.go (1)
156-161: Implementation is correct with proper parameter precedence.The logic correctly ensures that an invitation code provided as a parameter takes precedence over one set via options, as the
query.Set()calls happen after mergingflow.authURLOptions. URL encoding viaquery.Encode()at line 168 ensures the invitation code is safely escaped.Similar to the
WithInvitationCodeoption, you may want to consider trimming whitespace from theinvitationCodeparameter at the start of this method to maintain consistency and prevent whitespace-only codes.oauth2/authorization_code/authorization_code_test.go (1)
329-356: Test has incomplete verification of whitespace behavior.The test comments (lines 349-352) acknowledge that whitespace-only invitation codes will be added since the check is
!= "", but the test only asserts thatauthURLis not empty (line 353). This doesn't actually verify whether the whitespace invitation code was included in the URL or not.Consider enhancing the test to explicitly verify the actual behavior:
🔎 Suggested test enhancement
for _, whitespaceCode := range testCases { t.Run(fmt.Sprintf("whitespace_%q", whitespaceCode), func(t *testing.T) { authURL := kindeAuthFlow.GetAuthURLWithInvitation(whitespaceCode) - // Whitespace-only codes should still be added (they're not empty strings) - // but we should verify the behavior - // Actually, since we check for != "", whitespace will be added - // This might be a design decision - let's test what actually happens assert.NotEmpty(authURL, "AuthURL should not be empty") + // Verify that whitespace codes are actually added to the URL + assert.Contains(authURL, "invitation_code=", "Whitespace invitation code should be added") + assert.Contains(authURL, "is_invitation=true", "is_invitation should be set for whitespace codes") }) }This makes the test verify the actual behavior and documents whether whitespace codes are treated as valid invitation codes.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frameworks/gin_kinde/gin_kinde.gooauth2/authorization_code/authorization_code.gooauth2/authorization_code/authorization_code_test.gooauth2/authorization_code/options.go
🧰 Additional context used
🧬 Code graph analysis (1)
oauth2/authorization_code/options.go (2)
oauth2/client_credentials/options.go (2)
Option(12-12)WithAuthParameter(16-24)oauth2/authorization_code/authorization_code.go (1)
AuthorizationCodeFlow(80-92)
🔇 Additional comments (6)
frameworks/gin_kinde/gin_kinde.go (1)
154-156: LGTM! Clean integration of invitation code support.The middleware correctly extracts the
invitation_codequery parameter and passes it to the new method. Thectx.Query()call safely returns an empty string if the parameter is not present, which is the desired behavior.oauth2/authorization_code/authorization_code.go (2)
45-48: LGTM! Well-documented interface addition.The new method is properly documented and follows Go conventions. The interface change is backward compatible for users of the interface (they can continue using
GetAuthURL()), though implementers of the interface will need to add this method.
139-141: LGTM! Clean refactoring to eliminate duplication.Good use of delegation pattern. This preserves the existing API while reusing the implementation, which reduces code duplication and maintenance burden.
oauth2/authorization_code/authorization_code_test.go (3)
91-170: LGTM! Comprehensive basic test coverage.These tests effectively verify the core functionality:
- Invitation codes are properly included in the URL when provided
- Empty invitation codes are correctly handled (parameters not added)
- The option function correctly sets the necessary parameters
172-328: LGTM! Excellent coverage of edge cases and interactions.This suite thoroughly tests:
- Parameter precedence over options (critical for correct behavior)
- Integration with other features (PKCE, audience)
- URL encoding of special characters, spaces, and unicode
- Stateless behavior across multiple calls
- Option values used when parameter is empty
The test coverage here significantly increases confidence in the implementation.
358-379: LGTM! Verifies GetAuthURL() delegation works correctly.This test confirms that the refactored
GetAuthURL()method properly includes invitation codes set via options, validating that the delegation toGetAuthURLWithInvitation("")works as intended.
|
Good morning, @BrandtKruger. I just finished going through this. Functionality looks great, the only things I would like to ask you to look at is Whitespace trimming in GetAuthURLWithInvitation and WithInvitationCode as it currently doesn't handle it as js-utils does. I also noticed a small typo on line 44-46 in authorization_code.go. Happy to approve after those two minor changes are done. |
- Added a new test case to validate that whitespace-only invitation codes do not add parameters to the authentication URL. - Updated the invitation code handling in the authorization code flow to trim whitespace, ensuring that only valid codes are processed. - Enhanced existing tests to confirm that the generated authentication URLs do not include invitation_code or is_invitation parameters when provided with whitespace-only codes.
Changes done Keegan. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
oauth2/authorization_code/options.go (1)
331-342: Dual code paths for invitation parameters may cause confusion.
WithInvitationCodesetsinvitation_codeandis_invitationviaWithAuthParameterintoauthURLOptions, whileGetAuthURLWithInvitationindependently sets the same parameters viaquery.Set. If both are used together (e.g., option applied and a non-empty code passed toGetAuthURLWithInvitation),query.Setsilently overwrites the option-set values. This works but the overlapping responsibility is a maintenance hazard.Consider consolidating so that
GetAuthURLWithInvitationinternally appliesWithInvitationCode(or vice versa) rather than reimplementing the same logic.
Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
Explain your changes
team member invitations
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.