Ajax password reset #5332

moonion · 2019-01-30T16:48:19Z

These changes will allow more comfortable use of the password reset route, eliminating the redirects and page reloads when reaching it through ajax request.

For example, here on client side we will be able to handle the errors without reloading the page:

    const params = {
      new_password,
      confirm_new_password: new_password,
      username,
      token,
    };

    const formBody = Object.keys(params).map((key) => {
      return encodeURIComponent(key) + '=' + encodeURIComponent(params[key]);
    }).join('&');

    const url = `${Parse.serverURL}/apps/${Parse.applicationId}/request_password_reset`

    return fetch(url, {
      method: 'POST',
      headers: {
        'Content-Type': 'application/x-www-form-urlencoded',
        'X-Requested-With': 'XMLHttpRequest'
      },
      body: formBody
    }).then(fetchresp => {
      return fetchresp.json()
    })
      .then(payload => {
        console.log("CLIENT RESPONSE: ", payload)


        if (payload.code) {
          console.log("SOME ERROR HAPPENED")
          throw payload
        }

        return payload
      }
    )

Sync

flovilmart · 2019-01-30T17:06:18Z

This does not work with the fetch API (reportedly) https://stackoverflow.com/questions/41952913/node-js-express-how-can-i-know-if-a-request-is-an-ajax-request?rq=1

moonion · 2019-01-30T17:09:47Z

@flovilmart
req.xhr works for us when we added 'X-Requested-With': 'XMLHttpRequest' header manually

flovilmart · 2019-01-30T17:26:56Z

uhm ok, can you perhaps factor so req.xhr is not all over the place? The current implementation lacks elegance.

let error;
 if (!username) {
    error = new Parse.Error(
	          Parse.Error.USERNAME_MISSING,
	          'Missing username'
	        );
} else if (!token) {
   error = new Parse.Error(
	          Parse.Error.OTHER_CAUSE,
	          'Missing token'
	        );
} else if (!new_password) {
    error = new Parse.Error(
	          Parse.Error.PASSWORD_MISSING,
	          'Missing password'
	        );
}

if (error) {
   if (req.xhr) { throw error }
   return this.invalidLink(res);
}

flovilmart

Can we make the implementation a bit more elegant, right now it's kinda messy.
There are multiple avenues for that, from swapping the promise callbacks depending on the context or what I was suggesting.

src/Routers/PublicAPIRouter.js

codecov · 2019-01-30T17:32:14Z

Codecov Report

Merging #5332 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #5332      +/-   ##
=========================================
+ Coverage   93.88%   93.9%   +0.01%     
=========================================
  Files         123     123              
  Lines        9012    9024      +12     
=========================================
+ Hits         8461    8474      +13     
+ Misses        551     550       -1

Impacted Files	Coverage Δ
src/Controllers/UserController.js	`94.33% <100%> (+0.94%)`	⬆️
src/Routers/PublicAPIRouter.js	`92.1% <100%> (+0.92%)`	⬆️
src/RestWrite.js	`93.07% <0%> (ø)`	⬆️

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 bf033be...a8d9504. Read the comment docs.

flovilmart · 2019-01-31T20:26:57Z

@moonion the code is nicer, can you add tests please and make sure they pass?

_{Sent with GitHawk}

moonion · 2019-02-05T10:20:06Z

@flovilmart I believe this will do?

flovilmart · 2019-02-05T11:56:54Z

@moonion I am glad you added a test as it is not passing. I guess it needs more work

_{Sent with GitHawk}

moonion · 2019-02-05T12:39:55Z

Yes, i noticed, though wierdly it passes for me. Trying to figure out what's wrong.

moonion · 2019-02-05T13:07:21Z

if (!username) {
      throw new Error(
        Error.USERNAME_MISSING,
        'Missing username'
      );
    }

Am i supposed to call errors like this in src file? Must have been the source of problem.

flovilmart · 2019-02-05T13:30:41Z

I am not sure what you are referring to. But the test fail, which is either because the test is malformed and the username is improperly passed, or that the implementation cannot read the user as it should.

As it seems only your test is failing, I would think the test is not properly formed

_{Sent with GitHawk}

moonion · 2019-02-05T13:38:20Z

Wait a moment, i must have been looking in the wrong place. According to this https://travis-ci.org/parse-community/parse-server/jobs/489019227 ,for example, its not only my test is failing, but a few others, that are related to the update password function that was changed in the router by my commit. This confuses a bit.

moonion · 2019-02-05T14:06:48Z

