Tests for /api/user endpoints handling of group_ids

src/metabase/api/user.clj

@@ -16,14 +16,38 @@
             [schema.core :as s]
             [toucan
              [db :as db]
-             [hydrate :refer [hydrate]]]))
+             [hydrate :refer [hydrate]]]
+            [metabase.models.permissions-group :as group]))
 
 (defn- check-self-or-superuser
   "Check that USER-ID is *current-user-id*` or that `*current-user*` is a superuser, or throw a 403."
   [user-id]
   {:pre [(integer? user-id)]}
-  (api/check-403 (or (= user-id api/*current-user-id*)
-                     (:is_superuser @api/*current-user*))))
+  (api/check-403
+   (or
+    (= user-id api/*current-user-id*)
+    api/*is-superuser?*)))
+
+(defn- fetch-user [& query-criteria]
+  (apply db/select-one (vec (cons User user/admin-or-self-visible-columns)) query-criteria))
+
+(defn- maybe-set-user-permissions-groups! [user-or-id new-groups-or-ids & [is-superuser?]]
+  ;; if someone passed in both `:is_superuser` and `:group_ids`, make sure the whether the admin group is in group_ids
+  ;; agrees with is_superuser -- don't want to have ambiguous behavior
+  (when (and (some? is-superuser?)
+             new-groups-or-ids)
+    (api/checkp (= is-superuser? (contains? (set new-groups-or-ids) (u/get-id (group/admin))))
+      "is_superuser" (tru "Value of is_superuser must correspond to presence of Admin group ID in group_ids.")))
+  (when (some? new-groups-or-ids)
+    (when-not (= (user/group-ids user-or-id)
+                 (set (map u/get-id new-groups-or-ids)))
+      (api/check-superuser)
+      (user/set-permissions-groups! user-or-id new-groups-or-ids))))
+
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                   Fetching Users -- GET /api/user, GET /api/user/current, GET /api/user/:id                    |
+;;; +----------------------------------------------------------------------------------------------------------------+
 
 (api/defendpoint GET "/"
   "Fetch a list of `Users` for the admin People page or for Pulses. By default returns only active users. If
@@ -43,29 +67,23 @@
     ;; For admins, also include the IDs of the  Users' Personal Collections
     api/*is-superuser?* (hydrate :personal_collection_id :group_ids)))
 
-(defn- fetch-user [& query-criteria]
-  (apply db/select-one (vec (cons User user/admin-or-self-visible-columns)) query-criteria))
+(api/defendpoint GET "/current"
+  "Fetch the current `User`."
+  []
+  (-> (api/check-404 @api/*current-user*)
+      (hydrate :personal_collection_id :group_ids)))
 
-(defn- reactivate-user! [existing-user]
-  (db/update! User (u/get-id existing-user)
-    :is_active     true
-    :is_superuser  false
-    ;; if the user orignally logged in via Google Auth and it's no longer enabled, convert them into a regular user
-    ;; (see Issue #3323)
-    :google_auth   (boolean (and (:google_auth existing-user)
-                                 ;; if google-auth-client-id is set it means Google Auth is enabled
-                                 (session-api/google-auth-client-id)))
-    :ldap_auth     (boolean (and (:ldap_auth existing-user)
-                                 (ldap/ldap-configured?))))
-  ;; now return the existing user whether they were originally active or not
-  (fetch-user :id (u/get-id existing-user)))
+(api/defendpoint GET "/:id"
+  "Fetch a `User`. You must be fetching yourself *or* be a superuser."
+  [id]
+  (check-self-or-superuser id)
+  (-> (api/check-404 (fetch-user :id id, :is_active true))
+      (hydrate :group_ids)))
 
-(defn- maybe-set-user-permissions-groups! [user-or-id new-groups-or-ids]
-  (when (some? new-groups-or-ids)
-    (when-not (= (user/group-ids user-or-id)
-                 (set (map u/get-id new-groups-or-ids)))
-      (api/check-superuser)
-      (user/set-permissions-groups! user-or-id new-groups-or-ids))))
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                                     Creating a new User -- POST /api/user                                      |
+;;; +----------------------------------------------------------------------------------------------------------------+
 
 (api/defendpoint POST "/"
   "Create a new `User`, return a 400 if the email address is already taken"
@@ -78,25 +96,19 @@
   (api/check-superuser)
   (api/checkp (not (db/exists? User :email email))
     "email" (tru "Email address already in use."))
-  (let [new-user-id (u/get-id (user/invite-user! (select-keys body [:first_name :last_name :email :password :login_attributes])
-                                                 @api/*current-user*))]
-    (maybe-set-user-permissions-groups! new-user-id group_ids)
-    (-> (fetch-user :id new-user-id)
-        (hydrate :group_ids))))
-
-(api/defendpoint GET "/current"
-  "Fetch the current `User`."
-  []
-  (-> (api/check-404 @api/*current-user*)
-      (hydrate :personal_collection_id :group_ids)))
+  (db/transaction
+    (let [new-user-id (u/get-id (user/create-and-invite-user!
+                                 (u/select-keys-when body
+                                   :non-nil [:first_name :last_name :email :password :login_attributes])
+                                 @api/*current-user*))]
+      (maybe-set-user-permissions-groups! new-user-id group_ids)
+      (-> (fetch-user :id new-user-id)
+          (hydrate :group_ids)))))
 
 
-(api/defendpoint GET "/:id"
-  "Fetch a `User`. You must be fetching yourself *or* be a superuser."
-  [id]
-  (check-self-or-superuser id)
-  (-> (api/check-404 (fetch-user :id id, :is_active true))
-      (hydrate :group_ids)))
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                                      Updating a User -- PUT /api/user/:id                                      |
+;;; +----------------------------------------------------------------------------------------------------------------+
 
 (defn- valid-email-update?
   "This predicate tests whether or not the user is allowed to update the email address associated with this account."
@@ -118,6 +130,7 @@
    first_name       (s/maybe su/NonBlankString)
    last_name        (s/maybe su/NonBlankString)
    group_ids        (s/maybe [su/IntGreaterThanZero])
+   is_superuser     (s/maybe s/Bool)
    login_attributes (s/maybe user/LoginAttributes)}
   (check-self-or-superuser id)
   ;; only allow updates if the specified account is active
@@ -126,7 +139,8 @@
     (api/check-403 (valid-email-update? user-before-update email))
     ;; can't change email if it's already taken BY ANOTHER ACCOUNT
     (api/checkp (not (db/exists? User, :email email, :id [:not= id]))
-      "email" (tru "Email address already associated to another user."))
+      "email" (tru "Email address already associated to another user.")))
+  (db/transaction
     (api/check-500
      (db/update! User id
        (u/select-keys-when body
@@ -134,11 +148,30 @@
                     #{:login_attributes})
          :non-nil (set (concat [:first_name :last_name :email]
                                (when api/*is-superuser?*
-                                 [:is_superuser])))))))
-  (maybe-set-user-permissions-groups! id group_ids)
+                                 [:is_superuser]))))))
+    (maybe-set-user-permissions-groups! id group_ids is_superuser))
   (-> (fetch-user :id id)
       (hydrate :group_ids)))
 
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                              Reactivating a User -- PUT /api/user/:id/reactivate                               |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(defn- reactivate-user! [existing-user]
+  (db/update! User (u/get-id existing-user)
+    :is_active     true
+    :is_superuser  false
+    ;; if the user orignally logged in via Google Auth and it's no longer enabled, convert them into a regular user
+    ;; (see Issue #3323)
+    :google_auth   (boolean (and (:google_auth existing-user)
+                                 ;; if google-auth-client-id is set it means Google Auth is enabled
+                                 (session-api/google-auth-client-id)))
+    :ldap_auth     (boolean (and (:ldap_auth existing-user)
+                                 (ldap/ldap-configured?))))
+  ;; now return the existing user whether they were originally active or not
+  (fetch-user :id (u/get-id existing-user)))
+
 (api/defendpoint PUT "/:id/reactivate"
   "Reactivate user at `:id`"
   [id]
@@ -151,6 +184,10 @@
     (reactivate-user! user)))
 
 
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                               Updating a Password -- PUT /api/user/:id/password                                |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
 (api/defendpoint PUT "/:id/password"
   "Update a user's password."
   [id :as {{:keys [password old_password]} :body}]
@@ -168,6 +205,21 @@
   (fetch-user :id id))
 
 
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                             Deleting (Deactivating) a User -- DELETE /api/user/:id                             |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(api/defendpoint DELETE "/:id"
+  "Disable a `User`.  This does not remove the `User` from the DB, but instead disables their account."
+  [id]
+  (api/check-superuser)
+  (api/check-500 (db/update! User id, :is_active false))
+  {:success true})
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                  Other Endpoints -- PUT /api/user/:id/qpnewb, POST /api/user/:id/send_invite                   |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
 ;; TODO - This could be handled by PUT /api/user/:id, we don't need a separate endpoint
 (api/defendpoint PUT "/:id/qbnewb"
   "Indicate that a user has been informed about the vast intricacies of 'the' Query Builder."
@@ -176,7 +228,6 @@
   (api/check-500 (db/update! User id, :is_qbnewb false))
   {:success true})
 
-
 (api/defendpoint POST "/:id/send_invite"
   "Resend the user invite email for a given user."
   [id]
@@ -188,12 +239,4 @@
       (email/send-new-user-email! user @api/*current-user* join-url))))
 
 
-(api/defendpoint DELETE "/:id"
-  "Disable a `User`.  This does not remove the `User` from the DB, but instead disables their account."
-  [id]
-  (api/check-superuser)
-  (api/check-500 (db/update! User id, :is_active false))
-  {:success true})
-
-
 (api/define-routes)

src/metabase/models/permissions_group_membership.clj

@@ -53,7 +53,7 @@
 
 (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
+    ;; If we're adding a user to the admin group, set the `:is_superuser` flag for the user to whom membership was
     ;; granted
     (when (= group_id (:id (group/admin)))
       (db/update! 'User user_id

src/metabase/models/user.clj

@@ -196,7 +196,7 @@
   [new-user :- NewUser]
   (db/insert! User (update new-user :password #(or % (str (UUID/randomUUID))))))
 
-(s/defn invite-user!
+(s/defn create-and-invite-user!
   "Convenience function for inviting a new `User` and sending out the welcome email."
   [new-user :- NewUser, invitor :- Invitor]
   ;; create the new user

src/metabase/util.clj

@@ -568,7 +568,8 @@
        :non-nil #{:d :e :f})
      ;; -> {:a 100, :b nil, :d 200}"
   {:style/indent 1}
-  [m & {:keys [present non-nil]}]
+  [m & {:keys [present non-nil], :as options}]
+  {:pre [(every? #{:present :non-nil} (keys options))]}
   (merge (select-keys m present)
          (select-non-nil-keys m non-nil)))
 

test/metabase/api/permissions_test.clj

@@ -13,7 +13,6 @@
 ;; Should *not* include inactive users in the counts.
 ;; It should also *not* include the MetaBot group because MetaBot should *not* be enabled
 (defn- fetch-groups []
-  (test-users/delete-temp-users!)
   (set ((test-users/user->client :crowberto) :get 200 "permissions/group")))
 
 (expect
@@ -35,10 +34,9 @@
   #{{:first_name "Crowberto", :last_name "Corv",   :email "crowberto@metabase.com", :user_id (test-users/user->id :crowberto), :membership_id true}
     {:first_name "Lucky",     :last_name "Pigeon", :email "lucky@metabase.com",     :user_id (test-users/user->id :lucky),     :membership_id true}
     {:first_name "Rasta",     :last_name "Toucan", :email "rasta@metabase.com",     :user_id (test-users/user->id :rasta),     :membership_id true}}
-  (do
-    (test-users/delete-temp-users!)
-    (set (for [member (:members ((test-users/user->client :crowberto) :get 200 (str "permissions/group/" (u/get-id (group/all-users)))))]
-           (update member :membership_id (complement nil?))))))
+  (set
+   (for [member (:members ((test-users/user->client :crowberto) :get 200 (str "permissions/group/" (u/get-id (group/all-users)))))]
+     (update member :membership_id some?))))
 
 
 ;; make sure we can update the perms graph from the API

test/metabase/models/user_test.clj

@@ -82,12 +82,12 @@
                  :let             [address (if (= address new-user-email-address)
                                              "<New User>"
                                              address)]]
-             {address (for [{subject :subject} emails]
-                        (str/replace subject (str new-user-first-name " " new-user-last-name) "<New User>"))})))
+             [address (for [{subject :subject} emails]
+                        (str/replace subject (str new-user-first-name " " new-user-last-name) "<New User>"))])))
 
 
 (defn- invite-user-accept-and-check-inboxes!
-  "Create user by passing INVITE-USER-ARGS to `invite-user!` or `create-new-google-auth-user!`,
+  "Create user by passing INVITE-USER-ARGS to `create-and-invite-user!` or `create-new-google-auth-user!`,
   and return a map of addresses emails were sent to to the email subjects."
   [& {:keys [google-auth? accept-invite? password invitor]
       :or   {accept-invite? true}}]
@@ -103,7 +103,7 @@
         (try
           (if google-auth?
             (user/create-new-google-auth-user! (dissoc new-user :password))
-            (user/invite-user!                 new-user invitor))
+            (user/create-and-invite-user!                 new-user invitor))
           (when accept-invite?
             (maybe-accept-invite! new-user-email))
           (sent-emails new-user-email new-user-first-name new-user-last-name)
@@ -116,18 +116,14 @@
 
 ;; admin shouldn't get email saying user joined until they accept the invite (i.e., reset their password)
 (expect
-  {"<New User>"             ["You're invited to join Metabase's Metabase"]}
-  (do
-    (test-users/delete-temp-users!)
-    (invite-user-accept-and-check-inboxes! :invitor default-invitor, :accept-invite? false)))
+  {"<New User>" ["You're invited to join Metabase's Metabase"]}
+  (invite-user-accept-and-check-inboxes! :invitor default-invitor, :accept-invite? false))
 
 ;; admin should get an email when a new user joins...
 (expect
   {"<New User>"             ["You're invited to join Metabase's Metabase"]
    "crowberto@metabase.com" ["<New User> accepted their Metabase invite"]}
-  (do
-    (test-users/delete-temp-users!)
-    (invite-user-accept-and-check-inboxes! :invitor default-invitor)))
+  (invite-user-accept-and-check-inboxes! :invitor default-invitor))
 
 ;; ...including the site admin if it is set...
 (expect
@@ -135,26 +131,21 @@
    "crowberto@metabase.com" ["<New User> accepted their Metabase invite"]
    "cam@metabase.com"       ["<New User> accepted their Metabase invite"]}
   (tu/with-temporary-setting-values [admin-email "cam@metabase.com"]
-    (test-users/delete-temp-users!)
     (invite-user-accept-and-check-inboxes! :invitor default-invitor)))
 
 ;; ... but if that admin is inactive they shouldn't get an email
 (expect
   {"<New User>"             ["You're invited to join Metabase's Metabase"]
    "crowberto@metabase.com" ["<New User> accepted their Metabase invite"]}
-  (do
-    (test-users/delete-temp-users!)
-    (tt/with-temp User [inactive-admin {:is_superuser true, :is_active false}]
-      (invite-user-accept-and-check-inboxes! :invitor (assoc inactive-admin :is_active false)))))
+  (tt/with-temp User [inactive-admin {:is_superuser true, :is_active false}]
+    (invite-user-accept-and-check-inboxes! :invitor (assoc inactive-admin :is_active false))))
 
 ;; for google auth, all admins should get an email...
 (expect
   {"crowberto@metabase.com"        ["<New User> created a Metabase account"]
    "some_other_admin@metabase.com" ["<New User> created a Metabase account"]}
-  (do
-    (test-users/delete-temp-users!)
-    (tt/with-temp User [_ {:is_superuser true, :email "some_other_admin@metabase.com"}]
-      (invite-user-accept-and-check-inboxes! :google-auth? true))))
+  (tt/with-temp User [_ {:is_superuser true, :email "some_other_admin@metabase.com"}]
+    (invite-user-accept-and-check-inboxes! :google-auth? true)))
 
 ;; ...including the site admin if it is set...
 (expect
@@ -162,17 +153,14 @@
    "some_other_admin@metabase.com" ["<New User> created a Metabase account"]
    "cam@metabase.com"              ["<New User> created a Metabase account"]}
   (tu/with-temporary-setting-values [admin-email "cam@metabase.com"]
-    (test-users/delete-temp-users!)
     (tt/with-temp User [_ {:is_superuser true, :email "some_other_admin@metabase.com"}]
       (invite-user-accept-and-check-inboxes! :google-auth? true))))
 
 ;; ...unless they are inactive
 (expect
   {"crowberto@metabase.com" ["<New User> created a Metabase account"]}
-  (do
-    (test-users/delete-temp-users!)
-    (tt/with-temp User [_ {:is_superuser true, :is_active false}]
-      (invite-user-accept-and-check-inboxes! :google-auth? true))))
+  (tt/with-temp User [_ {:is_superuser true, :is_active false}]
+    (invite-user-accept-and-check-inboxes! :google-auth? true)))
 
 ;; LDAP users should not persist their passwords. Check that if somehow we get passed an LDAP user password, it gets
 ;; swapped with something random
@@ -199,7 +187,7 @@
 ;;; |                                            New Group IDs Functions                                             |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
-(defn- group-names [groups-or-ids]
+(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)])))
 
@@ -294,10 +282,10 @@
   nil
   (user/add-group-ids []))
 
-(defn- user-group-names [user]
-  (group-names (user/group-ids (if (keyword? user)
-                                 (test-users/fetch-user user)
-                                 user))))
+(defn user-group-names [user-or-id-or-kw]
+  (group-names (user/group-ids (if (keyword? user-or-id-or-kw)
+                                 (test-users/fetch-user user-or-id-or-kw)
+                                 user-or-id-or-kw))))
 
 ;; check that we can use `set-permissions-groups!` to add a User to new groups
 (expect

test/metabase/test/data/users.clj

@@ -155,14 +155,6 @@
   (create-users-if-needed! username)
   (partial client-fn username))
 
-
-(defn ^:deprecated delete-temp-users!
-  "Delete all users besides the 4 persistent test users.
-  This is a HACK to work around tests that don't properly clean up after themselves; one day we should be able to
-  remove this. (TODO)"
-  []
-  (db/delete! User :id [:not-in (map user->id [:crowberto :lucky :rasta :trashbird])]))
-
 (defn do-with-test-user
   "Call `f` with various `metabase.api.common` dynamic vars bound to the test User named by `user-kwd`."
   [user-kwd f]

test/metabase/test/util.clj

@@ -303,15 +303,20 @@
 (defn do-with-temp-vals-in-db
   "Implementation function for `with-temp-vals-in-db` macro. Prefer that to using this directly."
   [model object-or-id column->temp-value f]
-  (let [original-column->value (db/select-one (vec (cons model (keys column->temp-value)))
-                                 :id (u/get-id object-or-id))]
+  ;; use low-level `query` and `execute` functions here, because Toucan `select` and `update` functions tend to do
+  ;; things like add columns like `common_name` that don't actually exist, causing subsequent update to fail
+  (let [[original-column->value] (db/query {:select (keys column->temp-value)
+                                            :from   [model]
+                                            :where  [:= :id (u/get-id object-or-id)]})]
     (try
       (db/update! model (u/get-id object-or-id)
         column->temp-value)
       (f)
       (finally
-        (db/update! model (u/get-id object-or-id)
-          original-column->value)))))
+        (db/execute!
+         {:update model
+          :set    original-column->value
+          :where  [:= :id (u/get-id object-or-id)]})))))
 
 (defmacro with-temp-vals-in-db
   "Temporary set values for an `object-or-id` in the application database, execute `body`, and then restore the