From: Andy Allan Date: Wed, 27 Mar 2019 17:00:57 +0000 (+0100) Subject: Disentangle the api abilities from the web abilities X-Git-Tag: live~2619^2 X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/7b057545c0b2030aad9981bd93699f9e33ad7d5f?hp=-c Disentangle the api abilities from the web abilities This will allow us to rename api actions without causing permissions headaches. The choice of abilities files is made by inheriting from either api_controller or application_controller. Also rename capabilities to api_capabilites, for consistency. --- 7b057545c0b2030aad9981bd93699f9e33ad7d5f diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb index 7e8e921a2..d2864e452 100644 --- a/app/abilities/ability.rb +++ b/app/abilities/ability.rb @@ -6,28 +6,21 @@ class Ability def initialize(user) can [:relation, :relation_history, :way, :way_history, :node, :node_history, :changeset, :note, :new_note, :query], :browse - can :show, :capability - can :index, :change can :search, :direction can [:index, :permalink, :edit, :help, :fixthemap, :offline, :export, :about, :preview, :copyright, :key, :id], :site can [:finish, :embed], :export can [:search, :search_latlon, :search_ca_postcode, :search_osm_nominatim, :search_geonames, :search_osm_nominatim_reverse, :search_geonames_reverse], :geocoder - can :index, :map can [:token, :request_token, :access_token, :test_request], :oauth - can :show, :permission - can [:search_all, :search_nodes, :search_ways, :search_relations], :search - can [:trackpoints], :swf if Settings.status != "database_offline" - can [:index, :feed, :show, :download, :query], Changeset + can [:index, :feed], Changeset can :index, ChangesetComment can [:index, :rss, :show, :comments], DiaryEntry - can [:index, :create, :comment, :feed, :show, :search, :mine], Note + can [:mine], Note can [:index, :show], Redaction can [:index, :show, :data, :georss, :picture, :icon], Trace - can :index, Tracepoint - can [:terms, :api_users, :login, :logout, :new, :create, :save, :confirm, :confirm_resend, :confirm_email, :lost_password, :reset_password, :show, :api_read, :auth_success, :auth_failure], User + can [:terms, :login, :logout, :new, :create, :save, :confirm, :confirm_resend, :confirm_email, :lost_password, :reset_password, :show, :auth_success, :auth_failure], User can [:index, :show, :blocks_on, :blocks_by], UserBlock can [:index, :show], Node can [:index, :show, :full, :ways_for_node], Way @@ -47,31 +40,14 @@ class Ability can [:new, :create, :reply, :show, :inbox, :outbox, :mark, :destroy], Message can [:close, :reopen], Note can [:new, :create], Report - can [:mine, :new, :create, :edit, :update, :delete, :api_create, :api_read, :api_update, :api_delete, :api_data], Trace - can [:account, :go_public, :make_friend, :remove_friend, :api_details, :api_gpx_files], User - can [:read, :read_one, :update, :update_one, :delete_one], UserPreference - - if user.terms_agreed? - can [:create, :update, :upload, :close, :subscribe, :unsubscribe, :expand_bbox], Changeset - can :create, ChangesetComment - can [:create, :update, :delete], Node - can [:create, :update, :delete], Way - can [:create, :update, :delete], Relation - end + can [:mine, :new, :create, :edit, :update, :delete], Trace + can [:account, :go_public, :make_friend, :remove_friend], User if user.moderator? - can [:destroy, :restore], ChangesetComment can [:index, :show, :resolve, :ignore, :reopen], Issue can :create, IssueComment - can :destroy, Note can [:new, :create, :edit, :update, :destroy], Redaction can [:new, :edit, :create, :update, :revoke], UserBlock - - if user.terms_agreed? - can :redact, OldNode - can :redact, OldWay - can :redact, OldRelation - end end if user.administrator? diff --git a/app/abilities/api_ability.rb b/app/abilities/api_ability.rb new file mode 100644 index 000000000..54bc8fb4c --- /dev/null +++ b/app/abilities/api_ability.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +class ApiAbility + include CanCan::Ability + + def initialize(user) + can :show, :capability + can :index, :change + can :index, :map + can :show, :permission + can [:search_all, :search_nodes, :search_ways, :search_relations], :search + can [:trackpoints], :swf + + if Settings.status != "database_offline" + can [:show, :download, :query], Changeset + can [:index, :create, :comment, :feed, :show, :search], Note + can :index, Tracepoint + can [:api_users, :api_read], User + can [:index, :show], Node + can [:index, :show, :full, :ways_for_node], Way + can [:index, :show, :full, :relations_for_node, :relations_for_way, :relations_for_relation], Relation + can [:history, :version], OldNode + can [:history, :version], OldWay + can [:history, :version], OldRelation + end + + if user + can :welcome, :site + can [:revoke, :authorize], :oauth + + if Settings.status != "database_offline" + can [:index, :new, :create, :show, :edit, :update, :destroy], ClientApplication + can [:new, :create, :reply, :show, :inbox, :outbox, :mark, :destroy], Message + can [:close, :reopen], Note + can [:new, :create], Report + can [:api_create, :api_read, :api_update, :api_delete, :api_data], Trace + can [:api_details, :api_gpx_files], User + can [:read, :read_one, :update, :update_one, :delete_one], UserPreference + + if user.terms_agreed? + can [:create, :update, :upload, :close, :subscribe, :unsubscribe, :expand_bbox], Changeset + can :create, ChangesetComment + can [:create, :update, :delete], Node + can [:create, :update, :delete], Way + can [:create, :update, :delete], Relation + end + + if user.moderator? + can [:destroy, :restore], ChangesetComment + can :destroy, Note + + if user.terms_agreed? + can :redact, OldNode + can :redact, OldWay + can :redact, OldRelation + end + end + end + end + + # Define abilities for the passed in user here. For example: + # + # user ||= User.new # guest user (not logged in) + # if user.admin? + # can :manage, :all + # else + # can :read, :all + # end + # + # The first argument to `can` is the action you are giving the user + # permission to do. + # If you pass :manage it will apply to every action. Other common actions + # here are :read, :create, :update and :destroy. + # + # The second argument is the resource the user can perform the action on. + # If you pass :all it will apply to every resource. Otherwise pass a Ruby + # class of the resource. + # + # The third argument is an optional hash of conditions to further filter the + # objects. + # For example, here the user can only update published articles. + # + # can :update, Article, :published => true + # + # See the wiki for details: + # https://github.com/CanCanCommunity/cancancan/wiki/Defining-Abilities + end +end diff --git a/app/abilities/capability.rb b/app/abilities/api_capability.rb similarity index 98% rename from app/abilities/capability.rb rename to app/abilities/api_capability.rb index f4c24e97d..9f59b1f24 100644 --- a/app/abilities/capability.rb +++ b/app/abilities/api_capability.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -class Capability +class ApiCapability include CanCan::Ability def initialize(token) diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index 579af27cf..df7cfe93b 100644 --- a/app/controllers/api_controller.rb +++ b/app/controllers/api_controller.rb @@ -16,6 +16,15 @@ class ApiController < ApplicationController end end + def current_ability + # Use capabilities from the oauth token if it exists and is a valid access token + if Authenticator.new(self, [:token]).allow? + ApiAbility.new(nil).merge(ApiCapability.new(current_token)) + else + ApiAbility.new(current_user) + end + end + def deny_access(_exception) if current_token set_locale diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 3ab09b63d..c880e6add 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -329,12 +329,7 @@ class ApplicationController < ActionController::Base end def current_ability - # Use capabilities from the oauth token if it exists and is a valid access token - if Authenticator.new(self, [:token]).allow? - Ability.new(nil).merge(Capability.new(current_token)) - else - Ability.new(current_user) - end + Ability.new(current_user) end def deny_access(_exception) diff --git a/test/abilities/abilities_test.rb b/test/abilities/abilities_test.rb index b951e23e5..f43b6bf50 100644 --- a/test/abilities/abilities_test.rb +++ b/test/abilities/abilities_test.rb @@ -30,13 +30,9 @@ class GuestAbilityTest < AbilityTest test "note permissions for a guest" do ability = Ability.new nil - [:index, :create, :comment, :feed, :show, :search, :mine].each do |action| + [:mine].each do |action| assert ability.can?(action, Note), "should be able to #{action} Notes" end - - [:close, :reopen, :destroy].each do |action| - assert ability.cannot?(action, Note), "should not be able to #{action} Notes" - end end test "user roles permissions for a guest" do @@ -65,18 +61,6 @@ class UserAbilityTest < AbilityTest assert ability.cannot?(action, Issue), "should not be able to #{action} Issues" end end - - test "Note permissions" do - ability = Ability.new create(:user) - - [:index, :create, :comment, :feed, :show, :search, :mine, :close, :reopen].each do |action| - assert ability.can?(action, Note), "should be able to #{action} Notes" - end - - [:destroy].each do |action| - assert ability.cannot?(action, Note), "should not be able to #{action} Notes" - end - end end class ModeratorAbilityTest < AbilityTest @@ -88,14 +72,6 @@ class ModeratorAbilityTest < AbilityTest end end - test "Note permissions" do - ability = Ability.new create(:moderator_user) - - [:index, :create, :comment, :feed, :show, :search, :mine, :close, :reopen, :destroy].each do |action| - assert ability.can?(action, Note), "should be able to #{action} Notes" - end - end - test "User Roles permissions" do ability = Ability.new create(:moderator_user) diff --git a/test/abilities/api_abilities_test.rb b/test/abilities/api_abilities_test.rb new file mode 100644 index 000000000..7734ce996 --- /dev/null +++ b/test/abilities/api_abilities_test.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require "test_helper" + +class ApiAbilityTest < ActiveSupport::TestCase +end + +class GuestApiAbilityTest < ApiAbilityTest + test "note permissions for a guest" do + ability = ApiAbility.new nil + + [:index, :create, :comment, :feed, :show, :search].each do |action| + assert ability.can?(action, Note), "should be able to #{action} Notes" + end + + [:close, :reopen, :destroy].each do |action| + assert ability.cannot?(action, Note), "should not be able to #{action} Notes" + end + end +end + +class UserApiAbilityTest < ApiAbilityTest + test "Note permissions" do + ability = ApiAbility.new create(:user) + + [:index, :create, :comment, :feed, :show, :search, :close, :reopen].each do |action| + assert ability.can?(action, Note), "should be able to #{action} Notes" + end + + [:destroy].each do |action| + assert ability.cannot?(action, Note), "should not be able to #{action} Notes" + end + end +end + +class ModeratorApiAbilityTest < ApiAbilityTest + test "Note permissions" do + ability = ApiAbility.new create(:moderator_user) + + [:index, :create, :comment, :feed, :show, :search, :close, :reopen, :destroy].each do |action| + assert ability.can?(action, Note), "should be able to #{action} Notes" + end + end +end diff --git a/test/abilities/capability_test.rb b/test/abilities/api_capability_test.rb similarity index 80% rename from test/abilities/capability_test.rb rename to test/abilities/api_capability_test.rb index ed42ef01a..8d0e682f6 100644 --- a/test/abilities/capability_test.rb +++ b/test/abilities/api_capability_test.rb @@ -2,7 +2,7 @@ require "test_helper" -class CapabilityTest < ActiveSupport::TestCase +class ApiCapabilityTest < ActiveSupport::TestCase def tokens(*toks) AccessToken.new do |token| toks.each do |t| @@ -12,10 +12,10 @@ class CapabilityTest < ActiveSupport::TestCase end end -class ChangesetCommentCapabilityTest < CapabilityTest +class ChangesetCommentApiCapabilityTest < ApiCapabilityTest test "as a normal user with permissionless token" do token = create(:access_token) - capability = Capability.new token + capability = ApiCapability.new token [:create, :destroy, :restore].each do |action| assert capability.cannot? action, ChangesetComment @@ -24,7 +24,7 @@ class ChangesetCommentCapabilityTest < CapabilityTest test "as a normal user with allow_write_api token" do token = create(:access_token, :allow_write_api => true) - capability = Capability.new token + capability = ApiCapability.new token [:destroy, :restore].each do |action| assert capability.cannot? action, ChangesetComment @@ -37,7 +37,7 @@ class ChangesetCommentCapabilityTest < CapabilityTest test "as a moderator with permissionless token" do token = create(:access_token, :user => create(:moderator_user)) - capability = Capability.new token + capability = ApiCapability.new token [:create, :destroy, :restore].each do |action| assert capability.cannot? action, ChangesetComment @@ -46,7 +46,7 @@ class ChangesetCommentCapabilityTest < CapabilityTest test "as a moderator with allow_write_api token" do token = create(:access_token, :user => create(:moderator_user), :allow_write_api => true) - capability = Capability.new token + capability = ApiCapability.new token [:create, :destroy, :restore].each do |action| assert capability.can? action, ChangesetComment @@ -54,10 +54,10 @@ class ChangesetCommentCapabilityTest < CapabilityTest end end -class NoteCapabilityTest < CapabilityTest +class NoteApiCapabilityTest < ApiCapabilityTest test "as a normal user with permissionless token" do token = create(:access_token) - capability = Capability.new token + capability = ApiCapability.new token [:create, :comment, :close, :reopen, :destroy].each do |action| assert capability.cannot? action, Note @@ -66,7 +66,7 @@ class NoteCapabilityTest < CapabilityTest test "as a normal user with allow_write_notes token" do token = create(:access_token, :allow_write_notes => true) - capability = Capability.new token + capability = ApiCapability.new token [:destroy].each do |action| assert capability.cannot? action, Note @@ -79,7 +79,7 @@ class NoteCapabilityTest < CapabilityTest test "as a moderator with permissionless token" do token = create(:access_token, :user => create(:moderator_user)) - capability = Capability.new token + capability = ApiCapability.new token [:destroy].each do |action| assert capability.cannot? action, Note @@ -88,7 +88,7 @@ class NoteCapabilityTest < CapabilityTest test "as a moderator with allow_write_notes token" do token = create(:access_token, :user => create(:moderator_user), :allow_write_notes => true) - capability = Capability.new token + capability = ApiCapability.new token [:destroy].each do |action| assert capability.can? action, Note @@ -96,22 +96,22 @@ class NoteCapabilityTest < CapabilityTest end end -class UserCapabilityTest < CapabilityTest +class UserApiCapabilityTest < ApiCapabilityTest test "user preferences" do # a user with no tokens - capability = Capability.new nil + capability = ApiCapability.new nil [:read, :read_one, :update, :update_one, :delete_one].each do |act| assert capability.cannot? act, UserPreference end # A user with empty tokens - capability = Capability.new tokens + capability = ApiCapability.new tokens [:read, :read_one, :update, :update_one, :delete_one].each do |act| assert capability.cannot? act, UserPreference end - capability = Capability.new tokens(:allow_read_prefs) + capability = ApiCapability.new tokens(:allow_read_prefs) [:update, :update_one, :delete_one].each do |act| assert capability.cannot? act, UserPreference @@ -121,7 +121,7 @@ class UserCapabilityTest < CapabilityTest assert capability.can? act, UserPreference end - capability = Capability.new tokens(:allow_write_prefs) + capability = ApiCapability.new tokens(:allow_write_prefs) [:read, :read_one].each do |act| assert capability.cannot? act, UserPreference end