Skip to content
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 allowlist for isolated containers #2033

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ncallaway
Copy link

@ncallaway ncallaway commented Apr 25, 2021

This implements a per-container allow-list that is applied to isolated
containers. This can allow multiple containers to have an overlapping set of
sites that they are isolated to.

For example: Container A and Container B could both be isolated and have
calendar.example.com in their allow list. Both Container A and Container B
would then be able to visit calendar.example.com.

See: #1892 and #1887 for additional motivation.

Changes

  • Adds a per container allow list to the container storage
  • Shows the allowed sites in the "Manage Site List" panel
    • This is conditional on the container being isolated
    • Users to delete sites from the allowed sites in the "Manage Site List Panel"
    • Users can manually add a site to this panel
  • Adds an "Allow Opening This Site in..." panel to the main popup
    • This is conditional on at least one container being isolated
    • This displays the list of isolated containers, and when the user selects a container, the site is added to that containers allow list
    • This panel needs an icon made.
  • Updates the onBeforeRequest logic to account for the container's allow list
  • Adds a new icon container-allowin-16

Screenshots

Always Open This Site in... screenshot
Screen Shot 2021-04-25 at 1 31 16 PM Screen Shot 2021-04-25 at 1 31 25 PM

Updated Manage Site List screenshot
Screen Shot 2021-04-25 at 1 34 22 PM

Icon
Screen Shot 2021-04-27 at 8 05 38 PM

Demo

demo.mp4

Draft

This is a draft until an icon has been made for the "Allow Opening This Site in...". I'll work on creating that icon later this week.

License

I agree to license this code under the project's open source license (MPL 2.0). The new image (container-allowin-16) was authored by me and I agree to also license under MPL 2.0

This implements a per-container allow-list that is applied to isolated
containers. This can allow multiple containers to have an overlapping set of
sites that they are isolated to.

For example: Container A and Container B could both be isolated and have
calendar.example.com in their allow list. Both Container A and Container B
would then be able to visit calendar.example.com.

See: mozilla#1892 and
mozilla#1887 for additional
motivation.
@ncallaway ncallaway marked this pull request as ready for review April 28, 2021 03:08
@mikeyhew
Copy link

This looks like exactly what I need in order to use different containers for different google products:

  • Youtube container: youtube.com always opens in it, accounts.google.com is allowed to open in it, but nothing else
  • Gmail container: mail.google.com and gmail.com always open in it, accounts.google.com is allowed to open in it, but nothing else

@ncallaway
Copy link
Author

@mikeyhew You can pull the source from this PR and load it as a temporary extension https://extensionworkshop.com/documentation/develop/temporary-installation-in-firefox/ to test the UX and provide feedback on whether this works for your use-case.

I would imagine it would, because I use something similar (I have a container for each separate Google Account, but that has a similar need to overlap for certain domains like accounts.google.com)

@mikeyhew
Copy link

@ncallaway thanks, I installed it temporarily and it worked, and now I've installed it more permanently to use during my day-to-day. It's helpful that you can add sites manually, because sometimes it redirects away from the site you want to add before you can add it. I wish there was a way to add "always open in" sites for containers manually too, instead of having to visit the site first.

I have noticed an issue though where sometimes it doesn't show any of the UI you added, and sometimes it does. Initially I thought it somehow had installed the unmodified version of the app, but then all of a sudden the allowed sites section showed up.

One case where it pretty consistently doesn't show up is when there are no sites assigned to the container yet - when I click on "Manage Site List" link on the page where you edit the container, I get this:

Screen Shot 2021-05-14 at 12 21 41 AM

Of course, I said that it pretty consistently does that, but I tried it again just now and for the first time, it did show the UI you added even though there were no sites added to the container yet.

@gpoole
Copy link

gpoole commented Jun 5, 2021

Thanks for this @ncallaway, sorry for the slow response. I'm running this now and it's exactly what I was looking for with #1892, so that's perfect. This change really makes MAC much more usable for me.

I would agree with @mikeyhew that my first impression is it was a little tricky trying to set up sites with redirects. In my case I found it tricky adding gmail, since I usually type gmail.com which redirects to mail.google.com and then (for a fresh container) to accounts.google.com, so I had to keep an eye on the address bar and add all those to make it work. I did get it set up and working how I wanted without too much hassle though.

I'm going to run this for a while and I'll hopefully have some better feedback.

@gpoole
Copy link

gpoole commented Jul 24, 2021

This has been working great for me over the last couple of months. The only feedback I have is there are a few spots where the UI looks unstyled and as I mentioned it can be tricky to add pages that redirect, which makes it a little bit of extra work for using with e.g. Google products. Otherwise it's exactly what I wanted from MAC.

@gpoole
Copy link

gpoole commented Nov 23, 2021

@ncallaway this has been working great for me and I'd love to see it merged. Not really familiar with the review process here, nobody seems to have dropped in to review it. Do you know if we need to raise it with someone?

@ncallaway
Copy link
Author

@gpoole I'd love to get it merged in as well. I'm not sure exactly what the review and approval process is for contributions. I'm happy to take on the work remaining to get this ready to merge (resolving conflicts, addressing any outstanding bugs like that reported by @mikeyhew, etc). I just want to make sure that it's on a path to being accepted and merged before I finish it up.

Do you know who the appropriate reviewers are that can approve merging something in? I'd love to get a final set of feedback and issues to resolve before picking this up and finishing it.

@SeanDunford
Copy link

Would love to see this get merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow domain in multiple containers
4 participants