From 76a7b75d8ab606b76e2270aa4bb327e2784bc1f7 Mon Sep 17 00:00:00 2001 From: Dan Ungureanu Date: Wed, 29 Sep 2021 17:40:16 +0300 Subject: [PATCH] DEV: Reuse can_invite_to_forum? in can_invite_to? (#14392) This commit resolves refactors can_invite_to? to use can_invite_to_forum? for checking the site-wide permissions and then perform topic specific checkups. Similarly, can_invite_to? is always used with a topic object and this is now enforced. There was another problem before when `must_approve_users` site setting was not checked when inviting users to forum, but was checked when inviting to a topic. Another minor security issue was that group owners could invite to group topics even if they did not have the minimum trust level to do it. --- lib/guardian.rb | 41 ++++++++----------------- spec/components/guardian_spec.rb | 2 ++ spec/models/topic_spec.rb | 4 +-- spec/requests/topics_controller_spec.rb | 6 +++- 4 files changed, 21 insertions(+), 32 deletions(-) diff --git a/lib/guardian.rb b/lib/guardian.rb index 9312442f414..865a2fb6f77 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -357,33 +357,19 @@ class Guardian end def can_invite_to_forum?(groups = nil) - return false if !authenticated? - - invites_available = SiteSetting.max_invites_per_day.to_i.positive? - trust_level_requirement_met = @user.has_trust_level?(SiteSetting.min_trust_level_to_allow_invite.to_i) - - if !is_staff? - return false if !invites_available - return false if !trust_level_requirement_met - end - - if groups.present? - return is_admin? || groups.all? { |g| can_edit_group?(g) } - end - - true + authenticated? && + (is_staff? || !SiteSetting.must_approve_users?) && + (is_staff? || SiteSetting.max_invites_per_day.to_i.positive?) && + (is_staff? || @user.has_trust_level?(SiteSetting.min_trust_level_to_allow_invite.to_i)) && + (is_admin? || groups.blank? || groups.all? { |g| can_edit_group?(g) }) end def can_invite_to?(object, groups = nil) - return false unless authenticated? - is_topic = object.is_a?(Topic) - return true if is_admin? && !is_topic - return false if SiteSetting.max_invites_per_day.to_i == 0 && !is_staff? - return false if SiteSetting.must_approve_users? && !is_staff? - return false unless can_see?(object) + return false if !can_invite_to_forum?(groups) + return false if !object.is_a?(Topic) || !can_see?(object) return false if groups.present? - if is_topic + if object.is_a?(Topic) if object.private_message? return true if is_admin? return false unless SiteSetting.enable_personal_messages? @@ -391,19 +377,16 @@ class Guardian end if (category = object.category) && category.read_restricted - if (groups = category.groups&.where(automatic: false))&.any? - return groups.any? { |g| can_edit_group?(g) } ? true : false - else - return false - end + return category.groups&.where(automatic: false).any? { |g| can_edit_group?(g) } end end - user.has_trust_level?(SiteSetting.min_trust_level_to_allow_invite.to_i) + true end def can_invite_via_email?(object) - return false unless can_invite_to?(object) + return false if !can_invite_to?(object) + (SiteSetting.enable_local_logins || SiteSetting.enable_discourse_connect) && (!SiteSetting.must_approve_users? || is_staff?) end diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb index e3ef79e9fed..56d4b0a464e 100644 --- a/spec/components/guardian_spec.rb +++ b/spec/components/guardian_spec.rb @@ -566,6 +566,7 @@ describe Guardian do end it 'returns true for a group owner' do + group_owner.update!(trust_level: SiteSetting.min_trust_level_to_allow_invite) expect(Guardian.new(group_owner).can_invite_to?(group_private_topic)).to be_truthy end @@ -595,6 +596,7 @@ describe Guardian do end it 'should return true for a group owner' do + group_owner.update!(trust_level: SiteSetting.min_trust_level_to_allow_invite) expect(Guardian.new(group_owner).can_invite_to?(topic)).to eq(true) end diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 9798b931a33..a84253d9c0b 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -7,7 +7,7 @@ describe Topic do let(:now) { Time.zone.local(2013, 11, 20, 8, 0) } fab!(:user) { Fabricate(:user) } fab!(:another_user) { Fabricate(:user) } - fab!(:trust_level_2) { Fabricate(:user, trust_level: TrustLevel[2]) } + fab!(:trust_level_2) { Fabricate(:user, trust_level: SiteSetting.min_trust_level_to_allow_invite) } context 'validations' do let(:topic) { Fabricate.build(:topic) } @@ -899,7 +899,7 @@ describe Topic do end describe 'when user can invite via email' do - before { user.update!(trust_level: TrustLevel[2]) } + before { user.update!(trust_level: SiteSetting.min_trust_level_to_allow_invite) } it 'should create an invite' do Jobs.run_immediately! diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index c6becd3ed38..456ab0ab4c1 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -3868,7 +3868,7 @@ RSpec.describe TopicsController do fab!(:topic) { Fabricate(:topic, user: user) } it 'should return the right response' do - user.update!(trust_level: TrustLevel[2]) + user.update!(trust_level: SiteSetting.min_trust_level_to_allow_invite) expect do post "/t/#{topic.id}/invite.json", params: { @@ -3891,6 +3891,10 @@ RSpec.describe TopicsController do let!(:recipient) { 'jake@adventuretime.ooo' } + before do + user.update!(trust_level: SiteSetting.min_trust_level_to_allow_invite) + end + it "should attach group to the invite" do post "/t/#{group_private_topic.id}/invite.json", params: { user: recipient,