fix: return 400 on invalid POST data to project access endpoint (#5610)

This PR fixes the issue discussed in SR-234, where you would get a 200
OK response even if your POST request to
`/api/admin/projects/<project-name>/access` contains invalid data (and
nothing is persisted).

src/lib/db/access-store.test.ts

@@ -2,6 +2,7 @@ import dbInit from '../../test/e2e/helpers/database-init';
 import getLogger from '../../test/fixtures/no-logger';
 import { PermissionRef } from 'lib/services/access-service';
 import { AccessStore } from './access-store';
+import { BadDataError } from '../../lib/error';
 
 let db;
 
@@ -104,3 +105,29 @@ test.each([
         expect(result).toStrictEqual(expected);
     },
 );
+
+describe('addAccessToProject', () => {
+    test.each(['roles', 'groups', 'users'])(
+        'should throw a bad data error if there is invalid data in the %s property of the addAccessToProject payload',
+        async (property) => {
+            const access = db.stores.accessStore as AccessStore;
+
+            const payload = {
+                roles: [4, 5],
+                groups: [],
+                users: [],
+                [property]: [123456789],
+            };
+
+            await expect(() =>
+                access.addAccessToProject(
+                    payload.roles,
+                    payload.groups,
+                    payload.users,
+                    'projectId',
+                    'createdBy',
+                ),
+            ).rejects.toThrow(BadDataError);
+        },
+    );
+});

src/lib/db/access-store.ts

@@ -26,6 +26,7 @@ import {
     PermissionRef,
 } from 'lib/services/access-service';
 import { inTransaction } from './transaction';
+import BadDataError from '../error/bad-data-error';
 
 const T = {
     ROLE_USER: 'role_user',
@@ -618,6 +619,18 @@ export class AccessStore implements IAccessStore {
             .whereIn('type', PROJECT_ROLE_TYPES)
             .pluck('id');
 
+        if (validatedProjectRoleIds.length !== roles.length) {
+            const invalidRoles = roles.filter(
+                (role) => !validatedProjectRoleIds.includes(role),
+            );
+
+            throw new BadDataError(
+                `You can't add access to a project with roles that aren't project roles or that don't exist. These roles are not valid: ${invalidRoles.join(
+                    ', ',
+                )}`,
+            );
+        }
+
         const groupRows = groups.flatMap((group) =>
             validatedProjectRoleIds.map((role) => ({
                 group_id: group,
@@ -636,17 +649,54 @@ export class AccessStore implements IAccessStore {
         );
 
         await inTransaction(this.db, async (tx) => {
+            const errors: string[] = [];
             if (groupRows.length > 0) {
                 await tx(T.GROUP_ROLE)
                     .insert(groupRows)
                     .onConflict(['project', 'role_id', 'group_id'])
-                    .merge();
+                    .merge()
+                    .catch((err) => {
+                        if (
+                            err.message.includes(
+                                `violates foreign key constraint "group_role_group_id_fkey"`,
+                            )
+                        ) {
+                            errors.push(
+                                `Your request contains one or more group IDs that do not exist. You sent these group IDs: ${groups.join(
+                                    ', ',
+                                )}.`,
+                            );
+                        }
+                    });
             }
             if (userRows.length > 0) {
                 await tx(T.ROLE_USER)
                     .insert(userRows)
                     .onConflict(['project', 'role_id', 'user_id'])
-                    .merge();
+                    .merge()
+                    .catch((err) => {
+                        if (
+                            err.message.includes(
+                                `violates foreign key constraint "role_user_user_id_fkey"`,
+                            )
+                        ) {
+                            errors.push(
+                                `Your request contains one or more user IDs that do not exist. You sent these user IDs: ${users.join(
+                                    ', ',
+                                )}.`,
+                            );
+                        }
+                    });
+            }
+
+            if (errors.length) {
+                const mapped = errors.map((message) => ({
+                    message,
+                }));
+
+                // because TS doesn't understand that the non-empty
+                // array is guaranteed to have at least one element
+                throw new BadDataError('', [mapped[0], ...mapped.slice(1)]);
             }
         });
     }

src/lib/services/access-service.test.ts

@@ -19,6 +19,7 @@ import { IRole } from '../../lib/types/stores/access-store';
 import { IGroup, ROLE_CREATED } from '../../lib/types';
 import EventService from './event-service';
 import FakeFeatureTagStore from '../../test/fixtures/fake-feature-tag-store';
+import BadDataError from '../../lib/error/bad-data-error';
 
 function getSetup(customRootRolesKillSwitch: boolean = true) {
     const config = createTestConfig({
@@ -265,3 +266,18 @@ test('throws error when trying to delete a project role in use by group', async
         );
     }
 });
+
+describe('addAccessToProject', () => {
+    test('should throw an error when you try add access with an empty list of roles', async () => {
+        const { accessService } = getSetup();
+        await expect(() =>
+            accessService.addAccessToProject(
+                [],
+                [1],
+                [1],
+                'projectId',
+                'createdBy',
+            ),
+        ).rejects.toThrow(BadDataError);
+    });
+});

src/lib/services/access-service.ts

@@ -277,6 +277,11 @@ export class AccessService {
         projectId: string,
         createdBy: string,
     ): Promise<void> {
+        if (roles.length === 0) {
+            throw new BadDataError(
+                "You can't grant access without any roles. The roles array you sent was empty.",
+            );
+        }
         return this.store.addAccessToProject(
             roles,
             groups,

src/test/e2e/services/access-service.e2e.test.ts

@@ -21,6 +21,7 @@ import {
     createFeatureToggleService,
     createProjectService,
 } from '../../../lib/features';
+import { BadDataError } from '../../../lib/error';
 
 let db: ITestDb;
 let stores: IUnleashStores;
@@ -1382,13 +1383,15 @@ test('calling add access with invalid project role ids should not assign those r
 
     const adminRootRole = await accessService.getRoleByName(RoleName.ADMIN);
 
-    accessService.addAccessToProject(
-        [adminRootRole.id, 9999],
-        [],
-        [emptyUser.id],
-        projectName,
-        'some-admin-user',
-    );
+    await expect(() =>
+        accessService.addAccessToProject(
+            [adminRootRole.id, 9999],
+            [],
+            [emptyUser.id],
+            projectName,
+            'some-admin-user',
+        ),
+    ).rejects.toThrow(BadDataError);
 
     const newAssignedPermissions =
         await accessService.getPermissionsForUser(emptyUser);