From: Tom Hughes <tom@compton.nu>
Date: Wed, 26 Oct 2016 14:48:56 +0000 (+0100)
Subject: Merge remote-tracking branch 'openstreetmap/pull/1338'
X-Git-Tag: live~3736
X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/8920a56133d24d328b774ef166a07949746e008a?hp=a280f72beead569784c6bf353131d10706dfab00

Merge remote-tracking branch 'openstreetmap/pull/1338'
---

diff --git a/app/controllers/changeset_controller.rb b/app/controllers/changeset_controller.rb
index 09bad34bc..da066a73a 100644
--- a/app/controllers/changeset_controller.rb
+++ b/app/controllers/changeset_controller.rb
@@ -83,7 +83,7 @@ class ChangesetController < ApplicationController
 
     # the request is in pseudo-osm format... this is kind-of an
     # abuse, maybe should change to some other format?
-    doc = XML::Parser.string(request.raw_post).parse
+    doc = XML::Parser.string(request.raw_post, :options => XML::Parser::Options::NOERROR).parse
     doc.find("//osm/node").each do |n|
       lon << n["lon"].to_f * GeoRecord::SCALE
       lat << n["lat"].to_f * GeoRecord::SCALE
diff --git a/app/controllers/user_preference_controller.rb b/app/controllers/user_preference_controller.rb
index 78ab45308..bf9bab213 100644
--- a/app/controllers/user_preference_controller.rb
+++ b/app/controllers/user_preference_controller.rb
@@ -39,7 +39,7 @@ class UserPreferenceController < ApplicationController
 
     new_preferences = {}
 
-    doc = XML::Parser.string(request.raw_post).parse
+    doc = XML::Parser.string(request.raw_post, :options => XML::Parser::Options::NOERROR).parse
 
     doc.find("//preferences/preference").each do |pt|
       if preference = old_preferences.delete(pt["k"])
diff --git a/app/models/acl.rb b/app/models/acl.rb
index 8bb4ae4b9..529ccbe3b 100644
--- a/app/models/acl.rb
+++ b/app/models/acl.rb
@@ -1,4 +1,6 @@
 class Acl < ActiveRecord::Base
+  validates :k, :presence => true
+
   def self.match(address, domain = nil)
     if domain
       Acl.where("address >>= ? OR domain = ?", address, domain)
diff --git a/app/models/node.rb b/app/models/node.rb
index 578735688..2a64259c1 100644
--- a/app/models/node.rb
+++ b/app/models/node.rb
@@ -49,7 +49,7 @@ class Node < ActiveRecord::Base
 
   # Read in xml as text and return it's Node object representation
   def self.from_xml(xml, create = false)
-    p = XML::Parser.string(xml)
+    p = XML::Parser.string(xml, :options => XML::Parser::Options::NOERROR)
     doc = p.parse
 
     doc.find("//osm/node").each do |pt|
diff --git a/app/models/relation.rb b/app/models/relation.rb
index 4c80be210..ed37d159a 100644
--- a/app/models/relation.rb
+++ b/app/models/relation.rb
@@ -36,7 +36,7 @@ class Relation < ActiveRecord::Base
   TYPES = %w(node way relation).freeze
 
   def self.from_xml(xml, create = false)
-    p = XML::Parser.string(xml)
+    p = XML::Parser.string(xml, :options => XML::Parser::Options::NOERROR)
     doc = p.parse
 
     doc.find("//osm/relation").each do |pt|
diff --git a/app/models/trace.rb b/app/models/trace.rb
index 85c0244d5..47e5c38b7 100644
--- a/app/models/trace.rb
+++ b/app/models/trace.rb
@@ -174,7 +174,7 @@ class Trace < ActiveRecord::Base
 
   # Read in xml as text and return it's Node object representation
   def self.from_xml(xml, create = false)
-    p = XML::Parser.string(xml)
+    p = XML::Parser.string(xml, :options => XML::Parser::Options::NOERROR)
     doc = p.parse
 
     doc.find("//osm/gpx_file").each do |pt|
diff --git a/app/models/way.rb b/app/models/way.rb
index ecbdd2f9c..98c4902f9 100644
--- a/app/models/way.rb
+++ b/app/models/way.rb
@@ -34,7 +34,7 @@ class Way < ActiveRecord::Base
 
   # Read in xml as text and return it's Way object representation
   def self.from_xml(xml, create = false)
-    p = XML::Parser.string(xml)
+    p = XML::Parser.string(xml, :options => XML::Parser::Options::NOERROR)
     doc = p.parse
 
     doc.find("//osm/way").each do |pt|
