diff --git a/changes/issue-1286-improve-errors b/changes/issue-1286-improve-errors new file mode 100644 index 0000000000..7bd5cbb3a9 --- /dev/null +++ b/changes/issue-1286-improve-errors @@ -0,0 +1 @@ +* Improve error messages at the API level. Fixes issue 1286. diff --git a/server/datastore/datastore.go b/server/datastore/datastore.go index 98d5c846cf..aa8b3c154b 100644 --- a/server/datastore/datastore.go +++ b/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, diff --git a/server/datastore/mysql/datastore_test.go b/server/datastore/mysql/datastore_test.go index e5a9595f31..cd859d34b7 100644 --- a/server/datastore/mysql/datastore_test.go +++ b/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) } diff --git a/server/datastore/mysql/testing_utils.go b/server/datastore/mysql/testing_utils.go new file mode 100644 index 0000000000..3d4427b0bb --- /dev/null +++ b/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) + } +} diff --git a/server/service/http_auth_test.go b/server/service/http_auth_test.go index 472c6b1d0a..b1bc783bb3 100644 --- a/server/service/http_auth_test.go +++ b/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) { diff --git a/server/service/integration_test.go b/server/service/integration_test.go new file mode 100644 index 0000000000..b84feb1a46 --- /dev/null +++ b/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(¶ms) + 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(¶ms) + 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(¶ms) + 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, + }) +} diff --git a/server/service/transport_error.go b/server/service/transport_error.go index 3659bf02d6..7d088aa67a 100644 --- a/server/service/transport_error.go +++ b/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) } diff --git a/server/service/transport_error_test.go b/server/service/transport_error_test.go new file mode 100644 index 0000000000..ea5b1bac8b --- /dev/null +++ b/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) + }) + } + +} diff --git a/server/service/util_test.go b/server/service/util_test.go index 4a5af9baca..c31c2829ed 100644 --- a/server/service/util_test.go +++ b/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)