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

…tor UI to Logged Out UI whenever possible. (elastic#69676) # Conflicts: # x-pack/plugins/security/server/authentication/authenticator.ts # x-pack/plugins/security/server/authentication/providers/saml.test.ts # x-pack/plugins/security/server/authentication/providers/saml.ts
azasypkin · Jun 24, 2020 · f86cf69 · f86cf69
1 parent 7211305
commit f86cf69
Show file tree

Hide file tree

Showing 15 changed files with 125 additions and 73 deletions.
diff --git a/x-pack/plugins/security/server/authentication/authenticator.test.ts b/x-pack/plugins/security/server/authentication/authenticator.test.ts
@@ -113,6 +113,33 @@ describe('Authenticator', () => {
       ).toThrowError('Provider name "__http__" is reserved.');
     });
 
+    it('properly sets `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
@@ -243,6 +243,11 @@ export class Authenticator {
         logger: this.options.loggers.get('tokens'),
       }),
       getServerBaseURL: this.options.getServerBaseURL,
+      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,15 +15,15 @@ export type MockAuthenticationProviderOptions = ReturnType<
 >;
 
 export function mockAuthenticationProviderOptions(options?: { name: string }) {
-  const basePath = httpServiceMock.createSetupContract().basePath;
-  basePath.get.mockReturnValue('/base-path');
-
   return {
     getServerBaseURL: () => 'test-protocol://test-hostname:1234',
     client: elasticsearchServiceMock.createClusterClient(),
     logger: loggingSystemMock.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
@@ -27,6 +27,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);
   }
 
   /**