[BUG FIX] [MER-3619] Fix an issue where account creation silently fails in certain cases #5108
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eliknebel
commented
Sep 18, 2024
| end | ||
| end) | ||
| end | ||
| do: "Continue with #{Naming.humanize(conn.params["provider"])}" |
There was a problem hiding this comment.
This was some odd code I stumbled upon that clearly intended to be a simple string interpolation, so I simplified it
eliknebel
commented
Sep 18, 2024
|
|
||
|
|
||
|
|
||
| <table align="center" role="presentation" cellspacing="0" cellpadding="0" border="0" width="100%" style="margin: auto;"> |
There was a problem hiding this comment.
Email clients typically don't render html in the same sane way that browsers do, further they certainly do not have access to tailwind classes. I updated this template to be email friendly like the other templates and better match the typical email style since this in particular is an email we want a user to see as authentic
eliknebel
changed the title
darrensiegel
approved these changes
Sep 18, 2024
darrensiegel
deleted the
MER-3619-Delivery-Creating-Direct-Delivery-Independent-Accounts-Fails-Silently-if-an-LTI-Account-Exists-for-the-Same-Email-Address
branch
eliknebel
added a commit
that referenced
this pull request
Sep 24, 2024
* delay submission to ensure all save state calls have finished * set version * reduce deferred period * disable all activity inputs * Auto format * [BUG FIX] [MER-3775] Allow embedded page links to open (#5092) * account for unordered pages * use page_context instead of current_page * [CHORE] [MER-3786] allow setting of the interval and target for db_connection (#5093) * [BUG FIX] [MER-3797] Fix Youtube XAPI emitting (#5099) * directly provide atempt guid * Auto format --------- Co-authored-by: darrensiegel <darrensiegel@users.noreply.github.com> * bump version number * bump version number * [BUG FIX] [MER-3823] Convert numeric to text input (#5109) * convert numeric to text input * cleanup * Auto format * change regex to allow 300. and .300 as valid numbers * allow same numbers in answer authoring as in delivery --------- Co-authored-by: darrensiegel <darrensiegel@users.noreply.github.com> Co-authored-by: Anders Weinstein <andersw@cs.cmu.edu> * [BUG FIX] [MER-3619] Fix an issue where account creation silently fails in certain cases (#5108) * fix an issue where account creation was not properly filtering out LTI accounts * remove redundant clause * Auto format --------- Co-authored-by: eliknebel <eliknebel@users.noreply.github.com> * fix pow_test * fix formatting --------- Co-authored-by: Darren Siegel <siegel.darren@gmail.com> Co-authored-by: darrensiegel <darrensiegel@users.noreply.github.com> Co-authored-by: Anders Weinstein <andersw@cs.cmu.edu> Co-authored-by: eliknebel <eliknebel@users.noreply.github.com>
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://eliterate.atlassian.net/browse/MER-3619
This PR fixes 2 issues related to LTI and direct delivery accounts being intermingled if they have the same email address.
The issue is that the system is not properly constraining the existing account checks to
independent_learner: truein 2 important places in the code:lib/oli_web/pow/user_context.exlib/oli_web/pow/user_identities_context.exThis is preventing torus from creating a new account when one already exists in the LTI domain with the same email.
Further, my suspicion is that the callback to user_identities_context is being triggered during an LTI launch. This callback pattern matches on “email_verified: true” assuming that only social login providers can meet that condition, however on LTI launch this email_verified is explicity set to true before the Pow session is created, effectively triggering this callback codepath for every LTI launch in. Furthermore, when this condition is met, an upsert is performed with the LTI details, which effectively converts the direct delivery user to an LTI user.I verified this is not the case. However, I have left in the change here to ensure this function only runs for independent learners, as we never want a user to be able to add a social login capability to their existing LMS account. It could be that this was part of the confusion with the one-off account mentioned in the ticket.I also updated the email template here to be better client compatible (tailwind is not available in email renderers) and match the existing email styles. This email message especially should match the rest of the email styles to not help assure a user that the email is valid.
One other note to make here: I noticed that the identification of a unique LTI account relies solely on the uniqueness of a given "sub" field. In most (if not all) cases, this is a UUID supplied by an LMS. However, because this is technically outside of our control, it would be better to constrain this
insert_or_update_lms_userfunction to also only consider users for a given institution. I didn't make this change here though, as it seemed a bit more risky. I filed a followup ticket https://eliterate.atlassian.net/browse/MER-3824