Skip to content

feat: add name and description #752

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

Closed
wants to merge 15 commits into from

Conversation

tplooker
Copy link

@tplooker tplooker commented Nov 2, 2020

Many use-cases for verifiable credentials require rendering them in some form of user experience, in these instances having a standard way to represent a name and description for a credential helps.

This PR does the following

  1. Adds two new properties to the core data model to support these expressions
  2. Defines a v2 of the context defining the new properties
  3. Shifts the v2 context to not use compact iri's

Outstanding questions

  1. Should the name and description fields have a maximum length?
  2. Should we define other properties that help with rendering a credential such as an image for a credential? If so what constraints would we apply to this property?

Preview | Diff

@tplooker tplooker requested a review from msporny November 2, 2020 01:43
@msporny
Copy link
Member

msporny commented Nov 2, 2020

Should the name and description fields have a maximum length?

Probably not, we've avoided specifying definitive lengths for human readable strings. The assumption here is that we're not dealing w/ space-constrained devices... and if folks do want to put limits, they'll enforce that via JSON Schema or API constraints.

Should we define other properties that help with rendering a credential such as an image for a credential? If so what constraints would we apply to this property?

I've been meaning to write back on that specific topic. There is a non-trivial tracking/privacy danger with images and any sort of URL that's requested to render a VC. That's one of the driver's of hashlinks -- the ability to cache based on a content-addressed identifier.

Digital Bazaar has also explored using iframes, sandboxed content, SVGs, and other approaches to provide richer experiences. We are fairly convinced that a name, description, and image will not work at scale given the combination of issuer requirements, privacy requirements, and credential size requirements. This is a fairly rich area and we should be careful about what we do in the space.

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, a good start. Made some change suggestions. We're going to want to do more surgery on the v2 context... I expect @OR13 will want to inject the JsonWebSignature2020 stuff in here. We'll want to upgrade all the examples to use Ed25519Signature2020 as well.

@OR13
Copy link
Contributor

OR13 commented Nov 2, 2020

This is good, but it's unclear what changes need to be made to accept it.

tplooker and others added 4 commits November 3, 2020 08:50
}
},

