Skip to content

Commit e7f3b6b

Browse files
r-tomer-tome
authored
[PM-26430] Remove Type property from PolicyRequestModel to use route parameter only (#6472)
* 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. * Remove Type property from PolicyRequestModel to use route parameter only * Run dotnet format * 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: Remove PolicyType from PolicyRequestModel in PoliciesControllerTests * test: Update PolicyDataValidatorTests to validate organization data ownership metadata * Refactor PoliciesControllerTests to include policy type in PutVNext method calls
1 parent 7d39efe commit e7f3b6b

File tree

6 files changed

+30
-50
lines changed

6 files changed

+30
-50
lines changed

‎src/Api/AdminConsole/Controllers/PoliciesController.cs‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -209,29 +209,22 @@ public async Task<PolicyResponseModel> Put(Guid orgId, PolicyType type, [FromBod
209209
throw new NotFoundException();
210210
}
211211

212-
if (type != model.Type)
213-
{
214-
throw new BadRequestException("Mismatched policy type");
215-
}
216-
217-
var policyUpdate = await model.ToPolicyUpdateAsync(orgId, _currentContext);
212+
var policyUpdate = await model.ToPolicyUpdateAsync(orgId, type, _currentContext);
218213
var policy = await _savePolicyCommand.SaveAsync(policyUpdate);
219214
return new PolicyResponseModel(policy);
220215
}
221216

222-
223217
[HttpPut("{type}/vnext")]
224218
[RequireFeatureAttribute(FeatureFlagKeys.CreateDefaultLocation)]
225219
[Authorize<ManagePoliciesRequirement>]
226-
public async Task<PolicyResponseModel> PutVNext(Guid orgId, [FromBody] SavePolicyRequest model)
220+
public async Task<PolicyResponseModel> PutVNext(Guid orgId, PolicyType type, [FromBody] SavePolicyRequest model)
227221
{
228-
var savePolicyRequest = await model.ToSavePolicyModelAsync(orgId, _currentContext);
222+
var savePolicyRequest = await model.ToSavePolicyModelAsync(orgId, type, _currentContext);
229223

230224
var policy = _featureService.IsEnabled(FeatureFlagKeys.PolicyValidatorsRefactor) ?
231225
await _vNextSavePolicyCommand.SaveAsync(savePolicyRequest) :
232226
await _savePolicyCommand.VNextSaveAsync(savePolicyRequest);
233227

234228
return new PolicyResponseModel(policy);
235229
}
236-
237230
}

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

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,20 +9,18 @@ namespace Bit.Api.AdminConsole.Models.Request;
99

