Magic-Mirror/gecko-dev
1
0
Fork 0
mirror of https://github.com/mozilla/gecko-dev.git synced 2024-10-21 01:05:45 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Bug 1148504 - Protect Firefox Account state with a critical section. r=rnewman

Browse Source 
========

8b1d353ee8
Author: Nick Alexander <nalexander@mozilla.com>
    Bug 1148504 - Part 2: Make updating Firefox Account state happen in a critical section.

    It's worth noting that the two consumers of the shared state lock will
    only race for a very short window -- essentially only when creating or
    re-connecting an account.

    That's because Reading List oauth tokens are long-lived and do not
    expire (yet) in response to remote Account state changes, such as
    updating the Account password.  So Sync and RL will race to initialize
    the Account state; eventually RL will get an oauth token; and that token
    will be cached forever until RL produces a 401 for the token or Android
    expires the token.

    Since Sync requests a token server token at the start of every sync, the
    lock will be constantly exercised, but should never block.

========

d7a8611810
Author: Nick Alexander <nalexander@mozilla.com>
Date:   Fri Mar 27 08:27:28 2015 -0700

    Bug 1148504 - Part 1: Reduce scope of section that may set Account state.

    The only place that might throw a TokenServerException is the token
    server client code itself.  By handling such an exception earlier, we
    reduce the scope of the section that may update the Firefox Account
    state.  (This comes at the cost of threading AndroidFxAccount into
    syncWithAssertion, but c'est la vie.)

    This does not interact with the exist handling of 401s that we might see
    from the storage endpoint.  Those 401s never generated
    TokenServerExceptions; in fact, they were (essentially) ignored.  Since
    we fetch a fresh token every Sync, what was (and is) expected is that
    such 401s would be transient and fixed by authenticating with a fresher
    token.

    Test plan: manually verify that remotely changing the Firefox Account's
    password while the device is in the Married state does the following:

    1) uses the cached certificate to generate a local assertion;
    2) the assertion produces a 401 from the TokenServerException, since the
    certificate is no longer fresh;
    3) the TokenServerException drives the Account state to Cohabiting;
    4) the state machine discovers it cannot /sign, driving the Account
    state to Separated.
This commit is contained in:
Nick Alexander 2015-03-27 10:01:35 -07:00
parent 463d180cd8
commit 5a8d0a41bc
5 changed files with 116 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

54
mobile/android/base/fxa/authenticator/AndroidFxAccount.java
View File

@ -13,6 +13,7 @@ import java.util.EnumSet;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.Semaphore;
import org.mozilla.gecko.AppConstants;
import org.mozilla.gecko.background.common.GlobalConstants;
@ -35,6 +36,7 @@ import android.content.Context;
import android.content.Intent;
import android.content.SharedPreferences;
import android.os.Bundle;
import android.util.Log;
/**
* A Firefox Account that stores its details and state as user data attached to
@ -651,4 +653,56 @@ public class AndroidFxAccount {
accountManager.setUserData(account, ACCOUNT_KEY_IDP_SERVER, authServerEndpoint);
accountManager.setUserData(account, ACCOUNT_KEY_TOKEN_SERVER, tokenServerEndpoint);
}
/**
* Take the lock to own updating any Firefox Account's internal state.
*
* We use a <code>Semaphore</code> rather than a <code>ReentrantLock</code>
* because the callback that needs to release the lock may not be invoked on
* the thread that initially acquired the lock. Be aware!
*/
protected static final Semaphore sLock = new Semaphore(1, true /* fair */);
// Which consumer took the lock?
// Synchronized by this.
protected String lockTag = null;
// Are we locked? (It's not easy to determine who took the lock dynamically,
// so we maintain this flag internally.)
// Synchronized by this.
protected boolean locked = false;
// Block until we can take the shared state lock.
public synchronized void acquireSharedAccountStateLock(final String tag) throws InterruptedException {
final long id = Thread.currentThread().getId();
this.lockTag = tag;
Log.d(Logger.DEFAULT_LOG_TAG, "Thread with tag and thread id acquiring lock: " + lockTag + ", " + id + " ...");
sLock.acquire();
locked = true;
Log.d(Logger.DEFAULT_LOG_TAG, "Thread with tag and thread id acquiring lock: " + lockTag + ", " + id + " ... ACQUIRED");
}
// If we hold the shared state lock, release it. Otherwise, ignore the request.
public synchronized void releaseSharedAccountStateLock() {
final long id = Thread.currentThread().getId();
Log.d(Logger.DEFAULT_LOG_TAG, "Thread with tag and thread id releasing lock: " + lockTag + ", " + id + " ...");
if (locked) {
sLock.release();
locked = false;
Log.d(Logger.DEFAULT_LOG_TAG, "Thread with tag and thread id releasing lock: " + lockTag + ", " + id + " ... RELEASED");
} else {
Log.d(Logger.DEFAULT_LOG_TAG, "Thread with tag and thread id releasing lock: " + lockTag + ", " + id + " ... NOT LOCKED");
}
}
@Override
protected synchronized void finalize() {
if (locked) {
// Should never happen, but...
sLock.release();
locked = false;
final long id = Thread.currentThread().getId();
Log.e(Logger.DEFAULT_LOG_TAG, "Thread with tag and thread id releasing lock: " + lockTag + ", " + id + " ... RELEASED DURING FINALIZE");
}
}
}

31
mobile/android/base/fxa/authenticator/FxAccountAuthenticator.java
View File

@ -93,15 +93,16 @@ public class FxAccountAuthenticator extends AbstractAccountAuthenticator {
protected static class Responder {
final AccountAuthenticatorResponse response;
final Account account;
final AndroidFxAccount fxAccount;
public Responder(AccountAuthenticatorResponse response, Account account) {
public Responder(AccountAuthenticatorResponse response, AndroidFxAccount fxAccount) {
this.response = response;
this.account = account;
this.fxAccount = fxAccount;
}
public void fail(Exception e) {
Logger.warn(LOG_TAG, "Responding with error!", e);
fxAccount.releaseSharedAccountStateLock();
final Bundle result = new Bundle();
result.putInt(AccountManager.KEY_ERROR_CODE, UNKNOWN_ERROR_CODE);
result.putString(AccountManager.KEY_ERROR_MESSAGE, e.toString());
@ -110,9 +111,10 @@ public class FxAccountAuthenticator extends AbstractAccountAuthenticator {
public void succeed(String authToken) {
Logger.info(LOG_TAG, "Responding with success!");
fxAccount.releaseSharedAccountStateLock();
final Bundle result = new Bundle();
result.putString(AccountManager.KEY_ACCOUNT_NAME, account.name);
result.putString(AccountManager.KEY_ACCOUNT_TYPE, account.type);
result.putString(AccountManager.KEY_ACCOUNT_NAME, fxAccount.account.name);
result.putString(AccountManager.KEY_ACCOUNT_TYPE, fxAccount.account.type);
result.putString(AccountManager.KEY_AUTHTOKEN, authToken);
response.onResult(result);
}
@ -179,7 +181,7 @@ public class FxAccountAuthenticator extends AbstractAccountAuthenticator {
protected void getOAuthToken(final AccountAuthenticatorResponse response, final AndroidFxAccount fxAccount, final String scope) throws NetworkErrorException {
Logger.info(LOG_TAG, "Fetching oauth token with scope: " + scope);
final Responder responder = new Responder(response, fxAccount.getAndroidAccount());
final Responder responder = new Responder(response, fxAccount);
final String oauthServerUri = FxAccountConstants.DEFAULT_OAUTH_SERVER_ENDPOINT;
final String audience;
@ -270,14 +272,25 @@ public class FxAccountAuthenticator extends AbstractAccountAuthenticator {
if (authTokenType != null && authTokenType.startsWith(oauthPrefix)) {
final String scope = authTokenType.substring(oauthPrefix.length());
final AndroidFxAccount fxAccount = new AndroidFxAccount(context, account);
try {
fxAccount.acquireSharedAccountStateLock(LOG_TAG);
} catch (InterruptedException e) {
Logger.warn(LOG_TAG, "Could not acquire account state lock; return error bundle.");
final Bundle bundle = new Bundle();
bundle.putInt(AccountManager.KEY_ERROR_CODE, 1);
bundle.putString(AccountManager.KEY_ERROR_MESSAGE, "Could not acquire account state lock.");
return bundle;
}
getOAuthToken(response, fxAccount, scope);
return null;
}
// Otherwise, fail.
Logger.warn(LOG_TAG, "Returning null bundle for getAuthToken.");
return null;
Logger.warn(LOG_TAG, "Returning error bundle for getAuthToken with unknown token type.");
final Bundle bundle = new Bundle();
bundle.putInt(AccountManager.KEY_ERROR_CODE, 2);
bundle.putString(AccountManager.KEY_ERROR_MESSAGE, "Unknown token type: " + authTokenType);
return bundle;
}
@Override

50
mobile/android/base/fxa/sync/FxAccountSyncAdapter.java
View File

@ -115,7 +115,7 @@ public class FxAccountSyncAdapter extends AbstractThreadedSyncAdapter {
protected final Collection<String> stageNamesToSync;
public SyncDelegate(CountDownLatch latch, SyncResult syncResult, AndroidFxAccount fxAccount, Collection<String> stageNamesToSync) {
super(latch, syncResult, fxAccount);
super(latch, syncResult);
this.stageNamesToSync = Collections.unmodifiableCollection(stageNamesToSync);
}
@ -234,7 +234,8 @@ public class FxAccountSyncAdapter extends AbstractThreadedSyncAdapter {
final KeyBundle syncKeyBundle,
final String clientState,
final SessionCallback callback,
final Bundle extras) {
final Bundle extras,
final AndroidFxAccount fxAccount) {
final TokenServerClientDelegate delegate = new TokenServerClientDelegate() {
private boolean didReceiveBackoff = false;
@ -246,6 +247,7 @@ public class FxAccountSyncAdapter extends AbstractThreadedSyncAdapter {
@Override
public void handleSuccess(final TokenServerToken token) {
FxAccountUtils.pii(LOG_TAG, "Got token! uid is " + token.uid + " and endpoint is " + token.endpoint + ".");
fxAccount.releaseSharedAccountStateLock();
if (!didReceiveBackoff) {
// We must be OK to touch this token server.
@ -323,12 +325,24 @@ public class FxAccountSyncAdapter extends AbstractThreadedSyncAdapter {
@Override
public void handleFailure(TokenServerException e) {
handleError(e);
Logger.error(LOG_TAG, "Failed to get token.", e);
try {
// We should only get here *after* we're locked into the married state.
State state = fxAccount.getState();
if (state.getStateLabel() == StateLabel.Married) {
Married married = (Married) state;
fxAccount.setState(married.makeCohabitingState());
}
} finally {
fxAccount.releaseSharedAccountStateLock();
}
callback.handleError(null, e);
}
@Override
public void handleError(Exception e) {
Logger.error(LOG_TAG, "Failed to get token.", e);
fxAccount.releaseSharedAccountStateLock();
callback.handleError(null, e);
}
@ -410,14 +424,6 @@ public class FxAccountSyncAdapter extends AbstractThreadedSyncAdapter {
final SyncDelegate syncDelegate = new SyncDelegate(latch, syncResult, fxAccount, stageNamesToSync);
try {
final State state;
try {
state = fxAccount.getState();
} catch (Exception e) {
syncDelegate.handleError(e);
return;
}
// This will be the same chunk of SharedPreferences that we pass through to GlobalSession/SyncConfiguration.
final SharedPreferences sharedPrefs = fxAccount.getSyncPrefs();
@ -454,6 +460,24 @@ public class FxAccountSyncAdapter extends AbstractThreadedSyncAdapter {
final URI tokenServerEndpointURI = new URI(tokenServerEndpoint);
final String audience = FxAccountUtils.getAudienceForURL(tokenServerEndpoint);
try {
// The clock starts... now!
fxAccount.acquireSharedAccountStateLock(FxAccountSyncAdapter.LOG_TAG);
} catch (InterruptedException e) {
// OK, skip this sync.
syncDelegate.handleError(e);
return;
}
final State state;
try {
state = fxAccount.getState();
} catch (Exception e) {
fxAccount.releaseSharedAccountStateLock();
syncDelegate.handleError(e);
return;
}
final FxAccountLoginStateMachine stateMachine = new FxAccountLoginStateMachine();
stateMachine.advance(state, StateLabel.Married, new FxADefaultLoginStateMachineDelegate(context, fxAccount) {
@Override
@ -505,7 +529,7 @@ public class FxAccountSyncAdapter extends AbstractThreadedSyncAdapter {
final SessionCallback sessionCallback = new SessionCallback(syncDelegate, schedulePolicy);
final KeyBundle syncKeyBundle = married.getSyncKeyBundle();
final String clientState = married.getClientState();
syncWithAssertion(audience, assertion, tokenServerEndpointURI, tokenBackoffHandler, sharedPrefs, syncKeyBundle, clientState, sessionCallback, extras);
syncWithAssertion(audience, assertion, tokenServerEndpointURI, tokenBackoffHandler, sharedPrefs, syncKeyBundle, clientState, sessionCallback, extras, fxAccount);
} catch (Exception e) {
syncDelegate.handleError(e);
return;
@ -517,6 +541,8 @@ public class FxAccountSyncAdapter extends AbstractThreadedSyncAdapter {
} catch (Exception e) {
Logger.error(LOG_TAG, "Got error syncing.", e);
syncDelegate.handleError(e);
} finally {
fxAccount.releaseSharedAccountStateLock();
}
Logger.info(LOG_TAG, "Syncing done.");

21
mobile/android/base/fxa/sync/FxAccountSyncDelegate.java
View File

@ -6,32 +6,23 @@ package org.mozilla.gecko.fxa.sync;
import java.util.concurrent.CountDownLatch;
import org.mozilla.gecko.fxa.authenticator.AndroidFxAccount;
import org.mozilla.gecko.fxa.login.Married;
import org.mozilla.gecko.fxa.login.State;
import org.mozilla.gecko.fxa.login.State.StateLabel;
import org.mozilla.gecko.tokenserver.TokenServerException;
import android.content.SyncResult;
public class FxAccountSyncDelegate {
protected final CountDownLatch latch;
protected final SyncResult syncResult;
protected final AndroidFxAccount fxAccount;
public FxAccountSyncDelegate(CountDownLatch latch, SyncResult syncResult, AndroidFxAccount fxAccount) {
public FxAccountSyncDelegate(CountDownLatch latch, SyncResult syncResult) {
if (latch == null) {
throw new IllegalArgumentException("latch must not be null");
}
if (syncResult == null) {
throw new IllegalArgumentException("syncResult must not be null");
}
if (fxAccount == null) {
throw new IllegalArgumentException("fxAccount must not be null");
}
this.latch = latch;
this.syncResult = syncResult;
this.fxAccount = fxAccount;
}
/**
@ -65,16 +56,6 @@ public class FxAccountSyncDelegate {
public void handleError(Exception e) {
setSyncResultSoftError();
// This is awful, but we need to propagate bad assertions back up the
// chain somehow, and this will do for now.
if (e instanceof TokenServerException) {
// We should only get here *after* we're locked into the married state.
State state = fxAccount.getState();
if (state.getStateLabel() == StateLabel.Married) {
Married married = (Married) state;
fxAccount.setState(married.makeCohabitingState());
}
}
latch.countDown();
}

2
mobile/android/base/reading/ReadingListSyncAdapter.java
View File

@ -166,7 +166,7 @@ public class ReadingListSyncAdapter extends AbstractThreadedSyncAdapter {
final AndroidFxAccount fxAccount = new AndroidFxAccount(context, account);
final CountDownLatch latch = new CountDownLatch(1);
final FxAccountSyncDelegate syncDelegate = new FxAccountSyncDelegate(latch, syncResult, fxAccount);
final FxAccountSyncDelegate syncDelegate = new FxAccountSyncDelegate(latch, syncResult);
final AccountManager accountManager = AccountManager.get(context);
// If we have an auth failure that requires user intervention, FxA will show system