Skip to content

Commit 5a26609

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 e15c575 commit 5a26609

File tree

3 files changed

+105
-99
lines changed

3 files changed

+105
-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: 70 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -117,10 +117,10 @@ 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+
warn "LTI is not properly configured (no preferred_source_of_username). "
121+
. "Please contact your instructor or system administrator.";
122+
$self->{error} = $c->maketext("There was an error during the login process. "
123+
. "Please speak to your instructor or system administrator.");
124124
debug("No preferred_source_of_username in "
125125
. $c->ce->{'courseName'}
126126
. " so LTIAdvanced::get_credentials is returning a 0\n");
@@ -228,17 +228,17 @@ sub get_credentials {
228228
warn "================================\n";
229229
}
230230
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";
231+
croak "LTIAdvanced was unable to create a username from the data provided with the current settings. "
232+
. "Set \$debug_lti_parameters=1 in authen_LTI.conf to debug";
233233
}
234234

235235
$self->{login_type} = "normal";
236236
$self->{credential_source} = "LTIAdvanced";
237237
debug("LTIAdvanced::get_credentials is returning a 1\n");
238238
return 1;
239239
}
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.";
240+
warn "LTI is not properly configured (failed to set user_id from preferred_source_of_username or "
241+
. "fallback_source_of_username). Please contact your instructor or system administrator.";
242242
$self->{error} = $c->maketext(
243243
"There was an error during the login process. Please speak to your instructor or system administrator.");
244244
debug("LTIAdvanced::get_credentials is returning a 0\n");
@@ -285,9 +285,8 @@ sub check_user {
285285

286286
foreach my $key (keys(%options), ($use_lis_person_sourcedid_options ? @lis_person_sourcedid_options : ())) {
287287
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-
);
288+
debug("User |$user_id| is unknown but may be an new user from an LSM via LTI. "
289+
. "Saw a value for $key About to return a 1");
291290
return 1; #This may be a new user coming in from a LMS via LTI.
292291
}
293292
}
@@ -299,13 +298,13 @@ sub check_user {
299298
}
300299

301300
unless ($ce->status_abbrev_has_behavior($User->status, "allow_course_access")) {
302-
$self->{log_error} .= "LOGIN FAILED $user_id - course access denied";
301+
$self->{log_error} .= "$user_id - course access denied";
303302
$self->{error} = $c->maketext("Authentication failed. Please speak to your instructor.");
304303
return 0;
305304
}
306305

307306
unless ($authz->hasPermissions($user_id, "login")) {
308-
$self->{log_error} .= "LOGIN FAILED $user_id - no permission to login";
307+
$self->{log_error} .= "$user_id - no permission to login";
309308
$self->{error} = $c->maketext("Authentication failed. Please speak to your instructor.");
310309
return 0;
311310
}
@@ -360,12 +359,10 @@ sub authenticate {
360359
# Check nonce to see whether request is legitimate
361360
debug("Nonce = |" . $self->{oauth_nonce} . "|");
362361
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-
);
362+
if (!$nonce->ok) {
367363
debug("Failed to verify nonce");
368-
return 0;
364+
return $c->maketext("There was an error during the login process. "
365+
. "Please speak to your instructor or system administrator if this recurs.");
369366
}
370367

