FIX: Don't autojoin users when they have ready-only permissions (#20213)

After this change, in order to join a chat channel, a user needs to be in a group with at least “Reply” permission for the category. If the user only has “See” permission, they are able to preview the channel, but not join it or send messages. The auto-join function also follows this new restriction.

---------

Co-authored-by: Martin Brennan <martin@discourse.org>

lib/guardian/category_guardian.rb

@@ -49,7 +49,6 @@ module CategoryGuardian
     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
 

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

@@ -29,11 +29,12 @@ module Jobs
         suspended_until: Time.zone.now,
         last_seen_at: 3.months.ago,
         channel_category: channel.chatable_id,
+        permission_type: CategoryGroup.permission_types[:create_post],
+        everyone: Group::AUTO_GROUPS[:everyone],
         mode: Chat::UserChatChannelMembership.join_modes[:automatic],
       }
 
       new_member_ids = DB.query_single(create_memberships_query(category), query_args)
-
       # Only do this if we are running auto-join for a single user, if we
       # are doing it for many then we should do it after all batches are
       # complete for the channel in Jobs::AutoJoinChannelMemberships
@@ -54,28 +55,25 @@ module Jobs
         INNER JOIN user_options uo ON uo.user_id = users.id
         LEFT OUTER JOIN user_chat_channel_memberships uccm ON
           uccm.chat_channel_id = :chat_channel_id AND uccm.user_id = users.id
-      SQL
 
-      query += <<~SQL if category.read_restricted?
-          INNER JOIN group_users gu ON gu.user_id = users.id
-          LEFT OUTER JOIN category_groups cg ON cg.group_id = gu.group_id
-        SQL
+        LEFT OUTER JOIN group_users gu ON gu.user_id = users.id
+        LEFT OUTER JOIN category_groups cg ON cg.group_id = gu.group_id AND
+        cg.permission_type <= :permission_type
 
-      query += <<~SQL
         WHERE (users.id >= :start AND users.id <= :end) AND
-          users.staged IS FALSE AND users.active AND
+          users.staged IS FALSE AND
+          users.active AND
           NOT EXISTS(SELECT 1 FROM anonymous_users a WHERE a.user_id = users.id) AND
           (suspended_till IS NULL OR suspended_till <= :suspended_until) AND
-          (last_seen_at > :last_seen_at) AND
+          (last_seen_at IS NULL OR last_seen_at > :last_seen_at) AND
           uo.chat_enabled AND
-          uccm.id IS NULL
+          uccm.id IS NULL AND
+          (NOT EXISTS(SELECT 1 FROM category_groups WHERE category_id = :channel_category) 
+            OR EXISTS (SELECT 1 FROM category_groups WHERE category_id = :channel_category AND group_id = :everyone AND permission_type <= :permission_type)
+            OR cg.category_id = :channel_category)
+
+        RETURNING user_chat_channel_memberships.user_id
       SQL
-
-      query += <<~SQL if category.read_restricted?
-          AND cg.category_id = :channel_category
-        SQL
-
-      query += "RETURNING user_chat_channel_memberships.user_id"
     end
   end
 end

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

@@ -30,6 +30,8 @@ module Jobs
           suspended_until: Time.zone.now,
           last_seen_at: 3.months.ago,
           channel_category: channel.chatable_id,
+          permission_type: CategoryGroup.permission_types[:create_post],
+          everyone: Group::AUTO_GROUPS[:everyone],
           mode: ::Chat::UserChatChannelMembership.join_modes[:automatic],
         }
 
@@ -49,34 +51,31 @@ module Jobs
 
       def create_memberships_query(category)
         query = <<~SQL
