Improve exam mode security #9743

nwalters512 · 2024-04-18T22:31:20Z

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 legacy exam_mode_networks functionality.

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:

If we're in Exam mode because of PrairieTest, block access to assessments without exam_uuid.
If we're in Exam mode for any other reason, fall back to normal access checks that don't consider exam_uuid.

As discussed on #1974.

github-actions · 2024-04-18T22:41:57Z

All images

Image	Platform	Old Size	New Size	Change
prairielearn/executor:41632fc2bf3b842c4e865a68726646723f9f7cc9	null	1187.75 MB	1187.75 MB	-0.00%
prairielearn/prairielearn:41632fc2bf3b842c4e865a68726646723f9f7cc9	linux/amd64	1187.74 MB	1187.75 MB	0.00%

nwalters512 · 2024-04-18T23:06:23Z

apps/prairielearn/src/middlewares/authzCourseOrInstance.js

@@ -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'


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. 🥴

I guess I'm a little unclear about this myself. Maybe we do want to allow mode overrides to set mode_reason = PrairieTest so people can try out exam access themselves?

nwalters512 · 2024-04-18T23:07:19Z

apps/prairielearn/src/middlewares/authzCourseOrInstance.sql

+  coalesce($req_mode, (access_mode.mode)) AS mode,
+  (
+    CASE
+      WHEN $req_mode IS NOT NULL THEN 'Override'


🤷 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.

As I commented elsewhere, I think it would be cleaner to have mode_reason be an enum with a carefully thought-through list of possible values.

nwalters512 · 2024-04-18T23:07:56Z

apps/prairielearn/src/middlewares/authzCourseOrInstance.sql

+  access_mode AS (
+    SELECT
+      mode,
+      mode_reason
+    FROM
+      ip_to_mode ($ip, $req_date, $user_id)
+  )


I could also just join on ip_to_mode?

I don't think it really matters either way. We normally JOIN, but this is also fine if you think it's clearer.

apps/prairielearn/src/tests/sprocs/check_assessment_access.test.ts

codecov · 2024-05-17T20:32:54Z

Codecov Report

Attention: Patch coverage is 64.70588% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 67.15%. Comparing base (dafe0ff) to head (41632fc).

Files	Patch %	Lines
apps/prairielearn/src/ee/lib/terms.ts	16.66%	5 Missing ⚠️
...irielearn/src/middlewares/authzCourseOrInstance.js	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9743      +/-   ##
==========================================
- Coverage   67.27%   67.15%   -0.13%     
==========================================
  Files         458      458              
  Lines       71648    71660      +12     
  Branches     5775     5762      -13     
==========================================
- Hits        48203    48124      -79     
- Misses      23007    23100      +93     
+ Partials      438      436       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mwest1066 · 2024-05-27T19:01:57Z

apps/prairielearn/src/sprocs/ip_to_mode.sql

@@ -3,21 +3,24 @@ CREATE FUNCTION
        IN ip inet,
        IN date timestamptz,
        IN authn_user_id bigint,
-        OUT mode enum_mode
+        OUT mode enum_mode,
+        OUT mode_reason text


Use an enum for this?

I guess it feels a bit weird to me to create an enum for something that isn't actually stored in the database. Are you thinking this enum would just help with type safety as this value flows up/down through various sprocs and queries?

Yep, exactly. Definitely not essential, but given the importance of this code I feel like strong typing can only help us to get things correct.

mwest1066 · 2024-05-27T19:04:43Z

apps/prairielearn/src/sprocs/ip_to_mode.sql

    )
 AS $$
 DECLARE
    reservation RECORD;
 BEGIN
+    -- Default to 'Public' mode.
+    mode := 'Public';


Do we want to set mode_reason to a value here? Maybe Default?

apps/prairielearn/src/tests/sprocs/check_assessment_access.test.ts

mwest1066 · 2024-05-27T20:24:34Z

apps/prairielearn/src/sprocs/check_assessment_access_rule.sql

@@ -41,6 +42,17 @@ BEGIN

    << prairietest_access >>


I believe we could replace the <<prairietest_access>> block with the following, to obtain identical results with hopefully clearer control flow?

IF (exam_uuid IS NOT NULL AND mode != 'Exam') THEN -- only allowed to use exam_uuid in mode Exam authorized := FALSE; END IF; IF (exam_uuid IS NOT NULL AND mode_reason != 'PrairieTest') THEN -- only allowed to use exam_uuid with PT authorized := FALSE; END IF; IF (mode_reason = 'PrairieTest') THEN -- we must have `mode = 'Exam'` here because this is guaranteed by `mode_reason = 'PrairieTest'` -- is there a checked-in pt_reservation for the (possibly-NULL) exam_uuid? -- SAME QUERY AS WE ALREADY HAVE FOR THIS -- if exam_uuid IS NULL then we definitely won't find a reservation IF NOT FOUND THEN authorized := FALSE; END IF; END IF;

Perhaps this is solely based on vibes and the query planner will actually do the right thing, but it feels like we could save some work by skipping the reservation query entirely if there's no exam_uuid? Or maybe the query planner will see that we're trying to find a NULL value on a column with a NON NULL constraint and just immediately return zero rows? 🤔

Regardless, I like the idea of getting rid of << prairietest_access >> and EXIT prairietest_access, which we can still do whether or not we decide we can skip the exam_uuid IS NOT NULL check.

I'm fine to have an explicit check for exam_uuid IS NOT NULL, especially considering that it will serve as documentation for what we are doing.

I was ultimately able to get the tests to pass with this:

IF (assessment_access_rule.exam_uuid IS NULL AND mode = 'Exam') THEN -- Public mode is required for this access rule. authorized := FALSE; END IF; IF (assessment_access_rule.exam_uuid IS NOT NULL AND mode IS DISTINCT FROM 'Exam') THEN -- Exam mode is required for this access rule. authorized := FALSE; END IF; -- IF (assessment_access_rule.exam_uuid IS NOT NULL AND mode_reason != 'PrairieTest') THEN -- -- PrairieTest is required for this access rule. -- authorized := FALSE; -- END IF; IF (assessment_access_rule.exam_uuid IS NOT NULL AND mode = 'Exam' AND mode_reason = 'PrairieTest') THEN -- Look for a checked-in PrairieTest reservation. SELECT r.access_end INTO exam_access_end FROM pt_reservations AS r JOIN pt_enrollments AS e ON (e.id = r.enrollment_id) JOIN pt_exams AS x ON (x.id = r.exam_id) WHERE (date BETWEEN r.access_start AND r.access_end) AND e.user_id = check_assessment_access_rule.user_id AND x.uuid = assessment_access_rule.exam_uuid; IF NOT FOUND THEN -- No reservation found; block access. authorized := FALSE; END IF; END IF;

Note how one of the checks is commented out, and I added another check at the beginning. The commented-out check had no bearing on the tests passing. So, either it's truly unnecessary, or we're missing a test.

Or, looking at your other comments, we're passing authz_mode_reason: 'PrairieTest' where we shouldn't be.

I ultimately added another test and tweaked the behavior to get everything passing in 41632fc. Take a look, I'm pretty happy with it!

I like your new version of the code. Much cleaner!

I was getting too confused trying to think through all the cases so I ended up writing out all the legal variants of variables in the big table below.

Proposed code changes

If we care about the cases identified in the comments column as possibly wanting a change then I think we could fix them by adding:

IF (assessment_access_rule.exam_uuid IS NOT NULL AND mode_reason IS DISTINCT FROM 'PrairieTest') THEN -- only use exam_uuid when we are using PrairieTest authorized := FALSE; END IF; IF (assessment_access_rule.exam_uuid IS NOT NULL AND assessment_access_rule.mode IS DISTINCT FROM 'Exam') THEN -- only use exam_uuid if we have an explicit mode=Exam set in the access rule authorized := FALSE; END IF;

Definitions of columns and values

The first columns are the values in the assessment_access_rule and arguments, while the later columns are:

reservation: Does the SELECT for a pt_reservation find one?

authz: This is what I think the << prairietest_access >> block should be returning.

old authz: This is what the old code block used to return.

