Extract EE-specific lines to EE::Gitlab::Auth::Saml::User

Signed-off-by: Rémy Coutable <remy@rymai.me>

ee/lib/ee/gitlab/auth/saml/user.rb

+module EE
+  module Gitlab
+    module Auth
+      module Saml
+        module User
+          extend ::Gitlab::Utils::Override
+
+          override :find_user
+          def find_user
+            user = super
+
+            if user_in_required_group?
+              unblock_user(user, "in required group") if user.persisted? && user.blocked?
+            elsif user.persisted?
+              block_user(user, "not in required group") unless user.blocked?
+            else
+              user = nil
+            end
+
+            if user
+              # Check if there is overlap between the user's groups and the external groups
+              # setting then set user as external or internal.
+              user.admin = !(auth_hash.groups & saml_config.admin_groups).empty? if admin_groups_enabled?
+            end
+
+            user
+          end
+
+          protected
+
+          def block_user(user, reason)
+            user.ldap_block
+            log_user_changes(user, "#{reason}, blocking")
+          end
+
+          def unblock_user(user, reason)
+            user.activate
+            log_user_changes(user, "#{reason}, unblocking")
+          end
+
+          def log_user_changes(user, message)
+            ::Gitlab::AppLogger.info(
+              "SAML(#{auth_hash.provider}) account \"#{auth_hash.uid}\" #{message} " \
+              "Gitlab user \"#{user.name}\" (#{user.email})"
+            )
+          end
+
+          def user_in_required_group?
+            required_groups = saml_config.required_groups
+            required_groups.empty? || !(auth_hash.groups & required_groups).empty?
+          end
+
+          def admin_groups_enabled?
+            !saml_config.admin_groups.nil?
+          end
+        end
+      end
+    end
+  end
+end

ee/spec/lib/gitlab/auth/saml/user_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Auth::Saml::User do
+  include LdapHelpers
+  include LoginHelpers
+
+  let(:saml_user) { described_class.new(auth_hash) }
+  let(:gl_user) { saml_user.gl_user }
+  let(:uid) { 'my-uid' }
+  let(:dn) { 'uid=user1,ou=people,dc=example' }
+  let(:provider) { 'saml' }
+  let(:raw_info_attr) { { 'groups' => %w(Developers Freelancers Designers) } }
+  let(:auth_hash) { OmniAuth::AuthHash.new(uid: uid, provider: provider, info: info_hash, extra: { raw_info: OneLogin::RubySaml::Attributes.new(raw_info_attr) }) }
+  let(:info_hash) do
+    {
+      name: 'John',
+      email: 'john@mail.com'
+    }
+  end
+
+  describe '#save' do
+    def stub_omniauth_config(messages)
+      allow(Gitlab.config.omniauth).to receive_messages(messages)
+    end
+
+    def stub_basic_saml_config
+      allow(Gitlab::Auth::Saml::Config).to receive_messages({ options: { name: 'saml', args: {} } })
+    end
+
+    def stub_saml_required_group_config(groups)
+      allow(Gitlab::Auth::Saml::Config).to receive_messages({ options: { name: 'saml', groups_attribute: 'groups', required_groups: groups, args: {} } })
+    end
+
+    def stub_saml_admin_group_config(groups)
+      allow(Gitlab::Auth::Saml::Config).to receive_messages({ options: { name: 'saml', groups_attribute: 'groups', admin_groups: groups, args: {} } })
+    end
+
+    before do
+      stub_basic_saml_config
+    end
+
+    describe 'account exists on server' do
+      before do
+        stub_omniauth_config({ allow_single_sign_on: ['saml'], auto_link_saml_user: true })
+      end
+
+      context 'admin groups' do
+        context 'are defined' do
+          it 'marks the user as admin' do
+            stub_saml_admin_group_config(%w(Developers))
+            saml_user.save
+
+            expect(gl_user).to be_valid
+            expect(gl_user.admin).to be_truthy
+          end
+        end
+
+        before do
+          stub_saml_admin_group_config(%w(Admins))
+        end
+
+        context 'are defined but the user does not belong there' do
+          it 'does not mark the user as admin' do
+            saml_user.save
+
+            expect(gl_user).to be_valid
+            expect(gl_user.admin).to be_falsey
+          end
+        end
+
+        context 'user was admin, now should not be' do
+          it 'makes user non admin' do
+            create(:user, email: 'john@mail.com', username: 'john').update_attribute('admin', true)
+            saml_user.save
+
+            expect(gl_user).to be_valid
+            expect(gl_user.admin).to be_falsey
+          end
+        end
+      end
+    end
+
+    describe 'no account exists on server' do
+      context 'required groups' do
+        context 'not defined' do
+          it 'lets anyone in' do
+            saml_user.save
+
+            expect(gl_user).to be_valid
+          end
+        end
+
+        context 'are defined' do
+          before do
+            stub_omniauth_config(block_auto_created_users: false)
+          end
+
+          it 'lets members in' do
+            stub_saml_required_group_config(%w(Developers))
+            saml_user.save
+
+            expect(gl_user).to be_valid
+          end
+
+          it 'unblocks already blocked members' do
+            stub_saml_required_group_config(%w(Developers))
+            saml_user.save.ldap_block
+
+            expect(saml_user.find_user).to be_active
+          end
+
+          it 'does not allow non-members' do
+            stub_saml_required_group_config(%w(ArchitectureAstronauts))
+
+            expect { saml_user.save }.to raise_error Gitlab::Auth::OAuth::User::SignupDisabledError
+          end
+
+          it 'blocks non-members' do
+            orig_groups = auth_hash.extra.raw_info["groups"]
+            auth_hash.extra.raw_info.add("groups", "ArchitectureAstronauts")
+            stub_saml_required_group_config(%w(ArchitectureAstronauts))
+            saml_user.save
+            auth_hash.extra.raw_info.set("groups", orig_groups)
+
+            expect(saml_user.find_user).to be_ldap_blocked
+          end
+        end
+      end
+
+      context 'admin groups' do
+        context 'are defined' do
+          it 'marks the user as admin' do
+            stub_saml_admin_group_config(%w(Developers))
+            saml_user.save
+
+            expect(gl_user).to be_valid
+            expect(gl_user.admin).to be_truthy
+          end
+        end
+
+        context 'are defined but the user does not belong there' do
+          it 'does not mark the user as admin' do
+            stub_saml_admin_group_config(%w(Admins))
+            saml_user.save
+
+            expect(gl_user).to be_valid
+            expect(gl_user.admin).to be_falsey
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/auth/saml/user.rb

@@ -7,6 +7,8 @@ module Gitlab
   module Auth
     module Saml
       class User < Gitlab::Auth::OAuth::User
+        prepend ::EE::Gitlab::Auth::Saml::User
+
         extend ::Gitlab::Utils::Override
 
         def save
@@ -20,17 +22,8 @@ module Gitlab
           user ||= find_or_build_ldap_user if auto_link_ldap_user?
           user ||= build_new_user if signup_enabled?
 
-          if user_in_required_group?
-            unblock_user(user, "in required group") if user.persisted? && user.blocked?
-          elsif user.persisted?
-            block_user(user, "not in required group") unless user.blocked?
-          else
-            user = nil
-          end
-
           if user
             user.external = !(auth_hash.groups & saml_config.external_groups).empty? if external_users_enabled?
-            user.admin = !(auth_hash.groups & saml_config.admin_groups).empty? if admin_groups_enabled?
           end
 
           user
@@ -49,28 +42,6 @@ module Gitlab
           Gitlab::Auth::Saml::Config
         end
 
-        def block_user(user, reason)
-          user.ldap_block
-          log_user_changes(user, "#{reason}, blocking")
-        end
-
-        def unblock_user(user, reason)
-          user.activate
-          log_user_changes(user, "#{reason}, unblocking")
-        end
-
-        def log_user_changes(user, message)
-          Gitlab::AppLogger.info(
-            "SAML(#{auth_hash.provider}) account \"#{auth_hash.uid}\" #{message} " \
-            "Gitlab user \"#{user.name}\" (#{user.email})"
-          )
-        end
-
-        def user_in_required_group?
-          required_groups = saml_config.required_groups
-          required_groups.empty? || !(auth_hash.groups & required_groups).empty?
-        end
-
         def auto_link_saml_user?
           Gitlab.config.omniauth.auto_link_saml_user
         end
@@ -82,10 +53,6 @@ module Gitlab
         def auth_hash=(auth_hash)
           @auth_hash = Gitlab::Auth::Saml::AuthHash.new(auth_hash)
         end
-
-        def admin_groups_enabled?
-          !saml_config.admin_groups.nil?
-        end
       end
     end
   end

spec/lib/gitlab/auth/saml/user_spec.rb

