From 6124505c7a2562868d860c4ebcd7e9bdb8eb1dc0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ren=C3=A9=20Meusel?= <rene.meusel@nexenio.com>
Date: Thu, 5 Dec 2019 09:53:12 +0100
Subject: [PATCH] Simplify error code handling on Windows (#10)

* set error code only on failure

* make sure Error is initialized with 'NoError'

* makeTargetName() does internal error handling

* review adaptions

* FIX: compilation on windows

* Update keychain_win.cpp

Co-Authored-By: Hannes Rantzsch <hrantzsch@users.noreply.github.com>

* Update keychain_win.cpp

Co-Authored-By: Hannes Rantzsch <hrantzsch@users.noreply.github.com>
---
 keychain.h       |  2 ++
 keychain_win.cpp | 53 +++++++++++++++++++++++++++++-------------------
 2 files changed, 34 insertions(+), 21 deletions(-)

diff --git a/keychain.h b/keychain.h
index 24e41f3..d8db953 100644
--- a/keychain.h
+++ b/keychain.h
@@ -102,6 +102,8 @@ enum class ErrorType {
  * to a function.
  */
 struct Error {
+    Error() : type(ErrorType::NoError) {}
+
     /*! \brief The type or reason of the error
      *
      * Note that some types of errors can only occur on certain platforms. In
diff --git a/keychain_win.cpp b/keychain_win.cpp
index 1c59691..1b7050b 100644
--- a/keychain_win.cpp
+++ b/keychain_win.cpp
@@ -156,9 +156,21 @@ void updateError(keychain::Error &err) {
  * The result is wrapped in a ScopedLpwstr.
  */
 ScopedLpwstr makeTargetName(const std::string &package,
-                            const std::string &service,
-                            const std::string &user) {
-    return utf8ToWideChar(package + "." + service + '/' + user);
+                            const std::string &service, const std::string &user,
+                            keychain::Error &err) {
+    auto result = utf8ToWideChar(package + "." + service + '/' + user);
+    if (!result) {
+        updateError(err);
+
+        // make really sure that we set an error code if we will return nullptr
+        if (!err) {
+            err.type = keychain::ErrorType::GenericError;
+            err.message = "Failed to create credential target name.";
+            err.code = -1; // generic non-zero
+        }
+    }
+
+    return result;
 }
 
 } // namespace
@@ -168,11 +180,8 @@ namespace keychain {
 void setPassword(const std::string &package, const std::string &service,
                  const std::string &user, const std::string &password,
                  Error &err) {
-    ::SetLastError(0); // clear thread-global error
-
-    auto target_name = makeTargetName(package, service, user);
-    if (!target_name) {
-        updateError(err);
+    auto target_name = makeTargetName(package, service, user, err);
+    if (err) {
         return;
     }
 
@@ -198,42 +207,44 @@ void setPassword(const std::string &package, const std::string &service,
     cred.CredentialBlob = (LPBYTE)(password.data());
     cred.Persist = CRED_PERSIST_ENTERPRISE;
 
-    ::CredWrite(&cred, 0);
-    updateError(err);
+    if (::CredWrite(&cred, 0) == FALSE) {
+        updateError(err);
+    }
 }
 
 std::string getPassword(const std::string &package, const std::string &service,
                         const std::string &user, Error &err) {
-    ::SetLastError(0); // clear thread-global error
     std::string password;
 
-    auto target_name = makeTargetName(package, service, user);
-    if (!target_name) {
-        updateError(err);
+    auto target_name = makeTargetName(package, service, user, err);
+    if (err) {
         return password;
     }
 
     CREDENTIAL *cred;
     bool result = ::CredRead(target_name.get(), kCredType, 0, &cred);
-    updateError(err);
 
-    if (!err && result) {
+    if (result == TRUE) {
         password = std::string(reinterpret_cast<char *>(cred->CredentialBlob),
                                cred->CredentialBlobSize);
         ::CredFree(cred);
+    } else {
+        updateError(err);
     }
+
     return password;
 }
 
 void deletePassword(const std::string &package, const std::string &service,
                     const std::string &user, Error &err) {
-    ::SetLastError(0); // clear thread-global error
+    auto target_name = makeTargetName(package, service, user, err);
+    if (err) {
+        return;
+    }
 
-    auto target_name = makeTargetName(package, service, user);
-    if (target_name) {
-        ::CredDelete(target_name.get(), kCredType, 0);
+    if (::CredDelete(target_name.get(), kCredType, 0) == FALSE) {
+        updateError(err);
     }
-    updateError(err);
 }
 
 } // namespace keychain