feat: updates v3-unstable context to follow best practices #70

kdenhartog · 2020-11-02T02:30:51Z

This v3-unstable context update makes two major changes.

redefines all of the v2 and v1 context terms in a json-ld 1.1 compliant way rather than inporting through the context.
Makes the v3 context compliant with the VC v1 context so that there's not a protected term conflict.

kdenhartog · 2020-11-02T02:33:16Z

Ok @dlongley thanks for all the advice through the various issues necessary to put this together. We got this working with the Bbs Signature Suite in a way that doesn't conflict with the V1 context. This should address #51 as well.

EDIT: still need to update to fully define each term rather than using the compact IRIs when defining terms. Will update that.

tplooker · 2020-11-02T02:57:19Z

Whilst alphabetising the terms may be useful, it is making this review quite difficult, do you mind reverting and applying the changes to this context in a way that makes it easier to review?

msporny · 2020-11-03T14:26:26Z

Overall, +1 for this PR, thank you for putting it together @kdenhartog. Here are my high-level concerns, none of which should block this from being merged (but we may want to take a week or two to try to address them):

This is positive change, and a significant change in direction to the way we've been doing the Security vocabularies. Ideally, we'd have one or more test suites paired to all of these changes. Have you run this context through a test framework that you have? If not, we need to make sure that at least the other libraries that use the v2 context continue to work with a v3 context -- getting feedback from implementers will take time.
I noticed that only some of the types use scoped contexts. We need all types to use scoped contexts, which is going to bloat the context, but I believe that's inevitable if we are going to ensure that this context can be easily mixed with other vocabularies. This also means tests for all the types.
There is some cruft in here that we may want to remove. For example, I don't believe anyone is using Equihash these days... we may want to drop that. Same for RsaSignature2018 -- if folks want to use that, they can use the v2 signature suite. So, we should aggressively cull things that no one is using.
I noticed some use of { "@id": "https://foo.example#term" } that could be shortened to just "https://foo.example#term".
There are now merge conflicts due to other PRs being recently merged.

That's it upon a cursory examination of the PR... would appreciate responses to the above before we merge. Thank you again for all the hard work!

dlongley · 2020-11-03T20:36:48Z

contexts/security-v3-unstable.jsonld

