-
Notifications
You must be signed in to change notification settings - Fork 239
feat: custom verification callback for the authorization request on the holder side for verification #2303
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: custom verification callback for the authorization request on the holder side for verification #2303
Changes from 1 commit
798d673
8eff971
ae03ac5
cc916d4
d4b82f9
03b0011
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,20 +1,28 @@ | ||
import type { | ||
CanBePromise, | ||
DcqlCredentialsForRequest, | ||
DcqlQueryResult, | ||
DifPexCredentialsForRequest, | ||
DifPexInputDescriptorToCredentials, | ||
DifPresentationExchangeDefinition, | ||
EncodedX509Certificate, | ||
} from '@credo-ts/core' | ||
import { ResolvedOpenid4vpAuthorizationRequest } from '@openid4vc/openid4vp' | ||
import type { OpenId4VpAuthorizationRequestPayload } from '../shared' | ||
import { Openid4vpAuthorizationRequestDcApi, ResolvedOpenid4vpAuthorizationRequest } from '@openid4vc/openid4vp' | ||
import type { OpenId4VpAuthorizationRequestPayload, Openid4vpAuthorizationRequest } from '../shared' | ||
|
||
// TODO: export from oid4vp | ||
export type ParsedTransactionDataEntry = NonNullable<ResolvedOpenid4vpAuthorizationRequest['transactionData']>[number] | ||
|
||
export type VerifyAuthorizationRequestOptions = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel we're still missing relevant information from the Also we should try to stay as consistent as possible with So authorizationRequestPayload: OpenId4VpAuthorizationRequestPayload and ideally:
also add the origin (in case of DC-API):
Maybe we can just pass the same interface? Or do we call this before the DCQL and Presentation Exchange Queries are run? (in that case we can omit these from the interface). But i would rather not add |
||
authorizationRequest: Openid4vpAuthorizationRequest | Openid4vpAuthorizationRequestDcApi | ||
} | ||
|
||
export type VerifyAuthorizationRequestCallback = (options: VerifyAuthorizationRequestOptions) => CanBePromise<boolean> | ||
|
||
export interface ResolveOpenId4VpAuthorizationRequestOptions { | ||
trustedCertificates?: EncodedX509Certificate[] | ||
origin?: string | ||
verifyAuthorizationRequestCallback?: VerifyAuthorizationRequestCallback | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Soo looking at it like this, what does the callback provide over just doing
The callback just adds another layer of indirection. For the verifier side the callback makes more sense since we automatically send a http response (and you might want to block it). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also thinking whether the name of the callback may mislead into thinking it will override the verification (which it doesn't). To continue on my previous comment. The input to the verify is just the authorization request, which is not always the full context (which you get returned from the method). So I'd actually lean to not merge this, and just call our custom callback after calling the credo resolve method There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It enforces it slightly more, the callback is optional so it is not enforced strictly. The API will be extended before merging to get more values next to the |
||
} | ||
|
||
type VerifiedJarRequest = NonNullable<ResolvedOpenid4vpAuthorizationRequest['jar']> | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
This will swallow any errors. I think it might be better to make the inner method throw. Or we at least return the error so we can add it as cause. i think requiring the callback to throw has my preference