sprint 82 fixes (#943)

* make name in metrics list unique * fix publishing api content * fix publishing api content * upd service acct title * fix batch sync not returning non 200 error on failures * fix public flow access button defect * upd error messaging for revoke consumer access * remove the manual mode for cred issuer * make sure we render routes even if no path is provided * pending requests should be enforced editing consumer * ensure email name is provided * add confirmation dialogs * fix for error when deleting basic issuer * provide better error info in activity for issuers * fix delete namespace race condition * upds based on peer review * upds from peer review - simplify
bcgov · Nov 2, 2023 · 1d11ed5 · 1d11ed5
1 parent ffceb6f
commit 1d11ed5
Show file tree

Hide file tree

Showing 37 changed files with 401 additions and 129 deletions.
diff --git a/src/api-openapi.js b/src/api-openapi.js
@@ -12,6 +12,7 @@ var options = {
 const { Register } = require('./controllers/ioc/registry');
 const { UnauthorizedError } = require('express-jwt');
 const { AssertionError } = require('assert');
+const { BatchSyncException } = require('./batch/types');
 
 class ApiOpenapiApp {
   constructor() {}
@@ -88,7 +89,7 @@ class ApiOpenapiApp {
           message: err.message,
         });
       } else if (err instanceof ValidateError) {
-        console.warn(`Caught Validation Error for ${req.path}:`, err.fields);
+        logger.warn(`Caught Validation Error for ${req.path}:`, err.fields);
         return res.status(422).json({
           message: 'Validation Failed',
           details: err?.fields,
@@ -106,6 +107,16 @@ class ApiOpenapiApp {
           logger.warn('Failed to parse error message %s', e);
         }
         return res.status(422).json(response);
+      } else if (err instanceof SyntaxError) {
+        logger.error(err);
+        logger.error('Syntax Error PATH: %s', req.path);
+        return res.status(422).json({
+          message: 'Syntax Error Parsing JSON',
+        });
+      } else if (err instanceof BatchSyncException) {
+        logger.error(err);
+        logger.error('BatchSync PATH: %s', req.path);
+        return res.status(400).json(err.result);
       } else if (err instanceof Error) {
         logger.error(err);
         logger.error('Error PATH: %s', req.path);

diff --git a/src/batch/feed-worker.ts b/src/batch/feed-worker.ts
@@ -13,7 +13,7 @@ import {
 } from './transformations';
 import { handleNameChange, handleUsernameChange } from './hooks';
 import YAML from 'js-yaml';
-import { BatchResult } from './types';
+import { BatchResult, BatchSyncException } from './types';
 import {
   BatchService,
   BatchWhereClause,
@@ -288,6 +288,20 @@ export const getRecord = async function (
   );
 };
 
+export const syncRecordsThrowErrors = async function (
+  context: any,
+  feedEntity: string,
+  eid: string,
+  json: any,
+  children = false
+): Promise<BatchResult> {
+  const result = await syncRecords(context, feedEntity, eid, json, children);
+  if (result.status !== 200) {
+    throw new BatchSyncException(result);
+  }
+  return result;
+};
+
 export const syncRecords = async function (
   context: any,
   feedEntity: string,
@@ -399,7 +413,12 @@ export const syncRecords = async function (
       }
     } catch (ex) {
       logger.error('Caught exception %s', ex);
-      return { status: 400, result: 'create-failed', childResults };
+      return {
+        status: 400,
+        result: 'create-failed',
+        reason: ex.message,
+        childResults,
+      };
     }
   } else {
     try {
@@ -516,7 +535,12 @@ export const syncRecords = async function (
       }
     } catch (ex) {
       logger.error('Caught exception %s', ex);
-      return { status: 400, result: 'update-failed', childResults };
+      return {
+        status: 400,
+        result: 'update-failed',
+        reason: ex.message,
+        childResults,
+      };
     }
   }
 };

diff --git a/src/batch/transformations/connectOne.ts b/src/batch/transformations/connectOne.ts
@@ -39,7 +39,7 @@ export async function connectOne(
     logger.error(
       `Lookup failed for ${transformInfo['list']} ${transformInfo['refKey']}!`
     );
-    throw Error('Failed to find ' + value + ' in ' + transformInfo['list']);
+    throw Error(`Record not found [${_fieldKey}] ${value}`);
   } else if (
     currentData != null &&
     currentData[_fieldKey] &&

diff --git a/src/batch/types.ts b/src/batch/types.ts
@@ -6,3 +6,11 @@ export interface BatchResult {
   ownedBy?: string;
   childResults?: BatchResult[];
 }
+
+export class BatchSyncException extends Error {
+  result: BatchResult;
+  constructor(result: BatchResult) {
+    super();
+    this.result = result;
+  }
+}
diff --git a/src/controllers/ioc/assert.ts b/src/controllers/ioc/assert.ts
@@ -0,0 +1,19 @@
+import { strict as assert } from 'assert';
+import { ValidateError, FieldErrors } from 'tsoa';
+
+export function assertEqual(
+  condition: any,
+  match: any,
+  field: string,
+  message: string
+) {
+  try {
+    assert.strictEqual(condition, match, message);
+  } catch (e) {
+    var fieldErrors: FieldErrors = {};
+    fieldErrors[field] = {
+      message,
+    };
+    throw new ValidateError(fieldErrors, '');
+  }
+}
diff --git a/src/controllers/v2/ContentController.ts b/src/controllers/v2/ContentController.ts
@@ -20,7 +20,7 @@ import {
   parseJsonString,
   removeEmpty,
   removeKeys,
-  syncRecords,
+  syncRecordsThrowErrors,
 } from '../../batch/feed-worker';
 import express from 'express';
 import multer from 'multer';
@@ -59,7 +59,7 @@ export class ContentController extends Controller {
     @Request() request: any
   ): Promise<BatchResult> {
     body['namespace'] = ns;
-    return await syncRecords(
+    return await syncRecordsThrowErrors(
       this.keystone.createContext(request),
       'Content',
       body['externalLink'],

diff --git a/src/controllers/v2/DatasetController.ts b/src/controllers/v2/DatasetController.ts
@@ -17,7 +17,7 @@ import {
   parseJsonString,
   removeEmpty,
   removeKeys,
-  syncRecords,
+  syncRecordsThrowErrors,
   transformAllRefID,
 } from '../../batch/feed-worker';
 import { Dataset, DraftDataset } from './types';
@@ -56,7 +56,7 @@ export class DatasetController extends Controller {
     // - isInCatalog must be false (OrgDataset should only be updating this)
     removeKeys(body, ['isInDraft', 'isInCatalog']);
 
-    return await syncRecords(
+    return await syncRecordsThrowErrors(
       this.keystone.createContext(request),
       'DraftDataset',
       request.body['name'],

diff --git a/src/controllers/v2/IssuerController.ts b/src/controllers/v2/IssuerController.ts
@@ -14,7 +14,7 @@ import {
 import { KeystoneService } from '../ioc/keystoneInjector';
 import { inject, injectable } from 'tsyringe';
 import {
-  syncRecords,
+  syncRecordsThrowErrors,
   getRecords,
   removeEmpty,
   removeKeys,
@@ -51,7 +51,7 @@ export class IssuerController extends Controller {
     @Body() body: CredentialIssuer,
     @Request() request: any
   ): Promise<BatchResult> {
-    return await syncRecords(
+    return await syncRecordsThrowErrors(
       this.keystone.createContext(request),
       'CredentialIssuer',
       body['name'],

diff --git a/src/controllers/v2/OrgDatasetController.ts b/src/controllers/v2/OrgDatasetController.ts
@@ -15,7 +15,7 @@ import { strict as assert } from 'assert';
 import { KeystoneService } from '../ioc/keystoneInjector';
 import { inject, injectable } from 'tsyringe';
 import {
-  syncRecords,
+  syncRecordsThrowErrors,
   getRecords,
   parseJsonString,
   removeEmpty,
@@ -95,7 +95,7 @@ export class OrgDatasetController extends Controller {
     @Request() request: any
   ): Promise<BatchResult> {
     assert.strictEqual(org, body['organization'], 'Organization Mismatch');
-    return await syncRecords(
+    return await syncRecordsThrowErrors(
       this.keystone.createContext(request, true),
       'DraftDataset',
       body['name'],

diff --git a/src/controllers/v2/OrganizationController.ts b/src/controllers/v2/OrganizationController.ts
@@ -12,8 +12,8 @@ import {
   Get,
   Tags,
 } from 'tsoa';
-import { strict as assert } from 'assert';
 import { KeystoneService } from '../ioc/keystoneInjector';
+import { assertEqual } from '../ioc/assert';
 import { inject, injectable } from 'tsyringe';
 import {
   syncRecords,
@@ -70,9 +70,10 @@ export class OrganizationController extends Controller {
   public async listOrganizationUnits(@Path() org: string): Promise<any> {
     const orgs = await getOrganizations(this.keystone.sudo());
     const match = orgs.filter((o) => o.name === org).pop();
-    assert.strictEqual(
+    assertEqual(
       typeof match === 'undefined',
       false,
+      'org',
       'Organization not found.'
     );
 
@@ -128,9 +129,10 @@ export class OrganizationController extends Controller {
     @Body() body: GroupMembership
   ): Promise<void> {
     // must match either the 'name' or one of the parent nodes
-    assert.strictEqual(
+    assertEqual(
       org === body.name || isParent(body.parent, org),
       true,
+      'org',
       'Organization mismatch'
     );
 
@@ -172,9 +174,10 @@ export class OrganizationController extends Controller {
   ): Promise<{ result: string }> {
     const ctx = this.keystone.sudo();
     const orgLookup = await getOrganizationUnit(ctx, orgUnit);
-    assert.strictEqual(
+    assertEqual(
       orgLookup != null && orgLookup.name === org,
       true,
+      'org',
       'Invalid Organization'
     );
 
@@ -204,9 +207,10 @@ export class OrganizationController extends Controller {
   ): Promise<{ result: string }> {
     const ctx = this.keystone.sudo();
     const orgLookup = await getOrganizationUnit(ctx, orgUnit);
-    assert.strictEqual(
+    assertEqual(
       orgLookup != null && orgLookup.name === org,
       true,
+      'org',
       'Invalid Organization'
     );
 

diff --git a/src/controllers/v2/ProductController.ts b/src/controllers/v2/ProductController.ts
@@ -17,7 +17,7 @@ import {
 import { KeystoneService } from '../ioc/keystoneInjector';
 import { inject, injectable } from 'tsyringe';
 import {
-  syncRecords,
+  syncRecordsThrowErrors,
   getRecords,
   removeEmpty,
   removeKeys,
@@ -64,7 +64,7 @@ export class ProductController extends Controller {
     @Body() body: Product,
     @Request() request: any
   ): Promise<BatchResult> {
-    return await syncRecords(
+    return await syncRecordsThrowErrors(
       this.keystone.createContext(request),
       'Product',
       body['appId'],

diff --git a/src/lists/CredentialIssuer.js b/src/lists/CredentialIssuer.js
@@ -250,11 +250,13 @@ module.exports = {
     },
 
     afterDelete: async function ({ existingItem, context }) {
-      await DeleteClientsFromSharedIdP(
-        context,
-        existingItem.clientId,
-        `${existingItem.inheritFrom}`
-      );
+      if (Boolean(existingItem.inheritFrom)) {
+        await DeleteClientsFromSharedIdP(
+          context,
+          existingItem.clientId,
+          `${existingItem.inheritFrom}`
+        );
+      }
 
       await new StructuredActivityService(
         context,

diff --git a/src/lists/Metric.js b/src/lists/Metric.js
@@ -10,6 +10,7 @@ module.exports = {
     name: {
       type: Text,
       isRequired: true,
+      isUnique: true,
     },
     query: {
       type: Text,

diff --git a/src/lists/extensions/ConsumerProducts.ts b/src/lists/extensions/ConsumerProducts.ts
@@ -6,6 +6,7 @@ import {
   getFilteredNamespaceConsumers,
   getNamespaceConsumerAccess,
   grantAccessToConsumer,
+  enforceCheckForNoPendingRequests,
   revokeAllConsumerAccess,
   revokeAccessFromConsumer,
   saveConsumerLabels,
@@ -227,6 +228,11 @@ module.exports = {
               { query, access }: any
             ): Promise<boolean> => {
               const namespace = context.req.user.namespace;
+              await enforceCheckForNoPendingRequests(
+                context,
+                namespace,
+                consumerId
+              );
               try {
                 logger.debug(
                   '[grantAccessToConsumer] %s %s %j',
@@ -265,6 +271,11 @@ module.exports = {
               { query, access }: any
             ): Promise<boolean> => {
               const namespace = context.req.user.namespace;
+              await enforceCheckForNoPendingRequests(
+                context,
+                namespace,
+                consumerId
+              );
               try {
                 logger.debug(
                   '[revokeAccessFromConsumer] %s %s %j',
@@ -327,6 +338,11 @@ module.exports = {
               { query, access }: any
             ): Promise<boolean> => {
               const namespace = context.req.user.namespace;
+              await enforceCheckForNoPendingRequests(
+                context,
+                namespace,
+                consumerId
+              );
               try {
                 logger.debug(
                   '[updateConsumerAccess] %s %s %j',
@@ -365,6 +381,11 @@ module.exports = {
               { query, access }: any
             ): Promise<boolean> => {
               const namespace = context.req.user.namespace;
+              await enforceCheckForNoPendingRequests(
+                context,
+                namespace,
+                consumerId
+              );
               await saveConsumerLabels(context, namespace, consumerId, labels);
               return true;
             },

diff --git a/src/lists/extensions/Namespace.ts b/src/lists/extensions/Namespace.ts
@@ -575,18 +575,18 @@ module.exports = {
               );
               assert.strictEqual(nsResource.length, 1, 'Invalid Namespace');
 
-              if (args.force === false) {
-                await DeleteNamespaceValidate(
-                  context.createContext({ skipAccessControl: true }),
-                  args.namespace
-                );
-              }
+              await DeleteNamespaceValidate(
+                context.createContext({ skipAccessControl: true }),
+                args.namespace,
+                args.force
+              );
+
               await DeleteNamespace(
                 context.sudo(),
                 getSubjectToken(context.req),
                 args.namespace
               );
-              resourcesApi.deleteResourceSet(nsResource[0].id);
+              await resourcesApi.deleteResourceSet(nsResource[0].id);
 
               // Last thing to do is mark the Namespace group 'decommissioned'
               const nsService = new NamespaceService(