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.

Sign up for GitHub

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

Jump to bottom

MOSIP fixes #75

Closed
wants to merge 15 commits into from
Closed

MOSIP fixes #75

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
221f9a3
SPHEREON-1157: include presentationSubmission in auth response, inclu…
sanderPostma Feb 28, 2024
6285f12
SPHEREON-1157: verifiedIdToken can be null
sanderPostma Feb 28, 2024
5d75581
SPHEREON-1157: prettier
sanderPostma Feb 28, 2024
625de09
SPHEREON-1157: requestObject can be undefined. (At least in tests)
sanderPostma Feb 28, 2024
e133e96
SPHEREON-1157: removed rogue paste
sanderPostma Feb 28, 2024
6bae0d4
SPHEREON-1157: responseOpts.presentationExchange can be undefined
sanderPostma Feb 28, 2024
66358b3
SPHEREON-1157: responseOpts.presentationExchange can be undefined
sanderPostma Feb 28, 2024
e16d20b
SPHEREON-1157: deleted rogue paste
sanderPostma Feb 28, 2024
a026323
SPHEREON-1157: responseOpts.presentationExchange can be undefined
sanderPostma Feb 28, 2024
b74c350
SPHEREON-1157: presentationSubmission optional
sanderPostma Feb 28, 2024
142c25b
SPHEREON-1157: response_uri support
sanderPostma Mar 1, 2024
27ad7ef
v0.6.0-unstable.10
sanderPostma Mar 4, 2024
ffe4f96
Merge branch 'develop' into fix/SPHEREON-1157_MOSIP-demo-fixes
sanderPostma Mar 6, 2024
e28f000
SPHEREON-1157: remove unnecessary code
sanderPostma Mar 6, 2024
9426648
SPHEREON-1157: remove unnecessary code
sanderPostma Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/authorization-response/AuthorizationResponse.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,14 +115,16 @@ export class AuthorizationResponse {

if (hasVpToken) {
const wrappedPresentations = await extractPresentationsFromAuthorizationResponse(response, { hasher: verifyOpts.hasher });
const presentationSubmission =
responseOpts.presentationExchange?.presentationSubmission ?? authorizationResponsePayload.presentation_submission;

await assertValidVerifiablePresentations({
presentationDefinitions,
presentations: wrappedPresentations,
verificationCallback: verifyOpts.verification.presentationVerificationCallback,
opts: {
...responseOpts.presentationExchange,
presentationSubmission: responseOpts.presentationExchange?.presentationSubmission ?? authorizationResponsePayload.presentation_submission,
...(presentationSubmission ? { presentationSubmission: presentationSubmission } : {}),
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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

Copy link
Contributor Author

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.
image

But this line still has to be in opts

          ...(presentationSubmission ? { presentationSubmission: presentationSubmission } : {}),

It's asserted in assertValidVerifiablePresentations

  } else if (args.presentationDefinitions && !args.opts.presentationSubmission) {
    throw new Error(`No presentation submission present. Please use presentationSubmission opt argument!`);
  } else if (args.presentationDefinitions && presentationsWithFormat) {

Copy link
Contributor

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

Copy link
Contributor

@nklomp nklomp Mar 6, 2024 •
edited

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

Copy link
Contributor Author

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

hasher: verifyOpts.hasher,
},
});
Expand Down