varvet / pundit

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

Logging feature #832

Open thisismydesign opened 2 months ago

thisismydesign commented 2 months ago

Please consider

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).

Burgestrand commented 1 month ago

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:

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

thisismydesign commented 1 month ago

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

Burgestrand commented 1 month ago

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.

thisismydesign commented 1 month ago

@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.

Burgestrand commented 1 month ago

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.

thisismydesign commented 1 month ago

Thank you!