Create the internal user in a migration (#40907)

enterprise/backend/src/metabase_enterprise/audit_db.clj

@@ -3,7 +3,6 @@
    [babashka.fs :as fs]
    [clojure.java.io :as io]
    [clojure.string :as str]
-   [metabase-enterprise.internal-user :as ee.internal-user]
    [metabase-enterprise.serialization.cmd :as serialization.cmd]
    [metabase.db :as mdb]
    [metabase.models.database :refer [Database]]
@@ -251,7 +250,6 @@
 (defn- maybe-load-analytics-content!
   [audit-db]
   (when analytics-dir-resource
-    (ee.internal-user/ensure-internal-user-exists!)
     (adjust-audit-db-to-source! audit-db)
     (ia-content->plugins (plugins/plugins-dir))
     (let [[last-checksum current-checksum] (get-last-and-current-checksum)]

enterprise/backend/src/metabase_enterprise/internal_user.clj

-(ns metabase-enterprise.internal-user
-  (:require [metabase.config :as config]
-            [metabase.models :refer [User]]
-            [metabase.util.log :as log]
-            [toucan2.core :as t2]))
-
-(defn- install-internal-user! []
-  (t2/insert-returning-instances!
-   User
-   {:id config/internal-mb-user-id
-    :first_name "Metabase"
-    :last_name "Internal"
-    :email "internal@metabase.com"
-    :password (str (random-uuid))
-    :is_active false
-    :is_superuser false
-    :login_attributes nil
-    :sso_source nil
-    :type :internal}))
-
-(defn ensure-internal-user-exists!
-  "Creates the internal user"
-  []
-  (if-not (t2/exists? User :id config/internal-mb-user-id)
-    (do (log/info "No internal user found, creating now...")
-        (install-internal-user!)
-        ::installed)
-    ::no-op))

enterprise/backend/test/metabase_enterprise/serialization/cmd_test.clj

@@ -9,9 +9,11 @@
    [metabase-enterprise.serialization.v2.storage :as v2.storage]
    [metabase.analytics.snowplow-test :as snowplow-test]
    [metabase.cmd :as cmd]
-   [metabase.models :refer [Card Collection Dashboard DashboardCard Database User]]
+   [metabase.models :refer [Card Collection Dashboard DashboardCard Database
+                            User]]
    [metabase.test :as mt]
    [metabase.test.fixtures :as fixtures]
+   [metabase.test.initialize.test-users :as test-users]
    [metabase.util :as u]
    [metabase.util.log :as log]
    [metabase.util.yaml :as yaml]
@@ -66,17 +68,21 @@
 (deftest blank-target-db-test
   (testing "Loading a dump into an empty app DB still works (#16639)"
     (mt/with-premium-features #{:serialization}
-      (let [dump-dir                 (ts/random-dump-dir "serdes-")
-            user-pre-insert-called?  (atom false)]
-        (log/infof "Dumping to %s" dump-dir)
-        (cmd/dump dump-dir "--user" "crowberto@metabase.com")
-        (mt/with-empty-h2-app-db
-          (with-redefs [load/pre-insert-user  (fn [user]
-                                                (reset! user-pre-insert-called? true)
-                                                (assoc user :password "test-password"))]
-            (cmd/load dump-dir "--mode"     "update"
-                      "--on-error" "abort")
-            (is (true? @user-pre-insert-called?))))))))
+      (ts/with-dbs [source-db dest-db]
+        (let [dump-dir                 (ts/random-dump-dir "serdes-")
+              user-pre-insert-called?  (atom false)]
+          (log/infof "Dumping to %s" dump-dir)
+          (ts/with-db source-db
+            (test-users/init!)
+            (ts/create! :model/Collection :name "My_Collection")
+            (cmd/dump dump-dir "--user" "crowberto@metabase.com"))
+          (ts/with-db dest-db
+            (with-redefs [load/pre-insert-user (fn [user]
+                                                 (reset! user-pre-insert-called? true)
+                                                 (assoc user :password "test-password"))]
+              (cmd/load dump-dir "--mode"     "update"
+                        "--on-error" "abort")
+              (is (true? @user-pre-insert-called?)))))))))
 
 (deftest mode-update-remove-cards-test
   (testing "--mode update should remove Cards in a Dashboard if they're gone from the serialized YAML (#20786)"

enterprise/backend/test/metabase_enterprise/serialization/v2/e2e_test.clj

@@ -222,7 +222,7 @@
                :pulse-channel-recipient (many-random-fks 40 {} {:pulse_channel_id [:pulse-channel 30]
                                                                 :user_id          [:u 100]})}))
 
