From 52831ba9ef4cf9d223ad7d3eeb6d7dd5dbf607bc Mon Sep 17 00:00:00 2001 From: David Barroso Date: Sat, 3 Feb 2024 13:54:40 +0100 Subject: [PATCH 1/7] feat: added AUTH_REQUIRE_ELEVATED_CLAIM to require elevated permissions for certain actions --- docs/environment-variables.md | 1 + src/errors.ts | 4 ++++ src/middleware/auth.ts | 42 +++++++++++++++++++++++++++++------ src/routes/mfa/index.ts | 2 +- src/routes/pat/index.ts | 8 ++++++- src/routes/pat/pat.ts | 4 ---- src/routes/user/index.ts | 15 ++++++++----- src/utils/env.ts | 4 ++++ types/express-request.d.ts | 1 + 9 files changed, 63 insertions(+), 18 deletions(-) diff --git a/docs/environment-variables.md b/docs/environment-variables.md index c40cbc7c8..345e25c11 100644 --- a/docs/environment-variables.md +++ b/docs/environment-variables.md @@ -56,6 +56,7 @@ | AUTH_WEBAUTHN_RP_ID | Relying party id. If not set `AUTH_CLIENT_URL` will be used as a default. | | | AUTH_WEBAUTHN_RP_ORIGINS | Array of URLs where the registration is permitted and should have occurred on. `AUTH_CLIENT_URL` will be automatically added to the list of origins if is set. | | | AUTH_WEBAUTHN_ATTESTATION_TIMEOUT | How long (in ms) the user can take to complete authentication. | `60000` (1 minute) | +| AUTH_REQUIRE_ELEVATED_CLAIM | Require x-hasura-auth-elevated claim to perform certain actions: create PATs, change email and/or password, enable/disable MFA and add security keys. If set to `recommended` the claim check is only performed if the user has a security key attached. If set to `required` the only action that won't require the claim is setting a security key for the first time. | `disabled` | # OAuth environment variables diff --git a/src/errors.ts b/src/errors.ts index 15188f3f0..a88e71569 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -131,6 +131,10 @@ export const ERRORS = asErrors({ status: StatusCodes.UNAUTHORIZED, message: 'User is not logged in', }, + 'elevated-claim-required': { + status: StatusCodes.FORBIDDEN, + message: 'Elevated claim is required', + }, 'forbidden-endpoint-in-production': { status: StatusCodes.BAD_REQUEST, message: 'This endpoint is only available on test environments', diff --git a/src/middleware/auth.ts b/src/middleware/auth.ts index bb765862d..860f85a00 100644 --- a/src/middleware/auth.ts +++ b/src/middleware/auth.ts @@ -1,6 +1,8 @@ import { RequestHandler } from 'express'; import { getPermissionVariables } from '@/utils'; import { sendError } from '@/errors'; +import { ENV } from '../utils/env'; +import { gqlSdk } from '@/utils'; export const authMiddleware: RequestHandler = async (req, _, next) => { try { @@ -11,6 +13,7 @@ export const authMiddleware: RequestHandler = async (req, _, next) => { userId: permissionVariables['user-id'], defaultRole: permissionVariables['default-role'], isAnonymous: permissionVariables['is-anonymous'] === true, + elevated: permissionVariables['auth-elevated'] === permissionVariables['user-id'], }; } catch (e) { req.auth = null; @@ -18,10 +21,35 @@ export const authMiddleware: RequestHandler = async (req, _, next) => { next(); }; -export const authenticationGate: RequestHandler = (req, res, next) => { - if (!req.auth) { - return sendError(res, 'unauthenticated-user'); - } else { - next(); - } -}; +export const authenticationGate = (checkElevatedPermissions: boolean, bypassIfNoKeys = false): RequestHandler => { + return async (req, res, next) => { + if (!req.auth) { + return sendError(res, 'unauthenticated-user'); + } + + if (!checkElevatedPermissions || + ENV.AUTH_REQUIRE_ELEVATED_CLAIM === 'disabled' || + !ENV.AUTH_WEBAUTHN_ENABLED) { + return next(); + } + + const auth = req.auth as RequestAuth; + const { authUserSecurityKeys } = await gqlSdk.getUserSecurityKeys({ + id: auth.userId, + }); + + if (authUserSecurityKeys.length === 0 && ENV.AUTH_REQUIRE_ELEVATED_CLAIM === 'recommended') { + return next(); + } + + if (authUserSecurityKeys.length === 0 && bypassIfNoKeys) { + return next(); + } + + if (!auth.elevated) { + return sendError(res, 'elevated-claim-required'); + } + + return next(); + }; +} diff --git a/src/routes/mfa/index.ts b/src/routes/mfa/index.ts index 48f2bc6cc..4bc8f3110 100644 --- a/src/routes/mfa/index.ts +++ b/src/routes/mfa/index.ts @@ -17,7 +17,7 @@ const router = Router(); */ router.get( '/mfa/totp/generate', - authenticationGate, + authenticationGate(false), aw(mfatotpGenerateHandler) ); diff --git a/src/routes/pat/index.ts b/src/routes/pat/index.ts index 6528eb4ac..ffbd16f47 100644 --- a/src/routes/pat/index.ts +++ b/src/routes/pat/index.ts @@ -2,6 +2,7 @@ import { asyncWrapper as aw } from '@/utils'; import { bodyValidator } from '@/validation'; import { Router } from 'express'; import { createPATHandler, createPATSchema } from './pat'; +import { authenticationGate } from '@/middleware/auth'; const router = Router(); @@ -14,7 +15,12 @@ const router = Router(); * @return {UnauthorizedError} 401 - Unauthenticated user or invalid token - application/json * @tags General */ -router.post('/pat', bodyValidator(createPATSchema), aw(createPATHandler)); +router.post( + '/pat', + authenticationGate(true), + bodyValidator(createPATSchema), + aw(createPATHandler), +); const patRouter = router; export { patRouter }; diff --git a/src/routes/pat/pat.ts b/src/routes/pat/pat.ts index 7be09bbde..a133d7e5c 100644 --- a/src/routes/pat/pat.ts +++ b/src/routes/pat/pat.ts @@ -16,10 +16,6 @@ export const createPATHandler: RequestHandler< {}, { metadata: object; expiresAt: Date } > = async (req, res) => { - if (!req.auth) { - return sendError(res, 'unauthenticated-user'); - } - try { const { userId } = req.auth as RequestAuth; diff --git a/src/routes/user/index.ts b/src/routes/user/index.ts index 248165089..1d7b8f94a 100644 --- a/src/routes/user/index.ts +++ b/src/routes/user/index.ts @@ -38,7 +38,7 @@ const router = Router(); * @security BearerAuth * @tags User management */ -router.get('/user', authenticationGate, aw(userHandler)); +router.get('/user', authenticationGate(false), aw(userHandler)); /** * POST /user/password/reset @@ -66,6 +66,7 @@ router.post( */ router.post( '/user/password', + authenticationGate(true), bodyValidator(userPasswordSchema), aw(userPasswordHandler) ); @@ -96,8 +97,8 @@ router.post( */ router.post( '/user/email/change', + authenticationGate(true), bodyValidator(userEmailChangeSchema), - authenticationGate, aw(userEmailChange) ); @@ -113,8 +114,8 @@ router.post( */ router.post( '/user/mfa', + authenticationGate(true), bodyValidator(userMfaSchema), - authenticationGate, aw(userMFAHandler) ); @@ -130,8 +131,8 @@ router.post( */ router.post( '/user/deanonymize', + authenticationGate(false), bodyValidator(userDeanonymizeSchema), - authenticationGate, aw(userDeanonymizeHandler) ); @@ -161,7 +162,11 @@ router.post( * @return {DisabledEndpointError} 404 - The feature is not activated - application/json * @tags User management */ -router.post('/user/webauthn/add', aw(addSecurityKeyHandler)); +router.post( + '/user/webauthn/add', + authenticationGate(true, true), + aw(addSecurityKeyHandler), +); // TODO add @return payload on success /** diff --git a/src/utils/env.ts b/src/utils/env.ts index 9f76627a9..70a8e0054 100644 --- a/src/utils/env.ts +++ b/src/utils/env.ts @@ -236,6 +236,10 @@ export const ENV = { return castBooleanEnv('AUTH_DISABLE_SIGNUP', false); }, + get AUTH_REQUIRE_ELEVATED_CLAIM() { + return castStringEnv('AUTH_REQUIRE_ELEVATED_CLAIM', 'disabled'); + } + // * See ../server.ts // get AUTH_SKIP_INIT() { // return castBooleanEnv('AUTH_SKIP_INIT', false); diff --git a/types/express-request.d.ts b/types/express-request.d.ts index 5c67f278b..372a7665f 100644 --- a/types/express-request.d.ts +++ b/types/express-request.d.ts @@ -2,6 +2,7 @@ interface RequestAuth { userId: string; defaultRole: string; isAnonymous: boolean; + elevated: boolean; } declare namespace Express { From f1b8855912eef84c8d1c61f3da69c5741c075ad5 Mon Sep 17 00:00:00 2001 From: David Barroso Date: Sat, 3 Feb 2024 14:21:23 +0100 Subject: [PATCH 2/7] asd --- src/middleware/auth.ts | 10 ++++++++-- src/routes/user/index.ts | 2 +- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/middleware/auth.ts b/src/middleware/auth.ts index 860f85a00..9f40c5ea6 100644 --- a/src/middleware/auth.ts +++ b/src/middleware/auth.ts @@ -21,7 +21,11 @@ export const authMiddleware: RequestHandler = async (req, _, next) => { next(); }; -export const authenticationGate = (checkElevatedPermissions: boolean, bypassIfNoKeys = false): RequestHandler => { +export const authenticationGate = ( + checkElevatedPermissions: boolean, + bypassIfNoKeys = false, + bypassFn: (req: any) => boolean = () => false, +): RequestHandler => { return async (req, res, next) => { if (!req.auth) { return sendError(res, 'unauthenticated-user'); @@ -29,7 +33,9 @@ export const authenticationGate = (checkElevatedPermissions: boolean, bypassIfNo if (!checkElevatedPermissions || ENV.AUTH_REQUIRE_ELEVATED_CLAIM === 'disabled' || - !ENV.AUTH_WEBAUTHN_ENABLED) { + !ENV.AUTH_WEBAUTHN_ENABLED || + bypassFn(req) + ) { return next(); } diff --git a/src/routes/user/index.ts b/src/routes/user/index.ts index 1d7b8f94a..fcd12b097 100644 --- a/src/routes/user/index.ts +++ b/src/routes/user/index.ts @@ -66,7 +66,7 @@ router.post( */ router.post( '/user/password', - authenticationGate(true), + authenticationGate(true, false, (req) => req.body.ticket !== undefined), bodyValidator(userPasswordSchema), aw(userPasswordHandler) ); From e680c6fd05617881f26e3b2348dcc91ec6fb5b60 Mon Sep 17 00:00:00 2001 From: David Barroso Date: Sat, 3 Feb 2024 15:19:26 +0100 Subject: [PATCH 3/7] asd --- .changeset/eight-brooms-explain.md | 5 +++++ src/middleware/auth.ts | 10 ++++------ src/routes/user/index.ts | 14 +++++++++----- 3 files changed, 18 insertions(+), 11 deletions(-) create mode 100644 .changeset/eight-brooms-explain.md diff --git a/.changeset/eight-brooms-explain.md b/.changeset/eight-brooms-explain.md new file mode 100644 index 000000000..75441fbf8 --- /dev/null +++ b/.changeset/eight-brooms-explain.md @@ -0,0 +1,5 @@ +--- +'hasura-auth': minor +--- + +feat: added AUTH_REQUIRE_ELEVATED_CLAIM to require elevated permissions for certain action diff --git a/src/middleware/auth.ts b/src/middleware/auth.ts index 9f40c5ea6..11708cae8 100644 --- a/src/middleware/auth.ts +++ b/src/middleware/auth.ts @@ -31,15 +31,17 @@ export const authenticationGate = ( return sendError(res, 'unauthenticated-user'); } + const auth = req.auth as RequestAuth; + if (!checkElevatedPermissions || ENV.AUTH_REQUIRE_ELEVATED_CLAIM === 'disabled' || !ENV.AUTH_WEBAUTHN_ENABLED || + auth.elevated || bypassFn(req) ) { return next(); } - const auth = req.auth as RequestAuth; const { authUserSecurityKeys } = await gqlSdk.getUserSecurityKeys({ id: auth.userId, }); @@ -52,10 +54,6 @@ export const authenticationGate = ( return next(); } - if (!auth.elevated) { - return sendError(res, 'elevated-claim-required'); - } - - return next(); + return sendError(res, 'elevated-claim-required'); }; } diff --git a/src/routes/user/index.ts b/src/routes/user/index.ts index fcd12b097..7f8cbd3ae 100644 --- a/src/routes/user/index.ts +++ b/src/routes/user/index.ts @@ -38,7 +38,11 @@ const router = Router(); * @security BearerAuth * @tags User management */ -router.get('/user', authenticationGate(false), aw(userHandler)); +router.get( + '/user', + authenticationGate(false), + aw(userHandler), +); /** * POST /user/password/reset @@ -66,8 +70,8 @@ router.post( */ router.post( '/user/password', - authenticationGate(true, false, (req) => req.body.ticket !== undefined), bodyValidator(userPasswordSchema), + authenticationGate(true, false, (req) => req.body.ticket !== undefined), aw(userPasswordHandler) ); @@ -97,8 +101,8 @@ router.post( */ router.post( '/user/email/change', - authenticationGate(true), bodyValidator(userEmailChangeSchema), + authenticationGate(true), aw(userEmailChange) ); @@ -114,8 +118,8 @@ router.post( */ router.post( '/user/mfa', - authenticationGate(true), bodyValidator(userMfaSchema), + authenticationGate(true), aw(userMFAHandler) ); @@ -131,8 +135,8 @@ router.post( */ router.post( '/user/deanonymize', - authenticationGate(false), bodyValidator(userDeanonymizeSchema), + authenticationGate(false), aw(userDeanonymizeHandler) ); From 573c2ec121f232239248de722a81f85c178b59b8 Mon Sep 17 00:00:00 2001 From: David Barroso Date: Sat, 3 Feb 2024 15:38:00 +0100 Subject: [PATCH 4/7] asd --- src/middleware/auth.ts | 30 +++++++++++++++++++----------- src/routes/user/index.ts | 2 +- src/routes/user/password.ts | 7 +++++++ 3 files changed, 27 insertions(+), 12 deletions(-) diff --git a/src/middleware/auth.ts b/src/middleware/auth.ts index 11708cae8..22f9a2fd6 100644 --- a/src/middleware/auth.ts +++ b/src/middleware/auth.ts @@ -42,18 +42,26 @@ export const authenticationGate = ( return next(); } - const { authUserSecurityKeys } = await gqlSdk.getUserSecurityKeys({ - id: auth.userId, - }); - - if (authUserSecurityKeys.length === 0 && ENV.AUTH_REQUIRE_ELEVATED_CLAIM === 'recommended') { - return next(); - } - - if (authUserSecurityKeys.length === 0 && bypassIfNoKeys) { - return next(); + if (await failsElevatedCheck(auth.userId, bypassIfNoKeys)) { + return sendError(res, 'elevated-claim-required'); } - return sendError(res, 'elevated-claim-required'); + return next(); }; } + +export const failsElevatedCheck = async (userId: string, bypassIfNoKeys = false) => { + const response = await gqlSdk.getUserSecurityKeys({ + id: userId, + }); + + if (response.authUserSecurityKeys.length === 0 && ENV.AUTH_REQUIRE_ELEVATED_CLAIM === 'recommended') { + return false; + } + + if (response.authUserSecurityKeys.length === 0 && bypassIfNoKeys) { + return false; + } + + return true; +}; diff --git a/src/routes/user/index.ts b/src/routes/user/index.ts index 7f8cbd3ae..3054d0913 100644 --- a/src/routes/user/index.ts +++ b/src/routes/user/index.ts @@ -71,7 +71,7 @@ router.post( router.post( '/user/password', bodyValidator(userPasswordSchema), - authenticationGate(true, false, (req) => req.body.ticket !== undefined), + // authenticationGate(true, false, (req) => req.body.ticket !== undefined), // this is done in the handler because the handler has an auhtenticated and unauthenticated mode............. aw(userPasswordHandler) ); diff --git a/src/routes/user/password.ts b/src/routes/user/password.ts index 661257325..dd4cb04d4 100644 --- a/src/routes/user/password.ts +++ b/src/routes/user/password.ts @@ -1,6 +1,8 @@ import { RequestHandler } from 'express'; import { ReasonPhrases } from 'http-status-codes'; +import { failsElevatedCheck } from '@/middleware/auth'; + import { gqlSdk, hashPassword, getUserByTicket } from '@/utils'; import { sendError } from '@/errors'; import { Joi, password } from '@/validation'; @@ -27,6 +29,11 @@ export const userPasswordHandler: RequestHandler< if (!req.auth?.userId) { return sendError(res, 'unauthenticated-user'); } + + if (await failsElevatedCheck(req.auth?.userId)) { + return sendError(res, 'elevated-claim-required'); + } + user = (await gqlSdk.user({ id: req.auth?.userId })).user; } From a0967ccfc79e18977e3e0b031b207b7baaace0ae Mon Sep 17 00:00:00 2001 From: David Barroso Date: Sat, 3 Feb 2024 15:45:48 +0100 Subject: [PATCH 5/7] asd --- src/middleware/auth.ts | 17 +++++++++-------- src/routes/user/password.ts | 2 +- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/middleware/auth.ts b/src/middleware/auth.ts index 22f9a2fd6..193543617 100644 --- a/src/middleware/auth.ts +++ b/src/middleware/auth.ts @@ -24,7 +24,6 @@ export const authMiddleware: RequestHandler = async (req, _, next) => { export const authenticationGate = ( checkElevatedPermissions: boolean, bypassIfNoKeys = false, - bypassFn: (req: any) => boolean = () => false, ): RequestHandler => { return async (req, res, next) => { if (!req.auth) { @@ -35,14 +34,12 @@ export const authenticationGate = ( if (!checkElevatedPermissions || ENV.AUTH_REQUIRE_ELEVATED_CLAIM === 'disabled' || - !ENV.AUTH_WEBAUTHN_ENABLED || - auth.elevated || - bypassFn(req) - ) { + !ENV.AUTH_WEBAUTHN_ENABLED + ) { return next(); } - if (await failsElevatedCheck(auth.userId, bypassIfNoKeys)) { + if (await failsElevatedCheck(auth, bypassIfNoKeys)) { return sendError(res, 'elevated-claim-required'); } @@ -50,9 +47,13 @@ export const authenticationGate = ( }; } -export const failsElevatedCheck = async (userId: string, bypassIfNoKeys = false) => { +export const failsElevatedCheck = async (auth: RequestAuth, bypassIfNoKeys = false) => { + if (auth.elevated) { + return false; + } + const response = await gqlSdk.getUserSecurityKeys({ - id: userId, + id: auth.userId, }); if (response.authUserSecurityKeys.length === 0 && ENV.AUTH_REQUIRE_ELEVATED_CLAIM === 'recommended') { diff --git a/src/routes/user/password.ts b/src/routes/user/password.ts index dd4cb04d4..529d7f58c 100644 --- a/src/routes/user/password.ts +++ b/src/routes/user/password.ts @@ -30,7 +30,7 @@ export const userPasswordHandler: RequestHandler< return sendError(res, 'unauthenticated-user'); } - if (await failsElevatedCheck(req.auth?.userId)) { + if (await failsElevatedCheck(req.auth)) { return sendError(res, 'elevated-claim-required'); } From 9618af0d74d60a9742f38409eea89a2c8b0de038 Mon Sep 17 00:00:00 2001 From: David Barroso Date: Sat, 3 Feb 2024 15:49:31 +0100 Subject: [PATCH 6/7] asd --- src/middleware/auth.ts | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/middleware/auth.ts b/src/middleware/auth.ts index 193543617..381b6b103 100644 --- a/src/middleware/auth.ts +++ b/src/middleware/auth.ts @@ -31,14 +31,6 @@ export const authenticationGate = ( } const auth = req.auth as RequestAuth; - - if (!checkElevatedPermissions || - ENV.AUTH_REQUIRE_ELEVATED_CLAIM === 'disabled' || - !ENV.AUTH_WEBAUTHN_ENABLED - ) { - return next(); - } - if (await failsElevatedCheck(auth, bypassIfNoKeys)) { return sendError(res, 'elevated-claim-required'); } @@ -48,6 +40,13 @@ export const authenticationGate = ( } export const failsElevatedCheck = async (auth: RequestAuth, bypassIfNoKeys = false) => { + if (!checkElevatedPermissions || + ENV.AUTH_REQUIRE_ELEVATED_CLAIM === 'disabled' || + !ENV.AUTH_WEBAUTHN_ENABLED + ) { + return false; + } + if (auth.elevated) { return false; } From c99d24b5fe224f4813663373ed4d777277c594ff Mon Sep 17 00:00:00 2001 From: David Barroso Date: Sat, 3 Feb 2024 15:51:22 +0100 Subject: [PATCH 7/7] asd --- src/middleware/auth.ts | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/middleware/auth.ts b/src/middleware/auth.ts index 381b6b103..707666b49 100644 --- a/src/middleware/auth.ts +++ b/src/middleware/auth.ts @@ -30,9 +30,11 @@ export const authenticationGate = ( return sendError(res, 'unauthenticated-user'); } - const auth = req.auth as RequestAuth; - if (await failsElevatedCheck(auth, bypassIfNoKeys)) { + if (checkElevatedPermissions) { + const auth = req.auth as RequestAuth; + if (await failsElevatedCheck(auth, bypassIfNoKeys)) { return sendError(res, 'elevated-claim-required'); + } } return next(); @@ -40,14 +42,7 @@ export const authenticationGate = ( } export const failsElevatedCheck = async (auth: RequestAuth, bypassIfNoKeys = false) => { - if (!checkElevatedPermissions || - ENV.AUTH_REQUIRE_ELEVATED_CLAIM === 'disabled' || - !ENV.AUTH_WEBAUTHN_ENABLED - ) { - return false; - } - - if (auth.elevated) { + if (ENV.AUTH_REQUIRE_ELEVATED_CLAIM === 'disabled' || !ENV.AUTH_WEBAUTHN_ENABLED || auth.elevated) { return false; }