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

DIF Presentation Exchange and present-proof-v2 Updates #1125

Merged

Conversation

Signed-off-by: Shaanjot Gill <shaangill025@users.noreply.github.com>
Signed-off-by: Shaanjot Gill <shaangill025@users.noreply.github.com>
Signed-off-by: Shaanjot Gill <shaangill025@users.noreply.github.com>
Signed-off-by: Shaanjot Gill <shaangill025@users.noreply.github.com>
Signed-off-by: Shaanjot Gill <shaangill025@users.noreply.github.com>
Signed-off-by: Shaanjot Gill <shaangill025@users.noreply.github.com>
@shaangill025 shaangill025 marked this pull request as ready for review May 10, 2021 14:20
@TimoGlastra
Copy link
Contributor

ready to review? :)

@shaangill025
Copy link
Contributor Author

ready to review? :)

Yes

Signed-off-by: Shaanjot Gill <shaangill025@users.noreply.github.com>
Signed-off-by: Shaanjot Gill <shaangill025@users.noreply.github.com>
Signed-off-by: Shaanjot Gill <shaangill025@users.noreply.github.com>
Signed-off-by: Shaanjot Gill <shaangill025@users.noreply.github.com>
@shaangill025

This comment has been minimized.

@ianco
Copy link
Contributor

ianco commented Jun 4, 2021

If I issue a credential with no credentialSubject.id then the requested proof fails, with the following error on the holder/prover (same offer and request as I linked above, only difference is removed the credentialSubject.id attribute):

2021-06-04 06:42:02,831 aries_cloudagent.core.dispatcher ERROR Handler error: Dispatcher.handle_message
Traceback (most recent call last):
  File "/Users/icostanzo/Projects/aries-cloudagent-python/aries_cloudagent/core/dispatcher.py", line 198, in handle_message
    await handler(context, responder)
  File "/Users/icostanzo/Projects/aries-cloudagent-python/aries_cloudagent/protocols/present_proof/v2_0/handlers/pres_request_handler.py", line 86, in handle
    (pres_ex_record, pres_message) = await pres_manager.create_pres(
  File "/Users/icostanzo/Projects/aries-cloudagent-python/aries_cloudagent/protocols/present_proof/v2_0/manager.py", line 263, in create_pres
    await pres_exch_format.handler(self._profile).create_pres(
  File "/Users/icostanzo/Projects/aries-cloudagent-python/aries_cloudagent/protocols/present_proof/v2_0/formats/dif/handler.py", line 231, in create_pres
    pres = await dif_handler.create_vp(
  File "/Users/icostanzo/Projects/aries-cloudagent-python/aries_cloudagent/protocols/present_proof/dif/pres_exch_handler.py", line 1105, in create_vp
    result = await self.apply_requirements(req=req, credentials=credentials)
  File "/Users/icostanzo/Projects/aries-cloudagent-python/aries_cloudagent/protocols/present_proof/dif/pres_exch_handler.py", line 984, in apply_requirements
    filtered = await self.filter_constraints(
  File "/Users/icostanzo/Projects/aries-cloudagent-python/aries_cloudagent/protocols/present_proof/dif/pres_exch_handler.py", line 405, in filter_constraints
    signed_new_credential_dict = await derive_credential(
  File "/Users/icostanzo/Projects/aries-cloudagent-python/aries_cloudagent/vc/vc_ld/prove.py", line 135, in derive_credential
    return await derive(
  File "/Users/icostanzo/Projects/aries-cloudagent-python/aries_cloudagent/vc/ld_proofs/ld_proofs.py", line 107, in derive
    result = await ProofSet.derive(
  File "/Users/icostanzo/Projects/aries-cloudagent-python/aries_cloudagent/vc/ld_proofs/ProofSet.py", line 183, in derive
    derived_proof = await suite.derive_proof(
  File "/Users/icostanzo/Projects/aries-cloudagent-python/aries_cloudagent/vc/ld_proofs/suites/BbsBlsSignatureProof2020.py", line 120, in derive_proof
    reveal_document_statements = suite._create_verify_document_data(
  File "/Users/icostanzo/Projects/aries-cloudagent-python/aries_cloudagent/vc/ld_proofs/suites/BbsBlsSignature2020Base.py", line 34, in _create_verify_document_data
    c14n_doc = self._canonize(input=document, document_loader=document_loader)
  File "/Users/icostanzo/Projects/aries-cloudagent-python/aries_cloudagent/vc/ld_proofs/suites/LinkedDataProof.py", line 121, in _canonize
    raise LinkedDataProofException(
aries_cloudagent.vc.ld_proofs.error.LinkedDataProofException: 1 attributes dropped. Provide definitions in context to correct. ['credentialSubject']

@shaangill025

This comment has been minimized.

@shaangill025

This comment has been minimized.

Signed-off-by: Timo Glastra <timo@animo.id>
@TimoGlastra
Copy link
Contributor

OK found the issue. The blank node id transformation script was not handling nesting... This was also a problem in the MATTR libs, where we based our implementation on.

I pushed a commit that fixes this. Sorry about that.

See mattrglobal/jsonld-signatures-bbs#128 and mattrglobal/jsonld-signatures-bbs#129


The tests is still not passing, but now has to do with the credentialSubject.id being urn: where you try to find a did for. I think we should check if it is a placeholder blank node identifier (urn:bnid:_:c14n + num, e.g. => urn:bnid:_:c14n1) and then skip that step

@TimoGlastra
Copy link
Contributor

TimoGlastra commented Jun 5, 2021

Also, some of the tests are not using the local context from the injected document loader. Mainly from the test setup scripts I believe (I could at least find create_vcrecord test_data.py). This really slows down the tests, so would be nice to use the local contexts everywhere. You can easily see where we rely on remote contexts by running the tests with internet disabled.

@shaangill025

This comment has been minimized.

Signed-off-by: Shaanjot Gill <shaangill025@users.noreply.github.com>
…t-python into dif-pres-exch

Signed-off-by: Shaanjot Gill <shaangill025@users.noreply.github.com>
@ianco
Copy link
Contributor

ianco commented Jun 6, 2021

100% of tests pass not bad!!!

@TimoGlastra
Copy link
Contributor

Also, some of the tests are not using the local context from the injected document loader. Mainly from the test setup scripts I believe (I could at least find create_vcrecord test_data.py). This really slows down the tests, so would be nice to use the local contexts everywhere. You can easily see where we rely on remote contexts by running the tests with internet disabled.

I am a bit confused by the above comment, maybe I didn't understand it correctly. All credentials setup in test_data.py have a combination of https://w3id.org/security/bbs/v1 [BBS_V1], https://www.w3.org/2018/credentials/v1 [CREDENTIALS_V1], https://www.w3.org/2018/credentials/examples/v1 [EXAMPLES_V1] and https://w3id.org/citizenship/v1 [CITIZENSHIP_V1] in their contexts, all of these use local contexts in vc.tests.document_loader.py.

I mean the one in the tests, e.g. (may be others):

https://github.com/shaangill025/aries-cloudagent-python/blob/e18591fb00123d9a585dec70b6c024fcd04bc2c6/aries_cloudagent/protocols/present_proof/dif/tests/test_data.py#L41

@shaangill025

This comment has been minimized.

… expanded type incl, test updates

Signed-off-by: Shaanjot Gill <shaangill025@users.noreply.github.com>
…t-python into dif-pres-exch

Signed-off-by: Shaanjot Gill <shaangill025@users.noreply.github.com>
@TimoGlastra
Copy link
Contributor

Currently the whole proof object is always disclosed. This is the same in the MATTR library. The framing is applied to the credential without the proof object, so that's also why you get an empty object besides proofs:

  1. Proof is removed from credential
  2. Frame is applied to credential without proof
  3. New derived proof object is applied to derived credential

The frame is applied to the credential without the proof object. Therefore the framing is unsuccessful, because of the @requireAll property.

I want to verify with the AFGO team what they think should happen, but I think we shouldn't allow paths on the proof object (for now)

Signed-off-by: Shaanjot Gill <shaangill025@users.noreply.github.com>
@ianco
Copy link
Contributor

ianco commented Jun 7, 2021

Would be nice if there was a standard (unique) id attribute that was added to each w3c credential (by aca-py) when saved to the wallet, for example "credential_id": "1234-567-890" in the below:

{
      "contexts": [
        ...
      ],
      "expanded_types": [
        ...
      ],
      "schema_ids": [],
      "issuer_id": "did:key:zUC...VKcE",
      "subject_ids": [
        "did:key:zUC739...CqMSKA"
      ],
      "proof_types": [
        "BbsBlsSignature2020"
      ],
      "cred_value": {
        "@context": [
          ...
        ],
        "type": [
          ...
        ],
        "issuer": "did:key:zUC...VKcE",
        "issuanceDate": "2020-01-01T12:00:00Z",
        "credentialSubject": {
          "type": [
            "PermanentResident"
          ],
          "id": "did:key:zUC739...CqMSKA",
          "givenName": "ALICE",
          "familyName": "SMITH",
          "gender": "Female",
          "birthCountry": "Bahamas",
          "birthDate": "1958-07-17",
          "credential_id": "1234-567-890"
        },
        "proof": {
          ...
        }
      },
      "cred_tags": {},
      "record_id": "14537753f5ea4ab0b432e3813f982e81"
    }

When the holder/prover is presenting, this would give them a standard way of selecting a single credential.

Thoughts?

Or else we could allow record_id to be used for this purpose?

@shaangill025
Copy link
Contributor Author

shaangill025 commented Jun 7, 2021

Would be nice if there was a standard (unique) id attribute that was added to each w3c credential (by aca-py) when saved to the wallet, for example "credential_id": "1234-567-890" in the below:

{
      "contexts": [
        ...
      ],
      "expanded_types": [
        ...
      ],
      "schema_ids": [],
      "issuer_id": "did:key:zUC...VKcE",
      "subject_ids": [
        "did:key:zUC739...CqMSKA"
      ],
      "proof_types": [
        "BbsBlsSignature2020"
      ],
      "cred_value": {
        "@context": [
          ...
        ],
        "type": [
          ...
        ],
        "issuer": "did:key:zUC...VKcE",
        "issuanceDate": "2020-01-01T12:00:00Z",
        "credentialSubject": {
          "type": [
            "PermanentResident"
          ],
          "id": "did:key:zUC739...CqMSKA",
          "givenName": "ALICE",
          "familyName": "SMITH",
          "gender": "Female",
          "birthCountry": "Bahamas",
          "birthDate": "1958-07-17",
          "credential_id": "1234-567-890"
        },
        "proof": {
          ...
        }
      },
      "cred_tags": {},
      "record_id": "14537753f5ea4ab0b432e3813f982e81"
    }

When the holder/prover is presenting, this would give them a standard way of selecting a single credential.

Thoughts?

Or else we could allow record_id to be used for this purpose?

I think id within w3c credentials is used to specify the identifier of the credential and then in the presentation_definition constraint, we can match on $.id path. This id is also stored as given_id inside VCRecord.
https://www.w3.org/TR/vc-data-model/#example-1-a-simple-example-of-a-verifiable-credential

id is not enforced/required by the ld_proof handler so this might not work in every situation. Using record_id is a better solution, like the issuer_id in send_presentation endpoint, I can allow the user to specify list of record_id and only fetch those w3c credentials and pass it to the DIF pres_exch handler. This will not require updates to the handler implementation itself. Update: This is implemented now.

Signed-off-by: Shaanjot Gill <shaangill025@users.noreply.github.com>
@ianco
Copy link
Contributor

ianco commented Jun 8, 2021

For me this PR looks good, it passes the basic test scenarios and I like @shaangill025 's recent addition to specify credential(s) to present based on credential record_id (similar to how we can do this for Indy credentials).

@TimoGlastra anything outstanding here? I suspect there may be some fixes/changes etc we will need to do but for me it's good enough to merge now.

Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

Yay! 🎉

aries_cloudagent/vc/ld_proofs/constants.py Show resolved Hide resolved
@andrewwhitehead andrewwhitehead merged commit 76eee5a into openwallet-foundation:main Jun 9, 2021
@swcurran
Copy link
Contributor

swcurran commented Jun 9, 2021

w00t!!! Well done, Shaanjot @shaangill025 !! Awesome work.

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.

6 participants