Merge branch '211962-allow-group-owners-to-bypass-sso-enforce' into 'master'

Allow group owners to login to sso-enforced groups without sso

See merge request gitlab-org/gitlab!50199

changelogs/unreleased/211962-allow-group-owners-to-bypass-sso-enforce.yml

+---
+title: Allow group owners and auditors to login to SSO-enforced groups without SSO
+merge_request: 50199
+author:
+type: changed

doc/user/group/saml_sso/index.md

@@ -81,6 +81,7 @@ Please note that the certificate [fingerprint algorithm](#additional-providers-a
 - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/5291) in GitLab 11.8.
 - [Improved](https://gitlab.com/gitlab-org/gitlab/-/issues/9255) in GitLab 11.11 with ongoing enforcement in the GitLab UI.
 - [Improved](https://gitlab.com/gitlab-org/gitlab/-/issues/292811) in GitLab 13.8, with an updated timeout experience.
+- [Improved](https://gitlab.com/gitlab-org/gitlab/-/issues/211962) in GitLab 13.8 with allowing group owners to not go through SSO.
 
 With this option enabled, users must go through your group's GitLab single sign-on URL. They may also be added via SCIM, if configured. Users can't be added manually, and may only access project/group resources via the UI by signing in through the SSO URL.
 

ee/app/policies/ee/group_policy.rb

@@ -347,8 +347,9 @@ module EE
     def sso_enforcement_prevents_access?
       return false unless subject.persisted?
       return false if user&.admin?
+      return false if user&.auditor?
 
-      ::Gitlab::Auth::GroupSaml::SsoEnforcer.group_access_restricted?(subject)
+      ::Gitlab::Auth::GroupSaml::SsoEnforcer.group_access_restricted?(subject, user: user)
     end
 
     # Available in Core for self-managed but only paid, non-trial for .com to prevent abuse

ee/app/policies/ee/project_policy.rb

@@ -324,7 +324,7 @@ module EE
           .default_project_deletion_protection
       end
 
-      rule { needs_new_sso_session & ~admin }.policy do
+      rule { needs_new_sso_session & ~admin & ~auditor }.policy do
         prevent :guest_access
         prevent :reporter_access
         prevent :developer_access

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

@@ -28,13 +28,14 @@ module Gitlab
           saml_enforced? && !active_session?
         end
 
-        def self.group_access_restricted?(group)
+        def self.group_access_restricted?(group, user: nil)
           return false unless group
           return false unless group.root_ancestor
 
           saml_provider = group.root_ancestor.saml_provider
 
           return false unless saml_provider
+          return false if user_authorized?(user, group)
 
           new(saml_provider).access_restricted?
         end
@@ -48,6 +49,10 @@ module Gitlab
         def group
           saml_provider&.group
         end
+
+        def self.user_authorized?(user, group)
+          return true if !group.has_parent? && group.owned_by?(user)
+        end
       end
     end
   end

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

@@ -112,11 +112,28 @@ RSpec.describe Gitlab::Auth::GroupSaml::SsoEnforcer do
 
         expect(described_class).to be_group_access_restricted(sub_group)
       end
+    end
 
-      it 'for a project' do
-        project = create(:project, group: root_group)
+    context 'for group owner' do
+      let(:user) { create(:user) }
 
-        expect(described_class).to be_group_access_restricted(project)
+      before do
+        create(:group_saml_identity, user: user, saml_provider: root_group.saml_provider)
+        root_group.add_owner(user)
+      end
+
+      context 'for a root group' do
+        it 'is not restricted' do
+          expect(described_class).not_to be_group_access_restricted(root_group, user: user)
+        end
+      end
+
+      context 'for a subgroup' do
+        it 'is restricted' do
+          sub_group = create(:group, parent: root_group)
+
+          expect(described_class).to be_group_access_restricted(sub_group, user: user)
+        end
       end
     end
 

ee/spec/policies/group_policy_spec.rb

@@ -464,11 +464,34 @@ RSpec.describe GroupPolicy do
 
           context 'as a group owner' do
             before do
+              create(:group_saml_identity, user: current_user, saml_provider: saml_provider)
               group.add_owner(current_user)
             end
 
-            it 'prevents access without a SAML session' do
-              is_expected.not_to allow_action(:read_group)
+            it 'allows access without a SAML session' do
+              is_expected.to allow_action(:read_group)
+            end
+
+            it 'prevents access without a SAML session for subgroup' do
+              subgroup = create(:group, :private, parent: group)
+
+              expect(described_class.new(current_user, subgroup)).not_to allow_action(:read_group)
+            end
+          end
+
+          context 'as an admin' do
+            let(:current_user) { admin }
+
+            it 'allows access without a SAML session' do
+              is_expected.to allow_action(:read_group)
+            end
+          end
+
+          context 'as an auditor' do
+            let(:current_user) { create(:user, :auditor) }
+
+            it 'allows access without a SAML session' do
+              is_expected.to allow_action(:read_group)
             end
           end
 

ee/spec/policies/project_policy_spec.rb

@@ -358,6 +358,24 @@ RSpec.describe ProjectPolicy do
           end
         end
 
+        context 'as a group maintainer' do
+          before do
+            group.add_maintainer(current_user)
+          end
+
+          it 'prevents access without a SAML session' do
+            is_expected.not_to allow_action(:read_project)
+          end
+        end
+
+        context 'as an auditor' do
+          let(:current_user) { create(:user, :auditor) }
+
+          it 'allows access without a SAML session' do
+            is_expected.to allow_action(:read_project)
+          end
+        end
+
         context 'with public access' do
           let(:group) { create(:group, :public) }
           let(:project) { create(:project, :public, group: saml_provider.group) }