From e16cfdd2c489ccdb8136477439be19129219fd39 Mon Sep 17 00:00:00 2001 From: Akanksha Kedia Date: Wed, 8 Jul 2026 12:59:38 +0530 Subject: [PATCH 01/10] Add session-based UI authentication with proper server-side logout invalidation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements a Trino/Ambari-style session layer for the Pinot controller UI that works on top of any configured AccessControlFactory (BasicAuth, ZkBasicAuth, LDAP, or custom). **Security problems fixed:** - Credentials were visible in every browser network tab request (Authorization header) - Logout only cleared localStorage — server session survived indefinitely - No inactivity timeout; sessions never expired - Credentials stored in JS memory (XSS-readable) - Broker auth forwarded directly from the browser - Race condition in session lookup (non-atomic get→check→remove) - No CSRF protection on session cookie **How it works:** 1. `POST /auth/login` validates credentials via the configured AccessControl, creates a server-side session, and sets an HttpOnly + SameSite=Strict cookie 2. `GET /auth/logout` immediately removes the session from the server store (proper invalidation) 3. `GET /auth/session` checks session validity for page-refresh restoration 4. `SessionAuthenticationFilter` (AUTHENTICATION-10 priority) validates the cookie before `AuthenticationFilter` runs; passes through requests with Authorization header for API compat 5. `PinotQueryResource` injects stored Basic-auth token server-side when forwarding to broker **New files:** - `SessionManager.java`: ConcurrentHashMap session store with sliding TTL, atomic compute() - `SessionAuthenticationFilter.java`: JAX-RS filter for cookie-based auth - `SessionBasicAuthAccessControlFactory.java`: convenience wrapper factory - `PinotSessionLoginResource.java`: /auth/login, /auth/logout, /auth/session endpoints - `SessionManagerTest.java`: unit + 50-thread concurrency test **Configuration (all default-off for backward compatibility):** ```properties controller.ui.session.authentication.enabled=true controller.ui.session.cookie.secure=true # set false for HTTP-only dev controller.ui.session.inactivity.timeout.seconds=300 # default 5 min ``` **Frontend changes:** - `Models.ts`: SESSION enum added to AuthWorkflow - `axios-config.ts`: SESSION workflow deletes Authorization header (cookie sent automatically) - `AuthProvider.tsx`: reads timeout from /auth/info, checks /auth/session on page refresh - `App.tsx`: inactivity timer with 60-second warning dialog before auto-logout - `Header.tsx`: real logout button that calls /auth/logout server-side - `LoginPage.tsx`: SESSION path POSTs form data to /auth/login (no Basic header in JS) --- .../controller/BaseControllerStarter.java | 11 + .../pinot/controller/ControllerConf.java | 42 ++ .../api/ControllerAdminApiApplication.java | 18 +- .../controller/api/access/AccessControl.java | 23 +- .../api/access/AuthenticationFilter.java | 29 +- .../access/SessionAuthenticationFilter.java | 179 +++++++++ .../SessionBasicAuthAccessControlFactory.java | 107 +++++ .../controller/api/access/SessionManager.java | 273 +++++++++++++ .../PinotControllerAuthResource.java | 22 +- .../api/resources/PinotQueryResource.java | 90 ++++- .../resources/PinotSessionLoginResource.java | 375 ++++++++++++++++++ .../src/main/resources/app/App.tsx | 106 ++++- .../src/main/resources/app/Models.ts | 7 + .../main/resources/app/components/Header.tsx | 47 ++- .../app/components/auth/AuthProvider.tsx | 32 +- .../main/resources/app/pages/LoginPage.tsx | 50 ++- .../main/resources/app/utils/axios-config.ts | 53 ++- .../api/access/SessionManagerTest.java | 363 +++++++++++++++++ 18 files changed, 1783 insertions(+), 44 deletions(-) create mode 100644 pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionAuthenticationFilter.java create mode 100644 pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionBasicAuthAccessControlFactory.java create mode 100644 pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionManager.java create mode 100644 pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSessionLoginResource.java create mode 100644 pinot-controller/src/test/java/org/apache/pinot/controller/api/access/SessionManagerTest.java diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java b/pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java index 7865e9dbe9b4..9908334b69e6 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java @@ -97,6 +97,7 @@ import org.apache.pinot.common.version.PinotVersion; import org.apache.pinot.controller.api.ControllerAdminApiApplication; import org.apache.pinot.controller.api.access.AccessControlFactory; +import org.apache.pinot.controller.api.access.SessionManager; import org.apache.pinot.controller.api.events.MetadataEventNotifierFactory; import org.apache.pinot.controller.api.resources.ControllerFilePathProvider; import org.apache.pinot.controller.api.resources.InvalidControllerConfigException; @@ -746,6 +747,15 @@ private void setUpPinotController() { final MetadataEventNotifierFactory metadataEventNotifierFactory = MetadataEventNotifierFactory.loadFactory(_config.subset(METADATA_EVENT_NOTIFIER_PREFIX), _helixResourceManager); + // Create a singleton SessionManager for session-based UI authentication. + // Server-side TTL = inactivity timeout + 120s buffer so the server doesn't evict a session + // that the UI hasn't timed out yet (e.g. user is active but a background API call races with eviction). + long uiInactivityTimeoutSeconds = _config.getProperty( + ControllerConf.CONTROLLER_UI_SESSION_INACTIVITY_TIMEOUT_SECONDS, + ControllerConf.DEFAULT_UI_SESSION_INACTIVITY_TIMEOUT_SECONDS); + long serverSessionTtlSeconds = uiInactivityTimeoutSeconds + 120; + final SessionManager sessionManager = new SessionManager(serverSessionTtlSeconds); + LOGGER.info("Controller download url base: {}", _config.generateVipUrl()); LOGGER.info("Injecting configuration and resource managers to the API context"); // register all the controller objects for injection to jersey resources @@ -775,6 +785,7 @@ protected void configure() { bind(_storageQuotaChecker).to(StorageQuotaChecker.class); bind(_resourceUtilizationManager).to(ResourceUtilizationManager.class); bind(controllerStartTime).named(ControllerAdminApiApplication.START_TIME); + bind(sessionManager).to(SessionManager.class); bindAsContract(PinotTableReloadService.class).in(Singleton.class); bindAsContract(PinotTableReloadStatusReporter.class).in(Singleton.class); diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java b/pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java index ead5eb035479..370ae0126c7a 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java @@ -357,6 +357,48 @@ public static long getRandomInitialDelayInSeconds() { public static final String INGEST_FROM_URI_ALLOW_LOCAL_FILE_SYSTEM = "controller.ingestFromURI.allowLocalFileSystem"; public static final String ACCESS_CONTROL_FACTORY_CLASS = "controller.admin.access.control.factory.class"; + + /** + * When set to {@code true}, the Pinot UI uses session-based authentication (Trino/Ambari-style). + * + *

This is a UI-layer setting that is independent of the configured + * {@code controller.admin.access.control.factory.class}. It works with any auth backend: + * BasicAuth, ZkBasicAuth, LDAP/PasswordAuth, or custom factories. + * + *

When enabled: + *

+ * + *

Default: {@code false} (backward compatible – existing BASIC/NONE/OIDC workflows unchanged) + */ + public static final String CONTROLLER_UI_SESSION_ENABLED = "controller.ui.session.authentication.enabled"; + + /** + * When set to {@code true}, the HttpOnly session cookie is flagged as {@code Secure}, meaning the + * browser will only send it over HTTPS connections. Set to {@code false} only for local HTTP dev. + * + *

Default: {@code true} + */ + public static final String CONTROLLER_UI_SESSION_COOKIE_SECURE = "controller.ui.session.cookie.secure"; + + /** + * Inactivity timeout in seconds for the Pinot UI session. + * + *

After this many seconds of user inactivity, the UI shows a 60-second warning dialog and then + * calls {@code GET /auth/logout} to invalidate the server-side session. This value is returned to + * the browser via {@code GET /auth/info} so both the UI timer and server-side TTL derive from one config. + * Server-side session TTL is set automatically to this value + 120s buffer. + * + *

Default: {@code 300} (5 minutes) + */ + public static final String CONTROLLER_UI_SESSION_INACTIVITY_TIMEOUT_SECONDS = + "controller.ui.session.inactivity.timeout.seconds"; + public static final long DEFAULT_UI_SESSION_INACTIVITY_TIMEOUT_SECONDS = 300L; + public static final String ACCESS_CONTROL_USERNAME = "access.control.init.username"; public static final String ACCESS_CONTROL_PASSWORD = "access.control.init.password"; public static final String LINEAGE_MANAGER_CLASS = "controller.lineage.manager.class"; diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/ControllerAdminApiApplication.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/ControllerAdminApiApplication.java index 55f1fa2b1aa0..fd527d67bc8f 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/ControllerAdminApiApplication.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/ControllerAdminApiApplication.java @@ -39,6 +39,7 @@ import org.apache.pinot.common.swagger.SwaggerSetupUtils; import org.apache.pinot.controller.ControllerConf; import org.apache.pinot.controller.api.access.AuthenticationFilter; +import org.apache.pinot.controller.api.access.SessionAuthenticationFilter; import org.apache.pinot.core.api.ServiceAutoDiscoveryFeature; import org.apache.pinot.core.transport.ListenerConfig; import org.apache.pinot.core.util.ListenerConfigUtil; @@ -90,6 +91,9 @@ public ControllerAdminApiApplication(ControllerConf conf) { register(SwaggerSerializers.class); register(new CorsFilter()); register(AuthenticationFilter.class); + // Register session authentication filter (priority AUTHENTICATION-10, runs before AuthenticationFilter). + // Validates the HttpOnly session cookie for SESSION workflow. No-op when session mode is disabled. + register(SessionAuthenticationFilter.class); register(AuditLogFilter.class); _managedAsyncExecutor = createManagedAsyncExecutor(); register(new ManagedAsyncExecutorServiceProvider(_managedAsyncExecutor)); @@ -145,9 +149,19 @@ private class CorsFilter implements ContainerResponseFilter { public void filter(ContainerRequestContext containerRequestContext, ContainerResponseContext containerResponseContext) throws IOException { - containerResponseContext.getHeaders().add("Access-Control-Allow-Origin", "*"); + // For session-based auth with withCredentials:true, reflect the actual request origin + // instead of "*". Browsers reject "Access-Control-Allow-Origin: *" when + // "Access-Control-Allow-Credentials: true" is set. + String origin = containerRequestContext.getHeaderString("Origin"); + if (origin != null && !origin.isEmpty()) { + containerResponseContext.getHeaders().add("Access-Control-Allow-Origin", origin); + containerResponseContext.getHeaders().add("Access-Control-Allow-Credentials", "true"); + } else { + containerResponseContext.getHeaders().add("Access-Control-Allow-Origin", "*"); + } containerResponseContext.getHeaders().add("Access-Control-Allow-Methods", "GET, POST, PUT, OPTIONS, DELETE"); - containerResponseContext.getHeaders().add("Access-Control-Allow-Headers", "*"); + containerResponseContext.getHeaders() + .add("Access-Control-Allow-Headers", "Authorization, Content-Type, Accept, Origin, X-Requested-With"); if (containerRequestContext.getMethod().equals("OPTIONS")) { containerResponseContext.setStatus(HttpServletResponse.SC_OK); } diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java index cf2240d1d534..da74c49b71d3 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java @@ -30,6 +30,8 @@ public interface AccessControl extends FineGrainedAccessControl { String WORKFLOW_NONE = "NONE"; String WORKFLOW_BASIC = "BASIC"; + /** Session-based workflow: credentials validated once at login; subsequent requests use an HttpOnly cookie. */ + String WORKFLOW_SESSION = "SESSION"; /** * Return whether the client has permission to the given table @@ -78,16 +80,24 @@ default AuthWorkflowInfo getAuthWorkflowInfo() { /** * Container for authentication workflow info for the Pinot UI. May be extended by implementations. * - * Auth workflow info hold any configuration necessary to execute a UI workflow. We currently foresee supporting NONE - * (auth disabled) and BASIC (basic auth with username and password) + * Auth workflow info holds any configuration necessary to execute a UI workflow. Supports NONE + * (auth disabled), BASIC (basic auth with username and password), and SESSION (session-based + * UI authentication with HttpOnly cookie). */ class AuthWorkflowInfo { String _workflow; + /** UI inactivity timeout in seconds. -1 means not applicable (non-SESSION workflows). */ + long _inactivityTimeoutSeconds = -1; public AuthWorkflowInfo(String workflow) { _workflow = workflow; } + public AuthWorkflowInfo(String workflow, long inactivityTimeoutSeconds) { + _workflow = workflow; + _inactivityTimeoutSeconds = inactivityTimeoutSeconds; + } + public String getWorkflow() { return _workflow; } @@ -95,5 +105,14 @@ public String getWorkflow() { public void setWorkflow(String workflow) { _workflow = workflow; } + + /** Returns the UI inactivity timeout in seconds, or -1 if not applicable. */ + public long getInactivityTimeoutSeconds() { + return _inactivityTimeoutSeconds; + } + + public void setInactivityTimeoutSeconds(long inactivityTimeoutSeconds) { + _inactivityTimeoutSeconds = inactivityTimeoutSeconds; + } } } diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java index d99bce491c9f..1fad3441e44a 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java @@ -24,6 +24,7 @@ import java.lang.reflect.Method; import java.util.Arrays; import java.util.HashSet; +import java.util.Optional; import java.util.Set; import javax.inject.Inject; import javax.inject.Provider; @@ -34,11 +35,13 @@ import javax.ws.rs.container.ContainerRequestFilter; import javax.ws.rs.container.ResourceInfo; import javax.ws.rs.core.Context; +import javax.ws.rs.core.Cookie; import javax.ws.rs.core.HttpHeaders; import javax.ws.rs.core.MultivaluedMap; import javax.ws.rs.core.UriInfo; import org.apache.pinot.common.auth.AuthProviderUtils; import org.apache.pinot.common.utils.DatabaseUtils; +import org.apache.pinot.controller.ControllerConf; import org.apache.pinot.core.auth.FineGrainedAuthUtils; import org.apache.pinot.core.auth.ManualAuthorization; import org.glassfish.grizzly.http.server.Request; @@ -51,7 +54,8 @@ @javax.ws.rs.ext.Provider public class AuthenticationFilter implements ContainerRequestFilter { private static final Set UNPROTECTED_PATHS = - new HashSet<>(Arrays.asList("", "help", "auth/info", "auth/verify", "auth/verify/v2", "health")); + new HashSet<>(Arrays.asList("", "help", "auth/info", "auth/verify", "auth/verify/v2", "auth/login", + "auth/logout", "auth/session", "health")); private static final String KEY_TABLE_NAME = "tableName"; private static final String KEY_TABLE_NAME_WITH_TYPE = "tableNameWithType"; private static final String KEY_SCHEMA_NAME = "schemaName"; @@ -62,6 +66,12 @@ public class AuthenticationFilter implements ContainerRequestFilter { @Inject AccessControlFactory _accessControlFactory; + @Inject + SessionManager _sessionManager; + + @Inject + ControllerConf _controllerConf; + @Context ResourceInfo _resourceInfo; @@ -83,6 +93,23 @@ public void filter(ContainerRequestContext requestContext) return; } + // SESSION WORKFLOW: If session mode is enabled and the request carries a valid HttpOnly session + // cookie (set by POST /auth/login), skip Authorization-header validation entirely. + // The SessionAuthenticationFilter (priority AUTHENTICATION-10) already validated the cookie + // before this filter runs. Repeating the check here would fail because browser requests in + // SESSION mode never send an Authorization header. + if (_controllerConf != null + && _controllerConf.getProperty(ControllerConf.CONTROLLER_UI_SESSION_ENABLED, false) + && _sessionManager != null) { + Cookie sessionCookie = requestContext.getCookies().get(SessionManager.SESSION_COOKIE_NAME); + if (sessionCookie != null && sessionCookie.getValue() != null) { + Optional username = _sessionManager.getUsername(sessionCookie.getValue()); + if (username.isPresent()) { + return; + } + } + } + // check if authentication is required implicitly if (accessControl.protectAnnotatedOnly() && !endpointMethod.isAnnotationPresent(Authenticate.class)) { return; diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionAuthenticationFilter.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionAuthenticationFilter.java new file mode 100644 index 000000000000..d67409788065 --- /dev/null +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionAuthenticationFilter.java @@ -0,0 +1,179 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.controller.api.access; + + +import java.io.IOException; +import java.util.Arrays; +import java.util.HashSet; +import java.util.List; +import java.util.Optional; +import java.util.Set; +import javax.inject.Inject; +import javax.ws.rs.container.ContainerRequestContext; +import javax.ws.rs.container.ContainerRequestFilter; +import javax.ws.rs.core.Cookie; +import javax.ws.rs.core.HttpHeaders; +import javax.ws.rs.core.Response; +import org.apache.pinot.controller.ControllerConf; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + + +/** + * JAX-RS container filter that enforces session-based authentication for the Pinot UI. + * + *

Architecture (Trino/Ambari-style): + *

This filter is completely INDEPENDENT of the configured {@link AccessControlFactory}. + * It activates when {@code controller.ui.session.enabled=true} is set in the controller + * configuration, regardless of which auth factory is configured (BasicAuth, ZkBasicAuth, + * LDAP/PasswordAuth, or any custom factory). + * + *

This mirrors Trino's {@code FormWebUiAuthenticationFilter} which is independent of + * the {@code FormAuthenticator} implementation. The session layer is a UI concern, not + * an auth backend concern. + * + *

How it works: + *

    + *
  1. Skips public/unprotected paths (login, logout, health, auth/info, static assets)
    2. + *
  2. Reads the {@value SessionManager#SESSION_COOKIE_NAME} HttpOnly cookie
    3. + *
  3. Validates the session token against the server-side {@link SessionManager}
    4. + *
  4. If valid → allows the request to proceed
    5. + *
  5. If invalid/missing → returns 401 (UI redirects to login page)
    6. + *
+ * + *

Key security property: The Authorization header is not used + * for UI requests. The session cookie is the sole credential, and it is HttpOnly so + * JavaScript cannot read it. This eliminates the token-in-header exposure seen in the + * browser dev-tools network tab. + * + *

Activation: Set {@code controller.ui.session.enabled=true} in + * {@code pinot-controller.conf}. No factory class change required. + */ +@javax.ws.rs.ext.Provider +@javax.annotation.Priority(javax.ws.rs.Priorities.AUTHENTICATION - 10) +public class SessionAuthenticationFilter implements ContainerRequestFilter { + private static final Logger LOGGER = LoggerFactory.getLogger(SessionAuthenticationFilter.class); + + /** + * Paths that are always accessible without a session. + * Mirrors Trino's UNPROTECTED_PATHS set. + */ + private static final Set UNPROTECTED_PATHS = new HashSet<>(Arrays.asList( + "", + "help", + "health", + "auth/info", + "auth/verify", + "auth/login", // login endpoint itself must be accessible + "auth/logout", // logout must be accessible to clear the session + "auth/session" // session check must be accessible + )); + + @Inject + private SessionManager _sessionManager; + + @Inject + private ControllerConf _controllerConf; + + @Override + public void filter(ContainerRequestContext requestContext) throws IOException { + // Only activate when session mode is enabled via controller.ui.session.enabled=true. + // This is INDEPENDENT of the configured AccessControlFactory. + // Works with BasicAuth, ZkBasicAuth, LDAP/PasswordAuth, or any custom factory. + if (_controllerConf == null + || !_controllerConf.getProperty(ControllerConf.CONTROLLER_UI_SESSION_ENABLED, false)) { + // Session mode not enabled – let the existing AuthenticationFilter handle it + return; + } + + String path = requestContext.getUriInfo().getPath(); + + // Strip leading slash for comparison + String normalizedPath = path.startsWith("/") ? path.substring(1) : path; + + // Always allow unprotected paths + if (isUnprotected(normalizedPath)) { + return; + } + + // Allow static assets (files with extensions at the root level, e.g., index.html, favicon.ico) + if (isStaticAsset(normalizedPath)) { + return; + } + + + // Server-to-server API calls (e.g., pinot-admin.sh AddTable -authToken "Basic ...") carry an + // Authorization header. This filter handles session validation for browser UI requests only. + // Requests with an Authorization header are passed through here so the downstream + // AuthenticationFilter (priority AUTHENTICATION, which runs after this filter at + // AUTHENTICATION-10) can validate the credential. This is intentional: the session layer + // is a UI concern; API callers authenticate via the AuthenticationFilter, not via session cookies. + // NOTE: This means session mode does NOT block API callers that present a valid Authorization + // header — they are authenticated by AuthenticationFilter instead. + List authHeaders = requestContext.getHeaders().get(HttpHeaders.AUTHORIZATION); + if (authHeaders != null && !authHeaders.isEmpty()) { + // Has Authorization header – let the existing AuthenticationFilter validate it + LOGGER.debug("Request has Authorization header, bypassing session filter for path '{}'", path); + return; + } + + + // Check for a valid session cookie (browser UI requests) + Cookie sessionCookie = requestContext.getCookies().get(SessionManager.SESSION_COOKIE_NAME); + if (sessionCookie != null && sessionCookie.getValue() != null) { + Optional username = _sessionManager.getUsername(sessionCookie.getValue()); + if (username.isPresent()) { + // Valid session – allow the request to proceed + LOGGER.debug("Session valid for user '{}' on path '{}'", username.get(), path); + return; + } + // Session expired or invalid + LOGGER.debug("Session expired or invalid for path '{}'", path); + } + + // No valid session and no Authorization header – return 401 so the UI redirects to the login page + requestContext.abortWith( + Response.status(Response.Status.UNAUTHORIZED) + .entity("{\"error\":\"Session expired or not authenticated. Please log in.\"}") + .type(javax.ws.rs.core.MediaType.APPLICATION_JSON) + .build() + ); + } + + private static boolean isUnprotected(String path) { + // Exact match + if (UNPROTECTED_PATHS.contains(path)) { + return true; + } + // Prefix match for auth/* paths + if (path.startsWith("auth/login") || path.startsWith("auth/logout") + || path.startsWith("auth/info") || path.startsWith("auth/verify") + || path.startsWith("auth/session")) { + return true; + } + return false; + } + + private static boolean isStaticAsset(String path) { + // Files at root level with an extension (e.g., index.html, favicon.ico) + return !path.contains("/") && path.contains("."); + } +} + diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionBasicAuthAccessControlFactory.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionBasicAuthAccessControlFactory.java new file mode 100644 index 000000000000..e3e2ffc72185 --- /dev/null +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionBasicAuthAccessControlFactory.java @@ -0,0 +1,107 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.controller.api.access; + + +import java.util.Collection; +import java.util.Map; +import java.util.Optional; +import java.util.stream.Collectors; +import javax.ws.rs.core.HttpHeaders; +import org.apache.pinot.core.auth.BasicAuthPrincipal; +import org.apache.pinot.core.auth.BasicAuthUtils; +import org.apache.pinot.spi.env.PinotConfiguration; + + +/** + * Session-based Authentication Access Control Factory for Pinot Controller. + * + *

NOTE: This factory is an OPTIONAL convenience class. + * The preferred approach is to use your existing factory unchanged and just add + * {@code controller.ui.session.enabled=true} to your configuration. That approach + * works with ALL auth backends (BasicAuth, ZkBasicAuth, LDAP, custom). + * + *

This factory is identical to {@link BasicAuthAccessControlFactory} in terms of credential + * validation (username/password via HTTP Basic Auth headers), but it reports the + * {@link AccessControl#WORKFLOW_SESSION} workflow to the UI instead of {@code BASIC}. + * + *

Use this factory ONLY if you cannot add {@code controller.ui.session.enabled=true} + * to your configuration (e.g., in environments where config changes are restricted). + * + *

Preferred configuration (works with any factory): + * 

+ *   controller.ui.session.enabled=true
+ *   controller.admin.access.control.factory.class=org.apache.pinot.controller.api.access.BasicAuthAccessControlFactory
+ *
+ * + *

Alternative configuration using this factory: + * 

+ *   controller.admin.access.control.factory.class=\
+ *     org.apache.pinot.controller.api.access.SessionBasicAuthAccessControlFactory
+ *   controller.admin.access.control.principals=admin,user
+ *   controller.admin.access.control.principals.admin.password=verysecret
+ *
+ */ +public class SessionBasicAuthAccessControlFactory implements AccessControlFactory { + private static final String PREFIX = "controller.admin.access.control.principals"; + + private AccessControl _accessControl; + + @Override + public void init(PinotConfiguration configuration) { + _accessControl = new SessionBasicAuthAccessControl( + BasicAuthUtils.extractBasicAuthPrincipals(configuration, PREFIX)); + } + + @Override + public AccessControl create() { + return _accessControl; + } + + /** + * Access Control that uses Basic Auth credential validation but reports SESSION workflow. + * + *

When the UI calls {@code GET /auth/info}, this returns {@code {"workflow":"SESSION"}} + * which causes the UI to use POST /auth/login instead of sending Authorization headers. + */ + private static class SessionBasicAuthAccessControl extends AbstractBasicAuthAccessControl { + + private final Map _token2principal; + + public SessionBasicAuthAccessControl(Collection principals) { + _token2principal = principals.stream() + .collect(Collectors.toMap(BasicAuthPrincipal::getToken, p -> p)); + } + + @Override + protected Optional getPrincipal(HttpHeaders headers) { + return BasicAuthUtils.getPrincipal(getTokens(headers), _token2principal); + } + + /** + * Reports SESSION workflow to the UI. + * This causes the UI to use POST /auth/login instead of sending Authorization headers. + */ + @Override + public AuthWorkflowInfo getAuthWorkflowInfo() { + return new AuthWorkflowInfo(AccessControl.WORKFLOW_SESSION); + } + } +} + diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionManager.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionManager.java new file mode 100644 index 000000000000..554c8eab637c --- /dev/null +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionManager.java @@ -0,0 +1,273 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.controller.api.access; + + +import java.security.SecureRandom; +import java.time.Instant; +import java.util.Base64; +import java.util.Iterator; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + + +/** + * Server-side session manager for Pinot UI authentication. + * + *

Manages opaque session tokens that are issued on successful login and + * invalidated on explicit logout (Ambari-style proper logout invalidation). + * Sessions are stored in-memory with a configurable TTL and are cleaned up + * periodically to prevent unbounded growth. + * + *

Sliding window TTL: Each time a session is accessed via + * {@link #getUsername(String)}, the expiry is extended by the configured TTL. + * This means active users are never logged out due to inactivity — only truly + * idle sessions (no API calls for the full TTL duration) expire. + * + *

Broker credential forwarding: The session stores the + * Basic-auth token used to validate credentials at login. This allows the + * controller to inject a proper {@code Authorization} header when forwarding + * queries to the broker (server-to-server), without ever exposing credentials + * in the browser's network tab. + * + *

Design follows the Ambari / Trino FormWebUiAuthenticationFilter pattern: + *

+ */ +public class SessionManager { + private static final Logger LOGGER = LoggerFactory.getLogger(SessionManager.class); + + /** Cookie name used to carry the session token. */ + public static final String SESSION_COOKIE_NAME = "Pinot-UI-Session"; + + /** Default session TTL: 8 hours (sliding window – resets on each API call). */ + public static final long DEFAULT_SESSION_TTL_SECONDS = 8 * 60 * 60L; + + /** Cleanup interval: every 30 minutes. */ + private static final long CLEANUP_INTERVAL_SECONDS = 30 * 60L; + + /** Number of random bytes for the session token (256-bit entropy). */ + private static final int TOKEN_BYTES = 32; + + private final long _sessionTtlSeconds; + private final SecureRandom _secureRandom = new SecureRandom(); + + /** + * Active session store: token → {@link SessionEntry}. + * ConcurrentHashMap provides thread-safe reads without locking on the hot path. + */ + private final ConcurrentHashMap _sessions = new ConcurrentHashMap<>(); + + private final ScheduledExecutorService _cleanupExecutor = + Executors.newSingleThreadScheduledExecutor(r -> { + Thread t = new Thread(r, "pinot-session-cleanup"); + t.setDaemon(true); + return t; + }); + + public SessionManager() { + this(DEFAULT_SESSION_TTL_SECONDS); + } + + public SessionManager(long sessionTtlSeconds) { + _sessionTtlSeconds = sessionTtlSeconds; + _cleanupExecutor.scheduleAtFixedRate( + this::evictExpiredSessions, + CLEANUP_INTERVAL_SECONDS, + CLEANUP_INTERVAL_SECONDS, + TimeUnit.SECONDS); + LOGGER.info("SessionManager initialized with sliding TTL={}s", _sessionTtlSeconds); + } + + /** + * Creates a new session for the given username and returns the opaque session token. + * + * @param username the authenticated username + * @param basicAuthToken the Basic-auth token (e.g. "Basic dXNlcjpwYXNz") used to validate + * credentials at login. Stored server-side so the controller can inject + * it as an Authorization header when forwarding queries to the broker. + * Never sent to the browser. + * @return a cryptographically random, URL-safe session token + */ + public String createSession(String username, String basicAuthToken) { + String token = generateToken(); + Instant expiry = Instant.now().plusSeconds(_sessionTtlSeconds); + _sessions.put(token, new SessionEntry(username, expiry, basicAuthToken)); + LOGGER.debug("Created session for user '{}', expires at {}", username, expiry); + return token; + } + + /** + * Looks up the username associated with a session token. + * + *

Sliding window: If the session is valid, its expiry is extended + * by the configured TTL. This ensures active users are never logged out due to inactivity. + * + * @param token the session token from the cookie + * @return the username if the session is valid and not expired, empty otherwise + */ + public Optional getUsername(String token) { + if (token == null || token.isEmpty()) { + return Optional.empty(); + } + // Sliding window: single atomic compute() avoids a race condition where a non-atomic + // get+check+put sequence could see the session removed between the expiry check and the put. + // compute() provides an atomic read-modify-write on the ConcurrentHashMap entry. + final String[] resolvedUsername = {null}; + _sessions.compute(token, (k, v) -> { + if (v == null || Instant.now().isAfter(v.expiry())) { + // Expired or not found – remove the entry (return null removes key from map) + return null; + } + resolvedUsername[0] = v.username(); + return new SessionEntry(v.username(), Instant.now().plusSeconds(_sessionTtlSeconds), v.basicAuthToken()); + }); + return Optional.ofNullable(resolvedUsername[0]); + } + + /** + * Returns the Basic-auth token stored for the given session token. + * + *

Used by {@link org.apache.pinot.controller.api.resources.PinotQueryResource} to inject + * a proper {@code Authorization} header when forwarding queries to the broker (server-to-server). + * This ensures the broker can authenticate the request without the browser ever seeing credentials. + * + * @param token the session token from the cookie + * @return the Basic-auth token (e.g. "Basic dXNlcjpwYXNz"), or empty if session not found + */ + public Optional getBasicAuthToken(String token) { + if (token == null || token.isEmpty()) { + return Optional.empty(); + } + SessionEntry entry = _sessions.get(token); + if (entry == null || Instant.now().isAfter(entry.expiry())) { + return Optional.empty(); + } + return Optional.ofNullable(entry.basicAuthToken()); + } + + /** + * Invalidates a session token (called on logout). + * This is the key difference from stateless JWT: the server can immediately + * revoke access without waiting for token expiry. + * + * @param token the session token to invalidate + */ + public void invalidateSession(String token) { + if (token != null && !token.isEmpty()) { + SessionEntry removed = _sessions.remove(token); + if (removed != null) { + LOGGER.debug("Session invalidated for user '{}'", removed.username()); + } + } + } + + /** + * Returns the configured session TTL in seconds. + */ + public long getSessionTtlSeconds() { + return _sessionTtlSeconds; + } + + /** + * Returns the number of active (possibly including some expired) sessions. + * Useful for monitoring/metrics. + */ + public int getActiveSessionCount() { + return _sessions.size(); + } + + /** + * Shuts down the background cleanup executor. + */ + public void shutdown() { + _cleanupExecutor.shutdownNow(); + } + + // --------------------------------------------------------------------------- + // Private helpers + // --------------------------------------------------------------------------- + + private String generateToken() { + byte[] bytes = new byte[TOKEN_BYTES]; + _secureRandom.nextBytes(bytes); + return Base64.getUrlEncoder().withoutPadding().encodeToString(bytes); + } + + private void evictExpiredSessions() { + Instant now = Instant.now(); + int removed = 0; + Iterator> it = _sessions.entrySet().iterator(); + while (it.hasNext()) { + Map.Entry entry = it.next(); + if (now.isAfter(entry.getValue().expiry())) { + it.remove(); + removed++; + } + } + if (removed > 0) { + LOGGER.debug("Evicted {} expired sessions; {} active sessions remain", removed, _sessions.size()); + } + } + + // --------------------------------------------------------------------------- + // Inner record + // --------------------------------------------------------------------------- + + /** + * Immutable session entry holding the username, expiry instant, and stored Basic-auth token. + * + *

The {@code basicAuthToken} is stored server-side only — it is never sent to the browser. + * It is used to inject an {@code Authorization} header when the controller forwards queries + * to the broker (server-to-server communication). + */ + private static final class SessionEntry { + private final String _username; + private final Instant _expiry; + private final String _basicAuthToken; + + SessionEntry(String username, Instant expiry, String basicAuthToken) { + _username = username; + _expiry = expiry; + _basicAuthToken = basicAuthToken; + } + + String username() { + return _username; + } + + Instant expiry() { + return _expiry; + } + + String basicAuthToken() { + return _basicAuthToken; + } + } +} + diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java index 2834e8695f01..fc74fc87215c 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java @@ -36,6 +36,7 @@ import javax.ws.rs.core.Context; import javax.ws.rs.core.HttpHeaders; import javax.ws.rs.core.MediaType; +import org.apache.pinot.controller.ControllerConf; import org.apache.pinot.controller.api.access.AccessControl; import org.apache.pinot.controller.api.access.AccessControlFactory; import org.apache.pinot.controller.api.access.AccessType; @@ -56,6 +57,9 @@ public class PinotControllerAuthResource { @Inject private AccessControlFactory _accessControlFactory; + @Inject + private ControllerConf _controllerConf; + @Context HttpHeaders _httpHeaders; @@ -111,8 +115,15 @@ public boolean verifyV2(@ApiParam(value = "Table name without type") @QueryParam } /** - * Provide the auth workflow configuration for the Pinot UI to perform user authentication. Currently supports NONE - * (no auth) and BASIC (basic auth with username and password) + * Provide the auth workflow configuration for the Pinot UI to perform user authentication. + * + *

When {@code controller.ui.session.authentication.enabled=true}, returns + * {@code {"workflow":"SESSION","inactivityTimeoutSeconds":}} regardless of which + * {@link AccessControlFactory} is configured. The session layer is independent of the auth backend + * (BasicAuth, ZkBasicAuth, LDAP/PasswordAuth, or any custom factory). + * + *

When session mode is disabled (default), the underlying factory's workflow is returned + * (NONE, BASIC, OIDC, etc.), preserving backward compatibility. * * @return auth workflow info/configuration */ @@ -123,6 +134,13 @@ public boolean verifyV2(@ApiParam(value = "Table name without type") @QueryParam @ApiOperation(value = "Retrieve auth workflow info") @ApiResponses(value = {@ApiResponse(code = 200, message = "Auth workflow info provided")}) public AccessControl.AuthWorkflowInfo info() { + if (_controllerConf != null + && _controllerConf.getProperty(ControllerConf.CONTROLLER_UI_SESSION_ENABLED, false)) { + long inactivityTimeout = _controllerConf.getProperty( + ControllerConf.CONTROLLER_UI_SESSION_INACTIVITY_TIMEOUT_SECONDS, + ControllerConf.DEFAULT_UI_SESSION_INACTIVITY_TIMEOUT_SECONDS); + return new AccessControl.AuthWorkflowInfo(AccessControl.WORKFLOW_SESSION, inactivityTimeout); + } return _accessControlFactory.create().getAuthWorkflowInfo(); } } diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java index 47f9675fefa0..1776d5ed941b 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java @@ -53,6 +53,7 @@ import javax.ws.rs.QueryParam; import javax.ws.rs.WebApplicationException; import javax.ws.rs.core.Context; +import javax.ws.rs.core.Cookie; import javax.ws.rs.core.HttpHeaders; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; @@ -74,6 +75,7 @@ import org.apache.pinot.controller.api.access.AccessControl; import org.apache.pinot.controller.api.access.AccessControlFactory; import org.apache.pinot.controller.api.access.AccessType; +import org.apache.pinot.controller.api.access.SessionManager; import org.apache.pinot.controller.helix.core.PinotHelixResourceManager; import org.apache.pinot.core.auth.Actions; import org.apache.pinot.core.auth.ManualAuthorization; @@ -120,6 +122,10 @@ public class PinotQueryResource { @Inject ControllerConf _controllerConf; + @Inject + @org.glassfish.hk2.api.Optional + SessionManager _sessionManager; + @POST @Path("sql") @ManualAuthorization // performed by broker @@ -408,11 +414,15 @@ private StreamingOutput executeSqlQuery(@Context HttpHeaders httpHeaders, String throw QueryErrorCode.INTERNAL.asException("V2 Multi-Stage query engine not enabled."); } + // Compute session validity once here so both DQL paths don't each look up the session independently. + boolean sessionValid = hasValidSessionCookie(httpHeaders); + switch (sqlType) { case DQL: return isMse - ? getMultiStageQueryResponse(sqlQuery, queryOptions, httpHeaders, traceEnabled) - : getQueryResponse(sqlQuery, sqlNodeAndOptions.getSqlNode(), traceEnabled, queryOptions, httpHeaders); + ? getMultiStageQueryResponse(sqlQuery, queryOptions, httpHeaders, traceEnabled, sessionValid) + : getQueryResponse(sqlQuery, sqlNodeAndOptions.getSqlNode(), traceEnabled, queryOptions, httpHeaders, + sessionValid); case DML: Map headers = extractHeaders(httpHeaders); return output -> { @@ -425,14 +435,38 @@ private StreamingOutput executeSqlQuery(@Context HttpHeaders httpHeaders, String } } - private StreamingOutput getMultiStageQueryResponse(String query, String queryOptions, HttpHeaders httpHeaders, - String traceEnabled) { + /** + * Returns {@code true} if the request carries a valid server-side session cookie. + * + *

When session mode is enabled, browser UI requests carry an HttpOnly cookie instead of an + * Authorization header. If a valid session is present, the access-control check that expects an + * Authorization header is skipped to avoid a spurious 403 that would redirect the user to login. + */ + private boolean hasValidSessionCookie(HttpHeaders httpHeaders) { + if (_sessionManager == null + || _controllerConf == null + || !_controllerConf.getProperty(ControllerConf.CONTROLLER_UI_SESSION_ENABLED, false)) { + return false; + } + Cookie sessionCookie = httpHeaders.getCookies().get(SessionManager.SESSION_COOKIE_NAME); + if (sessionCookie == null || sessionCookie.getValue() == null) { + return false; + } + return _sessionManager.getUsername(sessionCookie.getValue()).isPresent(); + } - // Validate data access - // we don't have a cross table access control rule so only ADMIN can make request to multi-stage engine. - AccessControl accessControl = _accessControlFactory.create(); - if (!accessControl.hasAccess(AccessType.READ, httpHeaders, "/sql")) { - throw new WebApplicationException("Permission denied", Response.Status.FORBIDDEN); + private StreamingOutput getMultiStageQueryResponse(String query, String queryOptions, HttpHeaders httpHeaders, + String traceEnabled, boolean sessionValid) { + + // SESSION WORKFLOW: If the request has a valid session cookie, skip the Authorization-header + // access check. The session was already validated by SessionAuthenticationFilter before this method. + if (!sessionValid) { + // Validate data access + // we don't have a cross table access control rule so only ADMIN can make request to multi-stage engine. + AccessControl accessControl = _accessControlFactory.create(); + if (!accessControl.hasAccess(AccessType.READ, httpHeaders, "/sql")) { + throw new WebApplicationException("Permission denied", Response.Status.FORBIDDEN); + } } Map queryOptionsMap = RequestUtils.parseQuery(query).getOptions(); @@ -491,7 +525,7 @@ private List getInstanceIds(String query, List tableNames, Strin } private StreamingOutput getQueryResponse(String query, @Nullable SqlNode sqlNode, String traceEnabled, - String queryOptions, HttpHeaders httpHeaders) { + String queryOptions, HttpHeaders httpHeaders, boolean sessionValid) { // Get resource table name. String tableName; Map queryOptionsMap = RequestUtils.parseQuery(query).getOptions(); @@ -524,10 +558,14 @@ private StreamingOutput getQueryResponse(String query, @Nullable SqlNode sqlNode } String rawTableName = TableNameBuilder.extractRawTableName(tableName); - // Validate data access - AccessControl accessControl = _accessControlFactory.create(); - if (!accessControl.hasAccess(rawTableName, AccessType.READ, httpHeaders, Actions.Table.QUERY)) { - throw QueryErrorCode.ACCESS_DENIED.asException(); + // SESSION WORKFLOW: If the request has a valid session cookie, skip the Authorization-header + // access check. The session was already validated by SessionAuthenticationFilter before this method. + if (!sessionValid) { + // Validate data access + AccessControl accessControl = _accessControlFactory.create(); + if (!accessControl.hasAccess(rawTableName, AccessType.READ, httpHeaders, Actions.Table.QUERY)) { + throw QueryErrorCode.ACCESS_DENIED.asException(); + } } // Get brokers for the resource table. @@ -611,6 +649,21 @@ private StreamingOutput sendRequestToBroker(String query, String instanceId, Str // Forward client-supplied headers Map headers = extractHeaders(httpHeaders); + // SESSION WORKFLOW: Inject Authorization header for broker (server-to-server). + // Browser UI requests carry only the HttpOnly session cookie – no Authorization header. + // The broker requires auth, so we retrieve the stored Basic-auth token from the session + // and inject it as an Authorization header for the controller→broker request. + // This token is stored server-side only (never sent to the browser). + if (_sessionManager != null + && _controllerConf != null + && _controllerConf.getProperty(ControllerConf.CONTROLLER_UI_SESSION_ENABLED, false)) { + Cookie sessionCookie = httpHeaders.getCookies().get(SessionManager.SESSION_COOKIE_NAME); + if (sessionCookie != null && sessionCookie.getValue() != null) { + _sessionManager.getBasicAuthToken(sessionCookie.getValue()).ifPresent( + authToken -> headers.put(HttpHeaders.AUTHORIZATION, authToken)); + } + } + return sendRequestRaw(url, "POST", query, requestJson, headers); } @@ -806,9 +859,14 @@ private String getTimeSeriesQueryURL(String protocol, String hostName, int port, } private Map extractHeaders(HttpHeaders httpHeaders) { + // In SESSION mode, exclude the Authorization header from being forwarded to the broker. + // The broker auth is injected server-side from the session store in sendRequestToBroker(). + boolean isSessionMode = _controllerConf != null + && _controllerConf.getProperty(ControllerConf.CONTROLLER_UI_SESSION_ENABLED, false); return httpHeaders.getRequestHeaders().entrySet().stream() - .filter(entry -> !entry.getValue().isEmpty()) - .collect(Collectors.toMap(Entry::getKey, entry -> entry.getValue().get(0))); + .filter(entry -> !entry.getValue().isEmpty()) + .filter(entry -> !isSessionMode || !entry.getKey().equalsIgnoreCase(HttpHeaders.AUTHORIZATION)) + .collect(Collectors.toMap(Entry::getKey, entry -> entry.getValue().get(0))); } private InstanceConfig getInstanceConfig(String instanceId) { diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSessionLoginResource.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSessionLoginResource.java new file mode 100644 index 000000000000..5b392585ac7b --- /dev/null +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSessionLoginResource.java @@ -0,0 +1,375 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.controller.api.resources; + + +import io.swagger.annotations.Api; +import io.swagger.annotations.ApiOperation; +import io.swagger.annotations.ApiParam; +import io.swagger.annotations.ApiResponse; +import io.swagger.annotations.ApiResponses; +import java.nio.charset.StandardCharsets; +import java.util.Base64; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import javax.inject.Inject; +import javax.ws.rs.Consumes; +import javax.ws.rs.FormParam; +import javax.ws.rs.GET; +import javax.ws.rs.POST; +import javax.ws.rs.Path; +import javax.ws.rs.Produces; +import javax.ws.rs.core.Context; +import javax.ws.rs.core.Cookie; +import javax.ws.rs.core.HttpHeaders; +import javax.ws.rs.core.MediaType; +import javax.ws.rs.core.MultivaluedHashMap; +import javax.ws.rs.core.MultivaluedMap; +import javax.ws.rs.core.Response; +import org.apache.pinot.controller.ControllerConf; +import org.apache.pinot.controller.api.access.AccessControl; +import org.apache.pinot.controller.api.access.AccessControlFactory; +import org.apache.pinot.controller.api.access.AccessType; +import org.apache.pinot.controller.api.access.SessionManager; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + + +/** + * REST resource for session-based UI authentication. + * + *

Architecture (Trino/Ambari-style): + *

This resource is a session layer that sits on top of ANY configured + * {@link AccessControlFactory}. It does NOT require a specific factory class. + * Whether you use {@code BasicAuthAccessControlFactory}, {@code ZkBasicAuthAccessControlFactory}, + * {@code PasswordAuthAccessControlFactory} (LDAP), or any custom factory — this session + * layer works with all of them. + * + *

How it works: + *

    + *
  1. UI calls {@code GET /auth/info} → receives {@code {"workflow":"SESSION"}} when + * {@code controller.ui.session.enabled=true}
    2. + *
  2. UI shows login form → user enters username/password
    3. + *
  3. UI POSTs to {@code POST /auth/login} (form-encoded, NOT Basic auth header)
    4. + *
  4. This resource builds a synthetic Basic-auth header and calls the configured + * {@link AccessControl#hasAccess} to validate credentials against whatever + * backend is configured (Basic, LDAP, ZK, etc.)
    5. + *
  5. On success: creates a server-side session → sets an HttpOnly cookie
    6. + *
  6. Subsequent requests: browser sends cookie automatically, NO Authorization header
    7. + *
  7. {@code GET /auth/logout}: immediately invalidates the server-side session
    8. + *
+ * + *

Key security properties: + *

+ */ +@Api(tags = "Auth") +@Path("/") +public class PinotSessionLoginResource { + private static final Logger LOGGER = LoggerFactory.getLogger(PinotSessionLoginResource.class); + + /** Path for the login endpoint. */ + public static final String AUTH_LOGIN_PATH = "auth/login"; + + /** Path for the logout endpoint. */ + public static final String AUTH_LOGOUT_PATH = "auth/logout"; + + /** Path for the session-check endpoint. */ + public static final String AUTH_SESSION_PATH = "auth/session"; + + @Inject + private AccessControlFactory _accessControlFactory; + + @Inject + private SessionManager _sessionManager; + + @Inject + private ControllerConf _controllerConf; + + // --------------------------------------------------------------------------- + // POST /auth/login + // --------------------------------------------------------------------------- + + /** + * Validates username/password using the configured {@link AccessControlFactory} + * (works with BasicAuth, ZkBasicAuth, LDAP/PasswordAuth, or any custom factory) + * and on success creates a server-side session and returns an HttpOnly cookie. + * + *

This is the Trino/Ambari pattern: the session layer is independent of the + * auth backend. The same login endpoint works regardless of which factory is configured. + */ + @POST + @Path(AUTH_LOGIN_PATH) + @Consumes(MediaType.APPLICATION_FORM_URLENCODED) + @Produces(MediaType.APPLICATION_JSON) + @ApiOperation(value = "Login with username and password to obtain a session cookie") + @ApiResponses(value = { + @ApiResponse(code = 200, message = "Login successful – session cookie set"), + @ApiResponse(code = 401, message = "Invalid credentials") + }) + public Response login( + @ApiParam(value = "Username", required = true) @FormParam("username") String username, + @ApiParam(value = "Password", required = true) @FormParam("password") String password) { + + if (username == null || username.trim().isEmpty()) { + return Response.status(Response.Status.BAD_REQUEST) + .entity("{\"error\":\"username is required\"}") + .build(); + } + + // Build a Basic-auth header to validate credentials via the configured AccessControl. + // This works with ANY AccessControlFactory: + // - BasicAuthAccessControlFactory: validates against in-memory principals + // - ZkBasicAuthAccessControlFactory: validates against ZooKeeper-stored principals + // - PasswordAuthAccessControlFactory: validates against LDAP or other password backends + // - Any custom factory that implements hasAccess() + String basicAuthToken = "Basic " + Base64.getEncoder() + .encodeToString((username + ":" + (password == null ? "" : password)) + .getBytes(StandardCharsets.UTF_8)); + + AccessControl accessControl = _accessControlFactory.create(); + boolean valid; + try { + valid = accessControl.hasAccess(null, AccessType.READ, + new SyntheticHttpHeaders(basicAuthToken), AUTH_LOGIN_PATH); + } catch (javax.ws.rs.NotAuthorizedException e) { + valid = false; + } catch (Exception e) { + LOGGER.warn("Unexpected error during login for user '{}': {}", username, e.getMessage()); + valid = false; + } + + if (!valid) { + LOGGER.warn("Failed login attempt for user '{}'", username); + return Response.status(Response.Status.UNAUTHORIZED) + .entity("{\"error\":\"Invalid username or password\"}") + .build(); + } + + // Credentials are valid – create a server-side session. + // Store the Basic-auth token server-side so the controller can inject it as an + // Authorization header when forwarding queries to the broker (server-to-server). + // This token is NEVER sent to the browser – it stays in server memory only. + String sessionToken = _sessionManager.createSession(username.trim(), basicAuthToken); + LOGGER.info("User '{}' logged in successfully", username); + + boolean secureCookie = _controllerConf == null + || _controllerConf.getProperty(ControllerConf.CONTROLLER_UI_SESSION_COOKIE_SECURE, true); + String sessionCookieHeader = buildSessionCookieHeader( + sessionToken, (int) _sessionManager.getSessionTtlSeconds(), secureCookie); + + return Response.ok("{\"status\":\"ok\",\"username\":\"" + username.trim() + "\"}") + .header("Set-Cookie", sessionCookieHeader) + .build(); + } + + // --------------------------------------------------------------------------- + // GET /auth/logout + // --------------------------------------------------------------------------- + + /** + * Invalidates the server-side session immediately and clears the session cookie. + * + *

This is proper Ambari/Trino-style logout: the session is removed from the + * server store immediately. Even if someone captured the cookie, they cannot reuse it. + * This is the key difference from stateless JWT: the server can revoke access instantly. + */ + @GET + @Path(AUTH_LOGOUT_PATH) + @Produces(MediaType.APPLICATION_JSON) + @ApiOperation(value = "Logout and invalidate the current session") + @ApiResponses(value = { + @ApiResponse(code = 200, message = "Logout successful – session invalidated") + }) + public Response logout(@Context HttpHeaders httpHeaders) { + Cookie sessionCookie = httpHeaders.getCookies().get(SessionManager.SESSION_COOKIE_NAME); + if (sessionCookie != null && sessionCookie.getValue() != null) { + _sessionManager.invalidateSession(sessionCookie.getValue()); + LOGGER.info("Session invalidated on logout"); + } + + // Return a delete-cookie (Max-Age=0) to clear it from the browser + String deleteCookieHeader = buildDeleteCookieHeader(); + + return Response.ok("{\"status\":\"logged_out\"}") + .header("Set-Cookie", deleteCookieHeader) + .build(); + } + + // --------------------------------------------------------------------------- + // GET /auth/session + // --------------------------------------------------------------------------- + + /** + * Returns the current session status (username) if a valid session cookie is present. + * + *

Used by the UI to: + *

    + *
  1. Check whether the user is still authenticated after a page refresh
    2. + *
  2. Periodically verify session validity (every 5 minutes) to detect expiry
    3. + *
+ * + *

Returns 401 when the session has expired, triggering a redirect to the login page. + */ + @GET + @Path(AUTH_SESSION_PATH) + @Produces(MediaType.APPLICATION_JSON) + @ApiOperation(value = "Check current session validity") + @ApiResponses(value = { + @ApiResponse(code = 200, message = "Session is valid"), + @ApiResponse(code = 401, message = "No valid session – session expired or not authenticated") + }) + public Response checkSession(@Context HttpHeaders httpHeaders) { + Cookie sessionCookie = httpHeaders.getCookies().get(SessionManager.SESSION_COOKIE_NAME); + if (sessionCookie == null || sessionCookie.getValue() == null) { + return Response.status(Response.Status.UNAUTHORIZED) + .entity("{\"error\":\"No session\"}") + .build(); + } + + Optional username = _sessionManager.getUsername(sessionCookie.getValue()); + if (!username.isPresent()) { + // Session expired – return 401 so the UI redirects to login + return Response.status(Response.Status.UNAUTHORIZED) + .entity("{\"error\":\"Session expired\"}") + .build(); + } + + return Response.ok("{\"status\":\"ok\",\"username\":\"" + username.get() + "\"}") + .build(); + } + + // --------------------------------------------------------------------------- + // Cookie helpers + // --------------------------------------------------------------------------- + + /** + * Builds a raw {@code Set-Cookie} header value for the session cookie. + * + *

We construct the header manually (instead of using {@link javax.ws.rs.core.NewCookie}) + * because JAX-RS 2 {@code NewCookie} does not support the {@code SameSite} attribute. + * {@code SameSite=Strict} is required to prevent CSRF attacks: the browser will only send + * the cookie when the request originates from the same site (Pinot UI itself), blocking + * forged cross-site requests from attacker-controlled pages. + */ + private static String buildSessionCookieHeader(String token, int maxAgeSeconds, boolean secure) { + StringBuilder sb = new StringBuilder(); + sb.append(SessionManager.SESSION_COOKIE_NAME).append('=').append(token); + sb.append("; Path=/"); + sb.append("; Max-Age=").append(maxAgeSeconds); + sb.append("; HttpOnly"); + sb.append("; SameSite=Strict"); + if (secure) { + sb.append("; Secure"); + } + return sb.toString(); + } + + /** Builds a {@code Set-Cookie} header that instructs the browser to delete the session cookie. */ + private static String buildDeleteCookieHeader() { + return SessionManager.SESSION_COOKIE_NAME + "=deleted" + + "; Path=/" + + "; Max-Age=0" + + "; HttpOnly" + + "; SameSite=Strict"; + } + + // --------------------------------------------------------------------------- + // Synthetic HttpHeaders for credential validation + // --------------------------------------------------------------------------- + + /** + * Minimal {@link HttpHeaders} implementation that carries only the Authorization header. + * + *

Used to validate credentials via the configured {@link AccessControl#hasAccess} + * without requiring an actual HTTP request with an Authorization header. This allows + * the session login endpoint to work with ANY AccessControlFactory implementation. + */ + private static final class SyntheticHttpHeaders implements HttpHeaders { + private final String _authorizationValue; + + SyntheticHttpHeaders(String authorizationValue) { + _authorizationValue = authorizationValue; + } + + @Override + public List getRequestHeader(String name) { + if (HttpHeaders.AUTHORIZATION.equalsIgnoreCase(name) + || "authorization".equalsIgnoreCase(name)) { + return Collections.singletonList(_authorizationValue); + } + return Collections.emptyList(); + } + + @Override + public String getHeaderString(String name) { + List values = getRequestHeader(name); + return values.isEmpty() ? null : values.get(0); + } + + @Override + public MultivaluedMap getRequestHeaders() { + MultivaluedMap map = new MultivaluedHashMap<>(); + map.put(HttpHeaders.AUTHORIZATION, Collections.singletonList(_authorizationValue)); + return map; + } + + @Override + public List getAcceptableMediaTypes() { + return Collections.singletonList(javax.ws.rs.core.MediaType.WILDCARD_TYPE); + } + + @Override + public List getAcceptableLanguages() { + return Collections.emptyList(); + } + + @Override + public javax.ws.rs.core.MediaType getMediaType() { + return null; + } + + @Override + public java.util.Locale getLanguage() { + return null; + } + + @Override + public Map getCookies() { + return Collections.emptyMap(); + } + + @Override + public java.util.Date getDate() { + return null; + } + + @Override + public int getLength() { + return -1; + } + } +} + diff --git a/pinot-controller/src/main/resources/app/App.tsx b/pinot-controller/src/main/resources/app/App.tsx index fc3436b580b0..ddf96de3d564 100644 --- a/pinot-controller/src/main/resources/app/App.tsx +++ b/pinot-controller/src/main/resources/app/App.tsx @@ -17,7 +17,7 @@ * under the License. */ -import React, { useEffect, useState } from 'react'; +import React, { useEffect, useRef, useState } from 'react'; import { Switch, Route, Redirect, useHistory } from 'react-router-dom'; import Layout from './components/Layout'; import RouterData from './router'; @@ -26,6 +26,12 @@ import app_state from './app_state'; import { useAuthProvider } from './components/auth/AuthProvider'; import { AppLoadingIndicator } from './components/AppLoadingIndicator'; import { AuthWorkflow } from 'Models'; +import Button from '@material-ui/core/Button'; +import Dialog from '@material-ui/core/Dialog'; +import DialogActions from '@material-ui/core/DialogActions'; +import DialogContent from '@material-ui/core/DialogContent'; +import DialogContentText from '@material-ui/core/DialogContentText'; +import DialogTitle from '@material-ui/core/DialogTitle'; import { TimezoneProvider } from './contexts/TimezoneContext'; import { runBootstrapRequest } from './utils/bootstrap'; @@ -38,14 +44,13 @@ export const App = () => { const [loading, setLoading] = useState(true); const [isAuthenticated, setIsAuthenticated] = useState(null); const [role, setRole] = useState(''); - const { authUserName, authUserEmail, authenticated, authWorkflow } = useAuthProvider(); + const { authUserName, authUserEmail, authenticated, authWorkflow, inactivityTimeoutSeconds } = useAuthProvider(); const history = useHistory(); useEffect(() => { // authentication already handled by authProvider if (authUserEmail && authenticated) { // Authenticated with an auth method that supports user identity - // Any code that needs user identity can go here } if (authenticated) { @@ -54,10 +59,77 @@ export const App = () => { }, [authUserName, authUserEmail, authenticated]); useEffect(() => { - if(authWorkflow === AuthWorkflow.BASIC) { + // Both BASIC and SESSION workflows are handled by the login page. + if (authWorkflow === AuthWorkflow.BASIC || authWorkflow === AuthWorkflow.SESSION) { setLoading(false); } - }, [authWorkflow]) + }, [authWorkflow]); + + // ─── SESSION INACTIVITY TIMEOUT ─────────────────────────────────────────── + // Timeout duration comes from /auth/info (controller.ui.session.inactivity.timeout.seconds). + // Falls back to 300s (5 min) if not set. + // + // Timeline: + // t=0 user active + // t=timeout-60s show warning dialog with 60s countdown + // t=timeout call /auth/logout (server invalidates session), redirect to /login + // + // Any user interaction (mouse/key/click/scroll) resets the full timer. + + const [showTimeoutWarning, setShowTimeoutWarning] = useState(false); + const [warningCountdown, setWarningCountdown] = useState(60); + const inactivityTimerRef = useRef | null>(null); + const warningTimerRef = useRef | null>(null); + const countdownIntervalRef = useRef | null>(null); + + const performSessionLogout = async () => { + setShowTimeoutWarning(false); + try { + await fetch('/auth/logout', { method: 'GET', credentials: 'include' }); + } catch (e) { + // Even on network error, clear local state and redirect + } + app_state.authToken = null; + app_state.authWorkflow = null; + app_state.username = null; + setIsAuthenticated(false); + window.location.href = '/#/login'; + }; + + const resetInactivityTimer = () => { + if (inactivityTimerRef.current) clearTimeout(inactivityTimerRef.current); + if (warningTimerRef.current) clearTimeout(warningTimerRef.current); + if (countdownIntervalRef.current) clearInterval(countdownIntervalRef.current); + setShowTimeoutWarning(false); + + const timeoutMs = (inactivityTimeoutSeconds > 0 ? inactivityTimeoutSeconds : 300) * 1000; + const warningMs = Math.max(0, timeoutMs - 60000); + + warningTimerRef.current = setTimeout(() => { + setWarningCountdown(60); + setShowTimeoutWarning(true); + countdownIntervalRef.current = setInterval(() => { + setWarningCountdown((c) => c - 1); + }, 1000); + }, warningMs); + + inactivityTimerRef.current = setTimeout(performSessionLogout, timeoutMs); + }; + + useEffect(() => { + if (authWorkflow !== AuthWorkflow.SESSION || !isAuthenticated) return; + + const events = ['mousedown', 'keydown', 'click', 'scroll', 'touchstart']; + events.forEach((e) => window.addEventListener(e, resetInactivityTimer)); + resetInactivityTimer(); + + return () => { + events.forEach((e) => window.removeEventListener(e, resetInactivityTimer)); + if (inactivityTimerRef.current) clearTimeout(inactivityTimerRef.current); + if (warningTimerRef.current) clearTimeout(warningTimerRef.current); + if (countdownIntervalRef.current) clearInterval(countdownIntervalRef.current); + }; + }, [authWorkflow, isAuthenticated, inactivityTimeoutSeconds]); const applyClusterConfig = (clusterConfig?: Record) => { const nextQueryConsoleOnlyView = clusterConfig?.queryConsoleOnlyView === 'true'; @@ -192,6 +264,30 @@ export const App = () => { return ( + {/* SESSION inactivity warning dialog */} +

+ Session Expiring Soon + + + Your session will expire in {warningCountdown} second{warningCountdown !== 1 ? 's' : ''} due to inactivity. + + + + + + + {getRouterData().map(({ path, Component }, key) => ( void; @@ -100,6 +103,10 @@ const useStyles = makeStyles((theme) => ({ fontWeight: 500 } }, + logoutButton: { + color: '#ffffff', + marginLeft: theme.spacing(1), + }, timezoneContainer: { display: 'flex', alignItems: 'center', @@ -130,6 +137,35 @@ const useStyles = makeStyles((theme) => ({ const Header = ({ highlightSidebarLink, showHideSideBarHandler, openSidebar, clusterName, ...props }: Props) => { const classes = useStyles(); + const history = useHistory(); + + const handleLogout = async () => { + if (app_state.authWorkflow === AuthWorkflow.SESSION) { + // SESSION logout: call the server to invalidate the session immediately. + // The server removes the session from its store and returns a Max-Age=0 cookie. + try { + await fetch('/auth/logout', { + method: 'GET', + credentials: 'include', + }); + } catch (e) { + // Even if the request fails, clear local state and redirect + } + app_state.authToken = null; + app_state.authWorkflow = null; + app_state.username = null; + history.push('/login'); + } else if (app_state.authWorkflow === AuthWorkflow.BASIC) { + app_state.authToken = null; + app_state.authWorkflow = null; + app_state.username = null; + history.push('/login'); + } + }; + + const showLogout = app_state.authWorkflow === AuthWorkflow.SESSION + || app_state.authWorkflow === AuthWorkflow.BASIC; + return ( @@ -175,6 +211,13 @@ const Header = ({ highlightSidebarLink, showHideSideBarHandler, openSidebar, clu + {showLogout && ( + + + + + + )} diff --git a/pinot-controller/src/main/resources/app/components/auth/AuthProvider.tsx b/pinot-controller/src/main/resources/app/components/auth/AuthProvider.tsx index 8bdde8b150f2..8ccf225f2580 100644 --- a/pinot-controller/src/main/resources/app/components/auth/AuthProvider.tsx +++ b/pinot-controller/src/main/resources/app/components/auth/AuthProvider.tsx @@ -29,7 +29,9 @@ interface AuthProviderContextProps { authUserName: string; authUserEmail: string; authenticated: boolean; - authWorkflow: AuthWorkflow + authWorkflow: AuthWorkflow; + /** UI inactivity timeout in seconds from controller config. -1 if not applicable (non-SESSION). */ + inactivityTimeoutSeconds: number; } export const AuthProvider = ({ children }) => { @@ -37,6 +39,7 @@ export const AuthProvider = ({ children }) => { const [redirectUri, setRedirectUri] = useState(null); const [clientId, setClientId] = useState(null); const [authWorkflow, setAuthWorkflow] = useState(null); + const [inactivityTimeoutSeconds, setInactivityTimeoutSeconds] = useState(-1); const [accessToken, setAccessToken] = useState(""); const [authUserName, setAuthUserName] = useState(""); const [authUserEmail, setAuthUserEmail] = useState(""); @@ -83,6 +86,13 @@ export const AuthProvider = ({ children }) => { // set auth workflow setAuthWorkflow(authWorkFlowInternal); + // Read inactivity timeout from auth/info response (SESSION workflow only). + if (authWorkFlowInternal === AuthWorkflow.SESSION + && authInfoResponse && authInfoResponse.inactivityTimeoutSeconds + && authInfoResponse.inactivityTimeoutSeconds > 0) { + setInactivityTimeoutSeconds(authInfoResponse.inactivityTimeoutSeconds); + } + if (authWorkFlowInternal === AuthWorkflow.NONE) { // No authentication required setAuthenticated(true); @@ -92,6 +102,23 @@ export const AuthProvider = ({ children }) => { // basic auth is handled by login page } + if (authWorkFlowInternal === AuthWorkflow.SESSION) { + // Session auth: check if there is an active server-side session by calling /auth/session. + // The HttpOnly cookie is sent automatically by the browser – no JS token needed. + try { + const sessionResponse = await fetch('/auth/session', { credentials: 'include' }); + if (sessionResponse.ok) { + const sessionData = await sessionResponse.json(); + if (sessionData && sessionData.username) { + setAuthenticated(true); + } + } + // If not ok (401), the user needs to log in + } catch (e) { + // Network error – stay unauthenticated + } + } + // set OIDC auth details if (authWorkFlowInternal === AuthWorkflow.OIDC) { const issuer = @@ -223,7 +250,8 @@ export const AuthProvider = ({ children }) => { accessToken: accessToken, authUserName: authUserName, authUserEmail: authUserEmail, - authenticated: authenticated + authenticated: authenticated, + inactivityTimeoutSeconds: inactivityTimeoutSeconds, } if (loading) { diff --git a/pinot-controller/src/main/resources/app/pages/LoginPage.tsx b/pinot-controller/src/main/resources/app/pages/LoginPage.tsx index ca4585010578..c5525f076ee0 100644 --- a/pinot-controller/src/main/resources/app/pages/LoginPage.tsx +++ b/pinot-controller/src/main/resources/app/pages/LoginPage.tsx @@ -24,6 +24,7 @@ import { useForm } from 'react-hook-form'; import PinotMethodUtils from '../utils/PinotMethodUtils'; import app_state from '../app_state'; import { AuthWorkflow } from 'Models'; +import { useAuthProvider } from '../components/auth/AuthProvider'; interface FormData { username: string; @@ -71,11 +72,51 @@ const useStyles = makeStyles((theme: Theme) => { const LoginPage = (props) => { const { handleSubmit, register } = useForm(); const [invalidToken, setInvalidToken] = React.useState(null); + const [isLoading, setIsLoading] = React.useState(false); + const { authWorkflow } = useAuthProvider(); const onSubmit = handleSubmit(async (data) => { - const authToken = "Basic "+btoa(data.username+":"+data.password); + setIsLoading(true); + setInvalidToken(null); + + // SESSION workflow: POST credentials as form data to /auth/login. + // The server validates and sets an HttpOnly SameSite=Strict cookie. No Authorization header is used. + if (authWorkflow === AuthWorkflow.SESSION) { + try { + const formData = new URLSearchParams(); + formData.append('username', data.username); + formData.append('password', data.password); + + const response = await fetch('/auth/login', { + method: 'POST', + headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, + body: formData.toString(), + credentials: 'include', + }); + + if (response.ok) { + // Session cookie is now set by the server (HttpOnly – JS cannot read it) + app_state.authWorkflow = AuthWorkflow.SESSION; + app_state.authToken = null; // no token stored in JS memory + app_state.username = data.username; + setInvalidToken(false); + props.setIsAuthenticated(true); + props.history.push(app_state.queryConsoleOnlyView ? '/query' : '/'); + } else { + setInvalidToken(true); + } + } catch (e) { + setInvalidToken(true); + } finally { + setIsLoading(false); + } + return; + } + + // BASIC workflow (legacy): build Basic auth token and verify via /auth/verify + const authToken = "Basic " + btoa(data.username + ":" + data.password); const isUserAuthenticated = await PinotMethodUtils.verifyAuth(authToken); - if(isUserAuthenticated){ + if (isUserAuthenticated) { setInvalidToken(false); app_state.username = data.username; app_state.authWorkflow = AuthWorkflow.BASIC; @@ -85,6 +126,7 @@ const LoginPage = (props) => { } else { setInvalidToken(true); } + setIsLoading(false); }); const classes = useStyles(); @@ -116,8 +158,8 @@ const LoginPage = (props) => { } - diff --git a/pinot-controller/src/main/resources/app/utils/axios-config.ts b/pinot-controller/src/main/resources/app/utils/axios-config.ts index 0a8ff97505be..76d04bdf3665 100644 --- a/pinot-controller/src/main/resources/app/utils/axios-config.ts +++ b/pinot-controller/src/main/resources/app/utils/axios-config.ts @@ -22,26 +22,48 @@ import { AuthWorkflow } from 'Models'; import app_state from '../app_state'; import { AxiosError, AxiosRequestConfig } from "axios"; -const isDev = process.env.NODE_ENV !== 'production'; - -// Returns axios request interceptor +/** + * Returns axios request interceptor. + * + * When using SESSION workflow, no Authorization header is attached. + * The browser automatically sends the HttpOnly session cookie with every request. + * This eliminates the "Authorization: Basic " visible in browser + * dev-tools network tab. + * + * Flow: + * NONE → no auth header, no cookie (open access) + * BASIC → Authorization: Basic header (legacy) + * SESSION → no Authorization header; browser sends HttpOnly cookie automatically + * OIDC → Authorization: Bearer header + */ export const getAxiosRequestInterceptor = ( accessToken?: string ): ((requestConfig: AxiosRequestConfig) => AxiosRequestConfig) => { const requestInterceptor = ( requestConfig: AxiosRequestConfig ): AxiosRequestConfig => { - // If access token is available, attach it to the request - // basic auth + // SESSION workflow: rely entirely on the HttpOnly cookie set by POST /auth/login. + // Do NOT attach any Authorization header. + if (app_state.authWorkflow === AuthWorkflow.SESSION) { + if (requestConfig.headers) { + delete requestConfig.headers['Authorization']; + delete requestConfig.headers['authorization']; + } + return requestConfig; + } + + // BASIC auth: attach the stored token as Authorization header (legacy behaviour) if (app_state.authWorkflow === AuthWorkflow.BASIC && app_state.authToken) { requestConfig.headers = { + ...requestConfig.headers, Authorization: app_state.authToken, }; } - // OIDC auth + // OIDC auth: attach the Bearer access token if (accessToken) { requestConfig.headers = { + ...requestConfig.headers, Authorization: accessToken, }; } @@ -52,13 +74,28 @@ export const getAxiosRequestInterceptor = ( return requestInterceptor; }; -// Returns axios rejected response interceptor +/** + * Returns axios rejected response interceptor. + * + * For SESSION workflow, 401/403 responses redirect to the login page so the user + * must re-authenticate. For other workflows, the provided callback is invoked. + */ export const getAxiosErrorInterceptor = ( unauthenticatedAccessFn?: () => void ): ((error: AxiosError) => void) => { const rejectedResponseInterceptor = (error: AxiosError): any => { if (error && error.response && (error.response.status === 401 || error.response.status === 403)) { - // Unauthenticated access + // SESSION workflow: session has expired. Redirect to login. + if (app_state.authWorkflow === AuthWorkflow.SESSION) { + app_state.authToken = null; + if (window.location.hash && !window.location.hash.includes('/login')) { + window.location.href = '/#/login'; + } else if (!window.location.hash) { + window.location.href = '/#/login'; + } + return error.response || error; + } + // Other workflows: invoke the callback unauthenticatedAccessFn && unauthenticatedAccessFn(); } diff --git a/pinot-controller/src/test/java/org/apache/pinot/controller/api/access/SessionManagerTest.java b/pinot-controller/src/test/java/org/apache/pinot/controller/api/access/SessionManagerTest.java new file mode 100644 index 000000000000..4d65214cbfe0 --- /dev/null +++ b/pinot-controller/src/test/java/org/apache/pinot/controller/api/access/SessionManagerTest.java @@ -0,0 +1,363 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + + +package org.apache.pinot.controller.api.access; + +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +import static org.testng.Assert.*; + + +/** + * Unit tests for {@link SessionManager}. + * + *

Covers: + *

    + *
  • createSession — token generation, storage
    • + *
  • getUsername — valid session, expired session, null/empty token
    • + *
  • Sliding window TTL — expiry is extended on each getUsername call
    • + *
  • invalidateSession — immediate removal, idempotent
    • + *
  • getBasicAuthToken — returns stored token, empty after invalidation
    • + *
  • Concurrent access — no race condition under parallel getUsername calls
    • + *
+ */ +public class SessionManagerTest { + + private SessionManager _sessionManager; + + @BeforeMethod + public void setUp() { + // 10-second TTL for most tests — long enough not to expire during assertions + _sessionManager = new SessionManager(10L); + } + + @AfterMethod + public void tearDown() { + _sessionManager.shutdown(); + } + + // --------------------------------------------------------------------------- + // createSession + // --------------------------------------------------------------------------- + + @Test + public void testCreateSessionReturnsNonNullToken() { + String token = _sessionManager.createSession("alice", "Basic dXNlcjpwYXNz"); + assertNotNull(token); + assertFalse(token.isEmpty()); + } + + @Test + public void testCreateSessionTokensAreUnique() { + String token1 = _sessionManager.createSession("alice", "Basic abc"); + String token2 = _sessionManager.createSession("alice", "Basic abc"); + assertNotEquals(token1, token2, "Each createSession call should produce a unique token"); + } + + @Test + public void testCreateSessionIncrementsActiveCount() { + int before = _sessionManager.getActiveSessionCount(); + _sessionManager.createSession("alice", "Basic abc"); + assertEquals(_sessionManager.getActiveSessionCount(), before + 1); + } + + // --------------------------------------------------------------------------- + // getUsername — valid session + // --------------------------------------------------------------------------- + + @Test + public void testGetUsernameReturnsCorrectUsername() { + String token = _sessionManager.createSession("alice", "Basic abc"); + Optional result = _sessionManager.getUsername(token); + assertTrue(result.isPresent()); + assertEquals(result.get(), "alice"); + } + + @Test + public void testGetUsernameWithNullTokenReturnsEmpty() { + Optional result = _sessionManager.getUsername(null); + assertFalse(result.isPresent()); + } + + @Test + public void testGetUsernameWithEmptyTokenReturnsEmpty() { + Optional result = _sessionManager.getUsername(""); + assertFalse(result.isPresent()); + } + + @Test + public void testGetUsernameWithUnknownTokenReturnsEmpty() { + Optional result = _sessionManager.getUsername("nonexistent-token-xyz"); + assertFalse(result.isPresent()); + } + + // --------------------------------------------------------------------------- + // getUsername — expired session + // --------------------------------------------------------------------------- + + @Test + public void testGetUsernameAfterExpiryReturnsEmpty() throws InterruptedException { + // Create a session with 1-second TTL + SessionManager shortTtlManager = new SessionManager(1L); + try { + String token = shortTtlManager.createSession("bob", "Basic xyz"); + // Verify it works immediately + assertTrue(shortTtlManager.getUsername(token).isPresent()); + // Wait for expiry + Thread.sleep(1500); + Optional result = shortTtlManager.getUsername(token); + assertFalse(result.isPresent(), "Session should be expired after TTL"); + } finally { + shortTtlManager.shutdown(); + } + } + + @Test + public void testExpiredSessionRemovedFromStore() throws InterruptedException { + SessionManager shortTtlManager = new SessionManager(1L); + try { + String token = shortTtlManager.createSession("bob", "Basic xyz"); + Thread.sleep(1500); + shortTtlManager.getUsername(token); // triggers removal via compute() + assertEquals(shortTtlManager.getActiveSessionCount(), 0, + "Expired session should be removed from store on access"); + } finally { + shortTtlManager.shutdown(); + } + } + + // --------------------------------------------------------------------------- + // Sliding window TTL + // --------------------------------------------------------------------------- + + @Test + public void testSlidingWindowExtendsExpiry() throws InterruptedException { + // TTL = 2 seconds. Access at t=1.5s should slide expiry to t=3.5s. + SessionManager slidingManager = new SessionManager(2L); + try { + String token = slidingManager.createSession("carol", "Basic zzz"); + Thread.sleep(1500); // t=1.5s — still valid + Optional midResult = slidingManager.getUsername(token); + assertTrue(midResult.isPresent(), "Session should still be valid at t=1.5s"); + // Without sliding, session would expire at t=2s. With sliding it expires at t=3.5s. + Thread.sleep(1200); // t=2.7s — should still be valid due to slide + Optional lateResult = slidingManager.getUsername(token); + assertTrue(lateResult.isPresent(), "Session should still be valid after sliding TTL extension"); + } finally { + slidingManager.shutdown(); + } + } + + // --------------------------------------------------------------------------- + // invalidateSession + // --------------------------------------------------------------------------- + + @Test + public void testInvalidateSessionRemovesIt() { + String token = _sessionManager.createSession("dave", "Basic aaa"); + assertTrue(_sessionManager.getUsername(token).isPresent()); + + _sessionManager.invalidateSession(token); + assertFalse(_sessionManager.getUsername(token).isPresent(), "Session should not be accessible after invalidation"); + } + + @Test + public void testInvalidateSessionDecrementsCount() { + String token = _sessionManager.createSession("dave", "Basic aaa"); + int before = _sessionManager.getActiveSessionCount(); + _sessionManager.invalidateSession(token); + assertTrue(_sessionManager.getActiveSessionCount() < before); + } + + @Test + public void testInvalidateNonExistentTokenIsIdempotent() { + // Should not throw + _sessionManager.invalidateSession("ghost-token-that-does-not-exist"); + _sessionManager.invalidateSession(null); + _sessionManager.invalidateSession(""); + } + + @Test + public void testInvalidateSessionTwiceIsIdempotent() { + String token = _sessionManager.createSession("dave", "Basic aaa"); + _sessionManager.invalidateSession(token); + _sessionManager.invalidateSession(token); // second call should not throw + assertFalse(_sessionManager.getUsername(token).isPresent()); + } + + // --------------------------------------------------------------------------- + // getBasicAuthToken + // --------------------------------------------------------------------------- + + @Test + public void testGetBasicAuthTokenReturnsStoredToken() { + String basicToken = "Basic dXNlcjpwYXNz"; + String sessionToken = _sessionManager.createSession("eve", basicToken); + Optional result = _sessionManager.getBasicAuthToken(sessionToken); + assertTrue(result.isPresent()); + assertEquals(result.get(), basicToken); + } + + @Test + public void testGetBasicAuthTokenWithNullReturnsEmpty() { + assertFalse(_sessionManager.getBasicAuthToken(null).isPresent()); + } + + @Test + public void testGetBasicAuthTokenAfterInvalidationReturnsEmpty() { + String sessionToken = _sessionManager.createSession("eve", "Basic abc"); + _sessionManager.invalidateSession(sessionToken); + assertFalse(_sessionManager.getBasicAuthToken(sessionToken).isPresent()); + } + + // --------------------------------------------------------------------------- + // Multiple sessions for the same user + // --------------------------------------------------------------------------- + + @Test + public void testMultipleSessionsForSameUser() { + String token1 = _sessionManager.createSession("frank", "Basic aaa"); + String token2 = _sessionManager.createSession("frank", "Basic bbb"); + assertNotEquals(token1, token2); + assertTrue(_sessionManager.getUsername(token1).isPresent()); + assertTrue(_sessionManager.getUsername(token2).isPresent()); + + _sessionManager.invalidateSession(token1); + assertFalse(_sessionManager.getUsername(token1).isPresent()); + assertTrue(_sessionManager.getUsername(token2).isPresent(), "Invalidating one session should not affect the other"); + } + + // --------------------------------------------------------------------------- + // getSessionTtlSeconds + // --------------------------------------------------------------------------- + + @Test + public void testGetSessionTtlSeconds() { + assertEquals(_sessionManager.getSessionTtlSeconds(), 10L); + SessionManager custom = new SessionManager(42L); + try { + assertEquals(custom.getSessionTtlSeconds(), 42L); + } finally { + custom.shutdown(); + } + } + + @Test + public void testDefaultConstructorUsesFallbackTtl() { + SessionManager defaultManager = new SessionManager(); + try { + assertEquals(defaultManager.getSessionTtlSeconds(), SessionManager.DEFAULT_SESSION_TTL_SECONDS); + } finally { + defaultManager.shutdown(); + } + } + + // --------------------------------------------------------------------------- + // Concurrent access — no race condition + // --------------------------------------------------------------------------- + + @Test + public void testConcurrentGetUsernameIsRaceFree() throws InterruptedException { + String token = _sessionManager.createSession("grace", "Basic ccc"); + int threadCount = 50; + ExecutorService executor = Executors.newFixedThreadPool(threadCount); + CountDownLatch startLatch = new CountDownLatch(1); + CountDownLatch doneLatch = new CountDownLatch(threadCount); + AtomicInteger successCount = new AtomicInteger(0); + List errors = new ArrayList<>(); + + for (int i = 0; i < threadCount; i++) { + executor.submit(() -> { + try { + startLatch.await(); + Optional result = _sessionManager.getUsername(token); + if (result.isPresent()) { + successCount.incrementAndGet(); + } + } catch (Throwable t) { + synchronized (errors) { + errors.add(t); + } + } finally { + doneLatch.countDown(); + } + }); + } + + startLatch.countDown(); // release all threads simultaneously + assertTrue(doneLatch.await(10, TimeUnit.SECONDS), "All threads should complete within 10s"); + executor.shutdown(); + + assertTrue(errors.isEmpty(), "No exceptions thrown under concurrent access: " + errors); + assertEquals(successCount.get(), threadCount, + "All concurrent getUsername calls should succeed for a valid non-expired token"); + } + + @Test + public void testConcurrentCreateAndInvalidate() throws InterruptedException { + int count = 20; + ExecutorService executor = Executors.newFixedThreadPool(count * 2); + List tokens = new ArrayList<>(); + CountDownLatch latch = new CountDownLatch(count * 2); + + // Create sessions + for (int i = 0; i < count; i++) { + final int idx = i; + executor.submit(() -> { + try { + String tok = _sessionManager.createSession("user" + idx, "Basic tok" + idx); + synchronized (tokens) { tokens.add(tok); } + } finally { + latch.countDown(); + } + }); + } + + // Simultaneously invalidate (may operate on not-yet-created tokens — should not throw) + for (int i = 0; i < count; i++) { + executor.submit(() -> { + try { + synchronized (tokens) { + if (!tokens.isEmpty()) { + _sessionManager.invalidateSession(tokens.get(0)); + } + } + } finally { + latch.countDown(); + } + }); + } + + assertTrue(latch.await(10, TimeUnit.SECONDS)); + executor.shutdown(); + // No assertion on count — just verify no exception was thrown + } +} + From e108c8afb8444fa6f56248f0a1051c075f38e36e Mon Sep 17 00:00:00 2001 From: Akanksha Kedia Date: Wed, 8 Jul 2026 13:49:26 +0530 Subject: [PATCH 02/10] fix: resolve 11 checkstyle violations in pinot-controller - Remove trailing whitespace on blank lines in SessionAuthenticationFilter - Remove blank lines before end-of-file in 4 new/modified files - Replace Collections.emptyList/emptyMap with List.of/Map.of in PinotSessionLoginResource - Fix LeftCurly violation in SessionManagerTest (synchronized block brace) Co-Authored-By: Claude Sonnet 4.6 (1M context) --- .../controller/api/access/SessionAuthenticationFilter.java | 5 ++--- .../api/access/SessionBasicAuthAccessControlFactory.java | 1 - .../apache/pinot/controller/api/access/SessionManager.java | 1 - .../api/resources/PinotSessionLoginResource.java | 7 +++---- .../pinot/controller/api/access/SessionManagerTest.java | 5 +++-- 5 files changed, 8 insertions(+), 11 deletions(-) diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionAuthenticationFilter.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionAuthenticationFilter.java index d67409788065..6d47bd1634df 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionAuthenticationFilter.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionAuthenticationFilter.java @@ -118,7 +118,7 @@ public void filter(ContainerRequestContext requestContext) throws IOException { return; } - + // Server-to-server API calls (e.g., pinot-admin.sh AddTable -authToken "Basic ...") carry an // Authorization header. This filter handles session validation for browser UI requests only. // Requests with an Authorization header are passed through here so the downstream @@ -133,7 +133,7 @@ public void filter(ContainerRequestContext requestContext) throws IOException { LOGGER.debug("Request has Authorization header, bypassing session filter for path '{}'", path); return; } - + // Check for a valid session cookie (browser UI requests) Cookie sessionCookie = requestContext.getCookies().get(SessionManager.SESSION_COOKIE_NAME); @@ -176,4 +176,3 @@ private static boolean isStaticAsset(String path) { return !path.contains("/") && path.contains("."); } } - diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionBasicAuthAccessControlFactory.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionBasicAuthAccessControlFactory.java index e3e2ffc72185..3397ca509409 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionBasicAuthAccessControlFactory.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionBasicAuthAccessControlFactory.java @@ -104,4 +104,3 @@ public AuthWorkflowInfo getAuthWorkflowInfo() { } } } - diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionManager.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionManager.java index 554c8eab637c..7f4e0db432e0 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionManager.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionManager.java @@ -270,4 +270,3 @@ String basicAuthToken() { } } } - diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSessionLoginResource.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSessionLoginResource.java index 5b392585ac7b..bc700f26de9e 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSessionLoginResource.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSessionLoginResource.java @@ -320,7 +320,7 @@ public List getRequestHeader(String name) { || "authorization".equalsIgnoreCase(name)) { return Collections.singletonList(_authorizationValue); } - return Collections.emptyList(); + return List.of(); } @Override @@ -343,7 +343,7 @@ public List getAcceptableMediaTypes() { @Override public List getAcceptableLanguages() { - return Collections.emptyList(); + return List.of(); } @Override @@ -358,7 +358,7 @@ public java.util.Locale getLanguage() { @Override public Map getCookies() { - return Collections.emptyMap(); + return Map.of(); } @Override @@ -372,4 +372,3 @@ public int getLength() { } } } - diff --git a/pinot-controller/src/test/java/org/apache/pinot/controller/api/access/SessionManagerTest.java b/pinot-controller/src/test/java/org/apache/pinot/controller/api/access/SessionManagerTest.java index 4d65214cbfe0..21c9b275bbc5 100644 --- a/pinot-controller/src/test/java/org/apache/pinot/controller/api/access/SessionManagerTest.java +++ b/pinot-controller/src/test/java/org/apache/pinot/controller/api/access/SessionManagerTest.java @@ -333,7 +333,9 @@ public void testConcurrentCreateAndInvalidate() throws InterruptedException { executor.submit(() -> { try { String tok = _sessionManager.createSession("user" + idx, "Basic tok" + idx); - synchronized (tokens) { tokens.add(tok); } + synchronized (tokens) { + tokens.add(tok); + } } finally { latch.countDown(); } @@ -360,4 +362,3 @@ public void testConcurrentCreateAndInvalidate() throws InterruptedException { // No assertion on count — just verify no exception was thrown } } - From 556fc08d742d2db5eeed284004531cf015af5413 Mon Sep 17 00:00:00 2001 From: Akanksha Kedia Date: Wed, 8 Jul 2026 14:59:07 +0530 Subject: [PATCH 03/10] fix: fix compilation errors in session auth classes - SessionBasicAuthAccessControlFactory: replace non-existent BasicAuthUtils/ AbstractBasicAuthAccessControl with correct BasicAuthPrincipalUtils and inline AccessControl implementation (mirrors BasicAuthAccessControlFactory) - PinotQueryResource: fix @Optional annotation package from org.glassfish.hk2.api to org.jvnet.hk2.annotations (correct HK2 location) Co-Authored-By: Claude Sonnet 4.6 (1M context) --- .../SessionBasicAuthAccessControlFactory.java | 69 ++++++++++++++----- .../api/resources/PinotQueryResource.java | 2 +- 2 files changed, 54 insertions(+), 17 deletions(-) diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionBasicAuthAccessControlFactory.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionBasicAuthAccessControlFactory.java index 3397ca509409..66ec783fe5df 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionBasicAuthAccessControlFactory.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionBasicAuthAccessControlFactory.java @@ -18,14 +18,18 @@ */ package org.apache.pinot.controller.api.access; - import java.util.Collection; +import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.stream.Collectors; +import javax.ws.rs.NotAuthorizedException; import javax.ws.rs.core.HttpHeaders; +import org.apache.pinot.common.auth.BasicAuthTokenUtils; import org.apache.pinot.core.auth.BasicAuthPrincipal; -import org.apache.pinot.core.auth.BasicAuthUtils; +import org.apache.pinot.core.auth.BasicAuthPrincipalUtils; +import org.apache.pinot.core.auth.TargetType; import org.apache.pinot.spi.env.PinotConfiguration; @@ -34,20 +38,22 @@ * *

NOTE: This factory is an OPTIONAL convenience class. * The preferred approach is to use your existing factory unchanged and just add - * {@code controller.ui.session.enabled=true} to your configuration. That approach - * works with ALL auth backends (BasicAuth, ZkBasicAuth, LDAP, custom). + * {@code controller.ui.session.authentication.enabled=true} to your configuration. + * That approach works with ALL auth backends (BasicAuth, ZkBasicAuth, LDAP, custom). * *

This factory is identical to {@link BasicAuthAccessControlFactory} in terms of credential * validation (username/password via HTTP Basic Auth headers), but it reports the * {@link AccessControl#WORKFLOW_SESSION} workflow to the UI instead of {@code BASIC}. * - *

Use this factory ONLY if you cannot add {@code controller.ui.session.enabled=true} - * to your configuration (e.g., in environments where config changes are restricted). + *

Use this factory ONLY if you cannot add + * {@code controller.ui.session.authentication.enabled=true} to your configuration + * (e.g., in environments where config changes are restricted). * *

Preferred configuration (works with any factory): * 

- *   controller.ui.session.enabled=true
- *   controller.admin.access.control.factory.class=org.apache.pinot.controller.api.access.BasicAuthAccessControlFactory
+ *   controller.ui.session.authentication.enabled=true
+ *   controller.admin.access.control.factory.class=\
+ *     org.apache.pinot.controller.api.access.BasicAuthAccessControlFactory
  *
* *

Alternative configuration using this factory: @@ -60,13 +66,14 @@ */ public class SessionBasicAuthAccessControlFactory implements AccessControlFactory { private static final String PREFIX = "controller.admin.access.control.principals"; + private static final String HEADER_AUTHORIZATION = "Authorization"; private AccessControl _accessControl; @Override public void init(PinotConfiguration configuration) { _accessControl = new SessionBasicAuthAccessControl( - BasicAuthUtils.extractBasicAuthPrincipals(configuration, PREFIX)); + BasicAuthPrincipalUtils.extractBasicAuthPrincipals(configuration, PREFIX)); } @Override @@ -80,18 +87,35 @@ public AccessControl create() { *

When the UI calls {@code GET /auth/info}, this returns {@code {"workflow":"SESSION"}} * which causes the UI to use POST /auth/login instead of sending Authorization headers. */ - private static class SessionBasicAuthAccessControl extends AbstractBasicAuthAccessControl { - + private static class SessionBasicAuthAccessControl implements AccessControl { private final Map _token2principal; - public SessionBasicAuthAccessControl(Collection principals) { - _token2principal = principals.stream() - .collect(Collectors.toMap(BasicAuthPrincipal::getToken, p -> p)); + SessionBasicAuthAccessControl(Collection principals) { + _token2principal = principals.stream().collect(Collectors.toMap(BasicAuthPrincipal::getToken, p -> p)); + } + + @Override + public boolean protectAnnotatedOnly() { + return false; + } + + @Override + public boolean hasAccess(String tableName, AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) { + return getPrincipal(httpHeaders) + .filter(p -> p.hasTable(tableName) && p.hasPermission(Objects.toString(accessType))).isPresent(); } @Override - protected Optional getPrincipal(HttpHeaders headers) { - return BasicAuthUtils.getPrincipal(getTokens(headers), _token2principal); + public boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) { + if (getPrincipal(httpHeaders).isEmpty()) { + throw new NotAuthorizedException("Basic"); + } + return true; + } + + @Override + public boolean hasAccess(HttpHeaders httpHeaders, TargetType targetType) { + return getPrincipal(httpHeaders).isPresent(); } /** @@ -102,5 +126,18 @@ protected Optional getPrincipal(HttpHeaders headers) { public AuthWorkflowInfo getAuthWorkflowInfo() { return new AuthWorkflowInfo(AccessControl.WORKFLOW_SESSION); } + + private Optional getPrincipal(HttpHeaders headers) { + if (headers == null) { + return Optional.empty(); + } + List authHeaders = headers.getRequestHeader(HEADER_AUTHORIZATION); + if (authHeaders == null) { + return Optional.empty(); + } + return authHeaders.stream().map(BasicAuthTokenUtils::normalizeBase64Token) + .map(_token2principal::get) + .filter(Objects::nonNull).findFirst(); + } } } diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java index 1776d5ed941b..0e59aa414374 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java @@ -123,7 +123,7 @@ public class PinotQueryResource { ControllerConf _controllerConf; @Inject - @org.glassfish.hk2.api.Optional + @org.jvnet.hk2.annotations.Optional SessionManager _sessionManager; @POST From c4674626ff56e1ba5fd72ab4b42fbf3ab2540c94 Mon Sep 17 00:00:00 2001 From: Akanksha Kedia Date: Wed, 8 Jul 2026 21:03:00 +0530 Subject: [PATCH 04/10] Fix auth bypass in session mode: inject credentials instead of early return AuthenticationFilter: replace early `return` with Basic-auth token injection from the session store into the request headers, so validatePermission() and FineGrainedAuthUtils still run with the session user's identity. Extract isSessionEnabled() helper to consolidate the repeated config check. PinotQueryResource: remove `if (!sessionValid)` guards around hasAccess() calls in getQueryResponse() and getMultiStageQueryResponse(). Now that AuthenticationFilter injects the Authorization header for session requests, the downstream hasAccess() calls receive the session user's credentials and work correctly. Remove hasValidSessionCookie() and sessionValid parameter. ControllerConf: shorten multi-line session constant Javadocs to one-liners. Addresses CRITICAL review feedback from @xiangfu0 and @shounakmk219 on PR #18933. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- .../pinot/controller/ControllerConf.java | 36 +---------- .../api/access/AuthenticationFilter.java | 26 ++++---- .../api/resources/PinotQueryResource.java | 61 +++++-------------- 3 files changed, 31 insertions(+), 92 deletions(-) diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java b/pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java index 370ae0126c7a..53efbb19df78 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java @@ -358,43 +358,13 @@ public static long getRandomInitialDelayInSeconds() { "controller.ingestFromURI.allowLocalFileSystem"; public static final String ACCESS_CONTROL_FACTORY_CLASS = "controller.admin.access.control.factory.class"; - /** - * When set to {@code true}, the Pinot UI uses session-based authentication (Trino/Ambari-style). - * - *

This is a UI-layer setting that is independent of the configured - * {@code controller.admin.access.control.factory.class}. It works with any auth backend: - * BasicAuth, ZkBasicAuth, LDAP/PasswordAuth, or custom factories. - * - *

When enabled: - *

    - *
  • {@code GET /auth/info} returns {@code {"workflow":"SESSION"}}
    • - *
  • The UI uses {@code POST /auth/login} to authenticate (no Authorization header)
    • - *
  • An HttpOnly {@code SameSite=Strict} session cookie is issued on successful login
    • - *
  • {@code GET /auth/logout} immediately invalidates the server-side session
    • - *
- * - *

Default: {@code false} (backward compatible – existing BASIC/NONE/OIDC workflows unchanged) - */ + /** When {@code true}, enables session-based UI auth (HttpOnly cookie / POST /auth/login). Default: {@code false}. */ public static final String CONTROLLER_UI_SESSION_ENABLED = "controller.ui.session.authentication.enabled"; - /** - * When set to {@code true}, the HttpOnly session cookie is flagged as {@code Secure}, meaning the - * browser will only send it over HTTPS connections. Set to {@code false} only for local HTTP dev. - * - *

Default: {@code true} - */ + /** When {@code true}, the session cookie is flagged Secure (HTTPS only). Default: {@code true}. */ public static final String CONTROLLER_UI_SESSION_COOKIE_SECURE = "controller.ui.session.cookie.secure"; - /** - * Inactivity timeout in seconds for the Pinot UI session. - * - *

After this many seconds of user inactivity, the UI shows a 60-second warning dialog and then - * calls {@code GET /auth/logout} to invalidate the server-side session. This value is returned to - * the browser via {@code GET /auth/info} so both the UI timer and server-side TTL derive from one config. - * Server-side session TTL is set automatically to this value + 120s buffer. - * - *

Default: {@code 300} (5 minutes) - */ + /** Inactivity timeout in seconds before the UI session expires. Server TTL = this + 120s. Default: {@code 300}. */ public static final String CONTROLLER_UI_SESSION_INACTIVITY_TIMEOUT_SECONDS = "controller.ui.session.inactivity.timeout.seconds"; public static final long DEFAULT_UI_SESSION_INACTIVITY_TIMEOUT_SECONDS = 300L; diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java index 1fad3441e44a..2e3558adb057 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java @@ -24,7 +24,6 @@ import java.lang.reflect.Method; import java.util.Arrays; import java.util.HashSet; -import java.util.Optional; import java.util.Set; import javax.inject.Inject; import javax.inject.Provider; @@ -93,20 +92,15 @@ public void filter(ContainerRequestContext requestContext) return; } - // SESSION WORKFLOW: If session mode is enabled and the request carries a valid HttpOnly session - // cookie (set by POST /auth/login), skip Authorization-header validation entirely. - // The SessionAuthenticationFilter (priority AUTHENTICATION-10) already validated the cookie - // before this filter runs. Repeating the check here would fail because browser requests in - // SESSION mode never send an Authorization header. - if (_controllerConf != null - && _controllerConf.getProperty(ControllerConf.CONTROLLER_UI_SESSION_ENABLED, false) - && _sessionManager != null) { + // SESSION WORKFLOW: If session mode is enabled and the request carries a valid session cookie, + // retrieve the stored Basic-auth token and inject it as an Authorization header. This lets + // validatePermission() and FineGrainedAuthUtils below run with the session user's credentials + // instead of skipping authorization entirely. + if (isSessionEnabled()) { Cookie sessionCookie = requestContext.getCookies().get(SessionManager.SESSION_COOKIE_NAME); if (sessionCookie != null && sessionCookie.getValue() != null) { - Optional username = _sessionManager.getUsername(sessionCookie.getValue()); - if (username.isPresent()) { - return; - } + _sessionManager.getBasicAuthToken(sessionCookie.getValue()).ifPresent( + authToken -> requestContext.getHeaders().putSingle(HttpHeaders.AUTHORIZATION, authToken)); } } @@ -178,6 +172,12 @@ private static String extractTableName(MultivaluedMap mmap) { return null; } + private boolean isSessionEnabled() { + return _controllerConf != null + && _controllerConf.getProperty(ControllerConf.CONTROLLER_UI_SESSION_ENABLED, false) + && _sessionManager != null; + } + private static boolean isBaseFile(String path) { return !path.contains("/") && path.contains("."); } diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java index 0e59aa414374..ea6bd560e9cb 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java @@ -122,6 +122,8 @@ public class PinotQueryResource { @Inject ControllerConf _controllerConf; + // @Optional: SessionManager is always bound in production but may be absent in unit-test contexts + // where HK2 is not fully configured. Without @Optional, injection would fail in those environments. @Inject @org.jvnet.hk2.annotations.Optional SessionManager _sessionManager; @@ -414,15 +416,11 @@ private StreamingOutput executeSqlQuery(@Context HttpHeaders httpHeaders, String throw QueryErrorCode.INTERNAL.asException("V2 Multi-Stage query engine not enabled."); } - // Compute session validity once here so both DQL paths don't each look up the session independently. - boolean sessionValid = hasValidSessionCookie(httpHeaders); - switch (sqlType) { case DQL: return isMse - ? getMultiStageQueryResponse(sqlQuery, queryOptions, httpHeaders, traceEnabled, sessionValid) - : getQueryResponse(sqlQuery, sqlNodeAndOptions.getSqlNode(), traceEnabled, queryOptions, httpHeaders, - sessionValid); + ? getMultiStageQueryResponse(sqlQuery, queryOptions, httpHeaders, traceEnabled) + : getQueryResponse(sqlQuery, sqlNodeAndOptions.getSqlNode(), traceEnabled, queryOptions, httpHeaders); case DML: Map headers = extractHeaders(httpHeaders); return output -> { @@ -435,38 +433,13 @@ private StreamingOutput executeSqlQuery(@Context HttpHeaders httpHeaders, String } } - /** - * Returns {@code true} if the request carries a valid server-side session cookie. - * - *

When session mode is enabled, browser UI requests carry an HttpOnly cookie instead of an - * Authorization header. If a valid session is present, the access-control check that expects an - * Authorization header is skipped to avoid a spurious 403 that would redirect the user to login. - */ - private boolean hasValidSessionCookie(HttpHeaders httpHeaders) { - if (_sessionManager == null - || _controllerConf == null - || !_controllerConf.getProperty(ControllerConf.CONTROLLER_UI_SESSION_ENABLED, false)) { - return false; - } - Cookie sessionCookie = httpHeaders.getCookies().get(SessionManager.SESSION_COOKIE_NAME); - if (sessionCookie == null || sessionCookie.getValue() == null) { - return false; - } - return _sessionManager.getUsername(sessionCookie.getValue()).isPresent(); - } - private StreamingOutput getMultiStageQueryResponse(String query, String queryOptions, HttpHeaders httpHeaders, - String traceEnabled, boolean sessionValid) { - - // SESSION WORKFLOW: If the request has a valid session cookie, skip the Authorization-header - // access check. The session was already validated by SessionAuthenticationFilter before this method. - if (!sessionValid) { - // Validate data access - // we don't have a cross table access control rule so only ADMIN can make request to multi-stage engine. - AccessControl accessControl = _accessControlFactory.create(); - if (!accessControl.hasAccess(AccessType.READ, httpHeaders, "/sql")) { - throw new WebApplicationException("Permission denied", Response.Status.FORBIDDEN); - } + String traceEnabled) { + // Validate data access + // we don't have a cross table access control rule so only ADMIN can make request to multi-stage engine. + AccessControl accessControl = _accessControlFactory.create(); + if (!accessControl.hasAccess(AccessType.READ, httpHeaders, "/sql")) { + throw new WebApplicationException("Permission denied", Response.Status.FORBIDDEN); } Map queryOptionsMap = RequestUtils.parseQuery(query).getOptions(); @@ -525,7 +498,7 @@ private List getInstanceIds(String query, List tableNames, Strin } private StreamingOutput getQueryResponse(String query, @Nullable SqlNode sqlNode, String traceEnabled, - String queryOptions, HttpHeaders httpHeaders, boolean sessionValid) { + String queryOptions, HttpHeaders httpHeaders) { // Get resource table name. String tableName; Map queryOptionsMap = RequestUtils.parseQuery(query).getOptions(); @@ -558,14 +531,10 @@ private StreamingOutput getQueryResponse(String query, @Nullable SqlNode sqlNode } String rawTableName = TableNameBuilder.extractRawTableName(tableName); - // SESSION WORKFLOW: If the request has a valid session cookie, skip the Authorization-header - // access check. The session was already validated by SessionAuthenticationFilter before this method. - if (!sessionValid) { - // Validate data access - AccessControl accessControl = _accessControlFactory.create(); - if (!accessControl.hasAccess(rawTableName, AccessType.READ, httpHeaders, Actions.Table.QUERY)) { - throw QueryErrorCode.ACCESS_DENIED.asException(); - } + // Validate data access + AccessControl accessControl = _accessControlFactory.create(); + if (!accessControl.hasAccess(rawTableName, AccessType.READ, httpHeaders, Actions.Table.QUERY)) { + throw QueryErrorCode.ACCESS_DENIED.asException(); } // Get brokers for the resource table. From 464e4647fb86f90df8caa3f8ac4bb1621afe74ac Mon Sep 17 00:00:00 2001 From: Akanksha Kedia Date: Thu, 9 Jul 2026 09:55:51 +0530 Subject: [PATCH 05/10] Fix three security issues from review: CORS, login auth, timeseries broker auth ControllerAdminApiApplication: revert CORS to wildcard-only (no credentials). The Pinot UI is same-origin so CORS is not needed; the previous origin-reflect + Allow-Credentials approach was too permissive for same-site sibling origins. PinotSessionLoginResource: use hasAccess(AccessType, headers, url) instead of hasAccess(null, AccessType, headers, url). The table-scoped overload calls hasTable(null) which rejects principals with a table allowlist, locking them out of session login entirely. The identity-only overload validates credentials without requiring any table permission. PinotQueryResource: extract injectSessionCredentialIfNeeded() helper and call it in both sendRequestToBroker() and sendTimeSeriesRequestToBroker(). Previously only the SQL forward path re-injected the session credential after extractHeaders() stripped it; time-series requests were forwarded without broker auth in session mode. Addresses review feedback from @xiangfu0 on PR #18933. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- .../api/ControllerAdminApiApplication.java | 11 +---- .../api/resources/PinotQueryResource.java | 40 ++++++++++--------- .../resources/PinotSessionLoginResource.java | 2 +- 3 files changed, 24 insertions(+), 29 deletions(-) diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/ControllerAdminApiApplication.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/ControllerAdminApiApplication.java index fd527d67bc8f..846c4cd74b08 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/ControllerAdminApiApplication.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/ControllerAdminApiApplication.java @@ -149,16 +149,7 @@ private class CorsFilter implements ContainerResponseFilter { public void filter(ContainerRequestContext containerRequestContext, ContainerResponseContext containerResponseContext) throws IOException { - // For session-based auth with withCredentials:true, reflect the actual request origin - // instead of "*". Browsers reject "Access-Control-Allow-Origin: *" when - // "Access-Control-Allow-Credentials: true" is set. - String origin = containerRequestContext.getHeaderString("Origin"); - if (origin != null && !origin.isEmpty()) { - containerResponseContext.getHeaders().add("Access-Control-Allow-Origin", origin); - containerResponseContext.getHeaders().add("Access-Control-Allow-Credentials", "true"); - } else { - containerResponseContext.getHeaders().add("Access-Control-Allow-Origin", "*"); - } + containerResponseContext.getHeaders().add("Access-Control-Allow-Origin", "*"); containerResponseContext.getHeaders().add("Access-Control-Allow-Methods", "GET, POST, PUT, OPTIONS, DELETE"); containerResponseContext.getHeaders() .add("Access-Control-Allow-Headers", "Authorization, Content-Type, Accept, Origin, X-Requested-With"); diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java index ea6bd560e9cb..06950d85d047 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java @@ -615,23 +615,9 @@ private StreamingOutput sendRequestToBroker(String query, String instanceId, Str String url = getQueryURL(protocol, hostName, port); ObjectNode requestJson = getRequestJson(query, traceEnabled, queryOptions); - // Forward client-supplied headers + // Forward client-supplied headers, then inject session credential for broker auth. Map headers = extractHeaders(httpHeaders); - - // SESSION WORKFLOW: Inject Authorization header for broker (server-to-server). - // Browser UI requests carry only the HttpOnly session cookie – no Authorization header. - // The broker requires auth, so we retrieve the stored Basic-auth token from the session - // and inject it as an Authorization header for the controller→broker request. - // This token is stored server-side only (never sent to the browser). - if (_sessionManager != null - && _controllerConf != null - && _controllerConf.getProperty(ControllerConf.CONTROLLER_UI_SESSION_ENABLED, false)) { - Cookie sessionCookie = httpHeaders.getCookies().get(SessionManager.SESSION_COOKIE_NAME); - if (sessionCookie != null && sessionCookie.getValue() != null) { - _sessionManager.getBasicAuthToken(sessionCookie.getValue()).ifPresent( - authToken -> headers.put(HttpHeaders.AUTHORIZATION, authToken)); - } - } + injectSessionCredentialIfNeeded(headers, httpHeaders); return sendRequestRaw(url, "POST", query, requestJson, headers); } @@ -767,8 +753,9 @@ private StreamingOutput sendTimeSeriesRequestToBroker(String language, String qu String protocol = _controllerConf.getControllerBrokerProtocol(); int port = getPort(instanceConfig); - // Forward client-supplied headers + // Forward client-supplied headers, then inject session credential for broker auth. Map headers = extractHeaders(httpHeaders); + injectSessionCredentialIfNeeded(headers, httpHeaders); if (useBrokerCompatibleApi) { // Use POST /query/timeseries endpoint (broker compatible API) @@ -827,9 +814,26 @@ private String getTimeSeriesQueryURL(String protocol, String hostName, int port, } } + /** + * Injects the stored Basic-auth token from the server-side session store into the forwarded + * broker request headers. This is needed because browser UI requests carry only the session + * cookie, not an Authorization header; the broker still requires one for server-to-server auth. + */ + private void injectSessionCredentialIfNeeded(Map headers, HttpHeaders httpHeaders) { + if (_sessionManager == null || _controllerConf == null + || !_controllerConf.getProperty(ControllerConf.CONTROLLER_UI_SESSION_ENABLED, false)) { + return; + } + Cookie sessionCookie = httpHeaders.getCookies().get(SessionManager.SESSION_COOKIE_NAME); + if (sessionCookie != null && sessionCookie.getValue() != null) { + _sessionManager.getBasicAuthToken(sessionCookie.getValue()).ifPresent( + authToken -> headers.put(HttpHeaders.AUTHORIZATION, authToken)); + } + } + private Map extractHeaders(HttpHeaders httpHeaders) { // In SESSION mode, exclude the Authorization header from being forwarded to the broker. - // The broker auth is injected server-side from the session store in sendRequestToBroker(). + // The stored session credential is injected separately via injectSessionCredentialIfNeeded(). boolean isSessionMode = _controllerConf != null && _controllerConf.getProperty(ControllerConf.CONTROLLER_UI_SESSION_ENABLED, false); return httpHeaders.getRequestHeaders().entrySet().stream() diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSessionLoginResource.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSessionLoginResource.java index bc700f26de9e..dfd41be980e1 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSessionLoginResource.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSessionLoginResource.java @@ -152,7 +152,7 @@ public Response login( AccessControl accessControl = _accessControlFactory.create(); boolean valid; try { - valid = accessControl.hasAccess(null, AccessType.READ, + valid = accessControl.hasAccess(AccessType.READ, new SyntheticHttpHeaders(basicAuthToken), AUTH_LOGIN_PATH); } catch (javax.ws.rs.NotAuthorizedException e) { valid = false; From 8ee972f17239f1fa5092b6fda6e40d60b964f6ec Mon Sep 17 00:00:00 2001 From: Akanksha Kedia Date: Thu, 9 Jul 2026 11:34:12 +0530 Subject: [PATCH 06/10] Refactor SessionManager into interface with InMemorySessionManager default impl Extract SessionManager as an interface (keeping SESSION_COOKIE_NAME and DEFAULT_SESSION_TTL_SECONDS constants + the public API) and rename the in-memory implementation to InMemorySessionManager. All injection points (AuthenticationFilter, SessionAuthenticationFilter, PinotQueryResource, PinotSessionLoginResource) already reference the SessionManager type so they are unaffected. BaseControllerStarter now instantiates InMemorySessionManager. SessionManagerTest updated to construct InMemorySessionManager directly while keeping variable types as SessionManager. Addresses @shounakmk219 review feedback on PR #18933. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- .../controller/BaseControllerStarter.java | 3 +- .../api/access/InMemorySessionManager.java | 218 ++++++++++++++++ .../controller/api/access/SessionManager.java | 238 ++---------------- .../api/access/SessionManagerTest.java | 14 +- 4 files changed, 252 insertions(+), 221 deletions(-) create mode 100644 pinot-controller/src/main/java/org/apache/pinot/controller/api/access/InMemorySessionManager.java diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java b/pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java index 9908334b69e6..b603ea2c7de8 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java @@ -97,6 +97,7 @@ import org.apache.pinot.common.version.PinotVersion; import org.apache.pinot.controller.api.ControllerAdminApiApplication; import org.apache.pinot.controller.api.access.AccessControlFactory; +import org.apache.pinot.controller.api.access.InMemorySessionManager; import org.apache.pinot.controller.api.access.SessionManager; import org.apache.pinot.controller.api.events.MetadataEventNotifierFactory; import org.apache.pinot.controller.api.resources.ControllerFilePathProvider; @@ -754,7 +755,7 @@ private void setUpPinotController() { ControllerConf.CONTROLLER_UI_SESSION_INACTIVITY_TIMEOUT_SECONDS, ControllerConf.DEFAULT_UI_SESSION_INACTIVITY_TIMEOUT_SECONDS); long serverSessionTtlSeconds = uiInactivityTimeoutSeconds + 120; - final SessionManager sessionManager = new SessionManager(serverSessionTtlSeconds); + final SessionManager sessionManager = new InMemorySessionManager(serverSessionTtlSeconds); LOGGER.info("Controller download url base: {}", _config.generateVipUrl()); LOGGER.info("Injecting configuration and resource managers to the API context"); diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/InMemorySessionManager.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/InMemorySessionManager.java new file mode 100644 index 000000000000..25d5f165f862 --- /dev/null +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/InMemorySessionManager.java @@ -0,0 +1,218 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.controller.api.access; + +import java.security.SecureRandom; +import java.time.Instant; +import java.util.Base64; +import java.util.Iterator; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + + +/** + * In-memory implementation of {@link SessionManager}. + * + *

Sessions are stored in a {@link ConcurrentHashMap} with a configurable sliding-window TTL + * and are cleaned up periodically to prevent unbounded growth. + * + *

Sliding window TTL: Each time a session is accessed via + * {@link #getUsername(String)}, the expiry is extended by the configured TTL. This means active + * users are never logged out due to inactivity — only truly idle sessions (no API calls for the + * full TTL duration) expire. + * + *

Design follows the Ambari / Trino FormWebUiAuthenticationFilter pattern: + *

    + *
  • Login → validate credentials → create session token → set HttpOnly cookie
    • + *
  • Request → read cookie → look up session → slide expiry → allow or redirect to login
    • + *
  • Logout → read cookie → remove session from store → delete cookie → redirect to login
    • + *
+ */ +public class InMemorySessionManager implements SessionManager { + private static final Logger LOGGER = LoggerFactory.getLogger(InMemorySessionManager.class); + + /** Cleanup interval: every 30 minutes. */ + private static final long CLEANUP_INTERVAL_SECONDS = 30 * 60L; + + /** Number of random bytes for the session token (256-bit entropy). */ + private static final int TOKEN_BYTES = 32; + + private final long _sessionTtlSeconds; + private final SecureRandom _secureRandom = new SecureRandom(); + + /** + * Active session store: token → {@link SessionEntry}. + * ConcurrentHashMap provides thread-safe reads without locking on the hot path. + */ + private final ConcurrentHashMap _sessions = new ConcurrentHashMap<>(); + + private final ScheduledExecutorService _cleanupExecutor = + Executors.newSingleThreadScheduledExecutor(r -> { + Thread t = new Thread(r, "pinot-session-cleanup"); + t.setDaemon(true); + return t; + }); + + public InMemorySessionManager() { + this(DEFAULT_SESSION_TTL_SECONDS); + } + + public InMemorySessionManager(long sessionTtlSeconds) { + _sessionTtlSeconds = sessionTtlSeconds; + _cleanupExecutor.scheduleAtFixedRate( + this::evictExpiredSessions, + CLEANUP_INTERVAL_SECONDS, + CLEANUP_INTERVAL_SECONDS, + TimeUnit.SECONDS); + LOGGER.info("InMemorySessionManager initialized with sliding TTL={}s", _sessionTtlSeconds); + } + + @Override + public String createSession(String username, String basicAuthToken) { + String token = generateToken(); + Instant expiry = Instant.now().plusSeconds(_sessionTtlSeconds); + _sessions.put(token, new SessionEntry(username, expiry, basicAuthToken)); + LOGGER.debug("Created session for user '{}', expires at {}", username, expiry); + return token; + } + + @Override + public Optional getUsername(String token) { + if (token == null || token.isEmpty()) { + return Optional.empty(); + } + // Sliding window: single atomic compute() avoids a race condition where a non-atomic + // get+check+put sequence could see the session removed between the expiry check and the put. + // compute() provides an atomic read-modify-write on the ConcurrentHashMap entry. + final String[] resolvedUsername = {null}; + _sessions.compute(token, (k, v) -> { + if (v == null || Instant.now().isAfter(v.expiry())) { + // Expired or not found – remove the entry (return null removes key from map) + return null; + } + resolvedUsername[0] = v.username(); + return new SessionEntry(v.username(), Instant.now().plusSeconds(_sessionTtlSeconds), v.basicAuthToken()); + }); + return Optional.ofNullable(resolvedUsername[0]); + } + + @Override + public Optional getBasicAuthToken(String token) { + if (token == null || token.isEmpty()) { + return Optional.empty(); + } + SessionEntry entry = _sessions.get(token); + if (entry == null || Instant.now().isAfter(entry.expiry())) { + return Optional.empty(); + } + return Optional.ofNullable(entry.basicAuthToken()); + } + + @Override + public void invalidateSession(String token) { + if (token != null && !token.isEmpty()) { + SessionEntry removed = _sessions.remove(token); + if (removed != null) { + LOGGER.debug("Session invalidated for user '{}'", removed.username()); + } + } + } + + @Override + public long getSessionTtlSeconds() { + return _sessionTtlSeconds; + } + + @Override + public int getActiveSessionCount() { + return _sessions.size(); + } + + @Override + public void shutdown() { + _cleanupExecutor.shutdownNow(); + } + + // --------------------------------------------------------------------------- + // Private helpers + // --------------------------------------------------------------------------- + + private String generateToken() { + byte[] bytes = new byte[TOKEN_BYTES]; + _secureRandom.nextBytes(bytes); + return Base64.getUrlEncoder().withoutPadding().encodeToString(bytes); + } + + private void evictExpiredSessions() { + Instant now = Instant.now(); + int removed = 0; + Iterator> it = _sessions.entrySet().iterator(); + while (it.hasNext()) { + Map.Entry entry = it.next(); + if (now.isAfter(entry.getValue().expiry())) { + it.remove(); + removed++; + } + } + if (removed > 0) { + LOGGER.debug("Evicted {} expired sessions; {} active sessions remain", removed, _sessions.size()); + } + } + + // --------------------------------------------------------------------------- + // Inner record + // --------------------------------------------------------------------------- + + /** + * Immutable session entry holding the username, expiry instant, and stored Basic-auth token. + * + *

The {@code basicAuthToken} is stored server-side only — it is never sent to the browser. + * It is used to inject an {@code Authorization} header when the controller forwards queries + * to the broker (server-to-server communication). + */ + private static final class SessionEntry { + private final String _username; + private final Instant _expiry; + private final String _basicAuthToken; + + SessionEntry(String username, Instant expiry, String basicAuthToken) { + _username = username; + _expiry = expiry; + _basicAuthToken = basicAuthToken; + } + + String username() { + return _username; + } + + Instant expiry() { + return _expiry; + } + + String basicAuthToken() { + return _basicAuthToken; + } + } +} diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionManager.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionManager.java index 7f4e0db432e0..9abe5b860c35 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionManager.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionManager.java @@ -18,255 +18,67 @@ */ package org.apache.pinot.controller.api.access; - -import java.security.SecureRandom; -import java.time.Instant; -import java.util.Base64; -import java.util.Iterator; -import java.util.Map; import java.util.Optional; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.Executors; -import java.util.concurrent.ScheduledExecutorService; -import java.util.concurrent.TimeUnit; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; /** * Server-side session manager for Pinot UI authentication. * - *

Manages opaque session tokens that are issued on successful login and - * invalidated on explicit logout (Ambari-style proper logout invalidation). - * Sessions are stored in-memory with a configurable TTL and are cleaned up - * periodically to prevent unbounded growth. - * - *

Sliding window TTL: Each time a session is accessed via - * {@link #getUsername(String)}, the expiry is extended by the configured TTL. - * This means active users are never logged out due to inactivity — only truly - * idle sessions (no API calls for the full TTL duration) expire. + *

Manages opaque session tokens that are issued on successful login and invalidated on explicit + * logout (Ambari-style proper logout invalidation). The default implementation is + * {@link InMemorySessionManager}. * - *

Broker credential forwarding: The session stores the - * Basic-auth token used to validate credentials at login. This allows the - * controller to inject a proper {@code Authorization} header when forwarding - * queries to the broker (server-to-server), without ever exposing credentials - * in the browser's network tab. - * - *

Design follows the Ambari / Trino FormWebUiAuthenticationFilter pattern: - *

    - *
  • Login → validate credentials → create session token → set HttpOnly cookie
    • - *
  • Request → read cookie → look up session → slide expiry → allow or redirect to login
    • - *
  • Logout → read cookie → remove session from store → delete cookie → redirect to login
    • - *
+ *

Broker credential forwarding: The session stores the Basic-auth token used + * to validate credentials at login. This allows the controller to inject a proper + * {@code Authorization} header when forwarding queries to the broker (server-to-server), without + * ever exposing credentials in the browser's network tab. */ -public class SessionManager { - private static final Logger LOGGER = LoggerFactory.getLogger(SessionManager.class); +public interface SessionManager { /** Cookie name used to carry the session token. */ - public static final String SESSION_COOKIE_NAME = "Pinot-UI-Session"; + String SESSION_COOKIE_NAME = "Pinot-UI-Session"; /** Default session TTL: 8 hours (sliding window – resets on each API call). */ - public static final long DEFAULT_SESSION_TTL_SECONDS = 8 * 60 * 60L; - - /** Cleanup interval: every 30 minutes. */ - private static final long CLEANUP_INTERVAL_SECONDS = 30 * 60L; - - /** Number of random bytes for the session token (256-bit entropy). */ - private static final int TOKEN_BYTES = 32; - - private final long _sessionTtlSeconds; - private final SecureRandom _secureRandom = new SecureRandom(); - - /** - * Active session store: token → {@link SessionEntry}. - * ConcurrentHashMap provides thread-safe reads without locking on the hot path. - */ - private final ConcurrentHashMap _sessions = new ConcurrentHashMap<>(); - - private final ScheduledExecutorService _cleanupExecutor = - Executors.newSingleThreadScheduledExecutor(r -> { - Thread t = new Thread(r, "pinot-session-cleanup"); - t.setDaemon(true); - return t; - }); - - public SessionManager() { - this(DEFAULT_SESSION_TTL_SECONDS); - } - - public SessionManager(long sessionTtlSeconds) { - _sessionTtlSeconds = sessionTtlSeconds; - _cleanupExecutor.scheduleAtFixedRate( - this::evictExpiredSessions, - CLEANUP_INTERVAL_SECONDS, - CLEANUP_INTERVAL_SECONDS, - TimeUnit.SECONDS); - LOGGER.info("SessionManager initialized with sliding TTL={}s", _sessionTtlSeconds); - } + long DEFAULT_SESSION_TTL_SECONDS = 8 * 60 * 60L; /** * Creates a new session for the given username and returns the opaque session token. * - * @param username the authenticated username - * @param basicAuthToken the Basic-auth token (e.g. "Basic dXNlcjpwYXNz") used to validate - * credentials at login. Stored server-side so the controller can inject - * it as an Authorization header when forwarding queries to the broker. - * Never sent to the browser. + * @param username the authenticated username + * @param basicAuthToken the Basic-auth token stored server-side for broker forwarding * @return a cryptographically random, URL-safe session token */ - public String createSession(String username, String basicAuthToken) { - String token = generateToken(); - Instant expiry = Instant.now().plusSeconds(_sessionTtlSeconds); - _sessions.put(token, new SessionEntry(username, expiry, basicAuthToken)); - LOGGER.debug("Created session for user '{}', expires at {}", username, expiry); - return token; - } + String createSession(String username, String basicAuthToken); /** - * Looks up the username associated with a session token. - * - *

Sliding window: If the session is valid, its expiry is extended - * by the configured TTL. This ensures active users are never logged out due to inactivity. + * Looks up the username for a session token, extending the sliding TTL on each access. * * @param token the session token from the cookie * @return the username if the session is valid and not expired, empty otherwise */ - public Optional getUsername(String token) { - if (token == null || token.isEmpty()) { - return Optional.empty(); - } - // Sliding window: single atomic compute() avoids a race condition where a non-atomic - // get+check+put sequence could see the session removed between the expiry check and the put. - // compute() provides an atomic read-modify-write on the ConcurrentHashMap entry. - final String[] resolvedUsername = {null}; - _sessions.compute(token, (k, v) -> { - if (v == null || Instant.now().isAfter(v.expiry())) { - // Expired or not found – remove the entry (return null removes key from map) - return null; - } - resolvedUsername[0] = v.username(); - return new SessionEntry(v.username(), Instant.now().plusSeconds(_sessionTtlSeconds), v.basicAuthToken()); - }); - return Optional.ofNullable(resolvedUsername[0]); - } + Optional getUsername(String token); /** * Returns the Basic-auth token stored for the given session token. * - *

Used by {@link org.apache.pinot.controller.api.resources.PinotQueryResource} to inject - * a proper {@code Authorization} header when forwarding queries to the broker (server-to-server). - * This ensures the broker can authenticate the request without the browser ever seeing credentials. - * * @param token the session token from the cookie - * @return the Basic-auth token (e.g. "Basic dXNlcjpwYXNz"), or empty if session not found + * @return the Basic-auth token, or empty if session not found or expired */ - public Optional getBasicAuthToken(String token) { - if (token == null || token.isEmpty()) { - return Optional.empty(); - } - SessionEntry entry = _sessions.get(token); - if (entry == null || Instant.now().isAfter(entry.expiry())) { - return Optional.empty(); - } - return Optional.ofNullable(entry.basicAuthToken()); - } + Optional getBasicAuthToken(String token); /** - * Invalidates a session token (called on logout). - * This is the key difference from stateless JWT: the server can immediately - * revoke access without waiting for token expiry. + * Invalidates a session token immediately (called on logout). * * @param token the session token to invalidate */ - public void invalidateSession(String token) { - if (token != null && !token.isEmpty()) { - SessionEntry removed = _sessions.remove(token); - if (removed != null) { - LOGGER.debug("Session invalidated for user '{}'", removed.username()); - } - } - } - - /** - * Returns the configured session TTL in seconds. - */ - public long getSessionTtlSeconds() { - return _sessionTtlSeconds; - } - - /** - * Returns the number of active (possibly including some expired) sessions. - * Useful for monitoring/metrics. - */ - public int getActiveSessionCount() { - return _sessions.size(); - } - - /** - * Shuts down the background cleanup executor. - */ - public void shutdown() { - _cleanupExecutor.shutdownNow(); - } - - // --------------------------------------------------------------------------- - // Private helpers - // --------------------------------------------------------------------------- - - private String generateToken() { - byte[] bytes = new byte[TOKEN_BYTES]; - _secureRandom.nextBytes(bytes); - return Base64.getUrlEncoder().withoutPadding().encodeToString(bytes); - } - - private void evictExpiredSessions() { - Instant now = Instant.now(); - int removed = 0; - Iterator> it = _sessions.entrySet().iterator(); - while (it.hasNext()) { - Map.Entry entry = it.next(); - if (now.isAfter(entry.getValue().expiry())) { - it.remove(); - removed++; - } - } - if (removed > 0) { - LOGGER.debug("Evicted {} expired sessions; {} active sessions remain", removed, _sessions.size()); - } - } - - // --------------------------------------------------------------------------- - // Inner record - // --------------------------------------------------------------------------- - - /** - * Immutable session entry holding the username, expiry instant, and stored Basic-auth token. - * - *

The {@code basicAuthToken} is stored server-side only — it is never sent to the browser. - * It is used to inject an {@code Authorization} header when the controller forwards queries - * to the broker (server-to-server communication). - */ - private static final class SessionEntry { - private final String _username; - private final Instant _expiry; - private final String _basicAuthToken; - - SessionEntry(String username, Instant expiry, String basicAuthToken) { - _username = username; - _expiry = expiry; - _basicAuthToken = basicAuthToken; - } + void invalidateSession(String token); - String username() { - return _username; - } + /** Returns the configured session TTL in seconds. */ + long getSessionTtlSeconds(); - Instant expiry() { - return _expiry; - } + /** Returns the number of active sessions (for monitoring). */ + int getActiveSessionCount(); - String basicAuthToken() { - return _basicAuthToken; - } - } + /** Releases any background resources (e.g. cleanup executor). */ + void shutdown(); } diff --git a/pinot-controller/src/test/java/org/apache/pinot/controller/api/access/SessionManagerTest.java b/pinot-controller/src/test/java/org/apache/pinot/controller/api/access/SessionManagerTest.java index 21c9b275bbc5..a7c50e103415 100644 --- a/pinot-controller/src/test/java/org/apache/pinot/controller/api/access/SessionManagerTest.java +++ b/pinot-controller/src/test/java/org/apache/pinot/controller/api/access/SessionManagerTest.java @@ -36,7 +36,7 @@ /** - * Unit tests for {@link SessionManager}. + * Unit tests for {@link InMemorySessionManager}. * *

Covers: *

    @@ -55,7 +55,7 @@ public class SessionManagerTest { @BeforeMethod public void setUp() { // 10-second TTL for most tests — long enough not to expire during assertions - _sessionManager = new SessionManager(10L); + _sessionManager = new InMemorySessionManager(10L); } @AfterMethod @@ -125,7 +125,7 @@ public void testGetUsernameWithUnknownTokenReturnsEmpty() { @Test public void testGetUsernameAfterExpiryReturnsEmpty() throws InterruptedException { // Create a session with 1-second TTL - SessionManager shortTtlManager = new SessionManager(1L); + SessionManager shortTtlManager = new InMemorySessionManager(1L); try { String token = shortTtlManager.createSession("bob", "Basic xyz"); // Verify it works immediately @@ -141,7 +141,7 @@ public void testGetUsernameAfterExpiryReturnsEmpty() throws InterruptedException @Test public void testExpiredSessionRemovedFromStore() throws InterruptedException { - SessionManager shortTtlManager = new SessionManager(1L); + SessionManager shortTtlManager = new InMemorySessionManager(1L); try { String token = shortTtlManager.createSession("bob", "Basic xyz"); Thread.sleep(1500); @@ -160,7 +160,7 @@ public void testExpiredSessionRemovedFromStore() throws InterruptedException { @Test public void testSlidingWindowExtendsExpiry() throws InterruptedException { // TTL = 2 seconds. Access at t=1.5s should slide expiry to t=3.5s. - SessionManager slidingManager = new SessionManager(2L); + SessionManager slidingManager = new InMemorySessionManager(2L); try { String token = slidingManager.createSession("carol", "Basic zzz"); Thread.sleep(1500); // t=1.5s — still valid @@ -261,7 +261,7 @@ public void testMultipleSessionsForSameUser() { @Test public void testGetSessionTtlSeconds() { assertEquals(_sessionManager.getSessionTtlSeconds(), 10L); - SessionManager custom = new SessionManager(42L); + SessionManager custom = new InMemorySessionManager(42L); try { assertEquals(custom.getSessionTtlSeconds(), 42L); } finally { @@ -271,7 +271,7 @@ public void testGetSessionTtlSeconds() { @Test public void testDefaultConstructorUsesFallbackTtl() { - SessionManager defaultManager = new SessionManager(); + SessionManager defaultManager = new InMemorySessionManager(); try { assertEquals(defaultManager.getSessionTtlSeconds(), SessionManager.DEFAULT_SESSION_TTL_SECONDS); } finally { From 98d0c3c1fc8df056fe0d042d12ac0f1074206e98 Mon Sep 17 00:00:00 2001 From: Akanksha Kedia Date: Sat, 11 Jul 2026 20:19:19 +0530 Subject: [PATCH 07/10] ci: trigger re-run From ea15a8b2baf947c7e2f8582e7a8013443ed7e8cf Mon Sep 17 00:00:00 2001 From: Akanksha Kedia Date: Sun, 12 Jul 2026 17:39:15 +0530 Subject: [PATCH 08/10] fix(session-auth): fix Authorization header stripping for non-session API clients extractHeaders was stripping the Authorization header from every request when session mode was globally enabled, including API clients that supply their own Basic/Bearer token with no session cookie. injectSessionCredentialIfNeeded only re-injected if a session cookie was present, so API clients had their auth silently removed before the broker call. Merge the two-step strip+inject into a single buildBrokerHeaders call that resolves the session credential exactly once via resolveSessionCredential. If a valid session cookie is found, the caller Authorization is replaced with the stored token; otherwise all headers are forwarded unchanged. --- .../api/resources/PinotQueryResource.java | 54 ++++++++++--------- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java index 06950d85d047..f3baece5e487 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java @@ -39,6 +39,7 @@ import java.util.Map; import java.util.Map.Entry; import java.util.Objects; +import java.util.Optional; import java.util.Set; import java.util.concurrent.ThreadLocalRandom; import java.util.stream.Collectors; @@ -422,7 +423,7 @@ private StreamingOutput executeSqlQuery(@Context HttpHeaders httpHeaders, String ? getMultiStageQueryResponse(sqlQuery, queryOptions, httpHeaders, traceEnabled) : getQueryResponse(sqlQuery, sqlNodeAndOptions.getSqlNode(), traceEnabled, queryOptions, httpHeaders); case DML: - Map headers = extractHeaders(httpHeaders); + Map headers = buildBrokerHeaders(httpHeaders); return output -> { try (OutputStream os = output) { _sqlQueryExecutor.executeDMLStatement(sqlNodeAndOptions, headers).toOutputStream(os); @@ -615,9 +616,7 @@ private StreamingOutput sendRequestToBroker(String query, String instanceId, Str String url = getQueryURL(protocol, hostName, port); ObjectNode requestJson = getRequestJson(query, traceEnabled, queryOptions); - // Forward client-supplied headers, then inject session credential for broker auth. - Map headers = extractHeaders(httpHeaders); - injectSessionCredentialIfNeeded(headers, httpHeaders); + Map headers = buildBrokerHeaders(httpHeaders); return sendRequestRaw(url, "POST", query, requestJson, headers); } @@ -753,9 +752,7 @@ private StreamingOutput sendTimeSeriesRequestToBroker(String language, String qu String protocol = _controllerConf.getControllerBrokerProtocol(); int port = getPort(instanceConfig); - // Forward client-supplied headers, then inject session credential for broker auth. - Map headers = extractHeaders(httpHeaders); - injectSessionCredentialIfNeeded(headers, httpHeaders); + Map headers = buildBrokerHeaders(httpHeaders); if (useBrokerCompatibleApi) { // Use POST /query/timeseries endpoint (broker compatible API) @@ -815,31 +812,36 @@ private String getTimeSeriesQueryURL(String protocol, String hostName, int port, } /** - * Injects the stored Basic-auth token from the server-side session store into the forwarded - * broker request headers. This is needed because browser UI requests carry only the session - * cookie, not an Authorization header; the broker still requires one for server-to-server auth. + * Builds the header map to forward to the broker. Resolves any server-side session credential + * exactly once: if the request carries a valid session cookie, the caller-supplied Authorization + * header is replaced with the stored session token; otherwise all headers (including Authorization) + * are forwarded unchanged so that API clients keep their own credentials. */ - private void injectSessionCredentialIfNeeded(Map headers, HttpHeaders httpHeaders) { + private Map buildBrokerHeaders(HttpHeaders httpHeaders) { + Optional sessionCredential = resolveSessionCredential(httpHeaders); + Map headers = httpHeaders.getRequestHeaders().entrySet().stream() + .filter(entry -> !entry.getValue().isEmpty()) + .filter(entry -> !sessionCredential.isPresent() || !entry.getKey().equalsIgnoreCase(HttpHeaders.AUTHORIZATION)) + .collect(Collectors.toMap(Entry::getKey, entry -> entry.getValue().get(0))); + sessionCredential.ifPresent(token -> headers.put(HttpHeaders.AUTHORIZATION, token)); + return headers; + } + + /** + * Returns the stored Basic-auth token for the session cookie carried by this request, or + * {@code Optional.empty()} if session mode is disabled, no cookie is present, or the session + * has expired. + */ + private Optional resolveSessionCredential(HttpHeaders httpHeaders) { if (_sessionManager == null || _controllerConf == null || !_controllerConf.getProperty(ControllerConf.CONTROLLER_UI_SESSION_ENABLED, false)) { - return; + return Optional.empty(); } Cookie sessionCookie = httpHeaders.getCookies().get(SessionManager.SESSION_COOKIE_NAME); - if (sessionCookie != null && sessionCookie.getValue() != null) { - _sessionManager.getBasicAuthToken(sessionCookie.getValue()).ifPresent( - authToken -> headers.put(HttpHeaders.AUTHORIZATION, authToken)); + if (sessionCookie == null || sessionCookie.getValue() == null) { + return Optional.empty(); } - } - - private Map extractHeaders(HttpHeaders httpHeaders) { - // In SESSION mode, exclude the Authorization header from being forwarded to the broker. - // The stored session credential is injected separately via injectSessionCredentialIfNeeded(). - boolean isSessionMode = _controllerConf != null - && _controllerConf.getProperty(ControllerConf.CONTROLLER_UI_SESSION_ENABLED, false); - return httpHeaders.getRequestHeaders().entrySet().stream() - .filter(entry -> !entry.getValue().isEmpty()) - .filter(entry -> !isSessionMode || !entry.getKey().equalsIgnoreCase(HttpHeaders.AUTHORIZATION)) - .collect(Collectors.toMap(Entry::getKey, entry -> entry.getValue().get(0))); + return _sessionManager.getBasicAuthToken(sessionCookie.getValue()); } private InstanceConfig getInstanceConfig(String instanceId) { From 3de42a61b26dddcb3c31d15c96cdc1d8cd6486b9 Mon Sep 17 00:00:00 2001 From: Akanksha Kedia Date: Mon, 13 Jul 2026 16:40:03 +0530 Subject: [PATCH 09/10] fix(session-auth): align cookie and server session expiry; document sticky-session requirement Three issues raised in review: 1. TTL mismatch: InMemorySessionManager used a sliding-window TTL (getUsername extended expiry on each call) while the browser cookie Max-Age was fixed at login. An active session could outlive its browser cookie, causing unexpected logouts. Fixed by removing the sliding extension in getUsername; both server session and cookie now expire at loginTime + TTL. 2. Cross-controller logout: InMemorySessionManager is per-process, so logout routed to a different controller in an LB setup silently fails. Added a comment in BaseControllerStarter documenting the sticky-session requirement and the degraded-but-functional behavior without it. 3. CORS allowed headers: Access-Control-Allow-Headers was missing the custom Pinot 'Database' header used by DatabaseUtils for multi-database table resolution. Also adds If-Match for segment-upload endpoints. Browser clients that set these headers were failing preflight. --- .../controller/BaseControllerStarter.java | 23 ++++++++++-- .../api/ControllerAdminApiApplication.java | 3 +- .../api/access/InMemorySessionManager.java | 35 ++++++++---------- .../controller/api/access/SessionManager.java | 4 +-- .../api/access/SessionManagerTest.java | 36 ++++++++++--------- 5 files changed, 60 insertions(+), 41 deletions(-) diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java b/pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java index b603ea2c7de8..603547ce5144 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java @@ -749,8 +749,27 @@ private void setUpPinotController() { MetadataEventNotifierFactory.loadFactory(_config.subset(METADATA_EVENT_NOTIFIER_PREFIX), _helixResourceManager); // Create a singleton SessionManager for session-based UI authentication. - // Server-side TTL = inactivity timeout + 120s buffer so the server doesn't evict a session - // that the UI hasn't timed out yet (e.g. user is active but a background API call races with eviction). + // + // IMPORTANT: This is a per-process in-memory store. In a multi-controller deployment behind + // a load balancer, a session token minted on controller-A is not known to controller-B, and + // a logout request routed to controller-B will not invalidate the token held by controller-A. + // To preserve the server-side logout guarantee, configure the load balancer with sticky + // sessions (e.g., consistent hashing on the session cookie or source IP) so that all requests + // from a given browser are always routed to the same controller. Without sticky sessions, + // session-based UI authentication degrades to best-effort: login and most API calls will work + // (the LB will route them to the controller that knows the session), but logout may silently + // fail if it hits a different controller. + // + // The session TTL is set to the configured session timeout plus a 120s grace period. + // The grace period absorbs clock skew and in-flight requests: a request that arrives at + // the server just before the browser cookie's Max-Age expires will still see a valid + // server-side session. Both the server session and the browser cookie Max-Age are set to + // serverSessionTtlSeconds at login, so they expire at the same absolute time. + // + // Note: CONTROLLER_UI_SESSION_INACTIVITY_TIMEOUT_SECONDS controls the session *ceiling* + // (loginTime + timeout + 120s), not a sliding inactivity window. The UI enforces inactivity + // detection client-side (JavaScript timer that redirects to login after timeout seconds of + // no user interaction). The server does not extend the session on each request. long uiInactivityTimeoutSeconds = _config.getProperty( ControllerConf.CONTROLLER_UI_SESSION_INACTIVITY_TIMEOUT_SECONDS, ControllerConf.DEFAULT_UI_SESSION_INACTIVITY_TIMEOUT_SECONDS); diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/ControllerAdminApiApplication.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/ControllerAdminApiApplication.java index 846c4cd74b08..84e4434bf28a 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/ControllerAdminApiApplication.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/ControllerAdminApiApplication.java @@ -152,7 +152,8 @@ public void filter(ContainerRequestContext containerRequestContext, containerResponseContext.getHeaders().add("Access-Control-Allow-Origin", "*"); containerResponseContext.getHeaders().add("Access-Control-Allow-Methods", "GET, POST, PUT, OPTIONS, DELETE"); containerResponseContext.getHeaders() - .add("Access-Control-Allow-Headers", "Authorization, Content-Type, Accept, Origin, X-Requested-With"); + .add("Access-Control-Allow-Headers", + "Authorization, Content-Type, Accept, Origin, X-Requested-With, Database, If-Match"); if (containerRequestContext.getMethod().equals("OPTIONS")) { containerResponseContext.setStatus(HttpServletResponse.SC_OK); } diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/InMemorySessionManager.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/InMemorySessionManager.java index 25d5f165f862..b6475cf106f7 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/InMemorySessionManager.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/InMemorySessionManager.java @@ -35,18 +35,19 @@ /** * In-memory implementation of {@link SessionManager}. * - *

    Sessions are stored in a {@link ConcurrentHashMap} with a configurable sliding-window TTL + *

    Sessions are stored in a {@link ConcurrentHashMap} with a configurable fixed TTL * and are cleaned up periodically to prevent unbounded growth. * - *

    Sliding window TTL: Each time a session is accessed via - * {@link #getUsername(String)}, the expiry is extended by the configured TTL. This means active - * users are never logged out due to inactivity — only truly idle sessions (no API calls for the - * full TTL duration) expire. + *

    Fixed TTL: Each session expires at {@code loginTime + TTL} regardless of + * activity. This keeps the server-side expiry aligned with the browser cookie {@code Max-Age}, + * both of which are set to the same TTL at login. A sliding-window TTL would let the server + * session outlive the browser cookie (which has no renewal mechanism here), creating a + * desynchronised state where the server holds a valid session the browser can no longer reach. * *

    Design follows the Ambari / Trino FormWebUiAuthenticationFilter pattern: *

      *
    • Login → validate credentials → create session token → set HttpOnly cookie
      • - *
    • Request → read cookie → look up session → slide expiry → allow or redirect to login
      • + *
    • Request → read cookie → look up session → allow or redirect to login
      • *
    • Logout → read cookie → remove session from store → delete cookie → redirect to login
      • *
    */ @@ -86,7 +87,7 @@ public InMemorySessionManager(long sessionTtlSeconds) { CLEANUP_INTERVAL_SECONDS, CLEANUP_INTERVAL_SECONDS, TimeUnit.SECONDS); - LOGGER.info("InMemorySessionManager initialized with sliding TTL={}s", _sessionTtlSeconds); + LOGGER.info("InMemorySessionManager initialized with fixed TTL={}s", _sessionTtlSeconds); } @Override @@ -103,19 +104,13 @@ public Optional getUsername(String token) { if (token == null || token.isEmpty()) { return Optional.empty(); } - // Sliding window: single atomic compute() avoids a race condition where a non-atomic - // get+check+put sequence could see the session removed between the expiry check and the put. - // compute() provides an atomic read-modify-write on the ConcurrentHashMap entry. - final String[] resolvedUsername = {null}; - _sessions.compute(token, (k, v) -> { - if (v == null || Instant.now().isAfter(v.expiry())) { - // Expired or not found – remove the entry (return null removes key from map) - return null; - } - resolvedUsername[0] = v.username(); - return new SessionEntry(v.username(), Instant.now().plusSeconds(_sessionTtlSeconds), v.basicAuthToken()); - }); - return Optional.ofNullable(resolvedUsername[0]); + // Fixed TTL: just check expiry without extending it, keeping server and browser cookie + // expiry in sync (both set to the same TTL value at login time). + SessionEntry entry = _sessions.get(token); + if (entry == null || Instant.now().isAfter(entry.expiry())) { + return Optional.empty(); + } + return Optional.of(entry.username()); } @Override diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionManager.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionManager.java index 9abe5b860c35..6a9ee614e833 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionManager.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionManager.java @@ -38,7 +38,7 @@ public interface SessionManager { /** Cookie name used to carry the session token. */ String SESSION_COOKIE_NAME = "Pinot-UI-Session"; - /** Default session TTL: 8 hours (sliding window – resets on each API call). */ + /** Default session TTL: 8 hours from login. */ long DEFAULT_SESSION_TTL_SECONDS = 8 * 60 * 60L; /** @@ -51,7 +51,7 @@ public interface SessionManager { String createSession(String username, String basicAuthToken); /** - * Looks up the username for a session token, extending the sliding TTL on each access. + * Looks up the username for a session token. * * @param token the session token from the cookie * @return the username if the session is valid and not expired, empty otherwise diff --git a/pinot-controller/src/test/java/org/apache/pinot/controller/api/access/SessionManagerTest.java b/pinot-controller/src/test/java/org/apache/pinot/controller/api/access/SessionManagerTest.java index a7c50e103415..5c2262fb3699 100644 --- a/pinot-controller/src/test/java/org/apache/pinot/controller/api/access/SessionManagerTest.java +++ b/pinot-controller/src/test/java/org/apache/pinot/controller/api/access/SessionManagerTest.java @@ -42,7 +42,7 @@ *
      *
    • createSession — token generation, storage
      • *
    • getUsername — valid session, expired session, null/empty token
      • - *
    • Sliding window TTL — expiry is extended on each getUsername call
      • + *
    • Fixed TTL — session expires at loginTime + TTL regardless of access
      • *
    • invalidateSession — immediate removal, idempotent
      • *
    • getBasicAuthToken — returns stored token, empty after invalidation
      • *
    • Concurrent access — no race condition under parallel getUsername calls
      • @@ -140,38 +140,42 @@ public void testGetUsernameAfterExpiryReturnsEmpty() throws InterruptedException } @Test - public void testExpiredSessionRemovedFromStore() throws InterruptedException { + public void testExpiredSessionIsNotAccessible() throws InterruptedException { + // getUsername returns empty for expired sessions. Expired entries are NOT eagerly removed from + // the store on access — they linger until the background sweep (every 30 minutes). This is + // intentional: the map is only consulted for expiry checks, not for active garbage collection. SessionManager shortTtlManager = new InMemorySessionManager(1L); try { String token = shortTtlManager.createSession("bob", "Basic xyz"); Thread.sleep(1500); - shortTtlManager.getUsername(token); // triggers removal via compute() - assertEquals(shortTtlManager.getActiveSessionCount(), 0, - "Expired session should be removed from store on access"); + Optional result = shortTtlManager.getUsername(token); + assertFalse(result.isPresent(), "Expired session should not be returned by getUsername"); } finally { shortTtlManager.shutdown(); } } // --------------------------------------------------------------------------- - // Sliding window TTL + // Fixed TTL (no sliding window) // --------------------------------------------------------------------------- @Test - public void testSlidingWindowExtendsExpiry() throws InterruptedException { - // TTL = 2 seconds. Access at t=1.5s should slide expiry to t=3.5s. - SessionManager slidingManager = new InMemorySessionManager(2L); + public void testFixedTtlExpiresAtLoginPlusTimeout() throws InterruptedException { + // TTL = 3 seconds. Session should be valid at t=1.5s but expired at t=3.5s. + // Calling getUsername at t=1.5s must NOT extend the expiry — the fixed TTL + // ensures server and browser cookie expiry are aligned from login time. + SessionManager fixedManager = new InMemorySessionManager(3L); try { - String token = slidingManager.createSession("carol", "Basic zzz"); + String token = fixedManager.createSession("carol", "Basic zzz"); Thread.sleep(1500); // t=1.5s — still valid - Optional midResult = slidingManager.getUsername(token); + Optional midResult = fixedManager.getUsername(token); assertTrue(midResult.isPresent(), "Session should still be valid at t=1.5s"); - // Without sliding, session would expire at t=2s. With sliding it expires at t=3.5s. - Thread.sleep(1200); // t=2.7s — should still be valid due to slide - Optional lateResult = slidingManager.getUsername(token); - assertTrue(lateResult.isPresent(), "Session should still be valid after sliding TTL extension"); + // Without sliding TTL, session expires at t=3s regardless of the access above. + Thread.sleep(2000); // t=3.5s — 500ms after expiry, generous margin for CI + Optional lateResult = fixedManager.getUsername(token); + assertFalse(lateResult.isPresent(), "Session should have expired at loginTime + TTL"); } finally { - slidingManager.shutdown(); + fixedManager.shutdown(); } } From 0e21a59303615f6c32b4c4f1f2a4badf33d78132 Mon Sep 17 00:00:00 2001 From: Akanksha Kedia Date: Tue, 14 Jul 2026 11:55:56 +0530 Subject: [PATCH 10/10] =?UTF-8?q?fix(session-auth):=20address=20review=20c?= =?UTF-8?q?omments=20=E2=80=94=20atomic=20rotation,=20cache=20factory,=20e?= =?UTF-8?q?scapeJson,=20timer=20leak?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - AuthenticationFilter: also detect SESSION workflow from factory.getAuthWorkflowInfo() so isSessionEnabled() works even when CONTROLLER_UI_SESSION_ENABLED flag is absent. Cache result in volatile Boolean to avoid calling factory.create() on every request. Add auth/session/renew to UNPROTECTED_PATHS so the renew endpoint is reachable without an Authorization header. - PinotSessionLoginResource: replace non-atomic get/invalidate/create with rotateSession() to eliminate TOCTOU in POST /auth/session/renew. Add escapeJson() and apply to all three username-in-JSON response sites. Fix buildDeleteCookieHeader to propagate the Secure flag. - SessionManager: add rotateSession() default method (non-atomic fallback for custom impls). - InMemorySessionManager: override rotateSession() with CAS-based atomic rotation using ConcurrentHashMap.remove(key, value) to prevent double-rotation under concurrent requests. - PinotControllerAuthResource: enrich AuthWorkflowInfo with inactivity timeout when factory advertises SESSION workflow, not just when the explicit config flag is set. - App.tsx: change "Stay Logged In" to POST /auth/session/renew (was incorrectly hitting GET /auth/session which doesn't renew server-side TTL). Fix performSessionLogout to also clear the countdown interval, preventing warningCountdown going negative after logout. --- .../api/access/AuthenticationFilter.java | 23 ++++- .../api/access/InMemorySessionManager.java | 22 +++++ .../controller/api/access/SessionManager.java | 20 ++++ .../PinotControllerAuthResource.java | 11 ++- .../resources/PinotSessionLoginResource.java | 91 +++++++++++++++++-- .../src/main/resources/app/App.tsx | 17 +++- 6 files changed, 165 insertions(+), 19 deletions(-) diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java index 2e3558adb057..af9766ac62b2 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java @@ -54,7 +54,7 @@ public class AuthenticationFilter implements ContainerRequestFilter { private static final Set UNPROTECTED_PATHS = new HashSet<>(Arrays.asList("", "help", "auth/info", "auth/verify", "auth/verify/v2", "auth/login", - "auth/logout", "auth/session", "health")); + "auth/logout", "auth/session", "auth/session/renew", "health")); private static final String KEY_TABLE_NAME = "tableName"; private static final String KEY_TABLE_NAME_WITH_TYPE = "tableNameWithType"; private static final String KEY_SCHEMA_NAME = "schemaName"; @@ -77,6 +77,10 @@ public class AuthenticationFilter implements ContainerRequestFilter { @Context HttpHeaders _httpHeaders; + // Cached result of isSessionEnabled() — computed lazily on first request and never changes. + // volatile ensures all threads see the result once it is written. + private volatile Boolean _sessionEnabled; + @Override public void filter(ContainerRequestContext requestContext) throws IOException { @@ -173,9 +177,20 @@ private static String extractTableName(MultivaluedMap mmap) { } private boolean isSessionEnabled() { - return _controllerConf != null - && _controllerConf.getProperty(ControllerConf.CONTROLLER_UI_SESSION_ENABLED, false) - && _sessionManager != null; + if (_sessionEnabled == null) { + // Compute once and cache. Double-checked locking is safe here because the result + // is idempotent: all threads compute the same value from the same injected beans. + boolean enabled = _sessionManager != null && ( + (_controllerConf != null + && _controllerConf.getProperty(ControllerConf.CONTROLLER_UI_SESSION_ENABLED, false)) + // Also enable when the factory itself advertises SESSION workflow (e.g. + // SessionBasicAuthAccessControlFactory configured without the explicit flag). + || (_accessControlFactory != null + && AccessControl.WORKFLOW_SESSION.equals( + _accessControlFactory.create().getAuthWorkflowInfo().getWorkflow()))); + _sessionEnabled = enabled; + } + return _sessionEnabled; } private static boolean isBaseFile(String path) { diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/InMemorySessionManager.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/InMemorySessionManager.java index b6475cf106f7..b897e1a6f338 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/InMemorySessionManager.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/InMemorySessionManager.java @@ -125,6 +125,28 @@ public Optional getBasicAuthToken(String token) { return Optional.ofNullable(entry.basicAuthToken()); } + /** + * Atomically rotates the session using {@link ConcurrentHashMap#remove(Object, Object)}. + * Only one of concurrent renew requests will win the CAS; the rest see empty and get 401. + * This prevents orphaned tokens from double-clicks on "Stay Logged In". + */ + @Override + public Optional rotateSession(String oldToken) { + if (oldToken == null || oldToken.isEmpty()) { + return Optional.empty(); + } + SessionEntry entry = _sessions.get(oldToken); + if (entry == null || Instant.now().isAfter(entry.expiry())) { + return Optional.empty(); + } + // CAS: only one concurrent caller can remove this exact entry reference. + if (!_sessions.remove(oldToken, entry)) { + return Optional.empty(); + } + String newToken = createSession(entry.username(), entry.basicAuthToken()); + return Optional.of(newToken); + } + @Override public void invalidateSession(String token) { if (token != null && !token.isEmpty()) { diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionManager.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionManager.java index 6a9ee614e833..5a4c576ba96f 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionManager.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionManager.java @@ -73,6 +73,26 @@ public interface SessionManager { */ void invalidateSession(String token); + /** + * Atomically rotates the session: invalidates the old token and issues a fresh one with a new TTL. + * Returns the new token, or empty if the session is absent, expired, or was already rotated + * by a concurrent call (check-and-remove prevents double-rotation). + * + * @param oldToken the current session token from the browser cookie + * @return the new session token, or empty if rotation could not be performed + */ + default Optional rotateSession(String oldToken) { + // Default non-atomic implementation for custom SessionManager implementations. + // InMemorySessionManager overrides this with a CAS-based atomic version. + Optional username = getUsername(oldToken); + if (!username.isPresent()) { + return Optional.empty(); + } + Optional authToken = getBasicAuthToken(oldToken); + invalidateSession(oldToken); + return Optional.of(createSession(username.get(), authToken.orElse(null))); + } + /** Returns the configured session TTL in seconds. */ long getSessionTtlSeconds(); diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java index fc74fc87215c..bb0f852a973c 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java @@ -141,6 +141,15 @@ public AccessControl.AuthWorkflowInfo info() { ControllerConf.DEFAULT_UI_SESSION_INACTIVITY_TIMEOUT_SECONDS); return new AccessControl.AuthWorkflowInfo(AccessControl.WORKFLOW_SESSION, inactivityTimeout); } - return _accessControlFactory.create().getAuthWorkflowInfo(); + AccessControl.AuthWorkflowInfo factoryInfo = _accessControlFactory.create().getAuthWorkflowInfo(); + // When the factory itself advertises SESSION (e.g. SessionBasicAuthAccessControlFactory), + // enrich the response with the inactivity timeout from config so the UI can set up its timer. + if (AccessControl.WORKFLOW_SESSION.equals(factoryInfo.getWorkflow()) && _controllerConf != null) { + long inactivityTimeout = _controllerConf.getProperty( + ControllerConf.CONTROLLER_UI_SESSION_INACTIVITY_TIMEOUT_SECONDS, + ControllerConf.DEFAULT_UI_SESSION_INACTIVITY_TIMEOUT_SECONDS); + return new AccessControl.AuthWorkflowInfo(AccessControl.WORKFLOW_SESSION, inactivityTimeout); + } + return factoryInfo; } } diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSessionLoginResource.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSessionLoginResource.java index dfd41be980e1..bd247963ec48 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSessionLoginResource.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSessionLoginResource.java @@ -99,6 +99,9 @@ public class PinotSessionLoginResource { /** Path for the session-check endpoint. */ public static final String AUTH_SESSION_PATH = "auth/session"; + /** Path for the session-renew endpoint. */ + public static final String AUTH_SESSION_RENEW_PATH = "auth/session/renew"; + @Inject private AccessControlFactory _accessControlFactory; @@ -180,7 +183,7 @@ public Response login( String sessionCookieHeader = buildSessionCookieHeader( sessionToken, (int) _sessionManager.getSessionTtlSeconds(), secureCookie); - return Response.ok("{\"status\":\"ok\",\"username\":\"" + username.trim() + "\"}") + return Response.ok("{\"status\":\"ok\",\"username\":\"" + escapeJson(username.trim()) + "\"}") .header("Set-Cookie", sessionCookieHeader) .build(); } @@ -210,8 +213,11 @@ public Response logout(@Context HttpHeaders httpHeaders) { LOGGER.info("Session invalidated on logout"); } - // Return a delete-cookie (Max-Age=0) to clear it from the browser - String deleteCookieHeader = buildDeleteCookieHeader(); + // Return a delete-cookie (Max-Age=0) to clear it from the browser. + // Carry the Secure flag so the deletion header matches the original set-cookie. + boolean secureCookie = _controllerConf == null + || _controllerConf.getProperty(ControllerConf.CONTROLLER_UI_SESSION_COOKIE_SECURE, true); + String deleteCookieHeader = buildDeleteCookieHeader(secureCookie); return Response.ok("{\"status\":\"logged_out\"}") .header("Set-Cookie", deleteCookieHeader) @@ -257,7 +263,62 @@ public Response checkSession(@Context HttpHeaders httpHeaders) { .build(); } - return Response.ok("{\"status\":\"ok\",\"username\":\"" + username.get() + "\"}") + return Response.ok("{\"status\":\"ok\",\"username\":\"" + escapeJson(username.get()) + "\"}") + .build(); + } + + // --------------------------------------------------------------------------- + // POST /auth/session/renew + // --------------------------------------------------------------------------- + + /** + * Renews the current session by issuing a fresh token with a new fixed TTL. + * + *

      This is the server-side counterpart of the UI's "Stay Logged In" action. The old token is + * invalidated and a new one with a full TTL is returned via {@code Set-Cookie}, keeping both + * the server entry and the browser cookie in sync. + * + *

      Returns 401 when the current session cookie is absent or has already expired. + */ + @POST + @Path(AUTH_SESSION_RENEW_PATH) + @Produces(MediaType.APPLICATION_JSON) + @ApiOperation(value = "Renew the current session, issuing a fresh token and resetting the TTL") + @ApiResponses(value = { + @ApiResponse(code = 200, message = "Session renewed – new cookie set"), + @ApiResponse(code = 401, message = "No valid session to renew") + }) + public Response renewSession(@Context HttpHeaders httpHeaders) { + Cookie sessionCookie = httpHeaders.getCookies().get(SessionManager.SESSION_COOKIE_NAME); + if (sessionCookie == null || sessionCookie.getValue() == null) { + return Response.status(Response.Status.UNAUTHORIZED) + .entity("{\"error\":\"No session\"}") + .build(); + } + + // rotateSession() atomically invalidates the old token and creates a fresh one, + // preventing orphaned tokens from concurrent "Stay Logged In" clicks. + String oldToken = sessionCookie.getValue(); + Optional usernameOpt = _sessionManager.getUsername(oldToken); + Optional newTokenOpt = _sessionManager.rotateSession(oldToken); + if (!newTokenOpt.isPresent()) { + // Session was absent, expired, or already rotated by a concurrent request. + return Response.status(Response.Status.UNAUTHORIZED) + .entity("{\"error\":\"Session expired\"}") + .build(); + } + + String username = usernameOpt.orElse("unknown"); + String newToken = newTokenOpt.get(); + LOGGER.info("Session renewed for user '{}'", username); + + boolean secureCookie = _controllerConf == null + || _controllerConf.getProperty(ControllerConf.CONTROLLER_UI_SESSION_COOKIE_SECURE, true); + String sessionCookieHeader = buildSessionCookieHeader( + newToken, (int) _sessionManager.getSessionTtlSeconds(), secureCookie); + + return Response.ok("{\"status\":\"renewed\",\"username\":\"" + escapeJson(username) + "\"}") + .header("Set-Cookie", sessionCookieHeader) .build(); } @@ -287,13 +348,23 @@ private static String buildSessionCookieHeader(String token, int maxAgeSeconds, return sb.toString(); } + /** Escapes {@code "} and {@code \} so the value can be safely embedded in a JSON string literal. */ + private static String escapeJson(String value) { + return value.replace("\\", "\\\\").replace("\"", "\\\""); + } + /** Builds a {@code Set-Cookie} header that instructs the browser to delete the session cookie. */ - private static String buildDeleteCookieHeader() { - return SessionManager.SESSION_COOKIE_NAME + "=deleted" - + "; Path=/" - + "; Max-Age=0" - + "; HttpOnly" - + "; SameSite=Strict"; + private static String buildDeleteCookieHeader(boolean secure) { + StringBuilder sb = new StringBuilder(); + sb.append(SessionManager.SESSION_COOKIE_NAME).append("=deleted"); + sb.append("; Path=/"); + sb.append("; Max-Age=0"); + sb.append("; HttpOnly"); + sb.append("; SameSite=Strict"); + if (secure) { + sb.append("; Secure"); + } + return sb.toString(); } // --------------------------------------------------------------------------- diff --git a/pinot-controller/src/main/resources/app/App.tsx b/pinot-controller/src/main/resources/app/App.tsx index ddf96de3d564..6abfe2178edb 100644 --- a/pinot-controller/src/main/resources/app/App.tsx +++ b/pinot-controller/src/main/resources/app/App.tsx @@ -84,6 +84,10 @@ export const App = () => { const performSessionLogout = async () => { setShowTimeoutWarning(false); + // Clear all timers so no further callbacks fire after logout + if (inactivityTimerRef.current) clearTimeout(inactivityTimerRef.current); + if (warningTimerRef.current) clearTimeout(warningTimerRef.current); + if (countdownIntervalRef.current) clearInterval(countdownIntervalRef.current); try { await fetch('/auth/logout', { method: 'GET', credentials: 'include' }); } catch (e) { @@ -269,15 +273,20 @@ export const App = () => { Session Expiring Soon - Your session will expire in {warningCountdown} second{warningCountdown !== 1 ? 's' : ''} due to inactivity. + Your session will expire in {warningCountdown} second{warningCountdown !== 1 ? 's' : ''}.