Issue 1286 improve errors (#1322)

* Refactor error handling for better extensibility and add more scaffolding for specific db errors

* Add integration tests to check errors from mysql are translated properly

* Address review comments

* Add changes file

changes/issue-1286-improve-errors

@@ -0,0 +1 @@
+* Improve error messages at the API level. Fixes issue 1286.

server/datastore/datastore.go

@@ -9,7 +9,7 @@ import (
 
 // TestFunctions are the test functions that a Datastore implementation should
 // run to verify proper implementation.
-var TestFunctions = [...]func(*testing.T, fleet.Datastore){
+var TestFunctions = []func(*testing.T, fleet.Datastore){
 	testOrgInfo,
 	testAdditionalQueries,
 	testEnrollSecrets,

server/datastore/mysql/datastore_test.go

@@ -1,134 +1,12 @@
 package mysql
 
 import (
-	"database/sql"
-	"fmt"
-	"os"
-	"os/exec"
 	"testing"
 
-	"github.com/WatchBeam/clock"
-	"github.com/fleetdm/fleet/v4/server/config"
 	"github.com/fleetdm/fleet/v4/server/datastore"
-	"github.com/fleetdm/fleet/v4/server/fleet"
-	"github.com/fleetdm/fleet/v4/server/test"
-	"github.com/go-kit/kit/log"
-
 	_ "github.com/go-sql-driver/mysql"
-	"github.com/stretchr/testify/require"
 )
 
-const (
-	schemaDbName = "schemadb"
-	dumpfile     = "dump.sql"
-	testUsername = "root"
-	testPassword = "toor"
-	testAddress  = "localhost:3307"
-)
-
-func connectMySQL(t *testing.T, testName string) *Datastore {
-	config := config.MysqlConfig{
-		Username: testUsername,
-		Password: testPassword,
-		Database: testName,
-		Address:  testAddress,
-	}
-
-	// Create datastore client
-	ds, err := New(config, clock.NewMockClock(), Logger(log.NewNopLogger()), LimitAttempts(1))
-	require.Nil(t, err)
-	return ds
-}
-
-// initializeSchema initializes a database schema using the normal Fleet
-// migrations, then outputs the schema with mysqldump within the MySQL Docker
-// container.
-func initializeSchema(t *testing.T) {
-	// Create the database (must use raw MySQL client to do this)
-	db, err := sql.Open(
-		"mysql",
-		fmt.Sprintf("%s:%s@tcp(%s)/?multiStatements=true", testUsername, testPassword, testAddress),
-	)
-	require.NoError(t, err)
-	defer db.Close()
-	_, err = db.Exec("DROP DATABASE IF EXISTS schemadb; CREATE DATABASE schemadb;")
-	require.NoError(t, err)
-
-	// Create a datastore client in order to run migrations as usual
-	config := config.MysqlConfig{
-		Username: testUsername,
-		Password: testPassword,
-		Address:  testAddress,
-		Database: schemaDbName,
-	}
-	ds, err := New(config, clock.NewMockClock(), Logger(log.NewNopLogger()), LimitAttempts(1))
-	require.Nil(t, err)
-	defer ds.Close()
-	require.Nil(t, ds.MigrateTables())
-
-	// Dump schema to dumpfile
-	if out, err := exec.Command(
-		"docker-compose", "exec", "-T", "mysql_test",
-		// Command run inside container
-		"mysqldump",
-		"-u"+testUsername, "-p"+testPassword,
-		"schemadb",
-		"--compact", "--skip-comments",
-		"--result-file="+dumpfile,
-	).CombinedOutput(); err != nil {
-		t.Error(err)
-		t.Error(string(out))
-		t.FailNow()
-	}
-}
-
-// initializeDatabase loads the dumped schema into a newly created database in
-// MySQL. This is much faster than running the full set of migrations on each
-// test.
-func initializeDatabase(t *testing.T, dbName string) {
-	// Load schema from dumpfile
-	if out, err := exec.Command(
-		"docker-compose", "exec", "-T", "mysql_test",
-		// Command run inside container
-		"mysql",
-		"-u"+testUsername, "-p"+testPassword,
-		"-e",
-		fmt.Sprintf(
-			"DROP DATABASE IF EXISTS %s; CREATE DATABASE %s; USE %s; SET FOREIGN_KEY_CHECKS=0; SOURCE %s;",
-			dbName, dbName, dbName, dumpfile,
-		),
-	).CombinedOutput(); err != nil {
-		t.Error(err)
-		t.Error(string(out))
-		t.FailNow()
-	}
-
-}
-
-func runTest(t *testing.T, testFunc func(*testing.T, fleet.Datastore)) {
-	t.Run(test.FunctionName(testFunc), func(t *testing.T) {
-		t.Parallel()
-
-		// Create a new database and load the schema for each test
-		initializeDatabase(t, test.FunctionName(testFunc))
-
-		ds := connectMySQL(t, test.FunctionName(testFunc))
-		defer ds.Close()
-
-		testFunc(t, ds)
-	})
-}
-
 func TestMySQL(t *testing.T) {
-	if _, ok := os.LookupEnv("MYSQL_TEST"); !ok {
-		t.Skip("MySQL tests are disabled")
-	}
-
-	// Initialize the schema once for the entire test run.
-	initializeSchema(t)
-
-	for _, f := range datastore.TestFunctions {
-		runTest(t, f)
-	}
-
+	RunTestsAgainstMySQL(t, datastore.TestFunctions)
 }

server/datastore/mysql/testing_utils.go

@@ -0,0 +1,129 @@
+package mysql
+
+import (
+	"database/sql"
+	"fmt"
+	"github.com/WatchBeam/clock"
+	"github.com/fleetdm/fleet/v4/server/config"
+	"github.com/fleetdm/fleet/v4/server/fleet"
+	"github.com/fleetdm/fleet/v4/server/test"
+	"github.com/go-kit/kit/log"
+	"github.com/stretchr/testify/require"
+	"os"
+	"os/exec"
+	"testing"
+)
+
+const (
+	schemaDbName = "schemadb"
+	dumpfile     = "dump.sql"
+	testUsername = "root"
+	testPassword = "toor"
+	testAddress  = "localhost:3307"
+)
+
+func connectMySQL(t *testing.T, testName string) *Datastore {
+	config := config.MysqlConfig{
+		Username: testUsername,
+		Password: testPassword,
+		Database: testName,
+		Address:  testAddress,
+	}
+
+	// Create datastore client
+	ds, err := New(config, clock.NewMockClock(), Logger(log.NewNopLogger()), LimitAttempts(1))
+	require.Nil(t, err)
+	return ds
+}
+
+// initializeSchema initializes a database schema using the normal Fleet
+// migrations, then outputs the schema with mysqldump within the MySQL Docker
+// container.
+func initializeSchema(t *testing.T) {
+	// Create the database (must use raw MySQL client to do this)
+	db, err := sql.Open(
+		"mysql",
+		fmt.Sprintf("%s:%s@tcp(%s)/?multiStatements=true", testUsername, testPassword, testAddress),
+	)
+	require.NoError(t, err)
+	defer db.Close()
+	_, err = db.Exec("DROP DATABASE IF EXISTS schemadb; CREATE DATABASE schemadb;")
+	require.NoError(t, err)
+
+	// Create a datastore client in order to run migrations as usual
+	config := config.MysqlConfig{
+		Username: testUsername,
+		Password: testPassword,
+		Address:  testAddress,
+		Database: schemaDbName,
+	}
+	ds, err := New(config, clock.NewMockClock(), Logger(log.NewNopLogger()), LimitAttempts(1))
+	require.Nil(t, err)
+	defer ds.Close()
+	require.Nil(t, ds.MigrateTables())
+
+	// Dump schema to dumpfile
+	if out, err := exec.Command(
+		"docker-compose", "exec", "-T", "mysql_test",
+		// Command run inside container
+		"mysqldump",
+		"-u"+testUsername, "-p"+testPassword,
+		"schemadb",
+		"--compact", "--skip-comments",
+		"--result-file="+dumpfile,
+	).CombinedOutput(); err != nil {
+		t.Error(err)
+		t.Error(string(out))
+		t.FailNow()
+	}
+}
+
+// initializeDatabase loads the dumped schema into a newly created database in
+// MySQL. This is much faster than running the full set of migrations on each
+// test.
+func initializeDatabase(t *testing.T, dbName string) {
+	// Load schema from dumpfile
+	if out, err := exec.Command(
+		"docker-compose", "exec", "-T", "mysql_test",
+		// Command run inside container
+		"mysql",
+		"-u"+testUsername, "-p"+testPassword,
+		"-e",
+		fmt.Sprintf(
+			"DROP DATABASE IF EXISTS %s; CREATE DATABASE %s; USE %s; SET FOREIGN_KEY_CHECKS=0; SOURCE %s;",
+			dbName, dbName, dbName, dumpfile,
+		),
+	).CombinedOutput(); err != nil {
+		t.Error(err)
+		t.Error(string(out))
+		t.FailNow()
+	}
+
+}
+
+func runTest(t *testing.T, testFunc func(*testing.T, fleet.Datastore)) {
+	t.Run(test.FunctionName(testFunc), func(t *testing.T) {
+		t.Parallel()
+
+		// Create a new database and load the schema for each test
+		initializeDatabase(t, test.FunctionName(testFunc))
+
+		ds := connectMySQL(t, test.FunctionName(testFunc))
+		defer ds.Close()
+
+		testFunc(t, ds)
+	})
+}
+
+func RunTestsAgainstMySQL(t *testing.T, tests []func(*testing.T, fleet.Datastore)) {
+	if _, ok := os.LookupEnv("MYSQL_TEST"); !ok {
+		t.Skip("MySQL tests are disabled")
+	}
+
+	// Initialize the schema once for the entire test run.
+	initializeSchema(t)
+
+	for _, f := range tests {
+		runTest(t, f)
+	}
+}

server/service/http_auth_test.go

@@ -115,6 +115,11 @@ func TestLogin(t *testing.T) {
 
 func setupAuthTest(t *testing.T) (*inmem.Datastore, map[string]fleet.User, *httptest.Server) {
 	ds, _ := inmem.New(config.TestConfig())
+	users, server := runServerForTestsWithDS(t, ds)
+	return ds, users, server
+}
+
+func runServerForTestsWithDS(t *testing.T, ds fleet.Datastore) (map[string]fleet.User, *httptest.Server) {
 	svc := newTestService(ds, nil, nil)
 	users := createTestUsers(t, ds)
 	logger := kitlog.NewLogfmtLogger(os.Stdout)
@@ -138,7 +143,7 @@ func setupAuthTest(t *testing.T) (*inmem.Datastore, map[string]fleet.User, *http
 	}))
 
 	server := httptest.NewServer(r)
-	return ds, users, server
+	return users, server
 }
 
 func TestNoHeaderErrorsDifferently(t *testing.T) {

server/service/integration_test.go

@@ -0,0 +1,117 @@
+package service
+
+import (
+	"bytes"
+	"encoding/json"
+	"fmt"
+	"github.com/fleetdm/fleet/v4/server/datastore/mysql"
+	"github.com/fleetdm/fleet/v4/server/fleet"
+	"github.com/fleetdm/fleet/v4/server/ptr"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"io/ioutil"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+)
+
+func getTestAdminToken(t *testing.T, server *httptest.Server) string {
+	testUser := testUsers["admin1"]
+
+	params := loginRequest{
+		Email:    testUser.Email,
+		Password: testUser.PlaintextPassword,
+	}
+	j, err := json.Marshal(&params)
+	assert.Nil(t, err)
+
+	requestBody := &nopCloser{bytes.NewBuffer(j)}
+	resp, err := http.Post(server.URL+"/api/v1/fleet/login", "application/json", requestBody)
+	require.Nil(t, err)
+	assert.Equal(t, http.StatusOK, resp.StatusCode)
+
+	var jsn = struct {
+		User  *fleet.User         `json:"user"`
+		Token string              `json:"token"`
+		Err   []map[string]string `json:"errors,omitempty"`
+	}{}
+	err = json.NewDecoder(resp.Body).Decode(&jsn)
+	require.Nil(t, err)
+
+	return jsn.Token
+}
+
+func testDoubleUserCreationErrors(t *testing.T, ds fleet.Datastore) {
+	_, server := runServerForTestsWithDS(t, ds)
+	token := getTestAdminToken(t, server)
+
+	params := fleet.UserPayload{
+		Name:     ptr.String("user1"),
+		Email:    ptr.String("email@asd.com"),
+		Password: ptr.String("pass"),
+		//Teams                    *[]UserTeam `json:"teams,omitempty"`
+	}
+	j, err := json.Marshal(&params)
+	assert.Nil(t, err)
+
+	requestBody := &nopCloser{bytes.NewBuffer(j)}
+	req, _ := http.NewRequest("POST", server.URL+"/api/v1/fleet/users/admin", requestBody)
+	req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", token))
+	client := &http.Client{}
+	resp, err := client.Do(req)
+	require.Nil(t, err)
+	assert.Equal(t, http.StatusOK, resp.StatusCode)
+
+	requestBody = &nopCloser{bytes.NewBuffer(j)}
+	req, _ = http.NewRequest("POST", server.URL+"/api/v1/fleet/users/admin", requestBody)
+	req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", token))
+	resp, err = client.Do(req)
+	require.Nil(t, err)
+	assert.Equal(t, http.StatusConflict, resp.StatusCode)
+	assertBodyContains(t, resp, `Error 1062: Duplicate entry 'email@asd.com'`)
+}
+
+func testUserCreationWrongTeamErrors(t *testing.T, ds fleet.Datastore) {
+	_, server := runServerForTestsWithDS(t, ds)
+	token := getTestAdminToken(t, server)
+
+	teams := []fleet.UserTeam{
+		{
+			Team: fleet.Team{
+				ID: 9999,
+			},
+		},
+	}
+
+	params := fleet.UserPayload{
+		Name:     ptr.String("user1"),
+		Email:    ptr.String("email@asd.com"),
+		Password: ptr.String("pass"),
+		Teams:    &teams,
+	}
+	j, err := json.Marshal(&params)
+	assert.Nil(t, err)
+
+	requestBody := &nopCloser{bytes.NewBuffer(j)}
+	req, _ := http.NewRequest("POST", server.URL+"/api/v1/fleet/users/admin", requestBody)
+	req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", token))
+	client := &http.Client{}
+	resp, err := client.Do(req)
+	require.Nil(t, err)
+	assert.Equal(t, http.StatusUnprocessableEntity, resp.StatusCode)
+	assertBodyContains(t, resp, `Error 1452: Cannot add or update a child row: a foreign key constraint fails`)
+}
+
+func assertBodyContains(t *testing.T, resp *http.Response, expectedError string) {
+	bodyBytes, err := ioutil.ReadAll(resp.Body)
+	require.Nil(t, err)
+	bodyString := string(bodyBytes)
+	assert.Contains(t, bodyString, expectedError)
+}
+
+func TestSQLErrorsAreProperlyHandled(t *testing.T) {
+	mysql.RunTestsAgainstMySQL(t, []func(t *testing.T, ds fleet.Datastore){
+		testDoubleUserCreationErrors,
+		testUserCreationWrongTeamErrors,
+	})
+}

server/service/transport_error.go

@@ -3,12 +3,12 @@ package service
 import (
 	"context"
 	"encoding/json"
-	"net/http"
-	"strconv"
-
 	"github.com/fleetdm/fleet/v4/server/fleet"
 	kithttp "github.com/go-kit/kit/transport/http"
+	"github.com/go-sql-driver/mysql"
 	"github.com/pkg/errors"
+	"net/http"
+	"strconv"
 )
 
 // erroer interface is implemented by response structs to encode business logic errors
@@ -33,29 +33,46 @@ func baseError(err string) []map[string]string {
 	}
 }
 
+type validationErrorInterface interface {
+	error
+	Invalid() []map[string]string
+}
+
+type permissionErrorInterface interface {
+	error
+	PermissionError() []map[string]string
+}
+
+type notFoundErrorInterface interface {
+	error
+	IsNotFound() bool
+}
+
+type existsErrorInterface interface {
+	error
+	IsExists() bool
+}
+
+type causerInterface interface {
+	Cause() error
+}
+
 // encode error and status header to the client
 func encodeError(ctx context.Context, err error, w http.ResponseWriter) {
 	enc := json.NewEncoder(w)
 	enc.SetIndent("", "  ")
 
-	type validationError interface {
-		error
-		Invalid() []map[string]string
-	}
-	if e, ok := err.(validationError); ok {
+	err = errors.Cause(err)
+
+	switch e := err.(type) {
+	case validationErrorInterface:
 		ve := jsonError{
 			Message: "Validation Failed",
 			Errors:  e.Invalid(),
 		}
 		w.WriteHeader(http.StatusUnprocessableEntity)
 		enc.Encode(ve)
-		return
-	}
-
-	type permissionError interface {
-		PermissionError() []map[string]string
-	}
-	if e, ok := err.(permissionError); ok {
+	case permissionErrorInterface:
 		pe := jsonError{
 			Message: "Permission Denied",
 			Errors:  e.PermissionError(),
@@ -63,36 +80,14 @@ func encodeError(ctx context.Context, err error, w http.ResponseWriter) {
 		w.WriteHeader(http.StatusForbidden)
 		enc.Encode(pe)
 		return
-	}
-
-	if fleet.IsForeignKey(errors.Cause(err)) {
-		ve := jsonError{
-			Message: "Validation Failed",
-			Errors:  baseError(err.Error()),
-		}
-		w.WriteHeader(http.StatusUnprocessableEntity)
-		enc.Encode(ve)
-		return
-	}
-
-	type mailError interface {
-		MailError() []map[string]string
-	}
-	if e, ok := err.(mailError); ok {
+	case mailError:
 		me := jsonError{
 			Message: "Mail Error",
 			Errors:  e.MailError(),
 		}
 		w.WriteHeader(http.StatusInternalServerError)
 		enc.Encode(me)
-		return
-	}
-
-	type osqueryError interface {
-		error
-		NodeInvalid() bool
-	}
-	if e, ok := err.(osqueryError); ok {
+	case osqueryError:
 		// osquery expects to receive the node_invalid key when a TLS
 		// request provides an invalid node_key for authentication. It
 		// doesn't use the error message provided, but we provide this
@@ -108,54 +103,60 @@ func encodeError(ctx context.Context, err error, w http.ResponseWriter) {
 		}
 
 		enc.Encode(errMap)
-		return
-	}
-
-	type notFoundError interface {
-		error
-		IsNotFound() bool
-	}
-	if e, ok := err.(notFoundError); ok {
+	case notFoundErrorInterface:
 		je := jsonError{
 			Message: "Resource Not Found",
 			Errors:  baseError(e.Error()),
 		}
 		w.WriteHeader(http.StatusNotFound)
 		enc.Encode(je)
-		return
-	}
-
-	type existsError interface {
-		error
-		IsExists() bool
-	}
-	if e, ok := err.(existsError); ok {
+	case existsErrorInterface:
 		je := jsonError{
 			Message: "Resource Already Exists",
 			Errors:  baseError(e.Error()),
 		}
 		w.WriteHeader(http.StatusConflict)
 		enc.Encode(je)
-		return
-	}
+	case *mysql.MySQLError:
+		je := jsonError{
+			Message: "Validation Failed",
+			Errors:  baseError(e.Error()),
+		}
+		statusCode := http.StatusUnprocessableEntity
+		if e.Number == 1062 {
+			statusCode = http.StatusConflict
+		}
+		w.WriteHeader(statusCode)
+		enc.Encode(je)
+	default:
+		if fleet.IsForeignKey(errors.Cause(err)) {
+			ve := jsonError{
+				Message: "Validation Failed",
+				Errors:  baseError(err.Error()),
+			}
+			w.WriteHeader(http.StatusUnprocessableEntity)
+			enc.Encode(ve)
+			return
+		}
 
-	// Get specific status code if it is available from this error type,
-	// defaulting to HTTP 500
-	status := http.StatusInternalServerError
-	if e, ok := err.(kithttp.StatusCoder); ok {
-		status = e.StatusCode()
-	}
+		// Get specific status code if it is available from this error type,
+		// defaulting to HTTP 500
+		status := http.StatusInternalServerError
+		if e, ok := err.(kithttp.StatusCoder); ok {
+			status = e.StatusCode()
+		}
 
-	// See header documentation
-	// https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After)
-	if e, ok := err.(fleet.ErrWithRetryAfter); ok {
-		w.Header().Add("Retry-After", strconv.Itoa(e.RetryAfter()))
-	}
+		// See header documentation
+		// https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After)
+		if e, ok := err.(fleet.ErrWithRetryAfter); ok {
+			w.Header().Add("Retry-After", strconv.Itoa(e.RetryAfter()))
+		}
 
-	w.WriteHeader(status)
-	je := jsonError{
-		Message: err.Error(),
-		Errors:  baseError(err.Error()),
+		w.WriteHeader(status)
+		je := jsonError{
+			Message: err.Error(),
+			Errors:  baseError(err.Error()),
+		}
+		enc.Encode(je)
 	}
-	enc.Encode(je)
 }

server/service/transport_error_test.go

@@ -0,0 +1,91 @@
+package service
+
+import (
+	"github.com/fleetdm/fleet/v4/server/fleet"
+	"github.com/stretchr/testify/assert"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+)
+
+type foreignKeyError struct{}
+
+func (foreignKeyError) IsForeignKey() bool { return true }
+func (foreignKeyError) Error() string      { return "" }
+
+type alreadyExists struct{}
+
+func (alreadyExists) IsExists() bool { return false }
+func (alreadyExists) Error() string  { return "" }
+
+type newAndExciting struct{}
+
+func (newAndExciting) Error() string { return "" }
+
+func TestHandlesErrorsCode(t *testing.T) {
+	var errorTests = []struct {
+		name string
+		err  error
+		code int
+	}{
+		{
+			"validation",
+			fleet.NewInvalidArgumentError("a", "b"),
+			http.StatusUnprocessableEntity,
+		},
+		{
+			"permission",
+			fleet.NewPermissionError("a"),
+			http.StatusForbidden,
+		},
+		{
+			"foreign key",
+			foreignKeyError{},
+			http.StatusUnprocessableEntity,
+		},
+		{
+			"mail error",
+			mailError{},
+			http.StatusInternalServerError,
+		},
+		{
+			"osquery error - invalid node",
+			osqueryError{nodeInvalid: true},
+			http.StatusUnauthorized,
+		},
+		{
+			"osquery error - valid node",
+			osqueryError{},
+			http.StatusInternalServerError,
+		},
+		{
+			"data not found",
+			notFoundError{},
+			http.StatusNotFound,
+		},
+		{
+			"already exists",
+			alreadyExists{},
+			http.StatusConflict,
+		},
+		{
+			"status coder",
+			fleet.NewAuthFailedError(""),
+			http.StatusUnauthorized,
+		},
+		{
+			"default",
+			newAndExciting{},
+			http.StatusInternalServerError,
+		},
+	}
+
+	for _, tt := range errorTests {
+		t.Run(tt.name, func(t *testing.T) {
+			recorder := httptest.NewRecorder()
+			encodeError(nil, tt.err, recorder)
+			assert.Equal(t, recorder.Code, tt.code)
+		})
+	}
+
+}

server/service/util_test.go

@@ -1,6 +1,7 @@
 package service
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/WatchBeam/clock"
@@ -55,9 +56,14 @@ func createTestAppConfig(t *testing.T, ds fleet.Datastore) *fleet.AppConfig {
 func createTestUsers(t *testing.T, ds fleet.Datastore) map[string]fleet.User {
 	users := make(map[string]fleet.User)
 	for _, u := range testUsers {
+		role := fleet.RoleObserver
+		if strings.Contains(u.Email, "admin") {
+			role = fleet.RoleAdmin
+		}
 		user := &fleet.User{
-			Name:  "Test Name " + u.Email,
-			Email: u.Email,
+			Name:       "Test Name " + u.Email,
+			Email:      u.Email,
+			GlobalRole: &role,
 		}
 		err := user.SetPassword(u.PlaintextPassword, 10, 10)
 		require.Nil(t, err)