-
Notifications
You must be signed in to change notification settings - Fork 245
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
Add clearOriginJoinedAdInterestGroup to spec.bs #844
Merged
JensenPaul
merged 16 commits into
WICG:main
from
MattMenke2:MattMenke2-clearOriginJoinedAdInterestGroup
Oct 26, 2023
Merged
Changes from 10 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
e6f9c9d
Add clearOriginJoinedAdInterestGroup to spec.bs
MattMenke2 a1c6a7e
Update spec.bs
MattMenke2 44cff6d
Update spec.bs
MattMenke2 e77a7dc
Update spec.bs
MattMenke2 5cd22de
Update spec.bs
MattMenke2 56125fb
Update spec.bs
MattMenke2 0e0a996
Update spec.bs
MattMenke2 a7aef5f
Update spec.bs
MattMenke2 bd0d0c9
Update spec.bs
MattMenke2 af2744c
Update spec.bs
MattMenke2 283cbd7
Update spec.bs
MattMenke2 f5ac9b7
Update spec.bs
MattMenke2 ee2a856
Update spec.bs
MattMenke2 6ac49ce
Update spec.bs
MattMenke2 b71e7ba
Merge branch 'main' into MattMenke2-clearOriginJoinedAdInterestGroup
MattMenke2 79afd0d
Update spec.bs
MattMenke2 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if this is called in a fenced frame? Why does "top level page" mean there? Basically I'm trying to see if we can link to one of these:
so we can make this more rigorous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When run in a fenced frame, it behaves exactly like leaveAdInterestGroup() does when passed a non-empty group: It throws. Note that I exactly copied the verbiage for that, so if one is wrong, they both are. I assume the throwing is covered by the 'not allowed to use the "join-ad-interest-group" policy-controlled feature' bit, but could be wrong.
I copied "top level page's origin" from the definition of "joining origin" ("The top level page origin from where the interest group was joined.") - reusing terminology used when defining joining origin when mentioning joining origin seems reasonable to me. If that terminology is wrong, then that definition needs to be updated as well. The doc repeatedly refers to "top-level origin", which may be the term we want? I've replaced it here, but left joining origin's definition alone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. With a new mode of fenced frames being introduced soon-ish that will allow arbitrary permission policies to be enabled inside of a fenced frame (I think), we should figure out what this API should do in that context outside of the usual permission policy restrictions. Basically I'm just trying to figure out if the behavior reached beyond the fenced frame and into the outermost main frame to grab the "top-level" origin, or if it is scoped to the fenced frame.
If you could point me to the part of the spec where we run the comparison between the "all interest group joining origins" and "the top level page's origin" (you know, to determine whether the user should be removed from the IG or not) then I think that'd help me answer this definitively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The text for setting joining origin in joinAdInterestGroup is: "Set interestGroup’s joining origin to this's relevant settings object's top-level origin."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK thanks, so the top-level origin is scoped to a fenced frame here, and does not transcend the boundary (i.e., does not "jump" the fence).
In that case, I think the test we should use here is:
(Which should link to https://html.spec.whatwg.org/multipage/webappapis.html#relevant-settings-object and https://html.spec.whatwg.org/multipage/webappapis.html#concept-environment-top-level-origin respectively, of course)
Does that make sense / sound good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, done.
Unless we're letting these fenced frames set 1P cookies for the fenced frame's origin, I suspect this behavior is not what we want, long term, since this is much akin to setting 1P cookies for the embedded frame (albeit 1P cookies that can only be accessed in an auction - using the top-level-page's joining origin is more like partitioned cookies), but don't think this PR is the place to iron that out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's interesting. Let me just cc @shivanigithub just for extra exposure, but I agree that it shouldn't have a bearing on this PR specifically.