Check user exists for setup redirect (#19846)

* Add has-user-setup setting

- iff no user is setup aka has-user-setup == false, then redirect to /setup
- when set from the env variable the setup-token is completely
  immutable, so can remove clear-token!

* make /api/setup only setup accounts when not has-user-setup

- react router changes to respect 'has-user-setup' setting

- include docstring for print-setup-url

* allow tests to setup multiple users via POST /api/setup

* remove unused function hasSetupToken

* Adds a test to enforce a single creation only

- we now throw an ex-info with status-code 403 when has-user-setup and
  /api/setup route is hit (with error message mentioning the route)
- more code review responses

- fixup test to not delete users, (or set locale to spanish...)

* addressing code review concerns

- *disallow... -> *allow... for the dynamic var name
- has-user-settings test: do not assume the test db has users in it
- update test

frontend/src/metabase/lib/settings.ts

@@ -70,6 +70,7 @@ export type SettingName =
   | "ga-enabled"
   | "google-auth-client-id"
   | "has-sample-database?"
+  | "has-user-setup"
   | "hide-embed-branding?"
   | "is-hosted?"
   | "ldap-configured?"
@@ -164,8 +165,8 @@ class Settings {
     return this.get("google-auth-client-id") != null;
   }
 
-  hasSetupToken() {
-    return this.get("setup-token") != null;
+  hasUserSetup() {
+    return this.get("has-user-setup");
   }
 
   hideEmbedBranding() {

frontend/src/metabase/routes.jsx

@@ -90,9 +90,9 @@ import SearchApp from "metabase/home/containers/SearchApp";
 import { trackPageView } from "metabase/lib/analytics";
 
 const MetabaseIsSetup = UserAuthWrapper({
-  predicate: authData => !authData.hasSetupToken,
+  predicate: authData => authData.hasUserSetup,
   failureRedirectPath: "/setup",
-  authSelector: state => ({ hasSetupToken: MetabaseSettings.hasSetupToken() }), // HACK
+  authSelector: state => ({ hasUserSetup: MetabaseSettings.hasUserSetup() }), // HACK
   wrapperDisplayName: "MetabaseIsSetup",
   allowRedirectBack: false,
   redirectAction: routerActions.replace,
@@ -141,7 +141,7 @@ export const getRoutes = store => (
       path="/setup"
       component={SetupApp}
       onEnter={(nextState, replace) => {
-        if (!MetabaseSettings.hasSetupToken()) {
+        if (MetabaseSettings.hasUserSetup()) {
           replace("/");
         }
         trackPageView(location.pathname);

src/metabase/api/setup.clj

@@ -38,21 +38,33 @@
   (su/with-api-error-message (s/constrained su/NonBlankString setup/token-match?)
     "Token does not match the setup token."))
 
+(def ^:dynamic ^:private *allow-api-setup-after-first-user-is-created*
+  "We must not allow users to setup multiple super users after the first user is created. But tests still need to be able
+  to. This var is redef'd to false by certain tests to allow that."
+  false)
+
 (defn- setup-create-user! [{:keys [email first-name last-name password]}]
+  (when (and (setup/has-user-setup)
+             (not *allow-api-setup-after-first-user-is-created*))
+    ;; many tests use /api/setup to setup multiple users, so *allow-api-setup-after-first-user-is-created* is
+    ;; redefined by them
+    (throw (ex-info
+            (tru "The /api/setup route can only be used to create the first user, however a user currently exists.")
+            {:status-code 403})))
   (let [session-id (str (UUID/randomUUID))
         new-user   (db/insert! User
-                               :email        email
-                               :first_name   first-name
-                               :last_name    last-name
-                               :password     (str (UUID/randomUUID))
-                               :is_superuser true)
+                     :email        email
+                     :first_name   first-name
+                     :last_name    last-name
+                     :password     (str (UUID/randomUUID))
+                     :is_superuser true)
         user-id    (u/the-id new-user)]
     ;; this results in a second db call, but it avoids redundant password code so figure it's worth it
     (user/set-password! user-id password)
     ;; then we create a session right away because we want our new user logged in to continue the setup process
     (let [session (or (db/insert! Session
-                                  :id      session-id
-                                  :user_id user-id)
+                        :id      session-id
+                        :user_id user-id)
                       ;; HACK -- Toucan doesn't seem to work correctly with models with string IDs
                       (t.models/post-insert (Session (str session-id))))]
       ;; return user ID, session ID, and the Session object itself
@@ -137,8 +149,6 @@
                  (setup-set-settings!
                   request
                   {:email email, :site-name site_name, :site-locale site_locale, :allow-tracking? allow_tracking})
-                 ;; clear the setup token now, it's no longer needed
-                 (setup/clear-token!)
                  (assoc user-info :database db)))
               (catch Throwable e
                 ;; if the transaction fails, restore the Settings cache from the DB again so any changes made in this

src/metabase/core.clj

@@ -44,10 +44,9 @@
 
 ;;; --------------------------------------------------- Lifecycle ----------------------------------------------------
 
-(defn- -init-create-setup-token
-  "Create and set a new setup token and log it."
+(defn- print-setup-url
+  "Print the setup url during instance initialization."
   []
-  (setup/create-token!)                 ; we need this here to create the initial token
   (let [hostname  (or (config/config-str :mb-jetty-host) "localhost")
         port      (config/config-int :mb-jetty-port)
         setup-url (str "http://"
@@ -60,6 +59,12 @@
                                    setup-url
                                    "\n\n")))))
 
+(defn- create-setup-token-and-log-setup-url!
+  "Create and set a new setup token and log it."
+  []
+  (setup/create-token!)   ; we need this here to create the initial token
+  (print-setup-url))
+
 (defn- destroy!
   "General application shutdown function which should be called once at application shuddown."
   []
@@ -105,7 +110,7 @@
     (when new-install?
       (log/info (trs "Looks like this is a new installation ... preparing setup wizard"))
       ;; create setup token
-      (-init-create-setup-token)
+      (create-setup-token-and-log-setup-url!)
       ;; publish install event
       (events/publish-event! :install {}))
     (init-status/set-progress! 0.9)

src/metabase/models/setting.clj

@@ -932,7 +932,7 @@
   (binding [*database-local-values* nil]
     (into
      {}
-     (comp (filter (fn [[_ setting]]
+     (comp (filter (fn [[_setting-name setting]]
                      (and (not (:sensitive? setting))
                           (allows-site-wide-values? setting)
                           (= (:visibility setting) visibility))))

src/metabase/setup.clj

 (ns metabase.setup
   (:require [environ.core :as env]
             [metabase.models.setting :as setting :refer [defsetting Setting]]
+            [metabase.models.user :refer [User]]
             [toucan.db :as db])
   (:import java.util.UUID))
 
@@ -25,12 +26,20 @@
   ;;
   ;; TODO -- 95% sure we can just use [[setup-token]] directly now and not worry about manually fetching the env var
   ;; value or setting DB values and the like
-  (let [mb-setup-token (env/env :mb-setup-token)]
-    (or (when mb-setup-token (setting/set-value-of-type! :string :setup-token mb-setup-token))
-        (db/select-one-field :value Setting :key "setup-token")
-        (setting/set-value-of-type! :string :setup-token (str (UUID/randomUUID))))))
+  (or (when-let [mb-setup-token (env/env :mb-setup-token)]
+        (setting/set-value-of-type! :string :setup-token mb-setup-token))
+      (db/select-one-field :value Setting :key "setup-token")
+      (setting/set-value-of-type! :string :setup-token (str (UUID/randomUUID)))))
 
-(defn clear-token!
-  "Clear the setup token if it exists and reset it to `nil`."
-  []
-  (setting/set-value-of-type! :string :setup-token nil))
+(defsetting has-user-setup
+  "A value that is true iff the metabase instance has one or more users registered."
+  :visibility :public
+  :type       :boolean
+  :setter     :none
+  ;; Once a User is created it's impossible for this to ever become falsey -- deleting the last User is disallowed.
+  ;; After this returns true once the result is cached and it will continue to return true forever without any
+  ;; additional DB hits.
+  :getter     (fn []
+                (let [user-exists? (atom false)]
+                  (or @user-exists?
+                      (reset! user-exists? (db/exists? User))))))

test/metabase/api/setup_test.clj

@@ -61,12 +61,13 @@
     (do-with-setup*
      request-body
      (fn []
-       (testing "API response should return a Session UUID"
-         (is (schema= {:id (s/pred mt/is-uuid-string? "UUID string")}
-                      (http/client :post 200 "setup" request-body))))
-       ;; reset our setup token
-       (setup/create-token!)
-       (thunk)))))
+       (with-redefs [setup-api/*allow-api-setup-after-first-user-is-created* true]
+         (testing "API response should return a Session UUID"
+           (is (schema= {:id (s/pred mt/is-uuid-string? "UUID string")}
+                        (http/client :post 200 "setup" request-body))))
+         ;; reset our setup token
+         (setup/create-token!)
+         (thunk))))))
 
 (defmacro ^:private with-setup [request-body & body]
   `(do-with-setup ~request-body (fn [] ~@body)))
@@ -202,10 +203,11 @@
     (testing "error conditions"
       (testing "should throw Exception if driver is invalid"
         (is (= {:errors {:database {:engine "Cannot create Database: cannot find driver my-fake-driver."}}}
-               (http/client :post 400 "setup" (assoc (default-setup-input)
-                                                     :database {:engine  "my-fake-driver"
-                                                                :name    (mt/random-name)
-                                                                :details {}}))))))))
+               (with-redefs [setup-api/*allow-api-setup-after-first-user-is-created* true]
+                 (http/client :post 400 "setup" (assoc (default-setup-input)
+                                                       :database {:engine  "my-fake-driver"
+                                                                  :name    (mt/random-name)
+                                                                  :details {}})))))))))
 
 (defn- setup! [f & args]
   (let [body {:token (setup/create-token!)
@@ -277,6 +279,39 @@
         (with-setup {:database {:engine "h2", :name db-name}}
           (is (db/exists? Database :name db-name)))))))
 
+(deftest has-user-setup-setting-test
+  (testing "has-user-setup is true iff there are 1 or more users"
+    (let [user-count (db/count User)]
+      (if (zero? user-count)
+        (is (not (setup/has-user-setup)))
+        (is (setup/has-user-setup))))))
+
+(deftest create-superuser-only-once-test
+  (testing "POST /api/setup"
+    (testing "Check that we cannot create a new superuser via setup-token when a user exists"
+      (let [token (setup/create-token!)
+            body  {:token token
+                   :prefs {:site_locale "es_MX"
+                           :site_name   (mt/random-name)}
+                   :user  {:first_name (mt/random-name)
+                           :last_name  (mt/random-name)
+                           :email      (mt/random-email)
+                           :password   "p@ssword1"}}]
+        (with-redefs [setup/has-user-setup (let [value (atom false)]
+                                             (fn
+                                               ([] @value)
+                                               ([t-or-f] (reset! value t-or-f))))]
+          (mt/discard-setting-changes
+              [site-name site-locale anon-tracking-enabled admin-email]
+            (http/client :post 200 "setup" body))
+
+          ;; In the non-test context, this is 'set' iff there is one or more users, and doesn't have to be toggled
+          (setup/has-user-setup true)
+
+          (mt/discard-setting-changes
+              [site-name site-locale anon-tracking-enabled admin-email]
+            (http/client :post 403 "setup" (assoc-in body [:user :email] (mt/random-email)))))))))
+
 (deftest transaction-test
   (testing "POST /api/setup/"
     (testing "should run in a transaction -- if something fails, all changes should be rolled back"
@@ -296,7 +331,8 @@
         (do-with-setup*
          body
          (fn []
-           (with-redefs [setup-api/setup-set-settings! (let [orig @#'setup-api/setup-set-settings!]
+           (with-redefs [setup-api/*allow-api-setup-after-first-user-is-created* true
+                         setup-api/setup-set-settings! (let [orig @#'setup-api/setup-set-settings!]
                                                          (fn [& args]
                                                            (apply orig args)
                                                            (throw (ex-info "Oops!" {}))))]