From 371a3ef1c647d8aa8e315f4a5a6e4a56838f291f Mon Sep 17 00:00:00 2001 From: Awais Jibran Date: Tue, 21 Dec 2021 15:07:49 +0500 Subject: [PATCH] fix: add pii sharing allowed flag (#29628) --- .../djangoapps/discussions/serializers.py | 19 +- .../discussions/tests/test_views.py | 388 ++++++++++-------- 2 files changed, 231 insertions(+), 176 deletions(-) diff --git a/openedx/core/djangoapps/discussions/serializers.py b/openedx/core/djangoapps/discussions/serializers.py index 0849e5463a..20db67db3b 100644 --- a/openedx/core/djangoapps/discussions/serializers.py +++ b/openedx/core/djangoapps/discussions/serializers.py @@ -43,16 +43,6 @@ class LtiSerializer(serializers.ModelSerializer): } return payload - def to_representation(self, instance): - """ - Serialize object into a primitive data types. - """ - representation = super().to_representation(instance) - if not self.context.get('pii_sharing_allowed'): - representation.pop('pii_share_username') - representation.pop('pii_share_email') - return representation - def update(self, instance: LtiConfiguration, validated_data: dict) -> LtiConfiguration: """ Create/update a model-backed instance @@ -219,12 +209,13 @@ class DiscussionsConfigurationSerializer(serializers.ModelSerializer): """ course_key = instance.context_key payload = super().to_representation(instance) + course_pii_sharing_allowed = get_lti_pii_sharing_state_for_course(course_key) - lti_configuration = LtiSerializer( - instance.lti_configuration, - context={'pii_sharing_allowed': get_lti_pii_sharing_state_for_course(course_key)} - ) + lti_configuration = LtiSerializer(instance=instance.lti_configuration) lti_configuration_data = lti_configuration.data + lti_configuration_data.update({ + 'pii_sharing_allowed': course_pii_sharing_allowed + }) provider_type = instance.provider_type plugin_configuration = instance.plugin_configuration diff --git a/openedx/core/djangoapps/discussions/tests/test_views.py b/openedx/core/djangoapps/discussions/tests/test_views.py index d2e62fae16..848c254fe4 100644 --- a/openedx/core/djangoapps/discussions/tests/test_views.py +++ b/openedx/core/djangoapps/discussions/tests/test_views.py @@ -46,22 +46,35 @@ DEFAULT_LEGACY_CONFIGURATION = { 'available_division_schemes': [], } - DEFAULT_LTI_CONFIGURATION = { 'lti_1p1_client_key': '', 'lti_1p1_client_secret': '', 'lti_1p1_launch_url': '', 'version': None, + 'pii_sharing_allowed': False, 'pii_share_email': False, 'pii_share_username': False, } -DATA_LTI_CONFIGURATION = { +DATA_POST_LTI_CONFIGURATION = { 'lti_1p1_client_key': 'KEY', 'lti_1p1_client_secret': 'SECRET', 'lti_1p1_launch_url': 'https://localhost', 'version': 'lti_1p1' } +DATA_LTI_CONFIGURATION_DISABLED_PII = { + **DATA_POST_LTI_CONFIGURATION, + 'pii_sharing_allowed': False, + 'pii_share_email': False, + 'pii_share_username': False, +} + +DATA_LTI_CONFIGURATION_ENABLED_PII = { + **DATA_POST_LTI_CONFIGURATION, + 'pii_sharing_allowed': True, + 'pii_share_email': False, + 'pii_share_username': False, +} class ApiTest(ModuleStoreTestCase, APITestCase): @@ -82,10 +95,7 @@ class ApiTest(ModuleStoreTestCase, APITestCase): def url(self): """Returns the discussion API url. """ return reverse( - 'discussions', - kwargs={ - 'course_key_string': str(self.course.id), - } + 'discussions', kwargs={'course_key_string': str(self.course.id)} ) def _get(self): @@ -226,8 +236,10 @@ class DataTest(AuthorizedApiTest): """ def get(self): - """Makes a get request and returns json data""" - response = super()._get() + """ + Makes a get request and returns json data. + """ + response = self._get() return response.json() def get_and_assert_defaults(self): @@ -261,7 +273,7 @@ class DataTest(AuthorizedApiTest): payload = { 'enabled': True, 'provider_type': provider, - 'lti_configuration': DATA_LTI_CONFIGURATION, + 'lti_configuration': DATA_POST_LTI_CONFIGURATION, 'plugin_configuration': {} } response = self._post(payload) @@ -269,11 +281,19 @@ class DataTest(AuthorizedApiTest): assert response.status_code == self.expected_response_code return data - @contextmanager - def _pii_sharing_for_course(self, enabled): - instance = CourseAllowPIISharingInLTIFlag.objects.create(course_id=self.course.id, enabled=enabled) - yield - instance.delete() + def _configure_legacy_discussion_provider(self, configuration): + """ + Configure legacy discussion provider for a course. + """ + payload = { + 'enabled': True, + 'provider_type': Provider.LEGACY, + 'plugin_configuration': configuration, + } + response = self._post(payload) + assert response + assert response.status_code == self.expected_response_code + return response.json() def test_get_non_configured_provider_for_course(self): """ @@ -282,100 +302,6 @@ class DataTest(AuthorizedApiTest): """ self.get_and_assert_defaults() - @ddt.data( - {"pii_share_username": True}, - {"pii_share_email": True}, - {"pii_share_email": True, "pii_share_username": True}, - ) - def test_post_pii_fields_with_non_configured_pii(self, pii_fields): - """ - Tests that if PII sharing is not set, user is not able to update - PII settings for a course. - """ - data = self._configure_lti_discussion_provider() - data['lti_configuration'].update(pii_fields) - response = self._post(data) - assert response.status_code == status.HTTP_400_BAD_REQUEST - - @ddt.data( - {"pii_share_username": True}, - {"pii_share_email": True}, - {"pii_share_email": True, "pii_share_username": True}, - ) - def test_post_pii_fields_with_pii_disabled(self, pii_fields): - """ - Test that when PII sharing is disabled for the course, user is not able - update PII settings for a course. - """ - data = self._configure_lti_discussion_provider() - data['lti_configuration'].update(pii_fields) - with self._pii_sharing_for_course(enabled=False): - response = self._post(data) - assert response.status_code == status.HTTP_400_BAD_REQUEST - lti_configuration = self.get()['lti_configuration'] - no_pii_fields_updated = [ - lti_configuration.get(pii_field) != pii_value for pii_field, pii_value in pii_fields.items() - ] - assert all(no_pii_fields_updated) - - @ddt.data( - {"pii_share_username": True}, - {"pii_share_email": True}, - {"pii_share_email": True, "pii_share_username": True}, - ) - def test_post_pii_fields_with_pii_enabled(self, pii_fields): - """ - Test that when PII sharing is enabled for the course, user is able - update PII settings for the course. - """ - data = self._configure_lti_discussion_provider() - data['lti_configuration'].update(pii_fields) - with self._pii_sharing_for_course(enabled=True): - response = self._post(data) - assert response.status_code == status.HTTP_200_OK - - lti_configuration = self.get()['lti_configuration'] - all_pii_fields_updated = [ - lti_configuration[pii_field] == pii_value for pii_field, pii_value in pii_fields.items() - ] - assert all(all_pii_fields_updated) - - @ddt.data( - True, - False - ) - def test_get_pii_fields(self, pii_sharing): - """ - Tests that when PII sharing is enabled, API included PII info. - """ - self._configure_lti_discussion_provider() - with self._pii_sharing_for_course(enabled=pii_sharing): - data = self.get() - # If pii_sharing is true, then the fields should be present, and absent otherwise - assert ('pii_share_email' in data['lti_configuration']) == pii_sharing - assert ('pii_share_username' in data['lti_configuration']) == pii_sharing - - @ddt.data( - Provider.ED_DISCUSS, - Provider.INSCRIBE, - Provider.PIAZZA, - Provider.YELLOWDIG, - ) - def test_post_everything(self, provider): - """ - API should accept requests to update _all_ fields at once - """ - data = self._configure_lti_discussion_provider(provider=provider) - assert data['enabled'] - assert data['provider_type'] == provider - assert data['providers']['available'][provider] == AVAILABLE_PROVIDER_MAP[provider] - assert data['plugin_configuration'] == DEFAULT_LEGACY_CONFIGURATION - assert data['lti_configuration'] == DATA_LTI_CONFIGURATION - - response_data = self.get() - # the GET should pull back the same data as the POST - assert response_data == data - def test_post_invalid_key(self): """ Tests that unsupported keys should be gracefully ignored. @@ -391,6 +317,7 @@ class DataTest(AuthorizedApiTest): Provider.INSCRIBE, Provider.PIAZZA, Provider.YELLOWDIG, + Provider.LEGACY ) def test_add_valid_configuration(self, provider_type): """ @@ -409,6 +336,7 @@ class DataTest(AuthorizedApiTest): assert data['enabled'] assert data['provider_type'] == provider_type assert data['plugin_configuration'] == DEFAULT_LEGACY_CONFIGURATION + assert data['lti_configuration'] == DEFAULT_LTI_CONFIGURATION def test_change_plugin_configuration(self): """ @@ -446,25 +374,25 @@ class DataTest(AuthorizedApiTest): self.get_and_assert_defaults() @ddt.data( - *DATA_LTI_CONFIGURATION.items() + *DATA_POST_LTI_CONFIGURATION.items() ) @ddt.unpack def test_post_lti_valid(self, key, value): """ Test that we can set & retrieve LTI configuration. """ - provider_type = 'piazza' payload = { 'enabled': True, - 'provider_type': provider_type, + 'provider_type': Provider.PIAZZA, 'lti_configuration': { key: value, } } self._post(payload) + data = self.get() assert data['enabled'] - assert data['provider_type'] == provider_type + assert data['provider_type'] == Provider.PIAZZA assert data['lti_configuration'][key] == value def test_post_lti_invalid(self): @@ -473,11 +401,10 @@ class DataTest(AuthorizedApiTest): The fields are all open-ended strings and will accept any values. """ - provider_type = 'piazza' - for key, value in DATA_LTI_CONFIGURATION.items(): + for key, value in DATA_POST_LTI_CONFIGURATION.items(): payload = { 'enabled': True, - 'provider_type': provider_type, + 'provider_type': Provider.PIAZZA, 'lti_configuration': { key: value, 'ignored-key': 'ignored value', @@ -487,7 +414,7 @@ class DataTest(AuthorizedApiTest): assert response.status_code == status.HTTP_200_OK data = self.get() assert data['enabled'] - assert data['provider_type'] == provider_type + assert data['provider_type'] == Provider.PIAZZA assert data['lti_configuration'][key] == value assert 'ignored-key' not in data['lti_configuration'] @@ -495,20 +422,12 @@ class DataTest(AuthorizedApiTest): """ Test that we can set & retrieve edx provider configuration. """ - provider_type = 'legacy' for key, value in DATA_LEGACY_CONFIGURATION.items(): - payload = { - 'enabled': True, - 'provider_type': provider_type, - 'plugin_configuration': { - key: value, - } - } - response = self._post(payload) - assert response + self._configure_legacy_discussion_provider(configuration={key: value}) + data = self.get() assert data['enabled'] - assert data['provider_type'] == provider_type + assert data['provider_type'] == Provider.LEGACY assert data['plugin_configuration'][key] == value @ddt.data( @@ -521,10 +440,9 @@ class DataTest(AuthorizedApiTest): """ Check validation of legacy settings configuration """ - provider_type = 'legacy' payload = { 'enabled': True, - 'provider_type': provider_type, + 'provider_type': Provider.LEGACY, 'plugin_configuration': plugin_configuration, } with self.assertRaises(ValidationError): @@ -539,18 +457,11 @@ class DataTest(AuthorizedApiTest): """ Test that we can set & retrieve legacy cohorts configuration. """ - provider_type = 'legacy' - payload = { - 'enabled': True, - 'provider_type': provider_type, - 'plugin_configuration': { - key: value, - } - } - self._post(payload) + self._configure_legacy_discussion_provider(configuration={key: value}) + data = self.get() assert data['enabled'] - assert data['provider_type'] == provider_type + assert data['provider_type'] == Provider.LEGACY assert data['plugin_configuration'][key] == value @ddt.data(*DATA_LEGACY_COHORTS.items()) @@ -565,10 +476,10 @@ class DataTest(AuthorizedApiTest): else: # Otherwise, submit a string when non-string is required value = str(value) - provider_type = 'legacy' + payload = { 'enabled': True, - 'provider_type': provider_type, + 'provider_type': Provider.LEGACY, 'plugin_configuration': { key: value, } @@ -585,24 +496,18 @@ class DataTest(AuthorizedApiTest): When switching provider to LTI, the API should return both LTI & legacy data. """ - payload = { - 'enabled': True, - 'provider_type': 'legacy', - 'plugin_configuration': { - 'allow_anonymous': False, - }, - } - self._post(payload) + plugin_configuration = {'allow_anonymous': False} + self._configure_legacy_discussion_provider(configuration=plugin_configuration) self._configure_lti_discussion_provider(provider=Provider.ED_DISCUSS) - data = self.get() + data = self.get() assert data['enabled'] assert data['provider_type'] == Provider.ED_DISCUSS assert data['plugin_configuration'] == { **DEFAULT_LEGACY_CONFIGURATION, 'allow_anonymous': False, } - assert data['lti_configuration'] == DATA_LTI_CONFIGURATION + assert data['lti_configuration'] == DATA_LTI_CONFIGURATION_DISABLED_PII def test_change_from_lti(self): """ @@ -611,17 +516,11 @@ class DataTest(AuthorizedApiTest): When switching provider to LTI, the API should return both LTI & legacy data. """ self._configure_lti_discussion_provider() - payload = { - 'enabled': True, - 'provider_type': 'legacy', - 'plugin_configuration': { - 'allow_anonymous': False, - }, - } - response = self._post(payload) - data = response.json() + + plugin_configuration = {'allow_anonymous': False} + data = self._configure_legacy_discussion_provider(configuration=plugin_configuration) assert data['enabled'] - assert data['provider_type'] == 'legacy' + assert data['provider_type'] == Provider.LEGACY assert not data['plugin_configuration']['allow_anonymous'] @ddt.data( @@ -629,7 +528,7 @@ class DataTest(AuthorizedApiTest): ["enable_in_context", "enable_graded_units", "unit_level_visibility"], [True, False], ), - ("provider_type", "piazza"), + ("provider_type", Provider.PIAZZA), ) @ddt.unpack def test_change_course_fields(self, field, value): @@ -695,3 +594,168 @@ class DataTest(AuthorizedApiTest): 'provider_type': 'piazza', }) assert response.status_code == status.HTTP_200_OK + + +@ddt.ddt +class PIISettingsAPITests(DataTest): + """ + Test PII sharing setting for course. + """ + + @contextmanager + def _pii_sharing_for_course(self, enabled): + instance = CourseAllowPIISharingInLTIFlag.objects.create(course_id=self.course.id, enabled=enabled) + yield + instance.delete() + + def _assert_pii_flag_for_course(self, enabled): + """ + Asserts pii flag has given state. + """ + try: + course_pii = CourseAllowPIISharingInLTIFlag.objects.filter(course_id=self.course.id).latest('change_date') + assert course_pii.enabled == enabled + except CourseAllowPIISharingInLTIFlag.DoesNotExist: + assert enabled is False + + def test_pii_sharing_disabled(self): + """ + Tests that pii settings are not enabled by default when configuring LTI provider. + """ + self._configure_lti_discussion_provider() + self._assert_pii_flag_for_course(enabled=False) + + with self._pii_sharing_for_course(enabled=False): + self._assert_pii_flag_for_course(enabled=False) + + def test_pii_sharing_enabled(self): + """ + Tests that pii sharing settings can be enabled after configuring lti provider. + """ + self._configure_lti_discussion_provider() + with self._pii_sharing_for_course(enabled=True): + self._assert_pii_flag_for_course(enabled=True) + + @ddt.data( + {"pii_share_username": True}, + {"pii_share_email": True}, + {"pii_share_email": True, "pii_share_username": True}, + ) + def test_post_pii_fields_with_non_configured_pii(self, pii_configuration): + """ + Tests that if PII sharing is *not configured* for the course, the api call + to update PII settings for the course fails. + """ + data = self._configure_lti_discussion_provider() + data['lti_configuration'].update(pii_configuration) + response = self._post(data) + assert response.status_code == status.HTTP_400_BAD_REQUEST + + @ddt.data( + {"pii_share_username": True}, + {"pii_share_email": True}, + {"pii_share_email": True, "pii_share_username": True}, + ) + def test_post_pii_fields_with_pii_disabled(self, pii_configuration): + """ + Test that when PII sharing is *disabled* for the course, user is not able + update PII settings for a course. + """ + data = self._configure_lti_discussion_provider() + data['lti_configuration'].update(pii_configuration) + with self._pii_sharing_for_course(enabled=False): + response = self._post(data) + assert response.status_code == status.HTTP_400_BAD_REQUEST + lti_configuration = self.get()['lti_configuration'] + no_pii_fields_updated = [ + lti_configuration.get(pii_field) != pii_value + for pii_field, pii_value in pii_configuration.items() + ] + assert all(no_pii_fields_updated) + + @ddt.data( + {"pii_share_username": True}, + {"pii_share_email": True}, + {"pii_share_email": True, "pii_share_username": True}, + {"pii_share_email": False, "pii_share_username": True}, + {"pii_share_email": True, "pii_share_username": False}, + {"pii_share_email": False, "pii_share_username": False}, + ) + def test_post_pii_fields_with_pii_enabled(self, pii_configuration): + """ + Test that when PII sharing is enabled for the course, user is able + update PII settings for the course. + """ + data = self._configure_lti_discussion_provider() + data['lti_configuration'].update(pii_configuration) + with self._pii_sharing_for_course(enabled=True): + response = self._post(data) + assert response.status_code == status.HTTP_200_OK + + lti_configuration = self.get()['lti_configuration'] + all_pii_fields_updated = [ + lti_configuration[pii_field] == pii_value + for pii_field, pii_value in pii_configuration.items() + ] + assert all(all_pii_fields_updated) + + @ddt.data( + True, + False + ) + def test_get_pii_fields(self, pii_sharing): + """ + Tests that when PII sharing is included in the API regardless of course pii settings. + """ + self._configure_lti_discussion_provider() + with self._pii_sharing_for_course(enabled=pii_sharing): + lti_configuration = self.get()['lti_configuration'] + # If pii_sharing is true, then the fields should be present, and absent otherwise + assert lti_configuration['pii_sharing_allowed'] == pii_sharing + assert 'pii_share_email' in lti_configuration + assert 'pii_share_username' in lti_configuration + + @ddt.data( + Provider.ED_DISCUSS, + Provider.INSCRIBE, + Provider.PIAZZA, + Provider.YELLOWDIG, + ) + def test_post_everything_with_pii_disabled(self, provider): + """ + Test posting all data returns expected response when course PII flag is disabled. + """ + data = self._configure_lti_discussion_provider(provider=provider) + self._assert_pii_flag_for_course(enabled=False) + assert data['enabled'] + assert data['provider_type'] == provider + assert data['providers']['available'][provider] == AVAILABLE_PROVIDER_MAP[provider] + assert data['plugin_configuration'] == DEFAULT_LEGACY_CONFIGURATION + assert data['lti_configuration'] == DATA_LTI_CONFIGURATION_DISABLED_PII + + response_data = self.get() + # the GET should pull back the same data as the POST + assert response_data == data + + @ddt.data( + Provider.ED_DISCUSS, + Provider.INSCRIBE, + Provider.PIAZZA, + Provider.YELLOWDIG, + ) + def test_post_everything_with_pii_enabled(self, provider): + """ + Test posting all data returns expected response when course PII flag is disabled. + """ + with self._pii_sharing_for_course(enabled=True): + self._assert_pii_flag_for_course(enabled=True) + data = self._configure_lti_discussion_provider(provider=provider) + assert data['enabled'] + assert data['provider_type'] == provider + assert data['providers']['available'][provider] == AVAILABLE_PROVIDER_MAP[provider] + assert data['plugin_configuration'] == DEFAULT_LEGACY_CONFIGURATION + assert data['lti_configuration'] == DATA_LTI_CONFIGURATION_ENABLED_PII + + response_data = self.get() + # the GET should pull back the same data as the POST + assert response_data == data