Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add a separate permission to access the admin panel #303

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add a separate permission to access the admin panel #303

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8b233b9
update ErrorsController to authorize access to Spree::AdminPanel
mateus-po Jan 9, 2024
a1f7eaa
move admin panel :read access check functionality to base controller
mateus-po Jan 10, 2024
5994fd3
deleted extra empty line
mateus-po Jan 10, 2024
6f99c1e
added access_denied view, handle access denied logic
mateus-po Jan 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions app/controllers/spree/admin/errors_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
module Spree
module Admin
class ErrorsController < BaseController

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

throw _exception
end
end
end
end
Loading