Skip to content

Commit

Permalink
feat: ensure consistent handling of "not found" errors (#1520)
Browse files Browse the repository at this point in the history 
* fix: ensure consistent handling of "not found" errors

* fix: fix failing api test
  • Loading branch information
@Junjiequan
Junjiequan authored Nov 27, 2024
1 parent cc592f1 commit 0355fdc
Show file tree
Hide file tree
Showing 8 changed files with 142 additions and 129 deletions.
234 changes: 117 additions & 117 deletions src/datasets/datasets.controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
8 changes: 5 additions & 3 deletions src/origdatablocks/origdatablocks.controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ import {
Query,
HttpCode,
HttpStatus,
BadRequestException,
Req,
ForbiddenException,
NotFoundException,
} from "@nestjs/common";
import { Request } from "express";
import { OrigDatablocksService } from "./origdatablocks.service";
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -125,7 +125,9 @@ 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 for attaching an origdatablock`,
);
}

const origDatablockInstance =
Expand Down
5 changes: 5 additions & 0 deletions src/proposals/proposals.controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
ConflictException,
Logger,
InternalServerErrorException,
NotFoundException,
} from "@nestjs/common";
import { Request } from "express";
import { ProposalsService } from "./proposals.service";
Expand Down Expand Up @@ -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) {
Expand Down
5 changes: 5 additions & 0 deletions src/samples/samples.controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
BadRequestException,
Req,
Header,
NotFoundException,
} from "@nestjs/common";
import { SamplesService } from "./samples.service";
import { CreateSampleDto } from "./dto/create-sample.dto";
Expand Down Expand Up @@ -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) {
Expand Down
8 changes: 4 additions & 4 deletions test/DerivedDatasetOrigDatablock.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ describe("0800: DerivedDatasetOrigDatablock: Test OrigDatablocks and their relat
db.collection("Dataset").deleteMany({});
db.collection("OrigDatablock").deleteMany({});
});
beforeEach(async() => {
beforeEach(async () => {
accessTokenAdminIngestor = await utils.getToken(appUrl, {
username: "adminIngestor",
password: TestData.Accounts["adminIngestor"]["password"],
Expand Down Expand Up @@ -384,20 +384,20 @@ describe("0800: DerivedDatasetOrigDatablock: Test OrigDatablocks and their relat
});
});

it("0200: add a new origDatablock with invalid pid should fail", async () => {
it("0200: add a new origDatablock to the non-existent dataset should fail", async () => {
return request(appUrl)
.post(`/api/v3/origdatablocks`)
.send({ ...TestData.OrigDataBlockCorrect1, datasetId: "wrong" })
.set("Accept", "application/json")
.set({ Authorization: `Bearer ${accessTokenAdminIngestor}` })
.expect(TestData.BadRequestStatusCode)
.expect(TestData.NotFoundStatusCode)
.expect("Content-Type", /json/)
.then((res) => {
res.body.should.have.property("error");
});
});

it("0210: add a new origDatablock with valid pid should success", async () => {
it("0210: add a new origDatablock to the existent dataset should success", async () => {
return request(appUrl)
.post(`/api/v3/origdatablocks`)
.send({
Expand Down
4 changes: 2 additions & 2 deletions test/OrigDatablockForRawDataset.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -745,13 +745,13 @@ describe("1200: OrigDatablockForRawDataset: Test OrigDatablocks and their relati
});
});

it("0400: add a new origDatablock with invalid pid should fail", async () => {
it("0400: add a new origDatablock to the non-existent dataset should fail", async () => {
return request(appUrl)
.post(`/api/v3/origdatablocks`)
.send({ ...origDatablockData1, datasetId: "wrong" })
.set("Accept", "application/json")
.set({ Authorization: `Bearer ${accessTokenAdminIngestor}` })
.expect(TestData.BadRequestStatusCode)
.expect(TestData.NotFoundStatusCode)
.expect("Content-Type", /json/)
.then((res) => {
res.body.should.have.property("error");
Expand Down
Loading

0 comments on commit 0355fdc

Please sign in to comment.