-
Notifications
You must be signed in to change notification settings - Fork 18
Adds keychain availability checks for all platforms #46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -270,4 +270,49 @@ void deletePassword(const std::string &package, const std::string &service, | |
| updateError(err, SecItemDelete(query.get())); | ||
| } | ||
|
|
||
| bool isAvailable(Error &err) { | ||
| err = Error{}; | ||
|
|
||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wouldn't consider any runtime failure here as an indicator of the keychain not being available, and I wouldn't shadow the lower-level errors with the Can we use I think we can do without the specific test cases for
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that runtime errors should not be masked with I am okay with removing these test hooks
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I needed to go back and review I do feel like you're right for everything
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My only real thing is that the way I did it is more of a proactive probe... it's confirming the keychain is usable and can help indicate if By breaking it down to the lowest level parts and testing each step in isolation we can help improve the use experience |
||
| auto query = createCFMutableDictionary(err); | ||
| if (!query) { | ||
| err.type = ErrorType::Unavailable; | ||
| err.message = "Failed to create query dictionary"; | ||
| return false; | ||
| } | ||
|
|
||
| CFDictionaryAddValue(query.get(), kSecClass, kSecClassGenericPassword); | ||
|
|
||
| auto service = | ||
| createCFStringWithCString("keychain_availability_check_service", err); | ||
| auto account = | ||
| createCFStringWithCString("keychain_availability_check_account", err); | ||
| if (!service || !account) { | ||
| err.type = ErrorType::Unavailable; | ||
| err.message = "Failed to create service/account string"; | ||
| return false; | ||
| } | ||
|
|
||
| CFDictionaryAddValue(query.get(), kSecAttrService, service.get()); | ||
| CFDictionaryAddValue(query.get(), kSecAttrAccount, account.get()); | ||
| CFDictionaryAddValue(query.get(), kSecReturnData, kCFBooleanFalse); | ||
|
|
||
| #ifdef SIMULATE_FAILURES | ||
| // TEST HOOK: Simulate SecItemCopyMatching failure | ||
| if (getenv("KEYCHAIN_TEST_SIMULATED_FAILURE")) { | ||
| err.type = ErrorType::Unavailable; | ||
| err.message = "Simulated failure: SecItemCopyMatching"; | ||
| return false; | ||
| } | ||
| #endif | ||
|
|
||
| OSStatus status = SecItemCopyMatching(query.get(), nullptr); | ||
|
|
||
| if (status == errSecSuccess || status == errSecItemNotFound) { | ||
| return true; | ||
| } else { | ||
| err.type = ErrorType::Unavailable; | ||
| err.message = errorStatusToString(status); | ||
| return false; | ||
| } | ||
| } | ||
| } // namespace keychain | ||
Uh oh!
There was an error while loading. Please reload this page.