MBL-1161: Read and store OAuth token from keychain #1963

amy-at-kickstarter · 2024-02-27T22:25:51Z

📲 What

Store the OAuth token in the keychain, instead of in user defaults. Additional code moves the old token out of defaults and into the keychain if you were already logged in.

🤔 Why

Leaving the token in user defaults is insecure.

🛠 How

We store the OAuth token per-user, using a pseudo-account-name like kickstarter_12345 for user with id 12345. The change is gated behind a feature flag, and hopefully fairly robust to any keychain failures or exceptions.

⏰ TODO

Solicit feedback about storing token per-user vs. just one for the entire app
Run testing on-device

1) The tests in AppEnvironmentTests (the original test file) would all fail if the Keychain is used, because the Keychain requires a hosted app environment to work in unit tests. So any test touching the save/restore functionality has the feature flag turned OFF. 2) The tests in AppEnvironmentTests+OAuthInKeychain are hosted, so the Keychain works. They are a mix of feature flag ON and OFF.

nativeksr · 2024-02-27T22:29:54Z

	1 Warning
⚠️	Big PR

SwiftFormat found issues:

File	Rules
Library/AppEnvironment.swift:470:1	warning: (wrap) Wrap lines that exceed the specified maximum width.
Library/AppEnvironment.swift:471:1	warning: (wrap) Wrap lines that exceed the specified maximum width.
Library/AppEnvironment.swift:472:1	warning: (wrap) Wrap lines that exceed the specified maximum width.

Generated by 🚫 Danger

amy-at-kickstarter · 2024-02-28T15:27:27Z

Library/AppEnvironment.swift

@@ -295,6 +296,41 @@ public struct AppEnvironment: AppEnvironmentType {
    )
  }