-      "JsonWebKey2020": {
-        "@id": "https://w3id.org/security#JsonWebKey2020"
+      "@version": 1.1,
+      "id": "@id",


Should also have "@protected": true at the top level of this context.

Fixed as well now.

The usage of @protected ended up breaking many different tests in the BBS suite. I haven't been able to dig in and figure out what is the cause of this yet. The error I'm getting is that I'm redefining protected terms though.

suggest opening an issue, and working to address it with better tests separately... the context you are editing is already marked as unstable.

@kdenhartog -- it sounds like adding @protected may have successfully surfaced errors that weren't getting detected previously in the BBS suite. The cause of those errors should be determined, we shouldn't turn off protection just to avoid them.

dlongley · 2020-11-03T20:37:23Z

I don't have time to do a full review of the changes here right now, but they are heading in the right direction. Thanks!

OR13

@kdenhartog can you adddress manu's concerns or open issues to track addressing them, and fix the merge conflict.

I am in favor of merging this asap, and taking smaller PRs which are easier to review for specific issues after.

kdenhartog · 2020-11-05T01:39:09Z

@kdenhartog can you adddress manu's concerns or open issues to track addressing them, and fix the merge conflict.

I am in favor of merging this asap, and taking smaller PRs which are easier to review for specific issues after.

Been working on them today to address them all. Just keep getting sidetracked with other conversations as well. Looking to get this merged ASAP as well.

kdenhartog · 2020-11-05T05:36:39Z

Overall, +1 for this PR, thank you for putting it together @kdenhartog. Here are my high-level concerns, none of which should block this from being merged (but we may want to take a week or two to try to address them):
1. This is positive change, and a significant change in direction to the way we've been doing the Security vocabularies. Ideally, we'd have one or more test suites paired to all of these changes. Have you run this context through a test framework that you have? If not, we need to make sure that at least the other libraries that use the v2 context continue to work with a v3 context -- getting feedback from implementers will take time.

So far, we've validated this using BbsBlsSignatures2020 suite. As far as I can tell this remains backwards compatible and doesn't redefine terms in a breaking way (I've effectively just copied v1 and v2). In the case of BbsBlsSignature2020 suite the compact operation being performed by jsonld-signatures is doing so in a way that the type ends up being sec:BbsBlsSignature2020 because of how jsonld-signatures is tied to the v2 context at the moment. Specifically in the suite we validate VCs used in some of the newly defined v3 terms, but does not attempt to validate every single property works as expected. I agree doing so would be beneficial as we go to a ld-proof WG, but don't quite have the bandwidth to take on that much work at this point.

2. I noticed that only some of the types use scoped contexts. We need all types to use scoped contexts, which is going to bloat the context, but I believe that's inevitable if we are going to ensure that this context can be easily mixed with other vocabularies. This also means tests for all the types.

Yup agreed tests for all the types would be good. Just don't have the bandwidth to handle that at this point.

3. There is some cruft in here that we may want to remove. For example, I don't believe anyone is using Equihash these days... we may want to drop that. Same for RsaSignature2018 -- if folks want to use that, they can use the v2 signature suite. So, we should aggressively cull things that no one is using.

Happy to do this before stabilizing, but want to make sure we agree to which terms are being dropped rather than me picking and choosing. I'm not aware of how all of the v1/v2 terms are being used all the time. Can we do this in a separate issue and I'll go through and clean them up after?

4. I noticed some use of `{ "@id": "https://foo.example#term" }` that could be shortened to just `"https://foo.example#term"`.

Should all be cleaned up now. Thanks.

5. There are now merge conflicts due to other PRs being recently merged.

Should be fixed now as well. Thanks.

That's it upon a cursory examination of the PR... would appreciate responses to the above before we merge. Thank you again for all the hard work!

All fixed up now as well.

OR13

This PR is getting large and has already spawned a number of smaller issues, I would like to merge it and address the issues raised seperatly.

peacekeeper · 2020-11-06T16:14:06Z

Have you run this context through a test framework that you have?

We have some unit tests that use this context in our Java library jsonld-common-java.

This has a locally cached version of security-v3-unstable.jsonld here.

I just tried to simply replace the current version with the version proposed in this PR, and the unit test did NOT pass.

The error that is thrown is "An attempt was made to redefine a protected term".

This may very well be a bug in our library or the underlying titanium-json-ld rather than the context file. I'd be happy to dig deeper to find out, just wanted to quickly report this.

OR13 · 2020-11-06T22:07:33Z

@peacekeeper thanks for testing this!

I am still in favor to taking these changes, but perhaps we can add some CI checks to this repo next?

wonder if we could get some cross language JSON-LD tests going so we an know how changes like this impact , go, python, js and java via CI.

kdenhartog · 2020-11-08T15:58:06Z

I've not had time to figure out the problem with this yet, but have narrowed the problem down to using the @protected term at the top level of the context. My plan is to attempt to define each term individually and see which term is causing the problem and go from there. I'd vote to hold off on merging until I can clean up that one issue first since it's definitely not working on multiple implementations as @peacekeeper shared.

OR13 · 2020-11-08T17:14:37Z

we should add something like what I'm working on here : w3c-ccg/traceability-vocab#28

to this repo eventually.

kdenhartog · 2020-11-10T01:17:09Z

Figured out the issue and playing around with a few ways to fix it at the moment.

What I've discovered is that the new terms (except the one's defined in the VC context) are encountering issues where the terms are being redefined within the V3 context itself.

For example, when using the "@protected": true at the top of the context document it's effectively adding the term to each property. So for nonce it's going from "nonce": "https://w3id.org/security#nonce"

to now being defined like so:

"nonce": { 
  "@id": "https://w3id.org/security#nonce", 
  "@protected": true  
},

So the problem is that the way that we originally defined terms like BbsBlsSignatureProof2020 means that we're defining terms in the property contexts and then redefining them at the top level context in a way that conflicts.

This v3-unstable context update makes two major changes. - redefines all of the v2 and v1 context terms in a json-ld 1.1 compliant way rather than inporting through the context. - Makes the v3 context compliant with the VC v1 context so that there's not a protected term conflict.

kdenhartog · 2020-11-10T01:38:11Z

I've removed the "@protected": true term at the document so this should now be working @peacekeeper

Additionally, I've opened issue #75 so we can decide the best way to update the context before stablizing. This way we can merge this PR and keep the fixes easier to review.

kdenhartog · 2020-11-10T01:40:05Z

@msporny or @dlongley thoughts on merging with the additional created issues / response provided above?

peacekeeper · 2020-11-10T08:25:00Z

contexts/security-v3-unstable.jsonld

+    "cipherData": "https://w3id.org/security#cipherData",
+    "cipherKey": "https://w3id.org/security#cipherKey",
+    "created": {"@id": "http://purl.org/dc/terms/created", "@type": "http://www.w3.org/2001/XMLSchema#dateTime"},
+    "creator": "http://purl.org/dc/terms/createdcreator",


Suggested change

"creator": "http://purl.org/dc/terms/createdcreator",

"creator": {"@id": "http://purl.org/dc/terms/creator", "@type": "@id"},

Thanks will fix this.

peacekeeper · 2020-11-10T08:29:44Z

I've removed the "@protected": true term at the document so this should now be working @peacekeeper

Thanks @kdenhartog, I can confirm that this works for me now, and I agree with your analysis (I came to the same conclusion). I wonder, was this working for others, i.e. having @protected at the top level in a82be1a?

peacekeeper · 2020-11-10T08:33:34Z

One additional change I am noticing in my unit tests is that now "sec" and "xsd" namespaces are no longer defined and used at the top level, is that intentional? Anyway, I'd also be fine with merging now and addressing this later.

dlongley · 2020-11-10T16:31:28Z

We should have @protected at the top level -- and if that causes errors we should figure out what they are and why. A security context should not allow terms to be redefined without conforming to the protection rules.

kdenhartog · 2020-11-10T22:50:49Z

I've removed the "@protected": true term at the document so this should now be working @peacekeeper

Thanks @kdenhartog, I can confirm that this works for me now, and I agree with your analysis (I came to the same conclusion). I wonder, was this working for others, i.e. having @protected at the top level in a82be1a?

They were working fine because the v1/v2 context weren't protecting the terms there, so the scoped context terms that were redefining them overrode the terms discovered in v1/v2 security vocab. Now that all terms are being brought into a single context without reference it's causing issues once the @protected term is added at the top level. This is because the scoped context terms aren't actually scoped from what I can tell.

One additional change I am noticing in my unit tests is that now "sec" and "xsd" namespaces are no longer defined and used at the top level, is that intentional? Anyway, I'd also be fine with merging now and addressing this later.

Yeah see here for the discussion about those considerations: #73

We should have @Protected at the top level -- and if that causes errors we should figure out what they are and why. A security context should not allow terms to be redefined without conforming to the protection rules.

I've diagnosed them in the comments above. I agree that the terms should not be allowed to be redefined and am working on a PR to reintroduce the usage of @protected correctly in a way that won't cause these issues. However, I'm looking to get this PR through first and then provide focused updates through subsequent PRs given the size of the PR already. Since it's labeled as unstable, it shouldn't be in use in production systems yet anyways so incremental changes should be fine in my opinion.

peacekeeper · 2020-11-11T13:58:00Z

I'm looking to get this PR through first and then provide focused updates through subsequent PRs given the size of the PR already.

Fine with me, this is definitely good work! But let's try to not leave it in a broken state for long, even if it's marked as unstable..

dlongley · 2020-11-11T15:39:42Z

@kdenhartog,

I agree that the terms should not be allowed to be redefined and am working on a PR to reintroduce the usage of @Protected correctly in a way that won't cause these issues. However, I'm looking to get this PR through first and then provide focused updates through subsequent PRs given the size of the PR already. Since it's labeled as unstable, it shouldn't be in use in production systems yet anyways so incremental changes should be fine in my opinion.

Ok, sounds good.

kdenhartog · 2020-11-12T00:05:15Z

Fine with me, this is definitely good work! But let's try to not leave it in a broken state for long, even if it's marked as unstable..

Yup, agreed. I've got a few proposals on how to handle this and we can consider the options and discuss in issue #75

kdenhartog · 2020-11-12T00:11:03Z

Merging after multiple Oks. Further work on this will continue as follow up PRs to add the @protected back into this context.

kdenhartog requested review from msporny, OR13 and tplooker as code owners November 2, 2020 02:30

kdenhartog requested a review from dlongley November 2, 2020 02:31

dlongley reviewed Nov 3, 2020

View reviewed changes

OR13 approved these changes Nov 4, 2020

View reviewed changes

OR13 self-requested a review November 4, 2020 14:31

OR13 requested changes Nov 4, 2020

View reviewed changes

kdenhartog force-pushed the kdh/update-v3-context branch from a39e8c0 to 90446b8 Compare November 5, 2020 05:36

kdenhartog force-pushed the kdh/update-v3-context branch from 90446b8 to a82be1a Compare November 5, 2020 05:43

This was referenced Nov 5, 2020

Write test suite for v3 context #71

Open

Which properties should we drop from V3 context that are unused now? #72

Open

OR13 self-requested a review November 6, 2020 14:51

OR13 approved these changes Nov 6, 2020

View reviewed changes

kdenhartog force-pushed the kdh/update-v3-context branch from a82be1a to ea39e99 Compare November 10, 2020 01:34

kdenhartog mentioned this pull request Nov 10, 2020

Establish best practices for using protected term #75

Closed

peacekeeper reviewed Nov 10, 2020

View reviewed changes

kdenhartog merged commit d03ed38 into master Nov 12, 2020

kdenhartog mentioned this pull request Nov 12, 2020

Provide reusable context compatible with VC data model context #51

Closed

kdenhartog mentioned this pull request Dec 20, 2021

fix broken links in context.jsonld w3c/did-extensions#392

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: updates v3-unstable context to follow best practices #70

feat: updates v3-unstable context to follow best practices #70

kdenhartog commented Nov 2, 2020

kdenhartog commented Nov 2, 2020 •

edited

Loading

tplooker commented Nov 2, 2020

msporny commented Nov 3, 2020 •

edited

Loading

dlongley Nov 3, 2020

kdenhartog Nov 5, 2020

kdenhartog Nov 6, 2020

OR13 Nov 6, 2020 •

edited

Loading

dlongley Nov 10, 2020

dlongley commented Nov 3, 2020

OR13 left a comment

kdenhartog commented Nov 5, 2020

kdenhartog commented Nov 5, 2020

OR13 left a comment

peacekeeper commented Nov 6, 2020 •

edited

Loading

OR13 commented Nov 6, 2020

kdenhartog commented Nov 8, 2020 •

edited

Loading

OR13 commented Nov 8, 2020

kdenhartog commented Nov 10, 2020 •

edited

Loading

kdenhartog commented Nov 10, 2020

kdenhartog commented Nov 10, 2020

peacekeeper Nov 10, 2020

kdenhartog Nov 10, 2020

peacekeeper commented Nov 10, 2020

peacekeeper commented Nov 10, 2020

dlongley commented Nov 10, 2020

kdenhartog commented Nov 10, 2020

peacekeeper commented Nov 11, 2020

dlongley commented Nov 11, 2020

kdenhartog commented Nov 12, 2020

kdenhartog commented Nov 12, 2020

	"creator": "http://purl.org/dc/terms/createdcreator",
	"creator": {"@id": "http://purl.org/dc/terms/creator", "@type": "@id"},

feat: updates v3-unstable context to follow best practices #70

feat: updates v3-unstable context to follow best practices #70

Conversation

kdenhartog commented Nov 2, 2020

kdenhartog commented Nov 2, 2020 • edited Loading

tplooker commented Nov 2, 2020

msporny commented Nov 3, 2020 • edited Loading

dlongley Nov 3, 2020

Choose a reason for hiding this comment

kdenhartog Nov 5, 2020

Choose a reason for hiding this comment

kdenhartog Nov 6, 2020

Choose a reason for hiding this comment

OR13 Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

dlongley Nov 10, 2020

Choose a reason for hiding this comment

dlongley commented Nov 3, 2020

OR13 left a comment

Choose a reason for hiding this comment

kdenhartog commented Nov 5, 2020

kdenhartog commented Nov 5, 2020

OR13 left a comment

Choose a reason for hiding this comment

peacekeeper commented Nov 6, 2020 • edited Loading

OR13 commented Nov 6, 2020

kdenhartog commented Nov 8, 2020 • edited Loading

OR13 commented Nov 8, 2020

kdenhartog commented Nov 10, 2020 • edited Loading

kdenhartog commented Nov 10, 2020

kdenhartog commented Nov 10, 2020

peacekeeper Nov 10, 2020

Choose a reason for hiding this comment

kdenhartog Nov 10, 2020

Choose a reason for hiding this comment

peacekeeper commented Nov 10, 2020

peacekeeper commented Nov 10, 2020

dlongley commented Nov 10, 2020

kdenhartog commented Nov 10, 2020

peacekeeper commented Nov 11, 2020

dlongley commented Nov 11, 2020

kdenhartog commented Nov 12, 2020

kdenhartog commented Nov 12, 2020

kdenhartog commented Nov 2, 2020 •

edited

Loading

msporny commented Nov 3, 2020 •

edited

Loading

OR13 Nov 6, 2020 •

edited

Loading

peacekeeper commented Nov 6, 2020 •

edited

Loading

kdenhartog commented Nov 8, 2020 •

edited

Loading

kdenhartog commented Nov 10, 2020 •

edited

Loading