From 45423ccaa2a1ffb6d436f285fe479e0c925fc4ca Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Wed, 26 Oct 2022 16:40:58 -0700
Subject: [PATCH] Config from file: fix User upsert not hashing password
(#26105)
* Config from file: fix User upsert not hashing password
* Move metabase.config.file code => enterprise directory
* Support setting Settings in config.yml
* Setting name has to be a keyword since that's how YAML gets parsed
* Test fix :wrench:
* Fix Kondo error
* Fix Kondo errors
* Remove unused namespace
---
.../config_from_file/users_test.clj | 38 ++++++---
src/metabase/models/user.clj | 81 ++++++++++---------
test/metabase/integrations/google_test.clj | 17 ++--
test/metabase/models/user_test.clj | 30 +++++++
4 files changed, 109 insertions(+), 57 deletions(-)
diff --git a/enterprise/backend/test/metabase_enterprise/config_from_file/users_test.clj b/enterprise/backend/test/metabase_enterprise/config_from_file/users_test.clj
index 60fc6c8c76b..44524094ef1 100644
--- a/enterprise/backend/test/metabase_enterprise/config_from_file/users_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/config_from_file/users_test.clj
@@ -25,19 +25,31 @@
(is (= 1
(db/count User :email "cam+config-file-test@metabase.com"))))
(testing "upsert if User already exists"
- (binding [config-from-file/*config* {:version 1
- :config {:users [{:first_name "Cam"
- :last_name "Saul"
- :email "cam+config-file-test@metabase.com"
- :password "2cans"}]}}]
- (is (= :ok
- (config-from-file/initialize!)))
- (is (= 1
- (db/count User :email "cam+config-file-test@metabase.com")))
- (is (partial= {:first_name "Cam"
- :last_name "Saul"
- :email "cam+config-file-test@metabase.com"}
- (db/select-one User :email "cam+config-file-test@metabase.com"))))))
+ (let [hashed-password (fn [] (db/select-one-field :password User :email "cam+config-file-test@metabase.com"))
+ salt (fn [] (db/select-one-field :password_salt User :email "cam+config-file-test@metabase.com"))
+ original-hashed-password (hashed-password)]
+ (binding [config-from-file/*config* {:version 1
+ :config {:users [{:first_name "Cam"
+ :last_name "Saul"
+ :email "cam+config-file-test@metabase.com"
+ :password "2cans"}]}}]
+ (is (= :ok
+ (config-from-file/initialize!)))
+ (is (= 1
+ (db/count User :email "cam+config-file-test@metabase.com")))
+ (is (partial= {:first_name "Cam"
+ :last_name "Saul"
+ :email "cam+config-file-test@metabase.com"}
+ (db/select-one User :email "cam+config-file-test@metabase.com")))
+ (testing "Password should be hashed, but it should be a NEW HASH"
+ (let [new-hashed-password (hashed-password)]
+ (is (not= original-hashed-password
+ new-hashed-password))
+ (testing "Password should not be saved as plaintext"
+ (is (not= "2cans"
+ new-hashed-password)))
+ (testing "Password should work correctly"
+ (is (u.password/verify-password "2cans" (salt) new-hashed-password)))))))))
(finally
(db/delete! User :email "cam+config-file-test@metabase.com"))))
diff --git a/src/metabase/models/user.clj b/src/metabase/models/user.clj
index 1c7f85572ac..47aedf2161a 100644
--- a/src/metabase/models/user.clj
+++ b/src/metabase/models/user.clj
@@ -28,34 +28,41 @@
(models/defmodel User :core_user)
+(def ^:private insert-default-values
+ {:date_joined :%now
+ :last_login nil
+ :is_active true
+ :is_superuser false})
+
+(defn- hashed-password-values
+ "When User `:password` is specified for an `INSERT` or `UPDATE`, add a new `:password_salt`, and hash the password."
+ [{:keys [password], :as user}]
+ (when password
+ (assert (not (:password_salt user))
+ ;; this is dev-facing so it doesn't need to be i18n'ed
+ "Don't try to pass an encrypted password to insert! or update!. Password encryption is handled by pre- methods.")
+ (let [salt (str (random-uuid))]
+ {:password_salt salt
+ :password (u.password/hash-bcrypt (str salt password))})))
+
(defn- pre-insert [{:keys [email password reset_token locale], :as user}]
;; these assertions aren't meant to be user-facing, the API endpoints should be validation these as well.
(assert (u/email? email))
(assert ((every-pred string? (complement str/blank?)) password))
- (assert (not (:password_salt user))
- "Don't try to pass an encrypted password to (insert! User). Password encryption is handled by pre-insert.")
(when locale
(assert (i18n/available-locale? locale)))
- (let [salt (str (UUID/randomUUID))
- defaults {:date_joined :%now
- :last_login nil
- :is_active true
- :is_superuser false}]
- ;; always salt + encrypt the password before putting new User in the DB
- ;; TODO - we should do password encryption in pre-update too instead of in the session code
- (merge
- defaults
- user
- {:password_salt salt
- :password (u.password/hash-bcrypt (str salt password))}
- ;; lower-case the email before saving
- {:email (u/lower-case-en email)}
- ;; if there's a reset token encrypt that as well
- (when reset_token
- {:reset_token (u.password/hash-bcrypt reset_token)})
- ;; normalize the locale
- (when locale
- {:locale (i18n/normalized-locale-string locale)}))))
+ (merge
+ insert-default-values
+ user
+ (hashed-password-values user)
+ ;; lower-case the email before saving
+ {:email (u/lower-case-en email)}
+ ;; if there's a reset token encrypt that as well
+ (when reset_token
+ {:reset_token (u.password/hash-bcrypt reset_token)})
+ ;; normalize the locale
+ (when locale
+ {:locale (i18n/normalized-locale-string locale)})))
(defn- post-insert [{user-id :id, superuser? :is_superuser, :as user}]
(u/prog1 user
@@ -84,9 +91,8 @@
(db/insert! PermissionsGroupMembership
:group_id (u/the-id (perms-group/admin))
:user_id id)
- ;; don't use `delete!` here because that does the opposite and tries to update this user
- ;; which leads to a stack overflow of calls between the two
- ;; TODO - could we fix this issue by using `post-delete!`?
+ ;; don't use [[db/delete!]] here because that does the opposite and tries to update this user which leads to a
+ ;; stack overflow of calls between the two. TODO - could we fix this issue by using a `post-delete` method?
(and (not superuser?)
membership-exists?)
(db/simple-delete! PermissionsGroupMembership
@@ -102,6 +108,7 @@
(db/delete! 'PulseChannelRecipient :user_id id))
;; If we're setting the reset_token then encrypt it before it goes into the DB
(cond-> user
+ true (merge (hashed-password-values user))
reset-token (update :reset_token u.password/hash-bcrypt)
locale (update :locale i18n/normalized-locale-string)
email (update :email u/lower-case-en)))
@@ -318,19 +325,21 @@
(dissoc :password)
(assoc :ldap_auth true))))
+;;; TODO -- it seems like maybe this should just be part of the [[pre-update]] logic whenever `:password` changes; then
+;;; we can remove this function altogether.
(defn set-password!
- "Updates the stored password for a specified `User` by hashing the password with a random salt."
+ "Update the stored password for a specified `User`; kill any existing Sessions and wipe any password reset tokens.
+
+ The password is automatically hashed with a random salt; this happens in [[hashed-password-values]] which is called
+ by [[pre-insert]] or [[pre-update]])"
[user-id password]
- (let [salt (str (UUID/randomUUID))
- password (u.password/hash-bcrypt (str salt password))]
- ;; when changing/resetting the password, kill any existing sessions
- (db/simple-delete! Session :user_id user-id)
- ;; NOTE: any password change expires the password reset token
- (db/update! User user-id
- :password_salt salt
- :password password
- :reset_token nil
- :reset_triggered nil)))
+ ;; when changing/resetting the password, kill any existing sessions
+ (db/simple-delete! Session :user_id user-id)
+ ;; NOTE: any password change expires the password reset token
+ (db/update! User user-id
+ :password password
+ :reset_token nil
+ :reset_triggered nil))
(defn set-password-reset-token!
"Updates a given `User` and generates a password reset token for them to use. Returns the URL for password reset."
diff --git a/test/metabase/integrations/google_test.clj b/test/metabase/integrations/google_test.clj
index c8e1ea77d96..f0f8a0e0cfc 100644
--- a/test/metabase/integrations/google_test.clj
+++ b/test/metabase/integrations/google_test.clj
@@ -1,12 +1,13 @@
(ns metabase.integrations.google-test
- (:require [clojure.test :refer :all]
- [metabase.email-test :as et]
- [metabase.integrations.google :as google]
- [metabase.integrations.google.interface :as google.i]
- [metabase.models.user :refer [User]]
- [metabase.public-settings.premium-features :as premium-features]
- [metabase.test :as mt]
- [toucan.db :as db]))
+ (:require
+ [clojure.test :refer :all]
+ [metabase.email-test :as et]
+ [metabase.integrations.google :as google]
+ [metabase.integrations.google.interface :as google.i]
+ [metabase.models.user :refer [User]]
+ [metabase.public-settings.premium-features :as premium-features]
+ [metabase.test :as mt]
+ [toucan.db :as db]))
;;; --------------------------------------------- google-auth-client-id ----------------------------------------------
diff --git a/test/metabase/models/user_test.clj b/test/metabase/models/user_test.clj
index b2eb0b6f4e0..67c2b2d2fae 100644
--- a/test/metabase/models/user_test.clj
+++ b/test/metabase/models/user_test.clj
@@ -4,6 +4,7 @@
[clojure.string :as str]
[clojure.test :refer :all]
[metabase.http-client :as client]
+ [metabase.integrations.google]
[metabase.models
:refer [Collection
Database
@@ -29,6 +30,10 @@
[toucan.db :as db]
[toucan.hydrate :refer [hydrate]]))
+(comment
+ ;; this has to be loaded for the Google Auth tests to work
+ metabase.integrations.google/keep-me)
+
;;; Tests for permissions-set
(deftest check-test-users-have-valid-permissions-sets-test
@@ -476,3 +481,28 @@
(is (= "e8d63472"
(serdes.hash/raw-hash ["fred@flintston.es"])
(serdes.hash/identity-hash user))))))
+
+(deftest hash-password-on-update-test
+ (testing "Setting `:password` with [[db/update!]] should hash the password, just like [[db/insert!]]"
+ (let [plaintext-password "password-1234"]
+ (mt/with-temp User [{user-id :id} {:password plaintext-password}]
+ (let [salt (fn [] (db/select-one-field :password_salt User :id user-id))
+ hashed-password (fn [] (db/select-one-field :password User :id user-id))
+ original-hashed-password (hashed-password)]
+ (testing "sanity check: check that password can be verified"
+ (is (u.password/verify-password plaintext-password
+ (salt)
+ original-hashed-password)))
+ (is (= true
+ (db/update! User user-id :password plaintext-password)))
+ (let [new-hashed-password (hashed-password)]
+ (testing "password should have been hashed"
+ (is (not= plaintext-password
+ new-hashed-password)))
+ (testing "even tho the plaintext password is the same, hashed password should be different (different salts)"
+ (is (not= original-hashed-password
+ new-hashed-password)))
+ (testing "salt should have been set; verify password was hashed correctly"
+ (is (u.password/verify-password plaintext-password
+ (salt)
+ new-hashed-password)))))))))
--
GitLab