Reset password fix #1133

carmenlau · 2016-03-22T12:49:24Z

There are 2 updates in this PR.
1. Fix cannot reset password, when the app defined user before save.
In the original implementation, updateUserPassword update the password directly through RestWrite but the originalData argument is missing. So if the app has defined user before save, reset password will be fail. To fix this, I updated updateUserPassword function to reuse rest lib update function.

2. _perishable_token is a private field, clear it through RestWrite will cause "Permission denied for this action." error.
Fix this by access db directly when clear _perishable_token after reset password.

Let me know for any problems about the PR, hope this help!:)

flovilmart · 2016-03-22T12:51:26Z

src/Controllers/UserController.js

+      return this.config.database.adaptiveCollection('_User').then(function (collection) {
+        // Need direct database access because verification token is not a parse field
+        return collection.findOneAndUpdate({ username: username },// query
+          { $set: { _perishable_token: null } } // update


should we not unset that? to delete value altogether, I recall we had problems with $set: { key: null } with oAuth

You mean change it to { $unset: { _perishable_token: null } } instead of setting it to null or we should keep the token there? I am open to this. :)

Change to $unset :)

codecov-io · 2016-03-22T12:57:10Z

Current coverage is `91.24%`

Merging #1133 into master will decrease coverage by -0.63% as of cbedf67

@@            master   #1133   diff @@
======================================
  Files           93      93       
  Stmts         5812    5814     +2
  Branches      1056    1056       
  Methods          0       0       
======================================
- Hit           5340    5305    -35
  Partial         11      11       
- Missed         461     498    +37

Review entire Coverage Diff as of cbedf67

Powered by Codecov. Updated on successful CI builds.

facebook-github-bot · 2016-03-22T14:30:51Z

@carmenlau updated the pull request.

flovilmart · 2016-03-22T18:39:07Z

last tiny thing, can you add a unit test that would get the user from the DB and check that the update it OK? we had a previous unit test and it seemed to be working where it wasn't

facebook-github-bot · 2016-03-22T23:57:56Z

@carmenlau updated the pull request.

facebook-github-bot · 2016-03-23T09:28:49Z

@carmenlau updated the pull request.

carmenlau · 2016-03-23T12:37:34Z

Updated!

flovilmart · 2016-03-23T12:41:19Z

spec/ValidationAndPasswordsReset.spec.js

@@ -573,7 +573,15 @@ describe("Password Reset", () => {
            expect(response.body).toEqual('Found. Redirecting to http://localhost:8378/1/apps/password_reset_success.html');

            Parse.User.logIn("zxcv", "hello").then(function(user){
-              done();
+              let config = new Config('test');


flovilmart · 2016-03-23T12:46:05Z

Travis seems to be drunk...

flovilmart · 2016-03-23T17:35:04Z

can you rebase on master, a fix was pushed earlier for the failing builds

… object in user before save is empty when reset password. parse-community#951

…is not a parse field, cannot clear it through rest. Update it separately. parse-community#951

… to null. parse-community#951

…r password reset. parse-community#951

carmenlau · 2016-03-24T02:34:19Z

Done :)

drew-gross · 2016-03-24T17:34:56Z

Sweet, glad to see a fix :) If you want to help with some optimization, the current method does 1 database read (to find the user) and two writes (to update the password, and clear the token) but it could be done with no reads and one write (by making the token and the $unset part of the update)

facebook-github-bot added the GH Review: review-needed label Mar 22, 2016

flovilmart reviewed Mar 22, 2016
View reviewed changes

carmenlau force-pushed the reset-password-fix branch from 271de12 to ebbda59 Compare March 23, 2016 12:19

flovilmart reviewed Mar 23, 2016
View reviewed changes

carmenlau added 4 commits March 24, 2016 10:22

Instead of executing write directly, reuse rest.update to fix request…

53e152e

… object in user before save is empty when reset password. parse-community#951

Clear reset password token after reset password. _perishable_token …

b3c5e83

…is not a parse field, cannot clear it through rest. Update it separately. parse-community#951

Unset _perishable_token after reset password, instead of setting it…

120c6fe

… to null. parse-community#951

Add test case for checking _perishable_token, it should be unset afte…

603bf97

…r password reset. parse-community#951

carmenlau force-pushed the reset-password-fix branch from ebbda59 to 603bf97 Compare March 24, 2016 02:22

drew-gross added GH Review: accepted and removed GH Review: review-needed labels Mar 24, 2016

drew-gross merged commit 82ebba4 into parse-community:master Mar 24, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reset password fix #1133

Reset password fix #1133

carmenlau commented Mar 22, 2016

flovilmart Mar 22, 2016

carmenlau Mar 22, 2016

flovilmart Mar 22, 2016

codecov-io commented Mar 22, 2016

facebook-github-bot commented Mar 22, 2016

flovilmart commented Mar 22, 2016

facebook-github-bot commented Mar 22, 2016

facebook-github-bot commented Mar 23, 2016

carmenlau commented Mar 23, 2016

flovilmart Mar 23, 2016

flovilmart commented Mar 23, 2016

flovilmart commented Mar 23, 2016

carmenlau commented Mar 24, 2016

drew-gross commented Mar 24, 2016

Reset password fix #1133

Reset password fix #1133

Conversation

carmenlau commented Mar 22, 2016

flovilmart Mar 22, 2016

Choose a reason for hiding this comment

carmenlau Mar 22, 2016

Choose a reason for hiding this comment

flovilmart Mar 22, 2016

Choose a reason for hiding this comment

codecov-io commented Mar 22, 2016

Current coverage is 91.24%

facebook-github-bot commented Mar 22, 2016

flovilmart commented Mar 22, 2016

facebook-github-bot commented Mar 22, 2016

facebook-github-bot commented Mar 23, 2016

carmenlau commented Mar 23, 2016

flovilmart Mar 23, 2016

Choose a reason for hiding this comment

flovilmart commented Mar 23, 2016

flovilmart commented Mar 23, 2016

carmenlau commented Mar 24, 2016

drew-gross commented Mar 24, 2016

Current coverage is `91.24%`