-        INSERT INTO user_chat_channel_memberships (user_id, chat_channel_id, following, created_at, updated_at, join_mode)
-        SELECT DISTINCT(users.id), :chat_channel_id, TRUE, NOW(), NOW(), :mode
-        FROM users
-        INNER JOIN user_options uo ON uo.user_id = users.id
-        LEFT OUTER JOIN user_chat_channel_memberships uccm ON
-          uccm.chat_channel_id = :chat_channel_id AND uccm.user_id = users.id
-      SQL
-
-        query += <<~SQL if category.read_restricted?
-          INNER JOIN group_users gu ON gu.user_id = users.id
-          LEFT OUTER JOIN category_groups cg ON cg.group_id = gu.group_id
+          INSERT INTO user_chat_channel_memberships (user_id, chat_channel_id, following, created_at, updated_at, join_mode)
+          SELECT DISTINCT(users.id), :chat_channel_id, TRUE, NOW(), NOW(), :mode
+          FROM users
+          INNER JOIN user_options uo ON uo.user_id = users.id
+          LEFT OUTER JOIN user_chat_channel_memberships uccm ON
+            uccm.chat_channel_id = :chat_channel_id AND uccm.user_id = users.id
+  
+          LEFT OUTER JOIN group_users gu ON gu.user_id = users.id
+          LEFT OUTER JOIN category_groups cg ON cg.group_id = gu.group_id AND
+          cg.permission_type <= :permission_type
+  
+          WHERE (users.id >= :start AND users.id <= :end) AND
+            users.staged IS FALSE AND
+            users.active AND
+            NOT EXISTS(SELECT 1 FROM anonymous_users a WHERE a.user_id = users.id) AND
+            (suspended_till IS NULL OR suspended_till <= :suspended_until) AND
+            (last_seen_at IS NULL OR last_seen_at > :last_seen_at) AND
+            uo.chat_enabled AND
+            uccm.id IS NULL AND
+            (NOT EXISTS(SELECT 1 FROM category_groups WHERE category_id = :channel_category) 
+              OR EXISTS (SELECT 1 FROM category_groups WHERE category_id = :channel_category AND group_id = :everyone AND permission_type <= :permission_type)
+              OR cg.category_id = :channel_category)
+  
+          RETURNING user_chat_channel_memberships.user_id
         SQL
-
-        query += <<~SQL
-        WHERE (users.id >= :start AND users.id <= :end) AND
-          users.staged IS FALSE AND users.active AND
-          NOT EXISTS(SELECT 1 FROM anonymous_users a WHERE a.user_id = users.id) AND
-          (suspended_till IS NULL OR suspended_till <= :suspended_until) AND
-          (last_seen_at > :last_seen_at) AND
-          uo.chat_enabled AND
-          uccm.id IS NULL
-      SQL
-
-        query += <<~SQL if category.read_restricted?
-          AND cg.category_id = :channel_category
-        SQL
-
-        query += "RETURNING user_chat_channel_memberships.user_id"
       end
     end
   end

plugins/chat/app/serializers/chat/channel_serializer.rb

@@ -116,7 +116,15 @@ module Chat
       }
 
       ids[:kick] = kick_message_bus_id if !object.direct_message_channel?
-      { message_bus_last_ids: ids }
+      data = { message_bus_last_ids: ids }
+
+      if @opts.key?(:can_join_chat_channel)
+        data[:can_join_chat_channel] = @opts[:can_join_chat_channel]
+      else
+        data[:can_join_chat_channel] = scope.can_join_chat_channel?(object)
+      end
+
+      data
     end
 
     alias_method :include_archive_topic_id?, :include_archive_status?

plugins/chat/app/serializers/chat/structured_channel_serializer.rb

@@ -19,6 +19,10 @@ module Chat
             chat_message_bus_last_ids[Chat::Publisher.kick_users_message_bus_channel(channel.id)],
           channel_message_bus_last_id:
             chat_message_bus_last_ids[Chat::Publisher.root_message_bus_channel(channel.id)],
+          # NOTE: This is always true because the public channels passed into this serializer
+          # have been fetched with [Chat::ChannelFetcher], which only returns channels that
+          # the user has access to based on category permissions.
+          can_join_chat_channel: true,
         )
       end
     end

plugins/chat/assets/javascripts/discourse/components/chat-channel-preview-card.hbs

@@ -2,6 +2,7 @@
   class={{concat-class
     "chat-channel-preview-card"
     (unless this.hasDescription "-no-description")
+    (unless this.showJoinButton "-no-button")
   }}
 >
   <ChatChannelTitle @channel={{@channel}} />

plugins/chat/assets/javascripts/discourse/components/chat-channel-preview-card.js

@@ -6,7 +6,7 @@ export default class ChatChannelPreviewCard extends Component {
   @service chat;
 
   get showJoinButton() {
-    return this.args.channel?.isOpen;
+    return this.args.channel?.isOpen && this.args.channel?.canJoin;
   }
 
   get hasDescription() {

plugins/chat/assets/javascripts/discourse/models/chat-channel.js

@@ -215,6 +215,10 @@ export default class ChatChannel {
     return this.currentUserMembership.following;
   }
 
+  get canJoin() {
+    return this.meta.can_join_chat_channel;
+  }
+
   get visibleMessages() {
     return this.messages.filter((message) => message.visible);
   }

plugins/chat/assets/stylesheets/common/chat-channel-preview-card.scss

@@ -13,6 +13,12 @@
     }
   }
 
