diff --git a/auth/access-control.go b/auth/access-control.go index 90f60eb0..8fa0dab9 100644 --- a/auth/access-control.go +++ b/auth/access-control.go @@ -618,26 +618,6 @@ func verifyIdentityOnlyAccess(ctx fiber.Ctx, pe PolicyEvaluator, acc Account, ac return s3err.GetImplicitDenyAccessErr(principal, string(action), resourceArn) } -func IsAdminOrOwner(acct Account, isRoot bool, acl ACL) error { - // Owner check - if acct.Access == acl.Owner { - return nil - } - - // Root user has access over almost everything - if isRoot { - return nil - } - - // Admin user case - if acct.Role == RoleAdmin { - return nil - } - - // Return access denied in all other cases - return s3err.GetAPIError(s3err.ErrAccessDenied) -} - type PublicACLAllowedActions map[Action]struct{} var publicACLAllowedActions PublicACLAllowedActions = PublicACLAllowedActions{ diff --git a/s3api/controllers/bucket-get.go b/s3api/controllers/bucket-get.go index 9e287fd7..96423fa0 100644 --- a/s3api/controllers/bucket-get.go +++ b/s3api/controllers/bucket-get.go @@ -138,14 +138,6 @@ func (c S3ApiController) GetBucketVersioning(ctx fiber.Ctx) (*Response, error) { }, }, err } - // Only admin users and the bucket owner are allowed to get the versioning state of a bucket. - if err := auth.IsAdminOrOwner(acct, isRoot, parsedAcl); err != nil { - return &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: parsedAcl.Owner, - }, - }, err - } data, err := c.be.GetBucketVersioning(ctx.RequestCtx(), bucket) return &Response{ diff --git a/s3api/controllers/bucket-get_test.go b/s3api/controllers/bucket-get_test.go index 5736adcd..92ded301 100644 --- a/s3api/controllers/bucket-get_test.go +++ b/s3api/controllers/bucket-get_test.go @@ -231,7 +231,7 @@ func TestS3ApiController_GetBucketVersioning(t *testing.T) { }, }, { - name: "not admin or root", + name: "non owner user with verified access", input: testInput{ locals: map[utils.ContextKey]any{ utils.ContextKeyIsRoot: false, @@ -244,14 +244,15 @@ func TestS3ApiController_GetBucketVersioning(t *testing.T) { }, utils.ContextKeyPublicBucket: true, }, + beRes: validRes, }, output: testOutput{ response: &Response{ + Data: validRes, MetaOpts: &MetaOptions{ BucketOwner: "root", }, }, - err: s3err.GetAPIError(s3err.ErrAccessDenied), }, }, { diff --git a/tests/integration/GetBucketVersioning.go b/tests/integration/GetBucketVersioning.go index 517e01c3..128577f8 100644 --- a/tests/integration/GetBucketVersioning.go +++ b/tests/integration/GetBucketVersioning.go @@ -83,3 +83,116 @@ func GetBucketVersioning_success(s *S3Conf) error { return nil }, withVersioning(types.BucketVersioningStatusEnabled)) } + +func GetBucketVersioning_non_owner_access_denied(s *S3Conf) error { + testName := "GetBucketVersioning_non_owner_access_denied" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + testuser := getUser("user") + if err := createUsers(s, []user{testuser}); err != nil { + return err + } + + userClient := s.getUserClient(testuser) + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := userClient.GetBucketVersioning(ctx, &s3.GetBucketVersioningInput{ + Bucket: &bucket, + }) + cancel() + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)); err != nil { + return err + } + + return nil + }, withVersioning(types.BucketVersioningStatusEnabled)) +} + +func GetBucketVersioning_with_policy_access(s *S3Conf) error { + testName := "GetBucketVersioning_with_policy_access" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + testuser := getUser("user") + if err := createUsers(s, []user{testuser}); err != nil { + return err + } + + bucketResource := fmt.Sprintf(`"arn:aws:s3:::%v"`, bucket) + principal := fmt.Sprintf(`"%v"`, testuser.access) + userClient := s.getUserClient(testuser) + + // Error path: the policy grants the user another bucket action, + // but not s3:GetBucketVersioning. + policy := genPolicyDoc("Allow", principal, `"s3:GetBucketTagging"`, bucketResource) + if err := putBucketPolicy(s3client, bucket, policy); err != nil { + return err + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := userClient.GetBucketVersioning(ctx, &s3.GetBucketVersioningInput{ + Bucket: &bucket, + }) + cancel() + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)); err != nil { + return err + } + + // Happy path: the policy grants s3:GetBucketVersioning to a user + // who neither owns the bucket nor is an admin. + policy = genPolicyDoc("Allow", principal, `"s3:GetBucketVersioning"`, bucketResource) + if err := putBucketPolicy(s3client, bucket, policy); err != nil { + return err + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + res, err := userClient.GetBucketVersioning(ctx, &s3.GetBucketVersioningInput{ + Bucket: &bucket, + }) + cancel() + if err != nil { + return err + } + + if res.Status != types.BucketVersioningStatusEnabled { + return fmt.Errorf("expected bucket versioning status to be %v, instead got %v", + types.BucketVersioningStatusEnabled, res.Status) + } + + return nil + }, withVersioning(types.BucketVersioningStatusEnabled)) +} + +func GetBucketVersioning_with_acl_access(s *S3Conf) error { + testName := "GetBucketVersioning_with_acl_access" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + testuser := getUser("user") + if err := createUsers(s, []user{testuser}); err != nil { + return err + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s3client.PutBucketAcl(ctx, &s3.PutBucketAclInput{ + Bucket: &bucket, + GrantRead: &testuser.access, + }) + cancel() + if err != nil { + return err + } + + userClient := s.getUserClient(testuser) + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + res, err := userClient.GetBucketVersioning(ctx, &s3.GetBucketVersioningInput{ + Bucket: &bucket, + }) + cancel() + if err != nil { + return err + } + + if res.Status != types.BucketVersioningStatusEnabled { + return fmt.Errorf("expected bucket versioning status to be %v, instead got %v", + types.BucketVersioningStatusEnabled, res.Status) + } + + return nil + }, withVersioning(types.BucketVersioningStatusEnabled), + withOwnership(types.ObjectOwnershipBucketOwnerPreferred)) +} diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 40258da9..a4715a51 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -1874,6 +1874,9 @@ func TestVersioning(ts *TestState) { ts.Run(GetBucketVersioning_non_existing_bucket) ts.Run(GetBucketVersioning_empty_response) ts.Run(GetBucketVersioning_success) + ts.Run(GetBucketVersioning_non_owner_access_denied) + ts.Run(GetBucketVersioning_with_policy_access) + ts.Run(GetBucketVersioning_with_acl_access) // DeleteBucket action ts.Run(Versioning_DeleteBucket_not_empty) // PutObject action @@ -3344,6 +3347,9 @@ func GetIntTests() IntTests { "GetBucketVersioning_non_existing_bucket": GetBucketVersioning_non_existing_bucket, "GetBucketVersioning_empty_response": GetBucketVersioning_empty_response, "GetBucketVersioning_success": GetBucketVersioning_success, + "GetBucketVersioning_non_owner_access_denied": GetBucketVersioning_non_owner_access_denied, + "GetBucketVersioning_with_policy_access": GetBucketVersioning_with_policy_access, + "GetBucketVersioning_with_acl_access": GetBucketVersioning_with_acl_access, "Versioning_DeleteBucket_not_empty": Versioning_DeleteBucket_not_empty, "Versioning_PutObject_suspended_null_versionId_obj": Versioning_PutObject_suspended_null_versionId_obj, "Versioning_PutObject_null_versionId_obj": Versioning_PutObject_null_versionId_obj,