From 54799c5850b1cd617b1c477dc7428694211219d0 Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Wed, 11 Dec 2024 18:14:42 +0100 Subject: [PATCH 1/3] feat: Step-up auth: Show step-up auth for admin mode in active sessions The active session page is important for users to see the active sessions, manage and revoke them. As part of the step-up authentication for admin mode, it is important to identify the session that currently have the step-up authentication enabled. This change ensures that users can easily identify which sessions have step-up authentication enabled, providing better transparency and security management. Changelog: added --- app/controllers/admin/sessions_controller.rb | 4 + app/models/active_session.rb | 7 +- .../active_sessions/_active_session.html.haml | 2 + .../auth/oidc/step_up_authentication.rb | 8 ++ locale/gitlab.pot | 3 + .../admin/sessions_controller_spec.rb | 50 ++++++++ .../user_settings/active_sessions_spec.rb | 116 ++++++++++++++++++ .../auth/oidc/step_up_authentication_spec.rb | 52 ++++++++ spec/models/active_session_spec.rb | 39 +++++- 9 files changed, 277 insertions(+), 4 deletions(-) diff --git a/app/controllers/admin/sessions_controller.rb b/app/controllers/admin/sessions_controller.rb index 0ec4bf29afb7e3..1690fac45f3cc9 100644 --- a/app/controllers/admin/sessions_controller.rb +++ b/app/controllers/admin/sessions_controller.rb @@ -35,6 +35,10 @@ def create def destroy current_user_mode.disable_admin_mode! + if Feature.enabled?(:omniauth_step_up_auth_for_admin_mode, current_user) + ::Gitlab::Auth::Oidc::StepUpAuthentication.disable_step_up_authentication!(session: session, scope: :admin_mode) + end + redirect_to root_path, status: :found, notice: _('Admin mode disabled') end diff --git a/app/models/active_session.rb b/app/models/active_session.rb index 3fabe9fe1c1bfa..36701b09d69f81 100644 --- a/app/models/active_session.rb +++ b/app/models/active_session.rb @@ -29,7 +29,7 @@ class ActiveSession :ip_address, :browser, :os, :device_name, :device_type, :is_impersonated, :session_id, :session_private_id, - :admin_mode + :admin_mode, :step_up_authenticated ].freeze ATTR_READER_LIST = [ :created_at, :updated_at @@ -88,7 +88,10 @@ def self.set(user, request) updated_at: timestamp, session_private_id: session_private_id, is_impersonated: request.session[:impersonator_id].present?, - admin_mode: Gitlab::Auth::CurrentUserMode.new(user, request.session).admin_mode? + admin_mode: Gitlab::Auth::CurrentUserMode.new(user, request.session).admin_mode?, + step_up_authenticated: + Feature.enabled?(:omniauth_step_up_auth_for_admin_mode, user) && + ::Gitlab::Auth::Oidc::StepUpAuthentication.succeeded?(request.session) ) Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do diff --git a/app/views/user_settings/active_sessions/_active_session.html.haml b/app/views/user_settings/active_sessions/_active_session.html.haml index c98af9776e37ce..6cf99be309b207 100644 --- a/app/views/user_settings/active_sessions/_active_session.html.haml +++ b/app/views/user_settings/active_sessions/_active_session.html.haml @@ -26,6 +26,8 @@ = l(active_session.created_at, format: :short) - if active_session.try(:admin_mode) %strong= _('with Admin Mode') + - if active_session.try(:step_up_authenticated) + %strong= s_('ActiveSessions|with Step-up Authentication') - unless is_current_session .gl-float-right diff --git a/lib/gitlab/auth/oidc/step_up_authentication.rb b/lib/gitlab/auth/oidc/step_up_authentication.rb index 280604f3dcf0fb..1b853c833dbe95 100644 --- a/lib/gitlab/auth/oidc/step_up_authentication.rb +++ b/lib/gitlab/auth/oidc/step_up_authentication.rb @@ -97,6 +97,14 @@ def omniauth_step_up_auth_session_data(session) Gitlab::NamespacedSessionStore.new(SESSION_STORE_KEY, session) end + def disable_step_up_authentication!(session:, scope: STEP_UP_AUTH_SCOPE_ADMIN_MODE) + omniauth_step_up_auth_session_data(session) + &.to_h + &.each_value do |step_up_auth_object| + step_up_auth_object.delete(scope.to_s) + end + end + private def oauth_providers diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 824cfb1b8a02c0..da1002fe085a92 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -3501,6 +3501,9 @@ msgstr "" msgid "Active project access tokens" msgstr "" +msgid "ActiveSessions|with Step-up Authentication" +msgstr "" + msgid "Activity" msgstr "" diff --git a/spec/controllers/admin/sessions_controller_spec.rb b/spec/controllers/admin/sessions_controller_spec.rb index a6e5e7134a2cd4..2139dc2c0d22ae 100644 --- a/spec/controllers/admin/sessions_controller_spec.rb +++ b/spec/controllers/admin/sessions_controller_spec.rb @@ -293,6 +293,56 @@ def authenticate_2fa(user_params) expect(response).to redirect_to(root_path) expect(controller.current_user_mode.admin_mode?).to be(false) end + + context 'for step-up authenticated admin users' do + let(:session_with_step_up_auth_data) do + { + 'omniauth_step_up_auth' => { + 'openid_connect' => { + 'admin_mode' => { 'state' => 'succeeded' }, + 'other_scope' => { 'state' => 'succeeded' } + }, + 'other_provider' => { + 'admin_mode' => { 'state' => 'failed' } + } + } + } + end + + subject(:response) { delete :destroy, session: session_with_step_up_auth_data } + + before do + get :new + + post :create, params: { user: { password: user.password } } + end + + it 'calls disable_step_up_authentication! for OIDC step-up' do + expect(::Gitlab::Auth::Oidc::StepUpAuthentication) + .to receive(:disable_step_up_authentication!).and_call_original + + response + + expect(request.session.dig('omniauth_step_up_auth', 'openid_connect')).not_to have_key('admin_mode') + end + + it { is_expected.to redirect_to(root_path) } + + 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 + + it 'does not call disable_step_up_authentication!' do + expect(::Gitlab::Auth::Oidc::StepUpAuthentication) + .not_to receive(:disable_step_up_authentication!).and_call_original + + response + + expect(request.session.dig('omniauth_step_up_auth', 'openid_connect')).to have_key('admin_mode') + end + end + end end end end diff --git a/spec/features/user_settings/active_sessions_spec.rb b/spec/features/user_settings/active_sessions_spec.rb index da36c5853f1242..5ad97a3d8bdbc1 100644 --- a/spec/features/user_settings/active_sessions_spec.rb +++ b/spec/features/user_settings/active_sessions_spec.rb @@ -83,13 +83,129 @@ gitlab_sign_in(admin) visit user_settings_active_sessions_path expect(page).to have_content('with Admin Mode') + expect(page).not_to have_content('with Step-up Authentication') + end + end + + context 'when session step-up authenticated', :with_current_organization do + let(:admin) { create(:omniauth_user, :admin, password_automatically_set: false, extern_uid: extern_uid, provider: provider_oidc) } + let(:extern_uid) { 'my-uid' } + let(:provider_oidc) { 'openid_connect' } + + let(:provider_oidc_config_with_step_up_auth) do + GitlabSettings::Options.new( + name: provider_oidc, + step_up_auth: { + admin_mode: { + id_token: { + required: { acr: 'gold' } + } + } + } + ) + end + + let(:additional_info) { { extra: { raw_info: additional_info_extra_raw_info } } } + let(:additional_info_extra_raw_info) { { acr: 'gold' } } + + around do |example| + with_omniauth_full_host { example.run } + end + + before do + user + + stub_omniauth_setting(enabled: true, auto_link_user: true, providers: [provider_oidc_config_with_step_up_auth]) + end + + it 'marks admin session as step-up authenticated' do + using_session :admin_session do + gitlab_sign_in(admin) + + gitlab_enable_admin_mode_sign_in_via(provider_oidc, admin, extern_uid, additional_info: additional_info) + + visit user_settings_active_sessions_path + + within('.settings-section') do + expect(page).to have_content('with Admin Mode') + expect(page).to have_content('with Step-up Authentication') + end + end + end + + it 'does not marks admin session as step-up authenticated when acr is not matching' do + using_session :admin_session do + gitlab_sign_in(admin) + + gitlab_enable_admin_mode_sign_in_via(provider_oidc, admin, extern_uid, additional_info: { extra: { raw_info: { acr: 'bronze' } } }) + + visit user_settings_active_sessions_path + + within('.settings-section') do + expect(page).not_to have_content('with Admin Mode') + expect(page).not_to have_content('with Step-up Authentication') + end + end + end + + it 'does not marks admin session as step-up authenticated after leaving admin mode', :js do + using_session :admin_session do + gitlab_sign_in(admin) + + wait_for_requests + + gitlab_enable_admin_mode_sign_in_via(provider_oidc, admin, extern_uid, additional_info: additional_info) + + wait_for_requests + + visit user_settings_active_sessions_path + + wait_for_requests + + within('.settings-section') do + expect(page).to have_content('with Admin Mode') + expect(page).to have_content('with Step-up Authentication') + end + + gitlab_disable_admin_mode + + visit user_settings_active_sessions_path + + within('.settings-section') do + expect(page).not_to have_content('with Admin Mode') + expect(page).not_to have_content('with Step-up Authentication') + end + end + 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) + end + + it 'does not marks admin session as step-up authenticated' do + using_session :admin_session do + gitlab_sign_in(admin) + + gitlab_enable_admin_mode_sign_in_via(provider_oidc, admin, extern_uid, additional_info: additional_info) + + visit user_settings_active_sessions_path + + within('.settings-section') do + expect(page).to have_content('with Admin Mode') + expect(page).not_to have_content('with Step-up Authentication') + end + end + end end end it 'does not display admin mode text in case its not' do using_session :admin_session do gitlab_sign_in(admin) + visit user_settings_active_sessions_path + expect(page).not_to have_content('with Admin Mode') end end 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 9f38aef8db5679..abcbadb2f92cb3 100644 --- a/spec/lib/gitlab/auth/oidc/step_up_authentication_spec.rb +++ b/spec/lib/gitlab/auth/oidc/step_up_authentication_spec.rb @@ -168,4 +168,56 @@ it { is_expected.to eq expected_result } end end + + describe '.disable_step_up_authentication!' do + let(:required_id_token_claims) { { claim_1: 'gold' } } + let(:scope) { :admin_mode } + let(:session) do + { + 'omniauth_step_up_auth' => { + 'openid_connect' => { + 'admin_mode' => { 'state' => 'succeeded' }, + 'other_scope' => { 'state' => 'succeeded' } + }, + 'other_provider' => { + 'admin_mode' => { 'state' => 'failed' } + } + } + } + end + + before do + stub_omniauth_setting(enabled: true, providers: [ommiauth_provider_config]) + end + + it 'removes the step-up auth data for the given scope from all providers' do + described_class.disable_step_up_authentication!(session: session, scope: scope) + + 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']['other_provider']).not_to have_key('admin_mode') + end + + it 'removes succeeded state for the given scope from all providers' do + described_class.disable_step_up_authentication!(session: session, scope: scope) + + expect(described_class.succeeded?(session)).to be_falsey + end + + it 'does nothing if session does not contain step-up auth data' do + empty_session = {} + + described_class.disable_step_up_authentication!(session: empty_session, scope: scope) + + expect(empty_session).to eq({}) + end + + it 'does nothing if the scope does not exist for any provider' do + described_class.disable_step_up_authentication!(session: session, scope: :nonexistent_scope) + + 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']['other_provider']).to have_key('admin_mode') + end + end end diff --git a/spec/models/active_session_spec.rb b/spec/models/active_session_spec.rb index 8552675284fae8..734aacfd17bd62 100644 --- a/spec/models/active_session_spec.rb +++ b/spec/models/active_session_spec.rb @@ -11,7 +11,7 @@ end let(:rack_session) { Rack::Session::SessionId.new('6919a6f1bb119dd7396fadc38fd18d0d') } - let(:session) { instance_double(ActionDispatch::Request::Session, id: rack_session, '[]': {}) } + let(:session) { instance_double(ActionDispatch::Request::Session, id: rack_session, '[]': {}, dig: {}) } let(:request) do double(:request, { @@ -234,6 +234,8 @@ def make_session(id) end describe '.set' do + subject(:set_request) { described_class.set(user, request) } + it 'sets a new redis entry for the user session and a lookup entry' do described_class.set(user, request) @@ -262,7 +264,8 @@ def make_session(id) device_name: 'iPhone 6', device_type: 'smartphone', created_at: eq(time), - updated_at: eq(time) + updated_at: eq(time), + step_up_authenticated: false ) end end @@ -286,6 +289,38 @@ def make_session(id) ) end end + + context 'when session is step-up authenticated' do + it 'does not set the step-up authenticated' do + expect(::Gitlab::Auth::Oidc::StepUpAuthentication) + .to receive(:succeeded?).with(session).and_return(true) + + set_request + + new_session = described_class.list(user).first + + expect(new_session).to have_attributes(step_up_authenticated: true) + end + + context 'for session key step_up_authenticated' do + 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 + + it 'does not set the step-up authenticated' do + allow(::Gitlab::Auth::Oidc::StepUpAuthentication) + .to receive(:succeeded?).with(session).and_return(true) + + set_request + + new_session = described_class.list(user).first + + expect(new_session).to have_attributes(step_up_authenticated: false) + end + end + end + end end describe '.destroy_session' do -- GitLab From 8847500ee8e7dd68ec88dbb382a3845efd1b5c77 Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Tue, 27 May 2025 15:59:23 +0200 Subject: [PATCH 2/3] refactor: Apply suggestion from @Saahmed - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/175441#note_2525127947 - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/175441#note_2525128004 - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/175441#note_2525127915 --- .../user_settings/active_sessions_spec.rb | 3 +- .../auth/oidc/step_up_authentication_spec.rb | 33 ++++++++++--------- spec/models/active_session_spec.rb | 4 +-- 3 files changed, 21 insertions(+), 19 deletions(-) diff --git a/spec/features/user_settings/active_sessions_spec.rb b/spec/features/user_settings/active_sessions_spec.rb index 5ad97a3d8bdbc1..6d9239066e47e1 100644 --- a/spec/features/user_settings/active_sessions_spec.rb +++ b/spec/features/user_settings/active_sessions_spec.rb @@ -105,8 +105,7 @@ ) end - let(:additional_info) { { extra: { raw_info: additional_info_extra_raw_info } } } - let(:additional_info_extra_raw_info) { { acr: 'gold' } } + let(:additional_info) { { extra: { raw_info: { acr: 'gold' } } } } around do |example| with_omniauth_full_host { example.run } 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 abcbadb2f92cb3..116c992c7e15f0 100644 --- a/spec/lib/gitlab/auth/oidc/step_up_authentication_spec.rb +++ b/spec/lib/gitlab/auth/oidc/step_up_authentication_spec.rb @@ -186,38 +186,41 @@ } end + subject(:disable_step_up_authentication!) do + described_class.disable_step_up_authentication!(session: session, scope: scope) + session + end + before do stub_omniauth_setting(enabled: true, providers: [ommiauth_provider_config]) end it 'removes the step-up auth data for the given scope from all providers' do - described_class.disable_step_up_authentication!(session: session, scope: scope) + 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']['other_provider']).not_to have_key('admin_mode') - end - - it 'removes succeeded state for the given scope from all providers' do - described_class.disable_step_up_authentication!(session: session, scope: scope) expect(described_class.succeeded?(session)).to be_falsey end - it 'does nothing if session does not contain step-up auth data' do - empty_session = {} - - described_class.disable_step_up_authentication!(session: empty_session, scope: scope) + context 'when session is empty' do + let(:session) { {} } - expect(empty_session).to eq({}) + it { is_expected.to eq({}) } end - it 'does nothing if the scope does not exist for any provider' do - described_class.disable_step_up_authentication!(session: session, scope: :nonexistent_scope) + context 'when scope does not exist' do + let(:scope) { :nonexistent_scope } - 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']['other_provider']).to have_key('admin_mode') + it 'does not change the session' do + 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']['other_provider']).to have_key('admin_mode') + end end end end diff --git a/spec/models/active_session_spec.rb b/spec/models/active_session_spec.rb index 734aacfd17bd62..961cc44904cf73 100644 --- a/spec/models/active_session_spec.rb +++ b/spec/models/active_session_spec.rb @@ -290,8 +290,8 @@ def make_session(id) end end - context 'when session is step-up authenticated' do - it 'does not set the step-up authenticated' do + context 'when the request session is step-up authenticated' do + it 'sets step_up_authenticated in the active_user_session' do expect(::Gitlab::Auth::Oidc::StepUpAuthentication) .to receive(:succeeded?).with(session).and_return(true) -- GitLab From 5a5363584c5380727af1ebfb8851433762e35819 Mon Sep 17 00:00:00 2001 From: Roger Meier Date: Wed, 25 Jun 2025 14:46:41 +0200 Subject: [PATCH 3/3] Apply 1 suggestion(s) to 1 file(s) Co-authored-by: Smriti Garg --- spec/features/user_settings/active_sessions_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/user_settings/active_sessions_spec.rb b/spec/features/user_settings/active_sessions_spec.rb index 6d9239066e47e1..51b7e3d7e2f111 100644 --- a/spec/features/user_settings/active_sessions_spec.rb +++ b/spec/features/user_settings/active_sessions_spec.rb @@ -182,7 +182,7 @@ stub_feature_flags(omniauth_step_up_auth_for_admin_mode: false) end - it 'does not marks admin session as step-up authenticated' do + it 'does not mark admin session as step-up authenticated' do using_session :admin_session do gitlab_sign_in(admin) -- GitLab