Support ctx.state to pass user

[gingerich]

I believe that best practice for koa v2 would be to store the authenticated user in ctx.state, rather than ctx.request.

Documentation for koa v2 indicates that ctx.state is "the recommended namespace for passing information through middleware ..."
https://github.com/koajs/koa/blob/v2.x/docs/api/context.md#ctxstate

[rkusa]

Sounds good to me. What do you think, should we still keep it in ctx.request with a deprecation warning for now?

[gingerich]

Yeah I think that makes sense. I can put together a PR for this if you want

[rkusa]

A PR is of course highly welcome, but I am also totally fine with implementing it myself.

[gingerich]

I'd love to contribute. I'll take a stab at it and see what you think.

[rkusa]

Before I publish a new version I just like to get some feedback on the methods .login(), .logout(), .isAuthenticated() and .isUnauthenticated(). They are currently added to both ctx and ctx.req. I would also like to clean this up a little bit. What is your opinion for the best location where to put them, ctx or also ctx.state?

[gingerich]

I tend to think of ctx.state as a simple data store, where logic does not belong, so my preference would be for ctx.
However, .isAuthenticated() and .isUnauthenticated() pertain more to a specific request (depending whether sessions are used), so I could see an argument for adding those two to ctx.req.
It's hard to say what's best, but if I had to choose one place for all methods, I think ctx makes the most sense.

[rkusa]

Thanks for your feedback! I agree on your points. I think .isAuthenticated() and .isUnauthenticated() fit into both ctx and ctx.request (request should probably preferred over req). But since having it both does not make too much sense, I will probably go with ctx.

[gingerich]

Before you publish, I just discovered a problem with my implementation that could create a race condition.

Basically, next() can be called before ctx.state.user is assigned because resolve callback is invoked first.

I'll fix in a new PR

[jeffijoe]

It appears that the verify callback in strategies use the req setter. E.g. https://github.com/jaredhanson/passport-http/blob/master/lib/passport-http/strategies/basic.js#L95

I checked the stack trace:

at IncomingMessage.Object.defineProperty.set [as user] (\node_modules\koa-passport\lib\framework\koa.js:37:19)
at Object.properties.(anonymous function).set (\node_modules\koa-passport\lib\framework\request.js:113:16)
at Object.req.login.req.logIn (\node_modules\passport\lib\http\request.js:44:18)
at Object.<anonymous> (\node_modules\koa-passport\lib\framework\request.js:104:27)
at BasicStrategy.strategy.success (\node_modules\passport\lib\middleware\authenticate.js:235:13)
at verified (\node_modules\passport-http\lib\passport-http\strategies\basic.js:91:10)

So this deprecation notice is unavoidable it seems.

Should I open a new issue?

[rkusa]

Thanks for reporting, I've created an issue for this: #66