Allow users to update custom fields

[jlcarvalho]

Hi @colinskow, I see in #95 that you didn't like the addition of the endpoint, but I think this will be a nice feature. This will allow us to use Superlogin completely out of the box.

So, I digged a bit on #95 and I changed to allow update of multiple custom fields at once. All params will be validated against the whitelisted custom fields before the update. Also, I documented the endpoint and the helper function.

I'm open to discuss this feature.

[ghost]

+1

[numerized]

+1 need this.

[numerized]

@colinskow :))) go go go.

[numerized]

Hi Colin, what blocks you here?
How can I help?

[clariontools]

With the current "one-way" nature of the userModel whitelist custom fields that are only created and updated on registration, it is a trap to implement custom fields only to find out there is no api to manage the fields after registration.

Like many who run into this same issue, I started building out my own api until I did a quick check here and saw that others have already done a lot of work on it already. I would rather not create yet another fork or fork of a fork, so what is the best way to start using some of the goodies in this PR without getting to far off from Colin's master?

Also, I was pondering whether it would be better to drop the entire profile. custom fields feature and implement a profile as a doctype (type) of an existing user db? Alternatively, without having to filter out "profile" types on the user db, you could create a new user db to hold the profile information? In the case of a separate profile user db it might look something like application$john$profile as the db name?

I also considered using _local document storage for the profile, but then you lose benefits of replication. It seems that sl-users is replicating to and from the superlogin node server store so overall it seems appropriate to keep the profile with the user, but would welcome any thoughts on the other approaches to store the profile information of a superlogin user?

[numerized]

Sorry to insist but I need a status on this. What service on earth will have user creation without updating it? Superlogin should let this happen in my opinion this PR is perfect.

[clariontools]

@numerized , I could also use the PR and would welcome it, but it looks like Colin has already stated his opinion on this and offered a solution. Colin's opinion (Jan 9, 2017):

#95

And his reference sample, see the profile.js and inclusion in server.js:

https://github.com/colinskow/superlogin-demo/tree/master/server

This seems like a reasonable approach, but then again I would question the utility of the entire whitelist feature if it is limited to registration only? Maybe it would be better if the userModel: { } whitelist was enhanced a bit to make it more concise for various use cases?

[numerized]

I know i've commented there too. It's not acceptable solution to me. I really need full privacy on whitefields update I cannot give write access to this db to my users... who does????

[clariontools]

@numerized , again, I agree this PR from @jlcarvalho would make an excellent addition to Superlogin and there are far more use cases for it versus without it. My question really has to do with what is the best way to implement this now before Colin ever decides if it will make it in master or not?

For now it seems like you could add the updateCustomFields method from:

https://github.com/colinskow/superlogin/blob/b981e1fd7f66beb4571710b2b0aff1a47e872313/lib/user.js

and add that (updateCustomFields method) to your Server in a similar way that Colin described. That way, you would retain the privacy on the whitelist fields.

I am not sure if the unacceptable part is related to no response from Colin with no merge of this functionality into master or that it is impossible to achieve the same results any other way? Please clarify this as am planning to use the server.js with profile.js approach and cannot see the difference other than a plug and play solution vs. plug and pray :) ...

[jlcarvalho]

Also, I was pondering whether it would be better to drop the entire profile. custom fields feature and implement a profile as a doctype (type) of an existing user db? Alternatively, without having to filter out "profile" types on the user db, you could create a new user db to hold the profile information? In the case of a separate profile user db it might look something like application$john$profile as the db name?

We (me and @jpmsegurado) tried this approach. The main problem for us is that the replication of the profile db was very slow for our needs.

We store some user specifics configs for the app in this db, so we need the replication of this db before the app initialization. In this scenario it's way faster store this data as custom fields.

[clariontools]

@jlcarvalho , thanks for the information, I was also thinking about those same issues with the profile db approach so your comments would steer me away from that.

[clariontools]

@jlcarvalho , I have to agree with Colin's comments about this in #95 .

