From 8979adc3afa4cb6c0cd3b47a8d1172583a01a510 Mon Sep 17 00:00:00 2001 From: Osama Sayegh Date: Tue, 25 Oct 2022 11:25:26 +0300 Subject: [PATCH] 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//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 | 61 +++++++++++------ spec/models/discourse_connect_spec.rb | 98 +++++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 19 deletions(-) diff --git a/app/models/discourse_connect.rb b/app/models/discourse_connect.rb index 91e0181a349..fcdae32a4ff 100644 --- a/app/models/discourse_connect.rb +++ b/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 - .where('group_id IN (SELECT id FROM groups WHERE NOT automatic)') - .where(user_id: user.id) + to_be_added = Group.where('LOWER(NAME) in (?) AND NOT automatic', names) + to_be_removed = Group.joins(:group_users).where(automatic: false, group_users: { user_id: user.id }) - delete_group_users = group_users - if ids.length > 0 - delete_group_users = group_users.where('group_id NOT IN (?)', ids) + if to_be_added.present? + to_be_removed = to_be_removed.where("groups.id NOT IN (?)", to_be_added.map(&:id)).to_a 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| - GroupUser.create(group_id: group_id, user_id: user.id) + if to_be_added.present? || to_be_removed.present? + 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| - unless GroupUser.where(group_id: id, user_id: user.id).exists? - GroupUser.create(group_id: id, user_id: user.id) - end + to_be_added = Group.where('LOWER(name) in (?) AND NOT automatic', split) + if already_member = GroupUser.where(user_id: user.id).pluck(:group_id).presence + to_be_added = to_be_added.where("id NOT IN (?)", already_member) end end end + to_be_removed = nil if remove_groups split = remove_groups.split(",").map(&:downcase) if split.length > 0 - GroupUser - .where(user_id: user.id) - .where('group_id IN (SELECT id FROM groups WHERE LOWER(name) in (?))', split) - .destroy_all + to_be_removed = Group + .joins(:group_users) + .where(automatic: false, group_users: { user_id: user.id }) + .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 diff --git a/spec/models/discourse_connect_spec.rb b/spec/models/discourse_connect_spec.rb index fa2de7795ed..a55a50b519a 100644 --- a/spec/models/discourse_connect_spec.rb +++ b/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