Add contextID to originAttributes in nsIPrincipal

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: tanvi, Assigned: englehardt)

Tracking

(Blocks 1 bug)

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

We need a new attribute for Contextual Identity / Containers.  We want another key to the same origin policy so that users can segregate their behavior into different context.  Example uses cases:
* User has multiple accounts on websites and wants to be logged into both simultaneously.  One for personal use, the other for work related use.
* User wants a session that persists but doesn't leak information to trackers about their browsing behavior in other contexts.  (Ex: doesn't want the shopping context to affect the ads they see in their work context.  Or wants to remain logged into a social network without being tracked across the web)

A good example addon demonstrating the usage which uses the appId originAttribute - https://addons.mozilla.org/en-US/firefox/addon/priv8/

This bug is to extend nsIPrincipal to include contextID as an originAttribute.  We can set it to 0 as a default value.  Then in followup bugs we will create UX that will change the value for different contexts.
https://mxr.mozilla.org/mozilla-central/source/caps/nsIPrincipal.idl?force=1#158

Some more notes on the platform infrastructure and this project - https://etherpad.mozilla.org/Eo3sSnFQj4
Depends on: 1179985
No longer depends on: 1163254
See Also: → 1181953
Posted patch 1179557-v1.patch (obsolete) — Splinter Review
I've added containerId as an integer defaulting to 0 in originAttributes. I also added in some additional xpcshell unit tests. Currently on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d20b573db12
Attachment #8638161 - Flags: review?(tanvi)
Posted patch 1179557-part1.patch (obsolete) — Splinter Review
Forgot to bump CIDs
Attachment #8638161 - Attachment is obsolete: true
Attachment #8638161 - Flags: review?(tanvi)
Attachment #8638263 - Flags: review?(tanvi)
Posted patch 1179557-part2.patch (obsolete) — Splinter Review
Attachment #8639577 - Flags: review?(tanvi)
Attachment #8639577 - Flags: review?(bobbyholley)
Attachment #8638263 - Flags: review?(bobbyholley)
Comment on attachment 8639577 [details] [diff] [review]
1179557-part2.patch

Review of attachment 8639577 [details] [diff] [review]:
-----------------------------------------------------------------

I think you forgot to qref your changes to nsIPrincipal.idl?
Attachment #8639577 - Flags: review?(bobbyholley)
Comment on attachment 8638263 [details] [diff] [review]
1179557-part1.patch

Review of attachment 8638263 [details] [diff] [review]:
-----------------------------------------------------------------

::: caps/nsIScriptSecurityManager.idl
@@ +254,5 @@
>      const unsigned long NO_APP_ID = 0;
>      const unsigned long UNKNOWN_APP_ID = 4294967295; // UINT32_MAX
>      const unsigned long SAFEBROWSING_APP_ID = 4294967294; // UINT32_MAX - 1
>  
> +    const unsigned long NO_CONTAINER_ID = 0;

This isn't really "no" container, so much as the "default" container, right? Or am I missing something?

::: dom/webidl/ChromeUtils.webidl
@@ +45,5 @@
>   *     use OriginAttributes in their nsISerializable implementations.
>   */
>  dictionary OriginAttributesDictionary {
>    unsigned long appId = 0;
> +  unsigned long containerId = 0;

This is too generic of a name to be used here. How about "userContext"? I'm happy to workshop the name, but this shouldn't land until we have something we're all satisfied with.
Attachment #8638263 - Flags: review?(bobbyholley) → review+
Comment on attachment 8638263 [details] [diff] [review]
1179557-part1.patch

Name options are:
containerID
contextID
userContext
userContextID

If containerID is too generic, then contextID probably is to since "context" is overloaded in our codebase.
Attachment #8638263 - Flags: review?(tanvi) → review+
Attachment #8639577 - Flags: review?(tanvi) → review+
Attachment #8638263 - Flags: review+ → review?(tanvi)
userContextId sounds like the way to go then, assuming that we want this thing to be an integer index into some registry somewhere. Note that the spelling of "Id" should be consistent with "appId" and "addonId".
Comment on attachment 8638263 [details] [diff] [review]
1179557-part1.patch

r+ with the name changes discussed.
Attachment #8638263 - Flags: review?(tanvi) → review+
Carrying over r+ from Tanvi and Bobby. I agree that userContextId is the most appropriate name. I've updated containerId -> userContextId in this patch.
Attachment #8638263 - Attachment is obsolete: true
Attachment #8640236 - Flags: review+
Posted patch 1179557-part2.patch (obsolete) — Splinter Review
Including changes to nsIPrincipal.idl this time.
Attachment #8639577 - Attachment is obsolete: true
Attachment #8640237 - Flags: review?(tanvi)
Attachment #8640237 - Flags: review?(bobbyholley)
(In reply to Bobby Holley (:bholley) from comment #5)

> This isn't really "no" container, so much as the "default" container, right?
> Or am I missing something?

You're right in thinking that there isn't ever really "no" container. I've updated it to be DEFAULT_USER_CONTEXT_ID. The thinking here is that we won't add an attribute to the origin string when this is set to 0, and we won't have any tab/url bar decoration like the kind we've discussed in Bug 1181953. So this is the browsers default state or default container.
Comment on attachment 8640237 [details] [diff] [review]
1179557-part2.patch

Review of attachment 8640237 [details] [diff] [review]:
-----------------------------------------------------------------

::: caps/nsIPrincipal.idl
@@ +262,5 @@
> +     * Gets the id of the user context this principal is inside.  If this
> +     * principal is inside the default userContext, this returns
> +     * nsIScriptSecurityManager::DEFAULT_USER_CONTEXT_ID.
> +     */
> +    [infallible] readonly attribute unsigned long userContextId;

You'll need to rev the UUID of this interface.
Attachment #8640237 - Flags: review?(bobbyholley) → review+
Attachment #8640237 - Flags: review?(tanvi) → review+
Carrying over r+ from bholley, tanvi. Bumping the uuid.

Resubmit to try for unresolved WinXP error: https://treeherder.mozilla.org/#/jobs?repo=try&revision=363a8379666a
Attachment #8640237 - Attachment is obsolete: true
Attachment #8641291 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/993a36e7e0b3
https://hg.mozilla.org/mozilla-central/rev/7f05504085b1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Blocks: 1191460
Blocks: 1195881
You need to log in before you can comment on or make changes to this bug.