Skip to content

Commit

Permalink
MBL-1161: Read and store OAuth token from keychain (#1963)
Browse files Browse the repository at this point in the history 
* MBL-1161: Read and store OAuth token from keychain

* MBL-1161: Add test cases for migrating OAuth token to the keychain.

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.

* Review #1963: Remove redundant call to save keychain item

* Review #1963: Clarify test assertion message

* Review #1963: Use static key for currently logged in user
  • Loading branch information
@amy-at-kickstarter
amy-at-kickstarter committed Feb 29, 2024
1 parent e67deda commit 13c6d1d
Show file tree
Hide file tree
Showing 4 changed files with 529 additions and 12 deletions.
4 changes: 4 additions & 0 deletions Kickstarter.xcodeproj/project.pbxproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1517,6 +1517,7 @@
E17611E42B751E8100DF2F50 /* Paginator.swift in Sources */ = {isa = PBXBuildFile; fileRef = E17611E32B751E8100DF2F50 /* Paginator.swift */; };
E17611E62B75242A00DF2F50 /* PaginatorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = E17611E52B75242A00DF2F50 /* PaginatorTests.swift */; };
E182E5BA2B8CDFDE0008DD69 /* AppEnvironmentTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = A7ED1F121E830FDC00BFFA01 /* AppEnvironmentTests.swift */; };
E182E5BC2B8D36FE0008DD69 /* AppEnvironmentTests+OAuthInKeychain.swift in Sources */ = {isa = PBXBuildFile; fileRef = E182E5BB2B8D36FE0008DD69 /* AppEnvironmentTests+OAuthInKeychain.swift */; };
E1A1491E2ACDD76800F49709 /* FetchBackerProjectsQuery.graphql in Resources */ = {isa = PBXBuildFile; fileRef = E1A1491D2ACDD76700F49709 /* FetchBackerProjectsQuery.graphql */; };
E1A149202ACDD7BF00F49709 /* FetchProjectsEnvelope+FetchBackerProjectsQueryData.swift in Sources */ = {isa = PBXBuildFile; fileRef = E1A1491F2ACDD7BF00F49709 /* FetchProjectsEnvelope+FetchBackerProjectsQueryData.swift */; };
E1A149222ACE013100F49709 /* FetchProjectsEnvelope.swift in Sources */ = {isa = PBXBuildFile; fileRef = E1A149212ACE013100F49709 /* FetchProjectsEnvelope.swift */; };
Expand Down Expand Up @@ -3126,6 +3127,7 @@
E17611E12B73D9A400DF2F50 /* Data+PKCE.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "Data+PKCE.swift"; sourceTree = "<group>"; };
E17611E32B751E8100DF2F50 /* Paginator.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Paginator.swift; sourceTree = "<group>"; };
E17611E52B75242A00DF2F50 /* PaginatorTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PaginatorTests.swift; sourceTree = "<group>"; };
E182E5BB2B8D36FE0008DD69 /* AppEnvironmentTests+OAuthInKeychain.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "AppEnvironmentTests+OAuthInKeychain.swift"; sourceTree = "<group>"; };
E1889D8D2B6065D6004FBE21 /* CombineTestObserverTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CombineTestObserverTests.swift; sourceTree = "<group>"; };
E1A1491D2ACDD76700F49709 /* FetchBackerProjectsQuery.graphql */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text; path = FetchBackerProjectsQuery.graphql; sourceTree = "<group>"; };
E1A1491F2ACDD7BF00F49709 /* FetchProjectsEnvelope+FetchBackerProjectsQueryData.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "FetchProjectsEnvelope+FetchBackerProjectsQueryData.swift"; sourceTree = "<group>"; };
Expand Down Expand Up @@ -6000,6 +6002,7 @@
A709697D1D143D1300DB39D3 /* AlertError.swift */,
A7C725781C85D36D005A016B /* AppEnvironment.swift */,
A7ED1F121E830FDC00BFFA01 /* AppEnvironmentTests.swift */,
E182E5BB2B8D36FE0008DD69 /* AppEnvironmentTests+OAuthInKeychain.swift */,
D764373F22414FA900DAFC9E /* AppEnvironmentType.swift */,
77C9122823C4FC0B00F3D2C9 /* ApplePayCapabilities.swift */,
77C9122E23C508CF00F3D2C9 /* ApplePayCapabilitiesTests.swift */,
Expand Down Expand Up @@ -8994,6 +8997,7 @@
isa = PBXSourcesBuildPhase;
buildActionMask = 2147483647;
files = (
E182E5BC2B8D36FE0008DD69 /* AppEnvironmentTests+OAuthInKeychain.swift in Sources */,
E182E5BA2B8CDFDE0008DD69 /* AppEnvironmentTests.swift in Sources */,
E113BD842B7D255000D3A809 /* Library_Keychain_iOSTests.swift in Sources */,
E113BD8D2B7D255A00D3A809 /* KeychainTests.swift in Sources */,
Expand Down
73 changes: 67 additions & 6 deletions Library/AppEnvironment.swift
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import FBSDKCoreKit
import FirebaseCrashlytics
import Foundation
import KsApi
import Prelude
Expand Down Expand Up @@ -142,10 +143,12 @@ public struct AppEnvironment: AppEnvironmentType {
}

internal static func resetStackForUnitTests() {
while self.stack.popLast() != nil {}
while self.stack.count > 1 {
_ = self.stack.popLast()
}

let next = Environment()
self.pushEnvironment(next)
self.replaceCurrentEnvironment(next)
}

// Replace the current environment with a new environment.
Expand Down Expand Up @@ -295,6 +298,52 @@ public struct AppEnvironment: AppEnvironmentType {
)
}

