From: Kai Krueger <kakrueger@gmail.com>
Date: Wed, 28 Mar 2012 00:55:31 +0000 (-0600)
Subject: Copy the redaction code from nodes to ways
X-Git-Tag: live~6154
X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/72e59b49fa0512e5c7d16217edce62225094ffe3

Copy the redaction code from nodes to ways
---

diff --git a/app/controllers/old_way_controller.rb b/app/controllers/old_way_controller.rb
index 3836d4ab7..fc7366bb6 100644
--- a/app/controllers/old_way_controller.rb
+++ b/app/controllers/old_way_controller.rb
@@ -2,17 +2,25 @@ class OldWayController < ApplicationController
   require 'xml/libxml'
 
   skip_before_filter :verify_authenticity_token
+  before_filter :authorize, :only => [ :redact ]
+  before_filter :require_allow_write_api, :only => [ :redact ]
   before_filter :check_api_readable
   after_filter :compress_output
   around_filter :api_call_handle_error, :api_call_timeout
 
   def history
     way = Way.find(params[:id])
+
+    # TODO - maybe a bit heavyweight to do this on every
+    # call, perhaps try lazy auth.
+    setup_user_auth
     
     doc = OSM::API.new.get_xml_doc
     
     way.old_ways.each do |old_way|
-      doc.root << old_way.to_xml_node
+      unless old_way.redacted? and (@user.nil? or not @user.moderator?) and not params[:show_redactions] == "true"
+        doc.root << old_way.to_xml_node
+      end
     end
     
     render :text => doc.to_s, :content_type => "text/xml"
@@ -20,14 +28,31 @@ class OldWayController < ApplicationController
   
   def version
     if old_way = OldWay.where(:way_id => params[:id], :version => params[:version]).first
-      response.last_modified = old_way.timestamp
-
-      doc = OSM::API.new.get_xml_doc
-      doc.root << old_way.to_xml_node
+      # TODO - maybe a bit heavyweight to do this on every
+      # call, perhaps try lazy auth.
+      setup_user_auth
 
-      render :text => doc.to_s, :content_type => "text/xml"
+      if old_way.redacted? and (@user.nil? or not @user.moderator?) and not params[:show_redactions] == "true"
+        render :nothing => true, :status => :forbidden
+      else
+        response.last_modified = old_way.timestamp
+        
+        doc = OSM::API.new.get_xml_doc
+        doc.root << old_way.to_xml_node
+        
+        render :text => doc.to_s, :content_type => "text/xml"
+      end
     else
       render :nothing => true, :status => :not_found
     end
   end
+
+  def redact
+    if @user && @user.moderator?
+      render :nothing => true
+
+    else
+      render :nothing => true, :status => :forbidden
+    end
+  end
 end
diff --git a/app/models/old_way.rb b/app/models/old_way.rb
index 3df0c2d4f..474c3e02b 100644
--- a/app/models/old_way.rb
+++ b/app/models/old_way.rb
@@ -100,18 +100,26 @@ class OldWay < ActiveRecord::Base
     end
     el1['version'] = self.version.to_s
     el1['changeset'] = self.changeset.id.to_s
-    
-    self.old_nodes.each do |nd| # FIXME need to make sure they come back in the right order
-      e = XML::Node.new 'nd'
-      e['ref'] = nd.node_id.to_s
-      el1 << e
+
+    if self.redacted?
+      el1['redacted'] = self.redaction.title
     end
- 
-    self.old_tags.each do |tag|
-      e = XML::Node.new 'tag'
-      e['k'] = tag.k
-      e['v'] = tag.v
-      el1 << e
+    
+    unless self.redacted? and (@user.nil? or not @user.moderator?)
+      # If a way is redacted and the user isn't a moderator, only show
+      # meta-data from this revision, but no real data.
+      self.old_nodes.each do |nd| # FIXME need to make sure they come back in the right order
+        e = XML::Node.new 'nd'
+        e['ref'] = nd.node_id.to_s
+        el1 << e
+      end
+      
+      self.old_tags.each do |tag|
+        e = XML::Node.new 'tag'
+        e['k'] = tag.k
+        e['v'] = tag.v
+        el1 << e
+      end
     end
     return el1
   end
