From 6febfdffd2c62a64e6fcf32f32905ad0f781cf1d Mon Sep 17 00:00:00 2001
From: Mihai Doarna <mihai.doarna@grafana.com>
Date: Tue, 19 Mar 2024 12:20:19 +0200
Subject: [PATCH] Auth: add the missing fields for all SSO providers (#83813)

* add the missing fields for sso providers

* remove fields array

* add the validate_hd field for google

* submit only fields defined on the provider

* fix unit tests

* add unit tests for the new fields/sections from the form

* add hosted_domain field for the google provider

* reorder fields in user mapping

* remove authStyle field from gitlab and okta

---------

Co-authored-by: Mihaly Gyongyosi <mgyongyosi@users.noreply.github.com>
---
 .../auth-config/ProviderConfigForm.test.tsx   |  95 +++++++++---
 .../auth-config/ProviderConfigForm.tsx        |  74 ++++------
 public/app/features/auth-config/fields.tsx    | 136 +++++++++++++++---
 public/app/features/auth-config/types.ts      |   2 +
 public/app/features/auth-config/utils/data.ts |  50 ++++---
 5 files changed, 250 insertions(+), 107 deletions(-)

diff --git a/public/app/features/auth-config/ProviderConfigForm.test.tsx b/public/app/features/auth-config/ProviderConfigForm.test.tsx
index efab7d42947..a59f15d821f 100644
--- a/public/app/features/auth-config/ProviderConfigForm.test.tsx
+++ b/public/app/features/auth-config/ProviderConfigForm.test.tsx
@@ -77,24 +77,54 @@ describe('ProviderConfigForm', () => {
     jest.clearAllMocks();
   });
 
-  it('renders all fields correctly', async () => {
+  it('renders all general settings fields correctly', async () => {
     setup(<ProviderConfigForm config={testConfig} provider={testConfig.provider} />);
     expect(screen.getByRole('textbox', { name: /Client ID/i })).toBeInTheDocument();
-    expect(screen.getByRole('combobox', { name: /Team IDs/i })).toBeInTheDocument();
-    expect(screen.getByRole('combobox', { name: /Allowed organizations/i })).toBeInTheDocument();
+    expect(screen.getByRole('textbox', { name: /Client secret/i })).toBeInTheDocument();
+    expect(screen.getByRole('combobox', { name: /Scopes/i })).toBeInTheDocument();
+    expect(screen.getByRole('checkbox', { name: /Allow Sign Up/i })).toBeInTheDocument();
+    expect(screen.getByRole('checkbox', { name: /Auto login/i })).toBeInTheDocument();
+    expect(screen.getByRole('textbox', { name: /Sign out redirect URL/i })).toBeInTheDocument();
     expect(screen.getByRole('button', { name: /Save/i })).toBeInTheDocument();
     expect(screen.getByRole('link', { name: /Discard/i })).toBeInTheDocument();
   });
 
+  it('renders all user mapping fields correctly', async () => {
+    const { user } = setup(<ProviderConfigForm config={testConfig} provider={testConfig.provider} />);
+    await user.click(screen.getByText('User mapping'));
+    expect(screen.getByRole('textbox', { name: /Role attribute path/i })).toBeInTheDocument();
+    expect(screen.getByRole('checkbox', { name: /Role attribute strict mode/i })).toBeInTheDocument();
+    expect(screen.getByRole('checkbox', { name: /Skip organization role sync/i })).toBeInTheDocument();
+  });
+
+  it('renders all extra security fields correctly', async () => {
+    const { user } = setup(<ProviderConfigForm config={testConfig} provider={testConfig.provider} />);
+    await user.click(screen.getByText('Extra security measures'));
+    expect(screen.getByRole('combobox', { name: /Allowed organizations/i })).toBeInTheDocument();
+    expect(screen.getByRole('combobox', { name: /Allowed domains/i })).toBeInTheDocument();
+    expect(screen.getByRole('combobox', { name: /Team Ids/i })).toBeInTheDocument();
+    expect(screen.getByRole('checkbox', { name: /Use PKCE/i })).toBeInTheDocument();
+    expect(screen.getByRole('checkbox', { name: /Use refresh token/i })).toBeInTheDocument();
+    expect(screen.getByRole('checkbox', { name: /TLS skip verify/i })).toBeInTheDocument();
+  });
+
   it('should save and enable on form submit', async () => {
     const { user } = setup(<ProviderConfigForm config={emptyConfig} provider={emptyConfig.provider} />);
+
     await user.type(screen.getByRole('textbox', { name: /Client ID/i }), 'test-client-id');
     await user.type(screen.getByLabelText(/Client secret/i), 'test-client-secret');
-    // Type a team name and press enter to select it
-    await user.type(screen.getByRole('combobox', { name: /Team IDs/i }), '12324{enter}');
-    // Add two orgs
-    await user.type(screen.getByRole('combobox', { name: /Allowed organizations/i }), 'test-org1{enter}');
-    await user.type(screen.getByRole('combobox', { name: /Allowed organizations/i }), 'test-org2{enter}');
+    // Type a scope and press enter to select it
+    await user.type(screen.getByRole('combobox', { name: /Scopes/i }), 'user:email{enter}');
+    await user.click(screen.getByRole('checkbox', { name: /Auto login/i }));
+
+    await user.click(screen.getByText('User mapping'));
+    await user.type(screen.getByRole('textbox', { name: /Role attribute path/i }), 'new-attribute-path');
+    await user.click(screen.getByRole('checkbox', { name: /Role attribute strict mode/i }));
+
+    await user.click(screen.getByText('Extra security measures'));
+    await user.type(screen.getByRole('combobox', { name: /Allowed domains/i }), 'grafana.com{enter}');
+    await user.click(screen.getByRole('checkbox', { name: /Use PKCE/i }));
+
     await user.click(screen.getByRole('button', { name: /Save and enable/i }));
 
     await waitFor(() => {
@@ -104,12 +134,27 @@ describe('ProviderConfigForm', () => {
           id: '300f9b7c-0488-40db-9763-a22ce8bf6b3e',
           provider: 'github',
           settings: {
-            name: 'GitHub',
-            allowedOrganizations: 'test-org1,test-org2',
+            allowAssignGrafanaAdmin: false,
+            allowSignUp: false,
+            allowedDomains: 'grafana.com',
+            allowedOrganizations: '',
+            autoLogin: true,
             clientId: 'test-client-id',
             clientSecret: 'test-client-secret',
-            teamIds: '12324',
             enabled: true,
+            name: 'GitHub',
+            roleAttributePath: 'new-attribute-path',
+            roleAttributeStrict: true,
+            scopes: 'user:email',
+            signoutRedirectUrl: '',
+            skipOrgRoleSync: false,
+            teamIds: '',
+            tlsClientCa: '',
+            tlsClientCert: '',
+            tlsClientKey: '',
+            tlsSkipVerifyInsecure: false,
+            usePkce: true,
+            useRefreshToken: false,
           },
         },
         { showErrorAlert: false }
@@ -126,11 +171,9 @@ describe('ProviderConfigForm', () => {
     const { user } = setup(<ProviderConfigForm config={emptyConfig} provider={emptyConfig.provider} />);
     await user.type(screen.getByRole('textbox', { name: /Client ID/i }), 'test-client-id');
     await user.type(screen.getByLabelText(/Client secret/i), 'test-client-secret');
-    // Type a team name and press enter to select it
-    await user.type(screen.getByRole('combobox', { name: /Team IDs/i }), '12324{enter}');
-    // Add two orgs
-    await user.type(screen.getByRole('combobox', { name: /Allowed organizations/i }), 'test-org1{enter}');
-    await user.type(screen.getByRole('combobox', { name: /Allowed organizations/i }), 'test-org2{enter}');
+    // Type a scope and press enter to select it
+    await user.type(screen.getByRole('combobox', { name: /Scopes/i }), 'user:email{enter}');
+    await user.click(screen.getByRole('checkbox', { name: /Auto login/i }));
     await user.click(screen.getByText('Save'));
 
     await waitFor(() => {
@@ -140,12 +183,26 @@ describe('ProviderConfigForm', () => {
           id: '300f9b7c-0488-40db-9763-a22ce8bf6b3e',
           provider: 'github',
           settings: {
-            name: 'GitHub',
-            allowedOrganizations: 'test-org1,test-org2',
+            allowAssignGrafanaAdmin: false,
+            allowSignUp: false,
+            allowedDomains: '',
+            allowedOrganizations: '',
+            autoLogin: true,
             clientId: 'test-client-id',
             clientSecret: 'test-client-secret',
-            teamIds: '12324',
             enabled: false,
+            name: 'GitHub',
+            roleAttributePath: '',
+            roleAttributeStrict: false,
+            scopes: 'user:email',
+            signoutRedirectUrl: '',
+            skipOrgRoleSync: false,
+            teamIds: '',
+            tlsClientCa: '',
+            tlsClientCert: '',
+            tlsClientKey: '',
+            usePkce: false,
+            useRefreshToken: false,
           },
         },
         { showErrorAlert: false }
diff --git a/public/app/features/auth-config/ProviderConfigForm.tsx b/public/app/features/auth-config/ProviderConfigForm.tsx
index d8b2d3b6414..9427390d62c 100644
--- a/public/app/features/auth-config/ProviderConfigForm.tsx
+++ b/public/app/features/auth-config/ProviderConfigForm.tsx
@@ -21,7 +21,7 @@ import { FormPrompt } from '../../core/components/FormPrompt/FormPrompt';
 import { Page } from '../../core/components/Page/Page';
 
 import { FieldRenderer } from './FieldRenderer';
-import { fields, sectionFields } from './fields';
+import { sectionFields } from './fields';
 import { SSOProvider, SSOProviderDTO } from './types';
 import { dataToDTO, dtoToData } from './utils/data';
 
@@ -45,7 +45,6 @@ export const ProviderConfigForm = ({ config, provider, isLoading }: ProviderConf
     formState: { errors, dirtyFields, isSubmitted },
   } = useForm({ defaultValues: dataToDTO(config), mode: 'onSubmit', reValidateMode: 'onChange' });
   const [isSaving, setIsSaving] = useState(false);
-  const providerFields = fields[provider];
   const [submitError, setSubmitError] = useState(false);
   const dataSubmitted = isSubmitted && !submitError;
   const sections = sectionFields[provider];
@@ -155,55 +154,34 @@ export const ProviderConfigForm = ({ config, provider, isLoading }: ProviderConf
         <Field label="Enabled" hidden={true}>
           <Switch {...register('enabled')} id="enabled" label={'Enabled'} />
         </Field>
-        {sections ? (
-          <Stack gap={2} direction={'column'}>
-            {sections
-              .filter((section) => !section.hidden)
-              .map((section, index) => {
-                return (
-                  <CollapsableSection label={section.name} isOpen={index === 0} key={section.name}>
-                    {section.fields
-                      .filter((field) => (typeof field !== 'string' ? !field.hidden : true))
-                      .map((field) => {
-                        return (
-                          <FieldRenderer
-                            key={typeof field === 'string' ? field : field.name}
-                            field={field}
-                            control={control}
-                            errors={errors}
-                            setValue={setValue}
-                            register={register}
-                            watch={watch}
-                            unregister={unregister}
-                            provider={provider}
-                            secretConfigured={!!config?.settings.clientSecret}
-                          />
-                        );
-                      })}
-                  </CollapsableSection>
-                );
-              })}
-          </Stack>
-        ) : (
-          <>
-            {providerFields.map((field) => {
+        <Stack gap={2} direction={'column'}>
+          {sections
+            .filter((section) => !section.hidden)
+            .map((section, index) => {
               return (
-                <FieldRenderer
-                  key={field}
-                  field={field}
-                  control={control}
-                  errors={errors}
-                  setValue={setValue}
-                  register={register}
-                  watch={watch}
-                  unregister={unregister}
-                  provider={provider}
-                  secretConfigured={!!config?.settings.clientSecret}
-                />
+                <CollapsableSection label={section.name} isOpen={index === 0} key={section.name}>
+                  {section.fields
+                    .filter((field) => (typeof field !== 'string' ? !field.hidden : true))
+                    .map((field) => {
+                      return (
+                        <FieldRenderer
+                          key={typeof field === 'string' ? field : field.name}
+                          field={field}
+                          control={control}
+                          errors={errors}
+                          setValue={setValue}
+                          register={register}
+                          watch={watch}
+                          unregister={unregister}
+                          provider={provider}
+                          secretConfigured={!!config?.settings.clientSecret}
+                        />
+                      );
+                    })}
+                </CollapsableSection>
               );
             })}
-          </>
-        )}
+        </Stack>
         <Box display={'flex'} gap={2} marginTop={5}>
           <Stack alignItems={'center'} gap={2}>
             <Button
diff --git a/public/app/features/auth-config/fields.tsx b/public/app/features/auth-config/fields.tsx
index 38c28e519c8..f40536bf95b 100644
--- a/public/app/features/auth-config/fields.tsx
+++ b/public/app/features/auth-config/fields.tsx
@@ -9,24 +9,6 @@ import { FieldData, SSOProvider, SSOSettingsField } from './types';
 import { isSelectableValue } from './utils/guards';
 import { isUrlValid } from './utils/url';
 
-/** Map providers to their settings */
-export const fields: Record<SSOProvider['provider'], Array<keyof SSOProvider['settings']>> = {
-  github: ['name', 'clientId', 'clientSecret', 'teamIds', 'allowedOrganizations'],
-  google: ['name', 'clientId', 'clientSecret', 'allowedDomains'],
-  gitlab: ['name', 'clientId', 'clientSecret', 'allowedOrganizations', 'teamIds'],
-  okta: [
-    'name',
-    'clientId',
-    'clientSecret',
-    'authUrl',
-    'tokenUrl',
-    'apiUrl',
-    'roleAttributePath',
-    'allowedGroups',
-    'allowedDomains',
-  ],
-};
-
 type Section = Record<
   SSOProvider['provider'],
   Array<{
@@ -131,6 +113,113 @@ export const sectionFields: Section = {
       ],
     },
   ],
+  google: [
+    {
+      name: 'General settings',
+      id: 'general',
+      fields: ['name', 'clientId', 'clientSecret', 'scopes', 'allowSignUp', 'autoLogin', 'signoutRedirectUrl'],
+    },
+    {
+      name: 'User mapping',
+      id: 'user',
+      fields: ['roleAttributePath', 'roleAttributeStrict', 'allowAssignGrafanaAdmin', 'skipOrgRoleSync'],
+    },
+    {
+      name: 'Extra security measures',
+      id: 'extra',
+      fields: [
+        'validateHd',
+        'hostedDomain',
+        'allowedDomains',
+        'allowedGroups',
+        'usePkce',
+        'useRefreshToken',
+        'tlsSkipVerifyInsecure',
+        'tlsClientCert',
+        'tlsClientKey',
+        'tlsClientCa',
+      ],
+    },
+  ],
+  github: [
+    {
+      name: 'General settings',
+      id: 'general',
+      fields: ['name', 'clientId', 'clientSecret', 'scopes', 'allowSignUp', 'autoLogin', 'signoutRedirectUrl'],
+    },
+    {
+      name: 'User mapping',
+      id: 'user',
+      fields: ['roleAttributePath', 'roleAttributeStrict', 'allowAssignGrafanaAdmin', 'skipOrgRoleSync'],
+    },
+    {
+      name: 'Extra security measures',
+      id: 'extra',
+      fields: [
+        'allowedOrganizations',
+        'allowedDomains',
+        'teamIds',
+        'usePkce',
+        'useRefreshToken',
+        'tlsSkipVerifyInsecure',
+        'tlsClientCert',
+        'tlsClientKey',
+        'tlsClientCa',
+      ],
+    },
+  ],
+  gitlab: [
+    {
+      name: 'General settings',
+      id: 'general',
+      fields: ['name', 'clientId', 'clientSecret', 'scopes', 'allowSignUp', 'autoLogin', 'signoutRedirectUrl'],
+    },
+    {
+      name: 'User mapping',
+      id: 'user',
+      fields: ['roleAttributePath', 'roleAttributeStrict', 'allowAssignGrafanaAdmin', 'skipOrgRoleSync'],
+    },
+    {
+      name: 'Extra security measures',
+      id: 'extra',
+      fields: [
+        'allowedDomains',
+        'allowedGroups',
+        'usePkce',
+        'useRefreshToken',
+        'tlsSkipVerifyInsecure',
+        'tlsClientCert',
+        'tlsClientKey',
+        'tlsClientCa',
+      ],
+    },
+  ],
+  okta: [
+    {
+      name: 'General settings',
+      id: 'general',
+      fields: ['name', 'clientId', 'clientSecret', 'scopes', 'authUrl', 'tokenUrl', 'apiUrl', 'signoutRedirectUrl'],
+    },
+    {
+      name: 'User mapping',
+      id: 'user',
+      fields: ['roleAttributePath', 'roleAttributeStrict', 'allowAssignGrafanaAdmin', 'skipOrgRoleSync'],
+    },
+    {
+      name: 'Extra security measures',
+      id: 'extra',
+      fields: [
+        'allowedDomains',
+        'allowedGroups',
+        'usePkce',
+        'useRefreshToken',
+        'tlsSkipVerifyInsecure',
+        'tlsClientCert',
+        'tlsClientKey',
+        'tlsClientCa',
+      ],
+    },
+  ],
 };
 
 /**
@@ -485,6 +574,17 @@ export function fieldMap(provider: string): Record<string, FieldData> {
             }
           : undefined,
     },
+    hostedDomain: {
+      label: 'Hosted domain',
+      description: 'The domain under which Grafana is hosted and accessible.',
+      type: 'text',
+    },
+    validateHd: {
+      label: 'Validate hosted domain',
+      description:
+        'If enabled, Grafana will match the Hosted Domain retrieved from the Google ID Token against the Allowed Domains list specified by the user.',
+      type: 'checkbox',
+    },
   };
 }
 
diff --git a/public/app/features/auth-config/types.ts b/public/app/features/auth-config/types.ts
index 02d5d1fe3ba..b139efda4fb 100644
--- a/public/app/features/auth-config/types.ts
+++ b/public/app/features/auth-config/types.ts
@@ -55,6 +55,8 @@ export type SSOProviderSettingsBase = {
   tlsSkipVerifyInsecure?: boolean;
   // For Azure AD
   forceUseGraphApi?: boolean;
+  // For Google
+  validateHd?: boolean;
 };
 
 // SSO data received from the API and sent to it
diff --git a/public/app/features/auth-config/utils/data.ts b/public/app/features/auth-config/utils/data.ts
index d269dcc0ae8..fa7d425939c 100644
--- a/public/app/features/auth-config/utils/data.ts
+++ b/public/app/features/auth-config/utils/data.ts
@@ -1,6 +1,6 @@
 import { SelectableValue } from '@grafana/data';
 
-import { fieldMap, fields } from '../fields';
+import { fieldMap, sectionFields } from '../fields';
 import { FieldData, SSOProvider, SSOProviderDTO } from '../types';
 
 import { isSelectableValue } from './guards';
@@ -58,7 +58,8 @@ export function dataToDTO(data?: SSOProvider): SSOProviderDTO {
   if (!data) {
     return emptySettings;
   }
-  const arrayFields = getArrayFields(fieldMap(data.provider));
+  const providerFields = getFieldsForProvider(data.provider);
+  const arrayFields = getArrayFields(fieldMap(data.provider), providerFields);
   const settings = { ...data.settings };
   for (const field of arrayFields) {
     //@ts-expect-error
@@ -72,30 +73,35 @@ const valuesToString = (values: Array<SelectableValue<string>>) => {
   return values.map(({ value }) => value).join(',');
 };
 
-const includeRequiredKeysOnly = (
-  obj: SSOProviderDTO,
-  requiredKeys: Array<keyof SSOProvider['settings']>
-): Partial<SSOProviderDTO> => {
-  if (!requiredKeys) {
-    return obj;
-  }
-  let result: Partial<SSOProviderDTO> = {};
-  for (const key of requiredKeys) {
-    //@ts-expect-error
-    result[key] = obj[key];
-  }
-  return result;
+const getFieldsForProvider = (provider: string) => {
+  const sections = sectionFields[provider];
+
+  // include the enabled field because it is not part of the fields defined for providers
+  const fields = ['enabled'];
+
+  return Object.values(sections).reduce(
+    (result, section) => [
+      ...result,
+      ...section.fields.map((field) => (typeof field === 'string' ? field : field.name)),
+    ],
+    fields
+  );
 };
 
 // Convert the DTO to the data format used by the API
 export function dtoToData(dto: SSOProviderDTO, provider: string) {
-  const arrayFields = getArrayFields(fieldMap(provider));
   let current: Partial<SSOProviderDTO> = dto;
 
-  if (fields[provider]) {
-    current = includeRequiredKeysOnly(dto, [...fields[provider], 'enabled']);
-  }
-  const settings = { ...current };
+  const providerFields = getFieldsForProvider(provider);
+  const arrayFields = getArrayFields(fieldMap(provider), providerFields);
+
+  // filter out the fields that are not defined on the provider
+  const settings: Partial<SSOProviderDTO> = Object.keys(current)
+    .filter((key) => providerFields.includes(key))
+    .reduce((obj, key) => {
+      //@ts-expect-error
+      return { ...obj, [key]: current[key] };
+    }, {});
 
   for (const field of arrayFields) {
     const value = current[field];
@@ -112,8 +118,8 @@ export function dtoToData(dto: SSOProviderDTO, provider: string) {
   return settings;
 }
 
-export function getArrayFields(obj: Record<string, FieldData>): Array<keyof SSOProviderDTO> {
+export function getArrayFields(obj: Record<string, FieldData>, providerFields: string[]): Array<keyof SSOProviderDTO> {
   return Object.entries(obj)
-    .filter(([_, value]) => value.type === 'select')
+    .filter(([key, value]) => providerFields.includes(key) && value.type === 'select')
     .map(([key]) => key as keyof SSOProviderDTO);
 }