Skip to content

Commit 86c588d

Browse files
drgrice1drgrice1
committed
Don't create users for LTI users that do not have permission to login.
Currently if `$permissionLevels{login} = 'professor'` and a user signs in via LTI that would be assigned the role of "student", then webwork2 creates the user and signs the user in. However, on subsequent LTI logins authentication fails. This refuses to create a user if the requested role would not have permission to login. Clean up the error messages some. There is a lot of work left to do on this. The LTIAdvance.pm module has an extremely poor design for error handling and messaging to go with those errors. The LTIAdvantage.pm module is only a tad better (I largely just copied the poor design of the LTIAdvanced.pm module). The `log_error` key is set and appended to numerous times, frequently resulting in a long run on message that doesn't really make sense. Also, there were some of these errors that were adding "LOGIN FAILED". That was removed because The `Authen.pm` code always prepends that and that resulted in logs with "LOGIN FAILED LOGIN FAILED ...". The `authenticate` method is expected to return either 1 or a message indicating the failure. Currently it returns either 1 or 0. As a result the messages that are set in the `authenticate` method go into the abyss. Those messages should be returned instead of setting `$self->{error}`. Note that the method can still return 0 if no message should be set (as in the case of the OAuth token failing to verify for LTI 1.1). For LTI 1.3 make sure that the fallback_source_of_username is set before attempting to use it. Otherwise the claim extraction fails and it results in a database error later. Fix a minor issue in the authen_LTI.conf.dist file. The permissionLevels lines should end with semicolons, not commas.
1 parent a39e424 commit 86c588d

File tree

3 files changed

+104
-99
lines changed

3 files changed

+104
-99
lines changed

‎conf/authen_LTI.conf.dist‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,6 @@ $LTISendGradesEarlyThreshold = 'attempted';
213213
# page, then a new mass passback job will begin. Set this to -1 to disable mass passback.
214214
$LTIMassUpdateInterval = 86400;
215215

216-
217216
################################################################################################
218217
# Add an 'LTI' tab to the Course Configuration page
219218
################################################################################################
@@ -255,15 +254,15 @@ $LTIMassUpdateInterval = 86400;
255254

256255
# By default only admin users can modify the LTI secrets and lms_context_id. The following
257256
# permissions need to be modified to allow other users the permission to modify the values.
258-
#$permissionLevels{'change_config_LTI{v1p1}{BasicConsumerSecret}'} = "admin",
259-
#$permissionLevels{'change_config_LTI{v1p3}{PlatformID}'} = "admin",
260-
#$permissionLevels{'change_config_LTI{v1p3}{ClientID}'} = "admin",
261-
#$permissionLevels{'change_config_LTI{v1p3}{DeploymentID}'} = "admin",
262-
#$permissionLevels{'change_config_LTI{v1p3}{PublicKeysetURL}'} = "admin",
263-
#$permissionLevels{'change_config_LTI{v1p3}{AccessTokenURL}'} = "admin",
264-
#$permissionLevels{'change_config_LTI{v1p3}{AccessTokenAUD}'} = "admin",
265-
#$permissionLevels{'change_config_LTI{v1p3}{AuthReqURL}'} = "admin",
266-
#$permissionLevels{'change_config_lms_context_id'} = "admin",
257+
#$permissionLevels{'change_config_LTI{v1p1}{BasicConsumerSecret}'} = "admin";
258+
#$permissionLevels{'change_config_LTI{v1p3}{PlatformID}'} = "admin";
259+
#$permissionLevels{'change_config_LTI{v1p3}{ClientID}'} = "admin";
260+
#$permissionLevels{'change_config_LTI{v1p3}{DeploymentID}'} = "admin";
261+
#$permissionLevels{'change_config_LTI{v1p3}{PublicKeysetURL}'} = "admin";
262+
#$permissionLevels{'change_config_LTI{v1p3}{AccessTokenURL}'} = "admin";
263+
#$permissionLevels{'change_config_LTI{v1p3}{AccessTokenAUD}'} = "admin";
264+
#$permissionLevels{'change_config_LTI{v1p3}{AuthReqURL}'} = "admin";
265+
#$permissionLevels{'change_config_lms_context_id'} = "admin";
267266

268267
# Note that the lms_context_id is actually a database setting. It must be set for a course in
269268
# order for the instructor to utilize LTI content selection. This can also be set in the admin

‎lib/WeBWorK/Authen/LTIAdvanced.pm‎

