shounakmk219 commented on code in PR #18933:
URL: https://github.com/apache/pinot/pull/18933#discussion_r3544686142
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java:
##########
@@ -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)
Review Comment:
Extract this check into a helper method as its replicated at lot of places
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -120,6 +122,10 @@ public class PinotQueryResource {
@Inject
ControllerConf _controllerConf;
+ @Inject
+ @org.jvnet.hk2.annotations.Optional
Review Comment:
why do we need this annotation?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/SessionManager.java:
##########
@@ -0,0 +1,272 @@
+/**
+ * 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.
+ *
+ * <p>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.
+ *
+ * <p><strong>Sliding window TTL:</strong> 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.
+ *
+ * <p><strong>Broker credential forwarding:</strong> 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.
+ *
+ * <p>Design follows the Ambari / Trino FormWebUiAuthenticationFilter pattern:
+ * <ul>
+ * <li>Login → validate credentials → create session token → set HttpOnly
cookie</li>
+ * <li>Request → read cookie → look up session → slide expiry → allow or
redirect to login</li>
+ * <li>Logout → read cookie → remove session from store → delete cookie →
redirect to login</li>
+ * </ul>
+ */
+public class SessionManager {
Review Comment:
IMO this should be an interface with in-memory storage as the default
implementation
##########
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).
+ *
+ * <p>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.
+ *
+ * <p>When enabled:
+ * <ul>
+ * <li>{@code GET /auth/info} returns {@code {"workflow":"SESSION"}}</li>
+ * <li>The UI uses {@code POST /auth/login} to authenticate (no
Authorization header)</li>
+ * <li>An HttpOnly {@code SameSite=Strict} session cookie is issued on
successful login</li>
+ * <li>{@code GET /auth/logout} immediately invalidates the server-side
session</li>
+ * </ul>
+ *
+ * <p>Default: {@code false} (backward compatible – existing BASIC/NONE/OIDC
workflows unchanged)
Review Comment:
nit: squeeze the huge multi-line comment block into a one liner here and add
the details in public docs instead that would be more helpful
--
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]
