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 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/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) } } diff --git a/docs/db2.md b/docs/db2.md index 6922a829..91986c2f 100644 --- a/docs/db2.md +++ b/docs/db2.md @@ -140,6 +140,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. 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 - 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..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/config.go b/pkg/bsql/config.go index e5a656d7..9e3999e1 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, 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). + // + // 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_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/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..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 @@ -602,9 +616,24 @@ func (s *SQLSyncer) runPrincipalExistsCheck( return exists, nil } -func (s *SQLSyncer) RunProvisioningQueriesWithExecutor( +// 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: 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 { + case database.DB2, database.Oracle: + return true + default: + return false + } +} + +func (s *SQLSyncer) runValidationQueries( ctx context.Context, - queries, validationQueries []string, vars map[string]any, executor executor, @@ -632,19 +661,38 @@ 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() { + return 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 +1082,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 +1099,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 cb69ba02..0df76af4 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, 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 new file mode 100644 index 00000000..e1c67cc4 --- /dev/null +++ b/pkg/database/autherror.go @@ -0,0 +1,40 @@ +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. 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. +// +// 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.Errorf(codes.Unauthenticated, "database %q authentication failed", name) + } + + var myErr *mysql.MySQLError + if errors.As(err, &myErr) && myErr.Number == mysqlAccessDenied { + 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 new file mode 100644 index 00000000..883e3ff0 --- /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, "testdb") + 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)) + } + }) + } +}