From a79e2292d7c6b1ee53c5b91a68f657266682d737 Mon Sep 17 00:00:00 2001 From: Alice Gaudon Date: Sat, 25 Jul 2020 10:28:50 +0200 Subject: [PATCH] Greatly simplify authentication system --- src/auth/AuthComponent.ts | 8 +- src/auth/AuthController.ts | 3 +- src/auth/AuthGuard.ts | 91 +++++++------------ src/auth/AuthProof.ts | 43 +++++++-- .../magic_link/MagicLinkAuthController.ts | 16 +++- src/auth/magic_link/MagicLinkController.ts | 12 +-- src/auth/migrations/DropNameFromUsers.ts | 15 +++ .../migrations/FixUserMainEmailRelation.ts | 10 +- src/auth/models/MagicLink.ts | 33 +++---- src/auth/models/User.ts | 7 +- src/db/ModelComponent.ts | 4 +- test/_migrations.ts | 4 + 12 files changed, 137 insertions(+), 109 deletions(-) create mode 100644 src/auth/migrations/DropNameFromUsers.ts diff --git a/src/auth/AuthComponent.ts b/src/auth/AuthComponent.ts index 0bacb83..0605abf 100644 --- a/src/auth/AuthComponent.ts +++ b/src/auth/AuthComponent.ts @@ -15,7 +15,7 @@ export default class AuthComponent extends ApplicationComponent { public async init(router: Router): Promise { router.use(async (req, res, next) => { req.authGuard = this.authGuard; - res.locals.user = await req.authGuard.getUserForSession(req.session!); + res.locals.user = await (await req.authGuard.getProofForSession(req.session!))?.getResource(); next(); }); } @@ -30,13 +30,13 @@ export const REQUIRE_REQUEST_AUTH_MIDDLEWARE = async (req: Request, res: Respons return; } - req.models.user = await req.authGuard.getUserForRequest(req); + req.models.user = await (await req.authGuard.getProofForRequest(req))?.getResource(); next(); }; export const REQUIRE_AUTH_MIDDLEWARE = async (req: Request, res: Response, next: NextFunction): Promise => { if (await req.authGuard.isAuthenticatedViaRequest(req)) { - req.models.user = await req.authGuard.getUserForRequest(req); + req.models.user = await (await req.authGuard.getProofForRequest(req))?.getResource(); next(); } else { if (!await req.authGuard.isAuthenticated(req.session!)) { @@ -47,7 +47,7 @@ export const REQUIRE_AUTH_MIDDLEWARE = async (req: Request, res: Response, next: return; } - req.models.user = await req.authGuard.getUserForSession(req.session!); + req.models.user = await (await req.authGuard.getProofForSession(req.session!))?.getResource(); next(); } }; diff --git a/src/auth/AuthController.ts b/src/auth/AuthController.ts index 7bebc8e..f1e2734 100644 --- a/src/auth/AuthController.ts +++ b/src/auth/AuthController.ts @@ -26,7 +26,8 @@ export default abstract class AuthController extends Controller { protected abstract async getCheckAuth(req: Request, res: Response, next: NextFunction): Promise; protected async postLogout(req: Request, res: Response, next: NextFunction): Promise { - await req.authGuard.logout(req.session!); + const proof = await req.authGuard.getProofForSession(req.session!); + await proof?.revoke(); req.flash('success', 'Successfully logged out.'); res.redirect(req.query.redirect_uri?.toString() || '/'); } diff --git a/src/auth/AuthGuard.ts b/src/auth/AuthGuard.ts index cf87b49..5cd82b9 100644 --- a/src/auth/AuthGuard.ts +++ b/src/auth/AuthGuard.ts @@ -1,7 +1,6 @@ import AuthProof from "./AuthProof"; import MysqlConnectionManager from "../db/MysqlConnectionManager"; import User from "./models/User"; -import UserEmail from "./models/UserEmail"; import {Connection} from "mysql"; import {Request} from "express"; import {PENDING_ACCOUNT_REVIEW_MAIL_TEMPLATE} from "../Mails"; @@ -9,42 +8,34 @@ import Mail from "../Mail"; import Controller from "../Controller"; import config from "config"; -export default abstract class AuthGuard

{ +export default abstract class AuthGuard

> { public abstract async getProofForSession(session: Express.Session): Promise

; public async getProofForRequest(req: Request): Promise

{ return null; } - public async getUserForSession(session: Express.Session): Promise { - if (!await this.isAuthenticated(session)) return null; - return await User.getById(session.auth_id); - } + public async authenticateOrRegister( + session: Express.Session, + proof: P, + onLogin?: (user: User) => Promise, + onRegister?: (connection: Connection, user: User) => Promise + ): Promise { + if (!await proof.isValid()) throw new InvalidAuthProofError(); + if (!await proof.isAuthorized()) throw new UnauthorizedAuthProofError(); - public async authenticateOrRegister(session: Express.Session, proof: P, registerCallback?: (connection: Connection, userID: number) => Promise<(() => Promise)[]>): Promise { - if (!await proof.isAuthorized()) { - throw new AuthError('Invalid argument: cannot authenticate with an unauthorized proof.'); - } + let user = await proof.getResource(); - let user = await proof.getUser(); - if (!user) { // Register - const callbacks: (() => Promise)[] = []; + // Register if user doesn't exist + if (!user) { + const callbacks: RegisterCallback[] = []; await MysqlConnectionManager.wrapTransaction(async connection => { - const email = await proof.getEmail(); - user = new User({ - name: email.split('@')[0], - }); + user = new User({}); await user.save(connection, c => callbacks.push(c)); - const userEmail = new UserEmail({ - user_id: user.id, - email: email, - main: true, - }); - await userEmail.save(connection, c => callbacks.push(c)); - if (registerCallback) { - (await registerCallback(connection, user.id!)).forEach(c => callbacks.push(c)); + if (onRegister) { + (await onRegister(connection, user)).forEach(c => callbacks.push(c)); } }); @@ -62,36 +53,25 @@ export default abstract class AuthGuard

{ } else { throw new Error('Unable to register user.'); } - } else if (registerCallback) { - throw new UserAlreadyExistsAuthError(await proof.getEmail()); } + // Don't login if user isn't approved if (!user.isApproved()) { throw new PendingApprovalAuthError(); } + // Login session.auth_id = user.id; + if (onLogin) await onLogin(user); } public async isAuthenticated(session: Express.Session): Promise { - return await this.checkCurrentSessionProofValidity(session); - } - - public async logout(session: Express.Session): Promise { - const proof = await this.getProofForSession(session); - if (proof) { - await proof.revoke(session); - } - session.auth_id = undefined; - } - - private async checkCurrentSessionProofValidity(session: Express.Session): Promise { if (typeof session.auth_id !== 'number') return false; const proof = await this.getProofForSession(session); - if (!proof || !await proof.isValid() || !await proof.isAuthorized() || !await proof.isOwnedBy(session.auth_id)) { - await this.logout(session); + if (!proof || !await proof.isValid() || !await proof.isAuthorized()) { + await proof?.revoke(); return false; } @@ -100,16 +80,7 @@ export default abstract class AuthGuard

{ public async isAuthenticatedViaRequest(req: Request): Promise { const proof = await this.getProofForRequest(req); - if (proof && await proof.isValid() && await proof.isAuthorized()) { - return true; - } else { - return false; - } - } - - public async getUserForRequest(req: Request): Promise { - const proof = await this.getProofForRequest(req); - return proof ? await proof.getUser() : null; + return Boolean(proof && await proof.isValid() && await proof.isAuthorized()); } } @@ -120,12 +91,18 @@ export class AuthError extends Error { } } -export class UserAlreadyExistsAuthError extends AuthError { - public readonly email: string; +export class AuthProofError extends AuthError { +} - constructor(userEmail: string) { - super(`User with email ${userEmail} already exists.`); - this.email = userEmail; +export class InvalidAuthProofError extends AuthProofError { + constructor() { + super('Invalid auth proof.'); + } +} + +export class UnauthorizedAuthProofError extends AuthProofError { + constructor() { + super('Unauthorized auth proof.'); } } @@ -134,3 +111,5 @@ export class PendingApprovalAuthError extends AuthError { super(`User is not approved.`); } } + +export type RegisterCallback = () => Promise; diff --git a/src/auth/AuthProof.ts b/src/auth/AuthProof.ts index a7a4e65..785745b 100644 --- a/src/auth/AuthProof.ts +++ b/src/auth/AuthProof.ts @@ -1,15 +1,40 @@ -import User from "./models/User"; - -export default interface AuthProof { +/** + * This class is most commonly used for authentication. It can be more generically used to represent a verification + * state of whether a given resource is owned by a session. + * + * Any auth system should consider this auth proof valid if and only if both {@code isValid()} and {@code isAuthorized()} + * both return {@code true}. + * + * @type The resource type this AuthProof authorizes. + */ +export default interface AuthProof { + /** + * Is this auth proof valid in time (and context)? + * + * For example, it can return true for an initial short validity time period then false, and increase that time + * period if {@code isAuthorized()} returns true. + */ isValid(): Promise; + /** + * Was this proof authorized? + * + * Return true once the session is proven to own the associated resource. + */ isAuthorized(): Promise; - isOwnedBy(userId: number): Promise; + /** + * Retrieve the resource this auth proof is supposed to authorize. + * If this resource doesn't exist yet, return {@code null}. + */ + getResource(): Promise; - getUser(): Promise; - - getEmail(): Promise; - - revoke(session: Express.Session): Promise; + /** + * Manually revokes this authentication proof. Once this method is called, all of the following must be true: + * - {@code isAuthorized} returns {@code false} + * - There is no way to re-authorize this proof (i.e. {@code isAuthorized} can never return {@code true} again) + * + * Additionally, this method should delete any stored data that could lead to restoration of this AuthProof instance. + */ + revoke(): Promise; } \ No newline at end of file diff --git a/src/auth/magic_link/MagicLinkAuthController.ts b/src/auth/magic_link/MagicLinkAuthController.ts index 3162f54..f6a6714 100644 --- a/src/auth/magic_link/MagicLinkAuthController.ts +++ b/src/auth/magic_link/MagicLinkAuthController.ts @@ -5,7 +5,7 @@ import {BadRequestError} from "../../HttpError"; import UserEmail from "../models/UserEmail"; import MagicLinkController from "./MagicLinkController"; import {MailTemplate} from "../../Mail"; -import {AuthError, PendingApprovalAuthError} from "../AuthGuard"; +import {AuthError, PendingApprovalAuthError, RegisterCallback} from "../AuthGuard"; import geoip from "geoip-lite"; import AuthController from "../AuthController"; import NunjucksComponent from "../../components/NunjucksComponent"; @@ -19,7 +19,19 @@ export default abstract class MagicLinkAuthController extends AuthController { // Auth try { - await req.authGuard.authenticateOrRegister(req.session!, magicLink); + await req.authGuard.authenticateOrRegister(req.session!, magicLink, undefined, async (connection, user) => { + const callbacks: RegisterCallback[] = []; + + const userEmail = new UserEmail({ + user_id: user.id, + email: magicLink.getEmail(), + }); + await userEmail.save(connection, c => callbacks.push(c)); + user.main_email_id = userEmail.id; + await user.save(connection, c => callbacks.push(c)); + + return callbacks; + }); } catch (e) { if (e instanceof PendingApprovalAuthError) { res.format({ diff --git a/src/auth/magic_link/MagicLinkController.ts b/src/auth/magic_link/MagicLinkController.ts index 38a117e..a516db8 100644 --- a/src/auth/magic_link/MagicLinkController.ts +++ b/src/auth/magic_link/MagicLinkController.ts @@ -39,16 +39,16 @@ export default abstract class MagicLinkController extends Controller { this.magicLinkWebsocketPath = magicLinkWebsocketListener.path(); } - getRoutesPrefix(): string { + public getRoutesPrefix(): string { return '/magic'; } - routes(): void { + public routes(): void { this.get('/lobby', this.getLobby, 'magic_link_lobby'); this.get('/link', this.getMagicLink, 'magic_link'); } - private async getLobby(req: Request, res: Response): Promise { + protected async getLobby(req: Request, res: Response): Promise { const link = await MagicLink.bySessionID(req.sessionID!); if (!link) { throw new NotFoundHttpError('magic link', req.url); @@ -66,14 +66,14 @@ export default abstract class MagicLinkController extends Controller { } res.render('magic_link_lobby', { - email: await link.getEmail(), - type: await link.getActionType(), + email: link.getEmail(), + type: link.getActionType(), validUntil: link.getExpirationDate().getTime(), websocketUrl: config.get('public_websocket_url') + this.magicLinkWebsocketPath, }); } - private async getMagicLink(req: Request, res: Response): Promise { + protected async getMagicLink(req: Request, res: Response): Promise { const id = parseInt(req.query.id); const token = req.query.token; if (!id || !token) throw new BadRequestError('Need parameters id, token.', 'Please try again.', req.originalUrl); diff --git a/src/auth/migrations/DropNameFromUsers.ts b/src/auth/migrations/DropNameFromUsers.ts new file mode 100644 index 0000000..afb116e --- /dev/null +++ b/src/auth/migrations/DropNameFromUsers.ts @@ -0,0 +1,15 @@ +import Migration from "../../db/Migration"; +import {Connection} from "mysql"; + +export default class DropNameFromUsers extends Migration { + public async install(connection: Connection): Promise { + await this.query('ALTER TABLE users DROP COLUMN name', connection); + } + + public async rollback(connection: Connection): Promise { + await this.query('ALTER TABLE users ADD COLUMN name VARCHAR(64)', connection); + } + + public registerModels(): void { + } +} \ No newline at end of file diff --git a/src/auth/migrations/FixUserMainEmailRelation.ts b/src/auth/migrations/FixUserMainEmailRelation.ts index ab77fea..e44ef73 100644 --- a/src/auth/migrations/FixUserMainEmailRelation.ts +++ b/src/auth/migrations/FixUserMainEmailRelation.ts @@ -4,10 +4,10 @@ import {Connection} from "mysql"; export default class FixUserMainEmailRelation extends Migration { public async install(connection: Connection): Promise { await this.query(`ALTER TABLE users - ADD COLUMN main_user_email_id INT, - ADD FOREIGN KEY main_user_email_fk (main_user_email_id) REFERENCES user_emails (id)`, connection); + ADD COLUMN main_email_id INT, + ADD FOREIGN KEY main_user_email_fk (main_email_id) REFERENCES user_emails (id)`, connection); await this.query(`UPDATE users u LEFT JOIN user_emails ue ON u.id = ue.user_id - SET u.main_user_email_id=ue.id + SET u.main_email_id=ue.id WHERE ue.main = true`, connection); await this.query(`ALTER TABLE user_emails DROP COLUMN main`, connection); @@ -16,11 +16,11 @@ export default class FixUserMainEmailRelation extends Migration { public async rollback(connection: Connection): Promise { await this.query(`ALTER TABLE user_emails ADD COLUMN main BOOLEAN DEFAULT false`, connection); - await this.query(`UPDATE user_emails ue LEFT JOIN users u ON ue.id = u.main_user_email_id + await this.query(`UPDATE user_emails ue LEFT JOIN users u ON ue.id = u.main_email_id SET ue.main = true`, connection) await this.query(`ALTER TABLE users DROP FOREIGN KEY main_user_email_fk, - DROP COLUMN main_user_email_id`, connection); + DROP COLUMN main_email_id`, connection); } public registerModels(): void { diff --git a/src/auth/models/MagicLink.ts b/src/auth/models/MagicLink.ts index 255cb1e..0d3d422 100644 --- a/src/auth/models/MagicLink.ts +++ b/src/auth/models/MagicLink.ts @@ -7,7 +7,7 @@ import argon2 from "argon2"; import {WhereTest} from "../../db/ModelQuery"; import UserEmail from "./UserEmail"; -export default class MagicLink extends Model implements AuthProof { +export default class MagicLink extends Model implements AuthProof { public static async bySessionID(sessionID: string, actionType?: string | string[]): Promise { let query = this.select().where('session_id', sessionID); if (actionType !== undefined) { @@ -31,15 +31,12 @@ export default class MagicLink extends Model implements AuthProof { private action_type?: string = undefined; private original_url?: string = undefined; private generated_at?: Date = undefined; - private authorized?: boolean = undefined; + private authorized: boolean = false; constructor(data: any) { super(data); if (this.action_type === undefined) throw new Error('Action type must always be defined.'); if (this.original_url === undefined) throw new Error('Origin url must always be defined.'); - if (this.authorized === undefined) { - this.authorized = false; - } } protected init(): void { @@ -51,13 +48,9 @@ export default class MagicLink extends Model implements AuthProof { this.setValidation('authorized').defined(); } - public async isOwnedBy(userId: number): Promise { - const user = await this.getUser(); - return user !== null && user.id === userId; - } - - public async getUser(): Promise { + public async getResource(): Promise { const email = await UserEmail.select() + .with('user') .where('email', await this.getEmail()) .first(); return email ? email.user.get() : null; @@ -68,15 +61,18 @@ export default class MagicLink extends Model implements AuthProof { } public async isValid(): Promise { - if (await this.isAuthorized()) return true; - - return new Date().getTime() < this.getExpirationDate().getTime(); + return await this.isAuthorized() || + new Date().getTime() < this.getExpirationDate().getTime(); } public async isAuthorized(): Promise { return this.authorized!; } + public authorize() { + this.authorized = true; + } + public async generateToken(email: string): Promise { const rawToken = crypto.randomBytes(48).toString('base64'); // Raw token length = 64 this.email = email; @@ -84,7 +80,8 @@ export default class MagicLink extends Model implements AuthProof { this.token = await argon2.hash(rawToken, { timeCost: 10, memoryCost: 4096, - parallelism: 4 + parallelism: 4, + hashLength: 32, }); return rawToken; } @@ -104,7 +101,7 @@ export default class MagicLink extends Model implements AuthProof { return this.session_id!; } - public async getEmail(): Promise { + public getEmail(): string { return this.email!; } @@ -121,8 +118,4 @@ export default class MagicLink extends Model implements AuthProof { return new Date(this.generated_at.getTime() + MagicLink.validityPeriod()); } - - public authorize() { - this.authorized = true; - } } diff --git a/src/auth/models/User.ts b/src/auth/models/User.ts index 8155280..ec0c17a 100644 --- a/src/auth/models/User.ts +++ b/src/auth/models/User.ts @@ -13,8 +13,7 @@ export default class User extends Model { } public readonly id?: number = undefined; - public name?: string = undefined; - public main_user_email_id?: number = undefined; + public main_email_id?: number = undefined; public is_admin: boolean = false; public created_at?: Date = undefined; public updated_at?: Date = undefined; @@ -24,7 +23,7 @@ export default class User extends Model { foreignKey: 'user_id' }); - public readonly mainEmail = this.emails.cloneReduceToOne().constraint(q => q.where('id', this.main_user_email_id)); + public readonly mainEmail = this.emails.cloneReduceToOne().constraint(q => q.where('id', this.main_email_id)); public constructor(data: any) { super(data); @@ -32,7 +31,7 @@ export default class User extends Model { protected init(): void { this.setValidation('name').acceptUndefined().between(3, 64); - this.setValidation('main_user_email_id').acceptUndefined().exists(UserEmail, 'id'); + this.setValidation('main_email_id').acceptUndefined().exists(UserEmail, 'id'); if (User.isApprovalMode()) { this.setValidation('approved').defined(); } diff --git a/src/db/ModelComponent.ts b/src/db/ModelComponent.ts index f1f675d..5c4b6c9 100644 --- a/src/db/ModelComponent.ts +++ b/src/db/ModelComponent.ts @@ -19,8 +19,8 @@ export default abstract class ModelComponent { } } - protected setValidation(propertyName: keyof this): Validator { - const validator = new Validator(); + protected setValidation(propertyName: keyof this): Validator { + const validator = new Validator(); this._validators[propertyName as string] = validator; return validator; } diff --git a/test/_migrations.ts b/test/_migrations.ts index 2579d01..7611f2a 100644 --- a/test/_migrations.ts +++ b/test/_migrations.ts @@ -2,10 +2,14 @@ import CreateMigrationsTable from "../src/migrations/CreateMigrationsTable"; import CreateLogsTable from "../src/migrations/CreateLogsTable"; import CreateUsersAndUserEmailsTable from "../src/auth/migrations/CreateUsersAndUserEmailsTable"; import FixUserMainEmailRelation from "../src/auth/migrations/FixUserMainEmailRelation"; +import DropNameFromUsers from "../src/auth/migrations/DropNameFromUsers"; +import CreateMagicLinksTable from "../src/auth/migrations/CreateMagicLinksTable"; export const MIGRATIONS = [ CreateMigrationsTable, CreateLogsTable, CreateUsersAndUserEmailsTable, FixUserMainEmailRelation, + DropNameFromUsers, + CreateMagicLinksTable, ]; \ No newline at end of file