DEV: Reuse can_invite_to_forum? in can_invite_to? (#14392)

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.

diff --git a/lib/guardian.rb b/lib/guardian.rb
index 9312442..865a2fb 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 e3ef79e..56d4b0a 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 9798b93..a84253d 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 c6becd3..456ab0a 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,

GitHub sha: 76a7b75d8ab606b76e2270aa4bb327e2784bc1f7

This commit appears in #14392 which was approved by ZogStriP. It was merged by udan11.

This commit has been mentioned on Discourse Meta. There might be relevant details there: