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

Jump to bottom

Move to CanCanCan for authorization #2023

Merged
merged 29 commits into from Nov 3, 2018
Merged

Move to CanCanCan for authorization #2023

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
ffa65d4
Add cancancan and the first ability definitions for site_controller
gravitystorm Mar 1, 2018
2ab3d56
don't check authorization everywhere
cflipse Jun 8, 2018
b16aa11
fix tests for site controller
cflipse Jun 8, 2018
6da3ece
use token in ability checks
cflipse Jun 8, 2018
6b44a19
use a controller method to handle cancan denials
cflipse Jun 9, 2018
5232914
Implement the cancan filters for diary entries
cflipse Jun 9, 2018
ac7c45b
add test helper to set oauth tokens
cflipse Jun 9, 2018
060c686
Use cancancan to authorize user_preference_controller
cflipse Jun 9, 2018
2a44ff5
fix and improve ability coverage to account for tokens
cflipse Jun 10, 2018
464c7f8
Update capabilities check to actually reflect the existing logic
cflipse Jun 10, 2018
4d20a2c
Authorize actions on GeocoderController with CanCanCan Ability
benreyn Jun 10, 2018
91fc65a
separate ability and capability
cflipse Jun 17, 2018
25256a4
Make rubocop happy
cflipse Jun 18, 2018
420a728
Merge branch 'authz' of https://github.com/rubyforgood/openstreetmap-…
gravitystorm Oct 10, 2018
f8f7ab1
Change abilities based on upstream renamings
gravitystorm Oct 10, 2018
fb2c1f6
Refactor site#welcome to use abilities instead of require_user
gravitystorm Oct 10, 2018
901c29a
Fix typo in method name
gravitystorm Oct 10, 2018
dfb9e40
Move issues and reports to authorization system
gravitystorm Oct 10, 2018
8360f27
Refactor to show the Issues link based on the calculated permissions
gravitystorm Oct 10, 2018
b7baa2c
Remove temporary development code
gravitystorm Oct 10, 2018
ce761b3
Combine site permissions declarations
gravitystorm Oct 17, 2018
a50ad1c
Rework the default denied access handler to give different responses …
gravitystorm Oct 24, 2018
71b21ec
Rework capabilities to avoid assumptions about missing tokens
gravitystorm Oct 24, 2018
0888f43
Check the oauth token and then use the capabilities directly
gravitystorm Oct 24, 2018
f11221f
Merge branch 'master' into cancancan
gravitystorm Oct 31, 2018
149c07f
Remove unnecessary token granting from the user_preferences tests
gravitystorm Oct 31, 2018
4161959
Add testing for moderator users and issues
gravitystorm Oct 31, 2018
7a177cb
Fix error messages when users should not be able to do things
gravitystorm Oct 31, 2018
8c269ab
Move abilities to a sepatarate top level directory
tomhughes Nov 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 6 additions & 5 deletions app/controllers/application_controller.rb
View file
Open in desktop
Expand Up @@ -469,11 +469,12 @@ def better_errors_allow_inline
end

def current_ability
Ability.new(current_user).merge(granted_capability)
end

def granted_capability
Capability.new(current_user, current_token)
# Add in capabilities from the oauth token if it exists and is a valid access token
if Authenticator.new(self, [:token]).allow?
Ability.new(current_user).merge(Capability.new(current_token))
else
Ability.new(current_user)
end
end

def deny_access(_exception)
Expand Down
8 changes: 3 additions & 5 deletions app/models/capability.rb
View file
Open in desktop
Expand Up @@ -3,11 +3,9 @@
class Capability
Copy link
Member

Choose a reason for hiding this comment

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

Obviously similar comments apply here as with Ability but this one I think is something of your invention as I don't see it mentioned in the documentation?

I wonder if this is even needed - given we are overriding current_ability could we not pass both the user and token to Ability and have it do everything?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't have any views on this, beyond what @cflipse wrote in the commit message when he split them out.

To be blunt, I don't yet have a good understanding of how all the tokens stuff works, so I'm happy to take direction here. My experience elsewhere is just with straightforward approaches around having a current_user with particular roles, and our token handling here is more complex.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's been a while. IIRC, the capabilities reflect permissions granted to the application -- so, if it's making a request to access your GPS, and you say "yes", then you've granted the app that capability. Another common example is when you OAuth login and the system asks for permission to read your contact lists.

This is, more or less, inverse of the CanCanCan's normal idea of an Ability, which is the app deciding if the user has permission to do something; Capability is the user deciding if the app has permission to do something. (Capability was an inherited name, I suspect that something better could be determined) The end result of the calculation is the same, but separating them helps to keep them from getting too crossed.

Copy link
Member

Choose a reason for hiding this comment

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

The name capability comes from us - it was something we built on top of OAuth 1 to associate a token with a set of things it was allowed to do.

OAuth 2 has something similar built in but calls them scopes where when an application requests a token it indicates what scopes it wants access to.

include CanCan::Ability

def initialize(user, token)
if user
can [:read, :read_one], UserPreference if capability?(token, :allow_read_prefs)
can [:update, :update_one, :delete_one], UserPreference if capability?(token, :allow_write_prefs)
end
def initialize(token)
can [:read, :read_one], UserPreference if capability?(token, :allow_read_prefs)
can [:update, :update_one, :delete_one], UserPreference if capability?(token, :allow_write_prefs)
end

private
Expand Down
10 changes: 4 additions & 6 deletions test/models/capability_test.rb
View file
Open in desktop
Expand Up @@ -14,22 +14,20 @@ def tokens(*toks)

class UserCapabilityTest < CapabilityTest
test "user preferences" do
user = create(:user)

# a user with no tokens
capability = Capability.new create(:user), nil
capability = Capability.new nil
[:read, :read_one, :update, :update_one, :delete_one].each do |act|
assert capability.cannot? act, UserPreference
end

# A user with empty tokens
capability = Capability.new create(:user), tokens
capability = Capability.new tokens

[:read, :read_one, :update, :update_one, :delete_one].each do |act|
assert capability.cannot? act, UserPreference
end

capability = Capability.new user, tokens(:allow_read_prefs)
capability = Capability.new tokens(:allow_read_prefs)

[:update, :update_one, :delete_one].each do |act|
assert capability.cannot? act, UserPreference
Expand All @@ -39,7 +37,7 @@ class UserCapabilityTest < CapabilityTest
assert capability.can? act, UserPreference
end

capability = Capability.new user, tokens(:allow_write_prefs)
capability = Capability.new tokens(:allow_write_prefs)
[:read, :read_one].each do |act|
assert capability.cannot? act, UserPreference
end
Expand Down