FIX: Log user addition/deletion from groups when they're changed via DiscourseConnect (#18677)

Discourse Connect can be used to manage group memberships of users by including a `add_groups`, `remove_groups` or `groups` attribute in the Discourse Connect payload. However, additions/deletions of users from groups aren't logged to the groups logs (available at `/g/<group>/manage/logs`) which can cause confusions to admins they try to figure out when/how users were added or removed from a group. This commit makes Discourse Connect add entries to the groups logs when it makes changes to users' group memberships.

app/models/discourse_connect.rb

@@ -186,22 +186,23 @@ class DiscourseConnect < DiscourseConnectBase
 
   def synchronize_groups(user)
     names = (groups || "").split(",").map(&:downcase)
-    ids = Group.where('LOWER(NAME) in (?) AND NOT automatic', names).pluck(:id)
 
-    group_users = GroupUser
+    to_be_added = Group.where('LOWER(NAME) in (?) AND NOT automatic', names)
-      .where('group_id IN (SELECT id FROM groups WHERE NOT automatic)')
+    to_be_removed = Group.joins(:group_users).where(automatic: false, group_users: { user_id: user.id })
-      .where(user_id: user.id)
 
-    delete_group_users = group_users
+    if to_be_added.present?
-    if ids.length > 0
+      to_be_removed = to_be_removed.where("groups.id NOT IN (?)", to_be_added.map(&:id)).to_a
-      delete_group_users = group_users.where('group_id NOT IN (?)', ids)
     end
-    delete_group_users.destroy_all
 
-    ids -= group_users.where('group_id IN (?)', ids).pluck(:group_id)
+    if to_be_removed.present?
+      to_be_added = to_be_added.where("groups.id NOT IN (?)", to_be_removed.map(&:id)).to_a
+    end
 
-    ids.each do |group_id|
+    if to_be_added.present? || to_be_removed.present?
-      GroupUser.create(group_id: group_id, user_id: user.id)
+      GroupUser.transaction do
+        add_user_to_groups(user, to_be_added) if to_be_added.present?
+        remove_user_from_groups(user, to_be_removed) if to_be_removed.present?
+      end
     end
   end
 
@@ -211,24 +212,32 @@ class DiscourseConnect < DiscourseConnectBase
       return
     end
 
+    to_be_added = nil
     if add_groups
       split = add_groups.split(",").map(&:downcase)
       if split.length > 0
-        Group.where('LOWER(name) in (?) AND NOT automatic', split).pluck(:id).each do |id|
+        to_be_added = Group.where('LOWER(name) in (?) AND NOT automatic', split)
-          unless GroupUser.where(group_id: id, user_id: user.id).exists?
+        if already_member = GroupUser.where(user_id: user.id).pluck(:group_id).presence
-            GroupUser.create(group_id: id, user_id: user.id)
+          to_be_added = to_be_added.where("id NOT IN (?)", already_member)
-          end
         end
       end
     end
 
+    to_be_removed = nil
     if remove_groups
       split = remove_groups.split(",").map(&:downcase)
       if split.length > 0
-        GroupUser
+        to_be_removed = Group
-          .where(user_id: user.id)
+          .joins(:group_users)
-          .where('group_id IN (SELECT id FROM groups WHERE LOWER(name) in (?))', split)
+          .where(automatic: false, group_users: { user_id: user.id })
-          .destroy_all
+          .where("LOWER(name) IN (?)", split)
+      end
+    end
+
+    if to_be_added || to_be_removed
+      GroupUser.transaction do
+        add_user_to_groups(user, to_be_added) if to_be_added
+        remove_user_from_groups(user, to_be_removed) if to_be_removed
       end
     end
   end
@@ -394,4 +403,18 @@ class DiscourseConnect < DiscourseConnectBase
 
     name.presence || User.suggest_name(name_suggester_input)
   end