"RsaSignature2018": {
Copy link
Member

Choose a reason for hiding this comment

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

After looking into this more in the perspective of the v3-unstable security vocab, I'd prefer to drop these out of the V2 context here. Instead opting to have VCs use the security context as a second context or to list it as a secondary context in this so that only this context needs to be included still.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kdenhartog I believe that the VC WG objected to the idea of suggesting multiple / open world contexts... ironic, given the purpose of linked data....

To me this is similar to DID Core issue, we should have something like:

"@context": [
"https://example.com/stable/credential/formats/v1",
"https://extensions.example.com/unstable/credential/formats/v1",
]

Copy link
Member

Choose a reason for hiding this comment

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

In that case we'd want to define this context such that it imports in the new v3 security context. Technically we're doing the same thing, but the data model of the VCs wouldn't show it.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to separating those terms out into the v3 security context instead of duplicating them here.

@kdenhartog
Copy link
Member

I'd prefer to see us separate out the HTML changes where we're defining the terms from the update to the v2 context. I've got a few other changes as well that I'd like to see go into v2 context as well, but would rather see us agree on them all together so that we can make a WG decision to address issue #755 and add a stable version of that context.

@tplooker
Copy link
Author

tplooker commented Nov 19, 2020

Since the purpose of this PR is just to add two new properties to the core data model, should we either

  1. Just simplify this PR to address the spec changes and not update/create a new context
  2. Create a new context as a copy of v1 with the new properties added (what this PR is currently doing)
  3. Create a new context that tackles other changes we want to see in v2 (e.g refactoring how we handle security related terms)

@kdenhartog I believe your request is suggesting we do 1?

@kdenhartog
Copy link
Member

Since the purpose of this PR is just to add two new properties to the core data model, should we either

1. Just simplify this PR to address the spec changes and not update (create a new context)

2. Create a new context as a copy of v1 with the new properties added (what this PR is currently doing)

3. Create a new context that tackles other changes we want to see in v2 (e.g refactoring how we handle security related terms)

@kdenhartog I believe your request is suggesting we do 1?

Yup for this PR just do option 1 in my opinion, and we'll start getting a second context defined so that we can present it to the WG and get approval to address #755 (which I'm not certain if we can do that right now without changes in charter or anything like that because it's a normative change)

@tplooker
Copy link
Author

@msporny @peacekeeper @OR13 thoughts on the above ^?

@OR13
Copy link
Contributor

OR13 commented Nov 23, 2020

I suggest making this PR spec text changes only.

@tplooker
Copy link
Author

@OR13 changes to the context have been reverted out of this PR

@kdenhartog
Copy link
Member

@OR13 changes to the context have been reverted out of this PR

Awesome thanks! I'll take care of the V2 VC Context. I've already got a partially working one that I've been playing around with to make everything work together better.

@tplooker
Copy link
Author

tplooker commented Dec 2, 2020

ping @msporny @burnburn @iherman what are the next action items w.r.t this PR?

@iherman
Copy link
Member

iherman commented Dec 2, 2020

ping @msporny @burnburn @iherman what are the next action items w.r.t this PR?

There are several steps here

  1. I do not know what the "worfklow" was in the previous VC WG for the acceptance of PR-s (I was not part of that WG). There should be some sort of WG consensus to merge this, because it is an addition to the spec. Some sort of an email vote, for example.
  2. My comment on another PR partially stands. It is not an erratum, i.e., that part of my comment is irrelevant. But my note whereby there must be a change section in the final document, where this is duly recorded, stands.
  3. At some point in the future, when there are enough changes, we may want to re-issue a Rec. But that is down the line for now.

Copy link
Contributor

@peacekeeper peacekeeper left a comment

Choose a reason for hiding this comment

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

Adding name and description feels like a good idea. The historical Information Card design also included a name and description for each credential/card (e.g. see this old interface definition).

@kimdhamilton
Copy link
Contributor

kimdhamilton commented Dec 4, 2020

This is the first proposed change to the VC spec under the maintenance charter, so I'm uncertain about how to move this forward.

I recommend giving this some air time at a future (hopefully next) CCG meeting to discuss. Ideally we'd have a quorum of @msporny, @burnburn, and @iherman to help with this.

fyi: @wyc and @vsnt

Created w3c-ccg/community#170 with "review next" label to get air time

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.

Minor nits, otherwise LGTM. It's arguable that this is a normative change to the specification. There is normative language, but implementations wouldn't necessarily need to change (because everything is a MAY), so technically, the test suite wouldn't change. That said, the /v2 JSON-LD Contexts would cause test suite changes... so, I guess that would make this hard to argue that it's not normative. This change would require a 3 month CR/PR-period, which is fine... we'll just have to make the call on when to trigger that -- should be a CCG discussion topic.

@tplooker
Copy link
Author

@msporny addressed your comments, however the outstanding question is we will have to modify the normative statement around @context processing see here

The value of the @context property MUST be an ordered set where the first item is a URI with the value https://www.w3.org/2018/credentials/v1. For reference, a copy of the base context is provided in Appendix § B. Base Context. Subsequent items in the array MUST express context information and be composed of any combination of URIs or objects. It is RECOMMENDED that each URI in the @context be one which, if dereferenced, results in a document containing machine-readable information about the @context.

@msporny
Copy link
Member

msporny commented Feb 20, 2021

@msporny addressed your comments, however the outstanding question is we will have to modify the normative statement around @context processing see here

Well, yes, we'll have to change that. That's what makes this whole PR a substantive change and potentially not a "maintenance work item". This is a v1.1 (or v2) specification change we're discussing... implementations will have to change, the planets will have to align and all that. Since we're going to that trouble anyway, we might as well do things like add the 2020 cryptosuites, deprecate the 2018 cryptosuites, etc. :)

Food for thought... but this PR needs to change that normative statement as well.

The other decision we may want to make is on versioning... do we want the context to be ../credentials/v1.1 or ../credentials/v2? This is definitely a breaking change from previous versions, so major revision in semver... but do we really want to issue a Verifiable Credentials v2.0 specification"? I'm leaning toward the latter... major breaking change, multiple cryptosuites affected, v2, let's just commit to it now.

@msporny msporny added the v2 label Feb 20, 2021
@msporny msporny changed the base branch from main to v2 February 20, 2021 16:43
@msporny
Copy link
Member

msporny commented Feb 20, 2021

I have cleaned up all old stale branches and setup the current branch structure for the repo:

  • main - this is where changes to the next non-breaking release goes (bug fixes, errata, etc) -- the next target release is v1.1
  • v2 - this is where major breaking changes/additions go (new terms, contexts, features, etc.) -- the next target release is v2.0

This PR has been marked as a v2 feature and the target branch has been changed to v2. This enables the current repo to be used for both v1.1 and v2 work.

