From 4d7c7b55f7df813b69b3a75d78f1bf1234ddf384 Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Wed, 5 Feb 2020 10:29:21 +0100 Subject: [PATCH] Properly handle password change for users authenticated with provider other than `basic`. (#55206) --- .../server/alerts_client_factory.test.ts | 2 +- .../plugins/reporting/server/lib/get_user.ts | 19 +--- .../routes/lib/authorized_user_pre_routing.ts | 2 +- x-pack/legacy/plugins/security/index.js | 2 +- .../common/model/authenticated_user.mock.ts | 1 + .../common/model/authenticated_user.ts | 5 + .../account_management_page.test.tsx | 6 +- .../authentication/authenticator.test.ts | 71 +----------- .../server/authentication/authenticator.ts | 23 ++-- .../server/authentication/index.test.ts | 75 ++++++------- .../security/server/authentication/index.ts | 20 +--- .../server/authentication/providers/base.ts | 17 ++- .../server/authentication/providers/basic.ts | 5 + .../authentication/providers/kerberos.test.ts | 10 +- .../authentication/providers/kerberos.ts | 5 + .../authentication/providers/oidc.test.ts | 6 +- .../server/authentication/providers/oidc.ts | 7 +- .../authentication/providers/pki.test.ts | 12 +- .../server/authentication/providers/pki.ts | 5 + .../authentication/providers/saml.test.ts | 6 +- .../server/authentication/providers/saml.ts | 7 +- .../authentication/providers/token.test.ts | 10 +- .../server/authentication/providers/token.ts | 7 +- .../routes/authentication/common.test.ts | 13 +-- .../server/routes/authentication/common.ts | 8 +- .../routes/users/change_password.test.ts | 106 +++++++++++++++--- .../server/routes/users/change_password.ts | 77 +++++++------ .../apis/security/basic_login.js | 2 + .../apis/security/kerberos_login.ts | 2 + .../apis/authorization_code_flow/oidc_auth.js | 2 + .../apis/implicit_flow/oidc_auth.ts | 1 + .../apis/security/pki_auth.ts | 3 + .../apis/security/saml_login.ts | 2 + 33 files changed, 284 insertions(+), 255 deletions(-) diff --git a/x-pack/legacy/plugins/alerting/server/alerts_client_factory.test.ts b/x-pack/legacy/plugins/alerting/server/alerts_client_factory.test.ts index 754e02a3f1e5e..14c685237bf92 100644 --- a/x-pack/legacy/plugins/alerting/server/alerts_client_factory.test.ts +++ b/x-pack/legacy/plugins/alerting/server/alerts_client_factory.test.ts @@ -86,7 +86,7 @@ test('getUserName() returns a name when security is enabled', async () => { factory.create(KibanaRequest.from(fakeRequest), fakeRequest); const constructorCall = jest.requireMock('./alerts_client').AlertsClient.mock.calls[0][0]; - securityPluginSetup.authc.getCurrentUser.mockResolvedValueOnce({ username: 'bob' }); + securityPluginSetup.authc.getCurrentUser.mockReturnValueOnce({ username: 'bob' }); const userNameResult = await constructorCall.getUserName(); expect(userNameResult).toEqual('bob'); }); diff --git a/x-pack/legacy/plugins/reporting/server/lib/get_user.ts b/x-pack/legacy/plugins/reporting/server/lib/get_user.ts index 350004ddb78f8..ab02dfe0743f0 100644 --- a/x-pack/legacy/plugins/reporting/server/lib/get_user.ts +++ b/x-pack/legacy/plugins/reporting/server/lib/get_user.ts @@ -6,27 +6,14 @@ import { Legacy } from 'kibana'; import { KibanaRequest } from '../../../../../../src/core/server'; -import { Logger, ServerFacade } from '../../types'; +import { ServerFacade } from '../../types'; import { ReportingSetupDeps } from '../plugin'; -export function getUserFactory( - server: ServerFacade, - security: ReportingSetupDeps['security'], - logger: Logger -) { +export function getUserFactory(server: ServerFacade, security: ReportingSetupDeps['security']) { /* * Legacy.Request because this is called from routing middleware */ return async (request: Legacy.Request) => { - if (!security) { - return null; - } - - try { - return await security.authc.getCurrentUser(KibanaRequest.from(request)); - } catch (err) { - logger.error(err, ['getUser']); - return null; - } + return security?.authc.getCurrentUser(KibanaRequest.from(request)) ?? null; }; } diff --git a/x-pack/legacy/plugins/reporting/server/routes/lib/authorized_user_pre_routing.ts b/x-pack/legacy/plugins/reporting/server/routes/lib/authorized_user_pre_routing.ts index 874027251570c..57c3fcee222da 100644 --- a/x-pack/legacy/plugins/reporting/server/routes/lib/authorized_user_pre_routing.ts +++ b/x-pack/legacy/plugins/reporting/server/routes/lib/authorized_user_pre_routing.ts @@ -22,7 +22,7 @@ export const authorizedUserPreRoutingFactory = function authorizedUserPreRouting plugins: ReportingSetupDeps, logger: Logger ) { - const getUser = getUserFactory(server, plugins.security, logger); + const getUser = getUserFactory(server, plugins.security); const config = server.config(); return async function authorizedUserPreRouting(request: Legacy.Request) { diff --git a/x-pack/legacy/plugins/security/index.js b/x-pack/legacy/plugins/security/index.js index 4988c30a1398b..9016398463b5f 100644 --- a/x-pack/legacy/plugins/security/index.js +++ b/x-pack/legacy/plugins/security/index.js @@ -130,7 +130,7 @@ export const security = kibana => ); server.expose({ - getUser: request => securityPlugin.authc.getCurrentUser(KibanaRequest.from(request)), + getUser: async request => securityPlugin.authc.getCurrentUser(KibanaRequest.from(request)), }); initLoginView(securityPlugin, server); diff --git a/x-pack/plugins/security/common/model/authenticated_user.mock.ts b/x-pack/plugins/security/common/model/authenticated_user.mock.ts index 3a93efc57b5f6..220b284e76591 100644 --- a/x-pack/plugins/security/common/model/authenticated_user.mock.ts +++ b/x-pack/plugins/security/common/model/authenticated_user.mock.ts @@ -15,6 +15,7 @@ export function mockAuthenticatedUser(user: Partial = {}) { enabled: true, authentication_realm: { name: 'native1', type: 'native' }, lookup_realm: { name: 'native1', type: 'native' }, + authentication_provider: 'basic', ...user, }; } diff --git a/x-pack/plugins/security/common/model/authenticated_user.ts b/x-pack/plugins/security/common/model/authenticated_user.ts index 1b660ae919c09..6465b4a23eb48 100644 --- a/x-pack/plugins/security/common/model/authenticated_user.ts +++ b/x-pack/plugins/security/common/model/authenticated_user.ts @@ -26,6 +26,11 @@ export interface AuthenticatedUser extends User { * The name and type of the Realm where the user information were retrieved from. */ lookup_realm: UserRealm; + + /** + * Name of the Kibana authentication provider that used to authenticate user. + */ + authentication_provider: string; } export function canUserChangePassword(user: AuthenticatedUser) { diff --git a/x-pack/plugins/security/public/account_management/account_management_page.test.tsx b/x-pack/plugins/security/public/account_management/account_management_page.test.tsx index b7cf8e6dd1418..4caf3d25e887b 100644 --- a/x-pack/plugins/security/public/account_management/account_management_page.test.tsx +++ b/x-pack/plugins/security/public/account_management/account_management_page.test.tsx @@ -10,6 +10,7 @@ import { AuthenticatedUser } from '../../common/model'; import { AccountManagementPage } from './account_management_page'; import { coreMock } from 'src/core/public/mocks'; +import { mockAuthenticatedUser } from '../../common/model/authenticated_user.mock'; import { securityMock } from '../mocks'; import { userAPIClientMock } from '../management/users/index.mock'; @@ -19,11 +20,10 @@ interface Options { realm?: string; } const createUser = ({ withFullName = true, withEmail = true, realm = 'native' }: Options = {}) => { - return { + return mockAuthenticatedUser({ full_name: withFullName ? 'Casey Smith' : '', username: 'csmith', email: withEmail ? 'csmith@domain.com' : '', - enabled: true, roles: [], authentication_realm: { type: realm, @@ -33,7 +33,7 @@ const createUser = ({ withFullName = true, withEmail = true, realm = 'native' }: type: realm, name: realm, }, - }; + }); }; function getSecuritySetupMock({ currentUser }: { currentUser: AuthenticatedUser }) { diff --git a/x-pack/plugins/security/server/authentication/authenticator.test.ts b/x-pack/plugins/security/server/authentication/authenticator.test.ts index dd580c890bf94..8be1762133db6 100644 --- a/x-pack/plugins/security/server/authentication/authenticator.test.ts +++ b/x-pack/plugins/security/server/authentication/authenticator.test.ts @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -jest.mock('./providers/basic', () => ({ BasicAuthenticationProvider: jest.fn() })); +jest.mock('./providers/basic'); import Boom from 'boom'; import { duration, Duration } from 'moment'; @@ -225,75 +225,6 @@ describe('Authenticator', () => { expect(mockSessionStorage.set).not.toHaveBeenCalled(); expect(mockSessionStorage.clear).toHaveBeenCalled(); }); - - describe('stateless login', () => { - it('does not create session even if authentication provider returns state', async () => { - const user = mockAuthenticatedUser(); - const request = httpServerMock.createKibanaRequest(); - const authorization = `Basic ${Buffer.from('foo:bar').toString('base64')}`; - - mockBasicAuthenticationProvider.login.mockResolvedValue( - AuthenticationResult.succeeded(user, { state: { authorization } }) - ); - - const authenticationResult = await authenticator.login(request, { - provider: 'basic', - value: {}, - stateless: true, - }); - expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toEqual(user); - - expect(mockBasicAuthenticationProvider.login).toHaveBeenCalledWith(request, {}, null); - expect(mockSessionStorage.get).not.toHaveBeenCalled(); - expect(mockSessionStorage.set).not.toHaveBeenCalled(); - expect(mockSessionStorage.clear).not.toHaveBeenCalled(); - }); - - it('does not clear session even if provider asked to do so.', async () => { - const user = mockAuthenticatedUser(); - const request = httpServerMock.createKibanaRequest(); - - mockBasicAuthenticationProvider.login.mockResolvedValue( - AuthenticationResult.succeeded(user, { state: null }) - ); - - const authenticationResult = await authenticator.login(request, { - provider: 'basic', - value: {}, - stateless: true, - }); - expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toEqual(user); - - expect(mockBasicAuthenticationProvider.login).toHaveBeenCalledWith(request, {}, null); - expect(mockSessionStorage.get).not.toHaveBeenCalled(); - expect(mockSessionStorage.set).not.toHaveBeenCalled(); - expect(mockSessionStorage.clear).not.toHaveBeenCalled(); - }); - - it('does not clear session even if provider failed with 401.', async () => { - const request = httpServerMock.createKibanaRequest(); - - const failureReason = Boom.unauthorized(); - mockBasicAuthenticationProvider.login.mockResolvedValue( - AuthenticationResult.failed(failureReason) - ); - - const authenticationResult = await authenticator.login(request, { - provider: 'basic', - value: {}, - stateless: true, - }); - expect(authenticationResult.failed()).toBe(true); - expect(authenticationResult.error).toBe(failureReason); - - expect(mockBasicAuthenticationProvider.login).toHaveBeenCalledWith(request, {}, null); - expect(mockSessionStorage.get).not.toHaveBeenCalled(); - expect(mockSessionStorage.set).not.toHaveBeenCalled(); - expect(mockSessionStorage.clear).not.toHaveBeenCalled(); - }); - }); }); describe('`authenticate` method', () => { diff --git a/x-pack/plugins/security/server/authentication/authenticator.ts b/x-pack/plugins/security/server/authentication/authenticator.ts index be952a154cee4..ea7792e902ec1 100644 --- a/x-pack/plugins/security/server/authentication/authenticator.ts +++ b/x-pack/plugins/security/server/authentication/authenticator.ts @@ -80,13 +80,6 @@ export interface ProviderLoginAttempt { * Login attempt can have any form and defined by the specific provider. */ value: unknown; - - /** - * Indicates whether login attempt should be performed in a "stateless" manner. If `true` provider - * performing login will neither be able to retrieve or update existing state if any nor persist - * any new state it may produce as a result of the login attempt. It's `false` by default. - */ - stateless?: boolean; } export interface AuthenticatorOptions { @@ -107,12 +100,12 @@ const providerMap = new Map< providerSpecificOptions?: AuthenticationProviderSpecificOptions ) => BaseAuthenticationProvider >([ - ['basic', BasicAuthenticationProvider], - ['kerberos', KerberosAuthenticationProvider], - ['saml', SAMLAuthenticationProvider], - ['token', TokenAuthenticationProvider], - ['oidc', OIDCAuthenticationProvider], - ['pki', PKIAuthenticationProvider], + [BasicAuthenticationProvider.type, BasicAuthenticationProvider], + [KerberosAuthenticationProvider.type, KerberosAuthenticationProvider], + [SAMLAuthenticationProvider.type, SAMLAuthenticationProvider], + [TokenAuthenticationProvider.type, TokenAuthenticationProvider], + [OIDCAuthenticationProvider.type, OIDCAuthenticationProvider], + [PKIAuthenticationProvider.type, PKIAuthenticationProvider], ]); function assertRequest(request: KibanaRequest) { @@ -254,7 +247,7 @@ export class Authenticator { // If we detect an existing session that belongs to a different provider than the one requested // to perform a login we should clear such session. - let existingSession = attempt.stateless ? null : await this.getSessionValue(sessionStorage); + let existingSession = await this.getSessionValue(sessionStorage); if (existingSession && existingSession.provider !== attempt.provider) { this.logger.debug( `Clearing existing session of another ("${existingSession.provider}") provider.` @@ -281,7 +274,7 @@ export class Authenticator { (authenticationResult.failed() && getErrorStatusCode(authenticationResult.error) === 401); if (existingSession && shouldClearSession) { sessionStorage.clear(); - } else if (!attempt.stateless && authenticationResult.shouldUpdateState()) { + } else if (authenticationResult.shouldUpdateState()) { const { idleTimeoutExpiration, lifespanExpiration } = this.calculateExpiry(existingSession); sessionStorage.set({ state: authenticationResult.state, diff --git a/x-pack/plugins/security/server/authentication/index.test.ts b/x-pack/plugins/security/server/authentication/index.test.ts index 0c1095b56e6e2..d0de6d571b7a0 100644 --- a/x-pack/plugins/security/server/authentication/index.test.ts +++ b/x-pack/plugins/security/server/authentication/index.test.ts @@ -10,7 +10,6 @@ jest.mock('./api_keys'); jest.mock('./authenticator'); import Boom from 'boom'; -import { errors } from 'elasticsearch'; import { first } from 'rxjs/operators'; import { @@ -27,7 +26,6 @@ import { AuthToolkit, IClusterClient, CoreSetup, - ElasticsearchErrorHelpers, KibanaRequest, LoggerFactory, ScopedClusterClient, @@ -289,67 +287,66 @@ describe('setupAuthentication()', () => { }); describe('getCurrentUser()', () => { - let getCurrentUser: (r: KibanaRequest) => Promise; + let getCurrentUser: (r: KibanaRequest) => AuthenticatedUser | null; beforeEach(async () => { getCurrentUser = (await setupAuthentication(mockSetupAuthenticationParams)).getCurrentUser; }); - it('returns `null` if Security is disabled', async () => { + it('returns `null` if Security is disabled', () => { mockSetupAuthenticationParams.license.isEnabled.mockReturnValue(false); - await expect(getCurrentUser(httpServerMock.createKibanaRequest())).resolves.toBe(null); + expect(getCurrentUser(httpServerMock.createKibanaRequest())).toBe(null); }); - it('fails if `authenticate` call fails', async () => { - const failureReason = new Error('Something went wrong'); - mockScopedClusterClient.callAsCurrentUser.mockRejectedValue(failureReason); + it('returns user from the auth state.', () => { + const mockUser = mockAuthenticatedUser(); - await expect(getCurrentUser(httpServerMock.createKibanaRequest())).rejects.toBe( - failureReason - ); + const mockAuthGet = mockSetupAuthenticationParams.http.auth.get as jest.Mock; + mockAuthGet.mockReturnValue({ state: mockUser }); + + const mockRequest = httpServerMock.createKibanaRequest(); + expect(getCurrentUser(mockRequest)).toBe(mockUser); + expect(mockAuthGet).toHaveBeenCalledTimes(1); + expect(mockAuthGet).toHaveBeenCalledWith(mockRequest); }); - it('returns result of `authenticate` call.', async () => { - const mockUser = mockAuthenticatedUser(); - mockScopedClusterClient.callAsCurrentUser.mockResolvedValue(mockUser); + it('returns null if auth state is not available.', () => { + const mockAuthGet = mockSetupAuthenticationParams.http.auth.get as jest.Mock; + mockAuthGet.mockReturnValue({}); - await expect(getCurrentUser(httpServerMock.createKibanaRequest())).resolves.toBe(mockUser); + const mockRequest = httpServerMock.createKibanaRequest(); + expect(getCurrentUser(mockRequest)).toBeNull(); + expect(mockAuthGet).toHaveBeenCalledTimes(1); + expect(mockAuthGet).toHaveBeenCalledWith(mockRequest); }); }); describe('isAuthenticated()', () => { - let isAuthenticated: (r: KibanaRequest) => Promise; + let isAuthenticated: (r: KibanaRequest) => boolean; beforeEach(async () => { isAuthenticated = (await setupAuthentication(mockSetupAuthenticationParams)).isAuthenticated; }); - it('returns `true` if Security is disabled', async () => { - mockSetupAuthenticationParams.license.isEnabled.mockReturnValue(false); - - await expect(isAuthenticated(httpServerMock.createKibanaRequest())).resolves.toBe(true); - }); + it('returns `true` if request is authenticated', () => { + const mockIsAuthenticated = mockSetupAuthenticationParams.http.auth + .isAuthenticated as jest.Mock; + mockIsAuthenticated.mockReturnValue(true); - it('returns `true` if `authenticate` succeeds.', async () => { - const mockUser = mockAuthenticatedUser(); - mockScopedClusterClient.callAsCurrentUser.mockResolvedValue(mockUser); - - await expect(isAuthenticated(httpServerMock.createKibanaRequest())).resolves.toBe(true); - }); - - it('returns `false` if `authenticate` fails with 401.', async () => { - const failureReason = ElasticsearchErrorHelpers.decorateNotAuthorizedError(new Error()); - mockScopedClusterClient.callAsCurrentUser.mockRejectedValue(failureReason); - - await expect(isAuthenticated(httpServerMock.createKibanaRequest())).resolves.toBe(false); + const mockRequest = httpServerMock.createKibanaRequest(); + expect(isAuthenticated(mockRequest)).toBe(true); + expect(mockIsAuthenticated).toHaveBeenCalledTimes(1); + expect(mockIsAuthenticated).toHaveBeenCalledWith(mockRequest); }); - it('fails if `authenticate` call fails with unknown reason', async () => { - const failureReason = new errors.BadRequest(); - mockScopedClusterClient.callAsCurrentUser.mockRejectedValue(failureReason); + it('returns `false` if request is not authenticated', () => { + const mockIsAuthenticated = mockSetupAuthenticationParams.http.auth + .isAuthenticated as jest.Mock; + mockIsAuthenticated.mockReturnValue(false); - await expect(isAuthenticated(httpServerMock.createKibanaRequest())).rejects.toBe( - failureReason - ); + const mockRequest = httpServerMock.createKibanaRequest(); + expect(isAuthenticated(mockRequest)).toBe(false); + expect(mockIsAuthenticated).toHaveBeenCalledTimes(1); + expect(mockIsAuthenticated).toHaveBeenCalledWith(mockRequest); }); }); diff --git a/x-pack/plugins/security/server/authentication/index.ts b/x-pack/plugins/security/server/authentication/index.ts index 1002ad7709b80..4b73430ff13c4 100644 --- a/x-pack/plugins/security/server/authentication/index.ts +++ b/x-pack/plugins/security/server/authentication/index.ts @@ -55,14 +55,12 @@ export async function setupAuthentication({ * Retrieves currently authenticated user associated with the specified request. * @param request */ - const getCurrentUser = async (request: KibanaRequest) => { + const getCurrentUser = (request: KibanaRequest) => { if (!license.isEnabled()) { return null; } - return (await clusterClient - .asScoped(request) - .callAsCurrentUser('shield.authenticate')) as AuthenticatedUser; + return (http.auth.get(request).state ?? null) as AuthenticatedUser | null; }; const isValid = (sessionValue: ProviderSession) => { @@ -180,18 +178,6 @@ export async function setupAuthentication({ apiKeys.create(request, params), invalidateAPIKey: (request: KibanaRequest, params: InvalidateAPIKeyParams) => apiKeys.invalidate(request, params), - isAuthenticated: async (request: KibanaRequest) => { - try { - await getCurrentUser(request); - } catch (err) { - // Don't swallow server errors. - if (getErrorStatusCode(err) !== 401) { - throw err; - } - return false; - } - - return true; - }, + isAuthenticated: (request: KibanaRequest) => http.auth.isAuthenticated(request), }; } diff --git a/x-pack/plugins/security/server/authentication/providers/base.ts b/x-pack/plugins/security/server/authentication/providers/base.ts index 12253f8d8f548..a40732768810d 100644 --- a/x-pack/plugins/security/server/authentication/providers/base.ts +++ b/x-pack/plugins/security/server/authentication/providers/base.ts @@ -11,6 +11,7 @@ import { IClusterClient, Headers, } from '../../../../../../src/core/server'; +import { deepFreeze } from '../../../../../../src/core/utils'; import { AuthenticatedUser } from '../../../common/model'; import { AuthenticationResult } from '../authentication_result'; import { DeauthenticationResult } from '../deauthentication_result'; @@ -35,6 +36,11 @@ export type AuthenticationProviderSpecificOptions = Record; * Base class that all authentication providers should extend. */ export abstract class BaseAuthenticationProvider { + /** + * Type of the provider. + */ + static readonly type: string; + /** * Logger instance bound to a specific provider context. */ @@ -85,8 +91,13 @@ export abstract class BaseAuthenticationProvider { * @param [authHeaders] Optional `Headers` dictionary to send with the request. */ protected async getUser(request: KibanaRequest, authHeaders: Headers = {}) { - return (await this.options.client - .asScoped({ headers: { ...request.headers, ...authHeaders } }) - .callAsCurrentUser('shield.authenticate')) as AuthenticatedUser; + return deepFreeze({ + ...(await this.options.client + .asScoped({ headers: { ...request.headers, ...authHeaders } }) + .callAsCurrentUser('shield.authenticate')), + // We use `this.constructor` trick to get access to the static `type` field of the specific + // `BaseAuthenticationProvider` subclass. + authentication_provider: (this.constructor as any).type, + } as AuthenticatedUser); } } diff --git a/x-pack/plugins/security/server/authentication/providers/basic.ts b/x-pack/plugins/security/server/authentication/providers/basic.ts index 07d141b4e1b2b..a8e4e8705a7a8 100644 --- a/x-pack/plugins/security/server/authentication/providers/basic.ts +++ b/x-pack/plugins/security/server/authentication/providers/basic.ts @@ -34,6 +34,11 @@ interface ProviderState { * Provider that supports request authentication via Basic HTTP Authentication. */ export class BasicAuthenticationProvider extends BaseAuthenticationProvider { + /** + * Type of the provider. + */ + static readonly type = 'basic'; + /** * Performs initial login request using username and password. * @param request Request instance. diff --git a/x-pack/plugins/security/server/authentication/providers/kerberos.test.ts b/x-pack/plugins/security/server/authentication/providers/kerberos.test.ts index e609afb6ae3f3..e4b4df3feeae2 100644 --- a/x-pack/plugins/security/server/authentication/providers/kerberos.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/kerberos.test.ts @@ -155,7 +155,7 @@ describe('KerberosAuthenticationProvider', () => { expect(request.headers.authorization).toBe('negotiate spnego'); expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'kerberos' }); expect(authenticationResult.authHeaders).toEqual({ authorization: 'Bearer some-token' }); expect(authenticationResult.authResponseHeaders).toBeUndefined(); expect(authenticationResult.state).toEqual({ @@ -193,7 +193,7 @@ describe('KerberosAuthenticationProvider', () => { expect(request.headers.authorization).toBe('negotiate spnego'); expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'kerberos' }); expect(authenticationResult.authHeaders).toEqual({ authorization: 'Bearer some-token' }); expect(authenticationResult.authResponseHeaders).toEqual({ 'WWW-Authenticate': 'Negotiate response-token', @@ -337,7 +337,7 @@ describe('KerberosAuthenticationProvider', () => { expect(request.headers).not.toHaveProperty('authorization'); expect(authenticationResult.succeeded()).toBe(true); expect(authenticationResult.authHeaders).toEqual({ authorization }); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'kerberos' }); expect(authenticationResult.state).toBeUndefined(); }); @@ -370,7 +370,7 @@ describe('KerberosAuthenticationProvider', () => { expect(authenticationResult.succeeded()).toBe(true); expect(authenticationResult.authHeaders).toEqual({ authorization: 'Bearer newfoo' }); - expect(authenticationResult.user).toEqual(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'kerberos' }); expect(authenticationResult.state).toEqual({ accessToken: 'newfoo', refreshToken: 'newbar' }); expect(request.headers).not.toHaveProperty('authorization'); }); @@ -437,7 +437,7 @@ describe('KerberosAuthenticationProvider', () => { expect(request.headers.authorization).toBe('Bearer some-valid-token'); expect(authenticationResult.succeeded()).toBe(true); expect(authenticationResult.authHeaders).toBeUndefined(); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'kerberos' }); expect(authenticationResult.state).toBeUndefined(); }); diff --git a/x-pack/plugins/security/server/authentication/providers/kerberos.ts b/x-pack/plugins/security/server/authentication/providers/kerberos.ts index 767eab7b4311d..b8e3b7bc23790 100644 --- a/x-pack/plugins/security/server/authentication/providers/kerberos.ts +++ b/x-pack/plugins/security/server/authentication/providers/kerberos.ts @@ -43,6 +43,11 @@ const WWWAuthenticateHeaderName = 'WWW-Authenticate'; * Provider that supports Kerberos request authentication. */ export class KerberosAuthenticationProvider extends BaseAuthenticationProvider { + /** + * Type of the provider. + */ + static readonly type = 'kerberos'; + /** * Performs Kerberos request authentication. * @param request Request instance. diff --git a/x-pack/plugins/security/server/authentication/providers/oidc.test.ts b/x-pack/plugins/security/server/authentication/providers/oidc.test.ts index dd4f6820f89f3..dae3774955859 100644 --- a/x-pack/plugins/security/server/authentication/providers/oidc.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/oidc.test.ts @@ -311,7 +311,7 @@ describe('OIDCAuthenticationProvider', () => { expect(request.headers).not.toHaveProperty('authorization'); expect(authenticationResult.succeeded()).toBe(true); expect(authenticationResult.authHeaders).toEqual({ authorization }); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'oidc' }); expect(authenticationResult.state).toBeUndefined(); }); @@ -380,7 +380,7 @@ describe('OIDCAuthenticationProvider', () => { expect(authenticationResult.authHeaders).toEqual({ authorization: 'Bearer new-access-token', }); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'oidc' }); expect(authenticationResult.state).toEqual({ accessToken: 'new-access-token', refreshToken: 'new-refresh-token', @@ -492,7 +492,7 @@ describe('OIDCAuthenticationProvider', () => { expect(request.headers.authorization).toBe('Bearer some-valid-token'); expect(authenticationResult.succeeded()).toBe(true); expect(authenticationResult.authHeaders).toBeUndefined(); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'oidc' }); expect(authenticationResult.state).toBeUndefined(); }); diff --git a/x-pack/plugins/security/server/authentication/providers/oidc.ts b/x-pack/plugins/security/server/authentication/providers/oidc.ts index 3737123645379..f13a2ec05231a 100644 --- a/x-pack/plugins/security/server/authentication/providers/oidc.ts +++ b/x-pack/plugins/security/server/authentication/providers/oidc.ts @@ -6,9 +6,9 @@ import Boom from 'boom'; import type from 'type-detect'; -import { canRedirectRequest } from '../'; import { KibanaRequest } from '../../../../../../src/core/server'; import { AuthenticationResult } from '../authentication_result'; +import { canRedirectRequest } from '../can_redirect_request'; import { DeauthenticationResult } from '../deauthentication_result'; import { Tokens, TokenPair } from '../tokens'; import { @@ -62,6 +62,11 @@ interface ProviderState extends Partial { * Provider that supports authentication using an OpenID Connect realm in Elasticsearch. */ export class OIDCAuthenticationProvider extends BaseAuthenticationProvider { + /** + * Type of the provider. + */ + static readonly type = 'oidc'; + /** * Specifies Elasticsearch OIDC realm name that Kibana should use. */ diff --git a/x-pack/plugins/security/server/authentication/providers/pki.test.ts b/x-pack/plugins/security/server/authentication/providers/pki.test.ts index 76442733e7368..a2dda88c4680c 100644 --- a/x-pack/plugins/security/server/authentication/providers/pki.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/pki.test.ts @@ -200,7 +200,7 @@ describe('PKIAuthenticationProvider', () => { expect(request.headers).not.toHaveProperty('authorization'); expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'pki' }); expect(authenticationResult.authHeaders).toEqual({ authorization: 'Bearer access-token' }); expect(authenticationResult.authResponseHeaders).toBeUndefined(); expect(authenticationResult.state).toEqual({ @@ -242,7 +242,7 @@ describe('PKIAuthenticationProvider', () => { expect(request.headers).not.toHaveProperty('authorization'); expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'pki' }); expect(authenticationResult.authHeaders).toEqual({ authorization: 'Bearer access-token' }); expect(authenticationResult.authResponseHeaders).toBeUndefined(); expect(authenticationResult.state).toEqual({ @@ -287,7 +287,7 @@ describe('PKIAuthenticationProvider', () => { expect(request.headers).not.toHaveProperty('authorization'); expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'pki' }); expect(authenticationResult.authHeaders).toEqual({ authorization: 'Bearer access-token' }); expect(authenticationResult.authResponseHeaders).toBeUndefined(); expect(authenticationResult.state).toEqual({ @@ -331,7 +331,7 @@ describe('PKIAuthenticationProvider', () => { expect(request.headers).not.toHaveProperty('authorization'); expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'pki' }); expect(authenticationResult.authHeaders).toEqual({ authorization: 'Bearer access-token' }); expect(authenticationResult.authResponseHeaders).toBeUndefined(); expect(authenticationResult.state).toEqual({ @@ -448,7 +448,7 @@ describe('PKIAuthenticationProvider', () => { expect(authenticationResult.authHeaders).toEqual({ authorization: `Bearer ${state.accessToken}`, }); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'pki' }); expect(authenticationResult.state).toBeUndefined(); }); @@ -491,7 +491,7 @@ describe('PKIAuthenticationProvider', () => { expect(request.headers.authorization).toBe('Bearer some-valid-token'); expect(authenticationResult.succeeded()).toBe(true); expect(authenticationResult.authHeaders).toBeUndefined(); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'pki' }); expect(authenticationResult.state).toBeUndefined(); }); diff --git a/x-pack/plugins/security/server/authentication/providers/pki.ts b/x-pack/plugins/security/server/authentication/providers/pki.ts index c7d431422a248..6d5aa9f01f2ea 100644 --- a/x-pack/plugins/security/server/authentication/providers/pki.ts +++ b/x-pack/plugins/security/server/authentication/providers/pki.ts @@ -44,6 +44,11 @@ function getRequestAuthenticationScheme(request: KibanaRequest) { * Provider that supports PKI request authentication. */ export class PKIAuthenticationProvider extends BaseAuthenticationProvider { + /** + * Type of the provider. + */ + static readonly type = 'pki'; + /** * Performs PKI request authentication. * @param request Request instance. diff --git a/x-pack/plugins/security/server/authentication/providers/saml.test.ts b/x-pack/plugins/security/server/authentication/providers/saml.test.ts index a5d1010a1bec8..c4fdf0b25061b 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.test.ts @@ -635,7 +635,7 @@ describe('SAMLAuthenticationProvider', () => { expect(request.headers).not.toHaveProperty('authorization'); expect(authenticationResult.succeeded()).toBe(true); expect(authenticationResult.authHeaders).toEqual({ authorization }); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'saml' }); expect(authenticationResult.state).toBeUndefined(); }); @@ -696,7 +696,7 @@ describe('SAMLAuthenticationProvider', () => { expect(authenticationResult.authHeaders).toEqual({ authorization: 'Bearer new-access-token', }); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'saml' }); expect(authenticationResult.state).toEqual({ username: 'user', accessToken: 'new-access-token', @@ -842,7 +842,7 @@ describe('SAMLAuthenticationProvider', () => { expect(request.headers.authorization).toBe('Bearer some-valid-token'); expect(authenticationResult.succeeded()).toBe(true); expect(authenticationResult.authHeaders).toBeUndefined(); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'saml' }); expect(authenticationResult.state).toBeUndefined(); }); diff --git a/x-pack/plugins/security/server/authentication/providers/saml.ts b/x-pack/plugins/security/server/authentication/providers/saml.ts index faa19239fcc3b..a817159fcd445 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.ts @@ -10,8 +10,8 @@ import { KibanaRequest } from '../../../../../../src/core/server'; import { AuthenticationResult } from '../authentication_result'; import { DeauthenticationResult } from '../deauthentication_result'; import { AuthenticationProviderOptions, BaseAuthenticationProvider } from './base'; +import { canRedirectRequest } from '../can_redirect_request'; import { Tokens, TokenPair } from '../tokens'; -import { canRedirectRequest } from '..'; /** * The state supported by the provider (for the SAML handshake or established session). @@ -66,6 +66,11 @@ export function isSAMLRequestQuery(query: any): query is { SAMLRequest: string } * Provider that supports SAML request authentication. */ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { + /** + * Type of the provider. + */ + static readonly type = 'saml'; + /** * Specifies Elasticsearch SAML realm name that Kibana should use. */ diff --git a/x-pack/plugins/security/server/authentication/providers/token.test.ts b/x-pack/plugins/security/server/authentication/providers/token.test.ts index 3d377140cb42e..0a55219e25d91 100644 --- a/x-pack/plugins/security/server/authentication/providers/token.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/token.test.ts @@ -48,7 +48,7 @@ describe('TokenAuthenticationProvider', () => { const authenticationResult = await provider.login(request, credentials); expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toEqual(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'token' }); expect(authenticationResult.state).toEqual(tokenPair); expect(authenticationResult.authHeaders).toEqual({ authorization }); }); @@ -140,7 +140,7 @@ describe('TokenAuthenticationProvider', () => { const authenticationResult = await provider.authenticate(request); expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toEqual(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'token' }); expect(authenticationResult.authHeaders).toBeUndefined(); expect(authenticationResult.state).toBeUndefined(); }); @@ -158,7 +158,7 @@ describe('TokenAuthenticationProvider', () => { const authenticationResult = await provider.authenticate(request, tokenPair); expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toEqual(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'token' }); expect(authenticationResult.state).toBeUndefined(); expect(authenticationResult.authHeaders).toEqual({ authorization }); expect(request.headers).not.toHaveProperty('authorization'); @@ -192,7 +192,7 @@ describe('TokenAuthenticationProvider', () => { sinon.assert.calledOnce(mockOptions.tokens.refresh); expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toEqual(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'token' }); expect(authenticationResult.state).toEqual({ accessToken: 'newfoo', refreshToken: 'newbar' }); expect(authenticationResult.authHeaders).toEqual({ authorization: 'Bearer newfoo' }); expect(request.headers).not.toHaveProperty('authorization'); @@ -231,7 +231,7 @@ describe('TokenAuthenticationProvider', () => { const authenticationResult = await provider.authenticate(request, tokenPair); expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toEqual(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'token' }); expect(authenticationResult.state).toBeUndefined(); expect(authenticationResult.authHeaders).toBeUndefined(); expect(request.headers.authorization).toEqual('Bearer foo-from-header'); diff --git a/x-pack/plugins/security/server/authentication/providers/token.ts b/x-pack/plugins/security/server/authentication/providers/token.ts index c5f8f07e50b11..03fd003e2cbde 100644 --- a/x-pack/plugins/security/server/authentication/providers/token.ts +++ b/x-pack/plugins/security/server/authentication/providers/token.ts @@ -9,8 +9,8 @@ import { KibanaRequest } from '../../../../../../src/core/server'; import { AuthenticationResult } from '../authentication_result'; import { DeauthenticationResult } from '../deauthentication_result'; import { BaseAuthenticationProvider } from './base'; +import { canRedirectRequest } from '../can_redirect_request'; import { Tokens, TokenPair } from '../tokens'; -import { canRedirectRequest } from '..'; /** * Describes the parameters that are required by the provider to process the initial login request. @@ -29,6 +29,11 @@ type ProviderState = TokenPair; * Provider that supports token-based request authentication. */ export class TokenAuthenticationProvider extends BaseAuthenticationProvider { + /** + * Type of the provider. + */ + static readonly type = 'token'; + /** * Performs initial login request using username and password. * @param request Request instance. diff --git a/x-pack/plugins/security/server/routes/authentication/common.test.ts b/x-pack/plugins/security/server/routes/authentication/common.test.ts index 5d5868d4cc593..4666b5abad756 100644 --- a/x-pack/plugins/security/server/routes/authentication/common.test.ts +++ b/x-pack/plugins/security/server/routes/authentication/common.test.ts @@ -176,20 +176,9 @@ describe('Common authentication routes', () => { expect(routeConfig.validate).toBe(false); }); - it('returns 500 if cannot retrieve current user due to unhandled exception.', async () => { - const unhandledException = new Error('Something went wrong.'); - authc.getCurrentUser.mockRejectedValue(unhandledException); - - const response = await routeHandler(mockContext, mockRequest, kibanaResponseFactory); - - expect(response.status).toBe(500); - expect(response.payload).toEqual(unhandledException); - expect(authc.getCurrentUser).toHaveBeenCalledWith(mockRequest); - }); - it('returns current user.', async () => { const mockUser = mockAuthenticatedUser(); - authc.getCurrentUser.mockResolvedValue(mockUser); + authc.getCurrentUser.mockReturnValue(mockUser); const response = await routeHandler(mockContext, mockRequest, kibanaResponseFactory); diff --git a/x-pack/plugins/security/server/routes/authentication/common.ts b/x-pack/plugins/security/server/routes/authentication/common.ts index cb4ec196459ee..c9856e9dff7f1 100644 --- a/x-pack/plugins/security/server/routes/authentication/common.ts +++ b/x-pack/plugins/security/server/routes/authentication/common.ts @@ -59,7 +59,7 @@ export function defineCommonRoutes({ router, authc, basePath, logger }: RouteDef for (const path of ['/internal/security/me', '/api/security/v1/me']) { router.get( { path, validate: false }, - createLicensedRouteHandler(async (context, request, response) => { + createLicensedRouteHandler((context, request, response) => { if (path === '/api/security/v1/me') { logger.warn( `The "${basePath.serverBasePath}${path}" endpoint is deprecated and will be removed in the next major version.`, @@ -67,11 +67,7 @@ export function defineCommonRoutes({ router, authc, basePath, logger }: RouteDef ); } - try { - return response.ok({ body: (await authc.getCurrentUser(request)) as any }); - } catch (error) { - return response.customError(wrapIntoCustomErrorResponse(error)); - } + return response.ok({ body: authc.getCurrentUser(request)! }); }) ); } diff --git a/x-pack/plugins/security/server/routes/users/change_password.test.ts b/x-pack/plugins/security/server/routes/users/change_password.test.ts index 80a25e03ede62..34509edc2e9d2 100644 --- a/x-pack/plugins/security/server/routes/users/change_password.test.ts +++ b/x-pack/plugins/security/server/routes/users/change_password.test.ts @@ -4,6 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ +import { errors } from 'elasticsearch'; import { ObjectType } from '@kbn/config-schema'; import { IClusterClient, @@ -13,6 +14,7 @@ import { RequestHandler, RequestHandlerContext, RouteConfig, + ScopeableRequest, } from '../../../../../../src/core/server'; import { LICENSE_CHECK_STATE } from '../../../../licensing/server'; import { Authentication, AuthenticationResult } from '../../authentication'; @@ -38,10 +40,7 @@ describe('Change password', () => { let routeConfig: RouteConfig; let mockContext: RequestHandlerContext; - function checkPasswordChangeAPICall( - username: string, - request: ReturnType - ) { + function checkPasswordChangeAPICall(username: string, request: ScopeableRequest) { expect(mockClusterClient.asScoped).toHaveBeenCalledTimes(1); expect(mockClusterClient.asScoped).toHaveBeenCalledWith(request); expect(mockScopedClusterClient.callAsCurrentUser).toHaveBeenCalledTimes(1); @@ -55,8 +54,14 @@ describe('Change password', () => { router = httpServiceMock.createRouter(); authc = authenticationMock.create(); - authc.getCurrentUser.mockResolvedValue(mockAuthenticatedUser({ username: 'user' })); + authc.getCurrentUser.mockReturnValue(mockAuthenticatedUser({ username: 'user' })); authc.login.mockResolvedValue(AuthenticationResult.succeeded(mockAuthenticatedUser())); + authc.getSessionInfo.mockResolvedValue({ + now: Date.now(), + idleTimeoutExpiration: null, + lifespanExpiration: null, + provider: 'basic', + }); mockScopedClusterClient = elasticsearchServiceMock.createScopedClusterClient(); mockClusterClient = elasticsearchServiceMock.createClusterClient(); @@ -122,14 +127,24 @@ describe('Change password', () => { }); it('returns 403 if old password is wrong.', async () => { - const loginFailureReason = new Error('Something went wrong.'); - authc.login.mockResolvedValue(AuthenticationResult.failed(loginFailureReason)); + const changePasswordFailure = new (errors.AuthenticationException as any)('Unauthorized', { + body: { error: { header: { 'WWW-Authenticate': 'Negotiate' } } }, + }); + mockScopedClusterClient.callAsCurrentUser.mockRejectedValue(changePasswordFailure); const response = await routeHandler(mockContext, mockRequest, kibanaResponseFactory); expect(response.status).toBe(403); - expect(response.payload).toEqual(loginFailureReason); - expect(mockScopedClusterClient.callAsCurrentUser).not.toHaveBeenCalled(); + expect(response.payload).toEqual(changePasswordFailure); + + expect(mockScopedClusterClient.callAsCurrentUser).toHaveBeenCalledTimes(1); + expect(mockClusterClient.asScoped).toHaveBeenCalledTimes(1); + expect(mockClusterClient.asScoped).toHaveBeenCalledWith({ + headers: { + ...mockRequest.headers, + authorization: `Basic ${Buffer.from(`${username}:old-password`).toString('base64')}`, + }, + }); }); it(`returns 401 if user can't authenticate with new password.`, async () => { @@ -148,10 +163,15 @@ describe('Change password', () => { expect(response.status).toBe(401); expect(response.payload).toEqual(loginFailureReason); - checkPasswordChangeAPICall(username, mockRequest); + checkPasswordChangeAPICall(username, { + headers: { + ...mockRequest.headers, + authorization: `Basic ${Buffer.from(`${username}:old-password`).toString('base64')}`, + }, + }); }); - it('returns 500 if password update request fails.', async () => { + it('returns 500 if password update request fails with non-401 error.', async () => { const failureReason = new Error('Request failed.'); mockScopedClusterClient.callAsCurrentUser.mockRejectedValue(failureReason); @@ -160,7 +180,12 @@ describe('Change password', () => { expect(response.status).toBe(500); expect(response.payload).toEqual(failureReason); - checkPasswordChangeAPICall(username, mockRequest); + checkPasswordChangeAPICall(username, { + headers: { + ...mockRequest.headers, + authorization: `Basic ${Buffer.from(`${username}:old-password`).toString('base64')}`, + }, + }); }); it('successfully changes own password if provided old password is correct.', async () => { @@ -169,7 +194,62 @@ describe('Change password', () => { expect(response.status).toBe(204); expect(response.payload).toBeUndefined(); - checkPasswordChangeAPICall(username, mockRequest); + checkPasswordChangeAPICall(username, { + headers: { + ...mockRequest.headers, + authorization: `Basic ${Buffer.from(`${username}:old-password`).toString('base64')}`, + }, + }); + + expect(authc.login).toHaveBeenCalledTimes(1); + expect(authc.login).toHaveBeenCalledWith(mockRequest, { + provider: 'basic', + value: { username, password: 'new-password' }, + }); + }); + + it('successfully changes own password if provided old password is correct for non-basic provider.', async () => { + const mockUser = mockAuthenticatedUser({ + username: 'user', + authentication_provider: 'token', + }); + authc.getCurrentUser.mockReturnValue(mockUser); + authc.login.mockResolvedValue(AuthenticationResult.succeeded(mockUser)); + + const response = await routeHandler(mockContext, mockRequest, kibanaResponseFactory); + + expect(response.status).toBe(204); + expect(response.payload).toBeUndefined(); + + checkPasswordChangeAPICall(username, { + headers: { + ...mockRequest.headers, + authorization: `Basic ${Buffer.from(`${username}:old-password`).toString('base64')}`, + }, + }); + + expect(authc.login).toHaveBeenCalledTimes(1); + expect(authc.login).toHaveBeenCalledWith(mockRequest, { + provider: 'token', + value: { username, password: 'new-password' }, + }); + }); + + it('successfully changes own password but does not re-login if current session does not exist.', async () => { + authc.getSessionInfo.mockResolvedValue(null); + const response = await routeHandler(mockContext, mockRequest, kibanaResponseFactory); + + expect(response.status).toBe(204); + expect(response.payload).toBeUndefined(); + + checkPasswordChangeAPICall(username, { + headers: { + ...mockRequest.headers, + authorization: `Basic ${Buffer.from(`${username}:old-password`).toString('base64')}`, + }, + }); + + expect(authc.login).not.toHaveBeenCalled(); }); }); diff --git a/x-pack/plugins/security/server/routes/users/change_password.ts b/x-pack/plugins/security/server/routes/users/change_password.ts index b9d04b4bd1e0e..fc3ca4573d500 100644 --- a/x-pack/plugins/security/server/routes/users/change_password.ts +++ b/x-pack/plugins/security/server/routes/users/change_password.ts @@ -5,7 +5,8 @@ */ import { schema } from '@kbn/config-schema'; -import { wrapIntoCustomErrorResponse } from '../../errors'; +import { canUserChangePassword } from '../../../common/model'; +import { getErrorStatusCode, wrapIntoCustomErrorResponse } from '../../errors'; import { createLicensedRouteHandler } from '../licensed_route_handler'; import { RouteDefinitionParams } from '..'; @@ -13,7 +14,6 @@ export function defineChangeUserPasswordRoutes({ authc, router, clusterClient, - config, }: RouteDefinitionParams) { router.post( { @@ -27,54 +27,65 @@ export function defineChangeUserPasswordRoutes({ }, }, createLicensedRouteHandler(async (context, request, response) => { - const username = request.params.username; - const { password, newPassword } = request.body; - const isCurrentUser = username === (await authc.getCurrentUser(request))!.username; + const { username } = request.params; + const { password: currentPassword, newPassword } = request.body; - // We should prefer `token` over `basic` if possible. - const providerToLoginWith = config.authc.providers.includes('token') ? 'token' : 'basic'; + const currentUser = authc.getCurrentUser(request); + const isUserChangingOwnPassword = + currentUser && currentUser.username === username && canUserChangePassword(currentUser); + const currentSession = isUserChangingOwnPassword ? await authc.getSessionInfo(request) : null; - // If user tries to change own password, let's check if old password is valid first by trying - // to login. - if (isCurrentUser) { - try { - const authenticationResult = await authc.login(request, { - provider: providerToLoginWith, - value: { username, password }, - // We shouldn't alter authentication state just yet. - stateless: true, - }); - - if (!authenticationResult.succeeded()) { - return response.forbidden({ body: authenticationResult.error }); - } - } catch (error) { - return response.customError(wrapIntoCustomErrorResponse(error)); - } - } + // If user is changing their own password they should provide a proof of knowledge their + // current password via sending it in `Authorization: Basic base64(username:current password)` + // HTTP header no matter how they logged in to Kibana. + const scopedClusterClient = clusterClient.asScoped( + isUserChangingOwnPassword + ? { + headers: { + ...request.headers, + authorization: `Basic ${Buffer.from(`${username}:${currentPassword}`).toString( + 'base64' + )}`, + }, + } + : request + ); try { - await clusterClient.asScoped(request).callAsCurrentUser('shield.changePassword', { + await scopedClusterClient.callAsCurrentUser('shield.changePassword', { username, body: { password: newPassword }, }); + } catch (error) { + // This may happen only if user's credentials are rejected meaning that current password + // isn't correct. + if (isUserChangingOwnPassword && getErrorStatusCode(error) === 401) { + return response.forbidden({ body: error }); + } + + return response.customError(wrapIntoCustomErrorResponse(error)); + } - // Now we authenticate user with the new password again updating current session if any. - if (isCurrentUser) { + // If user previously had an active session and changed their own password we should re-login + // user with the new password and update session. We check this since it's possible to update + // password even if user is authenticated via HTTP headers and hence doesn't have an active + // session and in such cases we shouldn't create a new one. + if (isUserChangingOwnPassword && currentSession) { + try { const authenticationResult = await authc.login(request, { - provider: providerToLoginWith, + provider: currentUser!.authentication_provider, value: { username, password: newPassword }, }); if (!authenticationResult.succeeded()) { return response.unauthorized({ body: authenticationResult.error }); } + } catch (error) { + return response.customError(wrapIntoCustomErrorResponse(error)); } - - return response.noContent(); - } catch (error) { - return response.customError(wrapIntoCustomErrorResponse(error)); } + + return response.noContent(); }) ); } diff --git a/x-pack/test/api_integration/apis/security/basic_login.js b/x-pack/test/api_integration/apis/security/basic_login.js index 62deedad6c35c..d4b41603944f6 100644 --- a/x-pack/test/api_integration/apis/security/basic_login.js +++ b/x-pack/test/api_integration/apis/security/basic_login.js @@ -126,6 +126,7 @@ export default function({ getService }) { 'enabled', 'authentication_realm', 'lookup_realm', + 'authentication_provider', ]); expect(apiResponse.body.username).to.be(validUsername); }); @@ -165,6 +166,7 @@ export default function({ getService }) { 'enabled', 'authentication_realm', 'lookup_realm', + 'authentication_provider', ]); expect(apiResponse.body.username).to.be(validUsername); }); diff --git a/x-pack/test/kerberos_api_integration/apis/security/kerberos_login.ts b/x-pack/test/kerberos_api_integration/apis/security/kerberos_login.ts index 203f90c55aa82..570d7026cf99e 100644 --- a/x-pack/test/kerberos_api_integration/apis/security/kerberos_login.ts +++ b/x-pack/test/kerberos_api_integration/apis/security/kerberos_login.ts @@ -74,6 +74,7 @@ export default function({ getService }: FtrProviderContext) { expect(user.username).to.eql(username); expect(user.authentication_realm).to.eql({ name: 'reserved', type: 'reserved' }); + expect(user.authentication_provider).to.eql('basic'); }); describe('initiating SPNEGO', () => { @@ -129,6 +130,7 @@ export default function({ getService }: FtrProviderContext) { enabled: true, authentication_realm: { name: 'kerb1', type: 'kerberos' }, lookup_realm: { name: 'kerb1', type: 'kerberos' }, + authentication_provider: 'kerberos', }); }); diff --git a/x-pack/test/oidc_api_integration/apis/authorization_code_flow/oidc_auth.js b/x-pack/test/oidc_api_integration/apis/authorization_code_flow/oidc_auth.js index 57bcd1b041c49..094537fd61436 100644 --- a/x-pack/test/oidc_api_integration/apis/authorization_code_flow/oidc_auth.js +++ b/x-pack/test/oidc_api_integration/apis/authorization_code_flow/oidc_auth.js @@ -173,6 +173,7 @@ export default function({ getService }) { 'enabled', 'authentication_realm', 'lookup_realm', + 'authentication_provider', ]); expect(apiResponse.body.username).to.be('user1'); @@ -222,6 +223,7 @@ export default function({ getService }) { 'enabled', 'authentication_realm', 'lookup_realm', + 'authentication_provider', ]); expect(apiResponse.body.username).to.be('user2'); diff --git a/x-pack/test/oidc_api_integration/apis/implicit_flow/oidc_auth.ts b/x-pack/test/oidc_api_integration/apis/implicit_flow/oidc_auth.ts index 1f5a64835416a..7d013e97732f9 100644 --- a/x-pack/test/oidc_api_integration/apis/implicit_flow/oidc_auth.ts +++ b/x-pack/test/oidc_api_integration/apis/implicit_flow/oidc_auth.ts @@ -142,6 +142,7 @@ export default function({ getService }: FtrProviderContext) { 'enabled', 'authentication_realm', 'lookup_realm', + 'authentication_provider', ]); expect(apiResponse.body.username).to.be('user1'); diff --git a/x-pack/test/pki_api_integration/apis/security/pki_auth.ts b/x-pack/test/pki_api_integration/apis/security/pki_auth.ts index 186ed824b3b6c..1ae7488fcf379 100644 --- a/x-pack/test/pki_api_integration/apis/security/pki_auth.ts +++ b/x-pack/test/pki_api_integration/apis/security/pki_auth.ts @@ -90,6 +90,7 @@ export default function({ getService }: FtrProviderContext) { expect(user.username).to.eql(username); expect(user.authentication_realm).to.eql({ name: 'reserved', type: 'reserved' }); + expect(user.authentication_provider).to.eql('basic'); }); it('should properly set cookie and authenticate user', async () => { @@ -118,6 +119,7 @@ export default function({ getService }: FtrProviderContext) { }, authentication_realm: { name: 'pki1', type: 'pki' }, lookup_realm: { name: 'pki1', type: 'pki' }, + authentication_provider: 'pki', }); // Cookie should be accepted. @@ -160,6 +162,7 @@ export default function({ getService }: FtrProviderContext) { }, authentication_realm: { name: 'pki1', type: 'pki' }, lookup_realm: { name: 'pki1', type: 'pki' }, + authentication_provider: 'pki', }); checkCookieIsSet(request.cookie(response.headers['set-cookie'][0])!); diff --git a/x-pack/test/saml_api_integration/apis/security/saml_login.ts b/x-pack/test/saml_api_integration/apis/security/saml_login.ts index 0436d59906ea8..6ede8aadeb5a7 100644 --- a/x-pack/test/saml_api_integration/apis/security/saml_login.ts +++ b/x-pack/test/saml_api_integration/apis/security/saml_login.ts @@ -56,6 +56,7 @@ export default function({ getService }: FtrProviderContext) { 'enabled', 'authentication_realm', 'lookup_realm', + 'authentication_provider', ]); expect(apiResponse.body.username).to.be('a@b.c'); @@ -88,6 +89,7 @@ export default function({ getService }: FtrProviderContext) { expect(user.username).to.eql(username); expect(user.authentication_realm).to.eql({ name: 'reserved', type: 'reserved' }); + expect(user.authentication_provider).to.eql('basic'); }); describe('capture URL fragment', () => {