Handling of multiple concurrent requests from different clients in OAuth2TokenService

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 d63fbdf33861520b8b40c8d575c62e7770795636..3d9003aaa4eb3cc88c625f9cb2d8528ff22cd795 100644
--- a/chrome/browser/signin/oauth2_token_service.h
+++ b/chrome/browser/signin/oauth2_token_service.h
@@ -10,12 +10,16 @@
 #include <string>
 
 #include "base/basictypes.h"
+#include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/time/time.h"
+#include "base/timer/timer.h"
 #include "google_apis/gaia/google_service_auth_error.h"
+#include "google_apis/gaia/oauth2_access_token_consumer.h"
+#include "google_apis/gaia/oauth2_access_token_fetcher.h"
 
 namespace base {
 class Time;
@@ -119,8 +123,12 @@ class OAuth2TokenService {
 
   // This method does the same as |StartRequest| except it uses |client_id| and
   // |client_secret| to identify OAuth client app instead of using
-  // Chrome's default values.
+  // Chrome's default values. |request_origin| is used to differentiate where
+  // request originates from. It's expected to be empty for requests from
+  // the internal chrome services while we will use webapp id for their

[Andrew T Wilson (Slow), 2013/08/19 14:04:19]

So, one thing this CL (and the associated bug) is missing is an explanation
about why we want to silo requests on a per-request-origin basis.

If two apps want to access the same service (they pass the same scope - say
userinfo.email) why would we want to mint two separate access tokens? Seems like
siloing only based on client_id would be sufficient.

Also, it seems like we can't implement this API at all on Android, because
Chrome doesn't do the caching on that platform (we rely entirely on the Android
AccountManager to do the caching, and it doesn't know anything about
request_origin, client_id, etc). Not a huge issue, but maybe we should #if
!defined(OS_ANDROID) here...

+  // requests.
   virtual scoped_ptr<Request> StartRequestForClient(
+      const std::string& request_origin,
       const std::string& client_id,
       const std::string& client_secret,
       const ScopeSet& scopes,
@@ -150,6 +158,17 @@ class OAuth2TokenService {
   void set_max_authorization_token_fetch_retries_for_testing(int max_retries);
 
  protected:
+  struct ClientScopeSet {
+    ClientScopeSet(const std::string& request_origin,
+                   const std::string& client_id,
+                   const ScopeSet& scopes);
+    bool operator<(const ClientScopeSet& set) const;
+
+    std::string request_origin;
+    std::string client_id;
+    ScopeSet scopes;
+  };
+
   // Implements a cancelable |OAuth2TokenService::Request|, which should be
   // operated on the UI thread.
   // TODO(davidroche): move this out of header file.
@@ -180,19 +199,22 @@ class OAuth2TokenService {
   // Add a new entry to the cache.
   // Subclasses can override if there are implementation-specific reasons
   // that an access token should ever not be cached.
-  virtual void RegisterCacheEntry(const std::string& refresh_token,
+  virtual void RegisterCacheEntry(const std::string& request_origin,
+                                  const std::string& client_id,
+                                  const std::string& refresh_token,
                                   const ScopeSet& scopes,
                                   const std::string& access_token,
                                   const base::Time& expiration_date);
 
   // Returns true if GetCacheEntry would return a valid cache entry for the
   // given scopes.
-  bool HasCacheEntry(const ScopeSet& scopes);
+  bool HasCacheEntry(const ClientScopeSet& client_scopes);
 
   // Posts a task to fire the Consumer callback with the cached token.  Must
   // Must only be called if HasCacheEntry() returns true.
-  scoped_ptr<Request> StartCacheLookupRequest(const ScopeSet& scopes,
-                                              Consumer* consumer);
+  scoped_ptr<Request> StartCacheLookupRequest(
+      const ClientScopeSet& client_scopes,
+      Consumer* consumer);
 
   // Clears the internal token cache.
   void ClearCache();
@@ -211,15 +233,152 @@ class OAuth2TokenService {
   void FireRefreshTokensCleared();
 
  private:
-  // Derived classes must provide a request context used for fetching access
-  // tokens with the |StartRequest| method.
-  virtual net::URLRequestContextGetter* GetRequestContext() = 0;
-
   // Class that fetches an OAuth2 access token for a given set of scopes and

[Andrew T Wilson (Slow), 2013/08/19 14:04:19]

I don't really like the fact that we're exposing this in our header file just so
a test can call GetWaitingRequestCount().

Can we move this back into the .cc file, and instead just expose a simple
"GetNumPendingRequestsForTesting(request_origin, client_id, refresh_token,
scopes)"?

   // OAuth2 refresh token.
-  class Fetcher;
+
+  // Class that fetches OAuth2 access tokens for given scopes and refresh token.
+  //
+  // It aims to meet OAuth2TokenService's requirements on token fetching. Retry
+  // mechanism is used to handle failures.
+  //
+  // To use this class, call CreateAndStart() to create and start a Fetcher.
+  //
+  // The Fetcher will call back the service by calling
+  // OAuth2TokenService::OnFetchComplete() when it completes fetching, if it is
+  // not destructed before it completes fetching; if the Fetcher is destructed
+  // before it completes fetching, the service will never be called back. The
+  // Fetcher destructs itself after calling back the service when finishes
+  // fetching.
+  //
+  // Requests that are waiting for the fetching results of this Fetcher can be
+  // added to the Fetcher by calling
+  // OAuth2TokenService::Fetcher::AddWaitingRequest() before the Fetcher
+  // completes fetching.
+  //
+  // The waiting requests are taken as weak pointers and they can be deleted.
+  // The waiting requests will be called back with fetching results if they are
+  // not deleted
+  // - when the Fetcher completes fetching, if the Fetcher is not destructed
+  //   before it completes fetching, or
+  // - when the Fetcher is destructed if the Fetcher is destructed before it
+  //   completes fetching (in this case, the waiting requests will be called
+  //   back with error).
+  class Fetcher : public OAuth2AccessTokenConsumer {
+   public:
+    // Creates a Fetcher and starts fetching an OAuth2 access token for
+    // |refresh_token| and |scopes| in the request context obtained by |getter|.
+    // The given |oauth2_token_service| will be informed when fetching is done.
+    static Fetcher* CreateAndStart(OAuth2TokenService* oauth2_token_service,
+                                   net::URLRequestContextGetter* getter,
+                                   const std::string& request_origin,
+                                   const std::string& client_id,
+                                   const std::string& client_secret,
+                                   const std::string& refresh_token,
+                                   const ScopeSet& scopes,
+                                   base::WeakPtr<RequestImpl> waiting_request);
+    virtual ~Fetcher();
+
+    // Add a request that is waiting for the result of this Fetcher.
+    void AddWaitingRequest(base::WeakPtr<RequestImpl> waiting_request);
+
+    // Returns count of waiting requests.
+    size_t GetWaitingRequestCount() const;
+
+    void Cancel();
+
+    const ScopeSet& GetScopeSet() const;
+    const std::string& GetRefreshToken() const;
+    const std::string& GetClientId() const;
+    const std::string& GetRequestOrigin() const;
+
+    // The error result from this fetcher.
+    const GoogleServiceAuthError& error() const { return error_; }
+
+   protected:
+     // OAuth2AccessTokenConsumer
+    virtual void OnGetTokenSuccess(const std::string& access_token,
+                                   const base::Time& expiration_date) OVERRIDE;
+    virtual void OnGetTokenFailure(
+        const GoogleServiceAuthError& error) OVERRIDE;
+
+   private:
+    Fetcher(OAuth2TokenService* oauth2_token_service,
+            net::URLRequestContextGetter* getter,
+            const std::string& request_origin,
+            const std::string& client_id,
+            const std::string& client_secret,
+            const std::string& refresh_token,
+            const OAuth2TokenService::ScopeSet& scopes,
+            base::WeakPtr<RequestImpl> waiting_request);
+    void Start();
+    void InformWaitingRequests();
+    void InformWaitingRequestsAndDelete();
+    static bool ShouldRetry(const GoogleServiceAuthError& error);
+    int64 ComputeExponentialBackOffMilliseconds(int retry_num);
+
+    // |oauth2_token_service_| remains valid for the life of this Fetcher, since
+    // this Fetcher is destructed in the dtor of the OAuth2TokenService or is
+    // scheduled for deletion at the end of OnGetTokenFailure/OnGetTokenSuccess
+    // (whichever comes first).
+    OAuth2TokenService* const oauth2_token_service_;
+    scoped_refptr<net::URLRequestContextGetter> getter_;
+    const std::string refresh_token_;
+    const ScopeSet scopes_;
+    std::vector<base::WeakPtr<RequestImpl> > waiting_requests_;
+
+    int retry_number_;
+    base::OneShotTimer<Fetcher> retry_timer_;
+    scoped_ptr<OAuth2AccessTokenFetcher> fetcher_;
+
+    // Variables that store fetch results.
+    // Initialized to be GoogleServiceAuthError::SERVICE_UNAVAILABLE to handle
+    // destruction.
+    GoogleServiceAuthError error_;
+    std::string access_token_;
+    base::Time expiration_date_;
+
+    // Token fetch request origin identifier.
+    std::string request_origin_;
+
+    // OAuth2 client id and secret.
+    std::string client_id_;
+    std::string client_secret_;
+
+    DISALLOW_COPY_AND_ASSIGN(Fetcher);
+  };
+
   friend class Fetcher;
 
+  // The parameters used to fetch an OAuth2 access token.
+  struct FetchParameters {
+    FetchParameters(const std::string& request_origin,
+                    const std::string& client_id,
+                    const std::string& refresh_token,
+                    const ScopeSet& scopes);
+    bool operator<(const FetchParameters& params) const;
+
+    // Request origin identifier. It's empty for internal chrome services
+    // requests but the requests originating from webapps should be identified
+    // by their originating extension_id.
+    std::string request_origin;
+    // OAuth2 client id.
+    std::string client_id;
+    // Refresh token used for minting access tokens within this request.
+    std::string refresh_token;
+    // URL scopes for the requested access token.
+    ScopeSet scopes;
+  };
+
+  typedef std::map<FetchParameters, Fetcher*> PendingFetcherMap;
+
+  const PendingFetcherMap& GetPendingFetchersForTesting() const {
+    return pending_fetchers_;
+  }
+
+  // Derived classes must provide a request context used for fetching access
+  // tokens with the |StartRequest| method.
+  virtual net::URLRequestContextGetter* GetRequestContext() = 0;
+
   // Struct that contains the information of an OAuth2 access token.
   struct CacheEntry {
     std::string access_token;
@@ -231,6 +390,7 @@ class OAuth2TokenService {
   // client app instead of using Chrome's default values.
   scoped_ptr<Request> StartRequestForClientWithContext(
       net::URLRequestContextGetter* getter,
+      const std::string& request_origin,
       const std::string& client_id,
       const std::string& client_secret,
       const ScopeSet& scopes,
@@ -238,14 +398,14 @@ class OAuth2TokenService {
 
   // Returns a currently valid OAuth2 access token for the given set of scopes,
   // or NULL if none have been cached. Note the user of this method should
-  // ensure no entry with the same |scopes| is added before the usage of the
-  // returned entry is done.
-  const CacheEntry* GetCacheEntry(const ScopeSet& scopes);
+  // ensure no entry with the same |client_scopes| is added before the usage of
+  // the returned entry is done.
+  const CacheEntry* GetCacheEntry(const ClientScopeSet& client_scopes);
 
 
   // Removes an access token for the given set of scopes from the cache.
   // Returns true if the entry was removed, otherwise false.
-  bool RemoveCacheEntry(const OAuth2TokenService::ScopeSet& scopes,
+  bool RemoveCacheEntry(const ClientScopeSet& client_scopes,
                         const std::string& token_to_remove);
 
 
@@ -256,15 +416,12 @@ class OAuth2TokenService {
   void CancelFetchers(std::vector<Fetcher*> fetchers_to_cancel);
 
   // The cache of currently valid tokens.
-  typedef std::map<ScopeSet, CacheEntry> TokenCache;
+  typedef std::map<ClientScopeSet, CacheEntry> TokenCache;
   TokenCache token_cache_;
 
-  // The parameters (refresh token and scope set) used to fetch an OAuth2 access
-  // token.
-  typedef std::pair<std::string, ScopeSet> FetchParameters;
   // A map from fetch parameters to a fetcher that is fetching an OAuth2 access
   // token using these parameters.
-  std::map<FetchParameters, Fetcher*> pending_fetchers_;
+  PendingFetcherMap pending_fetchers_;
 
   // List of observers to notify when token availiability changes.
   // Makes sure list is empty on destruction.
@@ -273,6 +430,11 @@ class OAuth2TokenService {
   // Maximum number of retries in fetching an OAuth2 access token.
   static int max_fetch_retry_num_;
 
+  FRIEND_TEST_ALL_PREFIXES(OAuth2TokenServiceTest, ClientScopeSetOrderTest);
+  FRIEND_TEST_ALL_PREFIXES(OAuth2TokenServiceTest, FetchParametersOrderTest);
+  FRIEND_TEST_ALL_PREFIXES(OAuth2TokenServiceTest,
+                           SameScopesRequestedForDifferentClients);
+
   DISALLOW_COPY_AND_ASSIGN(OAuth2TokenService);
 };