markerdmann commented 4 months ago

Change response to be more central to Authboss. Make sure it has useful methods and works with the new rendering idioms.
Change the load user methods to all work with context keys, and even be able to set context keys on the current request to avoid setting contexts everywhere in the code base.

Summary by CodeRabbit

New Features
- Introduced context-based user authentication and session handling.
- Added new methods to ConfirmStoreLoader and ConfirmStorer interfaces for better token and confirmation management.
Improvements
- Enhanced security by updating SessionStoreMaker usage with Make method.
- Refactored callback initialization for improved code readability.
Bug Fixes
- Corrected type assertions in user-related test functions.
- Fixed template handling and redirection logic in response functions.
Refactor
- Renamed ConfirmStorer to ConfirmStoreLoader.
- Updated method signatures to use context.Context.
Tests
- Added new test functions for user authentication scenarios.
- Removed outdated attribute handling tests.
Documentation
- Updated ClientStoreMaker interface documentation for better clarity on security considerations.

coderabbitai[bot] commented 4 months ago

Walkthrough

The recent updates to the authboss package introduce enhancements to the Authboss struct, including new fields for template management and rendering. Significant refactoring has been done across various files to improve session handling, context usage, and error management. Additionally, new methods for handling user authentication, email responses, and redirects have been added, along with extensive updates to test files to reflect these changes.

Changes

File(s)	Change Summary
`authboss.go`, `authboss_test.go`	Added fields `templateNames` and `renderer` to `Authboss` struct. Updated `Init` method and tests to initialize `renderer`.
`callbacks.go`, `callbacks_test.go`	Refactored map initialization in `Callbacks` struct. Removed unused imports and logging. Updated tests accordingly.
`client_storer.go`, `client_storer_test.go`	Updated `ClientStoreMaker` interface documentation and method calls to use `Make`. Adjusted tests to match.
`confirm/confirm.go`	Renamed `ConfirmStorer` to `ConfirmStoreLoader`. Added new methods and updated signatures to use `context.Context`.
`context.go`, `context_test.go`	Introduced functions for managing user authentication and session handling. Added corresponding tests.
`errors.go`, `errors_test.go`	Removed `AttributeErr` and `ErrAndRedirect` structs. Updated error message format in `RenderErr`. Adjusted tests.
`expire.go`, `expire_test.go`	Updated method calls to use `Make` for session creation. Adjusted tests with `defer` statements and `newMockClientStoreMaker`.
`internal/mocks/mocks.go`	Added `context` import. Renamed fields and updated methods in `MockUser` struct. Introduced `MockStoreLoader` and updated `MockFailStorer`.
`internal/response/response.go`	Restructured template handling logic. Commented out template loading and redirection functionality.
`mailer_test.go`	Updated expected return value in `TestBoundary`.
`mocks_test.go`	Added `mockClientStoreMaker`, `mockRenderLoader`, and `mockRenderer` types for testing.
`module.go`, `module_test.go`	Added `Templates` method to `Modularizer` interface. Updated `loadModule` function. Adjusted tests.
`renderer.go`	Updated `RenderLoader` and `Renderer` interfaces to handle arrays and template names respectively.
`response.go`	Added functionality for handling HTTP responses, including rendering templates, sending emails, and redirects.
`router.go`	Renamed `contextRoute` to `abHandler`. Updated `ServeHTTP` method and added `isAPIRequest` function.
`storer_test.go`	Removed code related to attributes and binding. Focused on testing casing style conversions.

In the realm of code where changes bloom,
The Authboss struct found its room.
With templates and renderers, new paths to tread,
Context and sessions, smoothly led.
Errors refined, methods anew,
A rabbit's joy in code review.
🌸🐇✨

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 4 months ago

@coderabbitai review

coderabbitai[bot] commented 4 months 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.

markerdmann commented 4 months ago

@coderabbitai add tests

coderabbitai[bot] commented 4 months ago

[!TIP] For best results, initiate chat on the files or code changes.

@markerdmann, I'll add unit tests for several key components affected by the changes in this PR. Here are some examples for the Authboss struct in authboss.go. You can extend these tests similarly for other components.

package authboss_test

import (
    "testing"
    "github.com/stretchr/testify/assert"
    "github.com/markerdmann/authboss"
    "context"
)

