refactor: update plugin to treat logged-in users who have not passed 2FA check as guests, return standard API error on API calls when 2FA not passed [breaking]

docs/changelog.md

@@ -0,0 +1,19 @@
+# Changelog
+
+This file provides a running tally of breaking changes.
+It is not meant to be exhaustive — if you notice a breaking change that is not included in this list, please [open an issue](https://github.com/julianlam/NodeBB-plugin-2factor/issues/new) and let me know.
+
+## v7.x
+
+* If an account is protected by Two-Factor Authentication, you will still be considered a guest when the page is loaded  — specifically, `app.user` is no longer showing logged-in user data.
+* Requests to the API (both the Read and Write API) now return a standard API-style (an object with `status` and `payload`) `401 Unauthorized` error if you have not passed the 2FA check.
+
+## v6.x
+
+* The plugin was updated to support a minimum NodeBB version of v3.x.
+    * The plugin should still work under v2.x, but the UI may not look as polished as the front-end elements have been changed to use Bootstrap 5 class names.
+
+## v5.x
+
+* Users are now presented with a list of choices based on configured 2FA methods, instead of a priority-based challenge
+* Users can now set up multiple 2FA methods (e.g. both TOTP and Hardware key at the same time.)

lib/controllers.js

@@ -30,10 +30,11 @@ async function getGroups(set) {
 }
 
 Controllers.renderChoices = async (req, res) => {
+	const uid = res.locals['2factor'] || req.uid;
 	const [hasAuthn, hasTotp, hasBackupCodes] = await Promise.all([
-		parent.hasAuthn(req.uid),
-		parent.hasTotp(req.uid),
-		parent.hasBackupCodes(req.uid),
+		parent.hasAuthn(uid),
+		parent.hasTotp(uid),
+		parent.hasBackupCodes(uid),
 	]);
 	const count = [hasAuthn, hasTotp, hasBackupCodes].reduce((count, cur) => count + (cur ? 1 : 0), 0);
 
@@ -65,13 +66,15 @@ Controllers.renderChoices = async (req, res) => {
 };
 
 Controllers.renderTotpChallenge = async (req, res, next) => {
+	const uid = res.locals['2factor'] || req.uid;
+
 	if (req.session.tfa === true && ((req.query.next && !req.query.next.startsWith('/admin')) || !req.session.tfaForce)) {
 		return res.redirect(nconf.get('relative_path') + (req.query.next || '/'));
 	}
 
 	const error = req.flash('error');
 
-	if (!await parent.hasTotp(req.uid)) {
+	if (!await parent.hasTotp(uid)) {
 		return next();
 	}
 
@@ -84,15 +87,17 @@ Controllers.renderTotpChallenge = async (req, res, next) => {
 };
 
 Controllers.renderAuthnChallenge = async (req, res, next) => {
+	const uid = res.locals['2factor'] || req.uid;
+
 	if (req.session.tfa === true && ((req.query.next && !req.query.next.startsWith('/admin')) || !req.session.tfaForce)) {
 		return res.redirect(nconf.get('relative_path') + (req.query.next || '/'));
 	}
 
-	if (!await parent.hasAuthn(req.uid)) {
+	if (!await parent.hasAuthn(uid)) {
 		return next();
 	}
 
-	const keyIds = await parent.getAuthnKeyIds(req.uid);
+	const keyIds = await parent.getAuthnKeyIds(uid);
 	let authnOptions;
 	if (keyIds.length) {
 		authnOptions = await parent._f2l.assertionOptions();
@@ -120,13 +125,15 @@ Controllers.processTotpLogin = function (req, res, next) {
 };
 
 Controllers.renderBackup = async (req, res, next) => {
+	const uid = res.locals['2factor'] || req.uid;
+
 	if (req.session.tfa === true && ((req.query.next && !req.query.next.startsWith('/admin')) || !req.session.tfaForce)) {
 		return res.redirect(nconf.get('relative_path') + (req.query.next || '/'));
 	}
 
 	const error = req.flash('error');
 
-	if (!await parent.hasKey(req.user.uid)) {
+	if (!await parent.hasKey(uid)) {
 		return next();
 	}
 

lib/middlewares.js

@@ -8,7 +8,7 @@ Middlewares.requireSecondFactor = function (req, res, next) {
 	if (req.session.hasOwnProperty('tfa') && req.session.tfa === true) {
 		next();
 	} else {
-		translator.translate('[[2factor:second-factor-required]]', function (translated) {
+		translator.translate('[[2factor:second-factor-required]]', (translated) => {
 			res.status(403).json({
 				error: translated,
 			});

library.js

@@ -310,6 +310,17 @@ plugin.disassociate = async (uid) => {
 	await db.delete(`2factor:webauthn:${uid}`);
 };
 
+plugin.overrideUid = async ({ req, locals }) => {
+	if (req.uid && await plugin.hasKey(req.uid) && req.session.tfa !== true) {
+		locals['2factor'] = req.uid;
+		req.uid = 0;
+		delete req.user;
+		delete req.loggedIn;
+	}
+
+	return { req, locals };
+};
+
 plugin.check = async ({ req, res }) => {
 	if (!req.user || req.session.tfa === true) {
 		return;
@@ -317,10 +328,10 @@ plugin.check = async ({ req, res }) => {
 
 	const requestPath = req.baseUrl + req.path;
 	const exemptPaths = ['/login/2fa', '/login/2fa/authn', '/login/2fa/totp', '/login/2fa/backup', '/2factor/authn/verify', '/register/complete'];
-	const exemptPrefixes = ['/api/v3/'];
 	if (
-		exemptPaths.some(path => requestPath === nconf.get('relative_path') + path || requestPath === `${nconf.get('relative_path')}/api${path}`) ||
-		exemptPrefixes.some(prefix => requestPath.startsWith(nconf.get('relative_path') + prefix))
+		exemptPaths.some(path => requestPath === nconf.get('relative_path') + path ||
+			requestPath === `${nconf.get('relative_path')}/api${path}` ||
+			requestPath === `${nconf.get('relative_path')}/api/v3/plugins${path}`)
 	) {
 		return;
 	}
@@ -333,8 +344,12 @@ plugin.check = async ({ req, res }) => {
 		.replace(nconf.get('relative_path'), '');
 
 	if (await plugin.hasKey(req.user.uid)) {
-		// Account has TFA, redirect to login
-		controllerHelpers.redirect(res, `/login/2fa?next=${redirect}`);
+		if (!res.locals.isAPI) {
+			// Account has TFA, redirect to login
+			controllerHelpers.redirect(res, `/login/2fa?next=${redirect}`);
+		} else {
+			await controllerHelpers.formatApiResponse(401, res, new Error('[[2factor:second-factor-required]]'));
+		}
 	} else if (tfaEnforcedGroups.length && (await groups.isMemberOfGroups(req.uid, tfaEnforcedGroups)).includes(true)) {
 		if (req.url.startsWith('/admin') || (!req.url.startsWith('/admin') && !req.url.match('2factor'))) {
 			controllerHelpers.redirect(res, `/me/2factor?next=${redirect}`);

plugin.json

@@ -8,6 +8,7 @@
 		{ "hook": "filter:admin.header.build", "method": "addAdminNavigation" },
 		{ "hook": "filter:user.profileMenu", "method": "addProfileItem" },
 		{ "hook": "response:router.page", "method": "check" },
+		{ "hook": "filter:middleware.buildHeader", "method": "overrideUid" },
 		{ "hook": "static:user.loggedOut", "method": "clearSession" },
 		{ "hook": "filter:middleware.render", "method": "updateTitle" },
 		{ "hook": "static:sockets.validateSession", "method": "checkSocket" },