Add :sensitive? Settings; move Settings cache logic into new ns

src/metabase/api/setting.clj

@@ -22,11 +22,7 @@
   [key]
   {key su/NonBlankString}
   (api/check-superuser)
-  (let [k (keyword key)
-        v (setting/get k)]
-    ;; for security purposes, don't return value of a setting if it was defined via env var
-    (when (not= v (setting/env-var-value k))
-      v)))
+  (setting/user-facing-value key))
 
 (api/defendpoint PUT "/:key"
   "Create/update a `Setting`. You must be a superuser to do this.

src/metabase/email.clj

@@ -12,13 +12,25 @@
   (:import javax.mail.Session))
 
 ;;; CONFIG
+
+(defsetting email-from-address
+  (tru "Email address you want to use as the sender of Metabase.")
+  :default "notifications@metabase.com")
+
+(defsetting email-smtp-host
+  (tru "The address of the SMTP server that handles your emails."))
+
+(defsetting email-smtp-username
+  (tru "SMTP username."))
+
+(defsetting email-smtp-password
+  (tru "SMTP password.")
+  :sensitive? true)
+
 ;; TODO - smtp-port should be switched to type :integer
+(defsetting email-smtp-port
+  (tru "The port your SMTP server uses for outgoing emails."))
 
-(defsetting email-from-address  (tru "Email address you want to use as the sender of Metabase.") :default "notifications@metabase.com")
-(defsetting email-smtp-host     (tru "The address of the SMTP server that handles your emails."))
-(defsetting email-smtp-username (tru "SMTP username."))
-(defsetting email-smtp-password (tru "SMTP password."))
-(defsetting email-smtp-port     (tru "The port your SMTP server uses for outgoing emails."))
 (defsetting email-smtp-security
   (tru "SMTP secure connection protocol. (tls, ssl, starttls, or none)")
   :default (tru "none")

src/metabase/integrations/ldap.clj

@@ -39,7 +39,8 @@
   (tru "The Distinguished Name to bind as (if any), this user will be used to lookup information about other users."))
 
 (defsetting ldap-password
-  (tru "The password to bind with for the lookup user."))
+  (tru "The password to bind with for the lookup user.")
+  :sensitive? true)
 
 (defsetting ldap-user-base
   (tru "Search base for users. (Will be searched recursively)"))

src/metabase/models/setting/cache.clj