diff --git a/test/controllers/trace_controller_test.rb b/test/controllers/trace_controller_test.rb
index 2fec258af..5a5234cb5 100644
--- a/test/controllers/trace_controller_test.rb
+++ b/test/controllers/trace_controller_test.rb
@@ -1,7 +1,7 @@
 require "test_helper"
 
 class TraceControllerTest < ActionController::TestCase
-  fixtures :users, :user_preferences, :gpx_files
+  fixtures :users, :gpx_files
   set_fixture_class :gpx_files => Trace
 
   def setup
@@ -455,12 +455,14 @@ class TraceControllerTest < ActionController::TestCase
     assert_redirected_to :controller => :user, :action => :login, :referer => trace_create_path
 
     # Now authenticated as a user with gps.trace.visibility set
+    create(:user_preference, :user => users(:public_user), :k => "gps.trace.visibility", :v => "identifiable")
     get :create, {}, { :user => users(:public_user).id }
     assert_response :success
     assert_template :create
     assert_select "select#trace_visibility option[value=identifiable][selected]", 1
 
     # Now authenticated as a user with gps.trace.public set
+    create(:user_preference, :user => users(:second_public_user), :k => "gps.trace.public", :v => "default")
     get :create, {}, { :user => users(:second_public_user).id }
     assert_response :success
     assert_template :create
@@ -483,6 +485,7 @@ class TraceControllerTest < ActionController::TestCase
     assert_response :forbidden
 
     # Now authenticated
+    create(:user_preference, :user => users(:public_user), :k => "gps.trace.visibility", :v => "identifiable")
     assert_not_equal "trackable", users(:public_user).preferences.where(:k => "gps.trace.visibility").first.v
     post :create, { :trace => { :gpx_file => file, :description => "New Trace", :tagstring => "new,trace", :visibility => "trackable" } }, { :user => users(:public_user).id }
     assert_response :redirect
@@ -710,6 +713,7 @@ class TraceControllerTest < ActionController::TestCase
     assert_response :unauthorized
 
     # Now authenticated
+    create(:user_preference, :user => users(:public_user), :k => "gps.trace.visibility", :v => "identifiable")
     assert_not_equal "trackable", users(:public_user).preferences.where(:k => "gps.trace.visibility").first.v
     basic_authorization(users(:public_user).display_name, "test")
     post :api_create, :file => file, :description => "New Trace", :tags => "new,trace", :visibility => "trackable"
diff --git a/test/controllers/user_preference_controller_test.rb b/test/controllers/user_preference_controller_test.rb
index 752ec56cb..96ba4375c 100644
--- a/test/controllers/user_preference_controller_test.rb
+++ b/test/controllers/user_preference_controller_test.rb
@@ -1,7 +1,7 @@
 require "test_helper"
 
 class UserPreferenceControllerTest < ActionController::TestCase
-  fixtures :users, :user_preferences
+  fixtures :users
 
   ##
   # test all routes which lead to this controller
@@ -47,6 +47,8 @@ class UserPreferenceControllerTest < ActionController::TestCase
     end
 
     # authenticate as a user with preferences
+    user_preference = create(:user_preference, :user => users(:normal_user))
+    user_preference2 = create(:user_preference, :user => users(:normal_user))
     basic_authorization("test@openstreetmap.org", "test")
 
     # try the read again
@@ -56,8 +58,8 @@ class UserPreferenceControllerTest < ActionController::TestCase
     assert_select "osm" do
       assert_select "preferences", :count => 1 do
         assert_select "preference", :count => 2
-        assert_select "preference[k=\"#{user_preferences(:a).k}\"][v=\"#{user_preferences(:a).v}\"]", :count => 1
-        assert_select "preference[k=\"#{user_preferences(:two).k}\"][v=\"#{user_preferences(:two).v}\"]", :count => 1
+        assert_select "preference[k=\"#{user_preference.k}\"][v=\"#{user_preference.v}\"]", :count => 1
+        assert_select "preference[k=\"#{user_preference2.k}\"][v=\"#{user_preference2.v}\"]", :count => 1
       end
     end
   end
@@ -65,6 +67,8 @@ class UserPreferenceControllerTest < ActionController::TestCase
   ##
   # test read_one action
   def test_read_one
+    create(:user_preference, :user => users(:normal_user), :k => "key", :v => "value")
+
     # try a read without auth
     get :read_one, :preference_key => "key"
     assert_response :unauthorized, "should be authenticated"
