Verified Commit 63f98437 authored by James Edwards-Jones's avatar James Edwards-Jones Committed by Yorick Peterse

Obey GitLab.com group SAML enabled? setting

Previously we weren't checking this when visiting the /sso page,
or when hitting a callback. This is both incorrect behaviour and
a security issue as it can be used to join a group.

We don't check this on metadata endpoints still, since they are
used before SAML is configured for the group.
parent b1d1a5b9
Pipeline #52067863 passed with stages
in 71 minutes and 57 seconds
......@@ -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 @@ class Groups::SsoController < Groups::ApplicationController
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'
......
---
title: Prevent SAML access when disabled by group admin on GitLab.com
merge_request:
author:
type: security
......@@ -21,7 +21,7 @@ module Gitlab
end
def group_saml_enabled?
saml_provider && group.feature_available?(:group_saml)
saml_provider&.enabled? && group.feature_available?(:group_saml)
end
def token_discoverable?
......
......@@ -25,6 +25,34 @@ describe Groups::SsoController do
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)
......
......@@ -57,6 +57,14 @@ describe OmniAuth::Strategies::GroupSaml, type: :strategy do
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
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment