From 8febf3ed96fd24098e2e9fa693e6d38185490191 Mon Sep 17 00:00:00 2001 From: Zachary Wasserman Date: Fri, 4 May 2018 14:55:57 -0700 Subject: [PATCH] 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 | 24 +++--------------------- server/service/client_sessions.go | 28 +++++++++++----------------- server/service/client_setup.go | 10 +++++----- 3 files changed, 19 insertions(+), 43 deletions(-) diff --git a/server/service/client_errors.go b/server/service/client_errors.go index 9ddba3147e..4041f9c8f8 100644 --- a/server/service/client_errors.go +++ b/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", - } -} diff --git a/server/service/client_sessions.go b/server/service/client_sessions.go index 9ea02a5628..bf96de64b1 100644 --- a/server/service/client_sessions.go +++ b/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 diff --git a/server/service/client_setup.go b/server/service/client_setup.go index 171d9ad5ae..a5fd69ed19 100644 --- a/server/service/client_setup.go +++ b/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