From: Tom Hughes <tom@compton.nu>
Date: Wed, 6 Feb 2019 18:30:41 +0000 (+0000)
Subject: Merge remote-tracking branch 'upstream/pull/2136'
X-Git-Tag: live~2716
X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/908324323e8801ff76a7cde812e61dbbb03a33ce?hp=77dcaef44c19d43a3944affbfcf2d037dc11aba5

Merge remote-tracking branch 'upstream/pull/2136'
---

diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb
index c4cca7cc5..4127c0de7 100644
--- a/app/abilities/ability.rb
+++ b/app/abilities/ability.rb
@@ -42,7 +42,7 @@ class Ability
       can [:account, :go_public, :make_friend, :remove_friend, :api_details, :api_gpx_files], User
       can [:read, :read_one, :update, :update_one, :delete_one], UserPreference
 
-      if user.terms_agreed? || !REQUIRE_TERMS_AGREED
+      if user.terms_agreed?
         can [:create, :update, :upload, :close, :subscribe, :unsubscribe, :expand_bbox], Changeset
         can :create, ChangesetComment
         can [:create, :update, :delete], Node
@@ -57,7 +57,7 @@ class Ability
         can :destroy, Note
         can [:new, :create, :edit, :update, :destroy], Redaction
         can [:new, :edit, :create, :update, :revoke], UserBlock
-        if user.terms_agreed? || !REQUIRE_TERMS_AGREED
+        if user.terms_agreed?
           can :redact, OldNode
           can :redact, OldWay
           can :redact, OldRelation
diff --git a/app/abilities/capability.rb b/app/abilities/capability.rb
index 3d951900b..d8f51eefe 100644
--- a/app/abilities/capability.rb
+++ b/app/abilities/capability.rb
@@ -12,7 +12,7 @@ class Capability
     can [:read, :read_one], UserPreference if capability?(token, :allow_read_prefs)
     can [:update, :update_one, :delete_one], UserPreference if capability?(token, :allow_write_prefs)
 
-    if token&.user&.terms_agreed? || !REQUIRE_TERMS_AGREED
+    if token&.user&.terms_agreed?
       can [:create, :update, :upload, :close, :subscribe, :unsubscribe, :expand_bbox], Changeset if capability?(token, :allow_write_api)
       can :create, ChangesetComment if capability?(token, :allow_write_api)
       can [:create, :update, :delete], Node if capability?(token, :allow_write_api)
@@ -23,7 +23,7 @@ class Capability
     if token&.user&.moderator?
       can [:destroy, :restore], ChangesetComment if capability?(token, :allow_write_api)
       can :destroy, Note if capability?(token, :allow_write_notes)
-      if token&.user&.terms_agreed? || !REQUIRE_TERMS_AGREED
+      if token&.user&.terms_agreed?
         can :redact, OldNode if capability?(token, :allow_write_api)
         can :redact, OldWay if capability?(token, :allow_write_api)
         can :redact, OldRelation if capability?(token, :allow_write_api)
diff --git a/app/controllers/amf_controller.rb b/app/controllers/amf_controller.rb
index fdad432a8..2ad0fe6e0 100644
--- a/app/controllers/amf_controller.rb
+++ b/app/controllers/amf_controller.rb
@@ -144,7 +144,7 @@ class AmfController < ApplicationController
       user = getuser(usertoken)
       return -1, "You are not logged in, so Potlatch can't write any changes to the database." unless user
       return -1, t("application.setup_user_auth.blocked") if user.blocks.active.exists?
-      return -1, "You must accept the contributor terms before you can edit." if REQUIRE_TERMS_AGREED && user.terms_agreed.nil?
+      return -1, "You must accept the contributor terms before you can edit." if user.terms_agreed.nil?
 
       if cstags
         return -1, "One of the tags is invalid. Linux users may need to upgrade to Flash Player 10.1." unless tags_ok(cstags)
