Unauthorized redirect handling config #6051
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
changelog:solidus_backend
mamhoff
added a commit
to mamhoff/solidus_auth_devise
that referenced
this pull request
Dec 21, 2024
This makes the `prepare_{frontend,backend}` methods which instantiate
the base controller and the admin base controller, requiring all of
ActionController::Base, unnecessary to call for newer Solidus versions.
Depends on solidusio/solidus#6051
3 tasks
mamhoff
added a commit
to mamhoff/solidus_auth_devise
that referenced
this pull request
Dec 21, 2024
This makes the `prepare_{frontend,backend}` methods which instantiate
the base controller and the admin base controller, requiring all of
ActionController::Base, unnecessary to call for newer Solidus versions.
Depends on solidusio/solidus#6051
mamhoff
force-pushed
the
unauthorized-redirect-handling-config
branch
from
f24f2ae to
7753b24
Compare
adammathys
approved these changes
Jan 3, 2025
forkata
approved these changes
Jan 8, 2025
There was a problem hiding this comment.
Thanks @mamhoff! I think there are some failing specs because we're modifying global configuration in one of the specs added, but otherwise this change looks great and it'll make overriding this behaviour much easier on stores that require more granular permissions.
mamhoff
force-pushed
the
unauthorized-redirect-handling-config
branch
5 times, most recently
from
c259ad3 to
cc0a2eb
Compare
jarednorman
approved these changes
Jan 9, 2025
This lambda has to be set on the instantiated class, requiring the controller it's set on to be autoloaded on boot. Loading a controller also loads all helpers, and that takes quite a bit of time. So this comes with a performance benefit, because neither the controller nor the handler need to be loaded at configuration time.
The previous `unauthorized_redirect` can really only be set globally, but it's a common use case to handle unauthorized access differently for the admin. This adds a configuration option for the admin base controller, allowing customizers to set that behavior without having to patch any controller.
Customizers can now set their own unauthorized_redirect class with any behavior from the controller they might need. There's no need for this suggestion (and the comment is not entirely true either, as the handler provided does not re-raise the exception, but raises another one).
This make sure the admin also uses the configured redirect handler class.
tvdeyen
force-pushed
the
unauthorized-redirect-handling-config
branch
from
cc0a2eb to
8daded5
Compare
|
This change is VERY much appreciated 🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This replaces the
unauthorized_redirectlambda that can be set on any controller with two configurable classes that are by default both set to the same defaultSpree::UnauthorizedRedirectHandler. Two, because frontend and backend will require different strategies in many cases (not least in the case of thesolidus_auth_deviseextension).Care has been taken to maintain the legacy behavior and print out deprecation warnings.
A big part of the reason is that setting the lamda on the controllers requires loading the controllers, making us need to use a
config.to_prepareblock as well as making us inadvertently eager-load a lot of code.Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: