Fix SAML cookie interception on ChromeOS with network service.

This is similar to r593703 which fixed the old sign-in flow to work on desktop. The problem is that with network service Set-Cookie headers aren't visible by webRequest API. The fix is to read the cookies using the CookieManager API in C++. Other minor fixes: -FakeGaia doesn't have to set a Path for oauth_code cookie. GAIA doesn't set it in production, and having it set means that the C++ cookie reading code would need to set that path which is unnecessary. -convert SAMLPolicyTest to read cookies through CookieManager Note this doesn't address the Set-Cookie addition in saml_handler.js which still needs to be fixed. Bug: 887061 Change-Id: I902bdf0921f26368d7749838c69f03064e7ea9b4 Reviewed-on: https://chromium-review.googlesource.com/c/1285349 Reviewed-by: Xiyuan Xia <xiyuan@chromium.org> Reviewed-by: Roger Tawa <rogerta@chromium.org> Commit-Queue: John Abd-El-Malek <jam@chromium.org> Cr-Commit-Position: refs/heads/master@{#600570}
Denger-Network · Oct 17, 2018 · 4920f91 · 4920f91
1 parent 90a499f
commit 4920f91
Show file tree

Hide file tree

Showing 13 changed files with 119 additions and 148 deletions.
diff --git a/chrome/browser/chromeos/login/enterprise_enrollment_browsertest.cc b/chrome/browser/chromeos/login/enterprise_enrollment_browsertest.cc
@@ -181,18 +181,7 @@ class EnterpriseEnrollmentTestBase : public LoginManagerTest {
 
   // Submits regular enrollment credentials.
   void SubmitEnrollmentCredentials() {
-    // Trigger an authCompleted event from the authenticator.
-    // clang-format off
-    js_checker().Evaluate(
-      "$('oauth-enrollment').authenticator_.dispatchEvent("
-          "new CustomEvent('authCompleted',"
-                          "{"
-                            "detail: {"
-                              "email: 'testuser@test.com',"
-                              "authCode: 'test_auth_code'"
-                            "}"
-                          "}));");
-    // clang-format on
+    enrollment_screen()->OnLoginDone("testuser@test.com", "test_auth_code");
   }
 
   void DisableAttributePromptUpdate() {

diff --git a/chrome/browser/chromeos/login/saml/saml_browsertest.cc b/chrome/browser/chromeos/login/saml/saml_browsertest.cc
@@ -24,6 +24,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/post_task.h"
+#include "base/test/bind_test_util.h"
 #include "base/values.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/login/existing_user_controller.h"
@@ -80,6 +81,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/storage_partition.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/test/browser_test_utils.h"
@@ -1045,12 +1047,6 @@ class SAMLPolicyTest : public SamlTest {
   void GetCookies();
 
  protected:
-  void GetCookiesOnIOThread(
-      const scoped_refptr<net::URLRequestContextGetter>& request_context,
-      const base::Closure& callback);
-  void StoreCookieList(const base::Closure& callback,
-                       const net::CookieList& cookie_list);
-
   policy::DevicePolicyCrosTestHelper test_helper_;
 
   // FakeDBusThreadManager uses FakeSessionManagerClient.
@@ -1287,30 +1283,17 @@ void SAMLPolicyTest::GetCookies() {
       user_manager::UserManager::Get()->GetActiveUser());
   ASSERT_TRUE(profile);
   base::RunLoop run_loop;
-  base::PostTaskWithTraits(
-      FROM_HERE, {content::BrowserThread::IO},
-      base::BindOnce(&SAMLPolicyTest::GetCookiesOnIOThread,
-                     base::Unretained(this),
-                     scoped_refptr<net::URLRequestContextGetter>(
-                         profile->GetRequestContext()),
-                     run_loop.QuitClosure()));
+  network::mojom::CookieManagerPtr cookie_manager;
+  content::BrowserContext::GetDefaultStoragePartition(profile)
+      ->GetCookieManagerForBrowserProcess()
+      ->GetAllCookies(base::BindLambdaForTesting(
+          [&](const std::vector<net::CanonicalCookie>& cookies) {
+            cookie_list_ = cookies;
+            run_loop.Quit();
+          }));
   run_loop.Run();
 }
 
-void SAMLPolicyTest::GetCookiesOnIOThread(
-    const scoped_refptr<net::URLRequestContextGetter>& request_context,
-    const base::Closure& callback) {
-  request_context->GetURLRequestContext()->cookie_store()->GetAllCookiesAsync(
-      base::BindOnce(&SAMLPolicyTest::StoreCookieList, base::Unretained(this),
-                     callback));
-}
-
-void SAMLPolicyTest::StoreCookieList(const base::Closure& callback,
-                                     const net::CookieList& cookie_list) {
-  cookie_list_ = cookie_list;
-  base::PostTaskWithTraits(FROM_HERE, {content::BrowserThread::UI}, callback);
-}
-
 IN_PROC_BROWSER_TEST_F(SAMLPolicyTest, PRE_NoSAML) {
   // Set the offline login time limit for SAML users to zero.
   SetSAMLOfflineSigninTimeLimitPolicy(0);

diff --git a/chrome/browser/resources/chromeos/login/login_shared.js b/chrome/browser/resources/chromeos/login/login_shared.js
@@ -351,7 +351,7 @@ cr.define('cr.ui', function() {
       });
 
       waitForOobeScreen('oauth-enrollment', function() {
-        chrome.send('oauthEnrollCompleteLogin', [username, 'authcode']);
+        chrome.send('oauthEnrollCompleteLogin', [username]);
       });
     }
   };

diff --git a/chrome/browser/resources/chromeos/login/md_login_shared.js b/chrome/browser/resources/chromeos/login/md_login_shared.js
@@ -352,7 +352,7 @@ cr.define('cr.ui', function() {
       });
 
       waitForOobeScreen('oauth-enrollment', function() {
-        chrome.send('oauthEnrollCompleteLogin', [username, 'authcode']);
+        chrome.send('oauthEnrollCompleteLogin', [username]);
       });
     }
   };

diff --git a/chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js b/chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js
@@ -124,13 +124,12 @@ login.createScreen('OAuthEnrollmentScreen', 'oauth-enrollment', function() {
           'authCompleted',
           (function(e) {
             var detail = e.detail;
-            if (!detail.email || !detail.authCode) {
+            if (!detail.email) {
               this.showError(
                   loadTimeData.getString('fatalEnrollmentError'), false);
               return;
             }
-            chrome.send(
-                'oauthEnrollCompleteLogin', [detail.email, detail.authCode]);
+            chrome.send('oauthEnrollCompleteLogin', [detail.email]);
           }).bind(this));
 
       this.offlineAdUi_.addEventListener('authCompleted', function(e) {

diff --git a/chrome/browser/resources/chromeos/login/screen_gaia_signin.js b/chrome/browser/resources/chromeos/login/screen_gaia_signin.js
@@ -1101,16 +1101,10 @@ login.createScreen('GaiaSigninScreen', 'gaia-signin', function() {
         chrome.send(
             'completeOfflineAuthentication',
             [credentials.email, credentials.password]);
-      } else if (credentials.authCode) {
-        chrome.send('completeAuthentication', [
-          credentials.gaiaId, credentials.email, credentials.password,
-          credentials.authCode, credentials.usingSAML, credentials.gapsCookie,
-          credentials.services
-        ]);
       } else {
-        chrome.send('completeLogin', [
+        chrome.send('completeAuthentication', [
           credentials.gaiaId, credentials.email, credentials.password,
-          credentials.usingSAML
+          credentials.usingSAML, credentials.services
         ]);
       }
 

diff --git a/chrome/browser/resources/gaia_auth_host/authenticator.js b/chrome/browser/resources/gaia_auth_host/authenticator.js
@@ -29,8 +29,6 @@ cr.define('cr.login', function() {
   var EMBEDDED_FORM_HEADER = 'google-accounts-embedded';
   var LOCATION_HEADER = 'location';
   var COOKIE_HEADER = 'cookie';
-  var SET_COOKIE_HEADER = 'set-cookie';
-  var OAUTH_CODE_COOKIE = 'oauth_code';
   var GAPS_COOKIE = 'GAPS';
   var SERVICE_ID = 'chromeoslogin';
   var EMBEDDED_SETUP_CHROMEOS_ENDPOINT = 'embedded/setup/chromeos';
@@ -140,10 +138,8 @@ cr.define('cr.login', function() {
     this.initialFrameUrl_ = null;
     this.reloadUrl_ = null;
     this.trusted_ = true;
-    this.oauthCode_ = null;
     this.gapsCookie_ = null;
     this.gapsCookieSent_ = false;
-    this.newGapsCookie_ = null;
     this.readyFired_ = false;
     this.webviewEventManager_ = WebviewEventManager.create();
 
@@ -191,10 +187,8 @@ cr.define('cr.login', function() {
     this.email_ = null;
     this.gaiaId_ = null;
     this.password_ = null;
-    this.oauthCode_ = null;
     this.gapsCookie_ = null;
     this.gapsCookieSent_ = false;
-    this.newGapsCookie_ = null;
     this.readyFired_ = false;
     this.chooseWhatToSync_ = false;
     this.skipForNow_ = false;
@@ -302,7 +296,6 @@ cr.define('cr.login', function() {
     this.clientId_ = data.clientId;
     this.gapsCookie_ = data.gapsCookie;
     this.gapsCookieSent_ = false;
-    this.newGapsCookie_ = null;
     this.dontResizeNonEmbeddedPages = data.dontResizeNonEmbeddedPages;
     this.chromeOSApiVersion_ = data.chromeOSApiVersion;
 
@@ -554,16 +547,6 @@ cr.define('cr.login', function() {
         // URL will contain a source=3 field.
         var location = decodeURIComponent(header.value);
         this.chooseWhatToSync_ = !!location.match(/(\?|&)source=3($|&)/);
-      } else if (this.isNewGaiaFlow && headerName == SET_COOKIE_HEADER) {
-        var headerValue = header.value;
-        if (headerValue.startsWith(OAUTH_CODE_COOKIE + '=')) {
-          this.oauthCode_ =
-              headerValue.substring(OAUTH_CODE_COOKIE.length + 1).split(';')[0];
-        }
-        if (headerValue.startsWith(GAPS_COOKIE + '=')) {
-          this.newGapsCookie_ =
-              headerValue.substring(GAPS_COOKIE.length + 1).split(';')[0];
-        }
       }
     }
   };
@@ -609,6 +592,8 @@ cr.define('cr.login', function() {
 
       for (var i = 0, l = headers.length; i < l; ++i) {
         if (headers[i].name == COOKIE_HEADER) {
+          // TODO(jam): this doesn't work with network service since webRequest
+          // won't see the Cookie header. Who uses this?
           headers[i].value = this.updateCookieValue_(
               headers[i].value, GAPS_COOKIE, gapsCookie);
           found = true;
@@ -860,13 +845,11 @@ cr.define('cr.login', function() {
             email: this.email_ || '',
             gaiaId: this.gaiaId_ || '',
             password: this.password_ || '',
-            authCode: this.oauthCode_,
             usingSAML: this.authFlow == AuthFlow.SAML,
             chooseWhatToSync: this.chooseWhatToSync_,
             skipForNow: this.skipForNow_,
             sessionIndex: this.sessionIndex_ || '',
             trusted: this.trusted_,
-            gapsCookie: this.newGapsCookie_ || this.gapsCookie_ || '',
             services: this.services_ || [],
           }
         }));
@@ -919,9 +902,9 @@ cr.define('cr.login', function() {
    */
   Authenticator.prototype.onSamlApiPasswordAdded_ = function(e) {
     // Saml API 'add' password might be received after the 'loadcommit' event.
-    // In such case, maybeCompleteAuth_ should be attempted again if oauth code
-    // is available.
-    if (this.oauthCode_)
+    // In such case, maybeCompleteAuth_ should be attempted again if GAIA ID is
+    // available.
+    if (this.gaiaId_)
       this.maybeCompleteAuth_();
   };
 
@@ -997,7 +980,7 @@ cr.define('cr.login', function() {
    * @private
    */
   Authenticator.prototype.onLoadCommit_ = function(e) {
-    if (this.oauthCode_)
+    if (this.gaiaId_)
       this.maybeCompleteAuth_();
   };
 

diff --git a/chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc b/chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
@@ -35,6 +35,7 @@
 #include "components/login/localized_values_builder.h"
 #include "components/policy/core/browser/cloud/message_util.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/storage_partition.h"
 #include "google_apis/gaia/gaia_auth_util.h"
 #include "google_apis/gaia/gaia_urls.h"
 #include "google_apis/gaia/google_service_auth_error.h"
@@ -766,11 +767,40 @@ void EnrollmentScreenHandler::HandleClose(const std::string& reason) {
   }
 }
 
-void EnrollmentScreenHandler::HandleCompleteLogin(
-    const std::string& user,
-    const std::string& auth_code) {
+void EnrollmentScreenHandler::HandleCompleteLogin(const std::string& user) {
   VLOG(1) << "HandleCompleteLogin";
   observe_network_failure_ = false;
+
+  // When the network service is enabled, the webRequest API doesn't expose
+  // cookie headers. So manually fetch the cookies for the GAIA URL from the
+  // CookieManager.
+  login::SigninPartitionManager* signin_partition_manager =
+      login::SigninPartitionManager::Factory::GetForBrowserContext(
+          Profile::FromWebUI(web_ui()));
+  content::StoragePartition* partition =
+      signin_partition_manager->GetCurrentStoragePartition();
+  net::CookieOptions cookie_options;
+  cookie_options.set_include_httponly();
+
+  partition->GetCookieManagerForBrowserProcess()->GetCookieList(
+      GaiaUrls::GetInstance()->gaia_url(), cookie_options,
+      base::BindOnce(&EnrollmentScreenHandler::OnGetCookiesForCompleteLogin,
+                     weak_ptr_factory_.GetWeakPtr(), user));
+}
+
+void EnrollmentScreenHandler::OnGetCookiesForCompleteLogin(
+    const std::string& user,
+    const std::vector<net::CanonicalCookie>& cookies) {
+  std::string auth_code;
+  for (const auto& cookie : cookies) {
+    if (cookie.Name() == "oauth_code") {
+      auth_code = cookie.Value();
+      break;
+    }
+  }
+
+  DCHECK(!auth_code.empty());
+
   DCHECK(controller_);
   controller_->OnLoginDone(gaia::SanitizeEmail(user), auth_code);
 }

diff --git a/chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h b/chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h
@@ -17,6 +17,10 @@
 #include "chrome/browser/ui/webui/chromeos/login/network_state_informer.h"
 #include "net/base/net_errors.h"
 
+namespace net {
+class CanonicalCookie;
+}
+
 namespace chromeos {
 
 class ErrorScreensHistogramHelper;
@@ -96,8 +100,10 @@ class EnrollmentScreenHandler
   // Handlers for WebUI messages.
   void HandleToggleFakeEnrollment();
   void HandleClose(const std::string& reason);
-  void HandleCompleteLogin(const std::string& user,
-                           const std::string& auth_code);
+  void HandleCompleteLogin(const std::string& user);
+  void OnGetCookiesForCompleteLogin(
+      const std::string& user,
+      const std::vector<net::CanonicalCookie>& cookies);
   void HandleAdCompleteLogin(const std::string& machine_name,
                              const std::string& distinguished_name,
                              const std::string& encryption_types,

diff --git a/chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc b/chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc
@@ -67,6 +67,7 @@
 #include "components/version_info/version_info.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/storage_partition.h"
 #include "google_apis/gaia/gaia_auth_util.h"
 #include "google_apis/gaia/gaia_urls.h"
 #include "mojo/public/cpp/bindings/callback_helpers.h"
@@ -743,13 +744,49 @@ void GaiaScreenHandler::HandleCompleteAuthentication(
     const std::string& gaia_id,
     const std::string& email,
     const std::string& password,
-    const std::string& auth_code,
     bool using_saml,
-    const std::string& gaps_cookie,
     const ::login::StringList& services) {
   if (!LoginDisplayHost::default_host())
     return;
 
+  // When the network service is enabled, the webRequest API doesn't expose
+  // cookie headers. So manually fetch the cookies for the GAIA URL from the
+  // CookieManager.
+  login::SigninPartitionManager* signin_partition_manager =
+      login::SigninPartitionManager::Factory::GetForBrowserContext(
+          Profile::FromWebUI(web_ui()));
+  content::StoragePartition* partition =
+      signin_partition_manager->GetCurrentStoragePartition();
+  net::CookieOptions cookie_options;
+  cookie_options.set_include_httponly();
+
+  partition->GetCookieManagerForBrowserProcess()->GetCookieList(
+      GaiaUrls::GetInstance()->gaia_url(), cookie_options,
+      base::BindOnce(&GaiaScreenHandler::OnGetCookiesForCompleteAuthentication,
+                     weak_factory_.GetWeakPtr(), gaia_id, email, password,
+                     using_saml, services));
+}
+
+void GaiaScreenHandler::OnGetCookiesForCompleteAuthentication(
+    const std::string& gaia_id,
+    const std::string& email,
+    const std::string& password,
+    bool using_saml,
+    const ::login::StringList& services,
+    const std::vector<net::CanonicalCookie>& cookies) {
+  std::string auth_code, gaps_cookie;
+  for (const auto& cookie : cookies) {
+    if (cookie.Name() == "oauth_code")
+      auth_code = cookie.Value();
+    else if (cookie.Name() == "GAPS")
+      gaps_cookie = cookie.Value();
+  }
+
+  if (auth_code.empty()) {
+    HandleCompleteLogin(gaia_id, email, password, using_saml);
+    return;
+  }
+
   DCHECK(!email.empty());
   DCHECK(!gaia_id.empty());
   const std::string sanitized_email = gaia::SanitizeEmail(email);
@@ -883,9 +920,6 @@ void GaiaScreenHandler::DoCompleteLogin(const std::string& gaia_id,
                                         const std::string& typed_email,
                                         const std::string& password,
                                         bool using_saml) {
-  if (!LoginDisplayHost::default_host())
-    return;
-
   if (using_saml && !using_saml_api_)
     RecordSAMLScrapingVerificationResultInHistogram(true);