Fix blank error dialog from credential provider.

When the user enters an old password into the credential provider after
a password change, make sure to use an appropriate message.  At the same time
make sure that all error paths return a valid string otherwise a blank error
will be shown.

Bug: 902706
Change-Id: I2835f8b11d8f1f116e64333d47515115b642b066
Reviewed-on: https://chromium-review.googlesource.com/c/1323270
Reviewed-by: Owen Min <zmin@chromium.org>
Commit-Queue: Roger Tawa <rogerta@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606179}

chrome/credential_provider/gaiacp/gaia_credential.cc

@@ -52,6 +52,7 @@ HRESULT CGaiaCredential::FinishAuthentication(BSTR username,
     } else {
       LOGFN(INFO) << "CreateLogonToken hr=" << putHR(hrLogon)
                   << " account=" << OLE2CW(username) << " sid=" << sid;
+      *error_text = AllocErrorString(IDS_INVALID_PASSWORD);
     }
   } else if (FAILED(hr)) {
     LOGFN(ERROR) << "CreateNewUser hr=" << putHR(hr)

chrome/credential_provider/gaiacp/gaia_credential_base.cc

@@ -1016,19 +1016,21 @@ HRESULT CGaiaCredentialBase::ForkSaveAccountInfoStub(
       command_line, startupinfo.GetInfo(), &procinfo);
   if (FAILED(hr)) {
     LOGFN(ERROR) << "CreateProcessWithTokenW hr=" << putHR(hr);
+    *status_text = AllocErrorString(IDS_INTERNAL_ERROR);
     return hr;
   }
 
   // Write account info to stdin of child process.  This buffer is read by
-  // SaveAccountInfoW() in dllmain.cpp.
+  // SaveAccountInfoW() in dllmain.cpp.  If this fails, chrome won't pick up
+  // the credentials from the credential provider and will need to sign in
+  // manually.  TODO(crbug.com/902911): Figure out how to handle this.
   std::string json;
   if (base::JSONWriter::Write(*dict, &json)) {
     DWORD written;
     if (!::WriteFile(parent_handles.hstdin_write.Get(), json.c_str(),
                      json.length() + 1, &written, /*lpOverlapped=*/nullptr)) {
-      HRESULT hr = HRESULT_FROM_WIN32(::GetLastError());
-      LOGFN(ERROR) << "WriteFile hr=" << putHR(hr);
-      *status_text = AllocErrorString(IDS_INTERNAL_ERROR);
+      HRESULT hrWrite = HRESULT_FROM_WIN32(::GetLastError());
+      LOGFN(ERROR) << "WriteFile hr=" << putHR(hrWrite);
     }
   } else {
     LOGFN(ERROR) << "base::JSONWriter::Write failed";
@@ -1070,6 +1072,11 @@ unsigned __stdcall CGaiaCredentialBase::WaitForLoginUI(void* param) {
     // If hr is E_ABORT, this is a user initiated cancel.  Don't consider this
     // an error.
     LONG sts = hr == E_ABORT ? STATUS_SUCCESS : HRESULT_CODE(hr);
+
+    // Either WaitForLoginUIImpl did not fail or there should be an error
+    // message to display.
+    DCHECK(sts > 0 || status_text != nullptr);
+
     hr = uiprocinfo->credential->ReportError(sts, STATUS_SUCCESS, status_text);
     if (FAILED(hr)) {
       LOGFN(ERROR) << "uiprocinfo->credential->ReportError hr=" << putHR(hr);
@@ -1123,22 +1130,20 @@ HRESULT CGaiaCredentialBase::WaitForLoginUIImpl(
 
   dict->SetString(kKeySID, OLE2CA(sid));
 
-  if (SUCCEEDED(hr)) {
-    // Fire off a process to call SaveAccountInfo().
-    //
-    // The eventual call to OnUserAuthenticated() will tell winlogon that
-    // logging in is finished. It seems that winlogon will kill this process
-    // after a short time, which races with an attempt to save the account info
-    // to the registry if done here.  For this reason a child pocess is used.
-    hr = ForkSaveAccountInfoStub(dict, status_text);
-    if (FAILED(hr)) {
-      LOGFN(ERROR) << "ForkSaveAccountInfoStub hr=" << putHR(hr);
-      return hr;
-    }
-
-    *properties = std::move(dict);
+  // Fire off a process to call SaveAccountInfo().
+  //
+  // The eventual call to OnUserAuthenticated() will tell winlogon that
+  // logging in is finished. It seems that winlogon will kill this process
+  // after a short time, which races with an attempt to save the account info
+  // to the registry if done here.  For this reason a child pocess is used.
+  hr = ForkSaveAccountInfoStub(dict, status_text);
+  if (FAILED(hr)) {
+    LOGFN(ERROR) << "ForkSaveAccountInfoStub hr=" << putHR(hr);
+    return hr;
   }
 
+  *properties = std::move(dict);
+
   // When this function returns, winlogon will be told to logon to the newly
   // created account.  This is important, as the save account info process
   // can't actually save the info until the user's profile is created, which

chrome/credential_provider/gaiacp/gaia_resources.grd

@@ -136,6 +136,9 @@
       <message name="IDS_CANT_CREATE_USER" desc="">
         A user could not be created.
       </message>
+      <message name="IDS_INVALID_PASSWORD" desc="">
+        Sorry, your password couldn't be verified. Please try again.
+      </message>
       <message name="IDS_INTERNAL_ERROR" desc="">
         An internal error occurred.
       </message>

chrome/credential_provider/gaiacp/gaia_resources_grd/IDS_INVALID_PASSWORD.png.sha1

@@ -0,0 +1 @@
+9b9fa182547010b8e5b8c88c71bbca5926375d10

chrome/credential_provider/gaiacp/reauth_credential.cc

@@ -96,6 +96,7 @@ HRESULT CReauthCredential::FinishAuthentication(BSTR username,
   hr = SetUserProperty(OLE2CW(user_sid_), kUserNeedsReauth, 0);
   if (FAILED(hr)) {
     LOGFN(ERROR) << "SetUserProperty hr=" << putHR(hr);
+    *error_text = AllocErrorString(IDS_INTERNAL_ERROR);
     return hr;
   }