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.
This commit is contained in:
parent
a50ad1c895
commit
71b21ec473
4 changed files with 3 additions and 13 deletions
|
@ -13,6 +13,7 @@ class Ability
|
||||||
can :welcome, :site
|
can :welcome, :site
|
||||||
can [:create, :edit, :comment, :subscribe, :unsubscribe], DiaryEntry
|
can [:create, :edit, :comment, :subscribe, :unsubscribe], DiaryEntry
|
||||||
can [:new, :create], Report
|
can [:new, :create], Report
|
||||||
|
can [:read, :read_one, :update, :update_one, :delete_one], UserPreference
|
||||||
|
|
||||||
if user.moderator?
|
if user.moderator?
|
||||||
can [:index, :show, :resolve, :ignore, :reopen], Issue
|
can [:index, :show, :resolve, :ignore, :reopen], Issue
|
||||||
|
|
|
@ -7,15 +7,12 @@ class Capability
|
||||||
if user
|
if user
|
||||||
can [:read, :read_one], UserPreference if capability?(token, :allow_read_prefs)
|
can [:read, :read_one], UserPreference if capability?(token, :allow_read_prefs)
|
||||||
can [:update, :update_one, :delete_one], UserPreference if capability?(token, :allow_write_prefs)
|
can [:update, :update_one, :delete_one], UserPreference if capability?(token, :allow_write_prefs)
|
||||||
|
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
private
|
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
|
||||||
|
|
|
@ -54,12 +54,4 @@ class AdministratorAbilityTest < AbilityTest
|
||||||
assert ability.can?(action, DiaryComment), "should be able to #{action} DiaryComment"
|
assert ability.can?(action, DiaryComment), "should be able to #{action} DiaryComment"
|
||||||
end
|
end
|
||||||
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
|
||||||
|
|
|
@ -19,7 +19,7 @@ class UserCapabilityTest < CapabilityTest
|
||||||
# a user with no tokens
|
# a user with no tokens
|
||||||
capability = Capability.new create(:user), nil
|
capability = Capability.new create(:user), nil
|
||||||
[:read, :read_one, :update, :update_one, :delete_one].each do |act|
|
[:read, :read_one, :update, :update_one, :delete_one].each do |act|
|
||||||
assert capability.can? act, UserPreference
|
assert capability.cannot? act, UserPreference
|
||||||
end
|
end
|
||||||
|
|
||||||
# A user with empty tokens
|
# A user with empty tokens
|
||||||
|
|
Loading…
Add table
Add a link
Reference in a new issue