Skip to content

Commit

Permalink
Merge pull request #2494 from Infisical/daniel/api-errors
Browse files Browse the repository at this point in the history
feat(api): better errors and documentation
  • Loading branch information
DanielHougaard authored Sep 27, 2024
2 parents 02c51b0 + 180d269 commit d430293
Show file tree
Hide file tree
Showing 107 changed files with 979 additions and 854 deletions.
6 changes: 3 additions & 3 deletions backend/e2e-test/routes/v2/service-token.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,7 @@ describe("Service token fail cases", async () => {
authorization: `Bearer ${serviceToken}`
}
});
expect(fetchSecrets.statusCode).toBe(401);
expect(fetchSecrets.statusCode).toBe(403);
expect(fetchSecrets.json().error).toBe("PermissionDenied");
await deleteServiceToken();
});
Expand All @@ -532,7 +532,7 @@ describe("Service token fail cases", async () => {
authorization: `Bearer ${serviceToken}`
}
});
expect(fetchSecrets.statusCode).toBe(401);
expect(fetchSecrets.statusCode).toBe(403);
expect(fetchSecrets.json().error).toBe("PermissionDenied");
await deleteServiceToken();
});
Expand All @@ -557,7 +557,7 @@ describe("Service token fail cases", async () => {
authorization: `Bearer ${serviceToken}`
}
});
expect(writeSecrets.statusCode).toBe(401);
expect(writeSecrets.statusCode).toBe(403);
expect(writeSecrets.json().error).toBe("PermissionDenied");

// but read access should still work fine
Expand Down
6 changes: 3 additions & 3 deletions backend/e2e-test/routes/v3/secrets.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1075,7 +1075,7 @@ describe("Secret V3 Raw Router Without E2EE enabled", async () => {
},
body: createSecretReqBody
});
expect(createSecRes.statusCode).toBe(400);
expect(createSecRes.statusCode).toBe(404);
});

test("Update secret raw", async () => {
Expand All @@ -1093,7 +1093,7 @@ describe("Secret V3 Raw Router Without E2EE enabled", async () => {
},
body: updateSecretReqBody
});
expect(updateSecRes.statusCode).toBe(400);
expect(updateSecRes.statusCode).toBe(404);
});

test("Delete secret raw", async () => {
Expand All @@ -1110,6 +1110,6 @@ describe("Secret V3 Raw Router Without E2EE enabled", async () => {
},
body: deletedSecretReqBody
});
expect(deletedSecRes.statusCode).toBe(400);
expect(deletedSecRes.statusCode).toBe(404);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { z } from "zod";

import { IdentityProjectAdditionalPrivilegeTemporaryMode } from "@app/ee/services/identity-project-additional-privilege/identity-project-additional-privilege-types";
import { IDENTITY_ADDITIONAL_PRIVILEGE } from "@app/lib/api-docs";
import { BadRequestError } from "@app/lib/errors";
import { UnauthorizedError } from "@app/lib/errors";
import { alphaNumericNanoId } from "@app/lib/nanoid";
import { readLimit, writeLimit } from "@app/server/config/rateLimiter";
import { verifyAuth } from "@app/server/plugins/auth/verify-auth";
Expand Down Expand Up @@ -61,7 +61,7 @@ export const registerIdentityProjectAdditionalPrivilegeRouter = async (server: F
handler: async (req) => {
const { permissions, privilegePermission } = req.body;
if (!permissions && !privilegePermission) {
throw new BadRequestError({ message: "Permission or privilegePermission must be provided" });
throw new UnauthorizedError({ message: "Permission or privilegePermission must be provided" });
}

const permission = privilegePermission
Expand Down Expand Up @@ -140,7 +140,7 @@ export const registerIdentityProjectAdditionalPrivilegeRouter = async (server: F
handler: async (req) => {
const { permissions, privilegePermission } = req.body;
if (!permissions && !privilegePermission) {
throw new BadRequestError({ message: "Permission or privilegePermission must be provided" });
throw new UnauthorizedError({ message: "Permission or privilegePermission must be provided" });
}

const permission = privilegePermission
Expand Down Expand Up @@ -224,7 +224,7 @@ export const registerIdentityProjectAdditionalPrivilegeRouter = async (server: F
handler: async (req) => {
const { permissions, privilegePermission, ...updatedInfo } = req.body.privilegeDetails;
if (!permissions && !privilegePermission) {
throw new BadRequestError({ message: "Permission or privilegePermission must be provided" });
throw new UnauthorizedError({ message: "Permission or privilegePermission must be provided" });
}

const permission = privilegePermission
Expand Down
4 changes: 2 additions & 2 deletions backend/src/ee/routes/v1/rate-limit-router.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { z } from "zod";

import { RateLimitSchema } from "@app/db/schemas";
import { BadRequestError } from "@app/lib/errors";
import { NotFoundError } from "@app/lib/errors";
import { readLimit } from "@app/server/config/rateLimiter";
import { verifySuperAdmin } from "@app/server/plugins/auth/superAdmin";
import { verifyAuth } from "@app/server/plugins/auth/verify-auth";
Expand Down Expand Up @@ -29,7 +29,7 @@ export const registerRateLimitRouter = async (server: FastifyZodProvider) => {
handler: async () => {
const rateLimit = await server.services.rateLimit.getRateLimits();
if (!rateLimit) {
throw new BadRequestError({
throw new NotFoundError({
name: "Get Rate Limit Error",
message: "Rate limit configuration does not exist."
});
Expand Down
2 changes: 1 addition & 1 deletion backend/src/ee/routes/v1/saml-router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export const registerSamlRouter = async (server: FastifyZodProvider) => {
id: samlConfigId
};
} else {
throw new BadRequestError({ message: "Missing sso identitier or org slug" });
throw new BadRequestError({ message: "Missing sso identifier or org slug" });
}

const ssoConfig = await server.services.saml.getSaml(ssoLookupDetails);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { ForbiddenError, subject } from "@casl/ability";

import { BadRequestError } from "@app/lib/errors";
import { BadRequestError, ForbiddenRequestError } from "@app/lib/errors";
import { ActorType } from "@app/services/auth/auth-type";

import { ProjectPermissionActions, ProjectPermissionSub } from "../permission/project-permission";
import { TVerifyApprovers } from "./access-approval-policy-types";
import { TVerifyApprovers, VerifyApproversError } from "./access-approval-policy-types";

export const verifyApprovers = async ({
userIds,
Expand All @@ -13,7 +13,8 @@ export const verifyApprovers = async ({
envSlug,
actorAuthMethod,
secretPath,
permissionService
permissionService,
error
}: TVerifyApprovers) => {
for await (const userId of userIds) {
try {
Expand All @@ -30,7 +31,16 @@ export const verifyApprovers = async ({
subject(ProjectPermissionSub.Secrets, { environment: envSlug, secretPath })
);
} catch (err) {
throw new BadRequestError({ message: "One or more approvers doesn't have access to be specified secret path" });
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"
});
}
}
}
};
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ import {
TGetAccessApprovalPolicyByIdDTO,
TGetAccessPolicyCountByEnvironmentDTO,
TListAccessApprovalPoliciesDTO,
TUpdateAccessApprovalPolicy
TUpdateAccessApprovalPolicy,
VerifyApproversError
} from "./access-approval-policy-types";

type TSecretApprovalPolicyServiceFactoryDep = {
Expand Down Expand Up @@ -58,7 +59,7 @@ export const accessApprovalPolicyServiceFactory = ({
enforcementLevel
}: TCreateAccessApprovalPolicy) => {
const project = await projectDAL.findProjectBySlug(projectSlug, actorOrgId);
if (!project) throw new BadRequestError({ message: "Project not found" });
if (!project) throw new NotFoundError({ message: "Project not found" });

// If there is a group approver people might be added to the group later to meet the approvers quota
const groupApprovers = approvers
Expand Down Expand Up @@ -89,7 +90,7 @@ export const accessApprovalPolicyServiceFactory = ({
ProjectPermissionSub.SecretApproval
);
const env = await projectEnvDAL.findOne({ slug: environment, projectId: project.id });
if (!env) throw new BadRequestError({ message: "Environment not found" });
if (!env) throw new NotFoundError({ message: "Environment not found" });

let approverUserIds = userApprovers;
if (userApproverNames.length) {
Expand Down Expand Up @@ -139,7 +140,8 @@ export const accessApprovalPolicyServiceFactory = ({
secretPath,
actorAuthMethod,
permissionService,
userIds: verifyAllApprovers
userIds: verifyAllApprovers,
error: VerifyApproversError.BadRequestError
});

const accessApproval = await accessApprovalPolicyDAL.transaction(async (tx) => {
Expand Down Expand Up @@ -186,7 +188,7 @@ export const accessApprovalPolicyServiceFactory = ({
projectSlug
}: TListAccessApprovalPoliciesDTO) => {
const project = await projectDAL.findProjectBySlug(projectSlug, actorOrgId);
if (!project) throw new BadRequestError({ message: "Project not found" });
if (!project) throw new NotFoundError({ message: "Project not found" });

// Anyone in the project should be able to get the policies.
/* const { permission } = */ await permissionService.getProjectPermission(
Expand Down Expand Up @@ -237,7 +239,7 @@ export const accessApprovalPolicyServiceFactory = ({
throw new BadRequestError({ message: "Approvals cannot be greater than approvers" });
}

if (!accessApprovalPolicy) throw new BadRequestError({ message: "Secret approval policy not found" });
if (!accessApprovalPolicy) throw new NotFoundError({ message: "Secret approval policy not found" });
const { permission } = await permissionService.getProjectPermission(
actor,
actorId,
Expand Down Expand Up @@ -290,7 +292,8 @@ export const accessApprovalPolicyServiceFactory = ({
secretPath: doc.secretPath!,
actorAuthMethod,
permissionService,
userIds: userApproverIds
userIds: userApproverIds,
error: VerifyApproversError.BadRequestError
});

await accessApprovalPolicyApproverDAL.insertMany(
Expand Down Expand Up @@ -329,7 +332,8 @@ export const accessApprovalPolicyServiceFactory = ({
secretPath: doc.secretPath!,
actorAuthMethod,
permissionService,
userIds: verifyGroupApprovers
userIds: verifyGroupApprovers,
error: VerifyApproversError.BadRequestError
});
await accessApprovalPolicyApproverDAL.insertMany(
groupApprovers.map((groupId) => ({
Expand Down Expand Up @@ -357,7 +361,7 @@ export const accessApprovalPolicyServiceFactory = ({
actorOrgId
}: TDeleteAccessApprovalPolicy) => {
const policy = await accessApprovalPolicyDAL.findById(policyId);
if (!policy) throw new BadRequestError({ message: "Secret approval policy not found" });
if (!policy) throw new NotFoundError({ message: "Secret approval policy not found" });

const { permission } = await permissionService.getProjectPermission(
actor,
Expand Down Expand Up @@ -385,7 +389,7 @@ export const accessApprovalPolicyServiceFactory = ({
}: TGetAccessPolicyCountByEnvironmentDTO) => {
const project = await projectDAL.findProjectBySlug(projectSlug, actorOrgId);

if (!project) throw new BadRequestError({ message: "Project not found" });
if (!project) throw new NotFoundError({ message: "Project not found" });

const { membership } = await permissionService.getProjectPermission(
actor,
Expand All @@ -394,13 +398,13 @@ export const accessApprovalPolicyServiceFactory = ({
actorAuthMethod,
actorOrgId
);
if (!membership) throw new BadRequestError({ message: "User not found in project" });
if (!membership) throw new NotFoundError({ message: "User not found in project" });

const environment = await projectEnvDAL.findOne({ projectId: project.id, slug: envSlug });
if (!environment) throw new BadRequestError({ message: "Environment not found" });
if (!environment) throw new NotFoundError({ message: "Environment not found" });

const policies = await accessApprovalPolicyDAL.find({ envId: environment.id, projectId: project.id });
if (!policies) throw new BadRequestError({ message: "No policies found" });
if (!policies) throw new NotFoundError({ message: "No policies found" });

return { count: policies.length };
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@ import { ActorAuthMethod } from "@app/services/auth/auth-type";

import { TPermissionServiceFactory } from "../permission/permission-service";

export enum VerifyApproversError {
ForbiddenError = "ForbiddenError",
BadRequestError = "BadRequestError"
}

export type TVerifyApprovers = {
userIds: string[];
permissionService: Pick<TPermissionServiceFactory, "getProjectPermission">;
Expand All @@ -11,6 +16,7 @@ export type TVerifyApprovers = {
secretPath: string;
projectId: string;
orgId: string;
error: VerifyApproversError;
};

export enum ApproverType {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { PackRule, unpackRules } from "@casl/ability/extra";

import { UnauthorizedError } from "@app/lib/errors";
import { BadRequestError } from "@app/lib/errors";

import { TVerifyPermission } from "./access-approval-request-types";

Expand All @@ -19,7 +19,7 @@ export const verifyRequestedPermissions = ({ permissions }: TVerifyPermission) =
);

if (!permission || !permission.length) {
throw new UnauthorizedError({ message: "No permission provided" });
throw new BadRequestError({ message: "No permission provided" });
}

const requestedPermissions: string[] = [];
Expand All @@ -39,10 +39,10 @@ export const verifyRequestedPermissions = ({ permissions }: TVerifyPermission) =
const permissionEnv = firstPermission.conditions?.environment;

if (!permissionEnv || typeof permissionEnv !== "string") {
throw new UnauthorizedError({ message: "Permission environment is not a string" });
throw new BadRequestError({ message: "Permission environment is not a string" });
}
if (!permissionSecretPath || typeof permissionSecretPath !== "string") {
throw new UnauthorizedError({ message: "Permission path is not a string" });
throw new BadRequestError({ message: "Permission path is not a string" });
}

return {
Expand Down
Loading

0 comments on commit d430293

Please sign in to comment.