diff --git a/config/routes.rb b/config/routes.rb
index 46be25c97..c0a31b074 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -28,6 +28,7 @@ OpenStreetMap::Application.routes.draw do
   match 'api/0.6/way/:id/full' => 'way#full', :via => :get, :id => /\d+/
   match 'api/0.6/way/:id/relations' => 'relation#relations_for_way', :via => :get, :id => /\d+/
   match 'api/0.6/way/:id/:version' => 'old_way#version', :via => :get, :id => /\d+/, :version => /\d+/
+  match 'api/0.6/way/:id/:version/redact' => 'old_way#redact', :via => :put, :version => /\d+/, :id => /\d+/
   match 'api/0.6/way/:id' => 'way#read', :via => :get, :id => /\d+/
   match 'api/0.6/way/:id' => 'way#update', :via => :put, :id => /\d+/
   match 'api/0.6/way/:id' => 'way#delete', :via => :delete, :id => /\d+/
diff --git a/test/fixtures/current_ways.yml b/test/fixtures/current_ways.yml
index cd06d06d3..255be4a97 100644
--- a/test/fixtures/current_ways.yml
+++ b/test/fixtures/current_ways.yml
@@ -32,3 +32,10 @@ way_with_duplicate_nodes:
   timestamp: 2007-01-01 00:00:00
   visible: true
   version: 1
+
+way_with_redacted_versions:
+  id: 6
+  changeset_id: 4
+  timestamp: 2008-01-01 00:04:13
+  visible: true
+  version: 4
diff --git a/test/fixtures/way_nodes.yml b/test/fixtures/way_nodes.yml
index 19541d19a..262f29b31 100644
--- a/test/fixtures/way_nodes.yml
+++ b/test/fixtures/way_nodes.yml
@@ -75,3 +75,61 @@ w5_n2:
   node_id: 4
   sequence_id: 2
   version: 1
+
+w6_v1_n1:
+  way_id: 6
+  node_id: 3
+  sequence_id: 1
+  version: 1
+
+w6_v1_n2:
+  way_id: 6
+  node_id: 2
+  sequence_id: 2
+  version: 1
+
+w6_v2_n1:
+  way_id: 6
+  node_id: 3
+  sequence_id: 1
+  version: 2
+
+w6_v2_n2:
+  way_id: 6
+  node_id: 2
+  sequence_id: 2
+  version: 2
+
+w6_v3_n1:
+  way_id: 6
+  node_id: 3
+  sequence_id: 1
+  version: 3
+
+w6_v3_n2:
+  way_id: 6
+  node_id: 2
+  sequence_id: 2
+  version: 3
+
+w6_v4_n1:
+  way_id: 6
+  node_id: 3
+  sequence_id: 1
+  version: 4
+
+w6_v4_n2:
+  way_id: 6
+  node_id: 2
+  sequence_id: 2
+  version: 4
+
+w6_v4_n3:
+  way_id: 6
+  node_id: 4
+  sequence_id: 3
+  version: 4
+
+
+
+
diff --git a/test/fixtures/way_tags.yml b/test/fixtures/way_tags.yml
index dc727b5f5..6fb77d71b 100644
--- a/test/fixtures/way_tags.yml
+++ b/test/fixtures/way_tags.yml
@@ -15,3 +15,27 @@ t3:
   k: 'test'
   v: 'yes'
   version: 1
