[fedcm] Extend the LocalFrame to query/notify <federation>s elements [chromium/src : main]
Sam Goto (Gerrit)
Sam Goto added 1 comment![]( "Open in Gerrit")
Christian, I took your advice and started extending the LocalFrame mojo. I think this works much better than the other approaches. Can you PTAL?
Related details
- Christian Biesinger
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")
struct Federation {If I recall correctly, the design expects this element to contain a clickable element. If this is something we want to keep, maybe add something like `boolean is_clickable` that is true if there is a child element, and a function ClickFederationElement or something? Although maybe we should do that separately from this CL anyway.
if (Document* document = GetDocument()) {Everywhere else in this file assumes that GetDocument() can't return null. Do you have reason to believe that this instance is special?
If you want to keep the nullcheck, I'd turn this into an early return if document is null.
(same in the other function below)
element.GetId(), element.GetIdentityProviderRequestOptions()));We should probably allow for GetIdentityProviderRequestOptions() to return null if required attributes were not specified (e.g. no URL), and not add it to the result list if so
GetNonEmptyURLAttribute(html_names::kConfigurlAttr);We may want to verify that this is not an invalid URL
// TODO(crbug.com/441895082): allow a structured JSON be passed toSo..
That is actually not what I meant and I don't think this is possible because this is an HTML attribute.
However, params is supposed to be serialized JSON, and since we are just reading a string from the attribute, it is possible that it is not valid JSON. In that case, we probably should not send malformed JSON to the server.
DispatchEvent(*Event::Create(event_type_names::kToken));It would be better to create a new event interface and put the received token on the event, IMO.
readonly attribute DOMString token;that does not look right, seems better to expose this on the event object?
webexposed/global-interface-listing.html [ Failure ]I am confused, what is the mismatch this is referring to?
it would be better to update the expectations file so that future unexpected mismatches do not get undetected
Related details
- 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")
struct Federation {If I recall correctly, the design expects this element to contain a clickable element. If this is something we want to keep, maybe add something like `boolean is_clickable` that is true if there is a child element, and a function ClickFederationElement or something? Although maybe we should do that separately from this CL anyway.
Yeah, I was planning to leave the entire "what happens when you click on it" to a separate CL to keep this one a bit more self-contained.
I'm going to resolve, feel free to re-open if you feel like I should add that scope to this CL.
if (Document* document = GetDocument()) {Everywhere else in this file assumes that GetDocument() can't return null. Do you have reason to believe that this instance is special?
If you want to keep the nullcheck, I'd turn this into an early return if document is null.
(same in the other function below)
Just curious: when would GetDocuments() be null in this case? Doesn't an HTMLElement always need to be attached to a Document?
element.GetId(), element.GetIdentityProviderRequestOptions()));We should probably allow for GetIdentityProviderRequestOptions() to return null if required attributes were not specified (e.g. no URL), and not add it to the result list if so
Done. Also, added unittests and browser tests for this case.
GetNonEmptyURLAttribute(html_names::kConfigurlAttr);We may want to verify that this is not an invalid URL
Done, also added a unit test.
// TODO(crbug.com/441895082): allow a structured JSON be passed toSo..
That is actually not what I meant and I don't think this is possible because this is an HTML attribute.
However, params is supposed to be serialized JSON, and since we are just reading a string from the attribute, it is possible that it is not valid JSON. In that case, we probably should not send malformed JSON to the server.
Yeah, I think I understand what you are looking for now. I think I have addressed, PTAL?
One side effect of this is that if a developer wants to pass a "string" as a param, they have to wrap it around a \", which I think is OK.
WDYT?
Also, added a test.
DispatchEvent(*Event::Create(event_type_names::kToken));It would be better to create a new event interface and put the received token on the event, IMO.
Yeah, that's what I thought too, but mt raised to me in a TAG review that state should actually be in the element, rather than in the event from this principle here: https://w3ctag.github.io/design-principles/#state-and-subclassing
Resolving, lmk if you strongly disagree with the principle or its applications.
readonly attribute DOMString token;that does not look right, seems better to expose this on the event object?
Oh yeah, I thought so too, but that's what I heard from mt in a TAG review that pointed to to this design principle: https://w3ctag.github.io/design-principles/#state-and-subclassing
Resolving, lmk if you disagree.
webexposed/global-interface-listing.html [ Failure ]I am confused, what is the mismatch this is referring to?
it would be better to update the expectations file so that future unexpected mismatches do not get undetected
This may go away if I sync my main branch, but at the moment, there is an unrelated failure happening here. I'll add it to this CL.
Related details
- Christian Biesinger
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. |
Christian Biesinger (Gerrit)
Christian Biesinger added 5 comments![]( "Open in Gerrit")
<federation clientid="invalid-configurl" configurl="https://:80"></federation>maybe add a test with invalid JSON in params?
if (Document* document = GetDocument()) {Sam GotoEverywhere else in this file assumes that GetDocument() can't return null. Do you have reason to believe that this instance is special?
If you want to keep the nullcheck, I'd turn this into an early return if document is null.
(same in the other function below)
Just curious: when would GetDocuments() be null in this case? Doesn't an HTMLElement always need to be attached to a Document?
This code goes from LocalFrameMojoHandler to Document, not from Element. I do not actually know when/if this can be null, I am just going by what other functions do 😞
std::optional<mojom::blink::IdentityProviderRequestOptionsPtr>It's a pointer, I would just return null for failure instead of wrapping an optional around it
// TODO(crbug.com/441895082): allow a structured JSON be passed toSam GotoSo..
That is actually not what I meant and I don't think this is possible because this is an HTML attribute.
However, params is supposed to be serialized JSON, and since we are just reading a string from the attribute, it is possible that it is not valid JSON. In that case, we probably should not send malformed JSON to the server.
Yeah, I think I understand what you are looking for now. I think I have addressed, PTAL?
One side effect of this is that if a developer wants to pass a "string" as a param, they have to wrap it around a \", which I think is OK.
WDYT?
Also, added a test.
I think that's reasonable.
I can't decide whether I prefer ignoring invalid JSON like you do, or returning null and logging an error. so I'll go with what you did here.
DispatchEvent(*Event::Create(event_type_names::kToken));Sam GotoIt would be better to create a new event interface and put the received token on the event, IMO.
Yeah, that's what I thought too, but mt raised to me in a TAG review that state should actually be in the element, rather than in the event from this principle here: https://w3ctag.github.io/design-principles/#state-and-subclassing
Resolving, lmk if you strongly disagree with the principle or its applications.
Hmm I am not sure I agree that the received token is "state" in this way, but I'm not going to argue with the TAG, so this is fine.
Related details
- 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 4 comments![]( "Open in Gerrit")
<federation clientid="invalid-configurl" configurl="https://:80"></federation>maybe add a test with invalid JSON in params?
I added a unit test here:
I was hoping to keep these corner cases in the unit test, and cover only some of the main cases in the browser test.
Feel free to re-open if you disagree and I can add one here too.
if (Document* document = GetDocument()) {Sam GotoEverywhere else in this file assumes that GetDocument() can't return null. Do you have reason to believe that this instance is special?
If you want to keep the nullcheck, I'd turn this into an early return if document is null.
(same in the other function below)
Christian BiesingerJust curious: when would GetDocuments() be null in this case? Doesn't an HTMLElement always need to be attached to a Document?
This code goes from LocalFrameMojoHandler to Document, not from Element. I do not actually know when/if this can be null, I am just going by what other functions do 😞
Yeah, I don't think I have any reasons to believe hat GetDocument() can be null that would be inconsistent with how the other methods operate too. I'm going to remove the if () block here and go from there.
std::optional<mojom::blink::IdentityProviderRequestOptionsPtr>It's a pointer, I would just return null for failure instead of wrapping an optional around it
Done
webexposed/global-interface-listing.html [ Failure ]Sam GotoI am confused, what is the mismatch this is referring to?
it would be better to update the expectations file so that future unexpected mismatches do not get undetected
This may go away if I sync my main branch, but at the moment, there is an unrelated failure happening here. I'll add it to this CL.
- Christian Biesinger
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 voted and added 9 comments![]( "Open in Gerrit")
Votes added by Christian Biesinger
| Code-Review | +1 |
9 comments
<federation clientid="invalid-configurl" configurl="https://:80"></federation>Sam Gotomaybe add a test with invalid JSON in params?
I added a unit test here:
I was hoping to keep these corner cases in the unit test, and cover only some of the main cases in the browser test.
Feel free to re-open if you disagree and I can add one here too.
sounds good
void DispatchFederationEvent(const base::UnguessableToken& id,I would rename id to `element_id` or something like that
[CEReactions, Reflect] attribute DOMString clientid;Per https://www.w3.org/TR/design-principles/#casing-rules this should be `clientId`
[CEReactions, Reflect, URL] attribute USVString configurl;`configURL`
[CEReactions, Reflect] attribute DOMString loginhint;`loginHint`
[CEReactions, Reflect] attribute DOMString domainhint;`domainHint`
virtual/stable/webexposed/global-interface-listing.html [ Pass ]this shouldn't be needed now
interface XSLTProcessorcan you remove this change?
Related details
- 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 6 comments![]( "Open in Gerrit")
void DispatchFederationEvent(const base::UnguessableToken& id,I would rename id to `element_id` or something like that
Done
[CEReactions, Reflect] attribute DOMString clientid;Per https://www.w3.org/TR/design-principles/#casing-rules this should be `clientId`
Done
[CEReactions, Reflect, URL] attribute USVString configurl;Sam Goto`configURL`
Done
[CEReactions, Reflect] attribute DOMString loginhint;Sam Goto`loginHint`
Done
[CEReactions, Reflect] attribute DOMString domainhint;Sam Goto`domainHint`
Done
virtual/stable/webexposed/global-interface-listing.html [ Pass ]this shouldn't be needed now
- Christian Biesinger
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 1 comment![]( "Open in Gerrit")
can you remove this change?
Yes. Done.
Related details
- Christian Biesinger
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Goto (Gerrit)
Sam Goto added 1 comment![]( "Open in Gerrit")
Nasko and @chromium-ipc-reviewers,
Could you please take a look at:
third_party/blink/public/mojom/frame/ frame.mojom
content/public/test/ fake_local_frame.h
content/public/test/ fake_local_frame.cc
?
Related details
- Christian Biesinger
- Chromium IPC Reviews
- Nasko Oskov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: na...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): na...@chromium.org
Note: IPC gwsq added no new reviewers; existing reviewers satisfied requirements!
Reviewer source(s):
na...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Christian Biesinger
- Nasko Oskov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino added 10 comments![]( "Open in Gerrit")
methods to the LocalFrame renderer mojom service, so that the browser```suggestion
methods to the LocalFrame renderer mojom interface, so that the browser
```
https://github.com/fedidcg/declarative-loginI see no mention of the `<federation>` element there, but I do see lots of `<login>` and `<credential>`. What's missing?
GetFederations() => (array<Federation> federations);Technically per https://docs.google.com/document/d/1Kw4aTuISF7csHnjOpDJGc7JYIjlvOAKRprCTBVWw_E4/edit?tab=t.0#heading=h.jv90dicow9kc, these new methods need to have at least one non-test usage, but they're only really exercised in browser tests and no production path.
https://chromium-review.googlesource.com/c/chromium/src/+/7539587 probably has some "production"-paths. Would it be possible to chain the next CL on top of this one that shows how these are really going to be used? (or point out which parts of that aforementioned CL I should look at to get a feel for how these mojo methods would be used outside of tests)?
DispatchFederationEvent(mojo_base.mojom.UnguessableToken element_id, string token);What is this supposed to do besides just dispatching a DOM event? What Blink actions are supposed to happen, or what does this indicate?
String token_;Can you add some documentation describing what these members are, and how they relate to the concepts in the explainer?
options->config->client_id = "";```suggestion
options->config->client_id = g_empty_atom;
```
Maybe?
token_ = token;Can you `DCHECK(isConnected());` first?
attribute EventHandler ontoken;Per https://github.com/w3ctag/design-principles/pull/612, I think you'll want to add this to global_event_handlers.idl. I'd go with that approach in the meantime, and revert back to this one if my PR turns out to be in the wrong direction.
Related details
- Christian Biesinger
- Nasko Oskov
- 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. |
Christian Biesinger (Gerrit)
Christian Biesinger added 2 comments![]( "Open in Gerrit")
DispatchFederationEvent(mojo_base.mojom.UnguessableToken element_id, string token);What is this supposed to do besides just dispatching a DOM event? What Blink actions are supposed to happen, or what does this indicate?
That's it -- the website is supposed to listen to the event to receive the token that the IDP sent to the browser.
In a regular FedCM call, there's a promise returned from navigator.credentials.get. But in this case, because it's declarative, we instead deliver the token through this event. (Per https://www.w3.org/TR/design-principles/#state-and-subclassing, the token is stored on the federation element, not on the event object)
token_ = token;Can you `DCHECK(isConnected());` first?
just curious -- what's wrong with dispatching an event on a disconnected element? (I know this can't actually happen here, because of how we find the element)
Related details
- Nasko Oskov
- 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. |
Christian Biesinger (Gerrit)
Christian Biesinger added 1 comment![]( "Open in Gerrit")
const base::UnguessableToken id_;after looking around a bit, this should probably go away in favor of DOMNodeId.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/dom_node_ids.h;l=25;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;bpv=1;bpt=1?q=renderer%2Fcore%2Fdom%2Fdom_node_ids.h&ss=chromium%2Fchromium%2Fsrc
Dominic Farolino (Gerrit)
Dominic Farolino added 2 comments![]( "Open in Gerrit")
DispatchFederationEvent(mojo_base.mojom.UnguessableToken element_id, string token);Christian BiesingerWhat is this supposed to do besides just dispatching a DOM event? What Blink actions are supposed to happen, or what does this indicate?
That's it -- the website is supposed to listen to the event to receive the token that the IDP sent to the browser.
In a regular FedCM call, there's a promise returned from navigator.credentials.get. But in this case, because it's declarative, we instead deliver the token through this event. (Per https://www.w3.org/TR/design-principles/#state-and-subclassing, the token is stored on the federation element, not on the event object)
Ah so Blink literally isn't supposed to take any action besides storing the state, and telling the developer about it here so they can grab it off of the element?
token_ = token;Christian BiesingerCan you `DCHECK(isConnected());` first?
just curious -- what's wrong with dispatching an event on a disconnected element? (I know this can't actually happen here, because of how we find the element)
Nothing, it's just that this event is not intended to ever be dispatched on a disconnected federation element (at least through this path), so I figured we'd "enforce" that here.
Christian Biesinger (Gerrit)
Christian Biesinger added 1 comment![]( "Open in Gerrit")
DispatchFederationEvent(mojo_base.mojom.UnguessableToken element_id, string token);Christian BiesingerWhat is this supposed to do besides just dispatching a DOM event? What Blink actions are supposed to happen, or what does this indicate?
Dominic FarolinoThat's it -- the website is supposed to listen to the event to receive the token that the IDP sent to the browser.
In a regular FedCM call, there's a promise returned from navigator.credentials.get. But in this case, because it's declarative, we instead deliver the token through this event. (Per https://www.w3.org/TR/design-principles/#state-and-subclassing, the token is stored on the federation element, not on the event object)
Ah so Blink literally isn't supposed to take any action besides storing the state, and telling the developer about it here so they can grab it off of the element?
Yes that's correct
Dominic Farolino (Gerrit)
Dominic Farolino added 1 comment![]( "Open in Gerrit")
DispatchFederationEvent(mojo_base.mojom.UnguessableToken element_id, string token);Christian BiesingerWhat is this supposed to do besides just dispatching a DOM event? What Blink actions are supposed to happen, or what does this indicate?
Dominic FarolinoThat's it -- the website is supposed to listen to the event to receive the token that the IDP sent to the browser.
In a regular FedCM call, there's a promise returned from navigator.credentials.get. But in this case, because it's declarative, we instead deliver the token through this event. (Per https://www.w3.org/TR/design-principles/#state-and-subclassing, the token is stored on the federation element, not on the event object)
Christian BiesingerAh so Blink literally isn't supposed to take any action besides storing the state, and telling the developer about it here so they can grab it off of the element?
Yes that's correct
What "type" should `token` be? It's a string, but can it be described by any more specific type?
Nasko Oskov (Gerrit)
Nasko Oskov added 1 comment![]( "Open in Gerrit")
DispatchFederationEvent(mojo_base.mojom.UnguessableToken element_id, string token);I find it strange to add mojo IPC methods without actual usage. +1 on dom@'s comment that we should see a bit more of the picture and linking to the future CLs in this one is needed.
Related details
- Sam Goto
Sam Goto (Gerrit)
Sam Goto added 3 comments![]( "Open in Gerrit")
I re-based this CL on top of other three to try to answer some of the questions.
I still don't have a good answer on how to re-base this CL in a way that answers the question of "how does this get used outside of tests" for this CL specifically, because it is being used in agentic experiences and I'm not sure yet how to connect this CL with it.
I'm working with my team to see how things connect here and I'll report back, but in the meantime, I branched this CL out of other 3 CLs that I think can be looked at independently from this one here, specifically.
I see no mention of the `<federation>` element there, but I do see lots of `<login>` and `<credential>`. What's missing?
This is somewhat changing as I'm gathering more and more implementation experience, so apologies for the confusion.
I have rebased this change onto a renaming that more closely matches what's described in the explainer.
https://chromium-review.googlesource.com/c/chromium/src/+/7558441
I'm going to mark this as resolve since I believe the changes above (and rebasing this CL on top of that) hopefully clarifies how `<federation>` got renamed to `<credential type="federated">` and how `<login>` gets introduced (and how it relates to `<crednetial>` now.).
Feel free to re-open if this is still confusing.
DispatchFederationEvent(mojo_base.mojom.UnguessableToken element_id, string token);I find it strange to add mojo IPC methods without actual usage. +1 on dom@'s comment that we should see a bit more of the picture and linking to the future CLs in this one is needed.
SGTM
Let me rebase some of these changes so that it gets easier to see how this (specifically, the Mojom browser/renderer API changes) is intended to be used.
Related details
- Christian Biesinger
- Dominic Farolino
- Nasko Oskov
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. |