Patches to scaffold.pl so Preview does not modify which sections are open #506

taniwallach · 2020-10-29T20:37:25Z

Edited on 14-APR-2023 revised for current develop and with defaults set to retain prior behavior

The updated version depends on:

Revised handling of PERSISTENCE_HASH data #809
Revised handling of PERSISTENCE_HASH data webwork2#1940
and should be merged only after those PRs.

This is at attempt to address the first issue from #478

Patches to:

prevent scaffold from opening additional section when Preview of the last open section determines it was all correct (when the relevant setting is changed), and
prevent scaffold from closing open sections if an answer to a prior section became incorrect and Preview is being run (when the relevant setting is changed), and
control switches were added to regulate these behaviors, with defaults to the preexisting behavior. (updated to those defaults in response to the review comments)

The revised code stores the state about the last open section in the database using the code added in #809.

The current patch uses a hidden form field to provide the server with the "number" of the last scaffold section which was open. It does not have any mechanism to prevent a clever user from tampering with that value. As a result, the current implement can be abused to trick the system into opening additional sections if the value of the hidden field is modified to a large value.

My plan is to fix that by adding additional hidden fields including a (random) "nonce" and a "signature" that verifies (together with some server side state information which should remain constant for the user of a given problem) that these hidden fields were not tampered with. Such an approach may not be 100% secure, but should make it quite hard to abuse these changes.

~~However, the current draft code already provides the rough functionality I think we would like.~~

Testing - using the PG file in scaffold-sample-01.zip and the files in scaffold-sample-new.zip

Before installing the patch:

Open any of the sample problems, and enter answer 1,2 in part 1.
Click "Preview My Answers" .
Part 2 should open - even though the correct answer to part 1 was not submitted for grading. (No attempt was counted.)
Enter answer 1,2 in part 2.
Click "Preview My Answers" .
Part 3 should open - even though the correct answer to part 2 was not submitted for grading. (No attempt was counted.)
Make one of the answers in part 1 and one of the answers in part 2 incorrect.
Click "Preview My Answers" .
Parts 2 and 3 should close - even though only a "Preview" was requests.
Make the answer in part 1 correct, and click "Preview My Answers". (Part 2 should open.)
Make the answer in part 2 correct, and click "Preview My Answers". (Part 3 should open.)
Make the answer in part 2 incorrect, and click "Preview My Answers". (Part 3 should close.)
Make the answer in part 2 correct, and click "Preview My Answers". (Part 3 should open.)
Make the answer in part 1 incorrect, and click "Preview My Answers". (Part 3 should close, but part 1 will remain open as it is correct.)

The ~~desired~~ newly supported alternate behavior requires using "Submit Answers" to get the additional sections to open, and would not close open sections on use of "Preview My Answers" when the relevant settings are used.

The defaults have been changed - so the testing instructions here are somewhat outdated. See below for revised testing instructions.

After installing the patch:

Testing using scaffold-sample-03.pg should give exactly the same behavior as in the test before the patch, as it (explicitly) uses the settings to keep the behavior as it was.
Testing using scaffold-sample-02.pg should behave as follows (both settings are flipped):

Open the problem, and enter answer 1,2 in part 1.
Click "Preview My Answers" .
Part 2 should not open.
Click "Submit Answers".
Part 2 should now open (and an attempt was used up to get it to open).
Enter answer 1,2 in part 2.
Click "Submit Answers".
Part 3 should open (and an attempt was used up to get it to open).
Make one of the answers in part 1 and one of the answers in part 2 incorrect.
Click "Preview My Answers" .
Parts 2 and 3 should remain open.
Click "Submit Answers".
Parts 2 and 3 should now close.
Make the answer in part 1 correct, and click "Submit Answers". (Part 2 should open.)
Make the answer in part 2 correct, and click "Submit Answers". (Part 3 should open.)
Make the answer in part 2 incorrect, and click "Preview My Answers". (Part 3 should remain open.)
Click "Submit Answers". (Part 3 should close.)
Make the answer in part 2 correct the answer in part 1 incorrect, and click "Preview My Answers". (Part 2 should remain open).
Click "Submit Answers". (Part 2 should close.)

Testing using scaffold-sample-01.pg should behave as follows (only opening by preview is disabled, but closing remains enabled.):

