Make access rule conditions evaluate themselves

Move per-condition evaluation onto each AccessCondition via an abstract
Evaluate(AccessSignals). The engine is now only the combiner: it folds the
per-condition results (deny > approval > allow, empty allows, null entry fails
closed) and no longer knows how any kind decides.

Validate conditions through an exhaustive IAccessConditionVisitor rather than a
type switch, so adding a kind fails to compile until validation handles it.
UnsupportedCondition consequently only guards a null entry now — an unknown kind
is rejected at the JSON layer and can't reach evaluation.

Also document each condition's JSON wire format.

Author: Hinton

bitwarden_license/src/Services/Pam/Engine/AccessEvaluation.cs

@@ -30,7 +30,11 @@ public enum DenyReason
     /// <summary>The timezone was unknown/invalid, or the instant fell outside every configured window.</summary>
     NotWithinTimeWindow,
 
-    /// <summary>The engine did not recognize the condition kind, so it could not be shown satisfied (fail closed).</summary>
+    /// <summary>
+    /// A condition entry could not be evaluated — in practice a null entry from a malformed stored document — so it
+    /// fails closed. A genuinely unknown <c>kind</c> cannot reach here: the JSON layer rejects unknown kinds and the
+    /// visitor dispatch is exhaustive at compile time.
+    /// </summary>
     UnsupportedCondition,
 }
 

bitwarden_license/src/Services/Pam/Engine/AccessRuleEngine.cs

@@ -1,82 +1,19 @@
-using System.Globalization;
-using System.Net;
-using Bit.Services.Pam.Models.Conditions;
+using Bit.Services.Pam.Models.Conditions;
 
 namespace Bit.Services.Pam.Engine;
 
 /// <summary>
-/// Evaluates the access rule's flat list of <see cref="AccessCondition"/>s against the caller's signals. Each
-/// condition yields an <see cref="AccessEvaluation"/>; the results combine with deny taking precedence over a
-/// pending approval, which in turn takes precedence over allow. An empty list is vacuously satisfied (allow).
-/// Unparseable inputs fail closed before they reach the engine.
+/// Combines the results of an access rule's flat list of <see cref="AccessCondition"/>s into one decision. Each
+/// condition evaluates itself (<see cref="AccessCondition.Evaluate"/>); the engine only folds those results, with
+/// deny taking precedence over a pending approval, which in turn takes precedence over allow. An empty list is
+/// vacuously satisfied (allow). Unparseable inputs fail closed before they reach the engine.
 /// </summary>
 public sealed class AccessRuleEngine : IAccessRuleEngine
 {
     public AccessEvaluation Evaluate(IReadOnlyList<AccessCondition> conditions, AccessSignals signals) =>
-        AccessEvaluation.Combine(conditions.Select(condition => EvaluateCondition(condition, signals)));
+        AccessEvaluation.Combine(conditions.Select(condition => EvaluateOne(condition, signals)));
 
-    private static AccessEvaluation EvaluateCondition(AccessCondition condition, AccessSignals signals) => condition switch
-    {
-        HumanApprovalCondition => AccessEvaluation.RequiresApproval,
-        IpAllowlistCondition ip => EvaluateIpAllowlist(ip, signals),
-        TimeOfDayCondition time => EvaluateTimeOfDay(time, signals),
-        // A condition kind the engine does not understand cannot be shown to be satisfied, so deny.
-        _ => AccessEvaluation.Deny(DenyReason.UnsupportedCondition),
-    };
-
-    private static AccessEvaluation EvaluateIpAllowlist(IpAllowlistCondition condition, AccessSignals signals)
-    {
-        // An allowlist with no entries permits no address; combined with an unknown caller IP, both fail closed.
-        if (condition.Cidrs.Count == 0 || signals.IpAddress is null)
-        {
-            return AccessEvaluation.Deny(DenyReason.NotWithinIpRange);
-        }
-
-        foreach (var cidr in condition.Cidrs)
-        {
-            if (IPNetwork.TryParse(cidr, out var network) && network.Contains(signals.IpAddress))
-            {
-                return AccessEvaluation.Allow;
-            }
-        }
-
-        return AccessEvaluation.Deny(DenyReason.NotWithinIpRange);
-    }
-
-    private static AccessEvaluation EvaluateTimeOfDay(TimeOfDayCondition condition, AccessSignals signals)
-    {
-        if (!TimeZoneInfo.TryFindSystemTimeZoneById(condition.Tz, out var timeZone))
-        {
-            // The window cannot be evaluated without a valid timezone, so fail closed.
-            return AccessEvaluation.Deny(DenyReason.NotWithinTimeWindow);
-        }
-
-        var local = TimeZoneInfo.ConvertTime(signals.Timestamp, timeZone);
-        var day = local.DayOfWeek;
-        var time = TimeOnly.FromTimeSpan(local.TimeOfDay);
-
-        foreach (var window in condition.Windows)
-        {
-            if (WindowContains(window, day, time))
-            {
-                return AccessEvaluation.Allow;
-            }
-        }
-
-        return AccessEvaluation.Deny(DenyReason.NotWithinTimeWindow);
-    }
-
-    private static bool WindowContains(TimeWindow window, DayOfWeek day, TimeOnly time)
-    {
-        // AccessWeekday values align with System.DayOfWeek (Sunday = 0), so a direct cast compares correctly.
-        var dayMatches = window.Days.Any(d => (DayOfWeek)d == day);
-        if (!dayMatches)
-        {
-            return false;
-        }
-
-        return TimeOnly.TryParseExact(window.From, "HH:mm", CultureInfo.InvariantCulture, DateTimeStyles.None, out var from)
-            && TimeOnly.TryParseExact(window.To, "HH:mm", CultureInfo.InvariantCulture, DateTimeStyles.None, out var to)
-            && time >= from && time <= to;
-    }
+    private static AccessEvaluation EvaluateOne(AccessCondition? condition, AccessSignals signals) =>
+        // A null entry (malformed stored conditions) cannot be evaluated, so fail closed.
+        condition is null ? AccessEvaluation.Deny(DenyReason.UnsupportedCondition) : condition.Evaluate(signals);
 }

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

