From ad6e1aa067df380d0bad12b1d52d6b61641ed8f4 Mon Sep 17 00:00:00 2001 From: Oleg Gnidets Date: Wed, 1 Dec 2021 18:15:46 +0200 Subject: [PATCH 1/3] Improve some error handling --- Source/OktaError.swift | 24 +++++++------- Source/RestAPI/OktaAPIRequest.swift | 32 +++++++++++-------- .../Statuses/OktaAuthStatePasswordReset.swift | 2 +- Source/Statuses/OktaAuthStatus.swift | 4 +-- .../OktaAuthStatusFactorChallenge.swift | 4 +-- .../Statuses/OktaAuthStatusFactorEnroll.swift | 4 +-- .../OktaAuthStatusFactorEnrollActivate.swift | 6 ++-- .../OktaAuthStatusFactorRequired.swift | 4 +-- .../OktaAuthStatusPasswordExpired.swift | 2 +- .../OktaAuthStatusPasswordWarning.swift | 2 +- Source/Statuses/OktaAuthStatusRecovery.swift | 2 +- .../OktaAuthStatusRecoveryChallenge.swift | 6 ++-- .../OktaAuthStatusResponseHandler.swift | 2 +- 13 files changed, 50 insertions(+), 44 deletions(-) diff --git a/Source/OktaError.swift b/Source/OktaError.swift index 0ba9e39..a022325 100644 --- a/Source/OktaError.swift +++ b/Source/OktaError.swift @@ -12,11 +12,11 @@ import Foundation -public enum OktaError: Error { - case errorBuildingURLRequest +public enum OktaError: LocalizedError { + case errorBuildingURLRequest(String) case connectionError(Error) case emptyServerResponse - case invalidResponse + case invalidResponse(String) case responseSerializationError(Error, Data) case serverRespondedWithError(OktaAPIErrorResponse) case unexpectedResponse @@ -30,14 +30,14 @@ public enum OktaError: Error { public extension OktaError { var description: String { switch self { - case .errorBuildingURLRequest: - return "Error building URL request" + case let .errorBuildingURLRequest(reason): + return "Error building URL request.\nReason Failure: \(reason)." case .connectionError(let error): return "Connection error (\(error.localizedDescription))" case .emptyServerResponse: return "Empty server response" - case .invalidResponse: - return "Invalid server response" + case let .invalidResponse(error): + return "Invalid server response: \(error)" case .responseSerializationError(let error, _): return "Response serialization error (\(error.localizedDescription))" case .serverRespondedWithError(let error): @@ -58,14 +58,14 @@ public extension OktaError { return "Another request is in progress" case .unknownStatus: return "Received state is unknown" - case .internalError: - return "Internal error" - case .invalidParameters: - return "Invalid parameters" + case let .internalError(error): + return "Internal error: \(error)" + case let .invalidParameters(error): + return "Invalid parameters: \(error)" } } var localizedDescription: String { - return NSLocalizedString(self.description, comment: "") + NSLocalizedString(description, comment: "") } } diff --git a/Source/RestAPI/OktaAPIRequest.swift b/Source/RestAPI/OktaAPIRequest.swift index 98480f8..c4f64dd 100644 --- a/Source/RestAPI/OktaAPIRequest.swift +++ b/Source/RestAPI/OktaAPIRequest.swift @@ -52,16 +52,17 @@ public class OktaAPIRequest { case get, post, put, delete, options } - public func buildRequest() -> URLRequest? { + public func buildRequest() throws -> URLRequest { guard var components = URLComponents(url: baseURL, resolvingAgainstBaseURL: true) else { - return nil + throw OktaError.errorBuildingURLRequest("Cannot parse URL components") } + if let path = path { components.path = path } components.queryItems = urlParams?.map { URLQueryItem(name: $0.key, value: $0.value) } guard let url = components.url else { - return nil + throw OktaError.errorBuildingURLRequest("Cannot create URL from components") } var urlRequest = URLRequest(url: url, cachePolicy: .reloadIgnoringLocalAndRemoteCacheData, timeoutInterval: 60) @@ -73,8 +74,9 @@ public class OktaAPIRequest { if let bodyParams = bodyParams { guard let body = try? JSONSerialization.data(withJSONObject: bodyParams, options: []) else { - return nil + throw OktaError.errorBuildingURLRequest("Cannot parse `bodyParams` to JSON data.") } + urlRequest.httpBody = body } @@ -82,18 +84,22 @@ public class OktaAPIRequest { } public func run() { - guard isCancelled == false else { - return - } - guard let urlRequest = buildRequest() else { - completion(self, .error(.errorBuildingURLRequest)) + if isCancelled { return } + + do { + let urlRequest = try buildRequest() - if let httpClient = httpClient { - performRequest(urlRequest, withHTTPClient: httpClient) - } else { - performRequest(urlRequest, withURLSession: urlSession) + if let httpClient = httpClient { + performRequest(urlRequest, withHTTPClient: httpClient) + } else { + performRequest(urlRequest, withURLSession: urlSession) + } + } catch let oktaError as OktaError { + completion(self, .error(oktaError)) + } catch { + completion(self, .error(.errorBuildingURLRequest("Unknown error"))) } } diff --git a/Source/Statuses/OktaAuthStatePasswordReset.swift b/Source/Statuses/OktaAuthStatePasswordReset.swift index e820c37..c2f1c97 100644 --- a/Source/Statuses/OktaAuthStatePasswordReset.swift +++ b/Source/Statuses/OktaAuthStatePasswordReset.swift @@ -18,7 +18,7 @@ open class OktaAuthStatusPasswordReset : OktaAuthStatus { public override init(currentState: OktaAuthStatus, model: OktaAPISuccessResponse) throws { guard let stateToken = model.stateToken else { - throw OktaError.invalidResponse + throw OktaError.invalidResponse("State token is missed") } self.stateToken = stateToken if let policy = model.embedded?.policy { diff --git a/Source/Statuses/OktaAuthStatus.swift b/Source/Statuses/OktaAuthStatus.swift index 0941ccd..ab38c6b 100644 --- a/Source/Statuses/OktaAuthStatus.swift +++ b/Source/Statuses/OktaAuthStatus.swift @@ -73,7 +73,7 @@ open class OktaAuthStatus { } guard let stateToken = model.stateToken else { - onError(.invalidResponse) + onError(.invalidResponse("State token is missed")) return } @@ -106,7 +106,7 @@ open class OktaAuthStatus { return } guard let stateToken = model.stateToken else { - onError?(.invalidResponse) + onError?(.invalidResponse("State token is missed")) return } diff --git a/Source/Statuses/OktaAuthStatusFactorChallenge.swift b/Source/Statuses/OktaAuthStatusFactorChallenge.swift index 5de3280..57e94fb 100644 --- a/Source/Statuses/OktaAuthStatusFactorChallenge.swift +++ b/Source/Statuses/OktaAuthStatusFactorChallenge.swift @@ -18,10 +18,10 @@ open class OktaAuthStatusFactorChallenge : OktaAuthStatus { public override init(currentState: OktaAuthStatus, model: OktaAPISuccessResponse) throws { guard let stateToken = model.stateToken else { - throw OktaError.invalidResponse + throw OktaError.invalidResponse("State token is missed") } guard let factor = model.embedded?.factor else { - throw OktaError.invalidResponse + throw OktaError.invalidResponse("Embedded factor is missed") } self.stateToken = stateToken internalFactor = factor diff --git a/Source/Statuses/OktaAuthStatusFactorEnroll.swift b/Source/Statuses/OktaAuthStatusFactorEnroll.swift index ec5f792..69c224c 100644 --- a/Source/Statuses/OktaAuthStatusFactorEnroll.swift +++ b/Source/Statuses/OktaAuthStatusFactorEnroll.swift @@ -18,10 +18,10 @@ open class OktaAuthStatusFactorEnroll : OktaAuthStatus { public override init(currentState: OktaAuthStatus, model: OktaAPISuccessResponse) throws { guard let stateToken = model.stateToken else { - throw OktaError.invalidResponse + throw OktaError.invalidResponse("State token is missed") } guard let factors = model.embedded?.factors else { - throw OktaError.invalidResponse + throw OktaError.invalidResponse("Embedded factors are missed") } self.stateToken = stateToken self.factors = factors diff --git a/Source/Statuses/OktaAuthStatusFactorEnrollActivate.swift b/Source/Statuses/OktaAuthStatusFactorEnrollActivate.swift index 5bfd894..443a3ae 100644 --- a/Source/Statuses/OktaAuthStatusFactorEnrollActivate.swift +++ b/Source/Statuses/OktaAuthStatusFactorEnrollActivate.swift @@ -18,13 +18,13 @@ open class OktaAuthStatusFactorEnrollActivate : OktaAuthStatus { public override init(currentState: OktaAuthStatus, model: OktaAPISuccessResponse) throws { guard let stateToken = model.stateToken else { - throw OktaError.invalidResponse + throw OktaError.invalidResponse("State token is missed") } guard let factor = model.embedded?.factor else { - throw OktaError.invalidResponse + throw OktaError.invalidResponse("Embedded factor is missed") } guard let activateLink = model.links?.next else { - throw OktaError.invalidResponse + throw OktaError.invalidResponse("Links are missed") } self.stateToken = stateToken self.internalFactor = factor diff --git a/Source/Statuses/OktaAuthStatusFactorRequired.swift b/Source/Statuses/OktaAuthStatusFactorRequired.swift index a571583..147ce77 100644 --- a/Source/Statuses/OktaAuthStatusFactorRequired.swift +++ b/Source/Statuses/OktaAuthStatusFactorRequired.swift @@ -18,10 +18,10 @@ open class OktaAuthStatusFactorRequired : OktaAuthStatus { public override init(currentState: OktaAuthStatus, model: OktaAPISuccessResponse) throws { guard let stateToken = model.stateToken else { - throw OktaError.invalidResponse + throw OktaError.invalidResponse("State token is missed") } guard let factors = model.embedded?.factors else { - throw OktaError.invalidResponse + throw OktaError.invalidResponse("Embedded factors are missed") } self.stateToken = stateToken self.factors = factors diff --git a/Source/Statuses/OktaAuthStatusPasswordExpired.swift b/Source/Statuses/OktaAuthStatusPasswordExpired.swift index df3b89c..c4f1748 100644 --- a/Source/Statuses/OktaAuthStatusPasswordExpired.swift +++ b/Source/Statuses/OktaAuthStatusPasswordExpired.swift @@ -18,7 +18,7 @@ open class OktaAuthStatusPasswordExpired : OktaAuthStatus { public override init(currentState: OktaAuthStatus, model: OktaAPISuccessResponse) throws { guard let stateToken = model.stateToken else { - throw OktaError.invalidResponse + throw OktaError.invalidResponse("State token is missed") } self.stateToken = stateToken try super.init(currentState: currentState, model: model) diff --git a/Source/Statuses/OktaAuthStatusPasswordWarning.swift b/Source/Statuses/OktaAuthStatusPasswordWarning.swift index f0001e5..5a16a97 100644 --- a/Source/Statuses/OktaAuthStatusPasswordWarning.swift +++ b/Source/Statuses/OktaAuthStatusPasswordWarning.swift @@ -18,7 +18,7 @@ open class OktaAuthStatusPasswordWarning : OktaAuthStatus { public override init(currentState: OktaAuthStatus, model: OktaAPISuccessResponse) throws { guard let stateToken = model.stateToken else { - throw OktaError.invalidResponse + throw OktaError.invalidResponse("State token is missed") } self.stateToken = stateToken try super.init(currentState: currentState, model: model) diff --git a/Source/Statuses/OktaAuthStatusRecovery.swift b/Source/Statuses/OktaAuthStatusRecovery.swift index 6a36a1f..d155ff8 100644 --- a/Source/Statuses/OktaAuthStatusRecovery.swift +++ b/Source/Statuses/OktaAuthStatusRecovery.swift @@ -18,7 +18,7 @@ open class OktaAuthStatusRecovery : OktaAuthStatus { public override init(currentState: OktaAuthStatus, model: OktaAPISuccessResponse) throws { guard let stateToken = model.stateToken else { - throw OktaError.invalidResponse + throw OktaError.invalidResponse("State token is missed") } self.stateToken = stateToken try super.init(currentState: currentState, model: model) diff --git a/Source/Statuses/OktaAuthStatusRecoveryChallenge.swift b/Source/Statuses/OktaAuthStatusRecoveryChallenge.swift index 82adac6..ef067c3 100644 --- a/Source/Statuses/OktaAuthStatusRecoveryChallenge.swift +++ b/Source/Statuses/OktaAuthStatusRecoveryChallenge.swift @@ -58,7 +58,7 @@ open class OktaAuthStatusRecoveryChallenge : OktaAuthStatus { onStatusChange: @escaping (_ newStatus: OktaAuthStatus) -> Void, onError: @escaping (_ error: OktaError) -> Void) { guard let stateToken = model.stateToken else { - onError(.invalidResponse) + onError(.invalidResponse("State token is missed")) return } guard canVerify() else { @@ -83,7 +83,7 @@ open class OktaAuthStatusRecoveryChallenge : OktaAuthStatus { onStatusChange: @escaping (_ newStatus: OktaAuthStatus) -> Void, onError: @escaping (_ error: OktaError) -> Void) { guard let stateToken = model.stateToken else { - onError(.invalidResponse) + onError(.invalidResponse("State token is missed")) return } guard canVerify() else { @@ -107,7 +107,7 @@ open class OktaAuthStatusRecoveryChallenge : OktaAuthStatus { open func resendFactor(onStatusChange: @escaping (_ newStatus: OktaAuthStatus) -> Void, onError: @escaping (_ error: OktaError) -> Void) { guard let stateToken = model.stateToken else { - onError(.invalidResponse) + onError(.invalidResponse("State token is missed")) return } guard canResend() else { diff --git a/Source/Statuses/OktaAuthStatusResponseHandler.swift b/Source/Statuses/OktaAuthStatusResponseHandler.swift index d7dca20..8db652d 100644 --- a/Source/Statuses/OktaAuthStatusResponseHandler.swift +++ b/Source/Statuses/OktaAuthStatusResponseHandler.swift @@ -45,7 +45,7 @@ open class OktaAuthStatusResponseHandler { open func createAuthStatus(basedOn response: OktaAPISuccessResponse, and currentStatus: OktaAuthStatus) throws -> OktaAuthStatus { guard let statusType = response.status else { - throw OktaError.invalidResponse + throw OktaError.invalidResponse("Status is missed") } // create concrete status instance From ce69bbde4f94453b1efaaccbe6a0f95060d64824 Mon Sep 17 00:00:00 2001 From: Oleg Gnidets Date: Wed, 1 Dec 2021 19:08:08 +0200 Subject: [PATCH 2/3] Fix tests --- Tests/RestAPI/OktaAPIRequestTests.swift | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/Tests/RestAPI/OktaAPIRequestTests.swift b/Tests/RestAPI/OktaAPIRequestTests.swift index 808629c..bdff6f0 100644 --- a/Tests/RestAPI/OktaAPIRequestTests.swift +++ b/Tests/RestAPI/OktaAPIRequestTests.swift @@ -26,8 +26,8 @@ class OktaAPIRequestTests : XCTestCase { req = nil } - func testSetsHTTPSScheme() { - guard let URLString = req.buildRequest()?.url?.absoluteString else { + func testSetsHTTPSScheme() throws { + guard let URLString = try XCTUnwrap(req.buildRequest()).url?.absoluteString else { XCTFail("No URL string") return } @@ -40,7 +40,7 @@ class OktaAPIRequestTests : XCTestCase { req.path = "/test/path" req.urlParams = ["param": "value"] - guard let URLString = req.buildRequest()?.url?.absoluteString else { + guard let URLString = try? req.buildRequest().url?.absoluteString else { XCTFail("No URL string") return } @@ -49,7 +49,7 @@ class OktaAPIRequestTests : XCTestCase { } func testHeaders() { - guard let URLRequest = req.buildRequest() else { + guard let URLRequest = try? req.buildRequest() else { XCTFail("No URL request") return } @@ -64,7 +64,7 @@ class OktaAPIRequestTests : XCTestCase { let value = "HEADER_VALUE" req.additionalHeaders = [header: value] - guard let URLRequest = req.buildRequest() else { + guard let URLRequest = try? req.buildRequest() else { XCTFail("No URL request") return } @@ -75,7 +75,7 @@ class OktaAPIRequestTests : XCTestCase { func testBodyParams() { req.bodyParams = ["root_param": [ "nested_param": "value" ]] - guard let URLRequest = req.buildRequest() else { + guard let URLRequest = try? req.buildRequest() else { XCTFail("No URL request") return } @@ -92,7 +92,7 @@ class OktaAPIRequestTests : XCTestCase { func testMethod() { req.method = .delete - guard let methodString = req.buildRequest()?.httpMethod else { + guard let methodString = try? req.buildRequest().httpMethod else { XCTFail("No HTTP method") return } From 7350d077d38157e9e1a69c69710007722d250f0a Mon Sep 17 00:00:00 2001 From: Oleg Gnidets Date: Wed, 1 Dec 2021 22:10:25 +0200 Subject: [PATCH 3/3] Fix tests --- Tests/Statuses/OktaAuthStatusRecoveryChallengeTests.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Tests/Statuses/OktaAuthStatusRecoveryChallengeTests.swift b/Tests/Statuses/OktaAuthStatusRecoveryChallengeTests.swift index 8171360..fd15db9 100644 --- a/Tests/Statuses/OktaAuthStatusRecoveryChallengeTests.swift +++ b/Tests/Statuses/OktaAuthStatusRecoveryChallengeTests.swift @@ -40,7 +40,7 @@ class OktaAuthStatusRecoveryChallengeTests: XCTestCase { }, onError: { error in XCTAssertEqual( - "Invalid server response", + "Invalid server response: State token is missed", error.localizedDescription ) ex.fulfill() @@ -59,7 +59,7 @@ class OktaAuthStatusRecoveryChallengeTests: XCTestCase { }, onError: { error in XCTAssertEqual( - "Invalid server response", + "Invalid server response: State token is missed", error.localizedDescription ) ex.fulfill()