diff --git a/app/controllers/admin/sessions_controller.rb b/app/controllers/admin/sessions_controller.rb index 0ec4bf29afb7e3104e318b6b87acff8aae34925f..1690fac45f3cc9168263cce0df538610410735da 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 3fabe9fe1c1bfa5812730ed84ee1c85fac8d1f2d..36701b09d69f81fcb61f51c494a1ab35f393c76c 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 c98af9776e37ce27375f40348b569c6967d04166..6cf99be309b2071893b19182c544fff783945320 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 280604f3dcf0fb0d7d9e842a1533645c99e4bc90..1b853c833dbe95f7d9c56854adfd0988783fa5bf 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 824cfb1b8a02c0cacc8b5b15ee739ad08271115e..da1002fe085a920f387f3275e9a2471c8709c077 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 a6e5e7134a2cd45221bb4f5b23752a48e2a692d6..2139dc2c0d22ae542efc24e0ddee14b2af0a262d 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 da36c5853f12425f6b72f52ef3ef6a7e972e31ea..51b7e3d7e2f111cb2aff0b93eda6a4417bd46a7d 100644 --- a/spec/features/user_settings/active_sessions_spec.rb +++ b/spec/features/user_settings/active_sessions_spec.rb @@ -83,13 +83,128 @@ 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: { 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 mark 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 9f38aef8db5679d00e2b7c99e48315cb76930169..116c992c7e15f050d316974578dd1752a60dd4ae 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,59 @@ 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 + + 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 + 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') + + expect(described_class.succeeded?(session)).to be_falsey + end + + context 'when session is empty' do + let(:session) { {} } + + it { is_expected.to eq({}) } + end + + context 'when scope does not exist' do + let(:scope) { :nonexistent_scope } + + 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 8552675284fae8cbf328c2a73074f70e8cdc00ef..961cc44904cf73d54cca35967e1e6acf57b54ae8 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 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) + + 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