[<login>] Implement click interaction for <login> element [chromium/src : main]
Sam Goto (Gerrit)
Sam Goto added 2 comments![]( "Open in Gerrit")
Dominic,
I moved the purely DOM aspects of the other CL into this one, so that we can make <login> useful in isolation to how we are planning to use it in agentic flows (which requires the Mojom IPCs).
I addressed, I believe, the feedback you gave in the other CL.
Can you PTAL?
'base::Value',Sam GotoI'm wondering if I can avoid this change
Done
Related details
- Christian Biesinger
- Dominic Farolino
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Biesinger (Gerrit)
Christian Biesinger added 8 comments![]( "Open in Gerrit")
federated_auth_request_(document.GetExecutionContext()) {}Since this does not use CredentialManagerProxy and can't really use it, it is now possible to have multiple mojo connections from a renderer to the browser. Therefore, you will have to change https://source.chromium.org/chromium/chromium/src/+/main:content/browser/webid/request_service.cc;l=226?q=RequestService&ss=chromium to support multiple receivers. See related discussion in https://chromium-review.googlesource.com/c/chromium/src/+/7115160/6/content/browser/renderer_host/render_frame_host_impl.cc#14649 -- that patchset version may help.
DispatchEvent(*Event::Create(event_type_names::kSelect));I dislike reusing the select event... this event does not get dispatched when something is selected, but rather, when we receive a token. Perhaps the `complete` event is a better fit?
if (event.type() == event_type_names::kClick) {You'll want to check something like https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_anchor_element.cc;l=626;drc=f359b2876732d31100187ef4ad0d462864b168c2;bpv=1;bpt=1 so that a right click does not trigger the login
In addition, you probably want to allow enter to trigger the login as well, like https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_anchor_element.cc;l=219;drc=f359b2876732d31100187ef4ad0d462864b168c2;bpv=1;bpt=1
for (Node& child : NodeTraversal::ChildrenOf(*this)) {`for (HTMLCredentialElelemnt& credential : Traversal<HTMLCredentialElement>::ChildrenOf(*this))`
get_params->context = mojom::blink::RpContext::kSignIn;I would not set that here... this may not be right when it is triggered by the page
federated_auth_request_->RequestToken(Because this can be triggered by the page, we need to do the same security checks we do for navigator.credentials.get, especially permissions policy and CSP: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/credentialmanagement/authentication_credentials_container.cc;l=1427?q=authentication_credentials_c&ss=chromium
token && token->is_string()) {this seems like an unnecessary restriction
attribute EventHandler onselect;onselect already exists on global_event_handlers.idl so it is not needed here.
As discussed in https://github.com/w3ctag/design-principles/issues/611#issuecomment-3862411678, if the event was not already in global event handlers, it should be added there instead of here.
Related details
- Dominic Farolino
- Sam Goto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Goto (Gerrit)
Sam Goto added 8 comments![]( "Open in Gerrit")
federated_auth_request_(document.GetExecutionContext()) {}Since this does not use CredentialManagerProxy and can't really use it, it is now possible to have multiple mojo connections from a renderer to the browser. Therefore, you will have to change https://source.chromium.org/chromium/chromium/src/+/main:content/browser/webid/request_service.cc;l=226?q=RequestService&ss=chromium to support multiple receivers. See related discussion in https://chromium-review.googlesource.com/c/chromium/src/+/7115160/6/content/browser/renderer_host/render_frame_host_impl.cc#14649 -- that patchset version may help.
That makes sense.
Can you check if this is more or less what you had in mind?
and
did i get this rigth?
DispatchEvent(*Event::Create(event_type_names::kSelect));I dislike reusing the select event... this event does not get dispatched when something is selected, but rather, when we receive a token. Perhaps the `complete` event is a better fit?
`complete` makes sense to me too. done.
if (event.type() == event_type_names::kClick) {You'll want to check something like https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_anchor_element.cc;l=626;drc=f359b2876732d31100187ef4ad0d462864b168c2;bpv=1;bpt=1 so that a right click does not trigger the login
In addition, you probably want to allow enter to trigger the login as well, like https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_anchor_element.cc;l=219;drc=f359b2876732d31100187ef4ad0d462864b168c2;bpv=1;bpt=1
Ok, I think I got that right. Done?
for (Node& child : NodeTraversal::ChildrenOf(*this)) {`for (HTMLCredentialElelemnt& credential : Traversal<HTMLCredentialElement>::ChildrenOf(*this))`
Done
get_params->context = mojom::blink::RpContext::kSignIn;I would not set that here... this may not be right when it is triggered by the page
Done
federated_auth_request_->RequestToken(Because this can be triggered by the page, we need to do the same security checks we do for navigator.credentials.get, especially permissions policy and CSP: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/credentialmanagement/authentication_credentials_container.cc;l=1427?q=authentication_credentials_c&ss=chromium
Ok, I added CSP and permissions policies checks ... did I get this right?
this seems like an unnecessary restriction
Long term, I'd like to make "credential" be an Credential (rather than a DOMString, so that it can be an IdentityCredential or a PublicKeyCredential), but I ran into complications making core/ depend on //modules (which is where Credential is defined).
In the meantime, there is no harm in making "credential" take "any", to allow non-string tokens, so I went with that in this CL, but I figured it would be useful to say that longer term, this restriction would go away (when `credential` is of type `Credential`).
In the meantime, I store it as a member base::Value and encode/decode as JSON back and forth with the IDL.
onselect already exists on global_event_handlers.idl so it is not needed here.
As discussed in https://github.com/w3ctag/design-principles/issues/611#issuecomment-3862411678, if the event was not already in global event handlers, it should be added there instead of here.
Done, using "oncomplete" here and added to the global event handlers.
Related details
- Christian Biesinger
- Dominic Farolino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |