diff --git a/app/models/group.rb b/app/models/group.rb index a264127f523aefafafc85f4d8cefe7d3f28f0fff..cd1ac9cec4903aefc74d875f0931082e5eac663c 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -186,6 +186,7 @@ def with_developer_maintainer_owner_access :setup_for_company, :step_up_auth_required_oauth_provider, :step_up_auth_required_oauth_provider=, + :step_up_auth_required_oauth_provider_from_self_or_inherited, to: :namespace_settings ) diff --git a/app/models/namespace_setting.rb b/app/models/namespace_setting.rb index c0f8be12c66d553d25432acac316a47264fec437..8b1d06e7c096c1393b143e149471a8b61cba5af5 100644 --- a/app/models/namespace_setting.rb +++ b/app/models/namespace_setting.rb @@ -66,6 +66,8 @@ class NamespaceSetting < ApplicationRecord } }, allow_nil: true validates :duo_agent_platform_request_count, numericality: { greater_than_or_equal_to: 0 } + validate :validate_step_up_auth_inheritance, if: :will_save_change_to_step_up_auth_required_oauth_provider? + sanitizes! :default_branch_name nullify_if_blank :default_branch_name @@ -176,6 +178,32 @@ def enterprise_placeholder_bypass_enabled? allow_enterprise_bypass_placeholder_confirmation? && enterprise_bypass_expires_at.present? && enterprise_bypass_expires_at.future? end + # Returns the namespace_setting that provides the inherited step-up auth provider (excluding self) + # This is the base method that all other inheritance methods build upon + def step_up_auth_required_oauth_provider_inherited_namespace_setting + # Use traversal_ids for efficient ancestor lookup + # traversal_ids is an array like [root_id, parent_id, ..., current_id] + # We need to exclude self (current_id is the last element) + ancestor_ids = namespace.traversal_ids[0..-2] # All except the last element (self) + + return if ancestor_ids.empty? + + # Single optimized query using traversal_ids + # Order by position in traversal_ids array (root first, so most distant ancestor has precedence) + @step_up_auth_inherited_setting ||= self.class + .joins(:namespace) + .where(namespace_id: ancestor_ids) + .where.not(step_up_auth_required_oauth_provider: nil) + .order(Arel.sql("array_position(ARRAY[#{ancestor_ids.join(',')}]::bigint[], namespace_settings.namespace_id)")) + .includes(:namespace) + .first + end + + # Returns the active/effective step-up auth provider, considering inheritance from parent groups + def step_up_auth_required_oauth_provider_from_self_or_inherited + step_up_auth_required_oauth_provider_inherited_namespace_setting&.step_up_auth_required_oauth_provider || step_up_auth_required_oauth_provider + end + private def validate_enterprise_bypass_expires_at @@ -194,6 +222,19 @@ def validate_enterprise_bypass_expires_at end end + def validate_step_up_auth_inheritance + # Use the base method to check inheritance and get parent info in one call + inherited_namespace_setting = step_up_auth_required_oauth_provider_inherited_namespace_setting + return unless inherited_namespace_setting + + errors.add(:step_up_auth_required_oauth_provider, + format( + s_('GroupSettings|cannot be changed because it is inherited from parent group "%{parent_name}"'), + parent_name: inherited_namespace_setting.namespace.name + ) + ) + end + def set_pipeline_variables_default_role return if Gitlab::CurrentSettings.pipeline_variables_default_allowed diff --git a/app/views/groups/settings/_step_up_authentication.html.haml b/app/views/groups/settings/_step_up_authentication.html.haml index 470d1db15d5aa55143e294e08a9318ff8e8780c8..dc675c36b47d89838675242c429b5b36ec059cc1 100644 --- a/app/views/groups/settings/_step_up_authentication.html.haml +++ b/app/views/groups/settings/_step_up_authentication.html.haml @@ -1,4 +1,4 @@ -%h5= s_('GroupSettings|Step-up authentication') +%h5= f.label :step_up_auth_required_oauth_provider, s_('GroupSettings|Step-up authentication') %p = link_to s_('GroupSettings|What is step-up authentication?'), help_page_path('administration/auth/oidc.md', anchor: 'force-step-up-authentication-for-a-group'), @@ -6,10 +6,23 @@ rel: 'noopener noreferrer' .form-group - = f.select :step_up_auth_required_oauth_provider, + - inherited_namespace_setting = group.namespace_settings.step_up_auth_required_oauth_provider_inherited_namespace_setting + - inherited_namespace = inherited_namespace_setting&.namespace + - if inherited_namespace.present? + = render Pajamas::AlertComponent.new(variant: :info, dismissible: false, alert_options: { class: 'gl-mb-3', data: { testid: 'step-up-auth-inheritance-alert' } }) do |c| + - c.with_body do + = safe_format(s_('GroupSettings|Step-up authentication is inherited from parent group "%{parent_group_name}" and cannot be changed at this level.'), parent_group_name: inherited_namespace.name) + + = f.hidden_field :step_up_auth_required_oauth_provider + = f.select :step_up_auth_required_oauth_provider_from_self_or_inherited, + step_up_auth_provider_options_for_select, + { include_blank: s_('GroupSettings|Disabled') }, + { class: 'form-control gl-form-select', disabled: true, 'aria-label': s_('GroupSettings|Selected step-up authentication provider for this namespace'), id: 'group_step_up_auth_required_oauth_provider' } + - else + = f.select :step_up_auth_required_oauth_provider, step_up_auth_provider_options_for_select, { include_blank: s_('GroupSettings|Disabled') }, - { class: 'form-control gl-form-select' } + { class: 'form-control gl-form-select', 'aria-label': s_('GroupSettings|Selected step-up authentication provider for this namespace') } .form-text.gl-text-subtle = s_('GroupSettings|Forces users to complete step-up authentication before they can access this group.') diff --git a/locale/gitlab.pot b/locale/gitlab.pot index e83165c792055461b4f88933450d213e637c0775..db12625ee6ae06b43485e78ecb4fa403c92edba4 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -32645,6 +32645,9 @@ msgstr "" msgid "GroupSettings|Select the project containing your custom Insights file." msgstr "" +msgid "GroupSettings|Selected step-up authentication provider for this namespace" +msgstr "" + msgid "GroupSettings|Service access tokens expiration enforced setting was not saved" msgstr "" @@ -32660,6 +32663,9 @@ msgstr "" msgid "GroupSettings|Step-up authentication" msgstr "" +msgid "GroupSettings|Step-up authentication is inherited from parent group \"%{parent_group_name}\" and cannot be changed at this level." +msgstr "" + msgid "GroupSettings|The Auto DevOps pipeline runs if no alternative CI configuration file is found." msgstr "" @@ -32720,6 +32726,9 @@ msgstr "" msgid "GroupSettings|You will need to update your local repositories to point to the new location." msgstr "" +msgid "GroupSettings|cannot be changed because it is inherited from parent group \"%{parent_name}\"" +msgstr "" + msgid "GroupSettings|cannot be changed by you" msgstr "" diff --git a/spec/features/groups/group_settings_spec.rb b/spec/features/groups/group_settings_spec.rb index 69c8a002475a884e8bce2bfe18cc8fc2a5562a88..18ab34cfe79f875e982d535fad19890bf16ab67e 100644 --- a/spec/features/groups/group_settings_spec.rb +++ b/spec/features/groups/group_settings_spec.rb @@ -444,6 +444,123 @@ def remove_with_confirm(button_text, confirm_with, confirm_button_text = 'Confir end end + describe 'step-up authentication settings', :js do + context 'with configured step-up omniauth providers' do + let_it_be(:omniauth_provider_config_oidc) do + GitlabSettings::Options.new( + name: 'openid_connect', + label: 'OpenID Connect', + step_up_auth: { + namespace: { + id_token: { + required: { acr: 'gold' } + } + } + } + ) + end + + before do + stub_omniauth_setting(enabled: true, providers: [omniauth_provider_config_oidc]) + allow(Devise).to receive(:omniauth_providers).and_return([omniauth_provider_config_oidc.name]) + end + + context 'when group has no parent' do + it 'allows configuring step-up authentication' do + visit edit_group_path(group) + + within_testid('permissions-settings') do + expect(page).to have_content('Step-up authentication') + expect(page).to have_select('Step-up authentication', disabled: false, with_options: ['OpenID Connect']) + end + end + + it 'can select and save step-up authentication provider' do + visit edit_group_path(group) + + within_testid('permissions-settings') do + select 'OpenID Connect', from: 'group_step_up_auth_required_oauth_provider' + click_button 'Save changes' + end + + expect(page).to have_text("Group '#{group.name}' was successfully updated") + expect(group.reload.namespace_settings.step_up_auth_required_oauth_provider).to eq('openid_connect') + end + + it 'can disable step-up authentication' do + group.namespace_settings.update!(step_up_auth_required_oauth_provider: 'openid_connect') + + visit edit_group_path(group) + + within_testid('permissions-settings') do + select 'Disabled', from: 'group_step_up_auth_required_oauth_provider' + click_button 'Save changes' + end + + expect(page).to have_text("Group '#{group.name}' was successfully updated") + expect(group.reload.namespace_settings.step_up_auth_required_oauth_provider).to be_nil + end + end + + context 'when group inherits step-up authentication from parent' do + let_it_be_with_reload(:grandparent_group) { create(:group, owners: [user]) } + let_it_be_with_reload(:parent_group) { create(:group, parent: grandparent_group, owners: [user]) } + let_it_be_with_reload(:child_group) { create(:group, parent: parent_group, owners: [user]) } + + before do + parent_group.namespace_settings.update!(step_up_auth_required_oauth_provider: 'openid_connect') + end + + it 'shows complete inheritance alert with parent group name and guidance' do + visit edit_group_path(child_group) + + within_testid('permissions-settings') do + expect(page).to have_content('Step-up authentication') + + # Check the alert exists and has correct properties + alert = find_by_testid('step-up-auth-inheritance-alert') + expect(alert).to be_present + expect(alert[:class]).to include('gl-alert-info') + expect(alert).to have_content("Step-up authentication is inherited from parent group \"#{parent_group.name}\"") + end + end + + it 'disables form controls and shows inherited value with proper accessibility' do + visit edit_group_path(child_group) + + within_testid('permissions-settings') do + expect(page).to have_content('Step-up authentication') + + # Check inheritance alert with complete messaging + expect(find_by_testid('step-up-auth-inheritance-alert')) + .to have_content("Step-up authentication is inherited from parent group \"#{parent_group.name}\"") + + # Form controls should be disabled with correct inherited value + expect(page).to have_select('Step-up authentication', disabled: true, selected: 'OpenID Connect') + + child_group.namespace_settings.update!(step_up_auth_required_oauth_provider: nil) + end + end + end + end + + context 'without configured step-up omniauth providers' do + before do + stub_omniauth_setting(enabled: true, providers: []) + allow(Devise).to receive(:omniauth_providers).and_return([]) + end + + it 'shows only disabled option' do + visit edit_group_path(group) + + within_testid('permissions-settings') do + expect(page).to have_content('Step-up authentication') + expect(page).to have_select('Step-up authentication', disabled: false, selected: 'Disabled', with_options: []) + end + end + end + end + def update_path(new_group_path) visit edit_group_path(group) diff --git a/spec/models/namespace_setting_spec.rb b/spec/models/namespace_setting_spec.rb index 47f0ee64bf0262ffa25cb36106d1e5784a601976..bf71daac8acec5e92c9b876321a0e3d4794760c1 100644 --- a/spec/models/namespace_setting_spec.rb +++ b/spec/models/namespace_setting_spec.rb @@ -6,7 +6,7 @@ using RSpec::Parameterized::TableSyntax let_it_be(:group) { create(:group) } - let_it_be(:subgroup, refind: true) { create(:group, parent: group) } + let_it_be_with_reload(:subgroup) { create(:group, parent: group) } let(:namespace_settings) { group.namespace_settings } it_behaves_like 'sanitizable', :namespace_settings, %i[default_branch_name] @@ -732,6 +732,7 @@ before do stub_omniauth_setting(enabled: true, providers: [ommiauth_provider_config_with_step_up_auth]) + allow(Devise).to receive(:omniauth_providers).and_return([ommiauth_provider_config_with_step_up_auth.name]) end it { is_expected.to validate_inclusion_of(:step_up_auth_required_oauth_provider).in_array([ommiauth_provider_config_with_step_up_auth.name]) } @@ -739,6 +740,99 @@ it { is_expected.to allow_value(ommiauth_provider_config_with_step_up_auth.name).for(:step_up_auth_required_oauth_provider) } it { is_expected.to allow_value('').for(:step_up_auth_required_oauth_provider) } it { is_expected.not_to allow_value('google_oauth2').for(:step_up_auth_required_oauth_provider).with_message('is not included in the list') } + + context 'when parent group defines step-up auth provider' do + let_it_be_with_reload(:subsubgroup) { create(:group, parent: subgroup) } + + subject { subsubgroup.namespace_settings } + + before do + group.namespace_settings.update!(step_up_auth_required_oauth_provider: 'openid_connect') + end + + it { is_expected.not_to allow_value('openid_connect').for(:step_up_auth_required_oauth_provider).with_message("cannot be changed because it is inherited from parent group \"#{group.name}\"") } + it { is_expected.to allow_value(nil).for(:step_up_auth_required_oauth_provider) } + end + end + end + + describe '#step_up_auth_required_oauth_provider_inherited_namespace_setting and #step_up_auth_required_oauth_provider_from_self_or_inherited' do + let_it_be_with_reload(:group) { group } + let_it_be_with_reload(:subgroup) { subgroup } + let_it_be_with_reload(:subsubgroup) { create(:group, parent: subgroup) } + + let(:step_up_provider_oidc) do + GitlabSettings::Options.new( + name: 'oidc', + step_up_auth: { + namespace: { + id_token: { + required: { acr: 'gold' } + } + } + } + ) + end + + let(:step_up_provider_oidc_aad) do + GitlabSettings::Options.new( + name: 'oidc_aad', + step_up_auth: { + namespace: { + id_token: { + required: { + acr: 'gold' + } + } + } + } + ) + end + + before do + stub_omniauth_setting(enabled: true, providers: [step_up_provider_oidc, step_up_provider_oidc_aad]) + allow(Devise).to receive(:omniauth_providers).and_return([step_up_provider_oidc.name, step_up_provider_oidc_aad.name]) + + subsubgroup.namespace_settings.update!(step_up_auth_required_oauth_provider: subsubgroup_step_up_provider) + subgroup.namespace_settings.update!(step_up_auth_required_oauth_provider: subgroup_step_up_provider) + group.namespace_settings.update!(step_up_auth_required_oauth_provider: group_step_up_provider) + end + + where(:group_step_up_provider, :subgroup_step_up_provider, :subsubgroup_step_up_provider, :test_group, :expected_inherited_namespace, :expected_provider_from_self_or_inherited) do + # Test inheritance precedence (most distant ancestor wins) + 'oidc_aad' | 'oidc' | nil | ref(:subsubgroup) | ref(:group) | 'oidc_aad' + 'oidc_aad' | nil | nil | ref(:subsubgroup) | ref(:group) | 'oidc_aad' + nil | 'oidc' | nil | ref(:subsubgroup) | ref(:subgroup) | 'oidc' + + # Test own value takes precedence over inheritance + nil | 'oidc' | 'oidc' | ref(:subsubgroup) | ref(:subgroup) | 'oidc' + nil | nil | 'oidc' | ref(:subsubgroup) | nil | 'oidc' + + # Test middle level inheritance + 'oidc_aad' | nil | nil | ref(:subgroup) | ref(:group) | 'oidc_aad' + nil | 'oidc' | nil | ref(:subgroup) | nil | 'oidc' + + # Test root level (no inheritance possible) + 'oidc_aad' | nil | nil | ref(:group) | nil | 'oidc_aad' + + # Test no providers anywhere + nil | nil | nil | ref(:subsubgroup) | nil | nil + nil | nil | nil | ref(:subgroup) | nil | nil + nil | nil | nil | ref(:group) | nil | nil + end + + with_them do + describe '#step_up_auth_required_oauth_provider_inherited_namespace_setting' do + subject { test_group.namespace_settings.step_up_auth_required_oauth_provider_inherited_namespace_setting&.namespace } + + it { is_expected.to eq expected_inherited_namespace } + end + + describe '#step_up_auth_required_oauth_provider_from_self_or_inherited' do + subject { test_group.namespace_settings&.step_up_auth_required_oauth_provider_from_self_or_inherited } + + it { is_expected.to eq expected_provider_from_self_or_inherited } + end end end end diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 5b27f610d189b563e22890d90dfb49a2df3fe88f..9884f559338f2244abf017993e6739481bf2ab5c 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -669,6 +669,35 @@ expect(execute_update).to be_falsey end end + + context 'with inheritance from parent group' do + let_it_be_with_reload(:grandparent_group) { group } + let_it_be_with_reload(:parent_group) { create(:group, :private, parent: grandparent_group) } + let_it_be_with_reload(:group) { create(:group, :private, parent: parent_group) } + let_it_be(:user) { create(:user, owner_of: group) } + + before do + grandparent_group.namespace_settings.update!(step_up_auth_required_oauth_provider: 'openid_connect') + end + + context 'when updating to disabled (nil)' do + let(:params) { { step_up_auth_required_oauth_provider: nil } } + + it { is_expected.to be_truthy } + end + + context 'when updating with valid provider' do + let(:params) { { step_up_auth_required_oauth_provider: 'openid_connect_aad' } } + + it { is_expected.to be_falsey } + + it 'does not change step_up_auth_required_oauth_provider value' do + expect { execute_update } + .to not_change { group.reload.namespace_settings.step_up_auth_required_oauth_provider } + .and not_change { group.reload.namespace_settings.step_up_auth_required_oauth_provider_from_self_or_inherited } + end + end + end end context 'updating default_branch_protection' do