administrate icon indicating copy to clipboard operation
administrate copied to clipboard

Authorization errors should not return 500

Open pablobm opened this issue 2 years ago • 2 comments

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 avatar Apr 25 '23 11:04 pablobm

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

jubilee2 avatar Apr 26 '23 04:04 jubilee2

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.

pablobm avatar Apr 26 '23 14:04 pablobm