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.

Sign up for GitHub

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

Check for LMS ID first when authenticating from LMS #2645

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Check for LMS ID first when authenticating from LMS #2645

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
69fba59
Check for LMS ID first when authenticating from LMS
Alex-Jordan Dec 17, 2024
8eab14f
Suggestions to make using an existing user with given LMS id work.
drgrice1 Dec 18, 2024
828fb19
Merge pull request #33 from drgrice1/lti-userids-suggestion
Alex-Jordan Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 39 additions & 21 deletions lib/WeBWorK/Authen/LTIAdvantage.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,21 +132,14 @@ sub get_credentials ($self) {
return 0;
}

# Determine the user_id to use, if possible.
if (!$ce->{LTI}{v1p3}{preferred_source_of_username}) {
warn 'LTI is not properly configured (no preferred_source_of_username). '
. "Please contact your instructor or system administrator.\n";
$self->{error} = $c->maketext(
'There was an error during the login process. Please speak to your instructor or system administrator.');
debug("No preferred_source_of_username in $ce->{courseName} so LTIAdvantage::get_credentials is returning 0.");
return 0;
}
# First check if we already have a user with the current lis_source_did
my $user = ($c->db->getUsersWhere({ lis_source_did => $c->stash->{lti_lms_user_id} }))[0] // ''
if $c->stash->{lti_lms_user_id};
Copy link
Member

Choose a reason for hiding this comment

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

The $c->stash->{lti_lms_user_id} can never be set at this point. It isn't set until line 236 later in this same method. You will need to use $claims->{sub} at this point instead.


my $user_id;
my $user_id_source = '';
my $type_of_source = '';

$self->{email} = $claims->{email} // '';

my $extract_claim = sub ($key) {
my $value = $claims;
for (split '#', $key) {
Expand All @@ -159,19 +152,42 @@ sub get_credentials ($self) {
return $value;
};

if (my $user_id = $extract_claim->($ce->{LTI}{v1p3}{preferred_source_of_username})) {
$user_id_source = $ce->{LTI}{v1p3}{preferred_source_of_username};
$type_of_source = 'preferred_source_of_username';
$self->{user_id} = $user_id;
}
if ($user) {
$user_id_source = $c->stash->{lti_lms_user_id};
$type_of_source = 'lis_source_did';
Copy link
Member

Choose a reason for hiding this comment

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

This really shouldn't be called the lis_source_did. That isn't what it is. It is the sub claim which is a stable locally unique identifier for the user in the LMS. See https://www.imsglobal.org/spec/lti/v1p3#message-claims. Nowhere in the LTI 1.3 specification do they even refer to an lis_sourced_id. That is a LTI 1.1 thing.

Copy link
Member

Choose a reason for hiding this comment

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

I think that the $type_of_source should be "database user" in this case. Note that this is only used for the debugging message on line 217 below, so it can contain spaces and be informative. The type of source means the source of the webwork2 user_id. That is from the user_id column of the user table in the database in this case, and not the misnamed lis_source_did column.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, looking at this closer I also see that $user_id_source = $c->stash->{lti_lms_user_id} is not good. The values of $user_id_source and $type_of_source need to be set to something that will work well in the sentence "User id is |$self->{user_id}| (obtained from $user_id_source which was $type_of_source)\n". That is all that those variables are used for (other than the usage of $user_id_source in the case that it is equal to "email").

To be honest, I don't think anything will work in that sentence well in this case. So instead I think the $type_of_source should just be set to "database user" in this case, and "$user_id_source which was ..." in the other two cases. Then the debugging message changed to "User id is |$self->{user_id}| (obtained from $type_of_source)\n";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, what I was doing here was the best I could think of for User id is ____ (obtained from ____ which was ____). Limiting to that structure, User id is ____ (obtained from lis_source_did which was xxx-yyy-zzz) seemed OK, assuming someone reading the log would know what that means. But having a different sentence altogether is better.

$self->{user_id} = $user->user_id;
} else {
# Determine the user_id to use, if possible.
if (!$ce->{LTI}{v1p3}{preferred_source_of_username}) {
warn 'LTI is not properly configured (no preferred_source_of_username). '
. "Please contact your instructor or system administrator.\n";
$self->{error} = $c->maketext(
'There was an error during the login process. Please speak to your instructor or system administrator.'
);
debug(
"No preferred_source_of_username in $ce->{courseName} so LTIAdvantage::get_credentials is returning 0."
);
return 0;
}

if ($user_id = $extract_claim->($ce->{LTI}{v1p3}{preferred_source_of_username})) {
$user_id_source = $ce->{LTI}{v1p3}{preferred_source_of_username};
$type_of_source = 'preferred_source_of_username';
$self->{user_id} = $user_id;
}

# Fallback if necessary
if (!defined $self->{user_id} && (my $user_id = $extract_claim->($ce->{LTI}{v1p3}{fallback_source_of_username}))) {
$user_id_source = $ce->{LTI}{v1p3}{fallback_source_of_username};
$type_of_source = 'fallback_source_of_username';
$self->{user_id} = $user_id;
# Fallback if necessary
if (!defined $self->{user_id}
&& (my $user_id = $extract_claim->($ce->{LTI}{v1p3}{fallback_source_of_username})))
{
$user_id_source = $ce->{LTI}{v1p3}{fallback_source_of_username};
$type_of_source = 'fallback_source_of_username';
$self->{user_id} = $user_id;
}
}

$self->{email} = $claims->{email} // '';

if ($self->{user_id}) {
# Strip off the part of the address after @ if the email address was used and it was requested to do so.
$self->{user_id} =~ s/@.*$// if $user_id_source eq 'email' && $ce->{LTI}{v1p3}{strip_domain_from_email};
Expand All @@ -188,6 +204,8 @@ sub get_credentials ($self) {
[ recitation => 'https://purl.imsglobal.org/spec/lti/claim/custom#recitation' ],
);

$self->{lis_source_did} = $c->stash->{lti_lms_user_id} if $c->stash->{lti_lms_user_id};
Copy link
Member

Choose a reason for hiding this comment

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

Why is this set? It will never be needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was my attempt at recording the LMS ID as a new user is created. I didn't track the code, but I assumed the previous block was setting similar things like first_name, and that be setting this here it would trickle down to the lis_source_did column being filled out for the new user. I'm not surprised if that's wrong, but I couldn't test.


$self->{student_id} =
$ce->{LTI}{v1p3}{preferred_source_of_student_id}
? ($extract_claim->($ce->{LTI}{v1p3}{preferred_source_of_student_id}) // '')
Expand Down