Akanksha-kedia commented on PR #18933: URL: https://github.com/apache/pinot/pull/18933#issuecomment-4925441369
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) **`AuthenticationFilter`** — Replaced the early `return` on a valid session cookie with Basic-auth token injection (`requestContext.getHeaders().putSingle(AUTHORIZATION, authToken)`). `validatePermission()` and `FineGrainedAuthUtils` now run normally with the session user's credentials instead of being skipped entirely. **`PinotQueryResource`** — Removed the `if (!sessionValid)` guards around `hasAccess()` calls in `getQueryResponse()` and `getMultiStageQueryResponse()`. Table-level and endpoint-level authorization now runs unconditionally for all requests, including session-based ones. **`PinotSessionLoginResource`** — Changed credential validation at login from `hasAccess(null, AccessType.READ, ...)` (table-level overload, which calls `hasTable(null)` and rejects table-scoped principals) to `hasAccess(AccessType.READ, ...)` (identity-only overload). Table-scoped users can now log in through the session endpoint. **`PinotQueryResource` — time-series broker auth** — Extracted `injectSessionCredentialIfNeeded()` helper and called it in both `sendRequestToBroker()` and `sendTimeSeriesRequestToBroker()`. Previously only the SQL forward path re-injected the session credential; time-series requests (`/query/timeseries`, `/timeseries/api/v1/query_range`) were forwarded to the broker without an `Authorization` header in session mode. **`ControllerAdminApiApplication`** — Reverted `CorsFilter` to `Access-Control-Allow-Origin: *` without `Allow-Credentials`. The Pinot UI is served from the same origin as the API so credentialed CORS is not needed; the previous origin-reflect approach was too permissive for same-site sibling origins. --- ### Non-critical feedback **`AuthenticationFilter`** — Extracted `isSessionEnabled()` helper to consolidate the repeated `_controllerConf != null && getProperty(SESSION_ENABLED) && _sessionManager != null` check. **`SessionManager`** — Refactored into an interface. The existing in-memory implementation is now `InMemorySessionManager`. All injection points already reference `SessionManager` so no changes were needed there; `BaseControllerStarter` now instantiates `InMemorySessionManager`. **`PinotQueryResource`** — Added a comment explaining why `@org.jvnet.hk2.annotations.Optional` is needed on `SessionManager`: the binding is always present in production but may be absent in unit-test contexts without a full HK2 setup. **`ControllerConf`** — Shortened the three multi-line Javadoc blocks on the session constants to one-liners. --- ### CI status 10/11 checks passing. The one failure (`Pinot Binary Compatibility Check`) is a pre-existing issue caused by unrelated changes in `pinot-spi` on `master` that removed `ColumnReader` methods — it also fails on other open PRs and is not related to this change. -- 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]
