Gutenboarding: Add push 2fa support to auth store #40114

p-jackson · 2020-03-13T08:34:38Z

Changes proposed in this Pull Request

Add support for "push" 2nd login factor
Polls ever 5 seconds to see if push notification has been handed (this is the poll time currently used by /log-in)
Add downlevelIteration to TS config so we can use yield* in generator functions
Every time we poll update the nonce that we'll use for the next time we poll

"Push" means when we send a push notification to the WordPress app during login, and you click the button in the app to approve the login. Not the one where you have to end a code for the 2nd factor.

Testing instructions

Checkout branch locally
Install WordPress app on phone (I found I needed to enable notifications for the app, can always disable later 😄)
In console:
- wp.auth.submitUsernameOrEmail('username') using an account that's logged into the mobile app
- wp.auth.submitPassword('correctpassword')
- Console will print that it's now waiting for 2FA
- Go to your phone and approve the push notifcation
- After a few seconds the console will update to say you're logged in
- Also try running wp.auth.reset() while polling is happening, polling should stop

Should probably try a passwordless account with 2fa. I don't actually know if passwordless accounts can have a second factor 🤔 (we might ask them to pick a password)

matticbot · 2020-03-13T08:34:40Z

Test live: https://calypso.live/?branch=add/auth-store-2fa-app

matticbot · 2020-03-13T08:46:10Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~779 bytes added 📈 [gzipped])

name                   parsed_size           gzip_size
entry-gutenboarding        +2427 B  (+0.1%)     +630 B  (+0.1%)
entry-main                  +202 B  (+0.0%)      +49 B  (+0.0%)
entry-login                 +202 B  (+0.0%)      +50 B  (+0.0%)
entry-domains-landing       +202 B  (+0.0%)      +50 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

p-jackson · 2020-03-17T05:36:46Z

Should probably try a passwordless account with 2fa. I don't actually know if passwordless accounts can have a second factor 🤔 (we might ask them to pick a password)

Turns out passwordless account can have a second factor! It won't impact gutenboarding though because you enter the 2nd factor after following the email link, so that's done at /log-in

roo2

I couldn't test it with the app on my phone (I got no google services 😅). I had a comment on the style of reusing variables and the login response type which I think should be a union type, and also error handling.

roo2 · 2020-03-18T00:55:25Z

packages/data-stores/src/auth/actions.ts

+			if ( ! response.success ) {
+				two_step_nonce = response.data.two_step_nonce;
+				yield wait( POLL_APP_PUSH_INTERVAL_SECONDS * 1000 );
+			}


I think we should have a timeout here, I'm not sure what length of time it should be. Is this code called as we wait for the user to acknowledge the push notification on their phone?

Agree. I don't the current login has a timeout but it really should. It does handle errors by stopping the polling, and we should too.

packages/data-stores/src/auth/actions.ts

packages/data-stores/src/auth/types.ts

packages/data-stores/src/auth/actions.ts

roo2 · 2020-03-18T02:20:35Z

packages/data-stores/src/auth/actions.ts

