From: Andy Allan <git@gravitystorm.co.uk>
Date: Wed, 24 Oct 2018 10:07:00 +0000 (+0200)
Subject: Rework capabilities to avoid assumptions about missing tokens
X-Git-Tag: live~2816^2~6
X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/71b21ec47371c7210dfcb234f7bfa0adf00efad1

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.
---

diff --git a/app/models/ability.rb b/app/models/ability.rb
index 17ed6715c..f55f19e4e 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -13,6 +13,7 @@ class Ability
       can :welcome, :site
       can [:create, :edit, :comment, :subscribe, :unsubscribe], DiaryEntry
       can [:new, :create], Report
+      can [:read, :read_one, :update, :update_one, :delete_one], UserPreference
 
       if user.moderator?
         can [:index, :show, :resolve, :ignore, :reopen], Issue
diff --git a/app/models/capability.rb b/app/models/capability.rb
index db2d71711..72c5545cb 100644
--- a/app/models/capability.rb
+++ b/app/models/capability.rb
@@ -7,15 +7,12 @@ class Capability
     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)
+    token&.read_attribute(cap)
   end
 end
diff --git a/test/models/abilities_test.rb b/test/models/abilities_test.rb
index be659af4a..2bae4e88c 100644
--- a/test/models/abilities_test.rb
+++ b/test/models/abilities_test.rb
@@ -54,12 +54,4 @@ class AdministratorAbilityTest < AbilityTest
       assert ability.can?(action, DiaryComment), "should be able to #{action} DiaryComment"
     end
   end
-
-  test "administrator does not auto-grant user preferences" do
-    ability = Ability.new create(:administrator_user)
-
-    [:read, :read_one, :update, :update_one, :delete_one].each do |act|
-      assert ability.cannot? act, UserPreference
-    end
-  end
 end
diff --git a/test/models/capability_test.rb b/test/models/capability_test.rb
index 2d5d65002..d08d182c2 100644
--- a/test/models/capability_test.rb
+++ b/test/models/capability_test.rb
@@ -19,7 +19,7 @@ class UserCapabilityTest < CapabilityTest
     # a user with no tokens
     capability = Capability.new create(:user), nil
     [:read, :read_one, :update, :update_one, :delete_one].each do |act|
-      assert capability.can? act, UserPreference
+      assert capability.cannot? act, UserPreference
     end
 
     # A user with empty tokens