fix(ui): address PR review feedback for SSO re-auth flow

Rename getCurrentUser to getAuthResult, store AuthResult as single state,
use location.replace for re-auth redirect, restore login page relative
positioning, and clear error on successful authentication.

Signed-off-by: Dmytro Rashko <dmitriy.rashko@amdocs.com>

Author: dimetron

design/EP-2045-sso-session-expiry-redirect.md

@@ -8,7 +8,7 @@ kagent is frequently deployed behind [oauth2-proxy](https://github.com/oauth2-pr
 acting as an OIDC relying party. oauth2-proxy authenticates the user, maintains
 its own session cookie, and forwards the user's `id_token` to the kagent UI as a
 `Authorization: Bearer <jwt>` header. The UI decodes the JWT to derive the current
-user (`getCurrentUser`).
+user (`getAuthResult`).
 
 Two distinct failure modes were conflated in the previous implementation, which
 decoded the token and returned `CurrentUser | null`:
@@ -50,7 +50,7 @@ must manually reload to recover. This is confusing and looks like a kagent bug.
 
 ### Auth status model (`ui/src/app/actions/auth.ts`)
 
-`getCurrentUser()` now returns an `AuthResult` instead of `CurrentUser | null`:
+`getAuthResult()` now returns an `AuthResult` instead of `CurrentUser | null`:
 
 ```ts
 export type AuthStatus = "authenticated" | "expired" | "unsecured";
@@ -102,7 +102,7 @@ successful `authenticated` result.
 
 ## Test Plan
 
-- **Unit (UI):** `getCurrentUser` returns the correct `AuthStatus` for: no header,
+- **Unit (UI):** `getAuthResult` returns the correct `AuthStatus` for: no header,
   expired token, valid token. `AuthProvider` redirects only on `expired`, never on
   `unsecured`, and respects the loop guard.
 - **Manual / e2e:** Deploy behind oauth2-proxy with a short `id_token` lifetime;

docs/OIDC_PROXY_AUTH_ARCHITECTURE.md

@@ -51,7 +51,7 @@ flowchart TB
     subgraph UI["UI Layer (Next.js)"]
         LoginPage["/login Page<br/>SSO redirect button"]
         AuthContext["AuthContext Provider<br/>- User state management<br/>- Loading/error states"]
-        AuthActions["Server Actions<br/>getCurrentUser()"]
+        AuthActions["Server Actions<br/>getAuthResult()"]
         JWTLib["JWT Library<br/>- Decode tokens<br/>- Check expiry"]
         AuthLib["Auth Library<br/>- Header forwarding"]
     end
@@ -137,7 +137,7 @@ flowchart TD
     style I fill:#ff9,stroke:#333
 ```
 
-**Design rationale**: oauth2-proxy gates access using its session cookie (valid up to `cookie-expire`, default 168h), while the UI derives the user from the forwarded id_token. These lifetimes are decoupled, so the id_token can go stale while the session cookie is still valid. To keep them aligned, oauth2-proxy *can* be configured with `cookie-refresh` (and the `offline_access` scope) to refresh the id_token — note the chart's defaults do **not** enable these (default scope is `openid profile email groups`), so operators must opt in. As a safety net regardless of refresh configuration, `getCurrentUser()` distinguishes three states:
+**Design rationale**: oauth2-proxy gates access using its session cookie (valid up to `cookie-expire`, default 168h), while the UI derives the user from the forwarded id_token. These lifetimes are decoupled, so the id_token can go stale while the session cookie is still valid. To keep them aligned, oauth2-proxy *can* be configured with `cookie-refresh` (and the `offline_access` scope) to refresh the id_token — note the chart's defaults do **not** enable these (default scope is `openid profile email groups`), so operators must opt in. As a safety net regardless of refresh configuration, `getAuthResult()` distinguishes three states:
 
 - **authenticated** — valid token → set user state.
 - **expired** — `Authorization` header present but token missing/expired → the UI re-runs the OIDC flow (`/oauth2/start`) to mint a fresh token, guarded against redirect loops.

ui/src/app/actions/__tests__/auth.test.ts

@@ -1,4 +1,4 @@
-import { getCurrentUser } from "@/app/actions/auth";
+import { getAuthResult } from "@/app/actions/auth";
 import { headers } from "next/headers";
 import { decodeJWT, isTokenExpired } from "@/lib/jwt";
 
@@ -21,15 +21,15 @@ function withAuthorizationHeader(value: string | null) {
   });
 }
 