1010
public class PolicyRequestModel
1111
{
12-
[Required]
13-
public PolicyType? Type { get; set; }
1412
[Required]
1513
public bool? Enabled { get; set; }
1614
public Dictionary<string, object>? Data { get; set; }
1715

18-
public async Task<PolicyUpdate> ToPolicyUpdateAsync(Guid organizationId, ICurrentContext currentContext)
16+
public async Task<PolicyUpdate> ToPolicyUpdateAsync(Guid organizationId, PolicyType type, ICurrentContext currentContext)
1917
{
20-
var serializedData = PolicyDataValidator.ValidateAndSerialize(Data, Type!.Value);
18+
var serializedData = PolicyDataValidator.ValidateAndSerialize(Data, type);
2119
var performedBy = new StandardUser(currentContext.UserId!.Value, await currentContext.OrganizationOwner(organizationId));
2220

2321
return new()
2422
{
25-
Type = Type!.Value,
23+
Type = type,
2624
OrganizationId = organizationId,
2725
Data = serializedData,
2826
Enabled = Enabled.GetValueOrDefault(),

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using System.ComponentModel.DataAnnotations;
2+
using Bit.Core.AdminConsole.Enums;
23
using Bit.Core.AdminConsole.Models.Data;
34
using Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models;
45
using Bit.Core.AdminConsole.Utilities;
@@ -13,10 +14,10 @@ public class SavePolicyRequest
1314

1415
public Dictionary<string, object>? Metadata { get; set; }
1516

16-
public async Task<SavePolicyModel> ToSavePolicyModelAsync(Guid organizationId, ICurrentContext currentContext)
17+
public async Task<SavePolicyModel> ToSavePolicyModelAsync(Guid organizationId, PolicyType type, ICurrentContext currentContext)
1718
{
18-
var policyUpdate = await Policy.ToPolicyUpdateAsync(organizationId, currentContext);
19-
var metadata = PolicyDataValidator.ValidateAndDeserializeMetadata(Metadata, Policy.Type!.Value);
19+
var policyUpdate = await Policy.ToPolicyUpdateAsync(organizationId, type, currentContext);
20+
var metadata = PolicyDataValidator.ValidateAndDeserializeMetadata(Metadata, type);
2021
var performedBy = new StandardUser(currentContext.UserId!.Value, await currentContext.OrganizationOwner(organizationId));
2122

2223
return new SavePolicyModel(policyUpdate, performedBy, metadata);

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

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,6 @@ public async Task PutVNext_OrganizationDataOwnershipPolicy_Success()
6767
{
6868
Policy = new PolicyRequestModel
6969
{
70-
Type = policyType,
7170
Enabled = true,
7271
},
7372
Metadata = new Dictionary<string, object>
@@ -148,7 +147,6 @@ public async Task PutVNext_MasterPasswordPolicy_Success()
148147
{
149148
Policy = new PolicyRequestModel
150149
{
151-
Type = policyType,
152150
Enabled = true,
153151
Data = new Dictionary<string, object>
154152
{
@@ -218,7 +216,6 @@ public async Task Put_MasterPasswordPolicy_InvalidDataType_ReturnsBadRequest()
218216
var policyType = PolicyType.MasterPassword;
219217
var request = new PolicyRequestModel
220218
{
221-
Type = policyType,
222219
Enabled = true,
223220
Data = new Dictionary<string, object>
224221
{
@@ -244,7 +241,6 @@ public async Task Put_SendOptionsPolicy_InvalidDataType_ReturnsBadRequest()
244241
var policyType = PolicyType.SendOptions;
245242
var request = new PolicyRequestModel
246243
{
247-
Type = policyType,
248244
Enabled = true,
249245
Data = new Dictionary<string, object>
250246
{
@@ -267,7 +263,6 @@ public async Task Put_ResetPasswordPolicy_InvalidDataType_ReturnsBadRequest()
267263
var policyType = PolicyType.ResetPassword;
268264
var request = new PolicyRequestModel
269265
{
270-
Type = policyType,
271266
Enabled = true,
272267
Data = new Dictionary<string, object>
273268
{
@@ -292,7 +287,6 @@ public async Task PutVNext_MasterPasswordPolicy_InvalidDataType_ReturnsBadReques
292287
{
293288
Policy = new PolicyRequestModel
294289
{
295-
Type = policyType,
296290
Enabled = true,
297291
Data = new Dictionary<string, object>
298292
{
@@ -321,7 +315,6 @@ public async Task PutVNext_SendOptionsPolicy_InvalidDataType_ReturnsBadRequest()
321315
{
322316
Policy = new PolicyRequestModel
323317
{
324-
Type = policyType,
325318
Enabled = true,
326319
Data = new Dictionary<string, object>
327320
{
@@ -347,7 +340,6 @@ public async Task PutVNext_ResetPasswordPolicy_InvalidDataType_ReturnsBadRequest
347340
{
348341
Policy = new PolicyRequestModel
349342
{
350-
Type = policyType,
351343
Enabled = true,
352344
Data = new Dictionary<string, object>
353345
{
@@ -371,7 +363,6 @@ public async Task Put_PolicyWithNullData_Success()
371363
var policyType = PolicyType.SingleOrg;
372364
var request = new PolicyRequestModel
373365
{
374-
Type = policyType,
375366
Enabled = true,
376367
Data = null
377368
};
@@ -393,7 +384,6 @@ public async Task PutVNext_PolicyWithNullData_Success()
393384
{
394385
Policy = new PolicyRequestModel
395386
{
396-
Type = policyType,
397387
Enabled = true,
398388
Data = null
399389
},

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

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -24,19 +24,19 @@ public async Task ToSavePolicyModelAsync_WithValidData_ReturnsCorrectSavePolicyM
2424
currentContext.OrganizationOwner(organizationId).Returns(true);
2525

2626
var testData = new Dictionary<string, object> { { "test", "value" } };
27+
var policyType = PolicyType.TwoFactorAuthentication;
2728
var model = new SavePolicyRequest
2829
{
2930
Policy = new PolicyRequestModel
3031
{
31-
Type = PolicyType.TwoFactorAuthentication,
3232
Enabled = true,
3333
Data = testData
3434
},
3535
Metadata = new Dictionary<string, object>()
3636
};
3737

3838
// Act
39-
var result = await model.ToSavePolicyModelAsync(organizationId, currentContext);
39+
var result = await model.ToSavePolicyModelAsync(organizationId, policyType, currentContext);
4040

4141
// Assert
4242
Assert.Equal(PolicyType.TwoFactorAuthentication, result.PolicyUpdate.Type);
@@ -63,17 +63,17 @@ public async Task ToSavePolicyModelAsync_WithEmptyData_HandlesCorrectly(
6363
currentContext.UserId.Returns(userId);
6464
currentContext.OrganizationOwner(organizationId).Returns(false);
6565

66+
var policyType = PolicyType.SingleOrg;
6667
var model = new SavePolicyRequest
6768
{
6869
Policy = new PolicyRequestModel
6970
{
70-
Type = PolicyType.SingleOrg,
7171
Enabled = false
7272
}
7373
};
7474

7575
// Act
76-
var result = await model.ToSavePolicyModelAsync(organizationId, currentContext);
76+
var result = await model.ToSavePolicyModelAsync(organizationId, policyType, currentContext);
7777

7878
// Assert
7979
Assert.Null(result.PolicyUpdate.Data);
@@ -93,17 +93,17 @@ public async Task ToSavePolicyModelAsync_WithNonOrganizationOwner_HandlesCorrect
9393
currentContext.UserId.Returns(userId);
9494
currentContext.OrganizationOwner(organizationId).Returns(true);
9595

96+
var policyType = PolicyType.SingleOrg;
9697
var model = new SavePolicyRequest
9798
{
9899
Policy = new PolicyRequestModel
99100
{
100-
Type = PolicyType.SingleOrg,
101101
Enabled = false
102102
}
103103
};
104104

105105
// Act
106-
var result = await model.ToSavePolicyModelAsync(organizationId, currentContext);
106+
var result = await model.ToSavePolicyModelAsync(organizationId, policyType, currentContext);
107107

108108
// Assert
109109
Assert.Null(result.PolicyUpdate.Data);
@@ -124,11 +124,11 @@ public async Task ToSavePolicyModelAsync_OrganizationDataOwnership_WithValidMeta
124124
currentContext.UserId.Returns(userId);
125125
currentContext.OrganizationOwner(organizationId).Returns(true);
126126

127+
var policyType = PolicyType.OrganizationDataOwnership;
127128
var model = new SavePolicyRequest
128129
{
129130
Policy = new PolicyRequestModel
130131
{
131-
Type = PolicyType.OrganizationDataOwnership,
132132
Enabled = true
133133
},
134134
Metadata = new Dictionary<string, object>
@@ -138,7 +138,7 @@ public async Task ToSavePolicyModelAsync_OrganizationDataOwnership_WithValidMeta
138138
};
139139

140140
// Act
141-
var result = await model.ToSavePolicyModelAsync(organizationId, currentContext);
141+
var result = await model.ToSavePolicyModelAsync(organizationId, policyType, currentContext);
142142

143143
// Assert
144144
Assert.IsType<OrganizationModelOwnershipPolicyModel>(result.Metadata);
@@ -156,17 +156,17 @@ public async Task ToSavePolicyModelAsync_OrganizationDataOwnership_WithEmptyMeta
156156
currentContext.UserId.Returns(userId);
157157
currentContext.OrganizationOwner(organizationId).Returns(true);
158158

159+
var policyType = PolicyType.OrganizationDataOwnership;
159160
var model = new SavePolicyRequest
160161
{
161162
Policy = new PolicyRequestModel
162163
{
163-
Type = PolicyType.OrganizationDataOwnership,
164164
Enabled = true
165165
}
166166
};
167167

168168
// Act
169-
var result = await model.ToSavePolicyModelAsync(organizationId, currentContext);
169+
var result = await model.ToSavePolicyModelAsync(organizationId, policyType, currentContext);
170170

171171
// Assert
172172
Assert.NotNull(result);
@@ -193,20 +193,19 @@ public async Task ToSavePolicyModelAsync_ComplexData_SerializesCorrectly(
193193
currentContext.UserId.Returns(userId);
194194
currentContext.OrganizationOwner(organizationId).Returns(true);
195195

196-
196+
var policyType = PolicyType.ResetPassword;
197197
var model = new SavePolicyRequest
198198
{
199199
Policy = new PolicyRequestModel
200200
{
201-
Type = PolicyType.ResetPassword,
202201
Enabled = true,
203202
Data = _complexData
204203
},
205204
Metadata = new Dictionary<string, object>()
206205
};
207206

208207
// Act
209-
var result = await model.ToSavePolicyModelAsync(organizationId, currentContext);
208+
var result = await model.ToSavePolicyModelAsync(organizationId, policyType, currentContext);
210209

211210
// Assert
212211
var deserializedData = JsonSerializer.Deserialize<Dictionary<string, JsonElement>>(result.PolicyUpdate.Data);
@@ -234,11 +233,11 @@ public async Task MapToPolicyMetadata_UnknownPolicyType_ReturnsEmptyMetadata(
234233
currentContext.UserId.Returns(userId);
235234
currentContext.OrganizationOwner(organizationId).Returns(true);
236235

236+
var policyType = PolicyType.MaximumVaultTimeout;
237237
var model = new SavePolicyRequest
238238
{
239239
Policy = new PolicyRequestModel
240240
{
241-
Type = PolicyType.MaximumVaultTimeout,
242241
Enabled = true
243242
},
244243
Metadata = new Dictionary<string, object>
@@ -248,7 +247,7 @@ public async Task MapToPolicyMetadata_UnknownPolicyType_ReturnsEmptyMetadata(
248247
};
249248

250249
// Act
251-
var result = await model.ToSavePolicyModelAsync(organizationId, currentContext);
250+
var result = await model.ToSavePolicyModelAsync(organizationId, policyType, currentContext);
252251

253252
// Assert
254253
Assert.NotNull(result);
@@ -266,19 +265,18 @@ public async Task MapToPolicyMetadata_JsonSerializationException_ReturnsEmptyMet
266265
currentContext.OrganizationOwner(organizationId).Returns(true);
267266

268267
var errorDictionary = BuildErrorDictionary();
269-
268+
var policyType = PolicyType.OrganizationDataOwnership;
270269
var model = new SavePolicyRequest
271270
{
272271
Policy = new PolicyRequestModel
273272
{
274-
Type = PolicyType.OrganizationDataOwnership,
275273
Enabled = true
276274
},
277275
Metadata = errorDictionary
278276
};
279277

280278
// Act
281-
var result = await model.ToSavePolicyModelAsync(organizationId, currentContext);
279+
var result = await model.ToSavePolicyModelAsync(organizationId, policyType, currentContext);
282280

283281
// Assert
284282
Assert.NotNull(result);

‎test/Api.Test/Controllers/PoliciesControllerTests.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -487,14 +487,14 @@ public async Task PutVNext_WhenPolicyValidatorsRefactorEnabled_UsesVNextSavePoli
487487
.Returns(policy);
488488

489489
// Act
490-
var result = await sutProvider.Sut.PutVNext(orgId, model);
490+
var result = await sutProvider.Sut.PutVNext(orgId, policy.Type, model);
491491

492492
// Assert
493493
await sutProvider.GetDependency<IVNextSavePolicyCommand>()
494494
.Received(1)
495495
.SaveAsync(Arg.Is<SavePolicyModel>(
496496
m => m.PolicyUpdate.OrganizationId == orgId &&
497-
m.PolicyUpdate.Type == model.Policy.Type &&
497+
m.PolicyUpdate.Type == policy.Type &&
498498
m.PolicyUpdate.Enabled == model.Policy.Enabled &&
499499
m.PerformedBy.UserId == userId &&
500500
m.PerformedBy.IsOrganizationOwnerOrProvider == true));
@@ -534,14 +534,14 @@ public async Task PutVNext_WhenPolicyValidatorsRefactorDisabled_UsesSavePolicyCo
534534
.Returns(policy);
535535

536536
// Act
537-
var result = await sutProvider.Sut.PutVNext(orgId, model);
537+
var result = await sutProvider.Sut.PutVNext(orgId, policy.Type, model);
538538

539539
// Assert
540540
await sutProvider.GetDependency<ISavePolicyCommand>()
541541
.Received(1)
542542
.VNextSaveAsync(Arg.Is<SavePolicyModel>(
543543
m => m.PolicyUpdate.OrganizationId == orgId &&
544-
m.PolicyUpdate.Type == model.Policy.Type &&
544+
m.PolicyUpdate.Type == policy.Type &&
545545
m.PolicyUpdate.Enabled == model.Policy.Enabled &&
546546
m.PerformedBy.UserId == userId &&
547547
m.PerformedBy.IsOrganizationOwnerOrProvider == true));

0 commit comments

Comments
 (0)