Manually add URLs to a container

[kurahaupo]

This PR is a repeat of PR #1688 by @sherry13131, but rebased on top of today's master.

In addition I have:

• included some small tweaks to the style tags to bring it into line with other changes made to the same block.
• cleaned up trailing whitespace on introduced code.
• fixed a latent bug in the verification code when adding a new URL.

The original code allowed any text containing any URL to be added, which I'm pretty sure would result in entries that would never be matched, so I added my own commit that trims any provided URL so that they are just http(s)://dom.ain (with any username, password, port, path, query string and/or anchor removed).

Because I've rebased the code, a number of merge commits in the original PR have vanished, but they were purely between internal development branches in the same repo, not part of the main PR.

[robsonsobral]

Somebody please review and merge this! PLEEEEEEEEEEEEEEEEEEASE!

[phy1729]

I'm pretty sure normalization isn't necessary. Assuming the below call to Logic.setOrRemoveAssignment should be Utils.setOrRemoveAssignment,

• Utils.setOrRemoveAssignment send a message which

  multi-account-containers/src/js/utils.js

  Lines 93 to 99 in 50f5ebf

  	return browser.runtime.sendMessage({
  	method: "setOrRemoveAssignment",
  	tabId,
  	url,
  	userContextId,
  	value
  	});

• the anonymous function in messageHandler.init handles by calling

  multi-account-containers/src/js/background/messageHandler.js

  Line 49 in 50f5ebf

  response = assignManager._setOrRemoveAssignment(m.tabId, m.url, m.userContextId, m.value);

• assignManager._setOrRemoveAssignment which calls

  multi-account-containers/src/js/background/assignManager.js

  Lines 560 to 563 in 50f5ebf

  	await this.storageArea.set(pageUrl, {
  	userContextId,
  	neverAsk: false
  	}, exemptedTabIds);

• storageArea.set which filters the URL through

  multi-account-containers/src/js/background/assignManager.js

  Line 73 in 50f5ebf

  const siteStoreKey = this.getSiteStoreKey(pageUrlorUrlKey);

• storageArea.getSiteStoreKey which extracts the hostname and possibly the port from the URL

  multi-account-containers/src/js/background/assignManager.js

  Lines 14 to 20 in 50f5ebf

  	const url = new window.URL(pageUrlorUrlKey);
  	const storagePrefix = "siteContainerMap@@_";
  	if (url.port === "80" || url.port === "443") {
  	return `${storagePrefix}${url.hostname}`;
  	} else {
  	return `${storagePrefix}${url.hostname}${url.port}`;
  	}

[kurahaupo]

I'm fairly new to this, so any help you can give - perhaps a PR into my patch - would be very much appreciated.

[kurahaupo]

Isn't the point of normalizeUrl to trim the path off? Given that it's random user input, this seems like a fairly sensible idea.

It looks like you're right about the Utils.setOrRemoveAssignment since it's only defined in src/js/utils.js, so I've committed that change too.

[ryanseddon]

Can we get this moving again? Can I help? I have a lot of containers setup and I can't log into or use a lot of websites that do quick redirects at certain stages.

[phy1729]

While it'd be preferable to have a proper solution like this PR, in the mean time I've resorted to generating javascript to paste into the extension's console. I've documented the steps at https://github.com/phy1729/container-config in case that's useful for you too.

[ryanseddon]

This is extremely helpful, thanks for sharing

[dvd0101]

@phy1729 thank you for your hack! I use your code in this function (hope it can help others):

// open: about:devtools-toolbox?id=%40testpilot-containers&type=extension
//
// adapted from: https://github.com/phy1729/container-config
async function addUrl(container, url) {
  const identities = await browser.contextualIdentities.query({ name: container });
  if (identities.length > 1) throw new Error("too many container with the same name");
  if (identities.length == 0) throw new Error("container not found");

  const cookieStoreId = identities[0].cookieStoreId;
  const userContextId = backgroundLogic.getUserContextIdFromCookieStoreId(cookieStoreId);
  console.log("user context: ", userContextId);

  const assignManager = window.assignManager;

  await assignManager._setOrRemoveAssignment(false, url, userContextId, false);
}

To use it:

1. open about:devtools-toolbox?id=%40testpilot-containers&type=extension
2. paste and run the function in the console panel
3. still in the console panel call the function with the name of the container and the url to add, for example: addUrl("search", "https://domain.to.add")

[dvd0101]

@kurahaupo Tested right now and the "browser" symbol is visible (FF 96b10 and the mac extension up-to-date).

Are you sure you have opened the about:devtools-toolbox?id=%40testpilot-containers&type=extension url and pasted the code in the console of that page (not in the inspector console)?

[wolfferine]

Please implement this feature soon - we're dying for it :-)
btw the title "Always Open This Site in..." is not correct (imho)
cause most ppl will assume it will always open a link/url
in a specific container, which is not the case!

Because:
https://outlook.live.com/owa/?username=me@ive.com
is different compared to
https://outlook.live.com/owa/?username=you@ive.com

[swiffer]

LGTM

[TriMoon]

@mozilla @ctbmozilla-admin @dannycolin PLEASE slap anyone needed to get this merged already, what's holding this off for so many years?

[kurahaupo]

Rebased to current main again. (It was a clean rebase; there were no conflicting changes.)

[dannycolin]

I know we used Utilss.escaped somewhere else in the code but we actually wanted to remove it in favor of createElement, appendChild, textContent and whatever else that is considered safe. Plus, it always trigger the linter and AMO safety check because of innerHTML.

[TriMoon]

Oh man THAT looks very prehistoric indeed... 🤦‍♀️
But lets concetrate on getting the functionality merged in first, after that another PR can fix the innerHTML mess, unless this PR is the one who introduces them 😉

*edit:
I just saw it is indeed this PR that introduces them...
@kurahaupo please re-code the innerHTML mess 😉

[dannycolin]

PLEASE slap anyone needed to get this merged already, what's holding this off for so many years?

@TriMoon It won't get things to move faster. I'm pretty much the only person who contributed to anything containers lately. However, I'm not a paid staff and lack the free time to do a good review at the moment.

I can't promise anything but my University semester ends in ~1 month. Hopefully, I'll have time to look at it more closely.

[TriMoon]

@dannycolin i understand but the rest of us are unable to merge even if we would create PR's, so it's up to the ones with permissions to do it...

[Bazoogle]

I can't promise anything but my University semester ends in ~1 month. Hopefully, I'll have time to look at it more closely.

Hope you had a good holiday! I know you didn't promise anything, but I'm curious if this is still on your to-do list. Totally understandable if you have more pressing things to address, but I wanted to let you guys know there's still those of us who would love this feature! Thank you for your contributions, regardless of if you're able to do this or not. Your voluntary contributions have helped many

[Snake883]

Website redirects are causing me a lot of problems (live.com, outlook.com, microsoft.com, etc).
Being able to manually add websites/URL's to the container site list would presumably solve the problem of redirects going in and out of containers.