Device robot refresh token integrity validation.

chrome/browser/signin/oauth2_token_service.h

Index: chrome/browser/signin/oauth2_token_service.h
diff --git a/chrome/browser/signin/oauth2_token_service.h b/chrome/browser/signin/oauth2_token_service.h
index e96b9c17c3d84c16f364b707b646de6ef06fcb87..e4e91bc41a1325798a76d60047944e19e86e36be 100644
--- a/chrome/browser/signin/oauth2_token_service.h
+++ b/chrome/browser/signin/oauth2_token_service.h
@@ -26,7 +26,7 @@ class URLRequestContextGetter;
 class GoogleServiceAuthError;
 
 // Abstract base class for a service that fetches and caches OAuth2 access
-// tokens. Concrete subclasses should implement GetRefreshToken to return
+// tokens. Concrete subclasses should implement StartGetRefreshToken to provide
 // the appropriate refresh token.
 //
 // All calls are expected from the UI thread.
@@ -87,10 +87,6 @@ class OAuth2TokenService {
   virtual scoped_ptr<Request> StartRequest(const ScopeSet& scopes,
                                            Consumer* consumer);
 
-  // Returns true if a refresh token exists. If false, calls to
-  // |StartRequest| will result in a Consumer::OnGetTokenFailure callback.
-  bool RefreshTokenIsAvailable();
-
   // Mark an OAuth2 access token as invalid. This should be done if the token
   // was received from this class, but was not accepted by the server (e.g.,
   // the server returned 401 Unauthorized). The token will be removed from the
@@ -102,10 +98,46 @@ class OAuth2TokenService {
   int cache_size_for_testing() const;
 
  protected:
+  // Interface that StartGetRefreshToken calls back into.
+  class RefreshTokenValidationConsumer {
+   public:
+    virtual void OnRefreshTokenValidationComplete(
+        const std::string& refresh_token,
+        bool is_valid) = 0;
+  };
+
+  // Implements a cancelable |OAuth2TokenService::Request|, which should be
+  // operated on the UI thread.
+  class RequestImpl : public base::SupportsWeakPtr<RequestImpl>,
+                      public Request {
+   public:
+    // |consumer| is required to outlive this.
+    explicit RequestImpl(Consumer* consumer);
+    virtual ~RequestImpl();
+
+    // Informs |consumer_| that this request is completed.
+    void InformConsumer(const GoogleServiceAuthError& error,
+                        const std::string& access_token,
+                        const base::Time& expiration_date);
+
+   private:
+    // |consumer_| to call back when this request completes.
+    Consumer* const consumer_;
+  };
+
   // Subclasses should return the refresh token maintained.
   // If no token is available, return an empty string.
   virtual std::string GetRefreshToken() = 0;
 
+  // Subclasses can optionally implement this method to validate the given
+  // refresh token.  Return true if the validation is started and the
+  // callback should be expected, or false if validation is skipped and the
+  // token should be used directly.  This method may be invoked in parallel;
+  // subclasses must ensure that all consumers are notified of the results.
+  virtual bool StartRefreshTokenValidation(

[Mattias Nissler (ping if slow), 2013/06/17 05:34:17]

I wonder whether making the concept of refresh token validation visible on the
OAuth2TokenService level is actually a good idea. Have you thought about just
making GetRefreshToken asynchronous, i.e.:

void GetRefreshToken(base::Callback<void(const std::string&)>);

Then, you could hide away the validation part in DeviceOAuth2TokenService.

[David Roche, 2013/06/18 04:12:08]

I started doing that initially, but unfortunately it got to be messy.  The
problem is that the refresh token getter is on the token-service singleton,
while the async/concurrent fetch requests are on the inner Fetcher class.  So,
to stay with the current design of OAuth2TokenService, the fetching should stay
in the Fetcher, since it automatically takes care of handling concurrent
fetches, etc.  Then the obvious idea is to continue with the attempt to make
GetRefreshToken async, but call it from the Fetcher.  This was my attempt #2,
but it had issues b/c the OAuth2TokenService keeps the map of in-flight Fetchers
indexed by refresh_token/scopes pair.  So, I need the refresh token value before
the Fetcher's CreateAndStart method is called.  I considered a big refactor of
the OAuth2TokenService logic around Fetcher lifecycle tracking, so that it was a
2-phase affair -- the first tracking a list of Fetchers that were getting the
refresh token (indexed only by scope) and the 2nd phase of Fetchers that had a
refresh token and were fetching the access token (indexed by refresh token /
scope pair).  But, in the end that seemed like a lot of changes.  So, what you
see is attempt #3 ... keeping the change as simple as possible by having the
fetcher created with the refresh token initially (so it can be added to the
existing map), and then optionally validating the token in the Fetcher. 
Conceptually, validating the refresh token is something that other subclasses
could want to do in the future, so it seemed reasonable enough.  Although, I
agree with your initial design instinct ... it would have been nice to hide the
validation in DeviceOAuth2TokenService, if it could have been done relatively
easily.

+      const std::string refresh_token,
+      RefreshTokenValidationConsumer* consumer);
+
   // Subclasses can override if they want to report errors to the user.
   virtual void UpdateAuthError(const GoogleServiceAuthError& error);
 
@@ -121,33 +153,14 @@ class OAuth2TokenService {
   // given scopes.
   bool HasCacheEntry(const ScopeSet& scopes);
 
-  // Posts a task to fire the Consumer callback with the cached token.  Must
-  // only be called if HasCacheEntry() returns true.
+  // Posts a task to fetch the cached token for the given in-flight Request.
+  // Must only be called if HasCacheEntry() returns true.
   scoped_ptr<Request> StartCacheLookupRequest(const ScopeSet& scopes,
-                                              Consumer* consumer);
+                                              scoped_ptr<RequestImpl> request);
 
   // Clears the internal token cache.
   void ClearCache();
 
-  // Implements a cancelable |OAuth2TokenService::Request|, which should be
-  // operated on the UI thread.
-  class RequestImpl : public base::SupportsWeakPtr<RequestImpl>,
-                      public Request {
-   public:
-    // |consumer| is required to outlive this.
-    explicit RequestImpl(Consumer* consumer);
-    virtual ~RequestImpl();
-
-    // Informs |consumer_| that this request is completed.
-    void InformConsumer(const GoogleServiceAuthError& error,
-                        const std::string& access_token,
-                        const base::Time& expiration_date);
-
-   private:
-    // |consumer_| to call back when this request completes.
-    Consumer* const consumer_;
-  };
-
  private:
   // Class that fetches an OAuth2 access token for a given set of scopes and
   // OAuth2 refresh token.