diff --git a/frontend/src/auth/AuthProvider.test.tsx b/frontend/src/auth/AuthProvider.test.tsx index e96e138087..e3023b1e28 100644 --- a/frontend/src/auth/AuthProvider.test.tsx +++ b/frontend/src/auth/AuthProvider.test.tsx @@ -1,3 +1,4 @@ +import { StrictMode } from "react"; import { render, screen, waitFor } from "@testing-library/react"; // --------------------------------------------------------------------------- @@ -26,11 +27,8 @@ jest.mock("./msalConfig", () => ({ })); const mockSetMsalInstance = jest.fn(); -const mockSetClientId = jest.fn(); - jest.mock("../services/api", () => ({ setMsalInstance: (...args: unknown[]) => mockSetMsalInstance(...args), - setClientId: (...args: unknown[]) => mockSetClientId(...args), })); // MSAL browser mocks — PublicClientApplication instance methods @@ -39,7 +37,6 @@ const mockHandleRedirectPromise = jest.fn().mockResolvedValue(null); const mockGetActiveAccount = jest.fn().mockReturnValue(null); const mockGetAllAccounts = jest.fn().mockReturnValue([]); const mockSetActiveAccount = jest.fn(); -const mockAddEventCallback = jest.fn(); const mockLoginRedirect = jest.fn().mockResolvedValue(undefined); jest.mock("@azure/msal-browser", () => ({ @@ -49,7 +46,6 @@ jest.mock("@azure/msal-browser", () => ({ getActiveAccount: mockGetActiveAccount, getAllAccounts: mockGetAllAccounts, setActiveAccount: mockSetActiveAccount, - addEventCallback: mockAddEventCallback, loginRedirect: mockLoginRedirect, })), EventType: { LOGIN_SUCCESS: "msal:loginSuccess" }, @@ -78,6 +74,7 @@ jest.mock("@azure/msal-react", () => ({ })); import { AuthProvider } from "./AuthProvider"; +import type { AuthConfig } from "./msalConfig"; // --------------------------------------------------------------------------- // Tests @@ -95,8 +92,11 @@ describe("AuthProvider", () => { }); // Test 10: fetchAuthConfig never resolves → stuck in loading state - it("shows loading state while initializing", () => { - mockFetchAuthConfig.mockReturnValue(new Promise(() => {})); + it("shows loading state while initializing", async () => { + let resolveConfig: (config: AuthConfig) => void = () => {} + mockFetchAuthConfig.mockReturnValue(new Promise((resolve) => { + resolveConfig = resolve + })); render( @@ -105,6 +105,8 @@ describe("AuthProvider", () => { ); expect(screen.getByText("Initializing authentication...")).toBeVisible(); + resolveConfig({ clientId: "", tenantId: "", allowedGroupIds: "" }); + await screen.findByText("Child"); }); // Test 11: empty clientId + tenantId → auth disabled, children render directly @@ -200,7 +202,29 @@ describe("AuthProvider", () => { expect(mockInitialize).toHaveBeenCalled(); expect(mockHandleRedirectPromise).toHaveBeenCalled(); expect(mockSetMsalInstance).toHaveBeenCalled(); - expect(mockSetClientId).toHaveBeenCalledWith("test-client"); + }); + + it("handles the redirect once when Strict Mode replays effects", async () => { + mockFetchAuthConfig.mockResolvedValue({ + clientId: "test-client", + tenantId: "test-tenant", + allowedGroupIds: "g1", + }); + + render( + + +
Child
+
+
+ ); + + await waitFor(() => { + expect(screen.getByTestId("msal-provider")).toBeInTheDocument(); + }); + expect(mockHandleRedirectPromise).toHaveBeenCalledTimes(1); + expect(mockHandleRedirectPromise).toHaveBeenCalledWith({ navigateToLoginRequestUrl: false }); + expect(mockLoginRedirect).toHaveBeenCalledTimes(1); }); // Test 16: handleRedirectPromise returns account → setActiveAccount called diff --git a/frontend/src/auth/AuthProvider.tsx b/frontend/src/auth/AuthProvider.tsx index d619d03a48..34577c7b9b 100644 --- a/frontend/src/auth/AuthProvider.tsx +++ b/frontend/src/auth/AuthProvider.tsx @@ -10,12 +10,8 @@ * - Shows a loading state while auth initializes */ -import { useState, useEffect, type ReactNode } from 'react' -import { - PublicClientApplication, - EventType, - type AuthenticationResult, -} from '@azure/msal-browser' +import { useState, useEffect, useRef, type ReactNode } from 'react' +import { PublicClientApplication } from '@azure/msal-browser' import { MsalProvider, AuthenticatedTemplate, @@ -23,25 +19,28 @@ import { useMsal, } from '@azure/msal-react' import { fetchAuthConfig, buildMsalConfig, buildLoginRequest, type AuthConfig } from './msalConfig' -import { AuthConfigContext, useAuthConfig } from './AuthConfigContext' -import { setMsalInstance as setApiMsalInstance, setClientId as setApiClientId } from '../services/api' +import { AuthConfigContext } from './AuthConfigContext' +import { setMsalInstance as setApiMsalInstance } from '../services/api' function LoginRedirect() { const { instance } = useMsal() - const config = useAuthConfig() + const loginStarted = useRef(false) useEffect(() => { + if (loginStarted.current) return + loginStarted.current = true + // Capture the path the user originally requested so it can be restored // after the login round-trip (see the redirect handling in initMsal). instance .loginRedirect({ - ...buildLoginRequest(config.clientId), + ...buildLoginRequest(), state: window.location.pathname + window.location.search, }) .catch((error) => { console.error('Login redirect failed:', error) }) - }, [instance, config]) + }, [instance]) return
Redirecting to login...
} @@ -67,47 +66,33 @@ export function AuthProvider({ children }: AuthProviderProps) { const [authDisabled, setAuthDisabled] = useState(false) const [error, setError] = useState(null) const [authConfig, setAuthConfig] = useState({clientId: '', tenantId: '', allowedGroupIds: ''}) + const initializationStarted = useRef(false) useEffect(() => { - let cancelled = false + if (initializationStarted.current) return + initializationStarted.current = true async function initMsal() { try { const config = await fetchAuthConfig() + setAuthConfig(config) - // If no auth config (local dev), skip MSAL entirely if (!config.clientId || !config.tenantId) { - if (!cancelled) { - setMsalInstance(null) // null signals "auth disabled" - setAuthDisabled(true) - setAuthConfig(config) - } + setAuthDisabled(true) return } - const msalConfig = buildMsalConfig(config) - const instance = new PublicClientApplication(msalConfig) + const instance = new PublicClientApplication(buildMsalConfig(config)) await instance.initialize() + const redirectResult = await instance.handleRedirectPromise({ navigateToLoginRequestUrl: false }) - // Handle redirect response (after coming back from login) - instance.addEventCallback((event) => { - if (event.eventType === EventType.LOGIN_SUCCESS && event.payload) { - const result = event.payload as AuthenticationResult - instance.setActiveAccount(result.account) - } - }) - - // Await the redirect promise FIRST — on the initial redirect back - // from Entra, this caches the token and returns the auth result. - // On normal page loads (no redirect hash) it resolves to null. - const redirectResult = await instance.handleRedirectPromise() if (redirectResult?.account) { instance.setActiveAccount(redirectResult.account) + } else if (!instance.getActiveAccount()) { + const accounts = instance.getAllAccounts() + if (accounts.length > 0) instance.setActiveAccount(accounts[0]) } - // Restore the deep link the user originally requested, captured as - // MSAL state before the login redirect (see LoginRedirect). Only - // same-origin paths are honored, guarding against an open redirect. const requestedPath = typeof redirectResult?.state === 'string' ? redirectResult.state : null if ( requestedPath && @@ -117,33 +102,16 @@ export function AuthProvider({ children }: AuthProviderProps) { window.history.replaceState(null, '', requestedPath) } - // Fall back to any cached account from a previous session - if (!instance.getActiveAccount()) { - const accounts = instance.getAllAccounts() - if (accounts.length > 0) { - instance.setActiveAccount(accounts[0]) - } - } - - if (!cancelled) { - // Wire MSAL into the API client BEFORE React re-render, - // so child components' effects already have the token available. - setApiMsalInstance(instance) - setApiClientId(config.clientId) - setMsalInstance(instance) - setAuthConfig(config) - } + // Wire MSAL into the API client BEFORE React re-render, + // so child components' effects already have the token available. + setApiMsalInstance(instance) + setMsalInstance(instance) } catch (err) { - if (!cancelled) { - setError(err instanceof Error ? err.message : 'Failed to initialize authentication') - } + setError(err instanceof Error ? err.message : 'Failed to initialize authentication') } } initMsal() - return () => { - cancelled = true - } }, []) if (error) { diff --git a/frontend/src/auth/msalConfig.test.ts b/frontend/src/auth/msalConfig.test.ts index 2e36a518f8..bda21bf5fa 100644 --- a/frontend/src/auth/msalConfig.test.ts +++ b/frontend/src/auth/msalConfig.test.ts @@ -4,31 +4,19 @@ jest.mock("@azure/msal-browser", () => ({ LogLevel: { Warning: 3 }, })); -import { buildMsalConfig, getApiScopes, buildLoginRequest } from "./msalConfig"; +import { buildMsalConfig, getGraphScopes, buildLoginRequest } from "./msalConfig"; describe("msalConfig", () => { - // Tests 1-2: getApiScopes — two branches on the !clientId check (line 75) - describe("getApiScopes", () => { - it("returns default scopes when clientId is empty", () => { - expect(getApiScopes("")).toEqual(["openid", "profile", "email"]); - }); - - it("returns client-specific scope when clientId is provided", () => { - expect(getApiScopes("my-client-id")).toEqual(["api://my-client-id/access"]); + describe("getGraphScopes", () => { + it("returns the delegated Graph scope", () => { + expect(getGraphScopes()).toEqual(["User.Read"]); }); }); - // Tests 3-4: buildLoginRequest — wraps getApiScopes in { scopes } describe("buildLoginRequest", () => { - it("builds request with client-specific scopes", () => { - expect(buildLoginRequest("my-client-id")).toEqual({ - scopes: ["api://my-client-id/access"], - }); - }); - - it("builds request with default scopes when clientId is empty", () => { - expect(buildLoginRequest("")).toEqual({ - scopes: ["openid", "profile", "email"], + it("builds request with Graph scopes", () => { + expect(buildLoginRequest()).toEqual({ + scopes: ["User.Read"], }); }); }); diff --git a/frontend/src/auth/msalConfig.ts b/frontend/src/auth/msalConfig.ts index fa7d586bce..c63e3a68cf 100644 --- a/frontend/src/auth/msalConfig.ts +++ b/frontend/src/auth/msalConfig.ts @@ -8,12 +8,15 @@ * endpoint (served by the backend from environment variables). This avoids * hardcoding tenant-specific values in the frontend bundle. * - * Uses access tokens (not ID tokens) with an API-specific scope so that - * Entra ID includes the `groups` claim for group-based authorization. + * Uses a delegated Microsoft Graph access token. The backend forwards this + * token only to trusted Graph endpoints to authenticate the user and resolve + * group membership. */ import { type Configuration, LogLevel } from '@azure/msal-browser' +const GRAPH_USER_READ_SCOPE = 'User.Read' + export interface AuthConfig { clientId: string tenantId: string @@ -54,23 +57,13 @@ export function buildMsalConfig(authConfig: AuthConfig): Configuration { } } -/** - * Build the API scopes for token acquisition. - * - * Requests the app's custom `access` scope so the resulting access token has - * `aud` equal to the app's client ID. The explicit scope name avoids the - * `.default` shorthand, which resolves via `requiredResourceAccess` and - * triggers mandatory admin consent in the Microsoft corporate tenant. - * - * The `access` scope is defined under "Expose an API" on the app registration. - */ -export function getApiScopes(clientId: string): string[] { - if (!clientId) return ['openid', 'profile', 'email'] - return [`api://${clientId}/access`] +/** Build the delegated Microsoft Graph scopes used for authentication. */ +export function getGraphScopes(): string[] { + return [GRAPH_USER_READ_SCOPE] } -export function buildLoginRequest(clientId: string) { +export function buildLoginRequest(): { scopes: string[] } { return { - scopes: getApiScopes(clientId), + scopes: getGraphScopes(), } } diff --git a/frontend/src/components/UserAccountButton.test.tsx b/frontend/src/components/UserAccountButton.test.tsx index 73035e928d..f0fa8d755f 100644 --- a/frontend/src/components/UserAccountButton.test.tsx +++ b/frontend/src/components/UserAccountButton.test.tsx @@ -26,7 +26,7 @@ jest.mock('../auth/AuthConfigContext', () => ({ })) jest.mock('../auth/msalConfig', () => ({ - buildLoginRequest: (clientId: string) => ({ scopes: [`${clientId}/.default`] }), + buildLoginRequest: () => ({ scopes: ['User.Read'] }), })) const TestWrapper: React.FC<{ children: React.ReactNode }> = ({ children }) => ( @@ -78,7 +78,7 @@ describe('UserAccountButton', () => { await user.click(screen.getByRole('button', { name: /log in/i })) - expect(mockLoginRedirect).toHaveBeenCalledWith({ scopes: ['test-client-id/.default'] }) + expect(mockLoginRedirect).toHaveBeenCalledWith({ scopes: ['User.Read'] }) }) it('renders user display name and Sign Out when account exists', () => { diff --git a/frontend/src/components/UserAccountButton.tsx b/frontend/src/components/UserAccountButton.tsx index 9d258e6239..4bdc59bdd9 100644 --- a/frontend/src/components/UserAccountButton.tsx +++ b/frontend/src/components/UserAccountButton.tsx @@ -19,7 +19,6 @@ import { useUserAccountButtonStyles } from './UserAccountButton.styles' function MsalAccountButton() { const styles = useUserAccountButtonStyles() const { instance, accounts } = useMsal() - const config = useAuthConfig() const account = instance.getActiveAccount() ?? accounts[0] if (!account) { @@ -28,7 +27,7 @@ function MsalAccountButton() {