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

Move to CanCanCan for authorization #2023

Merged
merged 29 commits into from Nov 3, 2018

Conversation

Projects
None yet
4 participants
@gravitystorm
Collaborator

gravitystorm commented Oct 10, 2018

Resolves #1626. Builds on and replaces #1904

I've taken #1904, brought it up to date, and resolved a couple of things that I'd noticed and added a few more refactorings, including the first use of the can? in the views.

At this point, do we want to merge what we have already and then refactor the rest of the controllers in subsequent PRs, or should we wait until we're ready with a comprehensive PR that covers all controllers?

gravitystorm and others added some commits Mar 1, 2018

use a controller method to handle cancan denials
This will let controllers override for specific circumstances
Implement the cancan filters for diary entries
Access logic is not _entirely_ exported from the controller,
unfortunately.  For interface reasons, some actions which require admin
have to be listed within the controller's deny_access method.

This is required because, being a default-deny system, cancancan
_cannot_ tell you the reason you were denied access; and so
the "nice" feedback presenting next steps can't be gleaned from
the exception
Update capabilities check to actually reflect the existing logic
The OAuth capabilities are essentially user permissions that have been
granted to the app.  If the user authenticates through a non-oauth
method, they are assumed to have granted all capabilities to the app
separate ability and capability
These are asking fundamentally different questions;

Abilities are asking the application if the user has a role that allows
the user to take a certain action
Capabilities are asking if the user has granted the application to
perform a certain type of action

CanCanCan makes no distinction, however, so the `granted_capabilities`
method is provided as a point that can be checked in rescue methods, so
that one can _attempt_ to continue to provide the more informative error
messages around permission refusals
@tomhughes

I assume (given your question) that this can be used as is and the existing technology continues to work for unconverted methods?

If that is the case then I have no objection in principle to merging this and then proceeding with further refactoring separately.

@@ -0,0 +1,57 @@
# frozen_string_literal: true
class Ability

This comment has been minimized.

@tomhughes

tomhughes Oct 14, 2018

Member

Is app/model the right place for this? It doesn't look like a model and the CanCanCan documentation just refers to is as a "class" so I was expecting it to be in lib but I guess this is where the generator put it?

This comment has been minimized.

@gravitystorm

gravitystorm Oct 17, 2018

Collaborator

Yes, this is where the generator puts it. I'm happy to put it where ever suits - perhaps in config? I'm not sure about lib, I consider that somewhere where stuff goes that (in theory) could be extracted into a gem, but I can see the logic there since we put various classes into lib already.

From https://github.com/CanCanCommunity/cancancan/blob/develop/lib/generators/cancan/ability/USAGE:

The cancan:ability generator creates an Ability class in the models
directory. You can move this file anywhere you want as long as it
is in the load path.

This comment has been minimized.

@tomhughes

tomhughes Oct 17, 2018

Member

It's tricky... I'm not sure rails even creates a lib directory these days, and I know it was removed from the default load path but we put it back in config/application.rb.

This comment has been minimized.

@benreyn

benreyn Nov 1, 2018

Member

Just weighing in, FWIW, Ive seen a folder created for these and seen them put in app/abilities/ before. It would also make splitting the ability file (which are likely to want to do later) much easier.

This comment has been minimized.

@cflipse

cflipse Nov 1, 2018

Contributor

/lib is almost certainly the wrong place, as this isn't external / library code. I could see putting it in config, but it's active code that gets checked, and config isn't the first place I'd look. Ben's thought about app/abilities appeals, if the plan is to deconstruct the large object at some point ...

This comment has been minimized.

@tomhughes

tomhughes Nov 1, 2018

Member

I did do some googling before and there seems to be approximately no consensus on where to put things that aren't one of the "standard" types of class.

I'd always seen lib as being for random bits of non-classifiable code and not just for external things (which wouldn't normally be in the repo at all) and in the early days when lib was on the autoload path I think that was a more common view but then it got removed.

I didn't even know could just create random directories under app to be honest - how does that work? Does rails add every directory there to the autoload path?

This comment has been minimized.

@cflipse

cflipse Nov 1, 2018

Contributor

It does.

there are some additional app directories that are something approaching a standard: app/presenters, app/forms, app/services (especially the last one) but it's pretty much open season for organizing things under the top layer of app. Rails will loop through those directories and add them to autoload at boot time (so, if you add a new app/foo class, you'd need to reboot your dev server, but otherwise, classes underneath are subject to Rails' loading magicks)

Show resolved Hide resolved app/models/ability.rb Outdated
@@ -466,6 +468,23 @@ def better_errors_allow_inline
raise
end
def current_ability
Ability.new(current_user).merge(granted_capability)

This comment has been minimized.

@tomhughes

tomhughes Oct 14, 2018

Member

This has lost the memoization of the default method, which has @current_ability ||= as a prefix - is that deliberate?

