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

Should FederatedCredential be FederatedCredential2? #237

Open
dj2 opened this issue Mar 30, 2022 · 4 comments
Open

Should FederatedCredential be FederatedCredential2? #237

dj2 opened this issue Mar 30, 2022 · 4 comments

Comments

@dj2
Copy link
Collaborator

dj2 commented Mar 30, 2022

Currently the Credential Management spec has a FederatedCredential object defined. The FedCM spec is extending that definition but also has different formats for things like the providers entry in the federated dictionary. This overriding and patching may make it more difficult if we wanted to deprecate FederatedCredential in the future and it makes the feature detection more complicated as we have to look for specific methods on the prototype of FederatedCredential.

Should we rename FederatedCredential to FederatedCredential2 and use something like federated2 as the entry into get to determine which type of credential is being provided? (get could also inspect the contents of the federated entries to determine which of the two types to create as the Credential Management FederatedCredential provides a sequence of strings for providers and FedCM provides a sequence of FederatedIdentityProvider objects.)

@johannhof
Copy link
Contributor

I had a conversation with @domenic and some thoughts on this issue (and why it might not be ideal after all) came out of that:

  • It might be reasonable to move our changes to FederatedCredential into Credential Management after incubation, so we could just merge the implementation there and avoid this split.
  • It's a bit unclear whether FederatedCredential2 would extend FederatedCredential (which I had assumed initially) or exist separately (which seems to be suggested in this issue). Either way it seems like we need a mechanism to avoid too much code duplication between the two (like a "super()" call in case of extension).
  • Even with most of it abstracted away by navigator.credentials.get, some of this spec-complexity would still be exposed to developers. It seems like we should try to minimize developer confusion.

@npm1
Copy link
Collaborator

npm1 commented Apr 21, 2022

  • It might be reasonable to move our changes to FederatedCredential into Credential Management after incubation, so we could just merge the implementation there and avoid this split.

@samuelgoto may have thoughts too but the FederatedCredential in that spec is being considered for deprecation, so not sure why we'd want to spec FedCM in that specification.

  • It's a bit unclear whether FederatedCredential2 would extend FederatedCredential (which I had assumed initially) or exist separately (which seems to be suggested in this issue). Either way it seems like we need a mechanism to avoid too much code duplication between the two (like a "super()" call in case of extension).

Well, the name does imply that but if the goal is to have a different interface so that FederatedCredential is easier to deprecate then it should not extend it.

@npm1
Copy link
Collaborator

npm1 commented Jul 20, 2022

We renamed to IdentityCredential. I think we can close this now.

@npm1 npm1 closed this as completed Jul 20, 2022
@timcappalli
Copy link

timcappalli commented Sep 6, 2022

Credential Management L1 is still a draft specification. Outside of WPT code and documentation, I found less than 10 public repositories on Github that are using FederatedCredential, and most were for Google's IdP. While this obviously isn't a complete picture of usage, it is a pretty good sampling.

IdentityCredential is too broad and does not accurately represent what FedCM is doing/trying to solve (a public key credential or password could be considered Identity Credentials).

We should consider having FedCM replace the existing FederatedCredential interface in CredMan.

If that is not feasible, I'd propose FederatedAssertion as the backup option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants