Revert "crOS: Remove OAuth2LoginVerifier"

This reverts commit 2f70b4c.

Reason for revert: Broke login.ChangePassword tast test according to bisector.

Bug: b/272558325

Original change's description:
> crOS: Remove OAuth2LoginVerifier
>
> ChromeOS has a dedicated legacy class for minting cookies that
> essentially duplicates the work of `AccountReconcilor`. Since ChromeOS
> Account Manager was launched, this is no longer needed.
>
> Remove `OAuth2LoginVerifier` and make `OAuth2LoginManager` observe
> `AccountReconcilor` instead of attempting /MergeSession itself.
>
> This CL is essentially a reland of https://crrev.com/c/1674099. That CL
> had to be reverted because `AccountReconcilor`'s error callbacks were
> not deterministic. https://crrev.com/c/3998935 made
> `AccountReconcilor`'s error callbacks deterministic since M109.
>
> DD: http://doc/1fACI5GQgXvmlmEJj6aPedazXeDZAZPw9jH_E91WGe3w
>
> Test Plan:
>
> Automated tests:
>
> - browser_tests --gtest_filter="*OAuth2Test*"
> - browser_tests --gtest_filter="*MergeSessionTest*"
> - Tast tests
>
> Manual tests:
>
> Permutations of the following test matrix:
>
> - Existing / new users
> - 1 / 2 accounts in Account Manager
> - Device / Secondary Account in error
>
> Fixed: b/260076740
> Test: See above
> Change-Id: I317ec1de1f06c7d1f0a3e225309536463fefb879
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4306643
> Reviewed-by: Anastasiia N <anastasiian@chromium.org>
> Commit-Queue: Kush Sinha <sinhak@chromium.org>
> Reviewed-by: David Roger <droger@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1114614}

Change-Id: Id2e1e5c0adcdf01495f817d8d5dfe99b4162d6a6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4327900
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Kush Sinha <sinhak@chromium.org>
Owners-Override: Colin Kincaid <ckincaid@chromium.org>
Commit-Queue: Colin Kincaid <ckincaid@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1115813}

build/check_gn_headers_whitelist.txt

@@ -20,6 +20,7 @@ cc/layers/performance_properties.h
 chrome/browser/android/android_theme_resources.h
 chrome/browser/android/resource_id.h
 chrome/browser/ash/login/signin/oauth2_login_manager.h
+chrome/browser/ash/login/signin/oauth2_login_verifier.h
 chrome/browser/ash/login/signin/oauth2_token_fetcher.h
 chrome/browser/ash/profiles/profile_helper.h
 chrome/browser/ash/settings/cros_settings.h

chrome/browser/ash/BUILD.gn

