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

Add support for revocable credentials in vc_di handler #2967

Merged
merged 11 commits into from
Jun 10, 2024

Conversation

EmadAnwer
Copy link
Contributor

@EmadAnwer EmadAnwer commented May 24, 2024

This pull request supports issuing revocable verifiable credentials in the vc_di handler.

The main changes include:

  • Adding revocation when creating a credential for proof. This includes creating a new revocation registry
  • Fixing vc_di proof request issue by updating vc_di/handler.py issue_credential to include revocation missing data
  • Adding integration tests to cover revocation of credentials.

This allows Aries to support the W3C Verifiable Credentials Data Model, including revocation of issued credentials. It enables revocable credential issuance and presentation via the vc_di protocols.

@ianco
Copy link
Contributor

ianco commented May 31, 2024

This line is incorrect: https://github.com/hyperledger/aries-cloudagent-python/pull/2967/files#diff-f7d9c8d89f2442a02ac7a5924be4046e195d411791e4cbfd0fd9e7e2ecb987adL557

if cred["proof"][0].get("rev_reg_id"):

You can fetch these attributes from the credential, see for example:

https://github.com/ianco/aries-cloudagent-python/commit/1903eb6d45000ff57b65a2470bcf747adc8e5a39

(Not the most elegant approach but you can use it as an example)

if attempt > 0:
LOGGER.info(
"Retrying credential creation for revocation registry %s",
cred_def_id,

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

This expression logs
sensitive data (secret)
as clear text.
This expression logs
sensitive data (secret)
as clear text.
This expression logs
sensitive data (secret)
as clear text.
Copy link
Contributor

@ianco ianco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @EmadAnwer I've doen a very quick review of the code and it looks ok, I need to review in a bit more detail and get one other dev to do a review.

In the meantime can you add a description to the pull request? It will help anyone who needs to review, and also in the future if someone needs to look back to see what was done it will give them some context. (Also release note re built based on the PR descriptions.)

Also there are a few code issues reported by one of the scanning tools, if you can take a look ...

I've built a couple of integration tests so I'll send those to you once I get them working :-S

ianco and others added 2 commits June 3, 2024 12:17
Integration tests for vc_di issue and revocation
@ianco
Copy link
Contributor

ianco commented Jun 4, 2024

FYI the SonarCloud errors (if you don't have access to the details) are:

Failed conditions
 [42.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=hyperledger_aries-cloudagent-python&pullRequest=2967&metric=new_coverage&view=list) (required ≥ 80%)
 [24.7% Duplication on New Code](https://sonarcloud.io/component_measures?id=hyperledger_aries-cloudagent-python&pullRequest=2967&metric=new_duplicated_lines_density&view=list) (required ≤ 3%)

The coverage is just related to the unit tests. The duplication is on the new create_credential_w3c() method which duplicates a lot of the existing create_credential() code (mostly the blocks around the revocation registry handling, which could be re-factored into helper methods).

@EmadAnwer
Copy link
Contributor Author

Thanks for reviewing, im editing it and i will push asap

@EmadAnwer EmadAnwer requested a review from ianco June 4, 2024 22:07
@ianco
Copy link
Contributor

ianco commented Jun 5, 2024

@EmadAnwer the coverage test is still failing on the VCDICredFormatHandler class (in formats/vc_di/handler.py).

There are a couple of unit tests already that are marked as "skipped" (since they were added before revocation was implemented, I suspect just copied form the indy version of the unit test) - async def test_issue_credential_revocable(self): and async def test_issue_credential_non_revocable(self): - can you take a look at those and see what it would take to make these work with your updated revocation support?

Otherwise everything in the PR looks good to me.

@EmadAnwer
Copy link
Contributor Author

@EmadAnwer the coverage test is still failing on the VCDICredFormatHandler class (in formats/vc_di/handler.py).

There are a couple of unit tests already that are marked as "skipped" (since they were added before revocation was implemented, I suspect just copied form the indy version of the unit test) - async def test_issue_credential_revocable(self): and async def test_issue_credential_non_revocable(self): - can you take a look at those and see what it would take to make these work with your updated revocation support?

Otherwise everything in the PR looks good to me.

@ianco Thanks for the review and suggestions, sure I will take a look into it and update the PR

@ianco
Copy link
Contributor

ianco commented Jun 6, 2024

@EmadAnwer it looks like the most recent commit (removing the dup code) is missing the DCO signoff

@EmadAnwer EmadAnwer force-pushed the vcdi-revocation branch 8 times, most recently from 46ceff8 to 077ab9b Compare June 7, 2024 21:05
@EmadAnwer EmadAnwer force-pushed the vcdi-revocation branch 8 times, most recently from 61166ad to 727f8a8 Compare June 9, 2024 14:22
EmadAnwer and others added 2 commits June 9, 2024 17:41
- test_create_offer
- test_create_request
- test_issue_credential_revocable
- test_match_sent_cred_def_id_error
- test_store_credential

Signed-off-by: EmadAnwer <[email protected]>
Copy link

sonarcloud bot commented Jun 9, 2024

@ianco ianco requested a review from jamshale June 10, 2024 15:12
@@ -64,9 +64,10 @@ Feature: RFC 0453 Aries agent issue credential

@WalletType_Askar_AnonCreds @SwitchCredTypeTest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add an @Release tag here? So the tests run on our nightly and pre-release workflows.

When "Acme" sends a request with explicit revocation status for proof presentation <Proof_request> to "Bob"
Then "Acme" has the proof verified

@WalletType_Askar_AnonCreds @SwitchCredTypeTest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should probably have these as automated tests as well with the @Release tag.

Copy link
Contributor

@jamshale jamshale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to have this automatically integration tested with the @Release workflow. But, everything looks good 👍

Similar to work I'm doing right now, I want to make sure revocation registry rotation and other operations are working correctly in anoncreds. See https://github.com/hyperledger/aries-cloudagent-python/blob/main/aries_cloudagent/anoncreds/revocation.py#L875. This isn't in scope for this task. I will look at it separately.

@ianco ianco merged commit 85359ec into openwallet-foundation:main Jun 10, 2024
8 checks passed
@ianco
Copy link
Contributor

ianco commented Jun 10, 2024

@jamshale I've merged the PR, I'll look at updating the tags on the integration tests later ...

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.

3 participants