Wait for report result prior to resetting internal state for the scenario
where login attempt using CGaiaCredential is performed.
Bug: 973231
Change-Id: Ib062717efd220d558d849c0c7d2965e56ebbdc55
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1652715
Commit-Queue: Rakesh Soma <rakeshsoma@google.com>
Reviewed-by: Tien Mai <tienmai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#669485}
diff --git a/chrome/credential_provider/gaiacp/gaia_credential_base.cc b/chrome/credential_provider/gaiacp/gaia_credential_base.cc
index 4888f69b..5001663a 100644
--- a/chrome/credential_provider/gaiacp/gaia_credential_base.cc
+++ b/chrome/credential_provider/gaiacp/gaia_credential_base.cc
@@ -632,6 +632,7 @@
LOGFN(INFO);
username_.Empty();
domain_.Empty();
+ wait_for_report_result_ = false;
SecurelyClearBuffer((BSTR)password_, password_.ByteLength());
password_.Empty();
@@ -940,14 +941,22 @@
HRESULT CGaiaCredentialBase::SetDeselected(void) {
LOGFN(INFO);
- // Cancel logon so that the next time this credential is clicked everything
- // has to be re-entered by the user. This prevents a Windows password
- // entered into the password field by the user from being persisted too
- // long. The behaviour is similar to that of the normal windows password
- // text box. Whenever a different user is selected and then the original
- // credential is selected again, the password is cleared.
- ResetInternalState();
-
+ // This check is trying to handle the scenario when GetSerialization finishes
+ // with cpgsr set as CPGSR_RETURN_CREDENTIAL_FINISHED which indicates that
+ // the windows autologon is ready to go. In this case ideally ReportResult
+ // should be invoked by the windows login UI process prior to SetDeselected.
+ // But for OtherUserCredential scenario, SetDeselected is being invoked
+ // prior to ReportResult which is leading to clearing of the internalstate
+ // prior to saving the account user info in ReportResult.
+ if (!wait_for_report_result_) {
+ // Cancel logon so that the next time this credential is clicked everything
+ // has to be re-entered by the user. This prevents a Windows password
+ // entered into the password field by the user from being persisted too
+ // long. The behaviour is similar to that of the normal windows password
+ // text box. Whenever a different user is selected and then the original
+ // credential is selected again, the password is cleared.
+ ResetInternalState();
+ }
return S_OK;
}
@@ -1175,6 +1184,13 @@
if (submit_button_enabled)
token_update_locker_.reset();
+ // If cpgsr is CPGSR_RETURN_CREDENTIAL_FINISHED and the status is S_OK, then
+ // report result would be invoked. So we shouldn't be resetting the internal
+ // state prior to report result getting triggered.
+ if (*cpgsr == CPGSR_RETURN_CREDENTIAL_FINISHED && hr == S_OK) {
+ wait_for_report_result_ = true;
+ }
+
// Otherwise, keep the ui disabled forever now. ReportResult will eventually
// be called on success or failure and the reset of the state of the
// credential will be done there.
diff --git a/chrome/credential_provider/gaiacp/gaia_credential_base.h b/chrome/credential_provider/gaiacp/gaia_credential_base.h
index 632abbe..051bdf5 100644
--- a/chrome/credential_provider/gaiacp/gaia_credential_base.h
+++ b/chrome/credential_provider/gaiacp/gaia_credential_base.h
@@ -288,6 +288,10 @@
bool needs_windows_password_ = false;
bool request_force_password_change_ = false;
+ // Boolean to indicate if we should wait for ReportResult() prior to clearing
+ // internal state.
+ bool wait_for_report_result_ = false;
+
// The password entered into the FID_CURRENT_PASSWORD_FIELD to update the
// Windows password with the gaia password.
CComBSTR current_windows_password_;
diff --git a/chrome/credential_provider/gaiacp/gaia_credential_base_unittests.cc b/chrome/credential_provider/gaiacp/gaia_credential_base_unittests.cc
index 51590eb..6eee29a 100644
--- a/chrome/credential_provider/gaiacp/gaia_credential_base_unittests.cc
+++ b/chrome/credential_provider/gaiacp/gaia_credential_base_unittests.cc
@@ -2,6 +2,10 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+#include <windows.h>
+
+#include <sddl.h> // For ConvertSidToStringSid()
+
#include "base/strings/utf_string_conversions.h"
#include "chrome/credential_provider/common/gcp_strings.h"
#include "chrome/credential_provider/gaiacp/gaia_credential_base.h"
@@ -91,6 +95,62 @@
EXPECT_EQ(2ul, fake_os_user_manager()->GetUserCount());
}
+// This test emulates the scenario where SetDeselected is triggered by the
+// Windows Login UI process after GetSerialization prior to invocation of
+// ReportResult. Note: This currently happens only for OtherUser credential
+// workflow.
+TEST_F(GcpGaiaCredentialBaseTest,
+ GetSerialization_SetDeselectedBeforeReportResult) {
+ // Create provider and start logon.
+ CComPtr<ICredentialProviderCredential> cred;
+
+ ASSERT_EQ(S_OK, InitializeProviderAndGetCredential(0, &cred));
+
+ CComPtr<ITestCredential> test;
+ ASSERT_EQ(S_OK, cred.QueryInterface(&test));
+
+ ASSERT_EQ(S_OK, StartLogonProcessAndWait());
+
+ EXPECT_EQ(test->GetFinalEmail(), kDefaultEmail);
+
+ // Make sure a "foo" user was created.
+ PSID sid;
+ EXPECT_EQ(S_OK, fake_os_user_manager()->GetUserSID(
+ OSUserManager::GetLocalDomain().c_str(), kDefaultUsername,
+ &sid));
+
+ // New user should be created.
+ EXPECT_EQ(2ul, fake_os_user_manager()->GetUserCount());
+
+ // Finishing logon process should trigger credential changed and trigger
+ // GetSerialization.
+ ASSERT_EQ(S_OK, FinishLogonProcessWithCred(true, true, 0, cred));
+
+ // Trigger SetDeselected prior to ReportResult is invoked.
+ cred->SetDeselected();
+
+ // Verify that the authentication results dictionary is not empty.
+ ASSERT_FALSE(test->IsAuthenticationResultsEmpty());
+
+ // Trigger ReportResult and verify that the authentication results are saved
+ // into registry and ResetInternalState is triggered.
+ ReportLogonProcessResult(cred);
+
+ // Verify that the registry entry for the user was created.
+ wchar_t gaia_id[256];
+ ULONG length = base::size(gaia_id);
+ wchar_t* sidstr = nullptr;
+ ::ConvertSidToStringSid(sid, &sidstr);
+ ::LocalFree(sid);
+
+ HRESULT gaia_id_hr = GetUserProperty(sidstr, kUserId, gaia_id, &length);
+ ASSERT_EQ(S_OK, gaia_id_hr);
+ ASSERT_TRUE(gaia_id[0]);
+
+ // Verify that the authentication results dictionary is now empty.
+ ASSERT_TRUE(test->IsAuthenticationResultsEmpty());
+}
+
TEST_F(GcpGaiaCredentialBaseTest, GetSerialization_Abort) {
// Create provider and start logon.
CComPtr<ICredentialProviderCredential> cred;
diff --git a/chrome/credential_provider/test/test_credential.h b/chrome/credential_provider/test/test_credential.h
index eaa197d7..720297a 100644
--- a/chrome/credential_provider/test/test_credential.h
+++ b/chrome/credential_provider/test/test_credential.h
@@ -44,6 +44,7 @@
SetStartGlsEventName(const base::string16& event_name) = 0;
virtual BSTR STDMETHODCALLTYPE GetFinalUsername() = 0;
virtual std::string STDMETHODCALLTYPE GetFinalEmail() = 0;
+ virtual bool STDMETHODCALLTYPE IsAuthenticationResultsEmpty() = 0;
virtual BSTR STDMETHODCALLTYPE GetErrorText() = 0;
virtual bool STDMETHODCALLTYPE AreCredentialsValid() = 0;
virtual bool STDMETHODCALLTYPE CanAttemptWindowsLogon() = 0;
@@ -80,6 +81,7 @@
const base::string16& event_name) override;
BSTR STDMETHODCALLTYPE GetFinalUsername() override;
std::string STDMETHODCALLTYPE GetFinalEmail() override;
+ bool STDMETHODCALLTYPE IsAuthenticationResultsEmpty() override;
BSTR STDMETHODCALLTYPE GetErrorText() override;
bool STDMETHODCALLTYPE AreCredentialsValid() override;
bool STDMETHODCALLTYPE CanAttemptWindowsLogon() override;
@@ -186,6 +188,13 @@
}
template <class T>
+bool CTestCredentialBase<T>::IsAuthenticationResultsEmpty() {
+ auto& results = this->get_authentication_results();
+
+ return !results || (results->is_dict() && results->DictEmpty());
+}
+
+template <class T>
std::string CTestCredentialBase<T>::GetFinalEmail() {
auto& results = this->get_authentication_results();
