From dc46a6de343d61e510389103a70ead3fce425b1a Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Wed, 9 Sep 2026 11:46:09 -0500 Subject: [PATCH 1/5] gate ddl no-rows idempotency behind per-config opt-in for oracle and db2 --- README.md | 4 +- docs/db2.md | 9 +-- docs/oracle.md | 57 ++++++++++++++++ docs/provisioning.md | 67 ++++++++++++------- examples/oracle-test.yml | 33 +++++++++ pkg/bsql/config.go | 19 ++++-- pkg/bsql/provisioning.go | 2 + pkg/bsql/provisioning_grant_reject_test.go | 2 + pkg/bsql/provisioning_grant_replace_test.go | 1 + pkg/bsql/provisioning_revoke_deleted_test.go | 8 +++ ...ioning_validation_idempotency_gate_test.go | 21 +++--- ...rovisioning_validation_idempotency_test.go | 41 ++++++++++-- pkg/bsql/query.go | 49 +++++++++----- pkg/bsql/user_syncer.go | 4 +- pkg/connector/action.go | 2 +- 15 files changed, 250 insertions(+), 69 deletions(-) create mode 100644 docs/oracle.md diff --git a/README.md b/README.md index 6cf5ddac..d92d607f 100644 --- a/README.md +++ b/README.md @@ -21,7 +21,7 @@ - MySQL - Microsoft SQL Server -- Oracle +- Oracle — see [docs/oracle.md](docs/oracle.md) for idempotent grant/revoke - PostgreSQL - SAP HANA - Vertica @@ -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; see [docs/provisioning.md](docs/provisioning.md) for `validation_queries` semantics (including the DDL-engine no-rows-means-idempotent behavior, currently Db2 only) +- **Provisioning Actions**: SQL queries for granting/revoking entitlements; see [docs/provisioning.md](docs/provisioning.md) for `validation_queries` semantics (including the opt-in `validation_queries_signal_idempotency` that makes no-rows mean idempotent success on the DDL engines 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`). diff --git a/docs/db2.md b/docs/db2.md index d6be1211..8e58e494 100644 --- a/docs/db2.md +++ b/docs/db2.md @@ -223,10 +223,11 @@ 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 +Db2 is DDL-based: its `GRANT`/`REVOKE` don't report rows-affected. When you set +`validation_queries_signal_idempotency: true` on a grant or revoke, a `validation_query` +returning no rows is treated as an idempotent success rather than a failed precondition. The +flag is off by default, so you must opt in per entitlement; when it is on, you must not use +`validation_queries` as existence preconditions. See [Provisioning: `validation_queries` semantics](provisioning.md) for the full explanation and examples. diff --git a/docs/oracle.md b/docs/oracle.md new file mode 100644 index 00000000..4ae44add --- /dev/null +++ b/docs/oracle.md @@ -0,0 +1,57 @@ +# Oracle + +baton-sql talks to Oracle through the pure-Go `go-ora` driver, so no native client is needed +(unlike Db2). Connect with an `oracle://` DSN: + +```yaml +connect: + dsn: "oracle://${DB_HOST}:${DB_PORT}/${DB_SERVICE}" + user: "${DB_USER}" + password: "${DB_PASSWORD}" +``` + +See [examples/oracle-test.yml](../examples/oracle-test.yml) for a full config (users, roles, +privileges, account provisioning, enable/disable/update actions). + +## Idempotent grant / revoke: `validation_queries_signal_idempotency` + +Oracle applies `GRANT`/`REVOKE` of roles and privileges as DDL that does not report +rows-affected, and re-running an already-applied statement raises an error. A repeat revoke of a +role the user no longer has fails with `ORA-01951: ROLE '...' not granted to '...'`. So a resync +that re-issues a revoke, or a grant of a role the user already holds, would surface as a failure +even though nothing needs to change. + +To make grant and revoke idempotent, set `validation_queries_signal_idempotency: true` on the +grant or revoke and add a `validation_query` that answers **"is there work to do?"**. When the +query returns no rows, the connector reports an idempotent success (`GrantAlreadyExists` on grant, +`GrantAlreadyRevoked` on revoke) instead of running the DDL and hitting the error. + +```yaml +grant: + no_transaction: true + validation_queries_signal_idempotency: true + # returns a row only while the role is NOT yet granted (no rows => already granted) + validation_queries: + - | + SELECT 1 FROM dual WHERE NOT EXISTS ( + SELECT 1 FROM DBA_ROLE_PRIVS + WHERE GRANTEE = UPPER(?) AND GRANTED_ROLE = UPPER(?) + ) + queries: + - GRANT ? TO ? +revoke: + no_transaction: true + validation_queries_signal_idempotency: true + # returns a row only while the role IS still granted (no rows => already revoked) + validation_queries: + - | + SELECT 1 FROM DBA_ROLE_PRIVS + WHERE GRANTEE = UPPER(?) AND GRANTED_ROLE = UPPER(?) + queries: + - REVOKE ? FROM ? +``` + +The flag is off by default, so without it Oracle keeps failing loudly on the repeat operation. +The same warning as Db2 applies: with the flag on, do **not** use `validation_queries` as +existence preconditions, since a no-rows result is swallowed as an idempotent success. See +[provisioning.md](provisioning.md) for the full explanation. diff --git a/docs/provisioning.md b/docs/provisioning.md index 46c97ede..1d5e33c7 100644 --- a/docs/provisioning.md +++ b/docs/provisioning.md @@ -1,37 +1,58 @@ # 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. +**no-rows** result means depends on one per-entitlement flag. ## 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 +By default a `validation_query` returning no rows **fails the operation**. It is an existence +precondition that aborts loudly. This is the behavior on every engine unless you opt in below. + +## Opt-in: no rows means idempotent success + +Some engines apply `GRANT`/`REVOKE` as DDL that does not report rows-affected (Db2, Oracle), so +the connector cannot tell from the statement whether it changed anything, and re-running an +already-applied statement raises an error (Oracle `ORA-01951` on a repeat revoke, for example). +To make grant and revoke idempotent on these engines, set `validation_queries_signal_idempotency: +true` on the grant or revoke: + +```yaml +grant: + validation_queries_signal_idempotency: true + validation_queries: + - SELECT 1 FROM ... # returns a row only while the grant is MISSING + queries: + - GRANT ... +revoke: + validation_queries_signal_idempotency: true + validation_queries: + - SELECT 1 FROM ... # returns a row only while the grant is PRESENT + queries: + - REVOKE ... +``` + +With the flag on, 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". + +The flag only takes effect on the DDL engines that need it (Db2, Oracle). On every other engine a +no-rows result still fails loudly regardless of the flag, because those engines report +rows-affected and don't need the reinterpretation. Default off, so existing configs are unchanged. + +## Writing the query: "is there work to do?", not "does this exist?" + +When the flag is on, 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 = ?` will silently mask a bad `user_id`: it returns -no rows, and the grant is reported as `GrantAlreadyExists` even though nothing was granted. +**Do not use them as existence preconditions.** 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 query like `SELECT 1 FROM users WHERE name = ?` +silently masks 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`. See [oracle.md](oracle.md) and [db2.md](db2.md) for engine-specific notes. diff --git a/examples/oracle-test.yml b/examples/oracle-test.yml index 36f27241..3f60b46a 100644 --- a/examples/oracle-test.yml +++ b/examples/oracle-test.yml @@ -301,12 +301,29 @@ resource_types: grant: # Indicates that no database transaction is needed for the grant operation no_transaction: true + # Oracle GRANT is DDL with no rows-affected: opt into treating a no-rows validation + # result as an idempotent success so re-granting a held role reports GrantAlreadyExists. + validation_queries_signal_idempotency: true + # Returns a row only while the role is NOT yet granted (no rows => already granted). + validation_queries: + - | + SELECT 1 FROM dual WHERE NOT EXISTS ( + SELECT 1 FROM DBA_ROLE_PRIVS + WHERE GRANTEE = UPPER(?) AND GRANTED_ROLE = UPPER(?) + ) queries: - | GRANT ? TO ? revoke: # Indicates that the revoke operation runs without a transaction no_transaction: true + # Without this, a repeat revoke of an already-removed role fails with ORA-01951. + validation_queries_signal_idempotency: true + # Returns a row only while the role IS still granted (no rows => already revoked). + validation_queries: + - | + SELECT 1 FROM DBA_ROLE_PRIVS + WHERE GRANTEE = UPPER(?) AND GRANTED_ROLE = UPPER(?) queries: - | REVOKE ? FROM ? @@ -391,6 +408,15 @@ resource_types: privilege_name: resource.ID # Maps the privilege identifier grant: no_transaction: true + # Oracle GRANT is DDL with no rows-affected: report a held privilege as GrantAlreadyExists. + validation_queries_signal_idempotency: true + # Returns a row only while the privilege is NOT yet granted (no rows => already granted). + validation_queries: + - | + SELECT 1 FROM dual WHERE NOT EXISTS ( + SELECT 1 FROM DBA_SYS_PRIVS + WHERE GRANTEE = UPPER(?) AND PRIVILEGE = UPPER(?) + ) queries: - | GRANT ? TO ? @@ -398,6 +424,13 @@ resource_types: revoke: # no_transaction indicates this should execute outside a transaction block no_transaction: true + # Without this, a repeat revoke of an already-removed privilege fails loudly. + validation_queries_signal_idempotency: true + # Returns a row only while the privilege IS still granted (no rows => already revoked). + validation_queries: + - | + SELECT 1 FROM DBA_SYS_PRIVS + WHERE GRANTEE = UPPER(?) AND PRIVILEGE = UPPER(?) # SQL queries to execute when revoking the privilege queries: - | diff --git a/pkg/bsql/config.go b/pkg/bsql/config.go index b5383193..bd0950b5 100644 --- a/pkg/bsql/config.go +++ b/pkg/bsql/config.go @@ -423,15 +423,22 @@ type EntitlementProvisioningQueries struct { NoTransaction bool `yaml:"no_transaction,omitempty" json:"no_transaction,omitempty"` // 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). + // A query returning no rows fails the operation (an existence precondition), unless + // ValidationQueriesSignalIdempotency opts into the DDL no-rows-means-idempotent behavior. + ValidationQueries []string `yaml:"validation_queries,omitempty" json:"validation_queries,omitempty"` + + // ValidationQueriesSignalIdempotency opts this entitlement into treating a no-rows + // ValidationQueries result as an idempotent success (GrantAlreadyExists on grant, + // GrantAlreadyRevoked on revoke) instead of a failed precondition. It only takes effect + // on DDL engines whose GRANT/REVOKE don't report rows-affected (Db2, Oracle); on every + // other engine a no-rows result still fails loudly regardless of this flag. Default off. // - // Warning: on DDL-based engines, do NOT use these as existence preconditions + // Warning: when enabled, do NOT use ValidationQueries 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"` + // Write the query so no-rows genuinely means "already in the desired state": for a grant, + // check the membership is missing; for a revoke, check it is present. + ValidationQueriesSignalIdempotency bool `yaml:"validation_queries_signal_idempotency,omitempty" json:"validation_queries_signal_idempotency,omitempty"` // Queries is a list of SQL statements to execute for the provisioning operation. Queries []string `yaml:"queries,omitempty" json:"queries,omitempty"` diff --git a/pkg/bsql/provisioning.go b/pkg/bsql/provisioning.go index 91fcedad..3ae7a091 100644 --- a/pkg/bsql/provisioning.go +++ b/pkg/bsql/provisioning.go @@ -80,6 +80,7 @@ func (s *SQLSyncer) Grant(ctx context.Context, principal *v2.Resource, entitleme principal, provisioningConfig.Grant.Queries, provisioningConfig.Grant.ValidationQueries, + provisioningConfig.Grant.ValidationQueriesSignalIdempotency, provisioningVars, useTx, provisioningConfig.Grant.GrantReplace, @@ -149,6 +150,7 @@ func (s *SQLSyncer) Revoke(ctx context.Context, grant *v2.Grant) (annotations.An ctx, provisioningConfig.Revoke.Queries, provisioningConfig.Revoke.ValidationQueries, + provisioningConfig.Revoke.ValidationQueriesSignalIdempotency, existsCheck, provisioningVars, useTx, diff --git a/pkg/bsql/provisioning_grant_reject_test.go b/pkg/bsql/provisioning_grant_reject_test.go index bb9bfc4f..ce88bc92 100644 --- a/pkg/bsql/provisioning_grant_reject_test.go +++ b/pkg/bsql/provisioning_grant_reject_test.go @@ -49,6 +49,7 @@ func TestRunGrantProvisioning_RejectIfMatchReturnsGrantCancelledAndSkipsMutation nil, []string{`INSERT INTO user_roles (user_id, role) VALUES (?, ?)`}, nil, + false, map[string]any{ "user_id": "user-1", "role": "admin", @@ -80,6 +81,7 @@ func TestRunGrantProvisioning_RejectIfNoMatchProceedsWithGrant(t *testing.T) { nil, []string{`INSERT INTO user_roles (user_id, role) VALUES (?, ?)`}, nil, + false, map[string]any{ "user_id": "user-1", "role": "admin", diff --git a/pkg/bsql/provisioning_grant_replace_test.go b/pkg/bsql/provisioning_grant_replace_test.go index a919965b..edca262d 100644 --- a/pkg/bsql/provisioning_grant_replace_test.go +++ b/pkg/bsql/provisioning_grant_replace_test.go @@ -136,6 +136,7 @@ func withGrantReplaceDB2Config(s *SQLSyncer) { revoke.ValidationQueries = []string{ `SELECT 1 FROM user_roles WHERE user_id = ? AND role = 'does-not-exist'`, } + revoke.ValidationQueriesSignalIdempotency = true } // Db2 path: the revoke validation query returns no rows, so the revoke DELETE never diff --git a/pkg/bsql/provisioning_revoke_deleted_test.go b/pkg/bsql/provisioning_revoke_deleted_test.go index 194f99d7..ff62b67c 100644 --- a/pkg/bsql/provisioning_revoke_deleted_test.go +++ b/pkg/bsql/provisioning_revoke_deleted_test.go @@ -77,6 +77,7 @@ func TestRunRevokeProvisioning_LastRoleDeletesPrincipal(t *testing.T) { t.Context(), revokeDeletesUserQueries, nil, + false, principalExistsCheck(), map[string]any{"principal_id": "user-1", "role": "admin"}, true, @@ -96,6 +97,7 @@ func TestRunRevokeProvisioning_KeepsPrincipalWhenOtherRolesRemain(t *testing.T) t.Context(), revokeDeletesUserQueries, nil, + false, principalExistsCheck(), map[string]any{"principal_id": "user-1", "role": "admin"}, true, @@ -117,6 +119,7 @@ func TestRunRevokeProvisioning_AllZeroRowsWithDeletedPrincipal(t *testing.T) { t.Context(), revokeDeletesUserQueries, nil, + false, principalExistsCheck(), map[string]any{"principal_id": "user-1", "role": "admin"}, true, @@ -134,6 +137,7 @@ func TestRunRevokeProvisioning_AllZeroRowsWithSurvivingPrincipal(t *testing.T) { t.Context(), revokeDeletesUserQueries, nil, + false, principalExistsCheck(), map[string]any{"principal_id": "user-1", "role": "admin"}, true, @@ -157,6 +161,7 @@ func TestRunRevokeProvisioning_DDLValidationNoRowsSkipsExistsCheck(t *testing.T) t.Context(), []string{`DELETE FROM user_roles WHERE user_id = ? AND role = ?`}, []string{`SELECT 1 FROM user_roles WHERE user_id = ? AND role = ?`}, + true, principalExistsCheck(), map[string]any{"principal_id": "user-1", "role": "admin"}, true, @@ -174,6 +179,7 @@ func TestRunRevokeProvisioning_NoExistsCheckBehavesLikeBefore(t *testing.T) { t.Context(), []string{`DELETE FROM user_roles WHERE user_id = ? AND role = ?`}, nil, + false, nil, map[string]any{"principal_id": "user-1", "role": "admin"}, true, @@ -187,6 +193,7 @@ func TestRunRevokeProvisioning_NoExistsCheckBehavesLikeBefore(t *testing.T) { t.Context(), []string{`DELETE FROM user_roles WHERE user_id = ? AND role = ?`}, nil, + false, nil, map[string]any{"principal_id": "user-1", "role": "admin"}, true, @@ -205,6 +212,7 @@ func TestRunRevokeProvisioning_ProbeErrorKeepsRevoke(t *testing.T) { t.Context(), []string{`DELETE FROM user_roles WHERE user_id = ? AND role = ?`}, nil, + false, &PrincipalExistsCheck{Query: `SELECT 1 FROM nonexistent_table WHERE id = ?`}, map[string]any{"principal_id": "user-1", "role": "admin"}, true, diff --git a/pkg/bsql/provisioning_validation_idempotency_gate_test.go b/pkg/bsql/provisioning_validation_idempotency_gate_test.go index af68b7af..7ec3e96a 100644 --- a/pkg/bsql/provisioning_validation_idempotency_gate_test.go +++ b/pkg/bsql/provisioning_validation_idempotency_gate_test.go @@ -7,17 +7,15 @@ import ( "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. +// validationNoRowsMeansIdempotent gates validation "no rows" onto idempotency (not a failed +// precondition) only on DDL engines whose already-applied GRANT/REVOKE raises an error instead +// of affecting rows (Db2, Oracle), AND only when the entitlement opts in. With the opt-in off, +// every engine fails loudly; with it on, only the two DDL engines reinterpret no-rows. This +// guards against re-enabling any engine by accident or dropping the opt-in requirement. func TestValidationNoRowsMeansIdempotent_EngineGate(t *testing.T) { - ddl := map[database.DbEngine]bool{ + ddlEngines := map[database.DbEngine]bool{ database.DB2: true, - database.Oracle: false, + database.Oracle: true, database.SQLite: false, database.MySQL: false, database.PostgreSQL: false, @@ -25,8 +23,9 @@ func TestValidationNoRowsMeansIdempotent_EngineGate(t *testing.T) { database.HDB: false, database.Vertica: false, } - for engine, want := range ddl { + for engine, ddlOptIn := range ddlEngines { s := &SQLSyncer{dbEngine: engine} - require.Equal(t, want, s.validationNoRowsMeansIdempotent(), "engine=%v", engine) + require.False(t, s.validationNoRowsMeansIdempotent(false), "opt-in off must never signal idempotency, engine=%v", engine) + require.Equal(t, ddlOptIn, s.validationNoRowsMeansIdempotent(true), "opt-in on, engine=%v", engine) } } diff --git a/pkg/bsql/provisioning_validation_idempotency_test.go b/pkg/bsql/provisioning_validation_idempotency_test.go index 33cee689..55bf29f6 100644 --- a/pkg/bsql/provisioning_validation_idempotency_test.go +++ b/pkg/bsql/provisioning_validation_idempotency_test.go @@ -17,6 +17,10 @@ const grantValidationQuery = `SELECT 1 FROM users u WHERE u.id = ? const revokeValidationQuery = `SELECT 1 FROM user_roles WHERE user_id = ? AND role = ?` func withValidationQueryConfig(s *SQLSyncer) { + withValidationQueryConfigSignal(s, true) +} + +func withValidationQueryConfigSignal(s *SQLSyncer, signalIdempotency bool) { s.config = ResourceType{ StaticEntitlements: []*EntitlementMapping{ { @@ -28,14 +32,16 @@ func withValidationQueryConfig(s *SQLSyncer) { }, Grant: &GrantEntitlementProvisioningQueries{ EntitlementProvisioningQueries: EntitlementProvisioningQueries{ - ValidationQueries: []string{grantValidationQuery}, - Queries: []string{`INSERT INTO user_roles (user_id, role) VALUES (?, ?)`}, + ValidationQueries: []string{grantValidationQuery}, + ValidationQueriesSignalIdempotency: signalIdempotency, + Queries: []string{`INSERT INTO user_roles (user_id, role) VALUES (?, ?)`}, }, }, Revoke: &RevokeEntitlementProvisioningQueries{ EntitlementProvisioningQueries: EntitlementProvisioningQueries{ - ValidationQueries: []string{revokeValidationQuery}, - Queries: []string{`DELETE FROM user_roles WHERE user_id = ? AND role = ?`}, + ValidationQueries: []string{revokeValidationQuery}, + ValidationQueriesSignalIdempotency: signalIdempotency, + Queries: []string{`DELETE FROM user_roles WHERE user_id = ? AND role = ?`}, }, }, }, @@ -159,6 +165,7 @@ func TestRunProvisioningQueriesWithExecutor_ValidationNoRowsWrapsSentinel(t *tes []string{`DELETE FROM user_roles WHERE user_id = ?`}, []string{revokeValidationQuery}, "revoke provisioning", + true, map[string]any{"principal_id": "user-1", "role": "admin"}, db, ) @@ -166,3 +173,29 @@ func TestRunProvisioningQueriesWithExecutor_ValidationNoRowsWrapsSentinel(t *tes // guard the operation prefix so a rebase can't silently drop it (it's the only per-call diagnostic once swallowed into an annotation) require.Contains(t, err.Error(), "revoke provisioning") } + +// With the opt-in off, a no-rows validation result must fail loudly even on a DDL engine: +// the default keeps validation_queries as a hard existence precondition. +func TestGrant_ValidationNoRowsWithoutOptInFailsLoudly(t *testing.T) { + s, db := newRevokeProvisioningTestSyncer(t) + withValidationQueryConfigSignal(s, false) + s.dbEngine = database.DB2 + // membership already present: the grant validation query returns no rows + seedUserWithRoles(t, db, "user-1", "admin") + + annos, err := s.Grant(t.Context(), userPrincipal("user-1"), memberEntitlementFor("admin")) + require.Error(t, err) + require.Nil(t, annos) + require.Equal(t, 1, countRows(t, db, `SELECT COUNT(*) FROM user_roles WHERE user_id = ? AND role = ?`, "user-1", "admin")) +} + +func TestRevoke_ValidationNoRowsWithoutOptInFailsLoudly(t *testing.T) { + s, _ := newRevokeProvisioningTestSyncer(t) + withValidationQueryConfigSignal(s, false) + s.dbEngine = database.DB2 + // nothing seeded: the revoke validation query returns no rows + + annos, err := s.Revoke(t.Context(), revokeGrantFor("user-1", "admin")) + require.Error(t, err) + require.Nil(t, annos) +} diff --git a/pkg/bsql/query.go b/pkg/bsql/query.go index 219bb379..5864a5c0 100644 --- a/pkg/bsql/query.go +++ b/pkg/bsql/query.go @@ -391,6 +391,7 @@ func (s *SQLSyncer) RunProvisioningQueries( ctx context.Context, queries, validationQueries []string, + signalIdempotency bool, vars map[string]any, useTx bool, ) error { @@ -429,6 +430,7 @@ func (s *SQLSyncer) RunProvisioningQueries( queries, validationQueries, "provisioning", + signalIdempotency, vars, executor, ) @@ -471,6 +473,7 @@ func (s *SQLSyncer) RunRevokeProvisioning( ctx context.Context, queries, validationQueries []string, + signalIdempotency bool, existsCheck *PrincipalExistsCheck, vars map[string]any, useTx bool, @@ -486,7 +489,7 @@ func (s *SQLSyncer) RunRevokeProvisioning( return false, err } - allZero, fromValidation, err := s.runRevokeQueries(ctx, queries, validationQueries, vars, useTx, target) + allZero, fromValidation, err := s.runRevokeQueries(ctx, queries, validationQueries, signalIdempotency, vars, useTx, target) if err != nil { return false, err } @@ -527,6 +530,7 @@ func (s *SQLSyncer) runRevokeQueries( ctx context.Context, queries, validationQueries []string, + signalIdempotency bool, vars map[string]any, useTx bool, target *sql.DB, @@ -553,7 +557,7 @@ func (s *SQLSyncer) runRevokeQueries( } var allZero, fromValidation bool - err := s.RunProvisioningQueriesWithExecutor(ctx, queries, validationQueries, "revoke provisioning", vars, executor) + err := s.RunProvisioningQueriesWithExecutor(ctx, queries, validationQueries, "revoke provisioning", signalIdempotency, vars, executor) if err != nil { if !errors.Is(err, ErrQueryAffectedZeroRows) { return false, false, err @@ -618,20 +622,29 @@ func (s *SQLSyncer) runPrincipalExistsCheck( return exists, nil } -// validationNoRowsMeansIdempotent reports whether a validation query returning no rows -// means "already in the desired state" rather than a failed precondition. Only Db2 needs -// it today: its DDL GRANT/REVOKE don't report rows-affected, so the validation query is the -// only zero-effect signal, and Db2 ships opt-in behind the db2 build tag. Oracle and other -// DDL engines are a follow-up: they ship default-on, so flipping this would break existing -// configs that use validation_queries as loud preconditions, and need a per-config opt-in first. -func (s *SQLSyncer) validationNoRowsMeansIdempotent() bool { - return s.dbEngine == database.DB2 +// validationNoRowsMeansIdempotent reports whether a validation query returning no rows means +// "already in the desired state" (an idempotent success) rather than a failed precondition. It +// is true only on DDL engines whose GRANT/REVOKE raise an error instead of reporting +// rows-affected (Db2, Oracle) AND only when the entitlement opts in via +// validation_queries_signal_idempotency. Default off, so a no-rows result keeps failing loudly: +// swallowing it anywhere it could mean a missing or mistyped principal is a silent-access bug. +func (s *SQLSyncer) validationNoRowsMeansIdempotent(signalIdempotency bool) bool { + if !signalIdempotency { + return false + } + switch s.dbEngine { + case database.DB2, database.Oracle: + return true + default: + return false + } } func (s *SQLSyncer) runValidationQueries( ctx context.Context, validationQueries []string, operation string, + signalIdempotency bool, vars map[string]any, executor executor, ) error { @@ -667,7 +680,7 @@ func (s *SQLSyncer) runValidationQueries( } if !valid { - if s.validationNoRowsMeansIdempotent() { + if s.validationNoRowsMeansIdempotent(signalIdempotency) { l.Debug("validation query returned no rows; treating as idempotent success", zap.String("query", q), zap.String("operation", operation)) return fmt.Errorf("%s: validation query %q returned no rows: %w", operation, q, ErrValidationNoRows) } @@ -683,12 +696,13 @@ func (s *SQLSyncer) RunProvisioningQueriesWithExecutor( queries, validationQueries []string, operation string, + signalIdempotency bool, vars map[string]any, executor executor, ) error { l := ctxzap.Extract(ctx) - if err := s.runValidationQueries(ctx, validationQueries, operation, vars, executor); err != nil { + if err := s.runValidationQueries(ctx, validationQueries, operation, signalIdempotency, vars, executor); err != nil { return err } @@ -933,6 +947,7 @@ func (s *SQLSyncer) RunGrantProvisioning( resource *v2.Resource, queries, validationQueries []string, + signalIdempotency bool, vars map[string]any, useTx bool, replace *GrantReplaceProvisioningQueries, @@ -1078,14 +1093,16 @@ func (s *SQLSyncer) RunGrantProvisioning( provisioningConfig.Revoke.Queries, provisioningConfig.Revoke.ValidationQueries, "grant_replace revoke", + provisioningConfig.Revoke.ValidationQueriesSignalIdempotency, provisioningVars, executor, ) if err != nil { // A zero-rows sentinel means the replace revoke had nothing to remove: the revoke - // queries matched nothing, or (Db2 only) a validation query returned no rows. Reporting - // the latter as GrantReplaced is load-bearing on the Db2 validation_queries contract that - // no-rows means "already gone"; don't generalize it to existence-precondition queries. + // queries matched nothing, or a validation query returned no rows on a DDL engine + // that opted into validation_queries_signal_idempotency. Reporting the latter as + // GrantReplaced leans on that opt-in meaning "already gone"; the gate keeps it off + // for existence-precondition queries. if !errors.Is(err, ErrQueryAffectedZeroRows) { return anno, err } @@ -1099,7 +1116,7 @@ func (s *SQLSyncer) RunGrantProvisioning( } } - if err := s.runValidationQueries(ctx, validationQueries, "grant provisioning", vars, executor); err != nil { + if err := s.runValidationQueries(ctx, validationQueries, "grant provisioning", signalIdempotency, vars, executor); err != nil { return anno, err } diff --git a/pkg/bsql/user_syncer.go b/pkg/bsql/user_syncer.go index 114ebf0e..07202094 100644 --- a/pkg/bsql/user_syncer.go +++ b/pkg/bsql/user_syncer.go @@ -166,7 +166,7 @@ func (s *userSyncer) CreateAccount( // Execute account creation queries useTransaction := !provisioningConfig.Create.NoTransaction - if err := s.RunProvisioningQueries(ctx, provisioningConfig.Create.Queries, nil, queryInputs, useTransaction); err != nil { + if err := s.RunProvisioningQueries(ctx, provisioningConfig.Create.Queries, nil, false, queryInputs, useTransaction); err != nil { return nil, nil, nil, err } @@ -224,7 +224,7 @@ func (s *userSyncer) Rotate(ctx context.Context, resourceId *v2.ResourceId, cred // Execute account creation queries useTransaction := !rotationConfig.Update.NoTransaction - if err := s.RunProvisioningQueries(ctx, rotationConfig.Update.Queries, nil, queryInputs, useTransaction); err != nil { + if err := s.RunProvisioningQueries(ctx, rotationConfig.Update.Queries, nil, false, queryInputs, useTransaction); err != nil { return nil, nil, err } diff --git a/pkg/connector/action.go b/pkg/connector/action.go index 79d2ebed..09176786 100644 --- a/pkg/connector/action.go +++ b/pkg/connector/action.go @@ -211,7 +211,7 @@ func (c *Connector) handleQueryAction(ctx context.Context, actionKey string, act } else { queries = []string{actionCfg.Query} } - err = sqlSyncer.RunProvisioningQueries(ctx, queries, nil, argMap, !actionCfg.NoTransaction) + err = sqlSyncer.RunProvisioningQueries(ctx, queries, nil, false, argMap, !actionCfg.NoTransaction) if err != nil { return nil, nil, err } From 0b6f229e48aed0e28f7cbf5bd713ec3b4c9c3ec2 Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Thu, 10 Sep 2026 06:28:33 -0500 Subject: [PATCH 2/5] CXH-2417: add Oracle full-path idempotency tests; note admin non-idempotency in example Closes the review gap that Oracle was only exercised at the engine-gate unit level: add end-to-end Grant()/Revoke() tests under dbEngine=Oracle + opt-in that assert GrantAlreadyExists/GrantAlreadyRevoked. The SQLite harness can't bind Oracle's ":N" placeholders, so the validation query is bind-free; the main INSERT/DELETE is skipped on the idempotent no-rows path anyway. Also document in examples/oracle-test.yml that the admin (WITH ADMIN OPTION) entitlements omit validation_queries idempotency on purpose. --- examples/oracle-test.yml | 4 ++ ...rovisioning_validation_idempotency_test.go | 68 +++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/examples/oracle-test.yml b/examples/oracle-test.yml index 3f60b46a..de09d8db 100644 --- a/examples/oracle-test.yml +++ b/examples/oracle-test.yml @@ -338,6 +338,8 @@ resource_types: grantable_to: - "user" # Provisioning details for granting and revoking admin privileges + # Intentionally no validation_queries_signal_idempotency here: WITH ADMIN OPTION is left + # non-idempotent, so a repeat revoke still raises ORA-01951. Omitted on purpose, not missed. provisioning: vars: principal_name: principal.ID # Maps to the user receiving the admin rights @@ -441,6 +443,8 @@ resource_types: purpose: "permission" # Indicates this entitlement is a permission setting grantable_to: - "user" # This permission can be granted to user resources + # Intentionally no validation_queries_signal_idempotency here: WITH ADMIN OPTION is left + # non-idempotent, so a repeat grant/revoke still errors. Omitted on purpose, not missed. provisioning: vars: principal_name: principal.ID # User identifier for provisioning diff --git a/pkg/bsql/provisioning_validation_idempotency_test.go b/pkg/bsql/provisioning_validation_idempotency_test.go index 55bf29f6..38c166bd 100644 --- a/pkg/bsql/provisioning_validation_idempotency_test.go +++ b/pkg/bsql/provisioning_validation_idempotency_test.go @@ -16,6 +16,41 @@ const grantValidationQuery = `SELECT 1 FROM users u WHERE u.id = ? // revokeValidationQuery returns a row only while the membership is present. const revokeValidationQuery = `SELECT 1 FROM user_roles WHERE user_id = ? AND role = ?` +// withBindFreeValidationConfig mirrors withValidationQueryConfig but uses a validation query +// with no ?<...> tokens. Oracle renders binds as ":N", which the SQLite harness reads as named +// args it can't satisfy; a bind-free query that always returns no rows drives the idempotent +// path on Oracle without executing engine-specific bind SQL. +func withBindFreeValidationConfig(s *SQLSyncer) { + const alwaysNoRows = `SELECT 1 WHERE 1 = 0` + s.config = ResourceType{ + StaticEntitlements: []*EntitlementMapping{ + { + Id: "member", + Provisioning: &EntitlementProvisioning{ + Vars: map[string]string{ + "principal_id": "principal.ID", + "role": "resource.ID", + }, + Grant: &GrantEntitlementProvisioningQueries{ + EntitlementProvisioningQueries: EntitlementProvisioningQueries{ + ValidationQueries: []string{alwaysNoRows}, + ValidationQueriesSignalIdempotency: true, + Queries: []string{`INSERT INTO user_roles (user_id, role) VALUES (?, ?)`}, + }, + }, + Revoke: &RevokeEntitlementProvisioningQueries{ + EntitlementProvisioningQueries: EntitlementProvisioningQueries{ + ValidationQueries: []string{alwaysNoRows}, + ValidationQueriesSignalIdempotency: true, + Queries: []string{`DELETE FROM user_roles WHERE user_id = ? AND role = ?`}, + }, + }, + }, + }, + }, + } +} + func withValidationQueryConfig(s *SQLSyncer) { withValidationQueryConfigSignal(s, true) } @@ -79,6 +114,23 @@ func TestGrant_ValidationNoRowsReportsAlreadyExists(t *testing.T) { require.Equal(t, 1, countRows(t, db, `SELECT COUNT(*) FROM user_roles WHERE user_id = ? AND role = ?`, "user-1", "admin")) } +// Oracle full-path coverage. The SQLite test harness can't bind Oracle's ":N" placeholders, +// and Db2's "?" placeholders are what let its full-path test run here. On the idempotent +// no-rows path the INSERT is skipped, so a bind-free validation query lets the real Grant() +// run under dbEngine=Oracle and prove GrantAlreadyExists comes out. +func TestGrant_ValidationNoRowsReportsAlreadyExists_Oracle(t *testing.T) { + s, _ := newRevokeProvisioningTestSyncer(t) + withBindFreeValidationConfig(s) + s.dbEngine = database.Oracle + + annos, err := s.Grant(t.Context(), userPrincipal("user-1"), memberEntitlementFor("admin")) + require.NoError(t, err) + + ok, err := annos.Pick(&v2.GrantAlreadyExists{}) + require.NoError(t, err) + require.True(t, ok) +} + // On a non-DDL engine, validation "no rows" is a failed precondition, not idempotency: // Grant must return an error rather than reporting GrantAlreadyExists. func TestGrant_ValidationNoRowsOnNonDDLEngineFailsLoudly(t *testing.T) { @@ -126,6 +178,22 @@ func TestRevoke_ValidationNoRowsReportsAlreadyRevoked(t *testing.T) { require.True(t, ok) } +// Oracle full-path coverage; see TestGrant_ValidationNoRowsReportsAlreadyExists_Oracle for why +// the validation query is bind-free. On the no-rows path the DELETE is skipped, so the real +// Revoke() runs under dbEngine=Oracle and proves GrantAlreadyRevoked comes out. +func TestRevoke_ValidationNoRowsReportsAlreadyRevoked_Oracle(t *testing.T) { + s, _ := newRevokeProvisioningTestSyncer(t) + withBindFreeValidationConfig(s) + s.dbEngine = database.Oracle + + annos, err := s.Revoke(t.Context(), revokeGrantFor("user-1", "admin")) + require.NoError(t, err) + + ok, err := annos.Pick(&v2.GrantAlreadyRevoked{}) + require.NoError(t, err) + require.True(t, ok) +} + // On a non-DDL engine, validation "no rows" is a failed precondition, not idempotency: // Revoke must return an error rather than reporting GrantAlreadyRevoked. func TestRevoke_ValidationNoRowsOnNonDDLEngineFailsLoudly(t *testing.T) { From 24d82c728cf47c95416b9fca5bba57970a207d33 Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Thu, 10 Sep 2026 14:30:31 -0500 Subject: [PATCH 3/5] CXH-2417: address #152 review round (Db2 default-on, Oracle opt-in gate, |keyword renderer, opt-in validation) - validationNoRowsMeansIdempotent: keep Db2 on by engine (unchanged from #151, build-tag gated), require the validation_queries_signal_idempotency opt-in only for Oracle, which ships in every binary. Retarget the opt-in-off tests to Oracle and add Db2 default-on tests; make the engine-gate matrix explicit. - Add a |keyword token renderer for multiword system privileges (e.g. CREATE SESSION): charset-allowlisted (letters/digits/single spaces), whitespace collapsed, injection values rejected. |unquoted stripped the space, |identifier would wrongly quote the clause. - validate.go: run validation_queries through validateVarsInQuery, and when the opt-in is set require at least one validation query plus no_transaction on DDL engines. Warn when the opt-in is set on a non-DDL engine. - examples/oracle-test.yml + docs/oracle.md: quote role/principal operands with |identifier and drop UPPER() so quoted case-sensitive identifiers match; render system privileges with |keyword. - Add an Oracle grant_replace opt-in-off regression test. --- docs/oracle.md | 20 ++++- examples/oracle-test.yml | 42 +++++---- pkg/bsql/provisioning_grant_replace_test.go | 34 +++++++ ...ioning_validation_idempotency_gate_test.go | 36 ++++---- ...rovisioning_validation_idempotency_test.go | 60 +++++++++++-- pkg/bsql/query.go | 89 +++++++++++++++++-- pkg/bsql/query_test.go | 64 +++++++++++++ pkg/bsql/validate.go | 31 +++++++ pkg/bsql/validate_test.go | 63 +++++++++++++ 9 files changed, 383 insertions(+), 56 deletions(-) diff --git a/docs/oracle.md b/docs/oracle.md index 4ae44add..92c691c5 100644 --- a/docs/oracle.md +++ b/docs/oracle.md @@ -31,14 +31,16 @@ grant: no_transaction: true validation_queries_signal_idempotency: true # returns a row only while the role is NOT yet granted (no rows => already granted) + # exact-match (no UPPER): the GRANT quotes identifiers, so the stored GRANTEE/GRANTED_ROLE + # are case-sensitive and must equal the bound value validation_queries: - | SELECT 1 FROM dual WHERE NOT EXISTS ( SELECT 1 FROM DBA_ROLE_PRIVS - WHERE GRANTEE = UPPER(?) AND GRANTED_ROLE = UPPER(?) + WHERE GRANTEE = ? AND GRANTED_ROLE = ? ) queries: - - GRANT ? TO ? + - GRANT ? TO ? revoke: no_transaction: true validation_queries_signal_idempotency: true @@ -46,9 +48,19 @@ revoke: validation_queries: - | SELECT 1 FROM DBA_ROLE_PRIVS - WHERE GRANTEE = UPPER(?) AND GRANTED_ROLE = UPPER(?) + WHERE GRANTEE = ? AND GRANTED_ROLE = ? queries: - - REVOKE ? FROM ? + - REVOKE ? FROM ? +``` + +Use `|identifier` for role and principal names so they are engine-quoted (safe against +injection and case-sensitive). System-privilege entitlements are different: privilege names are +multiword keywords like `CREATE SESSION`, so their DDL operand must use `|keyword`, not +`|identifier` (quoting would break the clause and `|unquoted` would strip the space): + +```yaml +queries: + - GRANT ? TO ? ``` The flag is off by default, so without it Oracle keeps failing loudly on the repeat operation. diff --git a/examples/oracle-test.yml b/examples/oracle-test.yml index de09d8db..b8526680 100644 --- a/examples/oracle-test.yml +++ b/examples/oracle-test.yml @@ -305,28 +305,31 @@ resource_types: # result as an idempotent success so re-granting a held role reports GrantAlreadyExists. validation_queries_signal_idempotency: true # Returns a row only while the role is NOT yet granted (no rows => already granted). + # Exact-match (no UPPER) is required: the GRANT quotes identifiers with |identifier, so + # the stored GRANTEE/GRANTED_ROLE are case-sensitive and must equal the bound value. validation_queries: - | SELECT 1 FROM dual WHERE NOT EXISTS ( SELECT 1 FROM DBA_ROLE_PRIVS - WHERE GRANTEE = UPPER(?) AND GRANTED_ROLE = UPPER(?) + WHERE GRANTEE = ? AND GRANTED_ROLE = ? ) queries: - | - GRANT ? TO ? + GRANT ? TO ? revoke: # Indicates that the revoke operation runs without a transaction no_transaction: true # Without this, a repeat revoke of an already-removed role fails with ORA-01951. validation_queries_signal_idempotency: true # Returns a row only while the role IS still granted (no rows => already revoked). + # Exact-match (no UPPER) is required for the same reason as the grant validation. validation_queries: - | SELECT 1 FROM DBA_ROLE_PRIVS - WHERE GRANTEE = UPPER(?) AND GRANTED_ROLE = UPPER(?) + WHERE GRANTEE = ? AND GRANTED_ROLE = ? queries: - | - REVOKE ? FROM ? + REVOKE ? FROM ? - id: "admin" # Entitlement identifier for role administration privileges # Dynamic display name for admin entitlement, appending ' Role Admin' display_name: "resource.DisplayName + ' Role Admin'" @@ -348,14 +351,14 @@ resource_types: no_transaction: true queries: - | - GRANT ? TO ? WITH ADMIN OPTION + GRANT ? TO ? WITH ADMIN OPTION revoke: no_transaction: true queries: - | - REVOKE ? FROM ? + REVOKE ? FROM ? - | - GRANT ? TO ? + GRANT ? TO ? # Dynamic grants based on SQL queries to associate users with roles grants: - query: | @@ -420,8 +423,11 @@ resource_types: WHERE GRANTEE = UPPER(?) AND PRIVILEGE = UPPER(?) ) queries: + # System-privilege names are multiword keywords (e.g. CREATE SESSION), so they must + # use |keyword, not |identifier: quoting would break the clause and |unquoted would + # strip the space. The principal stays |unquoted to match the UPPER-based validation. - | - GRANT ? TO ? + GRANT ? TO ? # Revoke section defines how to remove a privilege from a user revoke: # no_transaction indicates this should execute outside a transaction block @@ -436,7 +442,7 @@ resource_types: # SQL queries to execute when revoking the privilege queries: - | - REVOKE ? FROM ? + REVOKE ? FROM ? - id: "admin" # Entitlement identifier for administrative control over privileges display_name: "resource.DisplayName + ' Privilege Admin'" # Dynamic display name for admin privileges on the resource description: "'Can grant the ' + resource.DisplayName + ' privilege to other users'" # Describes the ability to manage privileges @@ -453,21 +459,21 @@ resource_types: no_transaction: true queries: - | - # The ? placeholder will be replaced with the raw value of the privilege_name variable - # defined in the vars section above. The |unquoted flag means the value will be inserted directly into the SQL - # rather than using a prepared statement parameter. This is needed for DDL statements like GRANT that require - # the actual identifier names. + # System-privilege names are multiword keywords (e.g. CREATE SESSION), so the + # ? placeholder inlines the clause as-is (collapsing internal + # whitespace) instead of quoting it. Use |keyword here, not |identifier: quoting + # would break the clause and |unquoted would strip the space. # - # Similarly, ? inserts the principal_name variable's value directly into the query. - # Without |unquoted, the values would be passed as bind parameters like: GRANT ? TO ? WITH ADMIN OPTION - GRANT ? TO ? WITH ADMIN OPTION + # ? inserts the principal directly; DDL like GRANT cannot + # bind these as prepared-statement parameters. + GRANT ? TO ? WITH ADMIN OPTION revoke: no_transaction: true queries: - | - REVOKE ? FROM ? + REVOKE ? FROM ? - | - GRANT ? TO ? + GRANT ? TO ? # Dynamic grants to map privilege assignments based on database queries grants: - query: | diff --git a/pkg/bsql/provisioning_grant_replace_test.go b/pkg/bsql/provisioning_grant_replace_test.go index edca262d..5d423bfd 100644 --- a/pkg/bsql/provisioning_grant_replace_test.go +++ b/pkg/bsql/provisioning_grant_replace_test.go @@ -161,3 +161,37 @@ func TestGrant_ReplaceDB2RevokeValidationNoRowsStillReportsGrantReplaced(t *test // 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")) } + +// withGrantReplaceOracleOptInOffConfig is the grant_replace config on Oracle with the revoke +// opt-in OFF. Bind-free queries keep the SQLite harness happy under Oracle's ":N" placeholders; +// the revoke validation returns no rows, and with the opt-in off that is a hard failure rather +// than an idempotent success. +func withGrantReplaceOracleOptInOffConfig(s *SQLSyncer) { + withGrantReplaceConfig(s, true) // no_transaction + prov := s.config.StaticEntitlements[0].Provisioning + // bind-free replace query so Oracle's ":N" binds never reach SQLite + prov.Grant.GrantReplace.Query = `SELECT user_id, role FROM user_roles WHERE role = 'viewer'` + revoke := prov.Revoke + revoke.ValidationQueries = []string{`SELECT 1 WHERE 1 = 0`} + revoke.ValidationQueriesSignalIdempotency = false +} + +// Oracle path with the grant_replace revoke opt-in OFF: a no-rows revoke validation is a hard +// failure, not idempotency, so the whole grant errors and GrantReplaced is never reported. The +// viewer row survives because the revoke DELETE never ran. +func TestGrant_ReplaceOracleRevokeValidationNoRowsOptInOffFailsGrant(t *testing.T) { + s, db := newGrantReplaceTestSyncer(t) + s.dbEngine = database.Oracle + withGrantReplaceOracleOptInOffConfig(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.Error(t, err) + require.Nil(t, annos, "the whole grant must fail; no GrantReplaced annotation") + + // the revoke never 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 never ran either + require.Equal(t, 0, countRows(t, db, `SELECT COUNT(*) FROM user_roles WHERE user_id = ? AND role = ?`, "user-1", "admin")) +} diff --git a/pkg/bsql/provisioning_validation_idempotency_gate_test.go b/pkg/bsql/provisioning_validation_idempotency_gate_test.go index 7ec3e96a..bfcc973a 100644 --- a/pkg/bsql/provisioning_validation_idempotency_gate_test.go +++ b/pkg/bsql/provisioning_validation_idempotency_gate_test.go @@ -8,24 +8,28 @@ import ( ) // validationNoRowsMeansIdempotent gates validation "no rows" onto idempotency (not a failed -// precondition) only on DDL engines whose already-applied GRANT/REVOKE raises an error instead -// of affecting rows (Db2, Oracle), AND only when the entitlement opts in. With the opt-in off, -// every engine fails loudly; with it on, only the two DDL engines reinterpret no-rows. This -// guards against re-enabling any engine by accident or dropping the opt-in requirement. +// precondition) only on the two DDL engines whose already-applied GRANT/REVOKE raises an error +// instead of affecting rows. Db2 ships behind a build tag, so it stays on by engine regardless of +// the opt-in; Oracle ships in every binary, so it reinterprets no-rows only when the entitlement +// opts in. Every non-DDL engine fails loudly in both cases. This guards against re-enabling any +// engine by accident, flipping Db2's default-on behavior, or dropping Oracle's opt-in requirement. func TestValidationNoRowsMeansIdempotent_EngineGate(t *testing.T) { - ddlEngines := 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, + cases := map[database.DbEngine]struct { + optInOff bool + optInOn bool + }{ + database.DB2: {optInOff: true, optInOn: true}, + database.Oracle: {optInOff: false, optInOn: true}, + database.SQLite: {optInOff: false, optInOn: false}, + database.MySQL: {optInOff: false, optInOn: false}, + database.PostgreSQL: {optInOff: false, optInOn: false}, + database.MSSQL: {optInOff: false, optInOn: false}, + database.HDB: {optInOff: false, optInOn: false}, + database.Vertica: {optInOff: false, optInOn: false}, } - for engine, ddlOptIn := range ddlEngines { + for engine, want := range cases { s := &SQLSyncer{dbEngine: engine} - require.False(t, s.validationNoRowsMeansIdempotent(false), "opt-in off must never signal idempotency, engine=%v", engine) - require.Equal(t, ddlOptIn, s.validationNoRowsMeansIdempotent(true), "opt-in on, engine=%v", engine) + require.Equal(t, want.optInOff, s.validationNoRowsMeansIdempotent(false), "opt-in off, engine=%v", engine) + require.Equal(t, want.optInOn, s.validationNoRowsMeansIdempotent(true), "opt-in on, engine=%v", engine) } } diff --git a/pkg/bsql/provisioning_validation_idempotency_test.go b/pkg/bsql/provisioning_validation_idempotency_test.go index 38c166bd..16bd9a52 100644 --- a/pkg/bsql/provisioning_validation_idempotency_test.go +++ b/pkg/bsql/provisioning_validation_idempotency_test.go @@ -21,6 +21,10 @@ const revokeValidationQuery = `SELECT 1 FROM user_roles WHERE user_id = ?, ?)`}, }, }, Revoke: &RevokeEntitlementProvisioningQueries{ EntitlementProvisioningQueries: EntitlementProvisioningQueries{ ValidationQueries: []string{alwaysNoRows}, - ValidationQueriesSignalIdempotency: true, + ValidationQueriesSignalIdempotency: signalIdempotency, Queries: []string{`DELETE FROM user_roles WHERE user_id = ? AND role = ?`}, }, }, @@ -242,9 +246,37 @@ func TestRunProvisioningQueriesWithExecutor_ValidationNoRowsWrapsSentinel(t *tes require.Contains(t, err.Error(), "revoke provisioning") } -// With the opt-in off, a no-rows validation result must fail loudly even on a DDL engine: -// the default keeps validation_queries as a hard existence precondition. +// On Oracle the opt-in is required: with it off, a no-rows validation must fail loudly rather +// than reporting GrantAlreadyExists (Oracle ships in every binary, so the reinterpretation is +// opt-in only). Bind-free validation queries keep the SQLite harness happy under Oracle's ":N" +// placeholders, so this exercises the gate rather than a bind error. func TestGrant_ValidationNoRowsWithoutOptInFailsLoudly(t *testing.T) { + s, db := newRevokeProvisioningTestSyncer(t) + withBindFreeValidationConfigSignal(s, false) + s.dbEngine = database.Oracle + // the grant validation query always returns no rows + + annos, err := s.Grant(t.Context(), userPrincipal("user-1"), memberEntitlementFor("admin")) + require.Error(t, err) + require.Nil(t, annos) + // the INSERT never ran + require.Equal(t, 0, countRows(t, db, `SELECT COUNT(*) FROM user_roles WHERE user_id = ? AND role = ?`, "user-1", "admin")) +} + +func TestRevoke_ValidationNoRowsWithoutOptInFailsLoudly(t *testing.T) { + s, _ := newRevokeProvisioningTestSyncer(t) + withBindFreeValidationConfigSignal(s, false) + s.dbEngine = database.Oracle + // the revoke validation query always returns no rows + + annos, err := s.Revoke(t.Context(), revokeGrantFor("user-1", "admin")) + require.Error(t, err) + require.Nil(t, annos) +} + +// Db2 ships behind a build tag, so no-rows-means-idempotent stays on by engine even with the +// opt-in off: a no-rows grant validation still reports GrantAlreadyExists (unchanged from #151). +func TestGrant_Db2ValidationNoRowsIdempotentByDefault(t *testing.T) { s, db := newRevokeProvisioningTestSyncer(t) withValidationQueryConfigSignal(s, false) s.dbEngine = database.DB2 @@ -252,18 +284,28 @@ func TestGrant_ValidationNoRowsWithoutOptInFailsLoudly(t *testing.T) { seedUserWithRoles(t, db, "user-1", "admin") annos, err := s.Grant(t.Context(), userPrincipal("user-1"), memberEntitlementFor("admin")) - require.Error(t, err) - require.Nil(t, annos) + require.NoError(t, err) + + ok, err := annos.Pick(&v2.GrantAlreadyExists{}) + require.NoError(t, err) + require.True(t, ok) + + // the INSERT never ran, so no duplicate row was created require.Equal(t, 1, countRows(t, db, `SELECT COUNT(*) FROM user_roles WHERE user_id = ? AND role = ?`, "user-1", "admin")) } -func TestRevoke_ValidationNoRowsWithoutOptInFailsLoudly(t *testing.T) { +// Revoke counterpart: on Db2 a no-rows revoke validation still reports GrantAlreadyRevoked with +// the opt-in off. +func TestRevoke_Db2ValidationNoRowsIdempotentByDefault(t *testing.T) { s, _ := newRevokeProvisioningTestSyncer(t) withValidationQueryConfigSignal(s, false) s.dbEngine = database.DB2 // nothing seeded: the revoke validation query returns no rows annos, err := s.Revoke(t.Context(), revokeGrantFor("user-1", "admin")) - require.Error(t, err) - require.Nil(t, annos) + require.NoError(t, err) + + ok, err := annos.Pick(&v2.GrantAlreadyRevoked{}) + require.NoError(t, err) + require.True(t, ok) } diff --git a/pkg/bsql/query.go b/pkg/bsql/query.go index 5864a5c0..26207e8b 100644 --- a/pkg/bsql/query.go +++ b/pkg/bsql/query.go @@ -31,6 +31,7 @@ const ( limitKey = "limit" unquotedKey = "unquoted" identifierKey = "identifier" + keywordKey = "keyword" ) var ErrQueryAffectedZeroRows = errors.New("query affected 0 rows, ending and rolling back") @@ -74,6 +75,29 @@ type queryTokenOpts struct { // Identifier inlines as an engine-quoted SQL identifier (doubled embedded quotes). // Use where parameter binding isn't allowed by the SQL grammar (GRANT, DDL). Identifier bool + + // Keyword inlines a fixed multiword SQL keyword clause (e.g. "CREATE SESSION") as-is, + // after collapsing internal whitespace. For system-privilege GRANT/REVOKE where the value + // is a keyword rather than an identifier: quoting would break it and Unquoted would strip + // the space. Rejects anything outside letters, digits, and single spaces, so it is not an + // injection vector. + Keyword bool +} + +var keywordClauseRegex = regexp.MustCompile(`^[A-Za-z][A-Za-z0-9 ]*$`) + +var whitespaceRunRegex = regexp.MustCompile(`\s+`) + +// renderKeyword validates a fixed multiword SQL keyword clause (e.g. "CREATE SESSION") and +// returns it with internal whitespace collapsed to single spaces. It reports false for anything +// outside letters, digits, and single spaces (must start with a letter), so an injected value +// like "SESSION; DROP TABLE x" is rejected rather than inlined. +func renderKeyword(val string) (string, bool) { + collapsed := strings.TrimSpace(whitespaceRunRegex.ReplaceAllString(val, " ")) + if !keywordClauseRegex.MatchString(collapsed) { + return "", false + } + return collapsed, true } var queryOptRegex = regexp.MustCompile(`\?\<([a-zA-Z0-9_]+)(?:\|([a-zA-Z0-9_]+))?\>`) @@ -126,13 +150,21 @@ func parseToken(token string) (*queryTokenOpts, error) { opts.Unquoted = true case identifierKey: opts.Identifier = true + case keywordKey: + opts.Keyword = true default: return nil, fmt.Errorf("unknown option %s", opt) } } - if opts.Unquoted && opts.Identifier { - return nil, fmt.Errorf("token options unquoted and identifier are mutually exclusive") + set := 0 + for _, on := range []bool{opts.Unquoted, opts.Identifier, opts.Keyword} { + if on { + set++ + } + } + if set > 1 { + return nil, fmt.Errorf("token options unquoted, identifier and keyword are mutually exclusive") } return opts, nil @@ -206,6 +238,16 @@ func (s *SQLSyncer) parseQueryOpts(pCtx *paginationContext, query string, vars m return s.quoteIdentifier(fmt.Sprintf("%v", val)) } + if opts.Keyword { + strVal := fmt.Sprintf("%v", val) + rendered, ok := renderKeyword(strVal) + if !ok { + parseErr = errors.Join(parseErr, fmt.Errorf("token %s: value %q is not a valid SQL keyword clause", token, strVal)) + return token + } + return rendered + } + qArgs = append(qArgs, val) return s.getNextPlaceholder(qArgs) }) @@ -359,6 +401,16 @@ func (s *SQLSyncer) prepareProvisioningQuery(query string, vars map[string]any) return s.quoteIdentifier(fmt.Sprintf("%v", v)) } + if opts.Keyword { + strVal := fmt.Sprintf("%v", v) + rendered, ok := renderKeyword(strVal) + if !ok { + parseErr = errors.Join(parseErr, fmt.Errorf("token %s: value %q is not a valid SQL keyword clause", token, strVal)) + return token + } + return rendered + } + qArgs = append(qArgs, v) return s.getNextPlaceholder(qArgs) }) @@ -622,19 +674,34 @@ func (s *SQLSyncer) runPrincipalExistsCheck( return exists, nil } +// isDDLEngine reports whether the engine applies GRANT/REVOKE as DDL that raises an error on +// re-run instead of reporting rows-affected (Db2, Oracle). +func isDDLEngine(e database.DbEngine) bool { + switch e { + case database.DB2, database.Oracle: + return true + default: + return false + } +} + // validationNoRowsMeansIdempotent reports whether a validation query returning no rows means // "already in the desired state" (an idempotent success) rather than a failed precondition. It // is true only on DDL engines whose GRANT/REVOKE raise an error instead of reporting -// rows-affected (Db2, Oracle) AND only when the entitlement opts in via -// validation_queries_signal_idempotency. Default off, so a no-rows result keeps failing loudly: -// swallowing it anywhere it could mean a missing or mistyped principal is a silent-access bug. +// rows-affected (Db2, Oracle). Db2 ships behind a build tag, so it stays on by engine (unchanged +// from #151); Oracle ships in every binary, so it reinterprets no-rows only when the entitlement +// opts in via validation_queries_signal_idempotency. Everywhere else a no-rows result keeps +// failing loudly: swallowing it where it could mean a missing or mistyped principal is a +// silent-access bug. func (s *SQLSyncer) validationNoRowsMeansIdempotent(signalIdempotency bool) bool { - if !signalIdempotency { - return false - } switch s.dbEngine { - case database.DB2, database.Oracle: + case database.DB2: + // Db2 ships behind a build tag; no-rows-means-idempotent stays on by engine + // (unchanged from #151), no per-config opt-in required. return true + case database.Oracle: + // Oracle ships in every binary, so the reinterpretation is opt-in only. + return signalIdempotency default: return false } @@ -650,6 +717,10 @@ func (s *SQLSyncer) runValidationQueries( ) error { l := ctxzap.Extract(ctx) + if signalIdempotency && !isDDLEngine(s.dbEngine) { + l.Warn("validation_queries_signal_idempotency is set but ignored on this engine; only DDL engines (Db2, Oracle) reinterpret a no-rows validation as an idempotent success") + } + for _, q := range validationQueries { q, qArgs, err := s.prepareProvisioningQuery(q, vars) if err != nil { diff --git a/pkg/bsql/query_test.go b/pkg/bsql/query_test.go index 0e73fffa..a06305b9 100644 --- a/pkg/bsql/query_test.go +++ b/pkg/bsql/query_test.go @@ -104,6 +104,21 @@ func Test_parseToken(t *testing.T) { want: nil, wantErr: true, }, + { + name: "Token with keyword option", + token: "?", + want: &queryTokenOpts{ + Key: "privilege_name", + Keyword: true, + }, + wantErr: false, + }, + { //nolint:gosec // G101 false positive: "identifier" is the modifier name, not a credential. + name: "Keyword and identifier are mutually exclusive", + token: "?", + want: nil, + wantErr: true, + }, } for _, tt := range tests { @@ -502,6 +517,55 @@ func Test_parseQueryOpts(t *testing.T) { false, false, }, + { + "Test keyword renders a multiword privilege intact", + database.Oracle, + args{ + t.Context(), + "GRANT ? TO ?", + nil, + map[string]any{ + "privilege_name": "CREATE SESSION", + "grantee": "alice", + }, + }, + `GRANT CREATE SESSION TO "alice"`, + nil, + false, + false, + }, + { + "Test keyword collapses internal whitespace", + database.Oracle, + args{ + t.Context(), + "GRANT ? TO bob", + nil, + map[string]any{ + "privilege_name": "CREATE SESSION", + }, + }, + "GRANT CREATE SESSION TO bob", + nil, + false, + false, + }, + { + "Test keyword rejects an injection value", + database.Oracle, + args{ + t.Context(), + "GRANT ? TO bob", + nil, + map[string]any{ + "privilege_name": "SESSION; DROP TABLE x", + }, + }, + "", + nil, + false, + true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/bsql/validate.go b/pkg/bsql/validate.go index 86ff5bd7..d658e4e4 100644 --- a/pkg/bsql/validate.go +++ b/pkg/bsql/validate.go @@ -87,6 +87,29 @@ func (l *EntitlementMapping) staticValidate(ctx context.Context, s *SQLSyncer) e return nil } +// validateProvisioningIdempotency validates the validation_queries and the +// validation_queries_signal_idempotency opt-in shared by grant and revoke. Validation queries are +// always vars-checked; the opt-in additionally requires at least one validation query and, on a +// DDL engine, no_transaction (Db2/Oracle GRANT/REVOKE report no rows-affected under a tx). +func validateProvisioningIdempotency(s *SQLSyncer, pq EntitlementProvisioningQueries, vars map[string]string) error { + for _, query := range pq.ValidationQueries { + if err := validateVarsInQuery(s, query, vars); err != nil { + return err + } + } + + if pq.ValidationQueriesSignalIdempotency { + if len(pq.ValidationQueries) < 1 { + return errors.New("validation_queries_signal_idempotency requires at least one validation_query") + } + if isDDLEngine(s.dbEngine) && !pq.NoTransaction { + return errors.New("validation_queries_signal_idempotency requires no_transaction: true on DDL engines (Db2/Oracle)") + } + } + + return nil +} + func validateGrantProvisioningQueries(s *SQLSyncer, grant *GrantEntitlementProvisioningQueries, vars map[string]string) error { if grant == nil { return nil @@ -98,6 +121,10 @@ func validateGrantProvisioningQueries(s *SQLSyncer, grant *GrantEntitlementProvi } } + if err := validateProvisioningIdempotency(s, grant.EntitlementProvisioningQueries, vars); err != nil { + return err + } + for _, query := range grant.Queries { if err := validateVarsInQuery(s, query, vars); err != nil { return err @@ -112,6 +139,10 @@ func validateRevokeProvisioningQueries(s *SQLSyncer, revoke *RevokeEntitlementPr return nil } + if err := validateProvisioningIdempotency(s, revoke.EntitlementProvisioningQueries, vars); err != nil { + return err + } + for _, query := range revoke.Queries { if err := validateVarsInQuery(s, query, vars); err != nil { return err diff --git a/pkg/bsql/validate_test.go b/pkg/bsql/validate_test.go index 5c210e37..10aa9d81 100644 --- a/pkg/bsql/validate_test.go +++ b/pkg/bsql/validate_test.go @@ -3,6 +3,7 @@ package bsql import ( "testing" + "github.com/conductorone/baton-sql/pkg/database" "github.com/stretchr/testify/require" ) @@ -103,3 +104,65 @@ func TestValidate(t *testing.T) { }) } } + +// TestValidateProvisioningIdempotency covers the shared validation_queries_signal_idempotency +// checks: validation queries are always vars-checked, the opt-in requires at least one validation +// query, and on a DDL engine it requires no_transaction. The grant and revoke helpers both run it. +func TestValidateProvisioningIdempotency(t *testing.T) { + vars := map[string]string{"p": "principal.ID"} + oneValidation := []string{"SELECT 1 FROM dual WHERE x = ?

"} + + tests := []struct { + name string + engine database.DbEngine + pq EntitlementProvisioningQueries + wantErr bool + }{ + { + name: "signal on without validation queries fails", + engine: database.Oracle, + pq: EntitlementProvisioningQueries{ValidationQueriesSignalIdempotency: true, NoTransaction: true}, + wantErr: true, + }, + { + name: "signal on DDL engine without no_transaction fails", + engine: database.Oracle, + pq: EntitlementProvisioningQueries{ValidationQueriesSignalIdempotency: true, ValidationQueries: oneValidation}, + wantErr: true, + }, + { + name: "signal on DDL engine with no_transaction and a validation query ok", + engine: database.Oracle, + pq: EntitlementProvisioningQueries{ValidationQueriesSignalIdempotency: true, NoTransaction: true, ValidationQueries: oneValidation}, + wantErr: false, + }, + { + name: "signal on non-DDL engine does not require no_transaction", + engine: database.PostgreSQL, + pq: EntitlementProvisioningQueries{ValidationQueriesSignalIdempotency: true, ValidationQueries: oneValidation}, + wantErr: false, + }, + { + name: "validation query with undefined var fails", + engine: database.Oracle, + pq: EntitlementProvisioningQueries{NoTransaction: true, ValidationQueries: []string{"SELECT 1 WHERE x = ?"}}, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := &SQLSyncer{dbEngine: tt.engine} + + gErr := validateGrantProvisioningQueries(s, &GrantEntitlementProvisioningQueries{EntitlementProvisioningQueries: tt.pq}, vars) + rErr := validateRevokeProvisioningQueries(s, &RevokeEntitlementProvisioningQueries{EntitlementProvisioningQueries: tt.pq}, vars) + if tt.wantErr { + require.Error(t, gErr) + require.Error(t, rErr) + } else { + require.NoError(t, gErr) + require.NoError(t, rErr) + } + }) + } +} From 187057bf91cd574f8fff84c7de8cdc93508961ba Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Fri, 11 Sep 2026 12:41:09 -0500 Subject: [PATCH 4/5] CXH-2417: address review feedback on grant/revoke idempotency - reject validation_queries_signal_idempotency on non-DDL engines at config validation instead of logging an ignored-flag Warn - require no_transaction whenever a no-rows validation is reinterpreted as idempotent, so Db2's default-on path is covered, not just the raw flag - UPPER-normalize the Oracle role-membership principal (|unquoted + UPPER()) so default upper-folded users match - correct docs: on Db2 no-rows idempotency is on by default; the validation_queries_signal_idempotency flag is Oracle-only - replace the positional bools on the exported Run* provisioning functions with a ProvisioningOptions struct --- README.md | 2 +- docs/db2.md | 12 ++--- docs/oracle.md | 23 +++++----- docs/provisioning.md | 31 ++++++------- examples/oracle-test.yml | 20 ++++----- pkg/bsql/config.go | 7 +-- pkg/bsql/provisioning.go | 12 +++-- pkg/bsql/provisioning_grant_reject_test.go | 6 +-- pkg/bsql/provisioning_revoke_deleted_test.go | 24 ++++------ ...rovisioning_validation_idempotency_test.go | 2 +- pkg/bsql/query.go | 45 ++++++++++--------- pkg/bsql/user_syncer.go | 4 +- pkg/bsql/validate.go | 18 +++++--- pkg/bsql/validate_test.go | 16 ++++++- pkg/connector/action.go | 2 +- 15 files changed, 123 insertions(+), 101 deletions(-) diff --git a/README.md b/README.md index d92d607f..4363f8f8 100644 --- a/README.md +++ b/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; see [docs/provisioning.md](docs/provisioning.md) for `validation_queries` semantics (including the opt-in `validation_queries_signal_idempotency` that makes no-rows mean idempotent success on the DDL engines Db2 and Oracle) +- **Provisioning Actions**: SQL queries for granting/revoking entitlements; see [docs/provisioning.md](docs/provisioning.md) for `validation_queries` semantics (a no-rows validation is treated as idempotent success on the DDL engines Db2 and Oracle: on by default for Db2, opt-in via `validation_queries_signal_idempotency` for 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`). diff --git a/docs/db2.md b/docs/db2.md index 8e58e494..b570e14d 100644 --- a/docs/db2.md +++ b/docs/db2.md @@ -223,11 +223,13 @@ 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. When you set -`validation_queries_signal_idempotency: true` on a grant or revoke, a `validation_query` -returning no rows is treated as an idempotent success rather than a failed precondition. The -flag is off by default, so you must opt in per entitlement; when it is on, you must not use -`validation_queries` as existence preconditions. See +Db2 is DDL-based: its `GRANT`/`REVOKE` don't report rows-affected. On Db2 a `validation_query` +returning no rows is treated as an idempotent success rather than a failed precondition **by +default** (Db2 ships behind a build tag, so this is on by engine and does not need +`validation_queries_signal_idempotency`; that flag is only relevant on Oracle, which ships in +every binary). Because no rows means "already in the desired state", you must not use +`validation_queries` as existence preconditions, and every Db2 grant/revoke that has a +`validation_query` must set `no_transaction: true`. See [Provisioning: `validation_queries` semantics](provisioning.md) for the full explanation and examples. diff --git a/docs/oracle.md b/docs/oracle.md index 92c691c5..b10c880c 100644 --- a/docs/oracle.md +++ b/docs/oracle.md @@ -31,16 +31,16 @@ grant: no_transaction: true validation_queries_signal_idempotency: true # returns a row only while the role is NOT yet granted (no rows => already granted) - # exact-match (no UPPER): the GRANT quotes identifiers, so the stored GRANTEE/GRANTED_ROLE - # are case-sensitive and must equal the bound value + # UPPER-normalize the principal: the GRANT inserts it |unquoted, so Oracle folds it to + # upper-case (CREATE USER jdoe => JDOE); DBA_ROLES roles are already upper-case validation_queries: - | SELECT 1 FROM dual WHERE NOT EXISTS ( SELECT 1 FROM DBA_ROLE_PRIVS - WHERE GRANTEE = ? AND GRANTED_ROLE = ? + WHERE GRANTEE = UPPER(?) AND GRANTED_ROLE = ? ) queries: - - GRANT ? TO ? + - GRANT ? TO ? revoke: no_transaction: true validation_queries_signal_idempotency: true @@ -48,15 +48,18 @@ revoke: validation_queries: - | SELECT 1 FROM DBA_ROLE_PRIVS - WHERE GRANTEE = ? AND GRANTED_ROLE = ? + WHERE GRANTEE = UPPER(?) AND GRANTED_ROLE = ? queries: - - REVOKE ? FROM ? + - REVOKE ? FROM ? ``` -Use `|identifier` for role and principal names so they are engine-quoted (safe against -injection and case-sensitive). System-privilege entitlements are different: privilege names are -multiword keywords like `CREATE SESSION`, so their DDL operand must use `|keyword`, not -`|identifier` (quoting would break the clause and `|unquoted` would strip the space): +Use `|identifier` for role names so they are engine-quoted; they come from `DBA_ROLES` +already upper-cased. Insert the principal `|unquoted` so Oracle folds it to upper-case, and +compare it with `UPPER(?)` in the validation query so a lower-case +`principal.ID` still matches the stored `GRANTEE`. System-privilege entitlements are different: +privilege names are multiword keywords like `CREATE SESSION`, so their DDL operand must use +`|keyword`, not `|identifier` (quoting would break the clause and `|unquoted` would strip the +space): ```yaml queries: diff --git a/docs/provisioning.md b/docs/provisioning.md index 1d5e33c7..e63980e5 100644 --- a/docs/provisioning.md +++ b/docs/provisioning.md @@ -1,20 +1,24 @@ # Provisioning: `validation_queries` semantics `validation_queries` run before the provisioning `queries` in a grant or revoke. What a -**no-rows** result means depends on one per-entitlement flag. +**no-rows** result means depends on the engine: on the DDL engines (Db2, Oracle) it can mean an +idempotent success (on by default for Db2, opt-in for Oracle); on every other engine it fails the +operation. ## Default: no rows fails the operation By default a `validation_query` returning no rows **fails the operation**. It is an existence -precondition that aborts loudly. This is the behavior on every engine unless you opt in below. +precondition that aborts loudly. This is the behavior on every engine except where the DDL +no-rows-means-idempotent behavior below is active. -## Opt-in: no rows means idempotent success +## DDL engines: no rows means idempotent success -Some engines apply `GRANT`/`REVOKE` as DDL that does not report rows-affected (Db2, Oracle), so +The DDL engines (Db2, Oracle) apply `GRANT`/`REVOKE` as DDL that does not report rows-affected, so the connector cannot tell from the statement whether it changed anything, and re-running an -already-applied statement raises an error (Oracle `ORA-01951` on a repeat revoke, for example). -To make grant and revoke idempotent on these engines, set `validation_queries_signal_idempotency: -true` on the grant or revoke: +already-applied statement raises an error (Oracle `ORA-01951` on a repeat revoke, for example). On +these engines a `validation_query` returning no rows is reported as an idempotent success. This is +on by **default** for Db2; on Oracle you opt in per entitlement with +`validation_queries_signal_idempotency: true` on the grant or revoke: ```yaml grant: @@ -31,18 +35,15 @@ revoke: - REVOKE ... ``` -With the flag on, a `validation_query` returning no rows is reported as an **idempotent success** +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". - -The flag only takes effect on the DDL engines that need it (Db2, Oracle). On every other engine a -no-rows result still fails loudly regardless of the flag, because those engines report -rows-affected and don't need the reinterpretation. Default off, so existing configs are unchanged. +already as desired, there is no work to do". On Db2 (default-on) this reinterpretation also +requires `no_transaction: true`, since Db2 reports no rows-affected under a transaction. ## Writing the query: "is there work to do?", not "does this exist?" -When the flag is on, your `validation_queries` must answer **"is there work to do?"**, not -**"does this principal or role exist?"**. +On the DDL engines (Db2, Oracle), your `validation_queries` must answer **"is there work to do?"**, +not **"does this principal or role exist?"**. **Do not use them as existence preconditions.** A no-rows result is swallowed as idempotent success, so a missing, deleted, or mistyped principal or role is reported as "already done" diff --git a/examples/oracle-test.yml b/examples/oracle-test.yml index b8526680..c5dd3753 100644 --- a/examples/oracle-test.yml +++ b/examples/oracle-test.yml @@ -305,31 +305,31 @@ resource_types: # result as an idempotent success so re-granting a held role reports GrantAlreadyExists. validation_queries_signal_idempotency: true # Returns a row only while the role is NOT yet granted (no rows => already granted). - # Exact-match (no UPPER) is required: the GRANT quotes identifiers with |identifier, so - # the stored GRANTEE/GRANTED_ROLE are case-sensitive and must equal the bound value. + # UPPER-normalize the principal: the GRANT inserts it |unquoted, so Oracle folds it to + # upper-case (CREATE USER jdoe => JDOE); DBA_ROLES roles are already upper-case. validation_queries: - | SELECT 1 FROM dual WHERE NOT EXISTS ( SELECT 1 FROM DBA_ROLE_PRIVS - WHERE GRANTEE = ? AND GRANTED_ROLE = ? + WHERE GRANTEE = UPPER(?) AND GRANTED_ROLE = ? ) queries: - | - GRANT ? TO ? + GRANT ? TO ? revoke: # Indicates that the revoke operation runs without a transaction no_transaction: true # Without this, a repeat revoke of an already-removed role fails with ORA-01951. validation_queries_signal_idempotency: true # Returns a row only while the role IS still granted (no rows => already revoked). - # Exact-match (no UPPER) is required for the same reason as the grant validation. + # UPPER-normalize the principal for the same reason as the grant validation. validation_queries: - | SELECT 1 FROM DBA_ROLE_PRIVS - WHERE GRANTEE = ? AND GRANTED_ROLE = ? + WHERE GRANTEE = UPPER(?) AND GRANTED_ROLE = ? queries: - | - REVOKE ? FROM ? + REVOKE ? FROM ? - id: "admin" # Entitlement identifier for role administration privileges # Dynamic display name for admin entitlement, appending ' Role Admin' display_name: "resource.DisplayName + ' Role Admin'" @@ -351,14 +351,14 @@ resource_types: no_transaction: true queries: - | - GRANT ? TO ? WITH ADMIN OPTION + GRANT ? TO ? WITH ADMIN OPTION revoke: no_transaction: true queries: - | - REVOKE ? FROM ? + REVOKE ? FROM ? - | - GRANT ? TO ? + GRANT ? TO ? # Dynamic grants based on SQL queries to associate users with roles grants: - query: | diff --git a/pkg/bsql/config.go b/pkg/bsql/config.go index bd0950b5..077d336e 100644 --- a/pkg/bsql/config.go +++ b/pkg/bsql/config.go @@ -429,9 +429,10 @@ type EntitlementProvisioningQueries struct { // ValidationQueriesSignalIdempotency opts this entitlement into treating a no-rows // ValidationQueries result as an idempotent success (GrantAlreadyExists on grant, - // GrantAlreadyRevoked on revoke) instead of a failed precondition. It only takes effect - // on DDL engines whose GRANT/REVOKE don't report rows-affected (Db2, Oracle); on every - // other engine a no-rows result still fails loudly regardless of this flag. Default off. + // GrantAlreadyRevoked on revoke) instead of a failed precondition. The reinterpretation + // only applies on DDL engines whose GRANT/REVOKE don't report rows-affected (Db2, Oracle); + // Db2 does it by default and ignores this flag, so this opt-in is only needed on Oracle. On + // every other engine a no-rows result still fails loudly regardless of this flag. Default off. // // Warning: when enabled, do NOT use ValidationQueries as existence preconditions // (e.g. "does this user/role exist?"). A no-rows result is reported as idempotent diff --git a/pkg/bsql/provisioning.go b/pkg/bsql/provisioning.go index 3ae7a091..d1d0da26 100644 --- a/pkg/bsql/provisioning.go +++ b/pkg/bsql/provisioning.go @@ -80,9 +80,11 @@ func (s *SQLSyncer) Grant(ctx context.Context, principal *v2.Resource, entitleme principal, provisioningConfig.Grant.Queries, provisioningConfig.Grant.ValidationQueries, - provisioningConfig.Grant.ValidationQueriesSignalIdempotency, provisioningVars, - useTx, + ProvisioningOptions{ + SignalIdempotency: provisioningConfig.Grant.ValidationQueriesSignalIdempotency, + UseTransaction: useTx, + }, provisioningConfig.Grant.GrantReplace, provisioningConfig.Grant.RejectIf, ) @@ -150,10 +152,12 @@ func (s *SQLSyncer) Revoke(ctx context.Context, grant *v2.Grant) (annotations.An ctx, provisioningConfig.Revoke.Queries, provisioningConfig.Revoke.ValidationQueries, - provisioningConfig.Revoke.ValidationQueriesSignalIdempotency, existsCheck, provisioningVars, - useTx, + ProvisioningOptions{ + SignalIdempotency: provisioningConfig.Revoke.ValidationQueriesSignalIdempotency, + UseTransaction: useTx, + }, ) // The exists-check still runs when the revoke queries affected zero rows, so // principalDeleted is only meaningful on the success and already-revoked paths. diff --git a/pkg/bsql/provisioning_grant_reject_test.go b/pkg/bsql/provisioning_grant_reject_test.go index ce88bc92..23e1e750 100644 --- a/pkg/bsql/provisioning_grant_reject_test.go +++ b/pkg/bsql/provisioning_grant_reject_test.go @@ -49,12 +49,11 @@ func TestRunGrantProvisioning_RejectIfMatchReturnsGrantCancelledAndSkipsMutation nil, []string{`INSERT INTO user_roles (user_id, role) VALUES (?, ?)`}, nil, - false, map[string]any{ "user_id": "user-1", "role": "admin", }, - true, + ProvisioningOptions{UseTransaction: true}, nil, &GrantRejectIfProvisioningQuery{ Query: `SELECT 1 AS rejected`, @@ -81,12 +80,11 @@ func TestRunGrantProvisioning_RejectIfNoMatchProceedsWithGrant(t *testing.T) { nil, []string{`INSERT INTO user_roles (user_id, role) VALUES (?, ?)`}, nil, - false, map[string]any{ "user_id": "user-1", "role": "admin", }, - true, + ProvisioningOptions{UseTransaction: true}, nil, &GrantRejectIfProvisioningQuery{ Query: `SELECT 1 AS rejected WHERE 0`, diff --git a/pkg/bsql/provisioning_revoke_deleted_test.go b/pkg/bsql/provisioning_revoke_deleted_test.go index ff62b67c..f43d93da 100644 --- a/pkg/bsql/provisioning_revoke_deleted_test.go +++ b/pkg/bsql/provisioning_revoke_deleted_test.go @@ -77,10 +77,9 @@ func TestRunRevokeProvisioning_LastRoleDeletesPrincipal(t *testing.T) { t.Context(), revokeDeletesUserQueries, nil, - false, principalExistsCheck(), map[string]any{"principal_id": "user-1", "role": "admin"}, - true, + ProvisioningOptions{UseTransaction: true}, ) require.NoError(t, err) require.True(t, deleted) @@ -97,10 +96,9 @@ func TestRunRevokeProvisioning_KeepsPrincipalWhenOtherRolesRemain(t *testing.T) t.Context(), revokeDeletesUserQueries, nil, - false, principalExistsCheck(), map[string]any{"principal_id": "user-1", "role": "admin"}, - true, + ProvisioningOptions{UseTransaction: true}, ) require.NoError(t, err) require.False(t, deleted) @@ -119,10 +117,9 @@ func TestRunRevokeProvisioning_AllZeroRowsWithDeletedPrincipal(t *testing.T) { t.Context(), revokeDeletesUserQueries, nil, - false, principalExistsCheck(), map[string]any{"principal_id": "user-1", "role": "admin"}, - true, + ProvisioningOptions{UseTransaction: true}, ) require.ErrorIs(t, err, ErrQueryAffectedZeroRows) require.True(t, deleted) @@ -137,10 +134,9 @@ func TestRunRevokeProvisioning_AllZeroRowsWithSurvivingPrincipal(t *testing.T) { t.Context(), revokeDeletesUserQueries, nil, - false, principalExistsCheck(), map[string]any{"principal_id": "user-1", "role": "admin"}, - true, + ProvisioningOptions{UseTransaction: true}, ) require.ErrorIs(t, err, ErrQueryAffectedZeroRows) require.False(t, deleted) @@ -161,10 +157,9 @@ func TestRunRevokeProvisioning_DDLValidationNoRowsSkipsExistsCheck(t *testing.T) t.Context(), []string{`DELETE FROM user_roles WHERE user_id = ? AND role = ?`}, []string{`SELECT 1 FROM user_roles WHERE user_id = ? AND role = ?`}, - true, principalExistsCheck(), map[string]any{"principal_id": "user-1", "role": "admin"}, - true, + ProvisioningOptions{SignalIdempotency: true, UseTransaction: true}, ) require.ErrorIs(t, err, ErrQueryAffectedZeroRows) require.False(t, deleted, "exists-check must be skipped when the sentinel came from validation") @@ -179,10 +174,9 @@ func TestRunRevokeProvisioning_NoExistsCheckBehavesLikeBefore(t *testing.T) { t.Context(), []string{`DELETE FROM user_roles WHERE user_id = ? AND role = ?`}, nil, - false, nil, map[string]any{"principal_id": "user-1", "role": "admin"}, - true, + ProvisioningOptions{UseTransaction: true}, ) require.NoError(t, err) require.False(t, deleted) @@ -193,10 +187,9 @@ func TestRunRevokeProvisioning_NoExistsCheckBehavesLikeBefore(t *testing.T) { t.Context(), []string{`DELETE FROM user_roles WHERE user_id = ? AND role = ?`}, nil, - false, nil, map[string]any{"principal_id": "user-1", "role": "admin"}, - true, + ProvisioningOptions{UseTransaction: true}, ) require.ErrorIs(t, err, ErrQueryAffectedZeroRows) require.False(t, deleted) @@ -212,10 +205,9 @@ func TestRunRevokeProvisioning_ProbeErrorKeepsRevoke(t *testing.T) { t.Context(), []string{`DELETE FROM user_roles WHERE user_id = ? AND role = ?`}, nil, - false, &PrincipalExistsCheck{Query: `SELECT 1 FROM nonexistent_table WHERE id = ?`}, map[string]any{"principal_id": "user-1", "role": "admin"}, - true, + ProvisioningOptions{UseTransaction: true}, ) require.NoError(t, err) require.False(t, deleted) diff --git a/pkg/bsql/provisioning_validation_idempotency_test.go b/pkg/bsql/provisioning_validation_idempotency_test.go index 16bd9a52..b268565b 100644 --- a/pkg/bsql/provisioning_validation_idempotency_test.go +++ b/pkg/bsql/provisioning_validation_idempotency_test.go @@ -237,9 +237,9 @@ func TestRunProvisioningQueriesWithExecutor_ValidationNoRowsWrapsSentinel(t *tes []string{`DELETE FROM user_roles WHERE user_id = ?`}, []string{revokeValidationQuery}, "revoke provisioning", - true, map[string]any{"principal_id": "user-1", "role": "admin"}, db, + ProvisioningOptions{SignalIdempotency: true}, ) require.ErrorIs(t, err, ErrQueryAffectedZeroRows) // guard the operation prefix so a rebase can't silently drop it (it's the only per-call diagnostic once swallowed into an annotation) diff --git a/pkg/bsql/query.go b/pkg/bsql/query.go index 26207e8b..a5c3eb68 100644 --- a/pkg/bsql/query.go +++ b/pkg/bsql/query.go @@ -439,14 +439,21 @@ func (s *SQLSyncer) resolveProvisioningDB(vars map[string]any) (*sql.DB, error) return nil, fmt.Errorf("provisioning: primary database %q not found in handles (configured: %v)", s.primaryDBName, s.dbNames) } +// ProvisioningOptions carries the boolean knobs shared by the exported provisioning entrypoints so +// new options can be added without breaking out-of-repo callers. +type ProvisioningOptions struct { + SignalIdempotency bool + UseTransaction bool +} + func (s *SQLSyncer) RunProvisioningQueries( ctx context.Context, queries, validationQueries []string, - signalIdempotency bool, vars map[string]any, - useTx bool, + opts ProvisioningOptions, ) error { + useTx := opts.UseTransaction l := ctxzap.Extract(ctx).With( zap.Bool("use_tx", useTx), ) @@ -482,9 +489,9 @@ func (s *SQLSyncer) RunProvisioningQueries( queries, validationQueries, "provisioning", - signalIdempotency, vars, executor, + ProvisioningOptions{SignalIdempotency: opts.SignalIdempotency}, ) if err != nil { return err @@ -525,13 +532,12 @@ func (s *SQLSyncer) RunRevokeProvisioning( ctx context.Context, queries, validationQueries []string, - signalIdempotency bool, existsCheck *PrincipalExistsCheck, vars map[string]any, - useTx bool, + opts ProvisioningOptions, ) (bool, error) { l := ctxzap.Extract(ctx).With( - zap.Bool("use_tx", useTx), + zap.Bool("use_tx", opts.UseTransaction), ) ctx = ctxzap.ToContext(ctx, l) @@ -541,7 +547,7 @@ func (s *SQLSyncer) RunRevokeProvisioning( return false, err } - allZero, fromValidation, err := s.runRevokeQueries(ctx, queries, validationQueries, signalIdempotency, vars, useTx, target) + allZero, fromValidation, err := s.runRevokeQueries(ctx, queries, validationQueries, opts.SignalIdempotency, vars, opts.UseTransaction, target) if err != nil { return false, err } @@ -609,7 +615,7 @@ func (s *SQLSyncer) runRevokeQueries( } var allZero, fromValidation bool - err := s.RunProvisioningQueriesWithExecutor(ctx, queries, validationQueries, "revoke provisioning", signalIdempotency, vars, executor) + err := s.RunProvisioningQueriesWithExecutor(ctx, queries, validationQueries, "revoke provisioning", vars, executor, ProvisioningOptions{SignalIdempotency: signalIdempotency}) if err != nil { if !errors.Is(err, ErrQueryAffectedZeroRows) { return false, false, err @@ -717,10 +723,6 @@ func (s *SQLSyncer) runValidationQueries( ) error { l := ctxzap.Extract(ctx) - if signalIdempotency && !isDDLEngine(s.dbEngine) { - l.Warn("validation_queries_signal_idempotency is set but ignored on this engine; only DDL engines (Db2, Oracle) reinterpret a no-rows validation as an idempotent success") - } - for _, q := range validationQueries { q, qArgs, err := s.prepareProvisioningQuery(q, vars) if err != nil { @@ -767,13 +769,13 @@ func (s *SQLSyncer) RunProvisioningQueriesWithExecutor( queries, validationQueries []string, operation string, - signalIdempotency bool, vars map[string]any, executor executor, + opts ProvisioningOptions, ) error { l := ctxzap.Extract(ctx) - if err := s.runValidationQueries(ctx, validationQueries, operation, signalIdempotency, vars, executor); err != nil { + if err := s.runValidationQueries(ctx, validationQueries, operation, opts.SignalIdempotency, vars, executor); err != nil { return err } @@ -1018,12 +1020,12 @@ func (s *SQLSyncer) RunGrantProvisioning( resource *v2.Resource, queries, validationQueries []string, - signalIdempotency bool, vars map[string]any, - useTx bool, + opts ProvisioningOptions, replace *GrantReplaceProvisioningQueries, rejectIf *GrantRejectIfProvisioningQuery, ) (annotations.Annotations, error) { + useTx := opts.UseTransaction l := ctxzap.Extract(ctx) anno := annotations.New() @@ -1164,16 +1166,15 @@ func (s *SQLSyncer) RunGrantProvisioning( provisioningConfig.Revoke.Queries, provisioningConfig.Revoke.ValidationQueries, "grant_replace revoke", - provisioningConfig.Revoke.ValidationQueriesSignalIdempotency, provisioningVars, executor, + ProvisioningOptions{SignalIdempotency: provisioningConfig.Revoke.ValidationQueriesSignalIdempotency}, ) if err != nil { // A zero-rows sentinel means the replace revoke had nothing to remove: the revoke - // queries matched nothing, or a validation query returned no rows on a DDL engine - // that opted into validation_queries_signal_idempotency. Reporting the latter as - // GrantReplaced leans on that opt-in meaning "already gone"; the gate keeps it off - // for existence-precondition queries. + // queries matched nothing, or a no-rows validation was reinterpreted as idempotent + // on a DDL engine (Db2 by default, Oracle on validation_queries_signal_idempotency). + // Reporting the latter as GrantReplaced leans on that meaning "already gone". if !errors.Is(err, ErrQueryAffectedZeroRows) { return anno, err } @@ -1187,7 +1188,7 @@ func (s *SQLSyncer) RunGrantProvisioning( } } - if err := s.runValidationQueries(ctx, validationQueries, "grant provisioning", signalIdempotency, vars, executor); err != nil { + if err := s.runValidationQueries(ctx, validationQueries, "grant provisioning", opts.SignalIdempotency, vars, executor); err != nil { return anno, err } diff --git a/pkg/bsql/user_syncer.go b/pkg/bsql/user_syncer.go index 07202094..7eab2e88 100644 --- a/pkg/bsql/user_syncer.go +++ b/pkg/bsql/user_syncer.go @@ -166,7 +166,7 @@ func (s *userSyncer) CreateAccount( // Execute account creation queries useTransaction := !provisioningConfig.Create.NoTransaction - if err := s.RunProvisioningQueries(ctx, provisioningConfig.Create.Queries, nil, false, queryInputs, useTransaction); err != nil { + if err := s.RunProvisioningQueries(ctx, provisioningConfig.Create.Queries, nil, queryInputs, ProvisioningOptions{UseTransaction: useTransaction}); err != nil { return nil, nil, nil, err } @@ -224,7 +224,7 @@ func (s *userSyncer) Rotate(ctx context.Context, resourceId *v2.ResourceId, cred // Execute account creation queries useTransaction := !rotationConfig.Update.NoTransaction - if err := s.RunProvisioningQueries(ctx, rotationConfig.Update.Queries, nil, false, queryInputs, useTransaction); err != nil { + if err := s.RunProvisioningQueries(ctx, rotationConfig.Update.Queries, nil, queryInputs, ProvisioningOptions{UseTransaction: useTransaction}); err != nil { return nil, nil, err } diff --git a/pkg/bsql/validate.go b/pkg/bsql/validate.go index d658e4e4..1bf22621 100644 --- a/pkg/bsql/validate.go +++ b/pkg/bsql/validate.go @@ -89,8 +89,10 @@ func (l *EntitlementMapping) staticValidate(ctx context.Context, s *SQLSyncer) e // validateProvisioningIdempotency validates the validation_queries and the // validation_queries_signal_idempotency opt-in shared by grant and revoke. Validation queries are -// always vars-checked; the opt-in additionally requires at least one validation query and, on a -// DDL engine, no_transaction (Db2/Oracle GRANT/REVOKE report no rows-affected under a tx). +// always vars-checked. The opt-in is only valid on DDL engines (Db2, Oracle) and requires at least +// one validation query. no_transaction is required whenever a no-rows validation is reinterpreted +// as idempotent (Db2 by default, Oracle on opt-in): Db2/Oracle GRANT/REVOKE report no rows-affected +// under a tx, which would roll the statement back. func validateProvisioningIdempotency(s *SQLSyncer, pq EntitlementProvisioningQueries, vars map[string]string) error { for _, query := range pq.ValidationQueries { if err := validateVarsInQuery(s, query, vars); err != nil { @@ -99,12 +101,18 @@ func validateProvisioningIdempotency(s *SQLSyncer, pq EntitlementProvisioningQue } if pq.ValidationQueriesSignalIdempotency { + if !isDDLEngine(s.dbEngine) { + return errors.New("validation_queries_signal_idempotency is only supported on DDL engines (Db2, Oracle)") + } if len(pq.ValidationQueries) < 1 { return errors.New("validation_queries_signal_idempotency requires at least one validation_query") } - if isDDLEngine(s.dbEngine) && !pq.NoTransaction { - return errors.New("validation_queries_signal_idempotency requires no_transaction: true on DDL engines (Db2/Oracle)") - } + } + + if len(pq.ValidationQueries) > 0 && + s.validationNoRowsMeansIdempotent(pq.ValidationQueriesSignalIdempotency) && + !pq.NoTransaction { + return errors.New("validation_queries with no-rows idempotency require no_transaction: true on DDL engines (Db2/Oracle)") } return nil diff --git a/pkg/bsql/validate_test.go b/pkg/bsql/validate_test.go index 10aa9d81..0a9d3098 100644 --- a/pkg/bsql/validate_test.go +++ b/pkg/bsql/validate_test.go @@ -137,9 +137,21 @@ func TestValidateProvisioningIdempotency(t *testing.T) { wantErr: false, }, { - name: "signal on non-DDL engine does not require no_transaction", + name: "signal on non-DDL engine is rejected", engine: database.PostgreSQL, - pq: EntitlementProvisioningQueries{ValidationQueriesSignalIdempotency: true, ValidationQueries: oneValidation}, + pq: EntitlementProvisioningQueries{ValidationQueriesSignalIdempotency: true, NoTransaction: true, ValidationQueries: oneValidation}, + wantErr: true, + }, + { + name: "Db2 default-on validation without no_transaction fails", + engine: database.DB2, + pq: EntitlementProvisioningQueries{ValidationQueries: oneValidation}, + wantErr: true, + }, + { + name: "Db2 default-on validation with no_transaction ok", + engine: database.DB2, + pq: EntitlementProvisioningQueries{NoTransaction: true, ValidationQueries: oneValidation}, wantErr: false, }, { diff --git a/pkg/connector/action.go b/pkg/connector/action.go index 09176786..401107b8 100644 --- a/pkg/connector/action.go +++ b/pkg/connector/action.go @@ -211,7 +211,7 @@ func (c *Connector) handleQueryAction(ctx context.Context, actionKey string, act } else { queries = []string{actionCfg.Query} } - err = sqlSyncer.RunProvisioningQueries(ctx, queries, nil, false, argMap, !actionCfg.NoTransaction) + err = sqlSyncer.RunProvisioningQueries(ctx, queries, nil, argMap, bsql.ProvisioningOptions{UseTransaction: !actionCfg.NoTransaction}) if err != nil { return nil, nil, err } From bf1d3016be407aa3103cf5e9457ebb03a6f760d6 Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Fri, 11 Sep 2026 12:57:25 -0500 Subject: [PATCH 5/5] CXH-2417: docs: no_transaction is required on Oracle-with-flag too, not just Db2 --- docs/provisioning.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/provisioning.md b/docs/provisioning.md index e63980e5..33e69ebb 100644 --- a/docs/provisioning.md +++ b/docs/provisioning.md @@ -37,8 +37,9 @@ revoke: 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". On Db2 (default-on) this reinterpretation also -requires `no_transaction: true`, since Db2 reports no rows-affected under a transaction. +already as desired, there is no work to do". Wherever this reinterpretation is active (Db2 always, +Oracle when the flag is on), the grant/revoke must also set `no_transaction: true`, since Db2 and +Oracle report no rows-affected under a transaction. ## Writing the query: "is there work to do?", not "does this exist?"