From e32ecb394d5c2e90ed3de9d6e2fc99c0d92323cc Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 26 Apr 2022 11:29:06 +0530 Subject: [PATCH 01/12] refactor: Oauth20 tests Use App client app directly instead of requests. This removes dependency on needing a web server running for your tests. Also, contributes to coverage now. We can see which lines are impacted with each use case. --- frappe/tests/test_oauth20.py | 330 ++++++++++++++++++----------------- 1 file changed, 167 insertions(+), 163 deletions(-) diff --git a/frappe/tests/test_oauth20.py b/frappe/tests/test_oauth20.py index a634ace62a..8ebff2bca6 100644 --- a/frappe/tests/test_oauth20.py +++ b/frappe/tests/test_oauth20.py @@ -1,88 +1,111 @@ -# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors +# Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE import unittest +from typing import TYPE_CHECKING, Dict, Optional from urllib.parse import parse_qs, urljoin, urlparse import jwt import requests +from werkzeug.test import TestResponse import frappe from frappe.integrations.oauth2 import encode_params from frappe.test_runner import make_test_records +from frappe.tests.test_api import get_test_client, make_request, suppress_stdout + +if TYPE_CHECKING: + from frappe.integrations.doctype.social_login_key.social_login_key import SocialLoginKey -class TestOAuth20(unittest.TestCase): - def setUp(self): - make_test_records("OAuth Client") +class FrappeRequestTestCase(unittest.TestCase): + TEST_CLIENT = get_test_client() + + @property + def sid(self) -> str: + if not getattr(self, "_sid", None): + from frappe.auth import CookieManager, LoginManager + from frappe.utils import set_request + + set_request(path="/") + frappe.local.cookie_manager = CookieManager() + frappe.local.login_manager = LoginManager() + frappe.local.login_manager.login_as("test@example.com") + self._sid = frappe.session.sid + + return self._sid + + def get(self, path: str, params: Optional[Dict] = None, **kwargs) -> TestResponse: + return make_request(target=self.TEST_CLIENT.get, args=(path,), kwargs={"data": params, **kwargs}) + + def post(self, path, data, **kwargs) -> TestResponse: + return make_request(target=self.TEST_CLIENT.post, args=(path,), kwargs={"data": data, **kwargs}) + + def put(self, path, data, **kwargs) -> TestResponse: + return make_request(target=self.TEST_CLIENT.put, args=(path,), kwargs={"data": data, **kwargs}) + + def delete(self, path, **kwargs) -> TestResponse: + return make_request(target=self.TEST_CLIENT.delete, args=(path,), kwargs=kwargs) + + +class TestOAuth20(FrappeRequestTestCase): + @classmethod + def setUpClass(cls): + make_test_records("OAuth Client", force=True) make_test_records("User") + client = frappe.get_all("OAuth Client", fields=["*"])[0] - self.client_id = client.get("client_id") - self.client_secret = client.get("client_secret") - self.form_header = {"content-type": "application/x-www-form-urlencoded"} - self.scope = "all openid" - self.redirect_uri = "http://localhost" + cls.client_id = client.get("client_id") + cls.client_secret = client.get("client_secret") + cls.form_header = {"content-type": "application/x-www-form-urlencoded"} + cls.scope = "all openid" + cls.redirect_uri = "http://localhost" # Set Frappe server URL reqired for id_token generation - try: - frappe_login_key = frappe.get_doc("Social Login Key", "frappe") - except frappe.DoesNotExistError: - frappe_login_key = frappe.new_doc("Social Login Key") - + frappe_login_key: "SocialLoginKey" = frappe.new_doc("Social Login Key") frappe_login_key.get_social_login_provider("Frappe", initialize=True) frappe_login_key.base_url = frappe.utils.get_url() frappe_login_key.enable_social_login = 0 - frappe_login_key.save() + frappe_login_key.insert(ignore_if_duplicate=True) frappe.db.commit() def test_invalid_login(self): - self.assertFalse(check_valid_openid_response()) + with suppress_stdout(): + self.assertFalse(check_valid_openid_response(client=self)) def test_login_using_authorization_code(self): update_client_for_auth_code_grant(self.client_id) - session = requests.Session() - login(session) - - redirect_destination = None - # Go to Authorize url - try: - session.get( - get_full_url("/api/method/frappe.integrations.oauth2.authorize"), - params=encode_params( - { - "client_id": self.client_id, - "scope": self.scope, - "response_type": "code", - "redirect_uri": self.redirect_uri, - } - ), - ) - except requests.exceptions.ConnectionError as ex: - redirect_destination = ex.request.url - - # Get authorization code from redirected URL - query = parse_qs(urlparse(redirect_destination).query) + resp = self.get( + "/api/method/frappe.integrations.oauth2.authorize", + { + "sid": self.sid, + "client_id": self.client_id, + "scope": self.scope, + "response_type": "code", + "redirect_uri": self.redirect_uri, + }, + follow_redirects=True, + ) + query = parse_qs(resp.request.environ["QUERY_STRING"]) auth_code = query.get("code")[0] # Request for bearer token - token_response = requests.post( - get_full_url("/api/method/frappe.integrations.oauth2.get_token"), + token_response = self.post( + "/api/method/frappe.integrations.oauth2.get_token", headers=self.form_header, - data=encode_params( - { - "grant_type": "authorization_code", - "code": auth_code, - "redirect_uri": self.redirect_uri, - "client_id": self.client_id, - "scope": self.scope, - } - ), + data={ + "grant_type": "authorization_code", + "code": auth_code, + "redirect_uri": self.redirect_uri, + "client_id": self.client_id, + "scope": self.scope, + }, ) # Parse bearer token json - bearer_token = token_response.json() + bearer_token = token_response.json self.assertTrue(bearer_token.get("access_token")) self.assertTrue(bearer_token.get("expires_in")) @@ -90,7 +113,9 @@ class TestOAuth20(unittest.TestCase): self.assertTrue(bearer_token.get("refresh_token")) self.assertTrue(bearer_token.get("scope")) self.assertTrue(bearer_token.get("token_type") == "Bearer") - self.assertTrue(check_valid_openid_response(bearer_token.get("access_token"))) + self.assertTrue( + check_valid_openid_response(access_token=bearer_token.get("access_token"), client=self) + ) decoded_token = self.decode_id_token(bearer_token.get("id_token")) self.assertEqual(decoded_token["email"], "test@example.com") @@ -98,51 +123,41 @@ class TestOAuth20(unittest.TestCase): def test_login_using_authorization_code_with_pkce(self): update_client_for_auth_code_grant(self.client_id) - session = requests.Session() - login(session) - - redirect_destination = None - # Go to Authorize url - try: - session.get( - get_full_url("/api/method/frappe.integrations.oauth2.authorize"), - params=encode_params( - { - "client_id": self.client_id, - "scope": self.scope, - "response_type": "code", - "redirect_uri": self.redirect_uri, - "code_challenge_method": "S256", - "code_challenge": "21XaP8MJjpxCMRxgEzBP82sZ73PRLqkyBUta1R309J0", - } - ), - ) - except requests.exceptions.ConnectionError as ex: - redirect_destination = ex.request.url + resp = self.get( + "/api/method/frappe.integrations.oauth2.authorize", + { + "sid": self.sid, + "client_id": self.client_id, + "scope": self.scope, + "response_type": "code", + "redirect_uri": self.redirect_uri, + "code_challenge_method": "S256", + "code_challenge": "21XaP8MJjpxCMRxgEzBP82sZ73PRLqkyBUta1R309J0", + }, + follow_redirects=True, + ) # Get authorization code from redirected URL - query = parse_qs(urlparse(redirect_destination).query) + query = parse_qs(resp.request.environ["QUERY_STRING"]) auth_code = query.get("code")[0] # Request for bearer token - token_response = requests.post( - get_full_url("/api/method/frappe.integrations.oauth2.get_token"), + token_response = self.post( + "/api/method/frappe.integrations.oauth2.get_token", headers=self.form_header, - data=encode_params( - { - "grant_type": "authorization_code", - "code": auth_code, - "redirect_uri": self.redirect_uri, - "client_id": self.client_id, - "scope": self.scope, - "code_verifier": "420", - } - ), + data={ + "grant_type": "authorization_code", + "code": auth_code, + "redirect_uri": self.redirect_uri, + "client_id": self.client_id, + "scope": self.scope, + "code_verifier": "420", + }, ) # Parse bearer token json - bearer_token = token_response.json() + bearer_token = token_response.json self.assertTrue(bearer_token.get("access_token")) self.assertTrue(bearer_token.get("id_token")) @@ -157,51 +172,41 @@ class TestOAuth20(unittest.TestCase): client.save() frappe.db.commit() - session = requests.Session() - login(session) - - redirect_destination = None - # Go to Authorize url - try: - session.get( - get_full_url("/api/method/frappe.integrations.oauth2.authorize"), - params=encode_params( - { - "client_id": self.client_id, - "scope": self.scope, - "response_type": "code", - "redirect_uri": self.redirect_uri, - } - ), - ) - except requests.exceptions.ConnectionError as ex: - redirect_destination = ex.request.url + resp = self.get( + "/api/method/frappe.integrations.oauth2.authorize", + { + "sid": self.sid, + "client_id": self.client_id, + "scope": self.scope, + "response_type": "code", + "redirect_uri": self.redirect_uri, + }, + follow_redirects=True, + ) # Get authorization code from redirected URL - query = parse_qs(urlparse(redirect_destination).query) + query = parse_qs(resp.request.environ["QUERY_STRING"]) auth_code = query.get("code")[0] # Request for bearer token - token_response = requests.post( - get_full_url("/api/method/frappe.integrations.oauth2.get_token"), + token_response = self.post( + "/api/method/frappe.integrations.oauth2.get_token", headers=self.form_header, - data=encode_params( - { - "grant_type": "authorization_code", - "code": auth_code, - "redirect_uri": self.redirect_uri, - "client_id": self.client_id, - } - ), + data={ + "grant_type": "authorization_code", + "code": auth_code, + "redirect_uri": self.redirect_uri, + "client_id": self.client_id, + }, ) # Parse bearer token json - bearer_token = token_response.json() + bearer_token = token_response.json # Revoke Token - revoke_token_response = requests.post( - get_full_url("/api/method/frappe.integrations.oauth2.revoke_token"), + revoke_token_response = self.post( + "/api/method/frappe.integrations.oauth2.revoke_token", headers=self.form_header, data={"token": bearer_token.get("access_token")}, ) @@ -209,7 +214,9 @@ class TestOAuth20(unittest.TestCase): self.assertTrue(revoke_token_response.status_code == 200) # Check revoked token - self.assertFalse(check_valid_openid_response(bearer_token.get("access_token"))) + self.assertFalse( + check_valid_openid_response(access_token=bearer_token.get("access_token"), client=self) + ) def test_resource_owner_password_credentials_grant(self): client = frappe.get_doc("OAuth Client", self.client_id) @@ -219,31 +226,32 @@ class TestOAuth20(unittest.TestCase): frappe.db.commit() # Request for bearer token - token_response = requests.post( - get_full_url("/api/method/frappe.integrations.oauth2.get_token"), + token_response = self.post( + "/api/method/frappe.integrations.oauth2.get_token", + data={ + "grant_type": "password", + "username": "test@example.com", + "password": "Eastern_43A1W", + "client_id": self.client_id, + "scope": self.scope, + }, headers=self.form_header, - data=encode_params( - { - "grant_type": "password", - "username": "test@example.com", - "password": "Eastern_43A1W", - "client_id": self.client_id, - "scope": self.scope, - } - ), ) # Parse bearer token json - bearer_token = token_response.json() + bearer_token = token_response.json # Check token for valid response - self.assertTrue(check_valid_openid_response(bearer_token.get("access_token"))) + self.assertTrue( + check_valid_openid_response(access_token=bearer_token.get("access_token"), client=self) + ) def test_login_using_implicit_token(self): oauth_client = frappe.get_doc("OAuth Client", self.client_id) oauth_client.grant_type = "Implicit" oauth_client.response_type = "Token" oauth_client.save() + oauth_client_before = oauth_client.get_doc_before_save() frappe.db.commit() session = requests.Session() @@ -274,41 +282,34 @@ class TestOAuth20(unittest.TestCase): self.assertTrue(response_dict.get("scope")) self.assertTrue(response_dict.get("token_type")) self.assertTrue(check_valid_openid_response(response_dict.get("access_token")[0])) + oauth_client.delete(force=True) + oauth_client_before.insert() + frappe.db.commit() def test_openid_code_id_token(self): client = update_client_for_auth_code_grant(self.client_id) - - session = requests.Session() - login(session) - - redirect_destination = None - nonce = frappe.generate_hash() # Go to Authorize url - try: - session.get( - get_full_url("/api/method/frappe.integrations.oauth2.authorize"), - params=encode_params( - { - "client_id": self.client_id, - "scope": self.scope, - "response_type": "code", - "redirect_uri": self.redirect_uri, - "nonce": nonce, - } - ), - ) - except requests.exceptions.ConnectionError as ex: - redirect_destination = ex.request.url + resp = self.get( + "/api/method/frappe.integrations.oauth2.authorize", + { + "client_id": self.client_id, + "scope": self.scope, + "response_type": "code", + "redirect_uri": self.redirect_uri, + "nonce": nonce, + }, + follow_redirects=True, + ) # Get authorization code from redirected URL - query = parse_qs(urlparse(redirect_destination).query) + query = parse_qs(resp.request.environ["QUERY_STRING"]) auth_code = query.get("code")[0] # Request for bearer token - token_response = requests.post( - get_full_url("/api/method/frappe.integrations.oauth2.get_token"), + token_response = self.post( + "/api/method/frappe.integrations.oauth2.get_token", headers=self.form_header, data=encode_params( { @@ -322,7 +323,7 @@ class TestOAuth20(unittest.TestCase): ) # Parse bearer token json - bearer_token = token_response.json() + bearer_token = token_response.json payload = self.decode_id_token(bearer_token.get("id_token")) self.assertEqual(payload["email"], "test@example.com") @@ -338,17 +339,20 @@ class TestOAuth20(unittest.TestCase): ) -def check_valid_openid_response(access_token=None): +def check_valid_openid_response(access_token=None, client: "FrappeRequestTestCase" = None): """Return True for valid response.""" # Use token in header headers = {} + URL = "/api/method/frappe.integrations.oauth2.openid_profile" + if access_token: - headers["Authorization"] = "Bearer " + access_token + headers["Authorization"] = f"Bearer {access_token}" # check openid for email test@example.com - openid_response = requests.get( - get_full_url("/api/method/frappe.integrations.oauth2.openid_profile"), headers=headers - ) + if client: + openid_response = client.get(URL, headers=headers) + else: + openid_response = requests.get(get_full_url(URL), headers=headers) return openid_response.status_code == 200 From d6ba7caf923271380b9875cb11ac07fb2c91446c Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 26 Apr 2022 11:46:59 +0530 Subject: [PATCH 02/12] chore: Add typing for Document.doc_before_save --- frappe/model/document.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/model/document.py b/frappe/model/document.py index cadfa573d0..a183acae63 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -419,7 +419,7 @@ class Document(BaseDocument): df.options, {"parent": self.name, "parenttype": self.doctype, "parentfield": fieldname} ) - def get_doc_before_save(self): + def get_doc_before_save(self) -> "Document": return getattr(self, "_doc_before_save", None) def has_value_changed(self, fieldname): From b14f8f4e038020a548657a1ea708b2ebbf38067b Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 26 Apr 2022 11:47:50 +0530 Subject: [PATCH 03/12] feat(minor): Expose force to doc.delete --- frappe/model/document.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/frappe/model/document.py b/frappe/model/document.py index a183acae63..c5b6607da6 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -1025,10 +1025,14 @@ class Document(BaseDocument): """Rename the document to `name`. This transforms the current object.""" return self._rename(name=name, merge=merge, force=force, validate_rename=validate_rename) - def delete(self, ignore_permissions=False): + def delete(self, ignore_permissions=False, force=False): """Delete document.""" return frappe.delete_doc( - self.doctype, self.name, ignore_permissions=ignore_permissions, flags=self.flags + self.doctype, + self.name, + ignore_permissions=ignore_permissions, + flags=self.flags, + force=force, ) def run_before_save_methods(self): From 7f2c9e84b34a98e5579f088e7fd01d12f1aadf5a Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 26 Apr 2022 11:48:37 +0530 Subject: [PATCH 04/12] feat(minor): Expose use_cookies kwarg to test client --- frappe/tests/test_oauth20.py | 2 +- frappe/utils/__init__.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/frappe/tests/test_oauth20.py b/frappe/tests/test_oauth20.py index 8ebff2bca6..a5bd3739d3 100644 --- a/frappe/tests/test_oauth20.py +++ b/frappe/tests/test_oauth20.py @@ -35,7 +35,7 @@ class FrappeRequestTestCase(unittest.TestCase): return self._sid - def get(self, path: str, params: Optional[Dict] = None, **kwargs) -> TestResponse: + def get(self, path: str, params: dict | None = None, **kwargs) -> TestResponse: return make_request(target=self.TEST_CLIENT.get, args=(path,), kwargs={"data": params, **kwargs}) def post(self, path, data, **kwargs) -> TestResponse: diff --git a/frappe/utils/__init__.py b/frappe/utils/__init__.py index 47eae314f7..db176a5c0b 100644 --- a/frappe/utils/__init__.py +++ b/frappe/utils/__init__.py @@ -525,11 +525,11 @@ def touch_file(path): return path -def get_test_client() -> Client: +def get_test_client(use_cookies=True) -> Client: """Returns an test instance of the Frappe WSGI""" from frappe.app import application - return Client(application) + return Client(application, use_cookies=use_cookies) def get_hook_method(hook_name, fallback=None): From f7d5cb504a951058e9e6e618de7796fcc9bc4e63 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Wed, 3 Aug 2022 12:00:09 +0530 Subject: [PATCH 05/12] test(oauth-client): Generate new client for each test --- .../doctype/oauth_client/test_records.json | 15 ---------- frappe/tests/test_oauth20.py | 30 ++++++++++++++++--- 2 files changed, 26 insertions(+), 19 deletions(-) delete mode 100644 frappe/integrations/doctype/oauth_client/test_records.json diff --git a/frappe/integrations/doctype/oauth_client/test_records.json b/frappe/integrations/doctype/oauth_client/test_records.json deleted file mode 100644 index 11e6338a87..0000000000 --- a/frappe/integrations/doctype/oauth_client/test_records.json +++ /dev/null @@ -1,15 +0,0 @@ -[ - { - "app_name": "_Test OAuth Client", - "client_secret": "test_client_secret", - "default_redirect_uri": "http://localhost", - "docstatus": 0, - "doctype": "OAuth Client", - "grant_type": "Authorization Code", - "name": "test_client_id", - "redirect_uris": "http://localhost", - "response_type": "Code", - "scopes": "all openid", - "skip_authorization": 1 - } -] diff --git a/frappe/tests/test_oauth20.py b/frappe/tests/test_oauth20.py index a5bd3739d3..544a11d201 100644 --- a/frappe/tests/test_oauth20.py +++ b/frappe/tests/test_oauth20.py @@ -51,12 +51,8 @@ class FrappeRequestTestCase(unittest.TestCase): class TestOAuth20(FrappeRequestTestCase): @classmethod def setUpClass(cls): - make_test_records("OAuth Client", force=True) make_test_records("User") - client = frappe.get_all("OAuth Client", fields=["*"])[0] - cls.client_id = client.get("client_id") - cls.client_secret = client.get("client_secret") cls.form_header = {"content-type": "application/x-www-form-urlencoded"} cls.scope = "all openid" cls.redirect_uri = "http://localhost" @@ -69,6 +65,32 @@ class TestOAuth20(FrappeRequestTestCase): frappe_login_key.insert(ignore_if_duplicate=True) frappe.db.commit() + def setUp(self): + self.oauth_client = frappe.new_doc("OAuth Client") + self.oauth_client.update( + { + "app_name": "_Test OAuth Client", + "client_secret": "test_client_secret", + "default_redirect_uri": "http://localhost", + "docstatus": 0, + "doctype": "OAuth Client", + "grant_type": "Authorization Code", + "name": "test_client_id", + "redirect_uris": "http://localhost", + "response_type": "Code", + "scopes": "all openid", + "skip_authorization": 1, + } + ) + self.oauth_client.insert() + + self.client_id = self.oauth_client.get("client_id") + self.client_secret = self.oauth_client.get("client_secret") + + def tearDown(self): + self.oauth_client.delete(force=True) + frappe.db.rollback() + def test_invalid_login(self): with suppress_stdout(): self.assertFalse(check_valid_openid_response(client=self)) From 750618ca7cd16e90cb7f92c4406ff461dd25849e Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Wed, 3 Aug 2022 12:01:12 +0530 Subject: [PATCH 06/12] fix: Re-raise original exception from tenacity's retry --- frappe/core/doctype/access_log/access_log.py | 6 +++++- frappe/utils/background_jobs.py | 1 + 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/frappe/core/doctype/access_log/access_log.py b/frappe/core/doctype/access_log/access_log.py index b7a6d77206..ca2909b970 100644 --- a/frappe/core/doctype/access_log/access_log.py +++ b/frappe/core/doctype/access_log/access_log.py @@ -35,7 +35,11 @@ def make_access_log( @frappe.write_only() -@retry(stop=stop_after_attempt(3), retry=retry_if_exception_type(frappe.DuplicateEntryError)) +@retry( + stop=stop_after_attempt(3), + retry=retry_if_exception_type(frappe.DuplicateEntryError), + reraise=True, +) def _make_access_log( doctype=None, document=None, diff --git a/frappe/utils/background_jobs.py b/frappe/utils/background_jobs.py index 3d3df3504d..dde0d64169 100755 --- a/frappe/utils/background_jobs.py +++ b/frappe/utils/background_jobs.py @@ -295,6 +295,7 @@ def validate_queue(queue, default_queue_list=None): retry=retry_if_exception_type(BusyLoadingError) | retry_if_exception_type(ConnectionError), stop=stop_after_attempt(10), wait=wait_fixed(1), + reraise=True, ) def get_redis_conn(username=None, password=None): if not hasattr(frappe.local, "conf"): From 2ae50f911ade8fbaac256a692d8a778969c0c557 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Wed, 3 Aug 2022 12:01:36 +0530 Subject: [PATCH 07/12] chore: Minimize OAuth Client DocType --- .../doctype/oauth_client/oauth_client.json | 449 ++---------------- 1 file changed, 38 insertions(+), 411 deletions(-) diff --git a/frappe/integrations/doctype/oauth_client/oauth_client.json b/frappe/integrations/doctype/oauth_client/oauth_client.json index d0d45c36ab..3368d94fb0 100644 --- a/frappe/integrations/doctype/oauth_client/oauth_client.json +++ b/frappe/integrations/doctype/oauth_client/oauth_client.json @@ -1,517 +1,144 @@ { - "allow_copy": 0, - "allow_guest_to_view": 0, - "allow_import": 0, - "allow_rename": 0, - "autoname": "", - "beta": 0, + "actions": [], "creation": "2016-08-24 14:07:21.955052", - "custom": 0, - "docstatus": 0, "doctype": "DocType", "document_type": "Document", "editable_grid": 1, "engine": "InnoDB", + "field_order": [ + "client_id", + "app_name", + "user", + "cb_1", + "client_secret", + "skip_authorization", + "sb_1", + "scopes", + "cb_3", + "redirect_uris", + "default_redirect_uri", + "sb_advanced", + "grant_type", + "cb_2", + "response_type" + ], "fields": [ { - "allow_bulk_edit": 0, - "allow_on_submit": 0, - "bold": 0, - "collapsible": 0, - "columns": 0, - "default": "", "fieldname": "client_id", "fieldtype": "Data", - "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": "App Client ID", - "length": 0, - "no_copy": 0, - "permlevel": 0, - "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 + "read_only": 1 }, { - "allow_bulk_edit": 0, - "allow_on_submit": 0, - "bold": 0, - "collapsible": 0, - "columns": 0, "fieldname": "app_name", "fieldtype": "Data", - "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": "App Name", - "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": 1, - "search_index": 0, - "set_only_once": 0, - "unique": 0 + "reqd": 1 }, { - "allow_bulk_edit": 0, - "allow_on_submit": 0, - "bold": 0, - "collapsible": 0, - "columns": 0, "fieldname": "user", "fieldtype": "Link", "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": "User", - "length": 0, - "no_copy": 0, - "options": "User", - "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 + "options": "User" }, { - "allow_bulk_edit": 0, - "allow_on_submit": 0, - "bold": 0, - "collapsible": 0, - "columns": 0, "fieldname": "cb_1", - "fieldtype": "Column Break", - "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, - "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 + "fieldtype": "Column Break" }, { - "allow_bulk_edit": 0, - "allow_on_submit": 0, - "bold": 0, - "collapsible": 0, - "columns": 0, "fieldname": "client_secret", "fieldtype": "Data", - "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": "App Client Secret", - "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 + "read_only": 1 }, { - "allow_bulk_edit": 0, - "allow_on_submit": 0, - "bold": 0, - "collapsible": 0, - "columns": 0, + "default": "0", "description": "If checked, users will not see the Confirm Access dialog.", "fieldname": "skip_authorization", "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": "Skip Authorization", - "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 + "label": "Skip Authorization" }, { - "allow_bulk_edit": 0, - "allow_on_submit": 0, - "bold": 0, - "collapsible": 0, - "columns": 0, - "description": "", "fieldname": "sb_1", - "fieldtype": "Section Break", - "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": "", - "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 + "fieldtype": "Section Break" }, { - "allow_bulk_edit": 0, - "allow_on_submit": 0, - "bold": 0, - "collapsible": 0, - "columns": 0, "default": "all openid", "description": "A list of resources which the Client App will have access to after the user allows it.
e.g. project", "fieldname": "scopes", "fieldtype": "Text", - "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": "Scopes", - "length": 0, - "no_copy": 0, - "permlevel": 0, - "print_hide": 0, - "print_hide_if_no_value": 0, - "read_only": 0, - "remember_last_selected_value": 0, - "report_hide": 0, - "reqd": 1, - "search_index": 0, - "set_only_once": 0, - "unique": 0 + "reqd": 1 }, { - "allow_bulk_edit": 0, - "allow_on_submit": 0, - "bold": 0, - "collapsible": 0, - "columns": 0, "fieldname": "cb_3", - "fieldtype": "Column Break", - "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, - "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 + "fieldtype": "Column Break" }, { - "allow_bulk_edit": 0, - "allow_on_submit": 0, - "bold": 0, - "collapsible": 0, - "columns": 0, "description": "URIs for receiving authorization code once the user allows access, as well as failure responses. Typically a REST endpoint exposed by the Client App.\n
e.g. http://hostname//api/method/frappe.www.login.login_via_facebook", "fieldname": "redirect_uris", "fieldtype": "Text", - "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": "Redirect URIs", - "length": 0, - "no_copy": 0, - "permlevel": 0, - "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 + "label": "Redirect URIs" }, { - "allow_bulk_edit": 0, - "allow_on_submit": 0, - "bold": 0, - "collapsible": 0, - "columns": 0, "fieldname": "default_redirect_uri", "fieldtype": "Data", - "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": "Default Redirect URI", - "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": 1, - "search_index": 0, - "set_only_once": 0, - "unique": 0 + "reqd": 1 }, { - "allow_bulk_edit": 0, - "allow_on_submit": 0, - "bold": 0, "collapsible": 1, "collapsible_depends_on": "1", - "columns": 0, "fieldname": "sb_advanced", "fieldtype": "Section Break", - "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": "Advanced Settings", - "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 + "label": "Advanced Settings" }, { - "allow_bulk_edit": 0, - "allow_on_submit": 0, - "bold": 0, - "collapsible": 0, - "columns": 0, "fieldname": "grant_type", "fieldtype": "Select", - "hidden": 0, - "ignore_user_permissions": 0, - "ignore_xss_filter": 0, - "in_filter": 0, - "in_global_search": 0, "in_list_view": 1, "in_standard_filter": 1, "label": "Grant Type", - "length": 0, - "no_copy": 0, - "options": "Authorization Code\nImplicit", - "permlevel": 0, - "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 + "options": "Authorization Code\nImplicit" }, { - "allow_bulk_edit": 0, - "allow_on_submit": 0, - "bold": 0, - "collapsible": 0, - "columns": 0, "fieldname": "cb_2", - "fieldtype": "Column Break", - "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, - "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 + "fieldtype": "Column Break" }, { - "allow_bulk_edit": 0, - "allow_on_submit": 0, - "bold": 0, - "collapsible": 0, - "columns": 0, "default": "Code", "fieldname": "response_type", "fieldtype": "Select", - "hidden": 0, - "ignore_user_permissions": 0, - "ignore_xss_filter": 0, - "in_filter": 0, - "in_global_search": 0, "in_list_view": 1, "in_standard_filter": 1, "label": "Response Type", - "length": 0, - "no_copy": 0, - "options": "Code\nToken", - "permlevel": 0, - "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 + "options": "Code\nToken" } ], - "has_web_view": 0, - "hide_heading": 0, - "hide_toolbar": 0, - "idx": 0, - "image_view": 0, - "in_create": 0, - "is_submittable": 0, - "issingle": 0, - "istable": 0, - "max_attachments": 0, - "modified": "2020-04-07 21:07:39.476360", + "links": [], + "modified": "2022-08-03 11:51:27.709726", "modified_by": "Administrator", "module": "Integrations", "name": "OAuth Client", - "name_case": "", "owner": "Administrator", "permissions": [ { - "amend": 0, - "apply_user_permissions": 0, - "cancel": 0, "create": 1, "delete": 1, "email": 1, "export": 1, - "if_owner": 0, - "import": 0, - "permlevel": 0, "print": 1, "read": 1, "report": 1, "role": "System Manager", - "set_user_permissions": 0, "share": 1, - "submit": 0, "write": 1 } ], - "quick_entry": 0, - "read_only": 0, - "read_only_onload": 0, - "show_name_in_global_search": 0, "sort_field": "modified", "sort_order": "DESC", + "states": [], "title_field": "app_name", - "track_changes": 1, - "track_seen": 0 + "track_changes": 1 } \ No newline at end of file From 060498c3e4978464ad7c826560c495e094cf5c2b Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 5 Aug 2022 10:41:13 +0530 Subject: [PATCH 08/12] test: Pass sid to requests to maintain a session --- frappe/tests/test_oauth20.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frappe/tests/test_oauth20.py b/frappe/tests/test_oauth20.py index 544a11d201..8ef69f7a6a 100644 --- a/frappe/tests/test_oauth20.py +++ b/frappe/tests/test_oauth20.py @@ -316,6 +316,7 @@ class TestOAuth20(FrappeRequestTestCase): resp = self.get( "/api/method/frappe.integrations.oauth2.authorize", { + "sid": self.sid, "client_id": self.client_id, "scope": self.scope, "response_type": "code", @@ -335,6 +336,7 @@ class TestOAuth20(FrappeRequestTestCase): headers=self.form_header, data=encode_params( { + "sid": self.sid, "grant_type": "authorization_code", "code": auth_code, "redirect_uri": self.redirect_uri, From 832fbb6c853e861c4bc1ec77fbedf032704159e1 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 5 Aug 2022 11:39:39 +0530 Subject: [PATCH 09/12] chore: Remove dead imports --- frappe/tests/test_oauth20.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/tests/test_oauth20.py b/frappe/tests/test_oauth20.py index 8ef69f7a6a..0a16269121 100644 --- a/frappe/tests/test_oauth20.py +++ b/frappe/tests/test_oauth20.py @@ -2,7 +2,7 @@ # License: MIT. See LICENSE import unittest -from typing import TYPE_CHECKING, Dict, Optional +from typing import TYPE_CHECKING from urllib.parse import parse_qs, urljoin, urlparse import jwt From b00ae8043f83f0d09ae013438be8389e822e1137 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 5 Aug 2022 12:00:50 +0530 Subject: [PATCH 10/12] test: Set sid as client cookie instead of oauth payload --- frappe/tests/test_oauth20.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/frappe/tests/test_oauth20.py b/frappe/tests/test_oauth20.py index 0a16269121..d0ddd2facd 100644 --- a/frappe/tests/test_oauth20.py +++ b/frappe/tests/test_oauth20.py @@ -99,10 +99,10 @@ class TestOAuth20(FrappeRequestTestCase): update_client_for_auth_code_grant(self.client_id) # Go to Authorize url + self.TEST_CLIENT.set_cookie(frappe.local.site, key="sid", value=self.sid) resp = self.get( "/api/method/frappe.integrations.oauth2.authorize", { - "sid": self.sid, "client_id": self.client_id, "scope": self.scope, "response_type": "code", @@ -146,10 +146,10 @@ class TestOAuth20(FrappeRequestTestCase): update_client_for_auth_code_grant(self.client_id) # Go to Authorize url + self.TEST_CLIENT.set_cookie(frappe.local.site, key="sid", value=self.sid) resp = self.get( "/api/method/frappe.integrations.oauth2.authorize", { - "sid": self.sid, "client_id": self.client_id, "scope": self.scope, "response_type": "code", @@ -195,10 +195,10 @@ class TestOAuth20(FrappeRequestTestCase): frappe.db.commit() # Go to Authorize url + self.TEST_CLIENT.set_cookie(frappe.local.site, key="sid", value=self.sid) resp = self.get( "/api/method/frappe.integrations.oauth2.authorize", { - "sid": self.sid, "client_id": self.client_id, "scope": self.scope, "response_type": "code", @@ -313,10 +313,10 @@ class TestOAuth20(FrappeRequestTestCase): nonce = frappe.generate_hash() # Go to Authorize url + self.TEST_CLIENT.set_cookie(frappe.local.site, key="sid", value=self.sid) resp = self.get( "/api/method/frappe.integrations.oauth2.authorize", { - "sid": self.sid, "client_id": self.client_id, "scope": self.scope, "response_type": "code", @@ -336,7 +336,6 @@ class TestOAuth20(FrappeRequestTestCase): headers=self.form_header, data=encode_params( { - "sid": self.sid, "grant_type": "authorization_code", "code": auth_code, "redirect_uri": self.redirect_uri, From 7b149d4273bb133f3290995f180ddcce0ab2e055 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 5 Aug 2022 13:54:58 +0530 Subject: [PATCH 11/12] fix: Allow setting kwarg site for make_request test util --- frappe/tests/test_api.py | 12 ++++++++---- frappe/tests/test_oauth20.py | 24 ++++++++++++++++-------- 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/frappe/tests/test_api.py b/frappe/tests/test_api.py index 0464f54530..9ee17e91f3 100644 --- a/frappe/tests/test_api.py +++ b/frappe/tests/test_api.py @@ -32,9 +32,12 @@ def suppress_stdout(): def make_request( - target: str, args: tuple | None = None, kwargs: dict | None = None + target: str, + args: tuple | None = None, + kwargs: dict | None = None, + site: str = None, ) -> TestResponse: - t = ThreadWithReturnValue(target=target, args=args, kwargs=kwargs) + t = ThreadWithReturnValue(target=target, args=args, kwargs=kwargs, site=site) t.start() t.join() return t._return @@ -46,13 +49,14 @@ def patch_request_header(key, *args, **kwargs): class ThreadWithReturnValue(Thread): - def __init__(self, group=None, target=None, name=None, args=(), kwargs={}): + def __init__(self, group=None, target=None, name=None, args=(), kwargs={}, *, site=None): Thread.__init__(self, group, target, name, args, kwargs) self._return = None + self.site = site or _site def run(self): if self._target is not None: - with patch("frappe.app.get_site_name", return_value=_site): + with patch("frappe.app.get_site_name", return_value=self.site): header_patch = patch("frappe.get_request_header", new=patch_request_header) if authorization_token: header_patch.start() diff --git a/frappe/tests/test_oauth20.py b/frappe/tests/test_oauth20.py index d0ddd2facd..5ae797c28d 100644 --- a/frappe/tests/test_oauth20.py +++ b/frappe/tests/test_oauth20.py @@ -36,19 +36,27 @@ class FrappeRequestTestCase(unittest.TestCase): return self._sid def get(self, path: str, params: dict | None = None, **kwargs) -> TestResponse: - return make_request(target=self.TEST_CLIENT.get, args=(path,), kwargs={"data": params, **kwargs}) + return make_request( + target=self.TEST_CLIENT.get, args=(path,), kwargs={"data": params, **kwargs}, site=self.site + ) def post(self, path, data, **kwargs) -> TestResponse: - return make_request(target=self.TEST_CLIENT.post, args=(path,), kwargs={"data": data, **kwargs}) + return make_request( + target=self.TEST_CLIENT.post, args=(path,), kwargs={"data": data, **kwargs}, site=self.site + ) def put(self, path, data, **kwargs) -> TestResponse: - return make_request(target=self.TEST_CLIENT.put, args=(path,), kwargs={"data": data, **kwargs}) + return make_request( + target=self.TEST_CLIENT.put, args=(path,), kwargs={"data": data, **kwargs}, site=self.site + ) def delete(self, path, **kwargs) -> TestResponse: - return make_request(target=self.TEST_CLIENT.delete, args=(path,), kwargs=kwargs) + return make_request(target=self.TEST_CLIENT.delete, args=(path,), kwargs=kwargs, site=self.site) class TestOAuth20(FrappeRequestTestCase): + site = frappe.local.site + @classmethod def setUpClass(cls): make_test_records("User") @@ -99,7 +107,7 @@ class TestOAuth20(FrappeRequestTestCase): update_client_for_auth_code_grant(self.client_id) # Go to Authorize url - self.TEST_CLIENT.set_cookie(frappe.local.site, key="sid", value=self.sid) + self.TEST_CLIENT.set_cookie(self.site, key="sid", value=self.sid) resp = self.get( "/api/method/frappe.integrations.oauth2.authorize", { @@ -146,7 +154,7 @@ class TestOAuth20(FrappeRequestTestCase): update_client_for_auth_code_grant(self.client_id) # Go to Authorize url - self.TEST_CLIENT.set_cookie(frappe.local.site, key="sid", value=self.sid) + self.TEST_CLIENT.set_cookie(self.site, key="sid", value=self.sid) resp = self.get( "/api/method/frappe.integrations.oauth2.authorize", { @@ -195,7 +203,7 @@ class TestOAuth20(FrappeRequestTestCase): frappe.db.commit() # Go to Authorize url - self.TEST_CLIENT.set_cookie(frappe.local.site, key="sid", value=self.sid) + self.TEST_CLIENT.set_cookie(self.site, key="sid", value=self.sid) resp = self.get( "/api/method/frappe.integrations.oauth2.authorize", { @@ -313,7 +321,7 @@ class TestOAuth20(FrappeRequestTestCase): nonce = frappe.generate_hash() # Go to Authorize url - self.TEST_CLIENT.set_cookie(frappe.local.site, key="sid", value=self.sid) + self.TEST_CLIENT.set_cookie(self.site, key="sid", value=self.sid) resp = self.get( "/api/method/frappe.integrations.oauth2.authorize", { From e7a16135e0e0b0c6cd42e6780a09ccfe3833ab80 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 5 Aug 2022 15:12:29 +0530 Subject: [PATCH 12/12] fix: Setup test_client for each test --- frappe/tests/test_oauth20.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frappe/tests/test_oauth20.py b/frappe/tests/test_oauth20.py index 5ae797c28d..c615138810 100644 --- a/frappe/tests/test_oauth20.py +++ b/frappe/tests/test_oauth20.py @@ -19,8 +19,6 @@ if TYPE_CHECKING: class FrappeRequestTestCase(unittest.TestCase): - TEST_CLIENT = get_test_client() - @property def sid(self) -> str: if not getattr(self, "_sid", None): @@ -74,6 +72,7 @@ class TestOAuth20(FrappeRequestTestCase): frappe.db.commit() def setUp(self): + self.TEST_CLIENT = get_test_client() self.oauth_client = frappe.new_doc("OAuth Client") self.oauth_client.update( {