Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Password-protect assessment instance creation #5367

Merged
merged 7 commits into from
Feb 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 41 additions & 21 deletions middlewares/studentAssessmentAccess.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,27 +51,13 @@ router.all('/', function (req, res, next) {
}
}

// Password protect the assessment
if (
'authz_result' in res.locals &&
'password' in res.locals.authz_result &&
res.locals.authz_result.password &&
res.locals?.assessment_instance?.open
) {
// No password yet case
if (req.cookies.pl_assessmentpw == null) {
return badPassword(res, req);
}

// Invalid or expired password case
var pwData = csrf.getCheckedData(req.cookies.pl_assessmentpw, config.secretKey, {
maxAge: timeout * 60 * 60 * 1000,
});
if (pwData == null || pwData.password !== res.locals.authz_result.password) {
return badPassword(res, req);
}

// Successful password case: falls though
// Password protect the assessment. Note that this only handles the general
// case of an existing assessment instance. This middleware can't handle
// the intricacies of creating a new assessment instance. We handle those
// cases on the `studentAssessmentExam` and `studentAssessmentHomework`
// pages.
if (res.locals?.assessment_instance?.open && !module.exports.checkPasswordOrRedirect(req, res)) {
return;
}

// Pass-through for everything else
Expand All @@ -80,6 +66,40 @@ router.all('/', function (req, res, next) {

module.exports = router;

/**
* Checks if the given request has the correct password. If not, redirects to
* a password input page.
*
* Returns `true` if the password is correct, `false` otherwise. If this
* function returns `false`, the caller should not continue with the request.
*
* @returns {boolean}
*/
module.exports.checkPasswordOrRedirect = function (req, res) {
if (!res.locals.authz_result?.password) {
// No password is required.
return true;
}

if (req.cookies.pl_assessmentpw == null) {
// The user has not entered a password yet.
badPassword(res, req);
return false;
}

var pwData = csrf.getCheckedData(req.cookies.pl_assessmentpw, config.secretKey, {
maxAge: timeout * 60 * 60 * 1000,
});
if (pwData == null || pwData.password !== res.locals.authz_result.password) {
// The password is incorrect or the cookie is expired.
badPassword(res, req);
return false;
}

// The password is correct and not expired!
return true;
};

function badPassword(res, req) {
logger.verbose(`invalid password attempt for ${res.locals.user.uid}`);
res.cookie('pl_pw_origUrl', req.originalUrl);
Expand Down
21 changes: 21 additions & 0 deletions pages/studentAssessmentExam/studentAssessmentExam.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ var ERR = require('async-stacktrace');
var express = require('express');
var router = express.Router();

const { checkPasswordOrRedirect } = require('../../middlewares/studentAssessmentAccess');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not in love with requiring middleware from page routes, but I don’t strongly object either. We could shift this function to lib/, but I’m not convinced it would be neater. I wanted to at least raise the question to see whether you had any other ideas!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't love this either, but if we move this to lib/, I think we'd have to move much of studentAssessmentAccess too (along with the .ejs file), which basically feels like we're still importing middleware, just without /middleware/ in the path 😅 I'm inclined to leave this as-is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Let's stick with things as you have them.

var error = require('../../prairielib/lib/error');
var assessment = require('../../lib/assessment');
var sqldb = require('../../prairielib/lib/sql-db');
Expand All @@ -12,6 +13,14 @@ var sql = sqlLoader.loadSqlEquiv(__filename);
router.get('/', function (req, res, next) {
if (res.locals.assessment.type !== 'Exam') return next();
if (res.locals.assessment.multiple_instance) {
// The user has landed on this page to create a new assessment instance.
//
// Before allowing the user to create a new assessment instance, we need
// to check if the current access rules require a password. If they do,
// we'll ensure that the password has already been entered before allowing
// students to create and start a new assessment instance.
if (!checkPasswordOrRedirect(req, res)) return;

res.render(__filename.replace(/\.js$/, '.ejs'), res.locals);
} else {
var params = {
Expand All @@ -21,6 +30,12 @@ router.get('/', function (req, res, next) {
sqldb.query(sql.select_single_assessment_instance, params, function (err, result) {
if (ERR(err, next)) return;
if (result.rowCount === 0) {
// Before allowing the user to create a new assessment instance, we need
// to check if the current access rules require a password. If they do,
// we'll ensure that the password has already been entered before allowing
// students to create and start a new assessment instance.
if (!checkPasswordOrRedirect(req, res)) return;

res.render(__filename.replace(/\.js$/, '.ejs'), res.locals);
} else {
res.redirect(res.locals.urlPrefix + '/assessment_instance/' + result.rows[0].id);
Expand All @@ -42,6 +57,12 @@ router.post('/', function (req, res, next) {
// permission required to create an assessment for the effective user.

if (req.body.__action === 'new_instance') {
// Before allowing the user to create a new assessment instance, we need
// to check if the current access rules require a password. If they do,
// we'll ensure that the password has already been entered before allowing
// students to create and start a new assessment instance.
if (!checkPasswordOrRedirect(req, res)) return;

assessment.makeAssessmentInstance(
res.locals.assessment.id,
res.locals.user.user_id,
Expand Down
16 changes: 15 additions & 1 deletion pages/studentAssessmentHomework/studentAssessmentHomework.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ var router = express.Router();
var path = require('path');
var debug = require('debug')('prairielearn:' + path.basename(__filename, '.js'));

const { checkPasswordOrRedirect } = require('../../middlewares/studentAssessmentAccess');
var error = require('../../prairielib/lib/error');
var assessment = require('../../lib/assessment');
var sqldb = require('../../prairielib/lib/sql-db');
Expand Down Expand Up @@ -43,7 +44,8 @@ router.get('/', function (req, res, next) {
// student data in the course instance (which has already been checked), exactly the
// permission required to create an assessment for the effective user.

//if it is a group_work with no instance, jump to a confirm page.
// If this assessment is group work and there is no existing instance,
// show the group info page.
if (res.locals.assessment.group_work) {
sqldb.query(sql.get_config_info, params, function (err, result) {
if (ERR(err, next)) return;
Expand All @@ -67,6 +69,12 @@ router.get('/', function (req, res, next) {
});
});
} else {
// Before allowing the user to create a new assessment instance, we need
// to check if the current access rules require a password. If they do,
// we'll ensure that the password has already been entered before allowing
// students to create and start a new assessment instance.
if (!checkPasswordOrRedirect(req, res)) return;

const time_limit_min = null;
assessment.makeAssessmentInstance(
res.locals.assessment.id,
Expand Down Expand Up @@ -100,6 +108,12 @@ router.post('/', function (req, res, next) {
sqldb.query(sql.find_single_assessment_instance, params, function (err, result) {
if (ERR(err, next)) return;
if (result.rowCount === 0) {
// Before allowing the user to create a new assessment instance, we need
// to check if the current access rules require a password. If they do,
// we'll ensure that the password has already been entered before allowing
// students to create and start a new assessment instance.
if (!checkPasswordOrRedirect(req, res)) return;

const time_limit_min = null;
assessment.makeAssessmentInstance(
res.locals.assessment.id,
Expand Down