SECURITY: Require groups to be given when inviting to a restricted category. (#6715)

SECURITY: Require groups to be given when inviting to a restricted category. (#6715)
From 978f0db109cccfb5c1fd80640f0c080a1caca8b8 Mon Sep 17 00:00:00 2001
From: Guo Xiang Tan <gxtan1990@gmail.com>
Date: Wed, 5 Dec 2018 23:43:07 +0800
Subject: [PATCH] SECURITY: Require groups to be given when inviting to a
 restricted category. (#6715)


diff --git a/app/assets/javascripts/discourse/controllers/invite.js.es6 b/app/assets/javascripts/discourse/controllers/invite.js.es6
index b9f05da..2227d2c 100644
--- a/app/assets/javascripts/discourse/controllers/invite.js.es6
+++ b/app/assets/javascripts/discourse/controllers/invite.js.es6
@@ -289,7 +289,7 @@ export default Ember.Controller.extend(ModalFunctionality, {
 
       model.setProperties({ saving: true, error: false });
 
-      const onerror = function(e) {
+      const onerror = e => {
         if (e.jqXHR.responseJSON && e.jqXHR.responseJSON.errors) {
           self.set("errorMessage", e.jqXHR.responseJSON.errors[0]);
         } else {
diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb
index 887796e..a8e4822 100644
--- a/app/controllers/topics_controller.rb
+++ b/app/controllers/topics_controller.rb
@@ -563,7 +563,7 @@ class TopicsController < ApplicationController
       ))
     end
 
-    guardian.ensure_can_invite_to!(topic, groups)
+    guardian.ensure_can_invite_to!(topic)
     group_ids = groups.map(&:id)
 
     begin
@@ -576,7 +576,25 @@ class TopicsController < ApplicationController
           render json: success_json
         end
       else
-        render json: failed_json, status: 422
+        json = failed_json
+
+        unless topic.private_message?
+          group_names = topic.category
+            .visible_group_names(current_user)
+            .where(automatic: false)
+            .pluck(:name)
+            .join(", ")
+
+          if group_names.present?
+            json.merge!(errors: [
+              I18n.t("topic_invite.failed_to_invite",
+                group_names: group_names
+              )
+            ])
+          end
+        end
+
+        render json: json, status: 422
       end
     rescue Topic::UserExists => e
       render json: { errors: [e.message] }, status: 422
diff --git a/app/models/category.rb b/app/models/category.rb
index c7c1caf..5dcceb5 100644
--- a/app/models/category.rb
+++ b/app/models/category.rb
@@ -478,6 +478,10 @@ class Category < ActiveRecord::Base
     self.name_lower = name.downcase if self.name
   end
 
+  def visible_group_names(user)
+    self.groups.visible_groups(user)
+  end
+
   def secure_group_ids
     if self.read_restricted?
       groups.pluck("groups.id")
diff --git a/app/models/invite.rb b/app/models/invite.rb
index e59c4fc..cc44d69 100644
--- a/app/models/invite.rb
+++ b/app/models/invite.rb
@@ -57,19 +57,6 @@ class Invite < ActiveRecord::Base
     InviteRedeemer.new(self, username, name, password, user_custom_fields).redeem unless expired? || destroyed? || !link_valid?
   end
 
-  def self.extend_permissions(topic, user, invited_by)
-    if topic.private_message?
-      topic.grant_permission_to_user(user.email)
-    elsif topic.category && topic.category.groups.any?
-      if Guardian.new(invited_by).can_invite_via_email?(topic)
-        (topic.category.groups - user.groups).each do |group|
-          group.add(user)
-          GroupActionLogger.new(Discourse.system_user, group).log_add_user_to_group(user)
-        end
-      end
-    end
-  end
-
   def self.invite_by_email(email, invited_by, topic = nil, group_ids = nil, custom_message = nil)
     create_invite_by_email(email, invited_by,
       topic: topic,
@@ -103,8 +90,11 @@ class Invite < ActiveRecord::Base
     lower_email = Email.downcase(email)
 
     if user = find_user_by_email(lower_email)
-      extend_permissions(topic, user, invited_by) if topic
-      raise UserExists.new I18n.t("invite.user_exists", email: lower_email, username: user.username, base_path: Discourse.base_path)
+      raise UserExists.new(I18n.t("invite.user_exists",
+        email: lower_email,
+        username: user.username,
+        base_path: Discourse.base_path
+      ))
     end
 
     invite = Invite.with_deleted
@@ -134,14 +124,10 @@ class Invite < ActiveRecord::Base
 
     if group_ids.present?
       group_ids = group_ids - invite.invited_groups.pluck(:group_id)
+
       group_ids.each do |group_id|
         invite.invited_groups.create!(group_id: group_id)
       end
-    else
-      if topic && topic.category && Guardian.new(invited_by).can_invite_to?(topic)
-        group_ids = topic.category.groups.where(automatic: false).pluck(:id) - invite.invited_groups.pluck(:group_id)
-        group_ids.each { |group_id| invite.invited_groups.create!(group_id: group_id) }
-      end
     end
 
     Jobs.enqueue(:invite_email, invite_id: invite.id) if send_email
diff --git a/app/models/topic.rb b/app/models/topic.rb
index 734fb24..d9ad81b 100644
--- a/app/models/topic.rb
+++ b/app/models/topic.rb
@@ -839,62 +839,29 @@ class Topic < ActiveRecord::Base
   def invite(invited_by, username_or_email, group_ids = nil, custom_message = nil)
     target_user = User.find_by_username_or_email(username_or_email)
     guardian = Guardian.new(invited_by)
+    is_email = username_or_email =~ /^.+@.+$/
 
-    if target_user && topic_allowed_users.where(user_id: target_user.id).exists?
-      raise UserExists.new(I18n.t("topic_invite.user_exists"))
-    end
-
-    return true if target_user && invite_existing_muted?(target_user, invited_by)
-
-    if private_message? && target_user && !guardian.can_send_private_message?(target_user)
-      raise UserExists.new(I18n.t("activerecord.errors.models.topic.attributes.base.cant_send_pm"))
-    end
-
-    if target_user && private_message? && topic_allowed_users.create!(user_id: target_user.id)
-      rate_limit_topic_invitation(invited_by)
-      add_small_action(invited_by, "invited_user", target_user.username)
-
-      create_invite_notification!(
-        target_user,
-        Notification.types[:invited_to_private_message],
-        invited_by.username
-      )
-
-      true
-    elsif username_or_email =~ /^.+@.+$/ && guardian.can_invite_via_email?(self)
+    if target_user
+      if topic_allowed_users.exists?(user_id: target_user.id)
+        raise UserExists.new(I18n.t("topic_invite.user_exists"))
+      end
 
-      if target_user
-        rate_limit_topic_invitation(invited_by)
-        Invite.extend_permissions(self, target_user, invited_by)
+      if invite_existing_muted?(target_user, invited_by)
+        return true
+      end
 
-        create_invite_notification!(
-          target_user,
-          Notification.types[:invited_to_topic],
-          invited_by.username
-        )
+      if private_message?
+        !!invite_to_private_message(invited_by, target_user, guardian)
       else
-        invite_by_email(invited_by, username_or_email, group_ids, custom_message)
+        !!invite_to_topic(invited_by, target_user, group_ids, guardian)
       end
-
-      true
-    elsif target_user &&
-          rate_limit_topic_invitation(invited_by) &&
-          topic_allowed_users.create!(user_id: target_user.id)
-
-      create_invite_notification!(
-        target_user,
-        Notification.types[:invited_to_topic],
-        invited_by.username
+    elsif is_email && guardian.can_invite_via_email?(self)
+      !!Invite.invite_by_email(
+        username_or_email, invited_by, self, group_ids, custom_message
       )
-
-      true
     end
   end
 
-  def invite_by_email(invited_by, email, group_ids = nil, custom_message = nil)
-    Invite.invite_by_email(email, invited_by, self, group_ids, custom_message)
-  end
-
   def invite_existing_muted?(target_user, invited_by)
     if invited_by.id &&
        MutedUser.where(user_id: target_user.id, muted_user_id: invited_by.id)
@@ -1397,6 +1364,55 @@ class Topic < ActiveRecord::Base
 
   private
 
+  def invite_to_private_message(invited_by, target_user, guardian)
+    if !guardian.can_send_private_message?(target_user)
+      raise UserExists.new(I18n.t

GitHub

1 Like

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

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

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

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

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

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