xiangfu0 commented on code in PR #18933:
URL: https://github.com/apache/pinot/pull/18933#discussion_r3544663018
##########
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)
+ && _sessionManager != null) {
+ Cookie sessionCookie =
requestContext.getCookies().get(SessionManager.SESSION_COOKIE_NAME);
+ if (sessionCookie != null && sessionCookie.getValue() != null) {
+ Optional<String> username =
_sessionManager.getUsername(sessionCookie.getValue());
+ if (username.isPresent()) {
+ return;
Review 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.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -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) {
Review 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.
--
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]
