From b218952495b36d836c155e0ec0e7af9e3baf0832 Mon Sep 17 00:00:00 2001 From: Despina Adamopoulou <16343312+despadam@users.noreply.github.com> Date: Wed, 7 Aug 2024 15:33:19 +0200 Subject: [PATCH] Validate actions during their creation (#1354) * remove validation from actions, store only configVersion inside each job * remove action validation from testing * fix testing * restore configuration in the schema, fix tests * fix linting * fix error message --- .github/workflows/test.yml | 2 +- src/jobs/actions/emailaction.ts | 7 -- src/jobs/actions/logaction.ts | 4 - src/jobs/actions/rabbitmqaction.ts | 3 - src/jobs/actions/urlaction.ts | 19 +++- src/jobs/config/jobconfig.spec.ts | 2 +- src/jobs/config/jobconfig.ts | 5 - .../interceptors/job-create.interceptor.ts | 39 ++++--- src/jobs/jobs.controller.ts | 104 +++++++----------- test/Jobs.js | 19 ++-- 10 files changed, 83 insertions(+), 121 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 5b7067e54..453849894 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -178,5 +178,5 @@ jobs: run: | cp CI/ESS/docker-compose.api.yaml docker-compose.yaml cp functionalAccounts.json.test functionalAccounts.json - docker-compose up --build -d + docker compose up --build -d npm run test:api diff --git a/src/jobs/actions/emailaction.ts b/src/jobs/actions/emailaction.ts index 4c41587ee..f7c242ff9 100644 --- a/src/jobs/actions/emailaction.ts +++ b/src/jobs/actions/emailaction.ts @@ -65,13 +65,6 @@ export class EmailJobAction implements JobAction { this.bodyTemplate = compile(data["body"]); } - async validate(dto: T) { - Logger.log( - "Validating EmailJobAction: " + JSON.stringify(dto), - "EmailJobAction", - ); - } - async performJob(job: JobClass) { Logger.log( "Performing EmailJobAction: " + JSON.stringify(job), diff --git a/src/jobs/actions/logaction.ts b/src/jobs/actions/logaction.ts index 416f9e304..2a7d870da 100644 --- a/src/jobs/actions/logaction.ts +++ b/src/jobs/actions/logaction.ts @@ -14,10 +14,6 @@ export class LogJobAction implements JobAction { return LogJobAction.actionType; } - async validate(dto: T) { - Logger.log("Validating job: " + JSON.stringify(dto), "LogJobAction"); - } - async performJob(job: JobClass) { Logger.log("Performing job: " + JSON.stringify(job), "LogJobAction"); } diff --git a/src/jobs/actions/rabbitmqaction.ts b/src/jobs/actions/rabbitmqaction.ts index cc5703fd7..b7c0cbe81 100644 --- a/src/jobs/actions/rabbitmqaction.ts +++ b/src/jobs/actions/rabbitmqaction.ts @@ -54,9 +54,6 @@ export class RabbitMQJobAction implements JobAction { return RabbitMQJobAction.actionType; } - // eslint-disable-next-line @typescript-eslint/no-unused-vars - async validate(dto: T) {} - async performJob(job: JobClass) { Logger.log( "Performing RabbitMQJobAction: " + JSON.stringify(job), diff --git a/src/jobs/actions/urlaction.ts b/src/jobs/actions/urlaction.ts index 443fb29b6..3216fa4bf 100644 --- a/src/jobs/actions/urlaction.ts +++ b/src/jobs/actions/urlaction.ts @@ -52,12 +52,9 @@ export class URLAction implements JobAction { return URLAction.actionType; } - // eslint-disable-next-line @typescript-eslint/no-unused-vars - async validate(dto: T) {} - async performJob(job: JobClass) { const url = encodeURI(this.urlTemplate(job, jobTemplateOptions)); - Logger.log(`Requesting ${url}`, "URLAction"); + Logger.log(`Requesting ${url}`, "UrlJobAction"); const response = await fetch(url, { method: this.method, @@ -74,7 +71,11 @@ export class URLAction implements JobAction { : undefined, }); - Logger.log(`Request for ${url} returned ${response.status}`, "URLAction"); + Logger.log( + `Request for ${url} returned ${response.status}`, + "UrlJobAction", + ); + if (!response.ok) { throw new HttpException( { @@ -101,13 +102,20 @@ export class URLAction implements JobAction { */ // eslint-disable-next-line @typescript-eslint/no-explicit-any constructor(data: Record) { + Logger.log( + "Initializing UrlJobAction. Params: " + JSON.stringify(data), + "UrlJobAction", + ); + if (!data["url"]) { throw new NotFoundException("Param 'url' is undefined in url action"); } this.urlTemplate = Handlebars.compile(data.url); + if (data["method"]) { this.method = data.method; } + if (data["headers"]) { if (!isStringRecord(data.headers)) { throw new NotFoundException( @@ -121,6 +129,7 @@ export class URLAction implements JobAction { ]), ); } + if (data["body"]) { this.bodyTemplate = Handlebars.compile(data["body"]); } diff --git a/src/jobs/config/jobconfig.spec.ts b/src/jobs/config/jobconfig.spec.ts index 7f7db98b4..630bdab1d 100644 --- a/src/jobs/config/jobconfig.spec.ts +++ b/src/jobs/config/jobconfig.spec.ts @@ -22,6 +22,6 @@ describe("Job configuration", () => { const action = create.actions[0]; expect(action instanceof LogJobAction); expect(action.getActionType()).toBe("log"); - action.validate({ config: null }); + // action.validate({ config: null }); }); }); diff --git a/src/jobs/config/jobconfig.ts b/src/jobs/config/jobconfig.ts index 06658ef53..0d31e1d5d 100644 --- a/src/jobs/config/jobconfig.ts +++ b/src/jobs/config/jobconfig.ts @@ -154,11 +154,6 @@ function parseAction( * Superclass for all responses to Job changes */ export interface JobAction { - /** - * Validate the DTO, throwing an HttpException for problems - */ - validate: (dto: DtoType) => Promise; - /** * Respond to the action */ diff --git a/src/jobs/interceptors/job-create.interceptor.ts b/src/jobs/interceptors/job-create.interceptor.ts index f7e3997fc..cb85aaa1d 100644 --- a/src/jobs/interceptors/job-create.interceptor.ts +++ b/src/jobs/interceptors/job-create.interceptor.ts @@ -58,26 +58,25 @@ export class JobCreateInterceptor implements NestInterceptor { } const jc = matchingConfig[0]; - await Promise.all( - jc.create.actions.map((action) => { - return action.validate(createJobDto).catch((err) => { - Logger.error(err); - if (err instanceof HttpException) { - throw err; - } - - throw new HttpException( - { - status: HttpStatus.BAD_REQUEST, - message: `Invalid job input. Action ${action.getActionType()} unable to validate ${ - createJobDto.type - } job due to ${err}`, - }, - HttpStatus.BAD_REQUEST, - ); - }); - }), - ); + // await Promise.all( + // jc.create.actions.map((action) => { + // return action.validate(createJobDto).catch((err) => { + // Logger.error(err); + // if (err instanceof HttpException) { + // throw err; + // } + // throw new HttpException( + // { + // status: HttpStatus.BAD_REQUEST, + // message: `Invalid job input. Action ${action.getActionType()} unable to validate ${ + // createJobDto.type + // } job due to ${err}`, + // }, + // HttpStatus.BAD_REQUEST, + // ); + // }); + // }), + // ); return jc; } diff --git a/src/jobs/jobs.controller.ts b/src/jobs/jobs.controller.ts index 062231c6f..fb97fbca3 100644 --- a/src/jobs/jobs.controller.ts +++ b/src/jobs/jobs.controller.ts @@ -276,20 +276,6 @@ export class JobsController { return true; }; - /** - * Check that the user is authenticated - */ - checkAuthenticatedUser = (user: JWTUser) => { - if (user === null) { - throw new HttpException( - { - status: HttpStatus.UNAUTHORIZED, - }, - HttpStatus.UNAUTHORIZED, - ); - } - }; - /** * Check that the dataset ids list is valid */ @@ -310,11 +296,6 @@ export class JobsController { HttpStatus.BAD_REQUEST, ); } - interface condition { - where: { - pid: { $in: string[] }; - }; - } if (datasetIds.length == 0) { throw new HttpException( { @@ -325,11 +306,17 @@ export class JobsController { ); } + interface condition { + where: { + pid: { $in: string[] }; + }; + } const filter: condition = { where: { pid: { $in: datasetIds }, }, }; + const findDatasetsById = await this.datasetsService.findAll(filter); const findIds = findDatasetsById.map(({ pid }) => pid); const nonExistIds = datasetIds.filter((x) => !findIds.includes(x)); @@ -337,7 +324,7 @@ export class JobsController { throw new HttpException( { status: HttpStatus.BAD_REQUEST, - message: ` Datasets with pid ${nonExistIds} don't exist.`, + message: `Datasets with pid ${nonExistIds} don't exist.`, }, HttpStatus.BAD_REQUEST, ); @@ -350,10 +337,10 @@ export class JobsController { */ async generateJobInstanceForPermissions(job: JobClass): Promise { const jobInstance = new JobClass(); + jobInstance._id = job._id; jobInstance.id = job.id; jobInstance.type = job.type; - //jobInstance.configuration = configuration().statusUpdateJobGroups; jobInstance.ownerGroup = job.ownerGroup; jobInstance.ownerUser = job.ownerUser; @@ -363,25 +350,23 @@ export class JobsController { /** * Check job type matching configuration */ - getJobMatchingConfiguration = (createJobDtoType: string) => { + getJobTypeConfiguration = (jobType: string) => { const jobConfigs = configuration().jobConfiguration; - const matchingConfig = jobConfigs.filter( - (j) => j.jobType == createJobDtoType, - ); + const matchingConfig = jobConfigs.filter((j) => j.jobType == jobType); if (matchingConfig.length != 1) { if (matchingConfig.length > 1) { Logger.error( - "More than one job configurations matching type " + createJobDtoType, + "More than one job configurations matching type " + jobType, ); } else { - Logger.error("No job configuration matching type " + createJobDtoType); + Logger.error("No job configuration matching type " + jobType); } // return error that job type does not exists throw new HttpException( { status: HttpStatus.BAD_REQUEST, - message: "Invalid job type: " + createJobDtoType, + message: "Invalid job type: " + jobType, }, HttpStatus.BAD_REQUEST, ); @@ -393,15 +378,13 @@ export class JobsController { * Checking if user is allowed to create job according to auth field of job configuration */ async instanceAuthorizationJobCreate( - jobCreateDto: CreateJobDtoWithConfig, + jobCreateDto: CreateJobDto, user: JWTUser, ): Promise { // NOTE: We need JobClass instance because casl module works only on that. // If other fields are needed can be added later. const jobInstance = new JobClass(); - const jobConfiguration = this.getJobMatchingConfiguration( - jobCreateDto.type, - ); + const jobConfiguration = this.getJobTypeConfiguration(jobCreateDto.type); jobInstance._id = ""; jobInstance.accessGroups = []; @@ -537,14 +520,14 @@ export class JobsController { // instantiate the casl matrix for the user const ability = this.caslAbilityFactory.createForUser(user); - // check if he/she can create this dataset + // check if the user can create this job const canCreate = ability.can(AuthOp.JobCreateAny, JobClass) || ability.can(AuthOp.JobCreateOwner, jobInstance) || ability.can(AuthOp.JobCreateConfiguration, jobInstance); if (!canCreate) { - throw new ForbiddenException("Unauthorized to create this dataset."); + throw new ForbiddenException("Unauthorized to create this job."); } return jobInstance; @@ -564,9 +547,8 @@ export class JobsController { throw new HttpException( { status: HttpStatus.BAD_REQUEST, - message: `Invalid job input. Action ${action.getActionType()} unable to validate ${ - jobInstance.type - } job due to ${err}`, + message: `Invalid job input. Job '${jobInstance.type}' unable to perfom + action '${action.getActionType()}' due to ${err}`, }, HttpStatus.BAD_REQUEST, ); @@ -574,7 +556,7 @@ export class JobsController { } async performJobCreateAction(jobInstance: JobClass): Promise { - const jobConfig = this.getJobMatchingConfiguration(jobInstance.type); + const jobConfig = this.getJobTypeConfiguration(jobInstance.type); for (const action of jobConfig.create.actions) { await this.performJobAction(jobInstance, action); } @@ -582,28 +564,18 @@ export class JobsController { } async performJobStatusUpdateAction(jobInstance: JobClass): Promise { - const jobConfig = this.getJobMatchingConfiguration(jobInstance.type); - - await Promise.all( - jobConfig.statusUpdate.actions.map((action) => { - return action.validate(jobInstance).catch((err) => { - Logger.error(err); - if (err instanceof HttpException) { - throw err; - } - - throw new HttpException( - { - status: HttpStatus.BAD_REQUEST, - message: `Invalid job input. Action ${action.getActionType()} unable to validate ${ - jobInstance.type - } job due to ${err}`, - }, - HttpStatus.BAD_REQUEST, - ); - }); - }), - ); + const jobConfig = this.getJobTypeConfiguration(jobInstance.type); + + // TODO - what shall we do when configVersion does not match? + if (jobConfig.configVersion !== jobInstance.configuration.configVersion) { + Logger.log( + ` + Job was created with configVersion ${jobInstance.configuration.configVersion}. + Current configVersion is ${jobConfig.configVersion}. + `, + "JobStatusUpdate", + ); + } for (const action of jobConfig.statusUpdate.actions) { await this.performJobAction(jobInstance, action); @@ -618,7 +590,7 @@ export class JobsController { @CheckPolicies((ability: AppAbility) => ability.can(AuthOp.JobCreate, JobClass), ) - @UseInterceptors(JobCreateInterceptor) + // @UseInterceptors(JobCreateInterceptor) @Post() @ApiOperation({ summary: "It creates a new job.", @@ -636,13 +608,13 @@ export class JobsController { }) async create( @Req() request: Request, - @Body() createJobDtoWithConfig: CreateJobDtoWithConfig, + @Body() createJobDto: CreateJobDto, ): Promise { Logger.log("Creating job!"); // throw an error if no jobParams are passed if ( - !createJobDtoWithConfig.jobParams || - Object.keys(createJobDtoWithConfig.jobParams).length == 0 + !createJobDto.jobParams || + Object.keys(createJobDto.jobParams).length == 0 ) { throw new HttpException( { @@ -655,7 +627,7 @@ export class JobsController { // Validate that request matches the current configuration // Check job authorization const jobInstance = await this.instanceAuthorizationJobCreate( - createJobDtoWithConfig, + createJobDto, request.user as JWTUser, ); // Create actual job in database @@ -706,7 +678,7 @@ export class JobsController { } const currentJobInstance = await this.generateJobInstanceForPermissions(currentJob); - currentJobInstance.configuration = this.getJobMatchingConfiguration( + currentJobInstance.configuration = this.getJobTypeConfiguration( currentJobInstance.type, ); diff --git a/test/Jobs.js b/test/Jobs.js index e9d48638a..08f60dbda 100644 --- a/test/Jobs.js +++ b/test/Jobs.js @@ -340,7 +340,6 @@ describe("1100: Jobs: Test New Job Model", () => { .expect("Content-Type", /json/) .then((res) => { res.body.should.not.have.property("id"); - res.body.should.have.property("message").and.be.equal("Job parameters need to be defined."); }); }); @@ -349,6 +348,8 @@ describe("1100: Jobs: Test New Job Model", () => { type: "all_access", ownerUser: "admin", ownerGroup: "admin", + jobParams: { + }, }; return request(appUrl) @@ -967,7 +968,7 @@ describe("1100: Jobs: Test New Job Model", () => { }); }); - it("0310: Add a new job as a normal user himself/herself in '#datasetPublic' configuration with one unpublished dataset, which should fail ad forbidden", async () => { + it("0310: Add a new job as a normal user himself/herself in '#datasetPublic' configuration with one unpublished dataset, which should fail as forbidden", async () => { const newDataset = { ...jobDatasetPublic, ownerUser: "user5.1", @@ -987,7 +988,7 @@ describe("1100: Jobs: Test New Job Model", () => { .expect("Content-Type", /json/) .then((res) => { res.body.should.not.have.property("id"); - res.body.should.have.property("message").and.be.equal("Unauthorized to create this dataset."); + res.body.should.have.property("message").and.be.equal("Unauthorized to create this job."); }); }); @@ -1031,7 +1032,7 @@ describe("1100: Jobs: Test New Job Model", () => { .expect("Content-Type", /json/) .then((res) => { res.body.should.not.have.property("id"); - res.body.should.have.property("message").and.be.equal("Unauthorized to create this dataset."); + res.body.should.have.property("message").and.be.equal("Unauthorized to create this job."); }); }); @@ -1204,7 +1205,7 @@ describe("1100: Jobs: Test New Job Model", () => { .expect("Content-Type", /json/) .then((res) => { res.body.should.not.have.property("id"); - res.body.should.have.property("message").and.be.equal("Unauthorized to create this dataset."); + res.body.should.have.property("message").and.be.equal("Unauthorized to create this job."); }); }); @@ -1467,7 +1468,7 @@ describe("1100: Jobs: Test New Job Model", () => { .expect("Content-Type", /json/) .then((res) => { res.body.should.not.have.property("id"); - res.body.should.have.property("message").and.be.equal("Unauthorized to create this dataset."); + res.body.should.have.property("message").and.be.equal("Unauthorized to create this job."); }); }); @@ -1733,7 +1734,7 @@ describe("1100: Jobs: Test New Job Model", () => { .expect("Content-Type", /json/) .then((res) => { res.body.should.not.have.property("id"); - res.body.should.have.property("message").and.be.equal("Unauthorized to create this dataset."); + res.body.should.have.property("message").and.be.equal("Unauthorized to create this job."); }); }); @@ -2025,7 +2026,7 @@ describe("1100: Jobs: Test New Job Model", () => { .expect("Content-Type", /json/) .then((res) => { res.body.should.not.have.property("id"); - res.body.should.have.property("message").and.be.equal("Unauthorized to create this dataset."); + res.body.should.have.property("message").and.be.equal("Unauthorized to create this job."); }); }); @@ -2344,7 +2345,7 @@ describe("1100: Jobs: Test New Job Model", () => { .expect("Content-Type", /json/) .then((res) => { res.body.should.not.have.property("id"); - res.body.should.have.property("message").and.be.equal("Unauthorized to create this dataset."); + res.body.should.have.property("message").and.be.equal("Unauthorized to create this job."); }); });