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

Allow manipulation of params before checking credentials #186

Merged
merged 3 commits into from Apr 29, 2016

Conversation

Projects
None yet
3 participants
@saiichihashimoto
Copy link
Contributor

saiichihashimoto commented Apr 28, 2016

This allows manipulating the parameters of the user service before checking the credentials.

Closes #165 Closes #171

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Apr 28, 2016

@saiichihashimoto thanks. Can you give an example of how to use this?

@saiichihashimoto

This comment has been minimized.

Copy link
Contributor Author

saiichihashimoto commented Apr 28, 2016

Sure! In my case, I allows users to create different "organizations", which they interact with in a different subdomain. There is only one user db across all the organizations, however. So, when setting up feathers-authentication, I'd do something like:

app.configure(authentication({
  local: {
    checkCredentialsParams: function(req, username, password) {
      var params = { query: {} };
      // Add the organization onto params.query
      return params;
    }
  }
));

feathers-authentication would go on to add the user onto the query and do its checking. In the case of #171, since @toddgeist wants to add the password onto the request, he'd now be able to do that.

@@ -105,7 +103,7 @@ export class Service {
}

export default function(options){
options = Object.assign({}, defaults, options);
options = Object.assign({ passReqToCallback: true }, defaults, options);

This comment has been minimized.

@marshallswain

marshallswain Apr 28, 2016

Member

What does passReqToCallback do?

This comment has been minimized.

@saiichihashimoto

saiichihashimoto Apr 28, 2016

Author Contributor

If you look to checkCredentials, the first parameter is now the request.

Passport mentions it in http://passportjs.org/docs/configure#association-in-verify-callback

One downside to the approach described above is that it requires two instances of the same strategy and supporting routes.

To avoid this, set the strategy's passReqToCallback option to true. With this option enabled, req will be passed as the first argument to the verify callback.

This comment has been minimized.

@ekryski

ekryski Apr 29, 2016

Member

Ya this is pretty common when dealing with passport, especially if you need to do account consolidation. We're already doing this in the OAuth2 service. @saiichihashimoto let's move this up into the defaults object, that way all the defaults stay organized together.

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Apr 28, 2016

I think this is a step in the right direction.

@saiichihashimoto

This comment has been minimized.

Copy link
Contributor Author

saiichihashimoto commented Apr 28, 2016

I imagine there's a cleaner (and more in-line with the rest of feathers) way to accomplish this, but its somewhat of a blocker for me and I'd love to have this feature. ;-)

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Apr 29, 2016

@saiichihashimoto Yes this definitely is a step in the right direction so thanks for putting this together!

I think the best way for Feathers to stay flexible and accommodate all the different uses cases people have is to simply document more fully how you would extend an auth service. Other people have reported issues where they would like to do other things.

In reality someone should only need to extend the auth service and implement their own checkCredentials method. Then you can do whatever you need to inside of it. This is effectively the same as if you had to implement the Strategy callback with Passport. So what I suggest is that:

  • we norm on a consistent method name across the auth services, maybe authCallback
  • we allow you to extend the service
  • we keep the passRequestToCallback: true in the default options

Make sense?

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Apr 29, 2016

Maybe we can add another promise attribute that resolves with the params that you want, and you can just replace the promise with whatever you want.

buildCredentials(req){
  return new Promise(function(resolve, reject){
    let params = {
      username: req.body.username,
      password: req.body.password
    };
    resolve(params);
  }
}

checkCredentials(req, done) {
  this.buildCredentials(req).then(params => {
    // Look up the user
    this.app.service(this.options.userEndpoint).find(params)
    ...
  });
}

Then you can just provide your own buildCredentials.

We can of course make it more pure, this is just to illustrate the idea. :)

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Apr 29, 2016

@marshallswain I do like that!

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Apr 29, 2016

Cool! (that was fast) @saiichihashimoto do you want to modify your PR? If not I will probably get to this on Monday or Tuesday.

@saiichihashimoto

This comment has been minimized.

Copy link
Contributor Author

saiichihashimoto commented Apr 29, 2016

This should cover it! Let me know if its what you're looking for.

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Apr 29, 2016

Looks good to me. @ekryski you call the ship.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Apr 29, 2016

👍 thanks @saiichihashimoto! We'll need to document this but I have an open issue in the docs already. 🍻

@ekryski ekryski merged commit f6204cd into feathersjs:master Apr 29, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Apr 29, 2016

I'll push a release over the weekend. It's been a while since I've looked at auth and I think some PRs have gone in to master already that aren't released so I need to see what changed.

@saiichihashimoto

This comment has been minimized.

Copy link
Contributor Author

saiichihashimoto commented May 2, 2016

Let me know when this happens @ekryski, really looking forward to it!

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented May 3, 2016

@saiichihashimoto published v0.7.6! Thanks again for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.