+  &.-no-button {
+    .chat-channel-preview-card__browse-all {
+      margin-top: 0;
+    }
+  }
+
   &__description {
     color: var(--primary-600);
     text-align: center;

plugins/chat/lib/chat/channel_fetcher.rb

@@ -238,7 +238,7 @@ module Chat
       end
 
       raise Discourse::NotFound if chat_channel.blank?
-      raise Discourse::InvalidAccess if !guardian.can_join_chat_channel?(chat_channel)
+      raise Discourse::InvalidAccess if !guardian.can_preview_chat_channel?(chat_channel)
       chat_channel
     end
   end

plugins/chat/spec/jobs/regular/chat/auto_join_channel_batch_spec.rb

@@ -180,11 +180,67 @@ describe Jobs::Chat::AutoJoinChannelBatch do
 
         assert_users_follows_channel(channel, [user, another_user])
       end
+
+      it "doesn't join users with read-only access to the category" do
+        restricted_category = Fabricate(:category, read_restricted: true)
+        another_user = Fabricate(:user, last_seen_at: 15.minutes.ago)
+        non_chatters_group = Fabricate(:group)
+        readonly_channel =
+          Fabricate(:category_channel, chatable: restricted_category, auto_join_users: true)
+        Fabricate(
+          :category_group,
+          category: restricted_category,
+          group: non_chatters_group,
+          permission_type: CategoryGroup.permission_types[:readonly],
+        )
+        non_chatters_group.add(another_user)
+
+        subject.execute(
+          chat_channel_id: readonly_channel.id,
+          starts_at: another_user.id,
+          ends_at: another_user.id,
+        )
+
+        assert_user_skipped(readonly_channel, another_user)
+      end
+
+      it "does join users with at least one group with create_post or full permission" do
+        restricted_category = Fabricate(:category, read_restricted: true)
+        another_user = Fabricate(:user, last_seen_at: 15.minutes.ago)
+        non_chatters_group = Fabricate(:group)
+        private_channel =
+          Fabricate(:category_channel, chatable: restricted_category, auto_join_users: true)
+        Fabricate(
+          :category_group,
+          category: restricted_category,
+          group: non_chatters_group,
+          permission_type: CategoryGroup.permission_types[:readonly],
+        )
+        non_chatters_group.add(another_user)
+
+        other_group = Fabricate(:group)
+        Fabricate(
+          :category_group,
+          category: restricted_category,
+          group: other_group,
+          permission_type: CategoryGroup.permission_types[:create_post],
+        )
+        other_group.add(another_user)
+
+        subject.execute(
+          chat_channel_id: private_channel.id,
+          starts_at: another_user.id,
+          ends_at: another_user.id,
+        )
+
+        assert_users_follows_channel(private_channel, [another_user])
+      end
     end
   end
 
   def assert_users_follows_channel(channel, users)
     new_memberships = Chat::UserChatChannelMembership.where(user: users, chat_channel: channel)
+    expect(new_memberships.length).to eq(users.length)
     expect(new_memberships.all?(&:following)).to eq(true)
   end
 

plugins/chat/spec/requests/chat_controller_spec.rb

@@ -1125,7 +1125,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_join_chat_channel?).with(channel)
+      Guardian.any_instance.expects(:can_preview_chat_channel?).with(channel)
 
       sign_in(Fabricate(:user))
       get "/chat/message/#{message.id}.json"
@@ -1141,7 +1141,7 @@ RSpec.describe Chat::ChatController do
     before { sign_in(user) }
 
     it "ensures message's channel can be seen" do
-      Guardian.any_instance.expects(:can_join_chat_channel?).with(channel)
+      Guardian.any_instance.expects(:can_preview_chat_channel?).with(channel)
       get "/chat/lookup/#{message.id}.json", params: { chat_channel_id: channel.id }
     end
 

plugins/chat/spec/serializer/chat/structured_channel_serializer_spec.rb

@@ -179,6 +179,7 @@ RSpec.describe Chat::StructuredChannelSerializer do
             new_mentions_message_bus_last_id: 0,
             kick_message_bus_last_id: 0,
             channel_message_bus_last_id: 0,
+            can_join_chat_channel: true,
           )
           .once
         described_class.new(data, scope: guardian).as_json

plugins/chat/spec/system/jit_messages_spec.rb

@@ -14,21 +14,74 @@ RSpec.describe "JIT messages", type: :system, js: true do
     sign_in(current_user)
   end
 
-  context "when mentioning a user not on the channel" do
-    it "displays a mention warning" do
-      Jobs.run_immediately!
+  context "when mentioning a user" do
+    context "when user is not on the channel" do
+      it "displays a mention warning" do
+        Jobs.run_immediately!
 
-      chat.visit_channel(channel_1)
-      channel.send_message("hi @#{other_user.username}")
+        chat.visit_channel(channel_1)
+        channel.send_message("hi @#{other_user.username}")
 