@@ -1864,6 +1864,8 @@ source_set("ash") {
     "login/signin/oauth2_login_manager.h",
     "login/signin/oauth2_login_manager_factory.cc",
     "login/signin/oauth2_login_manager_factory.h",
+    "login/signin/oauth2_login_verifier.cc",
+    "login/signin/oauth2_login_verifier.h",
     "login/signin/oauth2_token_fetcher.cc",
     "login/signin/oauth2_token_fetcher.h",
     "login/signin/oauth2_token_initializer.cc",

chrome/browser/ash/login/session/user_session_manager.cc

@@ -1035,15 +1035,18 @@ void UserSessionManager::OnSessionRestoreStateChanged(
       IdentityManagerFactory::GetForProfile(user_profile);
   switch (state) {
     case OAuth2LoginManager::SESSION_RESTORE_DONE:
-      if (identity_manager) {
-        // SESSION_RESTORE_DONE state means that primary account has a valid
-        // token.
-        DCHECK(
-            !identity_manager->HasAccountWithRefreshTokenInPersistentErrorState(
-                identity_manager->GetPrimaryAccountInfo(ConsentLevel::kSignin)
-                    .account_id));
-      }
-      user_status = user_manager::User::OAUTH2_TOKEN_STATUS_VALID;
+      // Session restore done does not always mean valid token because the
+      // merge session operation could be skipped when the first account in
+      // Gaia cookies matches the primary account in TokenService. However
+      // the token could still be invalid in some edge cases. See
+      // http://crbug.com/760610
+      user_status =
+          (identity_manager &&
+           identity_manager->HasAccountWithRefreshTokenInPersistentErrorState(
+               identity_manager->GetPrimaryAccountInfo(ConsentLevel::kSignin)
+                   .account_id))
+              ? user_manager::User::OAUTH2_TOKEN_STATUS_INVALID
+              : user_manager::User::OAUTH2_TOKEN_STATUS_VALID;
       break;
     case OAuth2LoginManager::SESSION_RESTORE_FAILED:
       user_status = user_manager::User::OAUTH2_TOKEN_STATUS_INVALID;

chrome/browser/ash/login/session/user_session_manager.h

@@ -25,12 +25,12 @@
 #include "chrome/browser/ash/base/locale_util.h"
 #include "chrome/browser/ash/child_accounts/child_policy_observer.h"
 #include "chrome/browser/ash/hats/hats_notification_controller.h"
+#include "chrome/browser/ash/login/oobe_screen.h"
 #include "chrome/browser/ash/login/signin/oauth2_login_manager.h"
 #include "chrome/browser/ash/login/signin/token_handle_util.h"
 #include "chrome/browser/ash/net/secure_dns_manager.h"
 #include "chrome/browser/ash/release_notes/release_notes_notification.h"
 #include "chrome/browser/ash/web_applications/help_app/help_app_notification_controller.h"
-#include "chrome/browser/profiles/profile.h"
 #include "chromeos/ash/components/dbus/session_manager/session_manager_client.h"
 #include "chromeos/ash/components/login/auth/authenticator.h"
 #include "chromeos/ash/components/login/auth/public/user_context.h"

chrome/browser/ash/login/signin/merge_session_navigation_throttle.cc

@@ -7,7 +7,6 @@
 #include "base/time/time.h"
 #include "chrome/browser/ash/login/signin/merge_session_throttling_utils.h"
 #include "chrome/browser/ash/login/signin/oauth2_login_manager_factory.h"
-#include "chrome/browser/profiles/profile.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"

chrome/browser/ash/login/signin/merge_session_throttling_utils.cc

@@ -8,11 +8,11 @@
 
 #include "base/lazy_instance.h"
 #include "base/logging.h"
+#include "base/memory/singleton.h"
 #include "base/sequence_checker.h"
 #include "base/time/time.h"
 #include "chrome/browser/ash/login/signin/oauth2_login_manager.h"
 #include "chrome/browser/ash/login/signin/oauth2_login_manager_factory.h"
-#include "chrome/browser/profiles/profile.h"
 #include "chrome/common/url_constants.h"
 #include "components/google/core/common/google_util.h"
 #include "components/user_manager/user_manager.h"
@@ -22,7 +22,8 @@
 #include "services/network/public/cpp/network_connection_tracker.h"
 #include "url/gurl.h"
 
-namespace ash::merge_session_throttling_utils {
+namespace ash {
+namespace merge_session_throttling_utils {
 namespace {
 
 using ::content::BrowserThread;
@@ -33,7 +34,7 @@ const int64_t kMaxSessionRestoreTimeInSec = 60;
 // The set of blocked profiles.
 class ProfileSet : public std::set<Profile*> {
  public:
-  ProfileSet() = default;
+  ProfileSet() {}
 
   ProfileSet(const ProfileSet&) = delete;
   ProfileSet& operator=(const ProfileSet&) = delete;
@@ -209,4 +210,5 @@ bool IsSessionRestorePending(Profile* profile) {
   return pending_session_restore;
 }
 
-}  // namespace ash::merge_session_throttling_utils
+}  // namespace merge_session_throttling_utils
+}  // namespace ash

chrome/browser/ash/login/signin/oauth2_browsertest.cc

@@ -101,15 +101,6 @@ const char kTestIdTokenAdvancedProtectionDisabled[] =
     "dummy-header."
     "eyAic2VydmljZXMiOiBbXSB9"  // payload: { "services": [] }
     ".dummy-signature";
-constexpr char kGooglePageContent[] =
-    "<html><title>Hello!</title><script>alert('hello');</script>"
-    "<body>Hello Google!</body></html>";
-constexpr char kRandomPageContent[] =
-    "<html><title>SomthingElse</title><body>I am SomethingElse</body></html>";
-constexpr char kHelloPagePath[] = "/hello_google";
-constexpr char kRandomPagePath[] = "/non_google_page";
-constexpr char kMergeSessionPath[] = "/MergeSession";
-constexpr char kMultiLoginPath[] = "/oauth/multilogin";
 
 CoreAccountId PickAccountId(Profile* profile,
                             const std::string& gaia_id,
@@ -550,7 +541,7 @@ IN_PROC_BROWSER_TEST_F(OAuth2Test, PRE_PRE_PRE_MergeSession) {
 
 // MergeSession test is running merge session process for an existing profile
 // that was generated in PRE_PRE_PRE_MergeSession test. In this test, we
-// are not running cookie minting process since the /ListAccounts call confirms
+// are not running /MergeSession process since the /ListAccounts call confirms
 // that the session is not stale.
 IN_PROC_BROWSER_TEST_F(OAuth2Test, PRE_PRE_MergeSession) {
   SetupGaiaServerForUnexpiredAccount();
@@ -572,9 +563,9 @@ IN_PROC_BROWSER_TEST_F(OAuth2Test, PRE_MergeSession) {
   LoginAsExistingUser();
   CookieReader cookie_reader;
   cookie_reader.ReadCookies(GetProfile());
-  // These should be cookie values that we generated by calling the cookie
-  // minting endpoint, since /ListAccounts should have tell us that the initial
-  // session cookies are stale.
+  // These should be cookie values that we generated by calling /MergeSession,
+  // since /ListAccounts should have tell us that the initial session cookies
+  // are stale.
   EXPECT_EQ(cookie_reader.GetCookieValue("SID"), kTestSession2SIDCookie);
   EXPECT_EQ(cookie_reader.GetCookieValue("LSID"), kTestSession2LSIDCookie);
 }
@@ -717,6 +708,76 @@ IN_PROC_BROWSER_TEST_F(OAuth2Test,
           .is_under_advanced_protection);
 }
 
+// Sets up a new user with stored refresh token.
+IN_PROC_BROWSER_TEST_F(OAuth2Test, PRE_SetInvalidTokenStatus) {
+  StartNewUserSession(/*wait_for_merge=*/true,
+                      /*is_under_advanced_protection=*/false);
+}
+
+// Tests that an auth error marks invalid auth token status despite
+// OAuth2LoginManager thinks merge session is done successfully
+IN_PROC_BROWSER_TEST_F(OAuth2Test, SetInvalidTokenStatus) {
+  RequestDeferrer list_accounts_request_deferer;
+  AddRequestDeferer("/ListAccounts", &list_accounts_request_deferer);
+
+  SetupGaiaServerForUnexpiredAccount();
+  SimulateNetworkOnline();
+
+  // Signs in as the existing user created in pre test.
+  ExistingUserController* const controller =
+      ExistingUserController::current_controller();
+  UserContext user_context(
+      user_manager::USER_TYPE_REGULAR,
+      AccountId::FromUserEmailGaiaId(kTestEmail, kTestGaiaId));
+  user_context.SetKey(Key(kTestAccountPassword));
+  controller->Login(user_context, SigninSpecifics());
+
+  // Wait until /ListAccounts request happens so that an auth error can be
+  // generated after user profile is available but before merge session
+  // finishes.
+  list_accounts_request_deferer.WaitForRequestToStart();
+
+  // Make sure that merge session is not finished.
+  OAuth2LoginManager* const login_manager =
+      OAuth2LoginManagerFactory::GetInstance()->GetForProfile(GetProfile());
+  ASSERT_NE(OAuth2LoginManager::SESSION_RESTORE_DONE, login_manager->state());
+
+  CoreAccountId account_id =
+      PickAccountId(GetProfile(), kTestGaiaId, kTestEmail);
+  // Generate an auth error.
+  signin::SetInvalidRefreshTokenForAccount(
+      IdentityManagerFactory::GetInstance()->GetForProfile(GetProfile()),
+      account_id);
+
+  // Let go /ListAccounts request.
+  list_accounts_request_deferer.UnblockRequest();
+
+  // Wait for the session merge to finish with success.
+  WaitForMergeSessionCompletion(OAuth2LoginManager::SESSION_RESTORE_DONE);
+
+  base::RepeatingCallback<bool(const GoogleServiceAuthError&)> predicate =
+      base::BindRepeating([](const GoogleServiceAuthError& error) {
+        return error.state() ==
+               GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS;
+      });
+  signin::WaitForErrorStateOfRefreshTokenUpdatedForAccount(
+      identity_manager(), account_id, predicate);
+
+  // User oauth2 token status should be marked as invalid because of auth error
+  // and regardless of the merge session outcome.
+  EXPECT_EQ(GetOAuthStatusFromLocalState(kTestEmail),
+            user_manager::User::OAUTH2_TOKEN_STATUS_INVALID);
+}
+
+constexpr char kGooglePageContent[] =
+    "<html><title>Hello!</title><script>alert('hello');</script>"
+    "<body>Hello Google!</body></html>";
+constexpr char kRandomPageContent[] =
+    "<html><title>SomthingElse</title><body>I am SomethingElse</body></html>";
+constexpr char kHelloPagePath[] = "/hello_google";
+constexpr char kRandomPagePath[] = "/non_google_page";
+constexpr char kMergeSessionPath[] = "/MergeSession";
+
 // FakeGoogle serves content of http://www.google.com/hello_google page for
 // merge session tests.
 class FakeGoogle {
@@ -730,7 +791,7 @@ class FakeGoogle {
   FakeGoogle(const FakeGoogle&) = delete;
   FakeGoogle& operator=(const FakeGoogle&) = delete;
 
-  ~FakeGoogle() = default;
+  ~FakeGoogle() {}
 
   std::unique_ptr<HttpResponse> HandleRequest(const HttpRequest& request) {
     // The scheme and host of the URL is actually not important but required to
@@ -751,8 +812,7 @@ class FakeGoogle {
       http_response->set_code(net::HTTP_OK);
       http_response->set_content_type("text/html");
       http_response->set_content(kRandomPageContent);
-    } else if (hang_merge_session_ && (request_path == kMergeSessionPath ||
-                                       request_path == kMultiLoginPath)) {
+    } else if (hang_merge_session_ && request_path == kMergeSessionPath) {
       merge_session_event_.Signal();
       content::GetUIThreadTaskRunner({})->PostTask(
           FROM_HERE, base::BindOnce(&FakeGoogle::QuitMergeRunnerOnUIThread,
@@ -778,7 +838,7 @@ class FakeGoogle {
     run_loop_->Run();
   }
 
-  // Waits until we receive a request to serve the cookie minting endpoint.
+  // Waits until we receive a request to serve the /MergeSession page.
   void WaitForMergeSessionPageRequest() {
     // If we have already served the request, bail out.
     if (merge_session_event_.IsSignaled())
@@ -837,8 +897,7 @@ class MergeSessionTest : public OAuth2Test,
 
   void RegisterAdditionalRequestHandlers() override {
     OAuth2Test::RegisterAdditionalRequestHandlers();
-    AddRequestDeferer(kMergeSessionPath, &merge_session_deferer_);
-    AddRequestDeferer(kMultiLoginPath, &merge_session_deferer_);
+    AddRequestDeferer("/MergeSession", &merge_session_deferer_);
 
     embedded_test_server()->RegisterRequestHandler(base::BindRepeating(
         &FakeGoogle::HandleRequest, base::Unretained(&fake_google_)));
@@ -1096,12 +1155,12 @@ class MergeSessionTimeoutTest : public MergeSessionTest {
   void RegisterAdditionalRequestHandlers() override {
     OAuth2Test::RegisterAdditionalRequestHandlers();
 
-    // Do not defer cookie minting requests (like the base class does) because
+    // Do not defer /MergeSession requests (like the base class does) because
     // this test will intentionally hang that request to force a timeout.
     embedded_test_server()->RegisterRequestHandler(base::BindRepeating(
         &FakeGoogle::HandleRequest, base::Unretained(&fake_google_)));
-    // Hanging cookie minting requests is implemented in `fake_google_`, so
-    // register it with the GAIA test server as well.
+    // Hanging /MergeSession is implemented in `fake_google_`, so register it
+    // with the GAIA test server as well.
     fake_gaia_.gaia_server()->RegisterRequestHandler(base::BindRepeating(
         &FakeGoogle::HandleRequest, base::Unretained(&fake_google_)));
   }
@@ -1170,10 +1229,9 @@ IN_PROC_BROWSER_TEST_P(MergeSessionTimeoutTest, XHRMergeTimeout) {
     EXPECT_TRUE(fake_google_.IsPageRequested());
   }
 
-  // Because this test has hung the cookie minting response,
-  // `UserSessionManager` is still observing `OAuth2LoginManager` - which fails
-  // a DCHECK in `~OAuth2LoginManager()`. Manually change the state to avoid
-  // this.
+  // Because this test has hung the /MergeSession response the
+  // UserSessionManager is still observing the OAuth2LoginManager which fails
+  // a DCHECK in ~OAuth2LoginManager. Manually change the state to avoid this.
   SetSessionRestoreState(
       OAuth2LoginManager::SessionRestoreState::SESSION_RESTORE_FAILED);
 }