From caae753d748b73d30079ede321920d385dec2fb5 Mon Sep 17 00:00:00 2001 From: Alice Gaudon Date: Tue, 23 Feb 2021 17:42:25 +0100 Subject: [PATCH] Allow users to change their username every configurable period of time Closes #22 --- config/default.json5 | 5 +- src/TestApp.ts | 2 + src/auth/AccountController.ts | 40 ++++++++- src/auth/AuthGuard.ts | 2 +- src/auth/magic_link/MagicLinkController.ts | 9 +- .../AddNameChangedAtToUsersMigration.ts | 12 +++ src/auth/models/User.ts | 6 +- src/auth/models/UserNameComponent.ts | 34 ++++++- src/auth/password/PasswordAuthMethod.ts | 4 +- test/Authentication.test.ts | 89 ++++++++++++++++++- views/auth/{ => account}/account.njk | 4 + views/auth/account/name_panel.njk | 16 ++++ views/auth/{ => account}/password_panel.njk | 0 13 files changed, 209 insertions(+), 14 deletions(-) create mode 100644 src/auth/migrations/AddNameChangedAtToUsersMigration.ts rename views/auth/{ => account}/account.njk (97%) create mode 100644 views/auth/account/name_panel.njk rename views/auth/{ => account}/password_panel.njk (100%) diff --git a/config/default.json5 b/config/default.json5 index a1c8c4d..0e62cad 100644 --- a/config/default.json5 +++ b/config/default.json5 @@ -50,5 +50,8 @@ magic_link: { validity_period: 20, }, - approval_mode: false, + auth: { + approval_mode: false, // Registered accounts need to be approved by an administrator + name_change_wait_period: 2592000000, // 30 days + }, } diff --git a/src/TestApp.ts b/src/TestApp.ts index ee5253c..dd2333a 100644 --- a/src/TestApp.ts +++ b/src/TestApp.ts @@ -31,6 +31,7 @@ import MakeMagicLinksSessionNotUniqueMigration from "./auth/magic_link/MakeMagic import AddUsedToMagicLinksMigration from "./auth/magic_link/AddUsedToMagicLinksMigration"; import packageJson = require('./package.json'); import PreviousUrlComponent from "./components/PreviousUrlComponent"; +import AddNameChangedAtToUsersMigration from "./auth/migrations/AddNameChangedAtToUsersMigration"; export const MIGRATIONS = [ CreateMigrationsTable, @@ -40,6 +41,7 @@ export const MIGRATIONS = [ AddNameToUsersMigration, MakeMagicLinksSessionNotUniqueMigration, AddUsedToMagicLinksMigration, + AddNameChangedAtToUsersMigration, ]; export default class TestApp extends Application { diff --git a/src/auth/AccountController.ts b/src/auth/AccountController.ts index 851c0ac..a3f7c15 100644 --- a/src/auth/AccountController.ts +++ b/src/auth/AccountController.ts @@ -12,6 +12,8 @@ import MagicLinkController from "./magic_link/MagicLinkController"; import {MailTemplate} from "../mail/Mail"; import {ADD_EMAIL_MAIL_TEMPLATE, REMOVE_PASSWORD_MAIL_TEMPLATE} from "../Mails"; import AuthMagicLinkActionType from "./magic_link/AuthMagicLinkActionType"; +import UserNameComponent from "./models/UserNameComponent"; +import Time from "../Time"; export default class AccountController extends Controller { @@ -29,6 +31,10 @@ export default class AccountController extends Controller { public routes(): void { this.get('/', this.getAccount, 'account', RequireAuthMiddleware); + if (ModelFactory.get(User).hasComponent(UserNameComponent)) { + this.post('/change-name', this.postChangeName, 'change-name', RequireAuthMiddleware); + } + if (ModelFactory.get(User).hasComponent(UserPasswordComponent)) { this.post('/change-password', this.postChangePassword, 'change-password', RequireAuthMiddleware); this.post('/remove-password', this.postRemovePassword, 'remove-password', RequireAuthMiddleware); @@ -43,15 +49,47 @@ export default class AccountController extends Controller { const user = req.as(RequireAuthMiddleware).getUser(); const passwordComponent = user.asOptional(UserPasswordComponent); - res.render('auth/account', { + + const nameComponent = user.asOptional(UserNameComponent); + const nameChangeWaitPeriod = config.get('auth.name_change_wait_period'); + const nameChangedAt = nameComponent?.getNameChangedAt()?.getTime() || Date.now(); + const nameChangeRemainingTime = new Date(nameChangedAt + nameChangeWaitPeriod); + + res.render('auth/account/account', { main_email: await user.mainEmail.get(), emails: await user.emails.get(), display_email_warning: config.get('app.display_email_warning'), has_password_component: !!passwordComponent, has_password: passwordComponent?.hasPassword(), + has_name_component: !!nameComponent, + name_change_wait_period: Time.humanizeDuration(nameChangeWaitPeriod, false, true), + can_change_name: nameComponent?.canChangeName(), + can_change_name_in: Time.humanizeTimeTo(nameChangeRemainingTime), }); } + protected async postChangeName(req: Request, res: Response): Promise { + await Validator.validate({ + 'name': new Validator().defined(), + }, req.body); + + const user = req.as(RequireAuthMiddleware).getUser(); + const userNameComponent = user.as(UserNameComponent); + + if (!userNameComponent.setName(req.body.name)) { + const nameChangedAt = userNameComponent.getNameChangedAt()?.getTime() || Date.now(); + const nameChangeWaitPeriod = config.get('auth.name_change_wait_period'); + req.flash('error', `Your can't change your name until ${new Date(nameChangedAt + nameChangeWaitPeriod)}.`); + res.redirect(Controller.route('account')); + return; + } + + await user.save(); + + req.flash('success', `Your name was successfully changed to ${req.body.name}.`); + res.redirect(Controller.route('account')); + } + protected async postChangePassword(req: Request, res: Response): Promise { const validationMap = { 'new_password': new Validator().defined(), diff --git a/src/auth/AuthGuard.ts b/src/auth/AuthGuard.ts index 2742f62..cdce13b 100644 --- a/src/auth/AuthGuard.ts +++ b/src/auth/AuthGuard.ts @@ -141,7 +141,7 @@ export default class AuthGuard { if (!user.isApproved()) { await new Mail(this.app.as(NunjucksComponent).getEnvironment(), PENDING_ACCOUNT_REVIEW_MAIL_TEMPLATE, { - username: user.asOptional(UserNameComponent)?.name || + username: user.asOptional(UserNameComponent)?.getName() || (await user.mainEmail.get())?.getOrFail('email') || 'Could not find an identifier', link: config.get('public_url') + Controller.route('accounts-approval'), diff --git a/src/auth/magic_link/MagicLinkController.ts b/src/auth/magic_link/MagicLinkController.ts index 2aca47c..402bd3b 100644 --- a/src/auth/magic_link/MagicLinkController.ts +++ b/src/auth/magic_link/MagicLinkController.ts @@ -65,7 +65,12 @@ export default class MagicLinkController extends Controll return await req.as(AuthMiddleware).getAuthGuard().authenticateOrRegister( session, magicLink, !!session.wantsSessionPersistence, undefined, async (connection, user) => { const userNameComponent = user.asOptional(UserNameComponent); - if (userNameComponent) userNameComponent.name = magicLink.as(MagicLinkUserNameComponent).username; + const magicLinkUserNameComponent = magicLink.asOptional(MagicLinkUserNameComponent); + + if (userNameComponent && magicLinkUserNameComponent?.username) { + userNameComponent.setName(magicLinkUserNameComponent.username); + } + return []; }, async (connection, user) => { const callbacks: RegisterCallback[] = []; @@ -192,7 +197,7 @@ export default class MagicLinkController extends Controll if (!res.headersSent && user) { // Auth success - const name = user.asOptional(UserNameComponent)?.name; + const name = user.asOptional(UserNameComponent)?.getName(); req.flash('success', `Authentication success. Welcome${name ? `, ${name}` : ''}`); res.redirect(req.getIntendedUrl() || Controller.route('home')); } diff --git a/src/auth/migrations/AddNameChangedAtToUsersMigration.ts b/src/auth/migrations/AddNameChangedAtToUsersMigration.ts new file mode 100644 index 0000000..c83d6f1 --- /dev/null +++ b/src/auth/migrations/AddNameChangedAtToUsersMigration.ts @@ -0,0 +1,12 @@ +import Migration from "../../db/Migration"; + +export default class AddNameChangedAtToUsersMigration extends Migration { + public async install(): Promise { + await this.query(`ALTER TABLE users + ADD COLUMN name_changed_at DATETIME`); + } + + public async rollback(): Promise { + await this.query('ALTER TABLE users DROP COLUMN IF EXISTS name_changed_at'); + } +} diff --git a/src/auth/models/User.ts b/src/auth/models/User.ts index a08dc69..09c22f7 100644 --- a/src/auth/models/User.ts +++ b/src/auth/models/User.ts @@ -9,7 +9,7 @@ import UserNameComponent from "./UserNameComponent"; export default class User extends Model { public static isApprovalMode(): boolean { - return config.get('approval_mode') && + return config.get('auth.approval_mode') && MysqlConnectionManager.hasMigration(AddApprovedFieldToUsersTableMigration); } @@ -42,10 +42,10 @@ export default class User extends Model { protected getPersonalInfoFields(): { name: string, value: string }[] { const fields: { name: string, value: string }[] = []; const nameComponent = this.asOptional(UserNameComponent); - if (nameComponent && nameComponent.name) { + if (nameComponent && nameComponent.hasName()) { fields.push({ name: 'Name', - value: nameComponent.name, + value: nameComponent.getName(), }); } return fields; diff --git a/src/auth/models/UserNameComponent.ts b/src/auth/models/UserNameComponent.ts index 930b794..9440329 100644 --- a/src/auth/models/UserNameComponent.ts +++ b/src/auth/models/UserNameComponent.ts @@ -1,12 +1,44 @@ import ModelComponent from "../../db/ModelComponent"; import User from "../models/User"; +import config from "config"; export const USERNAME_REGEXP = /^[0-9a-z_-]+$/; export default class UserNameComponent extends ModelComponent { - public name?: string = undefined; + private name?: string = undefined; + private name_changed_at?: Date | null = undefined; public init(): void { this.setValidation('name').defined().between(3, 64).regexp(USERNAME_REGEXP).unique(this._model); } + + public hasName(): boolean { + return !!this.name; + } + + public getName(): string { + if (!this.name) throw new Error('User has no name.'); + return this.name; + } + + public setName(newName: string): boolean { + if (!this.canChangeName()) return false; + + this.name = newName; + this.name_changed_at = new Date(); + return true; + } + + public getNameChangedAt(): Date | null { + return this.name_changed_at || null; + } + + public forgetNameChangeDate(): void { + this.name_changed_at = null; + } + + public canChangeName(): boolean { + return !this.name_changed_at || + Date.now() - this.name_changed_at.getTime() >= config.get('auth.name_change_wait_period'); + } } diff --git a/src/auth/password/PasswordAuthMethod.ts b/src/auth/password/PasswordAuthMethod.ts index 27bd85e..6372c2e 100644 --- a/src/auth/password/PasswordAuthMethod.ts +++ b/src/auth/password/PasswordAuthMethod.ts @@ -113,7 +113,7 @@ export default class PasswordAuthMethod implements AuthMethod await user.as(UserPasswordComponent).setPassword(req.body.password); // Username - user.as(UserNameComponent).name = req.body.identifier; + user.as(UserNameComponent).setName(req.body.identifier); return callbacks; }, async (connection, user) => { @@ -132,7 +132,7 @@ export default class PasswordAuthMethod implements AuthMethod const user = await passwordAuthProof.getResource(); - req.flash('success', `Your account was successfully created! Welcome, ${user?.as(UserNameComponent).name}.`); + req.flash('success', `Your account was successfully created! Welcome, ${user?.as(UserNameComponent).getName()}.`); res.redirect(req.getIntendedUrl() || Controller.route('home')); } diff --git a/test/Authentication.test.ts b/test/Authentication.test.ts index 293f322..fff1ef0 100644 --- a/test/Authentication.test.ts +++ b/test/Authentication.test.ts @@ -72,7 +72,7 @@ describe('Register with username and password (password)', () => { .first(); expect(user).toBeDefined(); - expect(user?.as(UserNameComponent).name).toStrictEqual('entrapta'); + expect(user?.as(UserNameComponent).getName()).toStrictEqual('entrapta'); await expect(user?.as(UserPasswordComponent).verifyPassword('darla_is_cute')).resolves.toStrictEqual(true); }); @@ -186,7 +186,7 @@ describe('Register with email (magic_link)', () => { expect(email).toBeDefined(); expect(email?.email).toStrictEqual('glimmer@example.org'); - expect(user?.as(UserNameComponent).name).toStrictEqual('glimmer'); + expect(user?.as(UserNameComponent).getName()).toStrictEqual('glimmer'); await expect(user?.as(UserPasswordComponent).verifyPassword('')).resolves.toStrictEqual(false); }); @@ -575,7 +575,7 @@ describe('Authenticate with email and password (password)', () => { expect(email).toBeDefined(); expect(email?.email).toStrictEqual('double-trouble@example.org'); - expect(user?.as(UserNameComponent).name).toStrictEqual('double-trouble'); + expect(user?.as(UserNameComponent).getName()).toStrictEqual('double-trouble'); await expect(user?.as(UserPasswordComponent).verifyPassword('trick-or-treat')).resolves.toStrictEqual(true); }); @@ -871,6 +871,89 @@ describe('Change password', () => { }); }); +describe('Change username', () => { + let cookies: string[], csrf: string; + test('Prepare user', async () => { + const res = await agent.get('/csrf').expect(200); + cookies = res.get('Set-Cookie'); + csrf = res.text; + + await agent.post('/auth/register') + .set('Cookie', cookies) + .send({ + csrf: csrf, + auth_method: 'password', + identifier: 'richard', + password: 'a_very_strong_password', + password_confirmation: 'a_very_strong_password', + terms: 'on', + }) + .expect(302) + .expect('Location', '/'); + }); + + test('Initial value of name_changed_at should be the same as created_at', async () => { + const user = await User.select().where('name', 'richard').first(); + expect(user).toBeDefined(); + expect(user?.as(UserNameComponent).getNameChangedAt()?.getTime()).toBe(user?.created_at?.getTime()); + }); + + test('Cannot change username just after registration', async () => { + expect(await User.select().where('name', 'richard').count()).toBe(1); + + await agent.post('/account/change-name') + .set('Cookie', cookies) + .send({ + csrf: csrf, + name: 'robert', + }) + .expect(302) + .expect('Location', '/account/'); + + expect(await User.select().where('name', 'richard').count()).toBe(1); + }); + + test('Can change username after hold period', async () => { + // Set last username change to older date + const user = await User.select().where('name', 'richard').first(); + if (user) { + user.as(UserNameComponent).forgetNameChangeDate(); + await user.save(); + } + + expect(await User.select().where('name', 'richard').count()).toBe(1); + expect(await User.select().where('name', 'robert').count()).toBe(0); + + await agent.post('/account/change-name') + .set('Cookie', cookies) + .send({ + csrf: csrf, + name: 'robert', + }) + .expect(302) + .expect('Location', '/account/'); + + expect(await User.select().where('name', 'richard').count()).toBe(0); + expect(await User.select().where('name', 'robert').count()).toBe(1); + }); + + test('Cannot change username just after changing username', async () => { + expect(await User.select().where('name', 'robert').count()).toBe(1); + expect(await User.select().where('name', 'bebert').count()).toBe(0); + + await agent.post('/account/change-name') + .set('Cookie', cookies) + .send({ + csrf: csrf, + name: 'bebert', + }) + .expect(302) + .expect('Location', '/account/'); + + expect(await User.select().where('name', 'robert').count()).toBe(1); + expect(await User.select().where('name', 'bebert').count()).toBe(0); + }); +}); describe('Manage email addresses', () => { diff --git a/views/auth/account.njk b/views/auth/account/account.njk similarity index 97% rename from views/auth/account.njk rename to views/auth/account/account.njk index 6f4e633..c4f55dc 100644 --- a/views/auth/account.njk +++ b/views/auth/account/account.njk @@ -22,6 +22,10 @@ {% endif %} + {% if has_name_component %} + {% include './name_panel.njk' %} + {% endif %} + {% if has_password_component %} {% include './password_panel.njk' %} {% endif %} diff --git a/views/auth/account/name_panel.njk b/views/auth/account/name_panel.njk new file mode 100644 index 0000000..d2a5cdf --- /dev/null +++ b/views/auth/account/name_panel.njk @@ -0,0 +1,16 @@ +
+

Change name

+ {% if can_change_name %} +
+ {{ macros.field(_locals, 'text', 'name', null, 'New name', null, 'required') }} + + {{ macros.field(_locals, 'checkbox', 'terms', null, 'I understand that I can only change my name once every ' + name_change_wait_period, '', 'required') }} + + + + {{ macros.csrf(getCsrfToken) }} +
+ {% else %} + {{ macros.message('info', 'You can change your name in ' + can_change_name_in) }} + {% endif %} +
diff --git a/views/auth/password_panel.njk b/views/auth/account/password_panel.njk similarity index 100% rename from views/auth/password_panel.njk rename to views/auth/account/password_panel.njk