Akanksha-kedia commented on PR #18933: URL: https://github.com/apache/pinot/pull/18933#issuecomment-4951120875
Thank you for the thorough reviews! Addressing all open comments: **@xiangfu0 — resolved:** | Comment | Fix | |---|---| | `AuthenticationFilter.java:108` — early return bypassed authorization | Replaced with credential injection; `validatePermission` and `FineGrainedAuthUtils` now always run with the session user's identity | | `PinotQueryResource.java:563` — session bypassed table-level `hasAccess` | `hasAccess(rawTableName, READ, ...)` check is preserved; session only injects credentials, it does not skip authorization | | `PinotSessionLoginResource.java:155` — `hasTable(null)` locked out table-scoped users | Changed to `hasAccess(READ, syntheticHeaders, AUTH_LOGIN_PATH)` which validates identity without a table scope | | `ControllerAdminApiApplication.java:158` — credentialed CORS echoing Origin | CORS now uses `Access-Control-Allow-Origin: *` (no credential reflection) | | `PinotQueryResource.java:841` — timeseries path not re-injecting session credential | Both SQL and timeseries broker-forward paths now use `buildBrokerHeaders()` which resolves the credential once | | `PinotQueryResource.java:841` (July 12) — `extractHeaders` stripped Authorization from all requests in session mode, including API clients | Merged `extractHeaders` + `injectSessionCredentialIfNeeded` into a single `buildBrokerHeaders` / `resolveSessionCredential` pair that calls `getBasicAuthToken` exactly once — no TOCTOU window | **@shounakmk219 — resolved:** | Comment | Fix | |---|---| | `SessionManager.java:62` — should be interface | Refactored to `SessionManager` interface + `InMemorySessionManager` default impl | | `ControllerConf.java:376` — compress multi-line comment block | Each constant now has a single-line Javadoc | **Still open / acknowledged:** - `AuthenticationFilter.java:102` — I'll extract the session-cookie check into a shared helper as a follow-up cleanup commit if that's helpful, or happy to do it now. - `PinotQueryResource.java:128` — `@org.jvnet.hk2.annotations.Optional` is needed because `SessionManager` binding is only registered when session auth is enabled; without it HK2 injection fails in unit-test contexts where the full `ControllerAdminApiApplication` is not wired up. -- 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]
