[PM-26429] Add validation to policy data and metadata (#6460)

* Enhance PolicyRequestModel and SavePolicyRequest with validation for policy data and metadata.

* Add integration tests for policy updates to validate handling of invalid data types in PolicyRequestModel and SavePolicyRequest.

* Add missing using

* Update PolicyRequestModel for null safety by making Data and ValidateAndSerializePolicyData nullable

* Add integration tests for public PoliciesController to validate handling of invalid data types in policy updates.

* Add PolicyDataValidator class for validating and serializing policy data and metadata based on policy type.

* Refactor PolicyRequestModel, SavePolicyRequest, and PolicyUpdateRequestModel to utilize PolicyDataValidator for data validation and serialization, removing redundant methods and improving code clarity.

* Update PolicyRequestModel and SavePolicyRequest to initialize Data and Metadata properties with empty dictionaries.

* Refactor PolicyDataValidator to remove null checks for input data in validation methods

* Rename test methods in SavePolicyRequestTests to reflect handling of empty data and metadata, and remove null assignments in test cases for improved clarity.

* Enhance error handling in PolicyDataValidator to include field-specific details in BadRequestException messages.

* Enhance PoliciesControllerTests to verify error messages for BadRequest responses by checking for specific field names in the response content.

* refactor: Update PolicyRequestModel and SavePolicyRequest to use nullable dictionaries for Data and Metadata properties; enhance validation methods in PolicyDataValidator to handle null cases.

* test: Add integration tests for handling policies with null data in PoliciesController

* fix: Catch specific JsonException in PolicyDataValidator to improve error handling

* test: Add unit tests for PolicyDataValidator to validate and serialize policy data and metadata

* test: Update PolicyDataValidatorTests to validate organization data ownership metadata

src/Api/AdminConsole/Models/Request/PolicyRequestModel.cs

@@ -1,11 +1,8 @@
-// FIXME: Update this file to be null safe and then delete the line below
+using System.ComponentModel.DataAnnotations;
-#nullable disable
-
-using System.ComponentModel.DataAnnotations;
-using System.Text.Json;
 using Bit.Core.AdminConsole.Enums;
 using Bit.Core.AdminConsole.Models.Data;
 using Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models;
+using Bit.Core.AdminConsole.Utilities;
 using Bit.Core.Context;
 
 namespace Bit.Api.AdminConsole.Models.Request;
@@ -16,14 +13,20 @@ public class PolicyRequestModel
     public PolicyType? Type { get; set; }
     [Required]
     public bool? Enabled { get; set; }
-    public Dictionary<string, object> Data { get; set; }
+    public Dictionary<string, object>? Data { get; set; }
 
-    public async Task<PolicyUpdate> ToPolicyUpdateAsync(Guid organizationId, ICurrentContext currentContext) => new()
+    public async Task<PolicyUpdate> ToPolicyUpdateAsync(Guid organizationId, ICurrentContext currentContext)
     {
-        Type = Type!.Value,
+        var serializedData = PolicyDataValidator.ValidateAndSerialize(Data, Type!.Value);
-        OrganizationId = organizationId,
+        var performedBy = new StandardUser(currentContext.UserId!.Value, await currentContext.OrganizationOwner(organizationId));
-        Data = Data != null ? JsonSerializer.Serialize(Data) : null,
+
-        Enabled = Enabled.GetValueOrDefault(),
+        return new()
-        PerformedBy = new StandardUser(currentContext.UserId!.Value, await currentContext.OrganizationOwner(organizationId))
+        {
-    };
+            Type = Type!.Value,
+            OrganizationId = organizationId,
+            Data = serializedData,
+            Enabled = Enabled.GetValueOrDefault(),
+            PerformedBy = performedBy
+        };
+    }
 }

src/Api/AdminConsole/Models/Request/SavePolicyRequest.cs

@@ -1,10 +1,8 @@
 using System.ComponentModel.DataAnnotations;
-using System.Text.Json;
-using Bit.Core.AdminConsole.Enums;
 using Bit.Core.AdminConsole.Models.Data;
 using Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models;
