From ef6f4249fed4b6da7a66f82fd802a8d5146f359b Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Fri, 23 Feb 2024 16:20:50 -0600 Subject: [PATCH] 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 #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. --- lib/WeBWorK/Authen.pm | 39 ++++++++++ lib/WeBWorK/ContentGenerator/CourseAdmin.pm | 6 -- .../Instructor/AchievementEditor.pm | 8 +- .../AchievementNotificationEditor.pm | 18 ++--- .../Instructor/PGProblemEditor.pm | 74 +++++++++---------- lib/WeBWorK/ContentGenerator/Problem.pm | 2 +- lib/WeBWorK/ContentGenerator/ProblemSet.pm | 2 +- lib/WeBWorK/ContentGenerator/ProblemSets.pm | 2 +- 8 files changed, 85 insertions(+), 66 deletions(-) diff --git a/lib/WeBWorK/Authen.pm b/lib/WeBWorK/Authen.pm index e56e4d0bef..28bc657114 100644 --- a/lib/WeBWorK/Authen.pm +++ b/lib/WeBWorK/Authen.pm @@ -799,6 +799,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 @@ -814,6 +847,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}) }; @@ -877,6 +913,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 index f30be1fca9..8e91360a64 100644 --- 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 index 67f0ab6c00..2ef682c41c 100644 --- 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 index 01abffb81f..628f69df65 100644 --- 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 index 6b33021c42..f498deeec1 100644 --- 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,24 +786,24 @@ 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 => { @@ -810,12 +811,12 @@ 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 '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 => { @@ -823,19 +824,18 @@ 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 '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,6 +1273,7 @@ sub save_as_handler ($c) { return; } + $c->authen->flash(status_message => $c->{status_message}->join('')); $c->reply_with_redirect($c->systemLink( $problemPage, params => { @@ -1288,7 +1281,6 @@ sub save_as_handler ($c) { 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 index b37eaa5e7e..7854d49527 100644 --- 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}; diff --git a/lib/WeBWorK/ContentGenerator/ProblemSet.pm b/lib/WeBWorK/ContentGenerator/ProblemSet.pm index 6980d4c509..999a98bb61 100644 --- a/lib/WeBWorK/ContentGenerator/ProblemSet.pm +++ b/lib/WeBWorK/ContentGenerator/ProblemSet.pm @@ -51,7 +51,7 @@ async sub initialize ($c) { $c->{displayMode} = $user->displayMode || $ce->{pg}{options}{displayMode}; # Display status 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'); if ($authz->hasPermissions($userID, 'view_hidden_sets')) { if ($c->{set}->visible) { diff --git a/lib/WeBWorK/ContentGenerator/ProblemSets.pm b/lib/WeBWorK/ContentGenerator/ProblemSets.pm index 0d45caa50d..a50d5b649b 100644 --- a/lib/WeBWorK/ContentGenerator/ProblemSets.pm +++ b/lib/WeBWorK/ContentGenerator/ProblemSets.pm @@ -59,7 +59,7 @@ sub initialize ($c) { my $user = $c->param('user'); if ($authz->hasPermissions($user, 'access_instructor_tools')) { - my $status_message = $c->param('status_message'); + my $status_message = $c->authen->flash('status_message'); $c->addmessage($c->tag('p', $c->b($status_message))) if $status_message; }