@@ -1,4 +1,5 @@
 using System.Text.Json.Serialization;
+using Bit.Services.Pam.Engine;
 
 namespace Bit.Services.Pam.Models.Conditions;
 
@@ -10,4 +11,19 @@ namespace Bit.Services.Pam.Models.Conditions;
 [JsonDerivedType(typeof(HumanApprovalCondition), "human_approval")]
 [JsonDerivedType(typeof(IpAllowlistCondition), "ip_allowlist")]
 [JsonDerivedType(typeof(TimeOfDayCondition), "time_of_day")]
-public abstract class AccessCondition;
+public abstract class AccessCondition
+{
+    /// <summary>
+    /// Evaluates this condition against the request-time <paramref name="signals"/>, returning whether it allows,
+    /// denies, or requires human approval. The engine folds each condition's result into the rule's overall
+    /// decision; it does not know how any individual kind decides.
+    /// </summary>
+    public abstract AccessEvaluation Evaluate(AccessSignals signals);
+
+    /// <summary>
+    /// Double-dispatches to the <paramref name="visitor"/>'s method for this condition's concrete kind, used by the
+    /// validator to check each kind is well-formed. The compiler requires the visitor to handle every kind, so a
+    /// newly added condition can't be silently skipped by validation.
+    /// </summary>
+    public abstract T Accept<T>(IAccessConditionVisitor<T> visitor);
+}

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

@@ -1,6 +1,14 @@
-namespace Bit.Services.Pam.Models.Conditions;
+using Bit.Services.Pam.Engine;
+
+namespace Bit.Services.Pam.Models.Conditions;
 
 /// <summary>
 /// Always requires a human decision before a lease can be issued.
 /// </summary>
-public sealed class HumanApprovalCondition : AccessCondition;
+/// <remarks>Wire format: <c>{ "kind": "human_approval" }</c></remarks>
+public sealed class HumanApprovalCondition : AccessCondition
+{
+    public override AccessEvaluation Evaluate(AccessSignals signals) => AccessEvaluation.RequiresApproval;
+
+    public override T Accept<T>(IAccessConditionVisitor<T> visitor) => visitor.VisitHumanApproval(this);
+}

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

@@ -0,0 +1,15 @@
+namespace Bit.Services.Pam.Models.Conditions;
+
+/// <summary>
+/// A type-safe operation over the closed set of <see cref="AccessCondition"/> kinds, dispatched via
+/// <see cref="AccessCondition.Accept{T}"/>. Implemented once per operation — the engine evaluates a condition, the
+/// validator checks it is well-formed — with <typeparamref name="T"/> as that operation's result. Because the
+/// interface has one method per kind, the compiler forces every implementation to handle every kind, so a newly
+/// added condition can never be silently skipped by evaluation or validation.
+/// </summary>
+public interface IAccessConditionVisitor<out T>
+{
+    T VisitHumanApproval(HumanApprovalCondition condition);
+    T VisitIpAllowlist(IpAllowlistCondition condition);
+    T VisitTimeOfDay(TimeOfDayCondition condition);
+}

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

@@ -1,13 +1,43 @@
-namespace Bit.Services.Pam.Models.Conditions;
+using System.Net;
+using Bit.Services.Pam.Engine;
+
+namespace Bit.Services.Pam.Models.Conditions;
 
 /// <summary>
 /// Auto-approves a lease when the requester's IP matches a listed CIDR; otherwise denies.
 /// </summary>
