From 1632d010d3158cc0765fd4cd20cc46d784a31028 Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Wed, 12 Feb 2025 18:42:27 +0300 Subject: [PATCH 01/11] Rename api changeset comment tests --- .../api/changeset_comments_controller_test.rb | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/test/controllers/api/changeset_comments_controller_test.rb b/test/controllers/api/changeset_comments_controller_test.rb index 21f30714c..cc257eb9d 100644 --- a/test/controllers/api/changeset_comments_controller_test.rb +++ b/test/controllers/api/changeset_comments_controller_test.rb @@ -33,7 +33,7 @@ module Api ## # create comment success - def test_create_comment_success + def test_create user = create(:user) user2 = create(:user) private_user = create(:user, :data_public => false) @@ -100,7 +100,7 @@ module Api ## # create comment fail - def test_create_comment_fail + def test_create_fail # unauthorized post changeset_comment_path(create(:changeset, :closed), :text => "This is a comment") assert_response :unauthorized @@ -134,7 +134,7 @@ module Api ## # create comment rate limit for new users - def test_create_comment_new_user_rate_limit + def test_create_by_new_user_with_rate_limit changeset = create(:changeset, :closed) user = create(:user) @@ -155,7 +155,7 @@ module Api ## # create comment rate limit for experienced users - def test_create_comment_experienced_user_rate_limit + def test_create_by_experienced_user_with_rate_limit changeset = create(:changeset, :closed) user = create(:user) create_list(:changeset_comment, Settings.comments_to_max_changeset_comments, :author_id => user.id, :created_at => Time.now.utc - 1.day) @@ -177,7 +177,7 @@ module Api ## # create comment rate limit for reported users - def test_create_comment_reported_user_rate_limit + def test_create_by_reported_user_with_rate_limit changeset = create(:changeset, :closed) user = create(:user) create(:issue_with_reports, :reportable => user, :reported_user => user) @@ -199,7 +199,7 @@ module Api ## # create comment rate limit for moderator users - def test_create_comment_moderator_user_rate_limit + def test_create_by_moderator_user_with_rate_limit changeset = create(:changeset, :closed) user = create(:moderator_user) @@ -220,7 +220,7 @@ module Api ## # test hide comment fail - def test_destroy_comment_fail + def test_hide_fail # unauthorized comment = create(:changeset_comment) assert comment.visible @@ -246,7 +246,7 @@ module Api ## # test hide comment succes - def test_hide_comment_success + def test_hide comment = create(:changeset_comment) assert comment.visible @@ -259,7 +259,7 @@ module Api ## # test unhide comment fail - def test_restore_comment_fail + def test_unhide_fail # unauthorized comment = create(:changeset_comment, :visible => false) assert_not comment.visible @@ -285,7 +285,7 @@ module Api ## # test unhide comment succes - def test_unhide_comment_success + def test_unhide comment = create(:changeset_comment, :visible => false) assert_not comment.visible From 4073e0fc4784794eefa9922964cba1f807515e63 Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Wed, 12 Feb 2025 18:56:09 +0300 Subject: [PATCH 02/11] Split api changeset comment create fail tests --- .../api/changeset_comments_controller_test.rb | 43 ++++++++++--------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/test/controllers/api/changeset_comments_controller_test.rb b/test/controllers/api/changeset_comments_controller_test.rb index cc257eb9d..b3b3d9c75 100644 --- a/test/controllers/api/changeset_comments_controller_test.rb +++ b/test/controllers/api/changeset_comments_controller_test.rb @@ -98,38 +98,39 @@ module Api ActionMailer::Base.deliveries.clear end - ## - # create comment fail - def test_create_fail - # unauthorized - post changeset_comment_path(create(:changeset, :closed), :text => "This is a comment") - assert_response :unauthorized - - auth_header = bearer_authorization_header - - # bad changeset id + def test_create_by_unauthorized assert_no_difference "ChangesetComment.count" do - post changeset_comment_path(999111, :text => "This is a comment"), :headers => auth_header + post changeset_comment_path(create(:changeset, :closed), :text => "This is a comment") + assert_response :unauthorized end - assert_response :not_found + end - # not closed changeset + def test_create_on_missing_changeset assert_no_difference "ChangesetComment.count" do - post changeset_comment_path(create(:changeset), :text => "This is a comment"), :headers => auth_header + post changeset_comment_path(999111, :text => "This is a comment"), :headers => bearer_authorization_header + assert_response :not_found end - assert_response :conflict + end - # no text + def test_create_on_open_changeset assert_no_difference "ChangesetComment.count" do - post changeset_comment_path(create(:changeset, :closed)), :headers => auth_header + post changeset_comment_path(create(:changeset), :text => "This is a comment"), :headers => bearer_authorization_header + assert_response :conflict end - assert_response :bad_request + end - # empty text + def test_create_without_text assert_no_difference "ChangesetComment.count" do - post changeset_comment_path(create(:changeset, :closed), :text => ""), :headers => auth_header + post changeset_comment_path(create(:changeset, :closed)), :headers => bearer_authorization_header + assert_response :bad_request + end + end + + def test_create_with_empty_text + assert_no_difference "ChangesetComment.count" do + post changeset_comment_path(create(:changeset, :closed), :text => ""), :headers => bearer_authorization_header + assert_response :bad_request end - assert_response :bad_request end ## From c9057188aab64e39635c4230b117bf98ce667307 Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Wed, 12 Feb 2025 19:03:42 +0300 Subject: [PATCH 03/11] Split api changeset comment create scope and terms tests --- .../api/changeset_comments_controller_test.rb | 47 +++++++++---------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/test/controllers/api/changeset_comments_controller_test.rb b/test/controllers/api/changeset_comments_controller_test.rb index b3b3d9c75..c5e53ad6d 100644 --- a/test/controllers/api/changeset_comments_controller_test.rb +++ b/test/controllers/api/changeset_comments_controller_test.rb @@ -133,6 +133,28 @@ module Api end end + def test_create_when_not_agreed_to_terms + user = create(:user, :terms_agreed => nil) + auth_header = bearer_authorization_header user + changeset = create(:changeset, :closed) + + assert_difference "ChangesetComment.count", 0 do + post changeset_comment_path(changeset), :params => { :text => "This is a comment" }, :headers => auth_header + assert_response :forbidden + end + end + + def test_create_with_write_api_scope + user = create(:user) + auth_header = bearer_authorization_header user, :scopes => %w[write_api] + changeset = create(:changeset, :closed) + + assert_difference "ChangesetComment.count", 1 do + post changeset_comment_path(changeset), :params => { :text => "This is a comment" }, :headers => auth_header + assert_response :success + end + end + ## # create comment rate limit for new users def test_create_by_new_user_with_rate_limit @@ -296,30 +318,5 @@ module Api assert_response :success assert comment.reload.visible end - - # This test ensures that token capabilities behave correctly for a method that - # requires the terms to have been agreed. - # (This would be better as an integration or system testcase, since the changeset_comment - # create method is simply a stand-in for any method that requires terms agreement. - # But writing oauth tests is hard, and so it's easier to put in a controller test.) - def test_api_write_and_terms_agreed_via_token - user = create(:user, :terms_agreed => nil) - auth_header = bearer_authorization_header(user, :scopes => %w[write_api]) - changeset = create(:changeset, :closed) - - assert_difference "ChangesetComment.count", 0 do - post changeset_comment_path(changeset), :params => { :text => "This is a comment" }, :headers => auth_header - end - assert_response :forbidden - - # Try again, after agreement with the terms - user.terms_agreed = Time.now.utc - user.save! - - assert_difference "ChangesetComment.count", 1 do - post changeset_comment_path(changeset), :params => { :text => "This is a comment" }, :headers => auth_header - end - assert_response :success - end end end From c0ef1a88ad2e8c92e4e2da9c387da5e171938ccc Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Thu, 13 Feb 2025 03:33:48 +0300 Subject: [PATCH 04/11] Test state of created comment --- test/controllers/api/changeset_comments_controller_test.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/controllers/api/changeset_comments_controller_test.rb b/test/controllers/api/changeset_comments_controller_test.rb index c5e53ad6d..b3dc19325 100644 --- a/test/controllers/api/changeset_comments_controller_test.rb +++ b/test/controllers/api/changeset_comments_controller_test.rb @@ -153,6 +153,12 @@ module Api post changeset_comment_path(changeset), :params => { :text => "This is a comment" }, :headers => auth_header assert_response :success end + + comment = ChangesetComment.last + assert_equal changeset.id, comment.changeset_id + assert_equal user.id, comment.author_id + assert_equal "This is a comment", comment.body + assert comment.visible end ## From c3b0a278d819af9cb68254491e61b0e118f62908 Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Thu, 13 Feb 2025 04:05:56 +0300 Subject: [PATCH 05/11] Split changeset comment subscription emails test --- .../api/changeset_comments_controller_test.rb | 168 +++++++++++------- 1 file changed, 101 insertions(+), 67 deletions(-) diff --git a/test/controllers/api/changeset_comments_controller_test.rb b/test/controllers/api/changeset_comments_controller_test.rb index b3dc19325..4719ce49d 100644 --- a/test/controllers/api/changeset_comments_controller_test.rb +++ b/test/controllers/api/changeset_comments_controller_test.rb @@ -31,73 +31,6 @@ module Api ) end - ## - # create comment success - def test_create - user = create(:user) - user2 = create(:user) - private_user = create(:user, :data_public => false) - suspended_user = create(:user, :suspended) - deleted_user = create(:user, :deleted) - private_user_closed_changeset = create(:changeset, :closed, :user => private_user) - - auth_header = bearer_authorization_header user - - assert_difference "ChangesetComment.count", 1 do - assert_no_difference "ActionMailer::Base.deliveries.size" do - perform_enqueued_jobs do - post changeset_comment_path(private_user_closed_changeset, :text => "This is a comment"), :headers => auth_header - end - end - end - assert_response :success - - changeset = create(:changeset, :closed, :user => private_user) - changeset.subscribers.push(private_user) - changeset.subscribers.push(user) - changeset.subscribers.push(suspended_user) - changeset.subscribers.push(deleted_user) - - assert_difference "ChangesetComment.count", 1 do - assert_difference "ActionMailer::Base.deliveries.size", 1 do - perform_enqueued_jobs do - post changeset_comment_path(changeset, :text => "This is a comment"), :headers => auth_header - end - end - end - assert_response :success - - email = ActionMailer::Base.deliveries.first - assert_equal 1, email.to.length - assert_equal "[OpenStreetMap] #{user.display_name} has commented on one of your changesets", email.subject - assert_equal private_user.email, email.to.first - - ActionMailer::Base.deliveries.clear - - auth_header = bearer_authorization_header user2 - - assert_difference "ChangesetComment.count", 1 do - assert_difference "ActionMailer::Base.deliveries.size", 2 do - perform_enqueued_jobs do - post changeset_comment_path(changeset, :text => "This is a comment"), :headers => auth_header - end - end - end - assert_response :success - - email = ActionMailer::Base.deliveries.find { |e| e.to.first == private_user.email } - assert_not_nil email - assert_equal 1, email.to.length - assert_equal "[OpenStreetMap] #{user2.display_name} has commented on one of your changesets", email.subject - - email = ActionMailer::Base.deliveries.find { |e| e.to.first == user.email } - assert_not_nil email - assert_equal 1, email.to.length - assert_equal "[OpenStreetMap] #{user2.display_name} has commented on a changeset you are interested in", email.subject - - ActionMailer::Base.deliveries.clear - end - def test_create_by_unauthorized assert_no_difference "ChangesetComment.count" do post changeset_comment_path(create(:changeset, :closed), :text => "This is a comment") @@ -161,6 +94,107 @@ module Api assert comment.visible end + def test_create_on_changeset_with_no_subscribers + changeset = create(:changeset, :closed) + auth_header = bearer_authorization_header + + assert_difference "ChangesetComment.count", 1 do + assert_no_difference "ActionMailer::Base.deliveries.size" do + perform_enqueued_jobs do + post changeset_comment_path(changeset, :text => "This is a comment"), :headers => auth_header + assert_response :success + end + end + end + end + + def test_create_on_changeset_with_commenter_subscriber + user = create(:user) + changeset = create(:changeset, :closed, :user => user) + changeset.subscribers << user + auth_header = bearer_authorization_header user + + assert_difference "ChangesetComment.count", 1 do + assert_no_difference "ActionMailer::Base.deliveries.size" do + perform_enqueued_jobs do + post changeset_comment_path(changeset, :text => "This is a comment"), :headers => auth_header + assert_response :success + end + end + end + end + + def test_create_on_changeset_with_invisible_subscribers + changeset = create(:changeset, :closed) + changeset.subscribers << create(:user, :suspended) + changeset.subscribers << create(:user, :deleted) + auth_header = bearer_authorization_header + + assert_difference "ChangesetComment.count", 1 do + assert_no_difference "ActionMailer::Base.deliveries.size" do + perform_enqueued_jobs do + post changeset_comment_path(changeset, :text => "This is a comment"), :headers => auth_header + assert_response :success + end + end + end + end + + def test_create_on_changeset_with_changeset_creator_subscriber + creator_user = create(:user) + changeset = create(:changeset, :closed, :user => creator_user) + changeset.subscribers << creator_user + commenter_user = create(:user) + auth_header = bearer_authorization_header commenter_user + + assert_difference "ChangesetComment.count", 1 do + assert_difference "ActionMailer::Base.deliveries.size", 1 do + perform_enqueued_jobs do + post changeset_comment_path(changeset, :text => "This is a comment"), :headers => auth_header + assert_response :success + end + end + end + + email = ActionMailer::Base.deliveries.first + assert_equal 1, email.to.length + assert_equal "[OpenStreetMap] #{commenter_user.display_name} has commented on one of your changesets", email.subject + assert_equal creator_user.email, email.to.first + + ActionMailer::Base.deliveries.clear + end + + def test_create_on_changeset_with_changeset_creator_and_other_user_subscribers + creator_user = create(:user) + changeset = create(:changeset, :closed, :user => creator_user) + changeset.subscribers << creator_user + other_user = create(:user) + changeset.subscribers << other_user + commenter_user = create(:user) + auth_header = bearer_authorization_header commenter_user + + assert_difference "ChangesetComment.count", 1 do + assert_difference "ActionMailer::Base.deliveries.size", 2 do + perform_enqueued_jobs do + post changeset_comment_path(changeset, :text => "This is a comment"), :headers => auth_header + assert_response :success + end + end + end + + email = ActionMailer::Base.deliveries.find { |e| e.to.first == creator_user.email } + assert_not_nil email + assert_equal 1, email.to.length + assert_equal "[OpenStreetMap] #{commenter_user.display_name} has commented on one of your changesets", email.subject + + email = ActionMailer::Base.deliveries.find { |e| e.to.first == other_user.email } + assert_not_nil email + assert_equal 1, email.to.length + assert_equal "[OpenStreetMap] #{commenter_user.display_name} has commented on a changeset you are interested in", email.subject + + ActionMailer::Base.deliveries.clear + end + ## # create comment rate limit for new users def test_create_by_new_user_with_rate_limit From 97b9d5acdbbde1052cb1f21a9ca79d645b20be60 Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Thu, 13 Feb 2025 02:49:54 +0300 Subject: [PATCH 06/11] Split api changeset comment hide fail tests --- .../api/changeset_comments_controller_test.rb | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/test/controllers/api/changeset_comments_controller_test.rb b/test/controllers/api/changeset_comments_controller_test.rb index 4719ce49d..8a1a286df 100644 --- a/test/controllers/api/changeset_comments_controller_test.rb +++ b/test/controllers/api/changeset_comments_controller_test.rb @@ -281,30 +281,31 @@ module Api end end - ## - # test hide comment fail - def test_hide_fail - # unauthorized + def test_hide_by_unauthorized comment = create(:changeset_comment) - assert comment.visible post changeset_comment_hide_path(comment) + assert_response :unauthorized assert comment.reload.visible + end + def test_hide_by_normal_user + comment = create(:changeset_comment) auth_header = bearer_authorization_header - # not a moderator post changeset_comment_hide_path(comment), :headers => auth_header + assert_response :forbidden assert comment.reload.visible + end + def test_hide_missing_comment auth_header = bearer_authorization_header create(:moderator_user) - # bad comment id post changeset_comment_hide_path(999111), :headers => auth_header + assert_response :not_found - assert comment.reload.visible end ## From bb5bad9115a014730575083a2bbc1be13e3bb49a Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Thu, 13 Feb 2025 03:03:36 +0300 Subject: [PATCH 07/11] Split api changeset comment unhide fail tests --- .../api/changeset_comments_controller_test.rb | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/test/controllers/api/changeset_comments_controller_test.rb b/test/controllers/api/changeset_comments_controller_test.rb index 8a1a286df..91b8ad683 100644 --- a/test/controllers/api/changeset_comments_controller_test.rb +++ b/test/controllers/api/changeset_comments_controller_test.rb @@ -321,30 +321,31 @@ module Api assert_not comment.reload.visible end - ## - # test unhide comment fail - def test_unhide_fail - # unauthorized + def test_unhide_by_unauthorized comment = create(:changeset_comment, :visible => false) - assert_not comment.visible post changeset_comment_unhide_path(comment) + assert_response :unauthorized assert_not comment.reload.visible + end + def test_unhide_by_normal_user + comment = create(:changeset_comment, :visible => false) auth_header = bearer_authorization_header - # not a moderator post changeset_comment_unhide_path(comment), :headers => auth_header + assert_response :forbidden assert_not comment.reload.visible + end + def test_unhide_missing_comment auth_header = bearer_authorization_header create(:moderator_user) - # bad comment id post changeset_comment_unhide_path(999111), :headers => auth_header + assert_response :not_found - assert_not comment.reload.visible end ## From 304e0ef63843ddcb55aab62312ff18be8a5b4703 Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Wed, 12 Feb 2025 17:14:21 +0300 Subject: [PATCH 08/11] Pass user to ApiAbility --- app/abilities/api_ability.rb | 6 +-- app/controllers/api_controller.rb | 5 ++- test/abilities/api_abilities_test.rb | 12 +++--- test/abilities/api_capability_test.rb | 53 ++++++++++++++++----------- 4 files changed, 43 insertions(+), 33 deletions(-) diff --git a/app/abilities/api_ability.rb b/app/abilities/api_ability.rb index e774f6820..3bc82eab2 100644 --- a/app/abilities/api_ability.rb +++ b/app/abilities/api_ability.rb @@ -3,14 +3,12 @@ class ApiAbility include CanCan::Ability - def initialize(token) + def initialize(user, 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 :create, Note unless user can [:read, :download], Changeset can :read, Tracepoint diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index 5faa39165..27f262d00 100644 --- a/app/controllers/api_controller.rb +++ b/app/controllers/api_controller.rb @@ -65,9 +65,10 @@ 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(doorkeeper_token) + user = User.find(doorkeeper_token.resource_owner_id) + ApiAbility.new(user, doorkeeper_token) else - ApiAbility.new(nil) + ApiAbility.new(nil, nil) end end diff --git a/test/abilities/api_abilities_test.rb b/test/abilities/api_abilities_test.rb index 0c97dc6a0..c32300c60 100644 --- a/test/abilities/api_abilities_test.rb +++ b/test/abilities/api_abilities_test.rb @@ -7,7 +7,7 @@ end class GuestApiAbilityTest < ApiAbilityTest test "note permissions for a guest" do - ability = ApiAbility.new nil + ability = ApiAbility.new nil, nil [:index, :create, :feed, :show, :search].each do |action| assert ability.can?(action, Note), "should be able to #{action} Notes" @@ -21,8 +21,9 @@ end class UserApiAbilityTest < ApiAbilityTest test "Note permissions" do - token = create(:oauth_access_token, :scopes => %w[write_notes]) - ability = ApiAbility.new token + user = create(:user) + token = create(:oauth_access_token, :user => user, :scopes => %w[write_notes]) + ability = ApiAbility.new user, token [:index, :create, :comment, :feed, :show, :search, :close, :reopen].each do |action| assert ability.can?(action, Note), "should be able to #{action} Notes" @@ -36,8 +37,9 @@ end class ModeratorApiAbilityTest < ApiAbilityTest test "Note permissions" do - token = create(:oauth_access_token, :scopes => %w[write_notes], :user => create(:moderator_user)) - ability = ApiAbility.new token + user = create(:moderator_user) + token = create(:oauth_access_token, :user => user, :scopes => %w[write_notes]) + ability = ApiAbility.new user, token [:index, :create, :comment, :feed, :show, :search, :close, :reopen, :destroy].each do |action| assert ability.can?(action, Note), "should be able to #{action} Notes" diff --git a/test/abilities/api_capability_test.rb b/test/abilities/api_capability_test.rb index ca679dd71..12bbc4965 100644 --- a/test/abilities/api_capability_test.rb +++ b/test/abilities/api_capability_test.rb @@ -4,8 +4,9 @@ require "test_helper" class ChangesetCommentApiCapabilityTest < ActiveSupport::TestCase test "as a normal user with permissionless token" do - token = create(:oauth_access_token) - ability = ApiAbility.new token + user = create(:user) + token = create(:oauth_access_token, :user => user) + ability = ApiAbility.new user, token [:create, :destroy, :restore].each do |action| assert ability.cannot? action, ChangesetComment @@ -13,8 +14,9 @@ class ChangesetCommentApiCapabilityTest < ActiveSupport::TestCase end test "as a normal user with write_api token" do - token = create(:oauth_access_token, :scopes => %w[write_api]) - ability = ApiAbility.new token + user = create(:user) + token = create(:oauth_access_token, :user => user, :scopes => %w[write_api]) + ability = ApiAbility.new user, token [:destroy, :restore].each do |action| assert ability.cannot? action, ChangesetComment @@ -26,8 +28,9 @@ class ChangesetCommentApiCapabilityTest < ActiveSupport::TestCase end test "as a moderator with permissionless token" do - token = create(:oauth_access_token, :user => create(:moderator_user)) - ability = ApiAbility.new token + user = create(:moderator_user) + token = create(:oauth_access_token, :user => user) + ability = ApiAbility.new user, token [:create, :destroy, :restore].each do |action| assert ability.cannot? action, ChangesetComment @@ -35,8 +38,9 @@ class ChangesetCommentApiCapabilityTest < ActiveSupport::TestCase end test "as a moderator with write_api token" do - token = create(:oauth_access_token, :user => create(:moderator_user), :scopes => %w[write_api]) - ability = ApiAbility.new token + user = create(:moderator_user) + token = create(:oauth_access_token, :user => user, :scopes => %w[write_api]) + ability = ApiAbility.new user, token [:create, :destroy, :restore].each do |action| assert ability.can? action, ChangesetComment @@ -46,8 +50,9 @@ end class NoteApiCapabilityTest < ActiveSupport::TestCase test "as a normal user with permissionless token" do - token = create(:oauth_access_token) - ability = ApiAbility.new token + user = create(:user) + token = create(:oauth_access_token, :user => user) + ability = ApiAbility.new user, token [:create, :comment, :close, :reopen, :destroy].each do |action| assert ability.cannot? action, Note @@ -55,8 +60,9 @@ class NoteApiCapabilityTest < ActiveSupport::TestCase end test "as a normal user with write_notes token" do - token = create(:oauth_access_token, :scopes => %w[write_notes]) - ability = ApiAbility.new token + user = create(:user) + token = create(:oauth_access_token, :user => user, :scopes => %w[write_notes]) + ability = ApiAbility.new user, token [:destroy].each do |action| assert ability.cannot? action, Note @@ -68,8 +74,9 @@ class NoteApiCapabilityTest < ActiveSupport::TestCase end test "as a moderator with permissionless token" do - token = create(:oauth_access_token, :user => create(:moderator_user)) - ability = ApiAbility.new token + user = create(:moderator_user) + token = create(:oauth_access_token, :user => user) + ability = ApiAbility.new user, token [:destroy].each do |action| assert ability.cannot? action, Note @@ -77,8 +84,9 @@ class NoteApiCapabilityTest < ActiveSupport::TestCase end test "as a moderator with write_notes token" do - token = create(:oauth_access_token, :user => create(:moderator_user), :scopes => %w[write_notes]) - ability = ApiAbility.new token + user = create(:moderator_user) + token = create(:oauth_access_token, :user => user, :scopes => %w[write_notes]) + ability = ApiAbility.new user, token [:destroy].each do |action| assert ability.can? action, Note @@ -89,15 +97,16 @@ end class UserApiCapabilityTest < ActiveSupport::TestCase test "user preferences" do # A user with empty tokens - token = create(:oauth_access_token) - ability = ApiAbility.new token + user = create(:user) + token = create(:oauth_access_token, :user => user) + ability = ApiAbility.new user, token [:index, :show, :update_all, :update, :destroy].each do |act| assert ability.cannot? act, UserPreference end - token = create(:oauth_access_token, :scopes => %w[read_prefs]) - ability = ApiAbility.new token + token = create(:oauth_access_token, :user => user, :scopes => %w[read_prefs]) + ability = ApiAbility.new user, token [:update_all, :update, :destroy].each do |act| assert ability.cannot? act, UserPreference @@ -107,8 +116,8 @@ class UserApiCapabilityTest < ActiveSupport::TestCase assert ability.can? act, UserPreference end - token = create(:oauth_access_token, :scopes => %w[write_prefs]) - ability = ApiAbility.new token + token = create(:oauth_access_token, :user => user, :scopes => %w[write_prefs]) + ability = ApiAbility.new user, token [:index, :show].each do |act| assert ability.cannot? act, UserPreference From 77a2657d33f0066dbdda5fce831113b6e165a264 Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Wed, 12 Feb 2025 18:13:56 +0300 Subject: [PATCH 09/11] Pass scopes instead of token to ApiAbility --- app/abilities/api_ability.rb | 38 +++++++---------- app/controllers/api_controller.rb | 5 ++- test/abilities/api_abilities_test.rb | 11 ++--- test/abilities/api_capability_test.rb | 61 +++++++++++++-------------- 4 files changed, 55 insertions(+), 60 deletions(-) diff --git a/app/abilities/api_ability.rb b/app/abilities/api_ability.rb index 3bc82eab2..c62f65368 100644 --- a/app/abilities/api_ability.rb +++ b/app/abilities/api_ability.rb @@ -3,7 +3,7 @@ class ApiAbility include CanCan::Ability - def initialize(user, token) + def initialize(user, scopes) can :read, [:version, :capability, :permission, :map] if Settings.status != "database_offline" @@ -17,31 +17,31 @@ class ApiAbility can :read, UserBlock if user&.active? - can [:create, :comment, :close, :reopen], Note if scope?(token, :write_notes) - can [:create, :destroy], NoteSubscription if scope?(token, :write_notes) + can [:create, :comment, :close, :reopen], Note if scopes.include?("write_notes") + can [:create, :destroy], NoteSubscription if scopes.include?("write_notes") - can :read, Trace if scope?(token, :read_gpx) - can [:create, :update, :destroy], Trace if scope?(token, :write_gpx) + can :read, Trace if scopes.include?("read_gpx") + can [:create, :update, :destroy], Trace if scopes.include?("write_gpx") - can :details, User if scope?(token, :read_prefs) - can :read, UserPreference if scope?(token, :read_prefs) - can [:update, :update_all, :destroy], UserPreference if scope?(token, :write_prefs) + can :details, User if scopes.include?("read_prefs") + can :read, UserPreference if scopes.include?("read_prefs") + can [:update, :update_all, :destroy], UserPreference if scopes.include?("write_prefs") - can [:read, :update, :destroy], Message if scope?(token, :consume_messages) - can :create, Message if scope?(token, :send_messages) + can [:read, :update, :destroy], Message if scopes.include?("consume_messages") + can :create, Message if scopes.include?("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, :destroy], [Node, Way, Relation] if scope?(token, :write_api) + can [:create, :update, :upload, :close, :subscribe, :unsubscribe], Changeset if scopes.include?("write_api") + can :create, ChangesetComment if scopes.include?("write_api") + can [:create, :update, :destroy], [Node, Way, Relation] if scopes.include?("write_api") end if user.moderator? - can [:destroy, :restore], ChangesetComment if scope?(token, :write_api) + can [:destroy, :restore], ChangesetComment if scopes.include?("write_api") - can :destroy, Note if scope?(token, :write_notes) + can :destroy, Note if scopes.include?("write_notes") - can :redact, [OldNode, OldWay, OldRelation] if user&.terms_agreed? && scope?(token, :write_redactions) + can :redact, [OldNode, OldWay, OldRelation] if user&.terms_agreed? && scopes.include?("write_redactions") end end end @@ -73,10 +73,4 @@ 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 diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index 27f262d00..86924d55d 100644 --- a/app/controllers/api_controller.rb +++ b/app/controllers/api_controller.rb @@ -66,9 +66,10 @@ class ApiController < ApplicationController # Use capabilities from the oauth token if it exists and is a valid access token if doorkeeper_token&.accessible? user = User.find(doorkeeper_token.resource_owner_id) - ApiAbility.new(user, doorkeeper_token) + scopes = Set.new doorkeeper_token.scopes + ApiAbility.new(user, scopes) else - ApiAbility.new(nil, nil) + ApiAbility.new(nil, Set.new) end end diff --git a/test/abilities/api_abilities_test.rb b/test/abilities/api_abilities_test.rb index c32300c60..38154174c 100644 --- a/test/abilities/api_abilities_test.rb +++ b/test/abilities/api_abilities_test.rb @@ -7,7 +7,8 @@ end class GuestApiAbilityTest < ApiAbilityTest test "note permissions for a guest" do - ability = ApiAbility.new nil, nil + scopes = Set.new + ability = ApiAbility.new nil, scopes [:index, :create, :feed, :show, :search].each do |action| assert ability.can?(action, Note), "should be able to #{action} Notes" @@ -22,8 +23,8 @@ end class UserApiAbilityTest < ApiAbilityTest test "Note permissions" do user = create(:user) - token = create(:oauth_access_token, :user => user, :scopes => %w[write_notes]) - ability = ApiAbility.new user, token + scopes = Set.new %w[write_notes] + ability = ApiAbility.new user, scopes [:index, :create, :comment, :feed, :show, :search, :close, :reopen].each do |action| assert ability.can?(action, Note), "should be able to #{action} Notes" @@ -38,8 +39,8 @@ end class ModeratorApiAbilityTest < ApiAbilityTest test "Note permissions" do user = create(:moderator_user) - token = create(:oauth_access_token, :user => user, :scopes => %w[write_notes]) - ability = ApiAbility.new user, token + scopes = Set.new %w[write_notes] + ability = ApiAbility.new user, scopes [:index, :create, :comment, :feed, :show, :search, :close, :reopen, :destroy].each do |action| assert ability.can?(action, Note), "should be able to #{action} Notes" diff --git a/test/abilities/api_capability_test.rb b/test/abilities/api_capability_test.rb index 12bbc4965..0f69ddba9 100644 --- a/test/abilities/api_capability_test.rb +++ b/test/abilities/api_capability_test.rb @@ -3,20 +3,20 @@ require "test_helper" class ChangesetCommentApiCapabilityTest < ActiveSupport::TestCase - test "as a normal user with permissionless token" do + test "as a normal user without scopes" do user = create(:user) - token = create(:oauth_access_token, :user => user) - ability = ApiAbility.new user, token + scopes = Set.new + ability = ApiAbility.new user, scopes [:create, :destroy, :restore].each do |action| assert ability.cannot? action, ChangesetComment end end - test "as a normal user with write_api token" do + test "as a normal user with write_api scope" do user = create(:user) - token = create(:oauth_access_token, :user => user, :scopes => %w[write_api]) - ability = ApiAbility.new user, token + scopes = Set.new %w[write_api] + ability = ApiAbility.new user, scopes [:destroy, :restore].each do |action| assert ability.cannot? action, ChangesetComment @@ -27,20 +27,20 @@ class ChangesetCommentApiCapabilityTest < ActiveSupport::TestCase end end - test "as a moderator with permissionless token" do + test "as a moderator without scopes" do user = create(:moderator_user) - token = create(:oauth_access_token, :user => user) - ability = ApiAbility.new user, token + scopes = Set.new + ability = ApiAbility.new user, scopes [:create, :destroy, :restore].each do |action| assert ability.cannot? action, ChangesetComment end end - test "as a moderator with write_api token" do + test "as a moderator with write_api scope" do user = create(:moderator_user) - token = create(:oauth_access_token, :user => user, :scopes => %w[write_api]) - ability = ApiAbility.new user, token + scopes = Set.new %w[write_api] + ability = ApiAbility.new user, scopes [:create, :destroy, :restore].each do |action| assert ability.can? action, ChangesetComment @@ -49,20 +49,20 @@ class ChangesetCommentApiCapabilityTest < ActiveSupport::TestCase end class NoteApiCapabilityTest < ActiveSupport::TestCase - test "as a normal user with permissionless token" do + test "as a normal user without scopes" do user = create(:user) - token = create(:oauth_access_token, :user => user) - ability = ApiAbility.new user, token + scopes = Set.new + ability = ApiAbility.new user, scopes [:create, :comment, :close, :reopen, :destroy].each do |action| assert ability.cannot? action, Note end end - test "as a normal user with write_notes token" do + test "as a normal user with write_notes scope" do user = create(:user) - token = create(:oauth_access_token, :user => user, :scopes => %w[write_notes]) - ability = ApiAbility.new user, token + scopes = Set.new %w[write_notes] + ability = ApiAbility.new user, scopes [:destroy].each do |action| assert ability.cannot? action, Note @@ -73,20 +73,20 @@ class NoteApiCapabilityTest < ActiveSupport::TestCase end end - test "as a moderator with permissionless token" do + test "as a moderator without scopes" do user = create(:moderator_user) - token = create(:oauth_access_token, :user => user) - ability = ApiAbility.new user, token + scopes = Set.new + ability = ApiAbility.new user, scopes [:destroy].each do |action| assert ability.cannot? action, Note end end - test "as a moderator with write_notes token" do + test "as a moderator with write_notes scope" do user = create(:moderator_user) - token = create(:oauth_access_token, :user => user, :scopes => %w[write_notes]) - ability = ApiAbility.new user, token + scopes = Set.new %w[write_notes] + ability = ApiAbility.new user, scopes [:destroy].each do |action| assert ability.can? action, Note @@ -96,17 +96,16 @@ end class UserApiCapabilityTest < ActiveSupport::TestCase test "user preferences" do - # A user with empty tokens user = create(:user) - token = create(:oauth_access_token, :user => user) - ability = ApiAbility.new user, token + scopes = Set.new + ability = ApiAbility.new user, scopes [:index, :show, :update_all, :update, :destroy].each do |act| assert ability.cannot? act, UserPreference end - token = create(:oauth_access_token, :user => user, :scopes => %w[read_prefs]) - ability = ApiAbility.new user, token + scopes = Set.new %w[read_prefs] + ability = ApiAbility.new user, scopes [:update_all, :update, :destroy].each do |act| assert ability.cannot? act, UserPreference @@ -116,8 +115,8 @@ class UserApiCapabilityTest < ActiveSupport::TestCase assert ability.can? act, UserPreference end - token = create(:oauth_access_token, :user => user, :scopes => %w[write_prefs]) - ability = ApiAbility.new user, token + scopes = Set.new %w[write_prefs] + ability = ApiAbility.new user, scopes [:index, :show].each do |act| assert ability.cannot? act, UserPreference From 2d46b44872350975245a8b8bd10595d7d2bdfd4d Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Wed, 12 Feb 2025 19:32:14 +0300 Subject: [PATCH 10/11] Add write_changeset_comments scope --- app/abilities/api_ability.rb | 8 ++++---- app/controllers/api_controller.rb | 5 +++++ config/locales/en.yml | 10 ++++++---- lib/oauth.rb | 2 +- test/abilities/api_capability_test.rb | 8 ++++---- 5 files changed, 20 insertions(+), 13 deletions(-) diff --git a/app/abilities/api_ability.rb b/app/abilities/api_ability.rb index c62f65368..a0340c5cd 100644 --- a/app/abilities/api_ability.rb +++ b/app/abilities/api_ability.rb @@ -31,13 +31,13 @@ class ApiAbility can :create, Message if scopes.include?("send_messages") if user.terms_agreed? - can [:create, :update, :upload, :close, :subscribe, :unsubscribe], Changeset if scopes.include?("write_api") - can :create, ChangesetComment if scopes.include?("write_api") - can [:create, :update, :destroy], [Node, Way, Relation] if scopes.include?("write_api") + can [:create, :update, :upload, :close, :subscribe, :unsubscribe], Changeset if scopes.include?("write_map") + can :create, ChangesetComment if scopes.include?("write_changeset_comments") + can [:create, :update, :destroy], [Node, Way, Relation] if scopes.include?("write_map") end if user.moderator? - can [:destroy, :restore], ChangesetComment if scopes.include?("write_api") + can [:destroy, :restore], ChangesetComment if scopes.include?("write_changeset_comments") can :destroy, Note if scopes.include?("write_notes") diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index 86924d55d..bcd43a273 100644 --- a/app/controllers/api_controller.rb +++ b/app/controllers/api_controller.rb @@ -67,6 +67,11 @@ class ApiController < ApplicationController if doorkeeper_token&.accessible? user = User.find(doorkeeper_token.resource_owner_id) scopes = Set.new doorkeeper_token.scopes + if scopes.include?("write_api") + scopes.add("write_map") + scopes.add("write_changeset_comments") + scopes.delete("write_api") + end ApiAbility.new(user, scopes) else ApiAbility.new(nil, Set.new) diff --git a/config/locales/en.yml b/config/locales/en.yml index 9aabfc92a..5571a4232 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -87,12 +87,13 @@ en: url: Main Application URL (Required) callback_url: Callback URL support_url: Support URL - allow_read_prefs: read their user preferences + allow_read_prefs: read their user preferences allow_write_prefs: modify their user preferences allow_write_diary: create diary entries and comments - allow_write_api: modify the map - allow_read_gpx: read their private GPS traces - allow_write_gpx: upload GPS traces + allow_write_api: modify the map + allow_write_changeset_comments: comment on changesets + allow_read_gpx: read their private GPS traces + allow_write_gpx: upload GPS traces allow_write_notes: modify notes diary_comment: body: "Body" @@ -2697,6 +2698,7 @@ en: write_prefs: Modify user preferences write_diary: Create diary entries and comments write_api: Modify the map + write_changeset_comments: Comment on changesets read_gpx: Read private GPS traces write_gpx: Upload GPS traces write_notes: Modify notes diff --git a/lib/oauth.rb b/lib/oauth.rb index 679c564a6..dfa3a8028 100644 --- a/lib/oauth.rb +++ b/lib/oauth.rb @@ -1,7 +1,7 @@ module Oauth SCOPES = %w[ read_prefs write_prefs write_diary - write_api read_gpx write_gpx write_notes write_redactions + write_api write_changeset_comments read_gpx write_gpx write_notes write_redactions consume_messages send_messages openid ].freeze PRIVILEGED_SCOPES = %w[read_email skip_authorization].freeze diff --git a/test/abilities/api_capability_test.rb b/test/abilities/api_capability_test.rb index 0f69ddba9..58c8f7fe7 100644 --- a/test/abilities/api_capability_test.rb +++ b/test/abilities/api_capability_test.rb @@ -13,9 +13,9 @@ class ChangesetCommentApiCapabilityTest < ActiveSupport::TestCase end end - test "as a normal user with write_api scope" do + test "as a normal user with write_changeset_comments scope" do user = create(:user) - scopes = Set.new %w[write_api] + scopes = Set.new %w[write_changeset_comments] ability = ApiAbility.new user, scopes [:destroy, :restore].each do |action| @@ -37,9 +37,9 @@ class ChangesetCommentApiCapabilityTest < ActiveSupport::TestCase end end - test "as a moderator with write_api scope" do + test "as a moderator with write_changeset_comments scope" do user = create(:moderator_user) - scopes = Set.new %w[write_api] + scopes = Set.new %w[write_changeset_comments] ability = ApiAbility.new user, scopes [:create, :destroy, :restore].each do |action| From a91ba62c4c3e30506278f4d9b828adf2a0509307 Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Thu, 13 Feb 2025 04:31:23 +0300 Subject: [PATCH 11/11] Test commenting changesets with different scopes --- .../api/changeset_comments_controller_test.rb | 86 ++++++++++++++++--- 1 file changed, 74 insertions(+), 12 deletions(-) diff --git a/test/controllers/api/changeset_comments_controller_test.rb b/test/controllers/api/changeset_comments_controller_test.rb index 91b8ad683..e456a3ca4 100644 --- a/test/controllers/api/changeset_comments_controller_test.rb +++ b/test/controllers/api/changeset_comments_controller_test.rb @@ -77,6 +77,34 @@ module Api end end + def test_create_without_required_scope + user = create(:user) + auth_header = bearer_authorization_header user, :scopes => %w[read_prefs] + changeset = create(:changeset, :closed) + + assert_difference "ChangesetComment.count", 0 do + post changeset_comment_path(changeset), :params => { :text => "This is a comment" }, :headers => auth_header + assert_response :forbidden + end + end + + def test_create_with_write_changeset_comments_scope + user = create(:user) + auth_header = bearer_authorization_header user, :scopes => %w[write_changeset_comments] + changeset = create(:changeset, :closed) + + assert_difference "ChangesetComment.count", 1 do + post changeset_comment_path(changeset), :params => { :text => "This is a comment" }, :headers => auth_header + assert_response :success + end + + comment = ChangesetComment.last + assert_equal changeset.id, comment.changeset_id + assert_equal user.id, comment.author_id + assert_equal "This is a comment", comment.body + assert comment.visible + end + def test_create_with_write_api_scope user = create(:user) auth_header = bearer_authorization_header user, :scopes => %w[write_api] @@ -308,15 +336,32 @@ module Api assert_response :not_found end - ## - # test hide comment succes - def test_hide + def test_hide_without_required_scope comment = create(:changeset_comment) - assert comment.visible - - auth_header = bearer_authorization_header create(:moderator_user) + auth_header = bearer_authorization_header create(:moderator_user), :scopes => %w[read_prefs] post changeset_comment_hide_path(comment), :headers => auth_header + + assert_response :forbidden + assert comment.reload.visible + end + + def test_hide_with_write_changeset_comments_scope + comment = create(:changeset_comment) + auth_header = bearer_authorization_header create(:moderator_user), :scopes => %w[write_changeset_comments] + + post changeset_comment_hide_path(comment), :headers => auth_header + + assert_response :success + assert_not comment.reload.visible + end + + def test_hide_with_write_api_scope + comment = create(:changeset_comment) + auth_header = bearer_authorization_header create(:moderator_user), :scopes => %w[write_api] + + post changeset_comment_hide_path(comment), :headers => auth_header + assert_response :success assert_not comment.reload.visible end @@ -348,15 +393,32 @@ module Api assert_response :not_found end - ## - # test unhide comment succes - def test_unhide + def test_unhide_without_required_scope comment = create(:changeset_comment, :visible => false) - assert_not comment.visible - - auth_header = bearer_authorization_header create(:moderator_user) + auth_header = bearer_authorization_header create(:moderator_user), :scopes => %w[read_prefs] post changeset_comment_unhide_path(comment), :headers => auth_header + + assert_response :forbidden + assert_not comment.reload.visible + end + + def test_unhide_with_write_changeset_comments_scope + comment = create(:changeset_comment, :visible => false) + auth_header = bearer_authorization_header create(:moderator_user), :scopes => %w[write_changeset_comments] + + post changeset_comment_unhide_path(comment), :headers => auth_header + + assert_response :success + assert comment.reload.visible + end + + def test_unhide_with_write_api_scope + comment = create(:changeset_comment, :visible => false) + auth_header = bearer_authorization_header create(:moderator_user), :scopes => %w[write_api] + + post changeset_comment_unhide_path(comment), :headers => auth_header + assert_response :success assert comment.reload.visible end