hyperledger / aries-vcx

aries-vcx is set of crates to work with DIDs, DID Documents, DIDComm, Verifiable Credentials and Hyperledger Aries.
https://didcomm.org
Apache License 2.0
125 stars 83 forks source link

Handle messages with an empty ~thread decorator #875

Open gmulhearn-anonyome opened 1 year ago

gmulhearn-anonyome commented 1 year ago

I've found testing against ACA-py 0.8.1 that some ACA-py messages (particularly a new cred offer) will include a ~thread decorator with no content: "~thread": {}:

{
    "@type": "did:sov:BzCbsNYhMrjHiqZDTUASHg;spec/issue-credential/1.0/offer-credential",
    "@id": "3fb15d06-28ff-438d-b6d9-f8771f08e844",
    "~thread": {},
    "offers~attach": [
      {
        "@id": "libindy-cred-offer-0",
        "mime-type": "application/json",
...

Aries-vcx message create will fail to deserialize this message, as if OfferCredentialDecorators has a thread, then it expects there to be a thid.

So I'm wondering; is this an ACA-py bug, or an aries_vcx bug?... The language of the threading RFC makes it difficult to know whether an ~thread with nothing inside it is "valid" (despite being meaningless/useless); https://github.com/hyperledger/aries-rfcs/tree/main/concepts/0008-message-id-and-threading.

And further question; if it is an ACA-py bug, should aries_vcx build some tolerance for the bug anyway?

swcurran commented 1 year ago

FYI @usingtechnology @Jsyro @andrewwhitehead @TelegramSam @dbluhm

Given the RFCs, is this a bug in ACA-Py? It should be easy to fix — adding a thid and setting it to the same value as @id, right?

usingtechnology commented 1 year ago

The RFC is ambiguous:

If the Thread object is defined and a thid is given, the Thread ID is the value given there. But if the Thread object is not defined in a message, the Thread ID is implicitly defined as the Message ID (@id) of the given message and that message is the first message of a new thread.

So we have the case where Thread is defined with and explicit not for what to do when thid is provided which would imply that there is another case where thid is not provided... But that is different than what to do if Thread is not provided. So... does that mean a Thread with no thid is treated the same as no Thread: @id is the implicit Thread ID?

If I look at aries-protocol-test-suite, it assumes that thid is @id and deals accordingly. It does not assume that ~thread has a thid. Reading that would mean an thread object without a thid is fine, the controller uses @id.

We can certainly populate thread.thid with @id but no guarantee other ARIES implementations will do the same. Probably a safer fix would be if thread is empty, then do not decorate.

swcurran commented 1 year ago

Thanks, @usingtechnology — let’s get an issue into ACA-Py to do as you say and remove the empty ~thid. That said, being defensive, Aries might want to accept that an empty thid could happen.

We can do a quick PR to aries-rfc as a clarification — don’t put it in, but understand that implementations might.

usingtechnology commented 1 year ago

Added issue 2259 to ACA-Py.

We will not send an empty ~thread object. However, still best if aries-vcx acts defensively and handles missingthid.

usingtechnology commented 1 year ago

Just a follow up PR 2261 is ready for review. The PR sets the thid value for credential offer. It appears this was always intended but set with an uninitialized value.

Looking over the existing ACA-Py code, it is very defensive and always checks for a ~thread and a thid - expects that a thread may not have a thid - so highly recommended that aries-vcx do the same.

swcurran commented 1 year ago

Just a follow up: ACA-Py PR 2261 is merged, on the main branch and will be in Release 0.8.2.