From 009e81b4cffc4cffd65de85b122138ccc0532fdd Mon Sep 17 00:00:00 2001 From: Lemon-miaow Date: Sun, 27 Sep 2026 14:00:33 +0800 Subject: [PATCH] =?UTF-8?q?fix(api):=20op-login=20finish=20=E6=8B=92?= =?UTF-8?q?=E7=BB=9D=E5=AE=A1=E6=89=B9=E5=90=8E=E8=A2=AB=E5=81=9C=E7=94=A8?= =?UTF-8?q?=E6=88=96=E5=88=A0=E9=99=A4=E7=9A=84=E8=B4=A6=E5=8F=B7?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/openapi.yaml | 10 +++-- internal/api/api_test.go | 4 ++ internal/api/handlers_op_login.go | 14 ++++++ internal/api/handlers_op_login_test.go | 60 ++++++++++++++++++++++++++ panel/src/lib/openapi.gen.ts | 4 +- 5 files changed, 86 insertions(+), 6 deletions(-) diff --git a/docs/openapi.yaml b/docs/openapi.yaml index 6ea3201..b8c1c77 100644 --- a/docs/openapi.yaml +++ b/docs/openapi.yaml @@ -3300,8 +3300,9 @@ paths: Public, pre-session final leg: mints a host-only staff session only when BOTH factors have landed — the request is approved-and-live AND the mailed code verifies. Every failure (unknown handle, not-yet-approved, wrong or locked code, - an account past its daily wrong-code budget, lost race) collapses into one uniform 400 op_login_invalid, so a code-less - caller learns nothing. Admin is re-asserted before the session is issued. + an account past its daily wrong-code budget, an account disabled or deleted since + the start, lost race) collapses into one uniform 400 op_login_invalid, so a + code-less caller learns nothing. Admin is re-asserted before the session is issued. x-felis-face: [external] x-felis-tier: public security: [] @@ -3329,8 +3330,9 @@ paths: '400': description: >- request_id and code are required (bad_request); or the login could not be - completed — unknown handle, not approved, wrong or locked code, or lost - race, all uniform (op_login_invalid). + completed — unknown handle, not approved, wrong or locked code, an account + disabled or deleted since the start, or lost race, all uniform + (op_login_invalid). content: application/json: schema: { $ref: '#/components/schemas/Error' } diff --git a/internal/api/api_test.go b/internal/api/api_test.go index 8a0b0f7..9fa5c65 100644 --- a/internal/api/api_test.go +++ b/internal/api/api_test.go @@ -95,6 +95,7 @@ type fakeRepo struct { failMarkReauth error failGetSetting error failRedeemSetup error + failUserDetail error // player email OTPs (spec §B2). Keyed by row id; the verify path scans for the // newest live (user, purpose) just as the PG query does. otps map[string]*fakeEmailOTP @@ -1271,6 +1272,9 @@ func (f *fakeRepo) ListUsers(_ context.Context, opts ListUsersOpts) ([]UserView, } func (f *fakeRepo) UserDetail(_ context.Context, userID string) (*UserDetail, error) { + if f.failUserDetail != nil { + return nil, f.failUserDetail + } deletedAt := time.Unix(1_700_000_000, 0) for _, su := range f.seededUsers { if su.view.ID == userID { diff --git a/internal/api/handlers_op_login.go b/internal/api/handlers_op_login.go index b92ef8b..389e3dd 100644 --- a/internal/api/handlers_op_login.go +++ b/internal/api/handlers_op_login.go @@ -313,6 +313,20 @@ func (a *API) handleOpLoginFinish(w http.ResponseWriter, r *http.Request) { writeError(w, r, invalid) return } + // A disabled or soft-deleted account finishes nothing, whatever it proved: start + // found it through the live-only UserByEmail, but an admin may have retired it + // since (audit #33; a soft delete disables the account too). Checked before the + // code is touched, like the approval, and answered with the same uniform failure. + d, err := a.Repo.UserDetail(r.Context(), loginReq.UserID) + if err != nil { + writeError(w, r, err) + return + } + if d.Disabled { + a.authFailure(r, "op_login", "account_retired", a.opLoginAccount(r, loginReq.UserID)) + writeError(w, r, invalid) + return + } // Consume the mailed code (op_login purpose). A wrong/expired/locked code charges an // attempt without minting anything and returns the uniform failure — the code, not // the request, is the problem, and the request stays approved for a retry. diff --git a/internal/api/handlers_op_login_test.go b/internal/api/handlers_op_login_test.go index f33889b..3b24ee9 100644 --- a/internal/api/handlers_op_login_test.go +++ b/internal/api/handlers_op_login_test.go @@ -1,6 +1,7 @@ package api import ( + "errors" "net/http" "net/http/httptest" "strings" @@ -438,6 +439,65 @@ func TestOpLoginFinishUniform(t *testing.T) { }) } +// An account an admin retires between the approval and the finish signs in +// nowhere: finish answers the uniform failure, sets no cookie, stores no session, +// and leaves the code as it was. +func TestOpLoginFinishRefusesRetiredAccount(t *testing.T) { + for _, tc := range []struct { + name string + retire func(repo *fakeRepo) + }{ + {"disabled", func(repo *fakeRepo) { + u := UserView{ID: "a1", Username: "op", Email: "Op@Example.NET", Role: "admin", Disabled: true} + repo.seededUsers = append(repo.seededUsers, seededUser{view: u, detail: UserDetail{UserView: u}}) + }}, + {"deleted", func(repo *fakeRepo) { repo.deletedIDs["a1"] = true }}, + } { + t.Run(tc.name, func(t *testing.T) { + api, repo, mailer := seedOpLoginAPI(t) + eh, ih := api.ExternalHandler(), api.InternalHandler() + reqID := acctBody(t, startOp(eh, "op@example.net"))["request_id"].(string) + if w := approveOp(ih, reqID, opUUID, "op"); w.Code != http.StatusOK { + t.Fatalf("approve: %d (%s)", w.Code, w.Body.String()) + } + tc.retire(repo) + + w := finishOp(eh, reqID, mailer.code) + if w.Code != http.StatusBadRequest || decodeErr(t, w) != "op_login_invalid" || len(w.Result().Cookies()) != 0 { + t.Fatalf("finish: code = %d body %s cookies %v, want 400 op_login_invalid and none", + w.Code, w.Body.String(), w.Result().Cookies()) + } + if len(repo.sessions) != 0 { + t.Fatalf("sessions = %d, want none", len(repo.sessions)) + } + for _, o := range repo.otps { + if o.consumed || o.attempts != 0 { + t.Errorf("the code must stay untouched: consumed=%v attempts=%d", o.consumed, o.attempts) + } + } + }) + } + + // A store that cannot say whether the account is live is an outage: the finish + // can be retried with the same approval, so it is not told to start over. + t.Run("store outage", func(t *testing.T) { + api, repo, mailer := seedOpLoginAPI(t) + eh, ih := api.ExternalHandler(), api.InternalHandler() + reqID := acctBody(t, startOp(eh, "op@example.net"))["request_id"].(string) + if w := approveOp(ih, reqID, opUUID, "op"); w.Code != http.StatusOK { + t.Fatalf("approve: %d (%s)", w.Code, w.Body.String()) + } + repo.failUserDetail = errors.New("connection refused") + if w := finishOp(eh, reqID, mailer.code); w.Code != http.StatusInternalServerError { + t.Fatalf("finish: code = %d body %s, want 500", w.Code, w.Body.String()) + } + repo.failUserDetail = nil + if w := finishOp(eh, reqID, mailer.code); w.Code != http.StatusOK { + t.Fatalf("retry: code = %d body %s, want 200", w.Code, w.Body.String()) + } + }) +} + // TestOpLoginApproveGate pins the in-game approval gate: only a linked staff UUID // (role admin or owner) may vouch (all refusals share one 403 not_admin), a // missing/no-longer-pending request is 404, and a bare request without an diff --git a/panel/src/lib/openapi.gen.ts b/panel/src/lib/openapi.gen.ts index ba02db3..9112c8f 100644 --- a/panel/src/lib/openapi.gen.ts +++ b/panel/src/lib/openapi.gen.ts @@ -944,7 +944,7 @@ export interface paths { put?: never; /** * Redeem an approved op.console request plus its mailed code into a staff session (spec §B). - * @description Public, pre-session final leg: mints a host-only staff session only when BOTH factors have landed — the request is approved-and-live AND the mailed code verifies. Every failure (unknown handle, not-yet-approved, wrong or locked code, an account past its daily wrong-code budget, lost race) collapses into one uniform 400 op_login_invalid, so a code-less caller learns nothing. Admin is re-asserted before the session is issued. + * @description Public, pre-session final leg: mints a host-only staff session only when BOTH factors have landed — the request is approved-and-live AND the mailed code verifies. Every failure (unknown handle, not-yet-approved, wrong or locked code, an account past its daily wrong-code budget, an account disabled or deleted since the start, lost race) collapses into one uniform 400 op_login_invalid, so a code-less caller learns nothing. Admin is re-asserted before the session is issued. */ post: operations["opLoginFinish"]; delete?: never; @@ -5254,7 +5254,7 @@ export interface operations { }; }; }; - /** @description request_id and code are required (bad_request); or the login could not be completed — unknown handle, not approved, wrong or locked code, or lost race, all uniform (op_login_invalid). */ + /** @description request_id and code are required (bad_request); or the login could not be completed — unknown handle, not approved, wrong or locked code, an account disabled or deleted since the start, or lost race, all uniform (op_login_invalid). */ 400: { headers: { [name: string]: unknown;