@@ -537,7 +537,7 @@ class AmfController < ApplicationController
 
       return -1, "You are not logged in, so the relation could not be saved." unless user
       return -1, t("application.setup_user_auth.blocked") if user.blocks.active.exists?
-      return -1, "You must accept the contributor terms before you can edit." if REQUIRE_TERMS_AGREED && user.terms_agreed.nil?
+      return -1, "You must accept the contributor terms before you can edit." if user.terms_agreed.nil?
 
       return -1, "One of the tags is invalid. Linux users may need to upgrade to Flash Player 10.1." unless tags_ok(tags)
 
@@ -625,7 +625,7 @@ class AmfController < ApplicationController
       user = getuser(usertoken)
       return -1, "You are not logged in, so the way could not be saved." unless user
       return -1, t("application.setup_user_auth.blocked") if user.blocks.active.exists?
-      return -1, "You must accept the contributor terms before you can edit." if REQUIRE_TERMS_AGREED && user.terms_agreed.nil?
+      return -1, "You must accept the contributor terms before you can edit." if user.terms_agreed.nil?
 
       return -2, "Server error - way is only #{pointlist.length} points long." if pointlist.length < 2
 
@@ -735,7 +735,7 @@ class AmfController < ApplicationController
       user = getuser(usertoken)
       return -1, "You are not logged in, so the point could not be saved." unless user
       return -1, t("application.setup_user_auth.blocked") if user.blocks.active.exists?
-      return -1, "You must accept the contributor terms before you can edit." if REQUIRE_TERMS_AGREED && user.terms_agreed.nil?
+      return -1, "You must accept the contributor terms before you can edit." if user.terms_agreed.nil?
 
       return -1, "One of the tags is invalid. Linux users may need to upgrade to Flash Player 10.1." unless tags_ok(tags)
 
@@ -822,7 +822,7 @@ class AmfController < ApplicationController
       user = getuser(usertoken)
       return -1, "You are not logged in, so the way could not be deleted." unless user
       return -1, t("application.setup_user_auth.blocked") if user.blocks.active.exists?
-      return -1, "You must accept the contributor terms before you can edit." if REQUIRE_TERMS_AGREED && user.terms_agreed.nil?
+      return -1, "You must accept the contributor terms before you can edit." if user.terms_agreed.nil?
 
       way_id = way_id.to_i
       nodeversions = {}
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 9666adf12..227e5198f 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -105,7 +105,7 @@ class ApplicationController < ActionController::Base
       # if the user hasn't seen the contributor terms then don't
       # allow editing - they have to go to the web site and see
       # (but can decline) the CTs to continue.
-      if REQUIRE_TERMS_SEEN && !current_user.terms_seen && flash[:skip_terms].nil?
+      if !current_user.terms_seen && flash[:skip_terms].nil?
         set_locale
         report_error t("application.setup_user_auth.need_to_see_terms"), :forbidden
       end
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 01eee61df..d1a94b9f6 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -644,7 +644,7 @@ class UsersController < ApplicationController
     # - If they have a block on them, show them that.
     # - If they were referred to the login, send them back there.
     # - Otherwise, send them to the home page.
-    if REQUIRE_TERMS_SEEN && !user.terms_seen
+    if !user.terms_seen
       redirect_to :action => :terms, :referer => target
     elsif user.blocked_on_view
       redirect_to user.blocked_on_view, :referer => target
diff --git a/config/example.application.yml b/config/example.application.yml
index 7db7861b2..8596e10cc 100644
--- a/config/example.application.yml
+++ b/config/example.application.yml
@@ -87,10 +87,6 @@ defaults: &defaults
   #oauth_key: ""
   # OAuth consumer key for iD
   #id_key: ""
-  # Whether to require users to view the CTs before continuing to edit...
-  require_terms_seen: false
-  # Whether to require users to agree to the CTs before editing
-  require_terms_agreed: false
   # Imagery to return in capabilities as blacklisted
   imagery_blacklist:
     # Current Google imagery URLs have google or googleapis in the domain
