diff --git a/src/metabase/api/pulse.clj b/src/metabase/api/pulse.clj
index a98340490eb007fa95de290169f8f94eb333f69a..e2bd363d97c7ef8c432af692515bb08fe5c9e6fb 100644
--- a/src/metabase/api/pulse.clj
+++ b/src/metabase/api/pulse.clj
@@ -46,7 +46,7 @@
"Create a new `Pulse`."
[:as {{:keys [name cards channels skip_if_empty collection_id collection_position]} :body}]
{name su/NonBlankString
- cards (su/non-empty [pulse/CardRef])
+ cards (su/non-empty [pulse/CoercibleToCardRef])
channels (su/non-empty [su/Map])
skip_if_empty (s/maybe s/Bool)
collection_id (s/maybe su/IntGreaterThanZero)
@@ -79,7 +79,7 @@
"Update a Pulse with `id`."
[id :as {{:keys [name cards channels skip_if_empty collection_id], :as pulse-updates} :body}]
{name (s/maybe su/NonBlankString)
- cards (s/maybe (su/non-empty [pulse/CardRef]))
+ cards (s/maybe (su/non-empty [pulse/CoercibleToCardRef]))
channels (s/maybe (su/non-empty [su/Map]))
skip_if_empty (s/maybe s/Bool)
collection_id (s/maybe su/IntGreaterThanZero)}
diff --git a/src/metabase/api/user.clj b/src/metabase/api/user.clj
index 62bdebac745030d887337415d1a645d567e1abc7..cee471889faf679b4d329c730ee2f6f1b1b30e80 100644
--- a/src/metabase/api/user.clj
+++ b/src/metabase/api/user.clj
@@ -86,6 +86,19 @@
(check-self-or-superuser id)
(api/check-404 (fetch-user :id id, :is_active true)))
+(defn- valid-email-update?
+ "This predicate tests whether or not the user is allowed to update the email address associated with this account."
+ [{:keys [google_auth ldap_auth email] :as foo } maybe-new-email]
+ (or
+ ;; Admin users can update
+ api/*is-superuser?*
+ ;; If the email address didn't change, let it through
+ (= email maybe-new-email)
+ ;; We should not allow a regular user to change their email address if they are a google/ldap user
+ (and
+ (not google_auth)
+ (not ldap_auth))))
+
(api/defendpoint PUT "/:id"
"Update an existing, active `User`."
[id :as {{:keys [email first_name last_name is_superuser login_attributes] :as body} :body}]
@@ -95,18 +108,20 @@
login_attributes (s/maybe user/LoginAttributes)}
(check-self-or-superuser id)
;; only allow updates if the specified account is active
- (api/check-404 (db/exists? User, :id id, :is_active true))
- ;; can't change email if it's already taken BY ANOTHER ACCOUNT
- (api/checkp (not (db/exists? User, :email email, :id [:not= id]))
- "email" (tru "Email address already associated to another user."))
- (api/check-500
- (db/update! User id
- (u/select-keys-when body
- :present (when api/*is-superuser?*
- #{:login_attributes})
- :non-nil (set (concat [:first_name :last_name :email]
- (when api/*is-superuser?*
- [:is_superuser]))))))
+ (api/let-404 [user-before-update (fetch-user :id id, :is_active true)]
+ ;; Google/LDAP non-admin users can't change their email to prevent account hijacking
+ (api/check-403 (valid-email-update? user-before-update email))
+ ;; can't change email if it's already taken BY ANOTHER ACCOUNT
+ (api/checkp (not (db/exists? User, :email email, :id [:not= id]))
+ "email" (tru "Email address already associated to another user."))
+ (api/check-500
+ (db/update! User id
+ (u/select-keys-when body
+ :present (when api/*is-superuser?*
+ #{:login_attributes})
+ :non-nil (set (concat [:first_name :last_name :email]
+ (when api/*is-superuser?*
+ [:is_superuser])))))))
(fetch-user :id id))
(api/defendpoint PUT "/:id/reactivate"
diff --git a/src/metabase/db/migrations.clj b/src/metabase/db/migrations.clj
index fd7d2a3f86539d5b7c4a414a5827cf9fab7c7077..1aeed2b266b17732a92614d5ed061272f0564256 100644
--- a/src/metabase/db/migrations.clj
+++ b/src/metabase/db/migrations.clj
@@ -6,7 +6,8 @@
CREATE TABLE IF NOT EXISTS ... -- Good
CREATE TABLE ... -- Bad"
- (:require [clojure.java.jdbc :as jdbc]
+ (:require [cemerick.friend.credentials :as creds]
+ [clojure.java.jdbc :as jdbc]
[clojure.string :as str]
[clojure.tools.logging :as log]
[metabase
@@ -33,7 +34,8 @@
[metabase.util.date :as du]
[toucan
[db :as db]
- [models :as models]]))
+ [models :as models]])
+ (:import java.util.UUID))
;;; # Migration Helpers
@@ -368,7 +370,6 @@
:active true}
:has_field_values "list"))
-
;; In v0.30.0 we switiched to making standard SQL the default for BigQuery; up until that point we had been using
;; BigQuery legacy SQL. For a while, we've supported standard SQL if you specified the case-insensitive `#standardSQL`
;; directive at the beginning of your query, and similarly allowed you to specify legacy SQL with the `#legacySQL`
@@ -391,3 +392,11 @@
;; and save the updated dataset_query map
(db/update! Card (u/get-id card-id)
:dataset_query (assoc-in query [:native :query] updated-sql)))))))))
+
+;; Before 0.30.0, we were storing the LDAP user's password in the `core_user` table (though it wasn't used). This
+;; migration clears those passwords and replaces them with a UUID. This is similar to a new account setup, or how we
+;; disable passwords for Google authenticated users
+(defmigration ^{:author "senior", :added "0.30.0"} clear-ldap-user-local-passwords
+ (db/transaction
+ (doseq [user (db/select [User :id :password_salt] :ldap_auth [:= true])]
+ (db/update! User (u/get-id user) :password (creds/hash-bcrypt (str (:password_salt user) (UUID/randomUUID)))))))
diff --git a/src/metabase/integrations/ldap.clj b/src/metabase/integrations/ldap.clj
index 40bd54bde0621e7619c2de89dcda2f7dc7b5c23e..27ba7f9cefbb92c18d28fd65708282b066d8e993 100644
--- a/src/metabase/integrations/ldap.clj
+++ b/src/metabase/integrations/ldap.clj
@@ -211,11 +211,8 @@
(let [user (or (db/select-one [User :id :last_login] :email email)
(user/create-new-ldap-auth-user! {:first_name first-name
:last_name last-name
- :email email
- :password password}))]
+ :email email}))]
(u/prog1 user
- (when password
- (user/set-password! (:id user) password))
(when (ldap-group-sync)
(let [special-ids #{(:id (group/admin)) (:id (group/all-users))}
current-ids (set (map :group_id (db/select ['PermissionsGroupMembership :group_id] :user_id (:id user))))
diff --git a/src/metabase/models/interface.clj b/src/metabase/models/interface.clj
index d8ebb1d23836f17fff6c8b34768c3b90a4be264c..45ed8e4ec11393e3c7bf40cd4232cf352e0bc7c4 100644
--- a/src/metabase/models/interface.clj
+++ b/src/metabase/models/interface.clj
@@ -68,6 +68,10 @@
:in encrypted-json-in
:out (comp cached-encrypted-json-out u/jdbc-clob->str))
+(models/add-type! :encrypted-text
+ :in encryption/maybe-encrypt
+ :out (comp encryption/maybe-decrypt u/jdbc-clob->str))
+
(defn compress
"Compress OBJ, returning a byte array."
[obj]
diff --git a/src/metabase/models/pulse.clj b/src/metabase/models/pulse.clj
index 771f55de4d9a0411c3ae95f93fd95fc04ea96a21..b3cefa148d77d14c7560b22e18e2716ff9888a34 100644
--- a/src/metabase/models/pulse.clj
+++ b/src/metabase/models/pulse.clj
@@ -14,7 +14,8 @@
functions for fetching a specific Pulse). At some point in the future, we can clean this namespace up and bring the
code in line with the rest of the codebase, but for the time being, it probably makes sense to follow the existing
patterns in this namespace rather than further confuse things."
- (:require [clojure.tools.logging :as log]
+ (:require [clojure.string :as str]
+ [clojure.tools.logging :as log]
[medley.core :as m]
[metabase
[events :as events]
@@ -27,6 +28,7 @@
[pulse-channel :as pulse-channel :refer [PulseChannel]]
[pulse-channel-recipient :refer [PulseChannelRecipient]]]
[metabase.util.schema :as su]
+ [puppetlabs.i18n.core :refer [tru]]
[schema.core :as s]
[toucan
[db :as db]
@@ -73,6 +75,42 @@
:can-write? (partial i/current-user-has-full-permissions? :write)
:perms-objects-set perms-objects-set})
+;;; ---------------------------------------------------- Schemas -----------------------------------------------------
+
+(def AlertConditions
+ "Schema for valid values of `:alert_condition` for Alerts."
+ (s/enum "rows" "goal"))
+
+(def CardRef
+ "Schema for the map we use to internally represent the fact that a Card is in a Notification and the details about its
+ presence there."
+ (su/with-api-error-message {:id su/IntGreaterThanZero
+ :include_csv s/Bool
+ :include_xls s/Bool}
+ (tru "value must be a map with the keys `{0}`, `{1}`, and `{2}`." "id" "include_csv" "include_xls")))
+
+(def HybridPulseCard
+ "This schema represents the cards that are included in a pulse. This is the data from the `PulseCard` and some
+ additional information used by the UI to display it from `Card`. This is a superset of `CardRef` and is coercible to
+ a `CardRef`"
+ (su/with-api-error-message
+ (merge (:schema CardRef)
+ {:name (s/maybe s/Str)
+ :description (s/maybe s/Str)
+ :display (s/maybe su/KeywordOrString)
+ :collection_id (s/maybe su/IntGreaterThanZero)})
+ (tru "value must be a map with the following keys `({0})`"
+ (str/join ", " ["collection_id" "description" "display" "id" "include_csv" "include_xls" "name"]))))
+
+(def CoercibleToCardRef
+ "Schema for functions accepting either a `HybridPulseCard` or `CardRef`."
+ (s/conditional
+ (fn check-hybrid-pulse-card [maybe-map]
+ (and (map? maybe-map)
+ (some #(contains? maybe-map %) [:name :description :display :collection_id])))
+ HybridPulseCard
+ :else
+ CardRef))
;;; --------------------------------------------------- Hydration ----------------------------------------------------
@@ -81,8 +119,7 @@
[notification-or-id]
(db/select PulseChannel, :pulse_id (u/get-id notification-or-id)))
-
-(defn ^:hydrate cards
+(s/defn ^:hydrate cards :- [HybridPulseCard]
"Return the Cards associated with this `notification`."
[notification-or-id]
(map (partial models/do-post-select Card)
@@ -96,22 +133,6 @@
[:= :c.archived false]]
:order-by [[:pc.position :asc]]})))
-
-;;; ---------------------------------------------------- Schemas -----------------------------------------------------
-
-(def AlertConditions
- "Schema for valid values of `:alert_condition` for Alerts."
- (s/enum "rows" "goal"))
-
-(def CardRef
- "Schema for the map we use to internally represent the fact that a Card is in a Notification and the details about its
- presence there."
- (su/with-api-error-message {:id su/IntGreaterThanZero
- :include_csv s/Bool
- :include_xls s/Bool}
- "value must be a map with the keys `id`, `include_csv`, and `include_xls`."))
-
-
;;; ---------------------------------------- Notification Fetching Helper Fns ----------------------------------------
(s/defn ^:private hydrate-notification :- PulseInstance
@@ -342,7 +363,7 @@
(s/optional-key :skip_if_empty) s/Bool
(s/optional-key :collection_id) (s/maybe su/IntGreaterThanZero)
(s/optional-key :collection_position) (s/maybe su/IntGreaterThanZero)
- (s/optional-key :cards) [CardRef]
+ (s/optional-key :cards) [CoercibleToCardRef]
(s/optional-key :channels) [su/Map]}]
(db/update! Pulse (u/get-id notification)
(u/select-keys-when notification
diff --git a/src/metabase/models/setting.clj b/src/metabase/models/setting.clj
index 8aaa48168e5323385b916e76fc064ab5bc6b681e..2b2881c358df285cf195586ae93f85bad3dfc74c 100644
--- a/src/metabase/models/setting.clj
+++ b/src/metabase/models/setting.clj
@@ -55,7 +55,7 @@
(u/strict-extend (class Setting)
models/IModel
(merge models/IModelDefaults
- {:types (constantly {:value :clob})}))
+ {:types (constantly {:value :encrypted-text})}))
(def ^:private Type
@@ -301,8 +301,19 @@
(defn- update-setting!
"Update an existing Setting. Used internally by `set-string!` below; do not use directly."
[setting-name new-value]
- (db/update-where! Setting {:key setting-name}
- :value new-value))
+ ;; This is indeed a very annoying way of having to do things, but `update-where!` doesn't call `pre-update` (in case
+ ;; it updates thousands of objects). So we need to manually trigger `pre-update` behavior by calling `do-pre-update`
+ ;; so that `value` can get encrypted if `MB_ENCRYPTION_SECRET_KEY` is in use. Then take that possibly-encrypted
+ ;; value and pass that into `update-where!`.
+ (let [maybe-encrypted-new-value (if (= setting-name settings-last-updated-key)
+ ;; one more caveat: do not encrypt the `settings-last-updated` setting,
+ ;; since we use it directly in queries for determining whether the cache
+ ;; is out of date.
+ new-value
+ ;; all other Settings are subject to encryption
+ (:value (models/do-pre-update Setting {:value new-value})))]
+ (db/update-where! Setting {:key setting-name}
+ :value maybe-encrypted-new-value)))
(defn- set-new-setting!
"Insert a new row for a Setting. Used internally by `set-string!` below; do not use directly."
diff --git a/src/metabase/models/user.clj b/src/metabase/models/user.clj
index c7b930e5d68d239956c1534e91b94e81f91328c7..858558ab24621cb0c2054f02718af6fe5428580f 100644
--- a/src/metabase/models/user.clj
+++ b/src/metabase/models/user.clj
@@ -191,7 +191,10 @@
"Convenience for creating a new user via LDAP. This account is considered active immediately; thus all active admins
will recieve an email right away."
[new-user :- NewUser]
- (insert-new-user! (assoc new-user :ldap_auth true)))
+ (insert-new-user! (-> new-user
+ ;; We should not store LDAP passwords
+ (dissoc :password)
+ (assoc :ldap_auth true))))
(defn set-password!
"Updates the stored password for a specified `User` by hashing the password with a random salt."
diff --git a/src/metabase/util.clj b/src/metabase/util.clj
index 48984435a1e469201c5a9e55dc061308837a615b..34e204693edada38097b50be687eaee91bf1e061 100644
--- a/src/metabase/util.clj
+++ b/src/metabase/util.clj
@@ -473,7 +473,7 @@
(select-nested-keys v nested-keys))})))
(defn base64-string?
- "Is S a Base-64 encoded string?"
+ "Is `s` a Base-64 encoded string?"
^Boolean [s]
(boolean (when (string? s)
(re-find #"^[0-9A-Za-z/+]+=*$" s))))
diff --git a/src/metabase/util/encryption.clj b/src/metabase/util/encryption.clj
index fd920c98e479c8e84c9a472acf04d4d3cc8d0d9b..7f541c43dc42fa9ab07c5fe94841c0a3818232c9 100644
--- a/src/metabase/util/encryption.clj
+++ b/src/metabase/util/encryption.clj
@@ -1,5 +1,6 @@
(ns metabase.util.encryption
- "Utility functions for encrypting and decrypting strings using AES256 CBC + HMAC SHA512 and the `MB_ENCRYPTION_SECRET_KEY` env var."
+ "Utility functions for encrypting and decrypting strings using AES256 CBC + HMAC SHA512 and the
+ `MB_ENCRYPTION_SECRET_KEY` env var."
(:require [buddy.core
[codecs :as codecs]
[crypto :as crypto]
@@ -8,54 +9,62 @@
[clojure.tools.logging :as log]
[environ.core :as env]
[metabase.util :as u]
+ [puppetlabs.i18n.core :refer [trs]]
[ring.util.codec :as codec]))
(defn secret-key->hash
- "Generate a 64-byte byte array hash of SECRET-KEY using 100,000 iterations of PBKDF2+SHA512."
+ "Generate a 64-byte byte array hash of `secret-key` using 100,000 iterations of PBKDF2+SHA512."
^bytes [^String secret-key]
(kdf/get-bytes (kdf/engine {:alg :pbkdf2+sha512
:key secret-key
:iterations 100000}) ; 100,000 iterations takes about ~160ms on my laptop
64))
-;; apperently if you're not tagging in an arglist, `^bytes` will set the `:tag` metadata to `clojure.core/bytes` (ick) so you have to do `^{:tag 'bytes}` instead
+;; apperently if you're not tagging in an arglist, `^bytes` will set the `:tag` metadata to `clojure.core/bytes` (ick)
+;; so you have to do `^{:tag 'bytes}` instead
(defonce ^:private ^{:tag 'bytes} default-secret-key
(when-let [secret-key (env/env :mb-encryption-secret-key)]
(when (seq secret-key)
(assert (>= (count secret-key) 16)
- "MB_ENCRYPTION_SECRET_KEY must be at least 16 characters.")
+ (trs "MB_ENCRYPTION_SECRET_KEY must be at least 16 characters."))
(secret-key->hash secret-key))))
;; log a nice message letting people know whether DB details encryption is enabled
(log/info
- (format "DB details encryption is %s for this Metabase instance. %s"
- (if default-secret-key "ENABLED" "DISABLED")
- (u/emoji (if default-secret-key "ðŸ”" "🔓")))
- "\nSee"
- "http://www.metabase.com/docs/latest/operations-guide/start.html#encrypting-your-database-connection-details-at-rest"
- "for more information.")
+ (if default-secret-key
+ (trs "Saved credentials encryption is ENABLED for this Metabase instance.")
+ (trs "Saved credentials encryption is DISABLED for this Metabase instance."))
+ (u/emoji (if default-secret-key "ðŸ”" "🔓"))
+ (trs "\nFor more information, see")
+ "https://www.metabase.com/docs/latest/operations-guide/start.html#encrypting-your-database-connection-details-at-rest")
(defn encrypt
- "Encrypt string S as hex bytes using a SECRET-KEY (a 64-byte byte array), by default the hashed value of `MB_ENCRYPTION_SECRET_KEY`."
+ "Encrypt string `s` as hex bytes using a `secret-key` (a 64-byte byte array), by default the hashed value of
+ `MB_ENCRYPTION_SECRET_KEY`."
(^String [^String s]
(encrypt default-secret-key s))
(^String [^String secret-key, ^String s]
(let [initialization-vector (nonce/random-bytes 16)]
- (codec/base64-encode (byte-array (concat initialization-vector
- (crypto/encrypt (codecs/to-bytes s) secret-key initialization-vector {:algorithm :aes256-cbc-hmac-sha512})))))))
+ (codec/base64-encode
+ (byte-array
+ (concat initialization-vector
+ (crypto/encrypt (codecs/to-bytes s) secret-key initialization-vector
+ {:algorithm :aes256-cbc-hmac-sha512})))))))
(defn decrypt
- "Decrypt string S using a SECRET-KEY (a 64-byte byte array), by default the hashed value of `MB_ENCRYPTION_SECRET_KEY`."
+ "Decrypt string `s` using a `secret-key` (a 64-byte byte array), by default the hashed value of
+ `MB_ENCRYPTION_SECRET_KEY`."
(^String [^String s]
(decrypt default-secret-key s))
(^String [secret-key, ^String s]
(let [bytes (codec/base64-decode s)
[initialization-vector message] (split-at 16 bytes)]
- (codecs/bytes->str (crypto/decrypt (byte-array message) secret-key (byte-array initialization-vector) {:algorithm :aes256-cbc-hmac-sha512})))))
+ (codecs/bytes->str (crypto/decrypt (byte-array message) secret-key (byte-array initialization-vector)
+ {:algorithm :aes256-cbc-hmac-sha512})))))
(defn maybe-encrypt
- "If `MB_ENCRYPTION_SECRET_KEY` is set, return an encrypted version of S; otherwise return S as-is."
+ "If `MB_ENCRYPTION_SECRET_KEY` is set, return an encrypted version of `s`; otherwise return `s` as-is."
(^String [^String s]
(maybe-encrypt default-secret-key s))
(^String [secret-key, ^String s]
@@ -65,7 +74,7 @@
s)))
(defn maybe-decrypt
- "If `MB_ENCRYPTION_SECRET_KEY` is set and S is encrypted, decrypt S; otherwise return S as-is."
+ "If `MB_ENCRYPTION_SECRET_KEY` is set and `s` is encrypted, decrypt `s`; otherwise return `s` as-is."
(^String [^String s]
(maybe-decrypt default-secret-key s))
(^String [secret-key, ^String s]
@@ -75,7 +84,10 @@
(catch Throwable e
(if (u/base64-string? s)
;; if we can't decrypt `s`, but it *is* encrypted, log an error message and return `nil`
- (log/error "Cannot decrypt encrypted details. Have you changed or forgot to set MB_ENCRYPTION_SECRET_KEY?" (.getMessage e))
+ (log/error
+ (trs "Cannot decrypt encrypted credentials. Have you changed or forgot to set MB_ENCRYPTION_SECRET_KEY?")
+ (.getMessage e)
+ (u/pprint-to-str (u/filtered-stacktrace e)))
;; otherwise return S without decrypting. It's probably not decrypted in the first place
s)))
s)))
diff --git a/src/metabase/util/schema.clj b/src/metabase/util/schema.clj
index 75346c8b325c49751db8ca0be49e2a830dd22bab..195f16fe49953549ae310f8d6f2f1bb796a4ca56 100644
--- a/src/metabase/util/schema.clj
+++ b/src/metabase/util/schema.clj
@@ -50,6 +50,13 @@
(instance? java.util.regex.Pattern existing-schema) (tru "value must be a string that matches the regex `{0}`."
existing-schema)))
+(declare api-error-message)
+
+(defn- create-cond-schema-message [child-schemas]
+ (str (tru "value must satisfy one of the following requirements: ")
+ (str/join " " (for [[i child-schema] (m/indexed child-schemas)]
+ (format "%d) %s" (inc i) (api-error-message child-schema))))))
+
(defn api-error-message
"Extract the API error messages attached to a schema, if any.
This functionality is fairly sophisticated:
@@ -74,13 +81,16 @@
;; 1) value must be a boolean.
;; 2) value must be a valid boolean string ('true' or 'false').
(when (instance? schema.core.CondPre schema)
- (str (tru "value must satisfy one of the following requirements: ")
- (str/join " " (for [[i child-schema] (m/indexed (:schemas schema))]
- (format "%d) %s" (inc i) (api-error-message child-schema))))))
+ (create-cond-schema-message (:schemas schema)))
+
+ ;; For conditional schemas we'll generate a string similar to `cond-pre` above
+ (when (instance? schema.core.ConditionalSchema schema)
+ (create-cond-schema-message (map second (:preds-and-schemas schema))))
+
;; do the same for sequences of a schema
(when (vector? schema)
(str (tru "value must be an array.") (when (= (count schema) 1)
- (when-let [message (:api-error-message (first schema))]
+ (when-let [message (api-error-message (first schema))]
(str " " (tru "Each {0}" message))))))))
diff --git a/test/metabase/api/pulse_test.clj b/test/metabase/api/pulse_test.clj
index a2e40472b434c29a7f5439427c90efecea854747..87475c899b2f84d9c337389e489bd40ef441a7c8 100644
--- a/test/metabase/api/pulse_test.clj
+++ b/test/metabase/api/pulse_test.clj
@@ -103,24 +103,28 @@
;;; | POST /api/pulse |
;;; +----------------------------------------------------------------------------------------------------------------+
+(def ^:private default-post-card-ref-validation-error
+ {:errors
+ {:cards (str "value must be an array. Each value must satisfy one of the following requirements: "
+ "1) value must be a map with the following keys "
+ "`(collection_id, description, display, id, include_csv, include_xls, name)` "
+ "2) value must be a map with the keys `id`, `include_csv`, and `include_xls`. The array cannot be empty.")}})
+
(expect
{:errors {:name "value must be a non-blank string."}}
((user->client :rasta) :post 400 "pulse" {}))
(expect
- {:errors {:cards (str "value must be an array. Each value must be a map with the keys `id`, `include_csv`, and "
- "`include_xls`. The array cannot be empty.")}}
+ default-post-card-ref-validation-error
((user->client :rasta) :post 400 "pulse" {:name "abc"}))
(expect
- {:errors {:cards (str "value must be an array. Each value must be a map with the keys `id`, `include_csv`, and "
- "`include_xls`. The array cannot be empty.")}}
+ default-post-card-ref-validation-error
((user->client :rasta) :post 400 "pulse" {:name "abc"
:cards "foobar"}))
(expect
- {:errors {:cards (str "value must be an array. Each value must be a map with the keys `id`, `include_csv`, and "
- "`include_xls`. The array cannot be empty.")}}
+ default-post-card-ref-validation-error
((user->client :rasta) :post 400 "pulse" {:name "abc"
:cards ["abc"]}))
@@ -197,6 +201,42 @@
pulse-response
(update :channels remove-extra-channels-fields))))))
+;; Create a pulse with a HybridPulseCard and a CardRef, PUT accepts this format, we should make sure POST does as well
+(tt/expect-with-temp [Card [card-1]
+ Card [card-2 {:name "The card"
+ :description "Info"
+ :display :table}]]
+ (merge
+ pulse-defaults
+ {:name "A Pulse"
+ :creator_id (user->id :rasta)
+ :creator (user-details (fetch-user :rasta))
+ :cards (for [card [card-1 card-2]]
+ (assoc (pulse-card-details card)
+ :collection_id true))
+ :channels [(merge pulse-channel-defaults
+ {:channel_type "email"
+ :schedule_type "daily"
+ :schedule_hour 12
+ :recipients []})]
+ :collection_id true})
+ (card-api-test/with-cards-in-readable-collection [card-1 card-2]
+ (tt/with-temp Collection [collection]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ (tu/with-model-cleanup [Pulse]
+ (-> ((user->client :rasta) :post 200 "pulse" {:name "A Pulse"
+ :collection_id (u/get-id collection)
+ :cards [{:id (u/get-id card-1)
+ :include_csv false
+ :include_xls false}
+ (-> card-2
+ (select-keys [:id :name :description :display :collection_id])
+ (assoc :include_csv false, :include_xls false))]
+ :channels [daily-email-channel]
+ :skip_if_empty false})
+ pulse-response
+ (update :channels remove-extra-channels-fields))))))
+
;; Create a pulse with a csv and xls
(tt/expect-with-temp [Card [card-1]
Card [card-2]]
@@ -273,23 +313,28 @@
;;; | PUT /api/pulse/:id |
;;; +----------------------------------------------------------------------------------------------------------------+
+(def ^:private default-put-card-ref-validation-error
+ {:errors
+ {:cards (str "value may be nil, or if non-nil, value must be an array. "
+ "Each value must satisfy one of the following requirements: "
+ "1) value must be a map with the following keys "
+ "`(collection_id, description, display, id, include_csv, include_xls, name)` "
+ "2) value must be a map with the keys `id`, `include_csv`, and `include_xls`. The array cannot be empty.")}})
+
(expect
{:errors {:name "value may be nil, or if non-nil, value must be a non-blank string."}}
((user->client :rasta) :put 400 "pulse/1" {:name 123}))
(expect
- {:errors {:cards (str "value may be nil, or if non-nil, value must be an array. Each value must be a map with the "
- "keys `id`, `include_csv`, and `include_xls`. The array cannot be empty.")}}
+ default-put-card-ref-validation-error
((user->client :rasta) :put 400 "pulse/1" {:cards 123}))
(expect
- {:errors {:cards (str "value may be nil, or if non-nil, value must be an array. Each value must be a map with the "
- "keys `id`, `include_csv`, and `include_xls`. The array cannot be empty.")}}
+ default-put-card-ref-validation-error
((user->client :rasta) :put 400 "pulse/1" {:cards "foobar"}))
(expect
- {:errors {:cards (str "value may be nil, or if non-nil, value must be an array. Each value must be a map with the "
- "keys `id`, `include_csv`, and `include_xls`. The array cannot be empty.")}}
+ default-put-card-ref-validation-error
((user->client :rasta) :put 400 "pulse/1" {:cards ["abc"]}))
(expect
@@ -342,6 +387,35 @@
pulse-response
(update :channels remove-extra-channels-fields)))))
+;; Can we add a card to an existing pulse that has a card? Specifically this will include a HybridPulseCard (the
+;; original card associated with the pulse) and a CardRef (the new card)
+(tt/expect-with-temp [Pulse [pulse {:name "Original Pulse Name"}]
+ Card [card-1 {:name "Test"
+ :description "Just Testing"}]
+ PulseCard [_ {:card_id (u/get-id card-1)
+ :pulse_id (u/get-id pulse)}]
+ Card [card-2 {:name "Test2"
+ :description "Just Testing2"}]]
+ (merge
+ pulse-defaults
+ {:name "Original Pulse Name"
+ :creator_id (user->id :rasta)
+ :creator (user-details (fetch-user :rasta))
+ :cards (mapv (comp #(assoc % :collection_id true) pulse-card-details) [card-1 card-2])
+ :channels []
+ :collection_id true})
+ (with-pulses-in-writeable-collection [pulse]
+ (card-api-test/with-cards-in-readable-collection [card-1 card-2]
+ ;; The FE will include the original HybridPulseCard, similar to how the API returns the card via GET
+ (let [pulse-cards (:cards ((user->client :rasta) :get 200 (format "pulse/%d" (u/get-id pulse))))]
+ (-> ((user->client :rasta) :put 200 (format "pulse/%d" (u/get-id pulse))
+ {:cards (concat pulse-cards
+ [{:id (u/get-id card-2)
+ :include_csv false
+ :include_xls false}])})
+ pulse-response
+ (update :channels remove-extra-channels-fields))))))
+
;; Can we update *just* the Collection ID of a Pulse?
(expect
(tt/with-temp* [Pulse [pulse]
diff --git a/test/metabase/api/user_test.clj b/test/metabase/api/user_test.clj
index 91db0f9ad2dfdb01b10cc6167ea18a261ab62ddb..d2e059683bf5f43ee1a4de3f6674288459eccefc 100644
--- a/test/metabase/api/user_test.clj
+++ b/test/metabase/api/user_test.clj
@@ -322,6 +322,28 @@
((test-users/user->client :crowberto) :put 404 (str "user/" (test-users/user->id :trashbird))
{:email "toucan@metabase.com"}))
+;; Google auth users shouldn't be able to change their own password as we get that from Google
+(expect
+ "You don't have permissions to do that."
+ (tt/with-temp User [user {:email "anemail@metabase.com"
+ :password "def123"
+ :google_auth true}]
+ (let [creds {:username "anemail@metabase.com"
+ :password "def123"}]
+ (http/client creds :put 403 (format "user/%d" (u/get-id user))
+ {:email "adifferentemail@metabase.com"}))))
+
+;; Similar to Google auth accounts, we should not allow LDAP users to change their own email address as we get that
+;; from the LDAP server
+(expect
+ "You don't have permissions to do that."
+ (tt/with-temp User [user {:email "anemail@metabase.com"
+ :password "def123"
+ :ldap_auth true}]
+ (let [creds {:username "anemail@metabase.com"
+ :password "def123"}]
+ (http/client creds :put 403 (format "user/%d" (u/get-id user))
+ {:email "adifferentemail@metabase.com"}))))
;; ## PUT /api/user/:id/password
;; Test that a User can change their password (superuser and non-superuser)
diff --git a/test/metabase/db/migrations_test.clj b/test/metabase/db/migrations_test.clj
index 70dba4b7eda2e874b22d2cfe673f6988f144a2b4..3e4f70ad67184dce85b0c6e55919d878966d32b7 100644
--- a/test/metabase/db/migrations_test.clj
+++ b/test/metabase/db/migrations_test.clj
@@ -6,8 +6,10 @@
[metabase.db.migrations :as migrations]
[metabase.models
[card :refer [Card]]
- [database :refer [Database]]]
+ [database :refer [Database]]
+ [user :refer [User]]]
[metabase.util :as u]
+ [metabase.util.password :as upass]
[toucan.db :as db]
[toucan.util.test :as tt]))
@@ -38,3 +40,23 @@
(#'migrations/add-legacy-sql-directive-to-bigquery-sql-cards)
(->> (db/select-field->field :name :dataset_query Card :id [:in (map u/get-id [card-1 card-2])])
(m/map-vals #(update % :database integer?)))))
+
+;; Test clearing of LDAP user local passwords
+(expect
+ [false true]
+ (do
+ (tt/with-temp* [User [ldap-user {:email "ldapuser@metabase.com"
+ :password "something secret"
+ :ldap_auth true}]
+ User [user {:email "notanldapuser@metabase.com"
+ :password "no change"}]]
+ (#'migrations/clear-ldap-user-local-passwords)
+ (let [get-pass-and-salt #(db/select-one [User :password :password_salt] :id (u/get-id %))
+ {ldap-pass :password,
+ ldap-salt :password_salt} (get-pass-and-salt ldap-user)
+ {user-pass :password,
+ user-salt :password_salt} (get-pass-and-salt user)]
+ ;; The LDAP user password should be no good now that it's been cleared and replaced
+ [(upass/verify-password "something secret" ldap-salt ldap-pass)
+ ;; There should be no change for a non ldap user
+ (upass/verify-password "no change" user-salt user-pass)]))))
diff --git a/test/metabase/models/setting_test.clj b/test/metabase/models/setting_test.clj
index 3c95c8c3a0bbf5eac807eb4bc88bfda21b7ef2c4..ad3c1012ca96b901beb03bbab28a9bdeda63430f 100644
--- a/test/metabase/models/setting_test.clj
+++ b/test/metabase/models/setting_test.clj
@@ -4,7 +4,14 @@
[honeysql.core :as hsql]
[metabase.models.setting :as setting :refer [defsetting Setting]]
[metabase.test.util :refer :all]
+<<<<<<< HEAD
[metabase.util.honeysql-extensions :as hx]
+=======
+ [metabase.util :as u]
+ [metabase.util
+ [encryption :as encryption]
+ [encryption-test :as encryption-test]]
+>>>>>>> master
[puppetlabs.i18n.core :refer [tru]]
[toucan.db :as db]))
@@ -170,7 +177,12 @@
;; all
(expect
- {:key :test-setting-2, :value "TOUCANS", :description "Test setting - this only shows up in dev (2)", :is_env_setting false, :env_name "MB_TEST_SETTING_2", :default "[Default Value]"}
+ {:key :test-setting-2
+ :value "TOUCANS"
+ :description "Test setting - this only shows up in dev (2)"
+ :is_env_setting false
+ :env_name "MB_TEST_SETTING_2"
+ :default "[Default Value]"}
(do (set-settings! nil "TOUCANS")
(some (fn [setting]
(when (re-find #"^test-setting-2$" (name (:key setting)))
@@ -179,8 +191,18 @@
;; all
(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 "S2", :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 $MB_TEST_SETTING_1"}
+ {:key :test-setting-2
+ :value "S2"
+ :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 "S2")
(for [setting (setting/all)
:when (re-find #"^test-setting-\d$" (name (:key setting)))]
@@ -274,6 +296,35 @@
(toucan-name)))
+;;; ----------------------------------------------- Encrypted Settings -----------------------------------------------
+
+(defn- actual-value-in-db [setting-key]
+ (-> (db/query {:select [:value]
+ :from [:setting]
+ :where [:= :key (name setting-key)]})
+ first :value u/jdbc-clob->str))
+
+;; If encryption is *enabled*, make sure Settings get saved as encrypted!
+(expect
+ (encryption-test/with-secret-key "ABCDEFGH12345678"
+ (toucan-name "Sad Can")
+ (u/base64-string? (actual-value-in-db :toucan-name))))
+
+;; make sure it can be decrypted as well...
+(expect
+ "Sad Can"
+ (encryption-test/with-secret-key "12345678ABCDEFGH"
+ (toucan-name "Sad Can")
+ (encryption/decrypt (actual-value-in-db :toucan-name))))
+
+;; But if encryption is not enabled, of course Settings shouldn't get saved as encrypted.
+(expect
+ "Sad Can"
+ (encryption-test/with-secret-key nil
+ (toucan-name "Sad Can")
+ (actual-value-in-db :toucan-name)))
+
+
;;; --------------------------------------------- Cache Synchronization ----------------------------------------------
(def ^:private settings-last-updated-key @(resolve 'metabase.models.setting/settings-last-updated-key))
diff --git a/test/metabase/models/user_test.clj b/test/metabase/models/user_test.clj
index 8570ae119f17fe50ed543ae178077d0086a706df..26e67a8e4841172634c2c88c03e8f207d2dc93b8 100644
--- a/test/metabase/models/user_test.clj
+++ b/test/metabase/models/user_test.clj
@@ -12,6 +12,7 @@
[user :as user :refer [User]]]
[metabase.test.data.users :as test-users :refer [user->id]]
[metabase.test.util :as tu]
+ [metabase.util.password :as upass]
[toucan.db :as db]
[toucan.util.test :as tt]))
@@ -162,3 +163,17 @@
(test-users/delete-temp-users!)
(tt/with-temp User [_ {:is_superuser true, :is_active false}]
(invite-user-accept-and-check-inboxes! :google-auth? true))))
+
+;; LDAP users should not persist their passwords. Check that if somehow we get passed an LDAP user password, it gets
+;; swapped with something random
+(expect
+ false
+ (try
+ (user/create-new-ldap-auth-user! {:email "ldaptest@metabase.com"
+ :first_name "Test"
+ :last_name "SomeLdapStuff"
+ :password "should be removed"})
+ (let [{:keys [password password_salt]} (db/select-one [User :password :password_salt] :email "ldaptest@metabase.com")]
+ (upass/verify-password "should be removed" password_salt password))
+ (finally
+ (db/delete! User :email "ldaptest@metabase.com"))))
diff --git a/test/metabase/util/encryption_test.clj b/test/metabase/util/encryption_test.clj
index a858a6b1243f5060b8291ecc9ac9000e685d2155..026a6388ff4f6d8397b7652ea098ab706eddb507 100644
--- a/test/metabase/util/encryption_test.clj
+++ b/test/metabase/util/encryption_test.clj
@@ -5,6 +5,18 @@
[metabase.test.util :as tu]
[metabase.util.encryption :as encryption]))
+(defn do-with-secret-key [^String secret-key, f]
+ (with-redefs [encryption/default-secret-key (when (seq secret-key)
+ (encryption/secret-key->hash secret-key))]
+ (f)))
+
+(defmacro with-secret-key
+ "Run `body` with the encryption secret key temporarily bound to `secret-key`. Useful for testing how functions behave
+ with and without encryption disabled."
+ {:style/indent 1}
+ [^String secret-key, & body]
+ `(do-with-secret-key ~secret-key (fn [] ~@body)))
+
(def ^:private secret (encryption/secret-key->hash "Orw0AAyzkO/kPTLJRxiyKoBHXa/d6ZcO+p+gpZO/wSQ="))
(def ^:private secret-2 (encryption/secret-key->hash "0B9cD6++AME+A7/oR7Y2xvPRHX3cHA2z7w+LbObd/9Y="))
@@ -49,7 +61,7 @@
(expect
(some (fn [[_ _ message]]
- (str/includes? message (str "Cannot decrypt encrypted details. Have you changed or forgot to set "
+ (str/includes? message (str "Cannot decrypt encrypted credentials. Have you changed or forgot to set "
"MB_ENCRYPTION_SECRET_KEY? Message seems corrupt or manipulated.")))
(tu/with-log-messages
(encryption/maybe-decrypt secret-2 (encryption/encrypt secret "WOW")))))