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: remove bug for txCode #132

Merged
merged 3 commits into from
Jul 30, 2024
Merged

Conversation

cre8
Copy link
Contributor

@cre8 cre8 commented Jul 25, 2024

Fix for the bug in 7d17d13

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.

Signed-off-by: Mirko Mollik <[email protected]>
Copy link
Collaborator

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

This looks good to me. Do we need a test that covers this? Seems like a good one to cover

@cre8
Copy link
Contributor Author

cre8 commented Jul 26, 2024

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

Signed-off-by: Mirko Mollik <[email protected]>
@cre8
Copy link
Contributor Author

cre8 commented Jul 26, 2024

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:

  • We want to use tx_code for v13, so the first condition is true since we set tx_code and not user_pin_required.
  • We have set user_pin in the request, so the other condition is always true

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

@cre8 cre8 requested a review from TimoGlastra July 26, 2024 11:56
@nklomp
Copy link
Contributor

nklomp commented Jul 26, 2024

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
Copy link
Contributor Author

cre8 commented Jul 26, 2024

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
Copy link
Contributor Author

cre8 commented Jul 29, 2024

@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.

Copy link
Contributor

@nklomp nklomp left a comment

Choose a reason for hiding this comment

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

LGTM. Thx

@nklomp nklomp merged commit 858a8ea into Sphereon-Opensource:develop Jul 30, 2024
1 check passed
@cre8 cre8 deleted the fix/txCode branch July 31, 2024 12:10
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