Replace the deprecated SecKeychain with SecItem #46
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add missing ReleaseByteArrayElements releases
WalkthroughThis update involves refining the handling of macOS Keychain services in Cryptomator by transitioning from Changes
Sequence DiagramssequenceDiagram
participant JavaApp
participant NativeLib
participant macOSKeychain
JavaApp->>NativeLib: storePassword(service, key, password)
NativeLib->>macOSKeychain: SecItemCopyMatching (find existing password)
alt Password Exists
NativeLib->>macOSKeychain: SecItemUpdate (update password)
else Password Not Found
NativeLib->>macOSKeychain: SecItemAdd (add new password)
end
macOSKeychain-->>NativeLib: Status
NativeLib-->>JavaApp: Status
sequenceDiagram
participant JavaApp
participant NativeLib
participant macOSKeychain
JavaApp->>NativeLib: loadPassword(service, key)
NativeLib->>macOSKeychain: SecItemCopyMatching (retrieve password)
macOSKeychain-->>NativeLib: Password Data
NativeLib-->>JavaApp: Password Data
sequenceDiagram
participant JavaApp
participant NativeLib
participant macOSKeychain
JavaApp->>NativeLib: deletePassword(service, key)
NativeLib->>macOSKeychain: SecItemCopyMatching (find password)
NativeLib->>macOSKeychain: SecItemDelete (delete password)
macOSKeychain-->>NativeLib: Status
NativeLib-->>JavaApp: Status
Poem
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m (1 hunks)
Additional comments not posted (1)
src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m (1)
10-10: Ensure that the necessary headers are included for the newSecItemfunctions.Verification successful
The function
Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_storePasswordhas been fully extracted and reviewed. It includes proper memory management by releasing the byte arrays and handles different outcomes of the keychain operations (SecItemCopyMatching,SecItemUpdate, andSecItemAdd). The necessary headers are included, and the function appears to be implemented correctly.
- The header
<Security/Security.h>is included.- The function handles both updating existing keychain items and adding new ones.
- Proper memory management is ensured by releasing the byte arrays.
- Error handling is implemented with logging for unexpected statuses.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all necessary headers for SecItem functions are included. # Test: Search for the inclusion of Security/Security.h in the file. rg --type objc --files-with-matches '#import <Security/Security.h>' src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.mLength of output: 205
Script:
#!/bin/bash # Description: Extract the Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_storePassword function for review. ast-grep --lang objc --pattern 'JNIEXPORT $_ Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_storePassword($_) { $$$ }' src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.mLength of output: 312
Script:
#!/bin/bash # Description: Extract the Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_storePassword function for review. rg --type objc --context 20 'JNIEXPORT.*Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_storePassword' src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.mLength of output: 1700
Script:
#!/bin/bash # Description: Extract more lines of the Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_storePassword function for a complete review. rg --type objc --context 40 'JNIEXPORT.*Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_storePassword' src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.mLength of output: 2710
Script:
#!/bin/bash # Description: Extract the remaining lines of the Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_storePassword function for a complete review. rg --type objc --context 60 'JNIEXPORT.*Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_storePassword' src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.mLength of output: 3834
Comment on lines
105
to
110
| // find existing: | ||
| NSDictionary *searchAttributes = @{ | ||
| (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, | ||
| (__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding], | ||
| (__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding], | ||
| (__bridge id)kSecMatchLimit: (__bridge id)kSecMatchLimitOne |
There was a problem hiding this comment.
Refactor to improve error handling and reduce code duplication.
- if (status == errSecSuccess && result != NULL) {
- status = SecItemDelete((__bridge CFDictionaryRef)searchAttributes);
- } else if (status != errSecItemNotFound) {
- NSLog(@"Error deleting item from keychain. Status code: %d", (int)status);
- }
+ switch (status) {
+ case errSecSuccess:
+ if (result != NULL) {
+ status = SecItemDelete((__bridge CFDictionaryRef)searchAttributes);
+ }
+ break;
+ default:
+ NSLog(@"Error deleting item from keychain. Status code: %d", (int)status);
+ break;
+ }Also applies to: 116-122
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| // find existing: | |
| NSDictionary *searchAttributes = @{ | |
| (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, | |
| (__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecMatchLimit: (__bridge id)kSecMatchLimitOne | |
| // find existing: | |
| NSDictionary *searchAttributes = @{ | |
| (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, | |
| (__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecMatchLimit: (__bridge id)kSecMatchLimitOne | |
| }; | |
| switch (status) { | |
| case errSecSuccess: | |
| if (result != NULL) { | |
| status = SecItemDelete((__bridge CFDictionaryRef)searchAttributes); | |
| } | |
| break; | |
| default: | |
| NSLog(@"Error deleting item from keychain. Status code: %d", (int)status); | |
| break; | |
| } |
Comment on lines
20
to
27
| NSDictionary *searchAttributes = @{ | ||
| (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, | ||
| (__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding], | ||
| (__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding], | ||
| (__bridge id)kSecReturnAttributes: @YES, | ||
| (__bridge id)kSecReturnData: @YES, | ||
| (__bridge id)kSecMatchLimit: (__bridge id)kSecMatchLimitOne | ||
| }; |
There was a problem hiding this comment.
Refactor to improve error handling and reduce code duplication.
- if (status == errSecSuccess && result != NULL) {
- // update existing:
- NSDictionary *changeAttributes = @{
- (__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length]
- };
- status = SecItemUpdate((__bridge CFDictionaryRef)searchAttributes, (__bridge CFDictionaryRef)changeAttributes);
- } else if (status == errSecItemNotFound) {
- // add new:
- NSDictionary *keychainAttributes = @{
- (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword,
- (__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding],
- (__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding],
- (__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length]
- };
- status = SecItemAdd((__bridge CFDictionaryRef)keychainAttributes, NULL);
- } else {
- NSLog(@"Error storing item in keychain. Status code: %d", (int)status);
- }
+ switch (status) {
+ case errSecSuccess:
+ if (result != NULL) {
+ NSDictionary *changeAttributes = @{(__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length]};
+ status = SecItemUpdate((__bridge CFDictionaryRef)searchAttributes, (__bridge CFDictionaryRef)changeAttributes);
+ }
+ break;
+ case errSecItemNotFound:
+ NSDictionary *keychainAttributes = @{
+ (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword,
+ (__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding],
+ (__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding],
+ (__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length]
+ };
+ status = SecItemAdd((__bridge CFDictionaryRef)keychainAttributes, NULL);
+ break;
+ default:
+ NSLog(@"Error storing item in keychain. Status code: %d", (int)status);
+ break;
+ }Also applies to: 32-40, 41-49, 51-53
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| NSDictionary *searchAttributes = @{ | |
| (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, | |
| (__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecReturnAttributes: @YES, | |
| (__bridge id)kSecReturnData: @YES, | |
| (__bridge id)kSecMatchLimit: (__bridge id)kSecMatchLimitOne | |
| }; | |
| NSDictionary *searchAttributes = @{ | |
| (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, | |
| (__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecReturnAttributes: @YES, | |
| (__bridge id)kSecReturnData: @YES, | |
| (__bridge id)kSecMatchLimit: (__bridge id)kSecMatchLimitOne | |
| }; | |
| switch (status) { | |
| case errSecSuccess: | |
| if (result != NULL) { | |
| NSDictionary *changeAttributes = @{(__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length]}; | |
| status = SecItemUpdate((__bridge CFDictionaryRef)searchAttributes, (__bridge CFDictionaryRef)changeAttributes); | |
| } | |
| break; | |
| case errSecItemNotFound: | |
| NSDictionary *keychainAttributes = @{ | |
| (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, | |
| (__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length] | |
| }; | |
| status = SecItemAdd((__bridge CFDictionaryRef)keychainAttributes, NULL); | |
| break; | |
| default: | |
| NSLog(@"Error storing item in keychain. Status code: %d", (int)status); | |
| break; | |
| } |
Comment on lines
66
to
74
| // Create a dictionary of search attributes to find the item in the keychain | ||
| NSDictionary *searchAttributes = @{ | ||
| (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, | ||
| (__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding], | ||
| (__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding], | ||
| (__bridge id)kSecReturnAttributes: @YES, | ||
| (__bridge id)kSecReturnData: @YES, | ||
| (__bridge id)kSecMatchLimit: (__bridge id)kSecMatchLimitOne | ||
| }; |
There was a problem hiding this comment.
Ensure proper memory management and error handling.
- if (status == errSecSuccess && result != NULL) {
- NSDictionary *attributes = (__bridge_transfer NSDictionary *)result;
- NSData *passwordData = attributes[(__bridge id)kSecValueData];
- NSUInteger pwLen = [passwordData length];
- const void *pwBytes = [passwordData bytes];
- jbyteArray password = (*env)->NewByteArray(env, (int)pwLen);
- (*env)->SetByteArrayRegion(env, password, 0, (int)pwLen, (jbyte *)pwBytes);
- return password;
- } else if (status != errSecItemNotFound) {
- NSLog(@"Error retrieving item from keychain. Status code: %d", (int)status);
- }
+ switch (status) {
+ case errSecSuccess:
+ if (result != NULL) {
+ NSDictionary *attributes = (__bridge_transfer NSDictionary *)result;
+ NSData *passwordData = attributes[(__bridge id)kSecValueData];
+ NSUInteger pwLen = [passwordData length];
+ const void *pwBytes = [passwordData bytes];
+ jbyteArray password = (*env)->NewByteArray(env, (int)pwLen);
+ (*env)->SetByteArrayRegion(env, password, 0, (int)pwLen, (jbyte *)pwBytes);
+ return password;
+ }
+ break;
+ default:
+ NSLog(@"Error retrieving item from keychain. Status code: %d", (int)status);
+ return NULL;
+ }Also applies to: 82-94
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| // Create a dictionary of search attributes to find the item in the keychain | |
| NSDictionary *searchAttributes = @{ | |
| (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, | |
| (__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecReturnAttributes: @YES, | |
| (__bridge id)kSecReturnData: @YES, | |
| (__bridge id)kSecMatchLimit: (__bridge id)kSecMatchLimitOne | |
| }; | |
| // Create a dictionary of search attributes to find the item in the keychain | |
| NSDictionary *searchAttributes = @{ | |
| (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, | |
| (__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecReturnAttributes: @YES, | |
| (__bridge id)kSecReturnData: @YES, | |
| (__bridge id)kSecMatchLimit: (__bridge id)kSecMatchLimitOne | |
| }; | |
| switch (status) { | |
| case errSecSuccess: | |
| if (result != NULL) { | |
| NSDictionary *attributes = (__bridge_transfer NSDictionary *)result; | |
| NSData *passwordData = attributes[(__bridge id)kSecValueData]; | |
| NSUInteger pwLen = [passwordData length]; | |
| const void *pwBytes = [passwordData bytes]; | |
| jbyteArray password = (*env)->NewByteArray(env, (int)pwLen); | |
| (*env)->SetByteArrayRegion(env, password, 0, (int)pwLen, (jbyte *)pwBytes); | |
| return password; | |
| } | |
| break; | |
| default: | |
| NSLog(@"Error retrieving item from keychain. Status code: %d", (int)status); | |
| return NULL; | |
| } |
|
The force push was due to a rebase. A pulled the latest commits from devel and put my changes on top. |
|
BTW, existing keychain entries do work flawlessly and get updated etc. |
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m (1 hunks)
Additional comments not posted (1)
src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m (1)
10-58: Ensure proper memory management when handling JNI environment pointers.Verification successful
Ensure proper memory management when handling JNI environment pointers.
The search results confirm that the
GetByteArrayElementsandReleaseByteArrayElementsmethods are used correctly within theJava_org_cryptomator_macos_keychain_MacKeychain_00024Native_storePasswordfunction. TheGetByteArrayElementsmethod is used to obtain pointers to the byte arrays, and theReleaseByteArrayElementsmethod is used to release these pointers, ensuring proper memory management.
GetByteArrayElementsis used to obtain pointers to theservice,key, andpasswordbyte arrays.ReleaseByteArrayElementsis used to release these pointers after they are no longer needed.This confirms that the JNI memory management practices are correctly implemented in the function.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify memory management practices in JNI functions. # Test: Search for JNI memory management best practices. Expect: Proper use of Get/Release methods. rg --type c 'GetByteArrayElements' 'ReleaseByteArrayElements'Length of output: 124
Script:
#!/bin/bash # Description: Verify memory management practices in JNI functions. # Test: Search for JNI memory management best practices. Expect: Proper use of Get/Release methods. rg --type c 'GetByteArrayElements' rg --type c 'ReleaseByteArrayElements'Length of output: 73
Script:
#!/bin/bash # Description: Verify memory management practices in the specific JNI function. # Test: Search for the specific JNI function and check for proper use of Get/Release methods. rg --type c 'JNIEXPORT jint JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_storePassword' rg --type c 'GetByteArrayElements' src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m rg --type c 'ReleaseByteArrayElements' src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.mLength of output: 1433
Comment on lines
104
to
126
|
|
||
| // find existing: | ||
| NSDictionary *searchAttributes = @{ | ||
| (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, | ||
| (__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding], | ||
| (__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding], | ||
| (__bridge id)kSecMatchLimit: (__bridge id)kSecMatchLimitOne | ||
| }; | ||
|
|
||
| CFDictionaryRef result = NULL; | ||
| OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)searchAttributes, (CFTypeRef *)&result); | ||
|
|
||
| if (status == errSecSuccess && result != NULL) { | ||
|
|
||
| status = SecItemDelete((__bridge CFDictionaryRef)searchAttributes); | ||
|
|
||
| } else if (status != errSecItemNotFound) { | ||
| NSLog(@"Error deleting item from keychain. Status code: %d", (int)status); | ||
| } | ||
|
|
||
| (*env)->ReleaseByteArrayElements(env, service, serviceStr, JNI_ABORT); | ||
| (*env)->ReleaseByteArrayElements(env, key, keyStr, JNI_ABORT); | ||
| if (itemRef) { | ||
| CFRelease(itemRef); | ||
| } | ||
|
|
There was a problem hiding this comment.
Enhance error messages to provide more context in deletion operations.
- NSLog(@"Error deleting item from keychain. Status code: %d", (int)status);
+ NSLog(@"Error deleting item from keychain. Status code: %d. Ensure the item exists and is accessible.", (int)status);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| // find existing: | |
| NSDictionary *searchAttributes = @{ | |
| (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, | |
| (__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecMatchLimit: (__bridge id)kSecMatchLimitOne | |
| }; | |
| CFDictionaryRef result = NULL; | |
| OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)searchAttributes, (CFTypeRef *)&result); | |
| if (status == errSecSuccess && result != NULL) { | |
| status = SecItemDelete((__bridge CFDictionaryRef)searchAttributes); | |
| } else if (status != errSecItemNotFound) { | |
| NSLog(@"Error deleting item from keychain. Status code: %d", (int)status); | |
| } | |
| (*env)->ReleaseByteArrayElements(env, service, serviceStr, JNI_ABORT); | |
| (*env)->ReleaseByteArrayElements(env, key, keyStr, JNI_ABORT); | |
| if (itemRef) { | |
| CFRelease(itemRef); | |
| } | |
| // find existing: | |
| NSDictionary *searchAttributes = @{ | |
| (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, | |
| (__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecMatchLimit: (__bridge id)kSecMatchLimitOne | |
| }; | |
| CFDictionaryRef result = NULL; | |
| OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)searchAttributes, (CFTypeRef *)&result); | |
| if (status == errSecSuccess && result != NULL) { | |
| status = SecItemDelete((__bridge CFDictionaryRef)searchAttributes); | |
| } else if (status != errSecItemNotFound) { | |
| NSLog(@"Error deleting item from keychain. Status code: %d. Ensure the item exists and is accessible.", (int)status); | |
| } | |
| (*env)->ReleaseByteArrayElements(env, service, serviceStr, JNI_ABORT); | |
| (*env)->ReleaseByteArrayElements(env, key, keyStr, JNI_ABORT); | |
Comment on lines
66
to
98
| // Create a dictionary of search attributes to find the item in the keychain | ||
| NSDictionary *searchAttributes = @{ | ||
| (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, | ||
| (__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding], | ||
| (__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding], | ||
| (__bridge id)kSecReturnAttributes: @YES, | ||
| (__bridge id)kSecReturnData: @YES, | ||
| (__bridge id)kSecMatchLimit: (__bridge id)kSecMatchLimitOne | ||
| }; | ||
|
|
||
| CFDictionaryRef result = NULL; | ||
| OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)searchAttributes, (CFTypeRef *)&result); | ||
|
|
||
| (*env)->ReleaseByteArrayElements(env, service, serviceStr, JNI_ABORT); | ||
| (*env)->ReleaseByteArrayElements(env, key, keyStr, JNI_ABORT); | ||
| if (pwStr) { | ||
| SecKeychainItemFreeContent(NULL, pwStr); | ||
|
|
||
| if (status == errSecSuccess && result != NULL) { | ||
| NSDictionary *attributes = (__bridge_transfer NSDictionary *)result; | ||
| NSData *passwordData = attributes[(__bridge id)kSecValueData]; | ||
|
|
||
| NSUInteger pwLen = [passwordData length]; | ||
| const void *pwBytes = [passwordData bytes]; | ||
|
|
||
| jbyteArray password = (*env)->NewByteArray(env, (int)pwLen); | ||
| (*env)->SetByteArrayRegion(env, password, 0, (int)pwLen, (jbyte *)pwBytes); | ||
|
|
||
| return password; | ||
|
|
||
| } else if (status != errSecItemNotFound) { | ||
| NSLog(@"Error retrieving item from keychain. Status code: %d", (int)status); | ||
| } | ||
| return result; | ||
|
|
||
| return NULL; // empty |
There was a problem hiding this comment.
Refactor to simplify error handling in password retrieval.
- } else if (status != errSecItemNotFound) {
- NSLog(@"Error retrieving item from keychain. Status code: %d", (int)status);
- }
+ } else {
+ NSLog(@"Error retrieving item from keychain. Status code: %d", (int)status);
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| // Create a dictionary of search attributes to find the item in the keychain | |
| NSDictionary *searchAttributes = @{ | |
| (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, | |
| (__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecReturnAttributes: @YES, | |
| (__bridge id)kSecReturnData: @YES, | |
| (__bridge id)kSecMatchLimit: (__bridge id)kSecMatchLimitOne | |
| }; | |
| CFDictionaryRef result = NULL; | |
| OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)searchAttributes, (CFTypeRef *)&result); | |
| (*env)->ReleaseByteArrayElements(env, service, serviceStr, JNI_ABORT); | |
| (*env)->ReleaseByteArrayElements(env, key, keyStr, JNI_ABORT); | |
| if (pwStr) { | |
| SecKeychainItemFreeContent(NULL, pwStr); | |
| if (status == errSecSuccess && result != NULL) { | |
| NSDictionary *attributes = (__bridge_transfer NSDictionary *)result; | |
| NSData *passwordData = attributes[(__bridge id)kSecValueData]; | |
| NSUInteger pwLen = [passwordData length]; | |
| const void *pwBytes = [passwordData bytes]; | |
| jbyteArray password = (*env)->NewByteArray(env, (int)pwLen); | |
| (*env)->SetByteArrayRegion(env, password, 0, (int)pwLen, (jbyte *)pwBytes); | |
| return password; | |
| } else if (status != errSecItemNotFound) { | |
| NSLog(@"Error retrieving item from keychain. Status code: %d", (int)status); | |
| } | |
| return result; | |
| return NULL; // empty | |
| // Create a dictionary of search attributes to find the item in the keychain | |
| NSDictionary *searchAttributes = @{ | |
| (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, | |
| (__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecReturnAttributes: @YES, | |
| (__bridge id)kSecReturnData: @YES, | |
| (__bridge id)kSecMatchLimit: (__bridge id)kSecMatchLimitOne | |
| }; | |
| CFDictionaryRef result = NULL; | |
| OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)searchAttributes, (CFTypeRef *)&result); | |
| (*env)->ReleaseByteArrayElements(env, service, serviceStr, JNI_ABORT); | |
| (*env)->ReleaseByteArrayElements(env, key, keyStr, JNI_ABORT); | |
| if (status == errSecSuccess && result != NULL) { | |
| NSDictionary *attributes = (__bridge_transfer NSDictionary *)result; | |
| NSData *passwordData = attributes[(__bridge id)kSecValueData]; | |
| NSUInteger pwLen = [passwordData length]; | |
| const void *pwBytes = [passwordData bytes]; | |
| jbyteArray password = (*env)->NewByteArray(env, (int)pwLen); | |
| (*env)->SetByteArrayRegion(env, password, 0, (int)pwLen, (jbyte *)pwBytes); | |
| return password; | |
| } else { | |
| NSLog(@"Error retrieving item from keychain. Status code: %d", (int)status); | |
| } | |
| return NULL; // empty |
tobihagemann
approved these changes
Jul 1, 2024
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m (1 hunks)
Additional comments not posted (1)
src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m (1)
103-103: Enhance error messages to provide more context in deletion operations.The error message should provide more context to assist in troubleshooting.
- NSLog(@"Error deleting item from keychain. Status code: %d", (int)status); + NSLog(@"Error deleting item from keychain. Status code: %d. Ensure the item exists and is accessible.", (int)status);
Comment on lines
+98
to
104
| OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)query, (CFTypeRef *)&result); | ||
| if (status == errSecSuccess && result != NULL) { | ||
| // delete: | ||
| status = SecItemDelete((__bridge CFDictionaryRef)query); | ||
| } else if (status != errSecItemNotFound) { | ||
| NSLog(@"Error deleting item from keychain. Status code: %d", (int)status); | ||
| } |
There was a problem hiding this comment.
Refactor to improve error handling and reduce code duplication.
The current code can be refactored to use a switch statement for better readability and maintainability.
- if (status == errSecSuccess && result != NULL) {
- // delete:
- status = SecItemDelete((__bridge CFDictionaryRef)query);
- } else if (status != errSecItemNotFound) {
- NSLog(@"Error deleting item from keychain. Status code: %d", (int)status);
- }
+ switch (status) {
+ case errSecSuccess:
+ if (result != NULL) {
+ status = SecItemDelete((__bridge CFDictionaryRef)query);
+ }
+ break;
+ default:
+ NSLog(@"Error deleting item from keychain. Status code: %d", (int)status);
+ break;
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)query, (CFTypeRef *)&result); | |
| if (status == errSecSuccess && result != NULL) { | |
| // delete: | |
| status = SecItemDelete((__bridge CFDictionaryRef)query); | |
| } else if (status != errSecItemNotFound) { | |
| NSLog(@"Error deleting item from keychain. Status code: %d", (int)status); | |
| } | |
| OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)query, (CFTypeRef *)&result); | |
| switch (status) { | |
| case errSecSuccess: | |
| if (result != NULL) { | |
| status = SecItemDelete((__bridge CFDictionaryRef)query); | |
| } | |
| break; | |
| default: | |
| NSLog(@"Error deleting item from keychain. Status code: %d", (int)status); | |
| break; | |
| } |
Comment on lines
+69
to
79
| OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)query, (CFTypeRef *)&result); | ||
| jbyteArray password = NULL; | ||
| if (status == errSecSuccess && result != NULL) { | ||
| // retrieve password: | ||
| NSDictionary *attributes = (__bridge_transfer NSDictionary *)result; | ||
| NSData *passwordData = attributes[(__bridge id)kSecValueData]; | ||
| password = (*env)->NewByteArray(env, (int)passwordData.length); | ||
| (*env)->SetByteArrayRegion(env, password, 0, (int)passwordData.length, (jbyte *)passwordData.bytes); | ||
| } else if (status != errSecItemNotFound) { | ||
| NSLog(@"Error retrieving item from keychain. Status code: %d", (int)status); | ||
| } |
There was a problem hiding this comment.
Refactor to improve error handling and reduce code duplication.
The current code can be refactored to use a switch statement for better readability and maintainability.
- if (status == errSecSuccess && result != NULL) {
- // retrieve password:
- NSDictionary *attributes = (__bridge_transfer NSDictionary *)result;
- NSData *passwordData = attributes[(__bridge id)kSecValueData];
- password = (*env)->NewByteArray(env, (int)passwordData.length);
- (*env)->SetByteArrayRegion(env, password, 0, (int)passwordData.length, (jbyte *)passwordData.bytes);
- } else if (status != errSecItemNotFound) {
- NSLog(@"Error retrieving item from keychain. Status code: %d", (int)status);
- }
+ switch (status) {
+ case errSecSuccess:
+ if (result != NULL) {
+ NSDictionary *attributes = (__bridge_transfer NSDictionary *)result;
+ NSData *passwordData = attributes[(__bridge id)kSecValueData];
+ password = (*env)->NewByteArray(env, (int)passwordData.length);
+ (*env)->SetByteArrayRegion(env, password, 0, (int)passwordData.length, (jbyte *)passwordData.bytes);
+ }
+ break;
+ default:
+ NSLog(@"Error retrieving item from keychain. Status code: %d", (int)status);
+ break;
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)query, (CFTypeRef *)&result); | |
| jbyteArray password = NULL; | |
| if (status == errSecSuccess && result != NULL) { | |
| // retrieve password: | |
| NSDictionary *attributes = (__bridge_transfer NSDictionary *)result; | |
| NSData *passwordData = attributes[(__bridge id)kSecValueData]; | |
| password = (*env)->NewByteArray(env, (int)passwordData.length); | |
| (*env)->SetByteArrayRegion(env, password, 0, (int)passwordData.length, (jbyte *)passwordData.bytes); | |
| } else if (status != errSecItemNotFound) { | |
| NSLog(@"Error retrieving item from keychain. Status code: %d", (int)status); | |
| } | |
| OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)query, (CFTypeRef *)&result); | |
| jbyteArray password = NULL; | |
| switch (status) { | |
| case errSecSuccess: | |
| if (result != NULL) { | |
| NSDictionary *attributes = (__bridge_transfer NSDictionary *)result; | |
| NSData *passwordData = attributes[(__bridge id)kSecValueData]; | |
| password = (*env)->NewByteArray(env, (int)passwordData.length); | |
| (*env)->SetByteArrayRegion(env, password, 0, (int)passwordData.length, (jbyte *)passwordData.bytes); | |
| } | |
| break; | |
| default: | |
| NSLog(@"Error retrieving item from keychain. Status code: %d", (int)status); | |
| break; | |
| } |
Comment on lines
+29
to
47
| OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)query, (CFTypeRef *)&result); | ||
| if (status == errSecSuccess && result != NULL) { | ||
| // update existing: | ||
| status = SecKeychainItemModifyAttributesAndData( | ||
| itemRef, // the item reference | ||
| NULL, // no change to attributes | ||
| pwLen, // length of password | ||
| pwStr // pointer to password data | ||
| ); | ||
| NSDictionary *attributesToUpdate = @{ | ||
| (__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length] | ||
| }; | ||
| status = SecItemUpdate((__bridge CFDictionaryRef)query, (__bridge CFDictionaryRef)attributesToUpdate); | ||
| } else if (status == errSecItemNotFound) { | ||
| // add new: | ||
| status = SecKeychainAddGenericPassword( | ||
| NULL, // default keychain | ||
| serviceLen, // length of service name | ||
| (char *)serviceStr, // service name | ||
| keyLen, // length of account name | ||
| (char *)keyStr, // account name | ||
| pwLen, // length of password | ||
| pwStr, // pointer to password data | ||
| NULL // the item reference | ||
| ); | ||
| NSDictionary *attributes = @{ | ||
| (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, | ||
| (__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding], | ||
| (__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding], | ||
| (__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length] | ||
| }; | ||
| status = SecItemAdd((__bridge CFDictionaryRef)attributes, NULL); | ||
| } else { | ||
| NSLog(@"Error storing item in keychain. Status code: %d", (int)status); | ||
| } |
There was a problem hiding this comment.
Refactor to improve error handling and reduce code duplication.
The current code can be refactored to use a switch statement for better readability and maintainability.
- if (status == errSecSuccess && result != NULL) {
- // update existing:
- NSDictionary *attributesToUpdate = @{
- (__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length]
- };
- status = SecItemUpdate((__bridge CFDictionaryRef)query, (__bridge CFDictionaryRef)attributesToUpdate);
- } else if (status == errSecItemNotFound) {
- // add new:
- NSDictionary *attributes = @{
- (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword,
- (__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding],
- (__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding],
- (__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length]
- };
- status = SecItemAdd((__bridge CFDictionaryRef)attributes, NULL);
- } else {
- NSLog(@"Error storing item in keychain. Status code: %d", (int)status);
- }
+ switch (status) {
+ case errSecSuccess:
+ if (result != NULL) {
+ NSDictionary *attributesToUpdate = @{(__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length]};
+ status = SecItemUpdate((__bridge CFDictionaryRef)query, (__bridge CFDictionaryRef)attributesToUpdate);
+ }
+ break;
+ case errSecItemNotFound:
+ NSDictionary *attributes = @{
+ (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword,
+ (__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding],
+ (__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding],
+ (__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length]
+ };
+ status = SecItemAdd((__bridge CFDictionaryRef)attributes, NULL);
+ break;
+ default:
+ NSLog(@"Error storing item in keychain. Status code: %d", (int)status);
+ break;
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)query, (CFTypeRef *)&result); | |
| if (status == errSecSuccess && result != NULL) { | |
| // update existing: | |
| status = SecKeychainItemModifyAttributesAndData( | |
| itemRef, // the item reference | |
| NULL, // no change to attributes | |
| pwLen, // length of password | |
| pwStr // pointer to password data | |
| ); | |
| NSDictionary *attributesToUpdate = @{ | |
| (__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length] | |
| }; | |
| status = SecItemUpdate((__bridge CFDictionaryRef)query, (__bridge CFDictionaryRef)attributesToUpdate); | |
| } else if (status == errSecItemNotFound) { | |
| // add new: | |
| status = SecKeychainAddGenericPassword( | |
| NULL, // default keychain | |
| serviceLen, // length of service name | |
| (char *)serviceStr, // service name | |
| keyLen, // length of account name | |
| (char *)keyStr, // account name | |
| pwLen, // length of password | |
| pwStr, // pointer to password data | |
| NULL // the item reference | |
| ); | |
| NSDictionary *attributes = @{ | |
| (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, | |
| (__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length] | |
| }; | |
| status = SecItemAdd((__bridge CFDictionaryRef)attributes, NULL); | |
| } else { | |
| NSLog(@"Error storing item in keychain. Status code: %d", (int)status); | |
| } | |
| OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)query, (CFTypeRef *)&result); | |
| switch (status) { | |
| case errSecSuccess: | |
| if (result != NULL) { | |
| NSDictionary *attributesToUpdate = @{(__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length]}; | |
| status = SecItemUpdate((__bridge CFDictionaryRef)query, (__bridge CFDictionaryRef)attributesToUpdate); | |
| } | |
| break; | |
| case errSecItemNotFound: | |
| NSDictionary *attributes = @{ | |
| (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, | |
| (__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding], | |
| (__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length] | |
| }; | |
| status = SecItemAdd((__bridge CFDictionaryRef)attributes, NULL); | |
| break; | |
| default: | |
| NSLog(@"Error storing item in keychain. Status code: %d", (int)status); | |
| break; | |
| } |
No need to worry @tobihagemann. 🙂 Thanks for the review and the improvements! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As the title says, this introduces
SecItem, without breaking anything. So this PR is ready for review.Also,
SecItemis the base for having keychain access control and Touch ID support (which is not part of this PR, see cryptomator/cryptomator#3311 (comment)).