[Bugfix:Submission] release_hidden_gradeables fix#12422
Conversation
Student users are now able to be served hidden tests details if professor allows
John-Roy123
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12422 +/- ##
============================================
- Coverage 21.64% 21.64% -0.01%
- Complexity 9818 9823 +5
============================================
Files 268 268
Lines 36777 36781 +4
Branches 490 490
============================================
Hits 7960 7960
- Misses 28331 28335 +4
Partials 486 486
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
John-Roy123
changed the title
John-Roy123
changed the title
|
I was not able to reproduce your changes on this branch. Could you update your testing steps and maybe include screenshots to show your changes? |
I updated my PR to include screenshots and clearer testing steps - keep in mind not all hidden gradeables will have the option to show details. If you go into sample/Autograder Hidden and Extra Credit (C++ Hidden Tests), there is a hidden test with the "show details" option. The changes should work on that. |
Rkoester47
left a comment
Rkoester47
left a comment
There was a problem hiding this comment.
I was able to test the functionality of the PR and can confirm that the hidden details does show the hidden test case for me on the gradeable described. I was able to open the drop down and see the diff checker for that particular hidden test. The code implementation seems to have succeeded in accomplishing what is described in the PR with minimal changes.
I have questions about the changes made to the gradeable_results.spec.js cypress test. Did you change the gradeable that this test usually accesses so that it will check your specific PR's test case? With your changes to this cypress test, some of the other portions of the test now fail where they would have succeeded in the past. Would it be better to create your own test that navigates to this gradeable and checks to see if test case 4 is expandable?
Changes user from student (version conflict) to bitdiddle (valid version) for test
automateprojectmangement
Bot
moved this from Seeking Reviewer
to In Review
in Submitty Development
|
The Cypress test for this PR is taking longer than expected, but the intended functionality of this PR is correct. The Cypress test will be made into its own PR so that this change can be moved forward. |
Rkoester47
left a comment
Rkoester47
left a comment
There was a problem hiding this comment.
I was able to reproduce the steps described in this PR and see the hidden details. Once the gradeable was closed and released, the hidden details was viewable by the student, as intended.
github-project-automation
Bot
moved this from In Review
to Awaiting Maintainer Review
in Submitty Development
automateprojectmangement
Bot
moved this from Awaiting Maintainer Review
to In Review
in Submitty Development
cjreed121
left a comment
cjreed121
left a comment
There was a problem hiding this comment.
This unfortunately allows for students to access the results of a hidden test case (without release_hidden_details being set to true). If you look at the network request being made for expanding the test cases, you will see it contains an index query parameter. Specifically look at frame size 13 which will have an index of 4, if you try an index of 5, you can now see the results of it by manually querying. Before this PR, this would not work. The permissions should not allow for this. Adjustments will need to be made to ensure this is not allowed. Perhaps the permissions should be split up.
github-project-automation
Bot
moved this from In Review
to Work in Progress
in Submitty Development
John-Roy123
moved this from Work in Progress
to Seeking Reviewer
in Submitty Development
John-Roy123
moved this from Seeking Reviewer
to Work in Progress
in Submitty Development
Updated ElectronicGraderControllers so that student users can no longer make ajax requests to prematurely view releasable hidden tests
John-Roy123
moved this from Work in Progress
to Awaiting Maintainer Review
in Submitty Development
automateprojectmangement
Bot
moved this from Awaiting Maintainer Review
to In Review
in Submitty Development
|
Hi @John-Roy123, the commits you have made did not address the security issues mentioned by cjreed. For example, I was able to perform the following fetch request in the DevTools console: |
…est case before granting view access to student
dagemcn
left a comment
dagemcn
left a comment
There was a problem hiding this comment.
Functionality here looks good, the error where hidden test cases would be accessible through index manipulation is fixed. Code looks good although I can't really speak to the added cypress test.
github-project-automation
Bot
moved this from Seeking Reviewer
to Done
in Submitty Development
8 participants
Why is this Change Important & Necessary?
Fixes #12411
What is the New Behavior?
Student users are now able to view the details of a hidden test in a gradeable if instructors add the "release_hidden_gradeables" flag
Before:
After:
What steps should a reviewer take to reproduce or test the bug or new feature?
Testing Security Error Fix:
Automated Testing & Documentation
Added e2e test to show hidden results do not show before submission is due, and then are able to show after due date.
Other information
This change affects the permissions a student has when viewing a hidden gradeable. Someone more familiar with the permissions structure should double-check to make sure the permission scope is as narrow as necessary.