From b75b227ca151d8745aa9e9e7929c81bc2fce1f8c Mon Sep 17 00:00:00 2001 From: Alice Gaudon Date: Sat, 14 Nov 2020 17:24:42 +0100 Subject: [PATCH] Add required username to magic link authentication and fix many errors --- config/default.json5 | 3 + config/production.json5 | 5 +- config/test.json5 | 4 +- src/Mails.ts | 2 +- src/TestApp.ts | 40 +++++---- src/auth/AuthController.ts | 26 ++++-- src/auth/AuthGuard.ts | 2 +- .../magic_link/AuthMagicLinkActionType.ts | 4 +- src/auth/magic_link/MagicLinkAuthMethod.ts | 70 +++++++-------- src/auth/magic_link/MagicLinkController.ts | 16 +++- .../migrations/AddNameToUsersMigration.ts | 8 +- src/auth/models/MagicLinkUserNameComponent.ts | 12 +++ src/auth/{ => models}/UserNameComponent.ts | 4 +- .../password/AddPasswordToUsersMigration.ts | 2 +- src/auth/password/PasswordAuthMethod.ts | 2 +- src/components/MailComponent.ts | 2 +- src/helpers/BackendController.ts | 2 +- src/{ => mail}/Mail.ts | 6 +- src/{auth => mail}/MailController.ts | 2 +- test/views/home.njk | 7 ++ test/views/layouts/base.njk | 17 ++-- views/auth/auth.njk | 90 ++++++++++++------- views/magic_link.njk | 1 - views/magic_link_lobby.njk | 3 +- 24 files changed, 208 insertions(+), 122 deletions(-) create mode 100644 src/auth/models/MagicLinkUserNameComponent.ts rename src/auth/{ => models}/UserNameComponent.ts (77%) rename src/{ => mail}/Mail.ts (97%) rename src/{auth => mail}/MailController.ts (95%) create mode 100644 test/views/home.njk diff --git a/config/default.json5 b/config/default.json5 index bffbcb9..674b314 100644 --- a/config/default.json5 +++ b/config/default.json5 @@ -46,5 +46,8 @@ view: { cache: false }, + magic_link: { + validity_period: 20, + }, approval_mode: false, } diff --git a/config/production.json5 b/config/production.json5 index 9dfde6e..20581fe 100644 --- a/config/production.json5 +++ b/config/production.json5 @@ -14,5 +14,8 @@ mail: { secure: true, allow_invalid_tls: false - } + }, + magic_link: { + validity_period: 900, + }, } diff --git a/config/test.json5 b/config/test.json5 index cda9d47..e5cac9b 100644 --- a/config/test.json5 +++ b/config/test.json5 @@ -4,6 +4,6 @@ user: "root", password: "", database: "swaf_test", - create_database_automatically: true - } + create_database_automatically: true, + }, } diff --git a/src/Mails.ts b/src/Mails.ts index e3f063f..03317f2 100644 --- a/src/Mails.ts +++ b/src/Mails.ts @@ -1,5 +1,5 @@ import config from "config"; -import {MailTemplate} from "./Mail"; +import {MailTemplate} from "./mail/Mail"; export const MAGIC_LINK_MAIL = new MailTemplate( 'magic_link', diff --git a/src/TestApp.ts b/src/TestApp.ts index 8a387fc..d29595b 100644 --- a/src/TestApp.ts +++ b/src/TestApp.ts @@ -23,22 +23,23 @@ import MagicLinkWebSocketListener from "./auth/magic_link/MagicLinkWebSocketList import MagicLinkController from "./auth/magic_link/MagicLinkController"; import AddPasswordToUsersMigration from "./auth/password/AddPasswordToUsersMigration"; import AddNameToUsersMigration from "./auth/migrations/AddNameToUsersMigration"; -import packageJson = require('../package.json'); import CsrfProtectionComponent from "./components/CsrfProtectionComponent"; +import MailController from "./mail/MailController"; +import WebSocketServerComponent from "./components/WebSocketServerComponent"; +import Controller from "./Controller"; +import packageJson = require('../package.json'); export const MIGRATIONS = [ CreateMigrationsTable, CreateUsersAndUserEmailsTableMigration, - AddNameToUsersMigration, AddPasswordToUsersMigration, CreateMagicLinksTableMigration, + AddNameToUsersMigration, ]; export default class TestApp extends Application { private readonly addr: string; private readonly port: number; - private expressAppComponent?: ExpressAppComponent; - private magicLinkWebSocketListener?: MagicLinkWebSocketListener; public constructor(addr: string, port: number) { super(packageJson.version, true); @@ -57,12 +58,8 @@ export default class TestApp extends Application { } protected registerComponents(): void { - this.expressAppComponent = new ExpressAppComponent(this.addr, this.port); - const redisComponent = new RedisComponent(); - const mysqlComponent = new MysqlComponent(); - // Base - this.use(this.expressAppComponent); + this.use(new ExpressAppComponent(this.addr, this.port)); this.use(new LogRequestsComponent()); // Static files @@ -73,12 +70,12 @@ export default class TestApp extends Application { this.use(new RedirectBackComponent()); // Services - this.use(mysqlComponent); + this.use(new MysqlComponent()); this.use(new MailComponent()); // Session - this.use(redisComponent); - this.use(new SessionComponent(redisComponent)); + this.use(new RedisComponent()); + this.use(new SessionComponent(this.as(RedisComponent))); // Utils this.use(new FormHelperComponent()); @@ -88,18 +85,29 @@ export default class TestApp extends Application { // Auth this.use(new AuthComponent(this, new MagicLinkAuthMethod(this, MAGIC_LINK_MAIL), new PasswordAuthMethod(this))); + + // WebSocket server + this.use(new WebSocketServerComponent(this, this.as(ExpressAppComponent), this.as(RedisComponent))); } protected registerWebSocketListeners(): void { - this.magicLinkWebSocketListener = new MagicLinkWebSocketListener(); - this.use(this.magicLinkWebSocketListener); + this.use(new MagicLinkWebSocketListener()); } protected registerControllers(): void { + this.use(new MailController()); this.use(new AuthController()); - if (!this.magicLinkWebSocketListener) throw new Error('Magic link websocket listener not initialized.'); - this.use(new MagicLinkController(this.magicLinkWebSocketListener)); + this.use(new MagicLinkController(this.as>(MagicLinkWebSocketListener))); + + // Special home controller + this.use(new class extends Controller { + public routes(): void { + this.get('/', (req, res) => { + res.render('home'); + }, 'home'); + } + }()); } public getExpressApp(): Express { diff --git a/src/auth/AuthController.ts b/src/auth/AuthController.ts index 33e6a6a..e66698e 100644 --- a/src/auth/AuthController.ts +++ b/src/auth/AuthController.ts @@ -2,6 +2,11 @@ import Controller from "../Controller"; import {NextFunction, Request, Response} from "express"; import AuthComponent, {AuthMiddleware, RequireAuthMiddleware, RequireGuestMiddleware} from "./AuthComponent"; import {BadRequestError} from "../HttpError"; +import ModelFactory from "../db/ModelFactory"; +import User from "./models/User"; +import UserPasswordComponent from "./password/UserPasswordComponent"; +import UserNameComponent from "./models/UserNameComponent"; +import {log} from "../Logger"; export default class AuthController extends Controller { public getRoutesPrefix(): string { @@ -9,6 +14,8 @@ export default class AuthController extends Controller { } public routes(): void { + this.post('/logout', this.postLogout, 'logout', RequireAuthMiddleware); + this.use(async (req, res, next) => { const authGuard = this.getApp().as(AuthComponent).getAuthGuard(); if (await authGuard.interruptAuth(req, res)) return; @@ -18,14 +25,17 @@ export default class AuthController extends Controller { this.get('/', this.getAuth, 'auth', RequireGuestMiddleware); this.post('/login', this.postLogin, 'login', RequireGuestMiddleware); this.post('/register', this.postRegister, 'register', RequireGuestMiddleware); - this.post('/logout', this.postLogout, 'logout', RequireAuthMiddleware); } protected async getAuth(req: Request, res: Response, _next: NextFunction): Promise { const authGuard = this.getApp().as(AuthComponent).getAuthGuard(); + const userModelFactory = ModelFactory.get(User); + const hasUsername = userModelFactory.hasComponent(UserNameComponent); res.render('auth/auth', { auth_methods: authGuard.getAuthMethodNames(), + has_username: hasUsername, + register_with_password: hasUsername && userModelFactory.hasComponent(UserPasswordComponent), }); } @@ -53,9 +63,10 @@ export default class AuthController extends Controller { const user = await method.findUserByIdentifier(identifier); if (!user) { // Register - return isRegistration ? - await method.attemptRegister(req, res, identifier) : - await this.redirectToRegistration(req, res, identifier); + if (!isRegistration) return await this.redirectToRegistration(req, res, identifier); + + await method.attemptRegister(req, res, identifier); + return; } // Login @@ -66,9 +77,10 @@ export default class AuthController extends Controller { const methods = await authGuard.getAuthMethodsByIdentifier(identifier); if (methods.length === 0) { // Register - return isRegistration ? - await authGuard.getRegistrationMethod().attemptRegister(req, res, identifier) : - await this.redirectToRegistration(req, res, identifier); + if (!isRegistration) return await this.redirectToRegistration(req, res, identifier); + + await authGuard.getRegistrationMethod().attemptRegister(req, res, identifier); + return; } const {user, method} = methods[0]; diff --git a/src/auth/AuthGuard.ts b/src/auth/AuthGuard.ts index 1733879..5631a67 100644 --- a/src/auth/AuthGuard.ts +++ b/src/auth/AuthGuard.ts @@ -4,7 +4,7 @@ import User from "./models/User"; import {Connection} from "mysql"; import {Request, Response} from "express"; import {PENDING_ACCOUNT_REVIEW_MAIL_TEMPLATE} from "../Mails"; -import Mail from "../Mail"; +import Mail from "../mail/Mail"; import Controller from "../Controller"; import config from "config"; import Application from "../Application"; diff --git a/src/auth/magic_link/AuthMagicLinkActionType.ts b/src/auth/magic_link/AuthMagicLinkActionType.ts index 99a72e0..1ff51ae 100644 --- a/src/auth/magic_link/AuthMagicLinkActionType.ts +++ b/src/auth/magic_link/AuthMagicLinkActionType.ts @@ -1,4 +1,4 @@ export default { - LOGIN: 'Login', - REGISTER: 'Register', + LOGIN: 'login', + REGISTER: 'register', }; diff --git a/src/auth/magic_link/MagicLinkAuthMethod.ts b/src/auth/magic_link/MagicLinkAuthMethod.ts index 73a5da6..f58156e 100644 --- a/src/auth/magic_link/MagicLinkAuthMethod.ts +++ b/src/auth/magic_link/MagicLinkAuthMethod.ts @@ -9,7 +9,7 @@ import geoip from "geoip-lite"; import MagicLinkController from "./MagicLinkController"; import RedirectBackComponent from "../../components/RedirectBackComponent"; import Application from "../../Application"; -import {MailTemplate} from "../../Mail"; +import {MailTemplate} from "../../mail/Mail"; import AuthMagicLinkActionType from "./AuthMagicLinkActionType"; export default class MagicLinkAuthMethod implements AuthMethod { @@ -25,7 +25,7 @@ export default class MagicLinkAuthMethod implements AuthMethod { public async findUserByIdentifier(identifier: string): Promise { return (await UserEmail.select() - .with('user') + .with('user.mainEmail') .where('email', identifier) .first())?.user.getOrFail() || null; } @@ -40,15 +40,17 @@ export default class MagicLinkAuthMethod implements AuthMethod { public async interruptAuth(req: Request, res: Response): Promise { const pendingLink = await MagicLink.select() .where('session_id', req.getSession().id) - .where('action_type', [AuthMagicLinkActionType.LOGIN, AuthMagicLinkActionType.REGISTER], WhereTest.IN) - .where('authorized', false) .first(); - if (pendingLink && await pendingLink.isValid()) { - res.redirect(Controller.route('magic_link_lobby', undefined, { - redirect_uri: req.query.redirect_uri?.toString() || pendingLink.original_url || undefined, - })); - return true; + if (pendingLink) { + if (await pendingLink.isValid()) { + res.redirect(Controller.route('magic_link_lobby', undefined, { + redirect_uri: req.query.redirect_uri?.toString() || pendingLink.original_url || undefined, + })); + return true; + } else { + await pendingLink.delete(); + } } return false; @@ -70,34 +72,30 @@ export default class MagicLinkAuthMethod implements AuthMethod { } private async auth(req: Request, res: Response, isRegistration: boolean, email: string): Promise { - if (!isRegistration || req.body.confirm_register === 'confirm') { - const geo = geoip.lookup(req.ip); - const actionType = isRegistration ? AuthMagicLinkActionType.REGISTER : AuthMagicLinkActionType.LOGIN; + const geo = geoip.lookup(req.ip); + const actionType = isRegistration ? AuthMagicLinkActionType.REGISTER : AuthMagicLinkActionType.LOGIN; - await MagicLinkController.sendMagicLink( - this.app, - req.getSession().id, - actionType, - Controller.route('auth', undefined, { - redirect_uri: req.query.redirect_uri?.toString() || undefined, - }), - email, - this.magicLinkMailTemplate, - { - type: actionType, - ip: req.ip, - geo: geo ? `${geo.city}, ${geo.country}` : 'Unknown location', - }, - ); - - res.redirect(Controller.route('magic_link_lobby', undefined, { - redirect_uri: req.query.redirect_uri?.toString() || RedirectBackComponent.getPreviousURL(req), - })); - } else { - req.flash('register_identifier', email); - res.redirect(Controller.route('auth', undefined, { + await MagicLinkController.sendMagicLink( + this.app, + req.getSession().id, + actionType, + Controller.route('auth', undefined, { redirect_uri: req.query.redirect_uri?.toString() || undefined, - })); - } + }), + email, + this.magicLinkMailTemplate, + { + type: actionType, + ip: req.ip, + geo: geo ? `${geo.city}, ${geo.country}` : 'Unknown location', + }, + { + username: req.body.name, + }, + ); + + res.redirect(Controller.route('magic_link_lobby', undefined, { + redirect_uri: req.query.redirect_uri?.toString() || RedirectBackComponent.getPreviousURL(req), + })); } } diff --git a/src/auth/magic_link/MagicLinkController.ts b/src/auth/magic_link/MagicLinkController.ts index 67f5926..a0b7612 100644 --- a/src/auth/magic_link/MagicLinkController.ts +++ b/src/auth/magic_link/MagicLinkController.ts @@ -3,7 +3,7 @@ import {Request, Response} from "express"; import MagicLinkWebSocketListener from "./MagicLinkWebSocketListener"; import {BadRequestError, NotFoundHttpError} from "../../HttpError"; import Throttler from "../../Throttler"; -import Mail, {MailTemplate} from "../../Mail"; +import Mail, {MailTemplate} from "../../mail/Mail"; import MagicLink from "../models/MagicLink"; import config from "config"; import Application from "../../Application"; @@ -14,6 +14,9 @@ import AuthComponent, {AuthMiddleware} from "../AuthComponent"; import {AuthError, PendingApprovalAuthError, RegisterCallback} from "../AuthGuard"; import UserEmail from "../models/UserEmail"; import AuthMagicLinkActionType from "./AuthMagicLinkActionType"; +import {QueryVariable} from "../../db/MysqlConnectionManager"; +import UserNameComponent from "../models/UserNameComponent"; +import MagicLinkUserNameComponent from "../models/MagicLinkUserNameComponent"; export default class MagicLinkController extends Controller { public static async sendMagicLink( @@ -24,15 +27,16 @@ export default class MagicLinkController extends Controll email: string, mailTemplate: MailTemplate, data: ParsedUrlQueryInput, + magicLinkData: Record = {}, ): Promise { Throttler.throttle('magic_link', 2, MagicLink.validityPeriod(), sessionId, 0, 0); Throttler.throttle('magic_link', 1, MagicLink.validityPeriod(), email, 0, 0); - const link = MagicLink.create({ + const link = MagicLink.create(Object.assign(magicLinkData, { session_id: sessionId, action_type: actionType, original_url: original_url, - }); + })); const token = await link.generateToken(email); await link.save(); @@ -55,7 +59,11 @@ export default class MagicLinkController extends Controll // Auth try { return await req.as(AuthMiddleware).getAuthGuard().authenticateOrRegister( - session, magicLink, undefined, undefined, async (connection, user) => { + session, magicLink, undefined, async (connection, user) => { + const userNameComponent = user.asOptional(UserNameComponent); + if (userNameComponent) userNameComponent.name = magicLink.as(MagicLinkUserNameComponent).username; + return []; + }, async (connection, user) => { const callbacks: RegisterCallback[] = []; const userEmail = UserEmail.create({ diff --git a/src/auth/migrations/AddNameToUsersMigration.ts b/src/auth/migrations/AddNameToUsersMigration.ts index cd895c0..0c3bfb0 100644 --- a/src/auth/migrations/AddNameToUsersMigration.ts +++ b/src/auth/migrations/AddNameToUsersMigration.ts @@ -1,19 +1,25 @@ import Migration from "../../db/Migration"; import ModelFactory from "../../db/ModelFactory"; import User from "../models/User"; -import UserNameComponent from "../UserNameComponent"; +import UserNameComponent from "../models/UserNameComponent"; +import MagicLink from "../models/MagicLink"; +import MagicLinkUserNameComponent from "../models/MagicLinkUserNameComponent"; export default class AddNameToUsersMigration extends Migration { public async install(): Promise { await this.query(`ALTER TABLE users ADD COLUMN name VARCHAR(64) UNIQUE NOT NULL`); + await this.query(`ALTER TABLE magic_links + ADD COLUMN username VARCHAR(64) DEFAULT NULL`); } public async rollback(): Promise { await this.query('ALTER TABLE users DROP COLUMN name'); + await this.query('ALTER TABLE magic_links DROP COLUMN username'); } public registerModels(): void { ModelFactory.get(User).addComponent(UserNameComponent); + ModelFactory.get(MagicLink).addComponent(MagicLinkUserNameComponent); } } diff --git a/src/auth/models/MagicLinkUserNameComponent.ts b/src/auth/models/MagicLinkUserNameComponent.ts new file mode 100644 index 0000000..5bf9572 --- /dev/null +++ b/src/auth/models/MagicLinkUserNameComponent.ts @@ -0,0 +1,12 @@ +import ModelComponent from "../../db/ModelComponent"; +import MagicLink from "./MagicLink"; +import {USERNAME_REGEXP} from "./UserNameComponent"; +import User from "./User"; + +export default class MagicLinkUserNameComponent extends ModelComponent { + public readonly username?: string = undefined; + + protected init(): void { + this.setValidation('name').defined().between(3, 64).regexp(USERNAME_REGEXP).unique(User, 'name'); + } +} diff --git a/src/auth/UserNameComponent.ts b/src/auth/models/UserNameComponent.ts similarity index 77% rename from src/auth/UserNameComponent.ts rename to src/auth/models/UserNameComponent.ts index e3bff05..930b794 100644 --- a/src/auth/UserNameComponent.ts +++ b/src/auth/models/UserNameComponent.ts @@ -1,5 +1,5 @@ -import ModelComponent from "../db/ModelComponent"; -import User from "./models/User"; +import ModelComponent from "../../db/ModelComponent"; +import User from "../models/User"; export const USERNAME_REGEXP = /^[0-9a-z_-]+$/; diff --git a/src/auth/password/AddPasswordToUsersMigration.ts b/src/auth/password/AddPasswordToUsersMigration.ts index 227644c..520b4df 100644 --- a/src/auth/password/AddPasswordToUsersMigration.ts +++ b/src/auth/password/AddPasswordToUsersMigration.ts @@ -6,7 +6,7 @@ import UserPasswordComponent from "./UserPasswordComponent"; export default class AddPasswordToUsersMigration extends Migration { public async install(): Promise { await this.query(`ALTER TABLE users - ADD COLUMN password VARCHAR(128) NOT NULL`); + ADD COLUMN password VARCHAR(128) DEFAULT NULL`); } public async rollback(): Promise { diff --git a/src/auth/password/PasswordAuthMethod.ts b/src/auth/password/PasswordAuthMethod.ts index 3c6c495..60a0a83 100644 --- a/src/auth/password/PasswordAuthMethod.ts +++ b/src/auth/password/PasswordAuthMethod.ts @@ -10,7 +10,7 @@ import {AuthError, PendingApprovalAuthError, RegisterCallback} from "../AuthGuar import Validator, {InvalidFormatValidationError, ValidationBag} from "../../db/Validator"; import Controller from "../../Controller"; import UserPasswordComponent from "./UserPasswordComponent"; -import UserNameComponent, {USERNAME_REGEXP} from "../UserNameComponent"; +import UserNameComponent, {USERNAME_REGEXP} from "../models/UserNameComponent"; import ModelFactory from "../../db/ModelFactory"; import {WhereOperator, WhereTest} from "../../db/ModelQuery"; import {ServerError} from "../../HttpError"; diff --git a/src/components/MailComponent.ts b/src/components/MailComponent.ts index b4d5ed9..73c3e93 100644 --- a/src/components/MailComponent.ts +++ b/src/components/MailComponent.ts @@ -1,6 +1,6 @@ import ApplicationComponent from "../ApplicationComponent"; import {Express} from "express"; -import Mail from "../Mail"; +import Mail from "../mail/Mail"; import config from "config"; import SecurityError from "../SecurityError"; diff --git a/src/helpers/BackendController.ts b/src/helpers/BackendController.ts index 52c8607..107f46e 100644 --- a/src/helpers/BackendController.ts +++ b/src/helpers/BackendController.ts @@ -3,7 +3,7 @@ import Controller from "../Controller"; import User from "../auth/models/User"; import {Request, Response} from "express"; import {BadRequestError, NotFoundHttpError} from "../HttpError"; -import Mail from "../Mail"; +import Mail from "../mail/Mail"; import {ACCOUNT_REVIEW_NOTICE_MAIL_TEMPLATE} from "../Mails"; import UserEmail from "../auth/models/UserEmail"; import UserApprovedComponent from "../auth/models/UserApprovedComponent"; diff --git a/src/Mail.ts b/src/mail/Mail.ts similarity index 97% rename from src/Mail.ts rename to src/mail/Mail.ts index 820e40d..1551321 100644 --- a/src/Mail.ts +++ b/src/mail/Mail.ts @@ -3,10 +3,10 @@ import config from "config"; import {Options} from "nodemailer/lib/mailer"; import {Environment} from 'nunjucks'; import * as util from "util"; -import {WrappingError} from "./Utils"; +import {WrappingError} from "../Utils"; import mjml2html from "mjml"; -import {log} from "./Logger"; -import Controller from "./Controller"; +import {log} from "../Logger"; +import Controller from "../Controller"; import {ParsedUrlQueryInput} from "querystring"; export default class Mail { diff --git a/src/auth/MailController.ts b/src/mail/MailController.ts similarity index 95% rename from src/auth/MailController.ts rename to src/mail/MailController.ts index a31560e..f3b7c64 100644 --- a/src/auth/MailController.ts +++ b/src/mail/MailController.ts @@ -1,6 +1,6 @@ import {Request, Response} from "express"; import Controller from "../Controller"; -import Mail from "../Mail"; +import Mail from "./Mail"; import NunjucksComponent from "../components/NunjucksComponent"; export default class MailController extends Controller { diff --git a/test/views/home.njk b/test/views/home.njk new file mode 100644 index 0000000..498d4f4 --- /dev/null +++ b/test/views/home.njk @@ -0,0 +1,7 @@ +{% extends 'layouts/base.njk' %} + +{% set title = app.name + ' - Home' %} + +{% block body %} +

{{ title }}

+{% endblock %} diff --git a/test/views/layouts/base.njk b/test/views/layouts/base.njk index 48bd2e5..93732f5 100644 --- a/test/views/layouts/base.njk +++ b/test/views/layouts/base.njk @@ -1,4 +1,5 @@ {% extends 'layouts/barebone.njk' %} +{% import 'macros.njk' as macros %} {% block _stylesheets %} {{ super() }} @@ -16,14 +17,18 @@