Merge pull request #2718 from drgrice1/manage-otp-remove-large-course-restrictions

Alex-Jordan · web-flow · commit b937168f5621 · 2025-05-20T12:54:15.000-07:00
Remove the filtering of large courses on the course admin "Manage OTP Secrets" page and fix security vulnerability.
diff --git a/lib/WeBWorK/ContentGenerator/CourseAdmin.pm b/lib/WeBWorK/ContentGenerator/CourseAdmin.pm
@@ -2403,30 +2403,25 @@ 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.
+	# 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('');
@@ -2443,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.
@@ -2461,11 +2461,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.
@@ -2550,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(
@@ -2599,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,
diff --git a/templates/ContentGenerator/CourseAdmin/copy_otp_secrets_confirm.html.ep b/templates/ContentGenerator/CourseAdmin/copy_otp_secrets_confirm.html.ep
@@ -40,7 +40,7 @@
 			</tbody>
 		</table>
 	</div>
-	<%= $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
@@ -8,17 +8,6 @@
 %
 <h2><%= maketext('Manage OTP Secrets') %> <%= $c->helpMacro('AdminManageOTPSecrets') %></h2>
 <%= form_for current_route, method => 'POST', begin =%>
-	% if (@$skipped_courses) {
-		<div class="mb-3">
-			<div class="alert alert-warning mb-2">
-				<%= maketext('The following large courses are not shown: [_1]', join(', ', @$skipped_courses)); =%>
-			</div>
-			<p>
-				<%= submit_button maketext('Show All Courses'),
-					name => 'show_all_courses', class => 'btn btn-primary' =%>
-			</p>
-		</div>
-	% }
 	<div>
 		<ul class="nav nav-tabs mb-2" role="tablist">
 			% for my $tab ('single', 'multiple', 'reset') {
@@ -147,7 +136,7 @@
 									disabled                     => undef,
 									selected                     => undef,
 								],
-								sort (@course_keys, @$skipped_courses)
+								sort @course_keys
 							],
 							id       => 'destMultipleCourseID',
 							class    => 'form-select',
@@ -195,7 +184,7 @@
 			</div>
 		</div>
 	</div>
-	<%= $c->hidden_fields('subDisplay', 'show_all_courses') =%>
+	<%= $c->hidden_fields('subDisplay') =%>
 	<%= hidden_field action => $active_tab, id => 'current_action' =%>
 	<div class="mb-3">
 		<%= 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
@@ -28,7 +28,7 @@
 			</tbody>
 		</table>
 	</div>
-	<%= $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
@@ -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.') =%>
 </p>
-<p class="mb-0">
-	<%= 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.') =%>
-</p>
