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

Relax validation of holder DID when submitting a credential request #2731

Merged
merged 8 commits into from
Jan 29, 2024

Conversation

ianco
Copy link
Contributor

@ianco ianco commented Jan 23, 2024

See issue #2714

Relax validation to allow did:peer (or any value) for the holder/prover did on a credential request.

This works with an askar wallet, but anoncreds-askar will fail due to its own did validation (see issue hyperledger/anoncreds-rs#307)

This PR will work once the anoncreds-rs issue is also fixed.

@swcurran
Copy link
Contributor

The AnonCreds issue has been resolved and the PR is part of the 0.2.0-dev8 that was published tonight. I'll add a ticket to update ACA-Py to use that release. Since @jamshale did the 0.2.0-dev7 update, I'll ask if he can do the 0.2.0-dev8 one as well.

@ianco ianco marked this pull request as ready for review January 25, 2024 17:37
@ianco ianco requested a review from jamshale January 25, 2024 17:38
jamshale
jamshale previously approved these changes Jan 25, 2024
@ianco ianco marked this pull request as draft January 25, 2024 17:48
@ianco
Copy link
Contributor Author

ianco commented Jan 25, 2024

Hmmm integration tests are failing I'll take a look ...

@jamshale
Copy link
Contributor

Hmmm integration tests are failing I'll take a look ...

Those were different than the ones failing for me locally. Might be the sporadic fail that happens sometimes 🤷‍♂️

@ianco
Copy link
Contributor Author

ianco commented Jan 25, 2024

Hmmm integration tests are failing I'll take a look ...

Those were different than the ones failing for me locally. Might be the sporadic fail that happens sometimes 🤷‍♂️

Could be I'm trying to run locally

@ianco
Copy link
Contributor Author

ianco commented Jan 25, 2024

Hmmm integration tests are failing I'll take a look ...

Those were different than the ones failing for me locally. Might be the sporadic fail that happens sometimes 🤷‍♂️

This error:

anoncreds.error.AnoncredsError: type: RevocationRegistryDefinitionId, identifier: 5VieLs9kRQ2gMXWoxbKewg:4:5VieLs9kRQ2gMXWoxbKewg:3:CL:53:Acme.agent.Schema_DriversLicense_v2:CL_ACCUM:1 is invalid. It MUST be a URI or legacy identifier.

Investigating ...

@ianco
Copy link
Contributor Author

ianco commented Jan 25, 2024

The alice/faber demo is working but the integration tests are failing ... grrrr ...

@ianco
Copy link
Contributor Author

ianco commented Jan 26, 2024

@jamshale just double checked and the same integration tests were failing on your PR (which we merged). There was also one of our famous random failures ...

I think the anoncreds-rs update affected the validation for the revocation registry id, I'll log another issue on the anoncreds-rs repo.

Copy link

sonarcloud bot commented Jan 29, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@ianco ianco marked this pull request as ready for review January 29, 2024 23:02
@ianco ianco merged commit 60ea247 into openwallet-foundation:main Jan 29, 2024
8 checks passed
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