Skip to content

GitLab

T
tangled
Commit 6128c200 authored by rogerta's avatar rogerta Committed by Commit bot
Browse files
Options

Handle account removal correctly on all platforms. 

Don't validate refresh token before usage to reduce network traffic.

BUG=359700,416612

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

Cr-Commit-Position: refs/heads/master@{#296715}
parent 04cc6d8b
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 118 additions and 675 deletions
components/signin/core/browser/account_reconcilor.h
View file @ 6128c200
......@@ -27,7 +27,6 @@
class GaiaAuthFetcher;
class ProfileOAuth2TokenService;
class SigninClient;
class SigninOAuthHelper;
namespace net {
class CanonicalCookie;
......@@ -36,7 +35,6 @@ class CanonicalCookie;
class AccountReconcilor : public KeyedService,
public GaiaAuthConsumer,
public MergeSessionHelper::Observer,
public OAuth2TokenService::Consumer,
public OAuth2TokenService::Observer,
public SigninManagerBase::Observer {
public:
......@@ -73,43 +71,17 @@ class AccountReconcilor : public KeyedService,
virtual void GetAccountsFromCookie(GetAccountsFromCookieCallback callback);
private:
// An std::set<> for use with email addresses that uses
// gaia::CanonicalizeEmail() during comparisons.
// TODO(rogerta): this is a workaround for the fact that SigninManager and
// SigninOAuthHelper use the gaia "email" property when adding accounts to
// the token service, whereas gaia::ParseListAccountsData() returns email
// addresses that have been passed through gaia::CanonicalizeEmail(). These
// two types of email addresses are not directly comparable.
class EmailLessFunc : public std::less<std::string> {
public:
bool operator()(const std::string& s1, const std::string& s2) const;
};
typedef std::set<std::string, EmailLessFunc> EmailSet;
class RefreshTokenFetcher;
class UserIdFetcher;
bool IsRegisteredWithTokenService() const {
return registered_with_token_service_;
}
bool AreGaiaAccountsSet() const { return are_gaia_accounts_set_; }
bool AreAllRefreshTokensChecked() const;
const std::vector<std::pair<std::string, bool> >& GetGaiaAccountsForTesting()
const {
return gaia_accounts_;
}
const EmailSet& GetValidChromeAccountsForTesting() const {
return valid_chrome_accounts_;
}
const EmailSet& GetInvalidChromeAccountsForTesting() const {
return invalid_chrome_accounts_;
}
// Virtual so that it can be overridden in tests.
virtual void StartFetchingExternalCcResult();
......@@ -119,18 +91,14 @@ class AccountReconcilor : public KeyedService,
FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, ProfileAlreadyConnected);
FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, GetAccountsFromCookieSuccess);
FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, GetAccountsFromCookieFailure);
FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, ValidateAccountsFromTokens);
FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest,
ValidateAccountsFromTokensFailedUserInfo);
FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest,
ValidateAccountsFromTokensFailedTokenRequest);
FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, StartReconcileNoop);
FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, StartReconcileNoopWithDots);
FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, StartReconcileNoopMultiple);
FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, StartReconcileAddToCookie);
FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest,
StartReconcileRemoveFromCookie);
FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest,
StartReconcileAddToCookieTwice);
FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, StartReconcileAddToChrome);
FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, StartReconcileBadPrimary);
FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, StartReconcileOnlyOnce);
FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest,
......@@ -148,26 +116,10 @@ class AccountReconcilor : public KeyedService,
bool IsProfileConnected();
void DeleteFetchers();
// All actions with side effects. Virtual so that they can be overridden
// in tests.
virtual void PerformMergeAction(const std::string& account_id);
virtual void PerformAddToChromeAction(
const std::string& account_id,
int session_index,
const std::string& signin_scoped_device_id);
virtual void PerformLogoutAllAccountsAction();
virtual void PerformAddAccountToTokenService(
const std::string& account_id,
const std::string& refresh_token);
// Used to remove an account from chrome and the cookie jar.
virtual void PerformStartRemoveAction(const std::string& account_id);
virtual void PerformFinishRemoveAction(
const std::string& account_id,
const GoogleServiceAuthError& error,
const std::vector<std::pair<std::string, bool> >& accounts);
// Used during periodic reconciliation.
void StartReconcile();
......@@ -175,10 +127,6 @@ class AccountReconcilor : public KeyedService,
void AbortReconcile();
void CalculateIfReconcileIsDone();
void ScheduleStartReconcileIfChromeAccountsChanged();
void HandleSuccessfulAccountIdCheck(const std::string& account_id);
void HandleFailedAccountIdCheck(const std::string& account_id);
void HandleRefreshTokenFetched(const std::string& account_id,
const std::string& refresh_token);
void ContinueReconcileActionAfterGetGaiaAccounts(
const GoogleServiceAuthError& error,
......@@ -186,8 +134,6 @@ class AccountReconcilor : public KeyedService,
void ValidateAccountsFromTokenService();
// Note internally that this |account_id| is added to the cookie jar.
bool MarkAccountAsAddedToCookie(const std::string& account_id);
// Note internally that this |account_id| is added to the token service.
void MarkAccountAsAddedToChrome(const std::string& account_id);
void OnCookieChanged(const net::CanonicalCookie* cookie);
......@@ -201,15 +147,7 @@ class AccountReconcilor : public KeyedService,
const GoogleServiceAuthError& error)
OVERRIDE;
// Overriden from OAuth2TokenService::Consumer.
virtual void OnGetTokenSuccess(const OAuth2TokenService::Request* request,
const std::string& access_token,
const base::Time& expiration_time) OVERRIDE;
virtual void OnGetTokenFailure(const OAuth2TokenService::Request* request,
const GoogleServiceAuthError& error) OVERRIDE;
// Overriden from OAuth2TokenService::Observer.
virtual void OnRefreshTokenRevoked(const std::string& account_id) OVERRIDE;
virtual void OnEndBatchChanges() OVERRIDE;
// Overriden from SigninManagerBase::Observer.
......@@ -242,7 +180,7 @@ class AccountReconcilor : public KeyedService,
bool first_execution_;
// Used during reconcile action.
// These members are used used to validate the gaia cookie. |gaia_accounts_|
// These members are 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
......@@ -254,13 +192,7 @@ class AccountReconcilor : public KeyedService,
// These members are used to validate the tokens in OAuth2TokenService.
std::string primary_account_;
std::vector<std::string> chrome_accounts_;
scoped_ptr<OAuth2TokenService::Request>* requests_;
ScopedVector<UserIdFetcher> user_id_fetchers_;
ScopedVector<SigninOAuthHelper> refresh_token_fetchers_;
EmailSet valid_chrome_accounts_;
EmailSet invalid_chrome_accounts_;
std::vector<std::string> add_to_cookie_;
std::vector<std::pair<std::string, int> > add_to_chrome_;
std::deque<GetAccountsFromCookieCallback> get_gaia_accounts_callbacks_;
......
components/signin/core/browser/signin_metrics.cc
View file @ 6128c200
......@@ -22,19 +22,19 @@ DifferentPrimaryAccounts ComparePrimaryAccounts(bool primary_accounts_same,
void LogSigninAccountReconciliation(int total_number_accounts,
int count_added_to_cookie_jar,
int count_added_to_token,
int count_removed_from_cookie_jar,
bool primary_accounts_same,
bool is_first_reconcile,
int pre_count_gaia_cookies) {
UMA_HISTOGRAM_COUNTS_100("Profile.NumberOfAccountsPerProfile",
total_number_accounts);
// We want to include zeroes in the counts of added accounts to easily
// capture _relatively_ how often we merge accounts.
// We want to include zeroes in the counts of added or removed accounts to
// easily capture _relatively_ how often we merge accounts.
if (is_first_reconcile) {
UMA_HISTOGRAM_COUNTS_100("Signin.Reconciler.AddedToCookieJar.FirstRun",
count_added_to_cookie_jar);
UMA_HISTOGRAM_COUNTS_100("Signin.Reconciler.AddedToChrome.FirstRun",
count_added_to_token);
UMA_HISTOGRAM_COUNTS_100("Signin.Reconciler.RemovedFromCookieJar.FirstRun",
count_removed_from_cookie_jar);
UMA_HISTOGRAM_ENUMERATION(
"Signin.Reconciler.DifferentPrimaryAccounts.FirstRun",
ComparePrimaryAccounts(primary_accounts_same, pre_count_gaia_cookies),
......@@ -42,8 +42,9 @@ void LogSigninAccountReconciliation(int total_number_accounts,
} else {
UMA_HISTOGRAM_COUNTS_100("Signin.Reconciler.AddedToCookieJar.SubsequentRun",
count_added_to_cookie_jar);
UMA_HISTOGRAM_COUNTS_100("Signin.Reconciler.AddedToChrome.SubsequentRun",
count_added_to_token);
UMA_HISTOGRAM_COUNTS_100(
"Signin.Reconciler.RemovedFromCookieJar.SubsequentRun",
count_removed_from_cookie_jar);
UMA_HISTOGRAM_ENUMERATION(
"Signin.Reconciler.DifferentPrimaryAccounts.SubsequentRun",
ComparePrimaryAccounts(primary_accounts_same, pre_count_gaia_cookies),
......
components/signin/core/browser/signin_metrics.h
View file @ 6128c200
......@@ -53,9 +53,9 @@ enum ProfileSignout {
// |total_number_accounts| - How many accounts are in the browser for this
// profile.
// |count_added_to_cookie_jar| - How many accounts were in the browser but not
// the cookie jar.
// |count_added_to_token| - How may accounts were in the cookie jar but not in
// the browser.
// in the cookie jar.
// |count_removed_from_cookie_jar| - How many accounts were in the cookie jar
// but not in the browser.
// |primary_accounts_same| - False if the primary account for the cookie jar
// and the token service were different; else true.
// |is_first_reconcile| - True if these stats are from the first execution of
......@@ -64,7 +64,7 @@ enum ProfileSignout {
// the AccountReconcilor began modifying the state.
void LogSigninAccountReconciliation(int total_number_accounts,
int count_added_to_cookie_jar,
int count_added_to_token,
int count_removed_from_cookie_jar,
bool primary_accounts_same,
bool is_first_reconcile,
int pre_count_gaia_cookies);
......
tools/metrics/histograms/histograms.xml
View file @ 6128c200
......@@ -29834,18 +29834,22 @@ Therefore, the affected-histogram name has to have at least one dot in it.
</histogram>
<histogram name="Signin.Reconciler.AddedToChrome">
<obsolete>
Deprecated 2014-09 because chrome no longer tries to reconcile from the
cookie jar to the browser.
</obsolete>
<owner>mlerman@chromium.org</owner>
<summary>
After the first execution of the account reconciler, how many accounts were
added to the browser's token service because they were in the cookie jar.
How many accounts were added to the browser's token service because they
were in the cookie jar.
</summary>
</histogram>
<histogram name="Signin.Reconciler.AddedToCookieJar">
<owner>mlerman@chromium.org</owner>
<summary>
After the first execution of the account reconciler, how many accounts were
added to the cookie jar because they were in the browser's token service.
How many accounts were added to the cookie jar because they were in the
browser's token service.
</summary>
</histogram>
......@@ -29868,6 +29872,14 @@ Therefore, the affected-histogram name has to have at least one dot in it.
</summary>
</histogram>
<histogram name="Signin.Reconciler.RemovedFromCookieJar">
<owner>mlerman@chromium.org</owner>
<summary>
How many accounts were removed from the cookie jar because they were not in
the browser's token service.
</summary>
</histogram>
<histogram name="Signin.SignedInDurationBeforeSignout" units="minutes">
<owner>mlerman@chromium.org</owner>
<summary>
......@@ -55986,6 +55998,7 @@ To add a new entry, add it with any value and run test to compute valid value.
<affected-histogram name="Signin.Reconciler.AddedToChrome"/>
<affected-histogram name="Signin.Reconciler.AddedToCookieJar"/>
<affected-histogram name="Signin.Reconciler.DifferentPrimaryAccounts"/>
<affected-histogram name="Signin.Reconciler.RemovedFromCookieJar"/>
</histogram_suffixes>
<histogram_suffixes name="SocketType">
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