Merge branch 'feaure-ldap-oauth-tests' into 'master'

Feaure ldap oauth tests

Concerns #154 Improve LDAP test coverage

See merge request !167

lib/gitlab/ldap/access.rb

@@ -29,7 +29,7 @@ module Gitlab
 
       def allowed?(user)
         if Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter)
-          !Gitlab::LDAP::Person.active_directory_disabled?(user.extern_uid, adapter)
+          !Gitlab::LDAP::Person.disabled_via_active_directory?(user.extern_uid, adapter)
         else
           false
         end
@@ -114,6 +114,19 @@ module Gitlab
         end
       end
 
+      # Loop throug all ldap conneted groups, and update the users link with it
+      def update_ldap_group_links(user)
+        gitlab_groups_with_ldap_link.each do |group|
+          active_group_links = group.ldap_group_links.where(cn: cns_with_access(get_ldap_user(user)))
+
+          if active_group_links.any?
+            group.add_users([user.id], fetch_group_access(group, user, active_group_links))
+          else
+            group.users.delete(user)
+          end
+        end
+      end
+
       def ldap_groups
         @ldap_groups ||= ::LdapGroupLink.distinct(:cn).pluck(:cn).map do |cn|
           Gitlab::LDAP::Group.find_by_cn(cn, adapter)
@@ -127,24 +140,12 @@ module Gitlab
         end.map(&:cn)
       end
 
+      private
       def gitlab_groups_with_ldap_link
         ::Group.includes(:ldap_group_links).references(:ldap_group_links).
           where.not(ldap_group_links: { id: nil })
       end
 
-      # Loop throug all ldap conneted groups, and update the users link with it
-      def update_ldap_group_links(user)
-        gitlab_groups_with_ldap_link.each do |group|
-          active_group_links = group.ldap_group_links.where(cn: cns_with_access(get_ldap_user(user)))
-
-          if active_group_links.any?
-            group.add_users([user.id], fetch_group_access(group, user, active_group_links))
-          else
-            group.users.delete(user)
-          end
-        end
-      end
-
       # Get the group_access for a give user.
       # Always respect the current level, never downgrade it.
       def fetch_group_access(group, user, active_group_links)

lib/gitlab/ldap/adapter.rb

@@ -110,7 +110,8 @@ module Gitlab
       end
 
       def dn_matches_filter?(dn, filter)
-        ldap_search(base: dn, filter: filter, scope: Net::LDAP::SearchScope_BaseObject, attributes: %w{dn}).any?
+        ldap_search(base: dn, filter: filter,
+          scope: Net::LDAP::SearchScope_BaseObject, attributes: %w{dn}).any?
       end
 
       def ldap_search(*args)

lib/gitlab/ldap/person.rb

@@ -16,7 +16,7 @@ module Gitlab
         adapter.user('dn', dn)
       end
 
-      def self.active_directory_disabled?(dn, adapter=nil)
+      def self.disabled_via_active_directory?(dn, adapter=nil)
         adapter ||= Gitlab::LDAP::Adapter.new
         adapter.dn_matches_filter?(dn, AD_USER_DISABLED)
       end

lib/gitlab/ldap/user.rb

@@ -92,10 +92,6 @@ module Gitlab
           model.where("provider = ? and lower(extern_uid) = ?", provider, uid.downcase).last
         end
 
-        def username
-          auth.info.nickname.to_s.force_encoding("utf-8")
-        end
-
         def provider
           'ldap'
         end

lib/gitlab/oauth/user.rb

@@ -67,12 +67,11 @@ module Gitlab
         end
 
         def uid
-          uid = auth.info.uid || auth.uid
-          uid = uid.to_s unless uid.nil?
-          uid
+          auth.uid.to_s
         end
 
         def email
+          return unless auth.info.respond_to?(:email)
           auth.info.email.downcase unless auth.info.email.nil?
         end
 
@@ -85,6 +84,7 @@ module Gitlab
         end
 
         def username
+          return unless auth.info.respond_to?(:nickname)
           auth.info.nickname.to_s.force_encoding("utf-8")
         end
 

spec/lib/gitlab/ldap/access_spec.rb

@@ -4,38 +4,68 @@ describe Gitlab::LDAP::Access do
   let(:access) { Gitlab::LDAP::Access.new }
   let(:user) { create(:user) }
 
-  describe :update_user_email do
-    let(:user_ldap) { create(:user, provider: 'ldap', extern_uid: "66048")}
 