371368
debug("c->param(oauth_signature) = |" . $c->param("oauth_signature") . "|");
@@ -418,69 +415,56 @@ sub authenticate {
418415
debug("construction of Net::OAuth object failed: $@");
419416
debug("eval failed: ", $@, "<br /><br />");
420417

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

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

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.";
427+
$self->{log_error} .= "OAuth verification failed. "
428+
. "Check the Consumer Secret and that the URL in the LMS exactly matches the WeBWorK URL.";
435429
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-
);
430+
warn("OAuth verification failed. Check the Consumer Secret and that the URL in the LMS exactly "
431+
. "matches the WeBWorK URL as defined in site.conf. E.G. Check that if you have https in the "
432+
. "LMS url then you have https in \$server_root_url in site.conf");
439433
}
440434
return 0;
441435
} else {
442436
debug("OAuth verification SUCCEEDED !!");
443437

444438
my $userID = $self->{user_id};
445439

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.
450440
if (!$db->existsUser($userID)) { # New User. Create User record
451441
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.");
454442
$self->{log_error} .=
455443
"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;
444+
warn "Account creation is currently disabled in this course. "
445+
. "Please speak to your instructor or system administrator."
446+
if $ce->{debug_lti_parameters};
447+
return $c->maketext("Account creation is currently disabled in this course. "
448+
. "Please speak to your instructor or system administrator.");
462449
} else {
463450
# 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-
);
451+
unless ($self->create_user) {
468452
$self->{log_error} .= "Failed to create user $userID.";
469-
if ($ce->{debug_lti_parameters}) {
470-
warn("Failed to create user $userID.");
471-
}
453+
warn "Failed to create user $userID.\n" if $ce->{debug_lti_parameters};
454+
return $c->maketext('Unable to create a WeBWorK user. '
455+
. 'Please speak to your instructor or system administrator.');
472456
}
473457
}
474458
} 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.
459+
# Set here so login gets logged, even for accounts which maybe_update_user()
460+
# would not modify or when it fails to update.
461+
$self->{initial_login} = 1;
462+
463+
# Existing user. Possibly modify demographic information and permission level.
478464
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-
);
465+
# Do not fail the login if data update failed
466+
warn("The system failed to update some of your account information. "
467+
. "Please speak to your instructor or system administrator.");
484468
}
485469
} else {
486470
# Set here so login gets logged when $ce->{LMSManageUserData} is false
@@ -501,9 +485,8 @@ sub authenticate {
501485
}
502486

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

509492
# create a new user trying to log in
@@ -515,32 +498,35 @@ sub create_user {
515498
my $db = $c->db;
516499
my $courseName = $c->ce->{'courseName'};
517500

518-
############################################################
519501
# Determine the roles defined for this user by the LTI request
520502
# and assign a permission level on that basis.
521-
############################################################
503+
522504
my $LTIrolesString = $c->param("roles");
523505
my @LTIroles = split /,/, $LTIrolesString;
524506

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

528510
if ($ce->{debug_lti_parameters}) {
529511
warn "The adjusted LTI roles defined for this user are: \n--",
530-
join("\n--", @LTIroles), "\n",
512+
join("\n-- ", @LTIroles), "\n",
531513
"Any initial ^urn:lti:.*:ims/lis/ segments have been stripped off.\n",
532514
"The user will be assigned the highest role defined for them\n",
533515
"========================\n";
534516
}
535517

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] . ".");
518+
if (!defined $ce->{LTI}{v1p1}{LMSrolesToWeBWorKroles}{ $LTIroles[0] }
519+
|| !defined $ce->{userRoles}{ $ce->{LTI}{v1p1}{LMSrolesToWeBWorKroles}{ $LTIroles[0] } })
520+
{
521+
$self->{log_error} = "Cannot find a WeBWorK role that corresponds to the LMS role of $LTIroles[0].";
522+
warn "Cannot find a WeBWorK role that corresponds to the LMS role of $LTIroles[0].\n"
523+
if $ce->{debug_lti_parameters};
524+
return 0;
539525
}
540526

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++) {
527+
my $LTI_webwork_permissionLevel = $ce->{userRoles}{ $ce->{LTI}{v1p1}{LMSrolesToWeBWorKroles}{ $LTIroles[0] } };
528+
if (@LTIroles > 1) {
529+
for (my $j = 1; $j < @LTIroles; $j++) {
544530
my $wwRole = $ce->{LTI}{v1p1}{LMSrolesToWeBWorKroles}->{ $LTIroles[$j] };
545531
next unless defined $wwRole;
546532
if ($LTI_webwork_permissionLevel < $ce->{userRoles}->{$wwRole}) {
@@ -549,20 +535,26 @@ sub create_user {
549535
}
550536
}
551537

552-
####### End defining roles and $LTI_webwork_permissionLevel#######
538+
# End defining roles and $LTI_webwork_permissionLevel
553539

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

557-
# We dont create users with too high of a permission level
558-
# for security reasons.
543+
# Don't create a user that does not have permission to login.
544+
if ($LTI_webwork_permissionLevel < $ce->{userRoles}{ $ce->{permissionLevels}{login} }) {
545+
$self->{log_error} .= "$userID - no permission to login";
546+
return 0;
547+
}
548+
549+
# We dont create users with too high of a permission level for security reasons.
559550
if ($LTI_webwork_permissionLevel > $ce->{userRoles}->{ $ce->{LTIAccountCreationCutoff} }) {
560551
$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-
);
552+
"The instructor account with user id $userID does not exist. "
553+
. 'Instructor accounts must be created manually.';
554+
warn "The instructor account with user id $userID does not exist. "
555+
. "Instructor accounts must be created manually.\n"
556+
if $ce->{debug_lti_parameters};
557+
return 0;
566558
}
567559

568560
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)