Do not clear UserProperties if either gaia id or email is available in the

GCPW registry entries.

Change-Id: Ib218c1553507468f93dfb9476c9b22d12ccd038a
Bug: 1044781
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2011295
Commit-Queue: Tien Mai <tienmai@chromium.org>
Reviewed-by: Tien Mai <tienmai@chromium.org>
Auto-Submit: Rakesh Soma <rakeshsoma@google.com>
Cr-Commit-Position: refs/heads/master@{#735151}

chrome/credential_provider/gaiacp/associated_user_validator.cc

@@ -252,7 +252,10 @@ HRESULT AssociatedUserValidator::UpdateAssociatedSids(
   for (const auto& sid_to_association : sids_to_handle_info) {
     const base::string16& sid = sid_to_association.first;
     const UserTokenHandleInfo& info = sid_to_association.second;
-    if (info.gaia_id.empty()) {
+
+    // If both gaia id and email address are empty. Then remove the
+    // User Properties as it is invalid.
+    if (info.gaia_id.empty() && info.email_address.empty()) {
       users_to_delete.insert(sid_to_association.first);
       continue;
     }
@@ -416,8 +419,9 @@ void AssociatedUserValidator::CheckTokenHandleValidity(
       continue;
     }
 
-    // User exists, has a gaia id, but no token handle. Consider this an invalid
-    // token handle and the user needs to sign in with Gaia to get a new one.
+    // User exists, has a gaia id or email address, but no token handle.
+    // Consider this an invalid token handle and the user needs to sign in
+    // with Gaia to get a new one.
     if (it->second.empty()) {
       user_to_token_handle_info_[it->first] =
           std::make_unique<TokenHandleInfo>(base::string16());

chrome/credential_provider/gaiacp/associated_user_validator_unittests.cc

@@ -106,11 +106,11 @@ TEST_F(AssociatedUserValidatorTest, CleanupStaleUsers) {
   CComBSTR sid_bad;
   CreateDeletedGCPWUser(&sid_bad);
 
-  // Simulate a user created by GCPW that has no gaia id.
+  // Simulate a user created by GCPW that has no gaia id and email.
   CComBSTR sid_no_gaia_id;
   ASSERT_EQ(S_OK, fake_os_user_manager()->CreateTestOSUser(
                       L"username2", L"password", L"Full Name", L"Comment", L"",
-                      L"foo2@gmail.com", &sid_no_gaia_id));
+                      L"", &sid_no_gaia_id));
 
   // Simulate a user created by GCPW that has a gaia id, but no token handle
   // set.
@@ -369,6 +369,51 @@ TEST_F(AssociatedUserValidatorTest,
   EXPECT_TRUE(validator.IsAuthEnforcedOnAssociatedUsers());
 }
 
+// Clear the UserProperty from registry for those sids which doesn't
+// have either gaia id or email association available.
+class UpdateAssociatedSidsTest
+    : public AssociatedUserValidatorTest,
+      public ::testing::WithParamInterface<std::tuple<bool, bool>> {};
+
+TEST_P(UpdateAssociatedSidsTest, ClearUserPropertyWhenNoGaiaIdOrEmail) {
+  FakeAssociatedUserValidator validator;
+  bool is_gaia_id_available = std::get<0>(GetParam());
+  bool is_email_available = std::get<1>(GetParam());
+
+  CComBSTR sid;
+  // Created a test os user with an assigned domain.
+  ASSERT_EQ(S_OK, fake_os_user_manager()->CreateTestOSUser(
+                      L"username", L"password", L"fullname", L"comment",
+                      L"gaia-id", L"user@domain.com", L"domain", &sid));
+
+  // Clear gaia id if needed.
+  if (!is_gaia_id_available)
+    SetUserProperty((BSTR)sid, kUserId, L"");
+
+  // Clear email if needed.
+  if (!is_email_available)
+    SetUserProperty((BSTR)sid, kUserEmail, L"");
+
+  // Invalid token fetch result.
+  fake_http_url_fetcher_factory()->SetFakeResponse(
+      GURL(AssociatedUserValidator::kTokenInfoUrl),
+      FakeWinHttpUrlFetcher::Headers(), "{}");
+
+  validator.StartRefreshingTokenHandleValidity();
+  size_t count = validator.GetAssociatedUsersCount();
+  size_t expected_count;
+  if (is_gaia_id_available || is_email_available)
+    expected_count = 1;
+  else
+    expected_count = 0;
+  EXPECT_EQ(expected_count, count);
+}
+
+INSTANTIATE_TEST_SUITE_P(All,
+                         UpdateAssociatedSidsTest,
+                         ::testing::Combine(::testing::Bool(),
+                                            ::testing::Bool()));
+
 // Tests various scenarios where user access is blocked.
 // Parameters are:
 // 1. CREDENTIAL_PROVIDER_USAGE_SCENARIO - Usage scenario.

chrome/credential_provider/gaiacp/gaia_credential_base.cc

@@ -449,6 +449,14 @@ HRESULT MakeUsernameForAccount(const base::Value& result,
   // First try to detect if this gaia account has been used to create an OS
   // user already.  If so, return the OS username of that user.
   HRESULT hr = GetSidFromId(*gaia_id, sid, sid_length);
+  if (FAILED(hr)) {
+    LOGFN(INFO) << "Failed fetching Sid from Id : " << putHR(hr);
+    // If there is no gaia id user property available in the registry,
+    // fallback to email address mapping.
+    hr = GetSidFromEmail(email, sid, sid_length);
+    if (FAILED(hr))
+      LOGFN(INFO) << "Failed fetching Sid from email : " << putHR(hr);
+  }
 
   bool has_existing_user_sid = false;
   // Check if the machine is domain joined and get the domain name if domain
@@ -2220,8 +2228,8 @@ HRESULT CGaiaCredentialBase::ValidateOrCreateUser(const base::Value& result,
     }
   }
 
-  // If an existing user associated to the gaia id was found, make sure that it
-  // is valid for this credential.
+  // If an existing user associated to the gaia id or email address was found,
+  // make sure that it is valid for this credential.
   if (found_sid[0]) {
     hr = ValidateExistingUser(found_username, found_domain, found_sid,
                               error_text);

chrome/credential_provider/gaiacp/gaia_credential_provider_unittests.cc

@@ -443,11 +443,15 @@ INSTANTIATE_TEST_SUITE_P(
 // 3. bool - is internet available.
 // 4. bool - is active directory user.
 // 5. bool - is internet not available but validity expired.
+// 6. int - 0. Both GaiaID and Email are available.
+//          1. Gaia ID is not available
+//          2. Email is not available
+//          3. Both are unavailable.
 
 class GcpCredentialProviderWithGaiaUsersTest
     : public GcpCredentialProviderTest,
       public ::testing::WithParamInterface<
-          std::tuple<bool, bool, bool, bool, bool>> {
+          std::tuple<bool, bool, bool, bool, bool, int>> {
  protected:
   void SetUp() override;
 };
@@ -466,6 +470,7 @@ TEST_P(GcpCredentialProviderWithGaiaUsersTest, ReauthCredentialTest) {
       has_internet ? FakeInternetAvailabilityChecker::kHicForceYes
                    : FakeInternetAvailabilityChecker::kHicForceNo);
   const bool is_offline_validity_expired = std::get<4>(GetParam());
+  const int user_property_status = std::get<5>(GetParam());
 
   CComBSTR sid;
   if (is_ad_user) {
@@ -482,6 +487,15 @@ TEST_P(GcpCredentialProviderWithGaiaUsersTest, ReauthCredentialTest) {
                         L"gaia-id", L"foo@gmail.com", &sid));
   }
 
+  if (user_property_status & 1) {
+    // Gaia id is not available.
+    SetUserProperty((BSTR)sid, kUserId, L"");
+  }
+  if (user_property_status & 2) {
+    // Email is not available.
+    SetUserProperty((BSTR)sid, kUserEmail, L"");
+  }
+
   ASSERT_EQ(S_OK,
             SetUserProperty(
                 OLE2CW(sid),
@@ -505,8 +519,9 @@ TEST_P(GcpCredentialProviderWithGaiaUsersTest, ReauthCredentialTest) {
   ASSERT_EQ(S_OK, InitializeProviderWithCredentials(&count, &provider));
 
   bool should_reauth_user =
-      (!has_internet && is_offline_validity_expired) ||
-      (has_internet && (!has_token_handle || !valid_token_handle));
+      (user_property_status != 3) &&
+      ((!has_internet && is_offline_validity_expired) ||
+       (has_internet && (!has_token_handle || !valid_token_handle)));
 
   // Check if there is a IReauthCredential depending on the state of the token
   // handle.
@@ -526,7 +541,8 @@ INSTANTIATE_TEST_SUITE_P(All,
                                             ::testing::Bool(),
                                             ::testing::Bool(),
                                             ::testing::Bool(),
-                                            ::testing::Bool()));
+                                            ::testing::Bool(),
+                                            ::testing::Values(0, 1, 2, 3)));
 
 // Check that reauth credentials only exists when either user is an AD user or
 // the token handle for the associated user is no longer valid when internet is

chrome/credential_provider/gaiacp/reauth_credential.cc

@@ -33,25 +33,37 @@ HRESULT CReauthCredential::GetUserGlsCommandline(
   DCHECK(command_line);
   DCHECK(os_user_sid_.Length());
 
+  // This boolean is set to false if generating GlsCommandLine HRESULT
+  // is E_UNEXPECTED.
+  bool get_cmd_line_status = false;
+
   // If this is an existing user with an SID, try to get its gaia id and pass
   // it to the GLS for verification.
   base::string16 gaia_id;
   if (GetIdFromSid(OLE2CW(os_user_sid_), &gaia_id) == S_OK &&
       !gaia_id.empty()) {
     command_line->AppendSwitchNative(kGaiaIdSwitch, gaia_id);
-    if (email_for_reauth_.Length()) {
-      command_line->AppendSwitchNative(kPrefillEmailSwitch,
-                                       OLE2CW(email_for_reauth_));
-    }
-    return CGaiaCredentialBase::GetUserGlsCommandline(command_line);
+    get_cmd_line_status = true;
   } else if (CGaiaCredentialBase::IsCloudAssociationEnabled() &&
              OSUserManager::Get()->IsUserDomainJoined(OLE2CW(os_user_sid_))) {
     // Note that if ADAssociationIsEnabled and the reauth credential is an AD
     // user account, then fallback to the GaiaCredentialBase for loading Gls.
+    get_cmd_line_status = true;
+  }
+
+  // If there is an existing email with an SID then pass it to the GLS
+  // as PrefillEmail switch.
+  if (email_for_reauth_.Length()) {
+    get_cmd_line_status = true;
+    command_line->AppendSwitchNative(kPrefillEmailSwitch,
+                                     OLE2CW(email_for_reauth_));
+  }
+
+  if (get_cmd_line_status) {
     return CGaiaCredentialBase::GetUserGlsCommandline(command_line);
   } else {
     LOGFN(ERROR) << "Reauth credential on user=" << os_username_
-                 << " does not have an associated Gaia id";
+                 << " does not have an associated Gaia id or Email address";
     return E_UNEXPECTED;
   }
 }

chrome/credential_provider/gaiacp/reauth_credential_unittests.cc

@@ -285,14 +285,13 @@ INSTANTIATE_TEST_SUITE_P(All,
 
 class GcpReauthCredentialGlsRunnerTest : public GlsRunnerTestBase {};
 
-TEST_F(GcpReauthCredentialGlsRunnerTest, NoGaiaIdAvailable) {
+TEST_F(GcpReauthCredentialGlsRunnerTest, NoGaiaIdOrEmailAvailable) {
   USES_CONVERSION;
   CredentialProviderSigninDialogTestDataStorage test_data_storage;
 
   CComBSTR username = L"foo_bar";
   CComBSTR full_name = A2COLE(test_data_storage.GetSuccessFullName().c_str());
   CComBSTR password = A2COLE(test_data_storage.GetSuccessPassword().c_str());
-  CComBSTR email = A2COLE(test_data_storage.GetSuccessEmail().c_str());
 
   // Create a fake user to reauth.
   CComBSTR sid;
@@ -300,7 +299,7 @@ TEST_F(GcpReauthCredentialGlsRunnerTest, NoGaiaIdAvailable) {
             fake_os_user_manager()->CreateTestOSUser(
                 OLE2CW(username), OLE2CW(password), OLE2CW(full_name),
                 L"comment", base::UTF8ToUTF16(test_data_storage.GetSuccessId()),
-                OLE2CW(email), &sid));
+                base::string16(), &sid));
 
   // Create provider and start logon.
   Microsoft::WRL::ComPtr<ICredentialProviderCredential> cred;
@@ -309,7 +308,7 @@ TEST_F(GcpReauthCredentialGlsRunnerTest, NoGaiaIdAvailable) {
   SetDefaultTokenHandleResponse(kDefaultInvalidTokenHandleResponse);
   ASSERT_EQ(S_OK, InitializeProviderAndGetCredential(1, &cred));
 
-  // Change the registry entry for gaia id to empty string.
+  // Change the registry entry for gaia id and email to empty string.
   ASSERT_EQ(S_OK, SetUserProperty(OLE2CW(sid), kUserId, L""));
 
   // The GetSerialization call that loads the GLS should fail.
@@ -497,6 +496,43 @@ TEST_F(GcpReauthCredentialGlsRunnerTest, NormalReauthWithoutEmail) {
   // Teardown of the test should confirm that the logon was successful.
 }
 
+TEST_F(GcpReauthCredentialGlsRunnerTest, NormalReauthWithoutGaiaId) {
+  USES_CONVERSION;
+  CredentialProviderSigninDialogTestDataStorage test_data_storage;
+
+  CComBSTR username = L"foo_bar";
+  CComBSTR full_name = A2COLE(test_data_storage.GetSuccessFullName().c_str());
+  CComBSTR password = A2COLE(test_data_storage.GetSuccessPassword().c_str());
+
+  // Create a fake user to reauth with no gaia-id specified.
+  CComBSTR sid;
+  ASSERT_EQ(S_OK, fake_os_user_manager()->CreateTestOSUser(
+                      OLE2CW(username), OLE2CW(password), OLE2CW(full_name),
+                      L"comment", base::string16(),
+                      base::UTF8ToUTF16(kDefaultEmail), &sid));
+
+  // Create provider and start logon.
+  Microsoft::WRL::ComPtr<ICredentialProviderCredential> cred;
+
+  // Create with invalid token handle response so that a reauth occurs.
+  SetDefaultTokenHandleResponse(kDefaultInvalidTokenHandleResponse);
+  ASSERT_EQ(S_OK, InitializeProviderAndGetCredential(1, &cred));
+
+  Microsoft::WRL::ComPtr<ITestCredential> test;
+  ASSERT_EQ(S_OK, cred.As(&test));
+
+  // Don't send a forced e-mail. It will be sent from the user that was
+  // updated during the last sign in.
+  ASSERT_EQ(S_OK, test->SetGlsEmailAddress(std::string()));
+
+  ASSERT_EQ(S_OK, StartLogonProcessAndWait());
+
+  // Email associated should be the default one
+  EXPECT_EQ(test->GetFinalEmail(), kDefaultEmail);
+
+  // Teardown of the test should confirm that the logon was successful.
+}
+
 TEST_F(GcpReauthCredentialGlsRunnerTest, GaiaIdMismatch) {
   USES_CONVERSION;
   CredentialProviderSigninDialogTestDataStorage test_data_storage;

chrome/credential_provider/gaiacp/reg_utils.cc

@@ -14,6 +14,7 @@
 #include "base/win/win_util.h"
 #include "build/branding_buildflags.h"
 #include "chrome/credential_provider/common/gcp_strings.h"
+#include "chrome/credential_provider/gaiacp/logging.h"
 
 namespace credential_provider {
 
@@ -267,26 +268,32 @@ HRESULT GetUserTokenHandles(
     length = base::size(token_handle);
     HRESULT token_handle_hr =
         GetUserProperty(sid, kUserTokenHandle, token_handle, &length);
+    wchar_t email_address[256];
+    length = base::size(email_address);
+    HRESULT email_address_hr =
+        GetUserProperty(sid, kUserEmail, email_address, &length);
     sid_to_handle_info->emplace(
         sid,
         UserTokenHandleInfo{SUCCEEDED(gaia_id_hr) ? gaia_id : L"",
+                            SUCCEEDED(email_address_hr) ? email_address : L"",
                             SUCCEEDED(token_handle_hr) ? token_handle : L""});
   }
   return S_OK;
 }
 
-HRESULT GetSidFromId(const base::string16& id, wchar_t* sid, ULONG length) {
+HRESULT GetSidFromKey(const wchar_t* key,
+                      const base::string16& value,
+                      wchar_t* sid,
+                      ULONG length) {
   DCHECK(sid);
-
-
   bool result_found = false;
   base::win::RegistryKeyIterator iter(HKEY_LOCAL_MACHINE, kGcpUsersRootKeyName);
   for (; iter.Valid(); ++iter) {
     const wchar_t* user_sid = iter.Name();
-    wchar_t user_id[256];
-    ULONG user_length = base::size(user_id);
-    HRESULT hr = GetUserProperty(user_sid, kUserId, user_id, &user_length);
-    if (SUCCEEDED(hr) && id == user_id) {
+    wchar_t result[256];
+    ULONG result_length = base::size(result);
+    HRESULT hr = GetUserProperty(user_sid, key, result, &result_length);
+    if (SUCCEEDED(hr) && value == result) {
       // Make sure there are not 2 users with the same SID.
       if (result_found)
         return HRESULT_FROM_WIN32(ERROR_USER_EXISTS);
@@ -299,6 +306,16 @@ HRESULT GetSidFromId(const base::string16& id, wchar_t* sid, ULONG length) {
   return result_found ? S_OK : HRESULT_FROM_WIN32(ERROR_NONE_MAPPED);
 }
 
+HRESULT GetSidFromEmail(const base::string16& email,
+                        wchar_t* sid,
+                        ULONG length) {
+  return GetSidFromKey(kUserEmail, email, sid, length);
+}
+
+HRESULT GetSidFromId(const base::string16& id, wchar_t* sid, ULONG length) {
+  return GetSidFromKey(kUserId, id, sid, length);
+}
+
 HRESULT GetIdFromSid(const wchar_t* sid, base::string16* id) {
   DCHECK(id);
 

chrome/credential_provider/gaiacp/reg_utils.h

@@ -104,6 +104,7 @@ HRESULT RemoveAllUserProperties(const base::string16& sid);
 
 struct UserTokenHandleInfo {
   base::string16 gaia_id;
+  base::string16 email_address;
   base::string16 token_handle;
 };
 
@@ -120,6 +121,19 @@ HRESULT GetUserTokenHandles(
 // HRESULT_FROM_WIN32(ERROR_NONE_MAPPED).
 HRESULT GetSidFromId(const base::string16& id, wchar_t* sid, ULONG length);
 
+// Gets the SID associated with the given email.  If none exists, returns
+// HRESULT_FROM_WIN32(ERROR_NONE_MAPPED).
+HRESULT GetSidFromEmail(const base::string16& email,
+                        wchar_t* sid,
+                        ULONG length);
+
+// Gets the SID associated with the given input key. If none exists, returns
+// HRESULT_FROM_WIN32(ERROR_NONE_MAPPED).
+HRESULT GetSidFromKey(const wchar_t* key,
+                      const base::string16& value,
+                      wchar_t* sid,
+                      ULONG length);
+
 // Gets the gaia id associated with the given SID.  If none exists, returns
 // HRESULT_FROM_WIN32(ERROR_NONE_MAPPED).
 HRESULT GetIdFromSid(const wchar_t* sid, base::string16* id);