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 JSON Web Token Examples #60

Closed
wants to merge 1 commit into from
Closed

Add JSON Web Token Examples #60

wants to merge 1 commit into from

Conversation

OR13
Copy link
Contributor

@OR13 OR13 commented Apr 21, 2023

@OR13
Copy link
Contributor Author

OR13 commented Apr 21, 2023

I generated these examples from this implementation: https://github.com/transmute-industries/vc-jwt-status-list

I plan to add similar examples to this test suite:

https://github.com/transmute-industries/vc-jwt-test-suite

Copy link
Contributor

@mprorock mprorock left a comment

Choose a reason for hiding this comment

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

Thank you @OR13 !

</pre>
<pre class="example nohighlight" title="JSON Web Token Verifiable Credential With Status (Verified)">
{
"suspension": false,
Copy link
Member

Choose a reason for hiding this comment

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

What is this property for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a json member in the verification result reflecting the status bit for the credential.

It is an implementation artifact, I can remove it.

AFAIK, there is no consensus on how status impacts verification, or how to express that in a "verified JWT".

SafeJWT APIs tend to return an object with protected header and decoded and parsed payload after verification, I added this object member to that to communicate status, but it could just as easily be left to verifier to decide how to service the status check in a verification result.

Last I checked, data integrity does a similar thing for verified and status, but it's possible that has changed.

"id": "http://example.com/credentials/1872",
"type": [
"VerifiableCredential",
"NewCredentialType"
Copy link
Member

@msporny msporny Apr 25, 2023

Choose a reason for hiding this comment

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

Undefined term, please use a context that defines the term given that this is an example in a specification.

Suggest that we define a https://www.w3.org/ns/credentials/examples/v2 context that maps everything to an examples vocabulary document, much like undefined terms, but specifically state that this property is an example property and only used for examples.

Either that, or use a real context (traceability vocab would be fine).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-1 to adding industry specific contexts to examples

-1 to inventing a new examples context (vocab expands these terms automatically).

Copy link
Member

@msporny msporny left a comment

Choose a reason for hiding this comment

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

Overall, +1 to this PR. There is one question and one request for change before merge.

Copy link
Contributor

@mkhraisha mkhraisha left a comment

Choose a reason for hiding this comment

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

+1 pending manu's comments

"id": "did:example:123",
"type": [
"Organization",
"OrganizationType"
Copy link
Contributor

Choose a reason for hiding this comment

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

These are also undefined, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are not "undefined", they are "vocab defined".

Comment on lines +939 to +943
"type": [
"Person",
"JobType"
],
"claimName": "claimValue"
Copy link
Contributor

Choose a reason for hiding this comment

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

More undefined things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More vocab defined things.

@OR13
Copy link
Contributor Author

OR13 commented Apr 26, 2023

@msporny @dlongley I can remove all the terms that are not defined in the v2 context, but then why does the v2 context contain an vocab?

I think you are assuming the intention of the issuer, when you say these terms are "undefined"... the language we are using now is "issuer dependent".

@dlongley
Copy link
Contributor

dlongley commented Apr 26, 2023

@OR13,

I think it's important that we have "5 star" or "best practice" (or whatever we want to call these) examples that only use non-issuer-specific vocabularies first -- and we call them out as the preferred thing to do. We do not want to encourage everyone to increase the number of potentially "opaque" (or rejected!) VCs in digital wallets.

We can also have examples that show issuer-dependent terms, but we should say that those come with an additional burden, e.g., of digital wallets having to understand every issuer's definition of a term vs. just a single definition from an independent vocab that works across multiple issuers (for a given "type" of VC).

So, these things are not the same and just a matter of preference with no external consequences. Issuer-dependent terms can potentially generate market failures. There's significant burden added for "issuer-dependent" terms that should be understood from the three party model perspective. That approach preferences popular issuers by shaping markets because the increased implementation burden on digital wallet providers results in resources being allocated only to these popular issuers. Among other things, this can lead to a repeat of the NASCAR problem and difficulty with smaller issuers being able to get support. We should not suggest, as a first example, that people use "issuer-dependent" terms.

Copy link

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

I think it makes sense to have VC-JWT examples.

Copy link
Contributor

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

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

This aligns with the representation we are using in our service software and I'm supportive of this PR.

</pre>
<pre class="example nohighlight" title="JSON Web Token Verifiable Credential With Status (Verified)">
{
"suspension": false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"suspension": false,

"Person",
"JobType"
],
"claimName": "claimValue"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"claimName": "claimValue"
"email": "someperson@example.org"

"id": "did:example:123",
"type": [
"Organization",
"OrganizationType"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"OrganizationType"

"id": "http://example.com/credentials/1872",
"type": [
"VerifiableCredential",
"NewCredentialType"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"NewCredentialType"

@mprorock
Copy link
Contributor

@dlongley do the suggestions i made above resolve your concerns?

@mprorock
Copy link
Contributor

@msporny if the suggestions made above are merged, do they fix your concerns?

@dlongley
Copy link
Contributor

dlongley commented May 24, 2023

@dlongley do the suggestions i made above resolve your concerns?

No, because issuer-dependent terms are still used as the core example. I think such terms should only be used as auxiliary and appropriately contextualized examples as I mentioned on the call because of their obvious disadvantages for the ecosystem as a whole -- and because developers often use examples (and only examples) as contact points in specifications.

I recommend using the examples v2 context as a way to resolve this.

@mprorock
Copy link
Contributor

@dlongley do the suggestions i made above resolve your concerns?

No, because issuer-dependent terms are still used as the core example. I think such terms should only be used as auxiliary and appropriately contextualized examples as I mentioned on the call because of their obvious disadvantages for the ecosystem as a whole -- and because developers often use examples (and only examples) as contact points in specifications.

I recommend using the examples v2 context as a way to resolve this.

@dlongley do you mind listing which terms bother you or make concrete code suggestions as to what would work for you then?

Copy link
Contributor

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

@mprorock,

I've provided my previous comment as a github suggestion.

Comment on lines +880 to +881
"https://www.w3.org/ns/credentials/v2",
"https://w3id.org/vc/status-list/2021/v1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"https://www.w3.org/ns/credentials/v2",
"https://w3id.org/vc/status-list/2021/v1"
"https://www.w3.org/ns/credentials/v2",
"https://www.w3.org/ns/credentials/examples/v2",
"https://w3id.org/vc/status-list/2021/v1"

Comment on lines +914 to +915
"https://www.w3.org/ns/credentials/v2",
"https://w3id.org/vc/status-list/2021/v1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"https://www.w3.org/ns/credentials/v2",
"https://w3id.org/vc/status-list/2021/v1"
"https://www.w3.org/ns/credentials/v2",
"https://www.w3.org/ns/credentials/examples/v2",
"https://w3id.org/vc/status-list/2021/v1"

@msporny
Copy link
Member

msporny commented May 25, 2023

@dlongley wrote:

I've provided my previous comment as a github suggestion.

If the change requests suggested by @dlongley are merged, I would be a +1 for merging this example.

@iherman
Copy link
Member

iherman commented May 25, 2023

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

  • no resolutions were taken
View the transcript

1.6. Add JSON Web Token Examples (pr vc-status-list-2021#60)

See github pull request vc-status-list-2021#60.

Orie Steele: We seem to have a difference of opinion regarding adding examples to the specification. And I would like to merge the examples as-is. I don't agree with the perspective that every example in our spec needs multiple context values.
… I don't agree with that and in light of bundling the Data Integrity context.
… This adds informative examples, can we merge this, please, thank you.

Greg Berstein: Please!

Manu Sporny: -1 to merge until the WG discusses it. With all the examples it keeps coming up and we need to have a consistent way that we're doing examples and figure out what it is so the editors can apply that uniformly across all the specs.

Phillip Long: is this a special topics call?

Kristina Yasuda: I'm up for spending 5-10 minutes on this call on this.

Orie Steele: lets remove the DataIntegrityProof RDF class from the core context... and make the examples include multiple contexts.

Manu Sporny: I don't know if we'll get through it in 5 minutes. The proposal is to just use the example in the core spec.
… Use it as an example across all the specs that need to use an example. That way we're just using an example that we've agreed to use across all the specs and we don't need to do variations of that in every other specification.

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

Kristina Yasuda: Are you fundamentally not ok with the example, Manu?

Manu Sporny: Yes.

Kristina Yasuda: Anyone else who has strong opinions about what examples we use?

Orie Steele: All the examples conformant to the core data model.

Michael Prorock: My question is along the same line... what precisely is wrong with the examples?
… At least by my read and going and running the example code it all checks out fine.

Manu Sporny: One of the things is that we have a suspension false entry in here, I don't know where that property came from, I think Orie said that's an artifact in the PR and it's not in the data model.
… There's a concern over the use of the undefined / issuer-independent context / vocab.
… Now we're using the issuer-dependent terms in the example.
… There are a number of people that said in the other PR that's not a good practice.
… We've had people say that they would rather the examples use the examples context instead of the issuer-defined one.
… I don't know why we're doing in status list when core does something else.

Dave Longley: A lot of developers will come along and look at examples to get things started, and there are such things as good examples and bad examples.

Orie Steele: IMO a good example is one that uses the minimal required fields.
it is bad form to include optional stuff in example, and bad form to include DataIntegrityProof in the core v2 context.

Dave Longley: You can create examples that are bad examples, which would be conformant, but wouldn't look good. It's not enough to say "We created an example and it validates so it's fine." If we get people started off on he wrong foot, or encouraging market failures, we're making mistakes. We should not be flippant about the examples we put in the spec. They're important, developers will look at these examples and maybe not the text.
… Saying "this verifies" is not enough, these are entry points into the spec and possibly exit points.

Kristina Yasuda: Sounds like we're leaving this issue open for now.
… Anything on the BBS cryptosuite stuff?

@OR13
Copy link
Contributor Author

OR13 commented Jun 13, 2023

Closing the PR, now that #67 is up, we expect digital bazaar to provide complete examples for VC-JWT using the plugin.

I intend to block CR for this item until working examples for both securing formats are in the spec.

@OR13 OR13 closed this Jun 13, 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.

None yet

8 participants