-
Notifications
You must be signed in to change notification settings - Fork 204
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
feat!: auth code flow #2088
feat!: auth code flow #2088
Conversation
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]>
Signed-off-by: Timo Glastra <[email protected]>
…esentation during issuance and batch issuance. This is a big change to OpenID4VCI in Credo, with the neccsary breaking changes since we first added it to the framework. Over time the spec has changed significantly, but also our understanding of the standards and protocols. **Authorization Code Flow** Credo now supports the authorization code flow, for both issuer and holder. An issuer can configure multiple authorization servers, and work with external authorization servers as well. The integration is based on OAuth2, with several extension specifications, mainly the OAuth2 JWT Access Token Profile, as well as Token Introspection (for opaque access tokens). Verification works out of the box, as longs as the authorization server has a `jwks_uri` configured. For Token Introspection it's also required to provide a `clientId` and `clientSecret` in the authorization server config. To use an external authorization server, the authorization server MUST include the `issuer_state` parameter from the credential offer in the access token. Otherwise it's not possible for Credo to correlate the authorization session to the offer session. The demo-openid contains an example with external authorization server, which can be used as reference. The Credo authorization server supports DPoP and PKCE. **Batch Issuance** The credential request to credential mapper has been updated to support multiple proofs, and also multiple credential instances. The client can now also handle batch issuance. **Presentation During Issuance** The presenation during issuance allows to request presentation using OID4VP before granting authorization for issuance of one or more credentials. This flow is automatically handled by the `resolveAuthorizationRequest` method on the oid4vci holder service. 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]>
🦋 Changeset detectedLatest commit: fd0c71f The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are quite some TODO's still, but I assume everything that should be fixed before merging will be fixed :). Good addition @TimoGlastra !!
demo-openid/package.json
Outdated
@@ -10,15 +10,19 @@ | |||
"license": "Apache-2.0", | |||
"scripts": { | |||
"issuer": "ts-node src/IssuerInquirer.ts", | |||
"provider": "tsx src/Provider.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just use ts-node or tsx for all of them, we don't need both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yeah, provider needs esm support which does not work with ts-node. I'll update them all to tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait the issue is:
- tsx supports esm but no decorators (so works for provider)
- ts-node does nont support esm, but does support decorators (so works for issuer/holder/verifier)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did my best but no luck. We will either have to update to the new decorator support (still to experimental i think), or move away from decorators.
if (resolvedAuthorizationRequest.authorizationFlow === OpenId4VciAuthorizationFlow.PresentationDuringIssuance) { | ||
return { | ||
...resolvedAuthorizationRequest, | ||
authorizationFlow: `${OpenId4VciAuthorizationFlow.PresentationDuringIssuance}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
authorizationFlow: `${OpenId4VciAuthorizationFlow.PresentationDuringIssuance}`, | |
authorizationFlow: OpenId4VciAuthorizationFlow.PresentationDuringIssuance, |
or
authorizationFlow: `${OpenId4VciAuthorizationFlow.PresentationDuringIssuance}`, | |
authorizationFlow: OpenId4VciAuthorizationFlow.PresentationDuringIssuance.toString(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit unfortunate but both don't give the same result.
First one make it an enum value, which makes typescript more strict. In this case I need it as string.
Second one doesn't give type infer, but just casts it to string instead of a constant.
packages/openid4vc/src/openid4vc-issuer/OpenId4VcIssuerModule.ts
Outdated
Show resolved
Hide resolved
OpenId4VciSignSdJwtCredential, | ||
OpenId4VciSignW3cCredential, | ||
OpenId4VciSignMdocCredential, | ||
OpenId4VciSignW3cCredentials, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as with the holder service file. Would really benefit from a split up in files and some helper functions to make it a lot easier to read.
/** | ||
* Whether presentation during issuance is required. | ||
*/ | ||
required: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this not be a boolean
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The object will never be defined it it's not true. So i think true
is more clear here. Alternative is empty object to indicate it's required but that felt not explicit.
Or another top-level prop for presentationRequired
and then presentation
for the details, but then you can still have that presentation
is undefined (in TS) even if required
is true. So no if you check for required == true
, the other props will also be defined
packages/openid4vc/src/openid4vc-issuer/router/accessTokenEndpoint.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
…instead of `vp_formats supported` (#2089) Signed-off-by: Timo Glastra <[email protected]>
…oken issuer 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]>
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]>
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]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
See #2056