xiangfu0 commented on code in PR #18933: URL: https://github.com/apache/pinot/pull/18933#discussion_r3546273656
########## pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSessionLoginResource.java: ########## @@ -0,0 +1,374 @@ +/** + * 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. + * + * <p><strong>Architecture (Trino/Ambari-style):</strong> + * <p>This resource is a <em>session layer</em> 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. + * + * <p><strong>How it works:</strong> + * <ol> + * <li>UI calls {@code GET /auth/info} → receives {@code {"workflow":"SESSION"}} when + * {@code controller.ui.session.enabled=true}</li> + * <li>UI shows login form → user enters username/password</li> + * <li>UI POSTs to {@code POST /auth/login} (form-encoded, NOT Basic auth header)</li> + * <li>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.)</li> + * <li>On success: creates a server-side session → sets an HttpOnly cookie</li> + * <li>Subsequent requests: browser sends cookie automatically, NO Authorization header</li> + * <li>{@code GET /auth/logout}: immediately invalidates the server-side session</li> + * </ol> + * + * <p><strong>Key security properties:</strong> + * <ul> + * <li>Credentials are NOT visible in browser dev-tools network tab</li> + * <li>Cookie is HttpOnly (JS cannot read it → prevents XSS token theft)</li> + * <li>Logout immediately invalidates the session server-side</li> + * <li>Session expires automatically after 8 hours</li> + * </ul> + */ +@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. + * + * <p>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, Review Comment: This uses table-level authorization as credential validation. Existing Basic/ZK Basic principals with a non-empty table allowlist fail `hasTable(null)`, so valid table-scoped users are locked out of session mode before the normal endpoint/table checks can run. Validate identity through a non-table credential path and add a regression for a table-scoped principal. ########## pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java: ########## @@ -806,9 +828,14 @@ private String getTimeSeriesQueryURL(String protocol, String hostName, int port, } private Map<String, String> 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)) Review Comment: This strips `Authorization` for every session-mode broker forward, but only the SQL forwarding helper re-injects the stored session credential. The time-series path builds `headers` with this helper and sends them to the broker as-is, so valid UI sessions lose broker auth for `/query/timeseries` and `/timeseries/api/v1/query_range`. Move the session credential injection into a shared broker-forward helper used by both SQL and time-series requests. ########## pinot-controller/src/main/java/org/apache/pinot/controller/api/ControllerAdminApiApplication.java: ########## @@ -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"); Review Comment: This reflects any `Origin` and enables credentialed CORS globally. Session cookies use `SameSite=Strict`, but SameSite is site-based rather than origin-based, so a same-site sibling origin can still have the browser send the Pinot session cookie and then read API responses because this response echoes that origin. Please gate credentialed CORS behind an explicit trusted-origin allowlist, or keep credentials disabled unless such an allowlist is configured. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
