UserPolicyCache only becomes ready after policy has been fetched.

chrome/browser/chromeos/login/login_utils.cc

Index: chrome/browser/chromeos/login/login_utils.cc
diff --git a/chrome/browser/chromeos/login/login_utils.cc b/chrome/browser/chromeos/login/login_utils.cc
index e4bd18a52da67256ce19790e56df6a2e9e956d0d..522217650e6a9ac59ab1ec370b34002454b6c65b 100644
--- a/chrome/browser/chromeos/login/login_utils.cc
+++ b/chrome/browser/chromeos/login/login_utils.cc
@@ -103,22 +103,24 @@ const char kServiceScopeChromeOSDeviceManagement[] =
 // Task for fetching tokens from UI thread.
 class StartSyncOnUIThreadTask : public Task {
  public:
-  StartSyncOnUIThreadTask(
+  explicit StartSyncOnUIThreadTask(
       const GaiaAuthConsumer::ClientLoginResult& credentials)
       : credentials_(credentials) {}
   virtual ~StartSyncOnUIThreadTask() {}
 
   // Task override.
-  virtual void Run() {
+  virtual void Run() OVERRIDE {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     LoginUtils::Get()->FetchCookies(ProfileManager::GetDefaultProfile(),
                                     credentials_);
     LoginUtils::Get()->StartSync(ProfileManager::GetDefaultProfile(),
-                                   credentials_);
+                                 credentials_);
   }
 
  private:
   GaiaAuthConsumer::ClientLoginResult credentials_;
+
+  DISALLOW_COPY_AND_ASSIGN(StartSyncOnUIThreadTask);
 };
 
 // Transfers initial set of Profile cookies from the default profile.
@@ -132,7 +134,7 @@ class TransferDefaultCookiesOnIOThreadTask : public Task {
   virtual ~TransferDefaultCookiesOnIOThreadTask() {}
 
   // Task override.
-  virtual void Run() {
+  virtual void Run() OVERRIDE {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
     net::CookieStore* default_store =
         auth_context_->GetURLRequestContext()->cookie_store();
@@ -147,7 +149,7 @@ class TransferDefaultCookiesOnIOThreadTask : public Task {
   void InitializeCookieMonster(const net::CookieList& cookies) {
      DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
      net::CookieStore* new_store =
-       new_context_->GetURLRequestContext()->cookie_store();
+         new_context_->GetURLRequestContext()->cookie_store();
      net::CookieMonster* new_monster = new_store->GetCookieMonster();
 
     if (!new_monster->InitializeFrom(cookies)) {
@@ -224,8 +226,8 @@ class OAuthLoginVerifier : public GaiaOAuthConsumer {
   virtual void OnOAuthLoginSuccess(const std::string& sid,
                                    const std::string& lsid,
                                    const std::string& auth) OVERRIDE {
-    GaiaAuthConsumer::ClientLoginResult credentials(sid,
-      lsid, auth, std::string());
+    GaiaAuthConsumer::ClientLoginResult credentials(
+        sid, lsid, auth, std::string());
     UserManager::Get()->set_offline_login(false);
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                             new StartSyncOnUIThreadTask(credentials));
@@ -310,10 +312,15 @@ class UserSessionCookieFetcher : public GaiaAuthConsumer {
   DISALLOW_COPY_AND_ASSIGN(UserSessionCookieFetcher);
 };
 
-
-// Fetches an OAuth token and initializes user policy with it.
+// Fetches the oauth token for the device management service. Since Profile
+// creation might be blocking on a user policy fetch, this fetcher must always
+// send a (possibly empty) token to the BrowserPolicyConnector, which will then
+// let the policy subsystem proceed and resume Profile creation.
+// Sending the token even when no Profile is pending is also OK.
 class PolicyOAuthFetcher : public GaiaOAuthConsumer {
  public:
+  // Fetches the device management service's oauth token using |oauth1_token|
+  // and |oauth1_secret| as access tokens.
   PolicyOAuthFetcher(Profile* profile,
                      const std::string& oauth1_token,
                      const std::string& oauth1_secret)
@@ -323,34 +330,79 @@ class PolicyOAuthFetcher : public GaiaOAuthConsumer {
                        kServiceScopeChromeOSDeviceManagement),
         oauth1_token_(oauth1_token),
         oauth1_secret_(oauth1_secret) {
-    oauth_fetcher_.SetAutoFetchLimit(
-        GaiaOAuthFetcher::OAUTH2_SERVICE_ACCESS_TOKEN);
   }
+
+  // Fetches the device management service's oauth token, after also retrieving
+  // the access tokens.
+  explicit PolicyOAuthFetcher(Profile* profile)
+      : oauth_fetcher_(this,
+                       profile->GetRequestContext(),
+                       profile,
+                       kServiceScopeChromeOSDeviceManagement) {
+  }
+
   virtual ~PolicyOAuthFetcher() {}
 
   void Start() {
-    oauth_fetcher_.StartOAuthWrapBridge(
-        oauth1_token_, oauth1_secret_, GaiaConstants::kGaiaOAuthDuration,
-        std::string(kServiceScopeChromeOSDeviceManagement));
+    oauth_fetcher_.SetAutoFetchLimit(
+        GaiaOAuthFetcher::OAUTH2_SERVICE_ACCESS_TOKEN);
+
+    if (oauth1_token_.empty()) {
+      oauth_fetcher_.StartGetOAuthTokenRequest();
+    } else {
+      oauth_fetcher_.StartOAuthWrapBridge(
+          oauth1_token_, oauth1_secret_, GaiaConstants::kGaiaOAuthDuration,
+          std::string(kServiceScopeChromeOSDeviceManagement));
+    }
+  }
+
+  const std::string& oauth1_token() const { return oauth1_token_; }
+  const std::string& oauth1_secret() const { return oauth1_secret_; }
+
+ private:
+  virtual void OnGetOAuthTokenSuccess(const std::string& oauth_token) OVERRIDE {
+    VLOG(1) << "Got OAuth request token";
+  }
+
+  virtual void OnGetOAuthTokenFailure(
+      const GoogleServiceAuthError& error) OVERRIDE {
+    LOG(WARNING) << "Failed to get OAuth request token";
+    SetPolicyToken("");
+  }
+
+  virtual void OnOAuthGetAccessTokenSuccess(
+      const std::string& token,
+      const std::string& secret) OVERRIDE {
+    VLOG(1) << "Got OAuth access token";
+    oauth1_token_ = token;
+    oauth1_secret_ = secret;
+  }
+
+  virtual void OnOAuthGetAccessTokenFailure(
+      const GoogleServiceAuthError& error) OVERRIDE {
+    LOG(WARNING) << "Failed to get OAuth access token";
+    SetPolicyToken("");
   }
 
-  // GaiaOAuthConsumer implementation:
   virtual void OnOAuthWrapBridgeSuccess(
       const std::string& service_name,
       const std::string& token,
       const std::string& expires_in) OVERRIDE {
-    policy::BrowserPolicyConnector* browser_policy_connector =
-        g_browser_process->browser_policy_connector();
-    browser_policy_connector->RegisterForUserPolicy(token);
+    VLOG(1) << "Got OAuth access token for " << service_name;
+    SetPolicyToken(token);
   }
 
   virtual void OnOAuthWrapBridgeFailure(
       const std::string& service_name,
       const GoogleServiceAuthError& error) OVERRIDE {
     LOG(WARNING) << "Failed to get OAuth access token for " << service_name;
+    SetPolicyToken("");
+  }
+
+  void SetPolicyToken(const std::string& token) {
+    g_browser_process->browser_policy_connector()->RegisterForUserPolicy(token);
   }
 
- private:
   GaiaOAuthFetcher oauth_fetcher_;
   std::string oauth1_token_;
   std::string oauth1_secret_;
@@ -460,7 +512,7 @@ class LoginUtilsImpl : public LoginUtils,
   // GaiaOAuthConsumer overrides.
   virtual void OnGetOAuthTokenSuccess(const std::string& oauth_token) OVERRIDE;
   virtual void OnGetOAuthTokenFailure(
-    const GoogleServiceAuthError& error) OVERRIDE;
+      const GoogleServiceAuthError& error) OVERRIDE;
   virtual void OnOAuthGetAccessTokenSuccess(const std::string& token,
                                             const std::string& secret) OVERRIDE;
   virtual void OnOAuthGetAccessTokenFailure(
@@ -527,6 +579,7 @@ class LoginUtilsImpl : public LoginUtils,
   scoped_refptr<Authenticator> authenticator_;
   scoped_ptr<GaiaOAuthFetcher> oauth_fetcher_;
   scoped_ptr<PolicyOAuthFetcher> policy_oauth_fetcher_;
+  scoped_ptr<PolicyOAuthFetcher> policy_oauth_early_fetcher_;

[Mattias Nissler (ping if slow), 2011/11/11 11:41:23]

I don't see why we need two pointers here, wouldn't one do?

[Joao da Silva, 2011/11/11 12:55:14]

The 2nd pointer was used to distinguish early policy token fetches from those
done later or triggered through StartTokenServices. However, this fetch only has
to be performed once, so I've changed this to use only one fetcher now.

   scoped_ptr<OAuthLoginVerifier> oauth_login_verifier_;
 
   // Delegate to be fired when the profile will be prepared.
@@ -602,8 +655,23 @@ void LoginUtilsImpl::PrepareProfile(
   delegate_ = delegate;
 
   // Initialize user policy before the profile is created so the profile
-  // initialization code sees the policy settings.
-  g_browser_process->browser_policy_connector()->InitializeUserPolicy(username);
+  // initialization code sees the cached policy settings.
+  policy::BrowserPolicyConnector* connector =
+      g_browser_process->browser_policy_connector();
+  bool user_policy_needs_fetch = connector->InitializeUserPolicy(username);
+  if (user_policy_needs_fetch) {
+    if (using_oauth_ && authenticator_.get()) {
+      // Profile creation will block until user policy is fetched, which
+      // requires the DeviceManagement token. Try to fetch it now.
+      VLOG(1) << "Profile creation requires policy token, fetching now";
+      policy_oauth_early_fetcher_.reset(
+          new PolicyOAuthFetcher(authenticator_->authentication_profile()));
+      policy_oauth_early_fetcher_->Start();
+    } else {
+      // Tell the policy subsystem to resume without a policy fetch.
+      connector->RegisterForUserPolicy("");
+    }
+  }
 
   // The default profile will have been changed because the ProfileManager
   // will process the notification that the UserManager sends out.
@@ -632,12 +700,9 @@ void LoginUtilsImpl::OnProfileCreated(Profile* user_profile, Status status) {
   }
 
   // Initialize the user-policy backend.
-  policy::BrowserPolicyConnector* browser_policy_connector =
-      g_browser_process->browser_policy_connector();
-
   if (!using_oauth_) {
-    browser_policy_connector->SetUserPolicyTokenService(
-        user_profile->GetTokenService());
+    g_browser_process->browser_policy_connector()->
+        SetUserPolicyTokenService(user_profile->GetTokenService());
   }
 
   // We suck. This is a hack since we do not have the enterprise feature
@@ -653,6 +718,15 @@ void LoginUtilsImpl::OnProfileCreated(Profile* user_profile, Status status) {
   btl->AddLoginTimeMarker("UserProfileGotten", false);
 
   if (using_oauth_) {
+    // Reuse the access token fetched by the PolicyOAuthFetcher, if it was
+    // used to fetch policies before Profile creation.
+    if (policy_oauth_early_fetcher_.get()) {
+      VLOG(1) << "Resuming profile creation after fetching policy token";
+      StoreOAuth1AccessToken(user_profile,
+                             policy_oauth_early_fetcher_->oauth1_token(),
+                             policy_oauth_early_fetcher_->oauth1_secret());
+    }
+
     // Transfer cookies when user signs in using extension.
     if (has_cookies_) {
       // Transfer cookies from the profile that was used for authentication.
@@ -746,8 +820,8 @@ void LoginUtilsImpl::FetchOAuth1AccessToken(Profile* auth_profile) {
                                             auth_profile,
                                             kServiceScopeChromeOS));
   // Let's first get the Oauth request token and OAuth1 token+secret.
-  // One we get that, we will kick off individial requests for OAuth2 tokens for
-  // all our services.
+  // Once we get that, we will kick off individual requests for OAuth2 tokens
+  // for all our services.
   oauth_fetcher_->SetAutoFetchLimit(GaiaOAuthFetcher::OAUTH1_ALL_ACCESS_TOKEN);
   oauth_fetcher_->StartGetOAuthTokenRequest();
 }
@@ -800,7 +874,6 @@ void LoginUtilsImpl::StartSync(
   token_service->UpdateCredentials(credentials);
   if (token_service->AreCredentialsValid())
     token_service->StartFetchingTokens();
-
 }
 
 void LoginUtilsImpl::RespectLocalePreference(Profile* profile) {
@@ -1068,6 +1141,12 @@ void LoginUtilsImpl::OnOAuthGetAccessTokenSuccess(const std::string& token,
   VerifyOAuth1AccessToken(user_profile, token, secret);
 }
 
+void LoginUtilsImpl::OnOAuthGetAccessTokenFailure(
+    const GoogleServiceAuthError& error) {
+  // TODO(zelidrag): Pop up sync setup UI here?
+  LOG(WARNING) << "Failed fetching OAuth request token";

[Mattias Nissler (ping if slow), 2011/11/11 11:41:23]

This logged error.state() before, why did you drop this? It might also be
helpful to log more info about the fetch errors in the various LOG(WARNING)
incantations above.

[Joao da Silva, 2011/11/11 12:55:14]

Done.

+}
+
 void LoginUtilsImpl::FetchSecondaryTokens(Profile* offrecord_profile,
                                           const std::string& token,
                                           const std::string& secret) {
@@ -1142,11 +1221,18 @@ void LoginUtilsImpl::FetchCredentials(Profile* user_profile,
 void LoginUtilsImpl::FetchPolicyToken(Profile* offrecord_profile,
                                       const std::string& token,
                                       const std::string& secret) {
-  // Trigger oauth token fetch for user policy.
-  policy_oauth_fetcher_.reset(new PolicyOAuthFetcher(offrecord_profile,
-                                                     token,
-                                                     secret));
-  policy_oauth_fetcher_->Start();
+  if (policy_oauth_early_fetcher_.get()) {
+    // User policy has already been fetched. Reset the early policy fetcher
+    // now, so that subsequent calls of FetchSecondaryTokens fetch the policy
+    // token again.
+    policy_oauth_early_fetcher_.reset();
+  } else {
+    // Trigger oauth token fetch for user policy.
+    policy_oauth_fetcher_.reset(new PolicyOAuthFetcher(offrecord_profile,
+                                                       token,
+                                                       secret));
+    policy_oauth_fetcher_->Start();
+  }
 
   // TODO(zelidrag): We should add initialization of other services somewhere
   // here as well. This could be handled with TokenService class once it is
@@ -1159,12 +1245,6 @@ void LoginUtilsImpl::FetchPolicyToken(Profile* offrecord_profile,
   authenticator_ = NULL;
 }
 
-void LoginUtilsImpl::OnOAuthGetAccessTokenFailure(
-    const GoogleServiceAuthError& error) {
-  // TODO(zelidrag): Pop up sync setup UI here?
-  LOG(WARNING) << "Failed fetching OAuth v1 token, error: " << error.state();
-}
-
 void LoginUtilsImpl::OnOnlineStateChanged(bool online) {
   // If we come online for the first time after successful offline login,
   // we need to kick of OAuth token verification process again.