parse-community / parse-server

Parse Server for Node.js / Express
https://parseplatform.org
Apache License 2.0
20.82k stars 4.77k forks source link

Add password reset initiated via web form #7210

Open mman opened 3 years ago

mman commented 3 years ago

New Feature / Enhancement Checklist

Current Limitation

When a user needs to change his/her password, a POST request has to be performed to the REST endpoint /parse/requestPasswordReset with appropriately filled HTTP headers, namely X-Parse-Application-Id and X-Parse-REST-API-Key.

This is typically handled via JS, iOS, or Android client side SDK, and can not be triggered via plain HTML email or HTML webpage by simply using a form, without resorting to JavaScript XHR.

Example Use Case

  1. User receives an HTML email with simple button saying: To change your password, click the button below.
  2. The button click invokes simple HTTP form POST request with username filed to a parse-server.
  3. User is redirected to a webpage saying: Instructions to reset your password were sent to your email address.

Feature / Enhancement Description

I would like to offer my users a feature where they can change their password by clicking a link directly from HTML email, by simply HTTP POST-ing their username (email address) to some API endpoint to avoid use of client side JavaScript.

After they click the link, they should be redirected to a page informing them that the instructions to reset the password were sent to their email address.

Looking at the current state of https://github.com/parse-community/parse-server/blob/master/src/Routers/PublicAPIRouter.js I propose to modify POST to /request_password_reset to start the password reset flow when only username is present.

This is in line with how the /resend_verification_email endpoint works.

The functionality will then be as follows:

  1. HTML email uses a form and button that does POST /request_password_reset that requires username. Parse Server generates password reset token in a db, sends password reset email with username, and token, and redirects to password_reset_initiated.html.
  2. password reset email contains button with link to: GET /request_password_reset with requires username, and token, and redirects to choose_new_password.html
  3. choose_new_password.html form prompts for new password, and submits to:
  4. POST /request_password_reset with username, token, and new_password.
  5. Password is changed in a db, when token valid, and user is redirected to password_changed.html, or to invalid_link.html when token already expired.

Example implementation is provided here: https://github.com/parse-community/parse-server/pull/7207

Alternatives / Workarounds

The only alternative I am aware of is to use client side JavaScript and XHR to trigger the password reset flow by posting to /parse/requestPasswordReset endpoint, handling the response, and changing the HTML DOM appropriately to indicate that instructions were sent to email address.

mtrezza commented 3 years ago

Thanks for suggesting.

There are some parts I want to get clarified, it may just be the wording, but to make sure we approach this the right way:

The button click invokes simple HTTP form POST request (...) HTML email uses a form and button that does POST /request_password_reset that requires username.

A link in an email cannot practically invoke a POST request. I say practically because, while it is technically possible to add JS or a form to a HTML email, it will likely be blocked by every major email provider and email client. In addition, such an email has a higher SPAM score and is likely to harm your email domain reputation. In addition, consider users whose email client may fall back to plain-text.

The functionality will then be as follows:

  • HTML email uses a form and button (...)
  • password reset email contains button with link (...)

The desired flow indicates a likely flaw in your approach, where you describe a user clicking on a link in an email just to receive yet another email with a link to click on.

This seems like a XY problem where you go from an approach to looking for possible implementations, instead of going from a problem to looking for possible approaches. I suggest to zoom out and try to describe the scenic context in which you want a user to reset their password.

mman commented 3 years ago

I am actually using the afterLogin hook to inform the user about new login to their account, with a wording like "If you do not recognize this login, please change your password immediately by clicking the button/link below".

My initial attempt with the button/link clicked in an email actually needs to either POST submit a <form> to initiate the password reset. If that would not work or provide to be tricky with spam filters, I will replace it with a simple link to a webpage from where a user can initiate their password reset.

I want to do this as much as possible without requiring client side JavaScript to make thinks simple.

mman commented 3 years ago

I agree that the flow is rather complex, but it is based on what Parse Server provides right now.

It would be kind of nice and simpler when afterLogin with wording "If you do not recognize this login, please change your password immediately by clicking the button/link below" would already contain a password reset token and would redirect a user to the "choose-new-password.html" form.

But that would require generating fresh token (or reusing a valid one) for password reset with each afterLogin, which I do not think is the right approach.

How would you handle this? Martin

mtrezza commented 3 years ago

Thanks for providing more details. I assume this is the flow you would prefer:

Case a):

  1. A login happens.
  2. User receives an email "If you do not recognize this login..." with a link.
  3. User clicks the link and gets to a webpage where they can reset their password.
  4. User submits the form and the password is changed.

Then there is another use case, that could be considered:

Case b):

  1. User is logged in on a website using Parse.
  2. User navigates to a password reset webpage.
  3. User submits the form with a new password and password is changed.

The second use case may not seem to apply to yours at first sight, but it faces the same issue, that a password reset form requires a pre-generated token.

The difference between both cases is:

Possible approaches:

mman commented 3 years ago

My case is always case a) as I only use Parse Server via iOS and Android apps with their client side SDK.

Initiating password reset from an app via client side SDK is easy, but requires the afterLogin email to say something like: "If you do not recognize this login, open the app on one of your devices and do X, Y, and Z to change your password".

Which is not ideal and generates support requests where people can not find X, do not know what Y is, etc... I can improve this in the app, that is correct. But for now - sending an email with a simple link and clear flow sounds like a better approach to me.

In other words, I do not plan to develop a fully fledged client side web app handling account related things with Parse Server, yet even using Parse Server in a mobile app still at the moment provides certain valid email based use cases, like email validation, and password reset performed by showing an ownership of the email address.

I also think this is absolutely fine, since many apps and websites use these types of flow, so they are very familiar to most people nowadays.

mtrezza commented 3 years ago

Can you please provide a complete step-by-step (1,2,3...) example of the flow you are referring to and you want to achieve with Parse Server?

mman commented 3 years ago
  1. User performs a login to an account via app (does not matter which, iOS, Android, web, ...)
  2. New Login Detected email is triggered via parse-server afterLogin hook containing IP address, physical location, date & time, etc. describing what happened. And containing a phrase: "If this was you, please disregard this email. If you do not recognize this login, please change your password immediately by clicking the link/button below"
  3. User clicks the link/button below and is taken to the browser where he can either variant a. enter new password and click "Change Password" button variant b. click the "Request Password Reset" button to initiate password reset.
  4. Result variant a. Redirected to "your password was successfully changed" (or invalid link) variant b. Redirected to "instructions to reset your password were sent to your email". The email then sent contains the link that goes to 3.a

Neither variant a, nor variant b are at the moment possible to do easily without JavaScript talking client side via XHR to the REST API.

I am currently trying to implement variant b which may sound like more steps but is simpler to implement.

Variant a requires less steps but afterLogin hook sending New Login Detected email would need to already contain a valid password reset token, which I am not sure I can generate via cloud code.

Does my explanation make any sense?

The email verification flow, as well as password reset flow in parse server currently does not contain any HTML based way to start the process. Verification email is sent automatically after signup performed via REST API, and password reset email is sent automatically after performing POST to REST API endpoint /parse/requestPasswordReset.

Then the flow continues via series of 302 redirects and GET/POST requests going to /parse/apps/APP_ID/verify_email and GET/POST requests going to /parse/apps/APP_ID/request_password_reset. This is using the publicServerURL and customPages (soon to be replaced with the PagesRouter).

Recently, an endpoint was added to POST /parse/apps/APP_ID/resend_verification_email so that verification process can be initiated without going to REST API. This is at the moment used to trigger the re-send verification email when receiving an "email verification token has expired" email. Code here: https://github.com/parse-community/parse-server/blob/c05102b90c925498459b45c755298cb482ac596c/public/email_verification_link_expired.html#L17

I am proposing the same, to initiate password reset with a similar form as here, but using the request_password_reset endpoint: https://github.com/parse-community/parse-server/blob/c05102b90c925498459b45c755298cb482ac596c/public/email_verification_link_expired.html#L17

mtrezza commented 3 years ago

Step 3a):

Step 3b):

Generally, we want to follow best practice that provides a flow that is familiar for users. I looked into Instagram as an example and found:

That would conclude we should go with the current concept of pre-generating a token. Maybe a disadvantage we have here is that we do not use a TTL index to delete unused tokens, so they stay in the DB unless invalidated.

Now, to address the use case you described, I suggest to create a password reset token in the after afterLogin trigger. The solution could be to create a PR that adds a method like const link = Parse.User.generatePasswordResetLink(user, { useMasterKey: true }); that returns a URL, which also generates a password reset token. That link can then be used in a custom email template. Such a PR would not touch the PagesRouter (or PublicAPIRouter) but merely create an interface to the UserController.

@mman Would that address your use case?

mman commented 3 years ago

Thanks @mtrezza for the detailed proposal.

We are on the same page here.

My initial idea, and PR implementation goes with 3b) and POST. It does work, it does not break RFC 2616, uses the same method like /resend_verification_email, and was super simple to implement. And in addition to that, it offers a simple endpoint to reset passwords for the user. So when somebody asks me how do I change my password, I can direct them to a simple web form to do that without requiring full fledged account management and web app (note: we do not have a web app behind parse, only iOS and Android apps).

The instagram like solution where password reset token is generated in afterLogin sounds good to me as well, but touches areas unfamiliar to me.

I see only one drawback there:

  1. When first login happens, a password reset token would be generated valid for several hours.
  2. When another login happens immediately after that, a new password reset token will be generated and will invalidate the one generated in step 1, which IMHO should not happen.

The proper solution to this problem should probably be to just extend password reset validity period in case where a valid password reset token exists for a user. And this will require some more changes.

Minus of the instagram like solution will be that there is no simple endpoint to initiate password reset from the web. That will still require manual coding outside of parse-server.

What do you think?

I will investigate instagram like solution during today, but I'm still in favor of including the step 3b) solution as outlined in the PR https://github.com/parse-community/parse-server/pull/7207.

mtrezza commented 3 years ago

it does not break RFC 2616

How did you conclude that?

Another other issue with 3b) is that it basically circumvents the security mechanism of the password reset token and makes it useless, because it will only be generated as a by-product. If we allow an endpoint as suggested in 3b), we are short circuiting the token mechanism, which would be the same as removing the feature.

When another login happens immediately after that, a new password reset token will be generated and will invalidate the one generated

Tokens can be reused as long as they are valid.

mman commented 3 years ago

it does not break RFC 2616

How did you conclude that?

I meant that to point out that I went with POST instead of GET because POST that triggers password reset flow is not idempotent. Much like /resend_verification_email.

When another login happens immediately after that, a new password reset token will be generated and will invalidate the one generated

Tokens can be reused as long as they are valid.

Oh cool, thanks for the link. I did not catch this PR, let me take a look and draft something along this alternative solution.

mman commented 3 years ago

Now, to address the use case you described, I suggest to create a password reset token in the after afterLogin trigger. The solution could be to create a PR that adds a method like const link = Parse.User.generatePasswordResetLink(user, { useMasterKey: true }); that returns a URL, which also generates a password reset token. That link can then be used in a custom email template.

