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.
This commit is contained in:
Dan Ungureanu 2021-09-29 17:40:16 +03:00 committed by GitHub
parent 7737d56dd0
commit 76a7b75d8a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 21 additions and 32 deletions

View File

@ -357,33 +357,19 @@ class Guardian
end end
def can_invite_to_forum?(groups = nil) def can_invite_to_forum?(groups = nil)
return false if !authenticated? authenticated? &&
(is_staff? || !SiteSetting.must_approve_users?) &&
invites_available = SiteSetting.max_invites_per_day.to_i.positive? (is_staff? || 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) (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) })
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
end end
def can_invite_to?(object, groups = nil) def can_invite_to?(object, groups = nil)
return false unless authenticated? return false if !can_invite_to_forum?(groups)
is_topic = object.is_a?(Topic) return false if !object.is_a?(Topic) || !can_see?(object)
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 groups.present? return false if groups.present?
if is_topic if object.is_a?(Topic)
if object.private_message? if object.private_message?
return true if is_admin? return true if is_admin?
return false unless SiteSetting.enable_personal_messages? return false unless SiteSetting.enable_personal_messages?
@ -391,19 +377,16 @@ class Guardian
end end
if (category = object.category) && category.read_restricted if (category = object.category) && category.read_restricted
if (groups = category.groups&.where(automatic: false))&.any? return category.groups&.where(automatic: false).any? { |g| can_edit_group?(g) }
return groups.any? { |g| can_edit_group?(g) } ? true : false
else
return false
end
end end
end end
user.has_trust_level?(SiteSetting.min_trust_level_to_allow_invite.to_i) true
end end
def can_invite_via_email?(object) 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.enable_local_logins || SiteSetting.enable_discourse_connect) &&
(!SiteSetting.must_approve_users? || is_staff?) (!SiteSetting.must_approve_users? || is_staff?)
end end

View File

@ -566,6 +566,7 @@ describe Guardian do
end end
it 'returns true for a group owner' do 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 expect(Guardian.new(group_owner).can_invite_to?(group_private_topic)).to be_truthy
end end
@ -595,6 +596,7 @@ describe Guardian do
end end
it 'should return true for a group owner' do 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) expect(Guardian.new(group_owner).can_invite_to?(topic)).to eq(true)
end end

View File

@ -7,7 +7,7 @@ describe Topic do
let(:now) { Time.zone.local(2013, 11, 20, 8, 0) } let(:now) { Time.zone.local(2013, 11, 20, 8, 0) }
fab!(:user) { Fabricate(:user) } fab!(:user) { Fabricate(:user) }
fab!(:another_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 context 'validations' do
let(:topic) { Fabricate.build(:topic) } let(:topic) { Fabricate.build(:topic) }
@ -899,7 +899,7 @@ describe Topic do
end end
describe 'when user can invite via email' do 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 it 'should create an invite' do
Jobs.run_immediately! Jobs.run_immediately!

View File

@ -3868,7 +3868,7 @@ RSpec.describe TopicsController do
fab!(:topic) { Fabricate(:topic, user: user) } fab!(:topic) { Fabricate(:topic, user: user) }
it 'should return the right response' do 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 expect do
post "/t/#{topic.id}/invite.json", params: { post "/t/#{topic.id}/invite.json", params: {
@ -3891,6 +3891,10 @@ RSpec.describe TopicsController do
let!(:recipient) { 'jake@adventuretime.ooo' } 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 it "should attach group to the invite" do
post "/t/#{group_private_topic.id}/invite.json", params: { post "/t/#{group_private_topic.id}/invite.json", params: {
user: recipient, user: recipient,