From ca9b7a2474d52d1767318dca90c5b3740ff49c72 Mon Sep 17 00:00:00 2001 From: Logan Bertram Date: Wed, 30 Oct 2024 07:34:04 -0400 Subject: [PATCH 01/17] Long term fix for scope creep --- apps/capabilities/permissions.py | 23 ++++++++----------- apps/capabilities/tests.py | 2 -- .../tests/demographic_scopes_test_cases.py | 6 ++--- apps/dot_ext/tests/test_views.py | 5 +--- apps/dot_ext/views/authorization.py | 12 ++++++++-- 5 files changed, 23 insertions(+), 25 deletions(-) diff --git a/apps/capabilities/permissions.py b/apps/capabilities/permissions.py index a7bc3b726..ece09f048 100644 --- a/apps/capabilities/permissions.py +++ b/apps/capabilities/permissions.py @@ -37,20 +37,15 @@ def has_permission(self, request, view): slug__in=token_scopes ).values_list('protected_resources', flat=True).all()) - # this is a shorterm fix to reject all tokens that do not have either - # patient/coverage.read or patient/ExplanationOfBenefit.read - if ("patient/Coverage.read" in token_scopes) or ("patient/ExplanationOfBenefit.read" in token_scopes): - for scope in scopes: - for method, path in json.loads(scope): - if method != request.method: - continue - if path == request.path: - return True - if re.fullmatch(path, request.path) is not None: - return True - return False - else: - return False + for scope in scopes: + for method, path in json.loads(scope): + if method != request.method: + continue + if path == request.path: + return True + if re.fullmatch(path, request.path) is not None: + return True + return False else: # BB2-237: Replaces ASSERT with exception. We should never reach here. mesg = ("TokenHasScope requires the `oauth2_provider.rest_framework.OAuth2Authentication`" diff --git a/apps/capabilities/tests.py b/apps/capabilities/tests.py index b320398f5..396faf9d2 100644 --- a/apps/capabilities/tests.py +++ b/apps/capabilities/tests.py @@ -1,5 +1,4 @@ import json -import unittest from django.contrib.auth.models import Group from django.test import TestCase @@ -41,7 +40,6 @@ def setUp(self): protected_resources=json.dumps([["POST", "/path"]]), ) - @unittest.skip("Broke with quick fix") def test_request_is_protected(self): request = SimpleRequest("scope") request.method = "GET" diff --git a/apps/dot_ext/tests/demographic_scopes_test_cases.py b/apps/dot_ext/tests/demographic_scopes_test_cases.py index 03dd25bf4..d3b36dbbb 100644 --- a/apps/dot_ext/tests/demographic_scopes_test_cases.py +++ b/apps/dot_ext/tests/demographic_scopes_test_cases.py @@ -181,7 +181,7 @@ "request_scopes": APPLICATION_SCOPES_FULL, # Result: "result_has_error": False, - "result_token_scopes_granted": APPLICATION_SCOPES_FULL, + "result_token_scopes_granted": APPLICATION_SCOPES_NON_DEMOGRAPHIC, "result_access_token_count": 1, "result_refresh_token_count": 1, "result_archived_token_count": 0, @@ -221,7 +221,7 @@ "request_scopes": APPLICATION_SCOPES_FULL, # Result: "result_has_error": False, - "result_token_scopes_granted": APPLICATION_SCOPES_FULL, + "result_token_scopes_granted": APPLICATION_SCOPES_NON_DEMOGRAPHIC, "result_access_token_count": 3, "result_refresh_token_count": 3, "result_archived_token_count": 1, @@ -314,7 +314,7 @@ "request_scopes": SCOPES_JUST_PATIENT_AND_A, # Result: "result_has_error": False, - "result_token_scopes_granted": SCOPES_JUST_PATIENT_AND_A, + "result_token_scopes_granted": SCOPES_JUST_A, "result_access_token_count": 3, "result_refresh_token_count": 3, "result_archived_token_count": 8, diff --git a/apps/dot_ext/tests/test_views.py b/apps/dot_ext/tests/test_views.py index 4018606bd..8eca87ae1 100644 --- a/apps/dot_ext/tests/test_views.py +++ b/apps/dot_ext/tests/test_views.py @@ -1,6 +1,5 @@ import json import base64 -import unittest from datetime import date, timedelta from django.conf import settings @@ -163,18 +162,15 @@ def test_post_with_restricted_scopes_issues_token_with_same_scopes(self): # and here we test that only the capability-a scope has been issued self.assertEqual(content["scope"], "capability-a") - @unittest.skip("Broke with quick fix") def test_post_with_share_demographic_scopes(self): # Test with-out new_auth switch self.testing_post_with_share_demographic_scopes() - @unittest.skip("Broke with quick fix") @override_switch("new_auth", active=True) def test_post_with_share_demographic_scopes_new_auth_switch(self): # Test with new_auth switch. self.testing_post_with_share_demographic_scopes() - @unittest.skip("Broke with quick fix") @override_switch("require-scopes", active=True) def testing_post_with_share_demographic_scopes(self): """ @@ -206,6 +202,7 @@ def testing_post_with_share_demographic_scopes(self): # Loop through test cases in dictionary cases = VIEW_OAUTH2_SCOPES_TEST_CASES for case in cases: + print(case) # Setup request parameters for test case request_bene_share_demographic_scopes = cases[case][ "request_bene_share_demographic_scopes" diff --git a/apps/dot_ext/views/authorization.py b/apps/dot_ext/views/authorization.py index 770fa395e..06417db39 100644 --- a/apps/dot_ext/views/authorization.py +++ b/apps/dot_ext/views/authorization.py @@ -6,6 +6,7 @@ import waffle from waffle import get_waffle_flag_model +from django.conf import settings from django.http.response import HttpResponse, HttpResponseBadRequest from django.template.response import TemplateResponse from django.utils.decorators import method_decorator @@ -170,8 +171,15 @@ def form_valid(self, form): application_available_scopes = CapabilitiesScopes().get_available_scopes(application=application) # Set scopes to those available to application and beneficiary demographic info choices - scopes = ' '.join([s for s in scopes.split(" ") - if s in application_available_scopes]) + if share_demographic_scopes: + scopes = ' '.join( + [s for s in scopes.split(" ") if s in application_available_scopes] + ) + else: + scopes = ' '.join( + [s for s in scopes.split(" ") + if s in application_available_scopes and s not in settings.BENE_PERSONAL_INFO_SCOPES] + ) # Init deleted counts data_access_grant_delete_cnt = 0 From 9272b54317c0c13139ccf0ab004c5a53a3c4f40b Mon Sep 17 00:00:00 2001 From: Logan Bertram Date: Wed, 30 Oct 2024 09:51:17 -0400 Subject: [PATCH 02/17] Updated string check --- apps/dot_ext/views/authorization.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/dot_ext/views/authorization.py b/apps/dot_ext/views/authorization.py index 06417db39..44f6315fe 100644 --- a/apps/dot_ext/views/authorization.py +++ b/apps/dot_ext/views/authorization.py @@ -171,7 +171,7 @@ def form_valid(self, form): application_available_scopes = CapabilitiesScopes().get_available_scopes(application=application) # Set scopes to those available to application and beneficiary demographic info choices - if share_demographic_scopes: + if share_demographic_scopes == "True": scopes = ' '.join( [s for s in scopes.split(" ") if s in application_available_scopes] ) From 1f5aeb6ffe5e2ebfb4b4f098d1063b6a1863b119 Mon Sep 17 00:00:00 2001 From: Logan Bertram Date: Wed, 30 Oct 2024 11:27:19 -0400 Subject: [PATCH 03/17] Updated scope gate --- apps/dot_ext/tests/test_views.py | 1 - apps/dot_ext/views/authorization.py | 54 ++++------------------------- 2 files changed, 7 insertions(+), 48 deletions(-) diff --git a/apps/dot_ext/tests/test_views.py b/apps/dot_ext/tests/test_views.py index 8eca87ae1..8543d2c1b 100644 --- a/apps/dot_ext/tests/test_views.py +++ b/apps/dot_ext/tests/test_views.py @@ -202,7 +202,6 @@ def testing_post_with_share_demographic_scopes(self): # Loop through test cases in dictionary cases = VIEW_OAUTH2_SCOPES_TEST_CASES for case in cases: - print(case) # Setup request parameters for test case request_bene_share_demographic_scopes = cases[case][ "request_bene_share_demographic_scopes" diff --git a/apps/dot_ext/views/authorization.py b/apps/dot_ext/views/authorization.py index 40d7a956d..d8a677e9a 100644 --- a/apps/dot_ext/views/authorization.py +++ b/apps/dot_ext/views/authorization.py @@ -6,7 +6,6 @@ import waffle from waffle import get_waffle_flag_model -from django.conf import settings from django.http.response import HttpResponse, HttpResponseBadRequest from django.template.response import TemplateResponse from django.utils.decorators import method_decorator @@ -23,7 +22,7 @@ from oauth2_provider.models import get_application_model from oauthlib.oauth2.rfc6749.errors import InvalidClientError, InvalidGrantError from urllib.parse import urlparse, parse_qs -import html + from apps.dot_ext.scopes import CapabilitiesScopes import apps.logging.request_logger as bb2logging @@ -121,12 +120,12 @@ def sensitive_info_check(self, request): def get_template_names(self): flag = get_waffle_flag_model().get("limit_data_access") if waffle.switch_is_active('require-scopes'): - if flag.rollout or (flag.id is not None and self.application and flag.is_active_for_user(self.application.user)): + if flag.rollout or (flag.id is not None and flag.is_active_for_user(self.application.user)): return ["design_system/new_authorize_v2.html"] else: return ["design_system/authorize_v2.html"] else: - if flag.rollout or (flag.id is not None and self.user and flag.is_active_for_user(self.user)): + if flag.rollout or (flag.id is not None and flag.is_active_for_user(self.user)): return ["design_system/new_authorize_v2.html"] else: return ["design_system/authorize.html"] @@ -171,21 +170,16 @@ def form_valid(self, form): application_available_scopes = CapabilitiesScopes().get_available_scopes(application=application) # Set scopes to those available to application and beneficiary demographic info choices - if share_demographic_scopes == "True": - scopes = ' '.join( - [s for s in scopes.split(" ") if s in application_available_scopes] - ) - else: - scopes = ' '.join( - [s for s in scopes.split(" ") - if s in application_available_scopes and s not in settings.BENE_PERSONAL_INFO_SCOPES] - ) + scopes = ' '.join([s for s in scopes.split(" ") + if s in application_available_scopes]) # Init deleted counts data_access_grant_delete_cnt = 0 access_token_delete_cnt = 0 refresh_token_delete_cnt = 0 + if not scopes: + return self.error_response("No scopes defined", application) try: uri, headers, body, status = self.create_authorization_response( request=self.request, scopes=scopes, credentials=credentials, allow=allow @@ -362,40 +356,6 @@ def post(self, request, *args, **kwargs): return super().post(request, args, kwargs) -@method_decorator(csrf_exempt, name="dispatch") -class RevokeView(DotRevokeTokenView): - - @method_decorator(sensitive_post_parameters("password")) - def post(self, request, *args, **kwargs): - at_model = get_access_token_model() - try: - app = validate_app_is_active(request) - except (InvalidClientError, InvalidGrantError) as error: - return json_response_from_oauth2_error(error) - - tkn = request.POST.get('token') - if tkn is not None: - escaped_tkn = html.escape(tkn) - else: - escaped_tkn = "" - - try: - token = at_model.objects.get(token=tkn) - except at_model.DoesNotExist: - log.debug(f"Token {escaped_tkn} was not found.") - - try: - dag = DataAccessGrant.objects.get( - beneficiary=token.user, - application=app - ) - dag.delete() - except Exception: - log.debug(f"DAG lookup failed for token {escaped_tkn}.") - - return super().post(request, args, kwargs) - - @method_decorator(csrf_exempt, name="dispatch") class IntrospectTokenView(DotIntrospectTokenView): From 6ecb4d7e39ab98bf877b811db6b731dd6321c59a Mon Sep 17 00:00:00 2001 From: Logan Bertram Date: Wed, 30 Oct 2024 12:06:01 -0400 Subject: [PATCH 04/17] Fix revoke and tests --- .../tests/demographic_scopes_test_cases.py | 8 ++-- apps/dot_ext/tests/test_views.py | 1 + apps/dot_ext/views/authorization.py | 42 +++++++++++++++++-- 3 files changed, 43 insertions(+), 8 deletions(-) diff --git a/apps/dot_ext/tests/demographic_scopes_test_cases.py b/apps/dot_ext/tests/demographic_scopes_test_cases.py index d3b36dbbb..7dab48c0e 100644 --- a/apps/dot_ext/tests/demographic_scopes_test_cases.py +++ b/apps/dot_ext/tests/demographic_scopes_test_cases.py @@ -68,7 +68,7 @@ "request_scopes": APPLICATION_SCOPES_FULL, # Result: "result_form_is_valid": True, - "result_token_scopes_granted": APPLICATION_SCOPES_FULL, + "result_token_scopes_granted": APPLICATION_SCOPES_NON_DEMOGRAPHIC, }, "test 2: share_demographic_scopes = False": { # Request: @@ -181,7 +181,7 @@ "request_scopes": APPLICATION_SCOPES_FULL, # Result: "result_has_error": False, - "result_token_scopes_granted": APPLICATION_SCOPES_NON_DEMOGRAPHIC, + "result_token_scopes_granted": APPLICATION_SCOPES_FULL, "result_access_token_count": 1, "result_refresh_token_count": 1, "result_archived_token_count": 0, @@ -221,7 +221,7 @@ "request_scopes": APPLICATION_SCOPES_FULL, # Result: "result_has_error": False, - "result_token_scopes_granted": APPLICATION_SCOPES_NON_DEMOGRAPHIC, + "result_token_scopes_granted": APPLICATION_SCOPES_FULL, "result_access_token_count": 3, "result_refresh_token_count": 3, "result_archived_token_count": 1, @@ -314,7 +314,7 @@ "request_scopes": SCOPES_JUST_PATIENT_AND_A, # Result: "result_has_error": False, - "result_token_scopes_granted": SCOPES_JUST_A, + "result_token_scopes_granted": SCOPES_JUST_PATIENT_AND_A, "result_access_token_count": 3, "result_refresh_token_count": 3, "result_archived_token_count": 8, diff --git a/apps/dot_ext/tests/test_views.py b/apps/dot_ext/tests/test_views.py index 8543d2c1b..8eca87ae1 100644 --- a/apps/dot_ext/tests/test_views.py +++ b/apps/dot_ext/tests/test_views.py @@ -202,6 +202,7 @@ def testing_post_with_share_demographic_scopes(self): # Loop through test cases in dictionary cases = VIEW_OAUTH2_SCOPES_TEST_CASES for case in cases: + print(case) # Setup request parameters for test case request_bene_share_demographic_scopes = cases[case][ "request_bene_share_demographic_scopes" diff --git a/apps/dot_ext/views/authorization.py b/apps/dot_ext/views/authorization.py index d8a677e9a..93bf4068b 100644 --- a/apps/dot_ext/views/authorization.py +++ b/apps/dot_ext/views/authorization.py @@ -22,7 +22,7 @@ from oauth2_provider.models import get_application_model from oauthlib.oauth2.rfc6749.errors import InvalidClientError, InvalidGrantError from urllib.parse import urlparse, parse_qs - +import html from apps.dot_ext.scopes import CapabilitiesScopes import apps.logging.request_logger as bb2logging @@ -120,12 +120,12 @@ def sensitive_info_check(self, request): def get_template_names(self): flag = get_waffle_flag_model().get("limit_data_access") if waffle.switch_is_active('require-scopes'): - if flag.rollout or (flag.id is not None and flag.is_active_for_user(self.application.user)): + if flag.rollout or (flag.id is not None and self.application and flag.is_active_for_user(self.application.user)): return ["design_system/new_authorize_v2.html"] else: return ["design_system/authorize_v2.html"] else: - if flag.rollout or (flag.id is not None and flag.is_active_for_user(self.user)): + if flag.rollout or (flag.id is not None and self.user and flag.is_active_for_user(self.user)): return ["design_system/new_authorize_v2.html"] else: return ["design_system/authorize.html"] @@ -179,7 +179,7 @@ def form_valid(self, form): refresh_token_delete_cnt = 0 if not scopes: - return self.error_response("No scopes defined", application) + return self.error_response("No scopes", application) try: uri, headers, body, status = self.create_authorization_response( request=self.request, scopes=scopes, credentials=credentials, allow=allow @@ -356,6 +356,40 @@ def post(self, request, *args, **kwargs): return super().post(request, args, kwargs) +@method_decorator(csrf_exempt, name="dispatch") +class RevokeView(DotRevokeTokenView): + + @method_decorator(sensitive_post_parameters("password")) + def post(self, request, *args, **kwargs): + at_model = get_access_token_model() + try: + app = validate_app_is_active(request) + except (InvalidClientError, InvalidGrantError) as error: + return json_response_from_oauth2_error(error) + + tkn = request.POST.get('token') + if tkn is not None: + escaped_tkn = html.escape(tkn) + else: + escaped_tkn = "" + + try: + token = at_model.objects.get(token=tkn) + except at_model.DoesNotExist: + log.debug(f"Token {escaped_tkn} was not found.") + + try: + dag = DataAccessGrant.objects.get( + beneficiary=token.user, + application=app + ) + dag.delete() + except Exception: + log.debug(f"DAG lookup failed for token {escaped_tkn}.") + + return super().post(request, args, kwargs) + + @method_decorator(csrf_exempt, name="dispatch") class IntrospectTokenView(DotIntrospectTokenView): From b87ec145ab7f7314f0d96cf1bfd5802da6ba2097 Mon Sep 17 00:00:00 2001 From: Logan Bertram Date: Wed, 30 Oct 2024 12:17:21 -0400 Subject: [PATCH 05/17] Fix tests --- apps/dot_ext/tests/demographic_scopes_test_cases.py | 2 +- apps/dot_ext/tests/test_form_oauth2.py | 1 + apps/dot_ext/views/authorization.py | 12 ++++++------ 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/apps/dot_ext/tests/demographic_scopes_test_cases.py b/apps/dot_ext/tests/demographic_scopes_test_cases.py index 7dab48c0e..03dd25bf4 100644 --- a/apps/dot_ext/tests/demographic_scopes_test_cases.py +++ b/apps/dot_ext/tests/demographic_scopes_test_cases.py @@ -68,7 +68,7 @@ "request_scopes": APPLICATION_SCOPES_FULL, # Result: "result_form_is_valid": True, - "result_token_scopes_granted": APPLICATION_SCOPES_NON_DEMOGRAPHIC, + "result_token_scopes_granted": APPLICATION_SCOPES_FULL, }, "test 2: share_demographic_scopes = False": { # Request: diff --git a/apps/dot_ext/tests/test_form_oauth2.py b/apps/dot_ext/tests/test_form_oauth2.py index abd5c77f3..2e9b741b2 100644 --- a/apps/dot_ext/tests/test_form_oauth2.py +++ b/apps/dot_ext/tests/test_form_oauth2.py @@ -31,6 +31,7 @@ def test_form(self): # Loop through test cases in dictionary. cases = FORM_OAUTH2_SCOPES_TEST_CASES for case in cases: + print(case) # Setup request parameters for test case. request_bene_share_demographic_scopes = cases[case]["request_bene_share_demographic_scopes"] request_scopes = cases[case]["request_scopes"] diff --git a/apps/dot_ext/views/authorization.py b/apps/dot_ext/views/authorization.py index 93bf4068b..2a4944e09 100644 --- a/apps/dot_ext/views/authorization.py +++ b/apps/dot_ext/views/authorization.py @@ -178,6 +178,12 @@ def form_valid(self, form): access_token_delete_cnt = 0 refresh_token_delete_cnt = 0 + # Did the beneficiary choose not to share demographic scopes, or the application does not require them? + if share_demographic_scopes == "False" or (allow is True and application.require_demographic_scopes is False): + (data_access_grant_delete_cnt, + access_token_delete_cnt, + refresh_token_delete_cnt) = remove_application_user_pair_tokens_data_access(application, self.request.user) + if not scopes: return self.error_response("No scopes", application) try: @@ -207,12 +213,6 @@ def form_valid(self, form): data_access_grant_delete_cnt=data_access_grant_delete_cnt) return response - # Did the beneficiary choose not to share demographic scopes, or the application does not require them? - if share_demographic_scopes == "False" or (allow is True and application.require_demographic_scopes is False): - (data_access_grant_delete_cnt, - access_token_delete_cnt, - refresh_token_delete_cnt) = remove_application_user_pair_tokens_data_access(application, self.request.user) - beneficiary_authorized_application.send( sender=self, request=self.request, From a0ba1d3f09b5da1484bfa1b4f3268cad01731eca Mon Sep 17 00:00:00 2001 From: Logan Bertram Date: Wed, 30 Oct 2024 12:20:12 -0400 Subject: [PATCH 06/17] Shuffle allow checks --- apps/dot_ext/views/authorization.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/apps/dot_ext/views/authorization.py b/apps/dot_ext/views/authorization.py index 2a4944e09..5d7e4bd8a 100644 --- a/apps/dot_ext/views/authorization.py +++ b/apps/dot_ext/views/authorization.py @@ -178,11 +178,10 @@ def form_valid(self, form): access_token_delete_cnt = 0 refresh_token_delete_cnt = 0 - # Did the beneficiary choose not to share demographic scopes, or the application does not require them? - if share_demographic_scopes == "False" or (allow is True and application.require_demographic_scopes is False): + if allow is False: (data_access_grant_delete_cnt, - access_token_delete_cnt, - refresh_token_delete_cnt) = remove_application_user_pair_tokens_data_access(application, self.request.user) + access_token_delete_cnt, + refresh_token_delete_cnt) = remove_application_user_pair_tokens_data_access(application, self.request.user) if not scopes: return self.error_response("No scopes", application) @@ -193,11 +192,6 @@ def form_valid(self, form): except OAuthToolkitError as error: response = self.error_response(error, application) - if allow is False: - (data_access_grant_delete_cnt, - access_token_delete_cnt, - refresh_token_delete_cnt) = remove_application_user_pair_tokens_data_access(application, self.request.user) - beneficiary_authorized_application.send( sender=self, request=self.request, @@ -213,6 +207,12 @@ def form_valid(self, form): data_access_grant_delete_cnt=data_access_grant_delete_cnt) return response + # Did the beneficiary choose not to share demographic scopes, or the application does not require them? + if share_demographic_scopes == "False" or (allow is True and application.require_demographic_scopes is False): + (data_access_grant_delete_cnt, + access_token_delete_cnt, + refresh_token_delete_cnt) = remove_application_user_pair_tokens_data_access(application, self.request.user) + beneficiary_authorized_application.send( sender=self, request=self.request, From 065fc9354d72593297e4b1d56f1a62eafeba9395 Mon Sep 17 00:00:00 2001 From: Logan Bertram Date: Wed, 30 Oct 2024 12:28:28 -0400 Subject: [PATCH 07/17] status --- apps/dot_ext/views/authorization.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/apps/dot_ext/views/authorization.py b/apps/dot_ext/views/authorization.py index 5d7e4bd8a..96ded2bc3 100644 --- a/apps/dot_ext/views/authorization.py +++ b/apps/dot_ext/views/authorization.py @@ -6,7 +6,7 @@ import waffle from waffle import get_waffle_flag_model -from django.http.response import HttpResponse, HttpResponseBadRequest +from django.http.response import HttpResponse, HttpResponseBadRequest, JsonResponse from django.template.response import TemplateResponse from django.utils.decorators import method_decorator from django.views.decorators.csrf import csrf_exempt @@ -21,6 +21,7 @@ ) from oauth2_provider.models import get_application_model from oauthlib.oauth2.rfc6749.errors import InvalidClientError, InvalidGrantError +from rest_framework import status as http_status from urllib.parse import urlparse, parse_qs import html from apps.dot_ext.scopes import CapabilitiesScopes @@ -184,7 +185,20 @@ def form_valid(self, form): refresh_token_delete_cnt) = remove_application_user_pair_tokens_data_access(application, self.request.user) if not scopes: - return self.error_response("No scopes", application) + beneficiary_authorized_application.send( + sender=self, + request=self.request, + auth_status="FAIL", + auth_status_code=http_status.HTTP_400_BAD_REQUEST, + user=self.request.user, + application=application, + share_demographic_scopes=share_demographic_scopes, + scopes=scopes, + allow=allow, + access_token_delete_cnt=access_token_delete_cnt, + refresh_token_delete_cnt=refresh_token_delete_cnt, + data_access_grant_delete_cnt=data_access_grant_delete_cnt) + return JsonResponse({"error": 'The state parameter is required'}, status=http_status.HTTP_400_BAD_REQUEST) try: uri, headers, body, status = self.create_authorization_response( request=self.request, scopes=scopes, credentials=credentials, allow=allow From e87381d1873458dd36c0c24f280bb95e6b3438d0 Mon Sep 17 00:00:00 2001 From: Logan Bertram Date: Wed, 30 Oct 2024 12:32:20 -0400 Subject: [PATCH 08/17] error msg update --- apps/dot_ext/views/authorization.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/dot_ext/views/authorization.py b/apps/dot_ext/views/authorization.py index 96ded2bc3..c975b9c4e 100644 --- a/apps/dot_ext/views/authorization.py +++ b/apps/dot_ext/views/authorization.py @@ -198,7 +198,7 @@ def form_valid(self, form): access_token_delete_cnt=access_token_delete_cnt, refresh_token_delete_cnt=refresh_token_delete_cnt, data_access_grant_delete_cnt=data_access_grant_delete_cnt) - return JsonResponse({"error": 'The state parameter is required'}, status=http_status.HTTP_400_BAD_REQUEST) + return JsonResponse({"error": 'Requested scopes denied'}, status=http_status.HTTP_400_BAD_REQUEST) try: uri, headers, body, status = self.create_authorization_response( request=self.request, scopes=scopes, credentials=credentials, allow=allow From 55c70da0984b42264fe145904c62a98fcc8b0ebb Mon Sep 17 00:00:00 2001 From: Logan Bertram Date: Wed, 30 Oct 2024 12:39:21 -0400 Subject: [PATCH 09/17] Error respoonse --- apps/dot_ext/views/authorization.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/apps/dot_ext/views/authorization.py b/apps/dot_ext/views/authorization.py index c975b9c4e..a1c282161 100644 --- a/apps/dot_ext/views/authorization.py +++ b/apps/dot_ext/views/authorization.py @@ -6,7 +6,7 @@ import waffle from waffle import get_waffle_flag_model -from django.http.response import HttpResponse, HttpResponseBadRequest, JsonResponse +from django.http.response import HttpResponse, HttpResponseBadRequest from django.template.response import TemplateResponse from django.utils.decorators import method_decorator from django.views.decorators.csrf import csrf_exempt @@ -21,7 +21,6 @@ ) from oauth2_provider.models import get_application_model from oauthlib.oauth2.rfc6749.errors import InvalidClientError, InvalidGrantError -from rest_framework import status as http_status from urllib.parse import urlparse, parse_qs import html from apps.dot_ext.scopes import CapabilitiesScopes @@ -189,7 +188,7 @@ def form_valid(self, form): sender=self, request=self.request, auth_status="FAIL", - auth_status_code=http_status.HTTP_400_BAD_REQUEST, + auth_status_code=302, user=self.request.user, application=application, share_demographic_scopes=share_demographic_scopes, @@ -198,7 +197,7 @@ def form_valid(self, form): access_token_delete_cnt=access_token_delete_cnt, refresh_token_delete_cnt=refresh_token_delete_cnt, data_access_grant_delete_cnt=data_access_grant_delete_cnt) - return JsonResponse({"error": 'Requested scopes denied'}, status=http_status.HTTP_400_BAD_REQUEST) + return self.error_response('Requested scopes denied', application) try: uri, headers, body, status = self.create_authorization_response( request=self.request, scopes=scopes, credentials=credentials, allow=allow From 6edc849a3808478ceccd65f6f2225c3f1d663b1a Mon Sep 17 00:00:00 2001 From: Logan Bertram Date: Wed, 30 Oct 2024 12:45:21 -0400 Subject: [PATCH 10/17] Errors sorted --- apps/dot_ext/views/authorization.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apps/dot_ext/views/authorization.py b/apps/dot_ext/views/authorization.py index a1c282161..151ffb1d6 100644 --- a/apps/dot_ext/views/authorization.py +++ b/apps/dot_ext/views/authorization.py @@ -20,6 +20,7 @@ IntrospectTokenView as DotIntrospectTokenView, ) from oauth2_provider.models import get_application_model +from oauthlib.oauth2 import AccessDeniedError from oauthlib.oauth2.rfc6749.errors import InvalidClientError, InvalidGrantError from urllib.parse import urlparse, parse_qs import html @@ -197,7 +198,7 @@ def form_valid(self, form): access_token_delete_cnt=access_token_delete_cnt, refresh_token_delete_cnt=refresh_token_delete_cnt, data_access_grant_delete_cnt=data_access_grant_delete_cnt) - return self.error_response('Requested scopes denied', application) + raise AccessDeniedError(state=credentials.get("state", None)) try: uri, headers, body, status = self.create_authorization_response( request=self.request, scopes=scopes, credentials=credentials, allow=allow From b16f5ac79d6379f8f579a35c6d831a0ea45a2d66 Mon Sep 17 00:00:00 2001 From: Logan Bertram Date: Wed, 30 Oct 2024 12:50:20 -0400 Subject: [PATCH 11/17] less redundant --- apps/dot_ext/views/authorization.py | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-) diff --git a/apps/dot_ext/views/authorization.py b/apps/dot_ext/views/authorization.py index 151ffb1d6..10f9e6f87 100644 --- a/apps/dot_ext/views/authorization.py +++ b/apps/dot_ext/views/authorization.py @@ -20,7 +20,7 @@ IntrospectTokenView as DotIntrospectTokenView, ) from oauth2_provider.models import get_application_model -from oauthlib.oauth2 import AccessDeniedError +from oauthlib import oauth2 from oauthlib.oauth2.rfc6749.errors import InvalidClientError, InvalidGrantError from urllib.parse import urlparse, parse_qs import html @@ -179,33 +179,20 @@ def form_valid(self, form): access_token_delete_cnt = 0 refresh_token_delete_cnt = 0 - if allow is False: - (data_access_grant_delete_cnt, - access_token_delete_cnt, - refresh_token_delete_cnt) = remove_application_user_pair_tokens_data_access(application, self.request.user) - - if not scopes: - beneficiary_authorized_application.send( - sender=self, - request=self.request, - auth_status="FAIL", - auth_status_code=302, - user=self.request.user, - application=application, - share_demographic_scopes=share_demographic_scopes, - scopes=scopes, - allow=allow, - access_token_delete_cnt=access_token_delete_cnt, - refresh_token_delete_cnt=refresh_token_delete_cnt, - data_access_grant_delete_cnt=data_access_grant_delete_cnt) - raise AccessDeniedError(state=credentials.get("state", None)) try: uri, headers, body, status = self.create_authorization_response( request=self.request, scopes=scopes, credentials=credentials, allow=allow ) + if not scopes: + raise oauth2.AccessDeniedError(state=credentials.get("state", None)) except OAuthToolkitError as error: response = self.error_response(error, application) + if allow is False: + (data_access_grant_delete_cnt, + access_token_delete_cnt, + refresh_token_delete_cnt) = remove_application_user_pair_tokens_data_access(application, self.request.user) + beneficiary_authorized_application.send( sender=self, request=self.request, From dfee123c8c0a3121e405562d6e54f63681fb14ec Mon Sep 17 00:00:00 2001 From: Logan Bertram Date: Wed, 30 Oct 2024 13:38:40 -0400 Subject: [PATCH 12/17] examine scope erasure in postman --- apps/dot_ext/views/authorization.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/dot_ext/views/authorization.py b/apps/dot_ext/views/authorization.py index 10f9e6f87..df9920ab1 100644 --- a/apps/dot_ext/views/authorization.py +++ b/apps/dot_ext/views/authorization.py @@ -180,15 +180,15 @@ def form_valid(self, form): refresh_token_delete_cnt = 0 try: + if not scopes: + raise oauth2.AccessDeniedError(state=credentials.get("state", None)) uri, headers, body, status = self.create_authorization_response( request=self.request, scopes=scopes, credentials=credentials, allow=allow ) - if not scopes: - raise oauth2.AccessDeniedError(state=credentials.get("state", None)) except OAuthToolkitError as error: response = self.error_response(error, application) - if allow is False: + if allow is False or not scopes: (data_access_grant_delete_cnt, access_token_delete_cnt, refresh_token_delete_cnt) = remove_application_user_pair_tokens_data_access(application, self.request.user) From 2c75e47f897191de72be9b1f9fbd264d1403c495 Mon Sep 17 00:00:00 2001 From: Logan Bertram Date: Wed, 30 Oct 2024 13:46:30 -0400 Subject: [PATCH 13/17] Revert short-term fix --- apps/capabilities/permissions.py | 23 ++++++++++++++--------- apps/capabilities/tests.py | 2 ++ apps/dot_ext/tests/test_form_oauth2.py | 1 - apps/dot_ext/tests/test_views.py | 4 ++++ 4 files changed, 20 insertions(+), 10 deletions(-) diff --git a/apps/capabilities/permissions.py b/apps/capabilities/permissions.py index ece09f048..a7bc3b726 100644 --- a/apps/capabilities/permissions.py +++ b/apps/capabilities/permissions.py @@ -37,15 +37,20 @@ def has_permission(self, request, view): slug__in=token_scopes ).values_list('protected_resources', flat=True).all()) - for scope in scopes: - for method, path in json.loads(scope): - if method != request.method: - continue - if path == request.path: - return True - if re.fullmatch(path, request.path) is not None: - return True - return False + # this is a shorterm fix to reject all tokens that do not have either + # patient/coverage.read or patient/ExplanationOfBenefit.read + if ("patient/Coverage.read" in token_scopes) or ("patient/ExplanationOfBenefit.read" in token_scopes): + for scope in scopes: + for method, path in json.loads(scope): + if method != request.method: + continue + if path == request.path: + return True + if re.fullmatch(path, request.path) is not None: + return True + return False + else: + return False else: # BB2-237: Replaces ASSERT with exception. We should never reach here. mesg = ("TokenHasScope requires the `oauth2_provider.rest_framework.OAuth2Authentication`" diff --git a/apps/capabilities/tests.py b/apps/capabilities/tests.py index 396faf9d2..b320398f5 100644 --- a/apps/capabilities/tests.py +++ b/apps/capabilities/tests.py @@ -1,4 +1,5 @@ import json +import unittest from django.contrib.auth.models import Group from django.test import TestCase @@ -40,6 +41,7 @@ def setUp(self): protected_resources=json.dumps([["POST", "/path"]]), ) + @unittest.skip("Broke with quick fix") def test_request_is_protected(self): request = SimpleRequest("scope") request.method = "GET" diff --git a/apps/dot_ext/tests/test_form_oauth2.py b/apps/dot_ext/tests/test_form_oauth2.py index 2e9b741b2..abd5c77f3 100644 --- a/apps/dot_ext/tests/test_form_oauth2.py +++ b/apps/dot_ext/tests/test_form_oauth2.py @@ -31,7 +31,6 @@ def test_form(self): # Loop through test cases in dictionary. cases = FORM_OAUTH2_SCOPES_TEST_CASES for case in cases: - print(case) # Setup request parameters for test case. request_bene_share_demographic_scopes = cases[case]["request_bene_share_demographic_scopes"] request_scopes = cases[case]["request_scopes"] diff --git a/apps/dot_ext/tests/test_views.py b/apps/dot_ext/tests/test_views.py index 8eca87ae1..5b08ea957 100644 --- a/apps/dot_ext/tests/test_views.py +++ b/apps/dot_ext/tests/test_views.py @@ -1,6 +1,7 @@ import json import base64 from datetime import date, timedelta +import unittest from django.conf import settings from django.http import HttpRequest @@ -162,15 +163,18 @@ def test_post_with_restricted_scopes_issues_token_with_same_scopes(self): # and here we test that only the capability-a scope has been issued self.assertEqual(content["scope"], "capability-a") + @unittest.skip("Broke with quick fix") def test_post_with_share_demographic_scopes(self): # Test with-out new_auth switch self.testing_post_with_share_demographic_scopes() + @unittest.skip("Broke with quick fix") @override_switch("new_auth", active=True) def test_post_with_share_demographic_scopes_new_auth_switch(self): # Test with new_auth switch. self.testing_post_with_share_demographic_scopes() + @unittest.skip("Broke with quick fix") @override_switch("require-scopes", active=True) def testing_post_with_share_demographic_scopes(self): """ From cac5f0d17a1ed127f530dd2fcd6d26fe09ead8e8 Mon Sep 17 00:00:00 2001 From: Logan Bertram Date: Wed, 30 Oct 2024 14:20:26 -0400 Subject: [PATCH 14/17] Kill print --- apps/dot_ext/tests/test_views.py | 1 - 1 file changed, 1 deletion(-) diff --git a/apps/dot_ext/tests/test_views.py b/apps/dot_ext/tests/test_views.py index 5b08ea957..589fa2c18 100644 --- a/apps/dot_ext/tests/test_views.py +++ b/apps/dot_ext/tests/test_views.py @@ -206,7 +206,6 @@ def testing_post_with_share_demographic_scopes(self): # Loop through test cases in dictionary cases = VIEW_OAUTH2_SCOPES_TEST_CASES for case in cases: - print(case) # Setup request parameters for test case request_bene_share_demographic_scopes = cases[case][ "request_bene_share_demographic_scopes" From 48794d3bb989f85990a4695e313e2d90b972a3cd Mon Sep 17 00:00:00 2001 From: Logan Bertram Date: Wed, 30 Oct 2024 14:30:41 -0400 Subject: [PATCH 15/17] Tweaked error handling --- apps/dot_ext/tests/test_views.py | 2 +- apps/dot_ext/views/authorization.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/apps/dot_ext/tests/test_views.py b/apps/dot_ext/tests/test_views.py index 589fa2c18..4018606bd 100644 --- a/apps/dot_ext/tests/test_views.py +++ b/apps/dot_ext/tests/test_views.py @@ -1,7 +1,7 @@ import json import base64 -from datetime import date, timedelta import unittest +from datetime import date, timedelta from django.conf import settings from django.http import HttpRequest diff --git a/apps/dot_ext/views/authorization.py b/apps/dot_ext/views/authorization.py index df9920ab1..b83d2a5e6 100644 --- a/apps/dot_ext/views/authorization.py +++ b/apps/dot_ext/views/authorization.py @@ -185,13 +185,13 @@ def form_valid(self, form): uri, headers, body, status = self.create_authorization_response( request=self.request, scopes=scopes, credentials=credentials, allow=allow ) - except OAuthToolkitError as error: + except (oauth2.AccessDeniedError, OAuthToolkitError) as error: response = self.error_response(error, application) if allow is False or not scopes: (data_access_grant_delete_cnt, - access_token_delete_cnt, - refresh_token_delete_cnt) = remove_application_user_pair_tokens_data_access(application, self.request.user) + access_token_delete_cnt, + refresh_token_delete_cnt) = remove_application_user_pair_tokens_data_access(application, self.request.user) beneficiary_authorized_application.send( sender=self, From 0b1c42f972471a44ebe268effa6caf8f4befd6c1 Mon Sep 17 00:00:00 2001 From: Logan Bertram Date: Wed, 30 Oct 2024 14:33:36 -0400 Subject: [PATCH 16/17] Tweaked error handling --- apps/dot_ext/views/authorization.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/apps/dot_ext/views/authorization.py b/apps/dot_ext/views/authorization.py index b83d2a5e6..b25886ab2 100644 --- a/apps/dot_ext/views/authorization.py +++ b/apps/dot_ext/views/authorization.py @@ -181,11 +181,15 @@ def form_valid(self, form): try: if not scopes: - raise oauth2.AccessDeniedError(state=credentials.get("state", None)) + # Since the create_authorization_response will re-inject scopes even when none are + # valid, we want to pre-emptively treat this as an error case + raise OAuthToolkitError( + error=oauth2.AccessDeniedError(state=credentials.get("state", None)), redirect_uri=credentials["redirect_uri"] + ) uri, headers, body, status = self.create_authorization_response( request=self.request, scopes=scopes, credentials=credentials, allow=allow ) - except (oauth2.AccessDeniedError, OAuthToolkitError) as error: + except OAuthToolkitError as error: response = self.error_response(error, application) if allow is False or not scopes: From b2dad27a69fd7c9bcd3b04c64bdda1651e72e3c2 Mon Sep 17 00:00:00 2001 From: Logan Bertram Date: Wed, 30 Oct 2024 14:40:53 -0400 Subject: [PATCH 17/17] Cleanup --- apps/dot_ext/views/authorization.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/dot_ext/views/authorization.py b/apps/dot_ext/views/authorization.py index b25886ab2..dd63ba8a4 100644 --- a/apps/dot_ext/views/authorization.py +++ b/apps/dot_ext/views/authorization.py @@ -20,7 +20,7 @@ IntrospectTokenView as DotIntrospectTokenView, ) from oauth2_provider.models import get_application_model -from oauthlib import oauth2 +from oauthlib.oauth2 import AccessDeniedError from oauthlib.oauth2.rfc6749.errors import InvalidClientError, InvalidGrantError from urllib.parse import urlparse, parse_qs import html @@ -184,7 +184,7 @@ def form_valid(self, form): # Since the create_authorization_response will re-inject scopes even when none are # valid, we want to pre-emptively treat this as an error case raise OAuthToolkitError( - error=oauth2.AccessDeniedError(state=credentials.get("state", None)), redirect_uri=credentials["redirect_uri"] + error=AccessDeniedError(state=credentials.get("state", None)), redirect_uri=credentials["redirect_uri"] ) uri, headers, body, status = self.create_authorization_response( request=self.request, scopes=scopes, credentials=credentials, allow=allow