From b7957827ac0dcd987105ced97209c2dde19757ca Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Thu, 24 Apr 2025 08:49:29 -0500 Subject: [PATCH 1/2] Remove the filtering of large courses on the course admin "Manage OTP Secrets" page. This page loads quickly regardless of if these courses are filtered out. Furthermore the menus are generally responsive even when these large courses are included. The only case where something is a bit slow is in the case that on the "Copy Single Secret" tab the selected "Source Course ID" is one of these large courses. In that case if you click on the "Source User ID" it takes some time for the dropdown menu to appear and even then it has to be an astronomically large course for that to be slow (more than 20,000 users). However, even if the large courses are included as long as a large course is not selected the menus are quick. It is interesting that the multiple select elements on the "Copy Multiple Secrets" tab are still fast even with astronomically large courses. It is just the single select elements on the "Copy Single Secret" tab that experience a noticeable slowdown. In any case, the point is that there is no reason to filter these courses. Doing so just adds unnecessary steps for the user. --- lib/WeBWorK/ContentGenerator/CourseAdmin.pm | 23 ++++--------------- .../copy_otp_secrets_confirm.html.ep | 2 +- .../manage_otp_secrets_form.html.ep | 15 ++---------- .../reset_otp_secrets_confirm.html.ep | 2 +- .../HelpFiles/AdminManageOTPSecrets.html.ep | 5 ---- 5 files changed, 9 insertions(+), 38 deletions(-) diff --git a/lib/WeBWorK/ContentGenerator/CourseAdmin.pm b/lib/WeBWorK/ContentGenerator/CourseAdmin.pm index c8125cb2d0..3dfd372a6b 100644 --- a/lib/WeBWorK/ContentGenerator/CourseAdmin.pm +++ b/lib/WeBWorK/ContentGenerator/CourseAdmin.pm @@ -2403,23 +2403,14 @@ sub do_save_lti_course_map ($c) { # Form to copy or reset OTP secrets. sub manage_otp_secrets_form ($c) { - my $courses = {}; - my $dbs = {}; - my $skipped_courses = []; - my $show_all_courses = $c->param('show_all_courses') || 0; + my $courses = {}; + my $dbs = {}; # Create course data first, since it is used in all cases and initializes course db references. for my $courseID (listCourses($c->ce)) { my $ce = WeBWorK::CourseEnvironment->new({ courseName => $courseID }); - $dbs->{$courseID} = WeBWorK::DB->new($ce); - - # By default ignore courses larger than 200 users, as this can cause a large load building menus. - my @users = $dbs->{$courseID}->listUsers; - if ($show_all_courses || scalar @users < 200) { - $courses->{$courseID} = \@users; - } else { - push(@$skipped_courses, $courseID); - } + $dbs->{$courseID} = WeBWorK::DB->new($ce); + $courses->{$courseID} = [ $dbs->{$courseID}->listUsers ]; } # Process the confirmed rest or copy actions here. @@ -2461,11 +2452,7 @@ sub manage_otp_secrets_form ($c) { } } - return $c->include( - 'ContentGenerator/CourseAdmin/manage_otp_secrets_form', - courses => $courses, - skipped_courses => $skipped_courses - ); + return $c->include('ContentGenerator/CourseAdmin/manage_otp_secrets_form', courses => $courses); } # Deals with both single and multiple copy confirmation. diff --git a/templates/ContentGenerator/CourseAdmin/copy_otp_secrets_confirm.html.ep b/templates/ContentGenerator/CourseAdmin/copy_otp_secrets_confirm.html.ep index 81c8a3d3d9..4b72b3ee3b 100644 --- a/templates/ContentGenerator/CourseAdmin/copy_otp_secrets_confirm.html.ep +++ b/templates/ContentGenerator/CourseAdmin/copy_otp_secrets_confirm.html.ep @@ -40,7 +40,7 @@ - <%= $c->hidden_fields('subDisplay', 'show_all_courses') =%> + <%= $c->hidden_fields('subDisplay') =%> % if ($total > 0) { % my $skipped = @$action_rows - $total; <%= content 'hidden-rows' %> diff --git a/templates/ContentGenerator/CourseAdmin/manage_otp_secrets_form.html.ep b/templates/ContentGenerator/CourseAdmin/manage_otp_secrets_form.html.ep index 6d10a86335..e84af83c11 100644 --- a/templates/ContentGenerator/CourseAdmin/manage_otp_secrets_form.html.ep +++ b/templates/ContentGenerator/CourseAdmin/manage_otp_secrets_form.html.ep @@ -8,17 +8,6 @@ %

<%= maketext('Manage OTP Secrets') %> <%= $c->helpMacro('AdminManageOTPSecrets') %>

<%= form_for current_route, method => 'POST', begin =%> - % if (@$skipped_courses) { -
-
- <%= maketext('The following large courses are not shown: [_1]', join(', ', @$skipped_courses)); =%> -
-

- <%= submit_button maketext('Show All Courses'), - name => 'show_all_courses', class => 'btn btn-primary' =%> -

-
- % }
- <%= $c->hidden_fields('subDisplay', 'show_all_courses') =%> + <%= $c->hidden_fields('subDisplay') =%> <%= hidden_field action => $active_tab, id => 'current_action' =%>
<%= submit_button $active_tab eq 'single' diff --git a/templates/ContentGenerator/CourseAdmin/reset_otp_secrets_confirm.html.ep b/templates/ContentGenerator/CourseAdmin/reset_otp_secrets_confirm.html.ep index d804518ee5..7e936e6216 100644 --- a/templates/ContentGenerator/CourseAdmin/reset_otp_secrets_confirm.html.ep +++ b/templates/ContentGenerator/CourseAdmin/reset_otp_secrets_confirm.html.ep @@ -28,7 +28,7 @@
- <%= $c->hidden_fields('subDisplay', 'show_all_courses', 'sourceResetCourseID') =%> + <%= $c->hidden_fields('subDisplay', 'sourceResetCourseID') =%> % if ($total > 0) { % my $skipped = @$action_rows - $total; <%= content 'hidden-rows' %> diff --git a/templates/HelpFiles/AdminManageOTPSecrets.html.ep b/templates/HelpFiles/AdminManageOTPSecrets.html.ep index 7db4f8c18c..16c54b7ef0 100644 --- a/templates/HelpFiles/AdminManageOTPSecrets.html.ep +++ b/templates/HelpFiles/AdminManageOTPSecrets.html.ep @@ -19,8 +19,3 @@ <%= maketext('If resetting secrets, choose a source course ID, then choose one (or more) users whose secrets ' . 'will be reset. Note, the user(s) will have to setup two factor authentication afterwards.') =%>

-

- <%= maketext('By default, courses larger than 200 users will not be shown, as this can cause extra CPU usage ' - . 'when generating the user menus. If any large course is skipped, a message will inform you of which ' - . 'courses were skipped, and a button to show all courses can be used to include those courses.') =%> -

From d81c523c29f8b3bd1e552302b424862066cce4e0 Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Thu, 24 Apr 2025 10:49:32 -0500 Subject: [PATCH 2/2] Protect against a user in the admin course changing or resetting their own OTP secret. --- lib/WeBWorK/ContentGenerator/CourseAdmin.pm | 48 +++++++++++++++------ 1 file changed, 35 insertions(+), 13 deletions(-) diff --git a/lib/WeBWorK/ContentGenerator/CourseAdmin.pm b/lib/WeBWorK/ContentGenerator/CourseAdmin.pm index 3dfd372a6b..a11e7b560c 100644 --- a/lib/WeBWorK/ContentGenerator/CourseAdmin.pm +++ b/lib/WeBWorK/ContentGenerator/CourseAdmin.pm @@ -2413,11 +2413,15 @@ sub manage_otp_secrets_form ($c) { $courses->{$courseID} = [ $dbs->{$courseID}->listUsers ]; } - # Process the confirmed rest or copy actions here. + # Process the confirmed reset or copy actions here. if ($c->param('otp_confirm_reset')) { my $total = 0; my $courseID = $c->param('sourceResetCourseID'); for my $user ($c->param('otp_reset_row')) { + if ($courseID eq $c->ce->{courseName} && $user eq $c->param('user')) { + $c->addbadmessage($c->maketext('You may not reset your own OTP secret!')); + next; + } my $password = $dbs->{$courseID}->getPassword($user); if ($password && $password->otp_secret) { $password->otp_secret(''); @@ -2434,6 +2438,11 @@ sub manage_otp_secrets_form ($c) { my $total = 0; for my $row ($c->param('otp_copy_row')) { my ($s_course, $s_user, $d_course, $d_user) = split(':', $row); + if ($d_course eq $c->ce->{courseName} && $d_user eq $c->param('user')) { + $c->addbadmessage( + $c->maketext('You cannot overwrite your OTP secret with one from another course or user!')); + next; + } my $s_password = $dbs->{$s_course}->getPassword($s_user); if ($s_password && $s_password->otp_secret) { # Password may not be defined if using external auth, so create new password record if not. @@ -2537,17 +2546,24 @@ sub copy_otp_secrets_confirm ($c) { $dbs{$d_course} = WeBWorK::DB->new($dest_ce); } - my $d_user_password = $dbs{$d_course}->getPassword($d_user); - if (!defined $d_user_password) { - # Just because there is no password record, the user could still exist when using external auth. - unless ($dbs{$d_course}->existsUser($d_user)) { - $dest_error = 'warning'; - $error_message = $c->maketext('User does not exist - Skipping'); - $skip = 1; + if ($d_course eq $c->ce->{courseName} && $d_user eq $c->param('user')) { + $dest_error = 'danger'; + $error_message = + $c->maketext('You cannot overwrite your OTP secret with one from another course or user!'); + $skip = 1; + } else { + my $d_user_password = $dbs{$d_course}->getPassword($d_user); + if (!defined $d_user_password) { + # Just because there is no password record, the user could still exist when using external auth. + unless ($dbs{$d_course}->existsUser($d_user)) { + $dest_error = 'warning'; + $error_message = $c->maketext('User does not exist - Skipping'); + $skip = 1; + } + } elsif ($d_user_password->otp_secret) { + $dest_error = 'danger'; + $error_message = $c->maketext('OTP Secret is not empty - Overwritting'); } - } elsif ($d_user_password->otp_secret) { - $dest_error = 'danger'; - $error_message = $c->maketext('OTP Secret is not empty - Overwritting'); } push( @@ -2586,8 +2602,14 @@ sub reset_otp_secrets_confirm ($c) { my $db = WeBWorK::DB->new($ce); my @rows; for my $user (@dest_users) { - my $password = $db->getPassword($user); - my $error = $password && $password->otp_secret ? '' : $c->maketext('OTP Secret is empty - Skipping'); + my $error = ''; + + if ($source_course eq $c->ce->{courseName} && $user eq $c->param('user')) { + $error = $c->maketext('You may not reset your own OTP secret!'); + } else { + my $password = $db->getPassword($user); + $error = $c->maketext('OTP Secret is empty - Skipping') unless $password && $password->otp_secret; + } push( @rows,