Do not let API users archive the last superuser (#27524)

* Do not let API users archive the last superuser

[Fixes #26759]

* DRY up admin-archiving check

* Fix is_superuser bug in with-single-admin-user

* Get count of values correctly on H2

.clj-kondo/config.edn

@@ -403,6 +403,7 @@
   metabase.test.data.interface/defdataset-edn                                          clojure.core/def
   metabase.test/defdataset                                                             clojure.core/def
   metabase.test/with-open-channels                                                     clojure.core/let
+  metabase.test/with-single-admin-user                                                 clojure.core/fn
   metabase.test/with-temp-dir                                                          clojure.core/let
   metabase.test/with-temp-file                                                         clojure.core/let
   metabase.test/with-user-in-groups                                                    clojure.core/let

src/metabase/api/user.clj

@@ -310,9 +310,9 @@
    login_attributes       (s/maybe user/LoginAttributes)
    locale                 (s/maybe su/ValidLocalePlumatic)}
   (try
-   (check-self-or-superuser id)
-   (catch clojure.lang.ExceptionInfo _e
-     (validation/check-group-manager)))
+    (check-self-or-superuser id)
+    (catch clojure.lang.ExceptionInfo _e
+      (validation/check-group-manager)))
 
   ;; only allow updates if the specified account is active
   (api/let-404 [user-before-update (fetch-user :id id, :is_active true)]
@@ -327,20 +327,20 @@
         "last_name" (tru "Editing last name is not allowed for SSO users.")))
     ;; can't change email if it's already taken BY ANOTHER ACCOUNT
     (api/checkp (not (db/exists? User, :%lower.email (if email (u/lower-case-en 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
-     ;; only superuser or self can update user info
-     ;; implicitly prevent group manager from updating users' info
-     (when (or (= id api/*current-user-id*)
-               api/*is-superuser?*)
-       (api/check-500
-        (db/update! User id (u/select-keys-when body
-                              :present (cond-> #{:first_name :last_name :locale}
-                                         api/*is-superuser?* (conj :login_attributes))
-                              :non-nil (cond-> #{:email}
-                                         api/*is-superuser?* (conj :is_superuser)))))
-       (maybe-update-user-personal-collection-name! user-before-update body))
-     (maybe-set-user-group-memberships! id user_group_memberships is_superuser)))
+      ;; only superuser or self can update user info
+      ;; implicitly prevent group manager from updating users' info
+      (when (or (= id api/*current-user-id*)
+                api/*is-superuser?*)
+        (api/check-500
+         (db/update! User id (u/select-keys-when body
+                               :present (cond-> #{:first_name :last_name :locale}
+                                          api/*is-superuser?* (conj :login_attributes))
+                               :non-nil (cond-> #{:email}
+                                          api/*is-superuser?* (conj :is_superuser)))))
+        (maybe-update-user-personal-collection-name! user-before-update body))
+      (maybe-set-user-group-memberships! id user_group_memberships is_superuser)))
   (-> (fetch-user :id id)
       (hydrate :user_group_memberships)))
 

src/metabase/models/permissions_group_membership.clj

@@ -26,10 +26,22 @@
       (throw (ex-info (tru "You cannot add or remove users to/from the ''All Users'' group.")
                {:status-code 400})))))
 
-(defn- check-not-last-admin []
-  (when (<= (db/count PermissionsGroupMembership
-              :group_id (:id (perms-group/admin)))
-            1)
+(defn- admin-count
+  "The current number of non-archived admins (superusers)."
+  []
+  (:count
+   (first
+    (db/query {:select [[:%count.* :count]]
+               :from   [[:permissions_group_membership :pgm]]
+               :join   [[:core_user :user] [:= :user.id :pgm.user_id]]
+               :where  [:and [:= :pgm.group_id (u/the-id (perms-group/admin))]
+                             [:= :user.is_active true]]}))))
+
+(defn throw-if-last-admin!
+  "Throw an Exception if there is only one admin (superuser) left. The assumption is that the one admin is about to be
+  archived or have their admin status removed."
+  []
+  (when (<= (admin-count) 1)
     (throw (ex-info (str fail-to-remove-last-admin-msg)
                     {:status-code 400}))))
 
@@ -37,8 +49,8 @@
   (check-not-all-users-group group_id)
   ;; Otherwise if this is the Admin group...
   (when (= group_id (:id (perms-group/admin)))
-    ;; ...and this is the last membership throw an exception
-    (check-not-last-admin)
+    ;; ...and this is the last membership, throw an exception
+    (throw-if-last-admin!)
     ;; ...otherwise we're ok. Unset the `:is_superuser` flag for the user whose membership was revoked
     (db/update! 'User user_id
       :is_superuser false)))

src/metabase/models/user.clj

@@ -82,20 +82,24 @@
 (defn- pre-update
   [{reset-token :reset_token, superuser? :is_superuser, active? :is_active, :keys [email id locale], :as user}]
   ;; when `:is_superuser` is toggled add or remove the user from the 'Admin' group as appropriate
-  (when (some? superuser?)
-    (let [membership-exists? (db/exists? PermissionsGroupMembership
-                               :group_id (:id (perms-group/admin))
-                               :user_id  id)]
+  (let [in-admin-group?  (db/exists? PermissionsGroupMembership
+                           :group_id (:id (perms-group/admin))
+                           :user_id  id)]
+    ;; Do not let the last admin archive themselves
+    (when (and in-admin-group?
+               (false? active?))
+      (perms-group-membership/throw-if-last-admin!))
+    (when (some? superuser?)
       (cond
         (and superuser?
-             (not membership-exists?))
+             (not in-admin-group?))
         (db/insert! PermissionsGroupMembership
           :group_id (u/the-id (perms-group/admin))
           :user_id  id)
         ;; don't use [[db/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 a `post-delete` method?
         (and (not superuser?)
-             membership-exists?)
+             in-admin-group?)
         (db/simple-delete! PermissionsGroupMembership
           :group_id (u/the-id (perms-group/admin))
           :user_id  id))))

test/metabase/api/user_test.clj

@@ -982,7 +982,19 @@
         (is (= {:is_active false}
                (mt/derecordize (db/select-one [User :is_active] :id (:id user)))))))
 
-    (testing "Check that a non-superuser CANNOT update deactivate themselves"
+    (testing "Check that the last superuser cannot deactivate themselves"
+      (mt/with-single-admin-user [{id :id}]
+        (is (= "You cannot remove the last member of the 'Admin' group!"
+               (mt/user-http-request id :delete 400 (format "user/%d" id))))))
+
+    (testing "Check that the last non-archived superuser cannot deactivate themselves"
+      (mt/with-single-admin-user [{id :id}]
+        (mt/with-temp User [_ {:is_active    false
+                               :is_superuser true}]
+          (is (= "You cannot remove the last member of the 'Admin' group!"
+                 (mt/user-http-request id :delete 400 (format "user/%d" id)))))))
+
+    (testing "Check that a non-superuser CANNOT deactivate themselves"
       (is (= "You don't have permissions to do that."
              (mt/user-http-request :rasta :delete 403 (format "user/%d" (mt/user->id :rasta)) {}))))))
 

test/metabase/models/permissions_group_membership_test.clj

@@ -24,4 +24,16 @@
     (mt/with-temp User [user {:is_superuser true}]
       (db/delete! PermissionsGroupMembership :user_id (u/the-id user), :group_id (u/the-id (perms-group/admin)))
       (is (= false
-             (db/select-one-field :is_superuser User :id (u/the-id user)))))))
+             (db/select-one-field :is_superuser User :id (u/the-id user))))))
+
+  (testing "it should not let you remove the last admin"
+    (mt/with-single-admin-user [{id :id}]
+      (is (thrown? Exception
+                   (db/delete! PermissionsGroupMembership :user_id id, :group_id (u/the-id (perms-group/admin)))))))
+
+  (testing "it should not let you remove the last non-archived admin"
+    (mt/with-single-admin-user [{id :id}]
+      (mt/with-temp User [_ {:is_active    false
+                             :is_superuser true}]
+        (is (thrown? Exception
+                     (db/delete! PermissionsGroupMembership :user_id id, :group_id (u/the-id (perms-group/admin)))))))))

test/metabase/test.clj

@@ -16,6 +16,8 @@
    [metabase.driver.sql.query-processor-test-util :as sql.qp-test-util]
    [metabase.email-test :as et]
    [metabase.http-client :as client]
+   [metabase.models :refer [PermissionsGroupMembership User]]
+   [metabase.models.permissions-group :as perms-group]
    [metabase.plugins.classloader :as classloader]
    [metabase.query-processor :as qp]
    [metabase.query-processor-test :as qp.test]
@@ -44,6 +46,7 @@
    [pjstadig.humane-test-output :as humane-test-output]
    [potemkin :as p]
    [toucan.db :as db]
+   [toucan.models :as models]
    [toucan.util.test :as tt]))
 
 (humane-are/install!)
@@ -287,6 +290,39 @@
   [clock & body]
   `(do-with-clock ~clock (fn [] ~@body)))
 
+(defn do-with-single-admin-user
+  [attributes thunk]
+  (let [existing-admin-memberships (db/select PermissionsGroupMembership :group_id (:id (perms-group/admin)))
+        _                          (db/simple-delete! PermissionsGroupMembership :group_id (:id (perms-group/admin)))
+        existing-admin-ids         (db/select-ids User :is_superuser true)
+        _                          (when (seq existing-admin-ids)
+                                     (db/update-where! User {:id [:in existing-admin-ids]} :is_superuser false))
+        temp-admin                 (db/insert! User (merge (with-temp-defaults User)
+                                                           attributes
+                                                           {:is_superuser true}))
+        primary-key                (models/primary-key User)]
+    (try
+      (thunk temp-admin)
+      (finally
+        (db/delete! User primary-key (primary-key temp-admin))
+        (when (seq existing-admin-ids)
+          (db/update-where! User {:id [:in existing-admin-ids]} :is_superuser true))
+        (db/insert-many! PermissionsGroupMembership existing-admin-memberships)))))
+
+(defmacro with-single-admin-user
+  "Creates an admin user (with details described in the `options-map`) and (temporarily) removes the administrative
+  powers of all other users in the database.
+
+  Example:
+
+  (testing \"Check that the last superuser cannot deactivate themselves\"
+    (mt/with-single-admin-user [{id :id}]
+      (is (= \"You cannot remove the last member of the 'Admin' group!\"
+             (mt/user-http-request :crowberto :delete 400 (format \"user/%d\" id))))))"
+  [[binding-form & [options-map]] & body]
+  `(do-with-single-admin-user ~options-map (fn [~binding-form]
+                                             ~@body)))
+
 ;;;; New QP middleware test util fns. Experimental. These will be put somewhere better if confirmed useful.
 
 (defn test-qp-middleware