Merge pull request #317 from openedx/mroytman/MST-1736-enable-usernam…

…e-email-PII-LTI-1.3

Fix PII Sharing Behavior and Enable PII Sharing in LTI 1.3 Launches and Fix LTI 1.3 Modal Launches

CHANGELOG.rst

@@ -16,6 +16,41 @@ Please See the [releases tab](https://github.com/openedx/xblock-lti-consumer/rel
 Unreleased
 ~~~~~~~~~~
 
+7.2.0 - 2022-12-15
+------------------
+
+This release addresses a number of issues with and bugs in sharing personally identifiable information (PII) in LTI
+launches.
+
+* Replaces the PII sharing consent modal with an inline PII sharing consent dialog to better suit the three different
+  LTI launch types (i.e. ``inline``, ``modal``, and ``new_window``).
+* Adds a PII consent dialog for ``inline`` LTI launches.
+* Fixes a bug in the ``modal`` LTI launch in LTI 1.3 that was preventing the LTI launch.
+* Fixes a bug in evaluating and caching whether PII sharing is enabled via the ``CourseAllowPIISharingInLTIFlag``.
+
+  * This fixes a bug where the PII sharing fields in the LTI XBlock edit menu appeared regardless of the existence or
+    value of this flag. The PII sharing fields will now always be hidden if either no ``CourseAllowPIISharingInLTIFlag``
+    exists for a course or if a ``CourseAllowPIISharingInLTIFlag`` exists for the course but is not enabled.
+  * This fixes a bug in the backwards compatibility code in ``lti_access_to_learners_editable``. Now,
+    ``CourseAllowPIISharingInLTIFlag`` will always be created for courses that contain (an) LTI XBlock(s) that have (a)
+    PII sharing field(s) set to True when a user opens the LTI XBlock edit menu. Before, this would occur inconsistently
+    due to a bug in the caching code.
+
+* Enables sharing username and email in LTI 1.3 launches.
+
+  * Adds ``preferred_username`` and ``email`` attributes to the ``Lti1p3LaunchData`` class. The application or context
+    that instantiates ``Lti1p3LaunchData`` is responsible for ensuring that username and email can be sent via an LTI
+    1.3 launch and supplying these data, if appropriate.
+
+* Adds code to eventually support the value of ``CourseAllowPIISharingInLTIFlag`` controlling PII sharing for a given
+  course in LTI 1.1 and LTI 1.3 launches.
+
+  * This code does not currently work, because the LTI configuration service is not available or defined in all runtime
+    contexts. This code works in the LTI XBlock edit menu (i.e. the ``studio_view``), but it does not work in the Studio
+    preview context (i.e. the ``author_view``) or the LMS (i.e. the ``student_view``). The effect is that
+    the ``CourseAllowPIISharingInLTIFlag`` can only control the appearance of the username and email PII sharing fields
+    in the XBlock edit menu; it does not control PII sharing. We plan to fix this bug in the future.
+
 7.1.0 - 2022-12-09
 ------------------
 * Add support for platform setting `LTI_NRPS_DISALLOW_PII` to prevent sharing of pii over the names and roles

lti_consumer/__init__.py

@@ -4,4 +4,4 @@
 from .apps import LTIConsumerApp
 from .lti_xblock import LtiConsumerXBlock
 
-__version__ = '7.1.0'
+__version__ = '7.2.0'

lti_consumer/data.py

@@ -45,6 +45,8 @@ class Lti1p3LaunchData:
     * config_id (required): The config_id field of an LtiConfiguration to use for the launch.
     * resource_link_id (required): A unique identifier that is guaranteed to be unique for each placement of the LTI
         link.
+    * preferred_username (optional): The user's username.
+    * email (optional): The user's email.
     * external_user_id (optional): A unique identifier for the user that is requesting the LTI 1.3 launch that can be
         shared externally. The identifier must be stable to the issuer. This value will be sent to the the Tool in the
         form of both the login_hint in the login initiation request and the sub claim in the ID token of the LTI 1.3
@@ -73,6 +75,8 @@ class Lti1p3LaunchData:
     user_role = field()
     config_id = field()
     resource_link_id = field()
+    preferred_username = field(default=None)
+    email = field(default=None)
     external_user_id = field(default=None)
     launch_presentation_document_target = field(default=None)
     launch_presentation_return_url = field(default=None)

lti_consumer/lti_1p3/consumer.py

@@ -132,7 +132,8 @@ def set_user_data(
             user_id,
             role,
             full_name=None,
-            email_address=None
+            email_address=None,
+            preferred_username=None,
     ):
         """
         Set user data/roles and convert to IMS Specification
@@ -162,6 +163,11 @@ def set_user_data(
                 "email": email_address,
             })
 
+        if preferred_username:
+            self.lti_claim_user_data.update({
+                "preferred_username": preferred_username,
+            })
+
     def set_resource_link_claim(
         self,
         resource_link_id,

lti_consumer/lti_1p3/tests/test_consumer.py

@@ -210,12 +210,20 @@ def test_prepare_preflight_url(self):
         ),
         # User with extra data
         (
-            {"user_id": "1", "role": '', "full_name": "Jonh", "email_address": "[email protected]"},
+            {
+                "user_id": "1",
+                "role": '',
+                "full_name":
+                "Jonh",
+                "email_address":
+                "[email protected]",
+                "preferred_username": "johnuser"},
             {
                 "sub": "1",
                 "https://purl.imsglobal.org/spec/lti/claim/roles": [],
                 "name": "Jonh",
-                "email": "[email protected]"
+                "email": "[email protected]",
+                "preferred_username": "johnuser",
             }
         ),
     )

lti_consumer/lti_xblock.py

@@ -673,6 +673,26 @@ def get_parameter_processors(self):
             log.exception('Something went wrong in reading the LTI XBlock configuration.')
             raise
 
+    def get_pii_sharing_enabled(self):
+        """
+        Returns whether PII can be transmitted via this XBlock. This controls both whether the PII sharing XBlock
+        fields ask_to_send_username and ask_to_send_email are displayed in Studio and whether these data are shared
+        in LTI launches, regardless of the values of the settings on the XBlock.
+        """
+        config_service = self.runtime.service(self, 'lti-configuration')
+        if config_service:
+            is_already_sharing_learner_info = self.ask_to_send_email or self.ask_to_send_username
+
+            return config_service.configuration.lti_access_to_learners_editable(
+                self.scope_ids.usage_id.context_key,
+                is_already_sharing_learner_info,
+            )
+
+        # TODO: The LTI configuration service is currently only available from the studio_view. This means that
+        #       the CourseAllowPIISharingInLTIFlag does not control PII sharing in the author_view or student_view,
+        #       because the service is not defined in those contexts.
+        return True
+
     @property
     def editable_fields(self):
         """
@@ -710,14 +730,9 @@ def editable_fields(self):
 
         # update the editable fields if this XBlock is configured to not to allow the
         # editing of 'ask_to_send_username' and 'ask_to_send_email'.
-        config_service = self.runtime.service(self, 'lti-configuration')
-        if config_service:
-            is_already_sharing_learner_info = self.ask_to_send_email or self.ask_to_send_username
-            if not config_service.configuration.lti_access_to_learners_editable(
-                    self.scope_ids.usage_id.context_key,
-                    is_already_sharing_learner_info,
-            ):
-                noneditable_fields.extend(['ask_to_send_username', 'ask_to_send_email'])
+        pii_sharing_enabled = self.get_pii_sharing_enabled()
+        if not pii_sharing_enabled:
+            noneditable_fields.extend(['ask_to_send_username', 'ask_to_send_email'])
 
         editable_fields = tuple(
             field
@@ -1164,10 +1179,13 @@ def lti_launch_handler(self, request, suffix=''):  # pylint: disable=unused-argu
 
         username = None
         email = None
-        if self.ask_to_send_username and real_user_data['user_username']:
-            username = real_user_data['user_username']
-        if self.ask_to_send_email and real_user_data['user_email']:
-            email = real_user_data['user_email']
+        # Send PII fields only if this XBlock is configured to allow the sending PII.
+        pii_sharing_enabled = self.get_pii_sharing_enabled()
+        if pii_sharing_enabled:
+            if self.ask_to_send_username and real_user_data['user_username']:
+                username = real_user_data['user_username']
+            if self.ask_to_send_email and real_user_data['user_email']:
+                email = real_user_data['user_email']
 
         lti_consumer.set_user_data(
             user_id,
@@ -1478,12 +1496,26 @@ def get_lti_1p3_launch_data(self):
         location = self.scope_ids.usage_id
         course_key = str(location.context_key)
 
+        username = None
+        email = None
+
+        pii_sharing_enabled = self.get_pii_sharing_enabled()
+        if pii_sharing_enabled:
+            user_data = self.extract_real_user_data()
+
+            if self.ask_to_send_username and user_data['user_username']:
+                username = user_data['user_username']
+            if self.ask_to_send_email and user_data['user_email']:
+                email = user_data['user_email']
+
         launch_data = Lti1p3LaunchData(
             user_id=self.lms_user_id,
             user_role=self.role,
             config_id=config_id,
             resource_link_id=str(location),
             external_user_id=self.external_user_id,
+            preferred_username=username,
+            email=email,
             launch_presentation_document_target="iframe",
             context_id=course_key,
             context_type=["course_offering"],
@@ -1568,6 +1600,9 @@ def _get_context_for_template(self):
         lti_block_launch_handler = self._get_lti_block_launch_handler()
         lti_1p3_launch_url = self._get_lti_1p3_launch_url(lti_consumer)
 
+        # The values of ask_to_send_username and ask_to_send_email should only apply if PII sharing is enabled.
+        pii_sharing_enabled = self.get_pii_sharing_enabled()
+
         return {
             'launch_url': launch_url.strip(),
             'lti_1p3_launch_url': lti_1p3_launch_url,
@@ -1582,14 +1617,15 @@ def _get_context_for_template(self):
             'module_score': self.module_score,
             'comment': sanitized_comment,
             'description': self.description,
-            'ask_to_send_username': self.ask_to_send_username,
-            'ask_to_send_email': self.ask_to_send_email,
+            'ask_to_send_username': self.ask_to_send_username if pii_sharing_enabled else False,
+            'ask_to_send_email': self.ask_to_send_email if pii_sharing_enabled else False,
             'button_text': self.button_text,
             'inline_height': self.inline_height,
             'modal_vertical_offset': self._get_modal_position_offset(self.modal_height),
             'modal_horizontal_offset': self._get_modal_position_offset(self.modal_width),
             'modal_width': self.modal_width,
             'accept_grades_past_due': self.accept_grades_past_due,
+            'lti_version': self.lti_version,
         }
 
     def _get_modal_position_offset(self, viewport_percentage):

lti_consumer/models.py

@@ -23,7 +23,6 @@
 from lti_consumer.lti_1p3.consumer import LtiAdvantageConsumer, LtiProctoringConsumer
 from lti_consumer.lti_1p3.key_handlers import PlatformKeyHandler
 from lti_consumer.plugin import compat
-from lti_consumer.plugin.compat import request_cached
 from lti_consumer.utils import (
     get_lms_base,
     get_lti_ags_lineitems_url,
@@ -778,7 +777,6 @@ class CourseAllowPIISharingInLTIFlag(ConfigurationModel):
     course_id = CourseKeyField(max_length=255, db_index=True)
 
     @classmethod
-    @request_cached
     def lti_access_to_learners_editable(cls, course_id: CourseKey, is_already_sharing_learner_info: bool) -> bool:
         """
         Looks at the currently active configuration model to determine whether

lti_consumer/plugin/views.py

@@ -187,6 +187,8 @@ def launch_gate_endpoint(request, suffix=None):  # pylint: disable=unused-argume
         lti_consumer.set_user_data(
             user_id=user_id,
             role=user_role,
+            email_address=launch_data.email,
+            preferred_username=launch_data.preferred_username,
         )
 
         # Set resource_link claim.