Handle session timeout and user activity #98461
Conversation
thomheymann
added
release_note:enhancement
v8.0.0
v7.14.0
auto-backport
There was a problem hiding this comment.
It's looking good overall, I like the simplified UI elements a lot.
I'm still ambivalent about the response interceptor that updates expiresInMs. I think it adds unnecessary complexity that can introduce some unexpected behavior (see one of my comments below).
We already have the refresh timer that will fetch updated session info before showing the warning toast to the user.
The old purpose of the response interceptor before that if a warning toast was already visible, and we detected a request that looked like it had extended the user's session, we would fetch the latest session info from the server (and subsequently dismiss the toast automatically). I'd prefer if we go back to that approach, it seems simpler and less error-prone, the only downside is we have a single extra HTTP request which is negligible. I'm interested to hear what @azasypkin thinks about this when he reviews, too!
| isLifespanTimeout = true; | ||
| expiresInMs = lifespanExpiration - now; | ||
| canBeExtendedByMs = 0; | ||
| } |
There was a problem hiding this comment.
I haven't tested this but I believe we need an additional conditional here:
| } | |
| } else if ( | |
| lifespanExpiration && | |
| idleTimeoutExpiration && | |
| lifespanExpiration - idleTimeoutExpiration < canBeExtendedByMs | |
| ) { | |
| canBeExtendedByMs = lifespanExpiration - idleTimeoutExpiration; | |
| } |
| if (currentState.canBeExtendedByMs) { | ||
| const nextState = { | ||
| ...currentState, | ||
| lastExtensionTime: Date.now(), | ||
| expiresInMs: currentState.canBeExtendedByMs, | ||
| }; | ||
| this.sessionState$.next(nextState); |
There was a problem hiding this comment.
I haven't tested this, but it seems like it could run into issues in some corner cases. It would be difficult to trigger this behavior manually but I think could be checked in unit tests.
Example:
- I configure Kibana with an
idleTimeoutof 2m and alifespanof 5m - I log in, I wait 59s, and I refresh the page (
idleTimeoutExpirationis 2m from now,lifespanExpirationis 4m1s from now, socanBeExtendedByMsis set to 2000 andexpiresInMsis set to 2m from now) - I wait 1m59s, and I make an API call (
idleTimeoutExpirationis 2m from now,lifespanExpirationis 2m2s from now, andexpiresInMsis set to 2m from now) - I wait 59s, and I make an API call (
idleTimeoutExpirationis 2m from now,lifespanExpirationis 1m3s from now, andexpiresInMsis set to 2m from now)
All of the client-side timers are calculated based on expiresInMs, but they are now wrong. The session will expire 57s before the client thinks it will expire. Of course this is a contrived example but you could imagine it happening with larger values for idleTimeout and idleTimeoutExpiration.
I think the worst thing we can do here is to allow the session to expire without showing anything to a user -- if your session expires but the page doesn't know it, then click a button and all of a sudden you are kicked back to the login page, that's a frustrating experience.
One option that I can see to handle this edge case is to update the SessionState interface and add a maxExpiresInMs value that is calculated based on the lifespanExpiration. This could be calculated when SessionInfo is fetched from the server, and it could be used to ensure we don't set expiresInMs to an unrealistic value.
I had to write it out to wrap my head around it, so I'll just paste the diff here in case you want to use this approach:
See diff
diff --git a/x-pack/plugins/security/public/session/session_timeout.tsx b/x-pack/plugins/security/public/session/session_timeout.tsx
index 8fe9865854f..fcd72e02397 100644
--- a/x-pack/plugins/security/public/session/session_timeout.tsx
+++ b/x-pack/plugins/security/public/session/session_timeout.tsx
@@ -58,6 +58,7 @@ export interface ISessionTimeout {
export interface SessionState {
lastExtensionTime: number;
expiresInMs: number;
+ maxExpiresInMs: number;
canBeExtendedByMs: number;
}
@@ -70,6 +71,7 @@ export class SessionTimeout {
private sessionState$ = new BehaviorSubject<SessionState>({
lastExtensionTime: 0,
expiresInMs: 0,
+ maxExpiresInMs: 0,
canBeExtendedByMs: 0,
});
private subscription?: Subscription;
@@ -107,6 +109,7 @@ export class SessionTimeout {
const disabled = {
lastExtensionTime: 0,
expiresInMs: 0,
+ maxExpiresInMs: 0,
canBeExtendedByMs: 0,
};
this.toggleEventHandlers(disabled);
@@ -149,11 +152,16 @@ export class SessionTimeout {
// Only mark session as extended if not a system request
if (httpResponse.fetchOptions.asSystemRequest === false) {
const currentState = this.sessionState$.getValue();
- if (currentState.canBeExtendedByMs) {
+ const { expiresInMs, maxExpiresInMs, canBeExtendedByMs } = currentState;
+ if (canBeExtendedByMs && (!maxExpiresInMs || maxExpiresInMs > expiresInMs)) {
+ let newExpiresInMs = canBeExtendedByMs;
+ if (maxExpiresInMs && expiresInMs > maxExpiresInMs) {
+ newExpiresInMs = maxExpiresInMs;
+ }
const nextState = {
...currentState,
lastExtensionTime: Date.now(),
- expiresInMs: currentState.canBeExtendedByMs,
+ expiresInMs: newExpiresInMs,
};
this.sessionState$.next(nextState);
if (this.channel) {
@@ -268,12 +276,16 @@ export class SessionTimeout {
});
let expiresInMs = 0;
+ let maxExpiresInMs = 0;
let canBeExtendedByMs = sessionInfo.idleTimeout || 0;
const { now, idleTimeoutExpiration, lifespanExpiration } = sessionInfo;
if (idleTimeoutExpiration) {
expiresInMs = idleTimeoutExpiration - now;
}
+ if (lifespanExpiration) {
+ maxExpiresInMs = lifespanExpiration - now;
+ }
if (
lifespanExpiration &&
(idleTimeoutExpiration === null || lifespanExpiration <= idleTimeoutExpiration)
@@ -285,6 +297,7 @@ export class SessionTimeout {
const nextState = {
lastExtensionTime: Date.now(),
expiresInMs,
+ maxExpiresInMs,
canBeExtendedByMs,
};
this.sessionState$.next(nextState);There was a problem hiding this comment.
Good catch! There could be an issue when both, idleTimeout and lifespan are configured, where the session might get extended beyond its lifespan.
We can change this to fetch session information from the server after intercepting an HTTP response rather than "blindly" incrementing it client side. However, that would essentially double the HTTP requests the front-end makes which I'd prefer to avoid.
Is there a third option where any API call that extends the session returns that information as a custom header in its response so that the client can read that out instead of manually polling?
There was a problem hiding this comment.
I'm sure it's possible to tack on an additional HTTP response header but it sounds a bit wasteful IMO, not sure how the Core team would feel about it.
I don't have stats in front of me but I'm pretty confident that the vast majority of users won't have an idleTimeout less than 30 minutes (that is what NIST recommends). As I mentioned in #98461 (review), I don't think we need to update the client's timers with every response -- we already have a refresh timer set to kick off right before we show the warning.
Since the toast is shown 5 minutes before the session expires, assuming a 30 minute idle timeout, we are fetching session info once every 25 minutes. Or, to rephrase: this bit in the response interceptor to extend the client-side timers is really only saving us one HTTP request every 25 minutes. I don't think it's worth the additional complexity.
There was a problem hiding this comment.
The number of API calls we would introduce to the extend session endpoint is higher than that unfortunately. It’s defined by EXTENSION_THROTTLE_MS which I have set to 1 minute. So it would currently be 1 extra API call per minute for every single user. That's probably totally acceptable but want to make sure we're not adding unnecessary traffic.
There was a problem hiding this comment.
The number of API calls we would introduce to the extend session endpoint is higher than that unfortunately. It’s defined by
EXTENSION_THROTTLE_MSwhich I have set to 1 minute. So it would currently be 1 extra API call per minute for every single user. That's probably totally acceptable but want to make sure we're not adding unnecessary traffic.
I think an extra call per user per minute is acceptable. The amount of work we do for this particular request is minimal comparatively, and I don't foresee this being problematic in production setups.
| // Ignore requests to external URLs | ||
| if (!fetchOptions.path.startsWith('/')) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I don't think we need this, and I think it will actually miss valid internal requests that use relative paths such as fetch('some/api/call'.
This request interceptor doesn't handle all HTTP requests (e.g., those made by window.fetch()), it only handles HTTP requests which were made using the Core HTTP service's API wrapper (http.fetch()). This wrapper can only be used to fetch data from Kibana's backend (see fetch.ts, dev docs).
There was a problem hiding this comment.
Unfortunately we do need that condition since the interface is used to fetch the newsfeed from staging/production.
I'm not sure if any uses relative URLs like fetch('some/api/call' since that would be incredibly flaky but I can update the condition to be more restrictive.
There was a problem hiding this comment.
Ah I wasn't aware this is used for newsfeed, good catch. I guess http.fetch` wasn't intended to be used for third-party URLs but 🤷♂️
OK, in that case it's probably better to change it to this:
if (fetchOptions.path.startsWith('http')) {
return;
}If we went this route, we could wind up setting the kbn-system-request header if a plugin used fetch with a path that doesn't specify the protocol, like example.com/foo, but there's really no harm in that. I'd prefer to take the safest route to limit false negatives so we don't accidentally allow Kibana requests to extend the session in the background when they shouldn't.
Alternatively if you wanted to get really specific you could do something like this:
import { parse as parseUrl } from 'url';
...
const url = parseUrl(fetchOptions.path, false, true); // slashesDenoteHost: true
if (url.host !== null) {
return;
}We already use url in the security plugin's public code so we won't be increasing the bundle size by using it here. But I'd like to remove the existing usage of that module at some point if possible because it adds quite a bit to the bundle size.
|
@gchaps What are your thoughts on the copy for the session timeout toast? Idle timeout reached (can be extended)
Lifetime reached (cannot be extended)
I was thinking instead of "You will need to log in again" we could also say something along the lines of "Please save any outstanding work". |
|
@elasticmachine merge upstream |
| export function defineSessionInfoRoutes({ router, getSession }: RouteDefinitionParams) { | ||
| export function defineSessionInfoRoutes({ router, getSession, config }: RouteDefinitionParams) { | ||
| router.get( | ||
| { path: '/internal/security/session', validate: false }, | ||
| async (_context, request, response) => { | ||
| const sessionValue = await getSession().get(request); | ||
| if (sessionValue) { | ||
| const expirationTime = | ||
| sessionValue.idleTimeoutExpiration && sessionValue.lifespanExpiration | ||
| ? Math.min(sessionValue.idleTimeoutExpiration, sessionValue.lifespanExpiration) | ||
| : sessionValue.idleTimeoutExpiration || sessionValue.lifespanExpiration; | ||
|
|
||
| return response.ok({ | ||
| body: { | ||
| // We can't rely on the client's system clock, so in addition to returning expiration timestamps, we also return | ||
| // the current server time -- that way the client can calculate the relative time to expiration. | ||
| now: Date.now(), | ||
| idleTimeoutExpiration: sessionValue.idleTimeoutExpiration, | ||
| lifespanExpiration: sessionValue.lifespanExpiration, | ||
| expiresInMs: expirationTime ? expirationTime - Date.now() : null, | ||
| canBeExtended: | ||
| sessionValue.idleTimeoutExpiration !== null && | ||
| expirationTime !== sessionValue.lifespanExpiration, | ||
| provider: sessionValue.provider, | ||
| } as SessionInfo, | ||
| }); |
There was a problem hiding this comment.
I like your approach here, it makes more sense than sending three dates to the client.
I did notice one bug that users can run into though -- if idleTimeoutExpiration and lifespanExpiration are both non-null and they are less than 5 minutes apart (the amount of time that the warning toast is shown to the user), you can run into this situation:
Technically, was the user able to extend his session? Yes... but it would be frustrating to run into this.
Before, when we were sending the lifespanExpiration to the client, we were able to account for this and handle it accordingly (IIRC we just showed the "You will need to log in again" version of the toast earlier).
One way to prevent this behavior is:
- Move
WARNING_MStox-pack/plugins/security/common/constants.ts(and maybe rename it toSESSION_EXPIRATION_WARNING_MS?) - Then change this to use something like:
const canBeExtended = sessionValue.idleTimeoutExpiration !== null && expirationTime !== null && (sessionValue.lifespanExpiration === null || expirationTime + SESSION_EXPIRATION_WARNING_MS < sessionValue.lifespanExpiration);
Of course that wouldn't actually prevent the session from being extended, though we could modify x-pack/plugins/security/server/session_management/session.ts to take that into account too? 🤷♂️
| if (fetchOptions.asSystemRequest === undefined) { | ||
| return { asSystemRequest: !this.isVisible }; | ||
| } |
There was a problem hiding this comment.
It looks like this will clobber other fetch options. Also if some other code has explicitly set asSystemRequest to false, I think it's safe to say we want to override it here.
| if (fetchOptions.asSystemRequest === undefined) { | |
| return { asSystemRequest: !this.isVisible }; | |
| } | |
| if (!fetchOptions.asSystemRequest && !this.isVisible) { | |
| return { ...fetchOptions, asSystemRequest: true }; | |
| } |
There was a problem hiding this comment.
Ah, good catch, that would have been a nightmare to debug!
If asSystemRequest is explicitly set to false already then we don't need to do anything so it's better to treat it as a noop in that case and return void.
There was a problem hiding this comment.
If
asSystemRequestis explicitly set to false already then we don't need to do anything so it's better to treat it as a noop in that case and return void.
I'm not sure I follow? Regardless of what the consumer has explicitly, I think if the page is not visible, we want to set asSystemRequest to true, right? Otherwise the session would be extended when the user is not using the page.
There was a problem hiding this comment.
My assumption would be that we don't want to do that. If a developer explicitly sets asSystemRequest to false I would trust that they've got a good reason to do so.
There was a problem hiding this comment.
I feel pretty strongly that we don't want other plugins to circumvent security controls. There is no reason to explicitly set asSystemRequest to false, if a developer does that, they are misunderstanding the option, and it shouldn't result in an infinitely extended session when this is used for polling requests.
| } | ||
|
|
||
| // Extend session unless we're dealing with a system request | ||
| if (httpResponse.fetchOptions.asSystemRequest === false) { |
There was a problem hiding this comment.
This can be (and usually is?) undefined, so we should probably change this from an iff to an if:
| if (httpResponse.fetchOptions.asSystemRequest === false) { | |
| if (!httpResponse.fetchOptions.asSystemRequest) { |
There was a problem hiding this comment.
I only want to extend the session if this has been explicitly set to be a user request.
There can be situations, especially when the page loads / service starts, where requests are sent before our request intercepter has been registered but the response is received when our response interceptor is already active in which case we don't know how to handle the request.
There was a problem hiding this comment.
Oh! I didn't put two and two together and I assumed the request interceptor and response interceptor were completely independent. I did not realize one is intended to impact the other. Some additional comments here to clarify that would be appreciated then 🙏
| // Extend session unless we're dealing with a system request | ||
| if (httpResponse.fetchOptions.asSystemRequest === false) { | ||
| if (this.shouldExtend()) { | ||
| this.fetchSessionInfo(true); | ||
| } | ||
| } |
There was a problem hiding this comment.
I know that this.shouldExtend() will prevent us from extending the session more than once a minute, but I don't think we even need to do this much. If a non-system-request went through, it already extended the user's session. We check session expiration right before showing the warning, so the only time we need to know about this response is after the warning is visible to the user, right? (because then the warning is no longer valid, their session has already been extended)
Correct me if I'm wrong, but it looks like the way this is currently written: If the warning is NOT showing, and we detected a response to a non-system-request, make an API call to extend the session and fetch new session info.
If so, it seems like we are making extra API calls without any actual change in behavior. The session was already extended, and we already check the latest session info before showing the warning.
Does that sound right to you?
If you agree, also taking into account #98461 (comment), I think we should probably change this to something like:
if (!httpResponse.fetchOptions.asSystemRequest && this.warningToast) {
this.fetchSessionInfo();and we could maybe update the fetchSessionInfo function so that calling it multiple times before session info has been fetched won't result in multiple HTTP requests.
There was a problem hiding this comment.
I originally built this so that we mark the session as extended without making an additional API call. Are you proposing to revert this again?
Ignoring the implementation, which of these behaviours do you want to change / add to?
- extends session when detecting user activity
- extends session when receiving HTTP response
- refetches session info before warning is displayed
- does not extend session if recently extended
- marks HTTP requests as system requests when tab is not visible
- does not mark HTTP requests to external URLs as system requests
- marks HTTP requests as user requests when tab is visible
- resets timers when receiving message from other tabs
- shows warning before session expires
- hides warning if session gets extended
- logs user out slightly before session expires
- logs user out immediately if session expiration is imminent
- does not log user out if session does not expire
There was a problem hiding this comment.
- extends session when receiving HTTP response
I don't understand why this has been added, and I don't see the reason for it. It seems like it's adding extra complexity and API calls for no impact on the user experience.
There was a problem hiding this comment.
The reason this is there is to ensure that the session remains active for e.g. dashboards where no one is actively interacting with the page but the app is polling for changes.
There was a problem hiding this comment.
But the app polling for changes without asSystemRequest: true is already extending the session simply by making the requests. So what is this adding?
| } | ||
|
|
||
| private fetchSessionInfo = async (extend = false) => { | ||
| this.isExtending = true; |
There was a problem hiding this comment.
Nit: this is true whether the session is actually being extended or not. Perhaps it should be renamed to isFetchingSessionInfo? And/or split out into a separate variable so you can track these two things independently?
|
Is "end" the right word? Or might this wording be more appropriate:
For the message at 30 seconds, is that enough time for the user to save their work? And what's the message if there are no unsaved changes. Could the message be the same as above:
Or maybe this:
|
Thanks for suggestions @gchaps ! I tried to keep it short since there's not a lot of space in the toast and it starts to wrap which doesn't look great:
What do you think? Can we drop anything?
We can't detect whether there are any unsaved changes or not so maybe its better to keep the message neutral. |
There was a problem hiding this comment.
A couple more comments below.
In addition: after the session timeout toast was displayed, I tried to close it out using the "X" button, and this happened 🙈
It fetched new session info but did not dismiss the toast. For reference, my kibana.yml is configured with xpack.security.session.idleTimeout: 8m.
I think the behavior that we actually want here is to allow the toast to be dismissed and treat this as the user having clicked "Stay logged in".
Edit: I forgot to mention when I wrote this: I tested the behavior pretty thoroughly with different combinations of idleTimeout/lifespan, multiple browser tabs, etc. and, aside from the bug above, I think this is working great!
| return; | ||
| } | ||
|
|
||
| if (fetchOptions.asSystemRequest === undefined) { |
There was a problem hiding this comment.
I still think we need to change this so sessions cannot be extended indefinitely when the user is inactive: #98461 (comment)
| if (fetchOptions.asSystemRequest === undefined) { | |
| if (!fetchOptions.asSystemRequest) { |
There was a problem hiding this comment.
I think if a developer flags a request as either a user or a system request we should to respect that.
Otherwise we render the flag useless and should remove it completely.
There was a problem hiding this comment.
Otherwise we render the flag useless and should remove it completely.
I think what we need is to forbid false value for this flag. This way API consumers won't be able to unknowingly reduce effectiveness of our security controls and can either force it to be true or rely on the default behavior. But, yeah, it'd probably make sense to phase it out completely eventually.
| * @returns Function to remove all event handlers from window. | ||
| */ | ||
| function monitorActivity(callback: () => void) { | ||
| const eventTypes = ['mousemove', 'mousedown', 'wheel', 'touchstart', 'keydown']; |
There was a problem hiding this comment.
Thinking about this from an a11y perspective: I am guessing that because we included keydown here, using a screen reader will also count as session activity 👍.
I imagine that screen readers will also keep the window visible so user activity will still be captured. I'm not entirely sure, and I've never used a screen reader before. If you haven't already done so, it would be good to run this by someone knowledgeable in this area.
There was a problem hiding this comment.
Good point. I think screen readers trigger keyboard events so this should be covered but ket me double check with Michail.
@myasonik, is keydown going to be triggered when using a screen reader to interact with the page (e.g. moving between elements, interacting with the UI, "scrolling" etc.)?
|
ACK: reviewing... |
There was a problem hiding this comment.
I like the new approach! I left mostly nits and there is one weird thing:
When I try to close the toast it's not closed immediately until I click somewhere else (not the most obvious gif, but there I'm clicking all the time 🙈 ). Is it something expected or not? I see that some other toasts are closed immediately.
x-pack/plugins/security/server/routes/session_management/info.ts
Outdated
Show resolved
Hide resolved
x-pack/test/security_api_integration/tests/session_idle/extension.ts
Outdated
Show resolved
Hide resolved
| return; | ||
| } | ||
|
|
||
| if (fetchOptions.asSystemRequest === undefined) { |
Haha, yeh, this is definitely not as it should be, thanks for raising. Fixed this now. Didn't realise that overriding |
|
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
Unknown metric groupsReferences to deprecated APIs
History
To update your PR or re-run it, just comment with: |
* Handle session timeout and user activity * Added suggestions form code review * Fix i18n errors * Moved session timeout calculation to server side * fixed tests * Added suggestions from code review * fix types * Removed unecessary response interceptor and improved copy * Handle scenario when session does not exist correctly * Do not show warning again if previously dismissed * Added suggestions from code review Co-authored-by: Kibana Machine <[email protected]>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
* Handle session timeout and user activity * Added suggestions form code review * Fix i18n errors * Moved session timeout calculation to server side * fixed tests * Added suggestions from code review * fix types * Removed unecessary response interceptor and improved copy * Handle scenario when session does not exist correctly * Do not show warning again if previously dismissed * Added suggestions from code review Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Thom Heymann <[email protected]>
timroes
added
the
Team:Security
|
Pinging @elastic/kibana-security (Team:Security) |
Resolves #79889
Resolves #18751
Summary
This PR improves our session timeouts in a number of ways:
Idle timeout reached
Lifetime reached
Testing
Enable session timeout using:
Checklist