From 0c34fa3ea0662f94fdc565cfd3f921db40733821 Mon Sep 17 00:00:00 2001
From: Jan-Willem van der Meer <mail@jewilmeer.nl>
Date: Mon, 1 Sep 2014 12:59:04 +0200
Subject: [PATCH 1/8] Add tests for finding an oauth authenticated user

---
 lib/gitlab/oauth/user.rb           |  2 +-
 spec/lib/gitlab/oauth/user_spec.rb | 37 +++++++++++++++++++++---------
 2 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/lib/gitlab/oauth/user.rb b/lib/gitlab/oauth/user.rb
index 0056eb3a28..8c426d810c 100644
--- a/lib/gitlab/oauth/user.rb
+++ b/lib/gitlab/oauth/user.rb
@@ -67,7 +67,7 @@ module Gitlab
         end
 
         def uid
-          uid = auth.info.uid || auth.uid
+          uid = auth.info.try(:uid) || auth.uid
           uid = uid.to_s unless uid.nil?
           uid
         end
diff --git a/spec/lib/gitlab/oauth/user_spec.rb b/spec/lib/gitlab/oauth/user_spec.rb
index 2f15b5e034..a79ba3b588 100644
--- a/spec/lib/gitlab/oauth/user_spec.rb
+++ b/spec/lib/gitlab/oauth/user_spec.rb
@@ -2,39 +2,54 @@ 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(
+      uid: 'my-uid',
       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, 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')
+      @auth = double(info: info, provider: 'ldap')
       user = gl_auth.create(@auth)
 
       user.should be_valid
-      user.extern_uid.should == @info.uid
+      user.extern_uid.should == info.uid
       user.provider.should == 'ldap'
     end
 
     it "should create user from Omniauth" do
-      @auth = double(info: @info, provider: 'twitter')
+      @auth = double(info: info, provider: 'twitter')
       user = gl_auth.create(@auth)
 
       user.should be_valid
-      user.extern_uid.should == @info.uid
+      user.extern_uid.should == info.uid
       user.provider.should == 'twitter'
     end
 
     it "should apply defaults to user" do
-      @auth = double(info: @info, provider: 'ldap')
+      @auth = double(info: info, provider: 'ldap')
       user = gl_auth.create(@auth)
 
       user.should be_valid

From 0ec4abf73c5de8b36c33beba443d998652042ef5 Mon Sep 17 00:00:00 2001
From: Jan-Willem van der Meer <mail@jewilmeer.nl>
Date: Mon, 1 Sep 2014 13:02:41 +0200
Subject: [PATCH 2/8] Use local vars for tests

---
 spec/lib/gitlab/oauth/user_spec.rb | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/spec/lib/gitlab/oauth/user_spec.rb b/spec/lib/gitlab/oauth/user_spec.rb
index a79ba3b588..347723f694 100644
--- a/spec/lib/gitlab/oauth/user_spec.rb
+++ b/spec/lib/gitlab/oauth/user_spec.rb
@@ -31,8 +31,8 @@ describe Gitlab::OAuth::User do
 
   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, provider: 'ldap')
+      user = gl_auth.create(auth)
 
       user.should be_valid
       user.extern_uid.should == info.uid
@@ -40,8 +40,8 @@ describe Gitlab::OAuth::User do
     end
 
     it "should create user from Omniauth" do
-      @auth = double(info: info, provider: 'twitter')
-      user = gl_auth.create(@auth)
+      auth = double(info: info, provider: 'twitter')
+      user = gl_auth.create(auth)
 
       user.should be_valid
       user.extern_uid.should == info.uid
@@ -49,8 +49,8 @@ describe Gitlab::OAuth::User do
     end
 
     it "should apply defaults to user" do
-      @auth = double(info: info, provider: 'ldap')
-      user = gl_auth.create(@auth)
+      auth = double(info: info, provider: 'ldap')
+      user = gl_auth.create(auth)
 
       user.should be_valid
       user.projects_limit.should == Gitlab.config.gitlab.default_projects_limit

From 5801d520a75ce03339944d67cfde6cc3c1b05cfc Mon Sep 17 00:00:00 2001
From: Jan-Willem van der Meer <mail@jewilmeer.nl>
Date: Mon, 1 Sep 2014 13:48:17 +0200
Subject: [PATCH 3/8] Handle user creation if email is not provided

