From 91fc65a2e3ad47414344a6d5dc03cb5fb2a08ce1 Mon Sep 17 00:00:00 2001 From: Chris Flipse Date: Sun, 17 Jun 2018 13:15:49 -0400 Subject: [PATCH] separate ability and capability These are asking fundamentally different questions; Abilities are asking the application if the user has a role that allows the user to take a certain action Capabilities are asking if the user has granted the application to perform a certain type of action CanCanCan makes no distinction, however, so the `granted_capabilities` method is provided as a point that can be checked in rescue methods, so that one can _attempt_ to continue to provide the more informative error messages around permission refusals --- app/controllers/application_controller.rb | 6 ++- app/models/ability.rb | 11 +---- app/models/capability.rb | 19 ++++++++ test/models/abilities_test.rb | 55 +++-------------------- test/models/capability_test.rb | 51 +++++++++++++++++++++ 5 files changed, 81 insertions(+), 61 deletions(-) create mode 100644 app/models/capability.rb create mode 100644 test/models/capability_test.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index b6a2467a4..2be8c1637 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -471,7 +471,11 @@ class ApplicationController < ActionController::Base end def current_ability - Ability.new(current_user, current_token) + Ability.new(current_user).merge(granted_capabily) + end + + def granted_capabily + Capability.new(current_user, current_token) end def deny_access(exception) diff --git a/app/models/ability.rb b/app/models/ability.rb index d33430fb4..2f86ea412 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -3,7 +3,7 @@ class Ability include CanCan::Ability - def initialize(user, token) + def initialize(user) can :index, :site can [:permalink, :edit, :help, :fixthemap, :offline, :export, :about, :preview, :copyright, :key, :id, :welcome], :site @@ -17,9 +17,6 @@ class Ability can [:create, :edit, :comment, :subscribe, :unsubscribe], DiaryEntry - can [:read, :read_one], UserPreference if has_capability?(token, :allow_read_prefs) - can [:update, :update_one, :delete_one], UserPreference if has_capability?(token, :allow_write_prefs) - if user.administrator? can [:hide, :hidecomment], [DiaryEntry, DiaryComment] end @@ -51,10 +48,4 @@ class Ability # See the wiki for details: # https://github.com/CanCanCommunity/cancancan/wiki/Defining-Abilities end - - # If a user provides no tokens, they've authenticated via a non-oauth method - # and permission to access to all capabilities is assumed. - def has_capability?(token, cap) - token.nil? || token.read_attribute(cap) - end end diff --git a/app/models/capability.rb b/app/models/capability.rb new file mode 100644 index 000000000..174687503 --- /dev/null +++ b/app/models/capability.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class Capability + include CanCan::Ability + + def initialize(user, token) + if user + can [:read, :read_one], UserPreference if has_capability?(token, :allow_read_prefs) + can [:update, :update_one, :delete_one], UserPreference if has_capability?(token, :allow_write_prefs) + + end + end + + # If a user provides no tokens, they've authenticated via a non-oauth method + # and permission to access to all capabilities is assumed. + def has_capability?(token, cap) + token.nil? || token.read_attribute(cap) + end +end diff --git a/test/models/abilities_test.rb b/test/models/abilities_test.rb index 298e8299b..77c14f40f 100644 --- a/test/models/abilities_test.rb +++ b/test/models/abilities_test.rb @@ -3,21 +3,12 @@ require "test_helper" class AbilityTest < ActiveSupport::TestCase - - def tokens(*toks) - AccessToken.new do |token| - toks.each do |t| - token.public_send("#{t}=", true) - end - end - end - end class GuestAbilityTest < AbilityTest test "geocoder permission for a guest" do - ability = Ability.new nil, tokens + ability = Ability.new nil [:search, :search_latlon, :search_ca_postcode, :search_osm_nominatim, :search_geonames, :search_osm_nominatim_reverse, :search_geonames_reverse].each do |action| @@ -26,7 +17,7 @@ class GuestAbilityTest < AbilityTest end test "diary permissions for a guest" do - ability = Ability.new nil, tokens + ability = Ability.new nil [:list, :rss, :view, :comments].each do |action| assert ability.can?(action, DiaryEntry), "should be able to #{action} DiaryEntries" end @@ -42,7 +33,7 @@ end class UserAbilityTest < AbilityTest test "Diary permissions" do - ability = Ability.new create(:user), tokens + ability = Ability.new create(:user) [:list, :rss, :view, :comments, :create, :edit, :comment, :subscribe, :unsubscribe].each do |action| assert ability.can?(action, DiaryEntry), "should be able to #{action} DiaryEntries" @@ -53,48 +44,12 @@ class UserAbilityTest < AbilityTest assert ability.cannot?(action, DiaryComment), "should be able to #{action} DiaryEntries" end end - - test "user preferences" do - user = create(:user) - - # a user with no tokens - ability = Ability.new create(:user), nil - [:read, :read_one, :update, :update_one, :delete_one].each do |act| - assert ability.can? act, UserPreference - end - - # A user with empty tokens - ability = Ability.new create(:user), tokens - - [:read, :read_one, :update, :update_one, :delete_one].each do |act| - assert ability.cannot? act, UserPreference - end - - ability = Ability.new user, tokens(:allow_read_prefs) - - [:update, :update_one, :delete_one].each do |act| - assert ability.cannot? act, UserPreference - end - - [:read, :read_one].each do |act| - assert ability.can? act, UserPreference - end - - ability = Ability.new user, tokens(:allow_write_prefs) - [:read, :read_one].each do |act| - assert ability.cannot? act, UserPreference - end - - [:update, :update_one, :delete_one].each do |act| - assert ability.can? act, UserPreference - end - end end class AdministratorAbilityTest < AbilityTest test "Diary for an administrator" do - ability = Ability.new create(:administrator_user), tokens + ability = Ability.new create(:administrator_user) [:list, :rss, :view, :comments, :create, :edit, :comment, :subscribe, :unsubscribe, :hide, :hidecomment].each do |action| assert ability.can?(action, DiaryEntry), "should be able to #{action} DiaryEntries" end @@ -105,7 +60,7 @@ class AdministratorAbilityTest < AbilityTest end test "administrator does not auto-grant user preferences" do - ability = Ability.new create(:administrator_user), tokens + ability = Ability.new create(:administrator_user) [:read, :read_one, :update, :update_one, :delete_one].each do |act| assert ability.cannot? act, UserPreference diff --git a/test/models/capability_test.rb b/test/models/capability_test.rb new file mode 100644 index 000000000..2d5d65002 --- /dev/null +++ b/test/models/capability_test.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require "test_helper" + +class CapabilityTest < ActiveSupport::TestCase + def tokens(*toks) + AccessToken.new do |token| + toks.each do |t| + token.public_send("#{t}=", true) + end + end + end +end + +class UserCapabilityTest < CapabilityTest + test "user preferences" do + user = create(:user) + + # 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 + end + + # A user with empty tokens + capability = Capability.new create(:user), tokens + + [:read, :read_one, :update, :update_one, :delete_one].each do |act| + assert capability.cannot? act, UserPreference + end + + capability = Capability.new user, tokens(:allow_read_prefs) + + [:update, :update_one, :delete_one].each do |act| + assert capability.cannot? act, UserPreference + end + + [:read, :read_one].each do |act| + assert capability.can? act, UserPreference + end + + capability = Capability.new user, tokens(:allow_write_prefs) + [:read, :read_one].each do |act| + assert capability.cannot? act, UserPreference + end + + [:update, :update_one, :delete_one].each do |act| + assert capability.can? act, UserPreference + end + end +end -- 2.43.2