diff --git a/changes/issue-7835-translate-appconfig-and-team-error-messages b/changes/issue-7835-translate-appconfig-and-team-error-messages new file mode 100644 index 0000000000..e7de220a85 --- /dev/null +++ b/changes/issue-7835-translate-appconfig-and-team-error-messages @@ -0,0 +1 @@ +* Translated technical error messages returned by Organization's and Team's validations to be more user-friendly. diff --git a/cmd/fleetctl/apply_test.go b/cmd/fleetctl/apply_test.go index 715bd0743a..4e8b6df54b 100644 --- a/cmd/fleetctl/apply_test.go +++ b/cmd/fleetctl/apply_test.go @@ -305,7 +305,7 @@ spec: `) runAppCheckErr(t, []string{"apply", "-f", name}, - "applying fleet config: PATCH /api/latest/fleet/config received status 400 Bad request: json: unknown field \"enabled_software_inventory\"", + "applying fleet config: PATCH /api/latest/fleet/config received status 400 Bad Request: unsupported key provided: \"enabled_software_inventory\"", ) require.Nil(t, savedAppConfig) } @@ -743,7 +743,7 @@ spec: config: blah: nope `, - wantErr: `400 Bad request: common config: json: unknown field "blah"`, + wantErr: `400 Bad Request: unsupported key provided: "blah"`, }, { desc: "invalid top-level key for team", @@ -769,7 +769,7 @@ spec: config: blah: nope `, - wantErr: `400 Bad request: common config: json: unknown field "blah"`, + wantErr: `400 Bad Request: unsupported key provided: "blah"`, }, { desc: "invalid agent options dry-run", @@ -784,7 +784,7 @@ spec: blah: nope `, flags: []string{"--dry-run"}, - wantErr: `400 Bad request: common config: json: unknown field "blah"`, + wantErr: `400 Bad Request: unsupported key provided: "blah"`, }, { desc: "invalid agent options force", @@ -815,7 +815,7 @@ spec: aws_debug: 123 `, flags: []string{"--dry-run"}, - wantErr: `400 Bad request: common config: json: cannot unmarshal number into Go struct field osqueryOptions.options.aws_debug of type bool`, + wantErr: `400 Bad Request: invalid value type at 'options.aws_debug': expected bool but got number`, }, { desc: "invalid team agent options command-line flag", @@ -829,7 +829,7 @@ spec: command_line_flags: no_such_flag: 123 `, - wantErr: `400 Bad request: command-line flags: json: unknown field "no_such_flag"`, + wantErr: `400 Bad Request: unsupported key provided: "no_such_flag"`, }, { desc: "valid team agent options command-line flag", @@ -863,7 +863,7 @@ spec: options: aws_debug: 123 `, - wantErr: `400 Bad request: darwin platform config: json: cannot unmarshal number into Go struct field osqueryOptions.options.aws_debug of type bool`, + wantErr: `400 Bad Request: invalid value type at 'options.aws_debug': expected bool but got number`, }, { desc: "empty config", @@ -905,7 +905,7 @@ spec: server_settings: foo: bar `, - wantErr: `400 Bad request: json: unknown field "foo"`, + wantErr: `400 Bad Request: unsupported key provided: "foo"`, }, { desc: "config with invalid key type", @@ -928,7 +928,7 @@ spec: foo: bar `, flags: []string{"--dry-run"}, - wantErr: `400 Bad request: json: unknown field "foo"`, + wantErr: `400 Bad Request: unsupported key provided: "foo"`, }, { desc: "config with invalid agent options data type in dry-run", @@ -942,7 +942,7 @@ spec: aws_debug: 123 `, flags: []string{"--dry-run"}, - wantErr: `400 Bad request: common config: json: cannot unmarshal number into Go struct field osqueryOptions.options.aws_debug of type bool`, + wantErr: `400 Bad Request: invalid value type at 'options.aws_debug': expected bool but got number`, }, { desc: "config with invalid agent options data type with force", @@ -969,7 +969,7 @@ spec: enable_tables: "foo" no_such_flag: false `, - wantErr: `command-line flags: json: unknown field "no_such_flag"`, + wantErr: `400 Bad Request: unsupported key provided: "no_such_flag"`, }, { desc: "config with invalid value for agent options command-line flags", @@ -981,7 +981,7 @@ spec: command_line_flags: enable_tables: 123 `, - wantErr: `command-line flags: json: cannot unmarshal number into Go struct field osqueryCommandLineFlags.enable_tables of type string`, + wantErr: `400 Bad Request: invalid value type at 'enable_tables': expected string but got number`, }, { desc: "config with valid agent options command-line flags", @@ -1029,7 +1029,7 @@ spec: enable_software_inventory: true `, flags: []string{"--dry-run"}, - wantErr: `400 Bad request: warning: deprecated settings were used in the configuration`, + wantErr: `400 Bad request: warning: deprecated settings were used in the configuration: [host_settings]`, wantOutput: `[!] ignoring labels, dry run mode only supported for 'config' and 'team' spec`, }, { diff --git a/ee/server/service/teams.go b/ee/server/service/teams.go index a2c924ba8f..caf109d4bb 100644 --- a/ee/server/service/teams.go +++ b/ee/server/service/teams.go @@ -5,6 +5,7 @@ import ( "database/sql" "encoding/json" "fmt" + "net/http" "github.com/fleetdm/fleet/v4/server" "github.com/fleetdm/fleet/v4/server/authz" @@ -148,11 +149,12 @@ func (svc *Service) ModifyTeamAgentOptions(ctx context.Context, teamID uint, tea if teamOptions != nil { if err := fleet.ValidateJSONAgentOptions(teamOptions); err != nil { + err = fleet.NewUserMessageError(err, http.StatusBadRequest) if applyOptions.Force && !applyOptions.DryRun { level.Info(svc.logger).Log("err", err, "msg", "force-apply team agent options with validation errors") } if !applyOptions.Force { - return nil, ctxerr.Wrap(ctx, &fleet.BadRequestError{Message: err.Error()}, "validate agent options") + return nil, ctxerr.Wrap(ctx, err, "validate agent options") } } } @@ -435,11 +437,12 @@ func (svc *Service) ApplyTeamSpecs(ctx context.Context, specs []*fleet.TeamSpec, if spec.AgentOptions != nil { if err := fleet.ValidateJSONAgentOptions(*spec.AgentOptions); err != nil { + err = fleet.NewUserMessageError(err, http.StatusBadRequest) if applyOpts.Force && !applyOpts.DryRun { level.Info(svc.logger).Log("err", err, "msg", "force-apply team agent options with validation errors") } if !applyOpts.Force { - return ctxerr.Wrap(ctx, &fleet.BadRequestError{Message: err.Error()}, "validate agent options") + return ctxerr.Wrap(ctx, err, "validate agent options") } } } diff --git a/server/contexts/ctxerr/ctxerr.go b/server/contexts/ctxerr/ctxerr.go index e7578bbef2..6841e9f5ed 100644 --- a/server/contexts/ctxerr/ctxerr.go +++ b/server/contexts/ctxerr/ctxerr.go @@ -21,6 +21,7 @@ import ( "github.com/fleetdm/fleet/v4/server/contexts/host" "github.com/fleetdm/fleet/v4/server/contexts/viewer" + "github.com/fleetdm/fleet/v4/server/fleet" ) type key int @@ -165,13 +166,7 @@ func Wrapf(ctx context.Context, cause error, format string, args ...interface{}) // Cause returns the root error in err's chain. func Cause(err error) error { - for { - uerr := Unwrap(err) - if uerr == nil { - return err - } - err = uerr - } + return fleet.Cause(err) } // FleetCause is similar to Cause, but returns the root-most diff --git a/server/fleet/app.go b/server/fleet/app.go index daadbd0555..01fd48da7c 100644 --- a/server/fleet/app.go +++ b/server/fleet/app.go @@ -7,6 +7,7 @@ import ( "fmt" "io" "net/url" + "sort" "time" "github.com/fleetdm/fleet/v4/server/config" @@ -127,9 +128,9 @@ type AppConfig struct { // when true, strictDecoding causes the UnmarshalJSON method to return an // error if there are unknown fields in the raw JSON. strictDecoding bool - // this field is set to true during UnmarshalJSON if any legacy settings - // were set in the raw JSON. - didUnmarshalLegacySettings bool + // this field is set to the list of legacy settings keys during UnmarshalJSON + // if any legacy settings were set in the raw JSON. + didUnmarshalLegacySettings []string } // legacyConfig holds settings that have been replaced, superceded or @@ -274,9 +275,9 @@ func (c *AppConfig) ApplyDefaults() { // EnableStrictDecoding enables strict decoding of the AppConfig struct. func (c *AppConfig) EnableStrictDecoding() { c.strictDecoding = true } -// DidUnmarshalLegacySettings returns true if any legacy setting was set -// in the JSON used to unmarshal this AppConfig. -func (c *AppConfig) DidUnmarshalLegacySettings() bool { return c.didUnmarshalLegacySettings } +// DidUnmarshalLegacySettings returns the list of legacy settings keys that +// were set in the JSON used to unmarshal this AppConfig. +func (c *AppConfig) DidUnmarshalLegacySettings() []string { return c.didUnmarshalLegacySettings } // UnmarshalJSON implements the json.Unmarshaler interface. func (c *AppConfig) UnmarshalJSON(b []byte) error { @@ -291,6 +292,7 @@ func (c *AppConfig) UnmarshalJSON(b []byte) error { (*cfgStructUnmarshal)(c), } + c.didUnmarshalLegacySettings = nil decoder := json.NewDecoder(bytes.NewReader(b)) if c.strictDecoding { decoder.DisallowUnknownFields() @@ -306,9 +308,10 @@ func (c *AppConfig) UnmarshalJSON(b []byte) error { // This has the drawback of legacy fields taking precedence over new fields // if both are defined. if compatConfig.legacyConfig.HostSettings != nil { - c.didUnmarshalLegacySettings = true + c.didUnmarshalLegacySettings = append(c.didUnmarshalLegacySettings, "host_settings") c.Features = *compatConfig.legacyConfig.HostSettings } + sort.Strings(c.didUnmarshalLegacySettings) return nil } diff --git a/server/fleet/errors.go b/server/fleet/errors.go index cae4c732f3..650b563f17 100644 --- a/server/fleet/errors.go +++ b/server/fleet/errors.go @@ -1,9 +1,13 @@ package fleet import ( + "encoding/json" "errors" "fmt" "net/http" + "reflect" + "regexp" + "strings" ) var ( @@ -267,3 +271,74 @@ func NewErrorf(code int, format string, args ...interface{}) error { func (ge *Error) Error() string { return ge.Message } + +// UserMessageError is an error that adds the UserMessage interface +// implementation. +type UserMessageError struct { + error + statusCode int +} + +// NewUserMessageError creates a UserMessageError that will translate the +// error message of err to a user-friendly form. If statusCode is > 0, it +// will be used as the HTTP status code for the error, otherwise it defaults +// to http.StatusUnprocessableEntity (422). +func NewUserMessageError(err error, statusCode int) *UserMessageError { + if err == nil { + return nil + } + return &UserMessageError{err, statusCode} +} + +var rxJSONUnknownField = regexp.MustCompile(`^json: unknown field "(.+)"$`) + +// UserMessage implements the user-friendly translation of the error if its +// root cause is one of the supported types, otherwise it returns the error +// message. +func (e UserMessageError) UserMessage() string { + cause := Cause(e.error) + switch cause := cause.(type) { + case *json.UnmarshalTypeError: + var sb strings.Builder + curType := cause.Type + for curType.Kind() == reflect.Slice || curType.Kind() == reflect.Array { + sb.WriteString("array of ") + curType = curType.Elem() + } + sb.WriteString(curType.Name()) + if curType != cause.Type { + // it was an array + sb.WriteString("s") + } + + return fmt.Sprintf("invalid value type at '%s': expected %s but got %s", cause.Field, sb.String(), cause.Value) + + default: + // there's no specific error type for the strict json mode + // (DisallowUnknownFields), so resort to message-matching. + if matches := rxJSONUnknownField.FindStringSubmatch(cause.Error()); matches != nil { + return fmt.Sprintf("unsupported key provided: %q", matches[1]) + } + return e.Error() + } +} + +// StatusCode implements the kithttp.StatusCoder interface to return the status +// code to use in HTTP API responses. +func (e UserMessageError) StatusCode() int { + if e.statusCode > 0 { + return e.statusCode + } + return http.StatusUnprocessableEntity +} + +// Cause returns the root error in err's chain. +func Cause(err error) error { + for { + uerr := errors.Unwrap(err) + if uerr == nil { + return err + } + err = uerr + } +} diff --git a/server/fleet/errors_test.go b/server/fleet/errors_test.go new file mode 100644 index 0000000000..88fcbbb961 --- /dev/null +++ b/server/fleet/errors_test.go @@ -0,0 +1,47 @@ +package fleet + +import ( + "fmt" + "io" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestUserMessageErrors(t *testing.T) { + var barString struct { + Bar string `json:"bar"` + } + + type inner struct { + Foo int `json:"foo"` + Strings []string `json:"strings"` + IntsInts [][]int `json:"ints_ints"` + StringPtr *string `json:"string_ptr"` + } + type outer struct { + Inner inner `json:"inner"` + } + var nestedFoo outer + + cases := []struct { + in error + out string + }{ + {io.EOF, "EOF"}, + {jsonStrictDecode([]byte(`{"foo":1}`), &barString), `unsupported key provided: "foo"`}, + {jsonStrictDecode([]byte(`{"bar":1}`), &barString), `invalid value type at 'bar': expected string but got number`}, + {jsonStrictDecode([]byte(`{"inner":{"foo":"bar"}}`), &nestedFoo), `invalid value type at 'inner.foo': expected int but got string`}, + {jsonStrictDecode([]byte(`{"inner":{"strings":true}}`), &nestedFoo), `invalid value type at 'inner.strings': expected array of strings but got bool`}, + {jsonStrictDecode([]byte(`{"inner":{"ints_ints":true}}`), &nestedFoo), `invalid value type at 'inner.ints_ints': expected array of array of ints but got bool`}, + {jsonStrictDecode([]byte(`{"inner":{"string_ptr":true}}`), &nestedFoo), `invalid value type at 'inner.string_ptr': expected string but got bool`}, + } + + for _, c := range cases { + t.Run(fmt.Sprintf("%T: %[1]q", c.in), func(t *testing.T) { + ume := NewUserMessageError(c.in, 0) + got := ume.UserMessage() + require.Contains(t, got, c.out) + }) + } +} diff --git a/server/service/appconfig.go b/server/service/appconfig.go index 99d030f576..9be32ad8c3 100644 --- a/server/service/appconfig.go +++ b/server/service/appconfig.go @@ -8,8 +8,10 @@ import ( "encoding/json" "encoding/pem" "errors" + "fmt" "io" "net" + "net/http" "net/url" "github.com/fleetdm/fleet/v4/server/authz" @@ -276,14 +278,15 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle // We apply the config that is incoming to the old one appConfig.EnableStrictDecoding() if err := json.Unmarshal(p, &appConfig); err != nil { - return nil, ctxerr.Wrap(ctx, &fleet.BadRequestError{Message: err.Error()}) + err = fleet.NewUserMessageError(err, http.StatusBadRequest) + return nil, ctxerr.Wrap(ctx, err) } var legacyUsedWarning error - if appConfig.DidUnmarshalLegacySettings() { + if legacyKeys := appConfig.DidUnmarshalLegacySettings(); len(legacyKeys) > 0 { // this "warning" is returned only in dry-run mode, and if no other errors // were encountered. legacyUsedWarning = &fleet.BadRequestError{ - Message: "warning: deprecated settings were used in the configuration; consider updating to the new settings: https://fleetdm.com/docs/using-fleet/configuration-files#settings", + Message: fmt.Sprintf("warning: deprecated settings were used in the configuration: %v; consider updating to the new settings: https://fleetdm.com/docs/using-fleet/configuration-files#settings", legacyKeys), } } @@ -300,11 +303,12 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle // if there were Agent Options in the new app config, then it replaced the // agent options in the resulting app config, so validate those. if err := fleet.ValidateJSONAgentOptions(*appConfig.AgentOptions); err != nil { + err = fleet.NewUserMessageError(err, http.StatusBadRequest) if applyOpts.Force && !applyOpts.DryRun { level.Info(svc.logger).Log("err", err, "msg", "force-apply appConfig agent options with validation errors") } if !applyOpts.Force { - return nil, ctxerr.Wrap(ctx, &fleet.BadRequestError{Message: err.Error()}, "validate agent options") + return nil, ctxerr.Wrap(ctx, err, "validate agent options") } } } diff --git a/server/service/integration_enterprise_test.go b/server/service/integration_enterprise_test.go index ddc9300695..17a5f7efda 100644 --- a/server/service/integration_enterprise_test.go +++ b/server/service/integration_enterprise_test.go @@ -588,7 +588,7 @@ func (s *integrationEnterpriseTestSuite) TestTeamEndpoints() { }`), http.StatusBadRequest, "dry_run", "true") body, err := io.ReadAll(resp.Body) require.NoError(t, err) - require.Contains(t, string(body), "cannot unmarshal string into Go struct field osqueryOptions.options.aws_debug of type bool") + require.Contains(t, string(body), "invalid value type at 'options.aws_debug': expected bool but got string") // modify team agent using valid options with dry-run tmResp.Team = nil diff --git a/server/service/transport_error.go b/server/service/transport_error.go index 07da99eb9e..74ab82f490 100644 --- a/server/service/transport_error.go +++ b/server/service/transport_error.go @@ -185,7 +185,7 @@ func encodeError(ctx context.Context, err error, w http.ResponseWriter) { enc.Encode(je) return } - if fleet.IsForeignKey(ctxerr.Cause(err)) { + if fleet.IsForeignKey(err) { ve := jsonError{ Message: "Validation Failed", Errors: baseError(err.Error()), @@ -210,10 +210,20 @@ func encodeError(ctx context.Context, err error, w http.ResponseWriter) { w.Header().Add("Retry-After", strconv.Itoa(ewra.RetryAfter())) } + msg := err.Error() + reason := err.Error() + var ume *fleet.UserMessageError + if errors.As(err, &ume) { + if text := http.StatusText(status); text != "" { + msg = text + } + reason = ume.UserMessage() + } + w.WriteHeader(status) je := jsonError{ - Message: err.Error(), - Errors: baseError(err.Error()), + Message: msg, + Errors: baseError(reason), } enc.Encode(je) }