Merge pull request #4527 from Infisical/fix-approval-policy-org-check

fix: approval policy org check

backend/src/ee/services/access-approval-policy/access-approval-policy-service.ts

@@ -1,14 +1,14 @@
 import { ForbiddenError } from "@casl/ability";
 
-import { AccessScope, ActionProjectType } from "@app/db/schemas";
+import { ActionProjectType } from "@app/db/schemas";
 import { TPermissionServiceFactory } from "@app/ee/services/permission/permission-service-types";
 import { ProjectPermissionActions, ProjectPermissionSub } from "@app/ee/services/permission/project-permission";
 import { BadRequestError, NotFoundError } from "@app/lib/errors";
 import { groupBy } from "@app/lib/fn";
 import { TAdditionalPrivilegeDALFactory } from "@app/services/additional-privilege/additional-privilege-dal";
-import { TMembershipUserDALFactory } from "@app/services/membership-user/membership-user-dal";
 import { TProjectDALFactory } from "@app/services/project/project-dal";
 import { TProjectEnvDALFactory } from "@app/services/project-env/project-env-dal";
+import { TProjectMembershipDALFactory } from "@app/services/project-membership/project-membership-dal";
 import { TUserDALFactory } from "@app/services/user/user-dal";
 
 import { TAccessApprovalRequestDALFactory } from "../access-approval-request/access-approval-request-dal";
