From 4c925e03253985266bc7a2b1cddc707aaa1f3df3 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 11 Feb 2024 12:22:44 +0530 Subject: [PATCH] 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 --- frappe/core/doctype/user/patches/__init__.py | 0 .../user/patches/hash_reset_password_tokens.py | 16 ++++++++++++++++ frappe/core/doctype/user/user.json | 4 +++- frappe/core/doctype/user/user.py | 16 +++++++++------- frappe/patches.txt | 3 ++- frappe/tests/test_utils.py | 10 ++++++++++ frappe/utils/data.py | 8 ++++++++ 7 files changed, 48 insertions(+), 9 deletions(-) create mode 100644 frappe/core/doctype/user/patches/__init__.py create mode 100644 frappe/core/doctype/user/patches/hash_reset_password_tokens.py diff --git a/frappe/core/doctype/user/patches/__init__.py b/frappe/core/doctype/user/patches/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/frappe/core/doctype/user/patches/hash_reset_password_tokens.py b/frappe/core/doctype/user/patches/hash_reset_password_tokens.py new file mode 100644 index 0000000000..85a7f5ec80 --- /dev/null +++ b/frappe/core/doctype/user/patches/hash_reset_password_tokens.py @@ -0,0 +1,16 @@ +import frappe +from frappe.utils.data import sha256_hash + + +def execute(): + """hash reset password tokens""" + + users = frappe.get_all("User", {"reset_password_key": ("is", "set")}, ["name", "reset_password_key"]) + for user in users: + frappe.db.set_value( + "User", + user.name, + "reset_password_key", + sha256_hash(user.reset_password_key), + update_modified=False, + ) diff --git a/frappe/core/doctype/user/user.json b/frappe/core/doctype/user/user.json index 889984a95e..894216a3e0 100644 --- a/frappe/core/doctype/user/user.json +++ b/frappe/core/doctype/user/user.json @@ -329,6 +329,7 @@ "hidden": 1, "label": "Reset Password Key", "no_copy": 1, + "permlevel": 1, "print_hide": 1, "read_only": 1 }, @@ -627,6 +628,7 @@ "fieldtype": "Datetime", "hidden": 1, "label": "Last Reset Password Key Generated On", + "permlevel": 1, "read_only": 1 }, { @@ -771,7 +773,7 @@ "link_fieldname": "user" } ], - "modified": "2024-02-09 19:34:51.081079", + "modified": "2024-02-11 13:16:29.574666", "modified_by": "Administrator", "module": "Core", "name": "User", diff --git a/frappe/core/doctype/user/user.py b/frappe/core/doctype/user/user.py index 97c5a503c8..4f5009b9c8 100644 --- a/frappe/core/doctype/user/user.py +++ b/frappe/core/doctype/user/user.py @@ -31,6 +31,7 @@ from frappe.utils import ( now_datetime, today, ) +from frappe.utils.data import sha256_hash from frappe.utils.deprecations import deprecated, deprecation_warning from frappe.utils.password import check_password, get_password_reset_limit from frappe.utils.password import update_password as _update_password @@ -404,10 +405,11 @@ class User(Document): pass def reset_password(self, send_email=False, password_expired=False): - from frappe.utils import get_url, random_string + from frappe.utils import get_url - key = random_string(32) - self.db_set("reset_password_key", key) + key = frappe.generate_hash() + hashed_key = sha256_hash(key) + self.db_set("reset_password_key", hashed_key) self.db_set("last_reset_password_key_generated_on", now_datetime()) url = "/update-password?key=" + key @@ -948,8 +950,9 @@ def _get_user_for_update_password(key, old_password): # verify old password result = frappe._dict() if key: + hashed_key = sha256_hash(key) user = frappe.db.get_value( - "User", {"reset_password_key": key}, ["name", "last_reset_password_key_generated_on"] + "User", {"reset_password_key": hashed_key}, ["name", "last_reset_password_key_generated_on"] ) result.user, last_reset_password_key_generated_on = user or (None, None) if result.user: @@ -1041,11 +1044,10 @@ def sign_up(email: str, full_name: str, redirect_to: str) -> tuple[int, str]: @frappe.whitelist(allow_guest=True) @rate_limit(limit=get_password_reset_limit, seconds=60 * 60) def reset_password(user: str) -> str: - if user == "Administrator": - return "not allowed" - try: user: User = frappe.get_doc("User", user) + if user.name == "Administrator": + return "not allowed" if not user.enabled: return "disabled" diff --git a/frappe/patches.txt b/frappe/patches.txt index e2a0f3e849..a09008d921 100644 --- a/frappe/patches.txt +++ b/frappe/patches.txt @@ -233,4 +233,5 @@ frappe.patches.v15_0.set_file_type frappe.core.doctype.data_import.patches.remove_stale_docfields_from_legacy_version frappe.patches.v15_0.validate_newsletter_recipients frappe.patches.v15_0.sanitize_workspace_titles -frappe.patches.v15_0.migrate_role_profile_to_table_multi_select \ No newline at end of file +frappe.patches.v15_0.migrate_role_profile_to_table_multi_select +frappe.core.doctype.user.patches.hash_reset_password_tokens diff --git a/frappe/tests/test_utils.py b/frappe/tests/test_utils.py index 6ab87f23bf..312a9f3228 100644 --- a/frappe/tests/test_utils.py +++ b/frappe/tests/test_utils.py @@ -70,6 +70,7 @@ from frappe.utils.data import ( nowtime, pretty_date, rounded, + sha256_hash, to_timedelta, validate_python_code, ) @@ -1323,3 +1324,12 @@ class TestChangeLog(FrappeTestCase): self.assertIsNone(repo) self.assertRaises(ValueError, parse_github_url, remote_url=None) + + +class TestCrypto(FrappeTestCase): + def test_hashing(self): + self.assertEqual(sha256_hash(""), "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855") + self.assertEqual( + sha256_hash(b"The quick brown fox jumps over the lazy dog"), + "d7a8fbb307d7809469ca9abcb0082e4f8d5651e46d3cdb762d02d0bf37c9e592", + ) diff --git a/frappe/utils/data.py b/frappe/utils/data.py index 2ba5ea2d5f..fc07b4ad8f 100644 --- a/frappe/utils/data.py +++ b/frappe/utils/data.py @@ -3,6 +3,7 @@ import base64 import datetime +import hashlib import json import math import operator @@ -2248,6 +2249,13 @@ def generate_hash(*args, **kwargs) -> str: return frappe.generate_hash(*args, **kwargs) +def sha256_hash(input: str | bytes) -> str: + """Return hash of the string using sha256 algorithm.""" + if isinstance(input, str): + input = input.encode() + return hashlib.sha256(input).hexdigest() + + def dict_with_keys(dict, keys): """Return a new dict with a subset of keys.""" out = {}