-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
fix: make lis_person_contact_email_primary
matching case-insensitive (LTI Providers)
#34688
Conversation
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:
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. |
lis_person_contact_email_primary
matching case-insensitive (LTI Providers)
Hi @arslanashraf7! Just flagging that there's some failing checks here. |
b1acfc2
to
0dd7fd9
Compare
@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? |
lms/djangoapps/lti_provider/users.py
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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):
- A new account could be erroneously created because
User.objects.filter
would not return any account when the case does not match. - 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.
There was a problem hiding this comment.
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.
c117577
to
18e6ddf
Compare
18e6ddf
to
ce92ae0
Compare
There was a problem hiding this 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
@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. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
@arslanashraf7 please backport this to Redwood |
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:
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:
lis_person_contact_email_primary
in all caps or mixed small/capital characters. Idea is to test the email case insensitivity.Please provide detailed step-by-step instructions for testing this change.
Deadline
None