fix: make user creation transactional (#10327)

## About the changes
When inserting a user with an invalid role id, the user creation will
succeed but there will be no record in the audit log.

The API call returns a 400 misleading you to believe the user was not
created, but it actually was.

This makes the whole user creation transactional, so if something fails,
data will be in the right state.

## Testing
The e2e test was split in 2 scenarios, one with smtp and another one
without.

This test was added, and it was failing before adding the transaction,
because when fetching the users, the user was there, despite having
returned a 400 error in the API call:

80a2e65b6f/src/test/e2e/api/admin/user-admin.e2e.test.ts (L181-L204)

src/lib/features/users/createUserService.ts

@@ -0,0 +1,55 @@
+import { ResetTokenStore } from '../../db/reset-token-store.js';
+import SettingStore from '../../db/setting-store.js';
+import {
+    createAccessService,
+    createEventsService,
+    type Db,
+    EmailService,
+    type IUnleashConfig,
+    ResetTokenService,
+    SessionService,
+    SessionStore,
+    SettingService,
+    UserService,
+} from '../../server-impl.js';
+import { UserStore } from './user-store.js';
+
+export const createUserService = (
+    db: Db,
+    config: IUnleashConfig,
+): UserService => {
+    const userStore = new UserStore(db, config.getLogger);
+    const resetTokenStore = new ResetTokenStore(
+        db,
+        config.eventBus,
+        config.getLogger,
+    );
+    const resetTokenService = new ResetTokenService(
+        { resetTokenStore },
+        config,
+    );
+    const eventService = createEventsService(db, config);
+    const sessionStore = new SessionStore(
+        db,
+        config.eventBus,
+        config.getLogger,
+    );
+    const sessionService = new SessionService({ sessionStore }, config);
+    const settingStore = new SettingStore(db, config.getLogger);
+    const settingService = new SettingService(
+        { settingStore },
+        config,
+        eventService,
+    );
+    const accessService = createAccessService(db, config);
+    const emailService = new EmailService(config);
+
+    return new UserService({ userStore }, config, {
+        accessService,
+        resetTokenService,
+        emailService,
+        eventService,
+        sessionService,
+        settingService,
+    });
+};

src/lib/routes/admin-api/user-admin.ts

@@ -63,11 +63,12 @@ import {
     type UserAccessOverviewSchema,
     userAccessOverviewSchema,
 } from '../../openapi/index.js';
+import type { WithTransactional } from '../../server-impl.js';
 
 export default class UserAdminController extends Controller {
     private flagResolver: IFlagResolver;
 
-    private userService: UserService;
+    private userService: WithTransactional<UserService>;
 
     private accountService: AccountService;
 
@@ -600,7 +601,9 @@ export default class UserAdminController extends Controller {
             ? Number(rootRole)
             : (rootRole as RoleName);
 
-        const createdUser = await this.userService.createUser(
+        const responseData = await this.userService.transactional(
+            async (txUserService) => {
+                const createdUser = await txUserService.createUser(
                     {
                         username,
                         email,
@@ -611,14 +614,17 @@ export default class UserAdminController extends Controller {
                     req.audit,
                 );
 
-        const inviteLink = await this.userService.newUserInviteLink(
+                const inviteLink = await txUserService.newUserInviteLink(
                     createdUser,
                     req.audit,
                 );
 
                 // send email defaults to true
                 const emailSent = (sendEmail !== undefined ? sendEmail : true)
-            ? await this.userService.sendWelcomeEmail(createdUser, inviteLink)
+                    ? await txUserService.sendWelcomeEmail(
+                          createdUser,
+                          inviteLink,
+                      )
                     : false;
 
                 const { isAPI, ...user } = createdUser;
@@ -628,6 +634,9 @@ export default class UserAdminController extends Controller {
                     emailSent,
                     rootRole: normalizedRootRole,
                 };
+                return responseData;
+            },
+        );
 
         this.openApiService.respondWithValidation(
             201,

src/lib/services/index.ts

@@ -91,8 +91,6 @@ import {
     createDependentFeaturesService,
     createFakeDependentFeaturesService,
 } from '../features/dependent-features/createDependentFeaturesService.js';
-import { DependentFeaturesReadModel } from '../features/dependent-features/dependent-features-read-model.js';
-import { FakeDependentFeaturesReadModel } from '../features/dependent-features/fake-dependent-features-read-model.js';
 import {
     createFakeLastSeenService,
     createLastSeenService,
@@ -171,6 +169,7 @@ import type {
 import type { IPrivateProjectChecker } from '../features/private-project/privateProjectCheckerType.js';
 import { UnknownFlagsService } from '../features/metrics/unknown-flags/unknown-flags-service.js';
 import type FeatureLinkService from '../features/feature-links/feature-link-service.js';
+import { createUserService } from '../features/users/createUserService.js';
 
 export const createServices = (
     stores: IUnleashStores,
@@ -215,9 +214,6 @@ export const createServices = (
         unknownFlagsService,
     );
 
-    const dependentFeaturesReadModel = db
-        ? new DependentFeaturesReadModel(db)
-        : new FakeDependentFeaturesReadModel();
     const featureLifecycleReadModel = db
         ? new FeatureLifecycleReadModel(db)
         : new FakeFeatureLifecycleReadModel();
@@ -252,14 +248,18 @@ export const createServices = (
     );
     const sessionService = new SessionService(stores, config);
     const settingService = new SettingService(stores, config, eventService);
-    const userService = new UserService(stores, config, {
+    const userService = db
+        ? withTransactional((db) => createUserService(db, config), db)
+        : withFakeTransactional(
+              new UserService(stores, config, {
                   accessService,
                   resetTokenService,
                   emailService,
                   eventService,
                   sessionService,
                   settingService,
-    });
+              }),
+          );
     const accountService = new AccountService(stores, config, {
         accessService,
     });
@@ -607,7 +607,7 @@ export interface IUnleashServices {
     tagTypeService: TagTypeService;
     transactionalTagTypeService: WithTransactional<TagTypeService>;
     userFeedbackService: UserFeedbackService;
-    userService: UserService;
+    userService: WithTransactional<UserService>;
     versionService: VersionService;
     userSplashService: UserSplashService;
     segmentService: ISegmentService;

src/lib/services/user-service.ts

@@ -78,7 +78,7 @@ export interface ILoginUserRequest {
 const saltRounds = 10;
 const disallowNPreviousPasswords = 5;
 
-class UserService {
+export class UserService {
     private logger: Logger;
 
     private store: IUserStore;
@@ -404,7 +404,7 @@ class UserService {
 
     async deleteScimUsers(auditUser: IAuditUser): Promise<void> {
         const users = await this.store.deleteScimUsers();
-        // Note: after deletion we can't get the role for the user
+        // Note: after deletion we can't get the role for the user. This is a simplification
         const viewerRole = await this.accessService.getPredefinedRole(
             RoleName.VIEWER,
         );

src/test/e2e/api/admin/user-admin.e2e.test.ts

@@ -19,7 +19,6 @@ import { omitKeys } from '../../../../lib/util/omit-keys.js';
 import type { ISessionStore } from '../../../../lib/types/stores/session-store.js';
 import type { IUnleashStores } from '../../../../lib/types/index.js';
 import { createHash } from 'crypto';
-import { vi } from 'vitest';
 
 let stores: IUnleashStores;
 let db: ITestDb;
@@ -32,18 +31,84 @@ let sessionStore: ISessionStore;
 let editorRole: IRole;
 let adminRole: IRole;
 
-beforeAll(async () => {
+describe('User Admin API with email configuration', () => {
+    beforeAll(async () => {
         db = await dbInit('user_admin_api_serial', getLogger);
-    stores = db.stores;
+        app = await setupAppWithCustomConfig(
-    app = await setupAppWithCustomConfig(stores, {
+            db.stores,
+            {
+                email: {
+                    host: 'smtp.ethereal.email',
+                    smtpuser: 'rafaela.pouros@ethereal.email',
+                    smtppass: 'CuVPBSvUFBPuqXMFEe',
+                },
                 experimental: {
                     flags: {
                         strictSchemaValidation: true,
                         showUserDeviceCount: true,
                     },
                 },
+            },
+            db.rawDatabase,
+        );
+        const roles = await db.stores.roleStore.getRootRoles();
+        editorRole = roles.find((r) => r.name === RoleName.EDITOR)!!;
     });
 
+    afterAll(async () => {
+        await app.destroy();
+        await db.destroy();
+    });
+    test('Creates a user but does not send email if sendEmail is set to false', async () => {
+        await app.request
+            .post('/api/admin/user-admin')
+            .send({
+                email: 'some@getunelash.ai',
+                name: 'Some Name',
+                rootRole: editorRole.id,
+                sendEmail: false,
+            })
+            .set('Content-Type', 'application/json')
+            .expect(201)
+            .expect((res) => {
+                expect(res.body.emailSent).toBeFalsy();
+            });
+
+        await app.request
+            .post('/api/admin/user-admin')
+            .send({
+                email: 'some2@getunelash.ai',
+                name: 'Some2 Name',
+                rootRole: editorRole.id,
+            })
+            .set('Content-Type', 'application/json')
+            .expect(201)
+            .expect((res) => {
+                expect(res.body.emailSent).toBeTruthy();
+            });
+    });
+});
+
+describe('User Admin API without email', () => {
+    beforeAll(async () => {
+        db = await dbInit('user_admin_api_serial', getLogger);
+        stores = db.stores;
+        app = await setupAppWithCustomConfig(
+            stores,
+            {
+                experimental: {
+                    flags: {
+                        strictSchemaValidation: true,
+                        showUserDeviceCount: true,
+                    },
+                },
+                rateLimiting: {
+                    createUserMaxPerMinute: 100,
+                },
+            },
+            db.rawDatabase,
+        );
+
         userStore = stores.userStore;
         eventStore = stores.eventStore;
         roleStore = stores.roleStore;
@@ -51,18 +116,18 @@ beforeAll(async () => {
         const roles = await roleStore.getRootRoles();
         editorRole = roles.find((r) => r.name === RoleName.EDITOR)!!;
         adminRole = roles.find((r) => r.name === RoleName.ADMIN)!!;
-});
+    });
 
-afterAll(async () => {
+    afterAll(async () => {
         await app.destroy();
         await db.destroy();
-});
+    });
 
-afterEach(async () => {
+    afterEach(async () => {
         await userStore.deleteAll();
-});
+    });
 
-test('returns empty list of users', async () => {
+    test('returns empty list of users', async () => {
         return app.request
             .get('/api/admin/user-admin')
             .expect('Content-Type', /json/)
@@ -70,9 +135,9 @@ test('returns empty list of users', async () => {
             .expect((res) => {
                 expect(res.body.users.length).toBe(0);
             });
-});
+    });
 
-test('creates and returns all users', async () => {
+    test('creates and returns all users', async () => {
         const createUserRequests = [...Array(10).keys()].map((i) =>
             app.request
                 .post('/api/admin/user-admin')
@@ -94,9 +159,9 @@ test('creates and returns all users', async () => {
                 expect(res.body.users.length).toBe(10);
                 expect(res.body.users[2].rootRole).toBe(editorRole.id);
             });
-});
+    });
 
-test('creates editor-user without password', async () => {
+    test('creates editor-user without password', async () => {
         return app.request
             .post('/api/admin/user-admin')
             .send({
@@ -111,9 +176,34 @@ test('creates editor-user without password', async () => {
                 expect(res.body.rootRole).toBe(editorRole.id);
                 expect(res.body.id).toBeTruthy();
             });
-});
+    });
 
-test('creates admin-user with password', async () => {
+    test('When invalid role is provided the user is not created', async () => {
+        const invalidRoleId = 0;
+        await app.request
+            .post('/api/admin/user-admin')
+            .send({
+                email: 'should-not-exist@getunleash.ai',
+                name: 'I am the invisible man',
+                rootRole: invalidRoleId,
+            })
+            .set('Content-Type', 'application/json')
+            .expect(400);
+
+        return app.request
+            .get('/api/admin/user-admin')
+            .expect('Content-Type', /json/)
+            .expect(200)
+            .expect((res) => {
+                expect(res.body.users).not.toContainEqual(
+                    expect.objectContaining({
+                        email: 'should-not-exist@getunleash.ai',
+                    }),
+                );
+            });
+    });
+
+    test('creates admin-user with password', async () => {
         const { body } = await app.request
             .post('/api/admin/user-admin')
             .send({
@@ -137,9 +227,9 @@ test('creates admin-user with password', async () => {
         const roles = await stores.accessStore.getRolesForUserId(body.id);
         expect(roles.length).toBe(1);
         expect(roles[0].name).toBe(RoleName.ADMIN);
-});
+    });
 
-test('requires known root role', async () => {
+    test('requires known root role', async () => {
         return app.request
             .post('/api/admin/user-admin')
             .send({
@@ -149,9 +239,9 @@ test('requires known root role', async () => {
             })
             .set('Content-Type', 'application/json')
             .expect(400);
-});
+    });
 
-test('should require username or email on create', async () => {
+    test('should require username or email on create', async () => {
         await app.request
             .post('/api/admin/user-admin')
             .send({ rootRole: adminRole.id })
@@ -162,9 +252,9 @@ test('should require username or email on create', async () => {
                     'You must specify username or email',
                 );
             });
-});
+    });
 
-test('update user name', async () => {
+    test('update user name', async () => {
         const { body } = await app.request
             .post('/api/admin/user-admin')
             .send({
@@ -186,12 +276,15 @@ test('update user name', async () => {
                 expect(res.body.name).toBe('New name');
                 expect(res.body.id).toBe(body.id);
             });
-});
+    });
 
-test('should not require any fields on update', async () => {
+    test('should not require any fields on update', async () => {
         const { body: created } = await app.request
             .post('/api/admin/user-admin')
-        .send({ email: `${randomId()}@example.com`, rootRole: editorRole.id })
+            .send({
+                email: `${randomId()}@example.com`,
+                rootRole: editorRole.id,
+            })
             .set('Content-Type', 'application/json')
             .expect(201);
 
@@ -204,9 +297,9 @@ test('should not require any fields on update', async () => {
         expect(updated).toEqual(
             omitKeys(created, 'emailSent', 'inviteLink', 'rootRole'),
         );
-});
+    });
 
-test('get a single user', async () => {
+    test('get a single user', async () => {
         const { body } = await app.request
             .post('/api/admin/user-admin')
             .send({
@@ -223,40 +316,47 @@ test('get a single user', async () => {
         expect(user.email).toBe('some2@getunelash.ai');
         expect(user.name).toBe('Some Name 2');
         expect(user.id).toBe(body.id);
-});
+    });
 
-test('should delete user', async () => {
+    test('should delete user', async () => {
         const user = await userStore.insert({ email: 'some@mail.com' });
 
-    await app.request.delete(`/api/admin/user-admin/${user.id}`).expect(200);
+        await app.request
+            .delete(`/api/admin/user-admin/${user.id}`)
+            .expect(200);
         await app.request.get(`/api/admin/user-admin/${user.id}`).expect(404);
-});
+    });
 
-test('validator should require strong password', async () => {
+    test('validator should require strong password', async () => {
         return app.request
             .post('/api/admin/user-admin/validate-password')
             .send({ password: 'simple' })
             .expect(400);
-});
+    });
 
-test('validator should accept strong password', async () => {
+    test('validator should accept strong password', async () => {
         return app.request
             .post('/api/admin/user-admin/validate-password')
             .send({ password: 'simple123-_ASsad' })
             .expect(200);
-});
+    });
 
-test('should change password', async () => {
+    test('should change password', async () => {
         const user = await userStore.insert({ email: 'some@mail.com' });
-    const spy = vi.spyOn(sessionStore, 'deleteSessionsForUser');
+        await sessionStore.insertSession({
+            sid: '1',
+            sess: { user: { id: user.id } },
+        });
+        expect(await sessionStore.getSessionsForUser(user.id)).toHaveLength(1);
+
         await app.request
             .post(`/api/admin/user-admin/${user.id}/change-password`)
             .send({ password: 'simple123-_ASsad' })
             .expect(200);
-    expect(spy).toHaveBeenCalled();
+        expect(await sessionStore.getSessionsForUser(user.id)).toHaveLength(0);
-});
+    });
 
-test('should search for users', async () => {
+    test('should search for users', async () => {
         await userStore.insert({ email: 'some@mail.com' });
         await userStore.insert({ email: 'another@mail.com' });
         await userStore.insert({ email: 'another2@mail.com' });
@@ -266,13 +366,13 @@ test('should search for users', async () => {
             .expect(200)
             .expect((res) => {
                 expect(res.body.length).toBe(2);
-            expect(res.body.some((u) => u.email === 'another@mail.com')).toBe(
+                expect(
-                true,
+                    res.body.some((u) => u.email === 'another@mail.com'),
-            );
+                ).toBe(true);
+            });
     });
-});
 
-test('Creates a user and includes inviteLink and emailConfigured', async () => {
+    test('Creates a user and includes inviteLink and emailConfigured', async () => {
         return app.request
             .post('/api/admin/user-admin')
             .send({
@@ -289,47 +389,9 @@ test('Creates a user and includes inviteLink and emailConfigured', async () => {
                 expect(res.body.emailSent).toBeFalsy();
                 expect(res.body.id).toBeTruthy();
             });
-});
-
-test('Creates a user but does not send email if sendEmail is set to false', async () => {
-    const myAppConfig = await setupAppWithCustomConfig(stores, {
-        email: {
-            host: 'smtp.ethereal.email',
-            smtpuser: 'rafaela.pouros@ethereal.email',
-            smtppass: 'CuVPBSvUFBPuqXMFEe',
-        },
     });
 
-    await myAppConfig.request
+    test('generates USER_CREATED event', async () => {
-        .post('/api/admin/user-admin')
-        .send({
-            email: 'some@getunelash.ai',
-            name: 'Some Name',
-            rootRole: editorRole.id,
-            sendEmail: false,
-        })
-        .set('Content-Type', 'application/json')
-        .expect(201)
-        .expect((res) => {
-            expect(res.body.emailSent).toBeFalsy();
-        });
-    await myAppConfig.request
-        .post('/api/admin/user-admin')
-        .send({
-            email: 'some2@getunelash.ai',
-            name: 'Some2 Name',
-            rootRole: editorRole.id,
-        })
-        .set('Content-Type', 'application/json')
-        .expect(201)
-        .expect((res) => {
-            expect(res.body.emailSent).toBeTruthy();
-        });
-
-    await myAppConfig.destroy();
-});
-
-test('generates USER_CREATED event', async () => {
         const email = 'some@getunelash.ai';
         const name = 'Some Name';
 
@@ -351,19 +413,21 @@ test('generates USER_CREATED event', async () => {
         expect(events[0].data.name).toBe(name);
         expect(events[0].data.id).toBe(body.id);
         expect(events[0].data.password).toBeFalsy();
-});
+    });
 
-test('generates USER_DELETED event', async () => {
+    test('generates USER_DELETED event', async () => {
         const user = await userStore.insert({ email: 'some@mail.com' });
-    await app.request.delete(`/api/admin/user-admin/${user.id}`).expect(200);
+        await app.request
+            .delete(`/api/admin/user-admin/${user.id}`)
+            .expect(200);
 
         const events = await eventStore.getEvents();
         expect(events[0].type).toBe(USER_DELETED);
         expect(events[0].preData.id).toBe(user.id);
         expect(events[0].preData.email).toBe(user.email);
-});
+    });
 
-test('generates USER_UPDATED event', async () => {
+    test('generates USER_UPDATED event', async () => {
         const { body } = await app.request
             .post('/api/admin/user-admin')
             .send({
@@ -384,9 +448,9 @@ test('generates USER_UPDATED event', async () => {
         expect(events[0].type).toBe(USER_UPDATED);
         expect(events[0].data.id).toBe(body.id);
         expect(events[0].data.name).toBe('New name');
-});
+    });
 
-test('Anonymises name, username and email fields if anonymiseEventLog flag is set', async () => {
+    test('Anonymises name, username and email fields if anonymiseEventLog flag is set', async () => {
         const anonymisedApp = await setupAppWithCustomConfig(
             stores,
             { experimental: { flags: { anonymiseEventLog: true } } },
@@ -407,9 +471,9 @@ test('Anonymises name, username and email fields if anonymiseEventLog flag is se
         expect(body.users[0].email).toEqual('aeb83743e@unleash.run');
         expect(body.users[0].name).toEqual('3a8b17647@unleash.run');
         expect(body.users[0].username).toEqual(''); // Not set, so anonymise should return the empty string.
-});
+    });
 
-test('creates user with email sha256 hash', async () => {
+    test('creates user with email sha256 hash', async () => {
         await app.request
             .post('/api/admin/user-admin')
             .send({
@@ -417,7 +481,8 @@ test('creates user with email sha256 hash', async () => {
                 name: `Some Name Hash`,
                 rootRole: editorRole.id,
             })
-        .set('Content-Type', 'application/json');
+            .set('Content-Type', 'application/json')
+            .expect(201);
 
         const user = await db
             .rawDatabase('users')
@@ -429,9 +494,9 @@ test('creates user with email sha256 hash', async () => {
             .digest('hex');
 
         expect(user.email_hash).toBe(expectedHash);
-});
+    });
 
-test('should return number of sessions per user', async () => {
+    test('should return number of sessions per user', async () => {
         const user = await userStore.insert({ email: 'tester@example.com' });
         await sessionStore.insertSession({
             sid: '1',
@@ -448,7 +513,9 @@ test('should return number of sessions per user', async () => {
             sess: { user: { id: user2.id } },
         });
 
-    const response = await app.request.get(`/api/admin/user-admin`).expect(200);
+        const response = await app.request
+            .get(`/api/admin/user-admin`)
+            .expect(200);
 
         expect(response.body).toMatchObject({
             users: expect.arrayContaining([
@@ -462,9 +529,9 @@ test('should return number of sessions per user', async () => {
                 }),
             ]),
         });
-});
+    });
 
-test('should only delete scim users', async () => {
+    test('should only delete scim users', async () => {
         userStore.insert({
             email: 'boring@example.com',
         });
@@ -483,12 +550,17 @@ test('should only delete scim users', async () => {
                 .returning('id')
         )[0].id;
 
-    await app.request.delete('/api/admin/user-admin/scim-users').expect(200);
+        await app.request
-    const response = await app.request.get(`/api/admin/user-admin`).expect(200);
+            .delete('/api/admin/user-admin/scim-users')
+            .expect(200);
+        const response = await app.request
+            .get(`/api/admin/user-admin`)
+            .expect(200);
         const users = response.body.users;
 
         expect(users.length).toBe(2);
         expect(users.every((u) => u.email !== 'made-by-scim@example.com')).toBe(
             true,
         );
+    });
 });