multi-account-containers

multi-account-containers copied to clipboard

See pull request https://github.com/mozilla/multi-account-containers/pull/1500 and issue https://github.com/mozilla/multi-account-containers/issues/473

This is a second attempt at implementing a wildcard subdomains feature. This time round, I have tried to keep the amount of code that is changing to the bare minimum. This should make code review easier and minimise any risk of introducing regressions. We can potentially cancel the previous pull request if this one looks better.

https://user-images.githubusercontent.com/439593/166697696-78b3d20d-4032-41ac-b030-e017d36a28cb.mov

How the code works

When you click the www part of www.google.com in the popup, a wildcardHostname property with value google.com is added to the stored siteSettings object for www.google.com in assignManager.js See code here

If you then open a tab and navigate to mail.google.com, we need the code to find the matching google.com wildcard hostname stored in the www.google.com site. This is achieved by storing an additional map of wildcardHostname values to siteStoreKeys. See code here

So when handling the request for mail.google.com, the wildcard hostname map is queried to see if there is a mapping for mail.google.com, then google.com (and then theoretically com). It finds that google.com is mapped to www.google.com, and so uses the www.google.com site settings to handle the request. See code here

Syncing

The sync feature should just work as expected, since the new wildcardHostname property of siteSettings will be synced along with all the other properties of the siteSettings object.

Try it out

I have put up a temporary build here for you to try out the feature without having to build it yourself. Let me know if it looks good or if you find any issues.

I have also included unit tests in this pull request.

@mckenfra I can't thank you a 1000 times for your work!! I have tested it works as needed. Thank you so much!!

@groovecoder can you please review and accept this PR? It is working great.

Will this work with multi-level subdomains?

I wonder if it will do what I've been searching for.

I use these containers for local, staging and live sites. Mainly cause they have an elegant line on the tabs.

But I always have to select. "Always open site in..." so it remembers the specific client site.

Does the following work: Automatically open in the right container staging container: any-client.staging.main-domain.com live container: any-client.main-domain.com local container: any-client.localhost:1234 (whatever port)

@krisnrg It should work for your use case, except for the port. I haven't added any support for having a "wildcard" port. Would you want that?

Maybe you can try installing the proof-of-concept "wildcard subdomains" build and see if it does what you need. You can download it here

No wildcard is needed for the ports no. For me, it's always the same port. This is a God-sent!

I happened to come here today to see if the code was something I can edit and modify for my needs. Little did I know there would be a PR today lol

Thanks for the hard work.

I'll prolly try it today after I get free.

First of all, @mckenfra thanks a lot for reviving this idea. I'd definitely like to see this feature implemented in MAC. However, I have a few UX concerns over this implementation.

1. It isn't clear what happens by clicking on the www for all users depending on their technical knowledge about domains.
2. Using only colors to indicate the state is a big no no in term of web accessibility.

For 1, you could explore a solution that use a tooltip when you hover over the www. We could also have a short message at the top of the site list that is only shown if the user never used the wildcard feature. Something like, "Hey did you know you could ...," with a button that opens the documentation with a more in-depth how-to.

For 2, we could change the www part to * or [all] that'd be even more clear for those who don't know the meaning of the asterix.

Totally liking that 2e part, will make a lot more sense! love to see this merged with the changes purposed by @dannycolin

I support:

For 2, we could change the www part to * or [all] that'd be even more clear for those who don't know the meaning of the asterix.

But for 1, I see personally no need, because I thing that most people who are aware of privacy do have technical knowledge about domains.

But for 1, I see personally no need, because I thing that most people who are aware of privacy do have technical knowledge about domains.

For Mozilla, privacy is for everyone. It's at the core of Mozilla's manifesto. Empowering users, whatever their level of knowledge, is something we should strive to especially when it's very easy to implement like the proposition I made. It's even make more sense to do that small extra step since it's an addon managed by Mozilla itself. If it was a third-party addon, I'd be less concerned.

@dannycolin Thanks a lot for your feedback 👍 Regarding your suggestion no. 2, see video below. Is this better?

https://user-images.githubusercontent.com/439593/168251279-e1bfdecb-e63d-41c4-8723-dc7836a0acec.mov

I would more do it with a . in between too: *.google.com And perhaps make it not red that suggest something bad is happening

@bruvv In the current implementation of this pull request, *.google.com will match both www.google.com and google.com (i.e. without any leading dot or subdomain).

It will not match www.notgoogle.com or notgoogle.com of course!

Is that the behaviour you would expect?

this suggest me that anything can be matched before google.co.uk so I think people will mistake it and can be lead to confusing. Will it also match thisisgoogle.co.uk and or thisisnotgoogle.co.uk? if you add a . (dot) before the main domain it suggest that only subdomains will be matched like www.google.co.uk and or mail.google.co.uk etc.

so it should be *.google.co.uk so the implementation is perfect, the visualization is what I am talking about.

