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

Add account unlock on password reset #7146

Merged

Conversation

mtrezza
Copy link
Member

@mtrezza mtrezza commented Jan 28, 2021

New Pull Request Checklist

Issue Description

Resetting the password of an account does not lift the account lock, which is unusual behavior compared to major online services and therefore may cause confusion on the user side.

Related issue: closes #6773

Approach

Added new account policy that allows to automatically unlock an account after successfully resetting the password.

accountLockout: {
  unlockOnPasswordReset: true // Is true if the account lock should be removed after a successful password reset. Default: false
}

This is added as a policy option to account for different security policies, depending on use case.

TODOs before merging

  • Add test cases
  • Add Parse Server account policy option
  • Add entry to changelog
  • Add changes to documentation (guides, repository pages, in-code descriptions)

@codecov
Copy link

codecov bot commented Jan 28, 2021

Codecov Report

Merging #7146 (0a76ac8) into master (857d4ec) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7146   +/-   ##
=======================================
  Coverage   93.92%   93.92%           
=======================================
  Files         169      169           
  Lines       12535    12547   +12     
=======================================
+ Hits        11773    11785   +12     
  Misses        762      762           
Impacted Files Coverage Δ
src/Options/Definitions.js 100.00% <ø> (ø)
src/Options/index.js 100.00% <ø> (ø)
src/AccountLockout.js 97.56% <100.00%> (+0.19%) ⬆️
src/Config.js 91.22% <100.00%> (+0.26%) ⬆️
src/Controllers/SchemaCache.js 100.00% <100.00%> (ø)
src/Controllers/UserController.js 95.27% <100.00%> (+0.15%) ⬆️
src/RestWrite.js 93.67% <0.00%> (-0.17%) ⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 95.93% <0.00%> (-0.16%) ⬇️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 93.51% <0.00%> (+0.67%) ⬆️

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 857d4ec...8539315. Read the comment docs.

@mtrezza mtrezza requested review from a team and davimacedo January 28, 2021 15:31
@Moumouls
Copy link
Member

I'll take a look asap 🙂

@mtrezza mtrezza requested a review from dplewis January 28, 2021 20:53
env: 'PARSE_SERVER_WEBHOOK_KEY',
help: 'Key sent with outgoing webhook calls',
},
"accountLockout": {
Copy link
Member

@davimacedo davimacedo Jan 29, 2021

Choose a reason for hiding this comment

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

It is a minor item, but why do we have all these changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, these changes actually seem to occur for me on npm run definitions. The file is then built with double quotes. Could you try this out on your side, if that happens as well?

Copy link
Member Author

@mtrezza mtrezza Jan 30, 2021

Choose a reason for hiding this comment

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

It seems that the generator already adds the double quotes before writing to file, as I can see when inspecting the res output:

const res = babel.transformFileSync('./src/Options/index.js', { plugins: [ plugin, '@babel/transform-flow-strip-types' ], babelrc: false, auxiliaryCommentBefore, sourceMaps: false });

This babel issue describes that the quotes option has also been removed and babel outputs with double quotes. So much about babel's side at least.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, when I run npm run prettier the double quotes are removed. That also affects some other files though, which are not related to this PR. What is the current workflow - should we have prettier run as a pre-commit hook? It seems that currently a PR is not guaranteed to be merged with prettier, if the author did not manually run it?

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.

LGTM!

@mtrezza
Copy link
Member Author

mtrezza commented Jan 30, 2021

Not merging yet, looking into definitions quotes changes.

Update: Solved -- ready for review.

@mtrezza mtrezza merged commit 08b2ea4 into parse-community:master Feb 1, 2021
dplewis pushed a commit that referenced this pull request Feb 21, 2021
* added account unlock on password reset

* added account policy option

* added changelog entry

* Added docs entry

* moved changelog entry to correct position

* improved tests to ensure requesting password reset email does not unlock account

* run prettier
@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
@mtrezza mtrezza mentioned this pull request Mar 12, 2022
@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
@mtrezza mtrezza deleted the add-account-unlock-on-password-reset branch March 24, 2022 18:38
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.

FR: successful password reset should unlock account locked via accountLockout policy
4 participants