Skip to content

Presentations #1181

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

Merged
merged 11 commits into from
Jul 8, 2023
Merged

Presentations #1181

merged 11 commits into from
Jul 8, 2023

Conversation

brentzundel
Copy link
Member

@brentzundel brentzundel commented Jun 30, 2023

The goal of this PR is to make more explicit what a VP is.

This PR is based on feedback received on PR #1167 as well as the discussion in issue #996.
This PR fixes #996.

It changes the non-normative description (Section 3.3) of verifiable presentations to remove ambiguity.
It also adds and strengthens normative statements in section 4.10.


Preview | Diff

Copy link
Contributor

@awoie awoie left a comment

Choose a reason for hiding this comment

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

LGTM after suggestions applied. "optional" is not a normative verb according to 1.4 which is why I rewrote those sections.

@msporny
Copy link
Member

msporny commented Jul 3, 2023

LGTM after suggestions applied. "optional" is not a normative verb according to 1.4 which is why I rewrote those sections.

OPTIONAL is a normative word, but we were using the lowercase version of it (on purpose). We could also fix things by just using "OPTIONAL", as that's equivalent to "MAY".

I'll note that having "SHOULD" and "MAY" statements are often not tested because people complain when their implementations fail normative statements.

In any case, I agree with the normative changes @awoie is suggesting to the contents of the VerifiablePresentation object.

@@ -790,12 +790,7 @@ <h3>Presentations</h3>

<p>
A <a>verifiable presentation</a> expresses data from one or more
<a>verifiable credentials</a>, and is packaged in such a way that the
authorship of the data is <a>verifiable</a>. If <a>verifiable credentials</a>
are presented directly, they become <a>verifiable presentations</a>. Data
Copy link
Contributor

Choose a reason for hiding this comment

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

I like some of this text, don't think it should all be removed.

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'm open to suggestions. My attempt to adjust this section did not find consensus.

Copy link
Contributor

Choose a reason for hiding this comment

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

can we keep

<a>verifiable credentials</a>, and is packaged in such a way that the
authorship of the data is <a>verifiable</a>. If <a>verifiable credentials</a>
are presented directly, they become <a>verifiable presentations</a>.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now that we've introduced media types, I don't think it is true that a verifiable credential (vc+ld+json) becomes a verifiable presentation (vp+ld+json).

What about a sentence like: "It is also possible to present verifiable credentials directly."

Copy link
Contributor

Choose a reason for hiding this comment

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

sure I think that's a good sentence

Copy link
Member Author

@brentzundel brentzundel Jul 5, 2023

Choose a reason for hiding this comment

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

Re-worded in 29f5cc9

@TallTed
Copy link
Member

TallTed commented Jul 5, 2023

I'll note that having "SHOULD" and "MAY" statements are often not tested because people complain when their implementations fail normative statements.

Then we SHOULD be clear about what SHOULD be tested, and how the results SHOULD be interpreted. Failure of a MAY is only problematic if the implementation claims that it implemented that feature. Failure of a SHOULD is only problematic if the implementation claims that it implemented that feature according to the SHOULD. Both of these failures are helpful to evaluators who SHOULD be able to discern implementations which DID implement those features from implementations which did NOT.

People who complain at that point will need to make a very strong case for why we ought to pay attention to their complaints.

@awoie
Copy link
Contributor

awoie commented Jul 5, 2023

OPTIONAL is a normative word, but we were using the lowercase version of it (on purpose). We could also fix things by just using "OPTIONAL", as that's equivalent to "MAY".

@msporny According to the Conformance section, we only have the following:

The key words MAY, MUST, MUST NOT, RECOMMENDED, SHOULD, and SHOULD NOT in this document are to be interpreted as described in BCP 14 [RFC2119] [RFC8174] when, and only when, they appear in all capitals, as shown here.

I know that OPTIONAL is used in IETF RFCs but I couldn't find it in the paragraph above. Should we add OPTIONAL there too? Or what is the reason it was not added?

@msporny
Copy link
Member

msporny commented Jul 5, 2023

I know that OPTIONAL is used in IETF RFCs but I couldn't find it in the paragraph above. Should we add OPTIONAL there too? Or what is the reason it was not added?

That paragraph is auto-generated by ReSpec based on the normative RFC language used throughout the document. So, if you don't use a certain RFC term, it's not added to that paragraph. If we use "OPTIONAL" (in all caps) in the document, ReSpec will modify that paragraph I pointed to and will add the word automatically to that paragraph.

So, no reason to modify that particular paragraph, it'll auto-update if we use the OPTIONAL language in the places where we use lowercase "optional" today.

@brentzundel
Copy link
Member Author

@awoie I believe I have addressed your concerns, could you re-review?

index.html Outdated
Comment on lines 792 to 793
A <a>verifiable presentation</a> expresses data from one or more
<a>verifiable credentials</a>, and is packaged in such a way that the
authorship of the data is <a>verifiable</a>. If <a>verifiable credentials</a>
are presented directly, they become <a>verifiable presentations</a>. Data
formats derived from <a>verifiable credentials</a> that are cryptographically
<a>verifiable</a>, but do not of themselves contain
<a>verifiable credentials</a>, might also be <a>verifiable presentations</a>.
<a>verifiable credentials</a>.
Copy link
Contributor

Choose a reason for hiding this comment

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

0 or more credentials AND arbitrary additional data encoded in JSON-LD

Copy link
Member Author

Choose a reason for hiding this comment

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

Re-worded in 29f5cc9

index.html Outdated
type of <a>presentation</a>, such as <code>VerifiablePresentation</code>. For
The <code>type</code> <a>property</a> MUST be present. It is used to express the
type of <a>verifiable presentation</a>. The value of this property MUST be
<code>VerifiablePresentation</code>, but additional types MAY be included. For
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to comment on if its an array, what values are legal, etc...

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe (as above) the links to other sections where this is already described normatively aren't sufficient? Should I just be explicit and say if present, it MUST follow the guidance in the types section?

Copy link
Member Author

@brentzundel brentzundel Jul 5, 2023

Choose a reason for hiding this comment

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

I've re-worded the language in 75bfa7d to normatively point to the guidance you suggest.

details related to the use of this property, see Section <a href="#types"></a>.
</dd>
<dt><var>verifiableCredential</var></dt>
<dd>
If present, the value of the <code>verifiableCredential</code> <a>property</a>
MUST be constructed from one or more <a>verifiable credentials</a>, or of data
The <code>verifiableCredential</code> <a>property</a> MAY be present. The value
Copy link
Contributor

Choose a reason for hiding this comment

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

if its present, can it be empty?

Copy link
Member Author

Choose a reason for hiding this comment

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

What is better:

  1. required and can be empty, or
  2. not required but has to have at least one value?

Copy link
Contributor

@OR13 OR13 Jul 7, 2023

Choose a reason for hiding this comment

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

2 is better... this is a perfect case of "many different JSONs" produce the same "graph"... and proof mechanisms might not preserve all of them the same way...

We have the exact same problem come up with credentialSubject.

we said it MUST be present and it MUST NOT be empty.

Doing that for Verifiable Presentations would destroy "DID Auth" which use data integrity proofs to prove possession of a private key for a holder, but does not communicate any credentials.

So my reasoning for 2 is to allow for that behavior to continue to be legal, while removing equivalent JSON syntax for the same RDF Graph.

Copy link
Member Author

Choose a reason for hiding this comment

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

excellent. 2 is how it's currently worded, so I won't make any changes.

index.html Outdated
is expected to be a <a>URL</a> for the entity that is generating the
<a>presentation</a>.
The <a>verifiable presentation</a> MAY include a <code>holder</code> <a>property</a>. If present, the value
MUST be a <a>URL</a> for the entity that is generating the <a>verifiable presentation</a>.
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably need this URL reference everywhere we see id... and we probably want to allow for this as well:

holder: { id: ...., name: alice }

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'll modify the PR to mimic the language in the issuer property. I think that will address this.

Copy link
Member Author

Choose a reason for hiding this comment

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

modified the language in e5b8dd0

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.

Only blocking feedback is this:

https://github.com/w3c/vc-data-model/pull/1181/files#r1253523816

It might be better to just more directly state the normative requirements of vp+ld+json.

@context and type what is required.

@brentzundel
Copy link
Member Author

@OR13 and @awoie I have made changes in reference to your suggestions. Please re-review.

brentzundel and others added 3 commits July 5, 2023 16:54
Signed-off-by: Brent Zundel <[email protected]>
Signed-off-by: Brent Zundel <[email protected]>
Co-authored-by: Oliver Terbu <[email protected]>
Co-authored-by: Manu Sporny <[email protected]>
@iherman
Copy link
Member

iherman commented Jul 6, 2023

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

  • no resolutions were taken
View the transcript

2.6. Determine interoperable way for Holder to make claims directly in VPs (issue vc-data-model#860)

See github issue vc-data-model#860.

Kristina Yasuda: This is about self-signed VCs, right?

Orie Steele: Brent has a raised a PR that addresses a lot of this issue.
… Pull 1181.
… PR 1181 doesn't currently reflect what you can do with a VP.
… Brent, can you please revise per the comments.

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

Orie Steele: it could address that issue.

Brent Zundel: I don't remember if I tried to address the comments or not yet, but I definitely can.

Kristina Yasuda: I've assigned this to you, Brent.

Joe Andrieu: present?

@awoie awoie self-requested a review July 7, 2023 11:54
Copy link
Contributor

@awoie awoie left a comment

Choose a reason for hiding this comment

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

lgtm

Thanks Ted!

Co-authored-by: Ted Thibodeau Jr <[email protected]>
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.

Approved, assuming an issue marker or some attempt to address https://github.com/w3c/vc-data-model/pull/1181/files#r1256018885

@brentzundel
Copy link
Member Author

Approved, assuming an issue marker or some attempt to address https://github.com/w3c/vc-data-model/pull/1181/files#r1256018885

Can you say more what this issue would say? I think the current PR text supports what you're looking for here.

Comment on lines +1920 to +1921
unique identifier for the <a>verifiable presentation</a>. If present, the
normative guidance in Section <a href="#identifiers"></a> MUST be followed.
Copy link
Member

Choose a reason for hiding this comment

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

Just noting that we don't need the normative statement on identifiers, as all identifiers must follow that guidance anyway. This normative statement doesn't make the implementation do anything above and beyond what's stated in the #identifiers section. I think we can safely remove this statement (in a future PR).

Comment on lines +1927 to +1928
<code>VerifiablePresentation</code>, but additional types MAY be included. The
related normative guidance in Section <a href="#types"></a> MUST be followed.
Copy link
Member

Choose a reason for hiding this comment

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

Just noting that we don't need the normative statement on types, as all types must follow that guidance anyway. This normative statement doesn't make the implementation do anything above and beyond what's stated in the #types section. I think we can safely remove this statement (in a future PR).

@msporny
Copy link
Member

msporny commented Jul 8, 2023

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

@msporny msporny merged commit d0567d2 into w3c:main Jul 8, 2023
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.

Question about verifiable presentations
7 participants