Disentangle the api abilities from the web abilities
authorAndy Allan <git@gravitystorm.co.uk>
Wed, 27 Mar 2019 17:00:57 +0000 (18:00 +0100)
committerAndy Allan <git@gravitystorm.co.uk>
Wed, 27 Mar 2019 17:07:29 +0000 (18:07 +0100)
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.

app/abilities/ability.rb
app/abilities/api_ability.rb [new file with mode: 0644]
app/abilities/api_capability.rb [moved from app/abilities/capability.rb with 98% similarity]
app/controllers/api_controller.rb
app/controllers/application_controller.rb
test/abilities/abilities_test.rb
test/abilities/api_abilities_test.rb [new file with mode: 0644]
test/abilities/api_capability_test.rb [moved from test/abilities/capability_test.rb with 80% similarity]

index 7e8e921a29f4d0c35b69c4a2d1a45331d41147cf..d2864e452ab975c06ba98c4d2b6ac221f67ee693 100644 (file)
@@ -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 (file)
index 0000000..54bc8fb
--- /dev/null
@@ -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
similarity index 98%
rename from app/abilities/capability.rb
rename to app/abilities/api_capability.rb
index f4c24e97d7ffd24122888ee1d069294f567c7a4c..9f59b1f2442f3568f67395e629b393563dc8e34b 100644 (file)
@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-class Capability
+class ApiCapability
   include CanCan::Ability
 
   def initialize(token)
index 579af27cf4d8fcac9683cab09feaf640d7df5f64..df7cfe93be97d9ffa704db88d759be75523ffc32 100644 (file)
@@ -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
index 3ab09b63d805bc67899c6e54dabeac86f7d5fa69..c880e6add00d756ec0b0d6c061a76d00bfa4e40c 100644 (file)
@@ -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)
index b951e23e5b68b0b5fcfcb3e4f22a6dd04c0ec09b..f43b6bf50de811b2f5b5fcc66fdca7d82e99b299 100644 (file)
@@ -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 (file)
index 0000000..7734ce9
--- /dev/null
@@ -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
similarity index 80%
rename from test/abilities/capability_test.rb
rename to test/abilities/api_capability_test.rb
index ed42ef01a029937339a0042b24c8d69c2f03bee8..8d0e682f62b4a3ac91b5b875e43c8d311f82c483 100644 (file)
@@ -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