Sphereon-Opensource / OID4VC

OpenID for Verifiable Credentials - modules for issuers, holders and RPs
Apache License 2.0
62 stars 19 forks source link

fix: remove bug for txCode #132

Closed cre8 closed 1 month ago

cre8 commented 1 month ago

Fix for the bug in https://github.com/Sphereon-Opensource/OID4VC/commit/7d17d13d3485c5b6b55ef876eba8f09c9f7a788b

As explained in the comment there, the line below will overwrite the set value for the pin. We do not need to store the value of txCode since this is already in the grant. So it's fine to remove this line without any side effects.

cre8 commented 1 month ago

Tests would be good because we have none yet. This is my my e2e test of my application run into errors when using it with a pin

cre8 commented 1 month ago

I also corrected another bug in the client: in v13 we are using tx_code and not user_pin in the request (but it was still set to user_pin). Therefore we would always run into this error:

So the correct way would be to pass it in request.tx_code. If so, the first if statement would be false (we have it defined in the grant because we want it) and everything works :)

Another solution would be to pass user_pin and txCode to be compliant to possible older system and then remove it again when we drop the older versions

nklomp commented 1 month ago

Yeah I am leaning towards the latter. We for sure will drop it this fall, once the refactor work removing all the old support happens. In the mean time I want to have no impact on existing implementations and interop

cre8 commented 1 month ago

Yeah I am leaning towards the latter. We for sure will drop it this fall, once the refactor work removing all the old support happens. In the mean time I want to have no impact on existing implementations and interop

I will check if I can implement it without a breaking changes. If so, I will give you a ping here.

cre8 commented 1 month ago

@nklomp @TimoGlastra I added both into the request, since we have no only whitelist validation this is fine.

I extended the if statements for pin not required, otherwise it will always fail.