fix: Improve API authorization (#5612)

Co-authored-by: Matthew West <matt@prairielearn.com>

api/v1/endpoints/courseInstanceAssessmentInstances/index.js

@@ -10,11 +10,11 @@ const sqlLoader = require('../../../../prairielib/lib/sql-loader');
 
 const sql = sqlLoader.load(path.join(__dirname, '..', 'queries.sql'));
 
-router.get('/:assessment_instance_id', (req, res, next) => {
+router.get('/:unsafe_assessment_instance_id', (req, res, next) => {
   const params = {
     course_instance_id: res.locals.course_instance.id,
-    assessment_id: null,
-    assessment_instance_id: req.params.assessment_instance_id,
+    unsafe_assessment_id: null,
+    unsafe_assessment_instance_id: req.params.unsafe_assessment_instance_id,
   };
   sqldb.queryOneRow(sql.select_assessment_instances, params, (err, result) => {
     if (ERR(err, next)) return;
@@ -29,36 +29,52 @@ router.get('/:assessment_instance_id', (req, res, next) => {
   });
 });
 
-router.get('/:assessment_instance_id/instance_questions', (req, res, next) => {
+router.get('/:unsafe_assessment_instance_id/instance_questions', (req, res, next) => {
   const params = {
     course_instance_id: res.locals.course_instance.id,
-    assessment_instance_id: req.params.assessment_instance_id,
-    instance_question_id: null,
+    unsafe_assessment_instance_id: req.params.unsafe_assessment_instance_id,
   };
   sqldb.queryOneRow(sql.select_instance_questions, params, (err, result) => {
     if (ERR(err, next)) return;
     res.status(200).send(result.rows[0].item);
   });
 });
 
-router.get('/:assessment_instance_id/submissions', (req, res, next) => {
+router.get('/:unsafe_assessment_instance_id/submissions', (req, res, next) => {
   const params = {
     course_instance_id: res.locals.course_instance.id,
-    assessment_instance_id: req.params.assessment_instance_id,
-    submission_id: null,
+    unsafe_assessment_instance_id: req.params.unsafe_assessment_instance_id,
+    unsafe_submission_id: null,
   };
   sqldb.queryOneRow(sql.select_submissions, params, (err, result) => {
     if (ERR(err, next)) return;
     res.status(200).send(result.rows[0].item);
   });
 });
 
-router.get('/:assessment_instance_id/log', (req, res, next) => {
-  const params = [req.params.assessment_instance_id, true];
-  sqldb.call('assessment_instances_select_log', params, (err, result) => {
-    if (ERR(err, next)) return;
-    res.status(200).send(result.rows);
-  });
+router.get('/:unsafe_assessment_instance_id/log', (req, res, next) => {
+  sqldb.queryZeroOrOneRow(
+    sql.select_assessment_instance,
+    {
+      course_instance_id: res.locals.course_instance.id,
+      unsafe_assessment_instance_id: req.params.unsafe_assessment_instance_id,
+    },
+    (err, result) => {
+      if (ERR(err, next)) return;
+      if (result.rowCount === 0) {
+        res.status(404).send({
+          message: 'Not Found',
+        });
+        return;
+      }
+
+      const params = [result.rows[0].assessment_instance_id, true];
+      sqldb.call('assessment_instances_select_log', params, (err, result) => {
+        if (ERR(err, next)) return;
+        res.status(200).send(result.rows);
+      });
+    }
+  );
 });
 
 module.exports = router;

api/v1/endpoints/courseInstanceAssessments/index.js

@@ -13,18 +13,18 @@ const sql = sqlLoader.load(path.join(__dirname, '..', 'queries.sql'));
 router.get('/', (req, res, next) => {
   const params = {
     course_instance_id: res.locals.course_instance.id,
-    assessment_id: null,
+    unsafe_assessment_id: null,
   };
   sqldb.queryOneRow(sql.select_assessments, params, (err, result) => {
     if (ERR(err, next)) return;
     res.status(200).send(result.rows[0].item);
   });
 });
 
-router.get('/:assessment_id', (req, res, next) => {
+router.get('/:unsafe_assessment_id', (req, res, next) => {
   const params = {
     course_instance_id: res.locals.course_instance.id,
-    assessment_id: req.params.assessment_id,
+    unsafe_assessment_id: req.params.unsafe_assessment_id,
   };
   sqldb.queryOneRow(sql.select_assessments, params, (err, result) => {
     if (ERR(err, next)) return;
@@ -39,22 +39,22 @@ router.get('/:assessment_id', (req, res, next) => {
   });
 });
 
-router.get('/:assessment_id/assessment_instances', (req, res, next) => {
+router.get('/:unsafe_assessment_id/assessment_instances', (req, res, next) => {
   const params = {
     course_instance_id: res.locals.course_instance.id,
-    assessment_id: req.params.assessment_id,
-    assessment_instance_id: null,
+    unsafe_assessment_id: req.params.unsafe_assessment_id,
+    unsafe_assessment_instance_id: null,
   };
   sqldb.queryOneRow(sql.select_assessment_instances, params, (err, result) => {
     if (ERR(err, next)) return;
     res.status(200).send(result.rows[0].item);
   });
 });
 
-router.get('/:assessment_id/assessment_access_rules', (req, res, next) => {
+router.get('/:unsafe_assessment_id/assessment_access_rules', (req, res, next) => {
   const params = {
     course_instance_id: res.locals.course_instance.id,
-    assessment_id: req.params.assessment_id,
+    unsafe_assessment_id: req.params.unsafe_assessment_id,
   };
   sqldb.queryOneRow(sql.select_assessment_access_rules, params, (err, result) => {
     if (ERR(err, next)) return;

api/v1/endpoints/courseInstanceSubmissions/index.js

@@ -10,11 +10,11 @@ const sqlLoader = require('../../../../prairielib/lib/sql-loader');
 
 const sql = sqlLoader.load(path.join(__dirname, '..', 'queries.sql'));
 
-router.get('/:submission_id', (req, res, next) => {
+router.get('/:unsafe_submission_id', (req, res, next) => {
   const params = {
     course_instance_id: res.locals.course_instance.id,
-    assessment_instance_id: null,
-    submission_id: req.params.submission_id,
+    unsafe_assessment_instance_id: null,
+    unsafe_submission_id: req.params.unsafe_submission_id,
   };
   sqldb.queryOneRow(sql.select_submissions, params, (err, result) => {
     if (ERR(err, next)) return;

api/v1/endpoints/queries.sql

@@ -21,7 +21,7 @@ WITH object_data AS (
     WHERE
         ci.id = $course_instance_id
         AND a.deleted_at IS NULL
-        AND ($assessment_id::bigint IS NULL OR a.id = $assessment_id)
+        AND ($unsafe_assessment_id::bigint IS NULL OR a.id = $unsafe_assessment_id)
 )
 SELECT
     coalesce(jsonb_agg(
@@ -72,8 +72,8 @@ WITH object_data AS (
         LEFT JOIN users AS u ON (u.user_id = ai.user_id)
     WHERE
         ci.id = $course_instance_id
-        AND ($assessment_id::bigint IS NULL OR a.id = $assessment_id)
-        AND ($assessment_instance_id::bigint IS NULL OR ai.id = $assessment_instance_id)
+        AND ($unsafe_assessment_id::bigint IS NULL OR a.id = $unsafe_assessment_id)
+        AND ($unsafe_assessment_instance_id::bigint IS NULL OR ai.id = $unsafe_assessment_instance_id)
 )
 SELECT
     coalesce(jsonb_agg(
@@ -112,7 +112,7 @@ WITH object_data AS (
         JOIN assessment_access_rules AS aar ON (aar.assessment_id = a.id)
     WHERE
         ci.id = $course_instance_id
-        AND a.id = $assessment_id
+        AND a.id = $unsafe_assessment_id
 )
 SELECT
     coalesce(jsonb_agg(
@@ -191,8 +191,11 @@ WITH object_data AS (
         JOIN instance_questions AS iq ON (iq.assessment_instance_id = ai.id)
         JOIN assessment_questions AS aq ON (aq.id = iq.assessment_question_id)
         JOIN questions AS q ON (q.id = aq.question_id)
+        JOIN assessments AS a ON (a.id = aq.assessment_id)
+        JOIN course_instances AS ci ON (ci.id = a.course_instance_id)
     WHERE
-        ai.id = $assessment_instance_id
+        ai.id = $unsafe_assessment_instance_id
+        AND ci.id = $course_instance_id
 )
 SELECT
     coalesce(jsonb_agg(
@@ -254,8 +257,8 @@ WITH object_data AS (
         JOIN submissions AS s ON (s.variant_id = v.id)
     WHERE
         ci.id = $course_instance_id
-        AND ($assessment_instance_id::bigint IS NULL OR ai.id = $assessment_instance_id)
-        AND ($submission_id::bigint IS NULL OR s.id = $submission_id)
+        AND ($unsafe_assessment_instance_id::bigint IS NULL OR ai.id = $unsafe_assessment_instance_id)
+        AND ($unsafe_submission_id::bigint IS NULL OR s.id = $unsafe_submission_id)
 )
 SELECT
     coalesce(jsonb_agg(
@@ -264,3 +267,13 @@ SELECT
     ), '[]'::jsonb) AS item
 FROM
     object_data;
+
+-- BLOCK select_assessment_instance
+SELECT ai.id AS assessment_instance_id
+FROM
+    assessment_instances AS ai
+    JOIN assessments AS a ON (a.id = ai.assessment_id)
+    JOIN course_instances AS ci ON (ci.id = a.course_instance_id)
+WHERE
+    ai.id = $unsafe_assessment_instance_id
+    AND ci.id = $course_instance_id;

api/v1/error.js

@@ -2,7 +2,7 @@ const logger = require('../../lib/logger');
 const status = require('http-status');
 
 module.exports = (err, req, res, _next) => {
-  logger.info('API Error', err);
+  logger.error('API Error', err);
   const statusCode = err.status || 500;
   res.status(statusCode).send({
     message: status[statusCode],

api/v1/index.js

@@ -1,12 +1,35 @@
 const express = require('express');
+
+const error = require('../../prairielib/error');
+
 const router = express.Router();
 
+/**
+ * Used to prevent access to student data if the user doesn't have student data
+ * viewing permissions. This should be added to any routes that provide student
+ * data.
+ */
+function authzHasCourseInstanceView(req, res, next) {
+  if (!res.locals.authz_data.has_course_instance_permission_view) {
+    return next(
+      error.make(403, 'Requires student data view access', {
+        locals: res.locals,
+      })
+    );
+  }
+  next();
+}
+
 // Pretty-print all JSON responses
 router.use(require('./prettyPrintJson'));
 
 // All course instance pages require authorization
 router.use('/course_instances/:course_instance_id', [
   require('../../middlewares/authzCourseOrInstance'),
+  // Asserts that the user has either course preview or course instance student
+  // data access. If a route provides access to student data, you should also
+  // include the `authzHasCourseInstanceView` middleware to ensure that access
+  // to student data is properly limited.
   require('../../middlewares/authzHasCoursePreviewOrInstanceView'),
   require('./endpoints/courseInstanceInfo'),
 ]);
@@ -18,14 +41,17 @@ router.use(
 );
 router.use(
   '/course_instances/:course_instance_id/assessment_instances',
+  authzHasCourseInstanceView,
   require('./endpoints/courseInstanceAssessmentInstances')
 );
 router.use(
   '/course_instances/:course_instance_id/submissions',
+  authzHasCourseInstanceView,
   require('./endpoints/courseInstanceSubmissions')
 );
 router.use(
   '/course_instances/:course_instance_id/gradebook',
+  authzHasCourseInstanceView,
   require('./endpoints/courseInstanceGradebook')
 );
 router.use(