Skip to content

Commit

Permalink
Error handling (#17)
Browse files Browse the repository at this point in the history 
On a successful function call, the provided `Error` is now always set to `NoError`. Already set errors are ignored.
  • Loading branch information
@hrantzsch
hrantzsch authored Dec 18, 2019
1 parent 9526ef0 commit 09fd0c3
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 36 deletions.
14 changes: 6 additions & 8 deletions keychain.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,8 @@
* parameter in order to correspond with conventions.
*
* In addition, each function expects an instance of `keychain::Error` as an
* output parameter to indicate success or failure. Note that the error
* parameter must not already be set to an error at the time the function is
* called.
* output parameter to indicate success or failure. Note that previous states of
* the Error are ignored and potentially overwritten.
*
* Also note that all three functions are blocking (potentially indefinitely)
* for example if the OS prompts the user to unlock their credentials storage.
Expand All @@ -55,7 +54,7 @@ struct Error;
/*! \brief Retrieve a password
*
* \param package, service, user Used to identify the password to get
* \param err Output parameter set to indicate what error occurred, if any
* \param err Output parameter communicating success or error details
*
* \return The password, if the function was successful
*/
Expand All @@ -68,7 +67,7 @@ std::string getPassword(const std::string &package, const std::string &service,
*
* \param package, service, user Used to identify the password to set
* \param password The new password
* \param err Output parameter set to indicate what error occurred, if any
* \param err Output parameter communicating success or error details
*/
void setPassword(const std::string &package, const std::string &service,
const std::string &user, const std::string &password,
Expand All @@ -80,7 +79,7 @@ void setPassword(const std::string &package, const std::string &service,
* error.
*
* \param package, service, user Used to identify the password to delete
* \param err Output parameter set to indicate what error occurred, if any
* \param err Output parameter communicating success or error details
*/
void deletePassword(const std::string &package, const std::string &service,
const std::string &user, Error &err);
Expand All @@ -98,8 +97,7 @@ enum class ErrorType {
/*! \brief A struct to collect error information
*
* An instance of this struct is used as an output parameter to indicate success
* or failure. Note that Errors should not already contain an error when handed
* to a function.
* or failure.
*/
struct Error {
Error() : type(ErrorType::NoError) {}
Expand Down
47 changes: 26 additions & 21 deletions keychain_linux.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,11 @@ namespace keychain {
void setPassword(const std::string &package, const std::string &service,
const std::string &user, const std::string &password,
Error &err) {
err = Error{};
const auto schema = makeSchema(package);
const auto label = makeLabel(service, user);

GError *error = NULL;

secret_password_store_sync(&schema,
SECRET_COLLECTION_DEFAULT,
label.c_str(),
Expand All @@ -99,40 +100,44 @@ void setPassword(const std::string &package, const std::string &service,
user.c_str(),
NULL);

updateError(err, error);
if (error != NULL) {
updateError(err, error);
}
}

std::string getPassword(const std::string &package, const std::string &service,
const std::string &user, Error &err) {
err = Error{};
const auto schema = makeSchema(package);

GError *error = NULL;
gchar *raw_passwords;
raw_passwords = secret_password_lookup_sync(&schema,
NULL, // not cancellable
&error,
ServiceFieldName,
service.c_str(),
AccountFieldName,
user.c_str(),
NULL);

updateError(err, error);

gchar *raw_passwords = secret_password_lookup_sync(&schema,
NULL, // not cancellable
&error,
ServiceFieldName,
service.c_str(),
AccountFieldName,
user.c_str(),
NULL);

std::string password;

if (!err && raw_passwords != NULL) {
password = raw_passwords;
secret_password_free(raw_passwords);
if (error != NULL) {
updateError(err, error);
} else if (raw_passwords == NULL) {
// libsecret reports no error if the password was not found
setErrorNotFound(err);
} else {
password = raw_passwords;
secret_password_free(raw_passwords);
}

return password;
}

void deletePassword(const std::string &package, const std::string &service,
const std::string &user, Error &err) {
err = Error{};
const auto schema = makeSchema(package);
GError *error = NULL;

Expand All @@ -145,10 +150,10 @@ void deletePassword(const std::string &package, const std::string &service,
user.c_str(),
NULL);

updateError(err, error);

if (!err && !deleted) {
// password did not exist is considered an error
if (error != NULL) {
updateError(err, error);
} else if (!deleted) {
// libsecret reports no error if the password did not exist
setErrorNotFound(err);
}
}
Expand Down
29 changes: 22 additions & 7 deletions keychain_mac.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,9 @@ namespace keychain {
void setPassword(const std::string &package, const std::string &service,
const std::string &user, const std::string &password,
Error &err) {
err = Error{};
const auto serviceName = makeServiceName(package, service);

OSStatus status =
SecKeychainAddGenericPassword(defaultUserKeychain,
static_cast<UInt32>(serviceName.length()),
Expand All @@ -160,14 +162,18 @@ void setPassword(const std::string &package, const std::string &service,
status = modifyPassword(serviceName, user, password);
}

updateError(err, status);
if (status != errSecSuccess) {
updateError(err, status);
}
}

std::string getPassword(const std::string &package, const std::string &service,
const std::string &user, Error &err) {
err = Error{};
const auto serviceName = makeServiceName(package, service);
void *data;
UInt32 length;

OSStatus status = SecKeychainFindGenericPassword(
defaultUserKeychain,
static_cast<UInt32>(serviceName.length()),
Expand All @@ -180,8 +186,9 @@ std::string getPassword(const std::string &package, const std::string &service,

std::string password;

updateError(err, status);
if (!err && data != NULL) {
if (status != errSecSuccess) {
updateError(err, status);
} else if (data != NULL) {
password = std::string(reinterpret_cast<const char *>(data), length);
SecKeychainItemFreeContent(NULL, data);
}
Expand All @@ -191,8 +198,10 @@ std::string getPassword(const std::string &package, const std::string &service,

void deletePassword(const std::string &package, const std::string &service,
const std::string &user, Error &err) {
err = Error{};
const auto serviceName = makeServiceName(package, service);
SecKeychainItemRef item;

OSStatus status = SecKeychainFindGenericPassword(
defaultUserKeychain,
static_cast<UInt32>(serviceName.length()),
Expand All @@ -203,13 +212,19 @@ void deletePassword(const std::string &package, const std::string &service,
NULL, // unused output parameter
&item);

updateError(err, status);
if (!err) {
status = SecKeychainItemDelete(item);
if (status != errSecSuccess) {
updateError(err, status);
} else {
status = SecKeychainItemDelete(item);
if (status != errSecSuccess) {
updateError(err, status);
}
}

if (item) {
CFRelease(item);
}

CFRelease(item);
}

} // namespace keychain
3 changes: 3 additions & 0 deletions keychain_win.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ namespace keychain {
void setPassword(const std::string &package, const std::string &service,
const std::string &user, const std::string &password,
Error &err) {
err = Error{};
auto target_name = makeTargetName(package, service, user, err);
if (err) {
return;
Expand Down Expand Up @@ -214,6 +215,7 @@ void setPassword(const std::string &package, const std::string &service,

std::string getPassword(const std::string &package, const std::string &service,
const std::string &user, Error &err) {
err = Error{};
std::string password;

auto target_name = makeTargetName(package, service, user, err);
Expand All @@ -237,6 +239,7 @@ std::string getPassword(const std::string &package, const std::string &service,

void deletePassword(const std::string &package, const std::string &service,
const std::string &user, Error &err) {
err = Error{};
auto target_name = makeTargetName(package, service, user, err);
if (err) {
return;
Expand Down
15 changes: 15 additions & 0 deletions test/tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,4 +96,19 @@ TEST_CASE("Keychain", "[keychain]") {
deletePassword("no.package", "no.service", "no.user", ec);
CHECK(ec.type == ErrorType::NotFound);
}

SECTION("successful function call overrides previous Error to success") {
Error ec{};
ec.type = ErrorType::GenericError;
setPassword(package, service, user, password, ec);
check_no_error(ec);

ec.type = ErrorType::GenericError;
getPassword(package, service, user, ec);
check_no_error(ec);

ec.type = ErrorType::GenericError;
deletePassword(package, service, user, ec);
check_no_error(ec);
}
}

0 comments on commit 09fd0c3

Please sign in to comment.