@@ -20,26 +20,6 @@ describe Gitlab::Auth::Saml::User do
   let(:ldap_user) { Gitlab::Auth::LDAP::Person.new(Net::LDAP::Entry.new, 'ldapmain') }
 
   describe '#save' do
-    def stub_omniauth_config(messages)
-      allow(Gitlab.config.omniauth).to receive_messages(messages)
-    end
-
-    def stub_basic_saml_config
-      allow(Gitlab::Auth::Saml::Config).to receive_messages({ options: { name: 'saml', args: {} } })
-    end
-
-    def stub_saml_group_config(groups)
-      allow(Gitlab::Auth::Saml::Config).to receive_messages({ options: { name: 'saml', groups_attribute: 'groups', external_groups: groups, args: {} } })
-    end
-
-    def stub_saml_required_group_config(groups)
-      allow(Gitlab::Auth::Saml::Config).to receive_messages({ options: { name: 'saml', groups_attribute: 'groups', required_groups: groups, args: {} } })
-    end
-
-    def stub_saml_admin_group_config(groups)
-      allow(Gitlab::Auth::Saml::Config).to receive_messages({ options: { name: 'saml', groups_attribute: 'groups', admin_groups: groups, args: {} } })
-    end
-
     before do
       stub_basic_saml_config
     end
@@ -93,38 +73,6 @@ describe Gitlab::Auth::Saml::User do
           end
         end
       end
-
-      context 'admin groups' do
-        context 'are defined' do
-          it 'marks the user as admin' do
-            stub_saml_admin_group_config(%w(Developers))
-            saml_user.save
-            expect(gl_user).to be_valid
-            expect(gl_user.admin).to be_truthy
-          end
-        end
-
-        before do
-          stub_saml_admin_group_config(%w(Admins))
-        end
-
-        context 'are defined but the user does not belong there' do
-          it 'does not mark the user as admin' do
-            saml_user.save
-            expect(gl_user).to be_valid
-            expect(gl_user.admin).to be_falsey
-          end
-        end
-
-        context 'user was admin, now should not be' do
-          it 'makes user non admin' do
-            existing_user.update_attribute('admin', true)
-            saml_user.save
-            expect(gl_user).to be_valid
-            expect(gl_user.admin).to be_falsey
-          end
-        end
-      end
     end
 
     describe 'no account exists on server' do
@@ -185,67 +133,6 @@ describe Gitlab::Auth::Saml::User do
         end
       end
 
-      context 'required groups' do
-        context 'not defined' do
-          it 'lets anyone in' do
-            saml_user.save
-            expect(gl_user).to be_valid
-          end
-        end
-
-        context 'are defined' do
-          before do
-            stub_omniauth_config(block_auto_created_users: false)
-          end
-
-          it 'lets members in' do
-            stub_saml_required_group_config(%w(Developers))
-            saml_user.save
-            expect(gl_user).to be_valid
-          end
-
-          it 'unblocks already blocked members' do
-            stub_saml_required_group_config(%w(Developers))
-            saml_user.save.ldap_block
-            expect(saml_user.find_user).to be_active
-          end
-
-          it 'does not allow non-members' do
-            stub_saml_required_group_config(%w(ArchitectureAstronauts))
-            expect { saml_user.save }.to raise_error Gitlab::Auth::OAuth::User::SignupDisabledError
-          end
-
-          it 'blocks non-members' do
-            orig_groups = auth_hash.extra.raw_info["groups"]
-            auth_hash.extra.raw_info.add("groups", "ArchitectureAstronauts")
-            stub_saml_required_group_config(%w(ArchitectureAstronauts))
-            saml_user.save
-            auth_hash.extra.raw_info.set("groups", orig_groups)
-            expect(saml_user.find_user).to be_ldap_blocked
-          end
-        end
-      end
-
-      context 'admin groups' do
-        context 'are defined' do
-          it 'marks the user as admin' do
-            stub_saml_admin_group_config(%w(Developers))
-            saml_user.save
-            expect(gl_user).to be_valid
-            expect(gl_user.admin).to be_truthy
-          end
-        end
-
-        context 'are defined but the user does not belong there' do
-          it 'does not mark the user as admin' do
-            stub_saml_admin_group_config(%w(Admins))
-            saml_user.save
-            expect(gl_user).to be_valid
-            expect(gl_user.admin).to be_falsey
-          end
-        end
-      end
-
       context 'with auto_link_ldap_user disabled (default)' do
         before do
           stub_omniauth_config({ auto_link_ldap_user: false, auto_link_saml_user: false, allow_single_sign_on: ['saml'] })