Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move the callback scope up one level so that customCallback option has access to res and req #23

Merged
merged 1 commit into from Sep 10, 2014

Conversation

frankcarey
Copy link
Contributor

Otherwise it's impossible to do anything like redirects within it.

This option exists, but it isn't documented so it should be a non-breaking change unless others have used it.

To finish this PR a few more things should happen in my option:

  • Add a passportConfigurator.getOptions(strategy, optionOverrides){} function that would create the proper options for you per strategy. If you are using a custom function, then you loose access to the scope that is doing all the defaults stuff.
  • Implement this for every strategy, right now it's only setup for oauth w/ link == false and then only on the final callback url returned by the Oauth provider.

Here is an example of how I'm using it to redirect the user to back to my frontend angular app with the access_token and userId set as params in the url. (then I can grab them and set the values in LookBackAuth (loopback-sdk-angular)

// Configure passport strategies for third party auth providers
// Note that I changed from using for loop from docs because it can cause unexpected behavior.
// I also updated the names from "s" and "c" because it's not very clear what things are.
Object.keys(providers).forEach(function(strategy) {

  // opts will be in the scope of the callback below. 
  var opts = providers[strategy];

  // Doesn't look like we need this..yet, but it would be good to set proper options
  // up front so we can use the proper values directly below.
  //opts = passportConfigurator.options(strategy, opts);

  opts.session = opts.session !== false;

  // We can create a customCallback to set params on the callback url.
  // This will be called when the path is registered using app.get("/callback/path", opts.customCallback);
  // inside passportConfigurator.configureProvider();
  // res, req, and next are passed in via express.
  opts.customCallback = function (req, res, next) {
    // We need url, because we want to use to to parse the url and then reformat it with params.
    var url = require('url');

    // Note that we have to only use variables that are in scope right now, like opts.
    passport.authenticate(
      strategy,
      {session: false},
      //See http://passportjs.org/guide/authenticate/
      // err, user, and info are passed to this by passport
      function(err, user, info) {
        if (err) {
          return next(err);
        }
        if (!user) {
          // TODO - we might want to add some params here too for failures.
          return res.redirect(opts.failureRedirect);
        }
        // Add the tokens to the callback as params.
        var redirect = url.parse(opts.successRedirect, true);

        // this is needed or query is ignored. See url module docs.
        delete redirect.search;

        redirect.query = {
          'access_token': info.accessToken.id,
          // Note the .toString here is necessary.
          'userId': user.id.toString()
        };
        // Put the url back together. It should now have params set.
        redirect = url.format(redirect);
        return res.redirect(redirect);
      }
    )(req, res, next);
  };

  // Now that we added the opts.customCallback, got ahead and run the Configurator.
  passportConfigurator.configureProvider(strategy, opts);
});

…s access to res and req. Otherwise it's impossible to do anything like redirects within it.
@slnode
Copy link

slnode commented Sep 10, 2014

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

raymondfeng added a commit that referenced this pull request Sep 10, 2014
Move the callback scope up one level so that customCallback option has access to res and req
@raymondfeng raymondfeng merged commit 74b4d86 into strongloop:master Sep 10, 2014
@raymondfeng
Copy link
Member

@frankcarey It would be nice to make it an option so that we can handle the redirect with access token.

@slnode
Copy link

slnode commented Sep 10, 2014

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

@frankcarey
Copy link
Contributor Author

Yes was thinking the same thing. Shouldn't be hard to add. I'll do it later or tomorrow.

@acrodrig
Copy link

acrodrig commented Oct 5, 2016

Hi. I see the pull request as merged. Does this means that the functionality is now baked into Loopback? @frankcarey do you know the answer?

I am trying to do something similar to what you describe in #74.

Thanks much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants