From 35a2d66e1960b00e5ed0cecbe4263cdeedb1ad62 Mon Sep 17 00:00:00 2001 From: Andy Allan Date: Wed, 6 Feb 2019 15:50:57 +0100 Subject: [PATCH 1/1] Remove require_terms_agreed configuration option This has been set to true for 6 years in production. Refs #2097 As per other user settings, we set the terms as seen by default for tests, and we can override that when necessary for specific tests. --- app/abilities/ability.rb | 4 +- app/abilities/capability.rb | 4 +- app/controllers/amf_controller.rb | 10 +-- config/example.application.yml | 2 - .../changeset_comments_controller_test.rb | 80 ++++++++----------- test/controllers/users_controller_test.rb | 10 +-- test/factories/user.rb | 1 + test/integration/user_terms_seen_test.rb | 6 +- 8 files changed, 50 insertions(+), 67 deletions(-) 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/config/example.application.yml b/config/example.application.yml index bf9ebeb59..8596e10cc 100644 --- a/config/example.application.yml +++ b/config/example.application.yml @@ -87,8 +87,6 @@ defaults: &defaults #oauth_key: "" # OAuth consumer key for iD #id_key: "" - # 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 33a6950ee..416d226cc 100644 --- a/test/integration/user_terms_seen_test.rb +++ b/test/integration/user_terms_seen_test.rb @@ -6,7 +6,7 @@ class UserTermsSeenTest < ActionDispatch::IntegrationTest end def test_api_blocked - 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 @@ -20,7 +20,7 @@ class UserTermsSeenTest < ActionDispatch::IntegrationTest end def test_terms_presented_at_login - user = create(:user, :terms_seen => false) + user = create(:user, :terms_seen => false, :terms_agreed => nil) # try to log in get "/login" @@ -45,7 +45,7 @@ class UserTermsSeenTest < ActionDispatch::IntegrationTest end def test_terms_cant_be_circumvented - user = create(:user, :terms_seen => false) + user = create(:user, :terms_seen => false, :terms_agreed => nil) # try to log in get "/login" -- 2.43.2