Allow the Administrators group to sync (#13629)

* Allow the Administrators group to sync

Allows the Administrators group to sync from LDAP or SAML providers.
This was previously not allowed to avoid a user accidentally locking
themselves out of the admin group.

Fixes metabase/metabase-enterprise#445

* test that admins are properly removed

Test that when a sync happens, if the user was previously in the administrators group and no longer is, they are properly removed from that group.

src/metabase/integrations/common.clj

@@ -11,9 +11,9 @@
 
 (defn sync-group-memberships!
   "Update the PermissionsGroups a User belongs to, adding or deleting membership entries as needed so that Users is only
-  in `new-groups-or-ids`. Ignores special groups like `admin` and `all-users`."
+  in `new-groups-or-ids`. Ignores special groups like `all-users`."
   [user-or-id new-groups-or-ids]
-  (let [special-group-ids  #{(u/get-id (group/admin)) (u/get-id (group/all-users))}
+  (let [special-group-ids  #{(u/get-id (group/all-users))}
         user-id            (u/get-id user-or-id)
         ;; Get a set of Group IDs the user currently belongs to
         current-group-ids  (db/select-field :group_id PermissionsGroupMembership

test/metabase/integrations/common_test.clj

 (ns metabase.integrations.common-test
-  (:require [expectations :refer [expect]]
+  (:require [clojure.test :refer :all]
             [metabase.integrations.common :as integrations.common]
             [metabase.models
              [permissions-group :as group :refer [PermissionsGroup]]
@@ -46,68 +46,67 @@
     (let [[user-binding groups-or-ids-to-put-user-in] bindings]
       `(do-with-user-in-groups (fn [~user-binding] ~@body) ~groups-or-ids-to-put-user-in))))
 
-;; does syncing group memberships leave existing memberships in place if nothing has changed?
-(expect
-  #{"All Users" ":metabase.integrations.common-test/group"}
-  (with-user-in-groups [group {:name (str ::group)}
-                        user  [group]]
-    (integrations.common/sync-group-memberships! user #{group})
-    (group-memberships user)))
+(deftest sync-groups-test
+  (testing "does syncing group memberships leave existing memberships in place if nothing has changed?"
+    (is (= #{"All Users" ":metabase.integrations.common-test/group"}
+           (with-user-in-groups [group {:name (str ::group)}
+                                 user  [group]]
+             (integrations.common/sync-group-memberships! user #{group})
+             (group-memberships user)))))
 
-;; the actual `PermissionsGroupMembership` object should not have been replaced
-(expect
-  (with-user-in-groups [group {:name (str ::group)}
-                        user  [group]]
-    (let [membership-id          #(db/select-one-id PermissionsGroupMembership
-                                    :group_id (u/get-id group)
-                                    :user_id  (u/get-id user))
-          original-membership-id (membership-id)]
-      (integrations.common/sync-group-memberships! user #{group})
-      (= original-membership-id
-         (membership-id)))))
+  (testing "the actual `PermissionsGroupMembership` object should not have been replaced"
+    (with-user-in-groups [group {:name (str ::group)}
+                          user  [group]]
+      (let [membership-id          #(db/select-one-id PermissionsGroupMembership
+                                                      :group_id (u/get-id group)
+                                                      :user_id  (u/get-id user))
+            original-membership-id (membership-id)]
+        (integrations.common/sync-group-memberships! user #{group})
+        (is (= original-membership-id
+               (membership-id))))))
 
-;; does syncing group memberships add users to new groups correctly?
-(expect
-  #{":metabase.integrations.common-test/group-1"
-    ":metabase.integrations.common-test/group-2"
-    "All Users"}
-  (with-user-in-groups [group-1 {:name (str ::group-1)}
-                        group-2 {:name (str ::group-2)}
-                        user    [group-1]]
-    (integrations.common/sync-group-memberships! user #{group-1 group-2})
-    (group-memberships user)))
+  (testing "does syncing group memberships add users to new groups correctly?"
+    (is (= #{":metabase.integrations.common-test/group-1"
+             ":metabase.integrations.common-test/group-2"
+             "All Users"}
+           (with-user-in-groups [group-1 {:name (str ::group-1)}
+                                 group-2 {:name (str ::group-2)}
+                                 user    [group-1]]
+             (integrations.common/sync-group-memberships! user #{group-1 group-2})
+             (group-memberships user)))))
 
-;; does syncing group memberships remove users from old groups correctly?
-(expect
-  #{"All Users"}
-  (with-user-in-groups [group-1 {:name (str ::group-1)}
-                        group-2 {:name (str ::group-2)}
-                        user    [group-1]]
-    (integrations.common/sync-group-memberships! user #{})
-    (group-memberships user)))
+  (testing "does syncing group memberships remove users from old groups correctly?"
+    (is (= #{"All Users"}
+           (with-user-in-groups [group-1 {:name (str ::group-1)}
+                                 group-2 {:name (str ::group-2)}
+                                 user    [group-1]]
+             (integrations.common/sync-group-memberships! user #{})
+             (group-memberships user)))))
 
-;; does adding & removing at the same time work correctly?
-(expect
-  #{":metabase.integrations.common-test/group-2" "All Users"}
-  (with-user-in-groups [group-1 {:name (str ::group-1)}
-                        group-2 {:name (str ::group-2)}
-                        user    [group-1]]
-    (integrations.common/sync-group-memberships! user #{group-2})
-    (group-memberships user)))
+  (testing "does adding & removing at the same time work correctly?"
+    (is (= #{":metabase.integrations.common-test/group-2" "All Users"}
+           (with-user-in-groups [group-1 {:name (str ::group-1)}
+                                 group-2 {:name (str ::group-2)}
+                                 user    [group-1]]
+             (integrations.common/sync-group-memberships! user #{group-2})
+             (group-memberships user)))))
 
-;; if we attempt to add a user to a group that doesn't exist, does the group sync complete for the other groups?
+  (testing "if we attempt to add a user to a group that doesn't exist, does the group sync complete for the other groups?"
+    (is (= #{"All Users" ":metabase.integrations.common-test/group"}
+           (with-user-in-groups [group {:name (str ::group)}
+                                 user    [group]]
+             (tu.log/suppress-output
+              (integrations.common/sync-group-memberships! user [Integer/MAX_VALUE group]))
+             (group-memberships user)))))
 
-(expect
-  #{"All Users" ":metabase.integrations.common-test/group"}
-  (with-user-in-groups [group {:name (str ::group)}
-                        user    [group]]
-    (tu.log/suppress-output
-      (integrations.common/sync-group-memberships! user [Integer/MAX_VALUE group]))
-    (group-memberships user)))
+  (testing "are admins synced?"
+    (is (= #{"Administrators" "All Users"}
+           (with-user-in-groups [user]
+             (integrations.common/sync-group-memberships! user [(group/admin)])
+             (group-memberships user)))))
 
-;; are special groups like admin & all users ignored?
-(expect
-  #{"All Users"}
-  (with-user-in-groups [user]
-    (integrations.common/sync-group-memberships! user [(group/admin)])
-    (group-memberships user)))
+  (testing "are administrators removed appropriately?"
+    (is (= #{"All Users"}
+           (with-user-in-groups [user [(group/admin)]]
+             (integrations.common/sync-group-memberships! user [])
+             (group-memberships user))))))