[openstreetmap/openstreetmap-website] WIP: Move to CanCanCan for authorization (#2023)
Tom Hughes
notifications at github.com
Sun Oct 14 15:29:43 UTC 2018
tomhughes commented on this pull request.
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
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?
> @@ -0,0 +1,57 @@
+# frozen_string_literal: true
+
+class Ability
+ include CanCan::Ability
+
+ def initialize(user)
+ can :index, :site
+ can [:permalink, :edit, :help, :fixthemap, :offline, :export, :about, :preview, :copyright, :key, :id], :site
Can these not be merged into one?
Is the ability to use a symbol as the last argument to refer to actions in a controller that does correspond to a model documented anywhere? Everything I've seen just refers to using a class name there to authorize access to objects of that class?
> @@ -466,6 +468,23 @@ def better_errors_allow_inline
raise
end
+ def current_ability
+ Ability.new(current_user).merge(granted_capability)
This has lost the memoization of the default method, which has `@current_ability ||=` as a prefix - is that deliberate?
> @@ -466,6 +468,23 @@ def better_errors_allow_inline
raise
end
+ def current_ability
+ Ability.new(current_user).merge(granted_capability)
+ end
+
+ def granted_capability
+ Capability.new(current_user, current_token)
+ end
+
+ def deny_access(_exception)
+ if current_user
This should be `current_token` here I think? Otherwise we wind up talking about OAuth when it wasn't used - the current code is guarded by a token test at least.
If we did that and then had a suitably generic case after it for `current_user` then I think we could get rid of many of the special case overrides for `deny_access` in the other controllers - most of those are pretty much impossible to hit anyway so don't really need to give the precise detail they do now.
> @@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+class Capability
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?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/2023#pullrequestreview-164502055
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20181014/22cfbb80/attachment-0001.html>
More information about the rails-dev
mailing list