LoginDetection: Track OAuth requestor site based on the first navigation

Currently OAuthed site is detected at the last stage of login flow when
the redirect happens with the completion request parameters. However
this site at the final step may be wrong due to multiple sites using a
common site for handling the OAuth login.

Instead this CL detects this OAuth requestor site based on the first
navigation to the OAuth provider site.

This CL also refactors a bit to pass down the entire navigation
redirect chain to the detector.

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

chrome/browser/login_detection/login_detection_tab_helper.cc

@@ -13,6 +13,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "components/prefs/pref_service.h"
 #include "content/public/browser/browser_context.h"
+#include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
@@ -74,18 +75,23 @@ void LoginDetectionTabHelper::DidFinishNavigation(
   if (!url.is_valid() || !url.SchemeIsHTTPOrHTTPS())
     return;
 
+  GURL prev_navigation_url;
+  if (auto* prev_navigation =
+          web_contents()->GetController().GetEntryAtOffset(-1)) {
+    prev_navigation_url = prev_navigation->GetURL();
+  }
+
   // Check if OAuth login on the site happened now. This check should happen
   // first before other checks since this could be a repeated OAuth login and
   // the time of login will be updated.
-  for (const auto& redirect_url : navigation_handle->GetRedirectChain()) {
-    if (oauth_login_detector_.CheckSuccessfulLoginFlow(redirect_url)) {
+  if (auto signedin_site = oauth_login_detector_.GetSuccessfulLoginFlowSite(
+          prev_navigation_url, navigation_handle->GetRedirectChain())) {
     prefs::SaveSiteToOAuthSignedInList(GetPrefs(web_contents()),
-                                         redirect_url);
+                                       *signedin_site);
     RecordLoginDetectionMetrics(LoginDetectionType::kOauthFirstTimeLoginFlow,
                                 navigation_handle->GetNextPageUkmSourceId());
     return;
   }
-  }
 
   LoginDetectionKeyedService* login_detection_keyed_service =
       LoginDetectionKeyedServiceFactory::GetForProfile(

chrome/browser/login_detection/login_detection_tab_helper_unittest.cc

@@ -65,6 +65,10 @@ TEST_F(LoginDetectionTabHelperTest, NoLogin) {
 }
 
 TEST_F(LoginDetectionTabHelperTest, SimpleOAuthLogin) {
+  NavigateAndCommit(GURL("https://foo.com/page.html"));
+  VerifyLoginDetectionTypeMetrics(LoginDetectionType::kNoLogin);
+  ResetMetricsTesters();
+
   // OAuth login start
   NavigateAndCommit(GURL("https://oauth.com/authenticate?client_id=123"));
   VerifyLoginDetectionTypeMetrics(LoginDetectionType::kNoLogin);
@@ -81,6 +85,10 @@ TEST_F(LoginDetectionTabHelperTest, SimpleOAuthLogin) {
 }
 
 TEST_F(LoginDetectionTabHelperTest, NavigationToOAuthLoggedInSite) {
+  NavigateAndCommit(GURL("https://foo.com/page.html"));
+  VerifyLoginDetectionTypeMetrics(LoginDetectionType::kNoLogin);
+  ResetMetricsTesters();
+
   // Trigger OAuth login so that the site is saved in prefs.
   NavigateAndCommit(GURL("https://oauth.com/authenticate?client_id=123"));
   NavigateAndCommit(GURL("https://foo.com/redirect?code=secret"));
@@ -97,6 +105,10 @@ TEST_F(LoginDetectionTabHelperTest, NavigationToOAuthLoggedInSite) {
 }
 
 TEST_F(LoginDetectionTabHelperTest, OAuthLoginViaRedirect) {
+  NavigateAndCommit(GURL("https://foo.com/page.html"));
+  VerifyLoginDetectionTypeMetrics(LoginDetectionType::kNoLogin);
+  ResetMetricsTesters();
+
   // OAuth login start and complete via redirects.
   auto simulator = content::NavigationSimulator::CreateBrowserInitiated(
       GURL("https://foo.com/oauth_signin"), web_contents());
@@ -116,32 +128,39 @@ TEST_F(LoginDetectionTabHelperTest, OAuthLoginViaRedirect) {
   VerifyLoginDetectionTypeMetrics(LoginDetectionType::kOauthLogin);
 }
 
-// Test that OAuth login is not detected when there are intermediate navigations
-// to other sites.
+// Test that OAuth login is still detected when there are intermediate
+// navigations to other sites.
 TEST_F(LoginDetectionTabHelperTest, InvalidOAuthLogins) {
+  NavigateAndCommit(GURL("https://foo.com/page.html"));
+  VerifyLoginDetectionTypeMetrics(LoginDetectionType::kNoLogin);
+  ResetMetricsTesters();
+
   // OAuth login start
   NavigateAndCommit(GURL("https://oauth.com/authenticate?client_id=123"));
   VerifyLoginDetectionTypeMetrics(LoginDetectionType::kNoLogin);
 
-  // Invalid intermediate navigation
+  // Intermediate navigation just ignored
   ResetMetricsTesters();
   NavigateAndCommit(GURL("https://bar.com/page.html"));
   VerifyLoginDetectionTypeMetrics(LoginDetectionType::kNoLogin);
 
-  // OAuth login complete will not be detected
+  // OAuth login complete will be detected
   ResetMetricsTesters();
   NavigateAndCommit(GURL("https://foo.com/redirect?code=secret"));
-  VerifyLoginDetectionTypeMetrics(LoginDetectionType::kNoLogin);
+  VerifyLoginDetectionTypeMetrics(LoginDetectionType::kOauthFirstTimeLoginFlow);
 
-  // Subsequent navigations is also no login.
   ResetMetricsTesters();
   NavigateAndCommit(GURL("https://images.foo.com/page.html"));
-  VerifyLoginDetectionTypeMetrics(LoginDetectionType::kNoLogin);
+  VerifyLoginDetectionTypeMetrics(LoginDetectionType::kOauthLogin);
 }
 
-// Test that OAuth login is not detected when there are intermediate redirect
+// Test that OAuth login is still detected when there are intermediate redirect
 // navigations to other sites.
 TEST_F(LoginDetectionTabHelperTest, InvalidOAuthLoginsWithRedirect) {
+  NavigateAndCommit(GURL("https://foo.com/page.html"));
+  VerifyLoginDetectionTypeMetrics(LoginDetectionType::kNoLogin);
+  ResetMetricsTesters();
+
   // OAuth login start and complete via redirects.
   auto simulator = content::NavigationSimulator::CreateBrowserInitiated(
       GURL("https://foo.com/oauth_signin"), web_contents());
@@ -154,7 +173,7 @@ TEST_F(LoginDetectionTabHelperTest, InvalidOAuthLoginsWithRedirect) {
   simulator->Redirect(GURL("https://foo.com/redirect?code=secret"));
   simulator->Commit();
 
-  VerifyLoginDetectionTypeMetrics(LoginDetectionType::kNoLogin);
+  VerifyLoginDetectionTypeMetrics(LoginDetectionType::kOauthFirstTimeLoginFlow);
 }
 
 class LoginDetectionTabHelperTestWithFieldTrialLoggedInList

chrome/browser/login_detection/oauth_login_detector.cc

@@ -27,8 +27,10 @@ bool DoAllQueryParamsExist(const std::set<std::string>& request_params,
 }  // namespace
 
 OAuthLoginDetector::OAuthLoginFlowInfo::OAuthLoginFlowInfo(
-    const std::string& start_site)
-    : start_site(start_site) {}
+    const GURL& oauth_provider_site,
+    const GURL& oauth_requestor_site)
+    : oauth_provider_site(oauth_provider_site),
+      oauth_requestor_site(oauth_requestor_site) {}
 
 OAuthLoginDetector::OAuthLoginFlowInfo::OAuthLoginFlowInfo(
     const OAuthLoginFlowInfo&) = default;
@@ -43,20 +45,37 @@ OAuthLoginDetector::OAuthLoginDetector()
 
 OAuthLoginDetector::~OAuthLoginDetector() = default;
 
-bool OAuthLoginDetector::CheckSuccessfulLoginFlow(const GURL& navigation_url) {
+base::Optional<GURL> OAuthLoginDetector::GetSuccessfulLoginFlowSite(
+    const GURL& prev_navigation_url,
+    const std::vector<GURL>& redirect_chain) {
+  for (const auto& navigation_url : redirect_chain) {
     // Allow login flows to be detected only on HTTPS pages.
     if (!navigation_url.SchemeIs(url::kHttpsScheme)) {
       login_flow_info_ = base::nullopt;
-    return false;
+      return base::nullopt;
     }
 
-  if (!login_flow_info_) {
-    if (DoAllQueryParamsExist(login_flow_start_query_params_, navigation_url)) {
-      // Login flow start was detected.
-      login_flow_info_ = OAuthLoginFlowInfo(GetSiteNameForURL(navigation_url));
+    // Check for OAuth login completion.
+    if (login_flow_info_ && CheckSuccessfulLoginCompletion(navigation_url)) {
+      auto oauth_requestor_site = login_flow_info_->oauth_requestor_site;
+      login_flow_info_ = base::nullopt;
+      return oauth_requestor_site;
+    }
+
+    // Check for start of login flow.
+    if (!login_flow_info_ && prev_navigation_url.is_valid() &&
+        prev_navigation_url.SchemeIsHTTPOrHTTPS() &&
+        DoAllQueryParamsExist(login_flow_start_query_params_, navigation_url)) {
+      login_flow_info_ =
+          OAuthLoginFlowInfo(navigation_url, prev_navigation_url.GetOrigin());
     }
-    return false;
   }
+  return base::nullopt;
+}
+
+bool OAuthLoginDetector::CheckSuccessfulLoginCompletion(
+    const GURL& navigation_url) {
+  DCHECK(login_flow_info_.has_value());
 
   // Login flow had started previously, check if it completes within the
   // navigation limit.
@@ -69,28 +88,20 @@ bool OAuthLoginDetector::CheckSuccessfulLoginFlow(const GURL& navigation_url) {
   }
   std::string navigation_site = GetSiteNameForURL(navigation_url);
 
-  // Check the navigation only happens for the start site or completion site.
-  if (login_flow_info_->start_site != navigation_site &&
-      login_flow_info_->completion_site &&
-      *login_flow_info_->completion_site != navigation_site) {
-    login_flow_info_ = base::nullopt;
+  // Check the OAuth login completion that returns the authorzation code and
+  // token to the OAuth requestor site, does not happen for the OAuth provider
+  // site.
+  if (GetSiteNameForURL(login_flow_info_->oauth_provider_site) ==
+      navigation_site) {
+    login_flow_info_->count_navigations_since_login_flow_start++;
     return false;
   }
-  // Update any navigation to a non start site as completion site.
-  if (login_flow_info_->start_site != navigation_site &&
-      !login_flow_info_->completion_site) {
-    login_flow_info_->completion_site = navigation_site;
-  }
 
-  DCHECK(login_flow_info_->start_site == navigation_site ||
-         *login_flow_info_->completion_site == navigation_site);
   if (!DoAllQueryParamsExist(login_flow_complete_query_params_,
                              navigation_url)) {
     login_flow_info_->count_navigations_since_login_flow_start++;
     return false;
   }
-  // Successful login flow completion was detected.
-  login_flow_info_ = base::nullopt;
   return true;
 }
 

chrome/browser/login_detection/oauth_login_detector.h

@@ -13,7 +13,18 @@
 
 namespace login_detection {
 
-// Detects successful OAuth login flow based on heuristics.
+// Detects successful OAuth login flow based on heuristics that observe certain
+// request parameters to determine start and completion of OAuth login flow.
+//
+// Initially, there is a navigation to the OAuth requestor site, and that
+// triggers navigation to the OAuth provider site with certain request
+// parameters to identify the OAuth start.
+//
+// The OAuth requestor authenticates the user and returns the authorization code
+// or token to the requestor. This redirect request has certain reqest
+// parameters to identify as OAuth completion. Note that this requestor site can
+// be quite different from the OAuth requestor site seen in the initial
+// navigation.
 class OAuthLoginDetector {
  public:
   OAuthLoginDetector();
@@ -22,13 +33,19 @@ class OAuthLoginDetector {
   OAuthLoginDetector(const OAuthLoginDetector&) = delete;
   OAuthLoginDetector& operator=(const OAuthLoginDetector&) = delete;
 
-  // Processes the navigation to |navigation_url| and returns whether a
-  // successful OAuth login flow started and completed.
-  bool CheckSuccessfulLoginFlow(const GURL& navigation_url);
+  // Processes the navigation |redirect_chain| and returns the site that started
+  // the OAuth login flow and completed. base::nullopt is returned when there is
+  // no login flow detected or it has not yet completed. |prev_navigation_url|
+  // is the URL of the previous navigation on this detector, and can be invalid
+  // when no previous navigation happened.
+  base::Optional<GURL> GetSuccessfulLoginFlowSite(
+      const GURL& prev_navigation_url,
+      const std::vector<GURL>& redirect_chain);
 
  private:
   struct OAuthLoginFlowInfo {
-    explicit OAuthLoginFlowInfo(const std::string& start_site);
+    OAuthLoginFlowInfo(const GURL& oauth_provider_site,
+                       const GURL& oauth_requestor_site);
     OAuthLoginFlowInfo(const OAuthLoginFlowInfo&);
     ~OAuthLoginFlowInfo();
 
@@ -37,14 +54,18 @@ class OAuthLoginDetector {
     // counted as well.
     size_t count_navigations_since_login_flow_start = 1;
 
-    // Site that started the login flow. This is the OAuth provider site.
-    std::string start_site;
+    // The OAuth provider site.
+    GURL oauth_provider_site;
 
-    // Site that completed the login flow. This is the OAuth requestor site.
-    // Empty when OAuth flow started until a completion site is detected.
-    base::Optional<std::string> completion_site;
+    // The the OAuth requestor site that initiated the login flow.
+    GURL oauth_requestor_site;
   };
 
+  // Returns whether successful OAuth login completion was detected. Clears the
+  // login flow state when completion is detected or when completion is no
+  // longer possible.
+  bool CheckSuccessfulLoginCompletion(const GURL& navigation_url);
+
   // Set of query parameters that should be found in the navigation URL to
   // recognize the navigation as a start and completion of OAuth login flow.
   // These are populated from field trail.

chrome/browser/login_detection/oauth_login_detector_unittest.cc

@@ -24,17 +24,20 @@ class OAuthLoginDetectorTest : public testing::Test {
 };
 
 TEST_F(OAuthLoginDetectorTest, SimpleOAuthLogin) {
-  EXPECT_FALSE(oauth_login_detector_->CheckSuccessfulLoginFlow(
-      GURL("https://oauth.com/authenticate?client_id=123")));
-  EXPECT_TRUE(oauth_login_detector_->CheckSuccessfulLoginFlow(
-      GURL("https://foo.com/redirect?code=secret")));
+  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://foo.com/redirect?code=secret")}));
 }
 
 TEST_F(OAuthLoginDetectorTest, OAuthLoginWithMultipleQueryParams) {
-  EXPECT_FALSE(oauth_login_detector_->CheckSuccessfulLoginFlow(GURL(
-      "https://oauth.com/authenticate?client_id=123&redirect_uri=foo.com")));
-  EXPECT_TRUE(oauth_login_detector_->CheckSuccessfulLoginFlow(
-      GURL("https://foo.com/redirect?scope=userinfo&code=secret")));
+  EXPECT_EQ(GURL("https://foo.com/"),
+            *oauth_login_detector_->GetSuccessfulLoginFlowSite(
+                GURL("https://foo.com/login.html"),
+                {GURL("https://oauth.com/"
+                      "authenticate?client_id=123&redirect_uri=foo.com"),
+                 GURL("https://foo.com/redirect?scope=userinfo&code=secret")}));
 }
 
 TEST(OAuthLoginDetectorTestWithParams, OAuthLoginRequiringMultipleQueryParams) {
@@ -45,10 +48,13 @@ TEST(OAuthLoginDetectorTestWithParams, OAuthLoginRequiringMultipleQueryParams) {
        {"oauth_login_complete_request_params", "scope,code"}});
   OAuthLoginDetector detector;
 
-  EXPECT_FALSE(detector.CheckSuccessfulLoginFlow(GURL(
-      "https://oauth.com/authenticate?client_id=123&redirect_uri=foo.com")));
-  EXPECT_TRUE(detector.CheckSuccessfulLoginFlow(
-      GURL("https://foo.com/redirect?scope=userinfo&code=secret")));
+  EXPECT_EQ(GURL("https://foo.com/"),
+            *detector.GetSuccessfulLoginFlowSite(
+                GURL("https://foo.com/login.html"),
+                {GURL("https://oauth.com/"
+                      "authenticate?client_id=123&redirect_uri=foo.com"),
+
+                 GURL("https://foo.com/redirect?scope=userinfo&code=secret")}));
 }
 
 TEST(OAuthLoginDetectorTestWithParams, OAuthLoginMissingMultipleQueryParams) {
@@ -59,56 +65,69 @@ TEST(OAuthLoginDetectorTestWithParams, OAuthLoginMissingMultipleQueryParams) {
        {"oauth_login_complete_request_params", "scope,code"}});
   OAuthLoginDetector detector;
 
-  EXPECT_FALSE(detector.CheckSuccessfulLoginFlow(
-      GURL("https://oauth.com/authenticate?client_id=123")));
-  EXPECT_FALSE(detector.CheckSuccessfulLoginFlow(
-      GURL("https://foo.com/redirect?code=secret")));
+  EXPECT_FALSE(detector.GetSuccessfulLoginFlowSite(
+      GURL("https://foo.com/login.html"),
+      {GURL("https://oauth.com/authenticate?client_id=123"),
+       GURL("https://foo.com/redirect?code=secret")}));
 }
 
 TEST_F(OAuthLoginDetectorTest, LoginNotDetectedForHTTP) {
-  EXPECT_FALSE(oauth_login_detector_->CheckSuccessfulLoginFlow(
-      GURL("http://oauth.com/authenticate?client_id=123")));
-  EXPECT_FALSE(oauth_login_detector_->CheckSuccessfulLoginFlow(
-      GURL("http://foo.com/redirect?code=secret")));
+  EXPECT_FALSE(oauth_login_detector_->GetSuccessfulLoginFlowSite(
+      GURL("https://foo.com/login.html"),
+      {GURL("http://oauth.com/authenticate?client_id=123"),
+       GURL("http://foo.com/redirect?code=secret")}));
 }
 
 // Test that small number of intermediate navigations within OAuth start and
 // completion are allowed.
 TEST_F(OAuthLoginDetectorTest, IntermediateNavigationsAfterOAuthStart) {
-  EXPECT_FALSE(oauth_login_detector_->CheckSuccessfulLoginFlow(
-      GURL("https://oauth.com/authenticate?client_id=123")));
-  EXPECT_FALSE(oauth_login_detector_->CheckSuccessfulLoginFlow(
-      GURL("https://oauth.com/login")));
-  EXPECT_TRUE(oauth_login_detector_->CheckSuccessfulLoginFlow(
-      GURL("https://foo.com/redirect?code=secret")));
+  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://foo.com/redirect?code=secret")}));
 }
 
 // Test that OAuth login is not allowed when too many intermediate navigations
 // happen within OAuth start and completion.
 TEST_F(OAuthLoginDetectorTest, TooManyIntermediateNavigationsAfterOAuthStart) {
-  EXPECT_FALSE(oauth_login_detector_->CheckSuccessfulLoginFlow(
-      GURL("https://oauth.com/authenticate?client_id=123")));
-  EXPECT_FALSE(oauth_login_detector_->CheckSuccessfulLoginFlow(
-      GURL("https://oauth.com/login")));
-  EXPECT_FALSE(oauth_login_detector_->CheckSuccessfulLoginFlow(
-      GURL("https://oauth.com/login")));
-  EXPECT_FALSE(oauth_login_detector_->CheckSuccessfulLoginFlow(
-      GURL("https://oauth.com/login")));
-  EXPECT_FALSE(oauth_login_detector_->CheckSuccessfulLoginFlow(
-      GURL("https://oauth.com/login")));
-  EXPECT_FALSE(oauth_login_detector_->CheckSuccessfulLoginFlow(
-      GURL("https://foo.com/redirect?code=secret")));
+  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"), GURL("https://oauth.com/login"),
+       GURL("https://oauth.com/login"), GURL("https://oauth.com/login"),
+       GURL("https://foo.com/redirect?code=secret")}));
 }
 
-// Test that OAuth login is not detected when there are intermediate navigations
-// to other sites.
+// Test that OAuth login is detected when there are intermediate navigations to
+// other sites.
 TEST_F(OAuthLoginDetectorTest, IntermediateNavigationsToOtherSites) {
-  EXPECT_FALSE(oauth_login_detector_->CheckSuccessfulLoginFlow(
-      GURL("https://oauth.com/authenticate?client_id=123")));
-  EXPECT_FALSE(oauth_login_detector_->CheckSuccessfulLoginFlow(
-      GURL("https://bar.com/page.html")));
-  EXPECT_FALSE(oauth_login_detector_->CheckSuccessfulLoginFlow(
-      GURL("https://foo.com/redirect?code=secret")));
+  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://bar.com/page.html"),
+                 GURL("https://foo.com/redirect?code=secret")}));
+}
+
+// Test that OAuth requestor site is correctly detected when the site that
+// performs the OAuth completion step is different.
+TEST_F(OAuthLoginDetectorTest, DifferentOAuthCompletionSite) {
+  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://fooauth.com/redirect?code=secret")}));
+}
+
+// Test that OAuth completion navigation does not happen for the OAuth provider
+// site.
+TEST_F(OAuthLoginDetectorTest, OAuthCompletionOnOAuthProviderSite) {
+  EXPECT_FALSE(oauth_login_detector_->GetSuccessfulLoginFlowSite(
+      GURL("https://foo.com/login.html"),
+      {GURL("https://oauth.com/authenticate?client_id=123"),
+       GURL("https://oauth.com/redirect?code=secret")}));
 }
 
 }  // namespace login_detection