diff --git a/ee/app/controllers/groups/sso_controller.rb b/ee/app/controllers/groups/sso_controller.rb index f5f840aab7b53456001d075ceffcd8b3981e028d..c8a95e8725b9b16e888354cb96a1fab1bb9778c5 100644 --- a/ee/app/controllers/groups/sso_controller.rb +++ b/ee/app/controllers/groups/sso_controller.rb @@ -6,6 +6,7 @@ class Groups::SsoController < Groups::ApplicationController before_action :check_group_saml_configured before_action :check_group_saml_available! before_action :require_configured_provider + before_action :require_enabled_provider, except: [:unlink] before_action :authenticate_user!, only: [:unlink] before_action :check_user_can_sign_in_with_provider, only: [:saml] before_action :redirect_if_group_moved @@ -50,6 +51,16 @@ def unauthenticated_group def require_configured_provider return if @unauthenticated_group.saml_provider + redirect_settings_or_not_found + end + + def require_enabled_provider + return if @unauthenticated_group.saml_provider&.enabled? + + redirect_settings_or_not_found + end + + def redirect_settings_or_not_found if can?(current_user, :admin_group_saml, @unauthenticated_group) flash[:notice] = 'SAML sign on has not been configured for this group' diff --git a/ee/changelogs/unreleased/security-jej-restrict-group-saml-when-not-enabled.yml b/ee/changelogs/unreleased/security-jej-restrict-group-saml-when-not-enabled.yml new file mode 100644 index 0000000000000000000000000000000000000000..a58e83184ba1f5120b6170e0af58001d46ee16f3 --- /dev/null +++ b/ee/changelogs/unreleased/security-jej-restrict-group-saml-when-not-enabled.yml @@ -0,0 +1,5 @@ +--- +title: Prevent SAML access when disabled by group admin on GitLab.com +merge_request: +author: +type: security diff --git a/ee/lib/gitlab/auth/group_saml/group_lookup.rb b/ee/lib/gitlab/auth/group_saml/group_lookup.rb index a24fa02cbac336fd26dae4e3bb059df001ca3845..3bf1f5a712922af49a7107d779f0546aeb73a550 100644 --- a/ee/lib/gitlab/auth/group_saml/group_lookup.rb +++ b/ee/lib/gitlab/auth/group_saml/group_lookup.rb @@ -21,7 +21,7 @@ def saml_provider end def group_saml_enabled? - saml_provider && group.feature_available?(:group_saml) + saml_provider&.enabled? && group.feature_available?(:group_saml) end def token_discoverable? diff --git a/ee/spec/controllers/groups/sso_controller_spec.rb b/ee/spec/controllers/groups/sso_controller_spec.rb index 79261eaec84e0ddf6cd87ee65e37064c30e34c90..c023d0de5f0b2a25052539f6a9074637c79c2103 100644 --- a/ee/spec/controllers/groups/sso_controller_spec.rb +++ b/ee/spec/controllers/groups/sso_controller_spec.rb @@ -25,6 +25,34 @@ expect(assigns[:group_name]).to eq 'our-group' end + it 'allows account unlinking' do + create(:group_saml_identity, saml_provider: saml_provider, user: user) + + expect do + delete :unlink, params: { group_id: group } + end.to change(Identity, :count).by(-1) + end + + context 'when SAML is disabled for the group' do + before do + saml_provider.update!(enabled: false) + end + + it 'renders 404' do + get :saml, params: { group_id: group } + + expect(response).to have_gitlab_http_status(404) + end + + it 'still allows account unlinking' do + create(:group_saml_identity, saml_provider: saml_provider, user: user) + + expect do + delete :unlink, params: { group_id: group } + end.to change(Identity, :count).by(-1) + end + end + context 'when user is not signed in' do it 'acts as route not found' do sign_out(user) diff --git a/ee/spec/lib/omni_auth/strategies/group_saml_spec.rb b/ee/spec/lib/omni_auth/strategies/group_saml_spec.rb index ba80fc9dab06f7fb619ee62f4ffd6b92ec05e8a2..d42526b18e1176f531e2613c52647e2bfb8b93f9 100644 --- a/ee/spec/lib/omni_auth/strategies/group_saml_spec.rb +++ b/ee/spec/lib/omni_auth/strategies/group_saml_spec.rb @@ -57,6 +57,14 @@ def check(path) options = last_request.env['omniauth.strategy'].options expect(options['idp_cert_fingerprint']).to eq fingerprint end + + it 'returns 404 when SAML is disabled for the group' do + saml_provider.update!(enabled: false) + + expect do + post "/groups/my-group/-/saml/callback", SAMLResponse: saml_response + end.to raise_error(ActionController::RoutingError) + end end context 'with invalid SAMLResponse' do