diff --git a/test/controllers/changeset_comments_controller_test.rb b/test/controllers/changeset_comments_controller_test.rb
index 2b661a7a9..33ee1deb5 100644
--- a/test/controllers/changeset_comments_controller_test.rb
+++ b/test/controllers/changeset_comments_controller_test.rb
@@ -255,66 +255,50 @@ class ChangesetCommentsControllerTest < ActionController::TestCase
   # create method is simply a stand-in for any method that requires terms agreement.
   # But writing oauth tests is hard, and so it's easier to put in a controller test.)
   def test_api_write_and_terms_agreed_via_token
-    with_terms_agreed(true) do
-      user = create(:user, :terms_agreed => nil)
-      token = create(:access_token, :user => user, :allow_write_api => true)
-      changeset = create(:changeset, :closed)
-
-      # Hack together an oauth request - an alternative would be to sign the request properly
-      @request.env["oauth.version"] = 1
-      @request.env["oauth.strategies"] = [:token]
-      @request.env["oauth.token"] = token
-
-      assert_difference "ChangesetComment.count", 0 do
-        post :create, :params => { :id => changeset.id, :text => "This is a comment" }
-      end
-      assert_response :forbidden
+    user = create(:user, :terms_agreed => nil)
+    token = create(:access_token, :user => user, :allow_write_api => true)
+    changeset = create(:changeset, :closed)
 
-      # Try again, after agreement with the terms
-      user.terms_agreed = Time.now
-      user.save!
+    # Hack together an oauth request - an alternative would be to sign the request properly
+    @request.env["oauth.version"] = 1
+    @request.env["oauth.strategies"] = [:token]
+    @request.env["oauth.token"] = token
 
-      assert_difference "ChangesetComment.count", 1 do
-        post :create, :params => { :id => changeset.id, :text => "This is a comment" }
-      end
-      assert_response :success
+    assert_difference "ChangesetComment.count", 0 do
+      post :create, :params => { :id => changeset.id, :text => "This is a comment" }
     end
+    assert_response :forbidden
+
+    # Try again, after agreement with the terms
+    user.terms_agreed = Time.now
+    user.save!
+
+    assert_difference "ChangesetComment.count", 1 do
+      post :create, :params => { :id => changeset.id, :text => "This is a comment" }
+    end
+    assert_response :success
   end
 
   # This test does the same as above, but with basic auth, to similarly test that the
   # abilities take into account terms agreement too.
   def test_api_write_and_terms_agreed_via_basic_auth
-    with_terms_agreed(true) do
-      user = create(:user, :terms_agreed => nil)
-      changeset = create(:changeset, :closed)
-
-      basic_authorization user.email, "test"
-
-      assert_difference "ChangesetComment.count", 0 do
-        post :create, :params => { :id => changeset.id, :text => "This is a comment" }
-      end
-      assert_response :forbidden
+    user = create(:user, :terms_agreed => nil)
+    changeset = create(:changeset, :closed)
 
-      # Try again, after agreement with the terms
-      user.terms_agreed = Time.now
-      user.save!
+    basic_authorization user.email, "test"
 
-      assert_difference "ChangesetComment.count", 1 do
-        post :create, :params => { :id => changeset.id, :text => "This is a comment" }
-      end
-      assert_response :success
+    assert_difference "ChangesetComment.count", 0 do
+      post :create, :params => { :id => changeset.id, :text => "This is a comment" }
     end
-  end
-
-  private
-
-  def with_terms_agreed(value)
-    require_terms_agreed = Object.send("remove_const", "REQUIRE_TERMS_AGREED")
-    Object.const_set("REQUIRE_TERMS_AGREED", value)
+    assert_response :forbidden
 
-    yield
+    # Try again, after agreement with the terms
+    user.terms_agreed = Time.now
+    user.save!
 
