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

Sync and fix bugs in v2 context. #1259

Merged
merged 2 commits into from
Sep 11, 2023
Merged

Sync and fix bugs in v2 context. #1259

merged 2 commits into from
Sep 11, 2023

Conversation

msporny
Copy link
Member

@msporny msporny commented Aug 25, 2023

This PR is an attempt to address issue #1150 by ensuring that redefinition doesn't fail in verifiableCredential when holding a VC v1.x compliant VC. It also fixes two bugs in the v2 context: 1) ensures that redefinition doesn't fail for cnf, and 2) synchronizes the DataIntegrity scoped context terms to now include cryptosuiteString.

@@ -146,7 +150,8 @@
"verifiableCredential": {
"@id": "https://www.w3.org/2018/credentials#verifiableCredential",
"@type": "@id",
"@container": "@graph"
"@container": "@graph",
"@context": null
Copy link
Member

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 to null 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 the verifiableCredential property in JSON-LD is supposed to be a VerifiableCredential (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 to VerifiableCredential 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 the proof property?

Copy link
Contributor

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

{
  "@context": ["https://www.w3.org/ns/credentials/v2"],
  "type": ["VerifiablePresentation"],
  "holder": "urn:ietf:params:oauth:jwk-thumbprint:sha-256:_Fpfe27AuGmEljZE9s2lw2UH-qrZLRFNrWbJrWIe4SI",
  "verifiableCredential": [{
      "@context": [
        "https://www.w3.org/ns/credentials/v2"
      ],
      "type": [
        "VerifiableCredential"
      ],
      "issuer": "https://issuer.example/issuers/68",
      "validFrom": "2023-06-07T21:14:14.148Z",
      "credentialSubject": {
        "id": "https://subject.vendor.example"
      }
    },
    "https://vendor.example/credentials/42", "did:example:123",
    "urn:uuid:01ec9426-c175-4e39-a006-d30050e28214",
    "urn:ietf:params:oauth:jwk-thumbprint:sha-256:_Fpfe27AuGmEljZE9s2lw2UH-qrZLRFNrWbJrWIe4SI",
    "data:application/vc+ld+json;base64,eyJAY29udGV4dCI6WyJodHRwczovL3d3dy53My5vcmcvbnMvY3JlZGVudGlhbHMvdjIiXSwidHlwZSI6WyJWZXJpZmlhYmxlQ3JlZGVudGlhbCJdLCJpc3N1ZXIiOiJkaWQ6andrOmV5SnJhV1FpT2lKMWNtNDZhV1YwWmpwd1lYSmhiWE02YjJGMWRHZzZhbmRyTFhSb2RXMWljSEpwYm5RNmMyaGhMVEkxTmpwdlFtUm1kbVpET1hoNk1GOUJVWFpSTjNZMU1YbERXbDl6ZUdwNU56VkNUSEpJZWsxT1Jqa3lPV1U0SWl3aWEzUjVJam9pVDB0UUlpd2lZM0oySWpvaVJXUXlOVFV4T1NJc0ltRnNaeUk2SWtWa1JGTkJJaXdpZUNJNklqTmljbU5zYjBJNGFEUk5XbFZJYms5UVVHbGtTbXd0U2pkdVVsRkpXSFJUYUZwM1oyNW1jbHAxVDI4aWZRIiwidmFsaWRGcm9tIjoiMjAyMy0wNi0wN1QyMToxNDoxNC4xNDhaIiwiY3JlZGVudGlhbFN1YmplY3QiOnsiaWQiOiJodHRwczovL3N1YmplY3QudmVuZG9yLmV4YW1wbGUifX0="
  ]
}

Or are these ok, as long as they "dereference" to a compact JSON-LD object representing a conforming document for a verifiable credential ?

Copy link
Member Author

@msporny msporny Aug 26, 2023

Choose a reason for hiding this comment

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

Or are these ok, as long as they "dereference" to a compact JSON-LD object representing a conforming document for a verifiable credential?

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).

It seems like it would make some of these examples illegal:

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:

"https://vendor.example/credentials/42"

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.

"did:example:123",

A DID Document isn't a VC, so, no, that wouldn't work, AFAICT. It's not a conforming document.

"urn:uuid:01ec9426-c175-4e39-a006-d30050e28214",

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.

"urn:ietf:params:oauth:jwk-thumbprint:sha-256:_Fpfe27AuGmEljZE9s2lw2UH-qrZLRFNrWbJrWIe4SI",

Same answer as the above.

"data:application/vc+ld+json;base64,eyJAY29udG...ifX0=

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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:

_:c14n0 <http://www.w3.org/1999/02/22-rdf-syntax-ns#type> <https://www.w3.org/2018/credentials#VerifiablePresentation> .
_:c14n0 <https://www.w3.org/2018/credentials#holder> <urn:ietf:params:oauth:jwk-thumbprint:sha-256:_Fpfe27AuGmEljZE9s2lw2UH-qrZLRFNrWbJrWIe4SI> .
_:c14n0 <https://www.w3.org/2018/credentials#verifiableCredential> _:c14n1 .
_:c14n0 <https://www.w3.org/2018/credentials#verifiableCredential> _:c14n3 .
_:c14n0 <https://www.w3.org/2018/credentials#verifiableCredential> _:c14n4 .
_:c14n0 <https://www.w3.org/2018/credentials#verifiableCredential> _:c14n5 .
_:c14n0 <https://www.w3.org/2018/credentials#verifiableCredential> _:c14n6 .
_:c14n0 <https://www.w3.org/2018/credentials#verifiableCredential> _:c14n7 .
_:c14n2 <http://www.w3.org/1999/02/22-rdf-syntax-ns#type> <https://www.w3.org/2018/credentials#VerifiableCredential> _:c14n1 .
_:c14n2 <https://www.w3.org/2018/credentials#credentialSubject> <https://subject.vendor.example> _:c14n1 .
_:c14n2 <https://www.w3.org/2018/credentials#issuer> <https://issuer.example/issuers/68> _:c14n1 .
_:c14n2 <https://www.w3.org/2018/credentials#validFrom> "2023-06-07T21:14:14.148Z"^^<http://www.w3.org/2001/XMLSchema#dateTime> _:c14n1 .

A DID Document isn't a VC, so, no, that wouldn't work, AFAICT. It's not a conforming document.

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 an id that is a DID a DID Document... especially if its returned with content type application/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.html

This 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.

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.

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 the above.

Same answer as 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.

Its clear we don't have agreement on the domain / range of verifiableCredential... I see it as having the same structure as issuer and holder, or proof... it can be an object, or an array of strings and objects.... similar to verification relationships.

The v1 context seems to support this assertion.

"issuer": {
"@id": "cred:issuer",
"@type": "@id"
},
"proof": {
"@id": "sec:proof",
"@type": "@id",
"@container": "@graph"
},
"verifiableCredential": {
"@id": "cred:verifiableCredential",
"@type": "@id",
"@container": "@graph"
}

This is the definition in the v2 context today:

"verifiableCredential": {
"@id": "https://www.w3.org/2018/credentials#verifiableCredential",
"@type": "@id",
"@container": "@graph"
},

For comparison, here are the context definitions for verification relationships:

"assertionMethod": {
"@id": "https://w3id.org/security#assertionMethod",
"@type": "@id",
"@container": "@set"
},

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 an object that contains that id), 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

