Merge pull request #172 from open-craft/kshitij/bb-3899/pii-flag-cleanup

refactor: Rename CourseEditLTIFieldsEnabledFlag to CourseAllowPIISharingInLTIFlag and use it for LTI1.3 [BD-38] [BB-3899] [TNL-8104]

Author: awaisdar001

README.rst

@@ -318,9 +318,21 @@ Please do not report security issues in public. Send security concerns via email
 Changelog
 =========
 
+3.0.0 - 2021-06-16
+-------------------
+
+* Rename `CourseEditLTIFieldsEnabledFlag` to `CourseAllowPIISharingInLTIFlag`
+  to highlight its increased scope.
+* Use `CourseAllowPIISharingInLTIFlag` for LTI1.3 in lieu of the current
+  `CourseWaffleFlag`.
+
+
 2.11.0 - 2021-06-10
 -------------------
 
+* NOTE: This release requires a corresponding change in edx-platform that was
+  implemented in https://github.com/edx/edx-platform/pull/27529
+  As such, this release cannot be installed in releases before Maple.
 * Move ``CourseEditLTIFieldsEnabledFlag`` from ``edx-platform`` to this repo
   while retaining data from existing model.
 

lti_consumer/admin.py

@@ -4,9 +4,9 @@
 from config_models.admin import KeyedConfigurationModelAdmin
 from django.contrib import admin
 
-from lti_consumer.forms import CourseEditLTIFieldsEnabledAdminForm
+from lti_consumer.forms import CourseAllowPIISharingInLTIAdminForm
 from lti_consumer.models import (
-    CourseEditLTIFieldsEnabledFlag,
+    CourseAllowPIISharingInLTIFlag,
     LtiAgsLineItem,
     LtiAgsScore,
     LtiConfiguration,
@@ -23,12 +23,12 @@ class LtiConfigurationAdmin(admin.ModelAdmin):
     readonly_fields = ('location', 'config_id')
 
 
-class CourseEditLTIFieldsEnabledFlagAdmin(KeyedConfigurationModelAdmin):
+class CourseAllowPIISharingInLTIFlagAdmin(KeyedConfigurationModelAdmin):
     """
-    Admin for LTI Fields Editing feature on course-by-course basis.
+    Admin for enabling PII Sharing in LTI on course-by-course basis.
     Allows searching by course id.
     """
-    form = CourseEditLTIFieldsEnabledAdminForm
+    form = CourseAllowPIISharingInLTIAdminForm
     search_fields = ['course_id']
     fieldsets = (
         (None, {
@@ -38,7 +38,7 @@ class CourseEditLTIFieldsEnabledFlagAdmin(KeyedConfigurationModelAdmin):
     )
 
 
-admin.site.register(CourseEditLTIFieldsEnabledFlag, CourseEditLTIFieldsEnabledFlagAdmin)
+admin.site.register(CourseAllowPIISharingInLTIFlag, CourseAllowPIISharingInLTIFlagAdmin)
 admin.site.register(LtiConfiguration, LtiConfigurationAdmin)
 admin.site.register(LtiAgsLineItem)
 admin.site.register(LtiAgsScore)

lti_consumer/api.py

@@ -7,8 +7,10 @@
 
 import json
 
+from opaque_keys.edx.keys import CourseKey
+
 from .exceptions import LtiError
-from .models import LtiConfiguration, LtiDlContentItem
+from .models import CourseAllowPIISharingInLTIFlag, LtiConfiguration, LtiDlContentItem
 from .utils import (
     get_lti_deeplinking_content_url,
     get_lms_lti_keyset_link,
@@ -189,7 +191,20 @@ def get_deep_linking_data(deep_linking_id, config_id=None, block=None):
     # Retrieve LTI Configuration
     lti_config = _get_lti_config(config_id, block)
     # Only filter DL content item from content item set in the same LTI configuration.
-    # This avoid a malicious user to input a random LTI id and perform LTI DL
-    # content launches outsite the scope of it's configuration.
+    # This avoids a malicious user to input a random LTI id and perform LTI DL
+    # content launches outside the scope of its configuration.
     content_item = lti_config.ltidlcontentitem_set.get(pk=deep_linking_id)
     return content_item.attributes
+
+
+def get_lti_pii_sharing_state_for_course(course_key: CourseKey) -> bool:
+    """
+    Returns the status of PII sharing for the provided course.
+
+    Args:
+        course_key (CourseKey): Course key for the course to check for PII sharing
+
+    Returns:
+        bool: The state of PII sharing for this course for LTI.
+    """
+    return CourseAllowPIISharingInLTIFlag.current(course_key).enabled

lti_consumer/forms.py

@@ -7,19 +7,19 @@
 
 from django import forms
 
-from lti_consumer.models import CourseEditLTIFieldsEnabledFlag
+from lti_consumer.models import CourseAllowPIISharingInLTIFlag
 from lti_consumer.plugin.compat import clean_course_id
 
 log = logging.getLogger(__name__)
 
 
-class CourseEditLTIFieldsEnabledAdminForm(forms.ModelForm):
+class CourseAllowPIISharingInLTIAdminForm(forms.ModelForm):
     """
     Form for LTI consumer course-specific configuration to verify the course id.
     """
 
     class Meta:
-        model = CourseEditLTIFieldsEnabledFlag
+        model = CourseAllowPIISharingInLTIFlag
         fields = '__all__'
 
     def clean_course_id(self):

lti_consumer/migrations/0012_rename_courseeditltifieldsenabledflag_model.py

@@ -0,0 +1,19 @@
+# Generated by Django 2.2.24 on 2021-06-16 12:26
+
+from django.conf import settings
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        migrations.swappable_dependency(settings.AUTH_USER_MODEL),
+        ('lti_consumer', '0011_courseeditltifieldsenabledflag'),
+    ]
+
+    operations = [
+        migrations.RenameModel(
+            old_name='CourseEditLTIFieldsEnabledFlag',
+            new_name='CourseAllowPIISharingInLTIFlag',
+        ),
+    ]

lti_consumer/models.py

@@ -11,6 +11,7 @@
 from jsonfield import JSONField
 from Cryptodome.PublicKey import RSA
 from opaque_keys.edx.django.models import CourseKeyField, UsageKeyField
+from opaque_keys.edx.keys import CourseKey
 from config_models.models import ConfigurationModel
 
 # LTI 1.1
@@ -329,6 +330,22 @@ def get_lti_consumer(self):
 
         return self._get_lti_1p1_consumer()
 
+    @property
+    def pii_share_username(self):
+        return self.lti_config.get('pii_share_username', False)  # pylint: disable=no-member
+
+    @pii_share_username.setter
+    def pii_share_username(self, value):
+        self.lti_config['pii_share_username'] = value  # pylint: disable=unsupported-assignment-operation
+
+    @property
+    def pii_share_email(self):
+        return self.lti_config.get('pii_share_email', False)  # pylint: disable=no-member
+
+    @pii_share_email.setter
+    def pii_share_email(self, value):
+        self.lti_config['pii_share_email'] = value  # pylint: disable=unsupported-assignment-operation
+
     def __str__(self):
         return f"[{self.config_store}] {self.version} - {self.location}"
 
@@ -535,10 +552,14 @@ class Meta:
         app_label = 'lti_consumer'
 
 
-class CourseEditLTIFieldsEnabledFlag(ConfigurationModel):
+class CourseAllowPIISharingInLTIFlag(ConfigurationModel):
     """
-    Enables the editing of "request username" and "request email" fields
-    of LTI consumer for a specific course.
+    Enables the sharing of PII via LTI for the specific course.
+
+    Depending on where it's used, further action might be needed to actually
+    enable sharing PII. For instance, in the LTI XBlock setting this flag
+    will allow editing the "request username" and "request email" fields, which
+    will also need to be set to actually share PII.
 
     .. no_pii:
     """
@@ -548,7 +569,7 @@ class CourseEditLTIFieldsEnabledFlag(ConfigurationModel):
 
     @classmethod
     @request_cached
-    def lti_access_to_learners_editable(cls, course_id, is_already_sharing_learner_info):
+    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
         the feature that enables editing of "request username" and "request email"
@@ -563,13 +584,13 @@ def lti_access_to_learners_editable(cls, course_id, is_already_sharing_learner_i
             is_already_sharing_learner_info (bool): indicates whether LTI consumer is
             already sharing edX learner username/email.
         """
-        course_specific_config = (CourseEditLTIFieldsEnabledFlag.objects
+        course_specific_config = (CourseAllowPIISharingInLTIFlag.objects
                                   .filter(course_id=course_id)
                                   .order_by('-change_date')
                                   .first())
 
         if is_already_sharing_learner_info and not course_specific_config:
-            CourseEditLTIFieldsEnabledFlag.objects.create(course_id=course_id, enabled=True)
+            CourseAllowPIISharingInLTIFlag.objects.create(course_id=course_id, enabled=True)
             return True
 
         return course_specific_config.enabled if course_specific_config else False

lti_consumer/plugin/compat.py

@@ -14,24 +14,6 @@
 log = logging.getLogger(__name__)
 
 
-# Waffle flags configuration
-
-# Namespace
-WAFFLE_NAMESPACE = 'lti_consumer'
-
-# Course Waffle Flags
-# .. toggle_name: lti_consumer.lti_nrps_transmit_pii
-# .. toggle_implementation: CourseWaffleFlag
-# .. toggle_default: False
-# .. toggle_description: When enabled, the LTI NRPS endpoint will include learner PII
-#    in the response (username, email, etc).
-# .. toggle_use_cases: open_edx
-# .. toggle_creation_date: 2021-06-03
-# .. toggle_tickets: TNL-7974, https://github.com/edx/xblock-lti-consumer/pull/124
-# .. toggle_warnings: None.
-LTI_NRPS_TRANSMIT_PII = 'lti_nrps_transmit_pii'
-
-
 def run_xblock_handler(*args, **kwargs):
     """
     Import and run `handle_xblock_callback` from LMS
@@ -187,15 +169,6 @@ def get_course_members(course_key):
         raise LtiError('NRPS is not available for {}'.format(course_key)) from ex
 
 
-def get_lti_pii_course_waffle_flag():
-    """
-    Returns Course Waffle Flag Override for PII information exposure.
-    """
-    # pylint: disable=import-error,import-outside-toplevel
-    from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag
-    return CourseWaffleFlag(WAFFLE_NAMESPACE, LTI_NRPS_TRANSMIT_PII, __name__)
-
-
 def request_cached(func) -> Callable[[Callable], Callable]:
     """
     Import the `request_cached` decorator from LMS and apply it if available.

lti_consumer/plugin/views.py

@@ -19,6 +19,7 @@
 from rest_framework.response import Response
 from rest_framework.status import HTTP_403_FORBIDDEN
 
+from lti_consumer.api import get_lti_pii_sharing_state_for_course
 from lti_consumer.exceptions import LtiError
 from lti_consumer.models import (
     LtiConfiguration,
@@ -56,7 +57,7 @@
 )
 from lti_consumer.lti_1p3.extensions.rest_framework.utils import IgnoreContentNegotiation
 from lti_consumer.plugin import compat
-from lti_consumer.utils import _, expose_pii_fields
+from lti_consumer.utils import _
 
 
 log = logging.getLogger(__name__)
@@ -464,7 +465,7 @@ def get_serializer_class(self):
         Overrides ModelViewSet's `get_serializer_class` method.
         Checks if PII fields can be exposed and returns appropiate serializer.
         """
-        if expose_pii_fields(self.request.lti_configuration.location.course_key):
+        if get_lti_pii_sharing_state_for_course(self.request.lti_configuration.location.course_key):
             return LtiNrpsContextMembershipPIISerializer
         else:
             return LtiNrpsContextMembershipBasicSerializer

lti_consumer/tests/unit/plugin/test_views_lti_nrps.py

@@ -1,7 +1,7 @@
 """
 Tests for LTI Names and Role Provisioning Service views.
 """
-from mock import patch, PropertyMock
+from mock import Mock, patch, PropertyMock
 from Cryptodome.PublicKey import RSA
 from jwkest.jwk import RSAKey
 from rest_framework.test import APITransactionTestCase
@@ -226,12 +226,12 @@ def test_token_with_incorrect_scope(self):
         response = self.client.get(self.context_membership_endpoint)
         self.assertEqual(response.status_code, 403)
 
-    @patch('lti_consumer.plugin.views.expose_pii_fields', return_value=False)
+    @patch('lti_consumer.plugin.views.get_lti_pii_sharing_state_for_course', Mock(return_value=False))
     @patch(
         'lti_consumer.plugin.views.compat.get_course_members',
-        side_effect=patch_get_memberships()
+        Mock(side_effect=patch_get_memberships()),
     )
-    def test_token_with_correct_scope(self, get_course_members_patcher, expose_pii_fields_patcher):  # pylint: disable=unused-argument
+    def test_token_with_correct_scope(self):
         """
         Test if context membership returns correct response when token has correct scope
         """
@@ -240,14 +240,14 @@ def test_token_with_correct_scope(self, get_course_members_patcher, expose_pii_f
         self.assertEqual(response.status_code, 200)
         self.assertEqual(response['content-type'], 'application/vnd.ims.lti-nrps.v2.membershipcontainer+json')
 
-    @patch('lti_consumer.plugin.views.expose_pii_fields', return_value=False)
+    @patch('lti_consumer.plugin.views.get_lti_pii_sharing_state_for_course', return_value=False)
     @patch(
         'lti_consumer.plugin.views.compat.get_course_members',
-        side_effect=patch_get_memberships({
+        Mock(side_effect=patch_get_memberships({
             'student': 4
-        })
+        })),
     )
-    def test_get_without_pii(self, get_course_members_patcher, expose_pii_fields_patcher):  # pylint: disable=unused-argument
+    def test_get_without_pii(self, expose_pii_fields_patcher):
         """
         Test context membership endpoint response structure with PII not exposed.
         """
@@ -267,14 +267,14 @@ def test_get_without_pii(self, get_course_members_patcher, expose_pii_fields_pat
         self.assertNotIn('email', member_fields)
         self.assertNotIn('name', member_fields)
 
-    @patch('lti_consumer.plugin.views.expose_pii_fields', return_value=True)
+    @patch('lti_consumer.plugin.views.get_lti_pii_sharing_state_for_course', return_value=True)
     @patch(
         'lti_consumer.plugin.views.compat.get_course_members',
-        side_effect=patch_get_memberships({
+        Mock(side_effect=patch_get_memberships({
             'student': 4
-        })
+        })),
     )
-    def test_get_with_pii(self, get_course_members_patcher, expose_pii_fields_patcher):  # pylint: disable=unused-argument
+    def test_get_with_pii(self, expose_pii_fields_patcher):
         """
         Test context membership endpoint response structure with PII exposed.
         """
@@ -295,14 +295,14 @@ def test_get_with_pii(self, get_course_members_patcher, expose_pii_fields_patche
         self.assertIn('email', member_fields)
         self.assertIn('name', member_fields)
 
-    @patch('lti_consumer.plugin.views.expose_pii_fields', return_value=False)
+    @patch('lti_consumer.plugin.views.get_lti_pii_sharing_state_for_course', Mock(return_value=False))
     @patch(
         'lti_consumer.plugin.views.compat.get_course_members',
-        side_effect=patch_get_memberships({
+        Mock(side_effect=patch_get_memberships({
             'exception': True
-        })
+        })),
     )
-    def test_enrollment_limit_gate(self, get_course_members_patcher, expose_pii_fields_patcher):  # pylint: disable=unused-argument
+    def test_enrollment_limit_gate(self):
         """
         Test if number of enrolled user is larger than the limit, api returns 404 response.
         """