This fixes #1541
---
 lib/gitlab/oauth/user.rb           |  1 +
 spec/lib/gitlab/oauth/user_spec.rb | 12 ++++++++++++
 2 files changed, 13 insertions(+)

diff --git a/lib/gitlab/oauth/user.rb b/lib/gitlab/oauth/user.rb
index 8c426d810c..5efd5799f9 100644
--- a/lib/gitlab/oauth/user.rb
+++ b/lib/gitlab/oauth/user.rb
@@ -73,6 +73,7 @@ module Gitlab
         end
 
         def email
+          return unless auth.info.respond_to?(:email)
           auth.info.email.downcase unless auth.info.email.nil?
         end
 
diff --git a/spec/lib/gitlab/oauth/user_spec.rb b/spec/lib/gitlab/oauth/user_spec.rb
index 347723f694..d6ac0c0896 100644
--- a/spec/lib/gitlab/oauth/user_spec.rb
+++ b/spec/lib/gitlab/oauth/user_spec.rb
@@ -56,5 +56,17 @@ describe Gitlab::OAuth::User do
       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, provider: 'my-provider')
+
+      user = gl_auth.create(auth)
+      expect(user.email).to_not be_empty
+    end
   end
 end

From 262276c5f55802541f4df19a44488b4f5239054c Mon Sep 17 00:00:00 2001
From: Jan-Willem van der Meer <mail@jewilmeer.nl>
Date: Mon, 1 Sep 2014 14:26:10 +0200
Subject: [PATCH 4/8] Ensure oath callbacks without a nickname work (google)

---
 lib/gitlab/oauth/user.rb           |  1 +
 spec/lib/gitlab/oauth/user_spec.rb | 12 ++++++++++++
 2 files changed, 13 insertions(+)

diff --git a/lib/gitlab/oauth/user.rb b/lib/gitlab/oauth/user.rb
index 5efd5799f9..1b04dffc3a 100644
--- a/lib/gitlab/oauth/user.rb
+++ b/lib/gitlab/oauth/user.rb
@@ -86,6 +86,7 @@ module Gitlab
         end
 
         def username
+          return unless auth.info.respond_to?(:nickname)
           auth.info.nickname.to_s.force_encoding("utf-8")
         end
 
diff --git a/spec/lib/gitlab/oauth/user_spec.rb b/spec/lib/gitlab/oauth/user_spec.rb
index d6ac0c0896..7dcc849454 100644
--- a/spec/lib/gitlab/oauth/user_spec.rb
+++ b/spec/lib/gitlab/oauth/user_spec.rb
@@ -68,5 +68,17 @@ describe Gitlab::OAuth::User do
       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, provider: 'my-provider')
+
+      user = gl_auth.create(auth)
+      expect(user.username).to eql 'john'
+    end
   end
 end

From ca17e4b7adae4583b915f6ea7f2714b2225cd73c Mon Sep 17 00:00:00 2001
From: Jan-Willem van der Meer <mail@jewilmeer.nl>
Date: Mon, 1 Sep 2014 14:30:31 +0200
Subject: [PATCH 5/8] Remove duplicate method

---
 lib/gitlab/ldap/user.rb | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb
index 79aa145d87..e6aa389099 100644
--- a/lib/gitlab/ldap/user.rb
+++ b/lib/gitlab/ldap/user.rb
@@ -77,10 +77,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

From 92a9964940784063810d068f230088d7f297ba54 Mon Sep 17 00:00:00 2001
From: Jan-Willem van der Meer <mail@jewilmeer.nl>
Date: Mon, 1 Sep 2014 15:30:46 +0200
Subject: [PATCH 6/8] Add basic find / create specs for LDAP user

---
 spec/lib/gitlab/ldap/user_spec.rb | 52 +++++++++++++------------------
 1 file changed, 22 insertions(+), 30 deletions(-)

diff --git a/spec/lib/gitlab/ldap/user_spec.rb b/spec/lib/gitlab/ldap/user_spec.rb
index de5717417f..725338965b 100644
--- a/spec/lib/gitlab/ldap/user_spec.rb
+++ b/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_for_ldap_auth do
-    before do
-      @auth = double(
-        uid: '12djsak321',
-        info: @info,
-        provider: 'ldap'
-      )
+  describe :find_or_create do
+    let(:auth) do
+      double(info: info, provider: 'ldap', uid: 'my-uid')
     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 "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 "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 "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

From 26b14dd2d597d7bd5579cfcbad456abb0af6a5e5 Mon Sep 17 00:00:00 2001
From: Jan-Willem van der Meer <mail@jewilmeer.nl>
Date: Mon, 1 Sep 2014 16:31:27 +0200
Subject: [PATCH 7/8] Get uid from auth instead of info hash

As found in the omniauth specs: https://github.com/intridea/omniauth/wiki/Auth-Hash-Schema
---
 lib/gitlab/oauth/user.rb           |  4 +---
 spec/lib/gitlab/oauth/user_spec.rb | 17 ++++++++---------
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/lib/gitlab/oauth/user.rb b/lib/gitlab/oauth/user.rb
index 1b04dffc3a..9670aad2c5 100644
--- a/lib/gitlab/oauth/user.rb
+++ b/lib/gitlab/oauth/user.rb
@@ -67,9 +67,7 @@ module Gitlab
         end
 
         def uid
-          uid = auth.info.try(:uid) || auth.uid
-          uid = uid.to_s unless uid.nil?
-          uid
+          auth.uid.to_s
         end
 
         def email
diff --git a/spec/lib/gitlab/oauth/user_spec.rb b/spec/lib/gitlab/oauth/user_spec.rb
index 7dcc849454..c241e19860 100644
--- a/spec/lib/gitlab/oauth/user_spec.rb
+++ b/spec/lib/gitlab/oauth/user_spec.rb
@@ -4,7 +4,6 @@ describe Gitlab::OAuth::User do
   let(:gl_auth) { Gitlab::OAuth::User }
   let(:info) do
     double(
-      uid: 'my-uid',
       nickname: 'john',
       name: 'John',
       email: 'john@mail.com'
@@ -24,32 +23,32 @@ describe Gitlab::OAuth::User do
     end
 
     it "finds an existing user based on nested uid and provider" do
-      auth = double(info: info, provider: 'my-provider')
+      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')
+      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')
+      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')
+      auth = double(info: info, uid: 'my-uid', provider: 'ldap')
       user = gl_auth.create(auth)
 
       user.should be_valid
@@ -63,7 +62,7 @@ describe Gitlab::OAuth::User do
         nickname: 'john',
         name: 'John'
       )
-      auth = double(info: info, provider: 'my-provider')
+      auth = double(info: info, uid: 'my-uid', provider: 'my-provider')
 
       user = gl_auth.create(auth)
       expect(user.email).to_not be_empty
@@ -75,7 +74,7 @@ describe Gitlab::OAuth::User do
         name: 'John',
         email: 'john@example.com'
       )
-      auth = double(info: info, provider: 'my-provider')
+      auth = double(info: info, uid: 'my-uid', provider: 'my-provider')
 
       user = gl_auth.create(auth)
       expect(user.username).to eql 'john'

From c0323b40ee5633f2808f52f98bde0509a2f3ee59 Mon Sep 17 00:00:00 2001
From: Jan-Willem van der Meer <mail@jewilmeer.nl>
Date: Mon, 1 Sep 2014 16:35:18 +0200
Subject: [PATCH 8/8] Refactor: beter naming for active directory disabled
 users

---
 lib/gitlab/ldap/access.rb           | 2 +-
 lib/gitlab/ldap/person.rb           | 2 +-
 spec/lib/gitlab/ldap/access_spec.rb | 8 ++++----
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb
index 62709a1294..c054b6f586 100644
--- a/lib/gitlab/ldap/access.rb
+++ b/lib/gitlab/ldap/access.rb
@@ -28,7 +28,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
diff --git a/lib/gitlab/ldap/person.rb b/lib/gitlab/ldap/person.rb
index 9ad6618bd4..87c3d711db 100644
--- a/lib/gitlab/ldap/person.rb
+++ b/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
diff --git a/spec/lib/gitlab/ldap/access_spec.rb b/spec/lib/gitlab/ldap/access_spec.rb
index d8c107502b..2307a03f65 100644
--- a/spec/lib/gitlab/ldap/access_spec.rb
+++ b/spec/lib/gitlab/ldap/access_spec.rb
@@ -16,14 +16,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