func TestAuthboss_Init(t *testing.T) {
    ab := authboss.Authboss{}
    err := ab.Init("module1", "module2")
    assert.NoError(t, err)
    assert.NotNil(t, ab.Renderer)
    assert.Equal(t, []string{"template1", "template2"}, ab.TemplateNames)
}

func TestAuthboss_FlashSuccess(t *testing.T) {
    ab := authboss.Authboss{}
    w := httptest.NewRecorder()
    r := httptest.NewRequest("GET", "/", nil)
    result := ab.FlashSuccess(w, r)
    assert.Equal(t, "success", result)
}

func TestAuthboss_FlashError(t *testing.T) {
    ab := authboss.Authboss{}
    w := httptest.NewRecorder()
    r := httptest.NewRequest("GET", "/", nil)
    result := ab.FlashError(w, r)
    assert.Equal(t, "error", result)
}

These tests check the initialization of the Authboss struct and its methods for flashing success and error messages. You can add similar tests for other modified methods and new functionalities. If you need tests for specific methods or files, please let me know!

sweep-ai[bot] commented 4 months ago

Sweep: PR Review

Authors of pull request: @aarondl

This pull request enhanced the Authboss library by adding template management and rendering capabilities, improving user session handling, and refactoring various components for better maintainability and functionality.

The Authboss struct was updated to include templateNames and renderer fields, enabling it to manage and render templates. The Init method now initializes the renderer with the specified template names. The UpdatePassword method was refactored to use the Storer interface for user interactions and simplified by removing reflection logic.

New context-related functions were added in context.go to handle user sessions more effectively, including methods to load the current user and user ID into the request context. Corresponding tests were added in context_test.go to ensure these functions work correctly.

The ConfirmStorer interface was split into ConfirmStoreLoader and ConfirmStorer to separate user loading and confirmation attribute management, improving code clarity. The callbacks.go file was cleaned up by removing unused imports and logging statements, and the NewCallbacks function was updated for better readability.

The RenderLoader and Renderer interfaces were modified to handle multiple template names and include content type in the render output. The response.go file introduced new methods for handling HTTP responses and emails, centralizing response logic and improving maintainability.

Finally, various tests were updated or added to ensure the new functionalities and refactored components work as expected, including changes in authboss_test.go, router_test.go, and storer_test.go.

Sweep Found These Issues

authboss.go

The UpdatePassword method remains non-functional as it returns a "not implemented" error, which could lead to runtime errors if this method is called.

authboss_test.go

The TestAuthbossUpdatePassword and TestAuthbossUpdatePasswordFail functions are marked as skipped, which means the functionality they are supposed to test is not being verified.

callbacks.go

The removal of error logging in the FireAfter method means that errors occurring during callback execution will no longer be logged, potentially reducing the amount of diagnostic information available during error handling.

callbacks_test.go

The removal of log output verification in TestCallbacksAfterErrors means the test no longer checks if error messages are correctly logged, potentially missing issues with logging.

confirm/confirm.go

The removal of the ctx *authboss.Context parameter in the confirmHandler function may lead to issues accessing context-specific data, as it no longer directly receives the authboss.Context.

context.go

The LoadCurrentUserID method modifies the request context but does not handle potential errors from context.WithValue, which could lead to context corruption.
Sweep has identified a redundant function: The new function is redundant because its functionality is already fully covered by the existing CurrentUserID method in context.go.
Sweep has identified a redundant function: The new function CurrentUserIDP is redundant because an identical function with the same name and functionality already exists in the codebase.
Sweep has identified a redundant function: The new function is redundant because it duplicates the existing CurrentUser method, which already performs the same tasks.
Sweep has identified a redundant function: The new function CurrentUserP is redundant because an identical function with the same name and functionality already exists in the codebase.
Sweep has identified a redundant function: The new function is redundant because the existing currentUser method in context.go already performs the same tasks and serves the same purpose.
Sweep has identified a redundant function: The new function LoadCurrentUserID is redundant because its functionality is already covered by the existing LoadCurrentUserID function in the codebase.
Sweep has identified a redundant function: The new function LoadCurrentUserIDP is redundant as its functionality is already covered by the existing LoadCurrentUserIDP method in context.go.
Sweep has identified a redundant function: The new function is redundant because it duplicates the functionality of the existing LoadCurrentUser method.
Sweep has identified a redundant function: The new function LoadCurrentUserP is redundant as it duplicates the functionality of the existing LoadCurrentUserP method.

context_test.go