@mckenfra to respond on your last question i like hat behavior. For instance when you have https://gab.com and hhtps://media.gab.com only one rule is needed.

@bruvv

Will it also match thisisgoogle.co.uk and or thisisnotgoogle.co.uk?

No

@bruvv Yes I take your point about adding the leading dot. Yes the behaviour is that *.google.co.uk will also match google.co.uk, but not thisisgoogle.co.uk or thisisnotgoogle.co.uk.

And perhaps make it not red that suggest something bad is happening

@bruvv The red colour was used so that it stands out clearly, in case people click the subdomain accidentally... but I will be happy to change this to a different colour if you can suggest one? Maybe blue? Or the same colour as the container icon for that particular container?

I was talking about the visualization not the functionalist. Functional it is perfect, visually it is just missing the . and I agree same colour as the container icon is perfect!

See below a screencast showing the following changes:

1. extra dot after star icon
2. star icon background colour matches container icon colour

(I've edited the video to remove the steps where I changed the container icon colour each time)

https://user-images.githubusercontent.com/439593/168267449-42e53774-fb6b-42a9-b6e3-a4fa0d548de7.mov

@mckenfra awesome! I love it. Now hopefully this can be merged.

@bruvv Great! If there is general consensus on the above design, I'll update the PR to include these UI changes.

@mckenfra the last design is even better then your first proposal. I agree with it.

Hopefully @dannycolin agrees too (shameless ping :P )

Hopefully @dannycolin agrees too (shameless ping :P )

I'm only a contributor so I'm definitely not the one making the final decision here. However, since I'm working on MAC, I can always help to push this forward.

@mckenfra I got a patch merged last week that updates the UI to the new Firefox Design System and my goal is to continue to make it more in-line with it. So to make sure we don't break each other work, do you mind if I try to mockup something in Figma for the UX/UI part of this patch and ping you back later this week? If you have questions or want to brainstorm a bit more on this patch, feel free to also ping me on our Matrix server at https://chat.mozilla.org in #containers:mozilla.org. If you never used Matrix, there's a quick how-to on our Wiki at https://wiki.mozilla.org/Matrix.

In the meantime, you can try rebasing your branch on main if you wish to try the shiny new UI :).

@dannycolin Thanks a lot! Yes I already rebased this PR a few hours ago to incorporate the recently-merged changes to the main branch. Your design changes to MAC look great, well done! 👍

I have put out a new temporary release of this PR (including your MAC UI updates and my new design for this PR) for people to try out here

Thanks a lot for offering to work on an improved UX/UI for this PR. I'll happily include any improvements/changes!

@bakulf can you review this?

Hey folks,

Since a lot of you have pinged the developers for a review, I just wanted to give you a quick updates on why it's taking more time then initially planned to get this reviewed and merged.

Firstl, we're working on a fast and robust way to detect the domain TLDs based on the publicsuffixlist. This takes more time because:

1. we need a way that makes sure we keep in sync with the upstream list to avoid any mismatches that could have confusing consequences in terms of where cookies exist and/or get shared.
2. the solutions we're exploring may need WASM or directly talking to the Firefox codebase

I won't go more into details but I'm sure you can appreciate that this solutions aren't simple as copy/pasting code in the repo :).

Second, we needed to work on the UX of this feature to make sure it's as easy as possible to use and so to limit the chance a user shoot themselve in the foot by setting it wrong because the UI was confusing. I'm sharing below a screenshot of what it could look like.

Keep in mind that it may not cover all the use cases. This is done on purpose. We will most likely implement a basic feature that do something like "Enable everything or nothing from domain X". This doesn't mean that more advanced features won't be implemented but simply that we prefer to do it incrementally so it has more chances the maintainers of this addon will accept to merge it.

@mckenfra Just a quick question. Currently, when using your code when I add gab.com (without www. in front of it) to a container and then go to media.gab.com the subdomain isn't assigned. So I have always to make double rules for one website. (gab.com and ★.gab.com) In may opinion this is really annoying. How can I change your code to change that behaviour?

I think it has something to do with the following code in assignManager.js:

    getWildcardStoreKey(wildcardHostname) {
      return `wildcardMap@@_${wildcardHostname}`;
    },

    getWildcardStoreKeys(siteStoreKey) {
      // E.g. "siteContainerMap@@_www.mozilla.org" =>
      // ["wildcardMap@@_www.mozilla.org", "wildcardMap@@_mozilla.org", "wildcardMap@@_org"]
      let previous;
      return siteStoreKey.replace(/^siteContainerMap@@_/, "")
        .split(".")
        .reverse()
        .map((subdomain) => previous = previous ? `${subdomain}.${previous}` : subdomain)
        .map((hostname) => this.getWildcardStoreKey(hostname))
        .reverse();
    },

@BPower0036 You should only need to add media.gab.com and then click the media part to change it to a wildcard. That should then automatically also handle gab.com without having to explicitly add it.

@mckenfra Thanks for that! It works! I expected wrongfully to work the other way around, which is actually not possible.