@@ -86,16 +90,19 @@ class UserPreferenceControllerTest < ActionController::TestCase
   ##
   # test update action
   def test_update
+    create(:user_preference, :user => users(:normal_user), :k => "key", :v => "value")
+    create(:user_preference, :user => users(:normal_user), :k => "some_key", :v => "some_value")
+
     # try a put without auth
     assert_no_difference "UserPreference.count" do
       content "<osm><preferences><preference k='key' v='new_value'/><preference k='new_key' v='value'/></preferences></osm>"
       put :update
     end
     assert_response :unauthorized, "should be authenticated"
-    assert_equal "value", UserPreference.find([1, "key"]).v
-    assert_equal "some_value", UserPreference.find([1, "some_key"]).v
+    assert_equal "value", UserPreference.find([users(:normal_user).id, "key"]).v
+    assert_equal "some_value", UserPreference.find([users(:normal_user).id, "some_key"]).v
     assert_raises ActiveRecord::RecordNotFound do
-      UserPreference.find([1, "new_key"])
+      UserPreference.find([users(:normal_user).id, "new_key"])
     end
 
     # authenticate as a user with preferences
@@ -109,10 +116,10 @@ class UserPreferenceControllerTest < ActionController::TestCase
     assert_response :success
     assert_equal "text/plain", @response.content_type
     assert_equal "", @response.body
-    assert_equal "new_value", UserPreference.find([1, "key"]).v
-    assert_equal "value", UserPreference.find([1, "new_key"]).v
+    assert_equal "new_value", UserPreference.find([users(:normal_user).id, "key"]).v
+    assert_equal "value", UserPreference.find([users(:normal_user).id, "new_key"]).v
     assert_raises ActiveRecord::RecordNotFound do
-      UserPreference.find([1, "some_key"])
+      UserPreference.find([users(:normal_user).id, "some_key"])
     end
 
     # try a put with duplicate keys
@@ -123,7 +130,7 @@ class UserPreferenceControllerTest < ActionController::TestCase
     assert_response :bad_request
     assert_equal "text/plain", @response.content_type
     assert_equal "Duplicate preferences with key key", @response.body
-    assert_equal "new_value", UserPreference.find([1, "key"]).v
+    assert_equal "new_value", UserPreference.find([users(:normal_user).id, "key"]).v
 
     # try a put with invalid content
     assert_no_difference "UserPreference.count" do
@@ -143,7 +150,7 @@ class UserPreferenceControllerTest < ActionController::TestCase
     end
     assert_response :unauthorized, "should be authenticated"
     assert_raises ActiveRecord::RecordNotFound do
-      UserPreference.find([1, "new_key"])
+      UserPreference.find([users(:normal_user).id, "new_key"])
     end
 
     # authenticate as a user with preferences
@@ -157,7 +164,7 @@ class UserPreferenceControllerTest < ActionController::TestCase
     assert_response :success
     assert_equal "text/plain", @response.content_type
     assert_equal "", @response.body
-    assert_equal "new_value", UserPreference.find([1, "new_key"]).v
+    assert_equal "new_value", UserPreference.find([users(:normal_user).id, "new_key"]).v
 
     # try changing the value of a preference
     assert_no_difference "UserPreference.count" do
@@ -167,18 +174,20 @@ class UserPreferenceControllerTest < ActionController::TestCase
     assert_response :success
     assert_equal "text/plain", @response.content_type
     assert_equal "", @response.body
-    assert_equal "newer_value", UserPreference.find([1, "new_key"]).v
+    assert_equal "newer_value", UserPreference.find([users(:normal_user).id, "new_key"]).v
   end
 
   ##
   # test delete_one action
   def test_delete_one
+    create(:user_preference, :user => users(:normal_user), :k => "key", :v => "value")
+
     # try a delete without auth
     assert_no_difference "UserPreference.count" do
       delete :delete_one, :preference_key => "key"
     end
     assert_response :unauthorized, "should be authenticated"
-    assert_equal "value", UserPreference.find([1, "key"]).v
+    assert_equal "value", UserPreference.find([users(:normal_user).id, "key"]).v
 
     # authenticate as a user with preferences
     basic_authorization("test@openstreetmap.org", "test")
@@ -191,7 +200,7 @@ class UserPreferenceControllerTest < ActionController::TestCase
     assert_equal "text/plain", @response.content_type
     assert_equal "", @response.body
     assert_raises ActiveRecord::RecordNotFound do
-      UserPreference.find([1, "key"])
+      UserPreference.find([users(:normal_user).id, "key"])
     end
 
     # try the delete again for the same key
