fix: don't let foreign owned relations block database reconciliation

pkg/postgres/database.go

@@ -293,11 +293,15 @@ func revokeAllOnPublic(log logr.Logger, serviceConnection *sql.DB, serviceCreden
 	log.V(1).Info(fmt.Sprintf("Revoke ALL on role PUBLIC for database '%s'", serviceCredentials.Name))
 	err := execAsf(serviceConnection, serviceCredentials.User, `
 		REVOKE ALL ON DATABASE %s from PUBLIC;
-		REVOKE ALL ON SCHEMA public from PUBLIC;
-		REVOKE ALL ON ALL TABLES IN SCHEMA public from PUBLIC;`, serviceCredentials.Name)
+		REVOKE ALL ON SCHEMA public from PUBLIC;`, serviceCredentials.Name)
 	if err != nil {
 		return fmt.Errorf("revoke all for role PUBLIC on database '%s': %w, as %s", serviceCredentials.Name, err, serviceCredentials.User)
 	}
+
+	err = revokeAllOnExistingTablesFromPublicAs(serviceConnection, "public", serviceCredentials.User)
+	if err != nil {
+		return fmt.Errorf("revoke all on existing tables in schema public for role PUBLIC on database '%s': %w, as %s", serviceCredentials.Name, err, serviceCredentials.User)
+	}
 	return nil
 }
 
@@ -361,13 +365,60 @@ func setDefaultPrivilegesAs(db *sql.DB, schema, role, privileges, actor string)
 	if err != nil {
 		return fmt.Errorf("grant %s privileges on existing schema: %w, as %s", privileges, err, actor)
 	}
-	err = execAsf(db, actor, fmt.Sprintf("GRANT %s ON ALL TABLES IN SCHEMA %s TO %s", privileges, schema, role))
+	err = grantOnExistingTablesAs(db, schema, role, privileges, actor)
 	if err != nil {
 		return fmt.Errorf("grant %s privileges on existing tables: %w, as %s", privileges, err, actor)
 	}
 	return nil
 }
 
+// grantOnExistingTablesAs grants privileges on the existing relations in schema
+// that actor is allowed to grant on.
+func grantOnExistingTablesAs(db *sql.DB, schema, role, privileges, actor string) error {
+	return forEachOwnedRelationAs(db, schema, actor, fmt.Sprintf("GRANT %s ON TABLE %%s TO %s", privileges, role))
+}
+
+// revokeAllOnExistingTablesFromPublicAs revokes all privileges from the role
+// PUBLIC on the existing relations in schema that actor is allowed to revoke
+// on.
+func revokeAllOnExistingTablesFromPublicAs(db *sql.DB, schema, actor string) error {
+	return forEachOwnedRelationAs(db, schema, actor, "REVOKE ALL ON TABLE %s FROM PUBLIC")
+}
+
+// forEachOwnedRelationAs executes statement as grantingRole for every relation
+// in schema that grantingRole is allowed to grant and revoke privileges on,
+// ie. relations owned by grantingRole or by a role that grantingRole is a
+// member of. statement is a PostgreSQL format() template with a single %s
+// placeholder for the relation name.
+//
+// This intentionally does not use the GRANT/REVOKE ... ON ALL TABLES IN SCHEMA
+// statements as they fail hard with "permission denied for table x" if just a
+// single relation in the schema is owned by another role and actor holds no
+// privileges on it. That happens for relations created by extensions, as
+// extensions are installed by the admin user, and would block all further
+// reconciliation of the database.
+func forEachOwnedRelationAs(db *sql.DB, schema, grantingRole, statement string) error {
+	query := fmt.Sprintf(`
+		DO $$
+		DECLARE
+			relation regclass;
+		BEGIN
+			FOR relation IN
+				SELECT c.oid::regclass
+				FROM pg_class c
+				JOIN pg_namespace n ON n.oid = c.relnamespace
+				WHERE n.nspname = %s
+				  AND c.relkind IN ('r', 'p', 'v', 'm', 'f')
+				  AND pg_has_role(current_user, c.relowner, 'USAGE')
+			LOOP
+				EXECUTE format(%s, relation);
+			END LOOP;
+		END
+		$$;`, pq.QuoteLiteral(schema), pq.QuoteLiteral(statement))
+
+	return execAs(db, grantingRole, query)
+}
+
 // execf executes a formatted query on db.
 func execf(db *sql.DB, query string, args ...interface{}) error {
 	_, err := db.Exec(fmt.Sprintf(query, args...))
@@ -377,6 +428,17 @@ func execf(db *sql.DB, query string, args ...interface{}) error {
 	return nil
 }
 
+// execAs executes query on db as given role. Contrary to execAsf the query is
+// not treated as a format string.
+func execAs(db *sql.DB, role string, query string) error {
+	fullQuery := prependSetRole(query, role)
+	_, err := db.Exec(fullQuery)
+	if err != nil {
+		return fmt.Errorf("unable to execute query '%s'. %w", fullQuery, err)
+	}
+	return nil
+}
+
 // execf executes a formatted query on db as given role.
 func execAsf(db *sql.DB, role string, query string, args ...interface{}) error {
 	err := execf(db, prependSetRole(query, role), args...)

pkg/postgres/database_test.go

@@ -833,6 +833,172 @@ func TestDatabase_mixedOwnershipOnSharedDatabase(t *testing.T) {
 	assert.Equal(t, []string{"value-from-new-user", "value-from-shared-user"}, developerNonOwnedRows, "nonowned rows not as expected")
 }
 
+// TestDatabase_foreignOwnedRelationInServiceSchema verifies that a relation in
+// the service schema owned by another role, eg. a view created by an extension
+// installed by the admin user, does not block reconciliation. Such relations
+// are skipped when granting privileges on existing tables while relations owned
+// by the service user are still granted.
+func TestDatabase_foreignOwnedRelationInServiceSchema(t *testing.T) {
+	postgresqlHost := test.Integration(t)
+	log := test.SetLogger(t)
+	managerRole := "postgres_role_name"
+
+	db, err := postgres.Connect(postgres.ConnectionString{
+		Host:     postgresqlHost,
+		Database: "postgres",
+		User:     "iam_creator",
+		Password: "iam_creator",
+	})
+	require.NoError(t, err, "connect to database failed")
+	defer db.Close()
+
+	require.NoError(t, createManagerRole(log, db, managerRole), "create manager role failed")
+
+	name := fmt.Sprintf("test_%d", time.Now().UnixNano())
+	password := "test"
+	adminCredentials := postgres.Credentials{
+		User:     "iam_creator",
+		Password: "iam_creator",
+	}
+	serviceCredentials := postgres.Credentials{
+		Name:     name,
+		User:     name,
+		Password: password,
+	}
+
+	err = postgres.Database(log, postgresqlHost, adminCredentials, serviceCredentials, managerRole, nil)
+	require.NoError(t, err, "first Database call failed")
+
+	// connect as the admin user and create a relation in the service schema
+	// owned by the admin user without any privileges granted to the service user.
+	// This is the state an installed extension leaves behind.
+	adminConn, err := postgres.Connect(postgres.ConnectionString{
+		Host:     postgresqlHost,
+		Database: name,
+		User:     "iam_creator",
+		Password: "iam_creator",
+	})
+	require.NoError(t, err, "connect as admin to service database failed")
+	defer adminConn.Close()
+
+	dbExec(t, adminConn, `CREATE TABLE %s.extension_owned (title varchar(40) NOT NULL)`, name)
+	dbExec(t, adminConn, `CREATE VIEW %s.extension_owned_view AS SELECT * FROM %[1]s.extension_owned`, name)
+
+	// connect as the service user and create an owned table that should still get
+	// privileges granted
+	serviceConn, err := postgres.Connect(postgres.ConnectionString{
+		Host:     postgresqlHost,
+		Database: name,
+		User:     name,
+		Password: password,
+	})
+	require.NoError(t, err, "connect as service user to service database failed")
+	defer serviceConn.Close()
+
+	dbExec(t, serviceConn, `CREATE TABLE %s.owned (title varchar(40) NOT NULL)`, name)
+
+	// reconcile again. This used to fail with
+	// 'pq: permission denied for table extension_owned'
+	err = postgres.Database(log, postgresqlHost, adminCredentials, serviceCredentials, managerRole, nil)
+	require.NoError(t, err, "second Database call failed")
+
+	assert.True(t,
+		tableHasPrivilege(t, adminConn, fmt.Sprintf("%s_read", name), fmt.Sprintf("%s.owned", name), "SELECT"),
+		"read role should have SELECT on the service owned table",
+	)
+	assert.True(t,
+		tableHasPrivilege(t, adminConn, fmt.Sprintf("%s_readwrite", name), fmt.Sprintf("%s.owned", name), "INSERT"),
+		"readwrite role should have INSERT on the service owned table",
+	)
+	assert.False(t,
+		tableHasPrivilege(t, adminConn, fmt.Sprintf("%s_read", name), fmt.Sprintf("%s.extension_owned", name), "SELECT"),
+		"read role should not have SELECT on the foreign owned table",
+	)
+}
+
+func tableHasPrivilege(t *testing.T, db *sql.DB, role, table, privilege string) bool {
+	t.Helper()
+	var hasPrivilege bool
+	err := db.QueryRow("SELECT has_table_privilege($1, $2, $3)", role, table, privilege).Scan(&hasPrivilege)
+	require.NoError(t, err, "query table privilege failed")
+	return hasPrivilege
+}
+
+// TestDatabase_foreignOwnedRelationInPublicSchema verifies that a relation in
+// the public schema owned by another role, eg. an extension installed manually
+// without an explicit schema, does not block reconciliation. Such relations are
+// skipped when revoking privileges from PUBLIC while relations owned by the
+// service user are still revoked.
+func TestDatabase_foreignOwnedRelationInPublicSchema(t *testing.T) {
+	postgresqlHost := test.Integration(t)
+	log := test.SetLogger(t)
+	managerRole := "postgres_role_name"
+
+	db, err := postgres.Connect(postgres.ConnectionString{
+		Host:     postgresqlHost,
+		Database: "postgres",
+		User:     "iam_creator",
+		Password: "iam_creator",
+	})
+	require.NoError(t, err, "connect to database failed")
+	defer db.Close()
+
+	require.NoError(t, createManagerRole(log, db, managerRole), "create manager role failed")
+
+	name := fmt.Sprintf("test_%d", time.Now().UnixNano())
+	password := "test"
+	adminCredentials := postgres.Credentials{
+		User:     "iam_creator",
+		Password: "iam_creator",
+	}
+	serviceCredentials := postgres.Credentials{
+		Name:     name,
+		User:     name,
+		Password: password,
+	}
+
+	err = postgres.Database(log, postgresqlHost, adminCredentials, serviceCredentials, managerRole, nil)
+	require.NoError(t, err, "first Database call failed")
+
+	// create a relation in the public schema owned by the admin user without any
+	// privileges granted to the service user. This is the state a manually
+	// installed extension leaves behind, as public is the default schema.
+	adminConn, err := postgres.Connect(postgres.ConnectionString{
+		Host:     postgresqlHost,
+		Database: name,
+		User:     "iam_creator",
+		Password: "iam_creator",
+	})
+	require.NoError(t, err, "connect as admin to service database failed")
+	defer adminConn.Close()
+
+	dbExec(t, adminConn, `CREATE TABLE public.extension_owned (title varchar(40) NOT NULL)`)
+
+	// create a table in public owned by the service user with privileges granted
+	// to PUBLIC. These privileges are expected to be revoked on reconcile.
+	serviceConn, err := postgres.Connect(postgres.ConnectionString{
+		Host:     postgresqlHost,
+		Database: name,
+		User:     name,
+		Password: password,
+	})
+	require.NoError(t, err, "connect as service user to service database failed")
+	defer serviceConn.Close()
+
+	dbExec(t, serviceConn, `CREATE TABLE public.owned (title varchar(40) NOT NULL)`)
+	dbExec(t, serviceConn, `GRANT SELECT ON public.owned TO PUBLIC`)
+
+	// reconcile again. This used to fail with
+	// 'pq: permission denied for table extension_owned'
+	err = postgres.Database(log, postgresqlHost, adminCredentials, serviceCredentials, managerRole, nil)
+	require.NoError(t, err, "second Database call failed")
+
+	assert.False(t,
+		tableHasPrivilege(t, adminConn, "public", "public.owned", "SELECT"),
+		"PUBLIC should not have SELECT on the service owned table in schema public",
+	)
+}
+
 func TestDatabase_idempotency(t *testing.T) {
 	postgresqlHost := test.Integration(t)
 	log := test.SetLogger(t)