Fix multiple issues with v1.8+ login sequence

Squashed bugs:
* A v1.8+ user that has connected but not agreed will show up in chat and the userlist as a blank user name
* Race condition where a v1.8+ user in connected but not agreed state duplicated the ID of the next user to connect
* A v1.8+ user that is connected but not agreed will receive the user list

4 files changed, 25 insertions, 12 deletions

diff --git a/hotline/client_conn.go b/hotline/client_conn.go
index 367b70f..8e9a614 100644
--- a/hotline/client_conn.go
+++ b/hotline/client_conn.go
@@ -37,6 +37,7 @@ type ClientConn struct {
 	Idle       bool
 	AutoReply  *[]byte
 	Transfers  map[int][]*FileTransfer
+	Agreed     bool
 }
 
 func (cc *ClientConn) sendAll(t int, fields ...Field) {
@@ -168,7 +169,7 @@ func (cc ClientConn) Disconnect() {
 // NotifyOthers sends transaction t to other clients connected to the server
 func (cc ClientConn) NotifyOthers(t Transaction) {
 	for _, c := range sortedClients(cc.Server.Clients) {
-		if c.ID != cc.ID {
+		if c.ID != cc.ID && c.Agreed {
 			t.clientID = c.ID
 			cc.Server.outbox <- t
 		}
diff --git a/hotline/server.go b/hotline/server.go
index 4e2bde2..61cd7fa 100644
--- a/hotline/server.go
+++ b/hotline/server.go
@@ -111,7 +111,7 @@ func (s *Server) sendTransaction(t Transaction) error {
 	client := s.Clients[uint16(clientID)]
 	s.mux.Unlock()
 	if client == nil {
-		return errors.New("invalid client")
+		return fmt.Errorf("invalid client id %v", *t.clientID)
 	}
 	userName := string(client.UserName)
 	login := client.Account.Login
@@ -330,6 +330,7 @@ func (s *Server) NewClientConn(conn net.Conn) *ClientConn {
 		IdleTime:   new(int),
 		AutoReply:  &[]byte{},
 		Transfers:  make(map[int][]*FileTransfer),
+		Agreed:     false,
 	}
 	*s.NextGuestID++
 	ID := *s.NextGuestID
@@ -378,6 +379,9 @@ func (s *Server) connectedUsers() []Field {
 
 	var connectedUsers []Field
 	for _, c := range sortedClients(s.Clients) {
+		if c.Agreed == false {
+			continue
+		}
 		user := User{
 			ID:    *c.ID,
 			Icon:  *c.Icon,
@@ -536,9 +540,14 @@ func (s *Server) handleNewConnection(conn net.Conn) error {
 	// Show agreement to client
 	c.Server.outbox <- *NewTransaction(tranShowAgreement, c.ID, NewField(fieldData, s.Agreement))
 
-	if _, err := c.notifyNewUserHasJoined(); err != nil {
-		return err
+	// assume simplified hotline v1.2.3 login flow that does not require agreement
+	if *c.Version == nil {
+		c.Agreed = true
+		if _, err := c.notifyNewUserHasJoined(); err != nil {
+			return err
+		}
 	}
+
 	c.Server.Stats.LoginCount += 1
 
 	const readBuffSize = 1024000 // 1KB - TODO: what should this be?
diff --git a/hotline/transaction_handlers.go b/hotline/transaction_handlers.go
index ea46499..ade024c 100644
--- a/hotline/transaction_handlers.go
+++ b/hotline/transaction_handlers.go
@@ -821,11 +821,8 @@ func (cc *ClientConn) notifyNewUserHasJoined() (res []Transaction, err error) {
 }
 
 func HandleTranAgreed(cc *ClientConn, t *Transaction) (res []Transaction, err error) {
-	bs := make([]byte, 2)
-	binary.BigEndian.PutUint16(bs, *cc.Server.NextGuestID)
-
+	cc.Agreed = true
 	cc.UserName = t.GetField(fieldUserName).Data
-	*cc.ID = bs
 	*cc.Icon = t.GetField(fieldUserIconID).Data
 
 	options := t.GetField(fieldOptions).Data
diff --git a/hotline/transaction_handlers_test.go b/hotline/transaction_handlers_test.go
index 56968a1..7c5ee43 100644
--- a/hotline/transaction_handlers_test.go
+++ b/hotline/transaction_handlers_test.go
@@ -5,7 +5,6 @@ import (
 	"io/fs"
 	"math/rand"
 	"os"
-	"reflect"
 	"testing"
 )
 
@@ -227,12 +226,21 @@ func TestHandleGetUserNameList(t *testing.T) {
 								Icon:     &[]byte{0, 2},
 								Flags:    &[]byte{0, 3},
 								UserName: []byte{0, 4},
+								Agreed:   true,
 							},
 							uint16(2): {
 								ID:       &[]byte{0, 2},
 								Icon:     &[]byte{0, 2},
 								Flags:    &[]byte{0, 3},
 								UserName: []byte{0, 4},
+								Agreed:   true,
+							},
+							uint16(3): {
+								ID:       &[]byte{0, 3},
+								Icon:     &[]byte{0, 2},
+								Flags:    &[]byte{0, 3},
+								UserName: []byte{0, 4},
+								Agreed:   false,
 							},
 						},
 					},
@@ -272,9 +280,7 @@ func TestHandleGetUserNameList(t *testing.T) {
 				t.Errorf("HandleGetUserNameList() error = %v, wantErr %v", err, tt.wantErr)
 				return
 			}
-			if !reflect.DeepEqual(got, tt.want) {
-				t.Errorf("HandleGetUserNameList() got = %v, want %v", got, tt.want)
-			}
+			assert.Equal(t, tt.want, got)
 		})
 	}
 }