xiangfu0 commented on code in PR #18933:
URL: https://github.com/apache/pinot/pull/18933#discussion_r3577540471
##########
pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java:
##########
@@ -746,6 +748,34 @@ private void setUpPinotController() {
final MetadataEventNotifierFactory metadataEventNotifierFactory =
MetadataEventNotifierFactory.loadFactory(_config.subset(METADATA_EVENT_NOTIFIER_PREFIX),
_helixResourceManager);
+ // Create a singleton SessionManager for session-based UI authentication.
+ //
+ // 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);
+ long serverSessionTtlSeconds = uiInactivityTimeoutSeconds + 120;
+ final SessionManager sessionManager = new
InMemorySessionManager(serverSessionTtlSeconds);
Review Comment:
This creates a per-controller in-memory session store, so logout
invalidation is not authoritative in a normal multi-controller deployment. If
login/session use controller A but logout is routed to controller B, controller
A keeps accepting the old cookie until TTL, which violates the server-side
invalidation guarantee. Please use a shared/versioned session store or make
session auth fail closed unless sticky routing is explicitly configured and
documented for this auth mode.
##########
pinot-controller/src/main/resources/app/components/auth/AuthProvider.tsx:
##########
@@ -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);
Review Comment:
On page refresh, a valid SESSION cookie only calls `setAuthenticated(true)`.
It does not restore `app_state.authWorkflow` or `app_state.username`, but
`Header` uses `app_state.authWorkflow` to show/call logout and `fetchUserRole`
uses `app_state.username`. A restored session therefore loses the manual
server-side logout path. Please populate `app_state` for restored sessions the
same way `LoginPage` does.
--
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]
