8000 chore: More UI friendly errors by Emyrk · Pull Request #1994 · coder/coder · GitHub
[go: up one dir, main page]
Skip to content

chore: More UI friendly errors #1994

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 23 commits into from
Jun 3, 2022
Merged

chore: More UI friendly errors #1994

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
352b86a
chore: More UI friendly errors
Emyrk Jun 2, 2022
7f9b66a
chore: Update BE http errors to be ui friendly
Emyrk Jun 3, 2022
46b52d5
chore: More friendly errors as responses
Emyrk Jun 3, 2022
1592d08
Fix typo
Emyrk Jun 3, 2022
6b08a08
Add internal error to the cli
Emyrk Jun 3, 2022
1b3947f
Merge remote-tracking branch 'origin/main' into stevenmasley/ui_errors
Emyrk Jun 3, 2022
deb5cf1
Remove empty switch case
Emyrk Jun 3, 2022
5e34b5f
Update coderd/gitsshkey.go
Emyrk Jun 3, 2022
f355dff
Update coderd/gitsshkey.go
Emyrk Jun 3, 2022
6566e83
Update coderd/gitsshkey.go
Emyrk Jun 3, 2022
fed7b7f
Update coderd/gitsshkey.go
Emyrk Jun 3, 2022
5c4ec92
Update coderd/gitsshkey.go
Emyrk Jun 3, 2022
e373a06
Update coderd/httpapi/httpapi.go 8000
Emyrk Jun 3, 2022
73ffa22
Rename "Internal" -> "Detail"
Emyrk Jun 3, 2022
5d59312
Rename "Errors" -> "Validations"
Emyrk Jun 3, 2022
a8e78bb
Fix proper captalization
Emyrk Jun 3, 2022
ee2f547
Fix typo
Emyrk Jun 3, 2022
4a5d03a
Fix accidental 'detail'
Emyrk Jun 3, 2022
8259e51
Merge remote-tracking branch 'origin/main' into stevenmasley/ui_errors
Emyrk Jun 3, 2022
819f2e1
PR comments addressed
Emyrk Jun 3, 2022
1116f94
Linting changes
Emyrk Jun 3, 2022
102a01f
Fix test error asserts
Emyrk Jun 3, 2022
af169f0
Fix test error asserts
Emyrk Jun 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions coderd/authorize.go
View file
Open in desktop
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: what allowed us to make this change? Does it mean that we no longer send those random Message errors on forbidden routes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vapurrmaid for security reasons all forbidden messages should be identical. If they were different, then the different errors allow the end user to gain information.

There is a line of "security" vs "usability" that we will need to decide on these endpoints. As it's unhelpful by design.


