From d645baf1dbdd5c5327d19dc8315c4c1d5256172e Mon Sep 17 00:00:00 2001 From: Kasper Tvede Date: Thu, 8 Dec 2022 11:36:35 +0100 Subject: [PATCH 1/4] add better logging to potential user's error. stacktraces at least makes it possible to know where the error occurred. bumped app auth --- Package.resolved | 4 ++-- Package.swift | 2 +- Sources/TIM/AppAuth/AppAuthController.swift | 3 +-- Sources/TIM/Models/Errors.swift | 2 +- 4 files changed, 5 insertions(+), 6 deletions(-) diff --git a/Package.resolved b/Package.resolved index 1c03815..1c3ea92 100644 --- a/Package.resolved +++ b/Package.resolved @@ -6,8 +6,8 @@ "repositoryURL": "https://github.com/openid/AppAuth-iOS", "state": { "branch": null, - "revision": "01131d68346c8ae552961c768d583c715fbe1410", - "version": "1.4.0" + "revision": "3d36a58a2b736f7bc499453e996a704929b25080", + "version": "1.6.0" } }, { diff --git a/Package.swift b/Package.swift index 8cd9bae..4851df9 100644 --- a/Package.swift +++ b/Package.swift @@ -13,7 +13,7 @@ let package = Package( targets: ["TIM"]), ], dependencies: [ - .package(name: "AppAuth", url: "https://github.com/openid/AppAuth-iOS", .exact("1.4.0")), + .package(name: "AppAuth", url: "https://github.com/openid/AppAuth-iOS", .exact("1.6.0")), .package(name: "TIMEncryptedStorage", url: "https://github.com/trifork/TIMEncryptedStorage-iOS", .exact("2.2.1")), ], diff --git a/Sources/TIM/AppAuth/AppAuthController.swift b/Sources/TIM/AppAuth/AppAuthController.swift index 1b2b667..94cd136 100644 --- a/Sources/TIM/AppAuth/AppAuthController.swift +++ b/Sources/TIM/AppAuth/AppAuthController.swift @@ -1,4 +1,3 @@ -import UIKit import AppAuth import SafariServices @@ -177,7 +176,7 @@ public final class AppAuthController: OpenIDConnectController { public func accessToken(forceRefresh: Bool, _ completion: @escaping (Result) -> Void) { guard let authState = self.authState else { - completion(.failure(TIMAuthError.authStateNil)) + completion(.failure(TIMAuthError.authStateNil())) return } if forceRefresh { diff --git a/Sources/TIM/Models/Errors.swift b/Sources/TIM/Models/Errors.swift index a2e183a..d906857 100644 --- a/Sources/TIM/Models/Errors.swift +++ b/Sources/TIM/Models/Errors.swift @@ -19,7 +19,7 @@ public enum TIMError: Error, LocalizedError { /// Errors related to AppAuth operations public enum TIMAuthError: Error, LocalizedError { - case authStateNil + case authStateNil(stacktrace: [String] = Thread.callStackSymbols) case failedToDiscoverConfiguration case failedToBeginAuth case failedToGetAccessToken From 225e27387954d8490b8fe13e9f51bc156918fa20 Mon Sep 17 00:00:00 2001 From: Kasper Tvede Date: Thu, 8 Dec 2022 11:40:22 +0100 Subject: [PATCH 2/4] add consideration + errorDescription updated --- Sources/TIM/Models/Errors.swift | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Sources/TIM/Models/Errors.swift b/Sources/TIM/Models/Errors.swift index d906857..a9480f3 100644 --- a/Sources/TIM/Models/Errors.swift +++ b/Sources/TIM/Models/Errors.swift @@ -19,6 +19,7 @@ public enum TIMError: Error, LocalizedError { /// Errors related to AppAuth operations public enum TIMAuthError: Error, LocalizedError { + //TODO consider renaming this to "missingLogin" or alike which communicates the real issue. case authStateNil(stacktrace: [String] = Thread.callStackSymbols) case failedToDiscoverConfiguration case failedToBeginAuth @@ -33,8 +34,8 @@ public enum TIMAuthError: Error, LocalizedError { public var errorDescription: String? { switch self { - case .authStateNil: - return "The AuthState was nil, when it wasn't expected to be. Are you trying to get the access token, when there was no valid session?" + case .authStateNil(let stacktrace): + return "The AuthState was nil, when it wasn't expected to be. Are you trying to get the access token, when there was no valid session? (stacktrace: \(stacktrace))" case .failedToDiscoverConfiguration: return "Failed to discover the configuration on the server. Check your configuration setup and try again." case .failedToBeginAuth: From 42dc6bdde09519825238c48d2fcd6feff08c5a6d Mon Sep 17 00:00:00 2001 From: Kasper Tvede Date: Thu, 8 Dec 2022 12:19:56 +0100 Subject: [PATCH 3/4] split errors from single file. added tests. --- Sources/TIM/Models/Errors.swift | 160 ----------------------- Sources/TIM/Models/TIMAuthError.swift | 77 +++++++++++ Sources/TIM/Models/TIMStorageError.swift | 71 ++++++++++ Sources/TIM/Models/TimError.swift | 18 +++ Tests/TIMTests/TIMAuthErrorTests.swift | 86 ++++++++++++ Tests/TIMTests/TestHelpers.swift | 10 ++ 6 files changed, 262 insertions(+), 160 deletions(-) delete mode 100644 Sources/TIM/Models/Errors.swift create mode 100644 Sources/TIM/Models/TIMAuthError.swift create mode 100644 Sources/TIM/Models/TIMStorageError.swift create mode 100644 Sources/TIM/Models/TimError.swift create mode 100644 Tests/TIMTests/TIMAuthErrorTests.swift create mode 100644 Tests/TIMTests/TestHelpers.swift diff --git a/Sources/TIM/Models/Errors.swift b/Sources/TIM/Models/Errors.swift deleted file mode 100644 index a9480f3..0000000 --- a/Sources/TIM/Models/Errors.swift +++ /dev/null @@ -1,160 +0,0 @@ -import Foundation -import AppAuth -import TIMEncryptedStorage - -/// Shared wrapper for Auth and Storage errors. -public enum TIMError: Error, LocalizedError { - case auth(TIMAuthError) - case storage(TIMStorageError) - - public var errorDescription: String? { - switch self { - case .auth(let error): - return error.localizedDescription - case .storage(let error): - return error.localizedDescription - } - } -} - -/// Errors related to AppAuth operations -public enum TIMAuthError: Error, LocalizedError { - //TODO consider renaming this to "missingLogin" or alike which communicates the real issue. - case authStateNil(stacktrace: [String] = Thread.callStackSymbols) - case failedToDiscoverConfiguration - case failedToBeginAuth - case failedToGetAccessToken - case failedToGetRefreshToken - case networkError - case refreshTokenExpired - case appAuthFailed(Error?) - case safariViewControllerCancelled - case failedToGetRequiredDataInToken - case failedToValidateIDToken - - public var errorDescription: String? { - switch self { - case .authStateNil(let stacktrace): - return "The AuthState was nil, when it wasn't expected to be. Are you trying to get the access token, when there was no valid session? (stacktrace: \(stacktrace))" - case .failedToDiscoverConfiguration: - return "Failed to discover the configuration on the server. Check your configuration setup and try again." - case .failedToBeginAuth: - return "AppAuth returned a weird state, while we tried to begin the authentication." - case .failedToGetAccessToken: - return "Failed to get the access token." - case .failedToGetRefreshToken: - return "Failed to get the refresh token." - case .networkError: - return "Network error caused by AppAuth" - case .refreshTokenExpired: - return "The refresh token has expired." - case .appAuthFailed(let error): - return "Something went wrong in the AppAuth framework: \(error?.localizedDescription ?? "nil")" - case .safariViewControllerCancelled: - return "The user cancelled OpenID connect login via SafariViewController" - case .failedToGetRequiredDataInToken: - return "TIM did not find the required data (userId) in the token. The 'sub' property must be present in the token!" - case .failedToValidateIDToken: - return "AppAuth failed to validate the ID Token. This will happen if the client's time is more than 10 minutes off the current time." - } - } - - static func mapAppAuthError(_ error: Error?) -> TIMAuthError { - guard let error = error as NSError? else { - return .appAuthFailed(nil) - } - - if error.domain == OIDGeneralErrorDomain { - switch error.code { - case OIDErrorCode.networkError.rawValue: return .networkError - case OIDErrorCode.idTokenFailedValidationError.rawValue: return .failedToValidateIDToken - default: return .appAuthFailed(error) - } - } else if error.domain == OIDOAuthTokenErrorDomain { - switch error.code { - case OIDErrorCodeOAuth.invalidGrant.rawValue: return .refreshTokenExpired - default: return .appAuthFailed(error) - } - } - - return .appAuthFailed(error) - } - - /// Tells whether the error was a `.safariViewControllerCancelled` or not. - public func isSafariViewControllerCancelled() -> Bool { - switch self { - case .safariViewControllerCancelled: - return true - default: - return false - } - } -} - -/// Errors related to storage operations -public enum TIMStorageError: Error, LocalizedError { - case encryptedStorageFailed(TIMEncryptedStorageError) - - // This error was implemented because we discovered missing keyIds for some users, where the user was cleared while waiting for response with a new refreshToken - case incompleteUserDataSet - - public var errorDescription: String? { - switch self { - case .encryptedStorageFailed(let error): - return "The encrypted storage failed: \(error.localizedDescription)" - case .incompleteUserDataSet: - return "Attempt to store a refresh token for a user, that does not have a valid data set. This can happen if you clear the user data while waiting for a login (which definitely should be avoided!). The invalid data has now been cleared from the framework. The user will have to perform OIDC login again." - } - } - - public func isKeyLocked() -> Bool { - isKeyServiceError(.keyLocked) - } - - public func isWrongPassword() -> Bool { - isKeyServiceError(.badPassword) - } - - public func isBiometricFailedError() -> Bool { - switch self { - case .encryptedStorageFailed(let storageError): - switch storageError { - case .secureStorageFailed(let secureStorageError) where secureStorageError == .authenticationFailedForData: - return true - default: - return false - } - case .incompleteUserDataSet: - return false - } - } - - /// Determines whether this error is an error thrown by the KeyService. - /// - /// This might be useful for handling unexpected cases from the encryption part of the framework. - /// When the key service fails you don't want to do any drastic fallback, since the server might "just" be down or the user have no internet connection. You will be able to recover later on, from a key service error. - public func isKeyServiceError() -> Bool { - isKeyServiceError(nil) - } - - /// Determines whether this error is a specific kind of key service error. - /// - Parameter keyServiceError: The key service error to look for. If `nil` is passed it will look for any kind of key service error. - private func isKeyServiceError(_ keyServiceError: TIMKeyServiceError?) -> Bool { - let isKeyServiceError: Bool - switch self { - case .encryptedStorageFailed(let error): - if case TIMEncryptedStorageError.keyServiceFailed(let ksError) = error { - if let keyServiceError = keyServiceError { - isKeyServiceError = ksError == keyServiceError - } else { - isKeyServiceError = true // Is any kind of key service error! - } - } else { - isKeyServiceError = false - } - case .incompleteUserDataSet: - isKeyServiceError = false - } - return isKeyServiceError - } -} diff --git a/Sources/TIM/Models/TIMAuthError.swift b/Sources/TIM/Models/TIMAuthError.swift new file mode 100644 index 0000000..97fe543 --- /dev/null +++ b/Sources/TIM/Models/TIMAuthError.swift @@ -0,0 +1,77 @@ +import Foundation +import AppAuth +import TIMEncryptedStorage + +/// Errors related to AppAuth operations +public enum TIMAuthError: Error, LocalizedError { + //TODO consider renaming this to "missingLogin" or alike which communicates the real issue. + case authStateNil(stacktrace: [String] = Thread.callStackSymbols) + case failedToDiscoverConfiguration + case failedToBeginAuth + case failedToGetAccessToken + case failedToGetRefreshToken + case networkError + case refreshTokenExpired + case appAuthFailed(Error?) + case safariViewControllerCancelled + case failedToGetRequiredDataInToken + case failedToValidateIDToken + + public var errorDescription: String? { + switch self { + case .authStateNil(let stacktrace): + return "The AuthState was nil, when it wasn't expected to be. Are you trying to get the access token, when there was no valid session? (stacktrace: \(stacktrace))" + case .failedToDiscoverConfiguration: + return "Failed to discover the configuration on the server. Check your configuration setup and try again." + case .failedToBeginAuth: + return "AppAuth returned a weird state, while we tried to begin the authentication." + case .failedToGetAccessToken: + return "Failed to get the access token." + case .failedToGetRefreshToken: + return "Failed to get the refresh token." + case .networkError: + return "Network error caused by AppAuth" + case .refreshTokenExpired: + return "The refresh token has expired." + case .appAuthFailed(let error): + return "Something went wrong in the AppAuth framework: \(error?.localizedDescription ?? "nil")" + case .safariViewControllerCancelled: + return "The user cancelled OpenID connect login via SafariViewController" + case .failedToGetRequiredDataInToken: + return "TIM did not find the required data (userId) in the token. The 'sub' property must be present in the token!" + case .failedToValidateIDToken: + return "AppAuth failed to validate the ID Token. This will happen if the client's time is more than 10 minutes off the current time." + } + } + + static func mapAppAuthError(_ error: Error?) -> TIMAuthError { + guard let error = error as NSError? else { + return .appAuthFailed(nil) + } + + switch error.domain { + case OIDGeneralErrorDomain: + switch error.code { + case OIDErrorCode.networkError.rawValue: return .networkError + case OIDErrorCode.idTokenFailedValidationError.rawValue: return .failedToValidateIDToken + default: break + } + case OIDOAuthTokenErrorDomain: + switch error.code { + case OIDErrorCodeOAuth.invalidGrant.rawValue: return .refreshTokenExpired + default: break + } + default: break + } + + return .appAuthFailed(error) + } + + /// Tells whether the error was a `.safariViewControllerCancelled` or not. + public func isSafariViewControllerCancelled() -> Bool { + if case .safariViewControllerCancelled = self { + return true + } + return false + } +} diff --git a/Sources/TIM/Models/TIMStorageError.swift b/Sources/TIM/Models/TIMStorageError.swift new file mode 100644 index 0000000..7fd5de5 --- /dev/null +++ b/Sources/TIM/Models/TIMStorageError.swift @@ -0,0 +1,71 @@ +import Foundation +import AppAuth +import TIMEncryptedStorage + +/// Errors related to storage operations +public enum TIMStorageError: Error, LocalizedError { + case encryptedStorageFailed(TIMEncryptedStorageError) + + // This error was implemented because we discovered missing keyIds for some users, where the user was cleared while waiting for response with a new refreshToken + case incompleteUserDataSet + + public var errorDescription: String? { + switch self { + case .encryptedStorageFailed(let error): + return "The encrypted storage failed: \(error.localizedDescription)" + case .incompleteUserDataSet: + return "Attempt to store a refresh token for a user, that does not have a valid data set. This can happen if you clear the user data while waiting for a login (which definitely should be avoided!). The invalid data has now been cleared from the framework. The user will have to perform OIDC login again." + } + } + + public func isKeyLocked() -> Bool { + isKeyServiceError(.keyLocked) + } + + public func isWrongPassword() -> Bool { + isKeyServiceError(.badPassword) + } + + public func isBiometricFailedError() -> Bool { + switch self { + case .encryptedStorageFailed(let storageError): + switch storageError { + case .secureStorageFailed(let secureStorageError) where secureStorageError == .authenticationFailedForData: + return true + default: + return false + } + case .incompleteUserDataSet: + return false + } + } + + /// Determines whether this error is an error thrown by the KeyService. + /// + /// This might be useful for handling unexpected cases from the encryption part of the framework. + /// When the key service fails you don't want to do any drastic fallback, since the server might "just" be down or the user have no internet connection. You will be able to recover later on, from a key service error. + public func isKeyServiceError() -> Bool { + isKeyServiceError(nil) + } + + /// Determines whether this error is a specific kind of key service error. + /// - Parameter keyServiceError: The key service error to look for. If `nil` is passed it will look for any kind of key service error. + private func isKeyServiceError(_ keyServiceError: TIMKeyServiceError?) -> Bool { + let isKeyServiceError: Bool + switch self { + case .encryptedStorageFailed(let error): + if case TIMEncryptedStorageError.keyServiceFailed(let ksError) = error { + if let keyServiceError = keyServiceError { + isKeyServiceError = ksError == keyServiceError + } else { + isKeyServiceError = true // Is any kind of key service error! + } + } else { + isKeyServiceError = false + } + case .incompleteUserDataSet: + isKeyServiceError = false + } + return isKeyServiceError + } +} diff --git a/Sources/TIM/Models/TimError.swift b/Sources/TIM/Models/TimError.swift new file mode 100644 index 0000000..d7dd338 --- /dev/null +++ b/Sources/TIM/Models/TimError.swift @@ -0,0 +1,18 @@ +import Foundation +import AppAuth +import TIMEncryptedStorage + +/// Shared wrapper for Auth and Storage errors. +public enum TIMError: Error, LocalizedError { + case auth(TIMAuthError) + case storage(TIMStorageError) + + public var errorDescription: String? { + switch self { + case .auth(let error): + return error.localizedDescription + case .storage(let error): + return error.localizedDescription + } + } +} diff --git a/Tests/TIMTests/TIMAuthErrorTests.swift b/Tests/TIMTests/TIMAuthErrorTests.swift new file mode 100644 index 0000000..cb67bb7 --- /dev/null +++ b/Tests/TIMTests/TIMAuthErrorTests.swift @@ -0,0 +1,86 @@ +import XCTest +import AppAuth +@testable import TIM + +class TimErrorTests: XCTestCase { + + func testIsSafariViewControllerCancelled(){ + XCTAssertFalse(TIMAuthError.failedToBeginAuth.isSafariViewControllerCancelled()) + XCTAssertFalse(TIMAuthError.appAuthFailed(nil).isSafariViewControllerCancelled()) + XCTAssertFalse(TIMAuthError.authStateNil().isSafariViewControllerCancelled()) + XCTAssertFalse(TIMAuthError.failedToValidateIDToken.isSafariViewControllerCancelled()) + XCTAssertFalse(TIMAuthError.refreshTokenExpired.isSafariViewControllerCancelled()) + + XCTAssertTrue(TIMAuthError.safariViewControllerCancelled.isSafariViewControllerCancelled()) + } + + func testMapAppAuthErrorNil(){ + let error = TIMAuthError.mapAppAuthError(nil) + error.assertAppAuthFailed(expectedError: nil) + } + + func testMapAppAuthErrorRegularNSError(){ + let innerAuthError = NSError(domain: "someDomain", code: 42) + let error = TIMAuthError.mapAppAuthError(innerAuthError) + error.assertAppAuthFailed(expectedError: innerAuthError) + } + + func testMapAppAuthErrorOIDGeneralErrorDomainNetworkError() { + let innerAuthError = NSError( + domain: OIDGeneralErrorDomain, + code: OIDErrorCode.networkError.rawValue + ) + + let error = TIMAuthError.mapAppAuthError(innerAuthError) + error.assertNetworkError() + } + + func testMapAppAuthErrorOIDGeneralErrorDomainIdTokenError() { + let innerAuthError = NSError( + domain: OIDGeneralErrorDomain, + code: OIDErrorCode.idTokenFailedValidationError.rawValue + ) + + let error = TIMAuthError.mapAppAuthError(innerAuthError) + error.assertFailedToValidateIDToken() + } + + func testMapAppAuthErrorOIDGeneralErrorDomainUnknownErrorType(){ + let innerAuthError = NSError( + domain: OIDGeneralErrorDomain, + code: 999 + ) + + let error = TIMAuthError.mapAppAuthError(innerAuthError) + error.assertAppAuthFailed(expectedError: innerAuthError) + } + + + +} + +extension TIMAuthError { + func assertAppAuthFailed(expectedError: NSError?){ + if case TIMAuthError.appAuthFailed(let appAuthError) = self { + XCTAssertEqual(expectedError, appAuthError as? NSError) + } else { + XCTFailTest(message: "Expected an appAuthFailed but got \(self)") + } + } + + func assertNetworkError(){ + if case TIMAuthError.networkError = self { + + } else { + XCTFailTest(message: "Expected a networkError but got \(self)") + } + } + + func assertFailedToValidateIDToken(){ + if case TIMAuthError.failedToValidateIDToken = self { + + } else { + XCTFailTest(message: "Expected a failedToValidateIDToken but got \(self)") + } + } +} diff --git a/Tests/TIMTests/TestHelpers.swift b/Tests/TIMTests/TestHelpers.swift new file mode 100644 index 0000000..be488ab --- /dev/null +++ b/Tests/TIMTests/TestHelpers.swift @@ -0,0 +1,10 @@ +import XCTest +import Foundation + +func XCTFailTest( + message: String = "Should not have been called", + file: StaticString = #filePath, + line: UInt = #line +){ + XCTAssertTrue(false, message, file: file, line: line) +} From 0fcedfdc8d5b99dfd7ec1fb89b6b4b73f9f06f69 Mon Sep 17 00:00:00 2001 From: Kasper Tvede Date: Thu, 8 Dec 2022 12:24:45 +0100 Subject: [PATCH 4/4] finish adding tests --- Tests/TIMTests/TIMAuthErrorTests.swift | 30 ++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/Tests/TIMTests/TIMAuthErrorTests.swift b/Tests/TIMTests/TIMAuthErrorTests.swift index cb67bb7..b7e04ae 100644 --- a/Tests/TIMTests/TIMAuthErrorTests.swift +++ b/Tests/TIMTests/TIMAuthErrorTests.swift @@ -2,7 +2,7 @@ import XCTest import AppAuth @testable import TIM -class TimErrorTests: XCTestCase { +final class TimErrorTests: XCTestCase { func testIsSafariViewControllerCancelled(){ XCTAssertFalse(TIMAuthError.failedToBeginAuth.isSafariViewControllerCancelled()) @@ -55,7 +55,25 @@ class TimErrorTests: XCTestCase { error.assertAppAuthFailed(expectedError: innerAuthError) } - + func testMapAppAuthErrorOIDOAuthTokenErrorDomainInvalidGrant(){ + let innerAuthError = NSError( + domain: OIDOAuthTokenErrorDomain, + code: OIDErrorCodeOAuth.invalidGrant.rawValue + ) + + let error = TIMAuthError.mapAppAuthError(innerAuthError) + error.assertRefreshTokenExpired() + } + + func testMapAppAuthErrorOIDOAuthTokenErrorDomainUnknownErrorType(){ + let innerAuthError = NSError( + domain: OIDOAuthTokenErrorDomain, + code: 999 + ) + + let error = TIMAuthError.mapAppAuthError(innerAuthError) + error.assertAppAuthFailed(expectedError: innerAuthError) + } } @@ -83,4 +101,12 @@ extension TIMAuthError { XCTFailTest(message: "Expected a failedToValidateIDToken but got \(self)") } } + + func assertRefreshTokenExpired(){ + if case TIMAuthError.refreshTokenExpired = self { + + } else { + XCTFailTest(message: "Expected a refreshTokenExpired but got \(self)") + } + } }