From 7ccbbce5720bf16d5d3cc94c627e22ef0541e53b Mon Sep 17 00:00:00 2001 From: schilgod Date: Mon, 8 May 2017 18:40:12 +0800 Subject: [PATCH] Password Strength Policy Enhancement (#3194) * Add or_filters filter for export_fixtures, to filter by doctype or fieldnames Eg: fixtures = [ { "doctype": "Custom Field", "or_filters": { "dt": ["in", [ "Process Payroll", "Journal Entry Account" ]], "name": ["in", [ "Print Settings-compact_item_print", "Account-account_id", "Account-some_bank_name" ]] } } ] * Passsword Policy Enhancement In Security Settings, the Password Plociy can be enabled and the passcord strength score can be set. User will see helpful password validation messages when they change password. * Enhance Update Password to handle Password Policy * remove function argument * update test cases to use strong password * Add test cases, add default value for minimum password score, make error messages translatable * make message translatable * Update update-password.html --- .../system_settings/system_settings.js | 7 ++ .../system_settings/system_settings.json | 67 ++++++++++++++++++- .../system_settings/system_settings.py | 7 ++ frappe/core/doctype/user/test_records.json | 10 +-- frappe/core/doctype/user/test_user.py | 34 ++++++++-- frappe/core/doctype/user/user.js | 5 +- frappe/core/doctype/user/user.py | 39 ++++++++++- frappe/tests/test_password.py | 4 +- frappe/www/update-password.html | 33 +++++---- 9 files changed, 171 insertions(+), 35 deletions(-) diff --git a/frappe/core/doctype/system_settings/system_settings.js b/frappe/core/doctype/system_settings/system_settings.js index aa3f4a766b..f8ac1734f9 100644 --- a/frappe/core/doctype/system_settings/system_settings.js +++ b/frappe/core/doctype/system_settings/system_settings.js @@ -13,3 +13,10 @@ frappe.ui.form.on("System Settings", "refresh", function(frm) { }); }); +frappe.ui.form.on("System Settings", "enable_password_policy", function(frm) { + if(frm.doc.enable_password_policy == 0){ + frm.set_value("minimum_password_score", ""); + }else{ + frm.set_value("minimum_password_score", "2"); + } +}); \ No newline at end of file diff --git a/frappe/core/doctype/system_settings/system_settings.json b/frappe/core/doctype/system_settings/system_settings.json index 6f68cf3842..d742681469 100644 --- a/frappe/core/doctype/system_settings/system_settings.json +++ b/frappe/core/doctype/system_settings/system_settings.json @@ -614,6 +614,71 @@ "set_only_once": 0, "unique": 0 }, + { + "allow_bulk_edit": 0, + "allow_on_submit": 0, + "bold": 0, + "collapsible": 0, + "columns": 0, + "default": "0", + "description": "If enabled, the password strength will be enforced based on the Minimum Password Score value. A value of 2 being medium strong and 4 being very strong.", + "fieldname": "enable_password_policy", + "fieldtype": "Check", + "hidden": 0, + "ignore_user_permissions": 0, + "ignore_xss_filter": 0, + "in_filter": 0, + "in_global_search": 0, + "in_list_view": 0, + "in_standard_filter": 0, + "label": "Enable Password Policy", + "length": 0, + "no_copy": 0, + "permlevel": 0, + "precision": "", + "print_hide": 0, + "print_hide_if_no_value": 0, + "read_only": 0, + "remember_last_selected_value": 0, + "report_hide": 0, + "reqd": 0, + "search_index": 0, + "set_only_once": 0, + "unique": 0 + }, + { + "allow_bulk_edit": 0, + "allow_on_submit": 0, + "bold": 0, + "collapsible": 0, + "columns": 0, + "default": "2", + "depends_on": "eval:doc.enable_password_policy==1", + "fieldname": "minimum_password_score", + "fieldtype": "Select", + "hidden": 0, + "ignore_user_permissions": 0, + "ignore_xss_filter": 0, + "in_filter": 0, + "in_global_search": 0, + "in_list_view": 0, + "in_standard_filter": 0, + "label": "Minimum Password Score", + "length": 0, + "no_copy": 0, + "options": "2\n4", + "permlevel": 0, + "precision": "", + "print_hide": 0, + "print_hide_if_no_value": 0, + "read_only": 0, + "remember_last_selected_value": 0, + "report_hide": 0, + "reqd": 0, + "search_index": 0, + "set_only_once": 0, + "unique": 0 + }, { "allow_bulk_edit": 0, "allow_on_submit": 0, @@ -837,7 +902,7 @@ "issingle": 1, "istable": 0, "max_attachments": 0, - "modified": "2017-04-25 12:00:12.846542", + "modified": "2017-05-01 15:27:11.079447", "modified_by": "Administrator", "module": "Core", "name": "System Settings", diff --git a/frappe/core/doctype/system_settings/system_settings.py b/frappe/core/doctype/system_settings/system_settings.py index 2805151a25..8f64feda9f 100644 --- a/frappe/core/doctype/system_settings/system_settings.py +++ b/frappe/core/doctype/system_settings/system_settings.py @@ -12,6 +12,13 @@ from frappe.utils.momentjs import get_all_timezones class SystemSettings(Document): def validate(self): + enable_password_policy = cint(self.enable_password_policy) and True or False + minimum_password_score = cint(self.minimum_password_score) or 0 + if enable_password_policy and minimum_password_score <= 0: + frappe.throw(_("Please select Minimum Password Score")) + elif not enable_password_policy: + self.minimum_password_score = "" + for key in ("session_expiry", "session_expiry_mobile"): if self.get(key): parts = self.get(key).split(":") diff --git a/frappe/core/doctype/user/test_records.json b/frappe/core/doctype/user/test_records.json index a2dbb0b689..3f5dd87e55 100644 --- a/frappe/core/doctype/user/test_records.json +++ b/frappe/core/doctype/user/test_records.json @@ -4,7 +4,7 @@ "email": "test@example.com", "enabled": 1, "first_name": "_Test", - "new_password": "testpassword", + "new_password": "Eastern_43A1W", "roles": [ { "doctype": "Has Role", @@ -22,20 +22,20 @@ "doctype": "User", "email": "test1@example.com", "first_name": "_Test1", - "new_password": "testpassword" + "new_password": "Eastern_43A1W" }, { "doctype": "User", "email": "test2@example.com", "first_name": "_Test2", - "new_password": "testpassword", + "new_password": "Eastern_43A1W", "enabled": 1 }, { "doctype": "User", "email": "testperm@example.com", "first_name": "_Test Perm", - "new_password": "testpassword", + "new_password": "Eastern_43A1W", "enabled": 1 }, { @@ -43,7 +43,7 @@ "email": "testdelete@example.com", "enabled": 1, "first_name": "_Test", - "new_password": "testpassword", + "new_password": "Eastern_43A1W", "roles": [ { "doctype": "Has Role", diff --git a/frappe/core/doctype/user/test_user.py b/frappe/core/doctype/user/test_user.py index 31a1b58320..0b7bcb7ff8 100644 --- a/frappe/core/doctype/user/test_user.py +++ b/frappe/core/doctype/user/test_user.py @@ -11,7 +11,7 @@ from frappe import _dict from frappe.limits import update_limits, clear_limit from frappe.utils import get_url from frappe.core.doctype.user.user import get_total_users -from frappe.core.doctype.user.user import MaxUsersReachedError +from frappe.core.doctype.user.user import MaxUsersReachedError, test_password_strength test_records = frappe.get_test_records('User') @@ -147,7 +147,7 @@ class TestUser(unittest.TestCase): # # allow one session # user = frappe.get_doc('User', 'test@example.com') # user.simultaneous_sessions = 1 - # user.new_password = 'testpassword' + # user.new_password = 'Eastern_43A1W' # user.save() # # def test_request(conn): @@ -158,18 +158,18 @@ class TestUser(unittest.TestCase): # update_site_config('deny_multiple_sessions', 0) # # print 'conn1' - # conn1 = FrappeClient(get_url(), "test@example.com", "testpassword", verify=False) + # conn1 = FrappeClient(get_url(), "test@example.com", "Eastern_43A1W", verify=False) # test_request(conn1) # # print 'conn2' - # conn2 = FrappeClient(get_url(), "test@example.com", "testpassword", verify=False) + # conn2 = FrappeClient(get_url(), "test@example.com", "Eastern_43A1W", verify=False) # test_request(conn2) # # update_site_config('deny_multiple_sessions', 1) # # print 'conn3' # - # conn3 = FrappeClient(get_url(), "test@example.com", "testpassword", verify=False) + # conn3 = FrappeClient(get_url(), "test@example.com", "Eastern_43A1W", verify=False) # test_request(conn3) # # # first connection should fail @@ -178,7 +178,7 @@ class TestUser(unittest.TestCase): def test_site_expiry(self): user = frappe.get_doc('User', 'test@example.com') user.enabled = 1 - user.new_password = 'testpassword' + user.new_password = 'Eastern_43A1W' user.save() update_limits({'expiry': add_to_date(today(), days=-1), 'support_email': 'support@example.com'}) @@ -187,7 +187,7 @@ class TestUser(unittest.TestCase): frappe.db.commit() res = requests.post(get_url(), params={'cmd': 'login', 'usr': - 'test@example.com', 'pwd': 'testpassword', 'device': 'desktop'}) + 'test@example.com', 'pwd': 'Eastern_43A1W', 'device': 'desktop'}) # While site is expired status code returned is 417 Failed Expectation self.assertEqual(res.status_code, 417) @@ -214,3 +214,23 @@ class TestUser(unittest.TestCase): # Clear the user limit clear_limit('users') + def test_password_strength(self): + #Test Password without Password Strenth Policy + frappe.db.set_value("System Settings", "System Settings", "enable_password_policy", 0) + frappe.db.set_value("System Settings", "System Settings", "minimum_password_score", "") + + # Should pass password strength test + result = test_password_strength("test_password") + self.assertEqual(result['feedback']['password_policy_validation_passed'], True) + + # Test Password with Password Strenth Policy Set + frappe.db.set_value("System Settings", "System Settings", "enable_password_policy", 1) + frappe.db.set_value("System Settings", "System Settings", "minimum_password_score", 2) + + #Should fail password strength test + result = test_password_strength("test_password") + self.assertEqual(result['feedback']['password_policy_validation_passed'], False) + + # Should pass password strength test + result = test_password_strength("Eastern_43A1W") + self.assertEqual(result['feedback']['password_policy_validation_passed'], True) \ No newline at end of file diff --git a/frappe/core/doctype/user/user.js b/frappe/core/doctype/user/user.js index cbd839d425..b540601da1 100644 --- a/frappe/core/doctype/user/user.js +++ b/frappe/core/doctype/user/user.js @@ -64,7 +64,7 @@ frappe.ui.form.on('User', { frm.toggle_display(['sb1', 'sb3', 'modules_access'], true); } - + frm.add_custom_button(__("Reset Password"), function() { frappe.call({ method: "frappe.core.doctype.user.user.reset_password", @@ -125,7 +125,6 @@ frappe.ui.form.on('User', { frm.toggle_enable('email', doc.__islocal); } }, - create_user_email:function(frm) { frappe.call({ method: 'frappe.core.doctype.user.user.has_email_account', @@ -146,7 +145,7 @@ frappe.ui.form.on('User', { frappe.set_route("Form", "Email Account", doc.name); }) } else { - frappe.route_flags.create_user_account = frm.doc.name; + frappe.route_flags.create_user_account = frm.doc.name; frappe.set_route("Form", "Email Account", r.message[0]["name"]); } } diff --git a/frappe/core/doctype/user/user.py b/frappe/core/doctype/user/user.py index 1f58d3ab50..ed84cdf8c6 100644 --- a/frappe/core/doctype/user/user.py +++ b/frappe/core/doctype/user/user.py @@ -49,6 +49,7 @@ class User(Document): self.__new_password = self.new_password self.new_password = "" + self.password_strength_test() if self.name not in STANDARD_USERS: self.validate_email_type(self.email) self.validate_email_type(self.name) @@ -407,6 +408,14 @@ class User(Document): frappe.msgprint(_("Username should not contain any special characters other than letters, numbers and underscore")) self.username = "" + def password_strength_test(self): + if self.__new_password: + user_data = (self.first_name, self.middle_name, self.last_name, self.email, self.birth_date) + result = test_password_strength(self.__new_password, '', None, user_data) + + if not result['feedback']['password_policy_validation_passed']: + handle_password_test_fail(result) + def suggest_username(self): def _check_suggestion(suggestion): if self.username != suggestion and not self.username_exists(suggestion): @@ -494,6 +503,11 @@ def get_perm_info(role): @frappe.whitelist(allow_guest=True) def update_password(new_password, key=None, old_password=None): + result = test_password_strength(new_password, key, old_password) + + if not result['feedback']['password_policy_validation_passed']: + handle_password_test_fail(result) + res = _get_user_for_update_password(key, old_password) if res.get('message'): return res['message'] @@ -519,13 +533,25 @@ def update_password(new_password, key=None, old_password=None): return redirect_url if redirect_url else "/" @frappe.whitelist(allow_guest=True) -def test_password_strength(new_password, key=None, old_password=None): +def test_password_strength(new_password, key=None, old_password=None, user_data=[]): from frappe.utils.password_strength import test_password_strength as _test_password_strength - user_data = frappe.db.get_value('User', frappe.session.user, ['first_name', 'middle_name', 'last_name', 'email', 'birth_date']) + if not user_data: + user_data = frappe.db.get_value('User', frappe.session.user, ['first_name', 'middle_name', 'last_name', 'email', 'birth_date']) if new_password: - return _test_password_strength(new_password, user_inputs=user_data) + result = _test_password_strength(new_password, user_inputs=user_data) + + enable_password_policy = cint(frappe.db.get_single_value("System Settings", "enable_password_policy")) and True or False + minimum_password_score = cint(frappe.db.get_single_value("System Settings", "minimum_password_score")) or 0 + + password_policy_validation_passed = True + if enable_password_policy and result['score'] < minimum_password_score: + password_policy_validation_passed = False + + result['feedback']['password_policy_validation_passed'] = password_policy_validation_passed + + return result #for login @frappe.whitelist() @@ -837,3 +863,10 @@ def extract_mentions(txt): """Find all instances of @username in the string. The mentions will be separated by non-word characters or may appear at the start of the string""" return re.findall(r'(?:[^\w]|^)@([\w]*)', txt) + + +def handle_password_test_fail(result): + suggestions = result['feedback']['suggestions'][0] if result['feedback']['suggestions'] else '' + warning = result['feedback']['warning'] if 'warning' in result['feedback'] else '' + suggestions += _("{0} Hint : Include Underscores, Numbers and Capital Letters in the password {0} Example : Eastern_43A1W").format("
") + frappe.throw(_('Invalid Password: ' + ' '.join([warning, suggestions]))) \ No newline at end of file diff --git a/frappe/tests/test_password.py b/frappe/tests/test_password.py index acaca6a290..f35f778ab9 100644 --- a/frappe/tests/test_password.py +++ b/frappe/tests/test_password.py @@ -47,8 +47,8 @@ class TestPassword(unittest.TestCase): return frappe.get_doc('Email Account', name) def test_hashed_password(self, user='test@example.com'): - old_password = 'testpassword' - new_password = 'testpassword-new' + old_password = 'Eastern_43A1W' + new_password = 'Eastern_43A1W-new' update_password(user, new_password) diff --git a/frappe/www/update-password.html b/frappe/www/update-password.html index cffcdde37a..632c4e0c9f 100644 --- a/frappe/www/update-password.html +++ b/frappe/www/update-password.html @@ -132,12 +132,10 @@ frappe.ready(function() { feedback.crack_time_display = r.message.crack_time_display; feedback.score = score; - if (score < 2) { - set_strength_indicator('red', feedback); - } else if (score < 4) { - set_strength_indicator('yellow', feedback); - } else { + if(feedback.password_policy_validation_passed){ set_strength_indicator('green', feedback); + }else{ + set_strength_indicator('red', feedback); } } } @@ -148,18 +146,25 @@ frappe.ready(function() { window.set_strength_indicator = function(color, feedback) { var message = []; + feedback.help_msg = ""; + if(!feedback.password_policy_validation_passed){ + feedback.help_msg = __("Hint: Include symbols, numbers and capital letters in the password"); + } if (feedback) { - if (feedback.suggestions && feedback.suggestions.length) { - message = message.concat(feedback.suggestions); - } else if (feedback.warning) { - message.push(feedback.warning); - } + if(!feedback.password_policy_validation_passed){ + if (feedback.suggestions && feedback.suggestions.length) { + feedback.suggestions = feedback.suggestions + ' ' + feedback.help_msg; + message = message.concat(feedback.suggestions); + } else if (feedback.warning) { + feedback.warning = feedback.warning + ' ' + feedback.help_msg; + message.push(feedback.warning); + } - if (!message.length && feedback.crack_time_display) { - message.push(__('This password will take {0} to crack', [feedback.crack_time_display])); - if (feedback.score > 3) { - message.push('👍'); + if (!message.length) { + message.push(feedback.help_msg); } + }else{ + message.push(__('Success! You are good to go 👍')); } }