From cf7e9a0ea4c5ce12e3ddd88879463b1cb104dbbe Mon Sep 17 00:00:00 2001 From: Pascal Garber Date: Thu, 9 Nov 2023 19:27:27 +0100 Subject: [PATCH] feat(MFA): Only allow to disable 2FA with current token --- src/api/controllers/AuthController.ts | 10 +++++-- src/api/controllers/ContactController.ts | 22 ++++++++++----- src/api/data/ContactData/interface.ts | 16 ++++++++--- src/api/errors/ForbiddenError.ts | 16 +++++++++++ src/core/lib/passport.ts | 3 +- src/core/services/ContactMfaService.ts | 36 ++++++++++++++++++++++-- 6 files changed, 86 insertions(+), 17 deletions(-) create mode 100644 src/api/errors/ForbiddenError.ts diff --git a/src/api/controllers/AuthController.ts b/src/api/controllers/AuthController.ts index a5da05625..270b609d4 100644 --- a/src/api/controllers/AuthController.ts +++ b/src/api/controllers/AuthController.ts @@ -69,7 +69,12 @@ export class AuthController { ) => { // Forward HTTP errors if (err) { - if (err instanceof HttpError || (err as UnauthorizedError).code) { + if (err instanceof HttpError) { + // Passport errors only have a `message` property, so we handle the message as code + if (err instanceof UnauthorizedError) { + err.code ||= err.message || LOGIN_CODES.LOGIN_FAILED; + } + return reject(err); } } @@ -78,7 +83,8 @@ export class AuthController { if (err || !user) { return reject( new UnauthorizedError({ - code: info?.message || LOGIN_CODES.LOGIN_FAILED + code: info?.message || LOGIN_CODES.LOGIN_FAILED, + message: info?.message || LOGIN_CODES.LOGIN_FAILED }) ); } diff --git a/src/api/controllers/ContactController.ts b/src/api/controllers/ContactController.ts index 28ba6c7f9..8048a3c2d 100644 --- a/src/api/controllers/ContactController.ts +++ b/src/api/controllers/ContactController.ts @@ -20,8 +20,7 @@ import { Post, Put, QueryParams, - Res, - UnauthorizedError + Res } from "routing-controllers"; import { getRepository } from "typeorm"; @@ -43,9 +42,11 @@ import JoinFlow from "@models/JoinFlow"; import Payment from "@models/Payment"; import { UUIDParam } from "@api/data"; +import { UnauthorizedError } from "@api/errors/UnauthorizedError"; import { convertContactToData, CreateContactData, + DeleteContactMfaData, fetchPaginatedContacts, GetContactData, GetContactQuery, @@ -76,6 +77,7 @@ import { GetExportQuery } from "@api/data/PaginatedData"; import { GetPaymentData, GetPaymentsQuery } from "@api/data/PaymentData"; +import { LOGIN_CODES } from "@api/data/ContactData/interface"; import PartialBody from "@api/decorators/PartialBody"; import CantUpdateContribution from "@api/errors/CantUpdateContribution"; @@ -274,7 +276,7 @@ export class ContactController { /** * Get contact multi factor authentication if exists - * @param target The target contact (which is the current user or admin) + * @param target The target contact */ @Get("/:id/mfa") async getContactMfa( @@ -286,7 +288,7 @@ export class ContactController { /** * Create contact multi factor authentication - * @param target The target contact (which is the current user or admin) + * @param target The target contact * @param data The data to create the contact multi factor authentication */ @OnUndefined(201) @@ -300,12 +302,18 @@ export class ContactController { /** * Delete contact multi factor authentication - * @param target The target contact (which is the current user or admin) + * @param target The target contact + * @param data The data to delete the contact multi factor authentication + * @param id The contact id */ @OnUndefined(201) @Delete("/:id/mfa") - async deleteContactMfa(@TargetUser() target: Contact): Promise { - await ContactMfaService.delete(target); + async deleteContactMfa( + @TargetUser() target: Contact, + @Body() data: DeleteContactMfaData, + @Params() { id }: { id: string } + ): Promise { + await ContactMfaService.delete(target, id, data); } @OnUndefined(204) diff --git a/src/api/data/ContactData/interface.ts b/src/api/data/ContactData/interface.ts index b6f634af2..2bd09ba51 100644 --- a/src/api/data/ContactData/interface.ts +++ b/src/api/data/ContactData/interface.ts @@ -245,7 +245,8 @@ export enum LOGIN_CODES { LOGIN_FAILED = "login-failed", REQUIRES_2FA = "requires-2fa", UNSUPPORTED_2FA = "unsupported-2fa", - INVALID_TOKEN = "invalid-token" + INVALID_TOKEN = "invalid-token", + MISSING_TOKEN = "missing-token" } export interface PassportLoginInfo { @@ -269,9 +270,6 @@ export type PassportLocalDoneCallback = ( * TODO: Move to common */ interface ContactMfaData { - secret?: string; - /** The code from the authenticator app */ - token?: string; type: ContactMfaType; } @@ -297,3 +295,13 @@ export class CreateContactMfaData implements ContactMfaData { @IsString() type!: ContactMfaType; } + +export class DeleteContactMfaData implements ContactMfaData { + /** The code from the authenticator app, only required by the user itself, not by the admin */ + @IsString() + @IsOptional() + token?: string; + + @IsString() + type!: ContactMfaType; +} diff --git a/src/api/errors/ForbiddenError.ts b/src/api/errors/ForbiddenError.ts new file mode 100644 index 000000000..fdfb4538b --- /dev/null +++ b/src/api/errors/ForbiddenError.ts @@ -0,0 +1,16 @@ +import { ForbiddenError as _ForbiddenError } from "routing-controllers"; + +/** + * ForbiddenError with optional code + */ +export class ForbiddenError extends _ForbiddenError { + code?: string | undefined; + + constructor({ message, code }: { message?: string; code?: string } = {}) { + super(message); + Object.setPrototypeOf(this, ForbiddenError.prototype); + this.code = code; + } +} + +export default ForbiddenError; diff --git a/src/core/lib/passport.ts b/src/core/lib/passport.ts index 0b0218070..d7c46f970 100644 --- a/src/core/lib/passport.ts +++ b/src/core/lib/passport.ts @@ -17,7 +17,8 @@ import { LoginData } from "@api/controllers/AuthController"; import { ContactMfaType, LOGIN_CODES, - PassportLocalDoneCallback + PassportLocalDoneCallback, + PassportLoginInfo } from "@api/data/ContactData/interface"; import { UnauthorizedError } from "@api/errors/UnauthorizedError"; diff --git a/src/core/services/ContactMfaService.ts b/src/core/services/ContactMfaService.ts index 39ef8d9e3..bed445fc0 100644 --- a/src/core/services/ContactMfaService.ts +++ b/src/core/services/ContactMfaService.ts @@ -3,10 +3,15 @@ import { BadRequestError, NotFoundError } from "routing-controllers"; import Contact from "@models/Contact"; import { ContactMfa, ContactMfaSecure } from "@models/ContactMfa"; + import { validateTotpToken } from "@core/utils/auth"; -import { LOGIN_CODES } from "@api/data/ContactData/interface"; -import { CreateContactMfaData } from "@api/data/ContactData/interface"; +import { LOGIN_CODES } from "@api/data/ContactData/interface"; +import { + CreateContactMfaData, + DeleteContactMfaData +} from "@api/data/ContactData/interface"; +import { ForbiddenError } from "@api/errors/ForbiddenError"; /** * Contact multi factor authentication service @@ -56,15 +61,40 @@ class ContactMfaService { /** * Delete contact MFA + * + * ### ATTENTION + * If the id is `'me'` we check if the token is valid, otherwise the user must be an admin, this must be checked before calling this method. + * E.g. with the `@TargetUser()` or `@Authorized()` decorators. + * * @param contact The contact + * @param id The request contact ID (we check if the id is 'me' or the contact ID) + * @param data The MFA type and the token (if the user is not an admin) */ - async delete(contact: Contact) { + async delete(contact: Contact, id: string, data: DeleteContactMfaData) { const mfa = await this.get(contact); if (!mfa) { throw new NotFoundError("Contact has no MFA"); } + // If the id is 'me' we check if the token is valid + if (id === "me") { + if (!data.token) { + throw new ForbiddenError({ + code: LOGIN_CODES.MISSING_TOKEN, + message: + "The contact itself needs to enter the old code to delete its MFA" + }); + } + const tokenValidation = await this.checkToken(contact, data.token, 2); + if (!tokenValidation.isValid) { + throw new ForbiddenError({ + code: LOGIN_CODES.INVALID_TOKEN, + message: "Invalid token" + }); + } + } + await getRepository(ContactMfa).delete(mfa.id); }