Refactor to make implementation details private

* Expand initialize spec coverage
* Made implementation details private
* Removed tests of now private methods
* Simplified the private methods

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

@@ -9,36 +9,36 @@ module EE
         def initialize(auth_hash)
           super
 
-          with_proxy(auth_hash.provider) do |proxy|
-            set_external_with_external_groups(proxy)
-          end
+          set_external_with_external_groups
         end
 
+        private
+
         # 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)
+        def set_external_with_external_groups
+          gl_user.external = in_any_external_group?
         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)
+        def in_any_external_group?
+          with_proxy do |proxy|
+            external_groups = proxy.adapter.config.external_groups
+            external_groups.any? do |group_cn|
+              in_group?(group_cn, proxy)
+            end
           end
         end
 
         # Returns true if the User is a member of the group.
-        def in_group?(proxy, group_cn)
+        def in_group?(group_cn, proxy)
           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)
+        def with_proxy(&block)
+          ::EE::Gitlab::LDAP::Sync::Proxy.open(auth_hash.provider, &block)
         end
       end
     end

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

@@ -30,91 +30,75 @@ describe Gitlab::LDAP::User do
   end
 
   describe '#initialize' do
-    context 'when the user is in an external group' do
+    context 'when there is one 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 there is another user in the external group' do
+        context 'when the user is in the external group' do
+          let(:group_member_dns) { ['uid=someone_else,ou=people,dc=example,dc=com', auth_hash.uid] }
 
-    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
+          it "sets the user's external flag to true" do
+            expect(gl_user.external).to be_truthy
+          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] }
+        context 'when the user is not in the external group' do
+          let(:group_member_dns) { ['uid=someone_else,ou=people,dc=example,dc=com'] }
 
-      before do
-        gl_user.update!(external: false)
+          it "sets the user's external flag to false" do
+            expect(gl_user.external).to be_falsey
+          end
+        end
       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 there are no other users in the external group' do
+        context 'when the user is in the external group' do
+          let(:group_member_dns) { [auth_hash.uid] }
 
-    context 'when the LDAP user is not in an external group' do
-      before do
-        gl_user.update!(external: true)
-      end
+          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 the external group' do
+          let(:group_member_dns) { [] }
 
-      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)
+          it "sets the user's external flag to false" do
+            expect(gl_user.external).to be_falsey
+          end
+        end
       end
     end
-  end
 
-  describe '#in_any_external_group?' do
-    subject { ldap_user.in_any_external_group?(fake_proxy) }
+    context 'when there is more than one external group' do
+      let(:external_groups) { ['bar', group_cn] }
 
-    context 'when there is an external group' do
-      let(:external_groups) { [group_cn] }
+      before do
+        allow(fake_proxy).to receive(:dns_for_group_cn).with('bar').and_return(['uid=someone_else,ou=people,dc=example,dc=com'])
+      end
 
       context 'when the user is in an external group' do
-        it 'returns true' do
-          expect(subject).to be_truthy
+        let(:group_member_dns) { [auth_hash.uid] }
+
+        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
-        let(:group_member_dns) { ['uid=someone_else,ou=people,dc=example,dc=com'] }
+        let(:group_member_dns) { [] }
 
-        it 'returns false' do
-          expect(subject).to be_falsey
+        it "sets the user's external flag to false" do
+          expect(gl_user.external).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'] }
+    context 'when there are no external groups' do
+      let(:external_groups) { [] }
 
-      it 'returns false' do
-        expect(subject).to be_falsey
+      it "sets the user's external flag to false" do
+        expect(gl_user.external).to be_falsey
       end
     end
   end