Multifactor Auth Support

[jleandroperez]

Description:

This pull request implements support for WordPress.com Multifactor Authentication.

Details:

• Legacy class WPcomLoginViewController has been nuked, in favor of LoginViewController.
• Added service call to retrieve One Time Code via SMS.
• LoginViewController major overhaul.
• JetpackSettingsViewController has been also enhanced to support Multifactor Authentication.

Stats:

• WPAnalyticsStatSignedIn and WPAnalyticsStatLoginFailed both now have an extra property: multifactor
• WPAnalyticsStatTwoFactorCodeRequested is hit whenever a Two Factor Code is requested.
• WPAnalyticsStatTwoFactorSentSMS is hit when the user taps over the Send code over SMS button.
• WPAnalyticsStatOpenedSupport is not being tracked whenever the Help is opened.

Notes:

• Both LoginViewController and JetpackSettingsViewController share roughly the same functionality (and the UI is pretty similar). For the sake of preserving the mental health of the reviewer (and committer), i've added Issue Merge LoginViewController and JetpackSettingsViewController #3367 to tackle that issue.

Closes #3223

Testing Scenarios:

Below, we'll attempt to cover every possible SignIn flow. Please, feel free to add anything that might be missing. Thank you!

Scenario 1: Dotcom Multifactor Disabled

1. Fresh install WPiOS
2. Log into a WordPress.com with no MFA settings

Expected Results:

• The user should be logged into his WordPress.com account

#### Scenario 2: Dotcom Multifactor Enabled and One Time Code 1. Fresh install WPiOS 2. Enter the Username / Password of a DotCom account with 2FA enabled

Expected Results:

• The app should request the One Time code.
• After entering the Code, the user should be logged into WordPress.com

#### Scenario 3: Dotcom Multifactor Enabled and One Time Code via SMS 1. Fresh install WPiOS 2. Enter the Username / Password of a DotCom account with 2FA enabled 3. When visible, tap over the `Send one time code via Text Message` button. 4. Once the SMS has been received, proceed entering the code.

Expected Results:

• The user should be logged into his WordPress.com account

#### Scenario 4: Dotcom Multifactor Enabled and App Password 1. Fresh install WPiOS 2. Enter the Username + Application Specific Password

Expected Results:

• The app should log you in, and it should not request for the One Time Password

#### Scenario 5: Dotcom as "Self Hosted" 1. Fresh install WPiOS 2. Tap over `Add Self-Hosted Site` 3. In the `Site URL` enter WordPress.com 4. Enter your WordPress.com credentials

NOTE:

• This test scenario is a variant of flows 1..4. Please, repeat all of them, but make sure to perform the extra step of specifying WordPress.com as an endpoint.

Expected Results:

• The user should be logged into his WordPress.com account

#### Scenario 6: Self Hosted with no Jetpack 1. Fresh install WPiOS 2. Tap over `Add Self-Hosted Site` 3. Log into a Self Hosted WordPress blog with Jetpack disabled / unavailable

Expected Results:

• The user should be logged into his Self Hosted site.

#### Scenario 7: Self Hosted with Jetpack 1. Fresh install WPiOS 2. Tap over `Add Self-Hosted Site` 3. Log into a Self Hosted WordPress blog with Jetpack Enabled

NOTE:

• Jetpack Connect screen should behave in the exact same way as a regular DotCom signup. Since this is a variant of flows 1..4, please, make sure that Jetpack Login pass'ess those tests as well.

Expected Results:

• The user is logged into his both, his Self Hosted blog, and into his WordPress.com account.

#### Scenario 8: Re-Login Flow 1. WPiOS: Fresh install the app 2. WPiOS: Log into a WordPress.com account 3. WordPress.com: Log into the same account as in step 2 4. WordPress.com: Open Account Settings > Security 5. WordPress.com: In the 'Connected Applications', remove the connection for WordPress iOS App 6. WPiOS: In My Sites tab, try opening the Posts / Comments

Expected Results:

• The LoginViewController screen should be onscreen, with the username already pre-filled

#### Scenario 9: Connect Dotcom with Self Hosted already wired (Me tab) 1. WPiOS: Fresh install the app 2. WPiOS: Log into a Self Hosted blog 3. Skip Jetpack Setup 4. Tap over `Me` tab, and press `Connect to WordPress.com Account`

Expected Results:

• The LoginViewController screen should be onscreen.
• The user should Only be allowed to sign into a WordPress.com account

#### Scenario 10: Connect Dotcom with Self Hosted already wired (My Sites tab) 1. WPiOS: Fresh install the app 2. WPiOS: Log into a Self Hosted blog 3. Skip Jetpack Setup 4. Tap over `My Sites` tab, and press `Add a Site`

Expected Results:

• The LoginViewController screen should be onscreen.
• The user should be allowed to add either a WordPress.com account, or a self hosted blog

#### Scenario 11: Add Self Hosted Blog 1. WPiOS: Fresh install the app 2. WPiOS: Log into a WordPress.com account 3. Tap over `My Sites` tab, and press `Add a Site`

Expected Results:

• The LoginViewController screen should be onscreen.
• The user should be allowed to ONLY add a Self Hosted WordPress.com blog

[jleandroperez]

I'm hereby invoking @diegoreymendez + @astralbodies + @aerych for a review.

I promise i'll do my best, next time, to cut this into smaller PR's, so i don't drive you crazy. Again.

All of the possible login flows have been identified + documented up there. We'll be addressing code duplication between LoginViewController and JetpackSettingsViewController in #3367.

Thanks in advance !!

[aerych]

Would it make sense to use the term "Path" instead of "URL" for consistency with other NSString path methods?

[aerych]

Great candidate for a unit test :D

[diegoreymendez]

I second that. 👍

[jleandroperez]

Updating + adding test. Thank you!

[aerych]

So many changes :)
Seeing places where ivars are used instead of accessors, calls to NSLocalizedString without a description, integers assigned to props defined as floats, etc., but for the most part its how it was in the original code. I'm struggling a bit with how nitpicky to be since a lot of this isn't new code so much as its shuffling some existing code, and I'm thinking I want to focus on the multifactor support and pay less attention to previous code style faux pas. Maybe we can open an issue to scrub code style later?

[diegoreymendez]

@aerych, @jleandroperez - Just chiming in following what we discussed with @jleandroperez and following @aerych's comment above. I wanted to document the discussion behind the scenes.

I'd try to avoid going too deep into style/format checking the old code, since this PR could take ages to wrap up, due to its size and complexity.

IMHO, we gotta start dividing functional changes from style or formatting improvements, even though it can be tempting to mix them together. To the point where I'm inclined to propose we start rejecting PRs that fail to do so.

My opinion for this PR would be to try and assess it does what it intends to do, so we can merge it sooner and have more people testing it.

Thoughts on my comments above?

[jleandroperez]

@aerych + @diegoreymendez to be 100% honest with you... i held myself on many of the missing spots there (ivars being accessed directly // so on), because the code delta was huuuuuge.

I'll be (trying to) kill JetpackSettingsViewController in #3367 + nuke manual layout code, plus, perhaps, that'd be a good opportunity to address the missing fixes (ivars, localized strings, so on!).

[aerych]

Spent some time reading through the changes and tested scenarios 1, 2, 4, 5, 6, 9, 10, 11. Looking good so far. Will take it for another spin later and review the remaining scenarios.

[diegoreymendez]

@jleandroperez - Tested all cases, and they are working fine for me.

From the usability standpoint though I think this case looks weird (it was already there in the old code):

• Fresh install
• Log into your wordpress.com account
• Go to My Sites -> Add Site
• Log into any wordpress.com account

Expected behavior:

• An error should be displayed

Current behavior:

• No error is shown.

[diegoreymendez]

Btw - I suggest to wait for @aerych's approval too.

[jleandroperez]

@diegoreymendez + @aerych: isWordPressComPath helper added + Unit Test in place.

Thanks!

[aerych]

At the risk of piling on, and also cos @astralbodies's regex hate got me to thinking ;), maybe using NSURLComponents is enough. For instance, [components.scheme hasPrefix:@"http"] is probably sufficient for validating the protocol and [components.host hasSuffix:@wordpress.com"] looks like its sufficient for identifying a wpcom path. Thoughts?

[jleandroperez]

@aerych updated again, so that we support scenarios like this:

http(s):*.wordpress.com OR http(s)://wordpress.com

Thanks Eric!

[aerych]

Sorry @jleandroperez, I consistently crash signing in via scenario 7 in the isWordPressComPath regex check when trying to add "wpt.koke.me" to the app.
https://gist.github.com/aerych/b08331bdfd29f49d6a77

[jleandroperez]

@aerych crash should be gone for good, thanks Eric!

[aerych]

Looks like this was the cause:

(lldb) po components
<NSURLComponents 0x7fdb51d3ea80> {scheme = (null), user = (null), password = (null), host = (null), port = (null), path = wpt.koke.me, query = (null), fragment = (null)}

I entered just the domain for the site. NSURLComponents is interpreting it as the path, not the host, so nil is being passed to the regex check.

[aerych]

Ha! You beat me by 2 seconds :) I'll test the update.

[jleandroperez]

@aerych thanks for spotting the trailing slash + missing protocol glitches!. All should be good now.

Thank you!

[aerych]

💥

[jleandroperez]

@aerych + @diegoreymendez thank you both!