Show an IRC listener user what their session sends from other clients (closes #179)

The relay dropped every PRIVMSG, NOTICE, JOIN, PART, NICK, QUIT, TOPIC,
MODE and KICK sent under the connection's own nick, so an IRC listener
connection never saw what its session did from another client. The
relay now drops nothing by nick. Instead, the service methods that fan
out a user's own command take the client whose queue to skip: the IRC
listener passes its own client, which has already written the echo, and
the HTTP API passes 0. Channel messages and PARTs now also go to the
sender's own session, so the HTTP handler no longer queues the
channel-message echo itself, SendChannelMessage no longer returns the
message's row ID, and neoirc-cli ignores the PART event for its own
part, which it has already shown.

Model: opus-5-5

internal/db/queries.go

@@ -264,25 +264,6 @@ func (database *Database) GetSessionByNick(
 	return sessionID, nil
 }
 
-// GetSessionNick returns the nick a session holds.
-func (database *Database) GetSessionNick(
-	ctx context.Context,
-	sessionID int64,
-) (string, error) {
-	var nick string
-
-	err := database.conn.QueryRowContext(
-		ctx,
-		"SELECT nick FROM sessions WHERE id = ?",
-		sessionID,
-	).Scan(&nick)
-	if err != nil {
-		return "", fmt.Errorf("get session nick: %w", err)
-	}
-
-	return nick, nil
-}
-
 // SessionHostInfo holds the username, hostname, and IP
 // for a session.
 type SessionHostInfo struct {

internal/ircserver/commands.go

@@ -81,7 +81,7 @@ func (c *Conn) handleNick(
 	}
 
 	oldMask := c.hostmask()
-	oldNick := c.currentNick()
+	oldNick := c.nick
 
 	err := c.svc.ChangeNick(
 		ctx, c.sessionID, c.clientID, oldNick, newNick,
@@ -92,10 +92,9 @@ func (c *Conn) handleNick(
 		return
 	}
 
-	// Take the nick the session holds, not newNick: another
-	// client of the session may have changed it again since
-	// ChangeNick stored newNick.
-	c.loadSessionNick(ctx)
+	c.mu.Lock()
+	c.nick = newNick
+	c.mu.Unlock()
 
 	// Echo NICK change to the client on wire.
 	c.send(FormatMessage(oldMask, "NICK", newNick))
@@ -130,7 +129,7 @@ func (c *Conn) handlePrivmsg(
 
 	if strings.HasPrefix(target, "#") {
 		_, err := c.svc.SendChannelMessage(
-			ctx, c.sessionID, c.clientID, c.currentNick(),
+			ctx, c.sessionID, c.clientID, c.nick,
 			msg.Command, target, body, nil,
 		)
 		if err != nil {
@@ -138,7 +137,7 @@ func (c *Conn) handlePrivmsg(
 		}
 	} else {
 		result, err := c.svc.SendDirectMessage(
-			ctx, c.sessionID, c.clientID, c.currentNick(),
+			ctx, c.sessionID, c.clientID, c.nick,
 			msg.Command, target, body, nil,
 		)
 		if err != nil {
@@ -200,7 +199,7 @@ func (c *Conn) joinChannel(
 	channel, key string,
 ) {
 	result, err := c.svc.JoinChannel(
-		ctx, c.sessionID, c.clientID, c.currentNick(), channel, key,
+		ctx, c.sessionID, c.clientID, c.nick, channel, key,
 	)
 	if err != nil {
 		c.sendIRCError(err)
@@ -338,7 +337,7 @@ func (c *Conn) partChannel(
 	channel, reason string,
 ) {
 	err := c.svc.PartChannel(
-		ctx, c.sessionID, c.clientID, c.currentNick(), channel, reason,
+		ctx, c.sessionID, c.clientID, c.nick, channel, reason,
 	)
 	if err != nil {
 		c.sendIRCError(err)
@@ -402,7 +401,7 @@ func (c *Conn) handleTopic(
 	newTopic := msg.Params[1]
 
 	err := c.svc.SetTopic(
-		ctx, c.sessionID, c.clientID, c.currentNick(), channel, newTopic,
+		ctx, c.sessionID, c.clientID, c.nick, channel, newTopic,
 	)
 	if err != nil {
 		c.sendIRCError(err)
@@ -709,7 +708,7 @@ func (c *Conn) applyChannelModes(
 		c.send(FormatMessage(c.hostmask(), "MODE", params...))
 
 		c.svc.BroadcastMode(
-			ctx, c.clientID, c.currentNick(), channel, chID, modeReply,
+			ctx, c.clientID, c.nick, channel, chID, modeReply,
 		)
 	}
 }
@@ -1186,7 +1185,7 @@ func (c *Conn) handleKick(
 	}
 
 	err := c.svc.KickUser(
-		ctx, c.sessionID, c.clientID, c.currentNick(),
+		ctx, c.sessionID, c.clientID, c.nick,
 		channel, targetNick, reason,
 	)
 	if err != nil {
@@ -1226,7 +1225,7 @@ func (c *Conn) handlePassPostReg(
 		return
 	}
 
-	c.sendFromServer("NOTICE", c.currentNick(),
+	c.sendFromServer("NOTICE", c.nick,
 		"Password set. You can reconnect using "+
 			"PASS <password> with your nick.",
 	)
@@ -1250,7 +1249,7 @@ func (c *Conn) handleInvite(
 	channel := msg.Params[1]
 
 	targetID, err := c.svc.InviteUser(
-		ctx, c.sessionID, c.currentNick(), targetNick, channel,
+		ctx, c.sessionID, c.nick, targetNick, channel,
 	)
 	if err != nil {
 		c.sendIRCError(err)
@@ -1267,7 +1266,7 @@ func (c *Conn) handleInvite(
 	body, _ := json.Marshal([]string{channel}) //nolint:errchkjson
 
 	_, _, _ = c.svc.FanOut( //nolint:dogsled // fire-and-forget broadcast
-		ctx, "INVITE", c.currentNick(), targetNick, targetID, 0,
+		ctx, "INVITE", c.nick, targetNick, targetID, 0,
 		nil, body, nil, []int64{targetID}, 0,
 	)
 }

internal/ircserver/conn.go

@@ -313,22 +313,6 @@ func (c *Conn) currentNick() string {
 	return c.nick
 }
 
-// loadSessionNick sets c.nick to the nick the connection's
-// session holds in the database, read by session ID. The
-// connection's own NICK and the relay passing on a NICK line
-// both call it, and it reads and sets the nick under c.mu, so
-// whichever runs last leaves the connection on its session's
-// nick. If the read fails, c.nick is left as it is.
-func (c *Conn) loadSessionNick(ctx context.Context) {
-	c.mu.Lock()
-	defer c.mu.Unlock()
-
-	nick, err := c.database.GetSessionNick(ctx, c.sessionID)
-	if err == nil {
-		c.nick = nick
-	}
-}
-
 // send writes a formatted IRC line to the connection.
 func (c *Conn) send(line string) {
 	c.sendWithin(writeTimeout, line)
@@ -361,7 +345,7 @@ func (c *Conn) sendNumeric(
 	code irc.IRCMessageType,
 	params ...string,
 ) {
-	nick := c.currentNick()
+	nick := c.nick
 	if nick == "" {
 		nick = "*"
 	}
@@ -385,11 +369,9 @@ func (c *Conn) sendFromServer(
 // hostmask returns the client's full hostmask
 // (nick!user@host).
 func (c *Conn) hostmask() string {
-	nick := c.currentNick()
-
 	user := c.username
 	if user == "" {
-		user = nick
+		user = c.nick
 	}
 
 	host := c.hostname
@@ -397,7 +379,7 @@ func (c *Conn) hostmask() string {
 		host = c.remoteIP
 	}
 
-	return nick + "!" + user + "@" + host
+	return c.nick + "!" + user + "@" + host
 }
 
 // handleMessage dispatches a parsed IRC message using
@@ -675,7 +657,7 @@ func (c *Conn) checkPasswordLength(pw string) bool {
 		return true
 	}
 
-	c.sendFromServer("NOTICE", c.currentNick(),
+	c.sendFromServer("NOTICE", c.nick,
 		"Password must be 8 to 72 bytes",
 	)
 

internal/ircserver/relay.go

@@ -92,7 +92,7 @@ func (c *Conn) drainQueue(ctx context.Context) bool {
 //
 //nolint:cyclop // dispatch table
 func (c *Conn) deliverIRCMessage(
-	ctx context.Context,
+	_ context.Context,
 	msg *db.IRCMessage,
 ) {
 	command := msg.Command
@@ -120,7 +120,7 @@ func (c *Conn) deliverIRCMessage(
 	case command == irc.CmdPart:
 		c.deliverPart(msg, text)
 	case command == irc.CmdNick:
-		c.deliverNickChange(ctx, msg, text)
+		c.deliverNickChange(msg, text)
 	case command == irc.CmdQuit:
 		c.deliverQuitMsg(msg, text)
 	case command == irc.CmdTopic:
@@ -230,18 +230,11 @@ func (c *Conn) deliverPart(msg *db.IRCMessage, text string) {
 	}
 }
 
-// deliverNickChange sends a NICK change notification. The
-// change may be its own session's, made from another client,
-// so the connection first takes the nick its session holds
-// now. It does not go by the line's old nick: the connection
-// may have taken that nick after another user gave it up.
+// deliverNickChange sends a NICK change notification.
 func (c *Conn) deliverNickChange(
-	ctx context.Context,
 	msg *db.IRCMessage,
 	newNick string,
 ) {
-	c.loadSessionNick(ctx)
-
 	prefix := msg.From + "!" + msg.From + "@*"
 
 	c.send(FormatMessage(prefix, "NICK", newNick))

internal/ircserver/relay_test.go

@@ -511,10 +511,13 @@ func TestRelayOwnSessionOnce(t *testing.T) {
 	}
 }
 
-// ownSessionCopies sets up alice with aliceWithHTTPClient. It
-// makes the command line from her connection, or with api from
-// the HTTP API client when api is not nil, and returns how many
-// copies of it her connection and her HTTP API client got.
+// ownSessionCopies connects alice to the IRC listener and has
+// her join #chan, which bob is on too, then adds an HTTP API
+// client to her session with the password her connection set,
+// as POST /api/v1/login does. It makes the command line from
+// her connection, or with api from the HTTP API client when api
+// is not nil, and returns how many copies of it her connection
+// and her HTTP API client got.
 func ownSessionCopies(
 	t *testing.T,
 	line string,
@@ -522,9 +525,27 @@ func ownSessionCopies(
 ) (int, int) {
 	t.Helper()
 
+	const password = "alicepass"
+
 	ctx := t.Context()
 	env := newTestEnv(t)
-	alice, _, sessionID, httpClientID := aliceWithHTTPClient(t, env)
+
+	alice := env.dial(t)
+	alice.send("PASS " + password)
+	alice.register("alice")
+	// Registration stores the password before the JOIN runs.
+	alice.joinAndDrain("#chan")
+
+	bob := env.dial(t)
+	bob.register("bob")
+	bob.joinAndDrain("#chan")
+
+	sessionID, httpClientID, _, err := env.database.LoginUser(
+		ctx, "alice", password, "127.0.0.1", "localhost",
+	)
+	if err != nil {
+		t.Fatalf("log in as alice: %v", err)
+	}
 
 	var lines []string
 
@@ -534,7 +555,7 @@ func ownSessionCopies(
 		// Her echo comes before the reply to her next command.
 		lines = alice.sendAndExpect("PING :done", "PONG")
 	} else {
-		err := api(ctx, env.srv.Service(), sessionID)
+		err = api(ctx, env.srv.Service(), sessionID)
 		if err != nil {
 			t.Fatalf("%s over the HTTP API: %v", line, err)
 		}
@@ -580,118 +601,3 @@ func ownSessionCopies(
 
 	return ircCopies, httpCopies
 }
-
-// aliceWithHTTPClient connects alice to the IRC listener and
-// has her join #chan, which bob is on too, then adds an HTTP API
-// client to her session with the password her connection set,
-// as POST /api/v1/login does. It returns her connection, bob's
-// connection, her session and her HTTP API client.
-func aliceWithHTTPClient(
-	t *testing.T, env *testEnv,
-) (*testClient, *testClient, int64, int64) {
-	t.Helper()
-
-	const password = "alicepass"
-
-	alice := env.dial(t)
-	alice.send("PASS " + password)
-	alice.register("alice")
-	// Registration stores the password before the JOIN runs.
-	alice.joinAndDrain("#chan")
-
-	bob := env.dial(t)
-	bob.register("bob")
-	bob.joinAndDrain("#chan")
-
-	sessionID, httpClientID, _, err := env.database.LoginUser(
-		t.Context(), "alice", password, "127.0.0.1", "localhost",
-	)
-	if err != nil {
-		t.Fatalf("log in as alice: %v", err)
-	}
-
-	return alice, bob, sessionID, httpClientID
-}
-
-// TestRelayNickChangeFromHTTPAPI checks that once alice's nick
-// is changed from her HTTP API client, her IRC listener
-// connection uses the new nick: what she then sends from it
-// reaches bob under the new nick.
-func TestRelayNickChangeFromHTTPAPI(t *testing.T) {
-	t.Parallel()
-
-	env := newTestEnv(t)
-	alice, bob, sessionID, _ := aliceWithHTTPClient(t, env)
-
-	bob.joinAndDrain("#other")
-
-	// As the HTTP API does.
-	err := env.srv.Service().ChangeNick(
-		t.Context(), sessionID, 0, "alice", "alice2",
-	)
-	if err != nil {
-		t.Fatalf("change nick over the HTTP API: %v", err)
-	}
-
-	// Her connection has taken the new nick once it has sent her
-	// the change.
-	const change = ":alice!alice@* NICK alice2"
-
-	alice.readUntil(func(l string) bool { return l == change })
-	bob.readUntil(func(l string) bool { return l == change })
-
-	for _, step := range []struct {
-		line, want string
-	}{
-		{"PRIVMSG #chan :hi there", ":alice2!alice2@* PRIVMSG #chan :hi there"},
-		{"JOIN #other", ":alice2!alice2@* JOIN #other"},
-		{"NICK alice3", ":alice2!alice2@* NICK alice3"},
-	} {
-		alice.send(step.line)
-
-		got := bob.readLine()
-		if got != step.want {
-			t.Errorf("%s: got %q, want %q", step.line, got, step.want)
-		}
-	}
-}
-
-// TestRelayNickChangeKeepsTakenNick checks that an IRC
-// listener connection which takes a nick another user has
-// given up keeps it when that user's change reaches it
-// afterwards: what it then sends goes out under the nick its
-// session holds.
-func TestRelayNickChangeKeepsTakenNick(t *testing.T) {
-	t.Parallel()
-
-	env := newTestEnv(t)
-
-	bob := env.dial(t)
-	bob.register("bob")
-	bob.sendAndExpect("NICK bob2", " NICK bob2")
-
-	alice := env.dial(t)
-	alice.register("alice")
-	alice.sendAndExpect("NICK bob", " NICK bob")
-	alice.joinAndDrain("#chan")
-
-	bob.joinAndDrain("#chan")
-
-	// Bob's change reaches alice only now, as it does when she
-	// takes his old nick before her connection has passed the
-	// change on. She holds bob now, so it is queued to bob.
-	env.queueMessage(t, "bob", "NICK", "bob", "", []string{"bob2"})
-
-	alice.readUntil(func(l string) bool {
-		return l == ":bob!bob@* NICK bob2"
-	})
-
-	alice.send("PRIVMSG #chan :hi there")
-
-	const want = ":bob!bob@* PRIVMSG #chan :hi there"
-
-	got := bob.readLine()
-	if got != want {
-		t.Errorf("got %q, want %q", got, want)
-	}
-}