I added your code to the example profile.js example (changed config.getItem to superlogin.config.getItem and added missing packages accordingly to profile.js require statements and to the package.json.

Then modified code in server.js accordingly, it all works as expected and provides so much more flexibility. As Colin pointed out the design of the whitelist and profile object fields in from the whitelist were intended for registration only as a one way street. The whitelist profile object and fields are also returned on login as a session object (not returned with any other call, not even the session endpoint). This logic works perfectly and gives you the opportunity to add addition profile fields (not returned from the login session) in the onCreate method as documented here:

In my testing I also created additional profile fields that are not on the whitelist but I want to control creation and updates based on roles. So, for example I might have some fields in the profile that I only allow a user with "admin" role to update, maybe some profile fields are updated only by a service so I add a role named "service" and add addition an additional json object to the config that controls the role and fields relationship. In the changeProfile (you named updateCustomFields) method, I can add processing of the json object and filter out valid fields based on role (add roles parameter with req.user.roles) and create your own functions that are similar to the superlogin express middleware functions:

superlogin.requireRole(role);
superlogin.requireAnyRole(possibleRoles);
superlogin.requireAllRoles(requiredRoles);

You could try and call these express middleware functions directly, but that gets a bit messy and adds unnecessary overhead so just passing the user roles (from req.user.roles) makes it easy to replicate the same functionality. Since the middleware functions are already there in middleware.js, I would take the necessary parts and move into my profile.js as local functions.

So in my case I have a new endpoint named user/change-profile and then a user/profile and both of these work with a new json object under the config userModel: to validate user access to update and get profile fields of my choosing. The whitelist array is still valid for its original purpose of handling profile fields on registration and returning only those whitelist fields in the profile object from the auth/login endpoint.

This is just one use case that would be broken by adding the carte blanche ability to allow update of any whitelist profile fields. Also, adding the update capability to the whitelist profile fields would not allow you to add profile fields that are read-only based on role, nor would it allow you to add fields that are only visible by a certain user role. For example here is a json structure that might be used to filter both changes (plus additions) and further control read access when a GET is called on user/profile 👍

  userModel: {
    whitelist: ['profile.fullname','profile.autoLogin'],
    profileChange: {
      requireAnyRole: {
        roles: ['user','admin'],
        fields: ['profile.fullname','profile.autoLogin']
      },
      requireRole: {
        roles: ['admin'],
        fields: ['profile.cleanup']
      },
      requireAllRoles: {
        roles: ['admin','su'],
        fields: ['profile.isActive','profile.mergeWithUser']
      },
    },
    profileGet: {
      requireAnyRole: {
        roles: ['user','admin','su'],
        fields: ['profile.fullname','profile.autoLogin']
      },
      requireRole: {
        roles: ['admin'],
        fields: ['profile.cleanup']
      },
      requireAllRoles: {
        roles: ['admin','su'],
        fields: ['profile.isActive','profile.mergeWithUser']
      }
    }

Of course this could be as simple or complex as your requirements dictate. For example, if you want to give 'change' access to all roles, simple use the "requireAnyRole" property with all the fields and that is all you need, or you could add another property to allow full access to the whitelist array and then filter out other fields as needed. The point is that this approach provides much more flexibility and does not make any assumptions about profile fields other than what is currently in place with the whitelist array.

I understand the desire to have Colin update master with this capability because when I first hit the problem I was thinking it was broken too. However after following Colin's suggestion I think it provides so much more flexibility. In fact I have used Meteor for other projects and its Accounts package has always been a big productivity booster, but even it does not allow this flexibility on the profile fields. In the case of Meteor accounts, you either give the user access to all profile fields or block all of them.

You already have to configure the superlogin node server so this sort of profile configuration does not seem like too much effort for the flexibility you gain. The best thing would be to have this documented more clearly in the superlogin readme to clearly state the purpose of the whitelist and provide a link to the server sample that shows further management of profile fields in the whitelist or outside the whitelist.

On another note, I know how frustrating it can be to have a PR ignored or closed, so more important is that there is some sort of dialog. I am happy you made the PR and that I had a chance to contribute to the discussion.

[numerized]

Awesome!! Thanks

[jlcarvalho]

@clariontools great work.

But for me this PR doesn't invalidate Colin's suggestion.

The main purpose of this PR is to allow us to manage custom fields in an more sophisticated way than today. If this isn't enough for your needs, you'll still be able to extend Superlogin following Colin's sugestion.

[jlcarvalho]

You already have to configure the superlogin node server so this sort of profile configuration does not seem like too much effort for the flexibility you gain. The best thing would be to have this documented more clearly in the superlogin readme to clearly state the purpose of the whitelist and provide a link to the server sample that shows further management of profile fields in the whitelist or outside the whitelist.

You couldn't be more precise here. I agree.

[clariontools]

@jlcarvalho ,

The main purpose of this PR is to allow us to manage custom fields in an more sophisticated way. If this isn't enough for your needs, you'll still be able to extend Superlogin following Colin's sugestion.

To be precise, the whitelist is all about the profile object and the "fields" key / value pairs or additional objects contained therein. The "custom fields" name is misleading, in fact for me the pain initially started with the "field" named "name". I initially thought wow, "name" I need that to display the full name in my app. Then the realization that name is not returned by session info returned by auth/login and even worse, it cannot be changed :).

So in the case of allowing any whitelist field to be "managed" by default, that means that I cannot have a read-only whitelist profile field. Since the whitelist fields are returned in the session object of auth/login and this PR would allow any whitelist field to be changed by the user.

[clariontools]

@jlcarvalho ,

If this isn't enough for your needs

In fact I am saying it may be "too much" for my needs, to a point where I can no longer use the whitelist as originally intended as it goes from a write once on user registration read-only object returned on auth/login to an object that could be written to at anytime by an authenticated user.

[clariontools]

Having pondered a more sophisticated use case for implementing profile "field" management, I wanted to add a few other ideas that could be implemented to take things up a notch. Of course you could scale things back too, but for a solution that would cover more use cases I might consider the following:

• Change each of the role based permissions to an array to have more flexibility if needed for example:

  userModel: {
    whitelist: ['profile.fullname','profile.autoLogin'],
    profileChange: {
      requireAnyRole: [{
          roles: ['user','admin'],
          fields: ['profile.fullname','profile.autoLogin']
        },{
          roles: ['manager','admin'],
          fields: ['profile.division','profile.territory']
       }],
      requireRole: [{
        roles: ['admin'],
        fields: ['profile.cleanup']
      },{
        roles: ['service'],
        fields: ['profile.reportsActive']
      }],
      // and so on...

• Create endpoints, such as admin/change-profile, admin/profiles, service/change-profile etc..., that would allow an admin or other user role to manage profile "fields". These endpoints would normally first get a list of users, then allow the admin (or other authorized role) to manage individual profiles and fields that were defined in the userModel objects.

• The basic idea of the javascript objects (profileChange and profileGet) in the proposed use case is to take the role(s) of the authenticated user making the request and build up a list of valid fields that be changes or requested for return by GET. So it would be easy to expand on the example provided by @jlcarvalho instead of getting the keys from the whitelist array it would just involve a bit more looping through the appropriate javascript object (profileChange or profileGet) above, ultimately ending up with a list of profile fields that can be updated, code similar to @jlcarvalho:

  // 4UTODO: get customFields list not from whitelist array but you 
  // will need to create a new function to loop through
  // the config javascript object to build up a valid list for
  // the authenticated user making the request.
  // once you have the list of possible fields the remainder of the 
  // code can be reused...

   var canUpdate = function(customFields, keys) {
      return keys.every(function(key) {
        return customFields.indexOf(`profile.${key}`) !== -1;
      });
    };

    if(customFields && canUpdate(customFields, keys)) {
      return userDB.get(user_id)
   // and so on...

Again, this is one possible use case, just throwing this out there to present some ideas. I haven't implemented all of this code yet as I don't have the need to go that far with the profile, but having something this flexible would allow you to use it in quite a few different scenarios from very simple to complex.

It wouldn't be difficult to code and publish an example server demo with all these techniques, just not sure if that is really helpful because it is so simple to do on your own with your own implementation.

[fransyozef]

any updates about this? I also want to update the profile data.