Skip to content

GitLab

T
tangled
Commit 798a1075 authored by Colin Blundell's avatar Colin Blundell Committed by Commit Bot
Browse files
Options

Change undefined behavior of chrome.identity.onSigninChanged event 

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.

This CL changes this undocumented behavior, so that events for
secondary events will fire regardless of whether a primary account is
present. 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.

The CL implements this change by porting IdentityAPI away from using
gaia::AccountTracker to observing ProfileOAuth2TokenService and
AccountTrackerService directly. By doing so, IdentityAPI observes
(and fires events for) signin change events for secondary accounts
regardless of whether a primary account is present.

This change has a side benefit in removing usage of the AccountTracker
class, which is deprecated with an eye toward complete removal
(see details in crbug.com/729590). It will be followed up by a
conversion of this code to use the Identity Service client library.

To test, install a Chrome extension with the identity permissions in
its manifest. Go to chrome://extensions, enable developer mode, and
inspect the background page of the above app. At the JS console that
that brings up, execute:
chrome.identity.onSignInChanged.addListener((account, signed_in) => {console.log(account.id + " " + signed_in);} )
Sign out of the browser. Verify that you receive a callback at the console
with a value of false.
Sign back in. Verify that you receive another callback at the console for
the same account ID with a value of true.

Bug: 729589, 729542, 769700
Change-Id: I99dd48d34b380067ac34a2207effd9d3279191fd
Reviewed-on: https://chromium-review.googlesource.com/596368
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: default avatarMihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545081}
parent acd78b23
Show whitespace changes
Inline Side-by-side
Showing with 118 additions and 100 deletions
chrome/browser/extensions/api/identity/identity_api.cc
View file @ 798a1075
......@@ -29,13 +29,9 @@
#include "chrome/browser/signin/account_tracker_service_factory.h"
#include "chrome/browser/signin/chrome_signin_client_factory.h"
#include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
#include "chrome/browser/signin/signin_manager_factory.h"
#include "chrome/browser/ui/webui/signin/login_ui_service_factory.h"
#include "chrome/common/extensions/api/identity.h"
#include "chrome/common/url_constants.h"
#include "components/signin/core/browser/account_tracker_service.h"
#include "components/signin/core/browser/profile_oauth2_token_service.h"
#include "components/signin/core/browser/signin_manager.h"
#include "extensions/browser/extension_function_dispatcher.h"
#include "extensions/common/extension.h"
#include "extensions/common/extension_l10n_util.h"
......@@ -103,17 +99,9 @@ const base::Time& IdentityTokenCacheValue::expiration_time() const {
}
IdentityAPI::IdentityAPI(content::BrowserContext* context)
: browser_context_(context),
profile_identity_provider_(
SigninManagerFactory::GetForProfile(
Profile::FromBrowserContext(context)),
ProfileOAuth2TokenServiceFactory::GetForProfile(
Profile::FromBrowserContext(context)),
LoginUIServiceFactory::GetShowLoginPopupCallbackForProfile(
Profile::FromBrowserContext(context))),
account_tracker_(&profile_identity_provider_,
g_browser_process->system_request_context()) {
account_tracker_.AddObserver(this);
: profile_(Profile::FromBrowserContext(context)) {
AccountTrackerServiceFactory::GetForProfile(profile_)->AddObserver(this);
ProfileOAuth2TokenServiceFactory::GetForProfile(profile_)->AddObserver(this);
}
IdentityAPI::~IdentityAPI() {}
......@@ -155,8 +143,9 @@ const IdentityAPI::CachedTokens& IdentityAPI::GetAllCachedTokens() {
void IdentityAPI::Shutdown() {
on_shutdown_callback_list_.Notify();
account_tracker_.RemoveObserver(this);
account_tracker_.Shutdown();
AccountTrackerServiceFactory::GetForProfile(profile_)->RemoveObserver(this);
ProfileOAuth2TokenServiceFactory::GetForProfile(profile_)->RemoveObserver(
this);
}
static base::LazyInstance<BrowserContextKeyedAPIFactory<IdentityAPI>>::
......@@ -167,31 +156,41 @@ BrowserContextKeyedAPIFactory<IdentityAPI>* IdentityAPI::GetFactoryInstance() {
return g_identity_api_factory.Pointer();
}
void IdentityAPI::OnAccountSignInChanged(const gaia::AccountIds& ids,
void IdentityAPI::OnRefreshTokenAvailable(const std::string& account_id) {
const AccountInfo& account_info =
AccountTrackerServiceFactory::GetForProfile(profile_)->GetAccountInfo(
account_id);
// Refresh tokens are sometimes made available in contexts where
// AccountTrackerService is not tracking the account in question (one example
// is SupervisedUserService::InitSync()). Bail out in these cases.
if (account_info.gaia.empty())
return;
FireOnAccountSignInChanged(account_info.gaia, true);
}
void IdentityAPI::OnAccountRemoved(const AccountInfo& account_info) {
DCHECK(!account_info.gaia.empty());
FireOnAccountSignInChanged(account_info.gaia, false);
}
void IdentityAPI::FireOnAccountSignInChanged(const std::string& gaia_id,
bool is_signed_in) {
api::identity::AccountInfo account_info;
account_info.id = ids.gaia;
DCHECK(!gaia_id.empty());
api::identity::AccountInfo api_account_info;
api_account_info.id = gaia_id;
std::unique_ptr<base::ListValue> args =
api::identity::OnSignInChanged::Create(account_info, is_signed_in);
std::unique_ptr<Event> event(
new Event(events::IDENTITY_ON_SIGN_IN_CHANGED,
api::identity::OnSignInChanged::kEventName, std::move(args),
browser_context_));
api::identity::OnSignInChanged::Create(api_account_info, is_signed_in);
std::unique_ptr<Event> event(new Event(
events::IDENTITY_ON_SIGN_IN_CHANGED,
api::identity::OnSignInChanged::kEventName, std::move(args), profile_));
if (on_signin_changed_callback_for_testing_)
on_signin_changed_callback_for_testing_.Run(event.get());
EventRouter::Get(browser_context_)->BroadcastEvent(std::move(event));
}
void IdentityAPI::SetAccountStateForTesting(const std::string& account_id,
bool signed_in) {
gaia::AccountIds ids;
ids.account_key = account_id;
ids.email = account_id;
ids.gaia = account_id;
account_tracker_.SetAccountStateForTest(ids, signed_in);
EventRouter::Get(profile_)->BroadcastEvent(std::move(event));
}
template <>
......@@ -199,9 +198,7 @@ void BrowserContextKeyedAPIFactory<IdentityAPI>::DeclareFactoryDependencies() {
DependsOn(ExtensionsBrowserClient::Get()->GetExtensionSystemFactory());
DependsOn(ChromeSigninClientFactory::GetInstance());
DependsOn(LoginUIServiceFactory::GetInstance());
DependsOn(ProfileOAuth2TokenServiceFactory::GetInstance());
DependsOn(SigninManagerFactory::GetInstance());
}
} // namespace extensions
chrome/browser/extensions/api/identity/identity_api.h
View file @ 798a1075
......@@ -27,10 +27,9 @@
#include "chrome/browser/extensions/api/identity/identity_remove_cached_auth_token_function.h"
#include "chrome/browser/extensions/api/identity/web_auth_flow.h"
#include "chrome/browser/extensions/chrome_extension_function.h"
#include "components/signin/core/browser/profile_identity_provider.h"
#include "components/signin/core/browser/account_tracker_service.h"
#include "extensions/browser/browser_context_keyed_api_factory.h"
#include "extensions/browser/event_router.h"
#include "google_apis/gaia/account_tracker.h"
#include "google_apis/gaia/oauth2_mint_token_flow.h"
#include "google_apis/gaia/oauth2_token_service.h"
......@@ -38,6 +37,8 @@ namespace content {
class BrowserContext;
}
class Profile;
namespace extensions {
class IdentityTokenCacheValue {
......@@ -72,7 +73,8 @@ class IdentityTokenCacheValue {
};
class IdentityAPI : public BrowserContextKeyedAPI,
public gaia::AccountTracker::Observer {
public AccountTrackerService::Observer,
public OAuth2TokenService::Observer {
public:
typedef std::map<ExtensionTokenKey, IdentityTokenCacheValue> CachedTokens;
......@@ -92,24 +94,15 @@ class IdentityAPI : public BrowserContextKeyedAPI,
const CachedTokens& GetAllCachedTokens();
// BrowserContextKeyedAPI implementation.
// BrowserContextKeyedAPI:
void Shutdown() override;
static BrowserContextKeyedAPIFactory<IdentityAPI>* GetFactoryInstance();
// gaia::AccountTracker::Observer implementation:
void OnAccountSignInChanged(const gaia::AccountIds& ids,
bool is_signed_in) override;
std::unique_ptr<base::CallbackList<void()>::Subscription>
RegisterOnShutdownCallback(const base::Closure& cb) {
return on_shutdown_callback_list_.Add(cb);
}
// TODO(blundell): Eliminate this method once this class is no longer using
// AccountTracker.
// Makes |account_tracker_| aware of this account.
void SetAccountStateForTesting(const std::string& account_id, bool signed_in);
// Callback that is used in testing contexts to test the implementation of
// the chrome.identity.onSignInChanged event. Note that the passed-in Event is
// valid only for the duration of the callback.
......@@ -122,15 +115,31 @@ class IdentityAPI : public BrowserContextKeyedAPI,
private:
friend class BrowserContextKeyedAPIFactory<IdentityAPI>;
// BrowserContextKeyedAPI implementation.
// BrowserContextKeyedAPI:
static const char* service_name() { return "IdentityAPI"; }
static const bool kServiceIsNULLWhileTesting = true;
content::BrowserContext* browser_context_;
// OAuth2TokenService::Observer:
void OnRefreshTokenAvailable(const std::string& account_id) override;
// AccountTrackerService::Observer:
// NOTE: This class listens for signout events via this callback (which itself
// is triggered by O2TS::OnRefreshTokenRevoked()) rather than directly via
// OnRefreshTokenRevoked() in order to obtain the Gaia ID of the signed-out
// account, which is needed to send as input to the
// chrome.identity.onSigninChanged event. That Gaia ID is not guaranteed to be
// available from O2TS::OnRefreshTokenRevoked().
// TODO(blundell): Eliminate this kludge by porting this class to interact
// with IdentityManager.
void OnAccountRemoved(const AccountInfo& info) override;
// Fires the chrome.identity.onSignInChanged event.
void FireOnAccountSignInChanged(const std::string& gaia_id,
bool is_signed_in);
Profile* profile_;
IdentityMintRequestQueue mint_queue_;
CachedTokens token_cache_;
ProfileIdentityProvider profile_identity_provider_;
gaia::AccountTracker account_tracker_;
OnSignInChangedCallback on_signin_changed_callback_for_testing_;
......
chrome/browser/extensions/api/identity/identity_apitest.cc
View file @ 798a1075
......@@ -37,6 +37,7 @@
#include "chrome/browser/extensions/extension_function_test_utils.h"
#include "chrome/browser/extensions/extension_service.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/signin/account_fetcher_service_factory.h"
#include "chrome/browser/signin/account_tracker_service_factory.h"
#include "chrome/browser/signin/fake_gaia_cookie_manager_service_builder.h"
#include "chrome/browser/signin/fake_profile_oauth2_token_service_builder.h"
......@@ -51,6 +52,7 @@
#include "chrome/test/base/in_process_browser_test.h"
#include "components/crx_file/id_util.h"
#include "components/guest_view/browser/guest_view_base.h"
#include "components/signin/core/browser/account_fetcher_service.h"
#include "components/signin/core/browser/account_tracker_service.h"
#include "components/signin/core/browser/fake_gaia_cookie_manager_service.h"
#include "components/signin/core/browser/fake_profile_oauth2_token_service.h"
......@@ -414,14 +416,6 @@ class MockQueuedMintRequest : public IdentityMintRequestQueue::Request {
MOCK_METHOD1(StartMintToken, void(IdentityMintRequestQueue::MintType));
};
gaia::AccountIds CreateIds(const std::string& email, const std::string& obfid) {
gaia::AccountIds ids;
ids.account_key = email;
ids.email = email;
ids.gaia = obfid;
return ids;
}
class IdentityTestWithSignin : public AsyncExtensionBrowserTest {
public:
void SetUpInProcessBrowserTestFixture() override {
......@@ -445,6 +439,18 @@ class IdentityTestWithSignin : public AsyncExtensionBrowserTest {
context, &BuildFakeProfileOAuth2TokenService);
GaiaCookieManagerServiceFactory::GetInstance()->SetTestingFactory(
context, &BuildFakeGaiaCookieManagerService);
// Ensure that AccountFetcherService is (1) created at all and (2) created
// early enough for it to observe the Profile initialization process and
// loading of tokens by PO2TS. Explicitly forcing this setup (which happens
// naturally in production) is necessary for the flow of
// AccountTrackerService having accounts removed when tokens are revoked
// with PO2TS to work as expected in this testing context.
// TODO(blundell): Change these tests to interact with
// IdentityTestEnvironment once the production code is changed to interact
// with IdentityManager.
AccountFetcherServiceFactory::GetInstance()->GetForProfile(
Profile::FromBrowserContext(context));
}
void SetUpOnMainThread() override {
......@@ -461,6 +467,19 @@ class IdentityTestWithSignin : public AsyncExtensionBrowserTest {
GaiaCookieManagerServiceFactory::GetInstance()
->GetForProfile(profile())
->Init();
#if defined(OS_CHROMEOS)
// On ChromeOS, ProfileOAuth2TokenService does not fire
// OnRefreshTokensLoaded() in text contexts. However, AccountFetcherService
// must receive this call in order to forward later
// OnRefreshToken{Available, Revoked} callbacks on to AccountTrackerService
// as expected. Hence, we make that call explicitly here.
// TODO(blundell): Hide this detail when converting this code to interact
// with IdentityTestEnvironment.
AccountFetcherServiceFactory::GetInstance()
->GetForProfile(profile())
->OnRefreshTokensLoaded();
#endif
}
protected:
......@@ -717,10 +736,6 @@ class GetAuthTokenFunctionTest
command_line->AppendSwitch(switches::kExtensionsMultiAccount);
}
void IssueLoginRefreshTokenForAccount(const std::string& account_key) {
token_service_->UpdateCredentials(account_key, "refresh_token");
}
void IssueLoginAccessTokenForAccount(const std::string& account_key) {
token_service_->IssueAllTokensForAccount(
account_key,
......@@ -1743,8 +1758,7 @@ IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest,
IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest,
MultiPrimaryUserManuallyIssueToken) {
SignIn("primary@example.com");
IssueLoginRefreshTokenForAccount("secondary@example.com");
SeedAccountInfo("secondary@example.com");
AddAccount("secondary@example.com", "secondary@example.com");
scoped_refptr<FakeGetAuthTokenFunction> func(new FakeGetAuthTokenFunction());
scoped_refptr<const Extension> extension(CreateExtension(CLIENT_ID | SCOPES));
......@@ -1772,8 +1786,7 @@ IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest,
IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest,
MultiSecondaryUserManuallyIssueToken) {
SignIn("primary@example.com");
IssueLoginRefreshTokenForAccount("secondary@example.com");
SeedAccountInfo("secondary@example.com");
AddAccount("secondary@example.com", "secondary@example.com");
scoped_refptr<FakeGetAuthTokenFunction> func(new FakeGetAuthTokenFunction());
scoped_refptr<const Extension> extension(CreateExtension(CLIENT_ID | SCOPES));
......@@ -1801,8 +1814,7 @@ IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest,
IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest,
MultiUnknownUserGetTokenFromTokenServiceFailure) {
SignIn("primary@example.com");
IssueLoginRefreshTokenForAccount("secondary@example.com");
SeedAccountInfo("secondary@example.com");
AddAccount("secondary@example.com", "secondary@example.com");
scoped_refptr<FakeGetAuthTokenFunction> func(new FakeGetAuthTokenFunction());
scoped_refptr<const Extension> extension(CreateExtension(CLIENT_ID | SCOPES));
......@@ -1818,8 +1830,7 @@ IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest,
IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest,
MultiSecondaryNonInteractiveMintFailure) {
SignIn("primary@example.com");
IssueLoginRefreshTokenForAccount("secondary@example.com");
SeedAccountInfo("secondary@example.com");
AddAccount("secondary@example.com", "secondary@example.com");
scoped_refptr<FakeGetAuthTokenFunction> func(new FakeGetAuthTokenFunction());
func->set_extension(CreateExtension(CLIENT_ID | SCOPES));
......@@ -1836,8 +1847,7 @@ IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest,
IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest,
MultiSecondaryNonInteractiveLoginAccessTokenFailure) {
SignIn("primary@example.com");
IssueLoginRefreshTokenForAccount("secondary@example.com");
SeedAccountInfo("secondary@example.com");
AddAccount("secondary@example.com", "secondary@example.com");
scoped_refptr<FakeGetAuthTokenFunction> func(new FakeGetAuthTokenFunction());
func->set_extension(CreateExtension(CLIENT_ID | SCOPES));
......@@ -1852,8 +1862,7 @@ IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest,
IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest,
MultiSecondaryInteractiveApprovalAborted) {
SignIn("primary@example.com");
IssueLoginRefreshTokenForAccount("secondary@example.com");
SeedAccountInfo("secondary@example.com");
AddAccount("secondary@example.com", "secondary@example.com");
scoped_refptr<FakeGetAuthTokenFunction> func(new FakeGetAuthTokenFunction());
func->set_extension(CreateExtension(CLIENT_ID | SCOPES));
......@@ -2222,6 +2231,7 @@ class OnSignInChangedEventTest : public IdentityTestWithSignin {
id_api()->set_on_signin_changed_callback_for_testing(
base::Bind(&OnSignInChangedEventTest::OnSignInEventChanged,
base::Unretained(this)));
IdentityTestWithSignin::SetUpOnMainThread();
}
......@@ -2245,8 +2255,7 @@ class OnSignInChangedEventTest : public IdentityTestWithSignin {
private:
void OnSignInEventChanged(Event* event) {
if (!HasExpectedEvent())
return;
ASSERT_TRUE(HasExpectedEvent());
// Search for |event| in the set of expected events.
bool found_event = false;
......@@ -2283,8 +2292,6 @@ class OnSignInChangedEventTest : public IdentityTestWithSignin {
// Test that an event is fired when the primary account signs in.
IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest, FireOnPrimaryAccountSignIn) {
id_api()->SetAccountStateForTesting("primary", false);
api::identity::AccountInfo account_info;
account_info.id = "primary";
AddExpectedEvent(api::identity::OnSignInChanged::Create(account_info, true));
......@@ -2298,10 +2305,12 @@ IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest, FireOnPrimaryAccountSignIn) {
#if !defined(OS_CHROMEOS)
// Test that an event is fired when the primary account signs out.
IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest, FireOnPrimaryAccountSignOut) {
id_api()->SetAccountStateForTesting("primary", true);
api::identity::AccountInfo account_info;
account_info.id = "primary";
AddExpectedEvent(api::identity::OnSignInChanged::Create(account_info, true));
SignIn("primary", "primary");
AddExpectedEvent(api::identity::OnSignInChanged::Create(account_info, false));
// Sign out and verify that the callback fires.
......@@ -2315,10 +2324,12 @@ IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest, FireOnPrimaryAccountSignOut) {
// revoked.
IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest,
FireOnPrimaryAccountRefreshTokenRevoked) {
id_api()->SetAccountStateForTesting("primary", true);
api::identity::AccountInfo account_info;
account_info.id = "primary";
AddExpectedEvent(api::identity::OnSignInChanged::Create(account_info, true));
SignIn("primary", "primary");
AddExpectedEvent(api::identity::OnSignInChanged::Create(account_info, false));
// Revoke the refresh token and verify that the callback fires.
......@@ -2331,38 +2342,39 @@ IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest,
// newly available.
IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest,
FireOnPrimaryAccountRefreshTokenAvailable) {
id_api()->SetAccountStateForTesting("primary", false);
api::identity::AccountInfo account_info;
account_info.id = "primary";
AddExpectedEvent(api::identity::OnSignInChanged::Create(account_info, true));
SignIn("primary", "primary");
AddExpectedEvent(api::identity::OnSignInChanged::Create(account_info, false));
token_service_->RevokeCredentials("primary");
api::identity::AccountInfo account_info;
account_info.id = "primary";
AddExpectedEvent(api::identity::OnSignInChanged::Create(account_info, true));
// Make the primary account's refresh token available and check that the
// callback fires.
token_service_->UpdateCredentials("primary", "refresh_token");
// callback fires. Note that we must call AddAccount() here as the account's
// information must be present in the AccountTrackerService as well.
AddAccount("primary", "primary");
EXPECT_FALSE(HasExpectedEvent());
}
// 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.
api::identity::AccountInfo account_info;
account_info.id = "primary";
AddExpectedEvent(api::identity::OnSignInChanged::Create(account_info, true));
SignIn("primary", "primary");
api::identity::AccountInfo account_info;
account_info.id = "secondary";
AddExpectedEvent(api::identity::OnSignInChanged::Create(account_info, true));
// Make a secondary account's refresh token available and check that the
// callback fires.
token_service_->UpdateCredentials("secondary", "refresh_token");
// callback fires. Note that we must call AddAccount() here as the account's
// information must be present in the AccountTrackerService as well.
AddAccount("secondary", "secondary");
EXPECT_FALSE(HasExpectedEvent());
// Revoke the secondary account's refresh token and check that the callback
......
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