diff --git a/CLAUDE.md b/CLAUDE.md index 8a015bf..7153cda 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -91,11 +91,14 @@ client plain HTTP clients so it can reach a PDS on loopback. the tag (`make test`, `TEST_TAGS` in the Makefile, `GO_TAGS` in Air/Dockerfile.dev). - Production Dockerfiles and `make build-trixie` never set it. There is no runtime switch: a production binary cannot be configured to resolve local DIDs. -- Tests that need loopback did:web resolution call `t.Fatal` naming the tag - when `atproto.TestModeBuild` is false, so an untagged `go test` says why. -- `server.test_mode` (runtime) still gates the remaining behavioral branches - (hold issuer-mismatch tolerance, backfill warning suppression, registry - fall-back-to-default-hold); it no longer affects DID resolution. +- Tests that need loopback did:web resolution live in `*_testmode_test.go` + files (or whole files) under `//go:build testmode`, so a bare `go test ./...` + compiles them out and stays green while `make test` runs everything. The + integration harness carries the same constraint. +- The remaining local-dev behaviors (hold issuer-mismatch tolerance, skipped + relay crawl requests, backfill warning suppression, registry + fall-back-to-default-hold) read `atproto.TestModeBuild` too. There is no + runtime test-mode config key any more. ## Architecture Overview diff --git a/Makefile b/Makefile index 91487fe..80520bf 100644 --- a/Makefile +++ b/Makefile @@ -184,7 +184,7 @@ stripe-integration-test: ## Run Stripe sandbox-backed billing tests (needs STRIP @echo " STRIPE_TEST_PRICE_MONTHLY, STRIPE_TEST_PRICE_YEARLY" @echo " Optional env: STRIPE_TEST_TIER_NAME (default 'Supporter')," @echo " STRIPE_TEST_EXISTING_CUSTOMER_DID (skips portal search-lag wait)" - go test -tags="billing stripe_integration" -count=1 -timeout=180s ./test/stripe-integration/... + go test -tags="billing stripe_integration $(TEST_TAGS)" -count=1 -timeout=180s ./test/stripe-integration/... ##@ Quality Targets @@ -210,7 +210,7 @@ lint: check-golangci-lint ## Run golangci-lint @echo "→ Running golangci-lint (billing tag)..." golangci-lint run --build-tags=billing ./pkg/billing/... @echo "→ Running golangci-lint (testmode tag)..." - golangci-lint run --build-tags=testmode ./pkg/atproto/... + golangci-lint run --build-tags=testmode ./... lex-lint: ## Lint ATProto lexicon schemas goat lex lint ./lexicons/ diff --git a/config-appview.example.yaml b/config-appview.example.yaml index 071d8d3..03d5c6d 100644 --- a/config-appview.example.yaml +++ b/config-appview.example.yaml @@ -25,8 +25,6 @@ server: addr: :5000 # Public-facing URL for OAuth callbacks and JWT realm. Auto-detected if empty. base_url: "" - # Local development only. Routes pushes to the default hold when the user's chosen hold is unreachable and quiets backfill warnings about external holds. Does not affect DID resolution: that needs a -tags testmode build. - test_mode: false # Display name shown on OAuth authorization screens. client_name: AT Container Registry # Short name used in page titles and browser tabs. diff --git a/config-hold.example.yaml b/config-hold.example.yaml index 76e8354..53d0223 100644 --- a/config-hold.example.yaml +++ b/config-hold.example.yaml @@ -43,8 +43,6 @@ server: public: false # DID of successor hold for migration. Appview redirects all requests to the successor. successor: "" - # Local development only. Skips relay crawl requests (a local hold is not reachable by public relays) and tolerates an appview token issuer that differs from appview_did. Does not affect DID resolution: that needs a -tags testmode build. - test_mode: false # Endpoints used for proactive scan discovery. MUST support com.atproto.sync.listReposByCollection. Also sent requestCrawl on startup (best-effort, in addition to built-in known relays). relay_endpoints: - https://relay1.us-east.bsky.network diff --git a/deploy/upcloud/configs/appview.yaml.tmpl b/deploy/upcloud/configs/appview.yaml.tmpl index a2d4766..0a769e4 100644 --- a/deploy/upcloud/configs/appview.yaml.tmpl +++ b/deploy/upcloud/configs/appview.yaml.tmpl @@ -11,7 +11,6 @@ server: addr: :5000 base_url: "https://seamark.dev" client_name: Seamark - test_mode: false client_short_name: Seamark registry_domains: - "buoy.cr" diff --git a/deploy/upcloud/configs/hold.yaml.tmpl b/deploy/upcloud/configs/hold.yaml.tmpl index 8584d9a..bb9b0d5 100644 --- a/deploy/upcloud/configs/hold.yaml.tmpl +++ b/deploy/upcloud/configs/hold.yaml.tmpl @@ -19,7 +19,6 @@ server: public_url: "https://{{.HoldDomain}}" public: false successor: "" - test_mode: false relay_endpoints: - https://relay1.us-east.bsky.network - https://relay1.us-west.bsky.network diff --git a/docker-compose.yml b/docker-compose.yml index 4f34c34..ace2e99 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -32,7 +32,6 @@ services: # Labeler URL (HTTP for dev — ParseLabelerURL accepts it directly so we don't # have to round-trip through did:web → https:// resolution). ATCR_LABELER_DID: did:web:172.28.0.4%3A5002 - ATCR_SERVER_TEST_MODE: true ATCR_LOG_LEVEL: debug LOG_SHIPPER_BACKEND: victoria LOG_SHIPPER_URL: http://172.28.0.10:9428 @@ -76,7 +75,6 @@ services: HOLD_SERVER_PUBLIC_URL: http://localhost:8080 HOLD_REGISTRATION_OWNER_DID: did:plc:pddp4xt5lgnv2qsegbzzs4xg HOLD_REGISTRATION_ALLOW_ALL_CREW: true - HOLD_SERVER_TEST_MODE: true HOLD_LOG_LEVEL: debug # Subscribe to the dev labeler so takedowns purge records on this hold and # GC honors the reversibility window. Same value the appview uses for @@ -135,7 +133,6 @@ services: LABELER_LABELER_PUBLIC_URL: http://172.28.0.4:5002 LABELER_LABELER_OWNER_DID: did:plc:pddp4xt5lgnv2qsegbzzs4xg LABELER_LABELER_DATA_DIR: /var/lib/atcr-labeler - LABELER_SERVER_TEST_MODE: true LABELER_LOG_LEVEL: debug LOG_SHIPPER_BACKEND: victoria LOG_SHIPPER_URL: http://172.28.0.10:9428 diff --git a/docs/DEVELOPMENT.md b/docs/DEVELOPMENT.md index 0ddd948..c4a1c32 100644 --- a/docs/DEVELOPMENT.md +++ b/docs/DEVELOPMENT.md @@ -185,7 +185,6 @@ shorthand): | `ATCR_AUTH_CERT_PATH` | `auth.cert_path` | | `ATCR_JETSTREAM_BACKFILL_ENABLED` | `jetstream.backfill_enabled` | | `ATCR_LABELER_DID` | `labeler.did` | -| `ATCR_SERVER_TEST_MODE` | `server.test_mode` | | `ATCR_LOG_LEVEL` | `log.level` | There is **no** `ATCR_DEV_MODE` variable anywhere in the codebase. Likewise diff --git a/docs/HOLD_DISCOVERY.md b/docs/HOLD_DISCOVERY.md index 46ce382..bee097f 100644 --- a/docs/HOLD_DISCOVERY.md +++ b/docs/HOLD_DISCOVERY.md @@ -125,9 +125,10 @@ forged crew records placing a fake hold in targeted users' member lists. It does defend against a malicious actor running a real hold service — that is inherent to open federation, same as any open-registration hold. -In test mode (`SetTestMode(true)`), local `did:web` identifiers that the indigo -directory cannot resolve (HTTP, IP:port) are trusted, matching the -`ResolveHoldDIDToURL` fallback. +In a `-tags testmode` build, local `did:web` identifiers (HTTP, IP:port, +localhost) resolve through the directory wrapper in `pkg/atproto/indigo_local.go`, +so the same verification applies to them. A production build cannot resolve +them at all. ## Data Model diff --git a/docs/appview.md b/docs/appview.md index a3eed72..450eb6d 100644 --- a/docs/appview.md +++ b/docs/appview.md @@ -278,9 +278,11 @@ log_level: debug server: managed_holds: - "did:web:127.0.0.1:8080" - test_mode: true # allows HTTP for DID resolution ``` +Resolving the loopback `did:web` above needs a `-tags testmode` build; see +[DEVELOPMENT.md](DEVELOPMENT.md). + Run a hold service locally with Minio for S3-compatible storage. See [hold.md](hold.md) for hold setup. ## Web Interface diff --git a/docs/hold.md b/docs/hold.md index 2acac71..00c44b5 100644 --- a/docs/hold.md +++ b/docs/hold.md @@ -150,7 +150,7 @@ recover from it, and a proposed automatic fix that is **not implemented**. **1. The hold announces itself once, at boot.** `ServeWithListener` fires a single `requestCrawls()` goroutine during startup -(`pkg/hold/server.go:400`), skipped entirely when `server.test_mode` is set because local +(`pkg/hold/server.go:400`), skipped entirely in a `-tags testmode` build because local dev holds are not reachable by public relays. The implementation (`pkg/hold/server.go:448-483`) builds a deduplicated target list from `atproto.KnownRelays` (`pkg/atproto/relays.go:22`, the hardcoded list also documented in diff --git a/internal/testharness/doc.go b/internal/testharness/doc.go new file mode 100644 index 0000000..5efb5f2 --- /dev/null +++ b/internal/testharness/doc.go @@ -0,0 +1,13 @@ +// Package testharness boots an in-process ATCR stack (fake PDS, gofakes3, +// hold, appview) for integration smoke tests. It exposes thin helpers for +// adding sailors and obtaining basic-auth credentials for an OCI registry +// client — either a library-specific authn.Authenticator (RegistryAuth) or +// a neutral Auth value (RegistryCreds) consumed by the client-agnostic +// matrix in test/integration. +// +// The stack's holds and appview identify themselves by did:web on loopback +// ports, which only a `-tags testmode` build resolves (see +// pkg/atproto/indigo_local.go). harness.go carries that build constraint, so +// an untagged build sees an empty package and any importer fails to compile +// rather than dying on an opaque dial error at runtime. +package testharness diff --git a/internal/testharness/harness.go b/internal/testharness/harness.go index 0b77724..e43fdf0 100644 --- a/internal/testharness/harness.go +++ b/internal/testharness/harness.go @@ -1,9 +1,5 @@ -// Package testharness boots an in-process ATCR stack (fake PDS, gofakes3, -// hold, appview) for integration smoke tests. It exposes thin helpers for -// adding sailors and obtaining basic-auth credentials for an OCI registry -// client — either a library-specific authn.Authenticator (RegistryAuth) or -// a neutral Auth value (RegistryCreds) consumed by the client-agnostic -// matrix in test/integration. +//go:build testmode + package testharness import ( @@ -117,23 +113,13 @@ func New(t *testing.T, opts ...Option) *Harness { } h := &Harness{t: t} - // The stack's holds and appview identify themselves by did:web on loopback - // ports. Only a `-tags testmode` build resolves those (pkg/atproto's - // indigo_local.go); a production build would fail every lookup with an - // opaque dial error, so say why up front. - if !atproto.TestModeBuild { - t.Fatal("the integration harness needs a `-tags testmode` build: go test -tags integration,testmode ./test/integration/...") - } - // 1. Fake PDS. h.PDS = testpds.New(t) atproto.SetDirectory(h.PDS.Directory()) - atproto.SetTestMode(true) t.Cleanup(func() { // Reset to a fresh default so a later non-test process won't see our // fake. SetDirectory(nil) re-arms lazy init in GetDirectory. atproto.SetDirectory(nil) - atproto.SetTestMode(false) }) // 2. gofakes3 (S3-compatible in-memory). @@ -190,7 +176,6 @@ func New(t *testing.T, opts ...Option) *Harness { Addr: holdAddr, PublicURL: holdPublicURL, Public: !o.privateHold, // public: anyone may pull, crew may push. private: crew only, both ways. - TestMode: true, ReadTimeout: 60 * time.Second, WriteTimeout: 5 * time.Minute, }, @@ -438,7 +423,6 @@ func buildAppViewConfig(addr, baseURL, holdDID, dbPath string) *appview.Config { cfg.Server.Addr = addr cfg.Server.BaseURL = baseURL cfg.Server.ManagedHolds = []string{holdDID} - cfg.Server.TestMode = true // Registry domain is a bare hostname (no port). DomainRoutingMiddleware // strips ports before matching, so "127.0.0.1" is what /v2/ requests // will hit (since the listener binds to 127.0.0.1). BaseURL uses @@ -496,7 +480,6 @@ func buildDistributionConfig(addr, baseURL, holdDID string, services []string, c Name: "atproto-resolver", Options: configuration.Parameters{ "default_hold_did": holdDID, - "test_mode": true, "base_url": baseURL, }, }}, diff --git a/pkg/appview/authgate/push_authorizer_test.go b/pkg/appview/authgate/push_authorizer_test.go index 711c1ee..a3bfc08 100644 --- a/pkg/appview/authgate/push_authorizer_test.go +++ b/pkg/appview/authgate/push_authorizer_test.go @@ -2,11 +2,9 @@ package authgate import ( "context" - "slices" "strings" "testing" - "atcr.io/pkg/atproto" "atcr.io/pkg/auth" ) @@ -203,132 +201,6 @@ func TestCheckCrewBlobWrite_NullPermissions(t *testing.T) { // --- checkQuota ------------------------------------------------------------ -func TestCheckQuota_UnderLimit(t *testing.T) { - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) - - srv := quotaServer(t, 200, `{"totalSize":100,"limit":1000}`) - a := New(newTestDB(t), fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) - - if err := a.checkQuota(context.Background(), "did:plc:alice", srv.holdDID); err != nil { - t.Errorf("checkQuota under limit = %v, want nil", err) - } - if srv.hits != 1 { - t.Errorf("expected 1 hit on quota endpoint, got %d", srv.hits) - } - // The query value is percent-encoded by the client (see - // TestCheckQuota_EncodesUserDID), so plain "did:plc:alice" becomes - // "did%3Aplc%3Aalice" on the wire. - if !strings.Contains(srv.lastURL, "userDid=did%3Aplc%3Aalice") { - t.Errorf("expected userDid query param, got URL %q", srv.lastURL) - } - if !strings.Contains(srv.lastURL, atproto.HoldGetQuota) { - t.Errorf("expected URL path to contain %q, got %q", atproto.HoldGetQuota, srv.lastURL) - } -} - -func TestCheckQuota_OverLimit(t *testing.T) { - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) - - // 5 GiB exactly so the formatted message shows "5.00 GB / 5.00 GB". - srv := quotaServer(t, 200, `{"totalSize":5368709120,"limit":5368709120}`) - d := newTestDB(t) - seedUser(t, d, "did:plc:alice", "alice.bsky.social", "") - a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) - - err := a.checkQuota(context.Background(), "did:plc:alice", srv.holdDID) - if err == nil { - t.Fatal("checkQuota at limit should deny") - } - msg := err.Error() - for _, want := range []string{"quota exceeded", "5.00 GB", "did:plc:alice", "alice.bsky.social"} { - if !strings.Contains(msg, want) { - t.Errorf("expected %q in error %q", want, msg) - } - } -} - -// When no users row exists for the DID (handle unknown), the error still -// formats correctly with the bare DID. -func TestCheckQuota_OverLimit_NoHandle(t *testing.T) { - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) - - srv := quotaServer(t, 200, `{"totalSize":5368709120,"limit":5368709120}`) - a := New(newTestDB(t), fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) - - err := a.checkQuota(context.Background(), "did:plc:bob", srv.holdDID) - if err == nil { - t.Fatal("checkQuota at limit should deny") - } - msg := err.Error() - if !strings.Contains(msg, "did:plc:bob") || strings.Contains(msg, "(did:") { - t.Errorf("expected bare DID (no parenthesized form) in error %q", msg) - } -} - -func TestCheckQuota_NilLimitAllows(t *testing.T) { - // A user on the unlimited tier has limit == nil. Even huge totalSize - // must not deny. - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) - - srv := quotaServer(t, 200, `{"totalSize":99999999}`) - a := New(newTestDB(t), fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) - - if err := a.checkQuota(context.Background(), "did:plc:alice", srv.holdDID); err != nil { - t.Errorf("checkQuota with nil limit = %v, want nil", err) - } -} - -func TestCheckQuota_500FailsOpen(t *testing.T) { - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) - - srv := quotaServer(t, 500, `oops`) - a := New(newTestDB(t), fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) - - if err := a.checkQuota(context.Background(), "did:plc:alice", srv.holdDID); err != nil { - t.Errorf("checkQuota with 500 should fail open, got %v", err) - } -} - -func TestCheckQuota_BadJSONFailsOpen(t *testing.T) { - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) - - srv := quotaServer(t, 200, `not-json`) - a := New(newTestDB(t), fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) - - if err := a.checkQuota(context.Background(), "did:plc:alice", srv.holdDID); err != nil { - t.Errorf("checkQuota with malformed JSON should fail open, got %v", err) - } -} - -func TestCheckQuota_EncodesUserDID(t *testing.T) { - // did:web DIDs may contain percent-encoded characters (e.g. "%3A" for - // the port colon). Without proper query encoding the receiving server's - // query parser decodes "%3A" → ":", mangling the DID and missing the - // records that were keyed by the original form. The fix encodes the - // DID once at the client side so the server decodes it back exactly. - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) - - srv := quotaServer(t, 200, `{"totalSize":100,"limit":1000}`) - a := New(newTestDB(t), fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) - - encodedDID := "did:web:127.0.0.1%3A45397:user:alice.test" - if err := a.checkQuota(context.Background(), encodedDID, srv.holdDID); err != nil { - t.Errorf("checkQuota with encoded DID = %v, want nil", err) - } - // The URL the server saw should contain the double-encoded form, so - // that its single decode pass yields the original DID back. - if !strings.Contains(srv.lastURL, "did%3Aweb%3A127.0.0.1%253A45397%3Auser%3Aalice.test") { - t.Errorf("expected query value to be percent-encoded; got URL %q", srv.lastURL) - } -} - func TestCheckQuota_HoldURLResolutionFailsOpen(t *testing.T) { // A "did:" prefixed but otherwise malformed identifier makes // ResolveHoldURL → ResolveHoldDIDToURL → syntax.ParseDID error out @@ -361,221 +233,3 @@ func TestAuthorize_NoHoldAllowsAll(t *testing.T) { } } } - -func TestAuthorize_CaptainBypassesCrewCheck(t *testing.T) { - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) - - srv := quotaServer(t, 200, `{"totalSize":1,"limit":1000}`) - d := newTestDB(t) - seedUser(t, d, "did:plc:alice", "alice.test", srv.holdDID) - seedCaptain(t, d, srv.holdDID, "did:plc:alice") - // A contradictory crew row should NOT trip the gate — captain bypass. - seedCrewMember(t, d, srv.holdDID, "did:plc:alice", `["blob:read"]`) - - a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) - if err := a.Authorize(context.Background(), "did:plc:alice", "", pushAccess("alice/x")); err != nil { - t.Errorf("Authorize(captain push) = %v, want nil", err) - } -} - -func TestAuthorize_NonCaptainPushWithoutCrewDenied(t *testing.T) { - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) - - srv := quotaServer(t, 200, `{"totalSize":1,"limit":1000}`) - d := newTestDB(t) - seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) - seedCaptain(t, d, srv.holdDID, "did:plc:alice") // alice owns; bob is not crew - - a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) - err := a.Authorize(context.Background(), "did:plc:bob", "", pushAccess("bob/x")) - if err == nil || !strings.Contains(err.Error(), "crew membership required") { - t.Errorf("expected 'crew membership required', got %v", err) - } -} - -func TestAuthorize_NonCaptainPushWithoutBlobWriteDenied(t *testing.T) { - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) - - srv := quotaServer(t, 200, `{"totalSize":1,"limit":1000}`) - d := newTestDB(t) - seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) - seedCaptain(t, d, srv.holdDID, "did:plc:alice") - seedCrewMember(t, d, srv.holdDID, "did:plc:bob", `["blob:read"]`) - - a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) - err := a.Authorize(context.Background(), "did:plc:bob", "", pushAccess("bob/x")) - if err == nil || !strings.Contains(err.Error(), "lacks blob:write") { - t.Errorf("expected 'lacks blob:write', got %v", err) - } -} - -func TestAuthorize_NonCaptainPushUnderQuotaAllowed(t *testing.T) { - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) - - srv := quotaServer(t, 200, `{"totalSize":100,"limit":1000}`) - d := newTestDB(t) - seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) - seedCaptain(t, d, srv.holdDID, "did:plc:alice") - seedCrewMember(t, d, srv.holdDID, "did:plc:bob", `["blob:write"]`) - - a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) - if err := a.Authorize(context.Background(), "did:plc:bob", "", pushAccess("bob/x")); err != nil { - t.Errorf("Authorize(crew blob:write under quota) = %v, want nil", err) - } -} - -func TestAuthorize_NonCaptainPushOverQuotaDenied(t *testing.T) { - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) - - srv := quotaServer(t, 200, `{"totalSize":1000,"limit":1000}`) - d := newTestDB(t) - seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) - seedCaptain(t, d, srv.holdDID, "did:plc:alice") - seedCrewMember(t, d, srv.holdDID, "did:plc:bob", `["blob:write"]`) - - a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) - err := a.Authorize(context.Background(), "did:plc:bob", "", pushAccess("bob/x")) - if err == nil || !strings.Contains(err.Error(), "quota exceeded") { - t.Errorf("expected 'quota exceeded', got %v", err) - } -} - -// An over-quota user has to be able to delete: the denial message tells them -// to, and docker/crane ask for pull,push,delete on a delete. The gate grants -// the non-push subset instead of failing the whole request. -func TestAuthorize_OverQuotaGrantsDeleteWithoutPush(t *testing.T) { - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) - - srv := quotaServer(t, 200, `{"totalSize":1000,"limit":1000}`) - d := newTestDB(t) - seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) - seedCaptain(t, d, srv.holdDID, "did:plc:alice") - seedCrewMember(t, d, srv.holdDID, "did:plc:bob", `["blob:write"]`) - - a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) - access := []auth.AccessEntry{ - {Type: "repository", Name: "bob/x", Actions: []string{"pull", "push", "delete"}}, - } - if err := a.Authorize(context.Background(), "did:plc:bob", "", access); err != nil { - t.Fatalf("Authorize(over quota, delete requested) = %v, want nil", err) - } - if got := access[0].Actions; !slices.Equal(got, []string{"pull", "delete"}) { - t.Errorf("granted actions = %v, want [pull delete]", got) - } -} - -// Delete-only never carries push, so it must survive untouched. -func TestAuthorize_OverQuotaAllowsDeleteOnlyScope(t *testing.T) { - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) - - srv := quotaServer(t, 200, `{"totalSize":1000,"limit":1000}`) - d := newTestDB(t) - seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) - seedCaptain(t, d, srv.holdDID, "did:plc:alice") - - a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) - access := []auth.AccessEntry{ - {Type: "repository", Name: "bob/x", Actions: []string{"delete"}}, - } - if err := a.Authorize(context.Background(), "did:plc:bob", "", access); err != nil { - t.Fatalf("Authorize(delete only) = %v, want nil", err) - } - if got := access[0].Actions; !slices.Equal(got, []string{"delete"}) { - t.Errorf("granted actions = %v, want [delete]", got) - } - if srv.hits != 0 { - t.Errorf("quota endpoint hit %d times for delete-only scope, want 0", srv.hits) - } -} - -// A plain push must keep failing loudly, otherwise the client never sees the -// quota message and just gets an opaque 401 on the first blob upload. -func TestAuthorize_OverQuotaStillDeniesPlainPush(t *testing.T) { - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) - - srv := quotaServer(t, 200, `{"totalSize":1000,"limit":1000}`) - d := newTestDB(t) - seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) - seedCaptain(t, d, srv.holdDID, "did:plc:alice") - seedCrewMember(t, d, srv.holdDID, "did:plc:bob", `["blob:write"]`) - - a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) - access := pushAccess("bob/x") - err := a.Authorize(context.Background(), "did:plc:bob", "", access) - if err == nil || !strings.Contains(err.Error(), "quota exceeded") { - t.Fatalf("expected 'quota exceeded', got %v", err) - } - if got := access[0].Actions; !slices.Equal(got, []string{"pull", "push"}) { - t.Errorf("denied request should leave actions untouched, got %v", got) - } -} - -// Under quota, a delete request keeps its push action. -func TestAuthorize_UnderQuotaKeepsPushAlongsideDelete(t *testing.T) { - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) - - srv := quotaServer(t, 200, `{"totalSize":1,"limit":1000}`) - d := newTestDB(t) - seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) - seedCaptain(t, d, srv.holdDID, "did:plc:alice") - seedCrewMember(t, d, srv.holdDID, "did:plc:bob", `["blob:write"]`) - - a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) - access := []auth.AccessEntry{ - {Type: "repository", Name: "bob/x", Actions: []string{"pull", "push", "delete"}}, - } - if err := a.Authorize(context.Background(), "did:plc:bob", "", access); err != nil { - t.Fatalf("Authorize(under quota) = %v, want nil", err) - } - if got := access[0].Actions; !slices.Equal(got, []string{"pull", "push", "delete"}) { - t.Errorf("granted actions = %v, want all three preserved", got) - } -} - -func TestAuthorize_PullOnlySkipsMembershipAndQuota(t *testing.T) { - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) - - // Quota server installed but should never be hit: pull bypasses both - // the membership requirement and the quota call. - srv := quotaServer(t, 200, `{"totalSize":1000,"limit":1000}`) - d := newTestDB(t) - seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) - seedCaptain(t, d, srv.holdDID, "did:plc:alice") // bob is NOT captain, NOT crew - - a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) - if err := a.Authorize(context.Background(), "did:plc:bob", "", pullAccess("alice/x")); err != nil { - t.Errorf("Authorize(pull only) = %v, want nil", err) - } - if srv.hits != 0 { - t.Errorf("quota endpoint hit %d times for pull-only request, want 0", srv.hits) - } -} - -func TestAuthorize_WildcardPushTreatedAsPull(t *testing.T) { - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) - - srv := quotaServer(t, 200, `{"totalSize":1000,"limit":1000}`) - d := newTestDB(t) - seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) - seedCaptain(t, d, srv.holdDID, "did:plc:alice") - - a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) - wildcard := []auth.AccessEntry{{Type: "repository", Name: "*", Actions: []string{"pull", "push"}}} - if err := a.Authorize(context.Background(), "did:plc:bob", "", wildcard); err != nil { - t.Errorf("Authorize(wildcard push) = %v, want nil (treated as pull)", err) - } - if srv.hits != 0 { - t.Errorf("quota endpoint hit %d times for wildcard scope, want 0", srv.hits) - } -} diff --git a/pkg/appview/authgate/push_authorizer_testmode_test.go b/pkg/appview/authgate/push_authorizer_testmode_test.go new file mode 100644 index 0000000..d94dfde --- /dev/null +++ b/pkg/appview/authgate/push_authorizer_testmode_test.go @@ -0,0 +1,349 @@ +//go:build testmode + +package authgate + +import ( + "context" + "net/http" + "net/http/httptest" + "slices" + "strings" + "testing" + + "atcr.io/pkg/atproto" + "atcr.io/pkg/auth" + "atcr.io/pkg/testpds" +) + +// quotaServerResult captures HTTP traffic the server saw, for assertions. +type quotaServerResult struct { + server *httptest.Server + holdDID string // did:web:127.0.0.1%3APORT form + hits int + lastURL string +} + +// quotaServer spins up an httptest.Server that responds to every request +// with the given status + body, records hit count + last URL, and returns +// both the server URL and the did:web:HOST form that resolves to it. The +// server also serves its own did:web document (not counted in hits) so the +// test-mode identity directory can resolve that DID back to the server. +func quotaServer(t *testing.T, status int, body string) *quotaServerResult { + t.Helper() + res := "aServerResult{} + mux := http.NewServeMux() + mux.HandleFunc("/.well-known/did.json", func(w http.ResponseWriter, r *http.Request) { + base := "http://" + r.Host + testpds.HoldDIDDocumentHandler(testpds.DIDWebForURL(base), base)(w, r) + }) + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + res.hits++ + res.lastURL = r.URL.String() + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _, _ = w.Write([]byte(body)) + }) + res.server = httptest.NewServer(mux) + t.Cleanup(res.server.Close) + + // httptest.Server.URL has the form "http://127.0.0.1:PORT"; did:web + // percent-encodes the port colon. + res.holdDID = testpds.DIDWebForURL(res.server.URL) + return res +} + +// httpClient returns the server's client, which trusts its TLS cert (n/a +// here since httptest.NewServer is HTTP) and routes to the loopback. +func (r *quotaServerResult) httpClient() *http.Client { + return r.server.Client() +} + +func TestCheckQuota_UnderLimit(t *testing.T) { + srv := quotaServer(t, 200, `{"totalSize":100,"limit":1000}`) + a := New(newTestDB(t), fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) + + if err := a.checkQuota(context.Background(), "did:plc:alice", srv.holdDID); err != nil { + t.Errorf("checkQuota under limit = %v, want nil", err) + } + if srv.hits != 1 { + t.Errorf("expected 1 hit on quota endpoint, got %d", srv.hits) + } + // The query value is percent-encoded by the client (see + // TestCheckQuota_EncodesUserDID), so plain "did:plc:alice" becomes + // "did%3Aplc%3Aalice" on the wire. + if !strings.Contains(srv.lastURL, "userDid=did%3Aplc%3Aalice") { + t.Errorf("expected userDid query param, got URL %q", srv.lastURL) + } + if !strings.Contains(srv.lastURL, atproto.HoldGetQuota) { + t.Errorf("expected URL path to contain %q, got %q", atproto.HoldGetQuota, srv.lastURL) + } +} + +func TestCheckQuota_OverLimit(t *testing.T) { + // 5 GiB exactly so the formatted message shows "5.00 GB / 5.00 GB". + srv := quotaServer(t, 200, `{"totalSize":5368709120,"limit":5368709120}`) + d := newTestDB(t) + seedUser(t, d, "did:plc:alice", "alice.bsky.social", "") + a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) + + err := a.checkQuota(context.Background(), "did:plc:alice", srv.holdDID) + if err == nil { + t.Fatal("checkQuota at limit should deny") + } + msg := err.Error() + for _, want := range []string{"quota exceeded", "5.00 GB", "did:plc:alice", "alice.bsky.social"} { + if !strings.Contains(msg, want) { + t.Errorf("expected %q in error %q", want, msg) + } + } +} + +// When no users row exists for the DID (handle unknown), the error still +// formats correctly with the bare DID. +func TestCheckQuota_OverLimit_NoHandle(t *testing.T) { + srv := quotaServer(t, 200, `{"totalSize":5368709120,"limit":5368709120}`) + a := New(newTestDB(t), fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) + + err := a.checkQuota(context.Background(), "did:plc:bob", srv.holdDID) + if err == nil { + t.Fatal("checkQuota at limit should deny") + } + msg := err.Error() + if !strings.Contains(msg, "did:plc:bob") || strings.Contains(msg, "(did:") { + t.Errorf("expected bare DID (no parenthesized form) in error %q", msg) + } +} + +func TestCheckQuota_NilLimitAllows(t *testing.T) { + // A user on the unlimited tier has limit == nil. Even huge totalSize + // must not deny. + srv := quotaServer(t, 200, `{"totalSize":99999999}`) + a := New(newTestDB(t), fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) + + if err := a.checkQuota(context.Background(), "did:plc:alice", srv.holdDID); err != nil { + t.Errorf("checkQuota with nil limit = %v, want nil", err) + } +} + +func TestCheckQuota_500FailsOpen(t *testing.T) { + srv := quotaServer(t, 500, `oops`) + a := New(newTestDB(t), fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) + + if err := a.checkQuota(context.Background(), "did:plc:alice", srv.holdDID); err != nil { + t.Errorf("checkQuota with 500 should fail open, got %v", err) + } +} + +func TestCheckQuota_BadJSONFailsOpen(t *testing.T) { + srv := quotaServer(t, 200, `not-json`) + a := New(newTestDB(t), fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) + + if err := a.checkQuota(context.Background(), "did:plc:alice", srv.holdDID); err != nil { + t.Errorf("checkQuota with malformed JSON should fail open, got %v", err) + } +} + +func TestCheckQuota_EncodesUserDID(t *testing.T) { + // did:web DIDs may contain percent-encoded characters (e.g. "%3A" for + // the port colon). Without proper query encoding the receiving server's + // query parser decodes "%3A" → ":", mangling the DID and missing the + // records that were keyed by the original form. The fix encodes the + // DID once at the client side so the server decodes it back exactly. + srv := quotaServer(t, 200, `{"totalSize":100,"limit":1000}`) + a := New(newTestDB(t), fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) + + encodedDID := "did:web:127.0.0.1%3A45397:user:alice.test" + if err := a.checkQuota(context.Background(), encodedDID, srv.holdDID); err != nil { + t.Errorf("checkQuota with encoded DID = %v, want nil", err) + } + // The URL the server saw should contain the double-encoded form, so + // that its single decode pass yields the original DID back. + if !strings.Contains(srv.lastURL, "did%3Aweb%3A127.0.0.1%253A45397%3Auser%3Aalice.test") { + t.Errorf("expected query value to be percent-encoded; got URL %q", srv.lastURL) + } +} + +func TestAuthorize_CaptainBypassesCrewCheck(t *testing.T) { + srv := quotaServer(t, 200, `{"totalSize":1,"limit":1000}`) + d := newTestDB(t) + seedUser(t, d, "did:plc:alice", "alice.test", srv.holdDID) + seedCaptain(t, d, srv.holdDID, "did:plc:alice") + // A contradictory crew row should NOT trip the gate — captain bypass. + seedCrewMember(t, d, srv.holdDID, "did:plc:alice", `["blob:read"]`) + + a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) + if err := a.Authorize(context.Background(), "did:plc:alice", "", pushAccess("alice/x")); err != nil { + t.Errorf("Authorize(captain push) = %v, want nil", err) + } +} + +func TestAuthorize_NonCaptainPushWithoutCrewDenied(t *testing.T) { + srv := quotaServer(t, 200, `{"totalSize":1,"limit":1000}`) + d := newTestDB(t) + seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) + seedCaptain(t, d, srv.holdDID, "did:plc:alice") // alice owns; bob is not crew + + a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) + err := a.Authorize(context.Background(), "did:plc:bob", "", pushAccess("bob/x")) + if err == nil || !strings.Contains(err.Error(), "crew membership required") { + t.Errorf("expected 'crew membership required', got %v", err) + } +} + +func TestAuthorize_NonCaptainPushWithoutBlobWriteDenied(t *testing.T) { + srv := quotaServer(t, 200, `{"totalSize":1,"limit":1000}`) + d := newTestDB(t) + seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) + seedCaptain(t, d, srv.holdDID, "did:plc:alice") + seedCrewMember(t, d, srv.holdDID, "did:plc:bob", `["blob:read"]`) + + a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) + err := a.Authorize(context.Background(), "did:plc:bob", "", pushAccess("bob/x")) + if err == nil || !strings.Contains(err.Error(), "lacks blob:write") { + t.Errorf("expected 'lacks blob:write', got %v", err) + } +} + +func TestAuthorize_NonCaptainPushUnderQuotaAllowed(t *testing.T) { + srv := quotaServer(t, 200, `{"totalSize":100,"limit":1000}`) + d := newTestDB(t) + seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) + seedCaptain(t, d, srv.holdDID, "did:plc:alice") + seedCrewMember(t, d, srv.holdDID, "did:plc:bob", `["blob:write"]`) + + a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) + if err := a.Authorize(context.Background(), "did:plc:bob", "", pushAccess("bob/x")); err != nil { + t.Errorf("Authorize(crew blob:write under quota) = %v, want nil", err) + } +} + +func TestAuthorize_NonCaptainPushOverQuotaDenied(t *testing.T) { + srv := quotaServer(t, 200, `{"totalSize":1000,"limit":1000}`) + d := newTestDB(t) + seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) + seedCaptain(t, d, srv.holdDID, "did:plc:alice") + seedCrewMember(t, d, srv.holdDID, "did:plc:bob", `["blob:write"]`) + + a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) + err := a.Authorize(context.Background(), "did:plc:bob", "", pushAccess("bob/x")) + if err == nil || !strings.Contains(err.Error(), "quota exceeded") { + t.Errorf("expected 'quota exceeded', got %v", err) + } +} + +// An over-quota user has to be able to delete: the denial message tells them +// to, and docker/crane ask for pull,push,delete on a delete. The gate grants +// the non-push subset instead of failing the whole request. +func TestAuthorize_OverQuotaGrantsDeleteWithoutPush(t *testing.T) { + srv := quotaServer(t, 200, `{"totalSize":1000,"limit":1000}`) + d := newTestDB(t) + seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) + seedCaptain(t, d, srv.holdDID, "did:plc:alice") + seedCrewMember(t, d, srv.holdDID, "did:plc:bob", `["blob:write"]`) + + a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) + access := []auth.AccessEntry{ + {Type: "repository", Name: "bob/x", Actions: []string{"pull", "push", "delete"}}, + } + if err := a.Authorize(context.Background(), "did:plc:bob", "", access); err != nil { + t.Fatalf("Authorize(over quota, delete requested) = %v, want nil", err) + } + if got := access[0].Actions; !slices.Equal(got, []string{"pull", "delete"}) { + t.Errorf("granted actions = %v, want [pull delete]", got) + } +} + +// Delete-only never carries push, so it must survive untouched. +func TestAuthorize_OverQuotaAllowsDeleteOnlyScope(t *testing.T) { + srv := quotaServer(t, 200, `{"totalSize":1000,"limit":1000}`) + d := newTestDB(t) + seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) + seedCaptain(t, d, srv.holdDID, "did:plc:alice") + + a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) + access := []auth.AccessEntry{ + {Type: "repository", Name: "bob/x", Actions: []string{"delete"}}, + } + if err := a.Authorize(context.Background(), "did:plc:bob", "", access); err != nil { + t.Fatalf("Authorize(delete only) = %v, want nil", err) + } + if got := access[0].Actions; !slices.Equal(got, []string{"delete"}) { + t.Errorf("granted actions = %v, want [delete]", got) + } + if srv.hits != 0 { + t.Errorf("quota endpoint hit %d times for delete-only scope, want 0", srv.hits) + } +} + +// A plain push must keep failing loudly, otherwise the client never sees the +// quota message and just gets an opaque 401 on the first blob upload. +func TestAuthorize_OverQuotaStillDeniesPlainPush(t *testing.T) { + srv := quotaServer(t, 200, `{"totalSize":1000,"limit":1000}`) + d := newTestDB(t) + seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) + seedCaptain(t, d, srv.holdDID, "did:plc:alice") + seedCrewMember(t, d, srv.holdDID, "did:plc:bob", `["blob:write"]`) + + a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) + access := pushAccess("bob/x") + err := a.Authorize(context.Background(), "did:plc:bob", "", access) + if err == nil || !strings.Contains(err.Error(), "quota exceeded") { + t.Fatalf("expected 'quota exceeded', got %v", err) + } + if got := access[0].Actions; !slices.Equal(got, []string{"pull", "push"}) { + t.Errorf("denied request should leave actions untouched, got %v", got) + } +} + +// Under quota, a delete request keeps its push action. +func TestAuthorize_UnderQuotaKeepsPushAlongsideDelete(t *testing.T) { + srv := quotaServer(t, 200, `{"totalSize":1,"limit":1000}`) + d := newTestDB(t) + seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) + seedCaptain(t, d, srv.holdDID, "did:plc:alice") + seedCrewMember(t, d, srv.holdDID, "did:plc:bob", `["blob:write"]`) + + a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) + access := []auth.AccessEntry{ + {Type: "repository", Name: "bob/x", Actions: []string{"pull", "push", "delete"}}, + } + if err := a.Authorize(context.Background(), "did:plc:bob", "", access); err != nil { + t.Fatalf("Authorize(under quota) = %v, want nil", err) + } + if got := access[0].Actions; !slices.Equal(got, []string{"pull", "push", "delete"}) { + t.Errorf("granted actions = %v, want all three preserved", got) + } +} + +func TestAuthorize_PullOnlySkipsMembershipAndQuota(t *testing.T) { + // Quota server installed but should never be hit: pull bypasses both + // the membership requirement and the quota call. + srv := quotaServer(t, 200, `{"totalSize":1000,"limit":1000}`) + d := newTestDB(t) + seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) + seedCaptain(t, d, srv.holdDID, "did:plc:alice") // bob is NOT captain, NOT crew + + a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) + if err := a.Authorize(context.Background(), "did:plc:bob", "", pullAccess("alice/x")); err != nil { + t.Errorf("Authorize(pull only) = %v, want nil", err) + } + if srv.hits != 0 { + t.Errorf("quota endpoint hit %d times for pull-only request, want 0", srv.hits) + } +} + +func TestAuthorize_WildcardPushTreatedAsPull(t *testing.T) { + srv := quotaServer(t, 200, `{"totalSize":1000,"limit":1000}`) + d := newTestDB(t) + seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) + seedCaptain(t, d, srv.holdDID, "did:plc:alice") + + a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) + wildcard := []auth.AccessEntry{{Type: "repository", Name: "*", Actions: []string{"pull", "push"}}} + if err := a.Authorize(context.Background(), "did:plc:bob", "", wildcard); err != nil { + t.Errorf("Authorize(wildcard push) = %v, want nil (treated as pull)", err) + } + if srv.hits != 0 { + t.Errorf("quota endpoint hit %d times for wildcard scope, want 0", srv.hits) + } +} diff --git a/pkg/appview/authgate/testhelpers_test.go b/pkg/appview/authgate/testhelpers_test.go index 7e85134..ba4cc96 100644 --- a/pkg/appview/authgate/testhelpers_test.go +++ b/pkg/appview/authgate/testhelpers_test.go @@ -3,14 +3,11 @@ package authgate import ( "context" "database/sql" - "net/http" - "net/http/httptest" "testing" "atcr.io/pkg/appview/db" "atcr.io/pkg/atproto" "atcr.io/pkg/auth" - "atcr.io/pkg/testpds" ) // newTestDB returns an in-memory libsql DB with the full appview schema @@ -62,52 +59,6 @@ func seedCrewMember(t *testing.T, d *sql.DB, holdDID, memberDID, permsJSON strin } } -// quotaServerResult captures HTTP traffic the server saw, for assertions. -type quotaServerResult struct { - server *httptest.Server - holdDID string // did:web:127.0.0.1%3APORT form - hits int - lastURL string -} - -// quotaServer spins up an httptest.Server that responds to every request -// with the given status + body, records hit count + last URL, and returns -// both the server URL and the did:web:HOST form that resolves to it. The -// server also serves its own did:web document (not counted in hits) so the -// test-mode identity directory can resolve that DID back to the server. -func quotaServer(t *testing.T, status int, body string) *quotaServerResult { - t.Helper() - if !atproto.TestModeBuild { - t.Fatal("this test resolves a did:web on 127.0.0.1 and needs a `-tags testmode` build") - } - res := "aServerResult{} - mux := http.NewServeMux() - mux.HandleFunc("/.well-known/did.json", func(w http.ResponseWriter, r *http.Request) { - base := "http://" + r.Host - testpds.HoldDIDDocumentHandler(testpds.DIDWebForURL(base), base)(w, r) - }) - mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { - res.hits++ - res.lastURL = r.URL.String() - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(status) - _, _ = w.Write([]byte(body)) - }) - res.server = httptest.NewServer(mux) - t.Cleanup(res.server.Close) - - // httptest.Server.URL has the form "http://127.0.0.1:PORT"; did:web - // percent-encodes the port colon. - res.holdDID = testpds.DIDWebForURL(res.server.URL) - return res -} - -// httpClient returns the server's client, which trusts its TLS cert (n/a -// here since httptest.NewServer is HTTP) and routes to the loopback. -func (r *quotaServerResult) httpClient() *http.Client { - return r.server.Client() -} - // fakeHoldAuthorizer is a no-op auth.HoldAuthorizer stub. The Authorize // orchestration tests don't exercise the reconciliation closure (the // closure is nil for our purposes because we don't supply a refresher diff --git a/pkg/appview/config.go b/pkg/appview/config.go index d9bb37b..0024b49 100644 --- a/pkg/appview/config.go +++ b/pkg/appview/config.go @@ -55,9 +55,6 @@ type ServerConfig struct { // Public-facing URL for OAuth callbacks and JWT realm. BaseURL string `yaml:"base_url" comment:"Public-facing URL for OAuth callbacks and JWT realm. Auto-detected if empty."` - // Allows HTTP (not HTTPS) for DID resolution. - TestMode bool `yaml:"test_mode" comment:"Local development only. Routes pushes to the default hold when the user's chosen hold is unreachable and quiets backfill warnings about external holds. Does not affect DID resolution: that needs a -tags testmode build."` - // Display name shown on OAuth authorization screens. ClientName string `yaml:"client_name" comment:"Display name shown on OAuth authorization screens."` @@ -249,7 +246,6 @@ func setDefaults(v *viper.Viper) { // Server defaults v.SetDefault("server.addr", ":5000") v.SetDefault("server.base_url", "") - v.SetDefault("server.test_mode", false) v.SetDefault("server.client_name", "AT Container Registry") v.SetDefault("server.client_short_name", "ATCR") v.SetDefault("server.registry_domains", []string{}) @@ -486,7 +482,7 @@ func buildDistributionConfig(cfg *Config, v *viper.Viper) (*configuration.Config distConfig.Storage = buildStorageConfig() // Middleware (ATProto resolver) - distConfig.Middleware = buildMiddlewareConfig(cfg.Server.PrimaryHoldDID(), cfg.Server.BaseURL, cfg.Server.TestMode) + distConfig.Middleware = buildMiddlewareConfig(cfg.Server.PrimaryHoldDID(), cfg.Server.BaseURL) // Auth (use values from cfg.Auth) // @@ -566,14 +562,13 @@ func buildStorageConfig() configuration.Storage { } // buildMiddlewareConfig creates middleware configuration -func buildMiddlewareConfig(defaultHoldDID string, baseURL string, testMode bool) map[string][]configuration.Middleware { +func buildMiddlewareConfig(defaultHoldDID string, baseURL string) map[string][]configuration.Middleware { return map[string][]configuration.Middleware{ "registry": { { Name: "atproto-resolver", Options: configuration.Parameters{ "default_hold_did": defaultHoldDID, - "test_mode": testMode, "base_url": baseURL, }, }, diff --git a/pkg/appview/config_test.go b/pkg/appview/config_test.go index 954abcf..6595222 100644 --- a/pkg/appview/config_test.go +++ b/pkg/appview/config_test.go @@ -100,28 +100,17 @@ func TestBuildMiddlewareConfig(t *testing.T) { name string defaultHoldDID string baseURL string - testMode bool - wantTestMode bool }{ { - name: "normal mode", + name: "default hold and base URL", defaultHoldDID: "did:web:hold01.atcr.io", baseURL: "https://atcr.io", - testMode: false, - wantTestMode: false, - }, - { - name: "test mode enabled", - defaultHoldDID: "did:web:hold01.atcr.io", - baseURL: "https://atcr.io", - testMode: true, - wantTestMode: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := buildMiddlewareConfig(tt.defaultHoldDID, tt.baseURL, tt.testMode) + got := buildMiddlewareConfig(tt.defaultHoldDID, tt.baseURL) registryMW, ok := got["registry"] if !ok { @@ -144,10 +133,6 @@ func TestBuildMiddlewareConfig(t *testing.T) { if mw.Options["base_url"] != tt.baseURL { t.Errorf("base_url = %v, want %v", mw.Options["base_url"], tt.baseURL) } - - if mw.Options["test_mode"] != tt.wantTestMode { - t.Errorf("test_mode = %v, want %v", mw.Options["test_mode"], tt.wantTestMode) - } }) } } diff --git a/pkg/appview/holdclient/tier_update_test.go b/pkg/appview/holdclient/tier_update_test.go index 184e206..0c594c6 100644 --- a/pkg/appview/holdclient/tier_update_test.go +++ b/pkg/appview/holdclient/tier_update_test.go @@ -4,13 +4,10 @@ import ( "context" "net/http" "net/http/httptest" - "strings" "sync/atomic" "testing" - "time" "atcr.io/pkg/atproto" - "atcr.io/pkg/testpds" "github.com/bluesky-social/indigo/atproto/atcrypto" ) @@ -89,148 +86,3 @@ func TestUpdateCrewTierOnHold_PostsToEndpoint(t *testing.T) { // The tests above cover the two helpers. UpdateCrewTierOnAllHolds — the // function the Stripe webhook actually calls, and the one whose error decides // whether a paid upgrade is retried or dropped — had no test at all. - -// holdServer starts an httptest server standing in for a hold and returns it -// with the did:web that ResolveHoldDIDToURL maps back to it. The server serves -// its own DID document at /.well-known/did.json (always promptly, whatever the -// hold handler does), and handler gets every other request. Resolving a -// loopback did:web is what a `-tags testmode` build provides. -func holdServer(t *testing.T, handler http.HandlerFunc) (*httptest.Server, string) { - t.Helper() - if !atproto.TestModeBuild { - t.Fatal("this test resolves a did:web on 127.0.0.1 and needs a `-tags testmode` build") - } - mux := http.NewServeMux() - mux.HandleFunc("/.well-known/did.json", func(w http.ResponseWriter, r *http.Request) { - base := "http://" + r.Host - testpds.HoldDIDDocumentHandler(testpds.DIDWebForURL(base), base)(w, r) - }) - mux.Handle("/", handler) - srv := httptest.NewServer(mux) - t.Cleanup(srv.Close) - return srv, testpds.DIDWebForURL(srv.URL) -} - -func testKey(t *testing.T) *atcrypto.PrivateKeyP256 { - t.Helper() - priv, err := atcrypto.GeneratePrivateKeyP256() - if err != nil { - t.Fatalf("generate key: %v", err) - } - return priv -} - -// TestUpdateCrewTierOnAllHolds_JoinedErrorNamesEveryFailingHold: the caller -// 5xxs the Stripe webhook on any non-nil return, and the operator's only -// account of which holds are behind is this error. One failing hold must not -// mask another. -func TestUpdateCrewTierOnAllHolds_JoinedErrorNamesEveryFailingHold(t *testing.T) { - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) - - _, okDID := holdServer(t, func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - }) - _, bad1DID := holdServer(t, func(w http.ResponseWriter, r *http.Request) { - http.Error(w, "down", http.StatusServiceUnavailable) - }) - _, bad2DID := holdServer(t, func(w http.ResponseWriter, r *http.Request) { - http.Error(w, "broken", http.StatusInternalServerError) - }) - - err := UpdateCrewTierOnAllHolds(context.Background(), - []string{okDID, bad1DID, bad2DID}, "did:plc:user", 1, testKey(t), "did:web:appview") - if err == nil { - t.Fatal("expected an error when two of three holds fail") - } - for _, did := range []string{bad1DID, bad2DID} { - if !strings.Contains(err.Error(), did) { - t.Errorf("joined error does not name failing hold %s: %v", did, err) - } - } - if strings.Contains(err.Error(), okDID) { - t.Errorf("joined error names the hold that succeeded (%s): %v", okDID, err) - } -} - -// TestUpdateCrewTierOnAllHolds_SlowHoldDoesNotStarveOthers pins the concurrency -// the function's doc claims. -// -// Contacted serially, one hold that burns the whole deadline means the holds -// after it are never contacted at all — and since the webhook retries in the -// same order, a persistently slow first hold would mean later holds are never -// updated on any delivery. The assertion is that the healthy hold is reached -// even though the slow one is listed first and never answers. -func TestUpdateCrewTierOnAllHolds_SlowHoldDoesNotStarveOthers(t *testing.T) { - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) - - release := make(chan struct{}) - _, slowDID := holdServer(t, func(w http.ResponseWriter, r *http.Request) { - <-release - }) - defer close(release) - - var healthyHits atomic.Int32 - _, healthyDID := holdServer(t, func(w http.ResponseWriter, r *http.Request) { - healthyHits.Add(1) - w.WriteHeader(http.StatusOK) - }) - - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) - defer cancel() - - // Slow hold first: serial contact would spend the entire budget on it. - err := UpdateCrewTierOnAllHolds(ctx, - []string{slowDID, healthyDID}, - "did:plc:user", 1, testKey(t), "did:web:appview") - - if err == nil { - t.Error("expected an error naming the slow hold") - } - if got := healthyHits.Load(); got != 1 { - t.Errorf("healthy hold contacted %d times, want 1 — it was starved by the slow hold", got) - } -} - -// TestUpdateCrewTierOnAllHolds_DeadlineCutsRetriesShort documents a real -// mismatch rather than asserting an intent. -// -// tierUpdateMaxAttempts is 3 and each attempt is bounded by a 5s client -// timeout, so three attempts against a hold that accepts and never answers -// need ~15s. The Stripe webhook allows the whole fan-out 10s. Under a hang the -// budget therefore funds two attempts, never three, and the caller gets the -// context error rather than the "after N attempts" wrapper. If the deadline or -// either constant changes, this test is where the arithmetic gets re-checked. -func TestUpdateCrewTierOnAllHolds_DeadlineCutsRetriesShort(t *testing.T) { - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) - - release := make(chan struct{}) - var attempts atomic.Int32 - _, hungDID := holdServer(t, func(w http.ResponseWriter, r *http.Request) { - attempts.Add(1) - <-release - }) - defer close(release) - - // Deadline deliberately shorter than tierUpdateMaxAttempts would need. - ctx, cancel := context.WithTimeout(context.Background(), 400*time.Millisecond) - defer cancel() - - start := time.Now() - err := UpdateCrewTierOnAllHolds(ctx, []string{hungDID}, - "did:plc:user", 1, testKey(t), "did:web:appview") - elapsed := time.Since(start) - - if err == nil { - t.Fatal("expected an error from a hold that never answers") - } - if elapsed > 3*time.Second { - t.Errorf("fan-out took %v; the context deadline did not abort the retry loop", elapsed) - } - if got := attempts.Load(); got >= int32(tierUpdateMaxAttempts) { - t.Errorf("hung hold was attempted %d times under a deadline that cannot fund %d", - got, tierUpdateMaxAttempts) - } -} diff --git a/pkg/appview/holdclient/tier_update_testmode_test.go b/pkg/appview/holdclient/tier_update_testmode_test.go new file mode 100644 index 0000000..4ffb7a3 --- /dev/null +++ b/pkg/appview/holdclient/tier_update_testmode_test.go @@ -0,0 +1,150 @@ +//go:build testmode + +package holdclient + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" + + "atcr.io/pkg/testpds" + "github.com/bluesky-social/indigo/atproto/atcrypto" +) + +// holdServer starts an httptest server standing in for a hold and returns it +// with the did:web that ResolveHoldDIDToURL maps back to it. The server serves +// its own DID document at /.well-known/did.json (always promptly, whatever the +// hold handler does), and handler gets every other request. Resolving a +// loopback did:web is what a `-tags testmode` build provides, which this +// file's build constraint guarantees. +func holdServer(t *testing.T, handler http.HandlerFunc) (*httptest.Server, string) { + t.Helper() + mux := http.NewServeMux() + mux.HandleFunc("/.well-known/did.json", func(w http.ResponseWriter, r *http.Request) { + base := "http://" + r.Host + testpds.HoldDIDDocumentHandler(testpds.DIDWebForURL(base), base)(w, r) + }) + mux.Handle("/", handler) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + return srv, testpds.DIDWebForURL(srv.URL) +} + +func testKey(t *testing.T) *atcrypto.PrivateKeyP256 { + t.Helper() + priv, err := atcrypto.GeneratePrivateKeyP256() + if err != nil { + t.Fatalf("generate key: %v", err) + } + return priv +} + +// TestUpdateCrewTierOnAllHolds_JoinedErrorNamesEveryFailingHold: the caller +// 5xxs the Stripe webhook on any non-nil return, and the operator's only +// account of which holds are behind is this error. One failing hold must not +// mask another. +func TestUpdateCrewTierOnAllHolds_JoinedErrorNamesEveryFailingHold(t *testing.T) { + _, okDID := holdServer(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + _, bad1DID := holdServer(t, func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "down", http.StatusServiceUnavailable) + }) + _, bad2DID := holdServer(t, func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "broken", http.StatusInternalServerError) + }) + + err := UpdateCrewTierOnAllHolds(context.Background(), + []string{okDID, bad1DID, bad2DID}, "did:plc:user", 1, testKey(t), "did:web:appview") + if err == nil { + t.Fatal("expected an error when two of three holds fail") + } + for _, did := range []string{bad1DID, bad2DID} { + if !strings.Contains(err.Error(), did) { + t.Errorf("joined error does not name failing hold %s: %v", did, err) + } + } + if strings.Contains(err.Error(), okDID) { + t.Errorf("joined error names the hold that succeeded (%s): %v", okDID, err) + } +} + +// TestUpdateCrewTierOnAllHolds_SlowHoldDoesNotStarveOthers pins the concurrency +// the function's doc claims. +// +// Contacted serially, one hold that burns the whole deadline means the holds +// after it are never contacted at all — and since the webhook retries in the +// same order, a persistently slow first hold would mean later holds are never +// updated on any delivery. The assertion is that the healthy hold is reached +// even though the slow one is listed first and never answers. +func TestUpdateCrewTierOnAllHolds_SlowHoldDoesNotStarveOthers(t *testing.T) { + release := make(chan struct{}) + _, slowDID := holdServer(t, func(w http.ResponseWriter, r *http.Request) { + <-release + }) + defer close(release) + + var healthyHits atomic.Int32 + _, healthyDID := holdServer(t, func(w http.ResponseWriter, r *http.Request) { + healthyHits.Add(1) + w.WriteHeader(http.StatusOK) + }) + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + // Slow hold first: serial contact would spend the entire budget on it. + err := UpdateCrewTierOnAllHolds(ctx, + []string{slowDID, healthyDID}, + "did:plc:user", 1, testKey(t), "did:web:appview") + + if err == nil { + t.Error("expected an error naming the slow hold") + } + if got := healthyHits.Load(); got != 1 { + t.Errorf("healthy hold contacted %d times, want 1 — it was starved by the slow hold", got) + } +} + +// TestUpdateCrewTierOnAllHolds_DeadlineCutsRetriesShort documents a real +// mismatch rather than asserting an intent. +// +// tierUpdateMaxAttempts is 3 and each attempt is bounded by a 5s client +// timeout, so three attempts against a hold that accepts and never answers +// need ~15s. The Stripe webhook allows the whole fan-out 10s. Under a hang the +// budget therefore funds two attempts, never three, and the caller gets the +// context error rather than the "after N attempts" wrapper. If the deadline or +// either constant changes, this test is where the arithmetic gets re-checked. +func TestUpdateCrewTierOnAllHolds_DeadlineCutsRetriesShort(t *testing.T) { + release := make(chan struct{}) + var attempts atomic.Int32 + _, hungDID := holdServer(t, func(w http.ResponseWriter, r *http.Request) { + attempts.Add(1) + <-release + }) + defer close(release) + + // Deadline deliberately shorter than tierUpdateMaxAttempts would need. + ctx, cancel := context.WithTimeout(context.Background(), 400*time.Millisecond) + defer cancel() + + start := time.Now() + err := UpdateCrewTierOnAllHolds(ctx, []string{hungDID}, + "did:plc:user", 1, testKey(t), "did:web:appview") + elapsed := time.Since(start) + + if err == nil { + t.Fatal("expected an error from a hold that never answers") + } + if elapsed > 3*time.Second { + t.Errorf("fan-out took %v; the context deadline did not abort the retry loop", elapsed) + } + if got := attempts.Load(); got >= int32(tierUpdateMaxAttempts) { + t.Errorf("hung hold was attempted %d times under a deadline that cannot fund %d", + got, tierUpdateMaxAttempts) + } +} diff --git a/pkg/appview/jetstream/backfill.go b/pkg/appview/jetstream/backfill.go index 695d7f2..8455e65 100644 --- a/pkg/appview/jetstream/backfill.go +++ b/pkg/appview/jetstream/backfill.go @@ -29,7 +29,6 @@ type BackfillWorker struct { endpoints *EndpointRotator processor *Processor // Shared processor for DB operations defaultHoldDID string // Default hold DID from AppView config (e.g., "did:web:hold01.atcr.io") - testMode bool // If true, suppress warnings for external holds refresher *oauth.Refresher // OAuth refresher for PDS writes (optional, can be nil) // captainChecked tracks the last time we successfully fetched each hold's @@ -56,7 +55,7 @@ type BackfillState struct { // defaultHoldDID should be in format "did:web:hold01.atcr.io" // To find a hold's DID, visit: https://hold-url/.well-known/did.json // refresher is optional - if provided, backfill will try to update PDS records when fetching README content -func NewBackfillWorker(database *sql.DB, relayEndpoints []string, defaultHoldDID string, testMode bool, refresher *oauth.Refresher) (*BackfillWorker, error) { +func NewBackfillWorker(database *sql.DB, relayEndpoints []string, defaultHoldDID string, refresher *oauth.Refresher) (*BackfillWorker, error) { if len(relayEndpoints) == 0 { relayEndpoints = []string{"https://relay1.us-east.bsky.network"} } @@ -66,7 +65,6 @@ func NewBackfillWorker(database *sql.DB, relayEndpoints []string, defaultHoldDID endpoints: NewEndpointRotator(relayEndpoints), processor: NewProcessor(database, false, NewStatsCache()), // Stats cache for aggregation defaultHoldDID: defaultHoldDID, - testMode: testMode, refresher: refresher, captainChecked: make(map[string]time.Time), }, nil @@ -479,10 +477,9 @@ func (b *BackfillWorker) processRecordWith(ctx context.Context, proc *Processor, // queryCaptainRecordWrapper wraps queryCaptainRecord with backfill-specific logic func (b *BackfillWorker) queryCaptainRecordWrapper(ctx context.Context, holdDID string) error { if err := b.queryCaptainRecord(ctx, holdDID); err != nil { - // In test mode, only warn about default hold (local hold) + // In a testmode build, only warn about the default (local) hold. // External/production holds may not have captain records yet (dev ahead of prod) - if b.testMode && holdDID != b.defaultHoldDID { - // Suppress warning for external holds in test mode + if atproto.TestModeBuild && holdDID != b.defaultHoldDID { return nil } slog.Warn("Backfill failed to query captain record for hold", "hold_did", holdDID, "error", err) diff --git a/pkg/appview/middleware/registry.go b/pkg/appview/middleware/registry.go index ccadb58..7192e5e 100644 --- a/pkg/appview/middleware/registry.go +++ b/pkg/appview/middleware/registry.go @@ -257,17 +257,17 @@ func init() { // NamespaceResolver wraps a namespace and resolves names type NamespaceResolver struct { distribution.Namespace - defaultHoldDID string // Default hold DID (e.g., "did:web:hold01.atcr.io") - baseURL string // Base URL for error messages (e.g., "https://atcr.io") - testMode bool // If true, fallback to default hold when user's hold is unreachable - refresher *oauth.Refresher // OAuth session manager (copied from global on init) - database storage.HoldDIDLookup // Database for hold DID lookups (copied from global on init) - authorizer auth.HoldAuthorizer // Hold authorization (copied from global on init) - webhookDispatcher storage.PushWebhookDispatcher // Push webhook dispatcher (copied from global on init) - manifestRefChecker storage.ManifestReferenceChecker // Manifest reference checker (copied from global on init) - validationCache *validationCache // Request-level service token cache - readmeFetcher *readme.Fetcher // README fetcher for repo pages - userPrefs UserPrefsCache // Cached sailor profile preferences (copied from global on init) + defaultHoldDID string // Default hold DID (e.g., "did:web:hold01.atcr.io") + baseURL string // Base URL for error messages (e.g., "https://atcr.io") + fallbackUnreachable bool // Fall back to the default hold when the user's hold is unreachable (testmode builds) + refresher *oauth.Refresher // OAuth session manager (copied from global on init) + database storage.HoldDIDLookup // Database for hold DID lookups (copied from global on init) + authorizer auth.HoldAuthorizer // Hold authorization (copied from global on init) + webhookDispatcher storage.PushWebhookDispatcher // Push webhook dispatcher (copied from global on init) + manifestRefChecker storage.ManifestReferenceChecker // Manifest reference checker (copied from global on init) + validationCache *validationCache // Request-level service token cache + readmeFetcher *readme.Fetcher // README fetcher for repo pages + userPrefs UserPrefsCache // Cached sailor profile preferences (copied from global on init) } // initATProtoResolver initializes the name resolution middleware @@ -285,27 +285,21 @@ func initATProtoResolver(ctx context.Context, ns distribution.Namespace, _ drive baseURL = url } - // Check test mode from options (passed via env var) - testMode := false - if tm, ok := options["test_mode"].(bool); ok { - testMode = tm - } - // Copy shared services from globals into the instance // This avoids accessing globals during request handling return &NamespaceResolver{ - Namespace: ns, - defaultHoldDID: defaultHoldDID, - baseURL: baseURL, - testMode: testMode, - refresher: globalRefresher, - database: globalDatabase, - authorizer: globalAuthorizer, - webhookDispatcher: globalWebhookDispatcher, - manifestRefChecker: globalManifestRefChecker, - validationCache: newValidationCache(), - readmeFetcher: readme.NewFetcher(), - userPrefs: globalUserPrefs, + Namespace: ns, + defaultHoldDID: defaultHoldDID, + baseURL: baseURL, + fallbackUnreachable: atproto.TestModeBuild, + refresher: globalRefresher, + database: globalDatabase, + authorizer: globalAuthorizer, + webhookDispatcher: globalWebhookDispatcher, + manifestRefChecker: globalManifestRefChecker, + validationCache: newValidationCache(), + readmeFetcher: readme.NewFetcher(), + userPrefs: globalUserPrefs, }, nil } @@ -769,14 +763,14 @@ func (nr *NamespaceResolver) learnHoldPrefs(ctx context.Context, did, handle, pd } // applyTestModeFallback turns a user's chosen hold into the hold to actually -// use. An empty choice means the appview default. In test mode a chosen hold -// that is not answering also falls back, so a developer whose local hold is -// down can still push. +// use. An empty choice means the appview default. With fallbackUnreachable set +// (testmode builds) a chosen hold that is not answering also falls back, so a +// developer whose local hold is down can still push. func (nr *NamespaceResolver) applyTestModeFallback(ctx context.Context, userHoldDID string) string { if userHoldDID == "" { return nr.defaultHoldDID } - if nr.testMode && !nr.isHoldReachable(ctx, userHoldDID) { + if nr.fallbackUnreachable && !nr.isHoldReachable(ctx, userHoldDID) { slog.Debug("User's defaultHold unreachable, falling back to default", "component", "registry/middleware/testmode", "default_hold", userHoldDID) return nr.defaultHoldDID diff --git a/pkg/appview/middleware/registry_test.go b/pkg/appview/middleware/registry_test.go index 4df30e9..18ed7eb 100644 --- a/pkg/appview/middleware/registry_test.go +++ b/pkg/appview/middleware/registry_test.go @@ -79,16 +79,6 @@ func TestInitATProtoResolver(t *testing.T) { options: map[string]any{ "default_hold_did": "did:web:hold01.atcr.io", "base_url": "https://atcr.io", - "test_mode": false, - }, - wantErr: false, - }, - { - name: "with test mode enabled", - options: map[string]any{ - "default_hold_did": "did:web:hold01.atcr.io", - "base_url": "https://atcr.io", - "test_mode": true, }, wantErr: false, }, @@ -119,9 +109,7 @@ func TestInitATProtoResolver(t *testing.T) { if baseURL, ok := tt.options["base_url"].(string); ok { assert.Equal(t, baseURL, resolver.baseURL) } - if testMode, ok := tt.options["test_mode"].(bool); ok { - assert.Equal(t, testMode, resolver.testMode) - } + assert.Equal(t, atproto.TestModeBuild, resolver.fallbackUnreachable) }) } } @@ -187,7 +175,6 @@ func TestFindHoldDID_SailorProfile(t *testing.T) { resolver := &NamespaceResolver{ defaultHoldDID: "did:web:default.atcr.io", - testMode: false, } ctx := context.Background() @@ -224,7 +211,7 @@ func TestFindHoldDID_Priority(t *testing.T) { assert.Equal(t, "did:web:profile.hold.io", holdDID, "should prioritize sailor profile over hold records") } -// TestFindHoldDID_TestModeFallback tests test mode fallback when hold unreachable +// TestFindHoldDID_TestModeFallback tests the testmode-build fallback when the hold is unreachable func TestFindHoldDID_TestModeFallback(t *testing.T) { // Start a mock PDS server that returns a profile with unreachable hold mockPDS := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -242,15 +229,15 @@ func TestFindHoldDID_TestModeFallback(t *testing.T) { defer mockPDS.Close() resolver := &NamespaceResolver{ - defaultHoldDID: "did:web:default.atcr.io", - testMode: true, // Test mode enabled + defaultHoldDID: "did:web:default.atcr.io", + fallbackUnreachable: true, // what a testmode build sets } ctx := context.Background() holdDID, _ := resolver.findHoldDIDAndPrefs(ctx, "did:plc:test123", "test.example.com", mockPDS.URL) - // In test mode with unreachable hold, should fall back to default - assert.Equal(t, "did:web:default.atcr.io", holdDID, "should fall back to default in test mode when hold unreachable") + // In a testmode build with an unreachable hold, should fall back to default + assert.Equal(t, "did:web:default.atcr.io", holdDID, "should fall back to default in a testmode build when hold unreachable") } // TestIsHoldReachable tests the hold reachability check diff --git a/pkg/appview/server.go b/pkg/appview/server.go index c20be13..9091c0a 100644 --- a/pkg/appview/server.go +++ b/pkg/appview/server.go @@ -226,12 +226,10 @@ func NewAppViewServer(cfg *Config, branding *BrandingOverrides) (*AppViewServer, baseURL := cfg.Server.BaseURL defaultHoldDID := cfg.Server.PrimaryHoldDID() - testMode := cfg.Server.TestMode slog.Debug("Base URL for OAuth", "base_url", baseURL) - if testMode { - slog.Info("TEST_MODE enabled - will use HTTP for local DID resolution") - atproto.SetTestMode(true) + if atproto.TestModeBuild { + slog.Info("testmode build: local did:web resolution and loopback OAuth enabled") } oauthKey, err := loadOAuthKey(s.Database) @@ -276,7 +274,7 @@ func NewAppViewServer(cfg *Config, branding *BrandingOverrides) (*AppViewServer, middleware.SetGlobalLabelChecker(db.NewLabelChecker(s.Database)) // Create RemoteHoldAuthorizer for hold authorization with caching - s.HoldAuthorizer = auth.NewRemoteHoldAuthorizer(s.Database, testMode) + s.HoldAuthorizer = auth.NewRemoteHoldAuthorizer(s.Database) middleware.SetGlobalAuthorizer(s.HoldAuthorizer) slog.Info("Hold authorizer initialized with database caching") @@ -788,7 +786,7 @@ func (s *AppViewServer) ServeWithListener(listener net.Listener) error { case <-stop: slog.Info("Shutting down registry server") - if s.Config.Server.TestMode { + if atproto.TestModeBuild { listener.Close() } @@ -1119,9 +1117,8 @@ func (s *AppViewServer) initializeJetstream(ctx context.Context) { if s.Config.Jetstream.BackfillEnabled { relayEndpoints := s.Config.Jetstream.RelayEndpoints defaultHoldDID := s.Config.Server.PrimaryHoldDID() - testMode := s.Config.Server.TestMode - backfillWorker, err := jetstream.NewBackfillWorker(s.Database, relayEndpoints, defaultHoldDID, testMode, s.Refresher) + backfillWorker, err := jetstream.NewBackfillWorker(s.Database, relayEndpoints, defaultHoldDID, s.Refresher) if err != nil { slog.Warn("Failed to create backfill worker", "component", "jetstream/backfill", "error", err) } else { diff --git a/pkg/atproto/directory.go b/pkg/atproto/directory.go index d2c9660..391f00d 100644 --- a/pkg/atproto/directory.go +++ b/pkg/atproto/directory.go @@ -11,23 +11,8 @@ var ( // call. Tests may swap it out via SetDirectory(). sharedDirectory identity.Directory directoryMu sync.Mutex - - // testMode allows HTTP did:web resolution (IPs, non-TLS) for local development. - // Set via SetTestMode() on startup. - testMode bool ) -// SetTestMode enables relaxed did:web resolution for local development, -// allowing HTTP and IP-based did:web identifiers that the indigo directory rejects. -func SetTestMode(enabled bool) { - testMode = enabled -} - -// IsTestMode returns whether test mode is enabled. -func IsTestMode() bool { - return testMode -} - // SetDirectory replaces the shared identity.Directory used by all resolver // helpers. Intended for tests that wire in a fake directory. Production code // should never call this — leaving the default lazy-initialized indigo diff --git a/pkg/auth/denial_counter_test.go b/pkg/auth/denial_counter_test.go index e6eabdc..a4ad81d 100644 --- a/pkg/auth/denial_counter_test.go +++ b/pkg/auth/denial_counter_test.go @@ -40,7 +40,7 @@ func concurrentTestDB(t *testing.T) *sql.DB { func TestCacheDenialConcurrentIncrementsAreNotLost(t *testing.T) { testDB := concurrentTestDB(t) remote := NewRemoteHoldAuthorizerWithBackoffs( - testDB, false, + testDB, time.Hour, // firstDenialBackoff time.Hour, // cleanupInterval time.Hour, // cleanupGracePeriod @@ -101,7 +101,7 @@ func TestCacheDenialBackoffMatchesLadder(t *testing.T) { testDB := concurrentTestDB(t) ladder := []time.Duration{2 * time.Second, 30 * time.Second, 5 * time.Minute} remote := NewRemoteHoldAuthorizerWithBackoffs( - testDB, false, time.Hour, time.Hour, time.Hour, ladder, + testDB, time.Hour, time.Hour, time.Hour, ladder, ).(*RemoteHoldAuthorizer) defer close(remote.stopCleanup) @@ -158,7 +158,7 @@ func TestCacheDenialBackoffMatchesLadder(t *testing.T) { func TestCacheDenialBlocksAfterPersisting(t *testing.T) { testDB := concurrentTestDB(t) remote := NewRemoteHoldAuthorizerWithBackoffs( - testDB, false, time.Hour, time.Hour, time.Hour, + testDB, time.Hour, time.Hour, time.Hour, []time.Duration{time.Hour}, ).(*RemoteHoldAuthorizer) defer close(remote.stopCleanup) diff --git a/pkg/auth/hold_remote.go b/pkg/auth/hold_remote.go index c9c318d..63c6677 100644 --- a/pkg/auth/hold_remote.go +++ b/pkg/auth/hold_remote.go @@ -27,7 +27,6 @@ type RemoteHoldAuthorizer struct { cacheTTL time.Duration // TTL for captain record cache recentDenials sync.Map // In-memory cache for first denials stopCleanup chan struct{} // Signal to stop cleanup goroutine - testMode bool // If true, use HTTP for local DIDs firstDenialBackoff time.Duration // Backoff duration for first denial (default: 10s) cleanupInterval time.Duration // Cleanup goroutine interval (default: 10s) cleanupGracePeriod time.Duration // Grace period before cleanup (default: 5s) @@ -40,8 +39,8 @@ type denialEntry struct { } // NewRemoteHoldAuthorizer creates a new remote authorizer for AppView with production defaults -func NewRemoteHoldAuthorizer(db *sql.DB, testMode bool) HoldAuthorizer { - return NewRemoteHoldAuthorizerWithBackoffs(db, testMode, +func NewRemoteHoldAuthorizer(db *sql.DB) HoldAuthorizer { + return NewRemoteHoldAuthorizerWithBackoffs(db, 10*time.Second, // firstDenialBackoff 10*time.Second, // cleanupInterval 5*time.Second, // cleanupGracePeriod @@ -56,7 +55,7 @@ func NewRemoteHoldAuthorizer(db *sql.DB, testMode bool) HoldAuthorizer { // NewRemoteHoldAuthorizerWithBackoffs creates a new remote authorizer with custom backoff durations // Used for testing to avoid long sleeps -func NewRemoteHoldAuthorizerWithBackoffs(db *sql.DB, testMode bool, firstDenialBackoff, cleanupInterval, cleanupGracePeriod time.Duration, dbBackoffDurations []time.Duration) HoldAuthorizer { +func NewRemoteHoldAuthorizerWithBackoffs(db *sql.DB, firstDenialBackoff, cleanupInterval, cleanupGracePeriod time.Duration, dbBackoffDurations []time.Duration) HoldAuthorizer { a := &RemoteHoldAuthorizer{ db: db, httpClient: &http.Client{ @@ -64,7 +63,6 @@ func NewRemoteHoldAuthorizerWithBackoffs(db *sql.DB, testMode bool, firstDenialB }, cacheTTL: 1 * time.Hour, // 1 hour cache TTL stopCleanup: make(chan struct{}), - testMode: testMode, firstDenialBackoff: firstDenialBackoff, cleanupInterval: cleanupInterval, cleanupGracePeriod: cleanupGracePeriod, diff --git a/pkg/auth/hold_remote_captain_verify_test.go b/pkg/auth/hold_remote_captain_verify_test.go index fed1896..826e3ce 100644 --- a/pkg/auth/hold_remote_captain_verify_test.go +++ b/pkg/auth/hold_remote_captain_verify_test.go @@ -1,3 +1,5 @@ +//go:build testmode + package auth import ( @@ -29,7 +31,6 @@ import ( // can resolve the DID derived from its URL (see didFromServer) back to it. func captainServer(t *testing.T) *httptest.Server { t.Helper() - requireTestModeBuild(t) mux := http.NewServeMux() mux.HandleFunc("/.well-known/did.json", func(w http.ResponseWriter, r *http.Request) { base := "http://" + r.Host @@ -52,16 +53,6 @@ func captainServer(t *testing.T) *httptest.Server { return srv } -// requireTestModeBuild fails fast, with the reason, when the binary cannot -// resolve a loopback did:web. Without it these tests die on an opaque dial -// error from indigo's hardened directory. -func requireTestModeBuild(t *testing.T) { - t.Helper() - if !atproto.TestModeBuild { - t.Fatal("this test resolves a did:web on 127.0.0.1 and needs a `-tags testmode` build") - } -} - func didFromServer(url string) string { return "did:web:" + strings.ReplaceAll(strings.TrimPrefix(url, "http://"), ":", "%3A") } @@ -79,13 +70,10 @@ func captainRows(t *testing.T, a *RemoteHoldAuthorizer, holdDID string) int { func newVerifyAuthorizer(t *testing.T) *RemoteHoldAuthorizer { t.Helper() - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) return &RemoteHoldAuthorizer{ db: setupTestDB(t), httpClient: &http.Client{Timeout: 5 * time.Second}, cacheTTL: time.Hour, - testMode: true, } } diff --git a/pkg/auth/hold_remote_test.go b/pkg/auth/hold_remote_test.go index 8691d9e..222f5c1 100644 --- a/pkg/auth/hold_remote_test.go +++ b/pkg/auth/hold_remote_test.go @@ -14,22 +14,14 @@ import ( "atcr.io/pkg/atproto" ) -func TestNewRemoteHoldAuthorizer_TestMode(t *testing.T) { - // Test with testMode enabled - authorizer := NewRemoteHoldAuthorizer(nil, true) +func TestNewRemoteHoldAuthorizer(t *testing.T) { + authorizer := NewRemoteHoldAuthorizer(nil) if authorizer == nil { t.Fatal("Expected non-nil authorizer") } - - // Type assertion to access testMode field - remote, ok := authorizer.(*RemoteHoldAuthorizer) - if !ok { + if _, ok := authorizer.(*RemoteHoldAuthorizer); !ok { t.Fatal("Expected *RemoteHoldAuthorizer type") } - - if !remote.testMode { - t.Error("Expected testMode to be true") - } } // setupTestDB creates an in-memory database for testing @@ -89,7 +81,6 @@ func TestFetchCaptainRecordFromXRPC(t *testing.T) { // Create authorizer with test server URL as the hold DID remote := &RemoteHoldAuthorizer{ httpClient: &http.Client{Timeout: 10 * time.Second}, - testMode: true, } // Override resolveDIDToURL to return test server URL @@ -116,7 +107,6 @@ func TestGetCaptainRecord_CacheHit(t *testing.T) { httpClient: &http.Client{ Timeout: 10 * time.Second, }, - testMode: false, } holdDID := "did:web:hold01.atcr.io" @@ -161,7 +151,6 @@ func TestIsCrewMember_ApprovalCacheHit(t *testing.T) { httpClient: &http.Client{ Timeout: 10 * time.Second, }, - testMode: false, } holdDID := "did:web:hold01.atcr.io" @@ -191,7 +180,6 @@ func TestIsCrewMember_DenialBackoff_FirstDenial(t *testing.T) { // Create authorizer with fast backoffs for testing (10ms instead of 10s) remote := NewRemoteHoldAuthorizerWithBackoffs( testDB, - false, // testMode 10*time.Millisecond, // firstDenialBackoff (10ms instead of 10s) 50*time.Millisecond, // cleanupInterval (50ms instead of 10s) 50*time.Millisecond, // cleanupGracePeriod (50ms instead of 5s) @@ -240,7 +228,7 @@ func TestIsCrewMember_DenialBackoff_FirstDenial(t *testing.T) { func TestGetBackoffDuration(t *testing.T) { // Create authorizer with production backoff durations testDB := setupTestDB(t) - remote := NewRemoteHoldAuthorizer(testDB, false).(*RemoteHoldAuthorizer) + remote := NewRemoteHoldAuthorizer(testDB).(*RemoteHoldAuthorizer) defer close(remote.stopCleanup) tests := []struct { @@ -296,7 +284,7 @@ func TestCheckReadAccess_PublicHold(t *testing.T) { func TestClearCrewDenial_InMemory(t *testing.T) { testDB := setupTestDB(t) remote := NewRemoteHoldAuthorizerWithBackoffs( - testDB, false, + testDB, 10*time.Millisecond, // firstDenialBackoff 50*time.Millisecond, // cleanupInterval 50*time.Millisecond, // cleanupGracePeriod @@ -332,7 +320,7 @@ func TestClearCrewDenial_InMemory(t *testing.T) { func TestClearCrewDenial_Database(t *testing.T) { testDB := setupTestDB(t) remote := NewRemoteHoldAuthorizerWithBackoffs( - testDB, false, + testDB, 10*time.Millisecond, // firstDenialBackoff 50*time.Millisecond, // cleanupInterval 50*time.Millisecond, // cleanupGracePeriod @@ -372,7 +360,7 @@ func TestClearCrewDenial_Database(t *testing.T) { func TestDeniedUserBecomesCrewImmediateAccess(t *testing.T) { testDB := setupTestDB(t) remote := NewRemoteHoldAuthorizerWithBackoffs( - testDB, false, + testDB, 1*time.Hour, // Long backoff to ensure test would fail without fix 50*time.Millisecond, 50*time.Millisecond, @@ -409,7 +397,7 @@ func TestDeniedUserBecomesCrewImmediateAccess(t *testing.T) { func TestClearAllDenials_OnStartup(t *testing.T) { testDB := setupTestDB(t) remote := NewRemoteHoldAuthorizerWithBackoffs( - testDB, false, + testDB, 1*time.Hour, // Long backoff 50*time.Millisecond, 50*time.Millisecond, @@ -449,7 +437,7 @@ func TestClearAllDenials_OnStartup(t *testing.T) { func TestIsCachedCrewMember_Hit(t *testing.T) { testDB := setupTestDB(t) - remote := NewRemoteHoldAuthorizer(testDB, false).(*RemoteHoldAuthorizer) + remote := NewRemoteHoldAuthorizer(testDB).(*RemoteHoldAuthorizer) defer close(remote.stopCleanup) holdDID := "did:web:hold01.atcr.io" @@ -470,7 +458,7 @@ func TestIsCachedCrewMember_Hit(t *testing.T) { func TestIsCachedCrewMember_Miss(t *testing.T) { testDB := setupTestDB(t) - remote := NewRemoteHoldAuthorizer(testDB, false).(*RemoteHoldAuthorizer) + remote := NewRemoteHoldAuthorizer(testDB).(*RemoteHoldAuthorizer) defer close(remote.stopCleanup) cached, err := remote.IsCachedCrewMember(context.Background(), @@ -485,7 +473,7 @@ func TestIsCachedCrewMember_Miss(t *testing.T) { func TestIsCachedCrewMember_Expired(t *testing.T) { testDB := setupTestDB(t) - remote := NewRemoteHoldAuthorizer(testDB, false).(*RemoteHoldAuthorizer) + remote := NewRemoteHoldAuthorizer(testDB).(*RemoteHoldAuthorizer) defer close(remote.stopCleanup) holdDID := "did:web:hold01.atcr.io" @@ -523,7 +511,7 @@ func TestIsCachedCrewMember_Expired(t *testing.T) { func TestRecordCrewApproval_WritesAndReadsBack(t *testing.T) { testDB := setupTestDB(t) - remote := NewRemoteHoldAuthorizer(testDB, false).(*RemoteHoldAuthorizer) + remote := NewRemoteHoldAuthorizer(testDB).(*RemoteHoldAuthorizer) defer close(remote.stopCleanup) holdDID := "did:web:hold01.atcr.io" @@ -555,7 +543,7 @@ func TestRecordCrewApproval_WritesAndReadsBack(t *testing.T) { } func TestIsCachedCrewMember_NoDB(t *testing.T) { - remote := NewRemoteHoldAuthorizer(nil, false).(*RemoteHoldAuthorizer) + remote := NewRemoteHoldAuthorizer(nil).(*RemoteHoldAuthorizer) defer close(remote.stopCleanup) cached, err := remote.IsCachedCrewMember(context.Background(), diff --git a/pkg/billing/tier_resolution_test.go b/pkg/billing/tier_resolution_test.go index 921e2d9..2644cb7 100644 --- a/pkg/billing/tier_resolution_test.go +++ b/pkg/billing/tier_resolution_test.go @@ -3,16 +3,8 @@ package billing import ( - "encoding/json" - "fmt" - "net/http" - "net/http/httptest" "testing" - "time" - "atcr.io/pkg/atproto" - "atcr.io/pkg/testpds" - "github.com/bluesky-social/indigo/atproto/atcrypto" "github.com/stripe/stripe-go/v84" ) @@ -99,101 +91,3 @@ func TestResolveTier_UnknownEverythingIsUnresolved(t *testing.T) { t.Errorf("resolveTier(nil) = (%q, %d), want (\"\", -1)", name, rank) } } - -// managedHoldServer starts an httptest server standing in for a managed hold -// and returns it with the did:web the tier fan-out resolves back to it. The -// server serves its own DID document at /.well-known/did.json; handler gets -// every other request. Resolving a loopback did:web needs a `-tags testmode` -// build, so the test fails fast with that message otherwise. -func managedHoldServer(t *testing.T, handler http.HandlerFunc) (*httptest.Server, string) { - t.Helper() - if !atproto.TestModeBuild { - t.Fatal("this test resolves a did:web on 127.0.0.1 and needs a `-tags testmode` build") - } - mux := http.NewServeMux() - mux.HandleFunc("/.well-known/did.json", func(w http.ResponseWriter, r *http.Request) { - base := "http://" + r.Host - testpds.HoldDIDDocumentHandler(testpds.DIDWebForURL(base), base)(w, r) - }) - mux.Handle("/", handler) - srv := httptest.NewServer(mux) - t.Cleanup(srv.Close) - return srv, testpds.DIDWebForURL(srv.URL) -} - -// TestHandleSubscriptionChange_GrandfatheredSubscriberKeepsTier runs the same -// property through the real webhook path, so it covers the payload shape too: -// Stripe sends price.product as a bare ID string, and this fails if that is -// ever read wrongly. -// -// It asserts on the rank the managed hold is actually asked to apply. An -// earlier draft asserted that the event was recorded as processed, and passed -// against price-only resolution — because an unresolved tier ALSO records the -// event and returns nil. That is the defect itself, so any assertion it -// satisfies cannot be measuring the fix. -func TestHandleSubscriptionChange_GrandfatheredSubscriberKeepsTier(t *testing.T) { - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) - - const secret = "whsec_grandfather_test" - m, _ := newTestManager(t, secret) - m.cfg.Tiers = tierTestConfig().Tiers - - stripeAPIReturning(t, http.StatusOK, - `{"id":"cus_gf","object":"customer","metadata":{"user_did":"did:plc:grandfathered"}}`) - - type tierPush struct { - UserDID string `json:"userDid"` - TierRank int `json:"tierRank"` - } - pushes := make(chan tierPush, 4) - _, holdDID := managedHoldServer(t, func(w http.ResponseWriter, r *http.Request) { - var got tierPush - _ = json.NewDecoder(r.Body).Decode(&got) - pushes <- got - w.WriteHeader(http.StatusOK) - }) - m.managedHolds = []string{holdDID} - - priv, err := atcrypto.GeneratePrivateKeyP256() - if err != nil { - t.Fatalf("generate key: %v", err) - } - m.privateKey = priv - - // price_supporter_v1_retired appears nowhere in the config. Only its - // product does — this is the subscriber a price change left behind. - payload := fmt.Sprintf(`{ - "id": "evt_grandfathered", - "object": "event", - "api_version": %q, - "type": "customer.subscription.updated", - "created": %d, - "data": {"object": { - "id": "sub_gf", - "object": "subscription", - "status": "active", - "customer": "cus_gf", - "items": {"object":"list","data":[{"price":{ - "id":"price_supporter_v1_retired", - "product":"prod_supporter" - }}]} - }} - }`, stripe.APIVersion, time.Now().Unix()) - - if err := postWebhook(t, m, secret, []byte(payload)); err != nil { - t.Fatalf("webhook: %v", err) - } - - select { - case got := <-pushes: - if got.TierRank != 1 { - t.Errorf("hold was asked for tierRank %d, want 1 (supporter)", got.TierRank) - } - if got.UserDID != "did:plc:grandfathered" { - t.Errorf("hold was asked to update %q, want did:plc:grandfathered", got.UserDID) - } - case <-time.After(2 * time.Second): - t.Fatal("no tier push reached the hold — the grandfathered subscriber's tier was never applied") - } -} diff --git a/pkg/billing/tier_resolution_testmode_test.go b/pkg/billing/tier_resolution_testmode_test.go new file mode 100644 index 0000000..02cc6eb --- /dev/null +++ b/pkg/billing/tier_resolution_testmode_test.go @@ -0,0 +1,108 @@ +//go:build billing && testmode + +package billing + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "testing" + "time" + + "atcr.io/pkg/testpds" + "github.com/bluesky-social/indigo/atproto/atcrypto" + "github.com/stripe/stripe-go/v84" +) + +// managedHoldServer starts an httptest server standing in for a managed hold +// and returns it with the did:web the tier fan-out resolves back to it. The +// server serves its own DID document at /.well-known/did.json; handler gets +// every other request. Resolving a loopback did:web needs a `-tags testmode` +// build, which this file's build constraint guarantees. +func managedHoldServer(t *testing.T, handler http.HandlerFunc) (*httptest.Server, string) { + t.Helper() + mux := http.NewServeMux() + mux.HandleFunc("/.well-known/did.json", func(w http.ResponseWriter, r *http.Request) { + base := "http://" + r.Host + testpds.HoldDIDDocumentHandler(testpds.DIDWebForURL(base), base)(w, r) + }) + mux.Handle("/", handler) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + return srv, testpds.DIDWebForURL(srv.URL) +} + +// TestHandleSubscriptionChange_GrandfatheredSubscriberKeepsTier runs the same +// property through the real webhook path, so it covers the payload shape too: +// Stripe sends price.product as a bare ID string, and this fails if that is +// ever read wrongly. +// +// It asserts on the rank the managed hold is actually asked to apply. An +// earlier draft asserted that the event was recorded as processed, and passed +// against price-only resolution — because an unresolved tier ALSO records the +// event and returns nil. That is the defect itself, so any assertion it +// satisfies cannot be measuring the fix. +func TestHandleSubscriptionChange_GrandfatheredSubscriberKeepsTier(t *testing.T) { + const secret = "whsec_grandfather_test" + m, _ := newTestManager(t, secret) + m.cfg.Tiers = tierTestConfig().Tiers + + stripeAPIReturning(t, http.StatusOK, + `{"id":"cus_gf","object":"customer","metadata":{"user_did":"did:plc:grandfathered"}}`) + + type tierPush struct { + UserDID string `json:"userDid"` + TierRank int `json:"tierRank"` + } + pushes := make(chan tierPush, 4) + _, holdDID := managedHoldServer(t, func(w http.ResponseWriter, r *http.Request) { + var got tierPush + _ = json.NewDecoder(r.Body).Decode(&got) + pushes <- got + w.WriteHeader(http.StatusOK) + }) + m.managedHolds = []string{holdDID} + + priv, err := atcrypto.GeneratePrivateKeyP256() + if err != nil { + t.Fatalf("generate key: %v", err) + } + m.privateKey = priv + + // price_supporter_v1_retired appears nowhere in the config. Only its + // product does — this is the subscriber a price change left behind. + payload := fmt.Sprintf(`{ + "id": "evt_grandfathered", + "object": "event", + "api_version": %q, + "type": "customer.subscription.updated", + "created": %d, + "data": {"object": { + "id": "sub_gf", + "object": "subscription", + "status": "active", + "customer": "cus_gf", + "items": {"object":"list","data":[{"price":{ + "id":"price_supporter_v1_retired", + "product":"prod_supporter" + }}]} + }} + }`, stripe.APIVersion, time.Now().Unix()) + + if err := postWebhook(t, m, secret, []byte(payload)); err != nil { + t.Fatalf("webhook: %v", err) + } + + select { + case got := <-pushes: + if got.TierRank != 1 { + t.Errorf("hold was asked for tierRank %d, want 1 (supporter)", got.TierRank) + } + if got.UserDID != "did:plc:grandfathered" { + t.Errorf("hold was asked to update %q, want did:plc:grandfathered", got.UserDID) + } + case <-time.After(2 * time.Second): + t.Fatal("no tier push reached the hold — the grandfathered subscriber's tier was never applied") + } +} diff --git a/pkg/billing/webhook_retry_test.go b/pkg/billing/webhook_retry_test.go index af8c83b..96f168c 100644 --- a/pkg/billing/webhook_retry_test.go +++ b/pkg/billing/webhook_retry_test.go @@ -15,8 +15,6 @@ import ( "time" appdb "atcr.io/pkg/appview/db" - "atcr.io/pkg/atproto" - "github.com/bluesky-social/indigo/atproto/atcrypto" "github.com/stripe/stripe-go/v84" "github.com/stripe/stripe-go/v84/webhook" ) @@ -287,46 +285,3 @@ func TestHandleSubscriptionChange_NilCustomerDoesNotPanic(t *testing.T) { t.Errorf("HandleWebhook error = %v, want nil for an event with no customer", err) } } - -// TestHandleSubscriptionChange_HoldFanoutFailureIsRetryable closes the loop the -// other tests in this file only cover one half of. -// -// The tier is resolved, the customer is known, and the only thing that fails is -// the push to the managed hold. That has to reach Stripe as a 5xx and leave -// stripe_processed_events empty: a hold that is briefly down otherwise costs -// the customer their tier permanently, which is the same shape of loss as the -// customer-lookup hole above, one layer further out. -func TestHandleSubscriptionChange_HoldFanoutFailureIsRetryable(t *testing.T) { - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) - - const secret = "whsec_fanout_test" - m, database := newTestManager(t, secret) - - // The customer resolves cleanly — this test is about what happens after. - stripeAPIReturning(t, http.StatusOK, - `{"id":"cus_fanout","object":"customer","metadata":{"user_did":"did:plc:fanoutuser"}}`) - - _, holdDID := managedHoldServer(t, func(w http.ResponseWriter, r *http.Request) { - http.Error(w, "hold is down", http.StatusServiceUnavailable) - }) - m.managedHolds = []string{holdDID} - - priv, err := atcrypto.GeneratePrivateKeyP256() - if err != nil { - t.Fatalf("generate key: %v", err) - } - m.privateKey = priv - - err = postWebhook(t, m, secret, - signedSubscriptionEvent(t, secret, "evt_fanout_fail", "cus_fanout", time.Now().Unix())) - if err == nil { - t.Fatal("a hold that cannot be updated must fail the webhook so Stripe redelivers") - } - if !strings.Contains(err.Error(), "push tier to managed holds") { - t.Errorf("error does not identify the fan-out as the cause: %v", err) - } - if n := processedCount(t, database); n != 0 { - t.Errorf("stripe_processed_events holds %d rows; a failed event must stay redeliverable", n) - } -} diff --git a/pkg/billing/webhook_retry_testmode_test.go b/pkg/billing/webhook_retry_testmode_test.go new file mode 100644 index 0000000..5c85391 --- /dev/null +++ b/pkg/billing/webhook_retry_testmode_test.go @@ -0,0 +1,52 @@ +//go:build billing && testmode + +package billing + +import ( + "net/http" + "strings" + "testing" + "time" + + "github.com/bluesky-social/indigo/atproto/atcrypto" +) + +// TestHandleSubscriptionChange_HoldFanoutFailureIsRetryable closes the loop the +// other tests in this file only cover one half of. +// +// The tier is resolved, the customer is known, and the only thing that fails is +// the push to the managed hold. That has to reach Stripe as a 5xx and leave +// stripe_processed_events empty: a hold that is briefly down otherwise costs +// the customer their tier permanently, which is the same shape of loss as the +// customer-lookup hole above, one layer further out. +func TestHandleSubscriptionChange_HoldFanoutFailureIsRetryable(t *testing.T) { + const secret = "whsec_fanout_test" + m, database := newTestManager(t, secret) + + // The customer resolves cleanly — this test is about what happens after. + stripeAPIReturning(t, http.StatusOK, + `{"id":"cus_fanout","object":"customer","metadata":{"user_did":"did:plc:fanoutuser"}}`) + + _, holdDID := managedHoldServer(t, func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "hold is down", http.StatusServiceUnavailable) + }) + m.managedHolds = []string{holdDID} + + priv, err := atcrypto.GeneratePrivateKeyP256() + if err != nil { + t.Fatalf("generate key: %v", err) + } + m.privateKey = priv + + err = postWebhook(t, m, secret, + signedSubscriptionEvent(t, secret, "evt_fanout_fail", "cus_fanout", time.Now().Unix())) + if err == nil { + t.Fatal("a hold that cannot be updated must fail the webhook so Stripe redelivers") + } + if !strings.Contains(err.Error(), "push tier to managed holds") { + t.Errorf("error does not identify the fan-out as the cause: %v", err) + } + if n := processedCount(t, database); n != 0 { + t.Errorf("stripe_processed_events holds %d rows; a failed event must stay redeliverable", n) + } +} diff --git a/pkg/hold/config.go b/pkg/hold/config.go index 2b9763a..9123565 100644 --- a/pkg/hold/config.go +++ b/pkg/hold/config.go @@ -160,9 +160,6 @@ type ServerConfig struct { // DID of successor hold for migration. Successor string `yaml:"successor" comment:"DID of successor hold for migration. Appview redirects all requests to the successor."` - // Use localhost for OAuth redirects during development. - TestMode bool `yaml:"test_mode" comment:"Local development only. Skips relay crawl requests (a local hold is not reachable by public relays) and tolerates an appview token issuer that differs from appview_did. Does not affect DID resolution: that needs a -tags testmode build."` - // Relay endpoints used primarily for proactive scan discovery via // com.atproto.sync.listReposByCollection. Endpoints listed here MUST // support listReposByCollection. They are also sent requestCrawl on @@ -257,7 +254,6 @@ func setHoldDefaults(v *viper.Viper) { v.SetDefault("server.public_url", "") v.SetDefault("server.public", false) v.SetDefault("server.successor", "") - v.SetDefault("server.test_mode", false) v.SetDefault("server.relay_endpoints", []string{ "https://relay1.us-east.bsky.network", "https://relay1.us-west.bsky.network", diff --git a/pkg/hold/config_test.go b/pkg/hold/config_test.go index 734f0e0..a9373c8 100644 --- a/pkg/hold/config_test.go +++ b/pkg/hold/config_test.go @@ -46,7 +46,6 @@ func TestLoadConfig_Success(t *testing.T) { "HOLD_SERVER_PUBLIC_URL": "https://hold.example.com", "HOLD_SERVER_ADDR": ":9000", "HOLD_SERVER_PUBLIC": "true", - "HOLD_SERVER_TEST_MODE": "true", "HOLD_REGISTRATION_OWNER_DID": "did:plc:owner123", "HOLD_REGISTRATION_ALLOW_ALL_CREW": "true", "S3_BUCKET": "test-bucket", @@ -72,9 +71,6 @@ func TestLoadConfig_Success(t *testing.T) { if !cfg.Server.Public { t.Error("Expected Public=true") } - if !cfg.Server.TestMode { - t.Error("Expected TestMode=true") - } if cfg.Server.ReadTimeout != 5*time.Minute { t.Errorf("Expected ReadTimeout=5m, got %v", cfg.Server.ReadTimeout) } @@ -131,7 +127,6 @@ func TestLoadConfig_Defaults(t *testing.T) { // Don't set optional vars - test defaults "HOLD_SERVER_ADDR": "", "HOLD_SERVER_PUBLIC": "", - "HOLD_SERVER_TEST_MODE": "", "HOLD_REGISTRATION_OWNER_DID": "", "HOLD_REGISTRATION_ALLOW_ALL_CREW": "", "AWS_REGION": "", @@ -151,9 +146,6 @@ func TestLoadConfig_Defaults(t *testing.T) { if cfg.Server.Public { t.Error("Expected default Public=false") } - if cfg.Server.TestMode { - t.Error("Expected default TestMode=false") - } if cfg.Registration.OwnerDID != "" { t.Error("Expected default OwnerDID to be empty") } diff --git a/pkg/hold/pds/appview_token_test.go b/pkg/hold/pds/appview_token_test.go index 8b8157b..997bedd 100644 --- a/pkg/hold/pds/appview_token_test.go +++ b/pkg/hold/pds/appview_token_test.go @@ -1,3 +1,5 @@ +//go:build testmode + package pds import ( @@ -7,7 +9,6 @@ import ( "strings" "testing" - "atcr.io/pkg/atproto" "atcr.io/pkg/auth" "atcr.io/pkg/testpds" @@ -41,12 +42,6 @@ func newAppviewTestEnvWithKey(t *testing.T, priv atcrypto.PrivateKey) *appviewTe t.Fatalf("public key: %v", err) } - // fetchAppviewPublicKey resolves the appview DID through the identity - // directory; only a `-tags testmode` build resolves a loopback did:web. - if !atproto.TestModeBuild { - t.Fatal("this test resolves a did:web on 127.0.0.1 and needs a `-tags testmode` build") - } - // The document mirrors what the real appview serves: the key is // controlled by the document's own DID, so the directory keeps it. mux := http.NewServeMux() diff --git a/pkg/hold/pds/auth.go b/pkg/hold/pds/auth.go index b92a1a3..22a64e1 100644 --- a/pkg/hold/pds/auth.go +++ b/pkg/hold/pds/auth.go @@ -641,13 +641,14 @@ func ValidateAppviewToken(r *http.Request, appviewDID, holdDID string) (string, } // Verify issuer matches configured appview DID. - // In test mode the appview and hold often address each other under different - // did:web hosts (browser-facing 127.0.0.1 vs docker bridge IP), so the label - // won't match — signature verification below still uses the configured - // appviewDID's public key, so a forged token from another signer would fail. + // In a testmode build the appview and hold often address each other under + // different did:web hosts (browser-facing 127.0.0.1 vs docker bridge IP), so + // the label won't match — signature verification below still uses the + // configured appviewDID's public key, so a forged token from another signer + // would fail. if claims.Issuer != appviewDID { - if atproto.IsTestMode() { - slog.Warn("Appview token issuer mismatch tolerated in test mode", + if atproto.TestModeBuild { + slog.Warn("Appview token issuer mismatch tolerated in testmode build", "expected", appviewDID, "got", claims.Issuer) } else { return "", fmt.Errorf("token issuer mismatch: expected %s, got %s", appviewDID, claims.Issuer) diff --git a/pkg/hold/pds/crew_tier_test.go b/pkg/hold/pds/crew_tier_test.go index 6f48411..fd58aa5 100644 --- a/pkg/hold/pds/crew_tier_test.go +++ b/pkg/hold/pds/crew_tier_test.go @@ -1,3 +1,5 @@ +//go:build testmode + package pds import ( diff --git a/pkg/hold/server.go b/pkg/hold/server.go index bdb9f63..e40de4f 100644 --- a/pkg/hold/server.go +++ b/pkg/hold/server.go @@ -104,10 +104,6 @@ func NewHoldServer(cfg *Config) (*HoldServer, error) { Config: cfg, } - if cfg.Server.TestMode { - atproto.SetTestMode(true) - } - // Initialize embedded PDS if database path is configured var xrpcHandler *pds.XRPCHandler var s3Service *s3.S3Service @@ -412,11 +408,11 @@ func (s *HoldServer) ServeWithListener(listener net.Listener) error { // Request crawl from every known relay (plus any custom endpoint) so the // embedded PDS becomes discoverable. Without this, did:web holds are // invisible to relays — and to any appview that backfills via them. - // Skipped in test_mode: local dev holds aren't reachable by public relays. - if !s.Config.Server.TestMode { + // Skipped in a testmode build: local dev holds aren't reachable by public relays. + if !atproto.TestModeBuild { go s.requestCrawls() } else { - slog.Info("Skipping relay crawl requests (test_mode enabled)") + slog.Info("Skipping relay crawl requests (testmode build)") } // Start garbage collector (runs on startup + nightly) diff --git a/test/e2e/README.md b/test/e2e/README.md index 50ac57d..4b46cd6 100644 --- a/test/e2e/README.md +++ b/test/e2e/README.md @@ -35,8 +35,7 @@ against `localhost:5000` measures the redirect, not the endpoint. `ui.sessions` (`pkg/hold/admin/admin.go`), not the `admin_sessions` table, which is vestigial for this path. Air rebuilds the hold whenever tracked source changes — including a batch checkout — so budget one interactive login per -switch. There is no test-mode bypass; `server.test_mode` only affects OAuth -redirect URLs. +switch. There is no test-mode bypass. **Never drive the admin panel with curl.** Sessions are pinned to User-Agent and client IP prefix, and a mismatch does not merely reject the request — it calls