From fc853f193c6cc2b0a8989df54dcb27bb03ce0cd7 Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Fri, 16 Aug 2019 12:41:50 +0200 Subject: [PATCH 1/2] Properly validate current user password during password change. --- .../server/routes/api/v1/__tests__/users.js | 118 ++++++++---------- .../security/server/routes/api/v1/users.js | 33 +++-- .../authentication/authenticator.test.ts | 69 ++++++++++ .../server/authentication/authenticator.ts | 11 +- .../security/server/authentication/index.ts | 2 +- .../authentication/providers/basic.test.ts | 48 +------ .../server/authentication/providers/basic.ts | 49 +------- .../server/authentication/providers/index.ts | 2 +- x-pack/plugins/security/server/index.ts | 1 - .../apis/security/change_password.ts | 116 +++++++++++++++++ .../api_integration/apis/security/index.js | 1 + 11 files changed, 278 insertions(+), 172 deletions(-) create mode 100644 x-pack/test/api_integration/apis/security/change_password.ts diff --git a/x-pack/legacy/plugins/security/server/routes/api/v1/__tests__/users.js b/x-pack/legacy/plugins/security/server/routes/api/v1/__tests__/users.js index 69ebc526fd8982..8444efe8790e71 100644 --- a/x-pack/legacy/plugins/security/server/routes/api/v1/__tests__/users.js +++ b/x-pack/legacy/plugins/security/server/routes/api/v1/__tests__/users.js @@ -10,7 +10,7 @@ import sinon from 'sinon'; import { serverFixture } from '../../../../lib/__tests__/__fixtures__/server'; import { requestFixture } from '../../../../lib/__tests__/__fixtures__/request'; -import { AuthenticationResult, BasicCredentials } from '../../../../../../../../plugins/security/server'; +import { AuthenticationResult } from '../../../../../../../../plugins/security/server'; import { initUsersApi } from '../users'; import * as ClientShield from '../../../../../../../server/lib/get_client_shield'; import { KibanaRequest } from '../../../../../../../../../src/core/server'; @@ -71,35 +71,31 @@ describe('User routes', () => { }); describe('own password', () => { - let getUserStub; beforeEach(() => { request.params.username = request.auth.credentials.username; - - getUserStub = serverStub.plugins.security.getUser + loginStub = loginStub .withArgs( - sinon.match(BasicCredentials.decorateRequest(request, 'user', 'old-password')) - ); + sinon.match.instanceOf(KibanaRequest), + { provider: 'basic', value: { username: 'user', password: 'old-password' }, stateless: true } + ) + .resolves(AuthenticationResult.succeeded({})); }); it('returns 401 if old password is wrong.', async () => { - getUserStub.returns(Promise.reject(new Error('Something went wrong.'))); - - return changePasswordRoute - .handler(request) - .catch((response) => { - sinon.assert.notCalled(clusterStub.callWithRequest); - expect(response.isBoom).to.be(true); - expect(response.output.payload).to.eql({ - statusCode: 401, - error: 'Unauthorized', - message: 'Something went wrong.' - }); - }); + loginStub.resolves(AuthenticationResult.failed(new Error('Something went wrong.'))); + + const response = await changePasswordRoute.handler(request); + + sinon.assert.notCalled(clusterStub.callWithRequest); + expect(response.isBoom).to.be(true); + expect(response.output.payload).to.eql({ + statusCode: 401, + error: 'Unauthorized', + message: 'Something went wrong.' + }); }); it('returns 401 if user can authenticate with new password.', async () => { - getUserStub.returns(Promise.resolve({})); - loginStub .withArgs( sinon.match.instanceOf(KibanaRequest), @@ -107,24 +103,22 @@ describe('User routes', () => { ) .resolves(AuthenticationResult.failed(new Error('Something went wrong.'))); - return changePasswordRoute - .handler(request) - .catch((response) => { - sinon.assert.calledOnce(clusterStub.callWithRequest); - sinon.assert.calledWithExactly( - clusterStub.callWithRequest, - sinon.match.same(request), - 'shield.changePassword', - { username: 'user', body: { password: 'new-password' } } - ); - - expect(response.isBoom).to.be(true); - expect(response.output.payload).to.eql({ - statusCode: 401, - error: 'Unauthorized', - message: 'Something went wrong.' - }); - }); + const response = await changePasswordRoute.handler(request); + + sinon.assert.calledOnce(clusterStub.callWithRequest); + sinon.assert.calledWithExactly( + clusterStub.callWithRequest, + sinon.match.same(request), + 'shield.changePassword', + { username: 'user', body: { password: 'new-password' } } + ); + + expect(response.isBoom).to.be(true); + expect(response.output.payload).to.eql({ + statusCode: 401, + error: 'Unauthorized', + message: 'Something went wrong.' + }); }); it('returns 500 if password update request fails.', async () => { @@ -134,23 +128,19 @@ describe('User routes', () => { 'shield.changePassword', { username: 'user', body: { password: 'new-password' } } ) - .returns(Promise.reject(new Error('Request failed.'))); + .rejects(new Error('Request failed.')); + + const response = await changePasswordRoute.handler(request); - return changePasswordRoute - .handler(request) - .catch((response) => { - expect(response.isBoom).to.be(true); - expect(response.output.payload).to.eql({ - statusCode: 500, - error: 'Internal Server Error', - message: 'An internal server error occurred' - }); - }); + expect(response.isBoom).to.be(true); + expect(response.output.payload).to.eql({ + statusCode: 500, + error: 'Internal Server Error', + message: 'An internal server error occurred' + }); }); it('successfully changes own password if provided old password is correct.', async () => { - getUserStub.returns(Promise.resolve({})); - loginStub .withArgs( sinon.match.instanceOf(KibanaRequest), @@ -186,19 +176,17 @@ describe('User routes', () => { ) .returns(Promise.reject(new Error('Request failed.'))); - return changePasswordRoute - .handler(request) - .catch((response) => { - sinon.assert.notCalled(serverStub.plugins.security.getUser); - sinon.assert.notCalled(loginStub); - - expect(response.isBoom).to.be(true); - expect(response.output.payload).to.eql({ - statusCode: 500, - error: 'Internal Server Error', - message: 'An internal server error occurred' - }); - }); + const response = await changePasswordRoute.handler(request); + + sinon.assert.notCalled(serverStub.plugins.security.getUser); + sinon.assert.notCalled(loginStub); + + expect(response.isBoom).to.be(true); + expect(response.output.payload).to.eql({ + statusCode: 500, + error: 'Internal Server Error', + message: 'An internal server error occurred' + }); }); it('successfully changes user password.', async () => { diff --git a/x-pack/legacy/plugins/security/server/routes/api/v1/users.js b/x-pack/legacy/plugins/security/server/routes/api/v1/users.js index 09ade22d61456d..9cb2ad799a211c 100644 --- a/x-pack/legacy/plugins/security/server/routes/api/v1/users.js +++ b/x-pack/legacy/plugins/security/server/routes/api/v1/users.js @@ -10,7 +10,7 @@ import Joi from 'joi'; import { getClient } from '../../../../../../server/lib/get_client_shield'; import { userSchema } from '../../../lib/user_schema'; import { routePreCheckLicense } from '../../../lib/route_pre_check_license'; -import { BasicCredentials, wrapError } from '../../../../../../../plugins/security/server'; +import { wrapError } from '../../../../../../../plugins/security/server'; import { KibanaRequest } from '../../../../../../../../src/core/server'; export function initUsersApi({ authc: { login }, config }, server) { @@ -88,14 +88,27 @@ export function initUsersApi({ authc: { login }, config }, server) { const { password, newPassword } = request.payload; const isCurrentUser = username === request.auth.credentials.username; - // If user tries to change own password, let's check if old password is valid first. + // We should prefer `token` over `basic` if possible. + const providerToLoginWith = config.authc.providers.includes('token') + ? 'token' + : 'basic'; + + // If user tries to change own password, let's check if old password is valid first by trying + // to login. if (isCurrentUser) { try { - await server.plugins.security.getUser( - BasicCredentials.decorateRequest(request, username, password) - ); + const authenticationResult = await login(KibanaRequest.from(request), { + provider: providerToLoginWith, + value: { username, password }, + // We shouldn't alter authentication state just yet. + stateless: true, + }); + + if (!authenticationResult.succeeded()) { + return Boom.unauthorized(authenticationResult.error); + } } catch(err) { - throw Boom.unauthorized(err); + return Boom.unauthorized(err); } } @@ -105,21 +118,17 @@ export function initUsersApi({ authc: { login }, config }, server) { // Now we authenticate user with the new password again updating current session if any. if (isCurrentUser) { - // We should prefer `token` over `basic` if possible. - const providerToLoginWith = config.authc.providers.includes('token') - ? 'token' - : 'basic'; const authenticationResult = await login(KibanaRequest.from(request), { provider: providerToLoginWith, value: { username, password: newPassword } }); if (!authenticationResult.succeeded()) { - throw Boom.unauthorized((authenticationResult.error)); + return Boom.unauthorized((authenticationResult.error)); } } } catch(err) { - throw wrapError(err); + return wrapError(err); } return h.response().code(204); diff --git a/x-pack/plugins/security/server/authentication/authenticator.test.ts b/x-pack/plugins/security/server/authentication/authenticator.test.ts index 510ae6ecff33bf..78c6feac0fa297 100644 --- a/x-pack/plugins/security/server/authentication/authenticator.test.ts +++ b/x-pack/plugins/security/server/authentication/authenticator.test.ts @@ -210,6 +210,75 @@ 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 38b7b0ac0152a3..f01047eff7aa32 100644 --- a/x-pack/plugins/security/server/authentication/authenticator.ts +++ b/x-pack/plugins/security/server/authentication/authenticator.ts @@ -66,6 +66,13 @@ 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 { @@ -220,7 +227,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 = await this.getSessionValue(sessionStorage); + let existingSession = attempt.stateless ? null : await this.getSessionValue(sessionStorage); if (existingSession && existingSession.provider !== attempt.provider) { this.logger.debug( `Clearing existing session of another ("${existingSession.provider}") provider.` @@ -247,7 +254,7 @@ export class Authenticator { (authenticationResult.failed() && getErrorStatusCode(authenticationResult.error) === 401); if (existingSession && shouldClearSession) { sessionStorage.clear(); - } else if (authenticationResult.shouldUpdateState()) { + } else if (!attempt.stateless && authenticationResult.shouldUpdateState()) { sessionStorage.set({ state: authenticationResult.state, provider: attempt.provider, diff --git a/x-pack/plugins/security/server/authentication/index.ts b/x-pack/plugins/security/server/authentication/index.ts index ac06b650bf59e2..796c6305600e32 100644 --- a/x-pack/plugins/security/server/authentication/index.ts +++ b/x-pack/plugins/security/server/authentication/index.ts @@ -20,7 +20,7 @@ export { canRedirectRequest } from './can_redirect_request'; export { Authenticator, ProviderLoginAttempt } from './authenticator'; export { AuthenticationResult } from './authentication_result'; export { DeauthenticationResult } from './deauthentication_result'; -export { BasicCredentials, OIDCAuthenticationFlow } from './providers'; +export { OIDCAuthenticationFlow } from './providers'; interface SetupAuthenticationParams { core: CoreSetup; diff --git a/x-pack/plugins/security/server/authentication/providers/basic.test.ts b/x-pack/plugins/security/server/authentication/providers/basic.test.ts index 57ec808151f704..f9c665d6cea48c 100644 --- a/x-pack/plugins/security/server/authentication/providers/basic.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/basic.test.ts @@ -10,18 +10,10 @@ import { httpServerMock } from '../../../../../../src/core/server/mocks'; import { mockAuthenticatedUser } from '../../../common/model/authenticated_user.mock'; import { mockAuthenticationProviderOptions, mockScopedClusterClient } from './base.mock'; -import { BasicAuthenticationProvider, BasicCredentials } from './basic'; +import { BasicAuthenticationProvider } from './basic'; function generateAuthorizationHeader(username: string, password: string) { - const { - headers: { authorization }, - } = BasicCredentials.decorateRequest( - { headers: {} as Record }, - username, - password - ); - - return authorization as string; + return `Basic ${Buffer.from(`${username}:${password}`).toString('base64')}`; } describe('BasicAuthenticationProvider', () => { @@ -215,40 +207,4 @@ describe('BasicAuthenticationProvider', () => { ); }); }); - - describe('BasicCredentials', () => { - it('`decorateRequest` fails if username or password is not provided.', () => { - expect(() => - BasicCredentials.decorateRequest(undefined as any, undefined as any, undefined as any) - ).toThrowError('Request should be a valid object'); - expect(() => - BasicCredentials.decorateRequest({} as any, undefined as any, undefined as any) - ).toThrowError('Username should be a valid non-empty string'); - expect(() => BasicCredentials.decorateRequest({} as any, '', undefined as any)).toThrowError( - 'Username should be a valid non-empty string' - ); - expect(() => BasicCredentials.decorateRequest({} as any, '', '')).toThrowError( - 'Username should be a valid non-empty string' - ); - expect(() => BasicCredentials.decorateRequest({} as any, 'username', '')).toThrowError( - 'Password should be a valid non-empty string' - ); - expect(() => BasicCredentials.decorateRequest({} as any, '', 'password')).toThrowError( - 'Username should be a valid non-empty string' - ); - }); - - it('`decorateRequest` correctly sets authorization header.', () => { - const oneRequest = { headers: {} as Record }; - const anotherRequest = { headers: { authorization: 'Basic ***' } }; - - BasicCredentials.decorateRequest(oneRequest, 'one-user', 'one-password'); - BasicCredentials.decorateRequest(anotherRequest, 'another-user', 'another-password'); - - expect(oneRequest.headers.authorization).toBe('Basic b25lLXVzZXI6b25lLXBhc3N3b3Jk'); - expect(anotherRequest.headers.authorization).toBe( - 'Basic YW5vdGhlci11c2VyOmFub3RoZXItcGFzc3dvcmQ=' - ); - }); - }); }); diff --git a/x-pack/plugins/security/server/authentication/providers/basic.ts b/x-pack/plugins/security/server/authentication/providers/basic.ts index 8dcd783e7bd36e..07d141b4e1b2b3 100644 --- a/x-pack/plugins/security/server/authentication/providers/basic.ts +++ b/x-pack/plugins/security/server/authentication/providers/basic.ts @@ -4,49 +4,12 @@ * you may not use this file except in compliance with the Elastic License. */ -/* eslint-disable max-classes-per-file */ - -import { FakeRequest, KibanaRequest } from '../../../../../../src/core/server'; +import { KibanaRequest } from '../../../../../../src/core/server'; import { canRedirectRequest } from '../can_redirect_request'; import { AuthenticationResult } from '../authentication_result'; import { DeauthenticationResult } from '../deauthentication_result'; import { BaseAuthenticationProvider } from './base'; -/** - * Utility class that knows how to decorate request with proper Basic authentication headers. - */ -export class BasicCredentials { - /** - * Takes provided `username` and `password`, transforms them into proper `Basic ***` authorization - * header and decorates passed request with it. - * @param request Request instance. - * @param username User name. - * @param password User password. - */ - public static decorateRequest( - request: T, - username: string, - password: string - ) { - const typeOfRequest = typeof request; - if (!request || typeOfRequest !== 'object') { - throw new Error('Request should be a valid object.'); - } - - if (!username || typeof username !== 'string') { - throw new Error('Username should be a valid non-empty string.'); - } - - if (!password || typeof password !== 'string') { - throw new Error('Password should be a valid non-empty string.'); - } - - const basicCredentials = Buffer.from(`${username}:${password}`).toString('base64'); - request.headers.authorization = `Basic ${basicCredentials}`; - return request; - } -} - /** * Describes the parameters that are required by the provider to process the initial login request. */ @@ -84,13 +47,11 @@ export class BasicAuthenticationProvider extends BaseAuthenticationProvider { ) { this.logger.debug('Trying to perform a login.'); - try { - const { headers: authHeaders } = BasicCredentials.decorateRequest( - { headers: {} }, - username, - password - ); + const authHeaders = { + authorization: `Basic ${Buffer.from(`${username}:${password}`).toString('base64')}`, + }; + try { const user = await this.getUser(request, authHeaders); this.logger.debug('Login has been successfully performed.'); diff --git a/x-pack/plugins/security/server/authentication/providers/index.ts b/x-pack/plugins/security/server/authentication/providers/index.ts index 856c614775a934..fef3be16c8d918 100644 --- a/x-pack/plugins/security/server/authentication/providers/index.ts +++ b/x-pack/plugins/security/server/authentication/providers/index.ts @@ -9,7 +9,7 @@ export { AuthenticationProviderOptions, AuthenticationProviderSpecificOptions, } from './base'; -export { BasicAuthenticationProvider, BasicCredentials } from './basic'; +export { BasicAuthenticationProvider } from './basic'; export { KerberosAuthenticationProvider } from './kerberos'; export { SAMLAuthenticationProvider, isSAMLRequestQuery } from './saml'; export { TokenAuthenticationProvider } from './token'; diff --git a/x-pack/plugins/security/server/index.ts b/x-pack/plugins/security/server/index.ts index 2bc38990f45ac4..fe1ccf4ab7fa95 100644 --- a/x-pack/plugins/security/server/index.ts +++ b/x-pack/plugins/security/server/index.ts @@ -16,7 +16,6 @@ export { wrapError } from './errors'; export { canRedirectRequest, AuthenticationResult, - BasicCredentials, DeauthenticationResult, OIDCAuthenticationFlow, } from './authentication'; diff --git a/x-pack/test/api_integration/apis/security/change_password.ts b/x-pack/test/api_integration/apis/security/change_password.ts new file mode 100644 index 00000000000000..47063f031fe6a4 --- /dev/null +++ b/x-pack/test/api_integration/apis/security/change_password.ts @@ -0,0 +1,116 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { Cookie, cookie } from 'request'; +import { FtrProviderContext } from '../../ftr_provider_context'; + +export default function({ getService }: FtrProviderContext) { + const supertest = getService('supertestWithoutAuth'); + + const mockUserName = 'test-user'; + const mockUserPassword = 'test-password'; + + describe('Change password', () => { + let sessionCookie: Cookie; + beforeEach(async () => { + // Create mock user to change password for. + await getService('supertest') + .post(`/api/security/v1/users/${mockUserName}`) + .set('kbn-xsrf', 'xxx') + .send({ username: mockUserName, password: mockUserPassword, roles: [] }) + .expect(200); + + const loginResponse = await supertest + .post('/api/security/v1/login') + .set('kbn-xsrf', 'xxx') + .send({ username: mockUserName, password: mockUserPassword }) + .expect(204); + sessionCookie = cookie(loginResponse.headers['set-cookie'][0])!; + }); + + afterEach(async () => { + // Remove mock user. + await getService('supertest') + .delete(`/api/security/v1/users/${mockUserName}`) + .set('kbn-xsrf', 'xxx') + .expect(204); + }); + + it('should reject password change if current password is wrong', async () => { + const wrongPassword = `wrong-${mockUserPassword}`; + const newPassword = `xxx-${mockUserPassword}-xxx`; + + await supertest + .post(`/api/security/v1/users/${mockUserName}/password`) + .set('kbn-xsrf', 'xxx') + .set('Cookie', sessionCookie.cookieString()) + .send({ password: wrongPassword, newPassword }) + .expect(401); + + // Let's check that we can't login with wrong password (bug happen :shrug:). + await supertest + .post('/api/security/v1/login') + .set('kbn-xsrf', 'xxx') + .send({ username: mockUserName, password: wrongPassword }) + .expect(401); + + // Let's check that we can't login with the password we were supposed to set. + await supertest + .post('/api/security/v1/login') + .set('kbn-xsrf', 'xxx') + .send({ username: mockUserName, password: newPassword }) + .expect(401); + + // And can login with the current password. + await supertest + .post('/api/security/v1/login') + .set('kbn-xsrf', 'xxx') + .send({ username: mockUserName, password: mockUserPassword }) + .expect(204); + }); + + it('should allow password change if current password is correct', async () => { + const newPassword = `xxx-${mockUserPassword}-xxx`; + + const passwordChangeResponse = await supertest + .post(`/api/security/v1/users/${mockUserName}/password`) + .set('kbn-xsrf', 'xxx') + .set('Cookie', sessionCookie.cookieString()) + .send({ password: mockUserPassword, newPassword }) + .expect(204); + + const newSessionCookie = cookie(passwordChangeResponse.headers['set-cookie'][0])!; + + // Let's check that previous cookie isn't valid anymore. + await supertest + .get('/api/security/v1/me') + .set('kbn-xsrf', 'xxx') + .set('Cookie', sessionCookie.cookieString()) + .expect(401); + + // And that we can't login with the old password. + await supertest + .post('/api/security/v1/login') + .set('kbn-xsrf', 'xxx') + .send({ username: mockUserName, password: mockUserPassword }) + .expect(401); + + // But new cookie should be valid. + await supertest + .get('/api/security/v1/me') + .set('kbn-xsrf', 'xxx') + .set('Cookie', newSessionCookie.cookieString()) + .expect(200); + + // And that we can login with new credentials. + await supertest + .post('/api/security/v1/login') + .set('kbn-xsrf', 'xxx') + .send({ username: mockUserName, password: newPassword }) + .expect(204); + }); + }); +} diff --git a/x-pack/test/api_integration/apis/security/index.js b/x-pack/test/api_integration/apis/security/index.js index b272af4a354605..4d034622427fce 100644 --- a/x-pack/test/api_integration/apis/security/index.js +++ b/x-pack/test/api_integration/apis/security/index.js @@ -10,6 +10,7 @@ export default function ({ loadTestFile }) { loadTestFile(require.resolve('./basic_login')); loadTestFile(require.resolve('./builtin_es_privileges')); + loadTestFile(require.resolve('./change_password')); loadTestFile(require.resolve('./index_fields')); loadTestFile(require.resolve('./roles')); loadTestFile(require.resolve('./privileges')); From e090c503066670d28a422aabae0c2915d6a6491e Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Fri, 16 Aug 2019 19:49:53 +0200 Subject: [PATCH 2/2] Review#1: use `security` service to create and delete users in tests, fix typo in comment. --- .../apis/security/change_password.ts | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/x-pack/test/api_integration/apis/security/change_password.ts b/x-pack/test/api_integration/apis/security/change_password.ts index 47063f031fe6a4..f6ddc2a724c39a 100644 --- a/x-pack/test/api_integration/apis/security/change_password.ts +++ b/x-pack/test/api_integration/apis/security/change_password.ts @@ -6,9 +6,11 @@ import { Cookie, cookie } from 'request'; import { FtrProviderContext } from '../../ftr_provider_context'; +import { SecurityService } from '../../../common/services/security'; export default function({ getService }: FtrProviderContext) { const supertest = getService('supertestWithoutAuth'); + const security: SecurityService = getService('security'); const mockUserName = 'test-user'; const mockUserPassword = 'test-password'; @@ -16,12 +18,7 @@ export default function({ getService }: FtrProviderContext) { describe('Change password', () => { let sessionCookie: Cookie; beforeEach(async () => { - // Create mock user to change password for. - await getService('supertest') - .post(`/api/security/v1/users/${mockUserName}`) - .set('kbn-xsrf', 'xxx') - .send({ username: mockUserName, password: mockUserPassword, roles: [] }) - .expect(200); + await security.user.create(mockUserName, { password: mockUserPassword, roles: [] }); const loginResponse = await supertest .post('/api/security/v1/login') @@ -31,13 +28,7 @@ export default function({ getService }: FtrProviderContext) { sessionCookie = cookie(loginResponse.headers['set-cookie'][0])!; }); - afterEach(async () => { - // Remove mock user. - await getService('supertest') - .delete(`/api/security/v1/users/${mockUserName}`) - .set('kbn-xsrf', 'xxx') - .expect(204); - }); + afterEach(async () => await security.user.delete(mockUserName)); it('should reject password change if current password is wrong', async () => { const wrongPassword = `wrong-${mockUserPassword}`; @@ -50,7 +41,7 @@ export default function({ getService }: FtrProviderContext) { .send({ password: wrongPassword, newPassword }) .expect(401); - // Let's check that we can't login with wrong password (bug happen :shrug:). + // Let's check that we can't login with wrong password, just in case. await supertest .post('/api/security/v1/login') .set('kbn-xsrf', 'xxx')