mirror of
https://github.com/bookorbit/bookorbit.git
synced 2026-11-03 00:11:37 -06:00
feat(auth): add self-registration toggle and sign-up page
Wire the existing allow_registration setting to an admin toggle in the Users tab and a public /register page, so the documented self-registration flow is reachable from the UI instead of the API alone. Expose the flag on the public setup-status endpoint the router guard already calls, so the sign-in page can offer sign-up without an extra request. Harden the newly public path: return a conflict instead of a 500 when concurrent signups race the unique index, sanitize the logged username, and reject control, zero-width, and text-direction characters in the username and display name so a new account cannot impersonate another in the user list or audit log. Closes #952
This commit is contained in:
@@ -0,0 +1,16 @@
|
||||
/**
|
||||
* Identity fields chosen by anonymous visitors are rendered back to administrators in the user
|
||||
* list and the audit log, so they must not be able to carry invisible or direction-flipping
|
||||
* characters that let one account impersonate another. Unicode letters stay allowed: BookOrbit is
|
||||
* multilingual and legitimate usernames are written in every script.
|
||||
*/
|
||||
const CONTROL = '\\u0000-\\u001F\\u007F';
|
||||
const ZERO_WIDTH_AND_BIDI = '\\u200B-\\u200F\\u202A-\\u202E\\u2066-\\u2069\\uFEFF';
|
||||
|
||||
export const DISPLAY_SAFE_TEXT_PATTERN = new RegExp(`^[^${CONTROL}${ZERO_WIDTH_AND_BIDI}]+$`, 'u');
|
||||
|
||||
export const DISPLAY_SAFE_TEXT_MESSAGE = 'must not contain control, zero-width, or text-direction characters';
|
||||
|
||||
export const TRIMMED_TEXT_PATTERN = /^\S(.*\S)?$/u;
|
||||
|
||||
export const TRIMMED_TEXT_MESSAGE = 'must not start or end with whitespace';
|
||||
@@ -93,6 +93,7 @@ function makeService(dbOverrides?: Record<string, unknown>) {
|
||||
const appSettings = {
|
||||
getOidcConfig: vi.fn().mockResolvedValue({ enabled: false }),
|
||||
getDefaultLibraryAccessLibraryIds: vi.fn().mockResolvedValue([]),
|
||||
getValue: vi.fn().mockResolvedValue(null),
|
||||
};
|
||||
const oidcSessionRepo = {
|
||||
findActiveByUserId: vi.fn().mockResolvedValue(null),
|
||||
@@ -154,14 +155,33 @@ describe('AuthService', () => {
|
||||
const { service, db } = makeService();
|
||||
((db as unknown as Record<string, unknown>).$count as vi.Mock).mockResolvedValue(0);
|
||||
|
||||
await expect(service.setupStatus()).resolves.toEqual({ needsSetup: true });
|
||||
await expect(service.setupStatus()).resolves.toEqual({ needsSetup: true, allowRegistration: false });
|
||||
});
|
||||
|
||||
it('returns needsSetup=false when at least one user exists', async () => {
|
||||
const { service, db } = makeService();
|
||||
((db as unknown as Record<string, unknown>).$count as vi.Mock).mockResolvedValue(1);
|
||||
|
||||
await expect(service.setupStatus()).resolves.toEqual({ needsSetup: false });
|
||||
await expect(service.setupStatus()).resolves.toEqual({ needsSetup: false, allowRegistration: false });
|
||||
});
|
||||
|
||||
it('reports allowRegistration=true when the setting is enabled', async () => {
|
||||
const { service, db, appSettings } = makeService();
|
||||
((db as unknown as Record<string, unknown>).$count as vi.Mock).mockResolvedValue(1);
|
||||
appSettings.getValue.mockResolvedValue('true');
|
||||
|
||||
await expect(service.setupStatus()).resolves.toEqual({ needsSetup: false, allowRegistration: true });
|
||||
expect(appSettings.getValue).toHaveBeenCalledWith('allow_registration');
|
||||
});
|
||||
|
||||
it('treats an unset or non-boolean registration setting as closed', async () => {
|
||||
const { service, db, appSettings } = makeService();
|
||||
((db as unknown as Record<string, unknown>).$count as vi.Mock).mockResolvedValue(1);
|
||||
|
||||
for (const value of [null, '', 'TRUE', 'yes', '1']) {
|
||||
appSettings.getValue.mockResolvedValue(value);
|
||||
await expect(service.setupStatus()).resolves.toEqual({ needsSetup: false, allowRegistration: false });
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -229,17 +249,37 @@ describe('AuthService', () => {
|
||||
|
||||
describe('register', () => {
|
||||
it('throws ForbiddenException when registration is closed', async () => {
|
||||
const { service, db } = makeService();
|
||||
(db.query as never as Record<string, Record<string, vi.Mock>>).appSettings.findFirst.mockResolvedValue({ value: 'false' });
|
||||
const { service, appSettings } = makeService();
|
||||
appSettings.getValue.mockResolvedValue('false');
|
||||
|
||||
await expect(service.register({ username: 'u', name: 'U', password: 'P@ssw0rd!', email: 'u@example.com' } as never)).rejects.toThrow(
|
||||
ForbiddenException,
|
||||
);
|
||||
});
|
||||
|
||||
it('throws ForbiddenException when the registration setting is missing', async () => {
|
||||
const { service, appSettings } = makeService();
|
||||
appSettings.getValue.mockResolvedValue(null);
|
||||
|
||||
await expect(service.register({ username: 'u', name: 'U', password: 'P@ssw0rd!', email: 'u@example.com' } as never)).rejects.toThrow(
|
||||
ForbiddenException,
|
||||
);
|
||||
});
|
||||
|
||||
it('does not create a user when registration is closed', async () => {
|
||||
const { service, db, appSettings } = makeService();
|
||||
appSettings.getValue.mockResolvedValue('false');
|
||||
|
||||
await expect(service.register({ username: 'u', name: 'U', password: 'P@ssw0rd!', email: 'u@example.com' } as never)).rejects.toThrow(
|
||||
ForbiddenException,
|
||||
);
|
||||
expect((db as unknown as { transaction: vi.Mock }).transaction).not.toHaveBeenCalled();
|
||||
expect((db as unknown as { insert: vi.Mock }).insert).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('throws ConflictException when username already exists', async () => {
|
||||
const { service, db } = makeService();
|
||||
(db.query as never as Record<string, Record<string, vi.Mock>>).appSettings.findFirst.mockResolvedValue({ value: 'true' });
|
||||
const { service, db, appSettings } = makeService();
|
||||
appSettings.getValue.mockResolvedValue('true');
|
||||
(db.query as never as Record<string, Record<string, vi.Mock>>).users.findFirst.mockResolvedValueOnce({ id: 99, username: 'existing' });
|
||||
|
||||
await expect(
|
||||
@@ -248,8 +288,8 @@ describe('AuthService', () => {
|
||||
});
|
||||
|
||||
it('throws ConflictException when email already in use', async () => {
|
||||
const { service, db } = makeService();
|
||||
(db.query as never as Record<string, Record<string, vi.Mock>>).appSettings.findFirst.mockResolvedValue({ value: 'true' });
|
||||
const { service, db, appSettings } = makeService();
|
||||
appSettings.getValue.mockResolvedValue('true');
|
||||
(db.query as never as Record<string, Record<string, vi.Mock>>).users.findFirst
|
||||
.mockResolvedValueOnce(null)
|
||||
.mockResolvedValueOnce({ id: 88, email: 'existing@example.com' });
|
||||
@@ -260,8 +300,8 @@ describe('AuthService', () => {
|
||||
});
|
||||
|
||||
it('registers user successfully', async () => {
|
||||
const { service, db } = makeService();
|
||||
(db.query as never as Record<string, Record<string, vi.Mock>>).appSettings.findFirst.mockResolvedValue({ value: 'true' });
|
||||
const { service, db, appSettings } = makeService();
|
||||
appSettings.getValue.mockResolvedValue('true');
|
||||
(db.query as never as Record<string, Record<string, vi.Mock>>).users.findFirst.mockResolvedValueOnce(null);
|
||||
((db as unknown as Record<string, unknown>).returning as vi.Mock).mockResolvedValueOnce([{ id: 1, username: 'jdoe', name: 'John Doe' }]);
|
||||
|
||||
@@ -269,9 +309,75 @@ describe('AuthService', () => {
|
||||
expect(result).toEqual({ id: 1, username: 'jdoe', name: 'John Doe' });
|
||||
});
|
||||
|
||||
it('reports a conflict when a concurrent signup wins the unique index race', async () => {
|
||||
const { service, db, appSettings } = makeService();
|
||||
appSettings.getValue.mockResolvedValue('true');
|
||||
(db.query as never as Record<string, Record<string, vi.Mock>>).users.findFirst.mockResolvedValue(null);
|
||||
const pgError = Object.assign(new Error('duplicate key value violates unique constraint'), { code: '23505' });
|
||||
((db as unknown as Record<string, unknown>).transaction as vi.Mock).mockRejectedValue(pgError);
|
||||
|
||||
await expect(
|
||||
service.register({ username: 'jdoe', name: 'John Doe', password: 'P@ssw0rd!', email: 'jdoe@example.com' } as never),
|
||||
).rejects.toThrow(ConflictException);
|
||||
});
|
||||
|
||||
it('detects a unique violation wrapped as an error cause', async () => {
|
||||
const { service, db, appSettings } = makeService();
|
||||
appSettings.getValue.mockResolvedValue('true');
|
||||
(db.query as never as Record<string, Record<string, vi.Mock>>).users.findFirst.mockResolvedValue(null);
|
||||
((db as unknown as Record<string, unknown>).transaction as vi.Mock).mockRejectedValue(new Error('insert failed', { cause: { code: '23505' } }));
|
||||
|
||||
await expect(
|
||||
service.register({ username: 'jdoe', name: 'John Doe', password: 'P@ssw0rd!', email: 'jdoe@example.com' } as never),
|
||||
).rejects.toThrow(ConflictException);
|
||||
});
|
||||
|
||||
it('does not mask unrelated database failures as conflicts', async () => {
|
||||
const { service, db, appSettings } = makeService();
|
||||
appSettings.getValue.mockResolvedValue('true');
|
||||
(db.query as never as Record<string, Record<string, vi.Mock>>).users.findFirst.mockResolvedValue(null);
|
||||
((db as unknown as Record<string, unknown>).transaction as vi.Mock).mockRejectedValue(
|
||||
Object.assign(new Error('connection terminated'), { code: '08006' }),
|
||||
);
|
||||
|
||||
await expect(
|
||||
service.register({ username: 'jdoe', name: 'John Doe', password: 'P@ssw0rd!', email: 'jdoe@example.com' } as never),
|
||||
).rejects.toThrow('connection terminated');
|
||||
});
|
||||
|
||||
it('never grants superuser or explicit permissions to a self-registered account', async () => {
|
||||
const { service, db, appSettings } = makeService();
|
||||
appSettings.getValue.mockResolvedValue('true');
|
||||
(db.query as never as Record<string, Record<string, vi.Mock>>).users.findFirst.mockResolvedValueOnce(null);
|
||||
((db as unknown as Record<string, unknown>).returning as vi.Mock).mockResolvedValueOnce([{ id: 1, username: 'jdoe', name: 'John Doe' }]);
|
||||
|
||||
await service.register({ username: 'jdoe', name: 'John Doe', password: 'P@ssw0rd!', email: 'jdoe@example.com' } as never);
|
||||
|
||||
const insertedUser = ((db as unknown as { values: vi.Mock }).values.mock.calls[0] as [Record<string, unknown>])[0];
|
||||
expect(insertedUser).not.toHaveProperty('isSuperuser');
|
||||
expect(insertedUser).not.toHaveProperty('active');
|
||||
expect(insertedUser).not.toHaveProperty('provisioningMethod');
|
||||
expect(insertedUser.passwordHash).toBe('mock-hash:P@ssw0rd!');
|
||||
});
|
||||
|
||||
it('neutralises newlines in the logged username', async () => {
|
||||
const { service, db, appSettings } = makeService();
|
||||
appSettings.getValue.mockResolvedValue('true');
|
||||
(db.query as never as Record<string, Record<string, vi.Mock>>).users.findFirst.mockResolvedValueOnce(null);
|
||||
const forged = 'evil\n[auth.login] [end] userId=1 - forged';
|
||||
((db as unknown as Record<string, unknown>).returning as vi.Mock).mockResolvedValueOnce([{ id: 1, username: forged, name: 'Evil' }]);
|
||||
const logSpy = vi.spyOn((service as unknown as { logger: { log: (msg: string) => void } }).logger, 'log');
|
||||
|
||||
await service.register({ username: forged, name: 'Evil', password: 'P@ssw0rd!', email: 'evil@example.com' } as never);
|
||||
|
||||
const logged = logSpy.mock.calls.at(-1)?.[0] as string;
|
||||
expect(logged).not.toContain('\n');
|
||||
expect(logged).toContain('evil [auth.login]');
|
||||
});
|
||||
|
||||
it('grants configured default library access to self-registered users', async () => {
|
||||
const { service, db, appSettings } = makeService();
|
||||
(db.query as never as Record<string, Record<string, vi.Mock>>).appSettings.findFirst.mockResolvedValue({ value: 'true' });
|
||||
appSettings.getValue.mockResolvedValue('true');
|
||||
(db.query as never as Record<string, Record<string, vi.Mock>>).users.findFirst.mockResolvedValueOnce(null);
|
||||
appSettings.getDefaultLibraryAccessLibraryIds.mockResolvedValue([2, 4]);
|
||||
((db as unknown as Record<string, unknown>).returning as vi.Mock).mockResolvedValueOnce([{ id: 1, username: 'jdoe', name: 'John Doe' }]);
|
||||
|
||||
@@ -26,6 +26,7 @@ import { DB } from '../../db/db.module';
|
||||
import * as schema from '../../db/schema';
|
||||
import { AUDIT_EVENT, AuditEventsService } from '../audit/audit-events.service';
|
||||
import type { RequestUser } from '../../common/types/request-user';
|
||||
import { sanitizeLogValue } from '../../common/utils/log-sanitize.utils';
|
||||
import { resolveUserAvatarUrl } from '../../common/utils/user-avatar-url';
|
||||
import { SystemMailService } from '../email/system-mail.service';
|
||||
import { UserService } from '../user/user.service';
|
||||
@@ -67,6 +68,16 @@ function maskEmail(email: string): string {
|
||||
return `${email[0]}***@${email.slice(at + 1)}`;
|
||||
}
|
||||
|
||||
function isUniqueViolation(error: unknown): boolean {
|
||||
if (!error || typeof error !== 'object') return false;
|
||||
|
||||
const directCode = (error as { code?: unknown }).code;
|
||||
if (directCode === '23505') return true;
|
||||
|
||||
if (!(error instanceof Error)) return false;
|
||||
return (error.cause as { code?: unknown } | undefined)?.code === '23505';
|
||||
}
|
||||
|
||||
@Injectable()
|
||||
export class AuthService {
|
||||
private readonly logger = new Logger(AuthService.name);
|
||||
@@ -84,16 +95,28 @@ export class AuthService {
|
||||
@Inject(DB) private readonly db: NodePgDatabase<typeof schema>,
|
||||
) {}
|
||||
|
||||
private async isRegistrationOpen(): Promise<boolean> {
|
||||
return (await this.appSettings.getValue(APP_SETTING_KEYS.ALLOW_REGISTRATION)) === 'true';
|
||||
}
|
||||
|
||||
async register(dto: RegisterDto) {
|
||||
const setting = await this.db.query.appSettings.findFirst({
|
||||
where: eq(schema.appSettings.key, APP_SETTING_KEYS.ALLOW_REGISTRATION),
|
||||
});
|
||||
if (setting?.value !== 'true') {
|
||||
if (!(await this.isRegistrationOpen())) {
|
||||
throw new ForbiddenException('Registration is not open');
|
||||
}
|
||||
|
||||
const defaultLibraryIds = await this.appSettings.getDefaultLibraryAccessLibraryIds();
|
||||
const passwordHash = await hash(dto.password, 12);
|
||||
try {
|
||||
return await this.registerInTransaction(dto, passwordHash, defaultLibraryIds);
|
||||
} catch (error) {
|
||||
// Two concurrent signups for the same identifier pass the pre-checks and collide on the
|
||||
// lower(username)/lower(email) unique indexes; report the loser as a conflict, not a 500.
|
||||
if (isUniqueViolation(error)) throw new ConflictException('Registration failed');
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
|
||||
private registerInTransaction(dto: RegisterDto, passwordHash: string, defaultLibraryIds: number[]) {
|
||||
return this.db.transaction(async (tx) => {
|
||||
const existingUsername = await tx.query.users.findFirst({
|
||||
where: eq(sql`lower(${schema.users.username})`, dto.username.toLowerCase()),
|
||||
@@ -125,7 +148,7 @@ export class AuthService {
|
||||
.onConflictDoNothing();
|
||||
}
|
||||
|
||||
this.logger.log(`[auth.register] [end] userId=${user.id} username=${user.username} - registration completed`);
|
||||
this.logger.log(`[auth.register] [end] userId=${user.id} username="${sanitizeLogValue(user.username)}" - registration completed`);
|
||||
|
||||
this.auditEvents.emit(AUDIT_EVENT, {
|
||||
userId: user.id,
|
||||
@@ -138,9 +161,9 @@ export class AuthService {
|
||||
});
|
||||
}
|
||||
|
||||
async setupStatus(): Promise<{ needsSetup: boolean }> {
|
||||
const count = await this.db.$count(schema.users);
|
||||
return { needsSetup: count === 0 };
|
||||
async setupStatus(): Promise<{ needsSetup: boolean; allowRegistration: boolean }> {
|
||||
const [count, allowRegistration] = await Promise.all([this.db.$count(schema.users), this.isRegistrationOpen()]);
|
||||
return { needsSetup: count === 0, allowRegistration };
|
||||
}
|
||||
|
||||
async setup(dto: SetupDto, setupToken: string | undefined, reply: FastifyReply) {
|
||||
|
||||
@@ -0,0 +1,65 @@
|
||||
import { plainToInstance } from 'class-transformer';
|
||||
import { validate } from 'class-validator';
|
||||
|
||||
import { RegisterDto } from './register.dto';
|
||||
|
||||
const VALID = { username: 'ada_lovelace', name: 'Ada Lovelace', email: 'ada@example.com', password: 'Passw0rd' };
|
||||
|
||||
async function failedProperties(overrides: Partial<Record<keyof typeof VALID, string>>): Promise<string[]> {
|
||||
const errors = await validate(plainToInstance(RegisterDto, { ...VALID, ...overrides }));
|
||||
return errors.map((error) => error.property);
|
||||
}
|
||||
|
||||
describe('RegisterDto', () => {
|
||||
it('accepts a plain account', async () => {
|
||||
await expect(failedProperties({})).resolves.toEqual([]);
|
||||
});
|
||||
|
||||
it.each(['José-García', '夏目漱石', 'Кирилл', 'أحمد', 'ada99', 'user.name', 'ada-lovelace', 'Ada Lovelace'])(
|
||||
'accepts the non-spoofing username %s',
|
||||
async (username) => {
|
||||
await expect(failedProperties({ username })).resolves.toEqual([]);
|
||||
},
|
||||
);
|
||||
|
||||
it.each([
|
||||
['leading whitespace', ' admin'],
|
||||
['trailing whitespace', 'admin '],
|
||||
['a right-to-left override', 'adm\u202Ein'],
|
||||
['a left-to-right override', 'adm\u202Din'],
|
||||
['a right-to-left isolate', 'adm\u2067in'],
|
||||
['a zero-width space', 'ad\u200Bmin'],
|
||||
['a zero-width joiner', 'ad\u200Dmin'],
|
||||
['a byte order mark', 'ad\uFEFFmin'],
|
||||
['a newline', 'evil\nadmin'],
|
||||
['a carriage return', 'evil\radmin'],
|
||||
['a null byte', 'ad\u0000min'],
|
||||
['a delete control character', 'ad\u007Fmin'],
|
||||
])('rejects a username with %s', async (_label, username) => {
|
||||
await expect(failedProperties({ username })).resolves.toEqual(['username']);
|
||||
});
|
||||
|
||||
it.each([
|
||||
['leading whitespace', ' Ada'],
|
||||
['trailing whitespace', 'Ada '],
|
||||
['a right-to-left override', 'Ada\u202ELovelace'],
|
||||
['a zero-width space', 'Ada\u200BLovelace'],
|
||||
['a newline', 'Ada\nLovelace'],
|
||||
])('rejects a display name with %s', async (_label, name) => {
|
||||
await expect(failedProperties({ name })).resolves.toEqual(['name']);
|
||||
});
|
||||
|
||||
it('still enforces the username length bounds', async () => {
|
||||
await expect(failedProperties({ username: 'ab' })).resolves.toEqual(['username']);
|
||||
await expect(failedProperties({ username: 'a'.repeat(101) })).resolves.toEqual(['username']);
|
||||
});
|
||||
|
||||
it('still enforces the password policy', async () => {
|
||||
await expect(failedProperties({ password: 'alllowercase1' })).resolves.toEqual(['password']);
|
||||
await expect(failedProperties({ password: 'Sh0rt' })).resolves.toEqual(['password']);
|
||||
});
|
||||
|
||||
it('still enforces a well-formed email', async () => {
|
||||
await expect(failedProperties({ email: 'not-an-email' })).resolves.toEqual(['email']);
|
||||
});
|
||||
});
|
||||
@@ -1,14 +1,25 @@
|
||||
import { IsEmail, IsString, Matches, MaxLength, MinLength } from 'class-validator';
|
||||
|
||||
import {
|
||||
DISPLAY_SAFE_TEXT_MESSAGE,
|
||||
DISPLAY_SAFE_TEXT_PATTERN,
|
||||
TRIMMED_TEXT_MESSAGE,
|
||||
TRIMMED_TEXT_PATTERN,
|
||||
} from '../../../common/constants/display-safe-text.constants';
|
||||
|
||||
export class RegisterDto {
|
||||
@IsString()
|
||||
@MinLength(3)
|
||||
@MaxLength(100)
|
||||
@Matches(DISPLAY_SAFE_TEXT_PATTERN, { message: `username ${DISPLAY_SAFE_TEXT_MESSAGE}` })
|
||||
@Matches(TRIMMED_TEXT_PATTERN, { message: `username ${TRIMMED_TEXT_MESSAGE}` })
|
||||
username: string;
|
||||
|
||||
@IsString()
|
||||
@MinLength(1)
|
||||
@MaxLength(255)
|
||||
@Matches(DISPLAY_SAFE_TEXT_PATTERN, { message: `name ${DISPLAY_SAFE_TEXT_MESSAGE}` })
|
||||
@Matches(TRIMMED_TEXT_PATTERN, { message: `name ${TRIMMED_TEXT_MESSAGE}` })
|
||||
name: string;
|
||||
|
||||
@IsEmail()
|
||||
|
||||
@@ -241,7 +241,7 @@ describe('Auth session security (e2e)', () => {
|
||||
});
|
||||
|
||||
expect(statusAfter.statusCode).toBe(200);
|
||||
expect(statusAfter.json()).toEqual({ needsSetup: false });
|
||||
expect(statusAfter.json()).toEqual({ needsSetup: false, allowRegistration: false });
|
||||
});
|
||||
|
||||
it('rejects setup when already completed', async () => {
|
||||
|
||||
Reference in New Issue
Block a user