Skip to content
This repository has been archived by the owner on Dec 14, 2021. It is now read-only.

Lockwise keychain launch issues #1192

Merged
merged 7 commits into from
Feb 12, 2020
Merged

Lockwise keychain launch issues #1192

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e8b1ae8
Increase bundle version to 1.7.2
kaylagalway Feb 7, 2020
06e5ada
Clear account values and local database values from keychain explicit…
kaylagalway Feb 10, 2020
94c21ff
Fix AccountStore tests to use new KeychainKey values
kaylagalway Feb 10, 2020
f7f3f42
Merge branch 'master' into lockwise-launch-issues-1190
kaylagalway Feb 11, 2020
f34a466
Add .all case for KeychainKey clearing
kaylagalway Feb 11, 2020
7598a12
Merge branch 'lockwise-launch-issues-1190' of github.com:mozilla-lock…
kaylagalway Feb 11, 2020
2cee759
initFxa() must be called on the main thread or tests crash
kaylagalway Feb 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CredentialProvider/Info.plist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
<key>CFBundlePackageType</key>
<string>XPC!</string>
<key>CFBundleShortVersionString</key>
<string>1.7.1</string>
<string>1.7.2</string>
<key>CFBundleVersion</key>
<string>1</string>
<key>MozDevelopmentTeam</key>
Expand Down
16 changes: 16 additions & 0 deletions Shared/Common/Extensions/KeychainWrapper+.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,20 @@ public extension KeychainWrapper {
}
}
}

internal func clearAllValues(for valueType: KeychainKey.valueType) {
//This does not wipe entire keychain, but rather clears values defined in KeychainKey
switch valueType {
//Removes sync account values: email, displayName, accountJSON, avatarURL
case .account:
for identifier in KeychainKey.accountValues {
_ = self.removeObject(forKey: identifier.rawValue)
}
//Removes database encryption values: salt, key
case .database:
for identifier in KeychainKey.databaseValues {
_ = self.removeObject(forKey: identifier.rawValue)
}
}
}
}
9 changes: 8 additions & 1 deletion Shared/Common/Resources/BaseConstants.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,15 @@ enum KeychainKey: String {
case avatarURL
case accountJSON
case salt = "sqlcipher.key.logins.salt"
case loginsKey = "sqlcipher.key.logins.db"

public enum valueType {
case account
case database
}

static let allValues: [KeychainKey] = [.accountJSON, .email, .displayName, .avatarURL, .salt]
static let accountValues: [KeychainKey] = [.accountJSON, .email, .displayName, .avatarURL]
static let databaseValues: [KeychainKey] = [.salt, .loginsKey]

static let oldAccountValues: [KeychainKey] = [.email, .displayName, .avatarURL]
}
2 changes: 1 addition & 1 deletion Shared/Store/BaseDataStore.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ class BaseDataStore {
}()

internal var loginsKey: String? {
let key = "sqlcipher.key.logins.db"
let key = KeychainKey.loginsKey.rawValue
Copy link
Contributor

Choose a reason for hiding this comment

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

😸

if self.keychainWrapper.hasValue(forKey: key) {
return self.keychainWrapper.string(forKey: key)
}
Expand Down
5 changes: 3 additions & 2 deletions lockbox-ios/Common/AppDelegate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed

} else {
checkForKeychainVersionAbnormalities()
}
Expand Down Expand Up @@ -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)
Copy link
Contributor

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()?

Copy link
Contributor Author

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.

}
}
}
Expand Down
2 changes: 1 addition & 1 deletion lockbox-ios/Common/Resources/Info.plist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
<key>CFBundlePackageType</key>
<string>APPL</string>
<key>CFBundleShortVersionString</key>
<string>1.7.1</string>
<string>1.7.2</string>
<key>CFBundleURLTypes</key>
<array>
<dict>
Expand Down
2 changes: 1 addition & 1 deletion lockbox-ios/Store/AccountStore.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ extension AccountStore {
}

private func clear() {
KeychainWrapper.wipeKeychain()
keychainWrapper.clearAllValues(for: .account)
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes 👍


self.webData.fetchDataRecords(ofTypes: WKWebsiteDataStore.allWebsiteDataTypes()) { records in
self.webData.removeData(ofTypes: WKWebsiteDataStore.allWebsiteDataTypes(), for: records) { }
Expand Down