+(ns metabase.models.setting.cache
+  "Settings cache. Cache is a 1:1 mapping of what's in the DB. Cached lookup time is ~60µs, compared to ~1800µs for DB
+  lookup."
+  (:require [clojure.core :as core]
+            [clojure.core.memoize :as memoize]
+            [clojure.java.jdbc :as jdbc]
+            [clojure.tools.logging :as log]
+            [honeysql.core :as hsql]
+            [metabase
+             [db :as mdb]
+             [util :as u]]
+            [metabase.util
+             [honeysql-extensions :as hx]
+             [i18n :as ui18n :refer [trs]]]
+            [toucan.db :as db]))
+
+(def ^:private cache*
+  "Settings cache. Map of Setting key (string) -> Setting value (string)."
+  (atom nil))
+
+(defn cache
+  "Fetch the current contents of the Settings cache, a map of key (string) -> value (string)."
+  []
+  @cache*)
+
+(defn update-cache!
+  "Update the String value of a Setting in the Settings cache."
+  [setting-name, ^String new-value]
+  (if (seq new-value)
+    (swap! cache* assoc  setting-name new-value)
+    (swap! cache* dissoc setting-name)))
+
+;; CACHE SYNCHRONIZATION
+;;
+;; When running multiple Metabase instances (horizontal scaling), it is of course possible for one instance to update
+;; a Setting, and, since Settings are cached (to avoid tons of DB calls), for the other instances to then have an
+;; out-of-date cache. Thus we need a way for instances to know when their caches are out of date, so they can update
+;; them accordingly. Here is our solution:
+;;
+;; We will record the last time *any* Setting was updated in a special Setting called `settings-last-updated`.
+;;
+;; Since `settings-last-updated` itself is a Setting, it will get fetched as part of each instance's local cache; we
+;; can then periodically compare the locally cached value of `settings-last-updated` with the value in the DB. If our
+;; locally cached value is older than the one in the DB, we will flush our cache. When the cache is fetched again, it
+;; will have the up-to-date value.
+;;
+;; Because different machines can have out-of-sync clocks, we'll rely entirely on the application DB for caclulating
+;; and comparing values of `settings-last-updated`. Because the Setting table itself only stores text values, we'll
+;; need to cast it between TEXT and TIMESTAMP SQL types as needed.
+
+(def ^String settings-last-updated-key "settings-last-updated")
+
+(defn update-settings-last-updated!
+  "Update the value of `settings-last-updated` in the DB; if the row does not exist, insert one."
+  []
+  (log/debug (trs "Updating value of settings-last-updated in DB..."))
+  ;; for MySQL, cast(current_timestamp AS char); for H2 & Postgres, cast(current_timestamp AS text)
+  (let [current-timestamp-as-string-honeysql (hx/cast (if (= (mdb/db-type) :mysql) :char :text)
+                                                      (hsql/raw "current_timestamp"))]
+    ;; attempt to UPDATE the existing row. If no row exists, `update-where!` will return false...
+    (or (db/update-where! 'Setting {:key settings-last-updated-key} :value current-timestamp-as-string-honeysql)
+        ;; ...at which point we will try to INSERT a new row. Note that it is entirely possible two instances can both
+        ;; try to INSERT it at the same time; one instance would fail because it would violate the PK constraint on
+        ;; `key`, and throw a SQLException. As long as one instance updates the value, we are fine, so we can go ahead
+        ;; and ignore that Exception if one is thrown.
+        (try
+          ;; Use `simple-insert!` because we do *not* want to trigger pre-insert behavior, such as encrypting `:value`
+          (db/simple-insert! 'Setting :key settings-last-updated-key, :value current-timestamp-as-string-honeysql)
+          (catch java.sql.SQLException e
+            ;; go ahead and log the Exception anyway on the off chance that it *wasn't* just a race condition issue
+            (log/error (trs "Error inserting a new Setting: {0}"
+                            (with-out-str (jdbc/print-sql-exception-chain e))))))))
+  ;; Now that we updated the value in the DB, go ahead and update our cached value as well, because we know about the
+  ;; changes
+  (swap! cache* assoc settings-last-updated-key (db/select-one-field :value 'Setting :key settings-last-updated-key)))
+
+(defn- cache-out-of-date?
+  "Check whether our Settings cache is out of date. We know the cache is out of date if either of the following
+  conditions is true:
+
+   *  The cache is empty (the `cache*` atom is `nil`), which of course means it needs to be updated
+   *  There is a value of `settings-last-updated` in the cache, and it is older than the value of in the DB. (There
+      will be no value until the first time a normal Setting is updated; thus if it is not yet set, we do not yet need
+      to invalidate our cache.)"
+  []
+  (log/debug (trs "Checking whether settings cache is out of date (requires DB call)..."))
+  (boolean
+   (or
+    ;; is the cache empty?
+    (not @cache*)
+    ;; if not, get the cached value of `settings-last-updated`, and if it exists...
+    (when-let [last-known-update (core/get @cache* settings-last-updated-key)]
+      ;; compare it to the value in the DB. This is done be seeing whether a row exists
+      ;; WHERE value > <local-value>
+      (u/prog1 (db/select-one 'Setting
+                 {:where [:and
+                          [:= :key settings-last-updated-key]
+                          [:> :value last-known-update]]})
+        (when <>
+          (log/info (u/format-color 'red
+                        (str (trs "Settings have been changed on another instance, and will be reloaded here."))))))))))
+
+(def ^:private cache-update-check-interval-ms
+  "How often we should check whether the Settings cache is out of date (which requires a DB call)?"
+  ;; once a minute
+  (* 60 1000))
+
+(def ^:private ^{:arglists '([])} should-restore-cache?
+  "TTL-memoized version of `cache-out-of-date?`. Call this function to see whether we need to repopulate the cache with
+  values from the DB."
+  (memoize/ttl cache-out-of-date? :ttl/threshold cache-update-check-interval-ms))
+
+(def ^:private restore-cache-if-needed-lock (Object.))
+
+(defn restore-cache-if-needed!
+  "Check whether we need to repopulate the cache with fresh values from the DB (because the cache is either empty or
+  known to be out-of-date), and do so if needed. This is intended to be called every time a Setting value is
+  retrieved, so it should be efficient; thus the calculation (`should-restore-cache?`) is itself TTL-memoized."
+  []
+  ;; There's a potential race condition here where two threads both call this at the exact same moment, and both get
+  ;; `true` when they call `should-restore-cache`, and then both simultaneously try to update the cache (or, one
+  ;; updates the cache, but the other calls `should-restore-cache?` and gets `true` before the other calls
+  ;; `memo-swap!` (see below))
+  ;;
+  ;; This is not desirable, since either situation would result in duplicate work. Better to just add a quick lock
+  ;; here so only one of them does it, since at any rate waiting for the other thread to finish the task in progress is
+  ;; certainly quicker than starting the task ourselves from scratch
+  (locking restore-cache-if-needed-lock
+    (when (should-restore-cache?)
+      (log/debug (trs "Refreshing Settings cache..."))
+      (reset! cache* (db/select-field->field :key :value 'Setting))
+      ;; Now the cache is up-to-date. That is all good, but if we call `should-restore-cache?` again in a second it
+      ;; will still return `true`, because its result is memoized, and we would be on the hook to (again) update the
+      ;; cache. So go ahead and clear the memozied results for `should-restore-cache?`. The next time around when
+      ;; someone calls this it will cache the latest value (which should be `false`)
+      ;;
+      ;; NOTE: I tried using `memo-swap!` instead to set the cached response to `false` here, avoiding the extra DB
+      ;; call the next fn call would make, but it didn't seem to work correctly (I think it was still discarding the
+      ;; new value because of the TTL). So we will just stick with `memo-clear!` for now. (One extra DB call whenever
+      ;; the cache gets invalidated shouldn't be a huge deal)
+      (memoize/memo-clear! should-restore-cache?))))

test/metabase/api/setting_test.clj

 (ns metabase.api.setting-test
-  (:require [expectations :refer :all]
-            [metabase.models.setting-test :refer [set-settings! test-setting-1 test-setting-2]]
-            [metabase.test.data.users :refer :all]))
+  (:require [expectations :refer [expect]]
+            [metabase.models.setting-test :refer [set-settings! test-sensitive-setting test-setting-1 test-setting-2]]
+            [metabase.test.data.users :refer [user->client]]))
 
 ;; ## Helper Fns
 (defn- fetch-test-settings  []
@@ -15,8 +15,18 @@
 ;; ## GET /api/setting
 ;; Check that we can fetch all Settings for Org
 (expect
- [{:key "test-setting-1", :value nil,     :is_env_setting true,  :env_name "MB_TEST_SETTING_1", :description "Test setting - this only shows up in dev (1)", :default "Using $MB_TEST_SETTING_1"}
-  {:key "test-setting-2", :value "FANCY", :is_env_setting false, :env_name "MB_TEST_SETTING_2", :description "Test setting - this only shows up in dev (2)", :default "[Default Value]"}]
+  [{:key            "test-setting-1"
+    :value          nil
+    :is_env_setting true
+    :env_name       "MB_TEST_SETTING_1"
+    :description    "Test setting - this only shows up in dev (1)"
+    :default        "Using value of env var $MB_TEST_SETTING_1"}
+   {:key            "test-setting-2"
+    :value          "FANCY"
+    :is_env_setting false
+    :env_name       "MB_TEST_SETTING_2"
+    :description    "Test setting - this only shows up in dev (2)"
+    :default        "[Default Value]"}]
  (do (set-settings! nil "FANCY")
      (fetch-test-settings)))
 
@@ -44,3 +54,52 @@
 ;; ## Check non-superuser can't set a Setting
 (expect "You don't have permissions to do that."
   ((user->client :rasta) :put 403 "setting/test-setting-1" {:value "NICE!"}))
+
+
+;;; ----------------------------------------------- Sensitive Settings -----------------------------------------------
+
+;; Sensitive settings should always come back obfuscated
+
+;; GET /api/setting/:name should obfuscate sensitive settings
+(expect
+  "**********EF"
+  (do
+    (test-sensitive-setting "ABCDEF")
+    (fetch-setting :test-sensitive-setting)))
+
+;; GET /api/setting should obfuscate sensitive settings
+(expect
+  {:key            "test-sensitive-setting"
+   :value          "**********LM"
+   :is_env_setting false
+   :env_name       "MB_TEST_SENSITIVE_SETTING"
+   :description    "This is a sample sensitive Setting."
+   :default        nil}
+  (do
+    (test-sensitive-setting "GHIJKLM")
+    (some (fn [{setting-name :key, :as setting}]
+            (when (= setting-name "test-sensitive-setting")
+              setting))
+          ((user->client :crowberto) :get 200 "setting"))))
+
+;; Setting the Setting via an endpoint should still work as expected; the normal getter functions should *not*
+;; obfuscate sensitive Setting values -- that should be done by the API
+(expect
+  "123456"
+  (do
+    ((user->client :crowberto) :put 200 "setting/test-sensitive-setting" {:value "123456"})
+    (test-sensitive-setting)))
+
+;; Attempts to set the Setting to an obfuscated value should be ignored
+(expect
+  "123456"
+  (do
+    (test-sensitive-setting "123456")
+    ((user->client :crowberto) :put 200 "setting/test-sensitive-setting" {:value "**********56"})
+    (test-sensitive-setting)))
+
+(expect
+  (do
+    (test-sensitive-setting "123456")
+    ((user->client :crowberto) :put 200 "setting" {:test-sensitive-setting "**********56"})
+    (test-sensitive-setting)))

test/metabase/models/setting/cache_test.clj

+(ns metabase.models.setting.cache-test
+  (:require [clojure.core.memoize :as memoize]
+            [expectations :refer [expect]]
+            [honeysql.core :as hsql]
+            [metabase.db :as mdb]
+            [metabase.models
+             [setting :refer [Setting]]
+             [setting-test :as setting-test]]
+            [metabase.models.setting.cache :as cache]
+            [toucan.db :as db]))
+
+;;; --------------------------------------------- Cache Synchronization ----------------------------------------------
+
+(defn- clear-cache! []
+  (reset! @#'metabase.models.setting.cache/cache* nil))
+
+(defn- settings-last-updated-value-in-cache []
+  (get (cache/cache) cache/settings-last-updated-key))
+
+(defn- update-settings-last-updated-value-in-db!
+  "Simulate a different instance updating the value of `settings-last-updated` in the DB by updating its value without
+  updating our locally cached value.."
+  []
+  (db/update-where! Setting {:key cache/settings-last-updated-key}
+    :value (hsql/raw (case (mdb/db-type)
+                       ;; make it one second in the future so we don't end up getting an exact match when we try to test
+                       ;; to see if things update below
+                       :h2       "cast(dateadd('second', 1, current_timestamp) AS text)"
+                       :mysql    "cast((current_timestamp + interval 1 second) AS char)"
+                       :postgres "cast((current_timestamp + interval '1 second') AS text)"))))
+
+(defn- simulate-another-instance-updating-setting! [setting-name new-value]
+  (db/update-where! Setting {:key (name setting-name)} :value new-value)
+  (update-settings-last-updated-value-in-db!))
+
+(defn- flush-memoized-results-for-should-restore-cache!
+  "Remove any memoized results for `should-restore-cache?`, so we can test `restore-cache-if-needed!` works the way we'd
+  expect."
+  []
+  (memoize/memo-clear! @#'cache/should-restore-cache?))
+
+;; When I update a Setting, does it set/update `settings-last-updated`?
+(expect
+  (do
+    (setting-test/clear-settings-last-updated-value-in-db!)
+    (setting-test/toucan-name "Bird Can")
+    (string? (setting-test/settings-last-updated-value-in-db))))
+
+;; ...and is the value updated in the cache as well?
+(expect
+  (do
+    (clear-cache!)
+    (setting-test/toucan-name "Bird Can")
+    (string? (settings-last-updated-value-in-cache))))
+
+;; ...and if I update it again, will the value be updated?
+(expect
+  (do
+    (setting-test/clear-settings-last-updated-value-in-db!)
+    (setting-test/toucan-name "Bird Can")
+    (let [first-value (setting-test/settings-last-updated-value-in-db)]
+      ;; MySQL only has the resolution of one second on the timestamps here so we should wait that long to make sure
+      ;; the second-value actually ends up being greater than the first
+      (Thread/sleep 1200)
+      (setting-test/toucan-name "Bird Can")
+      (let [second-value (setting-test/settings-last-updated-value-in-db)]
+        ;; first & second values should be different, and first value should be "less than" the second value
+        (and (not= first-value second-value)
+             (neg? (compare first-value second-value)))))))
+
+;; If there is no cache, it should be considered out of date!`
+(expect
+  (do
+    (clear-cache!)
+    (#'cache/cache-out-of-date?)))
+
+;; But if I set a setting, it should cause the cache to be populated, and be up-to-date
+(expect
+  false
+  (do
+    (clear-cache!)
+    (setting-test/toucan-name "Reggae Toucan")
+    (#'cache/cache-out-of-date?)))
+
+;; If another instance updates a Setting, `cache-out-of-date?` should return `true` based on DB comparisons...
+;; be true!
+(expect
+  (do
+    (clear-cache!)
+    (setting-test/toucan-name "Reggae Toucan")
+    (simulate-another-instance-updating-setting! :setting-test/toucan-name "Bird Can")
+    (#'cache/cache-out-of-date?)))
+
+;; of course, `restore-cache-if-needed!` should use TTL memoization, and the cache should not get updated right away
+;; even if another instance updates a value...
+(expect
+  "Sam"
+  (do
+    (flush-memoized-results-for-should-restore-cache!)
+    (clear-cache!)
+    (setting-test/toucan-name "Sam")                 ; should restore cache, and put in {"setting-test/toucan-name" "Sam"}
+    ;; since we cleared the memoized value of `should-restore-cache?` call it again to make sure it gets set to
+    ;; `false` as it would IRL if we were calling it again from the same instance
+    (#'cache/should-restore-cache?)
+    ;; now have another instance change the value
+    (simulate-another-instance-updating-setting! :setting-test/toucan-name "Bird Can")
+    ;; our cache should not be updated yet because it's on a TTL
+    (setting-test/toucan-name)))
+
+;; ...and when it comes time to check our cache for updating (when calling `restore-cache-if-needed!`, it should get
+;; the updated value. (we're not actually going to wait a minute for the memoized values of `should-restore-cache?` to
+;; be invalidated, so we will manually flush the memoization cache to simulate it happening)
+(expect
+  "Bird Can"
+  (do
+    (clear-cache!)
+    (setting-test/toucan-name "Reggae Toucan")
+    (simulate-another-instance-updating-setting! :setting-test/toucan-name "Bird Can")
+    (flush-memoized-results-for-should-restore-cache!)
+    ;; calling `setting-test/toucan-name` will call `restore-cache-if-needed!`, which will in turn call `should-restore-cache?`.
+    ;; Since memoized value is no longer present, this should call `cache-out-of-date?`, which checks the DB; it will
+    ;; detect a cache out-of-date situation and flush the cache as appropriate, giving us the updated value when we
+    ;; call! :wow:
+    (setting-test/toucan-name)))

test/metabase/models/setting_test.clj

 (ns metabase.models.setting-test
-  (:require [clojure.core.memoize :as memoize]
-            [expectations :refer :all]
-            [honeysql.core :as hsql]
-            [metabase
-             [db :as mdb]
-             [util :as u]]
+  (:require [expectations :refer [expect]]
             [metabase.models.setting :as setting :refer [defsetting Setting]]
+            [metabase.models.setting.cache :as cache]
             [metabase.test.util :refer :all]
+            [metabase.util :as u]
             [metabase.util
              [encryption :as encryption]
              [encryption-test :as encryption-test]
@@ -151,12 +148,12 @@
 
 ;; #'setting/user-facing-info w/ no db value, env var value, no default value -- shouldn't leak env var value
 (expect
-  {:value nil, :is_env_setting true, :env_name "MB_TEST_SETTING_1", :default "Using $MB_TEST_SETTING_1"}
+  {:value nil, :is_env_setting true, :env_name "MB_TEST_SETTING_1", :default "Using value of env var $MB_TEST_SETTING_1"}
   (user-facing-info-with-db-and-env-var-values :test-setting-1 nil "TOUCANS"))
 
 ;; #'setting/user-facing-info w/ no db value, env var value, default value
 (expect
-  {:value nil,  :is_env_setting true, :env_name "MB_TEST_SETTING_2", :default "Using $MB_TEST_SETTING_2"}
+  {:value nil,  :is_env_setting true, :env_name "MB_TEST_SETTING_2", :default "Using value of env var $MB_TEST_SETTING_2"}
   (user-facing-info-with-db-and-env-var-values :test-setting-2 nil "TOUCANS"))
 
 ;; #'setting/user-facing-info w/ db value, no env var value, no default value
@@ -172,13 +169,13 @@
 ;; #'setting/user-facing-info w/ db value, env var value, no default value -- the DB value should take precedence over
 ;; #the env var
 (expect
-  {:value "WOW", :is_env_setting true, :env_name "MB_TEST_SETTING_1", :default "Using $MB_TEST_SETTING_1"}
+  {:value "WOW", :is_env_setting true, :env_name "MB_TEST_SETTING_1", :default "Using value of env var $MB_TEST_SETTING_1"}
   (user-facing-info-with-db-and-env-var-values :test-setting-1 "WOW" "ENV VAR"))
 
 ;; #'setting/user-facing-info w/ db value, env var value, default value -- env var should take precedence over default
 ;; #value
 (expect
-  {:value "WOW", :is_env_setting true, :env_name "MB_TEST_SETTING_2", :default "Using $MB_TEST_SETTING_2"}
+  {:value "WOW", :is_env_setting true, :env_name "MB_TEST_SETTING_2", :default "Using value of env var $MB_TEST_SETTING_2"}
   (user-facing-info-with-db-and-env-var-values :test-setting-2 "WOW" "ENV VAR"))
 
 ;; all
@@ -202,7 +199,7 @@
     :is_env_setting true
     :env_name       "MB_TEST_SETTING_1"
     :description    "Test setting - this only shows up in dev (1)"
-    :default        "Using $MB_TEST_SETTING_1"}
+    :default        "Using value of env var $MB_TEST_SETTING_1"}
    {:key            :test-setting-2
     :value          "S2"
     :is_env_setting false,
@@ -240,7 +237,7 @@
 
 ;; boolean settings shouldn't be obfuscated when set by env var
 (expect
-  {:value true, :is_env_setting true, :env_name "MB_TEST_BOOLEAN_SETTING", :default "Using $MB_TEST_BOOLEAN_SETTING"}
+  {:value true, :is_env_setting true, :env_name "MB_TEST_BOOLEAN_SETTING", :default "Using value of env var $MB_TEST_BOOLEAN_SETTING"}
   (user-facing-info-with-db-and-env-var-values :test-boolean-setting nil "true"))
 
 ;; env var values should be case-insensitive
@@ -291,7 +288,7 @@
 ;; make sure that if for some reason the cache gets out of sync it will reset so we can still set new settings values
 ;; (#4178)
 
-(setting/defsetting ^:private toucan-name
+(setting/defsetting toucan-name
   "Name for the Metabase Toucan mascot."
   :internal? true)
 
@@ -301,7 +298,7 @@
     ;; clear out any existing values of `toucan-name`
     (db/simple-delete! setting/Setting {:key "toucan-name"})
     ;; restore the cache
-    ((resolve 'metabase.models.setting/restore-cache-if-needed!))
+    (cache/restore-cache-if-needed!)
     ;; now set a value for the `toucan-name` setting the wrong way
     (db/insert! setting/Setting {:key "toucan-name", :value "Reggae"})
     ;; ok, now try to set the Setting the correct way
@@ -361,130 +358,13 @@
       (test-timestamp-setting)))
 
 
-;;; --------------------------------------------- Cache Synchronization ----------------------------------------------
-
-(def ^:private settings-last-updated-key @(resolve 'metabase.models.setting/settings-last-updated-key))
-
-(defn- clear-settings-last-updated-value-in-db! []
-  (db/simple-delete! Setting {:key settings-last-updated-key}))
-
-(defn- settings-last-updated-value-in-db []
-  (db/select-one-field :value Setting :key settings-last-updated-key))
-
-(defn- clear-cache! []
-  (reset! @(resolve 'metabase.models.setting/cache) nil))
-
-(defn- settings-last-updated-value-in-cache []
-  (get @@(resolve 'metabase.models.setting/cache) settings-last-updated-key))
-
-(defn- update-settings-last-updated-value-in-db!
-  "Simulate a different instance updating the value of `settings-last-updated` in the DB by updating its value without
-  updating our locally cached value.."
-  []
-  (db/update-where! Setting {:key settings-last-updated-key}
-    :value (hsql/raw (case (mdb/db-type)
-                       ;; make it one second in the future so we don't end up getting an exact match when we try to test
-                       ;; to see if things update below
-                       :h2       "cast(dateadd('second', 1, current_timestamp) AS text)"
-                       :mysql    "cast((current_timestamp + interval 1 second) AS char)"
-                       :postgres "cast((current_timestamp + interval '1 second') AS text)"))))
-
-(defn- simulate-another-instance-updating-setting! [setting-name new-value]
-  (db/update-where! Setting {:key (name setting-name)} :value new-value)
-  (update-settings-last-updated-value-in-db!))
-
-(defn- flush-memoized-results-for-should-restore-cache!
-  "Remove any memoized results for `should-restore-cache?`, so we can test `restore-cache-if-needed!` works the way we'd
-  expect."
-  []
-  (memoize/memo-clear! @(resolve 'metabase.models.setting/should-restore-cache?)))
-
-;; When I update a Setting, does it set/update `settings-last-updated`?
-(expect
-  (do
-    (clear-settings-last-updated-value-in-db!)
-    (toucan-name "Bird Can")
-    (string? (settings-last-updated-value-in-db))))
-
-;; ...and is the value updated in the cache as well?
-(expect
-  (do
-    (clear-cache!)
-    (toucan-name "Bird Can")
-    (string? (settings-last-updated-value-in-cache))))
-
-;; ...and if I update it again, will the value be updated?
-(expect
-  (do
-    (clear-settings-last-updated-value-in-db!)
-    (toucan-name "Bird Can")
-    (let [first-value (settings-last-updated-value-in-db)]
-      ;; MySQL only has the resolution of one second on the timestamps here so we should wait that long to make sure
-      ;; the second-value actually ends up being greater than the first
-      (Thread/sleep 1200)
-      (toucan-name "Bird Can")
-      (let [second-value (settings-last-updated-value-in-db)]
-        ;; first & second values should be different, and first value should be "less than" the second value
-        (and (not= first-value second-value)
-             (neg? (compare first-value second-value)))))))
-
-;; If there is no cache, it should be considered out of date!`
-(expect
-  (do
-    (clear-cache!)
-    (#'setting/cache-out-of-date?)))
-
-;; But if I set a setting, it should cause the cache to be populated, and be up-to-date
-(expect
-  false
-  (do
-    (clear-cache!)
-    (toucan-name "Reggae Toucan")
-    (#'setting/cache-out-of-date?)))
-
-;; If another instance updates a Setting, `cache-out-of-date?` should return `true` based on DB comparisons...
-;; be true!
-(expect
-  (do
-    (clear-cache!)
-    (toucan-name "Reggae Toucan")
-    (simulate-another-instance-updating-setting! :toucan-name "Bird Can")
-    (#'setting/cache-out-of-date?)))
-
-;; of course, `restore-cache-if-needed!` should use TTL memoization, and the cache should not get updated right away
-;; even if another instance updates a value...
-(expect
-  "Sam"
-  (do
-    (flush-memoized-results-for-should-restore-cache!)
-    (clear-cache!)
-    (toucan-name "Sam")                 ; should restore cache, and put in {"toucan-name" "Sam"}
-    ;; since we cleared the memoized value of `should-restore-cache?` call it again to make sure it gets set to
-    ;; `false` as it would IRL if we were calling it again from the same instance
-    (#'setting/should-restore-cache?)
-    ;; now have another instance change the value
-    (simulate-another-instance-updating-setting! :toucan-name "Bird Can")
-    ;; our cache should not be updated yet because it's on a TTL
-    (toucan-name)))
-
-;; ...and when it comes time to check our cache for updating (when calling `restore-cache-if-needed!`, it should get
-;; the updated value. (we're not actually going to wait a minute for the memoized values of `should-restore-cache?` to
-;; be invalidated, so we will manually flush the memoization cache to simulate it happening)
-(expect
-  "Bird Can"
-  (do
-    (clear-cache!)
-    (toucan-name "Reggae Toucan")
-    (simulate-another-instance-updating-setting! :toucan-name "Bird Can")
-    (flush-memoized-results-for-should-restore-cache!)
-    ;; calling `toucan-name` will call `restore-cache-if-needed!`, which will in turn call `should-restore-cache?`.
-    ;; Since memoized value is no longer present, this should call `cache-out-of-date?`, which checks the DB; it will
-    ;; detect a cache out-of-date situation and flush the cache as appropriate, giving us the updated value when we
-    ;; call! :wow:
-    (toucan-name)))
+;;; ----------------------------------------------- Uncached Settings ------------------------------------------------
 
+(defn clear-settings-last-updated-value-in-db! []
+  (db/simple-delete! Setting {:key cache/settings-last-updated-key}))
 
-;;; ----------------------------------------------- Uncached Settings ------------------------------------------------
+(defn settings-last-updated-value-in-db []
+  (db/select-one-field :value Setting :key cache/settings-last-updated-key))
 
 (defsetting ^:private uncached-setting
   "A test setting that should *not* be cached."
@@ -514,3 +394,25 @@
     (clear-settings-last-updated-value-in-db!)
     (uncached-setting "abcdef")
     (settings-last-updated-value-in-db)))
+
+
+;;; ----------------------------------------------- Sensitive Settings -----------------------------------------------
+
+(defsetting test-sensitive-setting
+  (tru "This is a sample sensitive Setting.")
+  :sensitive? true)
+
+;; `user-facing-value` should obfuscate sensitive settings
+(expect
+  "**********23"
+  (do
+    (test-sensitive-setting "ABC123")
+    (setting/user-facing-value "test-sensitive-setting")))
+
+;; Attempting to set a sensitive setting to an obfuscated value should be ignored -- it was probably done accidentally
+(expect
+  "123456"
+  (do
+    (test-sensitive-setting "123456")
+    (test-sensitive-setting "**********56")
+    (test-sensitive-setting)))