Encapsulate EE-specific LDAP User methods

ee/lib/ee/gitlab/ldap/user.rb

+# LDAP User EE mixin
+#
+# This module is intended to encapsulate EE-specific User methods
+# and be **prepended** in the `Gitlab::LDAP::User` class.
+module EE
+  module Gitlab
+    module LDAP
+      module User
+        def initialize(auth_hash)
+          super
+
+          with_proxy(auth_hash.provider) do |proxy|
+            set_external_with_external_groups(proxy)
+          end
+        end
+
+        # Intended to be called during #initialize, and #save should be called
+        # after initialize.
+        def set_external_with_external_groups(proxy)
+          gl_user.external = in_any_external_group?(proxy)
+        end
+
+        # Returns true if the User is found in an external group listed in the
+        # config.
+        #
+        # Only checks the LDAP provider where the User was authorized.
+        def in_any_external_group?(proxy)
+          external_groups = proxy.adapter.config.external_groups
+          external_groups.any? do |group_cn|
+            in_group?(proxy, group_cn)
+          end
+        end
+
+        # Returns true if the User is a member of the group.
+        def in_group?(proxy, group_cn)
+          member_dns = proxy.dns_for_group_cn(group_cn)
+          member_dns.include?(auth_hash.uid)
+        end
+
+        def with_proxy(provider, &block)
+          ::EE::Gitlab::LDAP::Sync::Proxy.open(provider, &block)
+        end
+      end
+    end
+  end
+end

lib/gitlab/ldap/user.rb

@@ -7,6 +7,8 @@
 module Gitlab
   module LDAP
     class User < Gitlab::OAuth::User
+      prepend ::EE::Gitlab::LDAP::User
+
       class << self
         def find_by_uid_and_provider(uid, provider)
           # LDAP distinguished name is case-insensitive
@@ -20,7 +22,6 @@ module Gitlab
       def initialize(auth_hash)
         super
         update_user_attributes
-        set_external_with_external_groups
       end
 
       def save
@@ -76,24 +77,6 @@ module Gitlab
       def auth_hash=(auth_hash)
         @auth_hash = Gitlab::LDAP::AuthHash.new(auth_hash)
       end
-
-      def set_external_with_external_groups
-        gl_user.external = in_any_external_group?
-      end
-
-      def in_any_external_group?
-        ::EE::Gitlab::LDAP::Sync::Proxy.open(auth_hash.provider) do |proxy|
-          external_groups = proxy.adapter.config.external_groups
-          external_groups.any? do |group_cn|
-            in_group?(proxy, group_cn)
-          end
-        end
-      end
-
-      def in_group?(proxy, group_cn)
-        member_dns = proxy.dns_for_group_cn(group_cn)
-        member_dns.include?(auth_hash.uid)
-      end
     end
   end
 end

spec/ee/spec/lib/ee/gitlab/ldap/user_spec.rb

+require 'spec_helper'
+
+describe Gitlab::LDAP::User do
+  include LdapHelpers
+
+  let(:ldap_user) { described_class.new(auth_hash) }
+  let(:gl_user) { ldap_user.gl_user }
+  let(:info) do
+    {
+      name: 'John',
+      email: 'john@example.com',
+      nickname: 'john'
+    }
+  end
+  let(:auth_hash) do
+    OmniAuth::AuthHash.new(uid: 'uid=john,ou=people,dc=example,dc=com', provider: 'ldapmain', info: info)
+  end
+  let(:adapter) { ldap_adapter }
+  let(:group_cn) { 'foo' }
+  let(:group_member_dns) { [auth_hash.uid] }
+  let(:external_groups) { [] }
+  let(:fake_proxy) { double(:proxy, adapter: adapter) }
+
+  before do
+    allow(fake_proxy).to receive(:dns_for_group_cn).with(group_cn).and_return(group_member_dns)
+    stub_ldap_config(external_groups: external_groups)
+  end
+
+  it 'includes the EE module' do
+    expect(described_class).to include_module(EE::Gitlab::LDAP::User)
+  end
+
+  describe '#initialize' do
+    before do
+      expect(::EE::Gitlab::LDAP::Sync::Proxy).to receive(:open).with(auth_hash.provider).and_yield(fake_proxy)
+    end
+
+    context 'when the user is in an external group' do
+      let(:external_groups) { [group_cn] }
+
+      it "sets the user's external flag to true" do
+        expect(gl_user.external).to be_truthy
+      end
+    end
+
+    context 'when the user is not in an external group' do
+      it "sets the user's external flag to false" do
+        expect(gl_user.external).to be_falsey
+      end
+    end
+  end
+
+  describe '#set_external_with_external_groups' do
+    context 'when the LDAP user is in an external group' do
+      let(:external_groups) { [group_cn] }
+
+      before do
+        gl_user.update!(external: false)
+      end
+
+      it 'sets the GitLab user external flag to true' do
+        expect do
+          ldap_user.set_external_with_external_groups(fake_proxy)
+        end.to change { gl_user.external }.from(false).to(true)
+      end
+    end
+
+    context 'when the LDAP user is not in an external group' do
+      before do
+        gl_user.update!(external: true)
+      end
+
+      it 'sets the GitLab user external flag to true' do
+        expect do
+          ldap_user.set_external_with_external_groups(fake_proxy)
+        end.to change { gl_user.external }.from(true).to(false)
+      end
+    end
+  end
+
+  describe '#in_any_external_group?' do
+    subject { ldap_user.in_any_external_group?(fake_proxy) }
+
+    context 'when there is an external group' do
+      let(:external_groups) { [group_cn] }
+
+      context 'when the user is in an external group' do
+        it 'returns true' do
+          expect(subject).to be_truthy
+        end
+      end
+
+      context 'when the user is not in an external group' do
+        let(:group_member_dns) { ['uid=someone_else,ou=people,dc=example,dc=com'] }
+
+        it 'returns false' do
+          expect(subject).to be_falsey
+        end
+      end
+    end
+
+    context 'when are no external groups' do
+      it 'returns false' do
+        expect(subject).to be_falsey
+      end
+    end
+  end
+
+  describe '#in_group?' do
+    subject { ldap_user.in_group?(fake_proxy, group_cn) }
+
+    context 'when the LDAP user is in the group' do
+      it 'returns true' do
+        expect(subject).to be_truthy
+      end
+    end
+
+    context 'when the LDAP user is not in the group' do
+      let(:group_member_dns) { ['uid=someone_else,ou=people,dc=example,dc=com'] }
+
+      it 'returns false' do
+        expect(subject).to be_falsey
+      end
+    end
+  end
+end

spec/lib/gitlab/ldap/user_spec.rb

@@ -27,13 +27,6 @@ describe Gitlab::LDAP::User do
     OmniAuth::AuthHash.new(uid: 'my-uid', provider: 'ldapmain', info: info_upper_case)
   end
 
-  describe '#initialize' do
-    it 'calls #set_external_with_external_groups' do
-      expect_any_instance_of(described_class).to receive(:set_external_with_external_groups)
-      ldap_user
-    end
-  end
-
   describe '#changed?' do
     it "marks existing ldap user as changed" do
       create(:omniauth_user, extern_uid: 'my-uid', provider: 'ldapmain')
@@ -236,98 +229,4 @@ describe Gitlab::LDAP::User do
       end
     end
   end
-
-  describe '#set_external_with_external_groups' do
-    context 'when the LDAP user is in an external group' do
-      before do
-        expect(ldap_user).to receive(:in_any_external_group?).and_return(true)
-      end
-
-      it 'sets the GitLab user external flag to true' do
-        expect do
-          ldap_user.set_external_with_external_groups
-        end.to change { gl_user.external }.from(false).to(true)
-      end
-    end
-
-    context 'when the LDAP user is not in an external group' do
-      before do
-        expect(ldap_user).to receive(:in_any_external_group?).and_return(false)
-      end
-
-      it 'sets the GitLab user external flag to true' do
-        gl_user.external = true
-        gl_user.save
-
-        expect do
-          ldap_user.set_external_with_external_groups
-        end.to change { gl_user.external }.from(true).to(false)
-      end
-    end
-  end
-
-  describe '#in_any_external_group?' do
-    context 'when there is an external group' do
-      before do
-        stub_ldap_config(external_groups: ['foo'])
-      end
-
-      context 'when the user is in an external group' do
-        before do
-          expect(ldap_user).to receive(:in_group?).and_return(true)
-        end
-
-        it 'returns true' do
-          expect(ldap_user.in_any_external_group?).to be_truthy
-        end
-      end
-
-      context 'when the user is not in an external group' do
-        before do
-          expect(ldap_user).to receive(:in_group?).and_return(false)
-        end
-
-        it 'returns false' do
-          expect(ldap_user.in_any_external_group?).to be_falsey
-        end
-      end
-    end
-
-    context 'when are no external groups' do
-      before do
-        stub_ldap_config(external_groups: [])
-      end
-
-      it 'returns false' do
-        expect(ldap_user.in_any_external_group?).to be_falsey
-      end
-    end
-  end
-
-  describe '#in_group?' do
-    let(:proxy) { double(:proxy) }
-    let(:group) { 'foo' }
-    let(:member_dns_in_group) { ['uid=alice,ou=people,dc=example,dc=com'] }
-    subject { ldap_user.in_group?(proxy, group) }
-
-    before do
-      expect(proxy).to receive(:dns_for_group_cn).with(group).and_return(member_dns_in_group)
-    end
-
-    context 'when the LDAP user is in the group' do
-      before do
-        member_dns_in_group << ldap_user.auth_hash.uid
-      end
-
-      it 'returns true' do
-        expect(subject).to be_truthy
-      end
-    end
-
-    context 'when the LDAP user is not in the group' do
-      it 'returns false' do
-        expect(subject).to be_falsey
-      end
-    end
-  end
 end