FIX: access to category chat only when user can create post (#19488)

Previously, restricted category chat channel was available for all groups - even `readonly`. From now on, only user who belong to group with `create_post` or `full` permissions can access that chat channel.

lib/guardian/category_guardian.rb

@@ -54,6 +54,14 @@ module CategoryGuardian
     secure_category_ids.include?(category.id)
   end
 
+  def can_post_in_category?(category)
+    return false unless category
+    return false if is_anonymous?
+    return true if is_admin?
+    return true if !category.read_restricted
+    Category.post_create_allowed(self).exists?(id: category.id)
+  end
+
   def can_edit_category_description?(category)
     can_perform_action_available_to_group_moderators?(category.topic)
   end

plugins/chat/app/controllers/api/category_chatables_controller.rb

@@ -9,6 +9,7 @@ class Chat::Api::CategoryChatablesController < ApplicationController
         Group
           .joins(:category_groups)
           .where(category_groups: { category_id: category.id })
+          .where("category_groups.permission_type IN (?)", [CategoryGroup.permission_types[:full], CategoryGroup.permission_types[:create_post]])
           .joins("LEFT OUTER JOIN group_users ON groups.id = group_users.group_id")
           .group("groups.id", "groups.name")
           .pluck("groups.name", "COUNT(group_users.user_id)")

plugins/chat/app/controllers/api/chat_channels_controller.rb

@@ -61,7 +61,7 @@ class Chat::Api::ChatChannelsController < Chat::Api
 
   def find_chat_channel
     chat_channel = ChatChannel.find(params.require(:chat_channel_id))
-    guardian.ensure_can_see_chat_channel!(chat_channel)
+    guardian.ensure_can_join_chat_channel!(chat_channel)
     chat_channel
   end
 

plugins/chat/app/controllers/chat_controller.rb

@@ -32,7 +32,7 @@ class Chat::ChatController < Chat::ChatBaseController
   def enable_chat
     chat_channel = ChatChannel.with_deleted.find_by(chatable: @chatable)
 
-    guardian.ensure_can_see_chat_channel!(chat_channel) if chat_channel
+    guardian.ensure_can_join_chat_channel!(chat_channel) if chat_channel
 
     if chat_channel && chat_channel.trashed?
       chat_channel.recover!
@@ -40,7 +40,7 @@ class Chat::ChatController < Chat::ChatBaseController
       return render_json_error I18n.t("chat.already_enabled")
     else
       chat_channel = @chatable.chat_channel
-      guardian.ensure_can_see_chat_channel!(chat_channel)
+      guardian.ensure_can_join_chat_channel!(chat_channel)
     end
 
     success = chat_channel.save
@@ -61,7 +61,7 @@ class Chat::ChatController < Chat::ChatBaseController
 
   def disable_chat
     chat_channel = ChatChannel.with_deleted.find_by(chatable: @chatable)
-    guardian.ensure_can_see_chat_channel!(chat_channel)
+    guardian.ensure_can_join_chat_channel!(chat_channel)
     return render json: success_json if chat_channel.trashed?
     chat_channel.trash!(current_user)
 
@@ -346,7 +346,7 @@ class Chat::ChatController < Chat::ChatBaseController
         .where(id: params[:user_ids])
     users.each do |user|
       guardian = Guardian.new(user)
-      if guardian.can_chat? && guardian.can_see_chat_channel?(@chat_channel)
+      if guardian.can_chat? && guardian.can_join_chat_channel?(@chat_channel)
         data = {
           message: "chat.invitation_notification",
           chat_channel_id: @chat_channel.id,

plugins/chat/app/jobs/regular/chat_notify_watching.rb

@@ -43,7 +43,7 @@ module Jobs
     def send_notifications(membership)
       user = membership.user
       guardian = Guardian.new(user)
-      return unless guardian.can_chat? && guardian.can_see_chat_channel?(@chat_channel)
+      return unless guardian.can_chat? && guardian.can_join_chat_channel?(@chat_channel)
       return if Chat::ChatNotifier.user_has_seen_message?(membership, @chat_message.id)
       return if online_user_ids.include?(user.id)
 

plugins/chat/lib/chat_channel_fetcher.rb

@@ -30,18 +30,10 @@ module Chat::ChatChannelFetcher
   end
 
   def self.generate_allowed_channel_ids_sql(guardian, exclude_dm_channels: false)
-    category_channel_sql =
-      ChatChannel
-        .select(:id)
-        .joins(
-          "INNER JOIN categories ON categories.id = chat_channels.chatable_id AND chat_channels.chatable_type = 'Category'",
-        )
-        .where(
-          "categories.id IN (:allowed_category_ids)",
-          allowed_category_ids: guardian.allowed_category_ids,
-        )
+    category_channel_sql = Category.post_create_allowed(guardian)
+      .joins("INNER JOIN chat_channels ON chat_channels.chatable_id = categories.id AND chat_channels.chatable_type = 'Category'")
+      .select("chat_channels.id")
       .to_sql
-
     dm_channel_sql = ""
     if !exclude_dm_channels
       dm_channel_sql = <<~SQL
@@ -254,7 +246,7 @@ module Chat::ChatChannelFetcher
     end
 
     raise Discourse::NotFound if chat_channel.blank?
-    raise Discourse::InvalidAccess if !guardian.can_see_chat_channel?(chat_channel)
+    raise Discourse::InvalidAccess if !guardian.can_join_chat_channel?(chat_channel)
     chat_channel
   end
 end

plugins/chat/lib/chat_message_bookmarkable.rb

@@ -31,7 +31,7 @@ class ChatMessageBookmarkable < BaseBookmarkable
   end
 
   def self.validate_before_create(guardian, bookmarkable)
-    if bookmarkable.blank? || !guardian.can_see_chat_channel?(bookmarkable.chat_channel)
+    if bookmarkable.blank? || !guardian.can_join_chat_channel?(bookmarkable.chat_channel)
       raise Discourse::InvalidAccess
     end
   end
@@ -55,7 +55,7 @@ class ChatMessageBookmarkable < BaseBookmarkable
   end
 
   def self.can_see?(guardian, bookmark)
-    guardian.can_see_chat_channel?(bookmark.bookmarkable.chat_channel)
+    guardian.can_join_chat_channel?(bookmark.bookmarkable.chat_channel)
   end
 
   def self.cleanup_deleted

plugins/chat/lib/chat_message_reactor.rb

@@ -12,7 +12,7 @@ class Chat::ChatMessageReactor
   end
 
   def react!(message_id:, react_action:, emoji:)
-    @guardian.ensure_can_see_chat_channel!(@chat_channel)
+    @guardian.ensure_can_join_chat_channel!(@chat_channel)
     @guardian.ensure_can_react!
     validate_channel_status!
     validate_reaction!(react_action, emoji)

plugins/chat/lib/chat_notifier.rb

@@ -195,7 +195,7 @@ class Chat::ChatNotifier
     potential_participants, unreachable =
       users.partition do |user|
         guardian = Guardian.new(user)
-        guardian.can_chat? && guardian.can_see_chat_channel?(@chat_channel)
+        guardian.can_chat? && guardian.can_join_chat_channel?(@chat_channel)
       end
 
     participants, welcome_to_join =

plugins/chat/lib/extensions/user_notifications_extension.rb

@@ -28,7 +28,7 @@ module Chat::UserNotificationsExtension
     return if @messages.empty?
     @grouped_messages = @messages.group_by { |message| message.chat_channel }
     @grouped_messages =
-      @grouped_messages.select { |channel, _| guardian.can_see_chat_channel?(channel) }
+      @grouped_messages.select { |channel, _| guardian.can_join_chat_channel?(channel) }
     return if @grouped_messages.empty?
 
     @grouped_messages.each do |chat_channel, messages|

plugins/chat/lib/guardian_extensions.rb

@@ -80,7 +80,7 @@ module Chat::GuardianExtensions
     is_staff? || @user.has_trust_level?(TrustLevel[4])
   end
 
-  def can_see_chat_channel?(chat_channel)
+  def can_preview_chat_channel?(chat_channel)
     return false unless chat_channel.chatable
 
     if chat_channel.direct_message_channel?
@@ -92,6 +92,12 @@ module Chat::GuardianExtensions
     end
   end
 
+  def can_join_chat_channel?(chat_channel)
+    return false if anonymous?
+    can_preview_chat_channel?(chat_channel) &&
+      (chat_channel.direct_message_channel? || can_post_in_category?(chat_channel.chatable))
+  end
+
   def can_flag_chat_messages?
     return false if @user.silenced?
     return true if @user.staff?
@@ -102,7 +108,7 @@ module Chat::GuardianExtensions
   def can_flag_in_chat_channel?(chat_channel)
     return false if !can_modify_channel_message?(chat_channel)
 
-    can_see_chat_channel?(chat_channel)
+    can_join_chat_channel?(chat_channel)
   end
 
   def can_flag_chat_message?(chat_message)

plugins/chat/plugin.rb

@@ -271,7 +271,7 @@ after_initialize do
         next if !chat_channel
       end
 
-      next if !Guardian.new.can_see_chat_channel?(chat_channel)
+      next if !Guardian.new.can_preview_chat_channel?(chat_channel)
 
       name = (chat_channel.name if chat_channel.name.present?)
 
@@ -353,7 +353,7 @@ after_initialize do
           end
       end
 
-      next if !Guardian.new.can_see_chat_channel?(chat_channel)
+      next if !Guardian.new.can_preview_chat_channel?(chat_channel)
 
       { url: url, title: title }
     end

plugins/chat/spec/lib/chat_channel_fetcher_spec.rb

@@ -136,6 +136,29 @@ describe Chat::ChatChannelFetcher do
         GroupUser.create!(group: private_category.groups.last, user: user1)
         expect(subject.all_secured_channel_ids(guardian)).to match_array([category_channel.id])
       end
+
+      context "when restricted category" do
+        fab!(:group) { Fabricate(:group) }
+        fab!(:category_group) { Fabricate(:category_group, category: private_category, group: group, permission_type: CategoryGroup.permission_types[:readonly]) }
+        fab!(:group_user) { Fabricate(:group_user, group: private_category.groups.last, user: user1) }
+        before do
+          category_channel.update!(chatable: private_category)
+        end
+
+        it "does not include the category channel for member of group with readonly access" do
+          expect(subject.all_secured_channel_ids(guardian)).to be_empty
+        end
+
+        it "includes the category channel for member of group with create_post access" do
+          category_group.update!(permission_type: CategoryGroup.permission_types[:create_post])
+          expect(subject.all_secured_channel_ids(guardian)).to match_array([category_channel.id])
+        end
+
+        it "includes the category channel for member of group with full access" do
+          category_group.update!(permission_type: CategoryGroup.permission_types[:full])
+          expect(subject.all_secured_channel_ids(guardian)).to match_array([category_channel.id])
+        end
+      end
     end
   end
 

plugins/chat/spec/lib/chat_message_reactor_spec.rb

@@ -9,6 +9,11 @@ describe Chat::ChatMessageReactor do
   fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel, user: reacting_user) }
   let(:subject) { described_class.new(reacting_user, channel) }
 
+  it 'calls guardian ensure_can_join_chat_channel!' do
+    Guardian.any_instance.expects(:ensure_can_join_chat_channel!).once
+    subject.react!(message_id: message_1.id, react_action: :add, emoji: ":+1:")
+  end
+
   it "raises an error if the user cannot see the channel" do
     channel.update!(chatable: Fabricate(:private_category, group: Group[:staff]))
     expect {

plugins/chat/spec/lib/chat_notifier_spec.rb

@@ -275,6 +275,12 @@ describe Chat::ChatNotifier do
 
       include_examples "ensure only channel members are notified"
 
+      it 'calls guardian can_join_chat_channel?' do
+        Guardian.any_instance.expects(:can_join_chat_channel?).at_least_once
+        msg = build_cooked_msg("Hello @#{group.name} and @#{user_2.username}", user_1)
+        to_notify = described_class.new(msg, msg.created_at).notify_new
+      end
+
       it "establishes a far-left precedence among group mentions" do
         Fabricate(
           :user_chat_channel_membership,

plugins/chat/spec/lib/guardian_extensions_spec.rb

@@ -72,18 +72,18 @@ RSpec.describe Chat::GuardianExtensions do
       expect(staff_guardian.can_change_channel_status?(channel, :read_only)).to eq(true)
     end
 
-    describe "#can_see_chat_channel?" do
+    describe "#can_join_chat_channel?" do
       context "for direct message channels" do
         fab!(:chatable) { Fabricate(:direct_message) }
         fab!(:channel) { Fabricate(:direct_message_channel, chatable: chatable) }
 
         it "returns false if the user is not part of the direct message" do
-          expect(guardian.can_see_chat_channel?(channel)).to eq(false)
+          expect(guardian.can_join_chat_channel?(channel)).to eq(false)
         end
 
         it "returns true if the user is part of the direct message" do
           DirectMessageUser.create!(user: user, direct_message: chatable)
-          expect(guardian.can_see_chat_channel?(channel)).to eq(true)
+          expect(guardian.can_join_chat_channel?(channel)).to eq(true)
         end
       end
 
@@ -92,15 +92,20 @@ RSpec.describe Chat::GuardianExtensions do
 
         before { channel.update(chatable: category) }
 
-        it "returns true if the user can see the category" do
-          expect(Guardian.new(user).can_see_chat_channel?(channel)).to eq(false)
-          group = Fabricate(:group)
-          CategoryGroup.create(group: group, category: category)
-          GroupUser.create(group: group, user: user)
+        it "returns true if the user can join the category" do
+          guardian = Guardian.new(user)
 
-          # have to make a new instance of guardian because `user.secure_category_ids`
-          # is memoized there
-          expect(Guardian.new(user).can_see_chat_channel?(channel)).to eq(true)
+          readonly_group = Fabricate(:group)
+          CategoryGroup.create(group: readonly_group, category: category, permission_type: CategoryGroup.permission_types[:readonly])
+          GroupUser.create(group: readonly_group, user: user)
+
+          create_post_group = Fabricate(:group)
+          CategoryGroup.create(group: create_post_group, category: category, permission_type: CategoryGroup.permission_types[:create_post])
+
+          expect(guardian.can_join_chat_channel?(channel)).to eq(false)
+
+          GroupUser.create(group: create_post_group, user: user)
+          expect(guardian.can_join_chat_channel?(channel)).to eq(true)
         end
       end
     end

plugins/chat/spec/mailers/user_notifications_spec.rb

@@ -25,6 +25,13 @@ describe UserNotifications do
         Chat::DirectMessageChannelCreator.create!(acting_user: sender, target_users: [sender, user])
       end
 
+      it 'calls guardian can_join_chat_channel?' do
+        Fabricate(:chat_message, user: sender, chat_channel: channel)
+        Guardian.any_instance.expects(:can_join_chat_channel?).once
+        email = described_class.chat_summary(user, {})
+        email.subject
+      end
+
       describe "email subject" do
         it "includes the sender username in the subject" do
           expected_subject = I18n.t(

plugins/chat/spec/requests/api/category_chatables_controller_spec.rb

@@ -13,9 +13,13 @@ describe Chat::Api::CategoryChatablesController do
       before { sign_in(admin) }
 
       it "returns a list with the group names that could access a chat channel" do
+        readonly_group = Fabricate(:group)
+        Fabricate(:category_group, category: private_category, group: readonly_group, permission_type: CategoryGroup.permission_types[:readonly])
+        create_post_group = Fabricate(:group)
+        create_post_category_group = Fabricate(:category_group, category: private_category, group: create_post_group, permission_type: CategoryGroup.permission_types[:create_post])
         get "/chat/api/category-chatables/#{private_category.id}/permissions.json"
 
-        expect(response.parsed_body["allowed_groups"]).to contain_exactly("@#{group.name}")
+        expect(response.parsed_body["allowed_groups"]).to contain_exactly("@#{group.name}", "@#{create_post_group.name}")
         expect(response.parsed_body["members_count"]).to eq(0)
         expect(response.parsed_body["private"]).to eq(true)
       end

plugins/chat/spec/requests/api/chat_channel_notifications_settings_controller_spec.rb

@@ -6,12 +6,24 @@ RSpec.describe Chat::Api::ChatChannelNotificationsSettingsController do
     SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:everyone]
   end
 
+  fab!(:chat_channel) { Fabricate(:category_channel) }
+  fab!(:user) { Fabricate(:user) }
+
   describe "#update" do
     include_examples "channel access example", :put, "/notifications_settings.json"
 
+    it 'calls guardian ensure_can_join_chat_channel!' do
+      sign_in(user)
+      Guardian.any_instance.expects(:ensure_can_join_chat_channel!).once
+      put "/chat/api/chat_channels/#{chat_channel.id}/notifications_settings.json",
+        params: {
+          muted: true,
+          desktop_notification_level: "always",
+          mobile_notification_level: "never",
+        }
+    end
+
     context "when category channel has invalid params" do
-      fab!(:chat_channel) { Fabricate(:category_channel) }
-      fab!(:user) { Fabricate(:user) }
       fab!(:membership) do
         Fabricate(:user_chat_channel_membership, user: user, chat_channel: chat_channel)
       end
@@ -32,8 +44,6 @@ RSpec.describe Chat::Api::ChatChannelNotificationsSettingsController do
     end
 
     context "when category channel has valid params" do
-      fab!(:chat_channel) { Fabricate(:category_channel) }
-      fab!(:user) { Fabricate(:user) }
       fab!(:membership) do
         Fabricate(
           :user_chat_channel_membership,

plugins/chat/spec/requests/chat_controller_spec.rb

@@ -247,7 +247,7 @@ RSpec.describe Chat::ChatController do
       let(:channel) { Fabricate(:category_channel, chatable: category) }
 
       it "ensures created channel can be seen" do
-        Guardian.any_instance.expects(:can_see_chat_channel?).with(channel)
+        Guardian.any_instance.expects(:can_join_chat_channel?).with(channel)
 
         sign_in(admin)
         post "/chat/enable.json", params: { chatable_type: "category", chatable_id: category.id }
@@ -255,7 +255,7 @@ RSpec.describe Chat::ChatController do
 
       # TODO: rewrite specs to ensure no exception is raised
       it "ensures existing channel can be seen" do
-        Guardian.any_instance.expects(:can_see_chat_channel?)
+        Guardian.any_instance.expects(:can_join_chat_channel?)
 
         sign_in(admin)
         post "/chat/enable.json", params: { chatable_type: "category", chatable_id: category.id }
@@ -270,7 +270,7 @@ RSpec.describe Chat::ChatController do
         channel = Fabricate(:category_channel, chatable: category)
         message = Fabricate(:chat_message, chat_channel: channel)
 
-        Guardian.any_instance.expects(:can_see_chat_channel?).with(channel)
+        Guardian.any_instance.expects(:can_join_chat_channel?).with(channel)
 
         sign_in(admin)
         post "/chat/disable.json", params: { chatable_type: "category", chatable_id: category.id }
@@ -1137,6 +1137,8 @@ RSpec.describe Chat::ChatController do
 
     it "returns a 403 if the user can't see the channel" do
       category.update!(read_restricted: true)
+      group = Fabricate(:group)
+      CategoryGroup.create(group: group, category: category, permission_type: CategoryGroup.permission_types[:create_post])
       sign_in(user)
       post "/chat/#{channel.id}/quote.json",
            params: {
@@ -1310,7 +1312,7 @@ RSpec.describe Chat::ChatController do
       channel = Fabricate(:category_channel, chatable: Fabricate(:category))
       message = Fabricate(:chat_message, chat_channel: channel)
 
-      Guardian.any_instance.expects(:can_see_chat_channel?).with(channel)
+      Guardian.any_instance.expects(:can_join_chat_channel?).with(channel)
 
       sign_in(Fabricate(:user))
       get "/chat/message/#{message.id}.json"
@@ -1326,7 +1328,7 @@ RSpec.describe Chat::ChatController do
     before { sign_in(user) }
 
     it "ensures message's channel can be seen" do
-      Guardian.any_instance.expects(:can_see_chat_channel?).with(channel)
+      Guardian.any_instance.expects(:can_join_chat_channel?).with(channel)
       get "/chat/lookup/#{message.id}.json", { params: { chat_channel_id: channel.id } }
     end
 

spec/lib/category_guardian_spec.rb

@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+
+RSpec.describe CategoryGuardian do
+  fab!(:admin) { Fabricate(:admin) }
+  fab!(:user) { Fabricate(:user) }
+  fab!(:can_create_user) { Fabricate(:user) }
+
+  describe "can_post_in_category?" do
+    fab!(:category) { Fabricate(:category) }
+    context "when not restricted category" do
+      it "returns false for anonymous user" do
+        expect(Guardian.new.can_post_in_category?(category)).to eq(false)
+      end
+      it "returns true for admin" do
+        expect(Guardian.new(admin).can_post_in_category?(category)).to eq(true)
+      end
+
+      it "returns true for regular user" do
+        expect(Guardian.new(user).can_post_in_category?(category)).to eq(true)
+      end
+    end
+
+    context "when restricted category" do
+      fab!(:category) { Fabricate(:category, read_restricted: true) }
+      fab!(:group) { Fabricate(:group) }
+      fab!(:group_user) { Fabricate(:group_user, group: group, user: user) }
+      fab!(:category_group) { Fabricate(:category_group, group: group, category: category, permission_type: CategoryGroup.permission_types[:readonly]) }
+
+      it "returns false for anonymous user" do
+        expect(Guardian.new.can_post_in_category?(category)).to eq(false)
+      end
+
+      it "returns false for member of group with readonly access" do
+        expect(Guardian.new(user).can_post_in_category?(category)).to eq(false)
+      end
+
+      it "returns true for admin" do
+        expect(Guardian.new(admin).can_post_in_category?(category)).to eq(true)
+      end
+
+      it "returns true for member of group with create_post access" do
+        category_group.update!(permission_type: CategoryGroup.permission_types[:create_post])
+        expect(Guardian.new(admin).can_post_in_category?(category)).to eq(true)
+      end
+
+      it "returns true for member of group with full access" do
+        category_group.update!(permission_type: CategoryGroup.permission_types[:full])
+        expect(Guardian.new(admin).can_post_in_category?(category)).to eq(true)
+      end
+    end
+  end
+end