Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: ensure consistent handling of "not found" errors #1520

Merged
merged 2 commits into from
Nov 27, 2024
+142 −129
Merged

feat: ensure consistent handling of "not found" errors #1520

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b35efb8
fix: ensure consistent handling of "not found" errors
Junjiequan Nov 25, 2024
da84a99
fix: fix failing api test
Junjiequan Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
234 changes: 117 additions & 117 deletions src/datasets/datasets.controller.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
ConflictException,
Logger,
InternalServerErrorException,
NotFoundException,
} from "@nestjs/common";
import { Request } from "express";
import { ProposalsService } from "./proposals.service";
Expand Down Expand Up @@ -160,7 +161,7 @@
Logger.error("Permission for the action is not specified");
return false;
}
} catch (error) {

Check warning on line 164 in src/proposals/proposals.controller.ts

View workflow job for this annotation

GitHub Actions / eslint 

'error' is defined but never used
throw new InternalServerErrorException();
}
}
Expand All @@ -174,6 +175,10 @@
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
BadRequestException,
Req,
Header,
NotFoundException,
} from "@nestjs/common";
import { SamplesService } from "./samples.service";
import { CreateSampleDto } from "./dto/create-sample.dto";
Expand Down Expand Up @@ -153,7 +154,7 @@
Logger.error("Permission for the action is not specified");
return false;
}
} catch (error) {

Check warning on line 157 in src/samples/samples.controller.ts

View workflow job for this annotation

GitHub Actions / eslint 

'error' is defined but never used
throw new InternalServerErrorException();
}
}
Expand All @@ -167,6 +168,10 @@
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
Open in desktop
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
Open in desktop
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
Loading