Fail out of auth flow on first provider failure

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