Skip to content

canary: race condition when signing up? #567

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
eikaramba opened this issue Sep 19, 2014 · 4 comments
Closed

canary: race condition when signing up? #567

eikaramba opened this issue Sep 19, 2014 · 4 comments

Comments

@eikaramba
Copy link

On a fresh checkout of the canary branch i experience the following problem:

if i sign up locally the user is created, however on the client side a /user/me call follows afterwards, this call fails as the exports.me endpoint on the server does not find the user(User.findOneAsync returns a null user object). If i try to login again manually it works (because some time has passed). For me this means, that the exports.create method for a user seems to return the token too early, somehow the user is not really created already so that the subsequent authentication call fails. I changed the newUser.saveAsync() to newUser.save back --> this works for now.

Also i noted another problem:

  • for /index and /me "-hashedPassword" is still used to remove the password, however this field is now "password" so this needs to be changed, otherwise the password is send to the client!
@eikaramba
Copy link
Author

HA! I got it (i think) - i found this: http://stackoverflow.com/questions/25798691/mongoose-with-bluebird-promisifyall-saveasync-on-model-object-results-in-an-ar

Well nothing special, but actually the current then function does indeed NOT only get the user object but instead an ARRAY of the user and the affected rows

Long but short: Change

.then(function(user) {

to

.spread(function (user, affectedRows) {

or jwt.sign({_id: user._id }... to jwt.sign({_id: user[0]._id }.....

I hope someone understands what i mean, unfortunately i'm not so good with githubs PRs, otherwise i would make one :)

So in the end it was not a race condition but just that the jwt wasn't getting the correct id instead... well null.

@eikaramba
Copy link
Author

ok i created a PR, but somehow... well there are too many files changed. i guess the problem is that i somehow created the branch in the canary.. arghhh - ok just ignore my PR and do it yourself if this is a problem.

#568

@kingcody
Copy link
Member

@eikaramba thats awesome! Good catch and find on the solution, I double checked and your findings appear to be spot on. I'll see about helping out with that PR ;)

Thanks again.

@eikaramba
Copy link
Author

wuhu, glad i can help! Actually i have some more improvements, maybe i improve my PR skills and also commit these.
e.g.
-I updated ALL dependencies(and thats a lot of major version jumps) here in my project and when changing all the .send(xxx,myvariable) lines for express to .status(xxx).send(myvariable) [express 4.x deprecated the old function) everything still works flawlessly. :)
-I added webp support to the gruntfile (they are not copied)
-some svgs files gets corrupted by grunt-contrib-svgmin but i guess this one needs to be adapted from project to project (but maybe a less aggresive approach by default makes sense)
-(optional) I added a grunt plugin to replace all static urls(css,js,images,but not ng html templates) to another domain. use case: i use a static.xxx.com subdomain for ALL static content and thus a simple /assets/images/.... does not work instead it has to be static.xxx.com/assets/images/.... i dunno whether this should be integrated into the generator options.

DaftMonk added a commit that referenced this issue Sep 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants