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

Old password is not required for signed in users to change password #47

Merged
merged 2 commits into from
Dec 8, 2021

Conversation

elitan
Copy link
Contributor

@elitan elitan commented Nov 26, 2021

hasura-auth-js PR: nhost/hasura-auth-js#7

Copy link
Contributor

@nunopato nunopato left a comment

Choose a reason for hiding this comment

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

Are there any cases where it would make sense to provide the old password? if yes, then we should consider making it optional? If not, this is good to go 👍

@elitan
Copy link
Contributor Author

elitan commented Nov 30, 2021

Yea maybe making the oldPassword optional is something we could do.

My reasoning is, that I foresee us refactoring this password-change feature in the near future to support:

  1. oldPassword - for people who know their old password
  2. Some ticket - that comes from the fact that the user clicks on the password reset link in the email.

Basically, the user can change password if they either know their old password or recently clicked on a password reset link. Not, just by the fact that they are signed in.

However, having the oldPassword optional feels a bit... strange.

My intuition is that we remove the oldPassword parameter for now, to avoid optimizing for the future before we really know what route we want to go.

What do you think is the better approach here @nunopato ?

For reference: supabase/supabase#4042

@nunopato
Copy link
Contributor

That sounds reasonable 🚢

@elitan elitan merged commit c328aa7 into main Dec 8, 2021
@elitan elitan deleted the NHOST-156 branch December 8, 2021 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants