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 Google 2FA login #13005

Merged
merged 7 commits into from Nov 27, 2019
Merged

Fix Google 2FA login #13005

merged 7 commits into from Nov 27, 2019

Conversation

@ScoutHarris
Copy link
Contributor

ScoutHarris commented Nov 26, 2019

Fixes #13004
WPAuth PR: wordpress-mobile/WordPressAuthenticator-iOS#157

This uses the WPAuth changes to fix the Google 2FA login flow.

To test:


The flow that is fixed:

  • Select Log In > Continue with Google.
  • Select a Google account that is already connected to WordPress, and has 2FA enabled.
  • Verify you get prompted for a verification code, and can log in.

verification_prompt


Since a refactor was needed, sanity check these cases to be sure I didn't break anything:

  • Select a Google account that is already connected to WordPress, but does not have 2FA enabled. You should get logged in.
  • Select a Google account that matches an existing WP account email, but is not yet connected to a WP account. You should get prompted to connect them.
  • Select a Google account that does not match a WP account email. You should see this list of options.

what_do_i_do_now


PR submission checklist:

  • I have considered adding unit tests where possible.

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@ScoutHarris ScoutHarris added the Login label Nov 26, 2019
@ScoutHarris ScoutHarris added this to the 13.8 milestone Nov 26, 2019
@ScoutHarris ScoutHarris self-assigned this Nov 26, 2019
@peril-wordpress-mobile

This comment has been minimized.

Copy link

peril-wordpress-mobile bot commented Nov 26, 2019

You can trigger an installable build for these changes by visiting CircleCI here.

@ScoutHarris ScoutHarris mentioned this pull request Nov 26, 2019
@ScoutHarris ScoutHarris requested a review from frosty Nov 26, 2019
Copy link
Contributor

frosty left a comment

Thanks for fixing this and for taking Woo into consideration! I left a few code comments. I tested the following cases:

2FA account
Non-2FA account
Unconnected account
Non-matching account

All of these flows worked for me. However, when comparing to the existing app, I noticed one main difference – when I tap on Continue with Google, I no longer get pushed to the 'waiting for Google to complete' screen that I see on develop:

IMG_60FDC684C321-1

This also means when I finish the Google web view portion of the login, I'm taken back to the opening NUX screen (with the animations) while the login completes in the background.

@frosty

This comment has been minimized.

Copy link
Contributor

frosty commented Nov 27, 2019

I guess the above comments are technically more about the authenticator branch than this one, but as the testing steps are here I guess we can keep the discussion here too :)

@ScoutHarris

This comment has been minimized.

Copy link
Contributor Author

ScoutHarris commented Nov 27, 2019

Regarding when I tap on Continue with Google, I no longer get pushed to the 'waiting for Google to complete' screen -

Google sign in never showed the loading view. Enabling the old login flow examples this.

google_log_in

It's happening on develop because it's erroneously showing the sign up flow (which caused the 2FA bug to start with). So, it's actually displaying correctly, it's just Google signup vs signin have different flows.

@ScoutHarris ScoutHarris requested a review from frosty Nov 27, 2019
@frosty
frosty approved these changes Nov 27, 2019
@ScoutHarris ScoutHarris merged commit c384ab1 into develop Nov 27, 2019
7 checks passed
7 checks passed
Hound No violations found. Woof!
Peril All green. Yay.
Details
ci/circleci: Build Tests Your tests passed on CircleCI!
Details
ci/circleci: Installable Build/Hold Your job is on hold on CircleCI!
Details
ci/circleci: UI Tests (iPad Air 3rd generation) Your tests passed on CircleCI!
Details
ci/circleci: UI Tests (iPhone 11) Your tests passed on CircleCI!
Details
ci/circleci: Unit Tests Your tests passed on CircleCI!
Details
@ScoutHarris ScoutHarris deleted the fix/13004-show_google_2fa_login branch Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.