Commit 498f5876 authored by Pâris MEULEMAN's avatar Pâris MEULEMAN Committed by Commit Bot

Use PrimaryAccountMutator in SigninManagerAndroid

Use the public API of IdentityManager, PrimaryAccountMutator, to set the
primary account instead of using IdentityManager's internals
(PrimaryAccountManager).

PrimaryAccountMutatorImpl::SetPrimaryAccount calls PrimaryAccountManager::Signin, with added pre-conditions:
  - Preferences allow the signin
  - There is no primary account already set (IsAuthenticated)
  - AccountTrackerService knows about the account, and it has both a
    valid account_id and email.
If those are not met, SetPrimaryAccount does not call PrimaryAccountManager::SignIn and returns false.

In practice, those are already met or were already required:
  - Most of SignIn calls do check the preferences beforehand. Two
    exceptions: Re-authentication cases (SyncAndServicesPreferences, SignInHelper), SigninFragment.
  - PrimaryAccountManager::Signin and
    PrimaryAccountManager::SetAuthenticatedAccountId also verify that.
    If the user is authenticated, the signin is not completed, no
    notifications are sent. A slight behavior change could happen here
    though: a DCHECK verifies that the same user is re-authenticating.
  - PrimaryAccountManager::SignIn also needs AccountTrackerService to be
    properly initialized with the account.

Bug: 987965
Change-Id: I70f30bcf3536a79bb5983333233c59aeb5b8a5a1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1738696
Commit-Queue: Pâris Meuleman <pmeuleman@chromium.org>
Reviewed-by: default avatarSylvain Defresne <sdefresne@chromium.org>
Reviewed-by: default avatarBoris Sazonov <bsazonov@chromium.org>
Auto-Submit: Pâris Meuleman <pmeuleman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#685149}
parent 87b0923f
...@@ -464,7 +464,7 @@ public class SigninManager implements AccountTrackerService.OnSystemAccountsSeed ...@@ -464,7 +464,7 @@ public class SigninManager implements AccountTrackerService.OnSystemAccountsSeed
// This method should be called at most once per sign-in flow. // This method should be called at most once per sign-in flow.
assert mSignInState != null; assert mSignInState != null;
SigninManagerJni.get().onSignInCompleted( SigninManagerJni.get().setPrimaryAccount(
mNativeSigninManagerAndroid, mSignInState.mAccount.name); mNativeSigninManagerAndroid, mSignInState.mAccount.name);
// Cache the signed-in account name. This must be done after the native call, otherwise // Cache the signed-in account name. This must be done after the native call, otherwise
...@@ -744,7 +744,7 @@ public class SigninManager implements AccountTrackerService.OnSystemAccountsSeed ...@@ -744,7 +744,7 @@ public class SigninManager implements AccountTrackerService.OnSystemAccountsSeed
boolean isForceSigninEnabled(long nativeSigninManagerAndroid); boolean isForceSigninEnabled(long nativeSigninManagerAndroid);
void onSignInCompleted(long nativeSigninManagerAndroid, String username); void setPrimaryAccount(long nativeSigninManagerAndroid, String username);
void signOut(long nativeSigninManagerAndroid, @SignoutReason int reason); void signOut(long nativeSigninManagerAndroid, @SignoutReason int reason);
......
...@@ -219,7 +219,7 @@ public class SigninManagerTest { ...@@ -219,7 +219,7 @@ public class SigninManagerTest {
doNothing().when(mNativeMock).fetchAndApplyCloudPolicy(anyLong(), any(), any()); doNothing().when(mNativeMock).fetchAndApplyCloudPolicy(anyLong(), any(), any());
doReturn(true).when(mSigninManager).isSigninSupported(); doReturn(true).when(mSigninManager).isSigninSupported();
doNothing().when(mNativeMock).onSignInCompleted(anyLong(), any()); doNothing().when(mNativeMock).setPrimaryAccount(anyLong(), any());
doNothing().when(mSigninManager).logInSignedInUser(); doNothing().when(mSigninManager).logInSignedInUser();
mSigninManager.onFirstRunCheckDone(); // Allow sign-in. mSigninManager.onFirstRunCheckDone(); // Allow sign-in.
......
...@@ -162,16 +162,19 @@ void SigninManagerAndroid::Shutdown() { ...@@ -162,16 +162,19 @@ void SigninManagerAndroid::Shutdown() {
java_signin_manager_); java_signin_manager_);
} }
void SigninManagerAndroid::OnSignInCompleted( void SigninManagerAndroid::SetPrimaryAccount(
JNIEnv* env, JNIEnv* env,
const JavaParamRef<jstring>& username) { const JavaParamRef<jstring>& username) {
DVLOG(1) << "SigninManagerAndroid::OnSignInCompleted"; DVLOG(1) << "SigninManagerAndroid::SetPrimaryAccount";
// TODO(crbug.com/987965): Migrate away from this direct usage of auto account =
// PrimaryAccountManager and eliminate this class needing to know about identity_manager_
// PrimaryAccountManager. ->FindExtendedAccountInfoForAccountWithRefreshTokenByEmailAddress(
identity_manager_->GetPrimaryAccountManager()->SignIn( base::android::ConvertJavaStringToUTF8(env, username));
base::android::ConvertJavaStringToUTF8(env, username)); DCHECK(account.has_value());
auto* account_mutator = identity_manager_->GetPrimaryAccountMutator();
bool result = account_mutator->SetPrimaryAccount(account->account_id);
CHECK(result);
} }
void SigninManagerAndroid::SignOut(JNIEnv* env, void SigninManagerAndroid::SignOut(JNIEnv* env,
......
...@@ -44,7 +44,7 @@ class SigninManagerAndroid : public KeyedService, ...@@ -44,7 +44,7 @@ class SigninManagerAndroid : public KeyedService,
// Indicates that the user has made the choice to sign-in. |username| // Indicates that the user has made the choice to sign-in. |username|
// contains the email address of the account to use as primary. // contains the email address of the account to use as primary.
void OnSignInCompleted(JNIEnv* env, void SetPrimaryAccount(JNIEnv* env,
const base::android::JavaParamRef<jstring>& username); const base::android::JavaParamRef<jstring>& username);
void SignOut(JNIEnv* env, void SignOut(JNIEnv* env,
......
...@@ -33,9 +33,7 @@ class SharedURLLoaderFactory; ...@@ -33,9 +33,7 @@ class SharedURLLoaderFactory;
class TestURLLoaderFactory; class TestURLLoaderFactory;
} // namespace network } // namespace network
// Necessary to declare these classes as friends.
class PrefRegistrySimple; class PrefRegistrySimple;
class SigninManagerAndroid;
class AccountFetcherService; class AccountFetcherService;
class AccountTrackerService; class AccountTrackerService;
...@@ -537,10 +535,6 @@ class IdentityManager : public KeyedService, ...@@ -537,10 +535,6 @@ class IdentityManager : public KeyedService,
const std::string& locale, const std::string& locale,
const std::string& picture_url); const std::string& picture_url);
// These friends are temporary during the conversion process.
// TODO(https://crbug.com/889902): Delete this when conversion is done.
friend SigninManagerAndroid;
// Temporary access to getters (e.g. GetTokenService()). // Temporary access to getters (e.g. GetTokenService()).
// TODO(https://crbug.com/944127): Remove this friendship by // TODO(https://crbug.com/944127): Remove this friendship by
// extending identity_test_utils.h as needed. // extending identity_test_utils.h as needed.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment