Skip to content

Commit

Permalink
chore: Update BE http errors to be ui friendly (#1994)
Browse files Browse the repository at this point in the history
* chore: More UI friendly errors

Mainly capitlization + messages prefix error
  • Loading branch information
Emyrk committed Jun 3, 2022
1 parent 847e2b1 commit c9a4642
Show file tree
Hide file tree
Showing 42 changed files with 544 additions and 317 deletions.
2 changes: 1 addition & 1 deletion cli/autostart.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func autostartShow() *cobra.Command {
validSchedule, err := schedule.Weekly(*workspace.AutostartSchedule)
if err != nil {
// This should never happen.
_, _ = fmt.Fprintf(cmd.OutOrStdout(), "invalid autostart schedule %q for workspace %s: %s\n", *workspace.AutostartSchedule, workspace.Name, err.Error())
_, _ = fmt.Fprintf(cmd.OutOrStdout(), "Invalid autostart schedule %q for workspace %s: %s\n", *workspace.AutostartSchedule, workspace.Name, err.Error())
return nil
}

Expand Down
4 changes: 2 additions & 2 deletions cli/autostart_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ func TestAutostart(t *testing.T) {
clitest.SetupConfig(t, client, root)

err := cmd.Execute()
require.ErrorContains(t, err, "status code 403: forbidden", "unexpected error")
require.ErrorContains(t, err, "status code 403: Forbidden", "unexpected error")
})

t.Run("Disable_NotFound", func(t *testing.T) {
Expand All @@ -125,7 +125,7 @@ func TestAutostart(t *testing.T) {
clitest.SetupConfig(t, client, root)

err := cmd.Execute()
require.ErrorContains(t, err, "status code 403: forbidden", "unexpected error")
require.ErrorContains(t, err, "status code 403: Forbidden", "unexpected error")
})

t.Run("Enable_DefaultSchedule", func(t *testing.T) {
Expand Down
4 changes: 2 additions & 2 deletions cli/ttl_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ func TestTTL(t *testing.T) {
clitest.SetupConfig(t, client, root)

err := cmd.Execute()
require.ErrorContains(t, err, "status code 403: forbidden", "unexpected error")
require.ErrorContains(t, err, "status code 403: Forbidden", "unexpected error")
})

t.Run("Unset_NotFound", func(t *testing.T) {
Expand All @@ -166,6 +166,6 @@ func TestTTL(t *testing.T) {
clitest.SetupConfig(t, client, root)

err := cmd.Execute()
require.ErrorContains(t, err, "status code 403: forbidden", "unexpected error")
require.ErrorContains(t, err, "status code 403: Forbidden", "unexpected error")
})
}
4 changes: 1 addition & 3 deletions coderd/authorize.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,7 @@ func (api *API) Authorize(rw http.ResponseWriter, r *http.Request, action rbac.A
roles := httpmw.AuthorizationUserRoles(r)
err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object.RBACObject())
if err != nil {
httpapi.Write(rw, http.StatusForbidden, httpapi.Response{
Message: err.Error(),
})
httpapi.Forbidden(rw)

// Log the errors for debugging
internalError := new(rbac.UnauthorizedError)
Expand Down
3 changes: 2 additions & 1 deletion coderd/csp.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ func (api *API) logReportCSPViolations(rw http.ResponseWriter, r *http.Request)
if err != nil {
api.Logger.Warn(ctx, "csp violation", slog.Error(err))
httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{
Message: "failed to read body",
Message: "Failed to read body, invalid json",
Detail: err.Error(),
})
return
}
Expand Down
13 changes: 8 additions & 5 deletions coderd/files.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func (api *API) postFile(rw http.ResponseWriter, r *http.Request) {
case "application/x-tar":
default:
httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{
Message: fmt.Sprintf("unsupported content type: %s", contentType),
Message: fmt.Sprintf("Unsupported content type header %q", contentType),
})
return
}
Expand All @@ -41,7 +41,8 @@ func (api *API) postFile(rw http.ResponseWriter, r *http.Request) {
data, err := io.ReadAll(r.Body)
if err != nil {
httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{
Message: fmt.Sprintf("read file: %s", err),
Message: "Failed to read file from request",
Detail: err.Error(),
})
return
}
Expand All @@ -64,7 +65,8 @@ func (api *API) postFile(rw http.ResponseWriter, r *http.Request) {
})
if err != nil {
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
Message: fmt.Sprintf("insert file: %s", err),
Message: "Internal error saving file",
Detail: err.Error(),
})
return
}
Expand All @@ -78,7 +80,7 @@ func (api *API) fileByHash(rw http.ResponseWriter, r *http.Request) {
hash := chi.URLParam(r, "hash")
if hash == "" {
httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{
Message: "hash must be provided",
Message: "File hash must be provided in url",
})
return
}
Expand All @@ -89,7 +91,8 @@ func (api *API) fileByHash(rw http.ResponseWriter, r *http.Request) {
}
if err != nil {
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
Message: fmt.Sprintf("get file: %s", err),
Message: "Internal error fetching file",
Detail: err.Error(),
})
return
}
Expand Down
25 changes: 16 additions & 9 deletions coderd/gitsshkey.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package coderd

