diff --git a/README.md b/README.md index 6cf5ddac..4363f8f8 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 (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 d6be1211..b570e14d 100644 --- a/docs/db2.md +++ b/docs/db2.md @@ -223,10 +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, 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. 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 new file mode 100644 index 00000000..b10c880c --- /dev/null +++ b/docs/oracle.md @@ -0,0 +1,72 @@ +# 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) + # 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 = UPPER(?) AND GRANTED_ROLE = ? + ) + 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 = ? + queries: + - REVOKE ? FROM ? +``` + +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: + - GRANT ? TO ? +``` + +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..33e69ebb 100644 --- a/docs/provisioning.md +++ b/docs/provisioning.md @@ -1,37 +1,60 @@ # 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 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 -On every engine except Db2, a `validation_query` returning no rows **fails the operation**. -It is an existence precondition that aborts loudly. This includes the DDL engines that don't -report rows-affected (Oracle): treating their no-rows as idempotency is a follow-up that needs -a per-config opt-in first, so today they still fail loudly like everyone else. - -## Db2 (opt-in behind the `db2` build tag) - -Db2 applies `GRANT`/`REVOKE` as DDL that does not report rows-affected, so the connector -cannot tell from the statement itself whether it changed anything, and an already-applied -statement raises an error. To make grant and revoke idempotent, a `validation_query` -returning no rows is reported as an **idempotent success** (`GrantAlreadyExists` on grant, -`GrantAlreadyRevoked` on revoke). No rows means "the state is already as desired, there is no -work to do". Db2 ships opt-in behind the `db2` build tag, so no default-build engine changes -behavior. - -Because of this, on Db2 your `validation_queries` must answer **"is there work to do?"**, not -**"does this principal or role exist?"**. - -**Do not use `validation_queries` as existence preconditions on Db2.** A no-rows result is -swallowed as idempotent success, so a missing, deleted, or mistyped principal or role is -reported as "already done" instead of erroring. For example, a validation query like -`SELECT 1 FROM users WHERE name = ?` will silently mask a bad `user_id`: it returns -no rows, and the grant is reported as `GrantAlreadyExists` even though nothing was granted. +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 except where the DDL +no-rows-means-idempotent behavior below is active. + +## DDL engines: no rows means idempotent success + +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). 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: + 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 ... +``` + +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". 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?" + +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" +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..c5dd3753 100644 --- a/examples/oracle-test.yml +++ b/examples/oracle-test.yml @@ -301,15 +301,35 @@ 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). + # 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 = 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). + # UPPER-normalize the principal for the same reason as the grant validation. + validation_queries: + - | + SELECT 1 FROM DBA_ROLE_PRIVS + 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'" @@ -321,6 +341,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 @@ -329,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: | @@ -391,23 +413,44 @@ 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: + # 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 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: - | - 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 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 @@ -416,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/config.go b/pkg/bsql/config.go index b5383193..077d336e 100644 --- a/pkg/bsql/config.go +++ b/pkg/bsql/config.go @@ -423,15 +423,23 @@ 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. 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: 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..d1d0da26 100644 --- a/pkg/bsql/provisioning.go +++ b/pkg/bsql/provisioning.go @@ -81,7 +81,10 @@ func (s *SQLSyncer) Grant(ctx context.Context, principal *v2.Resource, entitleme provisioningConfig.Grant.Queries, provisioningConfig.Grant.ValidationQueries, provisioningVars, - useTx, + ProvisioningOptions{ + SignalIdempotency: provisioningConfig.Grant.ValidationQueriesSignalIdempotency, + UseTransaction: useTx, + }, provisioningConfig.Grant.GrantReplace, provisioningConfig.Grant.RejectIf, ) @@ -151,7 +154,10 @@ func (s *SQLSyncer) Revoke(ctx context.Context, grant *v2.Grant) (annotations.An provisioningConfig.Revoke.ValidationQueries, 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 bb9bfc4f..23e1e750 100644 --- a/pkg/bsql/provisioning_grant_reject_test.go +++ b/pkg/bsql/provisioning_grant_reject_test.go @@ -53,7 +53,7 @@ func TestRunGrantProvisioning_RejectIfMatchReturnsGrantCancelledAndSkipsMutation "user_id": "user-1", "role": "admin", }, - true, + ProvisioningOptions{UseTransaction: true}, nil, &GrantRejectIfProvisioningQuery{ Query: `SELECT 1 AS rejected`, @@ -84,7 +84,7 @@ func TestRunGrantProvisioning_RejectIfNoMatchProceedsWithGrant(t *testing.T) { "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_grant_replace_test.go b/pkg/bsql/provisioning_grant_replace_test.go index a919965b..5d423bfd 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 @@ -160,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_revoke_deleted_test.go b/pkg/bsql/provisioning_revoke_deleted_test.go index 194f99d7..f43d93da 100644 --- a/pkg/bsql/provisioning_revoke_deleted_test.go +++ b/pkg/bsql/provisioning_revoke_deleted_test.go @@ -79,7 +79,7 @@ func TestRunRevokeProvisioning_LastRoleDeletesPrincipal(t *testing.T) { nil, principalExistsCheck(), map[string]any{"principal_id": "user-1", "role": "admin"}, - true, + ProvisioningOptions{UseTransaction: true}, ) require.NoError(t, err) require.True(t, deleted) @@ -98,7 +98,7 @@ func TestRunRevokeProvisioning_KeepsPrincipalWhenOtherRolesRemain(t *testing.T) nil, principalExistsCheck(), map[string]any{"principal_id": "user-1", "role": "admin"}, - true, + ProvisioningOptions{UseTransaction: true}, ) require.NoError(t, err) require.False(t, deleted) @@ -119,7 +119,7 @@ func TestRunRevokeProvisioning_AllZeroRowsWithDeletedPrincipal(t *testing.T) { nil, principalExistsCheck(), map[string]any{"principal_id": "user-1", "role": "admin"}, - true, + ProvisioningOptions{UseTransaction: true}, ) require.ErrorIs(t, err, ErrQueryAffectedZeroRows) require.True(t, deleted) @@ -136,7 +136,7 @@ func TestRunRevokeProvisioning_AllZeroRowsWithSurvivingPrincipal(t *testing.T) { nil, principalExistsCheck(), map[string]any{"principal_id": "user-1", "role": "admin"}, - true, + ProvisioningOptions{UseTransaction: true}, ) require.ErrorIs(t, err, ErrQueryAffectedZeroRows) require.False(t, deleted) @@ -159,7 +159,7 @@ func TestRunRevokeProvisioning_DDLValidationNoRowsSkipsExistsCheck(t *testing.T) []string{`SELECT 1 FROM user_roles WHERE user_id = ? AND role = ?`}, 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") @@ -176,7 +176,7 @@ func TestRunRevokeProvisioning_NoExistsCheckBehavesLikeBefore(t *testing.T) { nil, nil, map[string]any{"principal_id": "user-1", "role": "admin"}, - true, + ProvisioningOptions{UseTransaction: true}, ) require.NoError(t, err) require.False(t, deleted) @@ -189,7 +189,7 @@ func TestRunRevokeProvisioning_NoExistsCheckBehavesLikeBefore(t *testing.T) { nil, nil, map[string]any{"principal_id": "user-1", "role": "admin"}, - true, + ProvisioningOptions{UseTransaction: true}, ) require.ErrorIs(t, err, ErrQueryAffectedZeroRows) require.False(t, deleted) @@ -207,7 +207,7 @@ func TestRunRevokeProvisioning_ProbeErrorKeepsRevoke(t *testing.T) { nil, &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_gate_test.go b/pkg/bsql/provisioning_validation_idempotency_gate_test.go index af68b7af..bfcc973a 100644 --- a/pkg/bsql/provisioning_validation_idempotency_gate_test.go +++ b/pkg/bsql/provisioning_validation_idempotency_gate_test.go @@ -7,26 +7,29 @@ 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 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) { - ddl := map[database.DbEngine]bool{ - database.DB2: true, - database.Oracle: false, - database.SQLite: false, - database.MySQL: false, - database.PostgreSQL: false, - database.MSSQL: false, - database.HDB: false, - database.Vertica: false, + 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, want := range ddl { + for engine, want := range cases { s := &SQLSyncer{dbEngine: engine} - require.Equal(t, want, s.validationNoRowsMeansIdempotent(), "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 33cee689..b268565b 100644 --- a/pkg/bsql/provisioning_validation_idempotency_test.go +++ b/pkg/bsql/provisioning_validation_idempotency_test.go @@ -16,7 +16,50 @@ 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) { + withBindFreeValidationConfigSignal(s, true) +} + +func withBindFreeValidationConfigSignal(s *SQLSyncer, signalIdempotency bool) { + 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: signalIdempotency, + Queries: []string{`INSERT INTO user_roles (user_id, role) VALUES (?, ?)`}, + }, + }, + Revoke: &RevokeEntitlementProvisioningQueries{ + EntitlementProvisioningQueries: EntitlementProvisioningQueries{ + ValidationQueries: []string{alwaysNoRows}, + ValidationQueriesSignalIdempotency: signalIdempotency, + Queries: []string{`DELETE 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 +71,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 = ?`}, }, }, }, @@ -73,6 +118,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) { @@ -120,6 +182,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) { @@ -161,8 +239,73 @@ func TestRunProvisioningQueriesWithExecutor_ValidationNoRowsWrapsSentinel(t *tes "revoke provisioning", 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) require.Contains(t, err.Error(), "revoke provisioning") } + +// 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 + // 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.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")) +} + +// 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.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 219bb379..a5c3eb68 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) }) @@ -387,13 +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, vars map[string]any, - useTx bool, + opts ProvisioningOptions, ) error { + useTx := opts.UseTransaction l := ctxzap.Extract(ctx).With( zap.Bool("use_tx", useTx), ) @@ -431,6 +491,7 @@ func (s *SQLSyncer) RunProvisioningQueries( "provisioning", vars, executor, + ProvisioningOptions{SignalIdempotency: opts.SignalIdempotency}, ) if err != nil { return err @@ -473,10 +534,10 @@ func (s *SQLSyncer) RunRevokeProvisioning( validationQueries []string, 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) @@ -486,7 +547,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, opts.SignalIdempotency, vars, opts.UseTransaction, target) if err != nil { return false, err } @@ -527,6 +588,7 @@ func (s *SQLSyncer) runRevokeQueries( ctx context.Context, queries, validationQueries []string, + signalIdempotency bool, vars map[string]any, useTx bool, target *sql.DB, @@ -553,7 +615,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", vars, executor, ProvisioningOptions{SignalIdempotency: signalIdempotency}) if err != nil { if !errors.Is(err, ErrQueryAffectedZeroRows) { return false, false, err @@ -618,20 +680,44 @@ 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 +// 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). 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 { + switch s.dbEngine { + 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 + } } func (s *SQLSyncer) runValidationQueries( ctx context.Context, validationQueries []string, operation string, + signalIdempotency bool, vars map[string]any, executor executor, ) error { @@ -667,7 +753,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) } @@ -685,10 +771,11 @@ func (s *SQLSyncer) RunProvisioningQueriesWithExecutor( operation string, vars map[string]any, executor executor, + opts ProvisioningOptions, ) error { l := ctxzap.Extract(ctx) - if err := s.runValidationQueries(ctx, validationQueries, operation, vars, executor); err != nil { + if err := s.runValidationQueries(ctx, validationQueries, operation, opts.SignalIdempotency, vars, executor); err != nil { return err } @@ -934,10 +1021,11 @@ func (s *SQLSyncer) RunGrantProvisioning( queries, validationQueries []string, 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() @@ -1080,12 +1168,13 @@ func (s *SQLSyncer) RunGrantProvisioning( "grant_replace revoke", 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 (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 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 } @@ -1099,7 +1188,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", opts.SignalIdempotency, vars, executor); err != nil { return anno, err } 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/user_syncer.go b/pkg/bsql/user_syncer.go index 114ebf0e..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, 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, 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 86ff5bd7..1bf22621 100644 --- a/pkg/bsql/validate.go +++ b/pkg/bsql/validate.go @@ -87,6 +87,37 @@ 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 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 { + return err + } + } + + 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 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 +} + func validateGrantProvisioningQueries(s *SQLSyncer, grant *GrantEntitlementProvisioningQueries, vars map[string]string) error { if grant == nil { return nil @@ -98,6 +129,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 +147,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..0a9d3098 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,77 @@ 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 is rejected", + engine: database.PostgreSQL, + 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, + }, + { + 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) + } + }) + } +} diff --git a/pkg/connector/action.go b/pkg/connector/action.go index 79d2ebed..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, argMap, !actionCfg.NoTransaction) + err = sqlSyncer.RunProvisioningQueries(ctx, queries, nil, argMap, bsql.ProvisioningOptions{UseTransaction: !actionCfg.NoTransaction}) if err != nil { return nil, nil, err }