CXH-2379: fix grant/revoke idempotency for DDL-based engines - #151
CXH-2379: fix grant/revoke idempotency for DDL-based engines#151al-conductorone wants to merge 4 commits into
Conversation
Validation-query "no rows" now wraps ErrQueryAffectedZeroRows so the provisioning layer's errors.Is check reports GrantAlreadyExists / GrantAlreadyRevoked instead of failing the task. DDL dialects (e.g. Db2) whose GRANT/REVOKE raise an error rather than affecting rows can only signal prior state through validation_queries, which previously landed on the failing path. Adds regression tests driving Grant/Revoke end-to-end over in-memory sqlite for both the already-applied (idempotent) and apply cases.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return fmt.Errorf("validation query returned no rows") | ||
| // Wrap the sentinel so the idempotency path reports already-applied instead of | ||
| // failing; validation "no rows" is the only zero-effect signal DDL dialects (Db2) emit. | ||
| return fmt.Errorf("validation query returned no rows: %w", ErrQueryAffectedZeroRows) |
There was a problem hiding this comment.
🟠 Bug: This reinterprets every validation_queries "no rows" as "already applied", for all engines and all configs — not just DDL dialects. This repo's own shipped example (examples/postgres-test.yml:384-391) uses the grant validation query as an existence precondition (FROM users u, roles r WHERE u.username = ? AND r.role_name = ?), so after this change a grant for a nonexistent user or role returns nil error + GrantAlreadyExists and ConductorOne records access that was never applied. Suggest gating the new semantics behind an opt-in field on EntitlementProvisioningQueries (e.g. validation_queries_signal_idempotency: true) so existing precondition-style configs keep failing loudly.
| return anno, fmt.Errorf("grant provisioning: validation query returned no rows") | ||
| // Wrap the sentinel so the caller reports GrantAlreadyExists instead of failing; | ||
| // validation "no rows" is the only zero-effect signal DDL dialects (Db2) emit. | ||
| return anno, fmt.Errorf("grant provisioning: validation query returned no rows: %w", ErrQueryAffectedZeroRows) |
There was a problem hiding this comment.
🟠 Bug: Same concern as the sibling change in RunProvisioningQueriesWithExecutor — Grant (pkg/bsql/provisioning.go:89) now converts this into nil error + GrantAlreadyExists, so a genuine precondition failure (missing user/role, wrong tenant) is reported to ConductorOne as a successful grant instead of an error. Gate this behind opt-in config, or scope it to DDL dialects via s.dbEngine, rather than changing behavior for all existing configs.
Connector PR Review: CXH-2379: fix grant/revoke idempotency for DDL-based enginesBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe new commit adds Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agentsNote: this run could not write the machine-readable review-state marker (the sandbox blocked the required literal), so the next review will fall back to full mode against base |
A validation query returning no rows now maps to ErrQueryAffectedZeroRows (reported as GrantAlreadyExists / GrantAlreadyRevoked) only on DDL-based engines (Db2), which don't report rows-affected. Other engines keep using validation queries as existence preconditions that fail loudly, so a grant against a missing user or role is no longer silently reported as success. This also restores the grant_replace abort behavior on those engines: a replaced-grant revoke whose validation returns no rows returns a plain error instead of the sentinel, so GrantReplaced is not emitted. Document the engine-specific ValidationQueries semantics and add a test covering the non-DDL loud-failure path.
| } | ||
|
|
||
| func TestRevoke_ValidationNoRowsReportsAlreadyRevoked(t *testing.T) { | ||
| s, _ := newRevokeProvisioningTestSyncer(t) |
There was a problem hiding this comment.
🟡 Suggestion: the engine gate is covered asymmetrically — TestGrant_ValidationNoRowsOnNonDDLEngineFailsLoudly pins the non-DDL behavior for RunGrantProvisioning, but there's no equivalent for the revoke path through RunProvisioningQueriesWithExecutor. Adding a mirror test that leaves s.dbEngine at the default and asserts Revoke returns an error (and no GrantAlreadyRevoked) when the revoke validation query matches nothing would lock in both halves of validationNoRowsMeansIdempotent(). (medium confidence)
Mirror TestGrant_ValidationNoRowsOnNonDDLEngineFailsLoudly on the revoke path: on a non-DB2 engine, a revoke validation query returning no rows is a failed precondition, so Revoke returns an error with nil annotations rather than GrantAlreadyRevoked. Pins the false branch of validationNoRowsMeansIdempotent() for RunProvisioningQueriesWithExecutor.
|
|
||
| if !valid { | ||
| if s.validationNoRowsMeansIdempotent() { | ||
| return anno, fmt.Errorf("grant provisioning: validation query returned no rows: %w", ErrQueryAffectedZeroRows) |
There was a problem hiding this comment.
🟡 Suggestion: on the DB2 path this returns the sentinel with anno already populated, but Grant (pkg/bsql/provisioning.go:89-94) discards the returned annotations and builds a fresh annotations.Annotations{} with only GrantAlreadyExists. With no_transaction: true there is no rollback, so a grant_replace revoke that already committed above (line 1042) is lost: the DB revoked the old grant but GrantReplaced never reaches ConductorOne. Consider preserving the returned annotations in the errors.Is(err, ErrQueryAffectedZeroRows) branch of Grant (medium confidence — requires the DB2 + grant_replace + validation_queries + no_transaction combination).
| // don't report rows-affected, so the validation query is the only zero-effect signal | ||
| // available. Engines that report rows-affected keep using validation queries as | ||
| // existence preconditions that fail loudly. | ||
| func (s *SQLSyncer) validationNoRowsMeansIdempotent() bool { |
There was a problem hiding this comment.
[Review] this might mask real failures, not just idempotency
So validationNoRowsMeansIdempotent() just checks the engine, it doesn't care what the validation query was actually checking. Problem is, examples/postgres-test.yml:384-391 already does validation_queries as a plain existence precondition (checks a user + role row exist) that has nothing to do with grant idempotency — idempotency there is handled separately via ON CONFLICT DO NOTHING in the actual insert.
If someone copies that exact pattern into a DB2 config, a typo'd or dropped role would make the validation query return 0 rows too, and now that silently becomes GrantAlreadyExists/GrantAlreadyRevoked instead of a hard error. Kinda the opposite failure mode from what this PR is fixing lol. Nothing else in the config (RejectIf is opposite polarity + grant-only, PrincipalExistsCheck runs after commit and is non-fatal) would catch it, and the new tests only cover the intended idempotency-gate case.
Might be worth a flag/second list to distinguish "precondition" queries from "idempotency gate" queries, or at least a loud warning in the ValidationQueries doc comment so DB2 authors don't reuse the postgres-test.yml pattern verbatim.
| // don't report rows-affected, so the validation query is the only zero-effect signal | ||
| // available. Engines that report rows-affected keep using validation queries as | ||
| // existence preconditions that fail loudly. | ||
| func (s *SQLSyncer) validationNoRowsMeansIdempotent() bool { |
There was a problem hiding this comment.
[Review] why only DB2?
The bug report and this PR's own description frame this as a general "DDL-based engines" problem, not a DB2-only thing — and examples/oracle-test.yml has the exact same DDL-shaped GRANT ... TO / REVOKE pattern, so Oracle is probably exposed to the same bug. Hardcoding database.DB2 here means it's still broken there.
Could totally be intentional (only fix what's actually verified, per the stability-first vibe of this repo) — if so no action needed, just curious if that's the reasoning or if it's worth a quick follow-up ticket for Oracle/MSSQL/HDB/Vertica too.
| } | ||
|
|
||
| if !valid { | ||
| if s.validationNoRowsMeansIdempotent() { |
There was a problem hiding this comment.
[Review] duplicated with the same block in RunGrantProvisioning
This loop and the one in RunGrantProvisioning (~line 1087) were already near-identical before this PR, and now they both got the same 4-line idempotency check copy-pasted in. They've actually already drifted from each other independently — the RunGrantProvisioning copy does _ = result.Close() on the result.Err() failure path, this one doesn't.
Not blocking, just a nit — might be worth pulling into a shared runValidationQueries() helper since this PR touched both spots identically anyway, so future changes (like extending the DB2 check to other engines) don't need to be applied twice.
3 participants
Repeat grant or revoke requests against DDL-based databases (such as Db2) no longer fail; the connector now recognizes when access is already in the requested state and reports the operation as a successful no-op.