Make Pushed Authorization Request optional as per spec #658
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The spec does not require the Pushed Authorization Request (PAR) endpoint to be in the authorization server (AS) metadata, yet the wwwallet crashes if this pushed_authorzation_server_endpoint isn't there. This change makes it optional. It then initiates either a "normal" oidc flow, if the AS doesn't support the PAR. But intitiates a PAR if the server requires it. This fixes issue wwWallet#598.
Contributor
|
Closing this PR as per the reasoning in issue #598 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The spec does not require the Pushed Authorization Request (PAR) endpoint to be in the authorization server (AS) metadata, yet the wwwallet crashes if this pushed_authorzation_server_endpoint isn't there.
This change makes it optional. It then initiates either a "normal" oidc flow, if the AS doesn't support the PAR. But intitiates a PAR if the server requires it.
This partly fixes issue #598 .
It complements the PR in wwWallet/wallet-common#3
Inside "our own fork" in wip/verify, we have applied a version on some of the older code that we use in our env, this was merged in #639 - the code was manually ported from the version at main to our version so the patches are different.
Some notes on the code quality:
I chose to duplicate a lot of code from OpenID4VCIPushedAuthorizationRequest in a new OpenID4VCIAuthorizationRequest. This could be DRYd. I chose not to, to keep the intention of this change clear and focused. Please let me know if you want me to DRY up the similarities and shared logic from these two services. And if so, if you want this in this PR, or in a different PR.
I chose to only use the PAR if it is required - not if it is optional. That makes the "common" authorization request - redirect/GET the common flow, but allows PAR if the AS requires this. Please let me know if you want a different business logic for this.
I chose to follow the AS metadata to determine this logic, and not some configuration option. It could be a configuration, but that would mean configuration and server-metadata must be matched and the combination validated runtime - ie. if the config option requires PAR, but the AS doesn't have PAR, this creates a complex validation issue. I opted for the simple version where the AS dictates what we can and should use in interacting with the AS.