+  internal static func accountNameForUserId(_ userId: Int) -> String {
+    return "kickstarter_\(userId)"


This is something I went back-and-forth on. The keychain is designed to store passwords for accounts. However, the way our app works right now, we only ever store one OAuth token in the user defaults - we store it for the current user, and remove it when you log out.

So I had two choices here. I could name the keychain account something like kickstarter_current_user and only have one, or I could name it after the specific user who is logged in like I did here.

I decided to keep it linked to the specific user, but there's an argument to be made that YAGNI and I'm over-complicating things. The big disadvantage is that when you log out, we lose track of the currently logged in user, so it's not easy for us to delete the old keychain item.

I don't see why we'd need to name the token with the user id. It's not like we allow multiple users to be logged in at the same time, and the token is deleted when the user logs out. I don't think it hurts either, though.

However, how does this naming scheme work across debug/beta/release apps? Does the keychain add namespacing for us or should we be using bundle ids or something instead of just kickstarter to avoid name collisions between our different apps?

Yeah, I'm not sure I see a need any more, either. My original thinking was "hey, this way we can support multiple users in the future!" but I'm starting to think that's not ... particularly useful. You could definitely talk me into making it a single key.

As for the naming scheme, great question. We do use the bundle ID in Keychain.swift, which should identify the oauth token uniquely per app, but I haven't actually tested to make sure that it doesn't overlap between build environments. I think you have to do extra work to enable keychain sharing between app IDs but yeah, let's validate that.

Double-checked and yes, we're already using the bundle ID when we save the key. So there won't be any overlap.

amy-at-kickstarter · 2024-02-28T15:29:42Z

Library/AppEnvironment.swift

@@ -303,13 +339,21 @@ public struct AppEnvironment: AppEnvironmentType {
    let data = userDefaults.dictionary(forKey: self.environmentStorageKey) ?? [:]

    var service = self.current.apiService
-    var currentUser: User?
+    var currentUser: User? // Will only be set if an OAuth token is also set


Something that is a little confusing now is that you need both the User and the token to log in, but they're stored in different places 🤯

amy-at-kickstarter · 2024-02-28T15:29:56Z

Library/AppEnvironment.swift

+
+    // If there is an oauth token stored, then we can authenticate our api service
+
+    if let oauthToken = fetchOAuthTokenFromKeychain(forUserId: userFromDefaults?.id) {


n.B. the feature flag check is inside this method

amy-at-kickstarter · 2024-02-28T15:30:30Z

Library/AppEnvironment.swift

+
+    if let oauthToken = env.apiService.oauthToken?.token {
+      // Try to save to the keychain, but if that fails, save to user defaults
+      if !self.storeOAuthTokenToKeychain(oauthToken, forUserId: env.currentUser?.id) {


Again, this will fail if the feature flag is off, and it will resume saving it to user defaults.

amy-at-kickstarter · 2024-02-28T15:30:59Z

Library/AppEnvironmentTests+OAuthInKeychain.swift

+@testable import Library
+import XCTest
+
+final class AppEnvironmentTests_OAuthInKeychain: XCTestCase {


These are all tests run in the hosted environment, so they can actually test the keychain. I included a bunch of duplicates of tests in AppEnvironmentTests that just swap the feature flag to 'on' to correctly test that behavior.

amy-at-kickstarter · 2024-02-28T15:31:37Z

Library/AppEnvironmentTests+OAuthInKeychain.swift

+    /* Awkward bit of condition here - our feature flag values implicitly come from
+     AppEnvironment.current, which in some cases, is the PREVIOUS environment in the stack.
+     If you're pushing/saving a new stack, that can be confusing. */
+    AppEnvironment.updateRemoteConfigClient(mockConfigClient)


This was an interesting mess I got myself into - what does it mean to test a feature flag from within AppEnvironment, which is where we store the feature flags?

amy-at-kickstarter · 2024-02-28T15:38:33Z

Library/AppEnvironmentTests.swift

@@ -81,7 +93,9 @@ final class AppEnvironmentTests: XCTestCase {
    AppEnvironment.popEnvironment()
  }

-  func testFromStorage_WithNothingStored() {
+  func testFromStorage_WithNothingStored_featureUseKeychainEnabledIsFalse() {
+    self.setFeatureUseKeychainEnabled(false)


So now we have tests running in three different states:

AppEnvironmentTests running in the un-hosted test environment, with the feature flag OFF. This lets us run the original AppEnvironmentTests without breaking all of them.
AppEnvironmentTests running in the hosted test environment, with the feature flag OFF. This simulates the app with the feature flag off.
AppEnvironmentTests+OAuthInKeychain running in the hosted test environment, with the feature flag ON. This simulates the app with the feature flag on.

ifosli

I have some questions but overall, looks good!

ifosli · 2024-02-28T17:32:21Z

Library/AppEnvironment.swift

@@ -295,6 +296,41 @@ public struct AppEnvironment: AppEnvironmentType {
    )
  }

+  internal static func accountNameForUserId(_ userId: Int) -> String {
+    return "kickstarter_\(userId)"


I don't see why we'd need to name the token with the user id. It's not like we allow multiple users to be logged in at the same time, and the token is deleted when the user logs out. I don't think it hurts either, though.

However, how does this naming scheme work across debug/beta/release apps? Does the keychain add namespacing for us or should we be using bundle ids or something instead of just kickstarter to avoid name collisions between our different apps?

ifosli · 2024-02-28T17:40:12Z

Library/AppEnvironment.swift

+      service = service.login(OauthToken(token: oauthToken))
+
+      // Move it over to the keychain if we can
+      _ = self.storeOAuthTokenToKeychain(oauthToken, forUserId: userFromDefaults?.id)


Should we remove the token from user defaults if storing it to the keychain is successful? Alternatively, do we need to save the token here at all, since we save it in saveEnvironment?

Hmmm very good point about just keeping it in saveEnvironment! Let me check, that would be cleaner.

Library/AppEnvironmentTests+OAuthInKeychain.swift

amy-at-kickstarter · 2024-02-28T20:47:21Z

Library/AppEnvironment.swift


    let next = Environment()
-    self.pushEnvironment(next)
+    self.replaceCurrentEnvironment(next)


This fixes a crash in resetStackForUnitTests because it will never be truly empty. If it's empty, calls to isFeatureEnabled will fail.

amy-at-kickstarter · 2024-02-29T14:56:50Z

Regarding testing on device: I'm going to deploy the beta and then run the tests that way. I won't mark the ticket as complete until I'm happy with on-device testing.

amy-at-kickstarter added 2 commits February 27, 2024 17:26

MBL-1161: Read and store OAuth token from keychain

2f83516

amy-at-kickstarter force-pushed the feat/adyer/MBL-1161/migrate-to-keychain branch from f475880 to fa861f9 Compare February 27, 2024 22:26

amy-at-kickstarter commented Feb 28, 2024

View reviewed changes

amy-at-kickstarter requested review from Arkariang, ifosli and scottkicks February 28, 2024 15:40

amy-at-kickstarter self-assigned this Feb 28, 2024

amy-at-kickstarter marked this pull request as ready for review February 28, 2024 15:42

ifosli approved these changes Feb 28, 2024

View reviewed changes

amy-at-kickstarter added 3 commits February 28, 2024 14:56

Review #1963: Remove redundant call to save keychain item

0d76d21

Review #1963: Clarify test assertion message

3d49dd8

Review #1963: Use static key for currently logged in user

728a5ed

amy-at-kickstarter commented Feb 28, 2024

View reviewed changes

amy-at-kickstarter merged commit 13c6d1d into main Feb 29, 2024
5 checks passed

amy-at-kickstarter deleted the feat/adyer/MBL-1161/migrate-to-keychain branch February 29, 2024 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MBL-1161: Read and store OAuth token from keychain #1963

MBL-1161: Read and store OAuth token from keychain #1963

amy-at-kickstarter commented Feb 27, 2024 •

edited

nativeksr commented Feb 27, 2024 •

edited

amy-at-kickstarter Feb 28, 2024 •

edited

ifosli Feb 28, 2024

amy-at-kickstarter Feb 28, 2024 •

edited

amy-at-kickstarter Feb 28, 2024

amy-at-kickstarter Feb 28, 2024

amy-at-kickstarter Feb 28, 2024

amy-at-kickstarter Feb 28, 2024

amy-at-kickstarter Feb 28, 2024

amy-at-kickstarter Feb 28, 2024

amy-at-kickstarter Feb 28, 2024

ifosli left a comment

ifosli Feb 28, 2024

ifosli Feb 28, 2024

amy-at-kickstarter Feb 28, 2024

amy-at-kickstarter Feb 28, 2024

amy-at-kickstarter commented Feb 29, 2024


		// If there is an oauth token stored, then we can authenticate our api service

		if let oauthToken = fetchOAuthTokenFromKeychain(forUserId: userFromDefaults?.id) {

MBL-1161: Read and store OAuth token from keychain #1963

MBL-1161: Read and store OAuth token from keychain #1963

Conversation

amy-at-kickstarter commented Feb 27, 2024 • edited

📲 What

🤔 Why

🛠 How

⏰ TODO

nativeksr commented Feb 27, 2024 • edited

SwiftFormat found issues:

amy-at-kickstarter Feb 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-at-kickstarter Feb 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifosli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-at-kickstarter commented Feb 29, 2024

amy-at-kickstarter commented Feb 27, 2024 •

edited

nativeksr commented Feb 27, 2024 •

edited

amy-at-kickstarter Feb 28, 2024 •

edited

amy-at-kickstarter Feb 28, 2024 •

edited