Refactor user

[sielay]

• This is proof of concept for Discuss: Thoughts about identity handling #337.
• It contain fix for OAuth - not supported refresh case #288
• It partially address OAuth - not supported refresh case #288
• It allows multiple emails and usernames
• It keeps back compatibility

It moves all identities to collection in user object. It keeps password and salt shared.

Tested against Facebook and GitHub, unit tests added. Documentation required.

Current user structure

{
     _id : ObjectID
     displayName : "My name",
     salt: "<SALT>",
     password: "<HASHED_PASSWORD>",
     identities : [ {
          provider : UserModel.EMAIL,
          id: "test@test.com"
     }, {
          provider : UserModel.USERNAME,
          id : "Tony Stark"
     }, {
          provider : "github",
          id : "<ID>",
          accessToken : "<ACCESS_TOKEN>",
          refreshToken: "<REFRESH_TOKEN>",
          providerData : Object
     } ]
}

Useful methods:

user.getIdentity('slack');
OR
user.getIdentity('facebook', '<SECOND_ID>');

Previously you needed multiple lines to find if that was user.providerData or user.additionalProviderData[provider].

Hope you like it.

[ilanbiala]

I actually think a Map would be more suitable, because the array is less informative and requires virtuals on the server and custom methods on the client...

identities: {
  google: {
    id: "test@gmail.com",
    password: "abc123" // plain text just for simplicity
  },
  facebook: {
    id: "7778889999",
    password: "abc123"
  }
}

With this one you can just do user.identities.google.id. Wouldn't that be more convenient than having to do user.identities[0].id? I'm assuming that google exists, because in both cases you have to check for it, but still checking for the existence in an object is as simple as if (user.identities.google), it reads cleaner than a for loop.

[sielay]

Yes, but that works fine when you assume there is one identity of type per user. What may be not case. Some apps will look for np connecting multiple twitter account. Also how will we keep same structure for emails, allowing having multiple email addresses at the same time? Array base is not perfect, but most flexible. Proven in many previous projects.

[ilanbiala]

@sielay I did forget about multiple accounts for the same provider, but there you could probably get around that by using part of the provider data, no? I'm just trying to make it easier and and cleaner.

Any thoughts on possible keys inside the provider object? So using the email/username like user.identities.google['abc@gmail.com'] would be one possibility. Are there better ones?

[sielay]

Got your point. Wonder about JSON compatibility and how mongo will behave. Some implementations complain about special characters in keys. Let's wait for @roieki and others input. I can then align to whatever form will work for you. Can also create nice virtuals to help (current are for keeping back compatibility). As you see I can code fast :) Just want to code think that will work for all ;)

[rschwabco]

Sorry guys - I won't be able to go over this in the coming week - @amoshaviv @NeverOddOrEven @lirantal - could you please take a look?

[NeverOddOrEven]

There was a discussion about this in a different now-closed issue: #290

Other issues and links:
Original Issue: #288
Discussion: #337

Resume conversation here please. I will review once I have some availability.

[sielay]

Thanks. @NeverOddOrEven . I will do small cleanup and refactor once we all agree on arrayVSobject structure. Before that no detailed checkup has sense.

[ilanbiala]

@lirantal I think object is easier to work with if we can figure out a good structure, no?

[sielay]

I made some extra fixes to bugs I figured that in production project. I will update PR and test object. I am afraid of special chars in keys, need to see how mongo behaves. Will test and we'll see.

[ilanbiala]

@sielay sounds good, can't wait to see the changes.

[sielay]

Hi. I checked. Object structure would be in conflict with Mongo https://jira.mongodb.org/browse/SERVER-3229?jql=labels%20%3D%20syntax so doing that brings risks (even if by some chance it would work) @ilanbiala

[sielay]

Anyhow I added support for multiple emails to settings page. I leave it at the moment to review, need to go to project which I base on this PR (second, first http://pulzair.com is kind of live ;)). Feel free to discuss.

[ilanbiala]

@sielay there have been a couple changes in the user controller and a couple more will probably be merged in before this PR just because of they are smaller and simpler. @lirantal @NeverOddOrEven @roieki @amoshaviv what do you guys think of the current PR?

[sielay]

@ilanbiala no problem. I will rebase commit and retest it after those changes, just give me note when. I have to maintain this fork as I have projects basing on such structure.

[ilanbiala]

Can you revert these whitespace changes?

[ilanbiala]

@sielay sorry it took so long, just added some comments. You might want to run it through JSHint to properly format everything to be consistent with the rest of MEAN.js. Can you show some example code detailing how this works now?

[sielay]

@ilanbiala likely I will share something. Pulzair.com is using that structure so far.

[ilanbiala]

@sielay does this account for unique email support like in #439?

[lirantal]

Yeah, I think it's "safer" for when we want to merge 0.4 into master.

[wesleyfsmith]

I agree with @lirantal, seems like it will be a lot easier to make this change part of migration to 0.4, instead of throwing it in with all of the 0.3 changes currently.

[ilanbiala]

@sielay Is it possible for you to recreate this PR for 0.4.0 so that we can merge it into master much easier later on?

[sielay]

@ilanbiala I currently have no time to update it, but each time I use my fork I merge it with your changes. Once you release 0.4.0 I will simply update it and retest especially I already found some issues after update.

[ilanbiala]

@sielay we want to have the refactored user setup ready for the 0.4.0 release, so is there any way you can create a PR in the next few days with this same code for the 0.4.0 branch?

[ilanbiala]

@sielay also if you can squash the commits down to 1 when you create a new PR against 0.4.0, that would be awesome.

[britztopher]

@sielay any update on a PR for the 0.4 branch. I am trying to get auth token ticket finished, but need to know how authorization is going to be reworked in 0.4 branch before I can continue

[sielay]

@britztopher I am not able to pick it up now. Anyhow current PR works in main. I prefer you go with your stuff and then I re-align PR, or anyone else pick up the change and to it in own flavour.

[britztopher]

@sielay I just think that my token based stuff depends a ton on the authorization process. If authorization changes it will cause a major headache to get working with the change.

[ilanbiala]

Yes the token change is pretty heavily based on the user implementation. If @sielay can't make the changes now, maybe @britztopher can fork and we'll help him finish the refactor? @lirantal suggestions for moving this along.

[britztopher]

@ilanbiala I can try to fork repo, create feature branch, then try to merge his changes into 0.4.0. Also, doing this merge will also let me learn the changes from old auth to new making #389 easier to conceptualize

[britztopher]

@sielay how were you testing your PR in master?

[britztopher]

@sielay, did you say you were using this code already in your application? I only ask because I just forked your repo and checked out refactor_user branch and its throwing a bunch of errors when I try to login using google. Am I missing something?

@ilanbiala I have merged #343 into the 0.4.0 branch, however, things are not working like they should, so I am currently debugging them.

[sielay]

@britztopher version in this PR started to fail after some updates in dependencies. I am using version which wasn't applied into this PR. I run tests as they are set up in the project. As I said before, because things have changed a bit in the main project and I had no time to work on it, I think we should at the moment close PR without merging and come back to it afterwards.

[sielay]

I will create new PR when I align to 0.4.0

[sielay]

@britztopher @ilanbiala
I created new branch which is merged with 0.4.0. Anyhow because you made very drastic changes and continued to store junk into provideData and additionalProviderData (don't like it very much), I have plenty of conflicts. I am doing now MVP of new project and aligning to that I will be stabilising that. Thanks to that I will be able to test it against actual oauth authorities. Fork will be here and later will be new PR https://github.com/sielay/mean/tree/0.4.0

[lirantal]

@sielay any progress on the user refactoring PR for 0.4.0?
you should checkout the latest 0.4.0 branch though for all the updated 'master' merge and other PRs to get the updated version there.

[sielay]

@lirantal had no chance to come back to that last few weeks, just came back to the office and not sure when will be able, last refactor/rebase was quite time consuming.