Revert "Allow the Administrators group to sync (#13629)" (#13791)

This reverts commit 3e1dab30.

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 `all-users`."
+  in `new-groups-or-ids`. Ignores special groups like `admin` and `all-users`."
   [user-or-id new-groups-or-ids]
-  (let [special-group-ids  #{(u/get-id (group/all-users))}
+  (let [special-group-ids  #{(u/get-id (group/admin)) (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 [clojure.test :refer :all]
+  (:require [expectations :refer [expect]]
             [metabase.integrations.common :as integrations.common]
             [metabase.models
              [permissions-group :as group :refer [PermissionsGroup]]
@@ -46,67 +46,68 @@
     (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))))
 
-(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)))))
+;; 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)))
 
-  (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))))))
+;; 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 "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 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 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 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 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)))))
+;; 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 "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)))))
+;; if we attempt to add a user to a group that doesn't exist, does the group sync complete for the other groups?
 
-  (testing "are admins synced?"
-    (is (= #{"Administrators" "All Users"}
-           (with-user-in-groups [user]
-             (integrations.common/sync-group-memberships! user [(group/admin)])
-             (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 administrators removed appropriately?"
-    (is (= #{"All Users"}
-           (with-user-in-groups [user [(group/admin)]]
-             (integrations.common/sync-group-memberships! user [])
-             (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)))