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

[fix #8218] cleaning up reset tokens also cleans up enroll tokens #8474

Merged
merged 5 commits into from Mar 29, 2017

Conversation

Projects
None yet
3 participants
@mutdmour
Contributor

mutdmour commented Mar 10, 2017

The issue was with _.extend overriding two documents (a token filter by reason and a time filter) with the same key ($or). Put the two filters in an ($add) to solve the issue, and added two tests to ensure no interference between those two.

Fixes #8218

@abernix abernix changed the title from fix for bug #8218: cleaning up reset tokens also cleans up enroll tokens to [fix #8218] cleaning up reset tokens also cleans up enroll tokens Mar 11, 2017

@abernix

Thanks very much for working on this! Looks good functionality wise, but just a few formatting changes if you don't mind?

@@ -1697,6 +1697,34 @@ if (Meteor.isServer) (function () {
}
)

Tinytest.add(
'passwords - enroll tokens don\'t get cleaned up when reset tokens are cleaned up',

This comment has been minimized.

@abernix

abernix Mar 11, 2017

Member

Please just use double quotes around this to avoid the need to escape the '.

This comment has been minimized.

@mutdmour

mutdmour Mar 11, 2017

Contributor

will fix it. There was another test that I based this on.
Tinytest.add( 'passwords - reset password doesn\t work if email changed after email sent',
Now I look closely and realize that was a typo. I can fix this as well.

Accounts._expirePasswordResetTokens(new Date(), userId);

test.isTrue(Meteor.users.findOne(userId).services.password.reset);
}

This comment has been minimized.

@abernix

abernix Mar 11, 2017

Member

No real preference about how you format your commands and the assertions that follow them, but please try to make them consistent – maybe like this?

var userId = ...

Accounts.sendEnrollmentEmail(userId, email);
test.isTrue(!!Meteor.users.findOne(userId).services.password.reset);

Accounts._expirePasswordResetTokens(new Date(), userId);
test.isTrue(Meteor.users.findOne(userId).services.password.reset);

This comment has been minimized.

@mutdmour

mutdmour Mar 11, 2017

Contributor

Cool, I can change that, though this was consistent with the other tests in the same file.

test.isTrue(!!Meteor.users.findOne(userId).services.password.reset);

Accounts._expirePasswordEnrollTokens(new Date(), userId);

This comment has been minimized.

@abernix

abernix Mar 11, 2017

Member

Same questions as above.

var email = test.id + '-intercept@example.com';
var userId = Accounts.createUser({email: email, password: 'password'});
Accounts.sendEnrollmentEmail(userId, email);
test.isTrue(!!Meteor.users.findOne(userId).services.password.reset);

This comment has been minimized.

@abernix

abernix Mar 11, 2017

Member

Can you explain why you're using !! on this test? If you're trying to coerce something, perhaps it would be better to use a different test? Either way, it's not clear if you're trying to explicitly test for something different than the test.isTrue below.

This comment has been minimized.

@mutdmour

mutdmour Mar 11, 2017

Contributor

I did it mostly to stay consistent with the rest of the tests on token cleanup. I have looked into it and I don't see why use !!. Meteor.users.findOne(userId).services.password.reset returns an object or undefined. At first I thought it was to handle empty object case {}. Though both cases test.isTrue({}) and test.isTrue(!!{}) are true. Both cases test.isTrue(undefined) and test.isTrue(!!undefined) are false. I can remove the !!. What do you think?

This comment has been minimized.

@abernix

abernix Mar 13, 2017

Member

I think we could do without the the coercion notation (!!) however I believe this test is trying to test that it is entirely unchanged (right?) so it should probably test for exactly the same thing (i.e. with !!).

This comment has been minimized.

@mutdmour

mutdmour Mar 18, 2017

Contributor

It's not to test if it's exactly the same thing. It's test for existence of the reset or enroll token object, which is added in the line before.

This comment has been minimized.

@abernix

abernix Mar 22, 2017

Member

I mean that when line 1710 runs Meteor.users.findOne(userId).services.password.reset, it's meant to make sure that it's the same as when it ran on line 1706, right?

This comment has been minimized.

@mutdmour

mutdmour Mar 22, 2017

Contributor

Yes exactly. Maybe it's better to test for equality then directly.
var resetToken = Meteor.users.findOne(userId).services.password.reset;
test.isTrue(resetToken);
Accounts._expirePasswordResetTokens(new Date(), userId);
test.isEqual(resetToken, Meteor.users.findOne(userId).services.password.reset);

This comment has been minimized.

@abernix

abernix Mar 22, 2017

Member

Yeah, that sounds best to me. Mind making the fix? Then I think we're good to go.

This comment has been minimized.

@mutdmour

mutdmour Mar 22, 2017

Contributor

Done

{ "services.password.reset.when": { $lt: oldestValidDate } },
{ "services.password.reset.when": { $lt: +oldestValidDate } }
]
}]

This comment has been minimized.

@abernix

abernix Mar 11, 2017

Member

I realize the formatting here wasn't great to begin with, but this could still use some cleaning up - we should take this opportunity to fix it.

At the very least, with this complexity, the $or should become a variable:

  const resetRangeOr = {
    $or: [
      "services.password.reset.when": { $lt: oldestValidDate } },
      "services.password.reset.when": { $lt: +oldestValidDate } },
    ],
  };

Then, we should fix the indentation and stop extend-ing into a variable name which doesn't make sense after it's changed (Since _.extend mutates the first parameter, and then it's no longer the userFilter once the tokenFilter and resetRange are in there):

accounts.users.update(
  _.extend(
    {},
    userFilter,
    { $and: [ tokenFilter, resetRangeOr ] }
  )
);

Or instead of the last suggestion (though I realize this doesn't match the existing code, it's much more clear):

  // resetRangeOr and userFilter are already defined above.
  const expireFilter = { $and: [tokenFilter, resetRangeOf] };

  accounts.users.update({ ...userFilter, ...expireFilter });

This comment has been minimized.

@mutdmour

mutdmour Mar 11, 2017

Contributor

question, what's this ... notation? I have not seen it before and a hard one to get an answer for from google. why does it solve my "unknown operator" issue?

This comment has been minimized.

@abernix

abernix Mar 13, 2017

Member

Great question, and hard to search for dots on Google. 😄

This is Object spread notation, taking advantage of the Spread operator but using a Stage-3 ECMAScript proposal seen here. Stage-3 proposals are very likely to become a part of the official specification and we have enabled in Meteor via babel-preset-meteor.

mutdmour added some commits Mar 11, 2017

@abernix

This comment has been minimized.

Member

abernix commented Mar 24, 2017

I think your 00a2cb8 failed because of a problematic commit on the devel branch. If you rebase off devel and try again it should work. (And I think you can revert your attempt to fix it with b6d687e 😄)

Sorry for the troubles!

@mutdmour

This comment has been minimized.

Contributor

mutdmour commented Mar 26, 2017

@abernix I think this is ready. Let me know if there's anything else needed. I have a fix for another issue ready but cannot create branches. Can you add me as a collaborator so I can create branches?

@hwillson

This comment has been minimized.

Member

hwillson commented Mar 26, 2017

@mutdmour Since you're working off of your own fork, you can create as many branches as you want there. When you're ready to submit your PR, you can then do so from your branch. Creating branches within the Meteor project itself is very tightly controlled (to try to keep things nice and clean), and not necessary for submitting bug fix / feature request PR's.

@mutdmour

This comment has been minimized.

Contributor

mutdmour commented Mar 26, 2017

@hwillson Thanks ya, that makes sense. I thought I could not create branches on my fork either, but I just tried doing it locally first and that worked.

@abernix abernix merged commit 1f83143 into meteor:devel Mar 29, 2017

2 checks passed

CLA Author has signed the Meteor CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

abernix added a commit that referenced this pull request Mar 29, 2017

[fix #8218] cleaning up reset tokens also cleans up enroll tokens (#8474
)

* solved issue 8218 with expireresettoken removing enroll tokens

* style fix

* tests fix

* solved issue 8218 with expireresettoken removing enroll tokens

* style fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment