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

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

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

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

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

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

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

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