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

Bugfix: Fix mobile sync & Security options #989

Merged
merged 12 commits into from
Aug 16, 2019
Merged

Conversation

brunobar79
Copy link
Contributor

@brunobar79 brunobar79 commented Aug 6, 2019

This PR addresses three separate issues related to sync & security / biometrics

  1. I was able to reproduce the infinite spinner and at the same time found the root cause of it.

When one request failed, the retry logic kept two listeners active, which causes all the messages to arrive twice. That breaks the json structure while rebuilding the state, throwing an exception on JSON.parse

This PR updates the way we handle the listener removal based on the pubnub docs and also bumps pubnub dep to its latest version, which was different to the one that we were using in the extension and I'm updating on a separate PR.

In order to fix the issue with older versions of the extension where the same messages can still be sent twice I've added a check to make sure if it's repeated to be ignored.

Fixes #896

  1. Now users will get prompted to enable biometrics while syncing with the extension.
    Additionally fixes a bug with the flag biometryChoiceDisabled which was being reset every time the login screen was shown

Fixes #985 and also #927

  1. Added the ability to enable / disable device PIN / Passcode separately from biometrics, allowing users to use any of the 4 ways of logging in:
  • password
  • password + remember me
  • device PIN / PASSCODE
  • biometrics

If you disable biometrics it will fallback to device PIN, if you disable device PIN it will default to password.

@brunobar79 brunobar79 added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Aug 6, 2019
@brunobar79 brunobar79 changed the title Fix mobile sync Bugfix - Fix mobile sync Aug 6, 2019
@brunobar79 brunobar79 changed the title Bugfix - Fix mobile sync Bugfix: Fix mobile sync Aug 6, 2019
@brunobar79 brunobar79 changed the title Bugfix: Fix mobile sync Bugfix: Fix mobile sync & Security options Aug 7, 2019
Copy link
Contributor

@estebanmino estebanmino left a comment

Choose a reason for hiding this comment

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

💯 💯 nice pr, code lgtm

@ibrahimtaveras00
Copy link
Contributor

Issue 1:
happens on iOS

When saying no to touch ID and then killing/relaunching app, I’m prompted with touch ID

Steps:
-When prompted to use touch ID say no
-go to settings > privacy ensure both touch ID and passcode are disabled
-kill app and relaunch notice you’re prompted for touch ID (or faceID if enabled)

Issue 2:
happens on iOS

I enable touch ID via settings, then log out, I still see remember me and then once logging back in, touch ID is disabled

Steps:
-When prompted to use touch ID say no
-go to settings > privacy and enable touch ID
-log out, and notice how it still says remember me
-log back in and go to settings and notice how touch ID is now disabled

Same thing happens if I enable touch ID in settings, kill app, relaunch, then log out

Issue 3:
happens on both iOS and Android

When on a device that doesn't support biometrics, but does support pin/passcode I get an error

Note: this happens with a device that doesn't have pin enabled as well

Seen here = http://recordit.co/CSfy5TWEQj

image

image

Issue 4:
Happens on Android

When I try the import seed phrase flow, and toggle OFF the fingerprint option then continue onwards, before being shown the opt in view, I’m prompted to enter my pin passcode where it says “Please authenticate in order to use MetaMask”

Steps:
-Import via seed phrase but toggle OFF the fingerprint option before continuing

@ibrahimtaveras00
Copy link
Contributor

Fixes look good, QA Passed 👍

@brunobar79 brunobar79 merged commit d1d2180 into develop Aug 16, 2019
@brunobar79 brunobar79 deleted the fix-mobile-sync branch August 16, 2019 23:17
rickycodes pushed a commit that referenced this pull request Jan 31, 2022
* fix mobile sync

* clean up

* add device passcode option

* code review comments

* disable passcode while cancelling biometrics on sync

* fix issue 2

* fix import from seed scenario

* minor fixes

* update snapshots
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-qa Any New Features that needs a full manual QA prior to being added to a release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fingerprint unlock with fingerprint unlocking disabled! sync freeze
3 participants