Skip to content
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

Fix keychain key bug #237

Merged
merged 14 commits into from
Feb 6, 2024
Merged

Fix keychain key bug #237

merged 14 commits into from
Feb 6, 2024

Conversation

mdmathias
Copy link
Collaborator

This PR addresses a bug where the key attributes used to query the keychain were not being added correctly, and where therefore unused. The mistake was that the key names were assumed to match the static constants.

Fixes #236.

This change fixes a bug where the String representation of these keys were supplied to the keychain query as opposed to the actual key names.
@mdmathias
Copy link
Collaborator Author

Hi @olvrlrnz please take a look and help us to make sure that this change meets the need. Thanks!

@mdmathias mdmathias marked this pull request as ready for review January 29, 2024 22:47
@olvrlrnz
Copy link

Just tried it in our code base, LGTM
image

Also update use of kSecAttrAccessible on macOS to only occur if also using kSecUseDataProtectionKeychain per Apple docs: https://developer.apple.com/documentation/security/ksecattraccessible\?language\=objc
The other existing methods either take an accessibility param or an optional param.
@mdmathias
Copy link
Collaborator Author

@olvrlrnz Sorry for bugging you again, but I made some changes since you last looked. Would you mind double checking for me? 🙏

@mdmathias mdmathias merged commit d7b2c1f into master Feb 6, 2024
5 checks passed
@mdmathias mdmathias deleted the mdmathias/keychain-keys branch February 6, 2024 21:33
@olvrlrnz
Copy link

Thanks for fixing this. Would it be possible to create a 4.0.1 release so we can update the package in Xcode?

@mdmathias
Copy link
Collaborator Author

Absolutely. We need to figure out what version we call this. In any case, we will be making a new release to include a privacy manifest very shortly. I think that will come out in the next few weeks. Stay tuned for that, and sorry for the delay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KeychainAttribute does not work as expected
3 participants