Make an IRC listener connection follow its session's nick (closes #184)

An IRC listener connection kept its own copy of its nick, so after the session's nick was changed from another client it went on sending under the old nick, which another user could then take. Now, whenever the relay passes on a NICK line and after the connection's own NICK is stored, the connection takes the nick its session holds in the database, read by session ID under the connection's lock. Taking the nick from the database rather than from the line means a connection can never follow another user's rename and send as them. Handlers read the nick under the same lock.

Untested: an HTTP API rename landing while the connection's own NICK is being stored; that order cannot be produced reliably.

Model: opus-5-5

README.md

@@ -268,12 +268,15 @@ rules). Case-sensitive comparison is unambiguous.
 A single user session can have multiple clients (phone, laptop, terminal).
 
 - Each client gets a **separate server-to-client (S2C) message queue**.
-- The server fans out messages and events from other users, and those the user
-  sends from any of its clients, to every client queue for that user session.
-  The one exception is an IRC listener connection, which gets nothing queued for
-  a command it sent itself: it has already sent its user the echo, or, for
-  `PRIVMSG` and `NOTICE`, IRC sends none. Replies to a command (numerics) go
-  only to the client that sent it.
+- The server fans out messages and events from other users to every client queue
+  for that user session.
+- When the user sends a `PRIVMSG`, `NOTICE`, `JOIN`, `PART`, `NICK`, `TOPIC`,
+  `MODE` or `KICK` from one of its clients, the resulting event goes to every
+  client queue of the user's own session, as well as to the other users it is
+  meant for. The one exception is an IRC listener connection that sent the
+  command, which gets nothing queued for it: it has already sent its user the
+  echo, or, for `PRIVMSG` and `NOTICE`, IRC sends none.
+- Replies to a command (numerics) go only to the client that sent it.
 - `GET /api/v1/messages` delivers from the calling client's specific queue,
   identified by the auth cookie.
 - Client queues have **independent expiry/pruning** — one client going offline
@@ -746,7 +749,7 @@ Leave a channel.
 {"command": "PART", "to": "#general", "body": ["goodbye"]}
 ```
 
-**S2C (broadcast to the other channel members):**
+**S2C (broadcast to all channel members, including the departing user):**
 
 ```json
 {

internal/cli/app.go

@@ -757,7 +757,7 @@ func (a *App) handleServerMessage(msg *api.Message) {
 	case irc.CmdJoin:
 		a.handleJoinEvent(msg, timestamp)
 	case irc.CmdPart:
-		a.handlePartEvent(msg, timestamp)
+		a.handlePartEvent(msg, timestamp, myNick)
 	case irc.CmdQuit:
 		a.handleQuitEvent(msg, timestamp)
 	case irc.CmdNick:
@@ -813,10 +813,14 @@ func (a *App) handleJoinEvent(
 	))
 }
 
+// handlePartEvent shows another user's part. The server
+// also sends the user's own part back to it, which cmdPart
+// has already shown; showing it again would add a second
+// line and mark the channel just left as unread.
 func (a *App) handlePartEvent(
-	msg *api.Message, timestamp string,
+	msg *api.Message, timestamp, myNick string,
 ) {
-	if msg.To == "" {
+	if msg.To == "" || msg.From == myNick {
 		return
 	}
 

internal/cli/app_internal_test.go

@@ -3,7 +3,9 @@ package cli
 import (
 	"testing"
 
+	"github.com/gdamore/tcell/v2"
 	api "sneak.berlin/go/neoirc/internal/cli/api"
+	"sneak.berlin/go/neoirc/pkg/irc"
 )
 
 func TestMemberPrefix(t *testing.T) {
@@ -49,3 +51,66 @@ func TestMemberPrefix(t *testing.T) {
 		})
 	}
 }
+
+// TestPartEvent checks that a PART event adds a line to the
+// channel's buffer and marks it unread, except for the
+// user's own part, which cmdPart has already shown.
+func TestPartEvent(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name       string
+		from       string
+		wantLines  int
+		wantUnread int
+	}{
+		{name: "own part", from: "erin", wantLines: 0, wantUnread: 0},
+		{name: "other member", from: "bob", wantLines: 1, wantUnread: 1},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+
+			app := &App{ui: NewUI(), nick: "erin"}
+			runUI(t, app.ui)
+
+			app.handleServerMessage(&api.Message{
+				Command: irc.CmdPart, From: test.from, To: "#chan",
+			})
+
+			var lines, unread int
+
+			app.ui.app.QueueUpdate(func() {
+				buf := app.ui.getOrCreateBuffer("#chan")
+				lines = len(buf.Lines)
+				unread = buf.Unread
+			})
+
+			if lines != test.wantLines || unread != test.wantUnread {
+				t.Fatalf("got %d lines and %d unread, want %d and %d",
+					lines, unread, test.wantLines, test.wantUnread)
+			}
+		})
+	}
+}
+
+// runUI runs ui on a simulated screen until the test ends.
+func runUI(t *testing.T, ui *UI) {
+	t.Helper()
+
+	ui.app.SetScreen(tcell.NewSimulationScreen(""))
+
+	done := make(chan error)
+
+	go func() { done <- ui.Run() }()
+
+	t.Cleanup(func() {
+		ui.Stop()
+
+		err := <-done
+		if err != nil {
+			t.Errorf("run ui: %v", err)
+		}
+	})
+}

internal/db/queries.go

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

internal/ircserver/conn.go

@@ -313,6 +313,22 @@ 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)
@@ -345,7 +361,7 @@ func (c *Conn) sendNumeric(
 	code irc.IRCMessageType,
 	params ...string,
 ) {
-	nick := c.nick
+	nick := c.currentNick()
 	if nick == "" {
 		nick = "*"
 	}
@@ -369,9 +385,11 @@ 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 = c.nick
+		user = nick
 	}
 
 	host := c.hostname
@@ -379,7 +397,7 @@ func (c *Conn) hostmask() string {
 		host = c.remoteIP
 	}
 
-	return c.nick + "!" + user + "@" + host
+	return nick + "!" + user + "@" + host
 }
 
 // handleMessage dispatches a parsed IRC message using
@@ -657,7 +675,7 @@ func (c *Conn) checkPasswordLength(pw string) bool {
 		return true
 	}
 
-	c.sendFromServer("NOTICE", c.nick,
+	c.sendFromServer("NOTICE", c.currentNick(),
 		"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(
-	_ context.Context,
+	ctx 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(msg, text)
+		c.deliverNickChange(ctx, msg, text)
 	case command == irc.CmdQuit:
 		c.deliverQuitMsg(msg, text)
 	case command == irc.CmdTopic:
@@ -230,11 +230,18 @@ func (c *Conn) deliverPart(msg *db.IRCMessage, text string) {
 	}
 }
 
-// deliverNickChange sends a NICK change notification.
+// 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.
 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,13 +511,10 @@ func TestRelayOwnSessionOnce(t *testing.T) {
 	}
 }
 
-// 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.
+// 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.
 func ownSessionCopies(
 	t *testing.T,
 	line string,
@@ -525,27 +522,9 @@ func ownSessionCopies(
 ) (int, int) {
 	t.Helper()
 
-	const password = "alicepass"
-
 	ctx := t.Context()
 	env := newTestEnv(t)
-
-	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)
-	}
+	alice, _, sessionID, httpClientID := aliceWithHTTPClient(t, env)
 
 	var lines []string
 
@@ -555,7 +534,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)
 		}
@@ -601,3 +580,118 @@ 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)
+	}
+}