From ed270b21620c31fee370be3d1e7e98a7ae65f7ae Mon Sep 17 00:00:00 2001 From: Lungu Cristian Date: Thu, 30 Nov 2023 13:40:29 +0200 Subject: [PATCH] Enabling biometry without password during sync (#17960) * feat: added migration for the keychain hashed password * feat: added sync biometry without password entry * fix: biometry typo from develop * ref: moved migration side-effects outside the event * ref: some renaming for keychain migration * ref: addressed @cammellos' review comments * ref: removed unnecessary anon fn * fix: addressed @ilmotta's review comments * ref: removed theme from enable-biometrics * ref: addressed J-Son89's review comments * test: added tests for mask-data and hash-masked-password * test: added schema to hash-masked-password and fixed test * fix: forgot the threading * ref: improved the masked data schema * fix: no biometry error when canceled by user * fix: biometry error wasn't propagated during login * fix: alert dismiss button not passed properly * fix: show biometrics NOT_ENROLLED error only once * lint: removed unused require --- src/react_native/keychain.cljs | 18 ++-- src/status_im2/common/alert/events.cljs | 2 +- src/status_im2/common/biometric/events.cljs | 56 +++++++++++-- src/status_im2/common/keychain/events.cljs | 82 +++++++++++++++---- .../onboarding/enable_biometrics/view.cljs | 41 +++------- .../contexts/onboarding/events.cljs | 13 +-- .../contexts/profile/login/events.cljs | 67 +++++++++------ src/utils/security/core.cljs | 22 +++++ src/utils/security/security_test.cljs | 24 ++++++ 9 files changed, 236 insertions(+), 89 deletions(-) diff --git a/src/react_native/keychain.cljs b/src/react_native/keychain.cljs index 3eb0c3fc9c..66fd8cf4b4 100644 --- a/src/react_native/keychain.cljs +++ b/src/react_native/keychain.cljs @@ -58,14 +58,16 @@ (defn save-credentials "Stores the credentials for the address to the Keychain" - [server username password callback] - (-> (.setInternetCredentials ^js react-native-keychain - (string/lower-case server) - username - password - keychain-secure-hardware - keychain-restricted-availability) - (.then callback))) + ([server username password] + (save-credentials server username password identity)) + ([server username password callback] + (-> (.setInternetCredentials ^js react-native-keychain + (string/lower-case server) + username + password + keychain-secure-hardware + keychain-restricted-availability) + (.then callback)))) (defn get-credentials "Gets the credentials for a specified server from the Keychain" diff --git a/src/status_im2/common/alert/events.cljs b/src/status_im2/common/alert/events.cljs index 0664035f7d..e12e03e8b5 100644 --- a/src/status_im2/common/alert/events.cljs +++ b/src/status_im2/common/alert/events.cljs @@ -22,7 +22,7 @@ (vector action-button dismiss-button) - dismiss-button) + (vector dismiss-button)) (when on-dismiss {:cancelable false}))))) diff --git a/src/status_im2/common/biometric/events.cljs b/src/status_im2/common/biometric/events.cljs index 928c5e85e9..7508e197a0 100644 --- a/src/status_im2/common/biometric/events.cljs +++ b/src/status_im2/common/biometric/events.cljs @@ -2,9 +2,11 @@ (:require [native-module.core :as native-module] [re-frame.core :as re-frame] + [react-native.async-storage :as async-storage] [react-native.platform :as platform] [react-native.touch-id :as touch-id] [status-im2.common.keychain.events :as keychain] + [taoensso.timbre :as log] [utils.i18n :as i18n] [utils.re-frame :as rf])) @@ -41,16 +43,60 @@ {:db (assoc db :biometric/supported-type supported-type)}) (rf/defn show-message + {:events [:biometric/show-message]} [_ code] - (let [content (if (#{"NOT_AVAILABLE" "NOT_ENROLLED"} code) - (i18n/label :t/grant-face-id-permissions) - (when-not (or (= code "USER_CANCELED") (= code "USER_FALLBACK")) - (i18n/label :t/biometric-auth-error {:code code})))] - (when content + (let [handle-error? (and code + (not (contains? #{"USER_CANCELED" "USER_FALLBACK"} code))) + content (if (#{"NOT_AVAILABLE" "NOT_ENROLLED"} code) + (i18n/label :t/grant-face-id-permissions) + (i18n/label :t/biometric-auth-error {:code code}))] + (when handle-error? {:utils/show-popup {:title (i18n/label :t/biometric-auth-login-error-title) :content content}}))) +(defn- supress-biometry-error-key + [key-uid] + (keyword (str "biometric/supress-not-enrolled-error-" key-uid))) + +;; NOTE: if the account had biometrics registered, but it's not enrolled at the moment, +;; we should show the error message only once and supress further "NOT_ENROLLED" errors +;; until biometry is enrolled again. Note that we can only know that when :biometric/authenticate +;; is dispatched and fails with "NOT_ENROLLED", since :biometric/get-supported-biometric-type +;; only tells us what kind of biometric is available on the device, but it doesn't know of its +;; enrollment status. +(re-frame/reg-fx + :biometric/supress-not-enrolled-error + (fn [[key-uid dispatch-event]] + (let [storage-key (supress-biometry-error-key key-uid)] + (-> (async-storage/get-item storage-key identity) + (.then (fn [item] + (when (not item) + (rf/dispatch dispatch-event) + (async-storage/set-item! storage-key true)))) + (.catch (fn [err] + (log/error "Couldn't supress biometry NOT_ENROLLED error" + {:key-uid key-uid + :event :biometric/supress-not-enrolled-error + :error err}))))))) + +;; NOTE: when biometrics is re-enrolled, we erase the flag in async-storage to assure +;; the "NOT_ENROLLED" error message will be shown again if biometrics is un-enrolled +;; in the future. +(re-frame/reg-fx + :biometric/reset-not-enrolled-error + (fn [key-uid] + (let [storage-key (supress-biometry-error-key key-uid)] + (-> (async-storage/get-item storage-key identity) + (.then (fn [supress?] + (when supress? + (async-storage/set-item! storage-key nil)))) + (.catch (fn [err] + (log/error "Couldn't reset supressing biometry NOT_ENROLLED error" + {:key-uid key-uid + :event :biometric/reset-not-enrolled-error + :error err}))))))) + (re-frame/reg-fx :biometric/authenticate (fn [options] diff --git a/src/status_im2/common/keychain/events.cljs b/src/status_im2/common/keychain/events.cljs index 84d7c88481..f6eb8e2b27 100644 --- a/src/status_im2/common/keychain/events.cljs +++ b/src/status_im2/common/keychain/events.cljs @@ -49,16 +49,15 @@ (defn save-auth-method! [key-uid method] - (keychain/save-credentials - (str key-uid "-auth") - key-uid - method - #(when-not % - (log/error - (str "Error while saving auth method." - " " - "The app will continue to work normally, " - "but you will have to login again next time you launch it."))))) + (-> (keychain/save-credentials + (str key-uid "-auth") + key-uid + method) + (.catch (fn [err] + (log/error "Failed to save auth method in the keychain" + {:error err + :key-uid key-uid + :auth-method method}))))) (re-frame/reg-fx :keychain/save-auth-method @@ -79,27 +78,59 @@ (if can-save? (keychain/get-credentials (str key-uid "-auth") - #(callback (if % (oops/oget % "password") auth-method-none))) + (fn [value] + (callback (if value (oops/oget value "password") auth-method-none)))) (callback nil)))))) (defn save-user-password! [key-uid password] (keychain/save-credentials key-uid key-uid (security/safe-unmask-data password) #())) +(defn get-user-password! + [key-uid callback] + (keychain/get-credentials key-uid + (fn [value] + (callback (when value + (-> value + (oops/oget "password") + (security/mask-data))))))) + (re-frame/reg-fx :keychain/get-user-password (fn [[key-uid callback]] - (keychain/get-credentials - key-uid - #(if % (callback (security/mask-data (oops/oget % "password"))) (callback nil))))) + (get-user-password! key-uid callback))) (rf/defn get-user-password [_ key-uid callback] {:keychain/get-user-password [key-uid callback]}) +(defn- password-migration-key-name + [key-uid] + (str key-uid "-password-migration")) + +(defn save-password-migration! + [key-uid] + (-> (keychain/save-credentials + (password-migration-key-name key-uid) + key-uid + ;; NOTE: using the key-uid as the password, but we don't really care about the + ;; value, we only care that it's there + key-uid) + (.catch (fn [error] + (log/error "Failed to get the keychain password migration flag" + {:error error + :key-uid key-uid}))))) + +(defn get-password-migration! + [key-uid callback] + (keychain/get-credentials + (password-migration-key-name key-uid) + (comp callback boolean))) + (re-frame/reg-fx :keychain/clear-user-password (fn [key-uid] + (keychain/reset-credentials (password-migration-key-name key-uid)) (keychain/reset-credentials key-uid))) (re-frame/reg-fx @@ -107,5 +138,28 @@ (fn [{:keys [key-uid masked-password on-success on-error]}] (-> (save-user-password! key-uid masked-password) (.then #(save-auth-method! key-uid auth-method-biometric)) + (.then #(save-password-migration! key-uid)) (.then #(when on-success (on-success))) (.catch #(when on-error (on-error %)))))) + +;; NOTE: migrating the plaintext password in the keychain +;; with the hashed one. Added due to the sync onboarding +;; flow, where the password arrives already hashed. +(re-frame/reg-fx + :keychain/password-hash-migration + (fn [{:keys [key-uid callback] + :or {callback identity}}] + (-> (get-password-migration! key-uid identity) + (.then (fn [migrated?] + (if migrated? + (callback) + (-> (get-user-password! key-uid identity) + (.then security/hash-masked-password) + (.then #(save-user-password! key-uid %)) + (.then #(save-password-migration! key-uid)) + (.then callback))))) + (.catch (fn [err] + (log/error "Failed to migrate the keychain password" + {:error err + :key-uid key-uid + :event :keychain/password-hash-migration})))))) diff --git a/src/status_im2/contexts/onboarding/enable_biometrics/view.cljs b/src/status_im2/contexts/onboarding/enable_biometrics/view.cljs index 146d2e754d..8ad8a42c41 100644 --- a/src/status_im2/contexts/onboarding/enable_biometrics/view.cljs +++ b/src/status_im2/contexts/onboarding/enable_biometrics/view.cljs @@ -1,7 +1,6 @@ (ns status-im2.contexts.onboarding.enable-biometrics.view (:require [quo.core :as quo] - [quo.theme :as quo.theme] [react-native.core :as rn] [react-native.safe-area :as safe-area] [status-im2.common.biometric.events :as biometric] @@ -12,8 +11,7 @@ [status-im2.contexts.onboarding.enable-biometrics.style :as style] [status-im2.navigation.state :as state] [utils.i18n :as i18n] - [utils.re-frame :as rf] - [utils.security.core :as security])) + [utils.re-frame :as rf])) (defn page-title @@ -26,7 +24,7 @@ :description-accessibility-label :enable-biometrics-sub-title}]) (defn enable-biometrics-buttons - [insets theme] + [insets] (let [supported-biometric-type (rf/sub [:biometric/supported-type]) bio-type-label (biometric/get-label-by-type supported-biometric-type) profile-color (or (:color (rf/sub [:onboarding-2/profile])) @@ -34,23 +32,12 @@ syncing-results? (= :syncing-results @state/root-id)] [rn/view {:style (style/buttons insets)} [standard-auth/button - (merge - {:size 40 - :accessibility-label :enable-biometrics-button - :icon-left :i/face-id - :customization-color profile-color - :button-label (i18n/label :t/biometric-enable-button {:bio-type-label bio-type-label})} - (if syncing-results? - {:theme theme - :blur? true - :on-enter-password (fn [entered-password] - (rf/dispatch - [:onboarding-2/authenticate-enable-biometrics - (security/safe-unmask-data - entered-password)]) - (rf/dispatch [:hide-bottom-sheet])) - :auth-button-label (i18n/label :t/confirm)} - {:on-press #(rf/dispatch [:onboarding-2/enable-biometrics])}))] + {:size 40 + :accessibility-label :enable-biometrics-button + :icon-left :i/face-id + :customization-color profile-color + :on-press #(rf/dispatch [:onboarding-2/enable-biometrics]) + :button-label (i18n/label :t/biometric-enable-button {:bio-type-label bio-type-label})}] [quo/button {:accessibility-label :maybe-later-button :background :blur @@ -78,18 +65,16 @@ :source (resources/get-image :biometrics)}])) (defn f-enable-biometrics - [{:keys [theme]}] + [] (let [insets (safe-area/get-insets)] [rn/view {:style (style/page-container insets)} [page-title] (if whitelist/whitelisted? [enable-biometrics-parallax] [enable-biometrics-simple]) - [enable-biometrics-buttons insets theme]])) + [enable-biometrics-buttons insets]])) +(defn view + [] + [:f> f-enable-biometrics]) -(defn- internale-enable-biometrics - [params] - [:f> f-enable-biometrics params]) - -(def view (quo.theme/with-theme internale-enable-biometrics)) diff --git a/src/status_im2/contexts/onboarding/events.cljs b/src/status_im2/contexts/onboarding/events.cljs index 814ac84cb7..3295513428 100644 --- a/src/status_im2/contexts/onboarding/events.cljs +++ b/src/status_im2/contexts/onboarding/events.cljs @@ -35,15 +35,6 @@ {:biometric/authenticate {:on-success #(rf/dispatch [:onboarding-2/biometrics-done]) :on-fail #(rf/dispatch [:onboarding-2/biometrics-fail %])}}) -(rf/defn authenticate-enable-biometrics - {:events [:onboarding-2/authenticate-enable-biometrics]} - [{:keys [db]} password] - {:db (-> db - (assoc-in [:onboarding-2/profile :password] password) - (assoc-in [:onboarding-2/profile :syncing?] true)) - :biometric/authenticate {:on-success #(rf/dispatch [:onboarding-2/biometrics-done]) - :on-fail #(rf/dispatch [:onboarding-2/biometrics-fail %])}}) - (rf/defn navigate-to-enable-notifications {:events [:onboarding-2/navigate-to-enable-notifications]} [{:keys [db]}] @@ -154,7 +145,9 @@ biometric-enabled? (assoc :keychain/save-password-and-auth-method {:key-uid key-uid - :masked-password masked-password + :masked-password (if syncing? + masked-password + (security/hash-masked-password masked-password)) :on-success (fn [] (if syncing? (rf/dispatch [:onboarding-2/navigate-to-enable-notifications]) diff --git a/src/status_im2/contexts/profile/login/events.cljs b/src/status_im2/contexts/profile/login/events.cljs index 6734abe05b..f9737409bc 100644 --- a/src/status_im2/contexts/profile/login/events.cljs +++ b/src/status_im2/contexts/profile/login/events.cljs @@ -11,7 +11,6 @@ [status-im.group-chats.core :as group-chats] [status-im.mobile-sync-settings.core :as mobile-network] [status-im.transport.core :as transport] - [status-im2.common.biometric.events :as biometric] [status-im2.common.keychain.events :as keychain] [status-im2.common.log :as logging] [status-im2.config :as config] @@ -42,11 +41,22 @@ {:db (assoc-in db [:profile/login :processing] true) ::login [key-uid (native-module/sha3 (security/safe-unmask-data password))]})) +(rf/defn biometrics-login + {:events [:profile.login/biometrics-login]} + [{:keys [db]}] + (let [{:keys [key-uid password]} (:profile/login db)] + {:db (assoc-in db [:profile/login :processing] true) + ::login [key-uid (security/safe-unmask-data password)]})) + (rf/defn login-local-paired-user {:events [:profile.login/local-paired-user]} [{:keys [db]}] - (let [{:keys [key-uid password]} (get-in db [:syncing :profile])] - {::login [key-uid password]})) + (let [{:keys [key-uid password]} (get-in db [:syncing :profile]) + masked-password (security/mask-data password)] + {:db (-> db + (assoc-in [:onboarding-2/profile :password] masked-password) + (assoc-in [:onboarding-2/profile :syncing?] true)) + ::login [key-uid password]})) (rf/defn redirect-to-root [{:keys [db] :as cofx}] @@ -151,20 +161,16 @@ (rf/defn get-auth-method-success {:events [:profile.login/get-auth-method-success]} - [{:keys [db]} auth-method] + [{:keys [db]} auth-method key-uid] (merge {:db (assoc db :auth-method auth-method)} (when (= auth-method keychain/auth-method-biometric) - {:biometric/authenticate - {:on-success #(rf/dispatch [:profile.login/biometric-success]) - :on-faile #(rf/dispatch [:profile.login/biometric-auth-fail])}}))) - -(rf/defn biometric-auth-success - {:events [:profile.login/biometric-success]} - [{:keys [db] :as cofx}] - (let [key-uid (get-in db [:profile/login :key-uid])] - (keychain/get-user-password cofx - key-uid - #(rf/dispatch [:profile.login/get-user-password-success %])))) + {:keychain/password-hash-migration + {:key-uid key-uid + :callback (fn [] + (rf/dispatch [:biometric/authenticate + {:on-success #(rf/dispatch [:profile.login/biometric-success]) + :on-fail #(rf/dispatch + [:profile.login/biometric-auth-fail %])}]))}}))) ;; result of :keychain/get-auth-method above (rf/defn get-user-password-success @@ -175,14 +181,29 @@ cofx {:db (assoc-in db [:profile/login :password] password)} (navigation/init-root :progress) - (login)))) + (biometrics-login)))) + +(rf/reg-event-fx + :profile.login/biometric-success + (fn [{:keys [db]}] + (let [key-uid (get-in db [:profile/login :key-uid])] + {:db db + :fx [[:biometric/reset-not-enrolled-error key-uid] + [:keychain/get-user-password + [key-uid #(rf/dispatch [:profile.login/get-user-password-success %])]]]}))) + +(rf/reg-event-fx + :profile.login/biometric-auth-fail + (fn [{:keys [db]} [code]] + (let [key-uid (get-in db [:profile/login :key-uid])] + {:db db + :fx [[:dispatch [:init-root :profiles]] + (if (= code "NOT_ENROLLED") + [:biometric/supress-not-enrolled-error + [key-uid + [:biometric/show-message code]]] + [:dispatch [:biometric/show-message code]])]}))) -(rf/defn biometric-auth-fail - {:events [:profile.login/biometric-auth-fail]} - [{:keys [db] :as cofx} code] - (rf/merge cofx - (navigation/init-root :profiles) - (biometric/show-message code))) (rf/defn verify-database-password {:events [:profile.login/verify-database-password]} @@ -197,7 +218,7 @@ (rf/defn verify-database-password-success {:events [:profile.login/verified-database-password]} - [{:keys [db] :as cofx} valid? callback] + [{:keys [db]} valid? callback] (if valid? (do (when (fn? callback) diff --git a/src/utils/security/core.cljs b/src/utils/security/core.cljs index 84c9b61b67..029d723ceb 100644 --- a/src/utils/security/core.cljs +++ b/src/utils/security/core.cljs @@ -1,5 +1,7 @@ (ns utils.security.core (:require + [native-module.core :as native-module] + [schema.core :as schema] [utils.security.security-html :as h])) (defprotocol Unmaskable @@ -58,3 +60,23 @@ and does not contain an rtlo character, which might mean that the url is spoofed" [text] (not (re-matches rtlo-link-regex text))) + +(defn hash-masked-password + [masked-password] + (-> masked-password + safe-unmask-data + native-module/sha3 + mask-data)) + +(defn masked-data-instance? + [value] + (instance? MaskedData value)) + +(def ?masked-password + [:fn {:error/message "should be an instance of utils.security.core/MaskedData"} + masked-data-instance?]) + +(schema/=> hash-masked-password + [:=> + [:cat ?masked-password] + ?masked-password]) diff --git a/src/utils/security/security_test.cljs b/src/utils/security/security_test.cljs index 3e8c2a51f6..5c75644911 100644 --- a/src/utils/security/security_test.cljs +++ b/src/utils/security/security_test.cljs @@ -1,6 +1,7 @@ (ns utils.security.security-test (:require [cljs.test :refer-macros [deftest is testing]] + [native-module.core :as native-module] [utils.security.core :as security])) (def rtlo-link "‮http://google.com") @@ -29,3 +30,26 @@ (deftest safe-link-text-test-exceptions (testing "rtlo links" (is (not (security/safe-link-text? rtlo-link-text))))) + +(deftest mask-data-test + (testing "returns an instance of MaskedData" + (is (instance? security/MaskedData (security/mask-data "test")))) + (testing "hides the original value" + (is (= "******" (str (security/mask-data "test"))))) + (testing "succeeds the equality check between same MaskedData instances" + (is (= (security/mask-data "value") (security/mask-data "value")))) + (testing "fails the equality check between different MaskedData instances" + (is (not (= (security/mask-data "value-A") (security/mask-data "value-B"))))) + (testing "fails the equality check with non-MaskedData instances" + (is (not (= (security/mask-data "value") "value")))) + (testing "counts the masked data correctly" + (is (= (count "test") (count (security/mask-data "test"))))) + (testing "unmasks the data correctly" + (is (= "test" (-> "test" security/mask-data security/safe-unmask-data))))) + +(deftest hash-masked-password-test + (testing "returns an instance of MaskedData with the hashed content" + (is (= (-> "test" native-module/sha3 security/mask-data) + (-> "test" security/mask-data security/hash-masked-password)))) + (testing "throws a schema exception if the argument is not an instance of MaskedData" + (is (thrown? js/Error (security/hash-masked-password "test")))))