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

Initial text for conditional create #1951

Merged
merged 19 commits into from
May 15, 2024
Merged

Conversation

pascoej
Copy link
Contributor

@pascoej pascoej commented Aug 29, 2023

Fixes #1929


Preview | Diff

@pascoej pascoej marked this pull request as ready for review August 30, 2023 14:28
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated
### Conditional Create Extension (<dfn>conditionalCreate</dfn>) ### {#sctn-authenticator-conditional-create-extension}

This [=client extension|client=] [=authentication extension=] indicates that the RP would like to create a credential after an authenticaton ceremony is successfully mediated by the user agent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further up this change says:

prominent modal UI should not be shown unless the user has already consented to create a credential via the [=conditionalCreate|conditionalCreate extension=])

That suggests that a conditional create() shows a modal UI unless the user refused consent to create a credential during the previous conditional get(). It sounds like it's a way to avoid bothering the user with a modal creation UI if they didn't "opt-in" to being bothered during autofill. (Which is different from "the create() can resolve silently if the user consented previously".)

While user agents can ultimately implement any UI, how do you want sites thinking about how to use this extension?

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@MasterKale
Copy link
Contributor

Would anything in this proposal potentially blow up if we ended up in a future state where passkey providers offered users an option, entirely outside of the browser, to not silently register passkeys? That is, we'd have something like this:

  • User turns off "allow sites to silently create passkeys" in their passkey provider

[A few hours later]

  • RP sets up conditional creation with conditionalCreate extension
  • User logs in with username + password
  • Browser attempts to silently register a passkey
  • Passkey provider says "nope"
  • ???

Just trying to think ahead a bit, about potential controls that passkey providers might offer users (outside the influence of ostensibly user-centric WebAuthn) that would potentially clash with RP-friendly features like this one.

Copy link
Member

@nsatragno nsatragno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really excited for this! I'll summarize my thoughts I shared in the w3c call:

  • As it's written, making a credential through conditional registration requires opting in to a conditional get assertion. We should relax this requirement: it should be acceptable for an RP to offer conditional registration after the user opts in (e.g., after they chose a previously saved password in a sign in form) without also requiring them to offer previously registered passkeys on the same form. An example use case would be to create a passkey from a password field during registration (see WebAuthn Autofill (Conditional UI) for credential registration #1862). I understand you are using the get assertion extension to collect consent, but I don't see why consent needs to be tied in to a get assertion call.
    If we go this route, perhaps we can do away with the assertion extension, and instead have a create request with mediation: conditional that handles obtaining consent. When it resolves, it could either run a callback or return a promise with a method to fill in the username, challenge, etc.

  • CredentialCreationOptions needs to be updated to have a mediation parameter.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@pascoej
Copy link
Contributor Author

pascoej commented Sep 11, 2023

I still need to address the non-requirement of the extension and sort out a procedural issue in order to submit a PR against credential-management.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@MasterKale
Copy link
Contributor

MasterKale commented Sep 25, 2023

The thought occurred to me over the weekend: how can the browser know an auth is successful if A) the RP cannot use the conditionalCreate extension, and B) the user doesn't use the browser autofill to enter username + password (because we're assuming the user can't use WebAuthn hence the desire to conditionally register a credential, and maybe they use a browser extension for password autofill that suppresses the browser's default autofill.) If conditionalCreateLifetimeTimer can be started by the browser without the use of conditionalCreate, it's unclear to me right now what heuristics a client might use to know when to start the timer.

@plehegar plehegar modified the milestones: L3-WD-01, L3-WD-02 Oct 4, 2023
@pascoej
Copy link
Contributor Author

pascoej commented Dec 20, 2023

Associated credential management PR: w3c/webappsec-credential-management#224

@nadalin nadalin requested a review from ve7jtb January 10, 2024 20:21
@nsatragno
Copy link
Member

I asked for a few changes on the CredMan PR, but I don't think they will affect the integration on webauthn -- thanks!

My question about the extension requirement still stands. We can gain a lot from decoupling conditional creation from conditional assertion at the spec level.

@pascoej pascoej requested review from nsatragno and agl February 7, 2024 19:29
@pascoej
Copy link
Contributor Author

pascoej commented Feb 7, 2024

I've addressed comments and added more clarification that the extension is not required.

Copy link
Member

@timcappalli timcappalli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small change from 2024-04-03 call

index.bs Outdated
[=[RPS]=] can indicate that they would like to register a credential without prominent modal UI if user has already consented to create a credential. The [=[RP]=] SHOULD first check that {{ClientCapability/conditionalCreate}} is present
in the result of {{PublicKeyCredential/getClientCapabilities()}} in order to avoid the possibility of causing a user-visible error to be returned if the user agent does
not support {{CredentialMediationRequirement/conditional}} [=user mediation=] for {{CredentialsContainer/create()|navigator.credentials.create()}}.
The authenticator SHOULD set BOTH |userPresence| and |userVerification| to |FALSE| when <code>|options|.{{CredentialCreationOptions/mediation}}</code> is set to {{CredentialMediationRequirement/conditional}}
Copy link
Member

@timcappalli timcappalli Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/SHOULD/MUST

@timcappalli
Copy link
Member

The verification step needs to be updated as well to add an if mediation=conditional check.

1. Verify that the [=UP=] bit of the <code>[=flags=]</code> in |authData| is set.

webauthn/index.bs

Line 5372 in 89f204f

1. Verify that the [=UP=] bit of the <code>[=flags=]</code> in |authData| is set.

index.bs Outdated
Comment on lines 1747 to 1748
The authenticator SHOULD set BOTH |userPresence| and |userVerification| to |FALSE| when <code>|options|.{{CredentialCreationOptions/mediation}}</code> is set to {{CredentialMediationRequirement/conditional}}
unless explicitly collected during the ceremony.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section is the client operation, but this is a requirement on the authenticator. This needs to go in section 6 (WebAuthn Authenticator Model) instead, or be rewritten in terms of how the client is to set the arguments to the authenticator operation.

index.bs Outdated
Comment on lines 2028 to 2035
:: If <code>|options|.{{CredentialCreationOptions/mediation}}</code> is set to
<dl class="switch">
: {{CredentialMediationRequirement/conditional}}
:: throw a {{NotAllowedError}} {{DOMException}} unless it can be explicitly collected during the ceremony.

: empty or another value
:: Let |userVerification| be [TRUE].
</dl>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flow here is a bit convoluted, I would formulate it something more like this:

Suggested change
:: If <code>|options|.{{CredentialCreationOptions/mediation}}</code> is set to
<dl class="switch">
: {{CredentialMediationRequirement/conditional}}
:: throw a {{NotAllowedError}} {{DOMException}} unless it can be explicitly collected during the ceremony.
: empty or another value
:: Let |userVerification| be [TRUE].
</dl>
:: 1. If <code>|options|.{{CredentialCreationOptions/mediation}}</code> is set to {{CredentialMediationRequirement/conditional}}
and [=user verification=] cannot be collected during the ceremony,
throw a {{NotAllowedError}} {{DOMException}}.
1. Let |userVerification| be [TRUE].

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, is NotAllowedError the right behaviour here? The closest analogue currently in the spec is the "error code equivalent to ConstraintError" returned by the authenticator when UV is required but cannot be satisfied by that authenticator. Returning an error at all also differs from conditional mediation in get(), which simply hangs forever on most errors. Should conditional create do that too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea with conditional registration is that you invoke it right after logging in and get a result quickly. If the user didn't consent to this type of create, then they will quickly get an error, otherwise a credential.

I think ConstraintError does sound okay in this case. I've made the change.

index.bs Outdated Show resolved Hide resolved
Copy link
Member

@timcappalli timcappalli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines +2028 to +2030
:: 1. If <code>|options|.{{CredentialCreationOptions/mediation}}</code> is set to {{CredentialMediationRequirement/conditional}}
and [=user verification=] cannot be collected during the ceremony,
throw a {{ConstraintError}} {{DOMException}}.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: this new case for ConstraintError will need to get documented in the same Registration API Exceptions being proposed in #2047.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@pascoej pascoej requested a review from MasterKale May 15, 2024 18:44
@pascoej pascoej dismissed MasterKale’s stale review May 15, 2024 19:27

Can be addressed in Emlun's followup.

@pascoej pascoej merged commit 62b069e into w3c:main May 15, 2024
2 checks passed
@pascoej pascoej deleted the conditionalcreate branch May 15, 2024 19:29
github-actions bot added a commit that referenced this pull request May 15, 2024
SHA: 62b069e
Reason: push, by pascoej

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to selfissued/webauthn that referenced this pull request May 20, 2024
SHA: 62b069e
Reason: push, by selfissued

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non-modal registration during conditional assertion