This code seems to have never worked correctly. This code was based on
an error style that is thrown from one part of kinto-http.js but then
immediately rethrown using a different format. On top of that, the
code as written tries to parse JSON out of the response body and use
that. Instead, let's use the response status code, since that feels a
little more robust.
MozReview-Commit-ID: B5MG5tpduCd
--HG--
extra : rebase_source : 9f4ba94ef215297db6ba3b126852984a9a7878fe
This is another edge case that seems to be linked to the server-side
data loss bug in Kinto. It seems like you can get into a "server was
flushed" case if one device hadn't seen the most recent keyring before
another client caused the server to be wiped. We don't expect this to
happen in the future, but it affects some number of our users who were
already using storage.sync, so build a recovery pathway for them.
MozReview-Commit-ID: K0BJNQgN6Z4
--HG--
extra : rebase_source : 5a8707bc1edc1f30306a301d3c9f7b10a1f2fb76
It seems like the server-side keyring is getting deleted somehow. This
causes syncing of the keyring to fail. We haven't completely
fixed the root problem yet, but this adds logging to try to flag
down the dangerous operation of deleting a bucket to make sure we
aren't doing that by accident. We also try to recover from the
server-keyring-was-deleted situation by wiping all storage.sync data,
the same as if we found a keyring that we couldn't decrypt.
MozReview-Commit-ID: JMB0IxApbGq
--HG--
extra : rebase_source : 54982a374aa5c7d7e60e8175b99734c672b91799
The elements of the SyncResultObject#conflicts field are {local,
remote} pairs. However, the elements of the SyncResultObject#resolved
field are just the resolutions, and trying to access "local" and
"remote" fields of those resolutions causes a crash. Add a test that
demonstrates the error, and then fix it.
MozReview-Commit-ID: LF0NBw5VKBC
--HG--
extra : rebase_source : 7711111d35cfe962f57c7d08d74bad2e37090b7c
Get rid of the ugly hack where test code monkeypatches a singleton to
convince the rest of the ExtensionStorageSync code that a fake user is
logged in. Instead, take a handle to the fxAccounts service at
construction time. Test code can provide any kind of fxAccounts it
wants, including one that has a hard-coded user.
This provokes a bunch of changes:
- ExtensionStorageSync is now no longer a singleton, but a class. You
have to instantiate it in order to do anything with it. A global
instance extensionStorageSync is provided for use by Sync code as
well as WebExtension code.
- CryptoCollection is now also a class, and each ExtensionStorageSync
instance has its own CryptoCollection. This dependency should maybe
also be injected, but for the time being it doesn't provide us any
value to do so.
- Converting singletons with asynchronous methods to classes is a pain
in the neck. We convert async method foo from `foo:
Task.async(....)` to `async foo() { .... }`. While we're here,
convert KeyRingEncryptionRemoteTransformer#decode to async/await to
eliminate a need for `let self = this`.
- Update Sync code and WebExtension code to use extensionStorageSync.
- There's a cyclic dependency where CryptoCollection#sync depends on
ExtensionStorageSync#_syncCollection which depends on
CryptoCollection#getKeyRing. As a short-term hack, we now require an
ExtensionStorageSync argument to CryptoCollection#sync.
- KeyRingEncryptionRemoteTransformer now takes a handle to fxAccounts
on construction time as well. Because this is the only
EncryptionRemoteTransformer subclass that accesses fxAccounts, we
can get rid of the hack where the tests monkeypatch something in the
EncryptionRemoteTransformer prototype.
- CollectionKeyEncryptionRemoteTransformer now takes a handle to a
CryptoCollection, rather than relying on a global one.
- A bunch of methods that previously assumed access to fxAccounts now
check if fxAccounts is present (i.e. if we're on Android). Strictly
speaking, this isn't required by this change, but it helps make
certain kinds of failure a little easier to diagnose.
- Update tests, including extracting a domain-specific helper method
to hide the use of CollectionKeyEncryptionRemoteTransformer. We now
no longer monkeypatch in our mock fxaService, but pass it to the
test so that it can do whatever it wants with it. We also provide an
ExtensionStorageSync instance for convenience. Access to the global
cryptoCollection is now done through an ExtensionStorageSync
instance.
To summarize, we've gone from a situation where a bunch of singletons
had implicit dependencies on other singletons in a shared global
namespace, to a situation where dependencies are provided explicitly
using method/constructor arguments. This highlights some of the
dependencies present:
- ExtensionStorageSync depends on CryptoCollection and fxAccounts if
it needs to sync
- Every collection created via openCollection needs a handle to
CryptoCollection so it can correctly create its remote transformers
- CryptoCollection needs a handle to fxAccounts so it can create its
own remote transformer for its special collection
Most of this is only possible, or at least much easier, because we no
longer try to juggle Sqlite connections but just keep one around
forever.
However, please note:
- CryptoCollection needs a handle to ExtensionStorageSync to actually
perform syncing logic because that's where we foolishly put the
logic to make requests
- There's still a backing Sqlite store which is shared by everything
- There's still a singleton tracking contexts that opened extensions
which we manage to try to clean contexts up correctly
MozReview-Commit-ID: DGIzyRTdYZ1
--HG--
extra : rebase_source : 305a98b8e1ece3ae7a5e8a2fdb5c58d8e7c496d9
Since services/sync doesn't ship on Android, this meant
conditionally-defining some variables such as `cryptoCollection` and
`CollectionKeyEncryptionRemoteTransformer` depending on whether or not
we were on Android. However, none of these definitions really rely on
functionality that isn't present on Android (although you can't really
use them yet either). Move the dependency together with the dependant
code so we can simplify a bit. This lets us remove conditional uses of
`cryptoCollection` and `CollectionKeyEncryptionRemoteTransformer`.
Because the WebExtensions source directory has more stringent eslint
rules, we end up reformatting and commenting a bit in addition to
moving.
MozReview-Commit-ID: 2ddDeymYFNi
--HG--
rename : services/sync/tests/unit/test_extension_storage_crypto.js => toolkit/components/extensions/test/xpcshell/test_ext_storage_sync_crypto.js
extra : rebase_source : f84c0810b6a447f2104f96c9dc9d6139743f269c
This does a sha256 of record IDs, the same way we do for collection
IDs, during encryption.
The way we compute the new ID (using an overridden method) is a little
bit of a hack, but we use the new ID as part of the HMAC. This also
invalidates a previous assumption, which is that we kept record IDs
the same during decryption.
MozReview-Commit-ID: Gbzlo9OE1he
--HG--
extra : rebase_source : 3a303cce50e60ecf0cd709da4ebef5ff98bff314
extra : intermediate-source : 033163eeac84466553d4f9636093c3b824d7cf2b
extra : source : c5c115baff4a729b064e74e87821f7ee3b9d3edb
This ties the collection ID used for a given extension to the "salts"
record. This blocks an attack where, given the user's ID, an attacker
could just hash all the extension IDs on addons.mozilla.org to
generate possible hashed collection IDs.
MozReview-Commit-ID: OABsOOMn2F
--HG--
extra : rebase_source : 7c43968b9b7d695c98f00f99d7349d39eb1dc514
extra : source : ed19b98e6b8beb3009cee342bb97eb5684c02af6
Because these need to be encrypted with kB like the keyring does, and
wiped at the same time as the keyring does, just smush them into the
keyring. Rename `ensureKeysFor` to `ensureCanSync`, but don't update
it to return the salts -- for now, we don't need that.
MozReview-Commit-ID: DOJxdx5ugKl
--HG--
extra : rebase_source : 02fdae5d434232395d91a626213d91d4bdff40bc
`generateUUID()` returns an `nsID`, which is not exactly the same as a
UUID. `nsID`s can be converted to strings using `toString()`, but if
you use `JSON.stringify()`, they become `{}`. Object comparison in JS
performs identity comparison, which would be useless even if the UUIDs
were sensible, which they aren't. As a result, trying to sync keyrings
always failed, because it always seemed like UUIDs had changed, even
when they hadn't.
Because it never occurred to me that UUIDs wouldn't be strings, I
never even wrote a test for this. Correct this, and fix the test.
Thanks to :vasilica_mihasca and :markh for reporting and diagnosing this.
MozReview-Commit-ID: EthxkFFwRbQ
--HG--
extra : rebase_source : 7b2a258d85bf8aab2bf9895e3a8cac9d34ab8435
This major version of kinto.js released without a FirefoxStorage
adapter. Since Gecko is the only project with a need for this adapter
as well as the only one who can use it, that file moves to this repo
as a new kinto-storage-adapter.js. This file is mostly a copy of the
most recent FirefoxStorage.js, plus some other non-exported utility
functions copied from kinto.js to make it work.
This changes the export of the kinto-offline-client.js from a
`loadKinto` function which returns the Kinto class, to the actual
Kinto class itself, with the user expected to "hook up" the
adapter. It turns out that this doesn't change much in how we actually
use Kinto, since we were always previously explicitly selecting the
Kinto adapter.
This release of kinto.js also changes some of the build options
around, which changes the minified output of kinto-offline-client.js.
There are still some outstanding stylistic complaints about
FirefoxAdapter having to do with its _init static method which is
public and the fact that sometimes FirefoxAdapter manages its own
Sqlite connection and sometimes it doesn't. These will be addressed in
a future patch.
MozReview-Commit-ID: HF0oNCEDcFs
--HG--
rename : services/common/kinto-offline-client.js => services/common/kinto-storage-adapter.js
extra : rebase_source : 11d01e573b259798305494ac072575247ac01e2c
Note that this "enables" the engine using a pref, even though it might
not be ready yet, so that the tests can pass.
MozReview-Commit-ID: AZ0TVERiQDU
--HG--
extra : rebase_source : 8ca202c1ec79e068fb9395207792942a228d4433
extra : intermediate-source : 93f41b72d8e7023b5f22c727436e836de87c9ca7
extra : source : 183547f4dbbedc9ee3399b6a474016d0e89a12e8
Note that this "enables" the engine using a pref, even though it might
not be ready yet, so that the tests can pass.
MozReview-Commit-ID: AZ0TVERiQDU
--HG--
extra : rebase_source : 3078b86c4255afd66a9c4faf895dc2b6fc43a152
extra : intermediate-source : 93f41b72d8e7023b5f22c727436e836de87c9ca7
extra : source : 183547f4dbbedc9ee3399b6a474016d0e89a12e8
