Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make lis_person_contact_email_primary matching case-insensitive (LTI Providers) #34688

Merged

Conversation

arslanashraf7
Copy link
Contributor

@arslanashraf7 arslanashraf7 commented May 3, 2024

Related Ticket:

https://github.com/mitodl/hq/issues/3128

Description

The edX user's email matching with the LTI providers doesn't check for email case insensitivity. Hence, It doesn't properly create and associate the LTI users with edX users. In return, the PermissionDenied exception is thrown every time and the users are requested to re-login even when they already have.

Useful information to include:

  • This impacts the learners, interacting with LTI content.

The problem:

When a user tries to access edX content on LTI consumer e.g. Canvas they are always asked to re-login every time even though they have the same email ID on both ends. This can happen when edX can't find a user against the LTI user in the system even though the emails are the same with only case-sensitive differences.

Supporting information

Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.

Testing instructions

If you are able to configure the LTI locally:

  1. Create an account, enroll in a course
  2. Add content Unit(s) in the course
  3. Use this LTI content in the LTI
  4. Try to access this content through LTI
  5. A new LTI user should be created in edX and it should be associated with the user in edX
  6. Change your LTI consumer to send lis_person_contact_email_primary in all caps or mixed small/capital characters. Idea is to test the email case insensitivity.
  7. If this is an existing user, they should be able to load the content, if new user then a new LtiUser entry should be created with proper association with edX user

Please provide detailed step-by-step instructions for testing this change.

Deadline

None

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label May 3, 2024
@openedx-webhooks
Copy link

Thanks for the pull request, @arslanashraf7! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@arslanashraf7 arslanashraf7 changed the title fix: make lis_email matching case-insensitive fix: make lis_person_contact_email_primary matching case-insensitive (LTI Providers) May 3, 2024
@mphilbrick211
Copy link

Hi @arslanashraf7! Just flagging that there's some failing checks here.

@arslanashraf7 arslanashraf7 force-pushed the arslan/canvas-case-insensitve-email branch from b1acfc2 to 0dd7fd9 Compare May 6, 2024 11:49
@arslanashraf7
Copy link
Contributor Author

@mphilbrick211 The checks are all green now. However, I was reading through https://discuss.openedx.org/t/read-me-community-pull-request-process-updates-improvements/12709 and I think that this PR doesn't require product review but I just wanted to double check. Do you think this PR might require the product review?

Comment on lines 43 to 44
if request.user.is_authenticated and request.user.email.lower() == lis_email.lower():
lti_user = create_lti_user(lti_user_id, lti_consumer, lis_email)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if request.user.is_authenticated and request.user.email.lower() == lis_email.lower():
lti_user = create_lti_user(lti_user_id, lti_consumer, lis_email)
if request.user.is_authenticated and request.user.email.lower() == lis_email.lower():
lti_user = create_lti_user(lti_user_id, lti_consumer, request.user.email)

From a security and functionality perspective, we should pass request.user.email to the create_lti_user function to ensure consistency and avoid potential security issues related to email case sensitivity.

Please see the following line of code:
https://github.com/openedx/edx-platform/blob/0dd7fd9f83eacad432460da4eac274ae0dc10a14/lms/djangoapps/lti_provider/users.py#L63

If the underlying database uses a case-sensitive collation for storing email addresses (as might be the case with certain MySQL configurations) or if the system uses PostgreSQL (which treats identifiers as case-sensitive unless quoted):

  1. A new account could be erroneously created because User.objects.filter would not return any account when the case does not match.
  2. If we changed the filter to case-insensitive, an attacker could exploit this by creating or logging in with an email address with different letter cases, potentially leading to account hijacking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Agrendalath Thanks that's a good catch! I agree that this could have been a problem in case-sensitive collation.

I've applied your suggestions.

@arslanashraf7 arslanashraf7 force-pushed the arslan/canvas-case-insensitve-email branch from c117577 to 18e6ddf Compare May 17, 2024 08:43
@arslanashraf7 arslanashraf7 force-pushed the arslan/canvas-case-insensitve-email branch from 18e6ddf to ce92ae0 Compare May 17, 2024 08:47
@arslanashraf7 arslanashraf7 requested a review from Agrendalath May 17, 2024 10:10
Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

  • I tested this: checked that the case-sensitivity of the email provided by the LTI Provider does not prevent the currently logged-in user from being linked
  • I read through the code
  • I checked for accessibility issues: n/a
  • Includes documentation: n/a

@Agrendalath Agrendalath merged commit 9c485dd into openedx:master May 17, 2024
47 checks passed
@openedx-webhooks
Copy link

@arslanashraf7 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@arslanashraf7 arslanashraf7 deleted the arslan/canvas-case-insensitve-email branch May 21, 2024 09:33
@pdpinch
Copy link
Contributor

pdpinch commented May 22, 2024

@arslanashraf7 please backport this to Redwood

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants