Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enabling biometry without password during sync #17960

Merged
merged 20 commits into from
Nov 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions src/react_native/keychain.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion src/status_im2/common/alert/events.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
(vector
action-button
dismiss-button)
dismiss-button)
(vector dismiss-button))
(when on-dismiss
{:cancelable false})))))

Expand Down
56 changes: 51 additions & 5 deletions src/status_im2/common/biometric/events.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -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]))

Expand Down Expand Up @@ -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]
Expand Down
82 changes: 68 additions & 14 deletions src/status_im2/common/keychain/events.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -79,33 +78,88 @@
(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))))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should "password" be in a private def ? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So seems like oops considers const vars as "dynamic values" and throws warnings at compiled time, although it shouldn't for string constants. It would work with oops/oget+, but apparently it's slower .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not the same, but still related, I prefer to pass keywords to oget because they only need to be allocated once. It's not important really, just a nano optimization, but feels more idiomatic to me 🤷🏼

(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
:keychain/save-password-and-auth-method
(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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If calling callback throws an exception, it will be swallowed and ignored since no other code is piping a .catch on the return value of the re-frame effect.

You can move the .catch outside the inner function so it also catches errors from calling callback, and then improving the error message if it has been migrated or not.

You may also use try/catch around (callback) if you want to assume that part of the code is synchronous. I prefer not to mix and assume that, but it should be fine because we don't use the return value of this re-frame effect.

(-> (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}))))))
41 changes: 13 additions & 28 deletions src/status_im2/contexts/onboarding/enable_biometrics/view.cljs
Original file line number Diff line number Diff line change
@@ -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]
Expand All @@ -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
Expand All @@ -26,31 +24,20 @@
: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]))
(rf/sub [:profile/customization-color]))
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
Expand Down Expand Up @@ -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- internale-enable-biometrics
[params]
[:f> f-enable-biometrics params])
(defn view
[]
[:f> f-enable-biometrics])

(def view (quo.theme/with-theme internale-enable-biometrics))
13 changes: 3 additions & 10 deletions src/status_im2/contexts/onboarding/events.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -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]}]
Expand Down Expand Up @@ -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?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this reads a bit funny is the variable actually "masked" if syncing? is true?
If not it seems the variable naming should be adjusted some what here 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it's masked regardless of syncing?. The difference is that during syncing the password is already hashed (we don't have access to the plaintext password), so we need to hash it for the other cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems good as is so 👍

masked-password
(security/hash-masked-password masked-password))
:on-success (fn []
(if syncing?
(rf/dispatch [:onboarding-2/navigate-to-enable-notifications])
Expand Down
Loading