Skip to content

Commit 3f4b41d

Browse files
committed
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).
1 parent c871449 commit 3f4b41d

2 files changed

Lines changed: 42 additions & 3 deletions

File tree

bitwarden_license/src/Services/Pam/Services/GoverningRuleResolver.cs

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,13 +43,15 @@ public GoverningRuleResolver(
4343
.Where(c => collectionIds.Contains(c.Id) && c.AccessRuleId.HasValue);
4444

4545
// Load every rule on the collections through which the caller reaches the cipher, keeping each paired with
46-
// the collection it gates. A rule that no longer loads (deleted after the collection was read — deletes clear
47-
// the link, so this is only a race) is skipped, so a deleted rule stops governing.
46+
// the collection it gates. A rule is dropped — so it stops governing — when it is disabled (Enabled is false;
47+
// the admin has switched it off, and a disabled rule does not gate access) or no longer loads (deleted after
48+
// the collection was read; deletes clear the link, so a missing rule is only a race). Dropping a disabled rule
49+
// also stops it shadowing a newer active rule under the oldest-wins selection below.
4850
var candidates = new List<(Collection Collection, AccessRule Rule)>();
4951
foreach (var collection in governedCollections)
5052
{
5153
var accessRule = await _accessRuleRepository.GetByIdAsync(collection.AccessRuleId!.Value);
52-
if (accessRule is not null)
54+
if (accessRule is { Enabled: true })
5355
{
5456
candidates.Add((collection, accessRule));
5557
}

bitwarden_license/test/Services/Pam.Test/Services/GoverningRuleResolverTests.cs

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -263,6 +263,7 @@ public async Task ResolveAsync_OldestGovernedRuleDeleted_NextRuleGoverns(
263263
olderRule.CreationDate = new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc);
264264
newerRule.CreationDate = new DateTime(2026, 6, 1, 0, 0, 0, DateTimeKind.Utc);
265265
newerRule.Conditions = """[{"kind":"human_approval"}]""";
266+
newerRule.Enabled = true;
266267
olderCollection.AccessRuleId = olderRule.Id;
267268
newerCollection.AccessRuleId = newerRule.Id;
268269
SetupReachableCollections(sutProvider, userId, cipherId, olderCollection, newerCollection);
@@ -278,6 +279,40 @@ public async Task ResolveAsync_OldestGovernedRuleDeleted_NextRuleGoverns(
278279
Assert.True(result.RequiresHumanApproval);
279280
}
280281

282+
[Theory, BitAutoData]
283+
public async Task ResolveAsync_DisabledRule_NotGoverned(
284+
SutProvider<GoverningRuleResolver> sutProvider, Guid userId, Guid cipherId, Collection collection, AccessRule rule)
285+
{
286+
// A disabled rule is inactive and does not gate access, so a cipher reached only through it is ungoverned.
287+
SetupGovernedCollection(sutProvider, userId, cipherId, collection, rule);
288+
rule.Enabled = false;
289+
290+
Assert.Null(await sutProvider.Sut.ResolveAsync(userId, cipherId, _signals));
291+
}
292+
293+
[Theory, BitAutoData]
294+
public async Task ResolveAsync_OldestRuleDisabled_NewerEnabledRuleGoverns(
295+
SutProvider<GoverningRuleResolver> sutProvider, Guid userId, Guid cipherId,
296+
Collection olderCollection, AccessRule olderRule, Collection newerCollection, AccessRule newerRule)
297+
{
298+
// The oldest rule is disabled and auto-granting; the newer rule is enabled and needs human approval. A disabled
299+
// rule must not shadow a newer active one, so the newer rule governs — access is not silently auto-granted.
300+
olderRule.CreationDate = new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc);
301+
olderRule.Conditions = "[]";
302+
newerRule.CreationDate = new DateTime(2026, 6, 1, 0, 0, 0, DateTimeKind.Utc);
303+
newerRule.Conditions = """[{"kind":"human_approval"}]""";
304+
SetupGovernedCollections(sutProvider, userId, cipherId,
305+
(olderCollection, olderRule), (newerCollection, newerRule));
306+
olderRule.Enabled = false;
307+
308+
var result = await sutProvider.Sut.ResolveAsync(userId, cipherId, _signals);
309+
310+
Assert.NotNull(result);
311+
Assert.Equal(newerCollection.Id, result!.CollectionId);
312+
Assert.Equal(newerRule.Id, result.RuleId);
313+
Assert.True(result.RequiresHumanApproval);
314+
}
315+
281316
[Theory, BitAutoData]
282317
public async Task ResolveAsync_TimeOfDayCondition_ParsedAndEvaluatedThroughResolver(
283318
SutProvider<GoverningRuleResolver> sutProvider, Guid userId, Guid cipherId, Collection collection, AccessRule rule)
@@ -318,6 +353,8 @@ private static void SetupGovernedCollections(
318353
foreach (var (collection, rule) in pairs)
319354
{
320355
collection.AccessRuleId = rule.Id;
356+
// A governing rule must be enabled; pin it so the outcome does not depend on AutoFixture's bool sequence.
357+
rule.Enabled = true;
321358
}
322359

323360
SetupReachableCollections(sutProvider, userId, cipherId, pairs.Select(p => p.collection).ToArray());

0 commit comments

Comments
 (0)