@@ -38,13 +38,13 @@ type TAccessApprovalPolicyServiceFactoryDep = {
   projectEnvDAL: Pick<TProjectEnvDALFactory, "find" | "findOne">;
   accessApprovalPolicyApproverDAL: TAccessApprovalPolicyApproverDALFactory;
   accessApprovalPolicyBypasserDAL: TAccessApprovalPolicyBypasserDALFactory;
+  projectMembershipDAL: Pick<TProjectMembershipDALFactory, "findProjectMembershipsByUserIds">;
   groupDAL: TGroupDALFactory;
   userDAL: Pick<TUserDALFactory, "find">;
   accessApprovalRequestDAL: Pick<TAccessApprovalRequestDALFactory, "update" | "find" | "resetReviewByPolicyId">;
   additionalPrivilegeDAL: Pick<TAdditionalPrivilegeDALFactory, "delete">;
   accessApprovalRequestReviewerDAL: Pick<TAccessApprovalRequestReviewerDALFactory, "update" | "delete">;
   accessApprovalPolicyEnvironmentDAL: TAccessApprovalPolicyEnvironmentDALFactory;
-  membershipUserDAL: TMembershipUserDALFactory;
 };
 
 export const accessApprovalPolicyServiceFactory = ({
@@ -60,7 +60,7 @@ export const accessApprovalPolicyServiceFactory = ({
   accessApprovalRequestDAL,
   additionalPrivilegeDAL,
   accessApprovalRequestReviewerDAL,
-  membershipUserDAL
+  projectMembershipDAL
 }: TAccessApprovalPolicyServiceFactoryDep): TAccessApprovalPolicyServiceFactory => {
   const $policyExists = async ({
     envId,
@@ -83,6 +83,21 @@ export const accessApprovalPolicyServiceFactory = ({
     return policyId ? policy && policy.id !== policyId : Boolean(policy);
   };
 
+  const verifyProjectUserMembership = async (userIds: string[], orgId: string, projectId: string) => {
+    if (userIds.length === 0) return;
+    const projectMemberships = (await projectMembershipDAL.findProjectMembershipsByUserIds(orgId, userIds)).filter(
+      (v) => v.projectId === projectId
+    );
+
+    if (projectMemberships.length !== userIds.length) {
+      const projectMemberUserIds = new Set(projectMemberships.map((member) => member.userId));
+      const userIdsNotInProject = userIds.filter((id) => !projectMemberUserIds.has(id));
+      throw new BadRequestError({
+        message: `Some users are not members of the project: ${userIdsNotInProject.join(", ")}`
+      });
+    }
+  };
+
   const createAccessApprovalPolicy: TAccessApprovalPolicyServiceFactory["createAccessApprovalPolicy"] = async ({
     name,
     actor,
@@ -160,7 +175,7 @@ export const accessApprovalPolicyServiceFactory = ({
 
       if (invalidUsernames.length) {
         throw new BadRequestError({
-          message: `Invalid approver user: ${invalidUsernames.join(", ")}`
+          message: `Invalid approver user: ${invalidUsernames.map((i) => i.username).join(", ")}`
         });
       }
 
@@ -171,6 +186,14 @@ export const accessApprovalPolicyServiceFactory = ({
         }))
       );
     }
+
+    if (approverUserIds.length > 0) {
+      await verifyProjectUserMembership(
+        approverUserIds.map((au) => au.id),
+        project.orgId,
+        project.id
+      );
+    }
     let groupBypassers: string[] = [];
     let bypasserUserIds: string[] = [];
 
@@ -257,6 +280,8 @@ export const accessApprovalPolicyServiceFactory = ({
       }
 
       if (bypasserUserIds.length) {
+        await verifyProjectUserMembership(bypasserUserIds, project.orgId, project.id);
+
         await accessApprovalPolicyBypasserDAL.insertMany(
           bypasserUserIds.map((userId) => ({
             bypasserUserId: userId,
@@ -420,23 +445,6 @@ export const accessApprovalPolicyServiceFactory = ({
         bypasserUserIds = [...new Set(bypasserUserIds.concat(bypasserUsers.map((user) => user.id)))];
       }
 
-      // Validate user bypassers
-      if (bypasserUserIds.length > 0) {
-        const orgMemberships = await membershipUserDAL.find({
-          $in: { actorUserId: bypasserUserIds },
-          scopeOrgId: actorOrgId,
-          scope: AccessScope.Organization
-        });
-
-        if (orgMemberships.length !== bypasserUserIds.length) {
-          const foundUserIdsInOrg = new Set(orgMemberships.map((mem) => mem.actorUserId as string));
-          const missingUserIds = bypasserUserIds.filter((id) => !foundUserIdsInOrg.has(id));
-          throw new BadRequestError({
-            message: `One or more specified bypasser users are not part of the organization or do not exist. Invalid or non-member user IDs: ${missingUserIds.join(", ")}`
-          });
-        }
-      }
-
       // Validate group bypassers
       if (groupBypassers.length > 0) {
         const orgGroups = await groupDAL.find({
@@ -487,7 +495,7 @@ export const accessApprovalPolicyServiceFactory = ({
 
           if (invalidUsernames.length) {
             throw new BadRequestError({
-              message: `Invalid approver user: ${invalidUsernames.join(", ")}`
+              message: `Invalid approver user: ${invalidUsernames.map((i) => i.username).join(", ")}`
             });
           }
 
@@ -498,6 +506,14 @@ export const accessApprovalPolicyServiceFactory = ({
             }))
           );
         }
+
+        if (approverUserIds.length > 0) {
+          await verifyProjectUserMembership(
+            approverUserIds.map((au) => au.id),
+            actorOrgId,
+            accessApprovalPolicy.projectId
+          );
+        }
         await accessApprovalPolicyApproverDAL.insertMany(
           approverUserIds.map((el) => ({
             approverUserId: el.id,
@@ -536,6 +552,8 @@ export const accessApprovalPolicyServiceFactory = ({
       await accessApprovalPolicyBypasserDAL.delete({ policyId: doc.id }, tx);
 
       if (bypasserUserIds.length) {
+        await verifyProjectUserMembership(bypasserUserIds, actorOrgId, accessApprovalPolicy.projectId);
+
         await accessApprovalPolicyBypasserDAL.insertMany(
           bypasserUserIds.map((userId) => ({
             bypasserUserId: userId,

backend/src/ee/services/secret-approval-policy/secret-approval-policy-service.ts

@@ -8,6 +8,7 @@ import { BadRequestError, NotFoundError } from "@app/lib/errors";
 import { removeTrailingSlash } from "@app/lib/fn";
 import { containsGlobPatterns } from "@app/lib/picomatch";
 import { TProjectEnvDALFactory } from "@app/services/project-env/project-env-dal";
+import { TProjectMembershipDALFactory } from "@app/services/project-membership/project-membership-dal";
 import { TUserDALFactory } from "@app/services/user/user-dal";
 
 import { ApproverType, BypasserType } from "../access-approval-policy/access-approval-policy-types";
@@ -39,6 +40,7 @@ type TSecretApprovalPolicyServiceFactoryDep = {
   secretApprovalPolicyDAL: TSecretApprovalPolicyDALFactory;
   projectEnvDAL: Pick<TProjectEnvDALFactory, "findOne" | "find">;
   userDAL: Pick<TUserDALFactory, "find">;
+  projectMembershipDAL: Pick<TProjectMembershipDALFactory, "findProjectMembershipsByUserIds">;
   secretApprovalPolicyApproverDAL: TSecretApprovalPolicyApproverDALFactory;
   secretApprovalPolicyBypasserDAL: TSecretApprovalPolicyBypasserDALFactory;
   licenseService: Pick<TLicenseServiceFactory, "getPlan">;
@@ -56,9 +58,25 @@ export const secretApprovalPolicyServiceFactory = ({
   secretApprovalPolicyEnvironmentDAL,
   projectEnvDAL,
   userDAL,
+  projectMembershipDAL,
   licenseService,
   secretApprovalRequestDAL
 }: TSecretApprovalPolicyServiceFactoryDep) => {
+  const verifyProjectUserMembership = async (userIds: string[], orgId: string, projectId: string) => {
+    if (userIds.length === 0) return;
+    const projectMemberships = (await projectMembershipDAL.findProjectMembershipsByUserIds(orgId, userIds)).filter(
+      (v) => v.projectId === projectId
+    );
+
+    if (projectMemberships.length !== userIds.length) {
+      const projectMemberUserIds = new Set(projectMemberships.map((member) => member.userId));
+      const userIdsNotInProject = userIds.filter((id) => !projectMemberUserIds.has(id));
+      throw new BadRequestError({
+        message: `Some users are not members of the project: ${userIdsNotInProject.join(", ")}`
+      });
+    }
+  };
+
   const $policyExists = async ({
     envIds,
     envId,
@@ -202,6 +220,7 @@ export const secretApprovalPolicyServiceFactory = ({
         },
         tx
       );
+
       await secretApprovalPolicyEnvironmentDAL.insertMany(
         envs.map((env) => ({
           envId: env.id,
@@ -233,6 +252,8 @@ export const secretApprovalPolicyServiceFactory = ({
         userApproverIds = userApproverIds.concat(approverUsers.map((user) => user.id));
       }
 
+      await verifyProjectUserMembership(userApproverIds, actorOrgId, projectId);
+
       await secretApprovalPolicyApproverDAL.insertMany(
         userApproverIds.map((approverUserId) => ({
           approverUserId,
@@ -250,6 +271,8 @@ export const secretApprovalPolicyServiceFactory = ({
       );
 
       if (bypasserUserIds.length) {
+        await verifyProjectUserMembership(bypasserUserIds, actorOrgId, projectId);
+
         await secretApprovalPolicyBypasserDAL.insertMany(
           bypasserUserIds.map((userId) => ({
             bypasserUserId: userId,
@@ -425,6 +448,8 @@ export const secretApprovalPolicyServiceFactory = ({
           userApproverIds = userApproverIds.concat(approverUsers.map((user) => user.id));
         }
 
+        await verifyProjectUserMembership(userApproverIds, actorOrgId, secretApprovalPolicy.projectId);
+
         await secretApprovalPolicyApproverDAL.insertMany(
           userApproverIds.map((approverUserId) => ({
             approverUserId,
@@ -458,6 +483,8 @@ export const secretApprovalPolicyServiceFactory = ({
       await secretApprovalPolicyBypasserDAL.delete({ policyId: doc.id }, tx);
 
       if (bypasserUserIds.length) {
+        await verifyProjectUserMembership(bypasserUserIds, actorOrgId, secretApprovalPolicy.projectId);
+
         await secretApprovalPolicyBypasserDAL.insertMany(
           bypasserUserIds.map((userId) => ({
             bypasserUserId: userId,

backend/src/server/routes/index.ts

@@ -707,6 +707,7 @@ export const registerRoutes = async (
     secretApprovalPolicyDAL,
     licenseService,
     userDAL,
+    projectMembershipDAL,
     secretApprovalRequestDAL
   });
 
@@ -1539,7 +1540,7 @@ export const registerRoutes = async (
     accessApprovalRequestDAL,
     accessApprovalRequestReviewerDAL,
     additionalPrivilegeDAL,
-    membershipUserDAL
+    projectMembershipDAL
   });
 
   const accessApprovalRequestService = accessApprovalRequestServiceFactory({