From 5f5f882b2a2f262a4a226ba7c0cf3742844200b8 Mon Sep 17 00:00:00 2001 From: Evert Pot Date: Mon, 23 Dec 2024 00:18:06 -0500 Subject: [PATCH 01/10] Refactored password and totp login challenges into a strategy pattern. --- src/login/challenge/abstract.ts | 7 + src/login/challenge/password.ts | 33 +++++ src/login/challenge/totp.ts | 55 +++++++ src/login/error.ts | 45 ++++++ src/login/service.ts | 248 +++++++++----------------------- src/login/types.ts | 66 ++++++++- 6 files changed, 267 insertions(+), 187 deletions(-) create mode 100644 src/login/challenge/abstract.ts create mode 100644 src/login/challenge/password.ts create mode 100644 src/login/challenge/totp.ts create mode 100644 src/login/error.ts diff --git a/src/login/challenge/abstract.ts b/src/login/challenge/abstract.ts new file mode 100644 index 00000000..59240c2c --- /dev/null +++ b/src/login/challenge/abstract.ts @@ -0,0 +1,7 @@ +import { LoginChallengeContext } from '../types.js'; + +export abstract class AbstractLoginChallenge { + + abstract challenge(loginContext: LoginChallengeContext): Promise; + +} diff --git a/src/login/challenge/password.ts b/src/login/challenge/password.ts new file mode 100644 index 00000000..fa56c8f6 --- /dev/null +++ b/src/login/challenge/password.ts @@ -0,0 +1,33 @@ +import { AbstractLoginChallenge } from './abstract.js'; +import { LoginChallengeContext } from '../types.js'; +import { A12nLoginChallengeError } from '../error.js'; +import * as services from '../../services.js'; + +export class LoginChallengePassword extends AbstractLoginChallenge { + + async challenge(loginContext: LoginChallengeContext): Promise { + + if (loginContext.parameters.password === undefined) { + throw new A12nLoginChallengeError( + loginContext.session, + 'A username and password are required', + 'username_or_password_required', + ); + + } + + const { success, errorMessage } = await services.user.validateUserCredentials(loginContext.principal, loginContext.parameters.password, loginContext.log); + if (!success && errorMessage) { + throw new A12nLoginChallengeError( + loginContext.session, + errorMessage, + 'username_or_password_invalid', + ); + } + + loginContext.session.authFactorsPassed.push('password'); + loginContext.dirty = true; + + } + +} diff --git a/src/login/challenge/totp.ts b/src/login/challenge/totp.ts new file mode 100644 index 00000000..1268fedb --- /dev/null +++ b/src/login/challenge/totp.ts @@ -0,0 +1,55 @@ +import { AbstractLoginChallenge } from './abstract.js'; +import { LoginChallengeContext } from '../types.js'; +import { A12nLoginChallengeError } from '../error.js'; +import * as services from '../../services.js'; +import { InvalidGrant } from '../../oauth2/errors.js'; +import { getSetting } from '../../server-settings.js'; +export class LoginChallengeTotp extends AbstractLoginChallenge { + + async challenge(loginContext: LoginChallengeContext): Promise { + + const serverTotpMode = getSetting('totp'); + if (serverTotpMode === 'disabled') { + // Server-wide TOTP disabled. + loginContext.session.authFactorsPassed.push('totp'); + loginContext.dirty = true; + return; + } + const hasTotp = await services.mfaTotp.hasTotp(loginContext.principal); + if (!hasTotp) { + // Does this server require TOTP + if (serverTotpMode === 'required') { + throw new InvalidGrant('This server is configured to require TOTP, and this user does not have TOTP set up. Logging in is not possible for this user in its current state. Contact an administrator'); + } + // User didn't have TOTP so we just pass them + loginContext.session.authFactorsPassed.push('totp'); + loginContext.dirty = true; + return; + } + if (!loginContext.parameters.totp_code) { + // No TOTP code was provided + throw new A12nLoginChallengeError( + loginContext.session, + 'Please provide a TOTP code from the user\'s authenticator app.', + 'totp_required', + ); + } + if (!await services.mfaTotp.validateTotp(loginContext.principal, loginContext.parameters.totp_code)) { + loginContext.log('totp-failed'); + // TOTP code was incorrect + throw new A12nLoginChallengeError( + loginContext.session, + 'Incorrect TOTP code. Make sure your system clock is set to the correct time and try again', + 'totp_invalid', + ); + } else { + loginContext.log('totp-success'); + }; + + // TOTP check successful! + loginContext.session.authFactorsPassed.push('totp'); + loginContext.dirty = true; + + } + +} diff --git a/src/login/error.ts b/src/login/error.ts new file mode 100644 index 00000000..6bd5eb15 --- /dev/null +++ b/src/login/error.ts @@ -0,0 +1,45 @@ +import { OAuth2Error } from '../oauth2/errors.js'; +import { LoginSession } from './types.js'; + +type ChallengeErrorCode = + // Account is not activated + | 'account_not_active' + // The principal associated with the credentials are not a user + | 'not_a_user' + // Username or password was wrong + | 'username_or_password_invalid' + // Username or password must be provided + | 'username_or_password_required' + // User must enter a TOTP code to continue + | 'totp_required' + // The TOTP code that was provided is invalid. + | 'totp_invalid' + // The email address used to log in was not verified + | 'email_not_verified'; + +export class A12nLoginChallengeError extends OAuth2Error { + + httpStatus = 400; + errorCode: ChallengeErrorCode; + session: LoginSession; + + constructor(session: LoginSession, message: string, errorCode: ChallengeErrorCode) { + + super(message); + this.errorCode = errorCode; + this.session = session; + + } + + serializeErrorBody() { + + return { + error: this.errorCode, + error_description: this.message, + auth_session: this.session.authSession, + expires_at: this.session.expiresAt, + }; + + } + +} diff --git a/src/login/service.ts b/src/login/service.ts index 9c358275..5161b3b6 100644 --- a/src/login/service.ts +++ b/src/login/service.ts @@ -1,14 +1,15 @@ import { BadRequest, NotFound } from '@curveball/http-errors'; -import { LoginSession } from './types.js'; +import { LoginSession, LoginChallengeContext } from './types.js'; import { AuthorizationChallengeRequest } from '../api-types.js'; -import { InvalidGrant, OAuth2Error } from '../oauth2/errors.js'; +import { InvalidGrant } from '../oauth2/errors.js'; import { OAuth2Code } from '../oauth2/types.js'; -import { getSetting } from '../server-settings.js'; import * as services from '../services.js'; -import { AppClient, Principal, PrincipalIdentity, User } from '../types.js'; +import { AppClient } from '../types.js'; import { getLogger } from '../log/service.js'; -import { UserEventLogger } from '../log/types.js'; import { generateSecretToken } from '../crypto.js'; +import { LoginChallengePassword } from './challenge/password.js'; +import { LoginChallengeTotp } from './challenge/totp.js'; +import { A12nLoginChallengeError } from './error.js'; type ChallengeRequest = AuthorizationChallengeRequest; @@ -50,13 +51,16 @@ async function startLoginSession(client: AppClient, scope?: string[]): Promise { - let user; - // If set to true, we'll log this as a 'session started' event for this user. - let logSessionStart = false; - + // If the session doesn't already have a principalId, we must log a 'session start' + // event. + const logSessionStart = !session.principalId; + const loginContext = await initChallengeContext( + session, + parameters + ); try { - const principalService = new services.principal.PrincipalService('insecure'); - if (!session.principalId) { - await challengeUsernamePassword(session, parameters); - logSessionStart = true; - } - assertSessionStage2(session); - user = await principalService.findById(session.principalId, 'user'); - const log = getLogger( - user, - parameters.remote_addr ?? null, - parameters.user_agent ?? null - ); - if (logSessionStart) log('login-challenge-started'); + await passwordChallenge.challenge(loginContext); + + if (logSessionStart) loginContext.log('login-challenge-started'); if (!session.authFactorsPassed.includes('totp')) { - await challengeTotp(session, parameters, user, log); + totpChallenge.challenge(loginContext); } assertSessionStage3(session); - log('login-challenge-success'); + loginContext.log('login-challenge-success'); } finally { - if (session.dirty) { + if (loginContext.dirty) { await storeSession(session); - session.dirty = false; + loginContext.dirty = false; } } @@ -109,7 +106,7 @@ export async function challenge(client: AppClient, session: LoginSession, parame return await services.oauth2.generateAuthorizationCode({ client, - principal: user, + principal: loginContext.principal, scope: session.scope ?? [], redirectUri: null, grantType: 'authorization_challenge', @@ -162,75 +159,54 @@ async function deleteSession(session: LoginSession) { } -async function challengeUsernamePassword(session: LoginSession, parameters: ChallengeRequest): Promise { +async function initChallengeContext(session: LoginSession, parameters: ChallengeRequest): Promise { - if (parameters.username === undefined || parameters.password === undefined) { - throw new A12nLoginChallengeError( - session, - 'A username and password are required', - 'username_or_password_required', - ); + let principal; + let identity; + let dirty = false; + const ps = new services.principal.PrincipalService('insecure'); + if (session.principalIdentityId && session.principalId) { - } - const principalService = new services.principal.PrincipalService('insecure'); - let user: Principal; - let identity: PrincipalIdentity; - try { - identity = await services.principalIdentity.findByUri('mailto:' + parameters.username); - } catch (err) { - if (err instanceof NotFound) { - throw new A12nLoginChallengeError( - session, - 'Incorrect username or password', - 'username_or_password_invalid', - ); - } else { - throw err; - } - - } - - try { - user = await principalService.findByIdentity(identity); - } catch (err) { - if (err instanceof NotFound) { + principal = await ps.findById(session.principalId); + identity = await services.principalIdentity.findById(principal, session.principalIdentityId); + } else { + if (parameters.username === undefined) { throw new A12nLoginChallengeError( session, - 'Incorrect username or password', + 'A username and password are required', 'username_or_password_required', ); - } else { - throw err; } + try { + identity = await services.principalIdentity.findByUri('mailto:' + parameters.username); + principal = identity.principal; + + } catch (err) { + if (err instanceof NotFound) { + throw new A12nLoginChallengeError( + session, + 'Incorrect username or password', + 'username_or_password_invalid', + ); + } else { + throw err; + } + } + dirty = true; } - - if (user.type !== 'user') { + if (principal.type !== 'user') { throw new A12nLoginChallengeError( session, 'Credentials are not associated with a user', 'not_a_user', ); } - const log = getLogger( - user, + principal, parameters.remote_addr ?? null, parameters.user_agent ?? null ); - const { success, errorMessage } = await services.user.validateUserCredentials(user, parameters.password, log); - if (!success && errorMessage) { - throw new A12nLoginChallengeError( - session, - errorMessage, - 'username_or_password_invalid', - ); - } - - session.principalId = user.id; - session.authFactorsPassed.push('password'); - session.dirty = true; - - if (!user.active) { + if (!principal.active) { log('login-failed-inactive'); throw new A12nLoginChallengeError( session, @@ -246,113 +222,21 @@ async function challengeUsernamePassword(session: LoginSession, parameters: Chal 'email_not_verified', ); } - - return user; -} - -/** - * This function is responsible for ensuring that if TOTP is set up for a user, - * it gets checked. - */ -async function challengeTotp(session: LoginSession, parameters: ChallengeRequest, user: User, log: UserEventLogger): Promise { - - const serverTotpMode = getSetting('totp'); - if (serverTotpMode === 'disabled') { - // Server-wide TOTP disabled. - session.authFactorsPassed.push('totp'); - session.dirty = true; - return; - } - const hasTotp = await services.mfaTotp.hasTotp(user); - if (!hasTotp) { - // Does this server require TOTP - if (serverTotpMode === 'required') { - throw new InvalidGrant('This server is configured to require TOTP, and this user does not have TOTP set up. Logging in is not possible for this user in its current state. Contact an administrator'); - } - // User didn't have TOTP so we just pass them - session.authFactorsPassed.push('totp'); - session.dirty = true; - return; - } - if (!parameters.totp_code) { - // No TOTP code was provided - throw new A12nLoginChallengeError( - session, - 'Please provide a TOTP code from the user\'s authenticator app.', - 'totp_required', - ); - } - if (!await services.mfaTotp.validateTotp(user, parameters.totp_code)) { - log('totp-failed'); - // TOTP code was incorrect - throw new A12nLoginChallengeError( - session, - 'Incorrect TOTP code. Make sure your system clock is set to the correct time and try again', - 'totp_invalid', - ); - } else { - log('totp-success'); + return { + principal, + identity, + log, + session: { + ...session, + principalId: identity.principal.id, + principalIdentityId: identity.id, + }, + parameters, + dirty, }; - // TOTP check successful! - session.authFactorsPassed.push('totp'); - session.dirty = true; - } -type ChallengeErrorCode = - // Account is not activated - | 'account_not_active' - // The principal associated with the credentials are not a user - | 'not_a_user' - // Username or password was wrong - | 'username_or_password_invalid' - // Username or password must be provided - | 'username_or_password_required' - // User must enter a TOTP code to continue - | 'totp_required' - // The TOTP code that was provided is invalid. - | 'totp_invalid' - // The email address used to log in was not verified - | 'email_not_verified'; - -class A12nLoginChallengeError extends OAuth2Error { - - httpStatus = 400; - errorCode: ChallengeErrorCode; - session: LoginSession; - - constructor(session: LoginSession, message: string, errorCode: ChallengeErrorCode) { - - super(message); - this.errorCode = errorCode; - this.session = session; - - } - - serializeErrorBody() { - - return { - error: this.errorCode, - error_description: this.message, - auth_session: this.session.authSession, - expires_at: this.session.expiresAt, - }; - - } - -} - -function assertSessionStage2(session: LoginSession): asserts session is LoginSession & {principalId: number} { - - if (!session.principalId) { - throw new Error('Invalid state: missing principalId'); - } - if (!session.authSession.includes('password')) { - throw new Error('Invalid state: password was not checked'); - } - -} function assertSessionStage3(session: LoginSession) { if (!session.authSession.includes('totp')) { diff --git a/src/login/types.ts b/src/login/types.ts index 3882ec9d..54294965 100644 --- a/src/login/types.ts +++ b/src/login/types.ts @@ -1,4 +1,7 @@ +import { User, PrincipalIdentity } from '../types.js'; import { AuthFactorType } from '../user-auth-factor/types.js'; +import { UserEventLogger } from '../log/types.js'; +import { AuthorizationChallengeRequest } from '../api-types.js'; /** * The login session represents an ongoing login process for a specific @@ -32,15 +35,14 @@ export type LoginSession = { principalId: number | null; /** - * List of OAuth2 scopes + * Identity ID */ - scope?: string[]; + principalIdentityId: number | null; /** - * Internal marker. If something related to the session changed we'll - * set this to true to indicate the session store should be updated. + * List of OAuth2 scopes */ - dirty: boolean; + scope?: string[]; /** * Which Auth Factors have been successfully checked during the current @@ -49,3 +51,57 @@ export type LoginSession = { authFactorsPassed: AuthFactorType[]; }; + +/** + * A login session with a confirmed principal id and identity. + */ +export type LoginSessionWithPrincipal = LoginSession & { + + /** + * User id + */ + principalId: number; + + /** + * Identity ID + */ + principalIdentityId: number; +} + + +export type LoginChallengeContext = { + + /** + * The user that's trying to authenticate + */ + principal: User; + + /** + * The identity (email address usually) of the principal that was used to + * start this challenge, usually supplied as a username. + */ + identity: PrincipalIdentity; + + /** + * Easy access to a logger + */ + log: UserEventLogger; + + /** + * Parameters sent with the *current* request. + */ + parameters: AuthorizationChallengeRequest; + + /** + * Session data associated with the login challenge process. + * This is persistent data. + */ + session: LoginSession; + + /** + * Set to true if session data changed and we need to save + * it again. + */ + dirty: boolean; + +} From 8ec57e3e1ed1942c07bf54bf2fbd3b8f747e81eb Mon Sep 17 00:00:00 2001 From: Evert Pot Date: Mon, 6 Jan 2025 23:30:11 -0500 Subject: [PATCH 02/10] Fix a few logic bugs --- src/app-client/controller/new.ts | 1 + src/login/service.ts | 22 +++++++--------------- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/src/app-client/controller/new.ts b/src/app-client/controller/new.ts index 28274220..559c5551 100644 --- a/src/app-client/controller/new.ts +++ b/src/app-client/controller/new.ts @@ -22,3 +22,4 @@ class NewClientController extends Controller { } export default new NewClientController(); + diff --git a/src/login/service.ts b/src/login/service.ts index 5161b3b6..005e531b 100644 --- a/src/login/service.ts +++ b/src/login/service.ts @@ -81,28 +81,28 @@ export async function challenge(client: AppClient, session: LoginSession, parame ); try { - await passwordChallenge.challenge(loginContext); + if (!session.authFactorsPassed.includes('password')) { + await passwordChallenge.challenge(loginContext); + } if (logSessionStart) loginContext.log('login-challenge-started'); if (!session.authFactorsPassed.includes('totp')) { - totpChallenge.challenge(loginContext); + await totpChallenge.challenge(loginContext); } - assertSessionStage3(session); - loginContext.log('login-challenge-success'); } finally { if (loginContext.dirty) { - await storeSession(session); + await storeSession(loginContext.session); loginContext.dirty = false; } } - await deleteSession(session); + await deleteSession(loginContext.session); return await services.oauth2.generateAuthorizationCode({ client, @@ -148,7 +148,7 @@ async function storeSession(session: LoginSession) { sessionKey(session.authSession), session, { - ttl: session.expiresAt - Date.now(), + ttl: session.expiresAt * 1000 - Date.now(), } ); @@ -236,11 +236,3 @@ async function initChallengeContext(session: LoginSession, parameters: Challenge }; } - -function assertSessionStage3(session: LoginSession) { - - if (!session.authSession.includes('totp')) { - throw new Error('Invalid state: totp was not checked'); - } - -} From 49e633a6da1282d49162f387c3d656fd4839823e Mon Sep 17 00:00:00 2001 From: Evert Pot Date: Tue, 7 Jan 2025 13:56:04 -0500 Subject: [PATCH 03/10] Refactor challenges into strategy classes --- src/login/challenge/abstract.ts | 15 +++++++++++++++ src/login/challenge/password.ts | 11 +++++++++++ src/login/challenge/totp.ts | 12 ++++++++++++ src/login/service.ts | 5 +++-- 4 files changed, 41 insertions(+), 2 deletions(-) diff --git a/src/login/challenge/abstract.ts b/src/login/challenge/abstract.ts index 59240c2c..3a52f5ec 100644 --- a/src/login/challenge/abstract.ts +++ b/src/login/challenge/abstract.ts @@ -1,7 +1,22 @@ import { LoginChallengeContext } from '../types.js'; +import { User } from '../../types.js'; export abstract class AbstractLoginChallenge { + protected principal: User; + + constructor(principal: User) { + this.principal = principal; + } + + /** + * Returns true if the user has this auth factor set up. + * + * For example, if a user has a TOTP device setup this should + * return true for the totp challenge class. + */ + abstract hasFactor(): Promise; + abstract challenge(loginContext: LoginChallengeContext): Promise; } diff --git a/src/login/challenge/password.ts b/src/login/challenge/password.ts index fa56c8f6..3b797fe0 100644 --- a/src/login/challenge/password.ts +++ b/src/login/challenge/password.ts @@ -4,6 +4,17 @@ import { A12nLoginChallengeError } from '../error.js'; import * as services from '../../services.js'; export class LoginChallengePassword extends AbstractLoginChallenge { + /** + * Returns true if the user has this auth factor set up. + * + * For example, if a user has a TOTP device setup this should + * return true for the totp challenge class. + */ + hasFactor(): Promise { + + return services.user.hasPassword(this.principal); + + } async challenge(loginContext: LoginChallengeContext): Promise { diff --git a/src/login/challenge/totp.ts b/src/login/challenge/totp.ts index 1268fedb..90273d55 100644 --- a/src/login/challenge/totp.ts +++ b/src/login/challenge/totp.ts @@ -6,6 +6,18 @@ import { InvalidGrant } from '../../oauth2/errors.js'; import { getSetting } from '../../server-settings.js'; export class LoginChallengeTotp extends AbstractLoginChallenge { + /** + * Returns true if the user has this auth factor set up. + * + * For example, if a user has a TOTP device setup this should + * return true for the totp challenge class. + */ + hasFactor(): Promise { + + return services.mfaTotp.hasTotp(this.principal); + + } + async challenge(loginContext: LoginChallengeContext): Promise { const serverTotpMode = getSetting('totp'); diff --git a/src/login/service.ts b/src/login/service.ts index 005e531b..7793d5be 100644 --- a/src/login/service.ts +++ b/src/login/service.ts @@ -58,8 +58,6 @@ async function startLoginSession(client: AppClient, scope?: string[]): Promise Date: Tue, 7 Jan 2025 16:04:57 -0500 Subject: [PATCH 04/10] Small API tweaks to challenge system. --- src/login/challenge/abstract.ts | 18 ++++++++++++++++-- src/login/challenge/password.ts | 23 +++++++++++++++++++++-- src/login/challenge/totp.ts | 25 ++++++++++++++++++++----- src/login/service.ts | 4 ++-- src/login/types.ts | 1 + 5 files changed, 60 insertions(+), 11 deletions(-) diff --git a/src/login/challenge/abstract.ts b/src/login/challenge/abstract.ts index 3a52f5ec..6b93668b 100644 --- a/src/login/challenge/abstract.ts +++ b/src/login/challenge/abstract.ts @@ -1,4 +1,4 @@ -import { LoginChallengeContext } from '../types.js'; +import { LoginChallengeContext, AuthorizationChallengeRequest } from '../types.js'; import { User } from '../../types.js'; export abstract class AbstractLoginChallenge { @@ -17,6 +17,20 @@ export abstract class AbstractLoginChallenge { */ abstract hasFactor(): Promise; - abstract challenge(loginContext: LoginChallengeContext): Promise; + /** + * Handle the user response to a challenge. + * + * Should return true if the challenge passed. + * Should throw an Error ihe challenge failed. + */ + abstract checkResponse(loginContext: LoginChallengeContext): Promise; + + /** + * Should return true if parameters contain a response to the challenge. + * + * For example, for the password challenge this checks if the paremters contained + * a 'password' key. + */ + abstract parametersHasResponse(parameters: AuthorizationChallengeRequest): boolean; } diff --git a/src/login/challenge/password.ts b/src/login/challenge/password.ts index 3b797fe0..f41c62f7 100644 --- a/src/login/challenge/password.ts +++ b/src/login/challenge/password.ts @@ -1,5 +1,5 @@ import { AbstractLoginChallenge } from './abstract.js'; -import { LoginChallengeContext } from '../types.js'; +import { LoginChallengeContext, AuthorizationChallengeRequest } from '../types.js'; import { A12nLoginChallengeError } from '../error.js'; import * as services from '../../services.js'; @@ -16,7 +16,13 @@ export class LoginChallengePassword extends AbstractLoginChallenge { } - async challenge(loginContext: LoginChallengeContext): Promise { + /** + * Handle the user response to a challenge. + * + * Should return true if the challenge passed. + * Should throw an Error ihe challenge failed. + */ + async checkResponse(loginContext: LoginChallengeContext): Promise { if (loginContext.parameters.password === undefined) { throw new A12nLoginChallengeError( @@ -38,6 +44,19 @@ export class LoginChallengePassword extends AbstractLoginChallenge { loginContext.session.authFactorsPassed.push('password'); loginContext.dirty = true; + return true; + + } + + /** + * Should return true if parameters contain a response to the challenge. + * + * For example, for the password challenge this checks if the paremters contained + * a 'password' key. + */ + parametersHasResponse(parameters: AuthorizationChallengeRequest): boolean { + + return parameters.password !== undefined; } diff --git a/src/login/challenge/totp.ts b/src/login/challenge/totp.ts index 90273d55..875cd08b 100644 --- a/src/login/challenge/totp.ts +++ b/src/login/challenge/totp.ts @@ -1,9 +1,10 @@ import { AbstractLoginChallenge } from './abstract.js'; -import { LoginChallengeContext } from '../types.js'; +import { LoginChallengeContext, AuthorizationChallengeRequest } from '../types.js'; import { A12nLoginChallengeError } from '../error.js'; import * as services from '../../services.js'; import { InvalidGrant } from '../../oauth2/errors.js'; import { getSetting } from '../../server-settings.js'; + export class LoginChallengeTotp extends AbstractLoginChallenge { /** @@ -12,20 +13,22 @@ export class LoginChallengeTotp extends AbstractLoginChallenge { * For example, if a user has a TOTP device setup this should * return true for the totp challenge class. */ - hasFactor(): Promise { + async hasFactor(): Promise { + const serverTotpMode = getSetting('totp'); + if (serverTotpMode === 'disabled') return false; return services.mfaTotp.hasTotp(this.principal); } - async challenge(loginContext: LoginChallengeContext): Promise { + async checkResponse(loginContext: LoginChallengeContext): Promise { const serverTotpMode = getSetting('totp'); if (serverTotpMode === 'disabled') { // Server-wide TOTP disabled. loginContext.session.authFactorsPassed.push('totp'); loginContext.dirty = true; - return; + return true; } const hasTotp = await services.mfaTotp.hasTotp(loginContext.principal); if (!hasTotp) { @@ -36,7 +39,7 @@ export class LoginChallengeTotp extends AbstractLoginChallenge { // User didn't have TOTP so we just pass them loginContext.session.authFactorsPassed.push('totp'); loginContext.dirty = true; - return; + return true; } if (!loginContext.parameters.totp_code) { // No TOTP code was provided @@ -61,6 +64,18 @@ export class LoginChallengeTotp extends AbstractLoginChallenge { // TOTP check successful! loginContext.session.authFactorsPassed.push('totp'); loginContext.dirty = true; + return true; + + } + /** + * Should return true if parameters contain a response to the challenge. + * + * For example, for the password challenge this checks if the paremters contained + * a 'password' key. + */ + parametersHasResponse(parameters: AuthorizationChallengeRequest): boolean { + + return parameters.totp_code !== undefined; } diff --git a/src/login/service.ts b/src/login/service.ts index 7793d5be..52d3bec2 100644 --- a/src/login/service.ts +++ b/src/login/service.ts @@ -83,13 +83,13 @@ export async function challenge(client: AppClient, session: LoginSession, parame try { if (!session.authFactorsPassed.includes('password')) { - await passwordChallenge.challenge(loginContext); + await passwordChallenge.checkResponse(loginContext); } if (logSessionStart) loginContext.log('login-challenge-started'); if (!session.authFactorsPassed.includes('totp')) { - await totpChallenge.challenge(loginContext); + await totpChallenge.checkResponse(loginContext); } loginContext.log('login-challenge-success'); diff --git a/src/login/types.ts b/src/login/types.ts index 54294965..e14f1257 100644 --- a/src/login/types.ts +++ b/src/login/types.ts @@ -2,6 +2,7 @@ import { User, PrincipalIdentity } from '../types.js'; import { AuthFactorType } from '../user-auth-factor/types.js'; import { UserEventLogger } from '../log/types.js'; import { AuthorizationChallengeRequest } from '../api-types.js'; +export { AuthorizationChallengeRequest } from '../api-types.js'; /** * The login session represents an ongoing login process for a specific From a1ab7e5f12c1d95b07d563c8a9ab8ef3410c3fb7 Mon Sep 17 00:00:00 2001 From: Evert Pot Date: Tue, 7 Jan 2025 19:00:54 -0500 Subject: [PATCH 05/10] Splitting up logic further in separate funtions and fully abstracing the concept --- src/login/challenge/abstract.ts | 19 ++++++++- src/login/challenge/password.ts | 10 +++-- src/login/challenge/totp.ts | 13 +++---- src/login/error.ts | 2 + src/login/service.ts | 68 ++++++++++++++++++++++++++++----- src/login/types.ts | 2 +- 6 files changed, 92 insertions(+), 22 deletions(-) diff --git a/src/login/challenge/abstract.ts b/src/login/challenge/abstract.ts index 6b93668b..e41a77ea 100644 --- a/src/login/challenge/abstract.ts +++ b/src/login/challenge/abstract.ts @@ -1,10 +1,21 @@ import { LoginChallengeContext, AuthorizationChallengeRequest } from '../types.js'; import { User } from '../../types.js'; +import { AuthFactorType } from '../../user-auth-factor/types.js'; export abstract class AbstractLoginChallenge { + /** + * The principal associated with the process. + * + * This class will only be used for a single user, so this lets you cache responses if needed. + */ protected principal: User; + /** + * The type of authentication factor this class provides. + */ + abstract readonly authFactor: AuthFactorType; + constructor(principal: User) { this.principal = principal; } @@ -15,7 +26,7 @@ export abstract class AbstractLoginChallenge { * For example, if a user has a TOTP device setup this should * return true for the totp challenge class. */ - abstract hasFactor(): Promise; + abstract userHasChallenge(): Promise; /** * Handle the user response to a challenge. @@ -33,4 +44,10 @@ export abstract class AbstractLoginChallenge { */ abstract parametersHasResponse(parameters: AuthorizationChallengeRequest): boolean; + /** + * Emits the challenge. This is done in situations that no credentials have + * been received yet. + */ + abstract challenge(): never; + } diff --git a/src/login/challenge/password.ts b/src/login/challenge/password.ts index f41c62f7..4cac2479 100644 --- a/src/login/challenge/password.ts +++ b/src/login/challenge/password.ts @@ -4,13 +4,19 @@ import { A12nLoginChallengeError } from '../error.js'; import * as services from '../../services.js'; export class LoginChallengePassword extends AbstractLoginChallenge { + + /** + * The type of authentication factor this class provides. + */ + readonly authFactor = 'password'; + /** * Returns true if the user has this auth factor set up. * * For example, if a user has a TOTP device setup this should * return true for the totp challenge class. */ - hasFactor(): Promise { + userHasChallenge(): Promise { return services.user.hasPassword(this.principal); @@ -42,8 +48,6 @@ export class LoginChallengePassword extends AbstractLoginChallenge { ); } - loginContext.session.authFactorsPassed.push('password'); - loginContext.dirty = true; return true; } diff --git a/src/login/challenge/totp.ts b/src/login/challenge/totp.ts index 875cd08b..99c962e6 100644 --- a/src/login/challenge/totp.ts +++ b/src/login/challenge/totp.ts @@ -7,13 +7,18 @@ import { getSetting } from '../../server-settings.js'; export class LoginChallengeTotp extends AbstractLoginChallenge { + /** + * The type of authentication factor this class provides. + */ + readonly authFactor = 'totp'; + /** * Returns true if the user has this auth factor set up. * * For example, if a user has a TOTP device setup this should * return true for the totp challenge class. */ - async hasFactor(): Promise { + async userHasChallenge(): Promise { const serverTotpMode = getSetting('totp'); if (serverTotpMode === 'disabled') return false; @@ -26,8 +31,6 @@ export class LoginChallengeTotp extends AbstractLoginChallenge { const serverTotpMode = getSetting('totp'); if (serverTotpMode === 'disabled') { // Server-wide TOTP disabled. - loginContext.session.authFactorsPassed.push('totp'); - loginContext.dirty = true; return true; } const hasTotp = await services.mfaTotp.hasTotp(loginContext.principal); @@ -37,8 +40,6 @@ export class LoginChallengeTotp extends AbstractLoginChallenge { throw new InvalidGrant('This server is configured to require TOTP, and this user does not have TOTP set up. Logging in is not possible for this user in its current state. Contact an administrator'); } // User didn't have TOTP so we just pass them - loginContext.session.authFactorsPassed.push('totp'); - loginContext.dirty = true; return true; } if (!loginContext.parameters.totp_code) { @@ -62,8 +63,6 @@ export class LoginChallengeTotp extends AbstractLoginChallenge { }; // TOTP check successful! - loginContext.session.authFactorsPassed.push('totp'); - loginContext.dirty = true; return true; } diff --git a/src/login/error.ts b/src/login/error.ts index 6bd5eb15..b8be06b0 100644 --- a/src/login/error.ts +++ b/src/login/error.ts @@ -6,6 +6,8 @@ type ChallengeErrorCode = | 'account_not_active' // The principal associated with the credentials are not a user | 'not_a_user' + // The user doesn't have any credentials set up + | 'no_credentials' // Username or password was wrong | 'username_or_password_invalid' // Username or password must be provided diff --git a/src/login/service.ts b/src/login/service.ts index 52d3bec2..9d4d593c 100644 --- a/src/login/service.ts +++ b/src/login/service.ts @@ -4,12 +4,13 @@ import { AuthorizationChallengeRequest } from '../api-types.js'; import { InvalidGrant } from '../oauth2/errors.js'; import { OAuth2Code } from '../oauth2/types.js'; import * as services from '../services.js'; -import { AppClient } from '../types.js'; +import { AppClient, User } from '../types.js'; import { getLogger } from '../log/service.js'; import { generateSecretToken } from '../crypto.js'; import { LoginChallengePassword } from './challenge/password.js'; import { LoginChallengeTotp } from './challenge/totp.js'; import { A12nLoginChallengeError } from './error.js'; +import { AbstractLoginChallenge } from './challenge/abstract.js'; type ChallengeRequest = AuthorizationChallengeRequest; @@ -52,7 +53,7 @@ async function startLoginSession(client: AppClient, scope?: string[]): Promise { + + const challenges = [ + new LoginChallengePassword(principal), + new LoginChallengeTotp(principal) + ]; + + const result = []; + for(const challenge of challenges) { + if (await challenge.userHasChallenge()) { + result.push(challenge); + } + } + return result; + +} diff --git a/src/login/types.ts b/src/login/types.ts index e14f1257..d0d63fa0 100644 --- a/src/login/types.ts +++ b/src/login/types.ts @@ -49,7 +49,7 @@ export type LoginSession = { * Which Auth Factors have been successfully checked during the current * session. */ - authFactorsPassed: AuthFactorType[]; + challengesCompleted: AuthFactorType[]; }; From ad81f41780bca4c7dd7ddccb74387d16c0f44888 Mon Sep 17 00:00:00 2001 From: Evert Pot Date: Tue, 7 Jan 2025 19:33:21 -0500 Subject: [PATCH 06/10] Emit challenges --- src/login/challenge/abstract.ts | 20 +++++++++++++++---- src/login/challenge/password.ts | 34 +++++++++++++++++++++------------ src/login/challenge/totp.ts | 24 ++++++++++++++++++++--- src/login/error.ts | 8 +++++--- src/login/service.ts | 10 +++++----- 5 files changed, 69 insertions(+), 27 deletions(-) diff --git a/src/login/challenge/abstract.ts b/src/login/challenge/abstract.ts index e41a77ea..2ac6080b 100644 --- a/src/login/challenge/abstract.ts +++ b/src/login/challenge/abstract.ts @@ -1,8 +1,8 @@ -import { LoginChallengeContext, AuthorizationChallengeRequest } from '../types.js'; +import { LoginChallengeContext, AuthorizationChallengeRequest, LoginSession } from '../types.js'; import { User } from '../../types.js'; import { AuthFactorType } from '../../user-auth-factor/types.js'; -export abstract class AbstractLoginChallenge { +export abstract class AbstractLoginChallenge { /** * The principal associated with the process. @@ -42,12 +42,24 @@ export abstract class AbstractLoginChallenge { * For example, for the password challenge this checks if the paremters contained * a 'password' key. */ - abstract parametersHasResponse(parameters: AuthorizationChallengeRequest): boolean; + abstract parametersContainsResponse(parameters: AuthorizationChallengeRequest): parameters is TChallengeParameters & AuthorizationChallengeRequest; /** * Emits the challenge. This is done in situations that no credentials have * been received yet. */ - abstract challenge(): never; + abstract challenge(session: LoginSession): never; + + /** + * Validates whether the parameters object contains expected values. + * + * This for instance will make sure that a 'possword' key was provided for + * the Password challenge. + */ + validateParameters(parameters: AuthorizationChallengeRequest): asserts parameters is TChallengeParameters & AuthorizationChallengeRequest { + if (!this.parametersContainsResponse(parameters)) { + throw new Error('Invalid state. This should normally not happen unless there\'s a logic bug'); + } + } } diff --git a/src/login/challenge/password.ts b/src/login/challenge/password.ts index 4cac2479..1e8e0434 100644 --- a/src/login/challenge/password.ts +++ b/src/login/challenge/password.ts @@ -1,9 +1,13 @@ import { AbstractLoginChallenge } from './abstract.js'; -import { LoginChallengeContext, AuthorizationChallengeRequest } from '../types.js'; +import { LoginChallengeContext, AuthorizationChallengeRequest, LoginSession } from '../types.js'; import { A12nLoginChallengeError } from '../error.js'; import * as services from '../../services.js'; -export class LoginChallengePassword extends AbstractLoginChallenge { +type PasswordParameters = { + password: string; +} + +export class LoginChallengePassword extends AbstractLoginChallenge { /** * The type of authentication factor this class provides. @@ -30,15 +34,7 @@ export class LoginChallengePassword extends AbstractLoginChallenge { */ async checkResponse(loginContext: LoginChallengeContext): Promise { - if (loginContext.parameters.password === undefined) { - throw new A12nLoginChallengeError( - loginContext.session, - 'A username and password are required', - 'username_or_password_required', - ); - - } - + this.validateParameters(loginContext.parameters); const { success, errorMessage } = await services.user.validateUserCredentials(loginContext.principal, loginContext.parameters.password, loginContext.log); if (!success && errorMessage) { throw new A12nLoginChallengeError( @@ -58,10 +54,24 @@ export class LoginChallengePassword extends AbstractLoginChallenge { * For example, for the password challenge this checks if the paremters contained * a 'password' key. */ - parametersHasResponse(parameters: AuthorizationChallengeRequest): boolean { + parametersContainsResponse(parameters: AuthorizationChallengeRequest): parameters is PasswordParameters { return parameters.password !== undefined; } + /** + * Emits the challenge. This is done in situations that no credentials have + * been received yet. + */ + challenge(session: LoginSession): never { + + throw new A12nLoginChallengeError( + session, + 'A username and password are required', + 'password_required', + ); + + } + } diff --git a/src/login/challenge/totp.ts b/src/login/challenge/totp.ts index 99c962e6..3002bec3 100644 --- a/src/login/challenge/totp.ts +++ b/src/login/challenge/totp.ts @@ -1,11 +1,15 @@ import { AbstractLoginChallenge } from './abstract.js'; -import { LoginChallengeContext, AuthorizationChallengeRequest } from '../types.js'; +import { LoginChallengeContext, AuthorizationChallengeRequest, LoginSession } from '../types.js'; import { A12nLoginChallengeError } from '../error.js'; import * as services from '../../services.js'; import { InvalidGrant } from '../../oauth2/errors.js'; import { getSetting } from '../../server-settings.js'; -export class LoginChallengeTotp extends AbstractLoginChallenge { +type TotpParameters = { + totp_code: string; +} + +export class LoginChallengeTotp extends AbstractLoginChallenge { /** * The type of authentication factor this class provides. @@ -72,10 +76,24 @@ export class LoginChallengeTotp extends AbstractLoginChallenge { * For example, for the password challenge this checks if the paremters contained * a 'password' key. */ - parametersHasResponse(parameters: AuthorizationChallengeRequest): boolean { + parametersContainsResponse(parameters: AuthorizationChallengeRequest): parameters is TotpParameters { return parameters.totp_code !== undefined; } + /** + * Emits the challenge. This is done in situations that no credentials have + * been received yet. + */ + challenge(session: LoginSession): never { + + throw new A12nLoginChallengeError( + session, + 'Please provide a TOTP code from the user\'s authenticator app.', + 'totp_required', + ); + + } + } diff --git a/src/login/error.ts b/src/login/error.ts index b8be06b0..64639608 100644 --- a/src/login/error.ts +++ b/src/login/error.ts @@ -4,14 +4,16 @@ import { LoginSession } from './types.js'; type ChallengeErrorCode = // Account is not activated | 'account_not_active' - // The principal associated with the credentials are not a user + // The principal associated with the credentials is not a user | 'not_a_user' // The user doesn't have any credentials set up | 'no_credentials' // Username or password was wrong | 'username_or_password_invalid' - // Username or password must be provided - | 'username_or_password_required' + // Username must be provided + | 'username_required' + // Password must be provided + | 'password_required' // User must enter a TOTP code to continue | 'totp_required' // The TOTP code that was provided is invalid. diff --git a/src/login/service.ts b/src/login/service.ts index 9d4d593c..78bc3a56 100644 --- a/src/login/service.ts +++ b/src/login/service.ts @@ -100,7 +100,7 @@ export async function challenge(client: AppClient, session: LoginSession, parame // This challenge has already been checked previously. continue; } - if (challenge.parametersHasResponse(parameters)) { + if (challenge.parametersContainsResponse(parameters)) { // The user did submit a value for this challenge. Lets check it. const challengeResult = await challenge.checkResponse(loginContext); if (challengeResult) { @@ -116,7 +116,7 @@ export async function challenge(client: AppClient, session: LoginSession, parame // complete just 2 challenges. for(const challenge of challenges) { if (!loginContext.session.challengesCompleted.includes(challenge.authFactor)) { - challenge.challenge(); + challenge.challenge(loginContext.session); } } @@ -202,8 +202,8 @@ async function initChallengeContext(session: LoginSession, parameters: Challenge if (parameters.username === undefined) { throw new A12nLoginChallengeError( session, - 'A username and password are required', - 'username_or_password_required', + 'A username is required', + 'username_required', ); } try { @@ -269,7 +269,7 @@ async function initChallengeContext(session: LoginSession, parameters: Challenge /** * Returns the full list of login challenges the user has setup up. */ -async function getChallengesForPrincipal(principal: User): Promise { +async function getChallengesForPrincipal(principal: User): Promise[]> { const challenges = [ new LoginChallengePassword(principal), From bb612cbdbc408bcf35b4b415282a7051c7974644 Mon Sep 17 00:00:00 2001 From: Evert Pot Date: Tue, 7 Jan 2025 19:38:41 -0500 Subject: [PATCH 07/10] Require at most 2 factors for a successful login --- src/login/challenge/password.ts | 2 +- src/login/challenge/totp.ts | 2 +- src/login/service.ts | 16 ++++++++++------ 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/login/challenge/password.ts b/src/login/challenge/password.ts index 1e8e0434..483247f6 100644 --- a/src/login/challenge/password.ts +++ b/src/login/challenge/password.ts @@ -12,7 +12,7 @@ export class LoginChallengePassword extends AbstractLoginChallenge { /** * The type of authentication factor this class provides. */ - readonly authFactor = 'totp'; + readonly authFactor = 'totp'; /** * Returns true if the user has this auth factor set up. diff --git a/src/login/service.ts b/src/login/service.ts index 78bc3a56..16854c86 100644 --- a/src/login/service.ts +++ b/src/login/service.ts @@ -80,7 +80,7 @@ export async function challenge(client: AppClient, session: LoginSession, parame parameters ); - if (logSessionStart) loginContext.log('login-challenge-started'); + if (logSessionStart) loginContext.log('login-challenge-started'); const challenges = await getChallengesForPrincipal(loginContext.principal); @@ -112,11 +112,15 @@ export async function challenge(client: AppClient, session: LoginSession, parame } - // Right now every challenge has to be met. In the future we will let users - // complete just 2 challenges. - for(const challenge of challenges) { - if (!loginContext.session.challengesCompleted.includes(challenge.authFactor)) { - challenge.challenge(loginContext.session); + const completedChallenges = new Set(loginContext.session.challengesCompleted); + + if (completedChallenges.size < 2 && challenges.length > 1) { + // If there are 2 or more auth factors set up, we want at least 2 successful + // passes. If this is not the case we're going to emit a challenge error. + for(const challenge of challenges) { + if (!loginContext.session.challengesCompleted.includes(challenge.authFactor)) { + challenge.challenge(loginContext.session); + } } } From 755ad7d826dde6b4d53d75845a72a5966e813299 Mon Sep 17 00:00:00 2001 From: Evert Pot Date: Tue, 7 Jan 2025 22:04:47 -0500 Subject: [PATCH 08/10] Tweaked some awkward docblocks --- src/login/challenge/abstract.ts | 8 +++++--- src/login/challenge/password.ts | 6 ++++-- src/login/challenge/totp.ts | 6 ++++-- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/login/challenge/abstract.ts b/src/login/challenge/abstract.ts index 2ac6080b..2126acbb 100644 --- a/src/login/challenge/abstract.ts +++ b/src/login/challenge/abstract.ts @@ -45,15 +45,17 @@ export abstract class AbstractLoginChallenge { abstract parametersContainsResponse(parameters: AuthorizationChallengeRequest): parameters is TChallengeParameters & AuthorizationChallengeRequest; /** - * Emits the challenge. This is done in situations that no credentials have - * been received yet. + * Emits the initial challenge. + * + * This notifies the user that some kind of response is expected as a reply + * to this challenge. */ abstract challenge(session: LoginSession): never; /** * Validates whether the parameters object contains expected values. * - * This for instance will make sure that a 'possword' key was provided for + * This for instance will make sure that a 'password' key was provided for * the Password challenge. */ validateParameters(parameters: AuthorizationChallengeRequest): asserts parameters is TChallengeParameters & AuthorizationChallengeRequest { diff --git a/src/login/challenge/password.ts b/src/login/challenge/password.ts index 483247f6..877d77c0 100644 --- a/src/login/challenge/password.ts +++ b/src/login/challenge/password.ts @@ -61,8 +61,10 @@ export class LoginChallengePassword extends AbstractLoginChallenge { } /** - * Emits the challenge. This is done in situations that no credentials have - * been received yet. + * Emits the initial challenge. + * + * This notifies the user that some kind of response is expected as a reply + * to this challenge. */ challenge(session: LoginSession): never { From 21f1989a15f2c8959f1ee761c2934634d95fbc57 Mon Sep 17 00:00:00 2001 From: Evert Pot Date: Tue, 7 Jan 2025 23:08:54 -0500 Subject: [PATCH 09/10] Destroy LoginContext --- src/login/challenge/abstract.ts | 20 +++++++++--- src/login/challenge/password.ts | 12 ++++--- src/login/challenge/totp.ts | 23 ++++++++------ src/login/controller/login.ts | 3 ++ src/login/controller/mfa.ts | 5 +++ src/login/service.ts | 56 +++++++++++++++++---------------- 6 files changed, 73 insertions(+), 46 deletions(-) diff --git a/src/login/challenge/abstract.ts b/src/login/challenge/abstract.ts index 2126acbb..14706913 100644 --- a/src/login/challenge/abstract.ts +++ b/src/login/challenge/abstract.ts @@ -1,9 +1,18 @@ -import { LoginChallengeContext, AuthorizationChallengeRequest, LoginSession } from '../types.js'; +import { AuthorizationChallengeRequest, LoginSession } from '../types.js'; import { User } from '../../types.js'; import { AuthFactorType } from '../../user-auth-factor/types.js'; +import { UserEventLogger } from '../../log/types.js'; +/** + * This abstract class is implemented by various authentication challenge strategies. + */ export abstract class AbstractLoginChallenge { + /** + * The type of authentication factor this class provides. + */ + abstract readonly authFactor: AuthFactorType; + /** * The principal associated with the process. * @@ -12,12 +21,13 @@ export abstract class AbstractLoginChallenge { protected principal: User; /** - * The type of authentication factor this class provides. + * Logger function */ - abstract readonly authFactor: AuthFactorType; + protected log: UserEventLogger; - constructor(principal: User) { + constructor(principal: User, logger: UserEventLogger) { this.principal = principal; + this.log = logger; } /** @@ -34,7 +44,7 @@ export abstract class AbstractLoginChallenge { * Should return true if the challenge passed. * Should throw an Error ihe challenge failed. */ - abstract checkResponse(loginContext: LoginChallengeContext): Promise; + abstract checkResponse(session: LoginSession, parameters: TChallengeParameters): Promise; /** * Should return true if parameters contain a response to the challenge. diff --git a/src/login/challenge/password.ts b/src/login/challenge/password.ts index 877d77c0..c73aa904 100644 --- a/src/login/challenge/password.ts +++ b/src/login/challenge/password.ts @@ -1,5 +1,5 @@ import { AbstractLoginChallenge } from './abstract.js'; -import { LoginChallengeContext, AuthorizationChallengeRequest, LoginSession } from '../types.js'; +import { AuthorizationChallengeRequest, LoginSession } from '../types.js'; import { A12nLoginChallengeError } from '../error.js'; import * as services from '../../services.js'; @@ -7,6 +7,9 @@ type PasswordParameters = { password: string; } +/** + * Password-based authentication strategy. + */ export class LoginChallengePassword extends AbstractLoginChallenge { /** @@ -32,13 +35,12 @@ export class LoginChallengePassword extends AbstractLoginChallenge { + async checkResponse(session: LoginSession, parameters: PasswordParameters): Promise { - this.validateParameters(loginContext.parameters); - const { success, errorMessage } = await services.user.validateUserCredentials(loginContext.principal, loginContext.parameters.password, loginContext.log); + const { success, errorMessage } = await services.user.validateUserCredentials(this.principal, parameters.password, this.log); if (!success && errorMessage) { throw new A12nLoginChallengeError( - loginContext.session, + session, errorMessage, 'username_or_password_invalid', ); diff --git a/src/login/challenge/totp.ts b/src/login/challenge/totp.ts index aa45db34..1d58d2f6 100644 --- a/src/login/challenge/totp.ts +++ b/src/login/challenge/totp.ts @@ -1,5 +1,5 @@ import { AbstractLoginChallenge } from './abstract.js'; -import { LoginChallengeContext, AuthorizationChallengeRequest, LoginSession } from '../types.js'; +import { AuthorizationChallengeRequest, LoginSession } from '../types.js'; import { A12nLoginChallengeError } from '../error.js'; import * as services from '../../services.js'; import { InvalidGrant } from '../../oauth2/errors.js'; @@ -9,6 +9,11 @@ type TotpParameters = { totp_code: string; } +/** + * Time-based-one-time-passwords. + * + * This strategy handles authenticator apps. + */ export class LoginChallengeTotp extends AbstractLoginChallenge { /** @@ -30,14 +35,14 @@ export class LoginChallengeTotp extends AbstractLoginChallenge { } - async checkResponse(loginContext: LoginChallengeContext): Promise { + async checkResponse(session: LoginSession, parameters: TotpParameters): Promise { const serverTotpMode = getSetting('totp'); if (serverTotpMode === 'disabled') { // Server-wide TOTP disabled. return true; } - const hasTotp = await services.mfaTotp.hasTotp(loginContext.principal); + const hasTotp = await services.mfaTotp.hasTotp(this.principal); if (!hasTotp) { // Does this server require TOTP if (serverTotpMode === 'required') { @@ -46,24 +51,24 @@ export class LoginChallengeTotp extends AbstractLoginChallenge { // User didn't have TOTP so we just pass them return true; } - if (!loginContext.parameters.totp_code) { + if (!parameters.totp_code) { // No TOTP code was provided throw new A12nLoginChallengeError( - loginContext.session, + session, 'Please provide a TOTP code from the user\'s authenticator app.', 'totp_required', ); } - if (!await services.mfaTotp.validateTotp(loginContext.principal, loginContext.parameters.totp_code)) { - loginContext.log('totp-failed'); + if (!await services.mfaTotp.validateTotp(this.principal, parameters.totp_code)) { + this.log('totp-failed'); // TOTP code was incorrect throw new A12nLoginChallengeError( - loginContext.session, + session, 'Incorrect TOTP code. Make sure your system clock is set to the correct time and try again', 'totp_invalid', ); } else { - loginContext.log('totp-success'); + this.log('totp-success'); }; // TOTP check successful! diff --git a/src/login/controller/login.ts b/src/login/controller/login.ts index 666b06e9..153c62bb 100644 --- a/src/login/controller/login.ts +++ b/src/login/controller/login.ts @@ -12,6 +12,9 @@ import { PrincipalIdentity, User } from '../../types.js'; import { loginForm } from '../formats/html.js'; import { isValidRedirect } from '../utilities.js'; +/** + * The Login controller renders the basic login form + */ class LoginController extends Controller { async get(ctx: Context) { diff --git a/src/login/controller/mfa.ts b/src/login/controller/mfa.ts index 095e57c7..caf98cdd 100644 --- a/src/login/controller/mfa.ts +++ b/src/login/controller/mfa.ts @@ -8,6 +8,11 @@ import { MFALoginSession } from '../../mfa/types.js'; import { mfaForm } from '../formats/html.js'; import { getLoggerFromContext } from '../../log/service.js'; +/** + * Multi-factor-auth controller + * + * Handles both TOTP and Webauthn + */ class MFAController extends Controller { async get(ctx: Context) { diff --git a/src/login/service.ts b/src/login/service.ts index 16854c86..afeb3f00 100644 --- a/src/login/service.ts +++ b/src/login/service.ts @@ -1,5 +1,5 @@ import { BadRequest, NotFound } from '@curveball/http-errors'; -import { LoginSession, LoginChallengeContext } from './types.js'; +import { LoginSession } from './types.js'; import { AuthorizationChallengeRequest } from '../api-types.js'; import { InvalidGrant } from '../oauth2/errors.js'; import { OAuth2Code } from '../oauth2/types.js'; @@ -11,6 +11,7 @@ import { LoginChallengePassword } from './challenge/password.js'; import { LoginChallengeTotp } from './challenge/totp.js'; import { A12nLoginChallengeError } from './error.js'; import { AbstractLoginChallenge } from './challenge/abstract.js'; +import { UserEventLogger } from '../log/types.js'; type ChallengeRequest = AuthorizationChallengeRequest; @@ -75,14 +76,21 @@ export async function challenge(client: AppClient, session: LoginSession, parame // event. const logSessionStart = !session.principalId; - const loginContext = await initChallengeContext( + let { + principal, + identity, + dirty, + log, + } = await initChallengeContext( session, parameters ); + session.principalId = principal.id; + session.principalIdentityId = identity.id; - if (logSessionStart) loginContext.log('login-challenge-started'); + if (logSessionStart) log('login-challenge-started'); - const challenges = await getChallengesForPrincipal(loginContext.principal); + const challenges = await getChallengesForPrincipal(principal, log); if (challenges.length === 0) { throw new A12nLoginChallengeError( @@ -96,50 +104,50 @@ export async function challenge(client: AppClient, session: LoginSession, parame try { for(const challenge of challenges) { - if (loginContext.session.challengesCompleted.includes(challenge.authFactor)) { + if (session.challengesCompleted.includes(challenge.authFactor)) { // This challenge has already been checked previously. continue; } if (challenge.parametersContainsResponse(parameters)) { // The user did submit a value for this challenge. Lets check it. - const challengeResult = await challenge.checkResponse(loginContext); + const challengeResult = await challenge.checkResponse(session, parameters); if (challengeResult) { // Challenge passed. - loginContext.session.challengesCompleted.push(challenge.authFactor); - loginContext.dirty = true; + session.challengesCompleted.push(challenge.authFactor); + dirty = true; } } } - const completedChallenges = new Set(loginContext.session.challengesCompleted); + const completedChallenges = new Set(session.challengesCompleted); if (completedChallenges.size < 2 && challenges.length > 1) { // If there are 2 or more auth factors set up, we want at least 2 successful // passes. If this is not the case we're going to emit a challenge error. for(const challenge of challenges) { - if (!loginContext.session.challengesCompleted.includes(challenge.authFactor)) { - challenge.challenge(loginContext.session); + if (!session.challengesCompleted.includes(challenge.authFactor)) { + challenge.challenge(session); } } } - loginContext.log('login-challenge-success'); + log('login-challenge-success'); } finally { - if (loginContext.dirty) { - await storeSession(loginContext.session); - loginContext.dirty = false; + if (dirty) { + await storeSession(session); + dirty = false; } } - await deleteSession(loginContext.session); + await deleteSession(session); return await services.oauth2.generateAuthorizationCode({ client, - principal: loginContext.principal, + principal, scope: session.scope ?? [], redirectUri: null, grantType: 'authorization_challenge', @@ -192,7 +200,7 @@ async function deleteSession(session: LoginSession) { } -async function initChallengeContext(session: LoginSession, parameters: ChallengeRequest): Promise { +async function initChallengeContext(session: LoginSession, parameters: ChallengeRequest) { let principal; let identity; @@ -259,12 +267,6 @@ async function initChallengeContext(session: LoginSession, parameters: Challenge principal, identity, log, - session: { - ...session, - principalId: identity.principal.id, - principalIdentityId: identity.id, - }, - parameters, dirty, }; @@ -273,11 +275,11 @@ async function initChallengeContext(session: LoginSession, parameters: Challenge /** * Returns the full list of login challenges the user has setup up. */ -async function getChallengesForPrincipal(principal: User): Promise[]> { +async function getChallengesForPrincipal(principal: User, log: UserEventLogger): Promise[]> { const challenges = [ - new LoginChallengePassword(principal), - new LoginChallengeTotp(principal) + new LoginChallengePassword(principal, log), + new LoginChallengeTotp(principal, log) ]; const result = []; From 9b625b76f18c6672338afd68a8288d16f2a19305 Mon Sep 17 00:00:00 2001 From: Evert Pot Date: Tue, 7 Jan 2025 23:16:15 -0500 Subject: [PATCH 10/10] Remove dirty flag and simply always update the session state. --- src/login/service.ts | 12 ++---------- src/login/types.ts | 41 ----------------------------------------- 2 files changed, 2 insertions(+), 51 deletions(-) diff --git a/src/login/service.ts b/src/login/service.ts index afeb3f00..a311471f 100644 --- a/src/login/service.ts +++ b/src/login/service.ts @@ -76,10 +76,9 @@ export async function challenge(client: AppClient, session: LoginSession, parame // event. const logSessionStart = !session.principalId; - let { + const { principal, identity, - dirty, log, } = await initChallengeContext( session, @@ -114,7 +113,6 @@ export async function challenge(client: AppClient, session: LoginSession, parame if (challengeResult) { // Challenge passed. session.challengesCompleted.push(challenge.authFactor); - dirty = true; } } @@ -136,10 +134,7 @@ export async function challenge(client: AppClient, session: LoginSession, parame } finally { - if (dirty) { - await storeSession(session); - dirty = false; - } + await storeSession(session); } @@ -204,7 +199,6 @@ async function initChallengeContext(session: LoginSession, parameters: Challenge let principal; let identity; - let dirty = false; const ps = new services.principal.PrincipalService('insecure'); if (session.principalIdentityId && session.principalId) { @@ -233,7 +227,6 @@ async function initChallengeContext(session: LoginSession, parameters: Challenge throw err; } } - dirty = true; } if (principal.type !== 'user') { throw new A12nLoginChallengeError( @@ -267,7 +260,6 @@ async function initChallengeContext(session: LoginSession, parameters: Challenge principal, identity, log, - dirty, }; } diff --git a/src/login/types.ts b/src/login/types.ts index d0d63fa0..c5ca2e9a 100644 --- a/src/login/types.ts +++ b/src/login/types.ts @@ -1,7 +1,4 @@ -import { User, PrincipalIdentity } from '../types.js'; import { AuthFactorType } from '../user-auth-factor/types.js'; -import { UserEventLogger } from '../log/types.js'; -import { AuthorizationChallengeRequest } from '../api-types.js'; export { AuthorizationChallengeRequest } from '../api-types.js'; /** @@ -68,41 +65,3 @@ export type LoginSessionWithPrincipal = LoginSession & { */ principalIdentityId: number; } - - -export type LoginChallengeContext = { - - /** - * The user that's trying to authenticate - */ - principal: User; - - /** - * The identity (email address usually) of the principal that was used to - * start this challenge, usually supplied as a username. - */ - identity: PrincipalIdentity; - - /** - * Easy access to a logger - */ - log: UserEventLogger; - - /** - * Parameters sent with the *current* request. - */ - parameters: AuthorizationChallengeRequest; - - /** - * Session data associated with the login challenge process. - * This is persistent data. - */ - session: LoginSession; - - /** - * Set to true if session data changed and we need to save - * it again. - */ - dirty: boolean; - -}