-
Notifications
You must be signed in to change notification settings - Fork 5.9k
[unlock_keychain] fix crash when not having a default keychain #29173
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
Conversation
Hi @revolter |
Hi @mollyIV |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this is way beyond me 🙈
But the changes themselves seem pretty safe, and the description it quite thorough (except this outdated part).
As such, I'll approve this, and ask for help from more knowledgeable people.
Thanks for the review, @revolter. I updated the outdated PR description. |
UI.message("Reading existing default keychain") | ||
Actions.lane_context[Actions::SharedValues::ORIGINAL_DEFAULT_KEYCHAIN] = Fastlane::Actions.sh("security default-keychain").strip | ||
rescue => e | ||
raise unless e.message.include?("security: SecKeychainCopyDefault: A default keychain could not be found.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there another way we could recognize this error besides checking the error message? Maybe the status code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mollyIV
That would've been nice, but when I print the exception it just prints this
error : Exit status of command 'security default-keychain' was 1 instead of 0.
security: SecKeychainCopyDefault: A default keychain could not be found.
I don't know if "1" is a unique error code for this case and therefore I added a check for the error message instead. Maybe you know if we can just check for error code? Also, I don't know how to actually get that error code from the e
object in begin-rescue
.
Here is some documentation on what I believe the security default-keychain
command calls:
https://developer.apple.com/documentation/security/seckeychaincopydefault(_:)?language=objc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks for the explanation. Not sure if the status code 1 is the right on, sounds pretty generic. I think checking error description in this case will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mollyIV I agree. Thanks a lot for the review!
This PR makes it possible to run
unlock_keychain
withadd_to_search_list: :replace
without an existing default keychain.Checklist
bundle exec rspec
from the root directory to see all new and existing tests passbundle exec rubocop -a
to ensure the code style is validci/circleci
builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)Motivation and Context
It is currently not possible to run
unlock_keychain
withadd_to_search_list: :replace
if you don't have an existing default keychain. Even when also providingset_default: true
. For us this currently means that we have to first runcreate_keychain
withdefault_keychain: true
in order to create a temporary default dummy keychain and then unlock the actual keychain that we want to use withunlock_keychain
and then finally remove the unused dummy keychain withdelete_keychain
.Description
This PR wraps the read of the original/existing default keychain with a
begin-rescue
in order to make sure that it doesn't crash when no default keychain is set.I can add that the reading of the original default keychain is stored in
SharedValues::ORIGINAL_DEFAULT_KEYCHAIN
but that it is undocumented for theunlock_keychain
action (it is however documented for thecreate_keychain
task https://docs.fastlane.tools/actions/create_keychain/#create_keychain).I tried adding some tests for this (inside
fastlane/spec/actions_specs/unlock_keychain_spec.rb
) but I'm a rookie Ruby developer so I didn't manage to make it work because the test wouldn't fail even though I didn't have a default keychain set.Finally, I can add that a lot of tests (
bundle exec rspec
) fail if you don't have a default keychain set. Also in master of the fastlane repo without my changes from this PR.Testing Steps
This can be tested by removing the default keychain on your machine:
and then run a simple lane that creates and unlocks a keychain:
Note: To set your login keychain as the default again, run
security default-keychain -s <default keychain from the first command you ran>
, probably something likesecurity default-keychain -s /Users/<your-username>/Library/Keychains/login.keychain-db
.If there is no default keychain, the output will now look like this:
Instead of failing with: