Skip to content
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

fix: deepcopy attach decorator data #1352

Merged

Conversation

TimoGlastra
Copy link
Contributor

@Moopli had an interop issue between AFGO and ACA-Py. It came down to ACA-Py modifying the json returned from an attachment decorator which would cause issues later on because the object was missing some expected properties. (see more info below)

To prevent such mistakes in the future it is now fixed by making a deep copy both when setting and retrieving json from an attachment decorator. I'm not 100% what the impact of this is on performance, and whether json.dumps and json.loads may be faster.

We could also fix it by making a copy in the json ld handler, but this won't fix the issue all together. @andrewwhitehead I'm interested to hear which approach you'd take.

TimoGlastra I found the bug
from here:
https://github.com/hyperledger/aries-cloudagent-python/blob/main/aries_cloudagent/protocols/issue_credential/v2_0/manager.py#L571
we end up calling ld_proof/handler LDProofCredFormatHandler.receive_credential, and when it gets here:
https://github.com/hyperledger/aries-cloudagent-python/blob/main/aries_cloudagent/protocols/issue_credential/v2_0/formats/ld_proof/handler.py#L493
it modifies the original attachment in the message, removing the proof member from the credential. So when the ld proof handler returns, V20CredManager stores the damaged message, causing the later error on deserialization.
The issue is, AttachDecorator returns json attachments by reference: https://github.com/hyperledger/aries-cloudagent-python/blob/main/aries_cloudagent/messaging/decorators/attach_decorator.py#L560
To fix this issue alone, LDProofCredFormatHandler.receive_credential could simply build a shallow copy of cred_dict
but I imagine that where one bug appears, others lurk in silence - is AttachDecorator` supposed to provide a mutable reference? Or should it return a deep copy of the dict?
the workaround I'm doing right now is to make the issued credential a b64 attachment instead

@codecov-commenter
Copy link

Codecov Report

Merging #1352 (a1a6b6e) into main (9541edf) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1352   +/-   ##
=======================================
  Coverage   95.43%   95.43%           
=======================================
  Files         476      476           
  Lines       28988    28990    +2     
=======================================
+ Hits        27664    27666    +2     
  Misses       1324     1324           

@swcurran
Copy link
Contributor

Tested by @Moopli and @shaangill025

@andrewwhitehead
Copy link
Contributor

Looks good, thanks! I suspect round-tripping through JSON would be much slower and use even more memory. Can you update the branch?

@TimoGlastra TimoGlastra enabled auto-merge August 16, 2021 18:25
@TimoGlastra TimoGlastra merged commit a32b601 into openwallet-foundation:main Aug 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants