varvet / pundit

Minimal authorization through OO design and pure Ruby classes
MIT License
8.24k stars 627 forks source link

Add callbacks for logging pundit scope resolutions and authorizations. #687

Open bradgessler opened 3 years ago

bradgessler commented 3 years ago

I found it useful in my development environment rails logs to log authorization and scoping actions from the controller. In my controller code, I have the following methods:

    def after_pundit_authorize_resolution(query:, record:, policy:, user:)
      Rails.logger.info [:pundit_authorize, query, record, policy, user]
    end

    def after_pundit_policy_scope_resolution(policy_scope:, user:, scope:)
      Rails.logger.info [:pundit_policy_scope, policy_scope, user, scope]
    end

Which replaces the no-op callbacks from within this commit. This hook would also be useful if the application requires audit logging in a production environment.

Why don't I just log it from the authorization PORO? Great question: I could add a method that logs the objects that the policy is initialized with, but I then can't log the action that is subsequently called on the policy (known as the query from the pundit code) because its a basic policy.public_send(query).

There's a lot I don't like about how this code is currently structured, so I'm posting the PR here for feedback before moving this forward on approaches.

Keep it as-is: log via callbacks

I don't particularly like this approach because it pollutes the controllers with more methods that don't seem necessary. It does work and requires minimal re-architecture to the internal instance variables.

Replace boolean authorized variables with an Authorization object.

Within the Pundit code, there's various booleans set that tell Pundit if an authorization happened (or not). It looks like this:

  def authorize(record, query = nil, policy_class: nil)
    query ||= "#{action_name}?"

    @_pundit_policy_authorized = true

I could replace the boolean values with an object that stores more information on the result of the authorization:

  def authorize(record, query = nil, policy_class: nil)
    query ||= "#{action_name}?"
    policy = policy_class ? policy_class.new(pundit_user, record) : policy(record)
    @_pundit_policy_authorization = Authorization.new(query, policy)
    @_pundit_policy_authorization.authorize
    # ...

and some of the callback checks would be rewritten as:

  # @return [Boolean] whether authorization has been performed, i.e. whether
  #                   one {#authorize} or {#skip_authorization} has been called
  def pundit_policy_authorized?
    @_pundit_policy_authorization.authorized?
  end

The authorization object would store the response of whether or not the action is authorized:

class Authorization
  attr_reader :policy, :query

  def initialize(policy, query)
    @policy = policy
    @query = query
  end

  def authorize
    @authorized = true 
    @granted ||= policy.public_send(query)
    raise NotAuthorizedError, query: query, record: policy.record, policy: policy if denied?
  end

  def authorized?
    @authorized
  end

  def granted?
    @granted
  end

  def denied?
    !granted?
  end
end

This object could be passed around to a development logger or production audit logger and could then be checked by the callbacks that want to verify authorization happened.

Move the responsibility of authorization logging & callbacks into the policy itself

Pundit could be rigged up to call a method on the policy and scope object itself like before_authorization. That would look like this in the controller:

    if policy.respond_to? :before_authorize
      policy.before_authorize query: query
    end

And the policy would then optionally implement the following method:

class ApplicationPolicy
  def before_authorize(query:)
    Rails.logger.info [:pundit_authorize, query, record, policy, user]
  end
end

The Scope object would have a similar method for logging as well.


The big question is where does the responsibility of logging for authorization actions go? Ideally it can be moved to one place with a single responsibility so the users of Pundit can override the behavior and customize authorization logging.

ioquatix commented 2 years ago

I really like this idea, but there are a couple of things I'd consider:

We took a different approach to validating and tracking authorisations by specifically logging permissions checks, because it's possible more than one policy would be checked per request.

I think for me, the interface you defined here only really works at the controller level and only considers one "authorization" (the last one performed). But this won't be the full picture in a more complex environment. Can we address that concern some how?