Fixes + proposed changes to client error handling (#1759)

- Fix places where we accidentally return nil when we should return an error.
- Simplify interfaces/implementation of specialized errors
- Use more specific error messages
- Consistent JSON decoding

server/service/client_errors.go

@@ -10,19 +10,13 @@ type setupAlreadyErr struct {
 }
 
 func (e setupAlreadyErr) Error() string {
-	return e.reason
+	return "Kolide Fleet has already been setup"
 }
 
 func (e setupAlreadyErr) SetupAlready() bool {
 	return true
 }
 
-func setupAlready() error {
-	return setupAlreadyErr{
-		reason: "Kolide Fleet has already been setup",
-	}
-}
-
 type InvalidLoginErr interface {
 	InvalidLogin() bool
 	Error() string
@@ -33,19 +27,13 @@ type invalidLoginErr struct {
 }
 
 func (e invalidLoginErr) Error() string {
-	return e.reason
+	return "The credentials supplied were invalid"
 }
 
 func (e invalidLoginErr) InvalidLogin() bool {
 	return true
 }
 
-func invalidLogin() error {
-	return invalidLoginErr{
-		reason: "The credentials supplied were invalid",
-	}
-}
-
 type NotSetupErr interface {
 	NotSetup() bool
 	Error() string
@@ -56,15 +44,9 @@ type notSetupErr struct {
 }
 
 func (e notSetupErr) Error() string {
-	return e.reason
+	return "The Kolide Fleet instance is not set up yet"
 }
 
 func (e notSetupErr) NotSetup() bool {
 	return true
 }
-
-func notSetup() error {
-	return notSetupErr{
-		reason: "The Kolide Fleet instance is not setup yet",
-	}
-}

server/service/client_sessions.go

@@ -2,7 +2,6 @@ package service
 
 import (
 	"encoding/json"
-	"io/ioutil"
 	"net/http"
 
 	"github.com/pkg/errors"
@@ -18,29 +17,29 @@ func (c *Client) Login(email, password string) (string, error) {
 
 	response, err := c.Do("POST", "/api/v1/kolide/login", params)
 	if err != nil {
-		return "", errors.Wrap(err, "error making request")
+		return "", errors.Wrap(err, "POST /api/v1/kolide/login")
 	}
 	defer response.Body.Close()
 
 	switch response.StatusCode {
 	case http.StatusNotFound:
-		return "", notSetup()
+		return "", notSetupErr{}
 	case http.StatusUnauthorized:
-		return "", invalidLogin()
+		return "", invalidLoginErr{}
 	}
 
 	if response.StatusCode != http.StatusOK {
-		return "", errors.Errorf("Received HTTP %d instead of HTTP 200", response.StatusCode)
+		return "", errors.Errorf("login got HTTP %d, expected 200", response.StatusCode)
 	}
 
 	var responseBody loginResponse
 	err = json.NewDecoder(response.Body).Decode(&responseBody)
 	if err != nil {
-		return "", errors.Wrap(err, "error decoding HTTP response body")
+		return "", errors.Wrap(err, "decode login response")
 	}
 
 	if responseBody.Err != nil {
-		return "", errors.Wrap(err, "error setting up fleet instance")
+		return "", errors.Errorf("login: %s", responseBody.Err)
 	}
 
 	return responseBody.Token, nil
@@ -50,27 +49,22 @@ func (c *Client) Login(email, password string) (string, error) {
 func (c *Client) Logout() error {
 	response, err := c.AuthenticatedDo("POST", "/api/v1/kolide/logout", nil)
 	if err != nil {
-		return errors.Wrap(err, "error making request")
+		return errors.Wrap(err, "POST /api/v1/kolide/logout")
 	}
 	defer response.Body.Close()
 
 	if response.StatusCode != http.StatusOK {
-		return errors.Errorf("Received HTTP %d instead of HTTP 200", response.StatusCode)
-	}
-
-	responeBytes, err := ioutil.ReadAll(response.Body)
-	if err != nil {
-		return errors.Wrap(err, "error reading response body")
+		return errors.Errorf("logout got HTTP %d, expected 200", response.StatusCode)
 	}
 
 	var responseBody logoutResponse
-	err = json.Unmarshal(responeBytes, &responseBody)
+	err = json.NewDecoder(response.Body).Decode(&responseBody)
 	if err != nil {
-		return errors.Wrap(err, "error decoding HTTP response body")
+		return errors.Wrap(err, "decode logout response")
 	}
 
 	if responseBody.Err != nil {
-		return errors.Wrap(err, "error logging out of Fleet")
+		return errors.Errorf("logout: %s", responseBody.Err)
 	}
 
 	return nil

server/service/client_setup.go

@@ -27,28 +27,28 @@ func (c *Client) Setup(email, password, org string) (string, error) {
 
 	response, err := c.Do("POST", "/api/v1/setup", params)
 	if err != nil {
-		return "", errors.Wrap(err, "error making request")
+		return "", errors.Wrap(err, "POST /api/v1/setup")
 	}
 	defer response.Body.Close()
 
 	// If setup has already been completed, Kolide Fleet will not serve the
 	// setup route, which will cause the request to 404
 	if response.StatusCode == http.StatusNotFound {
-		return "", setupAlready()
+		return "", setupAlreadyErr{}
 	}
 
 	if response.StatusCode != http.StatusOK {
-		return "", errors.Errorf("Received HTTP %d instead of HTTP 200", response.StatusCode)
+		return "", errors.Errorf("setup got HTTP %d, expected 200", response.StatusCode)
 	}
 
 	var responseBody setupResponse
 	err = json.NewDecoder(response.Body).Decode(&responseBody)
 	if err != nil {
-		return "", errors.Wrap(err, "error decoding HTTP response body")
+		return "", errors.Wrap(err, "decode setup response")
 	}
 
 	if responseBody.Err != nil {
-		return "", errors.Wrap(err, "error setting up fleet instance")
+		return "", errors.Errorf("setup: %s", responseBody.Err)
 	}
 
 	return *responseBody.Token, nil