diff options
Diffstat (limited to 'hotline')
| -rw-r--r-- | hotline/client_conn.go | 55 | ||||
| -rw-r--r-- | hotline/server.go | 18 | ||||
| -rw-r--r-- | hotline/server_test.go | 6 |
3 files changed, 64 insertions, 15 deletions
diff --git a/hotline/client_conn.go b/hotline/client_conn.go index 3419e0a..fe4dc72 100644 --- a/hotline/client_conn.go +++ b/hotline/client_conn.go @@ -32,13 +32,13 @@ type ClientConn struct { ID ClientID Version []byte // TODO: make fixed size of 2 - Account *Account - Server *Server // TODO: consider adding methods to interact with server + Server *Server // TODO: consider adding methods to interact with server // The following fields hold mutable session state guarded by mu. They are read and written // by multiple goroutines (the client's own transaction loop, other clients' handlers, and the // server keepalive loop), so production code must use the accessor methods below. Direct // field access is only safe before the connection is shared, e.g. in tests. + Account *Account Flags UserFlags UserName []byte Icon []byte // TODO: make fixed size of 2 @@ -197,6 +197,45 @@ func (cc *ClientConn) GetAutoReply() []byte { return cc.AutoReply } +// SetAccount attaches the authenticated account to the client. +func (cc *ClientConn) SetAccount(account *Account) { + cc.mu.Lock() + defer cc.mu.Unlock() + + cc.Account = account +} + +// GetAccount returns the client's account, or nil before login completes. Every account field +// except Access is immutable after login, so callers may read them from the returned struct; +// Access must be read via Authorize or AccessBytes because SetAccountAccess mutates it. +func (cc *ClientConn) GetAccount() *Account { + cc.mu.RLock() + defer cc.mu.RUnlock() + + return cc.Account +} + +// SetAccountAccess replaces the access bitmap on the client's account. +func (cc *ClientConn) SetAccountAccess(access AccessBitmap) { + cc.mu.Lock() + defer cc.mu.Unlock() + + cc.Account.Access = access +} + +// AccessBytes returns a copy of the account's access bitmap, suitable for use as a transaction +// field. It returns nil before login completes. +func (cc *ClientConn) AccessBytes() []byte { + cc.mu.RLock() + defer cc.mu.RUnlock() + + if cc.Account == nil { + return nil + } + access := cc.Account.Access + return access[:] +} + // incrementIdleTime adds interval seconds to the client's idle time. It returns true if this // crossed the idle threshold and marked the client as away, in which case the caller should // notify other clients of the change. @@ -227,8 +266,8 @@ func (cc *ClientConn) clearIdleAndAway() bool { } func (cc *ClientConn) FileRoot() string { - if cc.Account.FileRoot != "" { - return cc.Account.FileRoot + if account := cc.GetAccount(); account != nil && account.FileRoot != "" { + return account.FileRoot } return cc.Server.Config.FileRoot } @@ -331,6 +370,9 @@ func (cc *ClientConn) Authenticate(login string, password []byte) bool { // Authorize checks if the user account has the specified permission func (cc *ClientConn) Authorize(access int) bool { + cc.mu.RLock() + defer cc.mu.RUnlock() + if cc.Account == nil { return false } @@ -424,11 +466,12 @@ func formatDownloadList(fts []FileTransfer) (s string) { } func (cc *ClientConn) String() string { + account := cc.GetAccount() template := fmt.Sprintf( userInfoTemplate, cc.GetUserName(), - cc.Account.Name, - cc.Account.Login, + account.Name, + account.Login, cc.RemoteAddr, formatDownloadList(cc.ClientFileTransferMgr.Get(FileDownload)), formatDownloadList(cc.ClientFileTransferMgr.Get(FolderDownload)), diff --git a/hotline/server.go b/hotline/server.go index 14cee04..700cf19 100644 --- a/hotline/server.go +++ b/hotline/server.go @@ -512,8 +512,6 @@ func (s *Server) NewClientConn(conn io.ReadWriteCloser, remoteAddr string) *Clie ClientFileTransferMgr: NewClientFileTransferMgr(), } - s.ClientMgr.Add(clientConn) - return clientConn } @@ -627,16 +625,17 @@ func (s *Server) handleNewConnection(ctx context.Context, rwc io.ReadWriteCloser c.SetIcon(clientLogin.GetField(FieldUserIconID).Data) } - c.Account = c.Server.AccountManager.Get(login) - if c.Account == nil { + account := c.Server.AccountManager.Get(login) + if account == nil { return nil } + c.SetAccount(account) if clientLogin.GetField(FieldUserName).Data != nil { if c.Authorize(AccessAnyName) { c.SetUserName(clientLogin.GetField(FieldUserName).Data) } else { - c.SetUserName([]byte(c.Account.Name)) + c.SetUserName([]byte(account.Name)) } } @@ -644,6 +643,11 @@ func (s *Server) handleNewConnection(ctx context.Context, rwc io.ReadWriteCloser c.SetFlag(UserFlagAdmin, 1) } + // Publish the client to the manager only now that its session state (Account, Version, + // UserName, Flags) is fully initialized. Other goroutines iterate ClientMgr.List() and + // dereference Account, so a client must never be visible before login completes. + s.ClientMgr.Add(c) + c.Send(c.NewReply(&clientLogin, NewField(FieldVersion, []byte{0x00, 0xbe}), NewField(FieldCommunityBannerID, []byte{0, 0}), @@ -651,7 +655,7 @@ func (s *Server) handleNewConnection(ctx context.Context, rwc io.ReadWriteCloser )) // Send user access privs so client UI knows how to behave - c.Send(NewTransaction(TranUserAccess, c.ID, NewField(FieldUserAccess, c.Account.Access[:]))) + c.Send(NewTransaction(TranUserAccess, c.ID, NewField(FieldUserAccess, c.AccessBytes()))) // Accounts with AccessNoAgreement do not receive the server agreement on login. The behavior is different between // client versions. For 1.2.3 client, we do not send TranShowAgreement. For other client versions, we send @@ -746,7 +750,7 @@ func (s *Server) handleFileTransfer(ctx context.Context, rwc io.ReadWriter) erro } rLogger := s.Logger.With( "remoteAddr", remoteAddr, - "login", fileTransfer.ClientConn.Account.Login, + "login", fileTransfer.ClientConn.GetAccount().Login, "Name", string(fileTransfer.ClientConn.GetUserName()), ) diff --git a/hotline/server_test.go b/hotline/server_test.go index ebcd073..a9842c8 100644 --- a/hotline/server_test.go +++ b/hotline/server_test.go @@ -15,7 +15,6 @@ import ( "time" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "golang.org/x/text/encoding" "golang.org/x/text/encoding/charmap" @@ -831,8 +830,11 @@ type nopCloserRWC struct { func (n *nopCloserRWC) Close() error { return nil } func TestServer_NewClientConn(t *testing.T) { + // The mock has no Add expectation on purpose: NewClientConn must NOT publish the connection + // to the ClientManager. Publication happens later in handleNewConnection, once the session + // state (Account, Version, UserName, Flags) is fully initialized, so that other goroutines + // never observe a partially initialized client. mockMgr := &MockClientMgr{} - mockMgr.On("Add", mock.AnythingOfType("*hotline.ClientConn")).Return() srv := &Server{ClientMgr: mockMgr} |