Merge branch 'cj/fix/acknowlege-session-manager-errors' into 'develop'

Merge-Request: apple/vpn/protonvpn!1568
Approved-by: John Biggs <[email protected]>

apps/macos/ProtonVPN-macOS-Unit-All.xctestplan

@@ -41,28 +41,21 @@
  },
  {
  "target" : {
- "containerPath" : "container:..\/..\/libraries\/ConnectionDetails",
- "identifier" : "ConnectionDetailsTests",
- "name" : "ConnectionDetailsTests"
+ "containerPath" : "container:..\/..\/libraries\/LegacyCommon",
+ "identifier" : "LegacyCommonTests",
+ "name" : "LegacyCommonTests"
  }
  },
  {
  "target" : {
- "containerPath" : "container:..\/..\/libraries\/Ergonomics",
+ "containerPath" : "container:..\/..\/libraries\/Foundations\/Ergonomics",
  "identifier" : "ErgonomicsTests",
  "name" : "ErgonomicsTests"
  }
  },
  {
  "target" : {
- "containerPath" : "container:..\/..\/libraries\/LegacyCommon",
- "identifier" : "LegacyCommonTests",
- "name" : "LegacyCommonTests"
- }
- },
- {
- "target" : {
- "containerPath" : "container:..\/..\/libraries\/LocalFeatureFlags",
+ "containerPath" : "container:..\/..\/libraries\/Foundations\/LocalFeatureFlags",
  "identifier" : "LocalFeatureFlagsTests",
  "name" : "LocalFeatureFlagsTests"
  }
@@ -81,6 +74,13 @@
  "name" : "NEHelperTests"
  }
  },
+ {
+ "target" : {
+ "containerPath" : "container:..\/..\/libraries\/NEHelper",
+ "identifier" : "VPNCryptoTests",
+ "name" : "VPNCryptoTests"
+ }
+ },
  {
  "target" : {
  "containerPath" : "container:..\/..\/libraries\/NEHelper",
@@ -90,7 +90,14 @@
  },
  {
  "target" : {
- "containerPath" : "container:..\/..\/libraries\/PMLogger",
+ "containerPath" : "container:..\/..\/libraries\/Shared\/Persistence",
+ "identifier" : "PersistenceTests",
+ "name" : "PersistenceTests"
+ }
+ },
+ {
+ "target" : {
+ "containerPath" : "container:..\/..\/libraries\/Foundations\/PMLogger",
  "identifier" : "PMLoggerTests",
  "name" : "PMLoggerTests"
  }
@@ -104,31 +111,24 @@
  },
  {
  "target" : {
- "containerPath" : "container:..\/..\/libraries\/SharedViews",
- "identifier" : "SharedViewsTests",
- "name" : "SharedViewsTests"
+ "containerPath" : "container:..\/..\/libraries\/ConnectionDetails",
+ "identifier" : "ConnectionDetailsTests",
+ "name" : "ConnectionDetailsTests"
  }
  },
  {
  "target" : {
- "containerPath" : "container:..\/..\/libraries\/Strings",
- "identifier" : "StringsTests",
- "name" : "StringsTests"
+ "containerPath" : "container:..\/..\/libraries\/Foundations\/Timer",
+ "identifier" : "TimerTests",
+ "name" : "TimerTests"
  }
  },
  {
  "target" : {
- "containerPath" : "container:..\/..\/libraries\/Timer",
+ "containerPath" : "container:..\/..\/libraries\/Foundations\/Timer",
  "identifier" : "TimerMockTests",
  "name" : "TimerMockTests"
  }
- },
- {
- "target" : {
- "containerPath" : "container:..\/..\/libraries\/Timer",
- "identifier" : "TimerTests",
- "name" : "TimerTests"
- }
  }
  ],
  "version" : 1

apps/macos/ProtonVPN/Scenes/Common/Services/AppSessionManager.swift

@@ -100,7 +100,11 @@ final class AppSessionManagerImplementation: AppSessionRefresherImplementation,
 
  override func attemptSilentLogIn(completion: @escaping (Result<(), Error>) -> Void) {
  // Invoke async implementation
- executeOnUIThread(attemptLogin, success: { completion(.success) }, failure: { completion(.failure($0)) })
+ executeOnUIThread(
+ attemptLogin,
+ success: { completion(.success) },
+ failure: { completion(.failure($0)) }
+ )
  }
 
  func finishLogin(authCredentials: AuthCredentials, success: @escaping () -> Void, failure: @escaping (Error) -> Void) {
@@ -200,6 +204,10 @@ final class AppSessionManagerImplementation: AppSessionRefresherImplementation,
  await successfulConsecutiveSessionRefreshes.increment()
  }
 
