From cfc632ba1a7c9b13b1e022979063a3e367e132ec Mon Sep 17 00:00:00 2001 From: Alice Gaudon Date: Thu, 22 Apr 2021 15:38:24 +0200 Subject: [PATCH] Approval mode: revoke unapproved users auth proofs Also add tests for auth approval mode --- config/test.json5 | 28 ++-- src/TestApp.ts | 4 +- src/auth/AuthGuard.ts | 7 +- src/auth/magic_link/MagicLinkController.ts | 14 +- src/auth/models/User.ts | 3 + src/auth/password/PasswordAuthProof.ts | 20 ++- test/Authentication.test.ts | 47 +++--- test/AuthenticationApprovalMode.test.ts | 160 +++++++++++++++++++++ test/AuthenticationNoUsername.test.ts | 44 ++---- test/_app.ts | 11 +- test/_authentication_common.ts | 60 ++++++++ 11 files changed, 296 insertions(+), 102 deletions(-) create mode 100644 test/AuthenticationApprovalMode.test.ts diff --git a/config/test.json5 b/config/test.json5 index d83228f..48bbfc3 100644 --- a/config/test.json5 +++ b/config/test.json5 @@ -1,14 +1,18 @@ { - mysql: { - host: "localhost", - user: "root", - password: "", - database: "swaf_test", - create_database_automatically: true, - }, - session: { - cookie: { - maxAge: 1000, // 1s - }, - }, + mysql: { + host: "localhost", + user: "root", + password: "", + database: "swaf_test", + create_database_automatically: true, + }, + session: { + cookie: { + // 1s + maxAge: 1000, + }, + }, + auth: { + approval_mode: true, + }, } diff --git a/src/TestApp.ts b/src/TestApp.ts index 3aa39bd..4f90556 100644 --- a/src/TestApp.ts +++ b/src/TestApp.ts @@ -29,9 +29,10 @@ import Controller from "./Controller"; import AccountController from "./auth/AccountController"; import MakeMagicLinksSessionNotUniqueMigration from "./auth/magic_link/MakeMagicLinksSessionNotUniqueMigration"; import AddUsedToMagicLinksMigration from "./auth/magic_link/AddUsedToMagicLinksMigration"; -import packageJson = require('./package.json'); import PreviousUrlComponent from "./components/PreviousUrlComponent"; import AddNameChangedAtToUsersMigration from "./auth/migrations/AddNameChangedAtToUsersMigration"; +import BackendController from "./helpers/BackendController"; +import packageJson = require('./package.json'); export const MIGRATIONS = [ CreateMigrationsTable, @@ -105,6 +106,7 @@ export default class TestApp extends Application { this.use(new MailController()); this.use(new AuthController()); this.use(new AccountController()); + this.use(new BackendController()); this.use(new MagicLinkController(this.as>(MagicLinkWebSocketListener))); diff --git a/src/auth/AuthGuard.ts b/src/auth/AuthGuard.ts index cdce13b..e10429c 100644 --- a/src/auth/AuthGuard.ts +++ b/src/auth/AuthGuard.ts @@ -119,6 +119,11 @@ export default class AuthGuard { let user = await proof.getResource(); + // Revoke proof early if user is not approved + if (user && !user.isApproved() || !user && User.isApprovalMode()) { + await proof.revoke(); + } + // Register if user doesn't exist if (!user) { const callbacks: RegisterCallback[] = []; @@ -139,7 +144,7 @@ export default class AuthGuard { await callback(); } - if (!user.isApproved()) { + if (User.isApprovalMode()) { await new Mail(this.app.as(NunjucksComponent).getEnvironment(), PENDING_ACCOUNT_REVIEW_MAIL_TEMPLATE, { username: user.asOptional(UserNameComponent)?.getName() || (await user.mainEmail.get())?.getOrFail('email') || diff --git a/src/auth/magic_link/MagicLinkController.ts b/src/auth/magic_link/MagicLinkController.ts index 402bd3b..5b0d77c 100644 --- a/src/auth/magic_link/MagicLinkController.ts +++ b/src/auth/magic_link/MagicLinkController.ts @@ -87,18 +87,8 @@ export default class MagicLinkController extends Controll }); } catch (e) { if (e instanceof PendingApprovalAuthError) { - res.format({ - json: () => { - res.json({ - 'status': 'warning', - 'message': `Your account is pending review. You'll receive an email once you're approved.`, - }); - }, - html: () => { - req.flash('warning', `Your account is pending review. You'll receive an email once you're approved.`); - res.redirect('/'); - }, - }); + req.flash('warning', `Your account is pending review. You'll receive an email once you're approved.`); + res.redirect(Controller.route('auth')); return null; } else { throw e; diff --git a/src/auth/models/User.ts b/src/auth/models/User.ts index 09c22f7..bbb648a 100644 --- a/src/auth/models/User.ts +++ b/src/auth/models/User.ts @@ -8,6 +8,9 @@ import UserApprovedComponent from "./UserApprovedComponent"; import UserNameComponent from "./UserNameComponent"; export default class User extends Model { + /** + * If true, new users are unapproved by default. + */ public static isApprovalMode(): boolean { return config.get('auth.approval_mode') && MysqlConnectionManager.hasMigration(AddApprovedFieldToUsersTableMigration); diff --git a/src/auth/password/PasswordAuthProof.ts b/src/auth/password/PasswordAuthProof.ts index dca436f..5149c0a 100644 --- a/src/auth/password/PasswordAuthProof.ts +++ b/src/auth/password/PasswordAuthProof.ts @@ -26,6 +26,7 @@ export default class PasswordAuthProof implements AuthProof { private forRegistration: boolean = false; private userId: number | null; private userPassword: UserPasswordComponent | null = null; + private revoked: boolean = false; private constructor(session: Session & Partial) { this.session = session; @@ -54,7 +55,14 @@ export default class PasswordAuthProof implements AuthProof { } public async revoke(): Promise { + this.revoked = true; this.session.authPasswordProof = undefined; + await new Promise((resolve, reject) => { + this.session.save(err => { + if (err) reject(err); + else resolve(); + }); + }); } private async getUserPassword(): Promise { @@ -74,11 +82,13 @@ export default class PasswordAuthProof implements AuthProof { } private save() { - this.session.authPasswordProof = { - authorized: this.authorized, - forRegistration: this.forRegistration, - userId: this.userId, - }; + if (!this.revoked) { + this.session.authPasswordProof = { + authorized: this.authorized, + forRegistration: this.forRegistration, + userId: this.userId, + }; + } } } diff --git a/test/Authentication.test.ts b/test/Authentication.test.ts index 6233458..1a3f7dc 100644 --- a/test/Authentication.test.ts +++ b/test/Authentication.test.ts @@ -1,46 +1,23 @@ -import TestApp from "../src/TestApp"; import useApp from "./_app"; -import Controller from "../src/Controller"; import supertest from "supertest"; -import CsrfProtectionComponent from "../src/components/CsrfProtectionComponent"; import User from "../src/auth/models/User"; import UserNameComponent from "../src/auth/models/UserNameComponent"; import UserPasswordComponent from "../src/auth/password/UserPasswordComponent"; import {popEmail} from "./_mail_server"; -import AuthComponent from "../src/auth/AuthComponent"; -import {followMagicLinkFromMail, testLogout} from "./_authentication_common"; +import {authAppProvider, followMagicLinkFromMail, testLogout} from "./_authentication_common"; import UserEmail from "../src/auth/models/UserEmail"; import * as querystring from "querystring"; -let app: TestApp; -useApp(async (addr, port) => { - return app = new class extends TestApp { - protected async init(): Promise { - this.use(new class extends Controller { - public routes(): void { - this.get('/', (req, res) => { - res.render('home'); - }, 'home'); - this.get('/csrf', (req, res) => { - res.send(CsrfProtectionComponent.getCsrfToken(req.getSession())); - }, 'csrf'); - this.get('/is-auth', async (req, res) => { - const proofs = await this.getApp().as(AuthComponent).getAuthGuard().getProofs(req); - if (proofs.length > 0) res.sendStatus(200); - else res.sendStatus(401); - }, 'is-auth'); - } - }()); - - await super.init(); - } - }(addr, port, true); -}); +const app = useApp(authAppProvider()); let agent: supertest.SuperTest; beforeAll(() => { - agent = supertest(app.getExpressApp()); + agent = supertest(app().getExpressApp()); +}); + +test('Approval Mode', () => { + expect(User.isApprovalMode()).toStrictEqual(false); }); describe('Register with username and password (password)', () => { @@ -74,6 +51,11 @@ describe('Register with username and password (password)', () => { expect(user).toBeDefined(); expect(user?.as(UserNameComponent).getName()).toStrictEqual('entrapta'); await expect(user?.as(UserPasswordComponent).verifyPassword('darla_is_cute')).resolves.toStrictEqual(true); + + // Proof must not be revoked + await agent.get('/has-any-password-auth-proof') + .set('Cookie', cookies) + .expect(200); }); test('Can\'t register when logged in', async () => { @@ -172,6 +154,11 @@ describe('Register with email (magic_link)', () => { await followMagicLinkFromMail(agent, cookies); + // Proof must not be revoked + await agent.get('/has-any-magic-link') + .set('Cookie', cookies) + .expect(200); + await testLogout(agent, cookies, csrf); // Verify saved user diff --git a/test/AuthenticationApprovalMode.test.ts b/test/AuthenticationApprovalMode.test.ts new file mode 100644 index 0000000..7440f11 --- /dev/null +++ b/test/AuthenticationApprovalMode.test.ts @@ -0,0 +1,160 @@ +import useApp from "./_app"; +import supertest from "supertest"; +import {authAppProvider, followMagicLinkFromMail} from "./_authentication_common"; +import User from "../src/auth/models/User"; +import querystring from "querystring"; +import UserApprovedComponent from "../src/auth/models/UserApprovedComponent"; +import {popEmail} from "./_mail_server"; + +const app = useApp(authAppProvider(true, true)); + +let agent: supertest.SuperTest; + +beforeAll(() => { + agent = supertest(app().getExpressApp()); +}); + +test('Approval Mode', () => { + expect(User.isApprovalMode()).toStrictEqual(true); +}); + +describe('Register with username and password (password)', () => { + let cookies: string[]; + let csrf: string; + + test('General case', async () => { + const res = await agent.get('/csrf').expect(200); + cookies = res.get('Set-Cookie'); + csrf = res.text; + + // Register user + await agent.post('/auth/register') + .set('Cookie', cookies) + .send({ + csrf: csrf, + auth_method: 'password', + identifier: 'entrapta2', + password: 'darla_is_cute', + password_confirmation: 'darla_is_cute', + terms: 'on', + }) + .expect(302) + .expect('Location', '/auth/'); + + // Verify saved user + const user = await User.select() + .where('name', 'entrapta2') + .first(); + + expect(user).toBeDefined(); + expect(user?.isApproved()).toBeFalsy(); + expect(user?.as(UserApprovedComponent).approved).toBeFalsy(); + + // Proof must be revoked + await agent.get('/has-any-password-auth-proof') + .set('Cookie', cookies) + .expect(404); + + await popEmail(); + }); +}); + +describe('Register with email (magic_link)', () => { + test('General case', async () => { + const res = await agent.get('/csrf').expect(200); + const cookies = res.get('Set-Cookie'); + const csrf = res.text; + + await agent.post('/auth/register?' + querystring.stringify({redirect_uri: '/redirect-uri'})) + .set('Cookie', cookies) + .send({ + csrf: csrf, + auth_method: 'magic_link', + identifier: 'glimmer2@example.org', + name: 'glimmer2', + }) + .expect(302) + .expect('Location', '/magic/lobby?redirect_uri=%2Fredirect-uri'); + + await followMagicLinkFromMail(agent, cookies, '/auth/'); + + // Verify saved user + const user = await User.select() + .with('mainEmail') + .where('name', 'glimmer2') + .first(); + + expect(user).toBeDefined(); + + const email = user?.mainEmail.getOrFail(); + expect(email).toBeDefined(); + + expect(user?.isApproved()).toBeFalsy(); + expect(user?.as(UserApprovedComponent).approved).toBeFalsy(); + + // Proof must be revoked + await agent.get('/has-any-magic-link') + .set('Cookie', cookies) + .expect(404); + + await popEmail(); + }); +}); + + +describe('Authenticate with username and password (password)', () => { + test('Force auth_method', async () => { + const res = await agent.get('/csrf').expect(200); + const cookies = res.get('Set-Cookie'); + const csrf = res.text; + + // Not authenticated + await agent.get('/is-auth').set('Cookie', cookies).expect(401); + + // Authenticate + await agent.post('/auth/login?' + querystring.stringify({redirect_uri: '/redirect-uri'})) + .set('Cookie', cookies) + .send({ + csrf: csrf, + identifier: 'entrapta2', + password: 'darla_is_cute', + auth_method: 'password', + }) + .expect(302) + .expect('Location', '/auth/'); + + // Proof must be revoked + await agent.get('/has-any-password-auth-proof') + .set('Cookie', cookies) + .expect(404); + }); +}); + +describe('Authenticate with email (magic_link)', () => { + test('Force auth_method', async () => { + const res = await agent.get('/csrf').expect(200); + const cookies = res.get('Set-Cookie'); + const csrf = res.text; + + // Not authenticated + await agent.get('/is-auth').set('Cookie', cookies).expect(401); + + // Authenticate + await agent.post('/auth/login?' + querystring.stringify({redirect_uri: '/redirect-uri'})) + .set('Cookie', cookies) + .send({ + csrf: csrf, + identifier: 'glimmer2@example.org', + auth_method: 'magic_link', + }) + .expect(302) + .expect('Location', '/magic/lobby?redirect_uri=%2Fredirect-uri'); + + await followMagicLinkFromMail(agent, cookies, '/auth/'); + + // Proof must be revoked + await agent.get('/has-any-magic-link') + .set('Cookie', cookies) + .expect(404); + }); +}); diff --git a/test/AuthenticationNoUsername.test.ts b/test/AuthenticationNoUsername.test.ts index ba7239d..46842f2 100644 --- a/test/AuthenticationNoUsername.test.ts +++ b/test/AuthenticationNoUsername.test.ts @@ -1,49 +1,21 @@ -import TestApp from "../src/TestApp"; import useApp from "./_app"; -import Controller from "../src/Controller"; import supertest from "supertest"; -import CsrfProtectionComponent from "../src/components/CsrfProtectionComponent"; import UserPasswordComponent from "../src/auth/password/UserPasswordComponent"; import {popEmail} from "./_mail_server"; -import AuthComponent from "../src/auth/AuthComponent"; -import Migration, {MigrationType} from "../src/db/Migration"; -import AddNameToUsersMigration from "../src/auth/migrations/AddNameToUsersMigration"; -import {followMagicLinkFromMail, testLogout} from "./_authentication_common"; +import {authAppProvider, followMagicLinkFromMail, testLogout} from "./_authentication_common"; import UserEmail from "../src/auth/models/UserEmail"; +import User from "../src/auth/models/User"; -let app: TestApp; -useApp(async (addr, port) => { - return app = new class extends TestApp { - protected async init(): Promise { - this.use(new class extends Controller { - public routes(): void { - this.get('/', (req, res) => { - res.render('home'); - }, 'home'); - this.get('/csrf', (req, res) => { - res.send(CsrfProtectionComponent.getCsrfToken(req.getSession())); - }, 'csrf'); - this.get('/is-auth', async (req, res) => { - const proofs = await this.getApp().as(AuthComponent).getAuthGuard().getProofs(req); - if (proofs.length > 0) res.sendStatus(200); - else res.sendStatus(401); - }, 'is-auth'); - } - }()); - - await super.init(); - } - - protected getMigrations(): MigrationType[] { - return super.getMigrations().filter(m => m !== AddNameToUsersMigration); - } - }(addr, port, true); -}); +const app = useApp(authAppProvider(false)); let agent: supertest.SuperTest; beforeAll(() => { - agent = supertest(app.getExpressApp()); + agent = supertest(app().getExpressApp()); +}); + +test('Approval Mode', () => { + expect(User.isApprovalMode()).toStrictEqual(false); }); describe('Register with username and password (password)', () => { diff --git a/test/_app.ts b/test/_app.ts index 2992023..5e5ba04 100644 --- a/test/_app.ts +++ b/test/_app.ts @@ -1,12 +1,11 @@ -import Application from "../src/Application"; import {setupMailServer, teardownMailServer} from "./_mail_server"; import TestApp from "../src/TestApp"; import MysqlConnectionManager from "../src/db/MysqlConnectionManager"; import config from "config"; -export default function useApp(appSupplier?: AppSupplier): void { - let app: Application; +export default function useApp(appSupplier: AppSupplier): () => T { + let app: T; beforeAll(async (done) => { await MysqlConnectionManager.prepare(); @@ -14,7 +13,7 @@ export default function useApp(appSupplier?: AppSupplier): void { await MysqlConnectionManager.endPool(); await setupMailServer(); - app = appSupplier ? await appSupplier('127.0.0.1', 8966) : new TestApp('127.0.0.1', 8966, true); + app = await appSupplier('127.0.0.1', 8966); await app.start(); done(); @@ -38,6 +37,8 @@ export default function useApp(appSupplier?: AppSupplier): void { if (errors.length > 0) throw errors; done(); }); + + return () => app; } -export type AppSupplier = (addr: string, port: number) => Promise; +export type AppSupplier = (addr: string, port: number) => Promise; diff --git a/test/_authentication_common.ts b/test/_authentication_common.ts index a0d9c2b..4554683 100644 --- a/test/_authentication_common.ts +++ b/test/_authentication_common.ts @@ -1,5 +1,15 @@ import {popEmail} from "./_mail_server"; import supertest from "supertest"; +import {AppSupplier} from "./_app"; +import TestApp from "../src/TestApp"; +import Controller from "../src/Controller"; +import CsrfProtectionComponent from "../src/components/CsrfProtectionComponent"; +import AuthComponent from "../src/auth/AuthComponent"; +import Migration, {MigrationType} from "../src/db/Migration"; +import AddNameToUsersMigration from "../src/auth/migrations/AddNameToUsersMigration"; +import AddApprovedFieldToUsersTableMigration from "../src/auth/migrations/AddApprovedFieldToUsersTableMigration"; +import PasswordAuthProof from "../src/auth/password/PasswordAuthProof"; +import MagicLink from "../src/auth/models/MagicLink"; export async function followMagicLinkFromMail( agent: supertest.SuperTest, @@ -37,3 +47,53 @@ export async function testLogout( // Not authenticated await agent.get('/is-auth').set('Cookie', cookies).expect(401); } + +export function authAppProvider(withUsername: boolean = true, approvalMode: boolean = false): AppSupplier { + return async (addr, port) => { + return new class extends TestApp { + protected async init(): Promise { + this.use(new class extends Controller { + public routes(): void { + this.get('/', (req, res) => { + res.render('home'); + }, 'home'); + this.get('/csrf', (req, res) => { + res.send(CsrfProtectionComponent.getCsrfToken(req.getSession())); + }, 'csrf'); + this.get('/is-auth', async (req, res) => { + const proofs = await this.getApp().as(AuthComponent).getAuthGuard().getProofs(req); + if (proofs.length > 0) res.sendStatus(200); + else res.sendStatus(401); + }, 'is-auth'); + this.get('/has-any-password-auth-proof', async (req, res) => { + const proof = await PasswordAuthProof.getProofForSession(req.getSession()); + if (proof) res.sendStatus(200); + else res.sendStatus(404); + }, 'is-auth'); + this.get('/has-any-magic-link', async (req, res) => { + const proofs = await MagicLink.select() + .where('session_id', req.getSession().id) + .get(); + if (proofs.length > 0) res.sendStatus(200); + else res.sendStatus(404); + }, 'is-auth'); + } + }()); + + await super.init(); + } + + protected getMigrations(): MigrationType[] { + let migrations = withUsername ? + super.getMigrations() : + super.getMigrations().filter(m => m !== AddNameToUsersMigration); + + migrations = approvalMode ? + [...migrations, AddApprovedFieldToUsersTableMigration] : + migrations; + + return migrations; + } + }(addr, port, true); + }; +}