-      expect(page).to have_content(
-        I18n.t("js.chat.mention_warning.without_membership", username: other_user.username),
-        wait: 5,
-      )
+        expect(page).to have_content(
+          I18n.t("js.chat.mention_warning.without_membership", username: other_user.username),
+          wait: 5,
+        )
+      end
+    end
+
+    context "when user can’t access the channel" do
+      fab!(:group_1) { Fabricate(:group) }
+      fab!(:private_channel_1) { Fabricate(:private_category_channel, group: group_1) }
+
+      before do
+        group_1.add(current_user)
+        private_channel_1.add(current_user)
+      end
+
+      it "displays a mention warning" do
+        Jobs.run_immediately!
+
+        chat.visit_channel(private_channel_1)
+        channel.send_message("hi @#{other_user.username}")
+
+        expect(page).to have_content(
+          I18n.t("js.chat.mention_warning.cannot_see", username: other_user.username),
+          wait: 5,
+        )
+      end
+    end
+
+    context "when user can’t access a non read_restrictd channel" do
+      let!(:everyone) { Group.find(Group::AUTO_GROUPS[:everyone]) }
+      fab!(:category) { Fabricate(:category) }
+      fab!(:readonly_channel) { Fabricate(:category_channel, chatable: category) }
+
+      before do
+        Fabricate(
+          :category_group,
+          category: category,
+          group: everyone,
+          permission_type: CategoryGroup.permission_types[:readonly],
+        )
+        everyone.add(other_user)
+        readonly_channel.add(current_user)
+      end
+
+      it "displays a mention warning" do
+        Jobs.run_immediately!
+
+        chat.visit_channel(readonly_channel)
+        channel.send_message("hi @#{other_user.username}")
+
+        expect(page).to have_content(
+          I18n.t("js.chat.mention_warning.cannot_see", username: other_user.username),
+          wait: 5,
+        )
+      end
     end
   end
 
-  context "when mentioning a user who can’t access the channel" do
+  context "when category channel permission is readonly for everyone" do
     fab!(:group_1) { Fabricate(:group) }
     fab!(:private_channel_1) { Fabricate(:private_category_channel, group: group_1) }
 

plugins/chat/spec/system/visit_channel_spec.rb

@@ -93,6 +93,37 @@ RSpec.describe "Visit channel", type: :system, js: true do
         end
       end
 
+      context "when category channel is read-only" do
+        fab!(:restricted_category) { Fabricate(:category, read_restricted: true) }
+        fab!(:readonly_group_1) { Fabricate(:group, users: [current_user]) }
+        fab!(:readonly_category_channel_1) do
+          Fabricate(:category_channel, chatable: restricted_category)
+        end
+        fab!(:message_1) { Fabricate(:chat_message, chat_channel: readonly_category_channel_1) }
+
+        before do
+          Fabricate(
+            :category_group,
+            category: restricted_category,
+            group: readonly_group_1,
+            permission_type: CategoryGroup.permission_types[:readonly],
+          )
+        end
+
+        it "doesn't allow user to join it" do
+          chat.visit_channel(readonly_category_channel_1)
+
+          expect(page).not_to have_content(I18n.t("js.chat.channel_settings.join_channel"))
+        end
+
+        it "shows a preview of the channel" do
+          chat.visit_channel(readonly_category_channel_1)
+
+          expect(page).to have_content(readonly_category_channel_1.name)
+          expect(chat).to have_message(message_1)
+        end
+      end
+
       context "when current user is not member of the channel" do
         context "when category channel" do
           fab!(:message_1) { Fabricate(:chat_message, chat_channel: category_channel_1) }

plugins/chat/test/javascripts/components/chat-channel-preview-card-test.js

@@ -18,6 +18,7 @@ module(
 
       this.channel.description = "Important stuff is announced here.";
       this.channel.title = "announcements";
+      this.channel.meta = { can_join_chat_channel: true };
       this.currentUser.set("has_chat_enabled", true);
       this.siteSettings.chat_enabled = true;
     });

spec/lib/category_guardian_spec.rb

@@ -39,6 +39,19 @@ RSpec.describe CategoryGuardian do
         expect(Guardian.new(user).can_post_in_category?(category)).to eq(false)
       end
 
+      it "returns false if everyone has readonly access" do
+        everyone = Group.find(Group::AUTO_GROUPS[:everyone])
+        everyone.add(user)
+        category = Fabricate(:category)
+        Fabricate(
+          :category_group,
+          category: category,
+          group: everyone,
+          permission_type: CategoryGroup.permission_types[:readonly],
+        )
+        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