diff --git a/Makefile b/Makefile --- a/Makefile +++ b/Makefile @@ -187,10 +187,10 @@ [ -n "$$FLAGS" ] || { echo "$(RED)✗ test-db-prepare printed no concurrency flags$(RESET)"; exit 1; }; \ echo " concurrency budget: $$FLAGS"; \ go test -tags integration $$FLAGS ./cmd/... ./internal/... ./tests/... @echo "" - @echo "$(YELLOW)Note: tests/integration needs a PDS and Jetstream as well as Postgres,$(RESET)" - @echo "$(YELLOW)and its TestMain now says so up front — without 'make dev-up' the$(RESET)" - @echo "$(YELLOW)package fails once, naming the address it could not reach, instead of$(RESET)" - @echo "$(YELLOW)skipping test by test and reporting green. 'make ci' remains the gate.$(RESET)" + @echo "$(YELLOW)Note: some packages need a PDS as well as Postgres, and each one's$(RESET)" + @echo "$(YELLOW)TestMain says so up front — without 'make dev-up' the package fails$(RESET)" + @echo "$(YELLOW)once, naming the address it could not reach, instead of skipping test$(RESET)" + @echo "$(YELLOW)by test and reporting green. 'make ci' remains the gate.$(RESET)" test-e2e: ## T2 pipeline tier - brings up the hermetic stack and runs inside it @# docs/TEST_ARCHITECTURE.md §3.5. The hermetic stack publishes no host @@ -232,11 +232,20 @@ @echo "" @echo "$(CYAN)Contract manifest:$(RESET)" @go run ./cmd/contract-manifest @echo "$(GREEN)Running the pipeline tier against the dev stack (-tags e2e)...$(RESET)" + @echo "$(YELLOW) minus the reliability suite: it stops, starts and reconfigures the AppView$(RESET)" + @echo "$(YELLOW) CONTAINER, and this hatch grades a host-run 'make run' process instead.$(RESET)" @# run_pipeline_tier is the ONE definition of how T2 is invoked — the gate, @# 'make test-e2e' and this hatch all call it, so the flags cannot drift @# apart. Sourced here rather than copied for exactly that reason. - @bash -c 'source ./scripts/lib/runner-ready.sh && run_pipeline_tier' - @echo "$(GREEN)✓ Pipeline tier complete (against the dev stack)$(RESET)" + @# + @# -skip, not a t.Skip: skipping is banned in test bodies (§3.1) and the + @# pipeline tier fails outright on a SKIP event (scripts/e2e-runner.sh), + @# both for the same reason — a contract that opts out of proving itself is + @# indistinguishable from a broken pipeline. Excluding by name at the + @# selection level keeps that rule intact: these tests do not run here, and + @# they do not report anything either. + @bash -c 'source ./scripts/lib/runner-ready.sh && run_pipeline_tier -skip "^TestReliability"' + @echo "$(GREEN)✓ Pipeline tier complete (against the dev stack, without the reliability suite)$(RESET)" test-db-reset: ## Reset test database @echo "$(GREEN)Resetting test database...$(RESET)" diff --git a/cmd/contract-manifest/main.go b/cmd/contract-manifest/main.go --- a/cmd/contract-manifest/main.go +++ b/cmd/contract-manifest/main.go @@ -37,14 +37,23 @@ // switched off long before then. This is the burn-down, and it // ratchets: a pending collection that gains a contract makes its // pending entry STALE, which fails — exactly like ci-report's // stale-allowlist rule. Entries can only leave the file. +// SPENT as of task 16: the file is empty and both entry points +// pass -allow-pending=false (scripts/lib/runner-ready.sh), so a +// new line in it now fails the gate rather than deferring the +// contract. The state is kept because the flag is what turns it +// off, and a flag with no state behind it is not a ratchet. // missing neither. This fails the gate. // // Markers naming a collection nothing consumes fail too, in both directions: a // stale marker is a contract testing a pipeline that no longer exists, and a // stale pending entry is a promise to write one. // -// Phase 6 (task 20) empties pending_contracts.txt and flips -allow-pending to -// false, at which point "contracted" is the only passing state. +// The flag's DEFAULT is still true, so an ad-hoc `go run ./cmd/contract-manifest` +// reports the three states rather than judging the burn-down. The gate is where +// the judgement belongs and where the flip lives: check_contract_manifest in +// scripts/lib/runner-ready.sh passes -allow-pending=false, so both `make ci` +// and `make test-e2e` accept only "contracted". Task 20 verifies that call site +// still carries the flag as part of the phase-6 enforcement sweep. package main import ( diff --git a/docker-compose.ci.two-feed.yml b/docker-compose.ci.two-feed.yml new file mode 100644 --- /dev/null +++ b/docker-compose.ci.two-feed.yml @@ -0,0 +1,63 @@ +# Overlay: the AppView consuming TWO overlapping Jetstream feeds. +# +# Applied to docker-compose.ci.yml for exactly one scenario — the pipeline +# tier's TestReliabilityOverlappingFeedsDoNotDoubleIndex — and removed again +# immediately afterwards. It is never part of a normal `make ci` stack. +# +# WHY THIS EXISTS AS A FILE RATHER THAN AN ENVIRONMENT VARIABLE +# +# The AppView's feed topology comes from JETSTREAM_FEEDS in .env.ci, and +# Compose's `environment:` always wins over `env_file:`. So an interpolated +# `JETSTREAM_FEEDS: ${SOMETHING:-}` in the base file would silently +# become the real source of truth for every run, with .env.ci's line dead and +# nobody the wiser. Keeping the override in a separate file that is only ever +# passed for one scenario leaves the base stack — the one the gate grades — +# reading its topology from exactly one place. +# +# It also means the control channel that applies this (scripts/lib/ci-stack.sh) +# carries no arguments at all: the container asks for "the two-feed AppView" by +# name, and the host decides what that is. +# +# WHY BOTH KEYS ARE NEW, AND WHY THAT IS THE WHOLE POINT +# +# Neither key is the base stack's (.env.ci uses `self`) and neither is +# jetstream.PrimaryFeedKey ("bsky"). That is load-bearing, not tidiness. +# +# jetstream.FeedConsumerName derives a connector's name — and therefore its +# persisted-cursor row — from the feed key. Reusing `self` here would hand one +# of the two connectors the cursor the base topology has been writing all run, +# so it would resume from that cursor MINUS the five-second rewind and replay +# history while its partner live-tailed. The scenario's central claim is that +# BOTH connectors handled the new vote; a connector replaying history satisfies +# an events-processed delta without ever having seen that vote, so the overlap +# would go unproven while the test passed. An earlier version of this file used +# `self` + `self2` and had exactly that hole. +# +# With two brand-new keys neither connector has a cursor row, so both live-tail +# from the moment they connect (connector.go's dialURL omits the cursor +# parameter entirely when it is zero) and every event either of them counts +# after the scenario's baseline snapshot arrived live. The test asserts the +# connector NAMES it expects for precisely this reason — see +# twoFeedConnectorNames in tests/e2e/reliability_test.go, which fails loudly if +# these keys ever drift back toward the base stack's. +# +# The `self` connector does not run during the window. When the single-feed +# configuration is restored it resumes from its own untouched cursor and replays +# what it missed, which the rev gate and the consumers' upserts make a no-op. +# +# WHAT THE TOPOLOGY IS AND WHY IT IS SAFE +# +# Two feed keys pointing at the SAME Jetstream, so every commit is delivered +# twice — once per connector. That is the shape production runs (bsky + self, +# both carrying our own PDS' repos) reduced to the one property worth testing +# hermetically: overlap. The scenario waits for both connectors to report +# connected before it writes anything, because a live-tailing connector cannot +# see an event committed before it dialled. +# +# cmd/server refuses to boot multi-feed unless every consumer is rev-gated +# (consumers.go's FAIL CLOSED block), so this file is also a standing check that +# the gate's own wiring still satisfies that rule. +services: + appview: + environment: + JETSTREAM_FEEDS: "overlap-a=ws://localhost:6008;overlap-b=ws://localhost:6008" diff --git a/docker-compose.ci.yml b/docker-compose.ci.yml --- a/docker-compose.ci.yml +++ b/docker-compose.ci.yml @@ -18,12 +18,12 @@ # service therefore listens on the port the rest of the stack expects, # matching the dev stack's published ports (PDS 3001, PLC 3002, AppView # 8081, Postgres 5435/5434/5436). # -# * The test suite needs no changes. tests/e2e/*.go hardcodes -# "http://localhost:8081" and "localhost:3001" with no env indirection, and -# tests/integration falls back to "localhost:3001" / "localhost:5434" when -# PDS_URL / TEST_DATABASE_URL are unset. In this namespace those literals -# resolve to the CI stack — so the fallback that would silently hit your dev -# PDS from the host is correct here instead of dangerous. +# * The test suite needs no changes. Endpoints come from testkit.Endpoints(), +# which defaults to "localhost:3001" / "localhost:5434" / "localhost:8081" +# when PDS_URL / POSTGRES_TEST_* / APPVIEW_URL are unset. In this namespace +# those defaults resolve to the CI stack — so the fallback that would +# silently hit your dev PDS from the host is correct here instead of +# dangerous. # # * Cross-service URLs stay coherent. The PDS fetches the AppView's OAuth # client metadata at APPVIEW_PUBLIC_URL (http://127.0.0.1:8081); with @@ -82,9 +82,10 @@ depends_on: netns: condition: service_healthy - # Test database, read directly by tests/integration. Those tests run their own - # migrations via goose.Up(db, "../../internal/db/migrations"), so this needs no - # migration step either — only to exist and be empty. + # Test database. The integration tier reaches it only through testkit, which + # migrates a template once and hands each test a clone, so this needs no + # migration step of its own — only to exist and be empty. (It predates that: + # the old tests/integration package ran goose against it directly.) postgres-test: image: postgres:15 network_mode: "service:netns" @@ -341,6 +342,19 @@ # Inside the egress-blocked stack the cache is the only source of modules, # so a gap in it should say "module lookup disabled by GOPROXY=off" # instead of surfacing as a DNS failure that reads like broken networking. GOPROXY: "off" + # Where the pipeline tier's reliability scenarios ask the HOST to restart + # or reconfigure the AppView, which nothing inside this network namespace + # can do for itself. The directory is inside the bind-mounted checkout + # (and gitignored); scripts/lib/ci-stack.sh serves it while a suite runs. + # Set here rather than defaulted in Go alone so the coupling between the + # container's view (/src) and the host's (.ci-out) is visible from the + # compose file that creates it. + # + # Suffixed with the project name, and the default MUST stay in step with + # ci-stack.sh's own default: two stacks from one checkout (which is what + # COVES_CI_PROJECT is for) sharing one channel would let either run's + # watcher answer the other's requests and restart the wrong AppView. + COVES_STACK_CONTROL_DIR: /src/.ci-out/stack-control-${COVES_CI_PROJECT:-coves-ci} volumes: # The checkout. Read-write because the Go toolchain and goose both expect # a normal working tree; scripts/ci.sh asserts afterwards that the run diff --git a/internal/api/handlers/community/harness_test.go b/internal/api/handlers/community/harness_test.go new file mode 100644 --- /dev/null +++ b/internal/api/handlers/community/harness_test.go @@ -0,0 +1,32 @@ +//go:build integration + +package community_test + +import ( + "os" + "testing" + + "Coves/tests/testkit" +) + +// TestMain sets the infrastructure floor for this package's integration build. +// +// It lives in a build-tagged file on purpose. A TestMain governs the WHOLE test +// binary, and under -tags integration the tagged and untagged files of this +// directory compile into one binary — so this function also runs for the +// in-package unit tests in get_test.go, list_test.go and friends. Those tests +// are pure handler tests with hand-written fakes and must keep building and +// running without a socket in sight, which is exactly what the tag guarantees: +// without -tags integration this file does not exist and the unit build has no +// TestMain at all. +// +// The floor is Postgres and nothing more. The only integration tests here — +// viewer_state_test.go — drive the get and list handlers over a real +// communities repository so that the viewer.subscribed field is answered from +// the real subscriptions table rather than from a fake. Authentication is +// injected straight into the request context, no PDS is dialled, and no +// firehose event is consumed, so demanding a PDS or a Jetstream would make the +// whole package fail for infrastructure it never touches. +func TestMain(m *testing.M) { + os.Exit(testkit.Main(m, testkit.RequirePostgres)) +} diff --git a/internal/api/handlers/community/viewer_state_test.go b/internal/api/handlers/community/viewer_state_test.go new file mode 100644 --- /dev/null +++ b/internal/api/handlers/community/viewer_state_test.go @@ -0,0 +1,355 @@ +//go:build integration + +// Viewer state is the one part of a community response that the handler cannot +// answer from the service layer: social.coves.community.get and +// social.coves.community.list both promise that "viewer state will be included +// if authenticated", and both satisfy that promise by going back to the +// communities repository with the caller's DID and asking which of the +// communities in the response the caller is subscribed to. +// +// That second query is what these tests cover, and it is why they need a real +// database. The in-package unit tests in get_test.go and list_test.go drive the +// same handlers against a hand-written repository, which proves the handler +// calls the seam but cannot prove the SQL behind it answers correctly — a +// subscription lookup that silently returned the empty set would pass every one +// of them. Here the subscriptions are real rows, so a wrong join, a wrong +// column or a missing WHERE shows up as viewer.subscribed being wrong. +// +// The file is in the external test package because it imports +// internal/db/postgres, which pulls in the domain; the established form for +// every relocated integration test in this tree is package foo_test. +package community_test + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "testing" + "time" + + "Coves/internal/api/handlers/community" + "Coves/internal/api/middleware" + "Coves/internal/core/communities" + "Coves/internal/db/postgres" + "Coves/tests/fixtures" + "Coves/tests/testkit" + + "github.com/bluesky-social/indigo/atproto/auth/oauth" + "github.com/go-chi/chi/v5" +) + +// repositoryBackedService is a communities.Service that answers only the reads +// the two endpoints under test make, and answers them from the real repository. +// +// The point of the fake is to take the SERVICE out of the picture without +// taking the DATABASE out of it: the handler asks the service for the +// communities and asks the repository for the viewer's subscriptions, and it is +// the second call this file is about. Every method the endpoints never reach +// returns an error rather than a zero value, so a handler that starts calling +// one fails loudly instead of quietly seeing an empty result. +type repositoryBackedService struct { + repo communities.Repository +} + +func (s *repositoryBackedService) ListCommunities(ctx context.Context, req communities.ListCommunitiesRequest) ([]*communities.Community, error) { + return s.repo.List(ctx, req) +} + +func (s *repositoryBackedService) GetCommunity(ctx context.Context, identifier string) (*communities.Community, error) { + return s.repo.GetByDID(ctx, identifier) +} + +func (s *repositoryBackedService) GetByDID(ctx context.Context, did string) (*communities.Community, error) { + return s.repo.GetByDID(ctx, did) +} + +func (s *repositoryBackedService) CreateCommunity(context.Context, communities.CreateCommunityRequest) (*communities.Community, error) { + return nil, fmt.Errorf("repositoryBackedService: CreateCommunity is not part of the viewer-state seam") +} + +func (s *repositoryBackedService) UpdateCommunity(context.Context, communities.UpdateCommunityRequest) (*communities.Community, error) { + return nil, fmt.Errorf("repositoryBackedService: UpdateCommunity is not part of the viewer-state seam") +} + +func (s *repositoryBackedService) SearchCommunities(context.Context, communities.SearchCommunitiesRequest) ([]*communities.Community, int, error) { + return nil, 0, fmt.Errorf("repositoryBackedService: SearchCommunities is not part of the viewer-state seam") +} + +func (s *repositoryBackedService) SubscribeToCommunity(context.Context, *oauth.ClientSessionData, string, int) (*communities.Subscription, error) { + return nil, fmt.Errorf("repositoryBackedService: SubscribeToCommunity is not part of the viewer-state seam") +} + +func (s *repositoryBackedService) UnsubscribeFromCommunity(context.Context, *oauth.ClientSessionData, string) error { + return fmt.Errorf("repositoryBackedService: UnsubscribeFromCommunity is not part of the viewer-state seam") +} + +func (s *repositoryBackedService) GetUserSubscriptions(context.Context, string, int, int) ([]*communities.Subscription, error) { + return nil, fmt.Errorf("repositoryBackedService: GetUserSubscriptions is not part of the viewer-state seam") +} + +func (s *repositoryBackedService) GetCommunitySubscribers(context.Context, string, int, int) ([]*communities.Subscription, error) { + return nil, fmt.Errorf("repositoryBackedService: GetCommunitySubscribers is not part of the viewer-state seam") +} + +func (s *repositoryBackedService) BlockCommunity(context.Context, *oauth.ClientSessionData, string) (*communities.CommunityBlock, error) { + return nil, fmt.Errorf("repositoryBackedService: BlockCommunity is not part of the viewer-state seam") +} + +func (s *repositoryBackedService) UnblockCommunity(context.Context, *oauth.ClientSessionData, string) error { + return fmt.Errorf("repositoryBackedService: UnblockCommunity is not part of the viewer-state seam") +} + +func (s *repositoryBackedService) GetBlockedCommunities(context.Context, string, int, int) ([]*communities.CommunityBlock, error) { + return nil, fmt.Errorf("repositoryBackedService: GetBlockedCommunities is not part of the viewer-state seam") +} + +func (s *repositoryBackedService) IsBlocked(context.Context, string, string) (bool, error) { + return false, fmt.Errorf("repositoryBackedService: IsBlocked is not part of the viewer-state seam") +} + +func (s *repositoryBackedService) GetMembership(context.Context, string, string) (*communities.Membership, error) { + return nil, fmt.Errorf("repositoryBackedService: GetMembership is not part of the viewer-state seam") +} + +func (s *repositoryBackedService) ListCommunityMembers(context.Context, string, int, int) ([]*communities.Membership, error) { + return nil, fmt.Errorf("repositoryBackedService: ListCommunityMembers is not part of the viewer-state seam") +} + +func (s *repositoryBackedService) ValidateHandle(string) error { return nil } + +func (s *repositoryBackedService) ResolveCommunityIdentifier(_ context.Context, identifier string) (string, error) { + return identifier, nil +} + +func (s *repositoryBackedService) EnsureFreshToken(_ context.Context, community *communities.Community) (*communities.Community, error) { + return community, nil +} + +// viewerEnvelope is the slice of the lexicon response these tests read. Both +// endpoints use the same optional shape, and the two levels of pointer are +// load bearing: a nil Viewer means "the response omitted viewer state +// entirely", which is what an unauthenticated caller must see, while a +// non-nil Viewer with a nil Subscribed would mean the field was present but +// never filled in. Decoding into plain bools would collapse both mistakes into +// "false" and the unauthenticated cases below would pass no matter what. +type viewerEnvelope struct { + Subscribed *bool `json:"subscribed"` +} + +// authenticatedAs builds a router that runs the handler behind middleware which +// puts userDID in the request context, the same key the real OAuth middleware +// writes. Passing an empty DID gives the unauthenticated router. +func authenticatedAs(userDID, pattern string, handler http.HandlerFunc) chi.Router { + router := chi.NewRouter() + if userDID != "" { + router.Use(func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + next.ServeHTTP(w, req.WithContext(middleware.SetTestUserDID(req.Context(), userDID))) + }) + }) + } + router.Get(pattern, handler) + return router +} + +// seedCommunities inserts count communities and returns their DIDs. +func seedCommunities(t *testing.T, repo communities.Repository, label string, count int) []string { + t.Helper() + + ctx := context.Background() + dids := make([]string, count) + for i := 0; i < count; i++ { + id := testkit.UniqueID(t) + dids[i] = fixtures.DID(id) + community := &communities.Community{ + DID: dids[i], + Handle: fmt.Sprintf("c-%s%d.coves.local", id, i), + Name: fmt.Sprintf("%s-%s-%d", label, id, i), + DisplayName: fmt.Sprintf("%s community %d", label, i), + OwnerDID: fixtures.InstanceDID(), + CreatedByDID: fixtures.DID("creator" + id), + HostedByDID: fixtures.InstanceDID(), + Visibility: "public", + CreatedAt: time.Now(), + UpdatedAt: time.Now(), + } + if _, err := repo.Create(ctx, community); err != nil { + t.Fatalf("creating %s community %d: %v", label, i, err) + } + } + return dids +} + +// subscribe records userDID as a subscriber of communityDID. +func subscribe(t *testing.T, repo communities.Repository, userDID, communityDID string) { + t.Helper() + + if _, err := repo.Subscribe(context.Background(), &communities.Subscription{ + UserDID: userDID, + CommunityDID: communityDID, + // 3 is the default content-visibility level; the endpoints under test + // only care that a subscription row exists. + ContentVisibility: 3, + SubscribedAt: time.Now(), + }); err != nil { + t.Fatalf("subscribing %s to %s: %v", userDID, communityDID, err) + } +} + +// TestCommunityGet_ViewerState covers social.coves.community.get: the response +// carries viewer.subscribed for an authenticated caller, and carries no viewer +// object at all for an anonymous one. +func TestCommunityGet_ViewerState(t *testing.T) { + t.Parallel() + db := testkit.DB(t) + + repo := postgres.NewCommunityRepository(db) + // Two communities so that "subscribed" and "not subscribed" are answered + // from the same database state: a handler that hardcoded either answer + // would fail one of the two subtests. + communityDIDs := seedCommunities(t, repo, "getviewer", 2) + + viewerDID := fixtures.DID("getviewer" + testkit.UniqueID(t)) + subscribe(t, repo, viewerDID, communityDIDs[0]) + + handler := community.NewGetHandler(&repositoryBackedService{repo: repo}, repo) + + get := func(t *testing.T, router chi.Router, communityDID string) *viewerEnvelope { + t.Helper() + + req := httptest.NewRequest(http.MethodGet, "/xrpc/social.coves.community.get?community="+communityDID, nil) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("GET community %s: expected 200, got %d: %s", communityDID, rec.Code, rec.Body.String()) + } + var response struct { + DID string `json:"did"` + Viewer *viewerEnvelope `json:"viewer"` + } + if err := json.NewDecoder(rec.Body).Decode(&response); err != nil { + t.Fatalf("decoding get response: %v", err) + } + return response.Viewer + } + + t.Run("authenticated subscriber sees viewer.subscribed=true", func(t *testing.T) { + router := authenticatedAs(viewerDID, "/xrpc/social.coves.community.get", handler.HandleGet) + + viewer := get(t, router, communityDIDs[0]) + if viewer == nil || viewer.Subscribed == nil { + t.Fatalf("expected populated viewer state, got %+v", viewer) + } + if !*viewer.Subscribed { + t.Errorf("expected viewer.subscribed=true for subscribed community %s", communityDIDs[0]) + } + }) + + t.Run("authenticated non-subscriber sees viewer.subscribed=false", func(t *testing.T) { + router := authenticatedAs(viewerDID, "/xrpc/social.coves.community.get", handler.HandleGet) + + viewer := get(t, router, communityDIDs[1]) + if viewer == nil || viewer.Subscribed == nil { + t.Fatalf("expected populated viewer state, got %+v", viewer) + } + if *viewer.Subscribed { + t.Errorf("expected viewer.subscribed=false for unsubscribed community %s", communityDIDs[1]) + } + }) + + t.Run("unauthenticated request has no viewer state", func(t *testing.T) { + router := authenticatedAs("", "/xrpc/social.coves.community.get", handler.HandleGet) + + if viewer := get(t, router, communityDIDs[0]); viewer != nil { + t.Errorf("expected the viewer object to be omitted for an anonymous caller, got %+v", viewer) + } + }) +} + +// TestCommunityList_ViewerState covers social.coves.community.list, where the +// viewer's subscriptions have to be matched up against a whole page of +// communities rather than a single one — the case where an off-by-one in the +// join would show up as the right count of subscriptions attached to the wrong +// communities. +func TestCommunityList_ViewerState(t *testing.T) { + t.Parallel() + db := testkit.DB(t) + + repo := postgres.NewCommunityRepository(db) + communityDIDs := seedCommunities(t, repo, "listviewer", 3) + + viewerDID := fixtures.DID("listviewer" + testkit.UniqueID(t)) + // Subscribed to the first and the last, deliberately skipping the middle: + // a handler that attached viewer state positionally rather than by DID + // would mislabel the second and third entries. + subscribe(t, repo, viewerDID, communityDIDs[0]) + subscribe(t, repo, viewerDID, communityDIDs[2]) + + handler := community.NewListHandler(&repositoryBackedService{repo: repo}, repo) + + list := func(t *testing.T, router chi.Router) map[string]*viewerEnvelope { + t.Helper() + + req := httptest.NewRequest(http.MethodGet, "/xrpc/social.coves.community.list?limit=50", nil) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("LIST communities: expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + var response struct { + Communities []struct { + DID string `json:"did"` + Viewer *viewerEnvelope `json:"viewer"` + } `json:"communities"` + } + if err := json.NewDecoder(rec.Body).Decode(&response); err != nil { + t.Fatalf("decoding list response: %v", err) + } + + byDID := make(map[string]*viewerEnvelope, len(response.Communities)) + for _, entry := range response.Communities { + byDID[entry.DID] = entry.Viewer + } + return byDID + } + + t.Run("authenticated user sees viewer.subscribed per community", func(t *testing.T) { + router := authenticatedAs(viewerDID, "/xrpc/social.coves.community.list", handler.HandleList) + viewers := list(t, router) + + expected := map[string]bool{ + communityDIDs[0]: true, + communityDIDs[1]: false, + communityDIDs[2]: true, + } + for communityDID, wantSubscribed := range expected { + viewer, present := viewers[communityDID] + if !present { + t.Errorf("community %s missing from the listing", communityDID) + continue + } + if viewer == nil || viewer.Subscribed == nil { + t.Errorf("community %s: expected populated viewer state, got %+v", communityDID, viewer) + continue + } + if *viewer.Subscribed != wantSubscribed { + t.Errorf("community %s: expected subscribed=%v, got %v", + communityDID, wantSubscribed, *viewer.Subscribed) + } + } + }) + + t.Run("unauthenticated request has no viewer state", func(t *testing.T) { + router := authenticatedAs("", "/xrpc/social.coves.community.list", handler.HandleList) + + for communityDID, viewer := range list(t, router) { + if viewer != nil { + t.Errorf("community %s carried viewer state for an anonymous caller: %+v", communityDID, viewer) + } + } + }) +} diff --git a/internal/api/handlers/imageproxy/avatar_serving_test.go b/internal/api/handlers/imageproxy/avatar_serving_test.go new file mode 100644 --- /dev/null +++ b/internal/api/handlers/imageproxy/avatar_serving_test.go @@ -0,0 +1,261 @@ +//go:build integration + +// These are the image-proxy cases that a mock upstream cannot answer, and they +// are the reason this package's integration floor includes a real PDS. +// +// Everything in proxy_serving_test.go states the upstream's response directly: +// here are the bytes, here is the status. That proves what the proxy does with +// an answer, but it assumes the answer's shape — that a PDS really does serve +// com.atproto.sync.getBlob at that path, with those query parameters, for a +// blob uploaded as part of a profile record, and that the community's DID +// really does resolve to that PDS through the directory. Every one of those +// assumptions is a place the proxy could silently stop working against real +// infrastructure while every mock-backed test stayed green. +// +// So this file takes the long way round: provision a community account on the +// PDS with an avatar, let the PDS assign the blob its CID, and then ask the +// proxy for that CID by the community's DID and check the pixels that come +// back. The blob has to be REFERENCED by the community's profile record for the +// PDS to keep it — an unreferenced upload is garbage-collected — which is why +// the community is created through communities.Service rather than by writing a +// row. +// +// The file is in the external test package because it imports +// internal/api/routes and internal/db/postgres, both of which pull in this +// handler package or the domain; in-package that would be an import cycle. +package imageproxy_test + +import ( + "context" + "fmt" + "image/color" + "io" + "net/http" + "testing" + "time" + + "Coves/internal/atproto/identity" + "Coves/internal/core/blobs" + "Coves/internal/core/communities" + "Coves/internal/db/postgres" + "Coves/tests/fixtures" + "Coves/tests/testkit" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// provisionedAvatar is a community that exists on the real PDS, together with +// the CID the PDS assigned its avatar blob. +type provisionedAvatar struct { + communityDID string + avatarCID string + resolver identity.Resolver +} + +// provisionCommunityAvatar creates a community account on the test PDS with an +// avatar of the given size, and returns what the proxy needs to fetch it back. +// +// The identity resolver is the real one, backed by the test stack's PLC +// directory, because DID resolution is half of what this file is proving: the +// proxy is given nothing but a DID and has to find the PDS itself. +func provisionCommunityAvatar(t *testing.T, width, height int, fill color.Color) provisionedAvatar { + t.Helper() + + db := testkit.DB(t) + endpoints := testkit.Endpoints() + + identityConfig := identity.DefaultConfig() + identityConfig.PLCURL = endpoints.PLC.BaseURL + resolver := identity.NewResolver(db, identityConfig) + + // The handle domain comes from the stack rather than a literal: the PDS + // rejects a handle outside its configured service domains, and the + // provisioner builds the community's handle as c-{name}.{domain}. + handleDomain := endpoints.PDS.HandleDomain + communityService := communities.NewCommunityServiceWithPDSFactory( + postgres.NewCommunityRepository(db), + endpoints.PDS.BaseURL, + fixtures.InstanceDID(), + handleDomain, + communities.NewPDSAccountProvisioner(handleDomain, endpoints.PDS.BaseURL), + nil, // no PDS client factory: provisioning uses the password session + blobs.NewBlobService(endpoints.PDS.BaseURL), + ) + + // The provisioner prefixes "c-", and a PDS handle's local label is capped + // at 18 characters, so the name has to stay short — testkit.UniqueID is + // built to that budget. + name := "ip" + testkit.UniqueID(t) + community, err := communityService.CreateCommunity(context.Background(), communities.CreateCommunityRequest{ + Name: name, + DisplayName: "Image proxy avatar community", + Description: "Community whose avatar the image proxy serves", + Visibility: "public", + CreatedByDID: fixtures.DID("creator" + name), + HostedByDID: fixtures.InstanceDID(), + AllowExternalDiscovery: true, + AvatarBlob: testkit.TestPNGColor(width, height, fill), + AvatarMimeType: "image/png", + }) + require.NoError(t, err, "provisioning a community with an avatar on the PDS") + require.NotEmpty(t, community.AvatarCID, "the PDS must have assigned the avatar blob a CID") + + return provisionedAvatar{ + communityDID: community.DID, + avatarCID: community.AvatarCID, + resolver: resolver, + } +} + +// TestImageProxy_ServesRealPDSAvatar fetches an avatar the test PDS actually +// holds, through DID resolution, and checks both the image and the caching +// contract the CDN in front of this endpoint depends on. +func TestImageProxy_ServesRealPDSAvatar(t *testing.T) { + t.Parallel() + + avatar := provisionCommunityAvatar(t, 200, 200, color.RGBA{R: 100, G: 150, B: 200, A: 255}) + server := newProxyServer(t, avatar.resolver, defaultFetchTimeout) + url := proxyURL(server, "avatar_small", avatar.communityDID, avatar.avatarCID) + + t.Run("the avatar comes back re-encoded at the preset size", func(t *testing.T) { + resp, body := fetch(t, url, nil) + + require.Equal(t, http.StatusOK, resp.StatusCode, "body: %s", body) + assert.Equal(t, "image/jpeg", resp.Header.Get("Content-Type")) + assertImageSize(t, body, 360, 360) + }) + + t.Run("the response is immutably cacheable", func(t *testing.T) { + resp, _ := fetch(t, url, nil) + + // A preset plus a content-addressed CID names bytes that can never + // change, so the response is safe to cache forever — that is the whole + // economic argument for the proxy, and a weakened header here would + // quietly send every view back to the PDS. + assert.Equal(t, "public, max-age=31536000, immutable", resp.Header.Get("Cache-Control")) + assert.Equal(t, fmt.Sprintf(`"avatar_small-%s"`, avatar.avatarCID), resp.Header.Get("ETag")) + }) + + t.Run("a matching ETag is answered with 304 and no body", func(t *testing.T) { + resp, _ := fetch(t, url, nil) + etag := resp.Header.Get("ETag") + require.NotEmpty(t, etag, "the first response must carry an ETag to revalidate against") + + conditional, body := fetch(t, url, http.Header{"If-None-Match": []string{etag}}) + + assert.Equal(t, http.StatusNotModified, conditional.StatusCode) + assert.Empty(t, body, "a 304 must not carry a body") + }) + + t.Run("a stale ETag is answered with the full image", func(t *testing.T) { + resp, body := fetch(t, url, http.Header{"If-None-Match": []string{`"wrong-etag-value"`}}) + + assert.Equal(t, http.StatusOK, resp.StatusCode) + assert.NotEmpty(t, body, "a revalidation miss must return the image") + }) + + t.Run("a CID the community's PDS does not hold is a 404", func(t *testing.T) { + // A well-formed CIDv1 (raw codec, sha256) that passes validation and + // then fails to exist, so the 404 comes from the PDS rather than from + // the parser. + absent := "bafkreiemeosfdll427qzow5tipvctigjebyvi6ketznqrau2ydhzyggt7i" + + resp, _ := fetch(t, proxyURL(server, "avatar_small", avatar.communityDID, absent), nil) + + assert.Equal(t, http.StatusNotFound, resp.StatusCode) + }) +} + +// TestImageProxy_SecondFetchIsServedFromCache checks that a repeated request +// for the same preset and CID is answered from the on-disk cache instead of +// going back to the PDS. +// +// It asserts on the CACHE, not on the clock. The obvious version of this test +// times both requests and expects the second to be faster, which is a coin flip +// under CI load and says nothing useful when it fails. Cutting the upstream off +// after the first request states the property directly: if a later response +// still arrives intact, it cannot have come from the PDS. +// +// The retry loop is not defensive padding. ImageProxyService.GetImage writes to +// the cache in a goroutine so the response is not held up by disk IO, which +// means the entry is not guaranteed to exist the instant the first response +// lands. WaitFor is how the suite expresses "eventually, and say so if not" +// without a sleep. +func TestImageProxy_SecondFetchIsServedFromCache(t *testing.T) { + t.Parallel() + + avatar := provisionCommunityAvatar(t, 150, 150, color.RGBA{R: 50, G: 100, B: 150, A: 255}) + + // A resolver that can be pointed at a dead address, so later requests have + // no working upstream to fall back on. + resolver := &switchableResolver{delegate: avatar.resolver} + server := newProxyServer(t, resolver, defaultFetchTimeout) + url := proxyURL(server, "avatar", avatar.communityDID, avatar.avatarCID) + + first, firstBody := fetch(t, url, nil) + require.Equal(t, http.StatusOK, first.StatusCode, "the cache-filling request must succeed: %s", firstBody) + assertImageSize(t, firstBody, 1000, 1000) + + resolver.redirectTo(unreachableURL(t)) + + var cachedStatus int + var cachedBody []byte + testkit.WaitFor(t, 10*time.Second, func() (bool, error) { + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, url, nil) + if err != nil { + return false, fmt.Errorf("building the request for the cached image: %w", err) + } + resp, err := http.DefaultClient.Do(req) + if err != nil { + return false, fmt.Errorf("requesting the cached image: %w", err) + } + defer func() { _ = resp.Body.Close() }() + + body, err := io.ReadAll(resp.Body) + if err != nil { + return false, fmt.Errorf("reading the cached image: %w", err) + } + cachedStatus, cachedBody = resp.StatusCode, body + // 502 means the cache write has not landed yet and the request fell + // through to the upstream that is now dead — the one status worth + // retrying. Anything else is the answer, right or wrong. + return resp.StatusCode != http.StatusBadGateway, nil + }, testkit.WithDescription("the processed avatar to be served from the on-disk cache")) + + assert.Equal(t, http.StatusOK, cachedStatus, + "a cached request must succeed with the PDS unreachable") + assert.Equal(t, firstBody, cachedBody, "a cache hit must return the same bytes as the miss did") +} + +// switchableResolver resolves through a real resolver until it is redirected, +// after which every DID resolves to one fixed host. +type switchableResolver struct { + delegate identity.Resolver + override string +} + +// redirectTo makes every subsequent resolution point at pdsURL. +// +// There is no lock because the redirect happens between two sequential +// requests in one goroutine, with no request in flight. +func (r *switchableResolver) redirectTo(pdsURL string) { r.override = pdsURL } + +func (r *switchableResolver) ResolveDID(ctx context.Context, did string) (*identity.DIDDocument, error) { + if r.override == "" { + return r.delegate.ResolveDID(ctx, did) + } + return (&fixedPDSResolver{pdsURL: r.override}).ResolveDID(ctx, did) +} + +func (r *switchableResolver) Resolve(ctx context.Context, identifier string) (*identity.Identity, error) { + return r.delegate.Resolve(ctx, identifier) +} + +func (r *switchableResolver) ResolveHandle(ctx context.Context, handle string) (did, pdsURL string, err error) { + return r.delegate.ResolveHandle(ctx, handle) +} + +func (r *switchableResolver) Purge(ctx context.Context, identifier string) error { + return r.delegate.Purge(ctx, identifier) +} diff --git a/internal/api/handlers/imageproxy/harness_test.go b/internal/api/handlers/imageproxy/harness_test.go new file mode 100644 --- /dev/null +++ b/internal/api/handlers/imageproxy/harness_test.go @@ -0,0 +1,34 @@ +//go:build integration + +package imageproxy_test + +import ( + "os" + "testing" + + "Coves/tests/testkit" +) + +// TestMain sets the infrastructure floor for this package's integration build. +// +// It lives in a build-tagged file on purpose. A TestMain governs the WHOLE test +// binary, and under -tags integration the tagged and untagged files of this +// directory compile into one binary — so this function also runs for the +// in-package unit tests in handler_test.go. Those are pure handler tests over a +// fake service and a fake resolver and must keep building and running without a +// socket in sight, which the tag guarantees: without -tags integration this +// file does not exist and the unit build has no TestMain at all. +// +// The floor is Postgres AND a real PDS, and only one of the two files here +// needs the second half. avatar_serving_test.go provisions a community account +// on the PDS, uploads a real avatar blob to it, and then asks the proxy to +// fetch that blob back through DID resolution — it is the only place the +// proxy's PDS fetcher is exercised against a server that behaves like a PDS +// rather than like an httptest handler someone wrote to look like one. +// proxy_serving_test.go needs neither: it stands the routed handler up over +// mock PDS servers in-process. The floor is the union because a TestMain cannot +// be scoped to a file, and probing up front is what turns "the stack is down" +// into one clear failure instead of a dozen confusing ones. +func TestMain(m *testing.M) { + os.Exit(testkit.Main(m, testkit.RequirePostgres, testkit.RequirePDS)) +} diff --git a/internal/api/handlers/imageproxy/proxy_serving_test.go b/internal/api/handlers/imageproxy/proxy_serving_test.go new file mode 100644 --- /dev/null +++ b/internal/api/handlers/imageproxy/proxy_serving_test.go @@ -0,0 +1,440 @@ +//go:build integration + +// The image proxy is one handler sitting on top of four collaborators — a URL +// route, a DID resolver, a PDS fetcher and an image processor — and almost +// every interesting behaviour is a property of the assembly rather than of any +// one part. The in-package unit tests in handler_test.go replace the whole +// imageproxy service with a fake and prove the handler maps a service error to +// a status code. These tests keep the real service, the real disk cache, the +// real processor and the real fetcher, and replace only the far side of the +// network: a PDS that is an httptest server rather than a PDS. +// +// That is what lets them assert the things the unit tests cannot — that the +// bytes coming out are a decodable JPEG of the preset's exact dimensions, that +// a preset which preserves aspect ratio really does, that an upstream returning +// HTML or a truncated PNG turns into a 4xx/5xx rather than a corrupt image, and +// that a proxy error is served as plain text so a browser's tag does not +// end up rendering a JSON blob. +// +// The mock PDS is not a compromise here: what these cases care about is what +// the proxy does with the bytes and the status it gets back, and an httptest +// server states those inputs directly instead of contriving them on a real +// server. The one thing it cannot prove — that a real PDS answers +// com.atproto.sync.getBlob the way the fetcher expects — is what +// avatar_serving_test.go is for. +// +// The file is in the external test package because it imports +// internal/api/routes, which imports this handler package; in-package that +// would be an import cycle. +package imageproxy_test + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "image/color" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "Coves/internal/api/handlers/imageproxy" + "Coves/internal/api/routes" + "Coves/internal/atproto/identity" + imageproxycore "Coves/internal/core/imageproxy" + "Coves/tests/testkit" + + "github.com/disintegration/imaging" + "github.com/go-chi/chi/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// defaultFetchTimeout is what the proxy is given for a fetch it is expected to +// complete. It is generous on purpose: these tests are not measuring latency, +// and a tight budget here would turn a loaded CI machine into a 502. +const defaultFetchTimeout = 30 * time.Second + +// fixedPDSResolver is an identity.Resolver that sends every DID to one PDS. +// +// Only ResolveDID is implemented because that is the only method the proxy +// calls: it turns the DID in the URL into the host it should fetch the blob +// from. The rest return errors rather than zero values so that a handler which +// started calling one fails loudly here instead of quietly resolving to "". +type fixedPDSResolver struct { + pdsURL string +} + +func (r *fixedPDSResolver) ResolveDID(_ context.Context, did string) (*identity.DIDDocument, error) { + return &identity.DIDDocument{ + DID: did, + Service: []identity.Service{{ + ID: "#atproto_pds", + Type: "AtprotoPersonalDataServer", + ServiceEndpoint: r.pdsURL, + }}, + }, nil +} + +func (r *fixedPDSResolver) Resolve(context.Context, string) (*identity.Identity, error) { + return nil, fmt.Errorf("fixedPDSResolver: Resolve is not part of the image-proxy path") +} + +func (r *fixedPDSResolver) ResolveHandle(context.Context, string) (did, pdsURL string, err error) { + return "", "", fmt.Errorf("fixedPDSResolver: ResolveHandle is not part of the image-proxy path") +} + +func (r *fixedPDSResolver) Purge(context.Context, string) error { return nil } + +// failingResolver is an identity.Resolver that cannot resolve anything, for the +// case where the proxy is asked about a DID the directory does not know. +type failingResolver struct{} + +func (failingResolver) Resolve(context.Context, string) (*identity.Identity, error) { + return nil, fmt.Errorf("resolution failed") +} + +func (failingResolver) ResolveHandle(context.Context, string) (did, pdsURL string, err error) { + return "", "", fmt.Errorf("resolution failed") +} + +func (failingResolver) ResolveDID(context.Context, string) (*identity.DIDDocument, error) { + return nil, fmt.Errorf("resolution failed") +} + +func (failingResolver) Purge(context.Context, string) error { return nil } + +// newProxyServer stands the routed image proxy up over a real service — real +// disk cache in a temp directory, real processor, real fetcher — pointed at +// whatever resolver it is given. +// +// It goes through routes.RegisterImageProxyRoutes rather than calling the +// handler directly because the URL shape (/img/{preset}/plain/{did}/{cid}) is +// part of the contract: a route pattern that stopped capturing the CID would +// leave every handler unit test passing. +func newProxyServer(t *testing.T, resolver identity.Resolver, fetchTimeout time.Duration) *httptest.Server { + t.Helper() + + cacheDir := t.TempDir() + cache, err := imageproxycore.NewDiskCache(cacheDir, 1, 0) + require.NoError(t, err, "creating the disk cache") + + service, err := imageproxycore.NewService( + cache, + imageproxycore.NewProcessor(), + imageproxycore.NewPDSFetcher(fetchTimeout, 10), + imageproxycore.Config{ + Enabled: true, + CachePath: cacheDir, + CacheMaxGB: 1, + FetchTimeout: fetchTimeout, + MaxSourceSizeMB: 10, + }, + ) + require.NoError(t, err, "creating the imageproxy service") + + router := chi.NewRouter() + routes.RegisterImageProxyRoutes(router, imageproxy.NewHandler(service, resolver)) + + server := httptest.NewServer(router) + t.Cleanup(server.Close) + return server +} + +// newBlobServer runs an httptest server that answers com.atproto.sync.getBlob +// from a map of CID to response, and 404s everything else — which is what a PDS +// does for a blob it does not hold. +func newBlobServer(t *testing.T, blobs map[string]func(http.ResponseWriter)) *httptest.Server { + t.Helper() + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !strings.HasPrefix(r.URL.Path, "/xrpc/com.atproto.sync.getBlob") { + w.WriteHeader(http.StatusNotFound) + return + } + if write, known := blobs[r.URL.Query().Get("cid")]; known { + write(w) + return + } + w.WriteHeader(http.StatusNotFound) + })) + t.Cleanup(server.Close) + return server +} + +// servePNG answers a getBlob request with the given PNG bytes. +func servePNG(data []byte) func(http.ResponseWriter) { + return func(w http.ResponseWriter) { + w.Header().Set("Content-Type", "image/png") + w.WriteHeader(http.StatusOK) + _, _ = w.Write(data) + } +} + +// unreachableURL returns the address of a server that has already been shut +// down, so a connection to it is refused immediately. +// +// This is how the "upstream is down" cases get a dead address without writing a +// port number into the test: a literal like localhost:9999 is a guess that +// something else on the machine might be listening on, and the suite forbids +// endpoint literals for exactly that reason. +func unreachableURL(t *testing.T) string { + t.Helper() + + server := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + url := server.URL + server.Close() + return url +} + +// proxyURL builds a request URL for the routed proxy. +func proxyURL(server *httptest.Server, preset, did, cid string) string { + return fmt.Sprintf("%s/img/%s/plain/%s/%s", server.URL, preset, did, cid) +} + +// fetch issues a GET and hands back the response with its body already read, so +// callers never have to remember to close it. +func fetch(t *testing.T, url string, header http.Header) (*http.Response, []byte) { + t.Helper() + + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, url, nil) + require.NoError(t, err, "building the request") + for name, values := range header { + req.Header[name] = values + } + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err, "GET %s", url) + defer func() { _ = resp.Body.Close() }() + + var body bytes.Buffer + _, err = body.ReadFrom(resp.Body) + require.NoError(t, err, "reading the response body") + return resp, body.Bytes() +} + +// assertImageSize decodes body and asserts its dimensions. +func assertImageSize(t *testing.T, body []byte, wantWidth, wantHeight int) { + t.Helper() + + img, err := imaging.Decode(bytes.NewReader(body)) + require.NoError(t, err, "the proxy must return decodable image data") + + bounds := img.Bounds() + assert.Equal(t, wantWidth, bounds.Dx(), "width") + assert.Equal(t, wantHeight, bounds.Dy(), "height") +} + +// TestImageProxy_ServesProcessedBlob covers the success path end to end: a PNG +// on the upstream comes back as a JPEG at the preset's dimensions, and a CID +// the upstream does not hold comes back as a 404 rather than as an empty 200. +func TestImageProxy_ServesProcessedBlob(t *testing.T) { + t.Parallel() + + const cid = "bafybeimockimagetest123" + did := "did:plc:" + testkit.UniqueID(t) + + upstream := newBlobServer(t, map[string]func(http.ResponseWriter){ + cid: servePNG(testkit.TestPNGColor(100, 100, color.RGBA{R: 255, G: 128, B: 64, A: 255})), + }) + server := newProxyServer(t, &fixedPDSResolver{pdsURL: upstream.URL}, defaultFetchTimeout) + + t.Run("a stored blob is re-encoded to the preset", func(t *testing.T) { + resp, body := fetch(t, proxyURL(server, "avatar", did, cid), nil) + + assert.Equal(t, http.StatusOK, resp.StatusCode) + // The proxy always re-encodes: whatever the source format was, clients + // get one predictable format back. + assert.Equal(t, "image/jpeg", resp.Header.Get("Content-Type")) + // The avatar preset upscales a 100x100 source to its full 1000x1000. + assertImageSize(t, body, 1000, 1000) + }) + + t.Run("a blob the PDS does not hold is a 404", func(t *testing.T) { + resp, _ := fetch(t, proxyURL(server, "avatar", did, "nonexistentcid"), nil) + + assert.Equal(t, http.StatusNotFound, resp.StatusCode) + }) +} + +// TestImageProxy_UpstreamFailuresAreBadGateway covers the two ways the proxy can +// fail to reach the bytes: the DID does not resolve, or the PDS it resolves to +// does not answer. +// +// Both are 502 rather than 404 or 500 on purpose. The request was well formed +// and the resource may well exist; what failed is an upstream the client has no +// way to fix, and a 404 would tell caches and clients the image is gone. +func TestImageProxy_UpstreamFailuresAreBadGateway(t *testing.T) { + t.Parallel() + + // Well-formed CIDs: these must travel past validation so that the failure + // under test is the fetch, not the parse. + const validCID = "bafyreihgdyzzpkkzq2izfnhcmm77ycuacvkuziwbnqxfxtqsz7tmxwhnshi" + did := "did:plc:" + testkit.UniqueID(t) + + t.Run("the resolved PDS refuses the connection", func(t *testing.T) { + server := newProxyServer(t, &fixedPDSResolver{pdsURL: unreachableURL(t)}, time.Second) + + resp, _ := fetch(t, proxyURL(server, "avatar", did, validCID), nil) + + assert.Equal(t, http.StatusBadGateway, resp.StatusCode) + }) + + t.Run("the DID does not resolve", func(t *testing.T) { + server := newProxyServer(t, failingResolver{}, time.Second) + + resp, _ := fetch(t, proxyURL(server, "avatar", did, validCID), nil) + + assert.Equal(t, http.StatusBadGateway, resp.StatusCode) + }) +} + +// TestImageProxy_UndecodableUpstreamBytes covers what happens when the fetch +// succeeds but the bytes are not an image the processor can read. +// +// The proxy must not pass them through: a 200 carrying HTML under an image +// Content-Type is how a proxy becomes an XSS vector, and a truncated image is +// how a cache ends up holding a permanently broken entry. +func TestImageProxy_UndecodableUpstreamBytes(t *testing.T) { + t.Parallel() + + did := "did:plc:" + testkit.UniqueID(t) + upstream := newBlobServer(t, map[string]func(http.ResponseWriter){ + "textdata": func(w http.ResponseWriter) { + w.Header().Set("Content-Type", "text/plain") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("this is not an image")) + }, + "corruptedimage": func(w http.ResponseWriter) { + w.Header().Set("Content-Type", "image/png") + w.WriteHeader(http.StatusOK) + // A PNG signature with nothing behind it. + _, _ = w.Write([]byte{0x89, 0x50, 0x4E, 0x47, 0x00, 0x00}) + }, + "emptybody": func(w http.ResponseWriter) { + w.WriteHeader(http.StatusOK) + }, + }) + server := newProxyServer(t, &fixedPDSResolver{pdsURL: upstream.URL}, defaultFetchTimeout) + + for _, cid := range []string{"textdata", "corruptedimage", "emptybody"} { + t.Run(cid, func(t *testing.T) { + resp, _ := fetch(t, proxyURL(server, "avatar", did, cid), nil) + + // The exact code differs by failure mode — a sniffable non-image is + // a 400, a decoder blowing up mid-stream is a 500 — and pinning + // each one would make this brittle about which layer noticed + // first. What must never happen is a 2xx. + assert.GreaterOrEqual(t, resp.StatusCode, 400, + "undecodable upstream bytes must not be served as a success") + }) + } +} + +// TestImageProxy_PresetGeometry covers the preset table: each named preset has +// a documented output geometry, and the fit mode decides whether a source is +// cropped to it or merely bounded by it. +func TestImageProxy_PresetGeometry(t *testing.T) { + t.Parallel() + + const cid = "bafybeipresetgeometry123" + did := "did:plc:" + testkit.UniqueID(t) + + // 1000x1000 so that both directions are exercised: the cover presets crop + // it down, and content_preview — the one preset that only bounds — has + // something to shrink. A source smaller than every preset would make the + // no-upscaling case below indistinguishable from a no-op. + upstream := newBlobServer(t, map[string]func(http.ResponseWriter){ + cid: servePNG(testkit.TestPNGColor(1000, 1000, color.RGBA{R: 200, G: 100, B: 50, A: 255})), + }) + server := newProxyServer(t, &fixedPDSResolver{pdsURL: upstream.URL}, defaultFetchTimeout) + + // The cover presets: the source is scaled and cropped to exactly these. + for _, preset := range []struct { + name string + width, height int + }{ + {"avatar", 1000, 1000}, + {"avatar_small", 360, 360}, + {"banner", 640, 300}, + {"embed_thumbnail", 720, 360}, + } { + t.Run(preset.name, func(t *testing.T) { + resp, body := fetch(t, proxyURL(server, preset.name, did, cid), nil) + + require.Equal(t, http.StatusOK, resp.StatusCode, "body: %s", body) + assertImageSize(t, body, preset.width, preset.height) + }) + } + + t.Run("content_preview bounds the width and keeps the aspect ratio", func(t *testing.T) { + resp, body := fetch(t, proxyURL(server, "content_preview", did, cid), nil) + + require.Equal(t, http.StatusOK, resp.StatusCode, "body: %s", body) + // 800 is the preset's maximum width; the square source stays square. + assertImageSize(t, body, 800, 800) + }) + + t.Run("content_preview does not upscale a small source", func(t *testing.T) { + const smallCID = "bafybeismallsource123" + smallUpstream := newBlobServer(t, map[string]func(http.ResponseWriter){ + smallCID: servePNG(testkit.TestPNGColor(200, 200, color.RGBA{R: 100, G: 150, B: 200, A: 255})), + }) + smallServer := newProxyServer(t, &fixedPDSResolver{pdsURL: smallUpstream.URL}, defaultFetchTimeout) + + resp, body := fetch(t, proxyURL(smallServer, "content_preview", did, smallCID), nil) + + require.Equal(t, http.StatusOK, resp.StatusCode, "body: %s", body) + // Bounding, not resizing: a source already inside the bound is left + // alone rather than blown up into a blurry 800x800. + assertImageSize(t, body, 200, 200) + }) + + t.Run("an unknown preset is a 400", func(t *testing.T) { + resp, body := fetch(t, proxyURL(server, "not_a_valid_preset", did, cid), nil) + + assert.Equal(t, http.StatusBadRequest, resp.StatusCode) + assert.Contains(t, string(body), "invalid preset") + }) + + t.Run("a request missing the DID or the CID is refused", func(t *testing.T) { + for name, url := range map[string]string{ + "missing CID": fmt.Sprintf("%s/img/avatar/plain/%s/", server.URL, did), + "missing DID": fmt.Sprintf("%s/img/avatar/plain//%s", server.URL, cid), + } { + t.Run(name, func(t *testing.T) { + resp, _ := fetch(t, url, nil) + + // 400 or 404 depending on whether the router matched the + // pattern at all; either is a refusal, and which one is a + // routing detail rather than a contract. + assert.True(t, resp.StatusCode == http.StatusBadRequest || resp.StatusCode == http.StatusNotFound, + "expected 400 or 404 for %s, got %d", name, resp.StatusCode) + }) + } + }) +} + +// TestImageProxy_ErrorsAreNotJSON covers the response format of a failure. +// +// Every other endpoint in the AppView answers XRPC and speaks JSON, so it would +// be an easy and invisible mistake to make this one do the same. It must not: +// the proxy is addressed by , and a browser handed a JSON body under +// an image request shows a broken image with no clue why. Plain text is what a +// developer sees when they open the URL directly. +func TestImageProxy_ErrorsAreNotJSON(t *testing.T) { + t.Parallel() + + server := newProxyServer(t, &fixedPDSResolver{pdsURL: unreachableURL(t)}, time.Second) + did := "did:plc:" + testkit.UniqueID(t) + + resp, body := fetch(t, proxyURL(server, "invalid_preset", did, "cid"), nil) + + assert.Contains(t, resp.Header.Get("Content-Type"), "text/plain") + + var decoded map[string]any + assert.Error(t, json.Unmarshal(body, &decoded), + "an error body must not parse as JSON, got: %s", body) +} diff --git a/internal/api/handlers/post/create_embed_validation_test.go b/internal/api/handlers/post/create_embed_validation_test.go new file mode 100644 --- /dev/null +++ b/internal/api/handlers/post/create_embed_validation_test.go @@ -0,0 +1,214 @@ +//go:build integration + +// Embeds are the part of a post record the AppView writes into somebody else's +// repository, so a malformed one is not a rendering bug — it is a permanently +// invalid record on a PDS that no later fix can retract. The create path +// therefore validates the embed union before it writes anything: the $type +// discriminator must be present and known, and an external embed's thumb must +// be a real blob reference rather than the URL string clients keep sending. +// +// These cases run against the real post and community services rather than a +// fake, because the regression they guard against is not "validateEmbed is +// wrong" — that has unit tests — but "validateEmbed is no longer called". +// A validation call moved behind an early return, or reordered after the +// record is assembled, keeps every unit test green and silently starts writing +// the corrupt records the validation was added to prevent. Only the wired path +// can catch that. +// +// The file is in the external test package because it imports +// internal/db/postgres and Coves/tests/fixtures, both of which pull in the +// domain; the established form for every relocated integration test in this +// tree is package foo_test. +package post_test + +import ( + "context" + "net/http" + "testing" + + "Coves/internal/core/communities" + "Coves/tests/fixtures" + "Coves/tests/testkit" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// seedCommunityWithCredentials inserts a community that looks provisioned: +// it carries the PDS credentials the create path reads before it attempts a +// write. +// +// The credentials are inert — the access token is a syntactically valid JWT +// with a far-future expiry and nothing on the other end honours it — which is +// exactly what these tests want. They need the request to travel past +// "community not found" and past "credentials expired" so that it reaches embed +// validation; whether the eventual PDS write would succeed is the pipeline +// tier's question, not this file's. +func seedCommunityWithCredentials(t *testing.T, repo communities.Repository, pdsURL string) *communities.Community { + t.Helper() + + id := testkit.UniqueID(t) + community, err := repo.Create(context.Background(), &communities.Community{ + DID: fixtures.DID("community" + id), + Name: "embedtest-" + id, + Handle: "c-embedtest-" + id + ".coves.local", + Description: "Community used to reach embed validation on the create path", + Visibility: "public", + PDSEmail: "c-embedtest-" + id + "@coves.local", + PDSPassword: "inert-test-password", + // Header and payload of an unsigned JWT whose "exp" is in the year + // 2286, so nothing short-circuits on an expired token. + PDSAccessToken: "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJkaWQ6cGxjOnRlc3Rjb21tdW5pdHkiLCJleHAiOjk5OTk5OTk5OTl9.test", + PDSRefreshToken: "inert-refresh-token", + PDSURL: pdsURL, + }) + require.NoError(t, err, "seeding a community with PDS credentials") + return community +} + +// TestPostCreate_ExternalEmbedThumb covers the thumb field of +// social.coves.embed.external, which must be an atProto blob reference. +// +// Clients repeatedly send a URL string here because that is what the rendered +// post looks like, and accepting one would write a record no other atProto +// implementation can read. Each rejection is asserted on its message, not just +// its status, because the message is what tells a client which of the blob's +// four required parts it left out. +func TestPostCreate_ExternalEmbedThumb(t *testing.T) { + t.Parallel() + db := testkit.DB(t) + stack := newCreateStack(t, db) + + author := fixtures.User(t, db, "thumbtest.bsky.social", fixtures.DID("thumb"+testkit.UniqueID(t))) + community := seedCommunityWithCredentials(t, stack.communityRepo, stack.communityPDSURL) + + // postWithThumb sends an external embed carrying thumb (or none, for a nil + // thumb) and reports the rejection message, if the request was rejected at + // all. + postWithThumb := func(t *testing.T, thumb any) (message any, rejected bool) { + t.Helper() + + external := map[string]any{"uri": "https://streamable.com/test"} + if thumb != nil { + external["thumb"] = thumb + } + rec := createPost(t, stack, author.DID, map[string]any{ + "community": community.DID, + "title": "Test Post", + "content": "Test content", + "embed": map[string]any{ + "$type": "social.coves.embed.external", + "external": external, + }, + }) + if rec.Code != http.StatusBadRequest { + return nil, false + } + return decodeXRPCError(t, rec)["message"], true + } + + t.Run("a URL string is not a blob", func(t *testing.T) { + message, rejected := postWithThumb(t, "https://example.com/thumb.jpg") + require.True(t, rejected, "a URL-string thumb must be rejected with 400") + assert.Contains(t, message, "thumb must be a blob reference") + assert.Contains(t, message, "not URL string") + }) + + t.Run("a blob without $type is rejected", func(t *testing.T) { + message, rejected := postWithThumb(t, map[string]any{ + "ref": map[string]any{"$link": "bafyrei123"}, + "mimeType": "image/jpeg", + "size": 12345, + }) + require.True(t, rejected, "a thumb missing $type must be rejected with 400") + assert.Contains(t, message, "thumb must have $type: blob") + }) + + t.Run("a blob without ref is rejected", func(t *testing.T) { + message, rejected := postWithThumb(t, map[string]any{ + "$type": "blob", + "mimeType": "image/jpeg", + "size": 12345, + }) + require.True(t, rejected, "a thumb missing ref must be rejected with 400") + assert.Contains(t, message, "thumb blob missing required 'ref' field") + }) + + t.Run("a blob without mimeType is rejected", func(t *testing.T) { + message, rejected := postWithThumb(t, map[string]any{ + "$type": "blob", + "ref": map[string]any{"$link": "bafyrei123"}, + "size": 12345, + }) + require.True(t, rejected, "a thumb missing mimeType must be rejected with 400") + assert.Contains(t, message, "thumb blob missing required 'mimeType' field") + }) + + t.Run("a well-formed blob passes validation", func(t *testing.T) { + // The write itself still fails — the credentials seeded above are inert + // and the CID names no blob anybody uploaded — so this asserts the + // negative: whatever goes wrong afterwards, it is not thumb validation. + // Without it, the four cases above would also pass if validation + // rejected every thumb it ever saw. + message, rejected := postWithThumb(t, map[string]any{ + "$type": "blob", + "ref": map[string]any{"$link": "bafyreib6tbnql2ux3whnfysbzabthaj2vvck53nimhbi5g5a7jgvgr5eqm"}, + "mimeType": "image/jpeg", + "size": 52813, + }) + if rejected { + assert.NotContains(t, message, "thumb must be") + assert.NotContains(t, message, "thumb blob missing") + } + }) + + t.Run("an absent thumb passes validation", func(t *testing.T) { + // The common case: the client sends a bare link and the unfurl service + // fills the thumbnail in later, so a missing thumb is legal. + message, rejected := postWithThumb(t, nil) + if rejected { + assert.NotContains(t, message, "thumb must be") + } + }) +} + +// TestPostCreate_EmbedUnionDiscriminator covers the $type discriminator that +// decides which member of the embed union a record claims to be. +func TestPostCreate_EmbedUnionDiscriminator(t *testing.T) { + t.Parallel() + db := testkit.DB(t) + stack := newCreateStack(t, db) + + author := fixtures.User(t, db, "embedtest.bsky.social", fixtures.DID("embed"+testkit.UniqueID(t))) + community := seedCommunityWithCredentials(t, stack.communityRepo, stack.communityPDSURL) + + postEmbed := func(t *testing.T, embed any) map[string]any { + t.Helper() + + rec := createPost(t, stack, author.DID, map[string]any{ + "community": community.DID, + "title": "Test Post", + "embed": embed, + }) + require.Equal(t, http.StatusBadRequest, rec.Code, + "an undiscriminated embed must be refused, got %d: %s", rec.Code, rec.Body.String()) + return decodeXRPCError(t, rec) + } + + t.Run("an embed without $type is rejected", func(t *testing.T) { + // The exact shape the frontend was sending when link posts were being + // written as unreadable records: a bare {uri} with no discriminator and + // no external wrapper. + assert.Contains(t, postEmbed(t, map[string]any{"uri": "https://example.com"})["message"], "$type") + }) + + t.Run("an embed with an unknown $type is rejected", func(t *testing.T) { + // A typo in the NSID must not fall through to "no embed": the record + // would be written without the content the author attached. + envelope := postEmbed(t, map[string]any{ + "$type": "social.coves.embed.externl", + "external": map[string]any{"uri": "https://example.com"}, + }) + assert.Contains(t, envelope["message"], "unknown embed") + }) +} diff --git a/internal/api/handlers/post/create_security_test.go b/internal/api/handlers/post/create_security_test.go new file mode 100644 --- /dev/null +++ b/internal/api/handlers/post/create_security_test.go @@ -0,0 +1,334 @@ +//go:build integration + +// The create endpoint is the AppView's only authenticated write path for posts, +// so its rejection behaviour is a security boundary rather than an ergonomic +// detail: the author of a post is derived from the session and never from the +// request body, the body is bounded before it is parsed, and the community +// identifier is validated before anything is written. +// +// These tests drive the real handler over the real post and community services, +// which is the difference between them and the in-package unit tests in this +// directory. A unit test with a fake service proves the handler forwards what +// it was given; only the real stack proves the check is still ON the path a +// request takes — the failure mode that matters is a validation call that gets +// moved behind an early return and stops running while its own unit test keeps +// passing. +// +// The file is in the external test package because it imports +// internal/db/postgres, which pulls in the domain; the established form for +// every relocated integration test in this tree is package foo_test. +package post_test + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "Coves/internal/api/middleware" + "Coves/internal/core/posts" + "Coves/tests/fixtures" + "Coves/tests/testkit" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const createPostPath = "/xrpc/social.coves.community.post.create" + +// createPost posts payload as the given DID and returns the recorder. An empty +// authorDID sends the request unauthenticated. +func createPost(t *testing.T, stack createStack, authorDID string, payload any) *httptest.ResponseRecorder { + t.Helper() + + body, err := json.Marshal(payload) + require.NoError(t, err, "marshalling the request payload") + return createPostRaw(t, stack, authorDID, body) +} + +// createPostRaw is createPost for the cases that need to send bytes the JSON +// encoder would never produce. +func createPostRaw(t *testing.T, stack createStack, authorDID string, body []byte) *httptest.ResponseRecorder { + t.Helper() + + req := httptest.NewRequest(http.MethodPost, createPostPath, bytes.NewReader(body)) + if authorDID != "" { + req = req.WithContext(middleware.SetTestUserDID(req.Context(), authorDID)) + } + + rec := httptest.NewRecorder() + stack.handler.HandleCreate(rec, req) + return rec +} + +// decodeXRPCError reads the {error, message} envelope every failure carries. +func decodeXRPCError(t *testing.T, rec *httptest.ResponseRecorder) map[string]any { + t.Helper() + + var envelope map[string]any + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &envelope), + "failure responses must be a JSON XRPC error envelope, got: %s", rec.Body.String()) + return envelope +} + +// TestPostCreate_HandlerRejections covers everything the create handler must +// refuse before the request reaches the service. +func TestPostCreate_HandlerRejections(t *testing.T) { + t.Parallel() + stack := newCreateStack(t, testkit.DB(t)) + + authorDID := fixtures.DID("author" + testkit.UniqueID(t)) + communityDID := fixtures.DID("community" + testkit.UniqueID(t)) + + t.Run("client-supplied authorDid is refused", func(t *testing.T) { + // Authorship comes from the session. Accepting it from the body would + // let any authenticated caller post as anybody, so the handler rejects + // the field outright rather than silently overwriting it — a silent + // overwrite would leave a client believing it had posted as someone + // else and succeeded. + rec := createPost(t, stack, authorDID, map[string]any{ + "community": communityDID, + "authorDid": fixtures.DID("attacker"), + "content": "Malicious post", + }) + + assert.Equal(t, http.StatusBadRequest, rec.Code) + envelope := decodeXRPCError(t, rec) + assert.Equal(t, "InvalidRequest", envelope["error"]) + assert.Contains(t, envelope["message"], "authorDid must not be provided") + }) + + t.Run("unauthenticated request is refused", func(t *testing.T) { + rec := createPost(t, stack, "", map[string]any{ + "community": communityDID, + "content": "Test post", + }) + + assert.Equal(t, http.StatusUnauthorized, rec.Code) + assert.Equal(t, "AuthRequired", decodeXRPCError(t, rec)["error"]) + }) + + t.Run("body over 1MB is refused before it is parsed", func(t *testing.T) { + rec := createPost(t, stack, authorDID, map[string]any{ + "community": communityDID, + "content": strings.Repeat("A", 1*1024*1024+1000), + }) + + assert.Equal(t, http.StatusRequestEntityTooLarge, rec.Code) + assert.Equal(t, "RequestTooLarge", decodeXRPCError(t, rec)["error"]) + }) + + t.Run("malformed JSON is refused", func(t *testing.T) { + rec := createPostRaw(t, stack, authorDID, []byte(`{"community": "did:plc:test123", "content": `)) + + assert.Equal(t, http.StatusBadRequest, rec.Code) + assert.Equal(t, "InvalidRequest", decodeXRPCError(t, rec)["error"]) + }) + + t.Run("empty community is refused", func(t *testing.T) { + rec := createPost(t, stack, authorDID, map[string]any{ + "community": "", + "content": "Test post", + }) + + assert.Equal(t, http.StatusBadRequest, rec.Code) + envelope := decodeXRPCError(t, rec) + assert.Equal(t, "InvalidRequest", envelope["error"]) + assert.Contains(t, envelope["message"], "community is required") + }) + + t.Run("non-POST methods are refused", func(t *testing.T) { + for _, method := range []string{http.MethodGet, http.MethodPut, http.MethodDelete, http.MethodPatch} { + t.Run(method, func(t *testing.T) { + rec := httptest.NewRecorder() + stack.handler.HandleCreate(rec, httptest.NewRequest(method, createPostPath, nil)) + assert.Equal(t, http.StatusMethodNotAllowed, rec.Code) + }) + } + }) +} + +// TestPostCreate_CommunityIdentifierFormats covers the at-identifier surface of +// the community field: which shapes are turned away as malformed, and which +// shapes get far enough to be looked up. +// +// The accepted shapes are asserted negatively — "did not fail FORMAT +// validation" — because none of these communities exist in the database, so the +// request is expected to end in a lookup failure. Asserting a success status +// would require provisioning a community with working PDS credentials, which is +// the pipeline tier's job; what this file can prove is that a legal identifier +// is not rejected as illegal. +func TestPostCreate_CommunityIdentifierFormats(t *testing.T) { + t.Parallel() + stack := newCreateStack(t, testkit.DB(t)) + + authorDID := fixtures.DID("author" + testkit.UniqueID(t)) + + t.Run("malformed identifiers are refused", func(t *testing.T) { + for _, identifier := range []string{ + "not-a-did-or-handle", + "just-plain-text", + "http://example.com", + } { + t.Run(identifier, func(t *testing.T) { + rec := createPost(t, stack, authorDID, map[string]any{ + "community": identifier, + "content": "Test post", + }) + + // 400 (rejected as malformed) and 404 (parsed, then not found) + // are both correct refusals; which one a given shape produces + // depends on how far the resolver gets before it gives up, and + // pinning that here would make the test brittle about an + // implementation detail rather than about the refusal. + assert.True(t, rec.Code == http.StatusBadRequest || rec.Code == http.StatusNotFound, + "expected 400 or 404 for %q, got %d: %s", identifier, rec.Code, rec.Body.String()) + + envelope := decodeXRPCError(t, rec) + assert.NotEmpty(t, envelope["error"], "refusals must name an error code") + assert.NotEmpty(t, envelope["message"], "refusals must carry a message") + }) + } + }) + + // The four legal spellings of a community, per the at-identifier rules: + // a bare DID, the scoped !name@instance form, the canonical DNS handle + // c-name.instance the scoped form expands to, and that handle with the + // atProto @ prefix. + wellFormed := []string{ + "did:plc:test123", + "did:web:example.com", + "!mycommunity@bsky.social", + "!gaming@test.coves.social", + "c-gaming.test.coves.social", + "c-books.bsky.social", + "@c-gaming.test.coves.social", + "@c-books.bsky.social", + } + + t.Run("well-formed identifiers pass format validation", func(t *testing.T) { + for _, identifier := range wellFormed { + t.Run(identifier, func(t *testing.T) { + rec := createPost(t, stack, authorDID, map[string]any{ + "community": identifier, + "content": "Test post", + }) + + if rec.Code != http.StatusBadRequest { + return // Reached the lookup, which is as far as this test goes. + } + message := decodeXRPCError(t, rec)["message"] + assert.NotContains(t, message, "community must be a DID", + "%q is a legal community identifier and must not be rejected as malformed", identifier) + assert.NotContains(t, message, "scoped handle must include", + "%q is a legal community identifier and must not be rejected as malformed", identifier) + }) + } + }) +} + +// TestPostCreate_HostileContent covers content the handler must carry through +// unharmed rather than refuse: text is text, and the injection-shaped strings +// below are dangerous only if something downstream interpolates them. +func TestPostCreate_HostileContent(t *testing.T) { + t.Parallel() + stack := newCreateStack(t, testkit.DB(t)) + + authorDID := fixtures.DID("author" + testkit.UniqueID(t)) + communityDID := fixtures.DID("community" + testkit.UniqueID(t)) + + t.Run("unicode and emoji are not rejected", func(t *testing.T) { + rec := createPost(t, stack, authorDID, map[string]any{ + "community": communityDID, + "content": "Hello 世界! 🌍 Testing unicode: café, naïve, Ω", + }) + + // The community does not exist, so the request cannot succeed; what it + // must not do is come back as a validation failure. + assert.NotEqual(t, http.StatusBadRequest, rec.Code, + "valid UTF-8 content must not be rejected as invalid: %s", rec.Body.String()) + }) + + t.Run("injection-shaped content does not reach an interpolator", func(t *testing.T) { + // Every one of these is a plain string as far as the AppView is + // concerned. A 500 would mean something tried to interpret it — a + // concatenated query, a template, a path join — which is precisely the + // bug this case exists to catch. + for _, hostile := range []string{ + "'; DROP TABLE posts; --", + "1' OR '1'='1", + "", + "../../../etc/passwd", + } { + t.Run(hostile, func(t *testing.T) { + rec := createPost(t, stack, authorDID, map[string]any{ + "community": communityDID, + "content": hostile, + }) + + assert.NotEqual(t, http.StatusInternalServerError, rec.Code, + "content %q must be handled as data, not interpreted: %s", hostile, rec.Body.String()) + }) + } + }) +} + +// TestPostCreate_ServiceEnforcesAuthorship covers the same authorship rule as +// the handler test above, one layer down. +// +// The duplication is deliberate defence in depth. The handler is not the only +// caller of posts.Service.CreatePost — a consumer, a future admin path or a +// refactor that moves the route can all reach the service directly — so the +// service re-derives the author from the context instead of trusting the +// request it was handed. These cases call the service with the handler removed +// from the picture, which is the only way to prove that second check exists. +func TestPostCreate_ServiceEnforcesAuthorship(t *testing.T) { + t.Parallel() + stack := newCreateStack(t, testkit.DB(t)) + + communityDID := fixtures.DID("community" + testkit.UniqueID(t)) + alice := fixtures.DID("alice" + testkit.UniqueID(t)) + bob := fixtures.DID("bob" + testkit.UniqueID(t)) + + createAs := func(contextDID, requestDID string) error { + content := "Test post" + _, err := stack.service.CreatePost( + middleware.SetTestUserDID(t.Context(), contextDID), + posts.CreatePostRequest{ + Community: communityDID, + AuthorDID: requestDID, + Content: &content, + }, + ) + return err + } + + t.Run("no authenticated DID in context is refused", func(t *testing.T) { + err := createAs("", alice) + + require.Error(t, err) + assert.Contains(t, strings.ToLower(err.Error()), "authenticated") + }) + + t.Run("request DID that differs from the session is refused", func(t *testing.T) { + // The spoofing case: authenticated as Alice, asking to post as Bob. + err := createAs(alice, bob) + + require.Error(t, err) + assert.Contains(t, strings.ToLower(err.Error()), "does not match") + }) + + t.Run("matching DIDs pass the authorship check", func(t *testing.T) { + // The community does not exist, so this still fails — but it must fail + // on the lookup, not on authorship. Without this case the two above + // would also pass if CreatePost rejected every request it ever saw. + err := createAs(alice, alice) + + if err != nil { + assert.NotContains(t, strings.ToLower(err.Error()), "does not match", + "a request whose DID matches the session must not fail the authorship check") + } + }) +} diff --git a/internal/api/handlers/post/harness_test.go b/internal/api/handlers/post/harness_test.go new file mode 100644 --- /dev/null +++ b/internal/api/handlers/post/harness_test.go @@ -0,0 +1,93 @@ +//go:build integration + +package post_test + +import ( + "database/sql" + "os" + "testing" + + "Coves/internal/api/handlers/post" + "Coves/internal/core/communities" + "Coves/internal/core/posts" + "Coves/internal/db/postgres" + "Coves/tests/fixtures" + "Coves/tests/testkit" +) + +// TestMain sets the infrastructure floor for this package's integration build. +// +// It lives in a build-tagged file on purpose. A TestMain governs the WHOLE test +// binary, and under -tags integration the tagged and untagged files of this +// directory compile into one binary — so this function also runs for the +// in-package unit tests in get_test.go and errors_test.go. Those are pure +// handler tests over hand-written fakes and must keep building and running +// without a socket in sight, which the tag guarantees: without +// -tags integration this file does not exist and the unit build has no TestMain +// at all. +// +// The floor is Postgres and nothing more. The integration tests here drive the +// create endpoint through the real post and community services down to the real +// repositories, because that is the only way to prove the validation is on the +// live path rather than merely unit-tested in isolation. Every case stops at +// validation or at "community not found", both of which are answered from the +// database — nothing here reaches the PDS, so requiring one would fail the +// package for infrastructure it never dials. +func TestMain(m *testing.M) { + os.Exit(testkit.Main(m, testkit.RequirePostgres)) +} + +// createStack is the create endpoint plus the two collaborators a test needs to +// reach around it: the service, for the defence-in-depth checks that must hold +// even when the handler is bypassed, and the community repository, for seeding +// the community a request names. +type createStack struct { + handler *post.CreateHandler + service posts.Service + communityRepo communities.Repository + communityPDSURL string +} + +// newCreateStack wires the create endpoint the way the server wires it, minus +// the services that are legitimately optional. +// +// The wiring is the point of these tests, so the handler is NOT given a fake +// post service: it gets the real posts.Service over the real repositories, so a +// validation rule that stopped being called — moved behind an early return, +// dropped from the request path — fails here even though its own unit test +// still passes. +// +// The aggregator, blob, unfurl and Bluesky services are nil because +// posts.NewPostService documents them as optional and no case below produces a +// post that would reach them: a request only gets that far with a community +// whose PDS credentials actually work, which is the pipeline tier's job to +// prove. +func newCreateStack(t *testing.T, db *sql.DB) createStack { + t.Helper() + + pdsURL := testkit.Endpoints().PDS.BaseURL + communityRepo := postgres.NewCommunityRepository(db) + communityService := communities.NewCommunityServiceWithPDSFactory( + communityRepo, + pdsURL, + fixtures.InstanceDID(), + testkit.Endpoints().PDS.HandleDomain, + nil, // no provisioner: no test here creates a community account + nil, // no PDS client factory + nil, // no blob service + ) + + postService := posts.NewPostService( + postgres.NewPostRepository(db), + communityService, + nil, nil, nil, nil, + pdsURL, + ) + + return createStack{ + handler: post.NewCreateHandler(postService), + service: postService, + communityRepo: communityRepo, + communityPDSURL: pdsURL, + } +} diff --git a/internal/api/middleware/harness_test.go b/internal/api/middleware/harness_test.go new file mode 100644 --- /dev/null +++ b/internal/api/middleware/harness_test.go @@ -0,0 +1,22 @@ +//go:build integration + +package middleware_test + +import ( + "os" + "testing" + + "Coves/tests/testkit" +) + +// TestMain sets the infrastructure floor for this package's integration build. +// +// It lives in a tagged file because a TestMain applies to the whole test +// binary: the untagged unit build of this package needs nothing out of +// process, and must not be made to probe the PDS before it can run. +// +// oauth_token_verification_test.go creates a real PDS account and never opens +// a database connection, so the floor here is RequirePDS alone. +func TestMain(m *testing.M) { + os.Exit(testkit.Main(m, testkit.RequirePDS)) +} diff --git a/internal/atproto/identity/harness_test.go b/internal/atproto/identity/harness_test.go new file mode 100644 --- /dev/null +++ b/internal/atproto/identity/harness_test.go @@ -0,0 +1,24 @@ +//go:build integration + +package identity_test + +import ( + "os" + "testing" + + "Coves/tests/testkit" +) + +// TestMain sets the infrastructure floor for this package's integration build. +// +// It lives in a tagged file because a TestMain applies to the whole test +// binary: the untagged unit build of this package needs nothing out of +// process, and must not be made to probe Postgres before it can run. +// +// The floor is Postgres alone. What this package's integration tests cover is +// the identity cache, whose storage is the identity_cache table — resolution +// against the PLC directory or a handle's DNS records belongs to tests/live, +// which is the only tier permitted to reach the public network. +func TestMain(m *testing.M) { + os.Exit(testkit.Main(m, testkit.RequirePostgres)) +} diff --git a/internal/atproto/jetstream/community_hostedby_verification_test.go b/internal/atproto/jetstream/community_hostedby_verification_test.go new file mode 100644 --- /dev/null +++ b/internal/atproto/jetstream/community_hostedby_verification_test.go @@ -0,0 +1,555 @@ +//go:build integration + +package jetstream + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "Coves/internal/db/postgres" + "Coves/tests/fixtures" + "Coves/tests/testkit" + + _ "github.com/lib/pq" +) + +// The hostedBy claim is the community consumer's trust boundary. A community +// record is written to its OWN repository and says which instance hosts it, so +// nothing stops a hostile repo from claiming `hostedBy: did:web:nintendo.com` +// while its handle sits on some other domain. verifyHostedByClaim is what +// refuses that, and these tests are its coverage: +// +// - the domain of the handle must equal the domain in the did:web; +// - hostedBy must be a did:web at all (a did:plc names an account, not a +// host, so there is no domain to compare against); +// - the registrable-domain extraction has to survive multi-part public +// suffixes, or `c-gaming.coves.co.uk` reads as hosted by `co.uk` and every +// .co.uk instance can impersonate every other. +// +// They run against real Postgres because half of what is being asserted is a +// negative about the index: a rejected event must leave NO community row +// behind. A stubbed repository can only report what it was told, not what +// survived. +// +// # WHY VERIFICATION IS PASSED AS AN ARGUMENT, NOT READ FROM THE ENVIRONMENT +// +// The deployed AppView reads SKIP_DID_WEB_VERIFICATION, and both .env.dev and +// .env.ci set it to true — so in CI the wired-up consumer performs no +// verification at all. These tests do not go through that wiring: they +// construct CommunityEventConsumer directly and pass skipVerification +// explicitly, so each case pins the behaviour it names regardless of how the +// surrounding stack is configured. That independence is the point; a test that +// inherited the CI setting would assert nothing here. +// +// The cases that DO enable verification are still hermetic. Every one of them +// fails on the did:web format check or the domain comparison, both of which run +// before verifyDIDDocument would fetch anything, so no DID document is ever +// requested over the network. + +// TestHostedByVerification_DomainMatching covers the domain comparison itself, +// and what the index looks like on either side of it. +func TestHostedByVerification_DomainMatching(t *testing.T) { + t.Parallel() + db := testkit.DB(t) + + repo := postgres.NewCommunityRepository(db) + ctx := context.Background() + + t.Run("rejects community with mismatched hostedBy domain", func(t *testing.T) { + // Verification ENABLED. The identity resolver is nil because the + // consumer derives the handle domain from the record, not by resolving + // anything. + consumer := NewCommunityEventConsumer(repo, "did:web:coves.social", false, nil) + + uniqueSuffix := testkit.UniqueID(t) + communityDID := fixtures.DID(uniqueSuffix) + uniqueHandle := fmt.Sprintf("c-gaming%s.coves.social", uniqueSuffix) + + // The attack: a coves.social handle claiming to be hosted by Nintendo. + event := &JetstreamEvent{ + Did: communityDID, + TimeUS: time.Now().UnixMicro(), + Kind: "commit", + Commit: &CommitEvent{ + Rev: "rev123", + Operation: "create", + Collection: "social.coves.community.profile", + RKey: "self", + CID: "bafy123abc", + Record: map[string]interface{}{ + "handle": uniqueHandle, + "name": "gaming", + "displayName": "Nintendo Gaming", + "description": "Fake Nintendo community", + "createdBy": "did:plc:attacker123", + "hostedBy": "did:web:nintendo.com", // spoofed + "visibility": "public", + "federation": map[string]interface{}{ + "allowExternalDiscovery": true, + }, + "memberCount": 0, + "subscriberCount": 0, + "createdAt": time.Now().Format(time.RFC3339), + }, + }, + } + + err := consumer.HandleEvent(ctx, event) + if err == nil { + t.Fatal("Expected verification error for mismatched hostedBy domain, got nil") + } + + // Rejection must be permanent: the mismatch is inherent to the record, + // so retrying it forever only fills the dead-letter queue. + if !strings.Contains(err.Error(), "doesn't match hostedBy domain") { + t.Errorf("Expected a domain-mismatch rejection, got: %v", err) + } + + // The negative that matters: nothing was indexed. + if _, getErr := repo.GetByDID(ctx, communityDID); getErr == nil { + t.Fatal("Community should not have been indexed, but was found in database") + } + }) + + t.Run("accepts community with matching hostedBy domain", func(t *testing.T) { + // Verification SKIPPED. This case is about the happy path through + // indexing — the handle and the hostedBy agree, and there is no real + // did:web document to fetch for a fixture domain. + consumer := NewCommunityEventConsumer(repo, "did:web:coves.social", true, nil) + + uniqueSuffix := testkit.UniqueID(t) + communityDID := fixtures.DID(uniqueSuffix) + uniqueHandle := fmt.Sprintf("c-gaming%s.coves.social", uniqueSuffix) + + event := &JetstreamEvent{ + Did: communityDID, + TimeUS: time.Now().UnixMicro(), + Kind: "commit", + Commit: &CommitEvent{ + Rev: "rev123", + Operation: "create", + Collection: "social.coves.community.profile", + RKey: "self", + CID: "bafy123abc", + Record: map[string]interface{}{ + "handle": uniqueHandle, + "name": "gaming", + "displayName": "Gaming Community", + "description": "Legitimate coves.social community", + "createdBy": "did:plc:user123", + "hostedBy": "did:web:coves.social", + "visibility": "public", + "federation": map[string]interface{}{ + "allowExternalDiscovery": true, + }, + "memberCount": 0, + "subscriberCount": 0, + "createdAt": time.Now().Format(time.RFC3339), + }, + }, + } + + if err := consumer.HandleEvent(ctx, event); err != nil { + t.Fatalf("Expected verification to succeed, got error: %v", err) + } + + community, getErr := repo.GetByDID(ctx, communityDID) + if getErr != nil { + t.Fatalf("Community should have been indexed: %v", getErr) + } + // The claim is persisted verbatim: downstream federation decisions read + // hosted_by_did, so a dropped or rewritten value would silently move + // the community to another instance. + if community.HostedByDID != "did:web:coves.social" { + t.Errorf("Expected hostedByDID 'did:web:coves.social', got '%s'", community.HostedByDID) + } + }) + + t.Run("rejects hostedBy with non-did:web format", func(t *testing.T) { + // Verification ENABLED. A did:plc identifies an account rather than a + // host, so there is no domain to compare and the claim is unverifiable + // by construction — it must be refused rather than trusted. + consumer := NewCommunityEventConsumer(repo, "did:web:coves.social", false, nil) + + uniqueSuffix := testkit.UniqueID(t) + communityDID := fixtures.DID(uniqueSuffix) + uniqueHandle := fmt.Sprintf("c-gaming%s.coves.social", uniqueSuffix) + + event := &JetstreamEvent{ + Did: communityDID, + TimeUS: time.Now().UnixMicro(), + Kind: "commit", + Commit: &CommitEvent{ + Rev: "rev123", + Operation: "create", + Collection: "social.coves.community.profile", + RKey: "self", + CID: "bafy123abc", + Record: map[string]interface{}{ + "handle": uniqueHandle, + "name": "gaming", + "displayName": "Test Community", + "description": "Test", + "createdBy": "did:plc:user123", + "hostedBy": "did:plc:xyz123", // must be did:web + "visibility": "public", + "federation": map[string]interface{}{ + "allowExternalDiscovery": true, + }, + "memberCount": 0, + "subscriberCount": 0, + "createdAt": time.Now().Format(time.RFC3339), + }, + }, + } + + err := consumer.HandleEvent(ctx, event) + if err == nil { + t.Fatal("Expected verification error for non-did:web hostedBy, got nil") + } + if !strings.Contains(err.Error(), "did:web") { + t.Errorf("Expected a did:web method rejection, got: %v", err) + } + + if _, getErr := repo.GetByDID(ctx, communityDID); getErr == nil { + t.Fatal("Community should not have been indexed, but was found in database") + } + }) + + t.Run("skip verification flag bypasses all checks", func(t *testing.T) { + // This is the dev/CI configuration (SKIP_DID_WEB_VERIFICATION=true) seen + // from the inside: with the flag on, a record that the case above + // rejects is indexed. Pinning it here is what stops the flag from + // quietly becoming a no-op — and what documents the exposure that comes + // with enabling it. + consumer := NewCommunityEventConsumer(repo, "did:web:coves.social", true, nil) + + uniqueSuffix := testkit.UniqueID(t) + communityDID := fixtures.DID(uniqueSuffix) + uniqueHandle := fmt.Sprintf("c-gaming%s.example.com", uniqueSuffix) + + event := &JetstreamEvent{ + Did: communityDID, + TimeUS: time.Now().UnixMicro(), + Kind: "commit", + Commit: &CommitEvent{ + Rev: "rev123", + Operation: "create", + Collection: "social.coves.community.profile", + RKey: "self", + CID: "bafy123abc", + Record: map[string]interface{}{ + "handle": uniqueHandle, + "name": "gaming", + "displayName": "Test", + "description": "Test", + "createdBy": "did:plc:user123", + "hostedBy": "did:web:nintendo.com", // mismatched, but unchecked + "visibility": "public", + "federation": map[string]interface{}{ + "allowExternalDiscovery": true, + }, + "memberCount": 0, + "subscriberCount": 0, + "createdAt": time.Now().Format(time.RFC3339), + }, + }, + } + + if err := consumer.HandleEvent(ctx, event); err != nil { + t.Fatalf("Expected success with skipVerification=true, got error: %v", err) + } + + if _, getErr := repo.GetByDID(ctx, communityDID); getErr != nil { + t.Fatalf("Community should have been indexed: %v", getErr) + } + }) +} + +// TestBidirectionalDIDVerification exercises the indexing path against handles +// on a domain served by a local DID document. +// +// CAVEAT, and it is a large one: both cases below construct the consumer with +// skipVerification=true, so the mock server is never contacted and the +// alsoKnownAs check the test is named for never runs. The second case asserts +// that a DID document WITHOUT alsoKnownAs is accepted, which is the opposite of +// the production requirement. What the cases actually prove is that a handle +// and hostedBy on a host:port domain index correctly and round-trip their +// hostedBy claim. +// +// The reason it is shaped this way is that httptest.NewTLSServer issues a +// self-signed certificate, so a consumer with verification enabled would fail +// on TLS rather than on alsoKnownAs, and the failure would say nothing about +// bidirectional verification either. Making these cases mean what their names +// say needs the verifier's HTTP client to be injectable so the test can hand it +// the mock server's certificate pool — a change to production code that is out +// of scope for relocating the file. +func TestBidirectionalDIDVerification(t *testing.T) { + t.Parallel() + db := testkit.DB(t) + + repo := postgres.NewCommunityRepository(db) + ctx := context.Background() + + t.Run("indexes a community whose domain serves a DID document with alsoKnownAs", func(t *testing.T) { + mockServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/.well-known/did.json" { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = fmt.Fprintf(w, `{ + "id": "did:web:example.com", + "alsoKnownAs": ["at://example.com"], + "verificationMethod": [], + "service": [] + }`) + return + } + http.NotFound(w, r) + })) + defer mockServer.Close() + + // The domain is the server's host:port, which is what a did:web would + // name for a non-standard port. + mockDomain := strings.TrimPrefix(mockServer.URL, "https://") + + consumer := NewCommunityEventConsumer(repo, fmt.Sprintf("did:web:%s", mockDomain), true, nil) + + uniqueSuffix := testkit.UniqueID(t) + communityDID := fixtures.DID(uniqueSuffix) + uniqueHandle := fmt.Sprintf("c-gaming%s.%s", uniqueSuffix, mockDomain) + + event := &JetstreamEvent{ + Did: communityDID, + TimeUS: time.Now().UnixMicro(), + Kind: "commit", + Commit: &CommitEvent{ + Rev: "rev123", + Operation: "create", + Collection: "social.coves.community.profile", + RKey: "self", + CID: "bafy123abc", + Record: map[string]interface{}{ + "handle": uniqueHandle, + "name": "gaming", + "displayName": "Gaming Community", + "description": "Test community with bidirectional verification", + "createdBy": "did:plc:user123", + "hostedBy": fmt.Sprintf("did:web:%s", mockDomain), + "visibility": "public", + "federation": map[string]interface{}{ + "allowExternalDiscovery": true, + }, + "memberCount": 0, + "subscriberCount": 0, + "createdAt": time.Now().Format(time.RFC3339), + }, + }, + } + + if err := consumer.HandleEvent(ctx, event); err != nil { + t.Fatalf("Expected verification to succeed, got error: %v", err) + } + + community, getErr := repo.GetByDID(ctx, communityDID) + if getErr != nil { + t.Fatalf("Community should have been indexed: %v", getErr) + } + if community.HostedByDID != fmt.Sprintf("did:web:%s", mockDomain) { + t.Errorf("Expected hostedByDID 'did:web:%s', got '%s'", mockDomain, community.HostedByDID) + } + }) + + t.Run("indexes a community whose domain serves a DID document without alsoKnownAs", func(t *testing.T) { + mockServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/.well-known/did.json" { + // No alsoKnownAs. With verification enabled this is what a + // bidirectional check would reject; with it skipped, nobody + // looks. + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = fmt.Fprintf(w, `{ + "id": "did:web:example.com", + "verificationMethod": [], + "service": [] + }`) + return + } + http.NotFound(w, r) + })) + defer mockServer.Close() + + mockDomain := strings.TrimPrefix(mockServer.URL, "https://") + + consumer := NewCommunityEventConsumer(repo, fmt.Sprintf("did:web:%s", mockDomain), true, nil) + + uniqueSuffix := testkit.UniqueID(t) + communityDID := fixtures.DID(uniqueSuffix) + uniqueHandle := fmt.Sprintf("c-gaming%s.%s", uniqueSuffix, mockDomain) + + event := &JetstreamEvent{ + Did: communityDID, + TimeUS: time.Now().UnixMicro(), + Kind: "commit", + Commit: &CommitEvent{ + Rev: "rev123", + Operation: "create", + Collection: "social.coves.community.profile", + RKey: "self", + CID: "bafy123abc", + Record: map[string]interface{}{ + "handle": uniqueHandle, + "name": "gaming", + "displayName": "Gaming Community", + "description": "Test community without alsoKnownAs", + "createdBy": "did:plc:user123", + "hostedBy": fmt.Sprintf("did:web:%s", mockDomain), + "visibility": "public", + "federation": map[string]interface{}{ + "allowExternalDiscovery": true, + }, + "memberCount": 0, + "subscriberCount": 0, + "createdAt": time.Now().Format(time.RFC3339), + }, + }, + } + + if err := consumer.HandleEvent(ctx, event); err != nil { + t.Fatalf("Expected verification to succeed with skipVerification:true, got error: %v", err) + } + }) +} + +// TestExtractDomainFromHandle drives extractDomainFromHandle through the +// consumer, one handle shape per case. +// +// The multi-part public suffixes are the reason this table exists. Extracting a +// registrable domain by taking the last two labels turns +// `c-gaming.coves.co.uk` into `co.uk`, at which point any .co.uk instance can +// claim to host any other .co.uk community. The negative case with +// `did:web:co.uk` pins that specific error. +// +// Verification is enabled exactly for the cases expected to FAIL, so the domain +// comparison actually runs; the cases expected to succeed use fixture domains +// with no DID document to serve, so they skip it. Either way no case reaches +// the network: a domain mismatch is decided before the document would be +// fetched. +func TestExtractDomainFromHandle(t *testing.T) { + t.Parallel() + db := testkit.DB(t) + + repo := postgres.NewCommunityRepository(db) + ctx := context.Background() + + testCases := []struct { + name string + handle string + hostedByDID string + shouldSucceed bool + }{ + { + name: "DNS-style handle with subdomain", + handle: "c-gaming.coves.social", + hostedByDID: "did:web:coves.social", + shouldSucceed: true, + }, + { + name: "Simple two-part domain", + handle: "gaming.coves.social", + hostedByDID: "did:web:coves.social", + shouldSucceed: true, + }, + { + name: "Multi-part subdomain", + handle: "c-gaming.test.example.com", + hostedByDID: "did:web:example.com", + shouldSucceed: true, + }, + { + name: "Mismatched domain", + handle: "c-gaming.coves.social", + hostedByDID: "did:web:example.com", + shouldSucceed: false, + }, + { + name: "Multi-part TLD: .co.uk", + handle: "c-gaming.coves.co.uk", + hostedByDID: "did:web:coves.co.uk", + shouldSucceed: true, + }, + { + name: "Multi-part TLD: .com.au", + handle: "c-gaming.example.com.au", + hostedByDID: "did:web:example.com.au", + shouldSucceed: true, + }, + { + name: "Multi-part TLD: Reject incorrect .co.uk extraction", + handle: "c-gaming.coves.co.uk", + hostedByDID: "did:web:co.uk", // wrong: should be coves.co.uk + shouldSucceed: false, + }, + { + name: "Multi-part TLD: .org.uk", + handle: "c-gaming.myinstance.org.uk", + hostedByDID: "did:web:myinstance.org.uk", + shouldSucceed: true, + }, + { + name: "Multi-part TLD: .ac.uk", + handle: "c-gaming.university.ac.uk", + hostedByDID: "did:web:university.ac.uk", + shouldSucceed: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + skipVerification := tc.shouldSucceed + consumer := NewCommunityEventConsumer(repo, "did:web:coves.social", skipVerification, nil) + + uniqueSuffix := testkit.UniqueID(t) + communityDID := fixtures.DID(uniqueSuffix) + + event := &JetstreamEvent{ + Did: communityDID, + TimeUS: time.Now().UnixMicro(), + Kind: "commit", + Commit: &CommitEvent{ + Rev: "rev123", + Operation: "create", + Collection: "social.coves.community.profile", + RKey: "self", + CID: "bafy123abc", + Record: map[string]interface{}{ + "handle": tc.handle, + "name": "test", + "displayName": "Test", + "description": "Test", + "createdBy": "did:plc:user123", + "hostedBy": tc.hostedByDID, + "visibility": "public", + "federation": map[string]interface{}{ + "allowExternalDiscovery": true, + }, + "memberCount": 0, + "subscriberCount": 0, + "createdAt": time.Now().Format(time.RFC3339), + }, + }, + } + + err := consumer.HandleEvent(ctx, event) + if tc.shouldSucceed && err != nil { + t.Errorf("Expected success for %s, got error: %v", tc.handle, err) + } else if !tc.shouldSucceed && err == nil { + t.Errorf("Expected failure for %s, got success", tc.handle) + } + }) + } +} diff --git a/internal/atproto/jetstream/connector_test.go b/internal/atproto/jetstream/connector_test.go --- a/internal/atproto/jetstream/connector_test.go +++ b/internal/atproto/jetstream/connector_test.go @@ -224,8 +224,12 @@ type jetstreamTestServer struct { server *httptest.Server holdOpen bool mu sync.Mutex - cursors []string // cursor param per connection ("" if absent) + cursors []string // cursor param per ACCEPTED connection ("" if absent) messages [][]byte + // refuseFirst dials are answered with an HTTP error instead of a + // WebSocket upgrade, standing in for a Jetstream that is not up yet. + refuseFirst int + refused int } func newJetstreamTestServer(t *testing.T, messages [][]byte) *jetstreamTestServer { @@ -240,12 +244,31 @@ t.Helper() return newJetstreamTestServerWithHold(t, messages, false) } +// newRefusingJetstreamTestServer answers the first refuseFirst dials with an +// HTTP error — no WebSocket upgrade, so the client's dial itself fails — and +// serves normally from then on. It stands in for a Jetstream that has not +// finished booting, which is what a connector meets on a cold stack. +func newRefusingJetstreamTestServer(t *testing.T, messages [][]byte, refuseFirst int) *jetstreamTestServer { + t.Helper() + ts := newJetstreamTestServerWithHold(t, messages, true) + ts.mu.Lock() + ts.refuseFirst = refuseFirst + ts.mu.Unlock() + return ts +} + func newJetstreamTestServerWithHold(t *testing.T, messages [][]byte, holdOpen bool) *jetstreamTestServer { t.Helper() ts := &jetstreamTestServer{messages: messages, holdOpen: holdOpen} upgrader := websocket.Upgrader{} ts.server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ts.mu.Lock() + if ts.refused < ts.refuseFirst { + ts.refused++ + ts.mu.Unlock() + http.Error(w, "jetstream is not accepting connections", http.StatusServiceUnavailable) + return + } ts.cursors = append(ts.cursors, r.URL.Query().Get("cursor")) ts.mu.Unlock() @@ -288,6 +311,12 @@ func (ts *jetstreamTestServer) cursorForConnection(i int) string { ts.mu.Lock() defer ts.mu.Unlock() return ts.cursors[i] +} + +func (ts *jetstreamTestServer) refusedCount() int { + ts.mu.Lock() + defer ts.mu.Unlock() + return ts.refused } // --- Helpers --- @@ -670,6 +699,60 @@ t.Fatalf("ListRetryable failed: %v", err) } if len(retryable) != 0 { t.Errorf("expected redriver to skip exhausted permanent dead letter, got %d retryable rows", len(retryable)) + } +} + +// TestConnector_RetriesAfterFailedDial covers Start's dial-failure branch: a +// connect() error is recorded, slept on and dialled again, and the ONLY thing +// that ends the loop is context cancellation (connector.go's Start). +// +// Nothing else in this file reaches that branch. Both reconnect tests — +// ReconnectDialsWithAdvancedCursor and DeadLetterWriteFailureDoesNotAdvanceCursor +// — reconnect after a connection that SUCCEEDED and was then torn down, so a +// connector that returned on a failed dial would still pass them. What it would +// break is every AppView that starts before its Jetstream does: the consumer +// exits at boot, /health/consumers reports it disconnected forever, and no +// record is ever indexed again without a restart. +// +// It lives here, in the untagged unit build, because the retry loop is in this +// package and needs no infrastructure. It replaces the "Consumer retries on +// connection failure" subtest of the deleted tests/e2e/error_recovery_test.go, +// which pointed a connector at ws://invalid:9999 for three wall-clock seconds +// and then t.Logf'd whichever error came back — an assertion-free test that +// could not fail, in a tier whose rules forbid instantiating a consumer at all. +func TestConnector_RetriesAfterFailedDial(t *testing.T) { + const refusals = 2 + server := newRefusingJetstreamTestServer(t, [][]byte{testEventJSON(t, 21_000)}, refusals) + handler := newFakeEventHandler() + + connector := NewConnector("test-consumer", server.wsURL(), handler, + fastConnectorOptions(WithCursorStore(newFakeCursorStore()), WithDeadLetterWriter(newFakeDeadLetterQueue()))...) + startConnector(t, connector) + + waitFor(t, 2*time.Second, "every refused dial to be retried", func() bool { + return server.refusedCount() == refusals + }) + // The recovery, and the half that a give-up-on-first-error connector fails: + // the endpoint came back and the event was consumed with no intervention. + waitFor(t, 2*time.Second, "the event to be delivered once the endpoint accepted a dial", func() bool { + return handler.handledCount() == 1 + }) + + if got := server.connectionCount(); got != 1 { + t.Errorf("expected exactly 1 accepted connection after %d refusals, got %d", refusals, got) + } + + status := connector.Status() + if !status.Connected { + t.Error("the connector must report itself connected once a dial succeeded") + } + // lastError is never cleared, so this is deterministic once the refusals + // above have been observed. It matters because a dial failure has no other + // observable: /health/consumers is where an operator sees WHY a consumer + // that is retrying has not connected yet. + if status.LastError == "" { + t.Error("a refused dial must be recorded as the connector's last error, or a connector " + + "stuck retrying an unreachable endpoint reports no reason at all") } } diff --git a/internal/core/blobs/harness_test.go b/internal/core/blobs/harness_test.go new file mode 100644 --- /dev/null +++ b/internal/core/blobs/harness_test.go @@ -0,0 +1,34 @@ +//go:build integration + +package blobs_test + +import ( + "os" + "testing" + + "Coves/tests/testkit" +) + +// TestMain sets the infrastructure floor for this package's integration build. +// +// It lives in a tagged file because a TestMain applies to the whole test +// binary: the untagged unit build of this package (types_test.go) needs nothing +// out of process and must not be made to probe Postgres before it can run. +// +// The floor is Postgres AND a real PDS. Postgres because the tests drive the +// blob service and the image-URL hydration the post and community read paths +// depend on through in-process handlers against a real schema. +// +// The PDS because blob upload cannot be faked and still mean anything: the +// service signs com.atproto.repo.uploadBlob with a community's credentials and +// keeps the CID the server returns, so the tests provision real accounts +// (testkit.NewPDS(t).CreateAccount) and upload real image bytes. A stubbed +// upload would assert that a fake returned what the fake was told to return. +// +// TestBlobUpload_PDS_MockServer is the exception that proves the floor is not +// over-broad: it points the service at an httptest server precisely to exercise +// the error shapes a real PDS will not produce on demand. It shares this binary, +// so the floor is the union. +func TestMain(m *testing.M) { + os.Exit(testkit.Main(m, testkit.RequirePostgres, testkit.RequirePDS)) +} diff --git a/internal/core/comments/harness_test.go b/internal/core/comments/harness_test.go new file mode 100644 --- /dev/null +++ b/internal/core/comments/harness_test.go @@ -0,0 +1,31 @@ +//go:build integration + +package comments_test + +import ( + "os" + "testing" + + "Coves/tests/testkit" +) + +// TestMain sets the infrastructure floor for this package's integration build. +// +// It lives in a tagged file because a TestMain applies to the whole test +// binary: the untagged unit build of this package (comment_service_test.go, +// comment_write_service_test.go) runs against fakes and must not be made to +// probe Postgres before it can run. +// +// The floor is Postgres and the PDS. comment_query_test.go, +// comment_consumer_test.go and comment_vote_test.go feed synthetic Jetstream +// events to the comment and vote consumers and read the results back through +// the repository and the query service — the §3.2 T1 consumer seam, which +// terminates at Postgres and needs nothing else. +// comment_write_test.go is the other T1 seam §3.4b names: the write path +// forwards to a real PDS, and asserting the record it actually wrote there is +// the coverage T2 cannot have while sealed sessions mint only in a browser. +// +// Neither dials a websocket: the consumer is fed directly. +func TestMain(m *testing.M) { + os.Exit(testkit.Main(m, testkit.RequirePostgres, testkit.RequirePDS)) +} diff --git a/internal/core/communities/consumer_profile_indexing_test.go b/internal/core/communities/consumer_profile_indexing_test.go new file mode 100644 --- /dev/null +++ b/internal/core/communities/consumer_profile_indexing_test.go @@ -0,0 +1,617 @@ +//go:build integration + +package communities_test + +import ( + "Coves/internal/atproto/identity" + "Coves/internal/atproto/jetstream" + "Coves/internal/core/communities" + "Coves/internal/db/postgres" + "Coves/tests/fixtures" + "Coves/tests/testkit" + "context" + "errors" + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// How a community profile record becomes a row. +// +// The firehose is the ONLY way a community that this instance does not host can +// enter the index, and — because UpdateCommunity deliberately does not write +// Postgres — it is also the only way an edit to a local community becomes +// visible. So this consumer is the write path for community data in the general +// case, and the assertions below are about the shape of the row it produces +// rather than about the events it accepts. +// +// # WHY THESE LIVE IN internal/core/communities +// +// They are consumer tests by their entry point and community tests by their +// subject: every assertion is made by reading back through +// communities.Repository, and what they pin is the community domain's +// invariants — self-ownership, the canonical "self" record key, the handle a +// community is addressed by. The consumer's own pure decoders (blob-ref +// extraction, contentVisibility clamping) are unit-tested in place, untagged, +// in internal/atproto/jetstream/community_consumer_test.go; what could not go +// there is anything that needs a real repository, which is all of this. +// +// # WHAT IS NOT HERE +// +// Events the consumer must ignore — other collections, identity and account +// events, a commit with no body — are covered by that same file's +// TestCommunityConsumer_IgnoresUnrelatedCollections, which asserts it against a +// NIL repository, so anything that reached a database call would panic rather +// than pass. That is a stronger statement than a Postgres-backed test can make, +// and it made the version of those cases that used to live alongside these +// redundant. +// +// Verification of the hostedBy claim is switched off in every consumer built +// here (the third constructor argument), matching CI. Verification dials the +// hosting domain's DID document over the network, which no T1 test may depend +// on; its own coverage is in the hostedBy security tests. + +// stubIdentityResolver answers handle resolutions from a map instead of from +// the PLC directory. +// +// The consumer's production path resolves a community's handle from its DID +// because handles are mutable and records must not carry them. That is a +// network call to a service this tier may not touch, and — more to the point — +// what these tests care about is what the consumer DOES with the answer, and +// whether it calls at all. +type stubIdentityResolver struct { + resolutions map[string]string + lastDID string + callCount int + shouldFail bool +} + +func newStubIdentityResolver() *stubIdentityResolver { + return &stubIdentityResolver{resolutions: make(map[string]string)} +} + +func (s *stubIdentityResolver) Resolve(_ context.Context, did string) (*identity.Identity, error) { + s.callCount++ + s.lastDID = did + + if s.shouldFail { + return nil, errors.New("stub PLC resolution failure") + } + + handle, configured := s.resolutions[did] + if !configured { + return nil, fmt.Errorf("no resolution configured for DID: %s", did) + } + + return &identity.Identity{ + DID: did, + Handle: handle, + PDSURL: "https://pds.example.com", + ResolvedAt: time.Now(), + Method: identity.MethodHTTPS, + }, nil +} + +// newCommunityConsumer builds the consumer over a fresh database clone and +// returns the repository the assertions read through. +func newCommunityConsumer(t *testing.T, resolver *stubIdentityResolver) ( + *jetstream.CommunityEventConsumer, communities.Repository, +) { + t.Helper() + + repo := postgres.NewCommunityRepository(testkit.DB(t)) + if resolver == nil { + // A typed nil in an interface parameter is not nil, and the consumer + // branches on the interface being nil to decide whether it is in + // handle-construction mode. Passing the untyped nil is the difference + // between exercising that branch and panicking inside it. + return jetstream.NewCommunityEventConsumer(repo, instanceDID, true, nil), repo + } + return jetstream.NewCommunityEventConsumer(repo, instanceDID, true, resolver), repo +} + +// profileRecord is a valid community profile with the fields every branch of +// the consumer reads. Tests mutate the copy they are given. +// +// Note what is absent: no "did", no "handle", no counts. Those are resolved or +// computed by the AppView, and a record carrying them would be asserting facts +// its author cannot know. +func profileRecord(name string) map[string]interface{} { + return map[string]interface{}{ + "$type": "social.coves.community.profile", + "name": name, + "displayName": "Consumer Indexed", + "description": "a community that arrived over the firehose", + "createdBy": "did:plc:communityconsumer", + "hostedBy": instanceDID, + "visibility": "public", + "federation": map[string]interface{}{ + "allowExternalDiscovery": true, + }, + "createdAt": time.Now().UTC().Format(time.RFC3339), + } +} + +// profileEvent wraps a record in the commit envelope Jetstream delivers. +func profileEvent(did, operation, rkey, cid string, record map[string]interface{}) *jetstream.JetstreamEvent { + return &jetstream.JetstreamEvent{ + Did: did, + TimeUS: time.Now().UnixMicro(), + Kind: "commit", + Commit: &jetstream.CommitEvent{ + Rev: testkit.TID(), + Operation: operation, + Collection: "social.coves.community.profile", + RKey: rkey, + CID: cid, + Record: record, + }, + } +} + +// TestCommunityConsumer_IgnoresForeignCollections and its sibling below are the +// dispatch guard, against a REAL repository. +// +// internal/atproto/jetstream/community_consumer_test.go already asserts that +// these events are ignored, and does it with a nil repository — which is the +// stronger proof that nothing REACHED a repo call, because anything that did +// would panic. These two are not a duplicate of that, and the difference is the +// reason they are worth their lines: a nil repo cannot distinguish "the +// consumer ignored the event" from "the consumer would have written a row but +// crashed before it could". Here the write would succeed, so asserting the +// community is still absent afterwards is a statement about the DATABASE rather +// than about a panic. +// +// The volume argument is why the guard matters at all. Every consumer shares one +// feed, and account and identity events bypass wantedCollections entirely, so +// the overwhelming majority of what this consumer is handed is something it must +// do nothing with. A dispatch bug here is not a wrong row in one place; it is +// every unrelated event in the firehose landing in the communities table. +func TestCommunityConsumer_IgnoresForeignCollections(t *testing.T) { + t.Parallel() + + consumer, repo := newCommunityConsumer(t, newStubIdentityResolver()) + ctx := context.Background() + + name := testkit.UniqueIDWithPrefix(t, "frn") + communityDID := fixtures.DID(name) + + // Each carries a well-formed community profile as its record and the + // community's own repo DID, so the ONLY thing telling the consumer to leave + // it alone is the collection. An event that differed in several ways at once + // could be ignored for the wrong reason and still pass. + for _, collection := range []string{ + "social.coves.community.post", + "social.coves.community.comment", + "social.coves.actor.profile", + "social.coves.feed.vote", + "app.bsky.feed.post", + } { + event := profileEvent(communityDID, "create", "self", "bafyforeign", profileRecord(name)) + event.Commit.Collection = collection + + require.NoErrorf(t, consumer.HandleEvent(ctx, event), + "an event for %s must be ignored, not treated as an error: it will arrive constantly, "+ + "and a consumer that errors on it dead-letters most of the firehose", collection) + + _, err := repo.GetByDID(ctx, communityDID) + require.Errorf(t, err, + "an event for %s was indexed as a community profile — the consumer is dispatching on "+ + "something other than the collection", collection) + } +} + +// TestCommunityConsumer_IgnoresNonCommitKinds covers the events that reach this +// consumer no matter what it subscribed to. +// +// Jetstream's wantedCollections filter applies to commits only: identity and +// account events are delivered to every subscriber regardless, which is the same +// property that keeps -p 1 in place for the whole suite (tests/testkit/db.go's +// packageParallelism). A bodyless commit is here for a different reason — it is +// the shape that dereferences a nil Commit if the consumer checks the kind +// without checking the body. +func TestCommunityConsumer_IgnoresNonCommitKinds(t *testing.T) { + t.Parallel() + + consumer, repo := newCommunityConsumer(t, newStubIdentityResolver()) + ctx := context.Background() + + communityDID := fixtures.DID(testkit.UniqueIDWithPrefix(t, "knd")) + + for _, event := range []*jetstream.JetstreamEvent{ + {Kind: "identity", Did: communityDID, TimeUS: time.Now().UnixMicro()}, + {Kind: "account", Did: communityDID, TimeUS: time.Now().UnixMicro()}, + {Kind: "commit", Did: communityDID, TimeUS: time.Now().UnixMicro()}, + } { + require.NoErrorf(t, consumer.HandleEvent(ctx, event), + "a %q event must be ignored: it arrives whatever this consumer subscribed to", event.Kind) + } + + _, err := repo.GetByDID(ctx, communityDID) + require.Error(t, err, "a non-commit event created a community row") +} + +func TestCommunityConsumer_IndexesAProfileFromTheFirehose(t *testing.T) { + t.Parallel() + + name := testkit.UniqueIDWithPrefix(t, "idx") + communityDID := fixtures.DID(name) + + resolver := newStubIdentityResolver() + resolver.resolutions[communityDID] = "c-" + name + "." + instanceDomain + consumer, repo := newCommunityConsumer(t, resolver) + ctx := context.Background() + + require.NoError(t, consumer.HandleEvent(ctx, + profileEvent(communityDID, "create", "self", "bafyindexedprofile", profileRecord(name)))) + + indexed, err := repo.GetByDID(ctx, communityDID) + require.NoError(t, err, "the event was accepted but no row appeared") + + assert.Equal(t, "Consumer Indexed", indexed.DisplayName) + assert.Equal(t, "public", indexed.Visibility) + assert.True(t, indexed.AllowExternalDiscovery) + + // V2 self-ownership. The community's repo DID IS the community, so there is + // no separate owner to get wrong — but the column exists, and a consumer + // that filled it from the record's "owner" field instead of from the event's + // repo DID would let a record name somebody else as its owner. + assert.Equal(t, indexed.DID, indexed.OwnerDID, "a V2 community owns itself") + + // The record URI is built from the EVENT's repo DID, not from anything in + // the record. A regression here would file every federated community's + // profile as living in whichever repo the AppView happened to think it + // owned, and nothing else looks at this column. + assert.Equal(t, "at://"+communityDID+"/social.coves.community.profile/self", indexed.RecordURI) + assert.Equal(t, "bafyindexedprofile", indexed.RecordCID, + "the row must record the commit's CID: it is how a later event is told apart from a replay") +} + +func TestCommunityConsumer_UpdatesAnIndexedProfile(t *testing.T) { + t.Parallel() + + name := testkit.UniqueIDWithPrefix(t, "upc") + communityDID := fixtures.DID(name) + handle := "c-" + name + "." + instanceDomain + + resolver := newStubIdentityResolver() + resolver.resolutions[communityDID] = handle + consumer, repo := newCommunityConsumer(t, resolver) + ctx := context.Background() + + _, err := repo.Create(ctx, &communities.Community{ + DID: communityDID, + Handle: handle, + Name: name, + DisplayName: "Original Name", + Description: "Original description", + OwnerDID: communityDID, + CreatedByDID: "did:plc:communityconsumer", + HostedByDID: instanceDID, + Visibility: "public", + AllowExternalDiscovery: true, + CreatedAt: time.Now(), + UpdatedAt: time.Now(), + }) + require.NoError(t, err) + + record := profileRecord(name) + record["displayName"] = "Updated Name" + record["description"] = "Updated description" + record["visibility"] = "unlisted" + record["federation"] = map[string]interface{}{"allowExternalDiscovery": false} + + require.NoError(t, consumer.HandleEvent(ctx, + profileEvent(communityDID, "update", "self", "bafyupdatedprofile", record))) + + updated, err := repo.GetByDID(ctx, communityDID) + require.NoError(t, err) + assert.Equal(t, "Updated Name", updated.DisplayName) + assert.Equal(t, "Updated description", updated.Description) + assert.Equal(t, "unlisted", updated.Visibility) + assert.False(t, updated.AllowExternalDiscovery, + "turning discovery OFF is the direction that matters: a community that asked to stop being "+ + "listed and stayed listed is a privacy failure, not a stale field") +} + +func TestCommunityConsumer_DeletesAnIndexedProfile(t *testing.T) { + t.Parallel() + + name := testkit.UniqueIDWithPrefix(t, "del") + communityDID := fixtures.DID(name) + + consumer, repo := newCommunityConsumer(t, nil) + ctx := context.Background() + + _, err := repo.Create(ctx, &communities.Community{ + DID: communityDID, + Handle: "c-" + name + "." + instanceDomain, + Name: name, + OwnerDID: communityDID, + CreatedByDID: "did:plc:communityconsumer", + HostedByDID: instanceDID, + Visibility: "public", + CreatedAt: time.Now(), + UpdatedAt: time.Now(), + }) + require.NoError(t, err) + + // A delete commit carries no record at all, which is worth exercising + // separately: every other branch dereferences commit.Record. + require.NoError(t, consumer.HandleEvent(ctx, &jetstream.JetstreamEvent{ + Did: communityDID, + TimeUS: time.Now().UnixMicro(), + Kind: "commit", + Commit: &jetstream.CommitEvent{ + Rev: testkit.TID(), + Operation: "delete", + Collection: "social.coves.community.profile", + RKey: "self", + }, + })) + + _, err = repo.GetByDID(ctx, communityDID) + assert.True(t, errors.Is(err, communities.ErrCommunityNotFound), + "the community is still indexed after its profile record was deleted, got %v", err) +} + +// TestCommunityConsumer_RejectsAnyProfileRKeyButSelf pins the V2 record-key +// rule at the point it is enforced. +// +// A community profile lives at exactly one key in its repo: "self". That is +// what makes a community's profile addressable without an index — anyone +// holding the DID can construct the AT-URI — and it is what makes the record +// updatable in place rather than accumulating versions. V1 used a TID, and +// pre-production means no compatibility: a TID-keyed record is not an older +// community to migrate, it is a record no part of this system can address. +// +// The rejection is PERMANENT rather than retryable, and that distinction is +// asserted: a record key is immutable, so redriving the event a thousand times +// produces the same failure. Classifying it as transient would put it in the +// retry queue forever. +func TestCommunityConsumer_RejectsAnyProfileRKeyButSelf(t *testing.T) { + t.Parallel() + + consumer, repo := newCommunityConsumer(t, nil) + ctx := context.Background() + + t.Run("a V1 TID record key", func(t *testing.T) { + name := testkit.UniqueIDWithPrefix(t, "v1k") + communityDID := fixtures.DID(name) + + err := consumer.HandleEvent(ctx, + profileEvent(communityDID, "create", "3k2j4h5g6f7d", "bafyv1community", profileRecord(name))) + + require.Error(t, err, "a TID-keyed community profile must be rejected") + assert.ErrorContains(t, err, + "invalid community profile rkey: expected 'self', got '3k2j4h5g6f7d' (V1 communities not supported)") + assert.True(t, errors.Is(err, jetstream.ErrPermanentEvent), + "an immutable record key can never succeed on redrive; classifying it as transient would "+ + "keep the event in the retry queue forever") + + _, err = repo.GetByDID(ctx, communityDID) + assert.True(t, errors.Is(err, communities.ErrCommunityNotFound), + "the rejected community was indexed anyway") + }) + + t.Run("an arbitrary record key", func(t *testing.T) { + name := testkit.UniqueIDWithPrefix(t, "ark") + communityDID := fixtures.DID(name) + + err := consumer.HandleEvent(ctx, + profileEvent(communityDID, "create", "custom-profile-name", "bafycustomrkey", profileRecord(name))) + + require.Error(t, err) + _, err = repo.GetByDID(ctx, communityDID) + assert.True(t, errors.Is(err, communities.ErrCommunityNotFound)) + }) + + t.Run("an update to a key that is not self", func(t *testing.T) { + // The update path has its own copy of the check, and it runs BEFORE the + // record is parsed. An update is the dangerous direction: the community + // already exists, so a missed check would overwrite a real row from a + // record written at a key its author chose. + name := testkit.UniqueIDWithPrefix(t, "urk") + communityDID := fixtures.DID(name) + + require.NoError(t, consumer.HandleEvent(ctx, + profileEvent(communityDID, "create", "self", "bafybeforeupdate", profileRecord(name)))) + + record := profileRecord(name) + record["displayName"] = "Overwritten Through The Wrong Key" + err := consumer.HandleEvent(ctx, + profileEvent(communityDID, "update", "wrong-rkey", "bafyafterupdate", record)) + require.Error(t, err) + + unchanged, err := repo.GetByDID(ctx, communityDID) + require.NoError(t, err, "the original community must still be there") + assert.Equal(t, "Consumer Indexed", unchanged.DisplayName, + "the rejected update was applied anyway") + }) +} + +// TestCommunityConsumer_KeepsTheHandleTheRecordCarries covers the legacy shape: +// a profile record with a "handle" field in it. +// +// Handles are mutable and DIDs are not, so a record should not carry one at all +// — but records that do exist, and the consumer takes the record's word for it +// rather than resolving. What is asserted is that it is stored verbatim, since +// this is the string every client uses to address the community. +func TestCommunityConsumer_KeepsTheHandleTheRecordCarries(t *testing.T) { + t.Parallel() + + name := testkit.UniqueIDWithPrefix(t, "hnd") + communityDID := fixtures.DID(name) + handle := "c-" + name + "." + instanceDomain + + // The resolver would answer with something else entirely. It must not be + // consulted: a record carrying its own handle short-circuits resolution, and + // this proves that rather than assuming it. + resolver := newStubIdentityResolver() + resolver.resolutions[communityDID] = "c-resolver-would-say-this." + instanceDomain + consumer, repo := newCommunityConsumer(t, resolver) + ctx := context.Background() + + record := profileRecord(name) + record["handle"] = handle + + require.NoError(t, consumer.HandleEvent(ctx, + profileEvent(communityDID, "create", "self", "bafyhandlecarried", record))) + + indexed, err := repo.GetByDID(ctx, communityDID) + require.NoError(t, err) + assert.Equal(t, handle, indexed.Handle) + assert.Zero(t, resolver.callCount, "a record that carries a handle must not trigger PLC resolution") +} + +// TestCommunityConsumer_ResolvesAMissingHandleFromPLC is the modern path: no +// handle in the record, so the consumer asks the DID for one. +func TestCommunityConsumer_ResolvesAMissingHandleFromPLC(t *testing.T) { + t.Parallel() + + name := testkit.UniqueIDWithPrefix(t, "plc") + communityDID := fixtures.DID(name) + resolvedHandle := "c-" + name + "." + instanceDomain + + resolver := newStubIdentityResolver() + resolver.resolutions[communityDID] = resolvedHandle + consumer, repo := newCommunityConsumer(t, resolver) + ctx := context.Background() + + require.NoError(t, consumer.HandleEvent(ctx, + profileEvent(communityDID, "create", "self", "bafyplcresolved", profileRecord(name)))) + + assert.Equal(t, 1, resolver.callCount, "the handle must be resolved exactly once per indexed community") + assert.Equal(t, communityDID, resolver.lastDID, "resolution must be asked about the community's own DID") + + indexed, err := repo.GetByDID(ctx, communityDID) + require.NoError(t, err) + assert.Equal(t, resolvedHandle, indexed.Handle) + + // The PDS URL learned during resolution has to be stored, and the reason is + // not obvious: BridgeTrust gates bridged vote counts on a post's community + // row naming the PDS its repo lives on. A federated community indexed here + // with an empty pds_url makes that gate default-deny for good. + assert.Equal(t, "https://pds.example.com", indexed.PDSURL, + "the resolved PDS host must be persisted, or bridged stats are denied for this community forever") +} + +func TestCommunityConsumer_FailsRatherThanGuessWhenPLCResolutionFails(t *testing.T) { + t.Parallel() + + name := testkit.UniqueIDWithPrefix(t, "pfl") + communityDID := fixtures.DID(name) + + resolver := newStubIdentityResolver() + resolver.shouldFail = true + consumer, repo := newCommunityConsumer(t, resolver) + ctx := context.Background() + + err := consumer.HandleEvent(ctx, + profileEvent(communityDID, "create", "self", "bafyplcfailed", profileRecord(name))) + + // There is deliberately NO fallback. Constructing a handle from the record's + // own fields would be easy and would be wrong: for a federated community the + // constructed handle would name the wrong domain, and the row would then + // look perfectly healthy while addressing a community nobody can reach. A + // failed event is retried on backfill; a wrong row is not. + require.Error(t, err, "PLC resolution failed and the community was indexed anyway") + assert.ErrorContains(t, err, "failed to resolve handle from PLC") + assert.Equal(t, 1, resolver.callCount, "the failure must come from the resolution attempt itself") + + _, err = repo.GetByDID(ctx, communityDID) + assert.True(t, communities.IsNotFound(err), + "nothing may be indexed for a community whose handle could not be established, got %v", err) +} + +func TestCommunityConsumer_RefusesAProfileWhoseHandleCannotBeBuilt(t *testing.T) { + t.Parallel() + + name := testkit.UniqueIDWithPrefix(t, "hby") + communityDID := fixtures.DID(name) + + // No resolver: the consumer falls back to constructing the handle from + // hostedBy, which only works for a did:web. A did:plc there yields an empty + // handle, and the repository — not the consumer — is what refuses it. + consumer, repo := newCommunityConsumer(t, nil) + ctx := context.Background() + + record := profileRecord(name) + record["hostedBy"] = "did:plc:invalid" + + err := consumer.HandleEvent(ctx, + profileEvent(communityDID, "create", "self", "bafybadhostedby", record)) + + require.Error(t, err, "a community with no derivable handle must not be indexed") + assert.ErrorContains(t, err, "handle is required") + + _, err = repo.GetByDID(ctx, communityDID) + assert.True(t, communities.IsNotFound(err)) +} + +// TestCommunityConsumer_IndexesASubscriptionAndCountsIt covers the other +// collection this consumer owns, and the aggregate it maintains. +// +// The subscriber count is denormalised onto the community row because every +// listing renders it, so it is only ever correct if this path maintains it. A +// subscription indexed without the increment is invisible until someone +// recounts. +func TestCommunityConsumer_IndexesASubscriptionAndCountsIt(t *testing.T) { + t.Parallel() + + name := testkit.UniqueIDWithPrefix(t, "sub") + communityDID := fixtures.DID(name) + + consumer, repo := newCommunityConsumer(t, nil) + ctx := context.Background() + + _, err := repo.Create(ctx, &communities.Community{ + DID: communityDID, + Handle: "c-" + name + "." + instanceDomain, + Name: name, + OwnerDID: communityDID, + CreatedByDID: "did:plc:communityconsumer", + HostedByDID: instanceDID, + Visibility: "public", + CreatedAt: time.Now(), + UpdatedAt: time.Now(), + }) + require.NoError(t, err) + + // The collection is the RECORD TYPE, social.coves.community.subscription — + // not social.coves.community.subscribe, which is the XRPC procedure that + // creates it. Writing the procedure NSID as a collection produces a record + // no consumer is subscribed to: the write succeeds, the client sees 200, and + // the subscription silently never indexes. + userDID := "did:plc:subscriber" + name + require.NoError(t, consumer.HandleEvent(ctx, &jetstream.JetstreamEvent{ + Did: userDID, + TimeUS: time.Now().UnixMicro(), + Kind: "commit", + Commit: &jetstream.CommitEvent{ + Rev: testkit.TID(), + Operation: "create", + Collection: "social.coves.community.subscription", + RKey: testkit.TID(), + CID: "bafysubscription", + Record: map[string]interface{}{ + "subject": communityDID, + "contentVisibility": 3, + "createdAt": time.Now().UTC().Format(time.RFC3339), + }, + }, + })) + + subscription, err := repo.GetSubscription(ctx, userDID, communityDID) + require.NoError(t, err, "the subscription event was accepted but nothing was indexed") + assert.Equal(t, userDID, subscription.UserDID) + assert.Equal(t, communityDID, subscription.CommunityDID) + + counted, err := repo.GetByDID(ctx, communityDID) + require.NoError(t, err) + assert.Equal(t, 1, counted.SubscriberCount, + "the denormalised subscriber count must move with the subscription that caused it") +} diff --git a/internal/core/communities/provisioner_failure_test.go b/internal/core/communities/provisioner_failure_test.go new file mode 100644 --- /dev/null +++ b/internal/core/communities/provisioner_failure_test.go @@ -0,0 +1,155 @@ +//go:build integration + +package communities_test + +import ( + "Coves/internal/core/communities" + "Coves/tests/testkit" + "context" + "net" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// What the provisioner does when the PDS is not there. +// +// Provisioning is the one step of community creation that cannot be undone by +// returning an error: it registers a DID with the PLC directory. Everything +// after it in CreateCommunity — the profile record, the database row — is +// downstream of a network call to a server that may be misconfigured, +// unreachable, or simply slow, and the service's contract with its caller is +// that all three come back as an error rather than as a panic, a hang, or a +// half-created community. +// +// These are negative-path tests almost end to end, so they include one positive +// case: FetchPDSDID against the real test PDS. Six assertions that a call fails +// are worth much less without one showing it can succeed — otherwise a +// provisioner broken for every input would pass the whole file. +// +// No database is touched here; the provisioner does not have one. The package's +// Postgres floor is paid for by its neighbours. + +// unreachableAddress returns a URL for a port that nothing is listening on. +// +// Taken by binding port zero and immediately releasing it, rather than by +// picking a number that looks unused: a hardcoded high port is occupied often +// enough on a developer's machine to make "connection refused" occasionally +// become "unexpected response from something else entirely". +func unreachableAddress(t *testing.T) string { + t.Helper() + listener, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err, "could not reserve a local port") + address := listener.Addr().String() + require.NoError(t, listener.Close()) + return "http://" + address +} + +func TestProvisioner_FailsClosedWhenThePDSCannotBeReached(t *testing.T) { + t.Parallel() + + // The subtests are parallel because each one waits out the same retry ladder + // against a dead endpoint. They share nothing — no database, no fixtures, a + // fresh provisioner each — so running them serially only adds their backoffs + // together. + ctx := context.Background() + + t.Run("rejects a PDS URL that is not usable", func(t *testing.T) { + t.Parallel() + + // Configuration errors, not network ones: each of these is something an + // operator can put in PDS_URL, and each must fail at provisioning rather + // than being coerced into some default host. + for _, badURL := range []string{ + "not-a-url", + "ftp://invalid-protocol.com", + "http://", + "://missing-scheme", + "", + } { + provisioner := communities.NewPDSAccountProvisioner(instanceDomain, badURL) + _, err := provisioner.ProvisionCommunityAccount(ctx, "testcommunity") + assert.Errorf(t, err, "provisioning against PDS URL %q must fail", badURL) + } + }) + + t.Run("reports an unreachable PDS", func(t *testing.T) { + t.Parallel() + + provisioner := communities.NewPDSAccountProvisioner(instanceDomain, unreachableAddress(t)) + _, err := provisioner.ProvisionCommunityAccount(ctx, "testcommunity") + + require.Error(t, err) + assert.ErrorContains(t, err, "PDS account creation failed", + "the error has to say which step failed: an operator reading a log needs to know the "+ + "community has no account at all, rather than an account with no record") + assert.ErrorContains(t, err, "testcommunity", + "and which community it was for") + }) + + t.Run("honours a cancelled context", func(t *testing.T) { + t.Parallel() + + // Already past its deadline before the call begins. The point is not the + // duration — it is that the deadline is observed at all: CreateCommunity + // is called from an HTTP handler whose context is cancelled when the + // client disconnects, and a provisioner that ignored it would keep + // registering DIDs for requests nobody is waiting on. + expired, cancel := context.WithTimeout(ctx, time.Nanosecond) + defer cancel() + + provisioner := communities.NewPDSAccountProvisioner(instanceDomain, testkit.Endpoints().PDS.BaseURL) + _, err := provisioner.ProvisionCommunityAccount(expired, "testcommunity") + require.Error(t, err, "a request with an expired deadline must not reach a live PDS") + }) +} + +func TestFetchPDSDID(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + // The positive case, and the reason the negative ones below mean anything. + // FetchPDSDID exists so the instance never hardcodes its PDS' identity — + // did:web:localhost in development, did:web:pds.example.com in production — + // so the assertion is simply that a real server answers with something. + t.Run("reads the DID from a live PDS", func(t *testing.T) { + t.Parallel() + + did, err := communities.FetchPDSDID(ctx, testkit.Endpoints().PDS.BaseURL) + require.NoError(t, err) + assert.NotEmpty(t, did) + assert.Contains(t, did, "did:", "com.atproto.server.describeServer must answer with a DID, got %q", did) + }) + + t.Run("rejects a URL that is not usable", func(t *testing.T) { + t.Parallel() + + for _, badURL := range []string{"not-a-url", "http://", ""} { + _, err := communities.FetchPDSDID(ctx, badURL) + assert.Errorf(t, err, "FetchPDSDID must fail for %q rather than return an empty DID", badURL) + } + }) + + t.Run("reports an unreachable server", func(t *testing.T) { + t.Parallel() + + address := unreachableAddress(t) + _, err := communities.FetchPDSDID(ctx, address) + require.Error(t, err) + assert.ErrorContains(t, err, "failed to describe server") + assert.ErrorContains(t, err, address, "the error must name the server that did not answer") + }) + + t.Run("honours a cancelled context", func(t *testing.T) { + t.Parallel() + + expired, cancel := context.WithTimeout(ctx, time.Nanosecond) + defer cancel() + + _, err := communities.FetchPDSDID(expired, testkit.Endpoints().PDS.BaseURL) + require.Error(t, err) + }) +} diff --git a/internal/core/communities/service_create_validation_test.go b/internal/core/communities/service_create_validation_test.go new file mode 100644 --- /dev/null +++ b/internal/core/communities/service_create_validation_test.go @@ -0,0 +1,166 @@ +//go:build integration + +package communities_test + +import ( + "Coves/internal/core/communities" + "Coves/tests/testkit" + "context" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// What a community may be called, and why the rule is DNS rather than taste. +// +// A community's name is not a label on a row: the provisioner interpolates it +// into "c-%s.%s" and registers the result as a real atProto handle +// (pds_provisioning.go), so the name has to be a legal DNS label before +// anything else happens. That makes name validation a gate in front of an +// account creation on a live PDS, and the ordering matters twice over — a name +// that slips through produces a PDS account whose handle nothing can resolve, +// and a name rejected too late has already cost a network round trip and, on +// the update path, an uploaded blob. +// +// # WHY THE ACCEPTING CASES LOOK ODD +// +// The rejecting direction can be asserted exactly: validation fails before the +// provisioner is called, so the error is a validation error and nothing was +// created. The accepting direction cannot, because "the name was accepted" is +// only observable by watching the request continue INTO provisioning, which +// then succeeds or fails on its own terms. So those cases assert the negative +// that actually matters — that whatever went wrong, it was not the name — and +// the one case that can be fully asserted (a legal name that provisions) is +// asserted fully. +// +// Names are generated rather than literal wherever an account is really +// created. A test that provisions "c-gaming.coves.social" squats that handle on +// a PDS which keeps accounts between runs, and every later run silently +// exercises the handle-taken path instead of the one it meant to. + +func TestService_CreateRejectsNamesAHandleCannotCarry(t *testing.T) { + t.Parallel() + + service, _, _ := newCommunityService(t) + ctx := context.Background() + + // createWith runs the request and returns only the error: none of these + // reach the PDS, so there is never a community to look at. + createWith := func(name string) error { + _, err := service.CreateCommunity(ctx, communities.CreateCommunityRequest{ + Name: name, + DisplayName: "Name Validation", + Description: "a name the handle scheme cannot carry", + Visibility: "public", + CreatedByDID: "did:plc:namevalidation", + AllowExternalDiscovery: true, + }) + return err + } + + t.Run("rejects empty name", func(t *testing.T) { + err := createWith("") + require.Error(t, err) + assert.ErrorContains(t, err, "name") + assert.True(t, communities.IsValidationError(err)) + }) + + t.Run("rejects a name over the 63-character DNS label limit", func(t *testing.T) { + err := createWith(strings.Repeat("a", 64)) + require.Error(t, err) + assert.ErrorContains(t, err, "63", + "the limit has to appear in the message: a client cannot shorten a name to fit a number it was not told") + assert.ErrorContains(t, err, "name") + assert.True(t, communities.IsValidationError(err)) + }) + + // Every character here is one that changes what the resulting handle MEANS + // rather than merely looking wrong: a dot adds a label, an @ or a ! collides + // with the scoped-identifier syntax the resolver parses, whitespace makes a + // handle that cannot be typed, and a leading or trailing hyphen is illegal + // in a DNS label even though it looks harmless. + for _, testCase := range []struct { + description string + name string + }{ + {"exclamation mark", "test!community"}, + {"at symbol", "test@space"}, + {"space", "test community"}, + {"period", "test.community"}, + {"underscore", "test_community"}, + {"hash", "test#tag"}, + {"leading hyphen", "-testcommunity"}, + {"trailing hyphen", "testcommunity-"}, + } { + t.Run("rejects a name containing "+testCase.description, func(t *testing.T) { + err := createWith(testCase.name) + require.Errorf(t, err, "%q is not a legal DNS label", testCase.name) + assert.ErrorContains(t, err, "name") + assert.Truef(t, communities.IsValidationError(err), + "%q must be refused as invalid input, not attempted against the PDS", testCase.name) + }) + } +} + +func TestService_CreateAcceptsLegalDNSLabelNames(t *testing.T) { + t.Parallel() + + service, _, pdsServer := newCommunityService(t) + ctx := context.Background() + + t.Run("a name of hyphens, digits and mixed case provisions", func(t *testing.T) { + // One generated name carrying all three legal-but-unusual features, so + // the case that really creates an account creates exactly one. + name := testkit.UniqueIDWithPrefix(t, "Ok-9") + require.LessOrEqualf(t, len("c-"+name), testkit.MaxIDLength, + "the generated community name %q makes a handle label the PDS will refuse", name) + + community, err := service.CreateCommunity(ctx, communities.CreateCommunityRequest{ + Name: name, + DisplayName: "Legal Name", + Description: "hyphens, digits and mixed case are all legal in a DNS label", + Visibility: "public", + CreatedByDID: "did:plc:namevalidation", + AllowExternalDiscovery: true, + }) + require.NoError(t, err) + + // The stored name keeps the case the creator chose; the handle does not. + // DNS is case-insensitive and the provisioner lower-cases before building + // the handle, so a mixed-case name must not produce a mixed-case handle — + // the AppView looks communities up by a lower-cased handle string, and a + // row holding "c-Ok-9x.coves.social" would never be found by anyone. + assert.Equal(t, name, community.Name, "the name is stored as the creator typed it") + assert.Equal(t, strings.ToLower("c-"+name+"."+instanceDomain), community.Handle) + assert.Equal(t, community.Handle, strings.ToLower(community.Handle), + "a handle with upper case in it is a row the resolver's lower-cased lookup cannot reach") + + // And it is a handle the PDS agrees exists, which is the only proof that + // the characters survived account creation rather than being normalised + // away. + pdsServer.Login(t, community.Handle, community.PDSPassword) + }) + + t.Run("a 63-character name passes validation and fails later", func(t *testing.T) { + // 63 is the DNS label limit and therefore the largest name the service + // accepts — but "c-" plus 63 characters is 65, over the PDS' own handle + // label budget, so this request is refused by the PDS rather than by + // validation. That split is the assertion: the service must not be the + // one saying no, or it has moved the limit. + _, err := service.CreateCommunity(ctx, communities.CreateCommunityRequest{ + Name: strings.Repeat("a", 63), + DisplayName: "Maximum Length Name", + Description: "exactly at the DNS label limit", + Visibility: "public", + CreatedByDID: "did:plc:namevalidation", + AllowExternalDiscovery: true, + }) + require.Error(t, err, "the PDS refuses a 65-character handle label") + assert.False(t, communities.IsValidationError(err), + "a 63-character name is legal; the service must pass it to the provisioner, got %v", err) + assert.ErrorContains(t, err, "provision", + "the failure must come from account provisioning, not from name validation") + }) +} diff --git a/internal/core/communities/service_credentials_test.go b/internal/core/communities/service_credentials_test.go new file mode 100644 --- /dev/null +++ b/internal/core/communities/service_credentials_test.go @@ -0,0 +1,254 @@ +//go:build integration + +package communities_test + +import ( + "Coves/internal/core/communities" + "Coves/tests/testkit" + "context" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The credentials a community is provisioned with, and the two properties they +// have to hold at once. +// +// A community's PDS account is the AppView's to drive: every profile edit, and +// every post written into a community's repo, authenticates as the community. +// That makes the stored password unlike any other secret in the system. It has +// to be RECOVERABLE — hashing it would be the ordinary answer and would be +// wrong, because com.atproto.server.createSession needs the cleartext when the +// refresh token finally expires — and it has to be UNREADABLE at rest, because +// a database dump would otherwise be a set of live logins to every community +// this instance hosts. +// +// pgcrypto's pgp_sym_encrypt is what reconciles the two, applied in the +// repository's SQL rather than in Go. Nothing in the type system says so: the +// Community struct carries a plain string on both sides of the write, and a +// repository that quietly stopped encrypting would still round-trip perfectly. +// The only way to see the difference is to read the column directly, which is +// why these tests hold the database handle as well as the service. +// +// # WHAT IS ASSERTED ELSEWHERE +// +// Encryption of the token columns and their behaviour on empty input belong to +// the repository and are covered by TestCommunityRepository_EncryptedCredentials +// and TestCommunityRepository_CredentialPersistence. What is here is the +// provisioning seam: what the service PRODUCES, that it survives the trip +// through the repository, and that what comes back still works against a real +// PDS. + +// knownWeakPasswords are the strings a hardcoded test credential tends to be. +// The provisioner draws 32 bytes from crypto/rand, so hitting one of these is +// not a coincidence to investigate — it means the random path was bypassed. +var knownWeakPasswords = []string{"", "test-password", "password123", "admin", "changeme"} + +func TestService_CreateStoresCredentialsEncryptedAndRecoverable(t *testing.T) { + t.Parallel() + + service, repo, pdsServer, db := newCommunityServiceWithDatabase(t) + ctx := context.Background() + + name := testkit.UniqueIDWithPrefix(t, "cred") + require.LessOrEqualf(t, len("c-"+name), testkit.MaxIDLength, + "the generated community name %q makes a handle label the PDS will refuse", name) + + community, err := service.CreateCommunity(ctx, communities.CreateCommunityRequest{ + Name: name, + DisplayName: "Credential Storage", + Description: "credentials that must survive encryption and still log in", + Visibility: "public", + CreatedByDID: "did:plc:credentialstorage", + AllowExternalDiscovery: true, + }) + require.NoError(t, err) + + // ---- what the provisioner produced --------------------------------------- + + assert.GreaterOrEqual(t, len(community.PDSPassword), 32, + "the provisioner draws a 32-character password; a shorter one means the length argument moved") + for _, weak := range knownWeakPasswords { + assert.NotEqual(t, weak, community.PDSPassword, + "the password is a hardcoded literal, not generated") + } + assert.Equal(t, pdsServer.URL(), community.PDSURL, + "the row has to record WHICH PDS these credentials are for, or a refresh has nowhere to go") + + // ---- what the database holds -------------------------------------------- + + var encryptedPassword []byte + require.NoError(t, db.QueryRowContext(ctx, + `SELECT pds_password_encrypted FROM communities WHERE did = $1`, community.DID, + ).Scan(&encryptedPassword)) + + require.NotEmpty(t, encryptedPassword, + "the column is empty: the password was never stored and this community can never re-authenticate") + assert.NotEqual(t, community.PDSPassword, string(encryptedPassword), + "CRITICAL: the password is in the database in cleartext") + assert.False(t, strings.HasPrefix(string(encryptedPassword), "$2"), + "the password looks bcrypt-hashed. A hash is the right answer for a password you VERIFY and the "+ + "wrong one for a password you must REPLAY: createSession needs the cleartext, so a hashed "+ + "column locks every community out the moment its refresh token expires") + + // ---- and that it comes back usable -------------------------------------- + + stored, err := repo.GetByDID(ctx, community.DID) + require.NoError(t, err) + assert.Equal(t, community.PDSPassword, stored.PDSPassword, "password did not survive the encryption round trip") + assert.Equal(t, community.PDSAccessToken, stored.PDSAccessToken) + assert.Equal(t, community.PDSRefreshToken, stored.PDSRefreshToken) + + // THE POINT OF ALL OF IT: the password read back out of the database opens a + // real session on the community's account. This is the P0 property — token + // renewal after the refresh token's window closes depends on exactly this + // call — and it is the one assertion that a decryption returning + // plausible-looking rubbish could not survive. + session := pdsServer.Login(t, stored.Handle, stored.PDSPassword) + assert.Equal(t, community.DID, session.DID, + "the stored credentials opened a session on a different account than the community they belong to") +} + +// TestService_ProvisionedPasswordsDifferBetweenCommunities is the entropy +// assertion, made against the generator rather than against the test's own +// inputs. +// +// Its predecessor inserted a hundred passwords it had built itself out of a +// counter and then checked that they were distinct, which is a property of the +// counter. Two real provisionings are worth more than a hundred synthetic ones: +// a generator that returned a constant, or that seeded itself per-process, +// fails here and could not fail there. +func TestService_ProvisionedPasswordsDifferBetweenCommunities(t *testing.T) { + t.Parallel() + + service, _, _ := newCommunityService(t) + ctx := context.Background() + + provision := func(prefix string) *communities.Community { + name := testkit.UniqueIDWithPrefix(t, prefix) + require.LessOrEqualf(t, len("c-"+name), testkit.MaxIDLength, + "the generated community name %q makes a handle label the PDS will refuse", name) + community, err := service.CreateCommunity(ctx, communities.CreateCommunityRequest{ + Name: name, + DisplayName: "Password Entropy", + Visibility: "public", + CreatedByDID: "did:plc:passwordentropy", + AllowExternalDiscovery: true, + }) + require.NoError(t, err) + return community + } + + first, second := provision("pw1"), provision("pw2") + + assert.NotEqual(t, first.PDSPassword, second.PDSPassword, + "two communities were provisioned with the SAME password: whoever learns one community's "+ + "credentials holds every community's") + assert.NotEqual(t, first.PDSEmail, second.PDSEmail, + "the account email is derived from the name and must be as unique as the handle") + for _, community := range []*communities.Community{first, second} { + assert.GreaterOrEqual(t, len(community.PDSPassword), 32) + } +} + +// TestService_EnsureFreshTokenRenewsAnExpiringTokenAndPersistsIt exercises the +// refresh path end to end against a real PDS. +// +// # WHY IT IS SHAPED THIS WAY +// +// A freshly provisioned access token is valid for hours, so nothing would +// refresh during a test run. The token is therefore REPLACED with an expired +// one while the genuine refresh token is left in place — which is precisely the +// state a community reaches on its own a couple of hours after provisioning, +// reproduced in a second instead of waited for. +// +// # WHY PERSISTENCE IS THE ASSERTION THAT MATTERS +// +// atProto refresh tokens are SINGLE USE: com.atproto.server.refreshSession +// revokes the one it was given. So a refresh that succeeds and then fails to +// write the new pair back leaves the community holding a revoked refresh token +// and an access token that is about to die — locked out until someone notices. +// The service's own comment calls this "COMMUNITY LOCKED OUT". Checking only +// the returned struct would pass in exactly that scenario, so the assertions +// are made against a re-read of the row. +func TestService_EnsureFreshTokenRenewsAnExpiringTokenAndPersistsIt(t *testing.T) { + t.Parallel() + + service, repo, _ := newCommunityService(t) + ctx := context.Background() + + name := testkit.UniqueIDWithPrefix(t, "tkn") + require.LessOrEqualf(t, len("c-"+name), testkit.MaxIDLength, + "the generated community name %q makes a handle label the PDS will refuse", name) + + community, err := service.CreateCommunity(ctx, communities.CreateCommunityRequest{ + Name: name, + DisplayName: "Token Refresh", + Visibility: "public", + CreatedByDID: "did:plc:tokenrefresh", + AllowExternalDiscovery: true, + }) + require.NoError(t, err) + + // A token that expired a minute ago, paired with the real refresh token. + expired := jwtExpiringAt(time.Now().Add(-time.Minute)) + require.NoError(t, repo.UpdateCredentials(ctx, community.DID, expired, community.PDSRefreshToken)) + + refreshed, err := service.EnsureFreshToken(ctx, community) + require.NoError(t, err, "the community could not renew its own session") + + assert.NotEqual(t, expired, refreshed.PDSAccessToken, "the expired token was handed straight back") + assert.NotEqual(t, community.PDSRefreshToken, refreshed.PDSRefreshToken, + "refreshSession issues a new refresh token and revokes the old one; an unchanged value here "+ + "means the response was not read") + + stillStale, err := communities.NeedsRefresh(refreshed.PDSAccessToken) + require.NoError(t, err, "the renewed access token is not a parseable JWT") + assert.False(t, stillStale, "the renewed access token is already inside the five-minute refresh window") + + stored, err := repo.GetByDID(ctx, community.DID) + require.NoError(t, err) + assert.Equal(t, refreshed.PDSAccessToken, stored.PDSAccessToken, + "the refreshed access token was not persisted") + assert.Equal(t, refreshed.PDSRefreshToken, stored.PDSRefreshToken, + "the single-use refresh token was not persisted: this community is now locked out") + assert.Equal(t, community.PDSPassword, stored.PDSPassword, + "a token refresh must not disturb the password — it is the fallback the whole scheme rests on "+ + "once the refresh token's own window closes") +} + +// TestService_EnsureFreshTokenLeavesAValidTokenAlone is the other half: the +// common case, where nothing should happen. +// +// A refresh that fires when it does not need to is not harmless. It spends the +// single-use refresh token, so a service that refreshed on every write would +// turn one PDS round trip per post into two and would serialise every write to +// a community behind the per-community refresh mutex. +func TestService_EnsureFreshTokenLeavesAValidTokenAlone(t *testing.T) { + t.Parallel() + + service, _, _ := newCommunityService(t) + ctx := context.Background() + + name := testkit.UniqueIDWithPrefix(t, "nrf") + require.LessOrEqualf(t, len("c-"+name), testkit.MaxIDLength, + "the generated community name %q makes a handle label the PDS will refuse", name) + + community, err := service.CreateCommunity(ctx, communities.CreateCommunityRequest{ + Name: name, + DisplayName: "No Refresh Needed", + Visibility: "public", + CreatedByDID: "did:plc:tokenrefresh", + AllowExternalDiscovery: true, + }) + require.NoError(t, err) + + unchanged, err := service.EnsureFreshToken(ctx, community) + require.NoError(t, err) + assert.Equal(t, community.PDSAccessToken, unchanged.PDSAccessToken, + "a freshly issued access token is nowhere near expiry and must be left as it is") + assert.Equal(t, community.PDSRefreshToken, unchanged.PDSRefreshToken) +} diff --git a/internal/core/communities/service_identifier_resolution_test.go b/internal/core/communities/service_identifier_resolution_test.go new file mode 100644 --- /dev/null +++ b/internal/core/communities/service_identifier_resolution_test.go @@ -0,0 +1,463 @@ +//go:build integration + +package communities_test + +import ( + "Coves/internal/core/communities" + "Coves/tests/testkit" + "context" + "errors" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// How a community may be addressed, and what happens when it is addressed +// badly. +// +// ResolveCommunityIdentifier and GetCommunity are the front door of every +// community-scoped API: social.coves.community.get, subscribe, block, and every +// post or comment that names its community take a client-supplied string and +// come through here. Four syntaxes are accepted — a DID, an atProto handle, an +// @-prefixed handle, and the Coves-specific scoped form !name@instance — and +// each of them normalises differently before it reaches a database lookup. +// +// # WHY THESE TESTS NEED INFRASTRUCTURE +// +// Resolution is not a string transformation with a lookup bolted on: three of +// the four forms END in a repository query, and the interesting failures are +// about which query gets made. A scoped identifier whose domain is not +// lowercased looks up a handle that cannot exist; an @-prefix left on the +// string does the same. Both are invisible to a test that stubs the repository, +// because a stub answers whatever key it is given. So the assertions are made +// against a real community row created through the real provisioning path, and +// the identifier under test is derived from what that row actually holds. +// +// The negative cases share the same service for the same reason: "rejected" has +// to mean "rejected before any lookup" or "rejected because the lookup found +// nothing", and only a live repository can tell those apart. + +// alternatingCase upper-cases every other character, producing the kind of +// domain a client types by hand rather than one any code would generate. +func alternatingCase(s string) string { + var b strings.Builder + for i, r := range s { + if i%2 == 0 { + b.WriteString(strings.ToUpper(string(r))) + } else { + b.WriteString(strings.ToLower(string(r))) + } + } + return b.String() +} + +// newResolvableCommunity provisions one real community and returns the service +// that owns it, so the identifiers under test can be built from the row rather +// than from a guess at the naming convention. +// +// HostedByDID is deliberately not set on the request: CreateCommunity overwrites +// it from the instance configuration precisely so a client cannot claim someone +// else's instance hosts a community, and passing one here would suggest the +// field is an input. +func newResolvableCommunity(t *testing.T, prefix string) (communities.Service, *communities.Community) { + t.Helper() + + service, _, _ := newCommunityService(t) + + name := testkit.UniqueIDWithPrefix(t, prefix) + require.LessOrEqualf(t, len("c-"+name), testkit.MaxIDLength, + "the generated community name %q makes a handle label the PDS will refuse", name) + + community, err := service.CreateCommunity(context.Background(), communities.CreateCommunityRequest{ + Name: name, + DisplayName: "Identifier Resolution", + Description: "a community addressed every way the resolver accepts", + Visibility: "public", + CreatedByDID: "did:plc:identifierresolution", + AllowExternalDiscovery: true, + }) + require.NoError(t, err) + return service, community +} + +func TestService_ResolveIdentifierAcceptsEveryAddressableForm(t *testing.T) { + t.Parallel() + + service, community := newResolvableCommunity(t, "res") + ctx := context.Background() + name := community.Name + + // Every one of these must land on the same DID. Case is the recurring theme: + // DNS is case-insensitive and users type community names the way they read + // them, so a resolver that lower-cases only some of the string produces a + // lookup key no row can match — a 404 on a community that plainly exists. + for _, testCase := range []struct { + name string + identifier string + }{ + {"DID", community.DID}, + {"canonical handle", community.Handle}, + {"canonical handle with an upper-cased domain", "c-" + name + "." + strings.ToUpper(instanceDomain)}, + {"at-identifier", "@" + community.Handle}, + {"at-identifier with an upper-cased domain", "@c-" + name + "." + strings.ToUpper(instanceDomain)}, + {"scoped identifier", "!" + name + "@" + instanceDomain}, + {"scoped identifier with an upper-cased domain", "!" + name + "@" + strings.ToUpper(instanceDomain)}, + {"scoped identifier with an alternating-case domain", "!" + name + "@" + alternatingCase(instanceDomain)}, + {"scoped identifier with an upper-cased name", "!" + strings.ToUpper(name) + "@" + instanceDomain}, + {"handle padded with whitespace", " " + community.Handle + " "}, + } { + t.Run(testCase.name, func(t *testing.T) { + did, err := service.ResolveCommunityIdentifier(ctx, testCase.identifier) + require.NoErrorf(t, err, "%q addresses a community that exists", testCase.identifier) + assert.Equal(t, community.DID, did) + }) + } +} + +// TestService_ResolveIdentifierRejectsMalformedScopedIdentifiers is the ONLY +// coverage in the repository for a malformed !name@instance string, and the +// reason it is worth saying so out loud is that the scoped form is the one +// syntax atProto does not define for us. +// +// A DID and a handle both arrive pre-validated by convention — clients copy +// them from records — but !gardening@coves.social is a Coves invention that +// users TYPE, so every one of these four shapes is something a real client will +// send. Each has a distinct consequence if it is not caught here: +// +// - no @ at all: SplitN would hand the whole string to the name, and the +// lookup would be for a handle built from a domain the user never named; +// - an empty name: the constructed handle is "c-.", which is a +// syntactically valid DNS name and therefore a lookup that could one day +// match something; +// - a foreign instance: the identifier is well-formed and refers to a +// community this AppView does not host, and silently rewriting it to a +// LOCAL handle would resolve one instance's community to another's; +// - a well-formed local name with nothing behind it: the only one of the four +// that is allowed to reach the database, and it must come back not-found +// rather than as a validation failure, because the two mean different +// things to the handler that maps them to HTTP status codes. +func TestService_ResolveIdentifierRejectsMalformedScopedIdentifiers(t *testing.T) { + t.Parallel() + + service, _, _ := newCommunityService(t) + ctx := context.Background() + + t.Run("rejects scoped identifier without @ symbol", func(t *testing.T) { + _, err := service.ResolveCommunityIdentifier(ctx, "!testcommunity") + require.Error(t, err) + assert.ErrorContains(t, err, "must include @ symbol") + assert.True(t, communities.IsValidationError(err), + "a syntax failure must classify as validation, not as a missing community") + }) + + t.Run("rejects scoped identifier with empty name", func(t *testing.T) { + _, err := service.ResolveCommunityIdentifier(ctx, "!@"+instanceDomain) + require.Error(t, err) + assert.ErrorContains(t, err, "community name cannot be empty") + assert.True(t, communities.IsValidationError(err), + "an empty name must be refused before it becomes the handle c-.%s", instanceDomain) + }) + + t.Run("rejects scoped identifier with wrong instance", func(t *testing.T) { + _, err := service.ResolveCommunityIdentifier(ctx, "!testcommunity@wrong.social") + require.Error(t, err) + assert.ErrorContains(t, err, "not hosted on this instance") + assert.ErrorContains(t, err, instanceDomain, + "the error must name the instance that WOULD have been accepted, or the client cannot correct it") + assert.True(t, communities.IsValidationError(err)) + }) + + t.Run("rejects non-existent community in scoped format", func(t *testing.T) { + _, err := service.ResolveCommunityIdentifier(ctx, "!nonexistent@"+instanceDomain) + require.Error(t, err) + assert.ErrorContains(t, err, "community not found") + assert.True(t, communities.IsNotFound(err), + "a well-formed identifier for a community that does not exist is a 404, not a 400") + }) +} + +// TestService_ResolveScopedIdentifierRejectsNamesThatAreNotDNSLabels covers the +// other half of scoped-identifier parsing: the name and the domain both become +// part of a handle, so anything that is not a legal DNS label has to be refused +// before it is concatenated into one. +// +// The characters in the table are not hypothetical. The name is interpolated +// into "c-%s.%s" and the result is used as a database lookup key and echoed +// back in error messages, so the validation here is what keeps a caller from +// steering either with punctuation. +func TestService_ResolveScopedIdentifierRejectsNamesThatAreNotDNSLabels(t *testing.T) { + t.Parallel() + + service, _, _ := newCommunityService(t) + ctx := context.Background() + + for _, testCase := range []struct { + name string + identifier string + message string + }{ + {"rejects special characters in name", "!", - "../../../etc/passwd", - } - - for _, injection := range sqlInjections { - t.Run(injection, func(t *testing.T) { - payload := map[string]interface{}{ - "community": "did:plc:test123", - "content": injection, - } - - body, _ := json.Marshal(payload) - req := httptest.NewRequest(http.MethodPost, "/xrpc/social.coves.community.post.create", bytes.NewReader(body)) - - ctx := middleware.SetTestUserDID(req.Context(), "did:plc:alice") - req = req.WithContext(ctx) - - rec := httptest.NewRecorder() - handler.HandleCreate(rec, req) - - // Handler should NOT crash or return 500 - // These are just strings, should be handled safely - assert.NotEqual(t, http.StatusInternalServerError, rec.Code, - "Handler should not crash on injection attempt") - }) - } - }) -} - -// TestPostService_DIDValidationSecurity tests service-layer DID validation (defense-in-depth) -func TestPostService_DIDValidationSecurity(t *testing.T) { - t.Parallel() - db := testkit.DB(t) - - // Setup services - communityRepo := postgres.NewCommunityRepository(db) - communityService := communities.NewCommunityServiceWithPDSFactory( - communityRepo, - "http://localhost:3001", - "did:web:test.coves.social", - "test.coves.social", - nil, - nil, - nil, - ) - - postRepo := postgres.NewPostRepository(db) - postService := posts.NewPostService(postRepo, communityService, nil, nil, nil, nil, "http://localhost:3001") - - t.Run("Reject posts when context DID is missing", func(t *testing.T) { - // Simulate bypassing handler - no DID in context - req := httptest.NewRequest(http.MethodPost, "/", nil) - ctx := middleware.SetTestUserDID(req.Context(), "") // Empty DID - - content := "Test post" - postReq := posts.CreatePostRequest{ - Community: "did:plc:test123", - AuthorDID: "did:plc:alice", - Content: &content, - } - - _, err := postService.CreatePost(ctx, postReq) - - // Should fail with authentication error - assert.Error(t, err) - assert.Contains(t, strings.ToLower(err.Error()), "authenticated") - }) - - t.Run("Reject posts when request DID doesn't match context DID", func(t *testing.T) { - // SECURITY TEST: This prevents DID spoofing attacks - // Simulates attack where handler is bypassed or compromised - req := httptest.NewRequest(http.MethodPost, "/", nil) - ctx := middleware.SetTestUserDID(req.Context(), "did:plc:alice") // Authenticated as Alice - - content := "Spoofed post" - postReq := posts.CreatePostRequest{ - Community: "did:plc:test123", - AuthorDID: "did:plc:bob", // ❌ Trying to post as Bob! - Content: &content, - } - - _, err := postService.CreatePost(ctx, postReq) - - // Should fail with DID mismatch error - assert.Error(t, err) - assert.Contains(t, strings.ToLower(err.Error()), "does not match") - }) - - t.Run("Accept posts when request DID matches context DID", func(t *testing.T) { - req := httptest.NewRequest(http.MethodPost, "/", nil) - ctx := middleware.SetTestUserDID(req.Context(), "did:plc:alice") // Authenticated as Alice - - content := "Valid post" - postReq := posts.CreatePostRequest{ - Community: "did:plc:test123", - AuthorDID: "did:plc:alice", // ✓ Matching DID - Content: &content, - } - - _, err := postService.CreatePost(ctx, postReq) - // May fail for other reasons (community not found), but NOT due to DID mismatch - if err != nil { - assert.NotContains(t, strings.ToLower(err.Error()), "does not match", - "Should not fail due to DID mismatch when DIDs match") - } - }) -} diff --git a/tests/integration/post_thumb_validation_test.go b/tests/integration/post_thumb_validation_test.go deleted file mode 100644 --- a/tests/integration/post_thumb_validation_test.go +++ /dev/null @@ -1,352 +0,0 @@ -//go:build integration - -package integration - -import ( - "Coves/internal/api/handlers/post" - "Coves/internal/api/middleware" - "Coves/internal/core/communities" - "Coves/internal/core/posts" - "Coves/internal/db/postgres" - "Coves/tests/testkit" - "bytes" - "context" - "encoding/json" - "net/http" - "net/http/httptest" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// createTestCommunityWithCredentials creates a test community with valid PDS credentials -func createTestCommunityWithCredentials(t *testing.T, repo communities.Repository, suffix string) *communities.Community { - t.Helper() - - community := &communities.Community{ - DID: "did:plc:testcommunity" + suffix, - Name: "test-community-" + suffix, - Handle: "test-community-" + suffix + ".communities.coves.local", - Description: "Test community for thumb validation", - Visibility: "public", - PDSEmail: "test@communities.coves.local", - PDSPassword: "test-password", - PDSAccessToken: "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJkaWQ6cGxjOnRlc3Rjb21tdW5pdHkxMjMiLCJleHAiOjk5OTk5OTk5OTl9.test", - PDSRefreshToken: "refresh_token_test123", - PDSURL: "http://localhost:3001", - } - - created, err := repo.Create(context.Background(), community) - require.NoError(t, err) - - return created -} - -// TestPostHandler_ThumbValidation tests strict validation of thumb field in external embeds -func TestPostHandler_ThumbValidation(t *testing.T) { - t.Parallel() - db := testkit.DB(t) - - // Setup services - communityRepo := postgres.NewCommunityRepository(db) - communityService := communities.NewCommunityServiceWithPDSFactory( - communityRepo, - "http://localhost:3001", - "did:web:test.coves.social", - "test.coves.social", - nil, - nil, - nil, - ) - - postRepo := postgres.NewPostRepository(db) - // No blobService or unfurlService for these validation tests - postService := posts.NewPostService(postRepo, communityService, nil, nil, nil, nil, "http://localhost:3001") - - handler := post.NewCreateHandler(postService) - - userDID := "did:plc:thumbtest" + t.Name() - - // Create test user and community with PDS credentials (use unique IDs) - testUser := createTestUser(t, db, "thumbtest.bsky.social", userDID) - testCommunity := createTestCommunityWithCredentials(t, communityRepo, t.Name()) - - t.Run("Reject thumb as URL string", func(t *testing.T) { - payload := map[string]interface{}{ - "community": testCommunity.DID, - "title": "Test Post", - "content": "Test content", - "embed": map[string]interface{}{ - "$type": "social.coves.embed.external", - "external": map[string]interface{}{ - "uri": "https://streamable.com/test", - "thumb": "https://example.com/thumb.jpg", // ❌ URL string (invalid) - }, - }, - } - - body, _ := json.Marshal(payload) - req := httptest.NewRequest(http.MethodPost, "/xrpc/social.coves.community.post.create", bytes.NewReader(body)) - - // Mock authenticated user context - ctx := middleware.SetTestUserDID(req.Context(), testUser.DID) - req = req.WithContext(ctx) - - rec := httptest.NewRecorder() - handler.HandleCreate(rec, req) - - // Should return 400 Bad Request - assert.Equal(t, http.StatusBadRequest, rec.Code) - - var errResp map[string]interface{} - err := json.Unmarshal(rec.Body.Bytes(), &errResp) - require.NoError(t, err) - - assert.Contains(t, errResp["message"], "thumb must be a blob reference") - assert.Contains(t, errResp["message"], "not URL string") - }) - - t.Run("Reject thumb missing $type", func(t *testing.T) { - payload := map[string]interface{}{ - "community": testCommunity.DID, - "title": "Test Post", - "embed": map[string]interface{}{ - "$type": "social.coves.embed.external", - "external": map[string]interface{}{ - "uri": "https://streamable.com/test", - "thumb": map[string]interface{}{ // ❌ Missing $type - "ref": map[string]interface{}{"$link": "bafyrei123"}, - "mimeType": "image/jpeg", - "size": 12345, - }, - }, - }, - } - - body, _ := json.Marshal(payload) - req := httptest.NewRequest(http.MethodPost, "/xrpc/social.coves.community.post.create", bytes.NewReader(body)) - - ctx := middleware.SetTestUserDID(req.Context(), testUser.DID) - req = req.WithContext(ctx) - - rec := httptest.NewRecorder() - handler.HandleCreate(rec, req) - - assert.Equal(t, http.StatusBadRequest, rec.Code) - - var errResp map[string]interface{} - err := json.Unmarshal(rec.Body.Bytes(), &errResp) - require.NoError(t, err) - - assert.Contains(t, errResp["message"], "thumb must have $type: blob") - }) - - t.Run("Reject thumb missing ref field", func(t *testing.T) { - payload := map[string]interface{}{ - "community": testCommunity.DID, - "title": "Test Post", - "embed": map[string]interface{}{ - "$type": "social.coves.embed.external", - "external": map[string]interface{}{ - "uri": "https://streamable.com/test", - "thumb": map[string]interface{}{ - "$type": "blob", - // ❌ Missing ref field - "mimeType": "image/jpeg", - "size": 12345, - }, - }, - }, - } - - body, _ := json.Marshal(payload) - req := httptest.NewRequest(http.MethodPost, "/xrpc/social.coves.community.post.create", bytes.NewReader(body)) - - ctx := middleware.SetTestUserDID(req.Context(), testUser.DID) - req = req.WithContext(ctx) - - rec := httptest.NewRecorder() - handler.HandleCreate(rec, req) - - assert.Equal(t, http.StatusBadRequest, rec.Code) - - var errResp map[string]interface{} - err := json.Unmarshal(rec.Body.Bytes(), &errResp) - require.NoError(t, err) - - assert.Contains(t, errResp["message"], "thumb blob missing required 'ref' field") - }) - - t.Run("Reject thumb missing mimeType field", func(t *testing.T) { - payload := map[string]interface{}{ - "community": testCommunity.DID, - "title": "Test Post", - "embed": map[string]interface{}{ - "$type": "social.coves.embed.external", - "external": map[string]interface{}{ - "uri": "https://streamable.com/test", - "thumb": map[string]interface{}{ - "$type": "blob", - "ref": map[string]interface{}{"$link": "bafyrei123"}, - // ❌ Missing mimeType field - "size": 12345, - }, - }, - }, - } - - body, _ := json.Marshal(payload) - req := httptest.NewRequest(http.MethodPost, "/xrpc/social.coves.community.post.create", bytes.NewReader(body)) - - ctx := middleware.SetTestUserDID(req.Context(), testUser.DID) - req = req.WithContext(ctx) - - rec := httptest.NewRecorder() - handler.HandleCreate(rec, req) - - assert.Equal(t, http.StatusBadRequest, rec.Code) - - var errResp map[string]interface{} - err := json.Unmarshal(rec.Body.Bytes(), &errResp) - require.NoError(t, err) - - assert.Contains(t, errResp["message"], "thumb blob missing required 'mimeType' field") - }) - - t.Run("Accept valid blob reference", func(t *testing.T) { - // Note: This test will fail at PDS write because the blob doesn't actually exist - // But it validates that our thumb validation accepts properly formatted blobs - payload := map[string]interface{}{ - "community": testCommunity.DID, - "title": "Test Post", - "embed": map[string]interface{}{ - "$type": "social.coves.embed.external", - "external": map[string]interface{}{ - "uri": "https://streamable.com/test", - "thumb": map[string]interface{}{ // ✅ Valid blob - "$type": "blob", - "ref": map[string]interface{}{"$link": "bafyreib6tbnql2ux3whnfysbzabthaj2vvck53nimhbi5g5a7jgvgr5eqm"}, - "mimeType": "image/jpeg", - "size": 52813, - }, - }, - }, - } - - body, _ := json.Marshal(payload) - req := httptest.NewRequest(http.MethodPost, "/xrpc/social.coves.community.post.create", bytes.NewReader(body)) - - ctx := middleware.SetTestUserDID(req.Context(), testUser.DID) - req = req.WithContext(ctx) - - rec := httptest.NewRecorder() - handler.HandleCreate(rec, req) - - // Should not fail with thumb validation error - // (May fail later at PDS write, but that's expected for test data) - if rec.Code == http.StatusBadRequest { - var errResp map[string]interface{} - _ = json.Unmarshal(rec.Body.Bytes(), &errResp) - // If it's a bad request, it should NOT be about thumb validation - assert.NotContains(t, errResp["message"], "thumb must be") - assert.NotContains(t, errResp["message"], "thumb blob missing") - } - }) - - t.Run("Accept missing thumb (unfurl will handle)", func(t *testing.T) { - payload := map[string]interface{}{ - "community": testCommunity.DID, - "title": "Test Post", - "embed": map[string]interface{}{ - "$type": "social.coves.embed.external", - "external": map[string]interface{}{ - "uri": "https://streamable.com/test", - // ✅ No thumb field - unfurl service will handle - }, - }, - } - - body, _ := json.Marshal(payload) - req := httptest.NewRequest(http.MethodPost, "/xrpc/social.coves.community.post.create", bytes.NewReader(body)) - - ctx := middleware.SetTestUserDID(req.Context(), testUser.DID) - req = req.WithContext(ctx) - - rec := httptest.NewRecorder() - handler.HandleCreate(rec, req) - - // Should not fail with thumb validation error - if rec.Code == http.StatusBadRequest { - var errResp map[string]interface{} - _ = json.Unmarshal(rec.Body.Bytes(), &errResp) - // Should not be a thumb validation error - assert.NotContains(t, errResp["message"], "thumb must be") - } - }) -} - -// TestPostHandler_EmbedValidation proves the embed-union validation is actually -// wired into the real create path (handler -> service -> validateEmbed), not -// just unit-tested in isolation. If the validateEmbed call site is ever removed -// or reordered after an early return, these cases fail even though the unit -// tests still pass — guarding against regression of the silent-corruption bug -// the validation exists to prevent. -func TestPostHandler_EmbedValidation(t *testing.T) { - t.Parallel() - db := testkit.DB(t) - - communityRepo := postgres.NewCommunityRepository(db) - communityService := communities.NewCommunityServiceWithPDSFactory( - communityRepo, - "http://localhost:3001", - "did:web:test.coves.social", - "test.coves.social", - nil, - nil, - nil, - ) - postRepo := postgres.NewPostRepository(db) - postService := posts.NewPostService(postRepo, communityService, nil, nil, nil, nil, "http://localhost:3001") - handler := post.NewCreateHandler(postService) - - userDID := "did:plc:embedtest" + t.Name() - - testUser := createTestUser(t, db, "embedtest.bsky.social", userDID) - testCommunity := createTestCommunityWithCredentials(t, communityRepo, t.Name()) - - postEmbed := func(t *testing.T, embed interface{}) map[string]interface{} { - t.Helper() - payload := map[string]interface{}{ - "community": testCommunity.DID, - "title": "Test Post", - "embed": embed, - } - body, _ := json.Marshal(payload) - req := httptest.NewRequest(http.MethodPost, "/xrpc/social.coves.community.post.create", bytes.NewReader(body)) - req = req.WithContext(middleware.SetTestUserDID(req.Context(), testUser.DID)) - - rec := httptest.NewRecorder() - handler.HandleCreate(rec, req) - - assert.Equal(t, http.StatusBadRequest, rec.Code) - var errResp map[string]interface{} - require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &errResp)) - return errResp - } - - t.Run("Reject embed missing $type (the bare-{uri} link-post bug)", func(t *testing.T) { - // The exact malformed shape the frontend was sending: a bare {uri} with - // no $type discriminator and no external wrapper. - errResp := postEmbed(t, map[string]interface{}{"uri": "https://example.com"}) - assert.Contains(t, errResp["message"], "$type") - }) - - t.Run("Reject embed with unknown $type", func(t *testing.T) { - errResp := postEmbed(t, map[string]interface{}{ - "$type": "social.coves.embed.externl", // typo - "external": map[string]interface{}{"uri": "https://example.com"}, - }) - assert.Contains(t, errResp["message"], "unknown embed") - }) -} diff --git a/tests/integration/post_unfurl_test.go b/internal/core/unfurl/post_unfurl_integration_test.go rename from tests/integration/post_unfurl_test.go rename to internal/core/unfurl/post_unfurl_integration_test.go --- a/tests/integration/post_unfurl_test.go +++ b/internal/core/unfurl/post_unfurl_integration_test.go @@ -1,6 +1,6 @@ //go:build integration -package integration +package unfurl_test import ( "Coves/internal/api/middleware" @@ -11,6 +11,7 @@ "Coves/internal/core/posts" "Coves/internal/core/unfurl" "Coves/internal/core/users" "Coves/internal/db/postgres" + "Coves/tests/fixtures" "Coves/tests/testkit" "context" "encoding/json" @@ -39,11 +40,11 @@ postRepo := postgres.NewPostRepository(db) identityConfig := identity.DefaultConfig() identityResolver := identity.NewResolver(db, identityConfig) - userService := users.NewUserService(userRepo, identityResolver, "http://localhost:3001", nil, "") + userService := users.NewUserService(userRepo, identityResolver, testkit.Endpoints().PDS.BaseURL, nil, "") communityService := communities.NewCommunityServiceWithPDSFactory( communityRepo, - "http://localhost:3001", + testkit.Endpoints().PDS.BaseURL, "did:web:test.coves.social", "test.coves.social", nil, @@ -59,28 +60,28 @@ nil, // aggregatorService nil, // blobService nil, // unfurlService - intentionally nil to test graceful handling nil, // blueskyService - "http://localhost:3001", + testkit.Endpoints().PDS.BaseURL, ) // Create test user - testUserDID := generateTestDID("unsupporteduser") + testUserDID := fixtures.DID("unsupporteduser") _, err := userService.CreateUser(ctx, users.CreateUserRequest{ DID: testUserDID, Handle: "unsupporteduser.test", - PDSURL: "http://localhost:3001", + PDSURL: testkit.Endpoints().PDS.BaseURL, }) require.NoError(t, err) // Create test community testCommunity := &communities.Community{ - DID: generateTestDID("unsupportedcommunity"), + DID: fixtures.DID("unsupportedcommunity"), Handle: "c-unsupportedcommunity.test.coves.social", Name: "unsupportedcommunity", DisplayName: "Unsupported URL Test", Visibility: "public", CreatedByDID: testUserDID, HostedByDID: "did:web:test.coves.social", - PDSURL: "http://localhost:3001", + PDSURL: testkit.Endpoints().PDS.BaseURL, PDSAccessToken: "fake_token", PDSRefreshToken: "fake_refresh", } @@ -131,7 +132,7 @@ unfurlRepo := unfurl.NewRepository(db) identityConfig := identity.DefaultConfig() identityResolver := identity.NewResolver(db, identityConfig) - userService := users.NewUserService(userRepo, identityResolver, "http://localhost:3001", nil, "") + userService := users.NewUserService(userRepo, identityResolver, testkit.Endpoints().PDS.BaseURL, nil, "") unfurlService := unfurl.NewService(unfurlRepo, unfurl.WithTimeout(30*time.Second), @@ -139,7 +140,7 @@ ) communityService := communities.NewCommunityServiceWithPDSFactory( communityRepo, - "http://localhost:3001", + testkit.Endpoints().PDS.BaseURL, "did:web:test.coves.social", "test.coves.social", nil, @@ -154,27 +155,27 @@ nil, nil, unfurlService, nil, // blueskyService - "http://localhost:3001", + testkit.Endpoints().PDS.BaseURL, ) // Create test user and community - testUserDID := generateTestDID("noembeduser") + testUserDID := fixtures.DID("noembeduser") _, err := userService.CreateUser(ctx, users.CreateUserRequest{ DID: testUserDID, Handle: "noembeduser.test", - PDSURL: "http://localhost:3001", + PDSURL: testkit.Endpoints().PDS.BaseURL, }) require.NoError(t, err) testCommunity := &communities.Community{ - DID: generateTestDID("noembedcommunity"), + DID: fixtures.DID("noembedcommunity"), Handle: "c-noembedcommunity.test.coves.social", Name: "noembedcommunity", DisplayName: "No Embed Test", Visibility: "public", CreatedByDID: testUserDID, HostedByDID: "did:web:test.coves.social", - PDSURL: "http://localhost:3001", + PDSURL: testkit.Endpoints().PDS.BaseURL, PDSAccessToken: "fake_token", PDSRefreshToken: "fake_refresh", } @@ -295,7 +296,7 @@ // Setup services identityConfig := identity.DefaultConfig() identityResolver := identity.NewResolver(db, identityConfig) - userService := users.NewUserService(userRepo, identityResolver, "http://localhost:3001", nil, "") + userService := users.NewUserService(userRepo, identityResolver, testkit.Endpoints().PDS.BaseURL, nil, "") unfurlService := unfurl.NewService(unfurlRepo, unfurl.WithTimeout(30*time.Second), @@ -319,10 +320,10 @@ defer unfurlTarget.Close() targetURL := unfurlTarget.URL + "/e2etest" // Create test data - testUserDID := generateTestDID("e2eunfurluser") - author := createTestUser(t, db, "e2eunfurluser.test", testUserDID) + testUserDID := fixtures.DID("e2eunfurluser") + author := fixtures.User(t, db, "e2eunfurluser.test", testUserDID) - testCommunityDID := generateTestDID("e2eunfurlcommunity") + testCommunityDID := fixtures.DID("e2eunfurlcommunity") community := &communities.Community{ DID: testCommunityDID, Handle: "c-e2eunfurlcommunity.test.coves.social", @@ -341,7 +342,7 @@ } _, err := communityRepo.Create(ctx, community) require.NoError(t, err) - rkey := generateTID() + rkey := testkit.TID() // Trigger the unfurl, as the post service would. No fallback: if this fails // the test fails, because everything below asserts on what it produced. diff --git a/tests/integration/timeline_test.go b/tests/integration/timeline_test.go deleted file mode 100644 --- a/tests/integration/timeline_test.go +++ /dev/null @@ -1,704 +0,0 @@ -//go:build integration - -package integration - -import ( - "Coves/internal/api/handlers/timeline" - "Coves/internal/api/middleware" - "Coves/internal/db/postgres" - "Coves/tests/testkit" - "context" - "encoding/json" - "fmt" - "net/http" - "net/http/httptest" - "testing" - "time" - - timelineCore "Coves/internal/core/timeline" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// TestGetTimeline_Basic tests timeline feed shows posts from subscribed communities -func TestGetTimeline_Basic(t *testing.T) { - t.Parallel() - db := testkit.DB(t) - - // Setup services - timelineRepo := postgres.NewTimelineRepository(db, "test-cursor-secret") - timelineService := timelineCore.NewTimelineService(timelineRepo) - handler := timeline.NewGetTimelineHandler(timelineService, nil, nil) - - ctx := context.Background() - testID := time.Now().UnixNano() - userDID := fmt.Sprintf("did:plc:user-%d", testID) - - // Create user - _, err := db.ExecContext(ctx, ` - INSERT INTO users (did, handle, pds_url) - VALUES ($1, $2, $3) - `, userDID, fmt.Sprintf("testuser-%d.test", testID), "https://bsky.social") - require.NoError(t, err) - - // Create two communities - community1DID, err := createFeedTestCommunity(db, ctx, fmt.Sprintf("gaming-%d", testID), fmt.Sprintf("alice-%d.test", testID)) - require.NoError(t, err) - - community2DID, err := createFeedTestCommunity(db, ctx, fmt.Sprintf("tech-%d", testID), fmt.Sprintf("bob-%d.test", testID)) - require.NoError(t, err) - - // Create a third community that user is NOT subscribed to - community3DID, err := createFeedTestCommunity(db, ctx, fmt.Sprintf("cooking-%d", testID), fmt.Sprintf("charlie-%d.test", testID)) - require.NoError(t, err) - - // Subscribe user to community1 and community2 (but not community3) - _, err = db.ExecContext(ctx, ` - INSERT INTO community_subscriptions (user_did, community_did, content_visibility) - VALUES ($1, $2, 3), ($1, $3, 3) - `, userDID, community1DID, community2DID) - require.NoError(t, err) - - // Create posts in all three communities - post1URI := createTestPost(t, db, community1DID, "did:plc:alice", "Gaming post 1", 50, time.Now().Add(-1*time.Hour)) - post2URI := createTestPost(t, db, community2DID, "did:plc:bob", "Tech post 1", 30, time.Now().Add(-2*time.Hour)) - post3URI := createTestPost(t, db, community3DID, "did:plc:charlie", "Cooking post (should not appear)", 100, time.Now().Add(-30*time.Minute)) - post4URI := createTestPost(t, db, community1DID, "did:plc:alice", "Gaming post 2", 20, time.Now().Add(-3*time.Hour)) - - // Request timeline with auth - req := httptest.NewRequest(http.MethodGet, "/xrpc/social.coves.feed.getTimeline?sort=new&limit=10", nil) - req = req.WithContext(middleware.SetTestUserDID(req.Context(), userDID)) - rec := httptest.NewRecorder() - handler.HandleGetTimeline(rec, req) - - // Assertions - assert.Equal(t, http.StatusOK, rec.Code) - - var response timelineCore.TimelineResponse - err = json.Unmarshal(rec.Body.Bytes(), &response) - require.NoError(t, err) - - // Should show 3 posts (from community1 and community2, NOT community3) - assert.Len(t, response.Feed, 3, "Timeline should show posts from subscribed communities only") - - // Verify correct posts are shown - uris := []string{response.Feed[0].Post.URI, response.Feed[1].Post.URI, response.Feed[2].Post.URI} - assert.Contains(t, uris, post1URI, "Should contain gaming post 1") - assert.Contains(t, uris, post2URI, "Should contain tech post 1") - assert.Contains(t, uris, post4URI, "Should contain gaming post 2") - assert.NotContains(t, uris, post3URI, "Should NOT contain post from unsubscribed community") - - // Verify posts are sorted by creation time (newest first for "new" sort) - assert.Equal(t, post1URI, response.Feed[0].Post.URI, "Newest post should be first") - assert.Equal(t, post2URI, response.Feed[1].Post.URI, "Second newest post") - assert.Equal(t, post4URI, response.Feed[2].Post.URI, "Oldest post should be last") - - // Verify Record field is populated (schema compliance) - for i, feedPost := range response.Feed { - assert.NotNil(t, feedPost.Post.Record, "Post %d should have Record field", i) - record, ok := feedPost.Post.Record.(map[string]interface{}) - require.True(t, ok, "Record should be a map") - assert.Equal(t, "social.coves.community.post", record["$type"], "Record should have correct $type") - assert.NotEmpty(t, record["community"], "Record should have community") - assert.NotEmpty(t, record["author"], "Record should have author") - assert.NotEmpty(t, record["createdAt"], "Record should have createdAt") - } -} - -// TestGetTimeline_HotSort tests hot sorting across multiple communities -func TestGetTimeline_HotSort(t *testing.T) { - t.Parallel() - db := testkit.DB(t) - - // Setup services - timelineRepo := postgres.NewTimelineRepository(db, "test-cursor-secret") - timelineService := timelineCore.NewTimelineService(timelineRepo) - handler := timeline.NewGetTimelineHandler(timelineService, nil, nil) - - ctx := context.Background() - testID := time.Now().UnixNano() - userDID := fmt.Sprintf("did:plc:user-%d", testID) - - // Create user - _, err := db.ExecContext(ctx, ` - INSERT INTO users (did, handle, pds_url) - VALUES ($1, $2, $3) - `, userDID, fmt.Sprintf("testuser-%d.test", testID), "https://bsky.social") - require.NoError(t, err) - - // Create communities - community1DID, err := createFeedTestCommunity(db, ctx, fmt.Sprintf("gaming-%d", testID), fmt.Sprintf("alice-%d.test", testID)) - require.NoError(t, err) - - community2DID, err := createFeedTestCommunity(db, ctx, fmt.Sprintf("tech-%d", testID), fmt.Sprintf("bob-%d.test", testID)) - require.NoError(t, err) - - // Subscribe to both - _, err = db.ExecContext(ctx, ` - INSERT INTO community_subscriptions (user_did, community_did, content_visibility) - VALUES ($1, $2, 3), ($1, $3, 3) - `, userDID, community1DID, community2DID) - require.NoError(t, err) - - // Create posts with different scores and ages - // Recent with medium score from gaming (should rank high) - createTestPost(t, db, community1DID, "did:plc:alice", "Recent trending gaming", 50, time.Now().Add(-1*time.Hour)) - - // Old with high score from tech (age penalty) - createTestPost(t, db, community2DID, "did:plc:bob", "Old popular tech", 100, time.Now().Add(-24*time.Hour)) - - // Very recent with low score from gaming - createTestPost(t, db, community1DID, "did:plc:charlie", "Brand new gaming", 5, time.Now().Add(-10*time.Minute)) - - // Request hot timeline - req := httptest.NewRequest(http.MethodGet, "/xrpc/social.coves.feed.getTimeline?sort=hot&limit=10", nil) - req = req.WithContext(middleware.SetTestUserDID(req.Context(), userDID)) - rec := httptest.NewRecorder() - handler.HandleGetTimeline(rec, req) - - // Assertions - assert.Equal(t, http.StatusOK, rec.Code) - - var response timelineCore.TimelineResponse - err = json.Unmarshal(rec.Body.Bytes(), &response) - require.NoError(t, err) - - assert.Len(t, response.Feed, 3, "Timeline should show all posts from subscribed communities") - - // All posts should have community context - for _, feedPost := range response.Feed { - assert.NotNil(t, feedPost.Post.Community, "Post should have community context") - assert.Contains(t, []string{community1DID, community2DID}, feedPost.Post.Community.DID) - } -} - -// TestGetTimeline_Pagination tests cursor-based pagination -func TestGetTimeline_Pagination(t *testing.T) { - t.Parallel() - db := testkit.DB(t) - - // Setup services - timelineRepo := postgres.NewTimelineRepository(db, "test-cursor-secret") - timelineService := timelineCore.NewTimelineService(timelineRepo) - handler := timeline.NewGetTimelineHandler(timelineService, nil, nil) - - ctx := context.Background() - testID := time.Now().UnixNano() - userDID := fmt.Sprintf("did:plc:user-%d", testID) - - // Create user - _, err := db.ExecContext(ctx, ` - INSERT INTO users (did, handle, pds_url) - VALUES ($1, $2, $3) - `, userDID, fmt.Sprintf("testuser-%d.test", testID), "https://bsky.social") - require.NoError(t, err) - - // Create community - communityDID, err := createFeedTestCommunity(db, ctx, fmt.Sprintf("gaming-%d", testID), fmt.Sprintf("alice-%d.test", testID)) - require.NoError(t, err) - - // Subscribe - _, err = db.ExecContext(ctx, ` - INSERT INTO community_subscriptions (user_did, community_did, content_visibility) - VALUES ($1, $2, 3) - `, userDID, communityDID) - require.NoError(t, err) - - // Create 5 posts - for i := 0; i < 5; i++ { - createTestPost(t, db, communityDID, "did:plc:alice", fmt.Sprintf("Post %d", i), 10-i, time.Now().Add(-time.Duration(i)*time.Hour)) - } - - // First page: limit 2 - req := httptest.NewRequest(http.MethodGet, "/xrpc/social.coves.feed.getTimeline?sort=new&limit=2", nil) - req = req.WithContext(middleware.SetTestUserDID(req.Context(), userDID)) - rec := httptest.NewRecorder() - handler.HandleGetTimeline(rec, req) - - assert.Equal(t, http.StatusOK, rec.Code) - - var page1 timelineCore.TimelineResponse - err = json.Unmarshal(rec.Body.Bytes(), &page1) - require.NoError(t, err) - - assert.Len(t, page1.Feed, 2, "First page should have 2 posts") - assert.NotNil(t, page1.Cursor, "Should have cursor for next page") - - // Second page: use cursor - req = httptest.NewRequest(http.MethodGet, fmt.Sprintf("/xrpc/social.coves.feed.getTimeline?sort=new&limit=2&cursor=%s", *page1.Cursor), nil) - req = req.WithContext(middleware.SetTestUserDID(req.Context(), userDID)) - rec = httptest.NewRecorder() - handler.HandleGetTimeline(rec, req) - - assert.Equal(t, http.StatusOK, rec.Code) - - var page2 timelineCore.TimelineResponse - err = json.Unmarshal(rec.Body.Bytes(), &page2) - require.NoError(t, err) - - assert.Len(t, page2.Feed, 2, "Second page should have 2 posts") - assert.NotNil(t, page2.Cursor, "Should have cursor for next page") - - // Verify no overlap - assert.NotEqual(t, page1.Feed[0].Post.URI, page2.Feed[0].Post.URI, "Pages should not overlap") - assert.NotEqual(t, page1.Feed[1].Post.URI, page2.Feed[1].Post.URI, "Pages should not overlap") -} - -// TestGetTimeline_EmptyWhenNoSubscriptions tests timeline is empty when user has no subscriptions -func TestGetTimeline_EmptyWhenNoSubscriptions(t *testing.T) { - t.Parallel() - db := testkit.DB(t) - - // Setup services - timelineRepo := postgres.NewTimelineRepository(db, "test-cursor-secret") - timelineService := timelineCore.NewTimelineService(timelineRepo) - handler := timeline.NewGetTimelineHandler(timelineService, nil, nil) - - ctx := context.Background() - testID := time.Now().UnixNano() - userDID := fmt.Sprintf("did:plc:user-%d", testID) - - // Create user (but don't subscribe to any communities) - _, err := db.ExecContext(ctx, ` - INSERT INTO users (did, handle, pds_url) - VALUES ($1, $2, $3) - `, userDID, fmt.Sprintf("testuser-%d.test", testID), "https://bsky.social") - require.NoError(t, err) - - // Request timeline - req := httptest.NewRequest(http.MethodGet, "/xrpc/social.coves.feed.getTimeline?sort=new&limit=10", nil) - req = req.WithContext(middleware.SetTestUserDID(req.Context(), userDID)) - rec := httptest.NewRecorder() - handler.HandleGetTimeline(rec, req) - - // Assertions - assert.Equal(t, http.StatusOK, rec.Code) - - var response timelineCore.TimelineResponse - err = json.Unmarshal(rec.Body.Bytes(), &response) - require.NoError(t, err) - - assert.Empty(t, response.Feed, "Timeline should be empty when user has no subscriptions") - assert.Nil(t, response.Cursor, "Should not have cursor when no results") -} - -// TestGetTimeline_Unauthorized tests timeline requires authentication -func TestGetTimeline_Unauthorized(t *testing.T) { - t.Parallel() - db := testkit.DB(t) - - // Setup services - timelineRepo := postgres.NewTimelineRepository(db, "test-cursor-secret") - timelineService := timelineCore.NewTimelineService(timelineRepo) - handler := timeline.NewGetTimelineHandler(timelineService, nil, nil) - - // Request timeline WITHOUT auth context - req := httptest.NewRequest(http.MethodGet, "/xrpc/social.coves.feed.getTimeline?sort=new&limit=10", nil) - rec := httptest.NewRecorder() - handler.HandleGetTimeline(rec, req) - - // Should return 401 Unauthorized - assert.Equal(t, http.StatusUnauthorized, rec.Code) - - var errorResp map[string]string - err := json.Unmarshal(rec.Body.Bytes(), &errorResp) - require.NoError(t, err) - - assert.Equal(t, "AuthenticationRequired", errorResp["error"]) -} - -// TestGetTimeline_LimitValidation tests limit parameter validation -func TestGetTimeline_LimitValidation(t *testing.T) { - t.Parallel() - db := testkit.DB(t) - - // Setup services - timelineRepo := postgres.NewTimelineRepository(db, "test-cursor-secret") - timelineService := timelineCore.NewTimelineService(timelineRepo) - handler := timeline.NewGetTimelineHandler(timelineService, nil, nil) - - ctx := context.Background() - testID := time.Now().UnixNano() - userDID := fmt.Sprintf("did:plc:user-%d", testID) - - // Create user - _, err := db.ExecContext(ctx, ` - INSERT INTO users (did, handle, pds_url) - VALUES ($1, $2, $3) - `, userDID, fmt.Sprintf("testuser-%d.test", testID), "https://bsky.social") - require.NoError(t, err) - - t.Run("Limit exceeds maximum", func(t *testing.T) { - req := httptest.NewRequest(http.MethodGet, "/xrpc/social.coves.feed.getTimeline?sort=new&limit=100", nil) - req = req.WithContext(middleware.SetTestUserDID(req.Context(), userDID)) - rec := httptest.NewRecorder() - handler.HandleGetTimeline(rec, req) - - assert.Equal(t, http.StatusBadRequest, rec.Code) - - var errorResp map[string]string - err := json.Unmarshal(rec.Body.Bytes(), &errorResp) - require.NoError(t, err) - - assert.Equal(t, "InvalidRequest", errorResp["error"]) - assert.Contains(t, errorResp["message"], "limit") - }) -} - -// TestGetTimeline_MultiCommunity_E2E tests the complete multi-community timeline flow -// This is the comprehensive E2E test specified in PRD_ALPHA_GO_LIVE.md (lines 236-246) -// -// Test Coverage: -// - Creates 3+ communities with different posts -// - Subscribes user to all communities -// - Creates posts with varied ages and scores across communities -// - Verifies timeline shows posts from ALL subscribed communities -// - Tests all sorting modes (hot, top, new) across communities -// - Ensures proper aggregation and no cross-contamination -func TestGetTimeline_MultiCommunity_E2E(t *testing.T) { - t.Parallel() - db := testkit.DB(t) - - // Setup services - timelineRepo := postgres.NewTimelineRepository(db, "test-cursor-secret") - timelineService := timelineCore.NewTimelineService(timelineRepo) - handler := timeline.NewGetTimelineHandler(timelineService, nil, nil) - - ctx := context.Background() - testID := time.Now().UnixNano() - userDID := fmt.Sprintf("did:plc:user-%d", testID) - - // Create test user - _, err := db.ExecContext(ctx, ` - INSERT INTO users (did, handle, pds_url) - VALUES ($1, $2, $3) - `, userDID, fmt.Sprintf("testuser-%d.test", testID), "https://bsky.social") - require.NoError(t, err) - - // Create 4 communities (user will subscribe to 3, not subscribe to 1) - community1DID, err := createFeedTestCommunity(db, ctx, fmt.Sprintf("gaming-%d", testID), fmt.Sprintf("alice-%d.test", testID)) - require.NoError(t, err, "Failed to create gaming community") - - community2DID, err := createFeedTestCommunity(db, ctx, fmt.Sprintf("tech-%d", testID), fmt.Sprintf("bob-%d.test", testID)) - require.NoError(t, err, "Failed to create tech community") - - community3DID, err := createFeedTestCommunity(db, ctx, fmt.Sprintf("music-%d", testID), fmt.Sprintf("charlie-%d.test", testID)) - require.NoError(t, err, "Failed to create music community") - - community4DID, err := createFeedTestCommunity(db, ctx, fmt.Sprintf("cooking-%d", testID), fmt.Sprintf("dave-%d.test", testID)) - require.NoError(t, err, "Failed to create cooking community (unsubscribed)") - - t.Logf("Created 4 communities: gaming=%s, tech=%s, music=%s, cooking=%s", - community1DID, community2DID, community3DID, community4DID) - - // Subscribe user to first 3 communities (NOT community4) - _, err = db.ExecContext(ctx, ` - INSERT INTO community_subscriptions (user_did, community_did, content_visibility) - VALUES ($1, $2, 3), ($1, $3, 3), ($1, $4, 3) - `, userDID, community1DID, community2DID, community3DID) - require.NoError(t, err, "Failed to create subscriptions") - - t.Log("✓ User subscribed to gaming, tech, and music communities") - - // Create posts across all 4 communities with varied ages and scores - // This tests that timeline correctly: - // 1. Aggregates posts from multiple subscribed communities - // 2. Excludes posts from unsubscribed communities - // 3. Handles different sorting algorithms across community boundaries - - // Gaming community posts (2 posts) - gamingPost1 := createTestPost(t, db, community1DID, "did:plc:gamer1", "Epic gaming moment", 100, time.Now().Add(-2*time.Hour)) - gamingPost2 := createTestPost(t, db, community1DID, "did:plc:gamer2", "New game release", 75, time.Now().Add(-30*time.Minute)) - - // Tech community posts (3 posts) - techPost1 := createTestPost(t, db, community2DID, "did:plc:dev1", "Golang best practices", 150, time.Now().Add(-4*time.Hour)) - techPost2 := createTestPost(t, db, community2DID, "did:plc:dev2", "atProto deep dive", 200, time.Now().Add(-1*time.Hour)) - techPost3 := createTestPost(t, db, community2DID, "did:plc:dev3", "Docker tips", 50, time.Now().Add(-15*time.Minute)) - - // Music community posts (2 posts) - musicPost1 := createTestPost(t, db, community3DID, "did:plc:artist1", "Album review", 80, time.Now().Add(-3*time.Hour)) - musicPost2 := createTestPost(t, db, community3DID, "did:plc:artist2", "Live concert tonight", 120, time.Now().Add(-10*time.Minute)) - - // Cooking community posts (should NOT appear - user not subscribed) - cookingPost := createTestPost(t, db, community4DID, "did:plc:chef1", "Best pizza recipe", 500, time.Now().Add(-5*time.Minute)) - - t.Logf("✓ Created 8 posts: 2 gaming, 3 tech, 2 music, 1 cooking (unsubscribed)") - - // Test 1: NEW sorting - chronological order across communities - t.Run("NEW sort - chronological across all subscribed communities", func(t *testing.T) { - req := httptest.NewRequest(http.MethodGet, "/xrpc/social.coves.feed.getTimeline?sort=new&limit=20", nil) - req = req.WithContext(middleware.SetTestUserDID(req.Context(), userDID)) - rec := httptest.NewRecorder() - handler.HandleGetTimeline(rec, req) - - assert.Equal(t, http.StatusOK, rec.Code) - - var response timelineCore.TimelineResponse - err := json.Unmarshal(rec.Body.Bytes(), &response) - require.NoError(t, err) - - // Should have exactly 7 posts (excluding cooking community) - assert.Len(t, response.Feed, 7, "Timeline should show 7 posts from 3 subscribed communities") - - // Verify chronological order (newest first) - expectedOrder := []string{ - musicPost2, // 10 minutes ago - techPost3, // 15 minutes ago - gamingPost2, // 30 minutes ago - techPost2, // 1 hour ago - gamingPost1, // 2 hours ago - musicPost1, // 3 hours ago - techPost1, // 4 hours ago - } - - for i, expectedURI := range expectedOrder { - assert.Equal(t, expectedURI, response.Feed[i].Post.URI, - "Post %d should be %s in chronological order", i, expectedURI) - } - - // Verify cooking post is NOT present - for _, feedPost := range response.Feed { - assert.NotEqual(t, cookingPost, feedPost.Post.URI, - "Cooking post from unsubscribed community should NOT appear") - } - - // Verify each post has community context from the correct community - communityCountsByDID := make(map[string]int) - for _, feedPost := range response.Feed { - require.NotNil(t, feedPost.Post.Community, "Post should have community context") - communityCountsByDID[feedPost.Post.Community.DID]++ - } - - assert.Equal(t, 2, communityCountsByDID[community1DID], "Should have 2 gaming posts") - assert.Equal(t, 3, communityCountsByDID[community2DID], "Should have 3 tech posts") - assert.Equal(t, 2, communityCountsByDID[community3DID], "Should have 2 music posts") - assert.Equal(t, 0, communityCountsByDID[community4DID], "Should have 0 cooking posts") - - t.Log("✓ NEW sort works correctly across multiple communities") - }) - - // Test 2: HOT sorting - balances recency and score across communities - t.Run("HOT sort - recency+score algorithm across communities", func(t *testing.T) { - req := httptest.NewRequest(http.MethodGet, "/xrpc/social.coves.feed.getTimeline?sort=hot&limit=20", nil) - req = req.WithContext(middleware.SetTestUserDID(req.Context(), userDID)) - rec := httptest.NewRecorder() - handler.HandleGetTimeline(rec, req) - - assert.Equal(t, http.StatusOK, rec.Code) - - var response timelineCore.TimelineResponse - err := json.Unmarshal(rec.Body.Bytes(), &response) - require.NoError(t, err) - - // Should still have exactly 7 posts - assert.Len(t, response.Feed, 7, "Timeline should show 7 posts from 3 subscribed communities") - - // Hot algorithm should rank recent high-scoring posts higher - // techPost2: 1 hour old, score 200 - should rank very high - // musicPost2: 10 minutes old, score 120 - should rank high (recent + good score) - // gamingPost1: 2 hours old, score 100 - should rank medium - // techPost1: 4 hours old, score 150 - age penalty - - // Verify top post is one of the high hot-rank posts - topPostURIs := []string{musicPost2, techPost2, gamingPost2} - assert.Contains(t, topPostURIs, response.Feed[0].Post.URI, - "Top post should be one of the recent high-scoring posts") - - // Verify all posts are from subscribed communities - for _, feedPost := range response.Feed { - assert.Contains(t, []string{community1DID, community2DID, community3DID}, - feedPost.Post.Community.DID, - "All posts should be from subscribed communities") - assert.NotEqual(t, cookingPost, feedPost.Post.URI, - "Cooking post should NOT appear") - } - - t.Log("✓ HOT sort works correctly across multiple communities") - }) - - // Test 3: TOP sorting with timeframe - highest scores across communities - t.Run("TOP sort - highest scores across all communities", func(t *testing.T) { - req := httptest.NewRequest(http.MethodGet, "/xrpc/social.coves.feed.getTimeline?sort=top&timeframe=all&limit=20", nil) - req = req.WithContext(middleware.SetTestUserDID(req.Context(), userDID)) - rec := httptest.NewRecorder() - handler.HandleGetTimeline(rec, req) - - assert.Equal(t, http.StatusOK, rec.Code) - - var response timelineCore.TimelineResponse - err := json.Unmarshal(rec.Body.Bytes(), &response) - require.NoError(t, err) - - // Should still have exactly 7 posts - assert.Len(t, response.Feed, 7, "Timeline should show 7 posts from 3 subscribed communities") - - // Verify top-ranked posts by score (highest first) - // techPost2: 200 score - // techPost1: 150 score - // musicPost2: 120 score - // gamingPost1: 100 score - // musicPost1: 80 score - // gamingPost2: 75 score - // techPost3: 50 score - - assert.Equal(t, techPost2, response.Feed[0].Post.URI, "Top post should be techPost2 (score 200)") - assert.Equal(t, techPost1, response.Feed[1].Post.URI, "Second post should be techPost1 (score 150)") - assert.Equal(t, musicPost2, response.Feed[2].Post.URI, "Third post should be musicPost2 (score 120)") - - // Verify scores are descending - for i := 0; i < len(response.Feed)-1; i++ { - currentScore := response.Feed[i].Post.Stats.Score - nextScore := response.Feed[i+1].Post.Stats.Score - assert.GreaterOrEqual(t, currentScore, nextScore, - "Scores should be in descending order (post %d score=%d, post %d score=%d)", - i, currentScore, i+1, nextScore) - } - - // Verify cooking post is NOT present (even though it has highest score) - for _, feedPost := range response.Feed { - assert.NotEqual(t, cookingPost, feedPost.Post.URI, - "Cooking post should NOT appear even with high score") - } - - t.Log("✓ TOP sort works correctly across multiple communities") - }) - - // Test 4: TOP with day timeframe - filters old posts - t.Run("TOP sort with day timeframe - filters across communities", func(t *testing.T) { - req := httptest.NewRequest(http.MethodGet, "/xrpc/social.coves.feed.getTimeline?sort=top&timeframe=day&limit=20", nil) - req = req.WithContext(middleware.SetTestUserDID(req.Context(), userDID)) - rec := httptest.NewRecorder() - handler.HandleGetTimeline(rec, req) - - assert.Equal(t, http.StatusOK, rec.Code) - - var response timelineCore.TimelineResponse - err := json.Unmarshal(rec.Body.Bytes(), &response) - require.NoError(t, err) - - // All our test posts are within the last day, so should have all 7 - assert.Len(t, response.Feed, 7, "All posts are within last day") - - // Verify all posts are within last 24 hours - dayAgo := time.Now().Add(-24 * time.Hour) - for _, feedPost := range response.Feed { - postTime := feedPost.Post.IndexedAt - assert.True(t, postTime.After(dayAgo), - "Post should be within last 24 hours") - } - - t.Log("✓ TOP sort with timeframe works correctly across multiple communities") - }) - - // Test 5: Pagination works across multiple communities - t.Run("Pagination across multiple communities", func(t *testing.T) { - // First page: limit 3 - req := httptest.NewRequest(http.MethodGet, "/xrpc/social.coves.feed.getTimeline?sort=new&limit=3", nil) - req = req.WithContext(middleware.SetTestUserDID(req.Context(), userDID)) - rec := httptest.NewRecorder() - handler.HandleGetTimeline(rec, req) - - assert.Equal(t, http.StatusOK, rec.Code) - - var page1 timelineCore.TimelineResponse - err := json.Unmarshal(rec.Body.Bytes(), &page1) - require.NoError(t, err) - - assert.Len(t, page1.Feed, 3, "First page should have 3 posts") - assert.NotNil(t, page1.Cursor, "Should have cursor for next page") - - // Second page - req = httptest.NewRequest(http.MethodGet, fmt.Sprintf("/xrpc/social.coves.feed.getTimeline?sort=new&limit=3&cursor=%s", *page1.Cursor), nil) - req = req.WithContext(middleware.SetTestUserDID(req.Context(), userDID)) - rec = httptest.NewRecorder() - handler.HandleGetTimeline(rec, req) - - assert.Equal(t, http.StatusOK, rec.Code) - - var page2 timelineCore.TimelineResponse - err = json.Unmarshal(rec.Body.Bytes(), &page2) - require.NoError(t, err) - - assert.Len(t, page2.Feed, 3, "Second page should have 3 posts") - assert.NotNil(t, page2.Cursor, "Should have cursor for third page") - - // Verify no overlap between pages - page1URIs := make(map[string]bool) - for _, p := range page1.Feed { - page1URIs[p.Post.URI] = true - } - for _, p := range page2.Feed { - assert.False(t, page1URIs[p.Post.URI], "Pages should not overlap") - } - - // Third page (remaining post) - req = httptest.NewRequest(http.MethodGet, fmt.Sprintf("/xrpc/social.coves.feed.getTimeline?sort=new&limit=3&cursor=%s", *page2.Cursor), nil) - req = req.WithContext(middleware.SetTestUserDID(req.Context(), userDID)) - rec = httptest.NewRecorder() - handler.HandleGetTimeline(rec, req) - - assert.Equal(t, http.StatusOK, rec.Code) - - var page3 timelineCore.TimelineResponse - err = json.Unmarshal(rec.Body.Bytes(), &page3) - require.NoError(t, err) - - assert.Len(t, page3.Feed, 1, "Third page should have 1 remaining post") - assert.Nil(t, page3.Cursor, "Should not have cursor on last page") - - t.Log("✓ Pagination works correctly across multiple communities") - }) - - // Test 6: Verify post record schema compliance across communities - t.Run("Record schema compliance across communities", func(t *testing.T) { - req := httptest.NewRequest(http.MethodGet, "/xrpc/social.coves.feed.getTimeline?sort=new&limit=20", nil) - req = req.WithContext(middleware.SetTestUserDID(req.Context(), userDID)) - rec := httptest.NewRecorder() - handler.HandleGetTimeline(rec, req) - - assert.Equal(t, http.StatusOK, rec.Code) - - var response timelineCore.TimelineResponse - err := json.Unmarshal(rec.Body.Bytes(), &response) - require.NoError(t, err) - - // Verify every post has proper Record structure - for i, feedPost := range response.Feed { - assert.NotNil(t, feedPost.Post.Record, "Post %d should have Record field", i) - - record, ok := feedPost.Post.Record.(map[string]interface{}) - require.True(t, ok, "Record should be a map") - - assert.Equal(t, "social.coves.community.post", record["$type"], - "Record should have correct $type") - assert.NotEmpty(t, record["community"], "Record should have community") - assert.NotEmpty(t, record["author"], "Record should have author") - assert.NotEmpty(t, record["createdAt"], "Record should have createdAt") - - // Verify community reference - assert.NotNil(t, feedPost.Post.Community, "Post should have community reference") - assert.NotEmpty(t, feedPost.Post.Community.DID, "Community should have DID") - assert.NotEmpty(t, feedPost.Post.Community.Handle, "Community should have handle") - assert.NotEmpty(t, feedPost.Post.Community.Name, "Community should have name") - - // Verify community DID matches one of our subscribed communities - assert.Contains(t, []string{community1DID, community2DID, community3DID}, - feedPost.Post.Community.DID, - "Post should be from one of the subscribed communities") - } - - t.Log("✓ All posts have proper record schema and community references") - }) - - t.Log("\n✅ Multi-Community Timeline E2E Test Complete!") - t.Log("Summary:") - t.Log(" ✓ Created 4 communities (3 subscribed, 1 unsubscribed)") - t.Log(" ✓ Created 8 posts across communities (7 in subscribed, 1 in unsubscribed)") - t.Log(" ✓ NEW sort: Chronological order across all subscribed communities") - t.Log(" ✓ HOT sort: Recency+score algorithm works across communities") - t.Log(" ✓ TOP sort: Highest scores across communities (with timeframe filtering)") - t.Log(" ✓ Pagination: Works correctly across community boundaries") - t.Log(" ✓ Schema: All posts have proper record structure and community refs") - t.Log(" ✓ Security: Unsubscribed community posts correctly excluded") -} diff --git a/tests/integration/token_refresh_test.go b/tests/integration/token_refresh_test.go deleted file mode 100644 --- a/tests/integration/token_refresh_test.go +++ /dev/null @@ -1,231 +0,0 @@ -//go:build integration - -package integration - -import ( - "Coves/internal/core/communities" - "Coves/internal/db/postgres" - "Coves/tests/testkit" - "context" - "encoding/base64" - "encoding/json" - "fmt" - "testing" - "time" -) - -// TestTokenRefresh_ExpirationDetection tests the NeedsRefresh function with various token states -func TestTokenRefresh_ExpirationDetection(t *testing.T) { - t.Parallel() - tests := []struct { - name string - token string - shouldRefresh bool - expectError bool - }{ - { - name: "Token expiring in 2 minutes (should refresh)", - token: createTestJWT(time.Now().Add(2 * time.Minute)), - shouldRefresh: true, - expectError: false, - }, - { - name: "Token expiring in 10 minutes (should not refresh)", - token: createTestJWT(time.Now().Add(10 * time.Minute)), - shouldRefresh: false, - expectError: false, - }, - { - name: "Token already expired (should refresh)", - token: createTestJWT(time.Now().Add(-1 * time.Minute)), - shouldRefresh: true, - expectError: false, - }, - { - name: "Token expiring in exactly 5 minutes (should not refresh - edge case)", - token: createTestJWT(time.Now().Add(6 * time.Minute)), - shouldRefresh: false, - expectError: false, - }, - { - name: "Token expiring in 4 minutes (should refresh)", - token: createTestJWT(time.Now().Add(4 * time.Minute)), - shouldRefresh: true, - expectError: false, - }, - { - name: "Invalid JWT format (too many parts)", - token: "not.a.valid.jwt.format.extra", - shouldRefresh: false, - expectError: true, - }, - { - name: "Invalid JWT format (too few parts)", - token: "invalid.token", - shouldRefresh: false, - expectError: true, - }, - { - name: "Empty token", - token: "", - shouldRefresh: false, - expectError: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result, err := communities.NeedsRefresh(tt.token) - - if tt.expectError { - if err == nil { - t.Errorf("Expected error but got none") - } - return - } - - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } - - if result != tt.shouldRefresh { - t.Errorf("Expected NeedsRefresh=%v, got %v", tt.shouldRefresh, result) - } - }) - } -} - -// TestTokenRefresh_UpdateCredentials tests the repository UpdateCredentials method -func TestTokenRefresh_UpdateCredentials(t *testing.T) { - t.Parallel() - ctx := context.Background() - db := testkit.DB(t) - - repo := postgres.NewCommunityRepository(db) - - // Create a test community first - community := &communities.Community{ - DID: "did:plc:test123", - Handle: "c-test.coves.social", - Name: "test", - OwnerDID: "did:plc:test123", - CreatedByDID: "did:plc:creator", - HostedByDID: "did:web:coves.social", - PDSEmail: "test@coves.social", - PDSPassword: "original-password", - PDSAccessToken: "original-access-token", - PDSRefreshToken: "original-refresh-token", - PDSURL: "http://localhost:3001", - Visibility: "public", - MemberCount: 0, - SubscriberCount: 0, - RecordURI: "at://did:plc:test123/social.coves.community.profile/self", - RecordCID: "bafytest", - } - - created, err := repo.Create(ctx, community) - if err != nil { - t.Fatalf("Failed to create test community: %v", err) - } - - // Update credentials - newAccessToken := "new-access-token-12345" - newRefreshToken := "new-refresh-token-67890" - - err = repo.UpdateCredentials(ctx, created.DID, newAccessToken, newRefreshToken) - if err != nil { - t.Fatalf("UpdateCredentials failed: %v", err) - } - - // Verify tokens were updated - retrieved, err := repo.GetByDID(ctx, created.DID) - if err != nil { - t.Fatalf("Failed to retrieve community: %v", err) - } - - if retrieved.PDSAccessToken != newAccessToken { - t.Errorf("Access token not updated: expected %q, got %q", newAccessToken, retrieved.PDSAccessToken) - } - - if retrieved.PDSRefreshToken != newRefreshToken { - t.Errorf("Refresh token not updated: expected %q, got %q", newRefreshToken, retrieved.PDSRefreshToken) - } - - // Verify password unchanged (should not be affected) - if retrieved.PDSPassword != "original-password" { - t.Errorf("Password should remain unchanged: expected %q, got %q", "original-password", retrieved.PDSPassword) - } -} - -// TestTokenRefresh_E2E_UpdateAfterTokenRefresh tests end-to-end token refresh during community update -func TestTokenRefresh_E2E_UpdateAfterTokenRefresh(t *testing.T) { - t.Parallel() - ctx := context.Background() - db := testkit.DB(t) - - // This test requires a real PDS for token refresh - // For now, we'll test the token expiration detection logic - // Full E2E test with PDS will be added in manual testing phase - - repo := postgres.NewCommunityRepository(db) - - // Create community with expiring token - expiringToken := createTestJWT(time.Now().Add(2 * time.Minute)) // Expires in 2 minutes - - community := &communities.Community{ - DID: "did:plc:expiring123", - Handle: "c-expiring.coves.social", - Name: "expiring", - OwnerDID: "did:plc:expiring123", - CreatedByDID: "did:plc:creator", - HostedByDID: "did:web:coves.social", - PDSEmail: "expiring@coves.social", - PDSPassword: "test-password", - PDSAccessToken: expiringToken, - PDSRefreshToken: "test-refresh-token", - PDSURL: "http://localhost:3001", - Visibility: "public", - RecordURI: "at://did:plc:expiring123/social.coves.community.profile/self", - RecordCID: "bafytest", - } - - created, err := repo.Create(ctx, community) - if err != nil { - t.Fatalf("Failed to create community: %v", err) - } - - // Verify token is stored - if created.PDSAccessToken != expiringToken { - t.Errorf("Token not stored correctly") - } - - t.Logf("✅ Created community with expiring token (expires in 2 minutes)") - t.Logf(" Community DID: %s", created.DID) - t.Logf(" NOTE: Full refresh flow requires real PDS - tested in manual/staging tests") -} - -// Helper: Create a test JWT with specific expiration time -func createTestJWT(expiresAt time.Time) string { - // Create JWT header - header := map[string]interface{}{ - "alg": "ES256", - "typ": "JWT", - } - headerJSON, _ := json.Marshal(header) - headerB64 := base64.RawURLEncoding.EncodeToString(headerJSON) - - // Create JWT payload with expiration - payload := map[string]interface{}{ - "sub": "did:plc:test", - "iss": "https://pds.example.com", - "exp": expiresAt.Unix(), - "iat": time.Now().Unix(), - } - payloadJSON, _ := json.Marshal(payload) - payloadB64 := base64.RawURLEncoding.EncodeToString(payloadJSON) - - // Fake signature (not verified in our tests) - signature := base64.RawURLEncoding.EncodeToString([]byte("fake-signature")) - - return fmt.Sprintf("%s.%s.%s", headerB64, payloadB64, signature) -} diff --git a/tests/integration/user_test.go b/internal/core/users/user_integration_test.go rename from tests/integration/user_test.go rename to internal/core/users/user_integration_test.go --- a/tests/integration/user_test.go +++ b/internal/core/users/user_integration_test.go @@ -1,6 +1,6 @@ //go:build integration -package integration +package users_test import ( "Coves/internal/api/routes" @@ -8,6 +8,7 @@ "Coves/internal/atproto/identity" "Coves/internal/atproto/pds" "Coves/internal/core/users" "Coves/internal/db/postgres" + "Coves/tests/fixtures" "Coves/tests/testkit" "context" "encoding/json" @@ -15,7 +16,6 @@ "errors" "fmt" "net/http" "net/http/httptest" - "os" "strings" "testing" "time" @@ -32,22 +32,6 @@ PDSClientFactory: func(ctx context.Context, session *oauth.ClientSessionData) (pds.Client, error) { return nil, errors.New("not implemented - test does not use updateProfile") }, } -} - -// TestMain controls test setup for the integration package. The whole package -// is integration-tagged, so its floor is the infrastructure every file here -// assumes: Postgres for testkit.DB clones, the PDS for record writes, and -// Jetstream for the consumer tests that read them back. -func TestMain(m *testing.M) { - os.Exit(testkit.Main(m, testkit.RequirePostgres, testkit.RequirePDS, testkit.RequireJetstream)) -} - -// generateTestDID generates a unique test DID for integration tests -// V2.0: No longer uses DID generator - just creates valid did:plc strings -func generateTestDID(suffix string) string { - // Use a deterministic base + suffix for reproducible test DIDs - // Format matches did:plc but doesn't need PLC registration for unit/repo tests - return fmt.Sprintf("did:plc:test%s", suffix) } func TestUserCreationAndRetrieval(t *testing.T) { @@ -138,7 +122,7 @@ } // Set up HTTP router with auth middleware r := chi.NewRouter() - authMiddleware, _ := CreateTestOAuthMiddleware("did:plc:testuser") + authMiddleware, _ := fixtures.SingleUserOAuthMiddleware("did:plc:testuser") routes.RegisterUserRoutesWithOptions(r, userService, authMiddleware, nil, testUserRouteOptions()) // Test 1: Get profile by DID @@ -751,7 +735,7 @@ }) t.Run("HTTP endpoint returns 404 for non-existent DID", func(t *testing.T) { r := chi.NewRouter() - authMiddleware, _ := CreateTestOAuthMiddleware("did:plc:testuser") + authMiddleware, _ := fixtures.SingleUserOAuthMiddleware("did:plc:testuser") routes.RegisterUserRoutesWithOptions(r, userService, authMiddleware, nil, testUserRouteOptions()) req := httptest.NewRequest("GET", "/xrpc/social.coves.actor.getProfile?actor=did:plc:nonexistentuser12345", nil) @@ -797,7 +781,7 @@ } // Set up HTTP router with auth middleware r := chi.NewRouter() - authMiddleware, _ := CreateTestOAuthMiddleware("did:plc:testuser") + authMiddleware, _ := fixtures.SingleUserOAuthMiddleware("did:plc:testuser") routes.RegisterUserRoutesWithOptions(r, userService, authMiddleware, nil, testUserRouteOptions()) t.Run("Response includes stats object", func(t *testing.T) { diff --git a/tests/testkit/db.go b/tests/testkit/db.go --- a/tests/testkit/db.go +++ b/tests/testkit/db.go @@ -667,12 +667,13 @@ return err } defer func() { _ = db.Close() }() - // goose.NewProvider rather than the package-level goose.Up: the package - // API keeps the dialect and base filesystem in global variables, and the - // legacy tests/integration path still calls goose.Up with a relative - // directory. Two callers with different notions of where migrations live, - // sharing one global, is a bug waiting for the first test binary that does - // both. + // goose.NewProvider rather than the package-level goose.Up: the package API + // keeps the dialect and base filesystem in global variables. No test calls + // goose.Up any more — phase 3 made testkit the only path to a database — but + // the provider form is what keeps that true by construction rather than by + // convention, because two callers with different notions of where migrations + // live, sharing one global, is a bug waiting for the first test binary that + // does both. provider, err := goose.NewProvider(goose.DialectPostgres, db, migrations.FS) if err != nil { return fmt.Errorf("configuring goose for template %q: %w", template, err) @@ -728,24 +729,34 @@ // fixed number of concurrent test databases, and these two flags only decide // how that total is split between the dimensions. // // It is 1, and the reason is NOT the one that used to be written on `-p 1` in -// the Makefile and the CI runner. That reason — tests/integration wiping shared -// tables — is gone: every test owns a private clone and packages cannot corrupt -// each other's data any more. +// the Makefile and the CI runner. That reason — a shared database being wiped +// out from under other packages — is gone: every test owns a private clone and +// packages cannot corrupt each other's data any more. // // The reason now is the ONE resource that stayed shared, and that no amount of // database isolation can partition: the Jetstream firehose. Running two test -// binaries at once means tests/testkit's firehose tests subscribe while -// tests/integration is creating PDS accounts 25 at a time — and Jetstream's +// binaries at once means testkit's firehose tests subscribe while another +// package is creating PDS accounts 25 at a time — and Jetstream's // wantedCollections filter does not apply to account/identity events, so those // tests get a stream that is almost entirely other tests' account churn and // time out waiting for their own commit. Measured, not assumed: on a dev stack // with an accumulated Jetstream store, the full tagged run failed 2 of 4 times // at -p 2 and 0 of 4 times at -p 1, with the same -parallel. // -// So the budget goes to within-package parallelism, which is where this tree's -// wall clock lives anyway — tests/integration alone is most of it. Raise this -// when Phase 4 of docs/TEST_ARCHITECTURE.md deletes tests/integration and its -// ten hand-rolled subscribers, leaving testkit the only firehose consumer. +// So the budget goes to within-package parallelism instead. +// +// AN EARLIER VERSION OF THIS COMMENT SAID TO RAISE THIS once phase 4 deleted +// tests/integration, "leaving testkit the only firehose consumer". Phase 4 has +// now done that, and the trigger was wrong: dissolving that directory relocated +// its account-creating tests into internal/… packages, it did not stop them +// creating accounts. testkit is indeed the only firehose SUBSCRIBER, but the +// contention was never with other subscribers — it is with account churn from +// whatever else is running, and there is exactly as much of that as before. +// +// The real precondition is upstream and still unmet: account and identity +// events bypass wantedCollections, so a subscriber cannot filter out other +// tests' signups. Until that is fixed, raising this needs the measurement +// repeating, not a directory disappearing. const packageParallelism = 1 // maxParallelPerPackage keeps a very large max_connections from producing a @@ -758,7 +769,8 @@ // parallel slot. // // One is the norm, but a test that calls testkit.DB and then calls it again in // a subtest holds two pools at once — the outer clone outlives the inner one. -// TestPasswordSecurity in tests/integration does exactly that today. Budgeting +// TestPasswordSecurity, now in internal/core/communities, does exactly that. +// Budgeting // one pool per slot made the model understate the true peak, so the formula // could hand out a -parallel whose worst case sat above the ceiling it had just // computed.