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

Consider not running internal methods in parallel or add "consumes user activation" to registry? #243

Open
marcoscaceres opened this issue Jul 2, 2024 · 8 comments

Comments

@marcoscaceres
Copy link
Member

It might be good to consider not running the internal methods in parallel, and allowing them to be called with a promise.

The problem is that the Cred Man is having to account for main thread behavior (e.g., permission policy checks) that ought to be potentially left to other specs.

Another situation where this has come up is with consuming the user action, which should be done on the main thread.

We could, however, make a hard determination that calling .get() (and possibly other methods, if they show UI... like maybe .create()) should consume the user activation.

Alternative, we add another column to the registry table for "consumes user activation?", and we add true/false values to that column (least amount of work... but again brings baggage from other specs into Cred Man... which may be ok)

@nsatragno, how would you prefer we proceed?

@nsatragno
Copy link
Member

This has come up in the past and I would not be surprised if there were already bugs in said other specs (webauthn example) where actions that should only be run in the main thread are being executed in internal methods. It's a pretty subtle issue for those of us who are not well versed in the browser event model.

It might be good to consider not running the internal methods in parallel, and allowing them to be called with a promise.

We could have the default implementation of the promise-based methods (discover from external source, collecting credentials, etc) be to call the non-promise methods in parallel to avoid having to rewrite every client spec in one fell swoop. Ideally this would come with some expectation that client specs eventually update to implementing the promise methods so we can remove the existing non-promise methods. It can even be done in small chunks, one navigator.credentials method at a time. It's doable, but also feels like a lot of work for the expectation that we'll make it a little easier on the handful of people writing dependant specs.

If you really want to do it (and slice the work up in manageable chunks) -- this seems like the right way to do it and I'll happily provide reviews and set up some time to update WebAuthn.

We could, however, make a hard determination that calling .get() (and possibly other methods, if they show UI... like maybe .create()) should consume the user activation.

This might not be enough: e.g. for WebAuthn the standard only requires consuming an activation for create on a cross-origin frame.

Alternative, we add another column to the registry table for "consumes user activation?", and we add true/false values to that column (least amount of work... but again brings baggage from other specs into Cred Man... which may be ok)

If we go this route I feel slightly more inclined to add specific wording on a case-by-case basis like we do for permissions policy before #242, just because of that cross-origin webauthn case. As you point out that means bringing more client-spec stuff into credman which is suboptimal, but if it lets you focus on more important things, it's probably okay too.

Overall: I'll support you through option 1 if you want to invest that amount of work, but will also stamp option 2 so we can move on to more substantial work (:

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Jul 3, 2024

for WebAuthn the standard only requires consuming an activation for create on a cross-origin frame.

Interesting. @timcappalli, do you know the rationale for this? This seems like a potential bug for the reasons I outlined above (if UI is presented, then activation should be consumed by top-level frame too otherwise one could trigger, for example, payment request in the same micro-task).

(I see that Tim is out on vacation... I'll try to follow up with some other folks around this)

@marcoscaceres
Copy link
Member Author

Found some history around user activation: w3c/webauthn#1801

@stephenmcgruer, can you help me understand the change a bit more? I don't know Web Authn very well... can .create() be called in a first-party context without presenting UI in Web Authn? I'm trying to understand why it's different in third-party contexts.

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Jul 4, 2024

Argh... I found more issues with this while implementing... in WebKit, both WebAuth and Digital Cred treat it as not running in parallel until necessary (i.e., we pass the promise).

Just for a bit more detail, I don't just want to "consume the user activation", I also want to hook into fully active descendant of a top-level traversable with user attention. I can only do that via having access to the document on the main thread.

What if, to keep backwards compat, we add the promise as optional argument... then in the spec we say, if the promise was passed, then you are in control of when you go into parallel... otherwise, run in parallel?

We can then reach out to various specs to change their behavior and hopefully get everyone to "choose their own adventure" 🤠

@stephenmcgruer
Copy link
Contributor

@stephenmcgruer, can you help me understand the change a bit more? I don't know Web Authn very well... can .create() be called in a first-party context without presenting UI in Web Authn? I'm trying to understand why it's different in third-party contexts.

Sorry Marcos, this question didn't go to my work email so I missed it >_<. You are correct that create() can be called in a first-party context without needing/consuming a user interaction, however as far as I know it will always show UI anyway.

I'm not an expert on webauthn, but I understand that there were historical reasons behind webauthn in general not requiring a user activation for either create() or get() calls. I believe there were existing user flows for which the webauthn team wished to make it possible to drop-in replace webauthn for the authentication method that was in use, but that in those flows no user interaction was present to initiate the authentication. (For example, flows where you are redirected to an authentication provider - upon arrival at the new webpage, you no longer have any user interaction).

When it came time to do create() in a third-party context, I felt we weren't trying to be a drop-in replacement for anything, and I was a lot more concerned about malicious iframes - in particular them trying to trick you into thinking you're registering a credential for the main frame rather than a different origin. Hence not only the permission policy requirement but also the user interaction one.


From the Chrome side, we're not unified on this but I think we more and more have the belief that user activation guards are not very significant in terms of protecting users from hostile websites - it's too easy for a website to cause the user to give it interaction. However they can still be useful as an anti-spam/anti-DOS technique (e.g., to stop websites triggering focus-capturing flows repeatedly). That's why we've begun exploring spaces such as 'first call is free' - currently scattered across multiple specs, but maybe one day we'll unify this to a core concept in the user activation spec itself?

@bvandersloot-mozilla
Copy link

Mozillian driving by... 👋

I don't think that you could guarantee a whether or not a given [[Discover...]] call will show a dialog from only the credential type and request parameters. So I don't think a boolean on the registry would work.

What if, to keep backwards compat, we add the promise as optional argument... then in the spec we say, if the promise was passed, then you are in control of when you go into parallel... otherwise, run in parallel?

I'm not sure if I follow where the optional argument goes exactly. IIUC the caller would need to decide whether to put a promise into the Credential subclass's internal method or to invoke it in parallel. If you add a different bit to the registry, that could do it: "internal methods in parallel", set to true for all current entries.

user activation guards are not very significant in terms of protecting users from hostile websites - it's too easy for a website to cause the user to give it interaction. However they can still be useful as an anti-spam/anti-DOS technique (e.g., to stop websites triggering focus-capturing flows repeatedly).

This matches my personal understanding. This is why requiring and consuming user activation for user-prompts or dialogs is a good heuristic in design. It also has a nice property of making that dialog occur after the user did a thing. Now if that thing is relevant or not to the dialog can't be guaranteed in any way (although PEPC seems to be trying to tackle that) but at least the user can draw some cause and effect. This lets them not do the thing over and over again if they are being spammed.

That's why we've begun exploring spaces such as 'first call is free' - currently scattered across multiple specs, but maybe one day we'll unify this to a core concept in the user activation spec itself?

I think unifying on a principled, unified decision would be nice :)

@nsatragno
Copy link
Member

I'm not sure if I follow where the optional argument goes exactly. IIUC the caller would need to decide whether to put a promise into the Credential subclass's internal method or to invoke it in parallel. If you add a different bit to the registry, that could do it: "internal methods in parallel", set to true for all current entries.

Adding the extra column in the registry means we have one more thing to update in cred man that depends on the specifics of each credential type. This isn't that much better than writing the specifics of how user activation is handled for webauthn & digital credentials on credman.

If we want to fix this right, why not add a set of internal methods that return (or take) a promise Promise<result> [[Discover...]](), and have their default implementation call the non-promise versions Result [[Discover...]]() in parallel? This also allows us to handle both cases the same on the algorithm that calls the internal methods, and lets credential types be updated piecemeal without opening more PRs against credman.

@marcoscaceres
Copy link
Member Author

Yeah, I'm landing at @nsatragno solution too. I'd prefer not to overload (or have two) methods tho. If we make the promise optional, then we can use it as a switch for to run in parallel or not in Cred Man.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants