Tests for user/group-ids and other new functions

src/metabase/api/permissions.clj

@@ -89,13 +89,15 @@
   "Return a map of `PermissionsGroup` ID -> number of members in the group. (This doesn't include entries for empty
   groups.)"
   []
-  (into {} (for [{:keys [group_id members]} (db/query
-                                             {:select    [[:pgm.group_id :group_id] [:%count.pgm.id :members]]
-                                              :from      [[:permissions_group_membership :pgm]]
-                                              :left-join [[:core_user :user] [:= :pgm.user_id :user.id]]
-                                              :where     [:= :user.is_active true]
-                                              :group-by  [:pgm.group_id]})]
-             {group_id members})))
+  (let [results (db/query
+                 {:select    [[:pgm.group_id :group_id] [:%count.pgm.id :members]]
+                  :from      [[:permissions_group_membership :pgm]]
+                  :left-join [[:core_user :user] [:= :pgm.user_id :user.id]]
+                  :where     [:= :user.is_active true]
+                  :group-by  [:pgm.group_id]})]
+    (zipmap
+     (map :group_id results)
+     (map :members results))))
 
 (defn- ordered-groups
   "Return a sequence of ordered `PermissionsGroups`, including the `MetaBot` group only if MetaBot is enabled."
@@ -106,14 +108,19 @@
                  [:not= :id (u/get-id (group/metabot))])
      :order-by [:%lower.name]}))
 
+(defn- add-member-counts
+  {:batched-hydrate :member_count}
+  [groups]
+  (let [group-id->num-members (group-id->num-members)]
+    (for [group groups]
+      (assoc group :member_count (get group-id->num-members (u/get-id group) 0)))))
+
 (api/defendpoint GET "/group"
   "Fetch all `PermissionsGroups`, including a count of the number of `:members` in that group."
   []
   (api/check-superuser)
-  (let [group-id->members (group-id->num-members)]
-    (for [group (ordered-groups)]
-      (assoc group :member_count (or (group-id->members (u/get-id group))
-                                     0)))))
+  (-> (ordered-groups)
+      (hydrate :member_count)))
 
 (api/defendpoint GET "/group/:id"
   "Fetch the details for a certain permissions group."

src/metabase/api/user.clj

@@ -65,7 +65,7 @@
     (when-not (= (user/group-ids user-or-id)
                  (set (map u/get-id new-groups-or-ids)))
       (api/check-superuser)
-      (user/set-user-permissions-groups! user-or-id new-groups-or-ids))))
+      (user/set-permissions-groups! user-or-id new-groups-or-ids))))
 
 (api/defendpoint POST "/"
   "Create a new `User`, return a 400 if the email address is already taken"
@@ -159,7 +159,7 @@
   (api/let-404 [user (db/select-one [User :password_salt :password], :id id, :is_active true)]
     ;; admins are allowed to reset anyone's password (in the admin people list) so no need to check the value of
     ;; `old_password` for them regular users have to know their password, however
-    (when-not (:is_superuser @api/*current-user*)
+    (when-not api/*is-superuser?*
       (api/checkp (creds/bcrypt-verify (str (:password_salt user) old_password) (:password user))
         "old_password"
         (tru "Invalid password"))))

src/metabase/models/collection.clj

@@ -794,10 +794,11 @@
 
 ;;; ----------------------------------------------------- DELETE -----------------------------------------------------
 
-(def ^:dynamic *allow-deleting-personal-collections*
-  "Whether to allow deleting Personal Collections. Normally we should *never* allow this, but in the single case of
-  deleting a User themselves, we need to allow this. (Note that in normal usage, Users never get deleted, but rather
-  archived; thus this code is used solely by our test suite, by things such as the `with-temp` macros.)"
+(defonce ^:dynamic ^{:doc "Whether to allow deleting Personal Collections. Normally we should *never* allow this, but
+  in the single case of deleting a User themselves, we need to allow this. (Note that in normal usage, Users never get
+  deleted, but rather archived; thus this code is used solely by our test suite, by things such as the `with-temp`
+  macros.)"}
+  *allow-deleting-personal-collections*
   false)
 
 (defn- pre-delete [collection]

src/metabase/models/permissions_group_membership.clj

@@ -15,9 +15,9 @@
     (throw (ui18n/ex-info (tru "You cannot add or remove users to/from the ''MetaBot'' group.")
              {:status-code 400}))))
 
-(def ^:dynamic ^Boolean *allow-changing-all-users-group-members*
-  "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."
+(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*
   false)
 
 (defn- check-not-all-users-group
@@ -53,7 +53,8 @@
 
 (defn- post-insert [{:keys [group_id user_id], :as membership}]
   (u/prog1 membership
-    ;; If we're adding a user to the admin group, set athe `:is_superuser` flag for the user to whom membership was granted
+    ;; If we're adding a user to the admin group, set athe `:is_superuser` flag for the user to whom membership was
+    ;; granted
     (when (= group_id (:id (group/admin)))
       (db/update! 'User user_id
         :is_superuser true))))

src/metabase/models/user.clj

@@ -15,7 +15,7 @@
              [permissions-group-membership :as perm-membership :refer [PermissionsGroupMembership]]]
             [metabase.util
              [date :as du]
-             [i18n :refer [tru]]
+             [i18n :refer [trs tru]]
              [schema :as su]]
             [schema.core :as s]
             [toucan
@@ -52,12 +52,12 @@
   (u/prog1 user
     ;; add the newly created user to the magic perms groups
     (binding [perm-membership/*allow-changing-all-users-group-members* true]
-      (log/info (format "Adding user %d to All Users permissions group..." user-id))
+      (log/info (trs "Adding User {0} to All Users permissions group..." user-id))
       (db/insert! PermissionsGroupMembership
         :user_id  user-id
         :group_id (:id (group/all-users))))
     (when superuser?
-      (log/info (format "Adding user %d to Admin permissions group..." user-id))
+      (log/info (trs "Adding User {0} to Admin permissions group..." user-id))
       (db/insert! PermissionsGroupMembership
         :user_id  user-id
         :group_id (:id (group/admin))))))
@@ -70,13 +70,19 @@
                                :user_id  id)]
       (cond
         (and is_superuser
-             (not membership-exists?)) (db/insert! PermissionsGroupMembership
-                                         :group_id (:id (group/admin))
-                                         :user_id  id)
+             (not membership-exists?))
+        (db/insert! PermissionsGroupMembership
+          :group_id (u/get-id (group/admin))
+          :user_id  id)
+
+        ;; don't use `delete!` here because that does the opposite and tries to update this user
+        ;; which leads to a stack overflow of calls between the two
+        ;; TODO - could we fix this issue by using `post-delete!`?
         (and (not is_superuser)
-             membership-exists?)       (db/simple-delete! PermissionsGroupMembership ; don't use `delete!` here because that does the opposite and tries to update this user
-                                         :group_id (:id (group/admin))               ; which leads to a stack overflow of calls between the two
-                                         :user_id  id))))                            ; TODO - could we fix this issue by using `post-delete!`?
+             membership-exists?)
+        (db/simple-delete! PermissionsGroupMembership
+          :group_id (u/get-id (group/admin))
+          :user_id  id))))
   (when email
     (assert (u/email? email)))
   ;; If we're setting the reset_token then encrypt it before it goes into the DB
@@ -137,19 +143,22 @@
           :pre-delete     pre-delete
           :types          (constantly {:login_attributes :json-no-keywordization})}))
 
-(defn ^{:hydrate :group_ids} group-ids
+(defn group-ids
   "Fetch set of IDs of PermissionsGroup a User belongs to."
   [user-or-id]
-  (db/select-field :group_id PermissionsGroupMembership :user_id (u/get-id user-or-id)))
+  (when user-or-id
+    (db/select-field :group_id PermissionsGroupMembership :user_id (u/get-id user-or-id))))
 
-(defn ^{:batched-hydrate :group_ids} add-group-ids
+(defn add-group-ids
   "Efficiently add PermissionsGroup `group_ids` to a collection of `users`."
+  {:batched-hydrate :group_ids}
   [users]
-  (let [user-id->memberships (when (seq users)
-                               (group-by :user_id (db/select [PermissionsGroupMembership :user_id :group_id]
-                                                    :user_id [:in (set (map u/get-id users))])))]
-    (for [user users]
-      (assoc user :group_ids (map :group_id (user-id->memberships (u/get-id user)))))))
+  (when (seq users)
+    (let [user-id->memberships (group-by :user_id (db/select [PermissionsGroupMembership :user_id :group_id]
+                                                    :user_id [:in (set (map u/get-id users))]))]
+      (for [user users]
+        (assoc user :group_ids (set (map :group_id (user-id->memberships (u/get-id user)))))))))
+
 
 ;;; --------------------------------------------------- Helper Fns ---------------------------------------------------
 
@@ -238,17 +247,25 @@
   {:pre [(string? reset-token)]}
   (str (public-settings/site-url) "/auth/reset_password/" reset-token))
 
-(defn set-user-permissions-groups!
-  "Set the user's group memberships to equal the supplied group IDs."
+(defn set-permissions-groups!
+  "Set the user's group memberships to equal the supplied group IDs. Returns `true` if updates were made, `nil`
+  otherwise."
   [user-or-id new-groups-or-ids]
-  (let [old-group-ids      (group-ids user-or-id)
+  (let [user-id            (u/get-id user-or-id)
+        old-group-ids      (group-ids user-id)
         new-group-ids      (set (map u/get-id new-groups-or-ids))
         [to-remove to-add] (data/diff old-group-ids new-group-ids)]
-    (when (seq to-remove)
-      (db/delete! PermissionsGroupMembership :user_id (u/get-id user-or-id), :group_id [:in to-remove]))
-    (when (seq to-add)
-      (db/insert-many! PermissionsGroupMembership (for [group-id to-add]
-                                                    {:user_id (u/get-id user-or-id), :group_id group-id})))))
+    (when (seq (concat to-remove to-add))
+      (db/transaction
+        (when (seq to-remove)
+          (db/delete! PermissionsGroupMembership :user_id user-id, :group_id [:in to-remove]))
+        ;; a little inefficient, but we need to do a separate `insert!` for each group we're adding membership to,
+        ;; because `insert-many!` does not currently trigger methods such as `pre-insert`. We rely on those methods to
+        ;; do things like automatically set the `is_superuser` flag for a User
+        (doseq [group-id to-add]
+          (db/insert! PermissionsGroupMembership {:user_id user-id, :group_id group-id})))
+      true)))
+
 
 ;;; -------------------------------------------------- Permissions ---------------------------------------------------
 

test/metabase/api/user_test.clj

@@ -8,6 +8,7 @@
              [util :as u]]
             [metabase.models
              [collection-test :as collection-test]
+             [permissions-group :as group]
              [user :refer [User]]]
             [metabase.test
              [data :refer :all]
@@ -49,6 +50,10 @@
     ;; Now do the request
     ((test-users/user->client :rasta) :get 200 "user")))
 
+(defn- group-ids->sets [users]
+  (for [user users]
+    (update user :group_ids set)))
+
 ;; Check that admins can get a list of active Users. Should include additional admin Fields
 (expect
   [(merge
@@ -57,7 +62,8 @@
      :first_name             "Crowberto"
      :last_name              "Corv"
      :is_superuser           true
-     :group_ids              [1, 2]
+     :group_ids              #{(u/get-id (group/all-users))
+                               (u/get-id (group/admin))}
      :personal_collection_id true
      :common_name            "Crowberto Corv"})
    (merge
@@ -65,7 +71,7 @@
     {:email                  "lucky@metabase.com"
      :first_name             "Lucky"
      :last_name              "Pigeon"
-     :group_ids              [1]
+     :group_ids              #{(u/get-id (group/all-users))}
      :personal_collection_id true
      :common_name            "Lucky Pigeon"})
    (merge
@@ -73,13 +79,14 @@
     {:email                  "rasta@metabase.com"
      :first_name             "Rasta"
      :last_name              "Toucan"
-     :group_ids              [1]
+     :group_ids              #{(u/get-id (group/all-users))}
      :personal_collection_id true
      :common_name            "Rasta Toucan"})]
   (do
     (test-users/delete-temp-users!)
     (collection-test/force-create-personal-collections!)
     (-> ((test-users/user->client :crowberto) :get 200 "user")
+        group-ids->sets
         tu/boolean-ids-and-timestamps)))
 
 ;; Non-admins should *not* be allowed to pass in include_deactivated
@@ -95,7 +102,7 @@
      :first_name             "Trash"
      :last_name              "Bird"
      :is_active              false
-     :group_ids              [1]
+     :group_ids              #{(u/get-id (group/all-users))}
      :personal_collection_id true
      :common_name            "Trash Bird"})
    (merge
@@ -104,7 +111,8 @@
      :first_name             "Crowberto"
      :last_name              "Corv"
      :is_superuser           true
-     :group_ids              [1 2]
+     :group_ids              #{(u/get-id (group/all-users))
+                               (u/get-id (group/admin))}
      :personal_collection_id true
      :common_name            "Crowberto Corv"})
    (merge
@@ -112,7 +120,7 @@
     {:email                  "lucky@metabase.com"
      :first_name             "Lucky"
      :last_name              "Pigeon"
-     :group_ids              [1]
+     :group_ids              #{(u/get-id (group/all-users))}
      :personal_collection_id true
      :common_name            "Lucky Pigeon"})
    (merge
@@ -120,13 +128,14 @@
     {:email                  "rasta@metabase.com"
      :first_name             "Rasta"
      :last_name              "Toucan"
-     :group_ids              [1]
+     :group_ids              #{(u/get-id (group/all-users))}
      :personal_collection_id true
      :common_name            "Rasta Toucan"})]
   (do
     (test-users/delete-temp-users!)
     (collection-test/force-create-personal-collections!)
     (-> ((test-users/user->client :crowberto) :get 200 "user", :include_deactivated true)
+        group-ids->sets
         tu/boolean-ids-and-timestamps)))
 
 
@@ -142,7 +151,7 @@
              :first_name       user-name
              :last_name        user-name
              :common_name      (str user-name " " user-name)
-             :group_ids        [1]
+             :group_ids        [(u/get-id (group/all-users))]
              :login_attributes {:test "value"}}))
     (et/with-fake-inbox
       (try
@@ -215,13 +224,14 @@
 ;; ## GET /api/user/current
 ;; Check that fetching current user will return extra fields like `is_active` and will return OrgPerms
 (expect
-  (merge user-defaults
-         {:email                  "rasta@metabase.com"
-          :first_name             "Rasta"
-          :last_name              "Toucan"
-          :common_name            "Rasta Toucan"
-          :group_ids              [1]
-          :personal_collection_id true})
+  (merge
+   user-defaults
+   {:email                  "rasta@metabase.com"
+    :first_name             "Rasta"
+    :last_name              "Toucan"
+    :common_name            "Rasta Toucan"
+    :group_ids              [(u/get-id (group/all-users))]
+    :personal_collection_id true})
   (do
     ;; Make sure personal Collections have been created so this endpoint won't randomly return `false` for
     ;; personal_collection_id
@@ -233,12 +243,13 @@
 ;; ## GET /api/user/:id
 ;; Should return a smaller set of fields, and should *not* return OrgPerms
 (expect
-  (merge user-defaults
-         {:email       "rasta@metabase.com"
-          :first_name  "Rasta"
-          :last_name   "Toucan"
-          :common_name "Rasta Toucan"
-          :group_ids   [1]})
+  (merge
+   user-defaults
+   {:email       "rasta@metabase.com"
+    :first_name  "Rasta"
+    :last_name   "Toucan"
+    :common_name "Rasta Toucan"
+    :group_ids   [(u/get-id (group/all-users))]})
   (tu/boolean-ids-and-timestamps ((test-users/user->client :rasta) :get 200 (str "user/" (test-users/user->id :rasta)))))
 
 ;; Check that a non-superuser CANNOT fetch someone else's user details
@@ -247,12 +258,13 @@
 
 ;; A superuser should be allowed to fetch another users data
 (expect
-  (merge user-defaults
-         {:email       "rasta@metabase.com"
-          :first_name  "Rasta"
-          :last_name   "Toucan"
-          :common_name "Rasta Toucan"
-          :group_ids   [1]})
+  (merge
+   user-defaults
+   {:email       "rasta@metabase.com"
+    :first_name  "Rasta"
+    :last_name   "Toucan"
+    :common_name "Rasta Toucan"
+    :group_ids   [(u/get-id (group/all-users))]})
   (tu/boolean-ids-and-timestamps ((test-users/user->client :crowberto) :get 200 (str "user/" (test-users/user->id :rasta)))))
 
 ;; We should get a 404 when trying to access a disabled account
@@ -271,20 +283,22 @@
                :first_name   "Cam"
                :last_name    "Eron"
                :is_superuser true
-               :group_ids    [1 2]})
+               :group_ids    #{(u/get-id (group/all-users))
+                               (u/get-id (group/admin))}})
    :after    {:first_name "Cam", :last_name "Eron", :is_superuser true, :email "cam.eron@metabase.com"}}
   (tt/with-temp User [{user-id :id} {:first_name   "Cam"
                                      :last_name    "Era"
                                      :email        "cam.era@metabase.com"
                                      :is_superuser true}]
-    (let [user (fn [] (into {} (dissoc (db/select-one [User :first_name :last_name :is_superuser :email], :id user-id)
-                                       :common_name)))]
+    (let [user (fn [] (into {} (-> (db/select-one [User :first_name :last_name :is_superuser :email], :id user-id)
+                                   (dissoc :common_name))))]
       (array-map
        :before   (user)
-       :response (tu/boolean-ids-and-timestamps
-                  ((test-users/user->client :crowberto) :put 200 (str "user/" user-id)
-                   {:last_name "Eron"
-                    :email     "cam.eron@metabase.com"}))
+       :response (-> ((test-users/user->client :crowberto) :put 200 (str "user/" user-id)
+                      {:last_name "Eron"
+                       :email     "cam.eron@metabase.com"})
+                     (update :group_ids set)
+                     tu/boolean-ids-and-timestamps)
        :after    (user)))))
 
 ;; Test that we can update login attributes after a user has been created
@@ -297,14 +311,17 @@
     :login_attributes {:test "value"}
     :common_name      "Test User"
     :last_name        "User"
-    :group_ids        [1 2]})
+    :group_ids        #{(u/get-id (group/all-users))
+                        (u/get-id (group/admin))}})
   (tt/with-temp User [{user-id :id} {:first_name   "Test"
                                      :last_name    "User"
                                      :email        "testuser@metabase.com"
                                      :is_superuser true}]
-    (tu/boolean-ids-and-timestamps
+    (->
      ((test-users/user->client :crowberto) :put 200 (str "user/" user-id) {:email            "testuser@metabase.com"
-                                                                           :login_attributes {:test "value"}}))))
+                                                                           :login_attributes {:test "value"}})
+     (update :group_ids set)
+     tu/boolean-ids-and-timestamps)))
 
 ;; ## PUT /api/user/:id
 ;; Test that updating a user's email to an existing inactive user's email fails

test/metabase/models/permissions_group_membership_test.clj

+(ns metabase.models.permissions-group-membership-test
+  (:require [expectations :refer [expect]]
+            [metabase.models
+             [permissions-group :as group]
+             [permissions-group-membership :as pgm :refer [PermissionsGroupMembership]]
+             [user :refer [User]]]
+            [metabase.util :as u]
+            [toucan.db :as db]
+            [toucan.util.test :as tt]))
+
+;; when you create a PermissionsGroupMembership for a User in the admin group, it should set their `is_superuser` flag
+(expect
+  true
+  (tt/with-temp User [user]
+    (db/insert! PermissionsGroupMembership {:user_id (u/get-id user), :group_id (u/get-id (group/admin))})
+    (db/select-one-field :is_superuser User :id (u/get-id user))))
+
+;; when you delete a PermissionsGroupMembership for a User in the admin group, it should set their `is_superuser` flag
+(expect
+  false
+  (tt/with-temp User [user {:is_superuser true}]
+    (db/delete! PermissionsGroupMembership :user_id (u/get-id user), :group_id (u/get-id (group/admin)))
+    (db/select-one-field :is_superuser User :id (u/get-id user))))

test/metabase/models/user_test.clj

@@ -3,18 +3,21 @@
             [expectations :refer :all]
             [metabase
              [email-test :as email-test]
-             [http-client :as http]]
+             [http-client :as http]
+             [util :as u]]
             [metabase.models
              [collection :as collection :refer [Collection]]
              [collection-test :as collection-test]
              [permissions :as perms]
-             [permissions-group :refer [PermissionsGroup]]
+             [permissions-group :as group :refer [PermissionsGroup]]
              [permissions-group-membership :refer [PermissionsGroupMembership]]
              [user :as user :refer [User]]]
             [metabase.test.data.users :as test-users :refer [user->id]]
             [metabase.test.util :as tu]
             [metabase.util.password :as upass]
-            [toucan.db :as db]
+            [toucan
+             [db :as db]
+             [hydrate :refer [hydrate]]]
             [toucan.util.test :as tt]))
 
 ;;; Tests for permissions-set
@@ -184,3 +187,192 @@
       (upass/verify-password "should be removed" password_salt password))
     (finally
       (db/delete! User :email "ldaptest@metabase.com"))))
+
+;; when you create a new user with `is_superuser` set to `true`, it should create a PermissionsGroupMembership object
+(expect
+  true
+  (tt/with-temp User [user {:is_superuser true}]
+    (db/exists? PermissionsGroupMembership :user_id (u/get-id user), :group_id (u/get-id (group/admin)))))
+
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                                            New Group IDs Functions                                             |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(defn- group-names [groups-or-ids]
+  (when (seq groups-or-ids)
+    (db/select-field :name PermissionsGroup :id [:in (map u/get-id groups-or-ids)])))
+
+(defn- do-with-group [group-properties group-members f]
+  (tt/with-temp PermissionsGroup [group group-properties]
+    (doseq [member group-members]
+      (db/insert! PermissionsGroupMembership
+        {:group_id (u/get-id group)
+         :user_id  (if (keyword? member)
+                     (test-users/user->id member)
+                     (u/get-id member))}))
+    (f group)))
+
+(defmacro ^:private with-groups [[group-binding group-properties members & more-groups] & body]
+  (if (seq more-groups)
+    `(with-groups [~group-binding ~group-properties ~members]
+       (with-groups ~more-groups
+         ~@body))
+    `(do-with-group ~group-properties ~members (fn [~group-binding] ~@body))))
+
+;; make sure that the `group-ids` hydration function works as expected
+(expect
+  #{"All Users" "Group 2" "Group 1"}
+  (with-groups [_ {:name "Group 1"} #{:lucky :rasta}
+                _ {:name "Group 2"} #{:lucky}
+                _ {:name "Group 3"} #{}]
+    (group-names (user/group-ids (test-users/user->id :lucky)))))
+
+;; `group-ids` should be a single DB call
+(expect
+  1
+  (with-groups [_ {:name "Group 1"} #{:lucky}
+                _ {:name "Group 2"} #{:lucky}
+                _ {:name "Group 3"} #{}]
+    (let [lucky-id (test-users/user->id :lucky)]
+      (db/with-call-counting [call-count]
+        (user/group-ids lucky-id)
+        (call-count)))))
+
+;; `group-ids` shouldn't barf if passed `nil`
+(expect
+  nil
+  (user/group-ids nil))
+
+;; check that the `add-group-ids` hydration function can do a batched hydrate
+(expect
+  {"Lucky" #{"All Users" "Group 1" "Group 2"}
+   "Rasta" #{"All Users" "Group 1"}}
+  (with-groups [_ {:name "Group 1"} #{:lucky :rasta}
+                _ {:name "Group 2"} #{:lucky}
+                _ {:name "Group 3"} #{}]
+    (let [users (user/add-group-ids (map test-users/fetch-user [:lucky :rasta]))]
+      (zipmap (map :first_name users)
+              (map (comp group-names :group_ids) users)))))
+
+;; `add-group-ids` should be the hydrate function for a `:group_ids` for a single User
+(expect
+  '(user/add-group-ids <users>)
+  (with-redefs [user/group-ids     (constantly '(user/group-ids <user>))
+                user/add-group-ids (fn [users]
+                                     (for [user users]
+                                       (assoc user :group_ids '(user/add-group-ids <users>))))]
+    (-> (hydrate (User (test-users/user->id :lucky)) :group_ids)
+        :group_ids)))
+
+;; `add-group-ids` should be the batched hydrate function for a `:group_ids`
+;; (Toucan can/will use batched hydrate functions to hydrate single objects)
+(expect
+  '[(user/add-group-ids <users>)
+    (user/add-group-ids <users>)]
+  (with-redefs [user/group-ids     (constantly '(user/group-ids <user>))
+                user/add-group-ids (fn [users]
+                                     (for [user users]
+                                       (assoc user :group_ids '(user/add-group-ids <users>))))]
+    (as-> (map test-users/fetch-user [:rasta :lucky]) users
+      (hydrate users :group_ids)
+      (mapv :group_ids users))))
+
+;; ...it should do it in a single DB call
+(expect
+  1
+  (with-groups [_ {:name "Group 1"} #{:lucky :rasta}
+                _ {:name "Group 2"} #{:lucky}
+                _ {:name "Group 3"} #{}]
+    (let [users (mapv test-users/fetch-user [:lucky :rasta])]
+      (db/with-call-counting [call-count]
+        (dorun (user/add-group-ids users))
+        (call-count)))))
+
+;; `add-group-ids` shouldn't barf if passed an empty seq
+(expect
+  nil
+  (user/add-group-ids []))
+
+(defn- user-group-names [user]
+  (group-names (user/group-ids (if (keyword? user)
+                                 (test-users/fetch-user user)
+                                 user))))
+
+;; check that we can use `set-permissions-groups!` to add a User to new groups
+(expect
+  #{"All Users" "Group 1" "Group 2"}
+  (with-groups [group-1 {:name "Group 1"} #{}
+                group-2 {:name "Group 2"} #{}]
+    (user/set-permissions-groups! (test-users/user->id :lucky) #{(group/all-users) group-1 group-2})
+    (user-group-names :lucky)))
+
+;; check that we can use `set-permissions-groups!` to remove a User from groups
+(expect
+  #{"All Users"}
+  (with-groups [group-1 {:name "Group 1"} #{:lucky}
+                group-2 {:name "Group 2"} #{:lucky}]
+    (user/set-permissions-groups! (test-users/user->id :lucky) #{(group/all-users)})
+    (user-group-names :lucky)))
+
+;; check that `set-permissions-groups!` can add & remove groups all at once! :wow:
+(expect
+  #{"All Users" "Group 2"}
+  (with-groups [group-1 {:name "Group 1"} #{:lucky}
+                group-2 {:name "Group 2"} #{}]
+    (user/set-permissions-groups! (test-users/user->id :lucky) #{(group/all-users) group-2})
+    (user-group-names :lucky)))
+
+;; `set-permissions-groups!` should throw an Exception if you attempt to remove someone from All Users
+(expect
+  Exception
+  (with-groups [group-1 {:name "Group 1"} #{}]
+    (user/set-permissions-groups! (test-users/user->id :lucky) #{group-1})))
+
+;; `set-permissions-groups!` should let someone be added to Admin group
+(expect
+  #{"Administrators" "All Users"}
+  (tt/with-temp User [user]
+    (user/set-permissions-groups! user #{(group/all-users) (group/admin)})
+    (user-group-names user)))
+
+;; is_superuser should get set when adding a user to admin via `set-permissions-groups!`
+(expect
+  {:is_superuser true}
+  (tt/with-temp User [user]
+    (user/set-permissions-groups! user #{(group/all-users) (group/admin)})
+    (db/select-one [User :is_superuser] :id (u/get-id user))))
+
+;; `set-permissions-groups!` should let someone be removed from Admin group
+(expect
+  #{"All Users"}
+  (tt/with-temp User [user {:is_superuser true}]
+    (user/set-permissions-groups! user #{(group/all-users)})
+    (user-group-names user)))
+
+(expect
+  false
+  (tt/with-temp User [user {:is_superuser true}]
+    (user/set-permissions-groups! user #{(group/all-users)})
+    (db/select-one-field :is_superuser User :id (u/get-id user))))
+
+;; The entire set of changes should run in a transaction -- if one set of changes fails, others should not be persisted
+;; [INVALID ADD]
+(expect
+  true
+  ;; User should not be removed from the admin group because the attempt to add them to the Integer/MAX_VALUE group
+  ;; should fail, causing the entire transaction to fail
+  (tt/with-temp User [user {:is_superuser true}]
+    (u/ignore-exceptions
+      (user/set-permissions-groups! user #{(group/all-users) Integer/MAX_VALUE}))
+    (db/select-one-field :is_superuser User :id (u/get-id user))))
+
+;; If an INVALID REMOVE is attempted, valid adds should not be persisted
+;; Attempt to remove someone from All Users + add to a valid group at the same time -- neither should persist
+(expect
+  #{"All Users"}
+  (tt/with-temp User [user]
+    (with-groups [group {:name "Group"} {}]
+      (u/ignore-exceptions
+        (user/set-permissions-groups! (test-users/fetch-user :lucky) #{group})))
+    (user-group-names :lucky)))