Merge branch 'feature-oauth-refactoring' into 'master'

app/controllers/omniauth_callbacks_controller.rb

@@ -15,15 +15,17 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
     error.to_s.humanize if error
   end
 
+  # We only find ourselves here
+  # if the authentication to LDAP was successful.
   def ldap
-    # We only find ourselves here
-    # if the authentication to LDAP was successful.
-    @user = Gitlab::LDAP::User.find_or_create(oauth)
-    @user.remember_me = true if @user.persisted?
+    @user = Gitlab::LDAP::User.new(oauth)
+    @user.save if @user.changed? # will also save new users
+    gl_user = @user.gl_user
+    gl_user.remember_me = true if @user.persisted?
 
     # Do additional LDAP checks for the user filter and EE features
-    if Gitlab::LDAP::Access.allowed?(@user)
-      sign_in_and_redirect(@user)
+    if Gitlab::LDAP::Access.allowed?(gl_user)
+      sign_in_and_redirect(gl_user)
     else
       flash[:alert] = "Access denied for your LDAP account."
       redirect_to new_user_session_path
@@ -46,24 +48,17 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
       current_user.save
       redirect_to profile_path
     else
-      @user = Gitlab::OAuth::User.find(oauth)
+      @user = Gitlab::OAuth::User.new(oauth)
 
-      # Create user if does not exist
-      # and allow_single_sign_on is true
-      if Gitlab.config.omniauth['allow_single_sign_on'] && !@user
-        @user, errors = Gitlab::OAuth::User.create(oauth)
+      if Gitlab.config.omniauth['allow_single_sign_on'] && @user.new?
+        @user.save
       end
 
-      if @user && !errors
-        sign_in_and_redirect(@user)
+      if @user.valid?
+        sign_in_and_redirect(@user.gl_user)
       else
-        if errors
-          error_message = errors.map{ |attribute, message| "#{attribute} #{message}" }.join(", ")
-          redirect_to omniauth_error_path(oauth['provider'], error: error_message) and return
-        else
-          flash[:notice] = "There's no such user!"
-        end
-        redirect_to new_user_session_path
+        error_message = @user.gl_user.errors.map{ |attribute, message| "#{attribute} #{message}" }.join(", ")
+        redirect_to omniauth_error_path(oauth['provider'], error: error_message) and return
       end
     end
   end

lib/gitlab/ldap/user.rb

@@ -10,22 +10,6 @@ module Gitlab
   module LDAP
     class User < Gitlab::OAuth::User
       class << self
-        def find_or_create(auth_hash)
-          self.auth_hash = auth_hash
-          find(auth_hash) || find_and_connect_by_email(auth_hash) || create(auth_hash)
-        end
-
-        def find_and_connect_by_email(auth_hash)
-          self.auth_hash = auth_hash
-          user = model.find_by(email: self.auth_hash.email)
-
-          if user
-            user.update_attributes(extern_uid: auth_hash.uid, provider: auth_hash.provider)
-            Gitlab::AppLogger.info("(LDAP) Updating legacy LDAP user #{self.auth_hash.email} with extern_uid => #{auth_hash.uid}")
-            return user
-          end
-        end
-
         def authenticate(login, password)
           # Check user against LDAP backend if user is not authenticated
           # Only check with valid login and password to prevent anonymous bind results
@@ -44,10 +28,18 @@ module Gitlab
           @adapter ||= OmniAuth::LDAP::Adaptor.new(ldap_conf)
         end
 
-        protected
+        def user_filter(login)
+          filter = Net::LDAP::Filter.eq(adapter.uid, login)
+          # Apply LDAP user filter if present
+          if ldap_conf['user_filter'].present?
+            user_filter = Net::LDAP::Filter.construct(ldap_conf['user_filter'])
+            filter = Net::LDAP::Filter.join(filter, user_filter)
+          end
+          filter
+        end
 
-        def find_by_uid_and_provider
-          find_by_uid(auth_hash.uid)
+        def ldap_conf
+          Gitlab.config.ldap
         end
 
         def find_by_uid(uid)
@@ -58,24 +50,39 @@ module Gitlab
         def provider
           'ldap'
         end
+      end
 
-        def raise_error(message)
-          raise OmniAuth::Error, "(LDAP) " + message
-        end
+      def initialize(auth_hash)
+        super
+        update_user_attributes
+      end
 
-        def ldap_conf
-          Gitlab.config.ldap
-        end
+      # instance methods
+      def gl_user
+        @gl_user ||= find_by_uid_and_provider || find_by_email || build_new_user
+      end
 
