Skip to content

Validate user login capability on each request. #2883

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
drgrice1 wants to merge 1 commit into
base: develop
Choose a base branch
from
+45 −38
Open

Validate user login capability on each request. #2883

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 27 additions & 16 deletions lib/WeBWorK/Authen.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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};
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down
20 changes: 9 additions & 11 deletions lib/WeBWorK/Authen/LTIAdvanced.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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});
Expand All @@ -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.
}
Expand All @@ -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;
Expand Down Expand Up @@ -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} . "|");
Expand Down Expand Up @@ -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.";
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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});
Expand Down
20 changes: 9 additions & 11 deletions lib/WeBWorK/Authen/LTIAdvantage.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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} .=
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -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});
Expand Down