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

.github/workflows/ci.yaml

@@ -41,14 +41,14 @@ jobs:
       - name: Build baton-sql
         run: go build ./cmd/baton-sql
       - name: Run sync tests
-        uses: ConductorOne/github-workflows/actions/sync-test@v2
+        uses: ConductorOne/github-workflows/actions/sync-test@v4
         with:
           connector: ./baton-sql
           baton-entitlement: 'role:admin:member'
           baton-principal: john.smith
           baton-principal-type: user
       - 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

@@ -35,7 +35,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`).
 

cmd/baton-sql/main.go

@@ -2,11 +2,10 @@ package main
 
 import (
 	"context"
-	"fmt"
-	"os"
 
 	configSdk "github.com/conductorone/baton-sdk/pkg/config"
 	"github.com/conductorone/baton-sdk/pkg/connectorbuilder"
+	"github.com/conductorone/baton-sdk/pkg/exit"
 	"github.com/conductorone/baton-sdk/pkg/field"
 	"github.com/conductorone/baton-sdk/pkg/types"
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap"
@@ -31,16 +30,14 @@ func main() {
 		},
 	)
 	if err != nil {
-		fmt.Fprintln(os.Stderr, err.Error())
-		os.Exit(1)
+		exit.LogExit(err)
 	}
 
 	cmd.Version = version
 
 	err = cmd.Execute()
 	if err != nil {
-		fmt.Fprintln(os.Stderr, err.Error())
-		os.Exit(1)
+		exit.LogExit(err)
 	}
 }
 

docs/db2.md

@@ -140,6 +140,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. This is
+the shared behavior of every DDL-based engine (Db2 and Oracle), and 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,36 @@
+# 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.
+
+## Engines that report rows-affected
+
+On engines whose `GRANT`/`REVOKE` report how many rows they changed (SQLite, MySQL,
+PostgreSQL, SQL Server, HANA, Vertica), a `validation_query` returning no rows **fails the
+operation**. It is an existence precondition that aborts loudly.
+
+## DDL-based engines (Db2, Oracle)
+
+Db2 and Oracle apply `GRANT`/`REVOKE` as DDL that does not report rows-affected, so the
+connector cannot tell from the statement itself whether it changed anything. On Oracle a
+repeat `GRANT` succeeds without changing anything and an already-applied `REVOKE` raises
+`ORA-01951`; on Db2 an already-applied statement raises an error. To make grant and revoke
+idempotent, on these engines 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".
+
+Because of this, on Db2 and Oracle 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 or Oracle.** 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, Oracle) 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_oracle_test.go

@@ -0,0 +1,35 @@
+package bsql
+
+import (
+	"testing"
+
+	"github.com/conductorone/baton-sql/pkg/database"
+	"github.com/stretchr/testify/require"
+)
+
+// validationNoRowsMeansIdempotent is the DDL-engine gate: it must be true only for
+// engines whose already-applied GRANT/REVOKE raises an error instead of affecting rows,
+// so validation "no rows" means idempotency rather than a failed precondition.
+//
+// The behavioral grant/revoke wiring is covered by the Db2 tests in
+// provisioning_validation_idempotency_test.go; Oracle can't reuse them because the
+// Oracle driver rewrites ?<name> placeholders to bind syntax the sqlite test backend
+// rejects. Oracle's end-to-end behavior was verified live against Oracle XE 21c on
+// 2026-09-03 (grant/re-grant -> GrantAlreadyExists, revoke/re-revoke -> GrantAlreadyRevoked,
+// ORA-01951 no longer surfaced).
+func TestValidationNoRowsMeansIdempotent_EngineGate(t *testing.T) {
+	ddl := map[database.DbEngine]bool{
+		database.DB2:        true,
+		database.Oracle:     true,
+		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)
+	}
+}