feat: move create user validations to the input (#9189)

## About the changes Validations in the constructor were executed on the way out (i.e. when reading users). Instead we should validate when we insert the users. We're also relaxing the email validation to support top domain emails (e.g. `...@jp`)
2025-03-09 00:18:26 +01:00 · 2025-02-03 15:18:20 +01:00 · 2025-02-03 15:18:20 +01:00 · 6f19ce090d
commit 6f19ce090d
parent b9aa554b0d
3 changed files with 63 additions and 79 deletions
--- a/src/lib/services/user-service.test.ts
+++ b/src/lib/services/user-service.test.ts
@ -1,7 +1,6 @@
 import { URL } from 'url';
 import UserService from './user-service';
 import UserStoreMock from '../../test/fixtures/fake-user-store';
-import EventStoreMock from '../../test/fixtures/fake-event-store';
 import AccessServiceMock from '../../test/fixtures/access-service-mock';
 import ResetTokenService from './reset-token-service';
 import { EmailService } from './email-service';
@ -21,50 +20,53 @@ const config: IUnleashConfig = createTestConfig();

 const systemUser = new User({ id: -1, username: 'system' });

-test('Should create new user', async () => {
-    const userStore = new UserStoreMock();
-    const eventStore = new EventStoreMock();
-    const accessService = new AccessServiceMock();
-    const resetTokenStore = new FakeResetTokenStore();
-    const resetTokenService = new ResetTokenService(
-        { resetTokenStore },
-        config,
-    );
-    const sessionStore = new FakeSessionStore();
-    const sessionService = new SessionService({ sessionStore }, config);
-    const emailService = new EmailService(config);
-    const eventService = createFakeEventsService(config);
-    const settingService = new SettingService(
-        {
-            settingStore: new FakeSettingStore(),
-        },
-        config,
-        eventService,
-    );
+test.each([undefined, 'test-unleash@example.com', 'top-level-domain@jp'])(
+    'Should create new user with email %s',
+    async (email?: string) => {
+        const userStore = new UserStoreMock();
+        const accessService = new AccessServiceMock();
+        const resetTokenStore = new FakeResetTokenStore();
+        const resetTokenService = new ResetTokenService(
+            { resetTokenStore },
+            config,
+        );
+        const sessionStore = new FakeSessionStore();
+        const sessionService = new SessionService({ sessionStore }, config);
+        const emailService = new EmailService(config);
+        const eventService = createFakeEventsService(config);
+        const settingService = new SettingService(
+            {
+                settingStore: new FakeSettingStore(),
+            },
+            config,
+            eventService,
+        );

-    const service = new UserService({ userStore }, config, {
-        accessService,
-        resetTokenService,
-        emailService,
-        eventService,
-        sessionService,
-        settingService,
-    });
-    const user = await service.createUser(
-        {
-            username: 'test',
-            rootRole: 1,
-        },
-        extractAuditInfoFromUser(systemUser),
-    );
-    const storedUser = await userStore.get(user.id);
-    const allUsers = await userStore.getAll();
+        const service = new UserService({ userStore }, config, {
+            accessService,
+            resetTokenService,
+            emailService,
+            eventService,
+            sessionService,
+            settingService,
+        });
+        const user = await service.createUser(
+            {
+                username: 'test',
+                rootRole: 1,
+                email,
+            },
+            extractAuditInfoFromUser(systemUser),
+        );
+        const storedUser = await userStore.get(user.id);
+        const allUsers = await userStore.getAll();

-    expect(user.id).toBeTruthy();
-    expect(user.username).toBe('test');
-    expect(allUsers.length).toBe(1);
-    expect(storedUser.username).toBe('test');
-});
+        expect(user.id).toBeTruthy();
+        expect(user.username).toBe('test');
+        expect(allUsers.length).toBe(1);
+        expect(storedUser.username).toBe('test');
+    },
+);

 describe('Default admin initialization', () => {
    const DEFAULT_ADMIN_USERNAME = 'admin';
@ -79,7 +81,6 @@ describe('Default admin initialization', () => {
        jest.clearAllMocks();

        const userStore = new UserStoreMock();
-        const eventStore = new EventStoreMock();
        const accessService = new AccessServiceMock();
        const resetTokenStore = new FakeResetTokenStore();
        const resetTokenService = new ResetTokenService(
@ -149,7 +150,6 @@ describe('Default admin initialization', () => {

    test('Should not create any default admin user if `createAdminUser` is not true and `initialAdminUser` is not set', async () => {
        const userStore = new UserStoreMock();
-        const eventStore = new EventStoreMock();
        const accessService = new AccessServiceMock();
        const resetTokenStore = new FakeResetTokenStore();
        const resetTokenService = new ResetTokenService(
@ -204,7 +204,6 @@ describe('Default admin initialization', () => {

 test('Should be a valid password', async () => {
    const userStore = new UserStoreMock();
-    const eventStore = new EventStoreMock();
    const accessService = new AccessServiceMock();
    const resetTokenStore = new FakeResetTokenStore();
    const resetTokenService = new ResetTokenService(
@ -240,7 +239,6 @@ test('Should be a valid password', async () => {

 test('Password must be at least 10 chars', async () => {
    const userStore = new UserStoreMock();
-    const eventStore = new EventStoreMock();
    const accessService = new AccessServiceMock();
    const resetTokenStore = new FakeResetTokenStore();
    const resetTokenService = new ResetTokenService(
@ -277,7 +275,6 @@ test('Password must be at least 10 chars', async () => {

 test('The password must contain at least one uppercase letter.', async () => {
    const userStore = new UserStoreMock();
-    const eventStore = new EventStoreMock();
    const accessService = new AccessServiceMock();
    const resetTokenStore = new FakeResetTokenStore();
    const resetTokenService = new ResetTokenService(
@ -315,7 +312,6 @@ test('The password must contain at least one uppercase letter.', async () => {

 test('The password must contain at least one number', async () => {
    const userStore = new UserStoreMock();
-    const eventStore = new EventStoreMock();
    const accessService = new AccessServiceMock();
    const resetTokenStore = new FakeResetTokenStore();
    const resetTokenService = new ResetTokenService(
@ -354,7 +350,6 @@ test('The password must contain at least one number', async () => {

 test('The password must contain at least one special character', async () => {
    const userStore = new UserStoreMock();
-    const eventStore = new EventStoreMock();
    const accessService = new AccessServiceMock();
    const resetTokenStore = new FakeResetTokenStore();
    const resetTokenService = new ResetTokenService(
@ -392,7 +387,6 @@ test('The password must contain at least one special character', async () => {

 test('Should be a valid password with special chars', async () => {
    const userStore = new UserStoreMock();
-    const eventStore = new EventStoreMock();
    const accessService = new AccessServiceMock();
    const resetTokenStore = new FakeResetTokenStore();
    const resetTokenService = new ResetTokenService(
@ -427,7 +421,6 @@ test('Should be a valid password with special chars', async () => {

 test('Should send password reset email if user exists', async () => {
    const userStore = new UserStoreMock();
-    const eventStore = new EventStoreMock();
    const accessService = new AccessServiceMock();
    const resetTokenStore = new FakeResetTokenStore();
    const resetTokenService = new ResetTokenService(
@ -478,7 +471,6 @@ test('Should send password reset email if user exists', async () => {

 test('Should throttle password reset email', async () => {
    const userStore = new UserStoreMock();
-    const eventStore = new EventStoreMock();
    const accessService = new AccessServiceMock();
    const resetTokenStore = new FakeResetTokenStore();
    const resetTokenService = new ResetTokenService(
--- a/src/lib/services/user-service.ts
+++ b/src/lib/services/user-service.ts
@ -240,6 +240,19 @@ class UserService {
        return this.store.getByQuery({ email });
    }

+    private validateEmail(email?: string): void {
+        if (email) {
+            Joi.assert(
+                email,
+                Joi.string().email({
+                    ignoreLength: true,
+                    minDomainSegments: 1,
+                }),
+                'Email',
+            );
+        }
+    }
+
    async createUser(
        { username, email, name, password, rootRole }: ICreateUser,
        auditUser: IAuditUser = SYSTEM_USER_AUDIT,
@ -248,13 +261,9 @@ class UserService {
            throw new BadDataError('You must specify username or email');
        }

-        if (email) {
-            Joi.assert(
-                email,
-                Joi.string().email({ ignoreLength: true }),
-                'Email',
-            );
-        }
+        Joi.assert(name, Joi.string(), 'Name');
+
+        this.validateEmail(email);

        const exists = await this.store.hasUser({ username, email });
        if (exists) {
@ -348,13 +357,7 @@ class UserService {
    ): Promise<IUserWithRootRole> {
        const preUser = await this.getUser(id);

-        if (email) {
-            Joi.assert(
-                email,
-                Joi.string().email({ ignoreLength: true }),
-                'Email',
-            );
-        }
+        this.validateEmail(email);

        if (rootRole) {
            await this.accessService.setUserRootRole(id, rootRole);
--- a/src/lib/types/user.ts
+++ b/src/lib/types/user.ts
@ -1,4 +1,4 @@
-import Joi, { ValidationError } from 'joi';
+import { ValidationError } from 'joi';
 import { generateImageUrl } from '../util/generateImageUrl';

 export const AccountTypes = ['User', 'Service Account'] as const;
@ -90,17 +90,6 @@ export default class User implements IUser {
        if (!id) {
            throw new ValidationError('Id is required', [], undefined);
        }
-        try {
-            Joi.assert(
-                email,
-                Joi.string().email({ ignoreLength: true }),
-                'Email',
-            );
-        } catch (e) {
-            console.error('Invalid email', email, e);
-        }
-        Joi.assert(username, Joi.string(), 'Username');
-        Joi.assert(name, Joi.string(), 'Name');

        this.id = id;
        this.name = name!;