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

Logging feature #832

Open
thisismydesign opened this issue Sep 26, 2024 · 6 comments
Open

Logging feature #832

thisismydesign opened this issue Sep 26, 2024 · 6 comments

Comments

@thisismydesign
Copy link

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

@Burgestrand
Copy link
Member

Burgestrand commented Oct 8, 2024

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, Split this into two methods? #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.

@thisismydesign
Copy link
Author

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

@Burgestrand
Copy link
Member

Oh, absolutely! You would probably rescue_from and trigger an audit event from there. The error raised has accessors to inspect what happened:

pundit/lib/pundit.rb

Lines 27 to 29 in ec75796

# Error that will be raised when authorization has failed
class NotAuthorizedError < Error
attr_reader :query, :record, :policy

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

@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
Copy link
Member

Burgestrand commented Oct 9, 2024

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 #832 (comment) 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
Copy link
Author

Thank you!

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

No branches or pull requests

2 participants