-    Object.send("remove_const", "REQUIRE_TERMS_AGREED")
-    Object.const_set("REQUIRE_TERMS_AGREED", require_terms_agreed)
+    assert_difference "ChangesetComment.count", 1 do
+      post :create, :params => { :id => changeset.id, :text => "This is a comment" }
+    end
+    assert_response :success
   end
 end
diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb
index a1fda5591..b85dcf65b 100644
--- a/test/controllers/users_controller_test.rb
+++ b/test/controllers/users_controller_test.rb
@@ -646,7 +646,7 @@ class UsersControllerTest < ActionController::TestCase
   end
 
   def test_terms_not_seen_without_referer
-    user = create(:user, :terms_seen => false)
+    user = create(:user, :terms_seen => false, :terms_agreed => nil)
 
     session[:user] = user.id
 
@@ -667,7 +667,7 @@ class UsersControllerTest < ActionController::TestCase
   end
 
   def test_terms_not_seen_with_referer
-    user = create(:user, :terms_seen => false)
+    user = create(:user, :terms_seen => false, :terms_agreed => nil)
 
     session[:user] = user.id
 
@@ -690,7 +690,7 @@ class UsersControllerTest < ActionController::TestCase
   # Check that if you haven't seen the terms, and make a request that requires authentication,
   # that your request is redirected to view the terms
   def test_terms_not_seen_redirection
-    user = create(:user, :terms_seen => false)
+    user = create(:user, :terms_seen => false, :terms_agreed => nil)
     session[:user] = user.id
 
     get :account, :params => { :display_name => user.display_name }
@@ -1098,8 +1098,8 @@ class UsersControllerTest < ActionController::TestCase
   # Test whether information about contributor terms is shown for users who haven't agreed
   def test_terms_not_agreed
     agreed_user = create(:user, :terms_agreed => 3.days.ago)
-    seen_user = create(:user, :terms_seen => true)
-    not_seen_user = create(:user, :terms_seen => false)
+    seen_user = create(:user, :terms_seen => true, :terms_agreed => nil)
+    not_seen_user = create(:user, :terms_seen => false, :terms_agreed => nil)
 
     get :show, :params => { :display_name => agreed_user.display_name }
     assert_response :success
diff --git a/test/factories/user.rb b/test/factories/user.rb
index 69d5ba376..48a124adb 100644
--- a/test/factories/user.rb
+++ b/test/factories/user.rb
@@ -8,6 +8,7 @@ FactoryBot.define do
     # a 'normal' user who can log in without being redirected etc.
     status { "active" }
     terms_seen { true }
+    terms_agreed { Time.now.getutc }
     data_public { true }
 
     trait :with_home_location do
diff --git a/test/integration/user_terms_seen_test.rb b/test/integration/user_terms_seen_test.rb
index 03e8c6f54..416d226cc 100644
--- a/test/integration/user_terms_seen_test.rb
+++ b/test/integration/user_terms_seen_test.rb
@@ -6,69 +6,63 @@ class UserTermsSeenTest < ActionDispatch::IntegrationTest
   end
 
   def test_api_blocked
-    with_terms_seen(true) do
-      user = create(:user, :terms_seen => false)
+    user = create(:user, :terms_seen => false, :terms_agreed => nil)
 
-      get "/api/#{API_VERSION}/user/preferences", :headers => auth_header(user.display_name, "test")
-      assert_response :forbidden
+    get "/api/#{API_VERSION}/user/preferences", :headers => auth_header(user.display_name, "test")
+    assert_response :forbidden
 
-      # touch it so that the user has seen the terms
-      user.terms_seen = true
-      user.save
+    # touch it so that the user has seen the terms
+    user.terms_seen = true
+    user.save
 
-      get "/api/#{API_VERSION}/user/preferences", :headers => auth_header(user.display_name, "test")
-      assert_response :success
-    end
+    get "/api/#{API_VERSION}/user/preferences", :headers => auth_header(user.display_name, "test")
+    assert_response :success
   end
 
   def test_terms_presented_at_login
