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

@chliangGoogle
Copy link
Contributor

@chliangGoogle chliangGoogle commented Oct 25, 2019

#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

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

@maksymmalyhin maksymmalyhin Oct 25, 2019

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

@chliangGoogle chliangGoogle Oct 25, 2019

Choose a reason for hiding this comment

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

good catch!

@chliangGoogle
Copy link
Contributor Author

@chliangGoogle chliangGoogle commented Oct 25, 2019

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.

@chliangGoogle chliangGoogle merged commit 4d10f32 into master Oct 25, 2019
3 checks passed
@chliangGoogle chliangGoogle deleted the oct-fcm-keychain branch Oct 25, 2019
@firebase firebase locked and limited conversation to collaborators Nov 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants