Added verify password to users router and tests. #4747

johnnydimas · 2018-05-02T18:03:37Z

We needed a way in our app to verify that a user knows their current password to proceed past some functions for additional security. One example could be that we would like a way for a user to update their password from within the app while logged in, but by first submitting their 'old password'. We tried to achieve this by logging the user in server-side, but then their current session tokens would become invalid and log them out of the app, and it did not really make sense to have to send a new session token back again.

There are also some other instances where this could come in handy, where logging a user in again or using the password reset email system isn't so optimal.

In short I added a route for verifying a user password by comparing the sent in password string against the hash that is saved in the database. If the password isn't a match, then appropriate errors are returned that are the same for login errors.

If the comparison is successful, the user object (without a session token) is returned.

Couple other things to note:

If account lockout is configured in the parse-server, this function will also makes use of the account lockout policy if the wrong password is sent in multiple times.
This function does not modify session tokens.
A test spec for this function was also added.

Thanks!

codecov · 2018-05-02T18:18:09Z

Codecov Report

Merging #4747 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4747      +/-   ##
==========================================
- Coverage   92.74%   92.73%   -0.01%     
==========================================
  Files         119      119              
  Lines        8722     8732      +10     
==========================================
+ Hits         8089     8098       +9     
- Misses        633      634       +1

