markerdmann commented 1 month ago

Add a new storer specifically for OAuth2 to enable clients to choose regular database storing OR Oauth2 but not have to have both.
Stop storing OAuth2 credentials in a combined form inside username.
Add new events to capture OAuth events just like auth.
Have pass-through parameters for OAuth init urls, this allows us to pass additional behavior options (redirects and remember me) as well as other things that should be present on the page that is redirected to.
Context.LoadUser is now OAuth aware.
Remember's callbacks now include an OAuth check to see if a horribly packed state variable contains a flag to say that we want to be remembered.
Change the OAuth2 Callback to use Attributes instead of that custom struct to allow people to append whatever attributes they want into the user that will be saved.

Summary by CodeRabbit

New Features
- Introduced OAuth2 functionality with new events for OAuth success and failure.
- Added support for storing and retrieving OAuth2 information.
Enhancements
- Updated user loading mechanism to handle more complex key structures.
- Improved OAuth2 provider handling and callback functionality.
Bug Fixes
- Adjusted tests to align with new OAuth2 features and configurations.
Documentation
- Added comprehensive documentation for new OAuth2-related changes.

coderabbitai[bot] commented 1 month ago

Walkthrough

The changes introduce enhanced OAuth2 functionality across various components, including adding new event types, expanding configuration settings, updating context handling, and refining OAuth2 provider interactions. Tests have been updated to reflect these modifications, ensuring comprehensive coverage and validation of the new features.

Changes

Files	Change Summary
`callbacks.go`, `callbacks_test.go`	Added `EventOAuth` and `EventOAuthFail` constants and updated related tests.
`config.go`	Added `OAuth2Storer` field to the `Config` struct.
`context.go`, `context_test.go`	Added global variables for OAuth2 state and updated `LoadUser` function and related tests.
`internal/mocks/mocks.go`, `mocks_test.go`	Added `PutOAuth` and `GetOAuth` methods to `MockStorer` and related tests.
`oauth2.go`, `oauth2/oauth2.go`, `oauth2/providers.go`	Updated OAuth2Provider struct, reorganized storage methods, modified callback functions, and updated Google provider.
`oauth2/oauth2_test.go`, `oauth2/providers_test.go`	Updated tests to reflect changes in OAuth2 functionality and parameter handling.
`recover/recover_test.go`	Updated email handling and URL generation logic in tests.
`remember/remember.go`, `remember/remember_test.go`	Added `afterOAuth` function and related test for post-OAuth actions.
`storer.go`	Renamed and reordered OAuth2 constants, updated Storer interface, introduced `OAuth2Storer`.

Sequence Diagram(s) (Beta)

sequenceDiagram
    participant User
    participant OAuth2Provider
    participant Server
    participant Database

    User->>OAuth2Provider: Initiate OAuth2 Login
    OAuth2Provider-->>User: Redirect to Authorization Page
    User->>OAuth2Provider: Authorize and Get Code
    OAuth2Provider-->>Server: Send Authorization Code
    Server->>OAuth2Provider: Exchange Code for Token
    OAuth2Provider-->>Server: Return Access Token
    Server->>Database: Store OAuth2 Attributes
    Database-->>Server: Confirmation
    Server-->>User: Login Success

In the code's realm, where changes flow, OAuth2's dance begins to show. Events expand, new states arise, With tests and mocks to scrutinize. A rabbit's joy, in code's delight, Celebrates this OAuth flight.

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?

Share

- [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)

Tips

### Chat There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai): - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit .` - `Generate unit testing code for this file.` - `Open a follow-up GitHub issue for this discussion.` - Files and specific lines of code (under the "Files changed" tab): Tag `@coderabbitai` in a new review comment at the desired location with your query. Examples: - `@coderabbitai generate unit testing code for this file.` - `@coderabbitai modularize this function.` - PR comments: Tag `@coderabbitai` in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples: - `@coderabbitai generate interesting stats about this repository and render them as a table.` - `@coderabbitai show all the console.log statements in this repository.` - `@coderabbitai read src/utils.ts and generate unit testing code.` - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.` 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 as PR comments) - `@coderabbitai pause` to pause the reviews on a PR. - `@coderabbitai resume` to resume the paused reviews. - `@coderabbitai review` to trigger an incremental review. This is useful when automatic reviews are disabled for the repository. - `@coderabbitai full review` to full the review from scratch and review all the files again. - `@coderabbitai summary` to regenerate the summary of the PR. - `@coderabbitai resolve` resolve all the CodeRabbit review comments. - `@coderabbitai help` to get help. Additionally, you can add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. ### CodeRabbit Configration File (`.coderabbit.yaml`) - You can programmatically configure CodeRabbit by adding a `.coderabbit.yaml` file to the root of your repository. - Please see the [configuration documentation](https://docs.coderabbit.ai/guides/configure-coderabbit) for more information. - If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: `# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json` ### Documentation and Community - Visit our [Documentation](https://coderabbit.ai/docs) for detailed information on how to use CodeRabbit. - Join our [Discord Community](https://discord.com/invite/GsXnASn26c) to get help, request features, and share feedback. - Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements.

markerdmann commented 1 month ago

@coderabbitai review

coderabbitai[bot] commented 1 month ago

Actions Performed

Review triggered. > Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

sweep-ai[bot] commented 1 month ago

Sweep: PR Review

Authors of pull request: @aarondl

This pull request added support for OAuth2 authentication and improved the handling of OAuth2-related events and data storage.

Two new events, EventOAuth and EventOAuthFail, were added to the Event enumeration in callbacks.go, and corresponding string representations and indexes were updated. The Config struct in config.go was extended to include an OAuth2Storer field, allowing separate management of OAuth2-related storage.

In context.go, the LoadUser method was modified to handle keys containing semicolons, enabling the retrieval of user data via OAuth2Storer. Corresponding tests in context_test.go were updated to verify this functionality.

The OAuth2Provider struct in oauth2.go was updated to use authboss.Attributes instead of a custom OAuth2Credentials struct, simplifying the handling of OAuth2 user data. The oauthCallback function was enhanced to handle additional URL parameters and to store OAuth2 user data using OAuth2Storer.

The remember package was updated to support OAuth2 authentication by adding an afterOAuth method, which sets a remember-me token for OAuth2-authenticated users. Tests were added to verify this functionality.

Finally, the OAuth2Storer interface was defined in storer.go, providing methods for storing and retrieving OAuth2 user data, and the Google OAuth2 provider callback in providers.go was updated to return authboss.Attributes.

Sweep Found These Issues

callbacks.go

The condition in the String method incorrectly checks i+1 >= Event(len(eventIndexes)) instead of i >= Event(len(eventIndexes)-1), which could lead to an out-of-bounds error when i is the last valid index.

context.go

The new conditional check in LoadUser does not handle the case where the key contains a semicolon but is not in the expected format, potentially leading to unexpected behavior or errors.

internal/mocks/mocks.go

The PutOAuth method does not handle concurrent access to the Users map, potentially leading to race conditions.

mocks_test.go

The GetOAuth method assumes that the "email" and "password" attributes are always present and of type string, which could cause a runtime panic if these assumptions are violated.

oauth2/oauth2.go

The change in the oauthCallback function to split the state parameter and validate only the first part may introduce issues if the state parameter format is not consistent or if additional data in the state parameter is not handled correctly.

recover/recover_test.go

The change from authboss.Cfg.HostName to authboss.Cfg.RootURL may cause issues if RootURL is not properly set or differs significantly from HostName, potentially leading to incorrect URLs in recovery emails.

remember/remember.go

The afterOAuth method incorrectly checks if the length of splState is less than 0, which is always false and should be checking if the length is less than 2.
Sweep has identified a redundant function: The new function afterOAuth is redundant because its purpose and functionality are already covered by the existing afterOAuth function in remember.go.

remember/remember_test.go

Sweep has identified a redundant function: The new function TestAfterOAuth is redundant as it duplicates the existing TestAfterOAuth method in the remember/remember_test.go file.

storer.go

The Put method in the Storer interface now explicitly states it should not store if the key does not exist, which may require changes in implementations to ensure compliance with this behavior.
Sweep has identified a redundant function: The new function PutOAuth(uid, provider string, attr Attributes) error is redundant because its functionality is already covered by the existing method authboss.Cfg.OAuth2Storer.PutOAuth(uid, provider, user) in oauth2/oauth2.go.
Sweep has identified a redundant function: The new function is redundant because the GetOAuth methods in mockStorer and MockStorer already provide the same functionality.

Potential Issues

Sweep is unsure if these are issues, but they might be worth checking out.

config.go

The new OAuth2Storer field is added to the Config struct but is not initialized in the NewConfig function, which could lead to a nil pointer dereference if accessed without being set.

oauth2/providers_test.go

The test function does not handle the case where the Google function returns a nil map, which could cause a runtime panic when accessing the map.