From a74bcce8bbf34425999475b996489aad00c50375 Mon Sep 17 00:00:00 2001 From: Zachary Trabookis Date: Wed, 27 Sep 2023 16:16:07 -0400 Subject: [PATCH 1/3] Make sure that the `lti_launch` view is not restricted by `X_FRAME_OPTIONS`. One of our customers `Securus` was having `X-Frame-Options` set to `sameorigin`. In order to prevent this restriction we're adding the decorator to except this restriction. **Decorators Used** https://github.com/django/django/blob/3c447b108ac70757001171f7a4791f493880bf5b/django/views/decorators/clickjacking.py#L46-L54 **EdX Documentation on Clickjacking** https://openedx.atlassian.net/wiki/spaces/AC/pages/144441658/Clickjacking **Django Documentation on Clickjacking** https://docs.djangoproject.com/en/3.2/ref/clickjacking/ --- lms/djangoapps/lti_provider/views.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lms/djangoapps/lti_provider/views.py b/lms/djangoapps/lti_provider/views.py index ab1774204809..11e359b9751e 100644 --- a/lms/djangoapps/lti_provider/views.py +++ b/lms/djangoapps/lti_provider/views.py @@ -7,6 +7,7 @@ from django.conf import settings from django.http import Http404, HttpResponseBadRequest, HttpResponseForbidden +from django.views.decorators.clickjacking import xframe_options_exempt from django.views.decorators.csrf import csrf_exempt from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey @@ -38,6 +39,7 @@ @csrf_exempt @add_p3p_header +@xframe_options_exempt def lti_launch(request, course_id, usage_id): """ Endpoint for all requests to embed edX content via the LTI protocol. This From d25ea7d1998d6892125e456191e7a48657a97038 Mon Sep 17 00:00:00 2001 From: Arunmozhi Date: Thu, 21 Sep 2023 17:50:41 +0530 Subject: [PATCH 2/3] feat: adds auto_link_users_using_email field to LtiConsumer --- ..._lticonsumer_auto_link_users_using_email.py | 18 ++++++++++++++++++ lms/djangoapps/lti_provider/models.py | 1 + 2 files changed, 19 insertions(+) create mode 100644 lms/djangoapps/lti_provider/migrations/0004_lticonsumer_auto_link_users_using_email.py diff --git a/lms/djangoapps/lti_provider/migrations/0004_lticonsumer_auto_link_users_using_email.py b/lms/djangoapps/lti_provider/migrations/0004_lticonsumer_auto_link_users_using_email.py new file mode 100644 index 000000000000..2ea7f88244ce --- /dev/null +++ b/lms/djangoapps/lti_provider/migrations/0004_lticonsumer_auto_link_users_using_email.py @@ -0,0 +1,18 @@ +# Generated by Django 3.2.21 on 2023-09-21 12:18 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('lti_provider', '0003_auto_20161118_1040'), + ] + + operations = [ + migrations.AddField( + model_name='lticonsumer', + name='auto_link_users_using_email', + field=models.BooleanField(blank=True, default=False), + ), + ] diff --git a/lms/djangoapps/lti_provider/models.py b/lms/djangoapps/lti_provider/models.py index 0cb165eeef69..93bde45d63bd 100644 --- a/lms/djangoapps/lti_provider/models.py +++ b/lms/djangoapps/lti_provider/models.py @@ -34,6 +34,7 @@ class LtiConsumer(models.Model): consumer_key = models.CharField(max_length=32, unique=True, db_index=True, default=short_token) consumer_secret = models.CharField(max_length=32, unique=True, default=short_token) instance_guid = CharNullField(max_length=255, blank=True, null=True, unique=True) + auto_link_users_using_email = models.BooleanField(blank=True, default=False) @staticmethod def get_or_supplement(instance_guid, consumer_key): From f1f9fc0fb14e836ef6eed30f1a92f4c12d0ec4d3 Mon Sep 17 00:00:00 2001 From: Zachary Trabookis Date: Wed, 27 Sep 2023 16:23:39 -0400 Subject: [PATCH 3/3] feat: link existing users to the LtiUser via email With this change the platform users who access content via LTI will automatically be linked their platform account instead of the anonymous account when the following conditions are met: * the LtiConsumer should be configured to auto link the users via email * the LTI Consumer should share the email of the user using the lis_person_contact_email_primary parameter in the LTI Launch POST data Internal-ref: https://tasks.opencraft.com/browse/BB-7875 --- .../lti_provider/tests/test_users.py | 26 +++++++-- lms/djangoapps/lti_provider/users.py | 54 +++++++++++-------- 2 files changed, 53 insertions(+), 27 deletions(-) diff --git a/lms/djangoapps/lti_provider/tests/test_users.py b/lms/djangoapps/lti_provider/tests/test_users.py index d945f6b9a65a..867d4874e27a 100644 --- a/lms/djangoapps/lti_provider/tests/test_users.py +++ b/lms/djangoapps/lti_provider/tests/test_users.py @@ -112,7 +112,7 @@ def test_authentication_with_new_user(self, _create_user, switch_user): lti_user.edx_user_id = self.edx_user_id with patch('lms.djangoapps.lti_provider.users.create_lti_user', return_value=lti_user) as create_user: users.authenticate_lti_user(self.request, self.lti_user_id, self.lti_consumer) - create_user.assert_called_with(self.lti_user_id, self.lti_consumer) + create_user.assert_called_with(self.lti_user_id, self.lti_consumer, "") switch_user.assert_called_with(self.request, lti_user, self.lti_consumer) def test_authentication_with_authenticated_user(self, create_user, switch_user): @@ -140,6 +140,18 @@ def test_authentication_with_wrong_user(self, create_user, switch_user): assert not create_user.called switch_user.assert_called_with(self.request, lti_user, self.lti_consumer) + def test_auto_linking_of_users_using_lis_person_contact_email_primary(self, create_user, switch_user): + request = RequestFactory().post("/", {"lis_person_contact_email_primary": self.old_user.email}) + request.user = self.old_user + + users.authenticate_lti_user(request, self.lti_user_id, self.lti_consumer) + create_user.assert_called_with(self.lti_user_id, self.lti_consumer, "") + + self.lti_consumer.auto_link_users_using_email = True + self.lti_consumer.save() + users.authenticate_lti_user(request, self.lti_user_id, self.lti_consumer) + create_user.assert_called_with(self.lti_user_id, self.lti_consumer, self.old_user.email) + class CreateLtiUserTest(TestCase): """ @@ -154,16 +166,17 @@ def setUp(self): consumer_secret='TestSecret' ) self.lti_consumer.save() + self.existing_user = UserFactory.create() def test_create_lti_user_creates_auth_user_model(self): users.create_lti_user('lti_user_id', self.lti_consumer) - assert User.objects.count() == 1 + assert User.objects.count() == 2 @patch('uuid.uuid4', return_value='random_uuid') @patch('lms.djangoapps.lti_provider.users.generate_random_edx_username', return_value='edx_id') def test_create_lti_user_creates_correct_user(self, uuid_mock, _username_mock): users.create_lti_user('lti_user_id', self.lti_consumer) - assert User.objects.count() == 1 + assert User.objects.count() == 2 user = User.objects.get(username='edx_id') assert user.email == 'edx_id@lti.example.com' uuid_mock.assert_called_with() @@ -173,10 +186,15 @@ def test_unique_username_created(self, username_mock): User(username='edx_id').save() users.create_lti_user('lti_user_id', self.lti_consumer) assert username_mock.call_count == 2 - assert User.objects.count() == 2 + assert User.objects.count() == 3 user = User.objects.get(username='new_edx_id') assert user.email == 'new_edx_id@lti.example.com' + def test_existing_user_is_linked(self): + lti_user = users.create_lti_user('lti_user_id', self.lti_consumer, self.existing_user.email) + assert lti_user.lti_consumer == self.lti_consumer + assert lti_user.edx_user == self.existing_user + class LtiBackendTest(TestCase): """ diff --git a/lms/djangoapps/lti_provider/users.py b/lms/djangoapps/lti_provider/users.py index c9435854e9b4..51e5ecc1e70e 100644 --- a/lms/djangoapps/lti_provider/users.py +++ b/lms/djangoapps/lti_provider/users.py @@ -38,7 +38,10 @@ def authenticate_lti_user(request, lti_user_id, lti_consumer): ) except LtiUser.DoesNotExist: # This is the first time that the user has been here. Create an account. - lti_user = create_lti_user(lti_user_id, lti_consumer) + lis_email = "" + if lti_consumer.auto_link_users_using_email: + lis_email = request.POST.get("lis_person_contact_email_primary", "") + lti_user = create_lti_user(lti_user_id, lti_consumer, lis_email) log.info( "authenticate_lti_user %s", lti_user @@ -51,34 +54,39 @@ def authenticate_lti_user(request, lti_user_id, lti_consumer): switch_user(request, lti_user, lti_consumer) -def create_lti_user(lti_user_id, lti_consumer): +def create_lti_user(lti_user_id, lti_consumer, email=""): """ Generate a new user on the edX platform with a random username and password, and associates that account with the LTI identity. """ edx_password = str(uuid.uuid4()) - created = False - while not created: - try: - edx_user_id = generate_random_edx_username() - edx_email = f"{edx_user_id}@{settings.LTI_USER_EMAIL_DOMAIN}" - with transaction.atomic(): - edx_user = User.objects.create_user( - username=edx_user_id, - password=edx_password, - email=edx_email, - ) - # A profile is required if PREVENT_CONCURRENT_LOGINS flag is set. - # TODO: We could populate user information from the LTI launch here, - # but it's not necessary for our current uses. - edx_user_profile = UserProfile(user=edx_user) - edx_user_profile.save() - created = True - except IntegrityError: - # The random edx_user_id wasn't unique. Since 'created' is still - # False, we will retry with a different random ID. - pass + existing_user = User.objects.filter(email=email).first() if email else None + + if existing_user: + edx_user = existing_user + else: + created = False + while not created: + try: + edx_user_id = generate_random_edx_username() + edx_email = f"{edx_user_id}@{settings.LTI_USER_EMAIL_DOMAIN}" + with transaction.atomic(): + edx_user = User.objects.create_user( + username=edx_user_id, + password=edx_password, + email=edx_email, + ) + # A profile is required if PREVENT_CONCURRENT_LOGINS flag is set. + # TODO: We could populate user information from the LTI launch here, + # but it's not necessary for our current uses. + edx_user_profile = UserProfile(user=edx_user) + edx_user_profile.save() + created = True + except IntegrityError: + # The random edx_user_id wasn't unique. Since 'created' is still + # False, we will retry with a different random ID. + pass lti_user = LtiUser( lti_consumer=lti_consumer,