Lines changed: 69 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -117,10 +117,9 @@ sub get_credentials {
117117
# Determine the WW user_id to use, if possible
118118

119119
if (!$ce->{LTI}{v1p1}{preferred_source_of_username}) {
120-
warn
121-
"LTI is not properly configured (no preferred_source_of_username). Please contact your instructor or system administrator.";
122-
$self->{error} = $c->maketext(
123-
"There was an error during the login process. Please speak to your instructor or system administrator.");
120+
$self->{error} = $c->maketext("There was an error during the login process. "
121+
. "Please speak to your instructor or system administrator.");
122+
warn "LTI is not properly configured (no preferred_source_of_username).\n" if $ce->{debug_lti_parameters};
124123
debug("No preferred_source_of_username in "
125124
. $c->ce->{'courseName'}
126125
. " so LTIAdvanced::get_credentials is returning a 0\n");
@@ -228,17 +227,17 @@ sub get_credentials {
228227
warn "================================\n";
229228
}
230229
if (!defined($self->{user_id})) {
231-
croak
232-
"LTIAdvanced was unable to create a username from the data provided with the current settings. Set \$debug_lti_parameters=1 in authen_LTI.conf to debug";
230+
croak "LTIAdvanced was unable to create a username from the data provided with the current settings. "
231+
. "Set \$debug_lti_parameters=1 in authen_LTI.conf to debug";
233232
}
234233

235234
$self->{login_type} = "normal";
236235
$self->{credential_source} = "LTIAdvanced";
237236
debug("LTIAdvanced::get_credentials is returning a 1\n");
238237
return 1;
239238
}
240-
warn
241-
"LTI is not properly configured (failed to set user_id from preferred_source_of_username or fallback_source_of_username). Please contact your instructor or system administrator.";
239+
warn "LTI is not properly configured (failed to set user_id from preferred_source_of_username or "
240+
. "fallback_source_of_username). Please contact your instructor or system administrator.";
242241
$self->{error} = $c->maketext(
243242
"There was an error during the login process. Please speak to your instructor or system administrator.");
244243
debug("LTIAdvanced::get_credentials is returning a 0\n");
@@ -285,9 +284,8 @@ sub check_user {
285284

286285
foreach my $key (keys(%options), ($use_lis_person_sourcedid_options ? @lis_person_sourcedid_options : ())) {
287286
if (defined($c->param($key))) {
288-
debug(
289-
"User |$user_id| is unknown but may be an new user from an LSM via LTI. Saw a value for $key About to return a 1"
290-
);
287+
debug("User |$user_id| is unknown but may be an new user from an LMS via LTI. "
288+
. "Saw a value for $key About to return a 1");
291289
return 1; #This may be a new user coming in from a LMS via LTI.
292290
}
293291
}
@@ -299,13 +297,13 @@ sub check_user {
299297
}
300298

301299
unless ($ce->status_abbrev_has_behavior($User->status, "allow_course_access")) {
302-
$self->{log_error} .= "LOGIN FAILED $user_id - course access denied";
300+
$self->{log_error} .= "$user_id - course access denied";
303301
$self->{error} = $c->maketext("Authentication failed. Please speak to your instructor.");
304302
return 0;
305303
}
306304

307305
unless ($authz->hasPermissions($user_id, "login")) {
308-
$self->{log_error} .= "LOGIN FAILED $user_id - no permission to login";
306+
$self->{log_error} .= "$user_id - no permission to login";
309307
$self->{error} = $c->maketext("Authentication failed. Please speak to your instructor.");
310308
return 0;
311309
}
@@ -360,12 +358,10 @@ sub authenticate {
360358
# Check nonce to see whether request is legitimate
361359
debug("Nonce = |" . $self->{oauth_nonce} . "|");
362360
my $nonce = WeBWorK::Authen::LTIAdvanced::Nonce->new($c, $self->{oauth_nonce}, $self->{oauth_timestamp});
363-
if (!($nonce->ok)) {
364-
$self->{error} .= $c->maketext(
365-
"There was an error during the login process. Please speak to your instructor or system administrator if this recurs."
366-
);
361+
if (!$nonce->ok) {
367362
debug("Failed to verify nonce");
368-
return 0;
363+
return $c->maketext("There was an error during the login process. "
364+
. "Please speak to your instructor or system administrator if this recurs.");
369365
}
370366

371367
debug("c->param(oauth_signature) = |" . $c->param("oauth_signature") . "|");
@@ -418,69 +414,56 @@ sub authenticate {
418414
debug("construction of Net::OAuth object failed: $@");
419415
debug("eval failed: ", $@, "<br /><br />");
420416

421-
$self->{error} .= $c->maketext(
422-
"There was an error during the login process. Please speak to your instructor or system administrator.");
423417
$self->{log_error} .= "Construction of OAuth request record failed";
424-
return 0;
418+
return $c->maketext(
419+
"There was an error during the login process. Please speak to your instructor or system administrator.");
425420
}
426421

427422
if (!$request->verify && !$altrequest->verify) {
428-
debug("LTIAdvanced::authenticate request-> verify failed");
429-
debug("OAuth verification Failed ");
423+
debug("LTIAdvanced::authenticate request->verify failed");
424+
debug("OAuth verification Failed");
430425

431-
$self->{error} .= $c->maketext(
432-
"There was an error during the login process. Please speak to your instructor or system administrator.");
433-
$self->{log_error} .=
434-
"OAuth verification failed. Check the Consumer Secret and that the URL in the LMS exactly matches the WeBWorK URL.";
426+
$self->{log_error} .= "OAuth verification failed. "
427+
. "Check the Consumer Secret and that the URL in the LMS exactly matches the WeBWorK URL.";
435428
if ($ce->{debug_lti_parameters}) {
436-
warn(
437-
"OAuth verification failed. Check the Consumer Secret and that the URL in the LMS exactly matches the WeBWorK URL as defined in site.conf. E.G. Check that if you have https in the LMS url then you have https in \$server_root_url in site.conf"
438-
);
429+
warn("OAuth verification failed. Check the Consumer Secret and that the URL in the LMS exactly "
430+
. "matches the WeBWorK URL as defined in site.conf. E.G. Check that if you have https in the "
431+
. "LMS url then you have https in \$server_root_url in site.conf");
439432
}
440433
return 0;
441434
} else {
442435
debug("OAuth verification SUCCEEDED !!");
443436

444437
my $userID = $self->{user_id};
445438

446-
# Indentation of the internal blocks below was modified to follow
447-
# the WW coding standard; however, the leading indentation of the
448-
# if/elsif/closing '}' was kept as in the original code for now.
449-
# Thus the apparenly overlarge indentation below.
450439
if (!$db->existsUser($userID)) { # New User. Create User record
451440
if ($ce->{block_lti_create_user}) {
452-
# We don't yet have the next string in the PO/POT files - so the next line is disabled.
453-
# $c->maketext("Account creation is currently disabled in this course. Please speak to your instructor or system administrator.");
454441
$self->{log_error} .=
455442
"Account creation blocked by block_lti_create_user setting. Did not create user $userID.";
456-
if ($ce->{debug_lti_parameters}) {
457-
warn(
458-
"Account creation is currently disabled in this course. Please speak to your instructor or system administrator."
459-
);
460-
}
461-
return 0;
443+
warn "Account creation is currently disabled in this course. "
444+
. "Please speak to your instructor or system administrator."
445+
if $ce->{debug_lti_parameters};
446+
return $c->maketext("Account creation is currently disabled in this course. "
447+
. "Please speak to your instructor or system administrator.");
462448
} else {
463449
# Attempt to create the user, and warn if that fails.
464-
unless ($self->create_user()) {
465-
$c->maketext(
466-
"There was an error during the login process. Please speak to your instructor or system administrator."
467-
);
450+
unless ($self->create_user) {
468451
$self->{log_error} .= "Failed to create user $userID.";
469-
if ($ce->{debug_lti_parameters}) {
470-
warn("Failed to create user $userID.");
471-
}
452+
warn "Failed to create user $userID.\n" if $ce->{debug_lti_parameters};
453+
return $c->maketext('Unable to create a WeBWorK user. '
454+
. 'Please speak to your instructor or system administrator.');
472455
}
473456
}
474457
} elsif ($ce->{LMSManageUserData}) {
475-
$self->{initial_login} = 1
476-
; # Set here so login gets logged, even for accounts which maybe_update_user() would not modify or when it fails to update
477-
# Existing user. Possibly modify demographic information and permission level.
458+
# Set here so login gets logged, even for accounts which maybe_update_user()
459+
# would not modify or when it fails to update.
460+
$self->{initial_login} = 1;
461+
462+
# Existing user. Possibly modify demographic information and permission level.
478463
unless ($self->maybe_update_user()) {
479-
# Do not fail the login if data update failed
480-
# FIXME - In the future we would like the message below (and other warn messages in this file) to be sent via maketext.
481-
warn(
482-
"The system failed to update some of your account information. Please speak to your instructor or system administrator."
483-
);
464+
# Do not fail the login if data update failed
465+
warn("The system failed to update some of your account information. "
466+
. "Please speak to your instructor or system administrator.");
484467
}
485468
} else {
486469
# Set here so login gets logged when $ce->{LMSManageUserData} is false
@@ -501,9 +484,8 @@ sub authenticate {
501484
}
502485

503486
debug("LTIAdvanced is returning a failed authentication");
504-
$self->{error} = $c->maketext(
487+
return $c->maketext(
505488
"There was an error during the login process. Please speak to your instructor or system administrator.");
506-
return (0);
507489
}
508490

509491
# create a new user trying to log in
@@ -515,32 +497,35 @@ sub create_user {
515497
my $db = $c->db;
516498
my $courseName = $c->ce->{'courseName'};
517499

518-
############################################################
519500
# Determine the roles defined for this user by the LTI request
520501
# and assign a permission level on that basis.
521-
############################################################
502+
522503
my $LTIrolesString = $c->param("roles");
523504
my @LTIroles = split /,/, $LTIrolesString;
524505

525-
#remove the urn string if its present
506+
# Remove the urn string if its present.
526507
s/^urn:lti:.*:ims\/lis\/// for @LTIroles;
527508

528509
if ($ce->{debug_lti_parameters}) {
529510
warn "The adjusted LTI roles defined for this user are: \n--",
530-
join("\n--", @LTIroles), "\n",
511+
join("\n-- ", @LTIroles), "\n",
531512
"Any initial ^urn:lti:.*:ims/lis/ segments have been stripped off.\n",
532513
"The user will be assigned the highest role defined for them\n",
533514
"========================\n";
534515
}
535516

536-
my $nr = scalar(@LTIroles);
537-
if (!defined($ce->{userRoles}->{ $ce->{LTI}{v1p1}{LMSrolesToWeBWorKroles}->{ $LTIroles[0] } })) {
538-
croak("Cannot find a WeBWorK role that corresponds to the LMS role of " . $LTIroles[0] . ".");
517+
if (!defined $ce->{LTI}{v1p1}{LMSrolesToWeBWorKroles}{ $LTIroles[0] }
518+
|| !defined $ce->{userRoles}{ $ce->{LTI}{v1p1}{LMSrolesToWeBWorKroles}{ $LTIroles[0] } })
519+
{
520+
$self->{log_error} = "Cannot find a WeBWorK role that corresponds to the LMS role of $LTIroles[0].";
521+
warn "Cannot find a WeBWorK role that corresponds to the LMS role of $LTIroles[0].\n"
522+
if $ce->{debug_lti_parameters};
523+
return 0;
539524
}
540525

541-
my $LTI_webwork_permissionLevel = $ce->{userRoles}->{ $ce->{LTI}{v1p1}{LMSrolesToWeBWorKroles}->{ $LTIroles[0] } };
542-
if ($nr > 1) {
543-
for (my $j = 1; $j < $nr; $j++) {
526+
my $LTI_webwork_permissionLevel = $ce->{userRoles}{ $ce->{LTI}{v1p1}{LMSrolesToWeBWorKroles}{ $LTIroles[0] } };
527+
if (@LTIroles > 1) {
528+
for (my $j = 1; $j < @LTIroles; $j++) {
544529
my $wwRole = $ce->{LTI}{v1p1}{LMSrolesToWeBWorKroles}->{ $LTIroles[$j] };
545530
next unless defined $wwRole;
546531
if ($LTI_webwork_permissionLevel < $ce->{userRoles}->{$wwRole}) {
@@ -549,20 +534,26 @@ sub create_user {
549534
}
550535
}
551536

552-
####### End defining roles and $LTI_webwork_permissionLevel#######
537+
# End defining roles and $LTI_webwork_permissionLevel
553538

554539
warn "New user: $userID -- requested permission level is $LTI_webwork_permissionLevel."
555-
if ($ce->{debug_lti_parameters});
540+
if $ce->{debug_lti_parameters};
556541

557-
# We dont create users with too high of a permission level
558-
# for security reasons.
542+
# Don't create a user that does not have permission to login.
543+
if ($LTI_webwork_permissionLevel < $ce->{userRoles}{ $ce->{permissionLevels}{login} }) {
544+
$self->{log_error} .= "$userID - no permission to login";
545+
return 0;
546+
}
547+
548+
# We dont create users with too high of a permission level for security reasons.
559549
if ($LTI_webwork_permissionLevel > $ce->{userRoles}->{ $ce->{LTIAccountCreationCutoff} }) {
560550
$self->{log_error} .=
561-
"userID: $userID -- Unknown instructor attempting to log in via LTI. Instructor accounts must be created manually";
562-
croak $c->maketext(
563-
"The instructor account with user id [_1] does not exist. Please create the account manually via WeBWorK.",
564-
$userID
565-
);
551+
"The instructor account with user id $userID does not exist. "
552+
. 'Instructor accounts must be created manually.';
553+
warn "The instructor account with user id $userID does not exist. "
554+
. "Instructor accounts must be created manually.\n"
555+
if $ce->{debug_lti_parameters};
556+
return 0;
566557
}
567558

568559
my $newUser = $db->newUser();

‎lib/WeBWorK/Authen/LTIAdvantage.pm‎

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,8 @@ sub get_credentials ($self) {
141141
}
142142

143143
# Fallback if necessary
144-
if (!defined $self->{user_id}
144+
if ($ce->{LTI}{v1p3}{fallback_source_of_username}
145+
&& !defined $self->{user_id}
145146
&& (my $user_id = $extract_claim->($ce->{LTI}{v1p3}{fallback_source_of_username})))
146147
{
147148
$user_id_source = $ce->{LTI}{v1p3}{fallback_source_of_username};
@@ -243,13 +244,13 @@ sub check_user ($self) {
243244
}
244245

245246
unless ($ce->status_abbrev_has_behavior($User->status, 'allow_course_access')) {
246-
$self->{log_error} .= "LOGIN FAILED $user_id - course access denied";
247+
$self->{log_error} .= "$user_id - course access denied";
247248
$self->{error} = $c->maketext('Authentication failed. Please speak to your instructor.');
248249
return 0;
249250
}
250251

251252
unless ($authz->hasPermissions($user_id, 'login')) {
252-
$self->{log_error} .= "LOGIN FAILED $user_id - no permission to login";
253+
$self->{log_error} .= "$user_id - no permission to login";
253254
$self->{error} = $c->maketext('Authentication failed. Please speak to your instructor.');
254255
return 0;
255256
}
@@ -303,12 +304,15 @@ sub authenticate ($self) {
303304
warn $c->maketext('Account creation is currently disabled in this course. '
304305
. 'Please speak to your instructor or system administrator.') . "\n";
305306
}
306-
return 0;
307+
return $c->maketext("Account creation is currently disabled in this course. "
308+
. "Please speak to your instructor or system administrator.");
307309
} else {
308310
# Attempt to create the user, and warn if that fails.
309311
unless ($self->create_user) {
310312
$self->{log_error} .= "Failed to create user $self->{user_id}.";
311-
warn "Failed to create user $self->{user_id}.\n" if ($ce->{debug_lti_parameters});
313+
warn "Failed to create user $self->{user_id}.\n" if $ce->{debug_lti_parameters};
314+
return $c->maketext('Unable to create a WeBWorK user. '
315+
. 'Please speak to your instructor or system administrator.');
312316
}
313317
}
314318
} elsif ($ce->{LMSManageUserData}) {
@@ -360,7 +364,10 @@ sub create_user ($self) {
360364
}
361365

362366
if (!defined($ce->{userRoles}{ $ce->{LTI}{v1p3}{LMSrolesToWeBWorKroles}{ $LTIroles[0] } })) {
363-
die "Cannot find a WeBWorK role that corresponds to the LMS role of $LTIroles[0].\n";
367+
$self->{log_error} = "Cannot find a WeBWorK role that corresponds to the LMS role of $LTIroles[0].\n";
368+
warn "Cannot find a WeBWorK role that corresponds to the LMS role of $LTIroles[0].\n"
369+
if $ce->{debug_lti_parameters};
370+
return 0;
364371
}
365372

366373
my $LTI_webwork_permissionLevel = $ce->{userRoles}{ $ce->{LTI}{v1p3}{LMSrolesToWeBWorKroles}{ $LTIroles[0] } };
@@ -378,11 +385,19 @@ sub create_user ($self) {
378385

379386
# We dont create users with too high of a permission level for security reasons.
380387
if ($LTI_webwork_permissionLevel > $ce->{userRoles}{ $ce->{LTIAccountCreationCutoff} }) {
381-
die $c->maketext(
382-
'The instructor account with user id [_1] does not exist. '
383-
. 'Instructor accounts must be created manually.',
384-
$userID
385-
) . "\n";
388+
$self->{log_error} =
389+
"The instructor account with user id $userID does not exist. "
390+
. 'Instructor accounts must be created manually.';
391+
warn "The instructor account with user id $userID does not exist. "
392+
. "Instructor accounts must be created manually.\n"
393+
if $ce->{debug_lti_parameters};
394+
return 0;
395+
}
396+
397+
# Don't create a user that does not have permission to login.
398+
if ($LTI_webwork_permissionLevel < $ce->{userRoles}{ $ce->{permissionLevels}{login} }) {
399+
$self->{log_error} .= "$userID - no permission to login";
400+
return 0;
386401
}
387402

388403
my $newUser = $db->newUser;

0 commit comments

Comments
 (0)