Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix login redirect for expired sessions #57157

Merged
merged 7 commits into from
Feb 16, 2020

Conversation

jportner
Copy link
Contributor

@jportner jportner commented Feb 7, 2020

This PR changes the behavior of session expiration logouts. It removes logic that strips basePath out of the next parameter.

Note: there is a known bug, #22440, that prevents the session expiration message and next parameter from showing on the login page when the user fails authentication in the legacy platform. That is partially addressed in this PR (for the basic and token auth providers), but it will need to be fully addressed in a separate PR.

Resolves #57113.

@jportner jportner requested a review from kobelb February 7, 2020 21:33
@jportner jportner marked this pull request as ready for review February 7, 2020 23:26
@jportner jportner requested a review from a team as a code owner February 7, 2020 23:26
Copy link
Contributor

@kobelb kobelb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this! Your changes address a majority of the issues that I've been able to find with our logout behavior. There's one other super-edge case here that Larry recently fixed with #56786 and #57201. When the user is auto logged out of a non-default space, they're still being redirected to the equivalent of http://localhost/s/foo/login instead of http://localhost/login. At this point, I think it's just cosmetic because the next query-string parameter prevents #56695 from occurring, but there might be some actual issue I'm overlooking or a future issue. How would you feel about using this PR to consume the logout URL injected metadata so we're consistent here also?

Use injected logoutUrl instead of basePath.
Also added `provider` param to legacy autoLogout service, and
rewrote tests for SessionExpired.
@jportner
Copy link
Contributor Author

jportner commented Feb 11, 2020

Thanks for fixing this! Your changes address a majority of the issues that I've been able to find with our logout behavior. There's one other super-edge case here that Larry recently fixed with #56786 and #57201. When the user is auto logged out of a non-default space, they're still being redirected to the equivalent of http://localhost/s/foo/login instead of http://localhost/login. At this point, I think it's just cosmetic because the next query-string parameter prevents #56695 from occurring, but there might be some actual issue I'm overlooking or a future issue. How would you feel about using this PR to consume the logout URL injected metadata so we're consistent here also?

Ah, I also think it's cosmetic but agree it's safer/better to use the single logout URL everywhere. I tested it and it worked fine that way, so I made the change. Good idea!

Copy link
Contributor Author

@jportner jportner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Author's notes.

@jportner
Copy link
Contributor Author

@elasticmachine merge upstream

@kobelb
Copy link
Contributor

kobelb commented Feb 14, 2020

ACK: rereviewing

@jportner
Copy link
Contributor Author

retest

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jportner jportner merged commit 3b232ae into elastic:master Feb 16, 2020
@jportner jportner deleted the issue-57113-fix-login-redirect branch February 16, 2020 00:32
jportner added a commit to jportner/kibana that referenced this pull request Feb 16, 2020
jloleysens added a commit to jloleysens/kibana that referenced this pull request Feb 17, 2020
…re/files-and-filetree

* 'master' of github.com:elastic/kibana: (139 commits)
  Move Ace XJSON lexer-rules, worker and utils to es_ui_shared (elastic#57563)
  [Upgrade Assistant] Fix filter deprecations search filter (elastic#57541)
  [ML] New Platform server shim: update indices routes (elastic#57685)
  Bump redux dependencies (elastic#53348)
  [Index management] Client-side NP ready (elastic#57295)
  change id of x-pack event_log plugin to eventLog (elastic#57612)
  [eventLog] get kibana.index name from config instead of hard-coding it (elastic#57607)
  revert
  allow using any path to generate
  fixes ui titles (elastic#57535)
  Fix login redirect for expired sessions (elastic#57157)
  Expose Vis on the contract as it requires visTypes (elastic#56968)
  [SIEM][Detection Engine] Fixes queries to ignore errors when signals index is not present
  [Remote clusters] Migrate client-side code out of legacy (elastic#57365)
  Fix failed test reporter for SIEM Cypress use (elastic#57240)
  skip flaky suite (elastic#45244)
  update chromedriver to 80.0.1 (elastic#57602)
  change slack action to only report on whitelisted host name (elastic#57582)
  [kbn/optimizer] throw errors into stream on invalid completion (elastic#57735)
  moving visualize/utils to new platform (elastic#56650)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kibana doesn't redirect to correct page after session expiration
4 participants