pundit icon indicating copy to clipboard operation
pundit copied to clipboard

Logging feature

Open thisismydesign opened this issue 1 year ago • 6 comments

Please consider

  • Could this feature break backwards-compatibility? no
  • Could this feature benefit the many who use Pundit? yup
  • Could this feature be useful in most projects that use Pundit? i think so
  • Would this feature require Rails? no
  • Am I open to creating a Pull Request with the necessary changes? not at the moment

Is your feature request related to a problem? Please describe. I would like to have logging of unauthorized access events. This is useful for security, debugging and general usage information.

Describe the solution you'd like Option to enable logging. Possibly options for log level and events (e.g. all authorization events or just unauthorized events).

Describe alternatives you've considered https://github.com/stevehodges/pundit_logger

These are complicated monkey-patches that can break. Also probably does not cover all use cases because it does rescue Pundit::NotAuthorizedError, I think it would not work with integrations that already do that (e.g. ActiveAdmin).

thisismydesign avatar Sep 26 '24 14:09 thisismydesign

While I don't believe this is useful in every application, I can definitely see a case for auditing sometimes. Perhaps it's worth opening up the github wiki to allow more space for documenting common patterns.

Spontaneously, rolling your own auditing would be to override the methods we expose in Pundit::Authorization, adding auditing around it and calling super.

class ApplicationController < ActionController::Base
  include Pundit::Authorization

  private

  def authorize(record, query = nil, policy_class: nil)
    audit(...) { super }
  end

  def skip_authorization = audit(...) { super }

  # ... and so on.
end

There are however a few areas that are tricky to reach:

  • knowwing which policy is being used by authorize, https://github.com/varvet/pundit/issues/746 is related to this.
  • ... similarly for policy
  • Reaching the query methods used directly needs another approach, e.g. policy(booking).show?
  • You would also want to audit the raised authorization error

There might be some room to adjust the implementation to make these easier to audit.

Burgestrand avatar Oct 08 '24 07:10 Burgestrand

Thanks @Burgestrand! I would specifically like to audit access-denied scenarios (i.e. raised authorization errors). Any suggestions for this?

thisismydesign avatar Oct 08 '24 09:10 thisismydesign

Oh, absolutely! You would probably rescue_from and trigger an audit event from there. The error raised has accessors to inspect what happened: https://github.com/varvet/pundit/blob/ec75796fbb6ff2e8c03e888f2c0028e10231810c/lib/pundit.rb#L27-L29

So, to copy the README with some changes:

class ApplicationController < ActionController::Base
  include Pundit::Authorization

  rescue_from Pundit::NotAuthorizedError, with: :user_not_authorized

  private

  def user_not_authorized(error)
    audit(:access_denied, message: error.message, policy: error.policy, record: error.record, query: error.query)
    # ...
  end
end

Again, this won't capture policy(record).show?, but you probably don't want that anyway since you might be using it just to hide/show UI.

Burgestrand avatar Oct 09 '24 07:10 Burgestrand

@Burgestrand Yes but the issue there is using integrations that already rescue this, such as ActiveAdmin. This is why it would be great if pundit could do the logging internally.

thisismydesign avatar Oct 09 '24 08:10 thisismydesign

Specifically ActiveAdmin aren't using authorize at all, they rolled their own Pundit adapter: https://github.com/activeadmin/activeadmin/blob/3-0-stable/lib/active_admin/pundit_adapter.rb

Having a Pundit adapter makes sense, but it also makes an internal audit/logging solution in Pundit unlikely to be sufficient and you would need to roll your own auditing for this case either way.

Another style is administrate which does use authorize, and for that case what I proposed in https://github.com/varvet/pundit/issues/832#issuecomment-2401571308 should be sufficient.

Something I haven't tried and is rails-specific is that using the error reporter to listen for Pundit::NotAuthorizedError and then log would probably also work rather well for this.

Burgestrand avatar Oct 09 '24 08:10 Burgestrand

Thank you!

thisismydesign avatar Oct 09 '24 08:10 thisismydesign