CXH-2379: fix grant/revoke idempotency for DDL-based engines

.github/workflows/ci.yaml

@@ -49,7 +49,7 @@ jobs:
           baton-principal-type: user
           bad-credentials: DB_PASSWORD=invalid
       - name: Run account provisioning tests
-        uses: ConductorOne/github-workflows/actions/account-provisioning@v3
+        uses: ConductorOne/github-workflows/actions/account-provisioning@v4
         with:
           connector: ./baton-sql
           account-email: robert.tables2@example.com

README.md

@@ -37,7 +37,7 @@ The connector is configured using a YAML file that defines:
 - **Resource Types**: Map database tables/queries to resources (users, roles, etc.)
 - **Account Provisioning**: Define schemas and credential options for user creation
 - **Entitlements**: Permissions and roles that can be granted to resources
-- **Provisioning Actions**: SQL queries for granting/revoking entitlements
+- **Provisioning Actions**: SQL queries for granting/revoking entitlements; see [docs/provisioning.md](docs/provisioning.md) for `validation_queries` semantics (including the DDL-engine no-rows-means-idempotent behavior on Db2 and Oracle)
 
 For Postgres behind a transaction-mode pooler (PgBouncer, Supabase pooler on port 6543, etc.), set `default_query_exec_mode` to `simple_protocol` via the DSN query string or `connect.params` to avoid prepared-statement conflicts (SQLSTATE 42P05). When unset, baton-sql leaves the URL unchanged and pgx uses its default (`cache_statement`).
 

docs/db2.md

@@ -221,6 +221,15 @@ the OS libxml2 package: `apt-get install libxml2` / `yum install libxml2`.
 **`go vet` / `golangci-lint` with `-tags db2` fails** — type-checking the tagged path needs
 the clidriver headers too. Default-tag lint and vet need nothing.
 
+## Provisioning: `validation_queries` semantics
+
+Db2 is DDL-based: its `GRANT`/`REVOKE` don't report rows-affected, so a `validation_query`
+returning no rows is treated as an idempotent success, not a failed precondition. Db2 is the
+only engine with this behavior today, and it ships opt-in behind the `db2` build tag. It means
+you must not use `validation_queries` as existence preconditions on Db2. See
+[Provisioning: `validation_queries` semantics](provisioning.md) for the full explanation and
+examples.
+
 ## Docker
 
 - The default release pipeline (goreleaser, `CGO_ENABLED=0`) is unaffected — DB2 does not

docs/provisioning.md

@@ -0,0 +1,37 @@
+# Provisioning: `validation_queries` semantics
+
+`validation_queries` run before the provisioning `queries` in a grant or revoke. What a
+**no-rows** result means depends on the engine.
+
+## Default: no rows fails the operation
+
+On every engine except Db2, a `validation_query` returning no rows **fails the operation**.
+It is an existence precondition that aborts loudly. This includes the DDL engines that don't
+report rows-affected (Oracle): treating their no-rows as idempotency is a follow-up that needs
+a per-config opt-in first, so today they still fail loudly like everyone else.
+
+## Db2 (opt-in behind the `db2` build tag)
+
+Db2 applies `GRANT`/`REVOKE` as DDL that does not report rows-affected, so the connector
+cannot tell from the statement itself whether it changed anything, and an already-applied
+statement raises an error. To make grant and revoke idempotent, a `validation_query`
+returning no rows is reported as an **idempotent success** (`GrantAlreadyExists` on grant,
+`GrantAlreadyRevoked` on revoke). No rows means "the state is already as desired, there is no
+work to do". Db2 ships opt-in behind the `db2` build tag, so no default-build engine changes
+behavior.
+
+Because of this, on Db2 your `validation_queries` must answer **"is there work to do?"**, not
+**"does this principal or role exist?"**.
+
+**Do not use `validation_queries` as existence preconditions on Db2.** A no-rows result is
+swallowed as idempotent success, so a missing, deleted, or mistyped principal or role is
+reported as "already done" instead of erroring. For example, a validation query like
+`SELECT 1 FROM users WHERE name = ?<user_id>` will silently mask a bad `user_id`: it returns
+no rows, and the grant is reported as `GrantAlreadyExists` even though nothing was granted.
+
+Write the query so no-rows genuinely means idempotent. For a grant, check whether the target
+membership is **missing** (no rows => already granted); for a revoke, check whether it is
+**present** (no rows => already revoked).
+
+This mirrors the warning on `EntitlementProvisioningQueries.ValidationQueries` in
+`pkg/bsql/config.go`.

pkg/bsql/config.go

