Commit ab9952d0 authored by Colin Blundell's avatar Colin Blundell Committed by Commit Bot

Have FakeProfileOAuth2TokenService cancel requests

I just discovered that FakeProfileOAuth2TokenService never cancels
access token requests:
- OAuth2TokenService::FetchOAuth2Token() is virtual and overridden by
  FakePO2TS. This is the method that creates pending fetchers for
  access token requests. FakePO2TS' override results in O2TS not
  creating any pending fetchers for access token requests.
- OAuth2TokenService::CancelRequests{ForAccount}(), however, are *not*
  virtual. O2TS simply looks in its set of pending fetchers to cancel
  the relevant ones.

The combination of these facts means that requests are not cancelled
in response to CancelRequests{ForAccount}() when FakePO2TS is used.
This in turn means that production flows are not accurately reflected,
and blocks creating a unittest for a just-discovered production bug
(that itself would perhaps have not landed if this situation was not
as it is).

This CL remedies the problem by having FakePO2TS override the
now-virtual methods for cancelling access token requests to respond
appropriately. It also updates a few unittests that had previously
(either intentionally or unintentionally) worked around the lack of
cancellation by cancelling requests manually after token revocation.

Change-Id: Id935831294849a191e6034f3fe1360bea1dddc9e
Reviewed-on: https://chromium-review.googlesource.com/1154925
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: default avatarMihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579352}
parent b2771571
...@@ -119,6 +119,21 @@ FakeProfileOAuth2TokenService::GetPendingRequests() { ...@@ -119,6 +119,21 @@ FakeProfileOAuth2TokenService::GetPendingRequests() {
return valid_requests; return valid_requests;
} }
void FakeProfileOAuth2TokenService::CancelAllRequests() {
CompleteRequests(
"", true, ScopeSet(),
GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED),
std::string(), base::Time());
}
void FakeProfileOAuth2TokenService::CancelRequestsForAccount(
const std::string& account_id) {
CompleteRequests(
account_id, true, ScopeSet(),
GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED),
std::string(), base::Time());
}
void FakeProfileOAuth2TokenService::FetchOAuth2Token( void FakeProfileOAuth2TokenService::FetchOAuth2Token(
RequestImpl* request, RequestImpl* request,
const std::string& account_id, const std::string& account_id,
......
...@@ -87,6 +87,10 @@ class FakeProfileOAuth2TokenService : public ProfileOAuth2TokenService { ...@@ -87,6 +87,10 @@ class FakeProfileOAuth2TokenService : public ProfileOAuth2TokenService {
protected: protected:
// OAuth2TokenService overrides. // OAuth2TokenService overrides.
void CancelAllRequests() override;
void CancelRequestsForAccount(const std::string& account_id) override;
void FetchOAuth2Token( void FetchOAuth2Token(
RequestImpl* request, RequestImpl* request,
const std::string& account_id, const std::string& account_id,
......
...@@ -284,11 +284,13 @@ class OAuth2TokenService { ...@@ -284,11 +284,13 @@ class OAuth2TokenService {
// used to request the tokens. // used to request the tokens.
void ClearCacheForAccount(const std::string& account_id); void ClearCacheForAccount(const std::string& account_id);
// Cancels all requests that are currently in progress. // Cancels all requests that are currently in progress. Virtual so it can be
void CancelAllRequests(); // overridden for tests.
virtual void CancelAllRequests();
// Cancels all requests related to a given |account_id|. // Cancels all requests related to a given |account_id|. Virtual so it can be
void CancelRequestsForAccount(const std::string& account_id); // overridden for tests.
virtual void CancelRequestsForAccount(const std::string& account_id);
// Fetches an OAuth token for the specified client/scopes. Virtual so it can // Fetches an OAuth token for the specified client/scopes. Virtual so it can
// be overridden for tests and for platform-specific behavior. // be overridden for tests and for platform-specific behavior.
......
...@@ -228,16 +228,13 @@ TEST_F(AccessTokenFetcherTest, RefreshTokenRevoked) { ...@@ -228,16 +228,13 @@ TEST_F(AccessTokenFetcherTest, RefreshTokenRevoked) {
run_loop.Run(); run_loop.Run();
// Simulate the refresh token getting invalidated. In this case, pending // Revoke the refresh token, which should cancel all pending requests. The
// access token requests get canceled, and the fetcher should *not* retry. // fetcher should *not* retry.
token_service()->RevokeCredentials(account_id);
EXPECT_CALL( EXPECT_CALL(
callback, callback,
Run(GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED), Run(GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED),
AccessTokenInfo())); AccessTokenInfo()));
token_service()->IssueErrorForAllPendingRequestsForAccount( token_service()->RevokeCredentials(account_id);
account_id,
GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED));
} }
TEST_F(AccessTokenFetcherTest, FailedAccessTokenRequest) { TEST_F(AccessTokenFetcherTest, FailedAccessTokenRequest) {
......
...@@ -334,13 +334,12 @@ TEST_F(PrimaryAccountAccessTokenFetcherTest, ...@@ -334,13 +334,12 @@ TEST_F(PrimaryAccountAccessTokenFetcherTest,
// Simulate the user signing out while the access token request is pending. // Simulate the user signing out while the access token request is pending.
// In this case, the pending request gets canceled, and the fetcher should // In this case, the pending request gets canceled, and the fetcher should
// *not* retry. // *not* retry.
identity_test_env()->ClearPrimaryAccount();
EXPECT_CALL( EXPECT_CALL(
callback, callback,
Run(GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED), Run(GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED),
AccessTokenInfo())); AccessTokenInfo()));
identity_test_env()->WaitForAccessTokenRequestIfNecessaryAndRespondWithError(
GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED)); identity_test_env()->ClearPrimaryAccount();
} }
#endif // !OS_CHROMEOS #endif // !OS_CHROMEOS
...@@ -357,15 +356,13 @@ TEST_F(PrimaryAccountAccessTokenFetcherTest, ...@@ -357,15 +356,13 @@ TEST_F(PrimaryAccountAccessTokenFetcherTest,
callback.Get(), callback.Get(),
PrimaryAccountAccessTokenFetcher::Mode::kWaitUntilAvailable); PrimaryAccountAccessTokenFetcher::Mode::kWaitUntilAvailable);
// Simulate the refresh token getting invalidated. In this case, pending // Simulate the refresh token getting removed. In this case, pending
// access token requests get canceled, and the fetcher should *not* retry. // access token requests get canceled, and the fetcher should *not* retry.
identity_test_env()->RemoveRefreshTokenForPrimaryAccount();
EXPECT_CALL( EXPECT_CALL(
callback, callback,
Run(GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED), Run(GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED),
AccessTokenInfo())); AccessTokenInfo()));
identity_test_env()->WaitForAccessTokenRequestIfNecessaryAndRespondWithError( identity_test_env()->RemoveRefreshTokenForPrimaryAccount();
GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED));
} }
TEST_F(PrimaryAccountAccessTokenFetcherTest, TEST_F(PrimaryAccountAccessTokenFetcherTest,
......
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