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鈥檒l 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

Merged
merged 2 commits into from Feb 14, 2024

Conversation

amy-at-kickstarter
Copy link
Contributor

@amy-at-kickstarter amy-at-kickstarter commented Feb 14, 2024

馃摬 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.

return Bundle.main.bundleIdentifier ?? "com.kickstarter.kickstarter"
}

public static func deleteAllPasswords() throws {
Copy link
Contributor Author

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 {
Copy link
Contributor Author

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";
Copy link
Contributor Author

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.
Screenshot 2024-02-14 at 11 05 18 AM
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.)

@amy-at-kickstarter amy-at-kickstarter force-pushed the feat/adyer/MBL-1161/add-keychain-code branch from c1f69bd to 30debcc Compare February 14, 2024 16:15
@amy-at-kickstarter amy-at-kickstarter self-assigned this Feb 14, 2024
@amy-at-kickstarter amy-at-kickstarter changed the title Feat/adyer/mbl 1161/add keychain code MBL-1161: Add code to handle storing/retrieving/deleting Keychain items Feb 14, 2024

public struct Keychain {
private static var serviceName: String {
return Bundle.main.bundleIdentifier ?? "com.kickstarter.kickstarter"
Copy link
Contributor Author

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.

@amy-at-kickstarter amy-at-kickstarter marked this pull request as ready for review February 14, 2024 16:19
@amy-at-kickstarter amy-at-kickstarter force-pushed the feat/adyer/MBL-1161/add-keychain-code branch from 30debcc to 7174fb5 Compare February 14, 2024 16:56
@@ -7256,6 +7324,7 @@
A7D1F9441C850B7C000D41D5 /* Kickstarter-iOS */,
A755113B1C8642B3005355CF /* Library-iOS */,
A75511441C8642B3005355CF /* Library-iOSTests */,
E113BD802B7D255000D3A809 /* Library-Keychain-iOSTests */,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, to get these tests to work, I had to make a new testing bundle, which is hosted, which just contains a single test, KeychainTests. Using an app host for Library-iOSTests breaks several tests, but without using the app host, KeychainTests will fail.

These changes look like this in the GUI:
Screenshot 2024-02-14 at 11 53 14 AM
Screenshot 2024-02-14 at 11 53 26 AM

BlueprintName = "Library-Keychain-iOSTests"
ReferencedContainer = "container:Kickstarter.xcodeproj">
</BuildableReference>
</TestableReference>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This schema change is so that when you test Library-iOS, it runs both Library-iOSTests and Library-Keychain-iOSTests

Screenshot 2024-02-14 at 11 52 59 AM

@nativeksr
Copy link
Collaborator

1 Warning
鈿狅笍 Big PR

Generated by 馃毇 Danger

@amy-at-kickstarter
Copy link
Contributor Author

Screenshot 2024-02-14 at 1 42 21 PM Tests work!

Copy link
Contributor

@ifosli ifosli left a 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.
@amy-at-kickstarter amy-at-kickstarter force-pushed the feat/adyer/MBL-1161/add-keychain-code branch from 7174fb5 to 1d0040b Compare February 14, 2024 19:16
@amy-at-kickstarter amy-at-kickstarter merged commit 50fd2f3 into main Feb 14, 2024
5 checks passed
@amy-at-kickstarter amy-at-kickstarter deleted the feat/adyer/MBL-1161/add-keychain-code branch February 14, 2024 20:04
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.

None yet

3 participants