From c4d0896609dc5b8aed744196ecbb92f769f27983 Mon Sep 17 00:00:00 2001 From: Daniel Hougaard Date: Thu, 16 Jan 2025 01:01:01 +0100 Subject: [PATCH] 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,