+using Bit.Core.AdminConsole.Utilities;
 using Bit.Core.Context;
-using Bit.Core.Utilities;
 
 namespace Bit.Api.AdminConsole.Models.Request;
 
@@ -17,45 +15,10 @@ public class SavePolicyRequest
 
     public async Task<SavePolicyModel> ToSavePolicyModelAsync(Guid organizationId, ICurrentContext currentContext)
     {
+        var policyUpdate = await Policy.ToPolicyUpdateAsync(organizationId, currentContext);
+        var metadata = PolicyDataValidator.ValidateAndDeserializeMetadata(Metadata, Policy.Type!.Value);
         var performedBy = new StandardUser(currentContext.UserId!.Value, await currentContext.OrganizationOwner(organizationId));
 
-        var updatedPolicy = new PolicyUpdate()
+        return new SavePolicyModel(policyUpdate, performedBy, metadata);
-        {
-            Type = Policy.Type!.Value,
-            OrganizationId = organizationId,
-            Data = Policy.Data != null ? JsonSerializer.Serialize(Policy.Data) : null,
-            Enabled = Policy.Enabled.GetValueOrDefault(),
-        };
-
-        var metadata = MapToPolicyMetadata();
-
-        return new SavePolicyModel(updatedPolicy, performedBy, metadata);
-    }
-
-    private IPolicyMetadataModel MapToPolicyMetadata()
-    {
-        if (Metadata == null)
-        {
-            return new EmptyMetadataModel();
-        }
-
-        return Policy?.Type switch
-        {
-            PolicyType.OrganizationDataOwnership => MapToPolicyMetadata<OrganizationModelOwnershipPolicyModel>(),
-            _ => new EmptyMetadataModel()
-        };
-    }
-
-    private IPolicyMetadataModel MapToPolicyMetadata<T>() where T : IPolicyMetadataModel, new()
-    {
-        try
-        {
-            var json = JsonSerializer.Serialize(Metadata);
-            return CoreHelpers.LoadClassFromJsonData<T>(json);
-        }
-        catch
-        {
-            return new EmptyMetadataModel();
-        }
     }
 }

src/Api/AdminConsole/Public/Models/Request/PolicyUpdateRequestModel.cs

@@ -1,19 +1,24 @@
-using System.Text.Json;
+using Bit.Core.AdminConsole.Enums;
-using Bit.Core.AdminConsole.Enums;
 using Bit.Core.AdminConsole.Models.Data;
 using Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models;
+using Bit.Core.AdminConsole.Utilities;
 using Bit.Core.Enums;
 
 namespace Bit.Api.AdminConsole.Public.Models.Request;
 
 public class PolicyUpdateRequestModel : PolicyBaseModel
 {
-    public PolicyUpdate ToPolicyUpdate(Guid organizationId, PolicyType type) => new()
+    public PolicyUpdate ToPolicyUpdate(Guid organizationId, PolicyType type)
     {
-        Type = type,
+        var serializedData = PolicyDataValidator.ValidateAndSerialize(Data, type);
-        OrganizationId = organizationId,
+
-        Data = Data != null ? JsonSerializer.Serialize(Data) : null,
+        return new()
-        Enabled = Enabled.GetValueOrDefault(),
+        {
-        PerformedBy = new SystemUser(EventSystemUser.PublicApi)
+            Type = type,
-    };
+            OrganizationId = organizationId,
+            Data = serializedData,
+            Enabled = Enabled.GetValueOrDefault(),
+            PerformedBy = new SystemUser(EventSystemUser.PublicApi)
+        };
+    }
 }

src/Core/AdminConsole/Utilities/PolicyDataValidator.cs

@@ -0,0 +1,81 @@
+using System.Text.Json;
+using Bit.Core.AdminConsole.Enums;
+using Bit.Core.AdminConsole.Models.Data.Organizations.Policies;
+using Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models;
+using Bit.Core.Exceptions;
+using Bit.Core.Utilities;
+
+namespace Bit.Core.AdminConsole.Utilities;
+
+public static class PolicyDataValidator
+{
+    /// <summary>
+    /// Validates and serializes policy data based on the policy type.
+    /// </summary>
+    /// <param name="data">The policy data to validate</param>
+    /// <param name="policyType">The type of policy</param>
+    /// <returns>Serialized JSON string if data is valid, null if data is null or empty</returns>
+    /// <exception cref="BadRequestException">Thrown when data validation fails</exception>
+    public static string? ValidateAndSerialize(Dictionary<string, object>? data, PolicyType policyType)
+    {
+        if (data == null || data.Count == 0)
+        {
+            return null;
+        }
+
+        try
+        {
+            var json = JsonSerializer.Serialize(data);
+
+            switch (policyType)
+            {
+                case PolicyType.MasterPassword:
+                    CoreHelpers.LoadClassFromJsonData<MasterPasswordPolicyData>(json);
+                    break;
+                case PolicyType.SendOptions:
+                    CoreHelpers.LoadClassFromJsonData<SendOptionsPolicyData>(json);
+                    break;
+                case PolicyType.ResetPassword:
+                    CoreHelpers.LoadClassFromJsonData<ResetPasswordDataModel>(json);
+                    break;
+            }
+
+            return json;
+        }
+        catch (JsonException ex)
+        {
+            var fieldInfo = !string.IsNullOrEmpty(ex.Path) ? $": field '{ex.Path}' has invalid type" : "";
+            throw new BadRequestException($"Invalid data for {policyType} policy{fieldInfo}.");
+        }
+    }
+
+    /// <summary>
+    /// Validates and deserializes policy metadata based on the policy type.
+    /// </summary>
+    /// <param name="metadata">The policy metadata to validate</param>
+    /// <param name="policyType">The type of policy</param>
+    /// <returns>Deserialized metadata model, or EmptyMetadataModel if metadata is null, empty, or validation fails</returns>
+    public static IPolicyMetadataModel ValidateAndDeserializeMetadata(Dictionary<string, object>? metadata, PolicyType policyType)
+    {
+        if (metadata == null || metadata.Count == 0)
+        {
+            return new EmptyMetadataModel();
+        }
+
+        try
+        {
+            var json = JsonSerializer.Serialize(metadata);
+
+            return policyType switch
+            {
+                PolicyType.OrganizationDataOwnership =>
+                    CoreHelpers.LoadClassFromJsonData<OrganizationModelOwnershipPolicyModel>(json),
+                _ => new EmptyMetadataModel()
+            };
+        }
+        catch (JsonException)
+        {
+            return new EmptyMetadataModel();
+        }
+    }
+}

test/Api.IntegrationTest/AdminConsole/Controllers/PoliciesControllerTests.cs

@@ -211,4 +211,200 @@ public class PoliciesControllerTests : IClassFixture<ApiApplicationFactory>, IAs
         }
     }
 
