Skip to content

Conversation

@mateus-po
Copy link
Contributor

@mateus-po mateus-po commented Jan 9, 2024

This PR works with: spree/spree#11993

skip_before_action :authorize_admin

def forbidden
authorize! :read, ::Spree::AdminPanel
Copy link
Member

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

Copy link
Member

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|
Copy link
Member

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?

Copy link
Contributor Author

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:
Screenshot 2024-01-10 at 10 20 17

I decided that plain 404 error page is more clear, so I overwritten said rescue_from CanCan::AccessDenied

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants