Fix account management edge cases

diff --git a/hotline/server.go b/hotline/server.go
index f4eef9bd63b646403228ed36547817e0075183c6..a77e79295e0e8e318a68855ac4fe5111328939e7 100644 (file)
--- a/hotline/server.go
+++ b/hotline/server.go
@@ -16,6 +16,7 @@ import (
        "math/rand"
        "net"
        "os"
+       "path"
        "path/filepath"
        "strings"
        "sync"
@@ -384,15 +385,14 @@ func (s *Server) NewClientConn(conn io.ReadWriteCloser, remoteAddr string) *Clie
                Server:     s,
                Version:    []byte{},
                AutoReply:  []byte{},
-               transfers:  map[int]map[[4]byte]*FileTransfer{},
                RemoteAddr: remoteAddr,
-       }
-       clientConn.transfers = map[int]map[[4]byte]*FileTransfer{
-               FileDownload:   {},
-               FileUpload:     {},
-               FolderDownload: {},
-               FolderUpload:   {},
-               bannerDownload: {},
+               transfers: map[int]map[[4]byte]*FileTransfer{
+                       FileDownload:   {},
+                       FileUpload:     {},
+                       FolderDownload: {},
+                       FolderUpload:   {},
+                       bannerDownload: {},
+               },
        }
 
        *s.NextGuestID++
@@ -419,9 +419,26 @@ func (s *Server) NewUser(login, name, password string, access accessBitmap) erro
        if err != nil {
                return err
        }
+
+       // Create account file, returning an error if one already exists.
+       file, err := os.OpenFile(
+               filepath.Join(s.ConfigDir, "Users", path.Join("/", login)+".yaml"),
+               os.O_CREATE|os.O_EXCL|os.O_WRONLY,
+               0644,
+       )
+       if err != nil {
+               return err
+       }
+       defer file.Close()
+
+       _, err = file.Write(out)
+       if err != nil {
+               return fmt.Errorf("error writing account file: %w", err)
+       }
+
        s.Accounts[login] = &account
 
-       return s.FS.WriteFile(filepath.Join(s.ConfigDir, "Users", login+".yaml"), out, 0666)
+       return nil
 }
 
 func (s *Server) UpdateUser(login, newLogin, name, password string, access accessBitmap) error {
@@ -460,9 +477,14 @@ func (s *Server) DeleteUser(login string) error {
        s.mux.Lock()
        defer s.mux.Unlock()
 
+       err := s.FS.Remove(filepath.Join(s.ConfigDir, "Users", path.Join("/", login)+".yaml"))
+       if err != nil {
+               return err
+       }
+
        delete(s.Accounts, login)
 
-       return s.FS.Remove(filepath.Join(s.ConfigDir, "Users", login+".yaml"))
+       return nil
 }
 
 func (s *Server) connectedUsers() []Field {
@@ -522,8 +544,8 @@ func (s *Server) loadAccounts(userDir string) error {
 
                account := Account{}
                decoder := yaml.NewDecoder(fh)
-               if err := decoder.Decode(&account); err != nil {
-                       return err
+               if err = decoder.Decode(&account); err != nil {
+                       return fmt.Errorf("error loading account %s: %w", file, err)
                }
 
                s.Accounts[account.Login] = &account

diff --git a/hotline/transaction_handlers.go b/hotline/transaction_handlers.go
index 22753194f101571e5dc7640506c93c99d805ff6b..d0651221a0921a7a440cfcbbea447c132a47d79c 100644 (file)
--- a/hotline/transaction_handlers.go
+++ b/hotline/transaction_handlers.go
@@ -663,6 +663,9 @@ func HandleSetUser(cc *ClientConn, t *Transaction) (res []Transaction, err error
        newAccessLvl := t.GetField(FieldUserAccess).Data
 
        account := cc.Server.Accounts[login]
+       if account == nil {
+               return append(res, cc.NewErrReply(t, "Account not found.")), nil
+       }
        account.Name = userName
        copy(account.Access[:], newAccessLvl)
 
@@ -671,7 +674,8 @@ func HandleSetUser(cc *ClientConn, t *Transaction) (res []Transaction, err error
        if t.GetField(FieldUserPassword).Data == nil {
                account.Password = hashAndSalt([]byte(""))
        }
-       if len(t.GetField(FieldUserPassword).Data) > 1 {
+
+       if !bytes.Equal([]byte{0}, t.GetField(FieldUserPassword).Data) {
                account.Password = hashAndSalt(t.GetField(FieldUserPassword).Data)
        }
 
@@ -796,7 +800,7 @@ func HandleUpdateUser(cc *ClientConn, t *Transaction) (res []Transaction, err er
                        // account exists, so this is an update action
                        if !cc.Authorize(accessModifyUser) {
                                res = append(res, cc.NewErrReply(t, "You are not allowed to modify accounts."))
-                               return res, err
+                               return res, nil
                        }
 
                        // This part is a bit tricky. There are three possibilities:
@@ -805,7 +809,7 @@ func HandleUpdateUser(cc *ClientConn, t *Transaction) (res []Transaction, err er
                        // 2) The transaction is intended to remove the password.
                        //    In this case, FieldUserPassword is not sent.
                        // 3) The transaction updates the users access bits, but not the password.
-                       //    In this case, FieldUserPassword is sent with zero as the only byte..
+                       //    In this case, FieldUserPassword is sent with zero as the only byte.
                        if getField(FieldUserPassword, &subFields) != nil {
                                newPass := getField(FieldUserPassword, &subFields).Data
                                if !bytes.Equal([]byte{0}, newPass) {
@@ -834,7 +838,7 @@ func HandleUpdateUser(cc *ClientConn, t *Transaction) (res []Transaction, err er
 
                        if !cc.Authorize(accessCreateUser) {
                                res = append(res, cc.NewErrReply(t, "You are not allowed to create new accounts."))
-                               return res, err
+                               return res, nil
                        }
 
                        newAccess := accessBitmap{}
@@ -844,14 +848,14 @@ func HandleUpdateUser(cc *ClientConn, t *Transaction) (res []Transaction, err er
                        for i := 0; i < 64; i++ {
                                if newAccess.IsSet(i) {
                                        if !cc.Authorize(i) {
-                                               return append(res, cc.NewErrReply(t, "Cannot create account with more access than yourself.")), err
+                                               return append(res, cc.NewErrReply(t, "Cannot create account with more access than yourself.")), nil
                                        }
                                }
                        }
 
-                       err := cc.Server.NewUser(login, string(getField(FieldUserName, &subFields).Data), string(getField(FieldUserPassword, &subFields).Data), newAccess)
+                       err = cc.Server.NewUser(login, string(getField(FieldUserName, &subFields).Data), string(getField(FieldUserPassword, &subFields).Data), newAccess)
                        if err != nil {
-                               return []Transaction{}, err
+                               return append(res, cc.NewErrReply(t, "Cannot create account because there is already an account with that login.")), nil
                        }
                }
        }
@@ -889,7 +893,8 @@ func HandleNewUser(cc *ClientConn, t *Transaction) (res []Transaction, err error
        }
 
        if err := cc.Server.NewUser(login, string(t.GetField(FieldUserName).Data), string(t.GetField(FieldUserPassword).Data), newAccess); err != nil {
-               return []Transaction{}, err
+               res = append(res, cc.NewErrReply(t, "Cannot create account because there is already an account with that login."))
+               return res, err
        }
 
        res = append(res, cc.NewReply(t))
@@ -899,10 +904,9 @@ func HandleNewUser(cc *ClientConn, t *Transaction) (res []Transaction, err error
 func HandleDeleteUser(cc *ClientConn, t *Transaction) (res []Transaction, err error) {
        if !cc.Authorize(accessDeleteUser) {
                res = append(res, cc.NewErrReply(t, "You are not allowed to delete accounts."))
-               return res, err
+               return res, nil
        }
 
-       // TODO: Handle case where account doesn't exist; e.g. delete race condition
        login := decodeString(t.GetField(FieldUserLogin).Data)
 
        if err := cc.Server.DeleteUser(login); err != nil {