feat: enable biometrics for standard-auth #17992
Conversation
J-Son89
requested review from
ilmotta,
FFFra,
siddarthkay,
ulisesmac,
Parveshdhull and
smohamedjavid
| (colors/custom-color :danger 50) | ||
| (colors/custom-color :danger 60) | ||
| theme)} | ||
| :color (colors/resolve-color :danger theme)} |
| {:on-enter-password on-enter-password | ||
| :button-icon-left auth-button-icon-left | ||
| :button-label auth-button-label}])}])))))) | ||
| (let [password-login (fn [{:keys [on-press-biometrics]}] |
There was a problem hiding this comment.
moved this to var for DRY purposes
| :on-press-biometrics on-press-biometrics | ||
| :button-icon-left auth-button-icon-left | ||
| :button-label auth-button-label}])}])) | ||
| biometrics-login (fn [on-press-biometrics] |
There was a problem hiding this comment.
moved this to a var for functional purposes. LMK if anyone has a cleaner approach.
Basically this kicks off biometrics authentication, but in the event biometrics fails it will fallback to password login, however that password screen will have a button to retrigger biometrics authentication. So this function needs to pass itself to as an argument
There was a problem hiding this comment.
maybe I should leave a comment on it?
There was a problem hiding this comment.
Yes, I think a comment would be nice.
It seems the reason for biometrics-login to be recursive is that we want to redispatch the event :show-bottom-sheet if biometrics fail, I think that's because we are executing on-close at the beggining of the biometrics-login fn.
So, what if we execute the on-close only when the user cancels or the login is successful?
I'm not familiar with this part of the code or UI flow, so I'm just suggesting.
Jenkins BuildsClick to see older builds (63)
|
| @@ -34,7 +35,6 @@ | |||
| :auth-button-icon-left auth-button-icon-left | |||
| :theme theme | |||
| :blur? blur? | |||
| :on-enter-password on-enter-password | |||
There was a problem hiding this comment.
removed on-enter-password in favor of generic -> on-auth-success
| @@ -76,7 +76,6 @@ | |||
| :dispatch [:onboarding-2/navigate-to-identifiers]}] | |||
| :db (-> db | |||
| (dissoc :profile/login) | |||
| (dissoc :auth-method) | |||
There was a problem hiding this comment.
does anyone know why we dissoc this here? 🤔 -> seems to me like we should keep it 🤷♂️
There was a problem hiding this comment.
We clean up the :profile/login and :auth-method keys after the successful generation of keys.
If we retain the keys, we will encounter this issue #15632.
| @@ -156,7 +156,7 @@ | |||
| (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])}}))) | |||
| :on-fail #(rf/dispatch [:profile.login/biometric-auth-fail])}}))) | |||
| @@ -105,7 +105,7 @@ | |||
| (rf/dispatch [:syncing/update-role constants/local-pairing-role-sender]) | |||
| (rf/dispatch [:hide-bottom-sheet])))] | |||
| (when-not (and error (string/blank? error)) | |||
| (let [key-uid (get-in db [:profile/login :key-uid]) | |||
| (let [key-uid (get-in db [:profile/profile :key-uid]) | |||
There was a problem hiding this comment.
not sure why we need both but :profile/login gets wiped and so this is persistent which is needed for bioetrics & password approach
J-Son89
force-pushed
the
jc/standard-auth-biometrics
branch
from
b0bf7da to
d647d3e
Compare
| :icon-only? true | ||
| :type :outline} | ||
| :i/face-id]) | ||
| ] |
| :label (i18n/label :t/profile-password) | ||
| :on-change-text on-change-password | ||
| :default-value (security/safe-unmask-data default-password)}] | ||
| [rn/view {:flex-direction :row} |
There was a problem hiding this comment.
Missing :style key for styles
| :on-press-biometrics on-press-biometrics | ||
| :button-icon-left auth-button-icon-left | ||
| :button-label auth-button-label}])}])) | ||
| biometrics-login (fn [on-press-biometrics] |
There was a problem hiding this comment.
Yes, I think a comment would be nice.
It seems the reason for biometrics-login to be recursive is that we want to redispatch the event :show-bottom-sheet if biometrics fail, I think that's because we are executing on-close at the beggining of the biometrics-login fn.
So, what if we execute the on-close only when the user cancels or the login is successful?
I'm not familiar with this part of the code or UI flow, so I'm just suggesting.
| :on-success (fn [response] | ||
| (rf/dispatch [:standard-auth/on-biometric-success | ||
| on-auth-success]) | ||
| (log/info "response" response)) |
There was a problem hiding this comment.
Do we need to log the success response? 😅
There was a problem hiding this comment.
seems good to remove :)
| @@ -76,7 +76,6 @@ | |||
| :dispatch [:onboarding-2/navigate-to-identifiers]}] | |||
| :db (-> db | |||
| (dissoc :profile/login) | |||
| (dissoc :auth-method) | |||
There was a problem hiding this comment.
We clean up the :profile/login and :auth-method keys after the successful generation of keys.
If we retain the keys, we will encounter this issue #15632.
There was a problem hiding this comment.
Worked well in my tests on Android 13 🚀
@smohamedjavid, @J-Son89, the library we use for biometrics https://github.com/naoufal/react-native-touch-id is as dead as a discarded draft. Have you heard of any discussion in the team to change this lib?
| (biometrics-login biometrics-login) | ||
| (do | ||
| (reset-password) | ||
| (password-login {:on-press-biometrics nil}))))))) |
There was a problem hiding this comment.
It's a bit unconventional in Clojure to pass nil explicitly.
;; Both return nil
(get {:on-press-biometrics nil} :on-press-biometrics) ; => nil
(get {} :on-press-biometrics) ; => nilThere was a problem hiding this comment.
sure, I can change that 👍
| @@ -34,7 +35,6 @@ | |||
| :auth-button-icon-left auth-button-icon-left | |||
There was a problem hiding this comment.
@J-Son89, probably something you realized at this point after having worked with the existing auth code. One of the reasons the code is convoluted is because I think we aren't using re-frame in the right spirit, i.e. we're breaking its architectural principles.
For instance, we call authorize/authorize in the UI layer, a function that calls functions with promises, hiding them behind callbacks, and which do further re-dispatches. Clearly, authorize/authorize should be decomposed as a pure event, and an effect that does the RN biometrics using the touch-id lib. The usual re-frame stuff.
I'm not suggesting changes in this PR because you are inheriting the debt and playing along, just something to keep in mind if you or somebody else touch auth code in a future PR.
There was a problem hiding this comment.
it's okay for me to make changes on this pr. it's a little unclear to me as to what details I should change here. If you want to pair quickly on this it would be great, or feel free to send a code snipet/highlight the issues and I can address 👍
There was a problem hiding this comment.
It's beyond my capacity now @J-Son89. There are different approaches. Maybe we should first start by looking at the biometric/events.cljs file. It seems we can leverage some things there. For example, the app db already has :biometric/supported-type set, so we don't need to call (biometric/get-supported-type) again in standard-auth.authorize/authorize. This simplifies a little bit.
If we could pair on the refactors next week it would be great, but of course this PR doesn't need to wait.
There was a problem hiding this comment.
Sure sure, sounds good to me! No pressure to do so. @clauxx was happy to work on this part of the codebase too so whenever you have capacity to take a call we can go through it and we can create the issues to work on it
| (rf/reg-event-fx :standard-auth/on-biometric-success | ||
| (fn [{:keys [db]} [callback]] | ||
| (let [key-uid (get-in db [:profile/profile :key-uid])] | ||
| {:keychain/get-user-password [key-uid callback]}))) |
There was a problem hiding this comment.
If you want to follow the guidelines by the book, the effects map should only use :fx and :db.
There was a problem hiding this comment.
sure, whatever is best 👍
thanks for testing @ilmotta 🙌 - I had not been involved in any discussions about dependencies etc, perhaps @clauxx knows something 🤔 |
True that the library has stopped maintaining it. Last time, I believe we discussed it in Istanbul. I guess it's time we need to start updating the libraries. There are only a few biometrics authentication libraries, and most of it is not actively maintained. The best one among those is https://github.com/SelfLender/react-native-biometrics. |
J-Son89
force-pushed
the
jc/standard-auth-biometrics
branch
2 times, most recently
from
4ee8b2b to
3d72746
Compare
| @@ -31,13 +30,13 @@ | |||
| (rf/sub [:profile/customization-color])) | |||
| syncing-results? (= :syncing-results @state/root-id)] | |||
| [rn/view {:style (style/buttons insets)} | |||
| [standard-auth/button | |||
| [quo/button | |||
There was a problem hiding this comment.
J-Son89
force-pushed
the
jc/standard-auth-biometrics
branch
3 times, most recently
from
6edcca0 to
cfb8ec0
Compare
J-Son89
force-pushed
the
jc/standard-auth-biometrics
branch
from
d66e0a8 to
85658fd
Compare
79% of end-end tests have passedFailed tests (4)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (6)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (38)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
|
|
@J-Son89 is it still not ready, right? Ping me when the PR is ready. |
|
Sorry @churik - not yet I added then removed the label |
J-Son89
force-pushed
the
jc/standard-auth-biometrics
branch
from
b3f776e to
239aee9
Compare
|
@churik - now it is ready for QA :) |
|
@J-Son89 there is still one flow with enter password: |
Hi @churik - I would like to add it there however afaik this is being worked on currently for the sharing address feature and so I figured there is no need to add this as part of this pr as it will be update very soon with the correct approach (which behaves slightly different to standard auth as it has the select address parts.) |
|
Ok, I will re-check that this flow include standard-auth when it is ready. All mentioned flows are checked on both devices. Also, I believe that face-id and touch-id should be different (on screenshot enabled biometric-auth is touch-id), but I believe it is part of the other PR. PR can be merged, thank you for your work! |
|
awesome, thanks @churik! :) |
J-Son89
force-pushed
the
jc/standard-auth-biometrics
branch
from
c45029b to
a38a0c5
Compare
J-Son89
force-pushed
the
jc/standard-auth-biometrics
branch
from
a38a0c5 to
3a298a3
Compare
fixes: #17445
This pr adds biometrics for standard authentication -
https://www.figma.com/file/V6nlpAWIf2e1XU8RJ9yQPe/Syncing-for-Mobile?type=design&node-id=1074-118387&mode=design&t=XdGbtSuHsABYU9C7-0
Wallet:
https://www.figma.com/file/HKncH4wnDwZDAhc4AryK8Y/Wallet-for-Mobile?type=design&node-id=1741-374592&mode=design&t=DEQa5r3JJAPDGMKj-0
Currently this is used in the wallet to create an account and the syncing page to get a Sync code.
To test:
-- Create an account with biometrics enabled ->
Go to new wallet, create a new account
-- Create a watch-only account with biometrics enabled
Go to new wallet, create add an address to watch.
Authorization is not needed but this stil should be tested for this pr.
-- Syncing, generate a new sync code.
Go to generate sync code page and generate
I also put in the error handling for when the biometrics fails multiple times and fallsback to password.
The same tests should also be run without biometrics enabled.
There are so many tests paths so let me know if I missed any 🙏