Add WebDriver commands #465
Conversation
|
|
||
| These WebDriver [=extension commands=] allow automated interactions with any | ||
| active FedCM dialogs. | ||
|
|
There was a problem hiding this comment.
Perhaps we add a subsection for each remote end step. Right now it's unclear what you're defining in each case unless you look at the POST closely. It could be in a title.
There was a problem hiding this comment.
I still don't understand how the behavior is limited in terms of interaction to make sure it cannot be exploited. Or is that a question for later?
There was a problem hiding this comment.
I'm not sure I understand the concern. Webdriver APIs are not exposed to the web (and also not enabled by default). See, for example, https://chromedriver.chromium.org/security-considerations
There are lots of ways webdriver can be exploited if that weren't the case since the APIs are very powerful.
This is only meant to be used by local automation/testing tools
|
That was an initial review, it mostly looks good. I'll take a closer look after the comments are addressed! |
|
PR LGTM too. Since this is normative, can you make sure we get Mozilla to review this too? |
|
@martinthomson @cboozar @bvandersloot-mozilla -- could you comment whether you are fine with this proposal? |
|
(I know Ben said as much in #395 (comment) but I'd like to reconfirm now that this is a spec PR) |
spec/index.bs
Outdated
| @@ -454,7 +465,8 @@ algorithm is invoked, the user agent MUST execute the following steps. This retu | |||
|
|
|||
| Issue: Support choosing accounts from multiple [=IDP=]s, as described [here](https://github.com/fedidcg/FedCM/issues/319). | |||
| 1. Run {{WindowOrWorkerGlobalScope/setTimeout()}} passing a [=task=] which throws a | |||
| {{NetworkError}}, after a timeout of 60 seconds. | |||
| {{NetworkError}}, after a timeout of 60 seconds, unless this <dfn>promise rejection | |||
There was a problem hiding this comment.
We're not doing this timeout in the implementation, so how is it supposed to work?
There was a problem hiding this comment.
That's an unfortunate aspect of #453 not having landed yet. Once either PR is pushed I'll update the other one.
There was a problem hiding this comment.
Updated this text per the timeout change in #453 .
|
I have added a getdialogtype command to this PR, to support the recently added re-authentication support. |
I am adding these as standard commands (not vendor-specific) because the spec PR is in progress right now: w3c-fedid/FedCM#465 and Firefox is also on board: w3c-fedid/FedCM#395 (comment) Bug: 1356159, 1441903 Change-Id: I561692ee695e63ee45b5f2b227bc49982a624eea Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4402971 Reviewed-by: Vladimir Nechaev <[email protected]> Commit-Queue: Christian Biesinger <[email protected]> Cr-Commit-Position: refs/heads/main@{#1142272}
I am adding these as standard commands (not vendor-specific) because the spec PR is in progress right now: w3c-fedid/FedCM#465 and Firefox is also on board: w3c-fedid/FedCM#395 (comment) Bug: 1356159, 1441903 Change-Id: I561692ee695e63ee45b5f2b227bc49982a624eea Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4402971 Reviewed-by: Vladimir Nechaev <[email protected]> Commit-Queue: Christian Biesinger <[email protected]> Cr-Commit-Position: refs/heads/main@{#1142272}
|
This is good as-is, assuming that IDP chooser endpoints similar to those for the account chooser and a new result for getdialogtype would be friendly additions when we resolve w3c-fedid/multi-idp#2 |
|
Absolutely! That's how I envisioned supporting your UI. |
|
Thanks for putting this together and thanks @bvandersloot-mozilla for the review! Merged! |
SHA: 865e7d6 Reason: push, by samuelgoto Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
SHA: 865e7d6 Reason: push, by pull[bot] Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Preview | Diff