-
Notifications
You must be signed in to change notification settings - Fork 239
feat: (main) decline request #2353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: (main) decline request #2353
Conversation
|
| // For routing the problem report message | ||
| const routing = | ||
| outOfBandRecord.reusable || outOfBandRecord.outOfBandInvitation.getInlineServices().length === 0 | ||
| ? await this.routingService.getRouting(this.agentContext) | ||
| : undefined | ||
|
|
||
| // Trigger creation of dids in the connectionRecord for sending the problem report | ||
| if (connectionRecord.protocol === HandshakeProtocol.DidExchange) { | ||
| await this.didExchangeProtocol.createResponse( | ||
| this.agentContext, | ||
| connectionRecord, | ||
| outOfBandRecord, | ||
| routing | ||
| ) | ||
| } else { | ||
| throw new CredoError(`Connection protocol is ${connectionRecord.protocol} not didexchange`) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we should trigger the whole response creation logic just to decline the request? Since we don't send the response, we don't send the signed attachments from the response, and thus it won't be possible for the other party to verify the problem report came from the inviter.
The only way to do that is by wrapping the problem report using the same key that was included in the invitation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you able to give me any pointers on how to do that more efficiently? I assumed the creation of the outbound message would automatically wrap/sign the message with the correct keys.
| // Alice's agent reflects this in her OOB record and connection record | ||
| expect(aliceConnectionAfterDecline.state).toBe(DidExchangeState.Abandoned) | ||
| const aliceOobAfterDecline = await aliceAgent.modules.oob.findById(aliceOutOfBandRecord.id) | ||
| expect(aliceOobAfterDecline.state).toBe(OutOfBandState.Done) | ||
|
|
||
| // Bob should receive the problem report correctly | ||
| expect(bobSpy).toHaveBeenCalled() | ||
| const [bobMessage] = bobSpy.mock.calls[0] | ||
|
|
||
| expect(bobSpy2).toHaveBeenCalled() | ||
| const [spy2] = bobSpy2.mock.calls | ||
| // expect(bobConnectionSpy).toHaveBeenCalled() | ||
| // const [spyMessage] = bobConnectionSpy.mock.calls[0] | ||
|
|
||
| console.debug(aliceMessage) | ||
| console.log(bobMessage) | ||
| console.log(spy2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be clean up. I am curious, how is the decline request currently handled by Bob? Since it should not allow the problem report to be assocaited with the invitation/connection, since we can't verify whether the sender of the problem report is the inviter based on the current code in declineRequest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests were WIP, but I agree the problem report must contain something known to both Alice and Bob which would either be the invitationId from the OOB record, or the threadId from the connectionRecord. I set the threadId of the problem report in the declineRequest method to equal that of the connectionRecord.
TimoGlastra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR, i have some concerns on the security of this implementation
Linked to issue #2338
Have created a method in ConnectionsAPI.ts for gracefully declining an incoming connection request in the request-received state by sending a problem report to the requester and marking the connection as
abandonedon our side. The OOB invite state is also transitioned toDoneon our side if not reusable to prevent replay.Submitting for your review and insights