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

update keychain access level #4172

Merged
merged 4 commits into from
Oct 25, 2019
Merged

update keychain access level #4172

merged 4 commits into from
Oct 25, 2019

Conversation

charlotteliang
Copy link
Contributor

#4167 user reports getting IID becomes nil when a token exists. We suspect this is due to a security issue while keychain access level is set to kSecAttrAccessibleAlwaysThisDeviceOnly that is deprecated in iOS 13. Neitherless we should update it to kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly since app only works in foreground so this is the proper access.

Also remove a parameter to set a optional access level for keychain cuz this parameter was never used.

Copy link
Contributor

@maksymmalyhin maksymmalyhin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
I would double check how the change will affect existing stored IID.

@@ -74,22 +74,19 @@ NS_ASSUME_NONNULL_BEGIN

/**
* Set the data for a given service and account with a specific accessibility. If
* accessibility is NULL we use `kSecAttrAccessibleAlwaysThisDeviceOnly` which
* accessibility is NULL we use `kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly` which
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the comment should be updated after removing accessibility parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

@charlotteliang
Copy link
Contributor Author

Yeah, I've tested with an existing keychain store and seems working well with settings updated.

We've updated keychain access in the past and haven't seen any regression issues.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.