From 762dd914efc1a7ba645cf766d6e3a80ccf60a2fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrien=20Coye=20de=20Brune=CC=81lis?= Date: Thu, 6 Jun 2024 11:51:11 +0200 Subject: [PATCH 1/6] fix: Logout on API token removed remotely --- kDrive/AppDelegate.swift | 4 +- .../Controller/Menu/MenuViewController.swift | 15 +--- kDriveCore/Data/Api/DriveApiFetcher.swift | 80 +++++++++---------- kDriveCore/Data/Cache/AccountManager.swift | 35 ++++++-- .../Operation/UploadOperation+Error.swift | 19 ++++- 5 files changed, 85 insertions(+), 68 deletions(-) diff --git a/kDrive/AppDelegate.swift b/kDrive/AppDelegate.swift index 74e3fa56d..642735066 100644 --- a/kDrive/AppDelegate.swift +++ b/kDrive/AppDelegate.swift @@ -459,9 +459,7 @@ final class AppDelegate: UIResponder, UIApplicationDelegate, AccountManagerDeleg // MARK: - Account manager delegate func currentAccountNeedsAuthentication() { - Task { @MainActor in - setRootViewController(SwitchUserViewController.instantiateInNavigationController()) - } + Log.appDelegate("currentAccountNeedsAuthentication", level: .error) } // MARK: - State restoration diff --git a/kDrive/UI/Controller/Menu/MenuViewController.swift b/kDrive/UI/Controller/Menu/MenuViewController.swift index ceb0eee6f..64c4ae40d 100644 --- a/kDrive/UI/Controller/Menu/MenuViewController.swift +++ b/kDrive/UI/Controller/Menu/MenuViewController.swift @@ -286,20 +286,7 @@ extension MenuViewController { .alertRemoveUserDescription(currentAccount?.user.displayName ?? ""), action: KDriveResourcesStrings.Localizable.buttonConfirm, destructive: true) { - if let currentAccount = self.accountManager.currentAccount { - self.accountManager.removeTokenAndAccount(account: currentAccount) - } - - let appDelegate = (UIApplication.shared.delegate as? AppDelegate) - - if let nextAccount = self.accountManager.accounts.first { - self.accountManager.switchAccount(newAccount: nextAccount) - appDelegate?.refreshCacheScanLibraryAndUpload(preload: true, isSwitching: true) - } else { - SentrySDK.setUser(nil) - } - self.accountManager.saveAccounts() - appDelegate?.updateRootViewControllerState() + self.accountManager.switchToNextAvailableAccountOrLogout() } present(alert, animated: true) case .help: diff --git a/kDriveCore/Data/Api/DriveApiFetcher.swift b/kDriveCore/Data/Api/DriveApiFetcher.swift index 5c611c9c8..4651360b7 100644 --- a/kDriveCore/Data/Api/DriveApiFetcher.swift +++ b/kDriveCore/Data/Api/DriveApiFetcher.swift @@ -22,6 +22,7 @@ import InfomaniakCore import InfomaniakDI import InfomaniakLogin import Kingfisher +import Sentry import UIKit public extension ApiFetcher { @@ -475,6 +476,37 @@ class SyncedAuthenticator: OAuthAuthenticator { @LazyInjectService var appContextService: AppContextServiceable @LazyInjectService var keychainHelper: KeychainHelper + func handleFailedRefreshingToken(oldToken: ApiToken, error: Error?) -> Result { + guard let error else { + // Couldn't refresh the token, keep the old token and fetch it later. Maybe because of bad network ? + SentrySDK + .addBreadcrumb(oldToken.generateBreadcrumb(level: .error, + message: "Refreshing token failed - Other \(error.debugDescription)")) + return .failure(DriveError.unknownError) + } + + if case .noRefreshToken = (error as? InfomaniakLoginError) { + // Couldn't refresh the token because we don't have a refresh token + SentrySDK + .addBreadcrumb(oldToken.generateBreadcrumb(level: .error, + message: "Refreshing token failed - Cannot refresh infinite token")) + refreshTokenDelegate?.didFailRefreshToken(oldToken) + return .failure(error) + } + + if (error as NSError).domain == "invalid_grant" { + // Couldn't refresh the token, API says it's invalid + SentrySDK + .addBreadcrumb(oldToken.generateBreadcrumb(level: .error, + message: "Refreshing token failed - Invalid grant")) + refreshTokenDelegate?.didFailRefreshToken(oldToken) + return .failure(error) + } + + // Something else happened + return .failure(error) + } + override func refresh( _ credential: OAuthAuthenticator.Credential, for session: Session, @@ -513,29 +545,9 @@ class SyncedAuthenticator: OAuthAuthenticator { } } - let group = DispatchGroup() - group.enter() - var taskIdentifier: UIBackgroundTaskIdentifier = .invalid - if !self.appContextService.isExtension { - // It is absolutely necessary that the app stays awake while we refresh the token - taskIdentifier = UIApplication.shared.beginBackgroundTask(withName: "Refresh token") { - let message = "Refreshing token failed - Background task expired" - SentryDebug.addBreadcrumb(message: message, category: .apiToken, level: .error, metadata: metadata) - - // If we didn't fetch the new token in the given time there is not much we can do apart from hoping that it - // wasn't revoked - if taskIdentifier != .invalid { - UIApplication.shared.endBackgroundTask(taskIdentifier) - taskIdentifier = .invalid - } - } - - if taskIdentifier == .invalid { - // We couldn't request additional time to refresh token maybe try later... - completion(.failure(DriveError.refreshToken)) - return - } - } + // It is necessary that the app stays awake while we refresh the token + let expiringActivity = ExpiringActivity() + expiringActivity.start() self.tokenable.refreshToken(token: credential) { token, error in // New token has been fetched correctly if let token { @@ -545,28 +557,10 @@ class SyncedAuthenticator: OAuthAuthenticator { self.refreshTokenDelegate?.didUpdateToken(newToken: token, oldToken: credential) completion(.success(token)) } else { - // Couldn't refresh the token, API says it's invalid - if let error = error as NSError?, error.domain == "invalid_grant" { - let message = "Refreshing token failed - Invalid grant" - SentryDebug.addBreadcrumb(message: message, category: .apiToken, level: .error, metadata: metadata) - - self.refreshTokenDelegate?.didFailRefreshToken(credential) - completion(.failure(error)) - } else { - // Couldn't refresh the token, keep the old token and fetch it later. Maybe because of bad network ? - let message = "Refreshing token failed - Other \(error.debugDescription)" - SentryDebug.addBreadcrumb(message: message, category: .apiToken, level: .error, metadata: metadata) - - completion(.success(credential)) - } - } - if taskIdentifier != .invalid { - UIApplication.shared.endBackgroundTask(taskIdentifier) - taskIdentifier = .invalid + completion(self.handleFailedRefreshingToken(oldToken: credential, error: error)) } - group.leave() + expiringActivity.endAll() } - group.wait() } } } diff --git a/kDriveCore/Data/Cache/AccountManager.swift b/kDriveCore/Data/Cache/AccountManager.swift index 5977d154f..2da225c51 100644 --- a/kDriveCore/Data/Cache/AccountManager.swift +++ b/kDriveCore/Data/Cache/AccountManager.swift @@ -30,6 +30,8 @@ public protocol UpdateAccountDelegate: AnyObject { public protocol AccountManagerDelegate: AnyObject { func currentAccountNeedsAuthentication() + func refreshCacheScanLibraryAndUpload(preload: Bool, isSwitching: Bool) + func updateRootViewControllerState() } public extension InfomaniakLogin { @@ -81,6 +83,7 @@ public protocol AccountManageable: AnyObject { func removeTokenAndAccount(account: Account) func account(for token: ApiToken) -> Account? func account(for userId: Int) -> Account? + func switchToNextAvailableAccountOrLogout() } public class AccountManager: RefreshTokenDelegate, AccountManageable { @@ -245,13 +248,16 @@ public class AccountManager: RefreshTokenDelegate, AccountManageable { let context = ["User id": token.userId, "Expiration date": token.expirationDate?.timeIntervalSince1970 ?? "Infinite"] as [String: Any] SentryDebug.capture(message: "Failed refreshing token", context: context, contextKey: "Token Infos") - tokenStore.removeTokenFor(userId: token.userId) - if let account = account(for: token), - account.userId == currentUserId { - delegate?.currentAccountNeedsAuthentication() - notificationHelper.sendDisconnectedNotification() - } + + DownloadQueue.instance.suspendAllOperations() + + @InjectService var uploadQueue: UploadQueue + uploadQueue.suspendAllOperations() + + delegate?.currentAccountNeedsAuthentication() + switchToNextAvailableAccountOrLogout() + notificationHelper.sendDisconnectedNotification() } public func createAndSetCurrentAccount(code: String, codeVerifier: String) async throws -> Account { @@ -434,4 +440,21 @@ public class AccountManager: RefreshTokenDelegate, AccountManageable { public func account(for userId: Int) -> Account? { return accounts.first { $0.userId == userId } } + + public func switchToNextAvailableAccountOrLogout() { + Task { @MainActor in + if let currentAccount { + removeTokenAndAccount(account: currentAccount) + } + + if let nextAccount = accounts.first { + switchAccount(newAccount: nextAccount) + delegate?.refreshCacheScanLibraryAndUpload(preload: true, isSwitching: true) + } else { + SentrySDK.setUser(nil) + } + saveAccounts() + delegate?.updateRootViewControllerState() + } + } } diff --git a/kDriveCore/Data/UploadQueue/Operation/UploadOperation+Error.swift b/kDriveCore/Data/UploadQueue/Operation/UploadOperation+Error.swift index 6ecfbc153..f9cfc2bf6 100644 --- a/kDriveCore/Data/UploadQueue/Operation/UploadOperation+Error.swift +++ b/kDriveCore/Data/UploadQueue/Operation/UploadOperation+Error.swift @@ -16,8 +16,10 @@ along with this program. If not, see . */ +import Alamofire import Foundation import InfomaniakCore +import InfomaniakDI extension UploadOperation { /// Enqueue a task, while making sure we catch the errors in a standard way @@ -145,8 +147,21 @@ extension UploadOperation { @discardableResult func handleRemoteErrors(error: Error) -> Bool { guard let error = error as? DriveError, (error.type == .networkError) || (error.type == .serverError) else { - Log.uploadOperation("error:\(error) not a remote one ufid:\(uploadFileId)") - return false + // TODO: App level interceptor when the auth stack was unable to keep a valid session to log out + guard let authError = error as? AuthenticationError, + authError == .excessiveRefresh else { + Log.uploadOperation("error:\(error) not a remote one ufid:\(uploadFileId)") + return false + } + + @InjectService var accountManager: AccountManageable + if let currentAccount = accountManager.currentAccount { + Log.uploadOperation("log out currentAccount:\(currentAccount)") + accountManager.removeTokenAndAccount(account: currentAccount) + } + + Log.uploadOperation("auth error:\(error), disconnecting ufid:\(uploadFileId)") + return true } var errorHandled = false From 13f5ac59e41e0505f6d1feded211a5d567b7be24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrien=20Coye=20de=20Brune=CC=81lis?= Date: Thu, 6 Jun 2024 13:00:19 +0200 Subject: [PATCH 2/6] chore: PR Feedback --- .../Operation/UploadOperation+Error.swift | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/kDriveCore/Data/UploadQueue/Operation/UploadOperation+Error.swift b/kDriveCore/Data/UploadQueue/Operation/UploadOperation+Error.swift index f9cfc2bf6..6ecfbc153 100644 --- a/kDriveCore/Data/UploadQueue/Operation/UploadOperation+Error.swift +++ b/kDriveCore/Data/UploadQueue/Operation/UploadOperation+Error.swift @@ -16,10 +16,8 @@ along with this program. If not, see . */ -import Alamofire import Foundation import InfomaniakCore -import InfomaniakDI extension UploadOperation { /// Enqueue a task, while making sure we catch the errors in a standard way @@ -147,21 +145,8 @@ extension UploadOperation { @discardableResult func handleRemoteErrors(error: Error) -> Bool { guard let error = error as? DriveError, (error.type == .networkError) || (error.type == .serverError) else { - // TODO: App level interceptor when the auth stack was unable to keep a valid session to log out - guard let authError = error as? AuthenticationError, - authError == .excessiveRefresh else { - Log.uploadOperation("error:\(error) not a remote one ufid:\(uploadFileId)") - return false - } - - @InjectService var accountManager: AccountManageable - if let currentAccount = accountManager.currentAccount { - Log.uploadOperation("log out currentAccount:\(currentAccount)") - accountManager.removeTokenAndAccount(account: currentAccount) - } - - Log.uploadOperation("auth error:\(error), disconnecting ufid:\(uploadFileId)") - return true + Log.uploadOperation("error:\(error) not a remote one ufid:\(uploadFileId)") + return false } var errorHandled = false From ef8acad3aa09600e92f42a63215c8038601597fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrien=20Coye=20de=20Brune=CC=81lis?= Date: Thu, 6 Jun 2024 14:31:32 +0200 Subject: [PATCH 3/6] fix: Only logout if account is matching --- .../Controller/Menu/MenuViewController.swift | 2 +- kDriveCore/Data/Cache/AccountManager.swift | 31 ++++++++++++++----- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/kDrive/UI/Controller/Menu/MenuViewController.swift b/kDrive/UI/Controller/Menu/MenuViewController.swift index 64c4ae40d..caeb8e3d8 100644 --- a/kDrive/UI/Controller/Menu/MenuViewController.swift +++ b/kDrive/UI/Controller/Menu/MenuViewController.swift @@ -286,7 +286,7 @@ extension MenuViewController { .alertRemoveUserDescription(currentAccount?.user.displayName ?? ""), action: KDriveResourcesStrings.Localizable.buttonConfirm, destructive: true) { - self.accountManager.switchToNextAvailableAccountOrLogout() + self.accountManager.logoutCurrentAccountAndSwitchToNextIfPossible() } present(alert, animated: true) case .help: diff --git a/kDriveCore/Data/Cache/AccountManager.swift b/kDriveCore/Data/Cache/AccountManager.swift index 2da225c51..f0da10d56 100644 --- a/kDriveCore/Data/Cache/AccountManager.swift +++ b/kDriveCore/Data/Cache/AccountManager.swift @@ -83,7 +83,7 @@ public protocol AccountManageable: AnyObject { func removeTokenAndAccount(account: Account) func account(for token: ApiToken) -> Account? func account(for userId: Int) -> Account? - func switchToNextAvailableAccountOrLogout() + func logoutCurrentAccountAndSwitchToNextIfPossible() } public class AccountManager: RefreshTokenDelegate, AccountManageable { @@ -248,16 +248,30 @@ public class AccountManager: RefreshTokenDelegate, AccountManageable { let context = ["User id": token.userId, "Expiration date": token.expirationDate?.timeIntervalSince1970 ?? "Infinite"] as [String: Any] SentryDebug.capture(message: "Failed refreshing token", context: context, contextKey: "Token Infos") + + let tokenUserId = token.userId tokenStore.removeTokenFor(userId: token.userId) - DownloadQueue.instance.suspendAllOperations() + // Remove matching account + guard let accountToDelete = accounts.first(where: { account in + account.userId == tokenUserId + }) else { + SentryDebug.capture(message: "Failed matching failed token to account \(tokenUserId)", + context: context, + contextKey: "Token Infos") + DDLogError("Failed matching failed token to account \(tokenUserId)") + return + } - @InjectService var uploadQueue: UploadQueue - uploadQueue.suspendAllOperations() + if accountToDelete == currentAccount { + DDLogInfo("matched \(currentAccount) to \(accountToDelete), removing current account") - delegate?.currentAccountNeedsAuthentication() - switchToNextAvailableAccountOrLogout() - notificationHelper.sendDisconnectedNotification() + delegate?.currentAccountNeedsAuthentication() + logoutCurrentAccountAndSwitchToNextIfPossible() + } else { + DDLogInfo("user with token error \(accountToDelete) do not match current account, doing nothing") + removeAccount(toDeleteAccount: accountToDelete) + } } public func createAndSetCurrentAccount(code: String, codeVerifier: String) async throws -> Account { @@ -441,10 +455,11 @@ public class AccountManager: RefreshTokenDelegate, AccountManageable { return accounts.first { $0.userId == userId } } - public func switchToNextAvailableAccountOrLogout() { + public func logoutCurrentAccountAndSwitchToNextIfPossible() { Task { @MainActor in if let currentAccount { removeTokenAndAccount(account: currentAccount) + notificationHelper.sendDisconnectedNotification() } if let nextAccount = accounts.first { From e3de79be687b77486b2559dd5480b8bf18e543be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrien=20Coye=20de=20Brune=CC=81lis?= Date: Thu, 6 Jun 2024 15:04:20 +0200 Subject: [PATCH 4/6] fix: Fix UT mocks --- kDriveTests/kDrive/Launch/MockAccountManager.swift | 2 ++ 1 file changed, 2 insertions(+) diff --git a/kDriveTests/kDrive/Launch/MockAccountManager.swift b/kDriveTests/kDrive/Launch/MockAccountManager.swift index c624c4122..eb2d9b0a1 100644 --- a/kDriveTests/kDrive/Launch/MockAccountManager.swift +++ b/kDriveTests/kDrive/Launch/MockAccountManager.swift @@ -86,4 +86,6 @@ class MockAccountManager: AccountManageable, RefreshTokenDelegate { func account(for userId: Int) -> Account? { fatalError("Not implemented") } func updateToken(newToken: ApiToken, oldToken: ApiToken) {} + + func logoutCurrentAccountAndSwitchToNextIfPossible() { fatalError("Not implemented") } } From e10308643d0f3aa6d202664bdc2cf6c9ab3bbc65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrien=20Coye=20de=20Brune=CC=81lis?= Date: Thu, 6 Jun 2024 15:09:59 +0200 Subject: [PATCH 5/6] chore: PR Feedback --- kDrive/AppDelegate.swift | 6 ------ kDriveCore/Data/Cache/AccountManager.swift | 3 --- 2 files changed, 9 deletions(-) diff --git a/kDrive/AppDelegate.swift b/kDrive/AppDelegate.swift index d964d94af..0c3e779df 100644 --- a/kDrive/AppDelegate.swift +++ b/kDrive/AppDelegate.swift @@ -460,12 +460,6 @@ final class AppDelegate: UIResponder, UIApplicationDelegate, AccountManagerDeleg } } - // MARK: - Account manager delegate - - func currentAccountNeedsAuthentication() { - Log.appDelegate("currentAccountNeedsAuthentication", level: .error) - } - // MARK: - State restoration func application(_ application: UIApplication, shouldSaveApplicationState coder: NSCoder) -> Bool { diff --git a/kDriveCore/Data/Cache/AccountManager.swift b/kDriveCore/Data/Cache/AccountManager.swift index f0da10d56..4e9eecf25 100644 --- a/kDriveCore/Data/Cache/AccountManager.swift +++ b/kDriveCore/Data/Cache/AccountManager.swift @@ -29,7 +29,6 @@ public protocol UpdateAccountDelegate: AnyObject { } public protocol AccountManagerDelegate: AnyObject { - func currentAccountNeedsAuthentication() func refreshCacheScanLibraryAndUpload(preload: Bool, isSwitching: Bool) func updateRootViewControllerState() } @@ -265,8 +264,6 @@ public class AccountManager: RefreshTokenDelegate, AccountManageable { if accountToDelete == currentAccount { DDLogInfo("matched \(currentAccount) to \(accountToDelete), removing current account") - - delegate?.currentAccountNeedsAuthentication() logoutCurrentAccountAndSwitchToNextIfPossible() } else { DDLogInfo("user with token error \(accountToDelete) do not match current account, doing nothing") From f4c7a8b42794807be39597ee18960621fcadf40e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrien=20Coye=20de=20Brune=CC=81lis?= Date: Fri, 7 Jun 2024 08:58:31 +0200 Subject: [PATCH 6/6] chore: PR Feedback --- kDriveCore/Data/Cache/AccountManager.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kDriveCore/Data/Cache/AccountManager.swift b/kDriveCore/Data/Cache/AccountManager.swift index 4e9eecf25..2efadacf8 100644 --- a/kDriveCore/Data/Cache/AccountManager.swift +++ b/kDriveCore/Data/Cache/AccountManager.swift @@ -264,6 +264,7 @@ public class AccountManager: RefreshTokenDelegate, AccountManageable { if accountToDelete == currentAccount { DDLogInfo("matched \(currentAccount) to \(accountToDelete), removing current account") + notificationHelper.sendDisconnectedNotification() logoutCurrentAccountAndSwitchToNextIfPossible() } else { DDLogInfo("user with token error \(accountToDelete) do not match current account, doing nothing") @@ -456,7 +457,6 @@ public class AccountManager: RefreshTokenDelegate, AccountManageable { Task { @MainActor in if let currentAccount { removeTokenAndAccount(account: currentAccount) - notificationHelper.sendDisconnectedNotification() } if let nextAccount = accounts.first {