+
+  def add_user_to_groups(user, groups)
+    groups.each do |group|
+      GroupUser.create!(user_id: user.id, group_id: group.id)
+      GroupActionLogger.new(Discourse.system_user, group).log_add_user_to_group(user)
+    end
+  end
+
+  def remove_user_from_groups(user, groups)
+    GroupUser.where(user_id: user.id, group_id: groups.map(&:id)).destroy_all
+    groups.each do |group|
+      GroupActionLogger.new(Discourse.system_user, group).log_remove_user_from_group(user)
+    end
+  end
 end

spec/models/discourse_connect_spec.rb

@@ -263,6 +263,104 @@ RSpec.describe DiscourseConnect do
     expect(add_group4.usernames).to eq(user.username)
   end
 
+  it 'creates group logs when users are added to groups' do
+    user = Fabricate(:user)
+    group = Fabricate(:group, name: 'group1')
+
+    sso = new_discourse_sso
+    sso.username = "bobsky"
+    sso.name = "Bob"
+    sso.email = user.email
+    sso.external_id = "A"
+    sso.add_groups = "#{group.name},badname"
+
+    GroupHistory.destroy_all
+
+    sso.lookup_or_create_user(ip_address)
+
+    expect(group.reload.usernames).to eq(user.username)
+    expect(GroupHistory.exists?(
+      target_user_id: user.id,
+      acting_user: Discourse.system_user.id,
+      action: GroupHistory.actions[:add_user_to_group]
+    )).to eq(true)
+  end
+
+  it 'creates group logs when users are removed from groups' do
+    user = Fabricate(:user)
+    group = Fabricate(:group, name: 'group1')
+    group.add(user)
+
+    sso = new_discourse_sso
+    sso.username = "bobsky"
+    sso.name = "Bob"
+    sso.email = user.email
+    sso.external_id = "A"
+    sso.remove_groups = "#{group.name},badname"
+
+    GroupHistory.destroy_all
+
+    sso.lookup_or_create_user(ip_address)
+
+    expect(group.reload.usernames).to be_blank
+    expect(GroupHistory.exists?(
+      target_user_id: user.id,
+      acting_user: Discourse.system_user.id,
+      action: GroupHistory.actions[:remove_user_from_group]
+    )).to eq(true)
+  end
+
+  it 'creates group logs when users are added to groups when discourse_connect_overrides_groups setting is true' do
+    SiteSetting.discourse_connect_overrides_groups = true
+
+    user = Fabricate(:user)
+    group = Fabricate(:group, name: 'group1')
+
+    sso = new_discourse_sso
+    sso.username = "bobsky"
+    sso.name = "Bob"
+    sso.email = user.email
+    sso.external_id = "A"
+    sso.groups = "#{group.name},badname"
+
+    GroupHistory.destroy_all
+
+    sso.lookup_or_create_user(ip_address)
+
+    expect(group.reload.usernames).to eq(user.username)
+    expect(GroupHistory.exists?(
+      target_user_id: user.id,
+      acting_user: Discourse.system_user.id,
+      action: GroupHistory.actions[:add_user_to_group]
+    )).to eq(true)
+  end
+
+  it 'creates group logs when users are removed from groups when discourse_connect_overrides_groups setting is true' do
+    SiteSetting.discourse_connect_overrides_groups = true
+
+    user = Fabricate(:user)
+    group = Fabricate(:group, name: 'group1')
+    group.add(user)
+
+    sso = new_discourse_sso
+    sso.username = "bobsky"
+    sso.name = "Bob"
+    sso.email = user.email
+    sso.external_id = "A"
+    sso.groups = "badname"
+
+    GroupHistory.destroy_all
+
+    sso.lookup_or_create_user(ip_address)
+
+    expect(group.reload.usernames).to be_blank
+    expect(GroupHistory.exists?(
+      target_user_id: user.id,
+      acting_user: Discourse.system_user.id,
+      action: GroupHistory.actions[:remove_user_from_group]
+    )).to eq(true)
+  end
+
   it 'behaves properly when auth_overrides_username is set but username is missing or blank' do
     SiteSetting.auth_overrides_username = true