-    with_terms_seen(true) do
-      user = create(:user, :terms_seen => false)
-
-      # try to log in
-      get "/login"
-      follow_redirect!
-      assert_response :success
-      assert_template "users/login"
-      post "/login", :params => { :username => user.email, :password => "test", :referer => "/diary/new" }
-      assert_response :redirect
-      # but now we need to look at the terms
-      assert_redirected_to :controller => :users, :action => :terms, :referer => "/diary/new"
-      follow_redirect!
-      assert_response :success
-
-      # don't agree to the terms, but hit decline
-      post "/user/save", :params => { :decline => true, :referer => "/diary/new" }
-      assert_redirected_to "/diary/new"
-      follow_redirect!
-
-      # should be carried through to a normal login with a message
-      assert_response :success
-      assert_not flash[:notice].nil?
-    end
+    user = create(:user, :terms_seen => false, :terms_agreed => nil)
+
+    # try to log in
+    get "/login"
+    follow_redirect!
+    assert_response :success
+    assert_template "users/login"
+    post "/login", :params => { :username => user.email, :password => "test", :referer => "/diary/new" }
+    assert_response :redirect
+    # but now we need to look at the terms
+    assert_redirected_to :controller => :users, :action => :terms, :referer => "/diary/new"
+    follow_redirect!
+    assert_response :success
+
+    # don't agree to the terms, but hit decline
+    post "/user/save", :params => { :decline => true, :referer => "/diary/new" }
+    assert_redirected_to "/diary/new"
+    follow_redirect!
+
+    # should be carried through to a normal login with a message
+    assert_response :success
+    assert_not flash[:notice].nil?
   end
 
   def test_terms_cant_be_circumvented
-    with_terms_seen(true) do
-      user = create(:user, :terms_seen => false)
-
-      # try to log in
-      get "/login"
-      follow_redirect!
-      assert_response :success
-      assert_template "users/login"
-      post "/login", :params => { :username => user.email, :password => "test", :referer => "/diary/new" }
-      assert_response :redirect
-      # but now we need to look at the terms
-      assert_redirected_to :controller => :users, :action => :terms, :referer => "/diary/new"
-
-      # check that if we go somewhere else now, it redirects
-      # back to the terms page.
-      get "/traces/mine"
-      assert_redirected_to :controller => :users, :action => :terms, :referer => "/traces/mine"
-      get "/traces/mine", :params => { :referer => "/diary/new" }
-      assert_redirected_to :controller => :users, :action => :terms, :referer => "/diary/new"
-    end
+    user = create(:user, :terms_seen => false, :terms_agreed => nil)
+
+    # try to log in
+    get "/login"
+    follow_redirect!
+    assert_response :success
+    assert_template "users/login"
+    post "/login", :params => { :username => user.email, :password => "test", :referer => "/diary/new" }
+    assert_response :redirect
+    # but now we need to look at the terms
+    assert_redirected_to :controller => :users, :action => :terms, :referer => "/diary/new"
+
+    # check that if we go somewhere else now, it redirects
+    # back to the terms page.
+    get "/traces/mine"
+    assert_redirected_to :controller => :users, :action => :terms, :referer => "/traces/mine"
+    get "/traces/mine", :params => { :referer => "/diary/new" }
+    assert_redirected_to :controller => :users, :action => :terms, :referer => "/diary/new"
   end
 
   private
@@ -76,14 +70,4 @@ class UserTermsSeenTest < ActionDispatch::IntegrationTest
   def auth_header(user, pass)
     { "HTTP_AUTHORIZATION" => format("Basic %{auth}", :auth => Base64.encode64("#{user}:#{pass}")) }
   end
-
-  def with_terms_seen(value)
-    require_terms_seen = Object.send("remove_const", "REQUIRE_TERMS_SEEN")
-    Object.const_set("REQUIRE_TERMS_SEEN", value)
-
-    yield
-
-    Object.send("remove_const", "REQUIRE_TERMS_SEEN")
-    Object.const_set("REQUIRE_TERMS_SEEN", require_terms_seen)
-  end
 end