From 374eebee3ab60405fb113a594284ff4cafe6ed2b Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Thu, 13 Feb 2025 20:34:06 -0600 Subject: [PATCH] Store the set level proctor password in the set Set level proctoring previously required a fake "set_id:" user with a login_proctor permission level and a hashed password. That user had to be created, deleted, and copied alongside the set, and filtered out of user lists throughout the code. Replace the restricted_login_proctor set field with restricted_login_password, which holds the password in plain text. A set uses a set level proctor password if and only if this field is non-empty. The password is shown to the instructor on the set detail page and copied with the set. The password is now an ordinary set field on the set detail page, and so it can be overridden to set a different password for individual users. That was not possible with the fake user. This also fixes saving a value of 0 for any set field when editing for users. Previously 0 was treated as empty, and so the user's value was reset to the set default. The proctor login form now posts a set_level_proctor flag instead of the fake user ID. When a set has a password, only that password can authorize starting the test, and other user credentials are rejected. This means that when getting all users from the database, it is no longer necessary to filter on users that are not like `set_id:%`. When a course database is upgraded and the restricted_login_password field is added to the set table, the password of each "set_id:" user is copied into restricted_login_password for sets that have restricted_login_proctor set to "Yes", and the "set_id:" users are deleted. Those passwords are crypt hashes, so the proctor login also accepts a password that matches a hashed set level proctor password. On the set detail page a hashed password is shown as "********", which keeps the hashed password if it is left unchanged. User IDs of the form "set_id:" are no longer valid. Since check_user_id croaks on an invalid user ID rather than returning false, the class list imports now catch that so that lines with invalid user IDs are skipped instead of aborting the import. Co-Authored-By: Claude Opus 5.5 --- .typos.toml | 1 + bin/dump-past-answers.pl | 2 +- bin/importClassList.pl | 2 +- lib/Caliper/Entity.pm | 2 +- .../WeBWorK/Tasks/LMSRosterSync.pm | 4 +- lib/WeBWorK/Authen/Proctor.pm | 112 ++++++++++----- .../ContentGenerator/API/CourseActions.pm | 2 +- .../ContentGenerator/API/SetActions.pm | 2 +- .../AchievementsLeaderboard.pm | 7 +- lib/WeBWorK/ContentGenerator/GatewayQuiz.pm | 6 +- lib/WeBWorK/ContentGenerator/Hardcopy.pm | 2 +- .../Instructor/AchievementList.pm | 2 +- .../Instructor/AchievementUserEditor.pm | 2 +- .../ContentGenerator/Instructor/Assigner.pm | 3 +- .../ContentGenerator/Instructor/Index.pm | 5 +- .../Instructor/ProblemSetDetail.pm | 132 +++++++----------- .../Instructor/ProblemSetList.pm | 16 +-- .../ContentGenerator/Instructor/Scoring.pm | 2 +- .../ContentGenerator/Instructor/SendMail.pm | 6 +- .../Instructor/ShowAnswers.pm | 3 +- .../ContentGenerator/Instructor/Stats.pm | 11 +- .../Instructor/StudentProgress.pm | 10 +- .../ContentGenerator/Instructor/UserList.pm | 14 +- .../Instructor/UsersAssignedToSet.pm | 3 +- lib/WeBWorK/DB.pm | 12 +- lib/WeBWorK/DB/Record/Set.pm | 2 +- lib/WeBWorK/DB/Record/UserSet.pm | 2 +- lib/WeBWorK/HTML/ScrollingRecordList.pm | 3 +- lib/WeBWorK/HTML/StudentNav.pm | 4 +- lib/WeBWorK/Utils.pm | 12 ++ lib/WeBWorK/Utils/CourseDBIntegrityCheck.pm | 64 ++++++++- lib/WeBWorK/Utils/CourseManagement.pm | 17 +-- lib/WeBWorK/Utils/Instructor.pm | 2 +- ...tricted_login_proctor_password_row.html.ep | 36 ----- .../ContentGenerator/LoginProctor.html.ep | 8 +- 35 files changed, 256 insertions(+), 257 deletions(-) delete mode 100644 templates/ContentGenerator/Instructor/ProblemSetDetail/restricted_login_proctor_password_row.html.ep diff --git a/.typos.toml b/.typos.toml index 44a547f652..a4bad401be 100644 --- a/.typos.toml +++ b/.typos.toml @@ -30,6 +30,7 @@ crypted = "crypted" crypted_ = "crypted_" cryptedPassword = "cryptedPassword" crypted_password = "crypted_password" +isCryptedPassword = "isCryptedPassword" # Variable name for the paired select-all checkboxes. pairedSelectAlls = "pairedSelectAlls" # Function name (write JSON to file). diff --git a/bin/dump-past-answers.pl b/bin/dump-past-answers.pl index 49ec33863b..9d86a90466 100755 --- a/bin/dump-past-answers.pl +++ b/bin/dump-past-answers.pl @@ -144,7 +144,7 @@ sub write_past_answers_csv { $row[1] = $courseID; - my @users = $db->getUsersWhere({ user_id => { not_like => 'set_id:%' } }); + my @users = $db->getUsersWhere; for my $user (@users) { my $userID = $user->user_id; diff --git a/bin/importClassList.pl b/bin/importClassList.pl index e175e31fce..40efabbb03 100755 --- a/bin/importClassList.pl +++ b/bin/importClassList.pl @@ -74,7 +74,7 @@ sub importUsersFromCSV { print "Saw user_id = $user_id\n"; - unless (WeBWorK::DB::check_user_id($user_id)) { # try to catch lines with bad characters + unless (eval { WeBWorK::DB::check_user_id($user_id) }) { # try to catch lines with bad characters push @skipped, $user_id; next; } diff --git a/lib/Caliper/Entity.pm b/lib/Caliper/Entity.pm index f9fd191cd6..7739bccde1 100644 --- a/lib/Caliper/Entity.pm +++ b/lib/Caliper/Entity.pm @@ -131,7 +131,7 @@ sub problem_set ($c, $set_id) { time_limit_cap => $problem_set->time_limit_cap, restrict_ip => $problem_set->restrict_ip, relax_restrict_ip => $problem_set->relax_restrict_ip, - restricted_login_proctor => $problem_set->restricted_login_proctor, + restricted_login_password => $problem_set->restricted_login_password, hide_hint => $problem_set->hide_hint, restrict_prob_progression => $problem_set->restrict_prob_progression } diff --git a/lib/Mojolicious/WeBWorK/Tasks/LMSRosterSync.pm b/lib/Mojolicious/WeBWorK/Tasks/LMSRosterSync.pm index 94203be472..7482f326b7 100644 --- a/lib/Mojolicious/WeBWorK/Tasks/LMSRosterSync.pm +++ b/lib/Mojolicious/WeBWorK/Tasks/LMSRosterSync.pm @@ -86,7 +86,7 @@ async sub synchronizeRoster ($job) { my (@messages, @addedUsers, @userAchievementRecordsToAdd, @globalAchievementRecordsToAdd, %usersInLMSCourse); my $updatedUsers = 0; - my %users = map { $_->user_id => $_ } $db->getUsersWhere({ user_id => { not_like => 'set_id:%' } }); + my %users = map { $_->user_id => $_ } $db->getUsersWhere; my @achievements = $db->getAchievementsWhere({ enabled => 1 }, ['achievement_id']); @@ -263,7 +263,7 @@ async sub synchronizeRoster ($job) { if @globalAchievementRecordsToAdd; my %permissionLevels = - map { $_->user_id => $_->permission } $db->getPermissionLevelsWhere({ user_id => { not_like => 'set_id:%' } }); + map { $_->user_id => $_->permission } $db->getPermissionLevelsWhere; # Mark all users not in the LMS roster and at or below the LTIAccountCreationCutoff as dropped. my @droppedUsers; diff --git a/lib/WeBWorK/Authen/Proctor.pm b/lib/WeBWorK/Authen/Proctor.pm index c72ecede03..26e9063941 100644 --- a/lib/WeBWorK/Authen/Proctor.pm +++ b/lib/WeBWorK/Authen/Proctor.pm @@ -10,8 +10,9 @@ WeBWorK::Authen::Proctor - Authenticate gateway test proctors. use strict; use warnings; -use WeBWorK::Utils qw(x); +use WeBWorK::Utils qw(x utf8Crypt isCryptedPassword); use WeBWorK::Utils::Sets qw(test_version_is_complete); +use WeBWorK::Debug qw(debug); use constant GENERIC_ERROR_MESSAGE => x('Invalid user ID or password.'); @@ -68,37 +69,34 @@ sub verify { return $self->SUPER::verify(@_); } -# This is similar to the method in the base class, with these differences: -# 1. no guest logins -# 2. no cookie -# 3. no session key -# 4. user_id/password come from POST request params proctor_user/proctor_passwd sub get_credentials { my ($self) = @_; my $c = $self->{c}; - my $set_id = $c->stash('setID'); - - if (defined $c->req->body_params->param('proctor_user')) { - $self->{user_id} = $c->req->body_params->param('proctor_user'); - $self->{user_id} = "set_id:$set_id" if $self->{user_id} eq $set_id; + if (defined $c->req->body_params->param('proctor_user') || $c->req->body_params->param('set_level_proctor')) { + $self->{user_id} = $c->req->body_params->param('proctor_user') // ''; + $self->{set_level_proctor} = 1 if $c->req->body_params->param('set_level_proctor'); $self->{password} = $c->req->body_params->param('proctor_passwd'); $self->{login_type} = $c->req->body_params->param('submitAnswers') ? 'proctor_grading' : 'proctor_login'; $self->{credential_source} = 'params'; + debug('proctor credential source: "params", ', + $self->{set_level_proctor} ? 'using set level proctor' : qq{user: "$self->{user_id}"}); return 1; } elsif ($c->authen->session('proctor_authorization_granted') && !$c->req->body_params->param('submitAnswers')) { $self->{login_type} = 'proctor_login'; $self->{credential_source} = 'session'; + debug('proctor credential source: "session"'); return 1; } return 0; } -# If proctor authorization is granted a proctor user is not needed. So skip checking the user. +# If proctor authorization is granted or if a set level proctor password is used then a proctor user is not needed. +# So skip checking the user. sub check_user { my $self = shift; - return 1 if $self->{credential_source} eq 'session'; + return 1 if $self->{credential_source} eq 'session' || $self->{set_level_proctor}; return $self->SUPER::check_user; } @@ -136,15 +134,9 @@ sub verify_normal_user { my $db = $c->db; my $authz = $c->authz; - my $user_id = $self->{user_id}; - - # Prepended "set_id:" for set-level authentication. - my $show_user_id = $user_id; - $show_user_id =~ s/^set_id://; - # A proctor user may have the Proctor status which does not have the course_access behavior. # So don't check that here. However, the user must be able to login. - unless ($authz->hasPermissions($user_id, 'login')) { + unless ($self->{set_level_proctor} || $authz->hasPermissions($self->{user_id}, 'login')) { $self->{log_error} = 'user not permitted to login'; $self->{error} = $c->maketext(GENERIC_ERROR_MESSAGE); return 0; @@ -153,11 +145,11 @@ sub verify_normal_user { # As mentioned above the setID will not have the set version number if a new version is being opened. That # will be added to the proctor session key by the GatewayQuiz module later. if ($self->{login_type} eq 'proctor_grading') { - unless ($authz->hasPermissions($user_id, 'proctor_quiz_grade')) { + unless ($authz->hasPermissions($self->{user_id}, 'proctor_quiz_grade')) { $self->{log_error} = 'user not permitted to proctor test grade submissions'; $self->{error} = $c->maketext('User [_1] is not authorized to proctor test grade submissions in this course.', - $show_user_id); + $self->{user_id}); return 0; } $c->authen->session('proctor_authorization_granted' => $c->stash('setID')); @@ -166,32 +158,37 @@ sub verify_normal_user { # A UserSet is needed to determine if it is configured to skip grade proctor authorization. Require a # grade_proctor permission level to start a quiz that skips authorization to grade it. This ensures that # a grade proctor level of authorization is always required. - my $userSet = $db->getMergedSet($c->param('effectiveUser'), $c->stash('setID')); - unless ( - $authz->hasPermissions($user_id, 'proctor_quiz_grade') - || (($userSet->use_grade_auth_proctor eq 'Yes' || $userSet->restricted_login_proctor eq 'Yes') - && $authz->hasPermissions($user_id, 'proctor_quiz_login')) + my $userSet = $self->{userSet} // $db->getMergedSet($c->param('effectiveUser'), $c->stash('setID')); + + # Only accept the set level proctor password if set, and reject any other credentials. + if ($userSet->restricted_login_password ne '' && !$self->{set_level_proctor}) { + $self->{log_error} = 'invalid set password to start quiz.'; + $self->{error} = $c->maketext(GENERIC_ERROR_MESSAGE); + return 0; + } + + if ( + $userSet->restricted_login_password eq '' + && !$authz->hasPermissions($self->{user_id}, 'proctor_quiz_grade') + && ($userSet->use_grade_auth_proctor eq 'No' + || !$authz->hasPermissions($self->{user_id}, 'proctor_quiz_login')) + ) { - # Set the error based on if a single set password was required, a grade - # proctor was required to start, or a login proctor was required. - if ($userSet->restricted_login_proctor eq 'Yes') { - $self->{log_error} = 'invalid set password to start quiz.'; - $self->{error} = - $c->maketext('This quiz requires a set password to start, and the password was invalid.'); - } elsif ($userSet->use_grade_auth_proctor ne 'Yes') { + # Set the error based on if a grade proctor was required to start, or a login proctor was required. + if ($userSet->use_grade_auth_proctor eq 'No') { $self->{log_error} = 'grade proctor required to login and user is not permitted to proctor quiz grading.'; $self->{error} = $c->maketext( 'This quiz requires a grade proctor to start, and user [_1] is ' . 'not authorized to proctor test grade submissions in this course.', - $show_user_id + $self->{user_id} ); } else { $self->{log_error} = 'user not permitted to proctor quiz logins.'; $self->{error} = - $c->maketext("User [_1] is not authorized to proctor test logins in this course.", - $show_user_id); + $c->maketext('User [_1] is not authorized to proctor test logins in this course.', + $self->{user_id}); } return 0; } @@ -203,7 +200,7 @@ sub verify_normal_user { delete $c->authen->session->{'proctor_authorization_granted'}; delete $c->authen->session->{'proctor_authorization_version'}; if ($auth_result == 0) { - $self->{log_error} = "authentication failed"; + $self->{log_error} = 'authentication failed'; $self->{error} = $c->maketext(GENERIC_ERROR_MESSAGE); } return 0; @@ -211,6 +208,45 @@ sub verify_normal_user { } } +# This checks the password for the set level proctor if the set is configured for that. +# Otherwise it checks the password using the base class. +sub authenticate { + my $self = shift; + + return -1 unless defined $self->{password}; + + if ($self->{set_level_proctor}) { + my $c = $self->{c}; + + # First verify that this set is configured to use a set level proctor. + my $userSet = $c->db->getMergedSet($c->param('effectiveUser'), $c->stash('setID')); + if (!$userSet || $userSet->restricted_login_password eq '') { + $self->write_log_entry('SET LEVEL PROCTOR: set does not have a set level proctor password'); + return 0; + } + + debug('Checking set level proctor password.'); + my $setPassword = $userSet->restricted_login_password; + my $password = $self->{password} =~ s/^\s*|\s*$//gr; + + # Set level proctor passwords migrated from courses created before these passwords were stored in the set are + # crypt hashes. + if ($setPassword eq $password + || (isCryptedPassword($setPassword) && utf8Crypt($password, $setPassword) eq $setPassword)) + { + $self->write_log_entry('SET LEVEL PROCTOR: password accepted'); + $self->{userSet} = $userSet; + return 1; + } else { + $self->write_log_entry('SET LEVEL PROCTOR: password rejected'); + return 0; + } + } + + debug('Checking proctor password.'); + return $self->SUPER::authenticate; +} + # This is similar to the method in the base class, except that the parameters # proctor_user and proctor_passwd are used and there is no session_key. sub set_params { diff --git a/lib/WeBWorK/ContentGenerator/API/CourseActions.pm b/lib/WeBWorK/ContentGenerator/API/CourseActions.pm index 9d530f1744..5d81e2b812 100644 --- a/lib/WeBWorK/ContentGenerator/API/CourseActions.pm +++ b/lib/WeBWorK/ContentGenerator/API/CourseActions.pm @@ -81,7 +81,7 @@ sub listUsers ($c) { my $db = $c->db; my $ce = $c->ce; - my @userInfo = map { unbless($_) } $db->getUsersWhere({ user_id => { not_like => 'set_id:%' } }); + my @userInfo = map { unbless($_) } $db->getUsersWhere; my $numGlobalSets = $db->countGlobalSets; for my $user (@userInfo) { diff --git a/lib/WeBWorK/ContentGenerator/API/SetActions.pm b/lib/WeBWorK/ContentGenerator/API/SetActions.pm index 10008da41b..e3356b2f89 100644 --- a/lib/WeBWorK/ContentGenerator/API/SetActions.pm +++ b/lib/WeBWorK/ContentGenerator/API/SetActions.pm @@ -115,7 +115,7 @@ sub updateSetProperties ($c) { $set->time_limit_cap($c->req->param('time_limit_cap')); $set->restrict_ip($c->req->param('restrict_ip')); $set->relax_restrict_ip($c->req->param('relax_restrict_ip')); - $set->restricted_login_proctor($c->req->param('restricted_login_proctor')); + $set->restricted_login_password($c->req->param('restricted_login_password')); $db->putGlobalSet($set); diff --git a/lib/WeBWorK/ContentGenerator/AchievementsLeaderboard.pm b/lib/WeBWorK/ContentGenerator/AchievementsLeaderboard.pm index 9be5a882f5..adf9b29ce0 100644 --- a/lib/WeBWorK/ContentGenerator/AchievementsLeaderboard.pm +++ b/lib/WeBWorK/ContentGenerator/AchievementsLeaderboard.pm @@ -25,18 +25,17 @@ sub initialize ($c) { return unless $c->authz->hasPermissions($c->{userName}, 'view_leaderboard'); - # Get list of all users (except set-level proctors) and achievements. + # Get list of all users and achievements. my @achievements = sortAchievements($db->getAchievementsWhere); my %achievementsById = map { $_->achievement_id => $_ } @achievements; my %globalUserAchievements = - map { $_->user_id => $_ } $db->getGlobalUserAchievementsWhere({ user_id => { not_like => 'set_id:%' } }); + map { $_->user_id => $_ } $db->getGlobalUserAchievementsWhere; $c->{showUserNames} = $c->authz->hasPermissions($c->{userName}, 'view_leaderboard_usernames'); $c->{showLevels} = 0; # Hide level column unless at least one user has a level achievement. my %allUserAchievements; for ($db->getUserAchievementsWhere({ - user_id => { not_like => 'set_id:%' }, achievement_id => [ map { $_->achievement_id } grep { $_->category ne 'level' } @achievements ], })) { @@ -44,7 +43,7 @@ sub initialize ($c) { } my @rows; - for my $user ($db->getUsersWhere({ user_id => { not_like => 'set_id:%' } })) { + for my $user ($db->getUsersWhere) { # Only include users who can be shown in stats. next unless $ce->status_abbrev_has_behavior($user->status, 'include_in_stats'); diff --git a/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm b/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm index 6099a44e32..2647606547 100644 --- a/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm +++ b/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm @@ -1334,10 +1334,8 @@ sub nav ($c, $args) { my %users = map { $_->[0] => 1 } @userVersions; my %allUserRecords = - map { $_->user_id => $_ } - grep { $users{ $_->user_id } } - $c->db->getUsersWhere( - { -and => { user_id => { not_like => 'set_id:%' } }, user_id => { '!=' => $userID } }); + map { $_->user_id => $_ } + grep { $users{ $_->user_id } } $c->db->getUsersWhere({ user_id => { '!=' => $userID } }); my $filter = $c->param('studentNavFilter'); diff --git a/lib/WeBWorK/ContentGenerator/Hardcopy.pm b/lib/WeBWorK/ContentGenerator/Hardcopy.pm index 2148619092..e499d008d6 100644 --- a/lib/WeBWorK/ContentGenerator/Hardcopy.pm +++ b/lib/WeBWorK/ContentGenerator/Hardcopy.pm @@ -356,7 +356,7 @@ sub display_form ($c) { if ($perm_multiuser && $perm_multiset) { # Get all users for selection. - @users = $db->getUsersWhere({ user_id => { not_like => 'set_id:%' } }); + @users = $db->getUsersWhere; # Get sets for selection. # Note that we are getting GlobalSets instead of using the list of UserSets assigned to the diff --git a/lib/WeBWorK/ContentGenerator/Instructor/AchievementList.pm b/lib/WeBWorK/ContentGenerator/Instructor/AchievementList.pm index 406b4ce4b4..dbbe54bb47 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/AchievementList.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/AchievementList.pm @@ -236,7 +236,7 @@ sub score_handler ($c) { # First get everything that is needed from the database. my @achievements = sortAchievements($db->getAchievements(@achievementsToScore)); - my @users = $db->getUsersWhere({ user_id => { not_like => 'set_id:%' } }, [qw(section last_name)]); + my @users = $db->getUsersWhere({}, [qw(section last_name)]); my %globalUserAchievements = map { $_->user_id => $_ } $db->getGlobalUserAchievementsWhere; diff --git a/lib/WeBWorK/ContentGenerator/Instructor/AchievementUserEditor.pm b/lib/WeBWorK/ContentGenerator/Instructor/AchievementUserEditor.pm index fa407fa750..099bf0ea74 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/AchievementUserEditor.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/AchievementUserEditor.pm @@ -22,7 +22,7 @@ sub initialize ($c) { return unless $authz->hasPermissions($user, 'edit_achievements'); $c->stash->{userRecords} = - [ $db->getUsersWhere({ user_id => { not_like => 'set_id:%' } }, [qw/section last_name first_name/]) ]; + [ $db->getUsersWhere({}, [qw/section last_name first_name/]) ]; $c->stash->{userAchievementRecords} = { map { $_->user_id => $_ } $db->getUserAchievementsWhere({ achievement_id => $achievementID }) }; diff --git a/lib/WeBWorK/ContentGenerator/Instructor/Assigner.pm b/lib/WeBWorK/ContentGenerator/Instructor/Assigner.pm index 699e9d9214..1139bedfba 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/Assigner.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/Assigner.pm @@ -23,11 +23,10 @@ sub initialize ($c) { unless $authz->hasPermissions($user, 'access_instructor_tools') && $authz->hasPermissions($user, 'assign_problem_sets'); - # Get all users except the set level proctors, and restrict to the sections or recitations that are allowed for the + # Get all users, and restrict to the sections or recitations that are allowed for the # user if such restrictions are defined. $c->stash->{users} = [ $db->getUsersWhere({ - user_id => { not_like => 'set_id:%' }, $ce->{viewable_sections}{$user} || $ce->{viewable_recitations}{$user} ? ( -or => [ diff --git a/lib/WeBWorK/ContentGenerator/Instructor/Index.pm b/lib/WeBWorK/ContentGenerator/Instructor/Index.pm index 5492dc0b26..fa7b2f95f9 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/Index.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/Index.pm @@ -162,12 +162,11 @@ sub initialize ($c) { return; } - # Get all users except the set level proctors, and restrict to the sections or recitations that are allowed for the - # user if such restrictions are defined. This list is sorted by last_name, then first_name, then user_id. + # Get all users, and restrict to the sections or recitations that are allowed for the user if such restrictions are + # defined. This list is sorted by last_name, then first_name, then user_id. $c->stash->{users} = [ $db->getUsersWhere( { - user_id => { not_like => 'set_id:%' }, $ce->{viewable_sections}{$userID} || $ce->{viewable_recitations}{$userID} ? ( -or => [ diff --git a/lib/WeBWorK/ContentGenerator/Instructor/ProblemSetDetail.pm b/lib/WeBWorK/ContentGenerator/Instructor/ProblemSetDetail.pm index 4d859c4a8f..4880a77d7e 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/ProblemSetDetail.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/ProblemSetDetail.pm @@ -10,7 +10,7 @@ specific user/set information as well as problem information use Exporter qw(import); -use WeBWorK::Utils qw(cryptPassword x); +use WeBWorK::Utils qw(x isCryptedPassword); use WeBWorK::Utils::Files qw(surePathToFile readFile); use WeBWorK::Utils::Instructor qw(assignProblemToAllSetUsers addProblemToSet); use WeBWorK::Utils::JITAR qw(seq_to_jitar_id jitar_id_to_seq); @@ -23,7 +23,8 @@ our @EXPORT_OK = qw(FIELD_PROPERTIES); use constant SET_FIELDS => [ qw(set_header hardcopy_header open_date reduced_scoring_date due_date answer_date visible description enable_reduced_scoring restricted_release restricted_status restrict_ip relax_restrict_ip - assignment_type use_grade_auth_proctor attempts_per_version version_time_limit time_limit_cap + assignment_type use_grade_auth_proctor restricted_login_password attempts_per_version version_time_limit + time_limit_cap versions_per_interval time_interval problem_randorder problems_per_page hide_score:hide_score_by_problem hide_work hide_hint restrict_prob_progression email_instructor) ]; @@ -435,6 +436,20 @@ use constant FIELD_PROPERTIES => { . 'authorization or a password specific to this set to start and no authorization to grade.' ), }, + restricted_login_password => { + name => x('Password'), + type => 'edit', + size => 10, + override => 'any', + help_text => x( + 'Provide a password to have a password students must enter to start a proctored test. Alternatively, ' + . 'leave this blank if you would like to have a proctor level user enter their username and ' + . q{password on the student's screen for authentication. When editing the set for specific } + . 'users, a different password can be set for those users. Note that if the password is changed ' + . 'after students have started the test, then the new password will be needed if a student needs ' + . 'to be authorized again to continue the test.' + ), + }, restrict_prob_progression => { name => x('Restrict Problem Progression'), type => 'choose', @@ -467,11 +482,8 @@ use constant FIELD_PROPERTIES => { ), }, - # In addition to the set fields above, there are a number of things - # that are set but aren't in this table: - # any set proctor information (which is in the user tables), and - # any set location restriction information (which is in the - # location tables) + # In addition to the set fields above, any set location restriction information is set but isn't in this table + # (it is in the location tables). # Problem information source_file => { @@ -637,7 +649,6 @@ sub fieldTable ($c, $userID, $setID, $problemID, $globalRecord, $userRecord = un my $ipFields = ''; my $numLocations = 0; - # Needed for set-level proctor my $proctorFields = ''; my @fieldOrder; @@ -861,6 +872,13 @@ sub fieldHTML ($c, $userID, $setID, $problemID, $globalRecord, $userRecord, $fie $globalValue = $properties{default}; } + # A password migrated from an older course is a hash that should not be shown, so show ******** instead. That is + # submitted back if left unchanged, and the hashed password is then kept. + if ($field eq 'restricted_login_password') { + $globalValue = '********' if isCryptedPassword($globalValue); + $userValue = '********' if isCryptedPassword($userValue); + } + if ($properties{convertby}) { $globalValue = $globalValue / $properties{convertby} if $globalValue; $userValue = $userValue / $properties{convertby} if $userValue; @@ -1188,10 +1206,14 @@ sub extraSetFields ($c, $userID, $setID, $globalRecord, $userRecord, $forUsers) $c->fieldHTML($userID, $setID, undef, $globalRecord, $userRecord, 'use_grade_auth_proctor') )->join('') ), - $forUsers ? '' : $c->include( - 'ContentGenerator/Instructor/ProblemSetDetail/restricted_login_proctor_password_row', - globalRecord => $globalRecord, - proctorClass => $proctorClass + # The password is never read from a set version, so don't show it when editing set versions. + $forUsers && $userRecord->version_id ? '' : $c->tag( + 'tr', + class => $proctorClass, + $c->c( + map { $c->tag('td', $_) } + $c->fieldHTML($userID, $setID, undef, $globalRecord, $userRecord, 'restricted_login_password') + )->join('') ) )->join(''); } @@ -1481,10 +1503,6 @@ sub initialize ($c) { # Save general set information (including headers) if ($forUsers) { - # Note that we don't deal with the proctor user fields here, with the assumption that it can't be possible - # to change them for users. - # FIXME: This is not the most robust treatment of the problem - my @userRecords = $db->getUserSetsWhere({ user_id => [@editForUser], set_id => $setID }); # If editing a set version, we want to edit that instead of the userset, so get it too. my $userSet = $userRecords[0]; @@ -1503,7 +1521,14 @@ sub initialize ($c) { $field eq 'restricted_release' ? join(',', grep {$_} @paramValues) : (@paramValues > 1 && $paramValues[0] eq 'numeric') ? $paramValues[1] : $paramValues[0]; - if ($param && $param ne '') { + + if ($field eq 'restricted_login_password' && defined $param) { + $param =~ s/^\s*|\s*$//g; + # ******** is shown in place of a hashed password, and in that case it is left unchanged. + next if $param eq '********'; + } + + if (defined $param && $param ne '') { $param = $param * $properties{$field}->{convertby} if $properties{$field}{convertby}; # Special case: Does field fill in multiple values? @@ -1589,6 +1614,12 @@ sub initialize ($c) { $param = $param * $properties{$field}->{convertby}; } + if ($field eq 'restricted_login_password') { + $param =~ s/^\s*|\s*$//g; + # ******** is shown in place of a hashed password, and in that case it is left unchanged. + next if $param eq '********'; + } + if ($field =~ /:/) { my @values = split(/:/, $param); my @fields = split(/:/, $field); @@ -1613,6 +1644,8 @@ sub initialize ($c) { )); } + $setRecord->restricted_login_password('') if $setRecord->assignment_type ne 'proctored_gateway'; + $db->putGlobalSet($setRecord); if ($ce->{LTIVersion} @@ -1652,71 +1685,6 @@ sub initialize ($c) { $db->deleteGlobalSetLocation($setID, $_); } } - - # Save proctored problem proctor user information - if ($c->param("set.$setID.restricted_login_proctor_password") - && $setRecord->assignment_type eq 'proctored_gateway') - { - # In this case add a set-level proctor or update the password. - my $procID = "set_id:$setID"; - my $pass = $c->param("set.$setID.restricted_login_proctor_password"); - # Should we carefully check in this case that the user and password exist? The code in the add stanza - # is pretty careful to be sure that there's a one-to-one correspondence between the existence of the - # user and the setting of the set restricted_login_proctor field, so we assume that just checking the - # latter here is sufficient. - if ($setRecord->restricted_login_proctor eq 'Yes') { - if ($pass ne '********') { - # A new password was submitted. So save it. - my $dbPass = eval { $db->getPassword($procID) }; - if ($@) { - $c->addbadmessage($c->maketext( - 'Error getting old set-proctor password from the database: [_1]. ' - . 'No update to the password was done.', - $@ - )); - } else { - $dbPass->password(cryptPassword($pass)); - $db->putPassword($dbPass); - } - } - } else { - $setRecord->restricted_login_proctor('Yes'); - my $procUser = $db->newUser(); - $procUser->user_id($procID); - $procUser->last_name("Proctor"); - $procUser->first_name("Login"); - $procUser->student_id("loginproctor"); - $procUser->status($ce->status_name_to_abbrevs('Proctor')); - my $procPerm = $db->newPermissionLevel; - $procPerm->user_id($procID); - $procPerm->permission($ce->{userRoles}{login_proctor}); - my $procPass = $db->newPassword; - $procPass->user_id($procID); - $procPass->password(cryptPassword($pass)); - - eval { $db->addUser($procUser) }; - if ($@) { - $c->addbadmessage($c->maketext("Error adding set-level proctor: [_1]", $@)); - } else { - $db->addPermissionLevel($procPerm); - $db->addPassword($procPass); - } - - # Set the restricted_login_proctor set field - $db->putGlobalSet($setRecord); - } - - } else { - # If the parameter isn't set, or if the assignment type is not 'proctored_gateway', then ensure that - # there is no set-level proctor defined. - if ($setRecord->restricted_login_proctor eq 'Yes') { - - $setRecord->restricted_login_proctor('No'); - $db->deleteUser("set_id:$setID"); - $db->putGlobalSet($setRecord); - - } - } } # Save problem information diff --git a/lib/WeBWorK/ContentGenerator/Instructor/ProblemSetList.pm b/lib/WeBWorK/ContentGenerator/Instructor/ProblemSetList.pm index b4dc785399..6b87aff5b0 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/ProblemSetList.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/ProblemSetList.pm @@ -469,25 +469,11 @@ sub create_handler ($c) { $db->addGlobalProblem($_); } - # also copy any set_location restrictions and set-level proctor - # information + # Also copy any set_location restrictions. foreach ($db->getAllGlobalSetLocations($oldSetID)) { $_->set_id($newSetID); $db->addGlobalSetLocation($_); } - if ($newSetRecord->restricted_login_proctor eq 'Yes') { - my $procUser = $db->getUser("set_id:$oldSetID"); - $procUser->user_id("set_id:$newSetID"); - eval { $db->addUser($procUser) }; - if (!$@) { - my $procPerm = $db->getPermissionLevel("set_id:$oldSetID"); - $procPerm->user_id("set_id:$newSetID"); - $db->addPermissionLevel($procPerm); - my $procPass = $db->getPassword("set_id:$oldSetID"); - $procPass->user_id("set_id:$newSetID"); - $db->addPassword($procPass); - } - } } # Assign set to current active user. diff --git a/lib/WeBWorK/ContentGenerator/Instructor/Scoring.pm b/lib/WeBWorK/ContentGenerator/Instructor/Scoring.pm index 5db0c635fa..7c92e9f467 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/Scoring.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/Scoring.pm @@ -51,7 +51,7 @@ sub initialize ($c) { # Get all users sorted by last_name, then first_name, then user_id. debug("pre-fetching users"); - my @Users = $db->getUsersWhere({ user_id => { not_like => 'set_id:%' } }, qw/last_name first_name user_id/); + my @Users = $db->getUsersWhere({}, qw/last_name first_name user_id/); my %Users; my @sortedUserIDs; for my $User (@Users) { diff --git a/lib/WeBWorK/ContentGenerator/Instructor/SendMail.pm b/lib/WeBWorK/ContentGenerator/Instructor/SendMail.pm index 81250c9e62..db75e321ac 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/SendMail.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/SendMail.pm @@ -81,11 +81,11 @@ sub initialize ($c) { $c->{preview_user} = $classList[0] && $classList[0] ne $userID ? $db->getUser($classList[0]) : $user; # Gather database data - # Get all users except set level proctors and practice users. If the current user has restrictions on viewable - # sections or recitations, those are filtered out as well. The users are sorted by user_id. + # Get all users except practice users. If the current user has restrictions on viewable sections or recitations, + # those are filtered out as well. The users are sorted by user_id. my @Users = $db->getUsersWhere( { - user_id => [ -and => { not_like => 'set_id:%' }, { not_like => "$ce->{practiceUserPrefix}\%" } ], + user_id => { not_like => "$ce->{practiceUserPrefix}\%" }, $ce->{viewable_sections}{$userID} || $ce->{viewable_recitations}{$userID} ? ( -or => [ diff --git a/lib/WeBWorK/ContentGenerator/Instructor/ShowAnswers.pm b/lib/WeBWorK/ContentGenerator/Instructor/ShowAnswers.pm index 53c780ff01..5f7c0e1342 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/ShowAnswers.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/ShowAnswers.pm @@ -233,10 +233,9 @@ sub getInstructorData ($c) { my $ce = $c->ce; my $user = $c->authen->{user_id}; - # Get all users except the set level proctors, and restrict to the sections or recitations that are allowed for + # Get all users, and restrict to the sections or recitations that are allowed for # the user if such restrictions are defined. my @users = $db->getUsersWhere({ - user_id => { not_like => 'set_id:%' }, $ce->{viewable_sections}{$user} || $ce->{viewable_recitations}{$user} ? ( -or => [ diff --git a/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm b/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm index 9f2aa9679b..749fcc4278 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm @@ -35,15 +35,14 @@ sub initialize ($c) { $c->{problemRecord} = $problemRecord; } - # Cache a list of all users except set level proctors and practice users, and restrict to the sections - # or recitations that are allowed for the user if such restrictions are defined. This list is sorted by - # last_name, then first_name, then user_id. This is used in multiple places in this module, and is used - # on every page except the main page, so it is done here to prevent extra database access. + # Cache a list of all users except practice users, and restrict to the sections or recitations that are allowed + # for the user if such restrictions are defined. This list is sorted by last_name, then first_name, then + # user_id. This is used in multiple places in this module, and is used on every page except the main page, so + # it is done here to prevent extra database access. $c->{student_records} = [ $db->getUsersWhere( { - user_id => - [ -and => { not_like => 'set_id:%' }, { not_like => "$ce->{practiceUserPrefix}\%" } ], + user_id => { not_like => "$ce->{practiceUserPrefix}\%" }, $ce->{viewable_sections}{$user} || $ce->{viewable_recitations}{$user} ? ( -or => [ diff --git a/lib/WeBWorK/ContentGenerator/Instructor/StudentProgress.pm b/lib/WeBWorK/ContentGenerator/Instructor/StudentProgress.pm index 4e6c346d60..38ec0c8afc 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/StudentProgress.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/StudentProgress.pm @@ -21,14 +21,14 @@ sub initialize ($c) { # Check permissions return unless $c->authz->hasPermissions($user, "access_instructor_tools"); - # Cache a list of all users except set level proctors and practice users, and restrict to the sections or - # recitations that are allowed for the user if such restrictions are defined. This list is sorted by last_name, - # then first_name, then user_id. This is used in multiple places in this module, and is guaranteed to be used at - # least once. So it is done here to prevent extra database access. + # Cache a list of all users except practice users, and restrict to the sections or recitations that are allowed for + # the user if such restrictions are defined. This list is sorted by last_name, then first_name, then user_id. This + # is used in multiple places in this module, and is guaranteed to be used at least once. So it is done here to + # prevent extra database access. $c->{student_records} = [ $db->getUsersWhere( { - user_id => [ -and => { not_like => 'set_id:%' }, { not_like => "$ce->{practiceUserPrefix}\%" } ], + user_id => { not_like => "$ce->{practiceUserPrefix}\%" }, $ce->{viewable_sections}{$user} || $ce->{viewable_recitations}{$user} ? ( -or => [ diff --git a/lib/WeBWorK/ContentGenerator/Instructor/UserList.pm b/lib/WeBWorK/ContentGenerator/Instructor/UserList.pm index 154a32dab5..9f14fa0df0 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/UserList.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/UserList.pm @@ -177,14 +177,12 @@ async sub initialize ($c) { return; } - # Get a list of all users except set-level proctors from the database. - my @allUsersDB = $db->getUsersWhere({ user_id => { not_like => 'set_id:%' } }); + # Get a list of all users from the database. + my @allUsersDB = $db->getUsersWhere; - my %permissionLevels = - map { $_->user_id => $_->permission } $db->getPermissionLevelsWhere({ user_id => { not_like => 'set_id:%' } }); + my %permissionLevels = map { $_->user_id => $_->permission } $db->getPermissionLevelsWhere; - my %passwordRecords = - map { $_->user_id => $_ } $db->getPasswordsWhere({ user_id => { not_like => 'set_id:%' } }); + my %passwordRecords = map { $_->user_id => $_ } $db->getPasswordsWhere; # Add permission level, a password exists field, and a must reset password field to the user record hash. for my $user (@allUsersDB) { @@ -733,11 +731,11 @@ sub importUsersFromCSV ($c, $fileName, $replaceExisting, $fallbackPasswordSource my %record = %$record; my $user_id = $record{user_id}; - unless (WeBWorK::DB::check_user_id($user_id)) { # try to catch lines with bad characters + unless (eval { WeBWorK::DB::check_user_id($user_id) }) { # try to catch lines with bad characters push @skipped, $user_id; next; } - if ($user_id eq $c->authen->{user_id}) { # don't replace yourself!! + if ($user_id eq $c->authen->{user_id}) { # don't replace yourself!! push @skipped, $user_id; next; } diff --git a/lib/WeBWorK/ContentGenerator/Instructor/UsersAssignedToSet.pm b/lib/WeBWorK/ContentGenerator/Instructor/UsersAssignedToSet.pm index 60592b6076..003fb93c17 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/UsersAssignedToSet.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/UsersAssignedToSet.pm @@ -36,8 +36,7 @@ sub initialize ($c) { return unless $c->stash->{set_record}; # Get all user records and cache them for later use. - my @userRecords = - $db->getUsersWhere({ user_id => { not_like => 'set_id:%' } }, [qw/section last_name first_name/]); + my @userRecords = $db->getUsersWhere({}, [qw/section last_name first_name/]); my %currentUsers = map { $ce->status_abbrev_has_behavior($_->status, 'include_in_assignment') ? ($_->user_id => 1) : () } @userRecords; diff --git a/lib/WeBWorK/DB.pm b/lib/WeBWorK/DB.pm index 07be455adc..d3bc7527c0 100644 --- a/lib/WeBWorK/DB.pm +++ b/lib/WeBWorK/DB.pm @@ -468,13 +468,12 @@ sub newUser { sub countUsers { return scalar shift->listUsers(@_) } -# Note: This returns a list of user_ids for all users except set level proctors. sub listUsers { my ($self) = shift->checkArgs(\@_); if (wantarray) { - return map {@$_} $self->{user}->get_fields_where(['user_id'], { user_id => { not_like => 'set_id:%' } }); + return map {@$_} $self->{user}->get_fields_where(['user_id']); } else { - return $self->{user}->count_where({ user_id => { not_like => 'set_id:%' } }); + return $self->{user}->count_where; } } @@ -2141,9 +2140,10 @@ sub getAllMergedProblemVersions { # utilities ################################################################################ -sub check_user_id { # (valid characters are [-a-zA-Z0-9_.,@]) +# Valid characters for the user_id column are [-a-zA-Z0-9_.,@]. +sub check_user_id { my $value = shift; - if ($value =~ m/^[-a-zA-Z0-9_.@]*,?(set_id:)?[-a-zA-Z0-9_.@]*(,g)?$/) { + if ($value =~ m/^[-a-zA-Z0-9_.,@]*$/) { return 1; } else { croak "invalid characters in user_id field: '$value' (valid characters are [-a-zA-Z0-9_.,@])"; @@ -2179,7 +2179,7 @@ sub validateKeyfieldValue { . "' (valid characters are [0-9])" unless $value =~ m/^[0-9]*$/; } elsif ($keyfield eq "user_id" || $keyfield eq 'userID') { - check_user_id($value); # (valid characters are [-a-zA-Z0-9_.,]) see above. + check_user_id($value); } elsif ($keyfield eq "ip_mask") { croak "invalid characters in '$keyfield' field: '$value' (valid characters are [-a-zA-Z0-9_.,])" unless $value =~ m/^[-a-fA-F0-9_.:\/]*$/; diff --git a/lib/WeBWorK/DB/Record/Set.pm b/lib/WeBWorK/DB/Record/Set.pm index 741dd58393..fa1fd324a0 100644 --- a/lib/WeBWorK/DB/Record/Set.pm +++ b/lib/WeBWorK/DB/Record/Set.pm @@ -39,7 +39,7 @@ BEGIN { time_limit_cap => { type => "ENUM('0','1')" }, restrict_ip => { type => "ENUM('No','RestrictTo','DenyFrom') DEFAULT 'No'" }, relax_restrict_ip => { type => "ENUM('No','AfterAnswerDate','AfterVersionAnswerDate') DEFAULT 'No'" }, - restricted_login_proctor => { type => "ENUM('No','Yes')" }, + restricted_login_password => { type => "TEXT" }, use_grade_auth_proctor => { type => "ENUM('No','Yes') DEFAULT 'Yes'" }, hide_hint => { type => "INT" }, restrict_prob_progression => { type => "INT" }, diff --git a/lib/WeBWorK/DB/Record/UserSet.pm b/lib/WeBWorK/DB/Record/UserSet.pm index d334358c42..0a2f15ea55 100644 --- a/lib/WeBWorK/DB/Record/UserSet.pm +++ b/lib/WeBWorK/DB/Record/UserSet.pm @@ -42,7 +42,7 @@ BEGIN { time_limit_cap => { type => "ENUM('0','1')" }, restrict_ip => { type => "ENUM('No','RestrictTo','DenyFrom')" }, relax_restrict_ip => { type => "ENUM('No','AfterAnswerDate','AfterVersionAnswerDate')" }, - restricted_login_proctor => { type => "ENUM('No','Yes')" }, + restricted_login_password => { type => "TEXT" }, use_grade_auth_proctor => { type => "ENUM('No','Yes')" }, hide_hint => { type => "INT" }, restrict_prob_progression => { type => "INT" }, diff --git a/lib/WeBWorK/HTML/ScrollingRecordList.pm b/lib/WeBWorK/HTML/ScrollingRecordList.pm index 2c766d8cdf..c4d758837a 100644 --- a/lib/WeBWorK/HTML/ScrollingRecordList.pm +++ b/lib/WeBWorK/HTML/ScrollingRecordList.pm @@ -92,8 +92,7 @@ sub scrollingRecordList ($options, @records) { sub userData ($c, @users) { my %permissionName = reverse %{ $c->ce->{userRoles} }; - my %permissions = map { $_->user_id => $permissionName{ $_->permission } } - $c->db->getPermissionLevelsWhere({ user_id => { not_like => 'set_id:%' } }); + my %permissions = map { $_->user_id => $permissionName{ $_->permission } } $c->db->getPermissionLevelsWhere; my @records = map { { value => $_->user_id, diff --git a/lib/WeBWorK/HTML/StudentNav.pm b/lib/WeBWorK/HTML/StudentNav.pm index be8caf88af..ba1b6affb1 100644 --- a/lib/WeBWorK/HTML/StudentNav.pm +++ b/lib/WeBWorK/HTML/StudentNav.pm @@ -15,9 +15,7 @@ sub studentNav ($c, $setID) { return '' unless $c->authz->hasPermissions($userID, 'become_student'); # Find all users (except the current user) sorted by last_name, then first_name, then user_id. - my @allUserRecords = - $c->db->getUsersWhere({ -and => { user_id => { not_like => 'set_id:%' } }, user_id => { '!=' => $userID } }, - [qw/last_name first_name user_id/]); + my @allUserRecords = $c->db->getUsersWhere({ user_id => { '!=' => $userID } }, [qw/last_name first_name user_id/]); # If $setID is provided, only show users assigned to given set. if ($setID) { diff --git a/lib/WeBWorK/Utils.pm b/lib/WeBWorK/Utils.pm index ea8e583483..22e2e9b32b 100644 --- a/lib/WeBWorK/Utils.pm +++ b/lib/WeBWorK/Utils.pm @@ -22,6 +22,7 @@ our @EXPORT_OK = qw( wwRound cryptPassword utf8Crypt + isCryptedPassword undefstr sortByName sortAchievements @@ -156,6 +157,10 @@ sub utf8Crypt ($clearPassword, $hash) { return $cryptedPassword; } +sub isCryptedPassword ($password) { + return ($password // '') =~ m{^\$[0-9a-z]+\$} ? 1 : 0; +} + sub undefstr ($default, @values) { return map { defined $_ ? $_ : $default } @values[ 0 .. $#values ]; } @@ -707,6 +712,13 @@ called on the UTF-8 encoded version of C<$clearPassword>. Note that C<$hash> can be a salt or a password hash generated by a previous call of this method with a salt.. +=head2 isCryptedPassword + +Usage: C + +Returns 1 if C<$password> looks like a password hash returned by C in the +modular crypt format "$id$salt$hash", and 0 otherwise. + =head2 undefstr Usage: C diff --git a/lib/WeBWorK/Utils/CourseDBIntegrityCheck.pm b/lib/WeBWorK/Utils/CourseDBIntegrityCheck.pm index 4689c89771..1d09997a88 100644 --- a/lib/WeBWorK/Utils/CourseDBIntegrityCheck.pm +++ b/lib/WeBWorK/Utils/CourseDBIntegrityCheck.pm @@ -258,12 +258,13 @@ sub updateTableFields { my $schema_obj = $db->{$table}; - my %newKeyFields; + my (%newFields, %newKeyFields); # Add fields for my $field_name (keys %$fieldStatus) { if ($fieldStatus->{$field_name}[0] == ONLY_IN_A) { if ($schema_obj->can('add_column_field') && $schema_obj->add_column_field($field_name)) { + $newFields{$field_name} = 1; if ($fieldStatus->{$field_name}[1]) { push(@messages, [ "Added primary key column '$field_name' to table '$table'", 1 ]); $newKeyFields{$field_name} = 1; @@ -286,6 +287,13 @@ sub updateTableFields { } } + # This must be done before fields are dropped since it uses the old restricted_login_proctor field. + push(@messages, $self->migrateSetLevelProctorPasswords) + if $table eq 'set' + && $newFields{restricted_login_password} + && $fieldStatus->{restricted_login_proctor} + && $fieldStatus->{restricted_login_proctor}[0] == ONLY_IN_B; + # Drop fields if listed in $delete_field_names. for my $field_name (@$delete_field_names) { if ($fieldStatus->{$field_name} && $fieldStatus->{$field_name}[0] == ONLY_IN_B) { @@ -357,6 +365,60 @@ sub migrateSetIdToSetIdVersionId { return @results; } +# Set level proctor passwords used to be the password of a "set_id:" user, and were used for sets with the +# restricted_login_proctor field set to 'Yes'. Copy the password of that user into the restricted_login_password field +# of the set. Then delete all of these users. +sub migrateSetLevelProctorPasswords { + my $self = shift; + my $db = $self->db; + my $record = $db->{set}; + + my @messages; + + my %passwords = map { substr($_->[0], 7) => $_->[1] } + $db->{password}->get_fields_where([qw(user_id password)], { user_id => { like => 'set_id:%' } }); + + my @fields = $record->fields; + my %values; + @values{@fields} = (0 .. $#fields); + + my @keyfields = $record->keyfields; + my %where; + @where{@keyfields} = map { $values{$_} } @keyfields; + + my ($stmt, @val_order) = $record->sql->update($record->table, \%values); + (substr($stmt, length($stmt), 0), my @where_order) = $record->sql->where(\%where); + + my $rows_i = $record->get_fields_where_i([@fields], { restricted_login_proctor => 'Yes' }); + my $sth = $record->dbh->prepare_cached($stmt, undef, 3); + + until ($rows_i->is_exhausted) { + my $fieldValues = $rows_i->value; + + my $setID = $fieldValues->[ $values{set_id} ]; + my $password = $passwords{$setID}; + next unless defined $password && $password =~ /\S/; + + my @bind_vals = @$fieldValues[@where_order]; + $fieldValues->[ $values{restricted_login_password} ] = $password; + unshift(@bind_vals, @$fieldValues[@val_order]); + + $record->dbh->debug_stmt($sth, @bind_vals); + $sth->execute(@bind_vals); + push(@messages, [ "Migrated the set level proctor password for set '$setID'", 1 ]); + } + $sth->finish; + + my $deleted = 0; + for my $table (qw(user password permission)) { + my $result = $db->{$table}->delete_where({ user_id => { like => 'set_id:%' } }); + $deleted = $result if $table eq 'user'; + } + push(@messages, [ 'Deleted ' . ($deleted + 0) . ' obsolete set level proctor users', 1 ]) if $deleted > 0; + + return @messages; +} + # Database locking utilities # Create a lock named 'webwork.dbugrade' that times out after 10 seconds. diff --git a/lib/WeBWorK/Utils/CourseManagement.pm b/lib/WeBWorK/Utils/CourseManagement.pm index 75b5850149..7d6c568ce4 100644 --- a/lib/WeBWorK/Utils/CourseManagement.pm +++ b/lib/WeBWorK/Utils/CourseManagement.pm @@ -353,8 +353,7 @@ sub addCourse { # as the role might be customized in that file. my @non_student_ids = map {@$_} ($db0->listPermissionLevelsWhere({ - permission => { '!=' => $options{copyConfig} ? $ce0->{userRoles}{student} : $ce->{userRoles}{student} }, - user_id => { not_like => 'set_id:%' } + permission => { '!=' => $options{copyConfig} ? $ce0->{userRoles}{student} : $ce->{userRoles}{student} } })); for my $user_id (@non_student_ids) { @@ -395,20 +394,6 @@ sub addCourse { eval { $db->addGlobalSetLocation($location) }; warn $@ if $@; } - - # Copy the set level proctor user for this set if there is one (despite the for loop there can only be one). - for my $setProctor ($db0->getUsersWhere({ user_id => 'set_id:' . $set->set_id })) { - eval { $db->addUser($setProctor) }; - warn $@ if $@; - - my $password = $db0->getPassword($setProctor->user_id); - eval { $db->addPassword($password) } if $password; - warn $@ if $@; - - my $permission = $db0->getPermissionLevel($setProctor->user_id); - eval { $db->addPermissionLevel($permission) } if $permission; - warn $@ if $@; - } } if ($options{copyNonStudents}) { foreach my $userTriple (@users) { diff --git a/lib/WeBWorK/Utils/Instructor.pm b/lib/WeBWorK/Utils/Instructor.pm index de6c3f03b7..e425480a74 100644 --- a/lib/WeBWorK/Utils/Instructor.pm +++ b/lib/WeBWorK/Utils/Instructor.pm @@ -259,7 +259,7 @@ sub assignSetToAllUsers { my ($db, $ce, $setID) = @_; debug("$setID: getting user list"); - my @userRecords = $db->getUsersWhere({ user_id => { not_like => 'set_id:%' } }); + my @userRecords = $db->getUsersWhere; debug("$setID: (done with that)"); return assignSetToGivenUsers($db, $ce, $setID, 0, @userRecords); diff --git a/templates/ContentGenerator/Instructor/ProblemSetDetail/restricted_login_proctor_password_row.html.ep b/templates/ContentGenerator/Instructor/ProblemSetDetail/restricted_login_proctor_password_row.html.ep deleted file mode 100644 index 45611dd0f8..0000000000 --- a/templates/ContentGenerator/Instructor/ProblemSetDetail/restricted_login_proctor_password_row.html.ep +++ /dev/null @@ -1,36 +0,0 @@ -% # This assumes that the login proctor password is something that can only be changed for the global set. -% -% # If the set doesn't require a login proctor, then we can assume that one doesn't exist. -% # Otherwise, we need to check the database to find if there's an already defined password. -% # In that case set the form parameter so it is picked up by the tag helper below. -% if ($globalRecord->restricted_login_proctor eq 'Yes' && $db->existsPassword("set_id:$setID")) { - % param("set.$setID.restricted_login_proctor_password", '********'); -% } -% - - - <%= label_for "set.$setID.restricted_login_proctor_password" => maketext('Password') =%> - - - - - - - - - <%= text_field - "set.$setID.restricted_login_proctor_password" => '', - id => "set.$setID.restricted_login_proctor_password", - size => 10, - class => 'form-control form-control-sm' =%> - - diff --git a/templates/ContentGenerator/LoginProctor.html.ep b/templates/ContentGenerator/LoginProctor.html.ep index fe4af28282..b69e688263 100644 --- a/templates/ContentGenerator/LoginProctor.html.ep +++ b/templates/ContentGenerator/LoginProctor.html.ep @@ -42,15 +42,13 @@ %= form_for current_route, method => 'POST', begin % # Add the form data posted to the requested URI in hidden fields. % my @fields_to_print = - % grep { !/^(user|effectiveUser|passwd|key|proctor_user|proctor_passwd)$/ } param; + % grep { !/^(user|effectiveUser|passwd|key|proctor_user|proctor_passwd|set_level_proctor)$/ } param; % if (@fields_to_print) { <%= $c->hidden_fields(@fields_to_print) %> % } <%= $c->hidden_authen_fields =%> % - % if (param('submitAnswers') - % || ($userSet->restricted_login_proctor eq '' || $userSet->restricted_login_proctor eq 'No')) - % { + % if (param('submitAnswers') || $userSet->restricted_login_password eq '') { % # The user info and username field for the proctor. - <%= hidden_field proctor_user => "set_id:$setID" =%> + <%= hidden_field set_level_proctor => 1 =%> % } % % # The password field for the proctor.