[Signin] Pass externalCcResult to multilogin requests
Pass externalCcResult in the multilogin params to make sure it correctly
sets cookies for secondary domains.
This had conflicts with https://crrev.com/c/1624189 that were resolved
manually.
TBR=msarda@chromium.org
(cherry picked from commit 0aec7c82277205ddd4bf8e8a3a6604fe186d5019)
Bug: 971393, b/133516269
Change-Id: Iab7d6779e8137fd677e19929113464aa3acd9be6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1658572
Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#669320}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1662416
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Cr-Commit-Position: refs/branch-heads/3809@{#378}
Cr-Branched-From: d82dec1a818f378c464ba307ddd9c92133eac355-refs/heads/master@{#665002}
diff --git a/components/signin/core/browser/gaia_cookie_manager_service.cc b/components/signin/core/browser/gaia_cookie_manager_service.cc
index 50df29d..e28e2b5 100644
--- a/components/signin/core/browser/gaia_cookie_manager_service.cc
+++ b/components/signin/core/browser/gaia_cookie_manager_service.cc
@@ -252,9 +252,11 @@
return base::JoinString(results, ",");
}
-void GaiaCookieManagerService::ExternalCcResultFetcher::Start() {
+void GaiaCookieManagerService::ExternalCcResultFetcher::Start(
+ base::OnceClosure callback) {
DCHECK(!helper_->external_cc_result_fetched_);
m_external_cc_result_start_time_ = base::Time::Now();
+ callback_ = std::move(callback);
CleanupTransientState();
results_.clear();
@@ -441,11 +443,7 @@
time_to_check_connections);
helper_->external_cc_result_fetched_ = true;
- // Since the ExternalCCResultFetcher is only Started in place of calling
- // StartFetchingMergeSession, we can assume we need to call
- // StartFetchingMergeSession. If this assumption becomes invalid, a Callback
- // will need to be passed to Start() and Run() here.
- helper_->StartFetchingMergeSession();
+ std::move(callback_).Run();
}
GaiaCookieManagerService::GaiaCookieManagerService(
@@ -506,11 +504,7 @@
}
if (requests_.size() == 1) {
fetcher_retries_ = 0;
- // Unretained is safe because GaiaCookieManagerService owns the helper.
- oauth_multilogin_helper_ = std::make_unique<signin::OAuthMultiloginHelper>(
- signin_client_, token_service_, account_ids,
- base::BindOnce(&GaiaCookieManagerService::OnSetAccountsFinished,
- base::Unretained(this)));
+ StartSetAccounts();
}
}
@@ -765,7 +759,9 @@
if (!external_cc_result_fetched_ &&
!external_cc_result_fetcher_.IsRunning()) {
- external_cc_result_fetcher_.Start();
+ external_cc_result_fetcher_.Start(
+ base::BindOnce(&GaiaCookieManagerService::StartFetchingMergeSession,
+ weak_ptr_factory_.GetWeakPtr()));
return;
}
@@ -966,6 +962,27 @@
gaia_auth_fetcher_->StartListAccounts();
}
+void GaiaCookieManagerService::StartSetAccounts() {
+ DCHECK(!requests_.empty());
+ DCHECK_EQ(GaiaCookieRequestType::SET_ACCOUNTS,
+ requests_.front().request_type());
+ DCHECK(!requests_.front().account_ids().empty());
+
+ if (!external_cc_result_fetched_ &&
+ !external_cc_result_fetcher_.IsRunning()) {
+ external_cc_result_fetcher_.Start(
+ base::BindOnce(&GaiaCookieManagerService::StartSetAccounts,
+ weak_ptr_factory_.GetWeakPtr()));
+ return;
+ }
+
+ oauth_multilogin_helper_ = std::make_unique<signin::OAuthMultiloginHelper>(
+ signin_client_, token_service_, requests_.front().account_ids(),
+ external_cc_result_fetcher_.GetExternalCcResult(),
+ base::BindOnce(&GaiaCookieManagerService::OnSetAccountsFinished,
+ weak_ptr_factory_.GetWeakPtr()));
+}
+
void GaiaCookieManagerService::OnSetAccountsFinished(
signin::SetAccountsInCookieResult result) {
MarkListAccountsStale();
@@ -1002,12 +1019,7 @@
weak_ptr_factory_.GetWeakPtr()));
break;
case GaiaCookieRequestType::SET_ACCOUNTS:
- DCHECK(!requests_.front().account_ids().empty());
- oauth_multilogin_helper_ =
- std::make_unique<signin::OAuthMultiloginHelper>(
- signin_client_, token_service_, requests_.front().account_ids(),
- base::BindOnce(&GaiaCookieManagerService::OnSetAccountsFinished,
- weak_ptr_factory_.GetWeakPtr()));
+ StartSetAccounts();
break;
case GaiaCookieRequestType::LOG_OUT:
DCHECK(requests_.front().account_ids().empty());
diff --git a/components/signin/core/browser/gaia_cookie_manager_service.h b/components/signin/core/browser/gaia_cookie_manager_service.h
index 13aeee1..94901f4e 100644
--- a/components/signin/core/browser/gaia_cookie_manager_service.h
+++ b/components/signin/core/browser/gaia_cookie_manager_service.h
@@ -184,7 +184,7 @@
// Start fetching the external CC result. If a fetch is already in progress
// it is canceled.
- void Start();
+ void Start(base::OnceClosure callback);
// Are external URLs still being checked?
bool IsRunning();
@@ -221,6 +221,7 @@
LoaderToToken loaders_;
ResultMap results_;
base::Time m_external_cc_result_start_time_;
+ base::OnceClosure callback_;
DISALLOW_COPY_AND_ASSIGN(ExternalCcResultFetcher);
};
@@ -354,6 +355,9 @@
// Virtual for testing purpose.
virtual void StartFetchingLogOut();
+ // Starts setting account using multilogin endpoint.
+ void StartSetAccounts();
+
// Start the next request, if needed.
void HandleNextRequest();
diff --git a/components/signin/core/browser/gaia_cookie_manager_service_unittest.cc b/components/signin/core/browser/gaia_cookie_manager_service_unittest.cc
index 80c2108c..85a5c442 100644
--- a/components/signin/core/browser/gaia_cookie_manager_service_unittest.cc
+++ b/components/signin/core/browser/gaia_cookie_manager_service_unittest.cc
@@ -809,7 +809,9 @@
InstrumentedGaiaCookieManagerService helper(token_service(), signin_client());
GaiaCookieManagerService::ExternalCcResultFetcher result_fetcher(&helper);
EXPECT_CALL(helper, StartFetchingMergeSession());
- result_fetcher.Start();
+ result_fetcher.Start(base::BindOnce(
+ &InstrumentedGaiaCookieManagerService::StartFetchingMergeSession,
+ base::Unretained(&helper)));
// Simulate a successful completion of GetCheckConnctionInfo.
SimulateGetCheckConnectionInfoSuccess(
@@ -834,7 +836,9 @@
InstrumentedGaiaCookieManagerService helper(token_service(), signin_client());
GaiaCookieManagerService::ExternalCcResultFetcher result_fetcher(&helper);
EXPECT_CALL(helper, StartFetchingMergeSession());
- result_fetcher.Start();
+ result_fetcher.Start(base::BindOnce(
+ &InstrumentedGaiaCookieManagerService::StartFetchingMergeSession,
+ base::Unretained(&helper)));
// Simulate a successful completion of GetCheckConnctionInfo.
SimulateGetCheckConnectionInfoSuccess(
@@ -863,7 +867,9 @@
InstrumentedGaiaCookieManagerService helper(token_service(), signin_client());
GaiaCookieManagerService::ExternalCcResultFetcher result_fetcher(&helper);
EXPECT_CALL(helper, StartFetchingMergeSession());
- result_fetcher.Start();
+ result_fetcher.Start(base::BindOnce(
+ &InstrumentedGaiaCookieManagerService::StartFetchingMergeSession,
+ base::Unretained(&helper)));
// Simulate a successful completion of GetCheckConnctionInfo.
SimulateGetCheckConnectionInfoSuccess(
@@ -905,7 +911,9 @@
TEST_F(GaiaCookieManagerServiceTest, UbertokenSuccessFetchesExternalCCOnce) {
InstrumentedGaiaCookieManagerService helper(token_service(), signin_client());
- helper.external_cc_result_fetcher_for_testing()->Start();
+ helper.external_cc_result_fetcher_for_testing()->Start(base::BindOnce(
+ &InstrumentedGaiaCookieManagerService::StartFetchingMergeSession,
+ base::Unretained(&helper)));
EXPECT_CALL(helper, StartFetchingUbertoken());
helper.AddAccountToCookie(
diff --git a/components/signin/core/browser/oauth_multilogin_helper.cc b/components/signin/core/browser/oauth_multilogin_helper.cc
index 230678c..ab38dc09 100644
--- a/components/signin/core/browser/oauth_multilogin_helper.cc
+++ b/components/signin/core/browser/oauth_multilogin_helper.cc
@@ -42,10 +42,12 @@
SigninClient* signin_client,
OAuth2TokenService* token_service,
const std::vector<std::string>& account_ids,
+ const std::string& external_cc_result,
base::OnceCallback<void(signin::SetAccountsInCookieResult)> callback)
: signin_client_(signin_client),
token_service_(token_service),
account_ids_(account_ids),
+ external_cc_result_(external_cc_result),
callback_(std::move(callback)),
weak_ptr_factory_(this) {
DCHECK(signin_client_);
@@ -102,7 +104,8 @@
DCHECK_EQ(token_id_pairs_.size(), account_ids_.size());
gaia_auth_fetcher_ =
signin_client_->CreateGaiaAuthFetcher(this, gaia::GaiaSource::kChrome);
- gaia_auth_fetcher_->StartOAuthMultilogin(token_id_pairs_);
+ gaia_auth_fetcher_->StartOAuthMultilogin(token_id_pairs_,
+ external_cc_result_);
}
void OAuthMultiloginHelper::OnOAuthMultiloginFinished(
diff --git a/components/signin/core/browser/oauth_multilogin_helper.h b/components/signin/core/browser/oauth_multilogin_helper.h
index 5fcda19..69064a9 100644
--- a/components/signin/core/browser/oauth_multilogin_helper.h
+++ b/components/signin/core/browser/oauth_multilogin_helper.h
@@ -39,6 +39,7 @@
SigninClient* signin_client,
OAuth2TokenService* token_service,
const std::vector<std::string>& account_ids,
+ const std::string& external_cc_result,
base::OnceCallback<void(signin::SetAccountsInCookieResult)> callback);
~OAuthMultiloginHelper() override;
@@ -74,6 +75,8 @@
// Account ids to set in the cookie.
const std::vector<std::string> account_ids_;
+ // See GaiaCookieManagerService::ExternalCcResultFetcher for details.
+ const std::string external_cc_result_;
// Access tokens, in the same order as the account ids.
std::vector<GaiaAuthFetcher::MultiloginTokenIDPair> token_id_pairs_;
diff --git a/components/signin/core/browser/oauth_multilogin_helper_unittest.cc b/components/signin/core/browser/oauth_multilogin_helper_unittest.cc
index 0de493c6b..8c19a4b 100644
--- a/components/signin/core/browser/oauth_multilogin_helper_unittest.cc
+++ b/components/signin/core/browser/oauth_multilogin_helper_unittest.cc
@@ -26,6 +26,8 @@
const char kAccessToken[] = "access_token_1";
const char kAccessToken2[] = "access_token_2";
+const char kExternalCcResult[] = "youtube:OK";
+
constexpr int kMaxFetcherRetries = 3;
const char kMultiloginSuccessResponse[] =
@@ -76,6 +78,35 @@
}
)";
+const char kMultiloginSuccessResponseWithSecondaryDomain[] =
+ R"()]}'
+ {
+ "status": "OK",
+ "cookies":[
+ {
+ "name":"SID",
+ "value":"SID_value",
+ "domain":".youtube.com",
+ "path":"/",
+ "isSecure":true,
+ "isHttpOnly":false,
+ "priority":"HIGH",
+ "maxAge":63070000
+ },
+ {
+ "name":"FOO",
+ "value":"FOO_value",
+ "domain":".google.com",
+ "path":"/",
+ "isSecure":true,
+ "isHttpOnly":false,
+ "priority":"HIGH",
+ "maxAge":63070000
+ }
+ ]
+ }
+ )";
+
const char kMultiloginRetryResponse[] =
R"()]}'
{
@@ -140,7 +171,15 @@
std::unique_ptr<OAuthMultiloginHelper> CreateHelper(
const std::vector<std::string> account_ids) {
return std::make_unique<OAuthMultiloginHelper>(
- &test_signin_client_, token_service(), account_ids,
+ &test_signin_client_, token_service(), account_ids, std::string(),
+ base::BindOnce(&OAuthMultiloginHelperTest::OnOAuthMultiloginFinished,
+ base::Unretained(this)));
+ }
+
+ std::unique_ptr<OAuthMultiloginHelper> CreateHelperWithExternalCcResult(
+ const std::vector<std::string> account_ids) {
+ return std::make_unique<OAuthMultiloginHelper>(
+ &test_signin_client_, token_service(), account_ids, kExternalCcResult,
base::BindOnce(&OAuthMultiloginHelperTest::OnOAuthMultiloginFinished,
base::Unretained(this)));
}
@@ -154,6 +193,11 @@
"?source=ChromiumBrowser";
}
+ std::string multilogin_url_with_external_cc_result() const {
+ return GaiaUrls::GetInstance()->oauth_multilogin_url().spec() +
+ "?source=ChromiumBrowser&externalCcResult=" + kExternalCcResult;
+ }
+
MockCookieManager* cookie_manager() { return mock_cookie_manager_; }
MockTokenService* token_service() { return &mock_token_service_; }
@@ -236,6 +280,43 @@
EXPECT_EQ(signin::SetAccountsInCookieResult::kSuccess, result_);
}
+// Multiple cookies in the multilogin response.
+TEST_F(OAuthMultiloginHelperTest, SuccessWithExternalCcResult) {
+ token_service()->AddAccount(kAccountId);
+ std::unique_ptr<OAuthMultiloginHelper> helper =
+ CreateHelperWithExternalCcResult({kAccountId});
+
+ // Configure mock cookie manager:
+ // - check that the cookie is the expected one
+ // - immediately invoke the callback
+ EXPECT_CALL(
+ *cookie_manager(),
+ SetCanonicalCookie(CookieMatcher("SID", "SID_value", ".youtube.com"),
+ "https", testing::_, testing::_))
+ .WillOnce(::testing::Invoke(RunSetCookieCallbackWithSuccess));
+ EXPECT_CALL(
+ *cookie_manager(),
+ SetCanonicalCookie(CookieMatcher("FOO", "FOO_value", ".google.com"),
+ "https", testing::_, testing::_))
+ .WillOnce(::testing::Invoke(RunSetCookieCallbackWithSuccess));
+
+ // Issue access token.
+ OAuth2AccessTokenConsumer::TokenResponse success_response;
+ success_response.access_token = kAccessToken;
+ token_service()->IssueAllTokensForAccount(kAccountId, success_response);
+
+ // Multilogin call.
+ EXPECT_FALSE(callback_called_);
+ EXPECT_TRUE(
+ url_loader()->IsPending(multilogin_url_with_external_cc_result()));
+ url_loader()->AddResponse(multilogin_url_with_external_cc_result(),
+ kMultiloginSuccessResponseWithSecondaryDomain);
+ EXPECT_FALSE(
+ url_loader()->IsPending(multilogin_url_with_external_cc_result()));
+ EXPECT_TRUE(callback_called_);
+ EXPECT_EQ(signin::SetAccountsInCookieResult::kSuccess, result_);
+}
+
// Failure to get the access token.
TEST_F(OAuthMultiloginHelperTest, OneAccountAccessTokenFailure) {
token_service()->AddAccount(kAccountId);
diff --git a/google_apis/gaia/gaia_auth_fetcher.cc b/google_apis/gaia/gaia_auth_fetcher.cc
index 19fea63..e39e285 100644
--- a/google_apis/gaia/gaia_auth_fetcher.cc
+++ b/google_apis/gaia/gaia_auth_fetcher.cc
@@ -747,7 +747,8 @@
}
void GaiaAuthFetcher::StartOAuthMultilogin(
- const std::vector<MultiloginTokenIDPair>& accounts) {
+ const std::vector<MultiloginTokenIDPair>& accounts,
+ const std::string& external_cc_result) {
DCHECK(!fetch_pending_) << "Tried to fetch two things at once!";
UMA_HISTOGRAM_COUNTS_100("Signin.Multilogin.NumberOfAccounts",
@@ -763,8 +764,13 @@
kOAuthMultiBearerHeaderFormat,
base::JoinString(authorization_header_parts, ",").c_str());
- std::string parameters = base::StringPrintf(
- "?source=%s", net::EscapeUrlEncodedData(source_, true).c_str());
+ std::string source_string = net::EscapeUrlEncodedData(source_, true);
+ std::string parameters =
+ external_cc_result.empty()
+ ? base::StringPrintf("?source=%s", source_string.c_str())
+ : base::StringPrintf(
+ "?source=%s&externalCcResult=%s", source_string.c_str(),
+ net::EscapeUrlEncodedData(external_cc_result, true).c_str());
net::NetworkTrafficAnnotationTag traffic_annotation =
net::DefineNetworkTrafficAnnotation("gaia_auth_multilogin", R"(
diff --git a/google_apis/gaia/gaia_auth_fetcher.h b/google_apis/gaia/gaia_auth_fetcher.h
index a0c555f..289c9f2 100644
--- a/google_apis/gaia/gaia_auth_fetcher.h
+++ b/google_apis/gaia/gaia_auth_fetcher.h
@@ -159,7 +159,8 @@
const std::string& service);
// Starts a request to get the cookie for list of accounts.
- void StartOAuthMultilogin(const std::vector<MultiloginTokenIDPair>& accounts);
+ void StartOAuthMultilogin(const std::vector<MultiloginTokenIDPair>& accounts,
+ const std::string& external_cc_result);
// Starts a request to list the accounts in the GAIA cookie.
void StartListAccounts();
diff --git a/google_apis/gaia/gaia_auth_fetcher_unittest.cc b/google_apis/gaia/gaia_auth_fetcher_unittest.cc
index eee429c7..248392f 100644
--- a/google_apis/gaia/gaia_auth_fetcher_unittest.cc
+++ b/google_apis/gaia/gaia_auth_fetcher_unittest.cc
@@ -386,7 +386,7 @@
TestGaiaAuthFetcher auth(&consumer, GetURLLoaderFactory());
auth.StartOAuthMultilogin(
- std::vector<GaiaAuthFetcher::MultiloginTokenIDPair>());
+ std::vector<GaiaAuthFetcher::MultiloginTokenIDPair>(), std::string());
EXPECT_TRUE(auth.HasPendingFetch());
auth.TestOnURLLoadCompleteInternal(net::OK, net::HTTP_OK,
@@ -421,7 +421,7 @@
TestGaiaAuthFetcher auth(&consumer, GetURLLoaderFactory());
auth.StartOAuthMultilogin(
- std::vector<GaiaAuthFetcher::MultiloginTokenIDPair>());
+ std::vector<GaiaAuthFetcher::MultiloginTokenIDPair>(), std::string());
EXPECT_TRUE(auth.HasPendingFetch());
auth.TestOnURLLoadCompleteInternal(net::ERR_ABORTED, net::HTTP_OK,
@@ -456,7 +456,7 @@
TestGaiaAuthFetcher auth(&consumer, GetURLLoaderFactory());
auth.StartOAuthMultilogin(
- std::vector<GaiaAuthFetcher::MultiloginTokenIDPair>());
+ std::vector<GaiaAuthFetcher::MultiloginTokenIDPair>(), std::string());
EXPECT_TRUE(auth.HasPendingFetch());
auth.TestOnURLLoadCompleteInternal(net::OK, net::HTTP_OK,