NOTE: None of this has any immediate effect on anything -- it's merely a bookkeeping measure for the Editors until the VC Maintenance WG can meet and decide if this structure is appropriate.

@RieksJ
Copy link

RieksJ commented Jul 27, 2022

@brentzundel just mentioned this PR in #248. This feature may have the potential to close #248. The idea there is to put data in a VC for the purpose of showing it to the user, e.g., as she browses through the VCs. It's a 'pet name' as @ChristopherA calls it. This implies that, while it seems a good idea to have an issuer supply the initial name and description, but enable the holder to change it so that it can fit in the holders world (i.e. the 'namespace' that every person manages for itself). I haven't thought over any implications, but it might seem reasonable to either have the issuer not sign these attributes (so the holder can change them without affecting the real contents), or to provide ways in which properties can be added to a VC by the holder regardless of the VC.

@peacekeeper
Copy link
Contributor

either have the issuer not sign these attributes (so the holder can change them without affecting the real contents), or to provide ways in which properties can be added to a VC by the holder regardless of the VC.

Interesting topic. I think it definitely makes sense to be able to associate unsigned (meta-)data with VCs such as petnames, but not sure which approach is best. This feels like a broader non-VC-specific question that maybe Linked Data experts can answer. Looking at the Data Integrity and RDF Dataset Canonicalization specifications, my best guess is that the unsigned (meta-)data would have to somehow go into a separate RDF graph (container) than the rest of the data.

In any case, these are probably two separate issues. I believe this issue here is originally about signed name+description rather than unsigned petnames.

@iherman
Copy link
Member

iherman commented Jul 27, 2022

Putting on my former linked-data-expert-hat :-)

Interesting topic. I think it definitely makes sense to be able to associate unsigned (meta-)data with VCs such as petnames, but not sure which approach is best. This feels like a broader non-VC-specific question that maybe Linked Data experts can answer. […], my best guess is that the unsigned (meta-)data would have to somehow go into a separate RDF graph (container) than the rest of the data.

That would work, but it means that the metadata portion should be very clearly separated from the rest. Ie,

{
    "id" : "https://example.org/id",
    "pet_name" : "Bobby",
    "pet_type" : "Golden Retriever" 
}

would not really work; one should, rather, say something like:

{
    "id" : "https://example.org/id",
    "metadata" : {
        "pet_name" : "Bobby",
        "pet_type" : "Golden Retriever" 
    }
}

and fiddle around the @context file to make it clear that the object of metadata is a separate RDF graph. Actually, regardless of the RDF serialization aspect, I think it is a good idea and engineering practice to clearly separate the metadata from the rest of the data (this may affect the current PR which does not seem to do that!). This would be analogous to the <meta> tag of, say, HTML…

(How exactly the Data Integrity spec and the RDF Dataset Canonicalization spec will specify that, although we have a collection of RDF Datasets, some of the constituent datasets should not be signed is, I believe, doable but not done at this moment. Something for the RCH WG?)

In any case, these are probably two separate issues. I believe this issue here is originally about signed name+description rather than unsigned petnames.

Maybe so, I do not know whether this was so clearly stated. If that is not the case, ie, these name+descriptions are not to be signed, then we would have to clearly separate them from the rest, imho, even in the current PR.

@msporny
Copy link
Member

msporny commented Jul 27, 2022

I haven't thought over any implications, but it might seem reasonable to either have the issuer not sign these attributes

There are dangerous implications here. One of the biggest flaws we could introduce at this point is "unsigned data in the VC" -- at present, I'm a very strong -1 to that because it introduces the possibility that a developer would process data from within a package that they thought was signed, that wasn't signed.

This "how to display things to the individual" is a "digital wallet" problem, not something we need to address at the VC layer. If the individual wants to give their credentials pet names, the wallet software can provide that mechanism, and even create an exportable version of that (as a VC, so you know who set the petnames).

That said, this is a separate topic. @RieksJ, you should raise an issue if you'd like to pursue the idea/concept in the VCWG.

@RieksJ
Copy link

RieksJ commented Jul 27, 2022

@msporny mentions "it introduces the possibility that a developer would process data from within a package that they thought was signed, that wasn't signed". If it is a matter of choice between requiring developers to do a proper job (for which they are hired) as opposed to set requirements for holders (with various (dis)abilities), I'd go for the first.

The issue of 'pet names' is indeed a "how to display things to the individual", but not exclusively a "digital wallet" problem. It is also for the user, discussions over which were started as issue #248. So no need to raise yet another issue.

