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.

Author: drgrice1

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;

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});

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});