Report as revoked only what the read-back shows gone, and take back what the reader passed on (review of hq issue 337)

A reader given a privilege WITH GRANT OPTION that passed it on made the
plain revoke fail with dependent privileges exist; the pass still listed the
table as revoked and logged it every minute. The pass now retries such a
change with CASCADE, which reaches only grants that came from the reader,
reads the grants back, reports as revoked only what is gone, and says what
is still held and why when anything is.

modules/postgres/README.md

@@ -68,8 +68,11 @@ for, so one removed by hand is installed again.
   what stands in the way. Declare columns by name where a table mixes the two.
   Every minute, on every database, the reader's role memberships are revoked and its grants brought to
   exactly that — each grant revoked as its own grantor, since a superuser's revoke acts as the table's
-  owner and leaves a grant another role made — and then read back: anything the reader still holds that
-  is not declared is reported as a failure, never as revoked. An existing install is narrowed without
+  owner and leaves a grant another role made — and then read back: only what the read-back shows gone
+  is reported as revoked, and anything the reader still holds that is not declared is reported as a
+  failure, with the reason. A reader that was given a privilege with grant option and passed it on makes
+  a plain revoke fail ("dependent privileges exist"); the pass then revokes with CASCADE, which reaches
+  only the grants that came from the reader. An existing install is narrowed without
   anybody acting, and a narrowed declaration takes effect at the next pass. The role itself is altered
   only when an attribute, a setting or its password has drifted, so the password is not sent every
   minute. Before a caller's statement runs, the reader's own session is asked which secret-named

modules/postgres/cmd/postgres-provider/readable.go

@@ -35,6 +35,7 @@ package main
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"os"
 	"regexp"
@@ -42,6 +43,8 @@ import (
 	"strings"
 	"sync"
 	"time"
+
+	"github.com/jackc/pgx/v5/pgconn"
 )
 
 // ReadableTable is one entry of a contribution's `readable`.
@@ -543,6 +546,40 @@ func (c *Client) LastReaderReport() ReaderReport {
 	return c.reader.last
 }
 