-        def user_filter(login)
-          filter = Net::LDAP::Filter.eq(adapter.uid, login)
-          # Apply LDAP user filter if present
-          if ldap_conf['user_filter'].present?
-            user_filter = Net::LDAP::Filter.construct(ldap_conf['user_filter'])
-            filter = Net::LDAP::Filter.join(filter, user_filter)
-          end
-          filter
-        end
+      def find_by_uid_and_provider
+        # LDAP distinguished name is case-insensitive
+        model.
+          where(provider: auth_hash.provider).
+          where('lower(extern_uid) = ?', auth_hash.uid.downcase).last
+      end
+
+      def find_by_email
+        model.find_by(email: auth_hash.email)
+      end
+
+      def update_user_attributes
+        gl_user.attributes = {
+          extern_uid: auth_hash.uid,
+          provider: auth_hash.provider,
+          email: auth_hash.email
+        }
+      end
+
+      def changed?
+        gl_user.changed?
       end
 
       def needs_blocking?

lib/gitlab/oauth/auth_hash.rb

@@ -21,7 +21,7 @@ module Gitlab
       end
 
       def name
-        (info.name || full_name).to_s.force_encoding('utf-8')
+        (info.try(:name) || full_name).to_s.force_encoding('utf-8')
       end
 
       def full_name

lib/gitlab/oauth/user.rb

@@ -6,55 +6,52 @@
 module Gitlab
   module OAuth
     class User
-      class << self
-        attr_reader :auth_hash
+      attr_accessor :auth_hash, :gl_user
 
-        def find(auth_hash)
-          self.auth_hash = auth_hash
-          find_by_uid_and_provider
-        end
-
-        def create(auth_hash)
-          user = new(auth_hash)
-          user.save_and_trigger_callbacks
-        end
+      def initialize(auth_hash)
+        self.auth_hash = auth_hash
+      end
 
-        def model
-          ::User
-        end
+      def persisted?
+        gl_user.persisted?
+      end
 
-        def auth_hash=(auth_hash)
-          @auth_hash = AuthHash.new(auth_hash)
-        end
+      def new?
+        !gl_user.persisted?
+      end
 
-        protected
-        def find_by_uid_and_provider
-          model.where(provider: auth_hash.provider, extern_uid: auth_hash.uid).last
-        end
+      def valid?
+        gl_user.valid?
       end
 
-      # Instance methods
-      attr_accessor :auth_hash, :user
+      def save
+        gl_user.save!
+        log.info "(OAuth) saving user #{auth_hash.email} from login with extern_uid => #{auth_hash.uid}"
+        gl_user.block if needs_blocking?
 
-      def initialize(auth_hash)
-        self.auth_hash = auth_hash
-        self.user = self.class.model.new(user_attributes)
-        user.skip_confirmation!
+        gl_user
+      rescue ActiveRecord::RecordInvalid => e
+        log.info "(OAuth) Error saving user: #{gl_user.errors.full_messages}"
+        return self, e.record.errors
+      end
+
+      def gl_user
+        @user ||= find_by_uid_and_provider || build_new_user
       end
 
+      protected
       def auth_hash=(auth_hash)
         @auth_hash = AuthHash.new(auth_hash)
       end
 
-      def save_and_trigger_callbacks
-        user.save!
-        log.info "(OAuth) Creating user #{auth_hash.email} from login with extern_uid => #{auth_hash.uid}"
-        user.block if needs_blocking?
+      def find_by_uid_and_provider
+        model.where(provider: auth_hash.provider, extern_uid: auth_hash.uid).last
+      end
 
-        user
-      rescue ActiveRecord::RecordInvalid => e
-        log.info "(OAuth) Email #{e.record.errors[:email]}. Username #{e.record.errors[:username]}"
-        return nil, e.record.errors
+      def build_new_user
+        model.new(user_attributes).tap do |user|
+          user.skip_confirmation!
+        end
       end
 
       def user_attributes
@@ -80,6 +77,10 @@ module Gitlab
       def needs_blocking?
         Gitlab.config.omniauth['block_auto_created_users']
       end
+
+      def model
+        ::User
+      end
     end
   end
 end

spec/lib/gitlab/ldap/user_spec.rb

 require 'spec_helper'
 
 describe Gitlab::LDAP::User do
-  let(:gl_user) { Gitlab::LDAP::User }
+  let(:gl_user) { Gitlab::LDAP::User.new(auth_hash) }
   let(:info) do
-    double(
+    {
       name: 'John',
       email: 'john@example.com',
       nickname: 'john'
-    )
+    }
+  end
+  let(:auth_hash) do
+    double(uid: 'my-uid', provider: 'ldap', info: double(info))
   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_user.find_or_create(auth) }.to_not change{ User.count }