Open the problem, and enter answer 1,2 in part 1.
Click "Preview My Answers" .
Part 2 should not open.
Click "Submit Answers".
Part 2 should now open (and an attempt was used up to get it to open).
Enter answer 1,2 in part 2.
Click "Submit Answers".
Part 3 should open (and an attempt was used up to get it to open).
Make one of the answers in part 1 and one of the answers in part 2 incorrect.
Click "Preview My Answers" .
Parts 2 and 3 should both close.

taniwallach · 2020-10-29T20:49:15Z

Note the use of scaffolding where some "later" sections are intended to be open before some earlier ones is likely to have issues with this patch. In such cases, the new functionality should probably be disabled using the new control switch options for Scaffold. Support for such usage would probably require storing an array of open section numbers instead of just the number of the last open section. Whether there is sufficient need to consider implementing such can be determined in the future.

taniwallach · 2020-10-30T08:25:32Z

No longer relevant. State is now in the database and not in a hidden form field and cannot be tampered with by the student.

I have been thinking some about this.

I think that the "security risk" of having prevent_close_by_preview default to 1, which would allow "hacking" hidden value to force all sections open outweighs the intended benefit of preventing sections from closing on Preview when a prior sections answers are damaged. I will change the default in a future commit. For testing, once that is done - the test file will need to be replaced to set it to 1 to have the behavior intended for testing.
I think that the "security risk" of having allow_next_section_to_open_on_preview default to 0, and of "hacked" input being able to open the next section using Preview of correct answers, without a submission be required is not that bad - it is essentially what happens now, but requires the hack to occur.
For the long term, I think we should try to create a mechanism for securing state parameters like this in a tamper resistant manner.
- I envision a local "API" in PG using which PG code can:
  - set a key to a value,
  - retrieve the value from the current submission (with the back-end code checking that the tamper resistance guarantee is satisfied
- The API would:
  - "envelop" all the necessary information including the "nonce" and the digital signature into something put into a hidden field (or a cookie, or something else) when a problem is generated, and
  - handle the parsing / tamper check, etc. and provide the data back to PG when needed.
- Using an API would allow the technology proving the feature to be modified with no need for problem code to be aware of it.
- Maybe JWT (JSON Web Token) https://jwt.io/introduction/ is a suitable technology for the initial implementation. (This technology was mentioned in a recent Zoom meeting as being something considered to help interface LibreTexts to WW.)

drgrice1

You have this set up to change the defaults to the new behavior that you are introducing. That should not be the case. There are instructors out there that depend on this default behavior. In fact @drdrew42 uses scaffolds in this way. His point is that he wants students to be able to use the preview button to work through the problem without ever having hit submit until all answers are entered. He wants this as it pertains to a certain achievement that tracks incorrect submissions. With your new settings the only way to progress to the next section is to hit submit and get that part correct. That means the unanswered latter parts are incorrect, and this achievement will note that.

macros/scaffold.pl

taniwallach · 2021-03-08T17:20:11Z

This was originally posted as a proof of concept, but now that the defaults keep the existing behavior unchanged, the only reason to defer merging it is the hidden issue that tampering with the hidden form field can make trouble. Fixing that requires some method of preventing such tampering which is not something I think I can have ready in time for the release candidate of PG 2.16. As such I have marked this as deferred. If people are comfortable with that risk, the current version can be merged, and improvements can be made later.

pstaabp · 2023-04-11T12:47:41Z

@taniwallach Do we want to revisit this to get into 2.18? If so, need to fix the conflicts.

macros/core/scaffold.pl

taniwallach · 2023-04-19T13:36:26Z

I need to fix this for a change made in #809 so it is marked as not ready to merge.
The missing change will not impact testing.

taniwallach · 2023-04-20T16:33:00Z

I made some additional changes so that when a MultiAnswer is in an unopened section and is using singleResult => 1 then the preview of the first section will not reveal structural information about the answer in the second section. This is a somewhat related flaw in scaffold.pl when used with such a MultiAnswer at present.

To see the behavior I think is not correct do a preview of the problem attached below (using a student account, not an instructor account) without the second section being opened before the changes of the second commit (or commenting out the line which sets scaffold_past_open in macros/core/scaffold.pl). In that case, the preview is "leaking" a hint that there are 2 vectors in the basis in part 2.

With the new changes, that does not occur.

Right now the new code sets scaffold_past_open in all answer hashes past the "last open" section, and parsers/parserMultiAnswer.pl was modified not to set values for preview_latex_string and preview_text_string if that is set.

@drgrice1 Do you have a better idea for how this can be done.

There are 2 local files to use with the sample problem. All files were renamed to end with .txt to GitHub allows them.

Files:

Note: The rank.pl and custom basis checker file relate to #409 and if we can decide how to add rank.pl (or nicely add the rank function to the Matrix MathObject in some other manner) and revise the basis checkers for the new release that would be nice. I have modified versions of the various basis checkers.

A version of the problem which demonstrates the bug in code based on the current basis checker (marking incorrect answers as correct) is in:

find_dim_basis_bug.pg.txt
An incorrect answer in part 2 which gets marked as correct due to the bug is

1,2,1,2
sqrt(253), 2sqrt(253), sqrt(253), 0.001+2sqrt(253)

It seems that there may be problems with the new features in this PR if a problem has answer blanks in an "always open" section which come after sections which are only open when the prior section was correct. I think not supporting that sort of use case for the features added is a relatively reasonable limitation.

pstaabp · 2023-04-21T13:36:19Z

With this branch, when I first encounter a problem, I'm getting the following error:

Use of uninitialized value $prior_last_open in numeric gt (>) at line 470 of [PG]/macros/core/scaffold.pl.

I think this is because there is no data in the problem_data field of the user problem. After the first time through, it works. Perhaps we need a default value for $prior_last_open around line 448 of scaffold.pl

are open when Preview is called: (a) Allow preventing opening an additional section when all answers of the last open section are correct during processing a Preview render, and (b) Allow preventing scaffold closing previously open sections if an answer to a prior section became incorrect when processing a Preview render. Control switches control if these modified behaviors are used or not. The defaults maintain the prior behavior. Some contributions to a prior version of this code by Glenn Rice.

preview by setting boolean scaffold_past_open in the rh_ans hash of all answers in sections which were past the last open one.

taniwallach · 2023-04-22T19:32:52Z

prior_last_open

@pstaabp I think I fixed that. I had moved some things around when working on the changes for multianswer but only tested on an old problem so did not see that error, and do not think I ever saw anything like that with the earlier versions of the branch.

Could we try to merge the two PRs about PERSISTENCE_HASH data on which this depends before we finish up this one. It became quite tedious updating this PR with the changes made to get #809 ready for review and to fix the issues/feedback there.

pstaabp · 2023-04-25T17:43:24Z

@taniwallach If you make changes to #506, you can then rebase this branch onto that.

The latest commit seems to have fixed the error I was seeing before.

taniwallach · 2023-04-25T22:11:57Z

I have been rebasing, possible not in an efficient manner. Right now this is built on the most recent version of #809.

taniwallach · 2023-04-25T22:14:16Z

@pstaabp Before you approved did you have a look at the changes to MultiAnswer preview handling for the singleResult => 1 case? That was a pretty new addition to this PR.

drgrice1

I mentioned that I don't like how in openwebwork/webwork2#1940 the problem_data field is set even when answers are not submitted. I still don't think that is a good idea, and I am rather certain that what you are trying to accomplish in this pull request can be done without that. Furthermore, I think that there is a way to do this that doesn't even use the database. This causes rather inconsistent behavior when rendering problems in many of the various different ways that problems can be rendered. Your new problem_data behavior was not implemented for the render_rpc or html2xml endpoints, it doesn't take into account instructor status, and in general does not seem to be fully thought out. There are other macros that preserve state without this problem_data database column, and I am certain that what you are trying to accomplish can be done using similar techniques. Both Geogebra applets and MathQuill do that sort of thing using hidden inputs and the KEPT_EXTRA_ANSWERS hash. The problem is that this is saving state and making it persistent in some cases where state should not be persistent.

When an instructor opens a scaffold problem in an assignment, then the "Check Answers" button is available and furthermore scaffolds can be manually opened by the instructors. This is not taken into account in what is done here, and results in some rather odd behavior of scaffolds. With or without this pull request and using your first sample problem (scaffold-sample-01.pg), if the instructor opens a later section, answers it correctly (other sections are left unanswered), and clicks "Check Answers", then that section comes back open and that section is colored green. The difference is in what happens if you next click "Preview My Answers". Without this pull request if you click preview then that last correctly answered section comes back still open and colored green. With this pull request, that section is closed and colored yellow. Only the first unanswered section is now open.

If the instructor is testing the problem on the homework set detail page or in the PG problem editor the new database column is not used, and completely different behavior is exhibited.

Another problem here is that this is just adding to the list of things that scaffold.pl does that are problem authoring decisions, that really should be instructor decisions. We have discussed this already several times during our meetings. Ultimately it is very confusing for a problem author what all of the options are and do that control when a scaffold is open or not. This is further compounded by the fact that authors are going to be testing the problem initially when writing the problem in either the PG problem editor or as an instructor, and the behavior will be inconsistent with what the student will see later in an actual set.

I think in general we need to have more discussion about what is really desired for scaffold behavior, and talk about a good way of implementing it that gives better consistency. I think that the saving of the problem_data column should be rewritten to not occur unless answers are actually being saved. Although, I think we also need to review the KEPT_EXTRA_ANSWERS and PERSISTENCE_HASH. Are they both needed?

As to your changes for parserMultiAnswer.pl problems, that doesn't seem to be working. I tested this both as an instructor and as a student, and I see the same behavior. If "Preview My Answers" is clicked on your find_dim_basis.pg problem, the results table still reveals information about the second part of the problem. In fact I see no difference at all for the behavior with or without this pull request.

drgrice1 · 2023-04-28T02:41:34Z

macros/core/scaffold.pl

+occur due to a Preview, while when set to 1 sections will not be closed by
+Preview.
+
+


Please delete the extra empty line here. No multiple empty lines! perltidy removes the ones not in POD, but doesn't handle multiple empty lines in POD.

drgrice1 · 2023-04-28T02:45:18Z

macros/core/scaffold.pl

+			} elsif (($section_no < $prior_last_open) && $self->{prevent_close_by_preview}) {
+				# This section was open before - AVOID closing it due to a change in prior sections and a Preview
+				for my $name (@{ $self->{ans_names} }) {
+					$self->{scores}{$name} = 1.0 if ($answers{$name});    # to prevent it from being closed


Why 1.0? Perl doesn't distinguish between integer and floating point data types, so there is no need. Just use 1.

drgrice1 · 2023-04-28T02:51:39Z

macros/core/scaffold.pl

+		$PG_ANSWERS_HASH->{$name}{ans_eval}{rh_ans}{scaffold_past_open} = 1 if ($section_no > $prior_last_open);
+	}
+
+	my $myLastSet;    # we save the name of the last answer field whose score was set


$myLastSet is a poor choice of variable name. my is a reserve word used to declare a variable, it should not be part of the name. Just use $lastSet. Although this is a confusing name. What was last set? Something more descriptive of what it means would be better.

drgrice1 · 2023-04-28T11:54:27Z

macros/parsers/parserMultiAnswer.pl

+	# Allow hiding the the structure of the answer for an
+	# answer in an unopened part of a scaffold problem.
+	my $hide_preview = 0;
+	$hide_preview = 1 if ($ans->{scaffold_past_open});


Why does this need to be here in this convoluted format?

drgrice1 · 2023-04-28T11:55:17Z

macros/parsers/parserMultiAnswer.pl

-				: join($self->{tex_separator}, @latex));
-		$ans->{preview_text_string} =
-			(defined($self->{format}) ? sprintf($self->{format}, @text) : join($self->{separator}, @text));
+		if ($hide_preview) {


Why can't this just be if ($ans->{scaffold_past_open}) {, with the part above deleted? (Although I don't think any of this is working anyway.)

taniwallach added needs testing Do Not Merge Yet labels Oct 29, 2020

taniwallach requested review from dpvc, Alex-Jordan and drgrice1 October 29, 2020 20:37

taniwallach mentioned this pull request Oct 29, 2020

scaffold.pl issues - next stage opened by preview, newly opened stage answers marked as incorrect when prior stage answers were submitted and correct #478

Open

drgrice1 requested changes Feb 26, 2021

View reviewed changes

macros/scaffold.pl Outdated Show resolved Hide resolved

macros/scaffold.pl Outdated Show resolved Hide resolved

macros/scaffold.pl Outdated Show resolved Hide resolved

macros/scaffold.pl Outdated Show resolved Hide resolved

macros/scaffold.pl Outdated Show resolved Hide resolved

taniwallach added the deferred label Mar 8, 2021

taniwallach requested review from drgrice1 and removed request for dpvc and Alex-Jordan March 8, 2021 17:20

taniwallach mentioned this pull request Apr 13, 2023

Revised handling of PERSISTENCE_HASH data #809

Merged

taniwallach force-pushed the scaffold-patches-preview-issues branch from 5f66a78 to fc7b992 Compare April 13, 2023 22:18

taniwallach changed the title ~~Patches to scaffold.pl so Preview does not modify which sections are open - work in progress~~ Patches to scaffold.pl so Preview does not modify which sections are open Apr 13, 2023

taniwallach requested a review from pstaabp April 13, 2023 22:26

taniwallach removed Do Not Merge Yet deferred labels Apr 13, 2023

taniwallach mentioned this pull request Apr 13, 2023

Revised handling of PERSISTENCE_HASH data openwebwork/webwork2#1940

Merged

taniwallach force-pushed the scaffold-patches-preview-issues branch from fc7b992 to 0fa323d Compare April 19, 2023 12:52

drgrice1 reviewed Apr 19, 2023

View reviewed changes

macros/core/scaffold.pl Outdated Show resolved Hide resolved

taniwallach force-pushed the scaffold-patches-preview-issues branch from 0fa323d to 00147ad Compare April 19, 2023 13:28

taniwallach added the Do Not Merge Yet label Apr 19, 2023

taniwallach force-pushed the scaffold-patches-preview-issues branch from 00147ad to a73c76b Compare April 20, 2023 09:54

taniwallach removed the Do Not Merge Yet label Apr 20, 2023

taniwallach force-pushed the scaffold-patches-preview-issues branch from a73c76b to 106b3a2 Compare April 20, 2023 13:59

taniwallach force-pushed the scaffold-patches-preview-issues branch 2 times, most recently from 358de88 to e3dc0ab Compare April 20, 2023 16:28

taniwallach added 2 commits April 22, 2023 22:27

Allow hiding the the structure of a MultiAnswer answer during

d7778a2

preview by setting boolean scaffold_past_open in the rh_ans hash of all answers in sections which were past the last open one.

taniwallach force-pushed the scaffold-patches-preview-issues branch from e3dc0ab to d7778a2 Compare April 22, 2023 19:28

pstaabp approved these changes Apr 25, 2023

View reviewed changes

drgrice1 reviewed Apr 28, 2023

View reviewed changes

taniwallach added the deferred label Jul 20, 2023

drgrice1 changed the base branch from develop to main August 5, 2023 19:56

drgrice1 changed the base branch from main to develop August 5, 2023 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Patches to scaffold.pl so Preview does not modify which sections are open #506

Patches to scaffold.pl so Preview does not modify which sections are open #506

taniwallach commented Oct 29, 2020 •

edited

Loading

taniwallach commented Oct 29, 2020

taniwallach commented Oct 30, 2020 •

edited

Loading

drgrice1 left a comment

taniwallach commented Mar 8, 2021

pstaabp commented Apr 11, 2023

taniwallach commented Apr 19, 2023

taniwallach commented Apr 20, 2023

pstaabp commented Apr 21, 2023

taniwallach commented Apr 22, 2023

pstaabp commented Apr 25, 2023

taniwallach commented Apr 25, 2023

taniwallach commented Apr 25, 2023

drgrice1 left a comment

drgrice1 Apr 28, 2023

drgrice1 Apr 28, 2023

drgrice1 Apr 28, 2023

drgrice1 Apr 28, 2023

drgrice1 Apr 28, 2023

		occur due to a Preview, while when set to 1 sections will not be closed by
		Preview.

Patches to scaffold.pl so Preview does not modify which sections are open #506

Are you sure you want to change the base?

Patches to scaffold.pl so Preview does not modify which sections are open #506

Conversation

taniwallach commented Oct 29, 2020 • edited Loading

taniwallach commented Oct 29, 2020

taniwallach commented Oct 30, 2020 • edited Loading

drgrice1 left a comment

Choose a reason for hiding this comment

taniwallach commented Mar 8, 2021

pstaabp commented Apr 11, 2023

taniwallach commented Apr 19, 2023

taniwallach commented Apr 20, 2023

pstaabp commented Apr 21, 2023

taniwallach commented Apr 22, 2023

pstaabp commented Apr 25, 2023

taniwallach commented Apr 25, 2023

taniwallach commented Apr 25, 2023

drgrice1 left a comment

Choose a reason for hiding this comment

drgrice1 Apr 28, 2023

Choose a reason for hiding this comment

drgrice1 Apr 28, 2023

Choose a reason for hiding this comment

drgrice1 Apr 28, 2023

Choose a reason for hiding this comment

drgrice1 Apr 28, 2023

Choose a reason for hiding this comment

drgrice1 Apr 28, 2023

Choose a reason for hiding this comment

taniwallach commented Oct 29, 2020 •

edited

Loading

taniwallach commented Oct 30, 2020 •

edited

Loading