From 985330237f0936d2e336376fa5e0154c9263cca3 Mon Sep 17 00:00:00 2001 From: jonaustin09 Date: Mon, 17 Jun 2024 16:50:05 -0400 Subject: [PATCH] fix: Fixed admin api error response statuses --- s3api/controllers/admin.go | 30 +++++++++++++++++++----------- s3api/controllers/admin_test.go | 18 +++++++++--------- 2 files changed, 28 insertions(+), 20 deletions(-) diff --git a/s3api/controllers/admin.go b/s3api/controllers/admin.go index 3d6745c..085b014 100644 --- a/s3api/controllers/admin.go +++ b/s3api/controllers/admin.go @@ -17,6 +17,7 @@ package controllers import ( "encoding/json" "fmt" + "strings" "github.com/gofiber/fiber/v2" "github.com/versity/versitygw/auth" @@ -35,31 +36,38 @@ func NewAdminController(iam auth.IAMService, be backend.Backend) AdminController func (c AdminController) CreateUser(ctx *fiber.Ctx) error { acct := ctx.Locals("account").(auth.Account) if acct.Role != "admin" { - return fmt.Errorf("access denied: only admin users have access to this resource") + return ctx.Status(fiber.StatusForbidden).SendString("access denied: only admin users have access to this resource") } var usr auth.Account err := json.Unmarshal(ctx.Body(), &usr) if err != nil { - return fmt.Errorf("failed to parse request body: %w", err) + return ctx.Status(fiber.StatusBadRequest).SendString(fmt.Errorf("failed to parse request body: %w", err).Error()) } if usr.Role != auth.RoleAdmin && usr.Role != auth.RoleUser && usr.Role != auth.RoleUserPlus { - return fmt.Errorf("invalid parameters: user role have to be one of the following: 'user', 'admin', 'userplus'") + return ctx.Status(fiber.StatusBadRequest).SendString("invalid parameters: user role have to be one of the following: 'user', 'admin', 'userplus'") } err = c.iam.CreateAccount(usr) if err != nil { - return fmt.Errorf("failed to create user: %w", err) + status := fiber.StatusInternalServerError + msg := fmt.Errorf("failed to create user: %w", err).Error() + + if strings.Contains(msg, "user already exists") { + status = fiber.StatusConflict + } + + return ctx.Status(status).SendString(msg) } - return ctx.SendString("The user has been created successfully") + return ctx.Status(fiber.StatusCreated).SendString("The user has been created successfully") } func (c AdminController) DeleteUser(ctx *fiber.Ctx) error { access := ctx.Query("access") acct := ctx.Locals("account").(auth.Account) if acct.Role != "admin" { - return fmt.Errorf("access denied: only admin users have access to this resource") + return ctx.Status(fiber.StatusForbidden).SendString("access denied: only admin users have access to this resource") } err := c.iam.DeleteUserAccount(access) @@ -73,7 +81,7 @@ func (c AdminController) DeleteUser(ctx *fiber.Ctx) error { func (c AdminController) ListUsers(ctx *fiber.Ctx) error { acct := ctx.Locals("account").(auth.Account) if acct.Role != "admin" { - return fmt.Errorf("access denied: only admin users have access to this resource") + return ctx.Status(fiber.StatusForbidden).SendString("access denied: only admin users have access to this resource") } accs, err := c.iam.ListUserAccounts() if err != nil { @@ -86,7 +94,7 @@ func (c AdminController) ListUsers(ctx *fiber.Ctx) error { func (c AdminController) ChangeBucketOwner(ctx *fiber.Ctx) error { acct := ctx.Locals("account").(auth.Account) if acct.Role != "admin" { - return fmt.Errorf("access denied: only admin users have access to this resource") + return ctx.Status(fiber.StatusForbidden).SendString("access denied: only admin users have access to this resource") } owner := ctx.Query("owner") bucket := ctx.Query("bucket") @@ -96,7 +104,7 @@ func (c AdminController) ChangeBucketOwner(ctx *fiber.Ctx) error { return err } if len(accs) > 0 { - return fmt.Errorf("user specified as the new bucket owner does not exist") + return ctx.Status(fiber.StatusNotFound).SendString("user specified as the new bucket owner does not exist") } err = c.be.ChangeBucketOwner(ctx.Context(), bucket, owner) @@ -104,13 +112,13 @@ func (c AdminController) ChangeBucketOwner(ctx *fiber.Ctx) error { return err } - return ctx.Status(201).SendString("Bucket owner has been updated successfully") + return ctx.SendString("Bucket owner has been updated successfully") } func (c AdminController) ListBuckets(ctx *fiber.Ctx) error { acct := ctx.Locals("account").(auth.Account) if acct.Role != "admin" { - return fmt.Errorf("access denied: only admin users have access to this resource") + return ctx.Status(fiber.StatusForbidden).SendString("access denied: only admin users have access to this resource") } buckets, err := c.be.ListBucketsAndOwners(ctx.Context()) diff --git a/s3api/controllers/admin_test.go b/s3api/controllers/admin_test.go index cdd8299..f26f247 100644 --- a/s3api/controllers/admin_test.go +++ b/s3api/controllers/admin_test.go @@ -85,7 +85,7 @@ func TestAdminController_CreateUser(t *testing.T) { req: httptest.NewRequest(http.MethodPatch, "/create-user", bytes.NewBuffer(succUsr)), }, wantErr: false, - statusCode: 200, + statusCode: 201, }, { name: "Admin-create-user-invalid-user-role", @@ -94,7 +94,7 @@ func TestAdminController_CreateUser(t *testing.T) { req: httptest.NewRequest(http.MethodPatch, "/create-user", bytes.NewBuffer(user)), }, wantErr: false, - statusCode: 500, + statusCode: 400, }, { name: "Admin-create-user-invalid-requester-role", @@ -103,7 +103,7 @@ func TestAdminController_CreateUser(t *testing.T) { req: httptest.NewRequest(http.MethodPatch, "/create-user", nil), }, wantErr: false, - statusCode: 500, + statusCode: 403, }, } for _, tt := range tests { @@ -173,7 +173,7 @@ func TestAdminController_DeleteUser(t *testing.T) { req: httptest.NewRequest(http.MethodPatch, "/delete-user?access=test", nil), }, wantErr: false, - statusCode: 500, + statusCode: 403, }, } for _, tt := range tests { @@ -251,7 +251,7 @@ func TestAdminController_ListUsers(t *testing.T) { req: httptest.NewRequest(http.MethodPatch, "/list-users", nil), }, wantErr: false, - statusCode: 500, + statusCode: 403, }, { name: "Admin-list-users-iam-error", @@ -368,7 +368,7 @@ func TestAdminController_ChangeBucketOwner(t *testing.T) { req: httptest.NewRequest(http.MethodPatch, "/change-bucket-owner", nil), }, wantErr: false, - statusCode: 500, + statusCode: 403, }, { name: "Change-bucket-owner-check-account-server-error", @@ -386,7 +386,7 @@ func TestAdminController_ChangeBucketOwner(t *testing.T) { req: httptest.NewRequest(http.MethodPatch, "/change-bucket-owner", nil), }, wantErr: false, - statusCode: 500, + statusCode: 404, }, { name: "Change-bucket-owner-success", @@ -395,7 +395,7 @@ func TestAdminController_ChangeBucketOwner(t *testing.T) { req: httptest.NewRequest(http.MethodPatch, "/change-bucket-owner?bucket=bucket&owner=owner", nil), }, wantErr: false, - statusCode: 201, + statusCode: 200, }, } for _, tt := range tests { @@ -455,7 +455,7 @@ func TestAdminController_ListBuckets(t *testing.T) { req: httptest.NewRequest(http.MethodPatch, "/list-buckets", nil), }, wantErr: false, - statusCode: 500, + statusCode: 403, }, { name: "List-buckets-success",