From 6cd5db91d6bbf20e1f482ae20d7de4c763286c68 Mon Sep 17 00:00:00 2001 From: Kit Cambridge Date: Wed, 9 Nov 2016 14:23:54 -0800 Subject: [PATCH] Bug 1303679 - Remove Places roots, reading list items, and pinned sites from the Sync server. r=markh MozReview-Commit-ID: AhOBOnXsTYi --HG-- extra : rebase_source : 1f39b406b1a9215ca04ff4602cc8fac54163916f --- services/sync/modules/engines.js | 13 +++++ services/sync/modules/engines/bookmarks.js | 19 +++++++ .../sync/tests/unit/test_bookmark_engine.js | 57 +++++++++++++++++++ 3 files changed, 89 insertions(+) diff --git a/services/sync/modules/engines.js b/services/sync/modules/engines.js index b62107e85a53..48389554f659 100644 --- a/services/sync/modules/engines.js +++ b/services/sync/modules/engines.js @@ -1123,6 +1123,12 @@ SyncEngine.prototype = { return; } + if (self._shouldDeleteRemotely(item)) { + self._log.trace("Deleting item from server without applying", item); + self._deleteId(item.id); + return; + } + let shouldApply; try { shouldApply = self._reconcile(item); @@ -1260,6 +1266,13 @@ SyncEngine.prototype = { Observers.notify("weave:engine:sync:applied", count, this.name); }, + // Indicates whether an incoming item should be deleted from the server at + // the end of the sync. Engines can override this method to clean up records + // that shouldn't be on the server. + _shouldDeleteRemotely(remoteItem) { + return false; + }, + _noteApplyFailure: function () { // here would be a good place to record telemetry... }, diff --git a/services/sync/modules/engines/bookmarks.js b/services/sync/modules/engines/bookmarks.js index ebd31d4718d0..04a598520ed4 100644 --- a/services/sync/modules/engines/bookmarks.js +++ b/services/sync/modules/engines/bookmarks.js @@ -46,6 +46,17 @@ const ORGANIZERQUERY_ANNO = "PlacesOrganizer/OrganizerQuery"; const ALLBOOKMARKS_ANNO = "AllBookmarks"; const MOBILE_ANNO = "MobileBookmarks"; +// Roots that should be deleted from the server, instead of applied locally. +// This matches `AndroidBrowserBookmarksRepositorySession::forbiddenGUID`, +// but allows tags because we don't want to reparent tag folders or tag items +// to "unfiled". +const FORBIDDEN_INCOMING_IDS = ["pinned", "places", "readinglist"]; + +// Items with these parents should be deleted from the server. We allow +// children of the Places root, to avoid orphaning left pane queries and other +// descendants of custom roots. +const FORBIDDEN_INCOMING_PARENT_IDS = ["pinned", "readinglist"]; + // The tracker ignores changes made by bookmark import and restore, and // changes made by Sync. We don't need to exclude `SOURCE_IMPORT`, but both // import and restore fire `bookmarks-restore-*` observer notifications, and @@ -706,6 +717,14 @@ BookmarksEngine.prototype = { // Simply adding this (now non-existing) ID to the tracker is enough. this._modified.set(localDupeGUID, { modified: now, deleted: true }); }, + + // Cleans up the Places root, reading list items (ignored in bug 762118, + // removed in bug 1155684), and pinned sites. + _shouldDeleteRemotely(incomingItem) { + return FORBIDDEN_INCOMING_IDS.includes(incomingItem.id) || + FORBIDDEN_INCOMING_PARENT_IDS.includes(incomingItem.parentid); + }, + getValidator() { return new BookmarkValidator(); } diff --git a/services/sync/tests/unit/test_bookmark_engine.js b/services/sync/tests/unit/test_bookmark_engine.js index 9de6c5c0dc09..498de1dc85c8 100644 --- a/services/sync/tests/unit/test_bookmark_engine.js +++ b/services/sync/tests/unit/test_bookmark_engine.js @@ -23,6 +23,63 @@ function* assertChildGuids(folderGuid, expectedChildGuids, message) { deepEqual(childGuids, expectedChildGuids, message); } +add_task(function* test_delete_invalid_roots_from_server() { + _("Ensure that we delete the Places and Reading List roots from the server."); + + let engine = new BookmarksEngine(Service); + let store = engine._store; + let tracker = engine._tracker; + let server = serverForFoo(engine); + new SyncTestingInfrastructure(server.server); + + let collection = server.user("foo").collection("bookmarks"); + + Svc.Obs.notify("weave:engine:start-tracking"); + + try { + collection.insert("places", encryptPayload(store.createRecord("places").cleartext)); + + let listBmk = new Bookmark("bookmarks", Utils.makeGUID()); + listBmk.bmkUri = "https://example.com"; + listBmk.title = "Example reading list entry"; + listBmk.parentName = "Reading List"; + listBmk.parentid = "readinglist"; + collection.insert(listBmk.id, encryptPayload(listBmk.cleartext)); + + let readingList = new BookmarkFolder("bookmarks", "readinglist"); + readingList.title = "Reading List"; + readingList.children = [listBmk.id]; + readingList.parentName = ""; + readingList.parentid = "places"; + collection.insert("readinglist", encryptPayload(readingList.cleartext)); + + let newBmk = new Bookmark("bookmarks", Utils.makeGUID()); + newBmk.bmkUri = "http://getfirefox.com"; + newBmk.title = "Get Firefox!"; + newBmk.parentName = "Bookmarks Toolbar"; + newBmk.parentid = "toolbar"; + collection.insert(newBmk.id, encryptPayload(newBmk.cleartext)); + + deepEqual(collection.keys().sort(), ["places", "readinglist", listBmk.id, newBmk.id].sort(), + "Should store Places root, reading list items, and new bookmark on server"); + + yield sync_engine_and_validate_telem(engine, false); + + ok(!store.itemExists("readinglist"), "Should not apply Reading List root"); + ok(!store.itemExists(listBmk.id), "Should not apply items in Reading List"); + ok(store.itemExists(newBmk.id), "Should apply new bookmark"); + + deepEqual(collection.keys().sort(), ["menu", "mobile", "toolbar", "unfiled", newBmk.id].sort(), + "Should remove Places root and reading list items from server; upload local roots"); + } finally { + store.wipe(); + Svc.Prefs.resetBranch(""); + Service.recordManager.clearCache(); + yield new Promise(resolve => server.stop(resolve)); + Svc.Obs.notify("weave:engine:stop-tracking"); + } +}); + add_task(function* test_change_during_sync() { _("Ensure that we track changes made during a sync.");