import (
"fmt"
"net/http"

"github.com/coder/coder/coderd/database"
Expand All @@ -22,7 +21,8 @@ func (api *API) regenerateGitSSHKey(rw http.ResponseWriter, r *http.Request) {
privateKey, publicKey, err := gitsshkey.Generate(api.SSHKeygenAlgorithm)
if err != nil {
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
Message: fmt.Sprintf("regenerate key pair: %s", err),
Message: "Internal error generating a new SSH keypair",
Detail: err.Error(),
})
return
}
Expand All @@ -35,15 +35,17 @@ func (api *API) regenerateGitSSHKey(rw http.ResponseWriter, r *http.Request) {
})
if err != nil {
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
Message: fmt.Sprintf("update git SSH key: %s", err),
Message: "Internal error updating user's git SSH key",
Detail: err.Error(),
})
return
}

newKey, err := api.Database.GetGitSSHKey(r.Context(), user.ID)
if err != nil {
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
Message: fmt.Sprintf("get git SSH key: %s", err),
Message: "Internal error fetching user's git SSH key",
Detail: err.Error(),
})
return
}
Expand All @@ -67,7 +69,8 @@ func (api *API) gitSSHKey(rw http.ResponseWriter, r *http.Request) {
gitSSHKey, err := api.Database.GetGitSSHKey(r.Context(), user.ID)
if err != nil {
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
Message: fmt.Sprintf("update git SSH key: %s", err),
Message: "Internal error fetching user's SSH key",
Detail: err.Error(),
})
return
}
Expand All @@ -86,31 +89,35 @@ func (api *API) agentGitSSHKey(rw http.ResponseWriter, r *http.Request) {
resource, err := api.Database.GetWorkspaceResourceByID(r.Context(), agent.ResourceID)
if err != nil {
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
Message: fmt.Sprintf("getting workspace resources: %s", err),
Message: "Internal error fetching workspace resource",
Detail: err.Error(),
})
return
}

job, err := api.Database.GetWorkspaceBuildByJobID(r.Context(), resource.JobID)
if err != nil {
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
Message: fmt.Sprintf("getting workspace build: %s", err),
Message: "Internal error fetching workspace build",
Detail: err.Error(),
})
return
}

workspace, err := api.Database.GetWorkspaceByID(r.Context(), job.WorkspaceID)
if err != nil {
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
Message: fmt.Sprintf("getting workspace: %s", err),
Message: "Internal error fetching workspace",
Detail: err.Error(),
})
return
}

gitSSHKey, err := api.Database.GetGitSSHKey(r.Context(), workspace.OwnerID)
if err != nil {
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
Message: fmt.Sprintf("getting git SSH key: %s", err),
Message: "Internal error fetching git SSH key",
Detail: err.Error(),
})
return
}
Expand Down
30 changes: 23 additions & 7 deletions coderd/httpapi/httpapi.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,22 @@ func init() {

// Response represents a generic HTTP response.
type Response struct {
Message string `json:"message" validate:"required"`
Errors []Error `json:"errors,omitempty" validate:"required"`
// Message is an actionable message that depicts actions the request took.
// These messages should be fully formed sentences with proper punctuation.
// Examples:
// - "A user has been created."
// - "Failed to create a user."
Message string `json:"message"`
// Detail is a debug message that provides further insight into why the
// action failed. This information can be technical and a regular golang
// err.Error() text.
// - "database: too many open connections"
// - "stat: too many open files"
Detail string `json:"detail"`
// Validations are form field-specific friendly error messages. They will be
// shown on a form field in the UI. These can also be used to add additional
// context if there is a set of errors in the primary 'Message'.
Validations []Error `json:"errors,omitempty"`
}

// Error represents a scoped error to a user input.
Expand All @@ -64,7 +78,7 @@ type Error struct {

func Forbidden(rw http.ResponseWriter) {
Write(rw, http.StatusForbidden, Response{
Message: "forbidden",
Message: "Forbidden",
})
}

Expand Down Expand Up @@ -93,7 +107,8 @@ func Read(rw http.ResponseWriter, r *http.Request, value interface{}) bool {
err := json.NewDecoder(r.Body).Decode(value)
if err != nil {
Write(rw, http.StatusBadRequest, Response{
Message: fmt.Sprintf("read body: %s", err.Error()),
Message: "Request body must be valid JSON",
Detail: err.Error(),
})
return false
}
Expand All @@ -108,14 +123,15 @@ func Read(rw http.ResponseWriter, r *http.Request, value interface{}) bool {
})
}
Write(rw, http.StatusBadRequest, Response{
Message: "Validation failed",
Errors: apiErrors,
Message: "Validation failed",
Validations: apiErrors,
})
return false
}
if err != nil {
Write(rw, http.StatusInternalServerError, Response{
Message: fmt.Sprintf("validation: %s", err.Error()),
Message: "Internal error validating request body payload",
Detail: err.Error(),
})
return false
}
Expand Down
6 changes: 3 additions & 3 deletions coderd/httpapi/httpapi_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,9 @@ func TestRead(t *testing.T) {
var v httpapi.Response
err := json.NewDecoder(rw.Body).Decode(&v)
require.NoError(t, err)
require.Len(t, v.Errors, 1)
require.Equal(t, "value", v.Errors[0].Field)
require.Equal(t, "Validation failed for tag \"required\" with value: \"\"", v.Errors[0].Detail)
require.Len(t, v.Validations, 1)
require.Equal(t, "value", v.Validations[0].Field)
require.Equal(t, "Validation failed for tag \"required\" with value: \"\"", v.Validations[0].Detail)
})
}

