w3c / webappsec-credential-management

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

Update Conditional UI mediation #170

Closed nsatragno closed 3 years ago

nsatragno commented 3 years ago

this is a meta-pr on PR #155


Preview | Diff

nsatragno commented 3 years ago

@equalsJeffH please take a look!

nsatragno commented 3 years ago

Thanks @nsatragno, the below is nominally overall. A couple of fine points:

I presume we'll update the webauthn spec to include something like:

          partial dictionary ConditionalMediationAvailability {
            boolean public-key = true;
          }

...?

Yes, that's right.

Also, specifying having the navigator.credentials.get() promise not resolve is going to require further spec changes in the "Request a credential algorithm" step 7 because it presently always either resolves or rejects the promise. Here's an (offhand) idea for step 7.9 in "Request a Credential" (likely/perhaps needs refinement?):

9. If result is a Credential, resolve p with result.
   If result is null, and options.mediation is not "conditional", resolve p 
   with result.
   Otherwise, if options.mediation is not "conditional", reject p with result.
   Note: If options.mediation is "conditional" and we did not discover a 
   credential, we do not resolve the promise p.

Good catch! Done.

equalsJeffH commented 3 years ago

Also, specifying having the navigator.credentials.get() promise not resolve is going to require further spec changes in the "Request a credential algorithm" step 7 because it presently always either resolves or rejects the promise. [ ... ] Good catch! Done.

thanks. Tho, on my ride home yesterday it occurred to me that if "conditional mediation" is applied when requesting a credential type other than public-key, we would want the nav.creds.get() promise to also not resolve for those cred types (in the face of errors, as with public-key) ...? If so, then further surgery is required in the "Request a credential algorithm" step 7 (is work-in-progress).

nsatragno commented 3 years ago

If so, then further surgery is required in the "Request a credential algorithm" step 7 (is work-in-progress).

I'm not sure I get what you mean, I believe the change you suggested earlier and I applied covers all credential types.

nsatragno commented 3 years ago

Okay, I think it looks better now @equalsJeffH, ptal!

I changed (yet again! sorry!) how feature discovery works by adding a static method to the Credential interface. This means it's consistent with isuvpaa and you can ask for a specific credential type (i.e. PublicKeyCredential.supportsConditionalMediation()), and it's straightforward to overload on WebAuthn. I think that might satisfy everyone enough :P

nsatragno commented 3 years ago

Hi @nsatragno -- this is getting close, thanks! I have a few comments below inline plus those immediately below. Also, there'll be a couple of conflicts if we try to merge this directly into PR #155, perhaps we should first merge from PR #155 into this branch and fix the conflicts here?

I can make this mergeable into #155, merge it, and then continue there.

  1. in the "request a credential" alg, should step 7.2 be something like:

If |credentials| is an [=exception=] and |options|.{{CredentialRequestOptions/mediation}} is not "{{CredentialMediationRequirement/conditional}}", [=reject=] |p| with |credentials|.

..?

Ugh, this might be more complicated than I thought.

I want errors that are returned immediately to be visible to the website. This is to make using the API easier: if the developer tries to use the API in an unsupported fashion, an error should be returned to facilitate debugging. The errors we should hide come from the user picking no credential, picking a credential that doesn't work (e.g. because they entered the wrong PIN), or credentials not being available.

That's why I left this bit not guarded by the conditional mediation check, but it doesn't work because [[DiscoverFromExternalSource]] may also throw the kind of errors we don't want to eat up.

Perhaps the right move is to remove this check from credman and move it on a case-by-case basis to WebAuthn's [[DiscoverFromExternalSource]].

  1. We should probably update the "credential selection" section appropriately with aspects of "conditional UI" 's particulars, e.g., differentiate between modal and non-modal cred selection (i.e., mention that "required" implies modal and "conditional" implies non-modal" ?). We touch upon some of this in the explanation of what {{CredentialMediationRequirement/conditional}} means -- we ought to link those sections up somehow --- but perhaps what we have now is good enough for now and we can submit an issue that doing such is work still to be done?

My vote is to keep as-is, since the proposed UI technically aligns with the Credential Selection section.

nsatragno commented 3 years ago

I merged it cause it was a bit of a pain to maintain both changes synced.