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.

Sign up for GitHub

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

Ensure the all_public_actions method works for Grape APIs #23

Merged
merged 2 commits into from
Aug 11, 2020
Merged

Ensure the all_public_actions method works for Grape APIs #23

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f8e430b
ensure the all_public_actions method works for Grape APIs
Aug 9, 2020
14983ee
Bump declarative_authorization to 0.12.1.tim1
Aug 9, 2020
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
10 changes: 8 additions & 2 deletions lib/declarative_authorization/test/helpers.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,14 @@ def this_is_an_abstract_controller_so_it_needs_no_access_tests
alias :access_tests_not_required :this_is_an_abstract_controller_so_it_needs_no_access_tests

def all_public_actions
actions = controller_class.public_instance_methods(false)
actions += controller_class.superclass.public_instance_methods(false)
actions = []
Copy link
Member

Choose a reason for hiding this comment

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

Could you update the README as well?
Currently it says:
Support for Rails 4 and 5
Instead, I think it should follow the Appraisals:
https://github.com/appfolio/ae_declarative_authorization/blob/master/Appraisals#L1-L2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree it should be updated, but that seems unrelated to this change.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough.

if defined?(Grape) && [Grape::API, Grape::API::Instance].any? { |base| controller_class < base }
actions += controller_class.routes.map { |api| "#{api.request_method} #{api.origin}" }
Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand how this works but I guess it adds a string that is somehow parsed somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In rails controllers, actions are uniquely identified by their method name (eg index, show, etc). In Grape APIs the endpoints are not generated in the same way, so we instead need to use a combination of the HTTP Verb and Path.

This means authorization rules for APIs look like this:

role :tenant do
  allowed to: ['GET /occupancies/:id'], when: [:read, :write_without_delete, :write]
end

The string GET /occupancies/:id is what is being checked here.

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, that string. Yeah makes sense.

else
actions += controller_class.public_instance_methods(false)
actions += controller_class.superclass.public_instance_methods(false)
end

actions.reject! do |method|
method =~ /^_/ ||
method =~ /^rescue_action/ ||
Expand Down