diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 618c4264..7ca1c94c 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -49,7 +49,7 @@ jobs: baton-principal-type: user bad-credentials: DB_PASSWORD=invalid - 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 diff --git a/README.md b/README.md index a3e00fd7..1858855e 100644 --- a/README.md +++ b/README.md @@ -37,7 +37,7 @@ The connector is configured using a YAML file that defines: - **Resource Types**: Map database tables/queries to resources (users, roles, etc.) - **Account Provisioning**: Define schemas and credential options for user creation - **Entitlements**: Permissions and roles that can be granted to resources -- **Provisioning Actions**: SQL queries for granting/revoking entitlements +- **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/docs/db2.md b/docs/db2.md index 751c3709..6f6b0942 100644 --- a/docs/db2.md +++ b/docs/db2.md @@ -221,6 +221,15 @@ 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 + +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 +[Provisioning: `validation_queries` semantics](provisioning.md) for the full explanation and +examples. + ## Docker - The default release pipeline (goreleaser, `CGO_ENABLED=0`) is unaffected — DB2 does not diff --git a/docs/provisioning.md b/docs/provisioning.md new file mode 100644 index 00000000..46c97ede --- /dev/null +++ b/docs/provisioning.md @@ -0,0 +1,37 @@ +# 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. + +## 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. + +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/config.go b/pkg/bsql/config.go index e5a656d7..b5383193 100644 --- a/pkg/bsql/config.go +++ b/pkg/bsql/config.go @@ -422,7 +422,15 @@ 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). + // + // 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..91fcedad 100644 --- a/pkg/bsql/provisioning.go +++ b/pkg/bsql/provisioning.go @@ -88,9 +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())) - anno := annotations.Annotations{} - 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..a919965b --- /dev/null +++ b/pkg/bsql/provisioning_grant_replace_test.go @@ -0,0 +1,162 @@ +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")) +} + +// 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/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/provisioning_validation_idempotency_gate_test.go b/pkg/bsql/provisioning_validation_idempotency_gate_test.go new file mode 100644 index 00000000..af68b7af --- /dev/null +++ b/pkg/bsql/provisioning_validation_idempotency_gate_test.go @@ -0,0 +1,32 @@ +package bsql + +import ( + "testing" + + "github.com/conductorone/baton-sql/pkg/database" + "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. +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, + } + for engine, want := range ddl { + s := &SQLSyncer{dbEngine: engine} + require.Equal(t, want, s.validationNoRowsMeansIdempotent(), "engine=%v", engine) + } +} diff --git a/pkg/bsql/provisioning_validation_idempotency_test.go b/pkg/bsql/provisioning_validation_idempotency_test.go new file mode 100644 index 00000000..f630b869 --- /dev/null +++ b/pkg/bsql/provisioning_validation_idempotency_test.go @@ -0,0 +1,165 @@ +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/conductorone/baton-sql/pkg/database" + "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) + // 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") + + 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")) +} + +// 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) + // 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) + // 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")) + 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) { + 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) + 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) + // validation "no rows" only signals idempotency on DDL engines (Db2) + s.dbEngine = database.DB2 + + 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..52a70c59 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 @@ -602,9 +616,18 @@ func (s *SQLSyncer) runPrincipalExistsCheck( return exists, nil } -func (s *SQLSyncer) RunProvisioningQueriesWithExecutor( +// 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 +} + +func (s *SQLSyncer) runValidationQueries( ctx context.Context, - queries, validationQueries []string, vars map[string]any, executor executor, @@ -632,19 +655,39 @@ 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) } if !valid { - return fmt.Errorf("validation query returned no rows") + if s.validationNoRowsMeansIdempotent() { + l.Warn("validation query returned no rows; treating as idempotent success", zap.String("query", q)) + return fmt.Errorf("validation query %q returned no rows: %w", q, ErrValidationNoRows) + } + return fmt.Errorf("validation query %q returned no rows", q) } } + 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 { @@ -1034,6 +1077,10 @@ func (s *SQLSyncer) RunGrantProvisioning( executor, ) if err != nil { + // 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 } @@ -1047,32 +1094,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 { - 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 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)