internal static let accountNameForKeychain = "kickstarter_currently_logged_in_user"

private static func storeOAuthTokenToKeychain(_ oauthToken: String) -> Bool {
guard featureUseKeychainForOAuthTokenEnabled()
else {
return false
}

do {
try Keychain.storePassword(oauthToken, forAccount: self.accountNameForKeychain)
return true
} catch {
Crashlytics.crashlytics().record(error: error)
}

return false
}

private static func fetchOAuthTokenFromKeychain() -> String? {
guard featureUseKeychainForOAuthTokenEnabled()
else {
return nil
}

do {
return try Keychain.fetchPassword(forAccount: self.accountNameForKeychain)
} catch {
Crashlytics.crashlytics().record(error: error)
}

return nil
}

private static func removeOAuthTokenFromKeychain() -> Bool {
guard featureUseKeychainForOAuthTokenEnabled() else { return false }

do {
try Keychain.deletePassword(forAccount: self.accountNameForKeychain)
return true
} catch {
Crashlytics.crashlytics().record(error: error)
}

return false
}

// Returns the last saved environment from user defaults.
public static func fromStorage(
ubiquitousStore _: KeyValueStoreType,
Expand All @@ -303,12 +352,15 @@ 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
let configDict: [String: Any]? = data["config"] as? [String: Any]
let config: Config? = configDict.flatMap(Config.decodeJSONDictionary)

if let oauthToken = data["apiService.oauthToken.token"] as? String {
// If there is an oauth token stored in the defaults, then we can authenticate our api service
// If there is an oauth token stored, then we can authenticate our api service

if let oauthToken = fetchOAuthTokenFromKeychain() {
service = service.login(OauthToken(token: oauthToken))
} else if let oauthToken = data["apiService.oauthToken.token"] as? String {
service = service.login(OauthToken(token: oauthToken))
}

Expand Down Expand Up @@ -404,7 +456,16 @@ public struct AppEnvironment: AppEnvironmentType {
var data: [String: Any] = [:]

// swiftformat:disable wrap
data["apiService.oauthToken.token"] = env.apiService.oauthToken?.token

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) {
data["apiService.oauthToken.token"] = oauthToken
}
} else {
_ = self.removeOAuthTokenFromKeychain()
}

data["apiService.serverConfig.apiBaseUrl"] = env.apiService.serverConfig.apiBaseUrl.absoluteString
data["apiService.serverConfig.apiClientAuth.clientId"] = env.apiService.serverConfig.apiClientAuth.clientId
data["apiService.serverConfig.basicHTTPAuth.username"] = env.apiService.serverConfig.basicHTTPAuth?.username
Expand Down

0 comments on commit 13c6d1d

Please sign in to comment.