Add biometric auth if available to communities password input #19336
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
Jenkins BuildsClick to see older builds (121)
|
ibrkhalil
force-pushed
the
19188
branch
2 times, most recently
from
4143dcd
to
f08dc8f
Compare
clauxx
requested changes
Mar 25, 2024
| (rf/dispatch | ||
| [:communities/edit-shared-addresses | ||
| {:community-id id | ||
| :password (security/safe-unmask-data password) |
There was a problem hiding this comment.
It's best to pass the password masked across events and only unmask it as it's passed to status-go. Masking prevents the password from accidentally being logged in the events.
Comment on lines
37
to
38
| :on-auth-fail (fn [err] | ||
| (log/info "Biometric authentication failed" err))}]) |
There was a problem hiding this comment.
The error is already logged in the :standard-auth/on-biometric-fail event. Same for all the other usages of :on-auth-fail in this PR.
There was a problem hiding this comment.
So that means we don't need an on-auth-fail on most usages of standard-auth/authorize, NIce!
There was a problem hiding this comment.
Yeah, it's in case you have some different behavior if auth failed. The logging is handled by the event.
clauxx
approved these changes
Mar 25, 2024
54% of end-end tests have passedFailed tests (21)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (26)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
Parveshdhull
approved these changes
Mar 26, 2024
| (rf/dispatch | ||
| [:standard-auth/authorize | ||
| {:auth-button-label (i18n/label :t/request-to-join) | ||
| :theme theme |
There was a problem hiding this comment.
Why this one has :theme? Or should we also add this key to above cases?
There was a problem hiding this comment.
Will double check, I remember that it might not be needed.
|
Hey @ibrkhalil, thanx for the PR! Please check at least basic flow prior to requesting manual QA. I am unable to join community neither using password nor biometry. Checked on Android 12 using latest available PR build https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-240326-082507-b7878a-pr19336-universal.apk Password flow: telegram-cloud-document-2-5195454425656739810.mp4Biometry flow: (NOTE: black screen on this video is the moment of scanning fingerprint, this is not a bug, but hiding biometry modal by the recoding app) telegram-cloud-document-2-5195454425656739821.mp4 |
Sorry, I couldn't reproduce this on Android/iOS. |
@ibrkhalil I will re-check tomorrow on the new build. Maybe previous build was broken or there is some other reason (device/user/community specific). |
Thank you! |
@ibrkhalil I have re-checked. Sometimes I am able to join but most of the time I am not. I don't know what is the reason, maybe logs will help you to investigate. Here is the community I am tying to join Logs Status-debug-logs - 2024-03-27T133342.266.zip telegram-cloud-document-2-5197314932474921893.mp4 |
-27 13:19:11.439 28148 28599 E ReactNativeJS: 2024-03-27T11:19:11.431Z ERROR [status-im.contexts.communities.overview.events:101] - failed to request to join community {:community-id
03-27 13:19:11.439 28148 28599 E ReactNativeJS: "0x03fce47ac61b7242d77b1f58ff1f91a6f0a641056a9b2e75f5a227a07dc44916a3",
03-27 13:19:11.439 28148 28599 E ReactNativeJS: :error {:code -32000, :message "already joined"},
03-27 13:19:11.439 28148 28599 E ReactNativeJS: :event :communities/requested-to-join-error}
03-27 13:19:19.484 28148 28599 I ReactNativeJS: Running "community-account-selection-sheet
03-27 13:19:20.861 28148 28599 D ReactNativeJS: 2024-03-27T11:19:20.860Z DEBUG [status-im.common.keychain.events:35] - [keychain] can-save-user-password?
03-27 13:19:20.869 28148 28599 D ReactNativeJS: 2024-03-27T11:19:20.865Z DEBUG [native-module.core:439] - [native-module] rooted-device?
03-27 13:19:23.554 28148 28599 E ReactNativeJS: 2024-03-27T11:19:23.540Z ERROR [status-im.contexts.communities.overview.events:101] - failed to request to join community {:community-id
03-27 13:19:23.554 28148 28599 E ReactNativeJS: "0x03fce47ac61b7242d77b1f58ff1f91a6f0a641056a9b2e75f5a227a07dc44916a3",
03-27 13:19:23.554 28148 28599 E ReactNativeJS: :error {:code -32000, :message "already joined"},
03-27 13:19:23.554 28148 28599 E ReactNativeJS: :event :communities/requested-to-join-error}
03-27 13:19:28.198 28148 28599 I ReactNativeJS: Running "settings
03-27 13:19:30.693 28148 28599 I ReactNativeJS: Running "settings-password
03-27 13:19:36.180 28148 28599 I ReactNativeJS: Running "community-account-selection-sheet
03-27 13:19:37.568 28148 28599 D ReactNativeJS: 2024-03-27T11:19:37.564Z DEBUG [status-im.common.keychain.events:35] - [keychain] can-save-user-password?
03-27 13:19:37.576 28148 28599 D ReactNativeJS: 2024-03-27T11:19:37.573Z DEBUG [native-module.core:439] - [native-module] rooted-device?
03-27 13:19:37.773 28148 28599 I ReactNativeJS: Running "bottom-sheet
03-27 13:19:42.445 28148 28599 D ReactNativeJS: 2024-03-27T11:19:42.444Z DEBUG [native-module.core:480] - [native-module] sha3
03-27 13:19:42.801 28148 28599 D ReactNativeJS: 2024-03-27T11:19:42.793Z DEBUG [native-module.core:480] - [native-module] sha3
03-27 13:19:43.895 28148 28599 E ReactNativeJS: 2024-03-27T11:19:43.884Z ERROR [status-im.contexts.communities.overview.events:101] - failed to request to join community {:community-id
03-27 13:19:43.895 28148 28599 E ReactNativeJS: "0x03fce47ac61b7242d77b1f58ff1f91a6f0a641056a9b2e75f5a227a07dc44916a3",
03-27 13:19:43.895 28148 28599 E ReactNativeJS: :error {:code -32000, :message "already joined"},
03-27 13:19:43.895 28148 28599 E ReactNativeJS: :event :communities/requested-to-join-error}The logs say, You are already a member |
I haven't faced such issue in develop, so looks like this is a PR problem. For some reason UI is not being updated to Pending state when request is sent. |
Will rebase again |
QA NOTES: Whoever will pick up this PR we need to try generating case when user is in fact joined the community but UI remains in state when user can send Join request. In order to generate this state we need to Send community request and try to cancel it at the moment when user already becomes community member (on admin side) but mobile UI still shows Pending state. You might need several tries to reach such state. This is how I faced ISSUE 1 It looks like that it is not PR issue although the behavior is slightly different in PR and develop:
Anyway, this looks like an edge case so does not seem to have high priority. But I might be missing something as I haven't deeply investigated and tested this PR. |
I assume that the fact that we're using biometrics now in this PR, And the flow of creating new request to join is faster might be a factor in this issue (Too fast subsequent request to join community requests). |
54% of end-end tests have passedFailed tests (21)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (26)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
|
@ibrkhalil thank you for the PR. Please take a look at the issue. ISSUE 1 In case of many too many failed biometric attempts user is unable to send request via password [ANDROID]Android only. Steps:
Expected result: password bottom sheet should be opened so user be able to send request via password. Actual result: error appears every time user tries to send request, password bottom sheet doe not appear. telegram-cloud-document-2-5213367467233001469.mp4 |
|
Hey @yevh-berdnyk! Could you please push a fix for e2e? Thank you! |
96% of end-end tests have passedFailed tests (1)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (46)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
|
Thank you so much Pavlo! |
|
Hi @ibrkhalil
What exactly has been fixed? Behaviour remained the same for me in latest build. |
Textual password input field should show after ~5 biometric failures on Android. |
Please check if it works for you here https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-240402-121459-57d96bf5841c23c93ea3c615c594e43e80e8b4b5-pr19336-universal.apk this is link to the latest build from this PR. |
screen-20240402-183359.mp4 |
Will add another proper video |
trim.7A755E79-43BB-4E11-9312-0BEEC426707B.MOV |
Both videos are from the link you posted. |
|
@ibrkhalil unfortunately this fix does not work for me (Samsung Galaxy A52, Android 12) + it causes small regression: ugly error after many attempts on login screen (see screenshot below). Could you please revert your last changes? I will log ISSUE 1 separately after we merge this PR. I believe it is not related to your PR directly but to biometric feature overall, so should be addressed separately.
|
Reverted |
Also, I am happy to fix the Samsung issue in this PR. |
56% of end-end tests have passedFailed tests (20)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (27)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
96% of end-end tests have passedFailed tests (1)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (46)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
|
@ibrkhalil thank you for the PR. Ready for merge. |
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 #19188
Summary
Adds biometric auth to communities request-to-join/edit-shared-addresses/edit-airdrop-addresses and adds a workaround when a too-many-attempts error is thrown when doing Android biometrics.
status: ready