mirror of
https://github.com/All-Hands-AI/OpenHands.git
synced 2026-04-29 03:00:45 -04:00
fix(frontend): preserve login_method param to enable session re-authentication (#13310)
This commit is contained in:
@@ -2,7 +2,7 @@ import { render, screen, waitFor } from "@testing-library/react";
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import userEvent from "@testing-library/user-event";
|
||||
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||
import { createRoutesStub } from "react-router";
|
||||
import { createRoutesStub, useSearchParams } from "react-router";
|
||||
import LoginPage from "#/routes/login";
|
||||
import OptionService from "#/api/option-service/option-service.api";
|
||||
import AuthService from "#/api/auth-service/auth-service.api";
|
||||
@@ -80,6 +80,29 @@ const RouterStub = createRoutesStub([
|
||||
},
|
||||
]);
|
||||
|
||||
function DestinationStub() {
|
||||
const [params] = useSearchParams();
|
||||
const loginMethod = params.get("login_method");
|
||||
return (
|
||||
<div data-testid="destination-page">
|
||||
{loginMethod && (
|
||||
<span data-testid="login-method-param">{loginMethod}</span>
|
||||
)}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
const RouterStubWithDestination = createRoutesStub([
|
||||
{
|
||||
Component: LoginPage,
|
||||
path: "/login",
|
||||
},
|
||||
{
|
||||
Component: DestinationStub,
|
||||
path: "/settings",
|
||||
},
|
||||
]);
|
||||
|
||||
const createWrapper = () => {
|
||||
const queryClient = new QueryClient({
|
||||
defaultOptions: {
|
||||
@@ -282,7 +305,9 @@ describe("LoginPage", () => {
|
||||
await user.click(gitlabButton);
|
||||
|
||||
// URL includes state parameter added by handleAuthRedirect
|
||||
expect(window.location.href).toContain("https://gitlab.com/oauth/authorize");
|
||||
expect(window.location.href).toContain(
|
||||
"https://gitlab.com/oauth/authorize",
|
||||
);
|
||||
});
|
||||
|
||||
it("should redirect to Bitbucket auth URL when Bitbucket button is clicked", async () => {
|
||||
@@ -347,6 +372,30 @@ describe("LoginPage", () => {
|
||||
);
|
||||
});
|
||||
|
||||
it("should preserve login_method param when redirecting authenticated users", async () => {
|
||||
// Arrange
|
||||
vi.spyOn(AuthService, "authenticate").mockResolvedValue(true);
|
||||
|
||||
// Act
|
||||
render(
|
||||
<RouterStubWithDestination
|
||||
initialEntries={["/login?returnTo=/settings&login_method=github"]}
|
||||
/>,
|
||||
{ wrapper: createWrapper() },
|
||||
);
|
||||
|
||||
// Assert
|
||||
await waitFor(
|
||||
() => {
|
||||
expect(screen.getByTestId("destination-page")).toBeInTheDocument();
|
||||
expect(screen.getByTestId("login-method-param")).toHaveTextContent(
|
||||
"github",
|
||||
);
|
||||
},
|
||||
{ timeout: 2000 },
|
||||
);
|
||||
});
|
||||
|
||||
it("should redirect OSS mode users to home", async () => {
|
||||
// @ts-expect-error - partial mock for testing
|
||||
vi.spyOn(OptionService, "getConfig").mockResolvedValue({
|
||||
@@ -552,10 +601,12 @@ describe("LoginPage", () => {
|
||||
|
||||
it("should pass buildOAuthStateData to LoginContent for OAuth state encoding", async () => {
|
||||
const user = userEvent.setup();
|
||||
const mockBuildOAuthStateData = vi.fn((baseState: Record<string, string>) => ({
|
||||
...baseState,
|
||||
invitation_token: "inv-test-token-12345",
|
||||
}));
|
||||
const mockBuildOAuthStateData = vi.fn(
|
||||
(baseState: Record<string, string>) => ({
|
||||
...baseState,
|
||||
invitation_token: "inv-test-token-12345",
|
||||
}),
|
||||
);
|
||||
|
||||
useInvitationMock.mockReturnValue({
|
||||
invitationToken: "inv-test-token-12345",
|
||||
@@ -585,10 +636,12 @@ describe("LoginPage", () => {
|
||||
|
||||
it("should include invitation token in OAuth state when invitation is present", async () => {
|
||||
const user = userEvent.setup();
|
||||
const mockBuildOAuthStateData = vi.fn((baseState: Record<string, string>) => ({
|
||||
...baseState,
|
||||
invitation_token: "inv-test-token-12345",
|
||||
}));
|
||||
const mockBuildOAuthStateData = vi.fn(
|
||||
(baseState: Record<string, string>) => ({
|
||||
...baseState,
|
||||
invitation_token: "inv-test-token-12345",
|
||||
}),
|
||||
);
|
||||
|
||||
useInvitationMock.mockReturnValue({
|
||||
invitationToken: "inv-test-token-12345",
|
||||
@@ -634,9 +687,14 @@ describe("LoginPage", () => {
|
||||
clearInvitation: vi.fn(),
|
||||
});
|
||||
|
||||
render(<RouterStub initialEntries={["/login?invitation_token=inv-url-token-67890"]} />, {
|
||||
wrapper: createWrapper(),
|
||||
});
|
||||
render(
|
||||
<RouterStub
|
||||
initialEntries={["/login?invitation_token=inv-url-token-67890"]}
|
||||
/>,
|
||||
{
|
||||
wrapper: createWrapper(),
|
||||
},
|
||||
);
|
||||
|
||||
await waitFor(() => {
|
||||
expect(screen.getByText("AUTH$INVITATION_PENDING")).toBeInTheDocument();
|
||||
|
||||
@@ -366,6 +366,130 @@ describe("MainApp", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("Re-authentication with stored login method", () => {
|
||||
it("should show ReauthModal instead of redirecting to /login when login method exists", async () => {
|
||||
// Arrange - user is unauthenticated but has a stored login method
|
||||
vi.spyOn(AuthService, "authenticate").mockRejectedValue({
|
||||
response: { status: 401 },
|
||||
isAxiosError: true,
|
||||
});
|
||||
|
||||
vi.stubGlobal("localStorage", {
|
||||
getItem: vi.fn((key: string) => {
|
||||
if (key === "openhands_login_method") {
|
||||
return "github";
|
||||
}
|
||||
return null;
|
||||
}),
|
||||
setItem: vi.fn(),
|
||||
removeItem: vi.fn(),
|
||||
clear: vi.fn(),
|
||||
});
|
||||
|
||||
// Act
|
||||
renderWithLoginStub(RouterStubWithLogin, ["/"]);
|
||||
|
||||
// Assert - should show ReauthModal (with "Logging back in" text), not redirect to /login
|
||||
await waitFor(
|
||||
() => {
|
||||
expect(screen.getByText("AUTH$LOGGING_BACK_IN")).toBeInTheDocument();
|
||||
},
|
||||
{ timeout: 2000 },
|
||||
);
|
||||
|
||||
// Login page should NOT be shown when login method exists
|
||||
expect(screen.queryByTestId("login-page")).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("should redirect to /login when no login method is stored", async () => {
|
||||
// Arrange - user is unauthenticated and has no stored login method
|
||||
vi.spyOn(AuthService, "authenticate").mockRejectedValue({
|
||||
response: { status: 401 },
|
||||
isAxiosError: true,
|
||||
});
|
||||
|
||||
vi.stubGlobal("localStorage", {
|
||||
getItem: vi.fn(() => null),
|
||||
setItem: vi.fn(),
|
||||
removeItem: vi.fn(),
|
||||
clear: vi.fn(),
|
||||
});
|
||||
|
||||
// Act
|
||||
renderWithLoginStub(RouterStubWithLogin, ["/"]);
|
||||
|
||||
// Assert - should redirect to /login
|
||||
await waitFor(
|
||||
() => {
|
||||
expect(screen.getByTestId("login-page")).toBeInTheDocument();
|
||||
},
|
||||
{ timeout: 2000 },
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("Loading states", () => {
|
||||
it("should show loading spinner while config is loading without redirecting", async () => {
|
||||
// Arrange - config never resolves (loading state)
|
||||
vi.spyOn(OptionService, "getConfig").mockImplementation(
|
||||
() => new Promise(() => {}),
|
||||
);
|
||||
|
||||
vi.stubGlobal("localStorage", {
|
||||
getItem: vi.fn((key: string) => {
|
||||
if (key === "openhands_login_method") {
|
||||
return "github";
|
||||
}
|
||||
return null;
|
||||
}),
|
||||
setItem: vi.fn(),
|
||||
removeItem: vi.fn(),
|
||||
clear: vi.fn(),
|
||||
});
|
||||
|
||||
// Act
|
||||
renderWithLoginStub(RouterStubWithLogin, ["/"]);
|
||||
|
||||
// Assert - should show loading spinner
|
||||
await waitFor(() => {
|
||||
expect(screen.getByTestId("loading-spinner")).toBeInTheDocument();
|
||||
});
|
||||
|
||||
// Should NOT redirect to login while loading
|
||||
expect(screen.queryByTestId("login-page")).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("should show loading spinner while auth is loading without redirecting", async () => {
|
||||
// Arrange - auth never resolves (loading state)
|
||||
vi.spyOn(AuthService, "authenticate").mockImplementation(
|
||||
() => new Promise(() => {}),
|
||||
);
|
||||
|
||||
vi.stubGlobal("localStorage", {
|
||||
getItem: vi.fn((key: string) => {
|
||||
if (key === "openhands_login_method") {
|
||||
return "github";
|
||||
}
|
||||
return null;
|
||||
}),
|
||||
setItem: vi.fn(),
|
||||
removeItem: vi.fn(),
|
||||
clear: vi.fn(),
|
||||
});
|
||||
|
||||
// Act
|
||||
renderWithLoginStub(RouterStubWithLogin, ["/"]);
|
||||
|
||||
// Assert - should show loading spinner
|
||||
await waitFor(() => {
|
||||
expect(screen.getByTestId("loading-spinner")).toBeInTheDocument();
|
||||
});
|
||||
|
||||
// Should NOT redirect to login while loading
|
||||
expect(screen.queryByTestId("login-page")).not.toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
||||
describe("Invitation URL Parameters", () => {
|
||||
beforeEach(() => {
|
||||
vi.spyOn(AuthService, "authenticate").mockRejectedValue({
|
||||
|
||||
@@ -40,11 +40,18 @@ export default function LoginPage() {
|
||||
}, [config.isLoading, config.data?.app_mode, navigate]);
|
||||
|
||||
// Redirect authenticated users away from login page
|
||||
// Preserve login_method param so useAuthCallback can store it for auto-login
|
||||
React.useEffect(() => {
|
||||
if (!isAuthLoading && isAuthed) {
|
||||
navigate(returnTo, { replace: true });
|
||||
const loginMethod = searchParams.get("login_method");
|
||||
let destination = returnTo;
|
||||
if (loginMethod) {
|
||||
const separator = returnTo.includes("?") ? "&" : "?";
|
||||
destination = `${returnTo}${separator}login_method=${encodeURIComponent(loginMethod)}`;
|
||||
}
|
||||
navigate(destination, { replace: true });
|
||||
}
|
||||
}, [isAuthed, isAuthLoading, navigate, returnTo]);
|
||||
}, [isAuthed, isAuthLoading, navigate, returnTo, searchParams]);
|
||||
|
||||
if (isAuthLoading || config.isLoading) {
|
||||
return (
|
||||
|
||||
@@ -173,14 +173,17 @@ export default function MainApp() {
|
||||
setLoginMethodExists(checkLoginMethodExists());
|
||||
}, [isAuthed, checkLoginMethodExists]);
|
||||
|
||||
// Show loading spinner while config or auth is loading
|
||||
const isLoading = config.isLoading || isAuthLoading;
|
||||
|
||||
// Only decide to redirect AFTER loading completes
|
||||
const shouldRedirectToLogin =
|
||||
config.isLoading ||
|
||||
isAuthLoading ||
|
||||
(!isAuthed &&
|
||||
!isAuthError &&
|
||||
!isOnIntermediatePage &&
|
||||
config.data?.app_mode === "saas" &&
|
||||
!loginMethodExists);
|
||||
!isLoading &&
|
||||
!isAuthed &&
|
||||
!isAuthError &&
|
||||
!isOnIntermediatePage &&
|
||||
config.data?.app_mode === "saas" &&
|
||||
!loginMethodExists;
|
||||
|
||||
React.useEffect(() => {
|
||||
if (shouldRedirectToLogin) {
|
||||
@@ -197,7 +200,8 @@ export default function MainApp() {
|
||||
}
|
||||
}, [shouldRedirectToLogin, pathname, searchParams, navigate]);
|
||||
|
||||
if (shouldRedirectToLogin) {
|
||||
// Show loading spinner while loading OR when about to redirect
|
||||
if (isLoading || shouldRedirectToLogin) {
|
||||
return (
|
||||
<div className="min-h-screen flex items-center justify-center bg-base">
|
||||
<LoadingSpinner size="large" />
|
||||
|
||||
Reference in New Issue
Block a user