From 73a4bca2843393a1d2ec455ffada08de52ec709a Mon Sep 17 00:00:00 2001 From: junjiequan Date: Mon, 25 Nov 2024 14:11:15 +0100 Subject: [PATCH] fix: ensure consistent handling of "not found" errors --- src/datasets/datasets.controller.ts | 234 +++++++++--------- .../origdatablocks.controller.ts | 6 +- src/proposals/proposals.controller.ts | 5 + src/samples/samples.controller.ts | 5 + 4 files changed, 130 insertions(+), 120 deletions(-) diff --git a/src/datasets/datasets.controller.ts b/src/datasets/datasets.controller.ts index 3c7123ea0..688e02bf5 100644 --- a/src/datasets/datasets.controller.ts +++ b/src/datasets/datasets.controller.ts @@ -207,82 +207,84 @@ export class DatasetsController { const dataset = await this.datasetsService.findOne({ where: { pid: id } }); const user: JWTUser = request.user as JWTUser; - if (dataset) { - const datasetInstance = - await this.generateDatasetInstanceForPermissions(dataset); - - const ability = this.caslAbilityFactory.datasetInstanceAccess(user); - - let canDoAction = false; - - if (group == Action.DatasetRead) { - canDoAction = - ability.can(Action.DatasetReadAny, DatasetClass) || - ability.can(Action.DatasetReadOneOwner, datasetInstance) || - ability.can(Action.DatasetReadOneAccess, datasetInstance) || - ability.can(Action.DatasetReadOnePublic, datasetInstance); - } else if (group == Action.DatasetAttachmentRead) { - canDoAction = - ability.can(Action.DatasetAttachmentReadAny, DatasetClass) || - ability.can(Action.DatasetAttachmentReadOwner, datasetInstance) || - ability.can(Action.DatasetAttachmentReadAccess, datasetInstance) || - ability.can(Action.DatasetAttachmentReadPublic, datasetInstance); - } else if (group == Action.DatasetAttachmentCreate) { - canDoAction = - ability.can(Action.DatasetAttachmentCreateAny, DatasetClass) || - ability.can(Action.DatasetAttachmentCreateOwner, datasetInstance); - } else if (group == Action.DatasetAttachmentUpdate) { - canDoAction = - ability.can(Action.DatasetAttachmentUpdateAny, DatasetClass) || - ability.can(Action.DatasetAttachmentUpdateOwner, datasetInstance); - } else if (group == Action.DatasetAttachmentDelete) { - canDoAction = - ability.can(Action.DatasetAttachmentDeleteAny, DatasetClass) || - ability.can(Action.DatasetAttachmentDeleteOwner, datasetInstance); - } else if (group == Action.DatasetOrigdatablockRead) { - canDoAction = - ability.can(Action.DatasetOrigdatablockReadAny, DatasetClass) || - ability.can(Action.DatasetOrigdatablockReadOwner, datasetInstance) || - ability.can(Action.DatasetOrigdatablockReadAccess, datasetInstance) || - ability.can(Action.DatasetOrigdatablockReadPublic, datasetInstance); - } else if (group == Action.DatasetOrigdatablockCreate) { - canDoAction = - ability.can(Action.DatasetOrigdatablockCreateAny, DatasetClass) || - ability.can(Action.DatasetOrigdatablockCreateOwner, datasetInstance); - } else if (group == Action.DatasetOrigdatablockUpdate) { - canDoAction = - ability.can(Action.DatasetOrigdatablockUpdateAny, DatasetClass) || - ability.can(Action.DatasetOrigdatablockUpdateOwner, datasetInstance); - } else if (group == Action.DatasetOrigdatablockDelete) { - canDoAction = - ability.can(Action.DatasetOrigdatablockDeleteAny, DatasetClass) || - ability.can(Action.DatasetOrigdatablockDeleteOwner, datasetInstance); - } else if (group == Action.DatasetDatablockRead) { - canDoAction = - ability.can(Action.DatasetOrigdatablockReadAny, DatasetClass) || - ability.can(Action.DatasetDatablockReadOwner, datasetInstance) || - ability.can(Action.DatasetDatablockReadAccess, datasetInstance) || - ability.can(Action.DatasetDatablockReadPublic, datasetInstance); - } else if (group == Action.DatasetDatablockCreate) { - canDoAction = - ability.can(Action.DatasetDatablockCreateAny, DatasetClass) || - ability.can(Action.DatasetDatablockCreateOwner, datasetInstance); - } else if (group == Action.DatasetDatablockUpdate) { - canDoAction = - ability.can(Action.DatasetDatablockUpdateAny, DatasetClass) || - ability.can(Action.DatasetDatablockUpdateOwner, datasetInstance); - } else if (group == Action.DatasetDatablockDelete) { - canDoAction = - ability.can(Action.DatasetDatablockDeleteAny, DatasetClass) || - ability.can(Action.DatasetDatablockDeleteOwner, datasetInstance); - } else if (group == Action.DatasetLogbookRead) { - canDoAction = - ability.can(Action.DatasetLogbookReadAny, DatasetClass) || - ability.can(Action.DatasetLogbookReadOwner, datasetInstance); - } - if (!canDoAction) { - throw new ForbiddenException("Unauthorized access"); - } + if (!dataset) { + throw new NotFoundException(`dataset: ${id} not found`); + } + + const datasetInstance = + await this.generateDatasetInstanceForPermissions(dataset); + + const ability = this.caslAbilityFactory.datasetInstanceAccess(user); + + let canDoAction = false; + + if (group == Action.DatasetRead) { + canDoAction = + ability.can(Action.DatasetReadAny, DatasetClass) || + ability.can(Action.DatasetReadOneOwner, datasetInstance) || + ability.can(Action.DatasetReadOneAccess, datasetInstance) || + ability.can(Action.DatasetReadOnePublic, datasetInstance); + } else if (group == Action.DatasetAttachmentRead) { + canDoAction = + ability.can(Action.DatasetAttachmentReadAny, DatasetClass) || + ability.can(Action.DatasetAttachmentReadOwner, datasetInstance) || + ability.can(Action.DatasetAttachmentReadAccess, datasetInstance) || + ability.can(Action.DatasetAttachmentReadPublic, datasetInstance); + } else if (group == Action.DatasetAttachmentCreate) { + canDoAction = + ability.can(Action.DatasetAttachmentCreateAny, DatasetClass) || + ability.can(Action.DatasetAttachmentCreateOwner, datasetInstance); + } else if (group == Action.DatasetAttachmentUpdate) { + canDoAction = + ability.can(Action.DatasetAttachmentUpdateAny, DatasetClass) || + ability.can(Action.DatasetAttachmentUpdateOwner, datasetInstance); + } else if (group == Action.DatasetAttachmentDelete) { + canDoAction = + ability.can(Action.DatasetAttachmentDeleteAny, DatasetClass) || + ability.can(Action.DatasetAttachmentDeleteOwner, datasetInstance); + } else if (group == Action.DatasetOrigdatablockRead) { + canDoAction = + ability.can(Action.DatasetOrigdatablockReadAny, DatasetClass) || + ability.can(Action.DatasetOrigdatablockReadOwner, datasetInstance) || + ability.can(Action.DatasetOrigdatablockReadAccess, datasetInstance) || + ability.can(Action.DatasetOrigdatablockReadPublic, datasetInstance); + } else if (group == Action.DatasetOrigdatablockCreate) { + canDoAction = + ability.can(Action.DatasetOrigdatablockCreateAny, DatasetClass) || + ability.can(Action.DatasetOrigdatablockCreateOwner, datasetInstance); + } else if (group == Action.DatasetOrigdatablockUpdate) { + canDoAction = + ability.can(Action.DatasetOrigdatablockUpdateAny, DatasetClass) || + ability.can(Action.DatasetOrigdatablockUpdateOwner, datasetInstance); + } else if (group == Action.DatasetOrigdatablockDelete) { + canDoAction = + ability.can(Action.DatasetOrigdatablockDeleteAny, DatasetClass) || + ability.can(Action.DatasetOrigdatablockDeleteOwner, datasetInstance); + } else if (group == Action.DatasetDatablockRead) { + canDoAction = + ability.can(Action.DatasetOrigdatablockReadAny, DatasetClass) || + ability.can(Action.DatasetDatablockReadOwner, datasetInstance) || + ability.can(Action.DatasetDatablockReadAccess, datasetInstance) || + ability.can(Action.DatasetDatablockReadPublic, datasetInstance); + } else if (group == Action.DatasetDatablockCreate) { + canDoAction = + ability.can(Action.DatasetDatablockCreateAny, DatasetClass) || + ability.can(Action.DatasetDatablockCreateOwner, datasetInstance); + } else if (group == Action.DatasetDatablockUpdate) { + canDoAction = + ability.can(Action.DatasetDatablockUpdateAny, DatasetClass) || + ability.can(Action.DatasetDatablockUpdateOwner, datasetInstance); + } else if (group == Action.DatasetDatablockDelete) { + canDoAction = + ability.can(Action.DatasetDatablockDeleteAny, DatasetClass) || + ability.can(Action.DatasetDatablockDeleteOwner, datasetInstance); + } else if (group == Action.DatasetLogbookRead) { + canDoAction = + ability.can(Action.DatasetLogbookReadAny, DatasetClass) || + ability.can(Action.DatasetLogbookReadOwner, datasetInstance); + } + if (!canDoAction) { + throw new ForbiddenException("Unauthorized access"); } return dataset; @@ -292,20 +294,22 @@ export class DatasetsController { const dataset = await this.datasetsService.findOne({ where: { pid: id } }); const user: JWTUser = request.user as JWTUser; - if (dataset) { - const datasetInstance = - await this.generateDatasetInstanceForPermissions(dataset); + if (!dataset) { + throw new NotFoundException(`dataset: ${id} not found`); + } - const ability = this.caslAbilityFactory.datasetInstanceAccess(user); - const canView = - ability.can(Action.DatasetReadAny, DatasetClass) || - ability.can(Action.DatasetReadOneOwner, datasetInstance) || - ability.can(Action.DatasetReadOneAccess, datasetInstance) || - ability.can(Action.DatasetReadOnePublic, datasetInstance); + const datasetInstance = + await this.generateDatasetInstanceForPermissions(dataset); - if (!canView) { - throw new ForbiddenException("Unauthorized access"); - } + const ability = this.caslAbilityFactory.datasetInstanceAccess(user); + const canView = + ability.can(Action.DatasetReadAny, DatasetClass) || + ability.can(Action.DatasetReadOneOwner, datasetInstance) || + ability.can(Action.DatasetReadOneAccess, datasetInstance) || + ability.can(Action.DatasetReadOnePublic, datasetInstance); + + if (!canView) { + throw new ForbiddenException("Unauthorized access"); } return dataset; @@ -359,38 +363,34 @@ export class DatasetsController { ) { const user: JWTUser = request.user as JWTUser; - if (dataset) { - // NOTE: We need DatasetClass instance because casl module can not recognize the type from dataset mongo database model. If other fields are needed can be added later. - const datasetInstance = - await this.generateDatasetInstanceForPermissions(dataset); - // instantiate the casl matrix for the user - const ability = this.caslAbilityFactory.datasetInstanceAccess(user); - // check if he/she can create this dataset - const canCreate = - ability.can(Action.DatasetCreateAny, DatasetClass) || - ability.can(Action.DatasetCreateOwnerNoPid, datasetInstance) || - ability.can(Action.DatasetCreateOwnerWithPid, datasetInstance); - - if (!canCreate) { - throw new ForbiddenException("Unauthorized to create this dataset"); - } + // NOTE: We need DatasetClass instance because casl module can not recognize the type from dataset mongo database model. If other fields are needed can be added later. + const datasetInstance = + await this.generateDatasetInstanceForPermissions(dataset); + // instantiate the casl matrix for the user + const ability = this.caslAbilityFactory.datasetInstanceAccess(user); + // check if he/she can create this dataset + const canCreate = + ability.can(Action.DatasetCreateAny, DatasetClass) || + ability.can(Action.DatasetCreateOwnerNoPid, datasetInstance) || + ability.can(Action.DatasetCreateOwnerWithPid, datasetInstance); - // now checks if we need to validate the pid - if ( - configuration().datasetCreationValidationEnabled && - configuration().datasetCreationValidationRegex && - dataset.pid - ) { - const re = new RegExp(configuration().datasetCreationValidationRegex); + if (!canCreate) { + throw new ForbiddenException("Unauthorized to create this dataset"); + } - if (!re.test(dataset.pid)) { - throw new BadRequestException( - "PID is not following required standards", - ); - } + // now checks if we need to validate the pid + if ( + configuration().datasetCreationValidationEnabled && + configuration().datasetCreationValidationRegex && + dataset.pid + ) { + const re = new RegExp(configuration().datasetCreationValidationRegex); + + if (!re.test(dataset.pid)) { + throw new BadRequestException( + "PID is not following required standards", + ); } - } else { - throw new ForbiddenException("Unauthorized to create datasets"); } return dataset; diff --git a/src/origdatablocks/origdatablocks.controller.ts b/src/origdatablocks/origdatablocks.controller.ts index 97e21d50b..5dafc179e 100644 --- a/src/origdatablocks/origdatablocks.controller.ts +++ b/src/origdatablocks/origdatablocks.controller.ts @@ -11,9 +11,9 @@ import { Query, HttpCode, HttpStatus, - BadRequestException, Req, ForbiddenException, + NotFoundException, } from "@nestjs/common"; import { Request } from "express"; import { OrigDatablocksService } from "./origdatablocks.service"; @@ -65,7 +65,7 @@ export class OrigDatablocksController { ) { const origDatablock = await this.origDatablocksService.findOne({ _id: id }); if (!origDatablock) { - throw new BadRequestException("Invalid origDatablock Id"); + throw new NotFoundException(`OrigDatablock: ${id} not found`); } await this.checkPermissionsForOrigDatablockExtended( @@ -125,7 +125,7 @@ export class OrigDatablocksController { const user: JWTUser = request.user as JWTUser; if (!dataset) { - throw new BadRequestException(`Invalid datasetId: ${id}`); + throw new NotFoundException(`Dataset: ${id} not found`); } const origDatablockInstance = diff --git a/src/proposals/proposals.controller.ts b/src/proposals/proposals.controller.ts index 3c5ad6456..a39b02656 100644 --- a/src/proposals/proposals.controller.ts +++ b/src/proposals/proposals.controller.ts @@ -16,6 +16,7 @@ import { ConflictException, Logger, InternalServerErrorException, + NotFoundException, } from "@nestjs/common"; import { Request } from "express"; import { ProposalsService } from "./proposals.service"; @@ -174,6 +175,10 @@ export class ProposalsController { proposalId: id, }); + if (!proposal) { + throw new NotFoundException(`Proposal: ${id} not found`); + } + const canDoAction = this.permissionChecker(group, proposal, request); if (!canDoAction) { diff --git a/src/samples/samples.controller.ts b/src/samples/samples.controller.ts index 9679e218c..857b650cf 100644 --- a/src/samples/samples.controller.ts +++ b/src/samples/samples.controller.ts @@ -17,6 +17,7 @@ import { BadRequestException, Req, Header, + NotFoundException, } from "@nestjs/common"; import { SamplesService } from "./samples.service"; import { CreateSampleDto } from "./dto/create-sample.dto"; @@ -167,6 +168,10 @@ export class SamplesController { sampleId: id, }); + if (!sample) { + throw new NotFoundException(`Sample: ${id} not found`); + } + const canDoAction = this.permissionChecker(group, sample, request); if (!canDoAction) {