Exclude all files and directories from iCloud backup

[Anderas]

Fixes element-hq/element-ios#5498

As listed in documentation, the iOS is automatically backing up a number of folders to iCloud / iTunes, including <AppHome>/Documents/ and <AppHome>/Library/Application Support/. Additionally the Matrix SDK stores files in AppGroup containers (potentially shared between app and an extension), which are also automatically backed up to iCloud. These folders include data for rooms, spaces etc.

To opt-out of backups completely, each url needs to be explicitely marked with isExcludedFromBackupKey (this is applied recursively). I converted all createDirectory calls with a custom method, which automatically does this when creating a new directory.

This does not yet address smaller files (e.g. User Defaults preferences only created on save) or any future folders, which might be created without the backup exclusion. To solve for this second problem I added two functions excludeAllUserDirectoriesFromBackup and excludeAllAppGroupDirectoriesFromBackup, which need to be called by the main application (e.g. in AppDelegate) and exclude all top-level directories.

Arguably this second approach might make it redundant to explicitly specify backup choice when creating a new folder, however the blanker exclusion is only called at specific moments (e.g. app start), which may be before a particular folder is created. This would expose the folder to backups until the next app launch.

[Anderas]

I considered an alternative api along the lines of createDirectory(at:withIntermediate:attributes:isExcluded:) but we just pass the same parameters everywhere anyway, so it felt cumbersome and pointless.

[stefanceriu]

This makes sense to me.
I would change it's signature slightly though, to better match its path counterpart: createDirectoryExcludedFromBackup(atURL url: URL)

[Anderas]

This api distinction already exists within FileManager's API ((at url: URL) vs (atPath path: String)). Reading swift conventions I assume that (at url: URL) is preferable, but then (at path: String) could not be easily distinguished / would be overloading. Since URL is preferable type over String path, the url-api is given preferential treatment. I don't mind aligning them with your suggestion, just wanted to point out that this was a deliberate choice given the existing FileManager api.

[stefanceriu]

If it were my choice I would make them symmetrical but no strong feelings really.

[Anderas]

Yea after I changed the excludeItemFromBackup method, it seems preferable to stick with (at url: URL) everywhere for consistency sake.

[stefanceriu]

This all looks very reasonable to me, the only thing I'm worried about is this line from the documentation: Set the key each time you save the file because certain common file operations clear the key.. I don't think we have control over all the saves.

We have quite a lot of folder within the application group container. Do we really need all of those shared with the other processes? 🤔

Also, have you checked how the integration tests behave with these changes? (MXBackgroundSyncServiceTests, MXSessionTests, MXCoreDataRoomListDataManagerUnitTests etc.)

[stefanceriu]

I would rename this to something like excludeURLFromBackup(_ url: URL) to be more in line with the others.

[Anderas]

Looking at FileManager docs, I think an even better name would be excludeItemFromBackup(at url: URL) which aligns with copyItem(at url: URL) or moveItem(at url: URL). WIll make that change 👍

[stefanceriu]

This makes sense to me.
I would change it's signature slightly though, to better match its path counterpart: createDirectoryExcludedFromBackup(atURL url: URL)

[Anderas]

This all looks very reasonable to me, the only thing I'm worried about is this line from the documentation: Set the key each time you save the file because certain common file operations clear the key.. I don't think we have control over all the saves.

Yea this is true, though the way I understand it, it would mostly be an issue if we were setting the attribute on specific files/folders that are often updated and saved. Instead we are either marking top-level folders that we do not directly create (Documents, Library), or use the createDirectoryExcludedFromBackup to always pair mutation/creation with setting the attribute. And if that fails, we'll be calling the blanket exclusion from app delegate.

We have quite a lot of folder within the application group container. Do we really need all of those shared with the other processes? 🤔

Very good question. I don't fully understand the integration with extensions yet, and would probably be a larger change, but worth taking that as the next logical step.

Also, have you checked how the integration tests behave with these changes? (MXBackgroundSyncServiceTests, MXSessionTests, MXCoreDataRoomListDataManagerUnitTests etc.)

I have some issues running tests in general, even on develop, so need to debug that first.

[Anderas]

Also, have you checked how the integration tests behave with these changes? (MXBackgroundSyncServiceTests, MXSessionTests, MXCoreDataRoomListDataManagerUnitTests etc.)

I have some issues running tests in general, even on develop, so need to debug that first.

So the integration tests (particularly MXSessionTests) seem quite flaky on develop, so in a future PR I can try to fix some of them, but I observed no difference in the failures between this PR and develop, so supposedly no regressions.

[SBiOSoftWhare]

Good job, this is well documented. Btw you can probably use MXLog.error instead of MXLog.debug for try/catch errors.

[stefanceriu]

Looks good, let's roll with it 👍

[Anderas]

Good job, this is well documented. Btw you can probably use MXLog.error instead of MXLog.debug for try/catch errors.

Ah yes, I thought I had. Thanks.