From 24de7321673f9f0a0477e87d3556486171369321 Mon Sep 17 00:00:00 2001 From: Alice Gaudon Date: Fri, 24 Jul 2020 13:00:20 +0200 Subject: [PATCH] Improve approval mode component security, reliability and usage --- src/auth/AuthGuard.ts | 8 +++--- src/helpers/BackendController.ts | 30 ++++++++++++++------- views/backend/accounts_approval.njk | 28 +++++++++++++++---- views/mails/pending_account_review.mjml.njk | 4 +-- 4 files changed, 51 insertions(+), 19 deletions(-) diff --git a/src/auth/AuthGuard.ts b/src/auth/AuthGuard.ts index 06bf53a..cf87b49 100644 --- a/src/auth/AuthGuard.ts +++ b/src/auth/AuthGuard.ts @@ -53,7 +53,7 @@ export default abstract class AuthGuard

{ } if (user) { - if (User.isApprovalMode()) { + if (!user!.isApproved()) { await new Mail(PENDING_ACCOUNT_REVIEW_MAIL_TEMPLATE, { username: user!.name, link: config.get('base_url') + Controller.route('accounts-approval'), @@ -66,7 +66,9 @@ export default abstract class AuthGuard

{ throw new UserAlreadyExistsAuthError(await proof.getEmail()); } - if (!user.isApproved()) throw new PendingApprovalAuthError(); + if (!user.isApproved()) { + throw new PendingApprovalAuthError(); + } session.auth_id = user.id; } @@ -131,4 +133,4 @@ export class PendingApprovalAuthError extends AuthError { constructor() { super(`User is not approved.`); } -} \ No newline at end of file +} diff --git a/src/helpers/BackendController.ts b/src/helpers/BackendController.ts index a421f5c..775e364 100644 --- a/src/helpers/BackendController.ts +++ b/src/helpers/BackendController.ts @@ -3,9 +3,10 @@ import Controller from "../Controller"; import {REQUIRE_ADMIN_MIDDLEWARE, REQUIRE_AUTH_MIDDLEWARE} from "../auth/AuthComponent"; import User from "../auth/models/User"; import {Request, Response} from "express"; -import {NotFoundHttpError} from "../HttpError"; +import {BadRequestError, NotFoundHttpError} from "../HttpError"; import Mail from "../Mail"; import {ACCOUNT_REVIEW_NOTICE_MAIL_TEMPLATE} from "../Mails"; +import UserEmail from "../auth/models/UserEmail"; export default class BackendController extends Controller { getRoutesPrefix(): string { @@ -16,8 +17,8 @@ export default class BackendController extends Controller { this.get('/', this.getIndex, 'backend', REQUIRE_AUTH_MIDDLEWARE, REQUIRE_ADMIN_MIDDLEWARE); if (User.isApprovalMode()) { this.get('/accounts-approval', this.getAccountApproval, 'accounts-approval', REQUIRE_AUTH_MIDDLEWARE, REQUIRE_ADMIN_MIDDLEWARE); - this.post('/accounts-approval/approve/:id', this.postApproveAccount, 'approve-account', REQUIRE_AUTH_MIDDLEWARE, REQUIRE_ADMIN_MIDDLEWARE); - this.post('/accounts-approval/reject/:id', this.postRejectAccount, 'reject-account', REQUIRE_AUTH_MIDDLEWARE, REQUIRE_ADMIN_MIDDLEWARE); + this.post('/accounts-approval/approve', this.postApproveAccount, 'approve-account', REQUIRE_AUTH_MIDDLEWARE, REQUIRE_ADMIN_MIDDLEWARE); + this.post('/accounts-approval/reject', this.postRejectAccount, 'reject-account', REQUIRE_AUTH_MIDDLEWARE, REQUIRE_ADMIN_MIDDLEWARE); } } @@ -36,9 +37,7 @@ export default class BackendController extends Controller { } public async postApproveAccount(req: Request, res: Response): Promise { - const account = await User.select().where('id', req.params.id).with('mainEmail').first(); - if (!account) throw new NotFoundHttpError('User', req.url); - const email = await account.mainEmail.get(); + const {account, email} = await this.accountRequest(req); account.approved = true; await account.save(); @@ -53,9 +52,7 @@ export default class BackendController extends Controller { } public async postRejectAccount(req: Request, res: Response): Promise { - const account = await User.select().where('id', req.params.id).with('mainEmail').first(); - if (!account) throw new NotFoundHttpError('User', req.url); - const email = await account.mainEmail.get(); + const {account, email} = await this.accountRequest(req); await account.delete(); @@ -66,4 +63,19 @@ export default class BackendController extends Controller { req.flash('success', `Account successfully deleted.`); res.redirectBack(Controller.route('accounts-approval')); } + + private async accountRequest(req: Request): Promise<{ + account: User, + email: UserEmail, + }> { + if (!req.body.user_id) throw new BadRequestError('Missing user_id field', 'Check your form', req.url); + const account = await User.select().where('id', req.body.user_id).with('mainEmail').first(); + if (!account) throw new NotFoundHttpError('User', req.url); + const email = await account.mainEmail.get(); + + return { + account: account, + email: email!, + }; + } } \ No newline at end of file diff --git a/views/backend/accounts_approval.njk b/views/backend/accounts_approval.njk index 3f72fe6..1f21e45 100644 --- a/views/backend/accounts_approval.njk +++ b/views/backend/accounts_approval.njk @@ -25,12 +25,18 @@ {{ user.created_at.toISOString() }}

- Approve +
+ + + {{ macros.csrf(getCSRFToken) }} +
- Reject +
+ + + {{ macros.csrf(getCSRFToken) }} +
@@ -38,4 +44,16 @@ + + {% endblock %} \ No newline at end of file diff --git a/views/mails/pending_account_review.mjml.njk b/views/mails/pending_account_review.mjml.njk index b79dea1..0687756 100644 --- a/views/mails/pending_account_review.mjml.njk +++ b/views/mails/pending_account_review.mjml.njk @@ -8,11 +8,11 @@ A new user is pending review on {{ app.name }}. - +
Username: {{ username }}
- Finalize my account registration + Go to account reviews {% endblock %}