recursion

weed/s3api/policy_conversion.go

@@ -99,16 +99,18 @@ func convertPrincipal(principal interface{}) (*policy_engine.StringOrStringSlice
 		return nil, nil
 	}
 
-	var strs []string
-
 	switch p := principal.(type) {
 	case string:
-		strs = []string{p}
+		result := policy_engine.NewStringOrStringSlice(p)
+		return &result, nil
 	case []string:
-		strs = p
+		if len(p) == 0 {
+			return nil, nil
+		}
+		result := policy_engine.NewStringOrStringSlice(p...)
+		return &result, nil
 	case []interface{}:
-		// Convert []interface{} to []string
-		strs = make([]string, 0, len(p))
+		strs := make([]string, 0, len(p))
 		for _, v := range p {
 			if v != nil {
 				str, err := convertToString(v)
@@ -118,6 +120,11 @@ func convertPrincipal(principal interface{}) (*policy_engine.StringOrStringSlice
 				strs = append(strs, str)
 			}
 		}
+		if len(strs) == 0 {
+			return nil, nil
+		}
+		result := policy_engine.NewStringOrStringSlice(strs...)
+		return &result, nil
 	case map[string]interface{}:
 		// Handle AWS-style principal with service/user keys
 		// Example: {"AWS": "arn:aws:iam::123456789012:user/Alice"}
@@ -129,35 +136,15 @@ func convertPrincipal(principal interface{}) (*policy_engine.StringOrStringSlice
 			return nil, fmt.Errorf("unsupported principal map, only a single 'AWS' key is supported, got keys: %v", getMapKeys(p))
 		}
 		
-		switch val := awsPrincipals.(type) {
-		case string:
-			strs = append(strs, val)
-		case []string:
-			strs = append(strs, val...)
-		case []interface{}:
-			for _, item := range val {
-				if item != nil {
-					str, err := convertToString(item)
-					if err != nil {
-						return nil, fmt.Errorf("failed to convert AWS principal item: %w", err)
-					}
-					strs = append(strs, str)
-				}
-			}
-		default:
-			glog.Warningf("unsupported type for 'AWS' principal value: %T", val)
-			return nil, fmt.Errorf("unsupported type for 'AWS' principal value: %T", val)
+		// Recursively convert the AWS principal value
+		res, err := convertPrincipal(awsPrincipals)
+		if err != nil {
+			return nil, fmt.Errorf("invalid 'AWS' principal value: %w", err)
 		}
+		return res, nil
 	default:
 		return nil, fmt.Errorf("unsupported principal type: %T", p)
 	}
-
-	if len(strs) > 0 {
-		result := policy_engine.NewStringOrStringSlice(strs...)
-		return &result, nil
-	}
-
-	return nil, nil
 }
 
 // convertCondition converts policy conditions to PolicyConditions

weed/s3api/policy_conversion_test.go

@@ -437,3 +437,111 @@ 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
+	
+	testCases := []struct {
+		name      string
+		statement *policy.Statement
+	}{
+		{
+			name: "NotAction field",
+			statement: &policy.Statement{
+				Sid:       "TestNotAction",
+				Effect:    "Deny",
+				Action:    []string{"s3:GetObject"},
+				NotAction: []string{"s3:PutObject", "s3:DeleteObject"},
+				Resource:  []string{"arn:aws:s3:::bucket/*"},
+			},
+		},
+		{
+			name: "NotResource field",
+			statement: &policy.Statement{
+				Sid:         "TestNotResource",
+				Effect:      "Allow",
+				Action:      []string{"s3:*"},
+				Resource:    []string{"arn:aws:s3:::bucket/*"},
+				NotResource: []string{"arn:aws:s3:::bucket/secret/*"},
+			},
+		},
+		{
+			name: "NotPrincipal field",
+			statement: &policy.Statement{
+				Sid:          "TestNotPrincipal",
+				Effect:       "Deny",
+				Action:       []string{"s3:*"},
+				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": "*"},
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			// The conversion should succeed but log warnings
+			result, err := convertStatement(tc.statement)
+			if err != nil {
+				t.Errorf("Unexpected error: %v", 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)
+			}
+			
+			// Note: We can't easily verify the warnings were logged without
+			// capturing log output, but the conversion should complete successfully
+		})
+	}
+}
+
+func TestConvertPolicyDocumentWithId(t *testing.T) {
+	// Test that policy document Id field triggers a warning
+	src := &policy.PolicyDocument{
+		Version: "2012-10-17",
+		Id:      "MyPolicyId",
+		Statement: []policy.Statement{
+			{
+				Sid:      "AllowGetObject",
+				Effect:   "Allow",
+				Action:   []string{"s3:GetObject"},
+				Resource: []string{"arn:aws:s3:::bucket/*"},
+			},
+		},
+	}
+
+	// The conversion should succeed but log a warning about Id
+	dest, err := ConvertPolicyDocumentToPolicyEngine(src)
+	if err != nil {
+		t.Fatalf("Unexpected error: %v", err)
+	}
+
+	if dest == nil {
+		t.Fatal("Expected non-nil result")
+	}
+
+	// Verify basic conversion worked
+	if dest.Version != src.Version {
+		t.Errorf("Expected Version %q, got %q", src.Version, dest.Version)
+	}
+	if len(dest.Statement) != 1 {
+		t.Errorf("Expected 1 statement, got %d", len(dest.Statement))
+	}
+}
+