Skip to content
This repository was archived by the owner on Feb 5, 2025. It is now read-only.

Conversation

@ScoutHarris
Copy link
Contributor

@ScoutHarris ScoutHarris commented Nov 26, 2019

Ref WPiOS issue: wordpress-mobile/WordPress-iOS#13004
Can be tested with WPiOS PR: wordpress-mobile/WordPress-iOS#13005
Woo test branch: try/google_login

This fixes the path where a user tries to login in with a Google account that has 2FA enabled on the WordPress account.

Previously, it was using the Google sign up flow, which was incorrect (and doesn't handle 2FA accounts).

This moves the Google sign in logic into LoginViewController so it can be accessed via LoginEmailViewController (flow used by Woo) and LoginPrologueViewController (flow used by WPiOS).

@ScoutHarris ScoutHarris self-assigned this Nov 26, 2019
@ScoutHarris
Copy link
Contributor Author

@jaclync or @pmusolino -

Can I bother you to test these changes in Woo? Specifically, the sanity check these cases noted in the WPiOS PR wordpress-mobile/WordPress-iOS#13005. I created Woo branch try/google_login that uses the WPAuth changes. I'm not going to merge this branch, it's just to facilitate testing this. To note, logging in with a Google account in Woo should not have changed at all. I'd just like to be sure. 😄

@jaclync
Copy link
Contributor

jaclync commented Nov 26, 2019

Specifically, the sanity check these cases noted in the WPiOS PR wordpress-mobile/WordPress-iOS#13005. I created Woo branch try/google_login that uses the WPAuth changes. I'm not going to merge this branch, it's just to facilitate testing this. To note, logging in with a Google account in Woo should not have changed at all. I'd just like to be sure. 😄

Thanks for creating the branch on WCiOS! I tested on develop and try/google_login, and logging in with Google + 2FA works on both branches 👍

@ScoutHarris
Copy link
Contributor Author

Thanks @jaclync !

Copy link
Contributor

@frosty frosty left a comment

Choose a reason for hiding this comment

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

Just a couple of comments to go along with my review of the WPiOS PR. Nothing major!

errorToPresent = error
performSegue(withIdentifier: .showWPComLogin, sender: self)
}
displayRemoteErrorForGoogle(error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the error displayed here always a Google error? There was previously a check for if awaitingGoogle... else – was the else case for non-Google errors?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see that the new method includes the else statement!

}

extension LoginPrologueViewController: GIDSignInDelegate {
open func sign(_ signIn: GIDSignIn?, didSignInFor user: GIDGoogleUser?, withError error: Error?) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we name this signIn, instead of sign?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method belongs to GIDSignInDelegate (the Google delegate), so... no. 😄 .

Copy link
Contributor

Choose a reason for hiding this comment

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

omg sorry 🙃 :homer-disappear:

@ScoutHarris ScoutHarris merged commit f36ea19 into develop Nov 27, 2019
@ScoutHarris ScoutHarris deleted the fix/13004-google_2fa_login branch November 27, 2019 19:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants