Skip to content

Commit 253c6ec

Browse files
HintonHinton
committed
Move condition validation onto the conditions
Give each AccessCondition a Validate() alongside Evaluate() and remove the IAccessConditionVisitor. AccessRuleValidator now delegates per-condition checks to condition.Validate() and only enforces the document-level shape (array, size limit, null-entry guard). TimeWindow gains its own Validate() plus a shared TryParseTime, so Contains and validation use one HH:mm parse rather than a regex that could drift from evaluation. AccessRuleValidationResult moves into the conditions namespace so the condition models do not depend on the service layer.
1 parent d4acfe4 commit 253c6ec

11 files changed

Lines changed: 103 additions & 119 deletions

‎bitwarden_license/src/Services/Pam/Models/Conditions/AccessCondition.cs‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,8 @@ public abstract class AccessCondition
2121
public abstract AccessEvaluation Evaluate(AccessSignals signals);
2222

2323
/// <summary>
24-
/// Double-dispatches to the <paramref name="visitor"/>'s method for this condition's concrete kind, used by the
25-
/// validator to check each kind is well-formed. The compiler requires the visitor to handle every kind, so a
26-
/// newly added condition can't be silently skipped by validation.
24+
/// Checks this condition is well-formed at write time, before it is persisted, returning an actionable error
25+
/// when it is not. The loud, reject-on-save counterpart to <see cref="Evaluate"/>'s fail-closed runtime behavior.
2726
/// </summary>
28-
public abstract T Accept<T>(IAccessConditionVisitor<T> visitor);
27+
public abstract AccessRuleValidationResult Validate();
2928
}

‎bitwarden_license/src/Services/Pam/Models/Conditions/AccessRuleValidationResult.cs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
namespace Bit.Services.Pam.Models.Conditions;
2+
3+
/// <summary>
4+
/// The result of checking a condition (or a rule's whole conditions document) is well-formed: whether it is valid
5+
/// and, when not, an actionable message. Produced at write time by <see cref="AccessCondition.Validate"/> and by
6+
/// <see cref="Bit.Services.Pam.Services.IAccessRuleValidator"/>.
7+
/// </summary>
8+
public sealed record AccessRuleValidationResult(bool IsValid, string? Error)
9+
{
10+
public static AccessRuleValidationResult Valid { get; } = new(true, null);
11+
public static AccessRuleValidationResult Invalid(string error) => new(false, error);
12+
}

‎bitwarden_license/src/Services/Pam/Models/Conditions/HumanApprovalCondition.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,5 +10,5 @@ public sealed class HumanApprovalCondition : AccessCondition
1010
{
1111
public override AccessEvaluation Evaluate(AccessSignals signals) => AccessEvaluation.RequiresApproval;
1212

13-
public override T Accept<T>(IAccessConditionVisitor<T> visitor) => visitor.VisitHumanApproval(this);
13+
public override AccessRuleValidationResult Validate() => AccessRuleValidationResult.Valid;
1414
}

‎bitwarden_license/src/Services/Pam/Models/Conditions/IAccessConditionVisitor.cs‎

Lines changed: 0 additions & 15 deletions
This file was deleted.

‎bitwarden_license/src/Services/Pam/Models/Conditions/IpAllowlistCondition.cs‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,5 +39,21 @@ public override AccessEvaluation Evaluate(AccessSignals signals)
3939
return AccessEvaluation.Deny(DenyReason.NotWithinIpRange);
4040
}
4141

42-
public override T Accept<T>(IAccessConditionVisitor<T> visitor) => visitor.VisitIpAllowlist(this);
42+
public override AccessRuleValidationResult Validate()
43+
{
44+
if (Cidrs.Count == 0)
45+
{
46+
return AccessRuleValidationResult.Invalid("ip_allowlist requires at least one CIDR.");
47+
}
48+
49+
foreach (var cidr in Cidrs)
50+
{
51+
if (string.IsNullOrWhiteSpace(cidr) || !IPNetwork.TryParse(cidr, out _))
52+
{
53+
return AccessRuleValidationResult.Invalid($"Invalid CIDR: '{cidr}'.");
54+
}
55+
}
56+
57+
return AccessRuleValidationResult.Valid;
58+
}
4359
}

‎bitwarden_license/src/Services/Pam/Models/Conditions/TimeOfDayCondition.cs‎

Lines changed: 58 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,34 @@ public override AccessEvaluation Evaluate(AccessSignals signals)
4141
return Windows.Any(window => window.Contains(day, time)) ? AccessEvaluation.Allow : AccessEvaluation.Deny(DenyReason.NotWithinTimeWindow);
4242
}
4343

44-
public override T Accept<T>(IAccessConditionVisitor<T> visitor) => visitor.VisitTimeOfDay(this);
44+
public override AccessRuleValidationResult Validate()
45+
{
46+
if (string.IsNullOrWhiteSpace(Tz))
47+
{
48+
return AccessRuleValidationResult.Invalid("time_of_day requires a tz.");
49+
}
50+
51+
try
52+
{
53+
TimeZoneInfo.FindSystemTimeZoneById(Tz);
54+
}
55+
catch (TimeZoneNotFoundException)
56+
{
57+
return AccessRuleValidationResult.Invalid($"Unknown timezone: '{Tz}'.");
58+
}
59+
catch (InvalidTimeZoneException)
60+
{
61+
return AccessRuleValidationResult.Invalid($"Invalid timezone: '{Tz}'.");
62+
}
63+
64+
if (Windows.Count == 0)
65+
{
66+
return AccessRuleValidationResult.Invalid("time_of_day requires at least one window.");
67+
}
68+
69+
return Windows.Select(window => window.Validate()).FirstOrDefault(result => !result.IsValid)
70+
?? AccessRuleValidationResult.Valid;
71+
}
4572
}
4673

4774
/// <summary>
@@ -71,8 +98,36 @@ public bool Contains(DayOfWeek day, TimeOnly time)
7198
return false;
7299
}
73100

74-
return TimeOnly.TryParseExact(From, "HH:mm", CultureInfo.InvariantCulture, DateTimeStyles.None, out var from)
75-
&& TimeOnly.TryParseExact(To, "HH:mm", CultureInfo.InvariantCulture, DateTimeStyles.None, out var to)
101+
return TryParseTime(From, out var from)
102+
&& TryParseTime(To, out var to)
76103
&& time >= from && time <= to;
77104
}
105+
106+
/// <summary>
107+
/// Checks the window is well-formed: at least one day, and start/end times in 24-hour <c>HH:mm</c>. Uses the
108+
/// same time parse as <see cref="Contains"/>, so validation and evaluation cannot disagree on the format.
109+
/// </summary>
110+
public AccessRuleValidationResult Validate()
111+
{
112+
if (Days.Count == 0)
113+
{
114+
return AccessRuleValidationResult.Invalid("time_of_day window requires at least one day.");
115+
}
116+
117+
// Day tokens were validated during deserialization by AccessWeekdayJsonConverter; only the times remain.
118+
if (!TryParseTime(From, out _))
119+
{
120+
return AccessRuleValidationResult.Invalid($"Invalid 'from' time: '{From}'. Expected HH:mm.");
121+
}
122+
123+
if (!TryParseTime(To, out _))
124+
{
125+
return AccessRuleValidationResult.Invalid($"Invalid 'to' time: '{To}'. Expected HH:mm.");
126+
}
127+
128+
return AccessRuleValidationResult.Valid;
129+
}
130+
131+
private static bool TryParseTime(string value, out TimeOnly time) =>
132+
TimeOnly.TryParseExact(value, "HH:mm", CultureInfo.InvariantCulture, DateTimeStyles.None, out time);
78133
}

‎bitwarden_license/src/Services/Pam/Services/AccessRuleValidator.cs‎

Lines changed: 6 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,12 @@
1-
using System.Net;
2-
using System.Text.Json;
3-
using System.Text.RegularExpressions;
1+
using System.Text.Json;
42
using Bit.Services.Pam.Models.Conditions;
53

64
namespace Bit.Services.Pam.Services;
75

8-
public sealed partial class AccessRuleValidator : IAccessRuleValidator
6+
public sealed class AccessRuleValidator : IAccessRuleValidator
97
{
108
private const int MaxConditions = 10;
119

12-
// Stateless, so one shared instance serves every call.
13-
private static readonly ConditionValidator _conditionValidator = new();
14-
15-
[GeneratedRegex(@"^([01][0-9]|2[0-3]):[0-5][0-9]$")]
16-
private static partial Regex TimeOfDayRegex();
17-
1810
public AccessRuleValidationResult Validate(string? conditionsJson)
1911
{
2012
if (conditionsJson is null)
@@ -50,88 +42,15 @@ public AccessRuleValidationResult Validate(string? conditionsJson)
5042
return AccessRuleValidationResult.Invalid($"Conditions cannot contain more than {MaxConditions} conditions.");
5143
}
5244

45+
// Each condition validates itself; the validator only enforces the document-level shape (it is an array,
46+
// within the size limit) and guards the one thing a condition cannot check for itself: a null entry.
5347
return conditions.Select(ValidateCondition).FirstOrDefault(result => !result.IsValid)
5448
?? AccessRuleValidationResult.Valid;
5549
}
5650

