Skip to content
Snippets Groups Projects
Unverified Commit efd28c7e authored by Cam Saul's avatar Cam Saul Committed by GitHub
Browse files

config-from-file should check that we have a premium token with `:advanced-config` (#26463) 

* Move the config-from-file code into the advanced-config directory since that's the feature we want to flag on

* Always apply the :settings section first in the config file

* config-from-file should check that we have a premium token with the :advanced-config feature

* Sort namespace

* Settings does not have to come first anymore

* Remove empty `binding` forms

* Test fix? :wrench:

* Test fix? :wrench:



* Fix failing tests, for real this time

* Revert "Fix failing tests, for real this time"

This reverts commit 0a57055b1be249d9ba5b6b6ebe4f163e1f21e2b2.

* Don't make the tests parallel since that breaks other stuff

Co-authored-by: default avatarNemanja Glumac <31325167+nemanjaglumac@users.noreply.github.com>
parent 80279d88
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
Showing
with 174 additions and 149 deletions
enterprise/backend/src/metabase_enterprise/advanced_config/file.clj
+ 9
− 1
View file @ efd28c7e
......@@ -104,9 +104,10 @@
[metabase-enterprise.advanced-config.file.users]
[metabase.driver.common.parameters]
[metabase.driver.common.parameters.parse :as params.parse]
[metabase.public-settings.premium-features :as premium-features]
[metabase.util :as u]
[metabase.util.files :as u.files]
[metabase.util.i18n :refer [trs]]
[metabase.util.i18n :refer [trs tru]]
[yaml.core :as yaml]))
(comment
......@@ -252,6 +253,13 @@
;; enabling that check tho)
(when-let [m (config)]
(doseq [[section-name section-config] (sort-by-initialization-order (:config m))]
;; you can only use the config-from-file stuff with an EE/Pro token with the `:advanced-config` feature. Since you
;; might have to use the `:settings` section to set the token, skip the check for Settings. But check it for the
;; other sections.
(when-not (= section-name :settings)
(when-not (premium-features/has-feature? :advanced-config)
(throw (ex-info (tru "Metabase config files require a Premium token with the :advanced-config feature.")
{}))))
(log/info (u/colorize :magenta (trs "Initializing {0} from config file..." section-name)) (u/emoji "🗄️"))
(advanced-config.file.i/initialize-section! section-name section-config))
(log/info (u/colorize :magenta (trs "Done initializing from file.")) (u/emoji "🗄️")))
......
enterprise/backend/test/metabase_enterprise/advanced_config/file/databases_test.clj
+ 20
− 30
View file @ efd28c7e
(ns metabase-enterprise.advanced-config.file.databases-test
(:require
[clojure.test :refer :all]
[flatland.ordered.map :as ordered-map]
[metabase-enterprise.advanced-config.file :as advanced-config.file]
[metabase.db.connection :as mdb.connection]
[metabase.models :refer [Database Table]]
[metabase.public-settings.premium-features-test :as premium-features-test]
[metabase.test :as mt]
[metabase.util :as u]
[toucan.db :as db]))
(use-fixtures :each (fn [thunk]
(binding [advanced-config.file/*supported-versions* {:min 1, :max 1}]
(premium-features-test/with-premium-features #{:advanced-config}
(thunk)))))
(def ^:private test-db-name (u/qualified-name ::test-db))
(deftest init-from-config-file-test
......@@ -16,11 +21,10 @@
(let [db-type (mdb.connection/db-type)
original-db (mt/with-driver db-type (mt/db))]
(try
(binding [advanced-config.file/*supported-versions* {:min 1, :max 1}
advanced-config.file/*config* {:version 1
:config {:databases [{:name test-db-name
:engine (name db-type)
:details (:details original-db)}]}}]
(binding [advanced-config.file/*config* {:version 1
:config {:databases [{:name test-db-name
:engine (name db-type)
:details (:details original-db)}]}}]
(testing "Create a Database if it does not already exist"
(is (= :ok
(advanced-config.file/initialize!)))
......@@ -42,13 +46,12 @@
(finally
(db/delete! Database :name test-db-name))))))
(deftest ^:parallel init-from-config-file-connection-validation-test
(deftest init-from-config-file-connection-validation-test
(testing "Validate connection details when creating a Database from a config file, and error if they are invalid"
(binding [advanced-config.file/*supported-versions* {:min 1, :max 1}
advanced-config.file/*config* {:version 1
:config {:databases [{:name (str test-db-name "-in-memory")
:engine "h2"
:details {:db "mem:some-in-memory-db"}}]}}]
(binding [advanced-config.file/*config* {:version 1
:config {:databases [{:name (str test-db-name "-in-memory")
:engine "h2"
:details {:db "mem:some-in-memory-db"}}]}}]
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
#"Database cannot be found\."
......@@ -59,24 +62,11 @@
;; make sure we're actually testing something if it was already set to false locally.
(mt/with-temporary-setting-values [config-from-file-sync-databases true]
(try
(binding [advanced-config.file/*supported-versions* {:min 1, :max 1}
advanced-config.file/*config* {:version 1
:config
;; `settings:` HAS to come before `databases:`, otherwise the
;; flag won't be set when database sync stuff happens.
;;
;; Using a [[flatland.ordered.map]] here really isn't necessary
;; since this map only has two keys and will be created as an
;; `ArrayMap`, preserving the originally specified order... but
;; using [[ordered-map]] explicitly here makes this constraint
;; clearer I think. Also the YAML library reads stuff in as an
;; ordered map so this more closely matches the behavior when
;; using a file
(ordered-map/ordered-map
:settings {:config-from-file-sync-databases false}
:databases [{:name test-db-name
:engine "h2"
:details (:details (mt/db))}])}]
(binding [advanced-config.file/*config* {:version 1
:config {:settings {:config-from-file-sync-databases false}
:databases [{:name test-db-name
:engine "h2"
:details (:details (mt/db))}]}}]
(testing "Create a Database since it does not already exist"
(is (= :ok
(advanced-config.file/initialize!)))
......
enterprise/backend/test/metabase_enterprise/advanced_config/file/settings_test.clj
+ 30
− 25
View file @ efd28c7e
......@@ -2,7 +2,13 @@
(:require
[clojure.test :refer :all]
[metabase-enterprise.advanced-config.file :as advanced-config.file]
[metabase.models.setting :refer [defsetting]]))
[metabase.models.setting :refer [defsetting]]
[metabase.public-settings.premium-features-test :as premium-features-test]))
(use-fixtures :each (fn [thunk]
(binding [advanced-config.file/*supported-versions* {:min 1, :max 1}]
(premium-features-test/with-premium-features #{:advanced-config}
(thunk)))))
(defsetting config-from-file-settings-test-setting
"Internal test setting."
......@@ -11,29 +17,28 @@
(deftest settings-test
(testing "Should be able to set settings with config-from-file"
(config-from-file-settings-test-setting! nil)
(binding [advanced-config.file/*supported-versions* {:min 1, :max 1}]
(testing "happy path"
(binding [advanced-config.file/*config* {:version 1
:config {:settings {:config-from-file-settings-test-setting "wow"}}}]
(advanced-config.file/initialize!)
(is (= "wow"
(config-from-file-settings-test-setting)))))
(testing "Wrong value type should throw an error."
(binding [advanced-config.file/*config* {:version 1
:config {:settings {:config-from-file-settings-test-setting 1000}}}]
(testing "happy path"
(binding [advanced-config.file/*config* {:version 1
:config {:settings {:config-from-file-settings-test-setting "wow"}}}]
(advanced-config.file/initialize!)
(is (= "wow"
(config-from-file-settings-test-setting)))))
(testing "Wrong value type should throw an error."
(binding [advanced-config.file/*config* {:version 1
:config {:settings {:config-from-file-settings-test-setting 1000}}}]
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
#"Input .* does not match schema"
(advanced-config.file/initialize!)))
(testing "value should not have been updated"
(is (= "wow"
(config-from-file-settings-test-setting))))))
(testing "Invalid Setting (does not exist)"
(binding [advanced-config.file/*config* {:version 1
:config {:settings {:config-from-file-settings-test-setting-FAKE 1000}}}]
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
#"Input .* does not match schema"
(advanced-config.file/initialize!)))
(testing "value should not have been updated"
(is (= "wow"
(config-from-file-settings-test-setting))))))
(testing "Invalid Setting (does not exist)"
(binding [advanced-config.file/*config* {:version 1
:config {:settings {:config-from-file-settings-test-setting-FAKE 1000}}}]
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
#"Unknown setting: :config-from-file-settings-test-setting-FAKE"
(advanced-config.file/initialize!))))))))
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
#"Unknown setting: :config-from-file-settings-test-setting-FAKE"
(advanced-config.file/initialize!)))))))
enterprise/backend/test/metabase_enterprise/advanced_config/file/users_test.clj
+ 74
− 72
View file @ efd28c7e
......@@ -4,17 +4,22 @@
[metabase-enterprise.advanced-config.file :as advanced-config.file]
[metabase-enterprise.advanced-config.file.users :as advanced-config.file.users]
[metabase.models :refer [User]]
[metabase.public-settings.premium-features-test :as premium-features-test]
[metabase.util.password :as u.password]
[toucan.db :as db]))
(use-fixtures :each (fn [thunk]
(binding [advanced-config.file/*supported-versions* {:min 1, :max 1}]
(premium-features-test/with-premium-features #{:advanced-config}
(thunk)))))
(deftest init-from-config-file-test
(try
(binding [advanced-config.file/*supported-versions* {:min 1, :max 1}
advanced-config.file/*config* {:version 1
:config {:users [{:first_name "Cam"
:last_name "Era"
:email "cam+config-file-test@metabase.com"
:password "2cans"}]}}]
(binding [advanced-config.file/*config* {:version 1
:config {:users [{:first_name "Cam"
:last_name "Era"
:email "cam+config-file-test@metabase.com"
:password "2cans"}]}}]
(testing "Create a User if it does not already exist"
(is (= :ok
(advanced-config.file/initialize!)))
......@@ -56,58 +61,56 @@
(deftest init-from-config-file-force-admin-for-first-user-test
(testing "If this is the first user being created, always make the user a superuser regardless of what is specified"
(try
(binding [advanced-config.file/*supported-versions* {:min 1, :max 1}]
(testing "Create the first User"
(binding [advanced-config.file/*config* {:version 1
:config {:users [{:first_name "Cam"
:last_name "Era"
:email "cam+config-file-admin-test@metabase.com"
:password "2cans"
:is_superuser false}]}}]
(with-redefs [advanced-config.file.users/init-from-config-file-is-first-user? (constantly true)]
(is (= :ok
(advanced-config.file/initialize!)))
(is (partial= {:first_name "Cam"
:last_name "Era"
:email "cam+config-file-admin-test@metabase.com"
:is_superuser true}
(db/select-one User :email "cam+config-file-admin-test@metabase.com")))
(is (= 1
(db/count User :email "cam+config-file-admin-test@metabase.com"))))))
(testing "Create the another User, DO NOT force them to be an admin"
(binding [advanced-config.file/*config* {:version 1
:config {:users [{:first_name "Cam"
:last_name "Saul"
:email "cam+config-file-admin-test-2@metabase.com"
:password "2cans"
:is_superuser false}]}}]
(testing "Create the first User"
(binding [advanced-config.file/*config* {:version 1
:config {:users [{:first_name "Cam"
:last_name "Era"
:email "cam+config-file-admin-test@metabase.com"
:password "2cans"
:is_superuser false}]}}]
(with-redefs [advanced-config.file.users/init-from-config-file-is-first-user? (constantly true)]
(is (= :ok
(advanced-config.file/initialize!)))
(is (partial= {:first_name "Cam"
:last_name "Saul"
:email "cam+config-file-admin-test-2@metabase.com"
:is_superuser false}
(db/select-one User :email "cam+config-file-admin-test-2@metabase.com")))
:last_name "Era"
:email "cam+config-file-admin-test@metabase.com"
:is_superuser true}
(db/select-one User :email "cam+config-file-admin-test@metabase.com")))
(is (= 1
(db/count User :email "cam+config-file-admin-test-2@metabase.com"))))))
(db/count User :email "cam+config-file-admin-test@metabase.com"))))))
(testing "Create the another User, DO NOT force them to be an admin"
(binding [advanced-config.file/*config* {:version 1
:config {:users [{:first_name "Cam"
:last_name "Saul"
:email "cam+config-file-admin-test-2@metabase.com"
:password "2cans"
:is_superuser false}]}}]
(is (= :ok
(advanced-config.file/initialize!)))
(is (partial= {:first_name "Cam"
:last_name "Saul"
:email "cam+config-file-admin-test-2@metabase.com"
:is_superuser false}
(db/select-one User :email "cam+config-file-admin-test-2@metabase.com")))
(is (= 1
(db/count User :email "cam+config-file-admin-test-2@metabase.com")))))
(finally (db/delete! User :email [:in #{"cam+config-file-admin-test@metabase.com"
"cam+config-file-admin-test-2@metabase.com"}])))))
(deftest init-from-config-file-env-var-for-password-test
(testing "Ensure that we can set User password using {{env ...}} templates"
(try
(binding [advanced-config.file/*supported-versions* {:min 1, :max 1}
advanced-config.file/*config* {:version 1
:config {:users [{:first_name "Cam"
:last_name "Era"
:email "cam+config-file-password-test@metabase.com"
:password "{{env USER_PASSWORD}}"}]}}
advanced-config.file/*env* (assoc @#'advanced-config.file/*env* :user-password "1234cans")]
(binding [advanced-config.file/*config* {:version 1
:config {:users [{:first_name "Cam"
:last_name "Era"
:email "cam+config-file-password-test@metabase.com"
:password "{{env USER_PASSWORD}}"}]}}
advanced-config.file/*env* (assoc @#'advanced-config.file/*env* :user-password "1234cans")]
(testing "Create a User if it does not already exist"
(is (= :ok
(advanced-config.file/initialize!)))
(let [user (db/select-one [User :first_name :last_name :email :password_salt :password]
:email "cam+config-file-password-test@metabase.com")]
:email "cam+config-file-password-test@metabase.com")]
(is (partial= {:first_name "Cam"
:last_name "Era"
:email "cam+config-file-password-test@metabase.com"}
......@@ -116,34 +119,33 @@
(finally
(db/delete! User :email "cam+config-file-password-test@metabase.com")))))
(deftest ^:parallel init-from-config-file-validation-test
(binding [advanced-config.file/*supported-versions* {:min 1, :max 1}]
(are [user error-pattern] (thrown-with-msg?
clojure.lang.ExceptionInfo
error-pattern
(binding [advanced-config.file/*config* {:version 1
:config {:users [user]}}]
(#'advanced-config.file/config)))
;; missing email
{:first_name "Cam"
:last_name "Era"
:password "2cans"}
(re-pattern (java.util.regex.Pattern/quote "failed: (contains? % :email)"))
(deftest init-from-config-file-validation-test
(are [user error-pattern] (thrown-with-msg?
clojure.lang.ExceptionInfo
error-pattern
(binding [advanced-config.file/*config* {:version 1
:config {:users [user]}}]
(#'advanced-config.file/config)))
;; missing email
{:first_name "Cam"
:last_name "Era"
:password "2cans"}
(re-pattern (java.util.regex.Pattern/quote "failed: (contains? % :email)"))
;; missing first name
{:last_name "Era"
:email "cam+config-file-admin-test@metabase.com"
:password "2cans"}
(re-pattern (java.util.regex.Pattern/quote "failed: (contains? % :first_name)"))
;; missing first name
{:last_name "Era"
:email "cam+config-file-admin-test@metabase.com"
:password "2cans"}
(re-pattern (java.util.regex.Pattern/quote "failed: (contains? % :first_name)"))
;; missing last name
{:first_name "Cam"
:email "cam+config-file-admin-test@metabase.com"
:password "2cans"}
(re-pattern (java.util.regex.Pattern/quote "failed: (contains? % :last_name)"))
;; missing last name
{:first_name "Cam"
:email "cam+config-file-admin-test@metabase.com"
:password "2cans"}
(re-pattern (java.util.regex.Pattern/quote "failed: (contains? % :last_name)"))
;; missing password
{:first_name "Cam"
:last_name "Era"
:email "cam+config-file-test@metabase.com"}
(re-pattern (java.util.regex.Pattern/quote "failed: (contains? % :password)")))))
;; missing password
{:first_name "Cam"
:last_name "Era"
:email "cam+config-file-test@metabase.com"}
(re-pattern (java.util.regex.Pattern/quote "failed: (contains? % :password)"))))
enterprise/backend/test/metabase_enterprise/advanced_config/file_test.clj
+ 20
− 9
View file @ efd28c7e
......@@ -5,13 +5,15 @@
[clojure.walk :as walk]
[metabase-enterprise.advanced-config.file :as advanced-config.file]
[metabase-enterprise.advanced-config.file.interface :as advanced-config.file.i]
[metabase.public-settings.premium-features-test :as premium-features-test]
[metabase.test :as mt]
[metabase.util :as u]
[yaml.core :as yaml]))
(use-fixtures :each (fn [thunk]
(binding [advanced-config.file/*supported-versions* {:min 1.0, :max 1.999}]
(thunk))))
(premium-features-test/with-premium-features #{:advanced-config}
(thunk)))))
(defn- re-quote [^String s]
(re-pattern (java.util.regex.Pattern/quote s)))
......@@ -20,7 +22,7 @@
{:version 1
:config {:settings {:my-setting "abc123"}}})
(deftest ^:parallel config-test
(deftest config-test
(testing "Specify a custom path and read from YAML"
(mt/with-temp-file [filename "temp-config-file.yml"]
(spit filename (yaml/generate-string mock-yaml))
......@@ -34,7 +36,7 @@
:config {:settings {:my-setting "abc123"}}}
(#'advanced-config.file/config))))))
(deftest ^:parallel validate-config-test
(deftest validate-config-test
(testing "Config should throw an error"
(testing "if it is not a map"
(binding [advanced-config.file/*config* [1 2 3 4]]
......@@ -68,7 +70,7 @@
(defn- mock-config-with-setting [s]
{:version 1.0, :config {:settings {:my-setting s}}})
(deftest ^:parallel expand-template-forms-test
(deftest expand-template-forms-test
(testing "Ignore single curly brackets, or brackets with spaces between them"
(are [s] (= (mock-config-with-setting s)
(binding [advanced-config.file/*config* (mock-config-with-setting s)]
......@@ -92,14 +94,14 @@
;; unknown template type
"{{bird \"Parrot Hilton\"}}" (re-quote "bird - failed: valid-template-type?"))))
(deftest ^:parallel recursive-template-form-expansion-test
(deftest recursive-template-form-expansion-test
(testing "Recursive expansion is unsupported, for now."
(binding [advanced-config.file/*env* (assoc @#'advanced-config.file/*env* :x "{{env Y}}", :y "Y")
advanced-config.file/*config* (mock-config-with-setting "{{env X}}")]
(is (= (mock-config-with-setting "{{env Y}}")
(#'advanced-config.file/config))))))
(deftest ^:parallel expand-template-env-var-values-test
(deftest expand-template-env-var-values-test
(testing "env var values"
(binding [advanced-config.file/*env* (assoc @#'advanced-config.file/*env* :config-file-bird-name "Parrot Hilton")]
(testing "Nothing weird"
......@@ -125,7 +127,7 @@
(is (= (mock-config-with-setting "Parrot Hilton")
(#'advanced-config.file/config))))))))
(deftest ^:parallel expand-template-env-var-values-validation-test
(deftest expand-template-env-var-values-validation-test
(testing "(config) should walk the config map and expand {{template}} forms"
(testing "env var values"
(testing "validation"
......@@ -141,7 +143,7 @@
;; wrong env var type
"{{env :SOME_ENV_VAR}}" (re-quote "SOME_ENV_VAR - failed: symbol?"))))))
(deftest ^:parallel optional-template-test
(deftest optional-template-test
(testing "[[optional {{template}}]] values"
(binding [advanced-config.file/*env* (assoc @#'advanced-config.file/*env* :my-sensitive-password "~~~SeCrEt123~~~")]
(testing "env var exists"
......@@ -176,7 +178,16 @@
(is (= [[:warn nil (u/colorize :yellow "Ignoring unknown config section :unknown-section.")]]
log-messages))))))
(deftest ^:parallel error-validation-do-not-leak-env-vars-test
(deftest require-advanced-config-test
(testing "Config files should require the `:advanced-config` token feature"
(premium-features-test/with-premium-features #{}
(binding [advanced-config.file/*config* {:version 1.0, :config {:unknown-section {}}}]
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
#"Metabase config files require a Premium token with the :advanced-config feature"
(advanced-config.file/initialize!)))))))
(deftest error-validation-do-not-leak-env-vars-test
(testing "spec errors should not include contents of env vars -- expand templates after spec validation."
(binding [advanced-config.file/*env* (assoc @#'advanced-config.file/*env* :my-sensitive-password "~~~SeCrEt123~~~")
advanced-config.file/*config* {:version 1
......
test/metabase/public_settings/premium_features_test.clj
+ 21
− 12
View file @ efd28c7e
......@@ -78,21 +78,30 @@
:error-details "network issues"}
(premium-features/fetch-token-status (apply str (repeat 64 "b")))))))
(testing "Only attempt the token once"
(let [call-count (atom 0)]
(let [call-count (atom 0)
token (random-token)]
(binding [clj-http.client/request (fn [& _]
(swap! call-count inc)
(throw (Exception. "no internet")))]
(mt/with-temporary-raw-setting-values [:premium-embedding-token (random-token)]
(doseq [premium-setting [premium-features/hide-embed-branding?
premium-features/enable-whitelabeling?
premium-features/enable-audit-app?
premium-features/enable-sandboxes?
premium-features/enable-sso?
premium-features/enable-advanced-config?
premium-features/enable-content-management?]]
(is (false? (premium-setting))
(str (:name (meta premium-setting)) "is not false")))
(is (= @call-count 1))))))
(mt/with-temporary-raw-setting-values [:premium-embedding-token token]
(testing "Sanity check"
(is (= token
(premium-features/premium-embedding-token)))
(is (= #{}
(#'premium-features/token-features))))
(doseq [has-feature? [#'premium-features/hide-embed-branding?
#'premium-features/enable-whitelabeling?
#'premium-features/enable-audit-app?
#'premium-features/enable-sandboxes?
#'premium-features/enable-sso?
#'premium-features/enable-advanced-config?
#'premium-features/enable-content-management?
#'premium-features/enable-serialization?]]
(testing (format "\n%s is false" (:name (meta has-feature?)))
(is (not (has-feature?)))))
(is (= 1
@call-count))))))
(testing "With a valid token"
(let [result (token-status-response random-fake-token {:status 200
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment