Skip to content

Commit

Permalink
Properly validate current user password during password change. (#43447)
Browse files Browse the repository at this point in the history
  • Loading branch information
@azasypkin
azasypkin authored Aug 16, 2019
1 parent b8303e4 commit 36b1760
Show file tree
Hide file tree
Showing 11 changed files with 269 additions and 172 deletions.
118 changes: 53 additions & 65 deletions x-pack/legacy/plugins/security/server/routes/api/v1/__tests__/users.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -71,60 +71,54 @@ 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),
{ provider: 'basic', value: { username: 'user', password: 'new-password' } }
)
.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 () => {
Expand All @@ -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),
Expand Down Expand Up @@ -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 () => {
Expand Down
33 changes: 21 additions & 12 deletions x-pack/legacy/plugins/security/server/routes/api/v1/users.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
}
}

Expand All @@ -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);
Expand Down
69 changes: 69 additions & 0 deletions x-pack/plugins/security/server/authentication/authenticator.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
11 changes: 9 additions & 2 deletions x-pack/plugins/security/server/authentication/authenticator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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.`
Expand All @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/security/server/authentication/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading

0 comments on commit 36b1760

Please sign in to comment.