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

[Core] Keychain migration #1203

Merged
merged 4 commits into from
Nov 1, 2023
Merged

Conversation

flypaper0
Copy link
Contributor

Description

tryMigrateAttrAccessible:

  • Check if Migration needed
  • Fetching old value
  • Deleting old value
  • Replacing with new value

Resolves # (issue)

How Has This Been Tested?

Due Dilligence

  • Breaking change
  • Requires a documentation update

@flypaper0 flypaper0 requested a review from llbartekll October 30, 2023 19:19
@flypaper0 flypaper0 temporarily deployed to internal October 30, 2023 19:19 — with GitHub Actions Inactive
@flypaper0 flypaper0 temporarily deployed to internal October 31, 2023 05:13 — with GitHub Actions Inactive
Comment on lines 125 to 126
var deleteQuery = buildBaseServiceQuery(for: key)
deleteQuery[kSecAttrAccessible] = kSecAttrAccessibleWhenUnlockedThisDeviceOnly
Copy link
Contributor

@llbartekll llbartekll Oct 31, 2023

Choose a reason for hiding this comment

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

if read failed for kSecAttrAccessibleWhenUnlockedThisDeviceOnly why delete won't?

@@ -55,7 +55,8 @@ public final class KeychainStorage: KeychainStorageProtocol {
case errSecSuccess:
return item as? Data
case errSecItemNotFound:
Copy link
Contributor

Choose a reason for hiding this comment

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

is it the error case we are getting when app is in background?

@flypaper0 flypaper0 temporarily deployed to internal October 31, 2023 13:42 — with GitHub Actions Inactive
@flypaper0 flypaper0 temporarily deployed to internal October 31, 2023 15:02 — with GitHub Actions Inactive
@flypaper0 flypaper0 temporarily deployed to internal October 31, 2023 16:52 — with GitHub Actions Inactive
@flypaper0 flypaper0 temporarily deployed to internal October 31, 2023 16:58 — with GitHub Actions Inactive
@flypaper0 flypaper0 requested a review from llbartekll October 31, 2023 17:19
Copy link
Contributor

@llbartekll llbartekll left a comment

Choose a reason for hiding this comment

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

tested and it worked as expected

@llbartekll llbartekll self-requested a review October 31, 2023 19:17
@flypaper0 flypaper0 merged commit 0491bad into develop Nov 1, 2023
@flypaper0 flypaper0 deleted the feature/keychain-attr-migration branch November 1, 2023 08:53
@flypaper0 flypaper0 mentioned this pull request Nov 1, 2023
2 tasks
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.

2 participants