Skip to content

Commit

Permalink
Don't sign out Unicorn users when account cookies are cleared
Browse files Browse the repository at this point in the history 
This is required once the signed-in-non-syncing state is supported for
Unicorn users, as otherwise we attempt to sign them out if they clear
cookies, or as part of the 'Turn off sync' flow.  Doing that leads to a
race condition between SigninChecker and the account reconciler.

Bug: 1294761
Change-Id: I17a0f91f2c474c18c37caf793ab513d06f36677c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3629407
Reviewed-by: Alice Wang <aliceywang@chromium.org>
Commit-Queue: James Lee <ljjlee@google.com>
Auto-Submit: James Lee <ljjlee@google.com>
Cr-Commit-Position: refs/heads/main@{#1002500}
  • Loading branch information
James Lee authored and Chromium LUCI CQ committed May 12, 2022
1 parent 361062f commit 3b52fd8
Show file tree
Hide file tree
Showing 2 changed files with 97 additions and 7 deletions.
37 changes: 31 additions & 6 deletions chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManagerImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,14 @@
import org.chromium.chrome.browser.browsing_data.BrowsingDataBridge;
import org.chromium.chrome.browser.browsing_data.BrowsingDataType;
import org.chromium.chrome.browser.browsing_data.TimePeriod;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.signin.services.SigninManager;
import org.chromium.chrome.browser.signin.services.SigninPreferencesManager;
import org.chromium.chrome.browser.sync.SyncService;
import org.chromium.components.externalauth.ExternalAuthUtils;
import org.chromium.components.signin.AccountManagerFacade;
import org.chromium.components.signin.AccountManagerFacadeProvider;
import org.chromium.components.signin.AccountUtils;
import org.chromium.components.signin.base.CoreAccountId;
import org.chromium.components.signin.base.CoreAccountInfo;
import org.chromium.components.signin.identitymanager.AccountInfoServiceProvider;
Expand Down Expand Up @@ -584,12 +588,33 @@ private void onSigninAllowedByPolicyChanged(boolean newSigninAllowedByPolicy) {

@Override
public void onAccountsCookieDeletedByUserAction() {
if (mIdentityManager.getPrimaryAccountInfo(ConsentLevel.SIGNIN) != null
&& mIdentityManager.getPrimaryAccountInfo(ConsentLevel.SYNC) == null) {
// Clearing account cookies should trigger sign-out only when user is signed in
// without sync.
// If the user consented for sync, then the user should not be signed out,
// since account cookies will be rebuilt by the account reconcilor.
// Clearing account cookies should trigger sign-out only when user is
// signed in without sync. If the user consented for sync, then the user
// should not be signed out, since account cookies will be rebuilt by
// the account reconcilor.
if (mIdentityManager.getPrimaryAccountInfo(ConsentLevel.SIGNIN) == null
|| mIdentityManager.getPrimaryAccountInfo(ConsentLevel.SYNC) != null) {
return;
}

if (ChromeFeatureList.isEnabled(ChromeFeatureList.ALLOW_SYNC_OFF_FOR_CHILD_ACCOUNTS)) {
// Child users are not allowed to sign out, so we check the child status in order to
// skip any signout step. This is guarded behind a flag for now, in case changing the
// timings by adding an async step causes any issues for non-child accounts.
//
// TODO(crbug.com/1324567): move this logic within signOut() rather than relying on
// callers like SigninManager implemting this logic.
final AccountManagerFacade accountManagerFacade =
AccountManagerFacadeProvider.getInstance();
accountManagerFacade.getAccounts().then(accounts -> {
AccountUtils.checkChildAccountStatus(
accountManagerFacade, accounts, (isChild, childAccount) -> {
if (!isChild) {
signOut(SignoutReason.USER_DELETED_ACCOUNT_COOKIES);
}
});
});
} else {
signOut(SignoutReason.USER_DELETED_ACCOUNT_COOKIES);
}
}
Expand Down
67 changes: 66 additions & 1 deletion chrome/android/junit/src/org/chromium/chrome/browser/signin/SigninManagerImplTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,19 +21,26 @@
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TestRule;
import org.junit.runner.RunWith;
import org.mockito.stubbing.Answer;
import org.robolectric.annotation.LooperMode;

import org.chromium.base.test.BaseRobolectricTestRunner;
import org.chromium.base.test.util.JniMocker;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.signin.services.SigninManager;
import org.chromium.chrome.browser.sync.SyncService;
import org.chromium.chrome.test.util.browser.Features;
import org.chromium.chrome.test.util.browser.Features.DisableFeatures;
import org.chromium.chrome.test.util.browser.Features.EnableFeatures;
import org.chromium.components.externalauth.ExternalAuthUtils;
import org.chromium.components.signin.AccountManagerFacadeProvider;
import org.chromium.components.signin.AccountUtils;
import org.chromium.components.signin.base.AccountCapabilities;
import org.chromium.components.signin.base.AccountInfo;
import org.chromium.components.signin.base.CoreAccountId;
import org.chromium.components.signin.base.CoreAccountInfo;
import org.chromium.components.signin.identitymanager.AccountInfoServiceProvider;
import org.chromium.components.signin.identitymanager.AccountTrackerService;
import org.chromium.components.signin.identitymanager.ConsentLevel;
Expand All @@ -44,6 +51,7 @@
import org.chromium.components.signin.metrics.SigninAccessPoint;
import org.chromium.components.signin.metrics.SignoutDelete;
import org.chromium.components.signin.metrics.SignoutReason;
import org.chromium.components.signin.test.util.FakeAccountManagerFacade;
import org.chromium.components.sync.ModelType;

import java.util.HashMap;
Expand All @@ -59,6 +67,13 @@ public class SigninManagerImplTest {
private static final AccountInfo ACCOUNT_INFO =
new AccountInfo(new CoreAccountId("gaia-id-user"), "user@domain.com", "gaia-id-user",
"full name", "given name", null, new AccountCapabilities(new HashMap<>()));
private static final CoreAccountInfo CHILD_CORE_ACCOUNT_INFO =
CoreAccountInfo.createFromEmailAndGaiaId(
FakeAccountManagerFacade.generateChildEmail("user@domain.com"),
"child-gaia-id-user");

@Rule
public final TestRule mFeaturesProcessorRule = new Features.JUnitProcessor();

@Rule
public final JniMocker mocker = new JniMocker();
Expand All @@ -73,6 +88,8 @@ public class SigninManagerImplTest {

private final IdentityManager mIdentityManager =
IdentityManager.create(NATIVE_IDENTITY_MANAGER, null /* OAuth2TokenService */);
private final FakeAccountManagerFacade mFakeAccountManagerFacade =
new FakeAccountManagerFacade();
private SigninManagerImpl mSigninManager;

@Before
Expand All @@ -96,6 +113,8 @@ public void setUp() {
NATIVE_IDENTITY_MANAGER, ACCOUNT_INFO.getEmail()))
.thenReturn(ACCOUNT_INFO);

AccountManagerFacadeProvider.setInstanceForTests(mFakeAccountManagerFacade);

mSigninManager = (SigninManagerImpl) SigninManagerImpl.create(
NATIVE_SIGNIN_MANAGER, mAccountTrackerService, mIdentityManager, mIdentityMutator);
}
Expand Down Expand Up @@ -352,7 +371,11 @@ public void revokeSyncConsentFromJavaWithNullDomain() {
}

@Test
@EnableFeatures({ChromeFeatureList.ALLOW_SYNC_OFF_FOR_CHILD_ACCOUNTS})
public void clearingAccountCookieDoesNotTriggerSignoutWhenUserIsSignedOut() {
mFakeAccountManagerFacade.addAccount(
AccountUtils.createAccountFromName(ACCOUNT_INFO.getEmail()));

mIdentityManager.onAccountsCookieDeletedByUserAction();

verify(mIdentityMutator, never()).clearPrimaryAccount(anyInt(), anyInt());
Expand All @@ -361,10 +384,13 @@ public void clearingAccountCookieDoesNotTriggerSignoutWhenUserIsSignedOut() {
}

@Test
@EnableFeatures({ChromeFeatureList.ALLOW_SYNC_OFF_FOR_CHILD_ACCOUNTS})
public void clearingAccountCookieDoesNotTriggerSignoutWhenUserIsSignedInAndSync() {
when(mIdentityManagerNativeMock.getPrimaryAccountInfo(
eq(NATIVE_IDENTITY_MANAGER), anyInt()))
.thenReturn(ACCOUNT_INFO);
mFakeAccountManagerFacade.addAccount(
AccountUtils.createAccountFromName(ACCOUNT_INFO.getEmail()));

mIdentityManager.onAccountsCookieDeletedByUserAction();

Expand All @@ -374,10 +400,13 @@ public void clearingAccountCookieDoesNotTriggerSignoutWhenUserIsSignedInAndSync(
}

@Test
public void clearingAccountCookieTriggersSignoutWhenUserIsSignedInWithoutSync() {
@EnableFeatures({ChromeFeatureList.ALLOW_SYNC_OFF_FOR_CHILD_ACCOUNTS})
public void clearingAccountCookieTriggersSignoutWhenNormalUserIsSignedInWithoutSync() {
when(mIdentityManagerNativeMock.getPrimaryAccountInfo(
NATIVE_IDENTITY_MANAGER, ConsentLevel.SIGNIN))
.thenReturn(ACCOUNT_INFO);
mFakeAccountManagerFacade.addAccount(
AccountUtils.createAccountFromName(ACCOUNT_INFO.getEmail()));

mIdentityManager.onAccountsCookieDeletedByUserAction();

Expand All @@ -389,6 +418,42 @@ public void clearingAccountCookieTriggersSignoutWhenUserIsSignedInWithoutSync()
verify(mNativeMock, never()).wipeGoogleServiceWorkerCaches(anyLong(), any());
}

@Test
@DisableFeatures({ChromeFeatureList.ALLOW_SYNC_OFF_FOR_CHILD_ACCOUNTS})
public void clearingAccountCookieTriggersSignoutWhenSupervisedUserIsSignedInWithoutSync() {
when(mIdentityManagerNativeMock.getPrimaryAccountInfo(
NATIVE_IDENTITY_MANAGER, ConsentLevel.SIGNIN))
.thenReturn(CHILD_CORE_ACCOUNT_INFO);
mFakeAccountManagerFacade.addAccount(
AccountUtils.createAccountFromName(CHILD_CORE_ACCOUNT_INFO.getEmail()));

mIdentityManager.onAccountsCookieDeletedByUserAction();

verify(mIdentityMutator)
.clearPrimaryAccount(
SignoutReason.USER_DELETED_ACCOUNT_COOKIES, SignoutDelete.IGNORE_METRIC);
// Sign-out triggered by wiping account cookies shouldn't wipe data.
verify(mNativeMock, never()).wipeProfileData(anyLong(), any());
verify(mNativeMock, never()).wipeGoogleServiceWorkerCaches(anyLong(), any());
}

@Test
@EnableFeatures({ChromeFeatureList.ALLOW_SYNC_OFF_FOR_CHILD_ACCOUNTS})
public void
clearingAccountCookieDoesNotTriggerSignoutWhenSupervisedUserIsSignedInWithoutSync() {
when(mIdentityManagerNativeMock.getPrimaryAccountInfo(
NATIVE_IDENTITY_MANAGER, ConsentLevel.SIGNIN))
.thenReturn(CHILD_CORE_ACCOUNT_INFO);
mFakeAccountManagerFacade.addAccount(
AccountUtils.createAccountFromName(CHILD_CORE_ACCOUNT_INFO.getEmail()));

mIdentityManager.onAccountsCookieDeletedByUserAction();

verify(mIdentityMutator, never()).clearPrimaryAccount(anyInt(), anyInt());
verify(mNativeMock, never()).wipeProfileData(anyLong(), any());
verify(mNativeMock, never()).wipeGoogleServiceWorkerCaches(anyLong(), any());
}

@Test
public void callbackNotifiedWhenNoOperationIsInProgress() {
AtomicInteger callCount = new AtomicInteger(0);
Expand Down

0 comments on commit 3b52fd8

Please sign in to comment.