From: Matt Amos Date: Mon, 13 Oct 2008 17:50:15 +0000 (+0000) Subject: Added a bunch more tests on the API 0.6. Fixed node/way/relation from_xml code to... X-Git-Tag: live~7604^2~270 X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/cf24a5a3ee68905c5f55cf6f17c5d2ea983cb34f Added a bunch more tests on the API 0.6. Fixed node/way/relation from_xml code to disallow duplicate tag keys. --- diff --git a/app/controllers/node_controller.rb b/app/controllers/node_controller.rb index 309b930b7..f1023e78f 100644 --- a/app/controllers/node_controller.rb +++ b/app/controllers/node_controller.rb @@ -71,9 +71,8 @@ class NodeController < ApplicationController else render :nothing => true, :status => :bad_request end - rescue OSM::APIVersionMismatchError => ex - render :text => "Version mismatch: Provided " + ex.provided.to_s + - ", server had: " + ex.latest.to_s, :status => :bad_request + rescue OSM::APIError => ex + render ex.render_opts rescue ActiveRecord::RecordNotFound render :nothing => true, :status => :not_found end diff --git a/app/models/node.rb b/app/models/node.rb index d6a5143db..3b59ac80e 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -75,7 +75,7 @@ class Node < ActiveRecord::Base def self.from_xml_node(pt, create=false) node = Node.new - node.version = pt['version'] + node.version = pt['version'].to_i node.lat = pt['lat'].to_f node.lon = pt['lon'].to_f node.changeset_id = pt['changeset'].to_i @@ -138,7 +138,7 @@ class Node < ActiveRecord::Base 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 ]) raise OSM::APIPreconditionFailedError.new - elsif RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id", :conditions => [ "visible = 1 AND member_type='node' and member_id=?", self.id]) + elsif RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id", :conditions => [ "visible = 1 AND member_type='node' and member_id=? ", self.id]) raise OSM::APIPreconditionFailedError.new else self.changeset_id = new_node.changeset_id @@ -227,6 +227,11 @@ class Node < ActiveRecord::Base def add_tag_key_val(k,v) @tags = Hash.new unless @tags + + # duplicate tags are now forbidden, so we can't allow values + # in the hash to be overwritten. + raise OSM::APIDuplicateTagsError.new if @tags.include? k + @tags[k] = v end diff --git a/app/models/relation.rb b/app/models/relation.rb index 195090e4b..436c1e32e 100644 --- a/app/models/relation.rb +++ b/app/models/relation.rb @@ -182,6 +182,11 @@ class Relation < ActiveRecord::Base def add_tag_keyval(k, v) @tags = Hash.new unless @tags + + # duplicate tags are now forbidden, so we can't allow values + # in the hash to be overwritten. + raise OSM::APIDuplicateTagsError.new if @tags.include? k + @tags[k] = v end diff --git a/app/models/way.rb b/app/models/way.rb index f26d7658b..90458006e 100644 --- a/app/models/way.rb +++ b/app/models/way.rb @@ -169,6 +169,11 @@ class Way < ActiveRecord::Base def add_tag_keyval(k, v) @tags = Hash.new unless @tags + + # duplicate tags are now forbidden, so we can't allow values + # in the hash to be overwritten. + raise OSM::APIDuplicateTagsError.new if @tags.include? k + @tags[k] = v end diff --git a/test/functional/node_controller_test.rb b/test/functional/node_controller_test.rb index a8ef700a3..085cd3078 100644 --- a/test/functional/node_controller_test.rb +++ b/test/functional/node_controller_test.rb @@ -16,7 +16,6 @@ class NodeControllerTest < Test::Unit::TestCase def test_create # cannot read password from fixture as it is stored as MD5 digest basic_authorization(users(:normal_user).email, "test"); - # FIXME we need to create a changeset first argh # create a node with random lat/lon lat = rand(100)-50 + rand @@ -57,7 +56,6 @@ class NodeControllerTest < Test::Unit::TestCase # this tests deletion restrictions - basic deletion is tested in the unit # tests for node! def test_delete - # first try to delete node without auth delete :delete, :id => current_nodes(:visible_node).id assert_response :unauthorized @@ -65,7 +63,18 @@ class NodeControllerTest < Test::Unit::TestCase # now set auth basic_authorization(users(:normal_user).email, "test"); - # delete now takes a payload + # try to delete with an invalid (closed) changeset + content update_changeset(current_nodes(:visible_node).to_xml, + changesets(:normal_user_closed_change).id) + delete :delete, :id => current_nodes(:visible_node).id + assert_response :conflict + + # try to delete with an invalid (non-existent) changeset + content update_changeset(current_nodes(:visible_node).to_xml,0) + delete :delete, :id => current_nodes(:visible_node).id + assert_response :conflict + + # valid delete now takes a payload content(nodes(:visible_node).to_xml) delete :delete, :id => current_nodes(:visible_node).id assert_response :success @@ -79,12 +88,116 @@ class NodeControllerTest < Test::Unit::TestCase delete :delete, :id => 0 assert_response :not_found - # this won't work since the node is in use + ## these test whether nodes which are in-use can be deleted: + # in a way... content(nodes(:used_node_1).to_xml) delete :delete, :id => current_nodes(:used_node_1).id assert_response :precondition_failed + + # 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 + end + + ## + # tests whether the API works and prevents incorrect use while trying + # to update nodes. + def test_update + # try and update a node without authorisation + # first try to delete node without auth + content current_nodes(:visible_node).to_xml + put :update, :id => current_nodes(:visible_node).id + assert_response :unauthorized + + # setup auth + basic_authorization(users(:normal_user).email, "test") + + ## trying to break changesets + + # try and update in someone else's changeset + content update_changeset(current_nodes(:visible_node).to_xml, + changesets(:second_user_first_change).id) + put :update, :id => current_nodes(:visible_node).id + assert_response :conflict, "update with other user's changeset should be rejected" + + # try and update in a closed changeset + content update_changeset(current_nodes(:visible_node).to_xml, + changesets(:normal_user_closed_change).id) + put :update, :id => current_nodes(:visible_node).id + assert_response :conflict, "update with closed changeset should be rejected" + + # try and update in a non-existant changeset + content update_changeset(current_nodes(:visible_node).to_xml, 0) + put :update, :id => current_nodes(:visible_node).id + assert_response :conflict, "update with changeset=0 should be rejected" + + ## try and submit invalid updates + content xml_attr_rewrite(current_nodes(:visible_node).to_xml, 'lat', 91.0); + put :update, :id => current_nodes(:visible_node).id + assert_response :bad_request, "node at lat=91 should be rejected" + + content xml_attr_rewrite(current_nodes(:visible_node).to_xml, 'lat', -91.0); + put :update, :id => current_nodes(:visible_node).id + assert_response :bad_request, "node at lat=-91 should be rejected" + + content xml_attr_rewrite(current_nodes(:visible_node).to_xml, 'lon', 181.0); + put :update, :id => current_nodes(:visible_node).id + assert_response :bad_request, "node at lon=181 should be rejected" + + content xml_attr_rewrite(current_nodes(:visible_node).to_xml, 'lon', -181.0); + put :update, :id => current_nodes(:visible_node).id + assert_response :bad_request, "node at lon=-181 should be rejected" + + ## next, attack the versioning + current_node_version = current_nodes(:visible_node).version + + # try and submit a version behind + content xml_attr_rewrite(current_nodes(:visible_node).to_xml, + 'version', current_node_version - 1); + put :update, :id => current_nodes(:visible_node).id + assert_response :conflict, "should have failed on old version number" + + # try and submit a version ahead + content xml_attr_rewrite(current_nodes(:visible_node).to_xml, + 'version', current_node_version + 1); + put :update, :id => current_nodes(:visible_node).id + assert_response :conflict, "should have failed on skipped version number" + + # try and submit total crap in the version field + content xml_attr_rewrite(current_nodes(:visible_node).to_xml, + 'version', 'p1r4t3s!'); + put :update, :id => current_nodes(:visible_node).id + assert_response :conflict, + "should not be able to put 'p1r4at3s!' in the version field" + + ## finally, produce a good request which should work + content current_nodes(:visible_node).to_xml + put :update, :id => current_nodes(:visible_node).id + assert_response :success, "a valid update request failed" end + ## + # test adding tags to a node + def test_duplicate_tags + # setup auth + basic_authorization(users(:normal_user).email, "test") + + # add an identical tag to the node + tag_xml = XML::Node.new("tag") + tag_xml['k'] = current_node_tags(:t1).k + tag_xml['v'] = current_node_tags(:t1).v + + # add the tag into the existing xml + node_xml = current_nodes(:visible_node).to_xml + node_xml.find("//osm/node").first << tag_xml + + # try and upload it + content node_xml + put :update, :id => current_nodes(:visible_node).id + assert_response :bad_request, + "adding duplicate tags to a node should fail with 'bad request'" + end def basic_authorization(user, pass) @request.env["HTTP_AUTHORIZATION"] = "Basic %s" % Base64.encode64("#{user}:#{pass}") @@ -93,4 +206,25 @@ class NodeControllerTest < Test::Unit::TestCase def content(c) @request.env["RAW_POST_DATA"] = c.to_s end + + ## + # update the changeset_id of a node element + def update_changeset(xml, changeset_id) + xml_attr_rewrite(xml, 'changeset', changeset_id) + end + + ## + # update an attribute in the node element + def xml_attr_rewrite(xml, name, value) + xml.find("//osm/node").first[name] = value.to_s + return xml + end + + ## + # parse some xml + def xml_parse(xml) + parser = XML::Parser.new + parser.string = xml + parser.parse + end end