Properly validate current user password during password change.

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,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 () => {
@@ -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 () => {

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);

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', () => {

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,

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;