Leave credentialStatus element in the LD credential

[tsabolov]

During the implementation we stumbled upon inability of a recipient to validate a credential offer. That happens because the credentialStatus fields is popped from the credential offer (see here). Later, the two entities (one from credential request, and another from the offer) are being compared, and naturally the comparison fails, which leads to inability to receive the credential offer. This change makes sure that the comparison of the provided reference data is comparable with the credential content, by replacing the pop method with get to leave the credentialStatus intact in the offer object.

[swcurran]

@shaangill025 @andrewwhitehead -- please review. Thanks!

Thanks for the PR.

[dbluhm]

See #1864 for some prior discussion on a similar change.

[swcurran]

Shoot -- just noticed that there is no DCO on the commit (DCO - Developer Certificate of Origin - https://github.com/apps/dco) and I clicked to merge from main, which probably means the DCO can't be fixed.

@tsabolov -- could you try to fix the commit, or close this PR, create a new branch, commit with the "-s" option (DCO) and create a new PR. Sorry to be a pain...DCO is required for all Hyperledger contributions.

[dbluhm]

If you haven't pulled changes from your remote branch and just have your original commit locally still, you can run:

git commit --amend -s
git push -f

If you have pulled the merge commit, I recommend resetting back to the original commit, amending and force pushing.

git reset --hard HEAD~1  # reset to before the merge commit
git commit --amend -s
git push -f

[codecov-commenter]

Codecov Report

Merging #1921 (e1c1713) into main (84fcfdc) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1921   +/-   ##
=======================================
  Coverage   93.60%   93.60%           
=======================================
  Files         539      539           
  Lines       34456    34456           
=======================================
  Hits        32251    32251           
  Misses       2205     2205           

[swcurran]

We finally have our SonarCloud check working reasonably well, and I think it would be worth looking at the identified code smell so that check will pass.

In the meantime -- @dbluhm -- could you please review the PR? Or suggest someone else?

Thanks!

[dbluhm]

So I think some of the same concerns raised in #1864 apply here. Namely, by permitting credentialStatus, there might be some implication that ACA-Py will verify the credentialStatus when in reality it is not. Our thoughts for #1864 was that this could be delegated to the controller with minimal changes required to ACA-Py (and it's even been done here in this PR with fewer lines than our go at it lol). After the discussion on that PR, I am in agreement with @TimoGlastra's thoughts expressed in this comment:

Having it available but not checked is something the user should know about before using ACA-Py. as with crypto libraries it should be as hard as possible to use it wrong.

Probably the best approach for now would be to at least support verifying it on the verifier side, which is a lot easier than issuance.

As Timo suggests, I think it would be wise to implement the verification of credentialStatus to make it as hard as possible to misuse ACA-Py "supporting" the credentialStatus feature of W3C credentials.

[swcurran]

Looking for input on this either here or we will discuss on the ACA-Pug call next Tuesday.

Based on my understanding, I think the right thing to do is merge this so verifications work, but with a big notice that says ACA-Py does not support setting or checking Credential Status with W3C credentials. And we add an Issue to see if anyone wants to add Credential Status support -- starting with a design document and moving into implementation.

[sonarqubecloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information