From a3e239264709d22227cb3d8eddffc42327468992 Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Fri, 23 Oct 2026 11:31:52 -0500 Subject: [PATCH] Prevent duplicate test submissions. If a test submission is slow (for example on a bad connection), then the student can submit the test again while the first submission is still being graded. Since the number of attempts is not saved until grading is complete, both submissions were graded as the same attempt, and the one that finished last overwrote the scores of the other. This happened even for tests that allow only one submission per version. The gateway form now only allows one submission. On the server, the version_last_attempt_time and number of attempts used when the page was rendered are included in the form. A submission is not graded if the number of attempts has changed. Otherwise the submission time is saved with a conditional database update that only succeeds if version_last_attempt_time is unchanged (the new value is always later than the old one, so that two submissions can not both succeed). The proctor login page saves the time in the same way and forwards the new value. Finally, before any scores are saved, the new number of attempts for one problem is saved with a conditional database update that only succeeds if that number is unchanged. That catches a submission from a page rendered while another submission was being graded. To reproduce the issues, make a test with the following problem and at least one other problem, set "Graded submissions per version" to 1 (except as noted), and assign it to a student. Grading the problem takes 10 seconds, which leaves time to submit again during grading. DOCUMENT(); loadMacros('PGstandard.pl', 'PGML.pl'); if ($inputs_ref->{submitAnswers}) { my $start = time; 1 while time - $start < 10; } BEGIN_PGML What is [`1 + 1`]? [_]{2} END_PGML ENDDOCUMENT(); In each case below, before this change both submissions are recorded and the last to finish overwrites the scores of the other. With this change only the first submission is recorded. 1. Answer all problems, click "Grade Test" and "Yes", and then click "Grade Test" again within 10 seconds. 2. Set the test to show one problem per page. Answer the first problem, click "Next Problem", answer the second problem, and click "Grade Test" on that page. Within 10 seconds, click the back button and click "Grade Test" again. The page that the back button returns to does not have the answer to the second problem, so before this change that answer is lost and the problem is scored as unanswered (the answer log shows both submissions). Grading an older copy of the test that is open in another tab is the same. 3. Answer all problems, grade the test, reload the page (F5) within 10 seconds, change an answer, and grade the test again. 4. With "Graded submissions per version" set to 2, repeat 2 but wait for the results before clicking the back button. Before this change, the submission from the old page is graded as the second attempt. With this change it is not graded. 5. Change the test to a proctored test that requires proctor authorization to grade, and repeat 1 and 2, authorizing each submission with a proctor account. Also click the proctor login button twice for one submission. Only one submission is graded. Co-Authored-By: Claude Opus 5.5 --- htdocs/js/GatewayQuiz/gateway.js | 19 ++++- lib/WeBWorK/ContentGenerator/GatewayQuiz.pm | 79 +++++++++++++++++-- lib/WeBWorK/ContentGenerator/LoginProctor.pm | 23 +++--- .../ContentGenerator/GatewayQuiz.html.ep | 2 + 4 files changed, 103 insertions(+), 20 deletions(-) diff --git a/htdocs/js/GatewayQuiz/gateway.js b/htdocs/js/GatewayQuiz/gateway.js index 19d58d2dd2..e863679e6c 100644 --- a/htdocs/js/GatewayQuiz/gateway.js +++ b/htdocs/js/GatewayQuiz/gateway.js @@ -8,6 +8,23 @@ (() => { if (!document.gwquiz) return; + // Only allow the form to be submitted once. The page remains displayed while the request is pending, and so if the + // server response is slow the user might click on "Grade Test" again which would send another submission without + // confirmation. + let formSubmitted = false; + document.gwquiz.addEventListener('submit', (evt) => { + if (formSubmitted) evt.preventDefault(); + formSubmitted = true; + }); + + // If the page is restored from the back-forward cache, then allow the form to be submitted again. The server will + // refuse to grade a submission from this page if the test was graded in the meantime. + window.addEventListener('pageshow', (evt) => { + if (!evt.persisted) return; + formSubmitted = false; + actuallySubmit = false; + }); + // Gateway timer const timerDiv = document.getElementById('gwTimer'); // The timer div element let actuallySubmit = false; // This needs to be set to true to allow an actual submission. @@ -386,7 +403,7 @@ // Prevent the link from being followed. evt.preventDefault(); document.gwquiz.newPage.value = pageChangeLink.dataset.pageNumber; - document.gwquiz.submit(); + document.gwquiz.requestSubmit(); }); }); diff --git a/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm b/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm index 90e9c46ebf..eaec5b4ad1 100644 --- a/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm +++ b/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm @@ -11,7 +11,7 @@ deal with versioning sets use Mojo::Promise; use Mojo::JSON qw(encode_json decode_json); -use WeBWorK::Utils qw(encodeAnswers decodeAnswers wwRound); +use WeBWorK::Utils qw(encodeAnswers decodeAnswers max wwRound); use WeBWorK::Utils::DateTime qw(before between after); use WeBWorK::Utils::Files qw(path_is_subdir); use WeBWorK::Utils::Instructor qw(assignSetVersionToUser); @@ -227,6 +227,39 @@ sub can_gradeUnsubmittedTest ($c, $user, $permissionLevel, $effectiveUser, $set, && (after($set->due_date + $c->ce->{gatewayGracePeriod}) && !$set->version_last_attempt_time); } +# Save the time of a test submission to the set version's version_last_attempt_time, but only if the test has not been +# submitted since the test page was rendered. Returns 1 if the time was saved, and 0 if the test was submitted since +# the page was rendered. This is used to protect against various forms of resubmission being graded and replacing the +# first valid submission. This is also used by the LoginProctor module when a proctored test is submitted. In that +# case the versionLastAttemptTime parameter is updated to the new value so that the proctor login form forwards it to +# this module when the proctor authorizes grading. +sub recordSubmissionTime ($c, $set, $problem) { + my $attemptNumber = $c->param('attemptNumber'); + return 0 + if defined $attemptNumber + && $attemptNumber =~ /^\d+$/ + && $attemptNumber != $problem->num_correct + $problem->num_incorrect; + + my $expectedTime = $c->param('versionLastAttemptTime'); + $expectedTime = $set->version_last_attempt_time || 0 unless defined $expectedTime && $expectedTime =~ /^\d+$/; + my $submissionTime = max(int($c->submitTime), $expectedTime + 1); + + return 0 + if $c->db->{set_version}->update_where( + { version_last_attempt_time => $submissionTime }, + { + user_id => $set->user_id, + set_id => $set->set_id, + version_id => $set->version_id, + version_last_attempt_time => $expectedTime + } + ) == 0; + + $c->{submissionTime} = $submissionTime; + $c->param(versionLastAttemptTime => $submissionTime); + return 1; +} + sub can_showScore ($c, $user, $permissionLevel, $effectiveUser, $set, $problem, $tmplSet) { return $c->authz->hasPermissions($user->user_id, 'view_hidden_work') @@ -799,9 +832,18 @@ async sub pre_header_initialize ($c) { useMathQuill => $user->useMathQuill ne '' ? $user->useMathQuill : $ce->{pg}{options}{useMathQuill}, ); - # Does the user have permission to use certain options? my @args = ($user, $permissionLevel, $effectiveUser, $set, $problem, $tmplSet); - my %can = ( + + if ($want{recordAnswers} && $c->can_recordAnswers(@args) && !$c->recordSubmissionTime($set, $problem)) { + $c->{submitAnswers} = 0; + $want{recordAnswers} = 0; + $c->addbadmessage( + $c->maketext( + 'This submission was not graded because this test was already submitted from another page or window.') + ); + } + + my %can = ( showOldAnswers => $c->can_showOldAnswers(@args), showCorrectAnswers => $c->can_showCorrectAnswers(@args), showProblemGrader => $c->can_showProblemGrader(@args), @@ -981,6 +1023,29 @@ async sub pre_header_initialize ($c) { } my @pureProblems = $db->getAllProblemVersions($effectiveUserID, $setID, $versionID); + + # Another submission may occur from a page rendered after this submission arrived but before it finishes + # grading. So before saving any scores, save the new number of attempts for one problem, but only if it still + # has the number of attempts that it had when this request started. This is done in a single database update, + # so only one submission can be recorded for each attempt. + my $alreadyGraded = 0; + if ($c->{submitAnswers} && $will{recordAnswers}) { + my $firstProblem = $problems[ $probOrder[0] ]; + my $state = $pg_results[ $probOrder[0] ]{state}; + $alreadyGraded = $db->{problem_version}->update_where( + { num_correct => $state->{num_of_correct_ans}, num_incorrect => $state->{num_of_incorrect_ans} }, + { + user_id => $effectiveUserID, + set_id => $setID, + version_id => $versionID, + problem_id => $firstProblem->problem_id, + num_correct => $firstProblem->num_correct || [ 0, undef ], + num_incorrect => $firstProblem->num_incorrect || [ 0, undef ] + } + ) == 0; + $will{recordAnswers} = 0 if $alreadyGraded; + } + for my $i (0 .. $#problems) { # Process each problem. my $pureProblem = $pureProblems[ $probOrder[$i] ]; @@ -1058,7 +1123,11 @@ async sub pre_header_initialize ($c) { . $problem->num_incorrect); } elsif ($c->{submitAnswers}) { # This is the case answers were submitted but cannot be saved. Report an error message. - if ($c->{isClosed}) { + if ($alreadyGraded) { + $scoreRecordedMessage[ $probOrder[$i] ] = $c->maketext( + 'Your score was not recorded because this test was already submitted from another page or ' + . 'window.'); + } elsif ($c->{isClosed}) { $scoreRecordedMessage[ $probOrder[$i] ] = $c->maketext('Your score was not recorded because this problem set version is not open.'); } elsif ($problem->num_correct + $problem->num_incorrect >= $set->attempts_per_version) { @@ -1169,7 +1238,7 @@ async sub pre_header_initialize ($c) { ) { # Save the submission time if we're recording the answer, or if the first submission occurs after the due_date. - $set->version_last_attempt_time($timeNowInt) + $set->version_last_attempt_time($c->{submissionTime} // $timeNowInt) if ( $c->{submitAnswers} && ( diff --git a/lib/WeBWorK/ContentGenerator/LoginProctor.pm b/lib/WeBWorK/ContentGenerator/LoginProctor.pm index 22998bb899..0c10fe31ab 100644 --- a/lib/WeBWorK/ContentGenerator/LoginProctor.pm +++ b/lib/WeBWorK/ContentGenerator/LoginProctor.pm @@ -48,23 +48,18 @@ async sub initialize ($c) { # This should never happen. die 'Request to grade a set version before any tests have been taken.' if $noSetVersions; - # Determine if answers can be recorded, and set last_attempt_time if appropriate. + # Determine if answers can be recorded, and if so record the submission time so that the time spent waiting + # for proctor authorization does not count against the time limit. If that fails because the test was + # already submitted, then this submission will not be graded after proctor authorization. + my $problem = $db->getMergedProblemVersion($effectiveUserID, $c->stash->{setID}, + $versionNum, ($db->listProblemVersions($effectiveUserID, $c->stash->{setID}, $versionNum))[0]); if (WeBWorK::ContentGenerator::GatewayQuiz::can_recordAnswers( - $c, - $db->getUser($userID), - $db->getPermissionLevel($userID), - $c->{effectiveUser}, - $c->stash->{userSet}, - $db->getMergedProblemVersion( - $effectiveUserID, $c->stash->{setID}, - $versionNum, ($db->listProblemVersions($effectiveUserID, $c->stash->{setID}, $versionNum))[0] - ) + $c, $db->getUser($userID), + $db->getPermissionLevel($userID), $c->{effectiveUser}, + $c->stash->{userSet}, $problem )) { - $c->stash->{userSet}->version_last_attempt_time(int($c->submitTime)); - # FIXME: This saves all of the merged set data into the set_user table. We live with this in other - # places for versioned sets, but it's not ideal. - $db->putSetVersion($c->stash->{userSet}); + WeBWorK::ContentGenerator::GatewayQuiz::recordSubmissionTime($c, $c->stash->{userSet}, $problem); } } } diff --git a/templates/ContentGenerator/GatewayQuiz.html.ep b/templates/ContentGenerator/GatewayQuiz.html.ep index 7793080d15..fb50eb7dcb 100644 --- a/templates/ContentGenerator/GatewayQuiz.html.ep +++ b/templates/ContentGenerator/GatewayQuiz.html.ep @@ -425,6 +425,8 @@ <%= hidden_field courseID => $ce->{courseName} =%> % <%= hidden_field startTime => param('startTime') || time =%> + <%= hidden_field versionLastAttemptTime => $c->{set}->version_last_attempt_time || 0 =%> + <%= hidden_field attemptNumber => $c->{attemptNumber} =%> % if ($numProbPerPage && $numPages > 1) { <%= hidden_field newPage => '' =%> <%= hidden_field currentPage => $pageNumber =%>