Sphereon-Opensource/OID4VC

Include txCode in the OpenID4VCIClient

cre8 opened this issue · 9 comments

cre8 commented

When parsing the uri, it would help to get the information from txCode to inform the user he needs to provide some values.

@nklomp I think it was included here: https://github.com/Sphereon-Opensource/OID4VCI/blob/40ad1ded998168e2703daeddc3a3afc78c9b19a1/packages/client/lib/CredentialOfferClient.ts#L76

Any reasons to not include it again? :)

cre8 commented

It seems to be available via client.credentialOffer.credential_offer.grants?.[ GrantTypes.PRE_AUTHORIZED_CODE ]?.tx_code,

But with the outcomment txCode the CredentialOfferRequestWithBaseUrl can lead to confusion because there the txCode is marked as optional, but it's value is never set

nklomp commented

Yeah I am working on it. It needed way more changes then initially estimates. Should have it fixed tonight

nklomp commented

Fixed in v0.14.0

  • ❤️1
cre8 commented

@nklomp I think it is not fixed :( When I am printing our the credental_offer and the request I get this:

{
  grants: {
    'urn:ietf:params:oauth:grant-type:pre-authorized_code': {
      'pre-authorized_code': '2e08465f-d04a-4d64-a75c-f8d8c6cfc154',
      tx_code: [Object]
    }
  },
  credential_configuration_ids: [ 'Identity' ],
  credential_issuer: 'http://localhost:3001'
}
{
  user_pin: '155195',
  grant_type: 'urn:ietf:params:oauth:grant-type:pre-authorized_code',
  'pre-authorized_code': '2e08465f-d04a-4d64-a75c-f8d8c6cfc154'
}

I am confused why now the user_pin is set since this was not before in the version when using v13

cre8 commented

It seems the request is build incorrect, see: https://github.com/Sphereon-Opensource/OID4VC/blob/develop/packages/client/lib/AccessTokenClient.ts#L107

Any reason to set it to user_pin or was it maybe a merge conflict? :)

cre8 commented

It seems that in the sessionsManager the value is still set as userPin, while the interface is designed to use txCode:

    const session: CredentialOfferSession = {
      preAuthorizedCode,
      issuerState,
      createdAt,
      lastUpdatedAt,
      status,
      notification_id: v4(),
      ...(userPin && { userPin }),
      ...(opts.credentialDataSupplierInput && { credentialDataSupplierInput: opts.credentialDataSupplierInput }),
      credentialOffer,
    }

I don't feel good to use userPin and txCode in the code, I suggest it should be refactored to only use txCode.

The small fix would be to update the check to:

if (request.tx_code !== credentialOfferSession.userPin) {

when we want to treat the object still as userPin for internal management

nklomp commented
cre8 commented

Okay, can we at least then patch the comparison line? Right now the function will fail all the time. I can open a PR for this (it will not break the old approaches)

nklomp commented

Sue, but it will need another set of eyes, to see whether we didn't miss other areas