Merge branch 'master' into handle-settings-across-multiple-instances

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)}

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"

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)))))))

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))))

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]

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

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."

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."

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))))

src/metabase/util/encryption.clj

 (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)))

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))))))))
 
 

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]

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)

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)]))))

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))

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"))))

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")))))