[7.8] Fix authentication loop when upgrading Kibana from 6.8 to 7.7+ (#…

…67687) (#67983)
elastic · Jun 2, 2020 · 173845d · 173845d
1 parent 3fc974b
commit 173845d
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 5 deletions.
diff --git a/x-pack/plugins/security/server/authentication/authenticator.test.ts b/x-pack/plugins/security/server/authentication/authenticator.test.ts
@@ -540,6 +540,29 @@ describe('Authenticator', () => {
       expect(mockSessionStorage.set).not.toHaveBeenCalled();
       expect(mockSessionStorage.clear).toHaveBeenCalled();
     });
+
+    it('clears legacy 6.8 session.', async () => {
+      const user = mockAuthenticatedUser();
+      const request = httpServerMock.createKibanaRequest();
+
+      // Use string format for the `provider` session value field and wrap state/provider in value object to emulate legacy 6.8 session.
+      mockSessionStorage.get.mockResolvedValue({
+        value: { state: mockSessVal.state, provider: 'basic' },
+        expires: null,
+      } as any);
+
+      mockBasicAuthenticationProvider.login.mockResolvedValue(AuthenticationResult.succeeded(user));
+
+      await expect(
+        authenticator.login(request, { provider: { type: 'basic' }, value: {} })
+      ).resolves.toEqual(AuthenticationResult.succeeded(user));
+
+      expect(mockBasicAuthenticationProvider.login).toHaveBeenCalledTimes(1);
+      expect(mockBasicAuthenticationProvider.login).toHaveBeenCalledWith(request, {}, null);
+
+      expect(mockSessionStorage.set).not.toHaveBeenCalled();
+      expect(mockSessionStorage.clear).toHaveBeenCalled();
+    });
   });
 
   describe('`authenticate` method', () => {
@@ -986,6 +1009,31 @@ describe('Authenticator', () => {
       expect(mockSessionStorage.clear).toHaveBeenCalled();
     });
 
+    it('clears legacy 6.8 session.', async () => {
+      const user = mockAuthenticatedUser();
+      const request = httpServerMock.createKibanaRequest();
+
+      // Use string format for the `provider` session value field and wrap state/provider in value object to emulate legacy 6.8 session.
+      mockSessionStorage.get.mockResolvedValue({
+        value: { state: mockSessVal.state, provider: 'basic' },
+        expires: null,
+      } as any);
+
+      mockBasicAuthenticationProvider.authenticate.mockResolvedValue(
+        AuthenticationResult.succeeded(user)
+      );
+
+      await expect(authenticator.authenticate(request)).resolves.toEqual(
+        AuthenticationResult.succeeded(user)
+      );
+
+      expect(mockBasicAuthenticationProvider.authenticate).toHaveBeenCalledTimes(1);
+      expect(mockBasicAuthenticationProvider.authenticate).toHaveBeenCalledWith(request, null);
+
+      expect(mockSessionStorage.set).not.toHaveBeenCalled();
+      expect(mockSessionStorage.clear).toHaveBeenCalled();
+    });
+
     it('does not clear session if provider can not handle system API request authentication with active session.', async () => {
       const request = httpServerMock.createKibanaRequest({
         headers: { 'kbn-system-request': 'true' },

diff --git a/x-pack/plugins/security/server/authentication/authenticator.ts b/x-pack/plugins/security/server/authentication/authenticator.ts
@@ -161,12 +161,11 @@ function isLoginAttemptWithProviderType(
 }
 
 /**
- * Determines if session value was created by the previous Kibana versions which had a different
- * session value format.
+ * Determines if session value was created by the current Kibana version. Previous versions had a different session value format.
  * @param sessionValue The session value to check.
  */
-function isLegacyProviderSession(sessionValue: any) {
-  return typeof sessionValue?.provider === 'string';
+function isSupportedProviderSession(sessionValue: any): sessionValue is ProviderSession {
+  return typeof sessionValue?.provider?.name === 'string';
 }
 
 /**
@@ -571,7 +570,7 @@ export class Authenticator {
     // we should clear session entirely.
     if (
       sessionValue &&
-      (isLegacyProviderSession(sessionValue) ||
+      (!isSupportedProviderSession(sessionValue) ||
         this.providers.get(sessionValue.provider.name)?.type !== sessionValue.provider.type)
     ) {
       sessionStorage.clear();