Improve clarity in SessionChallengeHandler (#1116)

TAKeanice · web-flow · commit 40e16a24849b · 2026-04-09T17:47:28.000+02:00
diff --git a/AGENTS.md b/AGENTS.md
@@ -26,7 +26,15 @@
 - Avoid trailing closure syntax when passing multiple closures (use parentheses for all closures to prevent multiple_closures_with_trailing_closure warnings)
 - Respect "BuildTools/.swiftformat"  and "BuildTools/.swiftlint.yml"
 - Always use Swift Regex with Swift 6 syntax
+- Prefer `guard` for early exits over `if/else if` chains — when a branch returns, use `guard`/early return to flatten nesting
+- Move logic to the type that owns the data — methods that operate on a type's internals belong on that type, not in the caller
+- Drop argument labels for parameters already implied by the function name — use `_` for positional parameters whose meaning is obvious from the function name, keep labels only for semantically distinct parameters
+- Prefer direct calls to shared helpers over thin wrapper closures that just forward arguments
+
+## Rules for writing tests
+
 - Always write tests with Swift Testing
+- Add a parameter with a default value (e.g. `networkTracker: NetworkTracker = .shared`) to make functions testable without coupling them to singletons
 
 ## git
 - Always use git commit with -s -S 
diff --git a/OpenHABCore/Sources/OpenHABCore/Util/ConnectionConfiguration.swift b/OpenHABCore/Sources/OpenHABCore/Util/ConnectionConfiguration.swift
@@ -89,6 +89,9 @@ public struct ConnectionConfiguration: Hashable, Sendable, Codable, Equatable {
 }
 
 public extension ConnectionConfiguration {
+    /// The host component of the connection URL, if parseable.
+    var host: String? { URL(string: url)?.host }
+
     /// Whether this connection is to an openHAB Cloud instance.
     /// Currently determined by the "openHAB Cloud Service" user preference (`supportsNotifications`).
     var isCloudConnection: Bool { supportsNotifications }
diff --git a/OpenHABCore/Sources/OpenHABCore/Util/NetworkTracker.swift b/OpenHABCore/Sources/OpenHABCore/Util/NetworkTracker.swift
@@ -513,8 +513,17 @@ public actor NetworkTracker {
 }
 
 public extension NetworkTracker {
-    func configuredConnections() -> [ConnectionConfiguration] {
-        connectionConfigurations
+    /// Finds the connection configuration whose URL host or proxy host matches the given host,
+    /// prioritising the active connection.
+    func connectionConfiguration(forHost host: String) -> ConnectionConfiguration? {
+        if let activeConnection,
+           activeConnection.configuration.host == host || activeConnection.proxyURL?.host == host {
+            activeConnection.configuration
+        } else {
+            connectionConfigurations
+                .filter { $0 != activeConnection?.configuration }
+                .first { $0.host == host }
+        }
     }
 
     private func service() async throws -> any OpenAPIServiceProtocol {
@@ -582,5 +591,9 @@ public extension NetworkTracker {
     func setMockConnection(_ connection: ConnectionInfo) {
         activeConnection = connection
     }
+
+    func setMockConnectionConfigurations(_ configurations: [ConnectionConfiguration]) {
+        connectionConfigurations = configurations
+    }
 }
 #endif
diff --git a/OpenHABCore/Sources/OpenHABCore/Util/SessionChallengeHandler.swift b/OpenHABCore/Sources/OpenHABCore/Util/SessionChallengeHandler.swift
@@ -12,105 +12,98 @@
 import Foundation
 import os
 
-@MainActor
-public func onReceiveSessionTaskChallenge(with challenge: URLAuthenticationChallenge) async -> (URLSession.AuthChallengeDisposition, URLCredential?) {
+private func logChallengeDecision(label: String, _ challenge: URLAuthenticationChallenge, _ disposition: URLSession.AuthChallengeDisposition, reason: String) {
     let host = challenge.protectionSpace.host
-    let authenticationMethod = challenge.protectionSpace.authenticationMethod
-    Logger.sessionChallenge.info("onReceiveSessionTaskChallenge host=\(host, privacy: .public), method=\(authenticationMethod, privacy: .public)")
+    let method = challenge.protectionSpace.authenticationMethod
+    Logger.sessionChallenge.info("\(label, privacy: .public) decision host=\(host, privacy: .public), method=\(method, privacy: .public), disposition=\(String(describing: disposition), privacy: .public), reason=\(reason, privacy: .public)")
+}
 
-    func logDecision(_ disposition: URLSession.AuthChallengeDisposition, reason: String) {
-        Logger.sessionChallenge.info("Session task challenge decision host=\(host, privacy: .public), method=\(authenticationMethod, privacy: .public), disposition=\(String(describing: disposition), privacy: .public), reason=\(reason, privacy: .public)")
-    }
+private func logSessionTaskChallengeDecision(_ challenge: URLAuthenticationChallenge, _ disposition: URLSession.AuthChallengeDisposition, reason: String) {
+    logChallengeDecision(label: "Session task challenge", challenge, disposition, reason: reason)
+}
 
-    if challenge.previousFailureCount > 0 {
-        let decision: URLSession.AuthChallengeDisposition = .cancelAuthenticationChallenge
-        logDecision(decision, reason: "previous-failure")
-        return (decision, nil)
-    } else if authenticationMethod.isAny(of: NSURLAuthenticationMethodHTTPBasic, NSURLAuthenticationMethodDefault) {
-        let networkTracker = NetworkTracker.shared
-        let activeConnection = await networkTracker.activeConnection
+private func logSessionChallengeDecision(_ challenge: URLAuthenticationChallenge, _ disposition: URLSession.AuthChallengeDisposition, reason: String) {
+    logChallengeDecision(label: "Session challenge", challenge, disposition, reason: reason)
+}
 
-        var candidateConfigurations: [ConnectionConfiguration] = []
-        if let activeConfiguration = activeConnection?.configuration {
-            candidateConfigurations.append(activeConfiguration)
-        }
-        for configuration in await networkTracker.configuredConnections() where !candidateConfigurations.contains(configuration) {
-            candidateConfigurations.append(configuration)
-        }
+private func credentialForMatchedHost(
+    _ challenge: URLAuthenticationChallenge,
+    networkTracker: NetworkTracker,
+    log: (URLAuthenticationChallenge, URLSession.AuthChallengeDisposition, String) -> Void
+) async -> (URLSession.AuthChallengeDisposition, URLCredential?) {
+    let host = challenge.protectionSpace.host
+    guard let matchedConfiguration = await networkTracker.connectionConfiguration(forHost: host) else {
+        Logger.sessionChallenge.error("No host match for challenge host=\(host, privacy: .public)")
+        let decision: URLSession.AuthChallengeDisposition = .performDefaultHandling
+        log(challenge, decision, "no-host-match")
+        return (decision, nil)
+    }
 
-        let proxyHost = activeConnection?.proxyURL?.host
-        let matchedConfiguration = candidateConfigurations.first { configuration in
-            URL(string: configuration.url)?.host == host
-        }
+    let credential = URLCredential(user: matchedConfiguration.username, password: matchedConfiguration.password, persistence: .forSession)
+    let decision: URLSession.AuthChallengeDisposition = .useCredential
+    log(challenge, decision, "matched-host")
+    return (decision, credential)
+}
 
-        if let matchedConfiguration {
-            let credential = URLCredential(user: matchedConfiguration.username, password: matchedConfiguration.password, persistence: .forSession)
-            let decision: URLSession.AuthChallengeDisposition = .useCredential
-            logDecision(decision, reason: "matched-connection-host")
-            return (decision, credential)
-        }
+@MainActor
+public func onReceiveSessionTaskChallenge(with challenge: URLAuthenticationChallenge, networkTracker: NetworkTracker = .shared) async -> (URLSession.AuthChallengeDisposition, URLCredential?) {
+    let host = challenge.protectionSpace.host
+    Logger.sessionChallenge.info("onReceiveSessionTaskChallenge host=\(host, privacy: .public), method=\(challenge.protectionSpace.authenticationMethod, privacy: .public)")
 
-        if let proxyHost, host == proxyHost, let activeConfiguration = activeConnection?.configuration {
-            let credential = URLCredential(user: activeConfiguration.username, password: activeConfiguration.password, persistence: .forSession)
-            let decision: URLSession.AuthChallengeDisposition = .useCredential
-            logDecision(decision, reason: "matched-active-proxy-host")
-            return (decision, credential)
-        }
+    guard challenge.previousFailureCount == 0 else {
+        let decision: URLSession.AuthChallengeDisposition = .cancelAuthenticationChallenge
+        logSessionTaskChallengeDecision(challenge, decision, reason: "previous-failure")
+        return (decision, nil)
+    }
 
-        let candidateHosts = candidateConfigurations.compactMap { URL(string: $0.url)?.host }.joined(separator: ",")
-        Logger.sessionChallenge.error("No host match for challenge host=\(host, privacy: .public). Candidate hosts: \(candidateHosts, privacy: .public)")
+    guard challenge.protectionSpace.authenticationMethod.isAny(of: NSURLAuthenticationMethodHTTPBasic, NSURLAuthenticationMethodDefault) else {
         let decision: URLSession.AuthChallengeDisposition = .performDefaultHandling
-        logDecision(decision, reason: "no-host-match-default-handling")
+        logSessionTaskChallengeDecision(challenge, decision, reason: "unsupported-auth-method")
         return (decision, nil)
     }
-    let decision: URLSession.AuthChallengeDisposition = .performDefaultHandling
-    logDecision(decision, reason: "unsupported-auth-method-default-handling")
-    return (decision, nil)
+
+    return await credentialForMatchedHost(challenge, networkTracker: networkTracker, log: logSessionTaskChallengeDecision)
 }
 
 @MainActor
-public func onReceiveSessionChallenge(with challenge: URLAuthenticationChallenge) async -> (URLSession.AuthChallengeDisposition, URLCredential?) {
+public func onReceiveSessionChallenge(with challenge: URLAuthenticationChallenge, networkTracker: NetworkTracker = .shared) async -> (URLSession.AuthChallengeDisposition, URLCredential?) {
     let host = challenge.protectionSpace.host
     let authenticationMethod = challenge.protectionSpace.authenticationMethod
     Logger.sessionChallenge.warning("onReceiveSessionChallenge is not implemented fully (see TODOs)")
     Logger.sessionChallenge.info("onReceiveSessionChallenge host=\(host, privacy: .public), method=\(authenticationMethod, privacy: .public)")
-    var disposition: URLSession.AuthChallengeDisposition = .performDefaultHandling
-
-    func logDecision(_ disposition: URLSession.AuthChallengeDisposition, reason: String) {
-        Logger.sessionChallenge.info("Session challenge decision host=\(host, privacy: .public), method=\(authenticationMethod, privacy: .public), disposition=\(String(describing: disposition), privacy: .public), reason=\(reason, privacy: .public)")
-    }
 
     switch challenge.protectionSpace.authenticationMethod {
     case NSURLAuthenticationMethodServerTrust:
         // Check if the active connection has ignoreSSL enabled
-        if let activeConnection = await NetworkTracker.shared.activeConnection,
+        if let activeConnection = await networkTracker.activeConnection,
            activeConnection.configuration.ignoreSSL,
            let serverTrust = challenge.protectionSpace.serverTrust {
             Logger.sessionChallenge.info("Ignoring SSL certificate validation (ignoreSSL enabled)")
             let decision: URLSession.AuthChallengeDisposition = .useCredential
-            logDecision(decision, reason: "ignore-ssl-enabled")
+            logSessionChallengeDecision(challenge, decision, reason: "ignore-ssl-enabled")
             return (decision, URLCredential(trust: serverTrust))
         }
         let result = await CertificateManagers.serverCertificateManager.evaluateTrust(with: challenge)
-        logDecision(result.0, reason: "server-trust-manager")
+        logSessionChallengeDecision(challenge, result.0, reason: "server-trust-manager")
         return result
     case NSURLAuthenticationMethodClientCertificate:
         let result = CertificateManagers.clientCertificateManager.evaluateTrust(with: challenge)
-        logDecision(result.0, reason: "client-certificate-manager")
+        logSessionChallengeDecision(challenge, result.0, reason: "client-certificate-manager")
         return result
     // attemptCredentialAuthentication
     default:
-        if challenge.previousFailureCount > 0 {
-            disposition = .cancelAuthenticationChallenge
-            logDecision(disposition, reason: "previous-failure")
-        } else {
-            // TODO: in the last version, the httpClient had never been set and always remained nil. Figure out if and how this worked and if it is still needed
-            // credential = await NetworkTracker.shared.httpClient?.session.configuration.urlCredentialStorage?.defaultCredential(for: challenge.protectionSpace)
-            // if credential != nil {
-            //    disposition = .useCredential
-            // }
-            logDecision(disposition, reason: "default-handling-no-credential")
+        guard challenge.previousFailureCount == 0 else {
+            let decision: URLSession.AuthChallengeDisposition = .cancelAuthenticationChallenge
+            logSessionChallengeDecision(challenge, decision, reason: "previous-failure")
+            return (decision, nil)
         }
-        return (disposition, nil)
+
+        // TODO: Figure out if credential lookup for the default case is needed.
+        // The old httpClient-based lookup was never wired up. A possible replacement:
+        // return await credentialForMatchedHost(challenge, networkTracker: networkTracker, log: logSessionChallengeDecision)
+
+        let decision: URLSession.AuthChallengeDisposition = .performDefaultHandling
+        logSessionChallengeDecision(challenge, decision, reason: "default-handling-no-credential")
+        return (decision, nil)
     }
 }
diff --git a/OpenHABCore/Tests/OpenHABCoreTests/NetworkTrackerTests.swift b/OpenHABCore/Tests/OpenHABCoreTests/NetworkTrackerTests.swift
@@ -15,6 +15,7 @@ import Network
 import OSLog
 
 @testable import OpenHABCore
+import Testing
 import XCTest
 
 final actor MockOpenAPIService: OpenAPIServiceProtocol {
@@ -264,3 +265,103 @@ final class NetworkTrackerTests: XCTestCase {
 //        await fulfillment(of: [becameNotConnected], timeout: 4.0)
 //    }
 }
+
+// MARK: - connectionConfiguration(forHost:) Tests
+
+private let localConfig = ConnectionConfiguration(
+    url: "https://local.openhab.org",
+    username: "localuser",
+    password: "localpass",
+    priority: 0
+)
+
+private let remoteConfig = ConnectionConfiguration(
+    url: "https://remote.openhab.org",
+    username: "remoteuser",
+    password: "remotepass",
+    priority: 10
+)
+
+private let proxyURL = URL(string: "https://proxy.openhab.org")!
+
+@Suite("NetworkTracker.connectionConfiguration(forHost:)")
+struct ConnectionConfigurationForHostTests {
+    @Test("Returns active connection configuration when host matches active connection URL")
+    func matchesActiveConnectionHost() async {
+        let tracker = NetworkTracker()
+        let connection = ConnectionInfo(configuration: localConfig, version: 1)
+        await tracker.setMockConnection(connection)
+        await tracker.setMockConnectionConfigurations([localConfig, remoteConfig])
+
+        let result = await tracker.connectionConfiguration(forHost: "local.openhab.org")
+        #expect(result == localConfig)
+    }
+
+    @Test("Returns active connection configuration when host matches proxy URL")
+    func matchesProxyHost() async {
+        let tracker = NetworkTracker()
+        let connection = ConnectionInfo(configuration: remoteConfig, version: 1, proxyURL: proxyURL)
+        await tracker.setMockConnection(connection)
+        await tracker.setMockConnectionConfigurations([remoteConfig])
+
+        let result = await tracker.connectionConfiguration(forHost: "proxy.openhab.org")
+        #expect(result == remoteConfig)
+    }
+
+    @Test("Falls back to configured connections when active connection does not match")
+    func fallsBackToConfiguredConnections() async {
+        let tracker = NetworkTracker()
+        let connection = ConnectionInfo(configuration: localConfig, version: 1)
+        await tracker.setMockConnection(connection)
+        await tracker.setMockConnectionConfigurations([localConfig, remoteConfig])
+
+        let result = await tracker.connectionConfiguration(forHost: "remote.openhab.org")
+        #expect(result == remoteConfig)
+    }
+
+    @Test("Returns nil when no configuration matches")
+    func returnsNilForUnknownHost() async {
+        let tracker = NetworkTracker()
+        let connection = ConnectionInfo(configuration: localConfig, version: 1)
+        await tracker.setMockConnection(connection)
+        await tracker.setMockConnectionConfigurations([localConfig, remoteConfig])
+
+        let result = await tracker.connectionConfiguration(forHost: "unknown.example.com")
+        #expect(result == nil)
+    }
+
+    @Test("Returns matching configured connection when there is no active connection")
+    func matchesWithoutActiveConnection() async {
+        let tracker = NetworkTracker()
+        await tracker.setMockConnectionConfigurations([localConfig, remoteConfig])
+
+        let result = await tracker.connectionConfiguration(forHost: "remote.openhab.org")
+        #expect(result == remoteConfig)
+    }
+
+    @Test("Returns nil when there are no configured connections and no active connection")
+    func returnsNilWhenEmpty() async {
+        let tracker = NetworkTracker()
+
+        let result = await tracker.connectionConfiguration(forHost: "local.openhab.org")
+        #expect(result == nil)
+    }
+
+    @Test("Active connection takes priority over same-host configured connection")
+    func activeConnectionPrioritisedOverConfigured() async {
+        let tracker = NetworkTracker()
+        let activeConfig = ConnectionConfiguration(
+            url: "https://local.openhab.org",
+            username: "activeuser",
+            password: "activepass",
+            priority: 5
+        )
+        let connection = ConnectionInfo(configuration: activeConfig, version: 1)
+        await tracker.setMockConnection(connection)
+        await tracker.setMockConnectionConfigurations([localConfig, remoteConfig])
+
+        let result = await tracker.connectionConfiguration(forHost: "local.openhab.org")
+        #expect(result == activeConfig)
+        #expect(result?.username == "activeuser")
+    }
+}
diff --git a/OpenHABCore/Tests/OpenHABCoreTests/SessionChallengeHandlerTests.swift b/OpenHABCore/Tests/OpenHABCoreTests/SessionChallengeHandlerTests.swift

Original file line number	Diff line number	Diff line change
`@@ -89,6 +89,9 @@ public struct ConnectionConfiguration: Hashable, Sendable, Codable, Equatable {`
`89`	`89`	`}`
`90`	`90`
`91`	`91`	`public extension ConnectionConfiguration {`
	`92`	`+ /// The host component of the connection URL, if parseable.`
	`93`	`+ var host: String? { URL(string: url)?.host }`
	`94`	`+`
`92`	`95`	`/// Whether this connection is to an openHAB Cloud instance.`
`93`	`96`	/// Currently determined by the "openHAB Cloud Service" user preference (`supportsNotifications`).
`94`	`97`	`var isCloudConnection: Bool { supportsNotifications }`