From 5999d37644bb847ea5d54819f60e4efebfe968ae Mon Sep 17 00:00:00 2001 From: Aurelien Poscia Date: Tue, 7 Nov 2023 11:17:23 +0100 Subject: [PATCH] fix SSF-434 --- .../org/sonar/auth/github/GitHubSettings.java | 4 +- .../js/api/mocks/AuthenticationServiceMock.ts | 1 + .../components/CategoryDefinitionsList.tsx | 67 +++++++++++++++---- .../apps/settings/components/Definition.tsx | 9 +++ .../settings/components/DefinitionsList.tsx | 2 + .../components/SubCategoryDefinitionsList.tsx | 2 + .../__tests__/Authentication-it.tsx | 37 ++++++++++ .../resources/org/sonar/l10n/core.properties | 2 + 8 files changed, 110 insertions(+), 14 deletions(-) diff --git a/server/sonar-auth-github/src/main/java/org/sonar/auth/github/GitHubSettings.java b/server/sonar-auth-github/src/main/java/org/sonar/auth/github/GitHubSettings.java index 12e01aa4d9b..5bd6f48caab 100644 --- a/server/sonar-auth-github/src/main/java/org/sonar/auth/github/GitHubSettings.java +++ b/server/sonar-auth-github/src/main/java/org/sonar/auth/github/GitHubSettings.java @@ -158,8 +158,8 @@ public class GitHubSettings { .build(), PropertyDefinition.builder(ORGANIZATIONS) .name("Organizations") - .description("Only members of these organizations will be able to authenticate to the server. " + - "If a user is a member of any of the organizations listed they will be authenticated.") + .description("Only members of these organizations will be able to authenticate to the server. " + + "⚠ if not set, any GitHub user will be able to authenticate to the server.") .multiValues(true) .category(CATEGORY) .subCategory(SUBCATEGORY) diff --git a/server/sonar-web/src/main/js/api/mocks/AuthenticationServiceMock.ts b/server/sonar-web/src/main/js/api/mocks/AuthenticationServiceMock.ts index 8aba451dd6e..3cc0d045089 100644 --- a/server/sonar-web/src/main/js/api/mocks/AuthenticationServiceMock.ts +++ b/server/sonar-web/src/main/js/api/mocks/AuthenticationServiceMock.ts @@ -30,6 +30,7 @@ export default class AuthenticationServiceMock { mockSettingValue({ key: 'test2', value: 'test2' }), mockSettingValue({ key: 'sonar.auth.saml.certificate.secured' }), mockSettingValue({ key: 'sonar.auth.saml.enabled', value: 'false' }), + mockSettingValue({ key: 'sonar.auth.github.enabled', value: 'true' }), ]; constructor() { diff --git a/server/sonar-web/src/main/js/apps/settings/components/CategoryDefinitionsList.tsx b/server/sonar-web/src/main/js/apps/settings/components/CategoryDefinitionsList.tsx index e18db13cf06..87e3e439083 100644 --- a/server/sonar-web/src/main/js/apps/settings/components/CategoryDefinitionsList.tsx +++ b/server/sonar-web/src/main/js/apps/settings/components/CategoryDefinitionsList.tsx @@ -19,7 +19,11 @@ */ import { keyBy } from 'lodash'; import * as React from 'react'; +import { FormattedMessage } from 'react-intl'; import { getValues } from '../../../api/settings'; +import DocLink from '../../../components/common/DocLink'; +import { Alert } from '../../../components/ui/Alert'; +import { translate } from '../../../helpers/l10n'; import { ExtendedSettingDefinition, SettingDefinitionAndValue, @@ -38,11 +42,12 @@ interface Props { interface State { settings: SettingDefinitionAndValue[]; + displayGithubOrganizationWarning: boolean; } export default class CategoryDefinitionsList extends React.PureComponent { mounted = false; - state: State = { settings: [] }; + state: State = { settings: [], displayGithubOrganizationWarning: false }; componentDidMount() { this.mounted = true; @@ -60,7 +65,25 @@ export default class CategoryDefinitionsList extends React.PureComponent { + const { category, subCategory } = this.props; + if (category !== 'authentication' || subCategory !== 'github') { + return false; + } + const isGithubEnabled = settings.find((s) => s.definition.key === 'sonar.auth.github.enabled'); + const organizationsSetting = settings.find( + (s) => s.definition.key === 'sonar.auth.github.organizations' + ); + if ( + isGithubEnabled?.settingValue?.value === 'true' && + organizationsSetting?.settingValue === undefined + ) { + return true; + } + return false; + }; + + loadSettingValues = async () => { const { category, component, definitions } = this.props; const categoryDefinitions = definitions.filter( @@ -83,21 +106,41 @@ export default class CategoryDefinitionsList extends React.PureComponent + <> + {displayGithubOrganizationWarning && ( + + + {translate('settings.authentication.github.organization.warning.learn_more')} + + ), + }} + /> + + )} + + ); } } diff --git a/server/sonar-web/src/main/js/apps/settings/components/Definition.tsx b/server/sonar-web/src/main/js/apps/settings/components/Definition.tsx index c0167864efc..81ed3dc2cb8 100644 --- a/server/sonar-web/src/main/js/apps/settings/components/Definition.tsx +++ b/server/sonar-web/src/main/js/apps/settings/components/Definition.tsx @@ -30,6 +30,7 @@ interface Props { component?: Component; definition: ExtendedSettingDefinition; initialSettingValue?: SettingValue; + onUpdate?: () => void; } interface State { @@ -82,6 +83,10 @@ export default class Definition extends React.PureComponent { await resetSettingValue({ keys: definition.key, component: component?.key }); const settingValue = await getValue({ key: definition.key, component: component?.key }); + if (this.props.onUpdate) { + this.props.onUpdate(); + } + this.setState({ changedValue: undefined, loading: false, @@ -171,6 +176,10 @@ export default class Definition extends React.PureComponent { await setSettingValue(definition, changedValue, component?.key); const settingValue = await getValue({ key: definition.key, component: component?.key }); + if (this.props.onUpdate) { + this.props.onUpdate(); + } + this.setState({ changedValue: undefined, isEditing: false, diff --git a/server/sonar-web/src/main/js/apps/settings/components/DefinitionsList.tsx b/server/sonar-web/src/main/js/apps/settings/components/DefinitionsList.tsx index 45c04a0978a..46f627d7b05 100644 --- a/server/sonar-web/src/main/js/apps/settings/components/DefinitionsList.tsx +++ b/server/sonar-web/src/main/js/apps/settings/components/DefinitionsList.tsx @@ -26,6 +26,7 @@ interface Props { component?: Component; scrollToDefinition: (element: HTMLLIElement) => void; settings: SettingDefinitionAndValue[]; + onUpdate?: () => void; } export default function DefinitionsList(props: Props) { @@ -42,6 +43,7 @@ export default function DefinitionsList(props: Props) { component={component} definition={setting.definition} initialSettingValue={setting.settingValue} + onUpdate={props.onUpdate} /> ))} diff --git a/server/sonar-web/src/main/js/apps/settings/components/SubCategoryDefinitionsList.tsx b/server/sonar-web/src/main/js/apps/settings/components/SubCategoryDefinitionsList.tsx index b904584590f..33103b38481 100644 --- a/server/sonar-web/src/main/js/apps/settings/components/SubCategoryDefinitionsList.tsx +++ b/server/sonar-web/src/main/js/apps/settings/components/SubCategoryDefinitionsList.tsx @@ -34,6 +34,7 @@ export interface SubCategoryDefinitionsListProps { settings: Array; subCategory?: string; displaySubCategoryTitle?: boolean; + onUpdate?: () => void; } export class SubCategoryDefinitionsList extends React.PureComponent { @@ -103,6 +104,7 @@ export class SubCategoryDefinitionsList extends React.PureComponent {this.renderEmailForm(subCategory.key)} diff --git a/server/sonar-web/src/main/js/apps/settings/components/authentication/__tests__/Authentication-it.tsx b/server/sonar-web/src/main/js/apps/settings/components/authentication/__tests__/Authentication-it.tsx index 1070420b649..f34136ca02f 100644 --- a/server/sonar-web/src/main/js/apps/settings/components/authentication/__tests__/Authentication-it.tsx +++ b/server/sonar-web/src/main/js/apps/settings/components/authentication/__tests__/Authentication-it.tsx @@ -79,6 +79,8 @@ const ui = { testButton: byText('settings.authentication.saml.form.test'), textbox1: byRole('textbox', { name: 'test1' }), textbox2: byRole('textbox', { name: 'test2' }), + githubTab: byRole('tab', { name: 'github GitHub' }), + githubOrganizationWarning: byText('settings.authentication.github.organization.warning'), }; it('should render tabs and allow navigation', async () => { @@ -209,6 +211,41 @@ describe('SAML tab', () => { }); }); +describe('GitHub tab', () => { + it('should display a warning if github authentication is enabled but no organizations are whitelisted', async () => { + const user = userEvent.setup(); + + const definitions = [ + mockDefinition({ + key: 'sonar.auth.github.enabled', + category: 'authentication', + subCategory: 'github', + name: '"Enabled"', + description: + 'Enable GitHub users to login. Value is ignored if client ID and secret are not defined.', + type: SettingType.BOOLEAN, + }), + mockDefinition({ + key: 'sonar.auth.github.organizations', + category: 'authentication', + subCategory: 'github', + name: 'Organizations', + description: + 'Only members of these organizations will be able to authenticate to the server. If a user is a member of any of the organizations listed they will be authenticated.', + type: SettingType.BOOLEAN, + fields: [], + multiValues: true, + options: [], + }), + ]; + + renderAuthentication(definitions); + + await user.click(await ui.githubTab.find()); + expect(ui.githubOrganizationWarning.get()).toBeInTheDocument(); + }); +}); + function renderAuthentication(definitions: ExtendedSettingDefinition[], features: Feature[] = []) { renderComponent( diff --git a/sonar-core/src/main/resources/org/sonar/l10n/core.properties b/sonar-core/src/main/resources/org/sonar/l10n/core.properties index 9f78437bb71..8f399e90dfe 100644 --- a/sonar-core/src/main/resources/org/sonar/l10n/core.properties +++ b/sonar-core/src/main/resources/org/sonar/l10n/core.properties @@ -1284,6 +1284,8 @@ settings.authentication.saml.form.save_success=Saved successfully settings.authentication.saml.form.save_partial=Saved partially settings.authentication.saml.form.save_warn=Please check the error messages in the form above, saving failed for {0} field(s). settings.authentication.saml.tooltip.required_fields=Please provide a value for the following required field(s): {0} +settings.authentication.github.organization.warning=GitHub authentication is activated but no allowed organization was provided. Please review your settings to make sure the integration is secure. {learn_more}. +settings.authentication.github.organization.warning.learn_more=Learn more settings.pr_decoration.binding.category=DevOps Platform Integration settings.pr_decoration.binding.no_bindings=A system administrator needs to enable this feature in the global settings. -- 2.39.5