From d5a4eb609aa9682b8cbac699fea12dfc4ef09bb9 Mon Sep 17 00:00:00 2001 From: Daniel Hougaard Date: Fri, 27 Sep 2024 21:22:14 +0400 Subject: [PATCH] fix: error improvements --- .../access-approval-policy-fns.ts | 28 ++++-------- .../access-approval-policy-service.ts | 45 +++++++++++++------ .../access-approval-policy-types.ts | 8 +--- .../access-approval-request-service.ts | 30 ++++++++----- .../services/permission/permission-service.ts | 4 +- .../identity-access-token-service.ts | 18 ++++---- backend/src/services/org/org-service.ts | 6 ++- .../src/services/project/project-service.ts | 4 +- .../service-token/service-token-service.ts | 4 +- 9 files changed, 79 insertions(+), 68 deletions(-) diff --git a/backend/src/ee/services/access-approval-policy/access-approval-policy-fns.ts b/backend/src/ee/services/access-approval-policy/access-approval-policy-fns.ts index 96d95be85..35a0df6c5 100644 --- a/backend/src/ee/services/access-approval-policy/access-approval-policy-fns.ts +++ b/backend/src/ee/services/access-approval-policy/access-approval-policy-fns.ts @@ -1,23 +1,21 @@ import { ForbiddenError, subject } from "@casl/ability"; -import { BadRequestError, ForbiddenRequestError } from "@app/lib/errors"; import { ActorType } from "@app/services/auth/auth-type"; import { ProjectPermissionActions, ProjectPermissionSub } from "../permission/project-permission"; -import { TVerifyApprovers, VerifyApproversError } from "./access-approval-policy-types"; +import { TIsApproversValid } from "./access-approval-policy-types"; -export const verifyApprovers = async ({ +export const isApproversValid = async ({ userIds, projectId, orgId, envSlug, actorAuthMethod, secretPath, - permissionService, - error -}: TVerifyApprovers) => { - for await (const userId of userIds) { - try { + permissionService +}: TIsApproversValid) => { + try { + for await (const userId of userIds) { const { permission: approverPermission } = await permissionService.getProjectPermission( ActorType.USER, userId, @@ -30,17 +28,9 @@ export const verifyApprovers = async ({ ProjectPermissionActions.Create, subject(ProjectPermissionSub.Secrets, { environment: envSlug, secretPath }) ); - } catch (err) { - if (error === VerifyApproversError.BadRequestError) { - throw new BadRequestError({ - message: "One or more approvers doesn't have access to be specified secret path" - }); - } - if (error === VerifyApproversError.ForbiddenError) { - throw new ForbiddenRequestError({ - message: "You don't have access to approve this request" - }); - } } + } catch (err) { + return false; } + return true; }; diff --git a/backend/src/ee/services/access-approval-policy/access-approval-policy-service.ts b/backend/src/ee/services/access-approval-policy/access-approval-policy-service.ts index 013444166..2633a026f 100644 --- a/backend/src/ee/services/access-approval-policy/access-approval-policy-service.ts +++ b/backend/src/ee/services/access-approval-policy/access-approval-policy-service.ts @@ -2,7 +2,7 @@ import { ForbiddenError } from "@casl/ability"; import { TPermissionServiceFactory } from "@app/ee/services/permission/permission-service"; import { ProjectPermissionActions, ProjectPermissionSub } from "@app/ee/services/permission/project-permission"; -import { BadRequestError, NotFoundError } from "@app/lib/errors"; +import { BadRequestError, ForbiddenRequestError, NotFoundError } from "@app/lib/errors"; 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"; @@ -11,7 +11,7 @@ import { TUserDALFactory } from "@app/services/user/user-dal"; import { TGroupDALFactory } from "../group/group-dal"; import { TAccessApprovalPolicyApproverDALFactory } from "./access-approval-policy-approver-dal"; import { TAccessApprovalPolicyDALFactory } from "./access-approval-policy-dal"; -import { verifyApprovers } from "./access-approval-policy-fns"; +import { isApproversValid } from "./access-approval-policy-fns"; import { ApproverType, TCreateAccessApprovalPolicy, @@ -19,8 +19,7 @@ import { TGetAccessApprovalPolicyByIdDTO, TGetAccessPolicyCountByEnvironmentDTO, TListAccessApprovalPoliciesDTO, - TUpdateAccessApprovalPolicy, - VerifyApproversError + TUpdateAccessApprovalPolicy } from "./access-approval-policy-types"; type TSecretApprovalPolicyServiceFactoryDep = { @@ -133,17 +132,22 @@ export const accessApprovalPolicyServiceFactory = ({ .map((user) => user.id); verifyAllApprovers.push(...verifyGroupApprovers); - await verifyApprovers({ + const approversValid = await isApproversValid({ projectId: project.id, orgId: actorOrgId, envSlug: environment, secretPath, actorAuthMethod, permissionService, - userIds: verifyAllApprovers, - error: VerifyApproversError.BadRequestError + userIds: verifyAllApprovers }); + if (!approversValid) { + throw new BadRequestError({ + message: "One or more approvers doesn't have access to be specified secret path" + }); + } + const accessApproval = await accessApprovalPolicyDAL.transaction(async (tx) => { const doc = await accessApprovalPolicyDAL.create( { @@ -285,17 +289,22 @@ export const accessApprovalPolicyServiceFactory = ({ userApproverIds = userApproverIds.concat(approverUsers.map((user) => user.id)); } - await verifyApprovers({ + const approversValid = await isApproversValid({ projectId: accessApprovalPolicy.projectId, orgId: actorOrgId, envSlug: accessApprovalPolicy.environment.slug, secretPath: doc.secretPath!, actorAuthMethod, permissionService, - userIds: userApproverIds, - error: VerifyApproversError.BadRequestError + userIds: userApproverIds }); + if (!approversValid) { + throw new BadRequestError({ + message: "One or more approvers doesn't have access to be specified secret path" + }); + } + await accessApprovalPolicyApproverDAL.insertMany( userApproverIds.map((userId) => ({ approverUserId: userId, @@ -325,16 +334,22 @@ export const accessApprovalPolicyServiceFactory = ({ .filter((user) => user.isPartOfGroup) .map((user) => user.id); - await verifyApprovers({ + const approversValid = await isApproversValid({ projectId: accessApprovalPolicy.projectId, orgId: actorOrgId, envSlug: accessApprovalPolicy.environment.slug, secretPath: doc.secretPath!, actorAuthMethod, permissionService, - userIds: verifyGroupApprovers, - error: VerifyApproversError.BadRequestError + userIds: verifyGroupApprovers }); + + if (!approversValid) { + throw new BadRequestError({ + message: "One or more approvers doesn't have access to be specified secret path" + }); + } + await accessApprovalPolicyApproverDAL.insertMany( groupApprovers.map((groupId) => ({ approverGroupId: groupId, @@ -398,7 +413,9 @@ export const accessApprovalPolicyServiceFactory = ({ actorAuthMethod, actorOrgId ); - if (!membership) throw new NotFoundError({ message: "User not found in project" }); + if (!membership) { + throw new ForbiddenRequestError({ message: "You are not a member of this project" }); + } const environment = await projectEnvDAL.findOne({ projectId: project.id, slug: envSlug }); if (!environment) throw new NotFoundError({ message: "Environment not found" }); diff --git a/backend/src/ee/services/access-approval-policy/access-approval-policy-types.ts b/backend/src/ee/services/access-approval-policy/access-approval-policy-types.ts index 8a5290581..a42c89e7a 100644 --- a/backend/src/ee/services/access-approval-policy/access-approval-policy-types.ts +++ b/backend/src/ee/services/access-approval-policy/access-approval-policy-types.ts @@ -3,12 +3,7 @@ import { ActorAuthMethod } from "@app/services/auth/auth-type"; import { TPermissionServiceFactory } from "../permission/permission-service"; -export enum VerifyApproversError { - ForbiddenError = "ForbiddenError", - BadRequestError = "BadRequestError" -} - -export type TVerifyApprovers = { +export type TIsApproversValid = { userIds: string[]; permissionService: Pick; envSlug: string; @@ -16,7 +11,6 @@ export type TVerifyApprovers = { secretPath: string; projectId: string; orgId: string; - error: VerifyApproversError; }; export enum ApproverType { diff --git a/backend/src/ee/services/access-approval-request/access-approval-request-service.ts b/backend/src/ee/services/access-approval-request/access-approval-request-service.ts index 4fab994ed..7c1f00a37 100644 --- a/backend/src/ee/services/access-approval-request/access-approval-request-service.ts +++ b/backend/src/ee/services/access-approval-request/access-approval-request-service.ts @@ -17,8 +17,7 @@ import { TUserDALFactory } from "@app/services/user/user-dal"; import { TAccessApprovalPolicyApproverDALFactory } from "../access-approval-policy/access-approval-policy-approver-dal"; import { TAccessApprovalPolicyDALFactory } from "../access-approval-policy/access-approval-policy-dal"; -import { verifyApprovers } from "../access-approval-policy/access-approval-policy-fns"; -import { VerifyApproversError } from "../access-approval-policy/access-approval-policy-types"; +import { isApproversValid } from "../access-approval-policy/access-approval-policy-fns"; import { TGroupDALFactory } from "../group/group-dal"; import { TPermissionServiceFactory } from "../permission/permission-service"; import { TProjectUserAdditionalPrivilegeDALFactory } from "../project-user-additional-privilege/project-user-additional-privilege-dal"; @@ -100,7 +99,7 @@ export const accessApprovalRequestServiceFactory = ({ }: TCreateAccessApprovalRequestDTO) => { const cfg = getConfig(); const project = await projectDAL.findProjectBySlug(projectSlug, actorOrgId); - if (!project) throw new ForbiddenRequestError({ message: "Project not found" }); + if (!project) throw new NotFoundError({ message: "Project not found" }); // Anyone can create an access approval request. const { membership } = await permissionService.getProjectPermission( @@ -110,7 +109,9 @@ export const accessApprovalRequestServiceFactory = ({ actorAuthMethod, actorOrgId ); - if (!membership) throw new ForbiddenRequestError({ message: "You are not a member of this project" }); + if (!membership) { + throw new ForbiddenRequestError({ message: "You are not a member of this project" }); + } const requestedByUser = await userDAL.findById(actorId); if (!requestedByUser) throw new ForbiddenRequestError({ message: "User not found" }); @@ -272,7 +273,9 @@ export const accessApprovalRequestServiceFactory = ({ actorAuthMethod, actorOrgId ); - if (!membership) throw new NotFoundError({ message: "You don't have a membership for the specified project" }); + if (!membership) { + throw new ForbiddenRequestError({ message: "You are not a member of this project" }); + } const policies = await accessApprovalPolicyDAL.find({ projectId: project.id }); let requests = await accessApprovalRequestDAL.findRequestsWithPrivilegeByPolicyIds(policies.map((p) => p.id)); @@ -308,7 +311,9 @@ export const accessApprovalRequestServiceFactory = ({ actorOrgId ); - if (!membership) throw new ForbiddenRequestError({ message: "You are not a member of this project" }); + if (!membership) { + throw new ForbiddenRequestError({ message: "You are not a member of this project" }); + } if ( !hasRole(ProjectMembershipRole.Admin) && @@ -320,17 +325,20 @@ export const accessApprovalRequestServiceFactory = ({ const reviewerProjectMembership = await projectMembershipDAL.findById(membership.id); - await verifyApprovers({ + const approversValid = await isApproversValid({ projectId: accessApprovalRequest.projectId, orgId: actorOrgId, envSlug: accessApprovalRequest.environment, secretPath: accessApprovalRequest.policy.secretPath!, actorAuthMethod, permissionService, - userIds: [reviewerProjectMembership.userId], - error: VerifyApproversError.ForbiddenError + userIds: [reviewerProjectMembership.userId] }); + if (!approversValid) { + throw new ForbiddenRequestError({ message: "You don't have access to approve this request" }); + } + const existingReviews = await accessApprovalRequestReviewerDAL.find({ requestId: accessApprovalRequest.id }); if (existingReviews.some((review) => review.status === ApprovalStatus.REJECTED)) { throw new BadRequestError({ message: "The request has already been rejected by another reviewer" }); @@ -422,7 +430,9 @@ export const accessApprovalRequestServiceFactory = ({ actorAuthMethod, actorOrgId ); - if (!membership) throw new NotFoundError({ message: "You don't have a membership for the specified project" }); + if (!membership) { + throw new ForbiddenRequestError({ message: "You are not a member of this project" }); + } const count = await accessApprovalRequestDAL.getCount({ projectId: project.id }); diff --git a/backend/src/ee/services/permission/permission-service.ts b/backend/src/ee/services/permission/permission-service.ts index 3871f6b45..119ac6fca 100644 --- a/backend/src/ee/services/permission/permission-service.ts +++ b/backend/src/ee/services/permission/permission-service.ts @@ -116,7 +116,7 @@ export const permissionServiceFactory = ({ if (userOrgId && userOrgId !== orgId) throw new ForbiddenRequestError({ message: "Invalid user token. Scoped to different organization." }); const membership = await permissionDAL.getOrgPermission(userId, orgId); - if (!membership) throw new ForbiddenRequestError({ name: "User is not a part of the specified organization" }); + if (!membership) throw new ForbiddenRequestError({ name: "You are not apart of this organization" }); if (membership.role === OrgMembershipRole.Custom && !membership.permissions) { throw new BadRequestError({ name: "Custom organization permission not found" }); } @@ -143,7 +143,7 @@ export const permissionServiceFactory = ({ const getIdentityOrgPermission = async (identityId: string, orgId: string) => { const membership = await permissionDAL.getOrgIdentityPermission(identityId, orgId); - if (!membership) throw new ForbiddenRequestError({ name: "Identity is not a part of the specified organization" }); + if (!membership) throw new ForbiddenRequestError({ name: "Identity is not apart of this organization" }); if (membership.role === OrgMembershipRole.Custom && !membership.permissions) { throw new NotFoundError({ name: "Custom organization permission not found" }); } diff --git a/backend/src/services/identity-access-token/identity-access-token-service.ts b/backend/src/services/identity-access-token/identity-access-token-service.ts index 4825773fb..c625c1407 100644 --- a/backend/src/services/identity-access-token/identity-access-token-service.ts +++ b/backend/src/services/identity-access-token/identity-access-token-service.ts @@ -2,7 +2,7 @@ import jwt, { JwtPayload } from "jsonwebtoken"; import { TableName, TIdentityAccessTokens } from "@app/db/schemas"; import { getConfig } from "@app/lib/config/env"; -import { BadRequestError, ForbiddenRequestError, UnauthorizedError } from "@app/lib/errors"; +import { BadRequestError, UnauthorizedError } from "@app/lib/errors"; import { checkIPAgainstBlocklist, TIp } from "@app/lib/ip"; import { TAccessTokenQueueServiceFactory } from "../access-token-queue/access-token-queue"; @@ -39,7 +39,7 @@ export const identityAccessTokenServiceFactory = ({ if (accessTokenNumUsesLimit > 0 && accessTokenNumUses > 0 && accessTokenNumUses >= accessTokenNumUsesLimit) { await identityAccessTokenDAL.deleteById(tokenId); - throw new ForbiddenRequestError({ + throw new UnauthorizedError({ message: "Unable to renew because access token number of uses limit reached" }); } @@ -55,7 +55,7 @@ export const identityAccessTokenServiceFactory = ({ if (currentDate > expirationDate) { await identityAccessTokenDAL.deleteById(tokenId); - throw new ForbiddenRequestError({ + throw new UnauthorizedError({ message: "Failed to renew MI access token due to TTL expiration" }); } @@ -67,7 +67,7 @@ export const identityAccessTokenServiceFactory = ({ if (currentDate > expirationDate) { await identityAccessTokenDAL.deleteById(tokenId); - throw new ForbiddenRequestError({ + throw new UnauthorizedError({ message: "Failed to renew MI access token due to TTL expiration" }); } @@ -82,7 +82,7 @@ export const identityAccessTokenServiceFactory = ({ identityAccessTokenId: string; }; if (decodedToken.authTokenType !== AuthTokenType.IDENTITY_ACCESS_TOKEN) { - throw new ForbiddenRequestError({ message: "Only identity access tokens can be renewed" }); + throw new BadRequestError({ message: "Only identity access tokens can be renewed" }); } const identityAccessToken = await identityAccessTokenDAL.findOne({ @@ -109,7 +109,7 @@ export const identityAccessTokenServiceFactory = ({ if (currentDate > expirationDate) { await identityAccessTokenDAL.deleteById(identityAccessToken.id); - throw new ForbiddenRequestError({ + throw new UnauthorizedError({ message: "Failed to renew MI access token due to Max TTL expiration" }); } @@ -117,7 +117,7 @@ export const identityAccessTokenServiceFactory = ({ const extendToDate = new Date(currentDate.getTime() + Number(accessTokenTTL * 1000)); if (extendToDate > expirationDate) { await identityAccessTokenDAL.deleteById(identityAccessToken.id); - throw new ForbiddenRequestError({ + throw new UnauthorizedError({ message: "Failed to renew MI access token past its Max TTL expiration" }); } @@ -137,7 +137,7 @@ export const identityAccessTokenServiceFactory = ({ identityAccessTokenId: string; }; if (decodedToken.authTokenType !== AuthTokenType.IDENTITY_ACCESS_TOKEN) { - throw new ForbiddenRequestError({ message: "Only identity access tokens can be revoked" }); + throw new UnauthorizedError({ message: "Only identity access tokens can be revoked" }); } const identityAccessToken = await identityAccessTokenDAL.findOne({ @@ -160,7 +160,7 @@ export const identityAccessTokenServiceFactory = ({ }); if (!identityAccessToken) throw new UnauthorizedError({ message: "No identity access token found" }); if (identityAccessToken.isAccessTokenRevoked) - throw new ForbiddenRequestError({ + throw new UnauthorizedError({ message: "Failed to authorize revoked access token, access token is revoked" }); diff --git a/backend/src/services/org/org-service.ts b/backend/src/services/org/org-service.ts index c80cbba67..faa3adba6 100644 --- a/backend/src/services/org/org-service.ts +++ b/backend/src/services/org/org-service.ts @@ -892,7 +892,7 @@ export const orgServiceFactory = ({ const membership = await orgMembershipDAL.findOrgMembershipById(membershipId); if (!membership) { - throw new NotFoundError({ message: "Failed to find organization membership" }); + throw new NotFoundError({ message: "Organization membership not found" }); } if (membership.orgId !== orgId) { throw new ForbiddenRequestError({ message: "Membership does not belong to organization" }); @@ -937,7 +937,9 @@ export const orgServiceFactory = ({ ForbiddenError.from(permission).throwUnlessCan(OrgPermissionActions.Read, OrgPermissionSubjects.Member); const membership = await orgMembershipDAL.findOrgMembershipById(orgMembershipId); - if (!membership) throw new NotFoundError({ message: "Failed to find organization membership" }); + if (!membership) { + throw new NotFoundError({ message: "Organization membership not found" }); + } if (membership.orgId !== orgId) throw new NotFoundError({ message: "Failed to find organization membership" }); const projectMemberships = await projectMembershipDAL.findProjectMembershipsByUserId(orgId, membership.user.id); diff --git a/backend/src/services/project/project-service.ts b/backend/src/services/project/project-service.ts index a39d42ebe..863cf24c2 100644 --- a/backend/src/services/project/project-service.ts +++ b/backend/src/services/project/project-service.ts @@ -909,9 +909,7 @@ export const projectServiceFactory = ({ ); if (!membership) { - throw new ForbiddenRequestError({ - message: "User is not a member of the project" - }); + throw new ForbiddenRequestError({ message: "You are not a member of this project" }); } const kmsKeyId = await kmsService.getProjectSecretManagerKmsKeyId(projectId); diff --git a/backend/src/services/service-token/service-token-service.ts b/backend/src/services/service-token/service-token-service.ts index 1487d9380..5ecbcf54d 100644 --- a/backend/src/services/service-token/service-token-service.ts +++ b/backend/src/services/service-token/service-token-service.ts @@ -6,7 +6,7 @@ import bcrypt from "bcrypt"; import { TPermissionServiceFactory } from "@app/ee/services/permission/permission-service"; import { ProjectPermissionActions, ProjectPermissionSub } from "@app/ee/services/permission/project-permission"; import { getConfig } from "@app/lib/config/env"; -import { ForbiddenRequestError, NotFoundError } from "@app/lib/errors"; +import { ForbiddenRequestError, NotFoundError, UnauthorizedError } from "@app/lib/errors"; import { TAccessTokenQueueServiceFactory } from "../access-token-queue/access-token-queue"; import { ActorType } from "../auth/auth-type"; @@ -168,7 +168,7 @@ export const serviceTokenServiceFactory = ({ } const isMatch = await bcrypt.compare(tokenSecret, serviceToken.secretHash); - if (!isMatch) throw new ForbiddenRequestError(); + if (!isMatch) throw new UnauthorizedError({ message: "Invalid service token" }); await accessTokenQueue.updateServiceTokenStatus(serviceToken.id); return { ...serviceToken, lastUsed: new Date(), orgId: project.orgId };