From 1d11ed5bdb9a8101680ed0a67600cdd3ca18f3f3 Mon Sep 17 00:00:00 2001 From: ike thecoder Date: Wed, 1 Nov 2023 18:03:13 -0700 Subject: [PATCH] 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 --- src/api-openapi.js | 13 ++++- src/batch/feed-worker.ts | 30 +++++++++-- src/batch/transformations/connectOne.ts | 2 +- src/batch/types.ts | 8 +++ src/controllers/ioc/assert.ts | 19 +++++++ src/controllers/v2/ContentController.ts | 4 +- src/controllers/v2/DatasetController.ts | 4 +- src/controllers/v2/IssuerController.ts | 4 +- src/controllers/v2/OrgDatasetController.ts | 4 +- src/controllers/v2/OrganizationController.ts | 14 +++-- src/controllers/v2/ProductController.ts | 4 +- src/lists/CredentialIssuer.js | 12 +++-- src/lists/Metric.js | 1 + src/lists/extensions/ConsumerProducts.ts | 21 ++++++++ src/lists/extensions/Namespace.ts | 14 ++--- .../api-product-item/api-product-item.tsx | 30 ++++------- .../authorization-form.tsx | 10 ++-- .../manage-labels/manage-labels.tsx | 2 +- .../service-routes/service-routes.tsx | 3 ++ .../manager/authorization-profiles/index.tsx | 13 +++-- src/nextapp/pages/manager/consumers/index.tsx | 20 ++++--- .../pages/manager/namespaces/index.tsx | 1 + src/nextapp/pages/manager/products/index.tsx | 9 ++-- .../pages/manager/service-accounts/index.tsx | 5 +- src/nextapp/shared/services/api.ts | 25 ++++++++- src/server.ts | 9 ++++ src/services/checkStatus.ts | 11 ++-- src/services/issuerMisconfigError.ts | 15 +++++- src/services/keystone/access-request.ts | 53 +++++++++++++++++++ src/services/keystone/index.ts | 1 + .../notification/notification.service.ts | 13 +++-- src/services/workflow/apply.ts | 36 +++++++++---- src/services/workflow/consumer-management.ts | 36 ++++++++++--- src/services/workflow/delete-access.ts | 8 +-- src/services/workflow/delete-namespace.ts | 32 +++++------ src/services/workflow/index.ts | 1 + .../integrated/keystonejs/accessRequest.ts | 43 +++++++++++++++ 37 files changed, 401 insertions(+), 129 deletions(-) create mode 100644 src/controllers/ioc/assert.ts create mode 100644 src/test/integrated/keystonejs/accessRequest.ts diff --git a/src/api-openapi.js b/src/api-openapi.js index b2d1f99a2..59a91fb0a 100644 --- 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 index b33db0978..b2e368a3c 100644 --- 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 { + 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 index d9921039a..ca95335e0 100644 --- 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 index 81601e87a..770cd390e 100644 --- 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 new file mode 100644 index 000000000..5c712a5ed --- /dev/null +++ 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 index b8f4327ef..501982517 100644 --- 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 { 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 index a295cd80f..f0882c6cb 100644 --- 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 index db1b4770a..98f414bda 100644 --- 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 { - 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 index af6a2613c..698ea58c4 100644 --- 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 { 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 index e7e576c0b..26a3652c9 100644 --- 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 { 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 { // 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 index 605919c83..81340dc13 100644 --- 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 { - 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 index 6147b2c5b..8f62cce4e 100644 --- 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 index b1ed549b0..d47d30f09 100644 --- 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 index 32d51ccd0..1fe020368 100644 --- 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 => { 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 => { 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 => { 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 => { 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 index c03f7f620..c680dd23b 100644 --- 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( diff --git a/src/nextapp/components/api-product-item/api-product-item.tsx b/src/nextapp/components/api-product-item/api-product-item.tsx index 72a4c8c73..e97879ecc 100644 --- a/src/nextapp/components/api-product-item/api-product-item.tsx +++ b/src/nextapp/components/api-product-item/api-product-item.tsx @@ -38,7 +38,7 @@ const ApiProductItem: React.FC = ({ id, preview, }) => { - const isPublic = data.environments.some((e) => e.flow === 'public'); + const isProtected = data.environments.some((e) => e.flow !== 'public'); const isTiered = data.environments.some((e) => e.anonymous); return ( @@ -49,7 +49,7 @@ const ApiProductItem: React.FC = ({ @@ -63,25 +63,13 @@ const ApiProductItem: React.FC = ({ )} - {!isTiered && ( - <> - {isPublic && ( - - )} - {!isPublic && ( - - )} - + {!isTiered && isProtected && ( + )} {isTiered && ( diff --git a/src/nextapp/components/authorization-profile-form/authorization-form.tsx b/src/nextapp/components/authorization-profile-form/authorization-form.tsx index 3e5bc0088..76f86fc84 100644 --- a/src/nextapp/components/authorization-profile-form/authorization-form.tsx +++ b/src/nextapp/components/authorization-profile-form/authorization-form.tsx @@ -99,11 +99,11 @@ const AuthorizationForm: React.FC = ({ description: `Automatic issuing of the credential means that this owner (${ownerName}) has configured appropriate credentials here to allow the API Manager to manage Clients on the particular OIDC Provider.`, value: 'auto', }, - { - title: 'Manual', - description: `Manual issuing of the credential means that this owner (${ownerName}) will complete setup of the new credential with the particular OIDC Provider, and communicate that to the requestor via email or other means.`, - value: 'manual', - }, + // { + // title: 'Manual', + // description: `Manual issuing of the credential means that this owner (${ownerName}) will complete setup of the new credential with the particular OIDC Provider, and communicate that to the requestor via email or other means.`, + // value: 'manual', + // }, ]} /> diff --git a/src/nextapp/components/manage-labels/manage-labels.tsx b/src/nextapp/components/manage-labels/manage-labels.tsx index e4537627d..3ea6ae004 100644 --- a/src/nextapp/components/manage-labels/manage-labels.tsx +++ b/src/nextapp/components/manage-labels/manage-labels.tsx @@ -133,7 +133,7 @@ const ManageLabels: React.FC = ({ data, id, queryKey }) => { } catch (err) { toast({ title: 'Labels Update Failed', - description: err.message, + description: err, status: 'error', }); } diff --git a/src/nextapp/components/service-routes/service-routes.tsx b/src/nextapp/components/service-routes/service-routes.tsx index 1a9680b96..3ea83b50b 100644 --- a/src/nextapp/components/service-routes/service-routes.tsx +++ b/src/nextapp/components/service-routes/service-routes.tsx @@ -25,6 +25,9 @@ const ServiceRoutes: React.FC = ({ data }) => { : ['ALL']; const hosts: string[] = JSON.parse(route.hosts); const paths: string[] = JSON.parse(route.paths) ?? ['/']; + if (paths.length === 0) { + paths.push('/'); + } const hostPaths = hosts .map((h: string) => paths.map((p) => `https://${h}${p}`)) .flat(); diff --git a/src/nextapp/pages/manager/authorization-profiles/index.tsx b/src/nextapp/pages/manager/authorization-profiles/index.tsx index 70bfc815d..f401af796 100644 --- a/src/nextapp/pages/manager/authorization-profiles/index.tsx +++ b/src/nextapp/pages/manager/authorization-profiles/index.tsx @@ -1,6 +1,7 @@ import * as React from 'react'; import api, { useApi, useApiMutation } from '@/shared/services/api'; import ActionsMenu from '@/components/actions-menu'; +import ConfirmationDialog from '@/components/confirmation-dialog'; import AuthorizationProfileForm from '@/components/authorization-profile-form'; import { Box, @@ -220,9 +221,15 @@ const AuthorizationProfiles: React.FC< - - Delete - + handleDelete(c.id))()} + > + Delete Profile... + diff --git a/src/nextapp/pages/manager/consumers/index.tsx b/src/nextapp/pages/manager/consumers/index.tsx index 5fabde642..187168192 100644 --- a/src/nextapp/pages/manager/consumers/index.tsx +++ b/src/nextapp/pages/manager/consumers/index.tsx @@ -1,5 +1,6 @@ import * as React from 'react'; import ActionsMenu from '@/components/actions-menu'; +import ConfirmationDialog from '@/components/confirmation-dialog'; import api, { useApi, useApiMutation } from '@/shared/services/api'; import { Box, @@ -293,13 +294,20 @@ const ConsumersPage: React.FC< > Grant Access - handleDelete(d.id))()} > - Delete Consumer - + + Delete Consumer... + + diff --git a/src/nextapp/pages/manager/namespaces/index.tsx b/src/nextapp/pages/manager/namespaces/index.tsx index 0a05dffbe..991053adf 100644 --- a/src/nextapp/pages/manager/namespaces/index.tsx +++ b/src/nextapp/pages/manager/namespaces/index.tsx @@ -157,6 +157,7 @@ const NamespacesPage: React.FC = () => { } catch (err) { toast({ title: 'Delete namespace failed', + description: err, status: 'error', isClosable: true, }); diff --git a/src/nextapp/pages/manager/products/index.tsx b/src/nextapp/pages/manager/products/index.tsx index f17978ca4..762042ba2 100644 --- a/src/nextapp/pages/manager/products/index.tsx +++ b/src/nextapp/pages/manager/products/index.tsx @@ -53,7 +53,7 @@ const ProductsPage: React.FC = () => { } catch (err) { toast({ title: 'Publish settings change failed', - description: `${err}`, + description: err, status: 'error', }); } @@ -104,11 +104,12 @@ const ProductsPage: React.FC = () => { size="sm" lineHeight="24px" > - Publish APIs + Grant permission to publish APIs - By enabling Publish APIs, consumers can find and request - access to your APIs from the Directory. + Allow providers to publish their products to the API + directory so they can make their APIs discoverable to + consumers. diff --git a/src/nextapp/pages/manager/service-accounts/index.tsx b/src/nextapp/pages/manager/service-accounts/index.tsx index 5e8fb4951..8801c4447 100644 --- a/src/nextapp/pages/manager/service-accounts/index.tsx +++ b/src/nextapp/pages/manager/service-accounts/index.tsx @@ -100,10 +100,7 @@ const ServiceAccountsPage: React.FC< title="Service Accounts" > - - Service Accounts allow you to access BC Government APIs via the - Gateway API or the Gateway CLI - + Service Accounts allow you to administer your gateway ( ); }; +async function prepareError(response: any) { + const contentType = response.headers.get('Content-Type'); + if (contentType?.includes('application/json')) { + const json = await response.json(); + const hasErrors = Boolean(json?.errors) || Boolean(json?.details); + if (hasErrors) { + if (json.errors && json.errors[0]?.data?.messages) { + throw json.errors[0]?.data?.messages.join('\n'); + } else if (json.details) { + const result = []; + Object.entries(json.details).forEach(([key, value]) => { + result.push(`${key}: ${(value as any).message}`); + }); + throw result.join('\n'); + } + throw json.errors?.map((e) => e.message).join('\n'); + } + } + throw `${response.statusText}`; +} + export default api; diff --git a/src/server.ts b/src/server.ts index 5ad25937c..749847e3e 100644 --- a/src/server.ts +++ b/src/server.ts @@ -281,6 +281,15 @@ const configureExpress = (app: any) => { const express = require('express'); app.use(express.json()); + app.use(function errorHandler(err: any, req: any, res: any, next: any) { + if (err instanceof SyntaxError) { + return res.status(422).json({ + message: 'Syntax Error Parsing JSON', + }); + } + next(); + }); + // app.get('/', (req, res, next) => { // console.log(req.path) // req.path == "/" ? res.redirect('/home') : next() diff --git a/src/services/checkStatus.ts b/src/services/checkStatus.ts index 99b1acf80..f83b44500 100644 --- a/src/services/checkStatus.ts +++ b/src/services/checkStatus.ts @@ -1,13 +1,17 @@ import { logger } from '../logger'; -import { IssuerMisconfigError } from './issuerMisconfigError'; +import { + IssuerMisconfigDetail, + IssuerMisconfigError, +} from './issuerMisconfigError'; export async function checkStatus(res: any) { if (res.ok) { return res; } else { - const error = { + const error: IssuerMisconfigDetail = { reason: 'unknown_error', + description: '', status: `${res.status} ${res.statusText}`, }; logger.error('Error - %d %s', res.status, res.statusText); @@ -16,7 +20,8 @@ export async function checkStatus(res: any) { logger.error('ERROR ' + body); try { const errors = JSON.parse(body); - error['reason'] = errors['error']; + error.reason = errors?.error ?? ''; + error.description = errors?.error_description ?? ''; logger.error('Added reason to error: %j', error); } catch (e) { logger.error('Not able to parse error response (%s)', e); diff --git a/src/services/issuerMisconfigError.ts b/src/services/issuerMisconfigError.ts index 35273d76f..68510d3df 100644 --- a/src/services/issuerMisconfigError.ts +++ b/src/services/issuerMisconfigError.ts @@ -1,11 +1,22 @@ +export interface IssuerMisconfigDetail { + reason: string; + description: string; + status: string; +} + export class IssuerMisconfigError extends Error { - public errors: any; + public errors: IssuerMisconfigDetail[]; - constructor(message: any) { + constructor(message: IssuerMisconfigDetail) { super(JSON.stringify(message)); Error.captureStackTrace(this, this.constructor); this.name = this.constructor.name; this.errors = [message]; } + + text() { + const detail = this.errors[0]; + return `[status] "${detail.status}" [reason] "${detail.reason}" [description] "${detail.description}"`; + } } diff --git a/src/services/keystone/access-request.ts b/src/services/keystone/access-request.ts index b298c504d..289bbd2c0 100644 --- a/src/services/keystone/access-request.ts +++ b/src/services/keystone/access-request.ts @@ -121,6 +121,59 @@ export async function getAccessRequestByNamespaceServiceAccess( : result.data.allAccessRequests[0]; } +export async function getOpenAccessRequestsByConsumer( + context: any, + ns: string, + consumerId: string +): Promise { + logger.debug( + '[getOpenAccessRequestsByConsumer] ns=%s consumer=%s', + ns, + consumerId + ); + const query = gql` + query GetNamespaceOpenAccessRequestsByConsumer( + $ns: String! + $consumerId: String! + ) { + allAccessRequests( + where: { + serviceAccess: { consumer: { id: $consumerId } } + isComplete: false + } + ) { + id + name + isApproved + isIssued + isComplete + additionalDetails + serviceAccess { + id + consumer { + username + } + } + createdAt + } + } + `; + + const result = await context.executeGraphQL({ + query, + variables: { ns, consumerId }, + }); + logger.debug('[getOpenAccessRequestsByConsumer] result %j', result); + + assert.strictEqual( + 'errors' in result, + false, + 'Error retrieving access request record' + ); + + return result.data.allAccessRequests; +} + export async function lookupEnvironmentAndApplicationByAccessRequest( context: any, id: string diff --git a/src/services/keystone/index.ts b/src/services/keystone/index.ts index 84558efab..5be38d522 100644 --- a/src/services/keystone/index.ts +++ b/src/services/keystone/index.ts @@ -1,5 +1,6 @@ export { getAccessRequestsByNamespace, + getOpenAccessRequestsByConsumer, lookupEnvironmentAndApplicationByAccessRequest, linkServiceAccessToRequest, markAccessRequestAsNotIssued, diff --git a/src/services/notification/notification.service.ts b/src/services/notification/notification.service.ts index 5322c654d..a38cd3f28 100644 --- a/src/services/notification/notification.service.ts +++ b/src/services/notification/notification.service.ts @@ -9,6 +9,8 @@ import { NotificationConfig, EmailNotification, User } from './config'; import { ConfigService } from '../config.service'; +const isEmpty = (str: string) => !str?.length; + export class NotificationService { private notifyConfig: NotificationConfig; constructor(private readonly config: ConfigService) { @@ -16,23 +18,24 @@ export class NotificationService { } private templateToContent(to: User, templateName: string) { + const { logger } = this; + const name = isEmpty(to.name) ? 'Portal User' : to.name; + + logger.debug('Notification triggered [%s] %j', name, to); const template = fs.readFileSync( path.resolve(__dirname, `templates/${templateName}.html`), 'utf8' ); - return template.replace('{{name}}', to.name); + return template.replace('{{name}}', name); } public async notify(user: User, email: EmailNotification) { const { logger } = this; - try { if (!this.notifyConfig.enabled) { return; } - logger.debug('Notification triggered', user); - //we don't notify the active user at all as they made the change var emailContent = this.templateToContent(user, email.template); @@ -78,7 +81,7 @@ export class NotificationService { // this.logger.debug("Email sent: " + info.response); // }) } catch (err) { - logger.error('Sending notification to %s failed - %s', user.email, err); + logger.error('[FAILED] Notification to %s failed - %s', user.email, err); } } diff --git a/src/services/workflow/apply.ts b/src/services/workflow/apply.ts index 98aba907a..752ef9720 100644 --- a/src/services/workflow/apply.ts +++ b/src/services/workflow/apply.ts @@ -32,6 +32,7 @@ import { saveConsumerLabels } from './consumer-management'; import { StructuredActivityService } from './namespace-activity'; import { KeycloakClientRolesService } from '../keycloak/client-roles'; import { genClientId } from './client-shared-idp'; +import { IssuerMisconfigError } from '../issuerMisconfigError'; const logger = Logger('wf.Apply'); @@ -136,17 +137,30 @@ export const Apply = async ( logger.error('Failed to rollback access request %s', err); } ); - await recordActivity( - context, - operation, - 'AccessRequest', - updatedItem.id, - 'Failed to Apply Workflow - ' + err, - 'failed', - '', - productNamespace - ); - throw err; + if (err instanceof IssuerMisconfigError) { + await recordActivity( + context, + operation, + 'AccessRequest', + updatedItem.id, + `Failed to Apply Workflow: ${err.text()}`, + 'failed', + '', + productNamespace + ); + } else { + await recordActivity( + context, + operation, + 'AccessRequest', + updatedItem.id, + 'Failed to Apply Workflow - ' + err, + 'failed', + '', + productNamespace + ); + } + throw new Error('Communication error with issuer'); } return; } diff --git a/src/services/workflow/consumer-management.ts b/src/services/workflow/consumer-management.ts index e009458f8..fe3fc55d4 100644 --- a/src/services/workflow/consumer-management.ts +++ b/src/services/workflow/consumer-management.ts @@ -51,8 +51,8 @@ import { getAccessRequestByNamespaceServiceAccess, + getOpenAccessRequestsByConsumer, lookupConsumerPlugins, - lookupCredentialReferenceByServiceAccess, lookupLabeledServiceAccessesForNamespace, lookupServiceAccessesByConsumer, lookupEnvironmentAndIssuerById, @@ -80,12 +80,10 @@ import { GatewayConsumer, LabelCreateInput, LabelUpdateInput, - Product, } from '../keystone/types'; import { KeycloakClientRegistrationService, KeycloakClientService, - KeycloakUserService, } from '../keycloak'; import { addConsumerLabel, @@ -94,7 +92,6 @@ import { } from '../keystone/labels'; import { getActivityByRefId } from '../keystone/activity'; import { syncPlugins, trimPlugin } from './consumer-plugins'; -import { removeAllButKeys } from '../../batch/feed-worker'; import { KeycloakClientRolesService } from '../keycloak/client-roles'; import { genClientId } from './client-shared-idp'; @@ -148,8 +145,12 @@ export async function allScopesAndRoles( envs .filter((env) => env.credentialIssuer) .forEach((env) => { - result.scopes.push(...JSON.parse(env.credentialIssuer.availableScopes)); - result.roles.push(...JSON.parse(env.credentialIssuer.clientRoles)); + result.scopes.push( + ...JSON.parse(env.credentialIssuer.availableScopes ?? '[]') + ); + result.roles.push( + ...JSON.parse(env.credentialIssuer.clientRoles ?? '[]') + ); }); result.scopes = [...new Set(result.scopes)]; @@ -523,6 +524,12 @@ export async function revokeAccessFromConsumer( const prodEnvAccessItem = prodEnvAccessFiltered[0]; + assert.strictEqual( + prodEnvAccessItem.serviceAccessId == null, + true, + 'Delete this consumer to revoke remaining access' + ); + assert.strictEqual(prodEnvAccessItem.revocable, true, 'Access not revocable'); const serviceAccessId = prodEnvAccessItem.serviceAccessId; @@ -852,6 +859,23 @@ export async function saveConsumerLabels( logger.debug('[saveConsumerLabels] Changes %j', changes); } +export async function enforceCheckForNoPendingRequests( + context: any, + ns: string, + consumerId: string +) { + const openRequests = await getOpenAccessRequestsByConsumer( + context, + ns, + consumerId + ); + assert.strictEqual( + openRequests.length == 0, + true, + 'Pending access requests exist for this consumer; requests must be approved or rejected first' + ); +} + /** * * @param prodEnv diff --git a/src/services/workflow/delete-access.ts b/src/services/workflow/delete-access.ts index b772f6292..7c4f245d5 100644 --- a/src/services/workflow/delete-access.ts +++ b/src/services/workflow/delete-access.ts @@ -135,11 +135,11 @@ export const DeleteAccess = async (context: any, keys: any) => { svc.consumerType == 'client' && svc.consumer.username != 'anonymous' && svc.productEnvironment && - deleteRecord(context, 'GatewayConsumer', { id: svc.consumer.id }, [ + (await deleteRecord(context, 'GatewayConsumer', { id: svc.consumer.id }, [ 'id', 'extForeignKey', 'customId', - ]); + ])); if ( svc.consumer != null && @@ -147,8 +147,8 @@ export const DeleteAccess = async (context: any, keys: any) => { svc.consumer.username != 'anonymous' && svc.productEnvironment ) { - // Asynchronously do the deletion of the backend IdP and Kong - kongApi + // Synchronously do the deletion of the backend IdP and Kong + await kongApi .deleteConsumer(svc.consumer.extForeignKey) .then(async () => { if (flow == 'client-credentials') { diff --git a/src/services/workflow/delete-namespace.ts b/src/services/workflow/delete-namespace.ts index 0d63d9704..4abd6063a 100644 --- a/src/services/workflow/delete-namespace.ts +++ b/src/services/workflow/delete-namespace.ts @@ -33,7 +33,8 @@ const logger = Logger('wf.DeleteNamespace'); export const DeleteNamespaceValidate = async ( context: any, - ns: string + ns: string, + force: boolean ): Promise => { logger.debug('Validate Deleting Namespace ns=%s', ns); @@ -45,11 +46,6 @@ export const DeleteNamespaceValidate = async ( const accessList = await lookupServiceAccessesByEnvironment(context, ns, ids); - const serviceAccountAccessList = await lookupServiceAccessesForNamespace( - context, - ns - ); - const messages = []; if (accessList.length > 0) { messages.push( @@ -65,21 +61,17 @@ export const DeleteNamespaceValidate = async ( } been configured in this namespace.` ); } - if (serviceAccountAccessList.length > 0) { - messages.push( - `${serviceAccountAccessList.length} ${ - serviceAccountAccessList.length == 1 - ? 'service account exists' - : 'service accounts exist' - } in this namespace.` - ); - } - assert.strictEqual( - gwServices.length == 0 && accessList.length == 0, - true, - messages.join(' ') - ); + // Even when force=true, if there are consumers then do not proceed + if (accessList.length != 0) { + const msg = `${accessList.length} ${ + accessList.length == 1 ? 'consumer has' : 'consumers have' + } access to products in this namespace.`; + assert.strictEqual(accessList.length == 0, true, msg); + } + if (!force) { + assert.strictEqual(gwServices.length == 0, true, messages.join(' ')); + } }; export const DeleteNamespaceRecordActivity = async ( diff --git a/src/services/workflow/index.ts b/src/services/workflow/index.ts index bb5b8627b..c6f57aa2d 100644 --- a/src/services/workflow/index.ts +++ b/src/services/workflow/index.ts @@ -18,6 +18,7 @@ export { revokeAllConsumerAccess, revokeAccessFromConsumer, updateConsumerAccess, + enforceCheckForNoPendingRequests, saveConsumerLabels, } from './consumer-management'; diff --git a/src/test/integrated/keystonejs/accessRequest.ts b/src/test/integrated/keystonejs/accessRequest.ts new file mode 100644 index 000000000..0ecade6ae --- /dev/null +++ b/src/test/integrated/keystonejs/accessRequest.ts @@ -0,0 +1,43 @@ +/* +Wire up directly with Keycloak and use the Services +To run: +npm run ts-build +npm run ts-watch +node dist/test/integrated/keystonejs/accessRequest.js +*/ + +import InitKeystone from './init'; +import { o } from '../util'; +import { getOpenAccessRequestsByConsumer } from '../../../services/keystone/access-request'; + +(async () => { + const keystone = await InitKeystone(); + + const ns = 'gw-0dcd7'; + const skipAccessControl = false; + + const identity = { + id: null, + username: 'sample_username', + namespace: ns, + roles: JSON.stringify(['api-owner']), + scopes: [], + userId: null, + } as any; + + const ctx = keystone.createContext({ + skipAccessControl, + authentication: { item: identity }, + }); + + // o(await getOrganizations(ctx)); + + const serviceAccess = await getOpenAccessRequestsByConsumer( + ctx, + ns, + '653860ee26683257394cfe3c' + ); + o(serviceAccess); + + await keystone.disconnect(); +})();