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

Use token endpoint from session #3126

Merged
merged 6 commits into from
May 24, 2024
Merged

Conversation

gerardsn
Copy link
Member

@gerardsn gerardsn commented May 17, 2024

@gerardsn gerardsn marked this pull request as ready for review May 22, 2024 16:31
@gerardsn gerardsn force-pushed the use-token-endpoint-from-session branch from d655f80 to 55ea77c Compare May 23, 2024 09:38
@@ -123,10 +124,14 @@ func (c *OpenID4VPClient) PresentationDefinition(ctx context.Context, endpoint s
return presentationDefinition, nil
}

func (c *OpenID4VPClient) AuthorizationServerMetadata(ctx context.Context, webdid did.DID) (*oauth.AuthorizationServerMetadata, error) {
func (c *OpenID4VPClient) AuthorizationServerMetadata(ctx context.Context, oauthIssuer string) (*oauth.AuthorizationServerMetadata, error) {
Copy link
Member

Choose a reason for hiding this comment

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

this method is not specific for OpenID4VCI, is it? Then the param oauthIssuer is weird.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a naming conflict on the issuer term we keep running into.
The oauth /.well-known/authorization-server metadata has an issuer field that is the identity of the AS. This issuer is what the metadata URL is derived from, and corresponds to did:web identity in URL form. So oauthIssuer is just a more generic name and not related to the issuer-holder-verifier model. (hence prefix oauth)

We need this change because OpenID4VCI defines a /.well-known/openid-credential-issuer metadata that contains field authorization_servers that lists multiple URLs, not DIDs.

Copy link
Member Author

Choose a reason for hiding this comment

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

would you prefer 2 methods for convenience?

Copy link
Member

Choose a reason for hiding this comment

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

aha, then just explain this on the interfaces

// AuthorizationServerMetadata returns the metadata of the remote wallet.
AuthorizationServerMetadata(ctx context.Context, webdid did.DID) (*oauth.AuthorizationServerMetadata, error)
AuthorizationServerMetadata(ctx context.Context, oauthIssuerURI string) (*oauth.AuthorizationServerMetadata, error)
Copy link
Member

Choose a reason for hiding this comment

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

weird param name change


metadataURL, err := oauth.IssuerIdToWellKnown(serverURL.String(), oauth.AuthzServerWellKnown, hb.strictMode)
func (hb HTTPClient) OAuthAuthorizationServerMetadata(ctx context.Context, oauthIssuer string) (*oauth.AuthorizationServerMetadata, error) {
metadataURL, err := oauth.IssuerIdToWellKnown(oauthIssuer, oauth.AuthzServerWellKnown, hb.strictMode)
Copy link
Member

Choose a reason for hiding this comment

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

This method is not only used for issuance. Is IssuerIdToWellKnown different then the "DID Web to well-known"?
Also godoc on method is no longer correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

this is the same oauth.IssuerId.... In most cases oauthIssuer is just the did web url. Updated godoc

@gerardsn gerardsn requested a review from woutslakhorst May 24, 2024 08:51
@gerardsn gerardsn force-pushed the use-token-endpoint-from-session branch from 0faac8c to 3eed39e Compare May 24, 2024 10:58
@gerardsn gerardsn merged commit 066ae44 into master May 24, 2024
8 of 9 checks passed
@gerardsn gerardsn deleted the use-token-endpoint-from-session branch May 24, 2024 14:13
@gerardsn gerardsn restored the use-token-endpoint-from-session branch May 24, 2024 14:13
@gerardsn gerardsn deleted the use-token-endpoint-from-session branch May 24, 2024 14:13
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.

OpenID4VCI uses wrong authorization server metadata endpoint OpenID4VCI uses wrong AccessToken endpoint
2 participants