[ZEPPELIN-6643] Log out on session expiry instead of throwing in the interceptor - #5464
Open
kimyenac wants to merge 1 commit into
Open
[ZEPPELIN-6643] Log out on session expiry instead of throwing in the interceptor#5464kimyenac wants to merge 1 commit into
kimyenac wants to merge 1 commit into
Conversation
…interceptor
The 405 branch in AppHttpInterceptor guarded logout with
event.url.contains('logout'). JavaScript strings have no contains method,
so the guard threw a TypeError inside catchError before ticketService.logout
was ever reached. The New UI therefore never logged out on session expiry:
the caller observed a TypeError instead of the 405, and the expired session
stayed in place until the user reloaded the page by hand.
The check becomes event.url?.includes('logout'). includes is the method that
exists, and the optional chain covers HttpErrorResponse.url being null, which
it is whenever the failure carries no resolved url. A 405 with no url cannot
be identified as the logout call, so it falls through to logout, which is the
same conclusion the branch already draws for every other request.
The 401 redirect branch is untouched. Tightening the substring match and the
wider typing of this interceptor belong to ZEPPELIN-6469, which waits on this
behaviour being correct first.
The spec constructs the interceptor directly with a logout stub rather than
starting TestBed, since no Angular wiring is involved. It pins the three
things the branch has to get right: a non-logout 405 calls logout exactly
once, that 405 is rethrown to the caller unchanged rather than replaced by a
TypeError, and a 405 from the logout request itself does not call logout
again. Reverting the source line fails three of the four.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this PR for?
The New UI never logs out on session expiry.
AppHttpInterceptorguards its 405 branch withevent.url.contains('logout'), and JavaScript strings have nocontainsmethod, so the guard throws aTypeErrorinsidecatchErrorbeforeticketService.logout()is reached. Both statements after it are skipped: logout never runs, and the caller observes aTypeErrorinstead 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:
includesis the method that exists. The optional chain coversHttpErrorResponse.urlbeingnull, 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
logoutis also skipped, and thecatchErrorparameter is untyped. Both belong to ZEPPELIN-6469, which waits on this behaviour being correct first.The spec constructs the interceptor directly with a
logoutstub rather than startingTestBed, perzeppelin-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 aTypeError, 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?
How should this be tested?
npm run test:shell— 53 tests across 12 files, green. The four new ones are insrc/app/app-http.interceptor.spec.ts.event.url.contains('logout')fails three of the four, withAssertionError: expected TypeError: event.url.contains is not a function to be HttpErrorResponse. The fourth — the recursion guard — passes either way, because theTypeErroralso happens to prevent the logout call; it is there to confirm the guard survives the fix, not to reproduce the bug.npx prettier --checkon both files, clean.npx eslinton both reports only the twoprefer-arrow/prefer-arrow-functionswarnings that the named test helpers produce, the same twosrc/app/services/save-as.service.spec.tsalready reports on master.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 functionfrom the interceptor, with no logout request following it. After this change the same 405 is followed byPOST /api/login/logout.Screenshots (if appropriate)
Not applicable.
Questions: