Bug 787273 - Part 2: Refactor Resource and Record to not rely on singletons; r=rnewman

Resource currently relies on the Identity singleton to perform
authentication. This is bad magic behavior. Resource instances should
authenticate according to the service instance they are associated with.

This patch removes Identity magic from Resource. Everything using
Resource now explicitly assigns an authenticator which comes from
the service instance/singleton. This required API changes to Collection
and Record.

The preferred method to obtain a Resource instance is to call
getResource() on a service instance.

The end result of this patch looks a little weird, especially in test
code. You have things like Service.resource(Service.cryptoKeysURL).
This ugliness will go away when a unified storage service client is
used.
This commit is contained in:
Gregory Szorc 2012-09-14 16:02:32 -07:00
parent c9586f34a5
commit 58bcd2801d
20 changed files with 104 additions and 79 deletions

View File

@ -750,7 +750,7 @@ SyncEngine.prototype = {
// Figure out how many total items to fetch this sync; do less on mobile.
let batchSize = Infinity;
let newitems = new Collection(this.engineURL, this._recordObj);
let newitems = new Collection(this.engineURL, this._recordObj, this.service);
let isMobile = (Svc.Prefs.get("client.type") == "mobile");
if (isMobile) {
@ -909,7 +909,7 @@ SyncEngine.prototype = {
// Mobile: check if we got the maximum that we requested; get the rest if so.
if (handled.length == newitems.limit) {
let guidColl = new Collection(this.engineURL);
let guidColl = new Collection(this.engineURL, null, this.service);
// Sort and limit so that on mobile we only get the last X records.
guidColl.limit = this.downloadLimit;
@ -1203,7 +1203,7 @@ SyncEngine.prototype = {
" outgoing records");
// collection we'll upload
let up = new Collection(this.engineURL);
let up = new Collection(this.engineURL, null, this.service);
let count = 0;
// Upload what we've got so far in the collection
@ -1269,7 +1269,7 @@ SyncEngine.prototype = {
this._tracker.resetScore();
let doDelete = Utils.bind2(this, function(key, val) {
let coll = new Collection(this.engineURL, this._recordObj);
let coll = new Collection(this.engineURL, this._recordObj, this.service);
coll[key] = val;
coll.delete();
});
@ -1320,7 +1320,7 @@ SyncEngine.prototype = {
let canDecrypt = false;
// Fetch the most recently uploaded record and try to decrypt it
let test = new Collection(this.engineURL, this._recordObj);
let test = new Collection(this.engineURL, this._recordObj, this.service);
test.limit = 1;
test.sort = "newest";
test.full = true;
@ -1348,7 +1348,7 @@ SyncEngine.prototype = {
},
wipeServer: function wipeServer() {
let response = new Resource(this.engineURL).delete();
let response = this.service.resource(this.engineURL).delete();
if (response.status != 200 && response.status != 404) {
throw response;
}

View File

@ -13,7 +13,6 @@ Cu.import("resource://services-common/stringbundle.js");
Cu.import("resource://services-sync/constants.js");
Cu.import("resource://services-sync/engines.js");
Cu.import("resource://services-sync/record.js");
Cu.import("resource://services-sync/resource.js");
Cu.import("resource://services-sync/util.js");
const CLIENTS_TTL = 1814400; // 21 days
@ -131,7 +130,7 @@ ClientEngine.prototype = {
},
removeClientData: function removeClientData() {
let res = new Resource(this.engineURL + "/" + this.localID);
let res = this.service.resource(this.engineURL + "/" + this.localID);
res.delete();
},

View File

@ -46,7 +46,7 @@ PasswordEngine.prototype = {
.map(function(info) {
return info.QueryInterface(Components.interfaces.nsILoginMetaInfo).guid;
});
let coll = new Collection(this.engineURL);
let coll = new Collection(this.engineURL, null, this.service);
coll.ids = ids;
let ret = coll.delete();
this._log.debug("Delete result: " + ret);

View File

@ -14,7 +14,6 @@ Cu.import("resource://gre/modules/XPCOMUtils.jsm");
Cu.import("resource://services-sync/engines.js");
Cu.import("resource://services-sync/engines/clients.js");
Cu.import("resource://services-sync/record.js");
Cu.import("resource://services-sync/resource.js");
Cu.import("resource://services-sync/util.js");
Cu.import("resource://services-sync/constants.js");
Cu.import("resource://services-common/preferences.js");
@ -75,7 +74,8 @@ TabEngine.prototype = {
},
removeClientData: function removeClientData() {
new Resource(this.engineURL + "/" + this.service.clientsEngine.localID).delete();
let url = this.engineURL + "/" + this.service.clientsEngine.localID;
this.service.resource(url).delete();
},
/* The intent is not to show tabs in the menu if they're already

View File

@ -18,10 +18,9 @@ const Cu = Components.utils;
const CRYPTO_COLLECTION = "crypto";
const KEYS_WBO = "keys";
Cu.import("resource://services-sync/constants.js");
Cu.import("resource://services-sync/identity.js");
Cu.import("resource://services-sync/keys.js");
Cu.import("resource://services-common/log4moz.js");
Cu.import("resource://services-sync/constants.js");
Cu.import("resource://services-sync/keys.js");
Cu.import("resource://services-sync/resource.js");
Cu.import("resource://services-sync/util.js");
@ -42,8 +41,12 @@ WBORecord.prototype = {
// Get thyself from your URI, then deserialize.
// Set thine 'response' field.
fetch: function fetch(uri) {
let r = new Resource(uri).get();
fetch: function fetch(resource) {
if (!resource instanceof Resource) {
throw new Error("First argument must be a Resource instance.");
}
let r = resource.get();
if (r.success) {
this.deserialize(r); // Warning! Muffles exceptions!
}
@ -51,8 +54,12 @@ WBORecord.prototype = {
return this;
},
upload: function upload(uri) {
return new Resource(uri).put(this);
upload: function upload(resource) {
if (!resource instanceof Resource) {
throw new Error("First argument must be a Resource instance.");
}
return resource.put(this);
},
// Take a base URI string, with trailing slash, and return the URI of this
@ -116,7 +123,7 @@ RecordManager.prototype = {
try {
// Clear out the last response with empty object if GET fails
this.response = {};
this.response = new Resource(url).get();
this.response = this.service.resource(url).get();
// Don't parse and save the record on failure
if (!this.response.success)
@ -504,9 +511,19 @@ CollectionKeyManager.prototype = {
}
}
function Collection(uri, recordObj) {
function Collection(uri, recordObj, service) {
if (!service) {
throw new Error("Collection constructor requires a service.");
}
Resource.call(this, uri);
// This is a bit hacky, but gets the job done.
let res = service.resource(uri);
this.authenticator = res.authenticator;
this._recordObj = recordObj;
this._service = service;
this._full = false;
this._ids = null;
@ -614,5 +631,5 @@ Collection.prototype = {
onRecord(record);
}
};
}
},
};

View File

@ -16,7 +16,6 @@ Cu.import("resource://services-common/async.js");
Cu.import("resource://services-sync/constants.js");
Cu.import("resource://services-common/observers.js");
Cu.import("resource://services-common/preferences.js");
Cu.import("resource://services-sync/identity.js");
Cu.import("resource://services-common/log4moz.js");
Cu.import("resource://services-sync/util.js");
@ -155,12 +154,8 @@ AsyncResource.prototype = {
let headers = this.headers;
let authenticator = this.authenticator;
if (!authenticator) {
authenticator = Identity.getResourceAuthenticator();
}
if (authenticator) {
let result = authenticator(this, method);
if (this.authenticator) {
let result = this.authenticator(this, method);
if (result && result.headers) {
for (let [k, v] in Iterator(result.headers)) {
headers[k.toLowerCase()] = v;

View File

@ -235,7 +235,7 @@ WeaveSvc.prototype = {
// Fetch keys.
let cryptoKeys = new CryptoWrapper(CRYPTO_COLLECTION, KEYS_WBO);
try {
let cryptoResp = cryptoKeys.fetch(this.cryptoKeysURL).response;
let cryptoResp = cryptoKeys.fetch(this.resource(this.cryptoKeysURL)).response;
// Save out the ciphertext for when we reupload. If there's a bug in
// CollectionKeys, this will prevent us from uploading junk.
@ -258,7 +258,7 @@ WeaveSvc.prototype = {
cryptoKeys.ciphertext = cipherText;
cryptoKeys.cleartext = null;
let uploadResp = cryptoKeys.upload(this.cryptoKeysURL);
let uploadResp = cryptoKeys.upload(this.resource(this.cryptoKeysURL));
if (uploadResp.success)
this._log.info("Successfully re-uploaded keys. Continuing sync.");
else
@ -471,6 +471,16 @@ WeaveSvc.prototype = {
}
},
/**
* Obtain a Resource instance with authentication credentials.
*/
resource: function resource(url) {
let res = new Resource(url);
res.authenticator = this._identity.getResourceAuthenticator();
return res;
},
/**
* Perform the info fetch as part of a login or key fetch.
*/
@ -480,7 +490,7 @@ WeaveSvc.prototype = {
this._log.trace("In _fetchInfo: " + infoURL);
let info;
try {
info = new Resource(infoURL).get();
info = this.resource(infoURL).get();
} catch (ex) {
this.errorHandler.checkServerError(ex);
throw ex;
@ -542,7 +552,7 @@ WeaveSvc.prototype = {
if (infoCollections && (CRYPTO_COLLECTION in infoCollections)) {
try {
cryptoKeys = new CryptoWrapper(CRYPTO_COLLECTION, KEYS_WBO);
let cryptoResp = cryptoKeys.fetch(this.cryptoKeysURL).response;
let cryptoResp = cryptoKeys.fetch(this.resource(this.cryptoKeysURL)).response;
if (cryptoResp.success) {
let keysChanged = this.handleFetchedKeys(syncKeyBundle, cryptoKeys);
@ -643,7 +653,7 @@ WeaveSvc.prototype = {
}
// Fetch collection info on every startup.
let test = new Resource(this.infoURL).get();
let test = this.resource(this.infoURL).get();
switch (test.status) {
case 200:
@ -706,7 +716,7 @@ WeaveSvc.prototype = {
wbo.encrypt(this._identity.syncKeyBundle);
this._log.info("Uploading...");
let uploadRes = wbo.upload(this.cryptoKeysURL);
let uploadRes = wbo.upload(this.resource(this.cryptoKeysURL));
if (uploadRes.status != 200) {
this._log.warn("Got status " + uploadRes.status + " uploading new keys. What to do? Throw!");
this.errorHandler.checkServerError(uploadRes);
@ -744,7 +754,7 @@ WeaveSvc.prototype = {
// Download and install them.
let cryptoKeys = new CryptoWrapper(CRYPTO_COLLECTION, KEYS_WBO);
let cryptoResp = cryptoKeys.fetch(this.cryptoKeysURL).response;
let cryptoResp = cryptoKeys.fetch(this.resource(this.cryptoKeysURL)).response;
if (cryptoResp.status != 200) {
this._log.warn("Failed to download keys.");
throw new Error("Symmetric key download failed.");
@ -980,7 +990,7 @@ WeaveSvc.prototype = {
newMeta.isNew = true;
this.recordManager.set(this.metaURL, newMeta);
if (!newMeta.upload(this.metaURL).success) {
if (!newMeta.upload(this.resource(this.metaURL)).success) {
this._log.warn("Unable to upload new meta/global. Failing remote setup.");
return false;
}
@ -1220,7 +1230,7 @@ WeaveSvc.prototype = {
meta.isNew = true;
this._log.debug("New metadata record: " + JSON.stringify(meta.payload));
let res = new Resource(this.metaURL);
let res = this.resource(this.metaURL);
// It would be good to set the X-If-Unmodified-Since header to `timestamp`
// for this PUT to ensure at least some level of transactionality.
// Unfortunately, the servers don't support it after a wipe right now
@ -1256,7 +1266,7 @@ WeaveSvc.prototype = {
let response;
if (!collections) {
// Strip the trailing slash.
let res = new Resource(this.storageURL.slice(0, -1));
let res = this.resource(this.storageURL.slice(0, -1));
res.setHeader("X-Confirm-Delete", "1");
try {
response = res.delete();
@ -1276,7 +1286,7 @@ WeaveSvc.prototype = {
for (let name of collections) {
let url = this.storageURL + name;
try {
response = new Resource(url).delete();
response = this.resource(url).delete();
} catch (ex) {
this._log.debug("Failed to wipe '" + name + "' collection: " +
Utils.exceptionStr(ex));

View File

@ -9,7 +9,6 @@ const {utils: Cu} = Components;
Cu.import("resource://services-common/log4moz.js");
Cu.import("resource://services-sync/constants.js");
Cu.import("resource://services-sync/policies.js");
Cu.import("resource://services-sync/resource.js");
Cu.import("resource://services-sync/status.js");
Cu.import("resource://services-sync/util.js");
@ -38,8 +37,8 @@ ClusterManager.prototype = {
// This should ideally use UserAPI10Client but the legacy hackiness is
// strong with this code.
let fail;
let res = new Resource(this.service.userAPIURI + this.identity.username +
"/node/weave");
let url = this.service.userAPIURI + this.identity.username + "/node/weave";
let res = this.service.resource(url);
try {
let node = res.get();
switch (node.status) {

View File

@ -14,7 +14,6 @@ Cu.import("resource://services-common/log4moz.js");
Cu.import("resource://services-sync/constants.js");
Cu.import("resource://services-sync/engines.js");
Cu.import("resource://services-sync/policies.js");
Cu.import("resource://services-sync/resource.js");
Cu.import("resource://services-sync/status.js");
Cu.import("resource://services-sync/util.js");
@ -165,7 +164,7 @@ EngineSynchronizer.prototype = {
// Upload meta/global if any engines changed anything
let meta = this.service.recordManager.get(this.service.metaURL);
if (meta.isNew || meta.changed) {
new Resource(this.service.metaURL).put(meta);
this.service.resource(this.service.metaURL).put(meta);
delete meta.isNew;
delete meta.changed;
}

View File

@ -53,7 +53,7 @@ add_test(function test_bad_hmac() {
generateNewKeys();
let serverKeys = CollectionKeys.asWBO("crypto", "keys");
serverKeys.encrypt(Weave.Identity.syncKeyBundle);
do_check_true(serverKeys.upload(Weave.Service.cryptoKeysURL).success);
do_check_true(serverKeys.upload(Service.resource(Service.cryptoKeysURL)).success);
}
try {
@ -82,7 +82,7 @@ add_test(function test_bad_hmac() {
generateNewKeys();
let serverKeys = CollectionKeys.asWBO("crypto", "keys");
serverKeys.encrypt(Weave.Identity.syncKeyBundle);
do_check_true(serverKeys.upload(Weave.Service.cryptoKeysURL).success);
do_check_true(serverKeys.upload(Service.resource(Service.cryptoKeysURL)).success);
_("Sync.");
engine._sync();

View File

@ -1,9 +1,13 @@
/* Any copyright is dedicated to the Public Domain.
http://creativecommons.org/publicdomain/zero/1.0/ */
_("Make sure Collection can correctly incrementally parse GET requests");
Cu.import("resource://services-sync/record.js");
Cu.import("resource://services-sync/service.js");
function run_test() {
let base = "http://fake/";
let coll = new Collection("http://fake/uri/", WBORecord);
let coll = new Collection("http://fake/uri/", WBORecord, Service);
let stream = { _data: "" };
let called, recCount, sum;

View File

@ -70,7 +70,7 @@ add_test(function test_locally_changed_keys() {
let m = new WBORecord("meta", "global");
m.payload = {"syncID": "foooooooooooooooooooooooooo",
"storageVersion": STORAGE_VERSION};
m.upload(Service.metaURL);
m.upload(Service.resource(Service.metaURL));
_("New meta/global: " + JSON.stringify(johndoe.collection("meta").wbo("global")));
@ -78,7 +78,7 @@ add_test(function test_locally_changed_keys() {
generateNewKeys();
let serverKeys = CollectionKeys.asWBO("crypto", "keys");
serverKeys.encrypt(Identity.syncKeyBundle);
do_check_true(serverKeys.upload(Service.cryptoKeysURL).success);
do_check_true(serverKeys.upload(Service.resource(Service.cryptoKeysURL)).success);
// Check that login works.
do_check_true(Service.login("johndoe", "ilovejane", passphrase));
@ -124,7 +124,7 @@ add_test(function test_locally_changed_keys() {
// Check that we can decrypt one.
let rec = new CryptoWrapper("history", "record-no--0");
rec.fetch(Service.storageURL + "history/record-no--0");
rec.fetch(Service.resource(Service.storageURL + "history/record-no--0"));
_(JSON.stringify(rec));
do_check_true(!!rec.decrypt());

View File

@ -61,7 +61,7 @@ function generateCredentialsChangedFailure() {
let newSyncKeyBundle = new SyncKeyBundle("johndoe", "23456234562345623456234562");
let keys = CollectionKeys.asWBO();
keys.encrypt(newSyncKeyBundle);
keys.upload(Service.cryptoKeysURL);
keys.upload(Service.resource(Service.cryptoKeysURL));
}
function service_unavailable(request, response) {
@ -134,7 +134,7 @@ function generateAndUploadKeys() {
generateNewKeys();
let serverKeys = CollectionKeys.asWBO("crypto", "keys");
serverKeys.encrypt(Identity.syncKeyBundle);
return serverKeys.upload(Service.cryptoKeysURL).success;
return serverKeys.upload(Service.resource(Service.cryptoKeysURL)).success;
}
function clean() {

View File

@ -9,7 +9,7 @@ Cu.import("resource://services-sync/service.js");
Cu.import("resource://services-sync/status.js");
Cu.import("resource://services-sync/util.js");
initTestLogging();
initTestLogging("Trace");
let engineManager = Service.engineManager;
engineManager.clear();
@ -62,7 +62,8 @@ function generateAndUploadKeys() {
generateNewKeys();
let serverKeys = CollectionKeys.asWBO("crypto", "keys");
serverKeys.encrypt(Weave.Identity.syncKeyBundle);
return serverKeys.upload("http://localhost:8080/1.1/johndoe/storage/crypto/keys").success;
let res = Service.resource("http://localhost:8080/1.1/johndoe/storage/crypto/keys");
return serverKeys.upload(res).success;
}

View File

@ -41,7 +41,7 @@ function setUp() {
generateNewKeys();
let serverKeys = CollectionKeys.asWBO("crypto", "keys");
serverKeys.encrypt(Identity.syncKeyBundle);
return serverKeys.upload(Service.cryptoKeysURL);
return serverKeys.upload(Service.resource(Service.cryptoKeysURL));
}
function run_test() {

View File

@ -51,7 +51,7 @@ function test_fetch() {
try {
_("Fetching a WBO record");
let rec = new WBORecord("coll", "record");
rec.fetch("http://localhost:8080/record");
rec.fetch(Service.resource("http://localhost:8080/record"));
do_check_eq(rec.id, "asdf-1234-asdf-1234"); // NOT "record"!
do_check_eq(rec.modified, 2454725.98283);

View File

@ -121,7 +121,7 @@ add_test(function v4_upgrade() {
serverKeys = serverResp = serverDecrypted = null;
serverKeys = new CryptoWrapper("crypto", "keys");
serverResp = serverKeys.fetch(Service.cryptoKeysURL).response;
serverResp = serverKeys.fetch(Service.resource(Service.cryptoKeysURL)).response;
do_check_true(serverResp.success);
serverDecrypted = serverKeys.decrypt(Identity.syncKeyBundle);
@ -149,7 +149,7 @@ add_test(function v4_upgrade() {
serverDecrypted.default = pair;
serverKeys.cleartext = serverDecrypted;
serverKeys.encrypt(Identity.syncKeyBundle);
serverKeys.upload(Service.cryptoKeysURL);
serverKeys.upload(Service.resource(Service.cryptoKeysURL));
}
_("Checking we have the latest keys.");
@ -248,7 +248,8 @@ add_test(function v5_upgrade() {
generateNewKeys();
serverKeys = CollectionKeys.asWBO("crypto", wboName);
serverKeys.encrypt(syncKeyBundle);
do_check_true(serverKeys.upload(Service.storageURL + collWBO).success);
let res = Service.resource(Service.storageURL + collWBO);
do_check_true(serverKeys.upload(res).success);
}
_("Bumping version.");
@ -256,7 +257,7 @@ add_test(function v5_upgrade() {
let m = new WBORecord("meta", "global");
m.payload = {"syncID": "foooooooooooooooooooooooooo",
"storageVersion": STORAGE_VERSION + 1};
m.upload(Service.metaURL);
m.upload(Service.resource(Service.metaURL));
_("New meta/global: " + JSON.stringify(meta_global));

View File

@ -156,7 +156,7 @@ function run_test() {
b.generateRandom();
collections.crypto = keys.modified = 100 + (Date.now()/1000); // Future modification time.
keys.encrypt(b);
keys.upload(Service.cryptoKeysURL);
keys.upload(Service.resource(Service.cryptoKeysURL));
do_check_false(Service.verifyAndFetchSymmetricKeys());
do_check_eq(Status.login, LOGIN_FAILED_INVALID_PASSPHRASE);

View File

@ -74,7 +74,7 @@ function setUp() {
generateNewKeys();
let serverKeys = CollectionKeys.asWBO("crypto", "keys");
serverKeys.encrypt(Identity.syncKeyBundle);
return serverKeys.upload(Service.cryptoKeysURL).success;
return serverKeys.upload(Service.resource(Service.cryptoKeysURL)).success;
}
const PAYLOAD = 42;
@ -256,7 +256,7 @@ add_test(function test_enabledRemotely() {
_("Upload some keys to avoid a fresh start.");
let wbo = CollectionKeys.generateNewKeysWBO();
wbo.encrypt(Identity.syncKeyBundle);
do_check_eq(200, wbo.upload(Service.cryptoKeysURL).status);
do_check_eq(200, wbo.upload(Service.resource(Service.cryptoKeysURL)).status);
_("Engine is disabled.");
do_check_false(engine.enabled);

View File

@ -58,7 +58,7 @@ function setUp() {
generateNewKeys();
let serverKeys = CollectionKeys.asWBO("crypto", "keys");
serverKeys.encrypt(Identity.syncKeyBundle);
return serverKeys.upload(Service.cryptoKeysURL).success;
return serverKeys.upload(Service.resource(Service.cryptoKeysURL)).success;
}
function cleanUpAndGo(server) {
@ -762,7 +762,7 @@ add_test(function test_sync_X_Weave_Backoff() {
clientsEngine._store.create({id: "foo", cleartext: "bar"});
let rec = clientsEngine._store.createRecord("foo", "clients");
rec.encrypt();
rec.upload(clientsEngine.engineURL + rec.id);
rec.upload(Service.resource(clientsEngine.engineURL + rec.id));
// Sync once to log in and get everything set up. Let's verify our initial
// values.
@ -819,7 +819,7 @@ add_test(function test_sync_503_Retry_After() {
clientsEngine._store.create({id: "foo", cleartext: "bar"});
let rec = clientsEngine._store.createRecord("foo", "clients");
rec.encrypt();
rec.upload(clientsEngine.engineURL + rec.id);
rec.upload(Service.resource(clientsEngine.engineURL + rec.id));
// Sync once to log in and get everything set up. Let's verify our initial
// values.