From 5c795bc48004d1fe7b3a4662ec391ca6cec815c1 Mon Sep 17 00:00:00 2001 From: Maja Komel Date: Wed, 17 Apr 2019 10:05:02 +0200 Subject: [PATCH] FIX: prevent anonymous users from changing their email/username/name (#7311) --- .../controllers/preferences/account.js.es6 | 28 ++++++++++----- lib/guardian.rb | 7 ++++ lib/guardian/user_guardian.rb | 11 +++--- spec/components/guardian_spec.rb | 35 +++++++++++++++++++ .../preferences-account-test.js.es6 | 9 +++-- 5 files changed, 76 insertions(+), 14 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/preferences/account.js.es6 b/app/assets/javascripts/discourse/controllers/preferences/account.js.es6 index b3c09ef6b95..d97c7202f3b 100644 --- a/app/assets/javascripts/discourse/controllers/preferences/account.js.es6 +++ b/app/assets/javascripts/discourse/controllers/preferences/account.js.es6 @@ -55,11 +55,15 @@ export default Ember.Controller.extend( return availableTitles.length > 0; }, - @computed() - canChangePassword() { - return ( - !this.siteSettings.enable_sso && this.siteSettings.enable_local_logins - ); + @computed("model.is_anonymous") + canChangePassword(isAnonymous) { + if (isAnonymous) { + return false; + } else { + return ( + !this.siteSettings.enable_sso && this.siteSettings.enable_local_logins + ); + } }, @computed("model.associated_accounts") @@ -92,9 +96,17 @@ export default Ember.Controller.extend( return userId !== this.get("currentUser.id"); }, - @computed("model.second_factor_enabled", "canCheckEmails") - canUpdateAssociatedAccounts(secondFactorEnabled, canCheckEmails) { - if (secondFactorEnabled || !canCheckEmails) { + @computed( + "model.second_factor_enabled", + "canCheckEmails", + "model.is_anonymous" + ) + canUpdateAssociatedAccounts( + secondFactorEnabled, + canCheckEmails, + isAnonymous + ) { + if (secondFactorEnabled || !canCheckEmails || isAnonymous) { return false; } diff --git a/lib/guardian.rb b/lib/guardian.rb index fa24d8426be..7bbb3bd44a2 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -31,6 +31,9 @@ class Guardian def moderator? false end + def anonymous? + true + end def approved? false end @@ -107,6 +110,10 @@ class Guardian @user.staged? end + def is_anonymous? + @user.anonymous? + end + # Can the user see the object? def can_see?(obj) if obj diff --git a/lib/guardian/user_guardian.rb b/lib/guardian/user_guardian.rb index 4bfbc094f1a..75f130eb310 100644 --- a/lib/guardian/user_guardian.rb +++ b/lib/guardian/user_guardian.rb @@ -17,23 +17,26 @@ module UserGuardian end def can_edit_username?(user) - return false if (SiteSetting.sso_overrides_username? && SiteSetting.enable_sso?) + return false if SiteSetting.sso_overrides_username? && SiteSetting.enable_sso? return true if is_staff? return false if SiteSetting.username_change_period <= 0 + return false if is_anonymous? is_me?(user) && ((user.post_count + user.topic_count) == 0 || user.created_at > SiteSetting.username_change_period.days.ago) end def can_edit_email?(user) - return false if (SiteSetting.sso_overrides_email? && SiteSetting.enable_sso?) + return false if SiteSetting.sso_overrides_email? && SiteSetting.enable_sso? return false unless SiteSetting.email_editable? return true if is_staff? + return false if is_anonymous? can_edit?(user) end def can_edit_name?(user) - return false if not(SiteSetting.enable_names?) - return false if (SiteSetting.sso_overrides_name? && SiteSetting.enable_sso?) + return false unless SiteSetting.enable_names? + return false if SiteSetting.sso_overrides_name? && SiteSetting.enable_sso? return true if is_staff? + return false if is_anonymous? can_edit?(user) end diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb index 1fff9137a9e..7324501421b 100644 --- a/spec/components/guardian_spec.rb +++ b/spec/components/guardian_spec.rb @@ -9,6 +9,7 @@ describe Guardian do let(:user) { Fabricate(:user) } let(:moderator) { Fabricate(:moderator) } let(:admin) { Fabricate(:admin) } + let(:anonymous_user) { Fabricate(:anonymous) } let(:trust_level_1) { build(:user, trust_level: 1) } let(:trust_level_2) { build(:user, trust_level: 2) } let(:trust_level_3) { build(:user, trust_level: 3) } @@ -2403,6 +2404,20 @@ describe Guardian do it "is true for admins" do expect(Guardian.new(admin).can_edit_username?(user)).to be_truthy end + + it "is true for admins when changing anonymous username" do + expect(Guardian.new(admin).can_edit_username?(anonymous_user)).to be_truthy + end + end + + context "for anonymous user" do + before do + SiteSetting.allow_anonymous_posting = true + end + + it "is false" do + expect(Guardian.new(anonymous_user).can_edit_username?(anonymous_user)).to be_falsey + end end context 'for a new user' do @@ -2476,6 +2491,16 @@ describe Guardian do SiteSetting.email_editable = true end + context "for anonymous user" do + before do + SiteSetting.allow_anonymous_posting = true + end + + it "is false" do + expect(Guardian.new(anonymous_user).can_edit_email?(anonymous_user)).to be_falsey + end + end + it "is false when not logged in" do expect(Guardian.new(nil).can_edit_email?(build(:user, created_at: 1.minute.ago))).to be_falsey end @@ -2554,6 +2579,16 @@ describe Guardian do expect(Guardian.new(build(:user)).can_edit_name?(build(:user, created_at: 1.minute.ago))).to be_falsey end + context "for anonymous user" do + before do + SiteSetting.allow_anonymous_posting = true + end + + it "is false" do + expect(Guardian.new(anonymous_user).can_edit_name?(anonymous_user)).to be_falsey + end + end + context 'for a new user' do let(:target_user) { build(:user, created_at: 1.minute.ago) } diff --git a/test/javascripts/controllers/preferences-account-test.js.es6 b/test/javascripts/controllers/preferences-account-test.js.es6 index c0f6914af17..2d40ac26f33 100644 --- a/test/javascripts/controllers/preferences-account-test.js.es6 +++ b/test/javascripts/controllers/preferences-account-test.js.es6 @@ -1,12 +1,13 @@ moduleFor("controller:preferences/account"); -QUnit.skip("updating of associated accounts", function(assert) { +QUnit.test("updating of associated accounts", function(assert) { const controller = this.subject({ siteSettings: { enable_google_oauth2_logins: true }, model: Ember.Object.create({ - second_factor_enabled: true + second_factor_enabled: true, + is_anonymous: true }), site: Ember.Object.create({ isMobileDevice: false @@ -21,6 +22,10 @@ QUnit.skip("updating of associated accounts", function(assert) { assert.equal(controller.get("canUpdateAssociatedAccounts"), false); + controller.set("model.is_anonymous", false); + + assert.equal(controller.get("canUpdateAssociatedAccounts"), false); + controller.set("canCheckEmails", true); assert.equal(controller.get("canUpdateAssociatedAccounts"), true);