Ignore intermediate unauthenticated session during repeated authentication attempt. (#79300)

.github/CODEOWNERS

@@ -263,10 +263,8 @@
 /x-pack/test/encrypted_saved_objects_api_integration/ @elastic/kibana-security
 /x-pack/test/functional/apps/security/ @elastic/kibana-security
 /x-pack/test/kerberos_api_integration/ @elastic/kibana-security
-/x-pack/test/login_selector_api_integration/ @elastic/kibana-security
 /x-pack/test/oidc_api_integration/ @elastic/kibana-security
 /x-pack/test/pki_api_integration/ @elastic/kibana-security
-/x-pack/test/saml_api_integration/ @elastic/kibana-security
 /x-pack/test/security_api_integration/ @elastic/kibana-security
 /x-pack/test/security_functional/ @elastic/kibana-security
 /x-pack/test/spaces_api_integration/ @elastic/kibana-security

x-pack/README.md

@@ -55,7 +55,7 @@ yarn test:mocha
 
 For more info, see [the Elastic functional test development guide](https://www.elastic.co/guide/en/kibana/current/development-functional-tests.html).
 
-The functional UI tests, the API integration tests, and the SAML API integration tests are all run against a live browser, Kibana, and Elasticsearch install. Each set of tests is specified with a unique config that describes how to start the Elasticsearch server, the Kibana server, and what tests to run against them. The sets of tests that exist today are *functional UI tests* ([specified by this config](test/functional/config.js)), *API integration tests* ([specified by this config](test/api_integration/config.ts)), and *SAML API integration tests* ([specified by this config](test/saml_api_integration/config.ts)).
+The functional UI tests, the API integration tests, and the SAML API integration tests are all run against a live browser, Kibana, and Elasticsearch install. Each set of tests is specified with a unique config that describes how to start the Elasticsearch server, the Kibana server, and what tests to run against them. The sets of tests that exist today are *functional UI tests* ([specified by this config](test/functional/config.js)), *API integration tests* ([specified by this config](test/api_integration/config.ts)), and *SAML API integration tests* ([specified by this config](test/security_api_integration/saml.config.ts)).
 
 The script runs all sets of tests sequentially like so:
 * builds Elasticsearch and X-Pack
@@ -108,7 +108,7 @@ node scripts/functional_tests --config test/api_integration/config
 We also have SAML API integration tests which set up Elasticsearch and Kibana with SAML support. Run _only_ API integration tests with SAML enabled like so:
 
 ```sh
-node scripts/functional_tests --config test/saml_api_integration/config
+node scripts/functional_tests --config test/security_api_integration/saml.config
 ```
 
 #### Running Jest integration tests

x-pack/plugins/security/server/authentication/authenticator.test.ts

@@ -1374,6 +1374,14 @@ describe('Authenticator', () => {
             '/mock-server-basepath/login?next=%2Fmock-server-basepath%2Fpath'
           )
         );
+
+        // Unauthenticated session should be treated as non-existent one.
+        mockOptions.session.get.mockResolvedValue({ ...mockSessVal, username: undefined });
+        await expect(authenticator.authenticate(request)).resolves.toEqual(
+          AuthenticationResult.redirectTo(
+            '/mock-server-basepath/login?next=%2Fmock-server-basepath%2Fpath'
+          )
+        );
         expect(mockBasicAuthenticationProvider.authenticate).not.toHaveBeenCalled();
       });
     });
@@ -1591,26 +1599,6 @@ describe('Authenticator', () => {
         );
       });
 
