Add extra tests around OAUTH / LDAP find_or_create

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

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

@@ -120,14 +120,14 @@ describe Gitlab::LDAP::Access do
     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) }
+      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 the Active Directory disabled flag is not set' do
-        before { Gitlab::LDAP::Person.stub(active_directory_disabled?: false) }
+      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

spec/lib/gitlab/ldap/user_spec.rb

@@ -2,45 +2,37 @@ require 'spec_helper'
 
 describe Gitlab::LDAP::User do
   let(:gl_auth) { Gitlab::LDAP::User }
-
-  before do
-    Gitlab.config.stub(omniauth: {})
-
-    @info = double(
-      uid: '12djsak321',
+  let(:info) do
+    double(
       name: 'John',
-      email: 'john@mail.com',
+      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')
 
-  describe :find_for_ldap_auth do
-    before do
-      @auth = double(
-        uid: '12djsak321',
-        info: @info,
-        provider: 'ldap'
-      )
+      expect{ gl_auth.find_or_create(auth) }.to_not change{ User.count }
     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)
+    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 "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
+    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
\ No newline at end of file
+end

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
\ No newline at end of file
+end