Skip to content
This repository has been archived by the owner on Nov 19, 2019. It is now read-only.

authority_success logging #119

Merged
merged 5 commits into from
Feb 7, 2017
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/authority/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ class Configuration

# Has default settings, which can be overridden in the initializer.

attr_accessor :abilities, :controller_action_map, :user_method, :security_violation_handler, :logger
attr_accessor :abilities, :controller_action_map, :user_method, :security_violation_handler, :logger, :log_success

def initialize

Expand Down
13 changes: 13 additions & 0 deletions lib/authority/controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,10 @@ def authorize_action_for(authority_resource, *options)
end

Authority.enforce(authority_action, authority_resource, authority_user, *options)

# This method is always invoked, but will only log if it's enabled
authority_success(authority_user, authority_action, authority_resource)

self.authorization_performed = true
end

Expand All @@ -144,6 +148,15 @@ def authority_forbidden(error)
render :file => Rails.root.join('public', '403.html'), :status => 403, :layout => false
end

# Log successful authorization attempt if enabled in the authority configuration.
#
# This method can be overloaded inside the application controller, similar to authority_forbidden.
def authority_success(user, action, resource)
if Authority.configuration.log_success
Authority.logger.info("#{user} successfully performed the #{action} action to this resource: #{resource}")
Copy link
Owner

@nathanl nathanl Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of thoughts:

  • We don't actually know that they successfully performed the action, only that they successfully made it through the authorization step. We should log something like "user was authorized to perform the #{action} to the resource #{resource}"
  • Is there a good reason to make this a controller method, or a method the user can override? authority_forbidden renders 403 (a controller responsibility) and some people might want to show 404 or something else (so it probably needs to be overridable). But it seems like Authority.enforce could just say "if the log_attempts option is true, log the attempt."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Fair point. I think the function name makes sense, since we successfully passed through authority.. but the default message should be changed. Will fix that.

  2. For my personal need I have to overwrite it because I need to capture specific information in the request such as path details and controller methods. Also figured that if it were a controller method other users would see the need to extend it as well since it's similar to the authority_forbidden action. At first I did have this being logged via Authority.enforce, but overriding that was less clean than just making it a controller method.

Really up to you! It's easy to make the change, but I think it's more restrictive.

Copy link
Owner

@nathanl nathanl Feb 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

One other thought - if authority_success can be overridden to do anything the user wants, the config option log_success isn't necessarily an accurate name. What if we don't even have a config option, and authority_success has a default implementation that does nothing? Then the instructions are "if you want to log successful authorization or do anything else about it, define authority_success however you like."

end
end

private

# The `before_filter` that will be setup to run when the class method
Expand Down
18 changes: 18 additions & 0 deletions spec/authority/controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,24 @@ def self.before_action(*args) ; end

end

describe "authority_success action" do

it "logs an info if enabled" do
Authority.configuration.log_success = true

expect(Authority.logger).to receive(:info)
controller_instance.send(:authority_success, "a", "b", "c")
end

it "does not log anything if disabled" do
Authority.configuration.log_success = false

expect(Authority.logger).not_to receive(:info)
controller_instance.send(:authority_success, "a", "b", "c")
end

end

end

end
Expand Down