Skip to content
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

Expire Password reset tokens #7534

Merged
merged 1 commit into from Aug 4, 2016
Merged

Conversation

@Ben305
Copy link

@Ben305 Ben305 commented Jul 31, 2016

This PR is based on #6833

I have added some tests for the new functionality and increased the default password expiration time to 3 days.

@Ben305 Ben305 force-pushed the Ben305:expire-password-reset-tokens branch Jul 31, 2016
@Ben305
Copy link
Author

@Ben305 Ben305 commented Jul 31, 2016

@tmeasday What do you think?

@Ben305 Ben305 changed the title Password reset tokens Expire Password reset tokens Aug 1, 2016
@tmeasday
tmeasday reviewed Aug 2, 2016
View changes
packages/accounts-password/password_server.js Outdated
var when = user.services.password.reset.when;
var tokenLifetimeMs = Accounts._getPasswordResetTokenLifetimeMs();
var currentTimeMs = Date.now();
if((currentTimeMs - when) > tokenLifetimeMs)

This comment has been minimized.

@tmeasday

tmeasday Aug 2, 2016
Contributor

Formatting (add a space between if and ()

@tmeasday
Copy link
Contributor

@tmeasday tmeasday commented Aug 2, 2016

This looks great @Ben305!

One minor issue with formatting and it's good to go!

@tmeasday tmeasday self-assigned this Aug 2, 2016
@Ben305 Ben305 force-pushed the Ben305:expire-password-reset-tokens branch Aug 3, 2016
@Ben305
Copy link
Author

@Ben305 Ben305 commented Aug 3, 2016

@tmeasday Thanks, I have fixed the formatting now

@Ben305 Ben305 force-pushed the Ben305:expire-password-reset-tokens branch to 4c31d68 Aug 3, 2016
@tmeasday tmeasday merged commit d7c509b into meteor:devel Aug 4, 2016
3 checks passed
3 checks passed
CLA Author has signed the Meteor CLA.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
test.isTrue(Meteor.call("resetPassword", resetPasswordToken, "new-password"));
test.isTrue(Meteor.call("login", {user: {username: username}, password: "new-password"}));

onComplete();

This comment has been minimized.

@benjamn

benjamn Aug 5, 2016
Member

This test completes prematurely, without calling onComplete(), since you used Tinytest.add instead of Tinytest.addAsync. Unfortunately, that's not the only problem: makeTestConnection doesn't modify the default connection used by Meteor.call, so these method calls don't work as written.

This comment has been minimized.

@Ben305

Ben305 Aug 6, 2016
Author

Thanks for fixing and explaining this!

This comment has been minimized.

@tmeasday

tmeasday Aug 8, 2016
Contributor

Are we sure the surrounding tests (which call test.throws) are working without being async?

benjamn added a commit that referenced this pull request Aug 5, 2016
// for them to actually expire. userId is used by tests to only expire
// tokens for the test user.
Ap._expirePasswordResetTokens = function (oldestValidDate, userId) {
var tokenLiftetimeMs = this. _getPasswordResetTokenLifetimeMs();

This comment has been minimized.

@menelike

menelike Aug 9, 2016
Contributor

This does not look like it should work, this. _getPasswordResetTokenLifetimeMs(); should be this._getPasswordResetTokenLifetimeMs();. Imho the extra space needs to be removed.

This comment has been minimized.

@benjamn

benjamn Aug 9, 2016
Member

Also tokenLiftetimeMs

benjamn added a commit that referenced this pull request Aug 9, 2016
Needed to add version constraints to all the dependencies in
accounts-base/package.js so that I can publish it independently from a
Meteor release.

Follow-up to #7534.
Fixes #7611.
benjamn added a commit that referenced this pull request Aug 11, 2016
}
}, { multi: true });
}

This comment has been minimized.

@mjmasn

mjmasn Aug 12, 2016
Contributor

I think there's an issue here if 'services.password.reset' isn't an array. Not sure why because it looks like it should only update if 'services.password.reset.when' exists. I'm running Meteor 1.4.0.1 but have updated accounts-base to 1.2.10 (maybe this is the issue?) Anyway I'm seeing a few errors in production:

MongoError: Cannot apply $pull to a non-array value ...[snip]... at AccountsServer.Ap._expirePasswordResetTokens
tmeasday added a commit that referenced this pull request Aug 15, 2016
Also added a test to actually execute this code
@tmeasday
Copy link
Contributor

@tmeasday tmeasday commented Aug 15, 2016

@mjmasn the issue is fixed on current devel. @benjamn I suppose we should release a fix to accounts-base / accounts-password?

@benjamn
Copy link
Member

@benjamn benjamn commented Aug 15, 2016

Already done!

@tmeasday
Copy link
Contributor

@tmeasday tmeasday commented Aug 15, 2016

@benjamn I mean the fix that I just made 8 minutes ago ;)

@mjmasn
Copy link
Contributor

@mjmasn mjmasn commented Aug 15, 2016

@tmeasday sweet, thanks 👍

@olizilla
Copy link

@olizilla olizilla commented Aug 16, 2016

I'm hitting

MongoError: Cannot apply $pull to a non-array value ...[snip]... at AccountsServer.Ap._expirePasswordResetTokens

in production as well, after upgrading. Can we get another release of accounts-base with @tmeasday's last fix in pls @benjamn ?

@lynchem
Copy link

@lynchem lynchem commented Aug 16, 2016

Any idea what release this will go in ?

@tmeasday
Copy link
Contributor

@tmeasday tmeasday commented Aug 17, 2016

It'll out in 1.4.1, to be released very soon (and will include a version you can update to independenly @olizilla)

@mitar
Copy link
Collaborator

@mitar mitar commented Aug 26, 2016

Ah ah ah. This broke my invitation system. :-( I agree that forget password tokens should probably have short invitation time. But invitation to the system tokens should probably have much longer one. At least in my case I expected before upgrading to new Meteor version that they will simply not expire at all.

Is there a way to add configuration to invite/enrollment tokens and that those would have a different expiration time?

@tmeasday
Copy link
Contributor

@tmeasday tmeasday commented Aug 29, 2016

@mitar - Accounts.config({passwordResetTokenExpirationInDays: ... })

@mitar
Copy link
Collaborator

@mitar mitar commented Aug 29, 2016

This is for password reset, not for enrollment. I would like that password reset is 3 days expiration, while enrollment is 30 days or more. :-)

@tmeasday
Copy link
Contributor

@tmeasday tmeasday commented Aug 29, 2016

Yeah, the code is the same right now. PR?

@mitar
Copy link
Collaborator

@mitar mitar commented Aug 29, 2016

But currently there is also only one method on the server side to handle both of those cases?

@tmeasday
Copy link
Contributor

@tmeasday tmeasday commented Aug 29, 2016

Well there are two methods but they use the same expiration logic.

@mitar
Copy link
Collaborator

@mitar mitar commented Aug 29, 2016

When they get back from the invitation workflow, the same method is called so at that point it is not possible to know how long the token should be for invitation.

So one way to address is to store in the database when it should expire, and not when it was added. Because the function you linked store as when current timestamp.

@alanning
Copy link
Contributor

@alanning alanning commented Sep 21, 2016

Like @mitar, this broke our enrollment system. :-(

Incredibly hard for us to debug this as only symptom was a blank "services.password" field until noticed the "no one complains until 3-days after invite" pattern in support tickets.

Would MDG be open to having this mentioned in the Breaking changes section of the migration doc?

Also, +1 on splitting password reset and enrollment timeouts.

@tmeasday
Copy link
Contributor

@tmeasday tmeasday commented Sep 21, 2016

@alanning sure, if you'd like to send a PR to the guide, maybe mentioning it was released in 1.4.1.

@mitar
Copy link
Collaborator

@mitar mitar commented Sep 21, 2016

@tmeasday: And what about splitting this configuration?

@tmeasday
Copy link
Contributor

@tmeasday tmeasday commented Sep 21, 2016

Oh, yeah definitely consider that pull-requests-heavily-encouraged

@mitar
Copy link
Collaborator

@mitar mitar commented Sep 21, 2016

But how it should be done. There is no way to do it without changing the database to store when to expire instead of time when invitation was sent. Or we would have to add extra flag to say the reason for the token (this could also be useful in general, to have in database why is token there, reset or invite).

@tmeasday
Copy link
Contributor

@tmeasday tmeasday commented Sep 21, 2016

I would suggest the second, but maybe we can open an issue to discuss the details?

@mitar
Copy link
Collaborator

@mitar mitar commented Sep 21, 2016

Done: #7794.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants