Improve base User extensibility

[JonnyBGod]

Description

The intention here is to provide a Base User that can easily be extended.
One simple exemple is that when you want to extend it to work with multiple emails, you currently cannot overwrite many email related functionality.

New methods that can be accessed and overwritten:

• User.setupRemoteMethods
• User.setupEmail
• User.setupValidations
• User.helpers.normalizeEmailCase
• User.helpers.beforeEmailUpdate
• User.helpers.afterEmailUpdate

This is just a style refactor no new methods or functionality were added.

Related issues

• None

Checklist

• [no need] New tests added or existing tests modified to cover all changes
• Code conforms with the style
  guide

[slnode]

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

[slnode]

Can one of the admins verify this patch?

[slnode]

Can one of the admins verify this patch?

[slnode]

Can one of the admins verify this patch?

[slnode]

Can one of the admins verify this patch?

[bajtos]

@slnode ok to test

[bajtos]

Hello @JonnyBGod, thank you for the pull request.

Your proposal to make the User model easier to extend makes sense in general, although I am concerned that exposing internal methods like the one you are proposing may tie our hands in the future, where we may want to change them.

Could you please describe what are you trying to achieve with your application that needs these customization/extensions hooks, so that we can discuss whether there are any existing options allowing you to achieve what you need?

I'll also need you to add unit-tests demonstrating the customization of User model using the new extension points. These tests will help us to prevent regressions in the future. Let's discuss your use cases before adding the tests though.

User.helpers.normalizeEmailCase

IMO, users wishing to customize this behaviour should set caseSensitiveEmail:true to disable the built-in normalization and then implement their own access observer. No need to add another extension point.

User.helpers.beforeEmailUpdate
User.helpers.afterEmailUpdate

If the intention is to disable the built-in token invalidation, then again I think it's better to implement a feature flag for that - see #3068.

[JonnyBGod]

Hi @bajtos,

After this PR I ended up working around it while doing https://github.com/JonnyBGod/loopback-multi-emails-and-phones-mixin. It is a WIP and very untested in production.

Although possible to do, I feel that it currently look a bit hackich to clean the user model in order to extend it and at least more parts like the User.observe() functions should be exposed.

I will give it another try at this soon but in general the User model seems to do too much on its own and probably the best way to do it in the future would be to strip it from a lot of functionality like email stuff, verify, etc, make a new simpler base User and place the rest in a mixin or two and that you could enable/disable through settings parameters.

I don't think it is good to have the need for:

delete User.validations.email;
User.base.clearObservers('before delete');
User.base.clearObservers('access');
User.base.clearObservers('before save');
User.base.clearObservers('after save');

to clean up, before extending and many of these features are common in simple model structures but not as common with more complex logics.

What do you think, for starters to create a new setting like 'email: true' by default and wrap all email related functionality within if (User.settings.email) {}? This would be backward compatible.

[bajtos]

After this PR I ended up working around it while doing https://github.com/JonnyBGod/loopback-multi-emails-and-phones-mixin. It is a WIP and very untested in production.

Awesome, thank you for the example. So IIUC, your requirement is for each user to have multiple emails and multiple phone numbers associated with their account. This looks like a very reasonable use case to me.

Currently, you are storing this information in two new models EmailAddress and PhoneNumber and setting up User embedsMany EmailAddress and User embedsMany PhoneNumber relations, which creates conflicts with the assumptions made by the User model.

in general the User model seems to do too much on its own and probably the best way to do it in the future would be to strip it from a lot of functionality like email stuff, verify, etc, make a new simpler base User and place the rest in a mixin or two and that you could enable/disable through settings parameters.

I totally agree! The trouble is how to get from the current design/code to the new, simpler one while making it reasonably easy for users to upgrade...

I think that ideally, LoopBack should only define a contract expected from a User-like model, and then the applications should be free to provide their own User model built from scratch. E.g. an user coming via oauth2/facebook/google authentication, a user with multiple emails like you are looking for, etc.