+ /// Ignore errors unless one of the following is true:
+ /// - API returns `ProtonVpnError.subuserWithoutSessions`
+ /// - Server storage is empty or user IP is not known
+ /// - We hit a keychain error
  private func getVPNProperties() async throws -> VpnProperties? {
  let isDisconnected = await appState.isDisconnected
  let location = propertiesManager.userLocation
@@ -215,9 +223,8 @@ final class AppSessionManagerImplementation: AppSessionRefresherImplementation,
  logOutCleanup()
  throw ProtonVpnError.subuserWithoutSessions
  } catch {
- log.error("Failed to obtain user's VPN properties: \(error.localizedDescription)", category: .app)
- if serverStorage.fetch().isEmpty, self.propertiesManager.userLocation?.ip == nil, (error is KeychainError) {
- // only throw if there is a major reason
+ log.error("Failed to obtain user's VPN properties", category: .app, metadata: ["error": "\(error)"])
+ if serverStorage.fetch().isEmpty || self.propertiesManager.userLocation?.ip == nil || (error is KeychainError) {
  throw error
  }
  }

apps/macos/ProtonVPNTests/Tests/AppSessionManagerImplementationTests.swift

@@ -26,8 +26,10 @@ import ProtonCoreNetworking
 @testable import ProtonVPN
 
 fileprivate let testData = MockTestData()
-fileprivate let testAuthCredentials = AuthCredentials(username: "username", accessToken: "", refreshToken: "", sessionId: "", userId: "", scopes: [])
-fileprivate let testVPNCredentials = VpnKeychainMock.vpnCredentials(accountPlan: .plus, maxTier: CoreAppConstants.VpnTiers.plus)
+fileprivate func mockAuthCredentials(username: String) -> AuthCredentials {
+ return AuthCredentials(username: username, accessToken: "", refreshToken: "", sessionId: "", userId: "", scopes: [])
+}
+fileprivate var testAuthCredentials: AuthCredentials = mockAuthCredentials(username: "username")
 
 fileprivate let subuserWithoutSessionsResponseError = ResponseError(httpCode: HttpStatusCode.accessForbidden,
  responseCode: ApiErrorCode.subuserWithoutSessions,
@@ -49,36 +51,34 @@ final class AppSessionManagerImplementationTests: XCTestCase {
 
  let asyncTimeout: TimeInterval = 5
 
- var mockVPNAPIService: VpnApiService {
- networking = NetworkingMock()
- networkingDelegate = FullNetworkingMockDelegate()
- networking.delegate = networkingDelegate
-
- return VpnApiService(
- networking: networking,
- vpnKeychain: VpnKeychainMock(),
- countryCodeProvider: CountryCodeProviderImplementation(),
- authKeychain: authKeychain
- )
- }
-
  override func setUp() {
  super.setUp()
  propertiesManager = PropertiesManagerMock()
  networking = NetworkingMock()
- networkingDelegate = FullNetworkingMockDelegate()
- networking.delegate = networkingDelegate
  authKeychain = AuthKeychainHandleMock()
  unauthKeychain = UnauthKeychainMock()
  vpnKeychain = VpnKeychainMock()
  alertService = AppSessionManagerAlertServiceMock()
  appStateManager = AppStateManagerMock()
 
+
+ networkingDelegate = FullNetworkingMockDelegate()
+ let freeCreds = VpnKeychainMock.vpnCredentials(accountPlan: .free, maxTier: CoreAppConstants.VpnTiers.free)
+ networkingDelegate.apiCredentials = freeCreds
+ networking.delegate = networkingDelegate
+
+ let mockAPIService = VpnApiService(
+ networking: networking,
+ vpnKeychain: VpnKeychainMock(),
+ countryCodeProvider: CountryCodeProviderImplementation(),
+ authKeychain: authKeychain
+ )
+
  manager = withDependencies {
  $0.date = .constant(Date())
  } operation: {
  let factory = ManagerFactoryMock(
- vpnAPIService: mockVPNAPIService,
+ vpnAPIService: mockAPIService,
  authKeychain: authKeychain,
  unauthKeychain: unauthKeychain,
  vpnKeychain: vpnKeychain,
@@ -110,7 +110,7 @@ final class AppSessionManagerImplementationTests: XCTestCase {
  networkingDelegate.apiClientConfig = testData.defaultClientConfig
 
  manager.finishLogin(
- authCredentials: testAuthCredentials,
+ authCredentials: mockAuthCredentials(username: "bob"),
  success: {
  loginExpectation.fulfill()
  XCTAssertTrue(self.manager.loggedIn)
@@ -122,6 +122,8 @@ final class AppSessionManagerImplementationTests: XCTestCase {
  )
 
  wait(for: [loginExpectation], timeout: asyncTimeout)
+
+ XCTAssertEqual(authKeychain.username, "bob", "Username should have been updated in auth keychain")
  }
 
  func testSuccessfulSilentLoginLogsIn() throws {
@@ -220,9 +222,6 @@ final class AppSessionManagerImplementationTests: XCTestCase {
  appStateManager.state = .connected(differentUserServerDescriptor)
  networkingDelegate.apiVpnLocation = .mock
  networkingDelegate.apiClientConfig = testData.defaultClientConfig
- let freeCreds = VpnKeychainMock.vpnCredentials(accountPlan: .free,
- maxTier: CoreAppConstants.VpnTiers.free)
- networkingDelegate.apiCredentials = freeCreds
  authKeychain.credentials = testAuthCredentials
  alertService.addAlertHandler(for: ActiveSessionWarningAlert.self, handler: { alert in
  activeSessionAlertExpectation.fulfill()
@@ -246,9 +245,6 @@ final class AppSessionManagerImplementationTests: XCTestCase {
  appStateManager.state = .connected(differentUserServerDescriptor)
  networkingDelegate.apiVpnLocation = .mock
  networkingDelegate.apiClientConfig = testData.defaultClientConfig
- let freeCreds = VpnKeychainMock.vpnCredentials(accountPlan: .free,
- maxTier: CoreAppConstants.VpnTiers.free)
- networkingDelegate.apiCredentials = freeCreds
  authKeychain.credentials = testAuthCredentials
  alertService.addAlertHandler(for: ActiveSessionWarningAlert.self, handler: { alert in
  activeSessionAlertExpectation.fulfill()
@@ -451,8 +447,12 @@ class AuthKeychainHandleMock: AuthKeychainHandle {
  var username: String?
  var userId: String?
 
- func saveToCache(_ credentials: VPNShared.AuthCredentials?) { }
- func store(_ credentials: VPNShared.AuthCredentials, forContext: VPNShared.AppContext?) throws { }
+ func saveToCache(_ credentials: VPNShared.AuthCredentials?) {
+ self.credentials = credentials
+ }
+ func store(_ credentials: VPNShared.AuthCredentials, forContext: VPNShared.AppContext?) throws {
+ self.credentials = credentials
+ }
  func fetch(forContext: AppContext?) -> AuthCredentials? { return credentials }
  func clear() { }
 }

libraries/LegacyCommon/Sources/LegacyCommon/Mocks/NetworkingMock.swift

@@ -15,6 +15,8 @@ import ProtonCoreFoundations
 import VPNShared
 import ProtonCoreEnvironment
 
+import XCTestDynamicOverlay
+
 public final class NetworkingMock {
  public weak var delegate: NetworkingMockDelegate?
 
@@ -229,7 +231,7 @@ public class FullNetworkingMockDelegate: NetworkingMockDelegate {
  do {
  return try handleMockNetworkingRequestThrowingOnUnexpectedError(request)
  } catch {
- assertionFailure("Unexpected error occurred: \(error)")
+ XCTFail("Unexpected error occurred: \(error)")
  return .failure(error)
  }
  }
@@ -248,16 +250,17 @@ public class FullNetworkingMockDelegate: NetworkingMockDelegate {
  defer { didHitRoute?(route) }
  switch route {
  case .vpn:
+ if let apiCredentialsResponseError {
+ return .failure(apiCredentialsResponseError)
+ }
  // for fetching client credentials
  guard let apiCredentials = apiCredentials else {
- guard let apiCredentialsResponseError else {
- return .failure(ResponseError(httpCode: HttpStatusCode.badRequest,
- responseCode: 2000,
- userFacingMessage: nil,
- underlyingError: nil)
- )
- }
- return .failure(apiCredentialsResponseError)
+ return .failure(ResponseError(
+ httpCode: HttpStatusCode.badRequest,
+ responseCode: 2000,
+ userFacingMessage: nil,
+ underlyingError: nil
+ ))
  }
 
  let data = try JSONSerialization.data(withJSONObject: apiCredentials.asDict)