From: Matt Amos Date: Tue, 14 Oct 2008 14:27:12 +0000 (+0000) Subject: Fixed up delete methods on nodes, ways and relations to return the new version number... X-Git-Tag: live~8182^2~268 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/b56f57ec43dfefa4171dc0cefe26d9e75d4ca2bc Fixed up delete methods on nodes, ways and relations to return the new version number and added some more tests. --- diff --git a/app/controllers/node_controller.rb b/app/controllers/node_controller.rb index f1023e78f..eed00a016 100644 --- a/app/controllers/node_controller.rb +++ b/app/controllers/node_controller.rb @@ -78,21 +78,17 @@ class NodeController < ApplicationController end end - # Delete a node. Doesn't actually delete it, but retains its history in a wiki-like way. - # FIXME remove all the fricking SQL + # Delete a node. Doesn't actually delete it, but retains its history + # in a wiki-like way. We therefore treat it like an update, so the delete + # method returns the new version number. def delete begin node = Node.find(params[:id]) new_node = Node.from_xml(request.raw_post) - # FIXME we no longer care about the user, (or maybe we want to check - # that the user of the changeset is the same user as is making this - # little change?) we really care about the - # changeset which must be open, and that the version that we have been - # given is the one that is currently stored in the database if new_node and new_node.id == node.id - node.delete_with_history(new_node, @user) - render :nothing => true + node.delete_with_history!(new_node, @user) + render :text => node.version.to_s, :content_type => "text/plain" else render :nothing => true, :status => :bad_request end diff --git a/app/controllers/relation_controller.rb b/app/controllers/relation_controller.rb index 4b3fdf34f..7ce58dae6 100644 --- a/app/controllers/relation_controller.rb +++ b/app/controllers/relation_controller.rb @@ -67,8 +67,8 @@ class RelationController < ApplicationController relation = Relation.find(params[:id]) new_relation = Relation.from_xml(request.raw_post) if new_relation and new_relation.id == relation.id - relation.delete_with_history(new_relation, @user) - render :nothing => true + relation.delete_with_history!(new_relation, @user) + render :text => relation.version.to_s, :content_type => "text/plain" else render :nothing => true, :status => :bad_request end diff --git a/app/controllers/way_controller.rb b/app/controllers/way_controller.rb index ac017ca6b..b00658cf0 100644 --- a/app/controllers/way_controller.rb +++ b/app/controllers/way_controller.rb @@ -67,11 +67,10 @@ class WayController < ApplicationController begin way = Way.find(params[:id]) new_way = Way.from_xml(request.raw_post) - if new_way and new_way.id == way.id - way.delete_with_history(new_way, @user) - # if we get here, all is fine, otherwise something will catch below. - render :nothing => true + if new_way and new_way.id == way.id + way.delete_with_history!(new_way, @user) + render :text => way.version.to_s, :content_type => "text/plain" else render :nothing => true, :status => :bad_request end diff --git a/app/models/node.rb b/app/models/node.rb index 3b59ac80e..c2a61906b 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -133,7 +133,7 @@ class Node < ActiveRecord::Base end # Should probably be renamed delete_from to come in line with update - def delete_with_history(new_node, user) + def delete_with_history!(new_node, user) if self.visible check_consistency(self, new_node, user) if WayNode.find(:first, :joins => "INNER JOIN current_ways ON current_ways.id = current_way_nodes.id", :conditions => [ "current_ways.visible = 1 AND current_way_nodes.node_id = ?", self.id ]) diff --git a/app/models/relation.rb b/app/models/relation.rb index 436c1e32e..db4dd52a6 100644 --- a/app/models/relation.rb +++ b/app/models/relation.rb @@ -224,14 +224,12 @@ class Relation < ActiveRecord::Base end end - def delete_with_history(new_relation, user) + def delete_with_history!(new_relation, user) if self.visible check_consistency(self, new_relation, user) - if RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id", :conditions => [ "visible = 1 AND member_type='relation' and member_id=?", self.id ]) + if RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id", :conditions => [ "visible = 1 AND member_type='relation' and member_id=? ", self.id ]) raise OSM::APIPreconditionFailedError.new else - #self.user_id = user.id - # FIXME we need to deal with changeset here, which is probably already dealt with self.changeset_id = new_relation.changeset_id self.tags = [] self.members = [] @@ -248,8 +246,6 @@ class Relation < ActiveRecord::Base if !new_relation.preconditions_ok? raise OSM::APIPreconditionFailedError.new end - # FIXME need to deal with changeset etc - #self.user_id = user.id self.changeset_id = new_relation.changeset_id self.tags = new_relation.tags self.members = new_relation.members diff --git a/app/models/way.rb b/app/models/way.rb index 90458006e..591dee9a2 100644 --- a/app/models/way.rb +++ b/app/models/way.rb @@ -245,19 +245,12 @@ class Way < ActiveRecord::Base return true end - def delete_with_history(new_way, user) + def delete_with_history!(new_way, user) check_consistency(self, new_way, user) if self.visible - # FIXME - # this should actually delete the relations, - # not just throw a PreconditionFailed if it's a member of a relation!! - # WHY?? The editor should decide whether the node is in the relation or not! - - # FIXME: this should probably renamed to delete_with_history if RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id", - :conditions => [ "visible = 1 AND member_type='way' and member_id=?", self.id]) + :conditions => [ "visible = 1 AND member_type='way' and member_id=? ", self.id]) raise OSM::APIPreconditionFailedError - # end FIXME else self.changeset_id = new_way.changeset_id self.tags = [] diff --git a/lib/osm.rb b/lib/osm.rb index 82fc835b4..ae8b81f5b 100644 --- a/lib/osm.rb +++ b/lib/osm.rb @@ -68,6 +68,21 @@ module OSM end end + # raised when a two tags have a duplicate key string in an element. + # this is now forbidden by the API. + class APIDuplicateTagsError < APIError + def initialize(type, id, tag_key) + @type, @id, @tag_key = type, id, tag_key + end + + attr_reader :type, :id, :tag_key + + def render_opts + { :text => "Element #{@type}/#{@id} has duplicate tags with key #{@tag_key}.", + :status => :bad_request } + end + end + # Helper methods for going to/from mercator and lat/lng. class Mercator include Math diff --git a/test/functional/node_controller_test.rb b/test/functional/node_controller_test.rb index 085cd3078..dbc00cbb2 100644 --- a/test/functional/node_controller_test.rb +++ b/test/functional/node_controller_test.rb @@ -79,6 +79,11 @@ class NodeControllerTest < Test::Unit::TestCase delete :delete, :id => current_nodes(:visible_node).id assert_response :success + # valid delete should return the new version number, which should + # be greater than the old version number + assert @response.body.to_i > current_nodes(:visible_node).version, + "delete request should return a new version number for node" + # this won't work since the node is already deleted content(nodes(:invisible_node).to_xml) delete :delete, :id => current_nodes(:invisible_node).id @@ -92,12 +97,14 @@ class NodeControllerTest < Test::Unit::TestCase # in a way... content(nodes(:used_node_1).to_xml) delete :delete, :id => current_nodes(:used_node_1).id - assert_response :precondition_failed + assert_response :precondition_failed, + "shouldn't be able to delete a node used in a way (#{@response.body})" # in a relation... content(nodes(:node_used_by_relationship).to_xml) delete :delete, :id => current_nodes(:node_used_by_relationship).id - assert_response :precondition_failed + assert_response :precondition_failed, + "shouldn't be able to delete a node used in a relation (#{@response.body})" end ## @@ -197,7 +204,36 @@ class NodeControllerTest < Test::Unit::TestCase put :update, :id => current_nodes(:visible_node).id assert_response :bad_request, "adding duplicate tags to a node should fail with 'bad request'" - end + end + + # test whether string injection is possible + def test_string_injection + basic_authorization(users(:normal_user).email, "test") + changeset_id = changesets(:normal_user_first_change).id + + # try and put something into a string that the API might + # use unquoted and therefore allow code injection... + content "" + + '' + + '' + put :create + assert_response :success + nodeid = @response.body + + # find the node in the database + checknode = Node.find(nodeid) + assert_not_nil checknode, "node not found in data base after upload" + + # and grab it using the api + get :read, :id => nodeid + assert_response :success + apinode = Node.from_xml(@response.body) + assert_not_nil apinode, "downloaded node is nil, but shouldn't be" + + # check the tags are not corrupted + assert_equal checknode.tags, apinode.tags + assert apinode.tags.include?('#{@user.inspect}') + end def basic_authorization(user, pass) @request.env["HTTP_AUTHORIZATION"] = "Basic %s" % Base64.encode64("#{user}:#{pass}") diff --git a/test/functional/relation_controller_test.rb b/test/functional/relation_controller_test.rb index b45c80874..b54d9bc98 100644 --- a/test/functional/relation_controller_test.rb +++ b/test/functional/relation_controller_test.rb @@ -210,16 +210,34 @@ class RelationControllerTest < Test::Unit::TestCase delete :delete, :id => current_relations(:visible_relation).id assert_response :bad_request + # this won't work because the relation is in-use by another relation + content(relations(:used_relation).to_xml) + delete :delete, :id => current_relations(:used_relation).id + assert_response :precondition_failed, + "shouldn't be able to delete a relation used in a relation (#{@response.body})" + # this should work when we provide the appropriate payload... content(relations(:visible_relation).to_xml) delete :delete, :id => current_relations(:visible_relation).id assert_response :success + # valid delete should return the new version number, which should + # be greater than the old version number + assert @response.body.to_i > current_relations(:visible_relation).version, + "delete request should return a new version number for relation" + # this won't work since the relation is already deleted content(relations(:invisible_relation).to_xml) delete :delete, :id => current_relations(:invisible_relation).id assert_response :gone + # this works now because the relation which was using this one + # has been deleted. + content(relations(:used_relation).to_xml) + delete :delete, :id => current_relations(:used_relation).id + assert_response :success, + "should be able to delete a relation used in an old relation (#{@response.body})" + # this won't work since the relation never existed delete :delete, :id => 0 assert_response :not_found diff --git a/test/functional/way_controller_test.rb b/test/functional/way_controller_test.rb index 58bb6a9a1..d889be2ba 100644 --- a/test/functional/way_controller_test.rb +++ b/test/functional/way_controller_test.rb @@ -165,17 +165,28 @@ class WayControllerTest < Test::Unit::TestCase delete :delete, :id => current_ways(:visible_way).id assert_response :bad_request - # Now try and get a changeset - changeset_id = changesets(:normal_user_first_change).id + # Now try with a valid changeset content current_ways(:visible_way).to_xml delete :delete, :id => current_ways(:visible_way).id assert_response :success + # check the returned value - should be the new version number + # valid delete should return the new version number, which should + # be greater than the old version number + assert @response.body.to_i > current_ways(:visible_way).version, + "delete request should return a new version number for way" + # this won't work since the way is already deleted content current_ways(:invisible_way).to_xml delete :delete, :id => current_ways(:invisible_way).id assert_response :gone + # this shouldn't work as the way is used in a relation + content current_ways(:used_way).to_xml + delete :delete, :id => current_ways(:used_way).id + assert_response :precondition_failed, + "shouldn't be able to delete a way used in a relation (#{@response.body})" + # this won't work since the way never existed delete :delete, :id => 0 assert_response :not_found