search

WICG / digital-credentials

Digital Credentials, like driver's licenses
https://wicg.github.io/digital-credentials/
Other
82 stars 9 forks source link

Add required mediation #149

Closed marcoscaceres closed 3 months ago

marcoscaceres commented 3 months ago

Closes #147

The following tasks have been completed:

Implementation commitment:

Documentation and checks

Preview | Diff

marcoscaceres commented 3 months ago

@bvandersloot-mozilla, right. I guess it depends on how you read the definition of "optional" from Cred Man:

optional: If credentials can be handed over for a given operation without user mediation, they will be. If user mediation is required, then the user agent will involve the user in the decision.

My reading from the above, and what I'm worried about, is that leaving it as "optional" invites optionality in presenting UI (which presuppose is always "required").

For completeness, here is how required is defined:

required: The user agent will not hand over credentials without user mediation, even if the prevent silent access flag is unset for an origin.

marcoscaceres commented 3 months ago

So, I guess we could do things differently... we could state in the spec that "mediation is always required" for this kind of credential, but we need to fix https://github.com/w3c/webappsec-credential-management/issues/248 (issue: user mediation is currently origin bound instead of type bound)... that could allow "optional", but enforce showing UI in prose and in Cred Man (possibly as part of the registry).

bvandersloot-mozilla commented 3 months ago

Yeah, that will work I think. It is just a little confusing because the mediation parameter prescribes the set of allowable behaviors for the manager and the credential types to take, rather than describing what will happen depending on state.

marcoscaceres commented 3 months ago

@samuelgoto, wouldn't mind your thoughts on this.

marcoscaceres commented 3 months ago

Ok, so, yeah... per step 8.1 of of Cred Man:

If options.mediation is conditional and interface does not support conditional user mediation, return a promise rejected with a "TypeError" DOMException.

So, we actually just need to state that mediation is required but not as part of the algorithm... will fix.

marcoscaceres commented 3 months ago

@TallTed 🦅 👀 🙏? ... 😊

marcoscaceres commented 3 months ago

For "required" and "optional"... I think I have a better solution for all this: https://github.com/w3c/webappsec-credential-management/issues/256

We need fix the Cred Man API to allow us to set our own default.

marcoscaceres commented 3 months ago

@samuelgoto, @bvandersloot-mozilla, here's a first stab at fixing this in Cred Man: https://github.com/w3c/webappsec-credential-management/pull/258

marcoscaceres commented 3 months ago

For those watching at home, this would mean that the following would not throw:

navigator.identity.get({digital: ...}); // this throws today, which is annoying. 
navigator.identity.get({digital: ..., mediation: "required"});

This will continue to throw:

navigator.identity.get({digital: ..., mediation: "conditional"});
navigator.identity.get({digital: ..., mediation: "silent"});
marcoscaceres commented 3 months ago

(also, "optional" mentioned by a few folks above is meant to be "conditional"... yay, naming things if fun 😜)

samuelgoto commented 3 months ago

(also, "optional" mentioned by a few folks above is meant to be "conditional"... yay, naming things if fun 😜)

I'm not sure I follow this reference, but I was actually discussion optional rather than conditional as:

https://w3c.github.io/webappsec-credential-management/#dom-credentialmediationrequirement-optional

marcoscaceres commented 3 months ago

Sorry @samuelgoto, I might have confused myself 🙈

samuelgoto commented 3 months ago

(added chromium bug for implementation)

samuelgoto commented 2 months ago

As we were implementing this PR here, something occurred to me while reviewing it:

(a) first, this is a backwards incompatible change for us: mediation is an optional field and it defaults to optional, so some developers have been using the default value (validly so). (b) second, is excluding optional really required? meaning, isn't the website leaving the choice to the user agent when they choose optional? meaning, wouldn't a valid implementation of optional be to threat it as required?

If this is correct, then can we make this more flexible and also allow the default value of optional to be used (and allow each implementation to implement it in whichever way it see fit)?

samuelgoto commented 1 month ago

As we were implementing this PR here, something occurred to me while reviewing it:

FWIW, we found a resolution to this here:

https://github.com/WICG/digital-credentials/issues/175#issuecomment-2397438963