strongloop / loopback-example-passport

LoopBack example for facebook login
Other
185 stars 134 forks source link

User afterRemote('create' not firing after adding passport #42

Closed mikesparr closed 8 years ago

mikesparr commented 9 years ago

I added a user.js file to this project and an email DS, etc. When I add some functionality for lost password, reset, etc. it works. When I try to add email verification to this app, it fails and the remote hook never gets fired.

Is there any reason why User.create is not working during signup when the User.create(newUser, ... is in the signup route and I add remote hook? Since user.json extends User model in core, do we have to somehow register the create remote method?

Added email verification from the loopback-faq-user-management example app and User.afterRemote('create' fails

var path = require('path');
var app = require(path.resolve(__dirname, '../../server/server'));

module.exports = function(user) {
  //send verification email after registration
  user.afterRemote('create', function(context, user, next) {
    console.log('> user.afterRemote triggered');

    var options = {
      type: 'email',
      to: user.email,
      from: 'noreply@myco.com',
      subject: 'Thanks for registering.',
      template: path.resolve(__dirname, '../../server/views/pages/verify.jade'),
      redirect: 'pages/verified',
      user: user
    };

    user.verify(options, function(err, response) {
      if (err) {
        next(err);
        return;
      }

      console.log('> verification email sent:', response);

      context.res.render('pages/response', {
        title: 'Signed up successfully',
        content: 'Please check your email and click on the verification link '
          + 'before logging in.',
        redirectTo: '/',
        redirectToLinkText: 'Log in'
      });
    });
  });

  //send password reset link when requested
  user.on('resetPasswordRequest', function(info) {
    var url = 'http://' + app.get('email_link_domain') + '/reset-password';
    var html = 'Click <a href="' + url + '?access_token=' + info.accessToken.id
      + '">here</a> to reset your password';

    user.app.models.Email.send({
      to: info.email,
      from: info.email,
      subject: 'Password reset',
      html: html
    }, function(err) {
      if (err) return console.log('> error sending password reset email');
      console.log('> sending password reset email to:', info.email);
    });
  });
};
eugenehp commented 9 years ago

hey @mikesparr any progress on this?

superkhau commented 9 years ago

Can you provide a link to a test project on GitHub? See https://github.com/strongloop/loopback/wiki/Reporting-issues#bug-report

mikesparr commented 9 years ago

Hi @eugenehp and @superkhau I will be diving back in on the project using LB next week and report. I'll fork the example repo and repeat my steps and share. Thanks.

julien-sarazin commented 8 years ago

@superkhau @mikesparr Might have found the reason. in user-identity.js, line 134

      userModel.findOrCreate({ where: query }, userObj, function (err, user) {
        if (err) {
          return cb(err);
        }
      ...

The method findOrCreate won't trigger the afterRemote create.

superkhau commented 8 years ago

@julien-sarazin Would you like to submit a PR with the fix?

talpasco commented 8 years ago

Same here... user.afterRemote('create'... Never fires. (so does user.verify)

BTW, Couldnt find the 'findOrCreate' function anywhere in the solution.

talpasco commented 8 years ago

@mikesparr ... were you able to get the user.afterRemote('create' event to fire?

julien-sarazin commented 8 years ago

Hey, 

Sorry guys don’t have much time those days. I’ll try next week.

jannyHou commented 8 years ago

@julien-sarazin thank you! @superkhau @mikesparr @talpasco @julien-sarazin I think for build-in method like create, we should use operation hook instead of remote hook? The hook looks like:

MyModel.observe('before save', function myFunction(ctx, next) {
//TODO
}

I am not sure if we support

user.afterRemote('create', myFunction(ctx,next) {
//TODO
}
bialikover commented 8 years ago

+1 Same issue here, thought it was a misconfiguration but the remote hook on create is not being triggered.

jannyHou commented 8 years ago

@bialikover could you try operation hook I mentioned above? https://github.com/strongloop/loopback-example-passport/issues/42#issuecomment-202505594

bialikover commented 8 years ago

@jannyHou It works, but since its operational, It will be triggered also on update. A condition inside the hook to verify if is a new record or an update will work,

I tried afterRemote('create) in another model based on User, and it's being triggered.

jannyHou commented 8 years ago

@bialikover thank you. I will label it as a bug since afterRemote('build-in-method') is not triggered properly.

richardpringle commented 8 years ago

@jannyHou, can you make sure that the right people see this (mention their names in case they have unsubscribed)?

The reason the the remote hook isn't triggered, is because the remote method is never called. When a method is called on the back-end like that, user.create, operational hooks are triggered but remote hooks aren't. That is because back-end methods are not remote operations.

If you want a remote hook to be triggered, you should POST to /api/users and move the immediate login logic that happens after a user is created to your remote hook beforeRemote('create', ...). If you are sending a verification email anyway, you probably want to remove the immediate login logic altogether.

jannyHou commented 8 years ago

@julien-sarazin @mikesparr @bialikover @talpasco @eugenehp please check the perfect explanation from @richardpringle https://github.com/strongloop/loopback-example-passport/issues/42#issuecomment-203649243

So the remote hook would ONLY be triggered when you are calling it from remote, which could be explorer in our case. Let's reproduce the difference of calling from remote and calling on beck end: [1]. Define a afterRemote hook for create:

module.exports = function(Myuser) {
    Myuser.afterRemote('create', function(ctx, instance, next){
        console.log('create called');
        next();
    });
};

[2]. Add a boot script to call create on back end:

module.exports = function(app) {
  var myUser = app.models.myuser;
  var mySample = {
    "username": "me",
    "password": "hah",
    "email": "haha@gmail.com"
  };
  myUser.create(mySample, function (err, user) {
     if (err) return cb(err);
     else console.log("created>>", user);
  });
};

[3]. Run node . so Myuser.create() will be called in boot script, you will see afterRemote is not triggered, since you are calling from beck end: not triggerd

[4]. Use POST/ Myusers to create a new user, you can see that the hook is triggered, because you all calling it from Remote: triggered

superkhau commented 8 years ago

I think for build-in method like create, we should use operation hook instead of remote hook?

Sorry for the late reply, this one slipped by.

Yes, you should use operation hooks when you want to trigger actions based on model events (ie. when you call User.create). loopback-example-user-management was probably written before operational hooks became a feature.

In any case, it's just a simple example to show you how you can implement it in your own app. In most cases, you will be calling the remote method API to create a user from your front-end app (ie. POST to /api/user, therefore triggering the remote method anyways).

@jannyHou @richardpringle Can you guys update the verify function to use the after save op hook instead? Create the issue, TOB, etc.

jannyHou commented 8 years ago

@superkhau thank you for confirming! issue created: https://github.com/strongloop-internal/scrum-loopback/issues/822 Will refactor it soon.

jannyHou commented 8 years ago

I am closing it since the answer of question is clear now, and also created a pr for correcting old code. Please feel free to reopen it or ask questions if you are still confused.