-
Notifications
You must be signed in to change notification settings - Fork 25
MOSIP fixes #75
MOSIP fixes #75
Changes from 10 commits
221f9a3
6285f12
5d75581
625de09
e133e96
6bae0d4
66358b3
e16d20b
a026323
b74c350
142c25b
27ad7ef
ffe4f96
e28f000
9426648
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 |
---|---|---|
|
@@ -19,11 +19,13 @@ export const createResponsePayload = async ( | |
|
||
// If state was in request, it must be in response | ||
const state: string | undefined = await authorizationRequest.getMergedProperty('state'); | ||
const payload = await authorizationRequest.requestObject?.getPayload(); | ||
|
||
const responsePayload: AuthorizationResponsePayload = { | ||
...(responseOpts.accessToken && { access_token: responseOpts.accessToken }), | ||
...(responseOpts.tokenType && { token_type: responseOpts.tokenType }), | ||
...(responseOpts.refreshToken && { refresh_token: responseOpts.refreshToken }), | ||
...(payload?.nonce && { nonce: payload.nonce }), | ||
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. Nonce should not endup in authorization response. That should be either in the id token or vp token 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.
When I remove nonce from AuthorizationResponsePayload it will not be in merged.nonce, I don't have idToken so I will need to debug verifyPresentations . 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. As mentioned it needs to end either in the id token and/or vp token depending om which token(s) are requested. Since this comes from the OID request object it should never end up in oauth2 response payload directly 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. So if there is no request object then there indeed is no nonce typically 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 reverted this. After retest with newer libraries this appears to be working again. |
||
expires_in: responseOpts.expiresIn || 3600, | ||
state, | ||
}; | ||
|
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 is wrong! If the above hasVpToken is true, there needs to be a submission. So either the logic to determine the hasVpToken is wrong, or something else is happening. This PR is wrong for sure
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 found the submission in authorizationResponsePayload.presentation_submission not in presentationExchange. So this needs to be fixed elsewhere then.
So that that makes this PR is not to merge, just for diagnostics. I will revert the change in my branch as soon as presentationSubmission appears in presentationExchange
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.
No you are likely simply misunderstanding how it works. Presentation Exchange is a service that provides the submission, but only when a vp_token is needed
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 have found the problem, in the SSI SDK version I am using with RSA support, presentationSubmission in OPSession is not loaded yet.
But this line still has to be in opts
It's asserted in assertValidVerifiablePresentations
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.
No that line should not be in opts
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.
Please reread from the top. Whenever a vp_token is requested by the RP a PE definition needs to be present. Then we require the PresentationExchange service to be present. That one can create the Presentation Submission using the definition from the RP and the VCs provided to it.
There is no situation where there is no submission or an empty submission when a vp_token is requested and thus a PE definition being present.
That is literally why those guards are in place
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 I see now it has the ... before responseOpts.presentationExchange so it should be in opts already. So the problem was my OPSession which was from before October 13th and I patched before it up by getting it from elsewhere.
I reverted that code