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

fix #658: add user cancelled operation Note #760

Merged
merged 4 commits into from Feb 6, 2018

Conversation

equalsJeffH
Copy link
Contributor

@equalsJeffH equalsJeffH commented Jan 24, 2018

fix #658


Preview | Diff

@emlun
Copy link
Member

emlun commented Jan 24, 2018

This might be slightly unrelated to this particular PR, but should these steps also make the client stop accepting new authenticators (but not terminate the timer) in step 19/17?

What I mean is, say an operation is initiated with two authenticators connected. The user cancels the operation on one of them, and the client then cancels the operation on the other authenticator as well. Say the user then plugs in a third authenticator, perhaps for a completely unrelated purpose. As currently specified (likely a relic from before hot plugging), the client would initiate the operation anew with this authenticator, which might surprise the user since they just canceled it. It could in some cases lead to the user inadvertently accepting the Webauthn operation instead of the unrelated task they plugged in the authenticator for.

What do you think?

@equalsJeffH
Copy link
Contributor Author

equalsJeffH commented Jan 24, 2018

wrt #760 (comment), sounds like a plausible situtaion, WDYT @jcjones @akshayku @leshi ?

@kpaulh
Copy link
Contributor

kpaulh commented Jan 24, 2018

The situation does sound plausible, and I agree with @emlun that this could be surprising to the user. Since the intent of the spec seems to be that canceling an operation on one authenticator cancels the entire operation across all authenticators, is there a reason we can't just terminate the timer?

@emlun
Copy link
Member

emlun commented Jan 24, 2018

is there a reason we can't just terminate the timer?

Yes, there is: See getAssertion step 19 and §14 Privacy Considerations 2. Authentication Ceremonies.

@kpaulh
Copy link
Contributor

kpaulh commented Jan 24, 2018

Right-o, thanks for reminding me. In that case, denying new authenticators seems like a reasonable solution.

Copy link
Contributor

@selfissued selfissued left a comment

Choose a reason for hiding this comment

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

I suspect that transport-specific is right and platform-specific is wrong.

index.bs Outdated
@@ -908,6 +908,9 @@ When this method is invoked, the user agent MUST execute the following algorithm
1. [=set/For each=] remaining |authenticator| in |issuedRequests| invoke the [=authenticatorCancel=] operation on
|authenticator| and [=set/remove=] it from |issuedRequests|.

Note: An [=authenticator=] may return some indication of "the user cancelled the entire operation".
Conveyance of such an indication is client platform-specific.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you mean transport-specific - not platform-specific.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hrmm; I think Jeff means it's up to the user agent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whether such a "user cancelled entire operation" indication is conveyed successfully to the RP client-side JS, is specific, as I understand it, to the entire userAgent+OSplatform+transport+authenticator stack. That's what "client platform-specific" is attempting to convey.

fyi/fwiw, "client platform" (and synonyms thereof) are among our currently undefined terms. So making this stuff more clear may ultimately involve getting around to defining at least that term, seems to me.

index.bs Outdated
@@ -1260,6 +1263,9 @@ When this method is invoked, the user agent MUST execute the following algorithm
1. [=set/For each=] remaining |authenticator| in |issuedRequests| invoke the [=authenticatorCancel=] operation
on |authenticator| and [=set/remove=] it from |issuedRequests|.

Note: An [=authenticator=] may return some indication of "the user cancelled the entire operation".
Conveyance of such an indication is client platform-specific.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah... maybe it could just be "It is unspecified how a user agent should convey this state to the user" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, how this state is annunciated is another aspect of this.

@equalsJeffH
Copy link
Contributor Author

@selfissued @jcjones -- pls re-review, thx for the wording suggesting @jcjones.

@nadalin
Copy link
Contributor

nadalin commented Feb 5, 2018

@selfissued Please re-review the changes

Copy link
Contributor

@selfissued selfissued left a comment

Choose a reason for hiding this comment

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

Looks ready to merge to me now

@equalsJeffH equalsJeffH merged commit f13e030 into master Feb 6, 2018
@emlun emlun deleted the jeffh-fix-658-user-cancelled-op branch June 12, 2019 11:27
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.

CTAP/U2F doesn't status indicating the user cancelled the operation
6 participants