Yes, i was confused by "Parse is undefined" in these reports and thought that I need to use Error instead of Parse.Error in src files for some reason. My bad, will revert the changes right now.

moonion · 2019-02-05T14:12:38Z

I am pretty confident this works as intended, can you confirm?

There still seem to be failure when making test related to password policy, since error handling was changed, looking into it.

flovilmart · 2019-02-05T14:52:40Z

The errors are pretty straightforward in Travis logs. I seems that the changes proposed are breaking existing tests.

_{Sent with GitHawk}

moonion · 2019-02-05T15:45:12Z

Found the error in code. Seems that tests are passing now...

flovilmart · 2019-02-05T17:46:58Z

@moonion this seems very light on the test side as you only test that the call fails when you fail to provide a username. Can we get at least one test of the 'happy path' which makes sure the call resolves correctly, and appropriately?

moonion · 2019-02-06T15:45:50Z

Right now i replaced the error to show neutral text about wrong username/token, similar to previous "invalid link" error, and check if the error refers to password policy, in which case i show it instead. Probably now tests cover everything in commit if i done everything right.

moonion · 2019-02-08T08:39:39Z

Will this be enough for current merge request?

flovilmart · 2019-02-08T11:59:31Z

I’ll review later before end of week

_{Sent with GitHawk}

moonion · 2019-02-08T13:03:57Z

Thank you

moonion · 2019-02-18T08:50:04Z

So?..

flovilmart · 2019-02-18T12:10:33Z

@dplewis would you have a chance to review it?

_{Sent with GitHawk}

dplewis

Just a few nits. Looks good so far.

I'm not an expert on password reset but looks like all the success / errors are handled.

dplewis · 2019-02-21T23:57:33Z

spec/PasswordPolicy.spec.js

@@ -909,6 +909,97 @@ describe('Password Policy: ', () => {
    });
  });

