Merge branch '327020-saml-enforcement-by-default-if-enabled' into 'master'

Add warning when SAML SSO checkbox is unchecked

See merge request gitlab-org/gitlab!60868

ee/app/assets/javascripts/saml_providers/saml_settings_form.js

@@ -5,13 +5,18 @@ import { fixTitle } from '~/tooltips';
 
 const CALLOUT_SELECTOR = '.js-callout';
 const HELPER_SELECTOR = '.js-helper-text';
+const WARNING_SELECTOR = '.js-warning';
 
 function getHelperText(el) {
-  return el.parentNode.querySelector(HELPER_SELECTOR);
+  return el?.parentNode?.querySelector(HELPER_SELECTOR) || null;
+}
+
+function getWarning(el) {
+  return el?.parentNode?.querySelector(WARNING_SELECTOR) || null;
 }
 
 function getCallout(el) {
-  return el.closest('.form-group').querySelector(CALLOUT_SELECTOR);
+  return el?.closest('.form-group')?.querySelector(CALLOUT_SELECTOR) || null;
 }
 
 function toggleElementVisibility(el, show) {
@@ -55,6 +60,7 @@ export default class SamlSettingsForm {
       .map((setting) => ({
         ...setting,
         helperText: getHelperText(setting.el),
+        warning: getWarning(setting.el),
         callout: getCallout(setting.el),
       }));
 
@@ -127,7 +133,7 @@ export default class SamlSettingsForm {
     this.settings
       .filter((setting) => setting.dependsOn)
       .forEach((setting) => {
-        const { helperText, callout, el } = setting;
+        const { helperText, warning, callout, el } = setting;
         const isRelatedToggleOn = this.getValueWithDeps(setting.dependsOn);
 
         if (helperText) {
@@ -136,6 +142,10 @@ export default class SamlSettingsForm {
 
         el.disabled = !isRelatedToggleOn;
 
+        if (warning) {
+          toggleElementVisibility(warning, !setting.value);
+        }
+
         if (callout) {
           toggleElementVisibility(callout, setting.value && isRelatedToggleOn);
         }

ee/app/views/groups/saml_providers/_form.html.haml

@@ -8,11 +8,14 @@
           = s_('GroupSAML|Enable SAML authentication for this group')
     .form-group
       .gl-form-checkbox.custom-control.custom-checkbox
-        = f.check_box :enforced_sso, { class: 'custom-control-input js-group-saml-enforced-sso-input', data: { qa_selector: 'enforced_sso_checkbox' } }
+        = f.check_box :enforced_sso, { checked: saml_provider.new_record? || saml_provider.enforced_sso?, class: 'custom-control-input js-group-saml-enforced-sso-input', data: { qa_selector: 'enforced_sso_checkbox' } }
         = f.label :enforced_sso, { class: 'custom-control-label' } do
           = s_('GroupSAML|Enforce SSO-only authentication for web activity for this group')
           %p.help-text.js-helper-text{ class: saml_provider.enabled? && 'gl-display-none' }
             = s_('GroupSAML|Before enforcing SSO, enable SAML authentication.')
+          %p.help-text.js-warning{ class: saml_provider.enforced_sso? && 'gl-display-none' }
+            - link_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: help_page_path('user/group/saml_sso/index', anchor: 'sso-enforcement') }
+            = html_escape(s_('GroupSAML|%{strongOpen}Warning%{strongClose} - Enabling %{linkStart}SSO enforcement%{linkEnd} can reduce security risks.')) % { strongOpen: '<strong>'.html_safe, strongClose: '</strong>'.html_safe, linkStart: link_start, linkEnd: '</a>'.html_safe }
     .form-group
       .gl-form-checkbox.custom-control.custom-checkbox
         = f.check_box :git_check_enforced, { class: 'custom-control-input js-group-saml-enforced-git-check-input' }

ee/changelogs/unreleased/327020-saml-enforcement-by-default-if-enabled-2.yml

+---
+title: Default enable SSO-only authentication for web activity and add warning if
+  it is disabled
+merge_request: 60868
+author:
+type: changed

ee/spec/features/groups/saml_providers_spec.rb

@@ -57,6 +57,22 @@ RSpec.describe 'SAML provider settings' do
       expect(response_headers['Content-Type']).to have_content("application/xml")
     end
 
+    context '"Enforce SSO-only authentication for web activity for this group" checkbox' do
+      it 'is checked by default' do
+        visit group_saml_providers_path(group)
+
+        expect(find_field('Enforce SSO-only authentication for web activity for this group')).to be_checked
+      end
+
+      it 'displays warning if unchecked', :js do
+        visit group_saml_providers_path(group)
+
+        uncheck 'Enforce SSO-only authentication for web activity for this group'
+
+        expect(page).to have_content 'Warning - Enabling SSO enforcement can reduce security risks.'
+      end
+    end
+
     it 'allows creation of new provider' do
       visit group_saml_providers_path(group)
 
@@ -75,7 +91,7 @@ RSpec.describe 'SAML provider settings' do
     end
 
     context 'with existing SAML provider' do
-      let!(:saml_provider) { create(:saml_provider, group: group, prohibited_outer_forks: false) }
+      let!(:saml_provider) { create(:saml_provider, group: group, prohibited_outer_forks: false, enforced_sso: true) }
 
       it 'allows provider to be disabled', :js do
         visit group_saml_providers_path(group)
@@ -97,9 +113,10 @@ RSpec.describe 'SAML provider settings' do
       it 'updates the enforced sso setting', :js do
         visit group_saml_providers_path(group)
 
-        check 'Enforce SSO-only authentication for web activity for this group'
+        uncheck 'Enforce SSO-only authentication for web activity for this group'
 
-        expect { submit }.to change { saml_provider.reload.enforced_sso }.to(true)
+        expect { submit }.to change { saml_provider.reload.enforced_sso }.to(false)
+        expect(page).to have_content 'Warning - Enabling SSO enforcement can reduce security risks.'
       end
 
       context 'enforced_group_managed_accounts enabled', :js do
@@ -111,7 +128,6 @@ RSpec.describe 'SAML provider settings' do
         it 'updates the enforced_group_managed_accounts flag' do
           visit group_saml_providers_path(group)
 
-          check 'Enforce SSO-only authentication for web activity for this group'
           check 'Enforce users to have dedicated group-managed accounts for this group'
 
           expect { submit }.to change { saml_provider.reload.enforced_group_managed_accounts }.to(true)
@@ -120,7 +136,6 @@ RSpec.describe 'SAML provider settings' do
         it 'updates the prohibited_outer_forks flag' do
           visit group_saml_providers_path(group)
 
-          check 'Enforce SSO-only authentication for web activity for this group'
           check 'Enforce users to have dedicated group-managed accounts for this group'
           check 'Prohibit outer forks for this group'
 

ee/spec/frontend/fixtures/saml_providers.rb

@@ -21,7 +21,7 @@ RSpec.describe Groups::SamlProvidersController, '(JavaScript fixtures)', type: :
   end
 
   it 'groups/saml_providers/show.html' do
-    create(:saml_provider, group: group)
+    create(:saml_provider, group: group, enforced_sso: true)
 
     get :show, params: { group_id: group }
 

ee/spec/frontend/saml_providers/saml_settings_form_spec.js

@@ -13,6 +13,8 @@ describe('SamlSettingsForm', () => {
 
   const findEnforcedGroupManagedAccountSetting = () =>
     samlSettingsForm.settings.find((s) => s.name === 'enforced-group-managed-accounts');
+  const findEnforcedSsoSetting = () =>
+    samlSettingsForm.settings.find((s) => s.name === 'enforced-sso');
   const findProhibitForksSetting = () =>
     samlSettingsForm.settings.find((s) => s.name === 'prohibited-outer-forks');
 
@@ -65,6 +67,16 @@ describe('SamlSettingsForm', () => {
     expect(findProhibitForksSetting().value).toBe(true);
   });
 
+  it('correctly shows warning text when checkbox is unchecked', () => {
+    expect(findEnforcedSsoSetting().warning.classList.contains('gl-display-none')).toBe(true);
+
+    findEnforcedSsoSetting().el.checked = false;
+    samlSettingsForm.updateSAMLSettings();
+    samlSettingsForm.updateView();
+
+    expect(findEnforcedSsoSetting().warning.classList.contains('gl-display-none')).toBe(false);
+  });
+
   it('correctly disables multiple dependent toggles', () => {
     samlSettingsForm.settings.forEach((s) => {
       const { el } = s;

locale/gitlab.pot

@@ -15699,6 +15699,9 @@ msgstr ""
 msgid "GroupRoadmap|To widen your search, change or remove filters; from %{startDate} to %{endDate}."
 msgstr ""
 
+msgid "GroupSAML|%{strongOpen}Warning%{strongClose} - Enabling %{linkStart}SSO enforcement%{linkEnd} can reduce security risks."
+msgstr ""
+
 msgid "GroupSAML|Active SAML Group Links (%{count})"
 msgstr ""