From c4c964578e1b6b7580f709adb418e8c2f1d7527b Mon Sep 17 00:00:00 2001 From: Minseong Choi Date: Wed, 22 Jul 2026 14:40:28 +0900 Subject: [PATCH] fix(setup): stop the forced-onboarding gate trapping players who have no email MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit setup_required is what the SPA polls to decide whether the onboarding wall is still owed, and it disagreed with the middleware that actually enforces the wall. requireOnboarded lifts on a verified email OR an enrolled passkey; setup_required answered `u.Email == "" || !hasPasskey`. A console-tier player joins through the bind-code door with no email at all — by design, there is no SMTP at that point — so the email term never clears and the SPA keeps them on the setup screen forever, even after they enroll the passkey that already unlocked the API for them. The predicate now lives in one place (setupRequired) and both endpoints call it, so the next edit to the unlock condition cannot drift them apart again. Keying it on EmailVerified rather than email presence is the deliberate part: presence is exactly the term that trapped the no-email player, and it was also wrong on its own terms — an unverified address is not an authentication factor, so it was never what the lockdown could safely lift on. Also lands the regression test for the mechanism behind the live claim-403 report: /me/servers answers 200 for a bind-onboarded player (which is why the dashboard renders the 认领 button at all) while claim, wake and status all answer 403 with code "setup_required" — i.e. the refusal comes from requireOnboarded before the handler, not from isOwnerOrAdmin inside it, which would have said "forbidden". Enrolling a passkey and changing nothing else lifts all three, which isolates the gate as the sole cause. The backend authz is correct; the button that leads a locked-down player into a 403 is the frontend's to hide. --- internal/api/api.go | 12 ++ internal/api/handlers_setup.go | 14 +- internal/api/handlers_setup_lockdown_test.go | 140 +++++++++++++++++++ internal/api/handlers_setup_test.go | 42 ++++++ 4 files changed, 198 insertions(+), 10 deletions(-) create mode 100644 internal/api/handlers_setup_lockdown_test.go diff --git a/internal/api/api.go b/internal/api/api.go index db2f6bf..3170464 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -624,6 +624,18 @@ func (a *API) requireOnboarded(h http.HandlerFunc) http.HandlerFunc { } } +// setupRequired reports whether the caller still owes the forced-onboarding step, +// for the SPA to poll. It MUST stay in lockstep with requireOnboarded's unlock +// condition above: the lockdown lifts on a verified email OR an enrolled passkey. +// Keying on email PRESENCE instead of a passkey would trap a console-tier player — +// they join through the bind-code door with no email by design (no SMTP) and can +// only ever complete setup by enrolling a passkey, so any email term loops them +// forever. Passkey alone is the durable gate; email verification is a later, +// SMTP-dependent step. +func setupRequired(emailVerified, hasPasskey bool) bool { + return !emailVerified && !hasPasskey +} + // ---- request context plumbing ---- type ctxKey int diff --git a/internal/api/handlers_setup.go b/internal/api/handlers_setup.go index 6e36484..8ce8229 100644 --- a/internal/api/handlers_setup.go +++ b/internal/api/handlers_setup.go @@ -104,11 +104,8 @@ func (a *API) handleSetupRedeem(w http.ResponseWriter, r *http.Request) { "email": u.Email, "email_verified": u.EmailVerified, "has_passkey": hasPasskey, - // Setup completes on email recorded + passkey enrolled. NOT email_verified: - // the bootstrap has no SMTP, so the Owner's address is stored unverified and a - // later Settings/SMTP flow verifies it. Passkey is the Owner's only pre-SMTP - // login credential, so it — not email verification — is the durable gate. - "setup_required": u.Email == "" || !hasPasskey, + // setup_required MUST mirror requireOnboarded's unlock (api.go:615); see setupRequired. + "setup_required": setupRequired(u.EmailVerified, hasPasskey), }) } @@ -135,10 +132,7 @@ func (a *API) handleSetupStatus(w http.ResponseWriter, r *http.Request) { "email": u.Email, "email_verified": u.EmailVerified, "has_passkey": hasPasskey, - // Setup completes on email recorded + passkey enrolled. NOT email_verified: - // the bootstrap has no SMTP, so the Owner's address is stored unverified and a - // later Settings/SMTP flow verifies it. Passkey is the Owner's only pre-SMTP - // login credential, so it — not email verification — is the durable gate. - "setup_required": u.Email == "" || !hasPasskey, + // setup_required MUST mirror requireOnboarded's unlock (api.go:615); see setupRequired. + "setup_required": setupRequired(u.EmailVerified, hasPasskey), }) } diff --git a/internal/api/handlers_setup_lockdown_test.go b/internal/api/handlers_setup_lockdown_test.go new file mode 100644 index 0000000..1338556 --- /dev/null +++ b/internal/api/handlers_setup_lockdown_test.go @@ -0,0 +1,140 @@ +package api + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" +) + +// TestAdvClaim403MechanismIsRequireOnboarded independently verifies the claimed +// root cause of the live claim-403 report. It asserts three things the claim +// implies, each of which would falsify the claim if it failed: +// +// 1. /me/servers (SetupAllowed) returns 200 for a bind-onboarded player, which is +// why the dashboard renders demo2 with a 认领 button at all. +// 2. claim, wake AND status all return 403 with code "setup_required" — i.e. the +// 403 comes from requireOnboarded (api.go:612-625) BEFORE the handler, not +// from isOwnerOrAdmin inside it (that path returns code "forbidden"). +// 3. Flipping exactly ONE bit — enrolling a passkey — lifts the 403 on all three. +// This isolates requireOnboarded as the sole cause: nothing about the server +// record, ownership, or quota changed between the failing and passing runs. +func TestAdvClaim403MechanismIsRequireOnboarded(t *testing.T) { + api, repo := seedBindAPI(t) + mintBindCode(t, api, repo, "DEMO2345", bindTestUUID, authSourceMojang) + + w := do(api.ExternalHandler(), "POST", "/api/v1/auth/bind", `{"code":"DEMO2345"}`, jsonHeader) + if w.Code != http.StatusOK { + t.Fatalf("bind = %d, want 200 (%s)", w.Code, w.Body.String()) + } + cookie := map[string]string{"Cookie": sessionCookieName + "=" + sessionCookieValue(t, w)} + userID := repo.staff[bindTestUUID].ID + + // Sanity: the bind-created player really is the lockdown-eligible state — + // unverified email (no email at all) and no passkey. + if u := repo.staff[bindTestUUID]; u.EmailVerified { + t.Fatalf("bind-created player has EmailVerified=true; lockdown premise is wrong") + } + if creds, _ := repo.PasskeyCredentialsForUser(t.Context(), userID); len(creds) != 0 { + t.Fatalf("bind-created player already has %d passkeys; lockdown premise is wrong", len(creds)) + } + + // demo2 exists, is unclaimed, and the player is linked + within quota. Every + // precondition for a SUCCESSFUL claim is satisfied, so any 403 here is the + // middleware, not the handler's own authorization. + cl := api.Cluster.(*fakeCluster) + cl.byName["demo2"] = &ServerInfo{Name: "demo2", Subdomain: "demo2", Phase: "Stopped"} + repo.claimOK["demo2"] = true + repo.quota[userID] = true + + if got := do(api.ExternalHandler(), "GET", "/api/v1/me/servers", "", cookie); got.Code != http.StatusOK { + t.Fatalf("/me/servers = %d, want 200 (%s)", got.Code, got.Body.String()) + } + + routes := []struct{ method, path string }{ + {"POST", "/api/v1/servers/demo2/claim"}, + {"POST", "/api/v1/servers/demo2/wake"}, + {"GET", "/api/v1/servers/demo2/status"}, + } + for _, tc := range routes { + got := do(api.ExternalHandler(), tc.method, tc.path, "", cookie) + if got.Code != http.StatusForbidden { + t.Fatalf("%s %s = %d, want 403 (%s)", tc.method, tc.path, got.Code, got.Body.String()) + } + // The DISCRIMINATOR: setup_required proves requireOnboarded fired. If the + // 403 came from isOwnerOrAdmin the code would be "forbidden". + if c := errCode(got.Body.Bytes()); c != "setup_required" { + t.Fatalf("%s %s 403 code = %q, want setup_required (%s)", + tc.method, tc.path, c, got.Body.String()) + } + } + + // One bit: enroll a passkey. Nothing else about the request changes. + repo.passkeyCreds["pk1"] = PasskeyCredential{ID: "pk1", UserID: userID, CredentialID: "c1"} + + // Claim now reaches its handler and succeeds. + if got := do(api.ExternalHandler(), "POST", "/api/v1/servers/demo2/claim", "", cookie); got.Code != http.StatusOK { + t.Fatalf("post-passkey claim = %d %q, want 200 (%s)", got.Code, errCode(got.Body.Bytes()), got.Body.String()) + } + // fakeRepo.ClaimServer reports success without writing ownership (the fake models + // the UPDATE's row count only), so mirror the PG write the real claim performs: + // UPDATE servers SET owner_id=$user WHERE name=$name AND owner_id IS NULL. + repo.byName["demo2"] = &ServerRecord{Name: "demo2", Subdomain: "demo2", OwnerID: userID} + + // With ownership recorded, wake and status pass authorizeWake/isOwnerOrAdmin — + // proving the earlier 403s were the lockdown, not the ownership check. + for _, tc := range routes[1:] { + got := do(api.ExternalHandler(), tc.method, tc.path, "", cookie) + if got.Code == http.StatusForbidden { + t.Fatalf("%s %s still 403 for the owner after passkey enrollment (%s)", + tc.method, tc.path, got.Body.String()) + } + } +} + +// TestAdvRequireOnboardedPredicate characterizes the ACTUAL 403-producing predicate +// (api.go:615) directly, bypassing fakeRepo.SessionUser — which, unlike the real +// PGRepo.SessionUser (pgrepo.go:927, it selects email_verified), never populates +// EmailVerified and so cannot express a verified-email session. +// +// This is the falsification test for the claim's wording: the lockdown is NOT +// unconditional on non-SetupAllowed routes. Three of the five states below sail +// straight through, so "the route is not in the exemption list" does not by itself +// yield 403 — the conjunction ViaSession && !EmailVerified && no-passkey does. +func TestAdvRequireOnboardedPredicate(t *testing.T) { + repo := newFakeRepo() + api := newTestAPI(repo, newFakeCluster()) + repo.passkeyCreds["pk-enrolled"] = PasskeyCredential{ID: "pk-enrolled", UserID: "has-pk", CredentialID: "c1"} + + const passed = http.StatusTeapot // the wrapped handler ran + h := api.requireOnboarded(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(passed) }) + + for _, tc := range []struct { + name string + p *Principal + want int + }{ + // The live demo2 player: bind-onboarded, no email, no passkey. + {"session player, unverified, no passkey", &Principal{UserID: "u1", ViaSession: true, EmailVerified: false}, http.StatusForbidden}, + {"session player, email verified", &Principal{UserID: "u1", ViaSession: true, EmailVerified: true}, passed}, + {"session player, unverified but passkey enrolled", &Principal{UserID: "has-pk", ViaSession: true, EmailVerified: false}, passed}, + // Admin Zero-Trust path carries no session flag; internal face carries no principal. + {"non-session principal", &Principal{UserID: "a1", ViaSession: false, EmailVerified: false}, passed}, + {"nil principal (internal face)", nil, passed}, + } { + t.Run(tc.name, func(t *testing.T) { + r := httptest.NewRequest("POST", "/api/v1/servers/demo2/wake", nil) + if tc.p != nil { + r = r.WithContext(context.WithValue(r.Context(), ctxKeyPrincipal, tc.p)) + } + w := httptest.NewRecorder() + h(w, r) + if w.Code != tc.want { + t.Fatalf("code = %d, want %d (%s)", w.Code, tc.want, w.Body.String()) + } + if tc.want == http.StatusForbidden && errCode(w.Body.Bytes()) != "setup_required" { + t.Fatalf("403 code = %q, want setup_required", errCode(w.Body.Bytes())) + } + }) + } +} diff --git a/internal/api/handlers_setup_test.go b/internal/api/handlers_setup_test.go index 432faf5..1bb86d4 100644 --- a/internal/api/handlers_setup_test.go +++ b/internal/api/handlers_setup_test.go @@ -95,6 +95,48 @@ func TestSetEmailClearsVerified(t *testing.T) { } } +// TestSetupCompletesForNoEmailPlayer pins the console-tier player fix: a bind-code +// player joins with NO email (by design — no SMTP) and completes forced onboarding by +// enrolling a passkey alone. setup_required MUST then report false, in lockstep with +// requireOnboarded lifting (api.go:615). The OLD predicate (email == "" || !hasPasskey) +// trapped exactly this state — email is empty forever, so it looped the player. +func TestSetupCompletesForNoEmailPlayer(t *testing.T) { + repo := newFakeRepo() + // A console-tier player: role=user, no email ever, no passkey. + repo.staff["p"] = &StaffUser{ID: "p1", Username: "player", Role: "user"} + + api := newTestAPI(repo, newFakeCluster()) + api.External = staticExternal{p: &Principal{UserID: "p1", Role: "user", ViaSession: true}} + h := api.ExternalHandler() + + status := func(t *testing.T) map[string]any { + t.Helper() + w := do(h, "GET", "/api/v1/auth/setup/status", "", nil) + if w.Code != http.StatusOK { + t.Fatalf("status code = %d, want 200 (%s)", w.Code, w.Body.String()) + } + var got map[string]any + if err := json.Unmarshal(w.Body.Bytes(), &got); err != nil { + t.Fatalf("status body not JSON: %v", err) + } + return got + } + + // No email, no passkey → forced onboarding still owed. + if s := status(t); s["setup_required"] != true || s["email"] != "" { + t.Fatalf("fresh player status = %v, want setup_required=true email=\"\"", s) + } + + // Enroll a passkey — the ONLY step a no-email player can complete. + repo.passkeyCreds["pk1"] = PasskeyCredential{ID: "pk1", UserID: "p1", CredentialID: "cred1"} + + // Setup is now COMPLETE even though email stays empty. The OLD predicate returned + // true here (email == "") and looped the player forever. + if s := status(t); s["setup_required"] != false || s["has_passkey"] != true || s["email"] != "" { + t.Fatalf("post-passkey player status = %v, want setup_required=false has_passkey=true email=\"\"", s) + } +} + // errCode returns the error.code of a JSON error body, or "" if body is not one (a // non-failing decodeErr for cases where the response may be a success). func errCode(body []byte) string {