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

Disallow multiple types via navigator.credentials's methods #244

Open
marcoscaceres opened this issue Jul 15, 2024 · 19 comments
Open

Disallow multiple types via navigator.credentials's methods #244

marcoscaceres opened this issue Jul 15, 2024 · 19 comments

Comments

@marcoscaceres
Copy link
Member

marcoscaceres commented Jul 15, 2024

Moving discussion from WICG/digital-credentials#140 to here....

As spec'ed, the current API allows calling the methods on CredentialsContainer with multiple request types. However, in practice, this doesn't quite work because some of the methods show quite complicated UIs. Additionally, as this capability is not something that's been implemented by anyone (AFAIK), we should consider not allowing that.

Thus, the proposal is to check if more than one credential request (and creation?) option has been passed, and if so, throw a NotAllowedError.

To be clear:

// Throws a NotSupportedError
await navigator.credentials.get({
  digital: ...,
  publicKey: ...,
  federated: ...,
});

cc @bvandersloot-mozilla, @samuelgoto, @nsatragno

@marcoscaceres
Copy link
Member Author

I'll note that .create() already forbids the above (it's restricted to 1 know type).

@nsatragno
Copy link
Member

As spec'ed, the current API allows calling the methods on CredentialsContainer with multiple request types. However, in practice, this doesn't quite work because some of the methods show quite complicated UIs. Additionally, as this capability is not something that's been implemented by anyone (AFAIK), we should consider not allowing that.

PasswordCredential & FederatedCredential types are both accepted by .get() on Chrome, and so are FederatedCredential and IdentityCredential.

We (meaning, the broader web authentication & identity teams at Chrome) want multiple credential types on the same request to work more broadly eventually. We designed FedCM and WebAuthn as part of CredMan with the aspirations that one day we'll unify the UI in an integrated sign-in experience.

It's true this is not the case right now for every combination of providers. However, I would prefer not to restrict this on the specification.

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Jul 29, 2024

PasswordCredential & FederatedCredential types are both accepted by .get() on Chrome, and so are FederatedCredential and IdentityCredential.

Ok, I see... it does a joint UI as per this article.

We (meaning, the broader web authentication & identity teams at Chrome) want multiple credential types on the same request to work more broadly eventually. We designed FedCM and WebAuthn as part of CredMan with the aspirations that one day we'll unify the UI in an integrated sign-in experience

It's true this is not the case right now for every combination of providers. However, I would prefer not to restrict this on the specification.

Right, there may definitely valid combinations, but it's also the case where we end up with invalid combinations (e.g., say, Web Authn and Digital Credentials and Passwords).

Would it make sense to only allow individual and well-established/standardized combinations? (e.g., {password: {}, federated: {})? That would allow us to reject non-sensical / problematic combinations and not the leave the spec open ended.

@nsatragno
Copy link
Member

The only combination that immediately strikes me as invalid is digital credentials and anything else, due to the very flexible nature of digital credential assertions, potentially allowing for very strong identification. I thought isolating access to digital credentials under the navigator.identity instance of CredentialsContainer was the way we'd establish which combinations are valid at the standard level, no?

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Aug 2, 2024

You are correct about isolating access to digital credentials under the navigator.identity instance of CredentialsContainer... But on reflection, having the separate seems unnecessary (barring this restriction and what's implemented in practice).

If we consolidate everything in navigator.credentials, it makes implementation much cleaner and there is potentially less overhead for developers, as they will have a single entry point for all credential types.

Also, we kinda don't want to use "identity" anymore, as the API is now generally about "digital credentials" (took us a couple of false starts to get here... naming things is hard).

@nsatragno
Copy link
Member

nsatragno commented Aug 2, 2024

Okay, consolidating everything under navigator.credentials sounds good. And I think it's reasonable to have the standard reflect the real world.

What if the spec is updated so that password and federated are allowed to be combined, but disallowing the other combinations, with a note stating that more combinations might be available in the future? That's very close to your original proposal above, I think.

@bvandersloot-mozilla
Copy link

I still really am not sure about making DigitalCredentials available on the same API surface as the rest of the Credential types, particularly because of the differences Nina pointed out here. Putting distance between auth and digital credentials for developers is a positive IMO.

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Aug 5, 2024

Right, but the "distance" is cosmetic. Implementation-wise (and yes, priority of constituents comes into play), it's making things messy having the artificial distance just be navigator.identity VS navigators.credentials. That's not a lot of distance, if we are being honest because they are both routing through essentially the same CredentialContainer implementation in C++.

I'm also thinking about this from a Web Developer's perspective: having a single entry point for getting to credentials doesn't strike me as negative.

because of the differences Nina pointed out here.

Note that I'm in agreement with Nina also about differences. But irrespective of Digital Credentials, leaving the combinations open ended is not good practice from in interop perspective. It's also not how things have played out in practice with how engines have implemented Cred Man.

I'm ok to sit on this for a bit and we can discuss it a bit more. But as I'm implemented in more and more of the spec in WebKit (and seeing the overlap with Web Auth), I'm starting the feel more and more we should really have a single entry point (navigator.credentials) for everything.

@samuelgoto, can you share your experience from the Chromium side?

@marcoscaceres
Copy link
Member Author

What if the spec is updated so that password and federated are allowed to be combined, but disallowing the other combinations, with a note stating that more combinations might be available in the future? That's very close to your original proposal above, I think.

That would work for me, yes.

@samuelgoto
Copy link

@samuelgoto, can you share your experience from the Chromium side?

I tend to agree with your suggestion @marcoscaceres but that's not a hill I'm willing to die on: I agree that the distinction between navigator.identity and navigator.credentials is artificial and will cause more problems than clarity, but having it allows us to move on and make forward progress without cornering ourselves.

Based on what I'm hearing from developers, I'm convinced that DigitalCredentials are going to be used for "Sign-up and Sign-in" (I'm deliberately trying to avoid the term "auth" because I think it means different things to different people), within the same site (e.g. issued and verified by the same site) and across sites (e.g. issued by one site, verified by third party sites), and that at some point, we'll have to reconcile and unify DigitalCredentials with PublicKeyCredential and IdentityCredential under the same call in the CredentialsContainer.

I don't mind crossing that bridge when we get there, so don't mind exposing navigator.identity.get() now and revisiting as we learn how these different credential types interact with one another.

So, to sum up, my personal preference is to use navigator.credentials.get({digital: {...}}), but I can see why it is also compelling to use navigator.identity.credentials.get({digital: {...}}).

@marcoscaceres
Copy link
Member Author

I'm convinced that DigitalCredentials are going to be used for "Sign-up and Sign-in"

That would be not great. We should say not to do that and provide guidance in the appropriate place (outside the scope of this discussion though). But it does frame some motivating factor here.

I still think we need to take a step back here, as this is larger than all the aforementioned specs. The fundamental issue that remains unaddressed is:

navigator.credentials.get({ password: ..., publicKey: ..., etc. });

And

navigator.identity.get({ digital: ..., futureThing: ..., etc. });

What happens leads to all sorts of custom handling for each entry point and a bunch of undefined behavior. For example, in WebKit, I'm already having to deal with the interplay of these (which is not in the spec):

https://github.com/WebKit/WebKit/blob/51781e32c383b3de02ab11c1e274249a471e73f0/Source/WebCore/Modules/identity/IdentityCredentialsContainer.cpp#L54-L57

Same here in Credential Container:

https://github.com/WebKit/WebKit/blob/51781e32c383b3de02ab11c1e274249a471e73f0/Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp#L116-L126

if (options.publicKey && options.digital) {
    promise.reject(Exception { ExceptionCode::NotSupportedError, "Only one request type is supported at a time."_s });
    return false;
}

So you can see we are already having code those checks into the implementation. There's potential interop pitfalls here which I we should deal with.

Also, take a look at Gecko's code for CredentialsContainer::Get:
https://github.com/mozilla/gecko-dev/blob/a85b25946f7f8eebf466bd7ad821b82b68a9231f/dom/credentialmanagement/CredentialsContainer.cpp#L157

Gecko checks the aOptions.mIdentity and aOptions.mPublicKey a bunch of times and there seems to be a bunch of interplay there. How can I be sure (from WebKit's perspective) that our behavior is interoperable?

What does Chrome do here?

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Aug 23, 2024

Oh! in Gecko, .publicKey, takes precedence over .identity. What if WebKit decided that .identity should win? Or, as we do right now, we throw.

Anyway, I hope you see my point now. This is not good and could be quite confusing from a developer (and implementer) perspective:

  • In Gecko, you basically have a fallback chain: try Web Authn, otherwise try FedID.
  • In WebKit: only one at a time, or throw.
  • In Chrome: ???

We should fix this. It's easy to fix and it would obliterate the need for .identity. in the process.

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Aug 23, 2024

Oh, and I think I found another issue:
#256

What happens if multiple credential types are allowed per request, but they have conflicting mediation requirements?

@samuelgoto
Copy link

What happens if multiple credential types are allowed per request, but they have conflicting mediation requirements?

The first thing that occurs to me is that we should start by throwing NotSupportedError when multiple credential types are requested, and if/when we figure out when we can make two credential types to be requested (e.g. they support non conflicting mediation requirements) we would manually allow list that pair as an allowed combination.

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Aug 24, 2024

Right, which is what WebKit already does (but, for instance, Gecko doesn't).

The ask here is that we standardize the behavior, as it's not specified. Otherwise we will end up with subtly different behavior (we evidently already have, as I showed above). That's a problem, IMO. And the longer we leave it, the more deviation there will be and we risk ending up with the usual site compat issues and developer relying on one browser's non-standard behavior.

Let's nip this in the bud now before we see wide adoption of DC, FedCM, and whatever future API might come to rely on Cred Man. They will have this exact same problem too, exponentially so if we stick to delineating things on .identity. and .whatever. as each will need to deal potential mix-and-match scenarios.

@samuelgoto
Copy link

The ask here is that we standardize the behavior, as it's not specified.

LGTM

What's the next step here? Would a (backwards compatible) spec PR to the CredMan API suffice to throw NotSupportedError when certain combinations are asked?

@marcoscaceres
Copy link
Member Author

Discussed with @samuelgoto. We could add a column to registry that says something like, can be used with "other-credential"(s).

We noted that Gecko makes Web Authn win (and other things get ignored).

@samuelgoto
Copy link

A few questions that occurred to @marcoscaceres and I as we were chatting about this as we were trying to figure out what algorithm to suggest in the spec:

  • If you ask for FederatedCredential and PasswordCredential at the same time, what happens? It seems like in Chrome that would be allowed, and the browser would know how to build that UI. Since Safari and Firefox don't support either, seems like a no-problem for them.
  • What happens when IdentityCredential and PublicKeyCredential are asked at the same time in the get()? My intuition is that PublicKeyCredential would win and IdentityCredential would be ignored. Gecko does implement both, so we'd have to check there too. Our intuition is that this should throw NotSupportedError as opposed to having PublicKeyCredential win.
  • Can OTPCredential be asked with any other one? Safari/Firefox don't implement OTPCredential, so that's also likely just a Chrome concern.

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Sep 4, 2024

We met and discussed:

  • adding a column to the registry for what types can be mixed (currently only Federated and Password)
  • getting an error to be thrown when there's an incompatible mix (NotSupportedError)
  • the possibility of checking if mixing type A and type B can used used together
  • And potentially then getting rid of .identity. namespace for Digital Credentials

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

No branches or pull requests

4 participants