From a9744e14c5b4cd43d51a27a4db65f2fd69eb5d8f Mon Sep 17 00:00:00 2001 From: Bretton Date: Sat, 08 Aug 2026 12:05:31 +0000 Subject: [PATCH] test(posts): RED cycle 1 — the write-path flip's outer contract Task 6 RED. Tests, fixtures and rule-7 zero-logic stubs only; no behaviour is implemented. 25 reds, all inside the flip's blast radius. T0 (internal/core/posts, in-package) submission_rkey_test.go the deterministic postv2 rkey of PRD §4.2. Four golden TID vectors computed OUTSIDE Go (python3 transcription of the spec), each differing from the canonical one in exactly ONE input, so a derivation that drops an input reproduces the canonical value and the assertion names what went missing. Plus valid-TID parsing across seven input shapes, timestamp-inside-its-own-dedupe-bucket, call stability, set distinctness, and delimiter ambiguity. postv2_record_test.go PostV2Record's shape: no author field (decoded keys, not a substring search), required fields survive marshalling, optionals are absent rather than empty, and every lexicon surface is emitted — a field the struct lacks is a field an EDIT silently erases. T1 outer contract service_writeforward_test.go rewritten for the flipped repo. The five adjudicated outcomes: create -> author repo (and the community repo gains NO post record, asserted by listRecords over BOTH collections); delete flipped; DeleteRefusesEveryoneButTheAuthor survives with its rationale rewritten to local authorization; the unknown-community 404 dies, replaced by wrong-authority ErrNotAuthorized; the malformed table survives with its wrong-collection row respelled, and a new test proves the deprecated collection still routes to the old credential path. service_writeflip_test.go the journey: synchronous acceptance in a hosted community; a byte-identical retry returning the same URI, the same CID and NO new commit in EITHER repo; an unhosted community and an injected acceptance failure both leaving the post pending with the author's record intact; and the update journey, including the pin that an edit leaves the submission ledger completely alone. Stubs (zero-logic, no panics — reds read as failed assertions) postv2.go with PostV2Record/StrongRef/BridgedStats, SubmissionRkey, AuthorRepo + AuthorRepoFactory, SubmissionAcceptor and the two options; ErrNoAuthorCredentials + ErrConcurrentModification; UpdatePostRequest/ Response; CreatePostResponse.Status; AcceptSubmission on the engine; UpdatePost on the service. Three adjudications, flagged for review 1. CreatePost gained an explicit session parameter, matching DeletePost and the comments write path, rather than smuggling the credential through a context value a service can silently tolerate missing. This is what ripples into the handler, three mocks and six test files. 2. The seam is AuthorRepoFactory returning a narrowed AuthorRepo, mirroring this package's CommunityRepoFactory/CommunityRepo pair. The narrowing is load-bearing: the guarded put the write path needs is not on pds.Client. 3. AcceptSubmission must NOT re-run the decider — the production decider looks the post up in Postgres, and on the fast path it is not indexed yet. The fixture scripts the decider to REFUSE so a fast path that consults it fails loudly. Co-Authored-By: Claude Fable 5 --- internal/api/handlers/actor/get_posts_test.go | 6 +++++- internal/api/handlers/post/create.go | 12 +++++++++--- internal/api/handlers/post/create_security_test.go | 3 +++ internal/api/handlers/post/get_test.go | 6 +++++- internal/api/routes/post_aggregator_test.go | 7 ++++++- internal/core/posts/engine.go | 25 +++++++++++++++++++++++++ internal/core/posts/errors.go | 25 +++++++++++++++++++++++++ internal/core/posts/interfaces.go | 51 +++++++++++++++++++++++++++++++++++++++++++-------- internal/core/posts/post.go | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ internal/core/posts/postv2.go | 200 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ internal/core/posts/postv2_record_test.go | 164 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ internal/core/posts/service.go | 25 ++++++++++++++++++++++++- internal/core/posts/service_admission_test.go | 50 ++++++++++++++++++++++++++++---------------------- internal/core/posts/service_aggregator_test.go | 39 ++++++++++++++++++++++++++------------- internal/core/posts/service_create_validation_test.go | 32 +++++++++++++++++++------------- internal/core/posts/service_writeflip_test.go | 513 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ internal/core/posts/service_writeforward_test.go | 582 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------------------------------------------------------------------- internal/core/posts/submission_rkey_test.go | 273 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ internal/core/unfurl/post_unfurl_integration_test.go | 25 ++++++++++++++----------- tests/live/post_unfurl_test.go | 5 +++-- 20 file(s) changed, 1905 insertion(s)(+), 186 deletion(s)(-) diff --git a/internal/api/handlers/actor/get_posts_test.go b/internal/api/handlers/actor/get_posts_test.go --- a/internal/api/handlers/actor/get_posts_test.go +++ b/internal/api/handlers/actor/get_posts_test.go @@ -30,7 +30,11 @@ Cursor: nil, }, nil } -func (m *mockPostService) CreatePost(ctx context.Context, req posts.CreatePostRequest) (*posts.CreatePostResponse, error) { +func (m *mockPostService) CreatePost(ctx context.Context, session *oauthlib.ClientSessionData, req posts.CreatePostRequest) (*posts.CreatePostResponse, error) { + return nil, nil +} + +func (m *mockPostService) UpdatePost(context.Context, *oauthlib.ClientSessionData, posts.UpdatePostRequest) (*posts.UpdatePostResponse, error) { return nil, nil } diff --git a/internal/api/handlers/post/create.go b/internal/api/handlers/post/create.go --- a/internal/api/handlers/post/create.go +++ b/internal/api/handlers/post/create.go @@ -88,9 +88,15 @@ // 7. Set author from authenticated user context req.AuthorDID = userDID - // 8. Call service to create post (write-forward to PDS) - // Note: Service layer will resolve community at-identifier (handle or DID) to DID - response, err := h.service.CreatePost(r.Context(), req) + // 8. Call service to create post (into the AUTHOR's repo, §4.2) + // Note: Service layer will resolve community at-identifier (handle or DID) to DID. + // + // The OAuth session travels with the request because the post is signed + // with the AUTHOR's credentials now, not the community's. It may be absent + // for a non-interactive author (an aggregator authenticated by API key), + // and the service resolves that author's stored tokens instead — so a nil + // session here is not an error to raise at the boundary. + response, err := h.service.CreatePost(r.Context(), middleware.GetOAuthSession(r), req) if err != nil { handleServiceError(w, err) return diff --git a/internal/api/handlers/post/create_security_test.go b/internal/api/handlers/post/create_security_test.go --- a/internal/api/handlers/post/create_security_test.go +++ b/internal/api/handlers/post/create_security_test.go @@ -296,6 +296,9 @@ createAs := func(contextDID, requestDID string) error { content := "Test post" _, err := stack.service.CreatePost( middleware.SetTestUserDID(t.Context(), contextDID), + // No session: the authorship checks under test run before any + // credential is needed, which is exactly the ordering they assert. + nil, posts.CreatePostRequest{ Community: communityDID, AuthorDID: requestDID, diff --git a/internal/api/handlers/post/get_test.go b/internal/api/handlers/post/get_test.go --- a/internal/api/handlers/post/get_test.go +++ b/internal/api/handlers/post/get_test.go @@ -20,7 +20,11 @@ type mockGetPostService struct { getPostsFunc func(ctx context.Context, req posts.GetPostsRequest) ([]*posts.PostResult, error) } -func (m *mockGetPostService) CreatePost(ctx context.Context, req posts.CreatePostRequest) (*posts.CreatePostResponse, error) { +func (m *mockGetPostService) CreatePost(ctx context.Context, session *oauthlib.ClientSessionData, req posts.CreatePostRequest) (*posts.CreatePostResponse, error) { + return nil, nil +} + +func (m *mockGetPostService) UpdatePost(context.Context, *oauthlib.ClientSessionData, posts.UpdatePostRequest) (*posts.UpdatePostResponse, error) { return nil, nil } diff --git a/internal/api/routes/post_aggregator_test.go b/internal/api/routes/post_aggregator_test.go --- a/internal/api/routes/post_aggregator_test.go +++ b/internal/api/routes/post_aggregator_test.go @@ -13,6 +13,7 @@ "Coves/internal/api/middleware" "Coves/internal/core/aggregators" "Coves/internal/core/posts" + "github.com/bluesky-social/indigo/atproto/auth/oauth" "github.com/go-chi/chi/v5" ) @@ -76,13 +77,17 @@ received posts.CreatePostRequest calls int } -func (s *stubPostService) CreatePost(_ context.Context, req posts.CreatePostRequest) (*posts.CreatePostResponse, error) { +func (s *stubPostService) CreatePost(_ context.Context, _ *oauth.ClientSessionData, req posts.CreatePostRequest) (*posts.CreatePostResponse, error) { s.calls++ s.received = req if s.err != nil { return nil, s.err } return s.response, nil +} + +func (s *stubPostService) UpdatePost(_ context.Context, _ *oauth.ClientSessionData, _ posts.UpdatePostRequest) (*posts.UpdatePostResponse, error) { + return nil, nil } // serviceJWTPrincipal is the auth middleware DualAuthMiddleware becomes once a diff --git a/internal/core/posts/engine.go b/internal/core/posts/engine.go --- a/internal/core/posts/engine.go +++ b/internal/core/posts/engine.go @@ -145,6 +145,31 @@ credentials: credentials, } } +// AcceptSubmission settles a post the write path has JUST written, without +// waiting for the firehose copy of it to arrive — §4.2 step 4's local-community +// fast path. +// +// IT IS NOT ProcessAdmission WITH A SHORTCUT. The difference is postCID, and it +// is a guard rather than a convenience: the caller has just committed a specific +// version of the record and is asking this engine to accept THAT version. The +// row it reads must be pending and must still hold that exact evaluated CID, or +// the pass defers — because between the write and this call the firehose may +// already have delivered an EDIT, and an acceptance pinning the version the +// author has replaced is an acceptance of content nobody is reading. +// +// A COMMUNITY THIS APPVIEW DOES NOT HOST IS NOT AN ERROR TO ESCALATE. The +// factory answers ErrCommunityNotHosted, and the write path's correct response +// is to leave the post pending and tell the author so — the community will +// decide for itself when the post reaches it (§4.2 step 5). The sentinel travels +// out wrapped so the caller can tell it from a genuine acceptance failure, which +// leaves the post pending too but is worth alerting on. +// +// RED STUB (task 6): pinned by service_writeflip_test.go. +func (e *AcceptanceEngine) AcceptSubmission(ctx context.Context, communityDID, postURI, postCID string) (EngineOutcome, error) { + _, _, _, _ = ctx, communityDID, postURI, postCID + return EngineDeferred, nil +} + // ProcessAdmission settles one (community, post) subject. // // ROUTING IS KEYED ON THE ROW'S STATUS AND THE DECISION TOGETHER, because the diff --git a/internal/core/posts/errors.go b/internal/core/posts/errors.go --- a/internal/core/posts/errors.go +++ b/internal/core/posts/errors.go @@ -53,6 +53,31 @@ // the admission gate, while a conflict is the indexer meeting a record it // already has, and collapsing them would let a refused post be reported as // successfully indexed. ErrDuplicateSubmission = errors.New("an identical submission from this author to this community was refused as a repeat") + + // ErrNoAuthorCredentials is returned when the AppView cannot open the + // AUTHOR's repository because it holds nothing to authenticate as them + // with: no OAuth session on the request and no stored session to resume + // (an aggregator whose tokens were never granted, or were revoked). + // + // IT IS ITS OWN SENTINEL RATHER THAN A GENERIC FAILURE because the two + // audiences need opposite things from it. A human's missing session is + // "sign in again" — a 401 the client can act on. An aggregator's revoked + // tokens are an operator problem: the service is running, correctly + // configured and completely unable to post, and a 500 saying "failed to + // write post to PDS" would have that diagnosed as a PDS outage. Posts used + // to be written with the COMMUNITY's credentials, so this class of failure + // did not exist before the write path flipped to the author's repo. + ErrNoAuthorCredentials = errors.New("no credentials to write to the author's repository") + + // ErrConcurrentModification is returned when an update's swap guard fires: + // the record changed between the read that shaped the edit and the write + // that would have applied it. + // + // The API boundary maps it to 409. Retrying is the client's decision, not + // the server's, because the edit was composed against content that no + // longer stands — silently re-reading and re-applying would let a second + // device's edit be overwritten by a first device that never saw it. + ErrConcurrentModification = errors.New("the post was modified concurrently") ) // ValidationError is the shared validation error type. It is aliased rather diff --git a/internal/core/posts/interfaces.go b/internal/core/posts/interfaces.go --- a/internal/core/posts/interfaces.go +++ b/internal/core/posts/interfaces.go @@ -13,10 +13,38 @@ // Service defines the business logic interface for posts // Coordinates between Repository, community service, and PDS type Service interface { - // CreatePost creates a new post in a community - // Flow: Validate -> Fetch community -> Ensure fresh token -> Write to PDS -> Return URI/CID - // AppView indexing happens asynchronously via Jetstream consumer - CreatePost(ctx context.Context, req CreatePostRequest) (*CreatePostResponse, error) + // CreatePost writes a new post into the AUTHOR's repository and, when this + // AppView hosts the community, settles the community's admission + // synchronously (docs/PRD_AUTHOR_OWNED_POSTS.md §4.2). + // + // Flow: Validate -> Admission -> Write postv2 to the author's repo at the + // deterministic rkey -> seed the admission row -> local fast-path acceptance + // -> return URI/CID/status. AppView indexing still happens asynchronously + // via the Jetstream consumer; the fast path only means the community's + // answer does not wait for it. + // + // THE SESSION IS THE AUTHOR'S OWN, and it is an argument rather than a + // context value because it is the credential the record gets signed under. + // It may be nil for a non-interactive author — an aggregator posting on its + // stored tokens — in which case the service resolves those and answers + // ErrNoAuthorCredentials if there are none. + CreatePost(ctx context.Context, session *oauth.ClientSessionData, req CreatePostRequest) (*CreatePostResponse, error) + + // UpdatePost edits an existing post in place, in the author's repository. + // + // The record's community and createdAt are PRESERVED from the standing + // record rather than taken from the request: the first is immutable by + // lexicon, and the second is what every feed orders by, so re-stamping it + // would jump an edited post back to the top of every sort. + // + // The edit is guarded by the standing record's CID, so an edit racing + // another edit is ErrConcurrentModification rather than a silent overwrite. + // The community's ADMISSION LEDGER is untouched: an edit is not a + // submission, so it consumes no quota and cannot be refused as a duplicate + // of the post it is editing. Whether the edited content is still acceptable + // is the acceptance engine's question, asked when the edit reaches the + // firehose (§5.5). + UpdatePost(ctx context.Context, session *oauth.ClientSessionData, req UpdatePostRequest) (*UpdatePostResponse, error) // GetAuthorPosts retrieves posts authored by a specific user for their profile page // Supports filtering by post type (with/without replies, media only) and community @@ -33,13 +61,20 @@ // and a BlockChecker is wired, posts authored by users the viewer has blocked are // returned as BlockedPost markers instead of full views (matching feed/timeline). GetPosts(ctx context.Context, req GetPostsRequest) ([]*PostResult, error) - // DeletePost deletes a post from the community's PDS repository - // SECURITY: Only the post author can delete their own posts - // Flow: Validate URI -> Fetch community -> Verify author -> Delete from PDS + // DeletePost removes a post record from the repository that holds it. + // SECURITY: Only the post author can delete their own posts. + // + // BOTH post collections are supported, and they authorize differently + // because they live in different repos. A postv2 URI names the AUTHOR's + // repo, so authorization is the URI's authority against the session DID — + // a local check, decided before anything is fetched. A deprecated + // community.post URI names the COMMUNITY's repo, where the delete goes out + // on the community's credentials, so the record's `author` field has to be + // read back and compared. The second path exists until task 8's + // re-materialization retires the collection. DeletePost(ctx context.Context, session *oauth.ClientSessionData, req DeletePostRequest) error // Future methods (Beta): - // UpdatePost(ctx context.Context, req UpdatePostRequest) (*Post, error) // ListCommunityPosts(ctx context.Context, communityDID string, limit, offset int) ([]*Post, error) } diff --git a/internal/core/posts/post.go b/internal/core/posts/post.go --- a/internal/core/posts/post.go +++ b/internal/core/posts/post.go @@ -69,6 +69,54 @@ // Matches social.coves.community.post.create lexicon output schema type CreatePostResponse struct { URI string `json:"uri"` // AT-URI of created post CID string `json:"cid"` // CID of created post + + // Status is the community's decision as of this response: PostStatusAccepted + // when the local fast path settled it synchronously, PostStatusPending when + // the community still owes a decision (§4.2 steps 4 and 5). + // + // IT IS NOT AN ERROR CHANNEL. Pending is a SUCCESS: the author's record + // exists and is theirs whatever the community decides, and the acceptance + // this AppView failed to write is retried idempotently by the firehose + // engine. A client that treated pending as a failure and resubmitted would + // be answered with its own post's URI, because the rkey is deterministic — + // but it would also show its author an error over a post that was written. + // + // Omitted when empty so pre-flip clients, which have never seen the field, + // decode a response identical to the one they used to get. + Status string `json:"status,omitempty"` +} + +// UpdatePostRequest represents input for editing an existing post. +// +// It carries the post's URI and the mutable content fields ONLY. There is +// deliberately no community field: the postv2 lexicon calls `community` +// immutable — retargeting a post means writing a new post record, and consumers +// discard an update event that changes it — so an edit that could express a +// retarget would be an edit whose only possible outcome is being ignored by +// every reader. +type UpdatePostRequest struct { + Title *string `json:"title,omitempty"` + Content *string `json:"content,omitempty"` + Embed map[string]interface{} `json:"embed,omitempty"` + Labels *SelfLabels `json:"labels,omitempty"` + + // Community is accepted so that a client which sends it can be REFUSED + // rather than silently obeyed-in-part. See the type comment: it is not a + // field an edit may change, and a request naming a different community is a + // validation error, not a partially applied update. + Community string `json:"community,omitempty"` + + URI string `json:"uri"` + Facets []interface{} `json:"facets,omitempty"` + Langs []string `json:"langs,omitempty"` + Tags []string `json:"tags,omitempty"` +} + +// UpdatePostResponse is the edited record's identity: the same URI it always +// had, and the NEW CID the edit committed. +type UpdatePostResponse struct { + URI string `json:"uri"` + CID string `json:"cid"` } // DeletePostRequest represents input for deleting a post diff --git a/internal/core/posts/postv2.go b/internal/core/posts/postv2.go new file mode 100644 --- /dev/null +++ b/internal/core/posts/postv2.go @@ -0,0 +1,200 @@ +package posts + +import ( + "context" + "time" + + "github.com/bluesky-social/indigo/atproto/auth/oauth" + + "Coves/internal/atproto/pds" +) + +// The author-repo half of the write path (docs/PRD_AUTHOR_OWNED_POSTS.md §3.1, +// §4.2): the record shape a post takes in its AUTHOR's repository, the +// deterministic key it is written at, and the seam the author's own credentials +// arrive through. +// +// STUB — the declarations exist so the contract can be written against them. +// Behaviour is task 6's GREEN cycle. + +// PostStatus is what a CreatePost response reports about the community's +// decision, so a client knows whether the post is already visible in the +// community or is waiting on one (§4.2 steps 4 and 5). +const ( + // PostStatusAccepted means this AppView hosts the community, ran admission + // synchronously, and a community acceptance now stands for the post. + PostStatusAccepted = "accepted" + + // PostStatusPending means the post exists in the author's repo but no + // community acceptance covers it yet — either the community is hosted + // elsewhere and has to decide for itself (§4.2 step 5), or the synchronous + // acceptance failed and the firehose engine will retry it (§5.6). + PostStatusPending = "pending" +) + +// StrongRef is com.atproto.repo.strongRef: a record URI pinned to the exact +// version of the record it names. +type StrongRef struct { + URI string `json:"uri"` + CID string `json:"cid"` +} + +// BridgedStats is social.coves.community.postv2#bridgedStats: origin-platform +// vote aggregates asserted by a bridge for federated content. +type BridgedStats struct { + Upvotes int `json:"upvotes"` + Downvotes int `json:"downvotes"` + AsOf string `json:"asOf"` +} + +// PostV2Record is the social.coves.community.postv2 record: a post as it lives +// in its AUTHOR's repository. +// +// THERE IS NO AUTHOR FIELD, AND THAT IS THE WHOLE POINT. Under the deprecated +// social.coves.community.post the record lived in the COMMUNITY's repo, so the +// only thing that could say who wrote it was a field the community's own +// credentials had signed — an assertion by the community about a third party. +// Here authorship is the repository the record is in, which a verifying relay +// or a DID-resolved direct fetch can attribute. Re-adding the field would +// reintroduce a self-asserted, unverifiable claim beside a verifiable one, and +// consumers would have two answers to one question. +// +// Community is a DID rather than the at-identifier the client typed, and the +// lexicon calls it immutable: retargeting a post means writing a NEW post +// record, so an update that changes it is discarded entire by consumers. +type PostV2Record struct { + Title *string `json:"title,omitempty"` + Content *string `json:"content,omitempty"` + Embed map[string]interface{} `json:"embed,omitempty"` + Labels *SelfLabels `json:"labels,omitempty"` + CrosspostOf *StrongRef `json:"crosspostOf,omitempty"` + BridgedStats *BridgedStats `json:"bridgedStats,omitempty"` + Type string `json:"$type"` + Community string `json:"community"` + CreatedAt string `json:"createdAt"` + Facets []interface{} `json:"facets,omitempty"` + Langs []string `json:"langs,omitempty"` + Tags []string `json:"tags,omitempty"` + CrosspostChain []StrongRef `json:"crosspostChain,omitempty"` +} + +// SubmissionRkey is the record key a submission's postv2 record is written at: +// a valid TID derived from the submission itself rather than from the clock. +// +// WHY DETERMINISTIC. §4.2 records a lost-response asymmetry on the write path: +// when a PDS write's outcome is ambiguous the record may or may not exist, and a +// client that retries produces a SECOND post. A server-chosen TID cannot fix +// that — every attempt gets a fresh one. A key derived from what is being +// submitted makes the retry aim at the record the first attempt may already have +// written, so a create-only write (swapRecord "must not exist") either creates +// it once or reports the standing one, and the duplicate becomes impossible +// rather than merely unlikely. +// +// THE MATERIAL IS (community, fingerprint, bucket), AND EACH PART EARNS ITS +// PLACE: +// +// - The RESOLVED community DID. The fingerprint deliberately excludes the +// community (see submissionFingerprint: the client types a handle one time +// and a DID the next, and the ledger's unique key already scopes it), but +// the rkey must NOT: the same content submitted to two communities is two +// posts, and two posts sharing one rkey in one author repo is one post that +// silently overwrote the other. +// - The fingerprint, which is what makes a retry of the same content collide +// with itself. +// - The dedupe bucket, which is what makes the collision EXPIRE. Without it an +// author could never repost identical content, because the rkey would name a +// record that already exists forever. +// +// THE ANSWER IS A REAL TID, not merely a 13-character string. The postv2 +// lexicon declares `"key": "tid"`, so a PDS may validate it, feed ordering reads +// the timestamp out of it, and a key that merely looked like one would sort +// posts to a plausible-but-wrong moment. The timestamp is placed INSIDE the +// submission's own dedupe bucket — bucket start plus an offset drawn from the +// digest — so the derived time is within one dedupe window of when the post was +// actually submitted, and the clock ID carries further digest bits so two +// submissions landing on the same microsecond still differ. +func SubmissionRkey(communityDID, fingerprint string, bucket int64, dedupeWindow time.Duration) string { + // RED STUB (task 6): the contract is pinned in submission_rkey_test.go. + // It answers the empty string rather than panicking so the reds read as + // failed assertions naming the expected key, not as a stack trace. + _, _, _, _ = communityDID, fingerprint, bucket, dedupeWindow + return "" +} + +// AuthorRepo is one author's PDS repository, narrowed to what the write path +// does with it. +// +// It is declared here rather than taken as pds.CommitClient for the same reason +// CommunityRepo is: the tests fake four methods instead of a dozen, and the +// dependency reads as what it is — a repo we read before we write. +// +// PutRecordWithCommit rather than CreateRecord, because every write on this path +// is GUARDED. A create needs swapRecord "" (the record must not exist) so a +// retry that finds the record already there is told rather than minting a +// second; an update needs the standing CID so a concurrent edit is a detected +// conflict rather than a silent clobber. +type AuthorRepo interface { + // GetRecord reads the standing record — the pre-read an update shapes its + // swap guard from, and what a create falls back to when its guard fires. + GetRecord(ctx context.Context, collection, rkey string) (*pds.RecordResponse, error) + + // PutRecordWithCommit writes one record under a swap guard. An empty + // swapRecord means "there must be nothing here". + PutRecordWithCommit(ctx context.Context, collection, rkey string, record any, swapRecord string) (*pds.RecordCommit, error) + + // DeleteRecord removes a record from the author's repo. + DeleteRecord(ctx context.Context, collection, rkey string) error + + // DID is the repo being written — the author's own identity, which is the + // authority half of every post URI this path produces. + DID() string +} + +// AuthorRepoFactory opens an authenticated client on ONE author's repo. +// +// The session is the author's own OAuth session, as the API boundary already +// holds it (middleware.GetOAuthSession) and as comments' PDSClientFactory +// already takes it. It may be nil for a non-interactive author — an aggregator +// posting under its stored tokens (migration 025) — and the production factory +// resolves those through the OAuth app's session store, answering +// ErrNoAuthorCredentials when there is nothing to resume. +// +// authorDID is passed alongside rather than read off the session so that the +// nil-session path has an identity to resolve at all, and so a factory can +// assert the two agree. +type AuthorRepoFactory func(ctx context.Context, authorDID string, session *oauth.ClientSessionData) (AuthorRepo, error) + +// WithAuthorRepoFactory supplies the seam the author's own credentials arrive +// through. Integration tests inject password auth over a real PDS; production +// injects OAuth/DPoP. +func WithAuthorRepoFactory(factory AuthorRepoFactory) PostServiceOption { + return func(s *postService) { s.authorRepos = factory } +} + +// SubmissionAcceptor is the acceptance engine's SYNCHRONOUS entry point — §4.2 +// step 4's local-community fast path. +// +// It is an interface on the post service rather than a *AcceptanceEngine so the +// write path can be tested against an acceptance that fails on purpose. That +// case is not exotic: the whole design of the fast path is that its failure is +// invisible to the author, and a fixture that cannot make it fail cannot prove +// the author's record survives. +type SubmissionAcceptor interface { + // AcceptSubmission settles a post this AppView has just written, without + // waiting for the firehose copy of it to come back. + AcceptSubmission(ctx context.Context, communityDID, postURI, postCID string) (EngineOutcome, error) +} + +// WithSyncAcceptance wires the local-community fast path: the admission row the +// post is seeded into, and the engine that settles it. +// +// Both or neither. A service holding the acceptor but not the repository would +// hand the engine a subject with no row to read; one holding the repository but +// not the acceptor would seed rows nothing ever settles until the firehose +// arrives. +func WithSyncAcceptance(admissions AdmissionRepository, acceptor SubmissionAcceptor) PostServiceOption { + return func(s *postService) { + s.admissions = admissions + s.acceptor = acceptor + } +} diff --git a/internal/core/posts/postv2_record_test.go b/internal/core/posts/postv2_record_test.go new file mode 100644 --- /dev/null +++ b/internal/core/posts/postv2_record_test.go @@ -0,0 +1,164 @@ +package posts + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The shape of the record that goes into the AUTHOR's repository. +// +// social.coves.community.postv2 is a different record from the deprecated +// social.coves.community.post, not a renamed one, and the difference that +// matters is a field that is GONE. A post used to live in the community's repo, +// so the only thing that could say who wrote it was an `author` field the +// COMMUNITY's credentials had signed — an assertion by one party about another, +// which no consumer could verify. In the author's own repo the repository IS the +// attribution, and a relay or a DID-resolved fetch can check it. +// +// So the assertion below is not a tidiness check. A PostV2Record that carried an +// author field would put a self-asserted claim beside a verifiable one, and every +// consumer would have two answers to "who wrote this" with no rule for which +// wins. The postv2 lexicon does not declare the field at all, so a record +// carrying it is also a record with an unknown key in it. + +func TestPostV2Record_CarriesNoAuthorField(t *testing.T) { + t.Parallel() + + title := "a post in its author's repo" + content := "the repository is the attribution" + + encoded, err := json.Marshal(PostV2Record{ + Type: PostV2Collection, + Community: "did:plc:aaaaaaaaaaaaaaaaaaaaaaaa", + Title: &title, + Content: &content, + CreatedAt: "2026-08-08T12:00:00Z", + }) + require.NoError(t, err) + + var decoded map[string]any + require.NoError(t, json.Unmarshal(encoded, &decoded)) + + // Asserted over the DECODED KEYS rather than by a substring search, so that + // a post whose CONTENT mentions the word "author" cannot make this pass or + // fail by accident. + for _, forbidden := range []string{"author", "authorDid", "author_did"} { + assert.NotContainsf(t, decoded, forbidden, + "the postv2 record carries %q; authorship is the repository the record lives in, and a "+ + "self-asserted author field beside it gives consumers two answers to one question "+ + "with no rule for which wins", forbidden) + } +} + +func TestPostV2Record_RequiredFieldsAreAlwaysPresent(t *testing.T) { + t.Parallel() + + // The lexicon requires community and createdAt. Both are spelled without + // omitempty for that reason: a record missing either is refused by any + // consumer validating against the schema, and an empty-but-present field + // fails loudly at the validator instead of silently at the reader. + encoded, err := json.Marshal(PostV2Record{Type: PostV2Collection}) + require.NoError(t, err) + + var decoded map[string]any + require.NoError(t, json.Unmarshal(encoded, &decoded)) + + for _, required := range []string{"$type", "community", "createdAt"} { + assert.Containsf(t, decoded, required, + "the postv2 lexicon requires %q, so it must survive marshalling even when unset — "+ + "an omitempty here turns a wiring bug into a record that silently fails validation "+ + "on every consumer but ours", required) + } +} + +func TestPostV2Record_OptionalFieldsAreOmittedWhenUnset(t *testing.T) { + t.Parallel() + + // The mirror of the above. An empty optional field is not the same as an + // absent one: `"embed": null` is a union member the lexicon has no ref for, + // and `"tags": []` is a tag list a client will render as an empty row of + // chips. Absent means absent. + encoded, err := json.Marshal(PostV2Record{ + Type: PostV2Collection, + Community: "did:plc:aaaaaaaaaaaaaaaaaaaaaaaa", + CreatedAt: "2026-08-08T12:00:00Z", + }) + require.NoError(t, err) + + var decoded map[string]any + require.NoError(t, json.Unmarshal(encoded, &decoded)) + + for _, optional := range []string{ + "title", "content", "facets", "embed", "langs", "labels", + "tags", "crosspostOf", "crosspostChain", "bridgedStats", + } { + assert.NotContainsf(t, decoded, optional, + "the postv2 record emitted %q with nothing in it", optional) + } +} + +func TestPostV2Record_CarriesEverySurfaceTheLexiconDeclares(t *testing.T) { + t.Parallel() + + // Every optional property of social.coves.community.postv2, populated. A + // field the Go type does not have is a field the write path can never + // produce and a field an EDIT silently drops on round-trip — UpdatePost + // reads the standing record, re-marshals it, and puts it back, so anything + // absent from this struct is erased from a post the first time its author + // fixes a typo. + title := "every surface" + content := "populated" + + encoded, err := json.Marshal(PostV2Record{ + Type: PostV2Collection, + Community: "did:plc:aaaaaaaaaaaaaaaaaaaaaaaa", + CreatedAt: "2026-08-08T12:00:00Z", + Title: &title, + Content: &content, + Facets: []interface{}{map[string]any{"$type": "social.coves.richtext.facet"}}, + Embed: map[string]interface{}{"$type": "social.coves.embed.external"}, + Langs: []string{"en", "fr"}, + Labels: &SelfLabels{Values: []SelfLabel{{Val: "nsfw"}}}, + Tags: []string{"gardening"}, + CrosspostOf: &StrongRef{ + URI: "at://did:plc:bbbbbbbbbbbbbbbbbbbbbbbb/" + PostV2Collection + "/3lrc77gmww4nc", + CID: "bafyoriginal", + }, + CrosspostChain: []StrongRef{{ + URI: "at://did:plc:bbbbbbbbbbbbbbbbbbbbbbbb/" + PostV2Collection + "/3lrc77gmww4nc", + CID: "bafyoriginal", + }}, + BridgedStats: &BridgedStats{Upvotes: 12, Downvotes: 3, AsOf: "2026-08-08T11:00:00Z"}, + }) + require.NoError(t, err) + + var decoded map[string]any + require.NoError(t, json.Unmarshal(encoded, &decoded)) + + for _, declared := range []string{ + "$type", "community", "createdAt", "title", "content", "facets", "embed", + "langs", "labels", "tags", "crosspostOf", "crosspostChain", "bridgedStats", + } { + assert.Containsf(t, decoded, declared, + "the postv2 lexicon declares %q and the Go record does not emit it", declared) + } + + // The two ref-typed surfaces are spelled the way the lexicons they point at + // spell them — com.atproto.repo.strongRef is {uri, cid}, and #bridgedStats + // is {upvotes, downvotes, asOf}. Asserted because a struct whose field names + // marshalled to anything else would still satisfy the presence checks above + // while producing a record no consumer can read. + assert.Equal(t, map[string]any{ + "uri": "at://did:plc:bbbbbbbbbbbbbbbbbbbbbbbb/" + PostV2Collection + "/3lrc77gmww4nc", + "cid": "bafyoriginal", + }, decoded["crosspostOf"]) + + assert.Equal(t, map[string]any{ + "upvotes": float64(12), + "downvotes": float64(3), + "asOf": "2026-08-08T11:00:00Z", + }, decoded["bridgedStats"]) +} diff --git a/internal/core/posts/service.go b/internal/core/posts/service.go --- a/internal/core/posts/service.go +++ b/internal/core/posts/service.go @@ -35,6 +35,14 @@ blueskyService blueskypost.Service blockChecker BlockChecker admission *AdmissionPolicy pdsURL string + + // The author-owned write path (§4.2). authorRepos opens the AUTHOR's repo + // under the author's own credentials; admissions and acceptor are the + // local-community fast path — the row the post is seeded into and the + // engine that settles it. See postv2.go. + authorRepos AuthorRepoFactory + admissions AdmissionRepository + acceptor SubmissionAcceptor } // PostServiceOption configures optional postService dependencies. Options keep the @@ -100,7 +108,13 @@ // more to the point, leaves no record in a community that refused it. Every // failure AFTER admission (steps 5-8) must release the ledger reservation the // admission took, or the failure costs the author a quota slot and refuses // their retry as a duplicate. -func (s *postService) CreatePost(ctx context.Context, req CreatePostRequest) (*CreatePostResponse, error) { +func (s *postService) CreatePost(ctx context.Context, session *oauth.ClientSessionData, req CreatePostRequest) (*CreatePostResponse, error) { + // RED STUB SEAM (task 6): the session is the author's credential and is + // consumed by the author-repo write the GREEN cycle installs below. It is + // accepted here so the contract compiles against the flipped signature + // while the body still write-forwards to the community's repo. + _ = session + // 1. Validate basic input (before DID checks to give clear validation errors) if err := s.validateCreateRequest(&req); err != nil { return nil, err @@ -251,6 +265,15 @@ return &CreatePostResponse{ URI: uri, CID: cid, }, nil +} + +// UpdatePost edits a post in place in the author's repository. +// +// RED STUB (task 6): see interfaces.go for the contract and +// service_writeflip_test.go for the pinned journey. +func (s *postService) UpdatePost(ctx context.Context, session *oauth.ClientSessionData, req UpdatePostRequest) (*UpdatePostResponse, error) { + _, _, _ = ctx, session, req + return nil, ErrNotFound } // postRecordFor builds the record a request describes, stamped with the given diff --git a/internal/core/posts/service_admission_test.go b/internal/core/posts/service_admission_test.go --- a/internal/core/posts/service_admission_test.go +++ b/internal/core/posts/service_admission_test.go @@ -117,12 +117,17 @@ base: base, service: posts.NewPostService( postgres.NewPostRepository(base.db), base.communityService, nil, nil, nil, nil, base.pds.URL(), - posts.WithAdmissionPolicy(posts.AdmissionPolicy{ - Ledger: postgres.NewSubmissionLedger(base.db), - Bans: base.communityService, - Limits: limits, - Now: clock.Now, - })), + // The write path needs the author's own credentials now (§4.2): a + // post is written to the AUTHOR's repo, so a service wired without + // the factory could not write one at all and every refusal here + // would pass for the wrong reason. + append(base.writePathOptions(), + posts.WithAdmissionPolicy(posts.AdmissionPolicy{ + Ledger: postgres.NewSubmissionLedger(base.db), + Bans: base.communityService, + Limits: limits, + Now: clock.Now, + }))...), repo: postgres.NewCommunityRepository(base.db), clock: clock, limits: limits, @@ -137,6 +142,7 @@ content := "a body that makes this a complete post" return f.service.CreatePost( middleware.SetTestUserDID(context.Background(), f.base.author.DID), + sessionFor(t, f.base.author, f.base.pds.URL()), posts.CreatePostRequest{ Community: communityDID, Title: &title, @@ -330,18 +336,17 @@ t.Parallel() f := newAdmissionFixture(t) - // A PDS that refuses every write. Pointing the community's stored pds_url at - // it is how the failure is injected: createPostOnPDS reads the URL off the - // community row it just fetched (service.go, "each community can be hosted - // on a different PDS instance"), so this is the real write path failing for - // a real reason rather than a stubbed-out client. + // A PDS that refuses every write. Pointing the AUTHOR's repo client at it is + // how the failure is injected, because the write goes to the author's repo + // now (§4.2 step 3) — the community's stored pds_url, which this test used to + // break, is no longer on the create path at all. It is still the real write + // failing for a real reason rather than a stubbed-out client. broken := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { http.Error(w, `{"error":"InternalServerError"}`, http.StatusInternalServerError) })) t.Cleanup(broken.Close) - healthyURL := communityPDSURL(t, f.base.db, f.base.community.DID) - setCommunityPDSURL(t, f.base.db, f.base.community.DID, broken.URL) + f.base.authorRepos.pointAt(f.base.author.DID, broken.URL) const repeatable = "a post whose write will fail the first time" _, err := f.submit(t, f.base.community.DID, repeatable) @@ -350,7 +355,7 @@ assert.Zerof(t, f.ledgerRows(t, f.base.community.DID), "the reservation for a post that was never written is still on the ledger: it has burned a quota slot and will refuse the retry as a duplicate") - setCommunityPDSURL(t, f.base.db, f.base.community.DID, healthyURL) + f.base.authorRepos.pointAt(f.base.author.DID, "") // The retry a client would actually send: byte-identical content. It must be // admitted, which is only possible if the reservation was released. @@ -361,10 +366,11 @@ assert.Equal(t, 1, f.ledgerRows(t, f.base.community.DID), "exactly one submission survived: the failed attempt released its row and the retry took a fresh one") - // The record really is in the community's repo, so "admitted" here means a - // post exists rather than merely that no error came back. - record := f.base.communityAccount(t).GetRecord(t, postCollection, rkeyOf(t, resp.URI)) - assert.Equal(t, f.base.author.DID, record.Value["author"]) + // The record really is in the AUTHOR's repo, so "admitted" here means a post + // exists rather than merely that no error came back. The repo flipped in + // task 6 (§3.1); what this assertion is for did not. + record := f.base.author.GetRecord(t, posts.PostV2Collection, rkeyOf(t, resp.URI)) + assert.Equal(t, f.base.community.DID, record.Value["community"]) } // A client that goes away MID-WRITE must still get its reservation back. @@ -400,12 +406,11 @@ http.Error(w, `{"error":"InternalServerError"}`, http.StatusInternalServerError) })) t.Cleanup(canceling.Close) - healthyURL := communityPDSURL(t, f.base.db, f.base.community.DID) - setCommunityPDSURL(t, f.base.db, f.base.community.DID, canceling.URL) + f.base.authorRepos.pointAt(f.base.author.DID, canceling.URL) const repeatable = "a post whose client disconnects mid-write" content := "a body that makes this a complete post" - _, err := f.service.CreatePost(ctx, posts.CreatePostRequest{ + _, err := f.service.CreatePost(ctx, sessionFor(t, f.base.author, f.base.pds.URL()), posts.CreatePostRequest{ Community: f.base.community.DID, Title: func() *string { s := repeatable; return &s }(), Content: &content, @@ -416,7 +421,7 @@ assert.Zerof(t, f.ledgerRows(t, f.base.community.DID), "the release ran on the caller's canceled context and was refused with it: the reservation leaked, burning a quota slot and blocking the retry as a duplicate") - setCommunityPDSURL(t, f.base.db, f.base.community.DID, healthyURL) + f.base.authorRepos.pointAt(f.base.author.DID, "") // The retry a reconnected client sends: byte-identical content on a live // context. Admissible only if the canceled attempt released its row. @@ -504,6 +509,7 @@ external["thumb"] = thumb } return f.service.CreatePost( middleware.SetTestUserDID(context.Background(), f.base.author.DID), + sessionFor(t, f.base.author, f.base.pds.URL()), posts.CreatePostRequest{ Community: f.base.community.DID, Title: &title, diff --git a/internal/core/posts/service_aggregator_test.go b/internal/core/posts/service_aggregator_test.go --- a/internal/core/posts/service_aggregator_test.go +++ b/internal/core/posts/service_aggregator_test.go @@ -49,6 +49,7 @@ type aggregatorFixture struct { base *postFixture service posts.Service index aggregators.Repository + aggregator *testkit.Account aggregatorDID string // The authorization's AT-URI, reused on every re-index so that flipping @@ -60,9 +61,12 @@ // newAggregatorFixture declares an aggregator, has the fixture's community // authorize it, and points a post service at both. // -// The aggregator gets no PDS account: a post lives in the COMMUNITY's repo and -// names its author in a field, so an aggregator needs an identity the AppView -// has indexed, not a repository of its own. +// THE AGGREGATOR NOW NEEDS A REPOSITORY OF ITS OWN. It used to need only an +// identity the AppView had indexed, because a post lived in the COMMUNITY's repo +// and named its author in a field. Under §4.2 step 3 an aggregator writes into +// its own repo like any other author — through its stored OAuth tokens +// (migration 025) in production, through a registered PDS account here — so the +// fixture provisions one and registers it with the author-repo factory. func newAggregatorFixture(t *testing.T) *aggregatorFixture { t.Helper() @@ -70,7 +74,9 @@ base := newPostFixture(t) ctx := context.Background() index := postgres.NewAggregatorRepository(base.db) - aggregatorDID := "did:plc:" + testkit.UniqueID(t) + aggregatorAccount := base.authorRepos.register( + base.pds.CreateAccount(t, testkit.WithHandlePrefix("ag"))) + aggregatorDID := aggregatorAccount.DID require.NoError(t, index.CreateAggregator(ctx, &aggregators.Aggregator{ DID: aggregatorDID, DisplayName: "RSS Feed Aggregator", @@ -86,10 +92,12 @@ service: posts.NewPostService( postgres.NewPostRepository(base.db), base.communityService, aggregators.NewAggregatorService(index, base.communityService), nil, nil, nil, base.pds.URL(), - // The aggregator's OWN hourly quota is the subject here; the §8 - // per-author policy is opted out of explicitly. - posts.WithAdmissionPolicy(posts.NewAllowAllAdmissionPolicyForTests())), + append(base.writePathOptions(), + // The aggregator's OWN hourly quota is the subject here; the §8 + // per-author policy is opted out of explicitly. + posts.WithAdmissionPolicy(posts.NewAllowAllAdmissionPolicyForTests()))...), index: index, + aggregator: aggregatorAccount, aggregatorDID: aggregatorDID, authorizationURI: "at://" + base.community.DID + "/social.coves.aggregator.authorization/" + testkit.UniqueID(t), @@ -131,6 +139,8 @@ content := "syndicated from a feed" return f.service.CreatePost( middleware.SetTestUserDID(context.Background(), f.aggregatorDID), + nil, // an aggregator authenticates by API key: there is no browser session, and + // the service resolves its stored tokens instead (§4.2 step 3). posts.CreatePostRequest{ Community: f.base.community.DID, Title: &title, @@ -156,13 +166,16 @@ f := newAggregatorFixture(t) resp, err := f.createPost(t, "Breaking news from an RSS feed") require.NoError(t, err) - // The record lands in the community's repo like any other post, with the - // aggregator named as its author. Read back from the PDS rather than from - // the response, because the response is the service quoting itself. - record := f.base.communityAccount(t).GetRecord(t, postCollection, rkeyOf(t, resp.URI)) + // The record lands in the AGGREGATOR's own repo like any other author's post, + // naming the community it was submitted to. Read back from the PDS rather + // than from the response, because the response is the service quoting itself. + assert.Equal(t, "at://"+f.aggregatorDID+"/"+posts.PostV2Collection+"/"+rkeyOf(t, resp.URI), resp.URI, + "an aggregator's post belongs to the aggregator's repo, not to the community's") + + record := f.aggregator.GetRecord(t, posts.PostV2Collection, rkeyOf(t, resp.URI)) assert.Equal(t, f.base.community.DID, record.Value["community"]) - assert.Equal(t, f.aggregatorDID, record.Value["author"], - "an aggregator's post must name the aggregator, not the community that hosts it") + assert.NotContains(t, record.Value, "author", + "authorship is the repo the record lives in — an aggregator's post is no exception") // And the post is billed against the aggregator's quota. Without this the // rate limit would never engage, because CreatePost records the post itself diff --git a/internal/core/posts/service_create_validation_test.go b/internal/core/posts/service_create_validation_test.go --- a/internal/core/posts/service_create_validation_test.go +++ b/internal/core/posts/service_create_validation_test.go @@ -32,16 +32,20 @@ // must refuse. Both halves need a real database — resolution is a lookup in the // communities table, and "community not found" is only meaningful against a // table that could have contained it. // -// # WHY THE COMMUNITY'S CREDENTIALS ARE DELIBERATELY FAKE +// # WHY THE AUTHOR HAS NO REPOSITORY HERE // -// The community row here is seeded straight into the index with an unusable PDS -// token, so every request that survives validation stops at the same place: -// "failed to refresh community credentials", the step immediately before the -// record is written. That is the assertion — reaching the write is what proves -// nothing earlier rejected the post — and it costs neither a provisioned PDS -// account nor a real repo write. A post that must actually arrive somewhere is -// service_writeforward_test.go's job, where the community is provisioned for -// real. +// The service is deliberately wired with NO author-repo factory, so every +// request that survives validation stops at the same place: opening the author's +// repository, the step immediately before the record is written, which answers +// ErrNoAuthorCredentials because there is nothing to authenticate as the author +// with. That is the assertion — reaching the write is what proves nothing +// earlier rejected the post — and it costs neither a provisioned PDS account nor +// a real repo write. A post that must actually arrive somewhere is +// service_writeforward_test.go's job, where both repos are provisioned for real. +// +// Before task 6 the same trick was played on the COMMUNITY's credentials, which +// the write path no longer touches: a post is written to its author's repo now +// (§4.2 step 3), so an unusable community token stops nothing. func TestService_CreateResolvesTheCommunityAndValidatesTheRequest(t *testing.T) { t.Parallel() @@ -92,7 +96,9 @@ // createPost sends a request as the author. The DID goes into the context as // well as the request body because the service cross-checks the two. createPost := func(req posts.CreatePostRequest) error { - _, err := postService.CreatePost(middleware.SetTestUserDID(ctx, authorDID), req) + // No session: this service has no author-repo factory either, so the two + // agree about what the author cannot be authenticated as. + _, err := postService.CreatePost(middleware.SetTestUserDID(ctx, authorDID), nil, req) return err } @@ -101,9 +107,9 @@ // the PDS write and failed only on the community's deliberately broken // credentials. reachedTheWrite := func(t *testing.T, err error) { t.Helper() - require.Error(t, err, "the seeded credentials cannot be refreshed, so the write must fail") - assert.Contains(t, err.Error(), "failed to refresh community credentials", - "the post should have been rejected by nothing before the PDS write") + require.Error(t, err, "there are no credentials for the author's repo, so the write must fail") + assert.ErrorIsf(t, err, posts.ErrNoAuthorCredentials, + "the post should have been rejected by nothing before the author-repo write; got: %v", err) } title := "Test Post Title" diff --git a/internal/core/posts/service_writeflip_test.go b/internal/core/posts/service_writeflip_test.go new file mode 100644 --- /dev/null +++ b/internal/core/posts/service_writeflip_test.go @@ -0,0 +1,513 @@ +//go:build integration + +package posts_test + +import ( + "context" + "errors" + "testing" + "time" + + "Coves/internal/api/middleware" + "Coves/internal/core/communities" + "Coves/internal/core/posts" + "Coves/internal/db/postgres" + "Coves/tests/testkit" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The whole journey of an author-owned post through the write path, against a +// real PDS and the real admissions table (docs/PRD_AUTHOR_OWNED_POSTS.md §4.2). +// +// service_writeforward_test.go proves WHERE the record lands. This file proves +// the four things that only become true once it lands there, and each one is a +// claim the AppView makes to a client that nothing else in the suite can check: +// +// - THE COMMUNITY'S ANSWER ARRIVES WITH THE RESPONSE, for a community this +// AppView hosts. §4.2 step 4 promises the UX is identical to the pre-flip +// one: the client gets URI, CID and an accepted post, without waiting for +// its own write to come back around the firehose. +// - A RETRY IS THE SAME POST. §4.2 names the lost-response asymmetry the +// deterministic rkey exists to close, and closing it means a retry produces +// the same URI, the same CID, and NO second commit — because a second commit +// is a second firehose event, and consumers would index an edit that never +// happened. +// - A COMMUNITY WE DO NOT HOST, AND AN ACCEPTANCE THAT FAILED, ARE THE SAME +// ANSWER TO THE AUTHOR: their record stands, the response succeeds, the post +// is pending. The author's record is NEVER rolled back — §4.2 is explicit +// that a failed acceptance is degraded latency, not data loss. +// - AN EDIT IS NOT A SUBMISSION. It preserves what the lexicon calls +// immutable, it is guarded against a concurrent edit, and it leaves the +// submission ledger completely alone. + +// hostedRkeyOf is the acceptance record key for a subject — the SAME derivation +// the engine and the firehose consumer use, asserted here so that a fast path +// which computed its own would be caught rather than merely producing a record +// nobody looks up. +func hostedRkeyOf(postURI string) string { return posts.SubjectRkey(postURI) } + +// admissionOf reads the row the fast path seeded and settled. +func (f *postFixture) admissionOf(t *testing.T, postURI string) *posts.Admission { + t.Helper() + + row, err := f.admissions.Get(context.Background(), f.community.DID, postURI) + require.NoErrorf(t, err, "reading the admission of %s", postURI) + require.NotNilf(t, row, "no admission row exists for %s — the write path must seed one before it "+ + "asks the engine to settle it, and the URI it seeds has to be byte-identical to the one the "+ + "firehose consumer will build, or the two will index the same post as two subjects", postURI) + return row +} + +// repoHead is a repo's current commit revision. A write that committed moves it; +// one that was correctly skipped does not. +func repoHead(t *testing.T, account *testkit.Account) string { + t.Helper() + + var resp struct { + Rev string `json:"rev"` + } + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + require.NoError(t, account.XRPC().Query(ctx, "com.atproto.sync.getLatestCommit", + map[string][]string{"did": {account.DID}}, &resp)) + require.NotEmpty(t, resp.Rev) + return resp.Rev +} + +func TestService_CreateAcceptsSynchronouslyInAHostedCommunity(t *testing.T) { + t.Parallel() + + f := newPostFixture(t) + resp := f.createPost(t, f.author.DID, "accepted on the way in", "body") + + // 1. THE RESPONSE SAYS ACCEPTED. This is the field a client renders the + // difference from: an accepted post appears in the community immediately, + // a pending one shows its author a "waiting for the community" state. + assert.Equal(t, posts.PostStatusAccepted, resp.Status, + "this AppView holds the community's credentials, so it IS the admission authority (§4.2 step 4) "+ + "and must settle the post before answering rather than leaving the author to poll") + + // 2. THE ADMISSION ROW IS ACCEPTED, pinning the exact CID that committed. + // The row is what every read path consults, so a response claiming accepted + // over a row that says pending would render a post the AppView believes is + // invisible. + row := f.admissionOf(t, resp.URI) + assert.Equal(t, posts.AdmissionStatusAccepted, row.Status) + require.NotNil(t, row.AcceptedCID) + assert.Equal(t, resp.CID, *row.AcceptedCID, + "the acceptance must pin the version that was actually written, not a later or earlier one") + require.NotNil(t, row.EvaluatedCID) + assert.Equal(t, resp.CID, *row.EvaluatedCID) + + // 3. AND A REAL ACCEPTANCE RECORD STANDS IN THE COMMUNITY'S REPO, at the + // deterministic rkey. Read from the PDS rather than from the row, because + // the row is the AppView quoting itself: a fast path that stamped the row + // without writing the record would leave the post visible here and invisible + // to every federated peer, which is the exact split the acceptance record + // exists to prevent. + rkey := hostedRkeyOf(resp.URI) + community := f.communityAccount(t) + acceptance := community.GetRecord(t, posts.AcceptanceCollection, rkey) + + subject, ok := acceptance.Value["subject"].(map[string]any) + require.Truef(t, ok, "the acceptance record has no subject strongRef: %#v", acceptance.Value) + assert.Equal(t, resp.URI, subject["uri"]) + assert.Equal(t, resp.CID, subject["cid"], + "an acceptance without the CID pins nothing, which is the one guarantee it exists to make") + + assert.Equal(t, []string{rkey}, listRecordKeys(t, community, posts.AcceptanceCollection), + "exactly one acceptance, at the derived key") + + // 4. The fast path was actually taken. Asserted because every other + // assertion here would also pass if the firehose had settled the row — and + // in this fixture there is no firehose, so a zero here means the test is + // proving something other than what it claims. + assert.Equal(t, 1, f.acceptor.callCount()) +} + +func TestService_CreateRetryIsTheSamePostAndCommitsNothingNew(t *testing.T) { + t.Parallel() + + // The lost-response case of §4.2, reproduced honestly: the client's first + // attempt succeeded but its answer never arrived, so it sends the identical + // submission again. Dedupe is NOT what saves it here — this fixture's ledger + // is unmetered on purpose, because dedupe is the first line of defence and + // the one that is gone in exactly the situation that matters (a reservation + // released by the failure handler, or a retry aimed at a different AppView). + // The deterministic rkey is the second, and it is the one under test. + f := newPostFixture(t) + + first := f.createPost(t, f.author.DID, "a submission whose response was lost", "body") + + authorHeadBefore := repoHead(t, f.author) + communityHeadBefore := repoHead(t, f.communityAccount(t)) + + second := f.createPost(t, f.author.DID, "a submission whose response was lost", "body") + + assert.Equal(t, first.URI, second.URI, + "a byte-identical retry must land on the SAME record key, or the author gets two posts") + assert.Equal(t, first.CID, second.CID, + "the retry must report the CID that is actually standing — a fresh CID means the record was "+ + "rewritten, and every strongRef a client built from the first response now dangles") + + assert.Equal(t, []string{rkeyOf(t, first.URI)}, + listRecordKeys(t, f.author, posts.PostV2Collection), + "the retry left a second post record in the author's repo") + + // NO NEW COMMIT, IN EITHER REPO. This is the assertion with teeth. A retry + // that re-PUT an identical record would satisfy the URI check, produce a new + // CID, and — worse — emit a second firehose commit, which every consumer + // reads as an EDIT: the admission row would move to pending_reacceptance and + // the post would drop out of the community it was already accepted into. + assert.Equal(t, authorHeadBefore, repoHead(t, f.author), + "the retry committed to the author's repo; a create-only write must report the standing "+ + "record instead of rewriting it") + assert.Equal(t, communityHeadBefore, repoHead(t, f.communityAccount(t)), + "the retry committed to the community's repo; the acceptance already pinned this CID, so the "+ + "writer had nothing to do") + + assert.Equal(t, posts.PostStatusAccepted, second.Status, + "the retry describes the state of the post that exists, which is accepted") +} + +func TestService_CreateLeavesThePostPendingInACommunityThisAppViewDoesNotHost(t *testing.T) { + t.Parallel() + + // §4.2 step 5: the author's server does not run admission for a community it + // does not host — it has no authoritative view of that community's bans, + // visibility or quotas, and a stale or hostile home server must not be able + // to fake either an admission or a rejection. So the write succeeds and the + // post waits. + f := newPostFixture(t) + remote := f.unhostedCommunity(t) + + resp, err := f.createPostIn(t, remote.DID, "submitted to a community we do not host", "body") + require.NoErrorf(t, err, "not hosting a community is not a reason to refuse its author's post — "+ + "the record belongs to the author either way") + + assert.Equal(t, posts.PostStatusPending, resp.Status) + + // The record is in the author's repo exactly as it would be for a local + // community. Nothing about the author's half of the write depends on who + // decides. + f.author.GetRecord(t, posts.PostV2Collection, rkeyOf(t, resp.URI)) + + // And NO acceptance was invented on the community's behalf. This is the + // security half of the case: an AppView that wrote an acceptance for a + // community whose keys it does not hold could not, but one that STAMPED the + // row accepted anyway would show every local reader a post the community + // never admitted. + row, err := f.admissions.Get(context.Background(), remote.DID, resp.URI) + require.NoError(t, err) + if row != nil { + assert.Equal(t, posts.AdmissionStatusPending, row.Status, + "a community we cannot write for must be left owing a decision, never marked accepted") + assert.Nil(t, row.AcceptedCID) + } +} + +func TestService_CreateSurvivesAnAcceptanceThatFails(t *testing.T) { + t.Parallel() + + // The failure mode §4.2 names by hand: "author-repo write succeeds, + // acceptance write fails → post stays pending; the firehose engine retries + // idempotently (same rkey). Degraded latency, not data loss. NEVER roll back + // the author's record." + // + // Rolling back would be the tempting repair and it is the wrong one twice + // over: the record is the AUTHOR's, not the AppView's, to withdraw — and a + // rollback whose own delete failed would leave a post nobody has a row for. + f := newPostFixture(t) + f.acceptor.fail(errors.New("the community's PDS is unreachable")) + + resp, err := f.submitPost(t, f.author.DID, "written while acceptance was broken", "body") + require.NoErrorf(t, err, "a failed acceptance must not fail the author's post — the record is "+ + "theirs and it committed") + + assert.Equal(t, posts.PostStatusPending, resp.Status, + "the response must tell the truth about a post no community has accepted yet") + + // THE AUTHOR'S RECORD STANDS. + record := f.author.GetRecord(t, posts.PostV2Collection, rkeyOf(t, resp.URI)) + assert.Equal(t, resp.CID, record.CID) + assert.Equal(t, "written while acceptance was broken", record.Value["title"]) + + // And the row is pending with the content CID recorded, which is what makes + // the firehose engine's retry possible at all: without an evaluated CID + // there is nothing for an acceptance to pin. + row := f.admissionOf(t, resp.URI) + assert.Equal(t, posts.AdmissionStatusPending, row.Status) + require.NotNil(t, row.EvaluatedCID) + assert.Equal(t, resp.CID, *row.EvaluatedCID) + + assert.Empty(t, listRecordKeys(t, f.communityAccount(t), posts.AcceptanceCollection), + "no acceptance may stand for a post the acceptance writer never managed to accept") +} + +func TestService_UpdateEditsInPlaceAndPreservesWhatIsImmutable(t *testing.T) { + t.Parallel() + + f := newPostFixture(t) + created := f.createPost(t, f.author.DID, "the original title", "the original body") + rkey := rkeyOf(t, created.URI) + + before := f.author.GetRecord(t, posts.PostV2Collection, rkey) + originalCreatedAt, ok := before.Value["createdAt"].(string) + require.True(t, ok) + + edited := "the corrected body" + updated, err := f.service.UpdatePost(context.Background(), sessionFor(t, f.author, f.pds.URL()), + posts.UpdatePostRequest{URI: created.URI, Content: &edited}) + require.NoError(t, err) + + assert.Equal(t, created.URI, updated.URI, + "an edit is the same post — a new URI would orphan every comment, vote and link to it") + assert.NotEqual(t, created.CID, updated.CID, + "the edit committed new content, so it has a new CID; the acceptance that pinned the old one "+ + "is what the engine re-decides against (§5.5)") + + after := f.author.GetRecord(t, posts.PostV2Collection, rkey) + assert.Equal(t, updated.CID, after.CID, "the reported CID must be the one that committed") + assert.Equal(t, "the corrected body", after.Value["content"]) + + // COMMUNITY AND createdAt SURVIVE THE EDIT, and both come from the STANDING + // RECORD rather than from the request. The community because the lexicon + // calls it immutable — a consumer discards an update event that changes it, + // so an edit that dropped it would be discarded entire and the post would + // freeze at its pre-edit content everywhere but here. createdAt because + // every feed orders by it: re-stamping it on an edit would jump a + // three-year-old post corrected for a typo to the top of every sort. + assert.Equal(t, f.community.DID, after.Value["community"]) + assert.Equal(t, originalCreatedAt, after.Value["createdAt"]) + + assert.NotContains(t, after.Value, "author", + "the edit reintroduced an author field the postv2 lexicon does not declare") +} + +func TestService_UpdateRefusesRetargetingTheCommunity(t *testing.T) { + t.Parallel() + + // Refused at the SERVICE, as a validation error, rather than silently + // ignored. Both leave the record's community intact, but only one tells the + // client that the thing it asked for did not happen — and a client that + // believed it had moved a post would show its author a community the post is + // not in. §3.1: retargeting means writing a NEW post record. + f := newPostFixture(t) + created := f.createPost(t, f.author.DID, "a post in one community", "body") + elsewhere := f.unhostedCommunity(t) + + body := "still here" + _, err := f.service.UpdatePost(context.Background(), sessionFor(t, f.author, f.pds.URL()), + posts.UpdatePostRequest{URI: created.URI, Content: &body, Community: elsewhere.DID}) + require.Error(t, err) + assert.Truef(t, posts.IsValidationError(err), + "expected a validation error the handler turns into a 400 naming the field, got: %v", err) + + after := f.author.GetRecord(t, posts.PostV2Collection, rkeyOf(t, created.URI)) + assert.Equal(t, f.community.DID, after.Value["community"]) + assert.Equal(t, "body", after.Value["content"], + "the refused update applied its content change anyway — a refusal must be total") +} + +func TestService_UpdateRefusesAConcurrentEdit(t *testing.T) { + t.Parallel() + + // The read-then-write window. UpdatePost reads the standing record to + // preserve community and createdAt, then writes the edit guarded by the CID + // it read. A second device's edit landing in between must make this one FAIL + // rather than overwrite it: the edit was composed against content that no + // longer stands, and silently re-applying it would erase a change its author + // never saw. + f := newPostFixture(t) + created := f.createPost(t, f.author.DID, "edited from two devices", "the original body") + rkey := rkeyOf(t, created.URI) + + f.authorRepos.raceAfterRead(f.author.DID, func() { + f.author.PutRecord(t, posts.PostV2Collection, rkey, map[string]any{ + "$type": posts.PostV2Collection, + "community": f.community.DID, + "title": "edited from two devices", + "content": "the other device got there first", + "createdAt": "2026-07-01T12:00:00Z", + }) + }) + + body := "the edit that was composed against stale content" + _, err := f.service.UpdatePost(context.Background(), sessionFor(t, f.author, f.pds.URL()), + posts.UpdatePostRequest{URI: created.URI, Content: &body}) + require.Error(t, err) + assert.ErrorIsf(t, err, posts.ErrConcurrentModification, + "the boundary maps this to a 409 so the client can re-read and decide; got: %v", err) + + after := f.author.GetRecord(t, posts.PostV2Collection, rkey) + assert.Equal(t, "the other device got there first", after.Value["content"], + "the losing edit overwrote the winner, which is the whole failure the swap guard exists to stop") +} + +func TestService_UpdateRefusesEveryoneButTheAuthor(t *testing.T) { + t.Parallel() + + f := newPostFixture(t) + created := f.createPost(t, f.author.DID, "the author's post", "body") + attacker := f.authorRepos.register(f.pds.CreateAccount(t, testkit.WithHandlePrefix("aup"))) + + body := "an edit by someone who does not own this repo" + _, err := f.service.UpdatePost(context.Background(), sessionFor(t, attacker, f.pds.URL()), + posts.UpdatePostRequest{URI: created.URI, Content: &body}) + assert.ErrorIs(t, err, posts.ErrNotAuthorized) + + after := f.author.GetRecord(t, posts.PostV2Collection, rkeyOf(t, created.URI)) + assert.Equal(t, "body", after.Value["content"]) +} + +func TestService_UpdateLeavesTheSubmissionLedgerUntouched(t *testing.T) { + t.Parallel() + + // AN EDIT IS NOT A SUBMISSION, and the ledger is where that has to be true. + // + // The ledger is both the dedupe gate and the per-author quota (§8). If an + // edit wrote to it, two things break at once: an author who fixed a typo + // would spend a quota slot for it, and — because the edit's fingerprint is + // the EDITED content — resubmitting that same content later would be refused + // as a duplicate of an edit rather than of a post. + // + // The pin is the mirror image: after an edit, the ORIGINAL content is still + // on the ledger, so resubmitting it inside the window is still a duplicate. + // An implementation that moved or rewrote the ledger row would admit it. + f := newLedgerFixture(t) + + created := f.submit(t, "a post that will be edited", "the original body") + require.Equal(t, 1, f.ledgerRows(t)) + + edited := "the corrected body" + _, err := f.service.UpdatePost(context.Background(), sessionFor(t, f.base.author, f.base.pds.URL()), + posts.UpdatePostRequest{URI: created.URI, Content: &edited}) + require.NoError(t, err) + + assert.Equal(t, 1, f.ledgerRows(t), + "the edit wrote to the submission ledger; an edit consumes no quota and creates no dedupe key") + + _, err = f.submitErr(t, "a post that will be edited", "the original body") + assert.ErrorIsf(t, err, posts.ErrDuplicateSubmission, + "the ORIGINAL submission's ledger row must survive the edit — an edit that overwrote it with "+ + "the edited fingerprint would let the original content be posted a second time; got: %v", err) + + // And the refused resubmission changed nothing about the post that exists. + after := f.base.author.GetRecord(t, posts.PostV2Collection, rkeyOf(t, created.URI)) + assert.Equal(t, "the corrected body", after.Value["content"]) + assert.Equal(t, 1, f.ledgerRows(t), "a refused submission consumes no quota (§8)") +} + +// --------------------------------------------------------------------------- +// Fixtures +// --------------------------------------------------------------------------- + +// unhostedCommunity provisions a community and then takes away the one thing +// that makes it ours: the stored refresh token. +// +// That is the honest way to build one. Hosting is credential presence and +// nothing else — NewCommunityRepoFactory refuses to consult hosted_by_did, +// because that column is populated from a profile record anyone can write — so a +// community with no stored refresh token is precisely what a community hosted by +// another instance looks like to this code. +func (f *postFixture) unhostedCommunity(t *testing.T) *communities.Community { + t.Helper() + + name := testkit.UniqueIDWithPrefix(t, "uh") + require.LessOrEqual(t, len("c-"+name), testkit.MaxIDLength) + + community, err := f.communityService.CreateCommunity(context.Background(), communities.CreateCommunityRequest{ + Name: name, + DisplayName: "Hosted elsewhere", + Description: "a community whose credentials this AppView does not hold", + Visibility: "public", + CreatedByDID: f.author.DID, + }) + require.NoError(t, err) + + result, err := f.db.ExecContext(context.Background(), + `UPDATE communities SET pds_refresh_token_encrypted = NULL WHERE did = $1`, community.DID) + require.NoError(t, err) + affected, err := result.RowsAffected() + require.NoError(t, err) + require.Equalf(t, int64(1), affected, + "the fixture cleared no refresh token for %s, so the community is still hosted and the test "+ + "would prove nothing", community.DID) + + return community +} + +// createPostIn submits to a named community rather than the fixture's own. +func (f *postFixture) createPostIn(t *testing.T, communityDID, title, content string) (*posts.CreatePostResponse, error) { + t.Helper() + return f.service.CreatePost( + middleware.SetTestUserDID(context.Background(), f.author.DID), + sessionFor(t, f.author, f.pds.URL()), + posts.CreatePostRequest{ + Community: communityDID, + Title: &title, + Content: &content, + AuthorDID: f.author.DID, + }) +} + +// ledgerFixture is the write path over a REAL submission ledger, which the +// default fixture deliberately opts out of. +type ledgerFixture struct { + base *postFixture + service posts.Service +} + +func newLedgerFixture(t *testing.T) *ledgerFixture { + t.Helper() + + base := newPostFixture(t) + return &ledgerFixture{ + base: base, + service: posts.NewPostService( + postgres.NewPostRepository(base.db), base.communityService, + nil, nil, nil, nil, base.pds.URL(), + append(base.writePathOptions(), + posts.WithAdmissionPolicy(posts.AdmissionPolicy{ + Ledger: postgres.NewSubmissionLedger(base.db), + Bans: base.communityService, + Limits: posts.SubmissionLimits{ + MaxPerAuthorPerCommunity: 10, + Window: time.Hour, + DedupeWindow: time.Hour, + }, + // The real clock: this fixture never crosses a window, and + // the ledger stamps created_at server-side, so an injected + // clock would only introduce disagreement between the two. + Now: time.Now, + }))...), + } +} + +func (f *ledgerFixture) submit(t *testing.T, title, content string) *posts.CreatePostResponse { + t.Helper() + resp, err := f.submitErr(t, title, content) + require.NoError(t, err) + return resp +} + +func (f *ledgerFixture) submitErr(t *testing.T, title, content string) (*posts.CreatePostResponse, error) { + t.Helper() + return f.service.CreatePost( + middleware.SetTestUserDID(context.Background(), f.base.author.DID), + sessionFor(t, f.base.author, f.base.pds.URL()), + posts.CreatePostRequest{ + Community: f.base.community.DID, + Title: &title, + Content: &content, + AuthorDID: f.base.author.DID, + }) +} + +func (f *ledgerFixture) ledgerRows(t *testing.T) int { + t.Helper() + return countSubmissions(t, f.base.db, f.base.author.DID, f.base.community.DID) +} diff --git a/internal/core/posts/service_writeforward_test.go b/internal/core/posts/service_writeforward_test.go --- a/internal/core/posts/service_writeforward_test.go +++ b/internal/core/posts/service_writeforward_test.go @@ -5,8 +5,10 @@ import ( "context" "database/sql" - + "fmt" + "sync" "testing" + "time" "Coves/internal/api/middleware" "Coves/internal/atproto/pds" @@ -21,8 +23,8 @@ "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -// What creating and deleting a post actually does to the community's repo, and -// who is allowed to do it. +// What creating and deleting a post actually does to a repository, and who is +// allowed to do it. // // This is the client-write half of the post domain: the half that // tests/e2e/post_contract_test.go structurally cannot reach. §3.4b of @@ -30,27 +32,36 @@ // docs/TEST_ARCHITECTURE.md records why — RequireAuth accepts only a sealed // session token, minted nowhere but the browser OAuth callback, so T2 can prove // that the write endpoints refuse an unauthenticated client and nothing beyond // it. Authenticated write BEHAVIOUR is therefore proven here, against a real -// PDS, and these are the assertions that used to live in -// tests/integration/post_e2e_test.go's "Write-Forward to PDS" and -// tests/integration/post_delete_test.go's authorization tests. +// PDS. +// +// # THE REPO MOVED, AND THAT IS WHAT THIS FILE IS NOW ABOUT +// +// Until task 6 a post record did NOT live in its author's repo. It lived in the +// COMMUNITY's, written with the community's own PDS credentials, carrying an +// `author` field that named the human who wrote it. Every assertion in this file +// used to pin that arrangement in place. // -// # WHAT MAKES THE REPO THE INTERESTING PART +// docs/PRD_AUTHOR_OWNED_POSTS.md §3.1 and §4.2 reverse it. A post is a +// social.coves.community.postv2 record in the AUTHOR's repository, signed with +// the author's own credentials, with NO author field at all — authorship is the +// repository, which a verifying relay or a DID-resolved fetch can check, rather +// than a claim one party signed about another. The community's answer is a +// separate record it publishes itself (§3.2). // -// A post record does not live in its author's repo. It lives in the COMMUNITY's -// repo, written with the community's own PDS credentials, carrying an `author` -// field that names the human who wrote it (internal/core/posts/service.go step -// 8, and the reason the Jetstream consumer's first security check is -// repoDID == record.community). +// That reversal is why the assertions here read the way they do: // -// That makes two things testable only from the PDS side. First, that the -// service really writes into the community's repository rather than the -// caller's — a post written to the author's repo would be rejected outright by -// the consumer, so the AppView would simply never index it and every test that -// only reads the service's return value would still pass. Second, that deletion -// is authorized against the RECORD's author field rather than against anything -// the caller supplies: DeletePost fetches the record from the PDS specifically -// to read `author` out of it, because the community's credentials would happily -// delete anyone's post. +// - The record's location is still the most consequential fact about it, so it +// is still asserted from the PDS side rather than from the service's return +// value. What changed is which repo has to hold it — and, just as load- +// bearing, which repo must NOT. +// - DELETE AUTHORIZATION IS NOW A LOCAL DECISION. It used to require fetching +// the record to read its `author` field, because the delete went out on the +// COMMUNITY's credentials and that field was the only thing standing between +// an attacker and someone else's post. A postv2 record is in the author's own +// repo, so the URI's authority IS the owner: the check is the session DID +// against the URI, decided before anything is fetched, and the credentials +// the delete goes out on cannot reach another author's repo even if the check +// were wrong. const ( // The instance identity these tests provision communities under. It matches @@ -60,17 +71,21 @@ // PDS_SERVICE_HANDLE_DOMAINS or account creation is refused. instanceDID = "did:web:coves.social" instanceDomain = "coves.social" + // postCollection is the DEPRECATED community-repo collection. It survives in + // this file only where a test is about the pre-flip records still standing + // in production repos — task 8 re-materializes them and retires it. postCollection = "social.coves.community.post" ) -// postFixture is the post service wired the way cmd/server wires it, over a -// real community that owns a real PDS repository. +// postFixture is the post service wired the way cmd/server wires it, over a real +// community that owns a real PDS repository and a real author who owns another. // // db and communityService are the provisioned halves of that wiring, kept so a // neighbouring test can build a differently-wired post service over the SAME -// community rather than provision a second one — see -// service_aggregator_test.go, which needs the aggregator collaborator this -// fixture deliberately leaves nil. +// community rather than provision a second one — see service_aggregator_test.go +// and service_admission_test.go, which need collaborators this fixture +// deliberately leaves nil. Those neighbours reuse authorRepos and the acceptance +// wiring through writePathOptions. type postFixture struct { service posts.Service pds *testkit.PDS @@ -78,18 +93,192 @@ db *sql.DB communityService communities.Service community *communities.Community author *testkit.Account + + // admissions is the real table the fast path seeds and the engine settles. + admissions posts.AdmissionRepository + + // acceptor is the real engine behind a switch a test can throw, so the + // "acceptance failed" branch can be reached without breaking the PDS. + acceptor *acceptorSpy + + // authorRepos hands out credentials per author DID — the seam that replaces + // the community's service token on the write path. + authorRepos *authorRepoRegistry } -// newPostFixture provisions a community on the test PDS and returns the post -// service pointed at it. +// authorRepoRegistry is the integration stand-in for the production +// AuthorRepoFactory. +// +// Production resolves an author's OAuth session (or an aggregator's stored +// tokens) and builds a DPoP client; here every author is a real PDS account with +// a password session, which is the same substitution comments' PDSClientFactory +// makes (testkit.PasswordAuthFactory) and for the same reason: OAuth's browser +// callback cannot be driven from a Go test, and what is under test is which repo +// the write lands in, not how the token was obtained. +// +// An author it does not know answers ErrNoAuthorCredentials, which is exactly +// what production answers for an aggregator whose stored session is gone. +type authorRepoRegistry struct { + pds *testkit.PDS + + mu sync.Mutex + accounts map[string]*testkit.Account + + // afterRead runs immediately after a repo read, per author DID. It is the + // only way to open the read-then-write window an update's swap guard exists + // to close: a competing commit has to land BETWEEN the service's pre-read + // and its put, and no amount of ordering from outside the service can + // arrange that. engine_contract_test.go forces a lost swapRecord race the + // same way. + afterRead map[string]func() + + // hosts overrides the PDS an author's repo client talks to. It is how a + // write FAILURE is injected now: the write goes to the AUTHOR's repo, so + // pointing the community's stored pds_url at a broken server — which is how + // the reservation-release tests used to do it — breaks nothing the write + // path touches any more. + hosts map[string]string +} + +func newAuthorRepoRegistry(pdsServer *testkit.PDS) *authorRepoRegistry { + return &authorRepoRegistry{ + pds: pdsServer, + accounts: map[string]*testkit.Account{}, + afterRead: map[string]func(){}, + hosts: map[string]string{}, + } +} + +// pointAt sends an author's repo writes to host instead of the test PDS. An +// empty host restores the real one. +func (r *authorRepoRegistry) pointAt(authorDID, host string) { + r.mu.Lock() + defer r.mu.Unlock() + + if host == "" { + delete(r.hosts, authorDID) + return + } + r.hosts[authorDID] = host +} + +// raceAfterRead arranges for fn to run once, right after the next read of +// authorDID's repo, so the service's next write meets a record that changed +// under it. +func (r *authorRepoRegistry) raceAfterRead(authorDID string, fn func()) { + r.mu.Lock() + defer r.mu.Unlock() + + var once sync.Once + r.afterRead[authorDID] = func() { once.Do(fn) } +} + +// racingAuthorRepo is an author repo that lets a competing write land between a +// read and the write shaped from it. +type racingAuthorRepo struct { + posts.AuthorRepo + afterRead func() +} + +func (r *racingAuthorRepo) GetRecord(ctx context.Context, collection, rkey string) (*pds.RecordResponse, error) { + record, err := r.AuthorRepo.GetRecord(ctx, collection, rkey) + if r.afterRead != nil { + r.afterRead() + } + return record, err +} + +// register makes an account's repo reachable by its DID. +func (r *authorRepoRegistry) register(account *testkit.Account) *testkit.Account { + r.mu.Lock() + defer r.mu.Unlock() + r.accounts[account.DID] = account + return account +} + +// factory is the AuthorRepoFactory the post service is wired with. +func (r *authorRepoRegistry) factory() posts.AuthorRepoFactory { + return func(_ context.Context, authorDID string, _ *oauth.ClientSessionData) (posts.AuthorRepo, error) { + r.mu.Lock() + account := r.accounts[authorDID] + race := r.afterRead[authorDID] + host := r.hosts[authorDID] + r.mu.Unlock() + + if account == nil { + return nil, fmt.Errorf("%w: no session is stored for %s", posts.ErrNoAuthorCredentials, authorDID) + } + + if host == "" { + host = r.pds.URL() + } + client, err := pds.NewFromAccessToken(host, account.DID, account.AccessToken) + if err != nil { + return nil, fmt.Errorf("opening the repo of %s: %w", authorDID, err) + } + repo, ok := client.(posts.AuthorRepo) + if !ok { + return nil, fmt.Errorf("opening the repo of %s: the PDS client does not implement the "+ + "author-repo write surface (guarded put + commit rev)", authorDID) + } + if race != nil { + return &racingAuthorRepo{AuthorRepo: repo, afterRead: race}, nil + } + return repo, nil + } +} + +// acceptorSpy delegates to the real acceptance engine, or fails on demand. +// +// The failure switch is not a convenience. The whole promise of the fast path is +// that its failure is INVISIBLE to the author — the record stands, the response +// succeeds, the post is merely pending — and a fixture that could not make the +// acceptance fail could not prove any of that. Breaking the PDS instead would +// break the author-repo write too, which is the case this is trying to hold +// still. +type acceptorSpy struct { + delegate posts.SubmissionAcceptor + + mu sync.Mutex + failWith error + calls int +} + +func (s *acceptorSpy) AcceptSubmission(ctx context.Context, communityDID, postURI, postCID string) (posts.EngineOutcome, error) { + s.mu.Lock() + s.calls++ + failure := s.failWith + s.mu.Unlock() + + if failure != nil { + return posts.EngineDeferred, failure + } + return s.delegate.AcceptSubmission(ctx, communityDID, postURI, postCID) +} + +// failWith makes every subsequent acceptance fail with err. +func (s *acceptorSpy) fail(err error) { + s.mu.Lock() + defer s.mu.Unlock() + s.failWith = err +} + +// callCount reports how many times the write path reached for the fast path. +func (s *acceptorSpy) callCount() int { + s.mu.Lock() + defer s.mu.Unlock() + return s.calls +} + +// newPostFixture provisions a community and an author on the test PDS and +// returns the post service pointed at both. // // The community is provisioned through communities.CreateCommunity rather than -// seeded into the index, because unlike the subscribe/block write-forwards this -// is a write into the COMMUNITY's repo: it needs an account that exists on the -// PDS and credentials the AppView can use, and both are what provisioning -// produces. The optional collaborators (aggregators, blobs, unfurl, bluesky) -// are nil — every one of them is a branch on the record's contents, and what is -// under test here is where the record lands. +// seeded into the index, because the community still owns a real repo: the +// ACCEPTANCE goes there, written with the credentials provisioning produces. The +// optional content collaborators (aggregators, blobs, unfurl, bluesky) are nil — +// every one of them is a branch on the record's contents, and what is under test +// here is where the record lands. func newPostFixture(t *testing.T) *postFixture { t.Helper() @@ -107,7 +296,8 @@ testkit.PasswordAuthFactory(pds.NewFromAccessToken), nil, ) - author := pdsServer.CreateAccount(t, testkit.WithHandlePrefix("pa")) + authorRepos := newAuthorRepoRegistry(pdsServer) + author := authorRepos.register(pdsServer.CreateAccount(t, testkit.WithHandlePrefix("pa"))) name := testkit.UniqueIDWithPrefix(t, "pw") require.LessOrEqualf(t, len("c-"+name), testkit.MaxIDLength, @@ -116,28 +306,67 @@ community, err := communityService.CreateCommunity(context.Background(), communities.CreateCommunityRequest{ Name: name, DisplayName: "Write forward", - Description: "a community whose repo receives posts", + Description: "a community whose repo receives acceptances", Visibility: "public", CreatedByDID: author.DID, }) require.NoError(t, err) - return &postFixture{ - service: posts.NewPostService( - postgres.NewPostRepository(db), communityService, - nil, nil, nil, nil, pdsServer.URL(), - // Write-forward is not about admission; the opt-out is explicit. - posts.WithAdmissionPolicy(posts.NewAllowAllAdmissionPolicyForTests())), + // The real engine, over the real production community-repo factory. Nothing + // here is a double: the factory's hosting test is credential presence, which + // is what the unhosted case in service_writeflip_test.go turns off. + admissions := postgres.NewAdmissionRepository(db) + engine := posts.NewAcceptanceEngine( + admissions, + // The fast path does not re-decide — CreatePost has already run + // admission — so the decider is present only to satisfy the engine's + // constructor. A decider that ADMITTED here would hide a fast path that + // wrongly re-ran policy; one that refused would make every acceptance + // fail. It is scripted to refuse, so a fast path that consulted it at + // all fails these tests loudly. + &scriptedDecider{code: posts.DecisionRuleViolation}, + posts.NewCommunityRecordWriter(posts.NewCommunityRepoFactory(communityService), time.Now), + posts.NewCommunityCredentialRefresher(communityService), + ) + acceptor := &acceptorSpy{delegate: engine} + + f := &postFixture{ pds: pdsServer, db: db, communityService: communityService, community: community, author: author, + admissions: admissions, + acceptor: acceptor, + authorRepos: authorRepos, } + f.service = posts.NewPostService( + postgres.NewPostRepository(db), communityService, + nil, nil, nil, nil, pdsServer.URL(), + append(f.writePathOptions(), + // Write-forward is not about admission; the opt-out is explicit. + posts.WithAdmissionPolicy(posts.NewAllowAllAdmissionPolicyForTests()))...) + return f } -// communityAccount returns a session on the community's own repo, which is how -// a test reads back what the service wrote there. +// writePathOptions are the collaborators every post service in this package +// needs now that a post is written to its author's repo: the credential seam and +// the local-community fast path. +// +// Exposed so the neighbouring fixtures (admission, aggregator) wire the SAME +// author registry and the SAME engine as this one. They used to need neither, +// because a post was written with the community's service token that +// CreateCommunity had already produced. +func (f *postFixture) writePathOptions() []posts.PostServiceOption { + return []posts.PostServiceOption{ + posts.WithAuthorRepoFactory(f.authorRepos.factory()), + posts.WithSyncAcceptance(f.admissions, f.acceptor), + } +} + +// communityAccount returns a session on the community's own repo, which is how a +// test reads back the acceptance the service wrote there — and how it proves the +// community's repo did NOT receive a post. func (f *postFixture) communityAccount(t *testing.T) *testkit.Account { t.Helper() return f.pds.Login(t, f.community.Handle, f.community.PDSPassword) @@ -149,23 +378,56 @@ // (defence-in-depth against a bypassed handler), so a test that sets only one // exercises the mismatch guard by accident. func (f *postFixture) createPost(t *testing.T, authorDID, title, content string) *posts.CreatePostResponse { t.Helper() - resp, err := f.service.CreatePost( + resp, err := f.submitPost(t, authorDID, title, content) + require.NoError(t, err) + require.NotEmpty(t, resp.URI) + require.NotEmpty(t, resp.CID) + return resp +} + +// submitPost is createPost without the success requirement, for the cases that +// are about what a failure leaves behind. +func (f *postFixture) submitPost(t *testing.T, authorDID, title, content string) (*posts.CreatePostResponse, error) { + t.Helper() + return f.service.CreatePost( middleware.SetTestUserDID(context.Background(), authorDID), + f.sessionForDID(t, authorDID), posts.CreatePostRequest{ Community: f.community.DID, Title: &title, Content: &content, AuthorDID: authorDID, }) - require.NoError(t, err) - require.NotEmpty(t, resp.URI) - require.NotEmpty(t, resp.CID) - return resp } -// sessionFor builds the OAuth session shape DeletePost takes. Only the DID is -// load-bearing: the delete itself goes out on the COMMUNITY's credentials, and -// the session exists to say who is asking. +// sessionForDID builds the session of a registered author, or a session with no +// repo behind it for a DID the registry has never heard of. +func (f *postFixture) sessionForDID(t *testing.T, did string) *oauth.ClientSessionData { + t.Helper() + + f.authorRepos.mu.Lock() + account := f.authorRepos.accounts[did] + f.authorRepos.mu.Unlock() + + if account == nil { + parsed, err := syntax.ParseDID(did) + require.NoError(t, err) + return &oauth.ClientSessionData{ + AccountDID: parsed, + SessionID: "post-write-flip-test", + HostURL: f.pds.URL(), + } + } + return sessionFor(t, account, f.pds.URL()) +} + +// authorAccount returns the fixture author's own PDS session, which is how a +// test reads back what the service wrote into their repo. +func (f *postFixture) authorAccount() *testkit.Account { return f.author } + +// sessionFor builds the OAuth session shape the write endpoints take. The DID is +// what says who is asking; the account behind it is what the author-repo factory +// resolves into credentials. func sessionFor(t *testing.T, account *testkit.Account, hostURL string) *oauth.ClientSessionData { t.Helper() did, err := syntax.ParseDID(account.DID) @@ -188,21 +450,29 @@ require.NotEmptyf(t, rkey, "the record URI %q has no record key", uri) return rkey } -func TestService_CreateWritesThePostIntoTheCommunityRepo(t *testing.T) { +func TestService_CreateWritesThePostIntoTheAuthorRepo(t *testing.T) { t.Parallel() f := newPostFixture(t) resp := f.createPost(t, f.author.DID, "write-forward title", "write-forward body") - // The authority of the URI is the community, not the author. This is the - // single most consequential fact about a post record's location: the - // consumer rejects any post whose repo DID differs from its community - // field, so a service that wrote to the author's repo would produce posts - // that never index, with no error anywhere on the write path. + // THE AUTHORITY OF THE URI IS THE AUTHOR. This is the single most + // consequential fact about where a post record lives, and it is the exact + // reverse of what this assertion said before the flip. A service that still + // wrote into the community's repo would produce records the postv2 consumer + // attributes to the community — every post in an instance authored by the + // community itself — with no error anywhere on the write path. rkey := rkeyOf(t, resp.URI) - assert.Equal(t, "at://"+f.community.DID+"/"+postCollection+"/"+rkey, resp.URI) + assert.Equal(t, "at://"+f.author.DID+"/"+posts.PostV2Collection+"/"+rkey, resp.URI) + + // And the rkey is a TID, because the lexicon declares `key: tid`. It is + // derived rather than minted (§4.2), which is what the retry case below + // proves; that it is WELL-FORMED is asserted here because a PDS running a + // stricter build refuses anything else outright. + _, err := syntax.ParseTID(rkey) + require.NoErrorf(t, err, "the post's record key %q is not a TID", rkey) - record := f.communityAccount(t).GetRecord(t, postCollection, rkey) + record := f.authorAccount().GetRecord(t, posts.PostV2Collection, rkey) // The CID the service reported is the CID of the record that actually // committed. Worth asserting rather than merely checking it is non-empty: @@ -213,17 +483,30 @@ // write path would notice. assert.Equal(t, record.CID, resp.CID, "the CID returned to the client must be the committed record's") - assert.Equal(t, postCollection, record.Value["$type"]) + assert.Equal(t, posts.PostV2Collection, record.Value["$type"]) assert.Equal(t, f.community.DID, record.Value["community"], - "the record's community field must match the repo it lives in, or the consumer rejects it as a spoof") - assert.Equal(t, f.author.DID, record.Value["author"], - "posts live in the community's repo but belong to their author, and this field is the only thing that says so") + "the record names the community it was submitted to, as a DID — the client may have typed a handle") assert.Equal(t, "write-forward title", record.Value["title"]) assert.Equal(t, "write-forward body", record.Value["content"]) assert.NotEmpty(t, record.Value["createdAt"]) + + // NO AUTHOR FIELD. The repository is the attribution now; a record carrying + // the field as well would give consumers two answers to one question, one of + // them unverifiable, with no rule for which wins. + assert.NotContains(t, record.Value, "author", + "a postv2 record must not carry an author field — authorship is the repo it lives in") + + // AND THE COMMUNITY'S REPO RECEIVED NO POST. Asserted by listing rather than + // by a single get, because a service that wrote to BOTH repos would satisfy + // every assertion above while doubling every post in the network. + assert.Emptyf(t, listRecordKeys(t, f.communityAccount(t), postCollection), + "the community's repo holds a %s record; posts belong to their authors now", postCollection) + assert.Emptyf(t, listRecordKeys(t, f.communityAccount(t), posts.PostV2Collection), + "the community's repo holds a %s record; the post belongs in the AUTHOR's repo", + posts.PostV2Collection) } -func TestService_DeleteRemovesTheRecordFromTheCommunityRepo(t *testing.T) { +func TestService_DeleteRemovesTheRecordFromTheAuthorRepo(t *testing.T) { t.Parallel() f := newPostFixture(t) @@ -231,26 +514,33 @@ ctx := context.Background() resp := f.createPost(t, f.author.DID, "to be deleted", "body") rkey := rkeyOf(t, resp.URI) + // Asserted before the delete, and not merely for symmetry with its + // neighbour: without it this test would pass against the PRE-FLIP write + // path, which puts the record in the community's repo — the author's repo + // would then be empty for the whole test, and the absence checks below would + // hold trivially. The delete has to be shown removing something. + require.Equal(t, "at://"+f.author.DID+"/"+posts.PostV2Collection+"/"+rkey, resp.URI) + f.author.GetRecord(t, posts.PostV2Collection, rkey) + require.NoError(t, f.service.DeletePost(ctx, sessionFor(t, f.author, f.pds.URL()), posts.DeletePostRequest{URI: resp.URI})) - community := f.communityAccount(t) - assert.True(t, testkit.IsNotFound(getRecordErr(ctx, community, postCollection, rkey)), - "the post record is still in the community's repo after its author deleted it") + assert.True(t, testkit.IsNotFound(getRecordErr(ctx, f.author, posts.PostV2Collection, rkey)), + "the post record is still in the author's repo after they deleted it") - // The idempotent-delete path is real now: the PDS answers a missing record - // with HTTP 400 named RecordNotFound, and the client's name-before-status - // mapping turns that into pds.ErrNotFound, so DeletePost's not-found branch - // is reachable. Previously pinned as a known defect (p3 from the - // test-refactor loop); fixed by task 4's PDS error mapping. + // The idempotent-delete path is real: the PDS answers a missing record with + // HTTP 400 named RecordNotFound, and the client's name-before-status mapping + // turns that into pds.ErrNotFound, so DeletePost's not-found branch is + // reachable. Previously pinned as a known defect (p3 from the test-refactor + // loop); fixed by task 4's PDS error mapping. assert.NoError(t, f.service.DeletePost(ctx, sessionFor(t, f.author, f.pds.URL()), posts.DeletePostRequest{URI: resp.URI}), "a repeated delete is idempotent — the retried delete after a lost response succeeds") - // And idempotent means the record STAYED gone: a second delete that - // somehow resurrected or re-wrote the record would also return success, - // so the absence has to be re-asserted, not assumed. - assert.True(t, testkit.IsNotFound(getRecordErr(ctx, community, postCollection, rkey)), + // And idempotent means the record STAYED gone: a second delete that somehow + // resurrected or re-wrote the record would also return success, so the + // absence has to be re-asserted, not assumed. + assert.True(t, testkit.IsNotFound(getRecordErr(ctx, f.author, posts.PostV2Collection, rkey)), "the record must still be absent after the idempotent re-delete") } @@ -262,11 +552,20 @@ ctx := context.Background() resp := f.createPost(t, f.author.DID, "the author's post", "body") rkey := rkeyOf(t, resp.URI) - // The attacker is a fully legitimate account with a real session. What they - // do not have is the record's author field — and since the delete goes out - // on the COMMUNITY's credentials rather than on theirs, that field is the - // ONLY thing standing between them and deleting someone else's post. - attacker := f.pds.CreateAccount(t, testkit.WithHandlePrefix("atk")) + // The attacker is a fully legitimate account with a real session and a real + // repo of their own. What they do not own is the repo the URI names. + // + // THE RATIONALE CHANGED WITH THE REPO. Before the flip, the delete went out + // on the COMMUNITY's credentials — which could delete anyone's post — so the + // record's `author` field was the only thing standing between an attacker + // and someone else's post, and it had to be fetched to be checked. Now the + // URI's authority IS the owner: the refusal is decided locally, before + // anything is fetched, and the credentials the delete would go out on cannot + // reach the author's repo in the first place. The check is defence in depth + // over a boundary the PDS already enforces — which is exactly why it must be + // proven to exist, since removing it would look harmless right up until an + // author-supplied repo DID reached the factory. + attacker := f.authorRepos.register(f.pds.CreateAccount(t, testkit.WithHandlePrefix("atk"))) err := f.service.DeletePost(ctx, sessionFor(t, attacker, f.pds.URL()), posts.DeletePostRequest{URI: resp.URI}) @@ -274,12 +573,40 @@ require.ErrorIs(t, err, posts.ErrNotAuthorized, "a user who is not the post's author must be refused") // And refused means refused: the record is still there. - community := f.communityAccount(t) - record := community.GetRecord(t, postCollection, rkey) - assert.Equal(t, f.author.DID, record.Value["author"], + record := f.authorAccount().GetRecord(t, posts.PostV2Collection, rkey) + assert.Equal(t, f.community.DID, record.Value["community"], "the rejected delete removed the record anyway") } +func TestService_DeleteRefusesAURIWhoseAuthorityIsNotTheCaller(t *testing.T) { + t.Parallel() + + // This REPLACES the old "an unknown community is reported as not found". + // + // That test existed because DeletePost's first act was to look the URI's + // authority up as a COMMUNITY, so an authority nobody had indexed was a 404 + // and the sentinel identity was the contract. A postv2 URI's authority is an + // AUTHOR, and there is no lookup to miss: the only question is whether it is + // the caller's own DID. An authority that is not is refused as + // unauthorized — never as "not found", which would tell an attacker that + // the DID they aimed at is one the AppView has never seen, and would answer + // 404 to a probe that deserves 403. + f := newPostFixture(t) + + // A well-formed DID that owns nothing here. It is a literal rather than a + // generated one: did:plc identifiers are 24 base32 characters (a-z, 2-7), + // which UniqueID does not promise, and a DID that failed the FORMAT check + // would take the validation path instead of the authorization path under + // test. Spelled at the full 24 characters deliberately — validateDIDFormat + // checks the character set but not the length, so a short one would start + // failing differently the day that omission is corrected. + uri := "at://did:plc:aaaaaaaasomeoneelsesrepo/" + posts.PostV2Collection + "/3lrc77gmww4nc" + + err := f.service.DeletePost(context.Background(), sessionFor(t, f.author, f.pds.URL()), + posts.DeletePostRequest{URI: uri}) + assert.ErrorIs(t, err, posts.ErrNotAuthorized) +} + func TestService_DeleteRejectsMalformedRequests(t *testing.T) { t.Parallel() @@ -292,14 +619,22 @@ // case is a client mistake, and each must be answerable without the PDS // having been asked anything — a validation error, not a 500 from a failed // fetch of a nonsense URI. for _, tc := range []struct { - name string session *oauth.ClientSessionData + name string uri string }{ - {name: "no session", session: nil, uri: "at://" + f.community.DID + "/" + postCollection + "/abc"}, + {name: "no session", session: nil, uri: "at://" + f.author.DID + "/" + posts.PostV2Collection + "/3lrc77gmww4nc"}, {name: "empty URI", session: session, uri: ""}, {name: "not an AT-URI", session: session, uri: "invalid-uri-format"}, - {name: "wrong collection", session: session, uri: "at://" + f.community.DID + "/social.coves.community.comment/abc"}, + { + // A collection that is neither post collection. The old spelling of + // this case used the DEPRECATED community.post NSID, because delete + // was narrowed to postv2's predecessor and postv2 itself was refused; + // both collections are accepted now (see the next test), so the + // malformed case has to be something that genuinely is not a post. + name: "a collection that is not a post at all", session: session, + uri: "at://" + f.author.DID + "/social.coves.community.comment/3lrc77gmww4nc", + }, } { t.Run(tc.name, func(t *testing.T) { t.Parallel() @@ -311,32 +646,59 @@ }) } } -func TestService_DeleteReportsAnUnknownCommunityAsNotFound(t *testing.T) { +func TestService_DeleteStillReachesPreFlipPostsInTheCommunityRepo(t *testing.T) { t.Parallel() + // The other half of the adjudicated wrong-collection row: BOTH collections + // are accepted, and the deprecated one still takes the OLD path. + // + // This is not backwards compatibility for its own sake. Every post written + // before the flip is a social.coves.community.post record standing in a + // community's repo right now, and its author's delete button has to keep + // working until task 8 re-materializes them. The two paths are genuinely + // different — this one authenticates as the COMMUNITY and reads the record's + // `author` field, because the caller has no credentials on that repo at all — + // which is precisely why refusing the collection outright is not an option + // and why the routing needs a test that watches the record disappear. f := newPostFixture(t) + ctx := context.Background() - // A well-formed URI whose authority is a community the AppView has never - // indexed. The distinction from the validation errors above is what the - // handler does with it — 404 rather than 400 — so the sentinel identity is - // the assertion, not merely that an error came back. - // - // The DID is a literal rather than a generated one: did:plc identifiers are - // 24 base32 characters (a-z, 2-7), which UniqueID does not promise, and a DID - // that fails the FORMAT check would take the validation path above instead of - // the lookup path under test. Uniqueness is not needed — the database is a - // per-test clone in which nothing has ever been indexed. - // - // It is spelled at the full 24 characters deliberately. validateDIDFormat - // (service.go) checks the CHARACTER SET but not the length, so a 23-character - // identifier would pass today and start failing the moment that omission is - // corrected — turning this lookup test into a validation test without anyone - // touching it. Fixing the validator is not this task's business; not depending - // on the gap is. - uri := "at://did:plc:aaaaaaaaneverindexedcomm/" + postCollection + "/abc" - err := f.service.DeletePost(context.Background(), sessionFor(t, f.author, f.pds.URL()), - posts.DeletePostRequest{URI: uri}) - assert.ErrorIs(t, err, posts.ErrCommunityNotFound) + // Written directly with the community's own credentials, which is exactly + // how the pre-flip write path produced it. + community := f.communityAccount(t) + legacy := community.CreateRecord(t, postCollection, map[string]any{ + "$type": postCollection, + "community": f.community.DID, + "author": f.author.DID, + "title": "written before the flip", + "content": "and still deletable by its author", + "createdAt": "2026-07-01T12:00:00Z", + }) + rkey := rkeyOf(t, legacy.URI) + + require.NoError(t, f.service.DeletePost(ctx, sessionFor(t, f.author, f.pds.URL()), + posts.DeletePostRequest{URI: legacy.URI}), + "an author must still be able to delete a post written before the write path flipped") + + assert.True(t, testkit.IsNotFound(getRecordErr(ctx, community, postCollection, rkey)), + "the pre-flip record is still in the community's repo after its author deleted it") + + // And the old path's authorization still comes from the RECORD, because the + // caller has no credentials on the community's repo to be checked against. + second := community.CreateRecord(t, postCollection, map[string]any{ + "$type": postCollection, + "community": f.community.DID, + "author": f.author.DID, + "title": "someone else's pre-flip post", + "content": "body", + "createdAt": "2026-07-01T12:00:00Z", + }) + attacker := f.authorRepos.register(f.pds.CreateAccount(t, testkit.WithHandlePrefix("atl"))) + assert.ErrorIs(t, + f.service.DeletePost(ctx, sessionFor(t, attacker, f.pds.URL()), + posts.DeletePostRequest{URI: second.URI}), + posts.ErrNotAuthorized, + "the deprecated path's author check is the record's field, and it must still refuse a stranger") } // getRecordErr asks the PDS for a record and returns only the error, so a test diff --git a/internal/core/posts/submission_rkey_test.go b/internal/core/posts/submission_rkey_test.go new file mode 100644 --- /dev/null +++ b/internal/core/posts/submission_rkey_test.go @@ -0,0 +1,273 @@ +package posts + +import ( + "testing" + "time" + + "github.com/bluesky-social/indigo/atproto/syntax" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The deterministic record key a post is written at in its AUTHOR's repo +// (docs/PRD_AUTHOR_OWNED_POSTS.md §4.2). +// +// This closes the lost-response asymmetry §4.2 names: when a PDS write's outcome +// is ambiguous the record may or may not exist, and a client that retries with a +// server-chosen TID gets a SECOND post. Derived from the submission instead, the +// retry aims at the record the first attempt may already have written, and a +// create-only write reports the standing one rather than minting a twin. +// +// THE GOLDEN VALUES ARE HARD-CODED, NOT RECOMPUTED — the same discipline +// rkey_test.go applies to SubjectRkey, and for a sharper reason here. A test that +// recomputed the derivation would pass against a key that ignored the community, +// against one that ignored the bucket, and against one whose timestamp landed in +// a different century; every one of those is a silent production defect the day +// it ships. The constants below were produced OUTSIDE Go, by a Python +// transcription of the SPEC: +// +// import hashlib +// ALPHABET = "234567abcdefghijklmnopqrstuvwxyz" +// material = community + "\n" + fingerprint + "\n" + str(bucket) +// d = hashlib.sha256(material.encode()).digest() +// micros = bucket * window_micros + int.from_bytes(d[0:8], "big") % window_micros +// clock = int.from_bytes(d[8:10], "big") & 0x3FF +// v = (((micros & 0x1FFFFFFFFFFFFF) << 10) | clock) & 0x7FFFFFFFFFFFFFFF +// rkey = "".join(ALPHABET[(v >> (5*i)) & 0x1F] for i in range(12, -1, -1)) + +const ( + // goldenSubmissionFingerprint is sha256("golden-submission") in hex — the + // shape submissionFingerprint produces. + goldenSubmissionFingerprint = "6ccdb9079108e824fcc444f7e8c1aabad14690d9ec5cb28f783bd6f33230bcce" + + // goldenOtherFingerprint is sha256("a different submission"): different + // content, everything else held equal. + goldenOtherFingerprint = "3628901f33d006a95ced161472d5cbb52575388a68fec22b831b86fa224d4132" + + goldenCommunityA = "did:plc:aaaaaaaaaaaaaaaaaaaaaaaa" + goldenCommunityB = "did:plc:bbbbbbbbbbbbbbbbbbbbbbbb" + + // goldenBucket is an ordinary dedupeBucket value for a one-hour window: the + // index of the hour, counted from the epoch. + goldenBucket = 486000 +) + +// The four vectors. Each pair differs from the first in exactly ONE input, so a +// derivation that dropped that input produces the first value again and the +// assertion names which input went missing. +const ( + goldenSubmissionRkey = "3lrc77gmww4nc" // community A, golden fingerprint, golden bucket + goldenOtherCommunityRky = "3lrc4zsbrqy6t" // community B, same content, same bucket + goldenNextBucketRkey = "3lrccdypy25km" // community A, same content, bucket + 1 + goldenOtherContentRkey = "3lrc72ssh73fl" // community A, different content, same bucket +) + +// goldenWindow is the dedupe window the vectors were computed against. It is +// spelled as a duration rather than as microseconds because that is what +// SubmissionLimits.DedupeWindow holds and what the call site passes. +const goldenWindow = time.Hour + +func TestSubmissionRkey_GoldenVectors(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + community string + + fingerprint string + want string + bucket int64 + }{ + { + name: "the canonical submission", + community: goldenCommunityA, + fingerprint: goldenSubmissionFingerprint, + bucket: goldenBucket, + want: goldenSubmissionRkey, + }, + { + // THE COMMUNITY IS IN THE MATERIAL, and this is the vector that + // proves it. submissionFingerprint deliberately EXCLUDES the + // community (the client types a handle one time and a DID the next, + // and the ledger's unique key already scopes it) — so a derivation + // that hashed only the fingerprint and the bucket would give the + // same author the same rkey for two genuinely different posts, and + // the second crosspost would overwrite the first in their own repo. + name: "the same content submitted to a different community", + community: goldenCommunityB, + fingerprint: goldenSubmissionFingerprint, + bucket: goldenBucket, + want: goldenOtherCommunityRky, + }, + { + // THE BUCKET IS IN THE MATERIAL, which is what makes the collision + // EXPIRE. Without it, the rkey for a piece of content would be fixed + // forever, and an author who deliberately reposted the same thing a + // year later would be writing at an rkey their old post still holds. + name: "the same submission one dedupe window later", + community: goldenCommunityA, + fingerprint: goldenSubmissionFingerprint, + bucket: goldenBucket + 1, + want: goldenNextBucketRkey, + }, + { + name: "different content, same author and community and window", + community: goldenCommunityA, + fingerprint: goldenOtherFingerprint, + bucket: goldenBucket, + want: goldenOtherContentRkey, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, tc.want, SubmissionRkey(tc.community, tc.fingerprint, tc.bucket, goldenWindow), + "the rkey is fixed by §4.2 and by every post record already written under it; a different "+ + "value here means the derivation changed and every in-flight client retry just stopped "+ + "being idempotent") + }) + } +} + +func TestSubmissionRkey_IsAValidTID(t *testing.T) { + t.Parallel() + + // The postv2 lexicon declares `"key": "tid"`. A key that merely looked like + // one — 13 characters of the right alphabet — would be accepted by a PDS + // that does not validate record keys and refused by one that does, which is + // the worst of the two outcomes: it works in development and fails on the + // first federated peer running a stricter build. + for _, tc := range []struct { + name string + community string + fingerprint string + bucket int64 + window time.Duration + }{ + {"canonical", goldenCommunityA, goldenSubmissionFingerprint, goldenBucket, goldenWindow}, + {"bucket zero", goldenCommunityA, goldenSubmissionFingerprint, 0, goldenWindow}, + {"a one-minute dedupe window", goldenCommunityA, goldenSubmissionFingerprint, 29160000, time.Minute}, + {"a one-day dedupe window", goldenCommunityA, goldenSubmissionFingerprint, 20250, 24 * time.Hour}, + {"an empty fingerprint", goldenCommunityA, "", goldenBucket, goldenWindow}, + {"a 2048-byte community DID", didOfLength(2048), goldenSubmissionFingerprint, goldenBucket, goldenWindow}, + { + // A misconfiguration rather than a legal input: config.Validate + // refuses a non-positive dedupe window at startup. It is here + // because the derivation divides by the window, and a total + // function must not turn an operator's mistake into a panic on the + // write path — dedupeBucket takes the same care for the same + // reason. + name: "a non-positive window, which config refuses but arithmetic must survive", + + community: goldenCommunityA, fingerprint: goldenSubmissionFingerprint, + bucket: 0, window: 0, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + rkey := SubmissionRkey(tc.community, tc.fingerprint, tc.bucket, tc.window) + + parsed, err := syntax.ParseTID(rkey) + require.NoErrorf(t, err, "SubmissionRkey produced %q, which is not a TID the lexicon's "+ + "`key: tid` will accept", rkey) + assert.Equal(t, rkey, parsed.String()) + }) + } +} + +func TestSubmissionRkey_TimestampLandsInsideItsOwnDedupeBucket(t *testing.T) { + t.Parallel() + + // A TID's timestamp is not decoration: feeds and repo listings order by it, + // and a client reading a post's rkey reads a time out of it. Placing the + // derived time inside the submission's own dedupe bucket keeps that time + // within one window of when the post was actually submitted — so an hourly + // window puts every post within the hour it was written, rather than at + // whatever moment 64 bits of digest happened to name. + for _, window := range []time.Duration{time.Minute, time.Hour, 24 * time.Hour} { + t.Run(window.String(), func(t *testing.T) { + t.Parallel() + + now := time.Date(2026, 8, 8, 13, 47, 11, 0, time.UTC) + bucket := dedupeBucket(now, window) + + rkey := SubmissionRkey(goldenCommunityA, goldenSubmissionFingerprint, bucket, window) + parsed, err := syntax.ParseTID(rkey) + require.NoError(t, err) + + start := time.Unix(0, bucket*int64(window)).UTC() + end := start.Add(window) + + stamped := parsed.Time() + assert.Falsef(t, stamped.Before(start), + "the derived timestamp %s is before its dedupe bucket [%s, %s)", stamped, start, end) + assert.Truef(t, stamped.Before(end), + "the derived timestamp %s is at or past the end of its dedupe bucket [%s, %s)", stamped, start, end) + }) + } +} + +func TestSubmissionRkey_IsStableAcrossCalls(t *testing.T) { + t.Parallel() + + // Stability IS the idempotence claim. If this ever stops holding — a + // timestamp read at call time, a random salt, a map iteration folded into + // the material — then a client retrying after a lost response writes a + // second post instead of colliding with its own first one, which is the + // exact duplicate §4.2 exists to close. + first := SubmissionRkey(goldenCommunityA, goldenSubmissionFingerprint, goldenBucket, goldenWindow) + require.NotEmpty(t, first) + + for i := 0; i < 32; i++ { + require.Equalf(t, first, + SubmissionRkey(goldenCommunityA, goldenSubmissionFingerprint, goldenBucket, goldenWindow), + "call %d returned a different key for the same submission", i) + } +} + +func TestSubmissionRkey_DistinctInputsGiveDistinctKeys(t *testing.T) { + t.Parallel() + + // The same four vectors as the golden table, asserted as a SET. The table + // above proves each value is the right one; this proves they are four + // values and not two — a derivation that silently dropped an input would + // pass a hand-updated golden table and fail here. + keys := map[string]string{ + "canonical": SubmissionRkey(goldenCommunityA, goldenSubmissionFingerprint, goldenBucket, goldenWindow), + "different community": SubmissionRkey(goldenCommunityB, goldenSubmissionFingerprint, goldenBucket, goldenWindow), + "next bucket": SubmissionRkey(goldenCommunityA, goldenSubmissionFingerprint, goldenBucket+1, goldenWindow), + "different content": SubmissionRkey(goldenCommunityA, goldenOtherFingerprint, goldenBucket, goldenWindow), + "community and bucket": SubmissionRkey(goldenCommunityB, goldenSubmissionFingerprint, goldenBucket+1, goldenWindow), + } + + seen := make(map[string]string, len(keys)) + for name, key := range keys { + require.NotEmptyf(t, key, "%s produced no key at all", name) + if previous, collided := seen[key]; collided { + t.Errorf("%q and %q derive the same rkey %q — one of the submission's identifying "+ + "inputs is not in the material, so two different posts would be written at one key "+ + "in the author's repo and the second would overwrite the first", + previous, name, key) + continue + } + seen[key] = name + } +} + +func TestSubmissionRkey_MaterialCannotBeAmbiguouslySplit(t *testing.T) { + t.Parallel() + + // The material is three fields concatenated, so the delimiter has to be + // something none of them can contain — otherwise a community DID ending in + // the delimiter and a fingerprint beginning with it would produce the same + // bytes as some other pair, and two different submissions would land on one + // rkey. A DID's legal charset and a hex fingerprint both exclude every + // control character; a naive concatenation with no delimiter at all does + // not, and this is the case that catches it. + assert.NotEqual(t, + SubmissionRkey("did:web:a.example", "b"+goldenSubmissionFingerprint, goldenBucket, goldenWindow), + SubmissionRkey("did:web:a.exampleb", goldenSubmissionFingerprint, goldenBucket, goldenWindow), + "the community and the fingerprint must not be able to run together: a derivation that "+ + "concatenated them without a delimiter lets one submission's key be forged from another's") +} diff --git a/internal/core/unfurl/post_unfurl_integration_test.go b/internal/core/unfurl/post_unfurl_integration_test.go --- a/internal/core/unfurl/post_unfurl_integration_test.go +++ b/internal/core/unfurl/post_unfurl_integration_test.go @@ -108,14 +108,17 @@ AuthorDID: testUserDID, } authCtx := middleware.SetTestUserDID(ctx, testUserDID) - _, err = postService.CreatePost(authCtx, createReq) + _, err = postService.CreatePost(authCtx, nil, createReq) - // Should still fail at token refresh (expected) - require.Error(t, err, "Expected error at token refresh") - assert.Contains(t, err.Error(), "failed to refresh community credentials") + // Should still fail at the author-repo write (expected): the service is + // wired with no author-repo factory, so it has nothing to sign the record + // with. Before task 6 the same role was played by the community's unusable + // token, which the write path no longer touches. + require.Error(t, err, "Expected error opening the author's repository") + assert.ErrorIs(t, err, posts.ErrNoAuthorCredentials) // The point is that it didn't fail earlier due to unsupported URL - t.Log("✓ Post creation with unsupported URL proceeded to PDS write stage") + t.Log("✓ Post creation with unsupported URL proceeded to the author-repo write stage") } // TestPostUnfurl_MissingEmbedType tests posts without external embed type don't trigger unfurling @@ -197,11 +200,11 @@ AuthorDID: testUserDID, } authCtx := middleware.SetTestUserDID(ctx, testUserDID) - _, err := postService.CreatePost(authCtx, createReq) + _, err := postService.CreatePost(authCtx, nil, createReq) - // Should fail at token refresh (expected) + // Should fail at the author-repo write (expected); see above. require.Error(t, err) - assert.Contains(t, err.Error(), "failed to refresh community credentials") + assert.ErrorIs(t, err, posts.ErrNoAuthorCredentials) t.Log("✓ Post without embed succeeded (no unfurl attempted)") }) @@ -230,11 +233,11 @@ AuthorDID: testUserDID, } authCtx := middleware.SetTestUserDID(ctx, testUserDID) - _, err := postService.CreatePost(authCtx, createReq) + _, err := postService.CreatePost(authCtx, nil, createReq) - // Should fail at token refresh (expected) + // Should fail at the author-repo write (expected); see above. require.Error(t, err) - assert.Contains(t, err.Error(), "failed to refresh community credentials") + assert.ErrorIs(t, err, posts.ErrNoAuthorCredentials) t.Log("✓ Post with images embed succeeded (no unfurl attempted)") }) diff --git a/tests/live/post_unfurl_test.go b/tests/live/post_unfurl_test.go --- a/tests/live/post_unfurl_test.go +++ b/tests/live/post_unfurl_test.go @@ -272,9 +272,10 @@ AuthorDID: testUserDID, } authCtx := middleware.SetTestUserDID(ctx, testUserDID) - _, err = postService.CreatePost(authCtx, createReq) + _, err = postService.CreatePost(authCtx, nil, createReq) - // Expected to fail at token refresh + // Expected to fail at the author-repo write: no author-repo factory is + // wired, so there is nothing to sign the record with (§4.2 step 3). require.Error(t, err) // The important check: verify unfurl happened but didn't overwrite user data -- tangled.sh