-describe("getCurrentUser", () => {
+describe("getAuthResult", () => {
   beforeEach(() => {
     jest.clearAllMocks();
   });
 
   it("returns unsecured when there is no Authorization header", async () => {
     withAuthorizationHeader(null);
 
-    const result = await getCurrentUser();
+    const result = await getAuthResult();
 
     expect(result).toEqual({ status: "unsecured", user: null });
     expect(mockedDecodeJWT).not.toHaveBeenCalled();
@@ -38,7 +38,7 @@ describe("getCurrentUser", () => {
   it("returns unsecured when the header is not a Bearer token", async () => {
     withAuthorizationHeader("Basic abc123");
 
-    const result = await getCurrentUser();
+    const result = await getAuthResult();
 
     expect(result).toEqual({ status: "unsecured", user: null });
   });
@@ -47,7 +47,7 @@ describe("getCurrentUser", () => {
     withAuthorizationHeader("Bearer not-a-jwt");
     mockedDecodeJWT.mockReturnValue(null);
 
-    const result = await getCurrentUser();
+    const result = await getAuthResult();
 
     expect(mockedDecodeJWT).toHaveBeenCalledWith("not-a-jwt");
     expect(result).toEqual({ status: "expired", user: null });
@@ -58,7 +58,7 @@ describe("getCurrentUser", () => {
     mockedDecodeJWT.mockReturnValue({ sub: "user-1", exp: 1 });
     mockedIsTokenExpired.mockReturnValue(true);
 
-    const result = await getCurrentUser();
+    const result = await getAuthResult();
 
     expect(result).toEqual({ status: "expired", user: null });
   });
@@ -69,7 +69,7 @@ describe("getCurrentUser", () => {
     mockedDecodeJWT.mockReturnValue(claims);
     mockedIsTokenExpired.mockReturnValue(false);
 
-    const result = await getCurrentUser();
+    const result = await getAuthResult();
 
     expect(result).toEqual({ status: "authenticated", user: claims });
   });

ui/src/app/actions/auth.ts

@@ -23,7 +23,7 @@ export interface AuthResult {
   user: CurrentUser | null;
 }
 
-export async function getCurrentUser(): Promise<AuthResult> {
+export async function getAuthResult(): Promise<AuthResult> {
   const headersList = await headers();
   const authHeader = headersList.get("Authorization");
 

ui/src/app/login/page.tsx

@@ -12,7 +12,7 @@ export default function LoginPage() {
       {/* Preload background image for faster rendering */}
       <link rel="preload" href="/login-bg.webp" as="image" type="image/webp" fetchPriority="high" />
 
-      <div className="login-page fixed inset-0 z-50 overflow-hidden bg-[#0B0B15] text-white">
+      <div className="login-page relative fixed inset-0 z-50 overflow-hidden bg-[#0B0B15] text-white">
         <a
           href="#login-main"
           className={cn(skipToContentLinkClassName, "text-white/90")}

ui/src/contexts/AuthContext.tsx

@@ -1,7 +1,7 @@
 "use client";
 
 import React, { createContext, useContext, useEffect, useState, type ReactNode } from "react";
-import { getCurrentUser, type CurrentUser, type AuthStatus } from "@/app/actions/auth";
+import { getAuthResult, type AuthResult, type CurrentUser, type AuthStatus } from "@/app/actions/auth";
 
 // oauth2-proxy endpoint that (re)starts the OIDC flow. Client components can only
 // read NEXT_PUBLIC_* env vars at runtime, so this mirrors the server-side
@@ -25,18 +25,15 @@ interface AuthContextValue {
 const AuthContext = createContext<AuthContextValue | undefined>(undefined);
 
 export function AuthProvider({ children }: { children: ReactNode }) {
-  const [user, setUser] = useState<CurrentUser | null>(null);
-  const [status, setStatus] = useState<AuthStatus>("unsecured");
+  const [authResult, setAuthResult] = useState<AuthResult>({ status: "unsecured", user: null });
   const [isLoading, setIsLoading] = useState(true);
   const [error, setError] = useState<Error | null>(null);
 
   const fetchUser = async () => {
     setIsLoading(true);
     setError(null);
     try {
-      const result = await getCurrentUser();
-      setStatus(result.status);
-      setUser(result.user);
+      setAuthResult(await getAuthResult());
     } catch (e) {
       setError(e instanceof Error ? e : new Error("Failed to fetch user"));
     } finally {
@@ -53,7 +50,7 @@ export function AuthProvider({ children }: { children: ReactNode }) {
   // of silently rendering a logged-out UI. Only triggers in secured ("expired")
   // mode — never in "unsecured" mode where there is no /oauth2 endpoint.
   useEffect(() => {
-    if (isLoading || status !== "expired" || typeof window === "undefined") return;
+    if (isLoading || authResult.status !== "expired" || typeof window === "undefined") return;
 
     const lastAttempt = Number(sessionStorage.getItem(REAUTH_GUARD_KEY) || "0");
     if (Date.now() - lastAttempt < REAUTH_GUARD_WINDOW_MS) {
@@ -64,17 +61,26 @@ export function AuthProvider({ children }: { children: ReactNode }) {
     }
     sessionStorage.setItem(REAUTH_GUARD_KEY, String(Date.now()));
     const rd = encodeURIComponent(window.location.pathname + window.location.search);
-    window.location.assign(`${SSO_REDIRECT_PATH}?rd=${rd}`);
-  }, [isLoading, status]);
+    window.location.replace(`${SSO_REDIRECT_PATH}?rd=${rd}`);
+  }, [isLoading, authResult.status]);
 
   useEffect(() => {
-    if (status === "authenticated" && typeof window !== "undefined") {
+    if (authResult.status === "authenticated" && typeof window !== "undefined") {
       sessionStorage.removeItem(REAUTH_GUARD_KEY);
+      setError(null);
     }
-  }, [status]);
+  }, [authResult.status]);
 
   return (
-    <AuthContext.Provider value={{ user, status, isLoading, error, refetch: fetchUser }}>
+    <AuthContext.Provider
+      value={{
+        user: authResult.user,
+        status: authResult.status,
+        isLoading,
+        error,
+        refetch: fetchUser,
+      }}
+    >
       {children}
     </AuthContext.Provider>
   );

ui/src/contexts/__tests__/AuthContext.test.tsx

@@ -1,13 +1,13 @@
 import React from "react";
 import { render, screen, waitFor } from "@testing-library/react";
 import { AuthProvider, useAuth } from "@/contexts/AuthContext";
-import { getCurrentUser, type AuthResult } from "@/app/actions/auth";
+import { getAuthResult, type AuthResult } from "@/app/actions/auth";
 
 jest.mock("@/app/actions/auth", () => ({
-  getCurrentUser: jest.fn(),
+  getAuthResult: jest.fn(),
 }));
 
-const mockedGetCurrentUser = getCurrentUser as jest.Mock;
+const mockedGetAuthResult = getAuthResult as jest.Mock;
 
 const REAUTH_GUARD_KEY = "kagent_reauth_attempt";
 
@@ -31,15 +31,15 @@ function renderProvider() {
 }
 
 function setResult(result: AuthResult) {
-  mockedGetCurrentUser.mockResolvedValue(result);
+  mockedGetAuthResult.mockResolvedValue(result);
 }
 
-// jsdom fully hardens window.location (it is non-configurable and assign is
-// read-only), so the redirect call (window.location.assign) cannot be spied on
+// jsdom fully hardens window.location (it is non-configurable and replace is
+// read-only), so the redirect call (window.location.replace) cannot be spied on
 // here. Instead we assert the observable contract that gates the redirect: the
-// sessionStorage re-auth guard, written immediately before assign() is invoked
+// sessionStorage re-auth guard, written immediately before replace() is invoked
 // ("guard written" == "redirect attempted"). console.error is silenced to drop
-// jsdom's expected "Not implemented: navigation" noise from the assign() call.
+// jsdom's expected "Not implemented: navigation" noise from the replace() call.
 describe("AuthProvider re-auth behavior", () => {
   let consoleErrorSpy: jest.SpyInstance;
 
@@ -92,7 +92,7 @@ describe("AuthProvider re-auth behavior", () => {
     expect(sessionStorage.getItem(REAUTH_GUARD_KEY)).toBe(previousAttempt);
   });
 
-  it("clears the re-auth guard once authenticated", async () => {
+  it("clears the re-auth guard and error once authenticated", async () => {
     sessionStorage.setItem(REAUTH_GUARD_KEY, String(Date.now()));
     setResult({ status: "authenticated", user: { sub: "user-1" } });
 
@@ -102,5 +102,6 @@ describe("AuthProvider re-auth behavior", () => {
       expect(screen.getByTestId("status").textContent).toBe("authenticated");
     });
     expect(sessionStorage.getItem(REAUTH_GUARD_KEY)).toBeNull();
+    expect(screen.getByTestId("error").textContent).toBe("");
   });
 });