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..f5970b6c505ae507cd22f03539df6d41505e7cae 100644
--- a/chrome/browser/signin/oauth2_token_service.h
+++ b/chrome/browser/signin/oauth2_token_service.h
@@ -100,6 +100,7 @@ class OAuth2TokenService {
 
   // A set of scopes in OAuth2 authentication.
   typedef std::set<std::string> ScopeSet;
+  typedef std::pair<std::string, ScopeSet> ClientScopeSet;
 
   OAuth2TokenService();
   virtual ~OAuth2TokenService();
@@ -117,6 +118,7 @@ class OAuth2TokenService {
   virtual scoped_ptr<Request> StartRequest(const ScopeSet& scopes,
                                            Consumer* consumer);
 
+#if defined(OS_CHROMEOS)
   // 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.
@@ -125,6 +127,7 @@ class OAuth2TokenService {
       const std::string& client_secret,
       const ScopeSet& scopes,
       Consumer* consumer);
+#endif

[(NOT FOR CODE REVIEWS), 2013/08/07 19:56:49]

+Michael and Filip.

I think this should be in all version of chrome.  This would be useful for the
chrome identity js api.

[Michael Courage, 2013/08/07 21:07:31]

Our input to a token request is (extension_id, client_id, scopes), so I suspect
we'd add a new public method with those parameters, which would call to the
common StartRequestForClientWithContext.

[zel, 2013/08/08 01:34:24]

To answer the first question, atwilson@ ask me to #ifdef this for ChromeOS in
the bug. I can easily remove this.

I can also easily add extension_id to the list of parameters. We can maybe make
it more generic - something like |request_origin|. WDYT?

 
   // This method does the same as |StartRequest| except it uses the request
   // context given by |getter| instead of using the one returned by
@@ -180,19 +183,21 @@ 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& 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();
@@ -238,14 +243,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,12 +261,14 @@ 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;
+  // The parameters (client_id, refresh token and scope set) used to fetch an
+  // OAuth2 access token.
+  typedef std::pair<std::pair<std::string /* client_id */,
+                              std::string /* refresh_token */>,
+                    ScopeSet> FetchParameters;

[(NOT FOR CODE REVIEWS), 2013/08/07 19:56:49]

Would be clearer to declare a struct with 3 distinct members.

[fgorski, 2013/08/07 21:36:59]

Alternative solution would be to use ClientScopeSet, as that is how we are
keying the TokenCache:
std::pair<std::string /*refresh_token*/, ClientScopeSet>
(Fetcher would not have to expose client_id, but a ClientScopeSet).

[zel, 2013/08/08 01:34:24]

I've used more complex key now and made this a struct - PTAL.

   // A map from fetch parameters to a fetcher that is fetching an OAuth2 access
   // token using these parameters.
   std::map<FetchParameters, Fetcher*> pending_fetchers_;