Disallow engine change on sample db (#20447)

* for sample db do not allow certain updates
- namely to engine in the model layer

* only check for new schedules one time

* remove unnecessary *current-user-id* binding in test
- as per code review

* Use `thrown-with-message?` to check for exceptions
- Previously this would pass if db/update did _not_ throw an excetption,
  which is not what we want here.

src/metabase/models/database.clj

@@ -135,36 +135,48 @@
     database))
 
 (defn- pre-update
-  [{new-metadata-schedule :metadata_sync_schedule, new-fieldvalues-schedule :cache_field_values_schedule, :as database}]
-  (u/prog1 (handle-secrets-changes database)
-    ;; TODO - this logic would make more sense in post-update if such a method existed
-    ;; if the sync operation schedules have changed, we need to reschedule this DB
-    (when (or new-metadata-schedule new-fieldvalues-schedule)
-      (let [{old-metadata-schedule    :metadata_sync_schedule
-             old-fieldvalues-schedule :cache_field_values_schedule
-             existing-engine          :engine
-             existing-name            :name} (db/select-one [Database
-                                                             :metadata_sync_schedule
-                                                             :cache_field_values_schedule
-                                                             :engine
-                                                             :name]
-                                                            :id (u/the-id database))
-            ;; if one of the schedules wasn't passed continue using the old one
-            new-metadata-schedule            (or new-metadata-schedule old-metadata-schedule)
-            new-fieldvalues-schedule         (or new-fieldvalues-schedule old-fieldvalues-schedule)]
-        (when-not (= [new-metadata-schedule new-fieldvalues-schedule]
-                     [old-metadata-schedule old-fieldvalues-schedule])
-          (log/info
-           (trs "{0} Database ''{1}'' sync/analyze schedules have changed!" existing-engine existing-name)
-           "\n"
-           (trs "Sync metadata was: ''{0}'' is now: ''{1}''" old-metadata-schedule new-metadata-schedule)
-           "\n"
-           (trs "Cache FieldValues was: ''{0}'', is now: ''{1}''" old-fieldvalues-schedule new-fieldvalues-schedule))
-          ;; reschedule the database. Make sure we're passing back the old schedule if one of the two wasn't supplied
-          (schedule-tasks!
-           (assoc database
-             :metadata_sync_schedule      new-metadata-schedule
-             :cache_field_values_schedule new-fieldvalues-schedule)))))))
+  [{new-metadata-schedule    :metadata_sync_schedule,
+    new-fieldvalues-schedule :cache_field_values_schedule,
+    new-engine               :engine
+    :as                      database}]
+  (let [{is-sample?               :is_sample
+         old-metadata-schedule    :metadata_sync_schedule
+         old-fieldvalues-schedule :cache_field_values_schedule
+         existing-engine          :engine
+         existing-name            :name} (db/select-one [Database
+                                                         :metadata_sync_schedule
+                                                         :cache_field_values_schedule
+                                                         :engine
+                                                         :name
+                                                         :is_sample] :id (u/the-id database))
+        new-engine                       (some-> new-engine keyword)]
+    (if (and is-sample?
+             new-engine
+             (not= new-engine existing-engine))
+      (throw (ex-info (trs "The engine on a sample database cannot be changed.")
+                      {:status-code     400
+                       :existing-engine existing-engine
+                       :new-engine      new-engine}))
+      (u/prog1 (handle-secrets-changes database)
+        ;; TODO - this logic would make more sense in post-update if such a method existed
+        ;; if the sync operation schedules have changed, we need to reschedule this DB
+        (when (or new-metadata-schedule new-fieldvalues-schedule)
+          ;; if one of the schedules wasn't passed continue using the old one
+          (let [new-metadata-schedule    (or new-metadata-schedule old-metadata-schedule)
+                new-fieldvalues-schedule (or new-fieldvalues-schedule old-fieldvalues-schedule)]
+            (when (not= [new-metadata-schedule new-fieldvalues-schedule]
+                        [old-metadata-schedule old-fieldvalues-schedule])
+              (log/info
+               (trs "{0} Database ''{1}'' sync/analyze schedules have changed!" existing-engine existing-name)
+               "\n"
+               (trs "Sync metadata was: ''{0}'' is now: ''{1}''" old-metadata-schedule new-metadata-schedule)
+               "\n"
+               (trs "Cache FieldValues was: ''{0}'', is now: ''{1}''" old-fieldvalues-schedule new-fieldvalues-schedule))
+              ;; reschedule the database. Make sure we're passing back the old schedule if one of the two wasn't supplied
+              (schedule-tasks!
+               (assoc database
+                      :metadata_sync_schedule      new-metadata-schedule
+                      :cache_field_values_schedule new-fieldvalues-schedule)))))))))
 
 (defn- pre-insert [database]
   (-> database

src/metabase/sample_data.clj

@@ -38,6 +38,7 @@
 (defn update-sample-database-if-needed!
   "Update the path to the sample database DB if it exists in case the JAR has moved."
   []
-  (when-let [db (Database :is_sample true)]
-    (db/update! Database (:id db)
-      :details (db-details))))
+  (when-let [sample-db (Database :is_sample true)]
+    (let [intended (db-details)]
+      (when (not= (:details sample-db) intended)
+        (db/update! Database (:id sample-db) :details intended)))))

test/metabase/models/database_test.clj

@@ -6,7 +6,7 @@
             [metabase.driver :as driver]
             [metabase.driver.util :as driver.u]
             [metabase.models :refer [Database]]
-            [metabase.models.database :as mdb]
+            [metabase.models.database :as database]
             [metabase.models.permissions :as perms]
             [metabase.models.secret :as secret :refer [Secret]]
             [metabase.models.user :as user]
@@ -62,7 +62,7 @@
   (let [encode-decode (fn [obj] (decode (encode obj)))
         project-id    "random-project-id" ; the actual value here doesn't seem to matter
         ;; this is trimmed for the parts we care about in the test
-        pg-db         (mdb/map->DatabaseInstance
+        pg-db         (database/map->DatabaseInstance
                        {:description nil
                         :name        "testpg"
                         :details     {:additional-options            nil
@@ -80,7 +80,7 @@
                                       :tunnel-user                   "a-tunnel-user"
                                       :tunnel-private-key-passphrase "Password1234"}
                         :id          3})
-        bq-db         (mdb/map->DatabaseInstance
+        bq-db         (database/map->DatabaseInstance
                        {:description nil
                         :name        "testbq"
                         :details     {:use-service-account  nil
@@ -159,9 +159,9 @@
            (driver.u/sensitive-fields :test-sensitive-driver))))
   (testing "get-sensitive-fields-for-db returns default fields for null or empty database map"
     (is (= driver.u/default-sensitive-fields
-           (mdb/sensitive-fields-for-db nil)))
+           (database/sensitive-fields-for-db nil)))
     (is (= driver.u/default-sensitive-fields
-           (mdb/sensitive-fields-for-db {})))))
+           (database/sensitive-fields-for-db {})))))
 
 (deftest secret-db-details-integration-test
   (testing "manipulating secret values in db-details works correctly"
@@ -216,6 +216,20 @@
                   (is (nil? (db/select-one Secret :id secret-id))
                       (format "Secret ID %d was not removed from the app DB" secret-id)))))))))))
 
+(deftest user-may-not-update-sample-database-test
+  (mt/with-temp Database [{:keys [id details] :as sample-database} {:engine    :h2
+                                                                    :is_sample true
+                                                                    :name      "Sample Database"
+                                                                    :details   {:db "./resources/sample-database.db;USER=GUEST;PASSWORD=guest"}}]
+    (testing " updating the engine of a sample database is not allowed"
+      (is (thrown-with-msg?
+           clojure.lang.ExceptionInfo
+           #"The engine on a sample database cannot be changed."
+           (db/update! Database id :engine :sqlite))))
+    (testing " updating other attributes of a sample database is allowed"
+      (db/update! Database id :name "My New Name")
+      (is (= "My New Name" (db/select-one-field :name Database :id id))))))
+
 (driver/register! ::test, :abstract? true)
 
 (deftest preserve-driver-namespaces-test