From 103e09e6c25aa691ebdad86bf8a0ef4a1e17d447 Mon Sep 17 00:00:00 2001 From: Rex Raphael Date: Tue, 25 Aug 2026 23:36:31 -0500 Subject: [PATCH 1/2] test(plugins): pin three store invariants that no backend was checking The ssftest and ssotest suites already run all four backends, and an older store_* conformance family covers some of the same ground on memory and sqlite only. Three of its cases have no counterpart in the newer suites, so those invariants go unchecked on postgres and mongo. This moves them across rather than growing a second suite that also needs a postgres runner. SubjectLinkUpsertIsConcurrencySafe fires thirty goroutines at one tuple. SubjectLinkUpsertIsIdempotent already proves a second call updates rather than collides when it arrives afterwards, and says nothing about both arriving at once. That is not hypothetical: a subject link is written on every SSO sign-in, so two writes for one subject is what happens when somebody opens two tabs. A read-then-write implementation loses the race, both readers miss, both insert, and the loser surfaces a raw constraint error. Every call must return nil and exactly one row must survive, so a deterministic write after the storm settles pins down which one. ActiveSignalsAreEnvironmentScoped keeps a signal raised in production from raising the risk on a development sign-in for the same app and user. A query that filters on app and user and forgets the environment predicate fails here and nowhere else. DomainLookupIsOrgScoped covers the tenancy level below DomainLookupIsAppScoped. One company runs SSO for several of its own organizations, and the partial unique index is on (app_id, org_id, domain) where active, so the same domain is legitimately configured twice inside one app. A lookup that drops the org predicate routes a user to a sibling organization's identity provider. This needed an org pair on ssotest.Fixture, added here so the struct and the case land together. org_id carries no foreign key, so they need no rows. The signals case passes a bare time.Now() because risk.go does. A case that builds a tidy truncated UTC input tests the store against a caller that does not exist. make test-integration runs the Store Conformance job locally, mongo replica set included, using the workflow's own -run pattern and -p 1. --- Makefile | 40 ++++++++++++- plugins/sharedsignals/ssftest/event_cases.go | 42 ++++++++++++++ plugins/sharedsignals/ssftest/ssftest.go | 2 + plugins/sharedsignals/ssftest/stream_cases.go | 56 +++++++++++++++++++ plugins/sso/conformance_test.go | 3 + plugins/sso/ssotest/cases.go | 52 +++++++++++++++++ plugins/sso/ssotest/ssotest.go | 7 +++ 7 files changed, 201 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index ef62e147..9078083c 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: help test clean fmt lint lint-fix vet tidy deps check coverage t c f l lf v check-deps +.PHONY: help test test-integration clean fmt lint lint-fix vet tidy deps check coverage t c f l lf v check-deps # Default target .DEFAULT_GOAL := help @@ -7,6 +7,11 @@ GO=go GOFLAGS=-v +# Local integration run. The port is deliberately not 27017, so this does not +# collide with a MongoDB you already have running. +MONGO_PORT ?= 27019 +MONGO_CONTAINER ?= authsome-integration-mongo + # Colors for output RED=\033[0;31m GREEN=\033[0;32m @@ -102,6 +107,39 @@ test-race: $(GO) test -race -v ./... @echo "$(GREEN)✓ Race tests complete$(NC)" +## test-integration: Run the Store Conformance CI job locally (needs Docker) +# Same command the "Store Conformance (pg + mongo)" job runs, including its +# -run pattern and its -p 1. Postgres comes up on its own through +# testcontainers. Mongo needs a replica set, because the app and org cascades +# run in transactions, so this starts one and tears it down again after. +# +# You get a new database every run, and you want that. Several of these tests +# write fixed literals and never clean up after themselves, so a second run +# against a database the first one dirtied fails on duplicate keys. CI never +# sees it because CI gets a fresh container. +test-integration: + @echo "$(BLUE)Starting MongoDB replica set on port $(MONGO_PORT)...$(NC)" + @docker rm -f $(MONGO_CONTAINER) >/dev/null 2>&1 || true + @docker run -d --name $(MONGO_CONTAINER) -p $(MONGO_PORT):27017 mongo:7 --replSet rs0 >/dev/null + @for i in $$(seq 1 30); do \ + docker exec $(MONGO_CONTAINER) mongosh --quiet --eval 'db.runCommand({ping:1})' >/dev/null 2>&1 && break; \ + sleep 2; \ + done + @docker exec $(MONGO_CONTAINER) mongosh --quiet \ + --eval 'rs.initiate({_id:"rs0",members:[{_id:0,host:"localhost:27017"}]})' >/dev/null 2>&1 || true + @for i in $$(seq 1 30); do \ + docker exec $(MONGO_CONTAINER) mongosh --quiet --eval 'db.hello().isWritablePrimary' 2>/dev/null | grep -q true && break; \ + sleep 2; \ + done + @echo "$(BLUE)Running integration suite...$(NC)" + @AUTHSOME_MONGO_URI='mongodb://localhost:$(MONGO_PORT)/authsome_test?replicaSet=rs0&directConnection=true' \ + $(GO) test -tags integration -p 1 -count=1 -timeout 15m \ + -run '^TestConformance$$|^TestStoreConformance_|^TestMigration_' ./store/... ./plugins/...; \ + status=$$?; \ + docker rm -f $(MONGO_CONTAINER) >/dev/null 2>&1 || true; \ + if [ $$status -ne 0 ]; then exit $$status; fi; \ + printf '$(GREEN)✓ Integration suite complete$(NC)\n' + ## coverage: Generate test coverage coverage: @echo "$(BLUE)Generating coverage report...$(NC)" diff --git a/plugins/sharedsignals/ssftest/event_cases.go b/plugins/sharedsignals/ssftest/event_cases.go index 6f007c7b..308fa81f 100644 --- a/plugins/sharedsignals/ssftest/event_cases.go +++ b/plugins/sharedsignals/ssftest/event_cases.go @@ -265,3 +265,45 @@ func testExpiredSignalIsNotActive(t *testing.T, f Fixture) { "a risk signal that expired an hour ago is still constraining sign-in") assert.True(t, sawLive, "a signal with an hour left must still apply") } + +// testActiveSignalsAreEnvironmentScoped keeps a signal raised in one +// environment out of another. Risk decisions run per environment, so a +// production compromise must not raise the risk on a development sign-in for +// the same app and the same user, and vice versa. A query that filters on +// app and user but forgets the environment predicate fails exactly here. +func testActiveSignalsAreEnvironmentScoped(t *testing.T, f Fixture) { + ctx := context.Background() + s := seedStream(t, f) + + // A second environment under the same app. Neither the signals table nor + // its indexes declare a foreign key on env_id, so this needs no row of + // its own on any backend. + otherEnv := id.NewEnvironmentID() + + sig := &ssf.Signal{ + ID: id.NewSSFSignalID(), AppID: f.AppID, EnvID: f.EnvID, UserID: f.UserID, + StreamID: s.ID, EventType: sessionRevoked, Severity: 100, + Reason: "credential-leak", EventAt: now(), + ExpiresAt: now().Add(time.Hour), CreatedAt: now(), + } + require.NoError(t, f.Store.CreateSignal(ctx, sig)) + + // time.Now() unqualified, matching the risk path's own call in risk.go. + other, err := f.Store.ListActiveSignals(ctx, f.AppID, otherEnv, f.UserID, time.Now()) + require.NoError(t, err) + assert.NotNil(t, other, "a zero result must be an empty slice rather than nil") + for _, got := range other { + assert.NotEqual(t, sig.ID, got.ID, + "a signal from another environment leaked into env %s", otherEnv) + } + + mine, err := f.Store.ListActiveSignals(ctx, f.AppID, f.EnvID, f.UserID, time.Now()) + require.NoError(t, err) + var found bool + for _, got := range mine { + if got.ID == sig.ID { + found = true + } + } + assert.True(t, found, "the signal must still be active in its own environment") +} diff --git a/plugins/sharedsignals/ssftest/ssftest.go b/plugins/sharedsignals/ssftest/ssftest.go index 3dadb600..15fc09a6 100644 --- a/plugins/sharedsignals/ssftest/ssftest.go +++ b/plugins/sharedsignals/ssftest/ssftest.go @@ -55,6 +55,7 @@ func RunConformance(t *testing.T, newFixture Factory, skip ...string) { {"DeleteStream", testDeleteStream}, {"SubjectLinkUpsertIsIdempotent", testSubjectLinkUpsertIsIdempotent}, {"SubjectLinkLookupIsTenantScoped", testSubjectLinkLookupIsTenantScoped}, + {"SubjectLinkUpsertIsConcurrencySafe", testSubjectLinkUpsertIsConcurrencySafe}, {"DuplicateJTIIsRejected", testDuplicateJTIIsRejected}, {"SameJTIOnAnotherStreamIsAllowed", testSameJTIOnAnotherStreamIsAllowed}, {"GetReceivedEventIsAppScoped", testGetReceivedEventIsAppScoped}, @@ -64,6 +65,7 @@ func RunConformance(t *testing.T, newFixture Factory, skip ...string) { {"DeleteReceivedEventFreesTheJTI", testDeleteReceivedEventFreesTheJTI}, {"CountEventsSince", testCountEventsSince}, {"ExpiredSignalIsNotActive", testExpiredSignalIsNotActive}, + {"ActiveSignalsAreEnvironmentScoped", testActiveSignalsAreEnvironmentScoped}, } for _, tc := range cases { if skipSet[tc.name] { diff --git a/plugins/sharedsignals/ssftest/stream_cases.go b/plugins/sharedsignals/ssftest/stream_cases.go index 41adbe04..e7aa75bb 100644 --- a/plugins/sharedsignals/ssftest/stream_cases.go +++ b/plugins/sharedsignals/ssftest/stream_cases.go @@ -3,6 +3,7 @@ package ssftest import ( "context" "errors" + "sync" "testing" "time" @@ -187,3 +188,58 @@ func testSubjectLinkLookupIsTenantScoped(t *testing.T, f Fixture) { require.Error(t, err, "subject link lookup crossed a tenant boundary") assert.True(t, errors.Is(err, ssf.ErrNotFound), "got %v", err) } + +// testSubjectLinkUpsertIsConcurrencySafe fires the same tuple at the store +// from many goroutines at once. This is not ceremony and the goroutine count +// is not arbitrary: a subject link is written on every SSO sign-in, so two +// writes for one subject arriving together is what happens when somebody +// opens two tabs. testSubjectLinkUpsertIsIdempotent proves the second call +// updates rather than collides when it arrives afterwards. It says nothing +// about what happens when both arrive at once. +// +// A read-then-write implementation loses that race. Both readers see "not +// found", both insert, and the loser hits the unique index and surfaces a raw +// constraint error instead of succeeding. So every call here must return nil, +// and exactly one row must survive. +func testSubjectLinkUpsertIsConcurrencySafe(t *testing.T, f Fixture) { + ctx := context.Background() + issuer := "https://" + unique("race") + ".test" + subject := unique("subject") + + const n = 30 + errs := make([]error, n) + var wg sync.WaitGroup + wg.Add(n) + for i := 0; i < n; i++ { + go func(i int) { + defer wg.Done() + errs[i] = f.Store.UpsertSubjectLink(ctx, &ssf.SubjectLink{ + ID: id.NewSSFLinkID(), AppID: f.AppID, EnvID: f.EnvID, + Issuer: issuer, Subject: subject, UserID: id.NewUserID(), + Source: "verified", CreatedAt: now(), LastSeenAt: now(), + }) + }(i) + } + wg.Wait() + + for i, err := range errs { + assert.NoError(t, err, "concurrent upsert %d must not surface a constraint error", i) + } + + // The storm proves no writer surfaces a raw constraint error, but real + // concurrency leaves no way to know which of the n writes landed last. + // One more deterministic write after it settles pins that down: if this + // one, and only this one, comes back, then exactly one row survived and + // the operation is genuinely last-write-wins. + last := id.NewUserID() + require.NoError(t, f.Store.UpsertSubjectLink(ctx, &ssf.SubjectLink{ + ID: id.NewSSFLinkID(), AppID: f.AppID, EnvID: f.EnvID, + Issuer: issuer, Subject: subject, UserID: last, + Source: "verified", CreatedAt: now(), LastSeenAt: now(), + })) + + got, err := f.Store.GetSubjectLink(ctx, f.AppID, f.EnvID, issuer, subject) + require.NoError(t, err) + assert.Equal(t, last, got.UserID, + "exactly one row must survive the race, holding the last-written UserID") +} diff --git a/plugins/sso/conformance_test.go b/plugins/sso/conformance_test.go index 8a0a8f0d..075b890e 100644 --- a/plugins/sso/conformance_test.go +++ b/plugins/sso/conformance_test.go @@ -11,6 +11,7 @@ import ( "github.com/xraph/grove/drivers/sqlitedriver" _ "github.com/xraph/grove/drivers/sqlitedriver/sqlitemigrate" + "github.com/xraph/authsome/id" "github.com/xraph/authsome/plugins/sso" "github.com/xraph/authsome/plugins/sso/ssotest" "github.com/xraph/authsome/store" @@ -57,6 +58,8 @@ func seedFixture(t *testing.T, core store.Store, plugin sso.Store) ssotest.Fixtu EnvID: tn.EnvID.String(), OtherAppID: other.AppID, OtherEnvID: other.EnvID.String(), + OrgID: id.NewOrgID(), + OtherOrgID: id.NewOrgID(), } } diff --git a/plugins/sso/ssotest/cases.go b/plugins/sso/ssotest/cases.go index 556683d2..64dab3a0 100644 --- a/plugins/sso/ssotest/cases.go +++ b/plugins/sso/ssotest/cases.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" "github.com/xraph/authsome/id" + "github.com/xraph/authsome/plugins/sso" ) func testConnectionCRUD(t *testing.T, f Fixture) { @@ -222,3 +223,54 @@ func testDeleteConnection(t *testing.T, f Fixture) { _, err = f.Store.GetConnectionByDomain(ctx, f.AppID, c.Domain) assert.Error(t, err, "a deleted connection must stop resolving by domain too") } + +// testDomainLookupIsOrgScoped covers the multi-tenant case one level below +// testDomainLookupIsAppScoped. One company can run SSO for several of its own +// organizations, and the unique index is on (app_id, org_id, domain) where +// active, so the same domain is legitimately configured twice inside one app. +// GetConnectionByDomainAndOrg has to return the right one. A lookup that +// drops the org predicate resolves an email to whichever org happens to sort +// first, sending that user to another org's identity provider. +func testDomainLookupIsOrgScoped(t *testing.T, f Fixture) { + if f.OrgID.IsNil() || f.OtherOrgID.IsNil() { + t.Skip("fixture provides no second organization") + } + ctx := context.Background() + + // One domain, two orgs, same app. Both rows are active, which the partial + // unique index permits precisely because org_id is part of the key. + domain := unique("shared") + ".test" + + mine := newConnection(f.AppID, f.EnvID) + mine.Domain, mine.OrgID = domain, f.OrgID + require.NoError(t, f.Store.CreateConnection(ctx, mine)) + + theirs := newConnection(f.AppID, f.EnvID) + theirs.Domain, theirs.OrgID = domain, f.OtherOrgID + require.NoError(t, f.Store.CreateConnection(ctx, theirs), + "the same domain must be configurable in a second org of the same app") + + got, err := f.Store.GetConnectionByDomainAndOrg(ctx, f.AppID, f.OrgID, domain) + require.NoError(t, err) + assert.Equal(t, mine.ID, got.ID, "domain lookup returned the wrong organization's connection") + + got, err = f.Store.GetConnectionByDomainAndOrg(ctx, f.AppID, f.OtherOrgID, domain) + require.NoError(t, err) + assert.Equal(t, theirs.ID, got.ID, "domain lookup returned the wrong organization's connection") + + // An org with nothing configured for the domain must come up empty rather + // than borrowing a sibling org's connection. + _, err = f.Store.GetConnectionByDomainAndOrg(ctx, f.AppID, id.NewOrgID(), domain) + assert.ErrorIs(t, err, sso.ErrConnectionNotFound) + + // Deactivating one org's connection must leave the other untouched. + mine.Active = false + require.NoError(t, f.Store.UpdateConnection(ctx, mine)) + _, err = f.Store.GetConnectionByDomainAndOrg(ctx, f.AppID, f.OrgID, domain) + assert.ErrorIs(t, err, sso.ErrConnectionNotFound) + + still, err := f.Store.GetConnectionByDomainAndOrg(ctx, f.AppID, f.OtherOrgID, domain) + require.NoError(t, err) + assert.Equal(t, theirs.ID, still.ID, + "deactivating one organization's connection removed another's") +} diff --git a/plugins/sso/ssotest/ssotest.go b/plugins/sso/ssotest/ssotest.go index 9d561303..ce95cff7 100644 --- a/plugins/sso/ssotest/ssotest.go +++ b/plugins/sso/ssotest/ssotest.go @@ -30,6 +30,12 @@ type Fixture struct { OtherAppID id.AppID // OtherEnvID is that second tenant's default environment. OtherEnvID string + // OrgID and OtherOrgID are two organizations inside AppID. Connections + // route per organization as well as per app, so proving that lookup needs + // two orgs under one tenant rather than two tenants. org_id carries no + // foreign key on any backend, so these need no row of their own. + OrgID id.OrgID + OtherOrgID id.OrgID } // Factory builds a fresh, empty, migrated fixture for a single test. @@ -49,6 +55,7 @@ func RunConformance(t *testing.T, newFixture Factory, skip ...string) { {"ConnectionCRUD", testConnectionCRUD}, {"ConnectionNotFound", testConnectionNotFound}, {"DomainLookupIsAppScoped", testDomainLookupIsAppScoped}, + {"DomainLookupIsOrgScoped", testDomainLookupIsOrgScoped}, {"ProviderLookupIsAppScoped", testProviderLookupIsAppScoped}, {"ListConnectionsIsAppScoped", testListConnectionsIsAppScoped}, {"SAMLFieldsRoundTrip", testSAMLFieldsRoundTrip}, From 671824a56d8256691172ebc3d395321448439ed2 Mon Sep 17 00:00:00 2001 From: Rex Raphael Date: Tue, 25 Aug 2026 23:41:31 -0500 Subject: [PATCH 2/2] test(plugins): tighten the not-found assertion and correct two claims Review follow-ups on the three cases. ConnectionNotFound asserted a bare error, so a backend returning something other than ErrConnectionNotFound passed it. Callers branch on that sentinel to tell "no SSO configured for this domain" from a backend that failed, and those two go different ways: the first falls through to password login, the second has to surface. All four backends already return it, so this only closes the hole rather than changing behaviour. The concurrency case claimed its final read proved exactly one row survived. It does not. GetSubjectLink is a FindOne and would return a row either way. What proves the count is the loop above it: a second row for the tuple could only come from a writer that hit the unique index and reported the collision, and that writer would have failed the loop. On memory there is no index and the mutex does that work. The final read proves last-write-wins, which is what it now says. Dropped the non-nil assertion on ListActiveSignals. All four backends return a non-nil empty slice, but by way of make([]*Signal, 0, n), which is a preallocation that happens to be non-nil rather than a contract anybody declared. The one consumer, risk.go, tests len() == 0, and the value is never marshalled, so nil and empty are indistinguishable to every caller. The non-nil contract elsewhere in this repo is stated for lists that do reach JSON, where null and [] differ to a client. This one does not, so the assertion would have failed a later `var out []*Signal` for a difference nothing can observe. The loop that checks no foreign signal leaked in stays. --- plugins/sharedsignals/ssftest/event_cases.go | 1 - plugins/sharedsignals/ssftest/stream_cases.go | 17 +++++++++++------ plugins/sso/ssotest/cases.go | 9 +++++++-- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/plugins/sharedsignals/ssftest/event_cases.go b/plugins/sharedsignals/ssftest/event_cases.go index 308fa81f..f474a163 100644 --- a/plugins/sharedsignals/ssftest/event_cases.go +++ b/plugins/sharedsignals/ssftest/event_cases.go @@ -291,7 +291,6 @@ func testActiveSignalsAreEnvironmentScoped(t *testing.T, f Fixture) { // time.Now() unqualified, matching the risk path's own call in risk.go. other, err := f.Store.ListActiveSignals(ctx, f.AppID, otherEnv, f.UserID, time.Now()) require.NoError(t, err) - assert.NotNil(t, other, "a zero result must be an empty slice rather than nil") for _, got := range other { assert.NotEqual(t, sig.ID, got.ID, "a signal from another environment leaked into env %s", otherEnv) diff --git a/plugins/sharedsignals/ssftest/stream_cases.go b/plugins/sharedsignals/ssftest/stream_cases.go index e7aa75bb..0fa33d5d 100644 --- a/plugins/sharedsignals/ssftest/stream_cases.go +++ b/plugins/sharedsignals/ssftest/stream_cases.go @@ -226,11 +226,16 @@ func testSubjectLinkUpsertIsConcurrencySafe(t *testing.T, f Fixture) { assert.NoError(t, err, "concurrent upsert %d must not surface a constraint error", i) } - // The storm proves no writer surfaces a raw constraint error, but real - // concurrency leaves no way to know which of the n writes landed last. - // One more deterministic write after it settles pins that down: if this - // one, and only this one, comes back, then exactly one row survived and - // the operation is genuinely last-write-wins. + // The loop above is also what establishes that exactly one row exists. A + // second row for this tuple could only come from a writer that hit the + // unique index on (app_id, env_id, issuer, subject) and reported it, and + // that writer would have failed the loop. On memory there is no index and + // the mutex does that work instead. The read below is a FindOne either + // way, so it is not what proves the count. + // + // What the storm leaves undetermined is which of the n writes landed + // last, because real concurrency gives no way to know. One more + // deterministic write after it settles pins that down. last := id.NewUserID() require.NoError(t, f.Store.UpsertSubjectLink(ctx, &ssf.SubjectLink{ ID: id.NewSSFLinkID(), AppID: f.AppID, EnvID: f.EnvID, @@ -241,5 +246,5 @@ func testSubjectLinkUpsertIsConcurrencySafe(t *testing.T, f Fixture) { got, err := f.Store.GetSubjectLink(ctx, f.AppID, f.EnvID, issuer, subject) require.NoError(t, err) assert.Equal(t, last, got.UserID, - "exactly one row must survive the race, holding the last-written UserID") + "the upsert must be last-write-wins, not first-write-wins") } diff --git a/plugins/sso/ssotest/cases.go b/plugins/sso/ssotest/cases.go index 64dab3a0..0dbb7d75 100644 --- a/plugins/sso/ssotest/cases.go +++ b/plugins/sso/ssotest/cases.go @@ -35,11 +35,16 @@ func testConnectionCRUD(t *testing.T, f Fixture) { func testConnectionNotFound(t *testing.T, f Fixture) { ctx := context.Background() + // The sentinel, not a bare Error: callers branch on ErrConnectionNotFound + // to tell "no SSO configured for this domain" from a backend that failed, + // and the first sends the user to password login while the second must + // surface. A backend returning some other error passes assert.Error and + // breaks that branch. _, err := f.Store.GetConnection(ctx, id.NewSSOConnectionID()) - assert.Error(t, err, "an unknown connection id must not resolve") + assert.ErrorIs(t, err, sso.ErrConnectionNotFound, "an unknown connection id must not resolve") _, err = f.Store.GetConnectionByDomain(ctx, f.AppID, unique("absent")+".test") - assert.Error(t, err, "an unconfigured domain must not resolve") + assert.ErrorIs(t, err, sso.ErrConnectionNotFound, "an unconfigured domain must not resolve") } // testDomainLookupIsAppScoped is the one that matters most in this store.