openid / OpenID4VP

56 stars 20 forks source link

Permit the use of the new query language instead of presentation exchange. #258

Closed martijnharing closed 4 weeks ago

martijnharing commented 2 months ago

Remove requirements to use the presentation exchange specification. Resolves #255

Sakurann commented 2 months ago

@David-Chadwick we can introduce an additional type parameter later on, at this stage, all we are trying to do is to prevent adding a new query language being a breaking change if we do it in 1.1 and not 1.0. so I think the changes Martijn is proposing are actually the simplest, cleanest thing we can/should do now

David-Chadwick commented 2 months ago

@Sakurann Adding the type parameter later on will be a breaking change. Adding it now will ensure extensibility in a non-breaking way for 1.1, 2, 2.1, 3 etc ad infinitum

c2bo commented 2 months ago

@Sakurann Adding the type parameter later on will be a breaking change. Adding it now will ensure extensibility in a non-breaking way for 1.1, 2, 2.1, 3 etc ad infinitum

In principle I agree that adding a query language type parameter would be beneficial IMHO.

Do you think it makes sense to introduce the query type parameter with the new query language PR? We can also add it as an optional parameter and defaulting to presentation exchange if not present - that way it would be a simple extension without any breaking changes.

c2bo commented 2 months ago

We should probably also adjust these lines:

244

The Verifier articulates requirements of the Credential(s) that are requested using presentation_definition and presentation_definition_uri parameters that contain a Presentation Definition JSON object as defined in Section 5 of [@!DIF.PresentationExchange]. Wallet implementations MUST process Presentation Definition JSON object and select candidate Verifiable Credential(s) using the evaluation process described in Section 8 of [@!DIF.PresentationExchange] unless implementing only a credential profile that provides rules on how to evaluate and process [@!DIF.PresentationExchange].

547

