Decide on and implement error handling

[mikestone14]

To get client-side error messages to display appropriately to a user, we need to decide on how we're sending back errors from the server.

Currently we're sending back a response body with a key of error and a value of <error status code name>: <error message>. (see below)

HTTP/1.1 401 Unauthorized
Access-Control-Allow-Headers: Origin, Content-Type
Access-Control-Allow-Methods: GET, POST, OPTIONS
Access-Control-Allow-Origin: *
Content-Type: application/json; charset=utf-8
Date: Mon, 12 Sep 2016 14:34:01 GMT
Content-Length: 63
Connection: close

{
  "error": "unauthorized: invalid password for user admin"
}

This response makes it difficult to show messages on the client as it's hard to know what form field the error might relate to and what exactly the message is to display. For example, the error in the screenshot below would be difficult to place on the client:

I would propose sending back and error object with keys representing an attribute with an error, and a value of the error message for that attribute. For example (using the design above):

HTTP/1.1 401 Unauthorized
Access-Control-Allow-Headers: Origin, Content-Type
Access-Control-Allow-Methods: GET, POST, OPTIONS
Access-Control-Allow-Origin: *
Content-Type: application/json; charset=utf-8
Date: Mon, 12 Sep 2016 14:34:01 GMT
Content-Length: 63
Connection: close

{
  "username": "username or email and password do not match",
}

[terracatta]

I think we wanted to model our approach off of Github's API - https://developer.github.com/v3/#client-errors. @mikestone14 would this work for you?

[zwass]

Previous to the transition to go-kit we were passing validation errors with the JSON key so that the client could make sense of it. @groob and @marpaia is that no longer the case? I agree that authorization errors should probably not include additional details as @groob mentioned in chat.

[groob]

validation errors still return the field where the issue occurred, it's just not enforced everywhere at the moment. See NewUser for example:

HTTP/1.1 422 Unprocessable Entity

{
  "error": "required argument invalid or missing: email"
}

vs

HTTP/1.1 422 Unprocessable Entity

{
  "error": "required argument invalid or missing: password"
}

I agree with @mikestone14 that parsing the error string is not a good way of solving the problem, but I'm also not sure what a perfect solution would be. The github api example covers validation errors, but that's only a small subset of the possible API errors returned.

[groob]

My original thoughts on the single error field are as follows:

• API should return an appropriate HTTP status code, with a default of 500 when the error cannot be determined.
• the error message should be shown to the user directly in cases where it's helpful to the user, like the 422 examples above. The error message is descriptive enough to pinpoint the problem.

However, I haven't considered the need to pinpoint a specific field in the request body. Is this a requirement for every error type? What would it look like outside of errors which are not related to validation errors?

[groob]

related #151

[zwass]

I think that our internal error representation should be able to hold a reference to which field in the request caused the error, and then the logic that writes the error response can decide whether to write that information.

While I agree that a user could be able to parse the error message to find the problem, I don't think this is a pleasant UX. We should aim for form fields that will highlight when they have errors, with the appropriate error message displayed nearby.

[groob]

The validation error on the backend already addresses this, so marshaling these in a similar way to the Github API example, would be easy.

type invalidArgumentError struct {
    field    string
    required bool
}

// invalidArgumentError is returned when one or more arguments are invalid.
func (e invalidArgumentError) Error() string {
    req := "optional"
    if e.required {
        req = "required"
    }
    return fmt.Sprintf("%s argument invalid or missing: %s", req, e.field)
}

Do you have a suggestion for how to address/handle non validation errors?

[groob]

Related, see the RFC for API Errors: https://tools.ietf.org/html/rfc7807

example:

   HTTP/1.1 400 Bad Request
   Content-Type: application/problem+json
   Content-Language: en

   {
   "type": "https://example.net/validation-error",
   "title": "Your request parameters didn't validate.",
   "invalid-params": [ {
                         "name": "age",
                         "reason": "must be a positive integer"
                       },
                       {
                         "name": "color",
                         "reason": "must be 'green', 'red' or 'blue'"}
                     ]
   }

[zwass]

invalidArgumentError looks like a good start, but it doesn't seem to have any support for multiple invalid arguments.

What if we made something like

type ParamError struct {
  Name string
  Reason string
}

and then each error type also included a []ParamError field?

Then these could be serialized in the RFC7807 format you describe above, or any other similar format.

[zwass]

We could also add a BaseError struct that could be embedded in each of the specialized error structs. Something like:

type BaseError struct {
  ParamErrors []ParamError
  Message string
  StatusCode int
}

[groob]

I don't like having the StatusCode as part of each error. Currently, there's a single switch that has cases for different error types when encoding errors into an HTTP response. This has two benefits,

• there's one place to check any error into a status code, as opposed to doing this in every error return.
• it doesn't tie the service/datastore to HTTP status codes directly. having the status code as part of every custom error seems like a leaky abstraction to me.

On the other hand, having the BaseError struct as part of the http encodeError function instead of the current map[string]interface{} would be an improvement.

[zwass]

Good point about status codes. I agree with you that they should not be defined at the point where the error is thrown.

[groob]

@marpaia I have some questions regarding Authentication errors which are returned to the client. When is it ok to be granular about the reason authentication failed, and when is it not?

Scenarios:

• user not found: should the client see no such user or bad credentials?
• account is disabled: should the client see account disabled or bad credentials?
• invalid password should the client see invalid password or bad credentials?
• invalid token should the client see invalid token or bad credentials?

I'd like to report bad credentials for both no such user and invalid password, but return the real reason for account disabled and invalid token. The server will log the real reason authentication failed. Thoughts?

[marpaia]

I could see returning "account disabled" for that error, but if someone is messing with their token, I personally think we should just return "bad credentials". A token is a credential and only an attacker would be messing with their tokens so let's not do them any favors.