From fea87d09dcf08f014c17b813d35b90cc892ca05c Mon Sep 17 00:00:00 2001 From: Revant Nandgaonkar Date: Fri, 17 Nov 2023 12:48:49 +0530 Subject: [PATCH 01/11] fix: call auth hooks before raising error --- frappe/auth.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/frappe/auth.py b/frappe/auth.py index 4b53e76533..65e1fe8fac 100644 --- a/frappe/auth.py +++ b/frappe/auth.py @@ -571,6 +571,7 @@ def validate_auth(): authorization_header = frappe.get_request_header("Authorization", "").split(" ") if len(authorization_header) == 2: + validate_auth_via_hooks() validate_oauth(authorization_header) validate_auth_via_api_keys(authorization_header) @@ -579,8 +580,6 @@ def validate_auth(): if frappe.session.user in ("", "Guest"): raise frappe.AuthenticationError - validate_auth_via_hooks() - def validate_oauth(authorization_header): """ @@ -621,7 +620,7 @@ def validate_oauth(authorization_header): frappe.set_user(frappe.db.get_value("OAuth Bearer Token", token, "user")) frappe.local.form_dict = form_dict except AttributeError: - raise frappe.AuthenticationError + pass def validate_auth_via_api_keys(authorization_header): From 1ecb60f1b0474ebca5dbf33e4299ee89f586bb23 Mon Sep 17 00:00:00 2001 From: Revant Nandgaonkar Date: Fri, 17 Nov 2023 14:10:37 +0530 Subject: [PATCH 02/11] fix: call auth hooks before validate auth --- frappe/app.py | 4 +++- frappe/auth.py | 1 - 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/frappe/app.py b/frappe/app.py index c036b65e9c..6b8ba0110b 100644 --- a/frappe/app.py +++ b/frappe/app.py @@ -22,7 +22,7 @@ import frappe.rate_limiter import frappe.recorder import frappe.utils.response from frappe import _ -from frappe.auth import SAFE_HTTP_METHODS, UNSAFE_HTTP_METHODS, HTTPRequest, validate_auth +from frappe.auth import SAFE_HTTP_METHODS, UNSAFE_HTTP_METHODS, HTTPRequest, validate_auth, validate_auth_via_hooks from frappe.middlewares import StaticDataMiddleware from frappe.utils import CallbackManager, cint, get_site_name from frappe.utils.data import escape_html @@ -94,6 +94,8 @@ def application(request: Request): init_request(request) + validate_auth_via_hooks() + validate_auth() if request.method == "OPTIONS": diff --git a/frappe/auth.py b/frappe/auth.py index 65e1fe8fac..25621951e0 100644 --- a/frappe/auth.py +++ b/frappe/auth.py @@ -571,7 +571,6 @@ def validate_auth(): authorization_header = frappe.get_request_header("Authorization", "").split(" ") if len(authorization_header) == 2: - validate_auth_via_hooks() validate_oauth(authorization_header) validate_auth_via_api_keys(authorization_header) From 8ea2803fbe5704ac9c2387f5e5de1554e17048bf Mon Sep 17 00:00:00 2001 From: Revant Nandgaonkar Date: Fri, 17 Nov 2023 09:52:07 +0000 Subject: [PATCH 03/11] fix: remove raised exceptions and fail in validate_auth --- frappe/app.py | 4 +--- frappe/auth.py | 7 +++---- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/frappe/app.py b/frappe/app.py index 6b8ba0110b..c036b65e9c 100644 --- a/frappe/app.py +++ b/frappe/app.py @@ -22,7 +22,7 @@ import frappe.rate_limiter import frappe.recorder import frappe.utils.response from frappe import _ -from frappe.auth import SAFE_HTTP_METHODS, UNSAFE_HTTP_METHODS, HTTPRequest, validate_auth, validate_auth_via_hooks +from frappe.auth import SAFE_HTTP_METHODS, UNSAFE_HTTP_METHODS, HTTPRequest, validate_auth from frappe.middlewares import StaticDataMiddleware from frappe.utils import CallbackManager, cint, get_site_name from frappe.utils.data import escape_html @@ -94,8 +94,6 @@ def application(request: Request): init_request(request) - validate_auth_via_hooks() - validate_auth() if request.method == "OPTIONS": diff --git a/frappe/auth.py b/frappe/auth.py index 25621951e0..294154a167 100644 --- a/frappe/auth.py +++ b/frappe/auth.py @@ -573,6 +573,7 @@ def validate_auth(): if len(authorization_header) == 2: validate_oauth(authorization_header) validate_auth_via_api_keys(authorization_header) + validate_auth_via_hooks() # If login via bearer, basic or keypair didn't work then authentication failed and we # should terminate here. @@ -645,7 +646,7 @@ def validate_auth_via_api_keys(authorization_header): frappe.InvalidAuthorizationToken, ) except (AttributeError, TypeError, ValueError): - raise frappe.AuthenticationError + pass def validate_api_key_secret(api_key, api_secret, frappe_authorization_source=None): @@ -653,7 +654,7 @@ def validate_api_key_secret(api_key, api_secret, frappe_authorization_source=Non doctype = frappe_authorization_source or "User" doc = frappe.db.get_value(doctype=doctype, filters={"api_key": api_key}, fieldname=["name"]) if not doc: - raise frappe.AuthenticationError + return form_dict = frappe.local.form_dict doc_secret = get_decrypted_password(doctype, doc, fieldname="api_secret") if api_secret == doc_secret: @@ -664,8 +665,6 @@ def validate_api_key_secret(api_key, api_secret, frappe_authorization_source=Non if frappe.local.login_manager.user in ("", "Guest"): frappe.set_user(user) frappe.local.form_dict = form_dict - else: - raise frappe.AuthenticationError def validate_auth_via_hooks(): From 5fc4400eee7cb2012961ade9bde63ac6d07412bd Mon Sep 17 00:00:00 2001 From: Revant Nandgaonkar Date: Fri, 17 Nov 2023 15:55:55 +0530 Subject: [PATCH 04/11] fix: revert raise error internal function get_decrypted_password raises error no point in removing error from call --- frappe/auth.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/auth.py b/frappe/auth.py index 294154a167..27aff63371 100644 --- a/frappe/auth.py +++ b/frappe/auth.py @@ -646,7 +646,7 @@ def validate_auth_via_api_keys(authorization_header): frappe.InvalidAuthorizationToken, ) except (AttributeError, TypeError, ValueError): - pass + raise frappe.AuthenticationError def validate_api_key_secret(api_key, api_secret, frappe_authorization_source=None): @@ -654,7 +654,7 @@ def validate_api_key_secret(api_key, api_secret, frappe_authorization_source=Non doctype = frappe_authorization_source or "User" doc = frappe.db.get_value(doctype=doctype, filters={"api_key": api_key}, fieldname=["name"]) if not doc: - return + raise frappe.AuthenticationError form_dict = frappe.local.form_dict doc_secret = get_decrypted_password(doctype, doc, fieldname="api_secret") if api_secret == doc_secret: From 29d8f2a568aae7ee54b9e46ea3cdacf77920d3b3 Mon Sep 17 00:00:00 2001 From: Revant Nandgaonkar Date: Fri, 17 Nov 2023 15:56:17 +0530 Subject: [PATCH 05/11] test: test auth_hooks --- frappe/tests/test_hooks.py | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/frappe/tests/test_hooks.py b/frappe/tests/test_hooks.py index 41a734e7ad..09a1810dbe 100644 --- a/frappe/tests/test_hooks.py +++ b/frappe/tests/test_hooks.py @@ -1,10 +1,9 @@ # Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE - import frappe from frappe.cache_manager import clear_controller_cache from frappe.desk.doctype.todo.todo import ToDo -from frappe.tests.utils import FrappeTestCase +from frappe.tests.utils import FrappeTestCase, patch_hooks class TestHooks(FrappeTestCase): @@ -95,11 +94,28 @@ class TestHooks(FrappeTestCase): event.delete() + def test_auth_hook(self): + import requests + with patch_hooks({ "auth_hooks": ["frappe.tests.test_hooks.custom_auth"] }): + site_url = frappe.utils.get_site_url(frappe.local.site) + response = requests.get( + site_url + "/api/method/frappe.auth.get_logged_user", + headers={"Authorization": "Bearer set_test_example_user"} + ).json() + # Test! + self.assertTrue(response.get("message") == "test@example.com") + def custom_has_permission(doc, ptype, user): if doc.flags.dont_touch_me: return False +def custom_auth(): + auth_type, token = frappe.get_request_header("Authorization", "Bearer ").split(" ") + if token == "set_test_example_user": + frappe.set_user("test@example.com") + + class CustomToDo(ToDo): pass From 2dd376543ab75cd88bb4601a73edb8142cc3f940 Mon Sep 17 00:00:00 2001 From: Revant Nandgaonkar Date: Fri, 17 Nov 2023 16:27:21 +0530 Subject: [PATCH 06/11] fix: pre-commit errors --- frappe/tests/test_hooks.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/frappe/tests/test_hooks.py b/frappe/tests/test_hooks.py index 09a1810dbe..9c6c383034 100644 --- a/frappe/tests/test_hooks.py +++ b/frappe/tests/test_hooks.py @@ -96,11 +96,12 @@ class TestHooks(FrappeTestCase): def test_auth_hook(self): import requests - with patch_hooks({ "auth_hooks": ["frappe.tests.test_hooks.custom_auth"] }): + + with patch_hooks({"auth_hooks": ["frappe.tests.test_hooks.custom_auth"]}): site_url = frappe.utils.get_site_url(frappe.local.site) response = requests.get( site_url + "/api/method/frappe.auth.get_logged_user", - headers={"Authorization": "Bearer set_test_example_user"} + headers={"Authorization": "Bearer set_test_example_user"}, ).json() # Test! self.assertTrue(response.get("message") == "test@example.com") From 53b8ab9a9deae5d4dd15ca65515319869c7fea37 Mon Sep 17 00:00:00 2001 From: Revant Nandgaonkar Date: Fri, 17 Nov 2023 17:59:13 +0530 Subject: [PATCH 07/11] test: fix test auth_hooks --- frappe/tests/test_hooks.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/frappe/tests/test_hooks.py b/frappe/tests/test_hooks.py index 9c6c383034..970699d01c 100644 --- a/frappe/tests/test_hooks.py +++ b/frappe/tests/test_hooks.py @@ -3,6 +3,7 @@ import frappe from frappe.cache_manager import clear_controller_cache from frappe.desk.doctype.todo.todo import ToDo +from frappe.tests.test_api import FrappeAPITestCase from frappe.tests.utils import FrappeTestCase, patch_hooks @@ -94,17 +95,17 @@ class TestHooks(FrappeTestCase): event.delete() - def test_auth_hook(self): - import requests +class TestAPIHooks(FrappeAPITestCase): + def test_auth_hook(self): with patch_hooks({"auth_hooks": ["frappe.tests.test_hooks.custom_auth"]}): site_url = frappe.utils.get_site_url(frappe.local.site) - response = requests.get( + response = self.get( site_url + "/api/method/frappe.auth.get_logged_user", headers={"Authorization": "Bearer set_test_example_user"}, - ).json() + ) # Test! - self.assertTrue(response.get("message") == "test@example.com") + self.assertTrue(response.json.get("message") == "test@example.com") def custom_has_permission(doc, ptype, user): From b37ac30dc65bee81cf23fadd884f19219558a508 Mon Sep 17 00:00:00 2001 From: Revant Nandgaonkar Date: Fri, 17 Nov 2023 13:48:18 +0000 Subject: [PATCH 08/11] fix: raise error on validate keys --- frappe/auth.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frappe/auth.py b/frappe/auth.py index 27aff63371..648f72f8e7 100644 --- a/frappe/auth.py +++ b/frappe/auth.py @@ -665,6 +665,8 @@ def validate_api_key_secret(api_key, api_secret, frappe_authorization_source=Non if frappe.local.login_manager.user in ("", "Guest"): frappe.set_user(user) frappe.local.form_dict = form_dict + else: + raise frappe.AuthenticationError def validate_auth_via_hooks(): From 7666ea74f1efb9d10d95f773889c4f804223feff Mon Sep 17 00:00:00 2001 From: Revant Nandgaonkar Date: Fri, 17 Nov 2023 19:26:28 +0530 Subject: [PATCH 09/11] fix: validate_auth hooks for non Authorization headers --- frappe/auth.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/frappe/auth.py b/frappe/auth.py index 648f72f8e7..e005331c50 100644 --- a/frappe/auth.py +++ b/frappe/auth.py @@ -573,12 +573,13 @@ def validate_auth(): if len(authorization_header) == 2: validate_oauth(authorization_header) validate_auth_via_api_keys(authorization_header) - validate_auth_via_hooks() - # If login via bearer, basic or keypair didn't work then authentication failed and we - # should terminate here. - if frappe.session.user in ("", "Guest"): - raise frappe.AuthenticationError + validate_auth_via_hooks() + + # If login via bearer, basic or keypair didn't work then authentication failed and we + # should terminate here. + if frappe.session.user in ("", "Guest"): + raise frappe.AuthenticationError def validate_oauth(authorization_header): From 693d079f167f54329695c231146f0df7bfb31384 Mon Sep 17 00:00:00 2001 From: Revant Nandgaonkar Date: Fri, 17 Nov 2023 15:17:37 +0000 Subject: [PATCH 10/11] fix: validate only authorization headers --- frappe/auth.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/frappe/auth.py b/frappe/auth.py index e005331c50..648f72f8e7 100644 --- a/frappe/auth.py +++ b/frappe/auth.py @@ -573,13 +573,12 @@ def validate_auth(): if len(authorization_header) == 2: validate_oauth(authorization_header) validate_auth_via_api_keys(authorization_header) + validate_auth_via_hooks() - validate_auth_via_hooks() - - # If login via bearer, basic or keypair didn't work then authentication failed and we - # should terminate here. - if frappe.session.user in ("", "Guest"): - raise frappe.AuthenticationError + # If login via bearer, basic or keypair didn't work then authentication failed and we + # should terminate here. + if frappe.session.user in ("", "Guest"): + raise frappe.AuthenticationError def validate_oauth(authorization_header): From 5ba53b05fbfffed20569e43d8f9a600a3d03cac9 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sat, 18 Nov 2023 11:24:54 +0530 Subject: [PATCH 11/11] fix: Revert possibly breaking behaviour Auth hooks should always run regardless of auth headers. These are supposed to be generic hooks without any expectation on what it's supposed to do. --- frappe/auth.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/frappe/auth.py b/frappe/auth.py index 648f72f8e7..f3fbe272c5 100644 --- a/frappe/auth.py +++ b/frappe/auth.py @@ -573,12 +573,13 @@ def validate_auth(): if len(authorization_header) == 2: validate_oauth(authorization_header) validate_auth_via_api_keys(authorization_header) - validate_auth_via_hooks() - # If login via bearer, basic or keypair didn't work then authentication failed and we - # should terminate here. - if frappe.session.user in ("", "Guest"): - raise frappe.AuthenticationError + validate_auth_via_hooks() + + # If login via bearer, basic or keypair didn't work then authentication failed and we + # should terminate here. + if len(authorization_header) == 2 and frappe.session.user in ("", "Guest"): + raise frappe.AuthenticationError def validate_oauth(authorization_header):