-
Notifications
You must be signed in to change notification settings - Fork 112
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
Sync and fix bugs in v2 context. #1259
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just trying to test my own understanding, @dlongley @msporny. My JSON-LD knowledge is rusty.
Setting
@context
tonull
means that, at this point, that the active context is reduced to nothing, except some basic mapping specified by the JSON-LD spec (like@type
mapping to a URL). Per our spec, the value of theverifiableCredential
property in JSON-LD is supposed to be aVerifiableCredential
(which is, in the RDF side, enclosed in a separate graph). The effect of this is new setting that unless the@type
is explicitly set toVerifiableCredential
in the value object things will, and should, go wrong. Put it another way, it reinforces the requirement that the value object must explicitly declare itself to be a Verifiable Credential (and relates to the fact that the vocabulary explicitly says that a VerifiableCredentialGraph should contain a single Verifiable Credential).Is this indeed the goal of this extra setting? If so, shouldn't there be a similar setting in the security
@context
for theproof
property?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like it would make some of these examples illegal:
https://w3c.github.io/vc-jose-cose/#presentations
Or are these ok, as long as they "dereference" to a compact JSON-LD object representing a conforming document for a
verifiable credential
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The processing rules don't allow it as the content is expected to be present (it will not be auto-dereferenced and even then, there are no content hashes).
Those examples were never legal in v1.0, v1.1, and v2.0... none of them are conforming documents as defined by the specification.
The addition is the null'ing of the context, which is required to address issue #1150. So, this change is purely additive and doesn't impact the examples you provided (the answers below would be the same w/o this PR).
That said:
If you pre-processed this URL and put this object into the array, it might be a conforming document. By itself, it's not a conforming document.
A DID Document isn't a VC, so, no, that wouldn't work, AFAICT. It's not a conforming document.
Presuming this thing is an identifier for a graph, you could potentially express it, but then it's not clear how you'd dereference it or guarantee the security around the object unless you were to provide a content hash. This couldn't work unless one were to presume that there was some sort of "graph dereferencing" function in play. Again, not a conforming document.
Same answer as the above.
If you pre-processed this URL and put the resulting object into the array, it might be a conforming document. By itself, it's not a conforming document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see spec text addressing this, its not obvious that what you are saying is true, and these n-quads look fine to me:
This can't be true, the only normative requirement for a DID Document is that it have an
id
that is a DID... that makes any VC that has anid
that is a DID a DID Document... especially if its returned with content typeapplication/did+json
(where context can be present and is ignored).... there are a lot of use cases where a controller document might need to be a VC. Consider also https://openid.net/specs/openid-connect-userinfo-vc-1_0-00.htmlThis is one step off of a signed did resolution result... we should not be designing incompatibility here, we don't want to preclude use cases we see developing in the wild.
Similar to DID Documents, you can't presume that DIDs can't be dereferenced, even though it's not defined normatively.
I didn't share the resolved document, but if I shared one that had an
id
that matched this, and was a status list credential, that would work... the verifier may have previously dereferenced the identifier, the holder does not need to send them a massive status list when thats the case.Same answer as above.
Its clear we don't have agreement on the domain / range of
verifiableCredential
... I see it as having the same structure asissuer
andholder
, orproof
... it can be an object, or an array of strings and objects.... similar to verification relationships.The v1 context seems to support this assertion.
This is the definition in the v2 context today:
For comparison, here are the context definitions for verification relationships:
As you can see the only difference is
@graph
vs@set
... this does not imply that the property must ALWAYS be an array of objects of type "VerifiableCredential".... afaik.It does mean that the n-quads produce blank nodes, as you can see from the example above.
There are lots of cases in RDF / JSON-LD where you may have previously dereferenced an identifier to a graph node / object... and you don't need to send a "full copy" of the same object over and over again... this is especially common in supply chain use cases cc @mprorock @mkhraisha ... we use this pattern when communicating references to previous "credentials" and "presentations" and "workflows" which are just another large JSON object that is referred to be "id".
TLDR, I am opposed to breaking changes to this experience in the v2 context.
I am fine leaving the behavior as it is above (where you can always replace an
id
with anobject
that contains thatid
), for places where you see"@type": "@id",
... This would preserve the experience we created with DIDs, where the same "verificationMethod" object is referred to by several "verification relationships", for example, a P-256 Public Key might be used for both authentication and encryption, or authentication and credential issuance.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There aren't breaking changes here --
verifiableCredential
has always used a@graph
container to ensure that the data expressed in the value there is put into its own graph. The VP has a relationship with other graph containers viaverifiableCredential
, where each value is its own graph containing either aVerifiableCredential
or other data conformant with the data model that is derived from aVerifiableCredential
. This latter bit was for the CL Signature ZKP folks that wanted to not necessarily put a VC directly in the value, but to put a "derived credential with ZKP proof data" in it. This has been the case since 1.1.If you just put a string value in that field, then all that happens is an empty graph container (with a blank node identifying it) is created and there's a relationship established between the VP and this empty graph. A string value (ID / URL) for a subject does not establish any claims or additional relationships so the new graph is just empty. The only relationship added is between the VP and the blank node of this empty graph via the
verifiableCredential
property (loosely, the claim is: VP -> verifiableCredential -> blank node of an empty graph). This empty graph is not a validVerifiableCredential
nor is it data derived from one, so it's not a valid use of the VCDM.But! There is a property that can do what you want to do in VCDM 2.0. I would recommend using
relatedResource
to add whatever additional resources you want -- and there's the added bonus of being able to include a content integrity hash to go along with it, which is something you'd want for that approach anyway. Hopefully this resolves the issue.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OR13,
I've also mentioned that we need to make sure to add
relatedResource
to both VCs and VPs here: #1263 (comment)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This AFAIK, never actually resulted in implementations.
Yes, unless we remove the
@container
, or change it to@set
, I expect this will continue to be the case.When you say
subject
here, you mean the identifier for a verifiable credential, (assuming it has graph node id), right?This seems to be confusing "VerifiableCredential" with "VerifiableCredentialGraph" cc @iherman & @msporny , we discussed confusion related to this on #1240
It would help to understand the normative / english language value of the "VerifiableCredentialGraph" since that is what is "empty".
I like the suggestion, lets not let it stop this PR, I will file a separate issue for follow up: #1265
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Staying on topic, I am opposed to adding the "null" context here: https://github.com/w3c/vc-data-model/pull/1259/files#diff-267b7e47789daf9a1d312b42826aa937a1bb5af4312ee90df6b8a64cd6426f4fR154
Until we achieve clarity / consensus on the topic above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if you wanted to strike that part ("or data derived from verifiable credentials"), I'd be ok with it and would favor just requiring embedded VCs.
Well, what the
subject
actually is doesn't matter, its identifier isn't associated with any statements because nothing is said about it (making the graph of statements "empty"). It's true that thesubject
could be a verifiable credential, but if nothing is claimed about that credential directly within the embedded data (as opposed to who knows what might be there by reference). In short, theverifiableCredential
property has not been for referencing things, but instead for directly embedding data-model-compliant things in a presentation whilst preserving a wrapper around them (a "named graph") to isolate them from one another.The
verifiableCredential
(property name) relationship is between a VP and a graph (VerifiableCredentialGraph`). This is what I said above when I said: "(loosely, the claim is: VP -> verifiableCredential -> blank node of an empty graph)". Of course, this embedded graph must not be empty to be valid, but I was pointing out the invalid case in that sentence.If I understand you, the value is to put a wrapper around a set of statements to isolate them from other statements made: to help keep track of associations. The statements made in a VC are "wrapped" by a separate graph (put "into" that graph) to keep them separate from statements made in any other VC embedded in the VP. A VP can carry N-many VCs this way, potentially coming from many different issuers, whilst tracking the association of the statements in the graph model.
+1