From a9c4e0656a63e4ee63b2f6e8f4e65dc092c7956e Mon Sep 17 00:00:00 2001
From: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Date: Fri, 20 Nov 2020 16:38:25 +0100
Subject: [PATCH] Review#2: support auth_provider_hint for providers that are
 hidden in Login Selector.

---
 x-pack/plugins/security/common/login_state.ts |   1 +
 .../components/login_form/login_form.test.tsx | 131 ++++++++++++++----
 .../components/login_form/login_form.tsx      |   7 +-
 .../server/routes/views/login.test.ts         |  78 ++++-------
 .../security/server/routes/views/login.ts     |  19 +--
 5 files changed, 143 insertions(+), 93 deletions(-)

diff --git a/x-pack/plugins/security/common/login_state.ts b/x-pack/plugins/security/common/login_state.ts
index fd2b1cb8d1cf..77edd1a4ea8d 100644
--- a/x-pack/plugins/security/common/login_state.ts
+++ b/x-pack/plugins/security/common/login_state.ts
@@ -10,6 +10,7 @@ export interface LoginSelectorProvider {
   type: string;
   name: string;
   usesLoginForm: boolean;
+  showInSelector: boolean;
   description?: string;
   hint?: string;
   icon?: string;
diff --git a/x-pack/plugins/security/public/authentication/login/components/login_form/login_form.test.tsx b/x-pack/plugins/security/public/authentication/login/components/login_form/login_form.test.tsx
index 5a1b5a05263e..2b67f2048488 100644
--- a/x-pack/plugins/security/public/authentication/login/components/login_form/login_form.test.tsx
+++ b/x-pack/plugins/security/public/authentication/login/components/login_form/login_form.test.tsx
@@ -74,7 +74,9 @@ describe('LoginForm', () => {
           loginAssistanceMessage=""
           selector={{
             enabled: false,
-            providers: [{ type: 'basic', name: 'basic', usesLoginForm: true }],
+            providers: [
+              { type: 'basic', name: 'basic', usesLoginForm: true, showInSelector: true },
+            ],
           }}
         />
       )
@@ -91,7 +93,7 @@ describe('LoginForm', () => {
         loginAssistanceMessage=""
         selector={{
           enabled: false,
-          providers: [{ type: 'basic', name: 'basic', usesLoginForm: true }],
+          providers: [{ type: 'basic', name: 'basic', usesLoginForm: true, showInSelector: true }],
         }}
       />
     );
@@ -111,7 +113,7 @@ describe('LoginForm', () => {
         loginAssistanceMessage=""
         selector={{
           enabled: false,
-          providers: [{ type: 'basic', name: 'basic', usesLoginForm: true }],
+          providers: [{ type: 'basic', name: 'basic', usesLoginForm: true, showInSelector: true }],
         }}
       />
     );
@@ -132,7 +134,7 @@ describe('LoginForm', () => {
         loginAssistanceMessage=""
         selector={{
           enabled: false,
-          providers: [{ type: 'basic', name: 'basic', usesLoginForm: true }],
+          providers: [{ type: 'basic', name: 'basic', usesLoginForm: true, showInSelector: true }],
         }}
       />
     );
@@ -164,7 +166,7 @@ describe('LoginForm', () => {
         loginAssistanceMessage=""
         selector={{
           enabled: false,
-          providers: [{ type: 'basic', name: 'basic', usesLoginForm: true }],
+          providers: [{ type: 'basic', name: 'basic', usesLoginForm: true, showInSelector: true }],
         }}
       />
     );
@@ -197,7 +199,7 @@ describe('LoginForm', () => {
         loginAssistanceMessage=""
         selector={{
           enabled: false,
-          providers: [{ type: 'basic', name: 'basic1', usesLoginForm: true }],
+          providers: [{ type: 'basic', name: 'basic1', usesLoginForm: true, showInSelector: true }],
         }}
       />
     );
@@ -239,7 +241,7 @@ describe('LoginForm', () => {
         loginHelp="**some help**"
         selector={{
           enabled: false,
-          providers: [{ type: 'basic', name: 'basic', usesLoginForm: true }],
+          providers: [{ type: 'basic', name: 'basic', usesLoginForm: true, showInSelector: true }],
         }}
       />
     );
@@ -278,14 +280,22 @@ describe('LoginForm', () => {
                 usesLoginForm: true,
                 hint: 'Basic hint',
                 icon: 'logoElastic',
+                showInSelector: true,
+              },
+              {
+                type: 'saml',
+                name: 'saml1',
+                description: 'Log in w/SAML',
+                usesLoginForm: false,
+                showInSelector: true,
               },
-              { type: 'saml', name: 'saml1', description: 'Log in w/SAML', usesLoginForm: false },
               {
                 type: 'pki',
                 name: 'pki1',
                 description: 'Log in w/PKI',
                 hint: 'PKI hint',
                 usesLoginForm: false,
+                showInSelector: true,
               },
             ],
           }}
@@ -326,8 +336,15 @@ describe('LoginForm', () => {
                 description: 'Login w/SAML',
                 hint: 'SAML hint',
                 usesLoginForm: false,
+                showInSelector: true,
+              },
+              {
+                type: 'pki',
+                name: 'pki1',
+                icon: 'some-icon',
+                usesLoginForm: false,
+                showInSelector: true,
               },
-              { type: 'pki', name: 'pki1', icon: 'some-icon', usesLoginForm: false },
             ],
           }}
         />
@@ -369,9 +386,21 @@ describe('LoginForm', () => {
           selector={{
             enabled: true,
             providers: [
-              { type: 'basic', name: 'basic', usesLoginForm: true },
-              { type: 'saml', name: 'saml1', description: 'Login w/SAML', usesLoginForm: false },
-              { type: 'pki', name: 'pki1', description: 'Login w/PKI', usesLoginForm: false },
+              { type: 'basic', name: 'basic', usesLoginForm: true, showInSelector: true },
+              {
+                type: 'saml',
+                name: 'saml1',
+                description: 'Login w/SAML',
+                usesLoginForm: false,
+                showInSelector: true,
+              },
+              {
+                type: 'pki',
+                name: 'pki1',
+                description: 'Login w/PKI',
+                usesLoginForm: false,
+                showInSelector: true,
+              },
             ],
           }}
         />
@@ -414,8 +443,8 @@ describe('LoginForm', () => {
           selector={{
             enabled: true,
             providers: [
-              { type: 'basic', name: 'basic', usesLoginForm: true },
-              { type: 'saml', name: 'saml1', usesLoginForm: false },
+              { type: 'basic', name: 'basic', usesLoginForm: true, showInSelector: true },
+              { type: 'saml', name: 'saml1', usesLoginForm: false, showInSelector: true },
             ],
           }}
         />
@@ -462,8 +491,8 @@ describe('LoginForm', () => {
           selector={{
             enabled: true,
             providers: [
-              { type: 'basic', name: 'basic', usesLoginForm: true },
-              { type: 'saml', name: 'saml1', usesLoginForm: false },
+              { type: 'basic', name: 'basic', usesLoginForm: true, showInSelector: true },
+              { type: 'saml', name: 'saml1', usesLoginForm: false, showInSelector: true },
             ],
           }}
         />
@@ -505,8 +534,8 @@ describe('LoginForm', () => {
           selector={{
             enabled: true,
             providers: [
-              { type: 'basic', name: 'basic', usesLoginForm: true },
-              { type: 'saml', name: 'saml1', usesLoginForm: false },
+              { type: 'basic', name: 'basic', usesLoginForm: true, showInSelector: true },
+              { type: 'saml', name: 'saml1', usesLoginForm: false, showInSelector: true },
             ],
           }}
         />
@@ -534,8 +563,8 @@ describe('LoginForm', () => {
           selector={{
             enabled: true,
             providers: [
-              { type: 'basic', name: 'basic', usesLoginForm: true },
-              { type: 'saml', name: 'saml1', usesLoginForm: false },
+              { type: 'basic', name: 'basic', usesLoginForm: true, showInSelector: true },
+              { type: 'saml', name: 'saml1', usesLoginForm: false, showInSelector: true },
             ],
           }}
         />
@@ -571,8 +600,8 @@ describe('LoginForm', () => {
           selector={{
             enabled: true,
             providers: [
-              { type: 'basic', name: 'basic', usesLoginForm: true },
-              { type: 'saml', name: 'saml1', usesLoginForm: false },
+              { type: 'basic', name: 'basic', usesLoginForm: true, showInSelector: true },
+              { type: 'saml', name: 'saml1', usesLoginForm: false, showInSelector: true },
             ],
           }}
         />
@@ -622,8 +651,8 @@ describe('LoginForm', () => {
           selector={{
             enabled: true,
             providers: [
-              { type: 'basic', name: 'basic1', usesLoginForm: true },
-              { type: 'saml', name: 'saml1', usesLoginForm: false },
+              { type: 'basic', name: 'basic1', usesLoginForm: true, showInSelector: true },
+              { type: 'saml', name: 'saml1', usesLoginForm: false, showInSelector: true },
             ],
           }}
         />
@@ -634,7 +663,51 @@ describe('LoginForm', () => {
       expect(findTestSubject(wrapper, 'loginAssistanceMessage').text()).toEqual('Need assistance?');
     });
 
-    it('automatically logs in if provider suggested by the auth provider hint does not need login form', async () => {
+    it('automatically logs in if provider suggested by the auth provider hint is displayed in the selector', async () => {
+      const currentURL = `https://some-host/login?next=${encodeURIComponent(
+        '/some-base-path/app/kibana#/home?_g=()'
+      )}`;
+      const coreStartMock = coreMock.createStart({ basePath: '/some-base-path' });
+      coreStartMock.http.post.mockResolvedValue({
+        location: 'https://external-idp/login?optional-arg=2#optional-hash',
+      });
+
+      window.location.href = currentURL;
+      const wrapper = mountWithIntl(
+        <LoginForm
+          http={coreStartMock.http}
+          notifications={coreStartMock.notifications}
+          loginHelp={'**Hey this is a login help message**'}
+          loginAssistanceMessage="Need assistance?"
+          authProviderHint="saml1"
+          selector={{
+            enabled: true,
+            providers: [
+              { type: 'basic', name: 'basic1', usesLoginForm: true, showInSelector: true },
+              { type: 'saml', name: 'saml1', usesLoginForm: false, showInSelector: true },
+            ],
+          }}
+        />
+      );
+
+      expectAutoLoginOverlay(wrapper);
+
+      await act(async () => {
+        await nextTick();
+        wrapper.update();
+      });
+
+      expect(coreStartMock.http.post).toHaveBeenCalledTimes(1);
+      expect(coreStartMock.http.post).toHaveBeenCalledWith('/internal/security/login', {
+        body: JSON.stringify({ providerType: 'saml', providerName: 'saml1', currentURL }),
+      });
+
+      expect(window.location.href).toBe('https://external-idp/login?optional-arg=2#optional-hash');
+      expect(wrapper.find(EuiCallOut).exists()).toBe(false);
+      expect(coreStartMock.notifications.toasts.addError).not.toHaveBeenCalled();
+    });
+
+    it('automatically logs in if provider suggested by the auth provider hint is not displayed in the selector', async () => {
       const currentURL = `https://some-host/login?next=${encodeURIComponent(
         '/some-base-path/app/kibana#/home?_g=()'
       )}`;
@@ -654,8 +727,8 @@ describe('LoginForm', () => {
           selector={{
             enabled: true,
             providers: [
-              { type: 'basic', name: 'basic1', usesLoginForm: true },
-              { type: 'saml', name: 'saml1', usesLoginForm: false },
+              { type: 'basic', name: 'basic1', usesLoginForm: true, showInSelector: true },
+              { type: 'saml', name: 'saml1', usesLoginForm: false, showInSelector: false },
             ],
           }}
         />
@@ -698,8 +771,8 @@ describe('LoginForm', () => {
           selector={{
             enabled: true,
             providers: [
-              { type: 'basic', name: 'basic1', usesLoginForm: true },
-              { type: 'saml', name: 'saml1', usesLoginForm: false },
+              { type: 'basic', name: 'basic1', usesLoginForm: true, showInSelector: true },
+              { type: 'saml', name: 'saml1', usesLoginForm: false, showInSelector: true },
             ],
           }}
         />
diff --git a/x-pack/plugins/security/public/authentication/login/components/login_form/login_form.tsx b/x-pack/plugins/security/public/authentication/login/components/login_form/login_form.tsx
index 9b16701990ae..e37d0024852d 100644
--- a/x-pack/plugins/security/public/authentication/login/components/login_form/login_form.tsx
+++ b/x-pack/plugins/security/public/authentication/login/components/login_form/login_form.tsx
@@ -283,9 +283,10 @@ export class LoginForm extends Component<Props, State> {
   };
 
   private renderSelector = () => {
+    const providers = this.props.selector.providers.filter((provider) => provider.showInSelector);
     return (
       <EuiPanel data-test-subj="loginSelector" paddingSize="none">
-        {this.props.selector.providers.map((provider) => (
+        {providers.map((provider) => (
           <button
             key={provider.name}
             data-test-subj={`loginCard-${provider.type}/${provider.name}`}
@@ -542,7 +543,9 @@ export class LoginForm extends Component<Props, State> {
   private showLoginSelector() {
     return (
       this.props.selector.enabled &&
-      this.props.selector.providers.some((provider) => !provider.usesLoginForm)
+      this.props.selector.providers.some(
+        (provider) => !provider.usesLoginForm && provider.showInSelector
+      )
     );
   }
 }
diff --git a/x-pack/plugins/security/server/routes/views/login.test.ts b/x-pack/plugins/security/server/routes/views/login.test.ts
index b90a44be7aad..11b2cdcac021 100644
--- a/x-pack/plugins/security/server/routes/views/login.test.ts
+++ b/x-pack/plugins/security/server/routes/views/login.test.ts
@@ -185,7 +185,7 @@ describe('Login view routes', () => {
         requiresSecureConnection: false,
         selector: {
           enabled: false,
-          providers: [{ name: 'basic', type: 'basic', usesLoginForm: true }],
+          providers: [{ name: 'basic', type: 'basic', usesLoginForm: true, showInSelector: true }],
         },
       };
       await expect(
@@ -209,7 +209,7 @@ describe('Login view routes', () => {
         requiresSecureConnection: false,
         selector: {
           enabled: false,
-          providers: [{ name: 'basic', type: 'basic', usesLoginForm: true }],
+          providers: [{ name: 'basic', type: 'basic', usesLoginForm: true, showInSelector: true }],
         },
       };
       await expect(
@@ -253,6 +253,7 @@ describe('Login view routes', () => {
               name: 'basic1',
               type: 'basic',
               usesLoginForm: true,
+              showInSelector: true,
               icon: 'logoElasticsearch',
               description: 'Log in with Elasticsearch',
             },
@@ -265,6 +266,7 @@ describe('Login view routes', () => {
               name: 'token1',
               type: 'token',
               usesLoginForm: true,
+              showInSelector: true,
               icon: 'logoElasticsearch',
               description: 'Log in with Elasticsearch',
             },
@@ -296,7 +298,7 @@ describe('Login view routes', () => {
       const contextMock = coreMock.createRequestHandlerContext();
 
       const cases: Array<[ConfigType['authc'], LoginSelectorProvider[]]> = [
-        // selector is disabled, multiple providers, but only basic provider should be returned.
+        // selector is disabled, multiple providers, all providers should be returned.
         [
           getAuthcConfig({
             selector: { enabled: false },
@@ -310,9 +312,16 @@ describe('Login view routes', () => {
               name: 'basic1',
               type: 'basic',
               usesLoginForm: true,
+              showInSelector: true,
               icon: 'logoElasticsearch',
               description: 'Log in with Elasticsearch',
             },
+            {
+              type: 'saml',
+              name: 'saml1',
+              usesLoginForm: false,
+              showInSelector: false,
+            },
           ],
         ],
         // selector is enabled, but only basic/token is available and should be returned.
@@ -326,12 +335,13 @@ describe('Login view routes', () => {
               name: 'basic1',
               type: 'basic',
               usesLoginForm: true,
+              showInSelector: true,
               icon: 'logoElasticsearch',
               description: 'Log in with Elasticsearch',
             },
           ],
         ],
-        // selector is enabled, all providers should be returned
+        // selector is enabled
         [
           getAuthcConfig({
             selector: { enabled: true },
@@ -345,7 +355,13 @@ describe('Login view routes', () => {
                 },
               },
               saml: {
-                saml1: { order: 1, description: 'some-desc2', realm: 'realm1', icon: 'some-icon2' },
+                saml1: {
+                  order: 1,
+                  description: 'some-desc2',
+                  realm: 'realm1',
+                  icon: 'some-icon2',
+                  showInSelector: false,
+                },
                 saml2: { order: 2, description: 'some-desc3', hint: 'some-hint3', realm: 'realm2' },
               },
             },
@@ -358,6 +374,7 @@ describe('Login view routes', () => {
               hint: 'some-hint1',
               icon: 'logoElasticsearch',
               usesLoginForm: true,
+              showInSelector: true,
             },
             {
               type: 'saml',
@@ -365,6 +382,7 @@ describe('Login view routes', () => {
               description: 'some-desc2',
               icon: 'some-icon2',
               usesLoginForm: false,
+              showInSelector: false,
             },
             {
               type: 'saml',
@@ -372,55 +390,7 @@ describe('Login view routes', () => {
               description: 'some-desc3',
               hint: 'some-hint3',
               usesLoginForm: false,
-            },
-          ],
-        ],
-        // selector is enabled, only providers that are enabled should be returned.
-        [
-          getAuthcConfig({
-            selector: { enabled: true },
-            providers: {
-              basic: {
-                basic1: {
-                  order: 0,
-                  description: 'some-desc1',
-                  hint: 'some-hint1',
-                  icon: 'some-icon1',
-                },
-              },
-              saml: {
-                saml1: {
-                  order: 1,
-                  description: 'some-desc2',
-                  realm: 'realm1',
-                  showInSelector: false,
-                },
-                saml2: {
-                  order: 2,
-                  description: 'some-desc3',
-                  hint: 'some-hint3',
-                  icon: 'some-icon3',
-                  realm: 'realm2',
-                },
-              },
-            },
-          }),
-          [
-            {
-              type: 'basic',
-              name: 'basic1',
-              description: 'some-desc1',
-              hint: 'some-hint1',
-              icon: 'some-icon1',
-              usesLoginForm: true,
-            },
-            {
-              type: 'saml',
-              name: 'saml2',
-              description: 'some-desc3',
-              hint: 'some-hint3',
-              icon: 'some-icon3',
-              usesLoginForm: false,
+              showInSelector: true,
             },
           ],
         ],
diff --git a/x-pack/plugins/security/server/routes/views/login.ts b/x-pack/plugins/security/server/routes/views/login.ts
index f72facb2e24c..93d43d04a86c 100644
--- a/x-pack/plugins/security/server/routes/views/login.ts
+++ b/x-pack/plugins/security/server/routes/views/login.ts
@@ -55,18 +55,21 @@ export function defineLoginRoutes({
       const { allowLogin, layout = 'form' } = license.getFeatures();
       const { sortedProviders, selector } = config.authc;
 
-      const providers = [];
-      for (const { type, name } of sortedProviders) {
+      const providers = sortedProviders.map(({ type, name }) => {
         // Since `config.authc.sortedProviders` is based on `config.authc.providers` config we can
         // be sure that config is present for every provider in `config.authc.sortedProviders`.
         const { showInSelector, description, hint, icon } = config.authc.providers[type]?.[name]!;
-
-        // Include provider into the list if either selector is enabled or provider uses login form.
         const usesLoginForm = type === 'basic' || type === 'token';
-        if (showInSelector && (usesLoginForm || selector.enabled)) {
-          providers.push({ type, name, usesLoginForm, description, hint, icon });
-        }
-      }
+        return {
+          type,
+          name,
+          usesLoginForm,
+          showInSelector: showInSelector && (usesLoginForm || selector.enabled),
+          description,
+          hint,
+          icon,
+        };
+      });
 
       const loginState: LoginState = {
         allowLogin,