Handle server connection errors

Fix regression with capability negotiation

CHANGELOG

@@ -9,6 +9,7 @@ vNEXT (in development)
    * Added support for EXTERNAL mechanism, disabled by default
    * Now attempts to renegotiate if the server doesn't recognise the SASL mechanism that was tried
  * Added UserNickChanged and corresponding ChannelNickChanged events
+ * Added ServerConnectionError, raised when connecting to the server fails
  * (Internal) Minor version updates for Gradle, Kotlin and JUnit
 
 v0.6.0

src/main/kotlin/com/dmdirc/ktirc/IrcClient.kt

@@ -1,23 +1,28 @@
 package com.dmdirc.ktirc
 
-import com.dmdirc.ktirc.events.*
-import com.dmdirc.ktirc.io.*
-import com.dmdirc.ktirc.messages.*
+import com.dmdirc.ktirc.events.IrcEvent
+import com.dmdirc.ktirc.io.CaseMapping
+import com.dmdirc.ktirc.messages.sendJoin
 import com.dmdirc.ktirc.model.*
-import com.dmdirc.ktirc.util.currentTimeProvider
-import com.dmdirc.ktirc.util.logger
-import io.ktor.util.KtorExperimentalAPI
-import kotlinx.coroutines.*
-import kotlinx.coroutines.channels.map
-import java.util.concurrent.atomic.AtomicBoolean
 
 /**
  * Primary interface for interacting with KtIrc.
  */
 interface IrcClient {
 
+    /**
+     * Holds state relating to the current server, its features, and capabilities.
+     */
     val serverState: ServerState
+
+    /**
+     * Holds the state for each channel we are currently joined to.
+     */
     val channelState: ChannelStateMap
+
+    /**
+     * Holds the state for all known users (those in common channels).
+     */
     val userState: UserState
 
     val caseMapping: CaseMapping
@@ -86,85 +91,3 @@ interface IrcClient {
 @Suppress("FunctionName")
 fun IrcClient(block: IrcClientConfigBuilder.() -> Unit): IrcClient =
         IrcClientImpl(IrcClientConfigBuilder().apply(block).build())
-
-/**
- * Concrete implementation of an [IrcClient].
- */
-// TODO: How should alternative nicknames work?
-// TODO: Should IRC Client take a pool of servers and rotate through, or make the caller do that?
-// TODO: Should there be a default profile?
-internal class IrcClientImpl(private val config: IrcClientConfig) : IrcClient, CoroutineScope {
-
-    private val log by logger()
-
-    @ExperimentalCoroutinesApi
-    override val coroutineContext = GlobalScope.newCoroutineContext(Dispatchers.IO)
-
-    @ExperimentalCoroutinesApi
-    @KtorExperimentalAPI
-    internal var socketFactory: (CoroutineScope, String, Int, Boolean) -> LineBufferedSocket = ::KtorLineBufferedSocket
-
-    override val serverState = ServerState(config.profile.nickname, config.server.host, config.sasl)
-    override val channelState = ChannelStateMap { caseMapping }
-    override val userState = UserState { caseMapping }
-
-    private val messageHandler = MessageHandler(messageProcessors.toList(), eventHandlers.toMutableList())
-
-    private val parser = MessageParser()
-    private var socket: LineBufferedSocket? = null
-
-    private val connecting = AtomicBoolean(false)
-
-    override fun send(message: String) {
-        socket?.sendChannel?.offer(message.toByteArray()) ?: log.warning { "No send channel for message: $message" }
-    }
-
-    override fun connect() {
-        check(!connecting.getAndSet(true))
-
-        @Suppress("EXPERIMENTAL_API_USAGE")
-        with(socketFactory(this, config.server.host, config.server.port, config.server.useTls)) {
-            // TODO: Proper error handling - what if connect() fails?
-            socket = this
-
-            emitEvent(ServerConnecting(currentTimeProvider()))
-
-            launch {
-                connect()
-                emitEvent(ServerConnected(currentTimeProvider()))
-                sendCapabilityList()
-                sendPasswordIfPresent()
-                sendNickChange(config.profile.nickname)
-                sendUser(config.profile.username, config.profile.realName)
-                messageHandler.processMessages(this@IrcClientImpl, receiveChannel.map { parser.parse(it) })
-                reset()
-                emitEvent(ServerDisconnected(currentTimeProvider()))
-            }
-        }
-    }
-
-    override fun disconnect() {
-        socket?.disconnect()
-    }
-
-    override fun onEvent(handler: (IrcEvent) -> Unit) {
-        messageHandler.handlers.add(object : EventHandler {
-            override fun processEvent(client: IrcClient, event: IrcEvent): List<IrcEvent> {
-                handler(event)
-                return emptyList()
-            }
-        })
-    }
-
-    private fun emitEvent(event: IrcEvent) = messageHandler.emitEvent(this, event)
-    private fun sendPasswordIfPresent() = config.server.password?.let(this::sendPassword)
-
-    internal fun reset() {
-        serverState.reset()
-        channelState.clear()
-        userState.reset()
-        socket = null
-        connecting.set(false)
-    }
-
-}

src/main/kotlin/com/dmdirc/ktirc/IrcClientImpl.kt

@@ -0,0 +1,104 @@
+package com.dmdirc.ktirc
+
+import com.dmdirc.ktirc.events.*
+import com.dmdirc.ktirc.io.KtorLineBufferedSocket
+import com.dmdirc.ktirc.io.LineBufferedSocket
+import com.dmdirc.ktirc.io.MessageHandler
+import com.dmdirc.ktirc.io.MessageParser
+import com.dmdirc.ktirc.messages.*
+import com.dmdirc.ktirc.model.ChannelStateMap
+import com.dmdirc.ktirc.model.ServerState
+import com.dmdirc.ktirc.model.UserState
+import com.dmdirc.ktirc.model.toConnectionError
+import com.dmdirc.ktirc.util.currentTimeProvider
+import com.dmdirc.ktirc.util.logger
+import io.ktor.util.KtorExperimentalAPI
+import kotlinx.coroutines.*
+import kotlinx.coroutines.channels.map
+import java.util.concurrent.atomic.AtomicBoolean
+
+/**
+ * Concrete implementation of an [IrcClient].
+ */
+// TODO: How should alternative nicknames work?
+// TODO: Should IRC Client take a pool of servers and rotate through, or make the caller do that?
+// TODO: Should there be a default profile?
+internal class IrcClientImpl(private val config: IrcClientConfig) : IrcClient, CoroutineScope {
+
+    private val log by logger()
+
+    @ExperimentalCoroutinesApi
+    override val coroutineContext = GlobalScope.newCoroutineContext(Dispatchers.IO)
+
+    @ExperimentalCoroutinesApi
+    @KtorExperimentalAPI
+    internal var socketFactory: (CoroutineScope, String, Int, Boolean) -> LineBufferedSocket = ::KtorLineBufferedSocket
+
+    override val serverState = ServerState(config.profile.nickname, config.server.host, config.sasl)
+    override val channelState = ChannelStateMap { caseMapping }
+    override val userState = UserState { caseMapping }
+
+    private val messageHandler = MessageHandler(messageProcessors.toList(), eventHandlers.toMutableList())
+
+    private val parser = MessageParser()
+    private var socket: LineBufferedSocket? = null
+
+    private val connecting = AtomicBoolean(false)
+
+    override fun send(message: String) {
+        socket?.sendChannel?.offer(message.toByteArray()) ?: log.warning { "No send channel for message: $message" }
+    }
+
+    override fun connect() {
+        check(!connecting.getAndSet(true))
+
+        @Suppress("EXPERIMENTAL_API_USAGE")
+        with(socketFactory(this, config.server.host, config.server.port, config.server.useTls)) {
+            socket = this
+
+            emitEvent(ServerConnecting(currentTimeProvider()))
+
+            launch {
+                try {
+                    connect()
+                    emitEvent(ServerConnected(currentTimeProvider()))
+                    sendCapabilityList()
+                    sendPasswordIfPresent()
+                    sendNickChange(config.profile.nickname)
+                    sendUser(config.profile.username, config.profile.realName)
+                    messageHandler.processMessages(this@IrcClientImpl, receiveChannel.map { parser.parse(it) })
+                } catch (ex : Exception) {
+                    emitEvent(ServerConnectionError(currentTimeProvider(), ex.toConnectionError(), ex.localizedMessage))
+                }
+
+                reset()
+                emitEvent(ServerDisconnected(currentTimeProvider()))
+            }
+        }
+    }
+
+    override fun disconnect() {
+        socket?.disconnect()
+    }
+
+    override fun onEvent(handler: (IrcEvent) -> Unit) {
+        messageHandler.handlers.add(object : EventHandler {
+            override fun processEvent(client: IrcClient, event: IrcEvent): List<IrcEvent> {
+                handler(event)
+                return emptyList()
+            }
+        })
+    }
+
+    private fun emitEvent(event: IrcEvent) = messageHandler.emitEvent(this, event)
+    private fun sendPasswordIfPresent() = config.server.password?.let(this::sendPassword)
+
+    internal fun reset() {
+        serverState.reset()
+        channelState.clear()
+        userState.reset()
+        socket = null
+        connecting.set(false)
+    }
+
+}

src/main/kotlin/com/dmdirc/ktirc/events/CapabilitiesHandler.kt

@@ -54,8 +54,8 @@ internal class CapabilitiesHandler : EventHandler {
             enabledCapabilities.putAll(capabilities)
 
             if (client.serverState.sasl.mechanisms.isNotEmpty()) {
-                enabledCapabilities[Capability.SaslAuthentication]?.let { serverCaps ->
-                    if (startSaslAuth(client, serverCaps.split(','))) {
+                advertisedCapabilities[Capability.SaslAuthentication]?.let { serverCaps ->
+                    if (startSaslAuth(client, if (serverCaps.isEmpty()) emptyList() else serverCaps.split(','))) {
                         return
                     }
                 }

src/main/kotlin/com/dmdirc/ktirc/events/Events.kt

@@ -1,6 +1,7 @@
 package com.dmdirc.ktirc.events
 
 import com.dmdirc.ktirc.model.Capability
+import com.dmdirc.ktirc.model.ConnectionError
 import com.dmdirc.ktirc.model.ServerFeatureMap
 import com.dmdirc.ktirc.model.User
 import java.time.LocalDateTime
@@ -17,6 +18,9 @@ class ServerConnected(time: LocalDateTime) : IrcEvent(time)
 /** Raised when the connection to the server has ended. */
 class ServerDisconnected(time: LocalDateTime) : IrcEvent(time)
 
+/** Raised when an error occurred trying to connect. */
+class ServerConnectionError(time: LocalDateTime, val error: ConnectionError, val details: String?) : IrcEvent(time)
+
 /** Raised when the server is ready for use. */
 class ServerReady(time: LocalDateTime) : IrcEvent(time)
 

src/main/kotlin/com/dmdirc/ktirc/io/LineBufferedSocket.kt

@@ -17,11 +17,14 @@ import kotlinx.coroutines.io.ByteReadChannel
 import kotlinx.coroutines.io.ByteWriteChannel
 import java.net.InetSocketAddress
 import java.security.SecureRandom
+import java.security.cert.CertificateException
 import javax.net.ssl.X509TrustManager
 
 internal interface LineBufferedSocket {
 
+    @Throws(CertificateException::class)
     fun connect()
+
     fun disconnect()
 
     val sendChannel: SendChannel<ByteArray>

src/main/kotlin/com/dmdirc/ktirc/model/ConnectionError.kt

@@ -0,0 +1,27 @@
+package com.dmdirc.ktirc.model
+
+import java.net.ConnectException
+import java.nio.channels.UnresolvedAddressException
+import java.security.cert.CertificateException
+
+/**
+ * Possible types of errors that occur whilst connecting.
+ */
+enum class ConnectionError {
+    /** An error occurred, but we don't really know what. */
+    Unknown,
+    /** The hostname did not resolve to an IP address. */
+    UnresolvableAddress,
+    /** A connection couldn't be established to the given host/port. */
+    ConnectionRefused,
+    /** There was an issue with the TLS certificate the server presented. */
+    BadTlsCertificate,
+}
+
+internal fun Exception.toConnectionError() =
+        when (this) {
+            is UnresolvedAddressException -> ConnectionError.UnresolvableAddress
+            is CertificateException -> ConnectionError.BadTlsCertificate
+            is ConnectException -> ConnectionError.ConnectionRefused
+            else -> ConnectionError.Unknown
+        }

src/test/kotlin/com/dmdirc/ktirc/IrcClientTest.kt → src/test/kotlin/com/dmdirc/ktirc/IrcClientImplTest.kt

@@ -1,12 +1,10 @@
 package com.dmdirc.ktirc
 
-import com.dmdirc.ktirc.events.IrcEvent
-import com.dmdirc.ktirc.events.ServerConnected
-import com.dmdirc.ktirc.events.ServerConnecting
-import com.dmdirc.ktirc.events.ServerWelcome
+import com.dmdirc.ktirc.events.*
 import com.dmdirc.ktirc.io.CaseMapping
 import com.dmdirc.ktirc.io.LineBufferedSocket
 import com.dmdirc.ktirc.model.ChannelState
+import com.dmdirc.ktirc.model.ConnectionError
 import com.dmdirc.ktirc.model.ServerFeature
 import com.dmdirc.ktirc.model.User
 import com.dmdirc.ktirc.util.currentTimeProvider
@@ -16,10 +14,14 @@ import kotlinx.coroutines.*
 import kotlinx.coroutines.channels.Channel
 import kotlinx.coroutines.channels.filter
 import kotlinx.coroutines.channels.map
+import kotlinx.coroutines.sync.Mutex
 import org.junit.jupiter.api.Assertions.*
 import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.assertThrows
+import java.nio.channels.UnresolvedAddressException
+import java.security.cert.CertificateException
+import java.util.concurrent.atomic.AtomicReference
 
 @KtorExperimentalAPI
 @ExperimentalCoroutinesApi
@@ -265,5 +267,51 @@ internal class IrcClientImplTest {
         }
     }
 
+    @Test
+    fun `sends connect error when host is unresolvable`() = runBlocking {
+        whenever(mockSocket.connect()).doThrow(UnresolvedAddressException())
+        with(IrcClientImpl(normalConfig)) {
+            socketFactory = mockSocketFactory
+            withTimeout(500) {
+                launch {
+                    delay(50)
+                    connect()
+                }
+                val event = waitForEvent<ServerConnectionError>()
+                assertEquals(ConnectionError.UnresolvableAddress, event.error)
+            }
+        }
+    }
+
+    @Test
+    fun `sends connect error when tls certificate is bad`() = runBlocking {
+        whenever(mockSocket.connect()).doThrow(CertificateException("Boooo"))
+        with(IrcClientImpl(normalConfig)) {
+            socketFactory = mockSocketFactory
+            withTimeout(500) {
+                launch {
+                    delay(50)
+                    connect()
+                }
+                val event = waitForEvent<ServerConnectionError>()
+                assertEquals(ConnectionError.BadTlsCertificate, event.error)
+                assertEquals("Boooo", event.details)
+            }
+        }
+    }
+
+    private suspend inline fun <reified T : IrcEvent> IrcClient.waitForEvent(): T {
+        val mutex = Mutex(true)
+        val value = AtomicReference<T>()
+        onEvent {
+            if (it is T) {
+                value.set(it)
+                mutex.unlock()
+            }
+        }
+        mutex.lock()
+        return value.get()
+    }
+
 
 }

src/test/kotlin/com/dmdirc/ktirc/events/CapabilitiesHandlerTest.kt

@@ -81,10 +81,10 @@ internal class CapabilitiesHandlerTest {
     }
 
     @Test
-    fun `sends END when capabilities acknowledged and no profile`() {
+    fun `sends END when capabilities acknowledged and no enabled mechanisms`() {
         handler.processEvent(ircClient, ServerCapabilitiesAcknowledged(TestConstants.time, hashMapOf(
                 Capability.EchoMessages to "",
-                Capability.HostsInNamesReply to "123"
+                Capability.HostsInNamesReply to ""
         )))
 
         verify(ircClient).send("CAP END")
@@ -95,7 +95,7 @@ internal class CapabilitiesHandlerTest {
         serverState.sasl.mechanisms.addAll(listOf(saslMech1, saslMech2, saslMech3))
         handler.processEvent(ircClient, ServerCapabilitiesAcknowledged(TestConstants.time, hashMapOf(
                 Capability.EchoMessages to "",
-                Capability.HostsInNamesReply to "123"
+                Capability.HostsInNamesReply to ""
         )))
 
         verify(ircClient).send("CAP END")
@@ -104,54 +104,70 @@ internal class CapabilitiesHandlerTest {
     @Test
     fun `sends END when capabilities acknowledged and no shared mechanism`() {
         serverState.sasl.mechanisms.addAll(listOf(saslMech1, saslMech2, saslMech3))
+        serverState.capabilities.advertisedCapabilities[Capability.SaslAuthentication] = "fake1,fake2"
         handler.processEvent(ircClient, ServerCapabilitiesAcknowledged(TestConstants.time, hashMapOf(
-                Capability.SaslAuthentication to "fake1,fake2",
-                Capability.HostsInNamesReply to "123"
+                Capability.SaslAuthentication to "",
+                Capability.HostsInNamesReply to ""
         )))
 
         verify(ircClient).send("CAP END")
     }
 
     @Test
-    fun `sends AUTHENTICATE when capabilities acknowledged with shared mechanism`() {
+    fun `sets current SASL mechanism when capabilities acknowledged with shared mechanism`() {
         serverState.sasl.mechanisms.addAll(listOf(saslMech1, saslMech2, saslMech3))
+        serverState.capabilities.advertisedCapabilities[Capability.SaslAuthentication] = "mech1,fake2"
         handler.processEvent(ircClient, ServerCapabilitiesAcknowledged(TestConstants.time, hashMapOf(
-                Capability.SaslAuthentication to "mech1,fake2",
-                Capability.HostsInNamesReply to "123"
+                Capability.SaslAuthentication to "",
+                Capability.HostsInNamesReply to ""
         )))
 
-        verify(ircClient).send("AUTHENTICATE mech1")
+        assertSame(saslMech1, serverState.sasl.currentMechanism)
     }
 
     @Test
-    fun `sets current SASL mechanism when capabilities acknowledged with shared mechanism`() {
+    fun `sets current SASL mechanism when capabilities acknowledged with no declared mechanisms`() {
         serverState.sasl.mechanisms.addAll(listOf(saslMech1, saslMech2, saslMech3))
+        serverState.capabilities.advertisedCapabilities[Capability.SaslAuthentication] = ""
         handler.processEvent(ircClient, ServerCapabilitiesAcknowledged(TestConstants.time, hashMapOf(
-                Capability.SaslAuthentication to "mech1,fake2",
-                Capability.HostsInNamesReply to "123"
+                Capability.SaslAuthentication to "",
+                Capability.HostsInNamesReply to ""
         )))
 
-        assertSame(saslMech1, serverState.sasl.currentMechanism)
+        assertSame(saslMech3, serverState.sasl.currentMechanism)
     }
 
-
     @Test
     fun `sends authenticate when capabilities acknowledged with shared mechanism`() {
         serverState.sasl.mechanisms.addAll(listOf(saslMech1, saslMech2, saslMech3))
+        serverState.capabilities.advertisedCapabilities[Capability.SaslAuthentication] = "mech1,fake2"
         handler.processEvent(ircClient, ServerCapabilitiesAcknowledged(TestConstants.time, hashMapOf(
-                Capability.SaslAuthentication to "mech1,fake2",
-                Capability.HostsInNamesReply to "123"
+                Capability.SaslAuthentication to "",
+                Capability.HostsInNamesReply to ""
         )))
 
         verify(ircClient).send("AUTHENTICATE mech1")
     }
 
+    @Test
+    fun `sends authenticate when capabilities acknowledged with no declared mechanisms`() {
+        serverState.sasl.mechanisms.addAll(listOf(saslMech1, saslMech2, saslMech3))
+        serverState.capabilities.advertisedCapabilities[Capability.SaslAuthentication] = ""
+        handler.processEvent(ircClient, ServerCapabilitiesAcknowledged(TestConstants.time, hashMapOf(
+                Capability.SaslAuthentication to "",
+                Capability.HostsInNamesReply to ""
+        )))
+
+        verify(ircClient).send("AUTHENTICATE mech3")
+    }
+
     @Test
     fun `updates negotiation state when capabilities acknowledged with shared mechanism`() {
         serverState.sasl.mechanisms.addAll(listOf(saslMech1, saslMech2, saslMech3))
+        serverState.capabilities.advertisedCapabilities[Capability.SaslAuthentication] = "mech1,fake2"
         handler.processEvent(ircClient, ServerCapabilitiesAcknowledged(TestConstants.time, hashMapOf(
-                Capability.SaslAuthentication to "mech1,fake2",
-                Capability.HostsInNamesReply to "123"
+                Capability.SaslAuthentication to "",
+                Capability.HostsInNamesReply to ""
         )))
 
         assertEquals(CapabilitiesNegotiationState.AUTHENTICATING, serverState.capabilities.negotiationState)

src/test/kotlin/com/dmdirc/ktirc/model/ConnectionErrorTest.kt

@@ -0,0 +1,19 @@
+package com.dmdirc.ktirc.model
+
+import org.junit.jupiter.api.Assertions.assertEquals
+import org.junit.jupiter.api.Test
+import java.net.ConnectException
+import java.nio.channels.UnresolvedAddressException
+import java.security.cert.CertificateException
+
+internal class ConnectionErrorTest {
+
+    @Test
+    fun `maps exceptions to ConnectionError types`() {
+        assertEquals(ConnectionError.ConnectionRefused, ConnectException().toConnectionError())
+        assertEquals(ConnectionError.BadTlsCertificate, CertificateException().toConnectionError())
+        assertEquals(ConnectionError.UnresolvableAddress, UnresolvedAddressException().toConnectionError())
+        assertEquals(ConnectionError.Unknown, IllegalArgumentException().toConnectionError())
+    }
+
+}