Add session-based UI authentication with server-side logout invalidation#18933
Add session-based UI authentication with server-side logout invalidation#18933Akanksha-kedia wants to merge 8 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #18933 +/- ##
============================================
- Coverage 65.05% 65.02% -0.04%
Complexity 1403 1403
============================================
Files 3399 3403 +4
Lines 212808 213052 +244
Branches 33568 33604 +36
============================================
+ Hits 138443 138530 +87
- Misses 63223 63372 +149
- Partials 11142 11150 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@apucher @shounakmk219 @soumitra-st — would appreciate a review when you get a chance. This PR adds a session-based UI authentication layer (Trino/Ambari-style) on top of any configured |
|
@apucher @shounakmk219 @soumitra-st @xiangfu0 — all CI checks are now passing ✅. Would appreciate a review when you get a chance. This PR adds a session-based UI authentication layer (Trino/Ambari-style) on top of any configured
Activated by setting |
xiangfu0
left a comment
xiangfu0
left a comment
There was a problem hiding this comment.
Found two high-severity authorization bypasses in the new session path; see inline comments.
| if (sessionCookie != null && sessionCookie.getValue() != null) { | ||
| Optional<String> username = _sessionManager.getUsername(sessionCookie.getValue()); | ||
| if (username.isPresent()) { | ||
| return; |
There was a problem hiding this comment.
CRITICAL: Returning here skips the rest of AuthenticationFilter, including AccessControlUtils.validatePermission(...) and fine-grained auth. In session mode, any user with a valid login cookie can hit controller endpoints regardless of the permissions enforced by the configured AccessControl. The session should authenticate the identity, but authorization still needs to run using that identity or an equivalent server-side principal.
| 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) { |
There was a problem hiding this comment.
CRITICAL: This makes a valid UI session bypass the table-level hasAccess(rawTableName, READ, ...) check, so a user who can log in can query any table through /sql even when their Basic/LDAP/ZK principal would be denied. Please keep the table authorization check and make the session supply the authenticated principal/credentials needed for that check instead of skipping it.
shounakmk219
left a comment
shounakmk219
left a comment
There was a problem hiding this comment.
Did a first pass. Is there an open issue or design doc on this?
| if (_controllerConf != null | ||
| && _controllerConf.getProperty(ControllerConf.CONTROLLER_UI_SESSION_ENABLED, false) |
There was a problem hiding this comment.
Extract this check into a helper method as its replicated at lot of places
| * <li>Logout → read cookie → remove session from store → delete cookie → redirect to login</li> | ||
| * </ul> | ||
| */ | ||
| public class SessionManager { |
There was a problem hiding this comment.
IMO this should be an interface with in-memory storage as the default implementation
| ControllerConf _controllerConf; | ||
|
|
||
| @Inject | ||
| @org.jvnet.hk2.annotations.Optional |
There was a problem hiding this comment.
why do we need this annotation?
| * <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) |
There was a problem hiding this 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
…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 apache#18933. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
xiangfu0
left a comment
xiangfu0
left a comment
There was a problem hiding this comment.
Found a few high-signal session-auth issues; see inline comments.
| 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"); |
There was a problem hiding this 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.
| AccessControl accessControl = _accessControlFactory.create(); | ||
| boolean valid; | ||
| try { | ||
| valid = accessControl.hasAccess(null, AccessType.READ, |
There was a problem hiding this 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.
| .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)) |
There was a problem hiding this 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.
…roker 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 apache#18933. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…fault 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 apache#18933. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
Thanks for the detailed review @xiangfu0 and @shounakmk219! Here is a summary of all the changes made in response to the feedback: Security fixes (CRITICAL)
Non-critical feedback
CI status10/11 checks passing. The one failure ( |
…validation 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)
- 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) <noreply@anthropic.com>
- 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) <noreply@anthropic.com>
…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 apache#18933. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…roker 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 apache#18933. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…fault 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 apache#18933. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Akanksha-kedia
force-pushed
the
feat/session-based-ui-auth
branch
from
89a4a81 to
98d0c3c
Compare
xiangfu0
left a comment
xiangfu0
left a comment
There was a problem hiding this comment.
Found one high-signal compatibility issue; see inline comment.
| .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)) |
There was a problem hiding this comment.
This drops Authorization for every controller /sql and time-series proxy request whenever session mode is enabled, including API clients that authenticated with Basic/Bearer and do not have a session cookie. The controller-side access check can pass with the original header, but the forwarded broker request then arrives without credentials and fails on broker-auth clusters. Keep the caller-supplied Authorization when there is no valid session cookie, or only replace it when injecting a session credential.
… 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.
|
Thank you for the thorough reviews! Addressing all open comments: @xiangfu0 — resolved:
@shounakmk219 — resolved:
Still open / acknowledged:
|
xiangfu0
left a comment
xiangfu0
left a comment
There was a problem hiding this comment.
Found a few high-signal issues; see inline comments.
| ControllerConf.CONTROLLER_UI_SESSION_INACTIVITY_TIMEOUT_SECONDS, | ||
| ControllerConf.DEFAULT_UI_SESSION_INACTIVITY_TIMEOUT_SECONDS); | ||
| long serverSessionTtlSeconds = uiInactivityTimeoutSeconds + 120; | ||
| final SessionManager sessionManager = new InMemorySessionManager(serverSessionTtlSeconds); |
There was a problem hiding this comment.
This creates one in-memory session store per controller process. Pinot controllers are commonly deployed behind a VIP/LB; a token minted on controller A will be unknown on controller B, and logout routed to B will not revoke the token minted on A. That breaks the server-side logout/invalidation guarantee and makes session mode unreliable without sticky routing. Please use a shared session store or gate/document this feature on sticky sessions, with a test for cross-controller logout/validation.
| StringBuilder sb = new StringBuilder(); | ||
| sb.append(SessionManager.SESSION_COOKIE_NAME).append('=').append(token); | ||
| sb.append("; Path=/"); | ||
| sb.append("; Max-Age=").append(maxAgeSeconds); |
There was a problem hiding this comment.
The server session TTL is sliding because getUsername extends it, but the browser cookie Max-Age is fixed at login and is never refreshed on later session checks or API calls. An active UI session can therefore lose the browser cookie after the original max age even though the server-side session was extended. Please either renew Set-Cookie when sliding the session or make the server-side session non-sliding so browser and server expiry semantics match.
| 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"); |
There was a problem hiding this comment.
This narrows CORS allowed headers from wildcard to a fixed list that omits the Pinot supported database request header, CommonConstants.DATABASE, which DatabaseUtils.translateTableName(..., HttpHeaders) uses for multi-database table/schema resolution. Browser clients that set database will now fail preflight before reaching the controller. Please preserve wildcard behavior or include Pinot-supported/custom headers such as database and existing upload headers like If-Match.
4 participants
Summary
Implements a Trino/Ambari-style session layer for the Pinot controller UI that addresses several security gaps in the existing Basic auth flow. The session layer sits on top of ANY configured
AccessControlFactory— no changes to the auth backend required.Problems fixed:
Authorization: Basic <base64>in the network tabapp_state.authTokenheldBasic <base64>(XSS-readable)Authorizationheader from browser directly to brokerSameSiteattributeHow it works:
New files:
SessionManager.java—ConcurrentHashMapsession store with sliding TTL; uses atomiccompute()to eliminate the get→check→remove race conditionSessionAuthenticationFilter.java— JAX-RS filter atAUTHENTICATION-10priority; validates HttpOnly cookie; passes through requests withAuthorizationheader for API backward compatSessionBasicAuthAccessControlFactory.java— convenience factory wrapperPinotSessionLoginResource.java—POST /auth/login,GET /auth/logout,GET /auth/sessionSessionManagerTest.java— unit tests + 50-thread concurrency testCookie construction: Uses raw
Set-Cookieheader string instead of JAX-RSNewCookiebecause JAX-RS 2NewCookiedoes not support theSameSiteattribute.SameSite=Strictblocks cross-site forged requests.Frontend changes:
Models.ts—SESSIONenum added toAuthWorkflowaxios-config.ts— SESSION workflow deletes theAuthorizationheader; cookie sent automatically by browserAuthProvider.tsx— reads inactivity timeout from/auth/info, checks/auth/sessionon page refresh to restore sessionApp.tsx— inactivity timer with configurable timeout; 60-second warning dialog before auto-logout; calls/auth/logouton timeoutHeader.tsx— logout button callsGET /auth/logoutfor server-side session invalidationLoginPage.tsx— SESSION path POSTs form data to/auth/login(noBasicheader in JS memory)Configuration
All defaults preserve existing behaviour. Feature is opt-in:
Test plan
SessionManagerTest— creation, sliding TTL, logout invalidation, 50-thread concurrencySessionAuthenticationFilter— unprotected path passthrough, cookie validation, Authorization header bypassPinotSessionLoginResource— login/logout/session endpointsControllerAdminApiApplicationTestand auth tests continue to pass unmodifiedcontroller.ui.session.authentication.enabled=true, verify:Authorizationheader in DevTools network tab after loginPinot-UI-Sessionpresent with HttpOnly flagcc @Jackie-Jiang @xiangfu0