This is an automated email from the ASF dual-hosted git repository.
voidmatcha pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/zeppelin.git
The following commit(s) were added to refs/heads/master by this push:
new 006a3e1578 [ZEPPELIN-6643] Log out on session expiry instead of
throwing in the interceptor
006a3e1578 is described below
commit 006a3e157860a4ba18cd2210ac7bb7ff8a082e7f
Author: 김예나 <[email protected]>
AuthorDate: Thu Sep 10 20:26:53 2026 +0900
[ZEPPELIN-6643] Log out on session expiry instead of throwing in the
interceptor
### What is this PR for?
The New UI never logs out on session expiry. `AppHttpInterceptor` guards
its 405 branch with `event.url.contains('logout')`, and JavaScript strings have
no `contains` method, so the guard throws a `TypeError` inside `catchError`
before `ticketService.logout()` is reached. Both statements after it are
skipped: logout never runs, and the caller observes a `TypeError` instead of
the 405 it needs to act on. The expired session stays in place until the user
reloads the page by hand.
The guard's intent is right and is kept. It exists so that a 405 on the
logout request itself does not call logout again, which would recurse. Only the
method name changes:
```diff
-} else if (event.status === 405 && !event.url.contains('logout')) {
+} else if (event.status === 405 && !event.url?.includes('logout')) {
```
`includes` is the method that exists. The optional chain covers
`HttpErrorResponse.url` being `null`, which it is whenever the failure carries
no resolved url, and which would otherwise throw at the same spot for a
different reason. A 405 with no url cannot be identified as the logout call, so
it falls through to logout — the same conclusion the branch already draws for
every other request, and the safe one when the session is likely gone.
The 401 redirect branch is untouched.
Out of scope, and left alone deliberately: the substring match means a 405
on a path that merely contains `logout` is also skipped, and the `catchError`
parameter is untyped. Both belong to
[ZEPPELIN-6469](https://issues.apache.org/jira/browse/ZEPPELIN-6469), which
waits on this behaviour being correct first.
The spec constructs the interceptor directly with a `logout` stub rather
than starting `TestBed`, per `zeppelin-web-angular/AGENTS.md`: no Angular
wiring is under test here, only the branch. It pins three things the branch has
to get right — a non-logout 405 calls logout exactly once, that 405 reaches the
caller unchanged rather than replaced by a `TypeError`, and a 405 from the
logout request itself does not call logout again — plus the null-url path.
### What type of PR is it?
Bug Fix
### Todos
None
### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-6643
### How should this be tested?
* `npm run test:shell` — 53 tests across 12 files, green. The four new ones
are in `src/app/app-http.interceptor.spec.ts`.
* The assertions were checked by breaking what they cover. Reverting the
source line to `event.url.contains('logout')` fails three of the four, with
`AssertionError: expected TypeError: event.url.contains is not a function to be
HttpErrorResponse`. The fourth — the recursion guard — passes either way,
because the `TypeError` also happens to prevent the logout call; it is there to
confirm the guard survives the fix, not to reproduce the bug.
* `npx prettier --check` on both files, clean. `npx eslint` on both reports
only the two `prefer-arrow/prefer-arrow-functions` warnings that the named test
helpers produce, the same two `src/app/services/save-as.service.spec.ts`
already reports on master.
* Not run locally: Playwright, and the production builds. Neither is
reachable from this change — it is one expression in an interceptor plus a unit
spec — but saying so rather than implying otherwise.
Manual reproduction, for a reviewer who wants to see the original failure:
with an expired session, any REST call from the New UI answers 405 and the
browser console shows `event.url.contains is not a function` from the
interceptor, with no logout request following it. After this change the same
405 is followed by `POST /api/login/logout`.
### Screenshots (if appropriate)
Not applicable.
### Questions:
* Does the license files need to update? No
* Is there breaking changes for older versions? No. The 405 branch did
nothing but throw before this change, so nothing could have depended on it.
* Does this needs documentation? No
Closes #5464 from kimyenac/ZEPPELIN-6643.
Signed-off-by: YONGJAE LEE <[email protected]>
---
.../src/app/app-http.interceptor.spec.ts | 160 +++++++++++++++++++++
.../src/app/app-http.interceptor.ts | 20 ++-
2 files changed, 176 insertions(+), 4 deletions(-)
diff --git a/zeppelin-web-angular/src/app/app-http.interceptor.spec.ts
b/zeppelin-web-angular/src/app/app-http.interceptor.spec.ts
new file mode 100644
index 0000000000..95922beac0
--- /dev/null
+++ b/zeppelin-web-angular/src/app/app-http.interceptor.spec.ts
@@ -0,0 +1,160 @@
+/*
+ * Licensed 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 { HttpClient, HttpErrorResponse, HttpEvent, HttpHandler, HttpRequest }
from '@angular/common/http';
+import { Router } from '@angular/router';
+import { defer, firstValueFrom, of, Subject, throwError } from 'rxjs';
+
+import { NzMessageService } from 'ng-zorro-antd/message';
+
+import { beforeEach, describe, expect, it, vi } from 'vitest';
+
+import { BaseUrlService, TicketService } from '@zeppelin/services';
+
+import { AppHttpInterceptor } from './app-http.interceptor';
+
+const REST_BASE = 'http://localhost:8080/api';
+
+/**
+ * The server answers an expired session with 405 on the REST base rather than
401, so the 405
+ * branch is the only path that reaches logout. The response carries no
Location header, which
+ * is what keeps the 401 branch out of the way.
+ */
+function sessionExpired(url: string | undefined): HttpErrorResponse {
+ return new HttpErrorResponse({ status: 405, url });
+}
+
+describe('AppHttpInterceptor', () => {
+ let logout: ReturnType<typeof vi.fn>;
+ let logoutSubscribed: ReturnType<typeof vi.fn<() => void>>;
+ let interceptor: AppHttpInterceptor;
+
+ /** Drives one request through the interceptor and returns whatever the
caller would observe. */
+ function intercept(failure: HttpErrorResponse, url = REST_BASE, method =
'GET'): Promise<unknown> {
+ const next: HttpHandler = { handle: () => throwError(() => failure) };
+ return firstValueFrom(interceptor.intercept(new HttpRequest(method, url,
null), next));
+ }
+
+ beforeEach(() => {
+ logoutSubscribed = vi.fn<() => void>();
+ logout = vi.fn(() =>
+ defer(() => {
+ logoutSubscribed();
+ return of({});
+ })
+ );
+ interceptor = new AppHttpInterceptor({ logout } as unknown as
TicketService);
+ });
+
+ it('logs out once when a non-logout request is answered with 405', async ()
=> {
+ const failure = sessionExpired(`${REST_BASE}/notebook`);
+
+ await expect(intercept(failure,
`${REST_BASE}/notebook`)).rejects.toBe(failure);
+
+ // `String.prototype.contains` does not exist, so this branch used to
throw before reaching logout
+ expect(logout).toHaveBeenCalledTimes(1);
+ expect(logoutSubscribed).toHaveBeenCalledTimes(1);
+ });
+
+ it('rethrows the 405 it logged out on instead of a TypeError', async () => {
+ const failure = sessionExpired(`${REST_BASE}/notebook`);
+
+ await expect(intercept(failure,
`${REST_BASE}/notebook`)).rejects.toBe(failure);
+ });
+
+ it.each([
+ ['the logout URL', `${REST_BASE}/login/logout`],
+ ['the redirected login URL', `${REST_BASE}/login`],
+ ['no URL', undefined]
+ ])('does not retry logout when its 405 response reports %s', async
(_description, responseUrl) => {
+ const failure = sessionExpired(responseUrl);
+
+ await expect(intercept(failure, `${REST_BASE}/login/logout`,
'POST')).rejects.toBe(failure);
+
+ expect(logout).not.toHaveBeenCalled();
+ expect(logoutSubscribed).not.toHaveBeenCalled();
+ });
+
+ it('logs out on a non-logout request whose 405 reports no url', async () => {
+ const failure = sessionExpired(undefined);
+
+ await expect(intercept(failure,
`${REST_BASE}/notebook`)).rejects.toBe(failure);
+
+ expect(logout).toHaveBeenCalledTimes(1);
+ expect(logoutSubscribed).toHaveBeenCalledTimes(1);
+ });
+
+ it('finishes ticket cleanup without another request when logout redirects to
a login 405', async () => {
+ const requests: Array<{ url: string; response: Subject<HttpEvent<unknown>>
}> = [];
+ const backend: HttpHandler = {
+ handle: request => {
+ const response = new Subject<HttpEvent<unknown>>();
+ requests.push({ url: request.url, response });
+ return response;
+ }
+ };
+ const client = new HttpClient({ handle: request =>
interceptor.intercept(request, backend) });
+ const navigate = vi.fn(() => Promise.resolve(true));
+ const service = new TicketService(
+ client,
+ { getRestApiBase: () => REST_BASE } as BaseUrlService,
+ { navigate } as unknown as Router,
+ { success: vi.fn() } as unknown as NzMessageService
+ );
+ service.ticket.init = true;
+ service.ticket.principal = 'user1';
+ interceptor = new AppHttpInterceptor(service);
+ const failure = sessionExpired(`${REST_BASE}/login`);
+ const result = firstValueFrom(client.get(`${REST_BASE}/notebook`));
+
+ requests[0].response.error(failure);
+ await expect(result).rejects.toBe(failure);
+ expect(service.logout$.value).toBe(true);
+ expect(requests.map(request =>
request.url)).toEqual([`${REST_BASE}/notebook`, `${REST_BASE}/login/logout`]);
+
+ requests[1].response.error(sessionExpired(`${REST_BASE}/login`));
+
+ expect(requests.map(request =>
request.url)).toEqual([`${REST_BASE}/notebook`, `${REST_BASE}/login/logout`]);
+ expect(service.ticket.init).toBe(false);
+ expect(service.ticket.principal).toBe('');
+ expect(service.logout$.value).toBe(false);
+ expect(navigate).toHaveBeenCalledExactlyOnceWith(['/login']);
+ });
+
+ it.each(['complete', 'error'])('deduplicates pending logout and allows
another after %s', async outcome => {
+ const pendingLogout = new Subject<object>();
+ logout.mockImplementationOnce(() =>
+ defer(() => {
+ logoutSubscribed();
+ return pendingLogout;
+ })
+ );
+ const firstFailure = sessionExpired(`${REST_BASE}/notebook`);
+ const secondFailure = sessionExpired(`${REST_BASE}/interpreter`);
+
+ await expect(intercept(firstFailure,
`${REST_BASE}/notebook`)).rejects.toBe(firstFailure);
+ await expect(intercept(secondFailure,
`${REST_BASE}/interpreter`)).rejects.toBe(secondFailure);
+
+ expect(logout).toHaveBeenCalledTimes(1);
+ expect(logoutSubscribed).toHaveBeenCalledTimes(1);
+
+ if (outcome === 'error') {
+ pendingLogout.error(sessionExpired(`${REST_BASE}/login`));
+ } else {
+ pendingLogout.complete();
+ }
+
+ await expect(intercept(firstFailure,
`${REST_BASE}/notebook`)).rejects.toBe(firstFailure);
+ expect(logout).toHaveBeenCalledTimes(2);
+ expect(logoutSubscribed).toHaveBeenCalledTimes(2);
+ });
+});
diff --git a/zeppelin-web-angular/src/app/app-http.interceptor.ts
b/zeppelin-web-angular/src/app/app-http.interceptor.ts
index 6a6a4a1853..c6a80d9f2c 100644
--- a/zeppelin-web-angular/src/app/app-http.interceptor.ts
+++ b/zeppelin-web-angular/src/app/app-http.interceptor.ts
@@ -12,8 +12,8 @@
import { HttpEvent, HttpHandler, HttpInterceptor, HttpRequest, HttpResponse }
from '@angular/common/http';
import { Injectable } from '@angular/core';
-import { throwError, Observable } from 'rxjs';
-import { catchError, map } from 'rxjs/operators';
+import { EMPTY, throwError, Observable } from 'rxjs';
+import { catchError, finalize, map } from 'rxjs/operators';
import { isNil } from 'lodash';
@@ -22,6 +22,8 @@ import { TicketService } from '@zeppelin/services';
@Injectable()
export class AppHttpInterceptor implements HttpInterceptor {
+ private logoutInProgress = false;
+
constructor(private ticketService: TicketService) {}
// eslint-disable-next-line @typescript-eslint/no-explicit-any
@@ -48,8 +50,18 @@ export class AppHttpInterceptor implements HttpInterceptor {
if (event.status === 401 && !isNil(redirect)) {
// Handle page redirect
window.location.href = redirect;
- } else if (event.status === 405 && !event.url.contains('logout')) {
- this.ticketService.logout().subscribe();
+ } else if (event.status === 405 && !httpRequest.url.includes('logout')
&& !this.logoutInProgress) {
+ this.logoutInProgress = true;
+ this.ticketService
+ .logout()
+ .pipe(
+ // TicketService clears the ticket and navigates even when the
logout request fails.
+ catchError(() => EMPTY),
+ finalize(() => {
+ this.logoutInProgress = false;
+ })
+ )
+ .subscribe();
}
return throwError(event);
})
