Add a session flash and use it to pass status messages for redirects.

The session `flash` method is similar to the `session` method added previously, and is a method of the WeBWorK::Authen object attached to the controller. It uses the `flash` method of `Mojolicious::Plugin::DefaultHelpers` if `session_management_via` is "session_cookie" and imitates that with the database session otherwise. This method saves data to the session that will persist only for the next request. This is then used to save `status_messages` when redirects occur. This fixes issue openwebwork#2336, since the `status_message` URL parameter is no longer used. We need to make sure that we never again use a URL parameter to pass HTML.
drgrice1 · Feb 23, 2024 · 50a053f · 50a053f
1 parent 27410ec
commit 50a053f
Show file tree

Hide file tree

Showing 8 changed files with 87 additions and 66 deletions.
diff --git a/lib/WeBWorK/Authen.pm b/lib/WeBWorK/Authen.pm
@@ -773,6 +773,39 @@ sub session {
 	return $c->session(@params);
 }
 
+=head2 flash
+
+This sets data in the session that only persists for the next request.
+
+=cut
+
+sub flash {
+	my ($self, @params) = @_;
+	my $c = $self->{c};
+
+	# If session_management_via is not "session_cookie" (so should be "key"), then use the database session.
+	if ($c->ce->{session_management_via} ne 'session_cookie' || $c->stash('disable_cookies')) {
+		# Note that the return values are the same as those returned by the
+		# Mojolicious::Plugin::DefaultHelpers flash method in the following cases.
+
+		# Note that this will be the database session in this case since
+		# the conditions are the same as in the session method above.
+		my $session = $self->session;
+
+		# Get old values.
+		return $session->{flash} ? $session->{flash}{ $params[0] } : undef if @params == 1 && !ref $params[0];
+
+		# Initialize new flash and merge values
+		my $values = ref $params[0] ? $params[0] : {@params};
+		@{ $session->{new_flash} //= {} }{ keys %$values } = values %$values;
+
+		return $c;
+	}
+
+	# If session_management_via is "session_cookie", then use the Mojolicious cookie session.
+	return $c->flash(@params);
+}
+
 =head2 store_session
 
 Store the database session. This is called after the current request has been
@@ -788,6 +821,9 @@ sub store_session {
 	if (my $session = $self->{c}->stash->{'webwork2.database_session'}) {
 		debug("Saving database session.  The database session contains\n", $self->{c}->dumper($session));
 
+		delete $session->{session}{flash};
+		delete $session->{session}{new_flash} unless keys %{ $session->{session}{new_flash} };
+
 		my $key = $db->newKey($session);
 		# DBFIXME:  This should be a REPLACE (but SQL::Abstract does not have REPLACE -- SQL::Abstract::mysql does!).
 		eval { $db->deleteKey($session->{user_id}) };
@@ -851,6 +887,9 @@ sub check_session {
 	if ($keyMatches && $timestampValid && $updateTimestamp) {
 		$Key->timestamp($currentTime);
 		$self->{c}->stash->{'webwork2.database_session'} = { $Key->toHash };
+		$self->{c}->stash->{'webwork2.database_session'}{session}{flash} =
+			delete $self->{c}->stash->{'webwork2.database_session'}{session}{new_flash}
+			if $self->{c}->stash->{'webwork2.database_session'}{session}{new_flash};
 	}
 
 	return (1, $keyMatches, $timestampValid);

diff --git a/lib/WeBWorK/ContentGenerator/CourseAdmin.pm b/lib/WeBWorK/ContentGenerator/CourseAdmin.pm
@@ -44,12 +44,6 @@ sub pre_header_initialize ($c) {
 
 	return unless $authz->hasPermissions($user, 'create_and_delete_courses');
 
-	# Get result and send to message
-	# FIXME: I am pretty sure this is not used anymore. All methods add their messages directly to the page except the
-	# table messages, and those are added below and not here.
-	my $status_message = $c->param('status_message');
-	$c->addmessage($c->tag('p', $c->b($status_message))) if $status_message;
-
 	# Check that the non-native tables are present in the database.
 	# These are the tables which are not course specific.
 	my @table_update_messages = initNonNativeTables($ce, $ce->{dbLayoutName});

diff --git a/lib/WeBWorK/ContentGenerator/Instructor/AchievementEditor.pm b/lib/WeBWorK/ContentGenerator/Instructor/AchievementEditor.pm
@@ -81,7 +81,7 @@ sub initialize ($c) {
 
 	return unless ($authz->hasPermissions($user, 'edit_achievements'));
 
-	$c->addmessage($c->param('status_message') || '');    # Record status messages carried over from a redirect
+	$c->addmessage($c->authen->flash('status_message') || '');    # Record status messages carried over from a redirect
 
 	# Check source file path
 	if (not(-e $sourceFilePath)) {
@@ -304,15 +304,13 @@ sub save_as_handler ($c) {
 
 	# Set up redirect
 	# The redirect gives the server time to detect that the new file exists.
+	$c->authen->flash(status_message => $c->{status_message}->join(''));
 	$c->reply_with_redirect($c->systemLink(
 		$c->url_for(
 			'instructor_achievement_editor',
 			achievementID => $saveMode eq 'use_in_new' ? $targetAchievementID : $achievementName
 		),
-		params => {
-			sourceFilePath => $c->getRelativeSourceFilePath($outputFilePath),
-			status_message => $c->{status_message}->join('')
-		}
+		params => { sourceFilePath => $c->getRelativeSourceFilePath($outputFilePath) }
 
 	));
 	return;

diff --git a/lib/WeBWorK/ContentGenerator/Instructor/AchievementNotificationEditor.pm b/lib/WeBWorK/ContentGenerator/Instructor/AchievementNotificationEditor.pm
@@ -76,7 +76,7 @@ sub initialize ($c) {
 
 	return unless ($authz->hasPermissions($user, 'edit_achievements'));
 
-	$c->addmessage($c->param('status_message') || '');    # Record status messages carried over from a redirect
+	$c->addmessage($c->authen->flash('status_message') || '');    # Record status messages carried over from a redirect
 
 	# Check source file path
 	if (!-e $sourceFilePath) {
@@ -233,10 +233,9 @@ sub save_as_handler ($c) {
 
 	# A redirect is needed to ensure that all data and parameters for page display are updated correctly.
 	# FIXME: This could be done without a redirect if the data and parameters were updated here instead.
+	$c->authen->flash(status_message => $c->{status_message}->join(''));
 	$c->reply_with_redirect($c->systemLink(
-		$c->url_for('instructor_achievement_notification', achievementID => $achievementName),
-		params => { status_message => $c->{status_message}->join('') }
-	));
+		$c->url_for('instructor_achievement_notification', achievementID => $achievementName)));
 	return;
 }
 
@@ -263,10 +262,9 @@ sub existing_handler ($c) {
 
 	# A redirect is needed to ensure that all data and parameters for page display are updated correctly.
 	# FIXME: This could be done without a redirect if the data and parameters were updated here instead.
+	$c->authen->flash(status_message => $c->{status_message}->join(''));
 	$c->reply_with_redirect($c->systemLink(
-		$c->url_for('instructor_achievement_notification', achievementID => $achievementID),
-		params => { status_message => $c->{status_message}->join('') }
-	));
+		$c->url_for('instructor_achievement_notification', achievementID => $achievementID)));
 	return;
 }
 
@@ -280,10 +278,8 @@ sub disable_handler ($c) {
 		));
 
 		# Redirect to the instructor_achievement_list.
-		$c->reply_with_redirect($c->systemLink(
-			$c->url_for('instructor_achievement_list'),
-			params => { status_message => $c->{status_message}->join('') }
-		));
+		$c->authen->flash(status_message => $c->{status_message}->join(''));
+		$c->reply_with_redirect($c->systemLink($c->url_for('instructor_achievement_list')));
 	} else {
 		$c->addbadmessage($c->maketext(
 			'Unable to disable the achievement notification template for achievement [_1]. Unknown error.',

diff --git a/lib/WeBWorK/ContentGenerator/Instructor/PGProblemEditor.pm b/lib/WeBWorK/ContentGenerator/Instructor/PGProblemEditor.pm
@@ -249,7 +249,7 @@ sub initialize ($c) {
 		&& $authz->hasPermissions($user, 'modify_problem_sets');
 
 	# Record status messages carried over if this is a redirect
-	$c->addmessage($c->param('status_message') || '');
+	$c->addmessage($c->authen->flash('status_message') || '');
 
 	$c->addbadmessage($c->maketext('Changes in this file have not yet been permanently saved.'))
 		if $c->{inputFilePath} eq $c->{tempFilePath} && -r $c->{tempFilePath};
@@ -777,6 +777,7 @@ sub view_handler ($c) {
 		# We need to know if the set is a gateway set to determine the redirect.
 		my $globalSet = $c->db->getGlobalSet($c->{setID});
 
+		$c->authen->flash(status_message => $c->{status_message}->join(''));
 		$c->reply_with_redirect($c->systemLink(
 			defined $globalSet && $globalSet->assignment_type =~ /gateway/
 			? $c->url_for('gateway_quiz',   setID => 'Undefined_Set')
@@ -785,57 +786,56 @@ sub view_handler ($c) {
 				displayMode    => $displayMode,
 				problemSeed    => $problemSeed,
 				editMode       => 'temporaryFile',
-				sourceFilePath => $relativeTempFilePath,
-				status_message => $c->{status_message}->join('')
+				sourceFilePath => $relativeTempFilePath
 			}
 		));
 	} elsif ($c->{file_type} eq 'blank_problem') {
 		# Redirect to Problem.pm.pm.
+		$c->authen->flash(status_message => $c->{status_message}->join(''));
 		$c->reply_with_redirect($c->systemLink(
 			$c->url_for('problem_detail', setID => 'Undefined_Set', problemID => 1),
 			params => {
 				displayMode    => $displayMode,
 				problemSeed    => $problemSeed,
 				editMode       => 'temporaryFile',
-				sourceFilePath => $relativeTempFilePath,
-				status_message => $c->{status_message}->join('')
+				sourceFilePath => $relativeTempFilePath
 			}
 		));
 	} elsif ($c->{file_type} eq 'set_header') {
 		# Redirect to ProblemSet
+		$c->authen->flash(status_message => $c->{status_message}->join(''));
 		$c->reply_with_redirect($c->systemLink(
 			$c->url_for('problem_list', setID => $c->{setID}),
 			params => {
 				set_header     => $c->{tempFilePath},
 				displayMode    => $displayMode,
 				problemSeed    => $problemSeed,
 				editMode       => 'temporaryFile',
-				sourceFilePath => $relativeTempFilePath,
-				status_message => $c->{status_message}->join('')
+				sourceFilePath => $relativeTempFilePath
 			}
 		));
 	} elsif ($c->{file_type} eq 'hardcopy_header') {
 		# Redirect to ProblemSet?? It's difficult to view temporary changes for hardcopy headers.
+		$c->authen->flash(status_message => $c->{status_message}->join(''));
 		$c->reply_with_redirect($c->systemLink(
 			$c->url_for('problem_list', setID => $c->{setID}),
 			params => {
 				set_header     => $c->{tempFilePath},
 				displayMode    => $displayMode,
 				problemSeed    => $problemSeed,
 				editMode       => 'temporaryFile',
-				sourceFilePath => $relativeTempFilePath,
-				status_message => $c->{status_message}->join('')
+				sourceFilePath => $relativeTempFilePath
 			}
 		));
 	} elsif ($c->{file_type} eq 'course_info') {
 		# Redirect to ProblemSets.pm.
+		$c->authen->flash(status_message => $c->{status_message}->join(''));
 		$c->reply_with_redirect($c->systemLink(
 			$c->url_for('set_list'),
 			params => {
 				course_info    => $c->{tempFilePath},
 				editMode       => 'temporaryFile',
-				sourceFilePath => $relativeTempFilePath,
-				status_message => $c->{status_message}->join('')
+				sourceFilePath => $relativeTempFilePath
 			}
 		));
 	} else {
@@ -908,6 +908,7 @@ sub add_problem_handler ($c) {
 		$c->{file_type} = 'problem';    # Change file type to problem if it is not already that.
 
 		# Redirect to problem editor page.
+		$c->authen->flash(status_message => $c->{status_message}->join(''));
 		$c->reply_with_redirect($c->systemLink(
 			$c->url_for(
 				'instructor_problem_editor_withset_withproblem',
@@ -919,7 +920,6 @@ sub add_problem_handler ($c) {
 				problemSeed    => $c->{problemSeed},
 				editMode       => 'savedFile',
 				sourceFilePath => $c->getRelativeSourceFilePath($sourceFilePath),
-				status_message => $c->{status_message}->join(''),
 				file_type      => 'problem',
 			}
 		));
@@ -944,13 +944,10 @@ sub add_problem_handler ($c) {
 		$c->{file_type} = 'set_header';    # Change file type to set_header if not already so.
 
 		# Redirect
+		$c->authen->flash(status_message => $c->{status_message}->join(''));
 		$c->reply_with_redirect($c->systemLink(
 			$c->url_for('problem_list', setID => $targetSetName),
-			params => {
-				displayMode    => $c->{displayMode},
-				editMode       => 'savedFile',
-				status_message => $c->{status_message}->join(''),
-			}
+			params => { displayMode => $c->{displayMode}, editMode => 'savedFile' }
 		));
 	} elsif ($targetFileType eq 'hardcopy_header') {
 		# Update set record
@@ -973,13 +970,10 @@ sub add_problem_handler ($c) {
 		$c->{file_type} = 'hardcopy_header';    # Change file type to set_header if not already so.
 
 		# Redirect
+		$c->authen->flash(status_message => $c->{status_message}->join(''));
 		$c->reply_with_redirect($c->systemLink(
 			$c->url_for('hardcopy_preselect_set', setID => $targetSetName),
-			params => {
-				displayMode    => $c->{displayMode},
-				editMode       => 'savedFile',
-				status_message => $c->{status_message}->join(''),
-			}
+			params => { displayMode => $c->{displayMode}, editMode => 'savedFile' }
 		));
 	} else {
 		die "Unsupported target file type $targetFileType";
@@ -1012,6 +1006,7 @@ sub save_handler ($c) {
 		# We need to know if the set is a gateway set to determine the redirect.
 		my $globalSet = $c->db->getGlobalSet($c->{setID});
 
+		$c->authen->flash(status_message => $c->{status_message}->join(''));
 		$c->reply_with_redirect($c->systemLink(
 			defined $globalSet && $globalSet->assignment_type =~ /gateway/
 			? $c->url_for('gateway_quiz',   setID => 'Undefined_Set')
@@ -1020,52 +1015,50 @@ sub save_handler ($c) {
 				displayMode    => $c->{displayMode},
 				problemSeed    => $c->{problemSeed},
 				editMode       => 'savedFile',
-				sourceFilePath => $c->getRelativeSourceFilePath($c->{editFilePath}),
-				status_message => $c->{status_message}->join('')
+				sourceFilePath => $c->getRelativeSourceFilePath($c->{editFilePath})
 			}
 		));
 	} elsif ($c->{file_type} eq 'set_header') {
 		# Redirect to ProblemSet.pm
+		$c->authen->flash(status_message => $c->{status_message}->join(''));
 		$c->reply_with_redirect($c->systemLink(
 			$c->url_for('problem_list', setID => $c->{setID}),
 			params => {
-				displayMode    => $c->{displayMode},
-				problemSeed    => $c->{problemSeed},
-				editMode       => 'savedFile',
-				status_message => $c->{status_message}->join('')
+				displayMode => $c->{displayMode},
+				problemSeed => $c->{problemSeed},
+				editMode    => 'savedFile'
 			}
 		));
 	} elsif ($c->{file_type} eq 'hardcopy_header') {
 		# Redirect to Hardcopy.pm
+		$c->authen->flash(status_message => $c->{status_message}->join(''));
 		$c->reply_with_redirect($c->systemLink(
 			$c->url_for('hardcopy_preselect_set', setID => $c->{setID}),
 			params => {
-				displayMode    => $c->{displayMode},
-				problemSeed    => $c->{problemSeed},
-				editMode       => 'savedFile',
-				status_message => $c->{status_message}->join('')
+				displayMode => $c->{displayMode},
+				problemSeed => $c->{problemSeed},
+				editMode    => 'savedFile'
 			}
 		));
 	} elsif ($c->{file_type} eq 'hardcopy_theme') {
 		# Redirect to PGProblemEditor.pm
+		$c->authen->flash(status_message => $c->{status_message}->join(''));
 		$c->reply_with_redirect($c->systemLink(
 			$c->url_for('instructor_problem_editor'),
 			params => {
 				editMode       => 'savedFile',
 				hardcopy_theme => $c->{hardcopy_theme},
 				file_type      => 'hardcopy_theme',
-				status_message => $c->{status_message}->join(''),
 				sourceFilePath => $c->getRelativeSourceFilePath($c->{editFilePath}),
 			}
 		));
 	} elsif ($c->{file_type} eq 'course_info') {
 		# Redirect to ProblemSets.pm
-		$c->reply_with_redirect($c->systemLink(
-			$c->url_for('set_list'),
-			params => { editMode => 'savedFile', status_message => $c->{status_message}->join('') }
-		));
+		$c->authen->flash(status_message => $c->{status_message}->join(''));
+		$c->reply_with_redirect($c->systemLink($c->url_for('set_list'), params => { editMode => 'savedFile' }));
 	} elsif ($c->{file_type} eq 'source_path_for_problem_file') {
 		# Redirect to PGProblemEditor.pm
+		$c->authen->flash(status_message => $c->{status_message}->join(''));
 		$c->reply_with_redirect($c->systemLink(
 			$c->url_for(
 				'instructor_problem_editor_withset_withproblem',
@@ -1078,8 +1071,7 @@ sub save_handler ($c) {
 				editMode    => 'savedFile',
 				# The path relative to the templates directory is required.
 				sourceFilePath => $c->{editFilePath},
-				file_type      => 'source_path_for_problem_file',
-				status_message => $c->{status_message}->join('')
+				file_type      => 'source_path_for_problem_file'
 			}
 		));
 	} else {
@@ -1237,7 +1229,7 @@ sub save_as_handler ($c) {
 				$c->shortPath($outputFilePath)
 			));
 			$c->addgoodmessage($c->maketext(' No changes have been made to set [_1]', $c->{setID}))
-				if ($c->{setID} ne 'Undefined_Set');
+				if $c->{setID} && $c->{setID} ne 'Undefined_Set';
 		} else {
 			$c->addbadmessage($c->maketext('Unkown saveMode: [_1].', $saveMode));
 			return;
@@ -1281,14 +1273,14 @@ sub save_as_handler ($c) {
 		return;
 	}
 
+	$c->authen->flash(status_message => $c->{status_message}->join(''));
 	$c->reply_with_redirect($c->systemLink(
 		$problemPage,
 		params => {
 			# The path relative to the templates directory is required.
 			sourceFilePath => $c->getRelativeSourceFilePath($outputFilePath),
 			problemSeed    => $c->{problemSeed},
 			file_type      => $new_file_type,
-			status_message => $c->{status_message}->join(''),
 			%extra_params
 		}
 	));

diff --git a/lib/WeBWorK/ContentGenerator/Problem.pm b/lib/WeBWorK/ContentGenerator/Problem.pm
@@ -430,7 +430,7 @@ async sub pre_header_initialize ($c) {
 	$c->{formFields}     = $formFields;
 
 	# Get the status message and add it to the messages.
-	$c->addmessage($c->tag('p', $c->b($c->param('status_message')))) if $c->param('status_message');
+	$c->addmessage($c->tag('p', $c->b($c->authen->flash('status_message')))) if $c->authen->flash('status_message');
 
 	# Now that the necessary variables are set, return if the set or problem is invalid.
 	return if $c->{invalidSet} || $c->{invalidProblem};