Redirect to Logged Out UI on SAML Logout Response. Prefer Login Selec…

…tor UI to Logged Out UI whenever possible.
elastic · Jun 23, 2020 · f5bdc10 · f5bdc10
1 parent cc4c172
commit f5bdc10
Show file tree

Hide file tree

Showing 15 changed files with 124 additions and 72 deletions.
diff --git a/x-pack/plugins/security/server/authentication/authenticator.test.ts b/x-pack/plugins/security/server/authentication/authenticator.test.ts
@@ -112,6 +112,33 @@ describe('Authenticator', () => {
       ).toThrowError('Provider name "__http__" is reserved.');
     });
 
+    it('properly set `loggedOut` URL.', () => {
+      const basicAuthenticationProviderMock = jest.requireMock('./providers/basic')
+        .BasicAuthenticationProvider;
+
+      basicAuthenticationProviderMock.mockClear();
+      new Authenticator(getMockOptions());
+      expect(basicAuthenticationProviderMock).toHaveBeenCalledWith(
+        expect.objectContaining({
+          urls: {
+            loggedOut: '/mock-server-basepath/security/logged_out',
+          },
+        }),
+        expect.anything()
+      );
+
+      basicAuthenticationProviderMock.mockClear();
+      new Authenticator(getMockOptions({ selector: { enabled: true } }));
+      expect(basicAuthenticationProviderMock).toHaveBeenCalledWith(
+        expect.objectContaining({
+          urls: {
+            loggedOut: `/mock-server-basepath/login?msg=LOGGED_OUT`,
+          },
+        }),
+        expect.anything()
+      );
+    });
+
     describe('HTTP authentication provider', () => {
       beforeEach(() => {
         jest

diff --git a/x-pack/plugins/security/server/authentication/authenticator.ts b/x-pack/plugins/security/server/authentication/authenticator.ts
@@ -242,6 +242,11 @@ export class Authenticator {
         client: this.options.clusterClient,
         logger: this.options.loggers.get('tokens'),
       }),
+      urls: {
+        loggedOut: options.config.authc.selector.enabled
+          ? `${options.basePath.serverBasePath}/login?msg=LOGGED_OUT`
+          : `${options.basePath.serverBasePath}/security/logged_out`,
+      },
     };
 
     this.providers = new Map(

diff --git a/x-pack/plugins/security/server/authentication/providers/base.mock.ts b/x-pack/plugins/security/server/authentication/providers/base.mock.ts
@@ -15,14 +15,14 @@ export type MockAuthenticationProviderOptions = ReturnType<
 >;
 
 export function mockAuthenticationProviderOptions(options?: { name: string }) {
-  const basePath = httpServiceMock.createSetupContract().basePath;
-  basePath.get.mockReturnValue('/base-path');
-
   return {
     client: elasticsearchServiceMock.createClusterClient(),
     logger: loggingServiceMock.create().get(),
-    basePath,
+    basePath: httpServiceMock.createBasePath(),
     tokens: { refresh: jest.fn(), invalidate: jest.fn() },
     name: options?.name ?? 'basic1',
+    urls: {
+      loggedOut: '/mock-server-basepath/security/logged_out',
+    },
   };
 }
diff --git a/x-pack/plugins/security/server/authentication/providers/base.ts b/x-pack/plugins/security/server/authentication/providers/base.ts
@@ -26,6 +26,9 @@ export interface AuthenticationProviderOptions {
   client: IClusterClient;
   logger: Logger;
   tokens: PublicMethodsOf<Tokens>;
+  urls: {
+    loggedOut: string;
+  };
 }
 
 /**

diff --git a/x-pack/plugins/security/server/authentication/providers/basic.test.ts b/x-pack/plugins/security/server/authentication/providers/basic.test.ts
@@ -107,7 +107,7 @@ describe('BasicAuthenticationProvider', () => {
         )
       ).resolves.toEqual(
         AuthenticationResult.redirectTo(
-          '/base-path/login?next=%2Fbase-path%2Fs%2Ffoo%2Fsome-path%20%23%20that%20needs%20to%20be%20encoded'
+          '/mock-server-basepath/login?next=%2Fmock-server-basepath%2Fs%2Ffoo%2Fsome-path%20%23%20that%20needs%20to%20be%20encoded'
         )
       );
     });
@@ -186,7 +186,7 @@ describe('BasicAuthenticationProvider', () => {
 
     it('always redirects to the login page.', async () => {
       await expect(provider.logout(httpServerMock.createKibanaRequest(), {})).resolves.toEqual(
-        DeauthenticationResult.redirectTo('/base-path/login?msg=LOGGED_OUT')
+        DeauthenticationResult.redirectTo('/mock-server-basepath/login?msg=LOGGED_OUT')
       );
     });
 
@@ -199,7 +199,9 @@ describe('BasicAuthenticationProvider', () => {
           {}
         )
       ).resolves.toEqual(
-        DeauthenticationResult.redirectTo('/base-path/login?next=%2Fapp%2Fml&msg=SESSION_EXPIRED')
+        DeauthenticationResult.redirectTo(
+          '/mock-server-basepath/login?next=%2Fapp%2Fml&msg=SESSION_EXPIRED'
+        )
       );
     });
   });

diff --git a/x-pack/plugins/security/server/authentication/providers/kerberos.test.ts b/x-pack/plugins/security/server/authentication/providers/kerberos.test.ts
@@ -518,7 +518,7 @@ describe('KerberosAuthenticationProvider', () => {
       expect(mockOptions.tokens.invalidate).toHaveBeenCalledWith(tokenPair);
     });
 
-    it('redirects to `/logged_out` page if tokens are invalidated successfully.', async () => {
+    it('redirects to `loggedOut` URL if tokens are invalidated successfully.', async () => {
       const request = httpServerMock.createKibanaRequest();
       const tokenPair = {
         accessToken: 'some-valid-token',
@@ -528,7 +528,7 @@ describe('KerberosAuthenticationProvider', () => {
       mockOptions.tokens.invalidate.mockResolvedValue(undefined);
 
       await expect(provider.logout(request, tokenPair)).resolves.toEqual(
-        DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out')
+        DeauthenticationResult.redirectTo(mockOptions.urls.loggedOut)
       );
 
       expect(mockOptions.tokens.invalidate).toHaveBeenCalledTimes(1);

diff --git a/x-pack/plugins/security/server/authentication/providers/kerberos.ts b/x-pack/plugins/security/server/authentication/providers/kerberos.ts
@@ -114,9 +114,7 @@ export class KerberosAuthenticationProvider extends BaseAuthenticationProvider {
       return DeauthenticationResult.failed(err);
     }
 
-    return DeauthenticationResult.redirectTo(
-      `${this.options.basePath.serverBasePath}/security/logged_out`
-    );
+    return DeauthenticationResult.redirectTo(this.options.urls.loggedOut);
   }
 
   /**

diff --git a/x-pack/plugins/security/server/authentication/providers/oidc.test.ts b/x-pack/plugins/security/server/authentication/providers/oidc.test.ts
@@ -353,7 +353,7 @@ describe('OIDCAuthenticationProvider', () => {
             state: {
               state: 'statevalue',
               nonce: 'noncevalue',
-              nextURL: '/base-path/s/foo/some-path',
+              nextURL: '/mock-server-basepath/s/foo/some-path',
               realm: 'oidc1',
             },
           }
@@ -575,7 +575,7 @@ describe('OIDCAuthenticationProvider', () => {
             state: {
               state: 'statevalue',
               nonce: 'noncevalue',
-              nextURL: '/base-path/s/foo/some-path',
+              nextURL: '/mock-server-basepath/s/foo/some-path',
               realm: 'oidc1',
             },
           }
@@ -702,7 +702,7 @@ describe('OIDCAuthenticationProvider', () => {
       });
     });
 
-    it('redirects to /logged_out if `redirect` field in OpenID Connect logout response is null.', async () => {
+    it('redirects to `loggedOut` URL if `redirect` field in OpenID Connect logout response is null.', async () => {
       const request = httpServerMock.createKibanaRequest();
       const accessToken = 'x-oidc-token';
       const refreshToken = 'x-oidc-refresh-token';
@@ -711,9 +711,7 @@ describe('OIDCAuthenticationProvider', () => {
 
       await expect(
         provider.logout(request, { accessToken, refreshToken, realm: 'oidc1' })
-      ).resolves.toEqual(
-        DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out')
-      );
+      ).resolves.toEqual(DeauthenticationResult.redirectTo(mockOptions.urls.loggedOut));
 
       expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledTimes(1);
       expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledWith('shield.oidcLogout', {

diff --git a/x-pack/plugins/security/server/authentication/providers/oidc.ts b/x-pack/plugins/security/server/authentication/providers/oidc.ts
@@ -433,9 +433,7 @@ export class OIDCAuthenticationProvider extends BaseAuthenticationProvider {
         return DeauthenticationResult.redirectTo(redirect);
       }
 
-      return DeauthenticationResult.redirectTo(
-        `${this.options.basePath.serverBasePath}/security/logged_out`
-      );
+      return DeauthenticationResult.redirectTo(this.options.urls.loggedOut);
     } catch (err) {
       this.logger.debug(`Failed to deauthenticate user: ${err.message}`);
       return DeauthenticationResult.failed(err);

diff --git a/x-pack/plugins/security/server/authentication/providers/pki.test.ts b/x-pack/plugins/security/server/authentication/providers/pki.test.ts
@@ -547,14 +547,14 @@ describe('PKIAuthenticationProvider', () => {
       expect(mockOptions.tokens.invalidate).toHaveBeenCalledWith({ accessToken: 'foo' });
     });
 
-    it('redirects to `/logged_out` page if access token is invalidated successfully.', async () => {
+    it('redirects to `loggedOut` URL if access token is invalidated successfully.', async () => {
       const request = httpServerMock.createKibanaRequest();
       const state = { accessToken: 'foo', peerCertificateFingerprint256: '2A:7A:C2:DD' };
 
       mockOptions.tokens.invalidate.mockResolvedValue(undefined);
 
       await expect(provider.logout(request, state)).resolves.toEqual(
-        DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out')
+        DeauthenticationResult.redirectTo(mockOptions.urls.loggedOut)
       );
 
       expect(mockOptions.tokens.invalidate).toHaveBeenCalledTimes(1);

diff --git a/x-pack/plugins/security/server/authentication/providers/pki.ts b/x-pack/plugins/security/server/authentication/providers/pki.ts
@@ -119,9 +119,7 @@ export class PKIAuthenticationProvider extends BaseAuthenticationProvider {
       return DeauthenticationResult.failed(err);
     }
 
-    return DeauthenticationResult.redirectTo(
-      `${this.options.basePath.serverBasePath}/security/logged_out`
-    );
+    return DeauthenticationResult.redirectTo(this.options.urls.loggedOut);
   }
 
   /**

diff --git a/x-pack/plugins/security/server/authentication/providers/saml.test.ts b/x-pack/plugins/security/server/authentication/providers/saml.test.ts
@@ -163,7 +163,7 @@ describe('SAMLAuthenticationProvider', () => {
           { requestId: 'some-request-id', redirectURL: '', realm: 'test-realm' }
         )
       ).resolves.toEqual(
-        AuthenticationResult.redirectTo('/base-path/', {
+        AuthenticationResult.redirectTo('/mock-server-basepath/', {
           state: {
             accessToken: 'user-initiated-login-token',
             refreshToken: 'user-initiated-login-refresh-token',
@@ -192,7 +192,7 @@ describe('SAMLAuthenticationProvider', () => {
           samlResponse: 'saml-response-xml',
         })
       ).resolves.toEqual(
-        AuthenticationResult.redirectTo('/base-path/', {
+        AuthenticationResult.redirectTo('/mock-server-basepath/', {
           state: {
             accessToken: 'idp-initiated-login-token',
             refreshToken: 'idp-initiated-login-refresh-token',
@@ -351,7 +351,7 @@ describe('SAMLAuthenticationProvider', () => {
               state
             )
           ).resolves.toEqual(
-            AuthenticationResult.redirectTo('/base-path/', {
+            AuthenticationResult.redirectTo('/mock-server-basepath/', {
               state: {
                 username: 'user',
                 accessToken: 'new-valid-token',
@@ -723,7 +723,7 @@ describe('SAMLAuthenticationProvider', () => {
       await expect(provider.authenticate(request)).resolves.toEqual(
         AuthenticationResult.redirectTo(
           '/mock-server-basepath/internal/security/saml/capture-url-fragment',
-          { state: { redirectURL: '/base-path/s/foo/some-path', realm: 'test-realm' } }
+          { state: { redirectURL: '/mock-server-basepath/s/foo/some-path', realm: 'test-realm' } }
         )
       );
 
@@ -753,7 +753,7 @@ describe('SAMLAuthenticationProvider', () => {
 
       expect(mockOptions.logger.warn).toHaveBeenCalledTimes(1);
       expect(mockOptions.logger.warn).toHaveBeenCalledWith(
-        'Max URL path size should not exceed 100b but it was 107b. URL is not captured.'
+        'Max URL path size should not exceed 100b but it was 118b. URL is not captured.'
       );
     });
 
@@ -998,7 +998,7 @@ describe('SAMLAuthenticationProvider', () => {
       await expect(provider.authenticate(request, state)).resolves.toEqual(
         AuthenticationResult.redirectTo(
           '/mock-server-basepath/internal/security/saml/capture-url-fragment',
-          { state: { redirectURL: '/base-path/s/foo/some-path', realm: 'test-realm' } }
+          { state: { redirectURL: '/mock-server-basepath/s/foo/some-path', realm: 'test-realm' } }
         )
       );
 
@@ -1054,7 +1054,7 @@ describe('SAMLAuthenticationProvider', () => {
 
       expect(mockOptions.logger.warn).toHaveBeenCalledTimes(1);
       expect(mockOptions.logger.warn).toHaveBeenCalledWith(
-        'Max URL path size should not exceed 100b but it was 107b. URL is not captured.'
+        'Max URL path size should not exceed 100b but it was 118b. URL is not captured.'
       );
     });
 
@@ -1124,7 +1124,7 @@ describe('SAMLAuthenticationProvider', () => {
       });
     });
 
-    it('redirects to /security/logged_out if `redirect` field in SAML logout response is null.', async () => {
+    it('redirects to `loggedOut` URL if `redirect` field in SAML logout response is null.', async () => {
       const request = httpServerMock.createKibanaRequest();
       const accessToken = 'x-saml-token';
       const refreshToken = 'x-saml-refresh-token';
@@ -1138,17 +1138,15 @@ describe('SAMLAuthenticationProvider', () => {
           refreshToken,
           realm: 'test-realm',
         })
-      ).resolves.toEqual(
-        DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out')
-      );
+      ).resolves.toEqual(DeauthenticationResult.redirectTo(mockOptions.urls.loggedOut));
 
       expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledTimes(1);
       expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledWith('shield.samlLogout', {
         body: { token: accessToken, refresh_token: refreshToken },
       });
     });
 
-    it('redirects to /security/logged_out if `redirect` field in SAML logout response is not defined.', async () => {
+    it('redirects to `loggedOut` URL if `redirect` field in SAML logout response is not defined.', async () => {
       const request = httpServerMock.createKibanaRequest();
       const accessToken = 'x-saml-token';
       const refreshToken = 'x-saml-refresh-token';
@@ -1162,9 +1160,7 @@ describe('SAMLAuthenticationProvider', () => {
           refreshToken,
           realm: 'test-realm',
         })
-      ).resolves.toEqual(
-        DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out')
-      );
+      ).resolves.toEqual(DeauthenticationResult.redirectTo(mockOptions.urls.loggedOut));
 
       expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledTimes(1);
       expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledWith('shield.samlLogout', {
@@ -1174,7 +1170,7 @@ describe('SAMLAuthenticationProvider', () => {
 
     it('relies on SAML logout if query string is not empty, but does not include SAMLRequest.', async () => {
       const request = httpServerMock.createKibanaRequest({
-        query: { Whatever: 'something unrelated' },
+        query: { Whatever: 'something unrelated', SAMLResponse: 'xxx yyy' },
       });
       const accessToken = 'x-saml-token';
       const refreshToken = 'x-saml-refresh-token';
@@ -1188,9 +1184,7 @@ describe('SAMLAuthenticationProvider', () => {
           refreshToken,
           realm: 'test-realm',
         })
-      ).resolves.toEqual(
-        DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out')
-      );
+      ).resolves.toEqual(DeauthenticationResult.redirectTo(mockOptions.urls.loggedOut));
 
       expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledTimes(1);
       expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledWith('shield.samlLogout', {
@@ -1210,23 +1204,21 @@ describe('SAMLAuthenticationProvider', () => {
           refreshToken: 'x-saml-refresh-token',
           realm: 'test-realm',
         })
-      ).resolves.toEqual(
-        DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out')
-      );
+      ).resolves.toEqual(DeauthenticationResult.redirectTo(mockOptions.urls.loggedOut));
 
       expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledTimes(1);
       expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledWith('shield.samlInvalidate', {
         body: { queryString: 'SAMLRequest=xxx%20yyy', realm: 'test-realm' },
       });
     });
 
-    it('redirects to /security/logged_out if `redirect` field in SAML invalidate response is null.', async () => {
+    it('redirects to `loggedOut` URL if `redirect` field in SAML invalidate response is null.', async () => {
       const request = httpServerMock.createKibanaRequest({ query: { SAMLRequest: 'xxx yyy' } });
 
       mockOptions.client.callAsInternalUser.mockResolvedValue({ redirect: null });
 
       await expect(provider.logout(request)).resolves.toEqual(
-        DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out')
+        DeauthenticationResult.redirectTo(mockOptions.urls.loggedOut)
       );
 
       expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledTimes(1);
@@ -1235,13 +1227,13 @@ describe('SAMLAuthenticationProvider', () => {
       });
     });
 
-    it('redirects to /security/logged_out if `redirect` field in SAML invalidate response is not defined.', async () => {
+    it('redirects to `loggedOut` URL if `redirect` field in SAML invalidate response is not defined.', async () => {
       const request = httpServerMock.createKibanaRequest({ query: { SAMLRequest: 'xxx yyy' } });
 
       mockOptions.client.callAsInternalUser.mockResolvedValue({ redirect: undefined });
 
       await expect(provider.logout(request)).resolves.toEqual(
-        DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out')
+        DeauthenticationResult.redirectTo(mockOptions.urls.loggedOut)
       );
 
       expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledTimes(1);
@@ -1250,6 +1242,16 @@ describe('SAMLAuthenticationProvider', () => {
       });
     });
 
+    it('redirects to `loggedOut` URL if SAML logout response is received.', async () => {
+      const request = httpServerMock.createKibanaRequest({ query: { SAMLResponse: 'xxx yyy' } });
+
+      await expect(provider.logout(request)).resolves.toEqual(
+        DeauthenticationResult.redirectTo(mockOptions.urls.loggedOut)
+      );
+
+      expect(mockOptions.client.callAsInternalUser).not.toHaveBeenCalled();
+    });
+
     it('redirects user to the IdP if SLO is supported by IdP in case of SP initiated logout.', async () => {
       const request = httpServerMock.createKibanaRequest();
       const accessToken = 'x-saml-token';