-      it('does not redirect to Overwritten Session if session was unauthenticated before this authentication attempt', async () => {
-        const request = httpServerMock.createKibanaRequest();
-        mockOptions.session.get.mockResolvedValue({ ...mockSessVal, username: undefined });
-
-        const newMockUser = mockAuthenticatedUser({ username: 'new-username' });
-        mockBasicAuthenticationProvider.authenticate.mockResolvedValue(
-          AuthenticationResult.succeeded(newMockUser, {
-            state: 'some-state',
-            authResponseHeaders: { 'WWW-Authenticate': 'Negotiate' },
-          })
-        );
-
-        await expect(authenticator.authenticate(request)).resolves.toEqual(
-          AuthenticationResult.succeeded(newMockUser, {
-            state: 'some-state',
-            authResponseHeaders: { 'WWW-Authenticate': 'Negotiate' },
-          })
-        );
-      });
-
       it('redirects to Overwritten Session when username changes', async () => {
         const request = httpServerMock.createKibanaRequest();
         mockOptions.session.get.mockResolvedValue({ ...mockSessVal, username: 'old-username' });

x-pack/plugins/security/server/authentication/authenticator.ts

@@ -131,6 +131,10 @@ function isLoginAttemptWithProviderType(
   );
 }
 
+function isSessionAuthenticated(sessionValue?: Readonly<SessionValue> | null) {
+  return !!sessionValue?.username;
+}
+
 /**
  * Instantiates authentication provider based on the provider key from config.
  * @param providerType Provider type key.
@@ -558,7 +562,7 @@ export class Authenticator {
       return ownsSession ? { value: existingSessionValue, overwritten: false } : null;
     }
 
-    const isExistingSessionAuthenticated = !!existingSessionValue?.username;
+    const isExistingSessionAuthenticated = isSessionAuthenticated(existingSessionValue);
     const isNewSessionAuthenticated = !!authenticationResult.user;
 
     const providerHasChanged = !!existingSessionValue && !ownsSession;
@@ -637,7 +641,7 @@ export class Authenticator {
     //  4. Request isn't attributed with HTTP Authorization header
     return (
       canRedirectRequest(request) &&
-      !sessionValue &&
+      !isSessionAuthenticated(sessionValue) &&
       this.options.config.authc.selector.enabled &&
       HTTPAuthorizationHeader.parseFromRequest(request) == null
     );
@@ -688,14 +692,14 @@ export class Authenticator {
       return authenticationResult;
     }
 
-    const isSessionAuthenticated = !!sessionUpdateResult?.value?.username;
+    const isUpdatedSessionAuthenticated = isSessionAuthenticated(sessionUpdateResult?.value);
 
     let preAccessRedirectURL;
-    if (isSessionAuthenticated && sessionUpdateResult?.overwritten) {
+    if (isUpdatedSessionAuthenticated && sessionUpdateResult?.overwritten) {
       this.logger.debug('Redirecting user to the overwritten session UI.');
       preAccessRedirectURL = `${this.options.basePath.serverBasePath}${OVERWRITTEN_SESSION_ROUTE}`;
     } else if (
-      isSessionAuthenticated &&
+      isUpdatedSessionAuthenticated &&
       this.shouldRedirectToAccessAgreement(sessionUpdateResult?.value ?? null)
     ) {
       this.logger.debug('Redirecting user to the access agreement UI.');

x-pack/scripts/functional_tests.js

@@ -31,16 +31,16 @@ const onlyNotInCoverageTests = [
   require.resolve('../test/plugin_api_integration/config.ts'),
   require.resolve('../test/kerberos_api_integration/config.ts'),
   require.resolve('../test/kerberos_api_integration/anonymous_access.config.ts'),
-  require.resolve('../test/saml_api_integration/config.ts'),
+  require.resolve('../test/security_api_integration/saml.config.ts'),
   require.resolve('../test/security_api_integration/session_idle.config.ts'),
   require.resolve('../test/security_api_integration/session_lifespan.config.ts'),
+  require.resolve('../test/security_api_integration/login_selector.config.ts'),
   require.resolve('../test/token_api_integration/config.js'),
   require.resolve('../test/oidc_api_integration/config.ts'),
   require.resolve('../test/oidc_api_integration/implicit_flow.config.ts'),
   require.resolve('../test/observability_api_integration/basic/config.ts'),
   require.resolve('../test/observability_api_integration/trial/config.ts'),
   require.resolve('../test/pki_api_integration/config.ts'),
-  require.resolve('../test/login_selector_api_integration/config.ts'),
   require.resolve('../test/encrypted_saved_objects_api_integration/config.ts'),
   require.resolve('../test/spaces_api_integration/spaces_only/config.ts'),
   require.resolve('../test/spaces_api_integration/security_and_spaces/config_trial.ts'),

x-pack/test/login_selector_api_integration/ftr_provider_context.d.ts

@@ -1,11 +0,0 @@
-/*
- * 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 { GenericFtrProviderContext } from '@kbn/test/types/ftr';
-
-import { services } from './services';
-
-export type FtrProviderContext = GenericFtrProviderContext<typeof services, {}>;

x-pack/test/login_selector_api_integration/services.ts

@@ -1,14 +0,0 @@
-/*
- * 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 { services as commonServices } from '../common/services';
-import { services as apiIntegrationServices } from '../api_integration/services';
-
-export const services = {
-  ...commonServices,
-  randomness: apiIntegrationServices.randomness,
-  supertestWithoutAuth: apiIntegrationServices.supertestWithoutAuth,
-};

x-pack/test/saml_api_integration/apis/index.ts

@@ -1,14 +0,0 @@
-/*
- * 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 { FtrProviderContext } from '../ftr_provider_context';
-
-export default function ({ loadTestFile }: FtrProviderContext) {
-  describe('apis SAML', function () {
-    this.tags('ciGroup6');
-    loadTestFile(require.resolve('./security'));
-  });
-}

x-pack/test/saml_api_integration/ftr_provider_context.d.ts

@@ -1,11 +0,0 @@
-/*
- * 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 { GenericFtrProviderContext } from '@kbn/test/types/ftr';
-
-import { services } from './services';
-
-export type FtrProviderContext = GenericFtrProviderContext<typeof services, {}>;

x-pack/test/saml_api_integration/services.ts

@@ -1,15 +0,0 @@
-/*
- * 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 { services as commonServices } from '../common/services';
-import { services as apiIntegrationServices } from '../api_integration/services';
-
-export const services = {
-  ...commonServices,
-  randomness: apiIntegrationServices.randomness,
-  legacyEs: apiIntegrationServices.legacyEs,
-  supertestWithoutAuth: apiIntegrationServices.supertestWithoutAuth,
-};

x-pack/test/security_api_integration/fixtures/saml/saml_provider/server/index.ts

@@ -4,7 +4,7 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import { PluginInitializer } from '../../../../../../src/core/server';
+import { PluginInitializer } from '../../../../../../../src/core/server';
 import { initRoutes } from './init_routes';
 
 export const plugin: PluginInitializer<void, void> = () => ({

x-pack/test/security_api_integration/fixtures/saml/saml_provider/server/init_routes.ts

@@ -4,7 +4,7 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import { CoreSetup } from '../../../../../../src/core/server';
+import { CoreSetup } from '../../../../../../../src/core/server';
 import { getSAMLResponse, getSAMLRequestId } from '../../saml_tools';
 
 export function initRoutes(core: CoreSetup) {

x-pack/test/security_api_integration/login_selector.config.ts

@@ -23,14 +23,8 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) {
 
   const pkiKibanaCAPath = resolve(__dirname, '../pki_api_integration/fixtures/kibana_ca.crt');
 
-  const saml1IdPMetadataPath = resolve(
-    __dirname,
-    '../saml_api_integration/fixtures/idp_metadata.xml'
-  );
-  const saml2IdPMetadataPath = resolve(
-    __dirname,
-    '../saml_api_integration/fixtures/idp_metadata_2.xml'
-  );
+  const saml1IdPMetadataPath = resolve(__dirname, './fixtures/saml/idp_metadata.xml');
+  const saml2IdPMetadataPath = resolve(__dirname, './fixtures/saml/idp_metadata_2.xml');
 
   const servers = {
     ...xPackAPITestsConfig.get('servers'),
@@ -45,7 +39,7 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) {
   };
 
   return {
-    testFiles: [require.resolve('./apis')],
+    testFiles: [require.resolve('./tests/login_selector')],
     servers,
     security: { disableTestUser: true },
     services: {
@@ -54,7 +48,7 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) {
       supertestWithoutAuth: xPackAPITestsConfig.get('services.supertestWithoutAuth'),
     },
     junit: {
-      reportName: 'X-Pack Login Selector API Integration Tests',
+      reportName: 'X-Pack Security API Integration Tests (Login Selector)',
     },
 
     esTestCluster: {

x-pack/test/security_api_integration/saml.config.ts

@@ -14,10 +14,10 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) {
   const xPackAPITestsConfig = await readConfigFile(require.resolve('../api_integration/config.ts'));
 
   const kibanaPort = xPackAPITestsConfig.get('servers.kibana.port');
-  const idpPath = resolve(__dirname, '../../test/saml_api_integration/fixtures/idp_metadata.xml');
+  const idpPath = resolve(__dirname, './fixtures/saml/idp_metadata.xml');
 
   return {
-    testFiles: [require.resolve('./apis')],
+    testFiles: [require.resolve('./tests/saml')],
     servers: xPackAPITestsConfig.get('servers'),
     security: { disableTestUser: true },
     services: {
@@ -26,7 +26,7 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) {
       supertestWithoutAuth: xPackAPITestsConfig.get('services.supertestWithoutAuth'),
     },
     junit: {
-      reportName: 'X-Pack SAML API Integration Tests',
+      reportName: 'X-Pack Security API Integration Tests (SAML)',
     },
 
     esTestCluster: {

x-pack/test/security_api_integration/services.ts

@@ -9,6 +9,5 @@ import { services as apiIntegrationServices } from '../api_integration/services'
 
 export const services = {
   ...commonServices,
-  legacyEs: apiIntegrationServices.legacyEs,
   supertestWithoutAuth: apiIntegrationServices.supertestWithoutAuth,
 };

x-pack/test/security_api_integration/tests/login_selector/basic_functionality.ts

@@ -10,13 +10,13 @@ import { resolve } from 'path';
 import url from 'url';
 import { CA_CERT_PATH } from '@kbn/dev-utils';
 import expect from '@kbn/expect';
-import { getStateAndNonce } from '../../oidc_api_integration/fixtures/oidc_tools';
+import { getStateAndNonce } from '../../../oidc_api_integration/fixtures/oidc_tools';
 import {
   getMutualAuthenticationResponseToken,
   getSPNEGOToken,
-} from '../../kerberos_api_integration/fixtures/kerberos_tools';
-import { getSAMLRequestId, getSAMLResponse } from '../../saml_api_integration/fixtures/saml_tools';
-import { FtrProviderContext } from '../ftr_provider_context';
+} from '../../../kerberos_api_integration/fixtures/kerberos_tools';
+import { getSAMLRequestId, getSAMLResponse } from '../../fixtures/saml/saml_tools';
+import { FtrProviderContext } from '../../ftr_provider_context';
 
 export default function ({ getService }: FtrProviderContext) {
   const randomness = getService('randomness');
@@ -29,7 +29,7 @@ export default function ({ getService }: FtrProviderContext) {
 
   const CA_CERT = readFileSync(CA_CERT_PATH);
   const CLIENT_CERT = readFileSync(
-    resolve(__dirname, '../../pki_api_integration/fixtures/first_client.p12')
+    resolve(__dirname, '../../../pki_api_integration/fixtures/first_client.p12')
   );
 
   async function checkSessionCookie(
@@ -97,11 +97,23 @@ export default function ({ getService }: FtrProviderContext) {
       // to fully authenticate user yet.
       const intermediateAuthCookie = request.cookie(handshakeResponse.headers['set-cookie'][0])!;
 
+      // When login page is accessed directly.
       await supertest
         .get('/login')
         .ca(CA_CERT)
         .set('Cookie', intermediateAuthCookie.cookieString())
         .expect(200);
+
+      // When user tries to access any other page in Kibana.
+      const response = await supertest
+        .get('/abc/xyz/handshake?one=two three')
+        .ca(CA_CERT)
+        .set('Cookie', intermediateAuthCookie.cookieString())
+        .expect(302);
+      expect(response.headers['set-cookie']).to.be(undefined);
+      expect(response.headers.location).to.be(
+        '/login?next=%2Fabc%2Fxyz%2Fhandshake%3Fone%3Dtwo%2520three'
+      );
     });
 
     describe('SAML', () => {

x-pack/test/security_api_integration/tests/login_selector/index.ts

@@ -4,11 +4,11 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import { FtrProviderContext } from '../ftr_provider_context';
+import { FtrProviderContext } from '../../ftr_provider_context';
 
 export default function ({ loadTestFile }: FtrProviderContext) {
-  describe('apis', function () {
+  describe('security APIs - Login Selector', function () {
     this.tags('ciGroup6');
-    loadTestFile(require.resolve('./login_selector'));
+    loadTestFile(require.resolve('./basic_functionality'));
   });
 }

x-pack/test/security_api_integration/tests/saml/index.ts

@@ -7,7 +7,9 @@
 import { FtrProviderContext } from '../../ftr_provider_context';
 
 export default function ({ loadTestFile }: FtrProviderContext) {
-  describe('security', () => {
+  describe('security APIs - SAML', function () {
+    this.tags('ciGroup6');
+
     loadTestFile(require.resolve('./saml_login'));
   });
 }

x-pack/test/security_api_integration/tests/saml/saml_login.ts

@@ -9,7 +9,11 @@ import url from 'url';
 import { delay } from 'bluebird';
 import expect from '@kbn/expect';
 import request, { Cookie } from 'request';
-import { getLogoutRequest, getSAMLRequestId, getSAMLResponse } from '../../fixtures/saml_tools';
+import {
+  getLogoutRequest,
+  getSAMLRequestId,
+  getSAMLResponse,
+} from '../../fixtures/saml/saml_tools';
 import { FtrProviderContext } from '../../ftr_provider_context';
 
 export default function ({ getService }: FtrProviderContext) {

x-pack/test/security_api_integration/tests/session_idle/cleanup.ts

@@ -30,7 +30,9 @@ export default function ({ getService }: FtrProviderContext) {
   }
 
   async function getNumberOfSessionDocuments() {
-    return (await es.search({ index: '.kibana_security_session*' })).hits.total.value;
+    return (((await es.search({ index: '.kibana_security_session*' })).hits.total as unknown) as {
+      value: number;
+    }).value;
   }
 
   describe('Session Idle cleanup', () => {

x-pack/test/security_api_integration/tests/session_lifespan/cleanup.ts

@@ -27,7 +27,9 @@ export default function ({ getService }: FtrProviderContext) {
   }
 
   async function getNumberOfSessionDocuments() {
-    return (await es.search({ index: '.kibana_security_session*' })).hits.total.value;
+    return (((await es.search({ index: '.kibana_security_session*' })).hits.total as unknown) as {
+      value: number;
+    }).value;
   }
 
   describe('Session Lifespan cleanup', () => {

x-pack/test/security_functional/login_selector.config.ts

@@ -20,8 +20,14 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) {
   );
 
   const kibanaPort = kibanaFunctionalConfig.get('servers.kibana.port');
-  const idpPath = resolve(__dirname, '../saml_api_integration/fixtures/saml_provider/metadata.xml');
-  const samlIdPPlugin = resolve(__dirname, '../saml_api_integration/fixtures/saml_provider');
+  const idpPath = resolve(
+    __dirname,
+    '../security_api_integration/fixtures/saml/saml_provider/metadata.xml'
+  );
+  const samlIdPPlugin = resolve(
+    __dirname,
+    '../security_api_integration/fixtures/saml/saml_provider'
+  );
 
   return {
     testFiles: [resolve(__dirname, './tests/login_selector')],

x-pack/test/security_functional/saml.config.ts

@@ -20,8 +20,14 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) {
   );
 
   const kibanaPort = kibanaFunctionalConfig.get('servers.kibana.port');
-  const idpPath = resolve(__dirname, '../saml_api_integration/fixtures/saml_provider/metadata.xml');
-  const samlIdPPlugin = resolve(__dirname, '../saml_api_integration/fixtures/saml_provider');
+  const idpPath = resolve(
+    __dirname,
+    '../security_api_integration/fixtures/saml/saml_provider/metadata.xml'
+  );
+  const samlIdPPlugin = resolve(
+    __dirname,
+    '../security_api_integration/fixtures/saml/saml_provider'
+  );
 
   return {
     testFiles: [resolve(__dirname, './tests/saml')],