cst1/openstreetmap-website
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Remove require_terms_agreed configuration option

Browse source 
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.
This commit is contained in:
Andy Allan 2019-02-06 15:50:57 +01:00
parent 3795da4014
commit 35a2d66e19
8 changed files with 50 additions and 67 deletions
Download patch file Download diff file Expand all files Collapse all files

4
app/abilities/ability.rb
View file

@ -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

4
app/abilities/capability.rb
View file

@ -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)

10
app/controllers/amf_controller.rb
View file

@ -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 = {}

2
config/example.application.yml
View file

@ -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

80
test/controllers/changeset_comments_controller_test.rb
View file

@ -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)
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
# 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
# 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
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)
user = create(:user, :terms_agreed => nil)
changeset = create(:changeset, :closed)
basic_authorization user.email, "test"
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
# 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
assert_difference "ChangesetComment.count", 0 do
post :create, :params => { :id => changeset.id, :text => "This is a comment" }
end
end
assert_response :forbidden
private
# Try again, after agreement with the terms
user.terms_agreed = Time.now
user.save!
def with_terms_agreed(value)
require_terms_agreed = Object.send("remove_const", "REQUIRE_TERMS_AGREED")
Object.const_set("REQUIRE_TERMS_AGREED", value)
yield
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

10
test/controllers/users_controller_test.rb
View file

@ -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

1
test/factories/user.rb
View file

@ -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

6
test/integration/user_terms_seen_test.rb
View file

@ -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"