Donot refresh login UI if auth enforcement was already performed on

that particular user sid.

Note: Tested the changes locally on a VM instance. It works great !

Bug: 1045080
Change-Id: I3e05cbd8d6448a601109787f3de2a9a36236709e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2135188
Commit-Queue: Rakesh Soma <rakeshsoma@google.com>
Reviewed-by: Yusuf Sengul <yusufsn@google.com>
Cr-Commit-Position: refs/heads/master@{#756361}

chrome/credential_provider/gaiacp/associated_user_validator.cc

@@ -603,25 +603,6 @@ bool AssociatedUserValidator::IsTokenHandleValidForUser(
   return validity_it->second->is_valid;
 }
 
-bool AssociatedUserValidator::IsAuthEnforcedOnAssociatedUsers() {
-  std::map<base::string16, UserTokenHandleInfo> sids_to_handle_info;
-  HRESULT hr = GetUserTokenHandles(&sids_to_handle_info);
-  if (FAILED(hr)) {
-    LOGFN(ERROR) << "GetUserTokenHandles hr=" << putHR(hr);
-    return hr;
-  }
-
-  for (const auto& sid_to_association : sids_to_handle_info) {
-    const base::string16& sid = sid_to_association.first;
-    // Return true even if one of the associated user sid
-    // has an auth enforced.
-    if (IsAuthEnforcedForUser(sid)) {
-      return true;
-    }
-  }
-  return false;
-}
-
 void AssociatedUserValidator::BlockDenyAccessUpdate() {
   base::AutoLock locker(validator_lock_);
   ++block_deny_access_update_;

chrome/credential_provider/gaiacp/associated_user_validator_unittests.cc

@@ -144,7 +144,6 @@ TEST_F(AssociatedUserValidatorTest, CleanupStaleUsers) {
   EXPECT_FALSE(validator.IsAuthEnforcedForUser(OLE2CW(sid_bad)));
   EXPECT_FALSE(validator.IsAuthEnforcedForUser(OLE2CW(sid_no_gaia_id)));
   EXPECT_TRUE(validator.IsAuthEnforcedForUser(OLE2CW(sid_no_token_handle)));
-  EXPECT_TRUE(validator.IsAuthEnforcedOnAssociatedUsers());
 
   // Expect deleted user and user with no gaia id to be deleted.
   EXPECT_NE(ERROR_SUCCESS, key.OpenKey(OLE2CW(sid_bad), KEY_READ));
@@ -165,7 +164,6 @@ TEST_F(AssociatedUserValidatorTest, NoTokenHandles) {
   // If there is no associated user then all token handles are valid.
   EXPECT_FALSE(
       validator.IsAuthEnforcedForUser(GetNewSidString(fake_os_user_manager())));
-  EXPECT_FALSE(validator.IsAuthEnforcedOnAssociatedUsers());
   EXPECT_EQ(0u, fake_http_url_fetcher_factory()->requests_created());
 }
 
@@ -187,7 +185,6 @@ TEST_F(AssociatedUserValidatorTest, ValidTokenHandle) {
   validator.StartRefreshingTokenHandleValidity();
 
   EXPECT_FALSE(validator.IsAuthEnforcedForUser(OLE2W(sid)));
-  EXPECT_FALSE(validator.IsAuthEnforcedOnAssociatedUsers());
   EXPECT_EQ(1u, fake_http_url_fetcher_factory()->requests_created());
 }
 
@@ -207,7 +204,6 @@ TEST_F(AssociatedUserValidatorTest, InvalidTokenHandle) {
   validator.StartRefreshingTokenHandleValidity();
 
   EXPECT_TRUE(validator.IsAuthEnforcedForUser(OLE2W(sid)));
-  EXPECT_TRUE(validator.IsAuthEnforcedOnAssociatedUsers());
   EXPECT_EQ(1u, fake_http_url_fetcher_factory()->requests_created());
 }
 
@@ -223,7 +219,6 @@ TEST_F(AssociatedUserValidatorTest, InvalidTokenHandleNoInternet) {
 
   validator.StartRefreshingTokenHandleValidity();
   EXPECT_FALSE(validator.IsAuthEnforcedForUser(OLE2W(sid)));
-  EXPECT_FALSE(validator.IsAuthEnforcedOnAssociatedUsers());
   EXPECT_EQ(0u, fake_http_url_fetcher_factory()->requests_created());
 }
 
@@ -244,7 +239,6 @@ TEST_F(AssociatedUserValidatorTest, InvalidTokenHandleTimeout) {
   validator.StartRefreshingTokenHandleValidity();
 
   EXPECT_FALSE(validator.IsAuthEnforcedForUser(OLE2W(sid)));
-  EXPECT_FALSE(validator.IsAuthEnforcedOnAssociatedUsers());
   EXPECT_EQ(1u, fake_http_url_fetcher_factory()->requests_created());
 
   http_fetcher_event.Signal();
@@ -269,7 +263,6 @@ TEST_F(AssociatedUserValidatorTest, TokenHandleValidityStillFresh) {
 
   EXPECT_FALSE(validator.IsAuthEnforcedForUser(OLE2W(sid)));
   EXPECT_FALSE(validator.IsAuthEnforcedForUser(OLE2W(sid)));
-  EXPECT_FALSE(validator.IsAuthEnforcedOnAssociatedUsers());
   EXPECT_EQ(1u, fake_http_url_fetcher_factory()->requests_created());
 }
 
@@ -343,7 +336,6 @@ TEST_F(AssociatedUserValidatorTest,
   EXPECT_TRUE(validator.IsAuthEnforcedForUser(OLE2W(sid)));
   EXPECT_TRUE(validator.DenySigninForUsersWithInvalidTokenHandles(CPUS_LOGON,
                                                                   reauth_sids));
-  EXPECT_TRUE(validator.IsAuthEnforcedOnAssociatedUsers());
 }
 
 // Donot deny user access even when the gaia handle is invalidated for a
@@ -372,7 +364,6 @@ TEST_F(AssociatedUserValidatorTest,
   EXPECT_TRUE(validator.IsAuthEnforcedForUser(OLE2W(sid)));
   EXPECT_FALSE(validator.DenySigninForUsersWithInvalidTokenHandles(
       CPUS_LOGON, reauth_sids));
-  EXPECT_TRUE(validator.IsAuthEnforcedOnAssociatedUsers());
 }
 
 // Clear the UserProperty from registry for those sids which doesn't
@@ -564,7 +555,6 @@ TEST_P(AssociatedUserValidatorUserAccessBlockingTest, BlockUserAccessAsNeeded) {
   EXPECT_EQ(should_user_be_blocked,
             validator.IsUserAccessBlockedForTesting(OLE2W(sid)));
   EXPECT_EQ(is_get_auth_enforced, validator.IsAuthEnforcedForUser(OLE2W(sid)));
-  EXPECT_EQ(is_get_auth_enforced, validator.IsAuthEnforcedOnAssociatedUsers());
 
   // Unlock the user.
   validator.AllowSigninForUsersWithInvalidTokenHandles();
@@ -616,7 +606,6 @@ TEST_F(AssociatedUserValidatorTest, ValidTokenHandle_Refresh) {
   validator.StartRefreshingTokenHandleValidity();
 
   EXPECT_FALSE(validator.IsAuthEnforcedForUser(OLE2W(sid)));
-  EXPECT_FALSE(validator.IsAuthEnforcedOnAssociatedUsers());
 
   // Make the next token fetch result invalid.
   fake_http_url_fetcher_factory()->SetFakeResponse(
@@ -626,7 +615,6 @@ TEST_F(AssociatedUserValidatorTest, ValidTokenHandle_Refresh) {
   // If the lifetime of the validity has not expired, even if the token is
   // invalid, no new fetch will be performed yet.
   EXPECT_FALSE(validator.IsAuthEnforcedForUser(OLE2W(sid)));
-  EXPECT_FALSE(validator.IsAuthEnforcedOnAssociatedUsers());
   EXPECT_EQ(1u, fake_http_url_fetcher_factory()->requests_created());
 
   // Advance the time so that a new fetch will be done and retrieve the
@@ -635,7 +623,6 @@ TEST_F(AssociatedUserValidatorTest, ValidTokenHandle_Refresh) {
       AssociatedUserValidator::kTokenHandleValidityLifetime +
       base::TimeDelta::FromMilliseconds(1);
   EXPECT_TRUE(validator.IsAuthEnforcedForUser(OLE2W(sid)));
-  EXPECT_TRUE(validator.IsAuthEnforcedOnAssociatedUsers());
   EXPECT_EQ(2u, fake_http_url_fetcher_factory()->requests_created());
 }
 
@@ -664,7 +651,6 @@ TEST_F(AssociatedUserValidatorTest, InvalidTokenHandle_MissingPasswordLsaData) {
   validator.StartRefreshingTokenHandleValidity();
 
   EXPECT_TRUE(validator.IsAuthEnforcedForUser(OLE2W(sid)));
-  EXPECT_TRUE(validator.IsAuthEnforcedOnAssociatedUsers());
 }
 
 TEST_F(AssociatedUserValidatorTest, ValidTokenHandle_PresentPasswordLsaData) {
@@ -694,7 +680,6 @@ TEST_F(AssociatedUserValidatorTest, ValidTokenHandle_PresentPasswordLsaData) {
   validator.StartRefreshingTokenHandleValidity();
 
   EXPECT_FALSE(validator.IsAuthEnforcedForUser(OLE2W(sid)));
-  EXPECT_FALSE(validator.IsAuthEnforcedOnAssociatedUsers());
 }
 
 }  // namespace testing

chrome/credential_provider/gaiacp/gaia_credential_provider.cc

@@ -120,25 +120,29 @@ HRESULT CreateCredentialObject(
 class BackgroundTokenHandleUpdater {
  public:
   explicit BackgroundTokenHandleUpdater(
-      ICredentialUpdateEventsHandler* event_handler);
+      ICredentialUpdateEventsHandler* event_handler,
+      const std::vector<base::string16>* reauth_sids);
   ~BackgroundTokenHandleUpdater();
 
  private:
   static unsigned __stdcall PeriodicTokenHandleUpdate(void* param);
+  bool IsAuthEnforcedOnAssociatedUsers();
 
   // Raw pointer to the interface on CGaiaCredentialProvider that is used
   // to notify that token handle validity has changed. Any instance of this
   // class should be owned by the CGaiaCredentialProvider to ensure that
   // this pointer outlives the updater.
   ICredentialUpdateEventsHandler* event_handler_;
+  const std::vector<base::string16>* reauth_sids_;
 
   base::win::ScopedHandle token_update_thread_;
   base::WaitableEvent token_update_quit_event_;
 };
 
 BackgroundTokenHandleUpdater::BackgroundTokenHandleUpdater(
-    ICredentialUpdateEventsHandler* event_handler)
-    : event_handler_(event_handler) {
+    ICredentialUpdateEventsHandler* event_handler,
+    const std::vector<base::string16>* reauth_sids)
+    : event_handler_(event_handler), reauth_sids_(reauth_sids) {
   unsigned wait_thread_id;
   uintptr_t wait_thread =
       _beginthreadex(nullptr, 0, PeriodicTokenHandleUpdate,
@@ -158,6 +162,31 @@ BackgroundTokenHandleUpdater::~BackgroundTokenHandleUpdater() {
   }
 }
 
+bool BackgroundTokenHandleUpdater::IsAuthEnforcedOnAssociatedUsers() {
+  std::map<base::string16, UserTokenHandleInfo> sids_to_handle_info;
+  HRESULT hr = GetUserTokenHandles(&sids_to_handle_info);
+  if (FAILED(hr)) {
+    LOGFN(ERROR) << "GetUserTokenHandles hr=" << putHR(hr);
+    return hr;
+  }
+
+  for (const auto& sid_to_association : sids_to_handle_info) {
+    const base::string16& sid = sid_to_association.first;
+    // Checks if the login UI was already refreshed due to
+    // auth enforcements on this sid.
+    if (reauth_sids_ != nullptr &&
+        (std::find(reauth_sids_->begin(), reauth_sids_->end(), sid) !=
+         reauth_sids_->end()))
+      continue;
+
+    // Return true if the associated user sid has auth enforced.
+    if (AssociatedUserValidator::Get()->IsAuthEnforcedForUser(sid)) {
+      return true;
+    }
+  }
+  return false;
+}
+
 unsigned __stdcall BackgroundTokenHandleUpdater::PeriodicTokenHandleUpdate(
     void* param) {
   BackgroundTokenHandleUpdater* updater =
@@ -173,8 +202,7 @@ unsigned __stdcall BackgroundTokenHandleUpdater::PeriodicTokenHandleUpdate(
     if (hr != WAIT_TIMEOUT)
       break;
 
-    bool user_access_changed =
-        AssociatedUserValidator::Get()->IsAuthEnforcedOnAssociatedUsers();
+    bool user_access_changed = updater->IsAuthEnforcedOnAssociatedUsers();
     if (user_access_changed) {
       LOGFN(VERBOSE) << "A user token handle has been invalidated. Refreshing "
                         "credentials";
@@ -381,8 +409,8 @@ HRESULT CGaiaCredentialProvider::CreateReauthCredentials(
   }
 
   LOGFN(VERBOSE) << "count=" << count;
+  reauth_cred_sids_.clear();
 
-  std::vector<base::string16> reauth_cred_sids;
   for (DWORD i = 0; i < count; ++i) {
     Microsoft::WRL::ComPtr<ICredentialProviderUser> user;
     hr = users->GetAt(i, &user);
@@ -466,14 +494,14 @@ HRESULT CGaiaCredentialProvider::CreateReauthCredentials(
 
     // Add SID to the vector to keep track of all the users that have a reauth
     // credential created.
-    reauth_cred_sids.push_back(sid);
+    reauth_cred_sids_.push_back(sid);
 
     LOGFN(VERBOSE) << "Reauth SID : " << sid;
   }
 
   // Deny sign in access for users that have a reauth credential added to them.
   AssociatedUserValidator::Get()->DenySigninForUsersWithInvalidTokenHandles(
-      cpus_, reauth_cred_sids);
+      cpus_, reauth_cred_sids_);
 
   return S_OK;
 }
@@ -702,8 +730,8 @@ HRESULT CGaiaCredentialProvider::Advise(ICredentialProviderEvents* pcpe,
   advise_context_ = context;
 
   if (AssociatedUserValidator::Get()->IsUserAccessBlockingEnforced(cpus_)) {
-    token_handle_updater_ =
-        std::make_unique<BackgroundTokenHandleUpdater>(this);
+    token_handle_updater_ = std::make_unique<BackgroundTokenHandleUpdater>(
+        this, &reauth_cred_sids_);
   }
 
   return S_OK;

chrome/credential_provider/gaiacp/gaia_credential_provider.h

@@ -259,6 +259,7 @@ class ATL_NO_VTABLE CGaiaCredentialProvider
   CredentialCreatorFn anonymous_cred_creator_ = nullptr;
   CredentialCreatorFn other_user_cred_creator_ = nullptr;
   CredentialCreatorFn reauth_cred_creator_ = nullptr;
+  std::vector<base::string16> reauth_cred_sids_;
 };
 
 // OBJECT_ENTRY_AUTO() contains an extra semicolon.