Jobs tests #1286

sofyalaski · 2024-06-26T12:54:16Z

Description

Implements API tests for jobs authorization endpoints and proposes fixes for missing cases

Motivation

Fixes are proposed to follow the authorization logic described in the documentation

Fixes:

jobs.controller.ts

use CreateJobAuth enum instead of AuthOp to define jobDatasetAuthorization
in instanceAuthorizationJobCreate shift an "if-block" checking the presence of ownerGroup down after the admin check was passed (reason: admin should be able to create a job for anonym)
in the same function, at the very beginning when initializing values of the JobClass instance add jobParams and leave ownerUser and ownerGroup undefined (for anonymous case)*
in the same function, check if the current authorization is in jobDatasetAuthorization (types mismatching)
in the same function, add a check if an unauthorized user creates a job for some other group (unauthorized users should only create jobs for themselves)
in the same function, for "dataset"-kind of job configurations, the "where" filtering wasn't grabbing the respective dataset values properly (maybe go back to Despina's implementation)
in the checkDatasetIds function add a check if each requested datasetId exists in database ( for the "dataset" -kind of job configurations)
implement generateJobInstanceForPermissions function, which is similar to the one in datasets.controller. mongoose findOne and findAll do not return an instance of JobClass as implemented but a simple JS Object. Since we still need to pass that object to casl( which expects the instance of JobClass) we need conversion. To not write all properties of JobClass, a minimal copy with only properties required for permission checking is created. The return instances are not affected by this and remain JS Object.
functionality of instanceAuthorizationJobStatusUpdate is now implemented in the patch endpoint function. This is done to avoid redefining a job return by findOne as a instance of JobClass with all properties.
for both GET endpoints only the endpoint authorisation was implemented. To add the instance authorization too, first create JobClass instance of the found job Objects (as described in two above items) and pass them to respective casl expressions.
adds implementation of delete endpoints. The rules are defined in the CheckPolicies decorator

jobs.service.ts

add a function to extract the username of a user sending a request ( should return "anonymous" if request sent by unauthenticated user). This is used both in create andstatusUpdate functions

casl-ability.factory.ts

for statusUpdate endpoint the user from update_job_groups had less rights that a normal user -> fixed that
add instance authorization for statusUpdate for anonymous user
implement delete endpoint authorization
fixed a negation typo

Changes

add deleteJobGroups to configuration.ts

Tests included

Included for each change/fix?
Passing? (Merge will not be approved unless this is checked)

* reset dataset before every api test * Clear collections before each api test

1. Use CreateJobAuth enum instead of AuthOp to define jobDatasetAuthorization 2. Shift a block if groupOwner is present down after the admin check was passed (admin create a job for anonym) 3. Check if current authorization is in jobDatasetAuthorization in instanceAuthorizationJobCreate (types mismatching) 4. Add a check if unauthorized user creates a job for someone else

if anonymous user creates a job, initialize a jwt instance for him/her

string array includes string enum

1. comment out jobConfig matchin, as now in interceptor 2. fix filtering object hierarchy in #dataset kind of jobConfig's 3. not defining jobInstance ownerUser/Group as empty string 4. add jobInstance.jobParams initialization 5. add a check if datasets passed in the ID list exist

* Anonymous user can patch status update to a job in #all config. Added instance authorization * Implement JobDelete authorization

* similarly to Datasets, mongoose returns an Object and not JobClass instances. To pass this into CASL we first need to make those class instances. generateJobInstanceForPermissions implements a copy of object as correct class instance only with properties required in casl. * functionality of instanceAuthorizationJobStatusUpdate is now implemented in the patch endpoint function. This is done to avoid redefining a found job as a instance of JobClass with all properties * For both GET endpoints only the endpoint authorisation was implemented for user in the functions of the controller. To add the instance authorization too, first create JobClass instance of the found job Objects and pass them to respective casl expressions. * adds implementation of delete endpoints. The rules are defined in the CheckPolicies decorator.

despadam

Reviewed jobs.controller

src/jobs/jobs.controller.ts

sofyalaski · 2024-07-01T16:35:03Z

@despadam I made some changes you requested. For other comments, we can discuss tomorrow in person

merge chanhes after conversation on how configuration will be stored

…e, st it can be used for all cases where we need to extract the configuration, without having to pass the full dto

sofyalaski · 2024-07-14T19:44:21Z

@Despina, I've updated the change to configuration grabbing for statusUpdate as we discussed

- Run lint:fix - Ignore no-explicit-any rule for jobParams (which are not type checked) - remove 'read' jobOperation (not implemented)

test/config/pretest.js

Co-authored-by: Jay <[email protected]>

- Revert previous changes to ignore no-explicit-any - Use `unknown` for jobParam inputs plus stronger runtime checks - Fix compilation error in elastic-search service relating to deleting non-optional variables. - Remove rabbitmqaction validation. RabbitMQ should ignore the DTO body, and validate the config itself in the constructor.

sbliven

This looks pretty good. I added a couple minor comments but it looks good.

