search

w3c-fedid / FedCM

A privacy preserving identity exchange Web API
https://w3c-fedid.github.io/FedCM/
Other
375 stars 72 forks source link

Add Identity Provider branding name as an optional field #432

Closed bvandersloot-mozilla closed 1 year ago

bvandersloot-mozilla commented 1 year ago

This adds one optional field to the IDP Branding information to provide a name the browser can show to represent the IDP. This shouldn't replace the IDP Site or Origin, because of security properties, but can be useful for UI elements.

Preview | Diff

npm1 commented 1 year ago

This could be useful... but as you mention, the user agent is already forced to show the IDP site/origin. Is this field required in the UI Firefox has been crafting? I'm not sure if Chrome would use this just because of the attacker threat.

samuelgoto commented 1 year ago

This could be useful...

+1. This seems like it would be useful to me too, but ...

but as you mention, the user agent is already forced to show the IDP site/origin.

... right, chrome's implementation strongly needs the site/origin to be shown (i.e. we could use name "in addition to" url but not "in replacement of" ).

Is this field required in the UI Firefox has been crafting? I'm not sure if Chrome would use this just because of the attacker threat.

Are you actively intending to use name? If so, adding this property here seems fine, but if you aren't (and neither is chrome), then we'd want to avoid writing something that no implementer is planning to use.

bvandersloot-mozilla commented 1 year ago

but as you mention, the user agent is already forced to show the IDP site/origin.

... right, chrome's implementation strongly needs the site/origin to be shown (i.e. we could use name "in addition to" url but not "in replacement of" ).

Strong agree. Firefox has the same constraint.

Is this field required in the UI Firefox has been crafting? I'm not sure if Chrome would use this just because of the attacker threat.

Are you actively intending to use name? If so, adding this property here seems fine, but if you aren't (and neither is chrome), then we'd want to avoid writing something that no implementer is planning to use.

There are places we would like to include name, assuming it isn't something that Google thinks is too dangerous to include.

samuelgoto commented 1 year ago

assuming it isn't something that Google thinks is too dangerous to include.

We can ask around to confirm, but I doubt that we'd arrive at a different position: seems safe (maybe comparable to <title>: content-provided string that is used in conjunction with the origin). If this is something that Firefox is planning to use, I think we should merge (and even if Chrome doesn't at first, I think we would at some point).

@npm1 wdyt?

npm1 commented 1 year ago

Sounds good to me. It could be used later on by Chrome as well. Can you rebase?

bvandersloot-mozilla commented 1 year ago

Rebase'D!

yi-gu commented 1 year ago

Just want to confirm that the feedback we got from Chrome stakeholders matches the analysis in this issue. i.e. it's OK to show IDP names but the domain must be displayed as well AND it has to be as prominent as the IDP name.

samuelgoto commented 1 year ago

Just want to confirm that the feedback we got from Chrome stakeholders matches the analysis in this issue. i.e. it's OK to show IDP names but the domain must be displayed as well AND it has to be as prominent as the IDP name.

Neat, thanks for confirming!