Merge pull request #2997 from Infisical/daniel/unique-group-names

feat(groups): unique names

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<void> {
+  // 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<void> {
+  // Remove the unique constraint
+  await knex.schema.alterTable(TableName.Groups, (t) => {
+    t.dropUnique(["orgId", "name"]);
+  });
+}

backend/src/ee/services/group/group-service.ts

@@ -32,7 +32,7 @@ type TGroupServiceFactoryDep = {
   userDAL: Pick<TUserDALFactory, "find" | "findUserEncKeyByUserIdsBatch" | "transaction" | "findOne">;
   groupDAL: Pick<
     TGroupDALFactory,
-    "create" | "findOne" | "update" | "delete" | "findAllGroupPossibleMembers" | "findById"
+    "create" | "findOne" | "update" | "delete" | "findAllGroupPossibleMembers" | "findById" | "transaction"
   >;
   groupProjectDAL: Pick<TGroupProjectDALFactory, "find">;
   orgDAL: Pick<TOrgDALFactory, "findMembership" | "countAllOrgMembers">;
@@ -88,12 +88,26 @@ export const groupServiceFactory = ({
     if (!hasRequiredPriviledges)
       throw new ForbiddenRequestError({ message: "Failed to create a more privileged group" });
 
-    const group = await groupDAL.create({
+    const group = await groupDAL.transaction(async (tx) => {
-      name,
+      const existingGroup = await groupDAL.findOne({ orgId: actorOrgId, name }, tx);
-      slug: slug || slugify(`${name}-${alphaNumericNanoId(4)}`),
+      if (existingGroup) {
-      orgId: actorOrgId,
+        throw new BadRequestError({
-      role: isCustomRole ? OrgMembershipRole.Custom : role,
+          message: `Failed to create group with name '${name}'. Group with the same name already exists`
-      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;
@@ -145,21 +159,36 @@ export const groupServiceFactory = ({
       if (isCustomRole) customRole = customOrgRole;
     }
 
-    const [updatedGroup] = await groupDAL.update(
+    const updatedGroup = await groupDAL.transaction(async (tx) => {
-      {
+      if (name) {
-        id: group.id
+        const existingGroup = await groupDAL.findOne({ orgId: actorOrgId, name }, tx);
-      },
+
-      {
+        if (existingGroup && existingGroup.id !== id) {
-        name,
+          throw new BadRequestError({
-        slug: slug ? slugify(slug) : undefined,
+            message: `Failed to update group with name '${name}'. Group with the same name already exists`
-        ...(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;
   };

backend/src/ee/services/scim/scim-service.ts

@@ -790,6 +790,21 @@ export const scimServiceFactory = ({
       });
 
     const newGroup = await groupDAL.transaction(async (tx) => {
+      const conflictingGroup = await groupDAL.findOne(
+        {
+          name: displayName,
+          orgId
+        },
+        tx
+      );
+
+      if (conflictingGroup) {
+        throw new ScimRequestError({
+          detail: `Group with name '${displayName}' already exists in the organization`,
+          status: 409
+        });
+      }
+
       const group = await groupDAL.create(
         {
           name: displayName,

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: {

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";

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;

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.groupIdOrName)
       }),
       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 };

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,20 @@ export const groupProjectServiceFactory = ({
     });
     ForbiddenError.from(permission).throwUnlessCan(ProjectPermissionActions.Create, ProjectPermissionSub.Groups);
 
-    const group = await groupDAL.findOne({ orgId: actorOrgId, id: groupId });
+    let group: TGroups | null = null;
-    if (!group) throw new NotFoundError({ message: `Failed to find group with ID ${groupId}` });
+    if (isUuidV4(groupIdOrName)) {
+      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) {
@@ -128,7 +136,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
@@ -163,7 +171,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);

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;

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<typeof secretSharingServiceFactory>;
 
-const isUuidV4 = (uuid: string) => z.string().uuid().safeParse(uuid).success;
-
 export const secretSharingServiceFactory = ({
   permissionService,
   secretSharingDAL,