Refactor authentication logic. Fix re-authentication.

[bengtan]

This is not finished yet, but please consider it as an early preview for discussion and feedback.

Main problems

... were ...

1. Re-authentication when coming out of background

... doesn't work (with firebase logins) once the (short-lived) firebase token expires because there's nothing stored to remember that it was a firebase login. The app would send wocky.password to the server again, but wocky.password is insufficient since it only lasts for 1 hour (it's dependent on the firebase token duration).

2. Firebase login doesn't get a new access token

but the more I worked with the code, the more bits and pieces I noticed that needed tidying up. So I ended up doing a bit of a refactor.

Main changes

• Wocky now stores the type of 'login provider' ie. firebase or login and re-uses it to re-login after coming out of the background.
• There is a new interface ILoginProvider and a new BypassStore.
• BypassStore and FirebaseStore both 'register' with Wocky and Wocky loads them as needed in order to login, refresh token, or logout.
  • Instead of creating a new registry mechanism, I just reused the MST one.
• I inverted the calling order for logout. Instead of store.logout->firebase.logout->wocky.logout (even if it was a bypass login!), it's store.logout->wocky.logout->provider.logout where provider is either firebase or bypass.
• Renamed a bunch of .logout() calls to .onLogout().
• generateWockyToken() has been changed-a-lot/simplified.

but there are lots of small changes here and there.

Questions

(self.username = uuid())

Why is there a (self.username = uuid()) in wocky.login? I couldn't see a reason for assigning to self.username so I removed the assignment.

errorReporting

I don't really understand this in errorReporting:

      // TODO: figure out a more elegant way of "restarting" from scratch
      await wocky.logout()

so I tried to patch it up as best I could although the patching up introduced a yucky new Wocky.restart() function. Happy to properly fix this up if I understand it better, but I didn't want to go too far outside scope.

mockStore

mockStore.ts needed updating. Is there an automated way to update these tests? I dind't know of one so I hand-edited mockStore.ts.

wocky-client tests fail

They fail a lot because login doesn't work anymore but I can fix them up later.

Maybe I might have to move BypassStore into wocky-client, or maybe the whole idea of having BypassStore is flawed and I should just go back to the phoneNumber || accessToken (kludge-y?) trick.

[aksonov]

LGTM. Do you want to me to find out why login doesn't work or you can fix it ?

[aksonov]

mobxjs/mobx-state-tree#477 ?

[aksonov]

I don't know about automatic way to update mockStore, maybe @southerneer knows.

[aksonov]

Moving bypassStore to wocky-client looks good idea for me.

[bengtan]

I can do it. I'm not quite done with it yet. I still have some more tidy up I want to do. The important thing was to see what you guys think about the general idea. On 11 Jan. 2019 18:50, Pavel Aksonov <notifications@github.com> wrote:@aksonov approved this pull request.

…

LGTM. Do you want to me to find out why login doesn't work or you can fix it ?

[aksonov]

This looks a little bit hacky to me... There is no type control for mistakenly written providerName, for example. Also it creates new dependency between stores (Wocky depends from Firebase store). I prefer only one directional dependencies for simplicity (i.e. FirebaseStore uses Wocky, Wocky doesn't know anything about FirebaseStore)

So for me it is better to generate token from FirebaseStore/BypassStore and just pass token to Wocky. This way Wocky will not depend from any provider and will just require JWT token to login.

About uuid usage - I don't know why @southerneer used it, probably because it is required?

[bengtan]

This looks a little bit hacky to me... There is no type control for mistakenly written providerName,

I agree about 'hacky'. If someone changes providerName without knowing that it has to match an obscure line in index.ts, then that would break things. This is why I'd like to somehow have the providers auto-discover providerName. I'll see if I can find an improvement.

Also it creates new dependency between stores (Wocky depends from Firebase store). I prefer only one directional dependencies for simplicity (i.e. FirebaseStore uses Wocky, Wocky doesn't know anything about FirebaseStore)

Well, yes, and no. As it is written, Wocky doesn't know anything about FirebaseStore. You could take FirebaseStore out of the code, leave BypassStore in, and Wocky would happily use BypassStore without knowing what it was. Wocky just calls an interface as defined by ILoginProvider.

So for me it is better to generate token from FirebaseStore/BypassStore and just pass token to Wocky. This way Wocky will not depend from any provider and will just require JWT token to login.

The direction flow of information you describe is the same as what's in the PR. FirebaseStore/Bypass generates an object with {sub, phone_number} and gives it to Wocky and Wocky turns that data into a JWT. Previously, FirebaseStore/BypassStore passes either phoneNumber or accessToken. With this PR, the same data is passed, but in a cleaner form.

This way Wocky will not depend from any provider and will just require JWT token to login.

Also, Wocky cannot re-login with just the JWT token (as stored in .password) if it's from Firebase and it's expired. Wocky needs to call getLoginCredentials (or whatever it is named) everytime so that the login provider (specifically, firebase) can refresh it as necessary.

[southerneer]

Why is there a (self.username = uuid()) in wocky.login

Because it's required. It might help to review the authentication docs in the wiki.

[southerneer]

I don't really understand this in errorReporting:

Well it looks like errorReporting.js isn't imported anywhere else in the source code anyway. Unless you're using wocky.restart elsewhere I think you should revert those changes and just delete that file.

[southerneer]

I don't know about automatic way to update mockStore, maybe @southerneer knows.

I don't know of a way to automatically update it...just manual.

[southerneer]

More broadly, I think we should hold off on merging these changes until they can be properly tested with the app (i.e. after Pavel finishes the relationship changes in #3248). Unit tests are one thing, but covering background/foreground, disconnect/reconnect, new user vs existing, bypass vs firebase, etc is difficult to test without running in the app context.

And given the fundamental nature of these changes it would be good to have a test plan included with the PR (since a lot of cases won't be covered by unit tests).

[bengtan]

@southerneer:

Because it's required. It might help to review the authentication docs in the wiki.

I'm not talking about the jti field/claim which, I agree, is required.

I'm talking about assigning the value of the jti field to self.username. Why assign a value to self.username which is incorrect and/or will be overwritten soon anyways?

[bengtan]

I've addressed the most glaring criticism ... being the stability of the provider names. I still have to fix up the wocky-client tests, and some tidy-up left to do.

[southerneer]

Why assign a value to self.username which is incorrect and/or will be overwritten soon anyways?

@bengtan Ah sorry, I see what you mean. This may be a vestige of needing to play nice with XMPP. In the case of a new user we need to generate a unique ID (in which case the returned id after login will match), but I can't think of a reason now why we need to assign to username there before getting the return value from the backend.

[bengtan]

Okay, this is ready to do. I've done all the tidy-up that I want to. And so I've removed 'WIP' from the title.

The unit tests should now work just as well as before. I fixed it up to be able to login.

However, since the tests are currently broken for unrelated reasons ... Okay, let's wait for the new user relationship stuff to be finished first and then we'll merge from there onto here, and re-assess the state of the tests.

---

One of the tidy-ups I did was to delete .password from wocky and various places since it's not used anymore. We use tokens, not passwords. I think I found all the places from which .password could be deleted but ...

I found some interesting stuff in PersistableModel ... loadMinimal() and tryMigrate() both reference wocky.password and there was enough uncertainty in my mind that I didn't purge password from them. I don't know if they're still relevant anymore. Any thoughts?

[aksonov]

@bengtan These methods use password to re-authenticate user when schema is changed (so only minimal data is needed)

I'm worrying that we will have numerous conflicts with 'new roster' changes, what we should to do?

[bengtan]

These methods use password to re-authenticate user when schema is changed (so only minimal data is needed)

We don't use passwords to authenticate anymore. So ... I think ... these don't make sense anymore?

If the schema gets changed, firebase should (I think?) still treat the user as logged in without requiring a new sms code verification. I'll try to test this at some point before this gets merged.

If it's what I think, we can take password out of the schema, or maybe we don't need loadMinimal at all anymore.

I'm worrying that we will have numerous conflicts with 'new roster' changes, what we should to do?

Go ahead and do the 'new roster' changes. If needed, I'll rebase this on top of that. Manually, if I need to. The roster changes are more intrusive, so let's get that done first.

[bengtan]

Note: Please don't merge yet.

I may need to re-roll after the 'new user relationships' PR is merged.

[southerneer]

If it's what I think, we can take password out of the schema, or maybe we don't need loadMinimal at all anymore.

We definitely still need loadMinimal. It's a fallback for the case where a user has persisted the MST in one schema/shape, later opens the app after a code change (via a code Codepush, app store update, etc), and the app tries to "hydrate" the MST expecting a different schema/shape. Rather than try to account for all cases where this could cause an error (which would be way more trouble than it's worth), we instead load the minimal amount of cached data needed to prevent the user from being completely logged out on app open. I'm not very familiar with the changes in this PR, but it looks like minimal data would include sub and phoneNumber now (?)

@bengtan if that all makes sense maybe make the necessary changes, but given the scope of this PR currently it might be better to just create a ticket to do that work after this has been merged.

[bengtan]

it looks like minimal data would include sub and phoneNumber now (?)

But this is precisely the problem.

If I leave loadMinimal() alone, it will minimally re-hydrate password but password is actually our own JWT token which embeds a firebase access token.

If it minimally re-hydrates sub, sub is a firebase access token.

In both cases, the firebase access token only lasts for 1 hour.

So, ie.

• User uses the app, then stops using the app.
• More than 1 hour elapses.
• (Without opening the app) User upgrades to a newer version of the app.

password or sub would have expired and can no longer be used to login. The app would have to get a new firebase access token. So ... there's not much point re-hydrating them.

I'm going to test what firebase actually does in this scenario. The situation is very different now.

Also, I'm going to delay merging of this PR until after this week's release. It's a big change, and I don't want mind delaying it if it will give more time to review it.

[bengtan]

Just did some testing of loadMinimal().

If I open the debug menu and do a 'Clear Cache' in order to trigger a call to loadMinimal, I get ...

Possible Unhandled Promise Rejection (id: 0):
Error: [mobx-state-tree] Error while converting `{"firebaseStore":{},"locationStore":{},"searchStore":{},"profileValidationStore":{},"homeStore":{},"navStore":{},"onceStore":{},"wocky":{"host":"next.dev.tinyrobot.com"}}` to `MainStore`:

    at path "/codePushStore" value `undefined` is not assignable to type: `CodePushStore` (Value is not a plain object).
Error: [mobx-state-tree] Error while converting `{"firebaseStore":{},"locationStore":{},"searchStore":{},"profileValidationStore":{},"homeStore":{},"navStore":{},"onceStore":{},"wocky":{"host":"next.dev.tinyrobot.com"}}` to `MainStore`:

    at path "/codePushStore" value `undefined` is not assignable to type: `CodePushStore` (Value is not a plain object).

... which looks scary enough that I don't want to go into it right now.

So I'm going to rollback this PR by one commit and try again.

I'll do the purge of password and look into loadMinimal() another time.

[bengtan]

I merged master to this branch. Github was clever enough to do it without conflicts, and the tests now pass. So this is ready for review and merge.

[southerneer]

So loadMinimal is still necessary. The minimum amount of persisted data needed to rehydrate (based on this PR) is phoneNumber and providerName. If we try login without this data then it will fail.

As @aksonov mentioned earlier, the addition of LoginProvider and BypassStore seems overly complex. I've created a PR against this branch with some improvements.

[southerneer]

I inverted the calling order for logout.

Using firebase.logout even for bypass was a little confusing (but harmless), but I found the "inverted" calling order in this PR even more confusing. The client (rn-chat) should take of all it's logout logic separately from wocky-client (without wocky-client needing to call provider.logout). This is one area that my PR hopefully improves upon.

[bengtan]

Waiting for #3267