From 6fb867938f4e6063af864260aceddfb0f3ae5d13 Mon Sep 17 00:00:00 2001
From: Kshitij Sobti <kshitij@sobti.in>
Date: Wed, 16 Jun 2021 18:01:51 +0530
Subject: [PATCH] refactor: Rename CourseEditLTIFieldsEnabledFlag to
CourseAllowPIISharingInLTIFlag and use it for LTI1.3 This commit renames the
CourseEditLTIFieldsEnabledFlag to CourseAllowPIISharingInLTIFlag since the
aim is to expand its scope to all LTI-related PII sharing. It also removes
the current LTI1.3 waffle flag for PII sharing.
---
README.rst | 12 +++++++
lti_consumer/admin.py | 12 +++----
lti_consumer/api.py | 21 ++++++++++--
lti_consumer/forms.py | 6 ++--
...me_courseeditltifieldsenabledflag_model.py | 19 +++++++++++
lti_consumer/models.py | 33 +++++++++++++++----
lti_consumer/plugin/compat.py | 27 ---------------
lti_consumer/plugin/views.py | 5 +--
.../tests/unit/plugin/test_views_lti_nrps.py | 32 +++++++++---------
lti_consumer/tests/unit/test_models.py | 14 ++++----
lti_consumer/utils.py | 12 -------
setup.py | 2 +-
12 files changed, 112 insertions(+), 83 deletions(-)
create mode 100644 lti_consumer/migrations/0012_rename_courseeditltifieldsenabledflag_model.py
diff --git a/README.rst b/README.rst
index f571c2c..dfbcb5d 100644
--- a/README.rst
+++ b/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.
diff --git a/lti_consumer/admin.py b/lti_consumer/admin.py
index c5fae56..c6360bb 100644
--- a/lti_consumer/admin.py
+++ b/lti_consumer/admin.py
@@ -4,9 +4,9 @@ Admin views for LTI related models.
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)
diff --git a/lti_consumer/api.py b/lti_consumer/api.py
index 9958ecc..ea552b8 100644
--- a/lti_consumer/api.py
+++ b/lti_consumer/api.py
@@ -7,8 +7,10 @@ return plaintext to allow easy testing/mocking.
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
diff --git a/lti_consumer/forms.py b/lti_consumer/forms.py
index 7ea866a..c4d8bbe 100644
--- a/lti_consumer/forms.py
+++ b/lti_consumer/forms.py
@@ -7,19 +7,19 @@ import logging
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):
diff --git a/lti_consumer/migrations/0012_rename_courseeditltifieldsenabledflag_model.py b/lti_consumer/migrations/0012_rename_courseeditltifieldsenabledflag_model.py
new file mode 100644
index 0000000..1a86777
--- /dev/null
+++ b/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',
+ ),
+ ]
diff --git a/lti_consumer/models.py b/lti_consumer/models.py
index dec339e..92e9701 100644
--- a/lti_consumer/models.py
+++ b/lti_consumer/models.py
@@ -11,6 +11,7 @@ from django.utils.translation import ugettext_lazy as _
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 @@ class LtiConfiguration(models.Model):
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 LtiDlContentItem(models.Model):
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 @@ class CourseEditLTIFieldsEnabledFlag(ConfigurationModel):
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
diff --git a/lti_consumer/plugin/compat.py b/lti_consumer/plugin/compat.py
index 588f819..6c9ed1f 100644
--- a/lti_consumer/plugin/compat.py
+++ b/lti_consumer/plugin/compat.py
@@ -14,24 +14,6 @@ from lti_consumer.exceptions import LtiError
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.
diff --git a/lti_consumer/plugin/views.py b/lti_consumer/plugin/views.py
index 38bad76..c5901d9 100644
--- a/lti_consumer/plugin/views.py
+++ b/lti_consumer/plugin/views.py
@@ -19,6 +19,7 @@ from rest_framework.decorators import action
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.parsers import (
)
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 @@ class LtiNrpsContextMembershipViewSet(viewsets.ReadOnlyModelViewSet):
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
diff --git a/lti_consumer/tests/unit/plugin/test_views_lti_nrps.py b/lti_consumer/tests/unit/plugin/test_views_lti_nrps.py
index 323d5b2..6b13c91 100644
--- a/lti_consumer/tests/unit/plugin/test_views_lti_nrps.py
+++ b/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 @@ class LtiNrpsContextMembershipViewsetTestCase(LtiNrpsTestCase):
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 @@ class LtiNrpsContextMembershipViewsetTestCase(LtiNrpsTestCase):
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 @@ class LtiNrpsContextMembershipViewsetTestCase(LtiNrpsTestCase):
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 @@ class LtiNrpsContextMembershipViewsetTestCase(LtiNrpsTestCase):
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.
"""
diff --git a/lti_consumer/tests/unit/test_models.py b/lti_consumer/tests/unit/test_models.py
index b6f3b9a..be7a614 100644
--- a/lti_consumer/tests/unit/test_models.py
+++ b/lti_consumer/tests/unit/test_models.py
@@ -17,7 +17,7 @@ from opaque_keys.edx.locator import CourseLocator
from lti_consumer.lti_1p1.consumer import LtiConsumer1p1
from lti_consumer.lti_xblock import LtiConsumerXBlock
from lti_consumer.models import (
- CourseEditLTIFieldsEnabledFlag,
+ CourseAllowPIISharingInLTIFlag,
LtiAgsLineItem,
LtiAgsScore,
LtiConfiguration,
@@ -371,7 +371,7 @@ def lti_consumer_fields_editing_flag(course_id, enabled_for_course=False):
enabled_for_course (bool): whether feature is enabled for 'course_id'
"""
RequestCache.clear_all_namespaces()
- CourseEditLTIFieldsEnabledFlag.objects.create(course_id=course_id, enabled=enabled_for_course)
+ CourseAllowPIISharingInLTIFlag.objects.create(course_id=course_id, enabled=enabled_for_course)
yield
@@ -403,7 +403,7 @@ class TestLTIConsumerHideFieldsFlag(TestCase):
course_id=self.course_id,
enabled_for_course=enabled_for_course
):
- feature_enabled = CourseEditLTIFieldsEnabledFlag.lti_access_to_learners_editable(
+ feature_enabled = CourseAllowPIISharingInLTIFlag.lti_access_to_learners_editable(
self.course_id,
is_already_sharing_learner_info,
)
@@ -418,11 +418,11 @@ class TestLTIConsumerHideFieldsFlag(TestCase):
This tests the backward compatibility which currently is: if an existing course run is already
sharing learner information then this feature should be enabled for that course run by default.
"""
- feature_enabled = CourseEditLTIFieldsEnabledFlag.lti_access_to_learners_editable(
+ feature_enabled = CourseAllowPIISharingInLTIFlag.lti_access_to_learners_editable(
self.course_id,
is_already_sharing_learner_info,
)
- feature_flag_created = CourseEditLTIFieldsEnabledFlag.objects.filter(course_id=self.course_id).exists()
+ feature_flag_created = CourseAllowPIISharingInLTIFlag.objects.filter(course_id=self.course_id).exists()
self.assertEqual(feature_flag_created, is_already_sharing_learner_info)
self.assertEqual(feature_enabled, is_already_sharing_learner_info)
@@ -434,10 +434,10 @@ class TestLTIConsumerHideFieldsFlag(TestCase):
course_id=self.course_id,
enabled_for_course=True
):
- self.assertTrue(CourseEditLTIFieldsEnabledFlag.lti_access_to_learners_editable(self.course_id, False))
+ self.assertTrue(CourseAllowPIISharingInLTIFlag.lti_access_to_learners_editable(self.course_id, False))
with lti_consumer_fields_editing_flag(
course_id=self.course_id,
enabled_for_course=False
):
- self.assertFalse(CourseEditLTIFieldsEnabledFlag.lti_access_to_learners_editable(self.course_id, False))
+ self.assertFalse(CourseAllowPIISharingInLTIFlag.lti_access_to_learners_editable(self.course_id, False))
diff --git a/lti_consumer/utils.py b/lti_consumer/utils.py
index 2b652b3..e81f177 100644
--- a/lti_consumer/utils.py
+++ b/lti_consumer/utils.py
@@ -2,7 +2,6 @@
Utility functions for LTI Consumer block
"""
from django.conf import settings
-from lti_consumer.plugin.compat import get_lti_pii_course_waffle_flag
def _(text):
@@ -12,17 +11,6 @@ def _(text):
return text
-def expose_pii_fields(course_key):
- """
- Returns `true` if Use's PII fields can be exposed to LTI endpoints
- for given course key. ex - LTI-NRPS Context Membership Endpoint.
-
- Args:
- course_key
- """
- return get_lti_pii_course_waffle_flag().is_enabled(course_key)
-
-
def get_lms_base():
"""
Returns LMS base url to be used as issuer on OAuth2 flows
diff --git a/setup.py b/setup.py
index 38a2ad4..597bb10 100644
--- a/setup.py
+++ b/setup.py
@@ -49,7 +49,7 @@ with open('README.rst') as _f:
setup(
name='lti-consumer-xblock',
- version='2.11.0',
+ version='3.0.0',
author='Open edX project',
author_email='oscm@edx.org',
description='This XBlock implements the consumer side of the LTI specification.',
--
GitLab