CXH-2417: extend DDL grant/revoke idempotency to Oracle behind a per-config opt-in - #152
Conversation
| database.Oracle: true, | ||
| database.SQLite: false, | ||
| database.MySQL: false, | ||
| database.PostgreSQL: false, |
There was a problem hiding this comment.
[Review] nice, the gate itself is solid — one gap though
This nails down the boolean gate (opt-in off never signals idempotency, opt-in on only for Db2/Oracle), but there's no actual end-to-end Grant()/Revoke() test with dbEngine = database.Oracle + opt-in on asserting the GrantAlreadyExists/GrantAlreadyRevoked annotations actually come out — Db2 gets that full-path test, Oracle only gets exercised at this unit level. Probably low risk since the code path past the engine switch is identical, but would fully close the loop this PR is meant to validate.
Also noticed (unrelated file, examples/oracle-test.yml): the "admin" entitlements (WITH ADMIN OPTION) don't opt in and have no validation_queries, so a repeat revoke there still hits ORA-01951. Probably intentional but a one-line comment in the example would stop someone from thinking it got missed.
There was a problem hiding this comment.
the ..._Oracle full-path grant/revoke tests are already on the branch, so oracle gets end-to-end coverage too. admin entitlements opt in and use |keyword now.
…potency 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.
0b23827 to
0b6f229
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // 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 |
There was a problem hiding this comment.
🟡 Suggestion: This is a silent behavior change for Db2, not just an Oracle addition. Before this PR validationNoRowsMeansIdempotent() returned true for Db2 based on engine alone, so any Db2 config with validation_queries got no-rows-means-idempotent for free; now it hard-errors unless the config adds validation_queries_signal_idempotency: true. The PR description says "Off by default, so existing connectors are unchanged," which isn't true for Db2 configs written against #151 — worth an explicit migration note in docs/db2.md ("existing configs relying on the previous default must add the flag") and a correction in the PR body.
There was a problem hiding this comment.
fixed. db2 stays default-on by engine (unchanged from #151), only oracle needs the opt-in now.
|
|
||
| if !valid { | ||
| if s.validationNoRowsMeansIdempotent() { | ||
| if s.validationNoRowsMeansIdempotent(signalIdempotency) { |
There was a problem hiding this comment.
🟡 Suggestion: When an operator sets validation_queries_signal_idempotency: true on a non-DDL engine (Postgres, MySQL, MSSQL…), the flag is silently ignored and the first symptom is a hard grant/revoke failure that looks unrelated to the config. Consider a one-time l.Warn here (or in config load) when signalIdempotency is set but the engine isn't in the DDL set, so the ignored opt-in is visible rather than inferred from docs.
There was a problem hiding this comment.
Nit, not blocking. Please drop this Warn — C1 surfaces Warn as an incident. Either reject the invalid opt-in at config-validate time, or log it at Debug. Pattern: https://github.com/ConductorOne/baton-google-cloud-platform/pull/75
| ctx context.Context, | ||
| queries, | ||
| validationQueries []string, | ||
| signalIdempotency bool, |
There was a problem hiding this comment.
🟡 Suggestion: RunProvisioningQueries, RunRevokeProvisioning, RunProvisioningQueriesWithExecutor, and RunGrantProvisioning are all exported and each gains a positional bool. All in-repo callers are updated, but any out-of-repo consumer of pkg/bsql breaks at compile time. Since these signatures are already long and now carry two independent bools (signalIdempotency, useTx), an options struct would make this and future additions non-breaking and remove the positional-bool readability cost.
There was a problem hiding this comment.
Nit, not blocking. The extra positional bool on the exported Run*Provisioning* helpers is a compile break for any out-of-repo pkg/bsql caller. An options struct would keep this additive; not required for this PR if there are no known external callers.
| revoke.ValidationQueries = []string{ | ||
| `SELECT 1 FROM user_roles WHERE user_id = ?<user_id> AND role = 'does-not-exist'`, | ||
| } | ||
| revoke.ValidationQueriesSignalIdempotency = true |
There was a problem hiding this comment.
🟡 Suggestion: The grant_replace path only has coverage with the opt-in on. The opt-in-off case on a DDL engine changed behavior in this PR — the replace revoke's no-rows validation now returns a plain error that does not wrap ErrQueryAffectedZeroRows, so RunGrantProvisioning fails the whole grant instead of reporting GrantReplaced. Worth a companion test asserting that, since it's the new default for existing Db2 grant_replace configs.
Connector PR Review: CXH-2417: extend DDL grant/revoke idempotency to Oracle behind a per-config opt-inBlocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0 Review SummaryThe new commit is docs-only: Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
…te, |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.
| 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. |
There was a problem hiding this comment.
🟠 Bug: This contradicts the code. validationNoRowsMeansIdempotent (pkg/bsql/query.go:676) returns true for database.DB2 unconditionally — the flag is not required on Db2, as TestGrant_Db2ValidationNoRowsIdempotentByDefault asserts. A Db2 config author following this paragraph will write validation_queries as a loud existence precondition without the flag, and instead get the no-rows result swallowed as GrantAlreadyExists/GrantAlreadyRevoked — the exact silent-access bug this section warns about. Same mismatch in docs/provisioning.md:8-9,38-40, README.md:19, pkg/bsql/config.go:430-434, and the grant_replace comment at pkg/bsql/query.go:1173.
There was a problem hiding this comment.
Nit, not blocking. Please align docs/db2.md (and the matching README / config.go comments) with the actual gate: Db2 still treats no-rows as idempotent by engine, without requiring the opt-in. Operators following this paragraph today will write existence preconditions that get swallowed.
| if len(pq.ValidationQueries) < 1 { | ||
| return errors.New("validation_queries_signal_idempotency requires at least one validation_query") | ||
| } | ||
| if isDDLEngine(s.dbEngine) && !pq.NoTransaction { |
There was a problem hiding this comment.
🟡 Suggestion: This no_transaction requirement is nested under if pq.ValidationQueriesSignalIdempotency, so it never fires for the Db2 path that gets no-rows-means-idempotent by engine with the flag unset. A Db2 config with validation_queries and no no_transaction still takes the idempotent reinterpretation inside a transaction — the case this check exists to prevent. Consider gating on s.validationNoRowsMeansIdempotent(pq.ValidationQueriesSignalIdempotency) instead of the raw flag.
There was a problem hiding this comment.
Nit, not blocking. Please gate the no_transaction check on the path that actually reinterprets no-rows (Db2 by engine, Oracle via the flag), not only when validation_queries_signal_idempotency is set. Otherwise a Db2 config with validation_queries inside a transaction still hits the case this check exists to prevent.
| # 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 = ?<principal_name> AND GRANTED_ROLE = ?<role_name> | ||
| ) |
There was a problem hiding this comment.
🟡 Suggestion: Quoting the principal with |identifier and dropping UPPER() makes the whole flow case-exact, which is only safe when principal.ID is byte-identical to Oracle's stored identifier. Oracle folds unquoted CREATE USER jdoe to JDOE, so a connector whose principal ID comes from an application table (as in examples/oracle-test.yml, where the user resource uses id: ".username") can end up with GRANTEE = 'jdoe' matching nothing: the revoke validation returns no rows and, with the opt-in on, that reports GrantAlreadyRevoked while the role is still granted. The privilege entitlement in the same example kept UPPER(...) + |unquoted for the principal; keeping the principal UPPER-normalized here too (roles from DBA_ROLES are already uppercase) would avoid the divergence.
There was a problem hiding this comment.
Nit, not blocking. Please keep the principal comparison UPPER-normalized (or otherwise match how Oracle stores unquoted CREATE USER names). With |identifier + exact match, a lowercase principal.ID from the app table can report GrantAlreadyRevoked while the role is still granted. The privilege entitlement in the same example already uses UPPER(...) + |unquoted for that reason.
|
Approving. Please still look at the bot threads I replied to in this review — they are nits, not blockers, but they should be cleaned up (the new |
- 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
| 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. |
There was a problem hiding this comment.
🟡 Suggestion: no_transaction: true is now required on Oracle too, not just Db2 — validateProvisioningIdempotency (pkg/bsql/validate.go:112-116) errors whenever validationNoRowsMeansIdempotent is true and no_transaction is unset, which includes Oracle with the opt-in on (see the signal on DDL engine without no_transaction fails Oracle case in validate_test.go). Related: the example block above (lines 23-36) omits no_transaction: true, so copying it verbatim produces a config that fails startup validation on both engines.
| WHERE GRANTEE = UPPER(?<principal_name>) AND GRANTED_ROLE = ?<role_name> | ||
| ) | ||
| queries: | ||
| - GRANT ?<role_name|identifier> TO ?<principal_name|unquoted> |
There was a problem hiding this comment.
🟡 Suggestion: |unquoted runs the value through SanitizeIdentifier (pkg/bsql/query.go:54-58), which drops every char outside [A-Za-z0-9_] — including $ and #, both legal in Oracle usernames (e.g. the CDB common-user prefix C##ADMIN). The validation query binds the raw UPPER(?<principal_name>), so for such a principal the check passes against C##ADMIN while the DDL emits GRANT ... TO CADMIN, which either fails with ORA-01917 or, if a CADMIN user exists, grants to the wrong principal. Consider keeping |identifier and upper-casing principal_name in vars instead, or documenting the special-character limitation. Same pattern in examples/oracle-test.yml (grant/revoke around lines 333, 341, 354-362 and the privilege blocks).
Requested changes addressed in bf1d301; the bot's subsequent re-reviews (latest 2026-09-11 18:08) report no blocking issues and all checks pass.
4 participants
Lets Oracle and Db2 connectors treat an already-applied grant or revoke as success instead of erroring (Oracle otherwise fails a repeat revoke with ORA-01951), when the config opts in. Off by default, so existing connectors are unchanged. Stacks on #151.