Skip to content

Commit

Permalink
Add URL parameter so that /ListAccounts returns valid json.
Browse files Browse the repository at this point in the history 
Check the valid_session bit to determine if the account in the cookie is valid.

BUG=305249

Review URL: https://codereview.chromium.org/166433005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@252240 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
rogerta@chromium.org committed Feb 20, 2014
1 parent ee7338f commit 71b6aaa
Show file tree
Hide file tree
Showing 9 changed files with 170 additions and 64 deletions.
8 changes: 5 additions & 3 deletions chrome/browser/chromeos/login/oauth2_login_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "chrome/browser/chromeos/login/oauth2_login_manager.h"

#include <utility>
#include <vector>

#include "base/command_line.h"
Expand Down Expand Up @@ -292,15 +293,16 @@ void OAuth2LoginManager::OnSessionMergeFailure(bool connection_error) {
void OAuth2LoginManager::OnListAccountsSuccess(const std::string& data) {
MergeVerificationOutcome outcome = POST_MERGE_SUCCESS;
// Let's analyze which accounts we see logged in here:
std::vector<std::string> accounts;
std::vector<std::pair<std::string, bool> > accounts;
gaia::ParseListAccountsData(data, &accounts);
std::string user_email = gaia::CanonicalizeEmail(GetPrimaryAccountId());
if (!accounts.empty()) {
bool found = false;
bool first = true;
for (std::vector<std::string>::const_iterator iter = accounts.begin();
for (std::vector<std::pair<std::string, bool> >::const_iterator iter =
accounts.begin();
iter != accounts.end(); ++iter) {
if (gaia::CanonicalizeEmail(*iter) == user_email) {
if (gaia::CanonicalizeEmail(iter->first) == user_email) {
found = true;
break;
}
Expand Down
35 changes: 23 additions & 12 deletions chrome/browser/signin/account_reconcilor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -362,13 +362,18 @@ void AccountReconcilor::StartRemoveAction(const std::string& account_id) {
void AccountReconcilor::FinishRemoveAction(
const std::string& account_id,
const GoogleServiceAuthError& error,
const std::vector<std::string>& accounts) {
const std::vector<std::pair<std::string, bool> >& accounts) {
VLOG(1) << "AccountReconcilor::FinishRemoveAction:"
<< " account=" << account_id
<< " error=" << error.ToString();
if (error.state() == GoogleServiceAuthError::NONE) {
AbortReconcile();
merge_session_helper_.LogOut(account_id, accounts);
std::vector<std::string> accounts_only;
for (std::vector<std::pair<std::string, bool> >::const_iterator i =
accounts.begin(); i != accounts.end(); ++i) {
accounts_only.push_back(i->first);
}
merge_session_helper_.LogOut(account_id, accounts_only);
}
// Wait for the next ReconcileAction if there is an error.
}
Expand Down Expand Up @@ -430,14 +435,14 @@ void AccountReconcilor::OnListAccountsSuccess(const std::string& data) {
gaia_fetcher_.reset();

// Get account information from response data.
std::vector<std::string> gaia_accounts;
std::vector<std::pair<std::string, bool> > gaia_accounts;
bool valid_json = gaia::ParseListAccountsData(data, &gaia_accounts);
if (!valid_json) {
VLOG(1) << "AccountReconcilor::OnListAccountsSuccess: parsing error";
} else if (gaia_accounts.size() > 0) {
VLOG(1) << "AccountReconcilor::OnListAccountsSuccess: "
<< "Gaia " << gaia_accounts.size() << " accounts, "
<< "Primary is '" << gaia_accounts[0] << "'";
<< "Primary is '" << gaia_accounts[0].first << "'";
} else {
VLOG(1) << "AccountReconcilor::OnListAccountsSuccess: No accounts";
}
Expand All @@ -459,7 +464,7 @@ void AccountReconcilor::OnListAccountsFailure(
const GoogleServiceAuthError& error) {
gaia_fetcher_.reset();
VLOG(1) << "AccountReconcilor::OnListAccountsFailure: " << error.ToString();
std::vector<std::string> empty_accounts;
std::vector<std::pair<std::string, bool> > empty_accounts;

// There must be at least one callback waiting for result.
DCHECK(!get_gaia_accounts_callbacks_.empty());
Expand All @@ -480,7 +485,7 @@ void AccountReconcilor::MayBeDoNextListAccounts() {

void AccountReconcilor::ContinueReconcileActionAfterGetGaiaAccounts(
const GoogleServiceAuthError& error,
const std::vector<std::string>& accounts) {
const std::vector<std::pair<std::string, bool> >& accounts) {
if (error.state() == GoogleServiceAuthError::NONE) {
gaia_accounts_ = accounts;
are_gaia_accounts_set_ = true;
Expand Down Expand Up @@ -569,13 +574,14 @@ void AccountReconcilor::FinishReconcile() {
DCHECK(add_to_cookie_.empty());
DCHECK(add_to_chrome_.empty());
bool are_primaries_equal =
gaia_accounts_.size() > 0 && primary_account_ == gaia_accounts_[0];
gaia_accounts_.size() > 0 && primary_account_ == gaia_accounts_[0].first;

if (are_primaries_equal) {
// Determine if we need to merge accounts from gaia cookie to chrome.
for (size_t i = 0; i < gaia_accounts_.size(); ++i) {
const std::string& gaia_account = gaia_accounts_[i];
if (valid_chrome_accounts_.find(gaia_account) ==
const std::string& gaia_account = gaia_accounts_[i].first;
if (gaia_accounts_[i].second &&
valid_chrome_accounts_.find(gaia_account) ==
valid_chrome_accounts_.end()) {
add_to_chrome_.push_back(std::make_pair(gaia_account, i));
}
Expand All @@ -585,10 +591,15 @@ void AccountReconcilor::FinishReconcile() {
for (std::set<std::string>::const_iterator i =
valid_chrome_accounts_.begin();
i != valid_chrome_accounts_.end(); ++i) {
if (std::find(gaia_accounts_.begin(), gaia_accounts_.end(), *i) ==
gaia_accounts_.end()) {
add_to_cookie_.push_back(*i);
bool add_to_cookie = true;
for (size_t j = 0; j < gaia_accounts_.size(); ++j) {
if (gaia_accounts_[j].first == *i) {
add_to_cookie = !gaia_accounts_[j].second;
break;
}
}
if (add_to_cookie)
add_to_cookie_.push_back(*i);
}
} else {
VLOG(1) << "AccountReconcilor::FinishReconcile: rebuild cookie";
Expand Down
25 changes: 18 additions & 7 deletions chrome/browser/signin/account_reconcilor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
#define CHROME_BROWSER_SIGNIN_ACCOUNT_RECONCILOR_H_

#include <deque>
#include <string>
#include <utility>
#include <vector>

#include "base/basictypes.h"
#include "base/callback_forward.h"
Expand Down Expand Up @@ -56,10 +59,12 @@ class AccountReconcilor : public BrowserContextKeyedService,

bool AreAllRefreshTokensChecked() const;

const std::vector<std::string>& GetGaiaAccountsForTesting() const {
const std::vector<std::pair<std::string, bool> >&
GetGaiaAccountsForTesting() const {
return gaia_accounts_;
}

private:
const std::set<std::string>& GetValidChromeAccountsForTesting() const {
return valid_chrome_accounts_;
}
Expand All @@ -68,12 +73,12 @@ class AccountReconcilor : public BrowserContextKeyedService,
return invalid_chrome_accounts_;
}

private:
// Used during GetAccountsFromCookie.
// Stores a callback for the next action to perform.
typedef base::Callback<void(
const GoogleServiceAuthError& error,
const std::vector<std::string>&)> GetAccountsFromCookieCallback;
const std::vector<std::pair<std::string, bool> >&)>
GetAccountsFromCookieCallback;

friend class AccountReconcilorTest;
FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, GetAccountsFromCookieSuccess);
Expand All @@ -89,6 +94,8 @@ class AccountReconcilor : public BrowserContextKeyedService,
FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, StartReconcileAddToChrome);
FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, StartReconcileBadPrimary);
FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, StartReconcileOnlyOnce);
FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest,
StartReconcileWithSessionInfoExpiredDefault);

class RefreshTokenFetcher;
class UserIdFetcher;
Expand Down Expand Up @@ -122,7 +129,7 @@ class AccountReconcilor : public BrowserContextKeyedService,
virtual void FinishRemoveAction(
const std::string& account_id,
const GoogleServiceAuthError& error,
const std::vector<std::string>& accounts);
const std::vector<std::pair<std::string, bool> >& accounts);

// Used during periodic reconciliation.
void StartReconcile();
Expand All @@ -137,7 +144,7 @@ class AccountReconcilor : public BrowserContextKeyedService,
void GetAccountsFromCookie(GetAccountsFromCookieCallback callback);
void ContinueReconcileActionAfterGetGaiaAccounts(
const GoogleServiceAuthError& error,
const std::vector<std::string>& accounts);
const std::vector<std::pair<std::string, bool> >& accounts);
void ValidateAccountsFromTokenService();

void OnCookieChanged(ChromeCookieDetails* details);
Expand Down Expand Up @@ -189,9 +196,13 @@ class AccountReconcilor : public BrowserContextKeyedService,
bool is_reconcile_started_;

// Used during reconcile action.
// These members are used used to validate the gaia cookie.
// These members are used used to validate the gaia cookie. |gaia_accounts_|
// holds the state of google accounts in the gaia cookie. Each element is
// a pair that holds the email address of the account and a boolean that
// indicates whether the account is valid or not. The accounts in the vector
// are ordered the in same way as the gaia cookie.
bool are_gaia_accounts_set_;
std::vector<std::string> gaia_accounts_;
std::vector<std::pair<std::string, bool> > gaia_accounts_;

// Used during reconcile action.
// These members are used to validate the tokens in OAuth2TokenService.
Expand Down
83 changes: 59 additions & 24 deletions chrome/browser/signin/account_reconcilor_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "chrome/browser/signin/signin_manager_factory.h"
#include "chrome/test/base/testing_profile.h"
#include "content/public/test/test_browser_thread_bundle.h"
#include "google_apis/gaia/gaia_urls.h"
#include "net/url_request/test_url_fetcher_factory.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
Expand All @@ -33,10 +34,11 @@ class MockAccountReconcilor : public testing::StrictMock<AccountReconcilor> {

MOCK_METHOD1(PerformMergeAction, void(const std::string& account_id));
MOCK_METHOD1(StartRemoveAction, void(const std::string& account_id));
MOCK_METHOD3(FinishRemoveAction,
void(const std::string& account_id,
const GoogleServiceAuthError& error,
const std::vector<std::string>& accounts));
MOCK_METHOD3(
FinishRemoveAction,
void(const std::string& account_id,
const GoogleServiceAuthError& error,
const std::vector<std::pair<std::string, bool> >& accounts));
MOCK_METHOD2(PerformAddToChromeAction, void(const std::string& account_id,
int session_index));
MOCK_METHOD0(PerformLogoutAllAccountsAction, void());
Expand Down Expand Up @@ -205,19 +207,19 @@ TEST_F(AccountReconcilorTest, GetAccountsFromCookieSuccess) {
AccountReconcilorFactory::GetForProfile(profile());
ASSERT_TRUE(reconcilor);

SetFakeResponse("https://accounts.google.com/ListAccounts",
"[\"foo\", [[\"b\", 0, \"n\", \"user@gmail.com\", \"p\", 0, 0, 0]]]",
SetFakeResponse(GaiaUrls::GetInstance()->list_accounts_url().spec(),
"[\"f\", [[\"b\", 0, \"n\", \"user@gmail.com\", \"p\", 0, 0, 0, 0, 0]]]",
net::HTTP_OK, net::URLRequestStatus::SUCCESS);

reconcilor->StartReconcile();
ASSERT_FALSE(reconcilor->AreGaiaAccountsSet());

base::RunLoop().RunUntilIdle();
ASSERT_TRUE(reconcilor->AreGaiaAccountsSet());
const std::vector<std::string>& accounts =
const std::vector<std::pair<std::string, bool> >& accounts =
reconcilor->GetGaiaAccountsForTesting();
ASSERT_EQ(1u, accounts.size());
ASSERT_EQ("user@gmail.com", accounts[0]);
ASSERT_EQ("user@gmail.com", accounts[0].first);
}

TEST_F(AccountReconcilorTest, GetAccountsFromCookieFailure) {
Expand All @@ -227,7 +229,7 @@ TEST_F(AccountReconcilorTest, GetAccountsFromCookieFailure) {
AccountReconcilorFactory::GetForProfile(profile());
ASSERT_TRUE(reconcilor);

SetFakeResponse("https://accounts.google.com/ListAccounts", "",
SetFakeResponse(GaiaUrls::GetInstance()->list_accounts_url().spec(), "",
net::HTTP_NOT_FOUND, net::URLRequestStatus::SUCCESS);

reconcilor->StartReconcile();
Expand Down Expand Up @@ -309,8 +311,8 @@ TEST_F(AccountReconcilorTest, StartReconcileNoop) {
AccountReconcilorFactory::GetForProfile(profile());
ASSERT_TRUE(reconcilor);

SetFakeResponse("https://accounts.google.com/ListAccounts",
"[\"foo\", [[\"b\", 0, \"n\", \"user@gmail.com\", \"p\", 0, 0, 0]]]",
SetFakeResponse(GaiaUrls::GetInstance()->list_accounts_url().spec(),
"[\"f\", [[\"b\", 0, \"n\", \"user@gmail.com\", \"p\", 0, 0, 0, 0, 1]]]",
net::HTTP_OK, net::URLRequestStatus::SUCCESS);
SetFakeResponse("https://www.googleapis.com/oauth2/v1/userinfo",
"{\"id\":\"foo\"}", net::HTTP_OK, net::URLRequestStatus::SUCCESS);
Expand Down Expand Up @@ -341,9 +343,9 @@ TEST_F(AccountReconcilorTest, StartReconcileNoopMultiple) {
AccountReconcilorFactory::GetForProfile(profile());
ASSERT_TRUE(reconcilor);

SetFakeResponse("https://accounts.google.com/ListAccounts",
"[\"foo\", [[\"b\", 0, \"n\", \"user@gmail.com\", \"p\", 0, 0, 0], "
"[\"b\", 0, \"n\", \"other@gmail.com\", \"p\", 0, 0, 0]]]",
SetFakeResponse(GaiaUrls::GetInstance()->list_accounts_url().spec(),
"[\"f\", [[\"b\", 0, \"n\", \"user@gmail.com\", \"p\", 0, 0, 0, 0, 1], "
"[\"b\", 0, \"n\", \"other@gmail.com\", \"p\", 0, 0, 0, 0, 1]]]",
net::HTTP_OK, net::URLRequestStatus::SUCCESS);
SetFakeResponse("https://www.googleapis.com/oauth2/v1/userinfo",
"{\"id\":\"foo\"}", net::HTTP_OK, net::URLRequestStatus::SUCCESS);
Expand Down Expand Up @@ -378,8 +380,8 @@ TEST_F(AccountReconcilorTest, StartReconcileAddToCookie) {

EXPECT_CALL(*GetMockReconcilor(), PerformMergeAction("other@gmail.com"));

SetFakeResponse("https://accounts.google.com/ListAccounts",
"[\"foo\", [[\"b\", 0, \"n\", \"user@gmail.com\", \"p\", 0, 0, 0]]]",
SetFakeResponse(GaiaUrls::GetInstance()->list_accounts_url().spec(),
"[\"f\", [[\"b\", 0, \"n\", \"user@gmail.com\", \"p\", 0, 0, 0, 0, 1]]]",
net::HTTP_OK, net::URLRequestStatus::SUCCESS);
SetFakeResponse("https://www.googleapis.com/oauth2/v1/userinfo",
"{\"id\":\"foo\"}", net::HTTP_OK, net::URLRequestStatus::SUCCESS);
Expand All @@ -405,9 +407,9 @@ TEST_F(AccountReconcilorTest, StartReconcileAddToChrome) {
EXPECT_CALL(*GetMockReconcilor(),
PerformAddToChromeAction("other@gmail.com", 1));

SetFakeResponse("https://accounts.google.com/ListAccounts",
"[\"foo\", [[\"b\", 0, \"n\", \"user@gmail.com\", \"p\", 0, 0, 0], "
"[\"b\", 0, \"n\", \"other@gmail.com\", \"p\", 0, 0, 0]]]",
SetFakeResponse(GaiaUrls::GetInstance()->list_accounts_url().spec(),
"[\"f\", [[\"b\", 0, \"n\", \"user@gmail.com\", \"p\", 0, 0, 0, 0, 1], "
"[\"b\", 0, \"n\", \"other@gmail.com\", \"p\", 0, 0, 0, 0, 1]]]",
net::HTTP_OK, net::URLRequestStatus::SUCCESS);
SetFakeResponse("https://www.googleapis.com/oauth2/v1/userinfo",
"{\"id\":\"foo\"}", net::HTTP_OK, net::URLRequestStatus::SUCCESS);
Expand All @@ -432,9 +434,9 @@ TEST_F(AccountReconcilorTest, StartReconcileBadPrimary) {
EXPECT_CALL(*GetMockReconcilor(), PerformMergeAction("user@gmail.com"));
EXPECT_CALL(*GetMockReconcilor(), PerformMergeAction("other@gmail.com"));

SetFakeResponse("https://accounts.google.com/ListAccounts",
"[\"foo\", [[\"b\", 0, \"n\", \"other@gmail.com\", \"p\", 0, 0, 0], "
"[\"b\", 0, \"n\", \"user@gmail.com\", \"p\", 0, 0, 0]]]",
SetFakeResponse(GaiaUrls::GetInstance()->list_accounts_url().spec(),
"[\"f\", [[\"b\", 0, \"n\", \"other@gmail.com\", \"p\", 0, 0, 0, 0, 1], "
"[\"b\", 0, \"n\", \"user@gmail.com\", \"p\", 0, 0, 0, 0, 1]]]",
net::HTTP_OK, net::URLRequestStatus::SUCCESS);
SetFakeResponse("https://www.googleapis.com/oauth2/v1/userinfo",
"{\"id\":\"foo\"}", net::HTTP_OK, net::URLRequestStatus::SUCCESS);
Expand Down Expand Up @@ -464,8 +466,8 @@ TEST_F(AccountReconcilorTest, StartReconcileOnlyOnce) {
AccountReconcilorFactory::GetForProfile(profile());
ASSERT_TRUE(reconcilor);

SetFakeResponse("https://accounts.google.com/ListAccounts",
"[\"foo\", [[\"b\", 0, \"n\", \"user@gmail.com\", \"p\", 0, 0, 0]]]",
SetFakeResponse(GaiaUrls::GetInstance()->list_accounts_url().spec(),
"[\"f\", [[\"b\", 0, \"n\", \"user@gmail.com\", \"p\", 0, 0, 0, 0, 1]]]",
net::HTTP_OK, net::URLRequestStatus::SUCCESS);
SetFakeResponse("https://www.googleapis.com/oauth2/v1/userinfo",
"{\"id\":\"foo\"}", net::HTTP_OK, net::URLRequestStatus::SUCCESS);
Expand All @@ -480,3 +482,36 @@ TEST_F(AccountReconcilorTest, StartReconcileOnlyOnce) {
base::RunLoop().RunUntilIdle();
ASSERT_FALSE(reconcilor->is_reconcile_started_);
}

TEST_F(AccountReconcilorTest, StartReconcileWithSessionInfoExpiredDefault) {
signin_manager()->SetAuthenticatedUsername("user@gmail.com");
token_service()->UpdateCredentials("user@gmail.com", "refresh_token");
token_service()->UpdateCredentials("other@gmail.com", "refresh_token");

EXPECT_CALL(*GetMockReconcilor(), PerformMergeAction("user@gmail.com"));

SetFakeResponse(GaiaUrls::GetInstance()->list_accounts_url().spec(),
"[\"f\", [[\"b\", 0, \"n\", \"user@gmail.com\", \"p\", 0, 0, 0, 0, 0],"
"[\"b\", 0, \"n\", \"other@gmail.com\", \"p\", 0, 0, 0, 0, 1]]]",
net::HTTP_OK, net::URLRequestStatus::SUCCESS);
SetFakeResponse("https://www.googleapis.com/oauth2/v1/userinfo",
"{\"id\":\"foo\"}", net::HTTP_OK, net::URLRequestStatus::SUCCESS);

AccountReconcilor* reconcilor =
AccountReconcilorFactory::GetForProfile(profile());
ASSERT_TRUE(reconcilor);

ASSERT_FALSE(reconcilor->is_reconcile_started_);
reconcilor->StartReconcile();
ASSERT_TRUE(reconcilor->is_reconcile_started_);

token_service()->IssueAllTokensForAccount("user@gmail.com", "access_token",
base::Time::Now() + base::TimeDelta::FromHours(1));
token_service()->IssueAllTokensForAccount("other@gmail.com", "access_token",
base::Time::Now() + base::TimeDelta::FromHours(1));

base::RunLoop().RunUntilIdle();
SimulateMergeSessionCompleted(reconcilor, "user@gmail.com",
GoogleServiceAuthError::AuthErrorNone());
ASSERT_FALSE(reconcilor->is_reconcile_started_);
}
5 changes: 3 additions & 2 deletions chrome/browser/ui/webui/chromeos/login/authenticated_user_email_retriever.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "chrome/browser/ui/webui/chromeos/login/authenticated_user_email_retriever.h"

#include <utility>
#include <vector>

#include "google_apis/gaia/gaia_auth_util.h"
Expand All @@ -28,12 +29,12 @@ AuthenticatedUserEmailRetriever::~AuthenticatedUserEmailRetriever() {

void AuthenticatedUserEmailRetriever::OnListAccountsSuccess(
const std::string& data) {
std::vector<std::string> accounts;
std::vector<std::pair<std::string, bool> > accounts;
gaia::ParseListAccountsData(data, &accounts);
if (accounts.size() != 1)
callback_.Run(std::string());
else
callback_.Run(accounts.front());
callback_.Run(accounts.front().first);
}

void AuthenticatedUserEmailRetriever::OnListAccountsFailure(
Expand Down
Loading

0 comments on commit 71b6aaa

Please sign in to comment.