[PR] [ZEPPELIN-6643] Log out on session expiry instead of throwing in the interceptor [zeppelin]

["kimyenac (via GitHub)" <gi...@apache.org>]

kimyenac opened a new pull request, #5464:
URL: https://github.com/apache/zeppelin/pull/5464

   ### 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
   


-- 
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: reviews-unsubscribe@zeppelin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [ZEPPELIN-6643] Log out on session expiry instead of throwing in the interceptor [zeppelin]

["voidmatcha (via GitHub)" <gi...@apache.org>]

voidmatcha merged PR #5464:
URL: https://github.com/apache/zeppelin/pull/5464


-- 
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: reviews-unsubscribe@zeppelin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [ZEPPELIN-6643] Log out on session expiry instead of throwing in the interceptor [zeppelin]

["voidmatcha (via GitHub)" <gi...@apache.org>]

voidmatcha commented on PR #5464:
URL: https://github.com/apache/zeppelin/pull/5464#issuecomment-5617923902

   Merged into master (006a3e15).


-- 
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: reviews-unsubscribe@zeppelin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [ZEPPELIN-6643] Log out on session expiry instead of throwing in the interceptor [zeppelin]

["kimyenac (via GitHub)" <gi...@apache.org>]

kimyenac commented on PR #5464:
URL: https://github.com/apache/zeppelin/pull/5464#issuecomment-5599822608

   You're right, and thanks for the diagnosis and the branch.
   
   The defect is mine: I guarded on `event.url`, which is the URL *after* redirects, so a logout POST redirected to `/api/login` returning 405 no longer matched and logout fired again. My tests missed it because I built the fixture from my own assumption about that field instead of from a redirected response — they confirmed the assumption, not the behaviour.
   
   I've cherry-picked your two commits, authorship preserved. Using `httpRequest.url` also makes my "a 405 with no url cannot be identified as the logout call" reasoning unnecessary, since the original request is always known.
   
   `npm run test:shell`: 58 passed. Restoring my original guard fails 5 of the 9 interceptor tests — exactly the cases I should have covered.
   
   On this push both Playwright legs are green, including `run-playwright-e2e-tests (auth, 3.9)`, which is the job that caught the loop. I could not reproduce that failure locally: with shiro enabled behind `ng serve`, the old and new code both stop at `global.setup.ts:42` on "Getting Ticket Data …", short of the line CI fails on, so the CI run is the evidence here rather than a local pass.
   
   Two unrelated reds on the same run, for anyone reading the checks: `npm-audit` fails on the new js-yaml high advisory (GHSA-2883-xcg3-v3hh), which is hitting other branches today as well, and `test-selenium-with-spark-module-for-spark-3-5` errored once in `AuthenticationIT.testSimpleAuthentication` with `ElementClickIntercepted` on the classic UI's login modal. Neither is reachable from this change — the branch edits two files under `zeppelin-web-angular/src/app/`.
   


-- 
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: reviews-unsubscribe@zeppelin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org