The recover function in TestCurrentUserIDP, TestCurrentUserP, TestLoadCurrentUserIDP, and TestLoadCurrentUserP uses t.Failed() instead of t.Fail() or t.Fatalf(), which does not stop the test execution immediately.

errors.go

The removal of AttributeErr and its methods eliminates the ability to handle and report specific attribute retrieval and type mismatch errors, which could lead to less informative error handling and debugging.
The removal of ErrAndRedirect and its methods removes the capability to handle errors that require a redirect response, potentially affecting error management and user experience.

expire.go

The change to use a.SessionStoreMaker.Make(w, r) assumes that SessionStoreMaker is always an object with a Make method, which could cause runtime errors if SessionStoreMaker is not properly initialized or if the Make method does not exist.

expire_test.go

The deferred function to reset nowTime to its original state is necessary to prevent side effects on other tests, but if nowTime is used in other tests without similar precautions, it could lead to inconsistent test results.

internal/mocks/mocks.go

The removal of MockStorer and its methods without providing equivalent functionality in MockStoreLoader may lead to missing functionality for creating, updating, and retrieving user data and tokens.

internal/response/response.go

The removal of the ErrTemplateNotFound variable will cause runtime errors in the reactivated functions that reference this variable for error handling.

mailer_test.go

The change in the expected boundary string may cause the test to fail if the boundary method has not been updated to generate the new expected string "fe3fhpsm69lx8jvnrnju0wr".

module.go

The Templates method in the Modularizer interface is not checked for nil or empty slices, which could lead to potential issues if a module returns nil or an empty slice.

renderer.go

The change to the Render method signature in the Renderer interface may break existing implementations that do not expect the additional name parameter and the new return value contentType.

response.go

The redirectAPI method does not set a default status code if ro.Code is zero, potentially causing issues with clients expecting a status code.
Sweep has identified a redundant function: The new function is redundant because its purpose and functionality are already covered by the existing Respond method in response.go.
Sweep has identified a redundant function: The new function is redundant as its functionality is already covered by the existing Email function in response.go and RenderEmail function in internal/response/response.go.
Sweep has identified a redundant function: The new function is redundant because the existing Redirect function in response.go already performs the same tasks and operations.
Sweep has identified a redundant function: The new function is redundant because it duplicates the existing redirectAPI method in the response.go file, which already handles the same functionality.
Sweep has identified a redundant function: The new function redirectNonAPI is redundant because an identical function with the same name and functionality already exists in the response.go file at line 149.

router.go

The removal of the redirectIfLoggedIn function may cause issues if any routes previously relied on this logic to redirect logged-in users appropriately.
The new ServeHTTP method in abHandler does not initialize a context, which may lead to issues if any handlers rely on context initialization.
The LoadCurrentUserID function call in the new ServeHTTP method may fail and cause a 500 error if the user ID cannot be loaded, potentially affecting all routes.

router_test.go

The removal of the TestRouter_Redirect function eliminates the automated test coverage for redirect scenarios, potentially allowing redirect-related bugs to go undetected.
Sweep has identified a redundant function: The new function is redundant because the Templates method in both testRouterModule and testModule already provides the same functionality.

storer.go

The removal of the DataType type and its related constants and methods could break any existing functionality that relies on these definitions for handling specific data types.

Potential Issues

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

authboss_test.go

The change from us := userStruct.(*mockUser) to us := userStruct.(mockStoredUser) may cause a runtime panic if userStruct is not of type mockStoredUser.

confirm/confirm.go

The change from *authboss.Context to context.Context in the afterRegister function may cause issues with accessing ctx.User and other context-related operations that rely on authboss.Context.

context.go

The CurrentUserID method does not handle the case where session.Get(SessionKey) returns an error, which could lead to unexpected behavior.
The LoadCurrentUserIDP and LoadCurrentUserP methods panic on error, which could lead to application crashes if not handled properly.

context_test.go

The recover() function in TestCurrentUserIDP, TestCurrentUserP, TestLoadCurrentUserIDP, and TestLoadCurrentUserP does not check if the recovered value is nil, which could cause a panic when trying to assert it as an error.

response.go

The Respond method does not handle the case where a.renderer.Render returns an empty mime type, potentially leading to incorrect content type headers.

markerdmann / authboss

More gigantic edits. #9

Summary by CodeRabbit

Walkthrough

Changes

Sweep: PR Review

Sweep Found These Issues

Potential Issues