From d9029750804099e907339b9bfbf311d6144db065 Mon Sep 17 00:00:00 2001 From: Sunny Date: Thu, 31 Aug 2017 17:00:06 +0800 Subject: [PATCH 1/5] Bypass 2FA if user login from restricted IP Address --- .../system_settings/system_settings.js | 8 ++- .../system_settings/system_settings.json | 62 +++++++++---------- .../system_settings/system_settings.py | 2 + frappe/tests/test_twofactor.py | 46 +++++++++++++- frappe/twofactor.py | 18 ++++-- 5 files changed, 96 insertions(+), 40 deletions(-) diff --git a/frappe/core/doctype/system_settings/system_settings.js b/frappe/core/doctype/system_settings/system_settings.js index 11c01305e3..057f6d7060 100644 --- a/frappe/core/doctype/system_settings/system_settings.js +++ b/frappe/core/doctype/system_settings/system_settings.js @@ -19,4 +19,10 @@ frappe.ui.form.on("System Settings", "enable_password_policy", function(frm) { } else { frm.set_value("minimum_password_score", "2"); } -}); \ No newline at end of file +}); + +frappe.ui.form.on("System Settings", "enable_two_factor_auth", function(frm) { + if(frm.doc.enable_password_policy == 0){ + frm.set_value("bypass_2fa_for_retricted_ip_users", ""); + } +}); diff --git a/frappe/core/doctype/system_settings/system_settings.json b/frappe/core/doctype/system_settings/system_settings.json index 42f873509a..7ca2896102 100644 --- a/frappe/core/doctype/system_settings/system_settings.json +++ b/frappe/core/doctype/system_settings/system_settings.json @@ -160,37 +160,37 @@ "unique": 0 }, { - "allow_bulk_edit": 0, - "allow_on_submit": 0, - "bold": 0, - "collapsible": 0, - "columns": 0, - "fieldname": "is_first_startup", - "fieldtype": "Check", - "hidden": 1, - "ignore_user_permissions": 0, - "ignore_xss_filter": 0, - "in_filter": 0, - "in_global_search": 0, - "in_list_view": 0, - "in_standard_filter": 0, - "label": "Is First Startup", - "length": 0, - "no_copy": 0, - "permlevel": 0, - "precision": "", - "print_hide": 0, - "print_hide_if_no_value": 0, - "read_only": 1, - "remember_last_selected_value": 0, - "report_hide": 0, - "reqd": 0, - "search_index": 0, - "set_only_once": 0, + "allow_bulk_edit": 0, + "allow_on_submit": 0, + "bold": 0, + "collapsible": 0, + "columns": 0, + "fieldname": "is_first_startup", + "fieldtype": "Check", + "hidden": 1, + "ignore_user_permissions": 0, + "ignore_xss_filter": 0, + "in_filter": 0, + "in_global_search": 0, + "in_list_view": 0, + "in_standard_filter": 0, + "label": "Is First Startup", + "length": 0, + "no_copy": 0, + "permlevel": 0, + "precision": "", + "print_hide": 0, + "print_hide_if_no_value": 0, + "read_only": 1, + "remember_last_selected_value": 0, + "report_hide": 0, + "reqd": 0, + "search_index": 0, + "set_only_once": 0, "unique": 0 - }, + }, { - "allow_bulk_edit": 0, + "allow_bulk_edit": 0, "allow_on_submit": 0, "bold": 0, "collapsible": 0, @@ -1216,8 +1216,8 @@ "issingle": 1, "istable": 0, "max_attachments": 0, - "modified": "2017-08-31 14:53:31.065925", - "modified_by": "ewfds@wfe.ef", + "modified": "2017-08-31 14:53:31.065925", + "modified_by": "ewfds@wfe.ef", "module": "Core", "name": "System Settings", "name_case": "", diff --git a/frappe/core/doctype/system_settings/system_settings.py b/frappe/core/doctype/system_settings/system_settings.py index cd7edc6a53..38ed4babea 100644 --- a/frappe/core/doctype/system_settings/system_settings.py +++ b/frappe/core/doctype/system_settings/system_settings.py @@ -31,6 +31,8 @@ class SystemSettings(Document): if not frappe.db.get_value('SMS Settings', None, 'sms_gateway_url'): frappe.throw(_('Please setup SMS before setting it as an authentication method, via SMS Settings')) toggle_two_factor_auth(True, roles=['All']) + else: + self.bypass_2fa_for_retricted_ip_users = 0 def on_update(self): for df in self.meta.get("fields"): diff --git a/frappe/tests/test_twofactor.py b/frappe/tests/test_twofactor.py index e993b2d517..6bf36683be 100644 --- a/frappe/tests/test_twofactor.py +++ b/frappe/tests/test_twofactor.py @@ -6,9 +6,10 @@ import unittest, frappe, pyotp from werkzeug.wrappers import Request from werkzeug.test import EnvironBuilder from frappe.auth import HTTPRequest +from frappe.utils import cint from frappe.twofactor import (should_run_2fa, authenticate_for_2factor, get_cached_user_pass, two_factor_is_enabled_for_, confirm_otp_token, get_otpsecret_for_, get_verification_obj, - render_string_template) + render_string_template, two_factor_is_enabled) import time @@ -47,6 +48,43 @@ class TestTwoFactor(unittest.TestCase): self.assertTrue(frappe.cache().get('{0}{1}'.format(tmp_id,k)), '{} not available'.format(k)) + def test_two_factor_is_enabled(self): + ''' + 1. Should return true, if enabled and not bypass_2fa_for_retricted_ip_users + 2. Should return false, if not enabled + 3. Should return true, if enabled and bypass_2fa_for_retricted_ip_users and not user.restricted_ip + 4. Should return false, if enabled and bypass_2fa_for_retricted_ip_users and user.restricted_ip + ''' + + #Scenario 1 + disable_2fa() + self.assertFalse(two_factor_is_enabled(self.user)) + + #Scenario 2 + enable_2fa() + self.assertTrue(two_factor_is_enabled(self.user)) + + #Scenario 3 + enable_2fa() + user = frappe.get_doc('User', self.user) + user.restrict_ip = frappe.local.request_ip + user.save() + self.assertTrue(two_factor_is_enabled(self.user)) + + #Scenario 4 + user = frappe.get_doc('User', self.user) + user.restrict_ip = "" + user.save() + enable_2fa(1) + self.assertTrue(two_factor_is_enabled(self.user)) + + #Scenario 5 + user = frappe.get_doc('User', self.user) + user.restrict_ip = frappe.local.request_ip + user.save() + enable_2fa(1) + self.assertFalse(two_factor_is_enabled(self.user)) + def test_two_factor_is_enabled_for_user(self): '''Should return true if enabled for user.''' toggle_2fa_all_role(state=True) @@ -102,10 +140,11 @@ def create_http_request(): http_requests = HTTPRequest() return http_requests -def enable_2fa(): +def enable_2fa(bypass_two_factor_auth=0): '''Enable Two factor in system settings.''' system_settings = frappe.get_doc('System Settings') system_settings.enable_two_factor_auth = 1 + system_settings.bypass_2fa_for_retricted_ip_users = cint(bypass_two_factor_auth) system_settings.two_factor_method = 'OTP App' system_settings.save(ignore_permissions=True) frappe.db.commit() @@ -113,6 +152,7 @@ def enable_2fa(): def disable_2fa(): system_settings = frappe.get_doc('System Settings') system_settings.enable_two_factor_auth = 0 + system_settings.bypass_2fa_for_retricted_ip_users = 0 system_settings.save(ignore_permissions=True) frappe.db.commit() @@ -129,4 +169,4 @@ def toggle_2fa_all_role(state=None): def get_otp(user): otp_secret = get_otpsecret_for_(user) otp = pyotp.TOTP(otp_secret) - return otp.now() \ No newline at end of file + return otp.now() diff --git a/frappe/twofactor.py b/frappe/twofactor.py index 887494932e..272eba27c4 100644 --- a/frappe/twofactor.py +++ b/frappe/twofactor.py @@ -12,7 +12,7 @@ from pyqrcode import create as qrcreate from six import StringIO from base64 import b64encode, b32encode from frappe.utils import get_url, get_datetime, time_diff_in_seconds -from six import iteritems, string_types +from six import string_types class ExpiredLoginException(Exception): pass @@ -26,6 +26,12 @@ def toggle_two_factor_auth(state, roles=[]): def two_factor_is_enabled(user=None): '''Returns True if 2FA is enabled.''' enabled = int(frappe.db.get_value('System Settings', None, 'enable_two_factor_auth') or 0) + if enabled: + bypass_two_factor_auth = int(frappe.db.get_value('System Settings', None, 'bypass_2fa_for_retricted_ip_users') or 0) + if bypass_two_factor_auth: + restrict_ip = frappe.db.get_value("User", filters={"name": user}, fieldname="restrict_ip") + if restrict_ip and bypass_two_factor_auth: + enabled = False if not user or not enabled: return enabled return two_factor_is_enabled_for_(user) @@ -68,7 +74,7 @@ def cache_2fa_data(user, token, otp_secret, tmp_id): frappe.cache().expire(tmp_id + '_token', expiry_time) else: expiry_time = 180 - for k, v in iteritems({'_usr': user, '_pwd': pwd, '_otp_secret': otp_secret}): + for k, v in {'_usr': user, '_pwd': pwd, '_otp_secret': otp_secret}.iteritems(): frappe.cache().set("{0}{1}".format(tmp_id, k), v) frappe.cache().expire("{0}{1}".format(tmp_id, k), expiry_time) @@ -81,8 +87,8 @@ def two_factor_is_enabled_for_(user): roles.append('All') query = """select name from `tabRole` where two_factor_auth=1 - and name in ({0}) limit 1""".format(', '.join('\"{}\"'.format(i) for i in roles)) - + and name in ({0}) limit 1""".format(', '.join('\"{}\"'.format(i) for \ + i in roles)) if len(frappe.db.sql(query)) > 0: return True @@ -149,6 +155,7 @@ def get_verification_obj(user, token, otp_secret): verification_obj = process_2fa_for_email(user, token, otp_secret, otp_issuer) return verification_obj + def process_2fa_for_sms(user, token, otp_secret): '''Process sms method for 2fa.''' phone = frappe.db.get_value('User', user, ['phone', 'mobile_no'], as_dict=1) @@ -378,4 +385,5 @@ def should_remove_barcode_image(barcode): return False def disable(): - frappe.db.set_value('System Settings', None, 'enable_two_factor_auth', 0) \ No newline at end of file + frappe.db.set_value('System Settings', None, 'enable_two_factor_auth', 0) + From cfd35d414b10c36a8e8d4a26af53a93de43788c9 Mon Sep 17 00:00:00 2001 From: Sunny Date: Fri, 1 Sep 2017 05:23:07 +0800 Subject: [PATCH 2/5] correct field name and field value --- frappe/core/doctype/system_settings/system_settings.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/core/doctype/system_settings/system_settings.js b/frappe/core/doctype/system_settings/system_settings.js index 057f6d7060..bb16473ce4 100644 --- a/frappe/core/doctype/system_settings/system_settings.js +++ b/frappe/core/doctype/system_settings/system_settings.js @@ -22,7 +22,7 @@ frappe.ui.form.on("System Settings", "enable_password_policy", function(frm) { }); frappe.ui.form.on("System Settings", "enable_two_factor_auth", function(frm) { - if(frm.doc.enable_password_policy == 0){ - frm.set_value("bypass_2fa_for_retricted_ip_users", ""); + if(frm.doc.enable_two_factor_auth == 0){ + frm.set_value("bypass_2fa_for_retricted_ip_users", 0); } }); From a99216482df3969696292d504f49f169eb84a520 Mon Sep 17 00:00:00 2001 From: Sunny Date: Thu, 7 Sep 2017 15:56:05 +0800 Subject: [PATCH 3/5] merge with upstream/frappe --- .../system_settings/system_settings.json | 36 +++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/frappe/core/doctype/system_settings/system_settings.json b/frappe/core/doctype/system_settings/system_settings.json index 7ca2896102..d3c0379823 100644 --- a/frappe/core/doctype/system_settings/system_settings.json +++ b/frappe/core/doctype/system_settings/system_settings.json @@ -986,8 +986,40 @@ "unique": 0 }, { - "allow_bulk_edit": 0, - "allow_on_submit": 0, + "allow_bulk_edit": 0, + "allow_on_submit": 0, + "bold": 0, + "collapsible": 0, + "columns": 0, + "default": "0", + "depends_on": "enable_two_factor_auth", + "fieldname": "bypass_2fa_for_retricted_ip_users", + "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": "Bypass Two Factor Auth for users who login from restricted IP Address", + "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, From fe8bae82e8d586a99d1975d4e649b5113138e8ac Mon Sep 17 00:00:00 2001 From: Sunny Date: Thu, 7 Sep 2017 15:58:56 +0800 Subject: [PATCH 4/5] merge with upstream/frappe --- frappe/twofactor.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/twofactor.py b/frappe/twofactor.py index 272eba27c4..6e62e6a070 100644 --- a/frappe/twofactor.py +++ b/frappe/twofactor.py @@ -12,7 +12,7 @@ from pyqrcode import create as qrcreate from six import StringIO from base64 import b64encode, b32encode from frappe.utils import get_url, get_datetime, time_diff_in_seconds -from six import string_types +from six import iteritems, string_types class ExpiredLoginException(Exception): pass From df343aec64c5ae2f37d42def81bc69546f7754d9 Mon Sep 17 00:00:00 2001 From: Sunny Date: Thu, 7 Sep 2017 16:04:55 +0800 Subject: [PATCH 5/5] merge with upstream/frappe --- frappe/core/doctype/system_settings/system_settings.js | 2 +- frappe/core/doctype/system_settings/system_settings.py | 2 +- frappe/tests/test_twofactor.py | 2 +- frappe/twofactor.py | 9 +++------ 4 files changed, 6 insertions(+), 9 deletions(-) diff --git a/frappe/core/doctype/system_settings/system_settings.js b/frappe/core/doctype/system_settings/system_settings.js index bb16473ce4..62c044121b 100644 --- a/frappe/core/doctype/system_settings/system_settings.js +++ b/frappe/core/doctype/system_settings/system_settings.js @@ -25,4 +25,4 @@ frappe.ui.form.on("System Settings", "enable_two_factor_auth", function(frm) { if(frm.doc.enable_two_factor_auth == 0){ frm.set_value("bypass_2fa_for_retricted_ip_users", 0); } -}); +}); \ No newline at end of file diff --git a/frappe/core/doctype/system_settings/system_settings.py b/frappe/core/doctype/system_settings/system_settings.py index 38ed4babea..ef2863fd46 100644 --- a/frappe/core/doctype/system_settings/system_settings.py +++ b/frappe/core/doctype/system_settings/system_settings.py @@ -61,4 +61,4 @@ def load(): return { "timezones": get_all_timezones(), "defaults": defaults - } + } \ No newline at end of file diff --git a/frappe/tests/test_twofactor.py b/frappe/tests/test_twofactor.py index 6bf36683be..e6e4552125 100644 --- a/frappe/tests/test_twofactor.py +++ b/frappe/tests/test_twofactor.py @@ -169,4 +169,4 @@ def toggle_2fa_all_role(state=None): def get_otp(user): otp_secret = get_otpsecret_for_(user) otp = pyotp.TOTP(otp_secret) - return otp.now() + return otp.now() \ No newline at end of file diff --git a/frappe/twofactor.py b/frappe/twofactor.py index 6e62e6a070..796f4310d8 100644 --- a/frappe/twofactor.py +++ b/frappe/twofactor.py @@ -74,7 +74,7 @@ def cache_2fa_data(user, token, otp_secret, tmp_id): frappe.cache().expire(tmp_id + '_token', expiry_time) else: expiry_time = 180 - for k, v in {'_usr': user, '_pwd': pwd, '_otp_secret': otp_secret}.iteritems(): + for k, v in iteritems({'_usr': user, '_pwd': pwd, '_otp_secret': otp_secret}): frappe.cache().set("{0}{1}".format(tmp_id, k), v) frappe.cache().expire("{0}{1}".format(tmp_id, k), expiry_time) @@ -87,8 +87,7 @@ def two_factor_is_enabled_for_(user): roles.append('All') query = """select name from `tabRole` where two_factor_auth=1 - and name in ({0}) limit 1""".format(', '.join('\"{}\"'.format(i) for \ - i in roles)) + and name in ({0}) limit 1""".format(', '.join('\"{}\"'.format(i) for i in roles)) if len(frappe.db.sql(query)) > 0: return True @@ -155,7 +154,6 @@ def get_verification_obj(user, token, otp_secret): verification_obj = process_2fa_for_email(user, token, otp_secret, otp_issuer) return verification_obj - def process_2fa_for_sms(user, token, otp_secret): '''Process sms method for 2fa.''' phone = frappe.db.get_value('User', user, ['phone', 'mobile_no'], as_dict=1) @@ -385,5 +383,4 @@ def should_remove_barcode_image(barcode): return False def disable(): - frappe.db.set_value('System Settings', None, 'enable_two_factor_auth', 0) - + frappe.db.set_value('System Settings', None, 'enable_two_factor_auth', 0) \ No newline at end of file