From 63f9843726b02f932c5f7d2d9ef85546bf26eb1c Mon Sep 17 00:00:00 2001
From: James Edwards-Jones <jedwardsjones@gitlab.com>
Date: Mon, 11 Feb 2019 18:07:52 +0000
Subject: [PATCH] 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.
---
 ee/app/controllers/groups/sso_controller.rb   | 11 ++++++++
 ...j-restrict-group-saml-when-not-enabled.yml |  5 ++++
 ee/lib/gitlab/auth/group_saml/group_lookup.rb |  2 +-
 .../controllers/groups/sso_controller_spec.rb | 28 +++++++++++++++++++
 .../omni_auth/strategies/group_saml_spec.rb   |  8 ++++++
 5 files changed, 53 insertions(+), 1 deletion(-)
 create mode 100644 ee/changelogs/unreleased/security-jej-restrict-group-saml-when-not-enabled.yml

diff --git a/ee/app/controllers/groups/sso_controller.rb b/ee/app/controllers/groups/sso_controller.rb
index f5f840aab7b534..c8a95e8725b9b1 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 00000000000000..a58e83184ba1f5
--- /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 a24fa02cbac336..3bf1f5a712922a 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 79261eaec84e0d..c023d0de5f0b2a 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 ba80fc9dab06f7..d42526b18e1176 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
-- 
GitLab