Closed ZECTBynmo closed 9 years ago
Thanks heaps. I was hoping to get some kind of authentication in soon. It's going to take me a few days to integrate and test this. Hang in there.
No rush at all, happy to help!
Hi just looking over your change sets. All looks pretty good, thanks for updating the readme as well. I have one change I'd like you to make. Can you please load auth.js from server.js, I think that would be a better place for it to keep it indendent of rest.js.
I'm also wondering if we can do this without Mongoose? Mongoose seems cool but I'm trying to keep the dependencies down. Any chance you would be able to refactor so that it just uses promised-mongo.
Also, not essential, but if you were to add some integration tests to test/integration.spec.js I'd really appreciate it.
Thanks again. If we can just get these couple of things sorted out then I see no reason not to pull the changes and include it in v0.10.10 on npm.
Also... looks like you forgot to finish updating the readme. At the end it seems like there is something missing.
Hey Mike, have you had a chance to have a look at my response to your pull request?
Cheers Ash
Hey @ashleydavis. Yes, I saw your response, and I was hoping I'd be able to respond back by completing the pull request, but I'm having trouble finding time. It might be a few weeks before I'm able to sit down and finish this,
Hi @ZECTBynmo, I'm I've integrated your changes and have some questions. It's been awhile hopefully you can remember ;)
In config.json you have two mongodb connection strings: tokenDbMongoUri and usersDbMongoUri. Presumably this means that users and tokens can be stored in separate databases to the main database? Is this intentional? What purpose does it serve? Does this make authentication more secure somehow?
You have a route defined to handle login. Should there also be a route to handle logout? Or is such a thing this unnecessary?
Cheers Ash
Hey Ash,
Yes that's correct. It's intended simply as a convenience thing, and has nothing to do with security. In my specific use case, I have 3 different mongo databases on 3 different serevers - one for users, one for tokens/auth, and one I'll be running queries against.
Logout is a good idea, I just hadn't really thought about it. You could just query for and delete tokens associated with the user.
Something worth thinking about and optimizing is the token refresh/expiration time. The faster tokens expire, the more secure this system is, but the more annoying it is to use. It's a tradeoff that I'll leave it to you to decide. Personally, I ended up being paranoid and setting up very fast expiration times.
Sorry that I never completed the PR. I'm happy to answer any questions you have as you bring my code into the fold.
ZECT
Another question. In the middleware that does the authentication, after finding the token in the database, and if the token is expired, then newToken is initialized to new Token(...). However newToken.save(...) is never called.
Is this an oversight? Or does Mongoose automatically save it?
Not a question this time, just letting you know about a bug I discovered. At the top of auth.js is this code:
module.exports = function(app, config) {
if( config.usersDBConnection == undefined || config.tokenDBConnection == undefined ) {
return;
}
The auth config is actually under an auth object, so it should be this:
module.exports = function(app, config) {
if( config.auth.usersDBConnection == undefined || config.auth.tokenDBConnection == undefined ) {
return;
}
The names are also wrong. In the code they are usersDBConnection and tokenDBConnection, whereas in the config they are usersDbMongoUri and tokenDbMongoUri.
I'm working through this at the moment. I'll let you know once I've pushed, it'll be great to get your comments on my changes.
I've made some changes to your code and I've pushed it. Be great if you could have a look over my commits and make comment. I have more changes I'd like to make before doing an npm release... before I do that I need to get some automated integration tests online for your code.
Hey @ZECTBynmo, I've made some heft changes to your auth code. I removed the Mongoose dependency, the code now depends on the NodeJS MongoDB driver. I also wrote a fairly complete set of jasmine integration tests.
Be great if you could have a look at it, try to use and make comments before I release it to npm.
Cheers Ash
Hey! I really appreciate the work you're doing to take mongodb-rest forward, and I thought I might be able to contribute a bit :+1: Let me know if I can answer any questions, etc.