LoginDetection: Do not consider redirects for max navigations limit

Currently redirects are also counted for applying the maximum number of
navigations limit between OAuth start and complete. This CL changes it
to only consider the navigations and not the redirects.

Bug: 1162327
Change-Id: I44d11f4c04256bebeeb743513f605720092ae2c7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2604946
Commit-Queue: rajendrant <rajendrant@chromium.org>
Reviewed-by: Michael Crouse <mcrouse@chromium.org>
Cr-Commit-Position: refs/heads/master@{#839714}

chrome/browser/login_detection/oauth_login_detector.cc

@@ -84,6 +84,8 @@ base::Optional<GURL> OAuthLoginDetector::GetSuccessfulLoginFlowSite(
       }
     }
   }
+  if (login_flow_info_)
+    login_flow_info_->count_navigations_since_login_flow_start++;
   return base::nullopt;
 }
 
@@ -125,7 +127,6 @@ bool OAuthLoginDetector::CheckSuccessfulLoginCompletion(
   // site.
   if (GetSiteNameForURL(navigation_url) ==
       GetSiteNameForURL(login_flow_info_->oauth_provider_site)) {
-    login_flow_info_->count_navigations_since_login_flow_start++;
     return false;
   }
 
@@ -139,12 +140,9 @@ bool OAuthLoginDetector::CheckSuccessfulLoginCompletion(
 
   // PopUp based login completion flow should only navigate within the OAuth
   // provider site.
-  if (popup_opener_navigation_site_) {
+  if (popup_opener_navigation_site_)
     login_flow_info_.reset();
-    return false;
-  }
 
-  login_flow_info_->count_navigations_since_login_flow_start++;
   return false;
 }
 

chrome/browser/login_detection/oauth_login_detector_unittest.cc

@@ -86,11 +86,28 @@ TEST_F(OAuthLoginDetectorTest, LoginNotDetectedForHTTP) {
 // Test that small number of intermediate navigations within OAuth start and
 // completion are allowed.
 TEST_F(OAuthLoginDetectorTest, IntermediateNavigationsAfterOAuthStart) {
-  EXPECT_EQ(GURL("https://foo.com/"),
-            *oauth_login_detector_->GetSuccessfulLoginFlowSite(
+  EXPECT_FALSE(oauth_login_detector_->GetSuccessfulLoginFlowSite(
       GURL("https://foo.com/login.html"),
-                {GURL("https://oauth.com/authenticate?client_id=123"),
+      {
+          GURL("https://oauth.com/authenticate?client_id=123"),
+          GURL("https://oauth.com/login"),
+      }));
+  EXPECT_FALSE(oauth_login_detector_->GetSuccessfulLoginFlowSite(
       GURL("https://oauth.com/login"),
+      {
+          GURL("https://oauth.com/login?username=123&password=123"),
+          GURL("https://oauth.com/relogin"),
+      }));
+  EXPECT_FALSE(oauth_login_detector_->GetSuccessfulLoginFlowSite(
+      GURL("https://oauth.com/relogin"),
+      {
+          GURL("https://oauth.com/login?username=123&password=123"),
+          GURL("https://oauth.com/relogin"),
+      }));
+  EXPECT_TRUE(oauth_login_detector_->GetSuccessfulLoginFlowSite(
+      GURL("https://oauth.com/relogin"),
+      {GURL("https://oauth.com/login?username=123&password=123"),
+       GURL("https://oauth.com/loginsuccess"),
        GURL("https://foo.com/redirect?code=secret")}));
   EXPECT_FALSE(oauth_login_detector_->GetPopUpLoginFlowSite());
 }
@@ -99,10 +116,47 @@ TEST_F(OAuthLoginDetectorTest, IntermediateNavigationsAfterOAuthStart) {
 // happen within OAuth start and completion.
 TEST_F(OAuthLoginDetectorTest, TooManyIntermediateNavigationsAfterOAuthStart) {
   EXPECT_FALSE(oauth_login_detector_->GetSuccessfulLoginFlowSite(
+      GURL("https://foo.com/login.html"),
+      {
+          GURL("https://oauth.com/authenticate?client_id=123"),
+          GURL("https://oauth.com/login"),
+      }));
+  EXPECT_FALSE(oauth_login_detector_->GetSuccessfulLoginFlowSite(
+      GURL("https://oauth.com/login"),
+      {
+          GURL("https://oauth.com/login?username=123&password=123"),
+          GURL("https://oauth.com/relogin"),
+      }));
+  EXPECT_FALSE(oauth_login_detector_->GetSuccessfulLoginFlowSite(
+      GURL("https://oauth.com/relogin"),
+      {
+          GURL("https://oauth.com/login?username=123&password=123"),
+          GURL("https://oauth.com/relogin"),
+      }));
+  EXPECT_FALSE(oauth_login_detector_->GetSuccessfulLoginFlowSite(
+      GURL("https://oauth.com/relogin"),
+      {
+          GURL("https://oauth.com/login?username=123&password=123"),
+          GURL("https://oauth.com/relogin"),
+      }));
+  EXPECT_FALSE(oauth_login_detector_->GetSuccessfulLoginFlowSite(
+      GURL("https://oauth.com/relogin"),
+      {GURL("https://oauth.com/login?username=123&password=123"),
+       GURL("https://oauth.com/loginsuccess"),
+       GURL("https://foo.com/redirect?code=secret")}));
+  EXPECT_FALSE(oauth_login_detector_->GetPopUpLoginFlowSite());
+}
+
+// Test that too many redirects are allowed within the same navigation.
+TEST_F(OAuthLoginDetectorTest, RedirectNavigationsAfterOAuthStart) {
+  EXPECT_EQ(GURL("https://foo.com/"),
+            *oauth_login_detector_->GetSuccessfulLoginFlowSite(
                 GURL("https://foo.com/login.html"),
                 {GURL("https://oauth.com/authenticate?client_id=123"),
-       GURL("https://oauth.com/login"), GURL("https://oauth.com/login"),
-       GURL("https://oauth.com/login"), GURL("https://oauth.com/login"),
+                 GURL("https://oauth.com/login"),
+                 GURL("https://oauth.com/loginfailed"),
+                 GURL("https://oauth.com/relogin"),
+                 GURL("https://oauth.com/loginsuccess"),
                  GURL("https://foo.com/redirect?code=secret")}));
   EXPECT_FALSE(oauth_login_detector_->GetPopUpLoginFlowSite());
 }