diff --git a/htdocs/js/AddUsers/add-users.js b/htdocs/js/AddUsers/add-users.js index 5976698420..3672c12176 100644 --- a/htdocs/js/AddUsers/add-users.js +++ b/htdocs/js/AddUsers/add-users.js @@ -13,4 +13,25 @@ passwordSelect.addEventListener('change', setPlaceholders); setPlaceholders(); + + // Each "force password reset" checkbox starts out unchecked. Automatically check it once a password is typed + // into that row's password field, or once a fallback password source other than "None" is selected (since that + // fallback value will be used as the password for any row left blank). If the instructor manually toggles a + // checkbox, stop overriding that row's checkbox for the rest of the page's lifetime. + const manuallySet = new Set(); + for (const autoCheckbox of document.querySelectorAll('input[data-auto-check="password"]')) { + const passwordField = autoCheckbox.closest('tr')?.querySelector('.new_password'); + if (!passwordField) continue; + + autoCheckbox.addEventListener('change', () => manuallySet.add(autoCheckbox)); + + const updateCheckbox = () => { + if (!manuallySet.has(autoCheckbox)) + autoCheckbox.checked = passwordField.value !== '' || passwordSelect.value !== ''; + }; + + passwordField.addEventListener('input', updateCheckbox); + passwordSelect.addEventListener('change', updateCheckbox); + updateCheckbox(); + } })(); diff --git a/htdocs/js/UserList/userlist.js b/htdocs/js/UserList/userlist.js index 483dbc1e19..63e8cf34a7 100644 --- a/htdocs/js/UserList/userlist.js +++ b/htdocs/js/UserList/userlist.js @@ -145,7 +145,26 @@ }); } - for (const deleteCheck of document.querySelectorAll('input[name$="password_delete"]')) { - new bootstrap.Tooltip(deleteCheck, { placement: 'top', fallbackPlacements: [] }); + for (const tooltipCheck of document.querySelectorAll( + 'input[name$="password_delete"]:not([disabled]), input[name$="password_must_reset"]' + )) { + new bootstrap.Tooltip(tooltipCheck, { placement: 'top', fallbackPlacements: [] }); + } + + // For a user with no password, the "force password reset" checkbox starts out unchecked (there is nothing to + // reset yet). Automatically check it once a new password is actually typed into the corresponding password + // field, and uncheck it again if that field is cleared back out. If the instructor manually toggles the + // checkbox themselves, stop overriding their choice for the rest of the page's lifetime. + for (const autoCheckbox of document.querySelectorAll('input[data-auto-check="password"]')) { + const passwordField = autoCheckbox.closest('.row')?.querySelector('input[name$=".password"]'); + if (!passwordField) continue; + + let manuallySet = false; + autoCheckbox.addEventListener('change', () => { + manuallySet = true; + }); + passwordField.addEventListener('input', () => { + if (!manuallySet) autoCheckbox.checked = passwordField.value !== ''; + }); } })(); diff --git a/lib/WeBWorK.pm b/lib/WeBWorK.pm index ae5fc3dd86..d5ab3e6428 100644 --- a/lib/WeBWorK.pm +++ b/lib/WeBWorK.pm @@ -33,6 +33,7 @@ use WeBWorK::Debug; use WeBWorK::Upload; use WeBWorK::Utils qw(runtime_use); use WeBWorK::ContentGenerator::Login; +use WeBWorK::ContentGenerator::ForcePasswordChange; use WeBWorK::ContentGenerator::TwoFactorAuthentication; use WeBWorK::ContentGenerator::LoginProctor; @@ -218,7 +219,11 @@ async sub dispatch ($c) { # If the user is logging out and authentication failed, still logout. return 1 if $displayModule eq 'WeBWorK::ContentGenerator::Logout'; - if ($c->authen->session->{two_factor_verification_needed}) { + if ($c->authen->session->{password_reset_needed}) { + debug("Login succeeded but the user's password must be reset.\n"); + debug("Rendering WeBWorK::ContentGenerator::ForcePasswordChange\n"); + await WeBWorK::ContentGenerator::ForcePasswordChange->new($c)->go(); + } elsif ($c->authen->session->{two_factor_verification_needed}) { debug("Login succeeded but two factor authentication is needed.\n"); debug("Rendering WeBWorK::ContentGenerator::TwoFactorAuthentication\n"); await WeBWorK::ContentGenerator::TwoFactorAuthentication->new($c)->go(); diff --git a/lib/WeBWorK/Authen.pm b/lib/WeBWorK/Authen.pm index ccc02f2cad..f2576b5e2c 100644 --- a/lib/WeBWorK/Authen.pm +++ b/lib/WeBWorK/Authen.pm @@ -40,7 +40,7 @@ use Scalar::Util qw(weaken); use Mojo::Util qw(b64_encode b64_decode); use WeBWorK::Debug; -use WeBWorK::Utils qw(x runtime_use utf8Crypt); +use WeBWorK::Utils qw(x runtime_use utf8Crypt cryptPassword); use WeBWorK::Utils::Logs qw(writeCourseLog); use WeBWorK::Utils::TOTP; use WeBWorK::Localize; @@ -161,7 +161,23 @@ sub verify { $remember_2fa = 0; } - if ($self->{was_verified} + # A forced password reset takes priority over two factor authentication setup. This is checked directly against + # the database on every request (rather than relying solely on a session flag like two factor authentication + # does below) since unlike two factor authentication, once resolved it is resolved permanently and does not need + # a per-session bypass mechanism. + if ( + $self->{was_verified} + && $self->{login_type} eq 'normal' + && !$self->{external_auth} + && (!$c->{rpc} || ($c->{rpc} && !$c->stash->{disable_cookies})) + && do { my $p = $c->db->getPassword($self->{user_id}); $p && $p->must_reset_password } + ) + { + $self->{was_verified} = 0; + $self->session(password_reset_needed => 1); + $self->maybe_send_cookie; + $self->set_params; + } elsif ($self->{was_verified} && $self->{login_type} eq 'normal' && !$self->{external_auth} && (!$c->{rpc} || ($c->{rpc} && !$c->stash->{disable_cookies})) @@ -460,6 +476,45 @@ sub verify_normal_user { debug("sessionExists='", $sessionExists, "' keyMatches='", $keyMatches, "' timestampValid='", $timestampValid, "'"); if ($sessionExists && $keyMatches && $timestampValid) { + if ($self->session->{password_reset_needed}) { + # All of the below falls through to below and returns 1. That only lets the user into the course once + # password_reset_needed is deleted from the session (which only happens once the password has actually + # been changed). + my $newPassword = trim($c->param('newPassword')); + my $confirmPassword = trim($c->param('confirmPassword')); + if (defined $newPassword && $newPassword ne '') { + if ($newPassword eq ($confirmPassword // '')) { + my $password = $c->db->getPassword($user_id); + if ($password->password && utf8Crypt($newPassword, $password->password) eq $password->password) { + $c->stash( + authen_error => $c->maketext( + 'Your new password must be different from your current password. ' + . 'Please choose a different password.' + ) + ); + } else { + $password->password(cryptPassword($newPassword)); + $password->must_reset_password(0); + eval { $c->db->putPassword($password) }; + if ($@) { + $c->stash(authen_error => + $c->maketext('Your password was not changed due to an internal error.')); + } else { + delete $self->session->{password_reset_needed}; + } + } + } else { + $c->stash( + authen_error => $c->maketext( + 'The passwords you entered in the "New Password" and "Confirm New Password" fields do not ' + . 'match. Please retype your new password and try again.' + ) + ); + } + } else { + $c->stash(authen_error => $c->maketext('You must enter a new password.')); + } + } if ($self->session->{two_factor_verification_needed}) { if ($c->param('cancel_otp_verification') || !$c->param('verify_otp')) { delete $self->session->{two_factor_verification_needed}; @@ -513,9 +568,10 @@ sub verify_normal_user { } else { my $auth_result = $self->authenticate; - # Don't try to obtain two factor verification in this case! Two factor authentication can only be done with an - # existing session. This can still be set if a session times out, for example. + # Don't try to obtain two factor verification or a forced password reset in this case! Those can only be done + # with an existing session. This can still be set if a session times out, for example. delete $self->session->{two_factor_verification_needed}; + delete $self->session->{password_reset_needed}; if ($auth_result > 0) { return 0 unless $self->validate_user; diff --git a/lib/WeBWorK/ContentGenerator/CourseAdmin.pm b/lib/WeBWorK/ContentGenerator/CourseAdmin.pm index d407a0a27a..bf632dcbf5 100644 --- a/lib/WeBWorK/ContentGenerator/CourseAdmin.pm +++ b/lib/WeBWorK/ContentGenerator/CourseAdmin.pm @@ -347,8 +347,9 @@ sub do_add_course ($c) { status => $permissionLevel == $ce->{userRoles}{student} ? 'C' : 'O', ); my $Password = $db->newPassword( - user_id => $userID, - password => $password ? cryptPassword($password) : '', + user_id => $userID, + password => $password ? cryptPassword($password) : '', + must_reset_password => $password ? 1 : 0, ); my $PermissionLevel = $db->newPermissionLevel( user_id => $userID, diff --git a/lib/WeBWorK/ContentGenerator/ForcePasswordChange.pm b/lib/WeBWorK/ContentGenerator/ForcePasswordChange.pm new file mode 100644 index 0000000000..ffa5bd7d40 --- /dev/null +++ b/lib/WeBWorK/ContentGenerator/ForcePasswordChange.pm @@ -0,0 +1,23 @@ +package WeBWorK::ContentGenerator::ForcePasswordChange; +use Mojo::Base 'WeBWorK::ContentGenerator::Login', -signatures; + +=head1 NAME + +WeBWorK::ContentGenerator::ForcePasswordChange - display a form requiring the user to change their password before +they can access the course. + +=cut + +sub pre_header_initialize ($c) { + # Preserve the form data posted to the requested URI. + my @fields_to_print = + grep { !m/^(user|passwd|key|force_passwd_authen|newPassword|confirmPassword)$/ } $c->param; + push(@fields_to_print, 'user', 'key') if $c->ce->{session_management_via} ne 'session_cookie'; + $c->stash->{hidden_fields} = @fields_to_print ? $c->hidden_fields(@fields_to_print) : ''; + + $c->stash->{authen_error} //= ''; + + return; +} + +1; diff --git a/lib/WeBWorK/ContentGenerator/Instructor/AddUsers.pm b/lib/WeBWorK/ContentGenerator/Instructor/AddUsers.pm index 471c3e68d1..d4543315aa 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/AddUsers.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/AddUsers.pm @@ -78,6 +78,7 @@ sub initialize ($c) { my $newPassword = $db->newPassword; $newPassword->user_id($new_user_id); $newPassword->password(cryptPassword($password)); + $newPassword->must_reset_password($c->param("must_reset_$i") ? 1 : 0); $db->addPassword($newPassword); } diff --git a/lib/WeBWorK/ContentGenerator/Instructor/UserList.pm b/lib/WeBWorK/ContentGenerator/Instructor/UserList.pm index 8228303c18..829c38c84d 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/UserList.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/UserList.pm @@ -167,10 +167,10 @@ async sub pre_header_initialize ($c) { my %permissionLevels = map { $_->user_id => $_->permission } $db->getPermissionLevelsWhere({ user_id => { not_like => 'set_id:%' } }); - my %passwordExists = - map { $_->user_id => defined $_->password } $db->getPasswordsWhere({ user_id => { not_like => 'set_id:%' } }); + my %passwordRecords = + map { $_->user_id => $_ } $db->getPasswordsWhere({ user_id => { not_like => 'set_id:%' } }); - # Add permission level and a password exists field to the user record hash. + # Add permission level, a password exists field, and a must reset password field to the user record hash. for my $user (@allUsersDB) { unless (defined $permissionLevels{ $user->user_id }) { # Uh oh! No permission level record found! @@ -187,8 +187,11 @@ async sub pre_header_initialize ($c) { $permissionLevels{ $user->user_id } = 0; } - $user->{permission} = $permissionLevels{ $user->user_id }; - $user->{passwordExists} = $passwordExists{ $user->user_id }; + $user->{permission} = $permissionLevels{ $user->user_id }; + $user->{passwordExists} = + $passwordRecords{ $user->user_id } && $passwordRecords{ $user->user_id }->password ? 1 : 0; + $user->{mustResetPassword} = + $passwordRecords{ $user->user_id } ? $passwordRecords{ $user->user_id }->must_reset_password : 0; } my %allUsers = map { $_->user_id => $_ } @allUsersDB; @@ -837,21 +840,31 @@ sub save_edit_handler ($c) { # Thus if the password is set again later, the user will need to setup two factor authentication again. $db->deletePassword($User->user_id) if $db->existsPassword($User->user_id); } else { - my $newPassword = $c->param("user.${userID}.password"); + my $newPassword = $c->param("user.${userID}.password"); + my $mustResetPassword = $c->param("user.${userID}.password_must_reset") ? 1 : 0; if ($newPassword && $newPassword =~ /\S/) { - my $Password = eval { $db->getPassword($User->user_id) }; - my $cryptPassword = cryptPassword($newPassword); + my $Password = eval { $db->getPassword($User->user_id) }; if ($Password) { # Note that in this case the otp_secret will be preserved. So the user will still be able to use the # configured two factor authentication with the new password. $Password->password(cryptPassword($newPassword)); + $Password->must_reset_password($mustResetPassword); eval { $db->putPassword($Password) }; } else { $Password = $db->newPassword(); $Password->user_id($userID); $Password->password(cryptPassword($newPassword)); + $Password->must_reset_password($mustResetPassword); eval { $db->addPassword($Password) }; } + } else { + # No new password was typed, but the "prompt to reset password" checkbox may have changed for an + # existing password. + my $Password = eval { $db->getPassword($User->user_id) }; + if ($Password && ($Password->must_reset_password ? 1 : 0) != $mustResetPassword) { + $Password->must_reset_password($mustResetPassword); + eval { $db->putPassword($Password) }; + } } } @@ -970,7 +983,10 @@ sub importUsersFromCSV ($c, $fileName, $replaceExisting, $fallbackPasswordSource $record{status} = $default_status_abbrev unless defined $record{status} && $record{status} ne ''; - # Determine what to use for the password (if anything). + # Determine what to use for the password (if anything). If the crypted password field was not itself set + # directly in the classlist file, then the user must be prompted to reset their password once one is + # determined below (whether from an unencrypted password column or a fallback source column). + my $mustResetPassword = $record{password} ? 0 : 1; if (!$record{password}) { if (defined $record{unencrypted_password} && $record{unencrypted_password} =~ /\S/) { $record{password} = cryptPassword($record{unencrypted_password}); @@ -988,7 +1004,13 @@ sub importUsersFromCSV ($c, $fileName, $replaceExisting, $fallbackPasswordSource my $User = $db->newUser(%record); my $PermissionLevel = $db->newPermissionLevel(user_id => $user_id, permission => $record{permission}); - my $Password = $record{password} ? $db->newPassword(user_id => $user_id, password => $record{password}) : undef; + my $Password = $record{password} + ? $db->newPassword( + user_id => $user_id, + password => $record{password}, + must_reset_password => $mustResetPassword + ) + : undef; # DBFIXME use REPLACE if (exists $allUserIDs{$user_id}) { diff --git a/lib/WeBWorK/ContentGenerator/Logout.pm b/lib/WeBWorK/ContentGenerator/Logout.pm index e2b28d0e4f..ae4f267570 100644 --- a/lib/WeBWorK/ContentGenerator/Logout.pm +++ b/lib/WeBWorK/ContentGenerator/Logout.pm @@ -38,7 +38,13 @@ sub pre_header_initialize ($c) { } } - $c->reply_with_redirect($authen->{redirect}) if $authen->{redirect}; + if ($authen->{redirect}) { + $c->reply_with_redirect($authen->{redirect}); + } elsif ($c->param('show_login')) { + # Allow a caller (e.g. the forced password reset page) to skip the logout confirmation page and go directly + # to the course's login page, so the user can immediately log in as someone else. + $c->reply_with_redirect($c->systemLink($c->url_for('set_list'))); + } return; } diff --git a/lib/WeBWorK/DB/Record/Password.pm b/lib/WeBWorK/DB/Record/Password.pm index f125e0b23d..e4fbb72667 100644 --- a/lib/WeBWorK/DB/Record/Password.pm +++ b/lib/WeBWorK/DB/Record/Password.pm @@ -17,6 +17,7 @@ BEGIN { otp_secret => { type => "TEXT" }, reset_token => { type => "TEXT" }, reset_token_expiration => { type => "BIGINT" }, + must_reset_password => { type => "INT" }, ); } diff --git a/templates/ContentGenerator/ForcePasswordChange.html.ep b/templates/ContentGenerator/ForcePasswordChange.html.ep new file mode 100644 index 0000000000..260570e835 --- /dev/null +++ b/templates/ContentGenerator/ForcePasswordChange.html.ep @@ -0,0 +1,29 @@ +% # WeBWorK::Authen::verify will set the note "authen_error" if invalid input is submitted here. +

