passing headers to getUser()

[chainlink]

What's the best way I can pass headers from the request to the getUser() method?

[maxtruxa]

I don't think that's possible right now.

How about a request context argument that can be passed to any of the top level methods and is passed down to any model function in the context of this request?
That way custom middleware could be inserted before any server middleware that extracts all required information from the request object.

[chainlink]

I like this idea and will start working on a PR for it. Alternative, how about extending this PR #320 to pass this instead of this.model? That way you would get the request object

[maxtruxa]

Using this instead of model would probably be a problem because this could be many things: AuthenticateHandler, AuthorizeHandler, TokenHandler, ...

[maxtruxa]

But then, I like the idea of hiding the implementation of the HTTP frontend used from the model, so making the request object accessible to the model would definitely be a good solution.. I'll have to think about that.

[chainlink]

I created a PR here #338 Would love your thoughts (would like to stick with the main project rather than forking if possible :) )

[mjsalinger]

I think there's definitely a need to be able to pass request context to the model methods.. I've run into it myself, but not sure if a request or attaching request to this is the best idea. Part of me likes attaching it to this, because the request is part of the context you're in rather than a specific param of the model. Plus, it has the added benefit of not being a breaking change.

@chainlink Can you outline some of your use cases so we can think about this a little more? Also, thanks for the PR!

[chainlink]

In our case we need to be able to pass along a header value into the getUser method because it makes a remote call which requires it. In general, having the request context seems useful to that method specifically in cases where the username / password do not reside locally in the OAuthDB.

[chainlink]

Another use is case is for Realms which are usually passed via header. We're currently adding Realm support as well and need to access the header in the getUser method.

[maxtruxa]

Just passing request to model#getUser seems like a fix tailored towards a very specific problem.
The solution should make arbitrary data available to all model functions, not just one piece to one single function.

Specifying the request context could look something like this:

// assuming request and response are valid Request and Response objects
var context = {request: request}; // the request context
oauthServer.authenticate(request, response, {context: context});

or with express-oauth-server:

app.get('/', function(req, res, next) {
  var context = {req: req}; // the request context
  // Maybe use `res.locals` instead of specifying the context through options?
  app.oauth.authenticate({context: context})(req, res, next);
});

This approach has the major benefit of being open to whatever data a user requires inside the model. Pass just the request; pre-parse parts of the request and pass the result; add additional data retrieved through a previous middleware; a combination of all that; ...

The question is how the context is best passed to the model. Using this wouldn't be a breaking change (because this inside the model is useless right now), but would be counterintuitive IMO and break PR #320 (which fixes model's this).

Another solution would be to add an additional argument to every single model function, which isn't pretty as it bloats the model interface.

getClient(clientId, clientSecret, [callback])
getUser(username, password, [callback])

becomes

getClient(clientId, clientSecret, context, [callback])
getUser(username, password, context, [callback])

For users utilizing promises it's fine because they don't care about the callback and can use

getUser(username, password)
getUser(username, password, context)

depending on whether they need the context or not.

For users using the callback, it sucks because they can't ignore the context argument:

getUser(username, password, context, callback) // don't care about context
getUser(username, password, context, callback)

Thoughts?

[chainlink]

Yeah, I had originally considered this but it seems murky and conflicts with #320.

I like the generalized context approach and am happy to extend the PR to pass context to all model methods. We could also add a flag passContext: true/false so it's not a breaking change for existing callback based implementations.

Sound good?

I know there's a big docs update coming, so I'll hold off on updating those until that drops.

[mjsalinger]

@chainlink Can you refactor your PR to include the context object that maxtruxa outlined? It's a breaking change, but if we get it in before the 3.0 release, it's a little bit of a pain for callback users, but fine.

[chainlink]

Working on a solution for this, if you pass context it will pass it down as an arg, if you don't the args will stay the same. Make sense?

[onishinji]

@chainlink any news ?