From a57262e91cc76da2d35da7651d35b2da53dd5c7e Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Wed, 2 Sep 2026 11:49:07 -0500 Subject: [PATCH 01/13] CXH-2379: fix grant/revoke idempotency for DDL-based engines 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. --- ...rovisioning_validation_idempotency_test.go | 130 ++++++++++++++++++ pkg/bsql/query.go | 8 +- 2 files changed, 136 insertions(+), 2 deletions(-) create mode 100644 pkg/bsql/provisioning_validation_idempotency_test.go diff --git a/pkg/bsql/provisioning_validation_idempotency_test.go b/pkg/bsql/provisioning_validation_idempotency_test.go new file mode 100644 index 00000000..c4807baf --- /dev/null +++ b/pkg/bsql/provisioning_validation_idempotency_test.go @@ -0,0 +1,130 @@ +package bsql + +import ( + "testing" + + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + sdkGrant "github.com/conductorone/baton-sdk/pkg/types/grant" + "github.com/stretchr/testify/require" +) + +// grantValidationQuery returns a row only while the membership is absent, mirroring +// the DDL-dialect pattern where the validation query is the "is there work to do?" gate. +const grantValidationQuery = `SELECT 1 FROM users u WHERE u.id = ? AND NOT EXISTS (SELECT 1 FROM user_roles WHERE user_id = ? AND role = ?)` + +// revokeValidationQuery returns a row only while the membership is present. +const revokeValidationQuery = `SELECT 1 FROM user_roles WHERE user_id = ? AND role = ?` + +func withValidationQueryConfig(s *SQLSyncer) { + 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{grantValidationQuery}, + 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 = ?`}, + }, + }, + }, + }, + }, + } +} + +func memberEntitlementFor(role string) *v2.Entitlement { + roleResource := &v2.Resource{Id: &v2.ResourceId{ResourceType: "role", Resource: role}} + principal := &v2.Resource{Id: &v2.ResourceId{ResourceType: "user", Resource: "unused"}} + return sdkGrant.NewGrant(roleResource, "member", principal).GetEntitlement() +} + +func userPrincipal(userID string) *v2.Resource { + return &v2.Resource{Id: &v2.ResourceId{ResourceType: "user", Resource: userID}} +} + +func TestGrant_ValidationNoRowsReportsAlreadyExists(t *testing.T) { + s, db := newRevokeProvisioningTestSyncer(t) + withValidationQueryConfig(s) + // 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")) +} + +func TestGrant_ValidationRowsAppliesGrant(t *testing.T) { + s, db := newRevokeProvisioningTestSyncer(t) + withValidationQueryConfig(s) + // user exists without the role: validation returns a row, grant proceeds + seedUserWithRoles(t, db, "user-1") + + 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.False(t, ok) + + require.Equal(t, 1, countRows(t, db, `SELECT COUNT(*) FROM user_roles WHERE user_id = ? AND role = ?`, "user-1", "admin")) +} + +func TestRevoke_ValidationNoRowsReportsAlreadyRevoked(t *testing.T) { + s, _ := newRevokeProvisioningTestSyncer(t) + withValidationQueryConfig(s) + // 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) +} + +func TestRevoke_ValidationRowsAppliesRevoke(t *testing.T) { + s, db := newRevokeProvisioningTestSyncer(t) + withValidationQueryConfig(s) + seedUserWithRoles(t, db, "user-1", "admin") + + 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.False(t, ok) + + require.Equal(t, 0, countRows(t, db, `SELECT COUNT(*) FROM user_roles WHERE user_id = ? AND role = ?`, "user-1", "admin")) +} + +// The revoke helper must map validation "no rows" onto the sentinel so the caller +// can detect idempotency with errors.Is. +func TestRunProvisioningQueriesWithExecutor_ValidationNoRowsWrapsSentinel(t *testing.T) { + s, db := newRevokeProvisioningTestSyncer(t) + + err := s.RunProvisioningQueriesWithExecutor( + t.Context(), + []string{`DELETE FROM user_roles WHERE user_id = ?`}, + []string{revokeValidationQuery}, + map[string]any{"principal_id": "user-1", "role": "admin"}, + db, + ) + require.ErrorIs(t, err, ErrQueryAffectedZeroRows) +} diff --git a/pkg/bsql/query.go b/pkg/bsql/query.go index d47cf0d1..683279ff 100644 --- a/pkg/bsql/query.go +++ b/pkg/bsql/query.go @@ -641,7 +641,9 @@ func (s *SQLSyncer) RunProvisioningQueriesWithExecutor( } if !valid { - 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) } } @@ -1071,7 +1073,9 @@ func (s *SQLSyncer) RunGrantProvisioning( } if !valid { - 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) } } From c1786708cf93180fdfbc8c45f875f7e95fb825e8 Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Wed, 2 Sep 2026 14:48:28 -0500 Subject: [PATCH 02/13] CXH-2379: bump sync-test and account-provisioning CI actions to v4 --- .github/workflows/ci.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 1df22297..6a243303 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -41,14 +41,14 @@ jobs: - name: Build baton-sql run: go build ./cmd/baton-sql - name: Run sync tests - uses: ConductorOne/github-workflows/actions/sync-test@v2 + uses: ConductorOne/github-workflows/actions/sync-test@v4 with: connector: ./baton-sql baton-entitlement: 'role:admin:member' baton-principal: john.smith baton-principal-type: user - name: Run account provisioning tests - uses: ConductorOne/github-workflows/actions/account-provisioning@v3 + uses: ConductorOne/github-workflows/actions/account-provisioning@v4 with: connector: ./baton-sql account-email: robert.tables2@example.com From 399c8557f2d12cd8e03c41c5bfa754bc2db7ab12 Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Wed, 2 Sep 2026 15:05:49 -0500 Subject: [PATCH 03/13] CXH-2379: gate validation-query idempotency to DDL engines 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. --- pkg/bsql/config.go | 6 ++++- ...rovisioning_validation_idempotency_test.go | 23 ++++++++++++++++++ pkg/bsql/query.go | 24 ++++++++++++++----- 3 files changed, 46 insertions(+), 7 deletions(-) diff --git a/pkg/bsql/config.go b/pkg/bsql/config.go index e5a656d7..799eb923 100644 --- a/pkg/bsql/config.go +++ b/pkg/bsql/config.go @@ -422,7 +422,11 @@ type EntitlementProvisioningQueries struct { // NoTransaction indicates whether the provisioning queries should be executed without a transaction. NoTransaction bool `yaml:"no_transaction,omitempty" json:"no_transaction,omitempty"` - // ValidationQueries is a list of SQL statements to execute for validating the provisioning operation before execution. + // ValidationQueries is a list of SQL statements run before the provisioning queries. + // On engines that report rows-affected, a query returning no rows fails the operation + // (an existence precondition). On DDL-based engines (Db2) 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). ValidationQueries []string `yaml:"validation_queries,omitempty" json:"validation_queries,omitempty"` // Queries is a list of SQL statements to execute for the provisioning operation. diff --git a/pkg/bsql/provisioning_validation_idempotency_test.go b/pkg/bsql/provisioning_validation_idempotency_test.go index c4807baf..cc5e09ef 100644 --- a/pkg/bsql/provisioning_validation_idempotency_test.go +++ b/pkg/bsql/provisioning_validation_idempotency_test.go @@ -5,6 +5,7 @@ import ( v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" sdkGrant "github.com/conductorone/baton-sdk/pkg/types/grant" + "github.com/conductorone/baton-sql/pkg/database" "github.com/stretchr/testify/require" ) @@ -56,6 +57,8 @@ func userPrincipal(userID string) *v2.Resource { func TestGrant_ValidationNoRowsReportsAlreadyExists(t *testing.T) { s, db := newRevokeProvisioningTestSyncer(t) withValidationQueryConfig(s) + // validation "no rows" only signals idempotency on DDL engines (Db2) + s.dbEngine = database.DB2 // membership already present: the grant validation query returns no rows seedUserWithRoles(t, db, "user-1", "admin") @@ -70,6 +73,22 @@ 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")) } +// 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) { + s, db := newRevokeProvisioningTestSyncer(t) + withValidationQueryConfig(s) + // 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) + + // 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 TestGrant_ValidationRowsAppliesGrant(t *testing.T) { s, db := newRevokeProvisioningTestSyncer(t) withValidationQueryConfig(s) @@ -89,6 +108,8 @@ func TestGrant_ValidationRowsAppliesGrant(t *testing.T) { func TestRevoke_ValidationNoRowsReportsAlreadyRevoked(t *testing.T) { s, _ := newRevokeProvisioningTestSyncer(t) withValidationQueryConfig(s) + // validation "no rows" only signals idempotency on DDL engines (Db2) + s.dbEngine = database.DB2 // nothing seeded: the revoke validation query returns no rows annos, err := s.Revoke(t.Context(), revokeGrantFor("user-1", "admin")) @@ -118,6 +139,8 @@ func TestRevoke_ValidationRowsAppliesRevoke(t *testing.T) { // can detect idempotency with errors.Is. func TestRunProvisioningQueriesWithExecutor_ValidationNoRowsWrapsSentinel(t *testing.T) { s, db := newRevokeProvisioningTestSyncer(t) + // validation "no rows" only signals idempotency on DDL engines (Db2) + s.dbEngine = database.DB2 err := s.RunProvisioningQueriesWithExecutor( t.Context(), diff --git a/pkg/bsql/query.go b/pkg/bsql/query.go index 683279ff..45409cf0 100644 --- a/pkg/bsql/query.go +++ b/pkg/bsql/query.go @@ -602,6 +602,16 @@ func (s *SQLSyncer) runPrincipalExistsCheck( return exists, nil } +// validationNoRowsMeansIdempotent reports whether a validation query returning no +// rows should be treated as "already in the desired state" rather than a failed +// precondition. Only DDL-based engines (Db2) need this: their GRANT/REVOKE statements +// 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 { + return s.dbEngine == database.DB2 +} + func (s *SQLSyncer) RunProvisioningQueriesWithExecutor( ctx context.Context, queries, @@ -641,9 +651,10 @@ func (s *SQLSyncer) RunProvisioningQueriesWithExecutor( } if !valid { - // 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) + if s.validationNoRowsMeansIdempotent() { + return fmt.Errorf("validation query returned no rows: %w", ErrQueryAffectedZeroRows) + } + return fmt.Errorf("validation query returned no rows") } } @@ -1073,9 +1084,10 @@ func (s *SQLSyncer) RunGrantProvisioning( } if !valid { - // 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) + if s.validationNoRowsMeansIdempotent() { + return anno, fmt.Errorf("grant provisioning: validation query returned no rows: %w", ErrQueryAffectedZeroRows) + } + return anno, fmt.Errorf("grant provisioning: validation query returned no rows") } } From 97654550cca2274d87589e4df3bab4404cbc4392 Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Wed, 2 Sep 2026 15:28:19 -0500 Subject: [PATCH 04/13] CXH-2379: test non-DDL revoke fails loudly on validation no-rows 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. --- pkg/bsql/provisioning_validation_idempotency_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/pkg/bsql/provisioning_validation_idempotency_test.go b/pkg/bsql/provisioning_validation_idempotency_test.go index cc5e09ef..f630b869 100644 --- a/pkg/bsql/provisioning_validation_idempotency_test.go +++ b/pkg/bsql/provisioning_validation_idempotency_test.go @@ -120,6 +120,18 @@ func TestRevoke_ValidationNoRowsReportsAlreadyRevoked(t *testing.T) { 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) { + s, _ := newRevokeProvisioningTestSyncer(t) + withValidationQueryConfig(s) + // 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) +} + func TestRevoke_ValidationRowsAppliesRevoke(t *testing.T) { s, db := newRevokeProvisioningTestSyncer(t) withValidationQueryConfig(s) From 70f62ace9c1713ee799df829fae048dcf19b0bd1 Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Thu, 3 Sep 2026 06:19:20 -0500 Subject: [PATCH 05/13] CXH-2379: address review feedback on validation-query idempotency - Extract shared runValidationQueries helper so the grant and revoke validation loops stop drifting (the copies had diverged on result.Close). - Warn in the ValidationQueries doc comment that DDL-engine authors must not reuse validation_queries as an existence precondition, since a no-rows result is reported as idempotent success and would mask real failures. - Preserve annotations returned by RunGrantProvisioning in the already-exists branch so a GrantReplaced from a committed grant_replace revoke survives. --- pkg/bsql/config.go | 4 +++ pkg/bsql/provisioning.go | 3 ++- pkg/bsql/query.go | 54 ++++++++++++++++------------------------ 3 files changed, 27 insertions(+), 34 deletions(-) diff --git a/pkg/bsql/config.go b/pkg/bsql/config.go index 799eb923..b5383193 100644 --- a/pkg/bsql/config.go +++ b/pkg/bsql/config.go @@ -427,6 +427,10 @@ type EntitlementProvisioningQueries struct { // (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). + // + // Warning: on DDL-based engines, do NOT use these as existence preconditions + // (e.g. "does this user/role exist?"). A no-rows result is reported as idempotent + // success, so a missing or mistyped principal is silently swallowed instead of erroring. ValidationQueries []string `yaml:"validation_queries,omitempty" json:"validation_queries,omitempty"` // Queries is a list of SQL statements to execute for the provisioning operation. diff --git a/pkg/bsql/provisioning.go b/pkg/bsql/provisioning.go index af90d649..fd3516a4 100644 --- a/pkg/bsql/provisioning.go +++ b/pkg/bsql/provisioning.go @@ -88,7 +88,8 @@ func (s *SQLSyncer) Grant(ctx context.Context, principal *v2.Resource, entitleme if err != nil { if errors.Is(err, ErrQueryAffectedZeroRows) { l.Debug("entitlement is already granted", zap.String("entitlement_id", entitlement.GetId())) - anno := annotations.Annotations{} + // Reuse the returned annotations so a GrantReplaced from an already-committed + // grant_replace revoke survives; a fresh set would drop it. anno.Update(&v2.GrantAlreadyExists{}) return anno, nil } diff --git a/pkg/bsql/query.go b/pkg/bsql/query.go index 45409cf0..c369d7ca 100644 --- a/pkg/bsql/query.go +++ b/pkg/bsql/query.go @@ -612,9 +612,8 @@ func (s *SQLSyncer) validationNoRowsMeansIdempotent() bool { return s.dbEngine == database.DB2 } -func (s *SQLSyncer) RunProvisioningQueriesWithExecutor( +func (s *SQLSyncer) runValidationQueries( ctx context.Context, - queries, validationQueries []string, vars map[string]any, executor executor, @@ -642,11 +641,11 @@ func (s *SQLSyncer) RunProvisioningQueriesWithExecutor( valid := result.Next() if err := result.Err(); err != nil { + _ = result.Close() return fmt.Errorf("failed to read validation query result: %w", err) } - err = result.Close() - if err != nil { + if err := result.Close(); err != nil { return fmt.Errorf("failed to close validation query result: %w", err) } @@ -658,6 +657,22 @@ func (s *SQLSyncer) RunProvisioningQueriesWithExecutor( } } + return nil +} + +func (s *SQLSyncer) RunProvisioningQueriesWithExecutor( + ctx context.Context, + queries, + validationQueries []string, + vars map[string]any, + executor executor, +) error { + l := ctxzap.Extract(ctx) + + if err := s.runValidationQueries(ctx, validationQueries, vars, executor); err != nil { + return err + } + zeroRowCount := 0 for idx, q := range queries { @@ -1060,35 +1075,8 @@ func (s *SQLSyncer) RunGrantProvisioning( } } - for _, q := range validationQueries { - q, qArgs, err := s.prepareProvisioningQuery(q, vars) - if err != nil { - return anno, fmt.Errorf("failed to prepare validation query: %w", err) - } - - result, err := executor.QueryContext(ctx, q, qArgs...) - if err != nil { - return anno, fmt.Errorf("failed to execute validation query: %w", err) - } - - valid := result.Next() - - if err := result.Err(); err != nil { - _ = result.Close() - return anno, fmt.Errorf("failed to read validation query result: %w", err) - } - - err = result.Close() - if err != nil { - return anno, fmt.Errorf("failed to close validation query result: %w", err) - } - - if !valid { - if s.validationNoRowsMeansIdempotent() { - return anno, fmt.Errorf("grant provisioning: validation query returned no rows: %w", ErrQueryAffectedZeroRows) - } - return anno, fmt.Errorf("grant provisioning: validation query returned no rows") - } + if err := s.runValidationQueries(ctx, validationQueries, vars, executor); err != nil { + return anno, err } zeroRowCount := 0 From 5024f30a2740deae6f5b60749172cdaa026437f1 Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Thu, 3 Sep 2026 14:39:22 -0500 Subject: [PATCH 06/13] CXH-2379: don't report GrantReplaced when the grant tx rolled back On the transactional grant path, RunGrantProvisioning returns the zero-rows sentinel before commit, so the deferred rollback undoes any grant_replace revoke. Grant reused those annotations, reporting GrantReplaced for a removal the database no longer reflects. Keep the returned annotations only on the no_transaction path, where the replace already committed; otherwise return a fresh GrantAlreadyExists. Adds regression tests for both the rolled-back (no GrantReplaced, old grant survives) and committed (GrantReplaced, old grant gone) paths. --- pkg/bsql/provisioning.go | 13 +- pkg/bsql/provisioning_grant_replace_test.go | 128 ++++++++++++++++++++ 2 files changed, 137 insertions(+), 4 deletions(-) create mode 100644 pkg/bsql/provisioning_grant_replace_test.go diff --git a/pkg/bsql/provisioning.go b/pkg/bsql/provisioning.go index fd3516a4..91fcedad 100644 --- a/pkg/bsql/provisioning.go +++ b/pkg/bsql/provisioning.go @@ -88,10 +88,15 @@ func (s *SQLSyncer) Grant(ctx context.Context, principal *v2.Resource, entitleme if err != nil { if errors.Is(err, ErrQueryAffectedZeroRows) { l.Debug("entitlement is already granted", zap.String("entitlement_id", entitlement.GetId())) - // Reuse the returned annotations so a GrantReplaced from an already-committed - // grant_replace revoke survives; a fresh set would drop it. - anno.Update(&v2.GrantAlreadyExists{}) - return anno, nil + // On the transactional path the zero-rows return rolls the tx back, undoing any + // grant_replace revoke, so a reused GrantReplaced would misreport a removal the DB + // no longer reflects. Keep the returned annotations only on the no_transaction path, + // where the replace already committed. + if provisioningConfig.Grant.NoTransaction { + anno.Update(&v2.GrantAlreadyExists{}) + return anno, nil + } + return annotations.New(&v2.GrantAlreadyExists{}), nil } return nil, err } diff --git a/pkg/bsql/provisioning_grant_replace_test.go b/pkg/bsql/provisioning_grant_replace_test.go new file mode 100644 index 00000000..b833acb5 --- /dev/null +++ b/pkg/bsql/provisioning_grant_replace_test.go @@ -0,0 +1,128 @@ +package bsql + +import ( + "database/sql" + "testing" + + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sql/pkg/bcel" + "github.com/conductorone/baton-sql/pkg/database" + "github.com/stretchr/testify/require" + _ "modernc.org/sqlite" +) + +// withGrantReplaceConfig wires a "member" entitlement whose grant replaces the +// principal's existing role: the grant_replace query finds the old membership and +// revokes it, then the main grant runs. The main grant uses INSERT OR IGNORE so a +// pre-existing target row makes it affect zero rows (the already-granted path). +func withGrantReplaceConfig(s *SQLSyncer, noTransaction bool) { + s.resourceType = &v2.ResourceType{Id: "role"} + s.config = ResourceType{ + StaticEntitlements: []*EntitlementMapping{ + { + Id: "member", + Provisioning: &EntitlementProvisioning{ + Vars: map[string]string{ + "user_id": "principal.ID", + "role": "resource.ID", + }, + Grant: &GrantEntitlementProvisioningQueries{ + EntitlementProvisioningQueries: EntitlementProvisioningQueries{ + NoTransaction: noTransaction, + Queries: []string{`INSERT OR IGNORE INTO user_roles (user_id, role) VALUES (?, ?)`}, + }, + GrantReplace: &GrantReplaceProvisioningQueries{ + Query: `SELECT user_id, role FROM user_roles WHERE user_id = ? AND role = 'viewer'`, + Map: []*GrantMapping{ + { + EntitlementResourceId: ".role", + PrincipalId: ".user_id", + PrincipalType: "user", + Entitlement: "member", + }, + }, + }, + }, + Revoke: &RevokeEntitlementProvisioningQueries{ + EntitlementProvisioningQueries: EntitlementProvisioningQueries{ + Queries: []string{`DELETE FROM user_roles WHERE user_id = ? AND role = ?`}, + }, + }, + }, + }, + }, + } +} + +func newGrantReplaceTestSyncer(t *testing.T) (*SQLSyncer, *sql.DB) { + t.Helper() + + db, err := sql.Open("sqlite", ":memory:") + require.NoError(t, err) + db.SetMaxOpenConns(1) + t.Cleanup(func() { require.NoError(t, db.Close()) }) + + _, err = db.ExecContext(t.Context(), `CREATE TABLE user_roles (user_id TEXT, role TEXT, UNIQUE(user_id, role))`) + require.NoError(t, err) + + env, err := bcel.NewEnv(t.Context()) + require.NoError(t, err) + + return &SQLSyncer{ + db: db, + dbs: map[string]*sql.DB{"primary": db}, + dbNames: []string{"primary"}, + primaryDBName: "primary", + currentDBName: "primary", + dbEngine: database.SQLite, + env: env, + }, db +} + +// Transactional path: the target grant already exists, so the main grant hits the +// zero-rows sentinel and the tx rolls back, undoing the grant_replace revoke. The +// response must NOT claim GrantReplaced, and the old row must survive. +func TestGrant_ReplaceRolledBackDoesNotReportGrantReplaced(t *testing.T) { + s, db := newGrantReplaceTestSyncer(t) + withGrantReplaceConfig(s, false) // transactional + _, err := db.ExecContext(t.Context(), `INSERT INTO user_roles (user_id, role) VALUES ('user-1','viewer'), ('user-1','admin')`) + require.NoError(t, err) + + annos, err := s.Grant(t.Context(), userPrincipal("user-1"), memberEntitlementFor("admin")) + require.NoError(t, err) + + exists, err := annos.Pick(&v2.GrantAlreadyExists{}) + require.NoError(t, err) + require.True(t, exists) + + replaced, err := annos.Pick(&v2.GrantReplaced{}) + require.NoError(t, err) + require.False(t, replaced, "GrantReplaced must not be reported when the tx rolled back") + + // the replace revoke was rolled back, so the old membership survives + require.Equal(t, 1, countRows(t, db, `SELECT COUNT(*) FROM user_roles WHERE user_id = ? AND role = ?`, "user-1", "viewer")) +} + +// no_transaction path: the grant_replace revoke commits immediately, so even when the +// main grant hits the zero-rows sentinel the removal really happened and GrantReplaced +// must be reported. +func TestGrant_ReplaceCommittedReportsGrantReplaced(t *testing.T) { + s, db := newGrantReplaceTestSyncer(t) + withGrantReplaceConfig(s, true) // no_transaction + _, err := db.ExecContext(t.Context(), `INSERT INTO user_roles (user_id, role) VALUES ('user-1','viewer'), ('user-1','admin')`) + require.NoError(t, err) + + annos, err := s.Grant(t.Context(), userPrincipal("user-1"), memberEntitlementFor("admin")) + require.NoError(t, err) + + exists, err := annos.Pick(&v2.GrantAlreadyExists{}) + require.NoError(t, err) + require.True(t, exists) + + replaced, err := annos.Pick(&v2.GrantReplaced{}) + require.NoError(t, err) + require.True(t, replaced, "GrantReplaced must be reported when the replace committed") + + // the replace revoke committed, so the old membership is gone + require.Equal(t, 0, countRows(t, db, `SELECT COUNT(*) FROM user_roles WHERE user_id = ? AND role = ?`, "user-1", "viewer")) +} From 12a64884fda5fc055d8c1f142373b272235c166f Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Thu, 3 Sep 2026 15:35:13 -0500 Subject: [PATCH 07/13] CXH-2379: document DB2 grant_replace no-rows semantics and add coverage On Db2 a grant_replace revoke whose validation query returns no rows swallows ErrQueryAffectedZeroRows and still reports GrantReplaced: the old grant is already gone, which is the state a replace aims for. Document this at the guard, cover it with a DB2 test, and add a validation_queries section to docs/db2.md warning against using them as existence preconditions on Db2. --- docs/db2.md | 29 ++++++++++++++++++ pkg/bsql/provisioning_grant_replace_test.go | 34 +++++++++++++++++++++ pkg/bsql/query.go | 3 ++ 3 files changed, 66 insertions(+) diff --git a/docs/db2.md b/docs/db2.md index 6922a829..56f4e6af 100644 --- a/docs/db2.md +++ b/docs/db2.md @@ -140,6 +140,35 @@ the OS libxml2 package: `apt-get install libxml2` / `yum install libxml2`. **`go vet` / `golangci-lint` with `-tags db2` fails** — type-checking the tagged path needs the clidriver headers too. Default-tag lint and vet need nothing. +## Provisioning: `validation_queries` semantics on Db2 + +Db2 uses DDL-based `GRANT`/`REVOKE` statements that do not report rows-affected, so the +connector cannot tell from the statement itself whether it changed anything. To make grant +and revoke idempotent, Db2 gives `validation_queries` a different meaning than every other +(pure-Go) engine: + +- **Other engines:** a `validation_query` returning no rows fails the operation. It is an + existence precondition that aborts loudly. +- **Db2:** a `validation_query` returning no rows is reported as an **idempotent success** + (`GrantAlreadyExists` on grant, `GrantAlreadyRevoked` on revoke). No rows means "the state + is already as desired, there is no work to do". + +Because of this, on Db2 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. + +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`. + ## Docker - The default release pipeline (goreleaser, `CGO_ENABLED=0`) is unaffected — DB2 does not diff --git a/pkg/bsql/provisioning_grant_replace_test.go b/pkg/bsql/provisioning_grant_replace_test.go index b833acb5..a919965b 100644 --- a/pkg/bsql/provisioning_grant_replace_test.go +++ b/pkg/bsql/provisioning_grant_replace_test.go @@ -126,3 +126,37 @@ func TestGrant_ReplaceCommittedReportsGrantReplaced(t *testing.T) { // the replace revoke committed, so the old membership is gone require.Equal(t, 0, countRows(t, db, `SELECT COUNT(*) FROM user_roles WHERE user_id = ? AND role = ?`, "user-1", "viewer")) } + +// withGrantReplaceDB2Config is the grant_replace config with a revoke validation +// query that never matches. On Db2 a no-rows validation means "nothing to revoke", +// so the revoke aborts before its DELETE runs but the flow still reports GrantReplaced. +func withGrantReplaceDB2Config(s *SQLSyncer) { + withGrantReplaceConfig(s, true) // no_transaction: the replace stands on its own + revoke := s.config.StaticEntitlements[0].Provisioning.Revoke + revoke.ValidationQueries = []string{ + `SELECT 1 FROM user_roles WHERE user_id = ? AND role = 'does-not-exist'`, + } +} + +// Db2 path: the revoke validation query returns no rows, so the revoke DELETE never +// runs, yet GrantReplaced is still reported because on Db2 a no-rows validation means +// the old grant is already gone. The old viewer row must survive (revoke never ran). +func TestGrant_ReplaceDB2RevokeValidationNoRowsStillReportsGrantReplaced(t *testing.T) { + s, db := newGrantReplaceTestSyncer(t) + s.dbEngine = database.DB2 + withGrantReplaceDB2Config(s) + _, err := db.ExecContext(t.Context(), `INSERT INTO user_roles (user_id, role) VALUES ('user-1','viewer')`) + require.NoError(t, err) + + annos, err := s.Grant(t.Context(), userPrincipal("user-1"), memberEntitlementFor("admin")) + require.NoError(t, err) + + replaced, err := annos.Pick(&v2.GrantReplaced{}) + require.NoError(t, err) + require.True(t, replaced, "GrantReplaced must be reported: on Db2 a no-rows revoke validation means the old grant is already gone") + + // the revoke validation aborted the revoke before its DELETE ran, so viewer survives + require.Equal(t, 1, countRows(t, db, `SELECT COUNT(*) FROM user_roles WHERE user_id = ? AND role = ?`, "user-1", "viewer")) + // the main grant still ran + require.Equal(t, 1, countRows(t, db, `SELECT COUNT(*) FROM user_roles WHERE user_id = ? AND role = ?`, "user-1", "admin")) +} diff --git a/pkg/bsql/query.go b/pkg/bsql/query.go index c369d7ca..366401ba 100644 --- a/pkg/bsql/query.go +++ b/pkg/bsql/query.go @@ -1062,6 +1062,9 @@ func (s *SQLSyncer) RunGrantProvisioning( executor, ) if err != nil { + // On DDL engines (Db2) a zero-rows revoke means "nothing to revoke": the + // old grant is already gone, which is the state a replace aims for. Treat + // that as success and still report GrantReplaced. Any other error aborts. if !errors.Is(err, ErrQueryAffectedZeroRows) { return anno, err } From a439489e96d9a79ac561123f7cdf39d0f804a2d2 Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Thu, 3 Sep 2026 16:00:50 -0500 Subject: [PATCH 08/13] CXH-2379: extend DDL grant/revoke idempotency to Oracle Oracle GRANT/REVOKE are DDL like Db2: an already-applied REVOKE raises ORA-01951 (and re-GRANT succeeds without affecting rows), so the validation query is the only zero-effect signal. Add Oracle to validationNoRowsMeansIdempotent() so validation 'no rows' maps to GrantAlreadyExists / GrantAlreadyRevoked instead of failing the task. Verified live against Oracle XE 21c (grant/re-grant -> GrantAlreadyExists, revoke/re-revoke -> GrantAlreadyRevoked, no ORA-01951, revoke DDL skipped). Adds an engine-gate regression test and updates the ValidationQueries doc. --- pkg/bsql/config.go | 2 +- ...ning_validation_idempotency_oracle_test.go | 35 +++++++++++++++++++ pkg/bsql/query.go | 15 +++++--- 3 files changed, 46 insertions(+), 6 deletions(-) create mode 100644 pkg/bsql/provisioning_validation_idempotency_oracle_test.go diff --git a/pkg/bsql/config.go b/pkg/bsql/config.go index b5383193..9e3999e1 100644 --- a/pkg/bsql/config.go +++ b/pkg/bsql/config.go @@ -424,7 +424,7 @@ type EntitlementProvisioningQueries struct { // 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, + // (an existence precondition). On DDL-based engines (Db2, Oracle) that don't report rows-affected, // a query returning no rows instead means the state is already as desired, so the operation // is reported as an idempotent success (GrantAlreadyExists / GrantAlreadyRevoked). // diff --git a/pkg/bsql/provisioning_validation_idempotency_oracle_test.go b/pkg/bsql/provisioning_validation_idempotency_oracle_test.go new file mode 100644 index 00000000..1455d3d6 --- /dev/null +++ b/pkg/bsql/provisioning_validation_idempotency_oracle_test.go @@ -0,0 +1,35 @@ +package bsql + +import ( + "testing" + + "github.com/conductorone/baton-sql/pkg/database" + "github.com/stretchr/testify/require" +) + +// validationNoRowsMeansIdempotent is the DDL-engine gate: it must be true only for +// engines whose already-applied GRANT/REVOKE raises an error instead of affecting rows, +// so validation "no rows" means idempotency rather than a failed precondition. +// +// The behavioral grant/revoke wiring is covered by the Db2 tests in +// provisioning_validation_idempotency_test.go; Oracle can't reuse them because the +// Oracle driver rewrites ? placeholders to bind syntax the sqlite test backend +// rejects. Oracle's end-to-end behavior was verified live against Oracle XE 21c on +// 2026-09-03 (grant/re-grant -> GrantAlreadyExists, revoke/re-revoke -> GrantAlreadyRevoked, +// ORA-01951 no longer surfaced). +func TestValidationNoRowsMeansIdempotent_EngineGate(t *testing.T) { + ddl := map[database.DbEngine]bool{ + database.DB2: true, + database.Oracle: true, + database.SQLite: false, + database.MySQL: false, + database.PostgreSQL: false, + database.MSSQL: false, + database.HDB: false, + database.Vertica: false, + } + for engine, want := range ddl { + s := &SQLSyncer{dbEngine: engine} + require.Equal(t, want, s.validationNoRowsMeansIdempotent(), "engine=%v", engine) + } +} diff --git a/pkg/bsql/query.go b/pkg/bsql/query.go index 366401ba..ec8be7da 100644 --- a/pkg/bsql/query.go +++ b/pkg/bsql/query.go @@ -604,12 +604,17 @@ func (s *SQLSyncer) runPrincipalExistsCheck( // validationNoRowsMeansIdempotent reports whether a validation query returning no // rows should be treated as "already in the desired state" rather than a failed -// precondition. Only DDL-based engines (Db2) need this: their GRANT/REVOKE statements -// 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. +// precondition. DDL-based engines (Db2, Oracle) need this: an already-applied GRANT or +// REVOKE raises an error (Db2 SQL0556N, Oracle ORA-01951) instead of affecting rows, 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 { - return s.dbEngine == database.DB2 + switch s.dbEngine { + case database.DB2, database.Oracle: + return true + default: + return false + } } func (s *SQLSyncer) runValidationQueries( From cf1847a47567d0bdc15661d47d4bee19b5b2efec Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Thu, 3 Sep 2026 16:10:47 -0500 Subject: [PATCH 09/13] CXH-2379: exit with gRPC status code on error via exit.LogExit Replace os.Exit(1) in main.go with exit.LogExit(err) so an auth failure exits with the mapped gRPC status code instead of a bare 1, letting the CI sync-test auth-error check actually assert. --- cmd/baton-sql/main.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/cmd/baton-sql/main.go b/cmd/baton-sql/main.go index 5fa963d5..90245d3d 100644 --- a/cmd/baton-sql/main.go +++ b/cmd/baton-sql/main.go @@ -2,11 +2,10 @@ package main import ( "context" - "fmt" - "os" configSdk "github.com/conductorone/baton-sdk/pkg/config" "github.com/conductorone/baton-sdk/pkg/connectorbuilder" + "github.com/conductorone/baton-sdk/pkg/exit" "github.com/conductorone/baton-sdk/pkg/field" "github.com/conductorone/baton-sdk/pkg/types" "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" @@ -31,16 +30,14 @@ func main() { }, ) if err != nil { - fmt.Fprintln(os.Stderr, err.Error()) - os.Exit(1) + exit.LogExit(err) } cmd.Version = version err = cmd.Execute() if err != nil { - fmt.Fprintln(os.Stderr, err.Error()) - os.Exit(1) + exit.LogExit(err) } } From f3823b7cd819d4a2c0580f481e9ec0970492da1b Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Thu, 3 Sep 2026 16:11:47 -0500 Subject: [PATCH 10/13] CXH-2379: document DDL validation_queries semantics for Oracle Address PR review on the Oracle idempotency change: - Fix the validationNoRowsMeansIdempotent doc comment. Re-GRANT on Oracle succeeds silently; ORA-01951 is a REVOKE-only error, so the old wording claiming a repeat GRANT raises ORA-01951 was wrong. - Add docs/provisioning.md, an engine-neutral home for the no-rows-means- idempotent behavior covering both DDL engines (Db2 and Oracle), so Oracle operators can find the existence-precondition warning. - Trim the Db2-scoped section in docs/db2.md to point at the shared doc and drop the now-stale "different meaning than every other (pure-Go) engine" claim, since Oracle (also pure-Go) now shares the behavior. --- docs/db2.md | 36 ++++++++---------------------------- docs/provisioning.md | 36 ++++++++++++++++++++++++++++++++++++ pkg/bsql/query.go | 7 ++++--- 3 files changed, 48 insertions(+), 31 deletions(-) create mode 100644 docs/provisioning.md diff --git a/docs/db2.md b/docs/db2.md index 56f4e6af..91986c2f 100644 --- a/docs/db2.md +++ b/docs/db2.md @@ -140,34 +140,14 @@ the OS libxml2 package: `apt-get install libxml2` / `yum install libxml2`. **`go vet` / `golangci-lint` with `-tags db2` fails** — type-checking the tagged path needs the clidriver headers too. Default-tag lint and vet need nothing. -## Provisioning: `validation_queries` semantics on Db2 - -Db2 uses DDL-based `GRANT`/`REVOKE` statements that do not report rows-affected, so the -connector cannot tell from the statement itself whether it changed anything. To make grant -and revoke idempotent, Db2 gives `validation_queries` a different meaning than every other -(pure-Go) engine: - -- **Other engines:** a `validation_query` returning no rows fails the operation. It is an - existence precondition that aborts loudly. -- **Db2:** a `validation_query` returning no rows is reported as an **idempotent success** - (`GrantAlreadyExists` on grant, `GrantAlreadyRevoked` on revoke). No rows means "the state - is already as desired, there is no work to do". - -Because of this, on Db2 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. - -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`. +## Provisioning: `validation_queries` semantics + +Db2 is DDL-based: its `GRANT`/`REVOKE` don't report rows-affected, so a `validation_query` +returning no rows is treated as an idempotent success, not a failed precondition. This is +the shared behavior of every DDL-based engine (Db2 and Oracle), and it means you must not +use `validation_queries` as existence preconditions on Db2. See +[Provisioning: `validation_queries` semantics](provisioning.md) for the full explanation and +examples. ## Docker diff --git a/docs/provisioning.md b/docs/provisioning.md new file mode 100644 index 00000000..de524a67 --- /dev/null +++ b/docs/provisioning.md @@ -0,0 +1,36 @@ +# Provisioning: `validation_queries` semantics + +`validation_queries` run before the provisioning `queries` in a grant or revoke. What a +**no-rows** result means depends on the engine. + +## Engines that report rows-affected + +On engines whose `GRANT`/`REVOKE` report how many rows they changed (SQLite, MySQL, +PostgreSQL, SQL Server, HANA, Vertica), a `validation_query` returning no rows **fails the +operation**. It is an existence precondition that aborts loudly. + +## DDL-based engines (Db2, Oracle) + +Db2 and Oracle apply `GRANT`/`REVOKE` as DDL that does not report rows-affected, so the +connector cannot tell from the statement itself whether it changed anything. On Oracle a +repeat `GRANT` succeeds without changing anything and an already-applied `REVOKE` raises +`ORA-01951`; on Db2 an already-applied statement raises an error. To make grant and revoke +idempotent, on these engines a `validation_query` returning no rows is reported as an +**idempotent success** (`GrantAlreadyExists` on grant, `GrantAlreadyRevoked` on revoke). No +rows means "the state is already as desired, there is no work to do". + +Because of this, on Db2 and Oracle your `validation_queries` must answer **"is there work to +do?"**, not **"does this principal or role exist?"**. + +**Do not use `validation_queries` as existence preconditions on Db2 or Oracle.** A no-rows +result is swallowed as idempotent success, so a missing, deleted, or mistyped principal or +role is reported as "already done" instead of erroring. For example, a validation query like +`SELECT 1 FROM users WHERE name = ?` will silently mask a bad `user_id`: it returns +no rows, and the grant is reported as `GrantAlreadyExists` even though nothing was granted. + +Write the query so no-rows genuinely means idempotent. For a grant, check whether the target +membership is **missing** (no rows => already granted); for a revoke, check whether it is +**present** (no rows => already revoked). + +This mirrors the warning on `EntitlementProvisioningQueries.ValidationQueries` in +`pkg/bsql/config.go`. diff --git a/pkg/bsql/query.go b/pkg/bsql/query.go index ec8be7da..a97b65e6 100644 --- a/pkg/bsql/query.go +++ b/pkg/bsql/query.go @@ -604,9 +604,10 @@ func (s *SQLSyncer) runPrincipalExistsCheck( // validationNoRowsMeansIdempotent reports whether a validation query returning no // rows should be treated as "already in the desired state" rather than a failed -// precondition. DDL-based engines (Db2, Oracle) need this: an already-applied GRANT or -// REVOKE raises an error (Db2 SQL0556N, Oracle ORA-01951) instead of affecting rows, so -// the validation query is the only zero-effect signal available. Engines that report +// precondition. DDL-based engines (Db2, Oracle) need this: their GRANT/REVOKE don't +// report rows-affected, so a repeat statement looks identical to a fresh one and the +// validation query is the only zero-effect signal. (On Oracle a repeat GRANT succeeds +// silently while an already-applied REVOKE raises ORA-01951.) Engines that report // rows-affected keep using validation queries as existence preconditions that fail loudly. func (s *SQLSyncer) validationNoRowsMeansIdempotent() bool { switch s.dbEngine { From 7cf7eb01dd50a5c577baa80273e63412ce250f8a Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Thu, 3 Sep 2026 16:39:28 -0500 Subject: [PATCH 11/13] CXH-2379: map DB auth failures to Unauthenticated in Validate Pairs with the exit.LogExit change: Validate wrapped the ping error plainly, so exit mapped auth failures to Unknown(2). database.AuthError maps SQLSTATE class 28 (Postgres/Redshift/Vertica/etc.) and MySQL 1045 to codes.Unauthenticated. --- pkg/connector/connector.go | 3 +++ pkg/database/autherror.go | 35 +++++++++++++++++++++++++++ pkg/database/autherror_test.go | 44 ++++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+) create mode 100644 pkg/database/autherror.go create mode 100644 pkg/database/autherror_test.go diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index cb69ba02..eabbf346 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -97,6 +97,9 @@ func (c *Connector) Validate(ctx context.Context) (annotations.Annotations, erro for name, db := range c.dbs { if err := db.PingContext(ctx); err != nil { + if authErr := database.AuthError(err); authErr != nil { + return nil, authErr + } return nil, fmt.Errorf("database %q ping failed: %w", name, err) } } diff --git a/pkg/database/autherror.go b/pkg/database/autherror.go new file mode 100644 index 00000000..96593707 --- /dev/null +++ b/pkg/database/autherror.go @@ -0,0 +1,35 @@ +package database + +import ( + "errors" + "strings" + + "github.com/go-sql-driver/mysql" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +const mysqlAccessDenied = 1045 + +// AuthError returns an Unauthenticated gRPC status when err is a database +// authentication/authorization failure, or nil otherwise. SQLSTATE class 28 +// ("invalid authorization") is the ANSI code drivers report on bad credentials +// (Postgres/Redshift/Vertica/etc. surface it via SQLState()); MySQL is the +// exception, reporting error 1045 with no SQLSTATE. +func AuthError(err error) error { + if err == nil { + return nil + } + + var sqlState interface{ SQLState() string } + if errors.As(err, &sqlState) && strings.HasPrefix(sqlState.SQLState(), "28") { + return status.Error(codes.Unauthenticated, "database authentication failed") + } + + var myErr *mysql.MySQLError + if errors.As(err, &myErr) && myErr.Number == mysqlAccessDenied { + return status.Error(codes.Unauthenticated, "database authentication failed") + } + + return nil +} diff --git a/pkg/database/autherror_test.go b/pkg/database/autherror_test.go new file mode 100644 index 00000000..3e1153cc --- /dev/null +++ b/pkg/database/autherror_test.go @@ -0,0 +1,44 @@ +package database + +import ( + "errors" + "fmt" + "testing" + + "github.com/go-sql-driver/mysql" + "github.com/jackc/pgx/v5/pgconn" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +func TestAuthError(t *testing.T) { + tests := []struct { + name string + err error + want codes.Code // codes.OK means expect nil + }{ + {"nil", nil, codes.OK}, + {"postgres invalid_password 28P01", &pgconn.PgError{Code: "28P01"}, codes.Unauthenticated}, + {"postgres invalid_authorization 28000", &pgconn.PgError{Code: "28000"}, codes.Unauthenticated}, + {"postgres non-auth relation missing 42P01", &pgconn.PgError{Code: "42P01"}, codes.OK}, + {"postgres auth error wrapped", fmt.Errorf("ping: %w", &pgconn.PgError{Code: "28P01"}), codes.Unauthenticated}, + {"mysql access denied 1045", &mysql.MySQLError{Number: 1045}, codes.Unauthenticated}, + {"mysql other 1146", &mysql.MySQLError{Number: 1146}, codes.OK}, + {"plain error", errors.New("boom"), codes.OK}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := AuthError(tt.err) + if tt.want == codes.OK { + if got != nil { + t.Fatalf("want nil, got %v", got) + } + return + } + if status.Code(got) != tt.want { + t.Fatalf("want %v, got %v", tt.want, status.Code(got)) + } + }) + } +} From 9e82a69f1c10a59ea5c9c417f97e9c379107119e Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Thu, 3 Sep 2026 16:50:25 -0500 Subject: [PATCH 12/13] CXH-2379: skip principal-exists probe on validation-sourced no-rows revoke On a DDL engine a revoke whose validation_queries return no rows short- circuits before any revoke runs. RunRevokeProvisioning still ran the principal_exists_check probe, so a mistyped principal_id (validation and probe both empty) falsely reported a still-present principal as deleted, contradicting the PrincipalExistsCheck contract. Add a distinct ErrValidationNoRows sentinel (wrapping ErrQueryAffectedZeroRows so idempotency reporting is unchanged) and skip the exists probe when the zero-rows result came from validation rather than the revoke queries running. Also: reword the grant_replace zero-rows comment to cover both sentinel sources (not just DDL), include the failing query in the loud validation error, and link docs/provisioning.md from README. Adds a regression test. --- README.md | 2 +- pkg/bsql/provisioning_revoke_deleted_test.go | 21 +++++++++ pkg/bsql/query.go | 49 +++++++++++++------- 3 files changed, 54 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index 92a32f40..1284d411 100644 --- a/README.md +++ b/README.md @@ -35,7 +35,7 @@ The connector is configured using a YAML file that defines: - **Resource Types**: Map database tables/queries to resources (users, roles, etc.) - **Account Provisioning**: Define schemas and credential options for user creation - **Entitlements**: Permissions and roles that can be granted to resources -- **Provisioning Actions**: SQL queries for granting/revoking entitlements +- **Provisioning Actions**: SQL queries for granting/revoking entitlements; see [docs/provisioning.md](docs/provisioning.md) for `validation_queries` semantics (including the DDL-engine no-rows-means-idempotent behavior on Db2 and Oracle) For Postgres behind a transaction-mode pooler (PgBouncer, Supabase pooler on port 6543, etc.), set `default_query_exec_mode` to `simple_protocol` via the DSN query string or `connect.params` to avoid prepared-statement conflicts (SQLSTATE 42P05). When unset, baton-sql leaves the URL unchanged and pgx uses its default (`cache_statement`). diff --git a/pkg/bsql/provisioning_revoke_deleted_test.go b/pkg/bsql/provisioning_revoke_deleted_test.go index 0dbf5639..194f99d7 100644 --- a/pkg/bsql/provisioning_revoke_deleted_test.go +++ b/pkg/bsql/provisioning_revoke_deleted_test.go @@ -144,6 +144,27 @@ func TestRunRevokeProvisioning_AllZeroRowsWithSurvivingPrincipal(t *testing.T) { require.Equal(t, 1, countRows(t, db, `SELECT COUNT(*) FROM users WHERE id = ?`, "user-1")) } +// On a DDL engine, a revoke whose validation query returns no rows short-circuits +// before any revoke runs. The principal-exists probe must be skipped: otherwise a +// mistyped principal_id (validation AND probe both empty) would falsely report the +// still-present principal as deleted. +func TestRunRevokeProvisioning_DDLValidationNoRowsSkipsExistsCheck(t *testing.T) { + s, _ := newRevokeProvisioningTestSyncer(t) + s.dbEngine = database.DB2 + // nothing seeded: the revoke validation query returns no rows, and the exists-check + // would also return no rows for user-1 — but no revoke ran, so no deletion happened. + deleted, err := s.RunRevokeProvisioning( + t.Context(), + []string{`DELETE FROM user_roles WHERE user_id = ? AND role = ?`}, + []string{`SELECT 1 FROM user_roles WHERE user_id = ? AND role = ?`}, + principalExistsCheck(), + map[string]any{"principal_id": "user-1", "role": "admin"}, + true, + ) + require.ErrorIs(t, err, ErrQueryAffectedZeroRows) + require.False(t, deleted, "exists-check must be skipped when the sentinel came from validation") +} + func TestRunRevokeProvisioning_NoExistsCheckBehavesLikeBefore(t *testing.T) { s, db := newRevokeProvisioningTestSyncer(t) seedUserWithRoles(t, db, "user-1", "admin") diff --git a/pkg/bsql/query.go b/pkg/bsql/query.go index a97b65e6..5943325b 100644 --- a/pkg/bsql/query.go +++ b/pkg/bsql/query.go @@ -36,6 +36,13 @@ const ( var ErrQueryAffectedZeroRows = errors.New("query affected 0 rows, ending and rolling back") var ErrQueryAffectedMoreThanOneRow = errors.New("query affected more than one row, ending and rolling back") +// ErrValidationNoRows means a validation query returned no rows on a DDL engine (see +// validationNoRowsMeansIdempotent). It wraps ErrQueryAffectedZeroRows so idempotency +// reporting still fires, but stays distinct so the revoke path can tell it apart from the +// revoke queries themselves affecting zero rows: no revoke ran, so the principal-exists +// probe must be skipped rather than reporting a spurious deletion. +var ErrValidationNoRows = fmt.Errorf("validation query returned no rows: %w", ErrQueryAffectedZeroRows) + const defaultGrantCancelledReason = "Grant cancelled by connector policy." type executor interface { @@ -477,13 +484,16 @@ func (s *SQLSyncer) RunRevokeProvisioning( return false, err } - allZero, err := s.runRevokeQueries(ctx, queries, validationQueries, vars, useTx, target) + allZero, fromValidation, err := s.runRevokeQueries(ctx, queries, validationQueries, vars, useTx, target) if err != nil { return false, err } var principalDeleted bool - if existsCheck != nil { + // Skip the probe when the zero-rows came from a validation query (DDL engines): no + // revoke ran, so a no-rows exists-check would falsely report the principal deleted + // "as a side effect of the revoke" when it may still be present. + if existsCheck != nil && !fromValidation { exists, err := s.runPrincipalExistsCheck(ctx, target, existsCheck, vars) if err != nil { l.Warn( @@ -505,9 +515,12 @@ func (s *SQLSyncer) RunRevokeProvisioning( } // runRevokeQueries executes the revoke queries against target, committing when -// useTx is set. It reports whether every query affected zero rows, which means -// the grant was already revoked; that case commits rather than failing so the -// caller can still probe the principal and annotate the response. +// useTx is set. It reports whether every query affected zero rows (allZero, the +// already-revoked case) and whether that zero-rows result came from a validation query +// rather than the revoke queries executing (fromValidation): on a DDL engine a no-rows +// validation short-circuits before any revoke runs, so the caller must skip the +// principal-exists probe. The already-revoked case commits rather than failing so the +// caller can still annotate the response. func (s *SQLSyncer) runRevokeQueries( ctx context.Context, queries, @@ -515,7 +528,7 @@ func (s *SQLSyncer) runRevokeQueries( vars map[string]any, useTx bool, target *sql.DB, -) (bool, error) { +) (bool, bool, error) { l := ctxzap.Extract(ctx) var committed bool @@ -524,7 +537,7 @@ func (s *SQLSyncer) runRevokeQueries( if useTx { tx, err := target.BeginTx(ctx, nil) if err != nil { - return false, err + return false, false, err } executor = tx @@ -537,27 +550,28 @@ func (s *SQLSyncer) runRevokeQueries( }() } - var allZero bool + var allZero, fromValidation bool err := s.RunProvisioningQueriesWithExecutor(ctx, queries, validationQueries, vars, executor) if err != nil { if !errors.Is(err, ErrQueryAffectedZeroRows) { - return false, err + return false, false, err } allZero = true + fromValidation = errors.Is(err, ErrValidationNoRows) } if useTx { tx, ok := executor.(*sql.Tx) if !ok { - return false, errors.New("transactional executor required") + return false, false, errors.New("transactional executor required") } if err := tx.Commit(); err != nil { - return false, err + return false, false, err } committed = true } - return allZero, nil + return allZero, fromValidation, nil } // runPrincipalExistsCheck executes the exists-check probe on the given @@ -657,9 +671,9 @@ func (s *SQLSyncer) runValidationQueries( if !valid { if s.validationNoRowsMeansIdempotent() { - return fmt.Errorf("validation query returned no rows: %w", ErrQueryAffectedZeroRows) + return ErrValidationNoRows } - return fmt.Errorf("validation query returned no rows") + return fmt.Errorf("validation query %q returned no rows", q) } } @@ -1068,9 +1082,10 @@ func (s *SQLSyncer) RunGrantProvisioning( executor, ) if err != nil { - // On DDL engines (Db2) a zero-rows revoke means "nothing to revoke": the - // old grant is already gone, which is the state a replace aims for. Treat - // that as success and still report GrantReplaced. Any other error aborts. + // A zero-rows sentinel means the replace revoke had nothing to remove: either + // its validation query found no rows on a DDL engine, or the revoke queries + // matched nothing on any engine. Either way the old grant is already gone, the + // state a replace aims for, so report GrantReplaced. Any other error aborts. if !errors.Is(err, ErrQueryAffectedZeroRows) { return anno, err } From 8c0917e81e80729b82049aa149af327996df8dad Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Thu, 3 Sep 2026 16:54:06 -0500 Subject: [PATCH 13/13] CXH-2379: include DB name in auth error and document driver coverage AuthError now takes the failing database name so a multi-DB config shows which handle rejected the credentials, and its doc comment records that coverage is limited to SQLState-reporting drivers plus MySQL (Oracle/Db2/ MSSQL/HDB fall through to a generic ping error, not Unauthenticated). --- pkg/connector/connector.go | 2 +- pkg/database/autherror.go | 17 +++++++++++------ pkg/database/autherror_test.go | 2 +- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index eabbf346..0df76af4 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -97,7 +97,7 @@ func (c *Connector) Validate(ctx context.Context) (annotations.Annotations, erro for name, db := range c.dbs { if err := db.PingContext(ctx); err != nil { - if authErr := database.AuthError(err); authErr != nil { + if authErr := database.AuthError(err, name); authErr != nil { return nil, authErr } return nil, fmt.Errorf("database %q ping failed: %w", name, err) diff --git a/pkg/database/autherror.go b/pkg/database/autherror.go index 96593707..e1c67cc4 100644 --- a/pkg/database/autherror.go +++ b/pkg/database/autherror.go @@ -12,23 +12,28 @@ import ( const mysqlAccessDenied = 1045 // AuthError returns an Unauthenticated gRPC status when err is a database -// authentication/authorization failure, or nil otherwise. SQLSTATE class 28 -// ("invalid authorization") is the ANSI code drivers report on bad credentials -// (Postgres/Redshift/Vertica/etc. surface it via SQLState()); MySQL is the +// authentication/authorization failure, or nil otherwise. name identifies the failing +// database so a multi-DB config still shows which handle rejected the credentials. +// SQLSTATE class 28 ("invalid authorization") is the ANSI code drivers report on bad +// credentials (Postgres/Redshift/Vertica/etc. surface it via SQLState()); MySQL is the // exception, reporting error 1045 with no SQLSTATE. -func AuthError(err error) error { +// +// Coverage is limited to drivers that expose SQLState() plus MySQL. Drivers that do not +// (Oracle go-ora, Db2 go_ibm_db, MSSQL, SAP HDB) fall through to nil, so their auth +// failures reach the caller as a generic ping error rather than Unauthenticated. +func AuthError(err error, name string) error { if err == nil { return nil } var sqlState interface{ SQLState() string } if errors.As(err, &sqlState) && strings.HasPrefix(sqlState.SQLState(), "28") { - return status.Error(codes.Unauthenticated, "database authentication failed") + return status.Errorf(codes.Unauthenticated, "database %q authentication failed", name) } var myErr *mysql.MySQLError if errors.As(err, &myErr) && myErr.Number == mysqlAccessDenied { - return status.Error(codes.Unauthenticated, "database authentication failed") + return status.Errorf(codes.Unauthenticated, "database %q authentication failed", name) } return nil diff --git a/pkg/database/autherror_test.go b/pkg/database/autherror_test.go index 3e1153cc..883e3ff0 100644 --- a/pkg/database/autherror_test.go +++ b/pkg/database/autherror_test.go @@ -29,7 +29,7 @@ func TestAuthError(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := AuthError(tt.err) + got := AuthError(tt.err, "testdb") if tt.want == codes.OK { if got != nil { t.Fatalf("want nil, got %v", got)