What do you think, for starters to create a new setting like 'email: true' by default and wrap all email related functionality within if (User.settings.email) {}? This would be backward compatible.

In principle, using feature flags to control what's exposed by a built-in model is the right approach 👍

I am not sure if some other parts of LoopBack don't rely on the single User.email property being present, we would have to check that before implementing your proposed flag.

@raymondfeng @ritch @superkhau what is your opinion?

[JonnyBGod]

I agree that the difficult part here is not too impose too many braking changes. But I do not see a way around it.

We could create a new "BaseUser" with minimum features and then keep "User" for backward compatibility, at least for a while where we would extend "BaseUser" with rest of current features and behaviors.

This approach would make it possible to extend from "BaseUser" for more advance use cases while the current apps would still work as is.

[JonnyBGod]

This would also make it easier to coordinate other libs that relate to user such as models that have relations with user. In a first phase they would work with "User" and we could update progressively to relate to "BaseUser".

Change docs, give it a fair amount of time before fully discontinue "User" model

[superkhau]

I think that ideally, LoopBack should only define a contract expected from a User-like model, and then the applications should be free to provide their own User model built from scratch. E.g. an user coming via oauth2/facebook/google authentication, a user with multiple emails like you are looking for, etc.

I'm in agreement with a minimal BaseUser with certain extension points (the contract @bajtos mentioned above). We should also provide a few other OOB User models such as OAuthUser, GoogleUser etc (they also extend BaseUser) if we need to provide capabilities from other 3rd party systems but also allow end-users to extend BaseUser on their own if they need custom behaviours.

Ultimately, we need an agreed upon interface and extension points for end users to inject/register their own custom User models + a set of OOB User-like models for basic functionality for those who don't need something custom.

@JonnyBGod's suggestion in keeping User and adding BaseUser going forward in LB4 seems reasonable for backwards compat.

[bajtos]

#2971 may be possible related in the sense that if we use a polymorphic relation to resolve the user model, then perhaps no BaseUser model will be needed at all.

As for adding BaseUser class, I am a bit concerned if we can do that while preserving backwards compatibility, as we will have to many places to stop using User and start using BaseUser instead.

I think it will be best to start adding feature flags, like the one to disable the built-in email property, and once we have enough feature flags added, then we will have better understanding of what must be a part of BaseUser and what is an optional addition.

@JonnyBGod Few questions came to my mind as I was thinking about your use case where each User can have multiple emails.

1. How do you handle "forgotten password" scenario, when the user wants to send an email with instructions for resetting the password? Do you provide a custom User.resetPassword implementation? Would it be better if the built-in User model provided an extension point allowing applications to provide their own solution for looking up a user based on email?
   By extension point, I mean moving this code to a new method, e.g. User._findByEmail(email, options, cb), so that subclasses can overwrite it.

2. How do you handle "login" scenario ? Do you allow users to provide an email instead of a username? Do you override built-in User.login method?

3. Same question for email verification - User.verify and User.confirm.

[JonnyBGod]

@bajtos regarding your questions:

In my mixing implementation I use a primary email/phone property defaulting to the first email/phone registerer to each user and then add a way to change primary email in the client.
Then I use this email/phone for password recovery in User.on('resetPasswordRequest', function(info) {} while letting the client pass an email if it does not want to use the primary.

For User.confirm in my mixing I do:

user.emails.findOne({
            where: {
              verificationToken: token,
            },
          }

To check the confirmation code and relate it to an email address confirming each email/phone separately, meaning that you have the choice to enforce confirmation for each separate email/phone.

For User.verify I am currently overriding User.prototype.sendVerificationCode because I am using a custom email template system in my apis. Current WIP implementation:

User.prototype.sendVerificationCode = function(data, fn) {
      var self = this;

      crypto.randomBytes(16, function(err, buf) {
        if (err) {
          fn(err);
        } else {
          if (data && data.id && data.type) {
            if (data.type === 'email') {
              self.emails.updateById(data.id, {
                verificationToken: buf.toString('hex')
              }, function(err, email) {
                if (err) return fn(err);

                var userVariables = {};
                userVariables[email.email] = {
                  name: self.name,
                  verificationCode: self.verificationToken,
                  verificationUrl: 'https://' + app.get('info').domain + '/verify/' + email.verificationToken,
                };
                sendVerificationEmail({
                  name: self.name,
                  email: email.email
                }, userVariables, fn);
              });
            } else if (data.type === 'phone') {
              self.phones.updateById(data.id, {
                verificationToken: buf.toString('hex')
              }, function(err, phone) {
                if (err) return fn(err);

                var userVariables = {};
                userVariables[phone.phone] = {
                  name: self.name,
                  verificationCode: self.verificationToken,
                  verificationUrl: 'https://' + app.get('info').domain + '/verify/' + phone.verificationToken,
                };
                sendVerificationPhone({
                  name: self.name,
                  phone: phone.phone
                }, userVariables, fn);
              });
            } else {
              fn(new Error('Type must be "email" or "phone"!'));
            }
          } else if (self.email) {
            self.verificationToken = buf.toString('hex');
            self.save(function(err) {
              if (err) return fn(err);

              var userVariables = {};
              userVariables[self.email] = {
                name: self.name,
                verificationCode: self.verificationToken,
                verificationUrl: 'https://' + app.get('info').domain + '/verify/' + self.verificationToken,
              };
              sendVerificationEmail(self, userVariables, fn);
            });
          } else {
            fn(new Error('Must provide id and type!'));
          }
        }
      });
    };

All of this is pretty much WIP atm and need more testing and possible tweaks.

[JonnyBGod]

forgot to include:

function sendVerificationEmail(to, userVariables, fn) {
      User.app.models.Email.sendWithTemplate('verify', 'en', {
        to: to,
        variables: {},
        userVariables: userVariables,
      });

      fn();
    }

    // TODO
    function sendVerificationPhone(to, userVariables, fn) {
      /*User.app.models.Email.sendWithTemplate('verify', 'en', {
        to: to,
        variables: {},
        userVariables: userVariables,
      });

      fn();*/
    }

[JonnyBGod]

User.on('resetPasswordRequest', function(info) {
      // TODO: remove once https://github.com/strongloop/loopback/pull/3020
      info.accessToken.destroy();
      // TODO: remove once https://github.com/strongloop/loopback/pull/3020
      info.user.createAccessToken(900, function(err, token) {
        if (err) return debug(err);

        // TODO: replace token.id with info.accessToken.id once https://github.com/strongloop/loopback/pull/3020
        var userVariables = {};
        userVariables[info.email] = {
          name: info.user.name,
          resetUrl: 'https://' + app.get('info').domain + '/reset-password/' + token.id + '/' + info.user.id,
        };

        User.app.models.Email.sendWithTemplate('passwordReset', 'en', {
          to: info.user,
          variables: {},
          userVariables: userVariables,
        });
      });
    });

[JonnyBGod]

@bajtos for logging in I am using the mixin implementation directly, which accepts logging in with username, any email or phone.

[bajtos]

@JonnyBGod I am little bit lost on the status of the pull request.

If I understand our previous discussion correctly, there was a proposal of creating smaller BaseUser class, but since that would be a breaking change, we won't be able to land it right now. Another alternative mentioned was a feature flag to configure the user model to not use the default "email" property, I think that's the right way to move this forward.

Could you please rework your pull request in this direction, and also rebase it on top of the current master (git rebase strongloop/master and git push -f)? Please keep the work in this pull request (the same master branch in your git remote), so that the discussion stays in one place.

[JonnyBGod]

@bajtos Got it. will try to find the time in the coming days.

[bajtos]

@JonnyBGod I am closing this request as abandoned. Please reopen it (or submit a new one) if/when you get time to work on this feature again.