Bug 1156752 - explicitly list where each FxA field is stored. r=zaach

This commit is contained in:
Mark Hammond 2015-07-27 08:58:53 +10:00
parent 283a89f3ed
commit 6830b9db61
10 changed files with 260 additions and 120 deletions

View File

@ -148,8 +148,13 @@ let wrapper = {
if (accountData.customizeSync) {
Services.prefs.setBoolPref(PREF_SYNC_SHOW_CUSTOMIZATION, true);
delete accountData.customizeSync;
}
delete accountData.customizeSync;
// sessionTokenContext is erroneously sent by the content server.
// https://github.com/mozilla/fxa-content-server/issues/2766
// To avoid having the FxA storage manager not knowing what to do with
// it we delete it here.
delete accountData.sessionTokenContext;
// We need to confirm a relink - see shouldAllowRelink for more
let newAccountEmail = accountData.email;

View File

@ -119,7 +119,7 @@ function configureFxAccountIdentity() {
let storageManager = new MockFxaStorageManager();
// and init storage with our user.
storageManager.initialize(user);
return new AccountState(this, storageManager);
return new AccountState(storageManager);
},
getCertificate(data, keyPair, mustBeValidUntil) {
this.cert = {

View File

@ -72,8 +72,7 @@ let publicProperties = [
// }
// If the state has changed between the function being called and the promise
// being resolved, the .resolve() call will actually be rejected.
let AccountState = this.AccountState = function(fxaInternal, storageManager) {
this.fxaInternal = fxaInternal;
let AccountState = this.AccountState = function(storageManager) {
this.storageManager = storageManager;
this.promiseInitialized = this.storageManager.getAccountData().then(data => {
this.oauthTokens = data && data.oauthTokens ? data.oauthTokens : {};
@ -84,13 +83,12 @@ let AccountState = this.AccountState = function(fxaInternal, storageManager) {
};
AccountState.prototype = {
cert: null,
keyPair: null,
oauthTokens: null,
whenVerifiedDeferred: null,
whenKeysReadyDeferred: null,
get isCurrent() this.fxaInternal && this.fxaInternal.currentAccountState === this,
// If the storage manager has been nuked then we are no longer current.
get isCurrent() this.storageManager != null,
abort() {
if (this.whenVerifiedDeferred) {
@ -108,7 +106,6 @@ AccountState.prototype = {
this.cert = null;
this.keyPair = null;
this.oauthTokens = null;
this.fxaInternal = null;
// Avoid finalizing the storageManager multiple times (ie, .signOut()
// followed by .abort())
if (!this.storageManager) {
@ -131,11 +128,14 @@ AccountState.prototype = {
});
},
getUserAccountData() {
// Get user account data. Optionally specify explcit field names to fetch
// (and note that if you require an in-memory field you *must* specify the
// field name(s).)
getUserAccountData(fieldNames = null) {
if (!this.isCurrent) {
return Promise.reject(new Error("Another user has signed in"));
}
return this.storageManager.getAccountData().then(result => {
return this.storageManager.getAccountData(fieldNames).then(result => {
return this.resolve(result);
});
},
@ -147,66 +147,6 @@ AccountState.prototype = {
return this.storageManager.updateAccountData(updatedFields);
},
getCertificate: function(data, keyPair, mustBeValidUntil) {
// TODO: get the lifetime from the cert's .exp field
if (this.cert && this.cert.validUntil > mustBeValidUntil) {
log.debug(" getCertificate already had one");
return this.resolve(this.cert.cert);
}
if (Services.io.offline) {
return this.reject(new Error(ERROR_OFFLINE));
}
let willBeValidUntil = this.fxaInternal.now() + CERT_LIFETIME;
return this.fxaInternal.getCertificateSigned(data.sessionToken,
keyPair.serializedPublicKey,
CERT_LIFETIME).then(
cert => {
log.debug("getCertificate got a new one: " + !!cert);
this.cert = {
cert: cert,
validUntil: willBeValidUntil
};
return cert;
}
).then(result => this.resolve(result));
},
getKeyPair: function(mustBeValidUntil) {
// If the debugging pref to ignore cached authentication credentials is set for Sync,
// then don't use any cached key pair, i.e., generate a new one and get it signed.
// The purpose of this pref is to expedite any auth errors as the result of a
// expired or revoked FxA session token, e.g., from resetting or changing the FxA
// password.
let ignoreCachedAuthCredentials = false;
try {
ignoreCachedAuthCredentials = Services.prefs.getBoolPref("services.sync.debug.ignoreCachedAuthCredentials");
} catch(e) {
// Pref doesn't exist
}
if (!ignoreCachedAuthCredentials && this.keyPair && (this.keyPair.validUntil > mustBeValidUntil)) {
log.debug("getKeyPair: already have a keyPair");
return this.resolve(this.keyPair.keyPair);
}
// Otherwse, create a keypair and set validity limit.
let willBeValidUntil = this.fxaInternal.now() + KEY_LIFETIME;
let d = Promise.defer();
jwcrypto.generateKeyPair("DS160", (err, kp) => {
if (err) {
return this.reject(err);
}
this.keyPair = {
keyPair: kp,
validUntil: willBeValidUntil
};
log.debug("got keyPair");
delete this.cert;
d.resolve(this.keyPair.keyPair);
});
return d.promise.then(result => this.resolve(result));
},
resolve: function(result) {
if (!this.isCurrent) {
log.info("An accountState promise was resolved, but was actually rejected" +
@ -427,7 +367,7 @@ FxAccountsInternal.prototype = {
newAccountState(credentials) {
let storage = new FxAccountsStorageManager();
storage.initialize(credentials);
return new AccountState(this, storage);
return new AccountState(storage);
},
/**
@ -559,6 +499,52 @@ FxAccountsInternal.prototype = {
})
},
/**
* returns a promise that fires with the keypair.
*/
getKeyPair: Task.async(function* (mustBeValidUntil) {
// If the debugging pref to ignore cached authentication credentials is set for Sync,
// then don't use any cached key pair, i.e., generate a new one and get it signed.
// The purpose of this pref is to expedite any auth errors as the result of a
// expired or revoked FxA session token, e.g., from resetting or changing the FxA
// password.
let ignoreCachedAuthCredentials = false;
try {
ignoreCachedAuthCredentials = Services.prefs.getBoolPref("services.sync.debug.ignoreCachedAuthCredentials");
} catch(e) {
// Pref doesn't exist
}
let currentState = this.currentAccountState;
let accountData = yield currentState.getUserAccountData("keyPair");
if (!ignoreCachedAuthCredentials && accountData.keyPair && (accountData.keyPair.validUntil > mustBeValidUntil)) {
log.debug("getKeyPair: already have a keyPair");
return accountData.keyPair.keyPair;
}
// Otherwse, create a keypair and set validity limit.
let willBeValidUntil = this.now() + KEY_LIFETIME;
let kp = yield new Promise((resolve, reject) => {
jwcrypto.generateKeyPair("DS160", (err, kp) => {
if (err) {
return reject(err);
}
log.debug("got keyPair");
let toUpdate = {
keyPair: {
keyPair: kp,
validUntil: willBeValidUntil
},
cert: null
};
currentState.updateUserAccountData(toUpdate).then(() => {
resolve(kp);
}).catch(err => {
log.error("Failed to update account data with keypair and cert");
});
});
});
return kp;
}),
/**
* returns a promise that fires with the assertion. If there is no verified
* signed-in user, fires with null.
@ -576,8 +562,8 @@ FxAccountsInternal.prototype = {
// Signed-in user has not verified email
return null;
}
return currentState.getKeyPair(mustBeValidUntil).then(keyPair => {
return currentState.getCertificate(data, keyPair, mustBeValidUntil)
return this.getKeyPair(mustBeValidUntil).then(keyPair => {
return this.getCertificate(data, keyPair, mustBeValidUntil)
.then(cert => {
return this.getAssertionFromCert(data, keyPair, cert, audience);
});
@ -845,6 +831,37 @@ FxAccountsInternal.prototype = {
);
},
/**
* returns a promise that fires with a certificate.
*/
getCertificate: Task.async(function* (data, keyPair, mustBeValidUntil) {
// TODO: get the lifetime from the cert's .exp field
let currentState = this.currentAccountState;
let accountData = yield currentState.getUserAccountData("cert");
if (accountData.cert && accountData.cert.validUntil > mustBeValidUntil) {
log.debug(" getCertificate already had one");
return accountData.cert.cert;
}
if (Services.io.offline) {
throw new Error(ERROR_OFFLINE);
}
let willBeValidUntil = this.now() + CERT_LIFETIME;
let cert = yield this.getCertificateSigned(data.sessionToken,
keyPair.serializedPublicKey,
CERT_LIFETIME);
log.debug("getCertificate got a new one: " + !!cert);
if (cert) {
let toUpdate = {
cert: {
cert: cert,
validUntil: willBeValidUntil
}
};
yield currentState.updateUserAccountData(toUpdate);
}
return cert;
}),
getUserAccountData: function() {
return this.currentAccountState.getUserAccountData();
},

View File

@ -212,13 +212,22 @@ exports.ERROR_MSG_METHOD_NOT_ALLOWED = "METHOD_NOT_ALLOWED";
// FxAccounts has the ability to "split" the credentials between a plain-text
// JSON file in the profile dir and in the login manager.
// These constants relate to that.
// In order to prevent new fields accidentally ending up in the "wrong" place,
// all fields stored are listed here.
// The fields we save in the plaintext JSON.
// See bug 1013064 comments 23-25 for why the sessionToken is "safe"
exports.FXA_PWDMGR_PLAINTEXT_FIELDS = ["email", "verified", "authAt",
"sessionToken", "uid", "oauthTokens",
"profile"];
exports.FXA_PWDMGR_PLAINTEXT_FIELDS = new Set(
["email", "verified", "authAt", "sessionToken", "uid", "oauthTokens", "profile"]);
// Fields we store in secure storage if it exists.
exports.FXA_PWDMGR_SECURE_FIELDS = new Set(
["kA", "kB", "keyFetchToken", "unwrapBKey", "assertion"]);
// Fields we keep in memory and don't persist anywhere.
exports.FXA_PWDMGR_MEMORY_FIELDS = new Set(
["cert", "keyPair"]);
// The pseudo-host we use in the login manager
exports.FXA_PWDMGR_HOST = "chrome://FirefoxAccounts";
// The realm we use in the login manager.

View File

@ -62,10 +62,18 @@ this.FxAccountsStorageManager.prototype = {
this._needToReadSecure = false;
// split it into the 2 parts, write it and we are done.
for (let [name, val] of Iterator(accountData)) {
if (FXA_PWDMGR_PLAINTEXT_FIELDS.indexOf(name) >= 0) {
if (FXA_PWDMGR_PLAINTEXT_FIELDS.has(name)) {
this.cachedPlain[name] = val;
} else {
} else if (FXA_PWDMGR_SECURE_FIELDS.has(name)) {
this.cachedSecure[name] = val;
} else {
// Hopefully it's an "in memory" field. If it's not we log a warning
// but still treat it as such (so it will still be available in this
// session but isn't persisted anywhere.)
if (!FXA_PWDMGR_MEMORY_FIELDS.has(name)) {
log.warn("Unknown FxA field name in user data, treating as in-memory", name);
}
this.cachedMemory[name] = val;
}
}
// write it out and we are done.
@ -121,7 +129,12 @@ this.FxAccountsStorageManager.prototype = {
},
// Get the account data by combining the plain and secure storage.
getAccountData: Task.async(function* () {
// If fieldNames is specified, it may be a string or an array of strings,
// and only those fields are returned. If not specified the entire account
// data is returned except for "in memory" fields. Note that not specifying
// field names will soon be deprecated/removed - we want all callers to
// specify the fields they care about.
getAccountData: Task.async(function* (fieldNames = null) {
yield this._promiseInitialized;
// We know we are initialized - this means our .cachedPlain is accurate
// and doesn't need to be read (it was read if necessary by initialize).
@ -130,21 +143,53 @@ this.FxAccountsStorageManager.prototype = {
return null;
}
let result = {};
for (let [name, value] of Iterator(this.cachedPlain)) {
result[name] = value;
if (fieldNames === null) {
// The "old" deprecated way of fetching a logged in user.
for (let [name, value] of Iterator(this.cachedPlain)) {
result[name] = value;
}
// But the secure data may not have been read, so try that now.
yield this._maybeReadAndUpdateSecure();
// .cachedSecure now has as much as it possibly can (which is possibly
// nothing if (a) secure storage remains locked and (b) we've never updated
// a field to be stored in secure storage.)
for (let [name, value] of Iterator(this.cachedSecure)) {
result[name] = value;
}
// Note we don't return cachedMemory fields here - they must be explicitly
// requested.
return result;
}
// But the secure data may not have been read, so try that now.
yield this._maybeReadAndUpdateSecure();
// .cachedSecure now has as much as it possibly can (which is possibly
// nothing if (a) secure storage remains locked and (b) we've never updated
// a field to be stored in secure storage.)
for (let [name, value] of Iterator(this.cachedSecure)) {
result[name] = value;
// The new explicit way of getting attributes.
if (!Array.isArray(fieldNames)) {
fieldNames = [fieldNames];
}
let checkedSecure = false;
for (let fieldName of fieldNames) {
if (FXA_PWDMGR_MEMORY_FIELDS.has(fieldName)) {
if (this.cachedMemory[fieldName] !== undefined) {
result[fieldName] = this.cachedMemory[fieldName];
}
} else if (FXA_PWDMGR_PLAINTEXT_FIELDS.has(fieldName)) {
if (this.cachedPlain[fieldName] !== undefined) {
result[fieldName] = this.cachedPlain[fieldName];
}
} else if (FXA_PWDMGR_SECURE_FIELDS.has(fieldName)) {
// We may not have read secure storage yet.
if (!checkedSecure) {
yield this._maybeReadAndUpdateSecure();
checkedSecure = true;
}
if (this.cachedSecure[fieldName] !== undefined) {
result[fieldName] = this.cachedSecure[fieldName];
}
} else {
throw new Error("unexpected field '" + name + "'");
}
}
return result;
}),
// Update just the specified fields. This DOES NOT allow you to change to
// a different user, nor to set the user as signed-out.
updateAccountData: Task.async(function* (newFields) {
@ -163,16 +208,27 @@ this.FxAccountsStorageManager.prototype = {
log.debug("_updateAccountData with items", Object.keys(newFields));
// work out what bucket.
for (let [name, value] of Iterator(newFields)) {
if (FXA_PWDMGR_PLAINTEXT_FIELDS.indexOf(name) >= 0) {
if (FXA_PWDMGR_MEMORY_FIELDS.has(name)) {
if (value == null) {
delete this.cachedMemory[name];
} else {
this.cachedMemory[name] = value;
}
} else if (FXA_PWDMGR_PLAINTEXT_FIELDS.has(name)) {
if (value == null) {
delete this.cachedPlain[name];
} else {
this.cachedPlain[name] = value;
}
} else {
} else if (FXA_PWDMGR_SECURE_FIELDS.has(name)) {
// don't do the "delete on null" thing here - we need to keep it until
// we have managed to read so we can nuke it on write.
this.cachedSecure[name] = value;
} else {
// Throwing seems reasonable here as some client code has explicitly
// specified the field name, so it's either confused or needs to update
// how this field is to be treated.
throw new Error("unexpected field '" + name + "'");
}
}
// If we haven't yet read the secure data, do so now, else we may write
@ -185,6 +241,7 @@ this.FxAccountsStorageManager.prototype = {
}),
_clearCachedData() {
this.cachedMemory = {};
this.cachedPlain = {};
// If we don't have secure storage available we have cachedPlain and
// cachedSecure be the same object.

View File

@ -155,7 +155,7 @@ function MockFxAccounts() {
// we use a real accountState but mocked storage.
let storage = new MockStorageManager();
storage.initialize(credentials);
return new AccountState(this, storage);
return new AccountState(storage);
},
getCertificateSigned: function (sessionToken, serializedPublicKey) {
_("mock getCertificateSigned\n");
@ -202,7 +202,7 @@ add_task(function test_get_signed_in_user_initially_unset() {
// we use a real accountState but mocked storage.
let storage = new MockStorageManager();
storage.initialize(credentials);
return new AccountState(this, storage);
return new AccountState(storage);
},
});
let credentials = {
@ -251,7 +251,7 @@ add_task(function* test_getCertificate() {
// we use a real accountState but mocked storage.
let storage = new MockStorageManager();
storage.initialize(credentials);
return new AccountState(this, storage);
return new AccountState(storage);
},
});
let credentials = {
@ -272,7 +272,7 @@ add_task(function* test_getCertificate() {
let offline = Services.io.offline;
Services.io.offline = true;
// This call would break from missing parameters ...
yield fxa.internal.currentAccountState.getCertificate().then(
yield fxa.internal.getCertificate().then(
result => {
Services.io.offline = offline;
do_throw("Unexpected success");
@ -505,8 +505,9 @@ add_task(function test_getAssertion() {
_("ASSERTION: " + assertion + "\n");
let pieces = assertion.split("~");
do_check_eq(pieces[0], "cert1");
let keyPair = fxa.internal.currentAccountState.keyPair;
let cert = fxa.internal.currentAccountState.cert;
let userData = yield fxa.getSignedInUser();
let keyPair = userData.keyPair;
let cert = userData.cert;
do_check_neq(keyPair, undefined);
_(keyPair.validUntil + "\n");
let p2 = pieces[1].split(".");
@ -553,9 +554,10 @@ add_task(function test_getAssertion() {
// expiration time of the assertion should be different. We compare this to
// the initial start time, to which they are relative, not the current value
// of "now".
userData = yield fxa.getSignedInUser();
keyPair = fxa.internal.currentAccountState.keyPair;
cert = fxa.internal.currentAccountState.cert;
keyPair = userData.keyPair;
cert = userData.cert;
do_check_eq(keyPair.validUntil, start + KEY_LIFETIME);
do_check_eq(cert.validUntil, start + CERT_LIFETIME);
exp = Number(payload.exp);
@ -576,8 +578,9 @@ add_task(function test_getAssertion() {
header = JSON.parse(atob(p2[0]));
payload = JSON.parse(atob(p2[1]));
do_check_eq(payload.aud, "fourth.example.com");
keyPair = fxa.internal.currentAccountState.keyPair;
cert = fxa.internal.currentAccountState.cert;
userData = yield fxa.getSignedInUser();
keyPair = userData.keyPair;
cert = userData.cert;
do_check_eq(keyPair.validUntil, now + KEY_LIFETIME);
do_check_eq(cert.validUntil, now + CERT_LIFETIME);
exp = Number(payload.exp);

View File

@ -85,7 +85,7 @@ function MockFxAccounts() {
// we use a real accountState but mocked storage.
let storage = new MockStorageManager();
storage.initialize(credentials);
return new AccountState(this, storage);
return new AccountState(storage);
},
});
}

View File

@ -51,9 +51,11 @@ function MockedSecureStorage(accountData) {
}
MockedSecureStorage.prototype = {
fetchCount: 0,
locked: false,
STORAGE_LOCKED: function() {},
get: Task.async(function* (uid, email) {
this.fetchCount++;
if (this.locked) {
throw new this.STORAGE_LOCKED();
}
@ -85,7 +87,7 @@ add_storage_task(function* checkInitializedEmpty(sm) {
}
yield sm.initialize();
Assert.strictEqual((yield sm.getAccountData()), null);
Assert.rejects(sm.updateAccountData({foo: "bar"}), "No user is logged in")
Assert.rejects(sm.updateAccountData({kA: "kA"}), "No user is logged in")
});
// Initialized with account data (ie, simulating a new user being logged in).
@ -130,9 +132,9 @@ add_storage_task(function* checkEverythingRead(sm) {
Assert.equal(accountData.email, "someone@somewhere.com");
// Update the data - we should be able to fetch it back and it should appear
// in our storage.
yield sm.updateAccountData({verified: true, foo: "bar", kA: "kA"});
yield sm.updateAccountData({verified: true, kA: "kA", kB: "kB"});
accountData = yield sm.getAccountData();
Assert.equal(accountData.foo, "bar");
Assert.equal(accountData.kB, "kB");
Assert.equal(accountData.kA, "kA");
// Check the new value was written to storage.
yield sm._promiseStorageComplete; // storage is written in the background.
@ -141,10 +143,10 @@ add_storage_task(function* checkEverythingRead(sm) {
// "kA" and "foo" are secure
if (sm.secureStorage) {
Assert.equal(sm.secureStorage.data.accountData.kA, "kA");
Assert.equal(sm.secureStorage.data.accountData.foo, "bar");
Assert.equal(sm.secureStorage.data.accountData.kB, "kB");
} else {
Assert.equal(sm.plainStorage.data.accountData.kA, "kA");
Assert.equal(sm.plainStorage.data.accountData.foo, "bar");
Assert.equal(sm.plainStorage.data.accountData.kB, "kB");
}
});
@ -231,6 +233,53 @@ add_task(function* checkEverythingReadSecure() {
Assert.equal(accountData.kA, "kA");
});
add_task(function* checkMemoryFieldsNotReturnedByDefault() {
let sm = new FxAccountsStorageManager();
sm.plainStorage = new MockedPlainStorage({uid: "uid", email: "someone@somewhere.com"})
sm.secureStorage = new MockedSecureStorage({kA: "kA"});
yield sm.initialize();
// keyPair is a memory field.
yield sm.updateAccountData({keyPair: "the keypair value"});
let accountData = yield sm.getAccountData();
// Requesting everything should *not* return in memory fields.
Assert.strictEqual(accountData.keyPair, undefined);
// But requesting them specifically does get them.
accountData = yield sm.getAccountData("keyPair");
Assert.strictEqual(accountData.keyPair, "the keypair value");
});
add_task(function* checkExplicitGet() {
let sm = new FxAccountsStorageManager();
sm.plainStorage = new MockedPlainStorage({uid: "uid", email: "someone@somewhere.com"})
sm.secureStorage = new MockedSecureStorage({kA: "kA"});
yield sm.initialize();
let accountData = yield sm.getAccountData(["uid", "kA"]);
Assert.ok(accountData, "read account data");
Assert.equal(accountData.uid, "uid");
Assert.equal(accountData.kA, "kA");
// We didn't ask for email so shouldn't have got it.
Assert.strictEqual(accountData.email, undefined);
});
add_task(function* checkExplicitGetNoSecureRead() {
let sm = new FxAccountsStorageManager();
sm.plainStorage = new MockedPlainStorage({uid: "uid", email: "someone@somewhere.com"})
sm.secureStorage = new MockedSecureStorage({kA: "kA"});
yield sm.initialize();
Assert.equal(sm.secureStorage.fetchCount, 0);
// request 2 fields in secure storage - it should have caused a single fetch.
let accountData = yield sm.getAccountData(["email", "uid"]);
Assert.ok(accountData, "read account data");
Assert.equal(accountData.uid, "uid");
Assert.equal(accountData.email, "someone@somewhere.com");
Assert.strictEqual(accountData.kA, undefined);
Assert.equal(sm.secureStorage.fetchCount, 1);
});
add_task(function* checkLockedUpdates() {
let sm = new FxAccountsStorageManager();
sm.plainStorage = new MockedPlainStorage({uid: "uid", email: "someone@somewhere.com"})

View File

@ -181,17 +181,17 @@ this.configureFxAccountIdentity = function(authService,
}
let storageManager = new MockFxaStorageManager();
storageManager.initialize(config.fxaccount.user);
let accountState = new AccountState(this, storageManager);
// mock getCertificate
accountState.getCertificate = function(data, keyPair, mustBeValidUntil) {
accountState.cert = {
validUntil: fxa.internal.now() + CERT_LIFETIME,
cert: "certificate",
};
return Promise.resolve(this.cert.cert);
}
let accountState = new AccountState(storageManager);
return accountState;
}
},
getCertificate(data, keyPair, mustBeValidUntil) {
let cert = {
validUntil: this.now() + CERT_LIFETIME,
cert: "certificate",
};
this.currentAccountState.updateUserAccountData({cert: cert});
return Promise.resolve(cert.cert);
},
};
fxa = new FxAccounts(MockInternal);

View File

@ -595,7 +595,7 @@ add_task(function test_getKeysMissing() {
}
let storageManager = new MockFxaStorageManager();
storageManager.initialize(identityConfig.fxaccount.user);
return new AccountState(this, storageManager);
return new AccountState(storageManager);
},
});
@ -674,7 +674,7 @@ function* initializeIdentityWithHAWKResponseFactory(config, cbGetResponse) {
}
let storageManager = new MockFxaStorageManager();
storageManager.initialize(config.fxaccount.user);
return new AccountState(this, storageManager);
return new AccountState(storageManager);
},
}
let fxa = new FxAccounts(internal);