Merge pull request #154 from okta/ognidets-OKTA-449793-Review-error-h…

…andling Improve error handling
okta · Dec 13, 2021 · e2c6e12 · e2c6e12
2 parents 2ce925e + a0448ac
commit e2c6e12
Show file tree

Hide file tree

Showing 15 changed files with 59 additions and 53 deletions.
diff --git 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
@@ -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,27 +74,32 @@ 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
         }
 
         return urlRequest
     }
 
     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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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

diff --git 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
         }

diff --git 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()