@mtrezza Just to clarify, adding a method to Parse.User would require modification of Parse JS SDK (https://github.com/parse-community/Parse-SDK-JS/blob/master/src/ParseUser.js) which is also available client side in the browser.

My opinion is that better fit would be adding a method to the UserController.js (https://github.com/parse-community/parse-server/blob/master/src/Controllers/UserController.js) where the token generation and link building happens now.

Is my understanding correct? Martin

mtrezza commented 3 years ago

To summarize, I think we can conclude the following:

adding a method to Parse.User would require modification of Parse JS SDK (https://github.com/parse-community/Parse-SDK-JS/blob/master/src/ParseUser.js) which is also available client side in the browser. My opinion is that better fit would be adding a method to the UserController.js (https://github.com/parse-community/parse-server/blob/master/src/Controllers/UserController.js) where the token generation and link building happens now.

It needs to be implemented in both, or how would you access a new method in the user controller?

mman commented 3 years ago

My opinion is that better fit would be adding a method to the UserController.js (https://github.com/parse-community/parse-server/blob/master/src/Controllers/UserController.js) where the token generation and link building happens now.

It needs to be implemented in both, or how would you access a new method in the user controller?

The afterLogin trigger has access to the user and user.email. Since it is running in the cloud code on the parse-server itself, I should be able to access the userController instance like this (NOTE: not verified yet)

Parse.Cloud.afterLogin(async request => {
  const { object: user }  = request;
  const userController = AppCache.get('unused').userController;
  const mailgunAdapter = AppCache.get('unused').userController.adapter;

  // use userController to request passwordResetLink for user

  // need to await here since getPasswordResetLink may need to save newly generated token to mongo underneath
  const link = await userController.getPasswordResetLink(user)

  // use link in a template 
  return mailgunAdapter.send({
      templateName: 'newLoginEmail',
      recipient: user.email,
      variables: { username: user.email, date: Date().toString(), link: link }
    });
}
mtrezza commented 3 years ago

Well, the UserController is an internal module that is not part of our official documentation. For this new method to become an official, documented feature, it needs to be exposed via Parse.User for example.

mman commented 3 years ago

UserController is definitely internal. I agree.

However when you expose this method via Parse.User then it will be part of client side JavaScript SDK, and can be invoked from a client side web app where it will does nothing, right?

I agree that use of AppCache may be internal, and for a concrete example, the email sending was replaced via Parse.Cloud.sendMail recently (https://github.com/parse-community/parse-server/pull/7096/files) so perhaps best for this functionality will be to add a public method:

Parse.Cloud.getPasswordResetLink(user)

what do you think? Martin

mtrezza commented 3 years ago

However when you expose this method via Parse.User then it will be part of client side JavaScript SDK, and can be invoked from a client side web app where it will does nothing, right?

Yes, the endpoint should require the master key.

Parse.Cloud.getPasswordResetLink(user)

Since this is user specific, it may be more intuitive and concise to implement a Parse.User.createPasswordResetLink(), that would be called like this:

Parse.Cloud.afterLogin(async request => {
  const { object: user }  = request;
  const link = await user.createPasswordResetLink();
});

Maybe it would better be called create... instead of get..., because get may imply that it is a property that does not change, but the method may yield a different link every time it is called.

Also, it may have to be called with option user.createPasswordResetLink({ useMasterKey: true });, just due to formality, as I think we also do with some other methods that are CloudCode only.

mman commented 3 years ago

I may be missing something (not familiar with JS SDK, only Android and iOS), but how would Parse.User actually create a password reset link? It does not have access to config (to get custom URLs) it does not have access to Mongo - at least not directly and has to go via HTTP to the parse server itself?

As for get vs create, I also think create is better option.

mtrezza commented 3 years ago

It would be implemented much like the requestPasswordReset method that also takes RequestOptions and sends it to the server. The server ensures that the master key is present in the request, generates the token and returns the link.

This way, this could even be called client side, if someone decides to do that; we already have some methods that require the master key to discourage invoking them client side (such as push notifications), but in a special set-up, a developer may want to call this client side.

The process to implement this would be:

  1. PR for Parse Server to provide the endpoint
  2. PR for Parse JS SDK to provide the method that calls the endpoint

If you look at Parse.Cloud, it is currently only used to declare and start Cloud Code functions and jobs, so it seems it would be an anti-pattern if we started to add unrelated functions like Parse.Cloud.getPasswordResetLink()to this class. Hence I suggest to add this to Parse.User, also because it is a user specific functionality.

mman commented 3 years ago

Honestly, although I try to understand why you may prefer to attach the method to the Parse.User class, as opposed to Parse.Cloud class, I think this is getting out of control.

Parse.User class is shared between client and the server, and should include methods and properties that that are shared and useful on both client and the server.

Adding new method to generated direct password reset link there means:

That means 4 PRs, across 4 repositories, spending time of at least 4 reviewers/core contributors.

All I need is to include a simple "change your password now" HTML link in an email generated via CloudCode.afterLogin hook on the server. And I'd love to do it with Parse Server itself, without deploying any additional components, HTML pages, and servers just to HTTP POST to existing /requestPasswordReset endpoint.

I am all for simplicity and consistency, and I do not want to pollute any public APIs with anything more that necessary.

The Cloud Code API is currently neat and clean and simple. And no, it is not only used to to declare and start Cloud Code functions and jobs. It is used to specify beforeSave and afterSave hooks, to specify live query event related methods. To send email using configured adapter, to react to afterLogin.

The cloud code API has been and is evolving over time as needs arise.

From the architectural and security point of view, I do not believe I want the createPasswordResetLink method to be available client side. Why would it be useful there?

The whole point of secure password reset flow nowadays is that:

  1. anybody can trigger it assuming they know the email
  2. password reset token is sent using another secure channel whose ownership you have to prove (email)
  3. token is required to change the password and has limited time span to limit its usefulness should it get stolen.

Why would I want to skip 2 and get the token directly on the client? What purpose would it serve?

I'd agree that we can attach the method to the request.user object that is passed to the afterLogin hook, and I know there is a JavaScript trickery that can do this, but that would be super unclean.

mtrezza commented 3 years ago

I try to understand why you may prefer to attach the method to the Parse.User class, as opposed to Parse.Cloud class

Maybe you want to read the class description of Parse.Cloud to help you understand its purpose. Cloud Code triggers, Cloud Code jobs, Cloud Code functions, these are all function definitions of a similar kind.

Parse.User class is shared between client and the server, and should include methods and properties that that are shared and useful on both client and the server.

I think I understand now where your objection comes from, but this is incorrect. The Parse JS SDK is also an integral part of Parse Server, not solely a client SDK. You are comparing the Parse JS SDK to other client SDKs, which leads to wrong assumptions about whether the method belongs there. The SDK is also used for methods are only considered "internal" by the fact that they require the master key. Therefore it makes sense to add the method to Parse.User.

To send email using configured adapter, to react to afterLogin.

I am actually not sure that was a fully correct implementation, but I approved of that PR because we do not have a Parse.Mail controller (yet) and it was a generic method to send mail to anyone (not a specific user). However, this feature here requires a specific user and it invokes the UserController, therefore it seems appropriate to add it to Parse.User.

That means 4 PRs, across 4 repositories, spending time of at least 4 reviewers/core contributors.

It takes 2 PRs as I already wrote earlier. As long as the time is invested in a sustainable solution, there will be reviewers available. If someone down the road wishes to implement this for other client SDKs, they are free to do that. But again, this is not about making this method available client side via master key, that's just a side effect, it is about making it available internally in Parse Server via Parse.User.

Look, one of the benefits of a PR compared to your custom solution is that we evaluate this in a broad context. I had to dismiss your earlier suggestions because of security implications, RFC violation and out-of-scope use of Parse.Cloud class. You could have implemented this and it may have satisfied your immediate need, but that does not automatically make it an acceptable PR for Parse Server.

Maybe you are overestimating the effort because of 2 PRs, or do you need any help with how to go about this?

mtrezza commented 3 years ago

@mman https://github.com/parse-community/parse-server/issues/7230 may be an interesting PR for your case.

It allows you to create custom routes so you can implement a custom password reset flow, with or without token, on GET or POST, however you see fit for your use case, without requiring a PR.

mman commented 3 years ago

Thanks @mtrezza, I will take a look. I have put into production our previously discussed Parse.Cloud.getPasswordResetLink(email) which was super simple and allows me to embed the password reset token directly in the new login email generated via afterLogin trigger and it works great.

Since the code does not align with the way you'd like to see it implemented I am keeping it for now in my fork and will try to get it incorporated via Parse.User instead of Parse.Cloud in the following weeks.

I have yet to investigate how to best prepare the custom route to trigger the password reset, but from my quick look at #7230 it seems to be a matter of rendering a web form and forwarding a POST to /parse/requestPasswordReset upon reception on GET on my custom route. Is my understanding correct?

Also, I have migrated from customPages to your PagesRouter and it works nicely. There is one bug where the PagesRouter must be mounted on /apps otherwise it will break the password reset logic (since UserController.js seems to be ignoring the pages router mount path. I will dig into this deeper and file a bug once I fully understand what is happening.

thanks, Martin

mtrezza commented 3 years ago

will try to get it incorporated via Parse.User instead of Parse.Cloud in the following weeks.

That would be nice.

I will dig into this deeper and file a bug once I fully understand what is happening.

Great! Even just a test case would be helpful. Described in https://github.com/parse-community/parse-server/issues/7235 and fixed.