This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-5392-6a9437ed6e46599044f4f7762dc23e4065624262 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 749798d8214a5999b2870ebcf3f62fefd609edfa Author: Yicong Huang <[email protected]> AuthorDate: Sat Jun 6 00:15:12 2026 -0700 fix(frontend): handle 401 by clearing session, skip sending expired JWTs (#5392) ### What changes were proposed in this PR? Adds `UnauthorizedHttpInterceptor` that watches every HTTP response: when the server returns 401 to a request that carried an `Authorization` header, it routes the logout through `UserService.logout()` (which clears the JWT and broadcasts `userChanged(undefined)` so header / dashboard / in-memory state stay in sync), surfaces a "session expired" notification, and routes to `/about` with `returnUrl`. 401 responses to anonymous or auth-endpoint requests pass through unchanged so a 401 on a public endpoint never wipes a freshly-stored token, and a wrong-password 401 on `/auth/login` cannot kick an already-authenticated user out. Two adversarial-review findings (caught during self-review, fixed in commit 2): 1. **Auth-endpoint allowlist.** `JwtModule` blanket-attaches `Authorization` to every request, including `POST /api/auth/login`. A wrong-password 401 there carries the user's existing token and would be misread as "session invalid", kicking an already-authenticated user out the moment they fat-finger a re-login. Allowlist regex `\/auth\/(login|register|refresh|google\/login)` short-circuits the interceptor for these URLs. 2. **Concurrent 401 dedup.** A burst of 8-10 parallel dashboard requests, all 401'd by a server-side revoke, would fire 8 toasts and 8 `router.navigate` calls. Routing the logout through `UserService.logout()` (which flips `isLogin()` from true to false) gives natural dedup: only the first 401 sees `isLogin() === true` and fires side effects; the rest of the burst short-circuits. The flag re-arms automatically after a successful re-login. Flips `JwtModule.forRoot`'s `skipWhenExpired` from `false` to `true`, so a stale `localStorage` token is no longer auto-appended to public pre-login endpoints (`/api/config/gui`, `/api/config/user-system`). That auto-append is the exact mechanism that caused the revert of #4903 in #5025 — without it, an expired token reaches public endpoints, gets rejected, and breaks the login page before the user can re-authenticate. `UserService` is resolved lazily via `Injector` inside `catchError` because it transitively depends on `HttpClient`, and direct injection would form a DI cycle through the interceptor chain. ### Any related issues, documentation, discussions? Closes #5391. Prerequisite for a re-attempt of #4901 (whose first try, #4903, was reverted by #5025 because of #5026). Discussion: https://lists.apache.org/thread/w5jmczrffxd5doc5hnss2zm7dbhnbhyy ### How was this PR tested? Added `unauthorized-http-interceptor.service.spec.ts` with 12 cases covering: 401 with Authorization → logout + notify + navigate; 401 without Authorization → no-op; non-401 with Authorization → no-op; root path → omits `returnUrl`; auth-endpoint URLs (login / register / refresh / google/login, with and without query strings) → no-op; over-broad URL match guard (`recent-logins` still logs out); concurrent 401 burst → exactly one side-effect set; re-arms after re-login. Tested manually as well. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 4.7) --- frontend/src/app/app.module.ts | 8 +- .../unauthorized-http-interceptor.service.spec.ts | 193 +++++++++++++++++++++ .../unauthorized-http-interceptor.service.ts | 87 ++++++++++ 3 files changed, 287 insertions(+), 1 deletion(-) diff --git a/frontend/src/app/app.module.ts b/frontend/src/app/app.module.ts index 78fc75d7cb..485b3c2e4a 100644 --- a/frontend/src/app/app.module.ts +++ b/frontend/src/app/app.module.ts @@ -78,6 +78,7 @@ import { NzCardModule } from "ng-zorro-antd/card"; import { NzTagModule } from "ng-zorro-antd/tag"; import { NzAvatarModule } from "ng-zorro-antd/avatar"; import { BlobErrorHttpInterceptor } from "./common/service/blob-error-http-interceptor.service"; +import { UnauthorizedHttpInterceptor } from "./common/service/unauthorized-http-interceptor.service"; import { ConsoleFrameComponent } from "./workspace/component/result-panel/console-frame/console-frame.component"; import { ResultTableFrameComponent } from "./workspace/component/result-panel/result-table-frame/result-table-frame.component"; import { RowModalComponent } from "./workspace/component/result-panel/result-panel-modal.component"; @@ -203,7 +204,7 @@ registerLocaleData(en); JwtModule.forRoot({ config: { tokenGetter: AuthService.getAccessToken, - skipWhenExpired: false, + skipWhenExpired: true, throwNoTokenError: false, disallowedRoutes: ["forum/api/users"], }, @@ -375,6 +376,11 @@ registerLocaleData(en); useClass: BlobErrorHttpInterceptor, multi: true, }, + { + provide: HTTP_INTERCEPTORS, + useClass: UnauthorizedHttpInterceptor, + multi: true, + }, { provide: "SocialAuthServiceConfig", useFactory: (googleAuthService: GoogleAuthService, userService: UserService) => { diff --git a/frontend/src/app/common/service/unauthorized-http-interceptor.service.spec.ts b/frontend/src/app/common/service/unauthorized-http-interceptor.service.spec.ts new file mode 100644 index 0000000000..1b11d9128b --- /dev/null +++ b/frontend/src/app/common/service/unauthorized-http-interceptor.service.spec.ts @@ -0,0 +1,193 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { HTTP_INTERCEPTORS, HttpClient } from "@angular/common/http"; +import { HttpClientTestingModule, HttpTestingController } from "@angular/common/http/testing"; +import { TestBed } from "@angular/core/testing"; +import { Router } from "@angular/router"; +import { ABOUT } from "../../app-routing.constant"; +import { NotificationService } from "./notification/notification.service"; +import { UserService } from "./user/user.service"; +import { UnauthorizedHttpInterceptor } from "./unauthorized-http-interceptor.service"; + +describe("UnauthorizedHttpInterceptor", () => { + let http: HttpClient; + let httpMock: HttpTestingController; + let routerSpy: { navigate: ReturnType<typeof vi.fn>; url: string }; + let notificationSpy: { error: ReturnType<typeof vi.fn> }; + let userServiceSpy: { logout: ReturnType<typeof vi.fn>; isLogin: ReturnType<typeof vi.fn> }; + + beforeEach(() => { + routerSpy = { navigate: vi.fn(), url: "/user/workflow/42" }; + notificationSpy = { error: vi.fn() }; + userServiceSpy = { logout: vi.fn(), isLogin: vi.fn().mockReturnValue(true) }; + + TestBed.configureTestingModule({ + imports: [HttpClientTestingModule], + providers: [ + { provide: HTTP_INTERCEPTORS, useClass: UnauthorizedHttpInterceptor, multi: true }, + { provide: Router, useValue: routerSpy }, + { provide: NotificationService, useValue: notificationSpy }, + { provide: UserService, useValue: userServiceSpy }, + ], + }); + + http = TestBed.inject(HttpClient); + httpMock = TestBed.inject(HttpTestingController); + }); + + afterEach(() => { + httpMock.verify(); + }); + + function authedGet(url: string) { + return http.get(url, { headers: { Authorization: "Bearer stale-token" } }); + } + + it("logs out, notifies, and redirects to ABOUT on 401 for an authenticated request", () => { + // The decision to log out hinges on whether *this* request was authenticated. + // A 401 from an anonymous request is the server saying "you need to log in", + // not "your session is invalid" — clearing the session there would wipe a + // freshly-stored token (e.g. mid-login race). + authedGet("/api/secret").subscribe({ error: () => {} }); + httpMock.expectOne("/api/secret").flush(null, { status: 401, statusText: "Unauthorized" }); + + expect(userServiceSpy.logout).toHaveBeenCalledTimes(1); + expect(notificationSpy.error).toHaveBeenCalledTimes(1); + expect(notificationSpy.error.mock.calls[0][0]).toMatch(/session.*expired|log in/i); + expect(routerSpy.navigate).toHaveBeenCalledWith([ABOUT], { + queryParams: { returnUrl: "/user/workflow/42" }, + }); + }); + + it("leaves the session untouched when 401 comes back for an anonymous request", () => { + // Reproduces the #5026 / #4903-revert scenario: a public endpoint (or one + // whose token JwtModule skipped because it was expired) returning 401 + // must NOT trigger a logout — the user may not even be logged in yet. + http.get("/api/public").subscribe({ error: () => {} }); + httpMock.expectOne("/api/public").flush(null, { status: 401, statusText: "Unauthorized" }); + + expect(userServiceSpy.logout).not.toHaveBeenCalled(); + expect(notificationSpy.error).not.toHaveBeenCalled(); + expect(routerSpy.navigate).not.toHaveBeenCalled(); + }); + + it("does not log out on non-401 errors even when Authorization was sent", () => { + authedGet("/api/oops").subscribe({ error: () => {} }); + httpMock.expectOne("/api/oops").flush(null, { status: 500, statusText: "Server Error" }); + + expect(userServiceSpy.logout).not.toHaveBeenCalled(); + expect(notificationSpy.error).not.toHaveBeenCalled(); + expect(routerSpy.navigate).not.toHaveBeenCalled(); + }); + + it("omits returnUrl when the current route is the root", () => { + routerSpy.url = "/"; + authedGet("/api/secret").subscribe({ error: () => {} }); + httpMock.expectOne("/api/secret").flush(null, { status: 401, statusText: "Unauthorized" }); + + expect(routerSpy.navigate).toHaveBeenCalledWith([ABOUT], { queryParams: { returnUrl: null } }); + }); + + // Adversarial-review fix #1: a stale token gets auto-attached to /auth/login + // by JwtModule. A wrong-password 401 from login must NOT be misread as + // "your session is invalid", or we'd kick an already-authenticated user + // out the moment they fat-finger a re-login. + describe("auth-endpoint allowlist", () => { + const authPaths = [ + "/api/auth/login", + "/api/auth/register", + "/api/auth/refresh", + "/api/auth/google/login", + "/api/auth/login?next=/dashboard", + ]; + + authPaths.forEach(path => { + it(`does not log out on 401 from ${path}`, () => { + authedGet(path).subscribe({ error: () => {} }); + httpMock.expectOne(path).flush(null, { status: 401, statusText: "Unauthorized" }); + + expect(userServiceSpy.logout).not.toHaveBeenCalled(); + expect(notificationSpy.error).not.toHaveBeenCalled(); + expect(routerSpy.navigate).not.toHaveBeenCalled(); + }); + }); + + it("still logs out on 401 from a URL that merely contains 'login' in a non-auth segment", () => { + // Guards against an over-broad regex like /login/ that would skip e.g. + // /api/dashboard/recent-logins. + authedGet("/api/dashboard/recent-logins").subscribe({ error: () => {} }); + httpMock.expectOne("/api/dashboard/recent-logins").flush(null, { status: 401, statusText: "Unauthorized" }); + + expect(userServiceSpy.logout).toHaveBeenCalledTimes(1); + }); + }); + + // Adversarial-review fix #2: dashboard load fires 8-10 parallel requests; a + // server-side revoke makes them all 401 within microseconds of each other. + // Side effects must fire exactly once, not once per request — otherwise we + // get a toast pile-up and redundant navigations. + describe("concurrent 401 deduplication", () => { + it("fires logout side effects exactly once for a burst of 401s", () => { + // Simulate UserService transitioning from logged-in → logged-out as soon + // as logout() is called; this is what UserService.changeUser(undefined) + // does in production and is the discriminator the interceptor reads. + userServiceSpy.logout.mockImplementation(() => { + userServiceSpy.isLogin.mockReturnValue(false); + }); + + authedGet("/api/a").subscribe({ error: () => {} }); + authedGet("/api/b").subscribe({ error: () => {} }); + authedGet("/api/c").subscribe({ error: () => {} }); + + httpMock.expectOne("/api/a").flush(null, { status: 401, statusText: "Unauthorized" }); + httpMock.expectOne("/api/b").flush(null, { status: 401, statusText: "Unauthorized" }); + httpMock.expectOne("/api/c").flush(null, { status: 401, statusText: "Unauthorized" }); + + expect(userServiceSpy.logout).toHaveBeenCalledTimes(1); + expect(notificationSpy.error).toHaveBeenCalledTimes(1); + expect(routerSpy.navigate).toHaveBeenCalledTimes(1); + }); + + it("re-arms after a successful re-login so a later 401 triggers logout again", () => { + // After logout, isLogin() is false. Dedup keys off isLogin(), so when the + // user re-logs in (isLogin() flips back to true) and a fresh request 401s, + // the interceptor must fire side effects again. Otherwise the user gets + // stuck in a half-logged-in state after a transient session error. + userServiceSpy.logout.mockImplementationOnce(() => { + userServiceSpy.isLogin.mockReturnValue(false); + }); + + authedGet("/api/a").subscribe({ error: () => {} }); + httpMock.expectOne("/api/a").flush(null, { status: 401, statusText: "Unauthorized" }); + expect(userServiceSpy.logout).toHaveBeenCalledTimes(1); + + // Simulate re-login: isLogin() back to true. + userServiceSpy.isLogin.mockReturnValue(true); + userServiceSpy.logout.mockImplementationOnce(() => { + userServiceSpy.isLogin.mockReturnValue(false); + }); + + authedGet("/api/b").subscribe({ error: () => {} }); + httpMock.expectOne("/api/b").flush(null, { status: 401, statusText: "Unauthorized" }); + + expect(userServiceSpy.logout).toHaveBeenCalledTimes(2); + }); + }); +}); diff --git a/frontend/src/app/common/service/unauthorized-http-interceptor.service.ts b/frontend/src/app/common/service/unauthorized-http-interceptor.service.ts new file mode 100644 index 0000000000..2ff0cbc5db --- /dev/null +++ b/frontend/src/app/common/service/unauthorized-http-interceptor.service.ts @@ -0,0 +1,87 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { HttpErrorResponse, HttpEvent, HttpHandler, HttpInterceptor, HttpRequest } from "@angular/common/http"; +import { Injectable, Injector } from "@angular/core"; +import { Router } from "@angular/router"; +import { Observable, throwError } from "rxjs"; +import { catchError } from "rxjs/operators"; +import { ABOUT } from "../../app-routing.constant"; +import { NotificationService } from "./notification/notification.service"; +import { UserService } from "./user/user.service"; + +// Endpoints whose own 401 means "wrong credentials", not "your session is invalid". +// JwtModule blanket-attaches Authorization to every request, so a stale token +// gets piggy-backed on login/register/refresh attempts even when the user is +// trying to *acquire* a session. Treating those 401s as session-invalidation +// would kick out an already-authenticated user who fat-fingers a re-login. +const AUTH_ENDPOINT_PATTERN = /\/auth\/(login|register|refresh|google\/login)(?:\?|$)/; + +/** + * Globally handles 401 responses that come back for *authenticated* requests: + * routes the logout through UserService (which clears the token and broadcasts + * `userChanged(undefined)` so header / dashboard / in-memory state stay in + * sync), surfaces a notification, and routes to the landing page with + * returnUrl. 401s on anonymous, auth-endpoint, or already-logged-out paths + * pass through unchanged. See #5391 / #4901 / #5026. + * + * UserService is resolved lazily via Injector because it (transitively) + * depends on HttpClient, and HttpClient depends on the interceptor chain — + * direct injection here would form a DI cycle. + */ +@Injectable() +export class UnauthorizedHttpInterceptor implements HttpInterceptor { + constructor( + private injector: Injector, + private router: Router, + private notificationService: NotificationService + ) {} + + public intercept(req: HttpRequest<unknown>, next: HttpHandler): Observable<HttpEvent<unknown>> { + return next.handle(req).pipe( + catchError((err: unknown) => { + if (this.shouldHandleAsSessionExpired(req, err)) { + const userService = this.injector.get(UserService); + // Dedup: if a burst of 401s arrives, only the first one through here + // sees isLogin()=true. logout() flips it to false (via changeUser), + // so the rest of the burst skip the side effects. After a real + // re-login isLogin() flips back to true and the interceptor re-arms. + if (userService.isLogin()) { + userService.logout(); + this.notificationService.error("Your session has expired. Please log in again."); + const currentUrl = this.router.url; + this.router.navigate([ABOUT], { + queryParams: { returnUrl: currentUrl === "/" ? null : currentUrl }, + }); + } + } + return throwError(() => err); + }) + ); + } + + private shouldHandleAsSessionExpired(req: HttpRequest<unknown>, err: unknown): boolean { + return ( + err instanceof HttpErrorResponse && + err.status === 401 && + req.headers.has("Authorization") && + !AUTH_ENDPOINT_PATTERN.test(req.url) + ); + } +}
