Ignore id property in credential preview sub message

[TimoGlastra]

This issue came to surface when testing interoperability between framework .NET and ACA-Py.

Framework .NET includes an @id property in the Credential Preview sub message. This is not in line with the RFC, but from what I understand, fields that are not mentioned in the RFCs should be ignored instead of rejected. So maybe ACA-Py should also allow it?

Corresponding .NET issue: https://github.com/hyperledger/aries-framework-dotnet/issues/117

2020-08-03 12:06:58,493 aries_cloudagent.messaging.models.base ERROR CredentialOffer message validation error:
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/aries_cloudagent/messaging/models/base.py", line 127, in deserialize
    return schema.loads(obj) if isinstance(obj, str) else schema.load(obj)
  File "/usr/local/lib/python3.7/site-packages/marshmallow/schema.py", line 723, in load
    data, many=many, partial=partial, unknown=unknown, postprocess=True
  File "/usr/local/lib/python3.7/site-packages/marshmallow/schema.py", line 904, in _do_load
    raise exc
marshmallow.exceptions.ValidationError: {'credential_preview': {'@id': ['Unknown field.']}}
2020-08-03 12:06:58,495 aries_cloudagent.core.dispatcher ERROR Message parsing failed: Error deserializing message: CredentialOffer schema validation failed, sending problem report
2020-08-03 12:06:58,519 aries_cloudagent.transport.outbound.manager ERROR >>> Posting error: http://192.168.65.3:9020; Re-queue failed message ...

[sklump]

Where do Aries RFC standards instruct implementations to ignore extra fields please? Aca-py is strict everywhere and introducing this novelty will invite a revamp of all sub-messages in all protocols to weaken the effect of having a standard at all.

[TimoGlastra]

Maybe I'm incorrect, that's just the general feeling I got from reading the RFCs. After searching trough the RFC repo I can't find it stated explicitly for all fields, only decorators and version mismatches.

I think this line in the decorators RFC threw me off:

1. As with all other aspects of DIDComm messages, unrecognized fields in decorators must be ignored.

[swcurran]

It's in the Protocols RFC in the Semver section - https://github.com/hyperledger/aries-rfcs/tree/master/concepts/0003-protocols#semver-rules-for-protocols

[sklump]

@swcurran what I get from that text is that it is OK to add new optional fields or to deprecate existing fields when bumping a minor version, not that implementations should ignore extra fields for an existing version.

I'm trying to figure out if we need to revamp our whole approach. I think it could mean small code changes with enormous semantic and testing implications if we have to accept what looks today like garbage and sift through it to find what we want. Hundreds of message types are in scope, but they all inherit from the same 2 or 3 ancestors ultimately.

[swcurran]

My understanding is that when @nrempel did the "minor/major" version handling, this was part of it.

But you raise a great point - should we accept extra fields if we know the claimed version of the message? If it is a higher minor version then clearly yes, but if it is one we know what should be done? My understanding from the discussions around the "protocols" RFC was yes, we should ignore those as well, but I'm not so sure.

[sklump]

I can do up a prototype and see what damage there is to mitigate. I may be overreacting.

update: prototyping this is a big undertaking. Marshmallow. I'm parking this effort for now.

[TimoGlastra]

Either way, I think we should update the protocols RFC to explicitly state whether extra fields MAY, SHOULD or MUST be ignored / accepted as this clearly creates ambiguity.

[swcurran]

Closing this. Clarifications have been made to RFC 0003 Protocols.