+    [Fact]
+    public async Task Put_MasterPasswordPolicy_InvalidDataType_ReturnsBadRequest()
+    {
+        // Arrange
+        var policyType = PolicyType.MasterPassword;
+        var request = new PolicyRequestModel
+        {
+            Type = policyType,
+            Enabled = true,
+            Data = new Dictionary<string, object>
+            {
+                { "minLength", "not a number" }, // Wrong type - should be int
+                { "requireUpper", true }
+            }
+        };
+
+        // Act
+        var response = await _client.PutAsync($"/organizations/{_organization.Id}/policies/{policyType}",
+            JsonContent.Create(request));
+
+        // Assert
+        Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode);
+        var content = await response.Content.ReadAsStringAsync();
+        Assert.Contains("minLength", content); // Verify field name is in error message
+    }
+
+    [Fact]
+    public async Task Put_SendOptionsPolicy_InvalidDataType_ReturnsBadRequest()
+    {
+        // Arrange
+        var policyType = PolicyType.SendOptions;
+        var request = new PolicyRequestModel
+        {
+            Type = policyType,
+            Enabled = true,
+            Data = new Dictionary<string, object>
+            {
+                { "disableHideEmail", "not a boolean" } // Wrong type - should be bool
+            }
+        };
+
+        // Act
+        var response = await _client.PutAsync($"/organizations/{_organization.Id}/policies/{policyType}",
+            JsonContent.Create(request));
+
+        // Assert
+        Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode);
+    }
+
+    [Fact]
+    public async Task Put_ResetPasswordPolicy_InvalidDataType_ReturnsBadRequest()
+    {
+        // Arrange
+        var policyType = PolicyType.ResetPassword;
+        var request = new PolicyRequestModel
+        {
+            Type = policyType,
+            Enabled = true,
+            Data = new Dictionary<string, object>
+            {
+                { "autoEnrollEnabled", 123 } // Wrong type - should be bool
+            }
+        };
+
+        // Act
+        var response = await _client.PutAsync($"/organizations/{_organization.Id}/policies/{policyType}",
+            JsonContent.Create(request));
+
+        // Assert
+        Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode);
+    }
+
+    [Fact]
+    public async Task PutVNext_MasterPasswordPolicy_InvalidDataType_ReturnsBadRequest()
+    {
+        // Arrange
+        var policyType = PolicyType.MasterPassword;
+        var request = new SavePolicyRequest
+        {
+            Policy = new PolicyRequestModel
+            {
+                Type = policyType,
+                Enabled = true,
+                Data = new Dictionary<string, object>
+                {
+                    { "minComplexity", "not a number" }, // Wrong type - should be int
+                    { "minLength", 12 }
+                }
+            }
+        };
+
+        // Act
+        var response = await _client.PutAsync($"/organizations/{_organization.Id}/policies/{policyType}/vnext",
+            JsonContent.Create(request));
+
+        // Assert
+        Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode);
+        var content = await response.Content.ReadAsStringAsync();
+        Assert.Contains("minComplexity", content); // Verify field name is in error message
+    }
+
+    [Fact]
+    public async Task PutVNext_SendOptionsPolicy_InvalidDataType_ReturnsBadRequest()
+    {
+        // Arrange
+        var policyType = PolicyType.SendOptions;
+        var request = new SavePolicyRequest
+        {
+            Policy = new PolicyRequestModel
+            {
+                Type = policyType,
+                Enabled = true,
+                Data = new Dictionary<string, object>
+                {
+                    { "disableHideEmail", "not a boolean" } // Wrong type - should be bool
+                }
+            }
+        };
+
+        // Act
+        var response = await _client.PutAsync($"/organizations/{_organization.Id}/policies/{policyType}/vnext",
+            JsonContent.Create(request));
+
+        // Assert
+        Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode);
+    }
+
+    [Fact]
+    public async Task PutVNext_ResetPasswordPolicy_InvalidDataType_ReturnsBadRequest()
+    {
+        // Arrange
+        var policyType = PolicyType.ResetPassword;
+        var request = new SavePolicyRequest
+        {
+            Policy = new PolicyRequestModel
+            {
+                Type = policyType,
+                Enabled = true,
+                Data = new Dictionary<string, object>
+                {
+                    { "autoEnrollEnabled", 123 } // Wrong type - should be bool
+                }
+            }
+        };
+
+        // Act
+        var response = await _client.PutAsync($"/organizations/{_organization.Id}/policies/{policyType}/vnext",
+            JsonContent.Create(request));
+
+        // Assert
+        Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode);
+    }
+
+    [Fact]
+    public async Task Put_PolicyWithNullData_Success()
+    {
+        // Arrange
+        var policyType = PolicyType.SingleOrg;
+        var request = new PolicyRequestModel
+        {
+            Type = policyType,
+            Enabled = true,
+            Data = null
+        };
+
+        // Act
+        var response = await _client.PutAsync($"/organizations/{_organization.Id}/policies/{policyType}",
+            JsonContent.Create(request));
+
+        // Assert
+        Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+    }
+
+    [Fact]
+    public async Task PutVNext_PolicyWithNullData_Success()
+    {
+        // Arrange
+        var policyType = PolicyType.TwoFactorAuthentication;
+        var request = new SavePolicyRequest
+        {
+            Policy = new PolicyRequestModel
+            {
+                Type = policyType,
+                Enabled = true,
+                Data = null
+            },
+            Metadata = null
+        };
+
+        // Act
+        var response = await _client.PutAsync($"/organizations/{_organization.Id}/policies/{policyType}/vnext",
+            JsonContent.Create(request));
+
+        // Assert
+        Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+    }
 }

