-
Notifications
You must be signed in to change notification settings - Fork 16
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
Credential format profiles: 'claims' definitions a̶r̶e̶ ̶b̶r̶o̶k̶e̶n̶ should be improved #266
Comments
Some of the solutions discussed on the last call:
I personally would prefer 1.3 or 1.1 and provide an option for 3. |
To help clarify the issue:
We favor Option 1.1, but we observe there might be some obscure use cases not covered by JSON Pointer (description for objects within arrays). Early implementation experience would be handy. We also favor Option 3 and see 1 as the default fallback option if a credential format does not further specify detailed metadata. |
I created a PR on VC Type Metadata to see what complexity solution 1.3 proposed by @bc-pi would bring and I think it is comparable to or easier to implement than the JSONPointer solution considering that the JSONPointer solution would require a workaround for arrays and entail escaping etc.. This is what the metadata could look like: https://vcstuff.github.io/sd-jwt-vc-types/danielfett/fix-claims/draft-fett-oauth-sd-jwt-vc-types.html#section-2 "claims":[
{
"path":["name"],
"display":{...}
},
{
"path":["address"],
"display":{...}
},
{
"path":["address", "street_address"],
"display":{...}
},
{
"path":["degrees", null],
"display":{...}
}
], And this is how to parse the (PR in SD-JWT VC Type Metadata) |
I don't think we currently use 'null' JSON values anywhere in the VCI spec. I would argue that JSON nulls are generally problematic and should be avoided as many JSON libraries (at least by default) consider 'null' to be equivalent to 'not present'. Perhaps we could follow the precedent set in the SD JWT spec with |
Option 1.4: |
For what it's worth, https://www.rfc-editor.org/rfc/rfc7592.html#section-2.2 treats For similar reasons, the OpenID Federation spec states that This is rooted in Java and Python behaviors where "getting" a value from a structure returns |
A |
I have a really hard time imagining how null could be a problem in this specific context, but -1 would be the second best option. |
I would prefer option 2, introduce a |
Agree it is a problem that should be addressed and support the proposed option 1.3 (aka 1.iii) "Use arrays as pointers." It also seems worthwhile to consider allowing for something along the lines of option 3. And have SD-JWT VC Type Metadata also use an arrays as pointers approach. |
Perhaps I am misunderstanding JSON Pointer, but from my understanding you can reference objects within an array? Wouldn't this work?
I don't fully understand or see the benefit of going with the Option 1.3 over 1.1 but I might very well be missing something. |
The problem is that you might want to say "every element in that array". So "/data/*/some" (which is not valid JSONPointer). More like a query language... |
My preference would be
It keeps things simple, closely linked to the attribute and straightforward to implement, improving adoption. |
Consensus in the call was to go ahead with solution 1.3 following the SD-JWT VC Type Metadata mechanism (and then collecting initial implementer's feedback before proceeding). If everyone could please review this section in VC Type Metadata and provide feedback before I create the PR for VCI on Monday, that would be very helpful: https://vcstuff.github.io/sd-jwt-vc-types/danielfett/fix-claims/draft-fett-oauth-sd-jwt-vc-types.html#name-claim-metadata PR for commenting here: vcstuff/sd-jwt-vc-types#5 |
I presume we'll carry across the 'mandatory' and 'value_types' fields that are in the current VCI draft? I'm not sure if it's clear whether or not the proposal is to add the 'sd' and 'verification' fields that are defined in the above link to VCI? (And as discussed in the call, this change would mean we can remove the 'order' parameter that's in the current spec.) |
We have been working on a similar problem in the context of a format-specific query syntax. Perhaps that work can help inform thinking and conversation around describing claim structures for different formats (IETF SD-JWT VC, W3C VCDM 2.0, and ISO mdocs) in JSON and vice versa. Here's a the link: https://docs.google.com/document/d/10JT--pXWsfwC4QVu3XJpXGwcO08M6tnpkZ4PKdtCEWo |
yes
I think we wouldn't use those.
Which has its own problems. |
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. So our preference is actually approach 2 with defining the new property As a compromise, my suggestion would be fixing the text that becomes ID-1 with |
As discussed on yesterday's working group call: #266 (comment) As it seems likely that #276 will not make it into implementer's draft 1, we instead add warnings about the known limitations of the current data structure so that implementer's are aware. Note that I have not applied this to the mdoc profile section, as that sections seems not to allow nested objects in the metadata.
@danielfett can you please change the issue name from "is broken" to something like "needs improvement"? Implementation feedback showed that the current structure works for majority of the current use-cases, so broken feels too strong and potentially misleading. thank you! |
Right now, the definitions (e.g., this) for
claims
/credentialSubject
in the credential format profiles are broken (as pointed out earlier).At least the following problems exist:
mandatory
and the syntax elementmandatory
(same fordisplay
etc.).address
claim and theaddress/region
sub-claim.This is also related to this issue, both go back to not properly accounting for nested structures.
The problem exists in all three profiles defined in Appendix A.
The text was updated successfully, but these errors were encountered: