Skip to content

Login: Add 2FA support for social login - #8010

Merged
nheagy merged 7 commits into
developfrom
feature/7675-2fa
Oct 20, 2017
Merged

Login: Add 2FA support for social login#8010
nheagy merged 7 commits into
developfrom
feature/7675-2fa

Conversation

@nheagy

@nheagy nheagy commented Oct 18, 2017

Copy link
Copy Markdown
Contributor

Refs #7675

This covers the scenario in which the user's account is already associated with Google, and their WordPress.com account requires 2FA.

To test:

  • Setup a wpcom account to be associated with Google, and require 2FA
  • Attempt to login to the app using Google
  • When prompted for a 2FA token cover these scenarios:
    • using app/generator token
    • using SMS token
    • using backup token
    • using an incorrect of the above, then the real one
    • using an incorrect of another sort, then a real one of a different kind (sms first, backup second, etc)

Needs review: @aerych

@nheagy nheagy added the Login label Oct 18, 2017
@nheagy nheagy added this to the 8.7 milestone Oct 18, 2017
@nheagy nheagy self-assigned this Oct 18, 2017
@nheagy
nheagy requested a review from aerych October 18, 2017 20:20

@aerych aerych left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Heya @nheagy! This is a really nice PR. :)
I had a few nitpicky comments after looking over the changes.
Will loop back after I finish with the testing.

guard let _ = loginFields.nonceInfo else {
return
}
sendCodeButton.isHidden = true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe a bit pedantic, but since this isn't related to a text field maybe create a new method so there is harmony between the method name and the work being done?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is a good idea.

if let newNonce = (error as NSError).userInfo[WordPressComOAuthClient.WordPressComOAuthErrorNewNonceKey] as? String {
loginFields.nonceInfo?.updateNonce(with: newNonce)
}
displayError(message: NSLocalizedString("Whoops, that's not a valid two-factor verification code. Double-check your code and try again!",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we could define the error string in a let before the if block to avoid some duplication?

static let lastUsedPlaceholder = "last_used_placeholder"
}

private enum AuthTypeLengths {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For extra clarity it might be nice to add a comment explaining these match the server-side constants.

@aerych

aerych commented Oct 19, 2017

Copy link
Copy Markdown
Contributor

simulator screen shot - iphone 6s - 2017-10-18 at 19 26 11

Looks like hiding the sms button changes the next button's layout.

@nheagy

nheagy commented Oct 19, 2017

Copy link
Copy Markdown
Contributor Author

I looked at that button dozens of times and didn't notice that haha.

@nheagy

nheagy commented Oct 19, 2017

Copy link
Copy Markdown
Contributor Author

Ok, @aerych ready for round 2!

@nheagy nheagy mentioned this pull request Oct 19, 2017
17 tasks
@nheagy

nheagy commented Oct 20, 2017

Copy link
Copy Markdown
Contributor Author

@aerych missed the commit for the new comment. Added!

@aerych

aerych commented Oct 20, 2017

Copy link
Copy Markdown
Contributor

Awesome! :shipit: once BB is green!

@nheagy
nheagy merged commit c773521 into develop Oct 20, 2017
@nheagy

nheagy commented Oct 20, 2017

Copy link
Copy Markdown
Contributor Author

Thanks @aerych!

@nheagy
nheagy deleted the feature/7675-2fa branch December 20, 2017 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants