Skip to content
This repository has been archived by the owner on Oct 2, 2024. It is now read-only.

chore: some changes for JFF interop #61

Closed

Conversation

TimoGlastra
Copy link
Contributor

@TimoGlastra TimoGlastra commented Sep 28, 2023

Have been tinkering a bit to get interop with MATTR launchpad.

It now seems to get stuck on the presentation definition validation, so will have to look at the PEX library for that.

Not sure if the JWT verification bypass is 'ok'. If there's no JWT I think it would be fine to just not validate it, but if there is one, it is validated.

Not ready to be merged, but if you do take a look, maybe these changes can be of help for getting the sphereon wallet working...

@TimoGlastra TimoGlastra marked this pull request as draft September 28, 2023 12:18
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.

Cool, I was going to work on this tomorrow

Be aware that at some points we might need to include some logic for version discovery. Because basically direct_post is replacing post from versions < 17. In the old post mode, redirect_uri was required to be present, whilst now it can also have the response_uri, but not both.

@@ -571,6 +571,10 @@ export enum ResponseMode {
FORM_POST = 'form_post',
POST = 'post',
QUERY = 'query',

// Defined in openid4vp
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Defined in openid4vp
// Defined in openid4vp version 1.0.17 and higher to replace 'post'

@@ -154,6 +154,10 @@ export class AuthorizationRequest {
// TODO: We need to do something with the metadata probably
}
await checkWellknownDIDFromRequest(mergedPayload, opts);

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be in the AuthrorizationRequest.verify function. As an OP calls that first indirectly to verify the request.

Around lines 140-150 the JWT payload (OIDC style) gets merged with the original payload (OAuth2 style). Directly after that would be a logical place to check.

Just for reference the important part of the spec:

response_uri:
OPTIONAL. The Response URI to which the Wallet MUST send the Authorization Response using an HTTPS POST request as defined by the Response Mode direct_post. The Response URI receives all Authorization Response parameters as defined by the respective Response Type. When the response_uri parameter is present, the redirect_uri Authorization Request parameter MUST NOT be present. If the redirect_uri Authorization Request parameter is present when the Response Mode is direct_post, the Wallet MUST return an invalid_request Authorization Response error.
Note: The Verifier's component providing the user interface (Frontend) and the Verifier's component providing the Response URI (Response Endpoint) need to be able to map authorization requests to the respective authorization responses. The Verifier MAY use the state Authorization Request parameter to add appropriate data to the Authorization Response for that purpose, for details see [Section 10.5](https://openid.net/specs/openid-4-verifiable-presentations-1_0.html#implementation_considerations_direct_post).

Note: If the Client Identifier scheme redirect_uri is used in conjunction with the Response Mode direct_post, and the redirect_uri parameter is present, the client_id value MUST be equal to the response_uri value.

@TimoGlastra
Copy link
Contributor Author

Awesome thanks! I wasn't aware that it replaces post. More version logic... 😅

@nklomp
Copy link
Contributor

nklomp commented Sep 28, 2023

Yeah, long live evolving specs. I cannot wait once things settle down to start refactoring and remove a lot of the complexities from handling multiple versions..

BTW be aware that some changes have been made to this lib, mainly bugfixes

Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
@TimoGlastra
Copy link
Contributor Author

Got it working with this very hacky encoding logic. Not sure how you're on adding dependencies, but with qs it's a one liner :)

https://www.npmjs.com/package/qs?activeTab=dependencies

Signed-off-by: Timo Glastra <[email protected]>
@nklomp
Copy link
Contributor

nklomp commented Oct 4, 2023

FYI: I am taking some of the changes from this PR in the branch I am working on for V18 support

@TimoGlastra
Copy link
Contributor Author

Yes, I was figuring that it's easiest if I push my 'hacks' and you can properly integrate it where needed. It's quite a mess in how I got things working :/

@TimoGlastra TimoGlastra closed this Apr 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants