Skip to content

Commit

Permalink
FEATURE: Auto-remove users without permission from channel (discourse…
Browse files Browse the repository at this point in the history
…#20344)

There are many situations that may cause users to lose permission to
send messages in a chat channel. Until now we have relied on security
checks in `Chat::ChatChannelFetcher` to remove channels which the
user may have a `UserChatChannelMembership` record for but which
they do not have access to.

This commit takes a more proactive approach. Now any of these following
`DiscourseEvent` triggers may cause `UserChatChannelMembership`
records to be deleted:

* `category_updated` - Permissions of the category changed
   (i.e. CategoryGroup records changed)
* `user_removed_from_group` - Means the user may not be able to access the
   channel based on `GroupUser` or also `chat_allowed_groups`
* `site_setting_changed` - The `chat_allowed_groups` was updated, some
   users may no longer be in groups that can access chat.
* `group_destroyed` - Means the user may not be able to access the
   channel based on `GroupUser` or also `chat_allowed_groups`

All of these are handled in a distinct service run in a background
job. Users removed are logged via `StaffActionLog` and then we
publish messages on a per-channel basis to users who had their
memberships deleted.

When the user has a channel they are kicked from open, we show
a dialog saying "You no longer have access to this channel".

When they click OK we redirect them either:

* To their first other public channel, if they have any followed
* The chat browse page if they don't

This is to save on tons of requests from kicked out users getting messages
from other channels.

When the user does not have the kicked channel open, we can just
silently yoink it out of their sidebar and turn off subscriptions.
  • Loading branch information
martin-brennan authored Mar 22, 2023
1 parent b5c4e1b commit 520d4f5
Show file tree
Hide file tree
Showing 38 changed files with 2,174 additions and 19 deletions.
12 changes: 11 additions & 1 deletion app/models/group.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ class Group < ActiveRecord::Base
after_commit :automatic_group_membership, on: %i[create update]
after_commit :trigger_group_created_event, on: :create
after_commit :trigger_group_updated_event, on: :update
before_destroy :cache_group_users_for_destroyed_event, prepend: true
after_commit :trigger_group_destroyed_event, on: :destroy
after_commit :set_default_notifications, on: %i[create update]

Expand Down Expand Up @@ -911,13 +912,22 @@ def self.owner_of(groups, user)
self.member_of(groups, user).where("gu.owner")
end

%i[group_created group_updated group_destroyed].each do |event|
def cache_group_users_for_destroyed_event
@cached_group_user_ids = group_users.pluck(:user_id)
end

%i[group_created group_updated].each do |event|
define_method("trigger_#{event}_event") do
DiscourseEvent.trigger(event, self)
true
end
end

def trigger_group_destroyed_event
DiscourseEvent.trigger(:group_destroyed, self, @cached_group_user_ids)
true
end

def flair_type
return :icon if flair_icon.present?
return :image if flair_upload.present?
Expand Down
25 changes: 25 additions & 0 deletions lib/job_time_spacer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# frozen_string_literal: true

##
# In some cases we may want to enqueue_at several of the same job with
# batches, spacing them out or incrementing by some amount of seconds,
# in case the jobs do heavy work or send many MessageBus messages and the like.
# This class handles figuring out the seconds increments.
#
# @example
# spacer = JobTimeSpacer.new
# user_ids.in_groups_of(200, false) do |user_id_batch|
# spacer.enqueue(:kick_users_from_topic, { topic_id: topic_id, user_ids: user_id_batch })
# end
class JobTimeSpacer
def initialize(seconds_space_increment: 1, seconds_delay: 5)
@seconds_space_increment = seconds_space_increment
@seconds_space_modifier = seconds_space_increment
@seconds_step = seconds_delay
end

def enqueue(job_name, job_args = {})
Jobs.enqueue_at((@seconds_step * @seconds_space_modifier).seconds.from_now, job_name, job_args)
@seconds_space_modifier += @seconds_space_increment
end
end
81 changes: 81 additions & 0 deletions plugins/chat/app/jobs/regular/auto_join_channel_batch.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
# NOTE: When changing auto-join logic, make sure to update the `settings.auto_join_users_info` translation as well.
# frozen_string_literal: true

module Jobs
class AutoJoinChannelBatch < ::Jobs::Base
def execute(args)
return "starts_at or ends_at missing" if args[:starts_at].blank? || args[:ends_at].blank?
start_user_id = args[:starts_at].to_i
end_user_id = args[:ends_at].to_i

return "End is higher than start" if end_user_id < start_user_id

channel =
ChatChannel.find_by(
id: args[:chat_channel_id],
auto_join_users: true,
chatable_type: "Category",
)

return if !channel

category = channel.chatable
return if !category

query_args = {
chat_channel_id: channel.id,
start: start_user_id,
end: end_user_id,
suspended_until: Time.zone.now,
last_seen_at: 3.months.ago,
channel_category: channel.chatable_id,
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
if start_user_id == end_user_id
Chat::ChatChannelMembershipManager.new(channel).recalculate_user_count
end

Chat::Publisher.publish_new_channel(channel.reload, User.where(id: new_member_ids))
end

private

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
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
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ def execute(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::Chat::AutoManageChannelMemberships
# complete for the channel in Jobs::Chat::AutoJoinChannelMemberships
if start_user_id == end_user_id
::Chat::ChannelMembershipManager.new(channel).recalculate_user_count
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

module Jobs
module Chat
class AutoManageChannelMemberships < ::Jobs::Base
class AutoJoinChannelMemberships < ::Jobs::Base
def execute(args)
channel =
::Chat::Channel.includes(:chatable).find_by(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# frozen_string_literal: true

module Jobs
module Chat
class AutoRemoveMembershipHandleCategoryUpdated < ::Jobs::Base
def execute(args)
::Chat::AutoRemove::HandleCategoryUpdated.call(**args)
end
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# frozen_string_literal: true

module Jobs
module Chat
class AutoRemoveMembershipHandleChatAllowedGroupsChange < ::Jobs::Base
def execute(args)
::Chat::AutoRemove::HandleChatAllowedGroupsChange.call(**args)
end
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# frozen_string_literal: true

module Jobs
module Chat
class AutoRemoveMembershipHandleDestroyedGroup < ::Jobs::Base
def execute(args)
::Chat::AutoRemove::HandleDestroyedGroup.call(**args)
end
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# frozen_string_literal: true

module Jobs
module Chat
class AutoRemoveMembershipHandleUserRemovedFromGroup < ::Jobs::Base
def execute(args)
::Chat::AutoRemove::HandleUserRemovedFromGroup.call(**args)
end
end
end
end
13 changes: 13 additions & 0 deletions plugins/chat/app/jobs/regular/chat/kick_users_from_channel.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# frozen_string_literal: true

module Jobs
module Chat
class KickUsersFromChannel < Jobs::Base
def execute(args)
return if !::Chat::Channel.exists?(id: args[:channel_id])
return if args[:user_ids].blank?
::Chat::Publisher.publish_kick_users(args[:channel_id], args[:user_ids])
end
end
end
end
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# frozen_string_literal: true

module Chat
class UserChatChannelMembership < ActiveRecord::Base
class Chat::UserChatChannelMembership < ActiveRecord::Base
self.table_name = "user_chat_channel_memberships"

NOTIFICATION_LEVELS = { never: 0, mention: 1, always: 2 }
Expand Down
26 changes: 20 additions & 6 deletions plugins/chat/app/serializers/chat/channel_serializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -112,12 +112,9 @@ def meta
{
message_bus_last_ids: {
channel_message_bus_last_id: MessageBus.last_id("/chat/#{object.id}"),
new_messages:
@opts[:new_messages_message_bus_last_id] ||
MessageBus.last_id(Chat::Publisher.new_messages_message_bus_channel(object.id)),
new_mentions:
@opts[:new_mentions_message_bus_last_id] ||
MessageBus.last_id(Chat::Publisher.new_mentions_message_bus_channel(object.id)),
new_messages: new_messages_message_bus_id,
new_mentions: new_mentions_message_bus_id,
kick: kick_message_bus_id,
},
}
end
Expand All @@ -127,5 +124,22 @@ def meta
alias_method :include_archived_messages?, :include_archive_status?
alias_method :include_archive_failed?, :include_archive_status?
alias_method :include_archive_completed?, :include_archive_status?

private

def new_messages_message_bus_id
@opts[:new_messages_message_bus_last_id] ||
MessageBus.last_id(Chat::Publisher.new_messages_message_bus_channel(object.id))
end

def new_mentions_message_bus_id
@opts[:new_mentions_message_bus_last_id] ||
MessageBus.last_id(Chat::Publisher.new_mentions_message_bus_channel(object.id))
end

def kick_message_bus_id
@opts[:kick_message_bus_last_id] ||
MessageBus.last_id(Chat::Publisher.kick_users_message_bus_channel(object.id))
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ def direct_message_channels
chat_message_bus_last_ids[Chat::Publisher.new_messages_message_bus_channel(channel.id)],
new_mentions_message_bus_last_id:
chat_message_bus_last_ids[Chat::Publisher.new_mentions_message_bus_channel(channel.id)],
kick_message_bus_last_id:
chat_message_bus_last_ids[Chat::Publisher.kick_users_message_bus_channel(channel.id)],
)
end
end
Expand Down Expand Up @@ -84,6 +86,7 @@ def chat_message_bus_last_ids
object[:public_channels].each do |channel|
message_bus_channels.push(Chat::Publisher.new_messages_message_bus_channel(channel.id))
message_bus_channels.push(Chat::Publisher.new_mentions_message_bus_channel(channel.id))
message_bus_channels.push(Chat::Publisher.kick_users_message_bus_channel(channel.id))
end

object[:direct_message_channels].each do |channel|
Expand Down
Loading

0 comments on commit 520d4f5

Please sign in to comment.