Darling live tests stop depending on PUBLIC's CONNECT on the shared test database (#5618)

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,16 +330,19 @@ 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
         {
             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[]
                      {

Darling/Darling.Tests/PgClusterRolesCollection.cs

@@ -32,8 +32,13 @@ namespace Darling.Tests;
 /// <see cref="LivePostgresStoreFixture"/> would run its residue check against a store the parallel collections
 /// are still writing. <see cref="PgClusterObjectCensusTests"/> fails when a live class changes a cluster-wide
 /// object under a fixed name and is neither here nor on its allow list.</para>
+///
+/// <para><see cref="PublicConnectGuardAttribute"/> (#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".</para>
 /// </summary>
 [CollectionDefinition("pg-cluster-roles", DisableParallelization = true)]
+[PublicConnectGuard]
 public sealed class PgClusterRolesCollection
 {
 }

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 <c>Row</c> while
 /// host, port, user and application_name survive.
 ///
-/// <para>Cluster-wide state (<c>ALTER SYSTEM</c> is not per-database), so this is serialized with the rest of
-/// the live-postgres collection and restores the setting in a <c>finally</c> that runs even on failure.</para>
+/// <para>Cluster-wide state (<c>ALTER SYSTEM</c> is not per-database), so this runs in the <c>pg-cluster-roles</c>
+/// collection, alone, and restores the setting in a <c>finally</c> 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.</para>
 /// </summary>
 [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);
             });
         }

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;
+
+/// <summary>
+/// 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.
+/// </summary>
+[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<PublicConnectGuardAttribute>());
+    }
+
+    [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);
+    }
+}

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;
+
+/// <summary>
+/// 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.
+/// </summary>
+internal static class PublicConnectDelta
+{
+    /// <summary>The databases whose PUBLIC CONNECT was granted in <paramref name="before"/> and is revoked in
+    /// <paramref name="after"/>, in name order. A database missing from either side, or revoked in both, is not a change.</summary>
+    internal static IReadOnlyList<string> DatabasesThatLostPublicConnect(
+        IReadOnlyDictionary<string, bool> before, IReadOnlyDictionary<string, bool> after) =>
+        before
+            .Where(b => b.Value && after.TryGetValue(b.Key, out var now) && !now)
+            .Select(b => b.Key)
+            .OrderBy(name => name, StringComparer.Ordinal)
+            .ToList();
+}

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;
+
+/// <summary>
+/// Fails the test that leaves PUBLIC without <c>CONNECT</c> on the shared test database (#5618).
+///
+/// <para><b>Why it exists.</b> The product's managed provisioning batch writes
+/// <c>REVOKE ALL ON DATABASE darling FROM PUBLIC</c>, and database privileges live in <c>pg_database</c>, 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 <c>darling</c> 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.</para>
+///
+/// <para><b>What it compares.</b> 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 <c>DARLING_TEST_PG</c> names and the managed store's fixed name <c>darling</c>, which are
+/// one database in CI and two on a laptop that keeps its suite in another database.</para>
+///
+/// <para><b>Where it runs.</b> On <see cref="PgClusterRolesCollection"/>, 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. <see cref="PgClusterObjectCensusTests"/> keeps every other class that changes a database-level
+/// privilege out of the parallel collections. Without <c>DARLING_TEST_PG</c> it does nothing, as the tests skip.</para>
+///
+/// <para><b>#1776 own-store</b>, not <c>[Collection("live-postgres")]</c>: it reads one cluster-wide catalog
+/// (<c>pg_database</c>) 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.</para>
+/// </summary>
+[AttributeUsage(AttributeTargets.Class, AllowMultiple = false, Inherited = false)]
+public sealed class PublicConnectGuardAttribute : BeforeAfterTestAttribute
+{
+    /// <summary>The state read in <see cref="Before"/>. 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.</summary>
+    private static Dictionary<string, bool>? 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.");
+        }
+    }
+
+    /// <summary>
+    /// Whether PUBLIC holds CONNECT on each of <paramref name="databases"/> that exists on the cluster
+    /// <paramref name="connectionString"/> reaches (<c>DARLING_TEST_PG</c>'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.
+    /// </summary>
+    internal static Dictionary<string, bool> 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<string, bool>(StringComparer.Ordinal);
+        using var reader = command.ExecuteReader();
+        while (reader.Read())
+        {
+            state[reader.GetString(0)] = reader.GetBoolean(1);
+        }
+
+        return state;
+    }
+}

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;
+
+/// <summary>
+/// The guard that fails the test which leaves PUBLIC without CONNECT on the shared test database (#5618), and its
+/// comparison. No store is touched.
+/// </summary>
+[Trait("Stage", "Guard")]
+public sealed class PublicConnectGuardTests
+{
+    [Fact]
+    public void OnlyAGrantedToRevokedChange_IsReported()
+    {
+        var before = new Dictionary<string, bool>
+        {
+            ["darling"] = true,
+            ["suite"] = true,
+            ["already_revoked"] = false,
+            ["regranted"] = false,
+            ["vanished"] = true,
+        };
+        var after = new Dictionary<string, bool>
+        {
+            ["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));
+    }
+}