fix: map ErrInsufficientPermissions to 403 in access-control

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

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))
+		})
+	}
+}

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"

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) {

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()
 	}

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) {