test/Api.IntegrationTest/AdminConsole/Public/Controllers/PoliciesControllerTests.cs

@@ -160,4 +160,86 @@ public class PoliciesControllerTests : IClassFixture<ApiApplicationFactory>, IAs
         Assert.Equal(15, data.MinLength);
         Assert.Equal(true, data.RequireUpper);
     }
+
+    [Fact]
+    public async Task Put_MasterPasswordPolicy_InvalidDataType_ReturnsBadRequest()
+    {
+        // Arrange
+        var policyType = PolicyType.MasterPassword;
+        var request = new PolicyUpdateRequestModel
+        {
+            Enabled = true,
+            Data = new Dictionary<string, object>
+            {
+                { "minLength", "not a number" }, // Wrong type - should be int
+                { "requireUpper", true }
+            }
+        };
+
+        // Act
+        var response = await _client.PutAsync($"/public/policies/{policyType}", JsonContent.Create(request));
+
+        // Assert
+        Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode);
+    }
+
+    [Fact]
+    public async Task Put_SendOptionsPolicy_InvalidDataType_ReturnsBadRequest()
+    {
+        // Arrange
+        var policyType = PolicyType.SendOptions;
+        var request = new PolicyUpdateRequestModel
+        {
+            Enabled = true,
+            Data = new Dictionary<string, object>
+            {
+                { "disableHideEmail", "not a boolean" } // Wrong type - should be bool
+            }
+        };
+
+        // Act
+        var response = await _client.PutAsync($"/public/policies/{policyType}", JsonContent.Create(request));
+
+        // Assert
+        Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode);
+    }
+
+    [Fact]
+    public async Task Put_ResetPasswordPolicy_InvalidDataType_ReturnsBadRequest()
+    {
+        // Arrange
+        var policyType = PolicyType.ResetPassword;
+        var request = new PolicyUpdateRequestModel
+        {
+            Enabled = true,
+            Data = new Dictionary<string, object>
+            {
+                { "autoEnrollEnabled", 123 } // Wrong type - should be bool
+            }
+        };
+
+        // Act
+        var response = await _client.PutAsync($"/public/policies/{policyType}", JsonContent.Create(request));
+
+        // Assert
+        Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode);
+    }
+
+    [Fact]
+    public async Task Put_PolicyWithNullData_Success()
+    {
+        // Arrange
+        var policyType = PolicyType.DisableSend;
+        var request = new PolicyUpdateRequestModel
+        {
+            Enabled = true,
+            Data = null
+        };
+
+        // Act
+        var response = await _client.PutAsync($"/public/policies/{policyType}", JsonContent.Create(request));
+
+        // Assert
+        Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+    }
 }

test/Api.Test/AdminConsole/Models/Request/SavePolicyRequestTests.cs

@@ -54,7 +54,7 @@ public class SavePolicyRequestTests
     }
 
     [Theory, BitAutoData]
-    public async Task ToSavePolicyModelAsync_WithNullData_HandlesCorrectly(
+    public async Task ToSavePolicyModelAsync_WithEmptyData_HandlesCorrectly(
         Guid organizationId,
         Guid userId)
     {
@@ -68,10 +68,8 @@ public class SavePolicyRequestTests
             Policy = new PolicyRequestModel
             {
                 Type = PolicyType.SingleOrg,
-                Enabled = false,
+                Enabled = false
-                Data = null
+            }
-            },
-            Metadata = null
         };
 
         // Act
@@ -100,10 +98,8 @@ public class SavePolicyRequestTests
             Policy = new PolicyRequestModel
             {
                 Type = PolicyType.SingleOrg,
-                Enabled = false,
+                Enabled = false
-                Data = null
+            }
-            },
-            Metadata = null
         };
 
         // Act
