From 326890fb4570cf9f728c6e89b4908aee6d52f7df Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Fri, 9 Jan 2026 07:33:48 -0600 Subject: [PATCH] Validate user login capability on each request. Currently the login capability of a user is only ever checked on initial sign in, and never again. So if a user logs in, and then the status or permission level of the user is changed so that the user no longer has the `allow_course_access` behavior or `login` permission level, then the user's current session remains valid, and the user may continue to work in the course (including submitting answers). This changes that so that those things are checked on each request. So, for example, if a user is dropped (status changed to "D"), then the next thing the user tries to do in the course that involves a request to the server will result in the user being logged out. This was reported for the Shibboleth authentication module in issue #2827, but really is an issue for all authentication modules. So this more generally fixes issue #2827 for all authentication modules. This has been tested for all functional authentication modules (i.e., for all but the `CAS` and `Moodle` authentication modules. If the `CAS` module is fixed this should work for that as well. I plan to remove the `Moodle` authentication module in another pull request. Note that this is done in such a way that no new database queries are needed. To make this happen the user record is cached in the `check_user` call, and then can be used any time after that. Future plans are to take this much further. There are many times in the code that the database record for the current user is fetched from the database, and now this cached user record from the current authentication module could directly be used instead. --- lib/WeBWorK/Authen.pm | 43 +++++++++++++++++++----------- lib/WeBWorK/Authen/LTIAdvanced.pm | 20 +++++++------- lib/WeBWorK/Authen/LTIAdvantage.pm | 20 +++++++------- 3 files changed, 45 insertions(+), 38 deletions(-) diff --git a/lib/WeBWorK/Authen.pm b/lib/WeBWorK/Authen.pm index 6e4f47d54a..ccc02f2cad 100644 --- a/lib/WeBWorK/Authen.pm +++ b/lib/WeBWorK/Authen.pm @@ -377,9 +377,9 @@ sub check_user { return 0; } - my $User = $db->getUser($user_id); + $self->{user} = $db->getUser($user_id); - unless ($User) { + unless ($self->{user}) { $self->{log_error} = "user unknown"; $self->{error} = $c->maketext(GENERIC_ERROR_MESSAGE); return 0; @@ -388,6 +388,29 @@ sub check_user { return 1; } +sub validate_user { + my $self = shift; + my $c = $self->{c}; + + # Deny access for certain roles (dropped students, proctor roles). + unless ($self->{login_type} =~ /^proctor/ + || $c->ce->status_abbrev_has_behavior($self->{user}->status, 'allow_course_access')) + { + $self->{log_error} = 'user not allowed course access'; + $self->{error} = $c->maketext('This user is not allowed to log in to this course'); + return 0; + } + + # Deny access for permission levels below 'login' permission level. + unless ($c->authz->hasPermissions($self->{user_id}, 'login')) { + $self->{log_error} = 'user not permitted to login'; + $self->{error} = $c->maketext('This user is not allowed to log in to this course'); + return 0; + } + + return 1; +} + sub verify_practice_user { my $self = shift; my $c = $self->{c}; @@ -485,6 +508,7 @@ sub verify_normal_user { $c->stash(authen_error => $c->maketext('The security code is required.')); } } + return 0 unless $self->validate_user; return 1; } else { my $auth_result = $self->authenticate; @@ -494,20 +518,7 @@ sub verify_normal_user { delete $self->session->{two_factor_verification_needed}; if ($auth_result > 0) { - # Deny certain roles (dropped students, proctor roles). - unless ($self->{login_type} =~ /^proctor/ - || $c->ce->status_abbrev_has_behavior($c->db->getUser($user_id)->status, "allow_course_access")) - { - $self->{log_error} = "user not allowed course access"; - $self->{error} = $c->maketext('This user is not allowed to log in to this course'); - return 0; - } - # Deny permission levels below "login" permission level. - unless ($c->authz->hasPermissions($user_id, "login")) { - $self->{log_error} = "user not permitted to login"; - $self->{error} = $c->maketext('This user is not allowed to log in to this course'); - return 0; - } + return 0 unless $self->validate_user; $self->{session_key} = $self->create_session($user_id); $self->{initial_login} = 1; return 1; diff --git a/lib/WeBWorK/Authen/LTIAdvanced.pm b/lib/WeBWorK/Authen/LTIAdvanced.pm index 31115e3dbf..0df122c8f7 100644 --- a/lib/WeBWorK/Authen/LTIAdvanced.pm +++ b/lib/WeBWorK/Authen/LTIAdvanced.pm @@ -262,9 +262,9 @@ sub check_user { return 0; } - my $User = $db->getUser($user_id); + $self->{user} = $db->getUser($user_id); - if (!$User) { + if (!$self->{user}) { my %options; $options{ $ce->{LTI}{v1p1}{preferred_source_of_username} } = 1 if ($ce->{LTI}{v1p1}{preferred_source_of_username}); @@ -285,7 +285,7 @@ sub check_user { foreach my $key (keys(%options), ($use_lis_person_sourcedid_options ? @lis_person_sourcedid_options : ())) { if (defined($c->param($key))) { - debug("User |$user_id| is unknown but may be an new user from an LMS via LTI. " + debug("User |$user_id| is unknown but may be a new user from an LMS via LTI. " . "Saw a value for $key About to return a 1"); return 1; #This may be a new user coming in from a LMS via LTI. } @@ -297,7 +297,7 @@ sub check_user { return 0; } - unless ($ce->status_abbrev_has_behavior($User->status, "allow_course_access")) { + unless ($ce->status_abbrev_has_behavior($self->{user}->status, "allow_course_access")) { $self->{log_error} .= "$user_id - course access denied"; $self->{error} = $c->maketext("Authentication failed. Please speak to your instructor."); return 0; @@ -352,9 +352,7 @@ sub authenticate { debug("LTIAdvanced::authenticate called for user |$user|"); debug "ref(c) = |" . ref($c) . "|"; - my $ce = $c->ce; - my $db = $c->db; - my $courseName = $c->ce->{'courseName'}; + my $ce = $c->ce; # Check nonce to see whether request is legitimate debug("Nonce = |" . $self->{oauth_nonce} . "|"); @@ -437,7 +435,7 @@ sub authenticate { my $userID = $self->{user_id}; - if (!$db->existsUser($userID)) { # New User. Create User record + if (!$self->{user}) { # New User. Create User record if ($ce->{block_lti_create_user}) { $self->{log_error} .= "Account creation blocked by block_lti_create_user setting. Did not create user $userID."; @@ -576,6 +574,7 @@ sub create_user { } $db->addUser($newUser); + $self->{user} = $newUser; $self->write_log_entry("New user $userID added via LTIAdvanced login"); # Assign permssion level @@ -641,7 +640,6 @@ sub maybe_update_user { my $db = $c->db; my $courseName = $c->ce->{'courseName'}; - my $user = $db->getUser($userID); my $permissionLevel = $db->getPermissionLevel($userID); # We don't alter records of users with too high a permission if (defined($permissionLevel->permission) @@ -676,10 +674,10 @@ sub maybe_update_user { my $change_made = 0; for my $element (@elements) { - if ($user->$element ne $tempUser->$element) { + if ($self->{user}->$element ne $tempUser->$element) { $change_made = 1; warn "WeBWorK User has $element: " - . $user->$element + . $self->{user}->$element . " but LMS user has $element " . $tempUser->$element . "\n" if ($ce->{debug_lti_parameters}); diff --git a/lib/WeBWorK/Authen/LTIAdvantage.pm b/lib/WeBWorK/Authen/LTIAdvantage.pm index 9c13bd7a4e..8be03a7c7a 100644 --- a/lib/WeBWorK/Authen/LTIAdvantage.pm +++ b/lib/WeBWorK/Authen/LTIAdvantage.pm @@ -236,14 +236,14 @@ sub check_user ($self) { return 0; } - my $User = $db->getUser($user_id); + $self->{user} = $db->getUser($user_id); - if (!$User) { - debug("User |$user_id| is unknown but may be an new user from an LMS via LTI."); + if (!$self->{user}) { + debug("User |$user_id| is unknown but may be a new user from an LMS via LTI."); return 1; } - unless ($ce->status_abbrev_has_behavior($User->status, 'allow_course_access')) { + unless ($ce->status_abbrev_has_behavior($self->{user}->status, 'allow_course_access')) { $self->{log_error} .= "$user_id - course access denied"; $self->{error} = $c->maketext('Authentication failed. Please speak to your instructor.'); return 0; @@ -291,11 +291,9 @@ sub authenticate ($self) { # The actual authentication for this module has already been done. This just creates and updates users if needed. - my $ce = $c->ce; - my $db = $c->db; - my $courseName = $c->ce->{courseName}; + my $ce = $c->ce; - if (!$db->existsUser($self->{user_id})) { + if (!$self->{user}) { # New User. Create User record. if ($ce->{block_lti_create_user}) { $self->{log_error} .= @@ -416,6 +414,7 @@ sub create_user ($self) { $ce->{LTI}{v1p3}{modify_user}($self, $newUser) if ref($ce->{LTI}{v1p3}{modify_user}) eq 'CODE'; $db->addUser($newUser); + $self->{user} = $newUser; $self->write_log_entry("New user $userID added via LTIAdvantange login"); # Set permission level. @@ -481,7 +480,6 @@ sub maybe_update_user ($self) { my $userID = $self->{user_id}; my $courseName = $ce->{courseName}; - my $user = $db->getUser($userID); my $permissionLevel = $db->getPermissionLevel($userID); # We don't alter records of users with too high a permission. @@ -507,10 +505,10 @@ sub maybe_update_user ($self) { my $change_made = 0; for my $element (qw(last_name first_name email_address status section recitation student_id)) { - if ($user->$element ne $tempUser->$element) { + if ($self->{user}->$element ne $tempUser->$element) { $change_made = 1; warn "WeBWorK User has $element: " - . $user->$element + . $self->{user}->$element . " but LMS user has $element " . $tempUser->$element . "\n" if ($ce->{debug_lti_parameters});