search

w3c / webappsec-credential-management

WebAppSec Credential Management
https://w3c.github.io/webappsec-credential-management/
Other
49 stars 36 forks source link

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

Open marcoscaceres opened 2 months ago

marcoscaceres commented 2 months ago

Moving discussion from https://github.com/WICG/digital-credentials/issues/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 commented 1 month ago

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

nsatragno commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

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 commented 3 weeks ago

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:

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

marcoscaceres commented 3 weeks ago

Oh, and I think I found another issue: https://github.com/w3c/webappsec-credential-management/issues/256

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

samuelgoto commented 3 weeks ago

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 commented 3 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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:

marcoscaceres commented 1 week ago

We met and discussed: