refactor: Implement JobConfig as a NestJS module (#1539)

* Complete refactor of job configuration as NestJS modules

- Developing new actions is documented in README.md for developers
- Move everything related to job config to src/config/job-config
- Major refactor of parsing from jobConfig.yaml to JobConfig objects
- Create module, interface, and creator for each JobAction
- Add handlebar-utils to help consistently applying templates to jobs
  and Dtos.
  - In test contexts the handlebars helpers may not be registered, so
    avoid using custom helpers or register them in the test
- Add options to LogJobAction. This is also good for templating tests
- Change MailService.sendMail to match nest's MailerService.sendMail
- Accept empty jobConfigurationFile as no jobs
- CaslModule now depends indirectly on the MailerModule. Thus all
  controllers need to either mock CaslAbilityFactory or add a mock
  MailerModule before importing CaslModule.
- Most tests disable jobConfig except for those that directly test it.
  These use test/config/jobconfig.yaml

package.json

@@ -62,7 +62,6 @@
     "migrate-mongo": "^11.0.0",
     "mongoose": "^8.4.0",
     "node-fetch": "^3.3.0",
-    "nodemailer": "^6.7.8",
     "openid-client": "^5.1.8",
     "passport": "^0.7.0",
     "passport-jwt": "^4.0.0",

src/app.module.ts

@@ -39,6 +39,8 @@ import { EventEmitterModule } from "@nestjs/event-emitter";
 import { AdminModule } from "./admin/admin.module";
 import { HealthModule } from "./health/health.module";
 import { LoggerModule } from "./loggers/logger.module";
+import { JobConfigModule } from "./config/job-config/jobconfig.module";
+import { CoreJobActionCreators } from "./config/job-config/actions/corejobactioncreators.module";
 
 @Module({
   imports: [
@@ -49,6 +51,8 @@ import { LoggerModule } from "./loggers/logger.module";
     ConfigModule.forRoot({
       load: [configuration],
     }),
+    CoreJobActionCreators,
+    JobConfigModule,
     LoggerModule,
     DatablocksModule,
     DatasetsModule,

src/auth/auth.module.ts

@@ -13,7 +13,7 @@ import { OidcConfig } from "src/config/configuration";
 import { BuildOpenIdClient, OidcStrategy } from "./strategies/oidc.strategy";
 import { accessGroupServiceFactory } from "./access-group-provider/access-group-service-factory";
 import { AccessGroupService } from "./access-group-provider/access-group.service";
-import { CaslAbilityFactory } from "src/casl/casl-ability.factory";
+import { CaslModule } from "src/casl/casl.module";
 
 const OidcStrategyFactory = {
   provide: "OidcStrategy",
@@ -56,14 +56,14 @@ const OidcStrategyFactory = {
       property: "user",
       session: false,
     }),
+    CaslModule,
     UsersModule,
   ],
   providers: [
     AuthService,
     JwtStrategy,
     LdapStrategy,
     LocalStrategy,
-    CaslAbilityFactory,
     OidcStrategyFactory,
     accessGroupServiceFactory,
   ],

src/casl/casl-ability.factory.spec.ts

@@ -1,7 +1,24 @@
+import { MailerModule, MailerService } from "@nestjs-modules/mailer";
 import { CaslAbilityFactory } from "./casl-ability.factory";
+import { Test, TestingModule } from "@nestjs/testing";
+import { CaslModule } from "./casl.module";
+
+class MailerServiceMock {}
 
 describe("CaslAbilityFactory", () => {
+  let casl: CaslAbilityFactory;
+  beforeEach(async () => {
+    const module: TestingModule = await Test.createTestingModule({
+      imports: [MailerModule.forRoot(), CaslModule],
+    })
+      .overrideProvider(MailerService)
+      .useClass(MailerServiceMock)
+      .compile();
+
+    casl = module.get<CaslAbilityFactory>(CaslAbilityFactory);
+  });
+
   it("should be defined", () => {
-    expect(new CaslAbilityFactory()).toBeDefined();
+    expect(casl).toBeDefined();
   });
 });

src/casl/casl-ability.factory.ts

@@ -26,12 +26,13 @@ import { UserSettings } from "src/users/schemas/user-settings.schema";
 import { User } from "src/users/schemas/user.schema";
 import { Action } from "./action.enum";
 import configuration from "src/config/configuration";
+import { JobConfigService } from "src/config/job-config/jobconfig.service";
 import {
   CreateJobAuth,
   StatusUpdateJobAuth,
 } from "src/jobs/types/jobs-auth.enum";
 
-import { JobConfig } from "src/jobs/config/jobconfig";
+import { JobConfig } from "src/config/job-config/jobconfig.interface";
 
 type Subjects =
   | string
@@ -60,6 +61,10 @@ export type AppAbility = MongoAbility<PossibleAbilities, Conditions>;
 
 @Injectable()
 export class CaslAbilityFactory {
+  constructor(private jobConfigService: JobConfigService) {
+    //Logger.log(`Creating CaslAbilityFactory with ${jobConfigService ? Object.keys(jobConfigService.allJobConfigs).length : 0} job types`);
+  }
+
   private endpointAccessors: {
     [endpoint: string]: (user: JWTUser) => AppAbility;
   } = {
@@ -356,7 +361,7 @@ export class CaslAbilityFactory {
 
       // job creation
       if (
-        configuration().jobConfiguration.some(
+        Object.values(this.jobConfigService.allJobConfigs).some(
           (j) => j.create.auth == CreateJobAuth.All,
         )
       ) {
@@ -366,7 +371,7 @@ export class CaslAbilityFactory {
       }
       cannot(Action.JobRead, JobClass);
       if (
-        configuration().jobConfiguration.some(
+        Object.values(this.jobConfigService.allJobConfigs).some(
           (j) => j.statusUpdate.auth == StatusUpdateJobAuth.All,
         )
       ) {
@@ -425,7 +430,7 @@ export class CaslAbilityFactory {
           can(Action.JobRead, JobClass);
 
           if (
-            configuration().jobConfiguration.some(
+            Object.values(this.jobConfigService.allJobConfigs).some(
               (j) =>
                 j.create.auth &&
                 jobCreateEndPointAuthorizationValues.includes(
@@ -448,7 +453,7 @@ export class CaslAbilityFactory {
           can(Action.JobStatusUpdate, JobClass);
         } else {
           if (
-            configuration().jobConfiguration.some(
+            Object.values(this.jobConfigService.allJobConfigs).some(
               (j) =>
                 j.statusUpdate.auth &&
                 jobUpdateEndPointAuthorizationValues.includes(

src/casl/casl.module.ts

@@ -1,7 +1,8 @@
 import { Module } from "@nestjs/common";
 import { CaslAbilityFactory } from "./casl-ability.factory";
-
+import { JobConfigModule } from "src/config/job-config/jobconfig.module";
 @Module({
+  imports: [JobConfigModule],
   providers: [CaslAbilityFactory],
   exports: [CaslAbilityFactory],
 })

src/common/handlebars-helpers.ts

@@ -131,3 +131,13 @@ export const urlencode = (context: string): string => {
 export const base64enc = (context: string): string => {
   return btoa(context);
 };
+
+export const handlebarsHelpers = {
+  unwrapJSON: unwrapJSON,
+  keyToWord: formatCamelCase,
+  eq: (a: unknown, b: unknown) => a === b,
+  jsonify: jsonify,
+  job_v3: job_v3,
+  urlencode: urlencode,
+  base64enc: base64enc,
+};

src/common/mail.service.ts

@@ -1,28 +1,25 @@
-import { MailerService } from "@nestjs-modules/mailer";
+import { ISendMailOptions, MailerService } from "@nestjs-modules/mailer";
 import { Injectable, Logger } from "@nestjs/common";
+import { SentMessageInfo } from "nodemailer";
 
+/**
+ * Service for sending emails using nodemailer.
+ *
+ * Use this rather than MailerService directly to allow configuration in AppModule
+ */
 @Injectable()
 export class MailService {
   constructor(private readonly mailerService: MailerService) {}
 
-  async sendMail(
-    to: string,
-    cc: string,
-    subject: string,
-    mailText: string | null,
-    html: string | null = null,
-  ) {
+  async sendMail(options: ISendMailOptions): Promise<SentMessageInfo> {
     try {
-      Logger.log("Sending email to: " + to, "Utils.sendMail");
-      await this.mailerService.sendMail({
-        to,
-        ...(cc && { cc }),
-        ...(subject && { subject }),
-        ...(html && { html }),
-        ...(mailText && { mailText }),
-      });
+      Logger.log("Sending email to: " + options.to, "Utils.sendMail");
+      await this.mailerService.sendMail(options);
     } catch (error) {
-      Logger.error("Failed sending email to: " + to, "MailService.sendMail");
+      Logger.error(
+        "Failed sending email to: " + options.to,
+        "MailService.sendMail",
+      );
       Logger.error(error, "MailService.sendMail");
     }
   }

src/config/configuration.ts

@@ -1,17 +1,7 @@
-import {
-  loadJobConfig,
-  registerCreateAction,
-  registerStatusUpdateAction,
-} from "../jobs/config/jobconfig";
-import { LogJobAction } from "../jobs/actions/logaction";
-import { EmailJobAction } from "../jobs/actions/emailaction";
-import { URLAction } from "src/jobs/actions/urlaction";
-import { RabbitMQJobAction } from "src/jobs/actions/rabbitmqaction";
 import * as fs from "fs";
 import { merge } from "lodash";
 import localconfiguration from "./localconfiguration";
 import { boolean } from "mathjs";
-import { ValidateAction } from "src/jobs/actions/validateaction";
 import { DEFAULT_PROPOSAL_TYPE } from "src/proposals/schemas/proposal.schema";
 
 const configuration = () => {
@@ -46,7 +36,7 @@ const configuration = () => {
     process.env.OIDC_USERINFO_MAPPING_FIELD_USERNAME || ("" as string);
 
   const jobConfigurationFile =
-    process.env.JOB_CONFIGURATION_FILE || ("jobConfig.yaml" as string);
+    process.env.JOB_CONFIGURATION_FILE || ("" as string);
 
   const defaultLogger = {
     type: "DefaultLogger",
@@ -75,9 +65,6 @@ const configuration = () => {
     }
   });
 
-  registerDefaultActions();
-  const job_configs = loadJobConfig(jobConfigurationFile);
-
   // NOTE: Add the default proposal type here
   Object.assign(jsonConfigMap.proposalTypes, {
     DefaultProposal: DEFAULT_PROPOSAL_TYPE,
@@ -89,7 +76,7 @@ const configuration = () => {
       api: "3",
     },
     swaggerPath: process.env.SWAGGER_PATH || "explorer",
-    jobConfiguration: job_configs,
+    jobConfigurationFile: jobConfigurationFile,
     loggerConfigs: jsonConfigMap.loggers || [defaultLogger],
     adminGroups: adminGroups.split(",").map((v) => v.trim()) ?? [],
     deleteGroups: deleteGroups.split(",").map((v) => v.trim()) ?? [],
@@ -232,23 +219,6 @@ const configuration = () => {
   return merge(config, localconfiguration);
 };
 
-/**
- * Registers built-in JobActions. Should be called exactly once.
- */
-export function registerDefaultActions() {
-  // Create
-  registerCreateAction(LogJobAction);
-  registerCreateAction(EmailJobAction);
-  registerCreateAction(URLAction);
-  registerCreateAction(RabbitMQJobAction);
-  registerCreateAction(ValidateAction);
-  // Status Update
-  registerStatusUpdateAction(LogJobAction);
-  registerStatusUpdateAction(EmailJobAction);
-  registerStatusUpdateAction(RabbitMQJobAction);
-  registerStatusUpdateAction(ValidateAction);
-}
-
 export type OidcConfig = ReturnType<typeof configuration>["oidc"];
 
 export default configuration;

src/config/job-config/README.md

@@ -0,0 +1,51 @@
+# Job Configuration
+
+## Background
+
+Jobs are SciCat's main way of interacting with external systems. Thus the actions which
+should be performed when a job is created or updated (with POST or PATCH requests; see
+[job.controller.ts](../../jobs/job.controller.ts)) tend to be facility specific. To
+facilitate this, the allowed jobs are configured in a YAML or JSON file. An example file
+is available at [jobConfig.example.yaml](../../../jobConfig.example.yaml). The location
+of the job config file is configurable in with the `JOB_CONFIGURATION_FILE` environment
+variable.
+
+The file is parsed when the application starts and converted to a `JobConfigService`
+instance. Job types are arbitrary and facility-specific, but `archive` and `retrieve`
+are traditional for interacting with the data storage system. Authorization can be
+configured for each job type for *create* and *update* requests, and then a list of
+actions are provided which should run after the request.
+
+## Implementing an Action
+
+Implementing an Action requires four (short) files:
+
+1. `action.ts` contains a class implementing `JobAction`. It's constructor can take any
+   arguments, but the existing actions take an `options` argument mirroring the expected
+   yaml config. It does not need to be `@Injectable()`, since it is constructed by a
+   JobActionCreator class.
+2. `action.interface.ts` can contain additional types, e.g. the definition of the
+   expected `options` and a type guard for casting to the options. It should also
+   defined an `actionType` string constant which is used in the yaml file to identity
+   this action.
+3. `action.service.ts` should provide an implementation of `JobActionCreator`. The
+   creator is provided by NestJS as a singleton, so it must be `@Injectable()`. This
+   means that dependencies can be injected into the creator. It has a `create(options)`
+   method, which constructs the action itself by combining the options from the yaml
+   file with any dependencies injected by NestJS.
+4. `action.module.ts` is an NestJS module that provides the creator.
+
+The lists of known creators are provided to Nest with the `CREATE_JOB_ACTION_CREATORS`
+and `UPDATE_JOB_ACTION_CREATORS` symbols. The top-level AppModule imports built-in
+actions from `CoreJobActionCreators`. Core actions should be added to this list. Plugins
+can use the NestJS dependency injection system to extend the lists if needed.
+
+## Accessing the jobConfig
+
+Parsing `jobConfig.yaml` is handled by the JobConfigService. NestJS injects the lists of
+creators (and the file path) during construction. When parsing reaches a new action, the
+correct creator is used to create an instance of the JobAction with the current list of
+options.
+
+Code which needs the configuration for a particular job type (eg jobs.controller.ts)
+injects the `JobConfigService` and can then call `jobConfigService.get(jobType)`.