From e120c942361f20ca727d6b806863d9f81b657f98 Mon Sep 17 00:00:00 2001
From: Osama Sayegh <asooomaasoooma90@gmail.com>
Date: Fri, 28 Oct 2022 13:27:12 +0300
Subject: [PATCH] FIX: Don't attempt to add user again to a group when syncing
 groups via SSO (#18772)

This commit fixes a regression introduced in 8979adc where under certain conditions the groups syncing logic in Discourse Connect would try to add users to groups they're already members of and cause errors when users try to sign in using Discourse Connect.
---
 app/models/discourse_connect.rb       | 22 ++++++++++------------
 spec/models/discourse_connect_spec.rb | 12 +++++++++++-
 2 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/app/models/discourse_connect.rb b/app/models/discourse_connect.rb
index ff256985858..ede77372f82 100644
--- a/app/models/discourse_connect.rb
+++ b/app/models/discourse_connect.rb
@@ -187,15 +187,17 @@ class DiscourseConnect < DiscourseConnectBase
   def synchronize_groups(user)
     names = (groups || "").split(",").map(&:downcase)
 
-    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 })
+    current_groups = user.groups.where(automatic: false)
+    desired_groups = Group.where('LOWER(NAME) in (?) AND NOT automatic', names)
 
-    if to_be_added.present?
-      to_be_removed = to_be_removed.where("groups.id NOT IN (?)", to_be_added.map(&:id)).to_a
+    to_be_added = desired_groups
+    if current_groups.present?
+      to_be_added = to_be_added.where("groups.id NOT IN (?)", current_groups.map(&:id))
     end
 
-    if to_be_removed.present?
-      to_be_added = to_be_added.where("groups.id NOT IN (?)", to_be_removed.map(&:id)).to_a
+    to_be_removed = current_groups
+    if desired_groups.present?
+      to_be_removed = to_be_removed.where("groups.id NOT IN (?)", desired_groups.map(&:id))
     end
 
     if to_be_added.present? || to_be_removed.present?
@@ -406,12 +408,8 @@ class DiscourseConnect < DiscourseConnectBase
 
   def add_user_to_groups(user, groups)
     groups.each do |group|
-      begin
-        GroupUser.create!(user_id: user.id, group_id: group.id)
-        GroupActionLogger.new(Discourse.system_user, group).log_add_user_to_group(user)
-      rescue Exception => e
-        Discourse.warn_exception(e, message: "User already in group")
-      end
+      GroupUser.create!(user_id: user.id, group_id: group.id)
+      GroupActionLogger.new(Discourse.system_user, group).log_add_user_to_group(user)
     end
   end
 
diff --git a/spec/models/discourse_connect_spec.rb b/spec/models/discourse_connect_spec.rb
index a55a50b519a..81a679ba33e 100644
--- a/spec/models/discourse_connect_spec.rb
+++ b/spec/models/discourse_connect_spec.rb
@@ -206,7 +206,7 @@ RSpec.describe DiscourseConnect do
     expect(group2.usernames).to eq("")
 
     group1.add(user)
-    group1.save
+    group1.save!
 
     sso.lookup_or_create_user(ip_address)
     expect(group1.usernames).to eq("")
@@ -216,6 +216,16 @@ RSpec.describe DiscourseConnect do
     sso.lookup_or_create_user(ip_address)
     expect(group1.usernames).to eq("")
     expect(group2.usernames).to eq("")
+
+    group1.add(user)
+    group1.save!
+    group2.add(user)
+    group2.save!
+    sso.groups = "#{group1.name},badname,trust_level_4"
+
+    sso.lookup_or_create_user(ip_address)
+    expect(group1.usernames).to eq(user.username)
+    expect(group2.usernames).to eq("")
   end
 
   it "can specify groups" do