From 110a115e89b6a46fbbe0599137277343e3e9362c Mon Sep 17 00:00:00 2001 From: wsj <279978307+wsj040303@users.noreply.github.com> Date: Wed, 11 Nov 2026 00:56:40 +0800 Subject: [PATCH] fix(iam): distinguish login service failures from invalid credentials --- docs/console-design.md | 8 +++ internal/iam/authenticator.go | 8 ++- internal/iam/authenticator_test.go | 45 +++++++++++++++ internal/server/oauth.go | 4 +- internal/server/oauth_test.go | 75 +++++++++++++++++++++++++ web/src/shell/api.test.ts | 76 ++++++++++++++++++++++++++ web/src/shell/api.ts | 22 ++++++-- web/src/shell/locales/shell.en-US.json | 3 +- web/src/shell/locales/shell.zh-CN.json | 3 +- 9 files changed, 233 insertions(+), 11 deletions(-) create mode 100644 internal/iam/authenticator_test.go create mode 100644 internal/server/oauth_test.go create mode 100644 web/src/shell/api.test.ts diff --git a/docs/console-design.md b/docs/console-design.md index 47c3230..e805fed 100644 --- a/docs/console-design.md +++ b/docs/console-design.md @@ -6,6 +6,14 @@ The ModelSphere community portal. A full-stack BFF that **owns identity** and **federates everything else**. It exists because the open-source inference stack (swiss and friends) has no user management, login, or unified entry point. +## Login failures + +| Failure | Token endpoint | Login page | +|---|---|---| +| Missing or invalid credentials, unknown or inactive user | `401 invalid_grant` | Generic incorrect-credentials message | +| Kubernetes user lookup or token issuance fails | `500 server_error`; cause in server logs, service-failure audit reason | Service-unavailable guidance; check connectivity/VPN or contact an administrator | +| Browser cannot reach the endpoint, or receives an unusable response | No session is stored | The same service-unavailable guidance | + ## Why a separate service (not part of swiss) swiss is a deploy control plane: near-stateless, "the cluster is the only source diff --git a/internal/iam/authenticator.go b/internal/iam/authenticator.go index 73f7ca4..f6c2ab0 100644 --- a/internal/iam/authenticator.go +++ b/internal/iam/authenticator.go @@ -3,7 +3,10 @@ package iam import ( "context" "errors" + "fmt" "log/slog" + + apierrors "k8s.io/apimachinery/pkg/api/errors" ) // ErrAuth is returned for every credential failure, without distinguishing @@ -45,9 +48,12 @@ func (a *Authenticator) Login(ctx context.Context, username, password string) (* return nil, newAuthFailure("username or password missing") } u, err := a.store.GetUser(ctx, username) - if err != nil { + if apierrors.IsNotFound(err) { return nil, newAuthFailure("user not found") } + if err != nil { + return nil, fmt.Errorf("get user: %w", err) + } if u.Status.State != "" && u.Status.State != UserActive { return nil, newAuthFailure("user is not active") } diff --git a/internal/iam/authenticator_test.go b/internal/iam/authenticator_test.go new file mode 100644 index 0000000..b994322 --- /dev/null +++ b/internal/iam/authenticator_test.go @@ -0,0 +1,45 @@ +package iam + +import ( + "context" + "errors" + "log/slog" + "testing" + "time" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + dynamicfake "k8s.io/client-go/dynamic/fake" + k8stesting "k8s.io/client-go/testing" +) + +func TestLoginPreservesUserReadErrors(t *testing.T) { + for _, cause := range []error{ + context.DeadlineExceeded, + context.Canceled, + errors.New("connection refused"), + apierrors.NewForbidden(UsersGVR.GroupResource(), "admin", errors.New("access denied")), + apierrors.NewUnauthorized("cluster credentials expired"), + } { + t.Run(cause.Error(), func(t *testing.T) { + dyn := dynamicfake.NewSimpleDynamicClient(runtime.NewScheme()) + dyn.PrependReactor("get", "users", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, cause + }) + authn := NewAuthenticator(NewStore(dyn), NewSigner("https://issuer.example.com", "test-only", time.Hour), slog.New(slog.DiscardHandler)) + token, err := authn.Login(t.Context(), "admin", "test-password") + if token != nil || !errors.Is(err, cause) || errors.Is(err, ErrAuth) { + t.Fatalf("Login() = %v, %v; want the user-read error, not ErrAuth", token, err) + } + }) + } +} + +func TestLoginMissingUserRemainsACredentialFailure(t *testing.T) { + dyn := dynamicfake.NewSimpleDynamicClient(runtime.NewScheme()) + authn := NewAuthenticator(NewStore(dyn), NewSigner("https://issuer.example.com", "test-only", time.Hour), slog.New(slog.DiscardHandler)) + token, err := authn.Login(t.Context(), "missing", "test-password") + if token != nil || !errors.Is(err, ErrAuth) || err.Error() != ErrAuth.Error() { + t.Fatalf("Login() = %v, %v; want the generic credential error", token, err) + } +} diff --git a/internal/server/oauth.go b/internal/server/oauth.go index 58d4cd1..e00d6be 100644 --- a/internal/server/oauth.go +++ b/internal/server/oauth.go @@ -48,8 +48,8 @@ func (s *Server) handleToken(w http.ResponseWriter, r *http.Request) { writeOAuthError(w, http.StatusUnauthorized, "invalid_grant") return } - reason = "token issuance failed" - s.log.Error("token issue", "err", err) + reason = "login service failed" + s.log.Error("login", "err", err) writeOAuthError(w, http.StatusInternalServerError, "server_error") return } diff --git a/internal/server/oauth_test.go b/internal/server/oauth_test.go new file mode 100644 index 0000000..3ef0df3 --- /dev/null +++ b/internal/server/oauth_test.go @@ -0,0 +1,75 @@ +package server + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "log/slog" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/modelsphere/console/internal/iam" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + dynamicfake "k8s.io/client-go/dynamic/fake" + k8stesting "k8s.io/client-go/testing" +) + +func TestTokenUserReadFailureIsAServerError(t *testing.T) { + for _, cause := range []error{ + context.DeadlineExceeded, + errors.New("connection refused"), + apierrors.NewForbidden(iam.UsersGVR.GroupResource(), "admin", errors.New("access denied")), + apierrors.NewUnauthorized("cluster credentials expired"), + } { + t.Run(cause.Error(), func(t *testing.T) { + srv := testServer(t) + dyn := srv.kube.Dynamic().(*dynamicfake.FakeDynamicClient) + dyn.PrependReactor("get", "users", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, cause + }) + var logs bytes.Buffer + srv.log = slog.New(slog.NewJSONHandler(&logs, nil)) + form := url.Values{"grant_type": {"password"}, "username": {"admin"}, "password": {"test-password"}} + req := httptest.NewRequest("POST", "/oauth/token", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + rec := httptest.NewRecorder() + srv.Handler().ServeHTTP(rec, req) + var body map[string]string + decodeErr := json.Unmarshal(rec.Body.Bytes(), &body) + if rec.Code != http.StatusInternalServerError || decodeErr != nil || len(body) != 1 || body["error"] != "server_error" { + t.Fatalf("token response = %d %s; want generic server_error", rec.Code, rec.Body.String()) + } + var entry map[string]any + logErr := json.NewDecoder(&logs).Decode(&entry) + loggedCause, _ := entry["err"].(string) + if logErr != nil || !strings.Contains(loggedCause, cause.Error()) || strings.Contains(loggedCause, "test-password") { + t.Fatalf("logs should preserve the cause without the password: %s", logs.String()) + } + records, err := srv.store.ListLoginRecords(t.Context(), "admin") + if err != nil || len(records) != 1 || records[0].Spec.Success || records[0].Spec.Reason != "login service failed" { + t.Fatalf("login audit = %v, %v; want a service failure", records, err) + } + }) + } +} + +func TestTokenCredentialFailuresStayIndistinguishable(t *testing.T) { + srv := testServer(t) + for _, credentials := range [][2]string{{"admin", "wrong"}, {"missing", "wrong"}, {"", "wrong"}, {"admin", ""}} { + form := url.Values{"grant_type": {"password"}, "username": {credentials[0]}, "password": {credentials[1]}} + req := httptest.NewRequest("POST", "/oauth/token", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + rec := httptest.NewRecorder() + srv.Handler().ServeHTTP(rec, req) + var body map[string]string + decodeErr := json.Unmarshal(rec.Body.Bytes(), &body) + if rec.Code != http.StatusUnauthorized || decodeErr != nil || len(body) != 1 || body["error"] != "invalid_grant" { + t.Fatalf("token response = %d %s; want generic invalid_grant", rec.Code, rec.Body.String()) + } + } +} diff --git a/web/src/shell/api.test.ts b/web/src/shell/api.test.ts new file mode 100644 index 0000000..4ea44a7 --- /dev/null +++ b/web/src/shell/api.test.ts @@ -0,0 +1,76 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { login } from "@/shell/api"; +import { DEFAULT_LOCALE, getT, setLocale, type Locale } from "@/shell/i18n"; + +afterEach(() => { + vi.unstubAllGlobals(); + setLocale(DEFAULT_LOCALE); +}); + +describe.each(["en-US", "zh-CN"])("login in %s", (locale) => { + beforeEach(() => { + setLocale(locale); + vi.stubGlobal("document", { cookie: "token=existing" }); + }); + + it("keeps invalid credentials generic", async () => { + vi.stubGlobal("fetch", vi.fn().mockResolvedValue(Response.json({ error: "invalid_grant" }, { status: 401 }))); + await expect(login("missing", "wrong")).rejects.toMatchObject({ + status: 401, + message: getT("shell")("login.badCredentials"), + }); + expect(document.cookie).toBe("token=existing"); + }); + + it.each([ + [500, '{"error":"server_error"}'], + [503, '{"error":"invalid_grant"}'], + [502, "upstream unavailable"], + [401, '{"error":"upstream_auth_failed"}'], + [401, "unauthorized"], + [400, '{"error":"invalid_request"}'], + [429, '{"error":"rate_limited"}'], + ])("reports HTTP %s service failures without exposing the response body", async (status, body) => { + vi.stubGlobal("fetch", vi.fn().mockResolvedValue(new Response(body, { status }))); + const message = getT("shell")("login.serviceUnavailable"); + expect(message).not.toBe("login.serviceUnavailable"); + await expect(login("admin", "test-password")).rejects.toMatchObject({ + status, + message, + }); + expect(document.cookie).toBe("token=existing"); + }); + + it("gives recovery guidance when the browser cannot reach the service", async () => { + vi.stubGlobal("fetch", vi.fn().mockRejectedValue(new TypeError("Failed to fetch"))); + await expect(login("admin", "test-password")).rejects.toMatchObject({ + status: 0, + message: getT("shell")("login.serviceUnavailable"), + }); + expect(document.cookie).toBe("token=existing"); + }); + + it.each(["proxy login", "{}", '{"access_token":""}', '{"access_token":123}', "null"])( + "does not store a malformed token response: %s", + async (body) => { + vi.stubGlobal("fetch", vi.fn().mockResolvedValue(new Response(body, { status: 200 }))); + await expect(login("admin", "test-password")).rejects.toMatchObject({ + status: 200, + message: getT("shell")("login.serviceUnavailable"), + }); + expect(document.cookie).toBe("token=existing"); + }, + ); + + it("keeps the password grant and stores a successful token", async () => { + const fetch = vi.fn().mockResolvedValue(Response.json({ access_token: "test/token" })); + vi.stubGlobal("fetch", fetch); + await login("name+space", "pass&word"); + expect(fetch).toHaveBeenCalledWith("/oauth/token", { + method: "POST", + headers: { "Content-Type": "application/x-www-form-urlencoded" }, + body: "grant_type=password&username=name%2Bspace&password=pass%26word", + }); + expect(document.cookie).toBe("token=test%2Ftoken; path=/; SameSite=Lax"); + }); +}); diff --git a/web/src/shell/api.ts b/web/src/shell/api.ts index 1732353..7be59bc 100644 --- a/web/src/shell/api.ts +++ b/web/src/shell/api.ts @@ -80,21 +80,31 @@ export interface ChangePasswordInput { // login exchanges credentials for a token at the OAuth2 password-grant endpoint // (form-encoded, exactly as Rise Global expects) and stores it. export async function login(username: string, password: string): Promise { - const form = new URLSearchParams({ grant_type: "password", username, password }); + const form = new URLSearchParams({ + grant_type: "password", + username, + password, + }); + const unavailable = getT("shell")("login.serviceUnavailable"); const res = await fetch("/oauth/token", { method: "POST", headers: { "Content-Type": "application/x-www-form-urlencoded" }, body: form.toString(), + }).catch(() => { + throw new ApiError(0, unavailable); }); + const body = await res.json().catch(() => null); if (!res.ok) { - throw new ApiError(res.status, getT("shell")("login.badCredentials")); + const message = res.status === 401 && body?.error === "invalid_grant" ? getT("shell")("login.badCredentials") : unavailable; + throw new ApiError(res.status, message); + } + if (typeof body?.access_token !== "string" || !body.access_token) { + throw new ApiError(res.status, unavailable); } - const tok = (await res.json()) as { access_token: string }; - setToken(tok.access_token); + setToken(body.access_token); } export const api = { me: () => request("GET", "/api/me"), - changePassword: (passwords: ChangePasswordInput) => - request<{ status: string }>("POST", "/api/me/password", passwords), + changePassword: (passwords: ChangePasswordInput) => request<{ status: string }>("POST", "/api/me/password", passwords), }; diff --git a/web/src/shell/locales/shell.en-US.json b/web/src/shell/locales/shell.en-US.json index 44641a4..d858c4e 100644 --- a/web/src/shell/locales/shell.en-US.json +++ b/web/src/shell/locales/shell.en-US.json @@ -30,7 +30,8 @@ "submit": "Sign in", "submitting": "Signing in…", "failed": "Sign-in failed", - "badCredentials": "Incorrect username or password" + "badCredentials": "Incorrect username or password", + "serviceUnavailable": "The sign-in service is unavailable. Check your network or VPN connection and try again, or contact your administrator." }, "password": { "current": "Current password", diff --git a/web/src/shell/locales/shell.zh-CN.json b/web/src/shell/locales/shell.zh-CN.json index de86487..bf14f89 100644 --- a/web/src/shell/locales/shell.zh-CN.json +++ b/web/src/shell/locales/shell.zh-CN.json @@ -27,7 +27,8 @@ "submit": "登录", "submitting": "登录中…", "failed": "登录失败", - "badCredentials": "用户名或密码错误" + "badCredentials": "用户名或密码错误", + "serviceUnavailable": "登录服务暂时不可用。请检查网络或 VPN 连接后重试，或联系管理员。" }, "password": { "current": "当前密码",