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

Add relatedResource property to VerifiablePresentation #1370

Closed
wants to merge 5 commits into from

Conversation

msporny
Copy link
Member

@msporny msporny commented Dec 3, 2023

This PR attempts to address issue #1360 by allowing the relatedResource property to be used on a VerifiablePresentation. It also adds an "at risk" issue marker noting that the feature might be removed in the Candidate Recommendation phase based on implementer feedback.


Preview | Diff

index.html Outdated
</p>
<p class="issue" title="Mandatory listing of contexts in relatedResouce are under debate.">
The requirement that contexts be listed in `relatedResource` is currently being
debated in the VCWG. This requirement might be removed in future iterations of
the specification.
</p>
<p class="issue at-risk" title="Use of `relatedResource` in `VerifiablePresentation`">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<p class="issue at-risk" title="Use of `relatedResource` in `VerifiablePresentation`">
<p class="issue at-risk" title="Use of `relatedResource` in `VerifiablePresentation` is at risk">

I think it is better to make the "at risk" features very explicit when reading the document. (I have not checked whether this is the case for the other, at risk features in the spec.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I got the "atrisk" class wrong and fixed it in de83559.

Which results in this rendering, does that work for you @iherman ?

image

Copy link
Member

Choose a reason for hiding this comment

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

Looks perfect! 😀

@brentzundel brentzundel changed the title Add relatedResource property to VerifiableCredential Add relatedResource property to VerifiablePresentation Dec 4, 2023
@@ -163,7 +163,7 @@ property:

- id: relatedResource
label: Related resource
domain: cred:VerifiableCredential
domain: [cred:VerifiableCredential, cred:VerifiablePresentation]
Copy link
Contributor

@OR13 OR13 Dec 4, 2023

Choose a reason for hiding this comment

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

sry, I meant for this comment to go on "range".

Given this points to credentials by URL, which can't you use a Data URI here?

{
  "@context": [
    "https://www.w3.org/ns/credentials/v2",
    "https://www.w3.org/ns/credentials/examples/v2"
  ],
  "id": "urn:uuid:5ec137ea-871e-11ee-a783-ef96a4397a9a",
  "type": ["VerifiablePresentation", "ExamplePresentation"],
  "relatedResource": [
    "https://vendor.example/resources/42",
    "data:application/jwt;base64,QzVjV...RMjUK==",
    "data:application/cwt;base64,ZmlOW...pYzMK="
 ]
}

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
Member

Choose a reason for hiding this comment

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

Because an enveloped credential is far more that just a "related resource".

Copy link
Contributor

Choose a reason for hiding this comment

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

it would be concerning to me to use this property to represent jwts/cwts that are transformable to w3c vcs

Copy link
Member Author

@msporny msporny Dec 11, 2023

Choose a reason for hiding this comment

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

@decentralgabe, would you find this mechanism more acceptable? #1379 (preview here)

<code>relatedResource</code> is present, there MUST be an object in the array
for each remote resource for each context used in the verifiable credential.
for each remote resource used in the verifiable credential.
Copy link
Contributor

Choose a reason for hiding this comment

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

data URIs are IRIs.

@jandrieu
Copy link
Contributor

jandrieu commented Dec 5, 2023

This is still a layering error

Copy link
Contributor

@jandrieu jandrieu left a comment

Choose a reason for hiding this comment

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

Remove this. It isn't a good adjustment.

@iherman
Copy link
Member

iherman commented Dec 6, 2023

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

  • no resolutions were taken
View the transcript

1.1. Add relatedResource property to VerifiablePresentation (pr vc-data-model#1370)

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

Brent Zundel: only has approvals.
… likely to get merged within the week.

Joe Andrieu: I still think this is a layering error. I don't know why Verifier would know how to interpret related resource, they didn't ask for it, it's not a VC, I think most use cases could be handled w/ a VC or how to secure contexts for ... generic mechanism seems wrong.

Manu Sporny: I largely agree with Joe on his use cases. We should probably have a context securing mechanism.
… and have something for enveloped VCs.
… a fear is that some implementers are going to use related resources as a dumping ground for all sorts of things.
… I want to express XYZ, I'm going to put that in there.
… All that to say, Joe has a point.
… But if folks want to use the data model in that way, they can.

Joe Andrieu: I want to note that anyone can add to the context and they can add properties. What we're talking about here is that everyone has to respect... we aren't preventing people that want to use relatedResource. I don't see reason for extensibility in this way.

Brent Zundel: I see the benefit of having more specific mechanisms for the different things.
… would the related resource be seen as a stepping stone?

Joe Andrieu: I think a lot of it is lessons learned from extensible HTTP headers, which was "X-something"... if people start using it, they won't stop using it. If burden is you have your own context that does something specific. That's the path towards getting new things into VCDM. I'm afraid of setting the wrong direction here.

Dave Longley: +1 to Joe that once people start using things they won't stop ... there are advantages for putting slight speed bumps in the way to make people do things in better ways.

Brent Zundel: this PR has nothing but approvals.

Copy link
Contributor

@jandrieu jandrieu left a comment

Choose a reason for hiding this comment

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

This should not go in. It's a layer violation.

@msporny
Copy link
Member Author

msporny commented Dec 7, 2023

@jandrieu wrote:

This should not go in. It's a layer violation.

You noted on the call that you were a non-blocking objection for this PR.

However, your comment above could be seen as objecting to this PR.

Are you ok with it being merged, or are you objecting to it being merged?

@jandrieu
Copy link
Contributor

jandrieu commented Dec 9, 2023

That was a miscommunication.

I do object to this going in.

@msporny
Copy link
Member Author

msporny commented Dec 9, 2023

@jandrieu wrote:

I do object to this going in.

Ok, marking this as pending close, then.

@msporny msporny added the pending close Close if no objection within 7 days label Dec 9, 2023
@iherman
Copy link
Member

iherman commented Dec 12, 2023

The issue was discussed in a meeting on 2023-12-12

  • no resolutions were taken
View the transcript

2.1. Add relatedResource property to VerifiablePresentation (pr vc-data-model#1370)

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

Brent Zundel: First up, 1370.
… Looks like Joe is opposed.
… We don't have consensus on this. Looking to close.
… happy to take comments. if folks are opposed, we can timebox a conversation.

Manu Sporny: As a heads up, I curated the PRs and put pending close on any thing that has an objection without a work-around.
… we need to get more aggressive about closing.
… Just an explanation about why things are marked pending closed.
… Another thing to ask is whether or not the objector would make an FO on this change.
… My concern about this PR is that if we don't put Related Resource will not have a way of adding a hash for a JSON LD context in addition to the base context.
… It has been asserted that you don't have to do that if you have static contexts.
… Orie's not here, who might be objecting.

Joe Andrieu: Yes, I am opposed to this, I would Formally Object, I think this is a huge layer violation. It creates an opportunity for surveillance through related resource, I think we can solve context securing in a different way.
… Manu, noted that VC-JOSE-COSE folks want to use JSON-LD Contexts... they should just use JSON-LD.

Ivan Herman: I'm neutral on the original thing. but what was just proposed to Joe seems the wrong thing to do.
… saying that it can only be used for certain purposes sounds bad.
… that requires additional restrictions through new properties.
… I can't object as a W3C staff, but I don't like it.

Manu Sporny: FWIW, I agree with Ivan, I was just trying to find a way through this where we don't end up w/ FOs.

Brent Zundel: I'm seeing no consensus here.
… We don't have Orie, so we can't ask him.
… based on the information we have, we should close this PR.
… I'd like to close it now, but we just put the pending close label on this three days ago.

Manu Sporny: note, if Orie is going to formally object (because you can't secure context).
… our way through this would be to have a "relatedContext" property.
… I think Joe would not be opposed to that. And maybe Orie would find that ok.

Ivan Herman: I want to remind people that there was a feature freeze a while ago.
… what you just proposed feels like a new feature. It's just too late.
… We can't work that way or we'll never finish.

Brent Zundel: appreciate the reminders.

@msporny
Copy link
Member Author

msporny commented Dec 15, 2023

PR failed to achieve consensus, marked "pending close" for over a week, closing.

@msporny msporny closed this Dec 15, 2023
@msporny msporny deleted the msporny-vp-relatedResource branch December 17, 2023 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
before-CR pending close Close if no objection within 7 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants