Commit 1f7826ee authored by Colin Blundell's avatar Colin Blundell Committed by Commit Bot

Remove tests for undefined chrome.identity.onSigninChanged behavior

The chrome.identity.onSigninChanged extension API event is defined
as follows:

"Fired when signin state changes for an account on the user's profile."
(https://developer.chrome.com/apps/identity#event-onSignInChanged)

The current browser-side implementation uses gaia::AccountTracker,
which ignores events for secondary accounts if there is no primary
account (i.e., syncing account) present. Thus, this browser-side
implementation also fires events for secondary accounts only if there is
a primary account present.

However:

(1) This behavior is not defined in the documented semantics above
(2) In practice, this case has historically never been encountered by
end users, as it has been impossible in desktop Chrome to have a
secondary account without a primary account present.

In this CL, we are removing browsertests that enforce this undocumented
behavior (note that we ourselves added these browsertests merely to
document the existing implementation's behavior before refactoring it).
By removing the requirement that the implementation behave in a specific
way in this case, we will ease the upcoming refactoring significantly.

Note that post-project DICE, it will be possible for this case to be
encountered by end users. In that world, it will actually be more
sensible for events to fire for secondary accounts regardless of
whether or not the user has designated a syncing account. This behavior
is what will occur post the refactoring that will follow this CL.

Bug: 729542
Change-Id: I02c1ffbcbfd732ba17056e939eb6f86ab3b26576
Reviewed-on: https://chromium-review.googlesource.com/966035Reviewed-by: default avatarMihai Sardarescu <msarda@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544665}
parent a73a7ccf
......@@ -2346,13 +2346,14 @@ IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest,
EXPECT_FALSE(HasExpectedEvent());
}
// Test that an event is fired for changes to a secondary account when there is
// a primary account available.
IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest,
FireForSecondaryAccountWhenPrimaryAccountExists) {
// Test that an event is fired for changes to a secondary account.
IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest, FireForSecondaryAccount) {
id_api()->SetAccountStateForTesting("primary", false);
id_api()->SetAccountStateForTesting("secondary", false);
// NOTE: The current implementation requires that the primary account be
// present for an event to fire for secondary accounts. This is not actually
// part of the semantics of chrome.identity.OnSignInEventChanged, however.
SignIn("primary", "primary");
api::identity::AccountInfo account_info;
......@@ -2372,75 +2373,6 @@ IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest,
EXPECT_FALSE(HasExpectedEvent());
}
// Test that an event is not fired for changes to a secondary account when
// there is no primary account available.
IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest,
DontFireForSecondaryAccountWhenNoPrimaryAccountExists) {
// Add an expected event to be able to verify that no event is fired.
api::identity::AccountInfo account_info;
account_info.id = "secondary";
AddExpectedEvent(api::identity::OnSignInChanged::Create(account_info, true));
// Check not firing on addition of secondary account.
AddAccount("secondary", "secondary");
EXPECT_TRUE(HasExpectedEvent());
// Check not firing on token revocation of secondary account.
token_service_->RevokeCredentials("primary");
EXPECT_TRUE(HasExpectedEvent());
}
// The below tests involve elements that aren't relevant on ChromeOS:
// - Signin of secondary accounts before the primary account signs in. On
// ChromeOS, this isn't possible.
// - Signout of the primary account. On ChromeOS, this isn't possible.
#if !defined(OS_CHROMEOS)
// Test that signin events are fired for all known accounts when the primary
// account signs in and there is a secondary account already present. Note that
// the order in which the events fire is undefined.
IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest,
FireForAllAccountsOnPrimaryAccountSignIn) {
id_api()->SetAccountStateForTesting("primary", false);
id_api()->SetAccountStateForTesting("secondary", false);
api::identity::AccountInfo account_info;
account_info.id = "secondary";
AddExpectedEvent(api::identity::OnSignInChanged::Create(account_info, true));
// Add the secondary account, and verify that no event is yet fired for it.
AddAccount("secondary", "secondary");
EXPECT_TRUE(HasExpectedEvent());
// Add an event for the primary account in preparation for signing in.
account_info.id = "primary";
AddExpectedEvent(api::identity::OnSignInChanged::Create(account_info, true));
// Sign in with the primary account, and verify that both events are fired.
SignIn("primary", "primary");
EXPECT_FALSE(HasExpectedEvent());
}
// Test that signout events are fired for all known accounts when the primary
// account signs out. Note that the order in which the events fire is undefined.
IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest,
FireForAllAccountsOnPrimaryAccountSignOut) {
id_api()->SetAccountStateForTesting("primary", true);
id_api()->SetAccountStateForTesting("secondary", true);
api::identity::AccountInfo account_info;
account_info.id = "primary";
AddExpectedEvent(api::identity::OnSignInChanged::Create(account_info, false));
account_info.id = "secondary";
AddExpectedEvent(api::identity::OnSignInChanged::Create(account_info, false));
// Sign out and verify that both events fire.
signin_manager_->ForceSignOut();
EXPECT_FALSE(HasExpectedEvent());
}
#endif // !defined(OS_CHROMEOS)
} // namespace extensions
// Tests the chrome.identity API implemented by custom JS bindings .
......
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