From 357a042937b2359f6c292ccb651384abe426b895 Mon Sep 17 00:00:00 2001 From: Nikolay Edigaryev Date: Mon, 27 Mar 2023 22:12:03 +0400 Subject: [PATCH] REST API: provide error messages in error responses (#66) * REST API: provide error messages in error responses * Fix role checking logic and add tests * Ignore testpackage linter error * Rename NewError() to NewErrorResponse() --- internal/controller/api.go | 28 ++++++++-- internal/controller/api_service_accounts.go | 41 +++++++------- internal/controller/api_test.go | 45 ++++++++++++++++ internal/controller/api_vms.go | 59 +++++++++++++-------- internal/controller/api_vms_portforward.go | 4 +- internal/controller/api_workers.go | 35 +++++++----- internal/controller/errorresponse.go | 13 +++++ 7 files changed, 163 insertions(+), 62 deletions(-) create mode 100644 internal/controller/api_test.go create mode 100644 internal/controller/errorresponse.go diff --git a/internal/controller/api.go b/internal/controller/api.go index 395e460..65d197f 100644 --- a/internal/controller/api.go +++ b/internal/controller/api.go @@ -14,6 +14,7 @@ import ( "github.com/go-openapi/runtime/middleware" "google.golang.org/grpc/metadata" "net/http" + "strings" ) const ctxServiceAccountKey = "service-account" @@ -163,19 +164,36 @@ func (controller *Controller) authenticateMiddleware(c *gin.Context) { c.Next() } -func (controller *Controller) authorize(ctx *gin.Context, scopes ...v1pkg.ServiceAccountRole) bool { +func (controller *Controller) authorize( + ctx *gin.Context, + requiredRoles ...v1pkg.ServiceAccountRole, +) responder.Responder { if controller.insecureAuthDisabled { - return true + return nil } serviceAccountUntyped, ok := ctx.Get(ctxServiceAccountKey) if !ok { - return false + return responder.Code(http.StatusUnauthorized) + } + serviceAccount := serviceAccountUntyped.(*v1pkg.ServiceAccount) + serviceAccountRolesSet := mapset.NewSet[v1pkg.ServiceAccountRole](serviceAccount.Roles...) + + requiredRolesSet := mapset.NewSet[v1pkg.ServiceAccountRole](requiredRoles...) + + missingRoles := requiredRolesSet.Difference(serviceAccountRolesSet).ToSlice() + if len(missingRoles) == 0 { + return nil } - serviceAccount := serviceAccountUntyped.(*v1pkg.ServiceAccount) + var missingRolesStrings []string - return mapset.NewSet[v1pkg.ServiceAccountRole](serviceAccount.Roles...).Contains(scopes...) + for _, missingRole := range missingRoles { + missingRolesStrings = append(missingRolesStrings, string(missingRole)) + } + + return responder.JSON(http.StatusUnauthorized, + NewErrorResponse("missing roles: %s", strings.Join(missingRolesStrings, ", "))) } func (controller *Controller) authorizeGRPC(ctx context.Context, scopes ...v1pkg.ServiceAccountRole) bool { diff --git a/internal/controller/api_service_accounts.go b/internal/controller/api_service_accounts.go index cdea23a..4718d00 100644 --- a/internal/controller/api_service_accounts.go +++ b/internal/controller/api_service_accounts.go @@ -12,25 +12,26 @@ import ( ) func (controller *Controller) createServiceAccount(ctx *gin.Context) responder.Responder { - if !controller.authorize(ctx, v1.ServiceAccountRoleAdminWrite) { - return responder.Code(http.StatusUnauthorized) + if responder := controller.authorize(ctx, v1.ServiceAccountRoleAdminWrite); responder != nil { + return responder } var serviceAccount v1.ServiceAccount if err := ctx.ShouldBindJSON(&serviceAccount); err != nil { - return responder.Code(http.StatusBadRequest) + return responder.JSON(http.StatusBadRequest, NewErrorResponse("invalid JSON was provided")) } if serviceAccount.Name == "" { - return responder.Code(http.StatusPreconditionFailed) + return responder.JSON(http.StatusPreconditionFailed, NewErrorResponse("service account name is empty")) } // validate roles for _, role := range serviceAccount.Roles { _, err := v1.NewServiceAccountRole(string(role)) if err != nil { - return responder.Code(http.StatusPreconditionFailed) + return responder.JSON(http.StatusPreconditionFailed, + NewErrorResponse("unsupported role \"%s\"", role)) } } @@ -43,8 +44,12 @@ func (controller *Controller) createServiceAccount(ctx *gin.Context) responder.R return controller.storeUpdate(func(txn storepkg.Transaction) responder.Responder { // Does the Service Account resource with this name already exists? _, err := txn.GetServiceAccount(serviceAccount.Name) - if !errors.Is(err, storepkg.ErrNotFound) { - return responder.Code(http.StatusConflict) + if err != nil && !errors.Is(err, storepkg.ErrNotFound) { + return responder.Code(http.StatusInternalServerError) + } + if err == nil { + return responder.JSON(http.StatusConflict, + NewErrorResponse("service account with this name already exists")) } if err := txn.SetServiceAccount(&serviceAccount); err != nil { @@ -56,22 +61,22 @@ func (controller *Controller) createServiceAccount(ctx *gin.Context) responder.R } func (controller *Controller) updateServiceAccount(ctx *gin.Context) responder.Responder { - if !controller.authorize(ctx, v1.ServiceAccountRoleAdminWrite) { - return responder.Code(http.StatusUnauthorized) + if responder := controller.authorize(ctx, v1.ServiceAccountRoleAdminWrite); responder != nil { + return responder } var userServiceAccount v1.ServiceAccount if err := ctx.ShouldBindJSON(&userServiceAccount); err != nil { - return responder.Code(http.StatusBadRequest) + return responder.JSON(http.StatusBadRequest, NewErrorResponse("invalid JSON was provided")) } if userServiceAccount.Name == "" { - return responder.Code(http.StatusPreconditionFailed) + return responder.JSON(http.StatusPreconditionFailed, NewErrorResponse("service account name is empty")) } if userServiceAccount.Token == "" { - return responder.Code(http.StatusPreconditionFailed) + return responder.JSON(http.StatusPreconditionFailed, NewErrorResponse("service account token is empty")) } return controller.storeUpdate(func(txn storepkg.Transaction) responder.Responder { @@ -89,8 +94,8 @@ func (controller *Controller) updateServiceAccount(ctx *gin.Context) responder.R } func (controller *Controller) getServiceAccount(ctx *gin.Context) responder.Responder { - if !controller.authorize(ctx, v1.ServiceAccountRoleAdminRead) { - return responder.Code(http.StatusUnauthorized) + if responder := controller.authorize(ctx, v1.ServiceAccountRoleAdminRead); responder != nil { + return responder } name := ctx.Param("name") @@ -106,8 +111,8 @@ func (controller *Controller) getServiceAccount(ctx *gin.Context) responder.Resp } func (controller *Controller) listServiceAccounts(ctx *gin.Context) responder.Responder { - if !controller.authorize(ctx, v1.ServiceAccountRoleAdminRead) { - return responder.Code(http.StatusUnauthorized) + if responder := controller.authorize(ctx, v1.ServiceAccountRoleAdminRead); responder != nil { + return responder } return controller.storeView(func(txn storepkg.Transaction) responder.Responder { @@ -121,8 +126,8 @@ func (controller *Controller) listServiceAccounts(ctx *gin.Context) responder.Re } func (controller *Controller) deleteServiceAccount(ctx *gin.Context) responder.Responder { - if !controller.authorize(ctx, v1.ServiceAccountRoleAdminWrite) { - return responder.Code(http.StatusUnauthorized) + if responder := controller.authorize(ctx, v1.ServiceAccountRoleAdminWrite); responder != nil { + return responder } name := ctx.Param("name") diff --git a/internal/controller/api_test.go b/internal/controller/api_test.go new file mode 100644 index 0000000..c4dd621 --- /dev/null +++ b/internal/controller/api_test.go @@ -0,0 +1,45 @@ +//nolint:testpackage // we need to have access for Controller for this test +package controller + +import ( + "github.com/cirruslabs/orchard/internal/responder" + v1pkg "github.com/cirruslabs/orchard/pkg/resource/v1" + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/require" + "net/http" + "testing" +) + +func TestAuthorizeInsecureAuthDisabled(t *testing.T) { + ctx := &gin.Context{} + controller := Controller{insecureAuthDisabled: true} + + require.Nil(t, controller.authorize(ctx, v1pkg.ServiceAccountRoleAdminWrite)) +} + +func TestAuthorizeUnauthenticated(t *testing.T) { + ctx := &gin.Context{} + controller := Controller{} + + require.Equal(t, responder.Code(http.StatusUnauthorized), controller.authorize(ctx)) +} + +func TestAuthorizeAuthenticatedNoRoles(t *testing.T) { + ctx := &gin.Context{} + ctx.Set(ctxServiceAccountKey, &v1pkg.ServiceAccount{}) + controller := Controller{} + + const requiredRole = v1pkg.ServiceAccountRoleAdminWrite + + require.Equal(t, responder.JSON(http.StatusUnauthorized, NewErrorResponse("missing roles: %s", requiredRole)), + controller.authorize(ctx, requiredRole)) +} + +func TestAuthorizeAuthenticatedHasRoles(t *testing.T) { + ctx := &gin.Context{} + const requiredRole = v1pkg.ServiceAccountRoleAdminWrite + ctx.Set(ctxServiceAccountKey, &v1pkg.ServiceAccount{Roles: []v1pkg.ServiceAccountRole{requiredRole}}) + controller := Controller{} + + require.Nil(t, controller.authorize(ctx, requiredRole)) +} diff --git a/internal/controller/api_vms.go b/internal/controller/api_vms.go index b589555..753d00f 100644 --- a/internal/controller/api_vms.go +++ b/internal/controller/api_vms.go @@ -12,18 +12,27 @@ import ( ) func (controller *Controller) createVM(ctx *gin.Context) responder.Responder { - if !controller.authorize(ctx, v1.ServiceAccountRoleComputeWrite) { - return responder.Code(http.StatusUnauthorized) + if responder := controller.authorize(ctx, v1.ServiceAccountRoleComputeWrite); responder != nil { + return responder } var vm v1.VM if err := ctx.ShouldBindJSON(&vm); err != nil { - return responder.Code(http.StatusBadRequest) + return responder.JSON(http.StatusBadRequest, NewErrorResponse("invalid JSON was provided")) } - if vm.Name == "" || vm.Image == "" || vm.CPU == 0 || vm.Memory == 0 { - return responder.Code(http.StatusPreconditionFailed) + if vm.Name == "" { + return responder.JSON(http.StatusPreconditionFailed, NewErrorResponse("VM name is empty")) + } + if vm.Image == "" { + return responder.JSON(http.StatusPreconditionFailed, NewErrorResponse("VM image is empty")) + } + if vm.CPU == 0 { + return responder.JSON(http.StatusPreconditionFailed, NewErrorResponse("VM CPU is zero")) + } + if vm.Memory == 0 { + return responder.JSON(http.StatusPreconditionFailed, NewErrorResponse("VM memory is zero")) } vm.Status = v1.VMStatusPending @@ -41,8 +50,11 @@ func (controller *Controller) createVM(ctx *gin.Context) responder.Responder { return controller.storeUpdate(func(txn storepkg.Transaction) responder.Responder { // Does the VM resource with this name already exists? _, err := txn.GetVM(vm.Name) - if !errors.Is(err, storepkg.ErrNotFound) { - return responder.Code(http.StatusConflict) + if err != nil && !errors.Is(err, storepkg.ErrNotFound) { + return responder.Code(http.StatusInternalServerError) + } + if err == nil { + return responder.JSON(http.StatusConflict, NewErrorResponse("VM with this name already exists")) } if err := txn.SetVM(vm); err != nil { @@ -54,18 +66,18 @@ func (controller *Controller) createVM(ctx *gin.Context) responder.Responder { } func (controller *Controller) updateVM(ctx *gin.Context) responder.Responder { - if !controller.authorize(ctx, v1.ServiceAccountRoleComputeWrite) { - return responder.Code(http.StatusUnauthorized) + if responder := controller.authorize(ctx, v1.ServiceAccountRoleComputeWrite); responder != nil { + return responder } var userVM v1.VM if err := ctx.ShouldBindJSON(&userVM); err != nil { - return responder.Code(http.StatusBadRequest) + return responder.JSON(http.StatusBadRequest, NewErrorResponse("invalid JSON was provided")) } if userVM.Name == "" { - return responder.Code(http.StatusPreconditionFailed) + return responder.JSON(http.StatusPreconditionFailed, NewErrorResponse("VM name is empty")) } return controller.storeUpdate(func(txn storepkg.Transaction) responder.Responder { @@ -75,7 +87,8 @@ func (controller *Controller) updateVM(ctx *gin.Context) responder.Responder { } if dbVM.TerminalState() && dbVM.Status != userVM.Status { - return responder.Code(http.StatusPreconditionFailed) + return responder.JSON(http.StatusPreconditionFailed, + NewErrorResponse("cannot update status for a VM in a terminal state")) } dbVM.Status = userVM.Status @@ -90,8 +103,8 @@ func (controller *Controller) updateVM(ctx *gin.Context) responder.Responder { } func (controller *Controller) getVM(ctx *gin.Context) responder.Responder { - if !controller.authorize(ctx, v1.ServiceAccountRoleComputeRead) { - return responder.Code(http.StatusUnauthorized) + if responder := controller.authorize(ctx, v1.ServiceAccountRoleComputeRead); responder != nil { + return responder } name := ctx.Param("name") @@ -107,8 +120,8 @@ func (controller *Controller) getVM(ctx *gin.Context) responder.Responder { } func (controller *Controller) listVMs(ctx *gin.Context) responder.Responder { - if !controller.authorize(ctx, v1.ServiceAccountRoleComputeRead) { - return responder.Code(http.StatusUnauthorized) + if responder := controller.authorize(ctx, v1.ServiceAccountRoleComputeRead); responder != nil { + return responder } return controller.storeView(func(txn storepkg.Transaction) responder.Responder { @@ -122,8 +135,8 @@ func (controller *Controller) listVMs(ctx *gin.Context) responder.Responder { } func (controller *Controller) deleteVM(ctx *gin.Context) responder.Responder { - if !controller.authorize(ctx, v1.ServiceAccountRoleComputeWrite) { - return responder.Code(http.StatusUnauthorized) + if responder := controller.authorize(ctx, v1.ServiceAccountRoleComputeWrite); responder != nil { + return responder } name := ctx.Param("name") @@ -146,14 +159,14 @@ func (controller *Controller) deleteVM(ctx *gin.Context) responder.Responder { }) } func (controller *Controller) appendVMEvents(ctx *gin.Context) responder.Responder { - if !controller.authorize(ctx, v1.ServiceAccountRoleComputeWrite) { - return responder.Code(http.StatusUnauthorized) + if responder := controller.authorize(ctx, v1.ServiceAccountRoleComputeWrite); responder != nil { + return responder } var events []v1.Event if err := ctx.ShouldBindJSON(&events); err != nil { - return responder.Code(http.StatusBadRequest) + return responder.JSON(http.StatusBadRequest, NewErrorResponse("invalid JSON was provided")) } name := ctx.Param("name") @@ -172,8 +185,8 @@ func (controller *Controller) appendVMEvents(ctx *gin.Context) responder.Respond } func (controller *Controller) listVMEvents(ctx *gin.Context) responder.Responder { - if !controller.authorize(ctx, v1.ServiceAccountRoleComputeRead) { - return responder.Code(http.StatusUnauthorized) + if responder := controller.authorize(ctx, v1.ServiceAccountRoleComputeRead); responder != nil { + return responder } name := ctx.Param("name") diff --git a/internal/controller/api_vms_portforward.go b/internal/controller/api_vms_portforward.go index 03b23e8..94e9142 100644 --- a/internal/controller/api_vms_portforward.go +++ b/internal/controller/api_vms_portforward.go @@ -14,8 +14,8 @@ import ( ) func (controller *Controller) portForwardVM(ctx *gin.Context) responder.Responder { - if !controller.authorize(ctx, v1.ServiceAccountRoleComputeWrite) { - return responder.Code(http.StatusUnauthorized) + if responder := controller.authorize(ctx, v1.ServiceAccountRoleComputeWrite); responder != nil { + return responder } // Retrieve and parse path and query parameters diff --git a/internal/controller/api_workers.go b/internal/controller/api_workers.go index df0b7c7..a0cbd6d 100644 --- a/internal/controller/api_workers.go +++ b/internal/controller/api_workers.go @@ -11,18 +11,19 @@ import ( ) func (controller *Controller) createWorker(ctx *gin.Context) responder.Responder { - if !controller.authorize(ctx, v1.ServiceAccountRoleComputeWrite, v1.ServiceAccountRoleWorker) { - return responder.Code(http.StatusUnauthorized) + if responder := controller.authorize(ctx, v1.ServiceAccountRoleComputeWrite, + v1.ServiceAccountRoleWorker); responder != nil { + return responder } var worker v1.Worker if err := ctx.ShouldBindJSON(&worker); err != nil { - return responder.Code(http.StatusBadRequest) + return responder.JSON(http.StatusBadRequest, NewErrorResponse("invalid JSON was provided")) } if worker.Name == "" { - return responder.Code(http.StatusPreconditionFailed) + return responder.JSON(http.StatusPreconditionFailed, NewErrorResponse("worker name is empty")) } currentTime := time.Now() @@ -40,7 +41,9 @@ func (controller *Controller) createWorker(ctx *gin.Context) responder.Responder return responder.Code(http.StatusInternalServerError) } if err == nil && worker.MachineID != dbWorker.MachineID { - return responder.Code(http.StatusConflict) + return responder.JSON(http.StatusConflict, + NewErrorResponse("this worker is managed from a different machine ID, "+ + "delete this worker first to be able to re-create it")) } if err := txn.SetWorker(worker); err != nil { @@ -52,14 +55,15 @@ func (controller *Controller) createWorker(ctx *gin.Context) responder.Responder } func (controller *Controller) updateWorker(ctx *gin.Context) responder.Responder { - if !controller.authorize(ctx, v1.ServiceAccountRoleComputeWrite, v1.ServiceAccountRoleWorker) { - return responder.Code(http.StatusUnauthorized) + if responder := controller.authorize(ctx, v1.ServiceAccountRoleComputeWrite, + v1.ServiceAccountRoleWorker); responder != nil { + return responder } var userWorker v1.Worker if err := ctx.ShouldBindJSON(&userWorker); err != nil { - return responder.Code(http.StatusBadRequest) + return responder.JSON(http.StatusBadRequest, NewErrorResponse("invalid JSON was provided")) } return controller.storeUpdate(func(txn storepkg.Transaction) responder.Responder { @@ -79,8 +83,9 @@ func (controller *Controller) updateWorker(ctx *gin.Context) responder.Responder } func (controller *Controller) getWorker(ctx *gin.Context) responder.Responder { - if !controller.authorize(ctx, v1.ServiceAccountRoleComputeRead, v1.ServiceAccountRoleWorker) { - return responder.Code(http.StatusUnauthorized) + if responder := controller.authorize(ctx, v1.ServiceAccountRoleComputeRead, + v1.ServiceAccountRoleWorker); responder != nil { + return responder } name := ctx.Param("name") @@ -96,8 +101,9 @@ func (controller *Controller) getWorker(ctx *gin.Context) responder.Responder { } func (controller *Controller) listWorkers(ctx *gin.Context) responder.Responder { - if !controller.authorize(ctx, v1.ServiceAccountRoleComputeRead, v1.ServiceAccountRoleWorker) { - return responder.Code(http.StatusUnauthorized) + if responder := controller.authorize(ctx, v1.ServiceAccountRoleComputeRead, + v1.ServiceAccountRoleWorker); responder != nil { + return responder } return controller.storeView(func(txn storepkg.Transaction) responder.Responder { @@ -111,8 +117,9 @@ func (controller *Controller) listWorkers(ctx *gin.Context) responder.Responder } func (controller *Controller) deleteWorker(ctx *gin.Context) responder.Responder { - if !controller.authorize(ctx, v1.ServiceAccountRoleComputeWrite, v1.ServiceAccountRoleWorker) { - return responder.Code(http.StatusUnauthorized) + if responder := controller.authorize(ctx, v1.ServiceAccountRoleComputeWrite, + v1.ServiceAccountRoleWorker); responder != nil { + return responder } name := ctx.Param("name") diff --git a/internal/controller/errorresponse.go b/internal/controller/errorresponse.go new file mode 100644 index 0000000..0a3e124 --- /dev/null +++ b/internal/controller/errorresponse.go @@ -0,0 +1,13 @@ +package controller + +import "fmt" + +type Error struct { + Message string `json:"message"` +} + +func NewErrorResponse(format string, args ...any) *Error { + return &Error{ + Message: fmt.Sprintf(format, args...), + } +}