diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 2e65e83532..43c1b1aeae 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -20,7 +20,7 @@ from edx_proctoring.api import ( ) from edx_proctoring.exceptions import ProctoredExamNotFoundException from help_tokens.core import HelpUrlExpert -from lti_consumer.models import CourseEditLTIFieldsEnabledFlag +from lti_consumer.models import CourseAllowPIISharingInLTIFlag from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import LibraryUsageLocator from pytz import UTC @@ -311,7 +311,7 @@ class StudioEditModuleRuntime: if service_name == "settings": return SettingsService() if service_name == "lti-configuration": - return ConfigurationService(CourseEditLTIFieldsEnabledFlag) + return ConfigurationService(CourseAllowPIISharingInLTIFlag) if service_name == "teams_configuration": return TeamsConfigurationService() if service_name == "library_tools": diff --git a/openedx/core/djangoapps/discussions/serializers.py b/openedx/core/djangoapps/discussions/serializers.py index 1649f11011..a9a48d6021 100644 --- a/openedx/core/djangoapps/discussions/serializers.py +++ b/openedx/core/djangoapps/discussions/serializers.py @@ -2,15 +2,16 @@ Serializers for Discussion views. """ +from lti_consumer.api import get_lti_pii_sharing_state_for_course from lti_consumer.models import LtiConfiguration +from opaque_keys.edx.keys import CourseKey from rest_framework import serializers +from xmodule.modulestore.django import modulestore from lms.djangoapps.discussion.rest_api.serializers import DiscussionSettingsSerializer from openedx.core.djangoapps.django_comment_common.models import CourseDiscussionSettings from openedx.core.lib.courses import get_course_by_id -from xmodule.modulestore.django import modulestore - -from .models import DEFAULT_PROVIDER_TYPE, AVAILABLE_PROVIDER_MAP, DiscussionsConfiguration, Features +from .models import AVAILABLE_PROVIDER_MAP, DEFAULT_PROVIDER_TYPE, DiscussionsConfiguration, Features class LtiSerializer(serializers.ModelSerializer): @@ -20,6 +21,8 @@ class LtiSerializer(serializers.ModelSerializer): class Meta: model = LtiConfiguration fields = [ + 'pii_share_username', + 'pii_share_email', 'lti_1p1_client_key', 'lti_1p1_client_secret', 'lti_1p1_launch_url', @@ -38,14 +41,26 @@ class LtiSerializer(serializers.ModelSerializer): } return payload + def to_representation(self, instance): + 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 """ instance = instance or LtiConfiguration() instance.config_store = LtiConfiguration.CONFIG_ON_DB + pii_sharing_allowed = self.context.get('pii_sharing_allowed', False) if validated_data: for key, value in validated_data.items(): + if key.startswith('pii_') and not pii_sharing_allowed: + raise serializers.ValidationError( + "Cannot enable sending PII data until PII sharing for LTI is enabled for the course." + ) if key in self.Meta.fields: setattr(instance, key, value) instance.save() @@ -185,16 +200,18 @@ class DiscussionsConfigurationSerializer(serializers.ModelSerializer): """ Serialize data into a dictionary, to be used as a response """ + course_key = instance.context_key payload = super().to_representation(instance) lti_configuration_data = {} supports_lti = instance.supports('lti') if supports_lti: - lti_configuration = LtiSerializer(instance.lti_configuration) + lti_configuration = LtiSerializer(instance.lti_configuration, context={ + 'pii_sharing_allowed': get_lti_pii_sharing_state_for_course(course_key), + }) lti_configuration_data = lti_configuration.data provider_type = instance.provider_type or DEFAULT_PROVIDER_TYPE plugin_configuration = instance.plugin_configuration if provider_type == 'legacy': - course_key = instance.context_key course = get_course_by_id(course_key) legacy_settings = LegacySettingsSerializer( course, @@ -224,12 +241,17 @@ class DiscussionsConfigurationSerializer(serializers.ModelSerializer): setattr(instance, key, value) # _update_* helpers assume `enabled` and `provider_type` # have already been set - instance = self._update_lti(instance, validated_data) + instance = self._update_lti(instance, validated_data, instance.context_key) instance = self._update_plugin_configuration(instance, validated_data) instance.save() return instance - def _update_lti(self, instance: DiscussionsConfiguration, validated_data: dict) -> DiscussionsConfiguration: + def _update_lti( + self, + instance: DiscussionsConfiguration, + validated_data: dict, + course_key: CourseKey + ) -> DiscussionsConfiguration: """ Update LtiConfiguration """ @@ -243,6 +265,9 @@ class DiscussionsConfigurationSerializer(serializers.ModelSerializer): lti_configuration, data=lti_configuration_data, partial=True, + context={ + 'pii_sharing_allowed': get_lti_pii_sharing_state_for_course(course_key), + } ) if lti_serializer.is_valid(raise_exception=True): lti_serializer.save() diff --git a/openedx/core/djangoapps/discussions/tests/test_views.py b/openedx/core/djangoapps/discussions/tests/test_views.py index 580a418f47..d1ad3e7e18 100644 --- a/openedx/core/djangoapps/discussions/tests/test_views.py +++ b/openedx/core/djangoapps/discussions/tests/test_views.py @@ -4,11 +4,13 @@ Test app view logic # pylint: disable=test-inherits-tests from datetime import datetime, timedelta, timezone import unittest +from contextlib import contextmanager import ddt from django.conf import settings from django.core.exceptions import ValidationError from django.urls import reverse +from lti_consumer.models import CourseAllowPIISharingInLTIFlag from rest_framework import status from rest_framework.test import APITestCase @@ -182,6 +184,59 @@ class DataTest(AuthorizedApiTest): response = self._get() self._assert_defaults(response) + @contextmanager + def _pii_sharing_for_course(self, enabled): + instance = CourseAllowPIISharingInLTIFlag.objects.create(course_id=self.course.id, enabled=enabled) + yield + instance.delete() + + @ddt.data( + {"pii_share_username": True}, + {"pii_share_email": True}, + {"pii_share_email": True, "pii_share_username": True}, + ) + def test_post_pii_fields_fail(self, pii_fields): + """ + If no record exists, defaults should be returned. + """ + data = self._setup_lti() + data['lti_configuration'].update(pii_fields) + response = self._post(data) + assert response.status_code == 400 + + @ddt.data( + {"pii_share_username": True}, + {"pii_share_email": True}, + {"pii_share_email": True, "pii_share_username": True}, + ) + def test_post_pii_fields(self, pii_fields): + """ + Only if PII sharing is enabled should a user be able to set pii fields. + """ + data = self._setup_lti() + data['lti_configuration'].update(pii_fields) + with self._pii_sharing_for_course(enabled=False): + response = self._post(data) + assert response.status_code == 400 + with self._pii_sharing_for_course(enabled=True): + response = self._post(data) + assert response.status_code == 200 + + @ddt.data( + True, False + ) + def test_get_pii_fields(self, pii_sharing): + """ + Only if PII is enabled should pii fields be returned. + """ + self._setup_lti() + with self._pii_sharing_for_course(enabled=pii_sharing): + response = self._get() + data = response.json() + # 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 + def test_post_everything(self): """ API should accept requests to update _all_ fields at once diff --git a/openedx/features/lti_course_tab/tab.py b/openedx/features/lti_course_tab/tab.py index a6c90e4023..b5bce357eb 100644 --- a/openedx/features/lti_course_tab/tab.py +++ b/openedx/features/lti_course_tab/tab.py @@ -8,6 +8,7 @@ from django.contrib.auth.models import AbstractBaseUser from django.contrib.sites.shortcuts import get_current_site from django.http import HttpRequest from django.utils.translation import get_language, to_locale, ugettext_lazy +from lti_consumer.api import get_lti_pii_sharing_state_for_course from lti_consumer.lti_1p1.contrib.django import lti_embed from lti_consumer.models import LtiConfiguration from opaque_keys.edx.keys import CourseKey @@ -34,7 +35,43 @@ class LtiCourseLaunchMixin: } DEFAULT_ROLE = 'Student' + def _get_pii_lti_parameters(self, course: CourseBlock, request: HttpRequest) -> Dict[str, str]: + """ + Get LTI parameters that contain PII. + + Args: + course (CourseBlock): CourseBlock object. + request (HttpRequest): Request object for view in which LTI will be embedded. + + Returns: + Dictionary with LTI parameters containing PII. + """ + pii_sharing_allowed = get_lti_pii_sharing_state_for_course(course.id) + if not pii_sharing_allowed: + return {} + lti_config = self._get_lti_config(course) + # Currently only LTI 1.1 is supported by the tab + if lti_config.version != lti_config.LTI_1P1: + return {} + + pii_config = {} + if lti_config.pii_share_username: + pii_config['person_sourcedid'] = request.user.username + if lti_config.pii_share_email: + pii_config['person_contact_email_primary'] = request.user.email + return pii_config + def _get_additional_lti_parameters(self, course: CourseBlock, request: HttpRequest) -> Dict[str, str]: + """ + Get additional misc LTI parameters. + + Args: + course (CourseBlock): CourseBlock object. + request (HttpRequest): Request object for view in which LTI will be embedded. + + Returns: + Dictionary with additional LTI parameters. + """ lti_config = self._get_lti_config(course) additional_config = lti_config.lti_config.get('additional_parameters', {}) return additional_config @@ -98,6 +135,7 @@ class LtiCourseLaunchMixin: context_title = self._get_context_title(course) result_sourcedid = quote(self._get_result_sourcedid(context_id, resource_link_id, user_id)) additional_params = self._get_additional_lti_parameters(course, request) + pii_params = self._get_pii_lti_parameters(course, request) locale = to_locale(get_language()) return lti_embed( @@ -111,6 +149,7 @@ class LtiCourseLaunchMixin: context_label=context_id, result_sourcedid=result_sourcedid, launch_presentation_locale=locale, + **pii_params, **additional_params, ) diff --git a/openedx/features/lti_course_tab/tests.py b/openedx/features/lti_course_tab/tests.py index a28401576c..3813246298 100644 --- a/openedx/features/lti_course_tab/tests.py +++ b/openedx/features/lti_course_tab/tests.py @@ -1,39 +1,76 @@ """ Tests for LTI Course tabs. """ +import itertools from unittest.mock import Mock, patch +import ddt +from django.test import RequestFactory +from lti_consumer.models import CourseAllowPIISharingInLTIFlag, LtiConfiguration + from lms.djangoapps.courseware.tests.test_tabs import TabTestCase +from openedx.core.djangoapps.discussions.models import DiscussionsConfiguration from openedx.features.lti_course_tab.tab import DiscussionLtiCourseTab +@ddt.ddt class DiscussionLtiCourseTabTestCase(TabTestCase): """Test cases for LTI Discussion Tab.""" + def setUp(self): + super().setUp() + self.discussion_config = DiscussionsConfiguration.objects.create(context_key=self.course.id, enabled=False) + self.discussion_config.lti_configuration = LtiConfiguration.objects.create( + config_store=LtiConfiguration.CONFIG_ON_DB, + lti_1p1_launch_url='http://test.url', + lti_1p1_client_key='test_client_key', + lti_1p1_client_secret='test_client_secret', + ) + self.discussion_config.save() + self.url = self.reverse('course_tab_view', args=[str(self.course.id), DiscussionLtiCourseTab.type]) + def check_discussion_tab(self): """Helper function for verifying the LTI discussion tab.""" return self.check_tab( tab_class=DiscussionLtiCourseTab, dict_tab={'type': DiscussionLtiCourseTab.type, 'name': 'same'}, - expected_link=self.reverse('course_tab_view', args=[str(self.course.id), DiscussionLtiCourseTab.type]), + expected_link=self.url, expected_tab_id=DiscussionLtiCourseTab.type, invalid_dict_tab=None, ) - @patch('openedx.features.lti_course_tab.tab.DiscussionsConfiguration.get') - @patch('common.djangoapps.student.models.CourseEnrollment.is_enrolled') - def test_discussion_lti_tab(self, is_enrolled, discussion_config_get): - is_enrolled.return_value = True - mock_config = Mock() - mock_config.lti_configuration = {} - mock_config.enabled = False - discussion_config_get.return_value = mock_config + @ddt.data(True, False) + @patch('common.djangoapps.student.models.CourseEnrollment.is_enrolled', Mock(return_value=True)) + def test_pii_params_on_discussion_lti_tab(self, discussion_config_enabled): + self.discussion_config.enabled = discussion_config_enabled + self.discussion_config.save() tab = self.check_discussion_tab() self.check_can_display_results( - tab, for_staff_only=True, for_enrolled_users_only=True, expected_value=False - ) - mock_config.enabled = True - self.check_discussion_tab() - self.check_can_display_results( - tab, for_staff_only=True, for_enrolled_users_only=True + tab, + for_staff_only=True, + for_enrolled_users_only=True, + expected_value=discussion_config_enabled, ) + + @ddt.data(*itertools.product((True, False), repeat=3)) + @ddt.unpack + def test_discussion_lti_tab_pii(self, enable_sending_pii, share_username, share_email): + CourseAllowPIISharingInLTIFlag.objects.create(course_id=self.course.id, enabled=enable_sending_pii) + self.discussion_config.lti_configuration.lti_config = { + "pii_share_username": share_username, + "pii_share_email": share_email, + } + self.discussion_config.lti_configuration.save() + tab = self.check_discussion_tab() + request = RequestFactory().get(self.url) + user = self.create_mock_user(is_enrolled=True) + request.user = user + embed_code = tab._get_lti_embed_code(self.course, request) # pylint: disable=protected-access + if enable_sending_pii and share_username: + assert user.username in embed_code + else: + assert user.username not in embed_code + if enable_sending_pii and share_email: + assert user.email in embed_code + else: + assert user.email not in embed_code diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 69ed9581d3..da88e7e941 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -596,7 +596,7 @@ libsass==0.10.0 # ora2 loremipsum==1.0.5 # via ora2 -lti-consumer-xblock==2.11.0 +lti-consumer-xblock==3.0.0 # via -r requirements/edx/base.in lxml==4.5.0 # via diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 6fbd03d1b6..526319585f 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -776,7 +776,7 @@ loremipsum==1.0.5 # via # -r requirements/edx/testing.txt # ora2 -lti-consumer-xblock==2.11.0 +lti-consumer-xblock==3.0.0 # via -r requirements/edx/testing.txt lxml==4.5.0 # via diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index a0a6eed2b1..46ed30c645 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -741,7 +741,7 @@ loremipsum==1.0.5 # via # -r requirements/edx/base.txt # ora2 -lti-consumer-xblock==2.11.0 +lti-consumer-xblock==3.0.0 # via -r requirements/edx/base.txt lxml==4.5.0 # via