From 99dd5b9fe6fd065c1fd1eea9572067b3986a4392 Mon Sep 17 00:00:00 2001 From: Mark Hammond Date: Sat, 14 Jun 2014 14:33:56 +1000 Subject: [PATCH] Bug 1013064 (part 4) - browserid_identity and sync changes to support FxA credentials in the login manager. r=ckarlof,rnewman From 9717484083e66b78eedfa14e539d51382aba760f Mon Sep 17 00:00:00 2001 --- services/sync/modules/browserid_identity.js | 61 ++++++++++++++++++++-- services/sync/modules/identity.js | 19 +++++++ services/sync/modules/service.js | 20 ++++--- .../sync/tests/unit/test_browserid_identity.js | 15 ++++++ 4 files changed, 102 insertions(+), 13 deletions(-) --- services/sync/modules/browserid_identity.js | 61 +++++++++++++++++-- services/sync/modules/identity.js | 19 ++++++ services/sync/modules/service.js | 20 +++--- .../tests/unit/test_browserid_identity.js | 15 +++++ 4 files changed, 102 insertions(+), 13 deletions(-) diff --git a/services/sync/modules/browserid_identity.js b/services/sync/modules/browserid_identity.js index 3b0e290ffcf..b489807cc8f 100644 --- a/services/sync/modules/browserid_identity.js +++ b/services/sync/modules/browserid_identity.js @@ -399,7 +399,10 @@ this.BrowserIDManager.prototype = { * The current state of the auth credentials. * * This essentially validates that enough credentials are available to use - * Sync. + * Sync, although it effectively ignores the state of the master-password - + * if that's locked and that's the only problem we can see, say everything + * is OK - unlockAndVerifyAuthState will be used to perform the unlock + * and re-verification if necessary. */ get currentAuthState() { if (this._authFailureReason) { @@ -416,14 +419,53 @@ this.BrowserIDManager.prototype = { // No need to check this.syncKey as our getter for that attribute // uses this.syncKeyBundle - // If bundle creation started, but failed. - if (this._shouldHaveSyncKeyBundle && !this.syncKeyBundle) { - return LOGIN_FAILED_NO_PASSPHRASE; + // If bundle creation started, but failed due to any reason other than + // the MP being locked... + if (this._shouldHaveSyncKeyBundle && !this.syncKeyBundle && !Utils.mpLocked()) { + // Return a state that says a re-auth is necessary so we can get keys. + return LOGIN_FAILED_LOGIN_REJECTED; } return STATUS_OK; }, + // Do we currently have keys, or do we have enough that we should be able + // to successfully fetch them? + _canFetchKeys: function() { + let userData = this._signedInUser; + // a keyFetchToken means we can almost certainly grab them. + // kA and kB means we already have them. + return userData && (userData.keyFetchToken || (userData.kA && userData.kB)); + }, + + /** + * Verify the current auth state, unlocking the master-password if necessary. + * + * Returns a promise that resolves with the current auth state after + * attempting to unlock. + */ + unlockAndVerifyAuthState: function() { + if (this._canFetchKeys()) { + return Promise.resolve(STATUS_OK); + } + // so no keys - ensure MP unlocked. + if (!Utils.ensureMPUnlocked()) { + // user declined to unlock, so we don't know if they are stored there. + return Promise.resolve(MASTER_PASSWORD_LOCKED); + } + // now we are unlocked we must re-fetch the user data as we may now have + // the details that were previously locked away. + return this._fxaService.getSignedInUser().then( + accountData => { + this._updateSignedInUser(accountData); + // If we still can't get keys it probably means the user authenticated + // without unlocking the MP or cleared the saved logins, so we've now + // lost them - the user will need to reauth before continuing. + return this._canFetchKeys() ? STATUS_OK : LOGIN_FAILED_LOGIN_REJECTED; + } + ); + }, + /** * Do we have a non-null, not yet expired token for the user currently * signed in? @@ -449,6 +491,14 @@ this.BrowserIDManager.prototype = { let fxa = this._fxaService; let userData = this._signedInUser; + // We need kA and kB for things to work. If we don't have them, just + // return null for the token - sync calling unlockAndVerifyAuthState() + // before actually syncing will setup the error states if necessary. + if (!this._canFetchKeys()) { + log.info("_fetchTokenForUser has no keys to use."); + return null; + } + log.info("Fetching assertion and token from: " + tokenServerURI); let maybeFetchKeys = () => { @@ -524,7 +574,8 @@ this.BrowserIDManager.prototype = { // set it to the "fatal" LOGIN_FAILED_LOGIN_REJECTED reason. this._authFailureReason = LOGIN_FAILED_LOGIN_REJECTED; } else { - this._log.error("Non-authentication error in _fetchTokenForUser: " + err.message); + this._log.error("Non-authentication error in _fetchTokenForUser: " + + (err.message || err)); // for now assume it is just a transient network related problem. this._authFailureReason = LOGIN_FAILED_NETWORK_ERROR; } diff --git a/services/sync/modules/identity.js b/services/sync/modules/identity.js index cb7a1f58423..27129b827bf 100644 --- a/services/sync/modules/identity.js +++ b/services/sync/modules/identity.js @@ -377,6 +377,25 @@ IdentityManager.prototype = { return STATUS_OK; }, + /** + * Verify the current auth state, unlocking the master-password if necessary. + * + * Returns a promise that resolves with the current auth state after + * attempting to unlock. + */ + unlockAndVerifyAuthState: function() { + // Try to fetch the passphrase - this will prompt for MP unlock as a + // side-effect... + try { + this.syncKey; + } catch (ex) { + this._log.debug("Fetching passphrase threw " + ex + + "; assuming master password locked."); + return Promise.resolve(MASTER_PASSWORD_LOCKED); + } + return Promise.resolve(STATUS_OK); + }, + /** * Persist credentials to password store. * diff --git a/services/sync/modules/service.js b/services/sync/modules/service.js index 7102dd7bae6..4ab113e896f 100644 --- a/services/sync/modules/service.js +++ b/services/sync/modules/service.js @@ -684,17 +684,21 @@ Sync11Service.prototype = { return false; } - // Unlock master password, or return. // Attaching auth credentials to a request requires access to // passwords, which means that Resource.get can throw MP-related // exceptions! - // Try to fetch the passphrase first, while we still have control. - try { - this.identity.syncKey; - } catch (ex) { - this._log.debug("Fetching passphrase threw " + ex + - "; assuming master password locked."); - this.status.login = MASTER_PASSWORD_LOCKED; + // So we ask the identity to verify the login state after unlocking the + // master password (ie, this call is expected to prompt for MP unlock + // if necessary) while we still have control. + let cb = Async.makeSpinningCallback(); + this.identity.unlockAndVerifyAuthState().then( + result => cb(null, result), + cb + ); + let unlockedState = cb.wait(); + this._log.debug("Fetching unlocked auth state returned " + unlockedState); + if (unlockedState != STATUS_OK) { + this.status.login = unlockedState; return false; } diff --git a/services/sync/tests/unit/test_browserid_identity.js b/services/sync/tests/unit/test_browserid_identity.js index 947b026fdbc..f2ec7cc0a25 100644 --- a/services/sync/tests/unit/test_browserid_identity.js +++ b/services/sync/tests/unit/test_browserid_identity.js @@ -83,9 +83,23 @@ add_task(function test_initialializeWithCurrentIdentity() { } ); +add_task(function test_initialializeWithNoKeys() { + _("Verify start after initializeWithCurrentIdentity without kA, kB or keyFetchToken"); + let identityConfig = makeIdentityConfig(); + delete identityConfig.fxaccount.user.kA; + delete identityConfig.fxaccount.user.kB; + // there's no keyFetchToken by default, so the initialize should fail. + configureFxAccountIdentity(browseridManager, identityConfig); + + yield browseridManager.initializeWithCurrentIdentity(); + yield browseridManager.whenReadyToAuthenticate.promise; + do_check_eq(Status.login, LOGIN_SUCCEEDED, "login succeeded even without keys"); + do_check_false(browseridManager._canFetchKeys(), "_canFetchKeys reflects lack of keys"); +}); add_test(function test_getResourceAuthenticator() { _("BrowserIDManager supplies a Resource Authenticator callback which returns a Hawk header."); + configureFxAccountIdentity(browseridManager); let authenticator = browseridManager.getResourceAuthenticator(); do_check_true(!!authenticator); let req = {uri: CommonUtils.makeURI( @@ -240,6 +254,7 @@ add_test(function test_RESTResourceAuthenticatorSkew() { add_task(function test_ensureLoggedIn() { configureFxAccountIdentity(browseridManager); yield browseridManager.initializeWithCurrentIdentity(); + yield browseridManager.whenReadyToAuthenticate.promise; Assert.equal(Status.login, LOGIN_SUCCEEDED, "original initialize worked"); yield browseridManager.ensureLoggedIn(); Assert.equal(Status.login, LOGIN_SUCCEEDED, "original ensureLoggedIn worked");