added presentation_definition endpoint #2492

woutslakhorst · 2023-09-15T13:30:00Z

This RP adds the presentation definition endpoint and client API. A new variable has been added to the OAuth Authorization Server Metadata: presentation_definition_endpoint

gerardsn

dealing with some of comments should probably deferred

auth/api/iam/api.go

docs/_static/auth/iam.yaml

auth/api/iam/api.go

gerardsn · 2023-09-22T09:39:53Z

docs/_static/auth/iam.yaml

+                type: array
+                items:
+                  "$ref": "#/components/schemas/PresentationDefinition"


OpenID4VP flow requires a single PD for a similar request. Perhaps we should combine the different PDs in to single PD and add a group label per PD source. This allows selection of a single group to comply with within the combined PD using Submission Requirements.

I'd like 1 PE flow instead of 1 per backend as well. I think this example illustrates what we want to do: https://identity.foundation/presentation-exchange/spec/v2.0.0/#submission-requirement-1

{ "presentation_definition": { "input_descriptors": [ { "name": "Geregistreede zorgorganisatie", "purpose": "Verificatie dat de zorgorganisatie erkend is door CIBG", "group": ["local", "mitz"], "constraints": "has NutsCareOrganization" }, { "name": "Mitz Toestemmingen", "purpose": "Mag gebruikmaken van toestemmingen in Mitz", "group": ["mitz"], "constraints": "has MitzCredential" } ] }, "submission_requirements": [ { "name": "Toegang tot bronsysteem", "rule": "pick", "min": 1, "from_nested": [ { "rule": "all", "from": "local" }, { "rule": "all", "from": "mitz" } ] } ] }

Note the min property, requesting at least one matching credential set. The PD backends then don't yield presentation definitions, but a list of input descriptors. Or they could still return a PD, but the Nuts node would only use the input descriptors and discard the rest.

Con: more complex on implementation of the PE spec side ("our" side). But also less complex on the XIS vendor's side (our users), and less calls going back and forth (thus, faster).

I checked the PE issue tracker on Github, and there does not seem to be discussion about whether to remove/keep supporting pick or from_nested

auth/api/iam/api.go

auth/api/iam/client.go

reinkrul · 2023-09-24T05:32:24Z

auth/api/iam/client.go

+// We pass the endpoint url for the presentation definition endpoint because we already retrieved the metadata in a previous step.
+// The scopes are evaluated as raw query params and encoded if needed.
+func (hb HTTPClient) PresentationDefinition(ctx context.Context, definitionEndpoint string, scopes []string) ([]PresentationDefinition, error) {
+	presentationDefinitionURL, err := url.Parse(definitionEndpoint)


if you want a url.URL, you can just ask for it (string -> url.URL)?

You get it as a string from the metadata, so it would move the parsing to the caller. That would duplicate parsing code for every call.

Maybe we should change the type in the metadata to url.URL, then?

Putting ur.URL in the metadata will require custom marshalling/unmarshalling for the metadata struct.

auth/api/iam/client.go

docs/_static/auth/iam.yaml

added tests refactor of http Test TLS Server added comment add a DIDToURL method for webdid added comment added test case PR feedback test fix remove generated client

added implementation of the presentation definition endpoint test fix added API client for presentation definition

reinkrul · 2023-10-03T12:40:24Z

auth/api/iam/api.go

+		return PresentationDefinition400JSONResponse{
+			Error: "invalid_scope",
+		}, nil


Suggested change

return PresentationDefinition400JSONResponse{

Error: "invalid_scope",

}, nil

return OAuth2Error{

Code: InvalidScope,

}, nil

Compile error due to missing methods on that type (oapi-codegen specific)

reinkrul · 2023-10-03T12:41:25Z

auth/api/iam/client.go

+// We pass the endpoint url for the presentation definition endpoint because we already retrieved the metadata in a previous step.
+// The scopes are evaluated as raw query params and encoded if needed.
+func (hb HTTPClient) PresentationDefinition(ctx context.Context, definitionEndpoint string, scopes []string) ([]PresentationDefinition, error) {
+	presentationDefinitionURL, err := url.Parse(definitionEndpoint)


Maybe we should change the type in the metadata to url.URL, then?

auth/api/iam/types.go

docs/_static/auth/iam.yaml

reinkrul · 2023-10-03T13:24:37Z

docs/_static/auth/iam.yaml

+                type: array
+                items:
+                  "$ref": "#/components/schemas/PresentationDefinition"


I'd like 1 PE flow instead of 1 per backend as well. I think this example illustrates what we want to do: https://identity.foundation/presentation-exchange/spec/v2.0.0/#submission-requirement-1

{ "presentation_definition": { "input_descriptors": [ { "name": "Geregistreede zorgorganisatie", "purpose": "Verificatie dat de zorgorganisatie erkend is door CIBG", "group": ["local", "mitz"], "constraints": "has NutsCareOrganization" }, { "name": "Mitz Toestemmingen", "purpose": "Mag gebruikmaken van toestemmingen in Mitz", "group": ["mitz"], "constraints": "has MitzCredential" } ] }, "submission_requirements": [ { "name": "Toegang tot bronsysteem", "rule": "pick", "min": 1, "from_nested": [ { "rule": "all", "from": "local" }, { "rule": "all", "from": "mitz" } ] } ] }

Note the min property, requesting at least one matching credential set. The PD backends then don't yield presentation definitions, but a list of input descriptors. Or they could still return a PD, but the Nuts node would only use the input descriptors and discard the rest.

Con: more complex on implementation of the PE spec side ("our" side). But also less complex on the XIS vendor's side (our users), and less calls going back and forth (thus, faster).

I checked the PE issue tracker on Github, and there does not seem to be discussion about whether to remove/keep supporting pick or from_nested

woutslakhorst · 2023-10-09T07:25:48Z

The PE suggestion won't work since you don't know which one of the backends you pick will actually give you access. The patient specific scope will only be checked after a client submitted a VP. If a client picks the wrong one, it'll fail and does not retry. There's no complexity on the CIS side since it's all handled by the node.

woutslakhorst · 2023-10-12T06:39:31Z

#2540 describes additional features required for Submission Requirement Feature.

gerardsn

just the one comment

gerardsn · 2023-10-17T09:57:40Z

auth/api/iam/client.go

+
+// PresentationDefinition retrieves the presentation definition from the presentation definition endpoint (as specified by RFC021) for the given scope.
+func (hb HTTPClient) PresentationDefinition(ctx context.Context, definitionEndpoint string, scopes []string) (*PresentationDefinition, error) {
+	presentationDefinitionURL, err := url.Parse(definitionEndpoint)


The URL comes from an external source. Validate using core.ParsePublicURL to require https, and no reserved addresses like localhost.

I also changed the parsing so it will allow IPs in non-strictmode

gerardsn

I don't think allowing IP in non-strictmode is going to be possible since parsing of did:web fails for IP addresses. But it might make unit tests easier if this is allowed?

gerardsn · 2023-10-25T08:48:31Z

auth/api/iam/client.go

@@ -85,7 +85,7 @@ func (hb HTTPClient) OAuthAuthorizationServerMetadata(ctx context.Context, webDI

 // PresentationDefinition retrieves the presentation definition from the presentation definition endpoint (as specified by RFC021) for the given scope.
 func (hb HTTPClient) PresentationDefinition(ctx context.Context, definitionEndpoint string, scopes []string) (*PresentationDefinition, error) {
-	presentationDefinitionURL, err := url.Parse(definitionEndpoint)
+	presentationDefinitionURL, err := core.ParsePublicURL(definitionEndpoint, !hb.config.Strictmode)


The third input was never intended to be optional, but having to add the scheme every time is annoying since strictmode forces the following

if strictmode { issuerURL, err = core.ParsePublicURL(issuer, false, "https") } else { issuerURL, err = core.ParsePublicURL(issuer, true, "https", "http") }

Merging #2560 should make this easier

Hmmmm #2560 flipped the 2nd argument, although the PR build succeeds, after merging it'll fail....

woutslakhorst · 2023-10-25T13:09:46Z

@reinkrul waiting for your review

* refactor ParsePublicURL * add tests

reinkrul

Small remarks

auth/api/iam/error.go

woutslakhorst force-pushed the feature/2491/presentation_definition_endpoint branch 4 times, most recently from 58c449a to 3da08a9 Compare September 21, 2023 06:56

woutslakhorst marked this pull request as ready for review September 21, 2023 06:58

woutslakhorst requested review from gerardsn and reinkrul September 21, 2023 06:58

gerardsn requested changes Sep 22, 2023

View reviewed changes

reinkrul requested changes Sep 24, 2023

View reviewed changes

woutslakhorst mentioned this pull request Sep 27, 2023

use presentation definition endpoint to get presentation definition from other party #2491

Closed

woutslakhorst added 4 commits October 3, 2023 14:34

added api client for getting OAuth Authorization Server metadata

174ce93

added tests refactor of http Test TLS Server added comment add a DIDToURL method for webdid added comment added test case PR feedback test fix remove generated client

added presentation_definition endpoint to the metadata

39f66c2

added implementation of the presentation definition endpoint test fix added API client for presentation definition

PR feedback

96a0ffb

changed scope usage to only use first main scope

9b1c8cf

woutslakhorst force-pushed the feature/2491/presentation_definition_endpoint branch from 1cdae8c to 9b1c8cf Compare October 3, 2023 12:35

PD client now returns custom error on invalid scope

ed57c44

reinkrul requested changes Oct 5, 2023

View reviewed changes

PR feedback

ff6f47c

woutslakhorst requested review from gerardsn and reinkrul October 9, 2023 07:28

changes due to updated spec

36b1d96

gerardsn requested changes Oct 17, 2023

View reviewed changes

PR feedback

b9922ba

woutslakhorst requested a review from gerardsn October 24, 2023 07:18

gerardsn approved these changes Oct 25, 2023

View reviewed changes

woutslakhorst added 2 commits October 25, 2023 15:05

Merge branch 'master' into feature/2491/presentation_definition_endpoint

1ad5000

fix after merge conflicts

85d03ab

gerardsn and others added 2 commits October 25, 2023 15:13

Refactor ParsePublicURL (#2560)

7f620c8

* refactor ParsePublicURL * add tests

fix after rebase

6e60d27

reinkrul approved these changes Oct 25, 2023

View reviewed changes

auth/api/iam/error.go Outdated Show resolved Hide resolved

auth/api/iam/error.go Outdated Show resolved Hide resolved

PR feedback

9af960b

woutslakhorst merged commit 6d4bef3 into master Oct 26, 2023
6 checks passed

woutslakhorst deleted the feature/2491/presentation_definition_endpoint branch October 26, 2023 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added presentation_definition endpoint #2492

added presentation_definition endpoint #2492

woutslakhorst commented Sep 15, 2023 •

edited

Loading

gerardsn left a comment

gerardsn Sep 22, 2023

reinkrul Oct 3, 2023

reinkrul Sep 24, 2023

woutslakhorst Sep 27, 2023

reinkrul Oct 3, 2023

woutslakhorst Oct 9, 2023

reinkrul Oct 3, 2023

woutslakhorst Oct 9, 2023

reinkrul Oct 3, 2023

reinkrul Oct 3, 2023

woutslakhorst commented Oct 9, 2023 •

edited

Loading

woutslakhorst commented Oct 12, 2023

gerardsn left a comment

gerardsn Oct 17, 2023

woutslakhorst Oct 24, 2023

gerardsn left a comment

gerardsn Oct 25, 2023

gerardsn Oct 25, 2023

woutslakhorst Oct 25, 2023

woutslakhorst commented Oct 25, 2023

reinkrul left a comment

added presentation_definition endpoint #2492

added presentation_definition endpoint #2492

Conversation

woutslakhorst commented Sep 15, 2023 • edited Loading

gerardsn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woutslakhorst commented Oct 9, 2023 • edited Loading

woutslakhorst commented Oct 12, 2023

gerardsn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerardsn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woutslakhorst commented Oct 25, 2023

reinkrul left a comment

Choose a reason for hiding this comment

woutslakhorst commented Sep 15, 2023 •

edited

Loading

woutslakhorst commented Oct 9, 2023 •

edited

Loading