Alwaysgaurav1 commented on PR #9043: URL: https://github.com/apache/texera/pull/9043#issuecomment-6335906686
@bobbai00 Thanks for the thorough review and verification trace! I have addressed all items in commit `7eadccc`: ### 🔴 Must-Fix - **Compilation in `AgentAccessAuthSpec.scala`**: Removed `u.setGoogleId(null)` (removed since #7055) and called `JwtAuth.jwtClaims(u)` without the invalid parameter. - **Added `@PATCH` to `AccessControlResource.scala`**: Added the `@PATCH` annotation/handler mirroring `authorizePut` so settings saves behind Envoy ext_authz no longer 405. Added a corresponding unit test in `AgentAccessAuthSpec.scala`. - **Anchored `apiAgents` Regex**: Anchored to `^/?(?:auth/)?api/agents(?:/.*)?$` so dynamic routes like `auth/wsapi/x/api/agents` are not inadvertently matched. Added a negative test for `/wsapi`. - **PR Title**: Updated title to `feat(agent-service): authenticate requests via JWT and gateway ext_authz` to follow module scoping and length conventions. ### 🟡 Advisory & Simplifications - **In-Service Role Guard**: `.onBeforeHandle` in `server.ts` now enforces `REGULAR` or `ADMIN` roles, rejecting `INACTIVE` users with 403 to match gateway behavior (with tests added for 403 INACTIVE and 200 ADMIN). - **Fail-Fast Secret Resolution**: Dropped the hardcoded fallback secret in `jwt.ts`; the service now throws on startup if neither `AUTH_JWT_SECRET` nor `auth.conf` is found. - **WebSocket Rejection Code**: Rejected sockets in `open(ws)` now close with code `1008` (Policy Violation) so the frontend detects denied access rather than normal completion. - **Frontend Authorization Header**: Dropped redundant manual `Authorization` header addition in `agentHeaders()` (letting Angular's `JwtModule` handle it automatically), while keeping `X-Agent-Workflow-Id` and the WebSocket `?access-token=` query param. - **K8s Secret Scoping**: Filtered `texeraEnvVars` in `agent-service-deployment.yaml` to inject only `AUTH_JWT_SECRET`. - **Dead Code Cleanup**: Removed redundant token validation logic and unused `ERROR_STATUS` entries in `server.ts`. - **Workflow / sbt Bump**: Noted; will rebase out the setup-sbt workflow commits once #9064 merges. ### 🟢 Polish - Corrected the routing comment in `bin/single-node/nginx.conf`. - Clarified the header forwarding comment in `AccessControlResource.scala`. --- **Verification**: - Formatted all touched files with Prettier. - All 330 unit & WebSocket integration tests in `agent-service` pass (`bun test`). Please take another look when you get a chance! -- 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]
