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

Add concurrent session limit logout message #152949

Merged
merged 1 commit into from
Mar 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions x-pack/plugins/security/common/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export interface SessionInfo {

export enum LogoutReason {
'SESSION_EXPIRED' = 'SESSION_EXPIRED',
'CONCURRENCY_LIMIT' = 'CONCURRENCY_LIMIT',
'AUTHENTICATION_ERROR' = 'AUTHENTICATION_ERROR',
'LOGGED_OUT' = 'LOGGED_OUT',
'UNAUTHENTICATED' = 'UNAUTHENTICATED',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,12 @@ const loginFormMessages: Record<LogoutReason, NonNullable<LoginFormProps['messag
defaultMessage: 'Your session has timed out. Please log in again.',
}),
},
CONCURRENCY_LIMIT: {
type: LoginFormMessageType.Info,
content: i18n.translate('xpack.security.login.concurrencyLimitDescription', {
defaultMessage: 'You have logged in on another device. Please log in again.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit/question: As the number of concurrent sessions is configurable, should we be more explicit? e.g. "The number of allowable concurrent logins has been exceeded." or "You have logged in on another device, exceeding the concurrent session limit of N."
Not sure this is necessary, but thought I'd mention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I phrased this intentionally simple as I don't think the technical details / specific settings are important here for the user. As an administrator it might be of value but my thinking was that they can refer to the debug logs which contains more information about the exact logout reason if required.

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems reasonable to me. 👍

}),
},
AUTHENTICATION_ERROR: {
type: LoginFormMessageType.Info,
content: i18n.translate('xpack.security.login.authenticationErrorDescription', {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { setup } from '@kbn/core-test-helpers-http-setup-browser';
import { applicationServiceMock } from '@kbn/core/public/mocks';

import { SESSION_ERROR_REASON_HEADER } from '../../common/constants';
import { LogoutReason } from '../../common/types';
import { SessionExpired } from './session_expired';
import { UnauthorizedResponseHttpInterceptor } from './unauthorized_response_http_interceptor';

Expand All @@ -38,9 +39,15 @@ afterEach(() => {
fetchMock.restore();
});

for (const reason of ['AUTHENTICATION_ERROR', 'SESSION_EXPIRED']) {
for (const reason of [
LogoutReason.AUTHENTICATION_ERROR,
LogoutReason.SESSION_EXPIRED,
LogoutReason.CONCURRENCY_LIMIT,
]) {
const headers =
reason === 'SESSION_EXPIRED' ? { [SESSION_ERROR_REASON_HEADER]: reason } : undefined;
reason === LogoutReason.SESSION_EXPIRED || reason === LogoutReason.CONCURRENCY_LIMIT
? { [SESSION_ERROR_REASON_HEADER]: reason }
: undefined;

it(`logs out 401 responses (reason: ${reason})`, async () => {
const http = setupHttp('/foo');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ export class UnauthorizedResponseHttpInterceptor implements HttpInterceptor {
if (response.status === 401) {
const reason = response.headers.get(SESSION_ERROR_REASON_HEADER);
this.sessionExpired.logout(
reason === LogoutReason.SESSION_EXPIRED
? LogoutReason.SESSION_EXPIRED
reason === LogoutReason.SESSION_EXPIRED || reason === LogoutReason.CONCURRENCY_LIMIT
? reason
: LogoutReason.AUTHENTICATION_ERROR
);
controller.halt();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import { ConfigSchema, createConfig } from '../config';
import { securityFeatureUsageServiceMock } from '../feature_usage/index.mock';
import { securityMock } from '../mocks';
import {
SessionConcurrencyLimitError,
type SessionError,
SessionExpiredError,
SessionMissingError,
Expand Down Expand Up @@ -1356,7 +1357,12 @@ describe('Authenticator', () => {
expectAuditEvents({ action: 'user_login', outcome: 'failure' });
});

for (const FailureClass of [SessionMissingError, SessionExpiredError, SessionUnexpectedError]) {
for (const FailureClass of [
SessionMissingError,
SessionExpiredError,
SessionConcurrencyLimitError,
SessionUnexpectedError,
]) {
describe(`session.get results in ${FailureClass.name}`, () => {
it('fails as expected for redirectable requests', async () => {
const request = httpServerMock.createKibanaRequest();
Expand Down Expand Up @@ -1455,7 +1461,10 @@ describe('Authenticator', () => {

const authenticationResult = await authenticator.authenticate(request);
expect(authenticationResult.redirected()).toBe(true);
if (failureReason instanceof SessionExpiredError) {
if (
failureReason instanceof SessionExpiredError ||
failureReason instanceof SessionConcurrencyLimitError
) {
expect(authenticationResult.redirectURL).toBe(
redirectUrl + '&msg=' + failureReason.code
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import { getErrorStatusCode } from '../errors';
import type { SecurityFeatureUsageServiceStart } from '../feature_usage';
import {
type Session,
SessionConcurrencyLimitError,
SessionExpiredError,
SessionUnexpectedError,
type SessionValue,
Expand Down Expand Up @@ -386,7 +387,8 @@ export class Authenticator {
)}`
: ''
}${
existingSession.error instanceof SessionExpiredError
existingSession.error instanceof SessionExpiredError ||
existingSession.error instanceof SessionConcurrencyLimitError
? `&${LOGOUT_REASON_QUERY_STRING_PARAMETER}=${encodeURIComponent(
existingSession.error.code
)}`
Expand Down Expand Up @@ -420,7 +422,8 @@ export class Authenticator {

if (requestIsRedirectable) {
if (
existingSession.error instanceof SessionExpiredError &&
(existingSession.error instanceof SessionExpiredError ||
existingSession.error instanceof SessionConcurrencyLimitError) &&
authenticationResult.redirectURL?.startsWith(
`${this.options.basePath.get(request)}/login?`
)
Expand Down Expand Up @@ -479,9 +482,9 @@ export class Authenticator {
}
}
}

if (
existingSession.error instanceof SessionExpiredError ||
existingSession.error instanceof SessionConcurrencyLimitError ||
existingSession.error instanceof SessionUnexpectedError
) {
const options = requestIsRedirectable
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export {
SessionMissingError,
SessionExpiredError,
SessionUnexpectedError,
SessionConcurrencyLimitError,
} from './session_errors';
export type { SessionManagementServiceStart } from './session_management_service';
export { SessionManagementService } from './session_management_service';
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,12 @@ import { ConfigSchema, createConfig } from '../config';
import { sessionCookieMock, sessionIndexMock, sessionMock } from './index.mock';
import { getPrintableSessionId, Session, type SessionValueContentToEncrypt } from './session';
import type { SessionCookie } from './session_cookie';
import { SessionExpiredError, SessionMissingError, SessionUnexpectedError } from './session_errors';
import {
SessionConcurrencyLimitError,
SessionExpiredError,
SessionMissingError,
SessionUnexpectedError,
} from './session_errors';
import type { SessionIndex } from './session_index';

describe('Session', () => {
Expand Down Expand Up @@ -233,7 +238,7 @@ describe('Session', () => {
mockSessionIndex.isWithinConcurrentSessionLimit.mockResolvedValue(false);

await expect(session.get(httpServerMock.createKibanaRequest())).resolves.toEqual({
error: expect.any(SessionUnexpectedError),
error: expect.any(SessionConcurrencyLimitError),
value: null,
});
expect(mockSessionCookie.clear).toHaveBeenCalledTimes(1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,12 @@ import type { AuthenticationProvider } from '../../common';
import { userSessionConcurrentLimitLogoutEvent } from '../audit';
import type { ConfigType } from '../config';
import type { SessionCookie } from './session_cookie';
import { SessionExpiredError, SessionMissingError, SessionUnexpectedError } from './session_errors';
import {
SessionConcurrencyLimitError,
SessionExpiredError,
SessionMissingError,
SessionUnexpectedError,
} from './session_errors';
import type { SessionIndex, SessionIndexValue } from './session_index';

/**
Expand Down Expand Up @@ -214,7 +219,7 @@ export class Session {
'Session is outside the concurrent session limit and will be invalidated.'
);
await this.invalidate(request, { match: 'current' });
return { error: new SessionUnexpectedError(), value: null };
return { error: new SessionConcurrencyLimitError(), value: null };
}

return {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,5 @@
export { SessionError } from './session_error';
export { SessionMissingError } from './session_missing_error';
export { SessionExpiredError } from './session_expired_error';
export { SessionConcurrencyLimitError } from './session_concurrency_limit_error';
export { SessionUnexpectedError } from './session_unexpected_error';
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { SessionError, SessionErrorReason } from './session_error';

export class SessionConcurrencyLimitError extends SessionError {
constructor() {
super(SessionErrorReason.CONCURRENCY_LIMIT, SessionErrorReason.CONCURRENCY_LIMIT);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
export enum SessionErrorReason {
'SESSION_MISSING' = 'SESSION_MISSING',
'SESSION_EXPIRED' = 'SESSION_EXPIRED',
'CONCURRENCY_LIMIT' = 'CONCURRENCY_LIMIT',
'UNEXPECTED_SESSION_ERROR' = 'UNEXPECTED_SESSION_ERROR',
}

Expand Down