Page MenuHomePhabricator

[native] Auth with authoritative keyserver directly from registration flow
ClosedPublic

Authored by ashoat on Wed, Apr 24, 12:41 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, May 2, 11:53 PM
Unknown Object (File)
Thu, May 2, 11:32 PM
Unknown Object (File)
Thu, May 2, 8:02 PM
Unknown Object (File)
Thu, May 2, 2:23 PM
Unknown Object (File)
Tue, Apr 30, 8:17 PM
Unknown Object (File)
Tue, Apr 30, 6:05 PM
Unknown Object (File)
Tue, Apr 30, 2:12 PM
Unknown Object (File)
Tue, Apr 30, 11:54 AM
Subscribers
None

Details

Summary

In an earlier diff, we extracted useKeyserverAuth out of KeyserverConnectionHandler. In this diff, we call it directly from the registration flow.

This addresses ENG-7669.

Depends on D11762

Test Plan

This whole stack was tested with the following steps:

  1. On native, attempting to log in with an ETH account that hasn’t been registered yet
  2. On native, attemping to register with an ETH account that has already been registered
  3. On native, log in with an ETH account that has already been registered
  4. On native, register with an ETH account that hasn’t already been registered
  5. On native, register with a password account
  6. On native, log in with a password account
  7. On web, attempting to log in with an ETH account that hasn’t been registered yet
  8. On web, log in with an ETH account that has already been registered
  9. On web, log in with a password account

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

native/account/registration/registration-server-call.js
288 ↗(On Diff #39449)

Since setCurrentStep(inactiveStep) is not called here, does that mean that it is not possible to retry identity registration?

317 ↗(On Diff #39449)

At this point, we are registered with identity. Can we name this step differently? 'identity_registration' maybe? The waiting_for really had me thinking for a second that we are not registered yet at this point.
We could also add a step 'identity_registration_concluded' and check that instead of isRegisteredOnIdentity. But I think it's better that we check if CSAT and user info are actually set.

342–343 ↗(On Diff #39449)

What happens if this runs? We are logged in with identity, but not the keyserver. Will a recovery login be triggered? Will the user be logged out of identity?

native/account/registration/registration-server-call.js
288 ↗(On Diff #39449)

No. Line 212 guarantees that if we're entering this code block, currentStep is already inactiveStep. There's no need to change it

native/account/registration/registration-server-call.js
317 ↗(On Diff #39449)

When we set currentStep.step to waiting_for_identity_registration on line 281, the request to the identity service has already concluded, but we are still waiting for the data to be updated in Redux as a result of the corresponding Redux action.

A more accurate name would be waiting_for_identity_registration_data_in_redux or something, but that's a bit of a mouthful.

I don't think I like identity_registration. That implies that the step we are currently on is identity registration, but actually during that step we are performing keyserver auth.

We could also add a step 'identity_registration_concluded' and check that instead of isRegisteredOnIdentity.

I am not sure what you are suggesting here. We already checking both the step and isRegisteredOnIdentity. It appears you are suggesting only checking the step, but I don't see what benefit that would have, or how it would address your concerns about naming.

342–343 ↗(On Diff #39449)

Handling this correctly is tracked in ENG-7775. In that task, this code will be updated to call deleteAccount to clear the data on the identity service and locally.

Will a recovery login be triggered?

Even without ENG-7775, I don't think this would happen. Generally a recovery login has to be triggered by a session invalidation coming from the keyserver, either via a REST request or via the socket.

Will the user be logged out of identity?

Not currently, but in ENG-7775 the deleteAccount call will cause the user to be logged out of identity.

native/account/registration/registration-server-call.js
317 ↗(On Diff #39449)

An alternative would be to have inactive, then authoritative_keyserver_registration, and then setting_avatar. I think it's a bit weird that we don't have identity_registration in the list, though. We could rename inactive to identity_registration, but then it's weird that we're in an identity_registration step for most of the lifetime of the app (both before and after the button is clicked), even though we're not actually doing identity registration.

I'd like to avoid adding fake steps here... the naming should not affect the logic. Open to other suggestions on the naming, but also don't know if it matters all that much. I can also use the long name waiting_for_identity_registration_data_in_redux if you'd like.

Suffix step names with _in_redux

inka added inline comments.
native/account/registration/registration-server-call.js
288 ↗(On Diff #39449)

Oh, because we only set the step to waiting_for_identity_registration_in_redux after everything has succeeded. I see, thank you

317 ↗(On Diff #39449)

waiting_for_identity_registration_in_redux is still incorrect at this point here, because if isRegisteredOnIdentity is true, then the redux must have been updated.

I am not sure what you are suggesting here. We already checking both the step and isRegisteredOnIdentity. It appears you are suggesting only checking the step, but I don't see what benefit that would have, or how it would address your concerns about naming.

I was suggesting adding another step - once redux data is updated, we could change the step from waiting_for_identity_registration to identity_registration_concluded. But if we don't want more steps then that doesn't help.

We set the step names after we dispatch identity register and keyserver register respectively, so I think we could use inactive, identity_registration_called, keyserver_registration_called (or dispatched). But if you don't like that either, then I think we can leave it as is. It's not very important, it just seems confusing, but not worth spending more time on I think.

This revision is now accepted and ready to land.Fri, Apr 26, 5:38 AM

waiting_for_identity_registration_in_redux -> identity_registration_dispatched
waiting_for_authoritative_keyserver_registration_in_redux -> authoritative_keyserver_registration_dispatched

This revision was landed with ongoing or failed builds.Fri, Apr 26, 8:18 AM
This revision was automatically updated to reflect the committed changes.

waiting_for_identity_registration_in_redux -> identity_registration_dispatched
waiting_for_authoritative_keyserver_registration_in_redux -> authoritative_keyserver_registration_dispatched

Thank you!