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 17ed6715cae2732ae73cee044c7094322b0d0028..f55f19e4ea7e2d918e81ee4a95f66990dea9b530 100644 (file)
--- 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 :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
 
       if user.moderator?
         can [:index, :show, :resolve, :ignore, :reopen], Issue

diff --git a/app/models/capability.rb b/app/models/capability.rb
index db2d7171170745acda1dc9898411473b8808ef53..72c5545cb4ec5a9b04d5e0c4b5d453765fa754cc 100644 (file)
--- 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)
     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
 
     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)
   def capability?(token, cap)

-    token.nil? || token.read_attribute(cap)
+    token&.read_attribute(cap)

   end
 end
   end
 end

diff --git a/test/models/abilities_test.rb b/test/models/abilities_test.rb
index be659af4a45c3baaad3a054f2bfeee02669e13b3..2bae4e88cb156aa2df05cdbf2e2c9bca02912fa2 100644 (file)
--- 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
       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
 end

diff --git a/test/models/capability_test.rb b/test/models/capability_test.rb
index 2d5d6500254bdc57031881d5768daa984074986b..d08d182c2bd90353e225dbf37a19fd2044cd4028 100644 (file)
--- 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|
     # 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
     end
 
     # A user with empty tokens