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

GraphQL: reset password with emailed token #7290

Merged

Conversation

sadakchap
Copy link
Member

New Pull Request Checklist

Issue Description

Related issue: resetting password with emailed token

Approach

  • add one more mutation in userMutation.js file that will take username, password, token as arguments.
  • add one more route in userRoutes.js file, that will add route POST /resetPassword that will basically call this updatePassword function

and with this, I think we should be able to complete reset password flow for GraphQL API also

TODOs before merging

  • Add test cases
  • Add entry to changelog
  • Add changes to documentation (guides, repository pages, in-code descriptions)
  • Add security check
  • Add new Parse Error codes to Parse JS SDK

@codecov
Copy link

codecov bot commented Mar 21, 2021

Codecov Report

Merging #7290 (1f95338) into master (a05e9b1) will decrease coverage by 0.00%.
The diff coverage is 70.58%.

❗ Current head 1f95338 differs from pull request most recent head c427ca9. Consider uploading reports for the commit c427ca9 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7290      +/-   ##
==========================================
- Coverage   93.91%   93.90%   -0.01%     
==========================================
  Files         179      179              
  Lines       13154    13168      +14     
==========================================
+ Hits        12353    12365      +12     
- Misses        801      803       +2     
Impacted Files Coverage Δ
src/Adapters/Storage/Postgres/PostgresClient.js 70.00% <0.00%> (+3.33%) ⬆️
src/GraphQL/loaders/usersMutations.js 91.20% <80.00%> (-2.22%) ⬇️
src/batch.js 91.37% <0.00%> (-1.73%) ⬇️
src/RestWrite.js 93.84% <0.00%> (-0.17%) ⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 95.52% <0.00%> (+0.15%) ⬆️

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 a05e9b1...c427ca9. Read the comment docs.

Copy link
Member

@davimacedo davimacedo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

Please find a comment below.

Also, could you please add a test case?

src/Routers/UsersRouter.js Outdated Show resolved Hide resolved
@sadakchap
Copy link
Member Author

@davimacedo about writing a test case, for faking email adaptor, I can see that in some test cases we are doing something like this

const emailAdapter = {
  sendVerificationEmail: () => {},
  sendPasswordResetEmail: () => Promise.resolve(),
  sendMail: () => {},
};

// reconfiguring Server 
reconfigureServer({
  appName: 'test',
  emailAdapter: emailAdapter,
  publicServerURL: 'http://localhost:8378/1',
})

But still, how we will get the token, to actually test resetPassword mutation?

@davimacedo
Copy link
Member

Instead of sendPasswordResetEmail: () => Promise.resolve(),, you can do:

sendPasswordResetEmail: ({ appName, link, user }) => { // You can now, within this scope, retrieve the token from the link, test the new mutation, try to login with the new password, and finally call done() to finish the test },

@sadakchap
Copy link
Member Author

Instead of sendPasswordResetEmail: () => Promise.resolve(),, you can do:

sendPasswordResetEmail: ({ appName, link, user }) => { // You can now, within this scope, retrieve the token from the link, test the new mutation, try to login with the new password, and finally call done() to finish the test },

@davimacedo , I tried adding a test case as suggested. Please let me know if any changes are required.

Copy link
Member

@davimacedo davimacedo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks all good to me! @mtrezza this is a breaking change as we are changing the name os one of the current mutations. How should we proceed with that?

@mtrezza
Copy link
Member

mtrezza commented Mar 26, 2021

@davimacedo that depends on the reason for renaming resetPasswordMutation to requestResetPasswordMutation.

  • If this is just cosmetic, I would ask whether the change is really necessary. A cosmetic change would not merit a breaking change, unless part of a larger overhaul. I would leave that to change organically in the future, for example as part of a larger feature change.

  • If the change is justified, then it should be phased-in. If that is "over the top" for a simple mutation renaming, I would ask again whether the change is really necessary. If yes, the change would be introduced with the next major release.

If the change is necessary, we could give this a pass and just merge it into master. We don't have the new branch structure set up yet and it doesn't seem to merit a new Parse Server option. From 5.0.0 onwards, with the new branch structure, it would be handled differently.

@davimacedo
Copy link
Member

It is actually kinda cosmetic. Let me explain. Currently we have a mutation called resetPassword that does not actually reset the password, but only sends the e-mail. This PR changes the name of this mutation to requestResetPassword and creates another mutation with the name resetPassword that receives the token (sent by e-mail) and the new password and actually resets the password. With this new mutation someone will be able to create a custom reset password form that could use the GraphQL API (and not the REST API) to complete flow. It will close the issue #7033.

But, maybe, to avoid the breaking change, we could keep the current mutation with the name resetPassword and create the new one with the name confirmResetPassword or something like this. The only advantage in changing the names is to be closer to what we currently have in the REST API and SDKs. What do you think?

@mtrezza
Copy link
Member

mtrezza commented Mar 27, 2021

To be pragmatic, am fine either way.

Actually, the current endpoints for password reset and email verification will be renamed with the new PagesRouter. The current endpoints are not semantically consistent. So aligning it probably isn't worth it for now. The new names are not defined yet, I will do that when the PagesRouter goes out of experimental state.

My suggestion is to not make this a breaking change for now and rename all endpoints (and mutations) in one go with 6.0.0. But I'm also fine with adding this breaking change to the master now, if you rather do that.

Up to release 5.0.0 I think we can be tolerant about breaking changes. I see it as a grace period until we fully switched to the new release system. From 5.0.0 onwards, we should follow the proposed versioning system and phased deprecation.

@davimacedo
Copy link
Member

I agree. @sadakchap would you mind to change the mutations names to resetPassword and confirmResetPassword?

@sadakchap
Copy link
Member Author

Right, @davimacedo changed the names of mutation both in userMutation file & test also.

@davimacedo davimacedo merged commit 5d9bf24 into parse-community:master Mar 29, 2021
Arul- pushed a commit to Arul-/parse-server that referenced this pull request Mar 29, 2021
* renamed "resetPassword" to "requestResetPassword" & created new "resetPassword" mutation

* added new route to handle resetPassword in UsersRouter.js

* updated resetPassword test to "requestResetPassword" mutation

* updated "resetPassword" mutation args description

* changed token arg description to rerun the tests

* directly using updatePassword for resetPassword

* removed handleResetPassword from UsersRouter.js file

* added test case for reset Password

* changed mutation names to "resetPassword" & "confirmResetPassword"

* changed mutation names in test also
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0-beta.1

@parseplatformorg parseplatformorg added the state:released-beta Released as beta version label Nov 1, 2021
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0

@parseplatformorg parseplatformorg added the state:released Released as stable version label Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:released Released as stable version state:released-beta Released as beta version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants