0

[iOS][MIM2] Use AuthenticationFlow in the account menu

Switching the account menu to use AuthenticationFlow since it now
supports profile switching.

This patch also update AuthenticationFlow to set the account switching
flag from AuthenticationService.

In a later patch, AuthenticationService needs to send a notification
when the flag is reset.
TODO: http://crbug.com/397194707.

Bug: 375604649
Change-Id: I06c691e7863aa3f14dcda3cbcf263dbb9c6a906e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6275964
Auto-Submit: Jérôme Lebel <jlebel@chromium.org>
Commit-Queue: Jérôme Lebel <jlebel@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1421131}
This commit is contained in:
Jérôme Lebel
2025-02-17 12:06:06 -08:00
committed by Chromium LUCI CQ
parent 694c756660
commit 1d9c8d59ae
6 changed files with 221 additions and 153 deletions

@ -65,62 +65,6 @@
#import "ios/chrome/grit/ios_strings.h"
#import "ui/base/l10n/l10n_util.h"
namespace {
// First part of a switch-account-after-switching-profile continuation: Sign out
// the current account if it's different from the desired one.
void ChangeProfileSignOutIfMismatchContinuation(
id<SystemIdentity> expected_identity,
SceneState* scene_state,
base::OnceClosure closure) {
Browser* browser =
scene_state.browserProviderInterface.mainBrowserProvider.browser;
AuthenticationService* authentication_service =
AuthenticationServiceFactory::GetForProfile(browser->GetProfile());
id<SystemIdentity> existing_identity =
authentication_service->GetPrimaryIdentity(signin::ConsentLevel::kSignin);
if (!existing_identity || existing_identity == expected_identity) {
// No need to sign-out as either the correct identity is signed-in or
// no identity is signed-in.
std::move(closure).Run();
return;
}
// Signing out is only allowed in the personal profile. Phrased another way,
// we shouldn't try to a work profile with a non-matching account.
// TODO(crbug.com/375605174): AuthenticationService should probably enforce
// this internally.
CHECK_EQ(browser->GetProfile()->GetProfileName(),
GetApplicationContext()
->GetAccountProfileMapper()
->GetPersonalProfileName());
authentication_service->SignOut(
signin_metrics::ProfileSignout::kChangeAccountInAccountMenu,
base::CallbackToBlock(std::move(closure)));
}
// Second part of a switch-account-after-switching-profile continuation: Sign in
// the desired account if it's not already signed in.
void ChangeProfileSignInContinuation(id<SystemIdentity> identity,
SceneState* scene_state,
base::OnceClosure closure) {
Browser* browser =
scene_state.browserProviderInterface.mainBrowserProvider.browser;
// TODO(crbug.com/375604649): This should probably go through
// AuthenticationFlow rather than using AuthenticationService directly, so
// that the snackbar gets shown, and also the enterprise onboarding screen if
// necessary.
AuthenticationService* authentication_service =
AuthenticationServiceFactory::GetForProfile(browser->GetProfile());
authentication_service->SignIn(identity,
signin_metrics::AccessPoint::kAccountMenu);
std::move(closure).Run();
}
} // anonymous namespace
@interface AccountMenuCoordinator () <AccountMenuMediatorDelegate,
ManageAccountsCoordinatorDelegate,
UIAdaptivePresentationControllerDelegate>
@ -149,7 +93,6 @@ void ChangeProfileSignInContinuation(id<SystemIdentity> identity,
SyncEncryptionPassphraseTableViewController*
_syncEncryptionPassphraseTableViewController;
id<ApplicationCommands> _applicationHandler;
id<ChangeProfileCommands> _changeProfileHandler;
raw_ptr<ChromeAccountManagerService> _accountManagerService;
// Callback to hide the activity overlay.
base::ScopedClosureRunner _activityOverlayCallback;
@ -182,9 +125,6 @@ void ChangeProfileSignInContinuation(id<SystemIdentity> identity,
_prefService = profile->GetPrefs();
_applicationHandler = HandlerForProtocol(self.browser->GetCommandDispatcher(),
ApplicationCommands);
_changeProfileHandler = HandlerForProtocol(
self.browser->GetSceneState().profileState.appState.appCommandDispatcher,
ChangeProfileCommands);
_viewController = [[AccountMenuViewController alloc] init];
@ -323,20 +263,6 @@ void ChangeProfileSignInContinuation(id<SystemIdentity> identity,
[_signoutActionSheetCoordinator start];
}
- (void)triggerProfileSwitchToProfileNamed:(std::string_view)profileName
andSigninWithSystemIdentity:(id<SystemIdentity>)identity {
CHECK(AreSeparateProfilesForManagedAccountsEnabled());
SceneState* sceneState = self.browser->GetSceneState();
ChangeProfileContinuation continuation = ChainChangeProfileContinuations(
base::BindOnce(&ChangeProfileSignOutIfMismatchContinuation, identity),
base::BindOnce(&ChangeProfileSignInContinuation, identity));
[_changeProfileHandler changeProfile:profileName
forScene:sceneState
continuation:std::move(continuation)];
}
- (void)didTapAddAccountWithCompletion:
(SigninCoordinatorCompletionCallback)completion {
[self openAddAccountWithBaseViewController:_navigationController

@ -18,6 +18,7 @@
#import "ios/chrome/browser/authentication/ui_bundled/account_menu/account_menu_data_source.h"
#import "ios/chrome/browser/authentication/ui_bundled/account_menu/account_menu_mediator_delegate.h"
#import "ios/chrome/browser/authentication/ui_bundled/account_menu/account_menu_view_controller.h"
#import "ios/chrome/browser/authentication/ui_bundled/authentication_flow/authentication_flow.h"
#import "ios/chrome/browser/authentication/ui_bundled/cells/table_view_account_item.h"
#import "ios/chrome/browser/authentication/ui_bundled/enterprise/enterprise_utils.h"
#import "ios/chrome/browser/authentication/ui_bundled/signin/signin_constants.h"
@ -325,25 +326,22 @@
_blockUpdates = YES;
self.userInteractionsBlocked = YES;
__weak __typeof(self) weakSelf = self;
id<SystemIdentity> fromIdentity = _primaryIdentity;
// TODO(crbug.com/375604649): Need to use AuthenticationFlow in both cases.
if (AreSeparateProfilesForManagedAccountsEnabled()) {
std::optional<std::string> profileName =
GetApplicationContext()
->GetAccountProfileMapper()
->FindProfileNameForGaiaID(GaiaId(gaiaID));
if (profileName &&
*profileName != _accountManagerService->GetProfileName()) {
// TODO(crbug.com/375604649): Unblock the UI (and show some error?) if
// switching failed.
[self.delegate triggerProfileSwitchToProfileNamed:*profileName
andSigninWithSystemIdentity:newIdentity];
return;
}
_authenticationFlow = [self.delegate
triggerSigninWithSystemIdentity:newIdentity
completion:^(SigninCoordinatorResult result) {
[weakSelf signinEndedWithResult:result
fromIdentity:fromIdentity
toIdentity:newIdentity];
}];
return;
}
_accountSwitchInProgress =
_authenticationService->DeclareAccountSwitchInProgress();
__weak __typeof(self) weakSelf = self;
id<SystemIdentity> fromIdentity = _primaryIdentity;
[self.delegate signOutFromTargetRect:targetRect
forSwitch:YES
completion:^(BOOL success) {

@ -40,10 +40,6 @@
forSwitch:(BOOL)forSwith
completion:(void (^)(BOOL))completion;
// Requests a switch to the profile with the given `profileName`.
- (void)triggerProfileSwitchToProfileNamed:(std::string_view)profileName
andSigninWithSystemIdentity:(id<SystemIdentity>)identity;
// Shows https://myaccount.google.com/ for the account currently signed-in
// to Chrome. The content is displayed in a new view in the stack, i.e.
// it doesn't close the current view.

@ -344,24 +344,52 @@ TEST_P(AccountMenuMediatorTest, TestAccountTapedSignoutFailed) {
// This variable will contain the callback that should be executed once
// sign-out ends.
__block signin_ui::SignoutCompletionCallback signoutCallback = nil;
__block signin_ui::SigninCompletionCallback signinCallback = nil;
const CGRect target = CGRect();
OCMExpect([delegate_mock_
signOutFromTargetRect:target
forSwitch:YES
completion:[OCMArg checkWithBlock:^BOOL(id value) {
signoutCallback = value;
return true;
}]]);
OCMExpect([consumer_mock_ switchingStarted]);
OCMExpect([consumer_mock_ setUserInteractionsEnabled:NO]);
switch (GetParam()) {
case kOldApiWithoutSeparateProfiles:
case kNewApiWithoutSeparateProfiles:
OCMExpect([delegate_mock_
signOutFromTargetRect:target
forSwitch:YES
completion:[OCMArg checkWithBlock:^BOOL(id value) {
signoutCallback = value;
return true;
}]]);
break;
case kNewApiWithSeparateProfiles:
OCMExpect(
[delegate_mock_
triggerSigninWithSystemIdentity:kSecondaryIdentity
completion:[OCMArg checkWithBlock:^BOOL(
id value) {
signinCallback = value;
return true;
}]])
.andReturn(authentication_flow_mock_);
break;
}
[mediator_ accountTappedWithGaiaID:kSecondaryIdentity.gaiaID
targetRect:target];
VerifyMock();
OCMExpect([consumer_mock_ switchingStopped]);
OCMExpect([consumer_mock_ setUserInteractionsEnabled:YES]);
// Simulate a sign-out failure
signoutCallback(false);
switch (GetParam()) {
case kOldApiWithoutSeparateProfiles:
case kNewApiWithoutSeparateProfiles:
// Simulate a sign-out failure.
signoutCallback(false);
EXPECT_EQ(signinCallback, nil);
break;
case kNewApiWithSeparateProfiles:
// Simulate AuthenticationFlow failure.
signinCallback(SigninCoordinatorResultCanceledByUser);
EXPECT_EQ(signoutCallback, nil);
break;
}
}
// Tests the result of accountTappedWithGaiaID:targetRect:
@ -375,26 +403,35 @@ TEST_P(AccountMenuMediatorTest, TestAccountTapedSignInFailed) {
// This variable will contain the callback that should be executed once
// sign-out ends.
__block signin_ui::SignoutCompletionCallback signoutCallback = nil;
__block signin_ui::SigninCompletionCallback signinCallback = nil;
const CGRect target = CGRect();
OCMExpect([delegate_mock_
signOutFromTargetRect:target
forSwitch:YES
completion:[OCMArg checkWithBlock:^BOOL(id value) {
signoutCallback = value;
// Actually sign-out, in order to test next step.
SignOut();
return true;
}]]);
OCMExpect([consumer_mock_ switchingStarted]);
OCMExpect([consumer_mock_ setUserInteractionsEnabled:NO]);
[mediator_ accountTappedWithGaiaID:kSecondaryIdentity.gaiaID
targetRect:target];
VerifyMock();
switch (GetParam()) {
case kOldApiWithoutSeparateProfiles:
case kNewApiWithoutSeparateProfiles:
OCMExpect([delegate_mock_
signOutFromTargetRect:target
forSwitch:YES
completion:[OCMArg checkWithBlock:^BOOL(id value) {
signoutCallback = value;
// Actually sign-out, in order to test next step.
SignOut();
return true;
}]]);
[mediator_ accountTappedWithGaiaID:kSecondaryIdentity.gaiaID
targetRect:target];
VerifyMock();
break;
case kNewApiWithSeparateProfiles:
// Sign-out is not done by the mediator but by AuthenticationFlow.
// This step can be skipped.
break;
}
// Simulate a sign-out success.
// This variable will contain the callback that should be executed once
// sign-in ended.
__block signin_ui::SigninCompletionCallback signinCallback = nil;
OCMExpect([delegate_mock_
triggerSigninWithSystemIdentity:kSecondaryIdentity
completion:[OCMArg checkWithBlock:^BOOL(
@ -403,12 +440,23 @@ TEST_P(AccountMenuMediatorTest, TestAccountTapedSignInFailed) {
return true;
}]])
.andReturn(authentication_flow_mock_);
signoutCallback(true);
switch (GetParam()) {
case kOldApiWithoutSeparateProfiles:
case kNewApiWithoutSeparateProfiles:
signoutCallback(true);
// Testing the sign-in callback.
// The delegate should not receive any message. The mediator directly sign
// the user back in the previous account.
OCMExpect([consumer_mock_ updatePrimaryAccount]);
break;
case kNewApiWithSeparateProfiles:
// Simulate account switching.
[mediator_ accountTappedWithGaiaID:kSecondaryIdentity.gaiaID
targetRect:target];
break;
}
// Testing the sign-in callback.
// The delegate should not receive any message. The mediator directly sign the
// user back in the previous account.
OCMExpect([consumer_mock_ updatePrimaryAccount]);
// Expect that the consumer unlocks the UI.
OCMExpect([consumer_mock_ switchingStopped]);
OCMExpect([consumer_mock_ setUserInteractionsEnabled:YES]);
signinCallback(SigninCoordinatorResult::SigninCoordinatorResultInterrupted);
@ -429,33 +477,52 @@ TEST_P(AccountMenuMediatorTest, TestAccountTapedWithSuccessfulSwitch) {
// This variable will contain the callback that should be executed once
// sign-out ends.
__block signin_ui::SignoutCompletionCallback signoutCallback = nil;
__block signin_ui::SigninCompletionCallback signinCallback = nil;
const CGRect target = CGRect();
OCMExpect([delegate_mock_
signOutFromTargetRect:target
forSwitch:YES
completion:[OCMArg checkWithBlock:^BOOL(id value) {
signoutCallback = value;
return true;
}]]);
OCMExpect([consumer_mock_ switchingStarted]);
OCMExpect([consumer_mock_ setUserInteractionsEnabled:NO]);
[mediator_ accountTappedWithGaiaID:kSecondaryIdentity.gaiaID
targetRect:target];
VerifyMock();
// Simulate a sign-out success.
// This variable will contain the callback that should be executed once
// sign-in ends.
__block signin_ui::SigninCompletionCallback signinCallback = nil;
OCMExpect([delegate_mock_
triggerSigninWithSystemIdentity:kSecondaryIdentity
completion:[OCMArg checkWithBlock:^BOOL(
id value) {
signinCallback = value;
return true;
}]])
.andReturn(authentication_flow_mock_);
signoutCallback(true);
switch (GetParam()) {
case kOldApiWithoutSeparateProfiles:
case kNewApiWithoutSeparateProfiles:
OCMExpect([delegate_mock_
signOutFromTargetRect:target
forSwitch:YES
completion:[OCMArg checkWithBlock:^BOOL(id value) {
signoutCallback = value;
return true;
}]]);
[mediator_ accountTappedWithGaiaID:kSecondaryIdentity.gaiaID
targetRect:target];
VerifyMock();
// Simulate a sign-out success.
// This variable will contain the callback that should be executed once
// sign-in ends.
OCMExpect(
[delegate_mock_
triggerSigninWithSystemIdentity:kSecondaryIdentity
completion:[OCMArg checkWithBlock:^BOOL(
id value) {
signinCallback = value;
return true;
}]])
.andReturn(authentication_flow_mock_);
signoutCallback(true);
break;
case kNewApiWithSeparateProfiles:
// Simulate account switching.
OCMExpect(
[delegate_mock_
triggerSigninWithSystemIdentity:kSecondaryIdentity
completion:[OCMArg checkWithBlock:^BOOL(
id value) {
signinCallback = value;
return true;
}]])
.andReturn(authentication_flow_mock_);
[mediator_ accountTappedWithGaiaID:kSecondaryIdentity.gaiaID
targetRect:target];
break;
}
VerifyMock();
OCMExpect([delegate_mock_

@ -246,6 +246,7 @@ void RecordIOSIdentityAvailableInProfile(
// `YES` if the profile switching is done.
BOOL _didSwitchProfile;
base::ScopedClosureRunner _accountSwitchInProgress;
}
@synthesize handlingError = _handlingError;
@ -626,14 +627,18 @@ void RecordIOSIdentityAvailableInProfile(
// Otherwise, this step does nothing and the flow continues to the next step.
- (void)signOutIfNeededStep {
ProfileIOS* profile = [self originalProfile];
AuthenticationService* authenticationService =
AuthenticationServiceFactory::GetForProfile(profile);
id<SystemIdentity> currentIdentity =
AuthenticationServiceFactory::GetForProfile(profile)->GetPrimaryIdentity(
signin::ConsentLevel::kSignin);
if (currentIdentity && ![currentIdentity isEqual:_identityToSignIn]) {
[_performer signOutProfile:profile];
authenticationService->GetPrimaryIdentity(signin::ConsentLevel::kSignin);
if (!currentIdentity || [currentIdentity isEqual:_identityToSignIn]) {
// No need to sign out.
[self continueFlow];
return;
}
[self continueFlow];
_accountSwitchInProgress =
authenticationService->DeclareAccountSwitchInProgress();
[_performer signOutProfile:profile];
}
// Sets the primary identity for the current profile.
@ -691,6 +696,7 @@ void RecordIOSIdentityAvailableInProfile(
- (void)completeWithSuccessStep {
DCHECK(_signInCompletion)
<< "`completeSignInWithResult` should not be called twice.";
_accountSwitchInProgress.RunAndReset();
signin_metrics::SigninAccountType accountType =
(_identityToSignInHostedDomain.length > 0)
? signin_metrics::SigninAccountType::kManaged
@ -713,6 +719,7 @@ void RecordIOSIdentityAvailableInProfile(
ProfileIOS* profile = [self originalProfile];
[_performer signOutImmediatelyFromProfile:profile];
}
_accountSwitchInProgress.RunAndReset();
SigninCoordinatorResult result;
switch (_cancelationReason) {
case CancelationReason::kFailed:

@ -3,6 +3,7 @@
// found in the LICENSE file.
#import "base/strings/sys_string_conversions.h"
#import "base/test/ios/wait_util.h"
#import "ios/chrome/browser/authentication/ui_bundled/account_menu/account_menu_constants.h"
#import "ios/chrome/browser/authentication/ui_bundled/signin/signin_constants.h"
#import "ios/chrome/browser/authentication/ui_bundled/signin_earl_grey.h"
@ -125,12 +126,27 @@ id<GREYMatcher> ContinueButtonWithIdentityMatcher(
[[EarlGrey selectElementWithMatcher:grey_accessibilityID(
kAccountMenuSecondaryAccountButtonId)]
performAction:grey_tap()];
// TODO(crbug.com/375604649): The enterprise onboarding screen should show up
// at this point.
// Wait for the enterprise onboarding screen.
ConditionBlock enterpriseOnboardingCondition = ^{
NSError* error;
[[EarlGrey selectElementWithMatcher:ManagedProfileCreationScreenMatcher()]
assertWithMatcher:grey_sufficientlyVisible()
error:&error];
// Wait for the new profile to finish loading.
// TODO(crbug.com/331783685): Find a better way to wait for this.
GREYWaitForAppToIdle(@"App failed to idle");
return error == nil;
};
GREYAssert(base::test::ios::WaitUntilConditionOrTimeout(
base::test::ios::kWaitForUIElementTimeout,
enterpriseOnboardingCondition),
@"Enterprise onboarding didn't appear.");
// Confirm the enterprise onboarding screen.
[[EarlGrey selectElementWithMatcher:
chrome_test_util::PromoStylePrimaryActionButtonMatcher()]
performAction:grey_tap()];
// Ensure the enterprise onboarding screen did disapepar.
[[EarlGrey selectElementWithMatcher:IdentityDiscMatcher()]
assertWithMatcher:grey_sufficientlyVisible()];
[SigninEarlGrey verifySignedInWithFakeIdentity:managedIdentity];
@ -158,6 +174,64 @@ id<GREYMatcher> ContinueButtonWithIdentityMatcher(
@"Profile should have been switched");
}
// Tests switching to a managed account and refuse the enterprise onboard
// screen.
- (void)testRefuseToSwitchToManageAccount {
// Separate profiles are only available in iOS 17+.
if (!@available(iOS 17, *)) {
return;
}
NSString* personalProfileName = [ChromeEarlGrey currentProfileName];
// Setup: There's 1 personal and 1 managed account. The personal account is
// signed in.
FakeSystemIdentity* const personalIdentity =
[FakeSystemIdentity fakeIdentity1];
[SigninEarlGrey addFakeIdentity:personalIdentity];
FakeSystemIdentity* const managedIdentity =
[FakeSystemIdentity fakeManagedIdentity];
[SigninEarlGrey addFakeIdentity:managedIdentity];
[SigninEarlGreyUI signinWithFakeIdentity:personalIdentity];
// Switch to the managed account, which triggers a switch to a new managed
// profile.
OpenAccountMenu();
[[EarlGrey selectElementWithMatcher:grey_accessibilityID(
kAccountMenuSecondaryAccountButtonId)]
performAction:grey_tap()];
// Wait for the enterprise onboarding screen.
ConditionBlock enterpriseOnboardingCondition = ^{
NSError* error;
[[EarlGrey selectElementWithMatcher:ManagedProfileCreationScreenMatcher()]
assertWithMatcher:grey_sufficientlyVisible()
error:&error];
return error == nil;
};
GREYAssert(base::test::ios::WaitUntilConditionOrTimeout(
base::test::ios::kWaitForUIElementTimeout,
enterpriseOnboardingCondition),
@"Enterprise onboarding didn't appear.");
// Refuse the enterprise onboarding screen.
[[EarlGrey selectElementWithMatcher:
chrome_test_util::PromoStyleSecondaryActionButtonMatcher()]
performAction:grey_tap()];
// Wait for the new profile to finish loading.
// TODO(crbug.com/331783685): Find a better way to wait for this.
GREYWaitForAppToIdle(@"App failed to idle");
[SigninEarlGrey verifySignedInWithFakeIdentity:personalIdentity];
// Verify that the profile was actually switched back.
GREYAssert(
[[ChromeEarlGrey currentProfileName] isEqualToString:personalProfileName],
@"Profile should have been switched");
}
@end
@interface SeparateProfilesFRETestCase : ChromeTestCase