Merge remote-tracking branch 'origin/master' into feature-multiple-ldap-servers

Conflicts:
	app/controllers/omniauth_callbacks_controller.rb

app/controllers/admin/users_controller.rb

@@ -4,6 +4,7 @@ class Admin::UsersController < Admin::ApplicationController
   def index
     @users = User.filter(params[:filter])
     @users = @users.search(params[:name]) if params[:name].present?
+    @users = @users.sort(@sort = params[:sort])
     @users = @users.alphabetically.page(params[:page])
   end
 

app/finders/snippets_finder.rb

@@ -30,7 +30,7 @@ class SnippetsFinder
     snippets = user.snippets.fresh.non_expired
 
     if user == current_user
-      snippets = case scope
+      case scope
       when 'are_internal' then
         snippets.are_internal
       when 'are_private' then
@@ -41,7 +41,7 @@ class SnippetsFinder
        snippets
       end
     else
-      snippets = snippets.public_and_internal
+      snippets.public_and_internal
     end
   end
 

app/models/user.rb

@@ -195,6 +195,16 @@ class User < ActiveRecord::Base
       end
     end
 
+    def sort(method)
+      case method.to_s
+      when 'recent_sign_in' then reorder('users.last_sign_in_at DESC')
+      when 'oldest_sign_in' then reorder('users.last_sign_in_at ASC')
+      when 'recently_created' then reorder('users.created_at DESC')
+      when 'late_created' then reorder('users.created_at ASC')
+      else reorder("users.name ASC")
+      end
+    end
+
     def find_for_commit(email, name)
       # Prefer email match over name match
       User.where(email: email).first ||

app/views/admin/users/index.html.haml

@@ -32,6 +32,26 @@
       .panel-heading
         Users (#{@users.total_count})
         .panel-head-actions
+          .dropdown.inline
+            %a.dropdown-toggle.btn{href: '#', "data-toggle" => "dropdown"}
+              %span.light sort:
+              - if @sort.present?
+                = @sort.humanize
+              - else
+                Name
+              %b.caret
+            %ul.dropdown-menu
+              %li
+                = link_to admin_users_path(sort: nil) do
+                  Name
+                = link_to admin_users_path(sort: 'recent_sign_in') do
+                  Recent sign in
+                = link_to admin_users_path(sort: 'oldest_sign_in') do
+                  Oldest sign in
+                = link_to admin_users_path(sort: 'recently_created') do
+                  Recently created
+                = link_to admin_users_path(sort: 'late_created') do
+                  Late created
           = link_to 'New User', new_admin_user_path, class: "btn btn-new"
       %ul.well-list
         - @users.each do |user|

doc/update/7.3-to-7.4.md

@@ -10,7 +10,7 @@ This document currently just serves as a place to keep track of updates that wil
 
 ## Optional optimizations for GitLab setups with MySQL databases
 
-Only applies if running MySQL database created with GitLab 6.7 or earlier. If you are not experiencing any issues you may not need the following instructions however following them will bring your database in line with the latest recommended installation configuration and help avoid future issues. Be sure to follow these directions exactly. These directions should be safe for any MySQL instance but to be sure take a current MySQL database backup beforehand.
+Only applies if running MySQL database created with GitLab 6.7 or earlier. If you are not experiencing any issues you may not need the following instructions however following them will bring your database in line with the latest recommended installation configuration and help avoid future issues. Be sure to follow these directions exactly. These directions should be safe for any MySQL instance but to be sure make a current MySQL database backup beforehand.
 
 ```
 # Secure your MySQL installation (added in GitLab 6.2)
@@ -61,7 +61,7 @@ mysql> \q
 # Update database configuration details
 # See config/database.yml.mysql for latest recommended configuration details
 #   Remove the reaping_frequency setting line if it exists (removed in GitLab 6.8)
-#   Set production -> pool: 10 (updated in GitLab 5.3 & 6.2)
+#   Set production -> pool: 10 (updated in GitLab 5.3)
 #   Set production -> username: git
 #   Set production -> password: the password your replaced $password with earlier
 sudo -u git -H editor /home/git/gitlab/config/database.yml

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
+      def initialize(auth_hash)
+        super
+        update_user_attributes
       end
 
-        def ldap_conf
-          Gitlab.config.ldap
+      # 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)
+      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
-          filter
+
+      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)
+      def initialize(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
+      def persisted?
+        gl_user.persisted?
       end
 
-        def model
-          ::User
+      def new?
+        !gl_user.persisted?
       end
 
-        def auth_hash=(auth_hash)
-          @auth_hash = AuthHash.new(auth_hash)
+      def valid?
+        gl_user.valid?
       end
 
-        protected
-        def find_by_uid_and_provider
-          model.where(provider: auth_hash.provider, extern_uid: auth_hash.uid).last
-        end
-      end
+      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?
 
-      # Instance methods
-      attr_accessor :auth_hash, :user
+        gl_user
+      rescue ActiveRecord::RecordInvalid => e
+        log.info "(OAuth) Error saving user: #{gl_user.errors.full_messages}"
+        return self, e.record.errors
+      end
 
-      def initialize(auth_hash)
-        self.auth_hash = auth_hash
-        self.user = self.class.model.new(user_attributes)
-        user.skip_confirmation!
+      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
-  before { Gitlab.config.stub(omniauth: {}) }
-
-  describe :find_or_create do
-    let(:auth) do
-      double(info: info, provider: 'ldap', uid: 'my-uid')
+  let(:auth_hash) do
+    double(uid: 'my-uid', provider: 'ldap', info: double(info))
   end
 
+  describe :find_or_create do
     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)
+  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 == 'ldap'
+        expect(gl_user).to be_valid
+        expect(gl_user.extern_uid).to eql uid
+        expect(gl_user.provider).to eql 'ldap'
       end
-
-    it "should create user from Omniauth" do
-      auth = double(info: info, uid: 'my-uid', provider: 'twitter')
-      user = gl_auth.create(auth)
-
-      user.should be_valid
-      user.extern_uid.should == auth.uid
-      user.provider.should == 'twitter'
     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
+    context "twitter" do
+      let(:provider) { 'twitter' }
 
-    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')
+      it "creates a user from Omniauth" do
+        oauth_user.save
 
-      user = gl_auth.create(auth)
-      expect(user.email).to_not be_empty
+        expect(gl_user).to be_valid
+        expect(gl_user.extern_uid).to eql uid
+        expect(gl_user.provider).to eql 'twitter'
       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

spec/models/user_spec.rb

@@ -429,4 +429,32 @@ describe User do
       expect(user.starred?(project)).to be_false
     end
   end
+
+  describe "#sort" do
+    before do
+      User.delete_all
+      @user = create :user, created_at: Date.today, last_sign_in_at: Date.today, name: 'Alpha'
+      @user1 = create :user, created_at: Date.today - 1, last_sign_in_at: Date.today - 1, name: 'Omega'
+    end
+    
+    it "sorts users as recently_signed_in" do
+      User.sort('recent_sign_in').first.should == @user
+    end
+
+    it "sorts users as late_signed_in" do
+      User.sort('oldest_sign_in').first.should == @user1
+    end
+
+    it "sorts users as recently_created" do
+      User.sort('recently_created').first.should == @user
+    end
+
+    it "sorts users as late_created" do
+      User.sort('late_created').first.should == @user1
+    end
+
+    it "sorts users by name when nil is passed" do
+      User.sort(nil).first.should == @user
+    end
+  end
 end