Improve exam mode security #9743
Conversation
nwalters512
requested review from
mwest1066,
jonatanschroeder,
trombonekenny and
mylesw27
All images
|
| @@ -348,6 +350,9 @@ export async function authzCourseOrInstance(req, res) { | |||
| res.locals.is_administrator = res.locals.authz_data.is_administrator; | |||
|
|
|||
| res.locals.authz_data.mode = effectiveParams.req_mode; | |||
| res.locals.authz_data.mode_reason = req.cookies.pl_requested_mode | |||
| ? 'Requested' | |||
There was a problem hiding this comment.
I think I need to think a little more about what values we want to use when overrides are in place.
I think our handling of mode above on line 352 is actually a little weird. effectiveParams.req_mode takes into account req.cookies.pl_requested_mode, but in this branch, we specifically don't want to take that into account, does that sound right? Or... now that I think about this, I think we actually need this to take into account the cookie so that instructorEffectiveUser can display the effective mode. 🥴
| coalesce($req_mode, (access_mode.mode)) AS mode, | ||
| ( | ||
| CASE | ||
| WHEN $req_mode IS NOT NULL THEN 'Override' |
There was a problem hiding this comment.
🤷 we could also leave this as NULL? We really only care whether or not the value is PrairieTest; what the "or not" value is doesn't really matter.
| access_mode AS ( | ||
| SELECT | ||
| mode, | ||
| mode_reason | ||
| FROM | ||
| ip_to_mode ($ip, $req_date, $user_id) | ||
| ) |
There was a problem hiding this comment.
I could also just join on ip_to_mode?
| course_role: 'None', | ||
| course_instance_role: 'None', | ||
| user_id: '1000', | ||
| uid: 'valid@example.com', | ||
| date: '2010-07-07 06:06:06-00', | ||
| display_timezone: 'US/Central', | ||
| }); | ||
| assert.isTrue(authorized); | ||
| assert.isFalse(authorized); |
There was a problem hiding this comment.
This was a change in behavior. Is it desirable? I think so! My guess is that this test meant to test an Exam-mode assessment that wasn't linked to a PT exam at all. We could always add another test for that? Though I'm pretty sure this is already covered by the "without PrairieTest" block above.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9743 +/- ##
==========================================
- Coverage 66.43% 66.43% -0.01%
==========================================
Files 453 453
Lines 70289 70301 +12
Branches 5648 5649 +1
==========================================
+ Hits 46694 46701 +7
- Misses 23170 23178 +8
+ Partials 425 422 -3 ☔ View full report in Codecov by Sentry. |
This PR closes an unlikely and obscure access issue where we allowed a student with a checked-in PrairieTest reservation to access Exam-mode assessment without an
exam_uuid. This behavior existed to support the legacyexam_mode_networksfunctionality.Now, we propagate the reason we consider a user to be in Exam mode so that we can use that to determine if we should allow access to assessments with out
exam_uuid:exam_uuid.exam_uuid.As discussed on #1974.