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.

Sign up for GitHub

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

Jump to bottom

Improve examples #104

Merged
merged 10 commits into from
Jun 15, 2023
Merged

Improve examples #104

merged 10 commits into from
Jun 15, 2023

Conversation

OR13
Copy link
Contributor

@OR13 OR13 commented Jun 7, 2023 •
edited by pr-preview bot
Loading

@OR13
Copy link
Contributor Author

OR13 commented Jun 7, 2023

Same issue here, https://github.com/w3c/vc-data-model/actions/runs/5169338498/jobs/9311525031

@iherman
Copy link
Member

iherman commented Jun 8, 2023

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

  • no resolutions were taken
View the transcript

1.2. JWT PRs.

Orie Steele: There are 2 new PRs on VC-JWT, 103 and 104.

See github pull request vc-jwt#103.

See github pull request vc-jwt#104.

Orie Steele: There are 2 new PRs on VC-JWT, 103 and 104.

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

Orie Steele: PRs up in the core data model 1144.

Phillip Long: pdl-ASU has joined #vcwg.

Joe Andrieu: Added a comment to 103. Would it be useful to reuse the pattern for VCs. A base media type plus securing?
… both securing mechanisms will need a payload with a common basis.

Dmitri Zagidulin: +1 Joe, I was wondering about that as well.

Orie Steele: correct, that is what it is doing.

Dave Longley: agrees it is following the same pattern.

Orie Steele: talking about confidence method in registry.

Dave Longley: +1 to adding confidence method to the reserved terms table + v2 context, sounds good.

Brent Zundel: 9 open PRs, please review and look thru them.
… moving on to final topic, issues.

Ted Thibodeau Jr.: suggest that we call out specific issues and PRs in agenda ahead of time.

Orie Steele: There are also several open PRs here: https://github.com/w3c/vc-status-list-2021/pulls which can be discussed.

This was referenced Jun 8, 2023
Merged
Merged
@mprorock
Copy link
Contributor

mprorock commented Jun 9, 2023

thank you @OR13 !!!

andresuribe87
andresuribe87 reviewed Jun 9, 2023
View reviewed changes
Copy link
Contributor

@andresuribe87 andresuribe87 left a comment

Choose a reason for hiding this comment

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

This is awesome! Thanks @OR13

index.html Show resolved Hide resolved
index.html
Comment on lines +550 to +551
<h2>Controllers</h2>
<pre class="example" title="A verifiable credential controller document">
Copy link
Contributor

Choose a reason for hiding this comment

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

This example seems out of place. What's the intent of putting a DID document example in vc-jwt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its necessary to explain controller documents, because they are related to iss and kid, similar to:

https://w3c.github.io/vc-data-integrity/#controller-documents

Copy link
Contributor

Choose a reason for hiding this comment

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

Just read that section, and I have to say that it's not intuitive at all. I think it would be worth either: adding an example that refers to this controller document, pointing to the spec you mentioned, or explicitly stating the relationship between controller docs and iss & kid.

Copy link
Contributor

Choose a reason for hiding this comment

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

Created #106

index.html Show resolved Hide resolved
index.html
Presentations
</h2>

<pre class="example" title="Credential">
Copy link
Contributor

@andresuribe87 andresuribe87 Jun 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
<pre class="example" title="Credential">
<pre class="example" title="A Verifiable Presentation referencing and containing Verifiable Credentials">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, but it does not contain them... it contains one and references many, can you amend your suggestion to "A Verifiable Presentation referencing and containing Verifiable Credentials" ? or something similar?

index.html
Comment on lines +712 to +714
"did:example:123",
"urn:uuid:01ec9426-c175-4e39-a006-d30050e28214",
"urn:ietf:params:oauth:jwk-thumbprint:sha-256:_Fpfe27AuGmEljZE9s2lw2UH-qrZLRFNrWbJrWIe4SI",
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious how these 3 items are Verifiable Credentials?

Copy link
Contributor Author

@OR13 OR13 Jun 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

they are not, but this RDF term defines what verifiableCredential means when it is in a presentation.

https://github.com/w3c/vc-data-model/blob/main/contexts/credentials/v2#L84

As you can see, its not defined in the vocabulary... cc @iherman ...

https://www.w3.org/2018/credentials#verifiableCredential

However, you can tell from the @context the above syntax is intended, see:

"verifiableCredential": {
          "@id": "https://www.w3.org/2018/credentials#verifiableCredential",
          "@type": "@id", // this tells you `id` values are acceptable
          "@container": "@graph" // this tells you members are separate graphs.
        },

index.html Outdated Show resolved Hide resolved
index.html
data:application/vp+ld+json;base64,eyJAY29udGV4dCI6WyJodHRwczovL3d3dy53My5vcmcvbnMvY3JlZGVudGlhbHMvdjIiXSwidHlwZSI6WyJWZXJpZmlhYmxlUHJlc2VudGF0aW9uIl0sInZlcmlmaWFibGVDcmVkZW50aWFsIjpbImRhdGE6YXBwbGljYXRpb24vdmMrbGQranNvbjtiYXNlNjQsZXlKQVkyOXVkR1Y0ZENJNld5Sm9kSFJ3Y3pvdkwzZDNkeTUzTXk1dmNtY3Zibk12WTNKbFpHVnVkR2xoYkhNdmRqSWlYU3dpZEhsd1pTSTZXeUpXWlhKcFptbGhZbXhsUTNKbFpHVnVkR2xoYkNKZExDSnBjM04xWlhJaU9pSm9kSFJ3Y3pvdkwybHpjM1ZsY2k1MlpXNWtiM0l1WlhoaGJYQnNaU0lzSW5aaGJHbGtSbkp2YlNJNklqSXdNak10TURZdE1EZFVNakU2TVRRNk1UUXVNVFE0V2lJc0ltTnlaV1JsYm5ScFlXeFRkV0pxWldOMElqcDdJbWxrSWpvaWFIUjBjSE02THk5emRXSnFaV04wTG5abGJtUnZjaTVsZUdGdGNHeGxJbjE5Il19
</pre>

<pre class="example" title="Nested Unsecured JWT">
Copy link
Contributor

Choose a reason for hiding this comment

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

The nested threw me off for a bit. What's the purpose of base64 encoding a JWT, which is already base64 encoded?

Copy link
Contributor Author

@OR13 OR13 Jun 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Compliance with https://datatracker.ietf.org/doc/html/rfc2397 requires it.

This highlights why COSE is nice, helps avoid the double encoding.

Regarding why this particular example, we need to show examples that are confusing / dangerous, because this is a security specification and we need to warn implementers about them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation!

Copy link
Contributor

Choose a reason for hiding this comment

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

confusing

Double encoding bad

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@OR13 OR13 mentioned this pull request Jun 9, 2023
Open
OR13 and others added 5 commits June 9, 2023 14:59
@OR13 @andresuribe87
Update index.html
e132847 
Co-authored-by: Andres Uribe <[email protected]>
@OR13 @andresuribe87
Update index.html
394c034 
Co-authored-by: Andres Uribe <[email protected]>
@OR13 @andresuribe87
Update index.html
e2594ed 
Co-authored-by: Andres Uribe <[email protected]>
@OR13 @andresuribe87
Update index.html
285f8f9 
Co-authored-by: Andres Uribe <[email protected]>
@OR13 @andresuribe87
Update index.html
b34f7e5 
Co-authored-by: Andres Uribe <[email protected]>
@OR13
Copy link
Contributor Author

OR13 commented Jun 9, 2023

@andresuribe87 Thank you for such a thorough review!

I have accepted most of your suggestions, if you agree with the refinements to the ones I did not accept yet, please apply them and ping me to review again.

@andresuribe87 andresuribe87 mentioned this pull request Jun 9, 2023
Closed
andresuribe87
andresuribe87 approved these changes Jun 9, 2023
View reviewed changes
Copy link
Contributor

@andresuribe87 andresuribe87 left a comment

Choose a reason for hiding this comment

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

@OR13 I've updated the suggestions so you can apply them. This is 🔥

@mprorock mprorock merged commit 8fde162 into main Jun 15, 2023
@dlongley dlongley mentioned this pull request Jun 25, 2023
Closed
@decentralgabe decentralgabe deleted the feat/better-v2-examples branch February 26, 2024 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mprorock mprorock mprorock approved these changes

@dlongley dlongley dlongley left review comments

@andresuribe87 andresuribe87 andresuribe87 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@OR13 @iherman @mprorock @dlongley @andresuribe87