A VP Token is only returned if the corresponding Authorization Request contained a presentation_definition parameter, a presentation_definition_uri parameter, or a scope parameter representing a Presentation Definition (#vp_token_request).

Apart from that looking good imho.

jogu commented 2 months ago

I think a type parameter is a separate discussion, and this PR also leaves the door open to adding a type parameter in the future (i.e. with language like 'if type isn't present look for presentation_definition parameter for backwards compatibility').

I think I am missing what the key advantage of a type parameter is over (if we really need to do it and I'm really hoping we don't) having a oid_query_language parameter and a oid_query_language_v2 parameter. And the latter actually seems more flexible to me as it leaves open the option that the verifier can use two query languages at the same time if there's a reason to (perhaps during a transition period where it doesn't know which one a wallet will support).

Sakurann commented 2 months ago

also agree that type parameter discussion requires a separate issue

David-Chadwick commented 2 months ago

I think I am missing what the key advantage of a type parameter is

It provides extensibility and backwards compatibility for ever. As requirements change, it automatically meets them. It is a well known and well recognised way of adding extensibility.

David-Chadwick commented 2 months ago

having a oid_query_language parameter and a oid_query_language_v2 parameter.

How does that cater for oid_query_language_v3 which there is sure to be, given the history of the last two years? And if the query parameter is made a set of type(s) and value(s), then you can have any number of query languages in the request

jogu commented 2 months ago

having a oid_query_language parameter and a oid_query_language_v2 parameter.

How does that cater for oid_query_language_v3 which there is sure to be, given the history of the last two years? And if the query parameter is made a set of type(s) and value(s), then you can have any number of query languages in the request

You can add a oid_query_language_v3 when it's needed. For clarity, this also works for v4, v5, v6, v7 and every version beyond that. https://github.com/openid/OpenID4VP/pull/240 clarifies that adding new parameters can be done at anytime (although this particular case was already possible as ignoring unknown parameters is the standard OAuth behaviour).

I'm not supportive of changing the query language parameter to be an array. It's adding unnecessary complexity.

But I think we're off topic for this PR, anyway.

jogu commented 2 months ago

I agree with @c2bo that we need to do this:

We should probably also adjust these lines:

244

The Verifier articulates requirements of the Credential(s) that are requested using presentation_definition and presentation_definition_uri parameters that contain a Presentation Definition JSON object as defined in Section 5 of [@!DIF.PresentationExchange]. Wallet implementations MUST process Presentation Definition JSON object and select candidate Verifiable Credential(s) using the evaluation process described in Section 8 of [@!DIF.PresentationExchange] unless implementing only a credential profile that provides rules on how to evaluate and process [@!DIF.PresentationExchange].

547

A VP Token is only returned if the corresponding Authorization Request contained a presentation_definition parameter, a presentation_definition_uri parameter, or a scope parameter representing a Presentation Definition (#vp_token_request).

Apart from that looking good imho.

jogu commented 2 months ago

This change does not say what should be present if presentation_definition and presentation_definition_uri and scope are missing. The current text allows all to be missing. A must better solution is to have a type, followed by one of these three, and then in future a new type can be added

I don't think this is a good idea. Particularly not once it was suggested that means having both type and the query as arrays and defining some new rules around that.

We can instead change the language to 'If neither presentation_definition nor presentation_definition_uri nor a scope indicating a credential to be used nor any other parameter indicating what credential is being requested'. That's all that would be in scope for this PR. This language would allow 'type' to be added in the future if the working group decided to. This PR is just about giving us options later.

Sakurann commented 2 months ago

I am ok with "If neither presentation_definition, presentation_definition_uri, a scope indicating a credential to be used or any other parameter indicating what credential is being requested defined by this specification/WG." I think it is important that we are not opening a rabbithole but leaving an option for this WG/specification to define such 4th parameter

Sakurann commented 2 months ago

@David-Chadwick could you please take a look at my suggestions?

David-Chadwick commented 1 month ago

@Sakurann This new wording is a definite improvement. But as I understand it, any optional parameters can always be omitted and so any protocol messages without them are conformant to the spec. So a message without any of these optional parameters is conformant to the protocol, but such a message is useless as it does not tell the wallet which credentials are needed. Thus in my opinion the specification is broken if an implementation can create a conformant message that is useless. So what still needs to be added to the text is a sentence along the lines of "the request message must contain at least one indication of which credentials need to be provided by the wallet"

Sakurann commented 1 month ago

@David-Chadwick I tried bring clarity along the lines you suggest here: https://github.com/openid/OpenID4VP/pull/258/files#r1766818286

martijnharing commented 1 month ago

I'll repeat with a change request so we don't forget this - IMHO we need to adjust these lines:

244

The Verifier articulates requirements of the Credential(s) that are requested using presentation_definition and presentation_definition_uri parameters that contain a Presentation Definition JSON object as defined in Section 5 of [@!DIF.PresentationExchange]. Wallet implementations MUST process Presentation Definition JSON object and select candidate Verifiable Credential(s) using the evaluation process described in Section 8 of [@!DIF.PresentationExchange] unless implementing only a credential profile that provides rules on how to evaluate and process [@!DIF.PresentationExchange].

547

A VP Token is only returned if the corresponding Authorization Request contained a presentation_definition parameter, a presentation_definition_uri parameter, or a scope parameter representing a Presentation Definition (#vp_token_request).

I changed those two lines to be in compliance with the others changes. Do these changes work for you?

jogu commented 1 month ago

Discussed on today's call - the new query language will still use vp_token, but we have a separate issue to address vp_token https://github.com/openid/OpenID4VP/issues/249

Sakurann commented 1 month ago

discussed on the wg call, agreed to modify the PR in the direction that allows to change the structure of VP Token in the future. and change defined by this specification to defined by this specification and all its future version

David-Chadwick commented 1 month ago

and all its future versions

i.e. plural

danielfett commented 1 month ago

To elaborate what I said on the call, the following should be our target:

From the view of a verifier,

From the view of a wallet,

To achieve this, the current spec without this PR is fine. For 1.1, we can decide if PE needs to be supported by every wallet and what the options for verifiers are. We don't need to decide that now.

Edited to add:

David-Chadwick commented 1 month ago

From the view of a wallet,

when I implement 1.0, ... if somebody sends a parameter with a query in a new query language, I'll ignore it, as I don't understand it.

And then I have a request with no query in it. So what reply do I send to the RP? Protocol error?

Ignoring it is the wrong behaviour. Rather the behaviour we want is: if somebody sends a parameter with a query in a new query language, I will know this has been standardised in a future version of the standard that I have not yet implemented, so I will reply "query language not implemented" or similar

tplooker commented 1 month ago

To elaborate what I said on the call, the following should be our target:

Appreciate the write up as I think its important to describe exactly what will be normatively required here in v1.0 vs v1.1. I think the most important question to answer is could a v1.1 RP (verifier) implementation and wallet use only the new query syntax if they wanted to OR would an RP effectively have to have the credential query repeated twice in the request one in the new format and one in the old (P.E)?

IMO if we take the position in 1.0 that the query syntax supported by both implementations is negotiate-able rather then fixed then we could allow v1.1 implementations to only use P.E if they so wished, without it being a breaking change. Otherwise IMO we are stuck in a situation where a P.E based query has to be in the request of every v1.x to remain backward compatible.

tlodderstedt commented 1 month ago

Negotiation requires the POST request based approach, right? How should that work for the DC API? Is the RP supposed to send two request objects at once?

javereec commented 1 month ago

I was wondering if we also should relax the link of scope to Presentation Definition. The current text reads

Using scope Parameter to Request Verifiable Credential(s) {#request_scope}

Wallets MAY support requesting presentation of Verifiable Credentials using OAuth 2.0 scope values.

Such a scope value MUST be an alias for a well-defined Presentation Definition that will be referred to in the presentation_submission response parameter.

In the future it could reference the new query language

Sakurann commented 1 month ago

The way the discussion has gone,

David-Chadwick commented 1 month ago

if PE is the only query language in 1.0 (ie mandatory in 1.0).... introduce new query language in 1.1 .in other words 1.1 is still a breaking change to 1.0.,

I do not think that these two statements logically follow. PE is the only query language in 1.0, this is true. But in other versions there are other languages. So if a 1.0 implementation knows this, which the 1.0 specification can make clear, then if it receives another query language it knows it is not talking to a 1.0 implementation. But this is not a breaking change, because a 1.0 implementation can code for this now.

Sakurann commented 4 weeks ago

I do not think that these two statements logically follow. PE is the only query language in 1.0, this is true. But in other versions there are other languages. So if a 1.0 implementation knows this, which the 1.0 specification can make clear, then if it receives another query language it knows it is not talking to a 1.0 implementation. But this is not a breaking change, because a 1.0 implementation can code for this now.

If PE is not mandatory in versions after 1.1, an implementation that built following 1.1 will not be able to interoperate with implementation following 1.0...

Sakurann commented 4 weeks ago

superseded by https://github.com/openid/OpenID4VP/pull/266

David-Chadwick commented 4 weeks ago

If PE is not mandatory in versions after 1.1, an implementation that built following 1.1 will not be able to interoperate with implementation following 1.0...

It depends what you mean by interoperate. If you mean, talk to each other and know how to respond with a conforming response, then it is not a breaking change. If you mean provide a set of credentials in response, then it is a breaking change. But there are many cases of the latter in the existing specification such as: support for different DIDs, different cryptographic functions, different credential schemas/formats. So support for different query formats is just one of many "breaking changes" that already exist.