// Log the errors for debugging
internalError := new(rbac.UnauthorizedError)
Expand Down
3 changes: 2 additions & 1 deletion coderd/csp.go
View file
Open in desktop
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",
Internal: err.Error(),
})
return
}
Expand Down
13 changes: 8 additions & 5 deletions coderd/files.go
View file
Open in desktop
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",
Internal: 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",
Internal: 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: fmt.Sprintf("Internal error fetching file"),
Internal: err.Error(),
})
return
}
Expand Down
25 changes: 16 additions & 9 deletions coderd/gitsshkey.go
View file
Open in desktop
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",
Internal: 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",
Internal: 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",
Internal: 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",
Internal: 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",
Internal: 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",
Internal: 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",
Internal: 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",
Internal: err.Error(),
})
return
}
Expand Down
23 changes: 18 additions & 5 deletions coderd/httpapi/httpapi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,19 @@ 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 for general user-friendly error messages. This message will
// be shown at the top/bottom of a form, or in a toast on the UI.
Message string `json:"message"`
// Internal has the technical error information (err.Error()). These details
// might come from external packages and might not be user friendly.
// Do not populate this error field with any sensitive information or
// any errors that may be a security implication. These details are still
// available to more technical users.
Internal string `json:"internal"`
// Errors 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'.
Errors []Error `json:"errors,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will be difficult to understand at a glance how I'd display an error to the user.

Here's an alternative structure that just tweaks the names a bit and adds examples, but could help:

type Response struct {
  // An actionable message that depicts actions the request took. Examples:
  // - "A user has been created."
  // - "Failed to create a user."
  Message string `json:"message"`
  // A debug message that provides further insight into why the action failed.
  // - "database: too many open connections"
  // - "stat: too many open files"
  Detail string `json:"detail"`
}

I don't know what to name Errors. since it's form-specific I'd lean towards Validations maybe?

I'm curious for your take @presleyp, you're a magician with words! 🪄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kira-Pilot suggested Verbose instead of Internal and I like that; Detail works too.

I like Validations better than Errors - more specific! Right now I feel like it requires an explanatory comment on the frontend.

But I know @Emyrk doesn't want this PR to be open for long, maybe these tweaks can be a followup.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change that now, that's easy with IDE tools

}

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

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

Expand Down Expand Up @@ -93,7 +104,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",
Internal: err.Error(),
})
return false
}
Expand All @@ -115,7 +127,8 @@ func Read(rw http.ResponseWriter, r *http.Request, value interface{}) bool {
}
if err != nil {
Write(rw, http.StatusInternalServerError, Response{
Message: fmt.Sprintf("validation: %s", err.Error()),
Message: "Internal error validating request body payload",
Internal: err.Error(),
})
return false
}
Expand Down
29 changes: 16 additions & 13 deletions coderd/httpmw/apikey.go
View file
Open in desktop
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",
Internal: 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",
Internal: 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",
Internal: 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
7 changes: 5 additions & 2 deletions coderd/httpmw/httpmw.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,18 @@ func parseUUID(rw http.ResponseWriter, r *http.Request, param string) (uuid.UUID
rawID := chi.URLParam(r, param)
if rawID == "" {
httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{
Message: fmt.Sprintf("%q must be provided", param),
Message: "Missing uuid in url",
// Url params mean nothing to a user
Internal: fmt.Sprintf("%q url param missing", param),
})
return uuid.UUID{}, false
}

parsed, err := uuid.Parse(rawID)
if err != nil {
httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{
Message: fmt.Sprintf("%q must be a uuid", param),
Message: fmt.Sprintf("Invalid uuid %q", param),
Internal: err.Error(),
})
return uuid.UUID{}, false
}
Expand Down
12 changes: 7 additions & 5 deletions coderd/httpmw/oauth2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ func ExtractOAuth2(config OAuth2Config) func(http.Handler) http.Handler {
state, err := cryptorand.String(32)
if err != nil {
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
Message: fmt.Sprintf("generate state string: %s", err),
Message: "Internal error generating state string",
Internal: err.Error(),
})
return
}
Expand Down Expand Up @@ -91,21 +92,21 @@ func ExtractOAuth2(config OAuth2Config) func(http.Handler) http.Handler {

if state == "" {
httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{
Message: "state must be provided",
Message: "State must be provided",
})
return
}

stateCookie, err := r.Cookie(oauth2StateCookieName)
if err != nil {
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
Message: fmt.Sprintf("%q cookie must be provided", oauth2StateCookieName),
Message: fmt.Sprintf("Cookie %q must be provided", oauth2StateCookieName),
})
return
}
if stateCookie.Value != state {
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
Message: "state mismatched",
Message: "State mismatched",
})
return
}
Expand All @@ -119,7 +120,8 @@ func ExtractOAuth2(config OAuth2Config) func(http.Handler) http.Handler {
oauthToken, err := config.Exchange(r.Context(), code)
if err != nil {
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
Message: fmt.Sprintf("exchange oauth code: %s", err),
Message: "Internal error exchanging oauth code",
Internal: err.Error(),
})
return
}
Expand Down
Loading
0