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 #140

Closed
marcoscaceres opened this issue Jul 5, 2024 · 9 comments
Closed

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

marcoscaceres opened this issue Jul 5, 2024 · 9 comments

Comments

@marcoscaceres
Copy link
Collaborator

As it's still early, and given that we are generalizing this API beyond "identity", should we renamed it to navigator.digitalCredentials ?

@samuelgoto
Copy link
Collaborator

should we renamed it to navigator.digitalCredentials

I don't mind navigator.identity as a CredentialsContainer, but FWIW, I still don't see any concrete benefit we (or developers) get by not exposing it as a navigator.credentials instead. The DigitalCredential interface still largely behaves like a Credential, so I think the navigator.identity is unnecessary. I don't think it is harmful per se, so if folks feel like navigator.credentials is for "authentication", I don't feel like that's necessarily a hill worth dying on (although, I'm pretty confident that developers will, at some point, use DigitalCredentials for login). FWIW, navigator.credentials.get({digital: ...}) wouldn't have identity in it.

Separately, I don't feel like navigator.digitalCredentials is necessary (much like we don't need a navigator.publicKeyCredentials rather than navigator.credentials.get({publicKey: ....}) or a navigator.otpCredentials or a navigator.federatedCredentials), but I do feel that navigator.identity is unnecessary (but non-harmful).

All in all, my intuition is that we have found a shared compromise on navigator.identity that's worth keeping while we are incubating this API and learning how it is going to be used. Whatever we pick is very likely to be incorrect.

@marcoscaceres
Copy link
Collaborator Author

marcoscaceres commented Jul 10, 2024

The problem was that navigator.credentials is currently specified to allow selection of multiple credentials at the same time (which is something that Cred Man allows, but no one implements afaik).

To be clear, this is the ol' problem:

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

If we can get Cred Man to disallow multiple request types, then we could use navigator.credentials... and this would solve a lot of problems everywhere, IMO!

I think we need to talk to @nsatragno about the feasibility (and backwards compatibility) of changing Cred Man to restrict to a single request type.

We could support that in WebKit for sure (we only support .publicKey right now... and we could expand support to .digital, and throw if multiple are provided...)

@marcoscaceres marcoscaceres changed the title navigator.identity could be renamed to 🥁🥁🥁 navigator.digitalCredentials Disallow multiple types via navigator.credentials's methods Jul 10, 2024
@samuelgoto
Copy link
Collaborator

The problem was that navigator.credentials is currently specified to allow selection of multiple credentials at the same time

Isn't that a problem we inherited by reusing the CredentialsContainer class in the navigator.identity object?

FWIW, the main reason I think navigator.identity makes sense to me is such that it leaves the door open to requesting multiple credentials types in the future, so that there is a path for two-party federations to participate too.

which is something that Cred Man allows, but no one implements afaik
We could support that in WebKit for sure
changing Cred Man to restrict to a single request type.

Yeah, we did check with @nsatragno that making the Credential Management API that throw an error in this case is viable: as you said, it just documents the implementation.

I think that would be feasible in Chrome too.

If we can get Cred Man to disallow multiple request types, then we could use navigator.credentials... and this would solve a lot of problems everywhere, IMO!

IMHO, that's my preferred route too. I can live with navigator.identity, but my personal preference is to expose it as a navigator.credentials credential: I don't personally see a lot of a benefit in the extra object.

If you and others wanted to go this route, I'd be fairly supportive of investigating further (but just to reaffirm: navigator.identity is perfectly fine too).

@bvandersloot-mozilla
Copy link

If we can get Cred Man to disallow multiple request types, then we could use navigator.credentials... and this would solve a lot of problems everywhere, IMO!

The tradeoff here is that it prevents (the purely theoretical so far) use by a site to have the user select between a passkey, federation, or a password manager entry in the same dialog. I like this as a possibility for the user because it enables a much easier "login" button that just gives them the options that they have used before in a browser dialog- they don't need to make up their mind about how to log in without that context.

@samuelgoto
Copy link
Collaborator

samuelgoto commented Jul 11, 2024

The tradeoff here is that it prevents (the purely theoretical so far) use by a site to have the user select between a passkey, federation, or a password manager entry in the same dialog.

But we wouldn't be cornering ourselves from this (purely theoretical, as you noted) future, would we? That is, if the CM API threw an NotSupportedError now when multiple credentials are requested, we would be able to "not throw" in the future in backwards compatible way, right?

@bvandersloot-mozilla
Copy link

Maybe I misunderstood marcos' proposal: I thought he was saying we would have an example request be navigator.credentials.publicKey.get or navigator.credentials.digital.get, preventing their combination ever.

but if he was using jq syntax in .publicKey rather than abbreviation, then I did misunderstand.

I still have my reservations in throwing digital as a new key under the options dict.

@samuelgoto
Copy link
Collaborator

samuelgoto commented Jul 11, 2024

I thought he was saying we would have an example request be navigator.credentials.publicKey.get or navigator.credentials.digital.get

Oh yeah, so then, maybe I'm the one that is misunderstanding @marcoscaceres 's proposal then :)

Just to try to paraphrase @marcoscaceres on this line:

If we can get Cred Man to disallow multiple request types, then we could use navigator.credentials... and this would solve a lot of problems everywhere, IMO!

I interpreted that as the following:

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

Which I'm supportive of, because it would mean we could move the API out of navigator.identity into navigator.credentials:

// returns a DigitalCredential object
await navigator.credentials.get({
  digital: ...,
});

Does that make sense?

@marcoscaceres
Copy link
Collaborator Author

@bvandersloot-mozilla, what @samuelgoto wrote above is my proposal. Sorry for not being clear.

@marcoscaceres
Copy link
Collaborator Author

Moved the discussion to w3c/webappsec-credential-management#244 ... pinged folks above there.

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

3 participants