search

eu-digital-identity-wallet / eudi-lib-jvm-openid4vci-kt

Implementation of OpenID for Verifiable Credential Issuance protocol (wallet's role) in Kotlin
Apache License 2.0
13 stars 5 forks source link

AuthorizeIssuance.prepareAuthorizationRequest should take or return generated state #205

Closed ydanneg closed 2 months ago

ydanneg commented 2 months ago

Dear team,

State is currently generated inside AuthorizeIssuanceImpl and can not be extracted or stored after calling AuthorizeIssuance.prepareAuthorizationRequest() without parsing resulting authorizationCodeURL

https://github.com/eu-digital-identity-wallet/eudi-lib-jvm-openid4vci-kt/blob/0b2946cab0483fbab170094fb0f5d221b5db3eb6/src/main/kotlin/eu/europa/ec/eudi/openid4vci/internal/AuthorizeIssuanceImpl.kt#L70

At the end of the flow Wallet instance will get the code and state (e.g. "state=umerLYauAS-E33UK64uD3SMoC2orCslWku9xSrGm5Rc&code=blDtU1XSgFyMv1FoBg6402GRaXHsDZ8JgKgzVR-Sv-E") At this point Wallet instance MUST parse this url to get the code to continue the flow by calling authorizeWithAuthorizationCode on a persisted AuthorizationRequestPrepared object. At this point Wallet needs to be sure this persisted object matches the original state.

I see several solutions:

All these solutions will help Wallets, to restore their state (AuthorizationRequestPrepared) by a state value and continue the flow.

Your thoughts?

Gennadi

babisRoutis commented 2 months ago

Hi @ydanneg

I have the impression that here there are two issues:

I prepared a PR for this. But I could use your feedback for the following: Do we really need to allow the wallet to provide a state value, or should we just keep only the random?

ydanneg commented 2 months ago

Hi @ydanneg

I have the impression that here there are two issues:

  • Since we use state in the authorization request, we should check what authorization server returns. This is missing and it is a bug
  • In addition, currently state is generated by the lib. We may allow, the wallet to optionally provide a state value. This is a feature.

I prepared a PR for this. But I could use your feedback for the following: Do we really need to allow the wallet to provide a state value, or should we just keep only the random?

we could still use random by default but it's better to return it to the wallet.

babisRoutis commented 2 months ago

we could still use random by default but it's better to return it to the wallet.

Dear @ydanneg ,

Perhaps I confused you. Sorry for that.

If you check the PR you will see that:

My question was about : if you find useful - library user - to allow the wallet to provide a state or whether it should be always generated ?

ydanneg commented 2 months ago

we could still use random by default but it's better to return it to the wallet.

Dear @ydanneg ,

Perhaps I confused you. Sorry for that.

If you check the PR you will see that:

  • Library always sets the state parameter to the auth. request.
  • The value of the state can be optionally set by the wallet. If not, a random value will be used
  • The value of the state used in auth. requested is being returned to the caller (the wallet) as part of the AuthorizationRequestPrepared

My question was about : if you find useful - library user - to allow the wallet to provide a state or whether it should be always generated ?

Currently I don't have any use-case to provide a custom state. But who knows what happens in the future? ;)

ydanneg commented 2 months ago

we could still use random by default but it's better to return it to the wallet.

Dear @ydanneg ,

Perhaps I confused you. Sorry for that.

If you check the PR you will see that:

  • Library always sets the state parameter to the auth. request.
  • The value of the state can be optionally set by the wallet. If not, a random value will be used
  • The value of the state used in auth. requested is being returned to the caller (the wallet) as part of the AuthorizationRequestPrepared

My question was about : if you find useful - library user - to allow the wallet to provide a state or whether it should be always generated ?

Currently I don't have any use-case to provide a custom state. But who knows what happens in the future? ;)

Actually that would be useful to allow providing custom state. For example to put the current user location into the state to return to after.

babisRoutis commented 2 months ago

Merged.

@ydanneg Many thanks!