Support ctx.state to pass user about koa-passport

[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