search

jaredhanson / passport-local

Username and password authentication strategy for Passport and Node.js.
https://www.passportjs.org/packages/passport-local/?utm_source=github&utm_medium=referral&utm_campaign=passport-local&utm_content=about
MIT License
2.74k stars 498 forks source link

Misleading code in express3-mongoose-rememberme example #59

Closed smokku closed 10 years ago

smokku commented 10 years ago

What is the point of accessToken / generateRandomToken() ?

Comments suggest that it is used for Remember Me implementation, while it isn't. The only thing req.body.rememberme handling middleware changes/uses is session cookie expiration.

The code uses accessToken as a unique identifier (while it isn't marked unique in schema - only enforced by code) to serialize/deserialize user to/from session. Looking at express3-mongoose-rememberme/app.js line 18, you find that there are already two unique identifiers for a User: username and email, which could be used to deserialize user. There is no need to implement such convoluted code in something supposed to be an example.

andr3w321 commented 10 years ago

Absolutely correct.
https://github.com/jaredhanson/passport-local/pull/60

andr3w321 commented 10 years ago

Actually I think this method introduces security concerns although I'm not sure. It may be best to delete the example altogether since Jared has created https://github.com/jaredhanson/passport-remember-me which is the official recommended way of implementing.

smokku commented 10 years ago

passport-remember-me is not the same as simple [ ] Remember me checkbox implementation, that changes expiration time of user session. I see this example worthwhile, as it shows how to extend other authentication method (such as password) with 'remember me' feature. It helped me implement it in my code.

Of course there are security concerns with cookies, but then passport-remember-me has the exact same issues.

jaredhanson commented 10 years ago

Merged #60.