diff --git a/Darling/Darling.Tests/PasswordKeyTablesLiveTests.cs b/Darling/Darling.Tests/PasswordKeyTablesLiveTests.cs index 135f268826..231c55d590 100644 --- a/Darling/Darling.Tests/PasswordKeyTablesLiveTests.cs +++ b/Darling/Darling.Tests/PasswordKeyTablesLiveTests.cs @@ -289,10 +289,30 @@ public async Task ASecondCurrentKeyFails_AndTheReadsReturnTheCurrentKeyAndTheNew private static ProvisioningTarget TargetFor(bool managed, NpgsqlConnection owner, ScratchPostgres scratch) => managed ? ProvisioningTarget.Managed : ProvisioningTarget.ComposeStore(OwnerRoleOf(owner), scratch.DatabaseName); - private static string ProvisioningBatch(bool managed, NpgsqlConnection owner, ScratchPostgres scratch) => - DarlingManagedRoles.BuildProvisioningSql( + private static string ProvisioningBatch(bool managed, NpgsqlConnection owner, ScratchPostgres scratch) + { + var batch = DarlingManagedRoles.BuildProvisioningSql( ProvisioningTestSecrets.Admin, ProvisioningTestSecrets.Viewer, ProvisioningTestSecrets.Mcp, 15, PasswordReassert.All, TargetFor(managed, owner, scratch)); + if (!managed) + { + return batch; + } + + /* #5618: the managed target names the cluster's shared "darling" database in its three database-level + statements (REVOKE ALL ... FROM PUBLIC, and the CONNECT grants to admin, viewer and mcp). Database + privileges live in pg_database, which every database in the cluster shares, so running the batch inside a + scratch database still took CONNECT away from PUBLIC on "darling" and never gave it back: the next test + that opened a connection through PUBLIC's CONNECT failed with 42501. The managed shape stays managed (its + owner, its role marker, the PUBLIC revoke), pointed at this test's own database, the way ViewerGrantReplay + points its replays. */ + var shared = "ON DATABASE " + ProvisioningTarget.Managed.DatabaseIdentifier + " "; + var scratchDatabase = "ON DATABASE \"" + scratch.DatabaseName.Replace("\"", "\"\"", StringComparison.Ordinal) + "\" "; + Assert.Contains("REVOKE ALL " + shared + "FROM PUBLIC;", batch, StringComparison.Ordinal); + batch = batch.Replace(shared, scratchDatabase, StringComparison.Ordinal); + Assert.DoesNotContain(shared, batch, StringComparison.Ordinal); + return batch; + } [Theory] [InlineData(true)] @@ -310,9 +330,8 @@ public async Task Provisioning_LeavesNoReadOfThePinTables_AndNoTriggerOrReferenc await ScalarAsync(owner, "SELECT EXISTS (SELECT 1 FROM pg_roles WHERE rolname IN ('admin', 'viewer', 'mcp'))", ct) is true, "A cluster-wide admin/viewer/mcp role already exists on this cluster; the provisioning batch would adopt it."); Assert.SkipWhen( - managed && (OwnerRoleOf(owner) != DarlingManagedPostgres.UserName - || await ScalarAsync(owner, "SELECT NOT EXISTS (SELECT 1 FROM pg_database WHERE datname = 'darling')", ct) is true), - "The managed shape names the owner and database 'darling', which this connection does not have."); + managed && OwnerRoleOf(owner) != DarlingManagedPostgres.UserName, + "The managed shape names the owner 'darling', which this connection is not."); var bodySucceeded = false; try @@ -320,6 +339,10 @@ await ScalarAsync(owner, "SELECT EXISTS (SELECT 1 FROM pg_roles WHERE rolname IN await ExecAsync(owner, ProvisioningBatch(managed, owner, scratch), ct); await SeedRowsAsync(owner, ct); + /* #5618: the managed batch's PUBLIC revoke, proven on the scratch database it now names (the compose shape + leaves PUBLIC's CONNECT alone). The shared "darling" database is not this test's to change. */ + Assert.Equal((object)!managed, await ScalarAsync(owner, "SELECT has_database_privilege('public', current_database(), 'CONNECT')", ct)); + /* The pin tables are read by the store owner only; the key and service-state tables are readable. */ foreach (var (role, password) in new[] { diff --git a/Darling/Darling.Tests/PgClusterRolesCollection.cs b/Darling/Darling.Tests/PgClusterRolesCollection.cs index 2547388e47..5f12142f28 100644 --- a/Darling/Darling.Tests/PgClusterRolesCollection.cs +++ b/Darling/Darling.Tests/PgClusterRolesCollection.cs @@ -32,8 +32,13 @@ namespace Darling.Tests; /// would run its residue check against a store the parallel collections /// are still writing. fails when a live class changes a cluster-wide /// object under a fixed name and is neither here nor on its allow list. +/// +/// (#5618) runs around every test in it and fails the one that leaves +/// PUBLIC without CONNECT on the shared test database. The product's managed batch revokes it, the privilege is +/// cluster-wide, and the tests after the one that did it fail with "permission denied for database". /// [CollectionDefinition("pg-cluster-roles", DisableParallelization = true)] +[PublicConnectGuard] public sealed class PgClusterRolesCollection { } diff --git a/Darling/Darling.Tests/PgSettingRedactionLivePostgresTests.cs b/Darling/Darling.Tests/PgSettingRedactionLivePostgresTests.cs index bfea3b5716..02eb653651 100644 --- a/Darling/Darling.Tests/PgSettingRedactionLivePostgresTests.cs +++ b/Darling/Darling.Tests/PgSettingRedactionLivePostgresTests.cs @@ -25,8 +25,10 @@ namespace Darling.Tests; /// collector's redaction call sites fails this test), and asserts the secret never reaches a Row while /// host, port, user and application_name survive. /// -/// Cluster-wide state (ALTER SYSTEM is not per-database), so this is serialized with the rest of -/// the live-postgres collection and restores the setting in a finally that runs even on failure. +/// Cluster-wide state (ALTER SYSTEM is not per-database), so this runs in the pg-cluster-roles +/// collection, alone, and restores the setting in a finally that runs even on failure. The probe role also +/// holds its own CONNECT on the database it connects to (#5618), so it passes whether or not an earlier test has +/// taken CONNECT away from PUBLIC. /// [Collection("pg-cluster-roles")] public sealed class PgSettingRedactionLivePostgresTests @@ -56,6 +58,7 @@ public async Task PrimaryConninfosPassword_IsRedactedByTheRealCollector_UnderAPg await using var owner = new NpgsqlConnection(cs); await owner.OpenAsync(ct); + var quotedDatabase = "\"" + owner.Database.Replace("\"", "\"\"", StringComparison.Ordinal) + "\""; var bodySucceeded = false; try @@ -67,6 +70,13 @@ public async Task PrimaryConninfosPassword_IsRedactedByTheRealCollector_UnderAPg await ExecAsync(owner, $"CREATE ROLE {RoleName} LOGIN PASSWORD '{RolePassword}'", ct); await ExecAsync(owner, $"GRANT pg_monitor TO {RoleName}", ct); + /* #5618: the probe role holds CONNECT on this database in its own right. A fresh role gets CONNECT only + through PUBLIC, and the product's provisioning batch takes that away from the cluster's "darling" database + (REVOKE ALL ... FROM PUBLIC), so a test that relied on PUBLIC failed with 42501 whenever an earlier test + had run that batch. Revoked in the cleanup below, before the role is dropped: a role that still holds a + privilege cannot be dropped. */ + await ExecAsync(owner, $"GRANT CONNECT ON DATABASE {quotedDatabase} TO {RoleName}", ct); + var probeConnectionString = new NpgsqlConnectionStringBuilder(cs) { Username = RoleName, @@ -150,6 +160,12 @@ await LiveStoreCleanup.RunOwnedAsync(bodySucceeded, async () => { await ExecAsync(owner, "ALTER SYSTEM SET primary_conninfo = ''", ct); await ExecAsync(owner, "SELECT pg_reload_conf()", ct); + + /* #5618: the grant goes before the role does. The role may not exist when the body failed before + creating it, hence the existence check. */ + await ExecAsync(owner, + $"DO $$ BEGIN IF EXISTS (SELECT 1 FROM pg_roles WHERE rolname = '{RoleName}') THEN " + + $"REVOKE CONNECT ON DATABASE {quotedDatabase} FROM {RoleName}; END IF; END $$", ct); await ExecAsync(owner, $"DROP ROLE IF EXISTS {RoleName}", ct); }); } diff --git a/Darling/Darling.Tests/PublicConnectCatalogReadLiveTests.cs b/Darling/Darling.Tests/PublicConnectCatalogReadLiveTests.cs new file mode 100644 index 0000000000..cd5a7bc4dd --- /dev/null +++ b/Darling/Darling.Tests/PublicConnectCatalogReadLiveTests.cs @@ -0,0 +1,65 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Linq; +using System.Reflection; +using System.Threading.Tasks; +using Npgsql; +using Xunit; + +namespace Darling.Tests; + +/// +/// The PUBLIC-CONNECT guard against a real cluster (#5618): its catalog read sees a database's PUBLIC CONNECT granted, +/// then revoked, and the comparison names the database (the revoke is on this test's own scratch database, never on +/// the shared one); and the collection that needs the guard still carries it. +/// +[Collection("live-postgres")] +public sealed class PublicConnectCatalogReadLiveTests +{ + private static string? ConnectionString => Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + + [Fact] + public void ThePgClusterRolesCollection_CarriesTheGuard() + { + /* The guard only guards what it is attached to. Taking it off the collection would leave the next test that + revokes the privilege to be found the way #5618 was, by the victim's name in a nightly run. */ + Assert.Single(typeof(PgClusterRolesCollection).GetCustomAttributes()); + } + + [Fact] + public async Task TheCatalogRead_SeesAGrantAndARevokeOfPublicConnect() + { + var cs = ConnectionString; + Assert.SkipWhen(string.IsNullOrEmpty(cs), "Set DARLING_TEST_PG to a Postgres connection string to run the #5618 guard read."); + var ct = TestContext.Current.CancellationToken; + + /* #1776 own-store: the revoke below is on this test's own scratch database, never on the shared one. */ + await using var scratch = await ScratchPostgres.CreateAsync(cs!, ct); + + var before = PublicConnectGuardAttribute.PublicConnectOn(cs!, scratch.DatabaseName, "database_that_does_not_exist_5618"); + Assert.Equal([scratch.DatabaseName], before.Keys.ToArray()); + Assert.True(before[scratch.DatabaseName], "a new database grants CONNECT to PUBLIC"); + + await using (var admin = new NpgsqlConnection(cs)) + { + await admin.OpenAsync(ct); + await using var revoke = new NpgsqlCommand($"REVOKE ALL ON DATABASE \"{scratch.DatabaseName}\" FROM PUBLIC", admin); + await revoke.ExecuteNonQueryAsync(ct); + } + + var after = PublicConnectGuardAttribute.PublicConnectOn(cs!, scratch.DatabaseName); + Assert.False(after[scratch.DatabaseName]); + Assert.Equal([scratch.DatabaseName], PublicConnectDelta.DatabasesThatLostPublicConnect(before, after)); + + /* The default read names the connection's own database, as CI has it. */ + var shared = PublicConnectGuardAttribute.PublicConnectOn(cs!); + Assert.Contains(new NpgsqlConnectionStringBuilder(cs).Database!, shared.Keys); + } +} diff --git a/Darling/Darling.Tests/PublicConnectDelta.cs b/Darling/Darling.Tests/PublicConnectDelta.cs new file mode 100644 index 0000000000..bb53580884 --- /dev/null +++ b/Darling/Darling.Tests/PublicConnectDelta.cs @@ -0,0 +1,30 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.Linq; + +namespace Darling.Tests; + +/// +/// The comparison the PUBLIC-CONNECT guard makes between two reads of PUBLIC's CONNECT (#5618). Its own +/// type, and free of any store access, so a guard-stage test can pin it without a PostgreSQL server. +/// +internal static class PublicConnectDelta +{ + /// The databases whose PUBLIC CONNECT was granted in and is revoked in + /// , in name order. A database missing from either side, or revoked in both, is not a change. + internal static IReadOnlyList DatabasesThatLostPublicConnect( + IReadOnlyDictionary before, IReadOnlyDictionary after) => + before + .Where(b => b.Value && after.TryGetValue(b.Key, out var now) && !now) + .Select(b => b.Key) + .OrderBy(name => name, StringComparer.Ordinal) + .ToList(); +} diff --git a/Darling/Darling.Tests/PublicConnectGuardAttribute.cs b/Darling/Darling.Tests/PublicConnectGuardAttribute.cs new file mode 100644 index 0000000000..72e0ff60e3 --- /dev/null +++ b/Darling/Darling.Tests/PublicConnectGuardAttribute.cs @@ -0,0 +1,108 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Reflection; +using Npgsql; +using PerformanceMonitor.Darling.Service; +using Xunit; +using Xunit.v3; + +namespace Darling.Tests; + +/// +/// Fails the test that leaves PUBLIC without CONNECT on the shared test database (#5618). +/// +/// Why it exists. The product's managed provisioning batch writes +/// REVOKE ALL ON DATABASE darling FROM PUBLIC, and database privileges live in pg_database, which every +/// database in the cluster shares. A live test that ran that batch inside its own scratch database still took CONNECT +/// away from PUBLIC on the cluster's darling database, and nothing gave it back. The next test that opened a +/// connection as a freshly created role (a role has CONNECT only through PUBLIC) failed with 42501, and it was +/// that victim, not the test that did it, whose name appeared in the nightly run. This puts the failure on the test +/// that does it. +/// +/// What it compares. PUBLIC's CONNECT on each database before the test against the same read after it, +/// and fails only a change from granted to revoked. It does not demand that PUBLIC hold CONNECT: a developer's own +/// managed store has it revoked on purpose, and a guard that failed there would fail every test in the collection. +/// The databases are the one DARLING_TEST_PG names and the managed store's fixed name darling, which are +/// one database in CI and two on a laptop that keeps its suite in another database. +/// +/// Where it runs. On , the collection for the tests that change +/// cluster-wide state under fixed names. That collection runs alone, so a change between one test's start and end is +/// that test's. keeps every other class that changes a database-level +/// privilege out of the parallel collections. Without DARLING_TEST_PG it does nothing, as the tests skip. +/// +/// #1776 own-store, not [Collection("live-postgres")]: it reads one cluster-wide catalog +/// (pg_database) and no row or relation of the shared store, so it has nothing to serialize. It runs around the +/// tests of the one collection that does. +/// +[AttributeUsage(AttributeTargets.Class, AllowMultiple = false, Inherited = false)] +public sealed class PublicConnectGuardAttribute : BeforeAfterTestAttribute +{ + /// The state read in . Static so it does not depend on whether xUnit reuses one attribute + /// instance; the collection it guards runs one test at a time, so one slot is enough. + private static Dictionary? s_before; + + public override void Before(MethodInfo methodUnderTest, IXunitTest test) + { + var cs = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + s_before = string.IsNullOrEmpty(cs) ? null : PublicConnectOn(cs); + } + + public override void After(MethodInfo methodUnderTest, IXunitTest test) + { + var cs = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + var before = s_before; + s_before = null; + if (string.IsNullOrEmpty(cs) || before is null) + { + return; + } + + var lost = PublicConnectDelta.DatabasesThatLostPublicConnect(before, PublicConnectOn(cs)); + if (lost.Count > 0) + { + throw new InvalidOperationException( + $"{test.TestDisplayName} left PUBLIC without CONNECT on database {string.Join(", ", lost.Select(d => "\"" + d + "\""))} " + + "(#5618). Database privileges live in the cluster's shared catalog, so a test that runs the managed provisioning batch " + + "(ProvisioningTarget.Managed, whose PUBLIC revoke names the shared database) takes CONNECT away " + + "from every later test that relies on it. Point the batch at the test's own scratch database " + + "(ProvisioningTarget.ComposeStore(owner, scratch.DatabaseName), or the managed text with its database-level statements retargeted), " + + "or grant CONNECT back in a finally."); + } + } + + /// + /// Whether PUBLIC holds CONNECT on each of that exists on the cluster + /// reaches (DARLING_TEST_PG's own database and the managed store's + /// fixed name when none is passed). Unpooled: a pooled session must not outlive a read of a shared catalog. + /// + internal static Dictionary PublicConnectOn(string connectionString, params string[] databases) + { + var builder = new NpgsqlConnectionStringBuilder(connectionString) { Pooling = false }; + var names = databases.Length > 0 + ? databases + : new[] { builder.Database ?? string.Empty, ProvisioningTarget.Managed.DatabaseIdentifier }; + + using var connection = new NpgsqlConnection(builder.ConnectionString); + connection.Open(); + using var command = new NpgsqlCommand( + "SELECT datname, has_database_privilege('public', datname, 'CONNECT') FROM pg_database WHERE datname = ANY (@names)", connection); + command.Parameters.AddWithValue("names", names.Distinct(StringComparer.Ordinal).ToArray()); + var state = new Dictionary(StringComparer.Ordinal); + using var reader = command.ExecuteReader(); + while (reader.Read()) + { + state[reader.GetString(0)] = reader.GetBoolean(1); + } + + return state; + } +} diff --git a/Darling/Darling.Tests/PublicConnectGuardTests.cs b/Darling/Darling.Tests/PublicConnectGuardTests.cs new file mode 100644 index 0000000000..d395ac1f13 --- /dev/null +++ b/Darling/Darling.Tests/PublicConnectGuardTests.cs @@ -0,0 +1,46 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System.Collections.Generic; +using Xunit; + +namespace Darling.Tests; + +/// +/// The guard that fails the test which leaves PUBLIC without CONNECT on the shared test database (#5618), and its +/// comparison. No store is touched. +/// +[Trait("Stage", "Guard")] +public sealed class PublicConnectGuardTests +{ + [Fact] + public void OnlyAGrantedToRevokedChange_IsReported() + { + var before = new Dictionary + { + ["darling"] = true, + ["suite"] = true, + ["already_revoked"] = false, + ["regranted"] = false, + ["vanished"] = true, + }; + var after = new Dictionary + { + ["darling"] = false, + ["suite"] = true, + ["already_revoked"] = false, + ["regranted"] = true, + ["newcomer"] = false, + }; + + /* "darling" lost it; "suite" kept it; "already_revoked" was not granted to begin with (a developer's own managed + store), so it is not this test's doing; "regranted" went the other way; a database that appears or disappears + between the two reads is not a change of the privilege. */ + Assert.Equal(["darling"], PublicConnectDelta.DatabasesThatLostPublicConnect(before, after)); + } +}