diff --git a/core/errors/errors.go b/core/errors/errors.go index e774d293..be2e99c0 100644 --- a/core/errors/errors.go +++ b/core/errors/errors.go @@ -27,19 +27,29 @@ var ( ErrNoSessionToSignOut = errors.New("no session to sign out: provide session_id or sign_out_all") ) +func StatusFor(err error) (int, bool) { + switch { + case errors.Is(err, ErrUnauthorized): + return http.StatusUnauthorized, true + case errors.Is(err, ErrForbidden), errors.Is(err, ErrInsufficientPermissions): + return http.StatusForbidden, true + case errors.Is(err, ErrNotFound): + return http.StatusNotFound, true + case errors.Is(err, ErrConflict): + return http.StatusConflict, true + case errors.Is(err, ErrUnprocessableEntity): + return http.StatusUnprocessableEntity, true + case errors.Is(err, ErrBadRequest): + return http.StatusBadRequest, true + default: + return 0, false + } +} + func HandleError(err error, reqCtx *models.RequestContext) { status := http.StatusBadRequest - switch err { - case ErrUnauthorized: - status = http.StatusUnauthorized - case ErrForbidden, ErrInsufficientPermissions: - status = http.StatusForbidden - case ErrNotFound: - status = http.StatusNotFound - case ErrConflict: - status = http.StatusConflict - case ErrUnprocessableEntity: - status = http.StatusUnprocessableEntity + if mapped, ok := StatusFor(err); ok { + status = mapped } reqCtx.SetJSONResponse(status, map[string]any{"message": err.Error()}) reqCtx.Handled = true diff --git a/core/errors/errors_test.go b/core/errors/errors_test.go new file mode 100644 index 00000000..130a30cc --- /dev/null +++ b/core/errors/errors_test.go @@ -0,0 +1,80 @@ +package errors_test + +import ( + "errors" + "fmt" + "net/http" + "testing" + + "github.com/stretchr/testify/require" + + coreerrors "github.com/Authula/authula/core/errors" + "github.com/Authula/authula/models" +) + +func TestStatusFor(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + err error + expectedCode int + expectedOK bool + }{ + {name: "unauthorized", err: coreerrors.ErrUnauthorized, expectedCode: http.StatusUnauthorized, expectedOK: true}, + {name: "forbidden", err: coreerrors.ErrForbidden, expectedCode: http.StatusForbidden, expectedOK: true}, + {name: "insufficient permissions maps to forbidden", err: coreerrors.ErrInsufficientPermissions, expectedCode: http.StatusForbidden, expectedOK: true}, + {name: "not found", err: coreerrors.ErrNotFound, expectedCode: http.StatusNotFound, expectedOK: true}, + {name: "conflict", err: coreerrors.ErrConflict, expectedCode: http.StatusConflict, expectedOK: true}, + {name: "bad request", err: coreerrors.ErrBadRequest, expectedCode: http.StatusBadRequest, expectedOK: true}, + {name: "unprocessable entity", err: coreerrors.ErrUnprocessableEntity, expectedCode: http.StatusUnprocessableEntity, expectedOK: true}, + {name: "wrapped sentinel is recognized", err: fmt.Errorf("authorizing: %w", coreerrors.ErrInsufficientPermissions), expectedCode: http.StatusForbidden, expectedOK: true}, + {name: "unknown error is not recognized", err: errors.New("boom"), expectedOK: false}, + {name: "nil is not recognized", err: nil, expectedOK: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + status, ok := coreerrors.StatusFor(tt.err) + + require.Equal(t, tt.expectedOK, ok) + require.Equal(t, tt.expectedCode, status) + }) + } +} + +func TestHandleError(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + err error + expectedStatus int + expectedMessage string + }{ + {name: "insufficient permissions is forbidden", err: coreerrors.ErrInsufficientPermissions, expectedStatus: http.StatusForbidden, expectedMessage: "insufficient permissions"}, + {name: "forbidden is forbidden", err: coreerrors.ErrForbidden, expectedStatus: http.StatusForbidden, expectedMessage: "forbidden"}, + {name: "unauthorized is unauthorized", err: coreerrors.ErrUnauthorized, expectedStatus: http.StatusUnauthorized, expectedMessage: "unauthorized"}, + {name: "not found", err: coreerrors.ErrNotFound, expectedStatus: http.StatusNotFound, expectedMessage: "not found"}, + {name: "conflict", err: coreerrors.ErrConflict, expectedStatus: http.StatusConflict, expectedMessage: "conflict"}, + {name: "unprocessable entity", err: coreerrors.ErrUnprocessableEntity, expectedStatus: http.StatusUnprocessableEntity, expectedMessage: "unprocessable entity"}, + {name: "unknown error defaults to bad request", err: errors.New("boom"), expectedStatus: http.StatusBadRequest, expectedMessage: "boom"}, + {name: "wrapped sentinel keeps its status", err: fmt.Errorf("authorizing: %w", coreerrors.ErrInsufficientPermissions), expectedStatus: http.StatusForbidden, expectedMessage: "authorizing: insufficient permissions"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + reqCtx := &models.RequestContext{} + + coreerrors.HandleError(tt.err, reqCtx) + + require.True(t, reqCtx.Handled) + require.Equal(t, tt.expectedStatus, reqCtx.ResponseStatus) + require.JSONEq(t, fmt.Sprintf(`{"message":%q}`, tt.expectedMessage), string(reqCtx.ResponseBody)) + }) + } +} diff --git a/plugins/access-control/handlers/error_helpers.go b/plugins/access-control/handlers/error_helpers.go index a16cbb57..2f72f1e5 100644 --- a/plugins/access-control/handlers/error_helpers.go +++ b/plugins/access-control/handlers/error_helpers.go @@ -9,26 +9,13 @@ import ( ) func mapHttpErrorStatus(err error) int { - if err == nil { - return http.StatusInternalServerError + if status, ok := coreerrors.StatusFor(err); ok { + return status } - - switch { - case errors.Is(err, coreerrors.ErrUnauthorized): - return http.StatusUnauthorized - case errors.Is(err, coreerrors.ErrForbidden), errors.Is(err, accesscontrolconstants.ErrCannotUpdateSystemRole): + if errors.Is(err, accesscontrolconstants.ErrCannotUpdateSystemRole) { return http.StatusForbidden - case errors.Is(err, coreerrors.ErrNotFound): - return http.StatusNotFound - case errors.Is(err, coreerrors.ErrConflict): - return http.StatusConflict - case errors.Is(err, coreerrors.ErrBadRequest): - return http.StatusBadRequest - case errors.Is(err, coreerrors.ErrUnprocessableEntity): - return http.StatusUnprocessableEntity - default: - return http.StatusInternalServerError } + return http.StatusInternalServerError } func mapHttpErrorMessage(err error) string { @@ -39,7 +26,7 @@ func mapHttpErrorMessage(err error) string { switch { case errors.Is(err, coreerrors.ErrUnauthorized): return "unauthorized" - case errors.Is(err, coreerrors.ErrForbidden): + case errors.Is(err, coreerrors.ErrForbidden), errors.Is(err, coreerrors.ErrInsufficientPermissions): return "forbidden" case errors.Is(err, coreerrors.ErrNotFound): return "not found" diff --git a/plugins/access-control/handlers/role_handlers_test.go b/plugins/access-control/handlers/role_handlers_test.go index 4116bf58..8a856fc2 100644 --- a/plugins/access-control/handlers/role_handlers_test.go +++ b/plugins/access-control/handlers/role_handlers_test.go @@ -144,6 +144,14 @@ func TestGetAllRolesHandler(t *testing.T) { expectedStatus: http.StatusForbidden, expectedBody: map[string]string{"message": "forbidden"}, }, + { + name: "insufficient permissions is forbidden, not internal server error", + setupMock: func(m *accesscontroltests.MockRolesRepository) { + m.On("GetAllRoles", mock.Anything).Return(([]types.Role)(nil), coreerrors.ErrInsufficientPermissions).Once() + }, + expectedStatus: http.StatusForbidden, + expectedBody: map[string]string{"message": "forbidden"}, + }, { name: "success", setupMock: func(m *accesscontroltests.MockRolesRepository) { diff --git a/plugins/admin/handlers/error_helpers.go b/plugins/admin/handlers/error_helpers.go index 4807599d..80860ede 100644 --- a/plugins/admin/handlers/error_helpers.go +++ b/plugins/admin/handlers/error_helpers.go @@ -8,24 +8,10 @@ import ( ) func mapAdminHttpErrorStatus(err error) int { - if err == nil { - return http.StatusInternalServerError - } - - switch { - case errors.Is(err, coreerrors.ErrBadRequest): - return http.StatusBadRequest - case errors.Is(err, coreerrors.ErrUnauthorized): - return http.StatusUnauthorized - case errors.Is(err, coreerrors.ErrForbidden): - return http.StatusForbidden - case errors.Is(err, coreerrors.ErrNotFound): - return http.StatusNotFound - case errors.Is(err, coreerrors.ErrConflict): - return http.StatusConflict - default: - return http.StatusInternalServerError + if status, ok := coreerrors.StatusFor(err); ok { + return status } + return http.StatusInternalServerError } func mapAdminHttpErrorMessage(err error) string { @@ -38,12 +24,14 @@ func mapAdminHttpErrorMessage(err error) string { return "bad request" case errors.Is(err, coreerrors.ErrUnauthorized): return "unauthorized" - case errors.Is(err, coreerrors.ErrForbidden): + case errors.Is(err, coreerrors.ErrForbidden), errors.Is(err, coreerrors.ErrInsufficientPermissions): return "forbidden" case errors.Is(err, coreerrors.ErrNotFound): return "not found" case errors.Is(err, coreerrors.ErrConflict): return "conflict" + case errors.Is(err, coreerrors.ErrUnprocessableEntity): + return "unprocessable entity" default: return err.Error() } diff --git a/plugins/admin/handlers/users_handlers_test.go b/plugins/admin/handlers/users_handlers_test.go index 0462003f..30bbb275 100644 --- a/plugins/admin/handlers/users_handlers_test.go +++ b/plugins/admin/handlers/users_handlers_test.go @@ -117,6 +117,15 @@ func TestGetAllUsersHandler(t *testing.T) { expectedStatus: http.StatusForbidden, expectedMessage: "forbidden", }, + { + name: "insufficient permissions is forbidden, not internal server error", + setup: func(repo *internaltests.MockUserRepository) { + repo.On("GetAll", mock.Anything, (*string)(nil), 10).Return(([]models.User)(nil), (*string)(nil), coreerrors.ErrInsufficientPermissions).Once() + }, + path: "/admin/users", + expectedStatus: http.StatusForbidden, + expectedMessage: "forbidden", + }, { name: "success with cursor and limit", setup: func(repo *internaltests.MockUserRepository) {