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.

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, :login, :logout, :new, :create, :save, :confirm, :confirm_resend, :confirm_email, :lost_password, :reset_password, :show, :auth_success, :auth_failure], User
-      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 [: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 [:mine, :new, :create, :edit, :update, :delete], Trace
-        can [:account, :go_public, :make_friend, :remove_friend, :api_details, :api_gpx_files], User
+        can [:account, :go_public, :make_friend, :remove_friend], 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 [: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?

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

app/abilities/api_capability.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-class Capability
+class ApiCapability
   include CanCan::Ability
 
   def initialize(token)

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

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
+    Ability.new(current_user)
-    if Authenticator.new(self, [:token]).allow?
-      Ability.new(nil).merge(Capability.new(current_token))
-    else
-      Ability.new(current_user)
-    end
   end
 
   def deny_access(_exception)

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)
 

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

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