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

Improve session idle timeout, add session lifespan #49855

Merged
merged 26 commits into from
Nov 26, 2019

Conversation

jportner
Copy link
Contributor

@jportner jportner commented Oct 31, 2019

Summary

The core purpose of this PR is to add an absolute timeout, or "lifespan", to user sessions. To facilitate this, changes needed to be made to the existing session idle timeout functionality.

If a lifespan is defined, sessions will expire at that point in time. If an idle timeout is also defined, then sessions can be extended up to the end of that lifespan. When a user is about to reach the end of their session lifespan, they will see a warning toast similar to the existing one for idle session timeout.

Changes to existing functionality are primarily on the client-side. Before, the client was completely unaware of when its session would actually expire -- it would just estimate this based on the injected config values. Now, the client fetches this information from an API, and shares this across tabs to synchronize the timers to all of them. This results in a much better user experience where these popups behave more logically, and one open tab will not erroneously log users out when the session has been extended in another tab.

Deprecates config option xpack.security.sessionTimeout, the new name is xpack.security.session.idleTimeout to better reflect its function.
Adds config option xpack.security.session.lifespan to control the session lifespan.

Note: there is a known bug, #22440, that prevents the session expiration message from showing on multiple tabs. 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 #18566.
Resolves #48859.

Screenshots

All warnings and messages now display on multiple tabs.

Added new session lifespan warning with progress indicator and countdown timer:

Screen Shot 2019-11-20 at 10 18 00 AM

Modified existing session idle timeout warning to match:

Screen Shot 2019-11-20 at 10 17 25 AM

Logout messages now display on all tabs when using basic or token auth providers:

Screen Shot 2019-11-20 at 10 19 20 AM

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@jportner jportner changed the title Issue 18566 absolute session timeout Change session timeout functionality, enforce absolute session timeout Oct 31, 2019
@jportner jportner changed the title Change session timeout functionality, enforce absolute session timeout Change session idle timeout, enforce absolute timeout Oct 31, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jportner jportner force-pushed the issue-18566-absolute-session-timeout branch 4 times, most recently from 1b2e61e to 20298ea Compare November 1, 2019 21:28
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jportner jportner force-pushed the issue-18566-absolute-session-timeout branch 3 times, most recently from 0959914 to b80b0e3 Compare November 5, 2019 02:25
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@jportner
Copy link
Contributor Author

jportner commented Nov 5, 2019

Note to reviewers: two commits (e143e48 and 7bc88ed) are concerned with how to define special routes which should not extend a user's session. After I wrote this code, @azasypkin pointed out to me that we have an existing mechanism (isSystemAPIRequest) to achieve the same end result.

Despite its name, this uses an HTTP request header to determine whether to extend a session or not. We both agree that this mechanism could use another look, and perhaps could be rewritten (such as the static route-based options that I defined in the two commits above). However, in the spirit of separation of concerns and making this PR a bit lighter, I opted to defer any such change/s to sometime in the future.

So, I reverted these two commits, but left them in the history in the PR for future reference.
Please ignore these for the purposes of reviewing. Thanks!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@jportner jportner force-pushed the issue-18566-absolute-session-timeout branch from 1f3bc81 to a79bc07 Compare November 7, 2019 20:53
@jportner jportner changed the title Change session idle timeout, enforce absolute timeout Improve session idle timeout, add session lifespan Nov 7, 2019
@jportner jportner force-pushed the issue-18566-absolute-session-timeout branch from a79bc07 to f92f629 Compare November 7, 2019 21:48
package.json Outdated Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Previously, the lifespan warning would flash on a user's screen
every time they clicked a link, because that would result in an
API call to get updated session info. Added a check to prevent
that from happening.
@legrego legrego self-requested a review November 21, 2019 18:55
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

Operations stuff LGTM

Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

I apologize for another round of nits. This is functioning really well, nice work!

x-pack/plugins/security/public/session/session_timeout.tsx Outdated Show resolved Hide resolved
x-pack/plugins/security/public/session/session_timeout.tsx Outdated Show resolved Hide resolved
x-pack/plugins/security/public/session/session_timeout.tsx Outdated Show resolved Hide resolved
x-pack/plugins/security/public/plugin.ts Outdated Show resolved Hide resolved
x-pack/package.json Outdated Show resolved Hide resolved
@legrego legrego self-requested a review November 25, 2019 16:31
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

LGTM pending merge conflicts/green CI!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@jportner jportner merged commit 38c17d6 into elastic:master Nov 26, 2019
jportner added a commit to jportner/kibana that referenced this pull request Nov 26, 2019
This adds an absolute session timeout (lifespan) to user sessions.
It also improves the existing session timeout toast and the overall
user experience in several ways.
jportner added a commit that referenced this pull request Nov 26, 2019
…1740)

This adds an absolute session timeout (lifespan) to user sessions.
It also improves the existing session timeout toast and the overall
user experience in several ways.
@jportner jportner deleted the issue-18566-absolute-session-timeout branch November 26, 2019 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:deprecation Team:Docs Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Session idle timeout is triggered when session has been extended Absolute Session Timeout
8 participants