Merge pull request #3 from NB-MikeRichardson/rfc0453

refactor: resolve feedback for problem report (openwallet-foundation#584)

Author: NB-MikeRichardson

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({

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

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',
 }

packages/core/src/modules/connections/models/ConnectionState.ts

@@ -10,5 +10,4 @@ export enum ConnectionState {
   Requested = 'requested',
   Responded = 'responded',
   Complete = 'complete',
-  None = 'none',
 }

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
     }
   }
 

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<ConnectionProblemReportMessage>
   ): Promise<ConnectionRecord> {
-    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
    *

packages/core/src/modules/credentials/CredentialState.ts

@@ -14,5 +14,4 @@ export enum CredentialState {
   CredentialIssued = 'credential-issued',
   CredentialReceived = 'credential-received',
   Done = 'done',
-  None = 'none',
 }

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<CredentialStateChangedEvent>(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', () => {

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<DefaultCredentialTags, CustomCr
   public credentialId?: string
   public state!: CredentialState
   public autoAcceptCredential?: AutoAcceptCredential
-  public errorMsg?: string
+  public errorMessage?: string
 
   // message data
   @Type(() => ProposeCredentialMessage)
@@ -90,7 +90,7 @@ export class CredentialRecord extends BaseRecord<DefaultCredentialTags, CustomCr
       this.credentialAttributes = props.credentialAttributes
       this.autoAcceptCredential = props.autoAcceptCredential
       this.linkedAttachments = props.linkedAttachments
-      this.errorMsg = props.errorMsg
+      this.errorMessage = props.errorMessage
     }
   }
 

packages/core/src/modules/credentials/services/CredentialService.ts

@@ -733,7 +733,9 @@ export class CredentialService {
   public async processProblemReport(
     messageContext: InboundMessageContext<CredentialProblemReportMessage>
   ): Promise<CredentialRecord> {
-    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
   }
 

packages/core/src/modules/problem-reports/index.ts

@@ -1,2 +1,3 @@
 export * from './errors'
 export * from './messages'
+export * from './models'

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
 

packages/core/src/modules/problem-reports/models/ProblemReportReason.ts

@@ -0,0 +1,3 @@
+export enum ProblemReportReason {
+  MessageParseFailure = 'message-parse-failure',
+}

packages/core/src/modules/problem-reports/models/index.ts

@@ -0,0 +1 @@
+export * from './ProblemReportReason'

packages/core/src/modules/proofs/ProofState.ts

@@ -12,5 +12,4 @@ export enum ProofState {
   PresentationReceived = 'presentation-received',
   Declined = 'declined',
   Done = 'done',
-  None = 'none',
 }

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({

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<ProofStateChangedEvent>(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,
-          }),
-        },
-      })
-    })
   })
 })

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',
 }

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<DefaultProofTags, CustomProofTags> {
   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<DefaultProofTags, CustomProofTags> {
       this.presentationId = props.presentationId
       this.autoAcceptProof = props.autoAcceptProof
       this._tags = props.tags ?? {}
-      this.errorMsg = props.errorMsg
+      this.errorMessage = props.errorMessage
     }
   }