PW change success detection for Ping

Ping does not have a particular success URL -
it just redirects back to the "returnurl" on success.

We can detect this if we listen for a POSTed request
to the password change URL, and then for a response that
redirects us to the returnurl, and not back to the
password change URL again.

This involves listening for redirects, not just URLs -
since visiting the returnurl normally would not mean
success.

I have modified all such success detection attempts to
listen for redirects - even when we have a success URL
to listen for eg "?status=0" - it is a bit more certain
to listen for a redirect *from* the password change URL
*to* the success URL, instead of just listening for a
visit to the success URL by itself.

Bug: 930109
Change-Id: Idfd081a603ca402367de774f6253e4af9b84bbdf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1878489Reviewed-by: Roman Sorokin [CET] <rsorokin@chromium.org>
Commit-Queue: A Olsen <olsen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#711235}

chrome/browser/resources/gaia_auth_host/password_change_authenticator.js

@@ -76,12 +76,12 @@ cr.define('cr.samlPasswordChange', function() {
   }
 
   /**
-   * @param {Object} details The web-request details.
+   * @param {URL?} postUrl Where the password change request was POSTed.
+   * @param {URL?} redirectUrl Where the response redirected the browser.
    * @return {boolean} True if we detect that a password change was successful.
    */
-  function detectPasswordChangeSuccess(details) {
-    const url = safeParseUrl_(details.url);
-    if (!url) {
+  function detectPasswordChangeSuccess(postUrl, redirectUrl) {
+    if (!postUrl || !redirectUrl) {
       return false;
     }
 
@@ -90,20 +90,26 @@ cr.define('cr.samlPasswordChange', function() {
     // that an otherwise unsupported IdP can also send it as a success message.
     // TODO(https://crbug.com/930109): Consider removing this entirely, or,
     // using a more self-documenting parameter like 'passwordChanged=1'.
-    if (url.searchParams.get('status') == '0') {
+    if (redirectUrl.searchParams.get('status') == '0') {
       return true;
     }
 
-    const pageProvider = detectProvider_(url);
+    const pageProvider = detectProvider_(postUrl);
     // These heuristics work for the following SAML IdPs:
     if (pageProvider == PasswordChangePageProvider.ADFS) {
-      return url.searchParams.get('status') == '0';
+      return redirectUrl.searchParams.get('status') == '0';
     }
     if (pageProvider == PasswordChangePageProvider.AZURE) {
-      return url.searchParams.get('ReturnCode') == '0';
+      return redirectUrl.searchParams.get('ReturnCode') == '0';
+    }
+    if (pageProvider == PasswordChangePageProvider.PING) {
+      // The returnurl is always preserved until password change succeeds - then
+      // it is no longer needed.
+      return !!postUrl.searchParams.get('returnurl') &&
+          !redirectUrl.searchParams.get('returnurl');
     }
 
-    // We can't currently detect success for Okta or Ping just by inspecting the
+    // We can't currently detect success for Okta just by inspecting the
     // URL or even response headers. To inspect the response body, we need
     // to inject scripts onto their page (see okta_detect_success_injected.js).
 
@@ -163,11 +169,12 @@ cr.define('cr.samlPasswordChange', function() {
           this.samlHandler_, 'authPageLoaded',
           this.onAuthPageLoaded_.bind(this));
 
-      // Listen for completed main-frame requests to check for password-change
-      // success.
+      // Listen for main-frame redirects to check for success - we can mostly
+      // detect success by detecting we POSTed something to the password-change
+      // URL, and the response redirected us to a particular success URL.
       this.webviewEventManager_.addWebRequestEventListener(
-          this.webview_.request.onCompleted,
-          this.onCompleted_.bind(this),
+          this.webview_.request.onBeforeRedirect,
+          this.onBeforeRedirect_.bind(this),
           {urls: ['*://*/*'], types: ['main_frame']},
       );
 
@@ -180,6 +187,13 @@ cr.define('cr.samlPasswordChange', function() {
         run_at: 'document_start'
       }]);
 
+      // Connect to the script running in Okta web pages once it loads.
+      this.webviewEventManager_.addWebRequestEventListener(
+          this.webview_.request.onCompleted,
+          this.onOktaCompleted_.bind(this),
+          {urls: ['*://*.okta.com/*'], types: ['main_frame']},
+      );
+
       // Okta-detect-success-inject script signals success by posting a message
       // that says "passwordChangeSuccess", which we listen for:
       this.webviewEventManager_.addEventListener(
@@ -265,25 +279,32 @@ cr.define('cr.samlPasswordChange', function() {
       this.webview_.focus();
     }
 
+
     /**
      * Invoked when a new document loading completes.
      * @param {Object} details The web-request details.
      * @private
      */
-    onCompleted_(details) {
-      if (detectPasswordChangeSuccess(details)) {
+    onBeforeRedirect_(details) {
+      if (details.method == 'POST' &&
+          detectPasswordChangeSuccess(
+              safeParseUrl_(details.url), safeParseUrl_(details.redirectUrl))) {
         this.onPasswordChangeSuccess_();
       }
+    }
 
+    /**
+     * Invoked when loading completes on an Okta page.
+     * @param {Object} details The web-request details.
+     * @private
+     */
+    onOktaCompleted_(details) {
       // Okta_detect_success_injected.js needs to be contacted by the parent,
       // so that it can send messages back to the parent.
-      const pageProvider = detectProvider_(safeParseUrl_(details.url));
-      if (pageProvider == PasswordChangePageProvider.OKTA) {
-        // Using setTimeout gives the page time to finish initializing.
-        setTimeout(() => {
-          this.webview_.contentWindow.postMessage('connect', details.url);
-        }, 1000);
-      }
+      // Using setTimeout gives the page time to finish initializing.
+      setTimeout(() => {
+        this.webview_.contentWindow.postMessage('connect', details.url);
+      }, 1000);
     }
 
     /**

chrome/browser/resources/gaia_auth_host/password_change_authenticator_test.unitjs

@@ -33,44 +33,67 @@ PasswordChangeAuthenticatorUnitTest = class extends testing.Test {
     ];
   }
 
-  assertSuccess(details) {
-    assertTrue(this.detectSuccess(details));
+  assertSuccess(postUrl, redirectUrl) {
+    assertTrue(this.detectSuccess(postUrl, redirectUrl));
   }
 
-  assertNotSuccess(details, responseData) {
-    assertFalse(this.detectSuccess(details));
+  assertNotSuccess(postUrl, redirectUrl) {
+    assertFalse(this.detectSuccess(postUrl, redirectUrl));
   }
 
-  detectSuccess(details) {
-    if (typeof details == 'string') {
-      details = {'url': details};
-    }
-    return cr.samlPasswordChange.detectPasswordChangeSuccess(details);
+  detectSuccess(postUrl, redirectUrl) {
+    postUrl = (typeof postUrl == 'string') ? new URL(postUrl) : postUrl;
+    redirectUrl =
+        (typeof redirectUrl == 'string') ? new URL(redirectUrl) : redirectUrl;
+    return cr.samlPasswordChange.detectPasswordChangeSuccess(postUrl,
+                                                             redirectUrl);
   }
 }
 
 TEST_F('PasswordChangeAuthenticatorUnitTest', 'DetectAdfsSuccess', function() {
   const endpointUrl = EXAMPLE_ADFS_ENDPOINT;
 
-  this.assertNotSuccess(endpointUrl);
-  this.assertNotSuccess(endpointUrl + '?status=1');
-  this.assertSuccess(endpointUrl + '?status=0');
+  this.assertNotSuccess(endpointUrl, endpointUrl);
+  this.assertNotSuccess(endpointUrl, endpointUrl + '?status=1');
+
+  this.assertSuccess(endpointUrl, endpointUrl + '?status=0');
+  this.assertSuccess(endpointUrl + '?status=1', endpointUrl + '?status=0');
 
   // We allow "status=0" to count as success everywhere right now, but this
   // should be narrowed down to ADFS - see the TODO in the code.
-  this.assertSuccess(EXAMPLE_AZURE_ENDPOINT + '?status=0');
+  this.assertSuccess(EXAMPLE_AZURE_ENDPOINT,
+                     EXAMPLE_AZURE_ENDPOINT + '?status=0');
 });
 
 TEST_F('PasswordChangeAuthenticatorUnitTest', 'DetectAzureSuccess', function() {
   const endpointUrl = EXAMPLE_AZURE_ENDPOINT;
   const extraParam = 'BrandContextID=O123';
 
-  this.assertNotSuccess(endpointUrl);
-  this.assertNotSuccess(endpointUrl + '?' + extraParam);
-  this.assertNotSuccess(endpointUrl + '?ReturnCode=1&' + extraParam);
-  this.assertNotSuccess(endpointUrl + '?' + extraParam + '&ReturnCode=1');
-  this.assertNotSuccess(EXAMPLE_PING_ENDPOINT + '?ReturnCode=0');
+  this.assertNotSuccess(endpointUrl, endpointUrl);
+  this.assertNotSuccess(endpointUrl, endpointUrl + '?' + extraParam);
+  this.assertNotSuccess(endpointUrl, endpointUrl + '?ReturnCode=1&' + extraParam);
+  this.assertNotSuccess(endpointUrl,
+                        endpointUrl + '?' + extraParam + '&ReturnCode=1');
+  this.assertNotSuccess(EXAMPLE_PING_ENDPOINT,
+                        endpointUrl + '?ReturnCode=0');
+
+  this.assertSuccess(endpointUrl,
+                     endpointUrl + '?ReturnCode=0');
+  this.assertSuccess(endpointUrl + '?' + extraParam,
+                     endpointUrl + '?ReturnCode=0&' + extraParam);
+  this.assertSuccess(endpointUrl + '?' + extraParam,
+                     endpointUrl + '?' + extraParam + '&ReturnCode=0');
+});
+
+TEST_F('PasswordChangeAuthenticatorUnitTest', 'DetectPingSuccess', function() {
+  const endpointUrl = EXAMPLE_PING_ENDPOINT;
+
+  this.assertNotSuccess(endpointUrl, endpointUrl);
+  this.assertNotSuccess(endpointUrl + '?returnurl=https://desktop.pingone.com',
+                        endpointUrl + '?returnurl=https://desktop.pingone.com');
+  this.assertNotSuccess(endpointUrl,
+                        endpointUrl + '?returnurl=https://desktop.pingone.com');
 
-  this.assertSuccess(endpointUrl + '?ReturnCode=0&' + extraParam);
-  this.assertSuccess(endpointUrl + '?' + extraParam + '&ReturnCode=0');
+  this.assertSuccess(endpointUrl + '?returnurl=https://desktop.pingone.com',
+                     'https://desktop.pingone.com/Selection?cmd=selection');
 });
\ No newline at end of file