Impacted Files	Coverage Δ
src/Routers/UsersRouter.js	`93.61% <100%> (+0.48%)`	⬆️
...dapters/Storage/Postgres/PostgresStorageAdapter.js	`97.2% <0%> (-0.09%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e29d02...604ce0e. Read the comment docs.

… function where authData null keys condition wasn't necessary.

johnnydimas · 2018-05-04T14:46:38Z

Hi there!

I have updated the code to pass the checks this time around.

I understand it might be a heavy request to add an additional API endpoint to parse-server like this, so if there are hesitations, I'd love to know about them!

I am also willing to follow through by adding this to the Parse JS SDK so we can get a Parse.User.verifyPassword function, as well as adding it to the appropriate documentation like the REST API docs and javascript docs.

Let me know if there's anything else I can do to support or if I can explain with any more reasoning.

Thanks so much!

flovilmart · 2018-05-07T16:53:52Z

That sounds like a great feature! Is there any particular reason we have both GET and POST handling for this feature? Because nothing gets created server side, I believe the right method would be just GET.

flovilmart

@johnnydimas The Tests look good, if everything is fine, I'd like to have the code shared between login and verifyPassword, rather than duplicated, as I will ease maintenance, and Login is just validating pass + creating a session :)

Can you update the PR and we'll be able to make a release :)

flovilmart · 2018-05-07T18:32:31Z

src/Routers/UsersRouter.js

@@ -267,6 +267,77 @@ export class UsersRouter extends ClassesRouter {
    });
  }

+  handleVerifyPassword(req) {


@johnnydimas this whole section could be factored differently, as this is shared 100% with the login code right?

We could probably have a function somewhere that handles password validation, in both login / verifyPassword and only create the session on login? What do you think?

That sounds great to me! Good call on removing the POST handler as well, it's not necessary for this.

I pushed up a fix removing the POST now, but I will update the pull request again with a helper to facilitate both login and verify password.

Thanks for looking it over!

…function to validate the password provided in the request.

johnnydimas · 2018-05-08T20:21:45Z

I have created a shared helper function to reduce the duplicate code between login and verify password that validates the password, and I kept the create session code in the login function.

I noticed there is this bit of code in there that I was wondering if we should remove or keep. I followed the url in the comment and it seems that this issue has since been addressed. Let me know, and I can update the PR. 😄

// Sometimes the authData still has null on that keys
// https://github.com/parse-community/parse-server/issues/935
if (user.authData) {
    Object.keys(user.authData).forEach((provider) => {
        if (user.authData[provider] === null) {
            delete user.authData[provider];
        }
    });
    if (Object.keys(user.authData).length == 0) {
        delete user.authData;
    }
}

Thanks!

flovilmart · 2018-05-08T20:34:56Z

src/Routers/UsersRouter.js

+              // password was created before expiry policy was enabled.
+              // simply update _User object so that it will start enforcing from now
+              changedAt = new Date();
+              req.config.database.update('_User', { username: user.username },


this part feels odd that we keep it in a verification, when the password is actually not changed there. What do you think?

Makes sense! I'll move this snippet back into the handleLogin function.

Exellent thanks! That,s quite the refactor you're on, I want to get it right before we merge ;)

flovilmart · 2018-05-08T20:36:23Z

src/Routers/UsersRouter.js

      });
  }

+  handleLogIn(req) {
+    try {


not a big fan of wrapping in try's why do we need those? the top level promise should never throw but return a failed promise. Can we change that?

I can update this to remove the try / catch's :)

flovilmart

Small nits otherwise, I like this level of code sharing!

flovilmart · 2018-05-08T20:36:48Z

src/Routers/UsersRouter.js

+          return { response: user };
+        });
+    } catch (error) {
+      throw error;


if we never to anything with the errors, please remove all try wrappers ;)

…ise returns the error. Moved login specific functions to login handler.

johnnydimas · 2018-05-09T18:44:30Z

I removed all the try / catch's and I moved the login specific code into that function. Let me know if you see any other issues with the PR. Thanks! :)

johnnydimas · 2018-05-18T12:44:18Z

Hey @flovilmart ! Just curious if there was anything else you'd like to see done here, happy to make any more edits. No rush and thanks! :)

flovilmart · 2018-05-18T12:47:13Z

Hi @johnnydimas there seems to be a conflict now, can you address it? I'll do a final review today.

johnnydimas · 2018-05-18T14:57:36Z

Hey! I'm working on the merge now and adding a test for the account lockout in verify password.

flovilmart · 2018-05-18T14:58:16Z

Awesome thanks!

…spec for account lockout in verify password.

… into verify_password # Conflicts: # .github/ISSUE_TEMPLATE/Bug_report.md

johnnydimas · 2018-05-18T16:32:30Z

I sorted out the merge conflict, added the lockout check to the shared helper function, and added a new test for the account lockout feature to verify password. :)

I had a weird issue with the .github/ISSUE_TEMPLATE/Bug_report.md file, and had to push it again to the branch for some reason. It looks to be exactly the same though.

flovilmart

Alright, last small change and we'll be good to go. We need the same level of cleanup on both methods. THe only difference is that one is creating the session the other isn't.

flovilmart · 2018-05-18T19:36:25Z

src/Routers/UsersRouter.js

@@ -162,13 +182,20 @@ export class UsersRouter extends ClassesRouter {
            delete user.authData;
          }
        }
+
+        // Remove hidden properties.
+        UsersRouter.removeHiddenProperties(user);


this should be moved to the _handleValidatePassword method as also shared with the other call.

flovilmart · 2018-05-18T19:36:43Z

src/Routers/UsersRouter.js

@@ -162,13 +182,20 @@ export class UsersRouter extends ClassesRouter {
            delete user.authData;


all this block shoudl be put in the extracted method as well.

So the issue here was if I move these two blocks to the _authenticateUserFromRequest function:

// Sometimes the authData still has null on that keys // https://github.com/parse-community/parse-server/issues/935 if (user.authData) { Object.keys(user.authData).forEach((provider) => { if (user.authData[provider] === null) { delete user.authData[provider]; } }); if (Object.keys(user.authData).length == 0) { delete user.authData; } } // Remove hidden properties. UsersRouter.removeHiddenProperties(user);

Three of PasswordPolicy tests for expiration will fail. It seems the order matters, here. The way I can see to solve it would be if I move this block into the _authenticateUserFromRequest as well so that the order is maintained, and so that this occurs before the authData check and removing hidden properties:

// handle password expiry policy if (req.config.passwordPolicy && req.config.passwordPolicy.maxPasswordAge) { let changedAt = user._password_changed_at; if (!changedAt) { // password was created before expiry policy was enabled. // simply update _User object so that it will start enforcing from now changedAt = new Date(); req.config.database.update('_User', { username: user.username }, { _password_changed_at: Parse._encode(changedAt) }); } else { // check whether the password has expired if (changedAt.__type == 'Date') { changedAt = new Date(changedAt.iso); } // Calculate the expiry time. const expiresAt = new Date(changedAt.getTime() + 86400000 * req.config.passwordPolicy.maxPasswordAge); if (expiresAt < new Date()) // fail of current time is past password expiry time throw new Parse.Error(Parse.Error.OBJECT_NOT_FOUND, 'Your password has expired. Please reset your password.'); } }

Is it ok to have this entire chunk of code moved into _authenticateUserFromRequest so it is like so?

// handle password expiry policy if (req.config.passwordPolicy && req.config.passwordPolicy.maxPasswordAge) { let changedAt = user._password_changed_at; if (!changedAt) { // password was created before expiry policy was enabled. // simply update _User object so that it will start enforcing from now changedAt = new Date(); req.config.database.update('_User', { username: user.username }, { _password_changed_at: Parse._encode(changedAt) }); } else { // check whether the password has expired if (changedAt.__type == 'Date') { changedAt = new Date(changedAt.iso); } // Calculate the expiry time. const expiresAt = new Date(changedAt.getTime() + 86400000 * req.config.passwordPolicy.maxPasswordAge); if (expiresAt < new Date()) // fail of current time is past password expiry time throw new Parse.Error(Parse.Error.OBJECT_NOT_FOUND, 'Your password has expired. Please reset your password.'); } } // Sometimes the authData still has null on that keys // https://github.com/parse-community/parse-server/issues/935 if (user.authData) { Object.keys(user.authData).forEach((provider) => { if (user.authData[provider] === null) { delete user.authData[provider]; } }); if (Object.keys(user.authData).length == 0) { delete user.authData; } } // Remove hidden properties. UsersRouter.removeHiddenProperties(user); delete user.password;

I'll have a look because. Seems that we need to keep is split and clean.

Had you wanted me to push with the changes I did as detailed above so you could look into it? Thanks!

flovilmart · 2018-05-18T19:36:59Z

src/Routers/UsersRouter.js

+      .then((user) => {
+
+        // Remove hidden properties.
+        UsersRouter.removeHiddenProperties(user);


remove as it will be put above.

flovilmart · 2018-05-18T19:37:42Z

src/Routers/UsersRouter.js

+   * @returns {Object} User object
+   * @private
+   */
+  _handleValidatePassword(req) {


we should rename to _authenticateUserFromRequest(req) what do you think?

THie method retuns the user, stripped of the private / hidden properties etc...

No problem, I can rename it.

…o the login function. The password expiration check in the login function is dependent on some hidden properties, otherwise three password policy tests fail.

johnnydimas · 2018-06-13T13:39:37Z

Hey @flovilmart ! I took another crack at this one.

I wasn't able to share the UsersRouter.removeHiddenProperties() snippet as the handle password expiration policy code in handleLogin() is dependent on some of those hidden properties. Let me know if you have any thoughts about this and I'll see what I can do.

Thanks!

flovilmart · 2018-06-13T18:19:46Z

@johnnydimas this looks great! It seems that all the tests are still passing and the additional tests look great. let's merge that, I don't believe we can hit regressions ! 👏

Thanks for keeping up!

* Added verify password to users router and tests. * Added more tests to support more coverage. * Added additional tests to spec. Removed condition from verifyPassword function where authData null keys condition wasn't necessary. * Removed POST handling from verifyPassword. * Refactored handleLogin and handleVerifyPassword to use shared helper function to validate the password provided in the request. * Refactored verifyPassword and login to not use try/catch. Parent promise returns the error. Moved login specific functions to login handler. * Added account lockout policy to verify password function. Added test spec for account lockout in verify password. * no message * Merged new changes from master. Made changes as requested from comments. * We cannot remove hidden properties from the helper before returning to the login function. The password expiration check in the login function is dependent on some hidden properties, otherwise three password policy tests fail.

Added verify password to users router and tests.

c7ca75e

Johnny Dimas added 2 commits May 4, 2018 15:29

Added more tests to support more coverage.

9988fc3

Added additional tests to spec. Removed condition from verifyPassword…

259b817

… function where authData null keys condition wasn't necessary.

Removed POST handling from verifyPassword.

f435091

flovilmart suggested changes May 7, 2018

View reviewed changes

Johnny Dimas and others added 2 commits May 8, 2018 21:58

Refactored handleLogin and handleVerifyPassword to use shared helper …

717aa46

…function to validate the password provided in the request.

Merge branch 'master' into verify_password

c3e5910

flovilmart reviewed May 8, 2018

View reviewed changes

flovilmart suggested changes May 8, 2018

View reviewed changes

johnnydimas and others added 2 commits May 9, 2018 10:03

Merge branch 'master' into verify_password

097b20c

Refactored verifyPassword and login to not use try/catch. Parent prom…

4d42928

…ise returns the error. Moved login specific functions to login handler.

Merge branch 'master' into verify_password

fe0650e

Johnny Dimas and others added 4 commits May 18, 2018 17:19

Added account lockout policy to verify password function. Added test …

4bc17d1

…spec for account lockout in verify password.

Merge branch 'master' into verify_password

13a6b2c

no message

b3dcc89

Merge branch 'verify_password' of github.com:johnnydimas/parse-server…

22d43b3

… into verify_password # Conflicts: # .github/ISSUE_TEMPLATE/Bug_report.md

Merge branch 'master' into verify_password

32a472c

flovilmart suggested changes May 18, 2018

View reviewed changes

Merge branch 'master' into verify_password

8038fa2

johnnydimas and others added 5 commits May 18, 2018 22:14

Merge branch 'master' into verify_password

8e090bc

Merge branch 'master' into verify_password

05d718b

merged in changes from master. redoing functions.

ca91601

Merged new changes from master. Made changes as requested from comments.

27e7f07

We cannot remove hidden properties from the helper before returning t…

604ce0e

…o the login function. The password expiration check in the login function is dependent on some hidden properties, otherwise three password policy tests fail.

flovilmart merged commit 2ef437a into parse-community:master Jun 13, 2018

johnnydimas deleted the verify_password branch June 13, 2018 19:00

kiarza2543 mentioned this pull request Dec 12, 2023

[Snyk] Upgrade @graphql-tools/utils from 8.12.0 to 8.13.1 kiarza2543/parse-server#3

Open

		@@ -162,13 +182,20 @@ export class UsersRouter extends ClassesRouter {
		delete user.authData;

Uh oh!

Added verify password to users router and tests. #4747

Added verify password to users router and tests. #4747

Uh oh!

Conversation

johnnydimas commented May 2, 2018

Uh oh!

codecov bot commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

johnnydimas commented May 4, 2018

Uh oh!

flovilmart commented May 7, 2018

Uh oh!

flovilmart left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnnydimas commented May 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flovilmart left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnnydimas commented May 9, 2018

Uh oh!

johnnydimas commented May 18, 2018

Uh oh!

flovilmart commented May 18, 2018

Uh oh!

johnnydimas commented May 18, 2018

Uh oh!

flovilmart commented May 18, 2018

Uh oh!

johnnydimas commented May 18, 2018

Uh oh!

flovilmart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnnydimas commented Jun 13, 2018

Uh oh!

flovilmart commented Jun 13, 2018

Uh oh!

Uh oh!

codecov bot commented May 2, 2018 •

edited

Loading

flovilmart left a comment •

edited

Loading

flovilmart left a comment •

edited

Loading