Expand Down
29 changes: 16 additions & 13 deletions coderd/httpmw/apikey.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,15 +65,15 @@ func ExtractAPIKey(db database.Store, oauth *OAuth2Configs) func(http.Handler) h
}
if cookieValue == "" {
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
Message: fmt.Sprintf("%q cookie or query parameter must be provided", SessionTokenKey),
Message: fmt.Sprintf("Cookie %q or query parameter must be provided", SessionTokenKey),
})
return
}
parts := strings.Split(cookieValue, "-")
// APIKeys are formatted: ID-SECRET
if len(parts) != 2 {
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
Message: fmt.Sprintf("invalid %q cookie api key format", SessionTokenKey),
Message: fmt.Sprintf("Invalid %q cookie API key format", SessionTokenKey),
})
return
}
Expand All @@ -82,26 +82,27 @@ func ExtractAPIKey(db database.Store, oauth *OAuth2Configs) func(http.Handler) h
// Ensuring key lengths are valid.
if len(keyID) != 10 {
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
Message: fmt.Sprintf("invalid %q cookie api key id", SessionTokenKey),
Message: fmt.Sprintf("Invalid %q cookie API key id", SessionTokenKey),
})
return
}
if len(keySecret) != 22 {
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
Message: fmt.Sprintf("invalid %q cookie api key secret", SessionTokenKey),
Message: fmt.Sprintf("Invalid %q cookie API key secret", SessionTokenKey),
})
return
}
key, err := db.GetAPIKeyByID(r.Context(), keyID)
if err != nil {
if errors.Is(err, sql.ErrNoRows) {
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
Message: "api key is invalid",
Message: "API key is invalid",
})
return
}
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
Message: fmt.Sprintf("get api key by id: %s", err.Error()),
Message: "Internal error fetching API key by id",
Detail: err.Error(),
})
return
}
Expand All @@ -110,7 +111,7 @@ func ExtractAPIKey(db database.Store, oauth *OAuth2Configs) func(http.Handler) h
// Checking to see if the secret is valid.
if subtle.ConstantTimeCompare(key.HashedSecret, hashed[:]) != 1 {
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
Message: "api key secret is invalid",
Message: "API key secret is invalid",
})
return
}
Expand All @@ -127,7 +128,7 @@ func ExtractAPIKey(db database.Store, oauth *OAuth2Configs) func(http.Handler) h
oauthConfig = oauth.Github
default:
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
Message: fmt.Sprintf("unexpected authentication type %q", key.LoginType),
Message: fmt.Sprintf("Unexpected authentication type %q", key.LoginType),
})
return
}
Expand All @@ -139,7 +140,8 @@ func ExtractAPIKey(db database.Store, oauth *OAuth2Configs) func(http.Handler) h
}).Token()
if err != nil {
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
Message: fmt.Sprintf("couldn't refresh expired oauth token: %s", err.Error()),
Message: "Could not refresh expired Oauth token",
Detail: err.Error(),
})
return
}
Expand All @@ -154,7 +156,7 @@ func ExtractAPIKey(db database.Store, oauth *OAuth2Configs) func(http.Handler) h
// Checking if the key is expired.
if key.ExpiresAt.Before(now) {
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
Message: fmt.Sprintf("api key expired at %q", key.ExpiresAt.String()),
Message: fmt.Sprintf("API key expired at %q", key.ExpiresAt.String()),
})
return
}
Expand Down Expand Up @@ -182,7 +184,7 @@ func ExtractAPIKey(db database.Store, oauth *OAuth2Configs) func(http.Handler) h
})
if err != nil {
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
Message: fmt.Sprintf("api key couldn't update: %s", err.Error()),
Message: fmt.Sprintf("API key couldn't update: %s", err.Error()),
})
return
}
Expand All @@ -194,14 +196,15 @@ func ExtractAPIKey(db database.Store, oauth *OAuth2Configs) func(http.Handler) h
roles, err := db.GetAuthorizationUserRoles(r.Context(), key.UserID)
if err != nil {
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
Message: "roles not found",
Message: "Internal error fetching user's roles",
Detail: err.Error(),
})
return
}

if roles.Status != database.UserStatusActive {
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
Message: fmt.Sprintf("user is not active (status = %q), contact an admin to reactivate your account", roles.Status),
Message: fmt.Sprintf("User is not active (status = %q). Contact an admin to reactivate your account.", roles.Status),
})
return
}
Expand Down

0 comments on commit c9a4642

Please sign in to comment.