Biometrics in new settings #18258
Merged
Biometrics in new settings #18258
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
clauxx
requested review from
cammellos,
smohamedjavid,
J-Son89 and
mohsen-ghafouri
clauxx
force-pushed
the
feat/biometrics-new-settings
branch
from
da32288 to
b12bb6a
Compare
Jenkins BuildsClick to see older builds (33)
|
cammellos
reviewed
Dec 20, 2023
| @@ -28,6 +29,12 @@ | |||
| :FaceID (i18n/label :t/biometric-faceid) | |||
| (i18n/label :t/biometric-touchid))) | |||
|
|
|||
| (defn get-icon-by-type | |||
| [biometric-type] | |||
| (case biometric-type | |||
There was a problem hiding this comment.
we generally prefer condp = as case does not pick up compile time errors, in this case you can probably just use an (if, though I take the idea is that we will have more eventually
There was a problem hiding this comment.
yes, in case we would like to differentiate touch id with fingerprint or the face biometrics on android in the future
| (rf/reg-event-fx | ||
| :standard-auth/reset-login-password | ||
| (fn [{:keys [db]}] | ||
| {:db (update-in db [:profile/login] dissoc :password :error)})) |
There was a problem hiding this comment.
looks like you can use update (update db :profile/login dissoc...)
mohsen-ghafouri
approved these changes
Dec 20, 2023
| :action :selector | ||
| :action-props {:disabled? (not supported?) | ||
| :on-change press-handler | ||
| :checked? (and supported? enabled?)} |
There was a problem hiding this comment.
biometric-on? let var can be re-used here.
Comment on lines
71
to
73
| {:style {:padding-horizontal 20 | ||
| :padding-bottom 8 | ||
| :padding-top 12}} |
There was a problem hiding this comment.
Styles can be moved to style.cljs.
Thanks @smohamedjavid! Addressed your comments |
71% of end-end tests have passedFailed tests (8)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (6)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Passed tests (34)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
|
|
@clauxx - I checked the PR build. There was a crash on opening the app for login for a profile with the biometric login, and it got disabled later. This comes from status-mobile/src/status_im/contexts/profile/login/events.cljs Lines 180 to 198 in e59e34d |
J-Son89
reviewed
Dec 21, 2023
| (rf/dispatch [:hide-bottom-sheet]) | ||
| (rf/dispatch [:standard-auth/reset-login-password]) | ||
| (rf/dispatch [:biometric/enable | ||
| (security/mask-data password)]))}))) |
There was a problem hiding this comment.
@clauxx if you take a look at on-auth-success the password is already hashed etc 👍
There was a problem hiding this comment.
It depends if the password is from biometrics or password etc
There was a problem hiding this comment.
I'm just masking it here, so no hashing involved. Maybe it should be masked inside authorize before it's passed to on-auth-success, otherwise the whole purpose is kinda lost.
J-Son89
approved these changes
Dec 21, 2023
Great job ! @clauxx |
|
ISSUE 3 Touch ID toggle in the Profile Settings has two different names Preconditions: Android device with set fingerprint Steps:
Actual result: Fingerprint toggle has Touch ID naming |
clauxx
commented
Dec 22, 2023
Comment on lines
122
to
+124
| :on-auth-success (fn [entered-password] | ||
| (prn entered-password) | ||
| (rf/dispatch [:wallet/derive-address-and-add-account | ||
| {:sha3-pwd entered-password | ||
| {:sha3-pwd (security/safe-unmask-data entered-password) |
There was a problem hiding this comment.
@J-Son89 I added the change to mask the pwd inside authorize's on-auth-success to avoid the password hash getting leaked e.g. as it would in line 122 (prn entered-password), as it's still sensitive info.
|
@Francesca-G please have a look. FYI we agreed to handle the issues mentioned by @mariia-skrypnyk in a follow-up. |
Francesca-G
approved these changes
Dec 22, 2023
briansztamfater
pushed a commit
that referenced
this pull request
Jan 11, 2024
* feat: added biometrics setting to new-settings * fix: fix renaming issues from status-im2 * ref: addressed @cammellos' review comments * fix: open password settings in a modal * ref: addressed review comments * fix: disabling biometric clears auth-method from keychain * chore: quo/overlay seqs the childrend so need to add keys * fix: don't pass the password unmasked between events to avoid leaks
mariia-skrypnyk
added
anothers-profile
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #18204
Summary
Added the
Passwordsetting screen and theEnable Biometricfunctionality. In order to enable biometrics from the settings, the user has to enter their password first.Platforms
Areas that maybe impacted
Functional
Steps to test
status: ready