+      expect{ gl_user.save }.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_user.find_or_create(auth) }.to_not change{ User.count }
+      expect{ gl_user.save }.to_not change{ User.count }
 
       existing_user.reload
       expect(existing_user.extern_uid).to eql 'my-uid'
@@ -32,7 +30,7 @@ describe Gitlab::LDAP::User do
     end
 
     it "creates a new user if not found" do
-      expect{ gl_user.find_or_create(auth) }.to change{ User.count }.by(1)
+      expect{ gl_user.save }.to change{ User.count }.by(1)
     end
   end
 

spec/lib/gitlab/oauth/auth_hash_spec.rb

+require 'spec_helper'
+
+describe Gitlab::OAuth::AuthHash do
+  let(:auth_hash) do
+    Gitlab::OAuth::AuthHash.new(double({
+      provider: 'twitter',
+      uid: uid,
+      info: double(info_hash)
+    }))
+  end
+  let(:uid) { 'my-uid' }
+  let(:email) { 'my-email@example.com' }
+  let(:nickname) { 'my-nickname' }
+  let(:info_hash) {
+    {
+      email: email,
+      nickname: nickname,
+      name: 'John',
+      first_name: "John",
+      last_name: "Who"
+    }
+  }
+
+  context "defaults" do
+    it { expect(auth_hash.provider).to eql 'twitter' }
+    it { expect(auth_hash.uid).to eql uid }
+    it { expect(auth_hash.email).to eql email }
+    it { expect(auth_hash.username).to eql nickname }
+    it { expect(auth_hash.name).to eql "John" }
+    it { expect(auth_hash.password).to_not be_empty }
+  end
+
+  context "email not provided" do
+    before { info_hash.delete(:email) }
+    it "generates a temp email" do
+      expect( auth_hash.email).to start_with('temp-email-for-oauth')
+    end
+  end
+
+  context "username not provided" do
+    before { info_hash.delete(:nickname) }
+
+    it "takes the first part of the email as username" do
+      expect( auth_hash.username ).to eql "my-email"
+    end
+  end
+
+  context "name not provided" do
+    before { info_hash.delete(:name) }
+
+    it "concats first and lastname as the name" do
+      expect( auth_hash.name ).to eql "John Who"
+    end
+  end
+end
\ No newline at end of file

spec/lib/gitlab/oauth/user_spec.rb

 require 'spec_helper'
 
 describe Gitlab::OAuth::User do
-  let(:gl_auth) { Gitlab::OAuth::User }
-  let(:info) do
-    double(
+  let(:oauth_user) { Gitlab::OAuth::User.new(auth_hash) }
+  let(:gl_user) { oauth_user.gl_user }
+  let(:uid) { 'my-uid' }
+  let(:provider) { 'my-provider' }
+  let(:auth_hash) { double(uid: uid, provider: provider, info: double(info_hash)) }
+  let(:info_hash) do
+    {
       nickname: 'john',
       name: 'John',
       email: 'john@mail.com'
-    )
+    }
   end
 
-  before do
-    Gitlab.config.stub(omniauth: {})
-  end
-
-  describe :find do
+  describe :persisted? 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)
+      expect( oauth_user.persisted? ).to be_true
     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)
+    it "returns false if use is not found in database" do
+      auth_hash.stub(uid: 'non-existing')
+      expect( oauth_user.persisted? ).to be_false
     end
   end
 
-  describe :create do
-    it "should create user from LDAP" do
-      auth = double(info: info, uid: 'my-uid', provider: 'ldap')
-      user = gl_auth.create(auth)
-
-      user.should be_valid
-      user.extern_uid.should == auth.uid
-      user.provider.should == 'ldap'
-    end
-
-    it "should create user from Omniauth" do
-      auth = double(info: info, uid: 'my-uid', provider: 'twitter')
-      user = gl_auth.create(auth)
+  describe :save do
+    context "LDAP" do
+      let(:provider) { 'ldap' }
+      it "creates a user from LDAP" do
+        oauth_user.save
 
-      user.should be_valid
-      user.extern_uid.should == auth.uid
-      user.provider.should == 'twitter'
+        expect(gl_user).to be_valid
+        expect(gl_user.extern_uid).to eql uid
+        expect(gl_user.provider).to eql 'ldap'
+      end
     end
 
-    it "should apply defaults to user" do
-      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
+    context "twitter" do
+      let(:provider) { 'twitter' }
 
-    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')
+      it "creates a user from Omniauth" do
+        oauth_user.save
 
-      user = gl_auth.create(auth)
-      expect(user.username).to eql 'john'
+        expect(gl_user).to be_valid
+        expect(gl_user.extern_uid).to eql uid
+        expect(gl_user.provider).to eql 'twitter'
+      end
     end
   end
 end