From 04f66ea578981069d1ee2db1f1569b30ceb93b17 Mon Sep 17 00:00:00 2001 From: Hinton Date: Thu, 16 Jul 2026 14:41:37 +0200 Subject: [PATCH 01/10] Add the PAM access rule evaluation engine Extract the pure rule-evaluation core from the PAM POC: AccessRuleEngine evaluates a flat list of AccessConditions against request-time AccessSignals, combining results as deny > requires-approval > allow and failing closed on any condition, timezone, or IP it cannot interpret. The engine reads no state and issues no leases. Registered as a singleton (pure and stateless). --- .../Services/Pam/Engine/AccessEvaluation.cs | 49 ++++ .../Services/Pam/Engine/AccessRuleEngine.cs | 82 +++++++ .../src/Services/Pam/Engine/AccessSignals.cs | 25 ++ .../Services/Pam/Engine/IAccessRuleEngine.cs | 14 ++ .../Utilities/ServiceCollectionExtensions.cs | 4 + .../Pam.Test/Engine/AccessRuleEngineTests.cs | 218 ++++++++++++++++++ 6 files changed, 392 insertions(+) create mode 100644 bitwarden_license/src/Services/Pam/Engine/AccessEvaluation.cs create mode 100644 bitwarden_license/src/Services/Pam/Engine/AccessRuleEngine.cs create mode 100644 bitwarden_license/src/Services/Pam/Engine/AccessSignals.cs create mode 100644 bitwarden_license/src/Services/Pam/Engine/IAccessRuleEngine.cs create mode 100644 bitwarden_license/test/Services/Pam.Test/Engine/AccessRuleEngineTests.cs diff --git a/bitwarden_license/src/Services/Pam/Engine/AccessEvaluation.cs b/bitwarden_license/src/Services/Pam/Engine/AccessEvaluation.cs new file mode 100644 index 000000000000..0632442e95df --- /dev/null +++ b/bitwarden_license/src/Services/Pam/Engine/AccessEvaluation.cs @@ -0,0 +1,49 @@ +namespace Bit.Services.Pam.Engine; + +public enum AccessEvaluationOutcome +{ + Allow, + RequiresApproval, + Deny, +} + +public enum DenyReason +{ + None = 0, + NotWithinIpRange, + NotWithinTimeWindow, + UnsupportedCondition, +} + +public sealed record AccessEvaluation +{ + public required AccessEvaluationOutcome Outcome { get; init; } + public DenyReason Reason { get; init; } = DenyReason.None; + + public static AccessEvaluation Allow { get; } = new() { Outcome = AccessEvaluationOutcome.Allow }; + public static AccessEvaluation RequiresApproval { get; } = new() { Outcome = AccessEvaluationOutcome.RequiresApproval }; + + public static AccessEvaluation Deny(DenyReason reason) => new() + { + Outcome = AccessEvaluationOutcome.Deny, + Reason = reason + }; + + public static AccessEvaluation Combine(IEnumerable evaluations) + { + var requiresApproval = false; + foreach (var evaluation in evaluations) + { + switch (evaluation.Outcome) + { + case AccessEvaluationOutcome.Deny: + return evaluation; + case AccessEvaluationOutcome.RequiresApproval: + requiresApproval = true; + break; + } + } + + return requiresApproval ? RequiresApproval : Allow; + } +} diff --git a/bitwarden_license/src/Services/Pam/Engine/AccessRuleEngine.cs b/bitwarden_license/src/Services/Pam/Engine/AccessRuleEngine.cs new file mode 100644 index 000000000000..4a1b9ec278f6 --- /dev/null +++ b/bitwarden_license/src/Services/Pam/Engine/AccessRuleEngine.cs @@ -0,0 +1,82 @@ +using System.Globalization; +using System.Net; +using Bit.Services.Pam.Models.Conditions; + +namespace Bit.Services.Pam.Engine; + +/// +/// Evaluates the access rule's flat list of s against the caller's signals. Each +/// condition yields an ; 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. +/// +public sealed class AccessRuleEngine : IAccessRuleEngine +{ + public AccessEvaluation Evaluate(IReadOnlyList conditions, AccessSignals signals) => + AccessEvaluation.Combine(conditions.Select(condition => EvaluateCondition(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; + } +} diff --git a/bitwarden_license/src/Services/Pam/Engine/AccessSignals.cs b/bitwarden_license/src/Services/Pam/Engine/AccessSignals.cs new file mode 100644 index 000000000000..459cb4a9403a --- /dev/null +++ b/bitwarden_license/src/Services/Pam/Engine/AccessSignals.cs @@ -0,0 +1,25 @@ +using System.Net; + +namespace Bit.Services.Pam.Engine; + +/// +/// The request-time inputs an access rule is evaluated against: the caller's source IP and the instant the +/// evaluation is performed. is null when the caller's address cannot be determined, which +/// IP-restricted rules treat as a denial so access never opens up on a missing signal. +/// +public sealed record AccessSignals +{ + public required IPAddress? IpAddress { get; init; } + public required DateTimeOffset Timestamp { get; init; } + + /// + /// Builds the signals for the current request: the caller's source IP (parsed, or null when it is absent or + /// unparseable) and the supplied evaluation . Callers typically pass the request's + /// source address (e.g. ICurrentContext.IpAddress). + /// + public static AccessSignals From(string? ipAddress, DateTimeOffset timestamp) => new() + { + IpAddress = IPAddress.TryParse(ipAddress, out var ip) ? ip : null, + Timestamp = timestamp, + }; +} diff --git a/bitwarden_license/src/Services/Pam/Engine/IAccessRuleEngine.cs b/bitwarden_license/src/Services/Pam/Engine/IAccessRuleEngine.cs new file mode 100644 index 000000000000..b2de653b7f32 --- /dev/null +++ b/bitwarden_license/src/Services/Pam/Engine/IAccessRuleEngine.cs @@ -0,0 +1,14 @@ +using Bit.Services.Pam.Models.Conditions; + +namespace Bit.Services.Pam.Engine; + +/// +/// Evaluates an access rule's conditions — a flat list of ANDed together — against +/// the request-time , deciding whether access is allowed, denied, or gated on human +/// approval. The engine is pure: it reads no state and issues no leases. Lease lifecycle is owned by the lease +/// commands and queries, which call the engine to decide whether a lease may be issued or its data handed over. +/// +public interface IAccessRuleEngine +{ + AccessEvaluation Evaluate(IReadOnlyList conditions, AccessSignals signals); +} diff --git a/bitwarden_license/src/Services/Pam/Utilities/ServiceCollectionExtensions.cs b/bitwarden_license/src/Services/Pam/Utilities/ServiceCollectionExtensions.cs index 8c055f68b44e..63fa5fa3c40b 100644 --- a/bitwarden_license/src/Services/Pam/Utilities/ServiceCollectionExtensions.cs +++ b/bitwarden_license/src/Services/Pam/Utilities/ServiceCollectionExtensions.cs @@ -1,6 +1,7 @@ using Bit.HttpExtensions; using Bit.Services.Pam.Api.Endpoints; using Bit.Services.Pam.Api.Endpoints.Handlers; +using Bit.Services.Pam.Engine; using Bit.Services.Pam.OrganizationFeatures.Commands; using Bit.Services.Pam.OrganizationFeatures.Commands.Interfaces; using Bit.Services.Pam.Services; @@ -17,6 +18,9 @@ public static IServiceCollection AddPamServices(this IServiceCollection services services.AddScoped(); services.AddScoped(); + // Rule evaluation engine. Pure and stateless, so a singleton is safe. + services.AddSingleton(); + // AccessRule write path. services.TryAddSingleton(TimeProvider.System); services.AddSingleton(); diff --git a/bitwarden_license/test/Services/Pam.Test/Engine/AccessRuleEngineTests.cs b/bitwarden_license/test/Services/Pam.Test/Engine/AccessRuleEngineTests.cs new file mode 100644 index 000000000000..ec00c19046a5 --- /dev/null +++ b/bitwarden_license/test/Services/Pam.Test/Engine/AccessRuleEngineTests.cs @@ -0,0 +1,218 @@ +using System.Net; +using Bit.Services.Pam.Engine; +using Bit.Services.Pam.Enums; +using Bit.Services.Pam.Models.Conditions; +using Xunit; + +namespace Bit.Services.Pam.Test.Engine; + +public class AccessRuleEngineTests +{ + // 2026-06-04T12:00:00Z is a Thursday, so "thu" windows match in UTC. + private static readonly DateTimeOffset _now = new(2026, 6, 4, 12, 0, 0, TimeSpan.Zero); + + private readonly AccessRuleEngine _sut = new(); + + private static AccessSignals Signals(IPAddress? ip = null, DateTimeOffset? at = null) => new() + { + IpAddress = ip, + Timestamp = at ?? _now, + }; + + private static AccessCondition[] Set(params AccessCondition[] conditions) => conditions; + + [Fact] + public void Evaluate_HumanApproval_RequiresApproval() + { + var evaluation = _sut.Evaluate(Set(new HumanApprovalCondition()), Signals()); + + Assert.Equal(AccessEvaluationOutcome.RequiresApproval, evaluation.Outcome); + } + + [Fact] + public void Evaluate_IpAllowlist_IpInRange_Allows() + { + var conditions = Set(new IpAllowlistCondition { Cidrs = ["10.0.0.0/8"] }); + + var evaluation = _sut.Evaluate(conditions, Signals(IPAddress.Parse("10.1.2.3"))); + + Assert.Equal(AccessEvaluationOutcome.Allow, evaluation.Outcome); + } + + [Fact] + public void Evaluate_IpAllowlist_IpOutOfRange_Denies() + { + var conditions = Set(new IpAllowlistCondition { Cidrs = ["10.0.0.0/8"] }); + + var evaluation = _sut.Evaluate(conditions, Signals(IPAddress.Parse("192.168.1.1"))); + + Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); + Assert.Equal(DenyReason.NotWithinIpRange, evaluation.Reason); + } + + [Fact] + public void Evaluate_IpAllowlist_UnknownIp_DeniesClosed() + { + var conditions = Set(new IpAllowlistCondition { Cidrs = ["10.0.0.0/8"] }); + + var evaluation = _sut.Evaluate(conditions, Signals(ip: null)); + + Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); + Assert.Equal(DenyReason.NotWithinIpRange, evaluation.Reason); + } + + [Fact] + public void Evaluate_IpAllowlist_NoEntries_DeniesClosed() + { + var evaluation = _sut.Evaluate(Set(new IpAllowlistCondition()), Signals(IPAddress.Parse("10.1.2.3"))); + + Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); + Assert.Equal(DenyReason.NotWithinIpRange, evaluation.Reason); + } + + [Fact] + public void Evaluate_TimeOfDay_WithinWindow_Allows() + { + var conditions = Set(new TimeOfDayCondition + { + Tz = "UTC", + Windows = [new TimeWindow { Days = [AccessWeekday.Thu], From = "09:00", To = "17:00" }], + }); + + var evaluation = _sut.Evaluate(conditions, Signals()); + + Assert.Equal(AccessEvaluationOutcome.Allow, evaluation.Outcome); + } + + [Fact] + public void Evaluate_TimeOfDay_OutsideTimeWindow_Denies() + { + var conditions = Set(new TimeOfDayCondition + { + Tz = "UTC", + Windows = [new TimeWindow { Days = [AccessWeekday.Thu], From = "00:00", To = "06:00" }], + }); + + var evaluation = _sut.Evaluate(conditions, Signals()); + + Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); + Assert.Equal(DenyReason.NotWithinTimeWindow, evaluation.Reason); + } + + [Fact] + public void Evaluate_TimeOfDay_DayNotListed_Denies() + { + var conditions = Set(new TimeOfDayCondition + { + Tz = "UTC", + Windows = [new TimeWindow { Days = [AccessWeekday.Fri], From = "00:00", To = "23:59" }], + }); + + var evaluation = _sut.Evaluate(conditions, Signals()); + + Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); + Assert.Equal(DenyReason.NotWithinTimeWindow, evaluation.Reason); + } + + [Fact] + public void Evaluate_TimeOfDay_EvaluatesInConfiguredTimezone() + { + // 23:00 UTC is 19:00 (Thursday) in America/New_York during June DST, inside the window. + var conditions = Set(new TimeOfDayCondition + { + Tz = "America/New_York", + Windows = [new TimeWindow { Days = [AccessWeekday.Thu], From = "18:00", To = "20:00" }], + }); + + var evaluation = _sut.Evaluate(conditions, Signals(at: new DateTimeOffset(2026, 6, 4, 23, 0, 0, TimeSpan.Zero))); + + Assert.Equal(AccessEvaluationOutcome.Allow, evaluation.Outcome); + } + + [Fact] + public void Evaluate_TimeOfDay_UnknownTimezone_DeniesClosed() + { + var conditions = Set(new TimeOfDayCondition + { + Tz = "Not/AZone", + Windows = [new TimeWindow { Days = [AccessWeekday.Thu], From = "00:00", To = "23:59" }], + }); + + var evaluation = _sut.Evaluate(conditions, Signals()); + + Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); + Assert.Equal(DenyReason.NotWithinTimeWindow, evaluation.Reason); + } + + [Fact] + public void Evaluate_AllConditionsAllow_Allows() + { + var conditions = Set( + new IpAllowlistCondition { Cidrs = ["10.0.0.0/8"] }, + new TimeOfDayCondition { Tz = "UTC", Windows = [new TimeWindow { Days = [AccessWeekday.Thu], From = "09:00", To = "17:00" }] }); + + var evaluation = _sut.Evaluate(conditions, Signals(IPAddress.Parse("10.1.2.3"))); + + Assert.Equal(AccessEvaluationOutcome.Allow, evaluation.Outcome); + } + + [Fact] + public void Evaluate_OneConditionDenies_DeniesWithThatReason() + { + var conditions = Set( + new IpAllowlistCondition { Cidrs = ["10.0.0.0/8"] }, + new TimeOfDayCondition { Tz = "UTC", Windows = [new TimeWindow { Days = [AccessWeekday.Thu], From = "00:00", To = "06:00" }] }); + + var evaluation = _sut.Evaluate(conditions, Signals(IPAddress.Parse("10.1.2.3"))); + + Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); + Assert.Equal(DenyReason.NotWithinTimeWindow, evaluation.Reason); + } + + [Fact] + public void Evaluate_AllowPlusHumanApproval_RequiresApproval() + { + var conditions = Set( + new IpAllowlistCondition { Cidrs = ["10.0.0.0/8"] }, + new HumanApprovalCondition()); + + var evaluation = _sut.Evaluate(conditions, Signals(IPAddress.Parse("10.1.2.3"))); + + Assert.Equal(AccessEvaluationOutcome.RequiresApproval, evaluation.Outcome); + } + + [Fact] + public void Evaluate_DenyOutranksApproval() + { + // A denying condition beats a pending approval: there is nothing to approve if access is barred outright. + var conditions = Set( + new HumanApprovalCondition(), + new IpAllowlistCondition { Cidrs = ["10.0.0.0/8"] }); + + var evaluation = _sut.Evaluate(conditions, Signals(IPAddress.Parse("192.168.1.1"))); + + Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); + Assert.Equal(DenyReason.NotWithinIpRange, evaluation.Reason); + } + + [Fact] + public void Evaluate_NoConditions_Allows() + { + // A rule with no conditions is vacuously satisfied: access is auto-granted while still flowing through + // PAM for audit logging. + var evaluation = _sut.Evaluate(Set(), Signals()); + + Assert.Equal(AccessEvaluationOutcome.Allow, evaluation.Outcome); + } + + [Fact] + public void Evaluate_UnsupportedConditionKind_DeniesClosed() + { + var evaluation = _sut.Evaluate(Set(new UnknownCondition()), Signals()); + + Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); + Assert.Equal(DenyReason.UnsupportedCondition, evaluation.Reason); + } + + private sealed class UnknownCondition : AccessCondition; +} From d46d9fe37288c5410bb2e4148c2400dfa8e37270 Mon Sep 17 00:00:00 2001 From: Hinton Date: Thu, 16 Jul 2026 14:43:46 +0200 Subject: [PATCH 02/10] Add the governing-rule resolver over the PAM engine Resolve the access rule that governs a cipher for a caller: load the rules on every collection through which the caller reaches the cipher, pick the oldest (earliest creation date, ties broken on id) so selection is deterministic and structural, then evaluate that rule's conditions through the engine to report whether it requires human approval. Malformed stored conditions fail safe to human approval so access never silently auto-approves. Registered scoped; the resolver is the shared entry point the access-request and lease flows build on. --- .../src/Services/Pam/Models/GoverningRule.cs | 36 +++ .../Pam/Services/GoverningRuleResolver.cs | 114 +++++++ .../Pam/Services/IGoverningRuleResolver.cs | 19 ++ .../Utilities/ServiceCollectionExtensions.cs | 3 + .../Services/GoverningRuleResolverTests.cs | 278 ++++++++++++++++++ 5 files changed, 450 insertions(+) create mode 100644 bitwarden_license/src/Services/Pam/Models/GoverningRule.cs create mode 100644 bitwarden_license/src/Services/Pam/Services/GoverningRuleResolver.cs create mode 100644 bitwarden_license/src/Services/Pam/Services/IGoverningRuleResolver.cs create mode 100644 bitwarden_license/test/Services/Pam.Test/Services/GoverningRuleResolverTests.cs diff --git a/bitwarden_license/src/Services/Pam/Models/GoverningRule.cs b/bitwarden_license/src/Services/Pam/Models/GoverningRule.cs new file mode 100644 index 000000000000..cb1b5a3ef780 --- /dev/null +++ b/bitwarden_license/src/Services/Pam/Models/GoverningRule.cs @@ -0,0 +1,36 @@ +using Bit.Services.Pam.Models.Conditions; + +namespace Bit.Services.Pam.Models; + +/// +/// The access rule that governs a cipher for a particular caller: which collection's rule applies, the owning +/// organization, whether the rule requires human approval, and the parsed flat list of s +/// so the rule engine can evaluate them against the caller's signals. A null governing rule means the cipher is not +/// leasing-gated for the caller. +/// +public sealed record GoverningRule( + Guid OrganizationId, + Guid CollectionId, + bool RequiresHumanApproval, + IReadOnlyList Conditions) +{ + /// + /// The identity of the resolved access rule. Resolution is deterministic (oldest rule wins; see + /// ), so this is the rule that a request should pin at submit once + /// pinning is persisted. Until then it is re-resolved on every operation and can drift if the governing rules + /// change between submit and a later read. + /// + public Guid RuleId { get; init; } + + /// + /// When true, a member holding an active lease under this rule may extend it once (always auto-approved), by up + /// to . + /// + public bool AllowsExtensions { get; init; } + + /// + /// The longest a single extension under this rule may run, in seconds; meaningful only when + /// is true. + /// + public int? MaxExtensionDurationSeconds { get; init; } +} diff --git a/bitwarden_license/src/Services/Pam/Services/GoverningRuleResolver.cs b/bitwarden_license/src/Services/Pam/Services/GoverningRuleResolver.cs new file mode 100644 index 000000000000..9277d332d82e --- /dev/null +++ b/bitwarden_license/src/Services/Pam/Services/GoverningRuleResolver.cs @@ -0,0 +1,114 @@ +using System.Text.Json; +using Bit.Core.Entities; +using Bit.Core.Repositories; +using Bit.Pam.Entities; +using Bit.Pam.Repositories; +using Bit.Services.Pam.Engine; +using Bit.Services.Pam.Models; +using Bit.Services.Pam.Models.Conditions; + +namespace Bit.Services.Pam.Services; + +public class GoverningRuleResolver : IGoverningRuleResolver +{ + private static readonly JsonSerializerOptions _jsonOptions = new() + { + PropertyNamingPolicy = JsonNamingPolicy.CamelCase, + PropertyNameCaseInsensitive = true, + }; + + private readonly ICollectionCipherRepository _collectionCipherRepository; + private readonly ICollectionRepository _collectionRepository; + private readonly IAccessRuleRepository _accessRuleRepository; + private readonly IAccessRuleEngine _ruleEngine; + + public GoverningRuleResolver( + ICollectionCipherRepository collectionCipherRepository, + ICollectionRepository collectionRepository, + IAccessRuleRepository accessRuleRepository, + IAccessRuleEngine ruleEngine) + { + _collectionCipherRepository = collectionCipherRepository; + _collectionRepository = collectionRepository; + _accessRuleRepository = accessRuleRepository; + _ruleEngine = ruleEngine; + } + + public async Task ResolveAsync(Guid userId, Guid cipherId, AccessSignals signals) + { + var collectionCiphers = await _collectionCipherRepository.GetManyByUserIdCipherIdAsync(userId, cipherId); + if (collectionCiphers.Count == 0) + { + return null; + } + + var collectionIds = collectionCiphers.Select(cc => cc.CollectionId).ToHashSet(); + var collections = await _collectionRepository.GetManyByManyIdsAsync(collectionIds); + + var governedCollections = collections + .Where(c => collectionIds.Contains(c.Id) && c.AccessRuleId.HasValue); + + // Load every rule on the collections through which the caller reaches the cipher, keeping each paired with + // the collection it gates. A rule that no longer loads (deleted after the collection was read — deletes clear + // the link, so this is only a race) is skipped, so a deleted rule stops governing. + var candidates = new List<(Collection Collection, AccessRule Rule)>(); + foreach (var collection in governedCollections) + { + var accessRule = await _accessRuleRepository.GetByIdAsync(collection.AccessRuleId!.Value); + if (accessRule is not null) + { + candidates.Add((collection, accessRule)); + } + } + + if (candidates.Count == 0) + { + return null; + } + + // Oldest wins: the rule with the earliest CreationDate governs, ties broken on rule id so the choice is total + // and stable. Selection is purely structural — it does NOT depend on how a rule's conditions evaluate for the + // current signals — so a newer path never pre-empts an older one, whichever is the more permissive. This is a + // deliberate trade of determinism over least-restriction: a member may be routed to an approver even though a + // newer path would have auto-granted, because the older rule governs. The chosen rule's conditions are then + // evaluated below only to decide whether it routes to a human or resolves automatically. + var (governingCollection, governingRule) = candidates + .OrderBy(c => c.Rule.CreationDate) + .ThenBy(c => c.Rule.Id) + .First(); + + var conditions = Parse(governingRule.Conditions); + var outcome = _ruleEngine.Evaluate(conditions, signals).Outcome; + + return new GoverningRule( + governingCollection.OrganizationId, + governingCollection.Id, + outcome == AccessEvaluationOutcome.RequiresApproval, + conditions) + { + RuleId = governingRule.Id, + AllowsExtensions = governingRule.AllowsExtensions, + MaxExtensionDurationSeconds = governingRule.MaxExtensionDurationSeconds, + }; + } + + /// + /// Parses the stored conditions JSON into a flat list of . A malformed or + /// unparseable document fails safe to a single human-approval condition so access is never silently auto-approved + /// on conditions the server could not understand; the human-approval path then routes it to an approver rather + /// than issuing an automatic lease. + /// + private static IReadOnlyList Parse(string conditionsJson) + { + try + { + return JsonSerializer.Deserialize>(conditionsJson, _jsonOptions) ?? FailSafe(); + } + catch (JsonException) + { + return FailSafe(); + } + } + + private static IReadOnlyList FailSafe() => [new HumanApprovalCondition()]; +} diff --git a/bitwarden_license/src/Services/Pam/Services/IGoverningRuleResolver.cs b/bitwarden_license/src/Services/Pam/Services/IGoverningRuleResolver.cs new file mode 100644 index 000000000000..46fd3f820b31 --- /dev/null +++ b/bitwarden_license/src/Services/Pam/Services/IGoverningRuleResolver.cs @@ -0,0 +1,19 @@ +using Bit.Services.Pam.Engine; +using Bit.Services.Pam.Models; + +namespace Bit.Services.Pam.Services; + +public interface IGoverningRuleResolver +{ + /// + /// Resolves the access rule that governs for the caller, or null when the cipher is + /// not leasing-gated for them (no reachable collection carries an access rule). When more than one governing + /// collection applies, the oldest rule wins — the one with the earliest creation date, ties broken on rule id so + /// the choice is total and stable. Selection is purely structural and does NOT depend on how a rule's conditions + /// evaluate for the current : a newer path never pre-empts an older one, whichever is + /// the more permissive, so a caller may be routed to an approver even though a newer path would have auto-granted. + /// The resolved rule's conditions are still evaluated against to report whether it + /// requires human approval. + /// + Task ResolveAsync(Guid userId, Guid cipherId, AccessSignals signals); +} diff --git a/bitwarden_license/src/Services/Pam/Utilities/ServiceCollectionExtensions.cs b/bitwarden_license/src/Services/Pam/Utilities/ServiceCollectionExtensions.cs index 63fa5fa3c40b..83d46595db9b 100644 --- a/bitwarden_license/src/Services/Pam/Utilities/ServiceCollectionExtensions.cs +++ b/bitwarden_license/src/Services/Pam/Utilities/ServiceCollectionExtensions.cs @@ -21,6 +21,9 @@ public static IServiceCollection AddPamServices(this IServiceCollection services // Rule evaluation engine. Pure and stateless, so a singleton is safe. services.AddSingleton(); + // Resolves the access rule governing a cipher for a caller, then evaluates it via the engine. + services.AddScoped(); + // AccessRule write path. services.TryAddSingleton(TimeProvider.System); services.AddSingleton(); diff --git a/bitwarden_license/test/Services/Pam.Test/Services/GoverningRuleResolverTests.cs b/bitwarden_license/test/Services/Pam.Test/Services/GoverningRuleResolverTests.cs new file mode 100644 index 000000000000..f546cfe4519a --- /dev/null +++ b/bitwarden_license/test/Services/Pam.Test/Services/GoverningRuleResolverTests.cs @@ -0,0 +1,278 @@ +using System.Net; +using Bit.Core.Entities; +using Bit.Core.Repositories; +using Bit.Pam.Entities; +using Bit.Pam.Repositories; +using Bit.Services.Pam.Engine; +using Bit.Services.Pam.Models.Conditions; +using Bit.Services.Pam.Services; +using Bit.Test.Common.AutoFixture; +using Bit.Test.Common.AutoFixture.Attributes; +using NSubstitute; +using Xunit; + +namespace Bit.Services.Pam.Test.Services; + +[SutProviderCustomize] +public class GoverningRuleResolverTests +{ + // Selection is structural (oldest rule wins); the resolver still evaluates the chosen rule's conditions to report + // whether it needs human approval, so the tests drive the real engine through the substitute for that step. + private static readonly IAccessRuleEngine _engine = new AccessRuleEngine(); + + // An in-range IP for the 10.0.0.0/8 allowlists below; out-of-range for the 192.168/172.16 allowlists, which + // therefore deny. No time-of-day conditions are used, so the timestamp is arbitrary. + private static readonly AccessSignals _signals = new() + { + IpAddress = IPAddress.Parse("10.0.0.5"), + Timestamp = new DateTimeOffset(2026, 1, 1, 12, 0, 0, TimeSpan.Zero), + }; + + [Theory, BitAutoData] + public async Task ResolveAsync_NoReachableCollections_ReturnsNull( + SutProvider sutProvider, Guid userId, Guid cipherId) + { + sutProvider.GetDependency() + .GetManyByUserIdCipherIdAsync(userId, cipherId) + .Returns(new List()); + + Assert.Null(await sutProvider.Sut.ResolveAsync(userId, cipherId, _signals)); + } + + [Theory, BitAutoData] + public async Task ResolveAsync_CollectionWithoutAccessRule_ReturnsNull( + SutProvider sutProvider, Guid userId, Guid cipherId, Collection collection) + { + collection.AccessRuleId = null; + SetupReachableCollections(sutProvider, userId, cipherId, collection); + + Assert.Null(await sutProvider.Sut.ResolveAsync(userId, cipherId, _signals)); + } + + [Theory, BitAutoData] + public async Task ResolveAsync_HumanApprovalCondition_RequiresHumanApproval( + SutProvider sutProvider, Guid userId, Guid cipherId, Collection collection, AccessRule rule) + { + rule.Conditions = """[{"kind":"human_approval"}]"""; + SetupGovernedCollection(sutProvider, userId, cipherId, collection, rule); + + var result = await sutProvider.Sut.ResolveAsync(userId, cipherId, _signals); + + Assert.NotNull(result); + Assert.True(result!.RequiresHumanApproval); + Assert.Equal(collection.Id, result.CollectionId); + Assert.Equal(collection.OrganizationId, result.OrganizationId); + Assert.IsType(Assert.Single(result.Conditions)); + } + + [Theory, BitAutoData] + public async Task ResolveAsync_PassingIpAllowlistCondition_DoesNotRequireHumanApproval( + SutProvider sutProvider, Guid userId, Guid cipherId, Collection collection, AccessRule rule) + { + rule.Conditions = """[{"kind":"ip_allowlist","cidrs":["10.0.0.0/8"]}]"""; + SetupGovernedCollection(sutProvider, userId, cipherId, collection, rule); + + var result = await sutProvider.Sut.ResolveAsync(userId, cipherId, _signals); + + Assert.NotNull(result); + Assert.False(result!.RequiresHumanApproval); + var ip = Assert.IsType(Assert.Single(result.Conditions)); + Assert.Equal("10.0.0.0/8", Assert.Single(ip.Cidrs)); + } + + [Theory, BitAutoData] + public async Task ResolveAsync_ConditionsContainingHumanApproval_RequiresHumanApproval( + SutProvider sutProvider, Guid userId, Guid cipherId, Collection collection, AccessRule rule) + { + rule.Conditions = """[{"kind":"ip_allowlist","cidrs":["10.0.0.0/8"]},{"kind":"human_approval"}]"""; + SetupGovernedCollection(sutProvider, userId, cipherId, collection, rule); + + var result = await sutProvider.Sut.ResolveAsync(userId, cipherId, _signals); + + Assert.NotNull(result); + Assert.True(result!.RequiresHumanApproval); + Assert.Equal(2, result.Conditions.Count); + Assert.Contains(result.Conditions, condition => condition is HumanApprovalCondition); + } + + [Theory, BitAutoData] + public async Task ResolveAsync_EmptyConditions_DoesNotRequireHumanApproval( + SutProvider sutProvider, Guid userId, Guid cipherId, Collection collection, AccessRule rule) + { + // A conditionless rule governs the collection for audit logging but auto-approves access. + rule.Conditions = "[]"; + SetupGovernedCollection(sutProvider, userId, cipherId, collection, rule); + + var result = await sutProvider.Sut.ResolveAsync(userId, cipherId, _signals); + + Assert.NotNull(result); + Assert.False(result!.RequiresHumanApproval); + Assert.Empty(result.Conditions); + } + + [Theory, BitAutoData] + public async Task ResolveAsync_MalformedRule_FailsSafeToHumanApproval( + SutProvider sutProvider, Guid userId, Guid cipherId, Collection collection, AccessRule rule) + { + rule.Conditions = "not json"; + SetupGovernedCollection(sutProvider, userId, cipherId, collection, rule); + + var result = await sutProvider.Sut.ResolveAsync(userId, cipherId, _signals); + + Assert.NotNull(result); + Assert.True(result!.RequiresHumanApproval); + // An unparseable rule fails safe to human approval rather than surfacing a rule the engine cannot evaluate. + Assert.IsType(Assert.Single(result.Conditions)); + } + + [Theory, BitAutoData] + public async Task ResolveAsync_MultipleRules_OldestCreationDateWins( + SutProvider sutProvider, Guid userId, Guid cipherId, + Collection olderCollection, AccessRule olderRule, Collection newerCollection, AccessRule newerRule) + { + // The older rule needs human approval; the newer one would auto-grant. Oldest wins even though it is the more + // restrictive path — the caller is routed to an approver rather than auto-granted (do not reintroduce the + // retired least-restrictive behaviour). + olderRule.CreationDate = new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc); + olderRule.Conditions = """[{"kind":"human_approval"}]"""; + newerRule.CreationDate = new DateTime(2026, 6, 1, 0, 0, 0, DateTimeKind.Utc); + newerRule.Conditions = """[{"kind":"ip_allowlist","cidrs":["10.0.0.0/8"]}]"""; + SetupGovernedCollections(sutProvider, userId, cipherId, + (olderCollection, olderRule), (newerCollection, newerRule)); + + var result = await sutProvider.Sut.ResolveAsync(userId, cipherId, _signals); + + Assert.NotNull(result); + Assert.True(result!.RequiresHumanApproval); + Assert.Equal(olderCollection.Id, result.CollectionId); + Assert.Equal(olderRule.Id, result.RuleId); + } + + [Theory, BitAutoData] + public async Task ResolveAsync_MultipleRules_OlderAutomaticWinsOverNewerHumanApproval( + SutProvider sutProvider, Guid userId, Guid cipherId, + Collection olderCollection, AccessRule olderRule, Collection newerCollection, AccessRule newerRule) + { + // The mirror of the previous case: here the oldest rule auto-grants and the newer one needs human approval, so + // the caller is auto-granted. Whichever is older governs, regardless of which is more permissive. + olderRule.CreationDate = new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc); + olderRule.Conditions = """[{"kind":"ip_allowlist","cidrs":["10.0.0.0/8"]}]"""; + newerRule.CreationDate = new DateTime(2026, 6, 1, 0, 0, 0, DateTimeKind.Utc); + newerRule.Conditions = """[{"kind":"human_approval"}]"""; + SetupGovernedCollections(sutProvider, userId, cipherId, + (olderCollection, olderRule), (newerCollection, newerRule)); + + var result = await sutProvider.Sut.ResolveAsync(userId, cipherId, _signals); + + Assert.NotNull(result); + Assert.False(result!.RequiresHumanApproval); + Assert.Equal(olderCollection.Id, result.CollectionId); + Assert.Equal(olderRule.Id, result.RuleId); + } + + [Theory, BitAutoData] + public async Task ResolveAsync_OldestRuleFailsAutomatedConditions_StillGoverns( + SutProvider sutProvider, Guid userId, Guid cipherId, + Collection olderCollection, AccessRule olderRule, Collection newerCollection, AccessRule newerRule) + { + // The oldest rule's IP allowlist fails for this caller; a newer rule would pass. Selection is structural, so + // the failing oldest rule still governs — the resolver never lets a newer path pre-empt it by evaluating + // conditions. (Downstream, the auto path then surfaces the denial; that is not the resolver's concern.) + olderRule.CreationDate = new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc); + olderRule.Conditions = """[{"kind":"ip_allowlist","cidrs":["192.168.0.0/16"]}]"""; + newerRule.CreationDate = new DateTime(2026, 6, 1, 0, 0, 0, DateTimeKind.Utc); + newerRule.Conditions = """[{"kind":"ip_allowlist","cidrs":["10.0.0.0/8"]}]"""; + SetupGovernedCollections(sutProvider, userId, cipherId, + (olderCollection, olderRule), (newerCollection, newerRule)); + + var result = await sutProvider.Sut.ResolveAsync(userId, cipherId, _signals); + + Assert.NotNull(result); + Assert.False(result!.RequiresHumanApproval); + Assert.Equal(olderCollection.Id, result.CollectionId); + Assert.Equal(olderRule.Id, result.RuleId); + } + + [Theory, BitAutoData] + public async Task ResolveAsync_TieOnCreationDate_LowerRuleIdWins( + SutProvider sutProvider, Guid userId, Guid cipherId, + Collection lowerCollection, AccessRule lowerRule, Collection higherCollection, AccessRule higherRule) + { + // Two rules created at the same instant: the tie breaks on rule id (lowest wins) so the choice is total and + // stable rather than dependent on iteration order. + var sharedCreation = new DateTime(2026, 3, 1, 0, 0, 0, DateTimeKind.Utc); + lowerRule.Id = new Guid("00000000-0000-0000-0000-000000000001"); + lowerRule.CreationDate = sharedCreation; + lowerRule.Conditions = """[{"kind":"ip_allowlist","cidrs":["10.0.0.0/8"]}]"""; + higherRule.Id = new Guid("00000000-0000-0000-0000-000000000002"); + higherRule.CreationDate = sharedCreation; + higherRule.Conditions = """[{"kind":"human_approval"}]"""; + SetupGovernedCollections(sutProvider, userId, cipherId, + (higherCollection, higherRule), (lowerCollection, lowerRule)); + + var result = await sutProvider.Sut.ResolveAsync(userId, cipherId, _signals); + + Assert.NotNull(result); + Assert.Equal(lowerRule.Id, result!.RuleId); + Assert.Equal(lowerCollection.Id, result.CollectionId); + } + + [Theory, BitAutoData] + public async Task ResolveAsync_OldestRuleMalformed_FailsSafeToHumanApprovalEvenWithNewerAutoPath( + SutProvider sutProvider, Guid userId, Guid cipherId, + Collection olderCollection, AccessRule olderRule, Collection newerCollection, AccessRule newerRule) + { + // The oldest rule is unparseable; a newer rule would auto-grant. Because the oldest rule governs, it fails safe + // to human approval rather than letting the newer parseable path auto-grant around it. + olderRule.CreationDate = new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc); + olderRule.Conditions = "not json"; + newerRule.CreationDate = new DateTime(2026, 6, 1, 0, 0, 0, DateTimeKind.Utc); + newerRule.Conditions = """[{"kind":"ip_allowlist","cidrs":["10.0.0.0/8"]}]"""; + SetupGovernedCollections(sutProvider, userId, cipherId, + (olderCollection, olderRule), (newerCollection, newerRule)); + + var result = await sutProvider.Sut.ResolveAsync(userId, cipherId, _signals); + + Assert.NotNull(result); + Assert.True(result!.RequiresHumanApproval); + Assert.Equal(olderCollection.Id, result.CollectionId); + Assert.IsType(Assert.Single(result.Conditions)); + } + + private static void SetupReachableCollections( + SutProvider sutProvider, Guid userId, Guid cipherId, params Collection[] collections) + { + sutProvider.GetDependency() + .GetManyByUserIdCipherIdAsync(userId, cipherId) + .Returns(collections.Select(c => new CollectionCipher { CollectionId = c.Id, CipherId = cipherId }).ToList()); + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(collections.ToList()); + } + + private static void SetupGovernedCollection( + SutProvider sutProvider, Guid userId, Guid cipherId, Collection collection, AccessRule rule) + => SetupGovernedCollections(sutProvider, userId, cipherId, (collection, rule)); + + private static void SetupGovernedCollections( + SutProvider sutProvider, Guid userId, Guid cipherId, + params (Collection collection, AccessRule rule)[] pairs) + { + foreach (var (collection, rule) in pairs) + { + collection.AccessRuleId = rule.Id; + } + + SetupReachableCollections(sutProvider, userId, cipherId, pairs.Select(p => p.collection).ToArray()); + + foreach (var (_, rule) in pairs) + { + sutProvider.GetDependency().GetByIdAsync(rule.Id).Returns(rule); + } + + // Drive the real engine through the substitute so resolution exercises true IP/time evaluation. + sutProvider.GetDependency() + .Evaluate(Arg.Any>(), Arg.Any()) + .Returns(ci => _engine.Evaluate(ci.ArgAt>(0), ci.ArgAt(1))); + } +} From c734d67f63ba64d3d20d55cd42b7a58c53a3b1e9 Mon Sep 17 00:00:00 2001 From: Hinton Date: Thu, 16 Jul 2026 14:57:32 +0200 Subject: [PATCH 03/10] Cover engine fail-closed paths, resolver deleted-rule race, weekday converter Close the coverage gaps in the PAM engine slice: - Engine: malformed-but-present CIDR and time-window bounds (fail closed), and matching on a later CIDR/window entry rather than only the first. - Resolver: a governing rule that no longer loads is skipped, both when it is the only candidate (no governance) and when a surviving newer rule takes over; and a time_of_day condition is parsed and evaluated through the resolver. - Port AccessWeekdayJsonConverterTests (source was already on the branch, untested), including the AccessWeekday/DayOfWeek numeric-alignment guard. --- .../Pam.Test/Engine/AccessRuleEngineTests.cs | 58 +++++++++++++++++ .../AccessWeekdayJsonConverterTests.cs | 52 ++++++++++++++++ .../Services/GoverningRuleResolverTests.cs | 62 +++++++++++++++++++ 3 files changed, 172 insertions(+) create mode 100644 bitwarden_license/test/Services/Pam.Test/Models/Conditions/AccessWeekdayJsonConverterTests.cs diff --git a/bitwarden_license/test/Services/Pam.Test/Engine/AccessRuleEngineTests.cs b/bitwarden_license/test/Services/Pam.Test/Engine/AccessRuleEngineTests.cs index ec00c19046a5..126758e6d92d 100644 --- a/bitwarden_license/test/Services/Pam.Test/Engine/AccessRuleEngineTests.cs +++ b/bitwarden_license/test/Services/Pam.Test/Engine/AccessRuleEngineTests.cs @@ -214,5 +214,63 @@ public void Evaluate_UnsupportedConditionKind_DeniesClosed() Assert.Equal(DenyReason.UnsupportedCondition, evaluation.Reason); } + [Fact] + public void Evaluate_IpAllowlist_MalformedCidr_DeniesClosed() + { + // A present-but-unparseable CIDR matches no address, so a caller with a known IP still fails closed. + var conditions = Set(new IpAllowlistCondition { Cidrs = ["not-a-cidr"] }); + + var evaluation = _sut.Evaluate(conditions, Signals(IPAddress.Parse("10.1.2.3"))); + + Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); + Assert.Equal(DenyReason.NotWithinIpRange, evaluation.Reason); + } + + [Fact] + public void Evaluate_IpAllowlist_LaterCidrMatches_Allows() + { + // The caller matches the second entry, so evaluation must not stop at the first non-matching CIDR. + var conditions = Set(new IpAllowlistCondition { Cidrs = ["192.168.0.0/16", "10.0.0.0/8"] }); + + var evaluation = _sut.Evaluate(conditions, Signals(IPAddress.Parse("10.1.2.3"))); + + Assert.Equal(AccessEvaluationOutcome.Allow, evaluation.Outcome); + } + + [Fact] + public void Evaluate_TimeOfDay_MalformedWindowTime_DeniesClosed() + { + // The day matches but the window's bounds are unparseable, so the window cannot admit the caller. + var conditions = Set(new TimeOfDayCondition + { + Tz = "UTC", + Windows = [new TimeWindow { Days = [AccessWeekday.Thu], From = "25:99", To = "30:00" }], + }); + + var evaluation = _sut.Evaluate(conditions, Signals()); + + Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); + Assert.Equal(DenyReason.NotWithinTimeWindow, evaluation.Reason); + } + + [Fact] + public void Evaluate_TimeOfDay_LaterWindowMatches_Allows() + { + // The first window is the wrong day; the second admits the caller, so windows past the first must be checked. + var conditions = Set(new TimeOfDayCondition + { + Tz = "UTC", + Windows = + [ + new TimeWindow { Days = [AccessWeekday.Fri], From = "09:00", To = "17:00" }, + new TimeWindow { Days = [AccessWeekday.Thu], From = "09:00", To = "17:00" }, + ], + }); + + var evaluation = _sut.Evaluate(conditions, Signals()); + + Assert.Equal(AccessEvaluationOutcome.Allow, evaluation.Outcome); + } + private sealed class UnknownCondition : AccessCondition; } diff --git a/bitwarden_license/test/Services/Pam.Test/Models/Conditions/AccessWeekdayJsonConverterTests.cs b/bitwarden_license/test/Services/Pam.Test/Models/Conditions/AccessWeekdayJsonConverterTests.cs new file mode 100644 index 000000000000..73698c554f08 --- /dev/null +++ b/bitwarden_license/test/Services/Pam.Test/Models/Conditions/AccessWeekdayJsonConverterTests.cs @@ -0,0 +1,52 @@ +using System.Text.Json; +using Bit.Services.Pam.Enums; +using Xunit; + +namespace Bit.Services.Pam.Test.Models.Conditions; + +public class AccessWeekdayJsonConverterTests +{ + [Theory] + [InlineData(AccessWeekday.Sun, "\"sun\"")] + [InlineData(AccessWeekday.Mon, "\"mon\"")] + [InlineData(AccessWeekday.Tue, "\"tue\"")] + [InlineData(AccessWeekday.Wed, "\"wed\"")] + [InlineData(AccessWeekday.Thu, "\"thu\"")] + [InlineData(AccessWeekday.Fri, "\"fri\"")] + [InlineData(AccessWeekday.Sat, "\"sat\"")] + public void Serializes_AsLowercaseToken(AccessWeekday day, string expectedJson) + { + Assert.Equal(expectedJson, JsonSerializer.Serialize(day)); + } + + [Theory] + [InlineData("\"mon\"", AccessWeekday.Mon)] + [InlineData("\"MON\"", AccessWeekday.Mon)] + [InlineData("\"Sun\"", AccessWeekday.Sun)] + public void Deserializes_TokenCaseInsensitively(string json, AccessWeekday expected) + { + Assert.Equal(expected, JsonSerializer.Deserialize(json)); + } + + [Theory] + [InlineData("\"funday\"")] + [InlineData("\"\"")] + [InlineData("3")] + public void Deserialize_InvalidToken_Throws(string json) + { + Assert.Throws(() => JsonSerializer.Deserialize(json)); + } + + [Fact] + public void EnumValues_AlignWithSystemDayOfWeek() + { + // The engine casts AccessWeekday straight to System.DayOfWeek, so the numeric values must match. + Assert.Equal((int)DayOfWeek.Sunday, (int)AccessWeekday.Sun); + Assert.Equal((int)DayOfWeek.Monday, (int)AccessWeekday.Mon); + Assert.Equal((int)DayOfWeek.Tuesday, (int)AccessWeekday.Tue); + Assert.Equal((int)DayOfWeek.Wednesday, (int)AccessWeekday.Wed); + Assert.Equal((int)DayOfWeek.Thursday, (int)AccessWeekday.Thu); + Assert.Equal((int)DayOfWeek.Friday, (int)AccessWeekday.Fri); + Assert.Equal((int)DayOfWeek.Saturday, (int)AccessWeekday.Sat); + } +} diff --git a/bitwarden_license/test/Services/Pam.Test/Services/GoverningRuleResolverTests.cs b/bitwarden_license/test/Services/Pam.Test/Services/GoverningRuleResolverTests.cs index f546cfe4519a..e7ed578327bb 100644 --- a/bitwarden_license/test/Services/Pam.Test/Services/GoverningRuleResolverTests.cs +++ b/bitwarden_license/test/Services/Pam.Test/Services/GoverningRuleResolverTests.cs @@ -4,6 +4,7 @@ using Bit.Pam.Entities; using Bit.Pam.Repositories; using Bit.Services.Pam.Engine; +using Bit.Services.Pam.Enums; using Bit.Services.Pam.Models.Conditions; using Bit.Services.Pam.Services; using Bit.Test.Common.AutoFixture; @@ -239,6 +240,62 @@ public async Task ResolveAsync_OldestRuleMalformed_FailsSafeToHumanApprovalEvenW Assert.IsType(Assert.Single(result.Conditions)); } + [Theory, BitAutoData] + public async Task ResolveAsync_GovernedRuleDeleted_ReturnsNull( + SutProvider sutProvider, Guid userId, Guid cipherId, Collection collection, AccessRule rule) + { + // The collection still points at a rule id, but the rule no longer loads (deleted after the collection was + // read). It is dropped from the candidates, leaving nothing to govern — GetByIdAsync is left unstubbed so it + // returns null. + collection.AccessRuleId = rule.Id; + SetupReachableCollections(sutProvider, userId, cipherId, collection); + + Assert.Null(await sutProvider.Sut.ResolveAsync(userId, cipherId, _signals)); + } + + [Theory, BitAutoData] + public async Task ResolveAsync_OldestGovernedRuleDeleted_NextRuleGoverns( + SutProvider sutProvider, Guid userId, Guid cipherId, + Collection olderCollection, AccessRule olderRule, Collection newerCollection, AccessRule newerRule) + { + // The oldest governing rule was deleted after the collection was read, so it is skipped and the surviving + // newer rule governs — a deleted rule stops governing even when it would otherwise have won on age. + olderRule.CreationDate = new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc); + newerRule.CreationDate = new DateTime(2026, 6, 1, 0, 0, 0, DateTimeKind.Utc); + newerRule.Conditions = """[{"kind":"human_approval"}]"""; + olderCollection.AccessRuleId = olderRule.Id; + newerCollection.AccessRuleId = newerRule.Id; + SetupReachableCollections(sutProvider, userId, cipherId, olderCollection, newerCollection); + // Only the newer rule loads; GetByIdAsync(olderRule.Id) is left unstubbed so the deleted oldest returns null. + sutProvider.GetDependency().GetByIdAsync(newerRule.Id).Returns(newerRule); + DriveRealEngine(sutProvider); + + var result = await sutProvider.Sut.ResolveAsync(userId, cipherId, _signals); + + Assert.NotNull(result); + Assert.Equal(newerCollection.Id, result!.CollectionId); + Assert.Equal(newerRule.Id, result.RuleId); + Assert.True(result.RequiresHumanApproval); + } + + [Theory, BitAutoData] + public async Task ResolveAsync_TimeOfDayCondition_ParsedAndEvaluatedThroughResolver( + SutProvider sutProvider, Guid userId, Guid cipherId, Collection collection, AccessRule rule) + { + // _signals' timestamp (2026-01-01 12:00 UTC) is a Thursday inside this window, so the rule auto-approves. This + // exercises the resolver's own camelCase parse of a time_of_day condition, including the weekday-token converter. + rule.Conditions = """[{"kind":"time_of_day","tz":"UTC","windows":[{"days":["thu"],"from":"09:00","to":"17:00"}]}]"""; + SetupGovernedCollection(sutProvider, userId, cipherId, collection, rule); + + var result = await sutProvider.Sut.ResolveAsync(userId, cipherId, _signals); + + Assert.NotNull(result); + Assert.False(result!.RequiresHumanApproval); + var time = Assert.IsType(Assert.Single(result.Conditions)); + Assert.Equal("UTC", time.Tz); + Assert.Equal(AccessWeekday.Thu, Assert.Single(Assert.Single(time.Windows).Days)); + } + private static void SetupReachableCollections( SutProvider sutProvider, Guid userId, Guid cipherId, params Collection[] collections) { @@ -270,6 +327,11 @@ private static void SetupGovernedCollections( sutProvider.GetDependency().GetByIdAsync(rule.Id).Returns(rule); } + DriveRealEngine(sutProvider); + } + + private static void DriveRealEngine(SutProvider sutProvider) + { // Drive the real engine through the substitute so resolution exercises true IP/time evaluation. sutProvider.GetDependency() .Evaluate(Arg.Any>(), Arg.Any()) From 9d51e259c009358fb092165c872788676b3799b9 Mon Sep 17 00:00:00 2001 From: Hinton Date: Fri, 17 Jul 2026 10:07:23 +0200 Subject: [PATCH 04/10] Document PAM access-evaluation enums and condition fields --- .../Services/Pam/Engine/AccessEvaluation.cs | 36 +++++++++++++++++++ .../Models/Conditions/IpAllowlistCondition.cs | 4 +++ .../Models/Conditions/TimeOfDayCondition.cs | 12 +++++++ 3 files changed, 52 insertions(+) diff --git a/bitwarden_license/src/Services/Pam/Engine/AccessEvaluation.cs b/bitwarden_license/src/Services/Pam/Engine/AccessEvaluation.cs index 0632442e95df..dfef17886581 100644 --- a/bitwarden_license/src/Services/Pam/Engine/AccessEvaluation.cs +++ b/bitwarden_license/src/Services/Pam/Engine/AccessEvaluation.cs @@ -1,34 +1,70 @@ namespace Bit.Services.Pam.Engine; +/// +/// The result of evaluating a single access condition, or the combined result of a rule's whole condition list. +/// public enum AccessEvaluationOutcome { + /// Access is granted automatically, with no human decision required. Allow, + + /// A human decision is required before a lease may be issued. RequiresApproval, + + /// Access is refused; the accompanying records why. Deny, } +/// +/// Why an evaluation denied. Carried on a deny ; is the default +/// for any non-deny outcome. +/// public enum DenyReason { + /// Not a denial (the outcome is allow or requires-approval). None = 0, + + /// The caller's IP was absent, the allowlist was empty, or the IP fell outside every listed CIDR. NotWithinIpRange, + + /// The timezone was unknown/invalid, or the instant fell outside every configured window. NotWithinTimeWindow, + + /// The engine did not recognize the condition kind, so it could not be shown satisfied (fail closed). UnsupportedCondition, } +/// +/// The outcome of evaluating an access condition (or a combined rule result): an plus, when +/// it is a denial, the . Build instances via , , +/// or , and fold a sequence together with . +/// public sealed record AccessEvaluation { + /// The evaluation's verdict. public required AccessEvaluationOutcome Outcome { get; init; } + + /// Why access was denied; unless is . public DenyReason Reason { get; init; } = DenyReason.None; + /// A shared allow result. public static AccessEvaluation Allow { get; } = new() { Outcome = AccessEvaluationOutcome.Allow }; + + /// A shared requires-approval result. public static AccessEvaluation RequiresApproval { get; } = new() { Outcome = AccessEvaluationOutcome.RequiresApproval }; + /// Builds a deny result carrying the given . public static AccessEvaluation Deny(DenyReason reason) => new() { Outcome = AccessEvaluationOutcome.Deny, Reason = reason }; + /// + /// Folds a sequence of per-condition evaluations into one, with deny > requires-approval > allow + /// precedence: the first deny short-circuits and is returned as-is; otherwise any requires-approval wins over + /// allow. An empty sequence is vacuously satisfied and returns . + /// public static AccessEvaluation Combine(IEnumerable evaluations) { var requiresApproval = false; diff --git a/bitwarden_license/src/Services/Pam/Models/Conditions/IpAllowlistCondition.cs b/bitwarden_license/src/Services/Pam/Models/Conditions/IpAllowlistCondition.cs index fd63567945cc..c73b762b1822 100644 --- a/bitwarden_license/src/Services/Pam/Models/Conditions/IpAllowlistCondition.cs +++ b/bitwarden_license/src/Services/Pam/Models/Conditions/IpAllowlistCondition.cs @@ -5,5 +5,9 @@ /// public sealed class IpAllowlistCondition : AccessCondition { + /// + /// The allowed source ranges in CIDR notation (e.g. "10.0.0.0/8"). 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. + /// public IReadOnlyList Cidrs { get; init; } = []; } diff --git a/bitwarden_license/src/Services/Pam/Models/Conditions/TimeOfDayCondition.cs b/bitwarden_license/src/Services/Pam/Models/Conditions/TimeOfDayCondition.cs index 6b6a33bd1d0a..c4ce4096db80 100644 --- a/bitwarden_license/src/Services/Pam/Models/Conditions/TimeOfDayCondition.cs +++ b/bitwarden_license/src/Services/Pam/Models/Conditions/TimeOfDayCondition.cs @@ -7,13 +7,25 @@ namespace Bit.Services.Pam.Models.Conditions; /// public sealed class TimeOfDayCondition : AccessCondition { + /// The IANA timezone (e.g. "America/New_York") the windows are evaluated in. An unknown or invalid zone denies. public string Tz { get; init; } = string.Empty; + + /// The windows that grant access; the condition allows when the instant falls in any one of them. At least one required. public IReadOnlyList Windows { get; init; } = []; } +/// +/// A single recurring window: the days it applies on and the daily start/end times, all in the parent +/// condition's timezone. +/// public sealed class TimeWindow { + /// The days of the week the window is active on. At least one required. public IReadOnlyList Days { get; init; } = []; + + /// Window start, 24-hour HH:mm (00:00–23:59). Inclusive. public string From { get; init; } = string.Empty; + + /// Window end, 24-hour HH:mm (00:00–23:59). Inclusive. public string To { get; init; } = string.Empty; } From 2c447ada1a3f8887e0d8e0f464e26a59506407fe Mon Sep 17 00:00:00 2001 From: Hinton Date: Fri, 17 Jul 2026 10:15:18 +0200 Subject: [PATCH 05/10] Share one JsonSerializerOptions for access-rule conditions --- .../Models/Conditions/AccessConditionJson.cs | 19 +++++++++++++++++++ .../Pam/Services/AccessRuleValidator.cs | 8 +------- .../Pam/Services/GoverningRuleResolver.cs | 8 +------- 3 files changed, 21 insertions(+), 14 deletions(-) create mode 100644 bitwarden_license/src/Services/Pam/Models/Conditions/AccessConditionJson.cs diff --git a/bitwarden_license/src/Services/Pam/Models/Conditions/AccessConditionJson.cs b/bitwarden_license/src/Services/Pam/Models/Conditions/AccessConditionJson.cs new file mode 100644 index 000000000000..60e048d5ffc0 --- /dev/null +++ b/bitwarden_license/src/Services/Pam/Models/Conditions/AccessConditionJson.cs @@ -0,0 +1,19 @@ +using System.Text.Json; + +namespace Bit.Services.Pam.Models.Conditions; + +/// +/// The single source of truth for how an access rule's conditions JSON is (de)serialized: camelCase property +/// names, read case-insensitively. Everything that parses the stored Conditions document — the validator +/// at write time and the resolver at read time — must use so the two never drift. +/// The accepted kind vocabulary itself lives on 's [JsonDerivedType] +/// attributes. +/// +public static class AccessConditionJson +{ + public static readonly JsonSerializerOptions Options = new() + { + PropertyNamingPolicy = JsonNamingPolicy.CamelCase, + PropertyNameCaseInsensitive = true, + }; +} diff --git a/bitwarden_license/src/Services/Pam/Services/AccessRuleValidator.cs b/bitwarden_license/src/Services/Pam/Services/AccessRuleValidator.cs index f4a48061991f..032d1b80fded 100644 --- a/bitwarden_license/src/Services/Pam/Services/AccessRuleValidator.cs +++ b/bitwarden_license/src/Services/Pam/Services/AccessRuleValidator.cs @@ -9,12 +9,6 @@ public sealed partial class AccessRuleValidator : IAccessRuleValidator { private const int MaxConditions = 10; - private static readonly JsonSerializerOptions JsonOptions = new() - { - PropertyNamingPolicy = JsonNamingPolicy.CamelCase, - PropertyNameCaseInsensitive = true, - }; - [GeneratedRegex(@"^([01][0-9]|2[0-3]):[0-5][0-9]$")] private static partial Regex TimeOfDayRegex(); @@ -33,7 +27,7 @@ public AccessRuleValidationResult Validate(string? conditionsJson) List? conditions; try { - conditions = JsonSerializer.Deserialize>(conditionsJson, JsonOptions); + conditions = JsonSerializer.Deserialize>(conditionsJson, AccessConditionJson.Options); } catch (JsonException ex) { diff --git a/bitwarden_license/src/Services/Pam/Services/GoverningRuleResolver.cs b/bitwarden_license/src/Services/Pam/Services/GoverningRuleResolver.cs index 9277d332d82e..62ff68027294 100644 --- a/bitwarden_license/src/Services/Pam/Services/GoverningRuleResolver.cs +++ b/bitwarden_license/src/Services/Pam/Services/GoverningRuleResolver.cs @@ -11,12 +11,6 @@ namespace Bit.Services.Pam.Services; public class GoverningRuleResolver : IGoverningRuleResolver { - private static readonly JsonSerializerOptions _jsonOptions = new() - { - PropertyNamingPolicy = JsonNamingPolicy.CamelCase, - PropertyNameCaseInsensitive = true, - }; - private readonly ICollectionCipherRepository _collectionCipherRepository; private readonly ICollectionRepository _collectionRepository; private readonly IAccessRuleRepository _accessRuleRepository; @@ -102,7 +96,7 @@ private static IReadOnlyList Parse(string conditionsJson) { try { - return JsonSerializer.Deserialize>(conditionsJson, _jsonOptions) ?? FailSafe(); + return JsonSerializer.Deserialize>(conditionsJson, AccessConditionJson.Options) ?? FailSafe(); } catch (JsonException) { From 6a77c35080d61919d585e566480e59f527080be3 Mon Sep 17 00:00:00 2001 From: Hinton Date: Fri, 17 Jul 2026 10:48:46 +0200 Subject: [PATCH 06/10] Make access rule conditions evaluate themselves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../Services/Pam/Engine/AccessEvaluation.cs | 6 +- .../Services/Pam/Engine/AccessRuleEngine.cs | 81 ++---------- .../Pam/Models/Conditions/AccessCondition.cs | 18 ++- .../Conditions/HumanApprovalCondition.cs | 12 +- .../Conditions/IAccessConditionVisitor.cs | 15 +++ .../Models/Conditions/IpAllowlistCondition.cs | 32 ++++- .../Models/Conditions/TimeOfDayCondition.cs | 49 +++++++- .../Pam/Services/AccessRuleValidator.cs | 116 +++++++++--------- .../Pam.Test/Engine/AccessRuleEngineTests.cs | 8 +- 9 files changed, 200 insertions(+), 137 deletions(-) create mode 100644 bitwarden_license/src/Services/Pam/Models/Conditions/IAccessConditionVisitor.cs diff --git a/bitwarden_license/src/Services/Pam/Engine/AccessEvaluation.cs b/bitwarden_license/src/Services/Pam/Engine/AccessEvaluation.cs index dfef17886581..af008ffebe7b 100644 --- a/bitwarden_license/src/Services/Pam/Engine/AccessEvaluation.cs +++ b/bitwarden_license/src/Services/Pam/Engine/AccessEvaluation.cs @@ -30,7 +30,11 @@ public enum DenyReason /// The timezone was unknown/invalid, or the instant fell outside every configured window. NotWithinTimeWindow, - /// The engine did not recognize the condition kind, so it could not be shown satisfied (fail closed). + /// + /// A condition entry could not be evaluated — in practice a null entry from a malformed stored document — so it + /// fails closed. A genuinely unknown kind cannot reach here: the JSON layer rejects unknown kinds and the + /// visitor dispatch is exhaustive at compile time. + /// UnsupportedCondition, } diff --git a/bitwarden_license/src/Services/Pam/Engine/AccessRuleEngine.cs b/bitwarden_license/src/Services/Pam/Engine/AccessRuleEngine.cs index 4a1b9ec278f6..4d9fd19159fe 100644 --- a/bitwarden_license/src/Services/Pam/Engine/AccessRuleEngine.cs +++ b/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; /// -/// Evaluates the access rule's flat list of s against the caller's signals. Each -/// condition yields an ; 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 s into one decision. Each +/// condition evaluates itself (); 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. /// public sealed class AccessRuleEngine : IAccessRuleEngine { public AccessEvaluation Evaluate(IReadOnlyList 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); } diff --git a/bitwarden_license/src/Services/Pam/Models/Conditions/AccessCondition.cs b/bitwarden_license/src/Services/Pam/Models/Conditions/AccessCondition.cs index 1cc88d98a4a3..821acfaca498 100644 --- a/bitwarden_license/src/Services/Pam/Models/Conditions/AccessCondition.cs +++ b/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 +{ + /// + /// Evaluates this condition against the request-time , 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. + /// + public abstract AccessEvaluation Evaluate(AccessSignals signals); + + /// + /// Double-dispatches to the '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. + /// + public abstract T Accept(IAccessConditionVisitor visitor); +} diff --git a/bitwarden_license/src/Services/Pam/Models/Conditions/HumanApprovalCondition.cs b/bitwarden_license/src/Services/Pam/Models/Conditions/HumanApprovalCondition.cs index d7a7b4f7e288..50c876b398ce 100644 --- a/bitwarden_license/src/Services/Pam/Models/Conditions/HumanApprovalCondition.cs +++ b/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; /// /// Always requires a human decision before a lease can be issued. /// -public sealed class HumanApprovalCondition : AccessCondition; +/// Wire format: { "kind": "human_approval" } +public sealed class HumanApprovalCondition : AccessCondition +{ + public override AccessEvaluation Evaluate(AccessSignals signals) => AccessEvaluation.RequiresApproval; + + public override T Accept(IAccessConditionVisitor visitor) => visitor.VisitHumanApproval(this); +} diff --git a/bitwarden_license/src/Services/Pam/Models/Conditions/IAccessConditionVisitor.cs b/bitwarden_license/src/Services/Pam/Models/Conditions/IAccessConditionVisitor.cs new file mode 100644 index 000000000000..9da4ef6e6951 --- /dev/null +++ b/bitwarden_license/src/Services/Pam/Models/Conditions/IAccessConditionVisitor.cs @@ -0,0 +1,15 @@ +namespace Bit.Services.Pam.Models.Conditions; + +/// +/// A type-safe operation over the closed set of kinds, dispatched via +/// . Implemented once per operation — the engine evaluates a condition, the +/// validator checks it is well-formed — with 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. +/// +public interface IAccessConditionVisitor +{ + T VisitHumanApproval(HumanApprovalCondition condition); + T VisitIpAllowlist(IpAllowlistCondition condition); + T VisitTimeOfDay(TimeOfDayCondition condition); +} diff --git a/bitwarden_license/src/Services/Pam/Models/Conditions/IpAllowlistCondition.cs b/bitwarden_license/src/Services/Pam/Models/Conditions/IpAllowlistCondition.cs index c73b762b1822..d03335baf8cc 100644 --- a/bitwarden_license/src/Services/Pam/Models/Conditions/IpAllowlistCondition.cs +++ b/bitwarden_license/src/Services/Pam/Models/Conditions/IpAllowlistCondition.cs @@ -1,8 +1,17 @@ -namespace Bit.Services.Pam.Models.Conditions; +using System.Net; +using Bit.Services.Pam.Engine; + +namespace Bit.Services.Pam.Models.Conditions; /// /// Auto-approves a lease when the requester's IP matches a listed CIDR; otherwise denies. /// +/// +/// Wire format: +/// +/// { "kind": "ip_allowlist", "cidrs": ["10.0.0.0/8", "2001:db8::/32"] } +/// +/// public sealed class IpAllowlistCondition : AccessCondition { /// @@ -10,4 +19,25 @@ public sealed class IpAllowlistCondition : AccessCondition /// IP is in any one of them. At least one required, and each must parse; an empty list denies. /// public IReadOnlyList 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(IAccessConditionVisitor visitor) => visitor.VisitIpAllowlist(this); } diff --git a/bitwarden_license/src/Services/Pam/Models/Conditions/TimeOfDayCondition.cs b/bitwarden_license/src/Services/Pam/Models/Conditions/TimeOfDayCondition.cs index c4ce4096db80..6e414fadaee8 100644 --- a/bitwarden_license/src/Services/Pam/Models/Conditions/TimeOfDayCondition.cs +++ b/bitwarden_license/src/Services/Pam/Models/Conditions/TimeOfDayCondition.cs @@ -1,10 +1,23 @@ -using Bit.Services.Pam.Enums; +using System.Globalization; +using Bit.Services.Pam.Engine; +using Bit.Services.Pam.Enums; + namespace Bit.Services.Pam.Models.Conditions; /// /// Auto-approves a lease when the request falls inside one of the configured windows, evaluated in /// the named IANA timezone; otherwise denies. /// +/// +/// Wire format: +/// +/// { +/// "kind": "time_of_day", +/// "tz": "America/New_York", +/// "windows": [{ "days": ["mon", "tue", "wed", "thu", "fri"], "from": "09:00", "to": "17:00" }] +/// } +/// +/// public sealed class TimeOfDayCondition : AccessCondition { /// The IANA timezone (e.g. "America/New_York") the windows are evaluated in. An unknown or invalid zone denies. @@ -12,6 +25,23 @@ public sealed class TimeOfDayCondition : AccessCondition /// The windows that grant access; the condition allows when the instant falls in any one of them. At least one required. public IReadOnlyList 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(IAccessConditionVisitor visitor) => visitor.VisitTimeOfDay(this); } /// @@ -28,4 +58,21 @@ public sealed class TimeWindow /// Window end, 24-hour HH:mm (00:00–23:59). Inclusive. public string To { get; init; } = string.Empty; + + /// + /// Whether this window admits the given and (both already in the + /// parent condition's timezone). Unparseable bounds admit nothing, so the window fails closed. + /// + 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; + } } diff --git a/bitwarden_license/src/Services/Pam/Services/AccessRuleValidator.cs b/bitwarden_license/src/Services/Pam/Services/AccessRuleValidator.cs index 032d1b80fded..972467023fff 100644 --- a/bitwarden_license/src/Services/Pam/Services/AccessRuleValidator.cs +++ b/bitwarden_license/src/Services/Pam/Services/AccessRuleValidator.cs @@ -9,6 +9,9 @@ public sealed partial class AccessRuleValidator : IAccessRuleValidator { private const int MaxConditions = 10; + // Stateless, so one shared instance serves every call. + private static readonly ConditionValidator _conditionValidator = new(); + [GeneratedRegex(@"^([01][0-9]|2[0-3]):[0-5][0-9]$")] private static partial Regex TimeOfDayRegex(); @@ -51,81 +54,84 @@ public AccessRuleValidationResult Validate(string? conditionsJson) ?? AccessRuleValidationResult.Valid; } - private static AccessRuleValidationResult ValidateCondition(AccessCondition? condition) - { - return condition switch - { - HumanApprovalCondition => AccessRuleValidationResult.Valid, - IpAllowlistCondition ip => ValidateIpAllowlist(ip), - TimeOfDayCondition tod => ValidateTimeOfDay(tod), - null => AccessRuleValidationResult.Invalid("Conditions cannot contain a null entry."), - _ => AccessRuleValidationResult.Invalid($"Unsupported condition kind: {condition.GetType().Name}."), - }; - } + private static AccessRuleValidationResult ValidateCondition(AccessCondition? condition) => + condition is null + ? AccessRuleValidationResult.Invalid("Conditions cannot contain a null entry.") + : condition.Accept(_conditionValidator); - private static AccessRuleValidationResult ValidateIpAllowlist(IpAllowlistCondition condition) + /// + /// Checks a single condition is well-formed. Stateless, mirroring the engine's evaluator: neither public + /// service is itself a visitor; each delegates to a private one. + /// + private sealed class ConditionValidator : IAccessConditionVisitor { - if (condition.Cidrs.Count == 0) - { - return AccessRuleValidationResult.Invalid("ip_allowlist requires at least one CIDR."); - } + public AccessRuleValidationResult VisitHumanApproval(HumanApprovalCondition condition) => + AccessRuleValidationResult.Valid; - foreach (var cidr in condition.Cidrs) + public AccessRuleValidationResult VisitIpAllowlist(IpAllowlistCondition condition) { - if (string.IsNullOrWhiteSpace(cidr) || !IPNetwork.TryParse(cidr, out _)) + if (condition.Cidrs.Count == 0) { - return AccessRuleValidationResult.Invalid($"Invalid CIDR: '{cidr}'."); + return AccessRuleValidationResult.Invalid("ip_allowlist requires at least one CIDR."); } - } - - return AccessRuleValidationResult.Valid; - } - private static AccessRuleValidationResult ValidateTimeOfDay(TimeOfDayCondition condition) - { - if (string.IsNullOrWhiteSpace(condition.Tz)) - { - return AccessRuleValidationResult.Invalid("time_of_day requires a tz."); - } + foreach (var cidr in condition.Cidrs) + { + if (string.IsNullOrWhiteSpace(cidr) || !IPNetwork.TryParse(cidr, out _)) + { + return AccessRuleValidationResult.Invalid($"Invalid CIDR: '{cidr}'."); + } + } - try - { - TimeZoneInfo.FindSystemTimeZoneById(condition.Tz); - } - catch (TimeZoneNotFoundException) - { - return AccessRuleValidationResult.Invalid($"Unknown timezone: '{condition.Tz}'."); - } - catch (InvalidTimeZoneException) - { - return AccessRuleValidationResult.Invalid($"Invalid timezone: '{condition.Tz}'."); + return AccessRuleValidationResult.Valid; } - if (condition.Windows.Count == 0) + public AccessRuleValidationResult VisitTimeOfDay(TimeOfDayCondition condition) { - return AccessRuleValidationResult.Invalid("time_of_day requires at least one window."); - } + if (string.IsNullOrWhiteSpace(condition.Tz)) + { + return AccessRuleValidationResult.Invalid("time_of_day requires a tz."); + } - foreach (var window in condition.Windows) - { - if (window.Days.Count == 0) + try + { + TimeZoneInfo.FindSystemTimeZoneById(condition.Tz); + } + catch (TimeZoneNotFoundException) + { + return AccessRuleValidationResult.Invalid($"Unknown timezone: '{condition.Tz}'."); + } + catch (InvalidTimeZoneException) { - return AccessRuleValidationResult.Invalid("time_of_day window requires at least one day."); + return AccessRuleValidationResult.Invalid($"Invalid timezone: '{condition.Tz}'."); } - // Day tokens are validated during deserialization by AccessWeekdayJsonConverter; an unknown token fails - // the JSON parse above and is reported as malformed. - if (!TimeOfDayRegex().IsMatch(window.From)) + if (condition.Windows.Count == 0) { - return AccessRuleValidationResult.Invalid($"Invalid 'from' time: '{window.From}'. Expected HH:mm."); + return AccessRuleValidationResult.Invalid("time_of_day requires at least one window."); } - if (!TimeOfDayRegex().IsMatch(window.To)) + foreach (var window in condition.Windows) { - return AccessRuleValidationResult.Invalid($"Invalid 'to' time: '{window.To}'. Expected HH:mm."); + if (window.Days.Count == 0) + { + return AccessRuleValidationResult.Invalid("time_of_day window requires at least one day."); + } + + // Day tokens are validated during deserialization by AccessWeekdayJsonConverter; an unknown token + // fails the JSON parse above and is reported as malformed. + if (!TimeOfDayRegex().IsMatch(window.From)) + { + return AccessRuleValidationResult.Invalid($"Invalid 'from' time: '{window.From}'. Expected HH:mm."); + } + + if (!TimeOfDayRegex().IsMatch(window.To)) + { + return AccessRuleValidationResult.Invalid($"Invalid 'to' time: '{window.To}'. Expected HH:mm."); + } } - } - return AccessRuleValidationResult.Valid; + return AccessRuleValidationResult.Valid; + } } } diff --git a/bitwarden_license/test/Services/Pam.Test/Engine/AccessRuleEngineTests.cs b/bitwarden_license/test/Services/Pam.Test/Engine/AccessRuleEngineTests.cs index 126758e6d92d..4d5dc8e8b4ff 100644 --- a/bitwarden_license/test/Services/Pam.Test/Engine/AccessRuleEngineTests.cs +++ b/bitwarden_license/test/Services/Pam.Test/Engine/AccessRuleEngineTests.cs @@ -206,9 +206,11 @@ public void Evaluate_NoConditions_Allows() } [Fact] - public void Evaluate_UnsupportedConditionKind_DeniesClosed() + public void Evaluate_NullConditionEntry_DeniesClosed() { - var evaluation = _sut.Evaluate(Set(new UnknownCondition()), Signals()); + // A null entry (only reachable from a malformed stored document) cannot be evaluated, so it fails closed. + // An unknown condition kind can no longer reach the engine: visitor dispatch is exhaustive at compile time. + var evaluation = _sut.Evaluate([null!], Signals()); Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); Assert.Equal(DenyReason.UnsupportedCondition, evaluation.Reason); @@ -271,6 +273,4 @@ public void Evaluate_TimeOfDay_LaterWindowMatches_Allows() Assert.Equal(AccessEvaluationOutcome.Allow, evaluation.Outcome); } - - private sealed class UnknownCondition : AccessCondition; } From 8b9c57865f8ce41ab5b5ff9b65aa0c0245910a7f Mon Sep 17 00:00:00 2001 From: Hinton Date: Fri, 17 Jul 2026 11:08:17 +0200 Subject: [PATCH 07/10] Relocate condition tests onto the conditions Move each condition's evaluation branches out of AccessRuleEngineTests into IpAllowlistConditionTests and TimeOfDayConditionTests, and test the combination semantics directly on AccessEvaluation.Combine. AccessRuleEngineTests keeps only the engine's orchestration (delegation, folding, signal forwarding, empty list, null-entry fail-closed), exercised with a stub condition. Also add direct TimeWindow.Contains coverage for the inclusive boundaries (time == from/to) and unparseable bounds, which the engine tests never hit. --- .../Pam.Test/Engine/AccessEvaluationTests.cs | 72 +++++ .../Pam.Test/Engine/AccessRuleEngineTests.cs | 256 +++--------------- .../Conditions/IpAllowlistConditionTests.cs | 80 ++++++ .../Conditions/TimeOfDayConditionTests.cs | 180 ++++++++++++ 4 files changed, 365 insertions(+), 223 deletions(-) create mode 100644 bitwarden_license/test/Services/Pam.Test/Engine/AccessEvaluationTests.cs create mode 100644 bitwarden_license/test/Services/Pam.Test/Models/Conditions/IpAllowlistConditionTests.cs create mode 100644 bitwarden_license/test/Services/Pam.Test/Models/Conditions/TimeOfDayConditionTests.cs diff --git a/bitwarden_license/test/Services/Pam.Test/Engine/AccessEvaluationTests.cs b/bitwarden_license/test/Services/Pam.Test/Engine/AccessEvaluationTests.cs new file mode 100644 index 000000000000..9810cae99cd4 --- /dev/null +++ b/bitwarden_license/test/Services/Pam.Test/Engine/AccessEvaluationTests.cs @@ -0,0 +1,72 @@ +using Bit.Services.Pam.Engine; +using Xunit; + +namespace Bit.Services.Pam.Test.Engine; + +public class AccessEvaluationTests +{ + [Fact] + public void Combine_Empty_Allows() + { + // An empty rule is vacuously satisfied. + Assert.Equal(AccessEvaluationOutcome.Allow, AccessEvaluation.Combine([]).Outcome); + } + + [Fact] + public void Combine_AllAllow_Allows() + { + var result = AccessEvaluation.Combine([AccessEvaluation.Allow, AccessEvaluation.Allow]); + + Assert.Equal(AccessEvaluationOutcome.Allow, result.Outcome); + } + + [Fact] + public void Combine_ApprovalOverAllow_RequiresApproval() + { + var result = AccessEvaluation.Combine([AccessEvaluation.Allow, AccessEvaluation.RequiresApproval]); + + Assert.Equal(AccessEvaluationOutcome.RequiresApproval, result.Outcome); + } + + [Fact] + public void Combine_DenyOutranksAllow_Denies() + { + var result = AccessEvaluation.Combine([AccessEvaluation.Allow, AccessEvaluation.Deny(DenyReason.NotWithinIpRange)]); + + Assert.Equal(AccessEvaluationOutcome.Deny, result.Outcome); + Assert.Equal(DenyReason.NotWithinIpRange, result.Reason); + } + + [Fact] + public void Combine_DenyOutranksApproval_Denies() + { + // Deny beats a pending approval regardless of order: there is nothing to approve if access is barred. + var result = AccessEvaluation.Combine([AccessEvaluation.RequiresApproval, AccessEvaluation.Deny(DenyReason.NotWithinTimeWindow)]); + + Assert.Equal(AccessEvaluationOutcome.Deny, result.Outcome); + Assert.Equal(DenyReason.NotWithinTimeWindow, result.Reason); + } + + [Fact] + public void Combine_ApprovalAfterDeny_StillDenies() + { + var result = AccessEvaluation.Combine([AccessEvaluation.Deny(DenyReason.NotWithinIpRange), AccessEvaluation.RequiresApproval]); + + Assert.Equal(AccessEvaluationOutcome.Deny, result.Outcome); + Assert.Equal(DenyReason.NotWithinIpRange, result.Reason); + } + + [Fact] + public void Combine_FirstDenyWins_PreservesItsReason() + { + // Combine short-circuits on the first deny, so its reason is the one reported. + var result = AccessEvaluation.Combine( + [ + AccessEvaluation.Deny(DenyReason.NotWithinTimeWindow), + AccessEvaluation.Deny(DenyReason.NotWithinIpRange), + ]); + + Assert.Equal(AccessEvaluationOutcome.Deny, result.Outcome); + Assert.Equal(DenyReason.NotWithinTimeWindow, result.Reason); + } +} diff --git a/bitwarden_license/test/Services/Pam.Test/Engine/AccessRuleEngineTests.cs b/bitwarden_license/test/Services/Pam.Test/Engine/AccessRuleEngineTests.cs index 4d5dc8e8b4ff..43a554b07161 100644 --- a/bitwarden_license/test/Services/Pam.Test/Engine/AccessRuleEngineTests.cs +++ b/bitwarden_license/test/Services/Pam.Test/Engine/AccessRuleEngineTests.cs @@ -1,6 +1,5 @@ using System.Net; using Bit.Services.Pam.Engine; -using Bit.Services.Pam.Enums; using Bit.Services.Pam.Models.Conditions; using Xunit; @@ -8,269 +7,80 @@ namespace Bit.Services.Pam.Test.Engine; public class AccessRuleEngineTests { - // 2026-06-04T12:00:00Z is a Thursday, so "thu" windows match in UTC. - private static readonly DateTimeOffset _now = new(2026, 6, 4, 12, 0, 0, TimeSpan.Zero); - private readonly AccessRuleEngine _sut = new(); - private static AccessSignals Signals(IPAddress? ip = null, DateTimeOffset? at = null) => new() + private static AccessSignals Signals() => new() { - IpAddress = ip, - Timestamp = at ?? _now, + IpAddress = IPAddress.Parse("10.1.2.3"), + Timestamp = new DateTimeOffset(2026, 6, 4, 12, 0, 0, TimeSpan.Zero), }; - private static AccessCondition[] Set(params AccessCondition[] conditions) => conditions; - - [Fact] - public void Evaluate_HumanApproval_RequiresApproval() - { - var evaluation = _sut.Evaluate(Set(new HumanApprovalCondition()), Signals()); - - Assert.Equal(AccessEvaluationOutcome.RequiresApproval, evaluation.Outcome); - } - - [Fact] - public void Evaluate_IpAllowlist_IpInRange_Allows() - { - var conditions = Set(new IpAllowlistCondition { Cidrs = ["10.0.0.0/8"] }); - - var evaluation = _sut.Evaluate(conditions, Signals(IPAddress.Parse("10.1.2.3"))); - - Assert.Equal(AccessEvaluationOutcome.Allow, evaluation.Outcome); - } - - [Fact] - public void Evaluate_IpAllowlist_IpOutOfRange_Denies() - { - var conditions = Set(new IpAllowlistCondition { Cidrs = ["10.0.0.0/8"] }); - - var evaluation = _sut.Evaluate(conditions, Signals(IPAddress.Parse("192.168.1.1"))); - - Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); - Assert.Equal(DenyReason.NotWithinIpRange, evaluation.Reason); - } - - [Fact] - public void Evaluate_IpAllowlist_UnknownIp_DeniesClosed() - { - var conditions = Set(new IpAllowlistCondition { Cidrs = ["10.0.0.0/8"] }); - - var evaluation = _sut.Evaluate(conditions, Signals(ip: null)); - - Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); - Assert.Equal(DenyReason.NotWithinIpRange, evaluation.Reason); - } - - [Fact] - public void Evaluate_IpAllowlist_NoEntries_DeniesClosed() - { - var evaluation = _sut.Evaluate(Set(new IpAllowlistCondition()), Signals(IPAddress.Parse("10.1.2.3"))); - - Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); - Assert.Equal(DenyReason.NotWithinIpRange, evaluation.Reason); - } - - [Fact] - public void Evaluate_TimeOfDay_WithinWindow_Allows() - { - var conditions = Set(new TimeOfDayCondition - { - Tz = "UTC", - Windows = [new TimeWindow { Days = [AccessWeekday.Thu], From = "09:00", To = "17:00" }], - }); - - var evaluation = _sut.Evaluate(conditions, Signals()); - - Assert.Equal(AccessEvaluationOutcome.Allow, evaluation.Outcome); - } - - [Fact] - public void Evaluate_TimeOfDay_OutsideTimeWindow_Denies() - { - var conditions = Set(new TimeOfDayCondition - { - Tz = "UTC", - Windows = [new TimeWindow { Days = [AccessWeekday.Thu], From = "00:00", To = "06:00" }], - }); - - var evaluation = _sut.Evaluate(conditions, Signals()); - - Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); - Assert.Equal(DenyReason.NotWithinTimeWindow, evaluation.Reason); - } - [Fact] - public void Evaluate_TimeOfDay_DayNotListed_Denies() + public void Evaluate_NoConditions_Allows() { - var conditions = Set(new TimeOfDayCondition - { - Tz = "UTC", - Windows = [new TimeWindow { Days = [AccessWeekday.Fri], From = "00:00", To = "23:59" }], - }); - - var evaluation = _sut.Evaluate(conditions, Signals()); - - Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); - Assert.Equal(DenyReason.NotWithinTimeWindow, evaluation.Reason); + // A rule with no conditions is vacuously satisfied: access is auto-granted while still flowing through + // PAM for audit logging. + Assert.Equal(AccessEvaluationOutcome.Allow, _sut.Evaluate([], Signals()).Outcome); } [Fact] - public void Evaluate_TimeOfDay_EvaluatesInConfiguredTimezone() + public void Evaluate_DefersToEachConditionsOwnResult() { - // 23:00 UTC is 19:00 (Thursday) in America/New_York during June DST, inside the window. - var conditions = Set(new TimeOfDayCondition - { - Tz = "America/New_York", - Windows = [new TimeWindow { Days = [AccessWeekday.Thu], From = "18:00", To = "20:00" }], - }); - - var evaluation = _sut.Evaluate(conditions, Signals(at: new DateTimeOffset(2026, 6, 4, 23, 0, 0, TimeSpan.Zero))); - - Assert.Equal(AccessEvaluationOutcome.Allow, evaluation.Outcome); + // The engine does not decide anything itself; it returns what the condition's Evaluate reports. + Assert.Equal(AccessEvaluationOutcome.RequiresApproval, + _sut.Evaluate([new StubCondition(AccessEvaluation.RequiresApproval)], Signals()).Outcome); } [Fact] - public void Evaluate_TimeOfDay_UnknownTimezone_DeniesClosed() + public void Evaluate_CombinesConditionResults_DenyWins() { - var conditions = Set(new TimeOfDayCondition + // Folding is delegated to AccessEvaluation.Combine (deny > approval > allow); a single denying condition + // drives the whole rule to deny. The full precedence matrix is covered in AccessEvaluationTests. + var conditions = new AccessCondition[] { - Tz = "Not/AZone", - Windows = [new TimeWindow { Days = [AccessWeekday.Thu], From = "00:00", To = "23:59" }], - }); + new StubCondition(AccessEvaluation.Allow), + new StubCondition(AccessEvaluation.Deny(DenyReason.NotWithinIpRange)), + }; var evaluation = _sut.Evaluate(conditions, Signals()); - Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); - Assert.Equal(DenyReason.NotWithinTimeWindow, evaluation.Reason); - } - - [Fact] - public void Evaluate_AllConditionsAllow_Allows() - { - var conditions = Set( - new IpAllowlistCondition { Cidrs = ["10.0.0.0/8"] }, - new TimeOfDayCondition { Tz = "UTC", Windows = [new TimeWindow { Days = [AccessWeekday.Thu], From = "09:00", To = "17:00" }] }); - - var evaluation = _sut.Evaluate(conditions, Signals(IPAddress.Parse("10.1.2.3"))); - - Assert.Equal(AccessEvaluationOutcome.Allow, evaluation.Outcome); - } - - [Fact] - public void Evaluate_OneConditionDenies_DeniesWithThatReason() - { - var conditions = Set( - new IpAllowlistCondition { Cidrs = ["10.0.0.0/8"] }, - new TimeOfDayCondition { Tz = "UTC", Windows = [new TimeWindow { Days = [AccessWeekday.Thu], From = "00:00", To = "06:00" }] }); - - var evaluation = _sut.Evaluate(conditions, Signals(IPAddress.Parse("10.1.2.3"))); - - Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); - Assert.Equal(DenyReason.NotWithinTimeWindow, evaluation.Reason); - } - - [Fact] - public void Evaluate_AllowPlusHumanApproval_RequiresApproval() - { - var conditions = Set( - new IpAllowlistCondition { Cidrs = ["10.0.0.0/8"] }, - new HumanApprovalCondition()); - - var evaluation = _sut.Evaluate(conditions, Signals(IPAddress.Parse("10.1.2.3"))); - - Assert.Equal(AccessEvaluationOutcome.RequiresApproval, evaluation.Outcome); - } - - [Fact] - public void Evaluate_DenyOutranksApproval() - { - // A denying condition beats a pending approval: there is nothing to approve if access is barred outright. - var conditions = Set( - new HumanApprovalCondition(), - new IpAllowlistCondition { Cidrs = ["10.0.0.0/8"] }); - - var evaluation = _sut.Evaluate(conditions, Signals(IPAddress.Parse("192.168.1.1"))); - Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); Assert.Equal(DenyReason.NotWithinIpRange, evaluation.Reason); } [Fact] - public void Evaluate_NoConditions_Allows() + public void Evaluate_ForwardsSignalsToConditions() { - // A rule with no conditions is vacuously satisfied: access is auto-granted while still flowing through - // PAM for audit logging. - var evaluation = _sut.Evaluate(Set(), Signals()); + var signals = Signals(); + var condition = new StubCondition(AccessEvaluation.Allow); + + _sut.Evaluate([condition], signals); - Assert.Equal(AccessEvaluationOutcome.Allow, evaluation.Outcome); + Assert.Same(signals, condition.ReceivedSignals); } [Fact] public void Evaluate_NullConditionEntry_DeniesClosed() { // A null entry (only reachable from a malformed stored document) cannot be evaluated, so it fails closed. - // An unknown condition kind can no longer reach the engine: visitor dispatch is exhaustive at compile time. + // An unknown condition kind can no longer reach the engine: it is rejected at JSON deserialization. var evaluation = _sut.Evaluate([null!], Signals()); Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); Assert.Equal(DenyReason.UnsupportedCondition, evaluation.Reason); } - [Fact] - public void Evaluate_IpAllowlist_MalformedCidr_DeniesClosed() + /// A condition with a fixed result, isolating the engine's folding from any real condition logic. + private sealed class StubCondition(AccessEvaluation result) : AccessCondition { - // A present-but-unparseable CIDR matches no address, so a caller with a known IP still fails closed. - var conditions = Set(new IpAllowlistCondition { Cidrs = ["not-a-cidr"] }); - - var evaluation = _sut.Evaluate(conditions, Signals(IPAddress.Parse("10.1.2.3"))); + public AccessSignals? ReceivedSignals { get; private set; } - Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); - Assert.Equal(DenyReason.NotWithinIpRange, evaluation.Reason); - } - - [Fact] - public void Evaluate_IpAllowlist_LaterCidrMatches_Allows() - { - // The caller matches the second entry, so evaluation must not stop at the first non-matching CIDR. - var conditions = Set(new IpAllowlistCondition { Cidrs = ["192.168.0.0/16", "10.0.0.0/8"] }); - - var evaluation = _sut.Evaluate(conditions, Signals(IPAddress.Parse("10.1.2.3"))); - - Assert.Equal(AccessEvaluationOutcome.Allow, evaluation.Outcome); - } - - [Fact] - public void Evaluate_TimeOfDay_MalformedWindowTime_DeniesClosed() - { - // The day matches but the window's bounds are unparseable, so the window cannot admit the caller. - var conditions = Set(new TimeOfDayCondition - { - Tz = "UTC", - Windows = [new TimeWindow { Days = [AccessWeekday.Thu], From = "25:99", To = "30:00" }], - }); - - var evaluation = _sut.Evaluate(conditions, Signals()); - - Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); - Assert.Equal(DenyReason.NotWithinTimeWindow, evaluation.Reason); - } - - [Fact] - public void Evaluate_TimeOfDay_LaterWindowMatches_Allows() - { - // The first window is the wrong day; the second admits the caller, so windows past the first must be checked. - var conditions = Set(new TimeOfDayCondition + public override AccessEvaluation Evaluate(AccessSignals signals) { - Tz = "UTC", - Windows = - [ - new TimeWindow { Days = [AccessWeekday.Fri], From = "09:00", To = "17:00" }, - new TimeWindow { Days = [AccessWeekday.Thu], From = "09:00", To = "17:00" }, - ], - }); - - var evaluation = _sut.Evaluate(conditions, Signals()); + ReceivedSignals = signals; + return result; + } - Assert.Equal(AccessEvaluationOutcome.Allow, evaluation.Outcome); + public override T Accept(IAccessConditionVisitor visitor) => throw new NotSupportedException(); } } diff --git a/bitwarden_license/test/Services/Pam.Test/Models/Conditions/IpAllowlistConditionTests.cs b/bitwarden_license/test/Services/Pam.Test/Models/Conditions/IpAllowlistConditionTests.cs new file mode 100644 index 000000000000..46adfaa07c8c --- /dev/null +++ b/bitwarden_license/test/Services/Pam.Test/Models/Conditions/IpAllowlistConditionTests.cs @@ -0,0 +1,80 @@ +using System.Net; +using Bit.Services.Pam.Engine; +using Bit.Services.Pam.Models.Conditions; +using Xunit; + +namespace Bit.Services.Pam.Test.Models.Conditions; + +public class IpAllowlistConditionTests +{ + private static AccessSignals Signals(IPAddress? ip) => new() + { + IpAddress = ip, + Timestamp = new DateTimeOffset(2026, 6, 4, 12, 0, 0, TimeSpan.Zero), + }; + + [Fact] + public void Evaluate_IpInRange_Allows() + { + var condition = new IpAllowlistCondition { Cidrs = ["10.0.0.0/8"] }; + + var evaluation = condition.Evaluate(Signals(IPAddress.Parse("10.1.2.3"))); + + Assert.Equal(AccessEvaluationOutcome.Allow, evaluation.Outcome); + } + + [Fact] + public void Evaluate_IpOutOfRange_Denies() + { + var condition = new IpAllowlistCondition { Cidrs = ["10.0.0.0/8"] }; + + var evaluation = condition.Evaluate(Signals(IPAddress.Parse("192.168.1.1"))); + + Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); + Assert.Equal(DenyReason.NotWithinIpRange, evaluation.Reason); + } + + [Fact] + public void Evaluate_UnknownIp_DeniesClosed() + { + var condition = new IpAllowlistCondition { Cidrs = ["10.0.0.0/8"] }; + + var evaluation = condition.Evaluate(Signals(ip: null)); + + Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); + Assert.Equal(DenyReason.NotWithinIpRange, evaluation.Reason); + } + + [Fact] + public void Evaluate_NoEntries_DeniesClosed() + { + // An allowlist with no entries permits no address. + var evaluation = new IpAllowlistCondition().Evaluate(Signals(IPAddress.Parse("10.1.2.3"))); + + Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); + Assert.Equal(DenyReason.NotWithinIpRange, evaluation.Reason); + } + + [Fact] + public void Evaluate_MalformedCidr_DeniesClosed() + { + // A present-but-unparseable CIDR matches no address, so a caller with a known IP still fails closed. + var condition = new IpAllowlistCondition { Cidrs = ["not-a-cidr"] }; + + var evaluation = condition.Evaluate(Signals(IPAddress.Parse("10.1.2.3"))); + + Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); + Assert.Equal(DenyReason.NotWithinIpRange, evaluation.Reason); + } + + [Fact] + public void Evaluate_LaterCidrMatches_Allows() + { + // The caller matches the second entry, so evaluation must not stop at the first non-matching CIDR. + var condition = new IpAllowlistCondition { Cidrs = ["192.168.0.0/16", "10.0.0.0/8"] }; + + var evaluation = condition.Evaluate(Signals(IPAddress.Parse("10.1.2.3"))); + + Assert.Equal(AccessEvaluationOutcome.Allow, evaluation.Outcome); + } +} diff --git a/bitwarden_license/test/Services/Pam.Test/Models/Conditions/TimeOfDayConditionTests.cs b/bitwarden_license/test/Services/Pam.Test/Models/Conditions/TimeOfDayConditionTests.cs new file mode 100644 index 000000000000..7511613d483a --- /dev/null +++ b/bitwarden_license/test/Services/Pam.Test/Models/Conditions/TimeOfDayConditionTests.cs @@ -0,0 +1,180 @@ +using Bit.Services.Pam.Engine; +using Bit.Services.Pam.Enums; +using Bit.Services.Pam.Models.Conditions; +using Xunit; + +namespace Bit.Services.Pam.Test.Models.Conditions; + +public class TimeOfDayConditionTests +{ + // 2026-06-04T12:00:00Z is a Thursday, so "thu" windows match in UTC. + private static readonly DateTimeOffset _now = new(2026, 6, 4, 12, 0, 0, TimeSpan.Zero); + + private static AccessSignals Signals(DateTimeOffset? at = null) => new() + { + IpAddress = null, + Timestamp = at ?? _now, + }; + + [Fact] + public void Evaluate_WithinWindow_Allows() + { + var condition = new TimeOfDayCondition + { + Tz = "UTC", + Windows = [new TimeWindow { Days = [AccessWeekday.Thu], From = "09:00", To = "17:00" }], + }; + + Assert.Equal(AccessEvaluationOutcome.Allow, condition.Evaluate(Signals()).Outcome); + } + + [Fact] + public void Evaluate_OutsideWindow_Denies() + { + var condition = new TimeOfDayCondition + { + Tz = "UTC", + Windows = [new TimeWindow { Days = [AccessWeekday.Thu], From = "00:00", To = "06:00" }], + }; + + var evaluation = condition.Evaluate(Signals()); + + Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); + Assert.Equal(DenyReason.NotWithinTimeWindow, evaluation.Reason); + } + + [Fact] + public void Evaluate_DayNotListed_Denies() + { + var condition = new TimeOfDayCondition + { + Tz = "UTC", + Windows = [new TimeWindow { Days = [AccessWeekday.Fri], From = "00:00", To = "23:59" }], + }; + + var evaluation = condition.Evaluate(Signals()); + + Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); + Assert.Equal(DenyReason.NotWithinTimeWindow, evaluation.Reason); + } + + [Fact] + public void Evaluate_EvaluatesInConfiguredTimezone_Allows() + { + // 23:00 UTC is 19:00 (Thursday) in America/New_York during June DST, inside the window. + var condition = new TimeOfDayCondition + { + Tz = "America/New_York", + Windows = [new TimeWindow { Days = [AccessWeekday.Thu], From = "18:00", To = "20:00" }], + }; + + var evaluation = condition.Evaluate(Signals(new DateTimeOffset(2026, 6, 4, 23, 0, 0, TimeSpan.Zero))); + + Assert.Equal(AccessEvaluationOutcome.Allow, evaluation.Outcome); + } + + [Fact] + public void Evaluate_UnknownTimezone_DeniesClosed() + { + var condition = new TimeOfDayCondition + { + Tz = "Not/AZone", + Windows = [new TimeWindow { Days = [AccessWeekday.Thu], From = "00:00", To = "23:59" }], + }; + + var evaluation = condition.Evaluate(Signals()); + + Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); + Assert.Equal(DenyReason.NotWithinTimeWindow, evaluation.Reason); + } + + [Fact] + public void Evaluate_MalformedWindowTime_DeniesClosed() + { + // The day matches but the window's bounds are unparseable, so the window cannot admit the caller. + var condition = new TimeOfDayCondition + { + Tz = "UTC", + Windows = [new TimeWindow { Days = [AccessWeekday.Thu], From = "25:99", To = "30:00" }], + }; + + var evaluation = condition.Evaluate(Signals()); + + Assert.Equal(AccessEvaluationOutcome.Deny, evaluation.Outcome); + Assert.Equal(DenyReason.NotWithinTimeWindow, evaluation.Reason); + } + + [Fact] + public void Evaluate_LaterWindowMatches_Allows() + { + // The first window is the wrong day; the second admits the caller, so windows past the first must be checked. + var condition = new TimeOfDayCondition + { + Tz = "UTC", + Windows = + [ + new TimeWindow { Days = [AccessWeekday.Fri], From = "09:00", To = "17:00" }, + new TimeWindow { Days = [AccessWeekday.Thu], From = "09:00", To = "17:00" }, + ], + }; + + Assert.Equal(AccessEvaluationOutcome.Allow, condition.Evaluate(Signals()).Outcome); + } +} + +public class TimeWindowContainsTests +{ + private static TimeWindow BusinessHours() => new() + { + Days = [AccessWeekday.Thu], + From = "09:00", + To = "17:00", + }; + + [Fact] + public void Contains_TimeEqualsFrom_IsInclusive() + { + // The lower bound is inclusive. + Assert.True(BusinessHours().Contains(DayOfWeek.Thursday, new TimeOnly(9, 0))); + } + + [Fact] + public void Contains_TimeEqualsTo_IsInclusive() + { + // The upper bound is inclusive. + Assert.True(BusinessHours().Contains(DayOfWeek.Thursday, new TimeOnly(17, 0))); + } + + [Fact] + public void Contains_TimeInside_True() + { + Assert.True(BusinessHours().Contains(DayOfWeek.Thursday, new TimeOnly(12, 0))); + } + + [Fact] + public void Contains_TimeJustBeforeFrom_False() + { + Assert.False(BusinessHours().Contains(DayOfWeek.Thursday, new TimeOnly(8, 59))); + } + + [Fact] + public void Contains_TimeJustAfterTo_False() + { + Assert.False(BusinessHours().Contains(DayOfWeek.Thursday, new TimeOnly(17, 1))); + } + + [Fact] + public void Contains_DayNotListed_False() + { + Assert.False(BusinessHours().Contains(DayOfWeek.Friday, new TimeOnly(12, 0))); + } + + [Fact] + public void Contains_UnparseableBounds_FailsClosed() + { + // Bounds that are not HH:mm admit nothing, even on a matching day and a plausible time. + var window = new TimeWindow { Days = [AccessWeekday.Thu], From = "9am", To = "5pm" }; + + Assert.False(window.Contains(DayOfWeek.Thursday, new TimeOnly(12, 0))); + } +} From 8cda1a071dfc20d65c4a0297fb1b4ddb51f6a0d9 Mon Sep 17 00:00:00 2001 From: Hinton Date: Fri, 17 Jul 2026 11:20:47 +0200 Subject: [PATCH 08/10] Skip disabled access rules when resolving the governing rule AccessRule.Enabled was never checked, so a disabled rule still governed its collections. Because selection is oldest-wins and structural, a disabled but permissive rule (e.g. empty conditions, which auto-grant) could shadow a newer enabled restrictive rule and silently grant access the active rule would have gated. Drop rules with Enabled == false from the candidate set, alongside the existing deleted-rule skip, so a disabled rule stops governing entirely. Pin Enabled in the resolver test fixtures so a governing rule is deterministically enabled (AutoFixture's bool sequence could otherwise disable it), and add coverage for a disabled sole rule (ungoverned) and a disabled oldest rule (newer enabled rule governs). --- .../Pam/Services/GoverningRuleResolver.cs | 8 ++-- .../Services/GoverningRuleResolverTests.cs | 37 +++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/bitwarden_license/src/Services/Pam/Services/GoverningRuleResolver.cs b/bitwarden_license/src/Services/Pam/Services/GoverningRuleResolver.cs index 62ff68027294..226956aef4e5 100644 --- a/bitwarden_license/src/Services/Pam/Services/GoverningRuleResolver.cs +++ b/bitwarden_license/src/Services/Pam/Services/GoverningRuleResolver.cs @@ -43,13 +43,15 @@ public GoverningRuleResolver( .Where(c => collectionIds.Contains(c.Id) && c.AccessRuleId.HasValue); // Load every rule on the collections through which the caller reaches the cipher, keeping each paired with - // the collection it gates. A rule that no longer loads (deleted after the collection was read — deletes clear - // the link, so this is only a race) is skipped, so a deleted rule stops governing. + // the collection it gates. A rule is dropped — so it stops governing — when it is disabled (Enabled is false; + // the admin has switched it off, and a disabled rule does not gate access) or no longer loads (deleted after + // the collection was read; deletes clear the link, so a missing rule is only a race). Dropping a disabled rule + // also stops it shadowing a newer active rule under the oldest-wins selection below. var candidates = new List<(Collection Collection, AccessRule Rule)>(); foreach (var collection in governedCollections) { var accessRule = await _accessRuleRepository.GetByIdAsync(collection.AccessRuleId!.Value); - if (accessRule is not null) + if (accessRule is { Enabled: true }) { candidates.Add((collection, accessRule)); } diff --git a/bitwarden_license/test/Services/Pam.Test/Services/GoverningRuleResolverTests.cs b/bitwarden_license/test/Services/Pam.Test/Services/GoverningRuleResolverTests.cs index e7ed578327bb..1b7eb336730f 100644 --- a/bitwarden_license/test/Services/Pam.Test/Services/GoverningRuleResolverTests.cs +++ b/bitwarden_license/test/Services/Pam.Test/Services/GoverningRuleResolverTests.cs @@ -263,6 +263,7 @@ public async Task ResolveAsync_OldestGovernedRuleDeleted_NextRuleGoverns( olderRule.CreationDate = new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc); newerRule.CreationDate = new DateTime(2026, 6, 1, 0, 0, 0, DateTimeKind.Utc); newerRule.Conditions = """[{"kind":"human_approval"}]"""; + newerRule.Enabled = true; olderCollection.AccessRuleId = olderRule.Id; newerCollection.AccessRuleId = newerRule.Id; SetupReachableCollections(sutProvider, userId, cipherId, olderCollection, newerCollection); @@ -278,6 +279,40 @@ public async Task ResolveAsync_OldestGovernedRuleDeleted_NextRuleGoverns( Assert.True(result.RequiresHumanApproval); } + [Theory, BitAutoData] + public async Task ResolveAsync_DisabledRule_NotGoverned( + SutProvider sutProvider, Guid userId, Guid cipherId, Collection collection, AccessRule rule) + { + // A disabled rule is inactive and does not gate access, so a cipher reached only through it is ungoverned. + SetupGovernedCollection(sutProvider, userId, cipherId, collection, rule); + rule.Enabled = false; + + Assert.Null(await sutProvider.Sut.ResolveAsync(userId, cipherId, _signals)); + } + + [Theory, BitAutoData] + public async Task ResolveAsync_OldestRuleDisabled_NewerEnabledRuleGoverns( + SutProvider sutProvider, Guid userId, Guid cipherId, + Collection olderCollection, AccessRule olderRule, Collection newerCollection, AccessRule newerRule) + { + // The oldest rule is disabled and auto-granting; the newer rule is enabled and needs human approval. A disabled + // rule must not shadow a newer active one, so the newer rule governs — access is not silently auto-granted. + olderRule.CreationDate = new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc); + olderRule.Conditions = "[]"; + newerRule.CreationDate = new DateTime(2026, 6, 1, 0, 0, 0, DateTimeKind.Utc); + newerRule.Conditions = """[{"kind":"human_approval"}]"""; + SetupGovernedCollections(sutProvider, userId, cipherId, + (olderCollection, olderRule), (newerCollection, newerRule)); + olderRule.Enabled = false; + + var result = await sutProvider.Sut.ResolveAsync(userId, cipherId, _signals); + + Assert.NotNull(result); + Assert.Equal(newerCollection.Id, result!.CollectionId); + Assert.Equal(newerRule.Id, result.RuleId); + Assert.True(result.RequiresHumanApproval); + } + [Theory, BitAutoData] public async Task ResolveAsync_TimeOfDayCondition_ParsedAndEvaluatedThroughResolver( SutProvider sutProvider, Guid userId, Guid cipherId, Collection collection, AccessRule rule) @@ -318,6 +353,8 @@ private static void SetupGovernedCollections( foreach (var (collection, rule) in pairs) { collection.AccessRuleId = rule.Id; + // A governing rule must be enabled; pin it so the outcome does not depend on AutoFixture's bool sequence. + rule.Enabled = true; } SetupReachableCollections(sutProvider, userId, cipherId, pairs.Select(p => p.collection).ToArray()); From 3bb204094aa05ca755058f6c9c037208ec2ebc48 Mon Sep 17 00:00:00 2001 From: Hinton Date: Fri, 17 Jul 2026 11:39:45 +0200 Subject: [PATCH 09/10] 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. --- .../Pam/Models/Conditions/AccessCondition.cs | 7 +- .../Conditions/AccessRuleValidationResult.cs | 12 +++ .../Conditions/HumanApprovalCondition.cs | 2 +- .../Conditions/IAccessConditionVisitor.cs | 15 --- .../Models/Conditions/IpAllowlistCondition.cs | 18 +++- .../Models/Conditions/TimeOfDayCondition.cs | 61 +++++++++++- .../Pam/Services/AccessRuleValidator.cs | 93 ++----------------- .../Pam/Services/IAccessRuleValidator.cs | 10 +- .../Commands/CreateAccessRuleCommandTests.cs | 1 + .../Commands/UpdateAccessRuleCommandTests.cs | 1 + .../Pam.Test/Engine/AccessRuleEngineTests.cs | 2 +- 11 files changed, 103 insertions(+), 119 deletions(-) create mode 100644 bitwarden_license/src/Services/Pam/Models/Conditions/AccessRuleValidationResult.cs delete mode 100644 bitwarden_license/src/Services/Pam/Models/Conditions/IAccessConditionVisitor.cs diff --git a/bitwarden_license/src/Services/Pam/Models/Conditions/AccessCondition.cs b/bitwarden_license/src/Services/Pam/Models/Conditions/AccessCondition.cs index 821acfaca498..31987ab3065d 100644 --- a/bitwarden_license/src/Services/Pam/Models/Conditions/AccessCondition.cs +++ b/bitwarden_license/src/Services/Pam/Models/Conditions/AccessCondition.cs @@ -21,9 +21,8 @@ public abstract class AccessCondition public abstract AccessEvaluation Evaluate(AccessSignals signals); /// - /// Double-dispatches to the '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. + /// Checks this condition is well-formed at write time, before it is persisted, returning an actionable error + /// when it is not. The loud, reject-on-save counterpart to 's fail-closed runtime behavior. /// - public abstract T Accept(IAccessConditionVisitor visitor); + public abstract AccessRuleValidationResult Validate(); } diff --git a/bitwarden_license/src/Services/Pam/Models/Conditions/AccessRuleValidationResult.cs b/bitwarden_license/src/Services/Pam/Models/Conditions/AccessRuleValidationResult.cs new file mode 100644 index 000000000000..4a85f3f0abd8 --- /dev/null +++ b/bitwarden_license/src/Services/Pam/Models/Conditions/AccessRuleValidationResult.cs @@ -0,0 +1,12 @@ +namespace Bit.Services.Pam.Models.Conditions; + +/// +/// The result of checking a condition (or a rule's whole conditions document) is well-formed: whether it is valid +/// and, when not, an actionable message. Produced at write time by and by +/// . +/// +public sealed record AccessRuleValidationResult(bool IsValid, string? Error) +{ + public static AccessRuleValidationResult Valid { get; } = new(true, null); + public static AccessRuleValidationResult Invalid(string error) => new(false, error); +} diff --git a/bitwarden_license/src/Services/Pam/Models/Conditions/HumanApprovalCondition.cs b/bitwarden_license/src/Services/Pam/Models/Conditions/HumanApprovalCondition.cs index 50c876b398ce..cb88bd2672cd 100644 --- a/bitwarden_license/src/Services/Pam/Models/Conditions/HumanApprovalCondition.cs +++ b/bitwarden_license/src/Services/Pam/Models/Conditions/HumanApprovalCondition.cs @@ -10,5 +10,5 @@ public sealed class HumanApprovalCondition : AccessCondition { public override AccessEvaluation Evaluate(AccessSignals signals) => AccessEvaluation.RequiresApproval; - public override T Accept(IAccessConditionVisitor visitor) => visitor.VisitHumanApproval(this); + public override AccessRuleValidationResult Validate() => AccessRuleValidationResult.Valid; } diff --git a/bitwarden_license/src/Services/Pam/Models/Conditions/IAccessConditionVisitor.cs b/bitwarden_license/src/Services/Pam/Models/Conditions/IAccessConditionVisitor.cs deleted file mode 100644 index 9da4ef6e6951..000000000000 --- a/bitwarden_license/src/Services/Pam/Models/Conditions/IAccessConditionVisitor.cs +++ /dev/null @@ -1,15 +0,0 @@ -namespace Bit.Services.Pam.Models.Conditions; - -/// -/// A type-safe operation over the closed set of kinds, dispatched via -/// . Implemented once per operation — the engine evaluates a condition, the -/// validator checks it is well-formed — with 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. -/// -public interface IAccessConditionVisitor -{ - T VisitHumanApproval(HumanApprovalCondition condition); - T VisitIpAllowlist(IpAllowlistCondition condition); - T VisitTimeOfDay(TimeOfDayCondition condition); -} diff --git a/bitwarden_license/src/Services/Pam/Models/Conditions/IpAllowlistCondition.cs b/bitwarden_license/src/Services/Pam/Models/Conditions/IpAllowlistCondition.cs index d03335baf8cc..5618ef93a689 100644 --- a/bitwarden_license/src/Services/Pam/Models/Conditions/IpAllowlistCondition.cs +++ b/bitwarden_license/src/Services/Pam/Models/Conditions/IpAllowlistCondition.cs @@ -39,5 +39,21 @@ public override AccessEvaluation Evaluate(AccessSignals signals) return AccessEvaluation.Deny(DenyReason.NotWithinIpRange); } - public override T Accept(IAccessConditionVisitor visitor) => visitor.VisitIpAllowlist(this); + public override AccessRuleValidationResult Validate() + { + if (Cidrs.Count == 0) + { + return AccessRuleValidationResult.Invalid("ip_allowlist requires at least one CIDR."); + } + + foreach (var cidr in Cidrs) + { + if (string.IsNullOrWhiteSpace(cidr) || !IPNetwork.TryParse(cidr, out _)) + { + return AccessRuleValidationResult.Invalid($"Invalid CIDR: '{cidr}'."); + } + } + + return AccessRuleValidationResult.Valid; + } } diff --git a/bitwarden_license/src/Services/Pam/Models/Conditions/TimeOfDayCondition.cs b/bitwarden_license/src/Services/Pam/Models/Conditions/TimeOfDayCondition.cs index 6e414fadaee8..1e6aa17c8312 100644 --- a/bitwarden_license/src/Services/Pam/Models/Conditions/TimeOfDayCondition.cs +++ b/bitwarden_license/src/Services/Pam/Models/Conditions/TimeOfDayCondition.cs @@ -41,7 +41,34 @@ public override AccessEvaluation Evaluate(AccessSignals signals) return Windows.Any(window => window.Contains(day, time)) ? AccessEvaluation.Allow : AccessEvaluation.Deny(DenyReason.NotWithinTimeWindow); } - public override T Accept(IAccessConditionVisitor visitor) => visitor.VisitTimeOfDay(this); + public override AccessRuleValidationResult Validate() + { + if (string.IsNullOrWhiteSpace(Tz)) + { + return AccessRuleValidationResult.Invalid("time_of_day requires a tz."); + } + + try + { + TimeZoneInfo.FindSystemTimeZoneById(Tz); + } + catch (TimeZoneNotFoundException) + { + return AccessRuleValidationResult.Invalid($"Unknown timezone: '{Tz}'."); + } + catch (InvalidTimeZoneException) + { + return AccessRuleValidationResult.Invalid($"Invalid timezone: '{Tz}'."); + } + + if (Windows.Count == 0) + { + return AccessRuleValidationResult.Invalid("time_of_day requires at least one window."); + } + + return Windows.Select(window => window.Validate()).FirstOrDefault(result => !result.IsValid) + ?? AccessRuleValidationResult.Valid; + } } /// @@ -71,8 +98,36 @@ public bool Contains(DayOfWeek day, TimeOnly time) 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) + return TryParseTime(From, out var from) + && TryParseTime(To, out var to) && time >= from && time <= to; } + + /// + /// Checks the window is well-formed: at least one day, and start/end times in 24-hour HH:mm. Uses the + /// same time parse as , so validation and evaluation cannot disagree on the format. + /// + public AccessRuleValidationResult Validate() + { + if (Days.Count == 0) + { + return AccessRuleValidationResult.Invalid("time_of_day window requires at least one day."); + } + + // Day tokens were validated during deserialization by AccessWeekdayJsonConverter; only the times remain. + if (!TryParseTime(From, out _)) + { + return AccessRuleValidationResult.Invalid($"Invalid 'from' time: '{From}'. Expected HH:mm."); + } + + if (!TryParseTime(To, out _)) + { + return AccessRuleValidationResult.Invalid($"Invalid 'to' time: '{To}'. Expected HH:mm."); + } + + return AccessRuleValidationResult.Valid; + } + + private static bool TryParseTime(string value, out TimeOnly time) => + TimeOnly.TryParseExact(value, "HH:mm", CultureInfo.InvariantCulture, DateTimeStyles.None, out time); } diff --git a/bitwarden_license/src/Services/Pam/Services/AccessRuleValidator.cs b/bitwarden_license/src/Services/Pam/Services/AccessRuleValidator.cs index 972467023fff..4ec2c86e4fbc 100644 --- a/bitwarden_license/src/Services/Pam/Services/AccessRuleValidator.cs +++ b/bitwarden_license/src/Services/Pam/Services/AccessRuleValidator.cs @@ -1,20 +1,12 @@ -using System.Net; -using System.Text.Json; -using System.Text.RegularExpressions; +using System.Text.Json; using Bit.Services.Pam.Models.Conditions; namespace Bit.Services.Pam.Services; -public sealed partial class AccessRuleValidator : IAccessRuleValidator +public sealed class AccessRuleValidator : IAccessRuleValidator { private const int MaxConditions = 10; - // Stateless, so one shared instance serves every call. - private static readonly ConditionValidator _conditionValidator = new(); - - [GeneratedRegex(@"^([01][0-9]|2[0-3]):[0-5][0-9]$")] - private static partial Regex TimeOfDayRegex(); - public AccessRuleValidationResult Validate(string? conditionsJson) { if (conditionsJson is null) @@ -50,88 +42,15 @@ public AccessRuleValidationResult Validate(string? conditionsJson) return AccessRuleValidationResult.Invalid($"Conditions cannot contain more than {MaxConditions} conditions."); } + // Each condition validates itself; the validator only enforces the document-level shape (it is an array, + // within the size limit) and guards the one thing a condition cannot check for itself: a null entry. return conditions.Select(ValidateCondition).FirstOrDefault(result => !result.IsValid) ?? AccessRuleValidationResult.Valid; } private static AccessRuleValidationResult ValidateCondition(AccessCondition? condition) => + // A JSON null in the array is not a condition and cannot validate itself, so it is rejected here. condition is null ? AccessRuleValidationResult.Invalid("Conditions cannot contain a null entry.") - : condition.Accept(_conditionValidator); - - /// - /// Checks a single condition is well-formed. Stateless, mirroring the engine's evaluator: neither public - /// service is itself a visitor; each delegates to a private one. - /// - private sealed class ConditionValidator : IAccessConditionVisitor - { - public AccessRuleValidationResult VisitHumanApproval(HumanApprovalCondition condition) => - AccessRuleValidationResult.Valid; - - public AccessRuleValidationResult VisitIpAllowlist(IpAllowlistCondition condition) - { - if (condition.Cidrs.Count == 0) - { - return AccessRuleValidationResult.Invalid("ip_allowlist requires at least one CIDR."); - } - - foreach (var cidr in condition.Cidrs) - { - if (string.IsNullOrWhiteSpace(cidr) || !IPNetwork.TryParse(cidr, out _)) - { - return AccessRuleValidationResult.Invalid($"Invalid CIDR: '{cidr}'."); - } - } - - return AccessRuleValidationResult.Valid; - } - - public AccessRuleValidationResult VisitTimeOfDay(TimeOfDayCondition condition) - { - if (string.IsNullOrWhiteSpace(condition.Tz)) - { - return AccessRuleValidationResult.Invalid("time_of_day requires a tz."); - } - - try - { - TimeZoneInfo.FindSystemTimeZoneById(condition.Tz); - } - catch (TimeZoneNotFoundException) - { - return AccessRuleValidationResult.Invalid($"Unknown timezone: '{condition.Tz}'."); - } - catch (InvalidTimeZoneException) - { - return AccessRuleValidationResult.Invalid($"Invalid timezone: '{condition.Tz}'."); - } - - if (condition.Windows.Count == 0) - { - return AccessRuleValidationResult.Invalid("time_of_day requires at least one window."); - } - - foreach (var window in condition.Windows) - { - if (window.Days.Count == 0) - { - return AccessRuleValidationResult.Invalid("time_of_day window requires at least one day."); - } - - // Day tokens are validated during deserialization by AccessWeekdayJsonConverter; an unknown token - // fails the JSON parse above and is reported as malformed. - if (!TimeOfDayRegex().IsMatch(window.From)) - { - return AccessRuleValidationResult.Invalid($"Invalid 'from' time: '{window.From}'. Expected HH:mm."); - } - - if (!TimeOfDayRegex().IsMatch(window.To)) - { - return AccessRuleValidationResult.Invalid($"Invalid 'to' time: '{window.To}'. Expected HH:mm."); - } - } - - return AccessRuleValidationResult.Valid; - } - } + : condition.Validate(); } diff --git a/bitwarden_license/src/Services/Pam/Services/IAccessRuleValidator.cs b/bitwarden_license/src/Services/Pam/Services/IAccessRuleValidator.cs index b706ccee6f8c..0e771c73bf35 100644 --- a/bitwarden_license/src/Services/Pam/Services/IAccessRuleValidator.cs +++ b/bitwarden_license/src/Services/Pam/Services/IAccessRuleValidator.cs @@ -1,4 +1,6 @@ -namespace Bit.Services.Pam.Services; +using Bit.Services.Pam.Models.Conditions; + +namespace Bit.Services.Pam.Services; public interface IAccessRuleValidator { @@ -8,9 +10,3 @@ public interface IAccessRuleValidator /// AccessRuleValidationResult Validate(string? conditionsJson); } - -public sealed record AccessRuleValidationResult(bool IsValid, string? Error) -{ - public static AccessRuleValidationResult Valid { get; } = new(true, null); - public static AccessRuleValidationResult Invalid(string error) => new(false, error); -} diff --git a/bitwarden_license/test/Services/Pam.Test/Commands/CreateAccessRuleCommandTests.cs b/bitwarden_license/test/Services/Pam.Test/Commands/CreateAccessRuleCommandTests.cs index 6d15d453a279..1d3343d93f25 100644 --- a/bitwarden_license/test/Services/Pam.Test/Commands/CreateAccessRuleCommandTests.cs +++ b/bitwarden_license/test/Services/Pam.Test/Commands/CreateAccessRuleCommandTests.cs @@ -3,6 +3,7 @@ using Bit.Core.Repositories; using Bit.Pam.Entities; using Bit.Pam.Repositories; +using Bit.Services.Pam.Models.Conditions; using Bit.Services.Pam.OrganizationFeatures.Commands; using Bit.Services.Pam.Services; using Bit.Test.Common.AutoFixture; diff --git a/bitwarden_license/test/Services/Pam.Test/Commands/UpdateAccessRuleCommandTests.cs b/bitwarden_license/test/Services/Pam.Test/Commands/UpdateAccessRuleCommandTests.cs index 656b5ef5c21e..06fc8779497b 100644 --- a/bitwarden_license/test/Services/Pam.Test/Commands/UpdateAccessRuleCommandTests.cs +++ b/bitwarden_license/test/Services/Pam.Test/Commands/UpdateAccessRuleCommandTests.cs @@ -4,6 +4,7 @@ using Bit.Pam.Entities; using Bit.Pam.Models; using Bit.Pam.Repositories; +using Bit.Services.Pam.Models.Conditions; using Bit.Services.Pam.OrganizationFeatures.Commands; using Bit.Services.Pam.Services; using Bit.Test.Common.AutoFixture; diff --git a/bitwarden_license/test/Services/Pam.Test/Engine/AccessRuleEngineTests.cs b/bitwarden_license/test/Services/Pam.Test/Engine/AccessRuleEngineTests.cs index 43a554b07161..b17c1ae09589 100644 --- a/bitwarden_license/test/Services/Pam.Test/Engine/AccessRuleEngineTests.cs +++ b/bitwarden_license/test/Services/Pam.Test/Engine/AccessRuleEngineTests.cs @@ -81,6 +81,6 @@ public override AccessEvaluation Evaluate(AccessSignals signals) return result; } - public override T Accept(IAccessConditionVisitor visitor) => throw new NotSupportedException(); + public override AccessRuleValidationResult Validate() => AccessRuleValidationResult.Valid; } } From f4a22541bed3c6949b9c38dcecf7d553a01fbeb4 Mon Sep 17 00:00:00 2001 From: Hinton Date: Fri, 17 Jul 2026 11:48:55 +0200 Subject: [PATCH 10/10] Relocate condition validation tests onto the conditions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now that each condition validates itself, move the per-kind validation assertions to the condition tests: IpAllowlistConditionTests (empty/invalid CIDR), TimeOfDayConditionTests (missing/unknown tz, no windows), and TimeWindowTests (no days, malformed HH:mm bounds). Trim AccessRuleValidatorTests to the validator's remaining job — JSON parse/shape/size, null entry, and delegation — keeping the unknown-weekday-token case as a deserialization concern. Mirrors the earlier relocation of the evaluation tests onto the conditions. --- .../Conditions/IpAllowlistConditionTests.cs | 29 +++++++ .../Conditions/TimeOfDayConditionTests.cs | 76 ++++++++++++++++++- .../Services/AccessRuleValidatorTests.cs | 62 ++------------- 3 files changed, 109 insertions(+), 58 deletions(-) diff --git a/bitwarden_license/test/Services/Pam.Test/Models/Conditions/IpAllowlistConditionTests.cs b/bitwarden_license/test/Services/Pam.Test/Models/Conditions/IpAllowlistConditionTests.cs index 46adfaa07c8c..840392386f94 100644 --- a/bitwarden_license/test/Services/Pam.Test/Models/Conditions/IpAllowlistConditionTests.cs +++ b/bitwarden_license/test/Services/Pam.Test/Models/Conditions/IpAllowlistConditionTests.cs @@ -77,4 +77,33 @@ public void Evaluate_LaterCidrMatches_Allows() Assert.Equal(AccessEvaluationOutcome.Allow, evaluation.Outcome); } + + [Fact] + public void Validate_NoCidrs_IsInvalid() + { + var result = new IpAllowlistCondition().Validate(); + + Assert.False(result.IsValid); + Assert.Contains("at least one CIDR", result.Error); + } + + [Theory] + [InlineData("")] + [InlineData("not-a-cidr")] + [InlineData("10.0.0.0/99")] + public void Validate_InvalidCidr_IsInvalid(string cidr) + { + var result = new IpAllowlistCondition { Cidrs = [cidr] }.Validate(); + + Assert.False(result.IsValid); + Assert.Contains("Invalid CIDR", result.Error); + } + + [Fact] + public void Validate_ValidCidrs_IsValid() + { + var result = new IpAllowlistCondition { Cidrs = ["10.0.0.0/8", "2001:db8::/32"] }.Validate(); + + Assert.True(result.IsValid); + } } diff --git a/bitwarden_license/test/Services/Pam.Test/Models/Conditions/TimeOfDayConditionTests.cs b/bitwarden_license/test/Services/Pam.Test/Models/Conditions/TimeOfDayConditionTests.cs index 7511613d483a..e4cfd4aec213 100644 --- a/bitwarden_license/test/Services/Pam.Test/Models/Conditions/TimeOfDayConditionTests.cs +++ b/bitwarden_license/test/Services/Pam.Test/Models/Conditions/TimeOfDayConditionTests.cs @@ -120,9 +120,46 @@ public void Evaluate_LaterWindowMatches_Allows() Assert.Equal(AccessEvaluationOutcome.Allow, condition.Evaluate(Signals()).Outcome); } + + [Fact] + public void Validate_MissingTz_IsInvalid() + { + var result = new TimeOfDayCondition { Windows = [ValidWindow()] }.Validate(); + + Assert.False(result.IsValid); + Assert.Contains("tz", result.Error); + } + + [Fact] + public void Validate_UnknownTz_IsInvalid() + { + var result = new TimeOfDayCondition { Tz = "Invalid/Zone", Windows = [ValidWindow()] }.Validate(); + + Assert.False(result.IsValid); + Assert.Contains("timezone", result.Error); + } + + [Fact] + public void Validate_NoWindows_IsInvalid() + { + var result = new TimeOfDayCondition { Tz = "UTC", Windows = [] }.Validate(); + + Assert.False(result.IsValid); + Assert.Contains("at least one window", result.Error); + } + + [Fact] + public void Validate_Valid_IsValid() + { + var result = new TimeOfDayCondition { Tz = "UTC", Windows = [ValidWindow()] }.Validate(); + + Assert.True(result.IsValid); + } + + private static TimeWindow ValidWindow() => new() { Days = [AccessWeekday.Mon], From = "09:00", To = "17:00" }; } -public class TimeWindowContainsTests +public class TimeWindowTests { private static TimeWindow BusinessHours() => new() { @@ -177,4 +214,41 @@ public void Contains_UnparseableBounds_FailsClosed() Assert.False(window.Contains(DayOfWeek.Thursday, new TimeOnly(12, 0))); } + + [Fact] + public void Validate_NoDays_IsInvalid() + { + var result = new TimeWindow { Days = [], From = "09:00", To = "17:00" }.Validate(); + + Assert.False(result.IsValid); + Assert.Contains("at least one day", result.Error); + } + + [Theory] + [InlineData("9am", "17:00")] + [InlineData("25:00", "17:00")] + public void Validate_InvalidFrom_IsInvalid(string from, string to) + { + var result = new TimeWindow { Days = [AccessWeekday.Mon], From = from, To = to }.Validate(); + + Assert.False(result.IsValid); + Assert.Contains("Expected HH:mm", result.Error); + } + + [Fact] + public void Validate_InvalidTo_IsInvalid() + { + var result = new TimeWindow { Days = [AccessWeekday.Mon], From = "09:00", To = "5pm" }.Validate(); + + Assert.False(result.IsValid); + Assert.Contains("Expected HH:mm", result.Error); + } + + [Fact] + public void Validate_ValidWindow_IsValid() + { + var result = new TimeWindow { Days = [AccessWeekday.Mon], From = "09:00", To = "17:00" }.Validate(); + + Assert.True(result.IsValid); + } } diff --git a/bitwarden_license/test/Services/Pam.Test/Services/AccessRuleValidatorTests.cs b/bitwarden_license/test/Services/Pam.Test/Services/AccessRuleValidatorTests.cs index aaededee765c..686d6245e88f 100644 --- a/bitwarden_license/test/Services/Pam.Test/Services/AccessRuleValidatorTests.cs +++ b/bitwarden_license/test/Services/Pam.Test/Services/AccessRuleValidatorTests.cs @@ -62,66 +62,14 @@ public void Validate_LegacyAllOfKind_IsInvalid() } [Fact] - public void Validate_HumanApproval_IsValid() + public void Validate_TimeOfDay_UnknownWeekdayToken_IsInvalid() { - var result = _sut.Validate("""[{"kind":"human_approval"}]"""); - - Assert.True(result.IsValid); - } - - [Theory] - [InlineData("""[{"kind":"ip_allowlist","cidrs":["10.0.0.0/8"]}]""")] - [InlineData("""[{"kind":"ip_allowlist","cidrs":["10.0.0.0/8","192.168.0.0/16","2001:db8::/32"]}]""")] - public void Validate_IpAllowlist_ValidCidrs_IsValid(string conditionsJson) - { - var result = _sut.Validate(conditionsJson); - - Assert.True(result.IsValid); - } - - [Theory] - [InlineData("""[{"kind":"ip_allowlist","cidrs":[]}]""", "at least one CIDR")] - [InlineData("""[{"kind":"ip_allowlist","cidrs":["not-a-cidr"]}]""", "Invalid CIDR")] - [InlineData("""[{"kind":"ip_allowlist","cidrs":["10.0.0.0/99"]}]""", "Invalid CIDR")] - public void Validate_IpAllowlist_InvalidCidrs_IsInvalid(string conditionsJson, string expectedMessageFragment) - { - var result = _sut.Validate(conditionsJson); - - Assert.False(result.IsValid); - Assert.Contains(expectedMessageFragment, result.Error); - } - - [Fact] - public void Validate_TimeOfDay_Valid_IsValid() - { - var result = _sut.Validate(""" - [ - { - "kind": "time_of_day", - "tz": "UTC", - "windows": [ - { "days": ["mon","tue","wed","thu","fri"], "from": "09:00", "to": "18:00" } - ] - } - ] - """); - - Assert.True(result.IsValid); - } - - [Theory] - [InlineData("""[{"kind":"time_of_day","tz":"Invalid/Zone","windows":[{"days":["mon"],"from":"09:00","to":"17:00"}]}]""", "timezone")] - [InlineData("""[{"kind":"time_of_day","tz":"UTC","windows":[]}]""", "at least one window")] - [InlineData("""[{"kind":"time_of_day","tz":"UTC","windows":[{"days":[],"from":"09:00","to":"17:00"}]}]""", "at least one day")] - [InlineData("""[{"kind":"time_of_day","tz":"UTC","windows":[{"days":["funday"],"from":"09:00","to":"17:00"}]}]""", "day")] - [InlineData("""[{"kind":"time_of_day","tz":"UTC","windows":[{"days":["mon"],"from":"9am","to":"5pm"}]}]""", "Expected HH:mm")] - [InlineData("""[{"kind":"time_of_day","tz":"UTC","windows":[{"days":["mon"],"from":"25:00","to":"26:00"}]}]""", "Expected HH:mm")] - public void Validate_TimeOfDay_Invalid_IsInvalid(string conditionsJson, string expectedMessageFragment) - { - var result = _sut.Validate(conditionsJson); + // An unknown weekday token fails deserialization (via AccessWeekdayJsonConverter) before any condition-level + // check runs, so the whole document is rejected as malformed. Per-condition rules are covered by the + // condition tests (IpAllowlistConditionTests, TimeOfDayConditionTests). + var result = _sut.Validate("""[{"kind":"time_of_day","tz":"UTC","windows":[{"days":["funday"],"from":"09:00","to":"17:00"}]}]"""); Assert.False(result.IsValid); - Assert.Contains(expectedMessageFragment, result.Error); } [Fact]