fix(auth): incorrect error when error occurs during PKCE flow (#592)

supabase · Nov 6, 2024 · 84ce6f2 · 84ce6f2
1 parent 1460660
commit 84ce6f2
Show file tree

Hide file tree

Showing 2 changed files with 85 additions and 29 deletions.
diff --git a/Sources/Auth/AuthClient.swift b/Sources/Auth/AuthClient.swift
@@ -756,33 +756,32 @@ public final class AuthClient: Sendable {
   /// Gets the session data from a OAuth2 callback URL.
   @discardableResult
   public func session(from url: URL) async throws -> Session {
-    logger?.debug("received \(url)")
+    logger?.debug("Received URL: \(url)")
 
     let params = extractParams(from: url)
 
-    if configuration.flowType == .implicit, !isImplicitGrantFlow(params: params) {
-      throw AuthError.implicitGrantRedirect(message: "Not a valid implicit grant flow url: \(url)")
-    }
-
-    if configuration.flowType == .pkce, !isPKCEFlow(params: params) {
-      throw AuthError.pkceGrantCodeExchange(message: "Not a valid PKCE flow url: \(url)")
-    }
-
-    if isPKCEFlow(params: params) {
-      guard let code = params["code"] else {
-        throw AuthError.pkceGrantCodeExchange(message: "No code detected.")
+    switch configuration.flowType {
+    case .implicit:
+      guard isImplicitGrantFlow(params: params) else {
+        throw AuthError.implicitGrantRedirect(
+          message: "Not a valid implicit grant flow URL: \(url)")
       }
+      return try await handleImplicitGrantFlow(params: params)
 
-      let session = try await exchangeCodeForSession(authCode: code)
-      return session
+    case .pkce:
+      guard isPKCEFlow(params: params) else {
+        throw AuthError.pkceGrantCodeExchange(message: "Not a valid PKCE flow URL: \(url)")
+      }
+      return try await handlePKCEFlow(params: params)
     }
+  }
 
-    if params["error"] != nil || params["error_description"] != nil || params["error_code"] != nil {
-      throw AuthError.pkceGrantCodeExchange(
-        message: params["error_description"] ?? "Error in URL with unspecified error_description.",
-        error: params["error"] ?? "unspecified_error",
-        code: params["error_code"] ?? "unspecified_code"
-      )
+  private func handleImplicitGrantFlow(params: [String: String]) async throws -> Session {
+    precondition(configuration.flowType == .implicit, "Method only allowed for implicit flow.")
+
+    if let errorDescription = params["error_description"] {
+      throw AuthError.implicitGrantRedirect(
+        message: errorDescription.replacingOccurrences(of: "+", with: " "))
     }
 
     guard
@@ -827,6 +826,25 @@ public final class AuthClient: Sendable {
     return session
   }
 
+  private func handlePKCEFlow(params: [String: String]) async throws -> Session {
+    precondition(configuration.flowType == .pkce, "Method only allowed for PKCE flow.")
+
+    if params["error"] != nil || params["error_description"] != nil || params["error_code"] != nil {
+      throw AuthError.pkceGrantCodeExchange(
+        message: params["error_description"]?.replacingOccurrences(of: "+", with: " ")
+          ?? "Error in URL with unspecified error_description.",
+        error: params["error"] ?? "unspecified_error",
+        code: params["error_code"] ?? "unspecified_code"
+      )
+    }
+
+    guard let code = params["code"] else {
+      throw AuthError.pkceGrantCodeExchange(message: "No code detected.")
+    }
+
+    return try await exchangeCodeForSession(authCode: code)
+  }
+
   /// Sets the session data from the current session. If the current session is expired, setSession
   /// will take care of refreshing it to obtain a new session.
   ///
@@ -1304,7 +1322,8 @@ public final class AuthClient: Sendable {
 
   private func isPKCEFlow(params: [String: String]) -> Bool {
     let currentCodeVerifier = codeVerifierStorage.get()
-    return params["code"] != nil && currentCodeVerifier != nil
+    return params["code"] != nil || params["error_description"] != nil || params["error"] != nil
+      || params["error_code"] != nil && currentCodeVerifier != nil
   }
 
   private func getURLForProvider(

diff --git a/Tests/AuthTests/AuthClientTests.swift b/Tests/AuthTests/AuthClientTests.swift
@@ -5,14 +5,15 @@
 //  Created by Guilherme Souza on 23/10/23.
 //
 
-@testable import Auth
 import ConcurrencyExtras
 import CustomDump
-@testable import Helpers
 import InlineSnapshotTesting
 import TestHelpers
 import XCTest
 
+@testable import Auth
+@testable import Helpers
+
 #if canImport(FoundationNetworking)
   import FoundationNetworking
 #endif
@@ -126,7 +127,9 @@ final class AuthClientTests: XCTestCase {
         message: "",
         errorCode: .unknown,
         underlyingData: Data(),
-        underlyingResponse: HTTPURLResponse(url: URL(string: "http://localhost")!, statusCode: 404, httpVersion: nil, headerFields: nil)!
+        underlyingResponse: HTTPURLResponse(
+          url: URL(string: "http://localhost")!, statusCode: 404, httpVersion: nil,
+          headerFields: nil)!
       )
     }
 
@@ -157,7 +160,9 @@ final class AuthClientTests: XCTestCase {
         message: "",
         errorCode: .invalidCredentials,
         underlyingData: Data(),
-        underlyingResponse: HTTPURLResponse(url: URL(string: "http://localhost")!, statusCode: 401, httpVersion: nil, headerFields: nil)!
+        underlyingResponse: HTTPURLResponse(
+          url: URL(string: "http://localhost")!, statusCode: 401, httpVersion: nil,
+          headerFields: nil)!
       )
     }
 
@@ -188,7 +193,9 @@ final class AuthClientTests: XCTestCase {
         message: "",
         errorCode: .invalidCredentials,
         underlyingData: Data(),
-        underlyingResponse: HTTPURLResponse(url: URL(string: "http://localhost")!, statusCode: 403, httpVersion: nil, headerFields: nil)!
+        underlyingResponse: HTTPURLResponse(
+          url: URL(string: "http://localhost")!, statusCode: 403, httpVersion: nil,
+          headerFields: nil)!
       )
     }
 
@@ -277,13 +284,17 @@ final class AuthClientTests: XCTestCase {
       response,
       OAuthResponse(
         provider: .github,
-        url: URL(string: "https://github.com/login/oauth/authorize?client_id=1234&redirect_to=com.supabase.swift-examples://&redirect_uri=http://127.0.0.1:54321/auth/v1/callback&response_type=code&scope=user:email&skip_http_redirect=true&state=jwt")!
+        url: URL(
+          string:
+            "https://github.com/login/oauth/authorize?client_id=1234&redirect_to=com.supabase.swift-examples://&redirect_uri=http://127.0.0.1:54321/auth/v1/callback&response_type=code&scope=user:email&skip_http_redirect=true&state=jwt"
+        )!
       )
     )
   }
 
   func testLinkIdentity() async throws {
-    let url = "https://github.com/login/oauth/authorize?client_id=1234&redirect_to=com.supabase.swift-examples://&redirect_uri=http://127.0.0.1:54321/auth/v1/callback&response_type=code&scope=user:email&skip_http_redirect=true&state=jwt"
+    let url =
+      "https://github.com/login/oauth/authorize?client_id=1234&redirect_to=com.supabase.swift-examples://&redirect_uri=http://127.0.0.1:54321/auth/v1/callback&response_type=code&scope=user:email&skip_http_redirect=true&state=jwt"
     let sut = makeSUT { _ in
       .stub(
         """
@@ -312,7 +323,8 @@ final class AuthClientTests: XCTestCase {
         fromFileName: "list-users-response",
         headers: [
           "X-Total-Count": "669",
-          "Link": "</admin/users?page=2&per_page=>; rel=\"next\", </admin/users?page=14&per_page=>; rel=\"last\"",
+          "Link":
+            "</admin/users?page=2&per_page=>; rel=\"next\", </admin/users?page=14&per_page=>; rel=\"last\"",
         ]
       )
     }
@@ -340,6 +352,31 @@ final class AuthClientTests: XCTestCase {
     XCTAssertEqual(response.lastPage, 14)
   }
 
+  func testSessionFromURL_withError() async throws {
+    sut = makeSUT()
+
+    Dependencies[sut.clientID].codeVerifierStorage.set("code-verifier")
+
+    let url = URL(
+      string:
+        "https://my.redirect.com?error=server_error&error_code=422&error_description=Identity+is+already+linked+to+another+user#error=server_error&error_code=422&error_description=Identity+is+already+linked+to+another+user"
+    )!
+
+    do {
+      try await sut.session(from: url)
+      XCTFail("Expect failure")
+    } catch {
+      expectNoDifference(
+        error as? AuthError,
+        AuthError.pkceGrantCodeExchange(
+          message: "Identity is already linked to another user",
+          error: "server_error",
+          code: "422"
+        )
+      )
+    }
+  }
+
   private func makeSUT(
     fetch: ((URLRequest) async throws -> HTTPResponse)? = nil
   ) -> AuthClient {