Merge pull request #1660 from Infisical/fix/delete-role-error

Fix: Error handling when deleting roles that are assigned to identities or users

backend/scripts/create-migration.ts

@@ -7,10 +7,10 @@ const prompt = promptSync({ sigint: true });
 
 const migrationName = prompt("Enter name for migration: ");
 
+// Remove spaces from migration name and replace with hyphens
+const formattedMigrationName = migrationName.replace(/\s+/g, "-");
+
 execSync(
-  `npx knex migrate:make --knexfile ${path.join(
-    __dirname,
-    "../src/db/knexfile.ts"
-  )} -x ts ${migrationName}`,
+  `npx knex migrate:make --knexfile ${path.join(__dirname, "../src/db/knexfile.ts")} -x ts ${formattedMigrationName}`,
   { stdio: "inherit" }
 );

backend/src/db/migrations/20240405000045_org-memberships-unique-constraint.ts

@@ -0,0 +1,111 @@
+import { Knex } from "knex";
+import { z } from "zod";
+
+import { TableName, TOrgMemberships } from "../schemas";
+
+const validateOrgMembership = (membershipToValidate: TOrgMemberships, firstMembership: TOrgMemberships) => {
+  const firstOrgId = firstMembership.orgId;
+  const firstUserId = firstMembership.userId;
+
+  if (membershipToValidate.id === firstMembership.id) {
+    return;
+  }
+
+  if (membershipToValidate.inviteEmail !== firstMembership.inviteEmail) {
+    throw new Error(`Invite emails are different for the same userId and orgId: ${firstUserId}, ${firstOrgId}`);
+  }
+  if (membershipToValidate.orgId !== firstMembership.orgId) {
+    throw new Error(`OrgIds are different for the same userId and orgId: ${firstUserId}, ${firstOrgId}`);
+  }
+  if (membershipToValidate.role !== firstMembership.role) {
+    throw new Error(`Roles are different for the same userId and orgId: ${firstUserId}, ${firstOrgId}`);
+  }
+  if (membershipToValidate.roleId !== firstMembership.roleId) {
+    throw new Error(`RoleIds are different for the same userId and orgId: ${firstUserId}, ${firstOrgId}`);
+  }
+  if (membershipToValidate.status !== firstMembership.status) {
+    throw new Error(`Statuses are different for the same userId and orgId: ${firstUserId}, ${firstOrgId}`);
+  }
+  if (membershipToValidate.userId !== firstMembership.userId) {
+    throw new Error(`UserIds are different for the same userId and orgId: ${firstUserId}, ${firstOrgId}`);
+  }
+};
+
+export async function up(knex: Knex): Promise<void> {
+  const RowSchema = z.object({
+    userId: z.string(),
+    orgId: z.string(),
+    cnt: z.string()
+  });
+
+  // Transactional find and delete duplicate rows
+  await knex.transaction(async (tx) => {
+    const duplicateRows = await tx(TableName.OrgMembership)
+      .select("userId", "orgId") // Select the userId and orgId so we can group by them
+      .count("* as cnt") // Count the number of rows for each userId and orgId, so we can make sure there are more than 1 row (a duplicate)
+      .groupBy("userId", "orgId")
+      .havingRaw("count(*) > ?", [1]); // Using havingRaw for direct SQL expressions
+
+    // Parse the rows to ensure they are in the correct format, and for type safety
+    const parsedRows = RowSchema.array().parse(duplicateRows);
+
+    // For each of the duplicate rows, loop through and find the actual memberships to delete
+    for (const row of parsedRows) {
+      const count = Number(row.cnt);
+
+      // An extra check to ensure that the count is actually a number, and the number is greater than 2
+      if (typeof count !== "number" || count < 2) {
+        // eslint-disable-next-line no-continue
+        continue;
+      }
+
+      // Find all the organization memberships that have the same userId and orgId
+      // eslint-disable-next-line no-await-in-loop
+      const rowsToDelete = await tx(TableName.OrgMembership).where({
+        userId: row.userId,
+        orgId: row.orgId
+      });
+
+      // Ensure that all the rows have exactly the same value, except id, createdAt, updatedAt
+      for (const rowToDelete of rowsToDelete) {
+        validateOrgMembership(rowToDelete, rowsToDelete[0]);
+      }
+
+      // Find the row with the latest createdAt, which we will keep
+
+      let lowestCreatedAt: number | null = null;
+      let latestCreatedRow: TOrgMemberships | null = null;
+
+      for (const rowToDelete of rowsToDelete) {
+        if (lowestCreatedAt === null || rowToDelete.createdAt.getTime() < lowestCreatedAt) {
+          lowestCreatedAt = rowToDelete.createdAt.getTime();
+          latestCreatedRow = rowToDelete;
+        }
+      }
+      if (!latestCreatedRow) {
+        throw new Error("Failed to find last created membership");
+      }
+
+      // Filter out the latest row from the rows to delete
+      const membershipIdsToDelete = rowsToDelete.map((r) => r.id).filter((id) => id !== latestCreatedRow!.id);
+
+      // eslint-disable-next-line no-await-in-loop
+      const numberOfRowsDeleted = await tx(TableName.OrgMembership).whereIn("id", membershipIdsToDelete).delete();
+
+      // eslint-disable-next-line no-console
+      console.log(
+        `Deleted ${numberOfRowsDeleted} duplicate organization memberships for ${row.userId} and ${row.orgId}`
+      );
+    }
+  });
+
+  await knex.schema.alterTable(TableName.OrgMembership, (table) => {
+    table.unique(["userId", "orgId"]);
+  });
+}
+
+export async function down(knex: Knex): Promise<void> {
+  await knex.schema.alterTable(TableName.OrgMembership, (table) => {
+    table.dropUnique(["userId", "orgId"]);
+  });
+}

backend/src/server/routes/index.ts

@@ -411,7 +411,12 @@ export const registerRoutes = async (
     folderDAL
   });
 
-  const projectRoleService = projectRoleServiceFactory({ permissionService, projectRoleDAL });
+  const projectRoleService = projectRoleServiceFactory({
+    permissionService,
+    projectRoleDAL,
+    projectUserMembershipRoleDAL,
+    identityProjectMembershipRoleDAL
+  });
 
   const snapshotService = secretSnapshotServiceFactory({
     permissionService,

backend/src/services/project-role/project-role-service.ts

@@ -14,16 +14,25 @@ import {
 import { BadRequestError } from "@app/lib/errors";
 
 import { ActorAuthMethod, ActorType } from "../auth/auth-type";
+import { TIdentityProjectMembershipRoleDALFactory } from "../identity-project/identity-project-membership-role-dal";
+import { TProjectUserMembershipRoleDALFactory } from "../project-membership/project-user-membership-role-dal";
 import { TProjectRoleDALFactory } from "./project-role-dal";
 
 type TProjectRoleServiceFactoryDep = {
   projectRoleDAL: TProjectRoleDALFactory;
   permissionService: Pick<TPermissionServiceFactory, "getProjectPermission" | "getUserProjectPermission">;
+  identityProjectMembershipRoleDAL: TIdentityProjectMembershipRoleDALFactory;
+  projectUserMembershipRoleDAL: TProjectUserMembershipRoleDALFactory;
 };
 
 export type TProjectRoleServiceFactory = ReturnType<typeof projectRoleServiceFactory>;
 
-export const projectRoleServiceFactory = ({ projectRoleDAL, permissionService }: TProjectRoleServiceFactoryDep) => {
+export const projectRoleServiceFactory = ({
+  projectRoleDAL,
+  permissionService,
+  identityProjectMembershipRoleDAL,
+  projectUserMembershipRoleDAL
+}: TProjectRoleServiceFactoryDep) => {
   const createRole = async (
     actor: ActorType,
     actorId: string,
@@ -96,8 +105,25 @@ export const projectRoleServiceFactory = ({ projectRoleDAL, permissionService }:
       actorOrgId
     );
     ForbiddenError.from(permission).throwUnlessCan(ProjectPermissionActions.Delete, ProjectPermissionSub.Role);
+
+    const identityRole = await identityProjectMembershipRoleDAL.findOne({ customRoleId: roleId });
+    const projectUserRole = await projectUserMembershipRoleDAL.findOne({ customRoleId: roleId });
+
+    if (identityRole) {
+      throw new BadRequestError({
+        message: "The role is assigned to one or more identities. Make sure to unassign them before deleting the role.",
+        name: "Delete role"
+      });
+    }
+    if (projectUserRole) {
+      throw new BadRequestError({
+        message: "The role is assigned to one or more users. Make sure to unassign them before deleting the role.",
+        name: "Delete role"
+      });
+    }
+
     const [deletedRole] = await projectRoleDAL.delete({ id: roleId, projectId });
-    if (!deletedRole) throw new BadRequestError({ message: "Role not found", name: "Update role" });
+    if (!deletedRole) throw new BadRequestError({ message: "Role not found", name: "Delete role" });
 
     return deletedRole;
   };

docs/mint.json

@@ -472,7 +472,7 @@
           ]
         },
         {
-          "group": "Secret tags",
+          "group": "Secret Tags",
           "pages": [
             "api-reference/endpoints/secret-tags/list",
             "api-reference/endpoints/secret-tags/create",
@@ -492,7 +492,7 @@
           ]
         },
         {
-          "group": "Secret imports",
+          "group": "Secret Imports",
           "pages": [
             "api-reference/endpoints/secret-imports/list",
             "api-reference/endpoints/secret-imports/create",

docs/style.css

@@ -63,6 +63,30 @@
   border-color: #ebebeb;
 }
 
+#content-area .mt-8 .rounded-xl{
+  border-radius: 0;
+}
+
+#content-area .mt-8 .rounded-lg{
+  border-radius: 0;
+}
+
+#content-area .mt-6 .rounded-xl{
+  border-radius: 0;
+}
+
+#content-area .mt-6 .rounded-lg{
+  border-radius: 0;
+}
+
+#content-area .mt-6 .rounded-md{
+  border-radius: 0;
+}
+
+#content-area .mt-8 .rounded-md{
+  border-radius: 0;
+}
+
 #content-area div.my-4{
   border-radius: 0;
   border-width: 1px;
@@ -78,6 +102,10 @@
   border-radius: 0;
 }
 
+#content-area a {
+  border-radius: 0;
+}
+
 #content-area .not-prose {
   border-radius: 0;
 }

frontend/src/views/Org/MembersPage/components/OrgRoleTabSection/OrgRoleTable.tsx

@@ -31,7 +31,7 @@ export const OrgRoleTable = ({ onSelectRole }: Props) => {
   const [searchRoles, setSearchRoles] = useState("");
   const { currentOrg } = useOrganization();
   const orgId = currentOrg?.id || "";
-  
+
   const { popUp, handlePopUpOpen, handlePopUpClose } = usePopUp(["deleteRole"] as const);
 
   const { data: roles, isLoading: isRolesLoading } = useGetOrgRoles(orgId);
@@ -49,7 +49,7 @@ export const OrgRoleTable = ({ onSelectRole }: Props) => {
       handlePopUpClose("deleteRole");
     } catch (err) {
       console.log(err);
-      createNotification({ type: "error", text: "Failed to create role" });
+      createNotification({ type: "error", text: "Failed to delete role" });
     }
   };
 

frontend/src/views/Project/MembersPage/components/ProjectRoleListTab/components/ProjectRoleList/ProjectRoleList.tsx

@@ -29,7 +29,7 @@ type Props = {
 
 export const ProjectRoleList = ({ onSelectRole }: Props) => {
   const [searchRoles, setSearchRoles] = useState("");
-  
+
   const { popUp, handlePopUpOpen, handlePopUpClose } = usePopUp(["deleteRole"] as const);
   const { currentWorkspace } = useWorkspace();
   const workspaceId = currentWorkspace?.id || "";
@@ -50,7 +50,7 @@ export const ProjectRoleList = ({ onSelectRole }: Props) => {
       handlePopUpClose("deleteRole");
     } catch (err) {
       console.log(err);
-      createNotification({ type: "error", text: "Failed to create role" });
+      createNotification({ type: "error", text: "Failed to delete role" });
     }
   };