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

Logout should redirect to the login screen at the server base path #56786

Merged
merged 8 commits into from
Feb 10, 2020

Conversation

legrego
Copy link
Member

@legrego legrego commented Feb 4, 2020

Summary

When logging out, the redirection to the login screen should always be at the root of the server's base path.

This fixes the scenario when logging out of any non-Default space would cause the login screen to be rendered at ${serverBasePath}/s/${spaceId}/login, instead of ${serverBasePath}/login

Resolves #56695

@legrego
Copy link
Member Author

legrego commented Feb 6, 2020

@elasticmachine merge upstream

@legrego legrego added release_note:fix Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! labels Feb 6, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

@legrego legrego marked this pull request as ready for review February 6, 2020 15:41
@legrego legrego requested a review from a team as a code owner February 6, 2020 15:41
@jportner jportner self-requested a review February 6, 2020 20:34
@jportner
Copy link
Contributor

jportner commented Feb 6, 2020

ACK: reviewing now...

@jportner
Copy link
Contributor

jportner commented Feb 7, 2020

Okay, I determined a problem with this PR, and I also discovered two more bugs.

I believe we should be accounting for two flows:

  1. User clicks "Log out" -> user sees Log In screen -> user logs in -> user sees the Space Selection screen
  2. User's session expires -> user sees Log In screen -> user logs in -> user sees whatever screen they were on before session timeout

This PR fixes behavior for the former, but breaks behavior for the latter.

Both of these flows currently function by redirecting the user to the "/logout" endpoint. So perhaps we need a flag on that route to determine whether to preserve the current space in the URL or not.


Aside from that:

I'll open issues for these and investigate.

Edit: I linked the issues above.

@legrego
Copy link
Member Author

legrego commented Feb 7, 2020

Thanks for reviewing and testing!

Both of these flows currently function by redirecting the user to the "/logout" endpoint. So perhaps we need a flag on that route to determine whether to preserve the current space in the URL or not.

I was originally going to fix the front-end "Logout" link, by making sure it always pointed to ${serverBasePath}/logout, but thought fixing it at the source here would be the more correct approach. However, given this breaks the session expiration flow, I think I should revisit this decision and just fix the logout link in the UI. This way session expiration can still remember the previous screen by calling logout from a space-aware context.

Session idle timeout is not working (session lasts forever, no matter which page you are on)
Logging in with a properly-formatted "next" query param doesn't appear to redirect you back to the correct page, you just land on the dashboard

Yikes. I'm surprised we don't have tests for at least the next query param scenario. I know testing idle timeout is rather tricky in an automated fashion in an e2e test

@jportner
Copy link
Contributor

jportner commented Feb 7, 2020

I was originally going to fix the front-end "Logout" link, by making sure it always pointed to ${serverBasePath}/logout, but thought fixing it at the source here would be the more correct approach. However, given this breaks the session expiration flow, I think I should revisit this decision and just fix the logout link in the UI. This way session expiration can still remember the previous screen by calling logout from a space-aware context.

Good idea!

Yikes. I'm surprised we don't have tests for at least the next query param scenario. I know testing idle timeout is rather tricky in an automated fashion in an e2e test

Yeah... if we do have them they might be broken, or if we don't, they need to be added 😛

@legrego
Copy link
Member Author

legrego commented Feb 7, 2020

Flaky test result not related to this PR:

 1) visualize app
12:53:44         
12:53:44           visual builder
12:53:44             switch index patterns
12:53:44               should be able to switch between index patterns:
12:53:44       Error: retry.try timeout: TimeoutError: Waiting for element to be located By(css selector, .euiFilterSelectItem)
12:53:44  Wait timed out after 18009ms
12:53:44      at /dev/shm/workspace/kibana/node_modules/selenium-webdriver/lib/webdriver.js:841:17
12:53:44      at process._tickCallback (internal/process/next_tick.js:68:7)
12:53:44        at onFailure (test/common/services/retry/retry_for_success.ts:28:9)
12:53:44        at retryForSuccess (test/common/services/retry/retry_for_success.ts:68:13)
12:53:44  
12:53:44                   └- ✖ fail: "visualize app  visual builder switch index patterns should be able to switch between index patterns"
12:53:44                   │
12:53:44                 └-> "after all" hook
12:53:44                 └-> "after all" hook
12:53:44               └-> "after all" hook
12:53:44             └-> "after all" hook
12:53:44           └-> "after all" hook

Copy link
Contributor

@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.

LGTM!

@legrego
Copy link
Member Author

legrego commented Feb 10, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

legrego added a commit to legrego/kibana that referenced this pull request Feb 10, 2020
…ic#56786)

* logout should redirect to the login screen at the server base path

* Revert "logout should redirect to the login screen at the server base path"

This reverts commit c80716b.

* fix logout url in nav control service

Co-authored-by: Elastic Machine <[email protected]>
legrego added a commit that referenced this pull request Feb 10, 2020
* Logout should redirect to the login screen at the server base… (#56786)

* logout should redirect to the login screen at the server base path

* Revert "logout should redirect to the login screen at the server base path"

This reverts commit c80716b.

* fix logout url in nav control service

Co-authored-by: Elastic Machine <[email protected]>

* fix merge conflict

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:fix Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.6.1 v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logging out maintains active space
4 participants