From 2624a78217d48fbc877b29b2178c4f4062a9cf03 Mon Sep 17 00:00:00 2001 From: Ernesto Cruz Date: Tue, 2 Jun 2020 10:11:35 -0500 Subject: [PATCH 1/3] fix: Check if 'lis_outcome_service_url' and 'lis_result_sourcedid' fields exist before register them in control file --- src/illumidesk/authenticators/authenticator.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/illumidesk/authenticators/authenticator.py b/src/illumidesk/authenticators/authenticator.py index 1527a902..828a1dcd 100644 --- a/src/illumidesk/authenticators/authenticator.py +++ b/src/illumidesk/authenticators/authenticator.py @@ -230,7 +230,9 @@ async def authenticate(self, handler: BaseHandler, data: Dict[str, str] = None) lis_outcome_service_url = args['lis_outcome_service_url'] if 'lis_result_sourcedid' in args and args['lis_result_sourcedid'] is not None: lis_result_sourcedid = args['lis_result_sourcedid'] - control_file.register_data(assignment_name, lis_outcome_service_url, lms_user_id, lis_result_sourcedid) + # only if both values exist we can register them to submit grades later + if lis_outcome_service_url and lis_result_sourcedid: + control_file.register_data(assignment_name, lis_outcome_service_url, lms_user_id, lis_result_sourcedid) return { 'name': username, From 635f2bf4f6485dd794411d4dd82bd192cec76fec Mon Sep 17 00:00:00 2001 From: Ernesto Cruz Date: Tue, 2 Jun 2020 12:05:15 -0500 Subject: [PATCH 2/3] Fix unit tests related to outcome_service_url --- .../authenticators/authenticator.py | 6 ++-- .../test_lti11_authenticator.py | 28 ++++++------------- src/tests/illumidesk/factory.py | 4 +-- 3 files changed, 14 insertions(+), 24 deletions(-) diff --git a/src/illumidesk/authenticators/authenticator.py b/src/illumidesk/authenticators/authenticator.py index 828a1dcd..b5760af1 100644 --- a/src/illumidesk/authenticators/authenticator.py +++ b/src/illumidesk/authenticators/authenticator.py @@ -223,8 +223,9 @@ async def authenticate(self, handler: BaseHandler, data: Dict[str, str] = None) # with all info extracted from lms request, register info for grades sender only if the user has # the Learner role - if user_role == 'Learner': - control_file = LTIGradesSenderControlFile(f'/home/grader-{course_id}/{course_id}') + lis_outcome_service_url = None + lis_result_sourcedid = None + if user_role == 'Learner': # the next fields must come in args if 'lis_outcome_service_url' in args and args['lis_outcome_service_url'] is not None: lis_outcome_service_url = args['lis_outcome_service_url'] @@ -232,6 +233,7 @@ async def authenticate(self, handler: BaseHandler, data: Dict[str, str] = None) lis_result_sourcedid = args['lis_result_sourcedid'] # only if both values exist we can register them to submit grades later if lis_outcome_service_url and lis_result_sourcedid: + control_file = LTIGradesSenderControlFile(f'/home/grader-{course_id}/{course_id}') control_file.register_data(assignment_name, lis_outcome_service_url, lms_user_id, lis_result_sourcedid) return { diff --git a/src/tests/illumidesk/authenticators/test_lti11_authenticator.py b/src/tests/illumidesk/authenticators/test_lti11_authenticator.py index b3db4d6d..32843777 100644 --- a/src/tests/illumidesk/authenticators/test_lti11_authenticator.py +++ b/src/tests/illumidesk/authenticators/test_lti11_authenticator.py @@ -116,12 +116,9 @@ async def test_authenticator_returns_auth_state_with_missing_lis_outcome_service """ Are we able to handle requests with a missing lis_outcome_service_url key? """ - utils = LTIUtils() - utils.convert_request_to_dict = MagicMock(name='convert_request_to_dict') - utils.convert_request_to_dict(3, 4, 5, key='value') with patch.object(LTI11LaunchValidator, 'validate_launch_request', return_value=True): authenticator = LTI11Authenticator() - args = factory_lti11_complete_launch_args('canvas') + args = factory_lti11_complete_launch_args('canvas', 'Learner') del args['lis_outcome_service_url'] handler = Mock( spec=RequestHandler, @@ -134,7 +131,7 @@ async def test_authenticator_returns_auth_state_with_missing_lis_outcome_service 'auth_state': { 'course_id': 'intro101', 'lms_user_id': '185d6c59731a553009ca9b59ca3a885100000', - 'user_role': 'Instructor', + 'user_role': 'Learner', }, } assert result == expected @@ -146,12 +143,9 @@ async def test_authenticator_returns_auth_state_with_missing_lis_result_sourcedi """ Are we able to handle requests with a missing lis_result_sourcedid key? """ - utils = LTIUtils() - utils.convert_request_to_dict = MagicMock(name='convert_request_to_dict') - utils.convert_request_to_dict(3, 4, 5, key='value') with patch.object(LTI11LaunchValidator, 'validate_launch_request', return_value=True): authenticator = LTI11Authenticator() - args = factory_lti11_complete_launch_args('canvas') + args = factory_lti11_complete_launch_args('canvas', 'Learner') del args['lis_result_sourcedid'] handler = Mock( spec=RequestHandler, @@ -164,7 +158,7 @@ async def test_authenticator_returns_auth_state_with_missing_lis_result_sourcedi 'auth_state': { 'course_id': 'intro101', 'lms_user_id': '185d6c59731a553009ca9b59ca3a885100000', - 'user_role': 'Instructor', + 'user_role': 'Learner', }, } assert result == expected @@ -176,12 +170,9 @@ async def test_authenticator_returns_auth_state_with_empty_lis_result_sourcedid( """ Are we able to handle requests with lis_result_sourcedid set to an empty value? """ - utils = LTIUtils() - utils.convert_request_to_dict = MagicMock(name='convert_request_to_dict') - utils.convert_request_to_dict(3, 4, 5, key='value') with patch.object(LTI11LaunchValidator, 'validate_launch_request', return_value=True): authenticator = LTI11Authenticator() - args = factory_lti11_complete_launch_args('canvas') + args = factory_lti11_complete_launch_args('canvas', 'Learner') args['lis_result_sourcedid'] = '' handler = Mock( spec=RequestHandler, @@ -194,7 +185,7 @@ async def test_authenticator_returns_auth_state_with_empty_lis_result_sourcedid( 'auth_state': { 'course_id': 'intro101', 'lms_user_id': '185d6c59731a553009ca9b59ca3a885100000', - 'user_role': 'Instructor', + 'user_role': 'Learner', }, } assert result == expected @@ -206,12 +197,9 @@ async def test_authenticator_returns_auth_state_with_empty_lis_outcome_service_u """ Are we able to handle requests with lis_outcome_service_url set to an empty value? """ - utils = LTIUtils() - utils.convert_request_to_dict = MagicMock(name='convert_request_to_dict') - utils.convert_request_to_dict(3, 4, 5, key='value') with patch.object(LTI11LaunchValidator, 'validate_launch_request', return_value=True): authenticator = LTI11Authenticator() - args = factory_lti11_complete_launch_args('canvas') + args = factory_lti11_complete_launch_args('canvas', 'Learner') args['lis_outcome_service_url'] = '' handler = Mock( spec=RequestHandler, @@ -224,7 +212,7 @@ async def test_authenticator_returns_auth_state_with_empty_lis_outcome_service_u 'auth_state': { 'course_id': 'intro101', 'lms_user_id': '185d6c59731a553009ca9b59ca3a885100000', - 'user_role': 'Instructor', + 'user_role': 'Learner', }, } assert result == expected diff --git a/src/tests/illumidesk/factory.py b/src/tests/illumidesk/factory.py index f74bf55a..aac081e3 100644 --- a/src/tests/illumidesk/factory.py +++ b/src/tests/illumidesk/factory.py @@ -45,7 +45,7 @@ def factory_lti11_basic_launch_args(oauth_consumer_key: str, oauth_consumer_secr return args -def factory_lti11_complete_launch_args(lms_vendor: str) -> Dict[str, str]: +def factory_lti11_complete_launch_args(lms_vendor: str, role: str='Instructor') -> Dict[str, str]: """ Valid response when retrieving jwks from the platform. """ @@ -81,7 +81,7 @@ def factory_lti11_complete_launch_args(lms_vendor: str) -> Dict[str, str]: 'oauth_callback': ['about:blank'.encode()], 'resource_link_id': ['888efe72d4bbbdf90619353bb8ab5965ccbe9b3f'.encode()], 'resource_link_title': ['IllumiDesk'.encode()], - 'roles': ['Instructor'.encode()], + 'roles': ['Instructor'.encode() if role == 'Instructor' else role.encode()], 'tool_consumer_info_product_family_code': [lms_vendor.encode()], 'tool_consumer_info_version': ['cloud'.encode()], 'tool_consumer_instance_contact_email': ['notifications@mylms.com'.encode()], From 533655063745464a52ba87e4739b6c600777055c Mon Sep 17 00:00:00 2001 From: Ernesto Cruz Date: Tue, 2 Jun 2020 12:08:00 -0500 Subject: [PATCH 3/3] minor change to remove unused mock --- .../illumidesk/authenticators/test_lti11_authenticator.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/tests/illumidesk/authenticators/test_lti11_authenticator.py b/src/tests/illumidesk/authenticators/test_lti11_authenticator.py index 32843777..14869704 100644 --- a/src/tests/illumidesk/authenticators/test_lti11_authenticator.py +++ b/src/tests/illumidesk/authenticators/test_lti11_authenticator.py @@ -45,9 +45,6 @@ async def test_authenticator_returns_auth_state_with_other_lms_vendor(lti11_auth """ Do we get a valid username with lms vendors other than canvas? """ - utils = LTIUtils() - utils.convert_request_to_dict = MagicMock(name='convert_request_to_dict') - utils.convert_request_to_dict(3, 4, 5, key='value') with patch.object(LTI11LaunchValidator, 'validate_launch_request', return_value=True): authenticator = LTI11Authenticator() handler = Mock(