[openstreetmap/openstreetmap-website] Initial cut at authorization patterns (#1904)

[Andy Allan]

gravitystorm commented on this pull request.



> +  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
+  end
+
+  private
+
+  # If a user provides no tokens, they've authenticated via a non-oauth method
+  # and permission to access to all capabilities is assumed.
+  def capability?(token, cap)
+    token.nil? || token.read_attribute(cap)

The original `token.nil?` check was here when you were combining both the Ability and Capability stuff, so for normal web stuff it was ok if the token was nil. But it's been refactored since, so that it's only used for token-based capability checks. I worry that this opens up a foot-gun situation, since as far as I can tell there's no reason for calling `capability?` without a token -  what do you think?

-- 
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/1904#pullrequestreview-161073475
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20181003/bc7fa092/attachment.html>