return errors

weed/s3api/policy_conversion.go

@@ -45,16 +45,16 @@ func ConvertPolicyDocumentToPolicyEngine(src *policy.PolicyDocument) (*policy_en
 
 // convertStatement converts a policy.Statement to policy_engine.PolicyStatement
 func convertStatement(src *policy.Statement) (policy_engine.PolicyStatement, error) {
-	// Warn about unsupported fields that will be ignored
-	// These fields invert the logic and are critical for policy semantics
+	// Check for unsupported fields that would fundamentally change policy semantics
+	// These fields invert the logic and ignoring them could create security holes
 	if len(src.NotAction) > 0 {
-		glog.Warningf("statement %q: NotAction is not supported and will be ignored (this may make the policy more permissive than intended)", src.Sid)
+		return policy_engine.PolicyStatement{}, fmt.Errorf("statement %q: NotAction is not supported (would invert action logic, creating potential security risk)", src.Sid)
 	}
 	if len(src.NotResource) > 0 {
-		glog.Warningf("statement %q: NotResource is not supported and will be ignored (this may make the policy more permissive than intended)", src.Sid)
+		return policy_engine.PolicyStatement{}, fmt.Errorf("statement %q: NotResource is not supported (would invert resource logic, creating potential security risk)", src.Sid)
 	}
 	if src.NotPrincipal != nil {
-		glog.Warningf("statement %q: NotPrincipal is not supported and will be ignored (this may make the policy more permissive than intended)", src.Sid)
+		return policy_engine.PolicyStatement{}, fmt.Errorf("statement %q: NotPrincipal is not supported (would invert principal logic, creating potential security risk)", src.Sid)
 	}
 
 	stmt := policy_engine.PolicyStatement{
@@ -76,7 +76,7 @@ func convertStatement(src *policy.Statement) (policy_engine.PolicyStatement, err
 	if src.Principal != nil {
 		principal, err := convertPrincipal(src.Principal)
 		if err != nil {
-			return stmt, fmt.Errorf("failed to convert principal: %w", err)
+			return policy_engine.PolicyStatement{}, fmt.Errorf("failed to convert principal: %w", err)
 		}
 		stmt.Principal = principal
 	}
@@ -85,7 +85,7 @@ func convertStatement(src *policy.Statement) (policy_engine.PolicyStatement, err
 	if len(src.Condition) > 0 {
 		condition, err := convertCondition(src.Condition)
 		if err != nil {
-			return stmt, fmt.Errorf("failed to convert condition: %w", err)
+			return policy_engine.PolicyStatement{}, fmt.Errorf("failed to convert condition: %w", err)
 		}
 		stmt.Condition = condition
 	}

weed/s3api/policy_conversion_test.go

@@ -1,6 +1,7 @@
 package s3api
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/seaweedfs/seaweedfs/weed/iam/policy"
@@ -438,12 +439,13 @@ func TestConvertPrincipalUnsupportedTypes(t *testing.T) {
 }
 
 func TestConvertStatementWithUnsupportedFields(t *testing.T) {
-	// Test that warnings are logged for unsupported fields
-	// These fields are critical for policy semantics and ignoring them is a security risk
+	// Test that errors are returned for unsupported fields
+	// These fields are critical for policy semantics and ignoring them would be a security risk
 	
 	testCases := []struct {
 		name      string
 		statement *policy.Statement
+		wantError string
 	}{
 		{
 			name: "NotAction field",
@@ -454,6 +456,7 @@ func TestConvertStatementWithUnsupportedFields(t *testing.T) {
 				NotAction: []string{"s3:PutObject", "s3:DeleteObject"},
 				Resource:  []string{"arn:aws:s3:::bucket/*"},
 			},
+			wantError: "NotAction is not supported",
 		},
 		{
 			name: "NotResource field",
@@ -464,6 +467,7 @@ func TestConvertStatementWithUnsupportedFields(t *testing.T) {
 				Resource:    []string{"arn:aws:s3:::bucket/*"},
 				NotResource: []string{"arn:aws:s3:::bucket/secret/*"},
 			},
+			wantError: "NotResource is not supported",
 		},
 		{
 			name: "NotPrincipal field",
@@ -474,43 +478,53 @@ func TestConvertStatementWithUnsupportedFields(t *testing.T) {
 				Resource:     []string{"arn:aws:s3:::bucket/*"},
 				NotPrincipal: map[string]interface{}{"AWS": "arn:aws:iam::123456789012:user/Admin"},
 			},
-		},
-		{
-			name: "All unsupported fields",
-			statement: &policy.Statement{
-				Sid:          "TestAllUnsupported",
-				Effect:       "Deny",
-				Action:       []string{"s3:GetObject"},
-				NotAction:    []string{"s3:PutObject"},
-				Resource:     []string{"arn:aws:s3:::bucket/*"},
-				NotResource:  []string{"arn:aws:s3:::bucket/public/*"},
-				NotPrincipal: map[string]interface{}{"AWS": "*"},
-			},
+			wantError: "NotPrincipal is not supported",
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			// The conversion should succeed but log warnings
+			// The conversion should fail with an error for security reasons
 			result, err := convertStatement(tc.statement)
-			if err != nil {
-				t.Errorf("Unexpected error: %v", err)
+			if err == nil {
+				t.Error("Expected error for unsupported field, got nil")
+			} else if !strings.Contains(err.Error(), tc.wantError) {
+				t.Errorf("Expected error containing %q, got: %v", tc.wantError, err)
 			}
 			
-			// Verify the result has the basic fields
-			if result.Sid != tc.statement.Sid {
-				t.Errorf("Expected Sid %q, got %q", tc.statement.Sid, result.Sid)
-			}
-			if string(result.Effect) != tc.statement.Effect {
-				t.Errorf("Expected Effect %q, got %q", tc.statement.Effect, result.Effect)
+			// Verify zero-value struct is returned on error
+			if result.Sid != "" || result.Effect != "" {
+				t.Error("Expected zero-value struct on error")
 			}
-			
-			// Note: We can't easily verify the warnings were logged without
-			// capturing log output, but the conversion should complete successfully
 		})
 	}
 }
 
+func TestConvertStatementSuccess(t *testing.T) {
+	// Test successful conversion without unsupported fields
+	statement := &policy.Statement{
+		Sid:      "AllowGetObject",
+		Effect:   "Allow",
+		Action:   []string{"s3:GetObject"},
+		Resource: []string{"arn:aws:s3:::bucket/*"},
+		Principal: map[string]interface{}{
+			"AWS": "arn:aws:iam::123456789012:user/Alice",
+		},
+	}
+
+	result, err := convertStatement(statement)
+	if err != nil {
+		t.Fatalf("Unexpected error: %v", err)
+	}
+
+	if result.Sid != statement.Sid {
+		t.Errorf("Expected Sid %q, got %q", statement.Sid, result.Sid)
+	}
+	if string(result.Effect) != statement.Effect {
+		t.Errorf("Expected Effect %q, got %q", statement.Effect, result.Effect)
+	}
+}
+
 func TestConvertPolicyDocumentWithId(t *testing.T) {
 	// Test that policy document Id field triggers a warning
 	src := &policy.PolicyDocument{