added authorize endpoint as specified by rfc6549 authorization code #2626
Conversation
This file contains 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
6 tasks
|
|
woutslakhorst
force-pushed
the
feature/2625/oauth2_authorize_endpoint
branch
2 times, most recently
from
efe8242 to
f32b958
Compare
reinkrul
reviewed
Dec 8, 2023
reinkrul
requested changes
Dec 11, 2023
auth/api/iam/openid4vp.go
Outdated
| } | ||
| metadata, err := r.auth.Verifier().AuthorizationServerMetadata(ctx, *walletDID) | ||
| if err != nil { | ||
| return nil, oauthError(oauth.ServerError, "failed to get authorization server metadata (holder)", redirectURL) |
There was a problem hiding this comment.
Verifier, Wallet, Holder? Too many roles/concepts mixed here
| callbackURL := *ownURL | ||
| callbackURL.Path, err = url.JoinPath(callbackURL.Path, "response") | ||
| if err != nil { | ||
| return nil, oauthError(oauth.ServerError, "failed to construct redirect path", redirectURL) |
There was a problem hiding this comment.
I don't think this can ever occur?
| "github.com/nuts-foundation/nuts-node/auth/oauth" | ||
| ) | ||
|
|
||
| var _ Verifier = (*VerifierServiceProvider)(nil) |
There was a problem hiding this comment.
I'd expect a Verifier, which is from OpenID4VP, to be in an OpenID4VP package? Otherwise, we might have to prefix the type name with something that indicates the protocol/spec/domain? E.g. PresentationVerifier?
There was a problem hiding this comment.
I'd like to complete the entire flow first and then see if stuff needs to move.
| // Verifier implements the OpenID4VP Verifier role. | ||
| type Verifier interface { | ||
| // AuthorizationServerMetadata returns the metadata of the remote wallet. | ||
| AuthorizationServerMetadata(ctx context.Context, webdid did.DID) (*oauth.AuthorizationServerMetadata, error) |
There was a problem hiding this comment.
This is not OpenID4VP-specific, so I'd expect this to be in a more generic OAuth2 type?
There was a problem hiding this comment.
This is true, but currently it's closer related to things the verifier needs/has. Let's also park this till the end.
| RedirectURI: redirectURL.String(), | ||
| } | ||
| // use nonce to store authorization request in session store | ||
| if err = r.oauthNonceStore().Put(nonce, openid4vpRequest); err != nil { |
There was a problem hiding this comment.
shouldn't this be client state instead?
There was a problem hiding this comment.
both probably, but this automatically emerges in the next PR because there it's needed!
woutslakhorst
force-pushed
the
feature/2625/oauth2_authorize_endpoint
branch
from
846b256 to
9d1f337
Compare
woutslakhorst
force-pushed
the
feature/2625/oauth2_authorize_endpoint
branch
from
9d1f337 to
3b47e20
Compare
reinkrul
approved these changes
Dec 12, 2023
woutslakhorst
added a commit
that referenced
this pull request
Jan 10, 2024
…2626) wip added bunch of tests added missing API tests fix failing test add tests for failing directpost responses refactor ami client and reuse response parsing/code checking happy flow tests for holder service error tests for holder service comment touchup on some comments
woutslakhorst
added a commit
that referenced
this pull request
Jan 10, 2024
…2626) wip added bunch of tests added missing API tests fix failing test add tests for failing directpost responses refactor ami client and reuse response parsing/code checking happy flow tests for holder service error tests for holder service comment touchup on some comments
woutslakhorst
added a commit
that referenced
this pull request
Jan 15, 2024
…2626) wip added bunch of tests added missing API tests fix failing test add tests for failing directpost responses refactor ami client and reuse response parsing/code checking happy flow tests for holder service error tests for holder service comment touchup on some comments
woutslakhorst
added a commit
that referenced
this pull request
Jan 16, 2024
…2626) wip added bunch of tests added missing API tests fix failing test add tests for failing directpost responses refactor ami client and reuse response parsing/code checking happy flow tests for holder service error tests for holder service comment touchup on some comments fix didkey pattern remove url decode from middleware, now handled by codegen bind add callback, fix request logger fix logger test add e2e test added handling of error direct_post
woutslakhorst
added a commit
that referenced
this pull request
Jan 22, 2024
* added authorize endpoint as specified by rfc6549 authorization code (#2626) * add e2e test
woutslakhorst
added a commit
that referenced
this pull request
Jan 22, 2024
…2626) wip added bunch of tests added missing API tests fix failing test add tests for failing directpost responses refactor ami client and reuse response parsing/code checking happy flow tests for holder service error tests for holder service comment touchup on some comments fix didkey pattern remove url decode from middleware, now handled by codegen bind add callback, fix request logger fix logger test add e2e test added handling of error direct_post
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
closes #2625
I changed the expectation of did:nuts for OpenID4VP into did:web.
The PR might also remove some unused stuff....