From cfa344b7dd8fd87ce923e7af879b554e6523ff94 Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Thu, 15 Oct 2026 17:39:15 +0530 Subject: [PATCH 1/2] fix(storage): make the cassandradb provider work on apache cassandra The provider backs two DB types, cassandradb and scylladb, but only the ScyllaDB path was ever tested -- cassandradb was absent from the storage suite's backend list. Wiring it in and pointing it at cassandra:4.1 fails immediately: three queries use CQL LIKE, which Apache Cassandra accepts only against a SASI index and rejects against the plain secondary index this provider creates. LIKE restriction is only supported on properly indexed columns ScyllaDB accepts the same statements, which is why it went unnoticed. - GetWebhookByEventName errored out on every Apache Cassandra deployment, so webhook dispatch by event name never fired. - DeleteAllSessionTokensByUserID and DeleteSessionTokensByNamespace errored the same way. All three already carried ALLOW FILTERING -- a full table scan -- so moving the comparison into Go changes nothing but where it runs. DeleteAllSessionTokensByUserID also matched "%%" unanchored, so a delete for "42" took out tokens for "142" or "provider:427". The SQL provider was anchored long ago; this one kept the defect because the assertion covering it lived inside a SQL-only subtest. It now runs on every backend as Anchored Session Token Delete, and fails on ScyllaDB when the unanchored match is restored. Also stops Webhook_Operations indexing a possibly-empty result: the panic aborted the whole run for every backend queued behind it. Adds make test-cassandradb and includes cassandradb in test-all-db. Full storage suite now green on cassandra:4.1 and on scylladb. --- AGENTS.md | 5 +- Makefile | 19 ++++- .../storage/db/cassandradb/session_token.go | 67 ++++++++++----- internal/storage/db/cassandradb/webhook.go | 17 +++- internal/storage/provider_test.go | 83 ++++++++++++------- scripts/wait-for-test-dbs.sh | 2 +- 6 files changed, 134 insertions(+), 59 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 929feec75..cc63e569b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -71,13 +71,14 @@ Run `make fmt` before committing; CI runs `make lint`. ```bash make test # Full module test run; TEST_DBS=sqlite (integration tests always SQLite) make test-sqlite # Same as test — explicit SQLite-only, no Docker -make test-all-db # All 7 DBs via Docker (postgres, sqlite, mongodb, arangodb, scylladb, dynamodb, couchbase) +make test-all-db # All 8 DBs via Docker (postgres, sqlite, mongodb, arangodb, scylladb, cassandradb, dynamodb, couchbase) make smoke # Release e2e smoke tests (build tag `smoke`, 5m timeout) # Single-DB targets (each spins up Docker, runs tests, tears down) make test-postgres make test-mongodb make test-scylladb +make test-cassandradb make test-arangodb make test-dynamodb make test-couchbase @@ -85,7 +86,7 @@ make test-couchbase # Docker helpers for test-all-db make test-docker-up # Start all test DB containers + Redis make test-cleanup # Remove all test containers -make test-cleanup-postgres | test-cleanup-mongodb | test-cleanup-scylladb +make test-cleanup-postgres | test-cleanup-mongodb | test-cleanup-scylladb | test-cleanup-cassandradb make test-cleanup-arangodb | test-cleanup-dynamodb | test-cleanup-couchbase ``` diff --git a/Makefile b/Makefile index 206156b83..092823503 100644 --- a/Makefile +++ b/Makefile @@ -152,6 +152,19 @@ test-scylladb: test-cleanup-scylladb docker rm -vf authorizer_scylla_db; \ exit $$status +# The cassandradb and scylladb providers share one implementation but take +# different keyspace-replication CQL, and only the ScyllaDB path was ever +# tested. The first run of this target against real Apache Cassandra failed: +# CQL LIKE needs a SASI index there, so three queries this provider issues were +# rejected outright. Different port from scylladb on purpose, so both can run. +test-cassandradb: test-cleanup-cassandradb + docker run -d --name authorizer_cassandra_db -p 9142:9042 -e MAX_HEAP_SIZE=512M -e HEAP_NEWSIZE=128M cassandra:4.1 + sh scripts/wait-for-test-dbs.sh cassandradb && \ + { go clean --testcache; TEST_DBS="cassandradb" $(GO_TEST_ALL); }; \ + status=$$?; \ + docker rm -vf authorizer_cassandra_db; \ + exit $$status + test-arangodb: test-cleanup-arangodb docker run -d --name authorizer_arangodb -p 8529:8529 -e ARANGO_NO_AUTH=1 arangodb/arangodb:3.10.3 @# Wait for readiness, then ALWAYS tear the container down — including when @@ -203,7 +216,7 @@ test-all-db: test-cleanup test-docker-up @# failed to start them and produced a second, misleading failure. Same @# capture-status-then-clean shape the e2e-playground target already uses. go clean --testcache; \ - TEST_DBS="couchbase,postgres,sqlite,mongodb,arangodb,scylladb,dynamodb" $(GO_TEST_ALL); \ + TEST_DBS="couchbase,postgres,sqlite,mongodb,arangodb,scylladb,cassandradb,dynamodb" $(GO_TEST_ALL); \ status=$$?; \ $(MAKE) test-cleanup; \ exit $$status @@ -214,6 +227,7 @@ test-docker-up: docker run -d --name authorizer_postgres -p 5434:5432 -e POSTGRES_PASSWORD=postgres -e POSTGRES_DB=postgres postgres docker run -d --name authorizer_mongodb_db -p 27017:27017 mongo:4.4.15 docker run -d --name authorizer_scylla_db -p 9042:9042 scylladb/scylla + docker run -d --name authorizer_cassandra_db -p 9142:9042 -e MAX_HEAP_SIZE=512M -e HEAP_NEWSIZE=128M cassandra:4.1 docker run -d --name authorizer_arangodb -p 8529:8529 -e ARANGO_NO_AUTH=1 arangodb/arangodb:3.10.3 docker run -d --name authorizer_dynamodb -p 8000:8000 amazon/dynamodb-local:latest docker run -d --name authorizer_couchbase -p 8091-8097:8091-8097 -p 11210:11210 -p 11207:11207 -p 18091-18095:18091-18095 -p 18096:18096 -p 18097:18097 couchbase:latest @@ -224,6 +238,7 @@ test-docker-up: test-cleanup: -docker rm -vf authorizer_postgres -docker rm -vf authorizer_scylla_db + -docker rm -vf authorizer_cassandra_db -docker rm -vf authorizer_mongodb_db -docker rm -vf authorizer_arangodb -docker rm -vf authorizer_dynamodb @@ -236,6 +251,8 @@ test-cleanup-mongodb: -docker rm -vf authorizer_mongodb_db test-cleanup-scylladb: -docker rm -vf authorizer_scylla_db +test-cleanup-cassandradb: + -docker rm -vf authorizer_cassandra_db test-cleanup-arangodb: -docker rm -vf authorizer_arangodb test-cleanup-dynamodb: diff --git a/internal/storage/db/cassandradb/session_token.go b/internal/storage/db/cassandradb/session_token.go index 172c2ebae..b18f229f8 100644 --- a/internal/storage/db/cassandradb/session_token.go +++ b/internal/storage/db/cassandradb/session_token.go @@ -3,6 +3,7 @@ package cassandradb import ( "context" "fmt" + "strings" "time" "github.com/gocql/gocql" @@ -82,18 +83,26 @@ func (p *provider) DeleteSessionTokenByUserIDAndKey(ctx context.Context, userId, return deleted, nil } -// DeleteAllSessionTokensByUserID deletes all session tokens for a user ID +// DeleteAllSessionTokensByUserID deletes all session tokens for a user ID. +// The userId parameter can be just the user ID (e.g. "123") or the full stored +// format (e.g. "auth_provider:123"), matching the SQL provider. +// +// The match is **anchored**: the exact value, or a ":" suffix. It used +// to be an unanchored "%%" contains-match, which let a delete for "42" +// take out tokens belonging to "142" or "provider:427" — the same defect the +// SQL provider already fixed, left live here because no test covered it on this +// backend. +// +// Matching happens in Go rather than with CQL LIKE: Apache Cassandra only +// accepts LIKE on a SASI index and rejects it against the plain secondary index +// this provider creates, so these two methods failed outright there. The query +// already carried ALLOW FILTERING, i.e. a full table scan, so the only thing +// that changes is where the comparison runs. func (p *provider) DeleteAllSessionTokensByUserID(ctx context.Context, userId string) error { - var ids []string - likePattern := "%" + userId + "%" - query := fmt.Sprintf(`SELECT id FROM %s WHERE user_id LIKE ? ALLOW FILTERING`, - KeySpace+"."+schemas.Collections.SessionToken) - iter := p.db.Query(query, likePattern).Iter() - var id string - for iter.Scan(&id) { - ids = append(ids, id) - } - if err := iter.Close(); err != nil { + ids, err := p.sessionTokenIDsMatching(func(storedUserID string) bool { + return storedUserID == userId || strings.HasSuffix(storedUserID, ":"+userId) + }) + if err != nil { return err } for _, id := range ids { @@ -106,17 +115,14 @@ func (p *provider) DeleteAllSessionTokensByUserID(ctx context.Context, userId st } // DeleteSessionTokensByNamespace deletes all session tokens for a namespace +// (e.g. "auth_provider"). A legitimate prefix match: stored user_id is +// ":". See DeleteAllSessionTokensByUserID for why the match +// is done in Go instead of with CQL LIKE. func (p *provider) DeleteSessionTokensByNamespace(ctx context.Context, namespace string) error { - likePattern := namespace + ":%" - var ids []string - query := fmt.Sprintf(`SELECT id FROM %s WHERE user_id LIKE ? ALLOW FILTERING`, - KeySpace+"."+schemas.Collections.SessionToken) - iter := p.db.Query(query, likePattern).Iter() - var id string - for iter.Scan(&id) { - ids = append(ids, id) - } - if err := iter.Close(); err != nil { + ids, err := p.sessionTokenIDsMatching(func(storedUserID string) bool { + return strings.HasPrefix(storedUserID, namespace+":") + }) + if err != nil { return err } for _, id := range ids { @@ -383,3 +389,22 @@ func (p *provider) GetAllOAuthStates(ctx context.Context) ([]*schemas.OAuthState } return states, nil } + +// sessionTokenIDsMatching scans the session-token table and returns the ids +// whose stored user_id satisfies match. One full scan, same as the ALLOW +// FILTERING query it replaces. +func (p *provider) sessionTokenIDsMatching(match func(storedUserID string) bool) ([]string, error) { + query := fmt.Sprintf(`SELECT id, user_id FROM %s`, KeySpace+"."+schemas.Collections.SessionToken) + iter := p.db.Query(query).Iter() + var ids []string + var id, storedUserID string + for iter.Scan(&id, &storedUserID) { + if match(storedUserID) { + ids = append(ids, id) + } + } + if err := iter.Close(); err != nil { + return nil, err + } + return ids, nil +} diff --git a/internal/storage/db/cassandradb/webhook.go b/internal/storage/db/cassandradb/webhook.go index 658394043..7ae2448f6 100644 --- a/internal/storage/db/cassandradb/webhook.go +++ b/internal/storage/db/cassandradb/webhook.go @@ -111,8 +111,15 @@ func (p *provider) GetWebhookByID(ctx context.Context, webhookID string) (*schem // GetWebhookByEventName to get webhook by event_name func (p *provider) GetWebhookByEventName(ctx context.Context, eventName string) ([]*schemas.Webhook, error) { - query := fmt.Sprintf(`SELECT id, event_description, event_name, endpoint, headers, enabled, created_at, updated_at FROM %s WHERE event_name LIKE ? ALLOW FILTERING`, KeySpace+"."+schemas.Collections.Webhook) - scanner := p.db.Query(query, eventName+"%").Iter().Scanner() + // Prefix match in Go, not with CQL LIKE. Apache Cassandra only accepts LIKE + // on a SASI index — with the plain secondary index this provider creates it + // rejects the query outright ("LIKE restriction is only supported on + // properly indexed columns"), so webhook dispatch failed on every Apache + // Cassandra deployment. ScyllaDB accepts the same statement, which is why + // it went unnoticed. Costs nothing: ALLOW FILTERING already scanned the + // whole table, so the only change is where the comparison happens. + query := fmt.Sprintf(`SELECT id, event_description, event_name, endpoint, headers, enabled, created_at, updated_at FROM %s`, KeySpace+"."+schemas.Collections.Webhook) + scanner := p.db.Query(query).Iter().Scanner() webhooks := []*schemas.Webhook{} for scanner.Next() { var webhook schemas.Webhook @@ -120,8 +127,14 @@ func (p *provider) GetWebhookByEventName(ctx context.Context, eventName string) if err != nil { return nil, err } + if !strings.HasPrefix(webhook.EventName, eventName) { + continue + } webhooks = append(webhooks, &webhook) } + if err := scanner.Err(); err != nil { + return nil, err + } return webhooks, nil } diff --git a/internal/storage/provider_test.go b/internal/storage/provider_test.go index b9c98628e..0d023a0ff 100644 --- a/internal/storage/provider_test.go +++ b/internal/storage/provider_test.go @@ -28,6 +28,7 @@ var allDBTypes = []string{ constants.DbTypeMongoDB, constants.DbTypeArangoDB, constants.DbTypeScyllaDB, + constants.DbTypeCassandraDB, constants.DbTypeCouchbaseDB, constants.DbTypeDynamoDB, } @@ -70,6 +71,11 @@ func getTestDBConfig(dbType string) *config.Config { cfg.DatabaseURL = "http://localhost:8529" case constants.DbTypeScyllaDB: cfg.DatabaseURL = "127.0.0.1:9042" + case constants.DbTypeCassandraDB: + // A separate port from ScyllaDB on purpose: this provider backs both DB + // types and they take different keyspace-replication CQL, so a shared + // port would silently test one engine twice. + cfg.DatabaseURL = "127.0.0.1:9142" case constants.DbTypeCouchbaseDB: cfg.DatabaseURL = "couchbase://127.0.0.1" // Allow extra time for Couchbase container to become ready in tests (test-all-db runs Couchbase last) @@ -230,6 +236,10 @@ func TestStorageProvider(t *testing.T) { testDeleteUserCascade(t, ctx, provider) }) + t.Run("Anchored Session Token Delete", func(t *testing.T) { + testAnchoredSessionTokenDelete(t, ctx, provider) + }) + if isSQLTestDB(dbType) { t.Run("SQL CRUD Correctness Fixes", func(t *testing.T) { testSQLCRUDCorrectnessFixes(t, ctx, provider) @@ -284,36 +294,6 @@ func testSQLCRUDCorrectnessFixes(t *testing.T, ctx context.Context, provider Pro require.NoError(t, provider.DeleteUser(ctx, userA)) - // #2: DeleteAllSessionTokensByUserID must delete only the target user's - // tokens, not another user whose stored id contains the target as a - // substring. otherUser ("xu"+suffix) contains targetUser ("u"+suffix). - suffix := uuid.New().String() - targetUser := "u" + suffix - otherUser := "xu" + suffix - targetStored := "auth_provider:" + targetUser - otherStored := "auth_provider:" + otherUser - - addToken := func(storedUserID, key string) { - require.NoError(t, provider.AddSessionToken(ctx, &schemas.SessionToken{ - UserID: storedUserID, - KeyName: key, - Token: "tok_" + uuid.New().String(), - ExpiresAt: time.Now().Add(60 * time.Second).Unix(), - })) - } - addToken(targetStored, "session_token_key") - addToken(otherStored, "session_token_key") - - require.NoError(t, provider.DeleteAllSessionTokensByUserID(ctx, targetUser)) - - _, err = provider.GetSessionTokenByUserIDAndKey(ctx, targetStored, "session_token_key") - assert.Error(t, err, "target user's session token should be deleted") - _, err = provider.GetSessionTokenByUserIDAndKey(ctx, otherStored, "session_token_key") - assert.NoError(t, err, "substring-matching other user's session token must survive") - - // cleanup the surviving token - _ = provider.DeleteAllSessionTokensByUserID(ctx, otherUser) - // #3a: UpdateUser rejects a partial struct (CreatedAt == 0, i.e. never loaded). _, err = provider.UpdateUser(ctx, &schemas.User{ ID: uuid.New().String(), @@ -348,6 +328,43 @@ func testSQLCRUDCorrectnessFixes(t *testing.T, ctx context.Context, provider Pro require.NoError(t, provider.DeleteUser(ctx, refetched)) } +// testAnchoredSessionTokenDelete asserts DeleteAllSessionTokensByUserID deletes +// only the target user's tokens, never another user whose stored id merely +// contains the target as a substring. otherUser ("xu"+suffix) contains +// targetUser ("u"+suffix). +// +// It runs on every backend because that is the only way this class of bug is +// caught: the SQL provider was anchored long ago, while Cassandra/ScyllaDB kept +// an unanchored "%%" match that deleted the other user's token too — +// and CI stayed green because this assertion lived inside a SQL-only subtest. +func testAnchoredSessionTokenDelete(t *testing.T, ctx context.Context, provider Provider) { + suffix := uuid.New().String() + targetUser := "u" + suffix + otherUser := "xu" + suffix + targetStored := "auth_provider:" + targetUser + otherStored := "auth_provider:" + otherUser + + addToken := func(storedUserID string) { + require.NoError(t, provider.AddSessionToken(ctx, &schemas.SessionToken{ + UserID: storedUserID, + KeyName: "session_token_key", + Token: "tok_" + uuid.New().String(), + ExpiresAt: time.Now().Add(60 * time.Second).Unix(), + })) + } + addToken(targetStored) + addToken(otherStored) + + require.NoError(t, provider.DeleteAllSessionTokensByUserID(ctx, targetUser)) + + _, err := provider.GetSessionTokenByUserIDAndKey(ctx, targetStored, "session_token_key") + assert.Error(t, err, "target user's session token should be deleted") + _, err = provider.GetSessionTokenByUserIDAndKey(ctx, otherStored, "session_token_key") + assert.NoError(t, err, "substring-matching other user's session token must survive") + + _ = provider.DeleteAllSessionTokensByUserID(ctx, otherUser) +} + // testUserSearchOperations verifies ListUsers' optional case-insensitive // substring filter across email/given_name/family_name/nickname, and that // pagination + total reflect the filtered set. Every backend must honour it, @@ -709,8 +726,10 @@ func testWebhookOperations(t *testing.T, ctx context.Context, provider Provider, // Test GetWebhookByEventName fetchedByEventName, err := provider.GetWebhookByEventName(ctx, webhook.EventName) - assert.NoError(t, err) - assert.NotNil(t, fetchedByEventName) + require.NoError(t, err) + // require, not assert: indexing an empty result panics, and a panic in one + // backend's subtest aborts the run for every backend after it. + require.NotEmpty(t, fetchedByEventName, "GetWebhookByEventName must find the webhook just created") assert.Equal(t, created.ID, fetchedByEventName[0].ID) // Test UpdateWebhook mutates only the changed field and preserves the rest. diff --git a/scripts/wait-for-test-dbs.sh b/scripts/wait-for-test-dbs.sh index 1dc8d7bf8..7e7f99fbc 100755 --- a/scripts/wait-for-test-dbs.sh +++ b/scripts/wait-for-test-dbs.sh @@ -15,7 +15,7 @@ TIMEOUT_SECONDS="${TEST_DB_WAIT_TIMEOUT:-120}" # name:port pairs. Couchbase is absent on purpose — scripts/couchbase-test.sh # already provisions and waits for it. -ALL_SERVICES="redis:6380 postgres:5434 mongodb:27017 scylladb:9042 arangodb:8529 dynamodb:8000" +ALL_SERVICES="redis:6380 postgres:5434 mongodb:27017 scylladb:9042 cassandradb:9142 arangodb:8529 dynamodb:8000" # With no arguments, wait for everything (make test-all-db). With arguments, # wait only for the named services, so a single-backend target does not block on From 18d44f7217a776ea2611f589ff92d70576f83d88 Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Thu, 15 Oct 2026 17:44:32 +0530 Subject: [PATCH 2/2] test(storage): stop the user suite depending on an almost-empty table Two self-inflicted failures that look like storage bugs. Both are invisible in CI, which gets a fresh container per run, and deterministic on any repeated local run -- the "flaky ScyllaDB storage tests" shape. User_Operations asserted the just-created user appears in ListUsers(Limit:10, Offset:0) *unfiltered*. That only holds while the table is nearly empty. At 463 accumulated rows it failed 15 of 15 runs with "created user should appear in ListUsers", i.e. exactly like a write that never landed. Now filtered by the user's own email, which keeps both assertions and makes them independent of how much data is already there. The two phone numbers were fixed literals, so the failure above -- which aborts before the cleanup at the bottom -- left +1234567891 in the table and every later run failed earlier with "user with given phone number already exists". One failure cascading into a different, misleading one. Unique per run now, like the emails. Verified against the same 463-row keyspace: was 15/15 failing, now 5/5 passing. Refs #813 --- internal/storage/provider_test.go | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/internal/storage/provider_test.go b/internal/storage/provider_test.go index 0d023a0ff..7a5761137 100644 --- a/internal/storage/provider_test.go +++ b/internal/storage/provider_test.go @@ -544,7 +544,15 @@ func testUserOperations(t *testing.T, ctx context.Context, provider Provider, db assert.Error(t, err) // Test GetUserByPhoneNumber - user.PhoneNumber = refs.NewStringRef("+1234567891") + // + // Unique numbers per run, like the emails above. These were two fixed + // literals, so a run that failed before the cleanup at the bottom left + // them in the table and every later run against the same database failed + // here instead with "user with given phone number already exists" — one + // failure cascading into a different, misleading one. + phoneBefore := "+1" + strings.ReplaceAll(uuid.New().String(), "-", "")[:10] + phoneAfter := "+2" + strings.ReplaceAll(uuid.New().String(), "-", "")[:10] + user.PhoneNumber = refs.NewStringRef(phoneBefore) createdUser, err = provider.AddUser(ctx, user) require.NoError(t, err) require.NotNil(t, createdUser) @@ -561,10 +569,19 @@ func testUserOperations(t *testing.T, ctx context.Context, provider Provider, db assert.Equal(t, *user.Password, *fetchedUser.Password) // Test UpdateUsers + // + // Filter by this user's own email rather than listing the first page + // unfiltered. The unfiltered form asserted the just-created user appears in + // Limit:10/Offset:0, which only holds while the table is nearly empty — it + // passes in CI (fresh container per run) and fails on every repeated local + // run once a few hundred rows have accumulated, looking exactly like a + // write that did not take effect. Scoping the query keeps both assertions + // (ListUsers works; the created user carries its password) independent of + // how much data is already there. users, _, err = provider.ListUsers(ctx, &model.Pagination{ Limit: 10, Offset: 0, - }, "") + }, *user.Email) assert.NoError(t, err) assert.NotNil(t, users) assert.Greater(t, len(users), 0) @@ -581,16 +598,16 @@ func testUserOperations(t *testing.T, ctx context.Context, provider Provider, db require.NotNil(t, foundInList.Password, "ListUsers must not drop the stored password") assert.Equal(t, *user.Password, *foundInList.Password) data := map[string]interface{}{ - "phone_number": "+3216549870", + "phone_number": phoneAfter, } err = provider.UpdateUsers(ctx, data, []string{createdUser.ID}) assert.NoError(t, err) // Test GetUserByPhoneNumber after update - user, err = provider.GetUserByPhoneNumber(ctx, "+3216549870") + user, err = provider.GetUserByPhoneNumber(ctx, phoneAfter) require.NoError(t, err) require.NotNil(t, user) - assert.Equal(t, "+3216549870", *user.PhoneNumber) + assert.Equal(t, phoneAfter, *user.PhoneNumber) // Cleanup: delete the user to avoid data leakage between test runs err = provider.DeleteUser(ctx, user)