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

Backup/restore settings to/from a file #1533

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

Conversation

Minigugus
Copy link

@Minigugus Minigugus commented Oct 13, 2019

Fix #1282
Fix #1409
Fix #1427
Fix #1455
#1236

This PR is not a fix for the bug that reset containers list, it just add buttons in the Options UI of the extension in about:addons to backup and restore the containers list.

Please note that currently, only containers' colors, icons, names and site associations are saved, not cookies, which means containers' sessions are erased when importing a backup.

Moreover, this PR allows to configure containers list with JSON directly, and to share configurations across browsers easily.

All tests passed via npm run test.

EDIT 1: Now, backup and restore site associations 😉
EDIT 2: Localization messages available here: mozilla-l10n/multi-account-containers-l10n#11
EDIT 3: Add Limit to designate websites checkbox status to backup files

@renesy
Copy link

renesy commented Oct 31, 2019

Does this also backup the association of sites to always open in a certain container?

@Minigugus
Copy link
Author

Minigugus commented Oct 31, 2019

Currently, no, but seems possible. Could be useful, I'm starting working on it Added 😉

@muhlinux
Copy link

muhlinux commented Feb 2, 2020

@stoically @jonathanKingston
Please merge this

@Minigugus
Copy link
Author

@groovecoder Up ?

@kineita
Copy link

kineita commented Jun 3, 2020

Still waiting Firefox team merge this feature :(

@rasmus-z
Copy link

Please merge this feature! I've had to reinstall OSes etc and it's always a bummer to have to redo my multi-account containers (esp. the first time, since I just assumed it would get synced via my Firefox account). Which means that initially I tend not to use them as well as before, so I'm leaking privacy etc :/

@kocjs
Copy link

kocjs commented Feb 14, 2021

I am looking for this too, please proceed

@muhlinux
Copy link

Any news on this? Seems like a much wanted feature #1282 #1409 #1427 #1455

@qwexvf
Copy link

qwexvf commented Jan 14, 2022

Any update on this?

@flodolo
Copy link
Collaborator

flodolo commented Jan 14, 2022

Sorry, too many open tabs 🤦🏼

I meant to close the PR in the l10n repository, not the one in the code repository. Please open a PR to add strings only after the feature has been reviewed and content agreed upon (all linked issue are really old).

@qwexvf
Copy link

qwexvf commented Jan 20, 2022

Sorry, too many open tabs 🤦🏼

I meant to close the PR in the l10n repository, not the one in the code repository. Please open a PR to add strings only after the feature has been reviewed and content agreed upon (all linked issue are really old).

I see! ill hold on for the op to see if he is interested to continue working on this PR. if not ill try to implement strings!
Thanks for the reply <3 !

@groovecoder groovecoder requested review from bakulf and removed request for groovecoder May 2, 2022 14:16
@Nomes77
Copy link

Nomes77 commented May 18, 2022

Since a see that @bakulf is assigned to review. It would be very pleasant for us all to implement this PR.

@Nomes77
Copy link

Nomes77 commented Jun 4, 2022

Can you @Minigugus also add an option to back-up whether a container is restricted to a certain websites?

@Minigugus
Copy link
Author

Can you @Minigugus also add an option to back-up whether a container is restricted to a certain websites?

Do you mean sites associations? I'm not using this extension so I'm not aware of new features 😅

Currently sites associations are already included in the backup: https://github.com/mozilla/multi-account-containers/pull/1533/files#diff-a3f1b60bcaa9aa659bc7e5bd50fa5109ec7313b7bb363cdceeb3c70a67808ba5R273-R278

@Nomes77
Copy link

Nomes77 commented Jun 7, 2022

No, I mean this setting Limit to designate websites, with my red circle around it -->
afbeelding

@Minigugus
Copy link
Author

Minigugus commented Jun 12, 2022

No, I mean this setting Limit to designate websites, with my red circle around it

@BPower0036 Added 👍 (not tested though)

@Minigugus Minigugus changed the title Backup & Restore containers configuration Add buttons to export/import containers configuration from a file Jun 12, 2022
@Nomes77
Copy link

Nomes77 commented Jun 13, 2022

No, I mean this setting Limit to designate websites, with my red circle around it

@BPower0036 Added 👍 (not tested though)

I will test it when I have time, thanks!

@Nomes77
Copy link

Nomes77 commented Jun 15, 2022

I have tested it, by implementing it, see MAC-wildcard.
Exporting works great. However when you want to restore and you have isolated container it restores them, but (and there is the problem) when you open the popup, you get a blank one. Looking into the console gives the following error:
afbeelding
with the following code:

/* proxified-containers.js */
  // Deletes the proxy information object for a specified cookieStoreId [useful for cleaning]
  async delete(cookieStoreId) {
    // Assumes proxy is a properly formatted object
    const proxifiedContainersStore = await proxifiedContainers.retrieveAll();
    const index = proxifiedContainersStore.findIndex(i => i.cookieStoreId === cookieStoreId);
    if (index !== -1) {
      proxifiedContainersStore.splice(index, 1);
    }
    await browser.storage.local.set({
      proxifiedContainersKey: proxifiedContainersStore
    });
  }

/* backgroundLogic.js */
  async deleteContainer(userContextId, removed = false) {
    await this._closeTabs(userContextId);

    if (!removed) {
      await browser.contextualIdentities.remove(this.cookieStoreId(userContextId));
    }

    assignManager.deleteContainer(userContextId);

    // Now remove the identity->proxy association in proxifiedContainers also
    proxifiedContainers.delete(this.cookieStoreId(userContextId));

    return {done: true, userContextId};
  },

/* messageHandles.js */
    if (browser.contextualIdentities.onRemoved) {
      browser.contextualIdentities.onRemoved.addListener(({contextualIdentity}) => {
        const userContextId = backgroundLogic.getUserContextIdFromCookieStoreId(contextualIdentity.cookieStoreId);
        backgroundLogic.deleteContainer(userContextId, true);
      });
    }

const content = JSON.stringify(
await browser.runtime.sendMessage({
method: "backupIdentitiesState"
})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty-printing the JSON file would make it a lot easier to read and make diffs, etc.

const content = JSON.stringify(
  await browser.runtime.sendMessage({
    method: "backupIdentitiesState"
  }),
  null,
  2
);

@Minigugus Minigugus changed the title Add buttons to export/import containers configuration from a file Backup/restore settings to/from a file Oct 12, 2022
@andrasfuchs
Copy link

@bakulf, could you take a look, please? The backup/restore feature would be a great addition!

@andrasfuchs
Copy link

@aaronkollasch, @dannycolin It looks like @bakulf doesn't have the capacity to look into this. Could someone else take over this PR to give it a chance to be merged?

@dannycolin
Copy link
Collaborator

@aaronkollasch, @dannycolin It looks like @bakulf doesn't have the capacity to look into this. Could someone else take over this PR to give it a chance to be merged?

I removed the reviewer because they aren't actively working on this project at the moment. I'd really like to see this feature merged but currently I don't think there's anyone with the bandwidth. This isn't a trivial patch to review.

First, if there are merge conflicts on import, we can't simply abort it. Users would see this as a bug and we're going to get a tons of angry customers, here, on GitHub. If we decide to offer a way to force the merge, we also need a way to revert back. If not, well again we'll have a bunch of angry customers knocking at the door.

Second, if Sync kicks in at the wrong time, it could overwrite the imported data thinking the remote one is still newer. At first glance, the patch doesn't seem to deal with that issue.

Finally, this absolutely needs unit tests to even have a chance to be considered as a mergeable patch since there's a major chance of data loss.

@ArthurKun21
Copy link

Second, if Sync kicks in at the wrong time, it could overwrite the imported data thinking the remote one is still newer. At first glance, the patch doesn't seem to deal with that issue.
Finally, this absolutely needs unit tests to even have a chance to be considered as a mergeable patch since there's a major chance of data loss.

This is the main reason I turn off sync last time, all of my container data is gone I have to rebuild it from scratch as well as turn off the sync again. If we could manually backup the data it would be great!

Nomes77 added a commit to Nomes77/multi-account-containers that referenced this pull request Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet