From ef6b8bc8095c82d5651c392a689a392f4c4f23cb Mon Sep 17 00:00:00 2001 From: Amit Padmani <91456036+nbAmit@users.noreply.github.com> Date: Tue, 21 Dec 2021 23:47:23 +0530 Subject: [PATCH] refactor: resolve feedback for problem report (#584) Signed-off-by: Amit Padmani --- packages/core/src/agent/MessageReceiver.ts | 8 ++--- .../signature/SignatureDecoratorUtils.ts | 6 ++-- .../common/messages/CommonMessageType.ts | 1 - .../connections/models/ConnectionState.ts | 1 - .../repository/ConnectionRecord.ts | 6 ++-- .../connections/services/ConnectionService.ts | 25 +++++++++++++--- .../modules/credentials/CredentialState.ts | 1 - .../__tests__/CredentialService.test.ts | 26 ++-------------- .../repository/CredentialRecord.ts | 6 ++-- .../credentials/services/CredentialService.ts | 8 +++-- .../core/src/modules/problem-reports/index.ts | 1 + .../messages/ProblemReportMessage.ts | 3 +- .../models/ProblemReportReason.ts | 3 ++ .../modules/problem-reports/models/index.ts | 1 + .../core/src/modules/proofs/ProofState.ts | 1 - .../core/src/modules/proofs/ProofsModule.ts | 2 +- .../proofs/__tests__/ProofService.test.ts | 30 +++---------------- .../errors/PresentationProblemReportReason.ts | 2 +- .../modules/proofs/repository/ProofRecord.ts | 6 ++-- .../modules/proofs/services/ProofService.ts | 18 ++++++----- 20 files changed, 63 insertions(+), 92 deletions(-) create mode 100644 packages/core/src/modules/problem-reports/models/ProblemReportReason.ts create mode 100644 packages/core/src/modules/problem-reports/models/index.ts diff --git a/packages/core/src/agent/MessageReceiver.ts b/packages/core/src/agent/MessageReceiver.ts index 7e49e16a4f..a9e07cbf95 100644 --- a/packages/core/src/agent/MessageReceiver.ts +++ b/packages/core/src/agent/MessageReceiver.ts @@ -9,12 +9,11 @@ import { Lifecycle, scoped } from 'tsyringe' import { AriesFrameworkError } from '../error' import { ConnectionService } from '../modules/connections/services/ConnectionService' -import { ProblemReportError, ProblemReportMessage } from '../modules/problem-reports' +import { ProblemReportError, ProblemReportMessage, ProblemReportReason } from '../modules/problem-reports' import { JsonTransformer } from '../utils/JsonTransformer' import { MessageValidator } from '../utils/MessageValidator' import { replaceLegacyDidSovPrefixOnMessage } from '../utils/messageType' -import { CommonMessageType } from './../modules/common/messages/CommonMessageType' import { AgentConfig } from './AgentConfig' import { Dispatcher } from './Dispatcher' import { EnvelopeService } from './EnvelopeService' @@ -23,9 +22,6 @@ import { TransportService } from './TransportService' import { createOutboundMessage } from './helpers' import { InboundMessageContext } from './models/InboundMessageContext' -export enum ProblemReportReason { - MessageParseFailure = 'message-parse-failure', -} @scoped(Lifecycle.ContainerScoped) export class MessageReceiver { private config: AgentConfig @@ -226,7 +222,7 @@ export class MessageReceiver { connection: ConnectionRecord, plaintextMessage: PlaintextMessage ) { - if (plaintextMessage['@type'] === CommonMessageType.ProblemReport) { + if (plaintextMessage['@type'] === ProblemReportMessage.type) { throw new AriesFrameworkError(message) } const problemReportMessage = new ProblemReportMessage({ diff --git a/packages/core/src/decorators/signature/SignatureDecoratorUtils.ts b/packages/core/src/decorators/signature/SignatureDecoratorUtils.ts index 54a603b10e..8c18f55855 100644 --- a/packages/core/src/decorators/signature/SignatureDecoratorUtils.ts +++ b/packages/core/src/decorators/signature/SignatureDecoratorUtils.ts @@ -1,6 +1,6 @@ import type { Wallet } from '../../wallet/Wallet' -import { ConnectionProblemReportError, ConnectionProblemReportReason } from '../../modules/connections/errors' +import { AriesFrameworkError } from '../../error' import { BufferEncoder } from '../../utils/BufferEncoder' import { JsonEncoder } from '../../utils/JsonEncoder' import { Buffer } from '../../utils/buffer' @@ -29,9 +29,7 @@ export async function unpackAndVerifySignatureDecorator( const isValid = await wallet.verify(signerVerkey, signedData, signature) if (!isValid) { - throw new ConnectionProblemReportError('Signature is not valid', { - problemCode: ConnectionProblemReportReason.RequestProcessingError, - }) + throw new AriesFrameworkError('Signature is not valid') } // TODO: return Connection instance instead of raw json diff --git a/packages/core/src/modules/common/messages/CommonMessageType.ts b/packages/core/src/modules/common/messages/CommonMessageType.ts index d0b36d2d37..0aba02a8dc 100644 --- a/packages/core/src/modules/common/messages/CommonMessageType.ts +++ b/packages/core/src/modules/common/messages/CommonMessageType.ts @@ -1,4 +1,3 @@ export enum CommonMessageType { Ack = 'https://didcomm.org/notification/1.0/ack', - ProblemReport = 'https://didcomm.org/notification/1.0/problem-report', } diff --git a/packages/core/src/modules/connections/models/ConnectionState.ts b/packages/core/src/modules/connections/models/ConnectionState.ts index 5b8c79ca48..15071c2623 100644 --- a/packages/core/src/modules/connections/models/ConnectionState.ts +++ b/packages/core/src/modules/connections/models/ConnectionState.ts @@ -10,5 +10,4 @@ export enum ConnectionState { Requested = 'requested', Responded = 'responded', Complete = 'complete', - None = 'none', } diff --git a/packages/core/src/modules/connections/repository/ConnectionRecord.ts b/packages/core/src/modules/connections/repository/ConnectionRecord.ts index da0187197f..4f0ed7079b 100644 --- a/packages/core/src/modules/connections/repository/ConnectionRecord.ts +++ b/packages/core/src/modules/connections/repository/ConnectionRecord.ts @@ -29,7 +29,7 @@ export interface ConnectionRecordProps { imageUrl?: string multiUseInvitation: boolean mediatorId?: string - errorMsg?: string + errorMessage?: string } export type CustomConnectionTags = TagsBase @@ -69,7 +69,7 @@ export class ConnectionRecord public threadId?: string public mediatorId?: string - public errorMsg?: string + public errorMessage?: string public static readonly type = 'ConnectionRecord' public readonly type = ConnectionRecord.type @@ -96,7 +96,7 @@ export class ConnectionRecord this.imageUrl = props.imageUrl this.multiUseInvitation = props.multiUseInvitation this.mediatorId = props.mediatorId - this.errorMsg = props.errorMsg + this.errorMessage = props.errorMessage } } diff --git a/packages/core/src/modules/connections/services/ConnectionService.ts b/packages/core/src/modules/connections/services/ConnectionService.ts index cf284df513..8f0aca39bd 100644 --- a/packages/core/src/modules/connections/services/ConnectionService.ts +++ b/packages/core/src/modules/connections/services/ConnectionService.ts @@ -323,7 +323,16 @@ export class ConnectionService { connectionRecord.assertState(ConnectionState.Requested) connectionRecord.assertRole(ConnectionRole.Invitee) - const connectionJson = await unpackAndVerifySignatureDecorator(message.connectionSig, this.wallet) + let connectionJson = null + try { + connectionJson = await unpackAndVerifySignatureDecorator(message.connectionSig, this.wallet) + } catch (error) { + if (error instanceof AriesFrameworkError) { + throw new ConnectionProblemReportError(error.message, { + problemCode: ConnectionProblemReportReason.RequestProcessingError, + }) + } + } const connection = JsonTransformer.fromJSON(connectionJson, Connection) await MessageValidator.validate(connection) @@ -417,7 +426,7 @@ export class ConnectionService { public async processProblemReport( messageContext: InboundMessageContext ): Promise { - const { message: connectionProblemReportMessage, recipientVerkey } = messageContext + const { message: connectionProblemReportMessage, recipientVerkey, senderVerkey } = messageContext this.logger.debug(`Processing connection problem report for verkey ${recipientVerkey}`) @@ -433,8 +442,12 @@ export class ConnectionService { ) } - connectionRecord.errorMsg = `${connectionProblemReportMessage.description.code} : ${connectionProblemReportMessage.description.en}` - await this.updateState(connectionRecord, ConnectionState.None) + if (connectionRecord.theirKey && connectionRecord.theirKey !== senderVerkey) { + throw new AriesFrameworkError("Sender verkey doesn't match verkey of connection record") + } + + connectionRecord.errorMessage = `${connectionProblemReportMessage.description.code} : ${connectionProblemReportMessage.description.en}` + await this.update(connectionRecord) return connectionRecord } @@ -529,6 +542,10 @@ export class ConnectionService { }) } + public update(connectionRecord: ConnectionRecord) { + return this.connectionRepository.update(connectionRecord) + } + /** * Retrieve all connections records * diff --git a/packages/core/src/modules/credentials/CredentialState.ts b/packages/core/src/modules/credentials/CredentialState.ts index dc306efb57..9e6c4c6000 100644 --- a/packages/core/src/modules/credentials/CredentialState.ts +++ b/packages/core/src/modules/credentials/CredentialState.ts @@ -14,5 +14,4 @@ export enum CredentialState { CredentialIssued = 'credential-issued', CredentialReceived = 'credential-received', Done = 'done', - None = 'none', } diff --git a/packages/core/src/modules/credentials/__tests__/CredentialService.test.ts b/packages/core/src/modules/credentials/__tests__/CredentialService.test.ts index 411056df7e..4979c351c2 100644 --- a/packages/core/src/modules/credentials/__tests__/CredentialService.test.ts +++ b/packages/core/src/modules/credentials/__tests__/CredentialService.test.ts @@ -981,7 +981,7 @@ describe('CredentialService', () => { }) }) - test(`updates state to ${CredentialState.None} and returns credential record`, async () => { + test(`updates problem report error message and returns credential record`, async () => { const repositoryUpdateSpy = jest.spyOn(credentialRepository, 'update') // given @@ -992,7 +992,7 @@ describe('CredentialService', () => { // then const expectedCredentialRecord = { - state: CredentialState.None, + errorMessage: 'issuance-abandoned: Indy error', } expect(credentialRepository.getSingleByQuery).toHaveBeenNthCalledWith(1, { threadId: 'somethreadid', @@ -1003,28 +1003,6 @@ describe('CredentialService', () => { expect(updatedCredentialRecord).toMatchObject(expectedCredentialRecord) expect(returnedCredentialRecord).toMatchObject(expectedCredentialRecord) }) - - test(`emits stateChange event from ${CredentialState.OfferReceived} to ${CredentialState.None}`, async () => { - const eventListenerMock = jest.fn() - eventEmitter.on(CredentialEventTypes.CredentialStateChanged, eventListenerMock) - - // given - mockFunction(credentialRepository.getSingleByQuery).mockReturnValue(Promise.resolve(credential)) - - // when - await credentialService.processProblemReport(messageContext) - - // then - expect(eventListenerMock).toHaveBeenCalledWith({ - type: 'CredentialStateChanged', - payload: { - previousState: CredentialState.OfferReceived, - credentialRecord: expect.objectContaining({ - state: CredentialState.None, - }), - }, - }) - }) }) describe('repository methods', () => { diff --git a/packages/core/src/modules/credentials/repository/CredentialRecord.ts b/packages/core/src/modules/credentials/repository/CredentialRecord.ts index ee35a523bd..68d0c31d81 100644 --- a/packages/core/src/modules/credentials/repository/CredentialRecord.ts +++ b/packages/core/src/modules/credentials/repository/CredentialRecord.ts @@ -33,7 +33,7 @@ export interface CredentialRecordProps { credentialAttributes?: CredentialPreviewAttribute[] autoAcceptCredential?: AutoAcceptCredential linkedAttachments?: Attachment[] - errorMsg?: string + errorMessage?: string } export type CustomCredentialTags = TagsBase @@ -50,7 +50,7 @@ export class CredentialRecord extends BaseRecord ProposeCredentialMessage) @@ -90,7 +90,7 @@ export class CredentialRecord extends BaseRecord ): Promise { - const { message: credentialProblemReportMessage, connection } = messageContext + const { message: credentialProblemReportMessage } = messageContext + + const connection = messageContext.assertReadyConnection() this.logger.debug(`Processing problem report with id ${credentialProblemReportMessage.id}`) @@ -743,8 +745,8 @@ export class CredentialService { ) // Update record - credentialRecord.errorMsg = `${credentialProblemReportMessage.description.code}: ${credentialProblemReportMessage.description.en}` - await this.updateState(credentialRecord, CredentialState.None) + credentialRecord.errorMessage = `${credentialProblemReportMessage.description.code}: ${credentialProblemReportMessage.description.en}` + await this.update(credentialRecord) return credentialRecord } diff --git a/packages/core/src/modules/problem-reports/index.ts b/packages/core/src/modules/problem-reports/index.ts index 52cb42ded3..479c831166 100644 --- a/packages/core/src/modules/problem-reports/index.ts +++ b/packages/core/src/modules/problem-reports/index.ts @@ -1,2 +1,3 @@ export * from './errors' export * from './messages' +export * from './models' diff --git a/packages/core/src/modules/problem-reports/messages/ProblemReportMessage.ts b/packages/core/src/modules/problem-reports/messages/ProblemReportMessage.ts index db62673913..8191b646d7 100644 --- a/packages/core/src/modules/problem-reports/messages/ProblemReportMessage.ts +++ b/packages/core/src/modules/problem-reports/messages/ProblemReportMessage.ts @@ -3,7 +3,6 @@ import { Expose } from 'class-transformer' import { Equals, IsEnum, IsOptional, IsString } from 'class-validator' import { AgentMessage } from '../../../agent/AgentMessage' -import { CommonMessageType } from '../../common/messages/CommonMessageType' export enum WhoRetriesStatus { You = 'YOU', @@ -80,7 +79,7 @@ export class ProblemReportMessage extends AgentMessage { @Equals(ProblemReportMessage.type) public readonly type: string = ProblemReportMessage.type - public static readonly type: string = CommonMessageType.ProblemReport + public static readonly type: string = 'https://didcomm.org/notification/1.0/problem-report' public description!: DescriptionOptions diff --git a/packages/core/src/modules/problem-reports/models/ProblemReportReason.ts b/packages/core/src/modules/problem-reports/models/ProblemReportReason.ts new file mode 100644 index 0000000000..6f85917c1a --- /dev/null +++ b/packages/core/src/modules/problem-reports/models/ProblemReportReason.ts @@ -0,0 +1,3 @@ +export enum ProblemReportReason { + MessageParseFailure = 'message-parse-failure', +} diff --git a/packages/core/src/modules/problem-reports/models/index.ts b/packages/core/src/modules/problem-reports/models/index.ts new file mode 100644 index 0000000000..1cbfb94d73 --- /dev/null +++ b/packages/core/src/modules/problem-reports/models/index.ts @@ -0,0 +1 @@ +export * from './ProblemReportReason' diff --git a/packages/core/src/modules/proofs/ProofState.ts b/packages/core/src/modules/proofs/ProofState.ts index 95c32dffaa..73869e80aa 100644 --- a/packages/core/src/modules/proofs/ProofState.ts +++ b/packages/core/src/modules/proofs/ProofState.ts @@ -12,5 +12,4 @@ export enum ProofState { PresentationReceived = 'presentation-received', Declined = 'declined', Done = 'done', - None = 'none', } diff --git a/packages/core/src/modules/proofs/ProofsModule.ts b/packages/core/src/modules/proofs/ProofsModule.ts index 2a3b0d395f..f7ffdb56b6 100644 --- a/packages/core/src/modules/proofs/ProofsModule.ts +++ b/packages/core/src/modules/proofs/ProofsModule.ts @@ -386,7 +386,7 @@ export class ProofsModule { const presentationProblemReportMessage = new PresentationProblemReportMessage({ description: { en: message, - code: PresentationProblemReportReason.abandoned, + code: PresentationProblemReportReason.Abandoned, }, }) presentationProblemReportMessage.setThread({ diff --git a/packages/core/src/modules/proofs/__tests__/ProofService.test.ts b/packages/core/src/modules/proofs/__tests__/ProofService.test.ts index e7b8f02ea6..4143fcc0dd 100644 --- a/packages/core/src/modules/proofs/__tests__/ProofService.test.ts +++ b/packages/core/src/modules/proofs/__tests__/ProofService.test.ts @@ -196,7 +196,7 @@ describe('ProofService', () => { const presentationProblemReportMessage = await new PresentationProblemReportMessage({ description: { en: 'Indy error', - code: PresentationProblemReportReason.abandoned, + code: PresentationProblemReportReason.Abandoned, }, }) @@ -224,7 +224,7 @@ describe('ProofService', () => { const presentationProblemReportMessage = new PresentationProblemReportMessage({ description: { en: 'Indy error', - code: PresentationProblemReportReason.abandoned, + code: PresentationProblemReportReason.Abandoned, }, }) presentationProblemReportMessage.setThread({ threadId: 'somethreadid' }) @@ -233,7 +233,7 @@ describe('ProofService', () => { }) }) - test(`updates state to ${ProofState.None} and returns proof record`, async () => { + test(`updates problem report error message and returns proof record`, async () => { const repositoryUpdateSpy = jest.spyOn(proofRepository, 'update') // given @@ -244,7 +244,7 @@ describe('ProofService', () => { // then const expectedCredentialRecord = { - state: ProofState.None, + errorMessage: 'abandoned: Indy error', } expect(proofRepository.getSingleByQuery).toHaveBeenNthCalledWith(1, { threadId: 'somethreadid', @@ -255,27 +255,5 @@ describe('ProofService', () => { expect(updatedCredentialRecord).toMatchObject(expectedCredentialRecord) expect(returnedCredentialRecord).toMatchObject(expectedCredentialRecord) }) - - test(`emits stateChange event from ${ProofState.RequestReceived} to ${ProofState.None}`, async () => { - const eventListenerMock = jest.fn() - eventEmitter.on(ProofEventTypes.ProofStateChanged, eventListenerMock) - - // given - mockFunction(proofRepository.getSingleByQuery).mockReturnValue(Promise.resolve(proof)) - - // when - await proofService.processProblemReport(messageContext) - - // then - expect(eventListenerMock).toHaveBeenCalledWith({ - type: 'ProofStateChanged', - payload: { - previousState: ProofState.RequestReceived, - proofRecord: expect.objectContaining({ - state: ProofState.None, - }), - }, - }) - }) }) }) diff --git a/packages/core/src/modules/proofs/errors/PresentationProblemReportReason.ts b/packages/core/src/modules/proofs/errors/PresentationProblemReportReason.ts index c216ee6776..0fc1676dcc 100644 --- a/packages/core/src/modules/proofs/errors/PresentationProblemReportReason.ts +++ b/packages/core/src/modules/proofs/errors/PresentationProblemReportReason.ts @@ -4,5 +4,5 @@ * @see https://github.com/hyperledger/aries-rfcs/blob/main/features/0037-present-proof/README.md */ export enum PresentationProblemReportReason { - abandoned = 'abandoned', + Abandoned = 'abandoned', } diff --git a/packages/core/src/modules/proofs/repository/ProofRecord.ts b/packages/core/src/modules/proofs/repository/ProofRecord.ts index 73efee8791..bf4faa5435 100644 --- a/packages/core/src/modules/proofs/repository/ProofRecord.ts +++ b/packages/core/src/modules/proofs/repository/ProofRecord.ts @@ -20,7 +20,7 @@ export interface ProofRecordProps { presentationId?: string tags?: CustomProofTags autoAcceptProof?: AutoAcceptProof - errorMsg?: string + errorMessage?: string // message data proposalMessage?: ProposePresentationMessage @@ -42,7 +42,7 @@ export class ProofRecord extends BaseRecord { public presentationId?: string public state!: ProofState public autoAcceptProof?: AutoAcceptProof - public errorMsg?: string + public errorMessage?: string // message data @Type(() => ProposePresentationMessage) @@ -71,7 +71,7 @@ export class ProofRecord extends BaseRecord { this.presentationId = props.presentationId this.autoAcceptProof = props.autoAcceptProof this._tags = props.tags ?? {} - this.errorMsg = props.errorMsg + this.errorMessage = props.errorMessage } } diff --git a/packages/core/src/modules/proofs/services/ProofService.ts b/packages/core/src/modules/proofs/services/ProofService.ts index df131aab10..95477939e9 100644 --- a/packages/core/src/modules/proofs/services/ProofService.ts +++ b/packages/core/src/modules/proofs/services/ProofService.ts @@ -355,7 +355,7 @@ export class ProofService { if (!proofRequest) { throw new PresentationProblemReportError( `Missing required base64 or json encoded attachment data for presentation request with thread id ${proofRequestMessage.threadId}`, - { problemCode: PresentationProblemReportReason.abandoned } + { problemCode: PresentationProblemReportReason.Abandoned } ) } await validateOrReject(proofRequest) @@ -424,7 +424,7 @@ export class ProofService { if (!indyProofRequest) { throw new PresentationProblemReportError( `Missing required base64 or json encoded attachment data for presentation with thread id ${proofRecord.threadId}`, - { problemCode: PresentationProblemReportReason.abandoned } + { problemCode: PresentationProblemReportReason.Abandoned } ) } @@ -491,14 +491,14 @@ export class ProofService { if (!indyProofJson) { throw new PresentationProblemReportError( `Missing required base64 or json encoded attachment data for presentation with thread id ${presentationMessage.threadId}`, - { problemCode: PresentationProblemReportReason.abandoned } + { problemCode: PresentationProblemReportReason.Abandoned } ) } if (!indyProofRequest) { throw new PresentationProblemReportError( `Missing required base64 or json encoded attachment data for presentation request with thread id ${presentationMessage.threadId}`, - { problemCode: PresentationProblemReportReason.abandoned } + { problemCode: PresentationProblemReportReason.Abandoned } ) } @@ -574,14 +574,16 @@ export class ProofService { public async processProblemReport( messageContext: InboundMessageContext ): Promise { - const { message: presentationProblemReportMessage, connection } = messageContext + const { message: presentationProblemReportMessage } = messageContext + + const connection = messageContext.assertReadyConnection() this.logger.debug(`Processing problem report with id ${presentationProblemReportMessage.id}`) const proofRecord = await this.getByThreadAndConnectionId(presentationProblemReportMessage.threadId, connection?.id) - proofRecord.errorMsg = `${presentationProblemReportMessage.description.code}: ${presentationProblemReportMessage.description.en}` - await this.updateState(proofRecord, ProofState.None) + proofRecord.errorMessage = `${presentationProblemReportMessage.description.code}: ${presentationProblemReportMessage.description.en}` + await this.update(proofRecord) return proofRecord } @@ -853,7 +855,7 @@ export class ProofService { `The encoded value for '${referent}' is invalid. ` + `Expected '${CredentialUtils.encode(attribute.raw)}'. ` + `Actual '${attribute.encoded}'`, - { problemCode: PresentationProblemReportReason.abandoned } + { problemCode: PresentationProblemReportReason.Abandoned } ) } }