diff --git a/app/controllers/concerns/enforces_step_up_authentication_for_namespace.rb b/app/controllers/concerns/enforces_step_up_authentication_for_namespace.rb new file mode 100644 index 0000000000000000000000000000000000000000..63fdcb847fe2690b5c988c5681cd86b58fb0f88c --- /dev/null +++ b/app/controllers/concerns/enforces_step_up_authentication_for_namespace.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true + +# Enforces step-up authentication requirements for namespace access +# +# This controller concern ensures users complete step-up authentication +# before accessing group/namespace resources that require additional authentication. +# Include this module in group controllers to enforce the authentication check. +# +# Controllers including this concern must also include InternalRedirect for +# redirect location storage functionality. +# +# @example +# class Groups::ApplicationController < ApplicationController +# include EnforcesStepUpAuthenticationForNamespace +# +# before_action :enforce_step_up_auth_for_group +# +# private +# +# def enforce_step_up_auth_for_group +# enforce_step_up_auth_for(@group) if @group +# end +# end +module EnforcesStepUpAuthenticationForNamespace + extend ActiveSupport::Concern + + private + + def enforce_step_up_auth_for(namespace) + return if Feature.disabled?(:omniauth_step_up_auth_for_namespace, namespace) + return unless step_up_auth_required_for_namespace?(namespace) + return if step_up_auth_succeeded_for_namespace? + + handle_failed_step_up_auth_for_namespace(namespace) + end + + def step_up_auth_required_for_namespace?(namespace) + return false unless namespace.present? + return false unless namespace.step_up_auth_required_oauth_provider.present? + + ::Gitlab::Auth::Oidc::StepUpAuthentication.enabled_for_provider?( + provider_name: namespace.step_up_auth_required_oauth_provider, + scope: ::Gitlab::Auth::Oidc::StepUpAuthentication::SCOPE_NAMESPACE + ) + end + + def step_up_auth_succeeded_for_namespace? + ::Gitlab::Auth::Oidc::StepUpAuthentication.succeeded?( + session, + scope: ::Gitlab::Auth::Oidc::StepUpAuthentication::SCOPE_NAMESPACE + ) + end + + def handle_failed_step_up_auth_for_namespace(namespace) + redirect_to(new_group_step_up_auth_path(namespace), notice: build_namespace_step_up_auth_notice) + end + + def build_namespace_step_up_auth_notice + notice_message = _('Step-up authentication required.') + + documentation_links = documentation_links_for_failed_step_up_auth_providers( + ::Gitlab::Auth::Oidc::StepUpAuthentication::SCOPE_NAMESPACE.to_s + ) + return notice_message if documentation_links.blank? + + links_sentence = helpers.to_sentence(documentation_links) + + helpers.safe_join([ + notice_message, + ' ', + _('Learn more about authentication requirements: '), + links_sentence, + '.' + ]) + end + + def documentation_links_for_failed_step_up_auth_providers(scope) + # Get the list of failed step-up auth flows from the session + ::Gitlab::Auth::Oidc::StepUpAuthentication + .failed_step_up_auth_flows(session, scope: scope) + .filter_map do |flow| + helpers.step_up_auth_documentation_link(flow) + end + end +end diff --git a/app/controllers/groups/application_controller.rb b/app/controllers/groups/application_controller.rb index 2c38625fd1f767822ccc356f330bc49aee9976ea..b853106c1a185966f2f75b8e1111887f60794ab5 100644 --- a/app/controllers/groups/application_controller.rb +++ b/app/controllers/groups/application_controller.rb @@ -6,6 +6,12 @@ class Groups::ApplicationController < ApplicationController include SortingHelper include SortingPreference + # TODO: Include EnforcesStepUpAuthenticationForNamespace when finalizing the implementation + # of step-up authentication for groups. This is necessary in order to enforce additional + # OAuth-based authentication for sensitive group resources based on namespace configuration. + # See app/controllers/concerns/enforces_step_up_authentication_for_namespace.rb + # Discussion: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/207665#note_2825918460 + layout 'group' skip_before_action :authenticate_user! diff --git a/app/controllers/groups/step_up_auths_controller.rb b/app/controllers/groups/step_up_auths_controller.rb new file mode 100644 index 0000000000000000000000000000000000000000..34442c9ac37c4ee3dda9cd293188832057a961f2 --- /dev/null +++ b/app/controllers/groups/step_up_auths_controller.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +module Groups + class StepUpAuthsController < Groups::ApplicationController + include InternalRedirect + + before_action :require_user! + + feature_category :system_access + + def new + unless step_up_auth_succeeded? + store_location_for(:redirect, redirect_path) + return + end + + redirect_to redirect_path, notice: _('Step-up authentication already completed') + end + + private + + def require_user! + render_404 unless current_user + end + + def step_up_auth_succeeded? + Feature.enabled?(:omniauth_step_up_auth_for_namespace, current_user) && + ::Gitlab::Auth::Oidc::StepUpAuthentication.succeeded?(session, scope: :namespace) + end + + def redirect_path + safe_redirect_path(stored_location_for(:redirect)) || group_path(group) + end + end +end diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb index 9a24d5fa4ce8b599ed5007e292e599e848c20e42..f2f5331ab8e8cf0dc3e8bb1a16f440fb94c4edec 100644 --- a/app/controllers/projects/application_controller.rb +++ b/app/controllers/projects/application_controller.rb @@ -6,6 +6,12 @@ class Projects::ApplicationController < ApplicationController include RoutableActions include SafeFormatHelper + # TODO: Include EnforcesStepUpAuthenticationForNamespace when finalizing the implementation + # of step-up authentication for projects. This is necessary in order to enforce additional + # OAuth-based authentication for sensitive project resources based on namespace configuration. + # See app/controllers/concerns/enforces_step_up_authentication_for_namespace.rb + # Discussion: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/207665#note_2825918460 + skip_before_action :authenticate_user! before_action :project before_action :repository diff --git a/app/helpers/auth_helper.rb b/app/helpers/auth_helper.rb index 30d94f9cd60084cd182771585e6632f5a1787900..bc26cd46681abe1971e32910d193e645a1398f66 100644 --- a/app/helpers/auth_helper.rb +++ b/app/helpers/auth_helper.rb @@ -166,7 +166,13 @@ def button_based_providers_enabled? end def step_up_auth_params(provider_name, step_up_auth_scope) - return {} if Feature.disabled?(:omniauth_step_up_auth_for_admin_mode, current_user) + return {} if step_up_auth_scope.blank? + + return {} if Feature.disabled?(:omniauth_step_up_auth_for_admin_mode, current_user) && + step_up_auth_scope.to_s == ::Gitlab::Auth::Oidc::StepUpAuthentication::SCOPE_ADMIN_MODE.to_s + + return {} if Feature.disabled?(:omniauth_step_up_auth_for_namespace, current_user) && + step_up_auth_scope.to_s == ::Gitlab::Auth::Oidc::StepUpAuthentication::SCOPE_NAMESPACE.to_s # Get provider configuration for step up auth scope provider_config = Gitlab::Auth::OAuth::Provider diff --git a/app/views/devise/shared/_omniauth_box.html.haml b/app/views/devise/shared/_omniauth_box.html.haml index 8918c8d6adaba782a3808e61c8110f3a13ec6e92..ff12dd8f16048ebf1c87f5ac4be9882e8222a7af 100644 --- a/app/views/devise/shared/_omniauth_box.html.haml +++ b/app/views/devise/shared/_omniauth_box.html.haml @@ -2,7 +2,8 @@ - step_up_auth_scope = local_assigns[:step_up_auth_scope] - if any_form_based_providers_enabled? || password_authentication_enabled_for_web? - = render 'shared/divider', text: _("or sign in with") + - if step_up_auth_scope.blank? || step_up_auth_scope == 'admin_mode' + = render 'shared/divider', text: _("or sign in with") .gl-mt-5.gl-text-center.gl-flex.gl-flex-col.gl-gap-3.js-oauth-login - enabled_button_based_providers.each do |provider| diff --git a/app/views/groups/step_up_auths/new.html.haml b/app/views/groups/step_up_auths/new.html.haml new file mode 100644 index 0000000000000000000000000000000000000000..c327444cdb06b8e0fad7aa90cb61d16ccf3f0787 --- /dev/null +++ b/app/views/groups/step_up_auths/new.html.haml @@ -0,0 +1,11 @@ +- page_title _('Step-up authentication required') +- add_page_specific_style 'page_bundles/login' + +.row.gl-mt-5.justify-content-center + .gl-col-md-5 + .login-page + %h3.gl-mb-4= _('Step-up authentication required') + %p.gl-text-secondary= _('This group requires additional authentication. Please sign in with your OAuth provider to continue.') + + - if omniauth_enabled? && button_based_providers_enabled? + = render 'devise/shared/omniauth_box', render_remember_me: false, step_up_auth_scope: Gitlab::Auth::Oidc::StepUpAuthentication::SCOPE_NAMESPACE diff --git a/config/routes/group.rb b/config/routes/group.rb index d4ea6bfac4330878d46be5a31059cb1fda40078f..7ddf341a42904648d2d9471ea85ef73c1f7280a0 100644 --- a/config/routes/group.rb +++ b/config/routes/group.rb @@ -197,6 +197,8 @@ end resources :observability, only: [:show] + resources :step_up_auths, only: [:new] + post :preview_markdown post '/restore' => '/groups#restore', as: :restore diff --git a/locale/gitlab.pot b/locale/gitlab.pot index e83165c792055461b4f88933450d213e637c0775..b303bdda59af8852a65848cb75aa8cadb2885ce6 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -38561,6 +38561,9 @@ msgstr "" msgid "Learn more about X.509 signed commits" msgstr "" +msgid "Learn more about authentication requirements: " +msgstr "" + msgid "Learn more about custom project templates" msgstr "" @@ -64378,6 +64381,15 @@ msgstr "" msgid "Step 4." msgstr "" +msgid "Step-up authentication already completed" +msgstr "" + +msgid "Step-up authentication required" +msgstr "" + +msgid "Step-up authentication required." +msgstr "" + msgid "Still loading…" msgstr "" @@ -67310,6 +67322,9 @@ msgstr "" msgid "This group is scheduled to be deleted on %{date}. You are about to delete this group, including its subgroups and projects, immediately. This action cannot be undone." msgstr "" +msgid "This group requires additional authentication. Please sign in with your OAuth provider to continue." +msgstr "" + msgid "This group will be deleted on %{date} because its parent group is scheduled for deletion." msgstr "" diff --git a/spec/controllers/concerns/enforces_step_up_authentication_for_namespace_spec.rb b/spec/controllers/concerns/enforces_step_up_authentication_for_namespace_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..c3a0d107d1c9a19f3f1c7fe4487f0685824ae67b --- /dev/null +++ b/spec/controllers/concerns/enforces_step_up_authentication_for_namespace_spec.rb @@ -0,0 +1,157 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe EnforcesStepUpAuthenticationForNamespace, feature_category: :system_access do + controller(Groups::ApplicationController) do + # InternalRedirect is required by EnforcesStepUpAuthenticationForNamespace + include InternalRedirect + include EnforcesStepUpAuthenticationForNamespace + + before_action :enforce_step_up_auth_for_namespace + + def index + head :ok + end + + private + + def enforce_step_up_auth_for_namespace + enforce_step_up_auth_for(group) if group + end + end + + let_it_be(:group) { create(:group) } + let_it_be(:user) { create(:user, owner_of: group) } + + subject do + get :index, params: { group_id: group.to_param } + response + end + + before do + sign_in(user) + end + + shared_examples 'passing check for namespace step up authentication' do + it { is_expected.to have_gitlab_http_status(:ok) } + end + + shared_examples 'redirecting to new_group_step_up_auth_path' do + it { is_expected.to redirect_to(new_group_step_up_auth_path(group)) } + + context 'when feature flag :omniauth_step_up_auth_for_namespace is disabled' do + before do + stub_feature_flags(omniauth_step_up_auth_for_namespace: false) + end + + it_behaves_like 'passing check for namespace step up authentication' + end + end + + shared_examples 'redirecting to new_group_step_up_auth_path with documentation link in notice' do + it_behaves_like 'redirecting to new_group_step_up_auth_path' + end + + describe '#enforce_step_up_auth_for' do + using RSpec::Parameterized::TableSyntax + + let(:example_doc_link) { 'https://example.com/company-internal-docs-for-step-up-auth' } + + let(:provider_oidc_no_step_up) { GitlabSettings::Options.new(name: 'oidc_no_step_up') } + let(:provider_oidc) do + GitlabSettings::Options.new( + name: 'oidc', + step_up_auth: { + namespace: { + id_token: { + required: { + acr: 'silver' + } + } + } + } + ) + end + + let(:provider_oidc_and_doc_link) do + GitlabSettings::Options.new( + name: 'oidc_and_doc_link', + step_up_auth: { + namespace: { + documentation_link: example_doc_link, + id_token: { + required: { + acr: 'silver' + } + } + } + } + ) + end + + let(:session_oidc_succeeded) do + { 'oidc' => { 'namespace' => { 'state' => 'succeeded' } } } + end + + let(:session_oidc_failed) do + { 'oidc' => { 'namespace' => { 'state' => 'failed' } } } + end + + let(:session_oidc_and_doc_link_failed) do + { 'oidc_and_doc_link' => { 'namespace' => { 'state' => 'failed' } } } + end + + before do + stub_omniauth_setting(enabled: true, providers: oauth_providers) + allow(Devise).to receive(:omniauth_providers).and_return(oauth_providers.map(&:name)) + + session[:omniauth_step_up_auth] = step_up_auth_session + + # Only set the provider if it's valid (has step-up auth configured) + if required_provider && + ::Gitlab::Auth::Oidc::StepUpAuthentication + .enabled_providers(scope: :namespace) + .include?(required_provider) + group.update!(step_up_auth_required_oauth_provider: required_provider) + end + end + + # rubocop:disable Layout/LineLength -- Avoid formatting table to keep oneline table syntax + where(:oauth_providers, :required_provider, :step_up_auth_session, :expected_examples) do + [] | nil | nil | 'passing check for namespace step up authentication' + [] | nil | ref(:session_oidc_succeeded) | 'passing check for namespace step up authentication' + [] | nil | ref(:session_oidc_failed) | 'passing check for namespace step up authentication' + [ref(:provider_oidc_no_step_up)] | nil | nil | 'passing check for namespace step up authentication' + + [ref(:provider_oidc), ref(:provider_oidc_no_step_up)] | 'oidc' | nil | 'redirecting to new_group_step_up_auth_path' + [ref(:provider_oidc), ref(:provider_oidc_and_doc_link), ref(:provider_oidc_no_step_up)] | 'oidc_and_doc_link' | ref(:session_oidc_and_doc_link_failed) | 'redirecting to new_group_step_up_auth_path with documentation link in notice' + [ref(:provider_oidc)] | 'oidc' | ref(:session_oidc_succeeded) | 'passing check for namespace step up authentication' + end + # rubocop:enable Layout/LineLength + + with_them do + it_behaves_like params[:expected_examples] + end + + context 'when group does not require step-up auth' do + let(:oauth_providers) { [provider_oidc] } + let(:required_provider) { nil } + let(:step_up_auth_session) { nil } + + it_behaves_like 'passing check for namespace step up authentication' + end + + context 'when group is not defined' do + let(:oauth_providers) { [provider_oidc] } + let(:required_provider) { 'oidc' } + let(:step_up_auth_session) { nil } + + before do + allow(controller).to receive(:group).and_return(nil) + end + + it_behaves_like 'passing check for namespace step up authentication' + end + end +end diff --git a/spec/helpers/auth_helper_spec.rb b/spec/helpers/auth_helper_spec.rb index 149b4543fe32064e250a4270c34490b828bec042..0ad8e3ed23fe81ec36a82e17bda81490a76ee603 100644 --- a/spec/helpers/auth_helper_spec.rb +++ b/spec/helpers/auth_helper_spec.rb @@ -758,7 +758,7 @@ def auth_active? let(:current_user) { instance_double('User', flipper_id: '1') } - let(:oidc_setting_with_step_up_auth) do + let(:oidc_setting_with_admin_mode_step_up) do GitlabSettings::Options.new( name: "openid_connect", step_up_auth: { @@ -772,12 +772,34 @@ def auth_active? ) end - let(:oidc_setting_without_step_up_auth_params) do - GitlabSettings::Options.new(name: "openid_connect", - step_up_auth: { admin_mode: { id_token: { required: { acr: 'gold' } } } }) + let(:oidc_setting_with_namespace_step_up) do + GitlabSettings::Options.new( + name: "openid_connect", + step_up_auth: { + namespace: { + params: { + claims: { acr_values: 'silver' }, + prompt: 'login' + } + } + } + ) + end + + let(:oidc_setting_without_step_up_params) do + GitlabSettings::Options.new( + name: "openid_connect", + step_up_auth: { + admin_mode: { + id_token: { + required: { acr: 'gold' } + } + } + } + ) end - let(:oidc_setting_without_step_up_auth) do + let(:oidc_setting_without_step_up) do GitlabSettings::Options.new(name: "openid_connect") end @@ -785,32 +807,73 @@ def auth_active? before do allow(helper).to receive(:current_user).and_return(current_user) + stub_omniauth_setting(enabled: true, providers: omniauth_setting_providers) end # rubocop:disable Layout/LineLength -- Ensure one-line table syntax for better readability - where(:omniauth_setting_providers, :provider_name, :scope, :expected_result) do - [ref(:oidc_setting_with_step_up_auth)] | 'openid_connect' | :admin_mode | { step_up_auth_scope: :admin_mode, 'claims' => '{"acr_values":"gold"}', 'prompt' => 'login' } - [ref(:oidc_setting_with_step_up_auth)] | 'openid_connect' | 'admin_mode' | { step_up_auth_scope: 'admin_mode', 'claims' => '{"acr_values":"gold"}', 'prompt' => 'login' } - [ref(:oidc_setting_with_step_up_auth)] | 'openid_connect' | nil | {} - [ref(:oidc_setting_with_step_up_auth)] | 'missing_provider_name' | :admin_mode | {} - [ref(:oidc_setting_with_step_up_auth)] | nil | nil | {} + where(:omniauth_setting_providers, :provider_name, :scope, :expected_params) do + [ref(:oidc_setting_with_admin_mode_step_up)] | 'openid_connect' | :admin_mode | { step_up_auth_scope: :admin_mode, 'claims' => '{"acr_values":"gold"}', 'prompt' => 'login' } + [ref(:oidc_setting_with_admin_mode_step_up)] | 'openid_connect' | 'admin_mode' | { step_up_auth_scope: 'admin_mode', 'claims' => '{"acr_values":"gold"}', 'prompt' => 'login' } + [ref(:oidc_setting_with_admin_mode_step_up)] | 'openid_connect' | nil | {} + [ref(:oidc_setting_with_admin_mode_step_up)] | 'missing_provider_name' | :admin_mode | {} + [ref(:oidc_setting_with_admin_mode_step_up)] | nil | nil | {} + + [] | 'openid_connect' | :admin_mode | {} + [ref(:oidc_setting_without_step_up)] | 'openid_connect' | :admin_mode | {} + [ref(:oidc_setting_without_step_up_params)] | 'openid_connect' | :admin_mode | { step_up_auth_scope: :admin_mode } - [] | 'openid_connect' | :admin_mode | {} - [ref(:oidc_setting_without_step_up_auth)] | 'openid_connect' | :admin_mode | {} - [ref(:oidc_setting_without_step_up_auth_params)] | 'openid_connect' | :admin_mode | { step_up_auth_scope: :admin_mode } + [ref(:oidc_setting_with_namespace_step_up)] | 'openid_connect' | :namespace | { step_up_auth_scope: :namespace, 'claims' => '{"acr_values":"silver"}', 'prompt' => 'login' } + [ref(:oidc_setting_with_namespace_step_up)] | 'openid_connect' | 'namespace' | { step_up_auth_scope: 'namespace', 'claims' => '{"acr_values":"silver"}', 'prompt' => 'login' } + [ref(:oidc_setting_without_step_up)] | 'openid_connect' | :namespace | {} end # rubocop:enable Layout/LineLength with_them do - it { is_expected.to eq expected_result } + it { is_expected.to eq expected_params } + end - context 'when feature flag :omniauth_step_up_auth_for_admin_mode is disabled' do - before do - stub_feature_flags(omniauth_step_up_auth_for_admin_mode: false) + context 'when feature flag :omniauth_step_up_auth_for_admin_mode is disabled' do + before do + stub_feature_flags(omniauth_step_up_auth_for_admin_mode: false) + end + + let(:omniauth_setting_providers) { [oidc_setting_with_admin_mode_step_up] } + let(:provider_name) { 'openid_connect' } + let(:scope) { :admin_mode } + + it { is_expected.to eq({}) } + + context 'when scope :namespace is given' do + let(:omniauth_setting_providers) { [oidc_setting_with_namespace_step_up] } + let(:scope) { :namespace } + + it 'works as expected because feature flag for scope :admin_mode is disabled' do + is_expected + .to eq({ step_up_auth_scope: :namespace, 'claims' => '{"acr_values":"silver"}', 'prompt' => 'login' }) end + end + end + + context 'when feature flag :omniauth_step_up_auth_for_namespace is disabled' do + before do + stub_feature_flags(omniauth_step_up_auth_for_namespace: false) + end - it { is_expected.to eq({}) } + let(:omniauth_setting_providers) { [oidc_setting_with_namespace_step_up] } + let(:provider_name) { 'openid_connect' } + let(:scope) { :namespace } + + it { is_expected.to eq({}) } + + context 'when scope :admin_mode is given' do + let(:omniauth_setting_providers) { [oidc_setting_with_admin_mode_step_up] } + let(:scope) { :admin_mode } + + it 'works as expected because feature flag for scope :namespace' do + is_expected + .to eq({ step_up_auth_scope: :admin_mode, 'claims' => '{"acr_values":"gold"}', 'prompt' => 'login' }) + end end end end diff --git a/spec/requests/groups/step_up_auths_controller_spec.rb b/spec/requests/groups/step_up_auths_controller_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..9f004b452e173700e8afd076f3a9798fa7636305 --- /dev/null +++ b/spec/requests/groups/step_up_auths_controller_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Groups::StepUpAuthsController, type: :controller, feature_category: :system_access do + let_it_be(:group) { create(:group) } + let_it_be(:user) { create(:user, owner_of: group) } + + before do + sign_in(user) + end + + describe 'GET #new' do + render_views false + + subject(:make_request) { get :new, params: { group_id: group.to_param } } + + context 'when user is not authenticated' do + before do + sign_out(user) + end + + it 'returns 404' do + make_request + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'when user is authenticated' do + it 'returns success' do + make_request + + expect(response).to have_gitlab_http_status(:ok) + end + + context 'when step-up auth already succeeded' do + before do + allow(Gitlab::Auth::Oidc::StepUpAuthentication).to receive(:succeeded?).and_return(true) + end + + it 'redirects with notice' do + make_request + + expect(response).to redirect_to(group_path(group)) + expect(flash[:notice]).to eq('Step-up authentication already completed') + end + end + end + end +end