-    it "should not update email if email attribute is not set" do
-      entry = Net::LDAP::Entry.new
-      Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry) }
-      updated = access.update_email(user_ldap)
-      updated.should == false
+  describe :allowed? do
+    subject { access.allowed?(user) }
+
+    context 'when the user cannot be found' do
+      before { Gitlab::LDAP::Person.stub(find_by_dn: nil) }
+
+      it { should be_false }
     end
 
-    it "should not update the email if the user has the same email in GitLab and in LDAP" do
-      entry = Net::LDAP::Entry.new
-      entry['mail'] = [user_ldap.email]
-      Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry) }
-      updated = access.update_email(user_ldap)
-      updated.should == false
+    context 'when the user is found' do
+      before { Gitlab::LDAP::Person.stub(find_by_dn: :ldap_user) }
+
+      context 'and the user is diabled via active directory' do
+        before { Gitlab::LDAP::Person.stub(disabled_via_active_directory?: true) }
+
+        it { should be_false }
+      end
+
+      context 'and has no disabled flag in active diretory' do
+        before { Gitlab::LDAP::Person.stub(disabled_via_active_directory?: false) }
+
+        it { should be_true }
+      end
     end
+  end
 
-    it "should not update the email if the user has the same email GitLab and in LDAP, but with upper case in LDAP" do
-      entry = Net::LDAP::Entry.new
-      entry['mail'] = [user_ldap.email.upcase]
-      Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry) }
-      updated = access.update_email(user_ldap)
-      updated.should == false
+  describe :update_permissions do
+    subject { access.update_permissions(user) }
+
+    before do
+      Gitlab.config.ldap['enabled'] = true
+      Gitlab.config.ldap['sync_ssh_keys'] = false
+      Gitlab.config.ldap['group_base'] = 'something'
+      Gitlab.config.ldap['admin_group'] = ''
     end
 
-    it "should update the email if the user email is different" do
-      entry = Net::LDAP::Entry.new
-      entry['mail'] = ["new_email@example.com"]
-      Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry) }
-      updated = access.update_email(user_ldap)
-      updated.should == true
+    after do
+      Gitlab.config.ldap['enabled'] = false
+    end
+
+    it "syncs ssh keys if enabled by configuration" do
+      Gitlab.config.ldap['sync_ssh_keys'] = true
+      expect(access).to receive(:update_ssh_keys).with(user).once
+
+      subject
+    end
+
+    it "does not update group permissions without a group base configured" do
+      Gitlab.config.ldap['group_base'] = ''
+      expect(access).not_to receive(:update_ldap_group_links).with(user)
+
+      subject
+    end
+
+    it "does update admin group permissions if admin group is configured" do
+      Gitlab.config.ldap['admin_group'] = 'NSA'
+
+      access.stub(:update_ldap_group_links)
+      expect(access).to receive(:update_admin_status).with(user)
+
+      subject
     end
   end
 
@@ -83,7 +113,6 @@ describe Gitlab::LDAP::Access do
     end
 
     context 'user has at least one LDAPKey' do
-
       it "should remove a SSH key if it is no longer in LDAP" do
         entry = Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com\n#{Gitlab.config.ldap['sync_ssh_keys']}:\n")
         Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry) }
@@ -105,35 +134,44 @@ describe Gitlab::LDAP::Access do
         expect(user_ldap.keys.size).to be(0)
       end
     end
-
   end
 
-  describe :allowed? do
-    subject { access.allowed?(user) }
-
-    context 'when the user cannot be found' do
-      before { Gitlab::LDAP::Person.stub(find_by_dn: nil) }
+  describe :update_user_email do
+    let(:user_ldap) { create(:user, provider: 'ldap', extern_uid: "66048")}
 
-      it { should be_false }
+    it "should not update email if email attribute is not set" do
+      entry = Net::LDAP::Entry.new
+      Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry) }
+      updated = access.update_email(user_ldap)
+      updated.should == false
     end
 
-    context 'when the user is found' do
-      before { Gitlab::LDAP::Person.stub(find_by_dn: :ldap_user) }
-
-      context 'and the Active Directory disabled flag is set' do
-        before { Gitlab::LDAP::Person.stub(active_directory_disabled?: true) }
-
-        it { should be_false }
-      end
+    it "should not update the email if the user has the same email in GitLab and in LDAP" do
+      entry = Net::LDAP::Entry.new
+      entry['mail'] = [user_ldap.email]
+      Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry) }
+      updated = access.update_email(user_ldap)
+      updated.should == false
+    end
 