Can you verify that mocha passes before merging? @despadam and I both ran into issues running the tests. Mocha should be added to the CI following this PR.

src/jobs/config/jobconfig.ts

sbliven · 2024-07-23T20:03:55Z

src/jobs/actions/urlaction.ts

  private body: Record<string, any> | null = null;

  getActionType(): string {
    return URLAction.actionType;
  }

+  // eslint-disable-next-line @typescript-eslint/no-unused-vars


Weird that this empty method causes a lint warning; it overrides the interface method.

sbliven · 2024-07-23T20:10:26Z

src/jobs/jobs.controller.ts

+          pid: { $eq: datasetIds[i] },
+        },
+      };
+      const idCount = await this.datasetsService.count(filter);


Wouldn't it be more performant to make a single database request here, rather than one request per datasetId?

Yes, you're right, I changed that in the last commit

sbliven · 2024-07-23T20:11:05Z

src/jobs/jobs.controller.ts

+        throw new HttpException(
+          {
+            status: HttpStatus.BAD_REQUEST,
+            message: "At least one dataset in the ids list doesn't exist.",


Should include datasetIds[i] in the error message to make debugging easier.

addressed in the last commit

src/jobs/jobs.controller.ts

sbliven · 2024-07-23T20:14:53Z

src/jobs/jobs.controller.ts

@@ -758,6 +866,7 @@ export class JobsController {
    @Req() request: Request,
    @Param("id") id: string,
  ): Promise<unknown> {
+    Logger.log("Deleting job!");


Should include the id in the log message

addressed in the last commit

sbliven · 2024-07-23T20:21:50Z

test/Jobs.js

+                                password: TestData.Accounts["archiveManager"]["password"],
+                              },
+                              (tokenVal) => {
+                                accessTokenArchiveManager = tokenVal;


That's a lot of nesting! Not necessary for this PR, but maybe getToken needs to be refactored to return a promise rather than use callbacks.

This one is addressed in PR #1350

sbliven · 2024-07-23T20:23:42Z

test/Jobs.js

+  //       res.body.should.not.have.property("id");
+  //       res.body.should.have.propert("message").and.be.equal("Dataset ids list was not provided in jobParams");
+  //     });
+  // });


Why commented?

yes, I was confused by the logic myself a bit. I changed this one into 0065. Now these tests at the beginning cover:

jobParams is passed with property datasetIds, but the array is empty --> fail (0040)

jobParams is passed with property datasetIds, but it refers to datasetIds that are not in database --> fail (0050)

jobParams is not passed at all --> fail (0060)

jobParams is passed as empty object --> fail (0065)

I additionally implemented the check for jobParams property in the create endpoint. It checks that jobParams is passed and is not empty. Before that if no jobParams were passed a 500 internal error was thrown. Now the error message is explicitly saying that jobParams need to be defined and not empty.

the last two tests are new. 0060 before was testing with an empty object as jobParams, which passed before. Now, as I I changed the implementation a little, this test fails. I think it is logical that if a job has no context it shouldn't be accepted? This test now is numbered as 0065.
0060 tests the case where no jobParams passed at all.

despadam · 2024-07-28T15:09:56Z

src/jobs/actions/rabbitmqaction.ts

-    }
-  }
+  // eslint-disable-next-line @typescript-eslint/no-unused-vars
+  async validate(dto: T) {}


@sbliven I struggle to understand the need for validate() in the action class. I was always wondering whether it makes sense validating after calling the constructor, and your changes here I think prove my point that it probably doesn't. I would suggest following this example and moving all validation inside the constructor of each action. Then:

On job create if creating an action fails, the job creation should also fail.

On job statusUpdate things get a bit more complicated. Since we are using a global job configuration, instead of job specific, if no changes were made to the configuration then no validation is needed at all and we can proceed directly to performing the job. But, if the configuration has changed, we would need a method that would first update the values of the class' attributes, before performing the action.

After discussing with @sofyalaski, I will create a separate PR to address this.

0060 now checks if a job was passed without a required parameter JobParams 0065 (new) checks if a job was passed with an empty object for jobParams 1950 now tries to delete a job that doesn't exist and fails 1960 was called 1950 before counts on GET are changed (because 0060 before was passing)

* now makes one request to the db to find datasets and extracts their pids. * loggs the names of all dataset pids that are not in the db in case of an error * job endpoint implements the check that the jobParams were passed and are not empty, otherwise it will throw a bad request with respective message * job endpoint loggs the id of job to be deleted and implements a check if job exist in db, otherwise throws an error.

sofyalaski · 2024-07-29T14:31:28Z

@sbliven in the commit two weeks ago I added running the tests on PR to the "relase-*" branches, and you can see the bars here at the bottom, I assume. For me, the API tests seem to be passing (API test include both test:api:jest and test:api:mocha. It might be because of some differences in the way we deploy then? We tried running the brand new docker container and running tests in it with Despina and it worked. Also, sorry for not telling that you need to start the app first!

sofyalaski and others added 29 commits May 8, 2024 08:55

first tests

fbc463b

feat: reset dataset before every api test (SciCatProject#1131)

0592631

* reset dataset before every api test * Clear collections before each api test

Fixes after jobs testing:

0d586fc

if anonymous user creates a job, initialize a jwt instance for him/her

Fixes after testing:

54ee6a6

string array includes string enum

dataset(s)Validation redundancy after a merged PR

f871c62

test Jobs authorization wip

8b8f470

fix filtering syntax

ea76968

minor changes

39f12b4

fix findOne filtering syntax

2716245

add Jobs tests

186ac28

remove redundancy

5edb913

fix numbering

c6fd75b

extend UPDATE_JOB_GROUP user's right to match docs

75f41d6

remove unneccessary arguments

83a2743

make function return value of findOne an instance of JobClass

7b80f14

add statusUpdate tests

52a791b

add jobParams to the job instance

af31a2a

Patch tests

c7dd474

merge changes from release-jobs

6630770

Fixes after tests:

ea61e32

* Anonymous user can patch status update to a job in #all config. Added instance authorization * Implement JobDelete authorization

add deleteJobGroups from env to configuration

4e7c618

add username for anonymous user when creating/updating status of the job

56a66ad

statusUpdate instead of update

fc8c3ae

Finalized tests for authorization

2718699

add delete_job_groups definition

29287bf

fix unit tests after test jobconfig.json changes

8810393

despadam requested a review from sbliven June 26, 2024 13:05

empty collections before testing

31d148e

despadam requested changes Jul 1, 2024

View reviewed changes

changes requested in PR

e274398

eremove version duplicate

6f50600

sofyalaski mentioned this pull request Jul 10, 2024

Jobs should only use the SciCat instance configuration #1320

Merged

sofyalaski added 3 commits July 11, 2024 21:15

Merge remote-tracking branch 'upstream/release-jobs' into jobs-tests

eb1962f

merge chanhes after conversation on how configuration will be stored

change getJobMatchingConfiguration function to explicitly use job typ…

070e964

…e, st it can be used for all cases where we need to extract the configuration, without having to pass the full dto

run full testing workflow on pushing to release branches

9fa1b8e

sofyalaski and others added 2 commits July 14, 2024 22:11

add env variable for jobs config path for github workflow

759ddfb

Fix lint errors

6667320

- Run lint:fix - Ignore no-explicit-any rule for jobParams (which are not type checked) - remove 'read' jobOperation (not implemented)

Junjiequan requested changes Jul 23, 2024

View reviewed changes

test/config/pretest.js Outdated Show resolved Hide resolved

sbliven and others added 5 commits July 23, 2024 20:39

Merge branch 'release-jobs' into jobs-tests

7655856

Fix lint following merge

420f1df

Update test/config/pretest.js to use "dotenv"

46ade45

Co-authored-by: Jay <[email protected]>

Fix pretest.js after merging suggestions

8a07ea2

Fix pretest.js

5210a82

Junjiequan approved these changes Jul 25, 2024

View reviewed changes

sbliven added 2 commits July 27, 2024 00:49

Fix jobconfig no-explicit-any issue (missed logaction)

7c9c8ae

sbliven approved these changes Jul 26, 2024

View reviewed changes

despadam reviewed Jul 28, 2024

View reviewed changes

sofyalaski added 2 commits July 29, 2024 15:11

despadam merged commit 4e67d3a into SciCatProject:release-jobs Jul 29, 2024
7 checks passed

despadam mentioned this pull request Jul 29, 2024

Validate actions during their creation, store only configVersion in the Job schema #1341

Closed

sofyalaski mentioned this pull request Jul 30, 2024

remove nested structure in tests authorization #1350

Merged

4 tasks

sofyalaski deleted the jobs-tests branch July 30, 2024 10:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jobs tests #1286

Jobs tests #1286

sofyalaski commented Jun 26, 2024 •

edited

Loading

despadam left a comment

sofyalaski commented Jul 1, 2024

sofyalaski commented Jul 14, 2024

sbliven left a comment

sbliven Jul 23, 2024

sbliven Jul 23, 2024

sofyalaski Jul 29, 2024

sbliven Jul 23, 2024

sofyalaski Jul 29, 2024

sbliven Jul 23, 2024

sofyalaski Jul 29, 2024

sbliven Jul 23, 2024

sofyalaski Jul 30, 2024

sbliven Jul 23, 2024

sofyalaski Jul 29, 2024

despadam Jul 28, 2024

despadam Jul 29, 2024

sofyalaski commented Jul 29, 2024 •

edited

Loading

Jobs tests #1286

Jobs tests #1286

Conversation

sofyalaski commented Jun 26, 2024 • edited Loading

Description

Motivation

Fixes:

jobs.controller.ts

jobs.service.ts

casl-ability.factory.ts

Changes

Tests included

despadam left a comment

Choose a reason for hiding this comment

sofyalaski commented Jul 1, 2024

sofyalaski commented Jul 14, 2024

sbliven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sofyalaski commented Jul 29, 2024 • edited Loading

sofyalaski commented Jun 26, 2024 •

edited

Loading

sofyalaski commented Jul 29, 2024 •

edited

Loading