+// dependentPrivileges says whether the server refused a revoke because grants depend on it
+// (dependent_privileges_exist): the reader passed on a privilege it holds with grant option.
+func dependentPrivileges(err error) bool {
+	var pg *pgconn.PgError
+	return errors.As(err, &pg) && pg.Code == "2BP01"
+}
+
+// cascaded is the change with every REVOKE made CASCADE.
+func cascaded(stmts []string) []string {
+	out := make([]string, len(stmts))
+	for i, s := range stmts {
+		if strings.HasPrefix(s, "REVOKE ") {
+			s += " CASCADE"
+		}
+		out[i] = s
+	}
+	return out
+}
+
+// gone is what was held before and is not held now.
+func gone(before, left []string) []string {
+	still := map[string]bool{}
+	for _, l := range left {
+		still[l] = true
+	}
+	var out []string
+	for _, b := range before {
+		if !still[b] {
+			out = append(out, b)
+		}
+	}
+	return out
+}
+
 // readHeld is every privilege the reader holds in one database, table-level and column-level, each
 // with its grantor.
 func (c *Client) readHeld(ctx context.Context, db string) (held, error) {
@@ -612,21 +649,34 @@ func (c *Client) reconcileDatabase(ctx context.Context, db string, decl []Readab
 	}
 	stmts, rep := planDatabase(decl, existing, h, noUsage)
 	rep.Database, rep.Declared = db, decl != nil
+	// What was revoked is what the read-back shows gone, never what was asked for.
+	rep.Revoked = nil
 	if len(stmts) > 0 {
+		want := wantedColumns(decl, existing, &DatabaseReport{})
+		before := unwanted(h, want)
 		// One string: the simple protocol runs it as one implicit transaction, all or nothing.
-		if _, err := c.Query(ctx, db, strings.Join(stmts, "; ")); err != nil {
-			rep.Error = "changing the reader's grants: " + err.Error()
-			return rep
+		_, changeErr := c.Query(ctx, db, strings.Join(stmts, "; "))
+		if changeErr != nil && dependentPrivileges(changeErr) {
+			// The reader holds a grant option and passed the privilege on: a plain revoke refuses
+			// while those grants stand. CASCADE takes them too — they all came from the reader, since
+			// only grants depending on the reader's grant option depend on what is revoked here.
+			_, changeErr = c.Query(ctx, db, strings.Join(cascaded(stmts), "; "))
 		}
-		// Read back: what the reader still holds beyond what is wanted is a failure, said as one —
-		// never reported as revoked while it stands.
 		after, err := c.readHeld(ctx, db)
 		if err != nil {
 			rep.Error = "reading the reader's grants back: " + err.Error()
 			return rep
 		}
-		if left := unwanted(after, wantedColumns(decl, existing, &DatabaseReport{})); len(left) > 0 {
-			rep.Error = "STILL HELD after revoking: the reader can still use " + strings.Join(left, ", ")
+		left := unwanted(after, want)
+		rep.Revoked = gone(before, left)
+		if len(left) > 0 {
+			why := "the revoke ran and they stand"
+			if changeErr != nil {
+				why = "changing the reader's grants failed: " + changeErr.Error()
+			}
+			rep.Error = "STILL HELD: the reader can still use " + strings.Join(left, ", ") + " (" + why + ")"
+		} else if changeErr != nil {
+			rep.Error = "changing the reader's grants: " + changeErr.Error()
 		}
 	}
 	// Asked as the admin on the reader's behalf: whatever else gives the reader a secret (PUBLIC, a

modules/postgres/cmd/postgres-provider/readable_live_test.go

@@ -181,3 +181,51 @@ func TestLiveAGrantByAnotherGrantorIsTakenBackAndDefinersAreSaid(t *testing.T) {
 		}
 	}
 }
+
+// The reader was given a table WITH GRANT OPTION and passed it on. A plain revoke then fails with
+// "dependent privileges exist" and changes nothing; the pass must take back the reader's grant and
+// what the reader passed on (CASCADE reaches only grants that came from the reader), report as
+// revoked only what the read-back shows gone, and say what is still held and why if anything is.
+func TestLiveAGrantTheReaderPassedOnIsTakenBack(t *testing.T) {
+	c := liveClient(t)
+	freshStore(t, c)
+	defer dropStore(c)
+	defer func() { _, _ = c.Query(ctx, "", "DROP ROLE IF EXISTS mesh_test_onward") }()
+	if _, err := c.Query(ctx, "", "DROP ROLE IF EXISTS mesh_test_onward; CREATE ROLE mesh_test_onward"); err != nil {
+		t.Fatal(err)
+	}
+	for _, sql := range []string{
+		"REVOKE ALL ON vault FROM " + Ident(Reader),
+		"GRANT SELECT ON notes TO " + Ident(Reader) + " WITH GRANT OPTION",
+		"SET ROLE " + Ident(Reader) + "; GRANT SELECT ON notes TO mesh_test_onward; RESET ROLE",
+	} {
+		if _, err := c.Query(ctx, liveStore, sql); err != nil {
+			t.Fatal(sql, err)
+		}
+	}
+	var mine DatabaseReport
+	for _, db := range c.ReconcileReader(ctx, Declarations{}, "").Databases {
+		if db.Database == liveStore {
+			mine = db
+		}
+	}
+	if mine.Error != "" {
+		t.Fatal("left standing:", mine.Error)
+	}
+	if columnReadable(t, c, liveStore, "notes", "body") {
+		t.Fatalf("the reader still reads notes, and the pass said revoked %v", mine.Revoked)
+	}
+	if strings.Join(mine.Revoked, ",") != "public.notes" {
+		t.Fatalf("revoked %v", mine.Revoked)
+	}
+	r, err := c.Query(ctx, liveStore, "SELECT has_table_privilege('mesh_test_onward', 'notes', 'SELECT')")
+	if err != nil || cell(r.Rows[0], 0) != "f" {
+		t.Fatal("what the reader passed on survived:", r.Rows, err)
+	}
+	// The next pass finds nothing held, and says nothing revoked.
+	for _, db := range c.ReconcileReader(ctx, Declarations{}, "").Databases {
+		if db.Database == liveStore && (len(db.Revoked) > 0 || db.Error != "") {
+			t.Fatalf("%+v", db)
+		}
+	}
+}

modules/postgres/cmd/postgres-provider/readable_test.go

@@ -6,6 +6,7 @@ package main
 
 import (
 	"encoding/json"
+	"errors"
 	"os"
 	"path/filepath"
 	"strings"
@@ -354,3 +355,26 @@ func TestTheCataloguesReadableDeclarationsParse(t *testing.T) {
 		}
 	}
 }
+
+// A change that fails reports nothing revoked, and says what is still held and why — every pass, not
+// "revoked" every pass.
+func TestAFailedChangeSaysWhatIsStillHeldAndRevokesNothing(t *testing.T) {
+	conn := admin
+	conn.ReaderPassword = "r"
+	f, c := newFake(conn)
+	f.answer(`^SELECT rolsuper`, []string{"a", "b", "c", "d", "e", "f", "g", "h", "i"},
+		[]string{"f", "f", "f", "f", "f", "t", "t", "t", "default_transaction_read_only=on,statement_timeout=60s"})
+	f.answer(`SELECT datname FROM pg_database WHERE datallowconn`, []string{"datname"}, []string{"app"})
+	f.answer(`aclexplode\(c\.relacl\)`, []string{"nspname", "relname", "relkind", "privilege_type", "grantor"},
+		[]string{"public", "vault", "r", "SELECT", "owner1"})
+	f.answer(`aclexplode\(a\.attacl\)`, []string{"nspname", "relname", "attname", "privilege_type", "grantor"})
+	f.fail(`^SET ROLE`, errors.New("server busy"))
+	rep := c.ReconcileReader(ctx, Declarations{}, "")
+	db := rep.Databases[0]
+	if len(db.Revoked) != 0 {
+		t.Fatalf("said revoked: %v", db.Revoked)
+	}
+	if !strings.Contains(db.Error, "STILL HELD") || !strings.Contains(db.Error, "public.vault") || !strings.Contains(db.Error, "server busy") {
+		t.Fatal(db.Error)
+	}
+}