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

Fix failed faceid clears auth settings #9882

Merged
merged 1 commit into from
Feb 25, 2020

Conversation

Ferossgp
Copy link
Contributor

Fixes #9871

@Ferossgp Ferossgp requested a review from a team as a code owner January 22, 2020 12:18
@Ferossgp Ferossgp self-assigned this Jan 22, 2020
@auto-assign auto-assign bot removed the request for review from a team January 22, 2020 12:18
@status-github-bot
Copy link

Pull Request Checklist

  • Docs: Updated the documentation, if affected
  • Docs: Added or updated inline comments explaining intention of the code
  • Tests: Ensured that all new UI elements have been assigned accessibility IDs
  • Tests: Signaled need for E2E tests with label, if applicable
  • Tests: Briefly described what was tested and what platforms were used
  • UI: In case of UI changes, ensured that UI matches Figma
  • UI: In case of UI changes, requested review from a Core UI designer
  • UI: In case of UI changes, included screenshots of implementation

@Ferossgp Ferossgp force-pushed the bug/failed-faceid-clears-auth-settings branch from 4663f3c to 43633af Compare January 22, 2020 12:24
@status-im-auto
Copy link
Member

status-im-auto commented Jan 22, 2020

Jenkins Builds

Click to see older builds (3)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 43633af #2 2020-01-22 12:33:24 ~8 min ios 📦ipa 📲
✔️ 43633af #2 2020-01-22 12:34:41 ~10 min android-e2e 📦apk 📲
✔️ 43633af #2 2020-01-22 12:35:24 ~10 min android 📦apk 📲
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ c8f2a94 #5 2020-01-28 09:11:33 ~10 min ios 📦ipa 📲
✔️ c8f2a94 #5 2020-01-28 09:13:24 ~12 min android-e2e 📦apk 📲
✔️ c8f2a94 #5 2020-01-28 09:13:52 ~12 min android 📦apk 📲
✔️ c8f2a94 #6 2020-02-13 10:45:38 ~9 min ios 📦ipa 📲
✔️ c8f2a94 #7 2020-02-24 15:38:53 ~10 min ios 📦ipa 📲
✔️ ff1022f #6 2020-02-25 11:41:06 ~13 min android-e2e 📦apk 📲

@hesterbruikman
Copy link
Contributor

by @simonricoo

The "faceid" icon is there but on the first click in just closes the keyboard. Second attempt worked just fine

@Ferossgp Ferossgp force-pushed the bug/failed-faceid-clears-auth-settings branch 3 times, most recently from 278c3bb to c8f2a94 Compare January 28, 2020 09:01
@Ferossgp Ferossgp requested a review from a team January 31, 2020 12:00
@Serhy
Copy link
Contributor

Serhy commented Feb 5, 2020

Looks good to me with iOS (FaceID) and Samsung S8 (TouchID) for the:

  • biometric icon (faceId/touchID) next to password input is present when it check is failed (two times in a row and 'Enter password' option offered) or cancelled

but from #9871 what's left is :
• biometric check should happen on the same view you're using to unlock your phone
That's not the case at the moment (both iOS and Android):
234234234234

VS

Screenshot 2020-02-05 at 17 43 49

If I enabled FaceID and re-open app, or login in to account from login page via tap on biometric icon next to password field (when FaceID failed) bio check happens on blank screen. @Ferossgp could we
make it transparent ( from Figma: background: #000000; opacity: 0.4;

@StatusWrike
Copy link

➤ Sergey Chumak commented:

Looks good to me with iOS (FaceID) and Samsung S8 (TouchID) for the:

  • biometric icon (faceId/touchID) next to password input is present when it check is failed (two times in a row and 'Enter password' option offered) or cancelled

but from #9871 what's left is :
• biometric check should happen on the same view you're using to unlock your phone
That's not the case at the moment (both iOS and Android):
234234234234 ( https://user-images.githubusercontent.com/8749671/73858499-28096580-4841-11ea-9fc9-f3fce606320e.gif )

VS

https://user-images.githubusercontent.com/8749671/73858745-90584700-4841-11ea-816d-53db11fdad79.png

If I enabled FaceID and re-open app, or login in to account from login page via tap on biometric icon next to password field (when FaceID failed) bio check happens on blank screen. @Ferossgp could we
make it transparent ( from Figma ( https://www.figma.com/file/dEIljL7UPbXgsZUA0Q4qlE5E/Onboarding?node-id=5038%3A214 ): background: #000000; opacity: 0.4;

@Ferossgp
Copy link
Contributor Author

Ferossgp commented Feb 9, 2020

@Serhy the background is hidden because you have enabled "Hide Status previews when app switching" in Privacy and security. The face-id popup is a native app, which is opened above the Status app and changes the focus.

@churik
Copy link
Member

churik commented Feb 18, 2020

Don't have phone with FaceID now, so will wait for @Serhy

@Serhy
Copy link
Contributor

Serhy commented Feb 24, 2020

@Ferossgp you are correct.
Well, I knew it was "reproducing" even with disabled "Hide Status previews" option, but I thought that I have to see this transparency on app start from it's closed state. It just looking like that FaceID not transparent but it's because when we start the app - the FaceID pop-up overlaps status logo (like there in gif above).
Which is not the case when login in via FaceID from login screen (with disabled security setting)
Basically it was my fault, all good!

Do not change auth method if user does not save password

Sanity check - fix only for biometric method

Check for new auth method only when save password

ScrollView Persist taps on login view

Signed-off-by: Gheorghe Pinzaru <feross95@gmail.com>
@Ferossgp Ferossgp force-pushed the bug/failed-faceid-clears-auth-settings branch from c8f2a94 to ff1022f Compare February 25, 2020 11:27
@Ferossgp Ferossgp merged commit ff1022f into develop Feb 25, 2020
@delete-merged-branch delete-merged-branch bot deleted the bug/failed-faceid-clears-auth-settings branch February 25, 2020 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Respond to FaceID failure callback
7 participants