+				two_step_push_token: push_web_token,
+			} );
+
+			if ( ! response.success ) {


There should also be error cases which indicate that the push authentication has really failed an will never succeed. I think you should be more specific with this check and then handle any other error's seperately

Agree, I reckon this goes hand in hand with timing out too.

andrewserong · 2020-03-18T07:11:34Z

Nice work so far @p-jackson! Following the steps in the testing instructions worked well for me.

I ran into an issue attempting to test when something doesn't quite work out right, like if a user doesn't have notifications switched on in the app, or accidentally clicks ignore, so we need to submit the password again.

The first time I attempted this, I appeared to get stuck in a situation where there were two lots of the while loop running checking the two-step-authentication-endpoint. So after I tapped Approve in the app, the other loop was still throwing a 403.

The steps to do this were:

I had the WordPress app open but notifications weren't enabled
I did wp.auth.submitPassword( myPassword );, realised that I didn't have the push notifications enabled so then
I switched on notifications
Called wp.auth.submitPassword( myPassword ); again
Tapped Approve in the app
Noticed the additional network requests firing in the Network tab in Chrome

I attempted to recreate these steps, but encountered slightly different behaviour. I tapped "Ignore" after the first password submit, and then after the second password submit, the network requests for checking the two-step-authentication-endpoint threw 400 errors instead of 403. The error response looks like:

{"success":false,"data":{"errors":[{"code":"empty_two_step_nonce","message":"two_step_nonce required."}]}}

Inspecting the network request that generated the 400, the request payload was missing the two_step_nonce param after the second submitPassword.

I've run out of time for today to investigate further, but I'm wondering if in the while loop it'd be worth trying to explore how it can check the loginFlowState as a means of breaking out of the endless loop? I'm not sure it needs a timeout, but would a mechanism for stopping the check / turning the check into kind of a singleton behaviour work?

This behaviour might be a little artificial of course, because we might handle things quite differently in the consuming code. Taking a look at the existing behaviour at /log-in if I get to the push notification screen with the app open, it asks me to verify in the app. If I click back, I get taken to the input field to enter my email address (not the password field). After entering my password, I get redirected to /log-in/sms. So the behaviour of dealing with the push notification appears to be closely tied to dealing with the other 2fa approaches, too 🤔

I'm sure you're well-aware of most of these complexities anyway, so apologies for the info dump! Happy to take a closer look again tomorrow.

p-jackson · 2020-03-19T03:52:36Z

I've updated polling logic so if there's an API error the polling will stop and return the user to the password state. However if it's a network error (i.e. fetch throws) then it keeps polling. This copies what the current log in does, I think it makes sense that having a shoddy connection doesn't stop the polling.

I decided not to add a timeout for polling. How long is long enough? Does it take a user 5min to find their phone? 10min? Instead I've made sure the reset() action will stop the polling. That way if the user wants to go back and start login again they can.

ramonjd · 2020-03-19T04:40:04Z

Just tested and works as described for me.

How long is long enough?

I clicked on Ignore in my app, just to see what would happen.

It's hard to guesstimate how long is appropriate. Sometimes I'm fumbling around with my phone and it takes me 20 seconds just to unlock the thing.

Would it make sense to give them, say, 1 min, after which we could offer someway to resend the permission request?

p-jackson · 2020-03-22T20:53:48Z

Would it make sense to give them, say, 1 min, after which we could offer someway to resend the permission request?

Yeah I was thinking something like this. The current UI has links like "I don't have access to my phone right now". So I think these sorts of things should actually be dealt with at the UI layer not automatically in the store.

ramonjd · 2020-03-26T22:36:14Z

client/landing/gutenboarding/devtools.ts

@@ -27,21 +27,30 @@ export const setupWpDataDebug = () => {
 				Site.register( clientCreds );

 				window.wp.auth = {};
+				let previousState = window.wp?.data.select( AUTH_STORE ).getLoginFlowState();


A question, because I'm not sure...

Does the optional chaining inwindow.wp? makes sense seeing as, in the line above we're assuming that wp is a property of window anyway? (window.wp.auth = {};)

Eagle eyes! No they're not needed, just a copy paste bug from when I copied code from inside the callback.

It is needed inside the callback because window.wp could be undefined again by the time the callback is called (according to the type checker). Given it's just devtools it's a case of do-what-you-need-to-do-to-shut-the-type-checker-up

andrewserong

This is working great for me now @p-jackson, I really like the elegant approach of handling both the wait and cancelling an existing poll via task ids. From testing in the console, it correctly errors out and stops polling when push authentication has been throttled, too.

Just left a couple of notes of tiny typos, but once they're fixed up, it's looking good to go to me!

packages/data-stores/src/auth/actions.ts

packages/data-stores/src/wpcom-request-controls/index.ts

Fixed typos

p-jackson · 2020-03-27T01:42:52Z

Thanks for the reviews. Merging to wrap up this round of the auth store work.

andrewserong

Nice work on all this! 🚢

p-jackson self-assigned this Mar 13, 2020

p-jackson added [Goal] New Onboarding previously called Gutenboarding [Status] In Progress API Data labels Mar 13, 2020

p-jackson changed the base branch from update/add-wpcom-proxy-request-branch to master March 17, 2020 04:52

p-jackson force-pushed the add/auth-store-2fa-app branch from 923ffe6 to 027010d Compare March 17, 2020 04:52

p-jackson marked this pull request as ready for review March 17, 2020 05:28

p-jackson requested a review from a team as a code owner March 17, 2020 05:28

p-jackson requested a review from a team March 17, 2020 05:29

roo2 reviewed Mar 18, 2020

View reviewed changes

p-jackson changed the title ~~WIP: Add push 2fa support to auth store~~ Add push 2fa support to auth store Mar 23, 2020

p-jackson added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Mar 23, 2020

p-jackson changed the title ~~Add push 2fa support to auth store~~ Gutenboarding: Add push 2fa support to auth store Mar 23, 2020

p-jackson added 6 commits March 26, 2020 21:13

WIP: Add push 2fa support to auth store

bb94098

Poll wp-login.php till push notification has been handled

be503bd

devtools watch auth store go through phases

abafab0

Stop 2fa polling if there's an error or the flow is reset

5c3dc32

Test that polling handles errors

c7d9b38

Return to password state if 2fa polling errors

9a56f30

p-jackson force-pushed the add/auth-store-2fa-app branch from 0aab026 to 9a56f30 Compare March 26, 2020 08:26

p-jackson requested a review from a team March 26, 2020 08:27

ramonjd reviewed Mar 26, 2020

View reviewed changes

Remove unnecessary optional chaining.

2db29ec

andrewserong previously requested changes Mar 27, 2020

View reviewed changes

packages/data-stores/src/auth/actions.ts Outdated Show resolved Hide resolved

packages/data-stores/src/auth/actions.ts Outdated Show resolved Hide resolved

packages/data-stores/src/wpcom-request-controls/index.ts Show resolved Hide resolved

Typos

f348704

andrewserong approved these changes Mar 27, 2020

View reviewed changes

p-jackson merged commit 3139a7f into master Mar 27, 2020

p-jackson deleted the add/auth-store-2fa-app branch March 27, 2020 01:45

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gutenboarding: Add push 2fa support to auth store #40114

Gutenboarding: Add push 2fa support to auth store #40114

p-jackson commented Mar 13, 2020 •

edited

Loading

matticbot commented Mar 13, 2020

matticbot commented Mar 13, 2020 •

edited

Loading

p-jackson commented Mar 17, 2020

roo2 left a comment

roo2 Mar 18, 2020

p-jackson Mar 18, 2020

roo2 Mar 18, 2020

p-jackson Mar 18, 2020

andrewserong commented Mar 18, 2020

p-jackson commented Mar 19, 2020

ramonjd commented Mar 19, 2020

p-jackson commented Mar 22, 2020

ramonjd Mar 26, 2020

p-jackson Mar 26, 2020

andrewserong left a comment

p-jackson commented Mar 27, 2020

andrewserong left a comment

Gutenboarding: Add push 2fa support to auth store #40114

Gutenboarding: Add push 2fa support to auth store #40114

Conversation

p-jackson commented Mar 13, 2020 • edited Loading

Changes proposed in this Pull Request

Testing instructions

matticbot commented Mar 13, 2020

matticbot commented Mar 13, 2020 • edited Loading

p-jackson commented Mar 17, 2020

roo2 left a comment

Choose a reason for hiding this comment

roo2 Mar 18, 2020

Choose a reason for hiding this comment

p-jackson Mar 18, 2020

Choose a reason for hiding this comment

roo2 Mar 18, 2020

Choose a reason for hiding this comment

p-jackson Mar 18, 2020

Choose a reason for hiding this comment

andrewserong commented Mar 18, 2020

p-jackson commented Mar 19, 2020

ramonjd commented Mar 19, 2020

p-jackson commented Mar 22, 2020

ramonjd Mar 26, 2020

Choose a reason for hiding this comment

p-jackson Mar 26, 2020

Choose a reason for hiding this comment

andrewserong left a comment

Choose a reason for hiding this comment

p-jackson commented Mar 27, 2020

andrewserong left a comment

Choose a reason for hiding this comment

p-jackson commented Mar 13, 2020 •

edited

Loading

matticbot commented Mar 13, 2020 •

edited

Loading