@@ -133,8 +129,7 @@ public class SavePolicyRequestTests
             Policy = new PolicyRequestModel
             {
                 Type = PolicyType.OrganizationDataOwnership,
-                Enabled = true,
+                Enabled = true
-                Data = null
             },
             Metadata = new Dictionary<string, object>
             {
@@ -152,7 +147,7 @@ public class SavePolicyRequestTests
     }
 
     [Theory, BitAutoData]
-    public async Task ToSavePolicyModelAsync_OrganizationDataOwnership_WithNullMetadata_ReturnsEmptyMetadata(
+    public async Task ToSavePolicyModelAsync_OrganizationDataOwnership_WithEmptyMetadata_ReturnsEmptyMetadata(
         Guid organizationId,
         Guid userId)
     {
@@ -166,10 +161,8 @@ public class SavePolicyRequestTests
             Policy = new PolicyRequestModel
             {
                 Type = PolicyType.OrganizationDataOwnership,
-                Enabled = true,
+                Enabled = true
-                Data = null
+            }
-            },
-            Metadata = null
         };
 
         // Act
@@ -246,8 +239,7 @@ public class SavePolicyRequestTests
             Policy = new PolicyRequestModel
             {
                 Type = PolicyType.MaximumVaultTimeout,
-                Enabled = true,
+                Enabled = true
-                Data = null
             },
             Metadata = new Dictionary<string, object>
             {
@@ -280,8 +272,7 @@ public class SavePolicyRequestTests
             Policy = new PolicyRequestModel
             {
                 Type = PolicyType.OrganizationDataOwnership,
-                Enabled = true,
+                Enabled = true
-                Data = null
             },
             Metadata = errorDictionary
         };

test/Core.Test/AdminConsole/Utilities/PolicyDataValidatorTests.cs

@@ -0,0 +1,59 @@
+using Bit.Core.AdminConsole.Enums;
+using Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models;
+using Bit.Core.AdminConsole.Utilities;
+using Bit.Core.Exceptions;
+using Xunit;
+
+namespace Bit.Core.Test.AdminConsole.Utilities;
+
+public class PolicyDataValidatorTests
+{
+    [Fact]
+    public void ValidateAndSerialize_NullData_ReturnsNull()
+    {
+        var result = PolicyDataValidator.ValidateAndSerialize(null, PolicyType.MasterPassword);
+
+        Assert.Null(result);
+    }
+
+    [Fact]
+    public void ValidateAndSerialize_ValidData_ReturnsSerializedJson()
+    {
+        var data = new Dictionary<string, object> { { "minLength", 12 } };
+
+        var result = PolicyDataValidator.ValidateAndSerialize(data, PolicyType.MasterPassword);
+
+        Assert.NotNull(result);
+        Assert.Contains("\"minLength\":12", result);
+    }
+
+    [Fact]
+    public void ValidateAndSerialize_InvalidDataType_ThrowsBadRequestException()
+    {
+        var data = new Dictionary<string, object> { { "minLength", "not a number" } };
+
+        var exception = Assert.Throws<BadRequestException>(() =>
+            PolicyDataValidator.ValidateAndSerialize(data, PolicyType.MasterPassword));
+
+        Assert.Contains("Invalid data for MasterPassword policy", exception.Message);
+        Assert.Contains("minLength", exception.Message);
+    }
+
+    [Fact]
+    public void ValidateAndDeserializeMetadata_NullMetadata_ReturnsEmptyMetadataModel()
+    {
+        var result = PolicyDataValidator.ValidateAndDeserializeMetadata(null, PolicyType.SingleOrg);
+
+        Assert.IsType<EmptyMetadataModel>(result);
+    }
+
+    [Fact]
+    public void ValidateAndDeserializeMetadata_ValidMetadata_ReturnsModel()
+    {
+        var metadata = new Dictionary<string, object> { { "defaultUserCollectionName", "collection name" } };
+
+        var result = PolicyDataValidator.ValidateAndDeserializeMetadata(metadata, PolicyType.OrganizationDataOwnership);
+
+        Assert.IsType<OrganizationModelOwnershipPolicyModel>(result);
+    }
+}