+
+t6_v1:
+  way_id: 6
+  k: 'test'
+  v: 'yes'
+  version: 1
+
+t6_v2:
+  way_id: 6
+  k: 'test'
+  v: 'yes'
+  version: 2
+
+t6_v3:
+  way_id: 6
+  k: 'test'
+  v: 'yes'
+  version: 3
+
+t6_v4:
+  way_id: 6
+  k: 'test'
+  v: 'yes'
+  version: 4
diff --git a/test/fixtures/ways.yml b/test/fixtures/ways.yml
index 95c001c30..56f382f88 100644
--- a/test/fixtures/ways.yml
+++ b/test/fixtures/ways.yml
@@ -53,3 +53,40 @@ way_with_duplicate_nodes:
   timestamp: 2007-01-01 00:00:00
   visible: true
   version: 1
+
+way_with_redacted_versions_v1:
+  way_id: 6
+  changeset_id: 4
+  timestamp: 2008-01-01 00:04:10
+  visible: true
+  version: 1
+
+way_with_redacted_versions_v1:
+  way_id: 6
+  changeset_id: 4
+  timestamp: 2008-01-01 00:04:10
+  visible: true
+  version: 1
+
+way_with_redacted_versions_v2:
+  way_id: 6
+  changeset_id: 4
+  timestamp: 2008-01-01 00:04:11
+  visible: true
+  version: 2
+  redaction_id: 1
+
+way_with_redacted_versions_v3:
+  way_id: 6
+  changeset_id: 4
+  timestamp: 2008-01-01 00:04:12
+  visible: true
+  version: 3
+  redaction_id: 1
+
+way_with_redacted_versions_v4:
+  way_id: 6
+  changeset_id: 4
+  timestamp: 2008-01-01 00:04:13
+  visible: true
+  version: 4
\ No newline at end of file
diff --git a/test/functional/old_way_controller_test.rb b/test/functional/old_way_controller_test.rb
index f0ab6bd85..e79d7b480 100644
--- a/test/functional/old_way_controller_test.rb
+++ b/test/functional/old_way_controller_test.rb
@@ -56,6 +56,112 @@ class OldWayControllerTest < ActionController::TestCase
     check_history_equals_versions(current_ways(:way_with_versions).id)
   end
 
