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.

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

Authorization errors should not return 500 #2372

Open
pablobm opened this issue Apr 25, 2023 · 2 comments
Open

Authorization errors should not return 500 #2372

pablobm opened this issue Apr 25, 2023 · 2 comments
Labels
bug breakages in functionality that is implemented

Comments

@pablobm
Copy link
Collaborator

pablobm commented Apr 25, 2023

At the moment, when Pundit::NotAuthorizedError or Administrate::NotAuthorizedError is raised within Administrate, the app returns an HTTP 500 status.

To reproduce:

  1. Visit the example app: https://administrate-demo.herokuapp.com/admin/
  2. On the Customers dashboard, click to "Become" any customer.
  3. Visit the payments dashboard at https://administrate-demo.herokuapp.com/admin/payments (it will have disappeared from the navigation as customers are not authorized).

Expected result: an appropriate HTTP 4xx status, probably 403, perhaps with an error message to match.
Actual result: HTTP 500 status and "We're sorry, but something went wrong" message.

Seems like these exceptions should be rescued by Admin::ApplicationController. More questions:

  • Should we also show an error message? What should it look like? Rails doesn't provide (that I know) error pages for errors other than 404, 422 and 500, so we would have to provide something ourselves.
  • If a user integrates their own authorization mechanism, the raised exceptions will be different (eg: they integrate with cancancan and raise CanCan::Error). Should we provide a way to tell Administrate what these exceptions might be, so that they are rescued the same way?
@pablobm pablobm added the bug breakages in functionality that is implemented label Apr 25, 2023
@jubilee2
Copy link
Contributor

One way to handle authorization errors in Rails is to use the rescue_from method within the admin/ApplicationController. This method allows developers to catch exceptions and define custom actions for them. For example, if a user tries to access a resource that they are not authorized to view, the rescue_from method can redirect them to a different page or show an error message. Developers can also adapt their own authorization package or gem to work with the rescue_from method.

it's what example to work with cancancan and devise

module Admin
  class ApplicationController < Administrate::ApplicationController
    before_action :authenticate_user!
    before_action :set_paper_trail_whodunnit

    def authorized_action?(_resource, _action_name)
      can? _action_name.to_sym, _resource
    end

    rescue_from Administrate::NotAuthorizedError do |_exception|
      redirect_to root_url, alert: 'Not Authorized Error'
    end

    # Override this value to specify the number of elements to display at a time
    # on index pages. Defaults to 20.
    # def records_per_page
    #   params[:per_page] || 20
    # end
  end
end

@pablobm
Copy link
Collaborator Author

pablobm commented Apr 26, 2023

Yeah, we may need something like that. Ideally I was thinking we could have it in Administrate::ApplicationController, so that users don't have to set it up. However if we just say the following:

    rescue_from Administrate::NotAuthorizedError, Pundit::NotAuthorizedError do |_exception|
      show_authorization_error # or something like that
    end

Then that doesn't cover other exceptions that users might be raising (eg: CanCan::Error). Options:

  1. We add to the documentation so that they set their own rescue_from.
  2. Offer some sort of configuration where they can list the errors they may raise, and this is used in the common rescue_from above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug breakages in functionality that is implemented
Projects
None yet
Development

No branches or pull requests

2 participants