new authz: This is what the current new code (as of 41632fc) returns.

For the authz values I listed FALSE if the << prairietest_access >> block set authorized := FALSE, and TRUE otherwise. A value of TRUE here really just means that authorized is not modified by the << prairietest_access >> block, not that it is explicitly set to TRUE.

Note the comments column at the end.

The big table of variables

aar.mode | aar.exam_uuid | mode | mode_reason | reservation | authz | old authz | new authz | comment ---------|---------------|--------|-------------|-------------|-------|-----------|-----------|-------- Exam | NULL | Exam | NULL | FOUND | --- | --- | --- | Exam | NULL | Exam | NULL | NOT FOUND | TRUE | TRUE | TRUE | Exam | NULL | Exam | PrairieTest | FOUND | --- | --- | --- | Exam | NULL | Exam | PrairieTest | NOT FOUND | FALSE | TRUE | FALSE | Intended change for this PR Exam | NULL | Exam | Other | FOUND | --- | --- | --- | Exam | NULL | Exam | Other | NOT FOUND | TRUE | TRUE | TRUE | Exam | not-NULL | Exam | NULL | FOUND | FALSE | TRUE | TRUE | Do we want a change here? Exam | not-NULL | Exam | NULL | NOT FOUND | FALSE | FALSE | TRUE | Unintended change? Exam | not-NULL | Exam | PrairieTest | FOUND | TRUE | TRUE | TRUE | Exam | not-NULL | Exam | PrairieTest | NOT FOUND | FALSE | FALSE | FALSE | Exam | not-NULL | Exam | Other | FOUND | FALSE | TRUE | TRUE | Do we want a change here? Exam | not-NULL | Exam | Other | NOT FOUND | FALSE | FALSE | TRUE | Unintended change? Public | NULL | Public | NULL | FOUND | --- | --- | --- | Public | NULL | Public | NULL | NOT FOUND | TRUE | TRUE | TRUE | Public | NULL | Public | PrairieTest | FOUND | --- | --- | --- | Public | NULL | Public | PrairieTest | NOT FOUND | --- | --- | --- | Public | NULL | Public | Other | FOUND | --- | --- | --- | Public | NULL | Public | Other | NOT FOUND | TRUE | TRUE | TRUE | Public | not-NULL | Public | NULL | FOUND | FALSE | FALSE | FALSE | Public | not-NULL | Public | NULL | NOT FOUND | FALSE | FALSE | FALSE | Public | not-NULL | Public | PrairieTest | FOUND | FALSE | FALSE | FALSE | Public | not-NULL | Public | PrairieTest | NOT FOUND | FALSE | FALSE | FALSE | Public | not-NULL | Public | Other | FOUND | FALSE | FALSE | FALSE | Public | not-NULL | Public | Other | NOT FOUND | FALSE | FALSE | FALSE | NULL | NULL | Exam | NULL | FOUND | --- | --- | --- | NULL | NULL | Exam | NULL | NOT FOUND | TRUE | TRUE | TRUE | NULL | NULL | Exam | PrairieTest | FOUND | --- | --- | --- | NULL | NULL | Exam | PrairieTest | NOT FOUND | FALSE | TRUE | FALSE | Intended change for this PR NULL | NULL | Exam | Other | FOUND | --- | --- | --- | NULL | NULL | Exam | Other | NOT FOUND | TRUE | TRUE | TRUE | NULL | not-NULL | Exam | NULL | FOUND | FALSE | TRUE | TRUE | Do we want a change here? NULL | not-NULL | Exam | NULL | NOT FOUND | FALSE | FALSE | TRUE | Unintended change? NULL | not-NULL | Exam | PrairieTest | FOUND | FALSE | TRUE | TRUE | Do we want a change here? NULL | not-NULL | Exam | PrairieTest | NOT FOUND | FALSE | FALSE | FALSE | NULL | not-NULL | Exam | Other | FOUND | FALSE | TRUE | TRUE | Do we want a change here? NULL | not-NULL | Exam | Other | NOT FOUND | FALSE | FALSE | TRUE | Unintended change? NULL | NULL | Public | NULL | FOUND | --- | --- | --- | NULL | NULL | Public | NULL | NOT FOUND | TRUE | TRUE | TRUE | NULL | NULL | Public | PrairieTest | FOUND | --- | --- | --- | NULL | NULL | Public | PrairieTest | NOT FOUND | --- | --- | --- | NULL | NULL | Public | Other | FOUND | --- | --- | --- | NULL | NULL | Public | Other | NOT FOUND | TRUE | TRUE | TRUE | NULL | not-NULL | Public | NULL | FOUND | FALSE | FALSE | FALSE | NULL | not-NULL | Public | NULL | NOT FOUND | FALSE | FALSE | FALSE | NULL | not-NULL | Public | PrairieTest | FOUND | FALSE | FALSE | FALSE | NULL | not-NULL | Public | PrairieTest | NOT FOUND | FALSE | FALSE | FALSE | NULL | not-NULL | Public | Other | FOUND | FALSE | FALSE | FALSE | NULL | not-NULL | Public | Other | NOT FOUND | FALSE | FALSE | FALSE |

mwest1066 · 2024-05-29T19:56:03Z

apps/prairielearn/src/sprocs/check_assessment_access_rule.sql

-                AND e.user_id = check_assessment_access_rule.user_id
-                AND x.uuid = assessment_access_rule.exam_uuid;
+    IF (assessment_access_rule.exam_uuid IS NOT NULL AND mode IS DISTINCT FROM 'Exam') THEN
+        -- Exam mode is required for this access rule.


Do we want to add a sync warning if an access rule has an exam_uuid without mode=Exam? Maybe only for accessible course instances (like our existing exam_uuid validity checks)?

nwalters512 added 4 commits April 18, 2024 14:51

WIP improvements to Exam mode security

6faa934

Merge branch 'master' into improve-exam-mode-security

d69b916

Get tests passing

6941ed1

Test mode_reason in ip_to_mode tests

3b013e4

nwalters512 requested review from mwest1066, jonatanschroeder, trombonekenny and mylesw27 April 18, 2024 22:31

nwalters512 self-assigned this Apr 18, 2024

nwalters512 commented Apr 18, 2024

View reviewed changes

nwalters512 added 2 commits May 17, 2024 13:19

Merge branch 'master' into improve-exam-mode-security

2f8f157

Reformat

b938d69

Merge branch 'master' into improve-exam-mode-security

4317942

nwalters512 mentioned this pull request May 18, 2024

Checks are failing even thought they are informational codecov/feedback#386

Open

mwest1066 approved these changes May 27, 2024

View reviewed changes

nwalters512 added 3 commits May 28, 2024 16:39

Merge branch 'master' into improve-exam-mode-security

5ea264e

Merge branch 'master' into improve-exam-mode-security

9f08433

Add missing test; simplify conditionals in sproc

41632fc

mwest1066 reviewed May 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve exam mode security #9743

Improve exam mode security #9743

nwalters512 commented Apr 18, 2024

github-actions bot commented Apr 18, 2024 •

edited

nwalters512 Apr 18, 2024

mwest1066 May 27, 2024

nwalters512 Apr 18, 2024

mwest1066 May 27, 2024

nwalters512 Apr 18, 2024

mwest1066 May 27, 2024

codecov bot commented May 17, 2024 •

edited

mwest1066 May 27, 2024

nwalters512 May 28, 2024

mwest1066 May 28, 2024

mwest1066 May 27, 2024

mwest1066 May 27, 2024

nwalters512 May 28, 2024

mwest1066 May 29, 2024

nwalters512 May 29, 2024

nwalters512 May 29, 2024

mwest1066 May 29, 2024

mwest1066 May 29, 2024

Improve exam mode security #9743

Are you sure you want to change the base?

Improve exam mode security #9743

Conversation

nwalters512 commented Apr 18, 2024

github-actions bot commented Apr 18, 2024 • edited

All images

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 17, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Proposed code changes

Definitions of columns and values

The big table of variables

Choose a reason for hiding this comment

github-actions bot commented Apr 18, 2024 •

edited

codecov bot commented May 17, 2024 •

edited