From 0cfe00780f32560c0cfb653111006c51cd7f14ba Mon Sep 17 00:00:00 2001 From: Kevin Bader Date: Mon, 1 Apr 2019 14:57:24 +0200 Subject: [PATCH 1/2] api: describe error handling approach in src/README.md --- api/src/README.md | 160 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 155 insertions(+), 5 deletions(-) diff --git a/api/src/README.md b/api/src/README.md index 2e54f1cdd..dcb88c108 100644 --- a/api/src/README.md +++ b/api/src/README.md @@ -1,10 +1,22 @@ -Dear reader! +# Hacking TruBudget -Welcome to the **TruBudget API Source Directory**. The source code is organized in _layers_. -There are three of them, each with its own _language_ (related to the idea of Ubiquitous Language in Domain-Driven Design): **application**, **service**, and **domain**. The following diagram describes their relationships and also shows some of their vocabulary: +Welcome to the **TruBudget API Source Directory**. This readme file offers an introduction into how the code here is organized. It also mentions some best practices along the way. + +Contents: + +- [Layout](#layout) + - [Overview](#overview) + - [This Directory](#this-directory) +- [Error Handling](#error-handling) + +## Layout + +### Overview + +The source code is organized in _layers_. There are three of them, each with its own _language_ (related to the idea of Ubiquitous Language in Domain-Driven Design): **application**, **service**, and **domain**. The following diagram describes their relationships and also shows some of their vocabulary: ```plain -+--src-----------------------------------+ ++--application---------------------------+ | | | App | | API | @@ -54,6 +66,144 @@ Some more practical rules: - Write unit tests for the domain layer. Write integration tests for the other layers that test the layer itself plus the layers below. For example, it makes little sense to replace business logic by a mock and test that the database works. Of course, there may be exceptions. - Put tests into a `.spec.ts` file next to the code under test. For example, code that tests code in `project_update.ts` should go into `project_update.spec.ts`. -# This Directory +### This Directory This directory defines the **application** context. If you're interested in the general setup, including environment variables and connection setup, take a look at `app.ts`. Most other files are named after the user intent they implement; for example, if a user wants to update a project, the corresponding intent would be `project.update`, with the API implemented in `project_update.ts`. Note that, the intents for creating a project, a subproject and a workflowitem are called `global.createProject`, `project.createSubproject` and `subproject.createWorkflowitem`, respectively. + +## Error Handling + +### Define custom error types + +Generally, errors are defined in the lowest layer they can occur. For example, an event-sourcing error is defined in the domain layer, whereas an HTTP related error is defined in the application layer. + +Errors must subclass `Error` and should contain as many additional properties as required in order to allow a caller to find out what happened. For example, an event-sourcing error must include the event that caused the error. Equally important, errors must set their `name`; this allows an error handler to select the root cause out of a `VError` later on (see below). + +### Using `VError` to add context + +Each caller can either handle the error or pass the error up the call stack. To do the latter, the error should be wrapped using [`VError`](https://github.com/joyent/node-verror/), adding additional context information. + +Example: + +```typescript +// `NotAuthorized` error as defined in the domain layer (simplified version): +class NotAuthorized extends Error { + constructor( + private readonly userId: string, + private readonly intent: Intent, + ) { + super(`user ${userId} is not authorized for ${intent}.`); + + // This allows us to identify this error in a chain of errors later on: + this.name = "NotAuthorized"; + + // Maintains proper stack trace for where our error was thrown: + if (Error.captureStackTrace) { + Error.captureStackTrace(this, NotAuthorized); + } + } +} + +function dropSomeTable(userId: string) { + // do stuff.. + + throw new NotAuthorized(userId, "table.drop"); +} + +function deleteAllData(userId: string) { + // do stuff.. + + try { + dropSomeTable(userId); + } catch (err) { + // `err` says that the table could not be dropped, but what were we dropping the + // table for in the first place? By wrapping `err` with `VError`, we can easily give + // the low-level error additional, high-level context: + throw new VError(err, "failed to delete all data"); + } +} +``` + +When handling a `VError`, we can ask for the root cause by name: + +```typescript +try { + deleteAllData("alice"); +} catch (error) { + // This will log out the error message (which concatenates all `message` fields in the + // error chain), the full stack of all errors and all fields attached to any errors in + // the chain: + logger.debug({ error }, error.message); + + if (VError.hasCauseWithName(error, "NotAuthorized")) { + // handle NotAuthorized error, e.g. by using a specific status code for the response + } else { + // handle other errors.. + } +} +``` + +### Use [`Result`](./result.ts) to return expected errors + +In the previous example, the authorization could be seen as an integral part of the business logic behind `dropSomeTable`. To make that apparent, we typically model that using the [`Result.Type`](./result.ts). + +**Use `Result` whenever a function may fail for non-technical reasons.** + +Technical reasons refer to disk or network failures, and so on. Non-technical reasons are insufficient permissions, missing entities, values that are out-of-range, etc. + +For example, consider the good old divide-by-zero example: + +```typescript +// Without Result, using throw/catch: + +function divide_or_throw(a: number, b: number): number { + if (b === 0) throw new Error("division by zero"); + return a / b; +} + +try { + const res = divide_or_throw(1, 0); +} catch (error) { + console.error(error); +} + +// With Result, you return the error instead of throwing it: + +function divide(a: number, b: number): Result.Type { + return b === 0 ? new Error("division by zero") : a / b; +} + +const res = divide(1, 0); +if (Result.isErr(res)) { + console.error(res); +} +``` + +Note how the function signature makes it very clear what to expect in the second case, while the first signature is quite misleading - it suggests you pass it two numbers and get back a number in return, while in fact it does so for most, but not for _all_ numbers. Also, the try/catch approach makes it easy to catch errors you didn't anticipate and, because of this, didn't mean to catch. + +With that in mind, we can rewrite the previous example: + +```typescript +// Simplified version of the `NotAuthorized` error defined in the domain layer: +class NotAuthorized extends Error { + ... +} + +function dropSomeTable(userId: string): Result.Type { + // do stuff.. + + // Instead of `throw`ing the error, we `return` it here: + return new NotAuthorized(userId, "table.drop"); +} + +function deleteAllData(userId: string): Result.Type { + // do stuff.. + + // The result is either a value (`undefined` in this case) or an Error: + const result = dropSomeTable(userId); + // If it is an Error, we wrap it using `VError` like before: + return Result.mapErr( + result, + err => new VError(err, "failed to delete all data"), + ); +} +``` From 417e06c8df246acf4b58e3337b346c4d9e735be1 Mon Sep 17 00:00:00 2001 From: Kevin Bader Date: Mon, 1 Apr 2019 16:36:49 +0200 Subject: [PATCH 2/2] api: refine NotAuthorized error + return correct http status code --- api/src/http_errors.ts | 5 ++-- api/src/project_view_details.ts | 4 +-- .../service/domain/errors/not_authorized.ts | 24 +++++++++++++----- .../domain/organization/group_create.ts | 5 ++-- .../domain/organization/group_member_add.ts | 5 ++-- .../organization/group_member_remove.ts | 5 ++-- .../domain/organization/user_create.ts | 6 ++--- .../workflow/global_permission_grant.ts | 5 ++-- .../workflow/global_permission_revoke.ts | 5 ++-- .../service/domain/workflow/project_assign.ts | 5 ++-- .../service/domain/workflow/project_close.ts | 5 ++-- .../service/domain/workflow/project_create.ts | 8 ++++-- .../service/domain/workflow/project_get.ts | 9 +++---- .../workflow/project_permission_grant.ts | 5 ++-- .../workflow/project_permission_revoke.ts | 5 ++-- .../project_projected_budget_delete.ts | 5 ++-- .../project_projected_budget_update.ts | 5 ++-- .../service/domain/workflow/project_update.ts | 5 ++-- .../domain/workflow/subproject_assign.ts | 5 ++-- .../domain/workflow/subproject_close.ts | 7 ++++-- .../domain/workflow/subproject_create.ts | 10 ++++---- .../service/domain/workflow/subproject_get.ts | 10 ++++---- .../workflow/subproject_permission_grant.ts | 5 ++-- .../workflow/subproject_permission_revoke.ts | 5 ++-- .../workflow/subproject_permissions_list.ts | 5 ++-- .../subproject_projected_budget_delete.ts | 10 ++++---- .../subproject_projected_budget_update.ts | 10 ++++---- .../domain/workflow/subproject_update.ts | 5 ++-- .../domain/workflow/workflowitem_assign.ts | 10 ++++---- .../domain/workflow/workflowitem_close.ts | 10 ++++---- .../domain/workflow/workflowitem_create.ts | 25 +++++++++++-------- .../domain/workflow/workflowitem_get.ts | 7 ++++-- .../workflow/workflowitem_permission_grant.ts | 10 ++++---- .../workflowitem_permission_revoke.ts | 5 ++-- .../workflow/workflowitem_permissions_list.ts | 5 ++-- .../domain/workflow/workflowitem_update.ts | 14 +++++------ .../domain/workflow/workflowitems_reorder.ts | 16 ++++++------ api/src/service/project_get.ts | 6 ++--- 38 files changed, 169 insertions(+), 127 deletions(-) diff --git a/api/src/http_errors.ts b/api/src/http_errors.ts index 425174904..c8df8038f 100644 --- a/api/src/http_errors.ts +++ b/api/src/http_errors.ts @@ -1,5 +1,6 @@ +import { VError } from "verror"; + import logger from "./lib/logger"; -import { NotAuthorized } from "./service/domain/errors/not_authorized"; import { NotFound } from "./service/domain/errors/not_found"; import { PreconditionError } from "./service/domain/errors/precondition_error"; @@ -26,7 +27,7 @@ function convertError(error: any): { code: number; message: string } { if (error instanceof PreconditionError) { logger.debug({ error }, error.message); return { code: 400, message: error.message }; - } else if (error instanceof NotAuthorized) { + } else if (VError.hasCauseWithName(error, "NotAuthorized")) { logger.debug({ error }, error.message); return { code: 403, message: error.message }; } else if (error instanceof NotFound) { diff --git a/api/src/project_view_details.ts b/api/src/project_view_details.ts index 75b72fc5d..d9bc4c939 100644 --- a/api/src/project_view_details.ts +++ b/api/src/project_view_details.ts @@ -1,4 +1,5 @@ import { FastifyInstance } from "fastify"; +import { VError } from "verror"; import { getAllowedIntents } from "./authz"; import Intent from "./authz/intents"; @@ -202,8 +203,7 @@ export function addHttpHandler(server: FastifyInstance, urlPrefix: string, servi try { const projectResult = await service.getProject(ctx, user, projectId); if (Result.isErr(projectResult)) { - projectResult.message = `project.viewDetails failed: ${projectResult.message}`; - throw projectResult; + throw new VError(projectResult, "project.viewDetails failed"); } const project: Project.Project = projectResult; diff --git a/api/src/service/domain/errors/not_authorized.ts b/api/src/service/domain/errors/not_authorized.ts index 67231887a..847172f16 100644 --- a/api/src/service/domain/errors/not_authorized.ts +++ b/api/src/service/domain/errors/not_authorized.ts @@ -1,23 +1,35 @@ +import { isArray } from "util"; + import Intent from "../../../authz/intents"; import { Ctx } from "../../../lib/ctx"; -import { BusinessEvent } from "../business_event"; export class NotAuthorized extends Error { + private readonly target?: object; + constructor( private readonly ctx: Ctx, private readonly userId: string, - private readonly businessEvent?: BusinessEvent, - private readonly intent?: Intent, + private readonly intent: Intent | Intent[], + target?: object, ) { super( - `User ${userId} is not authorized${ - businessEvent ? ` to apply ${businessEvent.type}` : ` to invoke ${intent}` - }.`, + `user ${userId} is not authorized for ${ + isArray(intent) ? `any of ${intent.join(", ")}` : intent + }`, ); + // This allows us to identify this error in a chain of errors later on: + this.name = "NotAuthorized"; + // Maintains proper stack trace for where our error was thrown (only available on V8): if (Error.captureStackTrace) { Error.captureStackTrace(this, NotAuthorized); } + + // Removing trace events as they're not needed and spam the log output when printed: + if (target !== undefined && (target as any).log !== undefined) { + delete (target as any).log; + } + this.target = target; } } diff --git a/api/src/service/domain/organization/group_create.ts b/api/src/service/domain/organization/group_create.ts index 6addd788d..f8ab7803f 100644 --- a/api/src/service/domain/organization/group_create.ts +++ b/api/src/service/domain/organization/group_create.ts @@ -71,12 +71,13 @@ export async function createGroup( // Check authorization (if not root): if (creatingUser.id !== "root") { + const intent = "global.createGroup"; const permissions = await repository.getGlobalPermissions(); - const isAuthorized = identitiesAuthorizedFor(permissions, "global.createGroup").some(identity => + const isAuthorized = identitiesAuthorizedFor(permissions, intent).some(identity => canAssumeIdentity(creatingUser, identity), ); if (!isAuthorized) { - return { newEvents: [], errors: [new NotAuthorized(ctx, creatingUser.id, createEvent)] }; + return { newEvents: [], errors: [new NotAuthorized(ctx, creatingUser.id, intent)] }; } } diff --git a/api/src/service/domain/organization/group_member_add.ts b/api/src/service/domain/organization/group_member_add.ts index 0e87b2ed3..f688afee9 100644 --- a/api/src/service/domain/organization/group_member_add.ts +++ b/api/src/service/domain/organization/group_member_add.ts @@ -32,10 +32,11 @@ export async function addMember( // Check authorization (if not root): if (issuer.id !== "root") { - if (!Group.permits(group, issuer, ["group.addUser"])) { + const intent = "group.addUser"; + if (!Group.permits(group, issuer, [intent])) { return { newEvents: [], - errors: [new NotAuthorized(ctx, issuer.id, memberAdded)], + errors: [new NotAuthorized(ctx, issuer.id, intent, group)], }; } } diff --git a/api/src/service/domain/organization/group_member_remove.ts b/api/src/service/domain/organization/group_member_remove.ts index 42f9d22a4..1b121c8a3 100644 --- a/api/src/service/domain/organization/group_member_remove.ts +++ b/api/src/service/domain/organization/group_member_remove.ts @@ -32,10 +32,11 @@ export async function removeMember( // Check authorization (if not root): if (issuer.id !== "root") { - if (!Group.permits(group, issuer, ["group.removeUser"])) { + const intent = "group.removeUser"; + if (!Group.permits(group, issuer, [intent])) { return { newEvents: [], - errors: [new NotAuthorized(ctx, issuer.id, memberRemoved)], + errors: [new NotAuthorized(ctx, issuer.id, intent, group)], }; } } diff --git a/api/src/service/domain/organization/user_create.ts b/api/src/service/domain/organization/user_create.ts index 87b474ce1..3995ab97f 100644 --- a/api/src/service/domain/organization/user_create.ts +++ b/api/src/service/domain/organization/user_create.ts @@ -77,15 +77,15 @@ export async function createUser( // Check authorization (if not root): if (creatingUser.id !== "root") { + const intent = "global.createUser"; const permissions = await repository.getGlobalPermissions(); - const isAuthorized = identitiesAuthorizedFor(permissions, "global.createUser").some(identity => + const isAuthorized = identitiesAuthorizedFor(permissions, intent).some(identity => canAssumeIdentity(creatingUser, identity), ); if (!isAuthorized) { - const unfinishedBusinessEvent = UserCreated.createEvent(source, publisher, eventTemplate); return { newEvents: [], - errors: [new NotAuthorized(ctx, creatingUser.id, unfinishedBusinessEvent)], + errors: [new NotAuthorized(ctx, creatingUser.id, intent)], }; } } diff --git a/api/src/service/domain/workflow/global_permission_grant.ts b/api/src/service/domain/workflow/global_permission_grant.ts index 51174198e..b5f55d390 100644 --- a/api/src/service/domain/workflow/global_permission_grant.ts +++ b/api/src/service/domain/workflow/global_permission_grant.ts @@ -30,11 +30,12 @@ export async function grantGlobalPermission( // Check authorization (if not root): if (issuer.id !== "root") { + const grantIntent = "global.grantPermission"; const currentGlobalPermissions = await repository.getGlobalPermissions(); - if (!GlobalPermissions.permits(currentGlobalPermissions, issuer, ["global.grantPermission"])) { + if (!GlobalPermissions.permits(currentGlobalPermissions, issuer, [grantIntent])) { return { newEvents: [], - errors: [new NotAuthorized(ctx, issuer.id, globalPermissionGranted)], + errors: [new NotAuthorized(ctx, issuer.id, grantIntent, currentGlobalPermissions)], }; } } diff --git a/api/src/service/domain/workflow/global_permission_revoke.ts b/api/src/service/domain/workflow/global_permission_revoke.ts index 34702fb94..2070322e9 100644 --- a/api/src/service/domain/workflow/global_permission_revoke.ts +++ b/api/src/service/domain/workflow/global_permission_revoke.ts @@ -30,11 +30,12 @@ export async function revokeGlobalPermission( // Check authorization (if not root): if (issuer.id !== "root") { + const revokeIntent = "global.revokePermission"; const currentGlobalPermissions = await repository.getGlobalPermissions(); - if (!GlobalPermissions.permits(currentGlobalPermissions, issuer, ["global.revokePermission"])) { + if (!GlobalPermissions.permits(currentGlobalPermissions, issuer, [revokeIntent])) { return { newEvents: [], - errors: [new NotAuthorized(ctx, issuer.id, globalPermissionRevoked)], + errors: [new NotAuthorized(ctx, issuer.id, revokeIntent, currentGlobalPermissions)], }; } } diff --git a/api/src/service/domain/workflow/project_assign.ts b/api/src/service/domain/workflow/project_assign.ts index 28588747d..d61f64a0c 100644 --- a/api/src/service/domain/workflow/project_assign.ts +++ b/api/src/service/domain/workflow/project_assign.ts @@ -42,8 +42,9 @@ export async function assignProject( } // Check authorization (if not root): - if (issuer.id !== "root" && !Project.permits(project, issuer, ["project.assign"])) { - return new NotAuthorized(ctx, issuer.id, projectAssigned); + const intent = "project.assign"; + if (issuer.id !== "root" && !Project.permits(project, issuer, [intent])) { + return new NotAuthorized(ctx, issuer.id, intent, project); } // Check that the new event is indeed valid: diff --git a/api/src/service/domain/workflow/project_close.ts b/api/src/service/domain/workflow/project_close.ts index 823d244af..bbd77fa1e 100644 --- a/api/src/service/domain/workflow/project_close.ts +++ b/api/src/service/domain/workflow/project_close.ts @@ -58,8 +58,9 @@ export async function closeProject( } // Check authorization (if not root): - if (issuer.id !== "root" && !Project.permits(project, issuer, ["project.close"])) { - return new NotAuthorized(ctx, issuer.id, projectClosed); + const intent = "project.close"; + if (issuer.id !== "root" && !Project.permits(project, issuer, [intent])) { + return new NotAuthorized(ctx, issuer.id, intent, project); } // Check that the new event is indeed valid: diff --git a/api/src/service/domain/workflow/project_create.ts b/api/src/service/domain/workflow/project_create.ts index 9e7358bab..54bc744c9 100644 --- a/api/src/service/domain/workflow/project_create.ts +++ b/api/src/service/domain/workflow/project_create.ts @@ -96,9 +96,13 @@ export async function createProject( // Check authorization (if not root): if (creatingUser.id !== "root") { + const intent = "global.createProject"; const permissions = await repository.getGlobalPermissions(); - if (!GlobalPermissions.permits(permissions, creatingUser, ["global.createProject"])) { - return { newEvents: [], errors: [new NotAuthorized(ctx, creatingUser.id, createEvent)] }; + if (!GlobalPermissions.permits(permissions, creatingUser, [intent])) { + return { + newEvents: [], + errors: [new NotAuthorized(ctx, creatingUser.id, intent, permissions)], + }; } } diff --git a/api/src/service/domain/workflow/project_get.ts b/api/src/service/domain/workflow/project_get.ts index 2c389f3e0..d4c85d535 100644 --- a/api/src/service/domain/workflow/project_get.ts +++ b/api/src/service/domain/workflow/project_get.ts @@ -1,4 +1,3 @@ -import { VError } from "verror"; import Intent from "../../../authz/intents"; import { Ctx } from "../../../lib/ctx"; import * as Result from "../../../result"; @@ -25,11 +24,9 @@ export async function getProject( return new NotFound(ctx, "project", projectId); } - if ( - user.id !== "root" && - !Project.permits(project, user, ["project.viewSummary", "project.viewDetails"]) - ) { - return new NotAuthorized(ctx, user.id, undefined, "project.viewSummary"); + const intents: Intent[] = ["project.viewSummary", "project.viewDetails"]; + if (user.id !== "root" && !Project.permits(project, user, intents)) { + return new NotAuthorized(ctx, user.id, intents, project); } return dropHiddenHistoryEvents(project, user); diff --git a/api/src/service/domain/workflow/project_permission_grant.ts b/api/src/service/domain/workflow/project_permission_grant.ts index bfd60618c..3a0a65885 100644 --- a/api/src/service/domain/workflow/project_permission_grant.ts +++ b/api/src/service/domain/workflow/project_permission_grant.ts @@ -42,8 +42,9 @@ export async function grantProjectPermission( // Check authorization (if not root): if (issuer.id !== "root") { - if (!Project.permits(project, issuer, ["project.intent.grantPermission"])) { - return new NotAuthorized(ctx, issuer.id, permissionGranted); + const grantIntent = "project.intent.grantPermission"; + if (!Project.permits(project, issuer, [grantIntent])) { + return new NotAuthorized(ctx, issuer.id, grantIntent, project); } } diff --git a/api/src/service/domain/workflow/project_permission_revoke.ts b/api/src/service/domain/workflow/project_permission_revoke.ts index f692270a1..10a910114 100644 --- a/api/src/service/domain/workflow/project_permission_revoke.ts +++ b/api/src/service/domain/workflow/project_permission_revoke.ts @@ -42,8 +42,9 @@ export async function revokeProjectPermission( // Check authorization (if not root): if (issuer.id !== "root") { - if (!Project.permits(project, issuer, ["project.intent.revokePermission"])) { - return new NotAuthorized(ctx, issuer.id, permissionRevoked); + const revokeIntent = "project.intent.revokePermission"; + if (!Project.permits(project, issuer, [revokeIntent])) { + return new NotAuthorized(ctx, issuer.id, revokeIntent, project); } } diff --git a/api/src/service/domain/workflow/project_projected_budget_delete.ts b/api/src/service/domain/workflow/project_projected_budget_delete.ts index c00636afe..41b9e38d9 100644 --- a/api/src/service/domain/workflow/project_projected_budget_delete.ts +++ b/api/src/service/domain/workflow/project_projected_budget_delete.ts @@ -40,8 +40,9 @@ export async function deleteProjectedBudget( // Check authorization (if not root): if (issuer.id !== "root") { - if (!Project.permits(project, issuer, ["project.budget.deleteProjected"])) { - return new NotAuthorized(ctx, issuer.id, budgetDeleted); + const intent = "project.budget.deleteProjected"; + if (!Project.permits(project, issuer, [intent])) { + return new NotAuthorized(ctx, issuer.id, intent, project); } } diff --git a/api/src/service/domain/workflow/project_projected_budget_update.ts b/api/src/service/domain/workflow/project_projected_budget_update.ts index d3f438a3a..ce9dbb561 100644 --- a/api/src/service/domain/workflow/project_projected_budget_update.ts +++ b/api/src/service/domain/workflow/project_projected_budget_update.ts @@ -43,8 +43,9 @@ export async function updateProjectedBudget( // Check authorization (if not root): if (issuer.id !== "root") { - if (!Project.permits(project, issuer, ["project.budget.updateProjected"])) { - return new NotAuthorized(ctx, issuer.id, budgetUpdated); + const intent = "project.budget.updateProjected"; + if (!Project.permits(project, issuer, [intent])) { + return new NotAuthorized(ctx, issuer.id, intent, project); } } diff --git a/api/src/service/domain/workflow/project_update.ts b/api/src/service/domain/workflow/project_update.ts index e38afa775..a662db1e1 100644 --- a/api/src/service/domain/workflow/project_update.ts +++ b/api/src/service/domain/workflow/project_update.ts @@ -45,8 +45,9 @@ export async function updateProject( // Check authorization (if not root): if (issuer.id !== "root") { - if (!Project.permits(project, issuer, ["project.update"])) { - return new NotAuthorized(ctx, issuer.id, projectUpdated); + const intent = "project.update"; + if (!Project.permits(project, issuer, [intent])) { + return new NotAuthorized(ctx, issuer.id, intent, project); } } diff --git a/api/src/service/domain/workflow/subproject_assign.ts b/api/src/service/domain/workflow/subproject_assign.ts index 6ad8f04b0..34730306b 100644 --- a/api/src/service/domain/workflow/subproject_assign.ts +++ b/api/src/service/domain/workflow/subproject_assign.ts @@ -51,8 +51,9 @@ export async function assignSubproject( // Check authorization (if not root): if (issuer.id !== "root") { - if (!Subproject.permits(subproject, issuer, ["subproject.assign"])) { - return new NotAuthorized(ctx, issuer.id, subprojectAssigned); + const intent = "subproject.assign"; + if (!Subproject.permits(subproject, issuer, [intent])) { + return new NotAuthorized(ctx, issuer.id, intent, subproject); } } diff --git a/api/src/service/domain/workflow/subproject_close.ts b/api/src/service/domain/workflow/subproject_close.ts index f8a2b7540..de4465604 100644 --- a/api/src/service/domain/workflow/subproject_close.ts +++ b/api/src/service/domain/workflow/subproject_close.ts @@ -74,8 +74,11 @@ export async function closeSubproject( } // Check authorization (if not root): - if (issuer.id !== "root" && !Subproject.permits(subproject, issuer, ["subproject.close"])) { - return new NotAuthorized(ctx, issuer.id, subprojectClosed); + if (issuer.id !== "root") { + const intent = "subproject.close"; + if (!Subproject.permits(subproject, issuer, [intent])) { + return new NotAuthorized(ctx, issuer.id, intent, subproject); + } } // Check that the new event is indeed valid: diff --git a/api/src/service/domain/workflow/subproject_create.ts b/api/src/service/domain/workflow/subproject_create.ts index fc3443af2..10182fcd7 100644 --- a/api/src/service/domain/workflow/subproject_create.ts +++ b/api/src/service/domain/workflow/subproject_create.ts @@ -111,11 +111,11 @@ export async function createSubproject( return new NotFound(ctx, "project", projectId); } - if ( - creatingUser.id !== "root" && - !AuthToken.permits(projectPermissions, creatingUser, ["project.createSubproject"]) - ) { - return new NotAuthorized(ctx, creatingUser.id, subprojectCreated); + if (creatingUser.id !== "root") { + const intent = "project.createSubproject"; + if (!AuthToken.permits(projectPermissions, creatingUser, [intent])) { + return new NotAuthorized(ctx, creatingUser.id, intent, { projectId, projectPermissions }); + } } // Check that the event is valid by trying to "apply" it: diff --git a/api/src/service/domain/workflow/subproject_get.ts b/api/src/service/domain/workflow/subproject_get.ts index f8c3a85c4..ca83de7f7 100644 --- a/api/src/service/domain/workflow/subproject_get.ts +++ b/api/src/service/domain/workflow/subproject_get.ts @@ -25,11 +25,11 @@ export async function getSubproject( } const subproject = subprojectResult; - if ( - user.id !== "root" && - !Subproject.permits(subproject, user, ["subproject.viewSummary", "subproject.viewDetails"]) - ) { - return new NotAuthorized(ctx, user.id, undefined, "subproject.viewDetails"); + if (user.id !== "root") { + const intents: Intent[] = ["subproject.viewSummary", "subproject.viewDetails"]; + if (!Subproject.permits(subproject, user, intents)) { + return new NotAuthorized(ctx, user.id, intents, subproject); + } } return dropHiddenHistoryEvents(subproject, user); diff --git a/api/src/service/domain/workflow/subproject_permission_grant.ts b/api/src/service/domain/workflow/subproject_permission_grant.ts index 016fae6ec..90f6fff57 100644 --- a/api/src/service/domain/workflow/subproject_permission_grant.ts +++ b/api/src/service/domain/workflow/subproject_permission_grant.ts @@ -48,8 +48,9 @@ export async function grantSubprojectPermission( // Check authorization (if not root): if (issuer.id !== "root") { - if (!Subproject.permits(subproject, issuer, ["subproject.intent.grantPermission"])) { - return new NotAuthorized(ctx, issuer.id, permissionGranted); + const grantIntent = "subproject.intent.grantPermission"; + if (!Subproject.permits(subproject, issuer, [grantIntent])) { + return new NotAuthorized(ctx, issuer.id, grantIntent, subproject); } } diff --git a/api/src/service/domain/workflow/subproject_permission_revoke.ts b/api/src/service/domain/workflow/subproject_permission_revoke.ts index 7186278ef..2699f4f6a 100644 --- a/api/src/service/domain/workflow/subproject_permission_revoke.ts +++ b/api/src/service/domain/workflow/subproject_permission_revoke.ts @@ -48,8 +48,9 @@ export async function revokeSubprojectPermission( // Check authorization (if not root): if (issuer.id !== "root") { - if (!Subproject.permits(subproject, issuer, ["subproject.intent.revokePermission"])) { - return new NotAuthorized(ctx, issuer.id, permissionRevoked); + const revokeIntent = "subproject.intent.revokePermission"; + if (!Subproject.permits(subproject, issuer, [revokeIntent])) { + return new NotAuthorized(ctx, issuer.id, revokeIntent, subproject); } } diff --git a/api/src/service/domain/workflow/subproject_permissions_list.ts b/api/src/service/domain/workflow/subproject_permissions_list.ts index 5633984a7..c8e210699 100644 --- a/api/src/service/domain/workflow/subproject_permissions_list.ts +++ b/api/src/service/domain/workflow/subproject_permissions_list.ts @@ -30,8 +30,9 @@ export async function getSubprojectPermissions( const subproject: Subproject.Subproject = subprojectResult; if (user.id !== "root") { - if (!Subproject.permits(subproject, user, ["subproject.intent.listPermissions"])) { - return new NotAuthorized(ctx, user.id, undefined, "subproject.intent.listPermissions"); + const intent = "subproject.intent.listPermissions"; + if (!Subproject.permits(subproject, user, [intent])) { + return new NotAuthorized(ctx, user.id, intent, subproject); } } return subproject.permissions; diff --git a/api/src/service/domain/workflow/subproject_projected_budget_delete.ts b/api/src/service/domain/workflow/subproject_projected_budget_delete.ts index dc39824e2..340c166d1 100644 --- a/api/src/service/domain/workflow/subproject_projected_budget_delete.ts +++ b/api/src/service/domain/workflow/subproject_projected_budget_delete.ts @@ -43,11 +43,11 @@ export async function deleteProjectedBudget( ); // Check authorization (if not root): - if ( - issuer.id !== "root" && - !Subproject.permits(subproject, issuer, ["subproject.budget.deleteProjected"]) - ) { - return new NotAuthorized(ctx, issuer.id, budgetDeleted); + if (issuer.id !== "root") { + const intent = "subproject.budget.deleteProjected"; + if (!Subproject.permits(subproject, issuer, [intent])) { + return new NotAuthorized(ctx, issuer.id, intent, subproject); + } } // Check that the new event is indeed valid: diff --git a/api/src/service/domain/workflow/subproject_projected_budget_update.ts b/api/src/service/domain/workflow/subproject_projected_budget_update.ts index dbcd4f45a..113bfcea8 100644 --- a/api/src/service/domain/workflow/subproject_projected_budget_update.ts +++ b/api/src/service/domain/workflow/subproject_projected_budget_update.ts @@ -43,11 +43,11 @@ export async function updateProjectedBudget( ); // Check authorization (if not root): - if ( - issuer.id !== "root" && - !Subproject.permits(subproject, issuer, ["subproject.budget.updateProjected"]) - ) { - return new NotAuthorized(ctx, issuer.id, budgetUpdated); + if (issuer.id !== "root") { + const intent = "subproject.budget.updateProjected"; + if (!Subproject.permits(subproject, issuer, [intent])) { + return new NotAuthorized(ctx, issuer.id, intent, subproject); + } } // Check that the new event is indeed valid: diff --git a/api/src/service/domain/workflow/subproject_update.ts b/api/src/service/domain/workflow/subproject_update.ts index d05fcaf3b..43be109aa 100644 --- a/api/src/service/domain/workflow/subproject_update.ts +++ b/api/src/service/domain/workflow/subproject_update.ts @@ -60,8 +60,9 @@ export async function updateSubproject( // Check authorization (if not root): if (issuer.id !== "root") { - if (!Subproject.permits(subproject, issuer, ["subproject.update"])) { - return new NotAuthorized(ctx, issuer.id, subprojectUpdated); + const intent = "subproject.update"; + if (!Subproject.permits(subproject, issuer, [intent])) { + return new NotAuthorized(ctx, issuer.id, intent, subproject); } } diff --git a/api/src/service/domain/workflow/workflowitem_assign.ts b/api/src/service/domain/workflow/workflowitem_assign.ts index 8207595ff..b9be8d237 100644 --- a/api/src/service/domain/workflow/workflowitem_assign.ts +++ b/api/src/service/domain/workflow/workflowitem_assign.ts @@ -55,11 +55,11 @@ export async function assignWorkflowitem( } // Check authorization: - if ( - publisher.id !== "root" && - !Workflowitem.permits(workflowitem, publisher, ["workflowitem.assign"]) - ) { - return new NotAuthorized(ctx, publisher.id, assignEvent); + if (publisher.id !== "root") { + const intent = "workflowitem.assign"; + if (!Workflowitem.permits(workflowitem, publisher, [intent])) { + return new NotAuthorized(ctx, publisher.id, intent, workflowitem); + } } // Check that the new event is indeed valid: diff --git a/api/src/service/domain/workflow/workflowitem_close.ts b/api/src/service/domain/workflow/workflowitem_close.ts index 80dec63e1..70afabb30 100644 --- a/api/src/service/domain/workflow/workflowitem_close.ts +++ b/api/src/service/domain/workflow/workflowitem_close.ts @@ -74,11 +74,11 @@ export async function closeWorkflowitem( return new VError(closeEvent, "failed to create event"); } - if ( - closingUser.id !== "root" && - !Workflowitem.permits(workflowitemToClose, closingUser, ["workflowitem.close"]) - ) { - return new NotAuthorized(ctx, closingUser.id, closeEvent); + if (closingUser.id !== "root") { + const intent = "workflowitem.close"; + if (!Workflowitem.permits(workflowitemToClose, closingUser, [intent])) { + return new NotAuthorized(ctx, closingUser.id, intent, workflowitemToClose); + } } // Make sure all previous items (wrt. the ordering) are already closed: diff --git a/api/src/service/domain/workflow/workflowitem_create.ts b/api/src/service/domain/workflow/workflowitem_create.ts index aec4cce2a..5f078d00d 100644 --- a/api/src/service/domain/workflow/workflowitem_create.ts +++ b/api/src/service/domain/workflow/workflowitem_create.ts @@ -1,15 +1,14 @@ import Joi = require("joi"); +import { VError } from "verror"; import Intent from "../../../authz/intents"; import { Ctx } from "../../../lib/ctx"; -import logger from "../../../lib/logger"; import * as Result from "../../../result"; import { randomString } from "../../hash"; import * as AdditionalData from "../additional_data"; import { BusinessEvent } from "../business_event"; import { InvalidCommand } from "../errors/invalid_command"; import { NotAuthorized } from "../errors/not_authorized"; -import { NotFound } from "../errors/not_found"; import { PreconditionError } from "../errors/precondition_error"; import { ServiceUser } from "../organization/service_user"; import { Permissions } from "../permissions"; @@ -118,16 +117,20 @@ export async function createWorkflowitem( // Check authorization (if not root): if (creatingUser.id !== "root") { - const subprojectResult = await repository.getSubproject( - reqData.projectId, - reqData.subprojectId, + const authorizationResult = Result.map( + await repository.getSubproject(reqData.projectId, reqData.subprojectId), + subproject => { + const intent = "subproject.createWorkflowitem"; + if (!Subproject.permits(subproject, creatingUser, [intent])) { + return new NotAuthorized(ctx, creatingUser.id, intent, subproject); + } + }, ); - if (Result.isErr(subprojectResult)) { - return new NotFound(ctx, "subproject", reqData.subprojectId); - } - const subproject = subprojectResult; - if (!Subproject.permits(subproject, creatingUser, ["subproject.createWorkflowitem"])) { - return new NotAuthorized(ctx, creatingUser.id, workflowitemCreated); + if (Result.isErr(authorizationResult)) { + return new VError( + authorizationResult, + "failed to create workflowitem, permission check failed", + ); } } diff --git a/api/src/service/domain/workflow/workflowitem_get.ts b/api/src/service/domain/workflow/workflowitem_get.ts index f5f8c63b3..a12d8a226 100644 --- a/api/src/service/domain/workflow/workflowitem_get.ts +++ b/api/src/service/domain/workflow/workflowitem_get.ts @@ -23,8 +23,11 @@ export async function getWorkflowitem( return new NotFound(ctx, "workflowitem", workflowitemId); } - if (user.id !== "root" && !Workflowitem.permits(workflowitem, user, ["workflowitem.view"])) { - return new NotAuthorized(ctx, user.id, undefined, "workflowitem.view"); + if (user.id !== "root") { + const intent = "workflowitem.view"; + if (!Workflowitem.permits(workflowitem, user, [intent])) { + return new NotAuthorized(ctx, user.id, intent, workflowitem); + } } return dropHiddenHistoryEvents(workflowitem, user); diff --git a/api/src/service/domain/workflow/workflowitem_permission_grant.ts b/api/src/service/domain/workflow/workflowitem_permission_grant.ts index 70c523f05..9647605c7 100644 --- a/api/src/service/domain/workflow/workflowitem_permission_grant.ts +++ b/api/src/service/domain/workflow/workflowitem_permission_grant.ts @@ -49,11 +49,11 @@ export async function grantWorkflowitemPermission( grantee, ); - if ( - issuer.id !== "root" && - !Workflowitem.permits(workflowitem, issuer, ["workflowitem.intent.grantPermission"]) - ) { - return new NotAuthorized(ctx, issuer.id, permissionGranted); + if (issuer.id !== "root") { + const grantIntent = "workflowitem.intent.grantPermission"; + if (!Workflowitem.permits(workflowitem, issuer, [grantIntent])) { + return new NotAuthorized(ctx, issuer.id, grantIntent, workflowitem); + } } // Check that the new event is indeed valid: diff --git a/api/src/service/domain/workflow/workflowitem_permission_revoke.ts b/api/src/service/domain/workflow/workflowitem_permission_revoke.ts index a787ed95d..4eeeae368 100644 --- a/api/src/service/domain/workflow/workflowitem_permission_revoke.ts +++ b/api/src/service/domain/workflow/workflowitem_permission_revoke.ts @@ -52,8 +52,9 @@ export async function revokeWorkflowitemPermission( // Check authorization (if not root): if (issuer.id !== "root") { - if (!Workflowitem.permits(workflowitem, issuer, ["workflowitem.intent.revokePermission"])) { - return new NotAuthorized(ctx, issuer.id, permissionRevoked); + const revokeIntent = "workflowitem.intent.revokePermission"; + if (!Workflowitem.permits(workflowitem, issuer, [revokeIntent])) { + return new NotAuthorized(ctx, issuer.id, revokeIntent, workflowitem); } } diff --git a/api/src/service/domain/workflow/workflowitem_permissions_list.ts b/api/src/service/domain/workflow/workflowitem_permissions_list.ts index 7729ac658..3e5125ed1 100644 --- a/api/src/service/domain/workflow/workflowitem_permissions_list.ts +++ b/api/src/service/domain/workflow/workflowitem_permissions_list.ts @@ -32,8 +32,9 @@ export async function getAll( const workflowitem: Workflowitem.Workflowitem = result; if (user.id !== "root") { - if (!Workflowitem.permits(workflowitem, user, ["workflowitem.intent.listPermissions"])) { - return new NotAuthorized(ctx, user.id, undefined, "workflowitem.intent.listPermissions"); + const intent = "workflowitem.intent.listPermissions"; + if (!Workflowitem.permits(workflowitem, user, [intent])) { + return new NotAuthorized(ctx, user.id, intent, workflowitem); } } diff --git a/api/src/service/domain/workflow/workflowitem_update.ts b/api/src/service/domain/workflow/workflowitem_update.ts index c0fc617ad..93f9fbf55 100644 --- a/api/src/service/domain/workflow/workflowitem_update.ts +++ b/api/src/service/domain/workflow/workflowitem_update.ts @@ -1,5 +1,7 @@ import { produce } from "immer"; +import isEqual = require("lodash.isequal"); import { VError } from "verror"; + import { Ctx } from "../../../lib/ctx"; import * as Result from "../../../result"; import { BusinessEvent } from "../business_event"; @@ -14,8 +16,6 @@ import * as Project from "./project"; import * as Subproject from "./subproject"; import * as Workflowitem from "./workflowitem"; import * as WorkflowitemUpdated from "./workflowitem_updated"; -import Joi = require("joi"); -import isEqual = require("lodash.isequal"); export type RequestData = WorkflowitemUpdated.Modification; export const requestDataSchema = WorkflowitemUpdated.modificationSchema; @@ -53,11 +53,11 @@ export async function updateWorkflowitem( } // Check authorization (if not root): - if ( - issuer.id !== "root" && - !Workflowitem.permits(workflowitem, issuer, ["workflowitem.update"]) - ) { - return new NotAuthorized(ctx, issuer.id, newEvent); + if (issuer.id !== "root") { + const intent = "workflowitem.update"; + if (!Workflowitem.permits(workflowitem, issuer, [intent])) { + return new NotAuthorized(ctx, issuer.id, intent, workflowitem); + } } // Check that the new event is indeed valid: diff --git a/api/src/service/domain/workflow/workflowitems_reorder.ts b/api/src/service/domain/workflow/workflowitems_reorder.ts index a03e65cae..3ff91d852 100644 --- a/api/src/service/domain/workflow/workflowitems_reorder.ts +++ b/api/src/service/domain/workflow/workflowitems_reorder.ts @@ -1,4 +1,6 @@ import { produce } from "immer"; +import isEqual = require("lodash.isequal"); + import { Ctx } from "../../../lib/ctx"; import * as Result from "../../../result"; import { BusinessEvent } from "../business_event"; @@ -8,10 +10,8 @@ import { NotFound } from "../errors/not_found"; import { ServiceUser } from "../organization/service_user"; import * as Project from "./project"; import * as Subproject from "./subproject"; -import * as WorkflowitemsReordered from "./workflowitems_reordered"; import * as WorkflowitemOrdering from "./workflowitem_ordering"; - -import isEqual = require("lodash.isequal"); +import * as WorkflowitemsReordered from "./workflowitems_reordered"; interface Repository { getSubproject( @@ -50,11 +50,11 @@ export async function setWorkflowitemOrdering( ); // Check authorization (if not root): - if ( - issuer.id !== "root" && - !Subproject.permits(subproject, issuer, ["subproject.reorderWorkflowitems"]) - ) { - return new NotAuthorized(ctx, issuer.id, reorderEvent); + if (issuer.id !== "root") { + const intent = "subproject.reorderWorkflowitems"; + if (!Subproject.permits(subproject, issuer, [intent])) { + return new NotAuthorized(ctx, issuer.id, intent, subproject); + } } // Check that the new event is indeed valid: diff --git a/api/src/service/project_get.ts b/api/src/service/project_get.ts index e9cac73be..f391fd482 100644 --- a/api/src/service/project_get.ts +++ b/api/src/service/project_get.ts @@ -16,13 +16,13 @@ export async function getProject( ): Promise> { const projectResult = await Cache.withCache(conn, ctx, async cache => ProjectGet.getProject(ctx, serviceUser, projectId, { - getProject: async projectId => { - return cache.getProject(projectId); + getProject: async pId => { + return cache.getProject(pId); }, }), ); return Result.mapErr( projectResult, - err => new VError(err, `could not read project ${projectId} from chain`), + err => new VError(err, `could not fetch project ${projectId}`), ); }