refactor: separate storage from data handling, improve error handling

- refactor to use a `Storage` interface, with `JsonStorage` and
`MemoryStorage` implementations
- improve error handling and input validation across the API and
command-line
- add and update unit tests

Author: shoobyban

Makefile

@@ -9,8 +9,8 @@ linux: frontend
 
 test: backend frontend
 	go test ./...
-	cd frontend && yarn lint
+	cd frontend && npm run lint
 
 frontend:
-	cd frontend && yarn && yarn build && cd ..
+	cd frontend && npm i && npm run build && cd ..
 	cp -R frontend/dist cmd/

README.md

@@ -8,21 +8,21 @@ import (
 	"github.com/shoobyban/sshman/backend"
 )
 
-// GroupsHandler handler
+// GroupsHandler is a handler for group-related operations.
 type GroupsHandler struct {
 	Prefix string
 }
 
-// Config returns a loaded configuration from context
-func (h GroupsHandler) Config(r *http.Request) *backend.Storage {
+// Config returns a loaded configuration from the context.
+func (h GroupsHandler) Config(r *http.Request) *backend.Data {
 	ctx := r.Context()
-	if cfg, ok := ctx.Value(ConfigKey).(*backend.Storage); ok {
+	if cfg, ok := ctx.Value(ConfigKey).(*backend.Data); ok {
 		return cfg
 	}
-	return &backend.Storage{}
+	return backend.DefaultConfig()
 }
 
-// AddRoutes adds group specific routes to the router
+// AddRoutes adds group-specific routes to the router.
 func (h GroupsHandler) AddRoutes(router *chi.Mux) {
 	router.Get(h.Prefix, h.GetAllGroups)
 	router.Get(h.Prefix+"/{id}", h.GetGroupDetails)
@@ -31,24 +31,28 @@ func (h GroupsHandler) AddRoutes(router *chi.Mux) {
 	router.Post(h.Prefix, h.CreateGroup)
 }
 
-// GetAllGroups handler returns all groups
+// GetAllGroups is a handler that returns all groups.
 func (h GroupsHandler) GetAllGroups(w http.ResponseWriter, r *http.Request) {
-	json.NewEncoder(w).Encode(h.Config(r).GetGroups())
+	if err := json.NewEncoder(w).Encode(h.Config(r).GetGroups()); err != nil {
+		http.Error(w, err.Error(), http.StatusInternalServerError)
+	}
 }
 
-// GetGroupDetails handler returns group details
+// GetGroupDetails is a handler that returns group details.
 func (h GroupsHandler) GetGroupDetails(w http.ResponseWriter, r *http.Request) {
 	id := chi.URLParam(r, "id")
-	json.NewEncoder(w).Encode(h.Config(r).GetGroup(id))
+	if err := json.NewEncoder(w).Encode(h.Config(r).GetGroup(id)); err != nil {
+		http.Error(w, err.Error(), http.StatusInternalServerError)
+	}
 }
 
-// CreateGroup handler creates a group and adds users and hosts to it
+// CreateGroup is a handler that creates a group and adds users and hosts to it.
 // format: label => [hosts: [host1, host2], users: [user1, user2]]
 func (h GroupsHandler) CreateGroup(w http.ResponseWriter, r *http.Request) {
 	h.UpdateGroup(w, r)
 }
 
-// UpdateGroup handler updates a group and updates users and hosts binding
+// UpdateGroup is a handler that updates a group and its user and host bindings.
 // format: label => [hosts: [host1, host2], users: [user1, user2]]
 func (h GroupsHandler) UpdateGroup(w http.ResponseWriter, r *http.Request) {
 	var group struct {
@@ -57,20 +61,29 @@ func (h GroupsHandler) UpdateGroup(w http.ResponseWriter, r *http.Request) {
 		Hosts []string
 	}
 	id := chi.URLParam(r, "id")
-	json.NewDecoder(r.Body).Decode(&group)
+	if err := json.NewDecoder(r.Body).Decode(&group); err != nil {
+		http.Error(w, err.Error(), http.StatusBadRequest)
+		return
+	}
+	if group.Label == "" {
+		http.Error(w, "Label cannot be empty", http.StatusBadRequest)
+		return
+	}
 	h.Config(r).UpdateGroup(group.Label, group.Hosts, group.Users)
 	if id != "" && group.Label != id {
 		h.Config(r).DeleteGroup(id)
 	}
-	json.NewEncoder(w).Encode(group)
+	if err := json.NewEncoder(w).Encode(group); err != nil {
+		http.Error(w, err.Error(), http.StatusInternalServerError)
+	}
 }
 
-// DeleteGroup handler deletes a group
+// DeleteGroup is a handler that deletes a group.
 func (h GroupsHandler) DeleteGroup(w http.ResponseWriter, r *http.Request) {
 	id := chi.URLParam(r, "id")
 	if h.Config(r).DeleteGroup(id) {
 		w.WriteHeader(http.StatusNoContent)
 	} else {
-		w.WriteHeader(http.StatusNotFound)
+		http.Error(w, "Group not found", http.StatusNotFound)
 	}
 }

api/groups.go

@@ -21,7 +21,7 @@ func contains(slice []string, s string) bool {
 }
 
 func TestCreateGroup(t *testing.T) {
-	cfg := backend.NewTestStorage()
+	cfg := backend.NewData(&backend.MemoryStorage{})
 	cfg.AddHost(
 		&backend.Host{Alias: "host1", Host: "host1.com", User: "user1", Groups: []string{"group2"}},
 		false)
@@ -45,7 +45,7 @@ func TestCreateGroup(t *testing.T) {
 }
 func TestUpdateGroup(t *testing.T) {
 
-	cfg := backend.NewTestStorage()
+	cfg := backend.NewData(&backend.MemoryStorage{})
 	cfg.AddHost(
 		&backend.Host{Alias: "host1", Host: "host1.com", User: "user1", Groups: []string{"group2"}},
 		false)
@@ -75,7 +75,7 @@ func TestUpdateGroup(t *testing.T) {
 
 func TestDeleteGroup(t *testing.T) {
 
-	cfg := backend.NewTestStorage()
+	cfg := backend.NewData(&backend.MemoryStorage{})
 	cfg.AddHost(
 		&backend.Host{Alias: "host1", Host: "host1.com", User: "user1", Groups: []string{"group2"}},
 		false)

api/groups_test.go

@@ -8,21 +8,21 @@ import (
 	"github.com/shoobyban/sshman/backend"
 )
 
-// HostsHandler is a struct for handling hosts
+// HostsHandler is a struct for handling hosts.
 type HostsHandler struct {
 	Prefix string
 }
 
-// Config returns a loaded configuration for the handler
-func (h HostsHandler) Config(r *http.Request) *backend.Storage {
+// Config returns a loaded configuration for the handler.
+func (h HostsHandler) Config(r *http.Request) backend.Config {
 	ctx := r.Context()
-	if cfg, ok := ctx.Value(ConfigKey).(*backend.Storage); ok {
+	if cfg, ok := ctx.Value(ConfigKey).(*backend.Data); ok {
 		return cfg
 	}
-	return backend.NewStorage(false)
+	return backend.DefaultConfig()
 }
 
-// AddRoutes adds hosthandler specific routes to the router
+// AddRoutes adds hosthandler specific routes to the router.
 func (h HostsHandler) AddRoutes(router *chi.Mux) {
 	router.Get(h.Prefix, h.GetAllHosts)
 	router.Get(h.Prefix+"/{id}", h.GetHostDetails)
@@ -33,23 +33,23 @@ func (h HostsHandler) AddRoutes(router *chi.Mux) {
 	router.Delete(h.Prefix+"/sync", h.StopSyncHandler)
 }
 
-// GetAllHosts returns all hosts
+// GetAllHosts returns all hosts.
 func (h HostsHandler) GetAllHosts(w http.ResponseWriter, r *http.Request) {
 	json.NewEncoder(w).Encode(h.Config(r).Hosts())
 }
 
-// GetHostDetails returns host details
+// GetHostDetails returns host details.
 func (h HostsHandler) GetHostDetails(w http.ResponseWriter, r *http.Request) {
 	id := chi.URLParam(r, "id")
 	json.NewEncoder(w).Encode(h.Config(r).GetHost(id))
 }
 
-// CreateHost creates a new host
+// CreateHost creates a new host.
 func (h HostsHandler) CreateHost(w http.ResponseWriter, r *http.Request) {
 	h.UpdateHost(w, r)
 }
 
-// UpdateHost updates a host
+// UpdateHost updates a host.
 func (h HostsHandler) UpdateHost(w http.ResponseWriter, r *http.Request) {
 	id := chi.URLParam(r, "id")
 	var host backend.Host
@@ -86,7 +86,7 @@ func (h HostsHandler) UpdateHost(w http.ResponseWriter, r *http.Request) {
 	json.NewEncoder(w).Encode(host)
 }
 
-// DeleteHost deletes a host
+// DeleteHost deletes a host.
 func (h HostsHandler) DeleteHost(w http.ResponseWriter, r *http.Request) {
 	id := chi.URLParam(r, "id")
 	cfg := h.Config(r)

api/hosts.go

@@ -16,7 +16,7 @@ import (
 
 func TestGetAllHosts(t *testing.T) {
 	// test Hosts.GetAllHosts method
-	cfg := backend.NewTestStorage()
+	cfg := backend.NewData(&backend.MemoryStorage{})
 	cfg.AddHost(
 		&backend.Host{Alias: "host1", Host: "host1.com", User: "user1", Groups: []string{"group1", "group2"}},
 		false)
@@ -43,7 +43,7 @@ func TestGetAllHosts(t *testing.T) {
 
 func TestGetHostDetails(t *testing.T) {
 	// test Hosts.GetHostDetails method
-	cfg := backend.NewTestStorage()
+	cfg := backend.NewData(&backend.MemoryStorage{})
 	testHosts := []backend.Host{
 		{Alias: "host1", Host: "host1.com", User: "user1", Groups: []string{"group1", "group2"}},
 	}
@@ -71,10 +71,14 @@ func TestGetHostDetails(t *testing.T) {
 
 func TestUpdateHost(t *testing.T) {
 	// test Hosts.UpdateHost method
-	cfg := backend.NewTestStorage()
+	cfg := backend.NewData(&backend.MemoryStorage{})
 	cfg.AddHost(
 		&backend.Host{Alias: "host1", Host: "host1.com", User: "user1", Groups: []string{"group1", "group2"}},
 		false)
+	cfg.Conn = backend.MockConn(map[string]backend.SFTPMockHost{
+		"a:22": {Host: "a:22", User: "test", File: "ssh-rsa foo foo\nssh-rsa bar2 bar2\n"},
+		"b:22": {Host: "b:22", User: "test", File: "ssh-rsa bar1 bar\nssh-rsa bar2 bar2\n"},
+	})
 	// mock http request
 	w := httptest.NewRecorder()
 	r := httptest.NewRequest(http.MethodPut, "/host1",
@@ -91,7 +95,7 @@ func TestUpdateHost(t *testing.T) {
 		t.Errorf("Expected status code %d, got %d", http.StatusOK, w.Code)
 	}
 
-	var host backend.Host
+	host := cfg.GetHost("host1")
 	json.NewDecoder(w.Body).Decode(&host)
 	if host.User != "user2" {
 		t.Errorf("Expected user2, got %s %#v", host.User, host)
@@ -100,7 +104,11 @@ func TestUpdateHost(t *testing.T) {
 
 // test Hosts.CreateHost method
 func TestCreateHost(t *testing.T) {
-	cfg := backend.NewTestStorage()
+	cfg := backend.NewData(&backend.MemoryStorage{})
+	cfg.Conn = backend.MockConn(map[string]backend.SFTPMockHost{
+		"a:22": {Host: "a:22", User: "test", File: "ssh-rsa foo foo\nssh-rsa bar2 bar2\n"},
+		"b:22": {Host: "b:22", User: "test", File: "ssh-rsa bar1 bar\nssh-rsa bar2 bar2\n"},
+	})
 	h := HostsHandler{Prefix: ""}
 	// mock http request
 	w := httptest.NewRecorder()
@@ -126,7 +134,7 @@ func TestDeleteHost(t *testing.T) {
 	testHosts := map[string]*backend.Host{
 		"host1": {Alias: "host1", Host: "host1.com", User: "user1", Groups: []string{"group1", "group2"}},
 	}
-	cfg := backend.NewTestStorage()
+	cfg := backend.NewData(&backend.MemoryStorage{})
 	cfg.AddHost(testHosts["host1"], false)
 	h := HostsHandler{Prefix: ""}
 	// mock http request

api/hosts_test.go

@@ -10,21 +10,21 @@ import (
 	"github.com/shoobyban/sshman/backend"
 )
 
-// KeysHandler returns a list of ssh key filename from ~/.ssh
+// KeysHandler returns a list of ssh key filenames from ~/.ssh.
 type KeysHandler struct {
 	Prefix string
 }
 
-// Config returns a loaded configuration for the handler
-func (h KeysHandler) Config(r *http.Request) *backend.Storage {
+// Config returns a loaded configuration for the handler.
+func (h KeysHandler) Config(r *http.Request) backend.Config {
 	ctx := r.Context()
-	if cfg, ok := ctx.Value(ConfigKey).(*backend.Storage); ok {
+	if cfg, ok := ctx.Value(ConfigKey).(*backend.Data); ok {
 		return cfg
 	}
-	return &backend.Storage{}
+	return backend.DefaultConfig()
 }
 
-// AddRoutes adds keyhandler specific routes to the router
+// AddRoutes adds keyhandler specific routes to the router.
 func (h KeysHandler) AddRoutes(router *chi.Mux) {
 	router.Get(h.Prefix, h.GetAllKeys)
 	// router.Get(h.Prefix+"/{filename}", h.GetKeyDetails)
@@ -33,7 +33,7 @@ func (h KeysHandler) AddRoutes(router *chi.Mux) {
 	router.Post(h.Prefix, h.CreateKey)
 }
 
-// GetAllKeys returns a list of all ssh key files from ~/.ssh filtered by type={all|public|private}
+// GetAllKeys returns a list of all ssh key files from ~/.ssh filtered by type={all|public|private}.
 func (h KeysHandler) GetAllKeys(w http.ResponseWriter, r *http.Request) {
 
 	t := r.URL.Query().Get("type")
@@ -75,7 +75,7 @@ func (h KeysHandler) GetAllKeys(w http.ResponseWriter, r *http.Request) {
 	e.Encode(keys)
 }
 
-// CreateKey creates a new ssh key from uploaded file
+// CreateKey creates a new ssh key from an uploaded file.
 func (h KeysHandler) CreateKey(w http.ResponseWriter, r *http.Request) {
 	var payload struct {
 		Filename string `json:"filename"`

api/keys.go

@@ -10,26 +10,26 @@ import (
 	"github.com/shoobyban/sshman/backend"
 )
 
-// LogsHandler is a struct for streaming logs to frontend
+// LogsHandler is a struct for streaming logs to the frontend.
 type LogsHandler struct {
 	Prefix string
 }
 
-// Config returns a loaded configuration for the handler
-func (h LogsHandler) Config(r *http.Request) *backend.Storage {
+// Config returns a loaded configuration for the handler.
+func (h LogsHandler) Config(r *http.Request) backend.Config {
 	ctx := r.Context()
-	if cfg, ok := ctx.Value(ConfigKey).(*backend.Storage); ok {
+	if cfg, ok := ctx.Value(ConfigKey).(*backend.Data); ok {
 		return cfg
 	}
-	return &backend.Storage{}
+	return backend.DefaultConfig()
 }
 
-// AddRoutes adds logs handler specific routes to the router
+// AddRoutes adds logs handler specific routes to the router.
 func (h LogsHandler) AddRoutes(router *chi.Mux) {
 	router.Get(h.Prefix, h.GetLogs)
 }
 
-// GetLogs streams logs on a keep-alive connection to the frontend
+// GetLogs streams logs on a keep-alive connection to the frontend.
 func (h LogsHandler) GetLogs(w http.ResponseWriter, r *http.Request) {
 	w.Header().Set("Content-Type", "text/event-stream")
 	w.Header().Set("Cache-Control", "no-cache")

api/logs.go

@@ -10,18 +10,18 @@ import (
 	"github.com/shoobyban/sshman/backend"
 )
 
-// UsersHandler is the handler for users
+// UsersHandler is the handler for user-related operations
 type UsersHandler struct {
 	Prefix string
 }
 
 // Config returns the config for the handler
 func (h UsersHandler) Config(r *http.Request) backend.Config {
 	ctx := r.Context()
-	if cfg, ok := ctx.Value(ConfigKey).(*backend.Storage); ok {
+	if cfg, ok := ctx.Value(ConfigKey).(*backend.Data); ok {
 		return cfg
 	}
-	return &backend.Storage{}
+	return backend.DefaultConfig()
 }
 
 // AddRoutes adds the routes for the handler
@@ -151,7 +151,7 @@ func (h UsersHandler) UpdateUser(w http.ResponseWriter, r *http.Request) {
 	json.NewEncoder(w).Encode(user)
 }
 
-// DeleteUser deletes a user
+// DeleteUser deletes a user.
 func (h UsersHandler) DeleteUser(w http.ResponseWriter, r *http.Request) {
 	id := chi.URLParam(r, "id")
 	log.Printf("Deleting user %s", id)