+  it('Should return error when password violates Password Policy and reset through ajax', done => {


Can you rewrite your tests using async / await?

Reading this is confusing with all the nested promises.

async / await is not used in any of the tests, and i noticed there is actually a lot of branching promises in the other tests, since .catch blocks contain specific error messages for almost each action. Is it really necessary to rewrite this test?

spec/PublicAPI.spec.js

spec/ValidationAndPasswordsReset.spec.js

src/Routers/PublicAPIRouter.js

spec/PublicAPI.spec.js

moonion · 2019-02-22T12:20:09Z

Thank you, will be on it next week.

This reverts commit 68ee2c4.

dplewis · 2019-03-14T19:52:02Z

@acinader @moonion A recent PR #5399 was causing this PR to fail. This has been addressed.

Also Cannot read "message" of undefined. error was fixed

acinader

some questions @dplewis to get up to speed.

acinader · 2019-03-14T19:57:09Z

src/Controllers/UserController.js

@@ -246,7 +246,7 @@ export class UserController extends AdaptableController {
    return this.checkResetTokenValidity(username, token)
      .then(user => updateUserPassword(user.objectId, password, this.config))
      .catch(error => {
-        if (error.message) {
+        if (error && error.message) {


why would we be in a catch and have no error? that seems bad.

I had the same feeling. I could back track and see what the underlying cause is.

yeah, we shouldn't throw without a message.

Just committed a fix

src/Routers/PublicAPIRouter.js

acinader · 2019-03-14T20:40:32Z

src/Controllers/UserController.js

@@ -90,7 +90,7 @@ export class UserController extends AdaptableController {
      )
      .then(results => {
        if (results.length != 1) {
-          throw undefined;


did a little git blame sleuthing on this one.

git show 7e868b2

note the comment that used to be there:

// Trying to verify email when not enabled // TODO: Better error here.

I didn't even notice that.

* adapted public api route for use with ajax * Elegant error handling * Fixed error return * Public API error flow redone, tests * Fixed code to pre-build form * Public API change password return params * Reverted errors in resetPassword * Fixed querystring call * Success test on ajax password reset * Added few more routes to tests for coverage * More tests and redone error return slightly * Updated error text * Console logs removal, renamed test, added {} to if * Wrong error sent * Revert changes * Revert "Revert changes" This reverts commit 68ee2c4. * real revert of {} * nits and test fix * fix tests * throw proper error

moonion and others added 2 commits January 30, 2019 18:38

Merge pull request #3 from parse-community/master

3aefd64

Sync

adapted public api route for use with ajax

6481a5a

moonion closed this Jan 30, 2019

moonion reopened this Jan 30, 2019

moonion changed the title ~~Ajax password reset~~ WIP: Ajax password reset Jan 30, 2019

moonion changed the title ~~WIP: Ajax password reset~~ Ajax password reset Jan 30, 2019

flovilmart suggested changes Jan 30, 2019

View reviewed changes

src/Routers/PublicAPIRouter.js Outdated Show resolved Hide resolved

Elegant error handling

2d50f20

Fixed error return

b0e8f3d

Public API error flow redone, tests

d2f3101

Fixed code to pre-build form

8e0e258

Public API change password return params

3c914df

Reverted errors in resetPassword

3a7e0f7

Fixed querystring call

a9828cc

Updated error text

f51db6c

dplewis suggested changes Feb 22, 2019

View reviewed changes

Vladyslav Chygrinov and others added 8 commits March 1, 2019 12:51

Console logs removal, renamed test, added {} to if

0e86783

Wrong error sent

c0688b3

Revert changes

68ee2c4

Revert "Revert changes"

a8cb050

This reverts commit 68ee2c4.

real revert of {}

4bf3667

nits and test fix

6f82c0b

Merge remote-tracking branch 'upstream/master' into ajax-password-reset

168e5ed

fix tests

5342a18

dplewis requested a review from acinader March 14, 2019 19:49

dplewis mentioned this pull request Mar 14, 2019

Add possibility to use async requests on choose password #5418

Closed

acinader reviewed Mar 14, 2019

View reviewed changes

throw proper error

a8d9504

acinader reviewed Mar 14, 2019

View reviewed changes

acinader approved these changes Mar 14, 2019

View reviewed changes

dplewis merged commit d84566a into parse-community:master Mar 14, 2019

This was referenced Apr 3, 2019

fix for PublicApiRouter.js #4538

Closed

checkResetTokenValidity throws undefined #4267

Closed

mtrezza mentioned this pull request Jul 27, 2020

Improve reset password API #6817

Open

snyk-bot mentioned this pull request Jul 14, 2021

[Snyk] Upgrade apollo-server-express from 2.25.1 to 2.25.2 #7465

Merged

Ajax password reset #5332

Ajax password reset #5332

Conversation

moonion commented Jan 30, 2019 • edited Loading

flovilmart commented Jan 30, 2019

moonion commented Jan 30, 2019 • edited Loading

flovilmart commented Jan 30, 2019 • edited Loading

flovilmart left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 30, 2019 • edited Loading

Codecov Report

flovilmart commented Jan 31, 2019

moonion commented Feb 5, 2019

flovilmart commented Feb 5, 2019 • edited Loading

moonion commented Feb 5, 2019

moonion commented Feb 5, 2019

flovilmart commented Feb 5, 2019 • edited Loading

moonion commented Feb 5, 2019

moonion commented Feb 5, 2019

moonion commented Feb 5, 2019 • edited Loading

flovilmart commented Feb 5, 2019

moonion commented Feb 5, 2019 • edited Loading

flovilmart commented Feb 5, 2019

moonion commented Feb 6, 2019

moonion commented Feb 8, 2019

flovilmart commented Feb 8, 2019

moonion commented Feb 8, 2019

moonion commented Feb 18, 2019

flovilmart commented Feb 18, 2019

dplewis left a comment

Choose a reason for hiding this comment

dplewis Feb 21, 2019

Choose a reason for hiding this comment

moonion Mar 1, 2019

Choose a reason for hiding this comment

moonion commented Feb 22, 2019

dplewis commented Mar 14, 2019

acinader left a comment

Choose a reason for hiding this comment

acinader Mar 14, 2019

Choose a reason for hiding this comment

dplewis Mar 14, 2019

Choose a reason for hiding this comment

acinader Mar 14, 2019

Choose a reason for hiding this comment

dplewis Mar 14, 2019

Choose a reason for hiding this comment

acinader Mar 14, 2019 • edited Loading

Choose a reason for hiding this comment

dplewis Mar 14, 2019

Choose a reason for hiding this comment

moonion commented Jan 30, 2019 •

edited

Loading

moonion commented Jan 30, 2019 •

edited

Loading

flovilmart commented Jan 30, 2019 •

edited

Loading

codecov bot commented Jan 30, 2019 •

edited

Loading

flovilmart commented Feb 5, 2019 •

edited

Loading

flovilmart commented Feb 5, 2019 •

edited

Loading

moonion commented Feb 5, 2019 •

edited

Loading

moonion commented Feb 5, 2019 •

edited

Loading

acinader Mar 14, 2019 •

edited

Loading