Merge pull request #2335 from versity/sis/getbucketversioning-access-control

feat: govern GetBucketVersioning with the regular access checks

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{

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{

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

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

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,