fix(groups): unique names, requested changes

Infisical · Jan 16, 2025 · ef87086 · ef87086
1 parent 4e68304
commit ef87086
Show file tree

Hide file tree

Showing 5 changed files with 57 additions and 44 deletions.
diff --git 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<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,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
@@ -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
@@ -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
@@ -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
@@ -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) {