TLDR, I am opposed to breaking changes to this experience in the v2 context.

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 via verifiableCredential, where each value is its own graph containing either a VerifiableCredential or other data conformant with the data model that is derived from a VerifiableCredential. 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 valid VerifiableCredential 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.

Copy link
Contributor

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

This AFAIK, never actually resulted in implementations.

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.

Yes, unless we remove the @container, or change it to @set, I expect this will continue to be the case.

A string value (ID / URL) for a subject does not establish any claims or additional relationships so the new graph is just empty.

When you say subject here, you mean the identifier for a verifiable credential, (assuming it has graph node id), right?

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 valid VerifiableCredential nor is it data derived from one, so it's not a valid use of the VCDM.

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".

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.

I like the suggestion, lets not let it stop this PR, I will file a separate issue for follow up: #1265

Copy link
Contributor

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.

Copy link
Contributor

@dlongley dlongley Sep 1, 2023

Choose a reason for hiding this comment

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

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.

This AFAIK, never actually resulted in implementations.

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.

A string value (ID / URL) for a subject does not establish any claims or additional relationships so the new graph is just empty.

When you say subject here, you mean the identifier for a verifiable credential, (assuming it has graph node id), right?

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 the subject 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, the verifiableCredential 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 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 valid VerifiableCredential nor is it data derived from one, so it's not a valid use of the VCDM.