@@ -422,7 +422,15 @@ type EntitlementProvisioningQueries struct {
 	// NoTransaction indicates whether the provisioning queries should be executed without a transaction.
 	NoTransaction bool `yaml:"no_transaction,omitempty" json:"no_transaction,omitempty"`
 
-	// ValidationQueries is a list of SQL statements to execute for validating the provisioning operation before execution.
+	// ValidationQueries is a list of SQL statements run before the provisioning queries.
+	// On engines that report rows-affected, a query returning no rows fails the operation
+	// (an existence precondition). On DDL-based engines (Db2) that don't report rows-affected,
+	// a query returning no rows instead means the state is already as desired, so the operation
+	// is reported as an idempotent success (GrantAlreadyExists / GrantAlreadyRevoked).
+	//
+	// Warning: on DDL-based engines, do NOT use these as existence preconditions
+	// (e.g. "does this user/role exist?"). A no-rows result is reported as idempotent
+	// success, so a missing or mistyped principal is silently swallowed instead of erroring.
 	ValidationQueries []string `yaml:"validation_queries,omitempty" json:"validation_queries,omitempty"`
 
 	// Queries is a list of SQL statements to execute for the provisioning operation.

pkg/bsql/provisioning.go

@@ -88,9 +88,15 @@ func (s *SQLSyncer) Grant(ctx context.Context, principal *v2.Resource, entitleme
 	if err != nil {
 		if errors.Is(err, ErrQueryAffectedZeroRows) {
 			l.Debug("entitlement is already granted", zap.String("entitlement_id", entitlement.GetId()))
-			anno := annotations.Annotations{}
-			anno.Update(&v2.GrantAlreadyExists{})
-			return anno, nil
+			// On the transactional path the zero-rows return rolls the tx back, undoing any
+			// grant_replace revoke, so a reused GrantReplaced would misreport a removal the DB
+			// no longer reflects. Keep the returned annotations only on the no_transaction path,
+			// where the replace already committed.
+			if provisioningConfig.Grant.NoTransaction {
+				anno.Update(&v2.GrantAlreadyExists{})
+				return anno, nil
+			}
+			return annotations.New(&v2.GrantAlreadyExists{}), nil
 		}
 		return nil, err
 	}

pkg/bsql/provisioning_grant_replace_test.go

@@ -0,0 +1,162 @@
+package bsql
+
+import (
+	"database/sql"
+	"testing"
+
+	v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2"
+	"github.com/conductorone/baton-sql/pkg/bcel"
+	"github.com/conductorone/baton-sql/pkg/database"
+	"github.com/stretchr/testify/require"
+	_ "modernc.org/sqlite"
+)
+
+// withGrantReplaceConfig wires a "member" entitlement whose grant replaces the
+// principal's existing role: the grant_replace query finds the old membership and
+// revokes it, then the main grant runs. The main grant uses INSERT OR IGNORE so a
+// pre-existing target row makes it affect zero rows (the already-granted path).
+func withGrantReplaceConfig(s *SQLSyncer, noTransaction bool) {
+	s.resourceType = &v2.ResourceType{Id: "role"}
+	s.config = ResourceType{
+		StaticEntitlements: []*EntitlementMapping{
+			{
+				Id: "member",
+				Provisioning: &EntitlementProvisioning{
+					Vars: map[string]string{
+						"user_id": "principal.ID",
+						"role":    "resource.ID",
+					},
+					Grant: &GrantEntitlementProvisioningQueries{
+						EntitlementProvisioningQueries: EntitlementProvisioningQueries{
+							NoTransaction: noTransaction,
+							Queries:       []string{`INSERT OR IGNORE INTO user_roles (user_id, role) VALUES (?<user_id>, ?<role>)`},
+						},
+						GrantReplace: &GrantReplaceProvisioningQueries{
+							Query: `SELECT user_id, role FROM user_roles WHERE user_id = ?<user_id> AND role = 'viewer'`,
+							Map: []*GrantMapping{
+								{
+									EntitlementResourceId: ".role",
+									PrincipalId:           ".user_id",
+									PrincipalType:         "user",
+									Entitlement:           "member",
+								},
+							},
+						},
+					},
+					Revoke: &RevokeEntitlementProvisioningQueries{
+						EntitlementProvisioningQueries: EntitlementProvisioningQueries{
+							Queries: []string{`DELETE FROM user_roles WHERE user_id = ?<user_id> AND role = ?<role>`},
+						},
+					},
+				},
+			},
+		},
+	}
+}
+
+func newGrantReplaceTestSyncer(t *testing.T) (*SQLSyncer, *sql.DB) {
+	t.Helper()
+
+	db, err := sql.Open("sqlite", ":memory:")
+	require.NoError(t, err)
+	db.SetMaxOpenConns(1)
+	t.Cleanup(func() { require.NoError(t, db.Close()) })
+
+	_, err = db.ExecContext(t.Context(), `CREATE TABLE user_roles (user_id TEXT, role TEXT, UNIQUE(user_id, role))`)
+	require.NoError(t, err)
+
+	env, err := bcel.NewEnv(t.Context())
+	require.NoError(t, err)
+
+	return &SQLSyncer{
+		db:            db,
+		dbs:           map[string]*sql.DB{"primary": db},
+		dbNames:       []string{"primary"},
+		primaryDBName: "primary",
+		currentDBName: "primary",
+		dbEngine:      database.SQLite,
+		env:           env,
+	}, db
+}
+
+// Transactional path: the target grant already exists, so the main grant hits the
+// zero-rows sentinel and the tx rolls back, undoing the grant_replace revoke. The
+// response must NOT claim GrantReplaced, and the old row must survive.
+func TestGrant_ReplaceRolledBackDoesNotReportGrantReplaced(t *testing.T) {
+	s, db := newGrantReplaceTestSyncer(t)
+	withGrantReplaceConfig(s, false) // transactional
+	_, err := db.ExecContext(t.Context(), `INSERT INTO user_roles (user_id, role) VALUES ('user-1','viewer'), ('user-1','admin')`)
+	require.NoError(t, err)
+
+	annos, err := s.Grant(t.Context(), userPrincipal("user-1"), memberEntitlementFor("admin"))
+	require.NoError(t, err)
+
+	exists, err := annos.Pick(&v2.GrantAlreadyExists{})
+	require.NoError(t, err)
+	require.True(t, exists)
+
+	replaced, err := annos.Pick(&v2.GrantReplaced{})
+	require.NoError(t, err)
+	require.False(t, replaced, "GrantReplaced must not be reported when the tx rolled back")
+
+	// the replace revoke was rolled back, so the old membership survives
+	require.Equal(t, 1, countRows(t, db, `SELECT COUNT(*) FROM user_roles WHERE user_id = ? AND role = ?`, "user-1", "viewer"))
+}
+
+// no_transaction path: the grant_replace revoke commits immediately, so even when the
+// main grant hits the zero-rows sentinel the removal really happened and GrantReplaced
+// must be reported.
+func TestGrant_ReplaceCommittedReportsGrantReplaced(t *testing.T) {
+	s, db := newGrantReplaceTestSyncer(t)
+	withGrantReplaceConfig(s, true) // no_transaction
+	_, err := db.ExecContext(t.Context(), `INSERT INTO user_roles (user_id, role) VALUES ('user-1','viewer'), ('user-1','admin')`)
+	require.NoError(t, err)
+
+	annos, err := s.Grant(t.Context(), userPrincipal("user-1"), memberEntitlementFor("admin"))
+	require.NoError(t, err)
+
+	exists, err := annos.Pick(&v2.GrantAlreadyExists{})
+	require.NoError(t, err)
+	require.True(t, exists)
+
+	replaced, err := annos.Pick(&v2.GrantReplaced{})
+	require.NoError(t, err)
+	require.True(t, replaced, "GrantReplaced must be reported when the replace committed")
+
+	// the replace revoke committed, so the old membership is gone
+	require.Equal(t, 0, countRows(t, db, `SELECT COUNT(*) FROM user_roles WHERE user_id = ? AND role = ?`, "user-1", "viewer"))
+}
+
+// withGrantReplaceDB2Config is the grant_replace config with a revoke validation
+// query that never matches. On Db2 a no-rows validation means "nothing to revoke",
+// so the revoke aborts before its DELETE runs but the flow still reports GrantReplaced.
+func withGrantReplaceDB2Config(s *SQLSyncer) {
+	withGrantReplaceConfig(s, true) // no_transaction: the replace stands on its own
+	revoke := s.config.StaticEntitlements[0].Provisioning.Revoke
+	revoke.ValidationQueries = []string{
+		`SELECT 1 FROM user_roles WHERE user_id = ?<user_id> AND role = 'does-not-exist'`,
+	}
+}
+
+// Db2 path: the revoke validation query returns no rows, so the revoke DELETE never
+// runs, yet GrantReplaced is still reported because on Db2 a no-rows validation means
+// the old grant is already gone. The old viewer row must survive (revoke never ran).
+func TestGrant_ReplaceDB2RevokeValidationNoRowsStillReportsGrantReplaced(t *testing.T) {
+	s, db := newGrantReplaceTestSyncer(t)
+	s.dbEngine = database.DB2
+	withGrantReplaceDB2Config(s)
+	_, err := db.ExecContext(t.Context(), `INSERT INTO user_roles (user_id, role) VALUES ('user-1','viewer')`)
+	require.NoError(t, err)
+
+	annos, err := s.Grant(t.Context(), userPrincipal("user-1"), memberEntitlementFor("admin"))
+	require.NoError(t, err)
+
+	replaced, err := annos.Pick(&v2.GrantReplaced{})
+	require.NoError(t, err)
+	require.True(t, replaced, "GrantReplaced must be reported: on Db2 a no-rows revoke validation means the old grant is already gone")
+
+	// the revoke validation aborted the revoke before its DELETE ran, so viewer survives
+	require.Equal(t, 1, countRows(t, db, `SELECT COUNT(*) FROM user_roles WHERE user_id = ? AND role = ?`, "user-1", "viewer"))
+	// the main grant still ran
+	require.Equal(t, 1, countRows(t, db, `SELECT COUNT(*) FROM user_roles WHERE user_id = ? AND role = ?`, "user-1", "admin"))
+}

pkg/bsql/provisioning_revoke_deleted_test.go

@@ -144,6 +144,27 @@ func TestRunRevokeProvisioning_AllZeroRowsWithSurvivingPrincipal(t *testing.T) {
 	require.Equal(t, 1, countRows(t, db, `SELECT COUNT(*) FROM users WHERE id = ?`, "user-1"))
 }
 
+// On a DDL engine, a revoke whose validation query returns no rows short-circuits
+// before any revoke runs. The principal-exists probe must be skipped: otherwise a
+// mistyped principal_id (validation AND probe both empty) would falsely report the
+// still-present principal as deleted.
+func TestRunRevokeProvisioning_DDLValidationNoRowsSkipsExistsCheck(t *testing.T) {
+	s, _ := newRevokeProvisioningTestSyncer(t)
+	s.dbEngine = database.DB2
+	// nothing seeded: the revoke validation query returns no rows, and the exists-check
+	// would also return no rows for user-1 — but no revoke ran, so no deletion happened.
+	deleted, err := s.RunRevokeProvisioning(
+		t.Context(),
+		[]string{`DELETE FROM user_roles WHERE user_id = ?<principal_id> AND role = ?<role>`},
+		[]string{`SELECT 1 FROM user_roles WHERE user_id = ?<principal_id> AND role = ?<role>`},
+		principalExistsCheck(),
+		map[string]any{"principal_id": "user-1", "role": "admin"},
+		true,
+	)
+	require.ErrorIs(t, err, ErrQueryAffectedZeroRows)
+	require.False(t, deleted, "exists-check must be skipped when the sentinel came from validation")
+}
+
 func TestRunRevokeProvisioning_NoExistsCheckBehavesLikeBefore(t *testing.T) {
 	s, db := newRevokeProvisioningTestSyncer(t)
 	seedUserWithRoles(t, db, "user-1", "admin")

pkg/bsql/provisioning_validation_idempotency_gate_test.go

@@ -0,0 +1,32 @@
+package bsql
+
+import (
+	"testing"
+
+	"github.com/conductorone/baton-sql/pkg/database"
+	"github.com/stretchr/testify/require"
+)
+
+// validationNoRowsMeansIdempotent is the DDL-engine gate: validation "no rows" is only
+// treated as idempotency (not a failed precondition) for engines whose already-applied
+// GRANT/REVOKE raises an error instead of affecting rows. Only Db2 qualifies today, and it
+// ships opt-in behind the db2 build tag. Oracle and the other DDL engines stay false: they
+// ship default-on, so flipping the gate would silently reinterpret existing configs that use
+// validation_queries as loud existence preconditions. Adding one back needs a per-config
+// opt-in first, so this test guards against re-enabling any of them by accident.
+func TestValidationNoRowsMeansIdempotent_EngineGate(t *testing.T) {
+	ddl := map[database.DbEngine]bool{
+		database.DB2:        true,
+		database.Oracle:     false,
+		database.SQLite:     false,
+		database.MySQL:      false,
+		database.PostgreSQL: false,
+		database.MSSQL:      false,
+		database.HDB:        false,
+		database.Vertica:    false,
+	}
+	for engine, want := range ddl {
+		s := &SQLSyncer{dbEngine: engine}
+		require.Equal(t, want, s.validationNoRowsMeansIdempotent(), "engine=%v", engine)
+	}
+}