From c4d0896609dc5b8aed744196ecbb92f769f27983 Mon Sep 17 00:00:00 2001 From: Daniel Hougaard Date: Thu, 16 Jan 2025 01:01:01 +0100 Subject: [PATCH 1/3] feat(groups): unique names --- .../20250115222458_groups-unique-name.ts | 49 +++++++++++++++++++ .../src/ee/services/group/group-service.ts | 17 +++++++ backend/src/ee/services/scim/scim-service.ts | 12 +++++ backend/src/lib/validator/index.ts | 1 + backend/src/lib/validator/validate-uuid.ts | 3 ++ .../server/routes/v2/group-project-router.ts | 6 +-- .../group-project/group-project-service.ts | 21 ++++++-- .../group-project/group-project-types.ts | 2 +- .../secret-sharing/secret-sharing-service.ts | 4 +- 9 files changed, 103 insertions(+), 12 deletions(-) create mode 100644 backend/src/db/migrations/20250115222458_groups-unique-name.ts create mode 100644 backend/src/lib/validator/validate-uuid.ts diff --git a/backend/src/db/migrations/20250115222458_groups-unique-name.ts b/backend/src/db/migrations/20250115222458_groups-unique-name.ts new file mode 100644 index 000000000..1c40c8cba --- /dev/null +++ b/backend/src/db/migrations/20250115222458_groups-unique-name.ts @@ -0,0 +1,49 @@ +import { Knex } from "knex"; + +import { TableName } from "../schemas"; + +export async function up(knex: Knex): Promise { + // find any duplicate group names within organizations + const duplicates = await knex(TableName.Groups) + .select("orgId", "name") + .count("* as count") + .groupBy("orgId", "name") + .having(knex.raw("count(*) > 1")); + + // for each set of duplicates, update all but one with a numbered suffix + for await (const duplicate of duplicates) { + const groups = await knex(TableName.Groups) + .select("id", "name") + .where({ + orgId: duplicate.orgId, + name: duplicate.name + }) + .orderBy("createdAt", "asc"); // keep original name for oldest group + + // skip the first (oldest) group, rename others with numbered suffix + for (let i = 1; i < groups.length; i += 1) { + // eslint-disable-next-line no-await-in-loop + await knex(TableName.Groups) + .where("id", groups[i].id) + .update({ + name: `${groups[i].name} (${i})`, + + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore TS doesn't know about Knex's timestamp types + updatedAt: new Date() + }); + } + } + + // add the unique constraint + await knex.schema.alterTable(TableName.Groups, (t) => { + t.unique(["orgId", "name"]); + }); +} + +export async function down(knex: Knex): Promise { + // Remove the unique constraint + await knex.schema.alterTable(TableName.Groups, (t) => { + t.dropUnique(["orgId", "name"]); + }); +} diff --git a/backend/src/ee/services/group/group-service.ts b/backend/src/ee/services/group/group-service.ts index 68c48524b..163956e32 100644 --- a/backend/src/ee/services/group/group-service.ts +++ b/backend/src/ee/services/group/group-service.ts @@ -88,6 +88,13 @@ export const groupServiceFactory = ({ if (!hasRequiredPriviledges) throw new ForbiddenRequestError({ message: "Failed to create a more privileged group" }); + const existingGroup = await groupDAL.findOne({ orgId: actorOrgId, name }); + if (existingGroup) { + throw new BadRequestError({ + message: `Failed to create group with name '${name}'. Group with the same name already exists` + }); + } + const group = await groupDAL.create({ name, slug: slug || slugify(`${name}-${alphaNumericNanoId(4)}`), @@ -145,6 +152,16 @@ export const groupServiceFactory = ({ if (isCustomRole) customRole = customOrgRole; } + if (name) { + const existingGroup = await groupDAL.findOne({ orgId: actorOrgId, name }); + + if (existingGroup && existingGroup.id !== id) { + throw new BadRequestError({ + message: `Failed to update group with name '${name}'. Group with the same name already exists` + }); + } + } + const [updatedGroup] = await groupDAL.update( { id: group.id diff --git a/backend/src/ee/services/scim/scim-service.ts b/backend/src/ee/services/scim/scim-service.ts index 271c9aa9a..32f3c492d 100644 --- a/backend/src/ee/services/scim/scim-service.ts +++ b/backend/src/ee/services/scim/scim-service.ts @@ -790,6 +790,18 @@ export const scimServiceFactory = ({ }); const newGroup = await groupDAL.transaction(async (tx) => { + const conflictingGroup = await groupDAL.findOne({ + name: displayName, + orgId + }); + + if (conflictingGroup) { + throw new ScimRequestError({ + detail: `Group with name '${displayName}' already exists in the organization`, + status: 409 + }); + } + const group = await groupDAL.create( { name: displayName, diff --git a/backend/src/lib/validator/index.ts b/backend/src/lib/validator/index.ts index 68cec8f4b..2400af8ee 100644 --- a/backend/src/lib/validator/index.ts +++ b/backend/src/lib/validator/index.ts @@ -1,3 +1,4 @@ export { isDisposableEmail } from "./validate-email"; export { isValidFolderName, isValidSecretPath } from "./validate-folder-name"; export { blockLocalAndPrivateIpAddresses } from "./validate-url"; +export { isUuidV4 } from "./validate-uuid"; diff --git a/backend/src/lib/validator/validate-uuid.ts b/backend/src/lib/validator/validate-uuid.ts new file mode 100644 index 000000000..a75e147f7 --- /dev/null +++ b/backend/src/lib/validator/validate-uuid.ts @@ -0,0 +1,3 @@ +import { z } from "zod"; + +export const isUuidV4 = (uuid: string) => z.string().uuid().safeParse(uuid).success; diff --git a/backend/src/server/routes/v2/group-project-router.ts b/backend/src/server/routes/v2/group-project-router.ts index cbc54f5ac..9b3bc8fce 100644 --- a/backend/src/server/routes/v2/group-project-router.ts +++ b/backend/src/server/routes/v2/group-project-router.ts @@ -16,7 +16,7 @@ import { ProjectUserMembershipTemporaryMode } from "@app/services/project-member export const registerGroupProjectRouter = async (server: FastifyZodProvider) => { server.route({ method: "POST", - url: "/:projectId/groups/:groupId", + url: "/:projectId/groups/:groupIdOrName", onRequest: verifyAuth([AuthMode.JWT, AuthMode.IDENTITY_ACCESS_TOKEN]), config: { rateLimit: writeLimit @@ -30,7 +30,7 @@ export const registerGroupProjectRouter = async (server: FastifyZodProvider) => ], params: z.object({ projectId: z.string().trim().describe(PROJECTS.ADD_GROUP_TO_PROJECT.projectId), - groupId: z.string().trim().describe(PROJECTS.ADD_GROUP_TO_PROJECT.groupId) + groupIdOrName: z.string().trim().describe(PROJECTS.ADD_GROUP_TO_PROJECT.groupId) }), body: z .object({ @@ -76,7 +76,7 @@ export const registerGroupProjectRouter = async (server: FastifyZodProvider) => actorOrgId: req.permission.orgId, roles: req.body.roles || [{ role: req.body.role }], projectId: req.params.projectId, - groupId: req.params.groupId + groupIdOrName: req.params.groupIdOrName }); return { groupMembership }; diff --git a/backend/src/services/group-project/group-project-service.ts b/backend/src/services/group-project/group-project-service.ts index 1084dcdd9..62a6c08b4 100644 --- a/backend/src/services/group-project/group-project-service.ts +++ b/backend/src/services/group-project/group-project-service.ts @@ -1,7 +1,7 @@ import { ForbiddenError } from "@casl/ability"; import ms from "ms"; -import { ActionProjectType, ProjectMembershipRole, SecretKeyEncoding } from "@app/db/schemas"; +import { ActionProjectType, ProjectMembershipRole, SecretKeyEncoding, TGroups } from "@app/db/schemas"; import { TPermissionServiceFactory } from "@app/ee/services/permission/permission-service"; import { ProjectPermissionActions, ProjectPermissionSub } from "@app/ee/services/permission/project-permission"; import { isAtLeastAsPrivileged } from "@app/lib/casl"; @@ -9,6 +9,7 @@ import { decryptAsymmetric, encryptAsymmetric } from "@app/lib/crypto"; import { infisicalSymmetricDecrypt } from "@app/lib/crypto/encryption"; import { BadRequestError, ForbiddenRequestError, NotFoundError } from "@app/lib/errors"; import { groupBy } from "@app/lib/fn"; +import { isUuidV4 } from "@app/lib/validator"; import { TGroupDALFactory } from "../../ee/services/group/group-dal"; import { TUserGroupMembershipDALFactory } from "../../ee/services/group/user-group-membership-dal"; @@ -62,7 +63,7 @@ export const groupProjectServiceFactory = ({ actorAuthMethod, roles, projectId, - groupId + groupIdOrName }: TCreateProjectGroupDTO) => { const project = await projectDAL.findById(projectId); @@ -79,13 +80,23 @@ export const groupProjectServiceFactory = ({ }); ForbiddenError.from(permission).throwUnlessCan(ProjectPermissionActions.Create, ProjectPermissionSub.Groups); - const group = await groupDAL.findOne({ orgId: actorOrgId, id: groupId }); - if (!group) throw new NotFoundError({ message: `Failed to find group with ID ${groupId}` }); + const isUuid = isUuidV4(groupIdOrName); + + let group: TGroups | null = null; + // id can only be a uuid, name can be anything + if (isUuid) { + group = await groupDAL.findOne({ orgId: actorOrgId, id: groupIdOrName }); + } + if (!group) { + group = await groupDAL.findOne({ orgId: actorOrgId, name: groupIdOrName }); + } + + if (!group) throw new NotFoundError({ message: `Failed to find group with ID or name ${groupIdOrName}` }); const existingGroup = await groupProjectDAL.findOne({ groupId: group.id, projectId: project.id }); if (existingGroup) throw new BadRequestError({ - message: `Group with ID ${groupId} already exists in project with id ${project.id}` + message: `Group with ID ${group.id} already exists in project with id ${project.id}` }); for await (const { role: requestedRoleChange } of roles) { diff --git a/backend/src/services/group-project/group-project-types.ts b/backend/src/services/group-project/group-project-types.ts index 1e1794963..f77615d2e 100644 --- a/backend/src/services/group-project/group-project-types.ts +++ b/backend/src/services/group-project/group-project-types.ts @@ -3,7 +3,7 @@ import { TProjectPermission } from "@app/lib/types"; import { ProjectUserMembershipTemporaryMode } from "../project-membership/project-membership-types"; export type TCreateProjectGroupDTO = { - groupId: string; + groupIdOrName: string; roles: ( | { role: string; diff --git a/backend/src/services/secret-sharing/secret-sharing-service.ts b/backend/src/services/secret-sharing/secret-sharing-service.ts index b262cad5d..b0f66c3e3 100644 --- a/backend/src/services/secret-sharing/secret-sharing-service.ts +++ b/backend/src/services/secret-sharing/secret-sharing-service.ts @@ -1,12 +1,12 @@ import crypto from "node:crypto"; import bcrypt from "bcrypt"; -import { z } from "zod"; import { TSecretSharing } from "@app/db/schemas"; import { TPermissionServiceFactory } from "@app/ee/services/permission/permission-service"; import { BadRequestError, ForbiddenRequestError, NotFoundError, UnauthorizedError } from "@app/lib/errors"; import { SecretSharingAccessType } from "@app/lib/types"; +import { isUuidV4 } from "@app/lib/validator"; import { TKmsServiceFactory } from "../kms/kms-service"; import { TOrgDALFactory } from "../org/org-dal"; @@ -28,8 +28,6 @@ type TSecretSharingServiceFactoryDep = { export type TSecretSharingServiceFactory = ReturnType; -const isUuidV4 = (uuid: string) => z.string().uuid().safeParse(uuid).success; - export const secretSharingServiceFactory = ({ permissionService, secretSharingDAL, From 4e68304262b448225bb9bd27d02161dd5ba74179 Mon Sep 17 00:00:00 2001 From: Daniel Hougaard Date: Thu, 16 Jan 2025 01:08:43 +0100 Subject: [PATCH 2/3] fix: type check --- backend/src/services/group-project/group-project-service.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/backend/src/services/group-project/group-project-service.ts b/backend/src/services/group-project/group-project-service.ts index 62a6c08b4..b84cfe1a2 100644 --- a/backend/src/services/group-project/group-project-service.ts +++ b/backend/src/services/group-project/group-project-service.ts @@ -83,7 +83,6 @@ export const groupProjectServiceFactory = ({ const isUuid = isUuidV4(groupIdOrName); let group: TGroups | null = null; - // id can only be a uuid, name can be anything if (isUuid) { group = await groupDAL.findOne({ orgId: actorOrgId, id: groupIdOrName }); } @@ -139,7 +138,7 @@ export const groupProjectServiceFactory = ({ const projectGroup = await groupProjectDAL.transaction(async (tx) => { const groupProjectMembership = await groupProjectDAL.create( { - groupId: group.id, + groupId: group!.id, projectId: project.id }, tx @@ -174,7 +173,7 @@ export const groupProjectServiceFactory = ({ // share project key with users in group that have not // individually been added to the project and that are not part of // other groups that are in the project - const groupMembers = await userGroupMembershipDAL.findGroupMembersNotInProject(group.id, project.id, tx); + const groupMembers = await userGroupMembershipDAL.findGroupMembersNotInProject(group!.id, project.id, tx); if (groupMembers.length) { const ghostUser = await projectDAL.findProjectGhostUser(project.id, tx); From ef87086272e788a77d64a505328dc1df3ab90647 Mon Sep 17 00:00:00 2001 From: Daniel Hougaard Date: Thu, 16 Jan 2025 17:38:54 +0100 Subject: [PATCH 3/3] fix(groups): unique names, requested changes --- .../src/ee/services/group/group-service.ts | 82 +++++++++++-------- backend/src/ee/services/scim/scim-service.ts | 11 ++- backend/src/lib/api-docs/constants.ts | 2 +- .../server/routes/v2/group-project-router.ts | 2 +- .../group-project/group-project-service.ts | 4 +- 5 files changed, 57 insertions(+), 44 deletions(-) diff --git a/backend/src/ee/services/group/group-service.ts b/backend/src/ee/services/group/group-service.ts index 163956e32..beb03f76e 100644 --- a/backend/src/ee/services/group/group-service.ts +++ b/backend/src/ee/services/group/group-service.ts @@ -32,7 +32,7 @@ type TGroupServiceFactoryDep = { userDAL: Pick; groupDAL: Pick< TGroupDALFactory, - "create" | "findOne" | "update" | "delete" | "findAllGroupPossibleMembers" | "findById" + "create" | "findOne" | "update" | "delete" | "findAllGroupPossibleMembers" | "findById" | "transaction" >; groupProjectDAL: Pick; orgDAL: Pick; @@ -88,19 +88,26 @@ export const groupServiceFactory = ({ if (!hasRequiredPriviledges) throw new ForbiddenRequestError({ message: "Failed to create a more privileged group" }); - const existingGroup = await groupDAL.findOne({ orgId: actorOrgId, name }); - if (existingGroup) { - throw new BadRequestError({ - message: `Failed to create group with name '${name}'. Group with the same name already exists` - }); - } + const group = await groupDAL.transaction(async (tx) => { + const existingGroup = await groupDAL.findOne({ orgId: actorOrgId, name }, tx); + if (existingGroup) { + throw new BadRequestError({ + message: `Failed to create group with name '${name}'. Group with the same name already exists` + }); + } - const group = await groupDAL.create({ - name, - slug: slug || slugify(`${name}-${alphaNumericNanoId(4)}`), - orgId: actorOrgId, - role: isCustomRole ? OrgMembershipRole.Custom : role, - roleId: customRole?.id + const newGroup = await groupDAL.create( + { + name, + slug: slug || slugify(`${name}-${alphaNumericNanoId(4)}`), + orgId: actorOrgId, + role: isCustomRole ? OrgMembershipRole.Custom : role, + roleId: customRole?.id + }, + tx + ); + + return newGroup; }); return group; @@ -152,31 +159,36 @@ export const groupServiceFactory = ({ if (isCustomRole) customRole = customOrgRole; } - if (name) { - const existingGroup = await groupDAL.findOne({ orgId: actorOrgId, name }); + const updatedGroup = await groupDAL.transaction(async (tx) => { + if (name) { + const existingGroup = await groupDAL.findOne({ orgId: actorOrgId, name }, tx); - if (existingGroup && existingGroup.id !== id) { - throw new BadRequestError({ - message: `Failed to update group with name '${name}'. Group with the same name already exists` - }); + if (existingGroup && existingGroup.id !== id) { + throw new BadRequestError({ + message: `Failed to update group with name '${name}'. Group with the same name already exists` + }); + } } - } - const [updatedGroup] = await groupDAL.update( - { - id: group.id - }, - { - name, - slug: slug ? slugify(slug) : undefined, - ...(role - ? { - role: customRole ? OrgMembershipRole.Custom : role, - roleId: customRole?.id ?? null - } - : {}) - } - ); + const [updated] = await groupDAL.update( + { + id: group.id + }, + { + name, + slug: slug ? slugify(slug) : undefined, + ...(role + ? { + role: customRole ? OrgMembershipRole.Custom : role, + roleId: customRole?.id ?? null + } + : {}) + }, + tx + ); + + return updated; + }); return updatedGroup; }; diff --git a/backend/src/ee/services/scim/scim-service.ts b/backend/src/ee/services/scim/scim-service.ts index 32f3c492d..cbad3d3ec 100644 --- a/backend/src/ee/services/scim/scim-service.ts +++ b/backend/src/ee/services/scim/scim-service.ts @@ -790,10 +790,13 @@ export const scimServiceFactory = ({ }); const newGroup = await groupDAL.transaction(async (tx) => { - const conflictingGroup = await groupDAL.findOne({ - name: displayName, - orgId - }); + const conflictingGroup = await groupDAL.findOne( + { + name: displayName, + orgId + }, + tx + ); if (conflictingGroup) { throw new ScimRequestError({ diff --git a/backend/src/lib/api-docs/constants.ts b/backend/src/lib/api-docs/constants.ts index 5235617aa..e6fa7344a 100644 --- a/backend/src/lib/api-docs/constants.ts +++ b/backend/src/lib/api-docs/constants.ts @@ -474,7 +474,7 @@ export const PROJECTS = { }, ADD_GROUP_TO_PROJECT: { projectId: "The ID of the project to add the group to.", - groupId: "The ID of the group to add to the project.", + groupIdOrName: "The ID or name of the group to add to the project.", role: "The role for the group to assume in the project." }, UPDATE_GROUP_IN_PROJECT: { diff --git a/backend/src/server/routes/v2/group-project-router.ts b/backend/src/server/routes/v2/group-project-router.ts index 9b3bc8fce..aa85a8c40 100644 --- a/backend/src/server/routes/v2/group-project-router.ts +++ b/backend/src/server/routes/v2/group-project-router.ts @@ -30,7 +30,7 @@ export const registerGroupProjectRouter = async (server: FastifyZodProvider) => ], params: z.object({ projectId: z.string().trim().describe(PROJECTS.ADD_GROUP_TO_PROJECT.projectId), - groupIdOrName: z.string().trim().describe(PROJECTS.ADD_GROUP_TO_PROJECT.groupId) + groupIdOrName: z.string().trim().describe(PROJECTS.ADD_GROUP_TO_PROJECT.groupIdOrName) }), body: z .object({ diff --git a/backend/src/services/group-project/group-project-service.ts b/backend/src/services/group-project/group-project-service.ts index b84cfe1a2..067ff17b0 100644 --- a/backend/src/services/group-project/group-project-service.ts +++ b/backend/src/services/group-project/group-project-service.ts @@ -80,10 +80,8 @@ export const groupProjectServiceFactory = ({ }); ForbiddenError.from(permission).throwUnlessCan(ProjectPermissionActions.Create, ProjectPermissionSub.Groups); - const isUuid = isUuidV4(groupIdOrName); - let group: TGroups | null = null; - if (isUuid) { + if (isUuidV4(groupIdOrName)) { group = await groupDAL.findOne({ orgId: actorOrgId, id: groupIdOrName }); } if (!group) {