This comment has been minimized.

@gravitystorm

gravitystorm Oct 17, 2018

Collaborator

Not deliberate by me. But adding in the memoization causes the tests to fail, and I would need to investigate why.

This comment has been minimized.

@gravitystorm

gravitystorm Oct 17, 2018

Collaborator

The tests fail because in our controller tests, the controller state (e.g. instance variables) is not reset between each "request" - see rails/rails#24566 . Calling get in a controller test isn't really making an http request, it just calls the methods on the controller object which exists for the length of the test.

So for a simple example, site_controller_test#test_welcome calls get twice. The memoized result from the first call means that the second logged-in call is also forbidden. This is, ultimately, because you're not really meant to make multiple requests in a single controller testcase - that should be in integration or system tests. But we do this a lot in our tests.

Show resolved Hide resolved app/controllers/application_controller.rb Outdated
@@ -0,0 +1,21 @@
# frozen_string_literal: true
class Capability

This comment has been minimized.

@tomhughes

tomhughes Oct 14, 2018

Member

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?

This comment has been minimized.

@gravitystorm

gravitystorm Oct 17, 2018

Collaborator

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.

This comment has been minimized.

@cflipse

cflipse Nov 1, 2018

Contributor

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.

This comment has been minimized.

@tomhughes

tomhughes Nov 1, 2018

Member

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.

gravitystorm added some commits Oct 17, 2018

Rework the default denied access handler to give different responses …
…to tokens, logged in users and other users
Rework capabilities to avoid assumptions about missing tokens
The logic about missing tokens implying logged in users (and that
all logged in users have access to any method protected by a token
capability) is correct. However, I believe it is both confusing and
brittle, and leaves a security-related door ajar for future foot-gun
incidents.

Instead, apply Abilities as normal, and keep the Capabilities
involvement only for situations where a token is provided. This
reduces the cognitive burden when considering Abilities in isolation.
@gravitystorm

This comment has been minimized.

Collaborator

gravitystorm commented Oct 24, 2018

I've pushed a few more changes, in particular a slight reworking of the token handling which I think makes the behaviour more obvious.

I also tried to convert another couple of controllers, but realised that there's a few edge cases in each case which deserve their own PR, so I don't think I'll expand the scope of this any further yet!

@tomhughes

This comment has been minimized.

Member

tomhughes commented on 71b21ec Oct 24, 2018

With this change made do we still need the if user guard around the capability initialisation? I think the only reason for that was to support the logic that being logged in and not having a token implied those permissions right?

This comment has been minimized.

Collaborator

gravitystorm replied Oct 24, 2018

I have no idea how the oauth tokens really work though (and reading the plugin source code hasn't yet enlightened me). Are there any circumstances (e.g. bogus tokens) where current_token might be set when current_user is not set?

This comment has been minimized.

Member

tomhughes replied Oct 24, 2018

So that turns out to be quite a complicated question ;-)

So firstly current_token will be set whenever the rack filter saw a request with a valid OAuth signature, but it might only be a request token that hasn't been authorised yet - that could be checked with .is_a?(AccessToken) if we wanted.

Secondly if it is an access token and setup_user_auth has been run then current_user will be set to the token's user by the Authenticator.new(self, [:token]).allow? call - strictly that would probably be the best way to validate the token before using to check capabilities but it is a bit nasty in that it wants a controller and will try and send a response if the token is invalid so we probably can't/don't want to use that.

So in principle there is no security issue so long as we check the token is an access token but on the other hand if setup_user_auth hasn't been called then current_user won't be set and although things might be allowed they might not actually work ;-)

@gravitystorm

This comment has been minimized.

Collaborator

gravitystorm commented Oct 24, 2018

My brain is melting from trying to understand all this, but I'm almost on top of it now. I've used the same check as setup_user_auth, for consistency, and removed the user check from Capabilities. I hope this is a valid approach!

@tomhughes

This comment has been minimized.

Member

tomhughes commented Oct 24, 2018

I think so, and long term it should mean we can get rid of setup_user_auth and having to remember to call it on requests that may need to be authorised and just have the CanCan setup trigger the checks automatically.

Fully achieving that will require also handling basic auth when setting up abilities. Plus we'll have to figure out what to do with the user blocks check...

@gravitystorm

This comment has been minimized.

Collaborator

gravitystorm commented Oct 31, 2018

I think this is ready for final review and/or merging now.

@gravitystorm gravitystorm changed the title from WIP: Move to CanCanCan for authorization to Move to CanCanCan for authorization Oct 31, 2018

@tomhughes tomhughes merged commit 8c269ab into openstreetmap:master Nov 3, 2018

2 of 3 checks passed

continuous-integration/travis-ci/push The Travis CI build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on cancancan at 85.09%
Details

@gravitystorm gravitystorm deleted the gravitystorm:cancancan branch Nov 7, 2018

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