[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