+/// <remarks>
+/// Wire format:
+/// <code>
+/// { "kind": "ip_allowlist", "cidrs": ["10.0.0.0/8", "2001:db8::/32"] }
+/// </code>
+/// </remarks>
 public sealed class IpAllowlistCondition : AccessCondition
 {
     /// <summary>
     /// The allowed source ranges in CIDR notation (e.g. <c>"10.0.0.0/8"</c>). The condition allows when the caller's
     /// IP is in any one of them. At least one required, and each must parse; an empty list denies.
     /// </summary>
     public IReadOnlyList<string> Cidrs { get; init; } = [];
+
+    public override AccessEvaluation Evaluate(AccessSignals signals)
+    {
+        // An allowlist with no entries permits no address; combined with an unknown caller IP, both fail closed.
+        if (Cidrs.Count == 0 || signals.IpAddress is null)
+        {
+            return AccessEvaluation.Deny(DenyReason.NotWithinIpRange);
+        }
+
+        foreach (var cidr in Cidrs)
+        {
+            if (IPNetwork.TryParse(cidr, out var network) && network.Contains(signals.IpAddress))
+            {
+                return AccessEvaluation.Allow;
+            }
+        }
+
+        return AccessEvaluation.Deny(DenyReason.NotWithinIpRange);
+    }
+
+    public override T Accept<T>(IAccessConditionVisitor<T> visitor) => visitor.VisitIpAllowlist(this);
 }

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

@@ -1,17 +1,47 @@
-using Bit.Services.Pam.Enums;
+using System.Globalization;
+using Bit.Services.Pam.Engine;
+using Bit.Services.Pam.Enums;
+
 namespace Bit.Services.Pam.Models.Conditions;
 
 /// <summary>
 /// Auto-approves a lease when the request falls inside one of the configured windows, evaluated in
 /// the named IANA timezone; otherwise denies.
 /// </summary>
+/// <remarks>
+/// Wire format:
+/// <code>
+/// {
+///   "kind": "time_of_day",
+///   "tz": "America/New_York",
+///   "windows": [{ "days": ["mon", "tue", "wed", "thu", "fri"], "from": "09:00", "to": "17:00" }]
+/// }
+/// </code>
+/// </remarks>
 public sealed class TimeOfDayCondition : AccessCondition
 {
     /// <summary>The IANA timezone (e.g. <c>"America/New_York"</c>) the windows are evaluated in. An unknown or invalid zone denies.</summary>
     public string Tz { get; init; } = string.Empty;
 
     /// <summary>The windows that grant access; the condition allows when the instant falls in any one of them. At least one required.</summary>
     public IReadOnlyList<TimeWindow> Windows { get; init; } = [];
+
+    public override AccessEvaluation Evaluate(AccessSignals signals)
+    {
+        if (!TimeZoneInfo.TryFindSystemTimeZoneById(Tz, out var timeZone))
+        {
+            // The window cannot be evaluated without a valid timezone, so fail closed.
+            return AccessEvaluation.Deny(DenyReason.NotWithinTimeWindow);
+        }
+
+        var local = TimeZoneInfo.ConvertTime(signals.Timestamp, timeZone);
+        var day = local.DayOfWeek;
+        var time = TimeOnly.FromTimeSpan(local.TimeOfDay);
+
+        return Windows.Any(window => window.Contains(day, time)) ? AccessEvaluation.Allow : AccessEvaluation.Deny(DenyReason.NotWithinTimeWindow);
+    }
+
+    public override T Accept<T>(IAccessConditionVisitor<T> visitor) => visitor.VisitTimeOfDay(this);
 }
 
 /// <summary>
@@ -28,4 +58,21 @@ public sealed class TimeWindow
 
     /// <summary>Window end, 24-hour <c>HH:mm</c> (<c>00:00</c>–<c>23:59</c>). Inclusive.</summary>
     public string To { get; init; } = string.Empty;
+
+    /// <summary>
+    /// Whether this window admits the given <paramref name="day"/> and <paramref name="time"/> (both already in the
+    /// parent condition's timezone). Unparseable bounds admit nothing, so the window fails closed.
+    /// </summary>
+    public bool Contains(DayOfWeek day, TimeOnly time)
+    {
+        // AccessWeekday values align with System.DayOfWeek (Sunday = 0), so a direct cast compares correctly.
+        if (Days.All(d => (DayOfWeek)d != day))
+        {
+            return false;
+        }
+
+        return TimeOnly.TryParseExact(From, "HH:mm", CultureInfo.InvariantCulture, DateTimeStyles.None, out var from)
+            && TimeOnly.TryParseExact(To, "HH:mm", CultureInfo.InvariantCulture, DateTimeStyles.None, out var to)
+            && time >= from && time <= to;
+    }
 }