From 8c794ba243619abf3ea4bd0c8a8492f83318f036 Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Thu, 19 Dec 2024 17:31:34 +0100 Subject: [PATCH] Step-up auth: Protect group routes with step-up-enabled oauth provider Step-up authentication is a security feature that requires additional verification for accessing sensitive data or performing critical actions. In a previous MR, we introduced step-up authentication for the admin mode, see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/171643. In this MR, we extend the step-up authentication for group-specific routes. When a namespace (group) is protected with step-up authentication, then only group-specific routes for this group require step-up authentication. This commit includes the following aspects: - Introduced a new concern `EnforcesNamespaceStepUpAuthentication` to encapsulate step-up authentication logic for namespaces. - Updated the `Groups::ApplicationController` to include new concern, ensuring all group-specific routes are protected. - Created a new controller `Groups::StepUpAuthsController` to handle step-up authentication flows for groups. - Modified existing controllers to integrate with the step-up authentication flow. - Added a new view for step-up authentication when users attempt to access group routes that require step-up auth. Changelog: added --- ...forces_namespace_step_up_authentication.rb | 47 ++++ .../groups/application_controller.rb | 6 + .../groups/step_up_auths_controller.rb | 30 +++ app/controllers/groups_controller.rb | 1 + .../omniauth_callbacks_controller.rb | 15 +- app/helpers/auth_helper.rb | 8 +- app/views/groups/step_up_auths/new.html.haml | 6 + config/routes/group.rb | 2 + .../oidc/step_up_auth_before_request_phase.rb | 34 ++- .../auth/oidc/step_up_authentication_flow.rb | 5 + locale/gitlab.pot | 12 + ...s_namespace_step_up_authentication_spec.rb | 106 ++++++++ .../groups/application_controller_spec.rb | 92 ++++++- spec/features/groups/step_up_auth_spec.rb | 253 ++++++++++++++++++ spec/helpers/auth_helper_spec.rb | 11 +- .../oidc/step_up_authentication_flow_spec.rb | 10 + .../auth/oidc/step_up_authentication_spec.rb | 8 +- spec/requests/groups/step_up_auths_spec.rb | 19 ++ 18 files changed, 646 insertions(+), 19 deletions(-) create mode 100644 app/controllers/concerns/enforces_namespace_step_up_authentication.rb create mode 100644 app/controllers/groups/step_up_auths_controller.rb create mode 100644 app/views/groups/step_up_auths/new.html.haml create mode 100644 spec/controllers/concerns/enforces_namespace_step_up_authentication_spec.rb create mode 100644 spec/features/groups/step_up_auth_spec.rb create mode 100644 spec/requests/groups/step_up_auths_spec.rb diff --git a/app/controllers/concerns/enforces_namespace_step_up_authentication.rb b/app/controllers/concerns/enforces_namespace_step_up_authentication.rb new file mode 100644 index 00000000000000..d9042410fbb417 --- /dev/null +++ b/app/controllers/concerns/enforces_namespace_step_up_authentication.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +# +# Enforces step-up authentication requirements for namespaces +# +# This module provides functionality to enforce additional authentication +# requirements for specific namespaces based on their OAuth provider settings. +# It integrates with the OIDC step-up authentication flow to ensure users +# complete the required authentication steps before accessing namespace resources. +# +# @example +# class Groups::ApplicationController < ApplicationController +# include EnforcesNamespaceStepUpAuthentication +# +# 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 EnforcesNamespaceStepUpAuthentication + extend ActiveSupport::Concern + + def enforce_step_up_auth_for(namespace) + return if Feature.disabled?(:omniauth_step_up_auth_for_namespace, namespace) + return if namespace.blank? + return if namespace.namespace_settings&.step_up_auth_required_oauth_provider.blank? + + return if step_up_auth_flow_succeeded?(namespace) + + # Store current location to redirect back after successful step-up auth + # Note: We use the scope :redirect because this scope is the default scope used by the application controller, + # see https://gitlab.com/gitlab-community/gitlab-org/gitlab/-/blob/master/app/controllers/application_controller.rb#L203 + store_location_for(:redirect, request.fullpath) + redirect_to new_group_step_up_auth_path(namespace), notice: _('Step-up auth not successful') + end + + def step_up_auth_flow_succeeded?(namespace) + ::Gitlab::Auth::Oidc::StepUpAuthentication.build_flow( + provider: namespace.namespace_settings&.step_up_auth_required_oauth_provider, + session: session, + scope: ::Gitlab::Auth::Oidc::StepUpAuthentication::STEP_UP_AUTH_SCOPE_NAMESPACE + ).succeeded? + end +end diff --git a/app/controllers/groups/application_controller.rb b/app/controllers/groups/application_controller.rb index 2c38625fd1f767..a6fd9b224bb6cb 100644 --- a/app/controllers/groups/application_controller.rb +++ b/app/controllers/groups/application_controller.rb @@ -2,6 +2,7 @@ class Groups::ApplicationController < ApplicationController include RoutableActions + include EnforcesNamespaceStepUpAuthentication include ControllerWithCrossProjectAccessCheck include SortingHelper include SortingPreference @@ -10,6 +11,7 @@ class Groups::ApplicationController < ApplicationController skip_before_action :authenticate_user! before_action :group + before_action :enforce_step_up_auth_for_namespace before_action :set_sorting requires_cross_project_access @@ -102,6 +104,10 @@ def method_missing(method_sym, *arguments, &block) super end end + + def enforce_step_up_auth_for_namespace + enforce_step_up_auth_for(@group) + end end Groups::ApplicationController.prepend_mod_with('Groups::ApplicationController') 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 00000000000000..dac809bb932c0c --- /dev/null +++ b/app/controllers/groups/step_up_auths_controller.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module Groups + class StepUpAuthsController < Groups::ApplicationController + include InternalRedirect + + before_action :authenticate_user! + skip_before_action :enforce_step_up_auth_for_namespace + + feature_category :system_access + + def new + # Check if step-up auth has already succeeded and redirect to stored location + return unless step_up_auth_flow_succeeded?(@group) + + redirect_path = stored_location_for(:redirect) || group_path(@group) + redirect_to redirect_path, notice: _('Step-up authentication successful') + end + + private + + def step_up_auth_flow_succeeded?(namespace) + ::Gitlab::Auth::Oidc::StepUpAuthentication.build_flow( + provider: namespace.namespace_settings&.step_up_auth_required_oauth_provider, + session: session, + scope: ::Gitlab::Auth::Oidc::StepUpAuthentication::STEP_UP_AUTH_SCOPE_NAMESPACE + ).succeeded? + end + end +end diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 22ed8321e84764..12f24a6d898511 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -18,6 +18,7 @@ class GroupsController < Groups::ApplicationController before_action :authenticate_user!, only: [:new, :create] before_action :group, except: [:index, :new, :create] + before_action :enforce_step_up_auth_for_namespace, except: [:index, :new, :create] # Authorize before_action :authorize_admin_group!, only: [:update, :transfer, :export, :download_export] diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index fc06c348ad8e4e..3a347e2c0d3fdc 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -426,10 +426,21 @@ def store_redirect_fragment(redirect_fragment) end def handle_step_up_auth - return if Feature.disabled?(:omniauth_step_up_auth_for_admin_mode, current_user) + handle_step_up_auth_for_admin_mode if Feature.enabled?(:omniauth_step_up_auth_for_admin_mode, current_user) + handle_step_up_auth_for_namespace if Feature.enabled?(:omniauth_step_up_auth_for_namespace, current_user) + end + + def handle_step_up_auth_for_admin_mode + handle_step_up_auth_for(::Gitlab::Auth::Oidc::StepUpAuthentication::STEP_UP_AUTH_SCOPE_ADMIN_MODE) + end + + def handle_step_up_auth_for_namespace + handle_step_up_auth_for(::Gitlab::Auth::Oidc::StepUpAuthentication::STEP_UP_AUTH_SCOPE_NAMESPACE) + end + def handle_step_up_auth_for(scope) step_up_auth_flow = - ::Gitlab::Auth::Oidc::StepUpAuthentication.build_flow(session: session, provider: oauth.provider) + ::Gitlab::Auth::Oidc::StepUpAuthentication.build_flow(session: session, provider: oauth.provider, scope: scope) return unless step_up_auth_flow.enabled_by_config? return unless step_up_auth_flow.requested? diff --git a/app/helpers/auth_helper.rb b/app/helpers/auth_helper.rb index e92d9743d551f2..0570119b96a107 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 step_up_auth_scope.to_sym == :admin_mode && + Feature.disabled?(:omniauth_step_up_auth_for_admin_mode, current_user) + + return {} if step_up_auth_scope.to_sym == :namespace && + Feature.disabled?(:omniauth_step_up_auth_for_namespace, current_user) # Get provider configuration for step up auth scope provider_config = Gitlab::Auth::OAuth::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 00000000000000..d8a9abe0786703 --- /dev/null +++ b/app/views/groups/step_up_auths/new.html.haml @@ -0,0 +1,6 @@ +%h1 + = s_('Step-up Auth|Sign in') +%p + = s_('Step-up Auth|Step-up authentication is required to access this group. Please sign in with your group account.') + += render 'devise/shared/omniauth_box', render_remember_me: false, step_up_auth_scope: Gitlab::Auth::Oidc::StepUpAuthentication::STEP_UP_AUTH_SCOPE_NAMESPACE diff --git a/config/routes/group.rb b/config/routes/group.rb index 072a9fb6395156..b995acc7b5fc4e 100644 --- a/config/routes/group.rb +++ b/config/routes/group.rb @@ -197,6 +197,8 @@ end resources :observability, only: [:show] + resource :step_up_auth, only: [:new] + post :preview_markdown post '/restore' => '/groups#restore', as: :restore diff --git a/lib/gitlab/auth/oidc/step_up_auth_before_request_phase.rb b/lib/gitlab/auth/oidc/step_up_auth_before_request_phase.rb index 2b75a371fc241f..d4f70472616df6 100644 --- a/lib/gitlab/auth/oidc/step_up_auth_before_request_phase.rb +++ b/lib/gitlab/auth/oidc/step_up_auth_before_request_phase.rb @@ -11,16 +11,30 @@ module StepUpAuthBeforeRequestPhase class << self def call(env) return if current_user_from(env).blank? - return if Feature.disabled?(:omniauth_step_up_auth_for_admin_mode, current_user_from(env)) - # If the step-up authentication scope is not included in the request params, - # then step-up authentication is likely not requested and we do not need to proceed. - return unless step_up_auth_requested_for_admin_mode?(env) + return {} if requested_step_up_auth_scope_from(env).blank? + + return {} if requested_step_up_auth_scope_from(env).to_sym == :admin_mode && + Feature.disabled?(:omniauth_step_up_auth_for_admin_mode, current_user) + + return {} if requested_step_up_auth_scope_from(env).to_sym == :namespace && + Feature.disabled?(:omniauth_step_up_auth_for_namespace, current_user) + + return unless allowed_step_up_auth_scope_from(env) session = session_from(env) + provider = current_provider_from(env) step_up_auth_flow = - ::Gitlab::Auth::Oidc::StepUpAuthentication.build_flow(session: session, provider: provider) + ::Gitlab::Auth::Oidc::StepUpAuthentication.build_flow( + session: session, + provider: provider, + scope: requested_step_up_auth_scope_from(env) + ) + + # TODO: Integrate the state uuid in the step-up auth session. + # Why? At the moment, there is a small security vulnerability + # where simultaneous authentication requests could lead to privilege escalation, https://gitlab.com/gitlab-org/gitlab/-/issues/555349. return unless step_up_auth_flow.enabled_by_config? @@ -30,9 +44,9 @@ def call(env) private - def step_up_auth_requested_for_admin_mode?(env) - request_param_step_up_auth_scope_from(env) == - ::Gitlab::Auth::Oidc::StepUpAuthentication::STEP_UP_AUTH_SCOPE_ADMIN_MODE.to_s + def allowed_step_up_auth_scope_from(env) + ::Gitlab::Auth::Oidc::StepUpAuthentication::ALLOWED_SCOPES + .include?(request_param_step_up_auth_scope_from(env).to_sym) end def current_user_from(env) @@ -43,6 +57,10 @@ def current_provider_from(env) env['omniauth.strategy']&.name end + def requested_step_up_auth_scope_from(env) + request_param_step_up_auth_scope_from(env) + end + def request_param_step_up_auth_scope_from(env) env.dig('rack.request.query_hash', 'step_up_auth_scope').to_s end diff --git a/lib/gitlab/auth/oidc/step_up_authentication_flow.rb b/lib/gitlab/auth/oidc/step_up_authentication_flow.rb index 7699f30ee69a92..6b4428cb50fc2c 100644 --- a/lib/gitlab/auth/oidc/step_up_authentication_flow.rb +++ b/lib/gitlab/auth/oidc/step_up_authentication_flow.rb @@ -14,6 +14,11 @@ def initialize(session:, provider:, scope:) @session = session @provider = provider @scope = scope + + return if ::Gitlab::Auth::Oidc::StepUpAuthentication::ALLOWED_SCOPES.include?(scope.to_sym) + + raise ArgumentError, + "Invalid scope: #{scope}" end def requested? diff --git a/locale/gitlab.pot b/locale/gitlab.pot index d6b6db5c6907e6..e6176a353654c7 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -62789,6 +62789,18 @@ msgstr "" msgid "Step 4." msgstr "" +msgid "Step-up Auth|Sign in" +msgstr "" + +msgid "Step-up Auth|Step-up authentication is required to access this group. Please sign in with your group account." +msgstr "" + +msgid "Step-up auth not successful" +msgstr "" + +msgid "Step-up authentication successful" +msgstr "" + msgid "Still loading…" msgstr "" diff --git a/spec/controllers/concerns/enforces_namespace_step_up_authentication_spec.rb b/spec/controllers/concerns/enforces_namespace_step_up_authentication_spec.rb new file mode 100644 index 00000000000000..79b8b857d7a6f2 --- /dev/null +++ b/spec/controllers/concerns/enforces_namespace_step_up_authentication_spec.rb @@ -0,0 +1,106 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe EnforcesNamespaceStepUpAuthentication, feature_category: :system_access do + controller(ApplicationController) do + include EnforcesNamespaceStepUpAuthentication + + before_action do + namespace = Namespace.find_by(id: params[:namespace_id]) + enforce_step_up_auth_for(namespace) + end + + def index + head :ok + end + end + + let_it_be(:user) { create(:user) } + let_it_be_with_reload(:group) { create(:group) } + let_it_be(:project) { create(:project, group: group) } + + subject do + get :index, params: { namespace_id: namespace&.id } + response + end + + before do + sign_in(user) + end + + shared_examples 'passing check for 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(namespace)) } + + 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 step up authentication' + end + end + + describe '#enforce_step_up_auth_for' do + using RSpec::Parameterized::TableSyntax + + let(:provider_without_step_up_auth) { GitlabSettings::Options.new(name: 'google_oauth2') } + let(:provider_with_step_up_auth) do + GitlabSettings::Options.new( + name: 'openid_connect', + step_up_auth: { + namespace: { + id_token: { + required: { + acr: 'gold' + } + } + } + } + ) + end + + let(:step_up_auth_session_succeeded) do + { 'openid_connect' => { 'namespace' => { 'state' => 'succeeded' } } } + end + + let(:step_up_auth_session_failed) do + { 'openid_connect' => { 'namespace' => { 'state' => 'rejected' } } } + end + + before do + stub_omniauth_setting(enabled: true, providers: oauth_providers) + + session[:omniauth_step_up_auth] = step_up_auth_session + + group.namespace_settings.update!(step_up_auth_required_oauth_provider: namespace_step_up_provider) + end + + # rubocop:disable Layout/LineLength -- Avoid formatting table to keep oneline table syntax + where(:namespace, :oauth_providers, :step_up_auth_session, :namespace_step_up_provider, :expected_examples) do + ref(:group) | [] | nil | nil | 'passing check for step up authentication' + ref(:group) | [] | ref(:step_up_auth_session_succeeded) | nil | 'passing check for step up authentication' + ref(:group) | [] | ref(:step_up_auth_session_failed) | nil | 'passing check for step up authentication' + ref(:group) | [ref(:provider_without_step_up_auth)] | nil | nil | 'passing check for step up authentication' + + ref(:group) | [ref(:provider_with_step_up_auth), ref(:provider_without_step_up_auth)] | nil | 'openid_connect' | 'redirecting to new_group_step_up_auth_path' + ref(:group) | [ref(:provider_with_step_up_auth)] | nil | 'openid_connect' | 'redirecting to new_group_step_up_auth_path' + ref(:group) | [ref(:provider_with_step_up_auth)] | ref(:step_up_auth_session_succeeded) | 'openid_connect' | 'passing check for step up authentication' + ref(:group) | [ref(:provider_with_step_up_auth)] | ref(:step_up_auth_session_failed) | 'openid_connect' | 'redirecting to new_group_step_up_auth_path' + + nil | [] | nil | nil | 'passing check for step up authentication' + nil | [ref(:provider_with_step_up_auth)] | nil | 'openid_connect' | 'passing check for step up authentication' + nil | [ref(:provider_with_step_up_auth)] | ref(:step_up_auth_session_succeeded) | 'openid_connect' | 'passing check for step up authentication' + nil | [ref(:provider_with_step_up_auth)] | ref(:step_up_auth_session_failed) | 'openid_connect' | 'passing check for step up authentication' + end + # rubocop:enable Layout/LineLength + + with_them do + it_behaves_like params[:expected_examples] + end + end +end diff --git a/spec/controllers/groups/application_controller_spec.rb b/spec/controllers/groups/application_controller_spec.rb index 46908fdb26a4cf..98bffe1fa82812 100644 --- a/spec/controllers/groups/application_controller_spec.rb +++ b/spec/controllers/groups/application_controller_spec.rb @@ -2,9 +2,9 @@ require 'spec_helper' -RSpec.describe Groups::ApplicationController do - let_it_be(:user) { create(:user) } - let_it_be(:group) { create(:group) } +RSpec.describe Groups::ApplicationController, feature_category: :groups_and_projects do + let_it_be(:user) { create(:user) } + let_it_be_with_reload(:group) { create(:group) } describe '#respond_to_missing?' do it 'returns true if the method matches the name structure' do @@ -42,4 +42,90 @@ def index expect(response).to have_gitlab_http_status(:ok) end end + + context 'for step-up auth' do + using RSpec::Parameterized::TableSyntax + + controller do + before_action :authenticate_user! + + def any_action + head :ok + end + end + + let(:oidc_step_up_auth_provider_config) do + GitlabSettings::Options.new( + name: "openid_connect", + step_up_auth: { + namespace: { + id_token: { + required: { acr: 'gold' } + } + } + } + ) + end + + let(:oidc_step_up_auth_provider_name) do + oidc_step_up_auth_provider_config.name + end + + let(:oauth_session_step_up_succeeded) do + { 'omniauth_step_up_auth' => { + oidc_step_up_auth_provider_name => { 'namespace' => { 'state' => 'succeeded' } } + } } + end + + let(:oauth_session_step_up_rejected) do + { 'omniauth_step_up_auth' => { oidc_step_up_auth_provider_name => { 'namespace' => { 'state' => 'rejected' } } } } + end + + before do + routes.draw { get 'any_action' => 'groups/application#any_action' } + + stub_omniauth_setting(enabled: true, providers: [oidc_step_up_auth_provider_config]) + + group.namespace_settings.update!(step_up_auth_required_oauth_provider: step_up_auth_required_oauth_provider) + + sign_in(user) + end + + subject { get :any_action, params: { group_id: group.to_param }, session: oauth_session_data } + + shared_examples 'passing step-up authentication' do + it { is_expected.to have_gitlab_http_status(:ok) } + + it_behaves_like 'skipping step-up authentication when feature flag disabled' + end + + shared_examples 'blocking step-up authentication and redirecting' do + it { is_expected.to redirect_to(new_group_step_up_auth_path(group)) } + + it_behaves_like 'skipping step-up authentication when feature flag disabled' + end + + shared_examples 'skipping step-up authentication when feature flag disabled' do + before do + stub_feature_flags(omniauth_step_up_auth_for_namespace: false) + end + + it { is_expected.to have_gitlab_http_status(:ok) } + end + + # rubocop:disable Layout/LineLength -- Acoid formatting to keep one-line table syntax + where(:step_up_auth_required_oauth_provider, :oauth_session_data, :expected_shared_example) do + nil | ref(:oauth_session_step_up_succeeded) | 'passing step-up authentication' + nil | nil | 'passing step-up authentication' + nil | ref(:oauth_session_step_up_rejected) | 'passing step-up authentication' + ref(:oidc_step_up_auth_provider_name) | ref(:oauth_session_step_up_succeeded) | 'passing step-up authentication' + ref(:oidc_step_up_auth_provider_name) | nil | 'blocking step-up authentication and redirecting' + ref(:oidc_step_up_auth_provider_name) | ref(:oauth_session_step_up_rejected) | 'blocking step-up authentication and redirecting' + end + # rubocop:enable Layout/LineLength + + with_them do + include_examples params[:expected_shared_example] + end + end end diff --git a/spec/features/groups/step_up_auth_spec.rb b/spec/features/groups/step_up_auth_spec.rb new file mode 100644 index 00000000000000..0f18e2fbfd0008 --- /dev/null +++ b/spec/features/groups/step_up_auth_spec.rb @@ -0,0 +1,253 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Group step-up authentication', :with_current_organization, :js, feature_category: :system_access do + let_it_be(:provider_oidc_extern_uid) { 'oidc_user_uid' } + let_it_be(:provider_oidc) { 'openid_connect' } + let_it_be_with_reload(:group) { create(:group) } + + let_it_be(:user) do + create(:omniauth_user, + password_automatically_set: false, + extern_uid: provider_oidc_extern_uid, + provider: provider_oidc, + developer_of: group + ) + end + + let(:provider_oidc_config_with_step_up_auth) do + GitlabSettings::Options.new( + name: provider_oidc, + step_up_auth: { + namespace: { + id_token: { + required: { acr: 'gold' } + } + } + } + ) + end + + let(:provider_oidc_config_without_step_up_auth) do + GitlabSettings::Options.new(name: provider_oidc) + end + + let(:additional_info_rejected_step_up_auth) { { extra: { raw_info: { acr: 'bronze' } } } } + let(:additional_info_success_step_up_auth) { { extra: { raw_info: { acr: 'gold' } } } } + + around do |example| + with_omniauth_full_host { example.run } + end + + context 'when group requires step-up authentication' do + before do + stub_omniauth_setting(enabled: true, auto_link_user: true, providers: [provider_oidc_config_with_step_up_auth]) + + group.namespace_settings.update!(step_up_auth_required_oauth_provider: provider_oidc) + + sign_in(user) + end + + context 'when step-up auth conditions are fulfilled' do + it 'user can access group page successfully after step-up auth' do + # First visit should redirect to step-up auth + visit group_path(group) + expect(page).to have_current_path(new_group_step_up_auth_path(group)) + + # Perform successful step-up auth + gitlab_group_step_up_auth_sign_in_via(provider_oidc, user, provider_oidc_extern_uid, + additional_info: additional_info_success_step_up_auth) + + # Should now be able to access the group page + visit group_path(group) + expect(page).to have_current_path(group_path(group)) + expect(page).to have_content(group.name) + end + + it 'user can navigate to group settings after successful step-up auth' do + # Perform successful step-up auth + visit group_path(group) + gitlab_group_step_up_auth_sign_in_via(provider_oidc, user, provider_oidc_extern_uid, + additional_info: additional_info_success_step_up_auth) + + # Navigate to different group pages + visit group_path(group) + expect(page).to have_current_path(group_path(group)) + + visit edit_group_path(group) + expect(page).to have_current_path(edit_group_path(group)) + + visit issues_group_path(group) + expect(page).to have_current_path(issues_group_path(group)) + end + + it 'user can navigate in and out of the group scope' do + # Perform successful step-up auth + visit group_path(group) + gitlab_group_step_up_auth_sign_in_via(provider_oidc, user, provider_oidc_extern_uid, + additional_info: additional_info_success_step_up_auth) + + # Visit group page + visit group_path(group) + expect(page).to have_current_path(group_path(group)) + + # Navigate away and back to group + visit root_path + expect(page).to have_current_path(root_path) + + visit group_path(group) + expect(page).to have_current_path(group_path(group)) + expect(page).not_to have_current_path(new_group_step_up_auth_path(group)) + 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 'user can access group without step-up auth' do + visit group_path(group) + + expect(page).to have_current_path(group_path(group)) + expect(page).to have_content(group.name) + end + end + + context 'when step-up auth conditions are not fulfilled' do + it 'user cannot access group and is redirected back to step-up auth page' do + # First visit should redirect to step-up auth + visit group_path(group) + expect(page).to have_current_path(new_group_step_up_auth_path(group)) + + # Perform failed step-up auth + gitlab_group_step_up_auth_sign_in_via(provider_oidc, user, provider_oidc_extern_uid, + additional_info: additional_info_rejected_step_up_auth) + + # Should be redirected back to step-up auth page + expect(page).to have_current_path(new_group_step_up_auth_path(group)) + end + + it 'user can retry step-up auth after initial failure' do + visit group_path(group) + + # First attempt - fail + gitlab_group_step_up_auth_sign_in_via(provider_oidc, user, provider_oidc_extern_uid, + additional_info: additional_info_rejected_step_up_auth) + expect(page).to have_current_path(new_group_step_up_auth_path(group)) + + # Second attempt - succeed + gitlab_group_step_up_auth_sign_in_via(provider_oidc, user, provider_oidc_extern_uid, + additional_info: additional_info_success_step_up_auth) + + # Should now be able to access group, e.g. accessing the group's issues page + expect(page).to have_current_path(group_path(group)) + expect(page).to have_content(group.name) + + visit issues_group_path(group) + expect(page).to have_current_path(issues_group_path(group)) + end + end + end + + context 'for different initial sign-in methods' do + shared_examples 'successful group step-up auth process' do + it 'user can access group after successful step-up auth process' do + wait_for_requests + expect(page).to have_current_path(root_path, ignore_query: true) + + # Try to access group - should redirect to step-up auth + visit group_path(group) + expect(page).to have_current_path(new_group_step_up_auth_path(group)) + + # Then succeed with step-up auth + gitlab_group_step_up_auth_sign_in_via(provider_oidc, user, provider_oidc_extern_uid, + additional_info: additional_info_success_step_up_auth) + + # Should now be able to access group + visit group_path(group) + expect(page).to have_current_path(group_path(group)) + expect(page).to have_content(group.name) + end + end + + before do + stub_omniauth_setting(enabled: true, auto_link_user: true, providers: [provider_oidc_config_with_step_up_auth]) + + group.namespace_settings.update!(step_up_auth_required_oauth_provider: provider_oidc) + end + + context 'when user signed in initially with username and password' do + before do + gitlab_sign_in(user) + end + + it_behaves_like 'successful group step-up auth process' + end + + context 'when user signed in initially with same omniauth provider (openid_connect)' do + before do + gitlab_sign_in_via(provider_oidc, user, provider_oidc_extern_uid) + end + + it_behaves_like 'successful group step-up auth process' + end + + context 'when user signed in initially with another omniauth provider (github)' do + let(:provider_github) { 'github' } + let(:provider_github_config) { GitlabSettings::Options.new(name: provider_github) } + let(:provider_github_extern_uid) { "github_user_uid" } + + before do + # Add both github and openid_connect identities to user + user.identities << create(:identity, provider: provider_github, extern_uid: provider_github_extern_uid) + + # Enable both providers + stub_omniauth_setting(enabled: true, auto_link_user: true, providers: [ + provider_oidc_config_with_step_up_auth, + provider_github_config + ]) + + gitlab_sign_in_via(provider_github, user, provider_github_extern_uid) + end + + it_behaves_like 'successful group step-up auth process' + end + end + + context 'when group does not require step-up authentication' do + before do + stub_omniauth_setting(enabled: true, auto_link_user: true, providers: [provider_oidc_config_without_step_up_auth]) + + sign_in(user) + end + + it 'user can access group directly without step-up auth' do + visit group_path(group) + expect(page).to have_current_path(group_path(group)) + expect(page).to have_content(group.name) + end + + context 'when group has no step_up_auth_required_oauth_provider configured' do + before do + group.namespace_settings.update!(step_up_auth_required_oauth_provider: nil) + end + + it 'user can access group directly' do + visit group_path(group) + expect(page).to have_current_path(group_path(group)) + expect(page).to have_content(group.name) + end + end + end + + private + + # Helper method for group step-up authentication + # This simulates the step-up auth flow for groups + def gitlab_group_step_up_auth_sign_in_via(provider, user, uid, additional_info: {}) + mock_auth_hash(provider, uid, user.email, additional_info: additional_info) + click_button Gitlab::Auth::OAuth::Provider.label_for(provider) + end +end diff --git a/spec/helpers/auth_helper_spec.rb b/spec/helpers/auth_helper_spec.rb index b2f64ff384c939..8b5ecbd8151042 100644 --- a/spec/helpers/auth_helper_spec.rb +++ b/spec/helpers/auth_helper_spec.rb @@ -738,7 +738,14 @@ def auth_active? claims: { acr_values: 'gold' }, prompt: 'login' } + }, + namespace: { + params: { + claims: { acr_values: 'silver' }, + prompt: 'login' + } } + } ) end @@ -766,10 +773,12 @@ def auth_active? [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 | {} + [ref(:oidc_setting_with_step_up_auth)] | 'openid_connect' | 'namespace' | { step_up_auth_scope: 'namespace', 'claims' => '{"acr_values":"silver"}', 'prompt' => 'login' } [] | '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_without_step_up_auth_params)] | 'openid_connect' | :namespace | {} end # rubocop:enable Layout/LineLength @@ -778,7 +787,7 @@ def auth_active? 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) + stub_feature_flags(omniauth_step_up_auth_for_admin_mode: false, omniauth_step_up_auth_for_namespace: false) end it { is_expected.to eq({}) } diff --git a/spec/lib/gitlab/auth/oidc/step_up_authentication_flow_spec.rb b/spec/lib/gitlab/auth/oidc/step_up_authentication_flow_spec.rb index 2bcf234489710b..49e1a3aff8987a 100644 --- a/spec/lib/gitlab/auth/oidc/step_up_authentication_flow_spec.rb +++ b/spec/lib/gitlab/auth/oidc/step_up_authentication_flow_spec.rb @@ -9,6 +9,16 @@ subject(:flow) { described_class.new(session: session, provider: provider, scope: scope) } + describe '#initialize' do + context 'when scope is invalid' do + let(:scope) { 'invalid_scope' } + + it 'raises argument error' do + expect { flow }.to raise_error(ArgumentError, 'Invalid scope: invalid_scope') + end + end + end + describe '#requested?, #succeeded?, #failed?' do using RSpec::Parameterized::TableSyntax diff --git a/spec/lib/gitlab/auth/oidc/step_up_authentication_spec.rb b/spec/lib/gitlab/auth/oidc/step_up_authentication_spec.rb index 9a734f610878d2..fc006c3a8b7e48 100644 --- a/spec/lib/gitlab/auth/oidc/step_up_authentication_spec.rb +++ b/spec/lib/gitlab/auth/oidc/step_up_authentication_spec.rb @@ -157,7 +157,7 @@ lazy { { 'openid_connect' => { 'admin_mode' => { 'state' => 'succeeded' } }, 'other_provider' => { 'admin_mode' => { 'state' => 'succeeded' } } } } | true lazy { { 'openid_connect' => { 'admin_mode' => { 'state' => 'failed' } }, 'other_provider' => { 'admin_mode' => { 'state' => 'failed' } } } } | false lazy { { 'openid_connect' => { 'admin_mode' => { 'state' => 'failed' } }, 'other_provider' => { 'admin_mode' => { 'state' => 'succeeded' } } } } | false - lazy { { 'openid_connect' => { 'other_mode' => { 'state' => 'succeeded' } }, 'other_provider' => { 'admin_mode' => { 'state' => 'failed' } } } } | false + lazy { { 'openid_connect' => { 'namespace' => { 'state' => 'succeeded' } }, 'other_provider' => { 'admin_mode' => { 'state' => 'failed' } } } } | false nil | false {} | false @@ -177,7 +177,7 @@ 'omniauth_step_up_auth' => { 'openid_connect' => { 'admin_mode' => { 'state' => 'succeeded' }, - 'other_scope' => { 'state' => 'succeeded' } + 'namespace' => { 'state' => 'succeeded' } }, 'other_provider' => { 'admin_mode' => { 'state' => 'failed' } @@ -199,7 +199,7 @@ disable_step_up_authentication! expect(session['omniauth_step_up_auth']['openid_connect']).not_to have_key('admin_mode') - expect(session['omniauth_step_up_auth']['openid_connect']).to have_key('other_scope') + expect(session['omniauth_step_up_auth']['openid_connect']).to have_key('namespace') expect(session['omniauth_step_up_auth']['other_provider']).not_to have_key('admin_mode') expect(described_class.succeeded?(session)).to be_falsey @@ -218,7 +218,7 @@ disable_step_up_authentication! expect(session['omniauth_step_up_auth']['openid_connect']).to have_key('admin_mode') - expect(session['omniauth_step_up_auth']['openid_connect']).to have_key('other_scope') + expect(session['omniauth_step_up_auth']['openid_connect']).to have_key('namespace') expect(session['omniauth_step_up_auth']['other_provider']).to have_key('admin_mode') end end diff --git a/spec/requests/groups/step_up_auths_spec.rb b/spec/requests/groups/step_up_auths_spec.rb new file mode 100644 index 00000000000000..2928481d51e43a --- /dev/null +++ b/spec/requests/groups/step_up_auths_spec.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe "Groups::StepUpAuths", 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 + it "returns http success" do + get "/groups/#{group.full_path}/-/step_up_auth/new" + expect(response).to have_gitlab_http_status(:success) + end + end +end -- GitLab