5751
private static AccessRuleValidationResult ValidateCondition(AccessCondition? condition) =>
52+
// A JSON null in the array is not a condition and cannot validate itself, so it is rejected here.
5853
condition is null
5954
? AccessRuleValidationResult.Invalid("Conditions cannot contain a null entry.")
60-
: condition.Accept(_conditionValidator);
61-
62-
/// <summary>
63-
/// Checks a single condition is well-formed. Stateless, mirroring the engine's evaluator: neither public
64-
/// service is itself a visitor; each delegates to a private one.
65-
/// </summary>
66-
private sealed class ConditionValidator : IAccessConditionVisitor<AccessRuleValidationResult>
67-
{
68-
public AccessRuleValidationResult VisitHumanApproval(HumanApprovalCondition condition) =>
69-
AccessRuleValidationResult.Valid;
70-
71-
public AccessRuleValidationResult VisitIpAllowlist(IpAllowlistCondition condition)
72-
{
73-
if (condition.Cidrs.Count == 0)
74-
{
75-
return AccessRuleValidationResult.Invalid("ip_allowlist requires at least one CIDR.");
76-
}
77-
78-
foreach (var cidr in condition.Cidrs)
79-
{
80-
if (string.IsNullOrWhiteSpace(cidr) || !IPNetwork.TryParse(cidr, out _))
81-
{
82-
return AccessRuleValidationResult.Invalid($"Invalid CIDR: '{cidr}'.");
83-
}
84-
}
85-
86-
return AccessRuleValidationResult.Valid;
87-
}
88-
89-
public AccessRuleValidationResult VisitTimeOfDay(TimeOfDayCondition condition)
90-
{
91-
if (string.IsNullOrWhiteSpace(condition.Tz))
92-
{
93-
return AccessRuleValidationResult.Invalid("time_of_day requires a tz.");
94-
}
95-
96-
try
97-
{
98-
TimeZoneInfo.FindSystemTimeZoneById(condition.Tz);
99-
}
100-
catch (TimeZoneNotFoundException)
101-
{
102-
return AccessRuleValidationResult.Invalid($"Unknown timezone: '{condition.Tz}'.");
103-
}
104-
catch (InvalidTimeZoneException)
105-
{
106-
return AccessRuleValidationResult.Invalid($"Invalid timezone: '{condition.Tz}'.");
107-
}
108-
109-
if (condition.Windows.Count == 0)
110-
{
111-
return AccessRuleValidationResult.Invalid("time_of_day requires at least one window.");
112-
}
113-
114-
foreach (var window in condition.Windows)
115-
{
116-
if (window.Days.Count == 0)
117-
{
118-
return AccessRuleValidationResult.Invalid("time_of_day window requires at least one day.");
119-
}
120-
121-
// Day tokens are validated during deserialization by AccessWeekdayJsonConverter; an unknown token
122-
// fails the JSON parse above and is reported as malformed.
123-
if (!TimeOfDayRegex().IsMatch(window.From))
124-
{
125-
return AccessRuleValidationResult.Invalid($"Invalid 'from' time: '{window.From}'. Expected HH:mm.");
126-
}
127-
128-
if (!TimeOfDayRegex().IsMatch(window.To))
129-
{
130-
return AccessRuleValidationResult.Invalid($"Invalid 'to' time: '{window.To}'. Expected HH:mm.");
131-
}
132-
}
133-
134-
return AccessRuleValidationResult.Valid;
135-
}
136-
}
55+
: condition.Validate();
13756
}

‎bitwarden_license/src/Services/Pam/Services/IAccessRuleValidator.cs‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
1-
namespace Bit.Services.Pam.Services;
1+
using Bit.Services.Pam.Models.Conditions;
2+
3+
namespace Bit.Services.Pam.Services;
24

35
public interface IAccessRuleValidator
46
{
@@ -8,9 +10,3 @@ public interface IAccessRuleValidator
810
/// </summary>
911
AccessRuleValidationResult Validate(string? conditionsJson);
1012
}
11-
12-
public sealed record AccessRuleValidationResult(bool IsValid, string? Error)
13-
{
14-
public static AccessRuleValidationResult Valid { get; } = new(true, null);
15-
public static AccessRuleValidationResult Invalid(string error) => new(false, error);
16-
}

‎bitwarden_license/test/Services/Pam.Test/Commands/CreateAccessRuleCommandTests.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
using Bit.Core.Repositories;
44
using Bit.Pam.Entities;
55
using Bit.Pam.Repositories;
6+
using Bit.Services.Pam.Models.Conditions;
67
using Bit.Services.Pam.OrganizationFeatures.Commands;
78
using Bit.Services.Pam.Services;
89
using Bit.Test.Common.AutoFixture;

‎bitwarden_license/test/Services/Pam.Test/Commands/UpdateAccessRuleCommandTests.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
using Bit.Pam.Entities;
55
using Bit.Pam.Models;
66
using Bit.Pam.Repositories;
7+
using Bit.Services.Pam.Models.Conditions;
78
using Bit.Services.Pam.OrganizationFeatures.Commands;
89
using Bit.Services.Pam.Services;
910
using Bit.Test.Common.AutoFixture;

0 commit comments

Comments
 (0)