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: report presentation result #2615

Conversation

dbluhm
Copy link
Contributor

@dbluhm dbluhm commented Nov 20, 2023

This PR ensures that the result of verifying the presentation for JSON-LD Credentials is factored into the final verified status.

cc @andrewwhitehead @swcurran

@dbluhm
Copy link
Contributor Author

dbluhm commented Nov 20, 2023

I'm expecting the integration tests to fail; I believe the challenge was not being correctly used by the holder side of the exchange previously. I will monitor and address this.

@dbluhm
Copy link
Contributor Author

dbluhm commented Nov 20, 2023

Once this PR is ready (pending a couple more anticipated fixes to get the integration tests failing/succeeding as expected), I will prepare a backport for 0.10.x.

@dbluhm
Copy link
Contributor Author

dbluhm commented Nov 20, 2023

Well shoot, tests passed... I'll look into this

@dbluhm
Copy link
Contributor Author

dbluhm commented Nov 21, 2023

The reason the tests passed when I expected them to fail was the testing I was performing outside of the integration tests happened to hit a slightly different path.

Where the integration tests call the send-presentation endpoint with a request body like the following:

{'dif': {'record_ids': {'citizenship_input_1': ['c02fc3be1ec74cb58d9c54f451caa2d2']}}}

My testing scenario was using:

{
  "dif": {
    "presentation_definition": {
      "format": {
        "ldp_vp": {
          "proof_type": [
            "BbsBlsSignature2020"
          ]
        }
      },
      "id": "461937c8-d69d-4f5b-9c39-dfa8d9622ea4",
      "input_descriptors": [
        {
          "constraints": {
            "fields": [
              {
                "id": "1f44d55f-f161-4938-a659-f8026467f126",
                "path": [
                  "$.credentialSubject.clearance"
                ],
                "purpose": "Get clearance"
              }
            ],
            "is_holder": [
              {
                "directive": "required",
                "field_id": [
                  "1f44d55f-f161-4938-a659-f8026467f126"
                ]
              }
            ],
            "limit_disclosure": "required"
          },
          "id": "building_access_1",
          "name": "BuildingAccess",
          "schema": [
            {
              "uri": "https://www.w3.org/2018/credentials#VerifiableCredential"
            },
            {
              "uri": "https://example.com/examples#Employment"
            }
          ]
        }
      ]
    }
  }
}

This resulted in hitting a buggy branch in the create_pres method of the dif format handler which caused the generated presentation to be based off of a newly generated random challenge instead of the one sent in the request. This behavior is what I witnessed that alerted me to this issue; even while the challenge was different, the presentation.verified was set to true. In my testing, the fixes in this PR caused the verified status to correctly come back as false and then the small correction I made to the create_pres handler fixed the other issue of using a bad challenge nonce.

I think the changes of this PR should stop here; however, the integration tests for JSON-LD creds are insufficient to get good coverage of the handler code. We should flesh out the JSON-LD tests further.

@dbluhm
Copy link
Contributor Author

dbluhm commented Nov 21, 2023

I need to revert my "fix" for the integration test "then" step. Turns out the verification was being performed elsewhere.

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
This reverts commit c36ced7.

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
@dbluhm dbluhm force-pushed the fix/presentation-result-verification branch from bfd3032 to 8663036 Compare November 21, 2023 18:29
Copy link

sonarcloud bot commented Nov 21, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@dbluhm
Copy link
Contributor Author

dbluhm commented Nov 21, 2023

@swcurran this PR is ready for review

@swcurran swcurran requested review from ianco and andrewwhitehead and removed request for ianco November 21, 2023 18:32
@swcurran
Copy link
Contributor

@andrewwhitehead — could you please look at this? I think it looks good, but you should verify.

@esune — you could look as well, as it doesn’t need a lot of context to review.

@ianco — I originally tagged you to review, but removed — you have too much to do. :-)

Copy link
Member

@esune esune left a comment

Choose a reason for hiding this comment

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

Changes look good to me 👍🏻

Copy link
Contributor

@swcurran swcurran left a comment

Choose a reason for hiding this comment

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

LGTM and @esune

@swcurran swcurran merged commit 0b01fff into openwallet-foundation:main Nov 21, 2023
9 checks passed
@dbluhm
Copy link
Contributor Author

dbluhm commented Nov 21, 2023

I'll prepare a backport to 0.10.x and will make some noise when it's ready

@dbluhm dbluhm deleted the fix/presentation-result-verification branch November 21, 2023 21:25
@swcurran swcurran removed the 0.11.0 label Nov 21, 2023
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