8000 refactor: Reset password flow by ankush · Pull Request #24857 · frappe/frappe · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

refactor: Reset password flow #24857

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

Merged
merged 4 commits into from
Feb 11, 2024
Merged

Conversation

ankush
Copy link
Member
@ankush ankush commented Feb 11, 2024
  • Hash one time reset tokens instead of storing them as is. We send actual key via email but only store hash of it. Verification happens against hash.
  • Up the perm level
  • Use better source of randomness for generating token
  • minor code cleanup here and there

TODO:

  • redo tests
  • fix reset pw UI, doesn't handle these use cases:
    • password managers
    • no password policy in system settings - breaks entire page

@ankush ankush requested review from a team and shariquerik and removed request for a team February 11, 2024 07:47
@ankush ankush added backport version-14-hotfix backport to version 14 backport version-15-hotfix Backport the PR to v15 labels Feb 11, 2024
@ankush ankush marked this pull request as draft February 11, 2024 07:52
@ankush ankush force-pushed the refactor_password_reset branch from dd82e8b to 34f6761 Compare February 11, 2024 07:52
- Hash one time reset tokens instead of storing them as is
- Up the perm level
- Use better source of randomness for generating token
- minor code cleanup here and there
@ankush ankush force-pushed the refactor_password_reset branch from 34f6761 to 38565a8 Compare February 11, 2024 09:01
@ankush ankush marked this pull request as ready for review February 11, 2024 09:37
- password managers use paste, added that event to trigger our code
- if no password policy then skip. Way too much needless business logic in HTML here.
@ankush ankush force-pushed the refactor_password_reset branch from cdfa46a to 78264a7 Compare February 11, 2024 11:00
@ankush ankush requested review from sagarvora and removed request for shariquerik February 11, 2024 11:01
@barredterra
Copy link
Collaborator

Did you remove the tour definitions on purpose?

@barredterra
Copy link
Collaborator

Tested the functionality - LGTM

@ankush
Copy link
Member Author
ankush commented Feb 11, 2024

Yeah tour removal is on purpose. Kinda broken after Espresso rework and also no one completes them based on telemetry data. We are working on other ways to improve onboarding.

@ankush ankush merged commit d0de5da into frappe:develop Feb 11, 2024
@ankush ankush deleted the refactor_password_reset branch February 11, 2024 15:26
ankush added a commit that referenced this pull request Feb 19, 2024
* chore: remove default UI tours

Experiment 🤷
(cherry picked from commit 74071c1)

* refactor: Reset password flow

- Hash one time reset tokens instead of storing them as is
- Up the perm level
- Use better source of randomness for generating token
- minor code cleanup here and there

(cherry picked from commit 4c925e0)

# Conflicts:
#	frappe/core/doctype/user/user.json
#	frappe/core/doctype/user/user.py
#	frappe/patches.txt
#	frappe/tests/test_utils.py

* test: redo reset password tests

(cherry picked from commit 38565a8)

# Conflicts:
#	frappe/core/doctype/user/test_user.py

* fix(UX): usability of reset password page

- password managers use paste, added that event to trigger our code
- if no password policy then skip. Way too much needless business logic in HTML here.

(cherry picked from commit 78264a7)

* chore: conflicts

* debug

---------

Co-authored-by: Ankush Menat <ankush@frappe.io>
ankush added a commit that referenced this pull request Feb 19, 2024
* chore: remove default UI tours

Experiment 🤷
(cherry picked from commit 74071c1)

* refactor: Reset password flow

- Hash one time reset tokens instead of storing them as is
- Up the perm level
- Use better source of randomness for generating token
- minor code cleanup here and there

(cherry picked from commit 4c925e0)

# Conflicts:
#	frappe/core/doctype/user/user.json
#	frappe/core/doctype/user/user.py
#	frappe/patches.txt
#	frappe/tests/test_utils.py

* test: redo reset password tests

(cherry picked from commit 38565a8)

# Conflicts:
#	frappe/core/doctype/user/test_user.py
#	frappe/tests/utils.py

* fix(UX): usability of reset password page

- password managers use paste, added that event to trigger our code
- if no password policy then skip. Way too much needless business logic in HTML here.

(cherry picked from commit 78264a7)

* chore: conflicts

---------

Co-authored-by: Ankush Menat <ankush@frappe.io>
6F35
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport version-14-hotfix backport to version 14 backport version-15-hotfix Backport the PR to v15
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0