search

Sphereon-Opensource / SIOP-OID4VP

Self Issued OpenID Provider v2 (SIOP) with optional OpenID for Verifiable Presentations (OpenID4VP)
77 stars 25 forks source link

Expected behaviour for `PresentationExchange.selectVerifiableCredentialsForSubmission` method #14

Closed siacomuzzi closed 1 year ago

siacomuzzi commented 2 years ago

Hello there!

I just found that the PresentationExchange.selectVerifiableCredentialsForSubmission method is throwing error when pejs.selectFrom includes at least 1 error as part of the result: https://github.com/Sphereon-Opensource/did-auth-siop/blob/2954cd53967e763fe4ca28918f208732d2779403/src/main/PresentationExchange.ts#L100-L102

I'm wondering what's the reason to do that, without checking by selectResults.areRequiredCredentialsPresent === 'info' before: https://github.com/Sphereon-Opensource/pex/blob/beb25e231d22858cfcccc075c1d1454dac36568b/lib/evaluation/core/evaluationResults.ts#L6-L15

Please correct me if I'm wrong but the selectResults.errors array will always contains elements unless that all of the VCs specified in the PresentationExchange constructor (allVerifiableCredentials property) meet the provided presentation definition.

Thank you

nklomp commented 2 years ago

Hi @siacomuzzi You are right.

We have a release outstanding for PEX which empties errors when they are not in scope of a descriptor. We expect to release that version tomorrow. Then we will update this library as well with the latest version.

We will check for the 'info'value as well. Reason we did not do that before, is very simple. At creation of the SIOP library the PEX library didn't have that property ;)

I expect we will release an updated version on thursday.

Thanks for bringing it up

siacomuzzi commented 2 years ago

Great! Thank you! I have another one 😃 https://github.com/Sphereon-Opensource/did-auth-siop/blob/2954cd53967e763fe4ca28918f208732d2779403/src/main/AuthenticationResponse.ts#L224

The original nonce provided as part of the request (verifiedJwt.payload.nonce) is ignored there, so it's not included as part of the response. Is that expected?

nklomp commented 2 years ago

No definitely not.

The SIOP Lib needs more TLC. Since the DIF Presentation Exchange spec v2.0 is now nearing its completion, we will update the PEX v2 support in our PEX library soon. Then this library will be next. There is certainly work to be done on this lib :)

nklomp commented 1 year ago

Fixed with PEX v2 and did-auth-siop v0.3.0