This seems to be confusing "VerifiableCredential" with "VerifiableCredentialGraph" cc @iherman & @msporny , we discussed confusion related to this on #1240

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.

It would help to understand the normative / english language value of the "VerifiableCredentialGraph" since that is what is "empty".

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.

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.

I like the suggestion, lets not let it stop this PR, I will file a separate issue for follow up: #1265

+1

"@id": "https://www.iana.org/assignments/jwt#jwk",
"@type": "@json"
"@context": [
null,
Copy link
Contributor

@OR13 OR13 Aug 26, 2023

Choose a reason for hiding this comment

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

can you explain what this is "fixing" ? (in more detail than the PR description has).

Specifically what cases would this prevent that might be valuable to RDF processing community who would be impacted by this change?

Copy link
Member Author

@msporny msporny Aug 26, 2023

Choose a reason for hiding this comment

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

can you explain what this is "fixing" ? (in more detail than the PR description has).

You defined kid as https://www.iana.org/assignments/jose#kid" above... and then re-define it to https://www.iana.org/assignments/jwt#kid below -- don't know why you are switching between .../jose# and .../jwt#, but presuming you meant to do that, this is ensuring that your re-definition when used in cnf works as intended.

If you didn't mean to do that, we wouldn't need to null out the context at this level and we're going to have to rework all the JWT/JOSE terms to align them with either .../jose# or .../jwt# URLs.

Copy link
Contributor

Choose a reason for hiding this comment

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

So what this is fixing is:

Using the more specific registry to define the terms underneath cnf, ensuring that kid and jwk are defined according to the JWT registry, and not "generic jose".

This seems like an excellent improvement.

Copy link
Contributor

Choose a reason for hiding this comment

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

@OR13,

If anything else needs to be defined under cnf, you'll want to do so as well. Right now only kid and jwk will be defined.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dlongley that seems like breaking changes, cnf should get issuer dependent terms when a none "registered claim name" is used imo.

Copy link
Contributor

Choose a reason for hiding this comment

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

@OR13,

If you want to keep all of the other terms we can just remove null and just let these other specific overrides happen. If you want to clear all the other terms we should keep null and add back in whatever else you specifically want.

Copy link
Contributor

@OR13 OR13 Aug 31, 2023

Choose a reason for hiding this comment

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

I suggest we let the default context apply, unless there is agreement to replace it with a different value, for all terms under cnf... I could see there being some utility in using a different vocab here, but I am not sure its "worth it"... thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy to just drop the null and let the two overrides take place, leaving the other things there. Seems like the simplest solution to me at this time.

contexts/credentials/v2 Outdated Show resolved Hide resolved
Copy link
Contributor

@OR13 OR13 left a comment

Choose a reason for hiding this comment

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

I have question about the "null" context behavior, how well supported is that, will this break existing JSON-LD processors, etc... assuming we don't think this artificially limits the software processor space, these seem like great additions, and we should take them.

"@context": {
"@protected": true,
"kid": {
"@id": "https://www.iana.org/assignments/jwt#kid",
Copy link
Member Author

@msporny msporny Aug 26, 2023

Choose a reason for hiding this comment

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

@OR13 Just to be clear, all values you've declared as "@type": "@id" MUST be URIs... you cannot have non-URL values in there... JOSE defines many of these values as strings, IIRC... are you sure you want to do this? Have you thought through all of the use cases?

Copy link
Contributor

Choose a reason for hiding this comment

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

JSON-LD also says that some text fields are datetime strings... my experience is that eventually you process a string that is supposed to be a URL or a datetime, and it turns out to be an emoji.... Also, google uses @id values that are not URLs in their enterprise knowledge graph search API, so i'm not sure I would assume that those values are URLs.

Copy link
Contributor

Choose a reason for hiding this comment

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

@OR13,

What's being said here is that processors will raise errors if you use "@type": "@id" in the term's context definition if something other than a URL or graph node is used in the data. This is not the same case as using a datetime string that is malformed, etc., as those will not be processed by default. So, if you want to allow any kind of string in the field, you should remove "@type": "@id", otherwise default processing will try to enforce the use of URLs or graph nodes by raising errors when something else is present.

Copy link
Contributor

Choose a reason for hiding this comment

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

A JOSE kid is a string and need not be a URI. The typing should reflect that.

Copy link
Contributor

Choose a reason for hiding this comment

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

@selfissued not sure it matters since URIs are strings, and only an RDF processor will see these term definitions.

Copy link
Contributor

@OR13 OR13 Aug 30, 2023

Choose a reason for hiding this comment

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

If it's possible for the @type: @id and String to both be legal, that would be preferred... but in absence of that, we prefer the processing of these fields with DIDs or other URLs or URNs to build useful graphs.

We also see cases where @id is not a URL from google:

https://cloud.google.com/enterprise-knowledge-graph/docs/mid
https://github.com/GoogleCloudPlatform/python-docs-samples/blob/aaf4a56ce412d300134fae8359b874359c8f3330/enterpriseknowledgegraph/search/lookup_sample_test.py#L22

{
  "@id": "c-026015wur", // not a URL
  "name": "Amazon.com",
  "image": {
    "url": "https://en.m.wikipedia.org/wiki/File:Amazon.com-Logo.svg",
    "contentUrl": "https://encrypted-tbn0.gstatic.com/images?q=tbn:ANd9GcRPR..."
  },
  "url": "http://www.amazon.com/",
  "detailedDescription": {
    "url": "https://en.wikipedia.org/wiki/Amazon_(company)",
    "license": "https://en.wikipedia.org/wiki/Wikipedia:Text_of_Creative_Commons_Attribution-ShareAlike_3.0_Unported_License",
    "articleBody": "Amazon.com, Inc. is an American multinational technology company focusing on e-commerce, cloud computing, online advertising, digital streaming, and artificial intelligence. "
  },
  "@type": ["Organization", "Thing", "Corporation", "Place"],
  "description": "E-commerce company",
  "identifier": [
    { "value": "/m/0mgkg", "name": "googleKgMID", "@type": "PropertyValue" }
  ]
}

Copy link
Member Author

@msporny msporny Aug 30, 2023

Choose a reason for hiding this comment

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

@selfissued not sure it matters since URIs are strings

URIs have a structure, they're not just arbitrary strings (which, IIUC, @selfissued is saying kid can be any arbitrary string).

If you make kid and iss an type: @id, a JSON-LD processor will throw an error when processing kid and iss if either value isn't a URI... I don't think we want that behavior, it'll lead to interop issues.

That said, if you want to enforce that kid and iss be a URI when used in VCs, then using type: @id is one way to do that... but again, we'd be narrowing the scope of JWT in that case, which I didn't think was the intention of the WG.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@OR13,

That value only works when there's a base URL (like there always is with Web pages during search crawling and all that). With VCs, there is no base URL, it's presumed to be null, so it would result in a relative URL being used and an error as shown here: https://tinyurl.com/pys6xa4k

So I recommend we remove "@type": "@id" from all of the JOSE term context definitions that can take something other than a URL.

Copy link
Member Author

Choose a reason for hiding this comment

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

This concern is now tracked in #1275.

@msporny
Copy link
Member Author

msporny commented Aug 26, 2023

I have question about the "null" context behavior, how well supported is that, will this break existing JSON-LD processors, etc... assuming we don't think this artificially limits the software processor space, these seem like great additions, and we should take them.

"null" context behaviour is supported by all modern JSON-LD Processors, AFAIK. There are explicit tests for the behavior: https://w3c.github.io/json-ld-api/reports/ , namely Test tpr14: Clear protection with null context. (new in JSON-LD 1.1), and
Test tpr15: Clear protection with array with null context (new in JSON-LD 1.1).

It won't break existing JSON-LD processors (see above).

Copy link
Contributor

@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.

A JOSE kid is a string and need not be a URI. The typing should reflect that.

Copy link
Contributor

@OR13 OR13 left a comment

Choose a reason for hiding this comment

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

Based on other reviews:

  • remove changes that cause cnf to have undefined terms... they should get the default issuer defined terms, they should not error... if new json members are added under it.

@msporny
Copy link
Member Author

msporny commented Aug 30, 2023

  • remove changes that cause cnf to have undefined terms... they should get the default issuer defined terms, they should not error... if new json members are added under it.

hmm, right, yes, will fix.

"@id": "https://www.iana.org/assignments/jwt#jwk",
"@type": "@json"
"@context": [
null,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not easy to suggest this in github right now, but want it tracked, so don't apply directly:

Suggested change
null,

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in c44da90

@iherman
Copy link
Member

iherman commented Sep 3, 2023

The issue was discussed in a meeting on 2023-08-29

  • no resolutions were taken
View the transcript

1.6. Sync and fix bugs in v2 context. (pr vc-data-model#1259)

See github pull request vc-data-model#1259.

Manu Sporny: at the point where we need to lock down the JSON-LD context, Dave Longley and Manu have taken passes to fix issues in the context.
… one new issue - concern about JWT properties are defined. They are currently defined as having to be URLs, may not be the right thing for all properties. For example, kid is defined as being a URL value.

Michael Jones: kid has no semantics, it is a string.

Manu Sporny: asks for note from selfisused on 1259 to kid, needs general review of types.

@iherman
Copy link
Member

iherman commented Sep 5, 2023

The issue was discussed in a meeting on 2023-09-05

  • no resolutions were taken
View the transcript

3.3. Sync and fix bugs in v2 context. (pr vc-data-model#1259)

See github pull request vc-data-model#1259.

Manu Sporny: we might be OK to merge this, as it addresses Orie's issue.

Orie Steele: Move that discussion to another issue.

Orie Steele: lots of graph nodes are identified with strings that are not URLs, so Google's implementation works with both.
… we should not make changes to @context that throws errors in processors.

Michael Jones: in XML we have namespaces, but in JSON we don't.

Orie Steele: See this link for examples of @id that are strings. #1259 (comment).

Manu Sporny: path forward seems to be to merge what there in the PR and raise issues on anything that we dont like.

Orie Steele: Not true, and not relevant to the issue.

Manu Sporny: web pages have a base URL for strings to be relative to, whereas VCs do not have.

Manu Sporny: yes, agreed w/ Orie's "This is what we're waiting for".

Dave Longley: +1 to what Orie said, just need to remove that one null.

Orie Steele: we are waiting for a patch from @dlongley before we can merge.

Sebastian Crane: no objections to this PR. Namespaces are valuable, so URLs are very useful.

Michael Jones: namespaces are totally unnecessary.

Orie Steele: A registry is just a namespace @selfissued... so your comment makes very little sense to me, as a frequent user of registries.

Benjamin Young: +1.

Dave Longley: +1 to Orie, registries are namespaces too.

@msporny
Copy link
Member Author

msporny commented Sep 11, 2023

Normative, multiple reviews, changes requested and made, remaining issues tracked, no objections, merging.

@msporny msporny merged commit d5c7a02 into main Sep 11, 2023
1 check passed
@msporny msporny deleted the msporny-fix-base-context branch September 11, 2023 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants