amadan

oficina / meet Public

config: fix round 1 - delete dead field, prove the with-relay path

- Deleted Config.RelayConfigured: nothing read it once resolve owned
  relayConfigured, so its comment was already a lie.
- config_test.go no longer prints a parsed region on failure (len and
  relayConfigured only); the outside-development refusal test now
  also asserts config.go's own wording, not just relay.Parse's.
- New TestAMalformedRelayBuiltFromTheVectorNeverLeaksIt: the
  config-layer regression test for relay's item 1, built from the
  vector key in region, key and username_suffix at once with an
  empty urls list.
- New internal/meet/app_relay_test.go: TestHandle.SetRelay flipping
  relayConfigured through a real build(), build()'s own INFO line
  asserted to carry the region count and not the key, SetRelay's
  type-consistency with build's initial atomic.Value store (swapping
  to a relay and back to none without a panic), and a malformed
  SetRelay leaving the existing regions untouched.
- New internal/meettest/relay_floor_test.go: a room-page floor test
  (there was none), and SetRelay hiding the floor sentence on both
  the holding and room pages - the "relay configured, so sentence
  absent" direction a hardcoded Floor would still have passed.

The with-relay wiring in app.go/config.go was already correct (the
Task 2 review's own finding); this round is new coverage, not a
production fix, except for the deleted field. Verified the config
regression test above fails against the pre-fix relay.go and passes
against the fixed one.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CzvnuKhLzudvq7jZ7SvVt2
Paul Campbell pushed by paul@keymail.dev 62853ed8cf54b8bbf0bda355409c0f33645f0f11 parent 991352e
4 files changed, +227 −5
  • internal/meet/app_relay_test.go +126 −0
  • internal/meet/config.go +0 −4
  • internal/meet/config_test.go +39 −1
  • internal/meettest/relay_floor_test.go +62 −0

CI passed — run details

diff --git a/internal/meet/app_relay_test.go b/internal/meet/app_relay_test.go
new file mode 100644
index 0000000..12efd9d
--- /dev/null
+++ b/internal/meet/app_relay_test.go
@@ -0,0 +1,126 @@
+package meet
+
+import (
+ "bytes"
+ "context"
+ "io"
+ "log/slog"
+ "path/filepath"
+ "strings"
+ "testing"
+
+ "amadan.net/rastrillo/rastrillo/db"
+)
+
+// openRelayTestDB is a fresh temp database for the app-level relay
+// wiring tests below: build() and NewForTest need a real *db.DB, not
+// the bare Config resolve() takes.
+func openRelayTestDB(t *testing.T, logger *slog.Logger) *db.DB {
+ t.Helper()
+ d, err := db.Open(filepath.Join(t.TempDir(), "relay.db"), logger)
+ if err != nil {
+ t.Fatal(err)
+ }
+ t.Cleanup(func() { d.Close() })
+ return d
+}
+
+// TestSetRelayFlipsRelayConfigured is the with-relay path through
+// build: nothing before this exercised SetRelay at all — the config
+// package tests only ever checked resolved.relayConfigured, which
+// build() never reads back through the app's own atomic.
+func TestSetRelayFlipsRelayConfigured(t *testing.T) {
+ cfg := minimalConfig(t)
+ cfg.Env = EnvDevelopment
+ d := openRelayTestDB(t, slog.New(slog.NewTextHandler(io.Discard, nil)))
+ _, ctl, err := NewForTest(d, cfg)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if ctl.a.relayConfigured() {
+ t.Fatal("expected no relay at boot")
+ }
+ if err := ctl.SetRelay(relayVector(t)); err != nil {
+ t.Fatal(err)
+ }
+ if !ctl.a.relayConfigured() || len(ctl.a.relayRegions()) != 1 {
+ t.Fatalf("SetRelay did not flip relayConfigured: configured=%v regions=%d", ctl.a.relayConfigured(), len(ctl.a.relayRegions()))
+ }
+}
+
+// TestBuildLogsOneINFOLineWithTheRegionCountAndNotTheKey: build()'s own
+// INFO line was untested — make ci passed with a hardcoded Floor and no
+// assertion on the log at all, so a mutation that dropped the line, or
+// one that logged the raw contract instead of its length, would have
+// survived.
+func TestBuildLogsOneINFOLineWithTheRegionCountAndNotTheKey(t *testing.T) {
+ var buf bytes.Buffer
+ logger := slog.New(slog.NewTextHandler(&buf, nil))
+ cfg := minimalConfig(t)
+ cfg.Logger = logger
+ cfg.Relay = relayVector(t)
+ d := openRelayTestDB(t, logger)
+ if _, _, err := build(context.Background(), d, cfg); err != nil {
+ t.Fatal(err)
+ }
+ log := buf.String()
+ if !strings.Contains(log, "meet: relay configured") || !strings.Contains(log, "regions=1") {
+ t.Fatalf("want one INFO line naming the region count, got:\n%s", log)
+ }
+ if strings.Contains(log, "uMhwW4P") {
+ t.Fatal("the relay key reached the log")
+ }
+}
+
+// TestSetRelayIsTypeConsistentWithBuildsInitialStore: build() stores a
+// nil []relay.Region into a.relay when no relay is configured, and
+// SetRelay later stores a real []relay.Region into the same
+// atomic.Value. atomic.Value panics on a Store whose concrete type
+// disagrees with an earlier one, so swapping to a real slice and back
+// to empty here without a panic is the proof the two call sites agree.
+func TestSetRelayIsTypeConsistentWithBuildsInitialStore(t *testing.T) {
+ cfg := minimalConfig(t)
+ cfg.Env = EnvDevelopment
+ d := openRelayTestDB(t, slog.New(slog.NewTextHandler(io.Discard, nil)))
+ _, ctl, err := NewForTest(d, cfg)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := ctl.SetRelay(relayVector(t)); err != nil {
+ t.Fatal(err)
+ }
+ if !ctl.a.relayConfigured() {
+ t.Fatal("SetRelay did not take effect")
+ }
+ if err := ctl.SetRelay(""); err != nil {
+ t.Fatal(err)
+ }
+ if ctl.a.relayConfigured() {
+ t.Fatal(`SetRelay("") did not clear the relay`)
+ }
+}
+
+// TestSetRelayOfAMalformedValueLeavesRegionsUnchanged: SetRelay parses
+// before it stores, so a malformed value must return the error and
+// leave whatever was there before untouched — never a half-applied
+// swap.
+func TestSetRelayOfAMalformedValueLeavesRegionsUnchanged(t *testing.T) {
+ cfg := minimalConfig(t)
+ cfg.Env = EnvDevelopment
+ d := openRelayTestDB(t, slog.New(slog.NewTextHandler(io.Discard, nil)))
+ _, ctl, err := NewForTest(d, cfg)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := ctl.SetRelay(relayVector(t)); err != nil {
+ t.Fatal(err)
+ }
+ before := ctl.a.relayRegions()
+ if err := ctl.SetRelay("%%%not-base64"); err == nil {
+ t.Fatal("want an error")
+ }
+ after := ctl.a.relayRegions()
+ if len(after) != len(before) || len(after) == 0 || after[0].Region != before[0].Region {
+ t.Fatalf("a malformed SetRelay mutated the regions: before len=%d after len=%d", len(before), len(after))
+ }
+}
diff --git a/internal/meet/config.go b/internal/meet/config.go
index f092cb6..a09b88b 100644
--- a/internal/meet/config.go
+++ b/internal/meet/config.go
@@ -44,10 +44,6 @@ type Config struct {
// §2). Absent means the floor (§5.5). It is parsed once here and the
// decoded value is held in memory and never logged (§5.2).
Relay string
- // RelayConfigured is derived from Relay by resolve; callers read the
- // resolved value, not this field. Kept so existing tests that set it
- // still compile; resolve overwrites it.
- RelayConfigured bool
// Zone is the IANA zone start times are entered and shown in.
// MEET_ZONE, default UTC. Per-person zones are deliberately not in
// version one.
diff --git a/internal/meet/config_test.go b/internal/meet/config_test.go
index 1ca701d..613aa1d 100644
--- a/internal/meet/config_test.go
+++ b/internal/meet/config_test.go
@@ -51,11 +51,49 @@ func TestAMalformedRelayRefusesToStartOutsideDevelopment(t *testing.T) {
if err == nil || !strings.Contains(err.Error(), "CARLOS_RELAY") {
t.Fatalf("want a refusal naming the variable, got %v", err)
}
+ // Not just any refusal that happens to name the variable: this must
+ // be config.go's own wording, so a case where relay.Parse's error
+ // text alone satisfied the assertion above (and resolve silently
+ // stopped wrapping it) would still be caught.
+ if !strings.Contains(err.Error(), "is present but malformed") {
+ t.Fatalf("want config.go's own refusal wording, got %v", err)
+ }
if strings.Contains(err.Error(), "%%%") {
t.Fatal("the refusal echoes the value")
}
}
+// TestAMalformedRelayBuiltFromTheVectorNeverLeaksIt is the config-layer
+// regression test for relay's Task 1 item 1: element 0's "no urls"
+// error used to embed the region field verbatim, so a raw value whose
+// region field held the platform vector key leaked it straight into
+// resolve's refusal. The key and the username_suffix's incarnation
+// segment are planted throughout the one region below so any of the
+// three fields' checks reintroducing an echo trips this test.
+func TestAMalformedRelayBuiltFromTheVectorNeverLeaksIt(t *testing.T) {
+ const vectorKey = "uMhwW4P/9O5dQUIs91t2CCHGhwJbeiM0xQMQ2G0CJLU="
+ cfg := minimalConfig(t)
+ cfg.Env = "production"
+ regions := []map[string]any{{
+ "region": vectorKey,
+ "urls": []string{},
+ "key": vectorKey,
+ "username_suffix": "abc:meet:0123456789abcdef:k1",
+ }}
+ b, err := json.Marshal(regions)
+ if err != nil {
+ t.Fatal(err)
+ }
+ cfg.Relay = base64.StdEncoding.EncodeToString(b)
+ _, err = resolve(cfg)
+ if err == nil {
+ t.Fatal("want a refusal")
+ }
+ if strings.Contains(err.Error(), vectorKey) || strings.Contains(err.Error(), "0123456789abcdef") {
+ t.Fatalf("the refusal echoes the vector: %v", err)
+ }
+}
+
func TestAMalformedRelayIsIgnoredWithAWarningInDevelopment(t *testing.T) {
var buf bytes.Buffer
cfg := minimalConfig(t)
@@ -84,7 +122,7 @@ func TestAGoodRelayIsParsedAndNeverLogged(t *testing.T) {
t.Fatal(err)
}
if len(out.relay) != 1 || !out.relayConfigured {
- t.Fatalf("parsed %+v", out.relay)
+ t.Fatalf("parsed len=%d relayConfigured=%v", len(out.relay), out.relayConfigured)
}
if strings.Contains(buf.String(), "uMhwW4P") || strings.Contains(buf.String(), "0123456789abcdef") {
t.Fatal("the key or the suffix reached a log line")
diff --git a/internal/meettest/relay_floor_test.go b/internal/meettest/relay_floor_test.go
new file mode 100644
index 0000000..b299573
--- /dev/null
+++ b/internal/meettest/relay_floor_test.go
@@ -0,0 +1,62 @@
+package meettest
+
+import (
+ "encoding/base64"
+ "encoding/json"
+ "strings"
+ "testing"
+)
+
+// relayVectorForTest is the base64 CARLOS_RELAY value for one region,
+// built from relay's own platform vector key and username suffix
+// (internal/meet/relay/relay_test.go) so a leak of either into a
+// rendered page would be unmistakable.
+func relayVectorForTest(t *testing.T) string {
+ t.Helper()
+ const vectorKey = "uMhwW4P/9O5dQUIs91t2CCHGhwJbeiM0xQMQ2G0CJLU="
+ regions := []map[string]any{{
+ "region": "eu-west-1",
+ "urls": []string{"stun:relay.eu-west-1.oncarlos.com:3478"},
+ "key": vectorKey,
+ "username_suffix": "abc:meet:0123456789abcdef:k1",
+ }}
+ b, err := json.Marshal(regions)
+ if err != nil {
+ t.Fatal(err)
+ }
+ return base64.StdEncoding.EncodeToString(b)
+}
+
+// TestRoomPageShowsFloorWithNoRelay: holding_test.go's
+// TestHoldingPageShowsTitleStandingAndFloor covers /join; room.html
+// renders the identical sentence and nothing proved it — there was no
+// room-page floor test before this one.
+func TestRoomPageShowsFloorWithNoRelay(t *testing.T) {
+ x, sqid, _ := memberWithMeeting(t)
+ c := x.browser(t)
+ c.signIn("paul@example.com")
+ body := c.get("/m/" + sqid).Body.String()
+ if !strings.Contains(body, "no relay yet") {
+ t.Fatalf("room page lacks the floor sentence with no relay configured:\n%s", body)
+ }
+}
+
+// TestSetRelayHidesTheFloorSentenceOnBothPages: the floor sentence,
+// present on both the holding and room pages with no relay, must
+// disappear from both once TestHandle.SetRelay configures one — the
+// "relay configured ⇒ sentence absent" direction that hardcoding Floor
+// to true would still have passed.
+func TestSetRelayHidesTheFloorSentenceOnBothPages(t *testing.T) {
+ x, sqid, _ := memberWithMeeting(t)
+ if err := x.ctl.SetRelay(relayVectorForTest(t)); err != nil {
+ t.Fatal(err)
+ }
+ c := x.browser(t)
+ c.signIn("paul@example.com")
+ if body := c.get("/m/" + sqid + "/join").Body.String(); strings.Contains(body, "no relay yet") {
+ t.Fatalf("holding page still shows the floor sentence with a relay configured:\n%s", body)
+ }
+ if body := c.get("/m/" + sqid).Body.String(); strings.Contains(body, "no relay yet") {
+ t.Fatalf("room page still shows the floor sentence with a relay configured:\n%s", body)
+ }
+}