-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
MBL-1161: Add code to handle storing/retrieving/deleting Keychain items #1944
MBL-1161: Add code to handle storing/retrieving/deleting Keychain items #1944
Conversation
return Bundle.main.bundleIdentifier ?? "com.kickstarter.kickstarter" | ||
} | ||
|
||
public static func deleteAllPasswords() throws { |
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.
Many of these functions were inspired by the example code @chris-laidlaw-kickstarter sent, here: https://www.advancedswift.com/secure-private-data-keychain-swift
} | ||
} | ||
|
||
public static func storePassword(_ password: String, forAccount account: String) throws { |
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.
Probably worth noting - I decided to keep the nomenclature of this code fairly close to the way the keychain actually works, which uses "accounts" and "passwords". In our case, we're going to be storing an OAuth token against some bit of user data (probably a user e-mail or a user ID). I plan on writing a quick little extension when we integrate this code with the rest of the app to make that clearer:
extension Keychain {
public static func storeOAuthToken(_ token: String, forUserID id: Int) {
self.storePassword(token, forAccount: "\(id)")
}
}
@@ -9128,6 +9153,7 @@ | |||
SDKROOT = iphoneos; | |||
SUPPORTS_MACCATALYST = NO; | |||
SWIFT_VERSION = 5.0; | |||
TEST_HOST = "$(BUILT_PRODUCTS_DIR)/KickDebug.app/$(BUNDLE_EXECUTABLE_FOLDER_PATH)/KickDebug"; |
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.
This is the change that was required to get the unit tests to work. It's fairly innocuous looking in Xcode. Annoyingly, I can't find any good documentation around test hosts - my guess is that it actually boots the app, which might increase the runtime for Library-Tests
. I'll keep an eye on that.
Without this switch, all code accessing the Keychain in the unit test fails due to invalid entitlements, which is a very odd error (considering that unit tests aren't signed or entitled in any meaningful way.)
c1f69bd
to
30debcc
Compare
|
||
public struct Keychain { | ||
private static var serviceName: String { | ||
return Bundle.main.bundleIdentifier ?? "com.kickstarter.kickstarter" |
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.
The documentation for this is poor; I think it can be an arbitrary string, but the bundle ID seems like a nice value to use.
30debcc
to
7174fb5
Compare
@@ -7256,6 +7324,7 @@ | |||
A7D1F9441C850B7C000D41D5 /* Kickstarter-iOS */, | |||
A755113B1C8642B3005355CF /* Library-iOS */, | |||
A75511441C8642B3005355CF /* Library-iOSTests */, | |||
E113BD802B7D255000D3A809 /* Library-Keychain-iOSTests */, |
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.
BlueprintName = "Library-Keychain-iOSTests" | ||
ReferencedContainer = "container:Kickstarter.xcodeproj"> | ||
</BuildableReference> | ||
</TestableReference> |
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.
Generated by 🚫 Danger |
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.
Looks good! Thanks for explaining all the things!
This is just so we can test a single file, KeychainTests, in a hosted environment, without affecting the rest of our tests.
7174fb5
to
1d0040b
Compare
📲 What
This PR adds
Keychain.swift
, which contains a utility methods for interacting with the system keychain.🤔 Why
Currently we store our authentication tokens in
UserDefaults
, which is insecure. As part of migrating to OAuth, both iOS and Android want to use their equivalent system keychains, which is the correct/recommended implementation for login data.This is the first step in that - writing code that allows us to actually interact with the keychain.