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.

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

Define claims display description and claims path query #276

Merged
merged 27 commits into from
Dec 17, 2024

Conversation

danielfett
Copy link
Contributor

@danielfett danielfett commented Feb 20, 2024

@danielfett danielfett marked this pull request as ready for review February 21, 2024 07:44
Copy link
Contributor

@mickrau mickrau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After review in our team: In principle, we like the proposal and support it.
We left two comments regarding claims object display property and value_type.

}
{"path": ["email"]},
{"path": ["phone_number"]},
{"path": ["address", "street_address"]},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would like to give display properties to an object of claims. The wallet could use this property when collapsing the object's claims in the UI. Is this possible and would the following be the right way?
If so, this should be part of an example.

                  {
                    "path": ["address"],
                    "display": [
                        {
                            "name": "Address",
                            "locale": "en-US"
                        },
                        {
                            "name": "Adresse",
                            "locale": "de-DE"
                        }
                    ],
                    "value_type": "object"
                },
                {"path": ["address", "street_address"]},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for this! I think supporting this is mandatory for having a clear and defined way to present this kind of data to the user. Let's use the example from your documentation and extend it with an arbitrary example:

... 
"address": {
    "street_address": "42 Market Street",
    "city": "Milliways",
    "postal_code": "12345"
},
...
"work_address":  {
   "city": "Berlin",
  ...
},

I would expect/imagine this to be displayed in the ui similar to this:

place of residence
+- street: 42 Market Street
+- city: Milliways
`- post code: 12345
....
place of work
+- city: Berlin
+...

@@ -2215,7 +2194,7 @@ The following is a non-normative example of an object comprising the `credential
The following additional claims are defined for authorization details of type `openid_credential` and this Credential format.

* `vct`: REQUIRED. String as defined in (#server-metadata-sd-jwt-vc). This claim contains the type values the Wallet requests authorization for at the Credential Issuer. It MUST only be present if the `format` claim is present. It MUST not be present otherwise.
* `claims`: OPTIONAL. Object as defined in (#server-metadata-sd-jwt-vc) excluding the `display` and `value_type` parameters. `mandatory` parameter here is used by the Wallet to indicate to the Issuer that it only accepts Credential(s) issued with those claim(s).
* `claims`: OPTIONAL. An array of claims description objects as defined in (#claims-description), excluding the `display` and `value_type` parameters. The `mandatory` parameter here is used by the Wallet to indicate to the Issuer that it only accepts Credential(s) issued with those claim(s).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `claims`: OPTIONAL. An array of claims description objects as defined in (#claims-description), excluding the `display` and `value_type` parameters. The `mandatory` parameter here is used by the Wallet to indicate to the Issuer that it only accepts Credential(s) issued with those claim(s).
* `claims`: OPTIONAL. An array of claims path queries as defined in (#claims-path-query). The parameter is used by the Wallet to indicate to the Issuer that it only accepts Credential(s) issued containing exactly those claim(s).

openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved
openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved
openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@Sakurann Sakurann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put a comment in issue #266, but after today's WG call where a concern that this is a very big breaking change was raised, we discussed this internally with Microsoft engineers. We do not currently have use-cases with the claims that have nested structure and array-based solution is too big of a breaking change since we just finished implementing OpenID4VCI based on the text that went to ID-1 voting. So our preference is actually defining the new property nested which may contain descriptions about nested objects (original approach 2).

As a compromise for the existing implementations, my suggestion would be fixing the text that becomes ID-1 with nested approach. and than after ID-1, incorporating the array approach defined in this PR.

@danielfett
Copy link
Contributor Author

So our preference is actually defining the new property nested which may contain descriptions about nested objects (original approach 2).

Then we need a new PR for that.

As a compromise for the existing implementations, my suggestion would be fixing the text that becomes ID-1 with nested approach. and than after ID-1, incorporating the array approach defined in this PR.

That would mean two breaking changes.

wallet did not request the inclusion of the claim, and/or if the Credential
Issuer chose to not include the claim. If the `mandatory` parameter is
omitted, the default value is `false`.
* `value_type`: OPTIONAL. String value determining the type of value of the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1


In detail, the array is processed from left to right as follows:

1. Select the root element of the Credential, i.e., the top-level JSON object.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the W3C VCDM 1.1, a credential can be represented by a JWT with the object following the data model inside a "vc" claim (https://www.w3.org/TR/vc-data-model/#example-usage-of-the-id-property). It presume that the root element of the Credential refers to the value of the "vc" claim and not to the JWT payload object, right? If so, should we be explicit about it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess a similar question may be made when using SD-JWT with JWS JSON Serialization (https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-14.html#section-8) - the root element of the Credential is the JWS payload object and not the top-level JSON object of the serialized credential, right?

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'll change the wording to "Select the root element of the Credential, i.e., the top-level object of the
JSON structure representing the claims in the Credential"

Copy link
Contributor

@paulbastian paulbastian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to exclude the value_type, as it seems under-specified and is not present in any example. Otherwise, I'd be ready to approve.

@danielfett
Copy link
Contributor Author

I would prefer to exclude the value_type, as it seems under-specified and is not present in any example. Otherwise, I'd be ready to approve.

Done

Copy link
Member

@selfissued selfissued left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Please answer my questions on particular lines before merging.

Copy link
Contributor

@awoie awoie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't like the path syntax for ISO mdocs but @leecam convinced me that it would actually save some code.

Copy link
Member

@c2bo c2bo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move the other discussion into separate issues - definitely a big improvement to align with the dcql syntax

Copy link

@sloops77 sloops77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed to move outstanding discussions to other tickets

@Sakurann Sakurann merged commit ba24382 into main Dec 17, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet