From: Tom Hughes Date: Sun, 25 Feb 2024 14:06:56 +0000 (+0000) Subject: Merge remote-tracking branch 'upstream/pull/4251' X-Git-Tag: live~1201 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/53817fa9e8d955df0891d156f21b7269b4ed08fc?ds=inline;hp=-c Merge remote-tracking branch 'upstream/pull/4251' --- 53817fa9e8d955df0891d156f21b7269b4ed08fc diff --combined app/controllers/api/changesets_controller.rb index b4e864f18,0b0c53bed..8676d16d3 --- a/app/controllers/api/changesets_controller.rb +++ b/app/controllers/api/changesets_controller.rb @@@ -6,6 -6,7 +6,7 @@@ module Ap before_action :check_api_writable, :only => [:create, :update, :upload, :subscribe, :unsubscribe] before_action :check_api_readable, :except => [:create, :update, :upload, :download, :query, :subscribe, :unsubscribe] + before_action :setup_user_auth, :only => [:show] before_action :authorize, :only => [:create, :update, :upload, :close, :subscribe, :unsubscribe] authorize_resource @@@ -24,7 -25,11 +25,11 @@@ # return anything about the nodes, ways and relations in the changeset. def show @changeset = Changeset.find(params[:id]) - @include_discussion = params[:include_discussion].presence + if params[:include_discussion].presence + @comments = @changeset.comments + @comments = @comments.unscope(:where => :visible) if params[:show_hidden_comments].presence && can?(:restore, ChangesetComment) + @comments = @comments.includes(:author) + end render "changeset" respond_to do |format| @@@ -44,7 -49,7 +49,7 @@@ cs.save_with_tags! # Subscribe user to changeset comments - cs.subscribers << current_user + cs.subscribe(current_user) render :plain => cs.id.to_s end @@@ -92,10 -97,6 +97,10 @@@ diff_reader = DiffReader.new(request.raw_post, changeset) Changeset.transaction do result = diff_reader.commit + # the number of changes in this changeset has already been + # updated and is visible in this transaction so we don't need + # to allow for any more when checking the limit + check_rate_limit(0) render :xml => result.to_s end end @@@ -233,10 -234,10 +238,10 @@@ # Find the changeset and check it is valid changeset = Changeset.find(id) - raise OSM::APIChangesetAlreadySubscribedError, changeset if changeset.subscribers.exists?(current_user.id) + raise OSM::APIChangesetAlreadySubscribedError, changeset if changeset.subscribed?(current_user) # Add the subscriber - changeset.subscribers << current_user + changeset.subscribe(current_user) # Return a copy of the updated changeset @changeset = changeset @@@ -259,10 -260,10 +264,10 @@@ # Find the changeset and check it is valid changeset = Changeset.find(id) - raise OSM::APIChangesetNotSubscribedError, changeset unless changeset.subscribers.exists?(current_user.id) + raise OSM::APIChangesetNotSubscribedError, changeset unless changeset.subscribed?(current_user) # Remove the subscriber - changeset.subscribers.delete(current_user) + changeset.unsubscribe(current_user) # Return a copy of the updated changeset @changeset = changeset @@@ -283,6 -284,7 +288,6 @@@ ## # if a bounding box was specified do some sanity checks. # restrict changesets to those enclosed by a bounding box - # we need to return both the changesets and the bounding box def conditions_bbox(changesets, bbox) if bbox bbox.check_boundaries diff --combined test/controllers/api/changesets_controller_test.rb index 3278b2101,97ce5f7c9..c8fed5cbc --- a/test/controllers/api/changesets_controller_test.rb +++ b/test/controllers/api/changesets_controller_test.rb @@@ -134,14 -134,12 +134,14 @@@ module Ap def test_create_wrong_method auth_header = basic_authorization_header create(:user).email, "test" - assert_raise ActionController::RoutingError do - get changeset_create_path, :headers => auth_header - end - assert_raise ActionController::RoutingError do - post changeset_create_path, :headers => auth_header - end + + get changeset_create_path, :headers => auth_header + assert_response :not_found + assert_template "rescues/routing_error" + + post changeset_create_path, :headers => auth_header + assert_response :not_found + assert_template "rescues/routing_error" end ## @@@ -154,23 -152,20 +154,20 @@@ assert_response :success, "cannot get first changeset" assert_select "osm[version='#{Settings.api_version}'][generator='#{Settings.generator}']", 1 - assert_select "osm>changeset[id='#{changeset.id}']", 1 - assert_select "osm>changeset>@open", "true" - assert_select "osm>changeset>@created_at", changeset.created_at.xmlschema - assert_select "osm>changeset>@closed_at", 0 + assert_single_changeset changeset assert_select "osm>changeset>discussion", 0 get changeset_show_path(changeset), :params => { :include_discussion => true } assert_response :success, "cannot get first changeset with comments" assert_select "osm[version='#{Settings.api_version}'][generator='#{Settings.generator}']", 1 - assert_select "osm>changeset[id='#{changeset.id}']", 1 - assert_select "osm>changeset>@open", "true" - assert_select "osm>changeset>@created_at", changeset.created_at.xmlschema - assert_select "osm>changeset>@closed_at", 0 + assert_single_changeset changeset assert_select "osm>changeset>discussion", 1 assert_select "osm>changeset>discussion>comment", 0 + end + def test_show_comments + # all comments visible changeset = create(:changeset, :closed) comment1, comment2, comment3 = create_list(:changeset_comment, 3, :changeset_id => changeset.id) @@@ -178,15 -173,62 +175,62 @@@ assert_response :success, "cannot get closed changeset with comments" assert_select "osm[version='#{Settings.api_version}'][generator='#{Settings.generator}']", 1 - assert_select "osm>changeset[id='#{changeset.id}']", 1 - assert_select "osm>changeset>@open", "false" - assert_select "osm>changeset>@created_at", changeset.created_at.xmlschema - assert_select "osm>changeset>@closed_at", changeset.closed_at.xmlschema + assert_single_changeset changeset assert_select "osm>changeset>discussion", 1 assert_select "osm>changeset>discussion>comment", 3 assert_select "osm>changeset>discussion>comment:nth-child(1)>@id", comment1.id.to_s + assert_select "osm>changeset>discussion>comment:nth-child(1)>@visible", "true" assert_select "osm>changeset>discussion>comment:nth-child(2)>@id", comment2.id.to_s + assert_select "osm>changeset>discussion>comment:nth-child(2)>@visible", "true" assert_select "osm>changeset>discussion>comment:nth-child(3)>@id", comment3.id.to_s + assert_select "osm>changeset>discussion>comment:nth-child(3)>@visible", "true" + + # one hidden comment not included because not asked for + comment2.update(:visible => false) + + get changeset_show_path(changeset), :params => { :include_discussion => true } + assert_response :success, "cannot get closed changeset with comments" + + assert_select "osm[version='#{Settings.api_version}'][generator='#{Settings.generator}']", 1 + assert_single_changeset changeset + assert_select "osm>changeset>discussion", 1 + assert_select "osm>changeset>discussion>comment", 2 + assert_select "osm>changeset>discussion>comment:nth-child(1)>@id", comment1.id.to_s + assert_select "osm>changeset>discussion>comment:nth-child(1)>@visible", "true" + assert_select "osm>changeset>discussion>comment:nth-child(2)>@id", comment3.id.to_s + assert_select "osm>changeset>discussion>comment:nth-child(2)>@visible", "true" + + # one hidden comment not included because no permissions + get changeset_show_path(changeset), :params => { :include_discussion => true, :show_hidden_comments => true } + assert_response :success, "cannot get closed changeset with comments" + + assert_select "osm[version='#{Settings.api_version}'][generator='#{Settings.generator}']", 1 + assert_single_changeset changeset + assert_select "osm>changeset>discussion", 1 + assert_select "osm>changeset>discussion>comment", 2 + assert_select "osm>changeset>discussion>comment:nth-child(1)>@id", comment1.id.to_s + assert_select "osm>changeset>discussion>comment:nth-child(1)>@visible", "true" + # maybe will show an empty comment element with visible=false in the future + assert_select "osm>changeset>discussion>comment:nth-child(2)>@id", comment3.id.to_s + assert_select "osm>changeset>discussion>comment:nth-child(2)>@visible", "true" + + # one hidden comment shown to moderators + moderator_user = create(:moderator_user) + auth_header = basic_authorization_header moderator_user.email, "test" + get changeset_show_path(changeset), :params => { :include_discussion => true, :show_hidden_comments => true }, + :headers => auth_header + assert_response :success, "cannot get closed changeset with comments" + + assert_select "osm[version='#{Settings.api_version}'][generator='#{Settings.generator}']", 1 + assert_single_changeset changeset + assert_select "osm>changeset>discussion", 1 + assert_select "osm>changeset>discussion>comment", 3 + assert_select "osm>changeset>discussion>comment:nth-child(1)>@id", comment1.id.to_s + assert_select "osm>changeset>discussion>comment:nth-child(1)>@visible", "true" + assert_select "osm>changeset>discussion>comment:nth-child(2)>@id", comment2.id.to_s + assert_select "osm>changeset>discussion>comment:nth-child(2)>@visible", "false" + assert_select "osm>changeset>discussion>comment:nth-child(3)>@id", comment3.id.to_s + assert_select "osm>changeset>discussion>comment:nth-child(3)>@visible", "true" end def test_show_json @@@ -200,10 -242,7 +244,7 @@@ assert_equal Settings.api_version, js["version"] assert_equal Settings.generator, js["generator"] - assert_equal changeset.id, js["changeset"]["id"] - assert_operator js["changeset"], :[], "open" - assert_equal changeset.created_at.xmlschema, js["changeset"]["created_at"] - assert_nil js["changeset"]["closed_at"] + assert_single_changeset_json changeset, js assert_nil js["changeset"]["tags"] assert_nil js["changeset"]["comments"] assert_equal changeset.user.id, js["changeset"]["uid"] @@@ -216,17 -255,17 +257,17 @@@ assert_not_nil js assert_equal Settings.api_version, js["version"] assert_equal Settings.generator, js["generator"] - assert_equal changeset.id, js["changeset"]["id"] - assert_operator js["changeset"], :[], "open" - assert_equal changeset.created_at.xmlschema, js["changeset"]["created_at"] - assert_nil js["changeset"]["closed_at"] + assert_single_changeset_json changeset, js assert_nil js["changeset"]["tags"] assert_nil js["changeset"]["min_lat"] assert_nil js["changeset"]["min_lon"] assert_nil js["changeset"]["max_lat"] assert_nil js["changeset"]["max_lon"] assert_equal 0, js["changeset"]["comments"].count + end + def test_show_comments_json + # all comments visible changeset = create(:changeset, :closed) comment0, comment1, comment2 = create_list(:changeset_comment, 3, :changeset_id => changeset.id) @@@ -237,14 -276,67 +278,67 @@@ assert_not_nil js assert_equal Settings.api_version, js["version"] assert_equal Settings.generator, js["generator"] - assert_equal changeset.id, js["changeset"]["id"] - assert_not js["changeset"]["open"] - assert_equal changeset.created_at.xmlschema, js["changeset"]["created_at"] - assert_equal changeset.closed_at.xmlschema, js["changeset"]["closed_at"] + assert_single_changeset_json changeset, js + assert_equal 3, js["changeset"]["comments"].count + assert_equal comment0.id, js["changeset"]["comments"][0]["id"] + assert_operator true, :==, js["changeset"]["comments"][0]["visible"] + assert_equal comment1.id, js["changeset"]["comments"][1]["id"] + assert_operator true, :==, js["changeset"]["comments"][1]["visible"] + assert_equal comment2.id, js["changeset"]["comments"][2]["id"] + assert_operator true, :==, js["changeset"]["comments"][2]["visible"] + + # one hidden comment not included because not asked for + comment1.update(:visible => false) + + get changeset_show_path(changeset), :params => { :format => "json", :include_discussion => true } + assert_response :success, "cannot get closed changeset with comments" + + js = ActiveSupport::JSON.decode(@response.body) + assert_not_nil js + assert_equal Settings.api_version, js["version"] + assert_equal Settings.generator, js["generator"] + assert_single_changeset_json changeset, js + assert_equal 2, js["changeset"]["comments"].count + assert_equal comment0.id, js["changeset"]["comments"][0]["id"] + assert_operator true, :==, js["changeset"]["comments"][0]["visible"] + assert_equal comment2.id, js["changeset"]["comments"][1]["id"] + assert_operator true, :==, js["changeset"]["comments"][1]["visible"] + + # one hidden comment not included because no permissions + get changeset_show_path(changeset), :params => { :format => "json", :include_discussion => true, :show_hidden_comments => true } + assert_response :success, "cannot get closed changeset with comments" + + js = ActiveSupport::JSON.decode(@response.body) + assert_not_nil js + assert_equal Settings.api_version, js["version"] + assert_equal Settings.generator, js["generator"] + assert_single_changeset_json changeset, js + assert_equal 2, js["changeset"]["comments"].count + assert_equal comment0.id, js["changeset"]["comments"][0]["id"] + assert_operator true, :==, js["changeset"]["comments"][0]["visible"] + # maybe will show an empty comment element with visible=false in the future + assert_equal comment2.id, js["changeset"]["comments"][1]["id"] + assert_operator true, :==, js["changeset"]["comments"][1]["visible"] + + # one hidden comment shown to moderators + moderator_user = create(:moderator_user) + auth_header = basic_authorization_header moderator_user.email, "test" + get changeset_show_path(changeset), :params => { :format => "json", :include_discussion => true, :show_hidden_comments => true }, + :headers => auth_header + assert_response :success, "cannot get closed changeset with comments" + + js = ActiveSupport::JSON.decode(@response.body) + assert_not_nil js + assert_equal Settings.api_version, js["version"] + assert_equal Settings.generator, js["generator"] + assert_single_changeset_json changeset, js assert_equal 3, js["changeset"]["comments"].count assert_equal comment0.id, js["changeset"]["comments"][0]["id"] + assert_operator true, :==, js["changeset"]["comments"][0]["visible"] assert_equal comment1.id, js["changeset"]["comments"][1]["id"] + assert_operator false, :==, js["changeset"]["comments"][1]["visible"] assert_equal comment2.id, js["changeset"]["comments"][2]["id"] + assert_operator true, :==, js["changeset"]["comments"][2]["visible"] end def test_show_tag_and_discussion_json @@@ -272,10 -364,7 +366,7 @@@ assert_not_nil js assert_equal Settings.api_version, js["version"] assert_equal Settings.generator, js["generator"] - assert_equal changeset.id, js["changeset"]["id"] - assert_not js["changeset"]["open"] - assert_equal changeset.created_at.xmlschema, js["changeset"]["created_at"] - assert_equal changeset.closed_at.xmlschema, js["changeset"]["closed_at"] + assert_single_changeset_json changeset, js assert_equal 2, js["changeset"]["tags"].count assert_equal 3, js["changeset"]["comments"].count assert_equal 3, js["changeset"]["comments_count"] @@@ -363,13 -452,13 +454,13 @@@ auth_header = basic_authorization_header user.email, "test" - assert_raise ActionController::RoutingError do - get changeset_close_path(changeset), :headers => auth_header - end + get changeset_close_path(changeset), :headers => auth_header + assert_response :not_found + assert_template "rescues/routing_error" - assert_raise ActionController::RoutingError do - post changeset_close_path(changeset), :headers => auth_header - end + post changeset_close_path(changeset), :headers => auth_header + assert_response :not_found + assert_template "rescues/routing_error" end ## @@@ -1606,107 -1695,6 +1697,107 @@@ assert_equal "Precondition failed: Node #{node.id} is still used by ways #{way.id}.", @response.body end + ## + # test initial rate limit + def test_upload_initial_rate_limit + # create a user + user = create(:user) + + # create some objects to use + node = create(:node) + way = create(:way_with_nodes, :nodes_count => 2) + relation = create(:relation) + + # create a changeset that puts us near the initial rate limit + changeset = create(:changeset, :user => user, + :created_at => Time.now.utc - 5.minutes, + :num_changes => Settings.initial_changes_per_hour - 2) + + # create authentication header + auth_header = basic_authorization_header user.email, "test" + + # simple diff to create a node way and relation using placeholders + diff = <<~CHANGESET + + + + + + + + + + + + + + + + + + + CHANGESET + + # upload it + post changeset_upload_path(changeset), :params => diff, :headers => auth_header + assert_response :too_many_requests, "upload did not hit rate limit" + end + + ## + # test maximum rate limit + def test_upload_maximum_rate_limit + # create a user + user = create(:user) + + # create some objects to use + node = create(:node) + way = create(:way_with_nodes, :nodes_count => 2) + relation = create(:relation) + + # create a changeset to establish our initial edit time + changeset = create(:changeset, :user => user, + :created_at => Time.now.utc - 28.days) + + # create changeset to put us near the maximum rate limit + total_changes = Settings.max_changes_per_hour - 2 + while total_changes.positive? + changes = [total_changes, Changeset::MAX_ELEMENTS].min + changeset = create(:changeset, :user => user, + :created_at => Time.now.utc - 5.minutes, + :num_changes => changes) + total_changes -= changes + end + + # create authentication header + auth_header = basic_authorization_header user.email, "test" + + # simple diff to create a node way and relation using placeholders + diff = <<~CHANGESET + + + + + + + + + + + + + + + + + + + CHANGESET + + # upload it + post changeset_upload_path(changeset), :params => diff, :headers => auth_header + assert_response :too_many_requests, "upload did not hit rate limit" + end + ## # when we make some simple changes we get the same changes back from the # diff download. @@@ -2284,11 -2272,7 +2375,11 @@@ # check that a changeset can contain a certain max number of changes. ## FIXME should be changed to an integration test due to the with_controller def test_changeset_limits - auth_header = basic_authorization_header create(:user).email, "test" + user = create(:user) + auth_header = basic_authorization_header user.email, "test" + + # create an old changeset to ensure we have the maximum rate limit + create(:changeset, :user => user, :created_at => Time.now.utc - 28.days) # open a new changeset xml = "" @@@ -2370,14 -2354,14 +2461,14 @@@ changeset = create(:changeset, :closed) assert_difference "changeset.subscribers.count", 1 do - post changeset_subscribe_path(changeset), :headers => auth_header + post api_changeset_subscribe_path(changeset), :headers => auth_header end assert_response :success # not closed changeset changeset = create(:changeset) assert_difference "changeset.subscribers.count", 1 do - post changeset_subscribe_path(changeset), :headers => auth_header + post api_changeset_subscribe_path(changeset), :headers => auth_header end assert_response :success end @@@ -2390,7 -2374,7 +2481,7 @@@ # unauthorized changeset = create(:changeset, :closed) assert_no_difference "changeset.subscribers.count" do - post changeset_subscribe_path(changeset) + post api_changeset_subscribe_path(changeset) end assert_response :unauthorized @@@ -2398,7 -2382,7 +2489,7 @@@ # bad changeset id assert_no_difference "changeset.subscribers.count" do - post changeset_subscribe_path(:id => 999111), :headers => auth_header + post api_changeset_subscribe_path(:id => 999111), :headers => auth_header end assert_response :not_found @@@ -2406,7 -2390,7 +2497,7 @@@ changeset = create(:changeset, :closed) changeset.subscribers.push(user) assert_no_difference "changeset.subscribers.count" do - post changeset_subscribe_path(changeset), :headers => auth_header + post api_changeset_subscribe_path(changeset), :headers => auth_header end assert_response :conflict end @@@ -2420,7 -2404,7 +2511,7 @@@ changeset.subscribers.push(user) assert_difference "changeset.subscribers.count", -1 do - post changeset_unsubscribe_path(changeset), :headers => auth_header + post api_changeset_unsubscribe_path(changeset), :headers => auth_header end assert_response :success @@@ -2429,7 -2413,7 +2520,7 @@@ changeset.subscribers.push(user) assert_difference "changeset.subscribers.count", -1 do - post changeset_unsubscribe_path(changeset), :headers => auth_header + post api_changeset_unsubscribe_path(changeset), :headers => auth_header end assert_response :success end @@@ -2440,7 -2424,7 +2531,7 @@@ # unauthorized changeset = create(:changeset, :closed) assert_no_difference "changeset.subscribers.count" do - post changeset_unsubscribe_path(changeset) + post api_changeset_unsubscribe_path(changeset) end assert_response :unauthorized @@@ -2448,20 -2432,47 +2539,47 @@@ # bad changeset id assert_no_difference "changeset.subscribers.count" do - post changeset_unsubscribe_path(:id => 999111), :headers => auth_header + post api_changeset_unsubscribe_path(:id => 999111), :headers => auth_header end assert_response :not_found # trying to unsubscribe when not subscribed changeset = create(:changeset, :closed) assert_no_difference "changeset.subscribers.count" do - post changeset_unsubscribe_path(changeset), :headers => auth_header + post api_changeset_unsubscribe_path(changeset), :headers => auth_header end assert_response :not_found end private + ## + # check that the output consists of one specific changeset + def assert_single_changeset(changeset) + assert_select "osm>changeset", 1 + assert_select "osm>changeset>@id", changeset.id.to_s + assert_select "osm>changeset>@created_at", changeset.created_at.xmlschema + if changeset.open? + assert_select "osm>changeset>@open", "true" + assert_select "osm>changeset>@closed_at", 0 + else + assert_select "osm>changeset>@open", "false" + assert_select "osm>changeset>@closed_at", changeset.closed_at.xmlschema + end + end + + def assert_single_changeset_json(changeset, js) + assert_equal changeset.id, js["changeset"]["id"] + assert_equal changeset.created_at.xmlschema, js["changeset"]["created_at"] + if changeset.open? + assert_operator true, :==, js["changeset"]["open"] + assert_nil js["changeset"]["closed_at"] + else + assert_operator false, :==, js["changeset"]["open"] + assert_equal changeset.closed_at.xmlschema, js["changeset"]["closed_at"] + end + end + ## # check that certain changesets exist in the output def assert_changesets(changesets)