+  ##
+  # test the redaction of an old version of a way, while not being
+  # authorised.
+  def test_redact_way_unauthorised
+    do_redact_way(ways(:way_with_versions),
+                   redactions(:example))
+    assert_response :unauthorized, "should need to be authenticated to redact."
+  end
+
+    ##
+  # test the redaction of an old version of a way, while being 
+  # authorised as a normal user.
+  def test_redact_way_normal_user
+    basic_authorization(users(:public_user).email, "test")
+
+    do_redact_way(ways(:way_with_versions),
+                   redactions(:example))
+    assert_response :forbidden, "should need to be moderator to redact."
+  end
+
+  ##
+  # test that, even as moderator, the current version of a way
+  # can't be redacted.
+  def test_redact_way_current_version
+    basic_authorization(users(:moderator_user).email, "test")
+
+    do_redact_way(ways(:way_with_versions_v4),
+                   redactions(:example))
+    assert_response :forbidden, "shouldn't be OK to redact current version as moderator."
+  end    
+
+  ##
+  # test that redacted ways aren't visible, regardless of 
+  # authorisation except as moderator...
+  def test_version_redacted
+    way = ways(:way_with_redacted_versions_v2)
+
+    get :version, :id => way.way_id, :version => way.version
+    assert_response :forbidden, "Redacted node shouldn't be visible via the version API."
+
+    # not even to a logged-in user
+    basic_authorization(users(:public_user).email, "test")
+    get :version, :id => way.way_id, :version => way.version
+    assert_response :forbidden, "Redacted node shouldn't be visible via the version API, even when logged in."
+  end
+
+  ##
+  # test that redacted nodes aren't visible in the history
+  def test_history_redacted
+    way = ways(:way_with_redacted_versions_v2)
+
+    get :history, :id => way.way_id
+    assert_response :success, "Redaction shouldn't have stopped history working."
+    assert_select "osm way[id=#{way.way_id}][version=#{way.version}]", 0, "redacted way #{way.way_id} version #{way.version} shouldn't be present in the history."
+
+    # not even to a logged-in user
+    basic_authorization(users(:public_user).email, "test")
+    get :version, :id => way.way_id, :version => way.version
+    get :history, :id => way.way_id
+    assert_response :success, "Redaction shouldn't have stopped history working."
+    assert_select "osm way[id=#{way.way_id}][version=#{way.version}]", 0, "redacted node #{way.way_id} version #{way.version} shouldn't be present in the history, even when logged in."
+  end
+
+  ##
+  # test the redaction of an old version of a way, while being 
+  # authorised as a moderator.
+  def test_redact_way_moderator
+    way = ways(:way_with_versions)
+    basic_authorization(users(:moderator_user).email, "test")
+
+    do_redact_way(way, redactions(:example))
+    assert_response :success, "should be OK to redact old version as moderator."
+
+    # check moderator can still see the redacted data
+    get :version, :id => way.way_id, :version => way.version
+    assert_response :success, "After redaction, node should not be gone for moderator."
+    
+    # and when accessed via history
+    get :history, :id => way.way_id
+    assert_response :success, "Redaction shouldn't have stopped history working."
+    assert_select "osm way[id=#{way.way_id}][version=#{way.version}]", 1, "way #{way.way_id} version #{way.version} should still be present in the history for moderators."
+  end
+
+  # testing that if the moderator drops auth, he can't see the
+  # redacted stuff any more.
+  def test_redact_way_is_redacted
+    way = ways(:way_with_versions)
+    basic_authorization(users(:moderator_user).email, "test")
+
+    do_redact_way(way, redactions(:example))
+    assert_response :success, "should be OK to redact old version as moderator."
+
+    # re-auth as non-moderator
+    basic_authorization(users(:public_user).email, "test")
+
+    # check can't see the redacted data
+    get :version, :id => way.way_id, :version => way.version
+    assert_response :forbidden, "Redacted node shouldn't be visible via the version API."
+    
+    # and when accessed via history
+    get :version, :id => way.node_id, :version => way.version
+    get :history, :id => way.node_id
+    assert_response :success, "Redaction shouldn't have stopped history working."
+    assert_select "osm way[id=#{way.way_id}][version=#{way.version}]", 0, "redacted way #{way.way_id} version #{way.version} shouldn't be present in the history."
+  end
+
   ##
   # check that the current version of a way is equivalent to the
   # version which we're getting from the versions call.
@@ -99,4 +205,12 @@ class OldWayControllerTest < ActionController::TestCase
     end
   end
 
+  def do_redact_way(way, redaction)
+    get :version, :id => way.way_id, :version => way.version
+    assert_response :success, "should be able to get version #{way.version} of node #{way.way_id}."
+    
+    # now redact it
+    post :redact, :id => way.way_id, :version => way.version, :redaction => redaction.id
+  end
+
 end
diff --git a/test/unit/old_way_tag_test.rb b/test/unit/old_way_tag_test.rb
index 334e7b86a..38023bb52 100644
--- a/test/unit/old_way_tag_test.rb
+++ b/test/unit/old_way_tag_test.rb
@@ -4,7 +4,7 @@ class WayTagTest < ActiveSupport::TestCase
   api_fixtures
   
   def test_tag_count
-    assert_equal 3, OldWayTag.count
+    assert_equal 7, OldWayTag.count
   end
   
   def test_length_key_valid
diff --git a/test/unit/way_test.rb b/test/unit/way_test.rb
index ca5b75176..d2a447e40 100644
--- a/test/unit/way_test.rb
+++ b/test/unit/way_test.rb
@@ -6,7 +6,7 @@ class WayTest < ActiveSupport::TestCase
   # Check that we have the correct number of currnet ways in the db
   # This will need to updated whenever the current_ways.yml is updated
   def test_db_count
-    assert_equal 5, Way.count
+    assert_equal 6, Way.count
   end
   
   def test_bbox