From ede4117a85d34a1c117169d538b297020e69de87 Mon Sep 17 00:00:00 2001 From: fgorski Date: Fri, 7 Nov 2014 16:38:35 -0800 Subject: [GCM] Fetching OAuth2 tokens periodically in account tracker BUG=374969 R=zea@chromium.org Review URL: https://codereview.chromium.org/631343002 Cr-Commit-Position: refs/heads/master@{#303340} --- chrome/browser/services/gcm/gcm_account_tracker.cc | 90 +++++++++++++++++----- chrome/browser/services/gcm/gcm_account_tracker.h | 28 ++++++- .../services/gcm/gcm_account_tracker_unittest.cc | 90 +++++++++++++++++++++- 3 files changed, 185 insertions(+), 23 deletions(-) (limited to 'chrome/browser/services') diff --git a/chrome/browser/services/gcm/gcm_account_tracker.cc b/chrome/browser/services/gcm/gcm_account_tracker.cc index a2cdba0..0521688 100644 --- a/chrome/browser/services/gcm/gcm_account_tracker.cc +++ b/chrome/browser/services/gcm/gcm_account_tracker.cc @@ -15,11 +15,18 @@ namespace gcm { namespace { + +// Scopes needed by the OAuth2 access tokens. const char kGCMGroupServerScope[] = "https://www.googleapis.com/auth/gcm"; const char kGCMCheckinServerScope[] = "https://www.googleapis.com/auth/android_checkin"; +// Name of the GCM account tracker for the OAuth2TokenService. const char kGCMAccountTrackerName[] = "gcm_account_tracker"; +// Minimum token validity when sending to GCM groups server. const int64 kMinimumTokenValidityMs = 500; +// Token reporting interval, when no account changes are detected. +const int64 kTokenReportingIntervalMs = 12 * 60 * 60 * 1000; // 12 hours in ms. + } // namespace GCMAccountTracker::AccountInfo::AccountInfo(const std::string& email, @@ -36,7 +43,8 @@ GCMAccountTracker::GCMAccountTracker( : OAuth2TokenService::Consumer(kGCMAccountTrackerName), account_tracker_(account_tracker.release()), driver_(driver), - shutdown_called_(false) { + shutdown_called_(false), + reporting_weak_ptr_factory_(this) { } GCMAccountTracker::~GCMAccountTracker() { @@ -56,11 +64,6 @@ void GCMAccountTracker::Start() { driver_->AddConnectionObserver(this); std::vector accounts = account_tracker_->GetAccounts(); - if (accounts.empty()) { - CompleteCollectingTokens(); - return; - } - for (std::vector::const_iterator iter = accounts.begin(); iter != accounts.end(); ++iter) { @@ -70,7 +73,22 @@ void GCMAccountTracker::Start() { } } - GetAllNeededTokens(); + if (IsTokenReportingRequired()) + ReportTokens(); + else + ScheduleReportTokens(); +} + +void GCMAccountTracker::ScheduleReportTokens() { + DVLOG(1) << "Deferring the token reporting for: " + << GetTimeToNextTokenReporting().InSeconds() << " seconds."; + + reporting_weak_ptr_factory_.InvalidateWeakPtrs(); + base::MessageLoop::current()->PostDelayedTask( + FROM_HERE, + base::Bind(&GCMAccountTracker::ReportTokens, + reporting_weak_ptr_factory_.GetWeakPtr()), + GetTimeToNextTokenReporting()); } void GCMAccountTracker::OnAccountAdded(const gaia::AccountIds& ids) { @@ -115,7 +133,7 @@ void GCMAccountTracker::OnGetTokenSuccess( } DeleteTokenRequest(request); - CompleteCollectingTokens(); + ReportTokens(); } void GCMAccountTracker::OnGetTokenFailure( @@ -137,21 +155,22 @@ void GCMAccountTracker::OnGetTokenFailure( } DeleteTokenRequest(request); - CompleteCollectingTokens(); + ReportTokens(); } void GCMAccountTracker::OnConnected(const net::IPEndPoint& ip_endpoint) { - if (SanitizeTokens()) - GetAllNeededTokens(); + if (IsTokenReportingRequired()) + ReportTokens(); } void GCMAccountTracker::OnDisconnected() { // We are disconnected, so no point in trying to work with tokens. } -void GCMAccountTracker::CompleteCollectingTokens() { +void GCMAccountTracker::ReportTokens() { + SanitizeTokens(); // Make sure all tokens are valid. - if (SanitizeTokens()) { + if (IsTokenFetchingRequired()) { GetAllNeededTokens(); return; } @@ -198,13 +217,14 @@ void GCMAccountTracker::CompleteCollectingTokens() { if (!account_tokens.empty() || account_removed) { DVLOG(1) << "Reporting the tokens to driver: " << account_tokens.size(); driver_->SetAccountTokens(account_tokens); + driver_->SetLastTokenFetchTime(base::Time::Now()); + ScheduleReportTokens(); } else { DVLOG(1) << "No tokens and nothing removed. Skipping callback."; } } -bool GCMAccountTracker::SanitizeTokens() { - bool tokens_needed = false; +void GCMAccountTracker::SanitizeTokens() { for (AccountInfos::iterator iter = account_infos_.begin(); iter != account_infos_.end(); ++iter) { @@ -216,12 +236,43 @@ bool GCMAccountTracker::SanitizeTokens() { iter->second.state = TOKEN_NEEDED; iter->second.expiration_time = base::Time(); } + } +} + +bool GCMAccountTracker::IsTokenReportingRequired() const { + if (GetTimeToNextTokenReporting() == base::TimeDelta()) + return true; + + bool reporting_required = false; + for (AccountInfos::const_iterator iter = account_infos_.begin(); + iter != account_infos_.end(); + ++iter) { + if (iter->second.state == ACCOUNT_REMOVED) + reporting_required = true; + } + return reporting_required; +} + +bool GCMAccountTracker::IsTokenFetchingRequired() const { + bool token_needed = false; + for (AccountInfos::const_iterator iter = account_infos_.begin(); + iter != account_infos_.end(); + ++iter) { if (iter->second.state == TOKEN_NEEDED) - tokens_needed = true; + token_needed = true; } - return tokens_needed; + return token_needed; +} + +base::TimeDelta GCMAccountTracker::GetTimeToNextTokenReporting() const { + base::TimeDelta time_till_next_reporting = + driver_->GetLastTokenFetchTime() + + base::TimeDelta::FromMilliseconds(kTokenReportingIntervalMs) - + base::Time::Now(); + return time_till_next_reporting < base::TimeDelta() ? + base::TimeDelta() : time_till_next_reporting; } void GCMAccountTracker::DeleteTokenRequest( @@ -235,6 +286,9 @@ void GCMAccountTracker::DeleteTokenRequest( void GCMAccountTracker::GetAllNeededTokens() { // Only start fetching tokens if driver is running, they have a limited // validity time and GCM connection is a good indication of network running. + // If the GetAllNeededTokens was called as part of periodic schedule, it may + // not have network. In that case the next network change will trigger token + // fetching. if (!driver_->IsConnected()) return; @@ -282,7 +336,7 @@ void GCMAccountTracker::OnAccountSignedOut(const gaia::AccountIds& ids) { iter->second.access_token.clear(); iter->second.state = ACCOUNT_REMOVED; - CompleteCollectingTokens(); + ReportTokens(); } OAuth2TokenService* GCMAccountTracker::GetTokenService() { diff --git a/chrome/browser/services/gcm/gcm_account_tracker.h b/chrome/browser/services/gcm/gcm_account_tracker.h index 0b1c5e8..39f357e 100644 --- a/chrome/browser/services/gcm/gcm_account_tracker.h +++ b/chrome/browser/services/gcm/gcm_account_tracker.h @@ -14,12 +14,21 @@ #include "google_apis/gaia/account_tracker.h" #include "google_apis/gaia/oauth2_token_service.h" +namespace base { +class Time; +} + namespace gcm { class GCMDriver; // Class for reporting back which accounts are signed into. It is only meant to // be used when the user is signed into sync. +// +// This class makes a check for tokens periodically, to make sure the user is +// still logged into the profile, so that in the case that the user is not, we +// can immediately report that to the GCM and stop messages addressed to that +// user from ever reaching Chrome. class GCMAccountTracker : public gaia::AccountTracker::Observer, public OAuth2TokenService::Consumer, public GCMConnectionObserver { @@ -76,6 +85,8 @@ class GCMAccountTracker : public gaia::AccountTracker::Observer, } private: + friend class GCMAccountTrackerTest; + // Maps account keys to account states. Keyed by account_ids as used by // OAuth2TokenService. typedef std::map AccountInfos; @@ -97,13 +108,22 @@ class GCMAccountTracker : public gaia::AccountTracker::Observer, void OnConnected(const net::IPEndPoint& ip_endpoint) override; void OnDisconnected() override; + // Schedules token reporting. + void ScheduleReportTokens(); // Report the list of accounts with OAuth2 tokens back using the |callback_| // function. If there are token requests in progress, do nothing. - void CompleteCollectingTokens(); + void ReportTokens(); // Verify that all of the tokens are ready to be passed down to the GCM // Driver, e.g. none of them has expired or is missing. Returns true if not // all tokens are valid and a fetching yet more tokens is required. - bool SanitizeTokens(); + void SanitizeTokens(); + // Indicates whether token reporting is required, either because it is due, or + // some of the accounts were removed. + bool IsTokenReportingRequired() const; + // Indicates whether there are tokens that still need fetching. + bool IsTokenFetchingRequired() const; + // Gets the time until next token reporting. + base::TimeDelta GetTimeToNextTokenReporting() const; // Deletes a token request. Should be called from OnGetTokenSuccess(..) or // OnGetTokenFailure(..). void DeleteTokenRequest(const OAuth2TokenService::Request* request); @@ -133,6 +153,10 @@ class GCMAccountTracker : public gaia::AccountTracker::Observer, ScopedVector pending_token_requests_; + // Creates weak pointers used to postpone reporting tokens. See + // ScheduleReportTokens. + base::WeakPtrFactory reporting_weak_ptr_factory_; + DISALLOW_COPY_AND_ASSIGN(GCMAccountTracker); }; diff --git a/chrome/browser/services/gcm/gcm_account_tracker_unittest.cc b/chrome/browser/services/gcm/gcm_account_tracker_unittest.cc index 2211d76..eb8030a 100644 --- a/chrome/browser/services/gcm/gcm_account_tracker_unittest.cc +++ b/chrome/browser/services/gcm/gcm_account_tracker_unittest.cc @@ -8,8 +8,6 @@ #include #include "base/memory/scoped_ptr.h" -#include "base/message_loop/message_loop.h" -#include "base/run_loop.h" #include "components/gcm_driver/fake_gcm_driver.h" #include "google_apis/gaia/fake_identity_provider.h" #include "google_apis/gaia/fake_oauth2_token_service.h" @@ -76,6 +74,8 @@ class CustomFakeGCMDriver : public FakeGCMDriver { void AddConnectionObserver(GCMConnectionObserver* observer) override; void RemoveConnectionObserver(GCMConnectionObserver* observer) override; bool IsConnected() const override { return connected_; } + base::Time GetLastTokenFetchTime() override; + void SetLastTokenFetchTime(const base::Time& time) override; // Test results and helpers. void SetConnected(bool connected); @@ -98,6 +98,7 @@ class CustomFakeGCMDriver : public FakeGCMDriver { GCMConnectionObserver* last_connection_observer_; GCMConnectionObserver* removed_connection_observer_; net::IPEndPoint ip_endpoint_; + base::Time last_token_fetch_time_; DISALLOW_COPY_AND_ASSIGN(CustomFakeGCMDriver); }; @@ -141,6 +142,15 @@ void CustomFakeGCMDriver::ResetResults() { removed_connection_observer_ = NULL; } + +base::Time CustomFakeGCMDriver::GetLastTokenFetchTime() { + return last_token_fetch_time_; +} + +void CustomFakeGCMDriver::SetLastTokenFetchTime(const base::Time& time) { + last_token_fetch_time_ = time; +} + } // namespace class GCMAccountTrackerTest : public testing::Test { @@ -165,6 +175,11 @@ class GCMAccountTrackerTest : public testing::Test { GCMAccountTracker* tracker() { return tracker_.get(); } CustomFakeGCMDriver* driver() { return &driver_; } + // Accessors to private methods of account tracker. + bool IsFetchingRequired() const; + bool IsTokenReportingRequired() const; + base::TimeDelta GetTimeToNextTokenReporting() const; + private: CustomFakeGCMDriver driver_; @@ -237,6 +252,18 @@ void GCMAccountTrackerTest::IssueError(const std::string& account_key) { GoogleServiceAuthError(GoogleServiceAuthError::SERVICE_UNAVAILABLE)); } +bool GCMAccountTrackerTest::IsFetchingRequired() const { + return tracker_->IsTokenFetchingRequired(); +} + +bool GCMAccountTrackerTest::IsTokenReportingRequired() const { + return tracker_->IsTokenReportingRequired(); +} + +base::TimeDelta GCMAccountTrackerTest::GetTimeToNextTokenReporting() const { + return tracker_->GetTimeToNextTokenReporting(); +} + TEST_F(GCMAccountTrackerTest, NoAccounts) { EXPECT_FALSE(driver()->update_accounts_called()); tracker()->Start(); @@ -403,7 +430,7 @@ TEST_F(GCMAccountTrackerTest, PostponeTokenFetchingUntilConnected) { EXPECT_EQ(1UL, tracker()->get_pending_token_request_count()); } -TEST_F(GCMAccountTrackerTest, IvalidateExpiredTokens) { +TEST_F(GCMAccountTrackerTest, InvalidateExpiredTokens) { StartAccountSignIn(kAccountId1); StartAccountSignIn(kAccountId2); tracker()->Start(); @@ -421,6 +448,63 @@ TEST_F(GCMAccountTrackerTest, IvalidateExpiredTokens) { EXPECT_EQ(1UL, tracker()->get_pending_token_request_count()); } +// Testing for whether there are still more tokens to be fetched. Typically the +// need for token fetching triggers immediate request, unless there is no +// connection, that is why connection is set on and off in this test. +TEST_F(GCMAccountTrackerTest, IsTokenFetchingRequired) { + tracker()->Start(); + driver()->SetConnected(false); + EXPECT_FALSE(IsFetchingRequired()); + StartAccountSignIn(kAccountId1); + FinishAccountSignIn(kAccountId1); + EXPECT_TRUE(IsFetchingRequired()); + + driver()->SetConnected(true); + EXPECT_FALSE(IsFetchingRequired()); // Indicates that fetching has started. + IssueAccessToken(kAccountId1); + EXPECT_FALSE(IsFetchingRequired()); + + driver()->SetConnected(false); + StartAccountSignIn(kAccountId2); + FinishAccountSignIn(kAccountId2); + EXPECT_TRUE(IsFetchingRequired()); + + IssueExpiredAccessToken(kAccountId2); + // Make sure that if the token was expired it is still needed. + EXPECT_TRUE(IsFetchingRequired()); +} + +// Tests what is the expected time to the next token fetching. +TEST_F(GCMAccountTrackerTest, GetTimeToNextTokenReporting) { + tracker()->Start(); + // At this point the last token fetch time is never. + EXPECT_EQ(base::TimeDelta(), GetTimeToNextTokenReporting()); + + driver()->SetLastTokenFetchTime(base::Time::Now()); + EXPECT_TRUE(GetTimeToNextTokenReporting() <= + base::TimeDelta::FromSeconds(12 * 60 * 60)); +} + +// Tests conditions when token reporting is required. +TEST_F(GCMAccountTrackerTest, IsTokenReportingRequired) { + tracker()->Start(); + // Required because it is overdue. + EXPECT_TRUE(IsTokenReportingRequired()); + + // Not required because it just happened. + driver()->SetLastTokenFetchTime(base::Time::Now()); + EXPECT_FALSE(IsTokenReportingRequired()); + + SignInAccount(kAccountId1); + IssueAccessToken(kAccountId1); + driver()->ResetResults(); + // Reporting was triggered, which means testing for required will give false, + // but we have the update call. + SignOutAccount(kAccountId1); + EXPECT_TRUE(driver()->update_accounts_called()); + EXPECT_FALSE(IsTokenReportingRequired()); +} + // TODO(fgorski): Add test for adding account after removal >> make sure it does // not mark removal. -- cgit v1.1