Merge remote-tracking branch 'upstream/pull/5429'

app/abilities/api_ability.rb

@@ -3,12 +3,16 @@
 class ApiAbility
   include CanCan::Ability
 
-  def initialize(user)
+  def initialize(token)
     can :read, [:version, :capability, :permission, :map]
 
     if Settings.status != "database_offline"
+      user = User.find(token.resource_owner_id) if token
+
+      can [:read, :feed, :search], Note
+      can :create, Note unless token
+
       can [:read, :download], Changeset
-      can [:read, :create, :feed, :search], Note
       can :read, Tracepoint
       can :read, User
       can :read, Node
@@ -18,22 +22,33 @@ class ApiAbility
       can :read, UserBlock
 
       if user&.active?
-        can [:comment, :close, :reopen], Note
-        can [:read, :create, :update, :destroy], Trace
-        can [:details, :gpx_files], User
-        can [:read, :update, :update_all, :destroy], UserPreference
+        can [:create, :comment, :close, :reopen], Note if scope?(token, :write_notes)
+        can [:create, :destroy], NoteSubscription if scope?(token, :write_notes)
+
+        can :read, Trace if scope?(token, :read_gpx)
+        can [:create, :update, :destroy], Trace if scope?(token, :write_gpx)
+
+        can :details, User if scope?(token, :read_prefs)
+        can :gpx_files, User if scope?(token, :read_gpx)
+
+        can :read, UserPreference if scope?(token, :read_prefs)
+        can [:update, :update_all, :destroy], UserPreference if scope?(token, :write_prefs)
+
+        can [:inbox, :outbox, :read, :update, :destroy], Message if scope?(token, :consume_messages)
+        can :create, Message if scope?(token, :send_messages)
 
         if user.terms_agreed?
-          can [:create, :update, :upload, :close, :subscribe, :unsubscribe], Changeset
-          can :create, ChangesetComment
-          can [:create, :update, :delete], [Node, Way, Relation]
+          can [:create, :update, :upload, :close, :subscribe, :unsubscribe], Changeset if scope?(token, :write_api)
+          can :create, ChangesetComment if scope?(token, :write_api)
+          can [:create, :update, :delete], [Node, Way, Relation] if scope?(token, :write_api)
         end
 
         if user.moderator?
-          can [:destroy, :restore], ChangesetComment
-          can :destroy, Note
+          can [:destroy, :restore], ChangesetComment if scope?(token, :write_api)
 
-          can :redact, [OldNode, OldWay, OldRelation] if user.terms_agreed?
+          can :destroy, Note if scope?(token, :write_notes)
+
+          can :redact, [OldNode, OldWay, OldRelation] if user&.terms_agreed? && scope?(token, :write_redactions)
         end
       end
     end
@@ -65,4 +80,10 @@ class ApiAbility
     # See the wiki for details:
     # https://github.com/CanCanCommunity/cancancan/wiki/Defining-Abilities
   end
+
+  private
+
+  def scope?(token, scope)
+    token&.includes_scope?(scope)
+  end
 end

app/abilities/api_capability.rb

@@ -1,42 +0,0 @@
-# frozen_string_literal: true
-
-class ApiCapability
-  include CanCan::Ability
-
-  def initialize(token)
-    if Settings.status != "database_offline"
-      user = User.find(token.resource_owner_id)
-
-      if user&.active?
-        can [:create, :comment, :close, :reopen], Note if scope?(token, :write_notes)
-        can [:create, :destroy], NoteSubscription if scope?(token, :write_notes)
-        can [:read, :data], Trace if scope?(token, :read_gpx)
-        can [:create, :update, :destroy], Trace if scope?(token, :write_gpx)
-        can [:details], User if scope?(token, :read_prefs)
-        can [:gpx_files], User if scope?(token, :read_gpx)
-        can :read, UserPreference if scope?(token, :read_prefs)
-        can [:update, :update_all, :destroy], UserPreference if scope?(token, :write_prefs)
-        can [:inbox, :outbox, :read, :update, :destroy], Message if scope?(token, :consume_messages)
-        can [:create], Message if scope?(token, :send_messages)
-
-        if user.terms_agreed?
-          can [:create, :update, :upload, :close, :subscribe, :unsubscribe], Changeset if scope?(token, :write_api)
-          can :create, ChangesetComment if scope?(token, :write_api)
-          can [:create, :update, :delete], [Node, Way, Relation] if scope?(token, :write_api)
-        end
-
-        if user.moderator?
-          can [:destroy, :restore], ChangesetComment if scope?(token, :write_api)
-          can :destroy, Note if scope?(token, :write_notes)
-          can :redact, [OldNode, OldWay, OldRelation] if user&.terms_agreed? && scope?(token, :write_redactions)
-        end
-      end
-    end
-  end
-
-  private
-
-  def scope?(token, scope)
-    token&.includes_scope?(scope)
-  end
-end

app/controllers/api_controller.rb

@@ -65,9 +65,9 @@ class ApiController < ApplicationController
   def current_ability
     # Use capabilities from the oauth token if it exists and is a valid access token
     if doorkeeper_token&.accessible?
-      ApiAbility.new(nil).merge(ApiCapability.new(doorkeeper_token))
+      ApiAbility.new(doorkeeper_token)
     else
-      ApiAbility.new(current_user)
+      ApiAbility.new(nil)
     end
   end
 

test/abilities/api_abilities_test.rb

@@ -21,7 +21,8 @@ end
 
 class UserApiAbilityTest < ApiAbilityTest
   test "Note permissions" do
-    ability = ApiAbility.new create(:user)
+    token = create(:oauth_access_token, :scopes => %w[write_notes])
+    ability = ApiAbility.new token
 
     [:index, :create, :comment, :feed, :show, :search, :close, :reopen].each do |action|
       assert ability.can?(action, Note), "should be able to #{action} Notes"
@@ -35,7 +36,8 @@ end
 
 class ModeratorApiAbilityTest < ApiAbilityTest
   test "Note permissions" do
-    ability = ApiAbility.new create(:moderator_user)
+    token = create(:oauth_access_token, :scopes => %w[write_notes], :resource_owner_id => create(:moderator_user).id)
+    ability = ApiAbility.new token
 
     [:index, :create, :comment, :feed, :show, :search, :close, :reopen, :destroy].each do |action|
       assert ability.can?(action, Note), "should be able to #{action} Notes"

test/abilities/api_capability_test.rb

@@ -5,41 +5,41 @@ require "test_helper"
 class ChangesetCommentApiCapabilityTest < ActiveSupport::TestCase
   test "as a normal user with permissionless token" do
     token = create(:oauth_access_token)
-    capability = ApiCapability.new token
+    ability = ApiAbility.new token
 
     [:create, :destroy, :restore].each do |action|
-      assert capability.cannot? action, ChangesetComment
+      assert ability.cannot? action, ChangesetComment
     end
   end
 
   test "as a normal user with write_api token" do
     token = create(:oauth_access_token, :scopes => %w[write_api])
-    capability = ApiCapability.new token
+    ability = ApiAbility.new token
 
     [:destroy, :restore].each do |action|
-      assert capability.cannot? action, ChangesetComment
+      assert ability.cannot? action, ChangesetComment
     end
 
     [:create].each do |action|
-      assert capability.can? action, ChangesetComment
+      assert ability.can? action, ChangesetComment
     end
   end
 
   test "as a moderator with permissionless token" do
     token = create(:oauth_access_token, :resource_owner_id => create(:moderator_user).id)
-    capability = ApiCapability.new token
+    ability = ApiAbility.new token
 
     [:create, :destroy, :restore].each do |action|
-      assert capability.cannot? action, ChangesetComment
+      assert ability.cannot? action, ChangesetComment
     end
   end
 
   test "as a moderator with write_api token" do
     token = create(:oauth_access_token, :resource_owner_id => create(:moderator_user).id, :scopes => %w[write_api])
-    capability = ApiCapability.new token
+    ability = ApiAbility.new token
 
     [:create, :destroy, :restore].each do |action|
-      assert capability.can? action, ChangesetComment
+      assert ability.can? action, ChangesetComment
     end
   end
 end
@@ -47,41 +47,41 @@ end
 class NoteApiCapabilityTest < ActiveSupport::TestCase
   test "as a normal user with permissionless token" do
     token = create(:oauth_access_token)
-    capability = ApiCapability.new token
+    ability = ApiAbility.new token
 
     [:create, :comment, :close, :reopen, :destroy].each do |action|
-      assert capability.cannot? action, Note
+      assert ability.cannot? action, Note
     end
   end
 
   test "as a normal user with write_notes token" do
     token = create(:oauth_access_token, :scopes => %w[write_notes])
-    capability = ApiCapability.new token
+    ability = ApiAbility.new token
 
     [:destroy].each do |action|
-      assert capability.cannot? action, Note
+      assert ability.cannot? action, Note
     end
 
     [:create, :comment, :close, :reopen].each do |action|
-      assert capability.can? action, Note
+      assert ability.can? action, Note
     end
   end
 
   test "as a moderator with permissionless token" do
     token = create(:oauth_access_token, :resource_owner_id => create(:moderator_user).id)
-    capability = ApiCapability.new token
+    ability = ApiAbility.new token
 
     [:destroy].each do |action|
-      assert capability.cannot? action, Note
+      assert ability.cannot? action, Note
     end
   end
 
   test "as a moderator with write_notes token" do
     token = create(:oauth_access_token, :resource_owner_id => create(:moderator_user).id, :scopes => %w[write_notes])
-    capability = ApiCapability.new token
+    ability = ApiAbility.new token
 
     [:destroy].each do |action|
-      assert capability.can? action, Note
+      assert ability.can? action, Note
     end
   end
 end
@@ -90,32 +90,32 @@ class UserApiCapabilityTest < ActiveSupport::TestCase
   test "user preferences" do
     # A user with empty tokens
     token = create(:oauth_access_token)
-    capability = ApiCapability.new token
+    ability = ApiAbility.new token
 
     [:index, :show, :update_all, :update, :destroy].each do |act|
-      assert capability.cannot? act, UserPreference
+      assert ability.cannot? act, UserPreference
     end
 
     token = create(:oauth_access_token, :scopes => %w[read_prefs])
-    capability = ApiCapability.new token
+    ability = ApiAbility.new token
 
     [:update_all, :update, :destroy].each do |act|
-      assert capability.cannot? act, UserPreference
+      assert ability.cannot? act, UserPreference
     end
 
     [:index, :show].each do |act|
-      assert capability.can? act, UserPreference
+      assert ability.can? act, UserPreference
     end
 
     token = create(:oauth_access_token, :scopes => %w[write_prefs])
-    capability = ApiCapability.new token
+    ability = ApiAbility.new token
 
     [:index, :show].each do |act|
-      assert capability.cannot? act, UserPreference
+      assert ability.cannot? act, UserPreference
     end
 
     [:update_all, :update, :destroy].each do |act|
-      assert capability.can? act, UserPreference
+      assert ability.can? act, UserPreference
     end
   end
 end