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

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
parent e1462273
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
enterprise/backend/test/metabase_enterprise/config_from_file/users_test.clj
+ 25
− 13
View file @ 45423cca
......@@ -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"))))
......
src/metabase/models/user.clj
+ 45
− 36
View file @ 45423cca
......@@ -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."
......
test/metabase/integrations/google_test.clj
+ 9
− 8
View file @ 45423cca
(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 ----------------------------------------------
......
test/metabase/models/user_test.clj
+ 30
− 0
View file @ 45423cca
......@@ -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)))))))))
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment