Merge branch '12189-sso-enforcement-should-enforce-on-subgroups' into 'master'

Resolve "SSO enforcement should enforce on subgroups"

Closes #12189

See merge request gitlab-org/gitlab-ee!14364

ee/app/controllers/concerns/ee/routable_actions/sso_enforcement_redirect.rb

@@ -20,7 +20,7 @@ module EE
       end
 
       def sso_redirect_url
-        sso_group_saml_providers_url(root_group, token: root_group.saml_discovery_token)
+        sso_group_saml_providers_url(root_group, url_params)
       end
 
       module ControllerActions
@@ -59,6 +59,13 @@ module EE
       def root_group
         @root_group ||= group.root_ancestor
       end
+
+      def url_params
+        {
+          token: root_group.saml_discovery_token,
+          redirect: "/#{routable.full_path}"
+        }
+      end
     end
   end
 end

ee/app/controllers/groups/sso_controller.rb

 # frozen_string_literal: true
 
 class Groups::SsoController < Groups::ApplicationController
+  include InternalRedirect
   skip_before_action :group
 
   before_action :authenticate_user!, only: [:unlink]
@@ -13,7 +14,8 @@ class Groups::SsoController < Groups::ApplicationController
   layout 'devise'
 
   def saml
-    @group_path = params[:group_id]
+    @redirect_path = safe_redirect_path(params[:redirect]) || group_path(unauthenticated_group)
+    @group_path = unauthenticated_group.path
     @group_name = unauthenticated_group.full_name
     @group_saml_identity = linked_identity
     @idp_url = unauthenticated_group.saml_provider.sso_url

ee/app/views/groups/sso/saml.html.haml

@@ -13,7 +13,7 @@
     - if @group_saml_identity || !user_signed_in?
       %p= _("This will redirect you to an external sign in page.")
 
-      = saml_link _('Sign in with Single Sign-On'), @group_path, html_class: 'btn btn-success btn-block qa-saml-sso-signin-button'
+      = saml_link _('Sign in with Single Sign-On'), @group_path, html_class: 'btn btn-success btn-block qa-saml-sso-signin-button', redirect: @redirect_path
     - else
       .card.card-body.bs-callout-warning
         = _("Only proceed if you trust %{idp_url} to control your GitLab account sign in.") % { idp_url: @idp_url }

ee/changelogs/unreleased/12189-sso-enforcement-should-enforce-on-subgroups.yml

+---
+title: Enforce SSO on subgroups and projects
+merge_request: 14364
+author:
+type: fixed

ee/lib/gitlab/auth/group_saml/sso_enforcer.rb

@@ -23,10 +23,11 @@ module Gitlab
         end
 
         def self.group_access_restricted?(group)
-          return false unless ::Feature.enabled?(:enforced_sso_requires_session, group)
           return false unless group
+          return false unless group.root_ancestor
+          return false unless ::Feature.enabled?(:enforced_sso_requires_session, group.root_ancestor)
 
-          saml_provider = group&.root_ancestor&.saml_provider
+          saml_provider = group.root_ancestor.saml_provider
 
           return false unless saml_provider
 

ee/spec/controllers/concerns/ee/routable_actions/sso_enforcement_redirect_spec.rb

@@ -81,7 +81,9 @@ describe EE::RoutableActions::SsoEnforcementRedirect do
   describe '#sso_redirect_url' do
     shared_examples 'a routable SSO url' do
       it 'returns the SSO url for the root group' do
-        expect(subject.sso_redirect_url).to match(/groups\/#{root_group.to_param}\/-\/saml\/sso\?token=/)
+        redirect_url = CGI.escape("/#{subject.routable.full_path}")
+
+        expect(subject.sso_redirect_url).to match(/groups\/#{root_group.to_param}\/-\/saml\/sso\?redirect=#{redirect_url}&token=/)
       end
     end
 

ee/spec/controllers/concerns/routable_actions_spec.rb

@@ -42,7 +42,7 @@ describe RoutableActions do
           get :show, params: request_params(routable)
 
           expect(response).to have_gitlab_http_status(302)
-          expect(response.location).to match(/groups\/.*\/-\/saml\/sso\?token=/)
+          expect(response.location).to match(/groups\/.*\/-\/saml\/sso\?redirect=.+&token=/)
         end
 
         it 'does not redirect on POST requests' do

ee/spec/controllers/groups/groups_controller_spec.rb

@@ -87,7 +87,7 @@ describe GroupsController do
         get :show, params: { id: group }
 
         expect(response).to have_gitlab_http_status(302)
-        expect(response.location).to match(/groups\/#{group.to_param}\/-\/saml\/sso\?token=/)
+        expect(response.location).to match(/groups\/#{group.to_param}\/-\/saml\/sso\?redirect=.+&token=/)
       end
     end
 

ee/spec/controllers/groups/sso_controller_spec.rb

@@ -19,6 +19,13 @@ describe Groups::SsoController do
       expect(response).to have_gitlab_http_status(200)
     end
 
+    it 'malicious redirect parameter falls back to group_path' do
+      get :saml, params: { group_id: group, redirect: '///malicious-url' }
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(assigns[:redirect_path]).to eq(group_path(group))
+    end
+
     it 'passes group name to the view' do
       get :saml, params: { group_id: group }
 

ee/spec/features/groups/saml_enforcement_spec.rb

@@ -20,7 +20,7 @@ describe 'SAML access enforcement' do
       visit group_path(group)
 
       expect(page).to have_content("SAML SSO Sign in to \"#{group.name}\"")
-      expect(current_url).to match(/groups\/#{group.to_param}\/-\/saml\/sso\?token=/)
+      expect(current_url).to match(/groups\/#{group.to_param}\/-\/saml\/sso\?redirect=.+&token=/)
     end
   end
 

ee/spec/lib/gitlab/auth/group_saml/sso_enforcer_spec.rb

@@ -79,4 +79,45 @@ describe Gitlab::Auth::GroupSaml::SsoEnforcer do
       expect(subject).not_to be_access_restricted
     end
   end
+
+  describe '.group_access_restricted?' do
+    let(:root_group) { create(:group, saml_provider: create(:saml_provider, enabled: true, enforced_sso: true)) }
+
+    context 'is restricted' do
+      before do
+        stub_feature_flags(enforced_sso_requires_session: { enabled: true, thing: root_group })
+      end
+
+      it 'for a group' do
+        expect(described_class).to be_group_access_restricted(root_group)
+      end
+
+      it 'for a subgroup' do
+        sub_group = create(:group, parent: root_group)
+
+        expect(described_class).to be_group_access_restricted(sub_group)
+      end
+
+      it 'for a project' do
+        project = create(:project, group: root_group)
+
+        expect(described_class).to be_group_access_restricted(project)
+      end
+    end
+
+    context 'is not restricted' do
+      it 'for the group without configured saml_provider' do
+        group = create(:group)
+        stub_feature_flags(enforced_sso_requires_session: { enabled: true, thing: group })
+
+        expect(described_class).not_to be_group_access_restricted(group)
+      end
+
+      it 'for the group without the feature flag' do
+        stub_feature_flags(enforced_sso_requires_session: { enabled: false, thing: root_group })
+
+        expect(described_class).not_to be_group_access_restricted(root_group)
+      end
+    end
+  end
 end