@@ -200,7 +209,7 @@ class UserPreferenceControllerTest < ActionController::TestCase
     end
     assert_response :not_found
     assert_raises ActiveRecord::RecordNotFound do
-      UserPreference.find([1, "key"])
+      UserPreference.find([users(:normal_user).id, "key"])
     end
   end
 end
diff --git a/test/factories/acls.rb b/test/factories/acls.rb
new file mode 100644
index 000000000..3f91df3ab
--- /dev/null
+++ b/test/factories/acls.rb
@@ -0,0 +1,5 @@
+FactoryGirl.define do
+  factory :acl do
+    sequence(:k) { |n| "Key #{n}" }
+  end
+end
diff --git a/test/factories/user_preferences.rb b/test/factories/user_preferences.rb
new file mode 100644
index 000000000..552cfcd75
--- /dev/null
+++ b/test/factories/user_preferences.rb
@@ -0,0 +1,9 @@
+FactoryGirl.define do
+  factory :user_preference do
+    sequence(:k) { |n| "Key #{n}" }
+    sequence(:v) { |n| "Value #{n}" }
+
+    # FIXME: needs user factory
+    user_id 1
+  end
+end
diff --git a/test/fixtures/acls.yml b/test/fixtures/acls.yml
deleted file mode 100644
index 399e08808..000000000
--- a/test/fixtures/acls.yml
+++ /dev/null
@@ -1,13 +0,0 @@
-# Read about fixtures at http://ar.rubyonrails.org/classes/Fixtures.html
-
-one:
-  address: 1
-  netmask: 1
-  k: MyText
-  v: MyText
-
-two:
-  address: 1
-  netmask: 1
-  k: MyText
-  v: MyText
diff --git a/test/fixtures/user_preferences.yml b/test/fixtures/user_preferences.yml
deleted file mode 100644
index 03065a9eb..000000000
--- a/test/fixtures/user_preferences.yml
+++ /dev/null
@@ -1,21 +0,0 @@
-# Read about fixtures at http://ar.rubyonrails.org/classes/Fixtures.html
-
-a:
-  user_id: 1
-  k: "key"
-  v: "value"
-
-two:
-  user_id: 1
-  k: "some_key"
-  v: "some_value"
-
-u2_trace:
-  user_id: 2
-  k: "gps.trace.visibility"
-  v: "identifiable"
-
-u3_trace:
-  user_id: 4
-  k: "gps.trace.public"
-  v: "default"
diff --git a/test/models/acl_test.rb b/test/models/acl_test.rb
index 4022b8adb..189f92677 100644
--- a/test/models/acl_test.rb
+++ b/test/models/acl_test.rb
@@ -1,8 +1,22 @@
 require "test_helper"
 
 class AclTest < ActiveSupport::TestCase
-  # Replace this with your real tests.
-  def test_truth
-    assert true
+  def test_k_required
+    acl = create(:acl)
+    assert acl.valid?
+    acl.k = nil
+    assert !acl.valid?
+  end
+
+  def test_no_account_creation_by_subnet
+    assert !Acl.no_account_creation("192.168.1.1")
+    create(:acl, :address => "192.168.0.0/16", :k => "no_account_creation")
+    assert Acl.no_account_creation("192.168.1.1")
+  end
+
+  def test_no_account_creation_by_domain
+    assert !Acl.no_account_creation("192.168.1.1", "example.com")
+    create(:acl, :domain => "example.com", :k => "no_account_creation")
+    assert Acl.no_account_creation("192.168.1.1", "example.com")
   end
 end
diff --git a/test/models/user_preference_test.rb b/test/models/user_preference_test.rb
index d3400bc9d..c61bd607d 100644
--- a/test/models/user_preference_test.rb
+++ b/test/models/user_preference_test.rb
@@ -2,19 +2,10 @@ require "test_helper"
 
 class UserPreferenceTest < ActiveSupport::TestCase
   api_fixtures
-  fixtures :user_preferences
-
-  # This checks to make sure that there are two user preferences
-  # stored in the test database.
-  # This test needs to be updated for every addition/deletion from
-  # the fixture file
-  def test_check_count
-    assert_equal 4, UserPreference.count
-  end
 
   # Checks that you cannot add a new preference, that is a duplicate
   def test_add_duplicate_preference
-    up = user_preferences(:a)
+    up = create(:user_preference)
     new_up = UserPreference.new
     new_up.user = users(:normal_user)
     new_up.k = up.k