Fixed up delete methods on nodes, ways and relations to return the new version number...

diff --git a/app/controllers/node_controller.rb b/app/controllers/node_controller.rb
index f1023e78fd2de7669193b4ddb0911cbf9b6f3e63..eed00a01606c01e24581ecd80c47568bc5039d47 100644 (file)
--- a/app/controllers/node_controller.rb
+++ b/app/controllers/node_controller.rb
@@ -78,21 +78,17 @@ class NodeController < ApplicationController
     end
   end
 
     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)
   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
       
       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
       else
         render :nothing => true, :status => :bad_request
       end

diff --git a/app/controllers/relation_controller.rb b/app/controllers/relation_controller.rb
index 4b3fdf34fe7342851b5801f2c0a13702c097b721..7ce58dae60d6c63dbf3ac4df128e2c567e31492b 100644 (file)
--- 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 = 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
       else
         render :nothing => true, :status => :bad_request
       end

diff --git a/app/controllers/way_controller.rb b/app/controllers/way_controller.rb
index ac017ca6bcf54c98950e80f5b6db678365712091..b00658cf08e7c4deab178b03e8fccf06f54eb05d 100644 (file)
--- 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)
     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
       else
         render :nothing => true, :status => :bad_request
       end

diff --git a/app/models/node.rb b/app/models/node.rb
index 3b59ac80ea558960ade0cb97437da82f639d88d5..c2a61906b1a1dc60d6e360d30d435d11ce3eebac 100644 (file)
--- 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
   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 ])
     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 436c1e32ef8693bf068429743000b38cfe8f4546..db4dd52a6799f68960b32a001bf1d0b7b50626c7 100644 (file)
--- a/app/models/relation.rb
+++ b/app/models/relation.rb
@@ -224,14 +224,12 @@ class Relation < ActiveRecord::Base
     end
   end
 
     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 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
         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 = []
         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
     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
     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 90458006e8cf15a64a85bd797a5bf6a4116d2130..591dee9a22c07a000cc4a413b709a2c4f13ad60a 100644 (file)
--- a/app/models/way.rb
+++ b/app/models/way.rb
@@ -245,19 +245,12 @@ class Way < ActiveRecord::Base
     return true
   end
 
     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
     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",
       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
         raise OSM::APIPreconditionFailedError

-      # end FIXME

       else
         self.changeset_id = new_way.changeset_id
         self.tags = []
       else
         self.changeset_id = new_way.changeset_id
         self.tags = []

diff --git a/lib/osm.rb b/lib/osm.rb
index 82fc835b4ad8c17e684ae6565604f5bd16976245..ae8b81f5b13f1433f8fa3162cdeded0592c27f64 100644 (file)
--- a/lib/osm.rb
+++ b/lib/osm.rb
@@ -68,6 +68,21 @@ module OSM
     end
   end
 
     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
   # 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 085cd3078f6d28d275f76d9f7f397eeea68df1ed..dbc00cbb2471e1918a525d3c7b5f8b1000740c47 100644 (file)
--- 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
 
     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
     # 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
     # 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
 
     # 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
 
   ##
   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'"
     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 "<osm><node lat='0' lon='0' changeset='#{changeset_id}'>" +
+      '<tag k="#{@user.inspect}" v="0"/>' +
+      '</node></osm>'
+    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}")
 
   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 b45c808747a6e1db29316b66a16a92cc4b0c763f..b54d9bc98cdce204c9ea8f78b1c58dbd9dda1fee 100644 (file)
--- 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
 
     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
 
     # 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 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
     # 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 58bb6a9a11a822fc4bde8c824c0680e97db459a1..d889be2ba83503313e8737cc766b0a132d4fdae3 100644 (file)
--- 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
     
     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
 
     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 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
     # this won't work since the way never existed
     delete :delete, :id => 0
     assert_response :not_found