Translate the AppConfig and Team validation messages to be more user-friendly (#8171)

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.

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`,
 		},
 		{

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")
 				}
 			}
 		}

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

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
 }

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
+	}
+}

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)
+		})
+	}
+}

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")
 			}
 		}
 	}

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

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)
 	}