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

Feature oauth refactoring

Only use class methods when needed. In addition, make a proper extension of Gitlab::Oauth::User.

See merge request !1062

app/models/user.rb

@@ -474,10 +474,6 @@ class User < ActiveRecord::Base
     email =~ /\Atemp-email-for-oauth/
   end
 
-  def generate_tmp_oauth_email
-    self.email = "temp-email-for-oauth-#{username}@gitlab.localhost"
-  end
-
   def public_profile?
     authorized_projects.public_only.any?
   end

lib/gitlab/ldap/user.rb

@@ -10,37 +10,20 @@ module Gitlab
   module LDAP
     class User < Gitlab::OAuth::User
       class << self
-        def find_or_create(auth)
-          @auth = auth
-
-          if uid.blank? || email.blank? || username.blank?
-            raise_error("Account must provide a dn, uid and email address")
-          end
-
-          user = find(auth)
+        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
 
-          unless user
-            # Look for user with same emails
-            #
-            # Possible cases:
-            # * When user already has account and need to link their LDAP account.
-            # * LDAP uid changed for user with same email and we need to update their uid
-            #
-            user = model.find_by(email: email)
+        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: uid, provider: provider)
-              log.info("(LDAP) Updating legacy LDAP user #{email} with extern_uid => #{uid}")
-            else
-              # Create a new user inside GitLab database
-              # based on LDAP credentials
-              #
-              #
-              user = create(auth)
-            end
+          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
-
-          user
         end
 
         def authenticate(login, password)
@@ -48,17 +31,8 @@ module Gitlab
           # Only check with valid login and password to prevent anonymous bind results
           return nil unless ldap_conf.enabled && login.present? && password.present?
 
-          ldap = OmniAuth::LDAP::Adaptor.new(ldap_conf)
-          filter = Net::LDAP::Filter.eq(ldap.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
-
-          ldap_user = ldap.bind_as(
-            filter: filter,
+          ldap_user = adapter.bind_as(
+            filter: user_filter(login),
             size: 1,
             password: password
           )
@@ -66,10 +40,14 @@ module Gitlab
           find_by_uid(ldap_user.dn) if ldap_user
         end
 
-        private
+        def adapter
+          @adapter ||= OmniAuth::LDAP::Adaptor.new(ldap_conf)
+        end
+
+        protected
 
         def find_by_uid_and_provider
-          find_by_uid(uid)
+          find_by_uid(auth_hash.uid)
         end
 
         def find_by_uid(uid)
@@ -88,6 +66,20 @@ module Gitlab
         def ldap_conf
           Gitlab.config.ldap
         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
+      end
+
+      def needs_blocking?
+        false
       end
     end
   end

lib/gitlab/oauth/auth_hash.rb

+# Class to parse and transform the info provided by omniauth
+#
+module Gitlab
+  module OAuth
+    class AuthHash
+      attr_reader :auth_hash
+      def initialize(auth_hash)
+        @auth_hash = auth_hash
+      end
+
+      def uid
+        auth_hash.uid.to_s
+      end
+
+      def provider
+        auth_hash.provider
+      end
+
+      def info
+        auth_hash.info
+      end
+
+      def name
+        (info.name || full_name).to_s.force_encoding('utf-8')
+      end
+
+      def full_name
+        "#{info.first_name} #{info.last_name}"
+      end
+
+      def username
+        (info.try(:nickname) || generate_username).to_s.force_encoding('utf-8')
+      end
+
+      def email
+        (info.try(:email) || generate_temporarily_email).downcase
+      end
+
+      def password
+        @password ||= Devise.friendly_token[0, 8].downcase
+      end
+
+      # Get the first part of the email address (before @)
+      # In addtion in removes illegal characters
+      def generate_username
+        email.match(/^[^@]*/)[0].parameterize
+      end
+
+      def generate_temporarily_email
+        "temp-email-for-oauth-#{username}@gitlab.localhost"
+      end
+    end
+  end
+end

lib/gitlab/oauth/user.rb

@@ -7,106 +7,78 @@ module Gitlab
   module OAuth
     class User
       class << self
-        attr_reader :auth
+        attr_reader :auth_hash
 
-        def find(auth)
-          @auth = auth
+        def find(auth_hash)
+          self.auth_hash = auth_hash
           find_by_uid_and_provider
         end
 
-        def create(auth)
-          @auth = auth
-          password = Devise.friendly_token[0, 8].downcase
-          opts = {
-            extern_uid: uid,
-            provider: provider,
-            name: name,
-            username: username,
-            email: email,
-            password: password,
-            password_confirmation: password,
-          }
-
-          user = model.build_user(opts)
-          user.skip_confirmation!
-
-          # Services like twitter and github does not return email via oauth
-          # In this case we generate temporary email and force user to fill it later
-          if user.email.blank?
-            user.generate_tmp_oauth_email
-          elsif provider != "ldap"
-            # Google oauth returns email but dont return nickname
-            # So we use part of email as username for new user
-            # For LDAP, username is already set to the user's
-            # uid/userid/sAMAccountName.
-            email_username = email.match(/^[^@]*/)[0]
-            # Strip apostrophes since they are disallowed as part of username
-            user.username = email_username.gsub("'", "")
-          end
-
-          begin
-            user.save!
-          rescue ActiveRecord::RecordInvalid => e
-            log.info "(OAuth) Email #{e.record.errors[:email]}. Username #{e.record.errors[:username]}"
-            return nil, e.record.errors
-          end
-
-          log.info "(OAuth) Creating user #{email} from login with extern_uid => #{uid}"
-
-          if Gitlab.config.omniauth['block_auto_created_users'] && !ldap?
-            user.block
-          end
+        def create(auth_hash)
+          user = new(auth_hash)
+          user.save_and_trigger_callbacks
+        end
 
-          user
+        def model
+          ::User
         end
 
-        private
+        def auth_hash=(auth_hash)
+          @auth_hash = AuthHash.new(auth_hash)
+        end
 
+        protected
         def find_by_uid_and_provider
-          model.where(provider: provider, extern_uid: uid).last
+          model.where(provider: auth_hash.provider, extern_uid: auth_hash.uid).last
         end
+      end
 
-        def uid
-          auth.uid.to_s
-        end
+      # Instance methods
+      attr_accessor :auth_hash, :user
 
-        def email
-          return unless auth.info.respond_to?(:email)
-          auth.info.email.downcase unless auth.info.email.nil?
-        end
+      def initialize(auth_hash)
+        self.auth_hash = auth_hash
+        self.user = self.class.model.new(user_attributes)
+        user.skip_confirmation!
+      end
 
-        def name
-          if auth.info.name.nil?
-            "#{auth.info.first_name} #{auth.info.last_name}".force_encoding('utf-8')
-          else
-            auth.info.name.to_s.force_encoding('utf-8')
-          end
-        end
+      def auth_hash=(auth_hash)
+        @auth_hash = AuthHash.new(auth_hash)
+      end
 
-        def username
-          return unless auth.info.respond_to?(:nickname)
-          auth.info.nickname.to_s.force_encoding("utf-8")
-        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 provider
-          auth.provider
-        end
+        user
+      rescue ActiveRecord::RecordInvalid => e
+        log.info "(OAuth) Email #{e.record.errors[:email]}. Username #{e.record.errors[:username]}"
+        return nil, e.record.errors
+      end
 
-        def log
-          Gitlab::AppLogger
-        end
+      def user_attributes
+        {
+          extern_uid: auth_hash.uid,
+          provider: auth_hash.provider,
+          name: auth_hash.name,
+          username: auth_hash.username,
+          email: auth_hash.email,
+          password: auth_hash.password,
+          password_confirmation: auth_hash.password,
+        }
+      end
 
-        def model
-          ::User
-        end
+      def log
+        Gitlab::AppLogger
+      end
 
-        def raise_error(message)
-          raise OmniAuth::Error, "(OAuth) " + message
-        end
+      def raise_error(message)
+        raise OmniAuth::Error, "(OAuth) " + message
+      end
 
-        def ldap?
-          provider == 'ldap'
-        end
+      def needs_blocking?
+        Gitlab.config.omniauth['block_auto_created_users']
       end
     end
   end

spec/lib/gitlab/auth_spec.rb

@@ -4,25 +4,44 @@ describe Gitlab::Auth do
   let(:gl_auth) { Gitlab::Auth.new }
 
   describe :find do
-    before do
-      @user = create(
-        :user,
-        username: 'john',
-        password: '88877711',
-        password_confirmation: '88877711'
-      )
+    let!(:user) do
+      create(:user,
+        username: username,
+        password: password,
+        password_confirmation: password)
     end
+    let(:username) { 'john' }
+    let(:password) { 'my-secret' }
 
     it "should find user by valid login/password" do
-      gl_auth.find('john', '88877711').should == @user
+      expect( gl_auth.find(username, password) ).to eql user
     end
 
     it "should not find user with invalid password" do
-      gl_auth.find('john', 'invalid11').should_not == @user
+      password = 'wrong'
+      expect( gl_auth.find(username, password) ).to_not eql user
     end
 
-    it "should not find user with invalid login and password" do
-      gl_auth.find('jon', 'invalid11').should_not == @user
+    it "should not find user with invalid login" do
+      user = 'wrong'
+      expect( gl_auth.find(username, password) ).to_not eql user
+    end
+
+    context "with ldap enabled" do
+      before { Gitlab.config.ldap['enabled'] = true }
+      after  { Gitlab.config.ldap['enabled'] = false }
+
+      it "tries to autheticate with db before ldap" do
+        expect(Gitlab::LDAP::User).not_to receive(:authenticate)
+
+        gl_auth.find(username, password)
+      end
+
+      it "uses ldap as fallback to for authentication" do
+        expect(Gitlab::LDAP::User).to receive(:authenticate)
+
+        gl_auth.find('ldap_user', 'password')
+      end
     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(:gl_user) { Gitlab::LDAP::User }
   let(:info) do
     double(
       name: 'John',
@@ -19,12 +19,12 @@ describe Gitlab::LDAP::User do
     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 }
+      expect{ gl_user.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 }
+      expect{ gl_user.find_or_create(auth) }.to_not change{ User.count }
 
       existing_user.reload
       expect(existing_user.extern_uid).to eql 'my-uid'
@@ -32,7 +32,23 @@ describe Gitlab::LDAP::User do
     end
 
     it "creates a new user if not found" do
-      expect{ gl_auth.find_or_create(auth) }.to change{ User.count }.by(1)
+      expect{ gl_user.find_or_create(auth) }.to change{ User.count }.by(1)
+    end
+  end
+
+  describe "authenticate" do
+    let(:login) { 'john' }
+    let(:password) { 'my-secret' }
+
+    before {
+      Gitlab.config.ldap['enabled'] = true
+      Gitlab.config.ldap['user_filter'] = 'employeeType=developer'
+    }
+    after  { Gitlab.config.ldap['enabled'] = false }
+
+    it "send an authentication request to ldap" do
+      expect( Gitlab::LDAP::User.adapter ).to receive(:bind_as)
+      Gitlab::LDAP::User.authenticate(login, password)
     end
   end
 end