-          (is (= 100 (count (t2/select-fn-set :email 'User))))
+          (is (= 101 (count (t2/select-fn-set :email 'User)))) ; +1 for the internal user
 
           (testing "extraction"
             (reset! extraction (serdes/with-cache (into [] (extract/extract {}))))

resources/migrations/001_update_migrations.yaml

@@ -6106,6 +6106,14 @@ databaseChangeLog:
                     nullable: false
                   defaultValue: true
 
+  - changeSet:
+      id: v50.2024-03-28T16:30:35
+      author: calherries
+      comment: Create internal user
+      changes:
+        - customChange:
+            class: "metabase.db.custom_migrations.CreateInternalUser"
+
   # >>>>>>>>>> DO NOT ADD NEW MIGRATIONS BELOW THIS LINE! ADD THEM ABOVE <<<<<<<<<<
 
 ########################################################################################################################

src/metabase/cmd/copy.clj

@@ -199,7 +199,8 @@
   "Make sure [target] application DB is empty before we start copying data."
   [data-source]
   ;; check that there are no Users yet
-  (let [[{:keys [cnt]}] (jdbc/query {:datasource data-source} "SELECT count(*) AS cnt FROM core_user;")]
+  (let [[{:keys [cnt]}] (jdbc/query {:datasource data-source} ["SELECT count(*) AS cnt FROM core_user where not id = ?;"
+                                                               config/internal-mb-user-id])]
     (assert (integer? cnt))
     (when (pos? cnt)
       (throw (ex-info (trs "Target DB is already populated!")

src/metabase/db/custom_migrations.clj

@@ -18,6 +18,7 @@
    [clojurewerkz.quartzite.scheduler :as qs]
    [clojurewerkz.quartzite.triggers :as triggers]
    [medley.core :as m]
+   [metabase.config :as config]
    [metabase.db.connection :as mdb.connection]
    [metabase.models.interface :as mi]
    [metabase.plugins.classloader :as classloader]
@@ -31,7 +32,8 @@
    (liquibase.change Change)
    (liquibase.change.custom CustomTaskChange CustomTaskRollback)
    (liquibase.exception ValidationErrors)
-   (liquibase.util BooleanUtil)))
+   (liquibase.util BooleanUtil)
+   (org.mindrot.jbcrypt BCrypt)))
 
 (set! *warn-on-reflection* true)
 
@@ -1068,6 +1070,49 @@
                                            :from   [:revision]
                                            :where  [:= :model "Card"]})))))
 
+(defn- hash-bcrypt
+  "Hashes a given plaintext password using bcrypt.  Should be used to hash
+   passwords included in stored user credentials that are to be later verified
+   using `bcrypt-credential-fn`."
+  [password]
+  (BCrypt/hashpw password (BCrypt/gensalt)))
+
+(defn- internal-user-exists? []
+  (pos? (first (vals (t2/query-one {:select [:%count.*] :from :core_user :where [:= :id config/internal-mb-user-id]})))))
+
+(define-migration CreateInternalUser
+  ;; the internal user may have been created in a previous version for Metabase Analytics, so don't add it again if it
+  ;; exists already.
+  (when (not (internal-user-exists?))
+    (let [salt     (str (random-uuid))
+          password (hash-bcrypt (str salt (random-uuid)))
+          user     {;; we insert the internal user ID directly because it's
+                    ;; deliberately high enough to not conflict with any other
+                    :id               config/internal-mb-user-id
+                    :password_salt    salt
+                    :password         password
+                    :email            "internal@metabase.com"
+                    :first_name       "Metabase"
+                    :locale           nil
+                    :last_login       nil
+                    :is_active        false
+                    :settings         nil
+                    :type             "internal"
+                    :is_qbnewb        true
+                    :updated_at       nil
+                    :reset_triggered  nil
+                    :is_superuser     false
+                    :login_attributes nil
+                    :reset_token      nil
+                    :last_name        "Internal"
+                    :date_joined      :%now
+                    :sso_source       nil
+                    :is_datasetnewb   true}]
+      (t2/query {:insert-into :core_user :values [user]}))
+    (let [all-users-id (first (vals (t2/query-one {:select [:id] :from :permissions_group :where [:= :name "All Users"]})))
+          perms-group  {:user_id config/internal-mb-user-id :group_id all-users-id}]
+      (t2/query {:insert-into :permissions_group_membership :values [perms-group]}))))
+
 ;; This was renamed to TruncateAuditTables, so we need to delete the old job & trigger
 (define-migration DeleteTruncateAuditLogTask
   (classloader/the-classloader)

enterprise/backend/test/metabase_enterprise/internal_user_test.clj → test/metabase/db/internal_user_test.clj

-(ns metabase-enterprise.internal-user-test
+(ns ^:mb/once metabase.db.internal-user-test
   (:require
    [clojure.string :as str]
    [clojure.test :refer :all]
-   [metabase-enterprise.internal-user :as ee.internal-user]
    [metabase.config :as config]
-   [metabase.models :refer [User]]
-   [metabase.setup :as setup]
    [metabase.test :as mt]
    [metabase.util :as u]
    [toucan2.core :as t2]))
 
-(defmacro with-internal-user-restoration [& body]
-  `(let [original-audit-db# (t2/select-one User :id config/internal-mb-user-id)]
-     (try
-       (t2/delete! User :id config/internal-mb-user-id)
-       ~@body
-       (finally
-         (t2/delete! User :id config/internal-mb-user-id)
-         (when original-audit-db#
-           (#'ee.internal-user/install-internal-user!))))))
-
-(deftest installation-of-internal-user-test
-  (with-internal-user-restoration
-    (is (= nil (t2/select-one User :id config/internal-mb-user-id)))
-    (ee.internal-user/ensure-internal-user-exists!)
-    (is (map? (t2/select-one User :id config/internal-mb-user-id)))))
-
-(deftest internal-user-installed-one-time-test
-  (with-internal-user-restoration
-    (is (= nil (t2/select-one User :id config/internal-mb-user-id)))
-    (ee.internal-user/ensure-internal-user-exists!)
-    (is (map? (t2/select-one User :id config/internal-mb-user-id)))
-    (ee.internal-user/ensure-internal-user-exists!)
-    (is (map? (t2/select-one User :id config/internal-mb-user-id)))))
-
-(deftest has-user-setup-ignores-internal-user
-  (with-internal-user-restoration
-    (if config/ee-available?
-      ;; ee:
-      (let [user-ids-minus-internal (t2/select-fn-set :id User {:where [:not= :id config/internal-mb-user-id]})]
-        (is (true? (setup/has-user-setup)))
-        (is (false? (contains? user-ids-minus-internal config/internal-mb-user-id))
-            "Selecting Users with a clause to ignore internal users does not return the internal user."))
-      ;; oss:
-      (is (= (t2/select-fn-set :id User {:where [:not= :id config/internal-mb-user-id]})
-             (t2/select-fn-set :id User))
-          "Ignore internal user where clause does nothing in ee mode.")))
-  (is (= (setup/has-user-setup)
-         (with-internal-user-restoration
-           ;; there's no internal-user in this block
-           (setup/has-user-setup)))))
+(deftest internal-user-is-in-all-users-test
+  (testing "The internal user should be in the All Users group, just like any other user."
+    (let [memberships  (-> (t2/select-one :model/User config/internal-mb-user-id)
+                           (t2/hydrate :user_group_memberships)
+                           :user_group_memberships
+                           (->> (map :id)))
+          all-users-id (t2/select-one-pk :model/PermissionsGroup :name "All Users")]
+      (is [all-users-id] memberships))))
 
 (deftest internal-user-is-unmodifiable-via-api-test
-  ;; ensure the internal user exists for these test assertions
-  (ee.internal-user/ensure-internal-user-exists!)
   (testing "GET /api/user"
-    ;; since the Internal user is deactivated, we only need to check in the `:include_deactivated` `true`
-    (let [{:keys [data total]} (mt/user-http-request :crowberto :get 200 "user", :include_deactivated true)]
+    ;; since the Internal user is inactive, we only need to check in the `:include_deactivated` `true`
+    (let [{:keys [data total]} (mt/user-http-request :crowberto :get 200 "user", :include_deactivated true)
+          internal-user-email  (t2/select-one-fn :email :model/User config/internal-mb-user-id)]
       (testing "does not return the internal user"
-        (is (not (some #{"internal@metabase.com"} (map :email data)))))
+        (is (not-any? (comp #{internal-user-email} :email) data)))
       (testing "does not count the internal user"
         (is (= total (count data))))))
   (testing "User Endpoints with :id"
@@ -69,7 +33,7 @@
                                            [:put "user/:id/modal/qbnewb" 400]
                                            [:post "user/:id/send_invite" 400]]]
       (let [endpoint (str/replace endpoint #":id" (str config/internal-mb-user-id))
-            testing-details-string (str/join " " [(u/upper-case-en (name :get))
+            testing-details-string (str/join " " [(u/upper-case-en (name method))
                                                   endpoint
                                                   "does not allow modifying the internal user"])]
         (testing testing-details-string

test/metabase/db/schema_migrations_test.clj

@@ -12,6 +12,7 @@
    [clojure.java.jdbc :as jdbc]
    [clojure.test :refer :all]
    [java-time.api :as t]
+   [metabase.config :as config]
    [metabase.db :as mdb]
    [metabase.db.custom-migrations-test :as custom-migrations-test]
    [metabase.db.query :as mdb.query]
@@ -1180,6 +1181,47 @@
                                         (t2/table-name :model/DataPermissions)
                                         :db_id db-id :group_id group-id :perm_type "perms/manage-database"))))))))
 
+(deftest create-internal-user-test
+  (testing "The internal user is created if it doesn't already exist"
+    (impl/test-migrations "v50.2024-03-28T16:30:35" [migrate!]
+      (let [get-users #(t2/query "SELECT * FROM core_user")]
+        (is (= [] (get-users)))
+        (migrate!)
+        (is (=? [{:id               config/internal-mb-user-id
+                  :first_name       "Metabase"
+                  :last_name        "Internal"
+                  :email            "internal@metabase.com"
+                  :password         some?
+                  :password_salt    some?
+                  :is_active        false
+                  :is_superuser     false
+                  :login_attributes nil
+                  :sso_source       nil
+                  :type             "internal"
+                  :date_joined      some?}]
+                (get-users))))))
+  (testing "The internal user isn't created again if it already exists"
+    (impl/test-migrations "v50.2024-03-28T16:30:35" [migrate!]
+      (t2/insert-returning-pks!
+       :core_user
+       ;; Copied from the old `metabase-enterprise.internal-user` namespace
+       {:id               config/internal-mb-user-id
+        :first_name       "Metabase"
+        :last_name        "Internal"
+        :email            "internal@metabase.com"
+        :password         (str (random-uuid))
+        :password_salt    (str (random-uuid))
+        :is_active        false
+        :is_superuser     false
+        :login_attributes nil
+        :sso_source       nil
+        :type             "internal"
+        :date_joined      :%now})
+      (let [get-users    #(t2/query "SELECT * FROM core_user")
+            users-before (get-users)]
+        (migrate!)
+        (is (= users-before (get-users)))))))
+
 (deftest data-permissions-migration-rollback-test
   (testing "Data permissions are correctly rolled back from `data_permissions` to `permissions`"
     (impl/test-migrations ["v50.2024-01-04T13:52:51" "v50.2024-02-19T21:32:04"] [migrate!]

test/metabase/query_processor/middleware/metrics_test.clj

@@ -107,7 +107,6 @@
                          (lib/aggregate (lib/avg (lib.metadata/field mp (mt/id :products :rating)))))]
     (mt/with-temp [:model/Card source-metric {:dataset_query (lib.convert/->legacy-MBQL source-query)
                                               :database_id (mt/id)
-                                              :creator_id 1
                                               :type :metric}]
       (let [query {:lib/type :mbql/query
                    :database (mt/id)

test/metabase/setup_test.clj

 (ns ^:mb/once metabase.setup-test
   (:require
    [clojure.test :refer :all]
-   [metabase.core :as mbc]
+   [metabase.config :as config]
    [metabase.db :as mdb]
-   [metabase.db.schema-migrations-test.impl :as schema-migrations-test.impl]
    [metabase.setup :as setup]
    [metabase.test :as mt]
    [toucan2.core :as t2]))
 
-(deftest has-user-setup-test
-  (testing "The has-user-setup getter should return falsey for an empty instance with only an internal user"
-    ;; create a new completely empty database.
-    (schema-migrations-test.impl/with-temp-empty-app-db [_conn :h2]
-      ;; make sure the DB is set up
-      (mdb/setup-db!)
-      ;; install audit DB, which creates an internal user as a side effect (on EE instances))
-      (mbc/ensure-audit-db-installed!)
-      (is (= false
-             (setup/has-user-setup))))))
+(deftest has-user-setup-ignores-internal-user-test
+  (mt/with-empty-h2-app-db
+    (is (t2/exists? :model/User :id config/internal-mb-user-id)
+        "Sense check the internal user exists")
+    (testing "`has-user-setup` should return false for an empty instance with only an internal user"
+      (is (false? (setup/has-user-setup))))
+    (testing "`has-user-setup` should return true as soon as a user is created"
+      (mt/with-temp [:model/User _ {}]
+        (is (true? (setup/has-user-setup)))))))
 
 (deftest has-user-setup-cached-test
   (testing "The has-user-setup getter should cache truthy results since it can never become falsey"