Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block user accounts if an incorrect password was entered 5 times #527

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion src/db-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ interface Tables {
server_settings: ServerSettingsRecord;
user_app_permissions: UserAppPermissionsRecord;
user_log: UserLogRecord;
user_login_activity: UserLoginActivityRecord;
user_passwords: UserPasswordsRecord;
user_privileges: UserPrivilegesRecord;
user_totp: UserTotpRecord;
Expand Down Expand Up @@ -158,7 +159,7 @@ export type PrincipalsRecord = {
modified_at: number;

/**
* System are built-in and cannot be deleted
* System principals are built-in and cannot be deleted
*/
system: number;
};
Expand Down Expand Up @@ -212,6 +213,23 @@ export type UserLogRecord = {
user_agent: string | null;
country: string | null;
};
export type UserLoginActivityRecord = {

/**
* Foreign key to the ‘principals’ table, representing the user
*/
principal_id: number;

/**
* Tracks the number of consecutive failed login attempts
*/
failed_login_attempts: number;

/**
* Indicates if the user's account is currently locked due to suspicious activity, such as too many failed login attempts
*/
account_locked: number;
};
export type UserPasswordsRecord = {
user_id: number;
password: string;
Expand Down
5 changes: 5 additions & 0 deletions src/log/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ export enum EventType {

oauth2BadRedirect = 11,
generateAccessToken = 12,

accountLocked = 13,
loginFailedAccountLocked = 14,
Comment on lines +16 to +17
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth adding some comments to these 2 lines so it's a bit clearer what the difference is.

}

export type LogEntry = {
Expand All @@ -34,4 +37,6 @@ export const eventTypeString = new Map<EventType, string>([
[EventType.loginFailedNotVerified,'login-failed-notverified'],
[EventType.tokenRevoked, 'token-revoked'],
[EventType.oauth2BadRedirect, 'oauth2-badredirect'],
[EventType.accountLocked, 'account-locked'],
[EventType.loginFailedAccountLocked, 'login-failed-account-locked'],
]);
25 changes: 20 additions & 5 deletions src/login/controller/login.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,12 @@ import log from '../../log/service.js';
import { EventType } from '../../log/types.js';
import { MFALoginSession } from '../../mfa/types.js';
import * as webAuthnService from '../../mfa/webauthn/service.js';
import { getSetting } from '../../server-settings.js';
import { hasUsers, PrincipalService } from '../../principal/service.js';
import { getSetting, requireSetting } from '../../server-settings.js';
import * as services from '../../services.js';
import { PrincipalIdentity, User } from '../../types.js';
import { isValidRedirect } from '../utilities.js';
import { loginForm } from '../formats/html.js';
import * as services from '../../services.js';
import { isValidRedirect } from '../utilities.js';

class LoginController extends Controller {

Expand Down Expand Up @@ -56,9 +56,23 @@ class LoginController extends Controller {
throw err;
}
}
const user = await principalService.findByIdentity(identity) as User;

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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit of a larger request, but I wonder if most of this logic should go into the service. The reason is that this is not the only place where we check a username and password, there are two more places:

  • OAuth2 has a password flow
  • I recently added a 'authorization_challenge` OAuth2 flow.

These 3 features all do a username/password check so it kinda makes sense that they all increment the 'failed login' counter if a password fails.

I'm not sure if this should just be part of 'validatePassword' or a new function.

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');
}
Expand All @@ -76,6 +90,8 @@ class LoginController extends Controller {
return;
}

await services.loginActivity.resetFailedLoginAttempts(user);

ctx.session = {
user: user,
};
Expand All @@ -88,7 +104,6 @@ class LoginController extends Controller {
}

ctx.response.redirect(303, getSetting('login.defaultRedirect'));

}

redirectToLogin(ctx: Context<any>, msg: string, error: string) {
Expand Down
65 changes: 65 additions & 0 deletions src/login/login-activity/service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import { UserLoginActivityRecord } from 'knex/types/tables.js';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is login-attempts a better name for this?

import db from '../../database.js';
import { User } from '../../types.js';

const MAX_FAILED_ATTEMPTS = 5;

export function reachedMaxAttempts(attempts: number) {
return attempts === MAX_FAILED_ATTEMPTS;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be >= instead of ===

}

async function getLoginActivity(user: User): Promise<UserLoginActivityRecord | undefined> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of returning undefined, how about always returning a record? Because maybe if there's no database record yet, you can guess what the record should be for the initial state

return await db<UserLoginActivityRecord>('user_login_activity')
.where({ principal_id: user.id })
.first();
}

async function ensureUserLoginActivityRecord(user: User): Promise<void> {
await db('user_login_activity')
.insert({
principal_id: user.id,
failed_login_attempts: 0,
account_locked: 0,
})
.onConflict('principal_id')
.ignore();
}

export async function incrementFailedLoginAttempts(user: User): Promise<number> {
await ensureUserLoginActivityRecord(user);

return await db.transaction(async (trx) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


const loginActivity = await trx('user_login_activity')
.where({ principal_id: user.id })
.forUpdate()
.first();

const newAttempts = loginActivity!.failed_login_attempts + 1;

await trx('user_login_activity')
.where({ principal_id: user.id })
.update({
failed_login_attempts: newAttempts,
account_locked: newAttempts >= MAX_FAILED_ATTEMPTS ? 1 : 0,
});

return newAttempts;
});
}

export async function resetFailedLoginAttempts(user: User): Promise<void> {
await db('user_login_activity')
.where({ principal_id: user.id })
.update({
failed_login_attempts: 0,
account_locked: 0,
});
}

export async function isAccountLocked(user: User): Promise<boolean> {
await ensureUserLoginActivityRecord(user);

const loginActivity = await getLoginActivity(user);
return !!loginActivity?.account_locked;
}
34 changes: 34 additions & 0 deletions src/migrations/20240908210700_block_failed_login_attempts.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { Knex } from 'knex';

export async function up(knex: Knex): Promise<void> {
await knex.schema.createTable('user_login_activity', (table) => {
table
.integer('principal_id')
.unsigned()
.notNullable()
.primary()
.comment('Foreign key to the ‘principals’ table, representing the user');
table
.foreign('principal_id')
.references('id')
.inTable('principals')
.onDelete('CASCADE');
table
.integer('failed_login_attempts')
.unsigned()
.defaultTo(0)
.notNullable()
.comment('Tracks the number of consecutive failed login attempts');
table
.boolean('account_locked')
.defaultTo(false)
.notNullable()
.comment(
"Indicates if the user's account is currently locked due to suspicious activity, such as too many failed login attempts"
);
});
}

export async function down(knex: Knex): Promise<void> {
await knex.schema.dropTable('user_login_activity');
}
5 changes: 3 additions & 2 deletions src/services.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
export * as appClient from './app-client/service.js';
export * as log from './log/service.js';
export * as loginActivity from './login/login-activity/service.js';
export * as mfaTotp from './mfa/totp/service.js';
export * as oauth2 from './oauth2/service.js';
export * as principal from './principal/service.js';
export * as principalIdentity from './principal-identity/service.js';
export * as principal from './principal/service.js';
export * as privilege from './privilege/service.js';
export * as resetPassword from './reset-password/service.js';
export * as user from './user/service.js';
export * as userAuthFactor from './user-auth-factor/service.js';
export * as user from './user/service.js';
export * as verificationToken from './verification-token/service.js';