From 2983a57eb9d4e5c51a62158118e3276c9849b459 Mon Sep 17 00:00:00 2001 From: Brian Botha Date: Tue, 15 Oct 2024 18:30:16 +1100 Subject: [PATCH 1/3] feat: adding better error stringification to `ErrorPolykey` [ci skip] --- src/ErrorPolykey.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/ErrorPolykey.ts b/src/ErrorPolykey.ts index b4a1d046f..05e217729 100644 --- a/src/ErrorPolykey.ts +++ b/src/ErrorPolykey.ts @@ -39,6 +39,13 @@ class ErrorPolykey extends AbstractError { json.data.exitCode = this.exitCode; return json; } + + public toString(): string { + const messageString = + this.message.length > 0 ? `("${this.message}")` : '()'; + const chainString = this.cause != null ? `>${String(this.cause)}` : ''; + return `${this.name}${messageString}${chainString}`; + } } export default ErrorPolykey; From 67dfb475965cc7012967df99c2aa3f5d7bca05c3 Mon Sep 17 00:00:00 2001 From: Brian Botha Date: Tue, 15 Oct 2024 18:31:02 +1100 Subject: [PATCH 2/3] fix: updating all `never()` usage to include a descriptive message --- src/PolykeyAgent.ts | 8 ++- src/client/authenticationMiddleware.ts | 4 +- src/client/handlers/IdentitiesAuthenticate.ts | 4 +- src/client/handlers/KeysDecrypt.ts | 2 +- src/client/handlers/KeysEncrypt.ts | 2 +- src/client/handlers/KeysVerify.ts | 2 +- src/discovery/Discovery.ts | 42 +++++++---- src/gestalts/GestaltGraph.ts | 28 +++++--- src/git/http.ts | 4 +- src/git/utils.ts | 4 +- src/nodes/NodeConnection.ts | 6 +- src/nodes/NodeConnectionManager.ts | 18 +++-- src/nodes/NodeGraph.ts | 4 +- src/nodes/NodeManager.ts | 16 +++-- src/nodes/agent/handlers/VaultsGitInfoGet.ts | 8 ++- src/nodes/agent/handlers/VaultsGitPackGet.ts | 8 ++- src/nodes/utils.ts | 6 +- src/notifications/NotificationsManager.ts | 8 ++- src/notifications/utils.ts | 4 +- src/vaults/VaultInternal.ts | 57 ++++++++------- tests/gestalts/utils.ts | 2 +- tests/git/utils.ts | 72 ++++++++++--------- tests/nodes/utils.test.ts | 4 +- tests/tasks/TaskManager.test.ts | 6 +- 24 files changed, 192 insertions(+), 127 deletions(-) diff --git a/src/PolykeyAgent.ts b/src/PolykeyAgent.ts index 337434b82..9848bc7cb 100644 --- a/src/PolykeyAgent.ts +++ b/src/PolykeyAgent.ts @@ -803,7 +803,9 @@ class PolykeyAgent { const setNodeProms = new Array>(); for (const nodeIdEncoded in optionsDefaulted.seedNodes) { const nodeId = nodesUtils.decodeNodeId(nodeIdEncoded); - if (nodeId == null) utils.never(); + if (nodeId == null) { + utils.never(`failed to decode NodeId "${nodeIdEncoded}"`); + } const setNodeProm = this.nodeManager.setNode( nodeId, optionsDefaulted.seedNodes[nodeIdEncoded], @@ -825,7 +827,9 @@ class PolykeyAgent { const initialNodes = seedNodeEntries.map( ([nodeIdEncoded, nodeAddress]) => { const nodeId = nodesUtils.decodeNodeId(nodeIdEncoded); - if (nodeId == null) utils.never('nodeId should be defined'); + if (nodeId == null) { + utils.never(`failed to decode NodeId "${nodeIdEncoded}"`); + } return [nodeId, nodeAddress] as [NodeId, NodeAddress]; }, ); diff --git a/src/client/authenticationMiddleware.ts b/src/client/authenticationMiddleware.ts index e2fe30f26..cbce3d485 100644 --- a/src/client/authenticationMiddleware.ts +++ b/src/client/authenticationMiddleware.ts @@ -105,7 +105,9 @@ function authenticationMiddlewareClient( >({ transform: async (chunk, controller) => { if (forwardFirst) { - if (chunk.params == null) utils.never(); + if (chunk.params == null) { + utils.never('chunk.params must be defined'); + } if (chunk.params.metadata?.authorization == null) { const token = await session.readToken(); if (token != null) { diff --git a/src/client/handlers/IdentitiesAuthenticate.ts b/src/client/handlers/IdentitiesAuthenticate.ts index 86645f71e..6f0c3cd61 100644 --- a/src/client/handlers/IdentitiesAuthenticate.ts +++ b/src/client/handlers/IdentitiesAuthenticate.ts @@ -50,7 +50,7 @@ class IdentitiesAuthenticate extends ServerHandler< const authFlow = provider.authenticate(ctx.timer.getTimeout()); let authFlowResult = await authFlow.next(); if (authFlowResult.done) { - never(); + never('authFlow signalled done too soon'); } if (ctx.signal.aborted) throw ctx.signal.reason; yield { @@ -61,7 +61,7 @@ class IdentitiesAuthenticate extends ServerHandler< }; authFlowResult = await authFlow.next(); if (!authFlowResult.done) { - never(); + never('authFlow did not signal done when expected'); } if (ctx.signal.aborted) throw ctx.signal.reason; yield { diff --git a/src/client/handlers/KeysDecrypt.ts b/src/client/handlers/KeysDecrypt.ts index d61c96bc0..5303c7b0f 100644 --- a/src/client/handlers/KeysDecrypt.ts +++ b/src/client/handlers/KeysDecrypt.ts @@ -19,7 +19,7 @@ class KeysDecrypt extends UnaryHandler< ): Promise> => { const { keyRing }: { keyRing: KeyRing } = this.container; const data = keyRing.decrypt(Buffer.from(input.data, 'binary')); - if (data == null) never(); + if (data == null) never('failed to decrypt DataMessage'); return { data: data.toString('binary'), }; diff --git a/src/client/handlers/KeysEncrypt.ts b/src/client/handlers/KeysEncrypt.ts index 5f6dd385e..11598460c 100644 --- a/src/client/handlers/KeysEncrypt.ts +++ b/src/client/handlers/KeysEncrypt.ts @@ -27,7 +27,7 @@ class KeysEncrypt extends UnaryHandler< try { const jwk = input.publicKeyJwk; publicKey = keysUtils.publicKeyFromJWK(jwk); - if (publicKey == null) never(); + if (publicKey == null) never('failed to get public key from JWK'); } catch (e) { throw new keysErrors.ErrorPublicKeyParse(undefined, { cause: e }); } diff --git a/src/client/handlers/KeysVerify.ts b/src/client/handlers/KeysVerify.ts index 24d29f451..2f6c0b171 100644 --- a/src/client/handlers/KeysVerify.ts +++ b/src/client/handlers/KeysVerify.ts @@ -26,7 +26,7 @@ class KeysVerify extends UnaryHandler< try { const jwk = input.publicKeyJwk; publicKey = keysUtils.publicKeyFromJWK(jwk); - if (publicKey == null) never(); + if (publicKey == null) never('failed to get public key from JWK'); } catch (e) { throw new keysErrors.ErrorPublicKeyParse(undefined, { cause: e }); } diff --git a/src/discovery/Discovery.ts b/src/discovery/Discovery.ts index e04d5cfd2..67456a4e4 100644 --- a/src/discovery/Discovery.ts +++ b/src/discovery/Discovery.ts @@ -171,7 +171,9 @@ class Discovery { if (e === discoveryStoppingTaskReason) { // We need to recreate the task for the vertex const vertexId = gestaltsUtils.decodeGestaltId(vertex); - if (vertexId == null) never(); + if (vertexId == null) { + never(`failed to decode vertex GestaltId "${vertex}"`); + } await this.scheduleDiscoveryForVertex( vertexId, undefined, @@ -399,7 +401,9 @@ class Discovery { ): Promise { this.logger.debug(`Processing vertex: ${vertex}`); const vertexId = gestaltsUtils.decodeGestaltId(vertex); - if (vertexId == null) never(); + if (vertexId == null) { + never(`failed to decode vertex GestaltId "${vertex}"`); + } const [type, id] = vertexId; switch (type) { case 'node': @@ -407,7 +411,7 @@ class Discovery { case 'identity': return await this.processIdentity(id, ctx, lastProcessedCutoffTime); default: - never(); + never(`type must be either "node" or "identity" got "${type}"`); } } @@ -446,7 +450,7 @@ class Discovery { this.logger.info( `Failed to discover ${nodesUtils.encodeNodeId( nodeId, - )} - ${e.toString()}`, + )} - Reason: ${String(e)}`, ); return; } @@ -470,7 +474,9 @@ class Discovery { ); break; default: - never(); + never( + `signedClaim.payload.typ must be "ClaimLinkNode" or "ClaimLinkIdentity" got "${signedClaim.payload.typ}"`, + ); } } await this.gestaltGraph.setVertexProcessedTime( @@ -488,9 +494,13 @@ class Discovery { // Could be node1 or node2 in the claim so get the one that's // not equal to nodeId from above const node1Id = nodesUtils.decodeNodeId(signedClaim.payload.iss); - if (node1Id == null) never(); + if (node1Id == null) { + never(`failed to decode issuer NodeId "${signedClaim.payload.iss}"`); + } const node2Id = nodesUtils.decodeNodeId(signedClaim.payload.sub); - if (node2Id == null) never(); + if (node2Id == null) { + never(`failed to decode subject NodeId "${signedClaim.payload.sub}"`); + } // Verify the claim const node1PublicKey = keysUtils.publicKeyFromNodeId(node1Id); const node2PublicKey = keysUtils.publicKeyFromNodeId(node2Id); @@ -519,7 +529,9 @@ class Discovery { }, ); const claimId = decodeClaimId(signedClaim.payload.jti); - if (claimId == null) never(); + if (claimId == null) { + never(`failed to decode claimId "${signedClaim.payload.jti}"`); + } await this.gestaltGraph.setClaimIdNewest(nodeId, claimId); // Add this vertex to the queue if it hasn't already been visited const linkedGestaltId: GestaltId = ['node', linkedNodeId]; @@ -556,7 +568,9 @@ class Discovery { return; } // Attempt to get the identity info on the identity provider - if (signedClaim.payload.sub == null) never(); + if (signedClaim.payload.sub == null) { + never('signedClaim.payload.sub must be defined'); + } const [providerId, identityId] = JSON.parse(signedClaim.payload.sub); const identityInfo = await this.getIdentityInfo( providerId, @@ -617,7 +631,9 @@ class Discovery { }, ); const claimId = decodeClaimId(signedClaim.payload.jti); - if (claimId == null) never(); + if (claimId == null) { + never(`failed to decode ClaimId "${signedClaim.payload.jti}"`); + } await this.gestaltGraph.setClaimIdNewest(nodeId, claimId); // Add this identity vertex to the queue if it is not present const providerIdentityId = JSON.parse(signedClaim.payload.sub!); @@ -675,7 +691,9 @@ class Discovery { // Claims on an identity provider will always be node -> identity // So just cast payload data as such const linkedNodeId = nodesUtils.decodeNodeId(claim.payload.iss); - if (linkedNodeId == null) never(); + if (linkedNodeId == null) { + never(`failed to decode issuer NodeId "${claim.payload.iss}"`); + } // With this verified chain, we can link const linkedVertexNodeInfo = { nodeId: linkedNodeId, @@ -927,7 +945,7 @@ class Discovery { const data = claim.payload; // Verify the claim with the public key of the node const nodeId = nodesUtils.decodeNodeId(data.iss); - if (nodeId == null) never(); + if (nodeId == null) never(`failed to decode issuer NodeId "${data.iss}"`); const publicKey = keysUtils.publicKeyFromNodeId(nodeId); const token = Token.fromSigned(claim); // If verified, add to the record diff --git a/src/gestalts/GestaltGraph.ts b/src/gestalts/GestaltGraph.ts index 77f6f67e8..30a1aa068 100644 --- a/src/gestalts/GestaltGraph.ts +++ b/src/gestalts/GestaltGraph.ts @@ -298,7 +298,7 @@ class GestaltGraph { break; case 'identity': default: - never(); + never(`type must be either "node" or "identity" got "${type}"`); } } // 2. remove the node information. @@ -318,7 +318,7 @@ class GestaltGraph { case 'identity': return this.setIdentity(info, tran); default: - never(); + never(`type must be either "node" or "identity" got "${type}"`); } } @@ -335,7 +335,7 @@ class GestaltGraph { case 'identity': return this.unsetIdentity(id, tran); default: - never(); + never(`type must be either "node" or "identity" got "${type}"`); } } @@ -468,7 +468,9 @@ class GestaltGraph { never('lastProcessedTime should be valid here'); } const gestaltId = gestaltsUtils.decodeGestaltId(gestaltIdEncoded); - if (gestaltId == null) never('GestaltId should be valid here'); + if (gestaltId == null) { + never(`failed to decode GestaltId "${gestaltIdEncoded}"`); + } yield [gestaltId, lastProcessedTime]; } } @@ -872,7 +874,9 @@ class GestaltGraph { tran, ); } else { - never(); + never( + `invalid claim between "${gestaltInfo1[0]}" and "${gestaltInfo2[0]}", must be a node to node or identity to node claim`, + ); } } @@ -1065,7 +1069,9 @@ class GestaltGraph { tran, ); } else { - never(); + never( + `invalid link between "${type1}" and "${type2}", must be node to node or node to identity`, + ); } } @@ -1104,7 +1110,7 @@ class GestaltGraph { return perm.gestalt; } default: - never(); + never(`type must be either "node" or "identity" got "${type}"`); } } @@ -1143,7 +1149,7 @@ class GestaltGraph { return; } default: - never(); + never(`type must be either "node" or "identity" got "${type}"`); } } @@ -1183,7 +1189,7 @@ class GestaltGraph { return; } default: - never(); + never(`type must be either "node" or "identity" got "${type}"`); } } @@ -1231,7 +1237,7 @@ class GestaltGraph { case 'identity': return await this.getGestaltByIdentity(id, tran); default: - never(); + never(`type must be either "node" or "identity" got "${type}"`); } } @@ -1335,7 +1341,7 @@ class GestaltGraph { return ['identity', gestaltIdentityInfo]; } default: - never(); + never(`type must be either "node" or "identity" got "${type}"`); } } diff --git a/src/git/http.ts b/src/git/http.ts index 3b4f83bf4..bd81dac36 100644 --- a/src/git/http.ts +++ b/src/git/http.ts @@ -324,9 +324,7 @@ async function parsePackRequest( case 'done': return [wants, haves, capabilities]; default: - utils.never( - `Type should be either 'want' or 'have', found '${type}'`, - ); + utils.never(`Type should be either "want" or "have", got "${type}"`); } } } diff --git a/src/git/utils.ts b/src/git/utils.ts index fef9d9b3a..c80b956bc 100644 --- a/src/git/utils.ts +++ b/src/git/utils.ts @@ -248,7 +248,9 @@ async function listObjectsAll({ const objectSet: Set = new Set(); const objectDirs = await fs.promises.readdir(objectsDirPath); for (const objectDir of objectDirs) { - if (typeof objectDir !== 'string') utils.never(); + if (typeof objectDir !== 'string') { + utils.never('objectDir should be a string'); + } if (excludedDirs.includes(objectDir)) continue; const objectIds = await fs.promises.readdir( path.join(objectsDirPath, objectDir), diff --git a/src/nodes/NodeConnection.ts b/src/nodes/NodeConnection.ts index 095379c06..bb93ef1de 100644 --- a/src/nodes/NodeConnection.ts +++ b/src/nodes/NodeConnection.ts @@ -279,7 +279,7 @@ class NodeConnection { } const quicConnection = quicClient.connection; const throwFunction = () => - never('We should never get connections before setting up handling'); + never('we should never get connections before setting up handling'); quicConnection.addEventListener( quicEvents.EventQUICConnectionStream.name, throwFunction, @@ -293,7 +293,9 @@ class NodeConnection { toError: networkUtils.toError, logger: logger.getChild(RPCClient.name), }); - if (validatedNodeId == null) never(); + if (validatedNodeId == null) { + never(`connection validated but no valid NodeId was returned`); + } // Obtaining remote node ID from certificate chain. It should always exist in the chain if validated. // This may de different from the NodeId we validated it as if it renewed at some point. const connection = quicClient.connection; diff --git a/src/nodes/NodeConnectionManager.ts b/src/nodes/NodeConnectionManager.ts index 26624115e..bc24ab83b 100644 --- a/src/nodes/NodeConnectionManager.ts +++ b/src/nodes/NodeConnectionManager.ts @@ -407,9 +407,9 @@ class NodeConnectionManager { const quicSocket = new QUICSocket({ resolveHostname: () => { - // `NodeConnectionManager` must resolve all hostnames before it reaches - // `QUICSocket`. - utils.never(); + utils.never( + '"NodeConnectionManager" must resolve all hostnames before it reaches "QUICSocket"', + ); }, logger: this.logger.getChild(QUICSocket.name), }); @@ -695,7 +695,7 @@ class NodeConnectionManager { const [release, conn] = await acquire(); let caughtError; try { - if (conn == null) utils.never(); + if (conn == null) utils.never('NodeConnection should exist'); return yield* g(conn); } catch (e) { caughtError = e; @@ -1042,12 +1042,16 @@ class NodeConnectionManager { const cert = keysUtils.certFromPEM( quicUtils.derToPEM(der) as CertificatePEM, ); - if (cert == null) utils.never(); + if (cert == null) { + utils.never('failed to parse certificate from connection cert chain'); + } return cert; }); - if (certChain == null) utils.never(); + if (certChain.length === 0) { + utils.never('there must be at least 1 certificate in the chain'); + } const nodeId = keysUtils.certNodeId(certChain[0]); - if (nodeId == null) utils.never(); + if (nodeId == null) utils.never('failed to get NodeId from certificate'); const nodeConnectionNew = NodeConnection.createNodeConnectionReverse({ nodeId, certChain, diff --git a/src/nodes/NodeGraph.ts b/src/nodes/NodeGraph.ts index c63543216..4b0674936 100644 --- a/src/nodes/NodeGraph.ts +++ b/src/nodes/NodeGraph.ts @@ -659,7 +659,9 @@ class NodeGraph { IdInternal.fromBuffer(nodeIdBuffer), tran, ); - if (nodeContact == null) utils.never(); + if (nodeContact == null) { + utils.never('failed to get expected NodeContact'); + } bucket.push([nodeId, nodeContact]); } } diff --git a/src/nodes/NodeManager.ts b/src/nodes/NodeManager.ts index 4f0b3747f..6ab4b5b8e 100644 --- a/src/nodes/NodeManager.ts +++ b/src/nodes/NodeManager.ts @@ -240,7 +240,9 @@ class NodeManager { ); } const nodeId = nodesUtils.decodeNodeId(nodeIdEncoded); - if (nodeId == null) utils.never(); + if (nodeId == null) { + utils.never(`failed to decode NodeId "${nodeIdEncoded}"`); + } return this.nodeConnectionManager.createConnectionMultiple( [nodeId], resolvedHosts, @@ -541,7 +543,7 @@ class NodeManager { const [release, conn] = await acquire(); let caughtError; try { - if (conn == null) utils.never(); + if (conn == null) utils.never('NodeConnection should exist'); return yield* g(conn); } catch (e) { caughtError = e; @@ -1106,7 +1108,7 @@ class NodeManager { for await (const result of resultStream) { const nodeIdNew = nodesUtils.decodeNodeId(result.nodeId); if (nodeIdNew == null) { - utils.never('failed to decode nodeId'); + utils.never(`failed to decode NodeId "${result.nodeId}"`); } nodeConnectionsQueue.queueNodeSignal(nodeIdNew, nodeId); } @@ -1118,7 +1120,9 @@ class NodeManager { }); for await (const { nodeIdEncoded, nodeContact } of resultStream) { const nodeId = nodesUtils.decodeNodeId(nodeIdEncoded); - if (nodeId == null) utils.never(); + if (nodeId == null) { + utils.never(`failed to decode NodeId "${nodeIdEncoded}"`); + } nodeConnectionsQueue.queueNodeDirect(nodeId, nodeContact); } })(); @@ -1826,7 +1830,7 @@ class NodeManager { this.nodeGraph.nodeBucketLimit, tran, ); - if (bucket == null) utils.never(); + if (bucket == null) utils.never('bucket should exist'); let removedNodes = 0; const unsetLock = new Lock(); const pendingPromises: Array> = []; @@ -2156,7 +2160,7 @@ class NodeManager { priority: 0, }); } - if (foundTask == null) utils.never(); + if (foundTask == null) utils.never('task should exist'); return foundTask; } diff --git a/src/nodes/agent/handlers/VaultsGitInfoGet.ts b/src/nodes/agent/handlers/VaultsGitInfoGet.ts index 0c820129b..8c0518713 100644 --- a/src/nodes/agent/handlers/VaultsGitInfoGet.ts +++ b/src/nodes/agent/handlers/VaultsGitInfoGet.ts @@ -33,15 +33,17 @@ class VaultsGitInfoGet extends RawHandler<{ const [headerMessage, inputStream] = input; await inputStream.cancel(); const params = headerMessage.params; - if (params == null || !utils.isObject(params)) utils.never(); + if (params == null || !utils.isObject(params)) { + utils.never('params must be defined and an object'); + } if ( !('vaultNameOrId' in params) || typeof params.vaultNameOrId != 'string' ) { - utils.never(); + utils.never('vaultNameOrId must be defined and a string'); } if (!('action' in params) || typeof params.action != 'string') { - utils.never(); + utils.never('action must be defined and a string'); } const vaultNameOrId = params.vaultNameOrId; const actionType = vaultsUtils.parseVaultAction(params.action); diff --git a/src/nodes/agent/handlers/VaultsGitPackGet.ts b/src/nodes/agent/handlers/VaultsGitPackGet.ts index d5c38b531..dcdc6846e 100644 --- a/src/nodes/agent/handlers/VaultsGitPackGet.ts +++ b/src/nodes/agent/handlers/VaultsGitPackGet.ts @@ -33,12 +33,14 @@ class VaultsGitPackGet extends RawHandler<{ } const nodeIdEncoded = nodesUtils.encodeNodeId(requestingNodeId); const params = headerMessage.params; - if (params == null || !utils.isObject(params)) utils.never(); + if (params == null || !utils.isObject(params)) { + utils.never('params must be defined and an object'); + } if (!('nameOrId' in params) || typeof params.nameOrId != 'string') { - utils.never(); + utils.never('nameOrId must be defined and a string'); } if (!('vaultAction' in params) || typeof params.vaultAction != 'string') { - utils.never(); + utils.never('vaultAction must be defined and a string'); } const nameOrId = params.nameOrId; const actionType = vaultsUtils.parseVaultAction(params.vaultAction); diff --git a/src/nodes/utils.ts b/src/nodes/utils.ts index 68f192002..a7d382aad 100644 --- a/src/nodes/utils.ts +++ b/src/nodes/utils.ts @@ -401,11 +401,13 @@ function parseRemoteCertsChain(remoteCertChain: Array) { const cert = keysUtils.certFromPEM( quicUtils.derToPEM(der) as CertificatePEM, ); - if (cert == null) utils.never(); + if (cert == null) { + utils.never('failed to parse certificate from cert chain'); + } return cert; }); const nodeId = keysUtils.certNodeId(certChain[0]); - if (nodeId == null) utils.never(); + if (nodeId == null) utils.never('failed to get NodeId from certificate'); return { nodeId, certChain }; } diff --git a/src/notifications/NotificationsManager.ts b/src/notifications/NotificationsManager.ts index cdd244dc8..c3ea4684a 100644 --- a/src/notifications/NotificationsManager.ts +++ b/src/notifications/NotificationsManager.ts @@ -497,7 +497,9 @@ class NotificationsManager { for await (const id of notificationIds) { const notification = await this.readOutboxNotificationById(id, tran); - if (notification == null) never(); + if (notification == null) { + never('failed to read existing outbox notification'); + } yield notification; } } @@ -695,7 +697,9 @@ class NotificationsManager { }); for await (const id of notificationIds) { const notification = await this.readInboxNotificationById(id, tran); - if (notification == null) never(); + if (notification == null) { + never('failed to read existing inbox notification'); + } yield notification; } } diff --git a/src/notifications/utils.ts b/src/notifications/utils.ts index cab46f956..8c94eb1b2 100644 --- a/src/notifications/utils.ts +++ b/src/notifications/utils.ts @@ -58,7 +58,9 @@ async function verifyAndDecodeNotif( const token = Token.fromEncoded(JSON.parse(signedNotification)); assertNotification(token.payload); const issuerNodeId = nodesUtils.decodeNodeId(token.payload.iss); - if (issuerNodeId == null) never(); + if (issuerNodeId == null) { + never(`failed to decode issuer NodeId "${token.payload.iss}"`); + } const issuerPublicKey = keysUtils.publicKeyFromNodeId(issuerNodeId); if (!token.verifyWithPublicKey(issuerPublicKey)) { throw new notificationsErrors.ErrorNotificationsVerificationFailed(); diff --git a/src/vaults/VaultInternal.ts b/src/vaults/VaultInternal.ts index 362b41d43..7d5a414c0 100644 --- a/src/vaults/VaultInternal.ts +++ b/src/vaults/VaultInternal.ts @@ -819,16 +819,16 @@ class VaultInternal { const result = vaultsGitInfoGetStream.meta?.result; if (result == null || !utils.isObject(result)) { - utils.never('`result` must be a defined object'); + utils.never('"result" must be a defined object'); } if (!('vaultName' in result) || typeof result.vaultName !== 'string') { - utils.never('`vaultName` must be defined and a string'); + utils.never('"vaultName" must be defined and a string'); } if ( !('vaultIdEncoded' in result) || typeof result.vaultIdEncoded !== 'string' ) { - utils.never('`vaultIdEncoded` must be defined and a string'); + utils.never('"vaultIdEncoded" must be defined and a string'); } const vaultName = result.vaultName; const remoteVaultId = ids.parseVaultId(result.vaultIdEncoded); @@ -850,30 +850,33 @@ class VaultInternal { headers: POJO; body: Array; }) { - if (method === 'GET') { - // Send back the GET request info response - return { - url: url, - method: method, - body: vaultsGitInfoGetStream.readable, - headers: headers, - statusCode: 200, - statusMessage: 'OK', - }; - } else if (method === 'POST') { - const writer = vaultsGitPackGetStream.writable.getWriter(); - await writer.write(body[0]); - await writer.close(); - return { - url: url, - method: method, - body: vaultsGitPackGetStream.readable, - headers: headers, - statusCode: 200, - statusMessage: 'OK', - }; - } else { - utils.never(); + switch (method) { + case 'GET': { + // Send back the GET request info response + return { + url: url, + method: method, + body: vaultsGitInfoGetStream.readable, + headers: headers, + statusCode: 200, + statusMessage: 'OK', + }; + } + case 'POST': { + const writer = vaultsGitPackGetStream.writable.getWriter(); + await writer.write(body[0]); + await writer.close(); + return { + url: url, + method: method, + body: vaultsGitPackGetStream.readable, + headers: headers, + statusCode: 200, + statusMessage: 'OK', + }; + } + default: + utils.never(`method must be "GET" or "POST" got "${method}"`); } }, vaultName, diff --git a/tests/gestalts/utils.ts b/tests/gestalts/utils.ts index 7f10f721e..78df7ad93 100644 --- a/tests/gestalts/utils.ts +++ b/tests/gestalts/utils.ts @@ -238,7 +238,7 @@ class UnsetVertexCommand implements GestaltGraphCommand { for (const vertex of vertices) { randomVertex2 = vertex; } - if (randomVertex2 == null) never(); + if (randomVertex2 == null) never('randomVertex2 must be defined'); const gestalt2ModelNew = getGestaltFromModel( model, gestaltsUtils.decodeGestaltId(randomVertex2)!, diff --git a/tests/git/utils.ts b/tests/git/utils.ts index 8652133c3..8205ebb90 100644 --- a/tests/git/utils.ts +++ b/tests/git/utils.ts @@ -123,7 +123,10 @@ function generateGitNegotiationLine(data: NegotiationTestData, rest: Buffer) { // Generate an empty buffer to simulate the stream running out of data to process return Buffer.alloc(0); default: - utils.never(); + // @ts-ignore: if we're here then data isn't the type we expect + utils.never( + `data.type must be "want", "have", "SEPARATOR", "done", "none", got "${data.type}"`, + ); } } @@ -151,39 +154,42 @@ function request({ body: Array; }) => { // Console.log('body', body.map(v => v.toString())) - if (method === 'GET') { - // Send back the GET request info response - const advertiseRefGen = gitHttp.advertiseRefGenerator({ - efs, - dir, - gitDir, - }); + switch (method) { + case 'GET': { + // Send back the GET request info response + const advertiseRefGen = gitHttp.advertiseRefGenerator({ + efs, + dir, + gitDir, + }); - return { - url: url, - method: method, - body: advertiseRefGen, - headers: headers, - statusCode: 200, - statusMessage: 'OK', - }; - } else if (method === 'POST') { - const packGen = gitHttp.generatePackRequest({ - efs, - dir, - gitDir, - body, - }); - return { - url: url, - method: method, - body: packGen, - headers: headers, - statusCode: 200, - statusMessage: 'OK', - }; - } else { - utils.never(); + return { + url: url, + method: method, + body: advertiseRefGen, + headers: headers, + statusCode: 200, + statusMessage: 'OK', + }; + } + case 'POST': { + const packGen = gitHttp.generatePackRequest({ + efs, + dir, + gitDir, + body, + }); + return { + url: url, + method: method, + body: packGen, + headers: headers, + statusCode: 200, + statusMessage: 'OK', + }; + } + default: + utils.never(`method must be "GET" or "POST" got "${method}"`); } }; } diff --git a/tests/nodes/utils.test.ts b/tests/nodes/utils.test.ts index ac6be9a91..2ebaaa03c 100644 --- a/tests/nodes/utils.test.ts +++ b/tests/nodes/utils.test.ts @@ -427,7 +427,9 @@ describe('nodes/utils', () => { cert.certChainPem.map((v) => wsUtils.pemToDER(v)), ); expect(result.result).toBe('fail'); - if (result.result !== 'fail') utils.never(); + if (result.result !== 'fail') { + utils.never('result.result should be "fail"'); + } expect(result.value).toBe(CryptoError.CertificateExpired); }); }); diff --git a/tests/tasks/TaskManager.test.ts b/tests/tasks/TaskManager.test.ts index 7d8a6c8d9..e48b6e6ed 100644 --- a/tests/tasks/TaskManager.test.ts +++ b/tests/tasks/TaskManager.test.ts @@ -927,7 +927,7 @@ describe(TaskManager.name, () => { // Task should be updated const oldTask = await taskManager.getTask(task1.id); - if (oldTask == null) utils.never(); + if (oldTask == null) utils.never('failed to get old task'); expect(oldTask.id.equals(task1.id)).toBeTrue(); expect(oldTask.handlerId).toEqual(handlerId2); expect(oldTask.delay).toBe(0); @@ -983,7 +983,7 @@ describe(TaskManager.name, () => { ).rejects.toThrow(tasksErrors.ErrorTaskRunning); // Task has not been updated const oldTask = await taskManager.getTask(task1.id); - if (oldTask == null) utils.never(); + if (oldTask == null) utils.never('failed to get old task'); expect(oldTask.delay).toBe(0); expect(oldTask.parameters).toEqual([]); await taskManager.stop(); @@ -1019,7 +1019,7 @@ describe(TaskManager.name, () => { }); // Task should be updated const newTask = await taskManager.getTask(task1.id); - if (newTask == null) utils.never(); + if (newTask == null) utils.never('failed to get new task'); expect(newTask.delay).toBe(0); expect(newTask.parameters).toEqual([1]); // Task should resolve with new parameter From 3bbf42eedb61bf308bc2db64a8b3c4cba742c8fb Mon Sep 17 00:00:00 2001 From: Brian Botha Date: Tue, 15 Oct 2024 19:30:00 +1100 Subject: [PATCH 3/3] feat: adding clearer error when a notification is rejected due to permissions --- src/notifications/NotificationsManager.ts | 19 +++++++++++++++++-- src/notifications/errors.ts | 6 ++++++ .../NotificationsManager.test.ts | 16 +++++++++------- 3 files changed, 32 insertions(+), 9 deletions(-) diff --git a/src/notifications/NotificationsManager.ts b/src/notifications/NotificationsManager.ts index c3ea4684a..e7620b7c1 100644 --- a/src/notifications/NotificationsManager.ts +++ b/src/notifications/NotificationsManager.ts @@ -26,6 +26,7 @@ import * as notificationsUtils from './utils'; import * as notificationsErrors from './errors'; import * as notificationsEvents from './events'; import config from '../config'; +import { ErrorPolykeyRemote } from '../network/errors'; import * as nodesUtils from '../nodes/utils'; import { never } from '../utils/utils'; @@ -385,8 +386,22 @@ class NotificationsManager { tran, ); const sendP = (async () => { - while (pendingTask != null) { - pendingTask = await pendingTask.promise(); + try { + while (pendingTask != null) { + pendingTask = await pendingTask.promise(); + } + } catch (e) { + if ( + e instanceof ErrorPolykeyRemote && + e.cause instanceof + notificationsErrors.ErrorNotificationsPermissionsNotFound + ) { + throw new notificationsErrors.ErrorNotificationsNotificationRejected( + undefined, + { cause: e }, + ); + } + throw e; } })(); sendP.catch(() => {}); diff --git a/src/notifications/errors.ts b/src/notifications/errors.ts index be9b40243..d4e66b213 100644 --- a/src/notifications/errors.ts +++ b/src/notifications/errors.ts @@ -23,6 +23,11 @@ class ErrorNotificationsPermissionsNotFound extends ErrorNotifications { exitCode = sysexits.NOUSER; } +class ErrorNotificationsNotificationRejected extends ErrorNotifications { + static description = 'Notification was rejected due to lack of permissions'; + exitCode = sysexits.NOPERM; +} + class ErrorNotificationsDb extends ErrorNotifications { static description = 'Database consistency error'; exitCode = sysexits.IOERR; @@ -79,6 +84,7 @@ export { ErrorNotificationsNotRunning, ErrorNotificationsDestroyed, ErrorNotificationsPermissionsNotFound, + ErrorNotificationsNotificationRejected, ErrorNotificationsDb, ErrorNotificationsParse, ErrorNotificationsInvalidType, diff --git a/tests/notifications/NotificationsManager.test.ts b/tests/notifications/NotificationsManager.test.ts index bf66d5908..363adee2d 100644 --- a/tests/notifications/NotificationsManager.test.ts +++ b/tests/notifications/NotificationsManager.test.ts @@ -345,8 +345,7 @@ describe('NotificationsManager', () => { pull: null, } as VaultActions, }; - - await testUtils.expectRemoteError( + await expect( notificationsManager .sendNotification({ nodeId: receiver.keyRing.getNodeId(), @@ -354,9 +353,10 @@ describe('NotificationsManager', () => { retries: 0, }) .then((value) => value.sendP), - notificationsErrors.ErrorNotificationsPermissionsNotFound, + ).rejects.toThrow( + notificationsErrors.ErrorNotificationsNotificationRejected, ); - await testUtils.expectRemoteError( + await expect( notificationsManager .sendNotification({ nodeId: receiver.keyRing.getNodeId(), @@ -364,9 +364,10 @@ describe('NotificationsManager', () => { retries: 0, }) .then((value) => value.sendP), - notificationsErrors.ErrorNotificationsPermissionsNotFound, + ).rejects.toThrow( + notificationsErrors.ErrorNotificationsNotificationRejected, ); - await testUtils.expectRemoteError( + await expect( notificationsManager .sendNotification({ nodeId: receiver.keyRing.getNodeId(), @@ -374,7 +375,8 @@ describe('NotificationsManager', () => { retries: 0, }) .then((value) => value.sendP), - notificationsErrors.ErrorNotificationsPermissionsNotFound, + ).rejects.toThrow( + notificationsErrors.ErrorNotificationsNotificationRejected, ); await expect( notificationsManager