Fail out of auth flow on first provider failure (elastic#26648)

In practical terms, the flexibility afforded by providers being able to
recover from the failures of previously configured providers isn't
compelling, but the ambiguity is not ideal.

x-pack/plugins/security/server/lib/authentication/__tests__/authenticator.js

@@ -102,7 +102,7 @@ describe('Authenticator', () => {
       }
     });
 
-    it('fails if all authentication providers fail.', async () => {
+    it('fails if any authentication providers fail.', async () => {
       const request = requestFixture({ headers: { authorization: 'Basic ***' } });
       session.get.withArgs(request).returns(Promise.resolve(null));
 

x-pack/plugins/security/server/lib/authentication/authenticator.js

@@ -169,6 +169,10 @@ class Authenticator {
         }
       }
 
+      if (authenticationResult.failed()) {
+        return authenticationResult;
+      }
+
       if (authenticationResult.succeeded()) {
         // we have to do this here, as the auth scope's could be dependent on this
         await this._authorizationMode.initialize(request);

x-pack/plugins/security/server/lib/authentication/providers/__tests__/basic.js

@@ -4,7 +4,6 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import Boom from 'boom';
 import expect from 'expect.js';
 import sinon from 'sinon';
 import { requestFixture } from '../../../__tests__/__fixtures__/request';
@@ -97,16 +96,15 @@ describe('BasicAuthenticationProvider', () => {
       sinon.assert.calledOnce(callWithRequest);
     });
 
-    it('fails if `authorization` header has unsupported schema even if state contains valid credentials.', async () => {
+    it('does not handle `authorization` header with unsupported schema even if state contains valid credentials.', async () => {
       const request = requestFixture({ headers: { authorization: 'Bearer ***' } });
       const authorization = generateAuthorizationHeader('user', 'password');
 
       const authenticationResult = await provider.authenticate(request, { authorization });
 
       sinon.assert.notCalled(callWithRequest);
       expect(request.headers.authorization).to.be('Bearer ***');
-      expect(authenticationResult.failed()).to.be(true);
-      expect(authenticationResult.error).to.eql(Boom.badRequest('Unsupported authentication schema: Bearer'));
+      expect(authenticationResult.notHandled()).to.be(true);
     });
 
     it('fails if state contains invalid credentials.', async () => {

x-pack/plugins/security/server/lib/authentication/providers/__tests__/saml.js

@@ -206,7 +206,7 @@ describe('SAMLAuthenticationProvider', () => {
       expect(authenticationResult.state).to.be(undefined);
     });
 
-    it('fails if `authorization` header has unsupported schema even if state contains a valid token.', async () => {
+    it('does not handle `authorization` header with unsupported schema even if state contains a valid token.', async () => {
       const request = requestFixture({ headers: { authorization: 'Basic some:credentials' } });
 
       const authenticationResult = await provider.authenticate(request, {
@@ -216,8 +216,7 @@ describe('SAMLAuthenticationProvider', () => {
 
       sinon.assert.notCalled(callWithRequest);
       expect(request.headers.authorization).to.be('Basic some:credentials');
-      expect(authenticationResult.failed()).to.be(true);
-      expect(authenticationResult.error).to.eql(Boom.badRequest('Unsupported authentication schema: Basic'));
+      expect(authenticationResult.notHandled()).to.be(true);
     });
 
     it('fails if token from the state is rejected because of unknown reason.', async () => {

x-pack/plugins/security/server/lib/authentication/providers/basic.js

@@ -4,7 +4,6 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import Boom from 'boom';
 import { canRedirectRequest } from '../../can_redirect_request';
 import { AuthenticationResult } from '../authentication_result';
 import { DeauthenticationResult } from '../deauthentication_result';
@@ -21,6 +20,14 @@ import { DeauthenticationResult } from '../deauthentication_result';
  * }} ProviderOptions
  */
 
+/**
+ * Object that represents return value of internal header auth
+ * @typedef {{
+ *  authenticationResult: AuthenticationResult,
+ *  headerNotRecognized?: boolean
+ * }} HeaderAuthAttempt
+ */
+
 /**
  * Provider that supports request authentication via Basic HTTP Authentication.
  */
@@ -49,7 +56,13 @@ export class BasicAuthenticationProvider {
   async authenticate(request, state) {
     this._options.log(['debug', 'security', 'basic'], `Trying to authenticate user request to ${request.url.path}.`);
 
-    let authenticationResult = await this._authenticateViaHeader(request);
+    let {
+      authenticationResult,
+      headerNotRecognized, // eslint-disable-line prefer-const
+    } = await this._authenticateViaHeader(request);
+    if (headerNotRecognized) {
+      return authenticationResult;
+    }
 
     if (authenticationResult.notHandled() && state) {
       authenticationResult = await this._authenticateViaState(request, state);
@@ -81,7 +94,7 @@ export class BasicAuthenticationProvider {
    * Validates whether request contains `Basic ***` Authorization header and just passes it
    * forward to Elasticsearch backend.
    * @param {Hapi.Request} request HapiJS request instance.
-   * @returns {Promise.<AuthenticationResult>}
+   * @returns {Promise.<HeaderAuthAttempt>}
    * @private
    */
   async _authenticateViaHeader(request) {
@@ -90,30 +103,33 @@ export class BasicAuthenticationProvider {
     const authorization = request.headers.authorization;
     if (!authorization) {
       this._options.log(['debug', 'security', 'basic'], 'Authorization header is not presented.');
-      return AuthenticationResult.notHandled();
+      return {
+        authenticationResult: AuthenticationResult.notHandled()
+      };
     }
 
     const authenticationSchema = authorization.split(/\s+/)[0];
     if (authenticationSchema.toLowerCase() !== 'basic') {
       this._options.log(['debug', 'security', 'basic'], `Unsupported authentication schema: ${authenticationSchema}`);
-
-      // It's essential that we fail if non-empty, but unsupported authentication schema
-      // is provided to allow authenticator to consult other authentication providers
-      // that may support that schema.
-      return AuthenticationResult.failed(
-        Boom.badRequest(`Unsupported authentication schema: ${authenticationSchema}`)
-      );
+      return {
+        authenticationResult: AuthenticationResult.notHandled(),
+        headerNotRecognized: true
+      };
     }
 
     try {
       const user = await this._options.client.callWithRequest(request, 'shield.authenticate');
 
       this._options.log(['debug', 'security', 'basic'], 'Request has been authenticated via header.');
 
-      return AuthenticationResult.succeeded(user, { authorization });
+      return {
+        authenticationResult: AuthenticationResult.succeeded(user, { authorization })
+      };
     } catch(err) {
       this._options.log(['debug', 'security', 'basic'], `Failed to authenticate request via header: ${err.message}`);
-      return AuthenticationResult.failed(err);
+      return {
+        authenticationResult: AuthenticationResult.failed(err)
+      };
     }
   }
 

x-pack/plugins/security/server/lib/authentication/providers/saml.js

@@ -21,6 +21,14 @@ import { DeauthenticationResult } from '../deauthentication_result';
  * }} ProviderOptions
  */
 
+/**
+ * Object that represents return value of internal header auth
+ * @typedef {{
+ *  authenticationResult: AuthenticationResult,
+ *  headerNotRecognized?: boolean
+ * }} HeaderAuthAttempt
+ */
+
 /**
  * Checks the error returned by Elasticsearch as the result of `authenticate` call and returns `true` if request
  * has been rejected because of expired token, otherwise returns `false`.
@@ -74,7 +82,14 @@ export class SAMLAuthenticationProvider {
   async authenticate(request, state) {
     this._options.log(['debug', 'security', 'saml'], `Trying to authenticate user request to ${request.url.path}.`);
 
-    let authenticationResult = await this._authenticateViaHeader(request);
+    let {
+      authenticationResult,
+      headerNotRecognized, // eslint-disable-line prefer-const
+    } = await this._authenticateViaHeader(request);
+    if (headerNotRecognized) {
+      return authenticationResult;
+    }
+
     if (state && authenticationResult.notHandled()) {
       authenticationResult = await this._authenticateViaState(request, state);
       if (authenticationResult.failed() && isAccessTokenExpiredError(authenticationResult.error)) {
@@ -98,7 +113,7 @@ export class SAMLAuthenticationProvider {
    * Validates whether request contains `Bearer ***` Authorization header and just passes it
    * forward to Elasticsearch backend.
    * @param {Hapi.Request} request HapiJS request instance.
-   * @returns {Promise.<AuthenticationResult>}
+   * @returns {Promise.<HeaderAuthAttempt>}
    * @private
    */
   async _authenticateViaHeader(request) {
@@ -107,19 +122,18 @@ export class SAMLAuthenticationProvider {
     const authorization = request.headers.authorization;
     if (!authorization) {
       this._options.log(['debug', 'security', 'saml'], 'Authorization header is not presented.');
-      return AuthenticationResult.notHandled();
+      return {
+        authenticationResult: AuthenticationResult.notHandled()
+      };
     }
 
     const authenticationSchema = authorization.split(/\s+/)[0];
     if (authenticationSchema.toLowerCase() !== 'bearer') {
       this._options.log(['debug', 'security', 'saml'], `Unsupported authentication schema: ${authenticationSchema}`);
-
-      // It's essential that we fail if non-empty, but unsupported authentication schema
-      // is provided to allow authenticator to consult other authentication providers
-      // that may support that schema.
-      return AuthenticationResult.failed(
-        Boom.badRequest(`Unsupported authentication schema: ${authenticationSchema}`)
-      );
+      return {
+        authenticationResult: AuthenticationResult.notHandled(),
+        headerNotRecognized: true
+      };
     }
 
     try {
@@ -130,10 +144,14 @@ export class SAMLAuthenticationProvider {
 
       this._options.log(['debug', 'security', 'saml'], 'Request has been authenticated via header.');
 
-      return AuthenticationResult.succeeded(user);
+      return {
+        authenticationResult: AuthenticationResult.succeeded(user)
+      };
     } catch(err) {
       this._options.log(['debug', 'security', 'saml'], `Failed to authenticate request via header: ${err.message}`);
-      return AuthenticationResult.failed(err);
+      return {
+        authenticationResult: AuthenticationResult.failed(err)
+      };
     }
   }
 

x-pack/test/api_integration/apis/security/basic_login.js

@@ -189,7 +189,7 @@ export default function ({ getService }) {
           .set('kbn-xsrf', 'xxx')
           .set('Authorization', 'Bearer AbCdEf')
           .set('Cookie', sessionCookie.cookieString())
-          .expect(400);
+          .expect(401);
 
         expect(apiResponse.headers['set-cookie']).to.be(undefined);
       });

x-pack/test/saml_api_integration/apis/security/saml_login.js

@@ -275,7 +275,7 @@ export default function ({ getService }) {
           .set('kbn-xsrf', 'xxx')
           .set('Authorization', 'Basic AbCdEf')
           .set('Cookie', sessionCookie.cookieString())
-          .expect(400);
+          .expect(401);
 
         expect(apiResponse.headers['set-cookie']).to.be(undefined);
       });