-
Notifications
You must be signed in to change notification settings - Fork 46
Conversation
…ly instead of wiping entire keychain.
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 very much like the approach you have here.
In the cases where you are wiping both .account
and .database
, I would recommend either an .all
case, or just wiping the keychain as before.
Excellent work.
@@ -109,7 +109,7 @@ class BaseDataStore { | |||
}() | |||
|
|||
internal var loginsKey: String? { | |||
let key = "sqlcipher.key.logins.db" | |||
let key = KeychainKey.loginsKey.rawValue |
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.
😸
lockbox-ios/Common/AppDelegate.swift
Outdated
@@ -25,7 +25,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate { | |||
func application(_ application: UIApplication, willFinishLaunchingWithOptions | |||
launchOptions: [UIApplication.LaunchOptionsKey: Any]? = nil) -> Bool { | |||
if isFirstRun { | |||
KeychainWrapper.wipeKeychain() | |||
KeychainWrapper.sharedAppContainerKeychain.clearAllValues(for: .account) |
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.
IIUIC, we should be able to clear values for .database
here as well.
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.
Agreed
@@ -142,7 +142,7 @@ extension AccountStore { | |||
} | |||
|
|||
private func clear() { | |||
KeychainWrapper.wipeKeychain() | |||
keychainWrapper.clearAllValues(for: .account) |
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.
IIUC This is the point which fixes the bug.
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.
Yes 👍
lockbox-ios/Common/AppDelegate.swift
Outdated
@@ -107,7 +107,8 @@ extension AppDelegate { | |||
} else if previous > current { | |||
//this would mean a user had an upgraded test version and has now downgraded to a previous version | |||
//we should wipe keychain data to remove any possible abnormalities | |||
KeychainWrapper.wipeKeychain() | |||
KeychainWrapper.sharedAppContainerKeychain.clearAllValues(for: .account) | |||
KeychainWrapper.sharedAppContainerKeychain.clearAllValues(for: .database) |
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.
In time, will we move back to wipeKeychain()
?
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.
Yes. I like being explicit about the values for now, but I think in the future we could do this.
Agreed - I will add a |
…wise/lockwise-ios into lockwise-launch-issues-1190
Fixes #1190
To test: