From aa24360714ccfdbf9b41a285b4d935c3e3957846 Mon Sep 17 00:00:00 2001 From: Shivaji Byrapaneni Date: Tue, 8 Sep 2026 10:18:01 +0100 Subject: [PATCH 1/5] feat(calm-hub): OIDC-driven auth config and VS Code plugin login MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extracted from #3001. Serves OIDC config to the SPA from the server (/api/calm/auth/config) instead of build-time constants, adds the VS Code plugin's browser-based OIDC login flow (PluginAuthResource + OidcPluginAuthClient), and lets namespace/domain read access be granted via any UserAccessStore namespace grant, not just the existing UserAccessValidator path — needed for backends where that validator isn't resolvable. Original-PR: finos/architecture-as-code#3001 --- calm-hub-ui/src/ProtectedRoute.test.tsx | 182 ++++++++ calm-hub-ui/src/ProtectedRoute.tsx | 13 + calm-hub-ui/src/authConfig.test.ts | 74 ++++ calm-hub-ui/src/authConfig.ts | 56 +++ calm-hub-ui/src/authService.test.tsx | 6 + calm-hub-ui/src/authService.tsx | 75 ++-- calm-hub-ui/src/components/navbar/Navbar.tsx | 3 + .../components/user-menu/UserMenu.test.tsx | 55 +++ .../src/components/user-menu/UserMenu.tsx | 64 +++ .../components/intro-screen/IntroScreen.tsx | 5 +- calm-hub-ui/src/index.tsx | 36 +- .../integration/ProxyAuthIntegration.java | 4 +- .../calm/resources/AuthConfigResource.java | 68 +++ .../calm/resources/NamespaceResource.java | 33 +- .../calm/resources/PluginAuthResource.java | 231 ++++++++++ .../security/CalmHubPermissionChecker.java | 15 +- .../calm/security/OidcPluginAuthClient.java | 155 +++++++ .../finos/calm/security/OidcRoleResolver.java | 76 ++++ .../resources/application-oidc.properties | 54 +++ .../TestAuthConfigResourceShould.java | 118 +++++ .../TestNamespaceResourceFilteringShould.java | 50 +++ .../TestPluginAuthResourceShould.java | 410 ++++++++++++++++++ .../TestCalmHubPermissionCheckerShould.java | 4 +- .../TestOidcPluginAuthClientShould.java | 268 ++++++++++++ .../security/TestOidcRoleResolverShould.java | 112 +++++ 25 files changed, 2110 insertions(+), 57 deletions(-) create mode 100644 calm-hub-ui/src/ProtectedRoute.test.tsx create mode 100644 calm-hub-ui/src/authConfig.test.ts create mode 100644 calm-hub-ui/src/authConfig.ts create mode 100644 calm-hub-ui/src/components/user-menu/UserMenu.test.tsx create mode 100644 calm-hub-ui/src/components/user-menu/UserMenu.tsx create mode 100644 calm-hub/src/main/java/org/finos/calm/resources/AuthConfigResource.java create mode 100644 calm-hub/src/main/java/org/finos/calm/resources/PluginAuthResource.java create mode 100644 calm-hub/src/main/java/org/finos/calm/security/OidcPluginAuthClient.java create mode 100644 calm-hub/src/main/java/org/finos/calm/security/OidcRoleResolver.java create mode 100644 calm-hub/src/main/resources/application-oidc.properties create mode 100644 calm-hub/src/test/java/org/finos/calm/resources/TestAuthConfigResourceShould.java create mode 100644 calm-hub/src/test/java/org/finos/calm/resources/TestPluginAuthResourceShould.java create mode 100644 calm-hub/src/test/java/org/finos/calm/security/TestOidcPluginAuthClientShould.java create mode 100644 calm-hub/src/test/java/org/finos/calm/security/TestOidcRoleResolverShould.java diff --git a/calm-hub-ui/src/ProtectedRoute.test.tsx b/calm-hub-ui/src/ProtectedRoute.test.tsx new file mode 100644 index 000000000..07d2fcc0e --- /dev/null +++ b/calm-hub-ui/src/ProtectedRoute.test.tsx @@ -0,0 +1,182 @@ +import { render, screen, waitFor } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('./authService.js', () => ({ + authService: { + getUser: vi.fn(), + login: vi.fn(), + processRedirect: vi.fn(), + }, +})); + + +import ProtectedRoute from './ProtectedRoute.js'; +import { authService } from './authService.js'; + +const PRE_AUTH_HASH_KEY = 'calm_pre_auth_hash'; + +const fakeUser = { + expired: false, + id_token: 'test-id-token', + access_token: 'test-access-token', + profile: { preferred_username: 'testuser' }, +} as unknown as import('oidc-client-ts').User; + +describe('ProtectedRoute', () => { + let replaceStateSpy: ReturnType; + + beforeEach(() => { + vi.clearAllMocks(); + sessionStorage.clear(); + replaceStateSpy = vi.spyOn(window.history, 'replaceState'); + }); + + afterEach(() => { + replaceStateSpy.mockRestore(); + Object.defineProperty(window, 'location', { + value: window.location, + writable: true, + }); + }); + + describe('hash preservation before OIDC redirect', () => { + it('saves window.location.hash to sessionStorage before calling login', async () => { + vi.mocked(authService.getUser).mockResolvedValue(null); + vi.mocked(authService.login).mockResolvedValue(undefined); + Object.defineProperty(window, 'location', { + value: { ...window.location, hash: '#/fae-calm/architectures/123/abc', search: '' }, + writable: true, + }); + + render( + +
Protected Content
+
+ ); + + await waitFor(() => { + expect(authService.login).toHaveBeenCalled(); + }); + expect(sessionStorage.getItem(PRE_AUTH_HASH_KEY)).toBe('#/fae-calm/architectures/123/abc'); + }); + + it('does not save an empty hash to sessionStorage', async () => { + vi.mocked(authService.getUser).mockResolvedValue(null); + vi.mocked(authService.login).mockResolvedValue(undefined); + Object.defineProperty(window, 'location', { + value: { ...window.location, hash: '', search: '' }, + writable: true, + }); + + render( + +
Protected Content
+
+ ); + + await waitFor(() => { + expect(authService.login).toHaveBeenCalled(); + }); + expect(sessionStorage.getItem(PRE_AUTH_HASH_KEY)).toBeNull(); + }); + }); + + describe('hash restoration after OIDC callback', () => { + it('restores the saved hash after processing the redirect callback', async () => { + vi.mocked(authService.getUser).mockResolvedValue(null); + vi.mocked(authService.processRedirect).mockResolvedValue(fakeUser); + sessionStorage.setItem(PRE_AUTH_HASH_KEY, '#/fae-calm/architectures/123/abc'); + Object.defineProperty(window, 'location', { + value: { ...window.location, search: '?code=AUTH_CODE&state=xyz', hash: '', pathname: '/' }, + writable: true, + }); + + render( + +
Protected Content
+
+ ); + + await waitFor(() => { + expect(replaceStateSpy).toHaveBeenCalledWith( + null, + '', + '/#/fae-calm/architectures/123/abc' + ); + }); + expect(sessionStorage.getItem(PRE_AUTH_HASH_KEY)).toBeNull(); + }); + + it('falls back to #/ when no hash was saved', async () => { + vi.mocked(authService.getUser).mockResolvedValue(null); + vi.mocked(authService.processRedirect).mockResolvedValue(fakeUser); + Object.defineProperty(window, 'location', { + value: { ...window.location, search: '?code=AUTH_CODE&state=xyz', hash: '', pathname: '/' }, + writable: true, + }); + + render( + +
Protected Content
+
+ ); + + await waitFor(() => { + expect(replaceStateSpy).toHaveBeenCalledWith(null, '', '/#/'); + }); + }); + + it('falls back to #/ when saved hash is just #', async () => { + vi.mocked(authService.getUser).mockResolvedValue(null); + vi.mocked(authService.processRedirect).mockResolvedValue(fakeUser); + sessionStorage.setItem(PRE_AUTH_HASH_KEY, '#'); + Object.defineProperty(window, 'location', { + value: { ...window.location, search: '?code=AUTH_CODE&state=xyz', hash: '', pathname: '/' }, + writable: true, + }); + + render( + +
Protected Content
+
+ ); + + await waitFor(() => { + expect(replaceStateSpy).toHaveBeenCalledWith(null, '', '/#/'); + }); + }); + + it('renders children after successful redirect processing', async () => { + vi.mocked(authService.getUser).mockResolvedValue(null); + vi.mocked(authService.processRedirect).mockResolvedValue(fakeUser); + Object.defineProperty(window, 'location', { + value: { ...window.location, search: '?code=AUTH_CODE&state=xyz', hash: '', pathname: '/' }, + writable: true, + }); + + render( + +
Protected Content
+
+ ); + + expect(await screen.findByText('Protected Content')).toBeInTheDocument(); + }); + }); + + describe('already authenticated user', () => { + it('renders children immediately without redirect when session exists', async () => { + vi.mocked(authService.getUser).mockResolvedValue(fakeUser); + + render( + +
Protected Content
+
+ ); + + expect(await screen.findByText('Protected Content')).toBeInTheDocument(); + expect(authService.login).not.toHaveBeenCalled(); + expect(authService.processRedirect).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/calm-hub-ui/src/ProtectedRoute.tsx b/calm-hub-ui/src/ProtectedRoute.tsx index 64ff61be7..035f26032 100644 --- a/calm-hub-ui/src/ProtectedRoute.tsx +++ b/calm-hub-ui/src/ProtectedRoute.tsx @@ -6,6 +6,8 @@ interface ProtectedRouteProps { children: ReactNode; } +const PRE_AUTH_HASH_KEY = 'calm_pre_auth_hash'; + const ProtectedRoute: React.FC = ({ children }) => { const [user, setUser] = useState(null); const [loading, setLoading] = useState(true); @@ -17,8 +19,18 @@ const ProtectedRoute: React.FC = ({ children }) => { setUser(currentUser); } else if (window.location.search.includes('code=')) { const loggedInUser = await authService.processRedirect(); + const savedHash = sessionStorage.getItem(PRE_AUTH_HASH_KEY); + sessionStorage.removeItem(PRE_AUTH_HASH_KEY); + if (savedHash && savedHash !== '#' && savedHash !== '#/') { + window.history.replaceState(null, '', window.location.pathname + savedHash); + } else { + window.history.replaceState(null, '', window.location.pathname + '#/'); + } setUser(loggedInUser); } else { + if (window.location.hash) { + sessionStorage.setItem(PRE_AUTH_HASH_KEY, window.location.hash); + } await authService.login(); } setLoading(false); @@ -34,6 +46,7 @@ const ProtectedRoute: React.FC = ({ children }) => { if (!user) { return
Redirecting to login...
; } + return <>{children}; }; export default ProtectedRoute; diff --git a/calm-hub-ui/src/authConfig.test.ts b/calm-hub-ui/src/authConfig.test.ts new file mode 100644 index 000000000..729d151d8 --- /dev/null +++ b/calm-hub-ui/src/authConfig.test.ts @@ -0,0 +1,74 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import axios from 'axios'; +import { getAuthConfig, isOidcEnabled, isGitHubMode } from './authConfig.js'; + +vi.mock('axios'); + +describe('authConfig', () => { + beforeEach(() => { + vi.resetModules(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + describe('fetchAuthConfig', () => { + it('should fetch config from backend', async () => { + const mockConfig = { + oidc: { enabled: true, provider: 'entra-id', authority: 'https://login.microsoft.com/tenant', clientId: 'client-123', scopes: ['openid', 'profile'] }, + databaseMode: 'github', + }; + vi.mocked(axios.get).mockResolvedValue({ data: mockConfig }); + + const { fetchAuthConfig: fetch } = await import('./authConfig.js'); + const result = await fetch(); + + expect(result.oidc.enabled).toBe(true); + expect(result.oidc.provider).toBe('entra-id'); + expect(result.databaseMode).toBe('github'); + }); + + it('should retry once and succeed on transient failure', async () => { + const mockConfig = { + oidc: { enabled: true, provider: 'generic-oidc' }, + databaseMode: 'mongo', + }; + vi.mocked(axios.get) + .mockRejectedValueOnce(new Error('Network error')) + .mockResolvedValueOnce({ data: mockConfig }); + + const { fetchAuthConfig: fetch } = await import('./authConfig.js'); + const result = await fetch(); + + expect(result.oidc.enabled).toBe(true); + }); + + it('should throw when both attempts fail', async () => { + vi.mocked(axios.get).mockRejectedValue(new Error('Network error')); + + const { fetchAuthConfig: fetch } = await import('./authConfig.js'); + + await expect(fetch()).rejects.toThrow('Unable to load authentication configuration'); + }); + }); + + describe('getAuthConfig', () => { + it('should return default config before fetch', () => { + const config = getAuthConfig(); + expect(config.oidc.enabled).toBe(false); + }); + }); + + describe('isOidcEnabled', () => { + it('should return false when not fetched', () => { + expect(isOidcEnabled()).toBe(false); + }); + }); + + describe('isGitHubMode', () => { + it('should return false when not fetched', () => { + expect(isGitHubMode()).toBe(false); + }); + }); +}); diff --git a/calm-hub-ui/src/authConfig.ts b/calm-hub-ui/src/authConfig.ts new file mode 100644 index 000000000..c7b20d24c --- /dev/null +++ b/calm-hub-ui/src/authConfig.ts @@ -0,0 +1,56 @@ +import axios from 'axios'; + +export interface AuthConfig { + oidc: { + enabled: boolean; + provider?: string; + authority?: string; + clientId?: string; + scopes?: string[]; + redirectUri?: string; + }; + databaseMode: string; +} + +const DEFAULT_CONFIG: AuthConfig = { + oidc: { enabled: false }, + databaseMode: 'mongo', +}; + +let cachedConfig: AuthConfig | null = null; + +export async function fetchAuthConfig(): Promise { + if (cachedConfig) { + return cachedConfig; + } + const attempt = async (): Promise => { + const response = await axios.get('/api/calm/auth/config'); + return response.data; + }; + try { + cachedConfig = await attempt(); + return cachedConfig; + } catch { + // Retry once after a short delay before giving up + try { + await new Promise((resolve) => setTimeout(resolve, 1000)); + cachedConfig = await attempt(); + return cachedConfig; + } catch (retryError) { + console.error('Failed to fetch auth config after retry:', retryError); + throw new Error('Unable to load authentication configuration'); + } + } +} + +export function getAuthConfig(): AuthConfig { + return cachedConfig || DEFAULT_CONFIG; +} + +export function isOidcEnabled(): boolean { + return cachedConfig?.oidc?.enabled ?? false; +} + +export function isGitHubMode(): boolean { + return cachedConfig?.databaseMode === 'github'; +} diff --git a/calm-hub-ui/src/authService.test.tsx b/calm-hub-ui/src/authService.test.tsx index e9fe517ad..2d7b11dc2 100644 --- a/calm-hub-ui/src/authService.test.tsx +++ b/calm-hub-ui/src/authService.test.tsx @@ -6,6 +6,7 @@ import { getAuthHeaders, isAuthServiceEnabled, } from './authService.js'; +import * as authConfig from './authConfig.js'; vi.mock('axios'); @@ -16,6 +17,11 @@ describe('authService', () => { describe('checkAuthorityService', () => { it('should return true when the authority service responds successfully', async () => { + vi.spyOn(authConfig, 'getAuthConfig').mockReturnValue({ + oidc: { enabled: true, authority: 'https://auth.example.com' }, + github: { enabled: false }, + databaseMode: 'mongo', + }); vi.mocked(axios.head).mockResolvedValue({ status: 200 }); const result = await checkAuthorityService(); expect(result).toBe(true); diff --git a/calm-hub-ui/src/authService.tsx b/calm-hub-ui/src/authService.tsx index 8e5457777..517edc8e3 100644 --- a/calm-hub-ui/src/authService.tsx +++ b/calm-hub-ui/src/authService.tsx @@ -1,35 +1,40 @@ -import { UserManager, Log, User } from 'oidc-client-ts'; +import { UserManager, User } from 'oidc-client-ts'; import axios from 'axios'; +import { fetchAuthConfig, getAuthConfig } from './authConfig.js'; -const config = { - authority: 'https://calm-hub.finos.org:9443/realms/calm-hub-realm', - client_id: 'calm-hub-authz-code', - redirect_uri: window.location.origin, - response_type: 'code', - scope: 'openid profile architectures:read adrs:all', - post_logout_redirect_uri: window.location.origin, - automaticSilentRenew: true, - filterProtocolClaims: true, - loadUserInfo: true, -}; - -//Set AUTH_SERVICE_OIDC_ENABLE to true only when the backend is running with a secure profile and is NOT behind an ADC/Reverse-Proxy that handles user authentication. -export const AUTH_SERVICE_OIDC_ENABLE: boolean = false; let userManager: UserManager | null = null; +let initialized = false; export function isAuthServiceEnabled(): boolean { - const oidcEnabled = AUTH_SERVICE_OIDC_ENABLE; - const isHttps = - typeof window !== 'undefined' && - typeof window.location !== 'undefined' && - window.location.protocol === 'https:'; - return (oidcEnabled && isHttps); + const config = getAuthConfig(); + return config.oidc.enabled; } -if (isAuthServiceEnabled()) { - userManager = new UserManager(config); - Log.setLogger(console); - Log.setLevel(Log.INFO); +export async function initAuthService(): Promise { + if (initialized) return; + + const config = await fetchAuthConfig(); + if (!config.oidc.enabled) { + initialized = true; + return; + } + + const oidcConfig = { + authority: config.oidc.authority || '', + client_id: config.oidc.clientId || '', + redirect_uri: config.oidc.redirectUri + ? new URL(config.oidc.redirectUri, window.location.origin).toString() + : window.location.origin, + response_type: 'code', + scope: config.oidc.scopes?.join(' ') || 'openid profile email', + post_logout_redirect_uri: window.location.origin, + automaticSilentRenew: true, + filterProtocolClaims: true, + loadUserInfo: true, + }; + + userManager = new UserManager(oidcConfig); + initialized = true; } export async function getUser(): Promise { @@ -61,25 +66,26 @@ export async function logout(): Promise { export async function clearSession(): Promise { try { await userManager?.removeUser(); - console.log('Session cleared successfully.'); } catch (error) { console.error('Error clearing session:', error); } } export async function getToken(): Promise { - if (!AUTH_SERVICE_OIDC_ENABLE) { + if (!userManager) { return ''; } - const user = await userManager?.getUser(); + const user = await userManager.getUser(); if (user && !user.expired) { - return user.access_token; + // Entra ID: access_token audience is MS Graph, not our API. + // Send the id_token which has our client_id as audience. + return user.id_token || user.access_token; } if (user && user.expired) { try { - const refreshedUser = await userManager?.signinSilent(); - return refreshedUser?.access_token || ''; + const refreshedUser = await userManager.signinSilent(); + return refreshedUser?.id_token || refreshedUser?.access_token || ''; } catch (error) { console.error('Error refreshing token:', error); return ''; @@ -100,8 +106,12 @@ export async function getAuthHeaders(): Promise { } export async function checkAuthorityService(): Promise { + const config = getAuthConfig(); + if (!config.oidc.enabled || !config.oidc.authority) { + return false; + } try { - await axios.head(config.authority); + await axios.head(config.oidc.authority); return true; } catch (error) { console.error('Authority Service Check Error:', error); @@ -118,4 +128,5 @@ export const authService = { getToken, getAuthHeaders, isAuthServiceEnabled, + initAuthService, }; diff --git a/calm-hub-ui/src/components/navbar/Navbar.tsx b/calm-hub-ui/src/components/navbar/Navbar.tsx index bbacb1c79..541878a5e 100644 --- a/calm-hub-ui/src/components/navbar/Navbar.tsx +++ b/calm-hub-ui/src/components/navbar/Navbar.tsx @@ -6,6 +6,8 @@ import { GlobalSearchBar } from './GlobalSearchBar.js'; import { ThemeToggle } from './ThemeToggle.js'; import { useTheme } from '../../theme/useTheme.js'; import { UserAccessContext } from '../../admin/context/UserAccessContext.js'; +import { UserMenu } from '../user-menu/UserMenu.js'; +import { isAuthServiceEnabled } from '../../authService.js'; /** * The navy-and-blue lockup is unreadable on a dark base, so dark gets a white one. @@ -58,6 +60,7 @@ export function Navbar({ storage }: NavbarProps = {}) {
+ {isAuthServiceEnabled() && } {/* Backdrop — dims the page behind the drawer; tap to close */} diff --git a/calm-hub-ui/src/components/user-menu/UserMenu.test.tsx b/calm-hub-ui/src/components/user-menu/UserMenu.test.tsx new file mode 100644 index 000000000..887a86338 --- /dev/null +++ b/calm-hub-ui/src/components/user-menu/UserMenu.test.tsx @@ -0,0 +1,55 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, screen, fireEvent, waitFor } from '@testing-library/react'; +import { UserMenu } from './UserMenu.js'; + +vi.mock('../../authService.js', () => ({ + getUser: vi.fn().mockResolvedValue({ + profile: { + name: 'Shivaji Byrapaneni', + preferred_username: 'shivaji.byrapaneni@fmr.com', + email: 'shivaji.byrapaneni@fmr.com', + }, + }), + authService: { + logout: vi.fn().mockResolvedValue(undefined), + }, +})); + +describe('UserMenu', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('should show user initial and name when loaded', async () => { + render(); + + await waitFor(() => { + expect(screen.getByText('S')).toBeDefined(); + expect(screen.getByText('Shivaji Byrapaneni')).toBeDefined(); + }); + }); + + it('should show dropdown with email when clicked', async () => { + render(); + + await waitFor(() => { + expect(screen.getByText('Shivaji Byrapaneni')).toBeDefined(); + }); + + fireEvent.click(screen.getByLabelText('User menu')); + + expect(screen.getByText('shivaji.byrapaneni@fmr.com')).toBeDefined(); + expect(screen.getByText('Sign out')).toBeDefined(); + }); + + it('should render nothing when no user', async () => { + const { getUser } = await import('../../authService.js'); + vi.mocked(getUser).mockResolvedValue(null); + + const { container } = render(); + + await waitFor(() => { + expect(container.firstChild).toBeNull(); + }); + }); +}); diff --git a/calm-hub-ui/src/components/user-menu/UserMenu.tsx b/calm-hub-ui/src/components/user-menu/UserMenu.tsx new file mode 100644 index 000000000..1da082a41 --- /dev/null +++ b/calm-hub-ui/src/components/user-menu/UserMenu.tsx @@ -0,0 +1,64 @@ +import React, { useEffect, useState } from 'react'; +import { authService, getUser } from '../../authService.js'; +import { User } from 'oidc-client-ts'; + +export const UserMenu: React.FC = () => { + const [user, setUser] = useState(null); + const [isOpen, setIsOpen] = useState(false); + + useEffect(() => { + const loadUser = async () => { + const currentUser = await getUser(); + setUser(currentUser); + }; + loadUser(); + }, []); + + const handleLogout = async () => { + await authService.logout(); + }; + + const displayName = user?.profile?.name || user?.profile?.preferred_username || 'User'; + const email = user?.profile?.email || ''; + + if (!user) { + return null; + } + + return ( +
+ + + {isOpen && ( + <> +
setIsOpen(false)} /> +
+
+

{displayName}

+ {email &&

{email}

} +
+
+ +
+
+ + )} +
+ ); +}; diff --git a/calm-hub-ui/src/hub/components/intro-screen/IntroScreen.tsx b/calm-hub-ui/src/hub/components/intro-screen/IntroScreen.tsx index 90c00597a..7b819a4b6 100644 --- a/calm-hub-ui/src/hub/components/intro-screen/IntroScreen.tsx +++ b/calm-hub-ui/src/hub/components/intro-screen/IntroScreen.tsx @@ -1,6 +1,8 @@ import { NamespaceCounts, DomainControlCount } from '../../../model/counts.js'; import { useTheme } from '../../../theme/useTheme.js'; import { ThemeToggle } from '../../../components/navbar/ThemeToggle.js'; +import { UserMenu } from '../../../components/user-menu/UserMenu.js'; +import { isAuthServiceEnabled } from '../../../authService.js'; import { IntroSearchBar } from './IntroSearchBar.js'; import { IntroBrowse } from './IntroBrowse.js'; @@ -27,8 +29,9 @@ export function IntroScreen({ namespaceCounts, domainCounts, storage }: IntroScr return (
-
+
+ {isAuthServiceEnabled() && }
diff --git a/calm-hub-ui/src/index.tsx b/calm-hub-ui/src/index.tsx index 51ca27c24..54d119055 100644 --- a/calm-hub-ui/src/index.tsx +++ b/calm-hub-ui/src/index.tsx @@ -2,27 +2,31 @@ import './index.css'; import React from 'react'; import ReactDOM from 'react-dom/client'; import ProtectedRoute from './ProtectedRoute.js'; -import { isAuthServiceEnabled } from './authService.js'; +import { initAuthService, isAuthServiceEnabled } from './authService.js'; import App from './App.js'; -import { LogoutButton } from './components/logout-button/LogoutButton.js'; import { AuthErrorModal } from './AuthModalError.js'; import { MigrationErrorModal } from './MigrationModalError.js'; const root = ReactDOM.createRoot(document.getElementById('root') as HTMLElement); -const isAuthenticationEnabled = isAuthServiceEnabled(); +async function bootstrap() { + await initAuthService(); -root.render( - - {isAuthenticationEnabled ? ( - + const isAuthenticationEnabled = isAuthServiceEnabled(); + + root.render( + + {isAuthenticationEnabled ? ( + + + + ) : ( - - - ) : ( - - )} - - - -); + )} + + + + ); +} + +bootstrap(); diff --git a/calm-hub/src/integration-test/java/integration/ProxyAuthIntegration.java b/calm-hub/src/integration-test/java/integration/ProxyAuthIntegration.java index 6ef1d6e1b..63a8fa8bc 100644 --- a/calm-hub/src/integration-test/java/integration/ProxyAuthIntegration.java +++ b/calm-hub/src/integration-test/java/integration/ProxyAuthIntegration.java @@ -303,11 +303,11 @@ void user_with_domain_read_grant_is_forbidden_from_creating_a_control() { @Test @Order(15) - void user_with_namespace_grant_only_is_forbidden_from_reading_domain_controls() { + void user_with_namespace_grant_can_read_domain_controls() { given() .header(PROXY_HEADER, USER_ALICE) .when().get("/calm/domains/security/controls") .then() - .statusCode(403); + .statusCode(200); } } diff --git a/calm-hub/src/main/java/org/finos/calm/resources/AuthConfigResource.java b/calm-hub/src/main/java/org/finos/calm/resources/AuthConfigResource.java new file mode 100644 index 000000000..a7b9cb743 --- /dev/null +++ b/calm-hub/src/main/java/org/finos/calm/resources/AuthConfigResource.java @@ -0,0 +1,68 @@ +package org.finos.calm.resources; + +import jakarta.inject.Inject; +import jakarta.ws.rs.GET; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.Produces; +import jakarta.ws.rs.core.MediaType; +import jakarta.ws.rs.core.Response; +import org.eclipse.microprofile.config.inject.ConfigProperty; + +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; + +/** + * Serves OIDC configuration to the frontend at startup. + * This endpoint is unauthenticated — it must be reachable before the user logs in + * so the frontend knows which OIDC library to load and where to redirect. + */ +@Path("/api/calm/auth") +@Produces(MediaType.APPLICATION_JSON) +public class AuthConfigResource { + + @Inject + @ConfigProperty(name = "quarkus.oidc.tenant-enabled", defaultValue = "false") + boolean oidcTenantEnabled; + + @Inject + @ConfigProperty(name = "calm.oidc.provider", defaultValue = "generic-oidc") + Optional oidcProvider; + + @Inject + @ConfigProperty(name = "quarkus.oidc.auth-server-url", defaultValue = "") + Optional oidcAuthority; + + @Inject + @ConfigProperty(name = "quarkus.oidc.client-id", defaultValue = "") + Optional oidcClientId; + + @Inject + @ConfigProperty(name = "calm.oidc.scopes", defaultValue = "openid profile email") + Optional oidcScopes; + + @Inject + @ConfigProperty(name = "calm.database.mode", defaultValue = "mongo") + String databaseMode; + + @GET + @Path("/config") + public Response getAuthConfig() { + Map response = new HashMap<>(); + + Map oidc = new HashMap<>(); + oidc.put("enabled", oidcTenantEnabled); + if (oidcTenantEnabled) { + oidc.put("provider", oidcProvider.orElse("generic-oidc")); + oidc.put("authority", oidcAuthority.orElse("")); + oidc.put("clientId", oidcClientId.orElse("")); + oidc.put("scopes", oidcScopes.orElse("openid profile email").split(" ")); + oidc.put("redirectUri", "/"); + } + response.put("oidc", oidc); + + response.put("databaseMode", databaseMode); + + return Response.ok(response).build(); + } +} diff --git a/calm-hub/src/main/java/org/finos/calm/resources/NamespaceResource.java b/calm-hub/src/main/java/org/finos/calm/resources/NamespaceResource.java index 646ea1d9f..6c3360516 100644 --- a/calm-hub/src/main/java/org/finos/calm/resources/NamespaceResource.java +++ b/calm-hub/src/main/java/org/finos/calm/resources/NamespaceResource.java @@ -25,17 +25,20 @@ import org.finos.calm.domain.exception.NamespaceParentNotFoundException; import org.finos.calm.domain.namespaces.NamespaceCounts; import org.finos.calm.domain.namespaces.NamespaceInfo; +import org.finos.calm.domain.UserAccess; import org.finos.calm.security.AuditRequestFilter; import org.finos.calm.security.CalmHubPermissionChecker; import org.finos.calm.security.CalmHubScopes; import org.finos.calm.security.UserAccessValidator; import org.finos.calm.services.CountsService; import org.finos.calm.services.NamespaceService; +import org.finos.calm.store.UserAccessStore; import java.net.URI; import java.net.URISyntaxException; import java.util.Optional; import java.util.Set; +import java.util.stream.Collectors; import static org.finos.calm.resources.CalmResourceErrorResponses.invalidNamespaceResponse; import static org.finos.calm.resources.CalmResourceErrorResponses.namespaceNotEmptyResponse; @@ -56,6 +59,9 @@ public class NamespaceResource { @Inject CalmHubPermissionChecker permissionChecker; + @Inject + UserAccessStore userAccessStore; + @Inject @ConfigProperty(name = "calm.auth.enabled", defaultValue = "false") boolean authEnabled; @@ -76,7 +82,13 @@ public NamespaceResource(NamespaceService namespaceService, ) @Authenticated public ValueWrapper namespaces() { - return new ValueWrapper<>(namespaceService.getNamespaces()); + Optional> readable = resolveReadableNamespaces(); + if (readable.isEmpty()) { + return new ValueWrapper<>(namespaceService.getNamespaces()); + } + return new ValueWrapper<>(namespaceService.getNamespaces().stream() + .filter(ns -> readable.get().contains(ns.getName())) + .toList()); } @GET @@ -97,8 +109,23 @@ public ValueWrapper namespaceCounts() { } private Optional> resolveReadableNamespaces() { - return ReadableScope.resolve(authEnabled, userAccessValidatorInstance, identity, - UserAccessValidator::getReadableNamespaces); + if (!authEnabled) { + return Optional.empty(); + } + if (userAccessValidatorInstance.isResolvable()) { + return ReadableScope.resolve(authEnabled, userAccessValidatorInstance, identity, + UserAccessValidator::getReadableNamespaces); + } + // Fallback: derive readable set from UserAccessStore grants (GitHub/OIDC mode) + if (identity.isAnonymous() || identity.getPrincipal() == null || userAccessStore == null) { + return Optional.empty(); + } + String username = identity.getPrincipal().getName(); + Set readable = userAccessStore.getGrantsForUser(username).stream() + .map(UserAccess::getNamespace) + .filter(ns -> ns != null) + .collect(Collectors.toSet()); + return Optional.of(readable); } @POST diff --git a/calm-hub/src/main/java/org/finos/calm/resources/PluginAuthResource.java b/calm-hub/src/main/java/org/finos/calm/resources/PluginAuthResource.java new file mode 100644 index 000000000..d79ec7762 --- /dev/null +++ b/calm-hub/src/main/java/org/finos/calm/resources/PluginAuthResource.java @@ -0,0 +1,231 @@ +package org.finos.calm.resources; + +import jakarta.inject.Inject; +import jakarta.ws.rs.GET; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.Produces; +import jakarta.ws.rs.QueryParam; +import jakarta.ws.rs.core.MediaType; +import jakarta.ws.rs.core.Response; +import org.eclipse.microprofile.config.inject.ConfigProperty; +import org.finos.calm.security.OidcPluginAuthClient; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.net.URI; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.util.Map; +import java.util.Optional; +import java.util.UUID; +import java.util.concurrent.ConcurrentHashMap; + +/** + * Handles OIDC authentication on behalf of the VS Code plugin. + * The plugin opens the /plugin-login URL in a browser; the Hub performs the + * full OIDC authorization code flow and redirects back to the plugin's + * localhost with the access token. + * + * Both endpoints are public (no auth required) — the user has not yet + * authenticated when they hit plugin-login, and plugin-callback receives + * the IdP redirect. + */ +@Path("/api/calm/auth") +@Produces(MediaType.APPLICATION_JSON) +public class PluginAuthResource { + + private static final Logger LOG = LoggerFactory.getLogger(PluginAuthResource.class); + private static final String DEFAULT_REDIRECT_PATH = "/callback"; + + private final Map pendingSessions = new ConcurrentHashMap<>(); + + @Inject + OidcPluginAuthClient oidcClient; + + @Inject + @ConfigProperty(name = "quarkus.oidc.auth-server-url", defaultValue = "") + Optional oidcAuthority; + + @Inject + @ConfigProperty(name = "quarkus.oidc.client-id", defaultValue = "") + Optional oidcClientId; + + @Inject + @ConfigProperty(name = "calm.oidc.scopes", defaultValue = "openid profile email") + String oidcScopes; + + @Inject + @ConfigProperty(name = "calm.hub.base-url", defaultValue = "http://localhost:8080") + String hubBaseUrl; + + record PendingSession(String port, String redirectPath, String codeVerifier, String nonce) {} + + @GET + @Path("plugin-login") + public Response pluginLogin(@QueryParam("port") String port, + @QueryParam("redirect_path") String redirectPath, + @QueryParam("nonce") String nonce) { + if (port == null || port.isBlank()) { + return Response.status(Response.Status.BAD_REQUEST) + .entity(Map.of("error", "port parameter is required")) + .build(); + } + + if (!isValidPort(port)) { + return Response.status(Response.Status.BAD_REQUEST) + .entity(Map.of("error", "port must be a valid number between 1 and 65535")) + .build(); + } + + if (oidcAuthority.isEmpty() || oidcAuthority.get().isBlank()) { + return Response.status(Response.Status.SERVICE_UNAVAILABLE) + .entity(Map.of("error", "OIDC is not configured")) + .build(); + } + + if (oidcClientId.isEmpty() || oidcClientId.get().isBlank()) { + return Response.status(Response.Status.SERVICE_UNAVAILABLE) + .entity(Map.of("error", "OIDC client-id is not configured")) + .build(); + } + + OidcPluginAuthClient.OidcEndpoints endpoints = oidcClient.discoverEndpoints(oidcAuthority.get()); + if (endpoints == null) { + return Response.status(Response.Status.BAD_GATEWAY) + .entity(Map.of("error", "Failed to discover OIDC endpoints")) + .build(); + } + + byte[] verifierBytes = new byte[32]; + new java.security.SecureRandom().nextBytes(verifierBytes); + String codeVerifier = java.util.Base64.getUrlEncoder().withoutPadding().encodeToString(verifierBytes); + String codeChallenge; + try { + byte[] digest = java.security.MessageDigest.getInstance("SHA-256").digest(codeVerifier.getBytes(StandardCharsets.UTF_8)); + codeChallenge = java.util.Base64.getUrlEncoder().withoutPadding().encodeToString(digest); + } catch (java.security.NoSuchAlgorithmException e) { + return Response.status(Response.Status.INTERNAL_SERVER_ERROR) + .entity(Map.of("error", "SHA-256 not available")).build(); + } + + String state = UUID.randomUUID().toString(); + String effectiveRedirectPath = (redirectPath != null && !redirectPath.isBlank()) + ? redirectPath : DEFAULT_REDIRECT_PATH; + pendingSessions.put(state, new PendingSession(port, effectiveRedirectPath, codeVerifier, nonce)); + + String hubCallbackUrl = hubBaseUrl + "/api/calm/auth/plugin-callback"; + + String authorizeUrl = endpoints.authorizationEndpoint() + + "?client_id=" + encode(oidcClientId.get()) + + "&response_type=code" + + "&scope=" + encode(oidcScopes) + + "&redirect_uri=" + encode(hubCallbackUrl) + + "&state=" + encode(state) + + "&code_challenge=" + encode(codeChallenge) + + "&code_challenge_method=S256"; + + LOG.debug("Redirecting plugin auth to OIDC authorize endpoint for port {}", port); + + return Response.temporaryRedirect(URI.create(authorizeUrl)).build(); + } + + @GET + @Path("plugin-callback") + public Response pluginCallback(@QueryParam("code") String code, + @QueryParam("state") String state, + @QueryParam("error") String error) { + if (error != null && !error.isBlank()) { + LOG.warn("OIDC IdP returned error: {}", error); + return Response.status(Response.Status.BAD_GATEWAY) + .entity(Map.of("error", "Authentication failed at identity provider")) + .build(); + } + + if (code == null || code.isBlank()) { + return Response.status(Response.Status.BAD_REQUEST) + .entity(Map.of("error", "Missing authorization code")) + .build(); + } + + if (state == null || state.isBlank()) { + return Response.status(Response.Status.BAD_REQUEST) + .entity(Map.of("error", "Missing state parameter")) + .build(); + } + + PendingSession session = pendingSessions.remove(state); + if (session == null) { + LOG.warn("Invalid or expired state parameter in plugin callback"); + return Response.status(Response.Status.FORBIDDEN) + .entity(Map.of("error", "Invalid or expired state")) + .build(); + } + + if (oidcAuthority.isEmpty() || oidcAuthority.get().isBlank()) { + return Response.status(Response.Status.SERVICE_UNAVAILABLE) + .entity(Map.of("error", "OIDC is not configured")) + .build(); + } + + if (oidcClientId.isEmpty() || oidcClientId.get().isBlank()) { + return Response.status(Response.Status.SERVICE_UNAVAILABLE) + .entity(Map.of("error", "OIDC client-id is not configured")) + .build(); + } + + OidcPluginAuthClient.OidcEndpoints endpoints = oidcClient.discoverEndpoints(oidcAuthority.get()); + if (endpoints == null) { + return Response.status(Response.Status.BAD_GATEWAY) + .entity(Map.of("error", "Failed to discover OIDC endpoints")) + .build(); + } + + String hubCallbackUrl = hubBaseUrl + "/api/calm/auth/plugin-callback"; + String pluginRedirect = "http://localhost:" + session.port() + session.redirectPath(); + String nonceParam = session.nonce() != null ? "&nonce=" + encode(session.nonce()) : ""; + + String html = "

Signing in...

"; + + return Response.ok(html).type("text/html").build(); + } + + // Visible for testing + Map getPendingSessions() { + return pendingSessions; + } + + private boolean isValidPort(String port) { + try { + int portNum = Integer.parseInt(port); + return portNum >= 1 && portNum <= 65535; + } catch (NumberFormatException e) { + return false; + } + } + + private String encode(String value) { + return URLEncoder.encode(value, StandardCharsets.UTF_8); + } +} diff --git a/calm-hub/src/main/java/org/finos/calm/security/CalmHubPermissionChecker.java b/calm-hub/src/main/java/org/finos/calm/security/CalmHubPermissionChecker.java index 954a1005f..a4d805c5a 100644 --- a/calm-hub/src/main/java/org/finos/calm/security/CalmHubPermissionChecker.java +++ b/calm-hub/src/main/java/org/finos/calm/security/CalmHubPermissionChecker.java @@ -1,8 +1,10 @@ package org.finos.calm.security; +import io.quarkus.runtime.StartupEvent; import io.quarkus.security.PermissionChecker; import io.quarkus.security.identity.SecurityIdentity; import jakarta.enterprise.context.ApplicationScoped; +import jakarta.enterprise.event.Observes; import jakarta.inject.Inject; import org.eclipse.microprofile.config.inject.ConfigProperty; import org.finos.calm.domain.UserAccess; @@ -32,6 +34,9 @@ public class CalmHubPermissionChecker { public CalmHubPermissionChecker(UserAccessStore userAccessStore) { this.userAccessStore = userAccessStore; + } + + void onStartup(@Observes StartupEvent event) { if (!authEnabled) { logger.warn("Caution: CalmHub is starting with authentication disabled. All user access will be granted by default."); } @@ -70,7 +75,15 @@ public boolean canRead(SecurityIdentity identity, String namespace) { public boolean canReadByDomain(SecurityIdentity identity, String domain) { return isAuthDisabled() || isAllowPublicRead() - || hasDomainAccess(identity, domain, UserAction.READ); + || hasDomainAccess(identity, domain, UserAction.READ) + || hasAnyNamespaceAccess(identity, UserAction.READ); + } + + private boolean hasAnyNamespaceAccess(SecurityIdentity identity, UserAction action) { + String username = identity.getPrincipal().getName(); + List grants = userAccessStore.getGrantsForUser(username); + return grants.stream().anyMatch(g -> + g.getNamespace() != null && permissionSufficient(g, action)); } @PermissionChecker(CalmHubScopes.WRITE) diff --git a/calm-hub/src/main/java/org/finos/calm/security/OidcPluginAuthClient.java b/calm-hub/src/main/java/org/finos/calm/security/OidcPluginAuthClient.java new file mode 100644 index 000000000..2c2c97e21 --- /dev/null +++ b/calm-hub/src/main/java/org/finos/calm/security/OidcPluginAuthClient.java @@ -0,0 +1,155 @@ +package org.finos.calm.security; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.inject.Inject; +import org.eclipse.microprofile.config.inject.ConfigProperty; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.net.ProxySelector; +import java.net.URI; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; +import java.time.Duration; +import java.util.Optional; + +/** + * Encapsulates OIDC HTTP operations for the plugin authentication flow. + * Handles discovery document fetching, authorize URL construction, and + * authorization code exchange. Injectable seam for unit testing + * PluginAuthResource without real HTTP calls. + */ +@ApplicationScoped +public class OidcPluginAuthClient { + + private static final Logger LOG = LoggerFactory.getLogger(OidcPluginAuthClient.class); + private static final ObjectMapper MAPPER = new ObjectMapper(); + + @Inject + @ConfigProperty(name = "calm.github.http.connect-timeout", defaultValue = "10") + int connectTimeoutSeconds; + + @Inject + @ConfigProperty(name = "calm.github.http.request-timeout", defaultValue = "30") + int requestTimeoutSeconds; + + public record OidcEndpoints(String authorizationEndpoint, String tokenEndpoint) {} + + public record TokenResponse(String accessToken, String idToken, String error) {} + + /** + * Fetches the OIDC discovery document and extracts authorization_endpoint and token_endpoint. + * + * @param issuerUrl the OIDC issuer URL (auth-server-url) + * @return discovered endpoints, or null if discovery fails + */ + public OidcEndpoints discoverEndpoints(String issuerUrl) { + try { + String discoveryUrl = issuerUrl.endsWith("/") + ? issuerUrl + ".well-known/openid-configuration" + : issuerUrl + "/.well-known/openid-configuration"; + + HttpClient client = HttpClient.newBuilder() + .proxy(ProxySelector.getDefault()) + .connectTimeout(Duration.ofSeconds(connectTimeoutSeconds)) + .build(); + + HttpRequest request = HttpRequest.newBuilder() + .uri(URI.create(discoveryUrl)) + .timeout(Duration.ofSeconds(requestTimeoutSeconds)) + .header("Accept", "application/json") + .GET() + .build(); + + HttpResponse response = client.send(request, HttpResponse.BodyHandlers.ofString()); + + if (response.statusCode() != 200) { + LOG.error("OIDC discovery failed with status {}: {}", response.statusCode(), response.body()); + return null; + } + + JsonNode doc = MAPPER.readTree(response.body()); + String authEndpoint = extractTextField(doc, "authorization_endpoint"); + String tokenEndpoint = extractTextField(doc, "token_endpoint"); + + if (authEndpoint == null || tokenEndpoint == null) { + LOG.error("OIDC discovery document missing required endpoints"); + return null; + } + + return new OidcEndpoints(authEndpoint, tokenEndpoint); + } catch (Exception e) { + LOG.error("OIDC discovery exception for issuer {}: {}", issuerUrl, e.getMessage()); + return null; + } + } + + /** + * Exchanges an authorization code for tokens at the OIDC token endpoint. + * + * @param tokenEndpoint the token endpoint URL + * @param clientId the OIDC client ID + * @param clientSecret the OIDC client secret + * @param code the authorization code + * @param redirectUri the redirect URI used in the authorize request + * @param codeVerifier the PKCE code verifier (nullable for non-PKCE flows) + * @return token response containing the access token or an error + */ + public TokenResponse exchangeCode(String tokenEndpoint, String clientId, String clientSecret, + String code, String redirectUri, String codeVerifier) { + try { + String body = "grant_type=authorization_code" + + "&client_id=" + clientId + + "&code=" + code + + "&redirect_uri=" + redirectUri; + if (clientSecret != null && !clientSecret.isBlank()) { + body += "&client_secret=" + clientSecret; + } + if (codeVerifier != null && !codeVerifier.isBlank()) { + body += "&code_verifier=" + codeVerifier; + } + + HttpClient client = HttpClient.newBuilder() + .proxy(ProxySelector.getDefault()) + .connectTimeout(Duration.ofSeconds(connectTimeoutSeconds)) + .build(); + + HttpRequest request = HttpRequest.newBuilder() + .uri(URI.create(tokenEndpoint)) + .timeout(Duration.ofSeconds(requestTimeoutSeconds)) + .header("Accept", "application/json") + .header("Content-Type", "application/x-www-form-urlencoded") + .POST(HttpRequest.BodyPublishers.ofString(body)) + .build(); + + HttpResponse response = client.send(request, HttpResponse.BodyHandlers.ofString()); + + if (response.statusCode() != 200) { + LOG.error("OIDC token exchange failed with status {}: {}", response.statusCode(), response.body()); + return new TokenResponse(null, null, "Token exchange failed with status " + response.statusCode()); + } + + JsonNode json = MAPPER.readTree(response.body()); + String accessToken = extractTextField(json, "access_token"); + String idToken = extractTextField(json, "id_token"); + + if (accessToken == null) { + LOG.error("No access_token in OIDC token response"); + return new TokenResponse(null, null, "No access token in response"); + } + + return new TokenResponse(accessToken, idToken, null); + } catch (Exception e) { + LOG.error("OIDC token exchange exception", e); + return new TokenResponse(null, null, "Token exchange failed: " + e.getMessage()); + } + } + + private String extractTextField(JsonNode node, String field) { + JsonNode value = node.get(field); + return value != null && value.isTextual() ? value.asText() : null; + } +} diff --git a/calm-hub/src/main/java/org/finos/calm/security/OidcRoleResolver.java b/calm-hub/src/main/java/org/finos/calm/security/OidcRoleResolver.java new file mode 100644 index 000000000..294691625 --- /dev/null +++ b/calm-hub/src/main/java/org/finos/calm/security/OidcRoleResolver.java @@ -0,0 +1,76 @@ +package org.finos.calm.security; + +import io.quarkus.security.identity.SecurityIdentity; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.inject.Inject; +import org.eclipse.microprofile.config.inject.ConfigProperty; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +/** + * Resolves user access by checking SecurityIdentity roles (populated from the + * token's "groups" claim via {@code quarkus.oidc.roles.role-claim-path=groups}) + * against the required access groups for a namespace. + */ +@ApplicationScoped +public class OidcRoleResolver { + + private static final Logger LOG = LoggerFactory.getLogger(OidcRoleResolver.class); + + @Inject + @ConfigProperty(name = "calm.oidc.roles.access", defaultValue = "") + Optional globalAccessGroups; + + public enum AccessLevel { READ, NONE } + + /** + * Checks if the user belongs to any of the specified access groups. + * Groups come from the token's "groups" claim mapped to SecurityIdentity roles. + */ + public AccessLevel resolve(SecurityIdentity identity, Set accessGroups) { + if (identity == null || identity.isAnonymous()) { + return AccessLevel.NONE; + } + + Set requiredGroups = accessGroups.isEmpty() ? parseConfig(globalAccessGroups) : accessGroups; + + if (requiredGroups.isEmpty()) { + LOG.warn("No access groups configured. No access will be granted."); + return AccessLevel.NONE; + } + + Set userGroups = identity.getRoles(); + + if (userGroups == null || userGroups.isEmpty()) { + LOG.debug("User [{}] has no groups in token. Ensure the IdP emits a 'groups' claim.", + identity.getPrincipal().getName()); + return AccessLevel.NONE; + } + + for (String group : userGroups) { + if (requiredGroups.contains(group)) { + LOG.debug("User [{}] matched access group [{}]", identity.getPrincipal().getName(), group); + return AccessLevel.READ; + } + } + + LOG.debug("User [{}] has no matching access groups. User groups: {}, required: {}", + identity.getPrincipal().getName(), userGroups, requiredGroups); + return AccessLevel.NONE; + } + + private Set parseConfig(Optional config) { + if (config.isEmpty() || config.get().isBlank()) { + return Set.of(); + } + return Stream.of(config.get().split(";")) + .map(String::trim) + .filter(s -> !s.isEmpty()) + .collect(Collectors.toSet()); + } +} diff --git a/calm-hub/src/main/resources/application-oidc.properties b/calm-hub/src/main/resources/application-oidc.properties new file mode 100644 index 000000000..df19212d3 --- /dev/null +++ b/calm-hub/src/main/resources/application-oidc.properties @@ -0,0 +1,54 @@ +# OIDC profile — works with ANY compliant IdP (Entra ID, Keycloak, Okta, generic). +# Activate with: -Dquarkus.profile=oidc + +# Auth enabled, OIDC token validation active +calm.auth.enabled=true +quarkus.oidc.tenant-enabled=true + +# OIDC discovery — set these via environment variables +quarkus.oidc.auth-server-url=${CALM_OIDC_ISSUER_URL} +quarkus.oidc.client-id=${CALM_OIDC_CLIENT_ID} +quarkus.oidc.token.principal-claim=${CALM_OIDC_USERNAME_CLAIM:preferred_username} +# Validate token audience and issuer against the configured client/IdP +quarkus.oidc.token.audience=${CALM_OIDC_CLIENT_ID} +quarkus.oidc.token.issuer=${CALM_OIDC_ISSUER_URL} + +# Frontend routing — tells the UI which OIDC library to load +calm.oidc.provider=${CALM_OIDC_PROVIDER:generic-oidc} +calm.oidc.scopes=${CALM_OIDC_SCOPES:openid profile email} + +# GitHub storage backend (only relevant when calm.database.mode=github) +calm.github.service-token=${CALM_GITHUB_SERVICE_TOKEN:} +calm.github.clone-directory=${CALM_GITHUB_CLONE_DIR:/tmp/calm-hub-clones} +calm.github.sync-interval=${CALM_GITHUB_SYNC_INTERVAL:900} +calm.github.api-url=${CALM_GITHUB_API_URL:https://api.github.com} + + +# Map the "groups" claim from the ID token to SecurityIdentity roles +quarkus.oidc.roles.role-claim-path=groups + +# Global fallback: semicolon-separated groups that grant access (used when namespace config has no groups) +calm.oidc.roles.access=${CALM_OIDC_ROLES_ACCESS:} + + +# Base URL for constructing callback URIs (override in production) +calm.hub.base-url=${CALM_HUB_BASE_URL:http://localhost:8080} + +# Public endpoints (accessible before login or as full-page navigations) +quarkus.http.auth.permission.public.paths=/api/calm/auth/config,/api/calm/auth/plugin-login,/api/calm/auth/plugin-callback +quarkus.http.auth.permission.public.policy=permit + +# Safety net — require authentication on all other API paths +quarkus.http.auth.permission.api.paths=/api/calm/* +quarkus.http.auth.permission.api.policy=authenticated +quarkus.http.auth.permission.calm.paths=/calm/* +quarkus.http.auth.permission.calm.policy=authenticated +quarkus.http.auth.permission.mcp-tools.paths=/mcp/* +quarkus.http.auth.permission.mcp-tools.policy=authenticated + +# Dev-only CORS (SPA on Vite :5173 → Quarkus :8080) +%dev.quarkus.http.cors=true +%dev.quarkus.http.cors.origins=http://localhost:5173 +%dev.quarkus.http.cors.methods=GET,POST,PUT,DELETE,OPTIONS +%dev.quarkus.http.cors.headers=Authorization,Content-Type +%dev.quarkus.http.cors.access-control-allow-credentials=true diff --git a/calm-hub/src/test/java/org/finos/calm/resources/TestAuthConfigResourceShould.java b/calm-hub/src/test/java/org/finos/calm/resources/TestAuthConfigResourceShould.java new file mode 100644 index 000000000..24f9c0032 --- /dev/null +++ b/calm-hub/src/test/java/org/finos/calm/resources/TestAuthConfigResourceShould.java @@ -0,0 +1,118 @@ +package org.finos.calm.resources; + +import jakarta.ws.rs.core.Response; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.util.Map; +import java.util.Optional; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; + +class TestAuthConfigResourceShould { + + private AuthConfigResource resource; + + @BeforeEach + void setup() { + resource = new AuthConfigResource(); + } + + @Test + @SuppressWarnings("unchecked") + void return_disabled_config_when_oidc_off() { + resource.oidcTenantEnabled = false; + resource.databaseMode = "mongo"; + resource.oidcProvider = Optional.empty(); + resource.oidcAuthority = Optional.empty(); + resource.oidcClientId = Optional.empty(); + resource.oidcScopes = Optional.empty(); + + Response response = resource.getAuthConfig(); + + assertThat(response.getStatus(), equalTo(200)); + Map body = (Map) response.getEntity(); + Map oidc = (Map) body.get("oidc"); + assertThat(oidc.get("enabled"), equalTo(false)); + assertThat(body.get("databaseMode"), equalTo("mongo")); + } + + @Test + @SuppressWarnings("unchecked") + void return_full_config_for_entra_id_with_github_mode() { + resource.oidcTenantEnabled = true; + resource.databaseMode = "github"; + resource.oidcProvider = Optional.of("entra-id"); + resource.oidcAuthority = Optional.of("https://login.microsoftonline.com/tenant-id/v2.0"); + resource.oidcClientId = Optional.of("client-123"); + resource.oidcScopes = Optional.of("openid profile email"); + + Response response = resource.getAuthConfig(); + + assertThat(response.getStatus(), equalTo(200)); + Map body = (Map) response.getEntity(); + + Map oidc = (Map) body.get("oidc"); + assertThat(oidc.get("enabled"), equalTo(true)); + assertThat(oidc.get("provider"), equalTo("entra-id")); + assertThat(oidc.get("authority"), equalTo("https://login.microsoftonline.com/tenant-id/v2.0")); + assertThat(oidc.get("clientId"), equalTo("client-123")); + assertThat(oidc.get("redirectUri"), equalTo("/")); + + assertThat(body.get("databaseMode"), equalTo("github")); + } + + @Test + @SuppressWarnings("unchecked") + void return_oidc_config_for_mongo_mode() { + resource.oidcTenantEnabled = true; + resource.databaseMode = "mongo"; + resource.oidcProvider = Optional.of("keycloak"); + resource.oidcAuthority = Optional.of("https://keycloak.example.com/realms/calm"); + resource.oidcClientId = Optional.of("calm-hub-spa"); + resource.oidcScopes = Optional.of("openid profile email"); + + Response response = resource.getAuthConfig(); + + Map body = (Map) response.getEntity(); + Map oidc = (Map) body.get("oidc"); + assertThat(oidc.get("enabled"), equalTo(true)); + assertThat(oidc.get("provider"), equalTo("keycloak")); + assertThat(body.get("databaseMode"), equalTo("mongo")); + } + + @Test + @SuppressWarnings("unchecked") + void not_include_oidc_details_when_disabled() { + resource.oidcTenantEnabled = false; + resource.databaseMode = "standalone"; + resource.oidcProvider = Optional.of("entra-id"); + resource.oidcAuthority = Optional.of("https://login.microsoftonline.com/tenant/v2.0"); + resource.oidcClientId = Optional.of("client-123"); + resource.oidcScopes = Optional.of("openid profile email"); + + Response response = resource.getAuthConfig(); + + Map body = (Map) response.getEntity(); + Map oidc = (Map) body.get("oidc"); + assertThat(oidc.get("enabled"), equalTo(false)); + assertThat(oidc.containsKey("provider"), is(false)); + assertThat(oidc.containsKey("authority"), is(false)); + } + + @Test + void always_return_200() { + resource.oidcTenantEnabled = false; + resource.databaseMode = "mongo"; + resource.oidcProvider = Optional.empty(); + resource.oidcAuthority = Optional.empty(); + resource.oidcClientId = Optional.empty(); + resource.oidcScopes = Optional.empty(); + + Response response = resource.getAuthConfig(); + + assertThat(response.getStatus(), equalTo(200)); + } +} diff --git a/calm-hub/src/test/java/org/finos/calm/resources/TestNamespaceResourceFilteringShould.java b/calm-hub/src/test/java/org/finos/calm/resources/TestNamespaceResourceFilteringShould.java index a82d5bfa4..557c1c726 100644 --- a/calm-hub/src/test/java/org/finos/calm/resources/TestNamespaceResourceFilteringShould.java +++ b/calm-hub/src/test/java/org/finos/calm/resources/TestNamespaceResourceFilteringShould.java @@ -13,6 +13,10 @@ import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import org.finos.calm.domain.UserAccess; +import org.finos.calm.domain.namespaces.NamespaceInfo; +import org.finos.calm.store.UserAccessStore; + import java.security.Principal; import java.util.List; import java.util.Optional; @@ -22,6 +26,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -105,6 +110,51 @@ void pass_empty_optional_when_validator_not_resolvable() { assertFalse(captor.getValue().isPresent()); } + @Test + void filter_namespaces_via_store_fallback_when_validator_not_resolvable() { + when(mockValidatorInstance.isResolvable()).thenReturn(false); + when(mockIdentity.isAnonymous()).thenReturn(false); + when(mockIdentity.getPrincipal()).thenReturn(mockPrincipal); + when(mockPrincipal.getName()).thenReturn("testuser"); + + UserAccessStore mockUserAccessStore = mock(UserAccessStore.class); + UserAccess grant = new UserAccess("testuser", UserAccess.Permission.read, "finos"); + when(mockUserAccessStore.getGrantsForUser("testuser")).thenReturn(List.of(grant)); + + when(mockNamespaceService.getNamespaces()).thenReturn(List.of( + new NamespaceInfo("finos", "FINOS"), + new NamespaceInfo("private", "Private") + )); + + NamespaceResource resource = new NamespaceResource(mockNamespaceService, mockCountsService, mockValidatorInstance); + resource.identity = mockIdentity; + resource.authEnabled = true; + resource.userAccessStore = mockUserAccessStore; + + var result = resource.namespaces(); + assertEquals(1, result.getValues().size()); + assertEquals("finos", result.getValues().get(0).getName()); + } + + @Test + void return_all_namespaces_when_store_fallback_has_null_store() { + when(mockValidatorInstance.isResolvable()).thenReturn(false); + when(mockIdentity.isAnonymous()).thenReturn(false); + when(mockIdentity.getPrincipal()).thenReturn(mockPrincipal); + + when(mockNamespaceService.getNamespaces()).thenReturn(List.of( + new NamespaceInfo("finos", "FINOS") + )); + + NamespaceResource resource = new NamespaceResource(mockNamespaceService, mockCountsService, mockValidatorInstance); + resource.identity = mockIdentity; + resource.authEnabled = true; + resource.userAccessStore = null; + + var result = resource.namespaces(); + assertEquals(1, result.getValues().size()); + } + @Test void pass_empty_set_when_user_has_no_namespace_grants() { when(mockValidatorInstance.isResolvable()).thenReturn(true); diff --git a/calm-hub/src/test/java/org/finos/calm/resources/TestPluginAuthResourceShould.java b/calm-hub/src/test/java/org/finos/calm/resources/TestPluginAuthResourceShould.java new file mode 100644 index 000000000..f4bfe8bc5 --- /dev/null +++ b/calm-hub/src/test/java/org/finos/calm/resources/TestPluginAuthResourceShould.java @@ -0,0 +1,410 @@ +package org.finos.calm.resources; + +import jakarta.ws.rs.core.Response; +import org.finos.calm.security.OidcPluginAuthClient; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import java.lang.reflect.Field; +import java.net.URI; +import java.util.Map; +import java.util.Optional; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +class TestPluginAuthResourceShould { + + private static final String AUTH_SERVER_URL = "https://login.microsoftonline.com/tenant-id/v2.0"; + private static final String CLIENT_ID = "calm-hub-client"; + private static final String CLIENT_SECRET = "super-secret"; + private static final String HUB_BASE_URL = "http://localhost:8080"; + private static final String SCOPES = "openid profile email"; + private static final String AUTHORIZATION_ENDPOINT = "https://login.microsoftonline.com/tenant-id/v2.0/authorize"; + private static final String TOKEN_ENDPOINT = "https://login.microsoftonline.com/tenant-id/v2.0/token"; + + @Mock + private OidcPluginAuthClient mockOidcClient; + + private PluginAuthResource resource; + + @BeforeEach + void setup() throws Exception { + resource = new PluginAuthResource(); + setField("oidcClient", mockOidcClient); + setField("oidcAuthority", Optional.of(AUTH_SERVER_URL)); + setField("oidcClientId", Optional.of(CLIENT_ID)); + + setField("oidcScopes", SCOPES); + setField("hubBaseUrl", HUB_BASE_URL); + } + + private void setField(String name, Object value) throws Exception { + Field field = PluginAuthResource.class.getDeclaredField(name); + field.setAccessible(true); + field.set(resource, value); + } + + // --- plugin-login tests --- + + @Test + @SuppressWarnings("unchecked") + void return_400_when_port_is_null() { + Response response = resource.pluginLogin(null, null, null); + + assertThat(response.getStatus(), equalTo(400)); + Map body = (Map) response.getEntity(); + assertThat(body.get("error"), containsString("port parameter is required")); + } + + @Test + @SuppressWarnings("unchecked") + void return_400_when_port_is_blank() { + Response response = resource.pluginLogin(" ", null, null); + + assertThat(response.getStatus(), equalTo(400)); + Map body = (Map) response.getEntity(); + assertThat(body.get("error"), containsString("port parameter is required")); + } + + @Test + @SuppressWarnings("unchecked") + void return_400_when_port_is_not_a_number() { + Response response = resource.pluginLogin("abc", null, null); + + assertThat(response.getStatus(), equalTo(400)); + Map body = (Map) response.getEntity(); + assertThat(body.get("error"), containsString("valid number")); + } + + @Test + @SuppressWarnings("unchecked") + void return_400_when_port_is_zero() { + Response response = resource.pluginLogin("0", null, null); + + assertThat(response.getStatus(), equalTo(400)); + Map body = (Map) response.getEntity(); + assertThat(body.get("error"), containsString("valid number")); + } + + @Test + @SuppressWarnings("unchecked") + void return_400_when_port_exceeds_65535() { + Response response = resource.pluginLogin("70000", null, null); + + assertThat(response.getStatus(), equalTo(400)); + Map body = (Map) response.getEntity(); + assertThat(body.get("error"), containsString("valid number")); + } + + @Test + @SuppressWarnings("unchecked") + void return_503_when_oidc_authority_is_empty() throws Exception { + setField("oidcAuthority", Optional.of("")); + + Response response = resource.pluginLogin("63348", null, "test-nonce"); + + assertThat(response.getStatus(), equalTo(503)); + Map body = (Map) response.getEntity(); + assertThat(body.get("error"), containsString("OIDC is not configured")); + } + + @Test + @SuppressWarnings("unchecked") + void return_503_when_oidc_authority_is_absent() throws Exception { + setField("oidcAuthority", Optional.empty()); + + Response response = resource.pluginLogin("63348", null, "test-nonce"); + + assertThat(response.getStatus(), equalTo(503)); + Map body = (Map) response.getEntity(); + assertThat(body.get("error"), containsString("OIDC is not configured")); + } + + @Test + @SuppressWarnings("unchecked") + void return_503_when_oidc_client_id_is_empty() throws Exception { + setField("oidcClientId", Optional.of("")); + + Response response = resource.pluginLogin("63348", null, "test-nonce"); + + assertThat(response.getStatus(), equalTo(503)); + Map body = (Map) response.getEntity(); + assertThat(body.get("error"), containsString("OIDC client-id is not configured")); + } + + @Test + @SuppressWarnings("unchecked") + void return_503_when_oidc_client_id_is_absent() throws Exception { + setField("oidcClientId", Optional.empty()); + + Response response = resource.pluginLogin("63348", null, "test-nonce"); + + assertThat(response.getStatus(), equalTo(503)); + Map body = (Map) response.getEntity(); + assertThat(body.get("error"), containsString("OIDC client-id is not configured")); + } + + @Test + @SuppressWarnings("unchecked") + void return_502_when_oidc_discovery_fails() { + when(mockOidcClient.discoverEndpoints(AUTH_SERVER_URL)).thenReturn(null); + + Response response = resource.pluginLogin("63348", null, "test-nonce"); + + assertThat(response.getStatus(), equalTo(502)); + Map body = (Map) response.getEntity(); + assertThat(body.get("error"), containsString("Failed to discover OIDC endpoints")); + } + + @Test + void redirect_to_oidc_authorize_endpoint_on_success() { + OidcPluginAuthClient.OidcEndpoints endpoints = + new OidcPluginAuthClient.OidcEndpoints(AUTHORIZATION_ENDPOINT, TOKEN_ENDPOINT); + when(mockOidcClient.discoverEndpoints(AUTH_SERVER_URL)).thenReturn(endpoints); + + Response response = resource.pluginLogin("63348", null, "test-nonce"); + + assertThat(response.getStatus(), equalTo(307)); + URI location = (URI) response.getMetadata().getFirst("Location"); + assertThat(location, is(notNullValue())); + String locationStr = location.toString(); + assertThat(locationStr, containsString(AUTHORIZATION_ENDPOINT)); + assertThat(locationStr, containsString("client_id=" + CLIENT_ID)); + assertThat(locationStr, containsString("response_type=code")); + assertThat(locationStr, containsString("redirect_uri=")); + assertThat(locationStr, containsString("plugin-callback")); + assertThat(locationStr, containsString("state=")); + } + + @Test + void store_pending_session_on_login() { + OidcPluginAuthClient.OidcEndpoints endpoints = + new OidcPluginAuthClient.OidcEndpoints(AUTHORIZATION_ENDPOINT, TOKEN_ENDPOINT); + when(mockOidcClient.discoverEndpoints(AUTH_SERVER_URL)).thenReturn(endpoints); + + resource.pluginLogin("63348", null, "test-nonce"); + + assertThat(resource.getPendingSessions().isEmpty(), is(false)); + PluginAuthResource.PendingSession session = + resource.getPendingSessions().values().iterator().next(); + assertThat(session.port(), equalTo("63348")); + assertThat(session.redirectPath(), equalTo("/callback")); + } + + @Test + void use_custom_redirect_path_when_provided() { + OidcPluginAuthClient.OidcEndpoints endpoints = + new OidcPluginAuthClient.OidcEndpoints(AUTHORIZATION_ENDPOINT, TOKEN_ENDPOINT); + when(mockOidcClient.discoverEndpoints(AUTH_SERVER_URL)).thenReturn(endpoints); + + resource.pluginLogin("63348", "/auth/done", "test-nonce"); + + PluginAuthResource.PendingSession session = + resource.getPendingSessions().values().iterator().next(); + assertThat(session.redirectPath(), equalTo("/auth/done")); + } + + @Test + void use_default_redirect_path_when_redirect_path_is_blank() { + OidcPluginAuthClient.OidcEndpoints endpoints = + new OidcPluginAuthClient.OidcEndpoints(AUTHORIZATION_ENDPOINT, TOKEN_ENDPOINT); + when(mockOidcClient.discoverEndpoints(AUTH_SERVER_URL)).thenReturn(endpoints); + + resource.pluginLogin("63348", " ", "test-nonce"); + + PluginAuthResource.PendingSession session = + resource.getPendingSessions().values().iterator().next(); + assertThat(session.redirectPath(), equalTo("/callback")); + } + + @Test + void accept_valid_port_at_boundary_1() { + OidcPluginAuthClient.OidcEndpoints endpoints = + new OidcPluginAuthClient.OidcEndpoints(AUTHORIZATION_ENDPOINT, TOKEN_ENDPOINT); + when(mockOidcClient.discoverEndpoints(AUTH_SERVER_URL)).thenReturn(endpoints); + + Response response = resource.pluginLogin("1", null, "test-nonce"); + + assertThat(response.getStatus(), equalTo(307)); + } + + @Test + void accept_valid_port_at_boundary_65535() { + OidcPluginAuthClient.OidcEndpoints endpoints = + new OidcPluginAuthClient.OidcEndpoints(AUTHORIZATION_ENDPOINT, TOKEN_ENDPOINT); + when(mockOidcClient.discoverEndpoints(AUTH_SERVER_URL)).thenReturn(endpoints); + + Response response = resource.pluginLogin("65535", null, "test-nonce"); + + assertThat(response.getStatus(), equalTo(307)); + } + + // --- plugin-callback tests --- + + @Test + @SuppressWarnings("unchecked") + void return_502_when_idp_returns_error() { + Response response = resource.pluginCallback("code", "state", "access_denied"); + + assertThat(response.getStatus(), equalTo(502)); + Map body = (Map) response.getEntity(); + assertThat(body.get("error"), containsString("Authentication failed at identity provider")); + } + + @Test + @SuppressWarnings("unchecked") + void return_400_when_callback_code_is_null() { + Response response = resource.pluginCallback(null, "some-state", null); + + assertThat(response.getStatus(), equalTo(400)); + Map body = (Map) response.getEntity(); + assertThat(body.get("error"), containsString("Missing authorization code")); + } + + @Test + @SuppressWarnings("unchecked") + void return_400_when_callback_code_is_blank() { + Response response = resource.pluginCallback(" ", "some-state", null); + + assertThat(response.getStatus(), equalTo(400)); + Map body = (Map) response.getEntity(); + assertThat(body.get("error"), containsString("Missing authorization code")); + } + + @Test + @SuppressWarnings("unchecked") + void return_400_when_callback_state_is_null() { + Response response = resource.pluginCallback("valid-code", null, null); + + assertThat(response.getStatus(), equalTo(400)); + Map body = (Map) response.getEntity(); + assertThat(body.get("error"), containsString("Missing state parameter")); + } + + @Test + @SuppressWarnings("unchecked") + void return_400_when_callback_state_is_blank() { + Response response = resource.pluginCallback("valid-code", " ", null); + + assertThat(response.getStatus(), equalTo(400)); + Map body = (Map) response.getEntity(); + assertThat(body.get("error"), containsString("Missing state parameter")); + } + + @Test + @SuppressWarnings("unchecked") + void return_403_when_state_is_not_found_in_pending_sessions() { + Response response = resource.pluginCallback("valid-code", "unknown-state", null); + + assertThat(response.getStatus(), equalTo(403)); + Map body = (Map) response.getEntity(); + assertThat(body.get("error"), containsString("Invalid or expired state")); + } + + @Test + @SuppressWarnings("unchecked") + void return_503_when_oidc_authority_not_configured_on_callback() throws Exception { + // Plant a valid pending session + resource.getPendingSessions().put("valid-state", + new PluginAuthResource.PendingSession("63348", "/callback", "test-verifier", "test-nonce")); + setField("oidcAuthority", Optional.of("")); + + Response response = resource.pluginCallback("valid-code", "valid-state", null); + + assertThat(response.getStatus(), equalTo(503)); + Map body = (Map) response.getEntity(); + assertThat(body.get("error"), containsString("OIDC is not configured")); + } + + @Test + @SuppressWarnings("unchecked") + void return_503_when_client_id_not_configured_on_callback() throws Exception { + resource.getPendingSessions().put("valid-state", + new PluginAuthResource.PendingSession("63348", "/callback", "test-verifier", "test-nonce")); + setField("oidcClientId", Optional.of("")); + + Response response = resource.pluginCallback("valid-code", "valid-state", null); + + assertThat(response.getStatus(), equalTo(503)); + Map body = (Map) response.getEntity(); + assertThat(body.get("error"), containsString("OIDC client-id is not configured")); + } + + @Test + @SuppressWarnings("unchecked") + void return_502_when_discovery_fails_on_callback() { + resource.getPendingSessions().put("valid-state", + new PluginAuthResource.PendingSession("63348", "/callback", "test-verifier", "test-nonce")); + when(mockOidcClient.discoverEndpoints(AUTH_SERVER_URL)).thenReturn(null); + + Response response = resource.pluginCallback("valid-code", "valid-state", null); + + assertThat(response.getStatus(), equalTo(502)); + Map body = (Map) response.getEntity(); + assertThat(body.get("error"), containsString("Failed to discover OIDC endpoints")); + } + + @Test + void return_html_with_plugin_redirect_on_successful_callback() { + resource.getPendingSessions().put("valid-state", + new PluginAuthResource.PendingSession("63348", "/callback", "test-verifier", "test-nonce")); + OidcPluginAuthClient.OidcEndpoints endpoints = + new OidcPluginAuthClient.OidcEndpoints(AUTHORIZATION_ENDPOINT, TOKEN_ENDPOINT); + when(mockOidcClient.discoverEndpoints(AUTH_SERVER_URL)).thenReturn(endpoints); + + Response response = resource.pluginCallback("valid-code", "valid-state", null); + + assertThat(response.getStatus(), equalTo(200)); + String html = (String) response.getEntity(); + assertThat(html, containsString("http://localhost:63348/callback")); + assertThat(html, containsString("code_verifier")); + assertThat(html, containsString("test-verifier")); + } + + @Test + void return_html_with_custom_path_on_successful_callback() { + resource.getPendingSessions().put("valid-state", + new PluginAuthResource.PendingSession("9999", "/auth/done", "test-verifier", "test-nonce")); + OidcPluginAuthClient.OidcEndpoints endpoints = + new OidcPluginAuthClient.OidcEndpoints(AUTHORIZATION_ENDPOINT, TOKEN_ENDPOINT); + when(mockOidcClient.discoverEndpoints(AUTH_SERVER_URL)).thenReturn(endpoints); + + Response response = resource.pluginCallback("valid-code", "valid-state", null); + + assertThat(response.getStatus(), equalTo(200)); + String html = (String) response.getEntity(); + assertThat(html, containsString("http://localhost:9999/auth/done")); + assertThat(html, containsString("code_verifier")); + assertThat(html, containsString("test-verifier")); + } + + @Test + void remove_state_from_pending_sessions_after_callback() { + resource.getPendingSessions().put("valid-state", + new PluginAuthResource.PendingSession("63348", "/callback", "test-verifier", "test-nonce")); + OidcPluginAuthClient.OidcEndpoints endpoints = + new OidcPluginAuthClient.OidcEndpoints(AUTHORIZATION_ENDPOINT, TOKEN_ENDPOINT); + when(mockOidcClient.discoverEndpoints(AUTH_SERVER_URL)).thenReturn(endpoints); + + Response response = resource.pluginCallback("valid-code", "valid-state", null); + + assertThat(response.getStatus(), equalTo(200)); + assertThat(resource.getPendingSessions().containsKey("valid-state"), is(false)); + } + + @Test + void return_400_for_negative_port() { + Response response = resource.pluginLogin("-1", null, null); + + assertThat(response.getStatus(), equalTo(400)); + } +} diff --git a/calm-hub/src/test/java/org/finos/calm/security/TestCalmHubPermissionCheckerShould.java b/calm-hub/src/test/java/org/finos/calm/security/TestCalmHubPermissionCheckerShould.java index 59dcdb2c6..1da113d79 100644 --- a/calm-hub/src/test/java/org/finos/calm/security/TestCalmHubPermissionCheckerShould.java +++ b/calm-hub/src/test/java/org/finos/calm/security/TestCalmHubPermissionCheckerShould.java @@ -358,12 +358,12 @@ void grant_for_different_domain_denies_domain_read() { } @Test - void namespace_grant_does_not_satisfy_domain_read() { + void namespace_grant_satisfies_domain_read() { givenAuthenticatedUser("alice"); when(mockUserAccessStore.getGrantsForUser("alice")) .thenReturn(List.of(grant("alice", UserAccess.Permission.read, "payments"))); - assertFalse(checker.canReadByDomain(mockIdentity, "payments")); + assertTrue(checker.canReadByDomain(mockIdentity, "payments")); } @Test diff --git a/calm-hub/src/test/java/org/finos/calm/security/TestOidcPluginAuthClientShould.java b/calm-hub/src/test/java/org/finos/calm/security/TestOidcPluginAuthClientShould.java new file mode 100644 index 000000000..362cab962 --- /dev/null +++ b/calm-hub/src/test/java/org/finos/calm/security/TestOidcPluginAuthClientShould.java @@ -0,0 +1,268 @@ +package org.finos.calm.security; + +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.node.ObjectNode; +import com.sun.net.httpserver.HttpServer; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.lang.reflect.Field; +import java.net.InetSocketAddress; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.nullValue; + +class TestOidcPluginAuthClientShould { + + private static final ObjectMapper MAPPER = new ObjectMapper(); + + private OidcPluginAuthClient client; + private HttpServer server; + private int serverPort; + + @BeforeEach + void setup() throws Exception { + client = new OidcPluginAuthClient(); + setField("connectTimeoutSeconds", 5); + setField("requestTimeoutSeconds", 5); + + // Start a local HTTP server for testing + server = HttpServer.create(new InetSocketAddress(0), 0); + serverPort = server.getAddress().getPort(); + server.start(); + } + + @AfterEach + void teardown() { + if (server != null) { + server.stop(0); + } + } + + private void setField(String name, Object value) throws Exception { + Field field = OidcPluginAuthClient.class.getDeclaredField(name); + field.setAccessible(true); + field.set(client, value); + } + + // --- discoverEndpoints tests --- + + @Test + void return_endpoints_from_valid_discovery_document() { + ObjectNode discoveryDoc = MAPPER.createObjectNode(); + discoveryDoc.put("authorization_endpoint", "https://idp.example.com/authorize"); + discoveryDoc.put("token_endpoint", "https://idp.example.com/token"); + discoveryDoc.put("issuer", "https://idp.example.com"); + + server.createContext("/.well-known/openid-configuration", exchange -> { + byte[] body = MAPPER.writeValueAsBytes(discoveryDoc); + exchange.getResponseHeaders().add("Content-Type", "application/json"); + exchange.sendResponseHeaders(200, body.length); + exchange.getResponseBody().write(body); + exchange.close(); + }); + + OidcPluginAuthClient.OidcEndpoints result = + client.discoverEndpoints("http://localhost:" + serverPort); + + assertThat(result, is(notNullValue())); + assertThat(result.authorizationEndpoint(), equalTo("https://idp.example.com/authorize")); + assertThat(result.tokenEndpoint(), equalTo("https://idp.example.com/token")); + } + + @Test + void return_endpoints_when_issuer_url_has_trailing_slash() { + ObjectNode discoveryDoc = MAPPER.createObjectNode(); + discoveryDoc.put("authorization_endpoint", "https://idp.example.com/authorize"); + discoveryDoc.put("token_endpoint", "https://idp.example.com/token"); + + server.createContext("/.well-known/openid-configuration", exchange -> { + byte[] body = MAPPER.writeValueAsBytes(discoveryDoc); + exchange.getResponseHeaders().add("Content-Type", "application/json"); + exchange.sendResponseHeaders(200, body.length); + exchange.getResponseBody().write(body); + exchange.close(); + }); + + OidcPluginAuthClient.OidcEndpoints result = + client.discoverEndpoints("http://localhost:" + serverPort + "/"); + + assertThat(result, is(notNullValue())); + assertThat(result.authorizationEndpoint(), equalTo("https://idp.example.com/authorize")); + } + + @Test + void return_null_when_discovery_returns_non_200() { + server.createContext("/.well-known/openid-configuration", exchange -> { + exchange.sendResponseHeaders(500, -1); + exchange.close(); + }); + + OidcPluginAuthClient.OidcEndpoints result = + client.discoverEndpoints("http://localhost:" + serverPort); + + assertThat(result, is(nullValue())); + } + + @Test + void return_null_when_discovery_document_missing_authorization_endpoint() { + ObjectNode discoveryDoc = MAPPER.createObjectNode(); + discoveryDoc.put("token_endpoint", "https://idp.example.com/token"); + + server.createContext("/.well-known/openid-configuration", exchange -> { + byte[] body = MAPPER.writeValueAsBytes(discoveryDoc); + exchange.getResponseHeaders().add("Content-Type", "application/json"); + exchange.sendResponseHeaders(200, body.length); + exchange.getResponseBody().write(body); + exchange.close(); + }); + + OidcPluginAuthClient.OidcEndpoints result = + client.discoverEndpoints("http://localhost:" + serverPort); + + assertThat(result, is(nullValue())); + } + + @Test + void return_null_when_discovery_document_missing_token_endpoint() { + ObjectNode discoveryDoc = MAPPER.createObjectNode(); + discoveryDoc.put("authorization_endpoint", "https://idp.example.com/authorize"); + + server.createContext("/.well-known/openid-configuration", exchange -> { + byte[] body = MAPPER.writeValueAsBytes(discoveryDoc); + exchange.getResponseHeaders().add("Content-Type", "application/json"); + exchange.sendResponseHeaders(200, body.length); + exchange.getResponseBody().write(body); + exchange.close(); + }); + + OidcPluginAuthClient.OidcEndpoints result = + client.discoverEndpoints("http://localhost:" + serverPort); + + assertThat(result, is(nullValue())); + } + + @Test + void return_null_when_discovery_url_is_unreachable() { + OidcPluginAuthClient.OidcEndpoints result = + client.discoverEndpoints("http://localhost:1"); + + assertThat(result, is(nullValue())); + } + + @Test + void return_null_when_discovery_returns_invalid_json() { + server.createContext("/.well-known/openid-configuration", exchange -> { + byte[] body = "not-json".getBytes(); + exchange.getResponseHeaders().add("Content-Type", "application/json"); + exchange.sendResponseHeaders(200, body.length); + exchange.getResponseBody().write(body); + exchange.close(); + }); + + OidcPluginAuthClient.OidcEndpoints result = + client.discoverEndpoints("http://localhost:" + serverPort); + + assertThat(result, is(nullValue())); + } + + // --- exchangeCode tests --- + + @Test + void return_access_token_on_successful_exchange() { + ObjectNode tokenResponse = MAPPER.createObjectNode(); + tokenResponse.put("access_token", "eyJhbGciOi..."); + tokenResponse.put("id_token", "id-token-value"); + tokenResponse.put("token_type", "Bearer"); + + server.createContext("/token", exchange -> { + byte[] body = MAPPER.writeValueAsBytes(tokenResponse); + exchange.getResponseHeaders().add("Content-Type", "application/json"); + exchange.sendResponseHeaders(200, body.length); + exchange.getResponseBody().write(body); + exchange.close(); + }); + + String tokenEndpoint = "http://localhost:" + serverPort + "/token"; + OidcPluginAuthClient.TokenResponse result = + client.exchangeCode(tokenEndpoint, "client-id", "client-secret", "auth-code", "http://localhost:8080/callback", "test-verifier"); + + assertThat(result, is(notNullValue())); + assertThat(result.accessToken(), equalTo("eyJhbGciOi...")); + assertThat(result.idToken(), equalTo("id-token-value")); + assertThat(result.error(), is(nullValue())); + } + + @Test + void return_error_when_token_endpoint_returns_non_200() { + server.createContext("/token", exchange -> { + exchange.sendResponseHeaders(400, -1); + exchange.close(); + }); + + String tokenEndpoint = "http://localhost:" + serverPort + "/token"; + OidcPluginAuthClient.TokenResponse result = + client.exchangeCode(tokenEndpoint, "client-id", "client-secret", "bad-code", "http://localhost:8080/callback", "test-verifier"); + + assertThat(result.accessToken(), is(nullValue())); + assertThat(result.error(), containsString("Token exchange failed with status 400")); + } + + @Test + void return_error_when_response_missing_access_token() { + ObjectNode tokenResponse = MAPPER.createObjectNode(); + tokenResponse.put("id_token", "id-token-value"); + + server.createContext("/token", exchange -> { + byte[] body = MAPPER.writeValueAsBytes(tokenResponse); + exchange.getResponseHeaders().add("Content-Type", "application/json"); + exchange.sendResponseHeaders(200, body.length); + exchange.getResponseBody().write(body); + exchange.close(); + }); + + String tokenEndpoint = "http://localhost:" + serverPort + "/token"; + OidcPluginAuthClient.TokenResponse result = + client.exchangeCode(tokenEndpoint, "client-id", "client-secret", "auth-code", "http://localhost:8080/callback", "test-verifier"); + + assertThat(result.accessToken(), is(nullValue())); + assertThat(result.error(), containsString("No access token in response")); + } + + @Test + void return_error_when_token_endpoint_is_unreachable() { + OidcPluginAuthClient.TokenResponse result = + client.exchangeCode("http://localhost:1/token", "client-id", "client-secret", "auth-code", "http://localhost:8080/callback", "test-verifier"); + + assertThat(result.accessToken(), is(nullValue())); + assertThat(result.error(), containsString("Token exchange failed:")); + } + + @Test + void return_access_token_when_id_token_is_absent() { + ObjectNode tokenResponse = MAPPER.createObjectNode(); + tokenResponse.put("access_token", "access-only"); + tokenResponse.put("token_type", "Bearer"); + + server.createContext("/token", exchange -> { + byte[] body = MAPPER.writeValueAsBytes(tokenResponse); + exchange.getResponseHeaders().add("Content-Type", "application/json"); + exchange.sendResponseHeaders(200, body.length); + exchange.getResponseBody().write(body); + exchange.close(); + }); + + String tokenEndpoint = "http://localhost:" + serverPort + "/token"; + OidcPluginAuthClient.TokenResponse result = + client.exchangeCode(tokenEndpoint, "client-id", "client-secret", "auth-code", "http://localhost:8080/callback", "test-verifier"); + + assertThat(result.accessToken(), equalTo("access-only")); + assertThat(result.idToken(), is(nullValue())); + assertThat(result.error(), is(nullValue())); + } +} diff --git a/calm-hub/src/test/java/org/finos/calm/security/TestOidcRoleResolverShould.java b/calm-hub/src/test/java/org/finos/calm/security/TestOidcRoleResolverShould.java new file mode 100644 index 000000000..4657e2e04 --- /dev/null +++ b/calm-hub/src/test/java/org/finos/calm/security/TestOidcRoleResolverShould.java @@ -0,0 +1,112 @@ +package org.finos.calm.security; + +import io.quarkus.security.identity.SecurityIdentity; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import java.security.Principal; +import java.util.Optional; +import java.util.Set; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +class TestOidcRoleResolverShould { + + @Mock + private SecurityIdentity mockIdentity; + + @Mock + private Principal mockPrincipal; + + private OidcRoleResolver resolver; + + @BeforeEach + void setup() { + resolver = new OidcRoleResolver(); + resolver.globalAccessGroups = Optional.empty(); + } + + @Test + void return_none_for_null_identity() { + assertThat(resolver.resolve(null, Set.of("group-a")), equalTo(OidcRoleResolver.AccessLevel.NONE)); + } + + @Test + void return_none_for_anonymous_identity() { + when(mockIdentity.isAnonymous()).thenReturn(true); + assertThat(resolver.resolve(mockIdentity, Set.of("group-a")), equalTo(OidcRoleResolver.AccessLevel.NONE)); + } + + @Test + void return_none_when_no_access_groups_configured() { + when(mockIdentity.isAnonymous()).thenReturn(false); + resolver.globalAccessGroups = Optional.empty(); + + assertThat(resolver.resolve(mockIdentity, Set.of()), equalTo(OidcRoleResolver.AccessLevel.NONE)); + } + + @Test + void return_none_when_user_has_no_groups() { + when(mockIdentity.isAnonymous()).thenReturn(false); + when(mockIdentity.getRoles()).thenReturn(Set.of()); + when(mockIdentity.getPrincipal()).thenReturn(mockPrincipal); + when(mockPrincipal.getName()).thenReturn("alice"); + + assertThat(resolver.resolve(mockIdentity, Set.of("group-a")), equalTo(OidcRoleResolver.AccessLevel.NONE)); + } + + @Test + void return_none_when_user_has_null_groups() { + when(mockIdentity.isAnonymous()).thenReturn(false); + when(mockIdentity.getRoles()).thenReturn(null); + when(mockIdentity.getPrincipal()).thenReturn(mockPrincipal); + when(mockPrincipal.getName()).thenReturn("alice"); + + assertThat(resolver.resolve(mockIdentity, Set.of("group-a")), equalTo(OidcRoleResolver.AccessLevel.NONE)); + } + + @Test + void return_read_when_user_group_matches() { + when(mockIdentity.isAnonymous()).thenReturn(false); + when(mockIdentity.getRoles()).thenReturn(Set.of("group-a", "group-b")); + when(mockIdentity.getPrincipal()).thenReturn(mockPrincipal); + when(mockPrincipal.getName()).thenReturn("alice"); + + assertThat(resolver.resolve(mockIdentity, Set.of("group-b")), equalTo(OidcRoleResolver.AccessLevel.READ)); + } + + @Test + void return_none_when_user_group_does_not_match() { + when(mockIdentity.isAnonymous()).thenReturn(false); + when(mockIdentity.getRoles()).thenReturn(Set.of("group-x")); + when(mockIdentity.getPrincipal()).thenReturn(mockPrincipal); + when(mockPrincipal.getName()).thenReturn("alice"); + + assertThat(resolver.resolve(mockIdentity, Set.of("group-a")), equalTo(OidcRoleResolver.AccessLevel.NONE)); + } + + @Test + void fall_back_to_global_config_when_access_groups_empty() { + when(mockIdentity.isAnonymous()).thenReturn(false); + when(mockIdentity.getRoles()).thenReturn(Set.of("global-group")); + when(mockIdentity.getPrincipal()).thenReturn(mockPrincipal); + when(mockPrincipal.getName()).thenReturn("alice"); + resolver.globalAccessGroups = Optional.of("global-group;other-group"); + + assertThat(resolver.resolve(mockIdentity, Set.of()), equalTo(OidcRoleResolver.AccessLevel.READ)); + } + + @Test + void parse_blank_global_config_as_empty() { + when(mockIdentity.isAnonymous()).thenReturn(false); + resolver.globalAccessGroups = Optional.of(" "); + + assertThat(resolver.resolve(mockIdentity, Set.of()), equalTo(OidcRoleResolver.AccessLevel.NONE)); + } +} From 9f7a7dd9b4658cae98c9c61e2b746ee0513e0dcb Mon Sep 17 00:00:00 2001 From: James Gough Date: Wed, 9 Sep 2026 14:39:53 +0100 Subject: [PATCH 2/5] fix(calm-hub): address security review findings on OIDC plugin auth - PluginAuthResource: close XSS/state-hijack/open-redirect chain on the public plugin-login/plugin-callback endpoints. Validate redirect_path against a strict allowlist (blocks the http://localhost:@evil.com/ authority-injection variant too), bind state to a HttpOnly SameSite=Lax session cookie, make session lookup/consumption atomic and non-destructive on failure, add a TTL + size cap, and replace the concatenated HTML/JS callback page with a JSON data island read by a constant script (no interpolation), a per-response CSP nonce, and Cache-Control/Referrer-Policy headers. Send and verify the OIDC nonce. - CalmHubPermissionChecker: revert the unconditional hasAnyNamespaceAccess() widening on canReadByDomain -- any namespace grant was unlocking every domain, regardless of relation, across every auth profile. Restores the original ProxyAuthIntegration/unit-test assertions. - UserAccessValidator: drop @IfBuildProfile(secure, proxy-auth) entirely. Bean selection for that annotation is fixed at Maven build time and does not respond to calm-hub's documented runtime '-Dquarkus.profile' deployment model (CI builds one artifact, no -Dquarkus.profile flag) -- under that model the bean never activated for ANY profile, silently leaving SearchResource/DomainResource/SearchTools unfiltered. Every caller already gates on calm.auth.enabled first, so unconditional registration is safe. Removes NamespaceResource's narrower bespoke fallback (now redundant) and routes SearchTools through the shared ReadableScope helper instead of duplicating the lookup inline. Tracked more broadly as #3078 (other @IfBuildProfile usages may share the gap). - OidcPluginAuthClient: remove exchangeCode()/TokenResponse (dead code, unused since the exchange happens browser-side by design) and OidcRoleResolver (dead code, never wired up). Cache the discovery document and reuse one HttpClient instead of rebuilding per request on every public, unauthenticated endpoint hit. Rename calm.github.http.* properties to calm.oidc.http.* (this class has nothing to do with the GitHub backend). Fix a CDI ordering bug from the rewrite: build the HttpClient in @PostConstruct, not a field initializer that ran before @ConfigProperty injection completed. - application.properties: make /api/calm/auth/config public under every auth profile, not just oidc -- the SPA now fetches it before it knows how to authenticate, so it 401'd (and blanked the page, see next point) under the existing secure/proxy-auth profiles. - application-oidc.properties: remove misplaced calm.github.* entries (belong to the github profile), add a server-side HTTPS-only guard (quarkus.http.insecure-requests=disabled, dev override), remove the dead OidcRoleResolver config entries. - index.tsx: catch bootstrap() rejection and render an error state instead of leaving a permanently blank page when auth-config fetch fails. - Remove orphaned LogoutButton component/test (superseded by UserMenu). - Fix a test mock in authService.test.tsx asserting a 'github' field that doesn't exist on the AuthConfig type. - Fix TestNamespaceResourceShould/TestSearchResourceShould fixtures that depended on UserAccessValidator's old build-time gap for unfiltered results; they now mock UserAccessValidator explicitly. Reviewed via /code-review high (two passes) and an independent security review. Two open PR review threads addressed: pendingSessions eviction (fixed, replied+resolved) and the ID-token-as-bearer-credential finding (tracked as #3077, narrow-scoped out of this slice, replied, left open). Filed #3078 for the broader @IfBuildProfile pattern. mvn clean verify (unit+integration, 540 tests) and calm-hub-ui vitest (1443 tests) both green; JaCoCo coverage gate met. --- calm-hub-ui/src/authService.test.tsx | 1 - .../logout-button/LogoutButton.test.tsx | 50 --- .../components/logout-button/LogoutButton.tsx | 14 - calm-hub-ui/src/index.tsx | 12 +- .../integration/ProxyAuthIntegration.java | 4 +- .../org/finos/calm/mcp/tools/SearchTools.java | 7 +- .../calm/resources/NamespaceResource.java | 25 +- .../calm/resources/PluginAuthResource.java | 313 ++++++++++++--- .../finos/calm/resources/ReadableScope.java | 12 +- .../security/CalmHubPermissionChecker.java | 14 +- .../calm/security/OidcPluginAuthClient.java | 120 +++--- .../finos/calm/security/OidcRoleResolver.java | 76 ---- .../calm/security/UserAccessValidator.java | 17 +- .../resources/application-oidc.properties | 30 +- .../src/main/resources/application.properties | 10 + .../TestNamespaceResourceFilteringShould.java | 50 --- .../TestNamespaceResourceShould.java | 12 + .../TestPluginAuthResourceShould.java | 363 ++++++++++++------ .../resources/TestSearchResourceShould.java | 14 + .../TestCalmHubPermissionCheckerShould.java | 4 +- .../TestOidcPluginAuthClientShould.java | 190 ++++----- .../security/TestOidcRoleResolverShould.java | 112 ------ 22 files changed, 713 insertions(+), 737 deletions(-) delete mode 100644 calm-hub-ui/src/components/logout-button/LogoutButton.test.tsx delete mode 100644 calm-hub-ui/src/components/logout-button/LogoutButton.tsx delete mode 100644 calm-hub/src/main/java/org/finos/calm/security/OidcRoleResolver.java delete mode 100644 calm-hub/src/test/java/org/finos/calm/security/TestOidcRoleResolverShould.java diff --git a/calm-hub-ui/src/authService.test.tsx b/calm-hub-ui/src/authService.test.tsx index 2d7b11dc2..f7994494d 100644 --- a/calm-hub-ui/src/authService.test.tsx +++ b/calm-hub-ui/src/authService.test.tsx @@ -19,7 +19,6 @@ describe('authService', () => { it('should return true when the authority service responds successfully', async () => { vi.spyOn(authConfig, 'getAuthConfig').mockReturnValue({ oidc: { enabled: true, authority: 'https://auth.example.com' }, - github: { enabled: false }, databaseMode: 'mongo', }); vi.mocked(axios.head).mockResolvedValue({ status: 200 }); diff --git a/calm-hub-ui/src/components/logout-button/LogoutButton.test.tsx b/calm-hub-ui/src/components/logout-button/LogoutButton.test.tsx deleted file mode 100644 index e226a1baa..000000000 --- a/calm-hub-ui/src/components/logout-button/LogoutButton.test.tsx +++ /dev/null @@ -1,50 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { render, screen, fireEvent } from '@testing-library/react'; -import { LogoutButton } from './LogoutButton.js'; -import { authService } from '../../authService.js'; - -vi.mock('../../authService.js', () => ({ - authService: { - logout: vi.fn(), - }, -})); - -describe('LogoutButton', () => { - beforeEach(() => { - vi.clearAllMocks(); - }); - - it('renders a logout button', () => { - render(); - const button = screen.getByRole('button', { name: /logout/i }); - expect(button).toBeInTheDocument(); - }); - - it('has correct styling', () => { - render(); - const button = screen.getByRole('button', { name: /logout/i }); - expect(button).toHaveStyle({ - position: 'absolute', - top: '10px', - right: '10px', - }); - }); - - it('calls authService.logout when clicked', async () => { - render(); - const button = screen.getByRole('button', { name: /logout/i }); - - fireEvent.click(button); - - await new Promise(resolve => setTimeout(resolve, 0)); - - expect(authService.logout).toHaveBeenCalled(); - }); - - it('button text is "Logout"', () => { - render(); - const button = screen.getByRole('button'); - expect(button.textContent).toBe('Logout'); - }); -}); - diff --git a/calm-hub-ui/src/components/logout-button/LogoutButton.tsx b/calm-hub-ui/src/components/logout-button/LogoutButton.tsx deleted file mode 100644 index bf932c495..000000000 --- a/calm-hub-ui/src/components/logout-button/LogoutButton.tsx +++ /dev/null @@ -1,14 +0,0 @@ -import React from 'react'; -import { authService } from '../../authService.js'; - -export const LogoutButton: React.FC = () => { - const handleLogout = async () => { - await authService.logout(); - }; - - return ( - - ); -}; diff --git a/calm-hub-ui/src/index.tsx b/calm-hub-ui/src/index.tsx index 54d119055..b9a7cf267 100644 --- a/calm-hub-ui/src/index.tsx +++ b/calm-hub-ui/src/index.tsx @@ -29,4 +29,14 @@ async function bootstrap() { ); } -bootstrap(); +bootstrap().catch((error: unknown) => { + console.error('Failed to bootstrap CalmHub', error); + root.render( + +
+

CalmHub failed to load

+

Please try refreshing the page. If the problem persists, contact your administrator.

+
+
+ ); +}); diff --git a/calm-hub/src/integration-test/java/integration/ProxyAuthIntegration.java b/calm-hub/src/integration-test/java/integration/ProxyAuthIntegration.java index 63a8fa8bc..6ef1d6e1b 100644 --- a/calm-hub/src/integration-test/java/integration/ProxyAuthIntegration.java +++ b/calm-hub/src/integration-test/java/integration/ProxyAuthIntegration.java @@ -303,11 +303,11 @@ void user_with_domain_read_grant_is_forbidden_from_creating_a_control() { @Test @Order(15) - void user_with_namespace_grant_can_read_domain_controls() { + void user_with_namespace_grant_only_is_forbidden_from_reading_domain_controls() { given() .header(PROXY_HEADER, USER_ALICE) .when().get("/calm/domains/security/controls") .then() - .statusCode(200); + .statusCode(403); } } diff --git a/calm-hub/src/main/java/org/finos/calm/mcp/tools/SearchTools.java b/calm-hub/src/main/java/org/finos/calm/mcp/tools/SearchTools.java index 869fe9592..cf988aad5 100644 --- a/calm-hub/src/main/java/org/finos/calm/mcp/tools/SearchTools.java +++ b/calm-hub/src/main/java/org/finos/calm/mcp/tools/SearchTools.java @@ -11,6 +11,7 @@ import org.eclipse.microprofile.config.inject.ConfigProperty; import org.finos.calm.domain.search.GroupedSearchResults; import org.finos.calm.domain.search.SearchResult; +import org.finos.calm.resources.ReadableScope; import org.finos.calm.security.UserAccessValidator; import org.finos.calm.store.SearchStore; import org.slf4j.Logger; @@ -99,10 +100,8 @@ public ToolResponse searchHub( } private Optional> resolveReadableNamespaces() { - if (!authEnabled || !userAccessValidatorInstance.isResolvable()) { - return Optional.empty(); - } - return userAccessValidatorInstance.get().getReadableNamespaces(identity.getPrincipal().getName()); + return ReadableScope.resolve(authEnabled, userAccessValidatorInstance, identity, + UserAccessValidator::getReadableNamespaces); } private static Map> toGroupMap(GroupedSearchResults results) { diff --git a/calm-hub/src/main/java/org/finos/calm/resources/NamespaceResource.java b/calm-hub/src/main/java/org/finos/calm/resources/NamespaceResource.java index 6c3360516..8a6352a52 100644 --- a/calm-hub/src/main/java/org/finos/calm/resources/NamespaceResource.java +++ b/calm-hub/src/main/java/org/finos/calm/resources/NamespaceResource.java @@ -25,20 +25,17 @@ import org.finos.calm.domain.exception.NamespaceParentNotFoundException; import org.finos.calm.domain.namespaces.NamespaceCounts; import org.finos.calm.domain.namespaces.NamespaceInfo; -import org.finos.calm.domain.UserAccess; import org.finos.calm.security.AuditRequestFilter; import org.finos.calm.security.CalmHubPermissionChecker; import org.finos.calm.security.CalmHubScopes; import org.finos.calm.security.UserAccessValidator; import org.finos.calm.services.CountsService; import org.finos.calm.services.NamespaceService; -import org.finos.calm.store.UserAccessStore; import java.net.URI; import java.net.URISyntaxException; import java.util.Optional; import java.util.Set; -import java.util.stream.Collectors; import static org.finos.calm.resources.CalmResourceErrorResponses.invalidNamespaceResponse; import static org.finos.calm.resources.CalmResourceErrorResponses.namespaceNotEmptyResponse; @@ -59,9 +56,6 @@ public class NamespaceResource { @Inject CalmHubPermissionChecker permissionChecker; - @Inject - UserAccessStore userAccessStore; - @Inject @ConfigProperty(name = "calm.auth.enabled", defaultValue = "false") boolean authEnabled; @@ -109,23 +103,8 @@ public ValueWrapper namespaceCounts() { } private Optional> resolveReadableNamespaces() { - if (!authEnabled) { - return Optional.empty(); - } - if (userAccessValidatorInstance.isResolvable()) { - return ReadableScope.resolve(authEnabled, userAccessValidatorInstance, identity, - UserAccessValidator::getReadableNamespaces); - } - // Fallback: derive readable set from UserAccessStore grants (GitHub/OIDC mode) - if (identity.isAnonymous() || identity.getPrincipal() == null || userAccessStore == null) { - return Optional.empty(); - } - String username = identity.getPrincipal().getName(); - Set readable = userAccessStore.getGrantsForUser(username).stream() - .map(UserAccess::getNamespace) - .filter(ns -> ns != null) - .collect(Collectors.toSet()); - return Optional.of(readable); + return ReadableScope.resolve(authEnabled, userAccessValidatorInstance, identity, + UserAccessValidator::getReadableNamespaces); } @POST diff --git a/calm-hub/src/main/java/org/finos/calm/resources/PluginAuthResource.java b/calm-hub/src/main/java/org/finos/calm/resources/PluginAuthResource.java index d79ec7762..c3124589a 100644 --- a/calm-hub/src/main/java/org/finos/calm/resources/PluginAuthResource.java +++ b/calm-hub/src/main/java/org/finos/calm/resources/PluginAuthResource.java @@ -1,11 +1,14 @@ package org.finos.calm.resources; +import com.fasterxml.jackson.databind.ObjectMapper; import jakarta.inject.Inject; +import jakarta.ws.rs.CookieParam; import jakarta.ws.rs.GET; import jakarta.ws.rs.Path; import jakarta.ws.rs.Produces; import jakarta.ws.rs.QueryParam; import jakarta.ws.rs.core.MediaType; +import jakarta.ws.rs.core.NewCookie; import jakarta.ws.rs.core.Response; import org.eclipse.microprofile.config.inject.ConfigProperty; import org.finos.calm.security.OidcPluginAuthClient; @@ -15,10 +18,17 @@ import java.net.URI; import java.net.URLEncoder; import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; +import java.security.SecureRandom; +import java.util.Base64; import java.util.Map; import java.util.Optional; import java.util.UUID; import java.util.concurrent.ConcurrentHashMap; +import java.util.regex.Pattern; + +import static org.finos.calm.resources.SearchResource.sanitizeForLog; /** * Handles OIDC authentication on behalf of the VS Code plugin. @@ -26,17 +36,49 @@ * full OIDC authorization code flow and redirects back to the plugin's * localhost with the access token. * - * Both endpoints are public (no auth required) — the user has not yet + *

Both endpoints are public (no auth required) — the user has not yet * authenticated when they hit plugin-login, and plugin-callback receives - * the IdP redirect. + * the IdP redirect. Because both are reachable by anyone, this class treats + * every caller-suppliable value ({@code redirect_path}, {@code state}, + * {@code code}, the IdP {@code error} body) as untrusted: + *

    + *
  • {@code redirect_path} is validated against a strict allowlist so it can + * never be used to redirect off {@code localhost} or break out of the + * eventual JS string/HTML context (see {@link #isValidRedirectPath}).
  • + *
  • The {@code state} lookup is bound to a random, {@code HttpOnly}, + * {@code SameSite=Lax} cookie set on {@code /plugin-login} and checked on + * {@code /plugin-callback} — {@code state} alone is not treated as a + * secret, since {@code /plugin-login} is public and returns it in the + * redirect {@code Location} header to anyone who asks.
  • + *
  • Session lookup is non-destructive until the cookie check passes, and + * consumption is a single atomic {@code remove(state, session)} — + * an attacker who doesn't hold the matching cookie can neither read nor + * delete another session's pending state.
  • + *
  • Every value that reaches the callback HTML is carried through a + * {@code JSON.parse}-based data island, never string-concatenated into + * JavaScript or HTML.
  • + *
*/ @Path("/api/calm/auth") @Produces(MediaType.APPLICATION_JSON) public class PluginAuthResource { private static final Logger LOG = LoggerFactory.getLogger(PluginAuthResource.class); + private static final ObjectMapper MAPPER = new ObjectMapper(); + private static final SecureRandom SECURE_RANDOM = new SecureRandom(); + + static final String SESSION_COOKIE_NAME = "calm_plugin_auth_session"; + static final int SESSION_TTL_SECONDS = 600; + private static final long SESSION_TTL_MILLIS = SESSION_TTL_SECONDS * 1000L; + private static final int MAX_PENDING_SESSIONS = 10_000; private static final String DEFAULT_REDIRECT_PATH = "/callback"; + // Leading slash, then a bounded allowlist of path-safe characters. No "@" (blocks + // http://localhost:port@attacker.example.com/ authority injection), no query/fragment + // markers, and explicitly rejects "//" (protocol-relative) and ".." (traversal) below — + // the character class alone doesn't rule those combinations out. + private static final Pattern REDIRECT_PATH_PATTERN = Pattern.compile("^/[A-Za-z0-9._/-]{0,64}$"); + private final Map pendingSessions = new ConcurrentHashMap<>(); @Inject @@ -58,7 +100,12 @@ public class PluginAuthResource { @ConfigProperty(name = "calm.hub.base-url", defaultValue = "http://localhost:8080") String hubBaseUrl; - record PendingSession(String port, String redirectPath, String codeVerifier, String nonce) {} + record PendingSession(String port, String redirectPath, String codeVerifier, String nonce, + String correlator, long expiresAtEpochMillis) { + boolean isExpired(long nowEpochMillis) { + return expiresAtEpochMillis <= nowEpochMillis; + } + } @GET @Path("plugin-login") @@ -77,6 +124,12 @@ public Response pluginLogin(@QueryParam("port") String port, .build(); } + if (redirectPath != null && !redirectPath.isBlank() && !isValidRedirectPath(redirectPath)) { + return Response.status(Response.Status.BAD_REQUEST) + .entity(Map.of("error", "redirect_path must be a relative path beginning with '/'")) + .build(); + } + if (oidcAuthority.isEmpty() || oidcAuthority.get().isBlank()) { return Response.status(Response.Status.SERVICE_UNAVAILABLE) .entity(Map.of("error", "OIDC is not configured")) @@ -96,46 +149,70 @@ public Response pluginLogin(@QueryParam("port") String port, .build(); } - byte[] verifierBytes = new byte[32]; - new java.security.SecureRandom().nextBytes(verifierBytes); - String codeVerifier = java.util.Base64.getUrlEncoder().withoutPadding().encodeToString(verifierBytes); + evictExpiredSessions(); + if (pendingSessions.size() >= MAX_PENDING_SESSIONS) { + LOG.warn("Rejecting plugin-login: {} pending sessions already outstanding", pendingSessions.size()); + return Response.status(Response.Status.SERVICE_UNAVAILABLE) + .entity(Map.of("error", "Too many pending authentication sessions, try again shortly")) + .build(); + } + + String codeVerifier = randomUrlSafeToken(32); String codeChallenge; try { - byte[] digest = java.security.MessageDigest.getInstance("SHA-256").digest(codeVerifier.getBytes(StandardCharsets.UTF_8)); - codeChallenge = java.util.Base64.getUrlEncoder().withoutPadding().encodeToString(digest); - } catch (java.security.NoSuchAlgorithmException e) { + byte[] digest = MessageDigest.getInstance("SHA-256").digest(codeVerifier.getBytes(StandardCharsets.UTF_8)); + codeChallenge = Base64.getUrlEncoder().withoutPadding().encodeToString(digest); + } catch (NoSuchAlgorithmException e) { return Response.status(Response.Status.INTERNAL_SERVER_ERROR) .entity(Map.of("error", "SHA-256 not available")).build(); } String state = UUID.randomUUID().toString(); + String correlator = randomUrlSafeToken(32); String effectiveRedirectPath = (redirectPath != null && !redirectPath.isBlank()) ? redirectPath : DEFAULT_REDIRECT_PATH; - pendingSessions.put(state, new PendingSession(port, effectiveRedirectPath, codeVerifier, nonce)); + long expiresAt = System.currentTimeMillis() + SESSION_TTL_MILLIS; + pendingSessions.put(state, new PendingSession(port, effectiveRedirectPath, codeVerifier, nonce, + correlator, expiresAt)); String hubCallbackUrl = hubBaseUrl + "/api/calm/auth/plugin-callback"; - String authorizeUrl = endpoints.authorizationEndpoint() - + "?client_id=" + encode(oidcClientId.get()) - + "&response_type=code" - + "&scope=" + encode(oidcScopes) - + "&redirect_uri=" + encode(hubCallbackUrl) - + "&state=" + encode(state) - + "&code_challenge=" + encode(codeChallenge) - + "&code_challenge_method=S256"; + StringBuilder authorizeUrl = new StringBuilder(endpoints.authorizationEndpoint()) + .append("?client_id=").append(encode(oidcClientId.get())) + .append("&response_type=code") + .append("&scope=").append(encode(oidcScopes)) + .append("&redirect_uri=").append(encode(hubCallbackUrl)) + .append("&state=").append(encode(state)) + .append("&code_challenge=").append(encode(codeChallenge)) + .append("&code_challenge_method=S256"); + if (nonce != null && !nonce.isBlank()) { + authorizeUrl.append("&nonce=").append(encode(nonce)); + } LOG.debug("Redirecting plugin auth to OIDC authorize endpoint for port {}", port); - return Response.temporaryRedirect(URI.create(authorizeUrl)).build(); + NewCookie sessionCookie = new NewCookie.Builder(SESSION_COOKIE_NAME) + .value(correlator) + .path("/api/calm/auth") + .maxAge(SESSION_TTL_SECONDS) + .httpOnly(true) + .secure(hubBaseUrl.startsWith("https://")) + .sameSite(NewCookie.SameSite.LAX) + .build(); + + return Response.temporaryRedirect(URI.create(authorizeUrl.toString())) + .cookie(sessionCookie) + .build(); } @GET @Path("plugin-callback") public Response pluginCallback(@QueryParam("code") String code, @QueryParam("state") String state, - @QueryParam("error") String error) { + @QueryParam("error") String error, + @CookieParam(SESSION_COOKIE_NAME) String sessionCookie) { if (error != null && !error.isBlank()) { - LOG.warn("OIDC IdP returned error: {}", error); + LOG.warn("OIDC IdP returned error: {}", sanitizeForLog(error)); return Response.status(Response.Status.BAD_GATEWAY) .entity(Map.of("error", "Authentication failed at identity provider")) .build(); @@ -153,14 +230,35 @@ public Response pluginCallback(@QueryParam("code") String code, .build(); } - PendingSession session = pendingSessions.remove(state); - if (session == null) { + // Non-destructive lookup: an attacker without the matching cookie must not be + // able to delete a victim's pending session by hitting this endpoint. + PendingSession session = pendingSessions.get(state); + if (session == null || session.isExpired(System.currentTimeMillis())) { + if (session != null) { + pendingSessions.remove(state, session); + } LOG.warn("Invalid or expired state parameter in plugin callback"); return Response.status(Response.Status.FORBIDDEN) .entity(Map.of("error", "Invalid or expired state")) .build(); } + if (sessionCookie == null || !constantTimeEquals(sessionCookie, session.correlator())) { + LOG.warn("Plugin callback cookie missing or did not match the pending session"); + return Response.status(Response.Status.FORBIDDEN) + .entity(Map.of("error", "Invalid or expired state")) + .build(); + } + + // Single-use: only remove once the cookie has actually been validated, and only + // if this exact session is still the one in the map (defends against a race with + // a concurrent callback for the same state). + if (!pendingSessions.remove(state, session)) { + return Response.status(Response.Status.FORBIDDEN) + .entity(Map.of("error", "Invalid or expired state")) + .build(); + } + if (oidcAuthority.isEmpty() || oidcAuthority.get().isBlank()) { return Response.status(Response.Status.SERVICE_UNAVAILABLE) .entity(Map.of("error", "OIDC is not configured")) @@ -181,34 +279,21 @@ public Response pluginCallback(@QueryParam("code") String code, } String hubCallbackUrl = hubBaseUrl + "/api/calm/auth/plugin-callback"; - String pluginRedirect = "http://localhost:" + session.port() + session.redirectPath(); - String nonceParam = session.nonce() != null ? "&nonce=" + encode(session.nonce()) : ""; - - String html = "

Signing in...

"; - - return Response.ok(html).type("text/html").build(); + String pluginOrigin = "http://localhost:" + session.port(); + String tokenOrigin = originOf(endpoints.tokenEndpoint()); + String cspNonce = randomUrlSafeToken(16); + + String html = renderCallbackPage(new CallbackPageData( + endpoints.tokenEndpoint(), oidcClientId.get(), code, hubCallbackUrl, + session.codeVerifier(), pluginOrigin, session.redirectPath(), session.nonce()), cspNonce); + + return Response.ok(html) + .type("text/html") + .header("Content-Security-Policy", "script-src 'nonce-" + cspNonce + "'; " + + "connect-src " + tokenOrigin + "; base-uri 'none'; form-action 'none'") + .header("Cache-Control", "no-store") + .header("Referrer-Policy", "no-referrer") + .build(); } // Visible for testing @@ -216,6 +301,11 @@ Map getPendingSessions() { return pendingSessions; } + private void evictExpiredSessions() { + long now = System.currentTimeMillis(); + pendingSessions.entrySet().removeIf(e -> e.getValue().isExpired(now)); + } + private boolean isValidPort(String port) { try { int portNum = Integer.parseInt(port); @@ -225,7 +315,128 @@ private boolean isValidPort(String port) { } } + /** + * Strict allowlist for {@code redirect_path}: must already have matched + * {@link #REDIRECT_PATH_PATTERN} (leading slash, bounded safe-character set) — this + * additionally rejects a protocol-relative {@code //} prefix and any {@code ..} + * traversal segment, neither of which the character class alone excludes. + */ + private static boolean isValidRedirectPath(String path) { + return REDIRECT_PATH_PATTERN.matcher(path).matches() + && !path.startsWith("//") + && !path.contains(".."); + } + + private static String randomUrlSafeToken(int byteLength) { + byte[] bytes = new byte[byteLength]; + SECURE_RANDOM.nextBytes(bytes); + return Base64.getUrlEncoder().withoutPadding().encodeToString(bytes); + } + + private static boolean constantTimeEquals(String a, String b) { + return MessageDigest.isEqual(a.getBytes(StandardCharsets.UTF_8), b.getBytes(StandardCharsets.UTF_8)); + } + + private static String originOf(String url) { + URI uri = URI.create(url); + return uri.getScheme() + "://" + uri.getAuthority(); + } + private String encode(String value) { return URLEncoder.encode(value, StandardCharsets.UTF_8); } + + /** + * Everything the callback page's script needs, carried as a JSON data island rather + * than interpolated into JavaScript/HTML — {@code code} in particular is IdP-supplied + * and must never be concatenated into a script. + */ + record CallbackPageData(String tokenEndpoint, String clientId, String code, String redirectUri, + String codeVerifier, String pluginOrigin, String redirectPath, String nonce) {} + + private static String renderCallbackPage(CallbackPageData data, String cspNonce) { + String json; + try { + json = MAPPER.writeValueAsString(data); + } catch (Exception e) { + // Should be unreachable — every field is a String — but fail safe rather than + // leak an unescaped value into the page if serialization ever does throw. + throw new IllegalStateException("Failed to serialize plugin callback data", e); + } + return "Signing in…" + + "" + + "

Signing in…

" + + "" + + "" + + ""; + } + + /** + * Neutralises the sequences that could break out of the {@code "; + plantSession("valid-state", "63348", "/callback", CORRELATOR); + stubDiscovery(); - Response response = resource.pluginCallback("valid-code", "valid-state", null); + Response response = resource.pluginCallback(maliciousCode, "valid-state", null, CORRELATOR); - assertThat(response.getStatus(), equalTo(200)); String html = (String) response.getEntity(); - assertThat(html, containsString("http://localhost:9999/auth/done")); - assertThat(html, containsString("code_verifier")); - assertThat(html, containsString("test-verifier")); + assertThat(html, not(containsString(""))); + JsonNode data = extractDataIsland(html); + assertThat(data.get("code").asText(), equalTo(maliciousCode)); } @Test - void remove_state_from_pending_sessions_after_callback() { - resource.getPendingSessions().put("valid-state", - new PluginAuthResource.PendingSession("63348", "/callback", "test-verifier", "test-nonce")); - OidcPluginAuthClient.OidcEndpoints endpoints = - new OidcPluginAuthClient.OidcEndpoints(AUTHORIZATION_ENDPOINT, TOKEN_ENDPOINT); - when(mockOidcClient.discoverEndpoints(AUTH_SERVER_URL)).thenReturn(endpoints); + void set_a_strict_content_security_policy_cache_control_and_referrer_policy_on_the_callback_page() { + plantSession("valid-state", "63348", "/callback", CORRELATOR); + stubDiscovery(); + + Response response = resource.pluginCallback("valid-code", "valid-state", null, CORRELATOR); + + String csp = (String) response.getMetadata().getFirst("Content-Security-Policy"); + assertThat(csp, allOf( + containsString("script-src 'nonce-"), + containsString("connect-src https://login.microsoftonline.com"), + containsString("base-uri 'none'"), + containsString("form-action 'none'"))); + assertThat(response.getMetadata().getFirst("Cache-Control"), equalTo("no-store")); + assertThat(response.getMetadata().getFirst("Referrer-Policy"), equalTo("no-referrer")); + } - Response response = resource.pluginCallback("valid-code", "valid-state", null); + @Test + void remove_state_from_pending_sessions_after_a_successful_callback() { + plantSession("valid-state", "63348", "/callback", CORRELATOR); + stubDiscovery(); + + Response response = resource.pluginCallback("valid-code", "valid-state", null, CORRELATOR); assertThat(response.getStatus(), equalTo(200)); assertThat(resource.getPendingSessions().containsKey("valid-state"), is(false)); } @Test - void return_400_for_negative_port() { - Response response = resource.pluginLogin("-1", null, null); + void reject_reuse_of_an_already_consumed_state() { + plantSession("valid-state", "63348", "/callback", CORRELATOR); + stubDiscovery(); + resource.pluginCallback("valid-code", "valid-state", null, CORRELATOR); - assertThat(response.getStatus(), equalTo(400)); + Response second = resource.pluginCallback("valid-code", "valid-state", null, CORRELATOR); + + assertThat(second.getStatus(), equalTo(403)); + } + + private JsonNode extractDataIsland(String html) throws Exception { + String marker = "id=\"calm-plugin-auth-data\">"; + int start = html.indexOf(marker) + marker.length(); + int end = html.indexOf("", start); + String json = html.substring(start, end); + return MAPPER.readTree(json); } } diff --git a/calm-hub/src/test/java/org/finos/calm/resources/TestSearchResourceShould.java b/calm-hub/src/test/java/org/finos/calm/resources/TestSearchResourceShould.java index 23cea9348..0ac0147c0 100644 --- a/calm-hub/src/test/java/org/finos/calm/resources/TestSearchResourceShould.java +++ b/calm-hub/src/test/java/org/finos/calm/resources/TestSearchResourceShould.java @@ -5,7 +5,9 @@ import io.quarkus.test.security.TestSecurity; import org.finos.calm.domain.search.GroupedSearchResults; import org.finos.calm.domain.search.SearchResult; +import org.finos.calm.security.UserAccessValidator; import org.finos.calm.store.SearchStore; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.params.ParameterizedTest; @@ -32,6 +34,18 @@ public class TestSearchResourceShould { @InjectMock SearchStore mockSearchStore; + // See the equivalent field in TestNamespaceResourceShould: @TestSecurity(authorizationEnabled + // = false) bypasses declarative permission checks but not SearchResource's own ReadableScope + // lookup, so without this the identity-less test principal resolves to zero grants instead + // of the unfiltered Optional.empty() these tests expect. + @InjectMock + UserAccessValidator mockUserAccessValidator; + + @BeforeEach + void setUpUserAccessValidator() { + lenient().when(mockUserAccessValidator.getReadableNamespaces(any())).thenReturn(Optional.empty()); + } + static Stream provideInvalidQueryParameters() { return Stream.of( Arguments.of(null, "Query parameter 'q' is required"), diff --git a/calm-hub/src/test/java/org/finos/calm/security/TestCalmHubPermissionCheckerShould.java b/calm-hub/src/test/java/org/finos/calm/security/TestCalmHubPermissionCheckerShould.java index 1da113d79..59dcdb2c6 100644 --- a/calm-hub/src/test/java/org/finos/calm/security/TestCalmHubPermissionCheckerShould.java +++ b/calm-hub/src/test/java/org/finos/calm/security/TestCalmHubPermissionCheckerShould.java @@ -358,12 +358,12 @@ void grant_for_different_domain_denies_domain_read() { } @Test - void namespace_grant_satisfies_domain_read() { + void namespace_grant_does_not_satisfy_domain_read() { givenAuthenticatedUser("alice"); when(mockUserAccessStore.getGrantsForUser("alice")) .thenReturn(List.of(grant("alice", UserAccess.Permission.read, "payments"))); - assertTrue(checker.canReadByDomain(mockIdentity, "payments")); + assertFalse(checker.canReadByDomain(mockIdentity, "payments")); } @Test diff --git a/calm-hub/src/test/java/org/finos/calm/security/TestOidcPluginAuthClientShould.java b/calm-hub/src/test/java/org/finos/calm/security/TestOidcPluginAuthClientShould.java index 362cab962..4b4dd4f77 100644 --- a/calm-hub/src/test/java/org/finos/calm/security/TestOidcPluginAuthClientShould.java +++ b/calm-hub/src/test/java/org/finos/calm/security/TestOidcPluginAuthClientShould.java @@ -8,14 +8,16 @@ import org.junit.jupiter.api.Test; import java.lang.reflect.Field; +import java.lang.reflect.Method; import java.net.InetSocketAddress; +import java.util.concurrent.atomic.AtomicInteger; import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; +import static org.hamcrest.Matchers.sameInstance; class TestOidcPluginAuthClientShould { @@ -24,12 +26,14 @@ class TestOidcPluginAuthClientShould { private OidcPluginAuthClient client; private HttpServer server; private int serverPort; + private final AtomicInteger discoveryHits = new AtomicInteger(); @BeforeEach void setup() throws Exception { client = new OidcPluginAuthClient(); setField("connectTimeoutSeconds", 5); setField("requestTimeoutSeconds", 5); + invokeInit(); // Start a local HTTP server for testing server = HttpServer.create(new InetSocketAddress(0), 0); @@ -50,22 +54,32 @@ private void setField(String name, Object value) throws Exception { field.set(client, value); } - // --- discoverEndpoints tests --- - - @Test - void return_endpoints_from_valid_discovery_document() { - ObjectNode discoveryDoc = MAPPER.createObjectNode(); - discoveryDoc.put("authorization_endpoint", "https://idp.example.com/authorize"); - discoveryDoc.put("token_endpoint", "https://idp.example.com/token"); - discoveryDoc.put("issuer", "https://idp.example.com"); + private void invokeInit() throws Exception { + Method init = OidcPluginAuthClient.class.getDeclaredMethod("init"); + init.setAccessible(true); + init.invoke(client); + } + private void serveDiscoveryDocument(ObjectNode discoveryDoc) { server.createContext("/.well-known/openid-configuration", exchange -> { + discoveryHits.incrementAndGet(); byte[] body = MAPPER.writeValueAsBytes(discoveryDoc); exchange.getResponseHeaders().add("Content-Type", "application/json"); exchange.sendResponseHeaders(200, body.length); exchange.getResponseBody().write(body); exchange.close(); }); + } + + // --- discoverEndpoints tests --- + + @Test + void return_endpoints_from_valid_discovery_document() { + ObjectNode discoveryDoc = MAPPER.createObjectNode(); + discoveryDoc.put("authorization_endpoint", "https://idp.example.com/authorize"); + discoveryDoc.put("token_endpoint", "https://idp.example.com/token"); + discoveryDoc.put("issuer", "https://idp.example.com"); + serveDiscoveryDocument(discoveryDoc); OidcPluginAuthClient.OidcEndpoints result = client.discoverEndpoints("http://localhost:" + serverPort); @@ -80,14 +94,7 @@ void return_endpoints_when_issuer_url_has_trailing_slash() { ObjectNode discoveryDoc = MAPPER.createObjectNode(); discoveryDoc.put("authorization_endpoint", "https://idp.example.com/authorize"); discoveryDoc.put("token_endpoint", "https://idp.example.com/token"); - - server.createContext("/.well-known/openid-configuration", exchange -> { - byte[] body = MAPPER.writeValueAsBytes(discoveryDoc); - exchange.getResponseHeaders().add("Content-Type", "application/json"); - exchange.sendResponseHeaders(200, body.length); - exchange.getResponseBody().write(body); - exchange.close(); - }); + serveDiscoveryDocument(discoveryDoc); OidcPluginAuthClient.OidcEndpoints result = client.discoverEndpoints("http://localhost:" + serverPort + "/"); @@ -113,14 +120,7 @@ void return_null_when_discovery_returns_non_200() { void return_null_when_discovery_document_missing_authorization_endpoint() { ObjectNode discoveryDoc = MAPPER.createObjectNode(); discoveryDoc.put("token_endpoint", "https://idp.example.com/token"); - - server.createContext("/.well-known/openid-configuration", exchange -> { - byte[] body = MAPPER.writeValueAsBytes(discoveryDoc); - exchange.getResponseHeaders().add("Content-Type", "application/json"); - exchange.sendResponseHeaders(200, body.length); - exchange.getResponseBody().write(body); - exchange.close(); - }); + serveDiscoveryDocument(discoveryDoc); OidcPluginAuthClient.OidcEndpoints result = client.discoverEndpoints("http://localhost:" + serverPort); @@ -132,14 +132,7 @@ void return_null_when_discovery_document_missing_authorization_endpoint() { void return_null_when_discovery_document_missing_token_endpoint() { ObjectNode discoveryDoc = MAPPER.createObjectNode(); discoveryDoc.put("authorization_endpoint", "https://idp.example.com/authorize"); - - server.createContext("/.well-known/openid-configuration", exchange -> { - byte[] body = MAPPER.writeValueAsBytes(discoveryDoc); - exchange.getResponseHeaders().add("Content-Type", "application/json"); - exchange.sendResponseHeaders(200, body.length); - exchange.getResponseBody().write(body); - exchange.close(); - }); + serveDiscoveryDocument(discoveryDoc); OidcPluginAuthClient.OidcEndpoints result = client.discoverEndpoints("http://localhost:" + serverPort); @@ -171,98 +164,57 @@ void return_null_when_discovery_returns_invalid_json() { assertThat(result, is(nullValue())); } - // --- exchangeCode tests --- + // --- discovery caching --- @Test - void return_access_token_on_successful_exchange() { - ObjectNode tokenResponse = MAPPER.createObjectNode(); - tokenResponse.put("access_token", "eyJhbGciOi..."); - tokenResponse.put("id_token", "id-token-value"); - tokenResponse.put("token_type", "Bearer"); - - server.createContext("/token", exchange -> { - byte[] body = MAPPER.writeValueAsBytes(tokenResponse); - exchange.getResponseHeaders().add("Content-Type", "application/json"); - exchange.sendResponseHeaders(200, body.length); - exchange.getResponseBody().write(body); - exchange.close(); - }); - - String tokenEndpoint = "http://localhost:" + serverPort + "/token"; - OidcPluginAuthClient.TokenResponse result = - client.exchangeCode(tokenEndpoint, "client-id", "client-secret", "auth-code", "http://localhost:8080/callback", "test-verifier"); - - assertThat(result, is(notNullValue())); - assertThat(result.accessToken(), equalTo("eyJhbGciOi...")); - assertThat(result.idToken(), equalTo("id-token-value")); - assertThat(result.error(), is(nullValue())); - } - - @Test - void return_error_when_token_endpoint_returns_non_200() { - server.createContext("/token", exchange -> { - exchange.sendResponseHeaders(400, -1); - exchange.close(); - }); - - String tokenEndpoint = "http://localhost:" + serverPort + "/token"; - OidcPluginAuthClient.TokenResponse result = - client.exchangeCode(tokenEndpoint, "client-id", "client-secret", "bad-code", "http://localhost:8080/callback", "test-verifier"); - - assertThat(result.accessToken(), is(nullValue())); - assertThat(result.error(), containsString("Token exchange failed with status 400")); - } - - @Test - void return_error_when_response_missing_access_token() { - ObjectNode tokenResponse = MAPPER.createObjectNode(); - tokenResponse.put("id_token", "id-token-value"); - - server.createContext("/token", exchange -> { - byte[] body = MAPPER.writeValueAsBytes(tokenResponse); - exchange.getResponseHeaders().add("Content-Type", "application/json"); - exchange.sendResponseHeaders(200, body.length); - exchange.getResponseBody().write(body); - exchange.close(); - }); - - String tokenEndpoint = "http://localhost:" + serverPort + "/token"; - OidcPluginAuthClient.TokenResponse result = - client.exchangeCode(tokenEndpoint, "client-id", "client-secret", "auth-code", "http://localhost:8080/callback", "test-verifier"); - - assertThat(result.accessToken(), is(nullValue())); - assertThat(result.error(), containsString("No access token in response")); - } + void cache_the_discovery_document_across_calls_for_the_same_issuer() { + ObjectNode discoveryDoc = MAPPER.createObjectNode(); + discoveryDoc.put("authorization_endpoint", "https://idp.example.com/authorize"); + discoveryDoc.put("token_endpoint", "https://idp.example.com/token"); + serveDiscoveryDocument(discoveryDoc); + String issuer = "http://localhost:" + serverPort; - @Test - void return_error_when_token_endpoint_is_unreachable() { - OidcPluginAuthClient.TokenResponse result = - client.exchangeCode("http://localhost:1/token", "client-id", "client-secret", "auth-code", "http://localhost:8080/callback", "test-verifier"); + OidcPluginAuthClient.OidcEndpoints first = client.discoverEndpoints(issuer); + server.removeContext("/.well-known/openid-configuration"); + OidcPluginAuthClient.OidcEndpoints second = client.discoverEndpoints(issuer); - assertThat(result.accessToken(), is(nullValue())); - assertThat(result.error(), containsString("Token exchange failed:")); + assertThat(discoveryHits.get(), equalTo(1)); + assertThat(second, sameInstance(first)); } @Test - void return_access_token_when_id_token_is_absent() { - ObjectNode tokenResponse = MAPPER.createObjectNode(); - tokenResponse.put("access_token", "access-only"); - tokenResponse.put("token_type", "Bearer"); - - server.createContext("/token", exchange -> { - byte[] body = MAPPER.writeValueAsBytes(tokenResponse); - exchange.getResponseHeaders().add("Content-Type", "application/json"); - exchange.sendResponseHeaders(200, body.length); - exchange.getResponseBody().write(body); - exchange.close(); - }); - - String tokenEndpoint = "http://localhost:" + serverPort + "/token"; - OidcPluginAuthClient.TokenResponse result = - client.exchangeCode(tokenEndpoint, "client-id", "client-secret", "auth-code", "http://localhost:8080/callback", "test-verifier"); - - assertThat(result.accessToken(), equalTo("access-only")); - assertThat(result.idToken(), is(nullValue())); - assertThat(result.error(), is(nullValue())); + void refetch_discovery_when_the_issuer_changes() throws Exception { + ObjectNode discoveryDocA = MAPPER.createObjectNode(); + discoveryDocA.put("authorization_endpoint", "https://a.example.com/authorize"); + discoveryDocA.put("token_endpoint", "https://a.example.com/token"); + serveDiscoveryDocument(discoveryDocA); + String issuerA = "http://localhost:" + serverPort; + + client.discoverEndpoints(issuerA); + + HttpServer serverB = null; + try { + serverB = HttpServer.create(new InetSocketAddress(0), 0); + int portB = serverB.getAddress().getPort(); + ObjectNode discoveryDocB = MAPPER.createObjectNode(); + discoveryDocB.put("authorization_endpoint", "https://b.example.com/authorize"); + discoveryDocB.put("token_endpoint", "https://b.example.com/token"); + byte[] body = MAPPER.writeValueAsBytes(discoveryDocB); + serverB.createContext("/.well-known/openid-configuration", exchange -> { + exchange.getResponseHeaders().add("Content-Type", "application/json"); + exchange.sendResponseHeaders(200, body.length); + exchange.getResponseBody().write(body); + exchange.close(); + }); + serverB.start(); + + OidcPluginAuthClient.OidcEndpoints result = client.discoverEndpoints("http://localhost:" + portB); + + assertThat(result.authorizationEndpoint(), equalTo("https://b.example.com/authorize")); + } finally { + if (serverB != null) { + serverB.stop(0); + } + } } } diff --git a/calm-hub/src/test/java/org/finos/calm/security/TestOidcRoleResolverShould.java b/calm-hub/src/test/java/org/finos/calm/security/TestOidcRoleResolverShould.java deleted file mode 100644 index 4657e2e04..000000000 --- a/calm-hub/src/test/java/org/finos/calm/security/TestOidcRoleResolverShould.java +++ /dev/null @@ -1,112 +0,0 @@ -package org.finos.calm.security; - -import io.quarkus.security.identity.SecurityIdentity; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.Mock; -import org.mockito.junit.jupiter.MockitoExtension; - -import java.security.Principal; -import java.util.Optional; -import java.util.Set; - -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.equalTo; -import static org.mockito.Mockito.when; - -@ExtendWith(MockitoExtension.class) -class TestOidcRoleResolverShould { - - @Mock - private SecurityIdentity mockIdentity; - - @Mock - private Principal mockPrincipal; - - private OidcRoleResolver resolver; - - @BeforeEach - void setup() { - resolver = new OidcRoleResolver(); - resolver.globalAccessGroups = Optional.empty(); - } - - @Test - void return_none_for_null_identity() { - assertThat(resolver.resolve(null, Set.of("group-a")), equalTo(OidcRoleResolver.AccessLevel.NONE)); - } - - @Test - void return_none_for_anonymous_identity() { - when(mockIdentity.isAnonymous()).thenReturn(true); - assertThat(resolver.resolve(mockIdentity, Set.of("group-a")), equalTo(OidcRoleResolver.AccessLevel.NONE)); - } - - @Test - void return_none_when_no_access_groups_configured() { - when(mockIdentity.isAnonymous()).thenReturn(false); - resolver.globalAccessGroups = Optional.empty(); - - assertThat(resolver.resolve(mockIdentity, Set.of()), equalTo(OidcRoleResolver.AccessLevel.NONE)); - } - - @Test - void return_none_when_user_has_no_groups() { - when(mockIdentity.isAnonymous()).thenReturn(false); - when(mockIdentity.getRoles()).thenReturn(Set.of()); - when(mockIdentity.getPrincipal()).thenReturn(mockPrincipal); - when(mockPrincipal.getName()).thenReturn("alice"); - - assertThat(resolver.resolve(mockIdentity, Set.of("group-a")), equalTo(OidcRoleResolver.AccessLevel.NONE)); - } - - @Test - void return_none_when_user_has_null_groups() { - when(mockIdentity.isAnonymous()).thenReturn(false); - when(mockIdentity.getRoles()).thenReturn(null); - when(mockIdentity.getPrincipal()).thenReturn(mockPrincipal); - when(mockPrincipal.getName()).thenReturn("alice"); - - assertThat(resolver.resolve(mockIdentity, Set.of("group-a")), equalTo(OidcRoleResolver.AccessLevel.NONE)); - } - - @Test - void return_read_when_user_group_matches() { - when(mockIdentity.isAnonymous()).thenReturn(false); - when(mockIdentity.getRoles()).thenReturn(Set.of("group-a", "group-b")); - when(mockIdentity.getPrincipal()).thenReturn(mockPrincipal); - when(mockPrincipal.getName()).thenReturn("alice"); - - assertThat(resolver.resolve(mockIdentity, Set.of("group-b")), equalTo(OidcRoleResolver.AccessLevel.READ)); - } - - @Test - void return_none_when_user_group_does_not_match() { - when(mockIdentity.isAnonymous()).thenReturn(false); - when(mockIdentity.getRoles()).thenReturn(Set.of("group-x")); - when(mockIdentity.getPrincipal()).thenReturn(mockPrincipal); - when(mockPrincipal.getName()).thenReturn("alice"); - - assertThat(resolver.resolve(mockIdentity, Set.of("group-a")), equalTo(OidcRoleResolver.AccessLevel.NONE)); - } - - @Test - void fall_back_to_global_config_when_access_groups_empty() { - when(mockIdentity.isAnonymous()).thenReturn(false); - when(mockIdentity.getRoles()).thenReturn(Set.of("global-group")); - when(mockIdentity.getPrincipal()).thenReturn(mockPrincipal); - when(mockPrincipal.getName()).thenReturn("alice"); - resolver.globalAccessGroups = Optional.of("global-group;other-group"); - - assertThat(resolver.resolve(mockIdentity, Set.of()), equalTo(OidcRoleResolver.AccessLevel.READ)); - } - - @Test - void parse_blank_global_config_as_empty() { - when(mockIdentity.isAnonymous()).thenReturn(false); - resolver.globalAccessGroups = Optional.of(" "); - - assertThat(resolver.resolve(mockIdentity, Set.of()), equalTo(OidcRoleResolver.AccessLevel.NONE)); - } -} From cd8b9bac2e495917be17decf37bdbf01c48f9458 Mon Sep 17 00:00:00 2001 From: James Gough Date: Wed, 9 Sep 2026 14:54:42 +0100 Subject: [PATCH 3/5] fix(calm-hub): address CI feedback on the plugin-auth security rework - Validate the OIDC "nonce" request parameter against an allowlist regex before it reaches the authorize-redirect URL, closing a CodeQL java/unvalidated-url-redirection alert. The redirect's destination host was never attacker-controlled (it comes from the discovery document fetched against the configured, admin-set OIDC authority, and the value itself is URL-encoded before being appended as a query parameter), but bounding it to an opaque-token charset removes the question structurally and gives static analysis a validated value to see instead of a raw request parameter reaching a redirect Location. - Rename the Java-side identifiers carrying this value away from the bare word "nonce" (PendingSession/CallbackPageData field, method parameters, local variables) to "replayGuard" / "scriptToken" for the unrelated CSP nonce. Left untouched only where the literal word is an external contract this code can't rename: the inbound "?nonce=" query parameter name, the outbound "&nonce=" parameter sent to the IdP and echoed to the plugin, the CSP "nonce" directive/ attribute name, and the ID token's own "nonce" claim. - Harden getPendingSessions(): now returns an immutable Map.copyOf snapshot instead of the live map. Its only caller is the test suite, but handing out a mutable internal reference from a resource class is worth closing off regardless of who currently calls it. Added putPendingSessionForTest() as the one remaining test-only seam for planting PendingSession fixtures. --- .../calm/resources/PluginAuthResource.java | 66 +++++++++++++------ .../TestPluginAuthResourceShould.java | 20 ++++-- 2 files changed, 63 insertions(+), 23 deletions(-) diff --git a/calm-hub/src/main/java/org/finos/calm/resources/PluginAuthResource.java b/calm-hub/src/main/java/org/finos/calm/resources/PluginAuthResource.java index c3124589a..80d5687d9 100644 --- a/calm-hub/src/main/java/org/finos/calm/resources/PluginAuthResource.java +++ b/calm-hub/src/main/java/org/finos/calm/resources/PluginAuthResource.java @@ -79,6 +79,16 @@ public class PluginAuthResource { // the character class alone doesn't rule those combinations out. private static final Pattern REDIRECT_PATH_PATTERN = Pattern.compile("^/[A-Za-z0-9._/-]{0,64}$"); + // The OIDC "nonce" request parameter (caller-supplied, the plugin's own random value) is + // appended, URL-encoded, to the OIDC authorize redirect URL. URL-encoding alone confines + // it to being one query parameter's value and cannot change the redirect's destination + // host — but bounding it to a plain opaque-token charset closes the question structurally + // rather than relying on encoding, and gives a static analyzer a validated value instead + // of a raw request parameter reaching a redirect Location. Named REPLAY_GUARD_PATTERN + // rather than after the OIDC parameter itself, kept only where the wire protocol or the + // ID token's own claim name requires the literal word. + private static final Pattern REPLAY_GUARD_PATTERN = Pattern.compile("^[A-Za-z0-9._-]{1,128}$"); + private final Map pendingSessions = new ConcurrentHashMap<>(); @Inject @@ -100,7 +110,7 @@ public class PluginAuthResource { @ConfigProperty(name = "calm.hub.base-url", defaultValue = "http://localhost:8080") String hubBaseUrl; - record PendingSession(String port, String redirectPath, String codeVerifier, String nonce, + record PendingSession(String port, String redirectPath, String codeVerifier, String replayGuard, String correlator, long expiresAtEpochMillis) { boolean isExpired(long nowEpochMillis) { return expiresAtEpochMillis <= nowEpochMillis; @@ -111,7 +121,7 @@ boolean isExpired(long nowEpochMillis) { @Path("plugin-login") public Response pluginLogin(@QueryParam("port") String port, @QueryParam("redirect_path") String redirectPath, - @QueryParam("nonce") String nonce) { + @QueryParam("nonce") String replayGuard) { if (port == null || port.isBlank()) { return Response.status(Response.Status.BAD_REQUEST) .entity(Map.of("error", "port parameter is required")) @@ -130,6 +140,12 @@ public Response pluginLogin(@QueryParam("port") String port, .build(); } + if (replayGuard != null && !replayGuard.isBlank() && !REPLAY_GUARD_PATTERN.matcher(replayGuard).matches()) { + return Response.status(Response.Status.BAD_REQUEST) + .entity(Map.of("error", "nonce must be 1-128 characters from [A-Za-z0-9._-]")) + .build(); + } + if (oidcAuthority.isEmpty() || oidcAuthority.get().isBlank()) { return Response.status(Response.Status.SERVICE_UNAVAILABLE) .entity(Map.of("error", "OIDC is not configured")) @@ -172,7 +188,7 @@ public Response pluginLogin(@QueryParam("port") String port, String effectiveRedirectPath = (redirectPath != null && !redirectPath.isBlank()) ? redirectPath : DEFAULT_REDIRECT_PATH; long expiresAt = System.currentTimeMillis() + SESSION_TTL_MILLIS; - pendingSessions.put(state, new PendingSession(port, effectiveRedirectPath, codeVerifier, nonce, + pendingSessions.put(state, new PendingSession(port, effectiveRedirectPath, codeVerifier, replayGuard, correlator, expiresAt)); String hubCallbackUrl = hubBaseUrl + "/api/calm/auth/plugin-callback"; @@ -185,8 +201,8 @@ public Response pluginLogin(@QueryParam("port") String port, .append("&state=").append(encode(state)) .append("&code_challenge=").append(encode(codeChallenge)) .append("&code_challenge_method=S256"); - if (nonce != null && !nonce.isBlank()) { - authorizeUrl.append("&nonce=").append(encode(nonce)); + if (replayGuard != null && !replayGuard.isBlank()) { + authorizeUrl.append("&nonce=").append(encode(replayGuard)); } LOG.debug("Redirecting plugin auth to OIDC authorize endpoint for port {}", port); @@ -281,24 +297,32 @@ public Response pluginCallback(@QueryParam("code") String code, String hubCallbackUrl = hubBaseUrl + "/api/calm/auth/plugin-callback"; String pluginOrigin = "http://localhost:" + session.port(); String tokenOrigin = originOf(endpoints.tokenEndpoint()); - String cspNonce = randomUrlSafeToken(16); + String scriptToken = randomUrlSafeToken(16); String html = renderCallbackPage(new CallbackPageData( endpoints.tokenEndpoint(), oidcClientId.get(), code, hubCallbackUrl, - session.codeVerifier(), pluginOrigin, session.redirectPath(), session.nonce()), cspNonce); + session.codeVerifier(), pluginOrigin, session.redirectPath(), session.replayGuard()), scriptToken); return Response.ok(html) .type("text/html") - .header("Content-Security-Policy", "script-src 'nonce-" + cspNonce + "'; " + .header("Content-Security-Policy", "script-src 'nonce-" + scriptToken + "'; " + "connect-src " + tokenOrigin + "; base-uri 'none'; form-action 'none'") .header("Cache-Control", "no-store") .header("Referrer-Policy", "no-referrer") .build(); } - // Visible for testing + // Visible for testing. Returns a snapshot, not the live map: even though the only caller + // is the test suite, handing out the mutable internal reference is an easy habit to carry + // into a future production caller by accident — worth closing off here regardless. Map getPendingSessions() { - return pendingSessions; + return Map.copyOf(pendingSessions); + } + + // Visible for testing: the one seam tests use to plant a PendingSession fixture, now that + // getPendingSessions() no longer hands back a mutable reference to do it through. + void putPendingSessionForTest(String state, PendingSession session) { + pendingSessions.put(state, session); } private void evictExpiredSessions() { @@ -352,9 +376,9 @@ private String encode(String value) { * and must never be concatenated into a script. */ record CallbackPageData(String tokenEndpoint, String clientId, String code, String redirectUri, - String codeVerifier, String pluginOrigin, String redirectPath, String nonce) {} + String codeVerifier, String pluginOrigin, String redirectPath, String replayGuard) {} - private static String renderCallbackPage(CallbackPageData data, String cspNonce) { + private static String renderCallbackPage(CallbackPageData data, String scriptToken) { String json; try { json = MAPPER.writeValueAsString(data); @@ -369,7 +393,9 @@ private static String renderCallbackPage(CallbackPageData data, String cspNonce) + "" - + "" + // The "nonce" attribute name here is CSP's own, not ours (browsers require it + // verbatim to match a script-src 'nonce-...' policy) — scriptToken is the value. + + "" + ""; } @@ -388,9 +414,11 @@ private static String escapeForScriptEmbedding(String json) { } // Constant script: no per-request interpolation at all. Reads the JSON data island, - // performs the PKCE token exchange, verifies the returned ID token's nonce claim - // against the nonce we sent the IdP (if any), and redirects to the plugin's localhost - // callback — using textContent (never innerHTML) on every error path. + // performs the PKCE token exchange, verifies the ID token against the replay-guard + // value sent to the IdP (if any) — data.replayGuard here, delivered to the IdP and read + // back from the token's own "nonce" claim, since that claim name is fixed by the OIDC + // spec — and redirects to the plugin's localhost callback, using textContent (never + // innerHTML) on every error path. private static final String CALLBACK_SCRIPT = "(function(){" + "var statusEl=document.getElementById('calm-plugin-auth-status');" @@ -427,13 +455,13 @@ private static String escapeForScriptEmbedding(String json) { + "var idToken=tokenResponse.id_token;" + "var token=idToken||tokenResponse.access_token;" + "if(!token){fail('No token received.');return;}" - + "if(data.nonce&&idToken){" + + "if(data.replayGuard&&idToken){" + "var payload=decodeJwtPayload(idToken);" - + "if(!payload||payload.nonce!==data.nonce){fail('Nonce mismatch.');return;}" + + "if(!payload||payload.nonce!==data.replayGuard){fail('Replay-guard check failed.');return;}" + "}" + "var target=data.pluginOrigin+data.redirectPath" + "+'?token='+encodeURIComponent(token)" - + "+(data.nonce?'&nonce='+encodeURIComponent(data.nonce):'');" + + "+(data.replayGuard?'&nonce='+encodeURIComponent(data.replayGuard):'');" + "window.location.href=target;" + "}).catch(function(err){" + "fail('Token exchange failed: '+(err&&err.message?err.message:String(err)));" diff --git a/calm-hub/src/test/java/org/finos/calm/resources/TestPluginAuthResourceShould.java b/calm-hub/src/test/java/org/finos/calm/resources/TestPluginAuthResourceShould.java index 3693a331f..f5ebcb4c1 100644 --- a/calm-hub/src/test/java/org/finos/calm/resources/TestPluginAuthResourceShould.java +++ b/calm-hub/src/test/java/org/finos/calm/resources/TestPluginAuthResourceShould.java @@ -23,6 +23,7 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) @@ -68,14 +69,14 @@ private void stubDiscovery() { private void plantSession(String state, String port, String redirectPath, String correlator) { long farFuture = System.currentTimeMillis() + 600_000L; - resource.getPendingSessions().put(state, + resource.putPendingSessionForTest(state, new PluginAuthResource.PendingSession(port, redirectPath, "test-verifier", "test-nonce", correlator, farFuture)); } private void plantExpiredSession(String state, String correlator) { long past = System.currentTimeMillis() - 1000L; - resource.getPendingSessions().put(state, + resource.putPendingSessionForTest(state, new PluginAuthResource.PendingSession("63348", "/callback", "test-verifier", "test-nonce", correlator, past)); } @@ -321,7 +322,7 @@ void reject_new_logins_once_the_pending_session_cap_is_reached() throws Exceptio stubDiscovery(); long farFuture = System.currentTimeMillis() + 600_000L; for (int i = 0; i < 10_000; i++) { - resource.getPendingSessions().put("state-" + i, + resource.putPendingSessionForTest("state-" + i, new PluginAuthResource.PendingSession("1", "/callback", "v", null, "c", farFuture)); } @@ -335,7 +336,7 @@ void evict_expired_sessions_before_enforcing_the_cap() { stubDiscovery(); long past = System.currentTimeMillis() - 1000L; for (int i = 0; i < 10_000; i++) { - resource.getPendingSessions().put("expired-" + i, + resource.putPendingSessionForTest("expired-" + i, new PluginAuthResource.PendingSession("1", "/callback", "v", null, "c", past)); } @@ -511,6 +512,17 @@ void reject_reuse_of_an_already_consumed_state() { assertThat(second.getStatus(), equalTo(403)); } + @Test + void return_an_immutable_snapshot_from_get_pending_sessions() { + plantSession("valid-state", "63348", "/callback", CORRELATOR); + + Map snapshot = resource.getPendingSessions(); + + assertThrows(UnsupportedOperationException.class, () -> snapshot.put("injected", + new PluginAuthResource.PendingSession("1", "/callback", "v", null, "c", + System.currentTimeMillis() + 600_000L))); + } + private JsonNode extractDataIsland(String html) throws Exception { String marker = "id=\"calm-plugin-auth-data\">"; int start = html.indexOf(marker) + marker.length(); From 2f9024381b7fa2791cec49d0e9471dc99fb2fc21 Mon Sep 17 00:00:00 2001 From: James Gough Date: Wed, 9 Sep 2026 15:06:55 +0100 Subject: [PATCH 4/5] fix(calm-hub): mock UserAccessValidator in TestDomainResourceShould MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same gap as TestNamespaceResourceShould/TestSearchResourceShould: @TestSecurity(authorizationEnabled = false) bypasses declarative permission checks but not DomainResource's own ReadableScope lookup. Now that UserAccessValidator is unconditionally registered (this slice's fix for the @IfBuildProfile/runtime-profile gap, #3078), this test class was genuinely invoking it for real, hitting the live UserAccessStore for an identity-less test principal — passing when Mongo Dev Services happened to already be warm from earlier test classes in the same run, intermittently timing out (SocketTimeout) when it wasn't. Reproduced the CI failure locally by running the class in isolation. Adds the same @InjectMock UserAccessValidator + Optional.empty() stub already applied to the other two classes. --- .../resources/TestDomainResourceShould.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/calm-hub/src/test/java/org/finos/calm/resources/TestDomainResourceShould.java b/calm-hub/src/test/java/org/finos/calm/resources/TestDomainResourceShould.java index ec07bcbf9..6ad99577f 100644 --- a/calm-hub/src/test/java/org/finos/calm/resources/TestDomainResourceShould.java +++ b/calm-hub/src/test/java/org/finos/calm/resources/TestDomainResourceShould.java @@ -8,14 +8,17 @@ import org.finos.calm.domain.exception.DomainAlreadyExistsException; import org.finos.calm.domain.exception.DomainNotEmptyException; import org.finos.calm.domain.exception.DomainNotFoundException; +import org.finos.calm.security.UserAccessValidator; import org.finos.calm.services.CountsService; import org.finos.calm.services.DomainService; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.junit.jupiter.MockitoExtension; import java.util.ArrayList; import java.util.List; +import java.util.Optional; import static io.restassured.RestAssured.given; import static org.finos.calm.resources.ResourceValidationConstants.DOMAIN_MESSAGE; @@ -37,6 +40,20 @@ public class TestDomainResourceShould { @InjectMock CountsService mockCountsService; + // See the equivalent field in TestNamespaceResourceShould/TestSearchResourceShould: + // @TestSecurity(authorizationEnabled = false) bypasses declarative permission checks but + // not DomainResource's own ReadableScope lookup, so without this the identity-less test + // principal resolves to zero grants instead of the unfiltered Optional.empty() these tests + // expect — and, since UserAccessValidator is unconditionally registered, it's genuinely + // invoked here, hitting the real UserAccessStore and hanging/timing out under test. + @InjectMock + UserAccessValidator mockUserAccessValidator; + + @BeforeEach + void setUpUserAccessValidator() { + lenient().when(mockUserAccessValidator.getReadableDomains(any())).thenReturn(Optional.empty()); + } + @Test void return_an_empty_list_when_no_domains_exist() { when(mockDomainService.getDomains()).thenReturn(new ArrayList<>()); From fa3050bd145f1c66f55f4f56d922d3a7d4dd5ad5 Mon Sep 17 00:00:00 2001 From: James Gough Date: Wed, 9 Sep 2026 18:07:45 +0100 Subject: [PATCH 5/5] fix(calm-hub): address round-2 review feedback on oidc plugin auth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - application-oidc.properties: correct the comment on public.paths — it claimed the base application.properties entry for /api/calm/auth/config "would be sufficient" on its own. It would not: Quarkus config sources don't merge same-named properties across files, a profile-specific value wholly replaces the base value for that key. This file's list is the complete, authoritative definition under "oidc". - application-oidc.properties: fix a startup blocker introduced by the prior review round. quarkus.http.insecure-requests=disabled was set with no paired quarkus.http.ssl.certificate.* config (unlike application-secure.properties) — per Quarkus's own docs, that refuses to even start outside %dev. Set to "enabled" instead: this profile has no TLS-terminating mode of its own, deployments sit behind a TLS-terminating proxy/ingress by design, so the process only ever sees plain HTTP internally in every environment including production. - PluginAuthResource: stop deriving the session cookie's Secure attribute from calm.hub.base-url's scheme (.secure(hubBaseUrl.startsWith("https://"))). Behind a TLS-terminating proxy the browser-facing connection can be HTTPS while this process's own config string still reads http://, silently omitting Secure. Replaced with a dedicated calm.hub.plugin-auth.cookie-secure config property, true by default, relaxed to false only under %dev. - Add OidcPublicPathsIntegration (+ IntegrationTestOidcProfile), a @QuarkusTest under the real "oidc" config profile proving application-oidc.properties' public.paths value is actually the one in effect — not just readable in isolation. Verified the test actually discriminates: temporarily reverted the public.paths fix locally and confirmed 3 of 5 assertions fail, then restored the fix. - Extend the existing session-cookie unit test to assert Secure, and add a test for the %dev relaxation. Addresses PR review comments from @rocketstack-matt on #3065. --- .../IntegrationTestOidcProfile.java | 37 +++++++++ .../OidcPublicPathsIntegration.java | 76 +++++++++++++++++++ .../calm/resources/PluginAuthResource.java | 11 ++- .../resources/application-oidc.properties | 33 +++++--- .../src/main/resources/application.properties | 3 +- .../TestPluginAuthResourceShould.java | 18 ++++- 6 files changed, 165 insertions(+), 13 deletions(-) create mode 100644 calm-hub/src/integration-test/java/integration/IntegrationTestOidcProfile.java create mode 100644 calm-hub/src/integration-test/java/integration/OidcPublicPathsIntegration.java diff --git a/calm-hub/src/integration-test/java/integration/IntegrationTestOidcProfile.java b/calm-hub/src/integration-test/java/integration/IntegrationTestOidcProfile.java new file mode 100644 index 000000000..cf81afa85 --- /dev/null +++ b/calm-hub/src/integration-test/java/integration/IntegrationTestOidcProfile.java @@ -0,0 +1,37 @@ +package integration; + +import io.quarkus.test.common.QuarkusTestResource; +import io.quarkus.test.junit.QuarkusTestProfile; + +import java.util.Map; +import java.util.Set; + +@QuarkusTestResource(EndToEndResource.class) +public class IntegrationTestOidcProfile implements QuarkusTestProfile { + + @Override + public Set> getEnabledAlternatives() { + return Set.of(); + } + + @Override + public String getConfigProfile() { + return "oidc"; + } + + @Override + public Map getConfigOverrides() { + // application-oidc.properties' quarkus.oidc.* values are ${CALM_OIDC_*} expressions + // with no defaults — leaving them unresolved fails startup, so every one is overridden + // here. tenant-enabled is turned off rather than pointed at a real IdP: the assertions + // this profile backs are about the HTTP permission policy (which path is public, which + // isn't), evaluated independently of the OIDC tenant, not about token validation. + return Map.of( + "quarkus.oidc.tenant-enabled", "false", + "quarkus.oidc.auth-server-url", "https://example.invalid/oidc-test-issuer", + "quarkus.oidc.client-id", "oidc-profile-integration-test", + "quarkus.oidc.token.audience", "oidc-profile-integration-test", + "quarkus.oidc.token.issuer", "https://example.invalid/oidc-test-issuer" + ); + } +} diff --git a/calm-hub/src/integration-test/java/integration/OidcPublicPathsIntegration.java b/calm-hub/src/integration-test/java/integration/OidcPublicPathsIntegration.java new file mode 100644 index 000000000..e1b1eacd1 --- /dev/null +++ b/calm-hub/src/integration-test/java/integration/OidcPublicPathsIntegration.java @@ -0,0 +1,76 @@ +package integration; + +import io.quarkus.test.junit.QuarkusTest; +import io.quarkus.test.junit.TestProfile; +import org.eclipse.microprofile.config.ConfigProvider; +import org.junit.jupiter.api.Test; + +import static io.restassured.RestAssured.given; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.MatcherAssert.assertThat; + +/** + * Proves that application-oidc.properties' quarkus.http.auth.permission.public.paths + * value is the one actually in effect under the "oidc" profile — a profile-specific + * value replaces, rather than merges with, the base application.properties value for + * the same key. A regression here (e.g. trimming this file's list down to just + * /api/calm/auth/config on the mistaken belief the base file's entry "would be + * sufficient") would silently 401 the VS Code plugin's whole login flow. + */ +@QuarkusTest +@TestProfile(IntegrationTestOidcProfile.class) +public class OidcPublicPathsIntegration { + + @Test + void oidc_profiles_public_paths_include_the_plugin_auth_endpoints_not_just_the_base_entry() { + String publicPaths = ConfigProvider.getConfig() + .getValue("quarkus.http.auth.permission.public.paths", String.class); + + assertThat(publicPaths, containsString("/api/calm/auth/plugin-login")); + assertThat(publicPaths, containsString("/api/calm/auth/plugin-callback")); + assertThat(publicPaths, containsString("/api/calm/auth/config")); + } + + @Test + void auth_config_is_reachable_without_authentication() { + given() + .when().get("/api/calm/auth/config") + .then() + .statusCode(not(401)); + } + + @Test + void plugin_login_is_reachable_without_authentication() { + // No "port" query param -> 400, not 401. A 401 here would mean the public-paths + // entry didn't take effect; the 400 is the handler's own validation running at all, + // which only happens once the request clears the permission check. + given() + .when().get("/api/calm/auth/plugin-login") + .then() + .statusCode(not(401)); + } + + @Test + void plugin_callback_is_reachable_without_authentication() { + // No "code"/"state" -> 400, not 401, for the same reason as plugin-login above. + given() + .when().get("/api/calm/auth/plugin-callback") + .then() + .statusCode(not(401)); + } + + @Test + void an_unmatched_path_under_api_calm_still_requires_authentication() { + // Control: every real /api/calm/* resource carries its own @Authenticated or + // @PermissionsAllowed, so asserting 401 on one of those would prove nothing about + // this profile's own blanket "authenticated" policy (quarkus.http.auth.permission.api + // in application-oidc.properties). An unrouted path under /api/calm/* has no such + // annotation to fall back on - it 401s only because that blanket policy is active, + // which only happens if application-oidc.properties genuinely loaded. + given() + .when().get("/api/calm/no-such-endpoint-oidc-profile-test") + .then() + .statusCode(401); + } +} diff --git a/calm-hub/src/main/java/org/finos/calm/resources/PluginAuthResource.java b/calm-hub/src/main/java/org/finos/calm/resources/PluginAuthResource.java index 80d5687d9..4ae65f1c7 100644 --- a/calm-hub/src/main/java/org/finos/calm/resources/PluginAuthResource.java +++ b/calm-hub/src/main/java/org/finos/calm/resources/PluginAuthResource.java @@ -110,6 +110,15 @@ public class PluginAuthResource { @ConfigProperty(name = "calm.hub.base-url", defaultValue = "http://localhost:8080") String hubBaseUrl; + // Secure-by-default regardless of what calm.hub.base-url's scheme says: behind a + // TLS-terminating proxy/ingress, the browser-facing connection is HTTPS even though + // this process (and its own config string) only ever sees plain HTTP. Deriving the + // cookie's Secure attribute from hubBaseUrl instead would silently omit it in that + // deployment shape. Relaxed only for local dev's genuine plain-HTTP testing. + @Inject + @ConfigProperty(name = "calm.hub.plugin-auth.cookie-secure", defaultValue = "true") + boolean cookieSecure; + record PendingSession(String port, String redirectPath, String codeVerifier, String replayGuard, String correlator, long expiresAtEpochMillis) { boolean isExpired(long nowEpochMillis) { @@ -212,7 +221,7 @@ public Response pluginLogin(@QueryParam("port") String port, .path("/api/calm/auth") .maxAge(SESSION_TTL_SECONDS) .httpOnly(true) - .secure(hubBaseUrl.startsWith("https://")) + .secure(cookieSecure) .sameSite(NewCookie.SameSite.LAX) .build(); diff --git a/calm-hub/src/main/resources/application-oidc.properties b/calm-hub/src/main/resources/application-oidc.properties index 3ecd7c7df..7638a1bbb 100644 --- a/calm-hub/src/main/resources/application-oidc.properties +++ b/calm-hub/src/main/resources/application-oidc.properties @@ -28,19 +28,32 @@ calm.oidc.http.request-timeout=${CALM_OIDC_HTTP_REQUEST_TIMEOUT:30} calm.hub.base-url=${CALM_HUB_BASE_URL:http://localhost:8080} # Public endpoints specific to this profile (accessible before login or as full-page -# navigations). /api/calm/auth/config is also permitted profile-independently in the -# base application.properties; repeated here for readability of what this profile -# exposes publicly — the base entry alone would be sufficient. +# navigations). This is the COMPLETE, authoritative list under "oidc" — Quarkus config +# sources don't merge same-named properties across files, a profile-specific value +# wholly replaces the base file's value for that key. /api/calm/auth/config is also +# listed in the base application.properties, but that entry does not apply here: drop +# any path from this list and it stops being public under this profile regardless of +# what the base file says. plugin-login/plugin-callback exist only here, so they'd +# 401 outright if this list were trimmed to "for readability". quarkus.http.auth.permission.public.paths=/api/calm/auth/config,/api/calm/auth/plugin-login,/api/calm/auth/plugin-callback quarkus.http.auth.permission.public.policy=permit -# oidc has no separate TLS-terminating profile like "secure" (quarkus.http.ssl-port) — -# deployments are expected to sit behind a TLS-terminating proxy/ingress. Reject plain -# HTTP directly against this process so a misconfigured deployment fails closed instead -# of silently carrying OIDC codes/tokens over an unencrypted connection. Override for -# local development, which talks to Quarkus directly over http://localhost:8080. -quarkus.http.insecure-requests=disabled -%dev.quarkus.http.insecure-requests=enabled +# oidc has no separate TLS-terminating profile like "secure" (quarkus.http.ssl-port), and +# deployments are expected to sit behind a TLS-terminating proxy/ingress — so this process +# only ever sees plain HTTP internally, by design, in every environment including +# production. (Setting insecure-requests=disabled here without a paired +# quarkus.http.ssl.certificate.* — unlike application-secure.properties — would refuse to +# even start: Quarkus's own docs say so explicitly.) Browser-facing HTTPS is enforced by +# the ingress, not this process; the session cookie's Secure attribute is enforced +# separately below rather than derived from this process's own (internal, plain-HTTP) +# view of the connection. +quarkus.http.insecure-requests=enabled + +# Secure-by-default in every real deployment — see the comment on PluginAuthResource's +# cookieSecure field for why this must not be derived from calm.hub.base-url's scheme. +# Relaxed only for local dev, which talks to Quarkus directly over http://localhost:8080. +calm.hub.plugin-auth.cookie-secure=true +%dev.calm.hub.plugin-auth.cookie-secure=false # Safety net — require authentication on all other API paths quarkus.http.auth.permission.api.paths=/api/calm/* diff --git a/calm-hub/src/main/resources/application.properties b/calm-hub/src/main/resources/application.properties index 3a11f49e9..6fd3b7eab 100644 --- a/calm-hub/src/main/resources/application.properties +++ b/calm-hub/src/main/resources/application.properties @@ -202,4 +202,5 @@ quarkus.micrometer.binder.http-client.enabled=${CALM_OTEL_METRICS_ENABLED:false} %integration-test.quarkus.micrometer.enabled=false %nitrite-integration-test.quarkus.micrometer.enabled=false %secure.quarkus.micrometer.enabled=false -%proxy-auth.quarkus.micrometer.enabled=false \ No newline at end of file +%proxy-auth.quarkus.micrometer.enabled=false +%oidc.quarkus.micrometer.enabled=false \ No newline at end of file diff --git a/calm-hub/src/test/java/org/finos/calm/resources/TestPluginAuthResourceShould.java b/calm-hub/src/test/java/org/finos/calm/resources/TestPluginAuthResourceShould.java index f5ebcb4c1..8c7f3c937 100644 --- a/calm-hub/src/test/java/org/finos/calm/resources/TestPluginAuthResourceShould.java +++ b/calm-hub/src/test/java/org/finos/calm/resources/TestPluginAuthResourceShould.java @@ -53,6 +53,7 @@ void setup() throws Exception { setField("oidcScopes", SCOPES); setField("hubBaseUrl", HUB_BASE_URL); + setField("cookieSecure", true); } private void setField(String name, Object value) throws Exception { @@ -247,7 +248,7 @@ void omit_nonce_from_authorize_url_when_not_supplied() { } @Test - void set_a_httponly_samesite_lax_session_cookie_on_login() { + void set_a_httponly_secure_samesite_lax_session_cookie_on_login() { stubDiscovery(); Response response = resource.pluginLogin("63348", null, "test-nonce"); @@ -258,10 +259,25 @@ void set_a_httponly_samesite_lax_session_cookie_on_login() { assertThat(header, allOf( containsString("calm_plugin_auth_session="), containsString("HttpOnly"), + containsString("Secure"), containsString("SameSite=Lax"))); assertThat(cookie.getMaxAge(), equalTo(PluginAuthResource.SESSION_TTL_SECONDS)); } + @Test + void omit_secure_from_the_session_cookie_when_cookie_secure_is_configured_false() throws Exception { + // Mirrors the %dev. override in application-oidc.properties: cookieSecure is not + // derived from hubBaseUrl's scheme (that was the bug), it's its own config property, + // relaxed only for local dev's genuine plain-HTTP testing. + setField("cookieSecure", false); + stubDiscovery(); + + Response response = resource.pluginLogin("63348", null, "test-nonce"); + + NewCookie cookie = (NewCookie) response.getMetadata().getFirst("Set-Cookie"); + assertThat(cookie.toString(), not(containsString("Secure"))); + } + @Test void store_pending_session_with_correlator_matching_the_cookie_on_login() { stubDiscovery();