I can live that. And for @brentzundel: that would imply that #248 is NOT addressed as part of this PR.

@iherman
Copy link
Member

iherman commented Jul 27, 2022

The issue was discussed in a meeting on 2022-07-27

  • no resolutions were taken
View the transcript

5.3. Add a summary text as meta information (issue vc-data-model#248)

See github issue vc-data-model#248.

Brent Zundel: This has received some back and forth alongside another PR.
… The proposal here is that we add some summary text/meta info that would allow the issuer or the holder to indicate "this is what this credential is about".
… A 'summary property' could be useful.
… I suggested that PR752 addressed this, but it seems not.

Manu Sporny: +1 to PR that tplooker raised as addressing this..

Brent Zundel: There some folks on the call who raised comments....

Manu Sporny: My understanding of Tobias's proposal is to add name and description.

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

Manu Sporny: I'd say the description is the summary.
… Do we need name, description and summary?.
… There are some preliminary proposals for rendering.
… if we can't describe the difference between summary and description then go with tplooker.

Ted Thibodeau Jr.: It seems the me the issuer will have no idea what pet name the holder may use.
… The pet name might go into an envelope that contains the credential but that's not within the VC.

Manu Sporny: -1 to petnames in VCs... associated metadata, fine, but that's a Holder preference..

Ivan Herman: That was a bit of a discussion on the pull request, not the issue.
… Are these additional metadata things to be signed/normalised?.

Manu Sporny: -1 for "bag of metadata" :) -- like, we have a mechanism for semantics... let's use those..

Ivan Herman: That's relevant because one way is to define a bunch of properties. The other way is to say there is a single metadata property that points to a bunch of stuff that is not part of the credential.
… Personally I'd prefer to separate the metadata from the core set of statements, but we have to decide on that.

Tobias Looker: yeah I agree with manu.

Ivan Herman: If you don't want to sign it then it's separate.

Brent Zundel: No time for all comments here. Please add your comments to the issue (not the PR).

Tobias Looker: The original proposal is to add name and description. I see description and summary as synonyms..
… The other clarification - it is proof format-specific, but it's issuer-signed.
… I don't think that stops a wallet also assigning a pet name.

Kristina Yasuda: Thanks everyone.

Manu Sporny: +1 to what tplooker just said..

Kristina Yasuda: We're running out of time, so DavidC, JoeAndrieu oliver please comment on the issue..
… Sorry we're over time.


@logpo
Copy link

logpo commented Aug 3, 2022

I haven't thought over any implications, but it might seem reasonable to either have the issuer not sign these attributes

There are dangerous implications here. One of the biggest flaws we could introduce at this point is "unsigned data in the VC" -- at present, I'm a very strong -1 to that because it introduces the possibility that a developer would process data from within a package that they thought was signed, that wasn't signed.

I agree that it is dangerous to mix signed and unsigned data for this use case. I don't see any reason why metadata can't sit outside the VC when it is just for the holder to manage their credentials.

@msporny
Copy link
Member

msporny commented Aug 3, 2022

don't see any reason why metadata can't sit outside the VC when it is just for the holder to manage their credentials.

Yes, agreed. There will be metadata that systems that manage VCs will associate with those VCs... petnames being one of the potentials there. That metadata does not have to go in the VC, rather, it can live "beside" the VC in the system that manages the VCs.

We should probably put something to this effect in the implementation guide.

@logpo
Copy link

logpo commented Aug 3, 2022

I think there is still a case for a standard place to put a human readable name and description, but it should be signed with the rest of the VC. And if the holder want's to assign a different name after signing then that should be stored next to the VC and not in it

@TallTed
Copy link
Member

TallTed commented Aug 5, 2022

I think there is still a case for a standard place to put a human readable name and description

Sure, similar to the standard VISA or MC or AMEX logo on their plastic, which the human carrier may tag in various ways to help them pull the employer-expense-card, or the triple-miles-bonus, etc. -- none of which human tags are actually part of the physical card nor stored in the data bearing mag-strip.

@msporny
Copy link
Member

msporny commented Aug 14, 2022

Raised PR #905 to implement the JSON-LD Context portion of what this PR was going for. Once we have agreement on the v2 context, we can adjust the main specification to update all of the examples to include names and descriptions for VCs.

@msporny
Copy link
Member

msporny commented Aug 24, 2022

name and description have been added to the v2 context in PR #905. Closing.

@msporny msporny closed this Aug 24, 2022
@OR13 OR13 mentioned this pull request Feb 24, 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.