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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bradgessler
Copy link

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

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

  • Are we willing to wear the cost of more object allocations on the critical path?
  • We are fundamentally changing the type of @_pundit_policy_authorization - is that acceptable?

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?

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

Successfully merging this pull request may close these issues.

3 participants