-      context 'and the Active Directory disabled flag is not set' do
-        before { Gitlab::LDAP::Person.stub(active_directory_disabled?: false) }
+    it "should not update the email if the user has the same email GitLab and in LDAP, but with upper case in LDAP" do
+      entry = Net::LDAP::Entry.new
+      entry['mail'] = [user_ldap.email.upcase]
+      Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry) }
+      updated = access.update_email(user_ldap)
+      updated.should == false
+    end
 
-        it { should be_true }
-      end
+    it "should update the email if the user email is different" do
+      entry = Net::LDAP::Entry.new
+      entry['mail'] = ["new_email@example.com"]
+      Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry) }
+      updated = access.update_email(user_ldap)
+      updated.should == true
     end
   end
 
+
   describe :update_admin_status do
     let(:gitlab_user) { create(:user, provider: 'ldap', extern_uid: "admin2")}
     let(:gitlab_admin) { create(:admin, provider: 'ldap', extern_uid: "admin2")}
@@ -181,6 +219,66 @@ objectclass: posixGroup
     end
   end
 
+
+  describe :update_ldap_group_links do
+    let(:cns_with_access) { %w(ldap-group1 ldap-group2) }
+    let(:gitlab_group_1) { create :group }
+    let(:gitlab_group_2) { create :group }
+
+    before do
+      access.stub(:get_ldap_user)
+      access.stub(cns_with_access: cns_with_access)
+    end
+
+    context "non existing access for group-1, allowed via ldap-group1 as MASTER" do
+      before do
+        gitlab_group_1.ldap_group_links.create cn: 'ldap-group1', group_access: Gitlab::Access::MASTER
+      end
+
+      it "gives the user master access for group 1" do
+        access.update_ldap_group_links(user)
+        expect( gitlab_group_1.has_master?(user) ).to be_true
+      end
+    end
+
+    context "existing access as guest for group-1, allowed via ldap-group1 as DEVELOPER" do
+      before do
+        gitlab_group_1.users_groups.guests.create(user_id: user.id)
+        gitlab_group_1.ldap_group_links.create cn: 'ldap-group1', group_access: Gitlab::Access::MASTER
+      end
+
+      it "upgrades the users access to master for group 1" do
+        expect { access.update_ldap_group_links(user) }.to \
+          change{ gitlab_group_1.has_master?(user) }.from(false).to(true)
+      end
+    end
+
+    context "existing access as MASTER for group-1, allowed via ldap-group1 as DEVELOPER" do
+      before do
+        gitlab_group_1.users_groups.masters.create(user_id: user.id)
+        gitlab_group_1.ldap_group_links.create cn: 'ldap-group1', group_access: Gitlab::Access::DEVELOPER
+      end
+
+      it "keeps the users master access for group 1" do
+        expect { access.update_ldap_group_links(user) }.not_to \
+          change{ gitlab_group_1.has_master?(user) }
+      end
+    end
+
+    context "existing access as master for group-1, not allowed" do
+      before do
+        gitlab_group_1.users_groups.masters.create(user_id: user.id)
+        gitlab_group_1.ldap_group_links.create cn: 'ldap-group1', group_access: Gitlab::Access::MASTER
+        access.stub(cns_with_access: ['ldap-group2'])
+      end
+
+      it "removes user from gitlab_group_1" do
+        expect { access.update_ldap_group_links(user) }.to \
+          change{ gitlab_group_1.members.where(user_id: user).any? }.from(true).to(false)
+      end
+    end
+  end
+
   describe 'ldap_groups' do
     let(:ldap_group_1) do
       Net::LDAP::Entry.from_single_ldif_string(
@@ -250,64 +348,5 @@ objectclass: posixGroup
       expect(access.cns_with_access(ldap_user)).to eql ['group1']
     end
   end
-
-  describe :update_ldap_group_links do
-    let(:cns_with_access) { %w(ldap-group1 ldap-group2) }
-    let(:gitlab_group_1) { create :group }
-    let(:gitlab_group_2) { create :group }
-
-    before do
-      access.stub(:get_ldap_user)
-      access.stub(cns_with_access: cns_with_access)
-    end
-
-    context "non existing access for group-1, allowed via ldap-group1 as MASTER" do
-      before do
-        gitlab_group_1.ldap_group_links.create cn: 'ldap-group1', group_access: Gitlab::Access::MASTER
-      end
-
-      it "gives the user master access for group 1" do
-        access.update_ldap_group_links(user)
-        expect( gitlab_group_1.has_master?(user) ).to be_true
-      end
-    end
-
-    context "existing access as guest for group-1, allowed via ldap-group1 as DEVELOPER" do
-      before do
-        gitlab_group_1.users_groups.guests.create(user_id: user.id)
-        gitlab_group_1.ldap_group_links.create cn: 'ldap-group1', group_access: Gitlab::Access::MASTER
-      end
-
-      it "upgrades the users access to master for group 1" do
-        expect { access.update_ldap_group_links(user) }.to \
-          change{ gitlab_group_1.has_master?(user) }.from(false).to(true)
-      end
-    end
-
-    context "existing access as MASTER for group-1, allowed via ldap-group1 as DEVELOPER" do
-      before do
-        gitlab_group_1.users_groups.masters.create(user_id: user.id)
-        gitlab_group_1.ldap_group_links.create cn: 'ldap-group1', group_access: Gitlab::Access::DEVELOPER
-      end
-
-      it "keeps the users master access for group 1" do
-        expect { access.update_ldap_group_links(user) }.not_to \
-          change{ gitlab_group_1.has_master?(user) }
-      end
-    end
-
-    context "existing access as master for group-1, not allowed" do
-      before do
-        gitlab_group_1.users_groups.masters.create(user_id: user.id)
-        gitlab_group_1.ldap_group_links.create cn: 'ldap-group1', group_access: Gitlab::Access::MASTER
-        access.stub(cns_with_access: ['ldap-group2'])
-      end
-
-      it "removes user from gitlab_group_1" do
-        expect { access.update_ldap_group_links(user) }.to \
-          change{ gitlab_group_1.members.where(user_id: user).any? }.from(true).to(false)
-      end
-    end
-  end
 end
 

spec/lib/gitlab/ldap/user_auth_spec.rb

-require 'spec_helper'
-
-describe Gitlab::LDAP do
-  let(:gl_auth) { Gitlab::LDAP::User }
-
-  before do
-    Gitlab.config.stub(omniauth: {})
-
-    @info = double(
-      uid: '12djsak321',
-      name: 'John',
-      email: 'john@mail.com',
-      nickname: 'john'
-    )
-  end
-
-  describe :find_for_ldap_auth do
-    before do
-      @auth = double(
-        uid: '12djsak321',
-        info: @info,
-        provider: 'ldap'
-      )
-    end
-
-    it "should update credentials by email if missing uid" do
-      user = double('User')
-      User.stub find_by_extern_uid_and_provider: nil
-      User.stub(:find_by).with(hash_including(email: anything())) { user }
-      user.should_receive :update_attributes
-      gl_auth.find_or_create(@auth)
-    end
-
-    it "should update credentials by username if missing uid and Gitlab.config.ldap.allow_username_or_email_login is true" do
-      user = double('User')
-      value = Gitlab.config.ldap.allow_username_or_email_login
-      Gitlab.config.ldap['allow_username_or_email_login'] = true
-      User.stub find_by_extern_uid_and_provider: nil
-      User.stub(:find_by).with(hash_including(email: anything())) { nil }
-      User.stub(:find_by).with(hash_including(username: anything())) { user }
-      user.should_receive :update_attributes
-      gl_auth.find_or_create(@auth)
-      Gitlab.config.ldap['allow_username_or_email_login'] = value
-    end
-
-    it "should not update credentials by username if missing uid and Gitlab.config.ldap.allow_username_or_email_login is false" do
-      user = double('User')
-      value = Gitlab.config.ldap.allow_username_or_email_login
-      Gitlab.config.ldap['allow_username_or_email_login'] = false
-      User.stub find_by_extern_uid_and_provider: nil
-      User.stub(:find_by).with(hash_including(email: anything())) { nil }
-      User.stub(:find_by).with(hash_including(username: anything())) { user }
-      user.should_not_receive :update_attributes
-      gl_auth.find_or_create(@auth)
-      Gitlab.config.ldap['allow_username_or_email_login'] = value
-    end
-  end
-end

spec/lib/gitlab/ldap/user_spec.rb

+require 'spec_helper'
+
+describe Gitlab::LDAP::User do
+  let(:gl_auth) { Gitlab::LDAP::User }
+  let(:info) do
+    double(
+      name: 'John',
+      email: 'john@example.com',
+      nickname: 'john'
+    )
+  end
+  before { Gitlab.config.stub(omniauth: {}) }
+
+  describe :find_or_create do
+    let(:auth) do
+      double(info: info, provider: 'ldap', uid: 'my-uid')
+    end
+
+    it "finds the user if already existing" do
+      existing_user = create(:user, extern_uid: 'my-uid', provider: 'ldap')
+
+      expect{ gl_auth.find_or_create(auth) }.to_not change{ User.count }
+    end
+
+    it "connects to existing non-ldap user if the email matches" do
+      existing_user = create(:user, email: 'john@example.com')
+      expect{ gl_auth.find_or_create(auth) }.to_not change{ User.count }
+
+      existing_user.reload
+      expect(existing_user.extern_uid).to eql 'my-uid'
+      expect(existing_user.provider).to eql 'ldap'
+    end
+
+    it "creates a new user if not found" do
+      expect{ gl_auth.find_or_create(auth) }.to change{ User.count }.by(1)
+    end
+  end
+end

spec/lib/oauth_spec.rb → spec/lib/gitlab/oauth/user_spec.rb

@@ -2,44 +2,82 @@ require 'spec_helper'
 
 describe Gitlab::OAuth::User do
   let(:gl_auth) { Gitlab::OAuth::User }
-
-  before do
-    Gitlab.config.stub(omniauth: {})
-
-    @info = double(
-      uid: '12djsak321',
+  let(:info) do
+    double(
       nickname: 'john',
       name: 'John',
       email: 'john@mail.com'
     )
   end
 
+  before do
+    Gitlab.config.stub(omniauth: {})
+  end
+
+  describe :find do
+    let!(:existing_user) { create(:user, extern_uid: 'my-uid', provider: 'my-provider') }
+
+    it "finds an existing user based on uid and provider (facebook)" do
+      auth = double(info: double(name: 'John'), uid: 'my-uid', provider: 'my-provider')
+      assert gl_auth.find(auth)
+    end
+
+    it "finds an existing user based on nested uid and provider" do
+      auth = double(info: info, uid: 'my-uid', provider: 'my-provider')
+      assert gl_auth.find(auth)
+    end
+  end
+
   describe :create do
     it "should create user from LDAP" do
-      @auth = double(info: @info, provider: 'ldap')
-      user = gl_auth.create(@auth)
+      auth = double(info: info, uid: 'my-uid', provider: 'ldap')
+      user = gl_auth.create(auth)
 
       user.should be_valid
-      user.extern_uid.should == @info.uid
+      user.extern_uid.should == auth.uid
       user.provider.should == 'ldap'
     end
 
     it "should create user from Omniauth" do
-      @auth = double(info: @info, provider: 'twitter')
-      user = gl_auth.create(@auth)
+      auth = double(info: info, uid: 'my-uid', provider: 'twitter')
+      user = gl_auth.create(auth)
 
       user.should be_valid
-      user.extern_uid.should == @info.uid
+      user.extern_uid.should == auth.uid
       user.provider.should == 'twitter'
     end
 
     it "should apply defaults to user" do
-      @auth = double(info: @info, provider: 'ldap')
-      user = gl_auth.create(@auth)
+      auth = double(info: info, uid: 'my-uid', provider: 'ldap')
+      user = gl_auth.create(auth)
 
       user.should be_valid
       user.projects_limit.should == Gitlab.config.gitlab.default_projects_limit
       user.can_create_group.should == Gitlab.config.gitlab.default_can_create_group
     end
+
+    it "Set a temp email address if not provided (like twitter does)" do
+      info = double(
+        uid: 'my-uid',
+        nickname: 'john',
+        name: 'John'
+      )
+      auth = double(info: info, uid: 'my-uid', provider: 'my-provider')
+
+      user = gl_auth.create(auth)
+      expect(user.email).to_not be_empty
+    end
+
+    it 'generates a username if non provided (google)' do
+      info = double(
+        uid: 'my-uid',
+        name: 'John',
+        email: 'john@example.com'
+      )
+      auth = double(info: info, uid: 'my-uid', provider: 'my-provider')
+
+      user = gl_auth.create(auth)
+      expect(user.username).to eql 'john'
+    end
   end
 end