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 initial spec #57

Merged
merged 26 commits into from
Feb 3, 2024
Merged

Add initial spec #57

merged 26 commits into from
Feb 3, 2024

Conversation

marcoscaceres
Copy link
Collaborator

@marcoscaceres marcoscaceres commented Jan 24, 2024

Adds the initial spec based on latest proposal.

closes #61


💥 Error: 502 Bad Gateway 💥

PR Preview failed to build. (Last tried on Feb 3, 2024, 12:42 AM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Spec Generator - Spec Generator is the web service used to build specs that rely on ReSpec.

🔗 Related URL

error code: 502

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

index.html Outdated Show resolved Hide resolved
Copy link
Collaborator

@samuelgoto samuelgoto left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together!! It is a great starting point and is really going to help us find the convergence that we need.

Comments inline!

index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
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.

I think it would probably be better to initialize registries as empty in the initial setup, and then add their contents in separate PRs.

Copy link

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

It would be good to have IETF SD-JWT VC as well.

Copy link

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

VCDM 2.0 is not a protocol. It technically is agnostic to securing mechanisms. IMO, this should be more specific.

Copy link
Contributor

@Sakurann Sakurann left a comment

Choose a reason for hiding this comment

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

Should also include IETF SD-JWT VC and not just ISO mdoc and W3C VCDM

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated
</h2>
<pre class="idl">
partial interface CredentialsContainer {
Promise&lt;DigitalCredential&gt; requestIdentity(IdentityRequestOptions options);

Choose a reason for hiding this comment

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

Reiterating from the call: It may be useful to abstract the DigitalCredential return type here and design an algorithm that both FedCM and DigitalCredential can hook into to provide identities. That way FedCM can intermingle in UI with 3-party ids and also with WebAuthN, but mDLs and WebAuthN can't be requested interchangably, which was the motivating concern for this API shape IIRC.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I haven't read the FedCM spec yet... but yeah, so long as this can be a generic thing without a normative requirement on FedCM, then that works.

I'm imagining DigitalCredential (or whatever we end up calling it) will end up in the Credential Management spec eventually.

Choose a reason for hiding this comment

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

That sounds perfect. That just means FedCM would subclass DigitalCredential, in the same way it currently subclasses Credential and would put itself in the registry.

I agree that the generic DigitalCredential (or whatever) would live well in the Credential Management spec.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we try to agree with the goals here first?

both FedCM and DigitalCredential can hook into to provide identities

This matches much of my intuition, and it is what Chrome would like to see this design converge to.

@marcoscaceres, does this match your intuition?

Copy link
Collaborator Author

@marcoscaceres marcoscaceres Jan 31, 2024

Choose a reason for hiding this comment

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

I don't have a strong opinion (because I'm still not very familiar with FedCM). If both specs can use the same thing, then great. However, the challenge remains with .get() conflating requesting multiple credential types, and it's impossible for an implementation to distinguish (because they are all part of the same CredentialCreationOptions dictionary).

In the Extensions Points section, the CM spec encourages spec authors to define "a reasonable mechanism for dealing with requesting credentials of distinct types", which is what .requestIdentity() is trying to do, and then to pitch that to the Web App Sec WG.

I think we need to document our options here #61 and present our options to the Web App Sec WG to help us decide what the right course of action is here.

I think we have general agreement that this poses issues:

const opts = {  publicKey , identity,  providers };
// who wins? what happens? What UI gets shown? 
navigator.credentials.get(opts);

Which, again, is the core of the problem - which the CM spec acknowledges is a problem, by asking for "a reasonable mechanism for dealing with requesting credentials of distinct types."

The . requestIdentity() disambiguates the problem by providing a unique method signature, only allowing this new DigitalCredentialRequestOptions dictionary, which deliberately does not inherit from CredentialRequestOptions to avoid its limitations, while providing the same functionality (i.e., it's built for purpose).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just as tl;dr: let's move this discussion to #61

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I think we arrived at a good conclusion on #61. This issue here is now resolved from where I stand, and since we are following @bvandersloot-mozilla 's suggestion, I think it would be consistent with his thoughts. @marcoscaceres is there anything else that you think is actionable here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm happy to proceed. Let's do additional discussions in a follow up.

marcoscaceres and others added 8 commits January 25, 2024 15:15
Co-authored-by: sam goto <samuelgoto@gmail.com>
Co-authored-by: sam goto <samuelgoto@gmail.com>
Co-authored-by: sam goto <samuelgoto@gmail.com>
Co-authored-by: sam goto <samuelgoto@gmail.com>
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Copy link
Collaborator

@samuelgoto samuelgoto left a comment

Choose a reason for hiding this comment

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

This PR really captured what I think is a design that will reconcile many different aspects of the space: reconciling the two party model with the three party model, reconciling the distinction between identity verification and sign-in, reusing the Credential Management spec and algorithms, etc.

I made some comments to try to clarify what I think is already shared understanding.

The backbone and the structure of this PR is solid. I'm playing with the code to see what would be involved in implementing, but my intuition is that this would set us in the right long term direction.

@bvandersloot-mozilla WDYT?

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
TBW
</p>
<h2>
Model
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are operating at the browser level, so our definition of "Credential" (in the Credential Manager sense) is different than other group's definition of "Credential", but there are many other concepts that I think are well articulated by the ISO and Verifiable Credentials community.

Here is one example of a data model that I think has a lot of good definitions.

I'm wondering if we are going to get held back agreeing on these definitions, and if we are better off just having the WebIDLs in this initial PR to unblock implementation ... WDYT?

Happy to go either way, but my intuition is that the WebIDL without the definitions more minimally viable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's run with this for now but let's definitely start taking definitions/concepts from other specs if we can. However, we should control whatever we feel strongly fall squarely in the purview of our spec.

index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
marcoscaceres and others added 2 commits February 3, 2024 11:00
Co-authored-by: sam goto <samuelgoto@gmail.com>
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
Co-authored-by: sam goto <samuelgoto@gmail.com>
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@samuelgoto samuelgoto mentioned this pull request Feb 3, 2024
10 tasks
index.html Outdated Show resolved Hide resolved
Co-authored-by: sam goto <samuelgoto@gmail.com>
Copy link
Collaborator

@samuelgoto samuelgoto left a comment

Choose a reason for hiding this comment

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

LGTM

There is a lot missing in this spec right now, but it contains a great baseline that we can work from!

Thanks for putting this together!

@marcoscaceres marcoscaceres merged commit fea3b06 into main Feb 3, 2024
1 check passed
@marcoscaceres marcoscaceres deleted the initial_spec branch February 3, 2024 00:47
@RByers
Copy link
Member

RByers commented Feb 3, 2024

retroactive LGTM as well, I like this direction. Thanks Marcos and Sam!

@bc-pi
Copy link

bc-pi commented Feb 3, 2024

Is the "rendered" version of this available and if so where? (sorry if that's an ignorant question - I am largely unfamiliar with w3c tooling and stuff)

@samuelgoto
Copy link
Collaborator

https://wicg.github.io/digital-credentials/

@bc-pi
Copy link

bc-pi commented Feb 3, 2024

Thanks @samuelgoto !

@marcoscaceres
Copy link
Collaborator Author

Renamed it to https://wicg.github.io/digital-identities/ ... to match the new spec title.

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.

Is "requestIdentity()" the best method name and do we need it?