Catch exception during sync group (#21547)

* catch remove last admin exception during sync group

* lint and make sure the log are catched

* fix weird problem when try to re-def a macro in test and update log message

* count warn log only

* force log in tests

src/metabase/integrations/common.clj

@@ -4,7 +4,7 @@
             [clojure.set :as set]
             [clojure.tools.logging :as log]
             [metabase.models.permissions-group :as group]
-            [metabase.models.permissions-group-membership :refer [PermissionsGroupMembership]]
+            [metabase.models.permissions-group-membership :as pgm :refer [PermissionsGroupMembership]]
             [metabase.util :as u]
             [metabase.util.i18n :refer [trs]]
             [toucan.db :as db]))
@@ -28,7 +28,16 @@
     ;; remove membership from any groups as needed
     (when (seq to-remove)
       (log/debugf "Removing user %s from group(s) %s" user-id to-remove)
-      (db/delete! PermissionsGroupMembership :group_id [:in to-remove], :user_id user-id))
+      (try
+       (db/delete! PermissionsGroupMembership :group_id [:in to-remove], :user_id user-id)
+       (catch clojure.lang.ExceptionInfo e
+         ;; in case sync attempts to delete the last admin, the pre-delete hooks of
+         ;; [[metabase.models.permissions-group-membership/PermissionsGroupMembership]] will throw an exception.
+         ;; but we don't want to block user from logging-in, so catch this exception and log a warning
+         (if (= (ex-message e) (str pgm/fail-to-remove-last-admin-msg))
+           (log/warn "Attempted to remove the last admin during group sync!"
+                     "Check your SSO group mappings and make sure the Administrators group is mapped correctly.")
+           (throw e)))))
     ;; add new memberships for any groups as needed
     (doseq [id    to-add
             :when (not (excluded-group-ids id))]
@@ -36,6 +45,6 @@
       ;; if adding membership fails for one reason or another (i.e. if the group doesn't exist) log the error add the
       ;; user to the other groups rather than failing entirely
       (try
-        (db/insert! PermissionsGroupMembership :group_id id, :user_id user-id)
-        (catch Throwable e
-          (log/error e (trs "Error adding User {0} to Group {1}" user-id id)))))))
+       (db/insert! PermissionsGroupMembership :group_id id, :user_id user-id)
+       (catch Throwable e
+         (log/error e (trs "Error adding User {0} to Group {1}" user-id id)))))))

src/metabase/models/permissions_group_membership.clj

 (ns metabase.models.permissions-group-membership
   (:require [metabase.models.permissions-group :as group]
             [metabase.util :as u]
-            [metabase.util.i18n :as ui18n :refer [tru]]
+            [metabase.util.i18n :as ui18n :refer [deferred-tru tru]]
             [toucan.db :as db]
             [toucan.models :as models]))
 
 (models/defmodel PermissionsGroupMembership :permissions_group_membership)
 
+(def fail-to-remove-last-admin-msg
+  "Exception message when try to remove the last admin."
+  (deferred-tru "You cannot remove the last member of the ''Admin'' group!"))
+
 (defonce ^:dynamic ^{:doc "Should we allow people to be added to or removed from the All Users permissions group? By
   default, this is `false`, but enable it when adding or deleting users."}
   *allow-changing-all-users-group-members*
@@ -24,8 +28,8 @@
   (when (<= (db/count PermissionsGroupMembership
               :group_id (:id (group/admin)))
             1)
-    (throw (ex-info (tru "You cannot remove the last member of the ''Admin'' group!")
-             {:status-code 400}))))
+    (throw (ex-info (str fail-to-remove-last-admin-msg)
+                    {:status-code 400}))))
 
 (defn- pre-delete [{:keys [group_id user_id]}]
   (check-not-all-users-group group_id)

test/metabase/integrations/common_test.clj

 (ns metabase.integrations.common-test
   (:require [clojure.test :refer :all]
+            [clojure.tools.logging :as log]
             [metabase.integrations.common :as integrations.common]
             [metabase.models.permissions-group :as group :refer [PermissionsGroup]]
-            [metabase.models.permissions-group-membership :refer [PermissionsGroupMembership]]
+            [metabase.models.permissions-group-membership :as pgm :refer [PermissionsGroupMembership]]
             [metabase.test :as mt :refer [with-user-in-groups]]
             [metabase.test.fixtures :as fixtures]
             [metabase.util :as u]
@@ -100,4 +101,22 @@
         (with-user-in-groups [user []]
           (integrations.common/sync-group-memberships! user #{(group/admin)} #{(group/admin)})
           (is (= #{"All Users" "Administrators"}
-                 (group-memberships user))))))))
+                 (group-memberships user)))))))
+
+  (testing "Make sure the delete last admin exception is catched"
+    (mt/with-log-level :warn
+      (with-user-in-groups [user [(group/admin)]]
+        (let [log-warn-count (atom #{})]
+          (with-redefs [db/delete! (fn [model & _args]
+                                     (when (= model PermissionsGroupMembership)
+                                       (throw (ex-info (str pgm/fail-to-remove-last-admin-msg)
+                                                       {:status-code 400}))))
+                        log/log*   (fn [_logger level _throwable msg]
+                                     (when (:= level :warn)
+                                       (swap! log-warn-count conj msg)))]
+            ;; make sure sync run without throwing exception
+            (integrations.common/sync-group-memberships! user #{} #{(group/admin)})
+            ;; make sure we log a msg for that
+            (is (@log-warn-count
+                 (str "Attempted to remove the last admin during group sync! "
+                      "Check your SSO group mappings and make sure the Administrators group is mapped correctly.")))))))))