Skip to content

Commit 9d60716

Browse files
somiajsomiaj
authored
Merge pull request #2883 from drgrice1/validate-user-login-each-request
Validate user login capability on each request.
2 parents 088a2ad + 326890f commit 9d60716

3 files changed

Lines changed: 45 additions & 38 deletions

File tree

‎lib/WeBWorK/Authen.pm‎

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -377,9 +377,9 @@ sub check_user {
377377
return 0;
378378
}
379379

380-
my $User = $db->getUser($user_id);
380+
$self->{user} = $db->getUser($user_id);
381381

382-
unless ($User) {
382+
unless ($self->{user}) {
383383
$self->{log_error} = "user unknown";
384384
$self->{error} = $c->maketext(GENERIC_ERROR_MESSAGE);
385385
return 0;
@@ -388,6 +388,29 @@ sub check_user {
388388
return 1;
389389
}
390390

391+
sub validate_user {
392+
my $self = shift;
393+
my $c = $self->{c};
394+
395+
# Deny access for certain roles (dropped students, proctor roles).
396+
unless ($self->{login_type} =~ /^proctor/
397+
|| $c->ce->status_abbrev_has_behavior($self->{user}->status, 'allow_course_access'))
398+
{
399+
$self->{log_error} = 'user not allowed course access';
400+
$self->{error} = $c->maketext('This user is not allowed to log in to this course');
401+
return 0;
402+
}
403+
404+
# Deny access for permission levels below 'login' permission level.
405+
unless ($c->authz->hasPermissions($self->{user_id}, 'login')) {
406+
$self->{log_error} = 'user not permitted to login';
407+
$self->{error} = $c->maketext('This user is not allowed to log in to this course');
408+
return 0;
409+
}
410+
411+
return 1;
412+
}
413+
391414
sub verify_practice_user {
392415
my $self = shift;
393416
my $c = $self->{c};
@@ -485,6 +508,7 @@ sub verify_normal_user {
485508
$c->stash(authen_error => $c->maketext('The security code is required.'));
486509
}
487510
}
511+
return 0 unless $self->validate_user;
488512
return 1;
489513
} else {
490514
my $auth_result = $self->authenticate;
@@ -494,20 +518,7 @@ sub verify_normal_user {
494518
delete $self->session->{two_factor_verification_needed};
495519

496520
if ($auth_result > 0) {
497-
# Deny certain roles (dropped students, proctor roles).
498-
unless ($self->{login_type} =~ /^proctor/
499-
|| $c->ce->status_abbrev_has_behavior($c->db->getUser($user_id)->status, "allow_course_access"))
500-
{
501-
$self->{log_error} = "user not allowed course access";
502-
$self->{error} = $c->maketext('This user is not allowed to log in to this course');
503-
return 0;
504-
}
505-
# Deny permission levels below "login" permission level.
506-
unless ($c->authz->hasPermissions($user_id, "login")) {
507-
$self->{log_error} = "user not permitted to login";
508-
$self->{error} = $c->maketext('This user is not allowed to log in to this course');
509-
return 0;
510-
}
521+
return 0 unless $self->validate_user;
511522
$self->{session_key} = $self->create_session($user_id);
512523
$self->{initial_login} = 1;
513524
return 1;

‎lib/WeBWorK/Authen/LTIAdvanced.pm‎

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -262,9 +262,9 @@ sub check_user {
262262
return 0;
263263
}
264264

265-
my $User = $db->getUser($user_id);
265+
$self->{user} = $db->getUser($user_id);
266266

267-
if (!$User) {
267+
if (!$self->{user}) {
268268
my %options;
269269
$options{ $ce->{LTI}{v1p1}{preferred_source_of_username} } = 1
270270
if ($ce->{LTI}{v1p1}{preferred_source_of_username});
@@ -285,7 +285,7 @@ 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("User |$user_id| is unknown but may be an new user from an LMS via LTI. "
288+
debug("User |$user_id| is unknown but may be a new user from an LMS via LTI. "
289289
. "Saw a value for $key About to return a 1");
290290
return 1; #This may be a new user coming in from a LMS via LTI.
291291
}
@@ -297,7 +297,7 @@ sub check_user {
297297
return 0;
298298
}
299299

300-
unless ($ce->status_abbrev_has_behavior($User->status, "allow_course_access")) {
300+
unless ($ce->status_abbrev_has_behavior($self->{user}->status, "allow_course_access")) {
301301
$self->{log_error} .= "$user_id - course access denied";
302302
$self->{error} = $c->maketext("Authentication failed. Please speak to your instructor.");
303303
return 0;
@@ -352,9 +352,7 @@ sub authenticate {
352352
debug("LTIAdvanced::authenticate called for user |$user|");
353353
debug "ref(c) = |" . ref($c) . "|";
354354

355-
my $ce = $c->ce;
356-
my $db = $c->db;
357-
my $courseName = $c->ce->{'courseName'};
355+
my $ce = $c->ce;
358356

359357
# Check nonce to see whether request is legitimate
360358
debug("Nonce = |" . $self->{oauth_nonce} . "|");
@@ -437,7 +435,7 @@ sub authenticate {
437435

438436
my $userID = $self->{user_id};
439437

440-
if (!$db->existsUser($userID)) { # New User. Create User record
438+
if (!$self->{user}) { # New User. Create User record
441439
if ($ce->{block_lti_create_user}) {
442440
$self->{log_error} .=
443441
"Account creation blocked by block_lti_create_user setting. Did not create user $userID.";
@@ -576,6 +574,7 @@ sub create_user {
576574
}
577575

578576
$db->addUser($newUser);
577+
$self->{user} = $newUser;
579578
$self->write_log_entry("New user $userID added via LTIAdvanced login");
580579

581580
# Assign permssion level
@@ -641,7 +640,6 @@ sub maybe_update_user {
641640
my $db = $c->db;
642641
my $courseName = $c->ce->{'courseName'};
643642

644-
my $user = $db->getUser($userID);
645643
my $permissionLevel = $db->getPermissionLevel($userID);
646644
# We don't alter records of users with too high a permission
647645
if (defined($permissionLevel->permission)
@@ -676,10 +674,10 @@ sub maybe_update_user {
676674
my $change_made = 0;
677675

678676
for my $element (@elements) {
679-
if ($user->$element ne $tempUser->$element) {
677+
if ($self->{user}->$element ne $tempUser->$element) {
680678
$change_made = 1;
681679
warn "WeBWorK User has $element: "
682-
. $user->$element
680+
. $self->{user}->$element
683681
. " but LMS user has $element "
684682
. $tempUser->$element . "\n"
685683
if ($ce->{debug_lti_parameters});

‎lib/WeBWorK/Authen/LTIAdvantage.pm‎

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -236,14 +236,14 @@ sub check_user ($self) {
236236
return 0;
237237
}
238238

239-
my $User = $db->getUser($user_id);
239+
$self->{user} = $db->getUser($user_id);
240240

241-
if (!$User) {
242-
debug("User |$user_id| is unknown but may be an new user from an LMS via LTI.");
241+
if (!$self->{user}) {
242+
debug("User |$user_id| is unknown but may be a new user from an LMS via LTI.");
243243
return 1;
244244
}
245245

246-
unless ($ce->status_abbrev_has_behavior($User->status, 'allow_course_access')) {
246+
unless ($ce->status_abbrev_has_behavior($self->{user}->status, 'allow_course_access')) {
247247
$self->{log_error} .= "$user_id - course access denied";
248248
$self->{error} = $c->maketext('Authentication failed. Please speak to your instructor.');
249249
return 0;
@@ -291,11 +291,9 @@ sub authenticate ($self) {
291291

292292
# The actual authentication for this module has already been done. This just creates and updates users if needed.
293293

294-
my $ce = $c->ce;
295-
my $db = $c->db;
296-
my $courseName = $c->ce->{courseName};
294+
my $ce = $c->ce;
297295

298-
if (!$db->existsUser($self->{user_id})) {
296+
if (!$self->{user}) {
299297
# New User. Create User record.
300298
if ($ce->{block_lti_create_user}) {
301299
$self->{log_error} .=
@@ -416,6 +414,7 @@ sub create_user ($self) {
416414
$ce->{LTI}{v1p3}{modify_user}($self, $newUser) if ref($ce->{LTI}{v1p3}{modify_user}) eq 'CODE';
417415

418416
$db->addUser($newUser);
417+
$self->{user} = $newUser;
419418
$self->write_log_entry("New user $userID added via LTIAdvantange login");
420419

421420
# Set permission level.
@@ -481,7 +480,6 @@ sub maybe_update_user ($self) {
481480
my $userID = $self->{user_id};
482481
my $courseName = $ce->{courseName};
483482

484-
my $user = $db->getUser($userID);
485483
my $permissionLevel = $db->getPermissionLevel($userID);
486484

487485
# We don't alter records of users with too high a permission.
@@ -507,10 +505,10 @@ sub maybe_update_user ($self) {
507505

508506
my $change_made = 0;
509507
for my $element (qw(last_name first_name email_address status section recitation student_id)) {
510-
if ($user->$element ne $tempUser->$element) {
508+
if ($self->{user}->$element ne $tempUser->$element) {
511509
$change_made = 1;
512510
warn "WeBWorK User has $element: "
513-
. $user->$element
511+
. $self->{user}->$element
514512
. " but LMS user has $element "
515513
. $tempUser->$element . "\n"
516514
if ($ce->{debug_lti_parameters});

0 commit comments

Comments
 (0)