From 5de4958b0f90b1d9105d2e3da41f4b168c3e4771 Mon Sep 17 00:00:00 2001 From: Mateusz Kwasniewski Date: Mon, 24 Jul 2023 11:05:55 +0200 Subject: [PATCH] fix: group cleanup (#4334) --- src/lib/db/group-store.ts | 91 ++++++++++++------- src/lib/features/group/createGroupService.ts | 21 +++++ src/lib/openapi/index.ts | 2 + src/lib/openapi/spec/create-group-schema.ts | 43 +++++++++ src/lib/openapi/spec/group-schema.ts | 2 +- src/lib/openapi/spec/index.ts | 1 + src/lib/services/group-service.ts | 13 ++- src/lib/services/index.ts | 4 + src/lib/types/group.ts | 9 ++ src/lib/types/services.ts | 1 + src/lib/types/stores/group-store.ts | 10 +- .../e2e/services/group-service.e2e.test.ts | 23 +++++ src/test/fixtures/fake-group-store.ts | 10 +- 13 files changed, 182 insertions(+), 48 deletions(-) create mode 100644 src/lib/features/group/createGroupService.ts create mode 100644 src/lib/openapi/spec/create-group-schema.ts diff --git a/src/lib/db/group-store.ts b/src/lib/db/group-store.ts index 63148aea5e..cd32b7b95e 100644 --- a/src/lib/db/group-store.ts +++ b/src/lib/db/group-store.ts @@ -1,16 +1,15 @@ import { IGroupStore, IStoreGroup } from '../types/stores/group-store'; -import { Knex } from 'knex'; import NotFoundError from '../error/notfound-error'; import Group, { + ICreateGroupModel, + ICreateGroupUserModel, IGroup, - IGroupModel, IGroupProject, IGroupRole, IGroupUser, - IGroupUserModel, } from '../types/group'; -import Transaction = Knex.Transaction; import { Db } from './db'; +import { BadDataError, FOREIGN_KEY_VIOLATION } from '../error'; const T = { GROUPS: 'groups', @@ -81,13 +80,23 @@ export default class GroupStore implements IGroupStore { return groups.map(rowToGroup); } - async update(group: IGroupModel): Promise { - const rows = await this.db(T.GROUPS) - .where({ id: group.id }) - .update(groupToRow(group)) - .returning(GROUP_COLUMNS); + async update(group: ICreateGroupModel): Promise { + try { + const rows = await this.db(T.GROUPS) + .where({ id: group.id }) + .update(groupToRow(group)) + .returning(GROUP_COLUMNS); - return rowToGroup(rows[0]); + return rowToGroup(rows[0]); + } catch (error) { + if ( + error.code === FOREIGN_KEY_VIOLATION && + error.constraint === 'fk_group_role_id' + ) { + throw new BadDataError(`Incorrect role id ${group.rootRole}`); + } + throw error; + } } async getProjectGroupRoles(projectId: string): Promise { @@ -176,10 +185,20 @@ export default class GroupStore implements IGroupStore { } async create(group: IStoreGroup): Promise { - const row = await this.db(T.GROUPS) - .insert(groupToRow(group)) - .returning('*'); - return rowToGroup(row[0]); + try { + const row = await this.db(T.GROUPS) + .insert(groupToRow(group)) + .returning('*'); + return rowToGroup(row[0]); + } catch (error) { + if ( + error.code === FOREIGN_KEY_VIOLATION && + error.constraint === 'fk_group_role_id' + ) { + throw new BadDataError(`Incorrect role id ${group.rootRole}`); + } + throw error; + } } async count(): Promise { @@ -190,25 +209,31 @@ export default class GroupStore implements IGroupStore { async addUsersToGroup( groupId: number, - users: IGroupUserModel[], + users: ICreateGroupUserModel[], userName: string, - transaction?: Transaction, ): Promise { - const rows = (users || []).map((user) => { - return { - group_id: groupId, - user_id: user.user.id, - created_by: userName, - }; - }); - return (transaction || this.db).batchInsert(T.GROUP_USER, rows); + try { + const rows = (users || []).map((user) => { + return { + group_id: groupId, + user_id: user.user.id, + created_by: userName, + }; + }); + return await this.db.batchInsert(T.GROUP_USER, rows); + } catch (error) { + if ( + error.code === FOREIGN_KEY_VIOLATION && + error.constraint === 'group_user_user_id_fkey' + ) { + throw new BadDataError('Incorrect user id in the users group'); + } + throw error; + } } - async deleteUsersFromGroup( - deletableUsers: IGroupUser[], - transaction?: Transaction, - ): Promise { - return (transaction || this.db)(T.GROUP_USER) + async deleteUsersFromGroup(deletableUsers: IGroupUser[]): Promise { + return this.db(T.GROUP_USER) .whereIn( ['group_id', 'user_id'], deletableUsers.map((user) => [user.groupId, user.userId]), @@ -218,14 +243,12 @@ export default class GroupStore implements IGroupStore { async updateGroupUsers( groupId: number, - newUsers: IGroupUserModel[], + newUsers: ICreateGroupUserModel[], deletableUsers: IGroupUser[], userName: string, ): Promise { - await this.db.transaction(async (tx) => { - await this.addUsersToGroup(groupId, newUsers, userName, tx); - await this.deleteUsersFromGroup(deletableUsers, tx); - }); + await this.addUsersToGroup(groupId, newUsers, userName); + await this.deleteUsersFromGroup(deletableUsers); } async getNewGroupsForExternalUser( diff --git a/src/lib/features/group/createGroupService.ts b/src/lib/features/group/createGroupService.ts new file mode 100644 index 0000000000..8e5a8128ce --- /dev/null +++ b/src/lib/features/group/createGroupService.ts @@ -0,0 +1,21 @@ +import { IUnleashConfig } from '../../types'; +import { GroupService } from '../../services'; +import { Db } from '../../db/db'; +import GroupStore from '../../db/group-store'; +import { AccountStore } from '../../db/account-store'; +import EventStore from '../../db/event-store'; + +export const createGroupService = ( + db: Db, + config: IUnleashConfig, +): GroupService => { + const { getLogger } = config; + const groupStore = new GroupStore(db); + const accountStore = new AccountStore(db, getLogger); + const eventStore = new EventStore(db, getLogger); + const groupService = new GroupService( + { groupStore, eventStore, accountStore }, + { getLogger }, + ); + return groupService; +}; diff --git a/src/lib/openapi/index.ts b/src/lib/openapi/index.ts index 59b07cf92a..5e8e2af1b3 100644 --- a/src/lib/openapi/index.ts +++ b/src/lib/openapi/index.ts @@ -152,6 +152,7 @@ import { strategyVariantSchema, createStrategyVariantSchema, clientSegmentSchema, + createGroupSchema, } from './spec'; import { IServerOption } from '../types'; import { mapValues, omitKeys } from '../util'; @@ -361,6 +362,7 @@ export const schemas: UnleashSchemas = { strategyVariantSchema, createStrategyVariantSchema, clientSegmentSchema, + createGroupSchema, }; // Remove JSONSchema keys that would result in an invalid OpenAPI spec. diff --git a/src/lib/openapi/spec/create-group-schema.ts b/src/lib/openapi/spec/create-group-schema.ts new file mode 100644 index 0000000000..3b7da90fd9 --- /dev/null +++ b/src/lib/openapi/spec/create-group-schema.ts @@ -0,0 +1,43 @@ +import { FromSchema } from 'json-schema-to-ts'; +import { groupSchema } from './group-schema'; + +export const createGroupSchema = { + $id: '#/components/schemas/createGroupSchema', + type: 'object', + additionalProperties: true, + required: ['name'], + description: 'A detailed information about a user group', + properties: { + name: groupSchema.properties.name, + description: groupSchema.properties.description, + mappingsSSO: groupSchema.properties.mappingsSSO, + rootRole: groupSchema.properties.rootRole, + users: { + type: 'array', + description: 'A list of users belonging to this group', + items: { + type: 'object', + description: 'A minimal user object', + required: ['user'], + properties: { + user: { + type: 'object', + description: 'A minimal user object', + required: ['id'], + properties: { + id: { + description: 'The user id', + type: 'integer', + minimum: 0, + example: 123, + }, + }, + }, + }, + }, + }, + }, + components: {}, +} as const; + +export type CreateGroupSchema = FromSchema; diff --git a/src/lib/openapi/spec/group-schema.ts b/src/lib/openapi/spec/group-schema.ts index 4fb5933a14..3e7e1e70cf 100644 --- a/src/lib/openapi/spec/group-schema.ts +++ b/src/lib/openapi/spec/group-schema.ts @@ -5,7 +5,7 @@ import { userSchema } from './user-schema'; export const groupSchema = { $id: '#/components/schemas/groupSchema', type: 'object', - additionalProperties: true, + additionalProperties: false, required: ['name'], description: 'A detailed information about a user group', properties: { diff --git a/src/lib/openapi/spec/index.ts b/src/lib/openapi/spec/index.ts index f7fce2c17a..93a67bcedc 100644 --- a/src/lib/openapi/spec/index.ts +++ b/src/lib/openapi/spec/index.ts @@ -151,3 +151,4 @@ export * from './create-strategy-variant-schema'; export * from './strategy-variant-schema'; export * from './client-segment-schema'; export * from './update-feature-type-lifetime-schema'; +export * from './create-group-schema'; diff --git a/src/lib/services/group-service.ts b/src/lib/services/group-service.ts index 53f5af51d2..b3e10f37a8 100644 --- a/src/lib/services/group-service.ts +++ b/src/lib/services/group-service.ts @@ -1,4 +1,5 @@ import { + ICreateGroupModel, IGroup, IGroupModel, IGroupModelWithProjectRole, @@ -81,7 +82,10 @@ export class GroupService { return this.mapGroupWithUsers(group, groupUsers, users); } - async createGroup(group: IGroupModel, userName: string): Promise { + async createGroup( + group: ICreateGroupModel, + userName: string, + ): Promise { await this.validateGroup(group); const newGroup = await this.groupStore.create(group); @@ -101,7 +105,10 @@ export class GroupService { return newGroup; } - async updateGroup(group: IGroupModel, userName: string): Promise { + async updateGroup( + group: ICreateGroupModel, + userName: string, + ): Promise { const preData = await this.groupStore.get(group.id); await this.validateGroup(group, preData); @@ -173,7 +180,7 @@ export class GroupService { } async validateGroup( - group: IGroupModel, + group: ICreateGroupModel, existingGroup?: IGroup, ): Promise { if (!group.name) { diff --git a/src/lib/services/index.ts b/src/lib/services/index.ts index 6800a6c7f3..20af6cb6d5 100644 --- a/src/lib/services/index.ts +++ b/src/lib/services/index.ts @@ -59,6 +59,7 @@ import { import ConfigurationRevisionService from '../features/feature-toggle/configuration-revision-service'; import { createFeatureToggleService } from '../features'; import EventAnnouncerService from './event-announcer-service'; +import { createGroupService } from '../features/group/createGroupService'; // TODO: will be moved to scheduler feature directory export const scheduleServices = async ( @@ -211,6 +212,8 @@ export const createServices = ( createExportImportTogglesService(txDb, config); const transactionalFeatureToggleService = (txDb: Knex.Transaction) => createFeatureToggleService(txDb, config); + const transactionalGroupService = (txDb: Knex.Transaction) => + createGroupService(txDb, config); const userSplashService = new UserSplashService(stores, config); const openApiService = new OpenApiService(config); const clientSpecService = new ClientSpecService(config); @@ -307,6 +310,7 @@ export const createServices = ( schedulerService, configurationRevisionService, transactionalFeatureToggleService, + transactionalGroupService, }; }; diff --git a/src/lib/types/group.ts b/src/lib/types/group.ts index 886c7e16cd..e2092ee3c6 100644 --- a/src/lib/types/group.ts +++ b/src/lib/types/group.ts @@ -33,6 +33,11 @@ export interface IGroupModel extends IGroup { projects?: string[]; } +export interface ICreateGroupModel extends IGroup { + users?: ICreateGroupUserModel[]; + projects?: string[]; +} + export interface IGroupProject { groupId: number; project: string; @@ -44,6 +49,10 @@ export interface IGroupUserModel { createdBy?: string; } +export interface ICreateGroupUserModel { + user: Pick; +} + export interface IGroupModelWithProjectRole extends IGroupModel { roleId: number; addedAt: Date; diff --git a/src/lib/types/services.ts b/src/lib/types/services.ts index 36e85b3902..94503ce1d4 100644 --- a/src/lib/types/services.ts +++ b/src/lib/types/services.ts @@ -96,4 +96,5 @@ export interface IUnleashServices { transactionalFeatureToggleService: ( db: Knex.Transaction, ) => FeatureToggleService; + transactionalGroupService: (db: Knex.Transaction) => GroupService; } diff --git a/src/lib/types/stores/group-store.ts b/src/lib/types/stores/group-store.ts index e9a5adcc6f..e30928392a 100644 --- a/src/lib/types/stores/group-store.ts +++ b/src/lib/types/stores/group-store.ts @@ -1,11 +1,11 @@ import { Store } from './store'; import Group, { + ICreateGroupModel, + ICreateGroupUserModel, IGroup, - IGroupModel, IGroupProject, IGroupRole, IGroupUser, - IGroupUserModel, } from '../group'; export interface IStoreGroup { @@ -38,20 +38,20 @@ export interface IGroupStore extends Store { updateGroupUsers( groupId: number, - newUsers: IGroupUserModel[], + newUsers: ICreateGroupUserModel[], deletableUsers: IGroupUser[], userName: string, ): Promise; deleteUsersFromGroup(deletableUsers: IGroupUser[]): Promise; - update(group: IGroupModel): Promise; + update(group: ICreateGroupModel): Promise; getAllUsersByGroups(groupIds: number[]): Promise; addUsersToGroup( groupId: number, - users: IGroupUserModel[], + users: ICreateGroupUserModel[], userName: string, ): Promise; diff --git a/src/test/e2e/services/group-service.e2e.test.ts b/src/test/e2e/services/group-service.e2e.test.ts index 9c4d1e4332..74e6a1884b 100644 --- a/src/test/e2e/services/group-service.e2e.test.ts +++ b/src/test/e2e/services/group-service.e2e.test.ts @@ -124,3 +124,26 @@ test('adding a root role to a group with a project role should fail', async () = expect.assertions(1); }); + +test('adding a nonexistent role to a group should fail', async () => { + const group = await groupStore.create({ + name: 'root_group', + description: 'root_group', + }); + + await expect(() => { + return groupService.updateGroup( + { + id: group.id, + name: group.name, + users: [], + rootRole: 100, + createdAt: new Date(), + createdBy: 'test', + }, + 'test', + ); + }).rejects.toThrow( + 'Request validation failed: your request body or params contain invalid data: Incorrect role id 100', + ); +}); diff --git a/src/test/fixtures/fake-group-store.ts b/src/test/fixtures/fake-group-store.ts index dbf4d323a3..5befbf235f 100644 --- a/src/test/fixtures/fake-group-store.ts +++ b/src/test/fixtures/fake-group-store.ts @@ -1,11 +1,11 @@ import { IGroupStore, IStoreGroup } from '../../lib/types/stores/group-store'; import Group, { + ICreateGroupModel, + ICreateGroupUserModel, IGroup, - IGroupModel, IGroupProject, IGroupRole, IGroupUser, - IGroupUserModel, } from '../../lib/types/group'; /* eslint-disable @typescript-eslint/no-unused-vars */ export default class FakeGroupStore implements IGroupStore { @@ -48,7 +48,7 @@ export default class FakeGroupStore implements IGroupStore { addUsersToGroup( id: number, - users: IGroupUserModel[], + users: ICreateGroupUserModel[], userName: string, ): Promise { throw new Error('Method not implemented.'); @@ -62,13 +62,13 @@ export default class FakeGroupStore implements IGroupStore { throw new Error('Method not implemented.'); } - update(group: IGroupModel): Promise { + update(group: ICreateGroupModel): Promise { throw new Error('Method not implemented.'); } updateGroupUsers( groupId: number, - newUsers: IGroupUserModel[], + newUsers: ICreateGroupUserModel[], deletableUsers: IGroupUser[], userName: string, ): Promise {