-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a separate permission to access the admin panel #303
base: main
Are you sure you want to change the base?
Add a separate permission to access the admin panel #303
Conversation
skip_before_action :authorize_admin | ||
|
||
def forbidden | ||
authorize! :read, ::Spree::AdminPanel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should define Spree::AdminPanel
in spree_core, and then use it only in spree_backend. Maybe a cleaner version would be checking whether there's a :read
permission on Spree::Admin
(which is already a module defined in spree_backend)?
https://github.com/CanCanCommunity/cancancan/blob/develop/docs/define_check_abilities.md#can-subjects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, maybe this authorize could be moved to the Admin::BaseController
, and we could handle the redirection to 404 page manually there?
render status: 403 | ||
end | ||
|
||
rescue_from CanCan::AccessDenied do |_exception| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we need this one for? I think this will also change the behavior of other controllers, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Spree::Core::ControllerHelper::Auth
there is access handling (which I guess fires when we are denied access to a reserce):
rescue_from CanCan::AccessDenied do |_exception|
redirect_unauthorized_access
end
which created theoretically infinite loop of redirecting to /admin/forbidden, being denied access, redirecting to /admin/forbidden... Which resulted in error:
I decided that plain 404 error page is more clear, so I overwritten said rescue_from CanCan::AccessDenied
This PR works with: spree/spree#11993