]> git.openstreetmap.org Git - rails.git/commitdiff
Merge remote-tracking branch 'upstream/pull/4251'
authorTom Hughes <tom@compton.nu>
Sun, 25 Feb 2024 14:06:56 +0000 (14:06 +0000)
committerTom Hughes <tom@compton.nu>
Sun, 25 Feb 2024 14:06:56 +0000 (14:06 +0000)
1  2 
app/controllers/api/changesets_controller.rb
test/controllers/api/changesets_controller_test.rb

index b4e864f187d4e76d3552186ec6e3e94631f4b528,0b0c53bed666171a256978252dffae6fa9b59f6e..8676d16d3699192698aa2e7fe37e566463a37811
@@@ -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
      # 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
        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
  
        # 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
  
        # 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
      ##
      # 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
index 3278b2101f2c477b65915cdc9a6a1cf864cdf6b4,97ce5f7c9a51b41d8467cdb0c229f43196317915..c8fed5cbcfeff39140c28173f94e9c0068a7fac1
@@@ -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
  
      ##
        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)
  
        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
  
        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"]
        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)
  
        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
        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"]
  
        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
  
      ##
        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
 +        <osmChange>
 +         <create>
 +          <node id='-1' lon='0' lat='0' changeset='#{changeset.id}'>
 +           <tag k='foo' v='bar'/>
 +           <tag k='baz' v='bat'/>
 +          </node>
 +          <way id='-1' changeset='#{changeset.id}'>
 +           <nd ref='#{node.id}'/>
 +          </way>
 +         </create>
 +         <create>
 +          <relation id='-1' changeset='#{changeset.id}'>
 +           <member type='way' role='some' ref='#{way.id}'/>
 +           <member type='node' role='some' ref='#{node.id}'/>
 +           <member type='relation' role='some' ref='#{relation.id}'/>
 +          </relation>
 +         </create>
 +        </osmChange>
 +      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
 +        <osmChange>
 +         <create>
 +          <node id='-1' lon='0' lat='0' changeset='#{changeset.id}'>
 +           <tag k='foo' v='bar'/>
 +           <tag k='baz' v='bat'/>
 +          </node>
 +          <way id='-1' changeset='#{changeset.id}'>
 +           <nd ref='#{node.id}'/>
 +          </way>
 +         </create>
 +         <create>
 +          <relation id='-1' changeset='#{changeset.id}'>
 +           <member type='way' role='some' ref='#{way.id}'/>
 +           <member type='node' role='some' ref='#{node.id}'/>
 +           <member type='relation' role='some' ref='#{relation.id}'/>
 +          </relation>
 +         </create>
 +        </osmChange>
 +      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.
      # 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 = "<osm><changeset/></osm>"
        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
        # 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
  
  
        # 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
  
        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
        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
  
        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
        # 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
  
  
        # 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)