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

authenticatorCancel seems like it can cancel too much #283

Closed
bzbarsky opened this issue Nov 4, 2016 · 11 comments
Closed

authenticatorCancel seems like it can cancel too much #283

bzbarsky opened this issue Nov 4, 2016 · 11 comments

Comments

@bzbarsky
Copy link

bzbarsky commented Nov 4, 2016

Say a client has makeCredential called. It kicks off a bunch of authenticatorMakeCredential operations and waits.

Now before those have finished there is a getAssertion call with a very short timeout. The client kicks off some authenticatorGetAssertion operations and waits. The timeout is reached. The client is expected to do an authenticatorCancel on all the authenticators, which will cancel those still-pending authenticatorMakeCredential calls. That seems pretty odd to me.

@equalsJeffH equalsJeffH added this to the WD-04 milestone Nov 7, 2016
@vijaybh vijaybh modified the milestones: WD-04, WD-05 Feb 16, 2017
@selfissued
Copy link
Contributor

Is there a specific change or clarification to the semantics of these operations being proposed?

@bzbarsky
Copy link
Author

bzbarsky commented Apr 5, 2017

Well, the specific change that would make sense to me is that a getAssertion call that times out should not cancel anything to do with completely unrelated makeCredential calls, no?

@selfissued
Copy link
Contributor

@bzbarsky 's suggestion sounds reasonable to me.

@equalsJeffH
Copy link
Contributor

@mikewest notes that this is related to #380

@bzbarsky
Copy link
Author

bzbarsky commented Apr 5, 2017

Similarly, should a getAssertion with a short timeout be able to cancel stuff from a different getAssertion call?

Basically, what's the actual model here?

@kpaulh
Copy link
Contributor

kpaulh commented Apr 6, 2017

I'm not sure this is a problem, given the way the spec is written. The spec specifies that authenticatorCancel should only apply to operations within a specific authenticator session. The timeout algorithm also further scopes cancellation to authenticators that are present in the issuedRequests list, which should only be authenticators processing the current getAssertion or makeCredential call. That sounds like it already specifies the behavior that @bzbarsky suggested?

@bzbarsky
Copy link
Author

bzbarsky commented Apr 6, 2017

I'm not sure what "current getAssertion call" means. These are async operations; multiple getAssertion calls can be in flight at once, right?

@kpaulh
Copy link
Contributor

kpaulh commented Apr 6, 2017

I was using 'current' to mean the call whose timeout was reached. Not quite sure how better to refer to that..

Right, there can be multiple getAssertion calls, and each getAssertion call has a timeout and spawns multiple authenticatorGetAssertion calls.

Put another way, I've been interpreting the spec to say that each authenticatorGetAssertion call to each authenticator from a given getAssertion call is within an authenticator session that is distinct from the authenticatorGetAssertion calls stemming from a different getAssertion call. And authenticatorCancel only applies to authenticator operations specific to that authenticator session. (Sorry that's a little tongue twisting - I didn't name this stuff!).

@bzbarsky
Copy link
Author

bzbarsky commented Apr 6, 2017

OK, let's step through the spec.

https://w3c.github.io/webauthn/#getAssertion step 13 says:

For each authenticator in issuedRequests invoke the authenticatorCancel operation onn authenticator and remove authenticator from issuedRequests.

This will issue an authenticatorCancel for every authenticator in issuedRequests. issuedRequests contains things placed there in step 11; it has no reference to "authenticator session"s.

https://w3c.github.io/webauthn/#authenticatorcancel does refer to "authenticator session". OK, so where is that coming from? https://w3c.github.io/webauthn/#authenticator-ops says that authenticator sessions need to exist, but doesn't define what they actually contain.

In any case, the problem is that the authenticatorCancel call from https://w3c.github.io/webauthn/#getAssertion step 13 has no reference to any "authenticator session". So either there is one global one that it can grab, or the authenticatorCancel is happening in a new session. If the intent is that it happen in the same authenticator session as the authenticatorGetAssertion call in step 11, that needs to be clearly specified.

I'm also not sure what the "This operation must be invoked in an authenticator session which has no other operations in progress." bit means in https://w3c.github.io/webauthn/#authenticatorgetassertion and https://w3c.github.io/webauthn/#op-make-cred. Does it mean that if there is no such session a new session should be created? Does it mean that you have to wait for such a session to become available? The places that invoke the operation certainly don't check this, so it can't be an actual precondition....

@AngeloKai AngeloKai modified the milestones: WD-06, WD-05 May 3, 2017
@equalsJeffH
Copy link
Contributor

@jyasskin notes that this may be addressed by our clarifying what an "authenticator session" is (on 17-May-2017 call)

@jcjones
Copy link
Contributor

jcjones commented Oct 11, 2017

Dupe/Wontfix of #537

@jcjones jcjones closed this as completed Oct 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants