From 2c736c7a5d2aa7b976b23c19e200abe1d608ddac Mon Sep 17 00:00:00 2001 From: x032205 Date: Mon, 8 Dec 2025 15:45:38 -0500 Subject: [PATCH] address reviews --- .../pam-account/pam-account-service.ts | 11 +- backend/src/server/routes/index.ts | 8 +- .../approval-policy/approval-policy-dal.ts | 196 +---------------- .../approval-policy-service.ts | 26 ++- .../approval-policy/approval-policy-types.ts | 8 +- .../approval-policy/approval-request-dal.ts | 199 ++++++++++++++++++ .../pam-access/pam-access-policy-factory.ts | 21 +- .../pam-access/pam-access-policy-schemas.ts | 16 +- .../resource-cleanup-queue.ts | 2 +- 9 files changed, 266 insertions(+), 221 deletions(-) create mode 100644 backend/src/services/approval-policy/approval-request-dal.ts diff --git a/backend/src/ee/services/pam-account/pam-account-service.ts b/backend/src/ee/services/pam-account/pam-account-service.ts index d3c7422d85..c3baeff0c4 100644 --- a/backend/src/ee/services/pam-account/pam-account-service.ts +++ b/backend/src/ee/services/pam-account/pam-account-service.ts @@ -26,12 +26,10 @@ import { } from "@app/lib/errors"; import { logger } from "@app/lib/logger"; import { OrgServiceActor } from "@app/lib/types"; -import { - TApprovalPolicyDALFactory, - TApprovalRequestGrantsDALFactory -} from "@app/services/approval-policy/approval-policy-dal"; +import { TApprovalPolicyDALFactory } from "@app/services/approval-policy/approval-policy-dal"; import { ApprovalPolicyType } from "@app/services/approval-policy/approval-policy-enums"; import { APPROVAL_POLICY_FACTORY_MAP } from "@app/services/approval-policy/approval-policy-factory"; +import { TApprovalRequestGrantsDALFactory } from "@app/services/approval-policy/approval-request-dal"; import { ActorType } from "@app/services/auth/auth-type"; import { TKmsServiceFactory } from "@app/services/kms/kms-service"; import { KmsDataKey } from "@app/services/kms/kms-types"; @@ -579,9 +577,8 @@ export const pamAccountServiceFactory = ({ const canAccess = await fac.canAccess(approvalRequestGrantsDAL, resource.projectId, actor.id, inputs); - if (canAccess) { - // Grant exists, allow access without checking permission - } else { + // Grant does not exist, check policy and fallback to permission check + if (!canAccess) { const policy = await fac.matchPolicy(approvalPolicyDAL, resource.projectId, inputs); if (policy) { diff --git a/backend/src/server/routes/index.ts b/backend/src/server/routes/index.ts index e191aa0432..6611f4a594 100644 --- a/backend/src/server/routes/index.ts +++ b/backend/src/server/routes/index.ts @@ -162,14 +162,16 @@ import { appConnectionServiceFactory } from "@app/services/app-connection/app-co import { approvalPolicyDALFactory, approvalPolicyStepApproversDALFactory, - approvalPolicyStepsDALFactory, + approvalPolicyStepsDALFactory +} from "@app/services/approval-policy/approval-policy-dal"; +import { approvalPolicyServiceFactory } from "@app/services/approval-policy/approval-policy-service"; +import { approvalRequestApprovalsDALFactory, approvalRequestDALFactory, approvalRequestGrantsDALFactory, approvalRequestStepEligibleApproversDALFactory, approvalRequestStepsDALFactory -} from "@app/services/approval-policy/approval-policy-dal"; -import { approvalPolicyServiceFactory } from "@app/services/approval-policy/approval-policy-service"; +} from "@app/services/approval-policy/approval-request-dal"; import { authDALFactory } from "@app/services/auth/auth-dal"; import { authLoginServiceFactory } from "@app/services/auth/auth-login-service"; import { authPaswordServiceFactory } from "@app/services/auth/auth-password-service"; diff --git a/backend/src/services/approval-policy/approval-policy-dal.ts b/backend/src/services/approval-policy/approval-policy-dal.ts index 3d7bffebd1..67dd7a5211 100644 --- a/backend/src/services/approval-policy/approval-policy-dal.ts +++ b/backend/src/services/approval-policy/approval-policy-dal.ts @@ -1,14 +1,9 @@ import { TDbClient } from "@app/db"; -import { TableName, TApprovalRequestApprovals } from "@app/db/schemas"; +import { TableName } from "@app/db/schemas"; import { DatabaseError } from "@app/lib/errors"; import { ormify } from "@app/lib/knex"; -import { - ApprovalPolicyType, - ApprovalRequestGrantStatus, - ApprovalRequestStatus, - ApproverType -} from "./approval-policy-enums"; +import { ApprovalPolicyType, ApproverType } from "./approval-policy-enums"; import { ApprovalPolicyStep } from "./approval-policy-types"; // Approval Policy @@ -153,190 +148,3 @@ export const approvalPolicyStepApproversDALFactory = (db: TDbClient) => { const orm = ormify(db, TableName.ApprovalPolicyStepApprovers); return orm; }; - -// Approval Request -export type TApprovalRequestDALFactory = ReturnType; -export const approvalRequestDALFactory = (db: TDbClient) => { - const orm = ormify(db, TableName.ApprovalRequests); - - const findStepsByRequestId = async (requestId: string) => { - try { - const dbInstance = db.replicaNode(); - const steps = await dbInstance(TableName.ApprovalRequestSteps).where({ requestId }).orderBy("stepNumber", "asc"); - - if (!steps.length) { - return []; - } - - const stepIds = steps.map((step) => step.id); - - const [approvers, approvals] = await Promise.all([ - dbInstance(TableName.ApprovalRequestStepEligibleApprovers) - .whereIn("stepId", stepIds) - .select("stepId", "userId", "groupId"), - dbInstance(TableName.ApprovalRequestApprovals).whereIn("stepId", stepIds) - ]); - - const approversByStepId = approvers.reduce>( - (acc, approver) => { - const stepApprovers = acc[approver.stepId] || []; - stepApprovers.push({ - type: approver.userId ? ApproverType.User : ApproverType.Group, - id: (approver.userId || approver.groupId) as string - }); - acc[approver.stepId] = stepApprovers; - return acc; - }, - {} - ); - - const approvalsByStepId = approvals.reduce>((acc, approval) => { - const stepApprovals = acc[approval.stepId] || []; - stepApprovals.push(approval); - acc[approval.stepId] = stepApprovals; - return acc; - }, {}); - - return steps.map((step) => { - return { - ...step, - approvers: approversByStepId[step.id] || [], - approvals: approvalsByStepId[step.id] || [] - }; - }); - } catch (error) { - throw new DatabaseError({ error, name: "Find approval request steps" }); - } - }; - - const findByProjectId = async (policyType: ApprovalPolicyType, projectId: string) => { - try { - const dbInstance = db.replicaNode(); - const requests = await dbInstance(TableName.ApprovalRequests).where({ type: policyType, projectId }); - - if (!requests.length) { - return []; - } - - const requestIds = requests.map((req) => req.id); - - const steps = await dbInstance(TableName.ApprovalRequestSteps) - .whereIn("requestId", requestIds) - .orderBy("stepNumber", "asc"); - - const stepsByRequestId: Record = {}; - - if (steps.length) { - const stepIds = steps.map((step) => step.id); - - const [approvers, approvals] = await Promise.all([ - dbInstance(TableName.ApprovalRequestStepEligibleApprovers) - .whereIn("stepId", stepIds) - .select("stepId", "userId", "groupId"), - dbInstance(TableName.ApprovalRequestApprovals).whereIn("stepId", stepIds) - ]); - - const approversByStepId = approvers.reduce>( - (acc, approver) => { - const stepApprovers = acc[approver.stepId] || []; - stepApprovers.push({ - type: approver.userId ? ApproverType.User : ApproverType.Group, - id: (approver.userId || approver.groupId) as string - }); - acc[approver.stepId] = stepApprovers; - return acc; - }, - {} - ); - - const approvalsByStepId = approvals.reduce>((acc, approval) => { - const stepApprovals = acc[approval.stepId] || []; - stepApprovals.push(approval); - acc[approval.stepId] = stepApprovals; - return acc; - }, {}); - - steps.forEach((step) => { - const formattedStep = { - ...step, - approvers: approversByStepId[step.id] || [], - approvals: approvalsByStepId[step.id] || [] - }; - - if (!stepsByRequestId[step.requestId]) { - stepsByRequestId[step.requestId] = []; - } - stepsByRequestId[step.requestId].push(formattedStep); - }); - } - - return requests.map((req) => ({ - ...req, - steps: stepsByRequestId[req.id] || [] - })); - } catch (error) { - throw new DatabaseError({ error, name: "Find approval requests by project id" }); - } - }; - - const markExpiredRequests = async () => { - try { - const result = await db(TableName.ApprovalRequests) - .where("status", ApprovalRequestStatus.Pending) - .whereNotNull("expiresAt") - .where("expiresAt", "<", new Date()) - .update({ status: ApprovalRequestStatus.Expired }); - - return result; - } catch (error) { - throw new DatabaseError({ error, name: "Mark expired approval requests" }); - } - }; - - return { ...orm, findStepsByRequestId, findByProjectId, markExpiredRequests }; -}; - -// Approval Request Steps -export type TApprovalRequestStepsDALFactory = ReturnType; -export const approvalRequestStepsDALFactory = (db: TDbClient) => { - const orm = ormify(db, TableName.ApprovalRequestSteps); - return orm; -}; - -// Approval Request Step Eligible Approvers -export type TApprovalRequestStepEligibleApproversDALFactory = ReturnType< - typeof approvalRequestStepEligibleApproversDALFactory ->; -export const approvalRequestStepEligibleApproversDALFactory = (db: TDbClient) => { - const orm = ormify(db, TableName.ApprovalRequestStepEligibleApprovers); - return orm; -}; - -// Approval Request Grants -export type TApprovalRequestGrantsDALFactory = ReturnType; -export const approvalRequestGrantsDALFactory = (db: TDbClient) => { - const orm = ormify(db, TableName.ApprovalRequestGrants); - - const markExpiredGrants = async () => { - try { - const result = await db(TableName.ApprovalRequestGrants) - .where("status", ApprovalRequestGrantStatus.Active) - .whereNotNull("expiresAt") - .where("expiresAt", "<", new Date()) - .update({ status: ApprovalRequestGrantStatus.Expired }); - - return result; - } catch (error) { - throw new DatabaseError({ error, name: "Mark expired approval grants" }); - } - }; - - return { ...orm, markExpiredGrants }; -}; - -// Approval Request Approvals -export type TApprovalRequestApprovalsDALFactory = ReturnType; -export const approvalRequestApprovalsDALFactory = (db: TDbClient) => { - const orm = ormify(db, TableName.ApprovalRequestApprovals); - return orm; -}; diff --git a/backend/src/services/approval-policy/approval-policy-service.ts b/backend/src/services/approval-policy/approval-policy-service.ts index 4c675be98d..80fa820ac4 100644 --- a/backend/src/services/approval-policy/approval-policy-service.ts +++ b/backend/src/services/approval-policy/approval-policy-service.ts @@ -18,12 +18,7 @@ import { TProjectMembershipDALFactory } from "../project-membership/project-memb import { TApprovalPolicyDALFactory, TApprovalPolicyStepApproversDALFactory, - TApprovalPolicyStepsDALFactory, - TApprovalRequestApprovalsDALFactory, - TApprovalRequestDALFactory, - TApprovalRequestGrantsDALFactory, - TApprovalRequestStepEligibleApproversDALFactory, - TApprovalRequestStepsDALFactory + TApprovalPolicyStepsDALFactory } from "./approval-policy-dal"; import { ApprovalPolicyType, @@ -41,6 +36,13 @@ import { TCreateRequestDTO, TUpdatePolicyDTO } from "./approval-policy-types"; +import { + TApprovalRequestApprovalsDALFactory, + TApprovalRequestDALFactory, + TApprovalRequestGrantsDALFactory, + TApprovalRequestStepEligibleApproversDALFactory, + TApprovalRequestStepsDALFactory +} from "./approval-request-dal"; type TApprovalPolicyServiceFactoryDep = { approvalPolicyDAL: TApprovalPolicyDALFactory; @@ -394,11 +396,17 @@ export const approvalPolicyServiceFactory = ({ const policy = await fac.matchPolicy(approvalPolicyDAL, projectId, requestData); if (!policy) { - throw new ForbiddenRequestError({ message: "Policy not found" }); + throw new ForbiddenRequestError({ + message: "No policies match the requested resource, you can access it without a request" + }); } - if (!fac.validateConstraints(policy, requestData)) { - throw new ForbiddenRequestError({ message: "Policy constraints not met" }); + const constraintValidation = fac.validateConstraints(policy, requestData); + if (!constraintValidation.valid) { + const errorMessage = constraintValidation.errors + ? `Policy constraints not met: ${constraintValidation.errors.join("; ")}` + : "Policy constraints not met"; + throw new ForbiddenRequestError({ message: errorMessage }); } let expiresAt: Date | undefined; diff --git a/backend/src/services/approval-policy/approval-policy-types.ts b/backend/src/services/approval-policy/approval-policy-types.ts index f92451c175..8dccc1453b 100644 --- a/backend/src/services/approval-policy/approval-policy-types.ts +++ b/backend/src/services/approval-policy/approval-policy-types.ts @@ -1,7 +1,5 @@ -import { - TApprovalPolicyDALFactory, - TApprovalRequestGrantsDALFactory -} from "@app/services/approval-policy/approval-policy-dal"; +import { TApprovalPolicyDALFactory } from "@app/services/approval-policy/approval-policy-dal"; +import { TApprovalRequestGrantsDALFactory } from "@app/services/approval-policy/approval-request-dal"; import { ApprovalPolicyType, ApproverType } from "./approval-policy-enums"; import { @@ -72,7 +70,7 @@ export type TApprovalRequestFactoryCanAccess = export type TApprovalRequestFactoryValidateConstraints

= ( policy: P, inputs: R -) => boolean; +) => { valid: boolean; errors?: string[] }; export type TApprovalRequestFactoryPostApprovalRoutine = ( approvalRequestGrantsDAL: TApprovalRequestGrantsDALFactory, request: TApprovalRequest diff --git a/backend/src/services/approval-policy/approval-request-dal.ts b/backend/src/services/approval-policy/approval-request-dal.ts new file mode 100644 index 0000000000..ced8bea419 --- /dev/null +++ b/backend/src/services/approval-policy/approval-request-dal.ts @@ -0,0 +1,199 @@ +import { TDbClient } from "@app/db"; +import { TableName, TApprovalRequestApprovals } from "@app/db/schemas"; +import { DatabaseError } from "@app/lib/errors"; +import { ormify } from "@app/lib/knex"; + +import { + ApprovalPolicyType, + ApprovalRequestGrantStatus, + ApprovalRequestStatus, + ApproverType +} from "./approval-policy-enums"; +import { ApprovalPolicyStep } from "./approval-policy-types"; + +// Approval Request +export type TApprovalRequestDALFactory = ReturnType; +export const approvalRequestDALFactory = (db: TDbClient) => { + const orm = ormify(db, TableName.ApprovalRequests); + + const findStepsByRequestId = async (requestId: string) => { + try { + const dbInstance = db.replicaNode(); + const steps = await dbInstance(TableName.ApprovalRequestSteps).where({ requestId }).orderBy("stepNumber", "asc"); + + if (!steps.length) { + return []; + } + + const stepIds = steps.map((step) => step.id); + + const [approvers, approvals] = await Promise.all([ + dbInstance(TableName.ApprovalRequestStepEligibleApprovers) + .whereIn("stepId", stepIds) + .select("stepId", "userId", "groupId"), + dbInstance(TableName.ApprovalRequestApprovals).whereIn("stepId", stepIds) + ]); + + const approversByStepId = approvers.reduce>( + (acc, approver) => { + const stepApprovers = acc[approver.stepId] || []; + stepApprovers.push({ + type: approver.userId ? ApproverType.User : ApproverType.Group, + id: (approver.userId || approver.groupId) as string + }); + acc[approver.stepId] = stepApprovers; + return acc; + }, + {} + ); + + const approvalsByStepId = approvals.reduce>((acc, approval) => { + const stepApprovals = acc[approval.stepId] || []; + stepApprovals.push(approval); + acc[approval.stepId] = stepApprovals; + return acc; + }, {}); + + return steps.map((step) => { + return { + ...step, + approvers: approversByStepId[step.id] || [], + approvals: approvalsByStepId[step.id] || [] + }; + }); + } catch (error) { + throw new DatabaseError({ error, name: "Find approval request steps" }); + } + }; + + const findByProjectId = async (policyType: ApprovalPolicyType, projectId: string) => { + try { + const dbInstance = db.replicaNode(); + const requests = await dbInstance(TableName.ApprovalRequests).where({ type: policyType, projectId }); + + if (!requests.length) { + return []; + } + + const requestIds = requests.map((req) => req.id); + + const steps = await dbInstance(TableName.ApprovalRequestSteps) + .whereIn("requestId", requestIds) + .orderBy("stepNumber", "asc"); + + const stepsByRequestId: Record = {}; + + if (steps.length) { + const stepIds = steps.map((step) => step.id); + + const [approvers, approvals] = await Promise.all([ + dbInstance(TableName.ApprovalRequestStepEligibleApprovers) + .whereIn("stepId", stepIds) + .select("stepId", "userId", "groupId"), + dbInstance(TableName.ApprovalRequestApprovals).whereIn("stepId", stepIds) + ]); + + const approversByStepId = approvers.reduce>( + (acc, approver) => { + const stepApprovers = acc[approver.stepId] || []; + stepApprovers.push({ + type: approver.userId ? ApproverType.User : ApproverType.Group, + id: (approver.userId || approver.groupId) as string + }); + acc[approver.stepId] = stepApprovers; + return acc; + }, + {} + ); + + const approvalsByStepId = approvals.reduce>((acc, approval) => { + const stepApprovals = acc[approval.stepId] || []; + stepApprovals.push(approval); + acc[approval.stepId] = stepApprovals; + return acc; + }, {}); + + steps.forEach((step) => { + const formattedStep = { + ...step, + approvers: approversByStepId[step.id] || [], + approvals: approvalsByStepId[step.id] || [] + }; + + if (!stepsByRequestId[step.requestId]) { + stepsByRequestId[step.requestId] = []; + } + stepsByRequestId[step.requestId].push(formattedStep); + }); + } + + return requests.map((req) => ({ + ...req, + steps: stepsByRequestId[req.id] || [] + })); + } catch (error) { + throw new DatabaseError({ error, name: "Find approval requests by project id" }); + } + }; + + const markExpiredRequests = async () => { + try { + const result = await db(TableName.ApprovalRequests) + .where("status", ApprovalRequestStatus.Pending) + .whereNotNull("expiresAt") + .where("expiresAt", "<", new Date()) + .update({ status: ApprovalRequestStatus.Expired }); + + return result; + } catch (error) { + throw new DatabaseError({ error, name: "Mark expired approval requests" }); + } + }; + + return { ...orm, findStepsByRequestId, findByProjectId, markExpiredRequests }; +}; + +// Approval Request Steps +export type TApprovalRequestStepsDALFactory = ReturnType; +export const approvalRequestStepsDALFactory = (db: TDbClient) => { + const orm = ormify(db, TableName.ApprovalRequestSteps); + return orm; +}; + +// Approval Request Step Eligible Approvers +export type TApprovalRequestStepEligibleApproversDALFactory = ReturnType< + typeof approvalRequestStepEligibleApproversDALFactory +>; +export const approvalRequestStepEligibleApproversDALFactory = (db: TDbClient) => { + const orm = ormify(db, TableName.ApprovalRequestStepEligibleApprovers); + return orm; +}; + +// Approval Request Grants +export type TApprovalRequestGrantsDALFactory = ReturnType; +export const approvalRequestGrantsDALFactory = (db: TDbClient) => { + const orm = ormify(db, TableName.ApprovalRequestGrants); + + const markExpiredGrants = async () => { + try { + const result = await db(TableName.ApprovalRequestGrants) + .where("status", ApprovalRequestGrantStatus.Active) + .whereNotNull("expiresAt") + .where("expiresAt", "<", new Date()) + .update({ status: ApprovalRequestGrantStatus.Expired }); + + return result; + } catch (error) { + throw new DatabaseError({ error, name: "Mark expired approval grants" }); + } + }; + + return { ...orm, markExpiredGrants }; +}; + +// Approval Request Approvals +export type TApprovalRequestApprovalsDALFactory = ReturnType; +export const approvalRequestApprovalsDALFactory = (db: TDbClient) => { + const orm = ormify(db, TableName.ApprovalRequestApprovals); + return orm; +}; diff --git a/backend/src/services/approval-policy/pam-access/pam-access-policy-factory.ts b/backend/src/services/approval-policy/pam-access/pam-access-policy-factory.ts index cd88e92ebe..21b97e0d16 100644 --- a/backend/src/services/approval-policy/pam-access/pam-access-policy-factory.ts +++ b/backend/src/services/approval-policy/pam-access/pam-access-policy-factory.ts @@ -79,8 +79,27 @@ export const pamAccessPolicyFactory: TApprovalResourceFactory< ) => { const reqDuration = ms(inputs.accessDuration); const durationConstraint = policy.constraints.constraints.accessDuration; + const minDuration = ms(durationConstraint.min); + const maxDuration = ms(durationConstraint.max); - return reqDuration >= ms(durationConstraint.min) && reqDuration <= ms(durationConstraint.max); + const errors: string[] = []; + + if (reqDuration < minDuration) { + errors.push( + `Access duration ${inputs.accessDuration} is below the minimum allowed duration of ${durationConstraint.min}` + ); + } + + if (reqDuration > maxDuration) { + errors.push( + `Access duration ${inputs.accessDuration} exceeds the maximum allowed duration of ${durationConstraint.max}` + ); + } + + return { + valid: errors.length === 0, + errors: errors.length > 0 ? errors : undefined + }; }; const postApprovalRoutine: TApprovalRequestFactoryPostApprovalRoutine = async (approvalRequestGrantsDAL, request) => { diff --git a/backend/src/services/approval-policy/pam-access/pam-access-policy-schemas.ts b/backend/src/services/approval-policy/pam-access/pam-access-policy-schemas.ts index d1eb137262..caa2cfa67f 100644 --- a/backend/src/services/approval-policy/pam-access/pam-access-policy-schemas.ts +++ b/backend/src/services/approval-policy/pam-access/pam-access-policy-schemas.ts @@ -1,3 +1,4 @@ +import picomatch from "picomatch"; import { z } from "zod"; import { ms } from "@app/lib/ms"; @@ -19,7 +20,20 @@ export const PamAccessPolicyInputsSchema = z.object({ // Conditions export const PamAccessPolicyConditionsSchema = z .object({ - accountPaths: z.string().array() // TODO(andrey): Add path & wildcard validation + accountPaths: z + .string() + .refine( + (el) => { + try { + picomatch.parse([el]); + return true; + } catch { + return false; + } + }, + { message: "Invalid glob pattern" } + ) + .array() }) .array(); diff --git a/backend/src/services/resource-cleanup/resource-cleanup-queue.ts b/backend/src/services/resource-cleanup/resource-cleanup-queue.ts index f0b9e5e205..68d261193b 100644 --- a/backend/src/services/resource-cleanup/resource-cleanup-queue.ts +++ b/backend/src/services/resource-cleanup/resource-cleanup-queue.ts @@ -7,7 +7,7 @@ import { logger } from "@app/lib/logger"; import { QueueJobs, QueueName, TQueueServiceFactory } from "@app/queue"; import { TUserNotificationDALFactory } from "@app/services/notification/user-notification-dal"; -import { TApprovalRequestDALFactory, TApprovalRequestGrantsDALFactory } from "../approval-policy/approval-policy-dal"; +import { TApprovalRequestDALFactory, TApprovalRequestGrantsDALFactory } from "../approval-policy/approval-request-dal"; import { TIdentityAccessTokenDALFactory } from "../identity-access-token/identity-access-token-dal"; import { TIdentityUaClientSecretDALFactory } from "../identity-ua/identity-ua-client-secret-dal"; import { TOrgServiceFactory } from "../org/org-service";