From 33288d2629c953dbb59581a631739c57e04cf7d8 Mon Sep 17 00:00:00 2001 From: yunhwan Date: Mon, 7 Oct 2024 23:24:08 -0400 Subject: [PATCH] review: consolidate validatePassword and loginActivity logic into a single userService method, replacing existing implementations --- .../controller/authorization-challenge.ts | 6 +-- src/login/controller/login.ts | 25 ++------- src/login/service.ts | 35 ++++++------ src/oauth2/controller/token.ts | 28 +++++----- src/user/service.ts | 53 ++++++++++++++++++- 5 files changed, 91 insertions(+), 56 deletions(-) diff --git a/src/login/controller/authorization-challenge.ts b/src/login/controller/authorization-challenge.ts index 56156ddf..aa74fccf 100644 --- a/src/login/controller/authorization-challenge.ts +++ b/src/login/controller/authorization-challenge.ts @@ -1,9 +1,9 @@ import { Controller } from '@curveball/controller'; +import { UnsupportedMediaType } from '@curveball/http-errors'; import { Context } from '@curveball/kernel'; +import { AuthorizationChallengeRequest } from '../../api-types.js'; import { getAppClientFromBasicAuth } from '../../app-client/service.js'; import { UnauthorizedClient } from '../../oauth2/errors.js'; -import { UnsupportedMediaType } from '@curveball/http-errors'; -import { AuthorizationChallengeRequest } from '../../api-types.js'; import * as loginService from '../service.js'; /** @@ -44,7 +44,7 @@ class AuthorizationChallengeController extends Controller { const request = ctx.request.body; const session = await loginService.getSession(client, request); - const code = await loginService.challenge(client, session, request); + const code = await loginService.challenge(client, session, request, ctx); ctx.response.body = { authorization_code: code.code, diff --git a/src/login/controller/login.ts b/src/login/controller/login.ts index 95e80ce3..919290c1 100644 --- a/src/login/controller/login.ts +++ b/src/login/controller/login.ts @@ -7,7 +7,7 @@ import { EventType } from '../../log/types.js'; import { MFALoginSession } from '../../mfa/types.js'; import * as webAuthnService from '../../mfa/webauthn/service.js'; import { hasUsers, PrincipalService } from '../../principal/service.js'; -import { getSetting, requireSetting } from '../../server-settings.js'; +import { getSetting } from '../../server-settings.js'; import * as services from '../../services.js'; import { PrincipalIdentity, User } from '../../types.js'; import { loginForm } from '../formats/html.js'; @@ -59,24 +59,6 @@ class LoginController extends Controller { const user = (await principalService.findByIdentity(identity)) as User; - if (await services.loginActivity.isAccountLocked(user)) { - await services.loginActivity.incrementFailedLoginAttempts(user); - log(EventType.loginFailedAccountLocked, ctx.ip(), user.id, ctx.request.headers.get('User-Agent')); - return this.redirectToLogin(ctx, '', `Too many failed login attempts, please contact ${requireSetting('smtp.emailFrom')} to unlock your account.`); - } - - if (!await services.user.validatePassword(user, ctx.request.body.password)) { - const incrementedAttempts = await services.loginActivity.incrementFailedLoginAttempts(user); - - if (services.loginActivity.reachedMaxAttempts(incrementedAttempts)) { - log(EventType.accountLocked, ctx.ip(), user.id, ctx.request.headers.get('User-Agent')); - return this.redirectToLogin(ctx, '', `Too many failed login attempts, please contact ${requireSetting('smtp.emailFrom')} to unlock your account.`); - } - - log(EventType.loginFailed, ctx.ip(), user.id); - return this.redirectToLogin(ctx, '', 'Incorrect username or password'); - } - if (!user.active) { log(EventType.loginFailedInactive, ctx.ip(), user.id, ctx.request.headers.get('User-Agent')); return this.redirectToLogin(ctx, '', 'This account is inactive. Please contact Admin'); @@ -90,7 +72,10 @@ class LoginController extends Controller { return; } - await services.loginActivity.resetFailedLoginAttempts(user); + const { success, errorMessage } = await services.user.validateUserCredentials(user, ctx.request.body.password, ctx); + if (!success && errorMessage) { + return this.redirectToLogin(ctx, '', errorMessage); + } ctx.session = { user: user, diff --git a/src/login/service.ts b/src/login/service.ts index 29c9421c..3ddeb426 100644 --- a/src/login/service.ts +++ b/src/login/service.ts @@ -1,10 +1,11 @@ -import { AppClient, Principal, PrincipalIdentity, User } from '../types.js'; -import { getSessionStore } from '../session-store.js'; -import { InvalidGrant, OAuth2Error } from '../oauth2/errors.js'; -import * as services from '../services.js'; import { BadRequest, NotFound } from '@curveball/http-errors'; +import { Context } from '@curveball/kernel'; import { AuthorizationChallengeRequest } from '../api-types.js'; +import { InvalidGrant, OAuth2Error } from '../oauth2/errors.js'; import { OAuth2Code } from '../oauth2/types.js'; +import * as services from '../services.js'; +import { getSessionStore } from '../session-store.js'; +import { AppClient, Principal, PrincipalIdentity, User } from '../types.js'; type ChallengeRequest = AuthorizationChallengeRequest; @@ -131,7 +132,7 @@ async function deleteSession(session: LoginSession) { * If more credentials are needed or if any information is incorrect, an error * will be thrown. */ -export async function challenge(client: AppClient, session: LoginSession, parameters: ChallengeRequest): Promise { +export async function challenge(client: AppClient, session: LoginSession, parameters: ChallengeRequest, ctx: Context): Promise { try { if (!session.principalId) { @@ -148,6 +149,7 @@ export async function challenge(client: AppClient, session: LoginSession, parame session, parameters.username!, parameters.password!, + ctx, ); } @@ -183,7 +185,7 @@ export async function challenge(client: AppClient, session: LoginSession, parame } -async function challengeUsernamePassword(session: LoginSession, username: string, password: string): Promise { +async function challengeUsernamePassword(session: LoginSession, username: string, password: string, ctx: Context): Promise { const principalService = new services.principal.PrincipalService('insecure'); let user: Principal; @@ -228,21 +230,11 @@ async function challengeUsernamePassword(session: LoginSession, username: string ); } - if (!await services.user.validatePassword(user, password)) { - throw new A12nLoginChallengeError( - session, - 'Incorrect username or password', - 'username-password', - true, - ); - } - session.principalId = user.id; session.passwordValid = true; session.dirty = true; if (!user.active) { - throw new A12nLoginChallengeError( session, 'This account is not active. Please contact support', @@ -258,6 +250,17 @@ async function challengeUsernamePassword(session: LoginSession, username: string true ); } + + const { success, errorMessage } = await services.user.validateUserCredentials(user, password, ctx); + if (!success && errorMessage) { + throw new A12nLoginChallengeError( + session, + errorMessage, + 'username-password', + true, + ); + } + return user; } diff --git a/src/oauth2/controller/token.ts b/src/oauth2/controller/token.ts index 81454e82..857f903a 100644 --- a/src/oauth2/controller/token.ts +++ b/src/oauth2/controller/token.ts @@ -1,19 +1,19 @@ import Controller from '@curveball/controller'; import { Context } from '@curveball/core'; import { NotFound } from '@curveball/http-errors'; +import { + getAppClientFromBasicAuth, + getAppClientFromBody, +} from '../../app-client/service.js'; import log from '../../log/service.js'; import { EventType } from '../../log/types.js'; +import * as principalIdentityService from '../../principal-identity/service.js'; import { PrincipalService } from '../../principal/service.js'; +import { AppClient, User } from '../../types.js'; +import * as userAppPermissions from '../../user-app-permissions/service.js'; import * as userService from '../../user/service.js'; -import { User, AppClient } from '../../types.js'; import { InvalidGrant, InvalidRequest, UnsupportedGrantType } from '../errors.js'; import * as oauth2Service from '../service.js'; -import { - getAppClientFromBasicAuth, - getAppClientFromBody, -} from '../../app-client/service.js'; -import * as userAppPermissions from '../../user-app-permissions/service.js'; -import * as principalIdentityService from '../../principal-identity/service.js'; class TokenController extends Controller { @@ -126,15 +126,6 @@ class TokenController extends Controller { throw new InvalidGrant('Unknown username or password'); } - if (!await userService.validatePassword(user, ctx.request.body.password)) { - await log( - EventType.loginFailed, - ctx.ip(), - user.id - ); - throw new InvalidGrant('Unknown username or password'); - } - if (!user.active) { await log( EventType.loginFailedInactive, @@ -145,6 +136,11 @@ class TokenController extends Controller { throw new InvalidGrant('User Inactive'); } + const { success, errorMessage } = await userService.validateUserCredentials(user, ctx.request.body.password, ctx); + if (!success && errorMessage) { + throw new InvalidGrant(errorMessage); + } + await log( EventType.loginSuccess, ctx.ip(), diff --git a/src/user/service.ts b/src/user/service.ts index 9933aba4..dfba8b8a 100644 --- a/src/user/service.ts +++ b/src/user/service.ts @@ -1,7 +1,12 @@ +import { Context } from '@curveball/core'; +import { UnprocessableContent } from '@curveball/http-errors'; import * as bcrypt from 'bcrypt'; import db from '../database.js'; +import log from '../log/service.js'; +import { EventType } from '../log/types.js'; +import * as loginActivityService from '../login/login-activity/service.js'; +import { requireSetting } from '../server-settings.js'; import { User } from '../types.js'; -import { UnprocessableContent } from '@curveball/http-errors'; export async function createPassword(user: User, password: string): Promise { @@ -67,3 +72,49 @@ function assertValidPassword(password: string) { } } + +type AuthenticationResult = { + success: boolean; + errorMessage?: string; +} + +/** + * Validate the user password and handle login attempts. + */ +export async function validateUserCredentials(user: User, password: string, ctx: Context): Promise { + const TOO_MANY_FAILED_ATTEMPTS = `Too many failed login attempts, please contact ${requireSetting('smtp.emailFrom')} to unlock your account.`; + + if (await loginActivityService.isAccountLocked(user)) { + await loginActivityService.incrementFailedLoginAttempts(user); + log(EventType.loginFailedAccountLocked, ctx.ip(), user.id, ctx.request.headers.get('User-Agent')); + return { + success: false, + errorMessage: TOO_MANY_FAILED_ATTEMPTS, + }; + } + + if (!await validatePassword(user, password)) { + const incrementedAttempts = await loginActivityService.incrementFailedLoginAttempts(user); + + if (loginActivityService.reachedMaxAttempts(incrementedAttempts)) { + log(EventType.accountLocked, ctx.ip(), user.id, ctx.request.headers.get('User-Agent')); + return { + success: false, + errorMessage: TOO_MANY_FAILED_ATTEMPTS, + }; + } + + log(EventType.loginFailed, ctx.ip(), user.id); + return { + success: false, + errorMessage: 'Incorrect username or password', + }; + } + + await loginActivityService.resetFailedLoginAttempts(user); + + return { + success: true, + }; +} +