+ <%= maketext('Your password must be changed before you can continue. ' + . 'Please enter a new password below.') =%> +

+% +<%= form_for current_route, method => 'POST', begin =%> + <%= $hidden_fields =%> + % +
+
+ <%= password_field 'newPassword', id => 'new_password', 'aria-required' => 'true', + class => 'form-control', placeholder => '' =%> + <%= label_for new_password => maketext('New Password') =%> +
+
+ <%= password_field 'confirmPassword', id => 'confirm_password', 'aria-required' => 'true', + class => 'form-control', placeholder => '' =%> + <%= label_for confirm_password => maketext('Confirm New Password') =%> +
+ % if ($authen_error) { +
<%= $authen_error =%>
+ % } +
+ <%= submit_button(maketext('Change Password'), class => 'btn btn-primary') =%> + <%= link_to maketext('Log in as a different user') => url_for('logout')->query(show_login => 1) =%> +
+
+<% end =%> diff --git a/templates/ContentGenerator/Instructor/AddUsers.html.ep b/templates/ContentGenerator/Instructor/AddUsers.html.ep index 03d0dc1cea..1f4d5935de 100644 --- a/templates/ContentGenerator/Instructor/AddUsers.html.ep +++ b/templates/ContentGenerator/Instructor/AddUsers.html.ep @@ -91,6 +91,7 @@ <%= maketext('Comment') %> <%= maketext('Permission Level') %> <%= maketext('Password') %> + <%= maketext('Force Password Reset') %> @@ -154,6 +155,14 @@ <%= text_field "password_$_" => '', size => '16', 'aria-labelledby' => 'password_header', class => 'form-control form-control-sm w-auto new_password' =%> + + % # Unchecked by default. js/AddUsers/add-users.js automatically checks this once a + % # password is typed into the password field above, or once a fallback password source + % # other than "None" is selected (unless the instructor manually toggles it themselves). + <%= check_box "must_reset_$_" => 1, class => 'form-check-input', + 'aria-label' => maketext('Force this user to reset their password at first login'), + data => { auto_check => 'password' } =%> + % } diff --git a/templates/ContentGenerator/Instructor/UserList/user_list_field.html.ep b/templates/ContentGenerator/Instructor/UserList/user_list_field.html.ep index e7e3831480..3b561d2479 100644 --- a/templates/ContentGenerator/Instructor/UserList/user_list_field.html.ep +++ b/templates/ContentGenerator/Instructor/UserList/user_list_field.html.ep @@ -76,12 +76,31 @@ class => 'form-control form-control-sm d-inline w-auto', size => 14, 'aria-labelledby' => 'password_header' =%> - % if ($user->{passwordExists}) { -
- <%= check_box "${fieldName}_delete" => 1, class => 'form-check-input', - data => { bs_toggle => 'tooltip', bs_title => 'Check to delete password' } =%> -
- % } + % # This checkbox is always rendered (even when there is no password to delete) so that the columns of + % # checkboxes line up from row to row. When there is no password, it is just made invisible and + % # disabled so it has no effect and cannot be interacted with. +
+ <%= check_box "${fieldName}_delete" => 1, + class => 'form-check-input' . ($user->{passwordExists} ? '' : ' invisible'), + $user->{passwordExists} + ? (data => { bs_toggle => 'tooltip', bs_title => 'Check to delete password' }) + : (disabled => undef, tabindex => -1, 'aria-hidden' => 'true') =%> +
+
+ % # For a user with no password, this checkbox is not checked by default (there is nothing to reset + % # yet). Instead, js/UserList/userlist.js automatically checks it once a new password is actually + % # typed into the password field above (unless the instructor has manually toggled it themselves). + <%= check_box "${fieldName}_must_reset" => 1, class => 'form-check-input', + 'aria-label' => maketext('Prompt this user to reset their password at next login'), + data => { + bs_toggle => 'tooltip', + bs_title => maketext( + 'Check to require this user to change their password before they can access the ' + . 'course again. Only takes effect if the user has or is given a password.'), + ($user->{passwordExists} ? () : (auto_check => 'password')) + }, + $user->{mustResetPassword} ? (checked => undef) : () =%> +
% }