diff --git a/app/controllers/concerns/enforces_step_up_authentication_for_namespace.rb b/app/controllers/concerns/enforces_step_up_authentication_for_namespace.rb index 63fdcb847fe2690b5c988c5681cd86b58fb0f88c..a438f5416057acb5cfdfda987c3e17ed8019f8da 100644 --- a/app/controllers/concerns/enforces_step_up_authentication_for_namespace.rb +++ b/app/controllers/concerns/enforces_step_up_authentication_for_namespace.rb @@ -4,10 +4,7 @@ # # 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. +# Include this module in group and project controllers to enforce the authentication check. # # @example # class Groups::ApplicationController < ApplicationController @@ -26,18 +23,43 @@ module EnforcesStepUpAuthenticationForNamespace private + # Looks up a namespace by ID and enforces step-up authentication + # Raises ActiveRecord::RecordNotFound if namespace_id is provided but namespace not found + def enforce_step_up_auth_for_namespace_id(namespace_id) + namespace = + Namespace.find_by_id(namespace_id) || + Namespace.find_by_full_path(namespace_id) + enforce_step_up_auth_for(namespace) + end + def enforce_step_up_auth_for(namespace) return if Feature.disabled?(:omniauth_step_up_auth_for_namespace, namespace) + + # Don't proceed if a render/redirect has already been performed + return if performed? + + # Security check: Always raise error for nil namespace BEFORE any other checks + # This prevents bypassing step-up auth enforcement by passing invalid namespace + raise ActiveRecord::RecordNotFound, 'Namespace not found' if namespace.blank? + + return unless step_up_auth_supported_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? + def step_up_auth_supported_for_namespace?(namespace) + # Step-up auth is only meaningful for authenticated users + # If there's no current_user, let the normal authentication flow handle it + return false unless current_user + namespace.present? && + namespace.is_a?(Group) && + namespace.try(:step_up_auth_required_oauth_provider).present? + end + + def step_up_auth_required_for_namespace?(namespace) ::Gitlab::Auth::Oidc::StepUpAuthentication.enabled_for_provider?( provider_name: namespace.step_up_auth_required_oauth_provider, scope: ::Gitlab::Auth::Oidc::StepUpAuthentication::SCOPE_NAMESPACE diff --git a/app/controllers/groups/application_controller.rb b/app/controllers/groups/application_controller.rb index b853106c1a185966f2f75b8e1111887f60794ab5..1cad4ebc99f6118f440996bf8e8670c87bc263fb 100644 --- a/app/controllers/groups/application_controller.rb +++ b/app/controllers/groups/application_controller.rb @@ -2,20 +2,16 @@ class Groups::ApplicationController < ApplicationController include RoutableActions + include EnforcesStepUpAuthenticationForNamespace include ControllerWithCrossProjectAccessCheck 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! before_action :group + before_action :enforce_step_up_auth_for_namespace before_action :set_sorting requires_cross_project_access @@ -108,6 +104,12 @@ def method_missing(method_sym, *arguments, &block) super end end + + def enforce_step_up_auth_for_namespace + # Use @group instance variable instead of calling group method + # to avoid triggering find_routable! when the :group before_action was skipped + enforce_step_up_auth_for(@group) + end end Groups::ApplicationController.prepend_mod_with('Groups::ApplicationController') diff --git a/app/controllers/groups/children_controller.rb b/app/controllers/groups/children_controller.rb index e0694b68c7d01bedbe3375d57a495ebca712843b..703a8f6ef9668e80400716d34276a7f914931373 100644 --- a/app/controllers/groups/children_controller.rb +++ b/app/controllers/groups/children_controller.rb @@ -5,7 +5,6 @@ class ChildrenController < Groups::ApplicationController include Gitlab::Utils::StrongMemoize extend ::Gitlab::Utils::Override - before_action :group before_action :validate_per_page skip_cross_project_access_check :index diff --git a/app/controllers/groups/shared_projects_controller.rb b/app/controllers/groups/shared_projects_controller.rb index 73f951d6ce45ee07ad1bfc0b551e815f55416cd9..bdb2ee20b8bb7d3a23a64cde5c358117489814ea 100644 --- a/app/controllers/groups/shared_projects_controller.rb +++ b/app/controllers/groups/shared_projects_controller.rb @@ -3,7 +3,6 @@ module Groups class SharedProjectsController < Groups::ApplicationController respond_to :json - before_action :group skip_cross_project_access_check :index feature_category :groups_and_projects diff --git a/app/controllers/groups/step_up_auths_controller.rb b/app/controllers/groups/step_up_auths_controller.rb index 34442c9ac37c4ee3dda9cd293188832057a961f2..af20831dcf29c0552b46ef19ae69df50b9017d53 100644 --- a/app/controllers/groups/step_up_auths_controller.rb +++ b/app/controllers/groups/step_up_auths_controller.rb @@ -5,6 +5,7 @@ class StepUpAuthsController < Groups::ApplicationController include InternalRedirect before_action :require_user! + skip_before_action :enforce_step_up_auth_for_namespace feature_category :system_access diff --git a/app/controllers/groups/uploads_controller.rb b/app/controllers/groups/uploads_controller.rb index b77d9ade8f8dd965d6017573eed407cb074b8782..400320b572f75862eeb7d795aa1c87631983ac79 100644 --- a/app/controllers/groups/uploads_controller.rb +++ b/app/controllers/groups/uploads_controller.rb @@ -6,6 +6,12 @@ class Groups::UploadsController < Groups::ApplicationController skip_before_action :group, if: -> { action_name == 'show' && embeddable? } + # TODO: Remove this skip and implement step-up auth enforcement for uploads + # See: https://gitlab.com/gitlab-org/gitlab/-/issues/578693 + # This skip_before_action is temporary to avoid breaking uploads while we add + # comprehensive test coverage and proper enforcement in a follow-up MR. + skip_before_action :enforce_step_up_auth_for_namespace + before_action :authorize_upload_file!, only: [:create, :authorize] before_action :verify_workhorse_api!, only: [:authorize] before_action :disallow_new_uploads!, only: :show diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 14eb0fa95dc730c0fecc1bd02e6e5c410c0c7003..1341f40ecf9cf8e6eedcab4ddf1e6b04bd32127d 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -26,6 +26,18 @@ class GroupsController < Groups::ApplicationController before_action :authorize_remove_group!, only: [:destroy, :restore] before_action :authorize_create_group!, only: [:new] + # Skip :index, :new, :create because the before_action :group has not been executed for these + # actions, so @group is not available. Enforcement requires a loaded group instance. + # TODO: For :new and :create actions, enforce step-up auth by checking the parent group + # (via params[:parent_id]) to check if parent group has step-up auth required. + # See: https://gitlab.com/gitlab-org/gitlab/-/issues/579212 + before_action :enforce_step_up_auth_for_namespace, except: [:index, :new, :create] + + # Skip for :edit and :update when user is a group admin/owner to enable self-service recovery + # from misconfiguration. Without this, owners could lock themselves out, requiring instance + # admin intervention for every affected group. Non-admin users still require step-up auth. + skip_before_action :enforce_step_up_auth_for_namespace, if: :skip_step_up_auth_for_owner_on_edit_and_update? + before_action :group_projects, only: [:activity, :merge_requests] before_action :event_filter, only: [:activity] @@ -414,6 +426,10 @@ def redirect_issues_to_work_items def work_items_redirect_params request.query_parameters end + + def skip_step_up_auth_for_owner_on_edit_and_update? + [:edit, :update].include?(action_name.to_sym) && can?(current_user, :admin_group, group) + end end GroupsController.prepend_mod diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb index f2f5331ab8e8cf0dc3e8bb1a16f440fb94c4edec..66c3b3db0c6e73811867b021fd35054cb1219bb6 100644 --- a/app/controllers/projects/application_controller.rb +++ b/app/controllers/projects/application_controller.rb @@ -5,16 +5,17 @@ class Projects::ApplicationController < ApplicationController include CookiesHelper 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 + include EnforcesStepUpAuthenticationForNamespace skip_before_action :authenticate_user! before_action :project before_action :repository + + # This before_action must execute AFTER the :project before_action. + # The enforce_step_up_auth_for_namespace method depends on @project being loaded first. + # By placing it after `before_action :project`, we guarantee the correct execution order. + before_action :enforce_step_up_auth_for_namespace + layout 'project' before_action do @@ -120,6 +121,16 @@ def handle_update_result(result) render 'edit' end end + + def enforce_step_up_auth_for_namespace + # Use @project instance variable instead of calling project method + # to avoid triggering find_routable! when the :project before_action was skipped + if @project&.namespace.present? + enforce_step_up_auth_for(@project.namespace) + elsif params[:namespace_id].present? + enforce_step_up_auth_for_namespace_id(params[:namespace_id]) + end + end end Projects::ApplicationController.prepend_mod_with('Projects::ApplicationController') diff --git a/app/controllers/projects/uploads_controller.rb b/app/controllers/projects/uploads_controller.rb index 90bed60971f566ca07cb84eb651274a0086d6ecb..4439f219f08eb21cfa710a46fac91733bcbaaa3f 100644 --- a/app/controllers/projects/uploads_controller.rb +++ b/app/controllers/projects/uploads_controller.rb @@ -8,6 +8,12 @@ class Projects::UploadsController < Projects::ApplicationController skip_before_action :project, :repository, if: -> { bypass_auth_checks_on_uploads? } + # TODO: Remove this skip and implement step-up auth enforcement for uploads + # See: https://gitlab.com/gitlab-org/gitlab/-/issues/578693 + # This skip_before_action is temporary to avoid breaking uploads while we add + # comprehensive test coverage and proper enforcement in a follow-up MR. + skip_before_action :enforce_step_up_auth_for_namespace + before_action :authorize_upload_file!, only: [:create, :authorize] before_action :verify_workhorse_api!, only: [:authorize] before_action :disallow_new_uploads!, only: :show diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 252d5239bb60d8fc829ab167db3b09fd142a53b3..45617662b3591cf24a1ff7fd82f765c8fa6288e3 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -32,6 +32,16 @@ class ProjectsController < Projects::ApplicationController before_action :authorize_archive_project!, only: [:archive, :unarchive] before_action :event_filter, only: [:show, :activity] + # Step-up authentication enforcement + # IMPORTANT: These before_actions are placed AFTER authorization checks to ensure that + # unauthorized users receive a 404/403 response immediately without being prompted for + # step-up authentication. This follows the same pattern as GroupsController. + # The enforce_step_up_auth_for_namespace method depends on @project being loaded first. + # For :new action, the parent's enforcement is sufficient as it checks params[:namespace_id]. + before_action :enforce_step_up_auth_for_namespace, except: [:index, :create] + skip_before_action :enforce_step_up_auth_for_namespace, only: [:create] + before_action :enforce_step_up_auth_for_namespace_on_create, only: [:create] + # Project Export Rate Limit before_action :check_export_rate_limit!, only: [:export, :download_export, :generate_new_export] @@ -646,6 +656,11 @@ def check_export_rate_limit! def render_edit render 'edit' end + + def enforce_step_up_auth_for_namespace_on_create + namespace_id = params.dig(:project, :namespace_id) + enforce_step_up_auth_for_namespace_id(namespace_id) + end end ProjectsController.prepend_mod_with('ProjectsController') diff --git a/ee/app/controllers/ee/groups_controller.rb b/ee/app/controllers/ee/groups_controller.rb index 407973815464586b47c10364d0b58c8f8c491698..94ac1bb914dbed06480a2fee09a5f1ce21d92a22 100644 --- a/ee/app/controllers/ee/groups_controller.rb +++ b/ee/app/controllers/ee/groups_controller.rb @@ -17,6 +17,13 @@ module GroupsController skip_before_action :authorize_admin_group!, only: [:edit] before_action :authorize_view_edit_page!, only: [:edit] + # We have to redefine the before_action :enforce_step_up_auth_for_namespace + # because otherwise it would run before :authorize_view_edit_page! + # due to the order of before_actions in the inheritance chain. + # In general, step-up auth should be enforced after authorization checks. + before_action :enforce_step_up_auth_for_namespace, except: [:index, :new, :create] + skip_before_action :enforce_step_up_auth_for_namespace, if: :skip_step_up_auth_for_owner_on_edit_and_update? + before_action do push_frontend_feature_flag(:saas_user_caps_auto_approve_pending_users_on_cap_increase, @group) push_frontend_feature_flag(:work_item_status_mvc2, @group&.root_ancestor) diff --git a/ee/app/controllers/groups/contribution_analytics_controller.rb b/ee/app/controllers/groups/contribution_analytics_controller.rb index 2ea2f284a66f2b4e2535533b9f8101622fbb31f6..1ba31aaf5e8af050d3bea70f3c6a76e179d75282 100644 --- a/ee/app/controllers/groups/contribution_analytics_controller.rb +++ b/ee/app/controllers/groups/contribution_analytics_controller.rb @@ -3,7 +3,6 @@ class Groups::ContributionAnalyticsController < Groups::ApplicationController include ProductAnalyticsTracking - before_action :group before_action :authorize_read_contribution_analytics! before_action :redirect_to_new_dashboard, only: :show, if: -> { Feature.enabled?(:contributions_analytics_dashboard, group) diff --git a/ee/app/controllers/groups/ldap_group_links_controller.rb b/ee/app/controllers/groups/ldap_group_links_controller.rb index f897c97ced922df9f39de07206ebfcd762c295c8..998b505cefd441d889aab2e10b5e6db65123a259 100644 --- a/ee/app/controllers/groups/ldap_group_links_controller.rb +++ b/ee/app/controllers/groups/ldap_group_links_controller.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true class Groups::LdapGroupLinksController < Groups::ApplicationController - before_action :group before_action :require_ldap_enabled before_action :authorize_admin_group! before_action :authorize_manage_ldap_group_links! diff --git a/ee/app/controllers/groups/ldaps_controller.rb b/ee/app/controllers/groups/ldaps_controller.rb index 9779d55eaa80d91b9049d90c53550c4c9f8c96e2..5f855d62b299935a2ba9220bbcbb8ab73b422e93 100644 --- a/ee/app/controllers/groups/ldaps_controller.rb +++ b/ee/app/controllers/groups/ldaps_controller.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true class Groups::LdapsController < Groups::ApplicationController - before_action :group before_action :authorize_admin_group! before_action :check_enabled_extras! diff --git a/ee/app/controllers/groups/sso_controller.rb b/ee/app/controllers/groups/sso_controller.rb index 476d13d34f489bd466c25a4b194c633abd09daf7..deceaab69cb6493901b5cac0760aa3dc91baf049 100644 --- a/ee/app/controllers/groups/sso_controller.rb +++ b/ee/app/controllers/groups/sso_controller.rb @@ -6,6 +6,12 @@ class Groups::SsoController < Groups::ApplicationController skip_before_action :group + # TODO: Assess how step-up auth should be enforced in the SSO context, then implement + # See: https://gitlab.com/gitlab-org/gitlab/-/issues/578868 + # This controller uses `unauthenticated_group` instead of `@group` to support SSO flows + # for unauthenticated users. Parent's enforcement expects @group, causing RecordNotFound. + skip_before_action :enforce_step_up_auth_for_namespace + before_action :init_preferred_language before_action :authenticate_user!, only: [:unlink] before_action :require_group_saml_instance! diff --git a/ee/spec/requests/groups_controller_spec.rb b/ee/spec/requests/groups_controller_spec.rb index 86aa9bd37a352236be7eefa95b7998c66ff64578..fd681cad51b2e2cd62593f5e734496695cc2a145 100644 --- a/ee/spec/requests/groups_controller_spec.rb +++ b/ee/spec/requests/groups_controller_spec.rb @@ -6,6 +6,34 @@ let(:user) { create(:user) } let(:group) { create(:group) } + describe 'GET #edit' do + context 'step-up authentication enforcement' do + let_it_be(:group, reload: true) { create(:group) } + + subject(:make_request) { get edit_group_path(group) } + + before do + sign_in(user) + end + + context 'when user is owner' do + let_it_be_with_reload(:user) { create(:user, owner_of: group) } + + let(:expected_success_status) { :ok } + + it_behaves_like 'does not enforce step-up authentication' + end + + context 'when user is maintainer' do + let_it_be_with_reload(:user) { create(:user, maintainer_of: group) } + + let(:expected_success_status) { :not_found } + + it_behaves_like 'does not enforce step-up authentication' + end + end + end + describe 'PUT update' do before do group.add_owner(user) 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 0000000000000000000000000000000000000000..7aad873fd877553adbd4aa167842b7c37b5ad033 --- /dev/null +++ b/spec/features/groups/step_up_auth_spec.rb @@ -0,0 +1,238 @@ +# 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 + + shared_examples 'user can access group page successfully' do + it 'grants access to the group page' do + # First visit should redirect to step-up auth + 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 provider exists' do + before do + stub_omniauth_setting(enabled: true, auto_link_user: true, providers: [provider_oidc_config_with_step_up_auth]) + end + + context 'when group requires step-up authentication' do + before do + group.namespace_settings.update!(step_up_auth_required_oauth_provider: provider_oidc) + end + + context 'when step-up auth conditions are fulfilled' do + before do + sign_in(user) + end + + it 'completes full step-up auth flow with comprehensive navigation' do + # Test Case 1: Initial redirect and successful step-up authentication + # First visit should redirect to step-up auth page + 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) + + # Test Case 2: Navigation to different group pages + # Verify step-up auth session persists across different group routes + 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(group_work_items_path(group, type: ['issue'])) + + # Test Case 3: Navigation in and out of group scope + # Verify step-up auth session persists when navigating away and returning + 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 + + 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 'user can access group page successfully' + end + end + + context 'for different initial sign-in methods' do + shared_examples 'successful group step-up auth process' do + before do + wait_for_requests + expect(page).to have_current_path(root_path, ignore_query: true) # rubocop:disable RSpec/ExpectInHook -- Just to ensure our setup is correct + + # 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)) # rubocop:disable RSpec/ExpectInHook -- Just to ensure our setup is correct + + # 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 + ) + end + + it_behaves_like 'user can access group page successfully' + 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 step-up auth conditions are not fulfilled' do + before do + sign_in(user) + end + + it 'redirects to step-up auth page when authentication fails' do + # Initial redirect when step-up auth is required + visit group_path(group) + expect(page).to have_current_path(new_group_step_up_auth_path(group)) + + # Failed step-up auth redirects back to step-up auth page + # Authentication fails due to insufficient acr level + 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)) + expect(page).to have_content('Step-up authentication required This group requires additional authentication.') + end + + it 'allows user to retry step-up auth after initial failure' do + visit group_path(group) + + # First attempt - authentication fails + 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 - authentication succeeds with correct acr level + gitlab_group_step_up_auth_sign_in_via(provider_oidc, user, provider_oidc_extern_uid, + additional_info: additional_info_success_step_up_auth) + + # Verify successful access to group and navigation to different pages + 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(group_work_items_path(group, type: ['issue'])) + end + end + end + + context 'when group does not require step-up authentication' do + before do + group.namespace_settings.update!(step_up_auth_required_oauth_provider: nil) + + sign_in(user) + end + + it_behaves_like 'user can access group page successfully' + end + end + + context 'when step-up auth provider does not exists' 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_behaves_like 'user can access group page successfully' + 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/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index 8b343e39569cc23f919d21d65573baf5467b0a50..7746c8fef72f605a89cf6272507e8974aefa3239 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -75,6 +75,22 @@ end end + describe 'GET #index' do + context 'step-up authentication enforcement' do + let_it_be(:group, reload: true) { create(:group) } + let_it_be(:user, reload: true) { create(:user, owner_of: group) } + let(:expected_success_status) { :found } + + subject(:make_request) { get groups_path } + + before do + sign_in(user) + end + + it_behaves_like 'does not enforce step-up authentication' + end + end + describe 'GET #show' do context 'when group path contains format extensions' do where(:extension) { %w[.html .json] } @@ -108,6 +124,76 @@ end end end + + context 'step-up authentication enforcement' do + let(:expected_success_status) { :ok } + + subject(:make_request) { get group_path(group) } + + context 'for private group' do + let_it_be(:group, reload: true) { create(:group, :private) } + let_it_be(:user, reload: true) { create(:user, owner_of: group) } + + context 'when user authenticated' do + before do + sign_in(user) + end + + it_behaves_like 'enforces step-up authentication' + end + end + + context 'for public group' do + let_it_be(:group, reload: true) { create(:group) } + let_it_be(:user, reload: true) { create(:user, owner_of: group) } + + context 'when user authenticated' do + before do + sign_in(user) + end + + it_behaves_like 'enforces step-up authentication' + end + + context 'when user unauthenticated' do + it_behaves_like 'does not enforce step-up authentication' + end + end + end + end + + describe 'GET #new' do + context 'step-up authentication enforcement' do + let_it_be(:group, reload: true) { create(:group) } + let_it_be(:user, reload: true) { create(:user, owner_of: group) } + let(:expected_success_status) { :ok } + + subject(:make_request) { get new_group_path } + + before do + sign_in(user) + end + + it_behaves_like 'does not enforce step-up authentication' + end + end + + describe 'POST #create' do + context 'step-up authentication enforcement' do + let_it_be(:group, reload: true) { create(:group) } + let_it_be(:user, reload: true) { create(:user, owner_of: group) } + let(:expected_success_status) { :found } + + subject(:make_request) do + post groups_path, params: { group: { name: 'New Group', path: 'new-group' } } + end + + before do + sign_in(user) + end + + it_behaves_like 'does not enforce step-up authentication' + end end describe 'GET #edit' do @@ -180,5 +266,164 @@ end end end + + context 'step-up authentication enforcement' do + let_it_be(:group, reload: true) { create(:group) } + + subject(:make_request) { get edit_group_path(group) } + + before do + sign_in(user) + end + + context 'when user is owner' do + let_it_be_with_reload(:user) { create(:user, owner_of: group) } + + let(:expected_success_status) { :ok } + + it_behaves_like 'does not enforce step-up authentication' + end + + context 'when user is maintainer' do + let_it_be_with_reload(:user) { create(:user, maintainer_of: group) } + + let(:expected_success_status) { :not_found } + + it_behaves_like 'does not enforce step-up authentication' + end + end + end + + describe 'GET #activity' do + context 'step-up authentication enforcement' do + let_it_be(:group, reload: true) { create(:group) } + let_it_be(:user, reload: true) { create(:user, owner_of: group) } + let(:expected_success_status) { :ok } + + subject(:make_request) { get activity_group_path(group) } + + before do + sign_in(user) + end + + it_behaves_like 'enforces step-up authentication' + end + end + + describe 'GET #issues' do + context 'step-up authentication enforcement' do + let_it_be(:group, reload: true) { create(:group) } + let_it_be(:user, reload: true) { create(:user, owner_of: group) } + + let(:expected_success_status) { :redirect } + + subject(:make_request) { get issues_group_path(group) } + + before do + sign_in(user) + end + + it_behaves_like 'enforces step-up authentication' + end + end + + describe 'GET #merge_requests' do + context 'step-up authentication enforcement' do + let_it_be(:group, reload: true) { create(:group) } + let_it_be(:user, reload: true) { create(:user, owner_of: group) } + let(:expected_success_status) { :ok } + + subject(:make_request) { get merge_requests_group_path(group) } + + before do + sign_in(user) + end + + it_behaves_like 'enforces step-up authentication' + end + end + + describe 'PATCH #update' do + context 'step-up authentication enforcement' do + let_it_be(:group, reload: true) { create(:group) } + + subject(:make_request) { patch group_path(group), params: { group: { name: 'Updated Name' } } } + + before do + sign_in(user) + end + + context 'when user is owner' do + let_it_be_with_reload(:user) { create(:user, owner_of: group) } + + let(:expected_success_status) { :found } + + it_behaves_like 'does not enforce step-up authentication' + end + + context 'when user is maintainer' do + let_it_be_with_reload(:user) { create(:user, maintainer_of: group) } + + let(:expected_success_status) { :not_found } + + it 'responds with 404 before step-up authentication is triggered because user is not authorized' do + make_request + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + end + + describe 'DELETE #destroy' do + context 'step-up authentication enforcement' do + let_it_be(:group, reload: true) { create(:group) } + let_it_be(:user, reload: true) { create(:user, owner_of: group) } + let(:expected_success_status) { :found } + + subject(:make_request) { delete group_path(group) } + + before do + sign_in(user) + end + + it_behaves_like 'enforces step-up authentication' + end + end + + describe 'PUT #transfer' do + context 'step-up authentication enforcement' do + let_it_be(:group, reload: true) { create(:group) } + let_it_be(:user, reload: true) { create(:user, owner_of: group) } + let_it_be(:parent_group, freeze: true) { create(:group) } + let(:expected_success_status) { :found } + + subject(:make_request) do + put transfer_group_path(group), params: { new_parent_group_id: parent_group.id } + end + + before do + sign_in(user) + parent_group.add_owner(user) + end + + it_behaves_like 'enforces step-up authentication' + end + end + + describe 'POST #export' do + context 'step-up authentication enforcement' do + let_it_be(:group, reload: true) { create(:group) } + let_it_be(:user, reload: true) { create(:user, owner_of: group) } + let(:expected_success_status) { :found } + + subject(:make_request) { post export_group_path(group) } + + before do + sign_in(user) + end + + it_behaves_like 'enforces step-up authentication' + end end end diff --git a/spec/requests/projects_controller_spec.rb b/spec/requests/projects_controller_spec.rb index a4cfad32e4a6bec9fd685d0801c80a6bac12ba8f..17a1b608fd9bf863fbbc36edec22bc16119b721c 100644 --- a/spec/requests/projects_controller_spec.rb +++ b/spec/requests/projects_controller_spec.rb @@ -24,4 +24,140 @@ end end end + + describe 'GET #show' do + context 'step-up authentication enforcement' do + let_it_be(:group, reload: true) { create(:group) } + let_it_be(:user, reload: true) { create(:user, developer_of: group) } + let(:expected_success_status) { :ok } + + subject(:make_request) { get project_path(project) } + + context 'for private project' do + let_it_be(:project, freeze: true) { create(:project, :private, namespace: group) } + + context 'when user authenticated' do + before do + sign_in(user) + end + + it_behaves_like 'enforces step-up authentication' + end + end + + context 'for public project' do + let_it_be(:project, freeze: true) { create(:project, :public, namespace: group) } + + context 'when user authenticated' do + before do + sign_in(user) + end + + it_behaves_like 'enforces step-up authentication' + end + + context 'when user unauthenticated' do + it_behaves_like 'does not enforce step-up authentication' + end + end + end + end + + describe 'GET #edit' do + context 'step-up authentication enforcement' do + let_it_be(:group, reload: true) { create(:group) } + let_it_be(:project, freeze: true) { create(:project, namespace: group) } + + subject(:make_request) { get edit_project_path(project) } + + before do + sign_in(user) + end + + context 'when user is maintainer' do + let_it_be(:user, reload: true) { create(:user, maintainer_of: project) } + let(:expected_success_status) { :ok } + + it_behaves_like 'enforces step-up authentication' + end + + context 'when user is developer' do + let_it_be(:user, reload: true) { create(:user, developer_of: project) } + let(:expected_success_status) { :not_found } + + it_behaves_like 'does not enforce step-up authentication' + end + + context 'when user is not a member' do + let_it_be(:user, reload: true) { create(:user) } + let(:expected_success_status) { :not_found } + + it_behaves_like 'does not enforce step-up authentication' + end + end + end + + context 'GET #new' do + let_it_be_with_reload(:group) { create(:group) } + let_it_be_with_reload(:user) { create(:user, :with_namespace, owner_of: group) } + let(:expected_success_status) { :ok } + + subject(:make_request) { get new_project_path(namespace_id: group.id) } + + before do + sign_in(user) + end + + it_behaves_like 'enforces step-up authentication' + + context 'with invalid namespace_id' do + subject(:make_request) { get new_project_path(namespace_id: non_existing_record_id) } + + it 'returns 404 when namespace does not exist' do + make_request + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + + context 'POST #create' do + let_it_be_with_reload(:group) { create(:group) } + let_it_be_with_reload(:user) { create(:user, :with_namespace, owner_of: group) } + let(:expected_success_status) { :found } + + let(:project_params) do + { + name: 'Test Project', + path: 'test-project', + namespace_id: group.id, + visibility_level: Gitlab::VisibilityLevel::PRIVATE + } + end + + subject(:make_request) { post projects_path, params: { project: project_params } } + + before do + sign_in(user) + end + + it_behaves_like 'enforces step-up authentication' + + context 'with invalid namespace_id' do + let(:project_params) do + { + name: 'Test Project', + path: 'test-project', + namespace_id: non_existing_record_id, + visibility_level: Gitlab::VisibilityLevel::PRIVATE + } + end + + it 'returns 404 when trying to create project with non-existent namespace' do + make_request + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end end diff --git a/spec/support/shared_examples/enforce_step_up_auth_shared_examples.rb b/spec/support/shared_examples/enforce_step_up_auth_shared_examples.rb new file mode 100644 index 0000000000000000000000000000000000000000..93571759e86892b4e92b05575ea0f49ffaa0d605 --- /dev/null +++ b/spec/support/shared_examples/enforce_step_up_auth_shared_examples.rb @@ -0,0 +1,148 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'enforces step-up authentication' do + let(:oidc_provider_config) do + GitlabSettings::Options.new( + name: 'openid_connect', + step_up_auth: { + namespace: { + id_token: { + required: { acr: 'gold' } + } + } + } + ) + end + + let(:oidc_provider_name) { oidc_provider_config.name } + + let(:session_step_up_succeeded) do + { 'omniauth_step_up_auth' => { + oidc_provider_name => { 'namespace' => { 'state' => 'succeeded' } } + } } + end + + let(:session_step_up_failed) do + { 'omniauth_step_up_auth' => { + oidc_provider_name => { 'namespace' => { 'state' => 'failed' } } + } } + end + + # Default: no session data (nested contexts can override) + let(:session_data) { nil } + + before do + stub_omniauth_setting(enabled: true, providers: [oidc_provider_config]) + allow(Devise).to receive(:omniauth_providers).and_return([oidc_provider_name]) + + # Handle session stubbing centrally + if session_data + test_session = ActionController::TestSession.new({ + 'warden.user.user.key' => [[user.id], user.authenticatable_salt] + }.merge(session_data)) + + allow_next_instance_of(ActionDispatch::Request) do |instance| + allow(instance).to receive(:session).and_return(test_session) + end + end + end + + context 'when group requires step-up auth' do + before do + group.namespace_settings.update!(step_up_auth_required_oauth_provider: oidc_provider_name) + end + + context 'when step-up auth has not been completed' do + it 'redirects to step-up auth page' do + subject + + expect(response).to redirect_to(new_group_step_up_auth_path(group)) + expect(response).to have_gitlab_http_status(:found) + end + + it 'sets flash notice about step-up auth requirement' do + subject + + expect(flash[:notice]).to include('Step-up authentication required') + end + end + + context 'when step-up auth session is failed' do + let(:session_data) { session_step_up_failed } + + it 'redirects to step-up auth page' do + subject + + expect(response).to redirect_to(new_group_step_up_auth_path(group)) + expect(response).to have_gitlab_http_status(:found) + end + end + + context 'when step-up auth session is succeeded' do + let(:session_data) { session_step_up_succeeded } + + it 'allows access to the action' do + subject + + expect(response).to have_gitlab_http_status(expected_success_status) + expect(response).not_to redirect_to(new_group_step_up_auth_path(group)) + end + end + + context 'when feature flag is disabled' do + before do + stub_feature_flags(omniauth_step_up_auth_for_namespace: false) + end + + it 'allows access without step-up auth' do + subject + + expect(response).to have_gitlab_http_status(expected_success_status) + expect(response).not_to redirect_to(new_group_step_up_auth_path(group)) + end + end + end + + context 'when group does not require step-up auth' do + before do + group.namespace_settings.update!(step_up_auth_required_oauth_provider: nil) + end + + it 'allows access without step-up auth' do + subject + + expect(response).to have_gitlab_http_status(expected_success_status) + expect(response).not_to redirect_to(new_group_step_up_auth_path(group)) + end + end +end + +RSpec.shared_examples 'does not enforce step-up authentication' do + let(:oidc_provider_config) do + GitlabSettings::Options.new( + name: 'openid_connect', + step_up_auth: { + namespace: { + id_token: { + required: { acr: 'gold' } + } + } + } + ) + end + + let(:oidc_provider_name) { oidc_provider_config.name } + + before do + stub_omniauth_setting(enabled: true, providers: [oidc_provider_config]) + allow(Devise).to receive(:omniauth_providers).and_return([oidc_provider_name]) + group.namespace_settings.update!(step_up_auth_required_oauth_provider: oidc_provider_name) + end + + it 'allows access without step-up auth' do + subject + + expect(response).to have_gitlab_http_status(expected_success_status) + expect(response).not_to redirect_to(new_group_step_up_auth_path(group)) + end +end