diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn
index 78227b399586341545f140b8da49071027352785..269fd55cee871329a0540e9959b1afda03ac6e00 100644
--- a/.clj-kondo/config.edn
+++ b/.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
diff --git a/src/metabase/api/user.clj b/src/metabase/api/user.clj
index b8fe3044eca44844f2ae80f4277ff8311e526af2..30e36a7bb592b56b5ed5526b6f8cfec5a9d0a960 100644
--- a/src/metabase/api/user.clj
+++ b/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)))
diff --git a/src/metabase/models/permissions_group_membership.clj b/src/metabase/models/permissions_group_membership.clj
index ad15e72c255ff7ea3ec81e8dce8171c5243465bf..c8b9df3cc3e14a98bbea4fc07fcc65c7d0e87249 100644
--- a/src/metabase/models/permissions_group_membership.clj
+++ b/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)))
diff --git a/src/metabase/models/user.clj b/src/metabase/models/user.clj
index 35181392b9bf298de097373433f8be29b403d34e..93aa74beba3566f393527325da6b476a30195d9e 100644
--- a/src/metabase/models/user.clj
+++ b/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))))
diff --git a/test/metabase/api/user_test.clj b/test/metabase/api/user_test.clj
index bc09a9aa7d13f58f320c03322f807b35f1da649d..b63c81c2127ac3e115f3b7907391e4029be534d2 100644
--- a/test/metabase/api/user_test.clj
+++ b/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)) {}))))))
diff --git a/test/metabase/models/permissions_group_membership_test.clj b/test/metabase/models/permissions_group_membership_test.clj
index 0cbbf4f1c3344da599a17cc4474fc0bd00027d52..35c755cdc1bf25498ea0a1f6d1b25374e83277b9 100644
--- a/test/metabase/models/permissions_group_membership_test.clj
+++ b/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)))))))))
diff --git a/test/metabase/test.clj b/test/metabase/test.clj
index d59ecf1618d814c75c57e9a07fa5286e9329ab73..fbf53f074f5ec889c601ffdc2308cdcf69ef4344 100644
--- a/test/metabase/test.clj
+++ b/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