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

Prover: fix credx credential retrieval #961

Merged
merged 10 commits into from
Aug 30, 2023
Merged

Prover: fix credx credential retrieval #961

merged 10 commits into from
Aug 30, 2023

Conversation

Patrik-Stas
Copy link
Contributor

@Patrik-Stas Patrik-Stas commented Aug 29, 2023

  • Fixes retrieval of credentials for prover. When multiple restriction objects are specified, they should be treated as OR instead of AND. Verifier is asking prover to satisfy at least 1 restriction object, not all of them.

  • Adds test test_agency_pool_it_should_select_credentials_for_satisfiable_restriction which was failing without modifications to WQL made in the PR. Passes with the modifications. See anoncreds spec https://hyperledger.github.io/anoncreds-spec/#restrictions

A boolean expression is formed by ORing and ANDing the source credential properties. The following JSON is an example. Any of the source credential properties listed above can be used in the expression components:

  "restrictions": [
    {
      "issuer_did": "<did>",
      "schema_id": "id"
    },
    {
      "cred_def_id" : "<id>",
      "attr::color::marker": "1",
      "attr::color::value" : "red"
    }
  ]

The properties in each list item are AND’d together, and the array elements are OR’d together. As such, the example above defines the logical expression:

The attributes must come from a source verifiable credential such that:
issuer_did = AND
schema_id =
OR
cred_def_id = AND
the credential must contain an attribute name "color" AND
the credential must contain an attribute name "color" with the attribute value "red"

@Patrik-Stas Patrik-Stas marked this pull request as ready for review August 29, 2023 18:17
@Patrik-Stas Patrik-Stas changed the title Prover/cred selection Prover: fix credential retrieval Aug 29, 2023
@Patrik-Stas Patrik-Stas changed the title Prover: fix credential retrieval Prover: fix credx credential retrieval Aug 29, 2023
Copy link
Contributor

@gmulhearn gmulhearn left a comment

Choose a reason for hiding this comment

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

LGTM; but would be good to fix up my comment while you're in this area.

(Or consider the comment and let me know if there is something I'm not understanding)

Good to merge after that

aries_vcx_core/src/anoncreds/credx_anoncreds.rs Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Aug 29, 2023

Codecov Report

Merging #961 (ada6976) into main (f8aaadc) will decrease coverage by 0.12%.
The diff coverage is 77.55%.

@@            Coverage Diff             @@
##             main     #961      +/-   ##
==========================================
- Coverage   39.35%   39.23%   -0.12%     
==========================================
  Files         417      417              
  Lines       28986    28968      -18     
  Branches     6185     6187       +2     
==========================================
- Hits        11407    11367      -40     
- Misses      14236    14259      +23     
+ Partials     3343     3342       -1     
Flag Coverage Δ
unittests-aries-vcx 39.23% <77.55%> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...ries_vcx/src/handlers/proof_presentation/prover.rs 43.90% <0.00%> (-0.36%) ⬇️
aries_vcx_core/src/anoncreds/credx_anoncreds.rs 57.20% <63.63%> (-0.26%) ⬇️
aries_vcx/tests/test_creds_proofs.rs 88.31% <83.33%> (+0.07%) ⬆️
aries_vcx/tests/utils/scenarios.rs 83.11% <84.61%> (-0.36%) ⬇️

... and 5 files with indirect coverage changes

Base automatically changed from fix/testing to main August 30, 2023 01:01
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
…of simpler testing approach

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
…e_restriction

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
…predicate for credx implementation

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
gmulhearn
gmulhearn previously approved these changes Aug 30, 2023
Copy link
Contributor

@gmulhearn gmulhearn left a comment

Choose a reason for hiding this comment

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

Fix from my last comment looks good

.github/workflows/main.yml Outdated Show resolved Hide resolved
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
@mirgee mirgee merged commit c2232d1 into main Aug 30, 2023
32 checks passed
@mirgee mirgee deleted the prover/cred-selection branch August 30, 2023 11:24
nain-F49FF806 pushed a commit to nain-F49FF806/aries-vcx that referenced this pull request Sep 1, 2023
* Test code adjustments

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* wip

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* Rename 'requested_credentials' to 'preselected_credentials'

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* Remove prover_select_credentials_and_fail_to_generate_proof in favor of simpler testing approach

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* Simplify common testing case of sending proof

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* Add test test_agency_pool_it_should_select_credentials_for_satisfiable_restriction

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* Fix credential retrieval by prover

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* Do not run test_agency_pool_it_should_fail_to_select_credentials_for_predicate for credx implementation

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* Address code review

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* Revert CI change

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

---------

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
bobozaur pushed a commit that referenced this pull request Sep 14, 2023
* Test code adjustments

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* wip

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* Rename 'requested_credentials' to 'preselected_credentials'

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* Remove prover_select_credentials_and_fail_to_generate_proof in favor of simpler testing approach

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* Simplify common testing case of sending proof

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* Add test test_agency_pool_it_should_select_credentials_for_satisfiable_restriction

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* Fix credential retrieval by prover

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* Do not run test_agency_pool_it_should_fail_to_select_credentials_for_predicate for credx implementation

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* Address code review

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* Revert CI change

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

---------

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
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