Update suggested API. #5

agl · 2023-02-10T17:05:08Z

This change updates the proposed API to use IdentityCredential instead.

davidz25

+1 from me on constructing the request via an API. I also like the fact that it's possible to generate on the server side. Thanks.

othermaciej · 2023-04-07T00:04:07Z

README.md

+dictionary IdentityProviderConfig {
+  required string scheme;

-    CredentialStorageDuration desiredStorageDuration;  // Not providing this is equivalent to not asking to store.
+  // Common fields across all identity provider types:
+  required DOMString nonce;


It's hard to follow how this integrates with FedCM or CredentialManager. A comment earlier says this extended a Credential Manager request, but what it links to is only specified in FedCM (afaict) and this IdentityProviderConfig definition is incompatible with the one in FedCM. It lacks two of the attributes defined in FedCM, has an extra attribute (scheme), and defines one attribute (nonce) with a different WebIDL type. What's the actual intent for how this integrates with FedCM and Credential Management? The PR (and ideally the post-PR explainer text) needs to spell it out.

othermaciej

This was difficult to review because the PR does not directly provide motivation, either in PR comments or in the changes to the Explainer document. It essentially deletes and replaces the existing API proposal; it appears to add two other specs as dependencies (Credential Management and FedCM) of which one is itself incubation-stage, making it harder for this proposal to progress and increasing the implementation cost; it makes simple requests for mdoc properties more verbose to express for content authors; and it increases the risk that websites will inappropriately try to use mdoc credentials as a form of user login/authentication, which is highly undesirable. These significant downsides need to be balanced by a strong upside, which is not really given here.

I have since been told there is an older closed PR and an open issue which give some of the motivation, but they aren't even linked here, and the PR and proposed Explainer text should explain these things directly for the benefit of future reviewers of the Explainer.

It was mentioned to me that the motivation is to allow an RP to interchangeably request info from either an mdoc or a FedCM provider as the source of common info (at the user's option). But it's not clear from this PR how these changes would make that use case possible, nor do they add anything to the explained that proposes this as a use case and justifies it as desirable and feasible. As it is, this PR seems like just a pun on two totally different senses of identity - the real-world identity document sense, and the federated login account sense. The ways to request an mdoc or a FedCM credential are divergent, what one gets back needs to be processed differently, and yet the same types are used. These choices need more explanation to evaluate whether this change is a good one.

othermaciej · 2023-04-07T00:25:49Z

README.md

-dictionary CredentialElement {
-    required DOMString namespace;  // As defined in ISO 18013-5 clause 8.
-    required DOMString name;
+We propose to build upon the Credential Manager’s “identity” request, which [Federated Credential Management](https://fedidcg.github.io/FedCM/#dictdef-identitycredentialrequestoptions) is also built on top of, with the following structures:


This says it's building on Credential Manager, but it's linking to FedCM, and as far as I can tell the linked interface is defined only in FedCM, not in Credential Management. Is the intent to push pieces in common between FedCM and Mobile Document Request API down to Credential Management? If so, I think that plan needs to be explained, both for reviewers of this PR and for readers of the Explainer if the PR is merged. If the plan is something else (such as this proposal adding a direct dependency on FedCM, or this proposal being merged into FedCM, then that too should be explained.

We've started an explainer for the common pieces here. We could incubate this separately in WICG if there's interest?

othermaciej · 2023-04-07T00:27:22Z

README.md

+partial dictionary CredentialRequestOptions {
+  IdentityCredentialRequestOptions identity;


This duplicates a partial dictionary defined in FedCM. I don't think you can define the same property on the same WebIDL interface in two places, so what is the plan here?

Fixed by IdentityCredential

othermaciej · 2023-04-07T00:31:10Z

README.md

+dictionary IdentityCredentialRequestOptions {
+  required sequence<IdentityProviderConfig> providers;

-    boolean forever;  // Cannot be used with any other properties.
+  // Common fields across all types of providers:

-    long days;  // Cannot (currently) be used with any other properties.
+  // Omitting `retention` implies that the identity information will not be stored.
+  IdentityStorageDuration retention;


This dictionary definition also exists in FedCM, but does not have the "retention" property. Is the proposal to add this to FedCM? Extend FedCM here? This isn't a partial interface.

Is retention meant to be common across all types of providers or no? It's hard to tell from the comment.

Fixed in latest PR. At the moment we're proposing keeping retention as an mdoc-specific thing from a desire to not go too far in trying to force things together as they're still evolving.

othermaciej · 2023-04-07T00:32:24Z

README.md

+  // Omitting `retention` implies that the identity information will not be stored.
+  IdentityStorageDuration retention;


This seems semantically incompatible with FedCM as written, since FedCM doesn't include retention at all, so it's always omitted, so that implies identity information will not be stored, but I'm pretty sure that the sense of "identity" that FedCM works with is intended to be stored indefinitely, in the sense that a federated login will persist on return visits; not never stored.

Agreed, let's keep retention mdoc-specific for now.

othermaciej · 2023-04-07T00:32:26Z

README.md

+dictionary IdentityStorageDuration {
+  // Exactly one of the following must be provided.
+  boolean forever;
+  long days;


Maybe this should be a double with +Infinity as the way to signal forever? Otherwise seems awkward to work with. Also, it's strange that "forever" is signaled in-band, but "no retention" isn't a boolean flag or a special value like 0, it's signaled out of band by omitting the duration property. That seems inconsistent.

Agreed thats simpler, done.

othermaciej · 2023-04-07T00:44:04Z

README.md

+partial dictionary IdentityProviderConfig {
+  // ...
+  // Federated provider specific configs, not covered here.
+  // ...


Why call it one dictionary type when the field definitions for this and FedCM are totally different? What is the benefit?

othermaciej · 2023-04-07T00:44:05Z

README.md

-    Exposed=Window,
-] interface CredentialRequest {
-    constructor(DOMString requesterIdentity, CredentialDocumentDescriptor documentDescriptor);  // This throws if anything in the `documentDescriptor` is not recognized (e.g. an invalid `documentType`).
+There is always a bijection from `IdentityCredentialRequestOptions` to JSON strings, and that bijection is implemented by `JSON.parse` and `JSON.stringify`. This allows requests to be generated on the server and easily transported to the user agent.


I suggest using plainer language than "bijection" because I'm betting in the strict mathematical sense it is wrong (JSON strings can vary in whitespace in a way that is not representable in the dictionary, so strictly speaking the mapping is not actually one-to-one and onto) and so will mislead people who know the term and take it literally, and will also confuse people who aren't familiar with the term at all. It's probably enough to say that this dictionary can be serialized and deserialized with JSON.parse and JSON.stringify without making claims about mathematical properties of these serialization or deserialization operations. This may sound pedantic, but in fairness, using the word "bijection" at all is kind of pedantic.

Agreed, fixed

othermaciej · 2023-04-07T00:52:11Z

README.md

+[Exposed=Window, SecureContext]
+interface IdentityCredential : Credential {
+  // The CBOR encoded `CredentialDocument` defined above, wrapped in base64.
+  DOMString token;


This definition is incompatible with the FedCM definition of IdentityCredential, which has a different WebIDL type for token. Also, why are these even the same type? It's not really reasonable to call the CBOR-encoded identity document in the mdoc case a "token", and the processing expected here and for FedCM seems entirely different. Credential Management is able to return different subtypes of Credential, so what is the benefit of treating this as the same credential type as one from FedCM? What kind of code would be able to generically work on either without type checking, that couldn't work on just a Credential?

We imagine that identity information will be passed between various pieces of tech stack where only the endpoints need to be able to parse and understand it. For example, in Chrome we currently plan to pass any information provided by the platform wallet through without parsing it ourselves. Website front ends may do the same, just passing the response on to some server component.

Agreed though that token is inappropriate. In our latest IdentityCredential proposal, we use an opaque response.

othermaciej · 2023-04-07T00:54:56Z

README.md

 };
 ```

+The `id` of the `IdentityCredential` for an mdoc response would be `mdoc`.


I guess it has to be something, but kind of highlights that Credential is an awkward fit as this field is not rally applicable. It can't even be used to distinguish mdoc credentials from others, because a user quite reasonably could have "mdoc" as the username.

Agreed, fixed

othermaciej · 2023-04-07T00:56:19Z

README.md

+let request = {
+  identity: {
+    retention: {
+      days: 90,
+    },
+    providers: [{
+      scheme: “mdoc”,
+      nonce: "gf69kepV+m5tGxMyMF0cnn9NCnRHez/LUIsFtLi6pwg=",
+      documentType: "org.iso.18013.5.1.mDL",
+      readerPublicKey: "ftl+VEHPB17r2oi6it3ENaqhOOB0AZbAkb5f4VlCPakpdNioc9QZ7X/6w...",
+      requestedElements: [
        { namespace: "org.iso.18013.5.1", name: "document_number" },
        { namespace: "org.iso.18013.5.1", name: "portrait" },
        { namespace: "org.iso.18013.5.1", name: "driving_privileges" },
        { namespace: "org.iso.18013.5.1.aamva", name: "organ_donor" },
        { namespace: "org.iso.18013.5.1.aamva", name: "hazmat_endorsement_expiration_date" },
-    ],
-    desiredStorageDuration: {
-        days: 7,
-    },
+      ],
+    }],
+  }
+};
+navigator.credentials.get(request).then(() => {


The new code example seems to be significantly more lines than the previous one. The extra verbosity is a cost on authors. There needs to be a really strong concrete benefit to justify it.

There's no example here showing how one would use Credential Management / FedCM integration to obtain data interchangeably from an mdoc or a federated login provider. How would one request from a federated login provider (say social.example) to provide any of the fields here? How could such an entity even report whether the user has driving privileges, or is registered as an organ donor? I'm lost thinking about this, so perhaps an example where the interoperability actually gets used would help motivate the PR.

See the example here

bslassey · 2023-04-07T21:56:53Z

I have since been told there is an older closed PR and an open issue which give some of the motivation,
@othermaciej I believe that PR #3 and issue #2 are the ones that were referenced.

agl · 2023-04-10T17:20:22Z

Thank you for the feedback!

This change was intended to be a quick aid to some ongoing discussions, but it has been hanging around longer than anticipated, so I apologize that the context was missing. I’ll try to fill it in:

We also want to see a clear separation between authentication and identity. While we cannot stop identity mechanisms from being used for authentication, we want that to be cutting against the grain. We started by putting this functionality into a new navigator.identity namespace, but ultimately decided that navigator.credentials was a better fit because of federation.

We would like federation to evolve into an identity mechanism, and for authentication needs to be served by passkeys. So we envision that a site would create an account by creating a passkey and then optionally attach identity attributes to that account, like an email address, by using federation. That's why FedCM currently uses navigator.credentials.get({identity: …}). But then if we put real-world identity functions in a navigator.identity namespace, we would have identity appearing in two different places. We felt that undermined the benefits of separation.

Federation is used for both authentication and identity today. We can't separate that in the short-term. And we do want to be able to support authentication requests which accept either a password, or passkey, or federated sign-in. (There are numerous examples of sites that have such a UI right now, although perhaps without the passkey option yet.)

So that's why we put the API into navigator.credentials.

As for tying it with FedCM: as mentioned, we would like to move federation towards being an identity mechanism and so we want to support sites being able to request identity attributes using either mDLs or federation. If mDLs are only ever used for mobile driver's licenses, that doesn't make a lot of sense. But we expect that the mDL data model will be inevitably used for other things too. For example, proving an academic affiliation is often implemented via a federation-like mechanism today (SAML, specifically), but it seems reasonable that it could be an mdoc too (e.g. University Certificates). (And the mdoc would have better privacy properties.) So an online research library might reasonably request proof of affiliation, but want to accept either mechanism.

While it is nice to be able to make invalid requests, like requesting a disjunction of an authentication and identity mechanism, inexpressible, it is only one method of communicating with developers. We expect that browsers would immediately reject any requests that ask for either a passkey or an mdoc (for example), and that communicates the separation pretty clearly too. There is also documentation, language in specs, established norms, etc to communicate the separation.

Another facet of the design comes from our experience with Web Authentication / passkeys. Web developers really want to be able to pass authentication requests/responses easily to the server, but WebAuthn makes frequent use of ArrayBuffers, which don’t work with JSON.stringify and so do not easily serialize into JSON. Thus one of the goals of the design here is that the request be in the subset of JavaScript that works with standard JSON functions.

We do propose that this spec be built on top of the IdentityCredential requested through the “identity” option of the Credential Manager API (of which FedCM is built on top of), but not in a way that requires an implementation to also implement FedCM.

This PR was only meant to be a sketch. As you note, it duplicates some bits from other specs where it was clearer to do so, rather than to refer to those documents. Ultimately, all these cases have to be fixed, but at the time we didn't want to distract from the core points. Putting retention as a top-level member was a mistake, one that we already realized some weeks ago, and were intending to address in a follow-up. It seemed like it might be a concept that spans multiple identity mechanisms, but we now feel that keeping mechanisms completely separate is probably best right now.

Speaking of multiple mechanisms, another goal that we have is to ensure that there is space in the API for requesting Verifiable Credentials. We are focused on building an mDL API, but the possibility that Verifiable Credentials will be an ecosystem necessity in the future is real, and we want to ensure that should they need to be supported, it would not damage the API too much.

So that's why it feels to us like we'll be reducing the risk of having a mess on our hands long term if we put this into navigator.credentials. But as mDL is rapidly gaining traction, we feel some urgency to pick a design so that we can work towards prototyping a first version in the coming months.

Does it seem reasonable if we open an issue in this repo, dedicated to this question, and ask other ecosystem members (including TAG) to weigh in?

othermaciej · 2023-04-11T16:02:35Z

@agl can you put more of what’s in your comment into the explainer draft in your PR, as well as addressing all the specific points of feedback of bits that are unclear or seem like outright errors? The intent of making a sketch is reasonable, but my understanding is that the Apple folks who originally proposed this spec have been asked to review and merge the PR, and I do not think it is mergeable in its current form.

implied requirements like rejecting any credential request for either a passkey or an mDL should be spelled out, I think. As should details like proposals to change things in FedCM, or to push bits of FedCM down to CM. It was impossible for me to tell which cases were things like that vs simply errors/oversights.

As for an interchangeable request for either an mdoc or a FedCM credential, I still do not understand even with your explanation how that is supposed to work. If it’s supposed to work with this PR and FedCM as it stands, then adding a worked example to the explainer would help. If it’s not supposed to work as-is and would require changes to FedCM proper, then sketching those changes would help. Moving this API to be on top of Credential Management has some clear downsides (more verbosity, makes it possible to express invalid requests) so the rationale for the proposed API shape needs to be understandable even to those who do not have the inside thinking of how FedCM is expected to evolve. So I think more of the context above needs to be in the Explainer and the path to joint use with FedCM needs to be spelled out with more specifics, ideally a code examaple.

I think that, plus fixing the technical errors highlighted in my specific line comments, is a reasonable request before a new round of review of this PR.

RByers · 2023-04-28T18:57:42Z

@othermaciej, it seems like the key design question to be answered (before spending time on a lot of these fine details) is whether unifying with CM is a good idea or not, right? We've taken your feedback here and updated the discussion in #2, shall we continue there?

I've also updated this PR to align and will respond to each comment individually, but perhaps we should see if we can get some consensus on #2 first before worrying too much about the details here?

snorp · 2023-05-23T19:07:28Z

README.md

+        nonce: "gf69kepV+m5tGxMyMF0cnn9NCnRHez/LUIsFtLi6pwg=",
+        documentType: "org.iso.18013.5.1.mDL",
+        retentionDays: 90,
+        readerPublicKey: ["ftl+VEHPB17r2oi6it3ENaqhOOB0AZbAkb5f4VlCPakpdNioc9QZ7X/6w..."],


This looks base64-encoded, but do we know any more than that? Is it a SEC 1 uncompressed EC key? Something else?

snorp · 2023-08-02T15:28:15Z

README.md

@@ -73,10 +140,12 @@ BrowserHandover = [
    pkEm
 ]
 ```
-where `Nonce` is the nonce provided by the request, `OriginInfoBytes` contains the origin of the request and is defined in ISO/IEC 18013-7, and `RequesterIdentity` is the requester identity object from the request.
+
+where `Nonce` is the nonce provided by the request, `OriginInfoBytes` contains the origin of the request and is defined in ISO/IEC 18013-7, and `RequesterIdentity` is the requester identity object from the request. (**TODO**: tighen the definition of `RequesterIdentity`, should it be the SubjectPublicKeyInfo?)


AFAICT RequesterIdentity isn't in the API anymore and should be removed from this handover array.

Update suggested API.

0609c97

This change updates the proposed API to use IdentityCredential instead.

bslassey mentioned this pull request Feb 10, 2023

Fit mdocs into the Credential Management API #3

Closed

davidz25 reviewed Feb 10, 2023

View reviewed changes

tomayac mentioned this pull request Mar 22, 2023

Syntax highlighting for IDL #4

Closed

othermaciej suggested changes Apr 7, 2023

View reviewed changes

bslassey mentioned this pull request Apr 24, 2023

Criticality #10

Open

Adam's work-in-progress prior to handoff to me

6d80fa1

RByers mentioned this pull request Apr 25, 2023

Consider lifting IdentityCredential out of FedCM spec fedidcg/FedCM#457

Open

RByers added 4 commits April 27, 2023 17:03

Add agl to authors

0d5d31c

Update to match latest IdentityCredential proposal

7498741

Move retention to a double per @othermaciej feedback

49961ad

Update proposed mdoc dictionary name

b68274e

RByers mentioned this pull request Apr 28, 2023

Consider incorporating into Credential Management API #2

Open

snorp reviewed May 23, 2023

View reviewed changes

snorp reviewed Aug 2, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update suggested API. #5

Update suggested API. #5

agl commented Feb 10, 2023

davidz25 left a comment

othermaciej Apr 7, 2023

othermaciej left a comment

othermaciej Apr 7, 2023

RByers Apr 28, 2023

othermaciej Apr 7, 2023

RByers Apr 28, 2023

othermaciej Apr 7, 2023

RByers Apr 28, 2023

othermaciej Apr 7, 2023

RByers Apr 28, 2023

othermaciej Apr 7, 2023

RByers Apr 28, 2023

othermaciej Apr 7, 2023

othermaciej Apr 7, 2023

RByers Apr 28, 2023

othermaciej Apr 7, 2023

RByers Apr 28, 2023

othermaciej Apr 7, 2023

RByers Apr 28, 2023

othermaciej Apr 7, 2023

othermaciej Apr 7, 2023

RByers Apr 28, 2023

bslassey commented Apr 7, 2023

agl commented Apr 10, 2023

othermaciej commented Apr 11, 2023 •

edited

RByers commented Apr 28, 2023

snorp May 23, 2023

snorp Aug 2, 2023

		partial dictionary CredentialRequestOptions {
		IdentityCredentialRequestOptions identity;

		// Omitting `retention` implies that the identity information will not be stored.
		IdentityStorageDuration retention;

Update suggested API. #5

Are you sure you want to change the base?

Update suggested API. #5

Conversation

agl commented Feb 10, 2023

davidz25 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

othermaciej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bslassey commented Apr 7, 2023

agl commented Apr 10, 2023

othermaciej commented Apr 11, 2023 • edited

RByers commented Apr 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

othermaciej commented Apr 11, 2023 •

edited