feat(awm, mbe): added error handling between expresses and kms

Ticket: WP-5241

Author: alextse-bg

src/__tests__/api/advancedWalletManager/kmsClient.test.ts

@@ -0,0 +1,112 @@
+import { AppMode, AdvancedWalletManagerConfig, TlsMode } from '../../../initConfig';
+import { app as expressApp } from '../../../advancedWalletManagerApp';
+
+import express from 'express';
+import nock from 'nock';
+import 'should';
+import * as request from 'supertest';
+
+describe('postMpcV2Key', () => {
+  let cfg: AdvancedWalletManagerConfig;
+  let app: express.Application;
+  let agent: request.SuperAgentTest;
+
+  // test config
+  const kmsUrl = 'http://kms.invalid';
+  const coin = 'tsol';
+  const accessToken = 'test-token';
+
+  before(() => {
+    // nock config
+    nock.disableNetConnect();
+    nock.enableNetConnect('127.0.0.1');
+
+    // app config
+    cfg = {
+      appMode: AppMode.ADVANCED_WALLET_MANAGER,
+      port: 0, // Let OS assign a free port
+      bind: 'localhost',
+      timeout: 60000,
+      httpLoggerFile: '',
+      kmsUrl: kmsUrl,
+      tlsMode: TlsMode.DISABLED,
+      allowSelfSigned: true,
+    };
+
+    // app setup
+    app = expressApp(cfg);
+    agent = request.agent(app);
+  });
+
+  afterEach(() => {
+    nock.cleanAll();
+  });
+
+  it('should bubble up 400 KMS errors', async () => {
+    nock(kmsUrl).post(/.*/).reply(400, { message: 'This is an error message' }).persist();
+
+    const response = await agent
+      .post(`/api/${coin}/mpcv2/initialize`)
+      .set('Authorization', `Bearer ${accessToken}`)
+      .send({ source: 'user' });
+
+    response.status.should.equal(400);
+    response.body.should.have.property('error', 'BadRequestError');
+    response.body.should.have.property('details', 'This is an error message');
+  });
+
+  it('should bubble up 404 KMS errors', async () => {
+    nock(kmsUrl).post(/.*/).reply(404, { message: 'This is an error message' }).persist();
+
+    const response = await agent
+      .post(`/api/${coin}/mpcv2/initialize`)
+      .set('Authorization', `Bearer ${accessToken}`)
+      .send({ source: 'user' });
+
+    response.status.should.equal(404);
+    response.body.should.have.property('error', 'NotFoundError');
+    response.body.should.have.property('details', 'This is an error message');
+  });
+
+  it('should bubble up 409 KMS errors', async () => {
+    nock(kmsUrl).post(/.*/).reply(409, { message: 'This is an error message' }).persist();
+
+    const response = await agent
+      .post(`/api/${coin}/mpcv2/initialize`)
+      .set('Authorization', `Bearer ${accessToken}`)
+      .send({ source: 'user' });
+
+    response.status.should.equal(409);
+    response.body.should.have.property('error', 'ConflictError');
+    response.body.should.have.property('details', 'This is an error message');
+  });
+
+  it('should bubble up 500 KMS errors', async () => {
+    nock(kmsUrl).post(/.*/).reply(500, { message: 'This is an error message' }).persist();
+
+    const response = await agent
+      .post(`/api/${coin}/mpcv2/initialize`)
+      .set('Authorization', `Bearer ${accessToken}`)
+      .send({ source: 'user' });
+
+    response.status.should.equal(500);
+    response.body.should.have.property('error', 'Internal Server Error');
+    response.body.should.have.property('details', 'This is an error message');
+  });
+
+  it('should handle unexpected KMS errors', async () => {
+    nock(kmsUrl).post(/.*/).reply(502, { message: 'Unexpected error' }).persist();
+
+    const response = await agent
+      .post(`/api/${coin}/mpcv2/initialize`)
+      .set('Authorization', `Bearer ${accessToken}`)
+      .send({ source: 'user' });
+
+    response.status.should.equal(500);
+    response.body.should.have.property('error', 'Internal Server Error');
+    response.body.should.have.property(
+      'details',
+      'KMS returned unexpected response. 502: Unexpected error',
+    );
+  });
+});

src/__tests__/api/advancedWalletManager/recoveryMpcV2.test.ts

@@ -155,7 +155,9 @@ describe('recoveryMpcV2', async () => {
 
     signatureResponse.status.should.equal(400);
     signatureResponse.body.should.have.property('error');
-    signatureResponse.body.error.should.startWith(
+    signatureResponse.body.error.should.equal('BadRequestError');
+    signatureResponse.body.should.have.property('details');
+    signatureResponse.body.details.should.startWith(
       'Failed to construct eth transaction from message hex',
     );
   });

src/__tests__/api/master/recoveryWalletMpcV2.test.ts

@@ -202,7 +202,9 @@ describe('MBE mpcv2 recovery', () => {
 
     response.status.should.equal(422);
     response.body.should.have.property('error');
-    response.body.error.should.equal(
+    response.body.error.should.equal('ValidationError');
+    response.body.should.have.property('details');
+    response.body.details.should.equal(
       'ECDSA ETH-like recovery specific parameters are required for MPC recovery',
     );
   });

src/__tests__/api/master/sendMany.test.ts

@@ -5,7 +5,7 @@ import * as request from 'supertest';
 import nock from 'nock';
 import { app as expressApp } from '../../../masterBitGoExpressApp';
 import { AppMode, MasterExpressConfig, TlsMode } from '../../../shared/types';
-import { ApiResponseError, Environments, Wallet } from '@bitgo-beta/sdk-core';
+import { Environments, Wallet } from '@bitgo-beta/sdk-core';
 import { Tbtc } from '@bitgo-beta/sdk-coin-btc';
 import assert from 'assert';
 
@@ -747,15 +747,11 @@ describe('POST /api/:coin/wallet/:walletId/sendmany', () => {
 
     const verifyStub = sinon.stub(Tbtc.prototype, 'verifyTransaction').resolves(true);
 
-    // Mock advanced wallet manager sign request to return an error
-    const signNock = nock(advancedWalletManagerUrl)
-      .post(`/api/${coin}/multisig/sign`)
-      .replyWithError(
-        new ApiResponseError('Custom API error', 500, {
-          error: 'Custom API error',
-          requestId: 'test-request-id',
-        }),
-      );
+    // Mock enclaved express sign request to return an error
+    const signNock = nock(advancedWalletManagerUrl).post(`/api/${coin}/multisig/sign`).reply(500, {
+      error: 'Internal Server Error',
+      details: 'Custom API error details',
+    });
 
     const response = await agent
       .post(`/api/${coin}/wallet/${walletId}/sendMany`)
@@ -775,11 +771,8 @@ describe('POST /api/:coin/wallet/:walletId/sendmany', () => {
     response.status.should.equal(500);
     response.body.should.have.property('error');
     response.body.should.have.property('details');
-    response.body.error.should.equal('BitGoApiResponseError');
-    response.body.details.should.deepEqual({
-      error: 'Custom API error',
-      requestId: 'test-request-id',
-    });
+    response.body.error.should.equal('Internal Server Error');
+    response.body.details.should.deepEqual('Custom API error details');
 
     walletGetNock.done();
     keychainGetNock.done();

src/advancedWalletManager/routers/advancedWalletManagerApiSpec.ts

@@ -17,12 +17,8 @@ import { DklsDkg, DklsTypes } from '@bitgo-beta/sdk-lib-mpc';
 import express from 'express';
 import * as t from 'io-ts';
 
-import {
-  BadRequestResponse,
-  InternalServerErrorResponse,
-  NotImplementedResponse,
-  UnprocessableEntityResponse,
-} from '../../shared/errors';
+import coinFactory from '../../shared/coinFactory';
+import { ErrorResponses, NotImplementedError } from '../../shared/errors';
 
 import { postIndependentKey } from '../../api/advancedWalletManager/handlers/postIndependentKey';
 import { recoveryMultisigTransaction } from '../../api/advancedWalletManager/handlers/recoveryMultisigTransaction';
@@ -39,8 +35,6 @@ import { ecdsaMPCv2Finalize } from '../../api/advancedWalletManager/handlers/ecd
 import { ecdsaMPCv2Recovery } from '../../api/advancedWalletManager/handlers/ecdsaMPCv2Recovery';
 import { signEddsaRecoveryTransaction } from '../../api/advancedWalletManager/handlers/signEddsaRecoveryTransaction';
 import { isEddsaCoin } from '../../shared/coinUtils';
-import { MethodNotImplementedError } from '@bitgo-beta/sdk-core';
-import coinFactory from '../../shared/coinFactory';
 
 // Request type for /key/independent endpoint
 const IndependentKeyRequest = {
@@ -52,7 +46,7 @@ const IndependentKeyRequest = {
 const IndependentKeyResponse: HttpResponse = {
   // TODO: Define proper response type
   200: t.any,
-  ...InternalServerErrorResponse,
+  ...ErrorResponses,
 };
 
 // Request type for /multisig/sign endpoint
@@ -66,7 +60,7 @@ const SignMultisigRequest = {
 const SignMultisigResponse: HttpResponse = {
   // TODO: Define proper response type for signed multisig transaction
   200: t.any,
-  ...InternalServerErrorResponse,
+  ...ErrorResponses,
 };
 
 // Request type for /multisig/recovery endpoint
@@ -83,7 +77,7 @@ const RecoveryMultisigResponse: HttpResponse = {
   200: t.type({
     txHex: t.string,
   }), // the full signed tx
-  ...InternalServerErrorResponse,
+  ...ErrorResponses,
 };
 
 const RecoveryMpcRequest = {
@@ -105,7 +99,7 @@ const RecoveryMpcResponse: HttpResponse = {
   200: t.type({
     txHex: t.string,
   }), // the full signed tx
-  ...InternalServerErrorResponse,
+  ...ErrorResponses,
 };
 
 // Request type for /mpc/sign endpoint
@@ -163,7 +157,7 @@ const SignMpcResponse: HttpResponse = {
       signatureShareRound3: t.any,
     }),
   ]),
-  ...InternalServerErrorResponse,
+  ...ErrorResponses,
 };
 
 const KeyShare = {
@@ -373,10 +367,7 @@ export const AdvancedWalletManagerApiSpec = apiSpec({
       }),
       response: {
         200: t.type(MpcInitializeResponse),
-        500: t.type({
-          error: t.string,
-          details: t.string,
-        }),
+        ...ErrorResponses,
       },
       description: 'Initialize MPC for EdDSA key generation',
     }),
@@ -414,10 +405,7 @@ export const AdvancedWalletManagerApiSpec = apiSpec({
       }),
       response: {
         200: MpcFinalizeResponseType,
-        500: t.type({
-          error: t.string,
-          details: t.string,
-        }),
+        ...ErrorResponses,
       },
       description: 'Finalize key generation and confirm commonKeychain',
     }),
@@ -432,10 +420,7 @@ export const AdvancedWalletManagerApiSpec = apiSpec({
       }),
       response: {
         200: MpcV2InitializeResponseType,
-        500: t.type({
-          error: t.string,
-          details: t.string,
-        }),
+        ...ErrorResponses,
       },
       description: 'Initialize MPC for EdDSA key generation',
     }),
@@ -450,9 +435,7 @@ export const AdvancedWalletManagerApiSpec = apiSpec({
       }),
       response: {
         200: MpcV2RoundResponseType,
-        ...InternalServerErrorResponse,
-        ...BadRequestResponse,
-        ...UnprocessableEntityResponse,
+        ...ErrorResponses,
       },
       description: 'Perform a round in the MPC protocol',
     }),
@@ -467,10 +450,7 @@ export const AdvancedWalletManagerApiSpec = apiSpec({
       }),
       response: {
         200: MpcV2FinalizeResponseType,
-        500: t.type({
-          error: t.string,
-          details: t.string,
-        }),
+        ...ErrorResponses,
       },
       description: 'Finalize the MPC protocol',
     }),
@@ -485,9 +465,7 @@ export const AdvancedWalletManagerApiSpec = apiSpec({
       }),
       response: {
         200: MpcV2RecoveryResponseType,
-        ...BadRequestResponse,
-        ...InternalServerErrorResponse,
-        ...NotImplementedResponse,
+        ...ErrorResponses,
       },
       description: 'Recover a MPC transaction',
     }),
@@ -567,7 +545,9 @@ export function createKeyGenRouter(
         });
         return Response.ok(result);
       } else {
-        throw new MethodNotImplementedError();
+        throw new NotImplementedError(
+          `MPC recovery is not implemented for ${coin.getFamily()} coins`,
+        );
       }
     }),
   ]);