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

diff --git a/hotline/client_conn.go b/hotline/client_conn.go
index 367b70fdd9f1d92cab447d7035648aaf3e2756f1..8e9a6147c5e3d864d8b2a26378dcdd096ffc7b14 100644 (file)
--- 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 4e2bde2fdfcd613211620efc2980e65ba67fe1a9..61cd7fa0a7e82fae0fb4f63df7b534695c23efc7 100644 (file)
--- 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 ea4649933e26534180831b60df0a8a1ad89e0dd9..ade024c4a3803c397e293c1c8606fa270311ad34 100644 (file)
--- 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 56968a1e1f0b4e48797766e836964e83b5716cb5..7c5ee43268cad1433177115e637cafb5832bf656 100644 (file)
--- 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)
                })
        }
 }