Merge pull request #2458 from meetcshah19/meet/eng-1443-add-groups-as…

…-eligible-approvers

feat: allow access approvals with user groups

backend/e2e-test/routes/v1/secret-approval-policy.spec.ts

@@ -1,6 +1,7 @@
 import { seedData1 } from "@app/db/seed-data";
+import { ApproverType } from "@app/ee/services/access-approval-policy/access-approval-policy-types";
 
-const createPolicy = async (dto: { name: string; secretPath: string; approvers: string[]; approvals: number }) => {
+const createPolicy = async (dto: { name: string; secretPath: string; approvers: {type: ApproverType.User, id: string}[]; approvals: number }) => {
   const res = await testServer.inject({
     method: "POST",
     url: `/api/v1/secret-approvals`,
@@ -26,7 +27,7 @@ describe("Secret approval policy router", async () => {
     const policy = await createPolicy({
       secretPath: "/",
       approvals: 1,
-      approvers: [seedData1.id],
+      approvers: [{id:seedData1.id, type: ApproverType.User}],
       name: "test-policy"
     });
 

backend/src/db/migrations/20240918005344_add-group-approvals.ts

@@ -0,0 +1,36 @@
+import { Knex } from "knex";
+
+import { TableName } from "../schemas";
+
+export async function up(knex: Knex): Promise<void> {
+  if (await knex.schema.hasTable(TableName.AccessApprovalPolicyApprover)) {
+    // add column approverGroupId to AccessApprovalPolicyApprover
+    await knex.schema.alterTable(TableName.AccessApprovalPolicyApprover, (table) => {
+      // make nullable
+      table.uuid("approverGroupId").nullable().references("id").inTable(TableName.Groups).onDelete("CASCADE");
+      // make approverUserId nullable
+      table.uuid("approverUserId").nullable().alter();
+    });
+    // add column approverGroupId to SecretApprovalPolicyApprover
+    await knex.schema.alterTable(TableName.SecretApprovalPolicyApprover, (table) => {
+      table.uuid("approverGroupId").references("id").inTable(TableName.Groups).onDelete("CASCADE");
+      table.uuid("approverUserId").nullable().alter();
+    });
+  }
+}
+
+export async function down(knex: Knex): Promise<void> {
+  if (await knex.schema.hasTable(TableName.AccessApprovalPolicyApprover)) {
+    // remove
+    await knex.schema.alterTable(TableName.AccessApprovalPolicyApprover, (table) => {
+      table.dropColumn("approverGroupId");
+      table.uuid("approverUserId").notNullable().alter();
+    });
+
+    // remove
+    await knex.schema.alterTable(TableName.SecretApprovalPolicyApprover, (table) => {
+      table.dropColumn("approverGroupId");
+      table.uuid("approverUserId").notNullable().alter();
+    });
+  }
+}

backend/src/db/schemas/access-approval-policies-approvers.ts

@@ -12,7 +12,8 @@ export const AccessApprovalPoliciesApproversSchema = z.object({
   policyId: z.string().uuid(),
   createdAt: z.date(),
   updatedAt: z.date(),
-  approverUserId: z.string().uuid()
+  approverUserId: z.string().uuid().nullable().optional(),
+  approverGroupId: z.string().uuid().nullable().optional()
 });
 
 export type TAccessApprovalPoliciesApprovers = z.infer<typeof AccessApprovalPoliciesApproversSchema>;

backend/src/db/schemas/secret-approval-policies-approvers.ts

@@ -12,7 +12,8 @@ export const SecretApprovalPoliciesApproversSchema = z.object({
   policyId: z.string().uuid(),
   createdAt: z.date(),
   updatedAt: z.date(),
-  approverUserId: z.string().uuid()
+  approverUserId: z.string().uuid().nullable().optional(),
+  approverGroupId: z.string().uuid().nullable().optional()
 });
 
 export type TSecretApprovalPoliciesApprovers = z.infer<typeof SecretApprovalPoliciesApproversSchema>;

backend/src/ee/routes/v1/access-approval-policy-router.ts

@@ -1,6 +1,7 @@
 import { nanoid } from "nanoid";
 import { z } from "zod";
 
+import { ApproverType } from "@app/ee/services/access-approval-policy/access-approval-policy-types";
 import { EnforcementLevel } from "@app/lib/types";
 import { verifyAuth } from "@app/server/plugins/auth/verify-auth";
 import { sapPubSchema } from "@app/server/routes/sanitizedSchemas";
@@ -11,20 +12,18 @@ export const registerAccessApprovalPolicyRouter = async (server: FastifyZodProvi
     url: "/",
     method: "POST",
     schema: {
-      body: z
-        .object({
-          projectSlug: z.string().trim(),
-          name: z.string().optional(),
-          secretPath: z.string().trim().default("/"),
-          environment: z.string(),
-          approvers: z.string().array().min(1),
-          approvals: z.number().min(1).default(1),
-          enforcementLevel: z.nativeEnum(EnforcementLevel).default(EnforcementLevel.Hard)
-        })
-        .refine((data) => data.approvals <= data.approvers.length, {
-          path: ["approvals"],
-          message: "The number of approvals should be lower than the number of approvers."
-        }),
+      body: z.object({
+        projectSlug: z.string().trim(),
+        name: z.string().optional(),
+        secretPath: z.string().trim().default("/"),
+        environment: z.string(),
+        approvers: z
+          .object({ type: z.nativeEnum(ApproverType), id: z.string() })
+          .array()
+          .min(1, { message: "At least one approver should be provided" }),
+        approvals: z.number().min(1).default(1),
+        enforcementLevel: z.nativeEnum(EnforcementLevel).default(EnforcementLevel.Hard)
+      }),
       response: {
         200: z.object({
           approval: sapPubSchema
@@ -58,14 +57,15 @@ export const registerAccessApprovalPolicyRouter = async (server: FastifyZodProvi
         200: z.object({
           approvals: sapPubSchema
             .extend({
-              userApprovers: z
-                .object({
-                  userId: z.string()
-                })
-                .array(),
-              secretPath: z.string().optional().nullable()
+              approvers: z
+                .object({ type: z.nativeEnum(ApproverType), id: z.string().nullable().optional() })
+                .array()
+                .nullable()
+                .optional()
             })
             .array()
+            .nullable()
+            .optional()
         })
       }
     },
@@ -119,22 +119,20 @@ export const registerAccessApprovalPolicyRouter = async (server: FastifyZodProvi
       params: z.object({
         policyId: z.string()
       }),
-      body: z
-        .object({
-          name: z.string().optional(),
-          secretPath: z
-            .string()
-            .trim()
-            .optional()
-            .transform((val) => (val === "" ? "/" : val)),
-          approvers: z.string().array().min(1),
-          approvals: z.number().min(1).default(1),
-          enforcementLevel: z.nativeEnum(EnforcementLevel).default(EnforcementLevel.Hard)
-        })
-        .refine((data) => data.approvals <= data.approvers.length, {
-          path: ["approvals"],
-          message: "The number of approvals should be lower than the number of approvers."
-        }),
+      body: z.object({
+        name: z.string().optional(),
+        secretPath: z
+          .string()
+          .trim()
+          .optional()
+          .transform((val) => (val === "" ? "/" : val)),
+        approvers: z
+          .object({ type: z.nativeEnum(ApproverType), id: z.string() })
+          .array()
+          .min(1, { message: "At least one approver should be provided" }),
+        approvals: z.number().min(1).optional(),
+        enforcementLevel: z.nativeEnum(EnforcementLevel).default(EnforcementLevel.Hard)
+      }),
       response: {
         200: z.object({
           approval: sapPubSchema

backend/src/ee/routes/v1/secret-approval-policy-router.ts

@@ -1,6 +1,7 @@
 import { nanoid } from "nanoid";
 import { z } from "zod";
 
+import { ApproverType } from "@app/ee/services/access-approval-policy/access-approval-policy-types";
 import { removeTrailingSlash } from "@app/lib/fn";
 import { EnforcementLevel } from "@app/lib/types";
 import { readLimit, writeLimit } from "@app/server/config/rateLimiter";
@@ -16,25 +17,23 @@ export const registerSecretApprovalPolicyRouter = async (server: FastifyZodProvi
       rateLimit: writeLimit
     },
     schema: {
-      body: z
-        .object({
-          workspaceId: z.string(),
-          name: z.string().optional(),
-          environment: z.string(),
-          secretPath: z
-            .string()
-            .optional()
-            .nullable()
-            .default("/")
-            .transform((val) => (val ? removeTrailingSlash(val) : val)),
-          approvers: z.string().array().min(1),
-          approvals: z.number().min(1).default(1),
-          enforcementLevel: z.nativeEnum(EnforcementLevel).default(EnforcementLevel.Hard)
-        })
-        .refine((data) => data.approvals <= data.approvers.length, {
-          path: ["approvals"],
-          message: "The number of approvals should be lower than the number of approvers."
-        }),
+      body: z.object({
+        workspaceId: z.string(),
+        name: z.string().optional(),
+        environment: z.string(),
+        secretPath: z
+          .string()
+          .optional()
+          .nullable()
+          .default("/")
+          .transform((val) => (val ? removeTrailingSlash(val) : val)),
+        approvers: z
+          .object({ type: z.nativeEnum(ApproverType), id: z.string() })
+          .array()
+          .min(1, { message: "At least one approver should be provided" }),
+        approvals: z.number().min(1).default(1),
+        enforcementLevel: z.nativeEnum(EnforcementLevel).default(EnforcementLevel.Hard)
+      }),
       response: {
         200: z.object({
           approval: sapPubSchema
@@ -67,23 +66,21 @@ export const registerSecretApprovalPolicyRouter = async (server: FastifyZodProvi
       params: z.object({
         sapId: z.string()
       }),
-      body: z
-        .object({
-          name: z.string().optional(),
-          approvers: z.string().array().min(1),
-          approvals: z.number().min(1).default(1),
-          secretPath: z
-            .string()
-            .optional()
-            .nullable()
-            .transform((val) => (val ? removeTrailingSlash(val) : val))
-            .transform((val) => (val === "" ? "/" : val)),
-          enforcementLevel: z.nativeEnum(EnforcementLevel).optional()
-        })
-        .refine((data) => data.approvals <= data.approvers.length, {
-          path: ["approvals"],
-          message: "The number of approvals should be lower than the number of approvers."
-        }),
+      body: z.object({
+        name: z.string().optional(),
+        approvers: z
+          .object({ type: z.nativeEnum(ApproverType), id: z.string() })
+          .array()
+          .min(1, { message: "At least one approver should be provided" }),
+        approvals: z.number().min(1).default(1),
+        secretPath: z
+          .string()
+          .optional()
+          .nullable()
+          .transform((val) => (val ? removeTrailingSlash(val) : val))
+          .transform((val) => (val === "" ? "/" : val)),
+        enforcementLevel: z.nativeEnum(EnforcementLevel).optional()
+      }),
       response: {
         200: z.object({
           approval: sapPubSchema
@@ -147,9 +144,10 @@ export const registerSecretApprovalPolicyRouter = async (server: FastifyZodProvi
         200: z.object({
           approvals: sapPubSchema
             .extend({
-              userApprovers: z
+              approvers: z
                 .object({
-                  userId: z.string()
+                  id: z.string().nullable().optional(),
+                  type: z.nativeEnum(ApproverType)
                 })
                 .array()
             })
@@ -186,7 +184,7 @@ export const registerSecretApprovalPolicyRouter = async (server: FastifyZodProvi
         200: z.object({
           policy: sapPubSchema
             .extend({
-              userApprovers: z.object({ userId: z.string() }).array()
+              userApprovers: z.object({ userId: z.string().nullable().optional() }).array()
             })
             .optional()
         })

backend/src/ee/routes/v1/secret-approval-request-router.ts

@@ -13,7 +13,7 @@ import { verifyAuth } from "@app/server/plugins/auth/verify-auth";
 import { secretRawSchema } from "@app/server/routes/sanitizedSchemas";
 import { AuthMode } from "@app/services/auth/auth-type";
 
-const approvalRequestUser = z.object({ userId: z.string() }).merge(
+const approvalRequestUser = z.object({ userId: z.string().nullable().optional() }).merge(
   UsersSchema.pick({
     email: true,
     firstName: true,
@@ -46,15 +46,23 @@ export const registerSecretApprovalRequestRouter = async (server: FastifyZodProv
               id: z.string(),
               name: z.string(),
               approvals: z.number(),
-              approvers: z.string().array(),
+              approvers: z
+                .object({
+                  userId: z.string().nullable().optional()
+                })
+                .array(),
               secretPath: z.string().optional().nullable(),
               enforcementLevel: z.string()
             }),
             committerUser: approvalRequestUser,
             commits: z.object({ op: z.string(), secretId: z.string().nullable().optional() }).array(),
             environment: z.string(),
             reviewers: z.object({ userId: z.string(), status: z.string() }).array(),
-            approvers: z.string().array()
+            approvers: z
+              .object({
+                userId: z.string().nullable().optional()
+              })
+              .array()
           }).array()
         })
       }

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

@@ -5,6 +5,8 @@ import { AccessApprovalPoliciesSchema, TableName, TAccessApprovalPolicies } from
 import { DatabaseError } from "@app/lib/errors";
 import { buildFindFilter, ormify, selectAllTableCols, sqlNestRelationships, TFindFilter } from "@app/lib/knex";
 
+import { ApproverType } from "./access-approval-policy-types";
+
 export type TAccessApprovalPolicyDALFactory = ReturnType<typeof accessApprovalPolicyDALFactory>;
 
 export const accessApprovalPolicyDALFactory = (db: TDbClient) => {
@@ -21,6 +23,7 @@ export const accessApprovalPolicyDALFactory = (db: TDbClient) => {
         `${TableName.AccessApprovalPolicyApprover}.policyId`
       )
       .select(tx.ref("approverUserId").withSchema(TableName.AccessApprovalPolicyApprover))
+      .select(tx.ref("approverGroupId").withSchema(TableName.AccessApprovalPolicyApprover))
       .select(tx.ref("name").withSchema(TableName.Environment).as("envName"))
       .select(tx.ref("slug").withSchema(TableName.Environment).as("envSlug"))
       .select(tx.ref("id").withSchema(TableName.Environment).as("envId"))
@@ -30,10 +33,10 @@ export const accessApprovalPolicyDALFactory = (db: TDbClient) => {
     return result;
   };
 
-  const findById = async (id: string, tx?: Knex) => {
+  const findById = async (policyId: string, tx?: Knex) => {
     try {
       const doc = await accessApprovalPolicyFindQuery(tx || db.replicaNode(), {
-        [`${TableName.AccessApprovalPolicy}.id` as "id"]: id
+        [`${TableName.AccessApprovalPolicy}.id` as "id"]: policyId
       });
       const formattedDoc = sqlNestRelationships({
         data: doc,
@@ -50,9 +53,18 @@ export const accessApprovalPolicyDALFactory = (db: TDbClient) => {
         childrenMapper: [
           {
             key: "approverUserId",
-            label: "userApprovers" as const,
-            mapper: ({ approverUserId }) => ({
-              userId: approverUserId
+            label: "approvers" as const,
+            mapper: ({ approverUserId: id }) => ({
+              id,
+              type: "user"
+            })
+          },
+          {
+            key: "approverGroupId",
+            label: "approvers" as const,
+            mapper: ({ approverGroupId: id }) => ({
+              id,
+              type: "group"
             })
           }
         ]
@@ -84,9 +96,18 @@ export const accessApprovalPolicyDALFactory = (db: TDbClient) => {
         childrenMapper: [
           {
             key: "approverUserId",
-            label: "userApprovers" as const,
-            mapper: ({ approverUserId }) => ({
-              userId: approverUserId
+            label: "approvers" as const,
+            mapper: ({ approverUserId: id }) => ({
+              id,
+              type: ApproverType.User
+            })
+          },
+          {
+            key: "approverGroupId",
+            label: "approvers" as const,
+            mapper: ({ approverGroupId: id }) => ({
+              id,
+              type: ApproverType.Group
             })
           }
         ]