Merge branch 'ldap/cache_check' into 'master'

Cache LDAP check everywhere

Tested as follows:

- sign in LDAP user;
- disable access via LDAP (in this case, disable the account in AD);
- check that the user can still access Git via SSH;
- check that the user can still browse and do API calls (issue dropdown);
- set the last_credential_check_at attribute to 2 hours ago in the console;
- SSH access fails;
- API call returns 401.

If I try to sign in again after the LDAP account was disabled, the effect is immediate.

See merge request !139

app/controllers/application_controller.rb

@@ -201,16 +201,10 @@ class ApplicationController < ActionController::Base
 
   def ldap_security_check
     if current_user && current_user.requires_ldap_check?
-      gitlab_ldap_access do |access|
-        if access.allowed?(current_user)
-          access.update_permissions(current_user)
-          current_user.last_credential_check_at = Time.now
-          current_user.save
-        else
-          sign_out current_user
-          flash[:alert] = "Access denied for your LDAP account."
-          redirect_to new_user_session_path
-        end
+      unless Gitlab::LDAP::Access.allowed?(current_user)
+        sign_out current_user
+        flash[:alert] = "Access denied for your LDAP account."
+        redirect_to new_user_session_path
       end
     end
   end

app/controllers/omniauth_callbacks_controller.rb

@@ -21,15 +21,12 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
     @user = Gitlab::LDAP::User.find_or_create(oauth)
     @user.remember_me = true if @user.persisted?
 
-    gitlab_ldap_access do |access|
-      if access.allowed?(@user)
-        access.update_permissions(@user)
-        access.update_email(@user)
-        sign_in_and_redirect(@user)
-      else
-        flash[:alert] = "Access denied for your LDAP account."
-        redirect_to new_user_session_path
-      end
+    # Do additional LDAP checks for the user filter and EE features
+    if Gitlab::LDAP::Access.allowed?(@user)
+      sign_in_and_redirect(@user)
+    else
+      flash[:alert] = "Access denied for your LDAP account."
+      redirect_to new_user_session_path
     end
   end
 

app/models/user.rb

@@ -405,7 +405,9 @@ class User < ActiveRecord::Base
   end
 
   def requires_ldap_check?
-    if ldap_user?
+    if !Gitlab.config.ldap.enabled
+      false
+    elsif ldap_user?
       !last_credential_check_at || (last_credential_check_at + 1.hour) < Time.now
     else
       false

lib/gitlab/ldap/access.rb

@@ -9,6 +9,20 @@ module Gitlab
         end
       end
 
+      def self.allowed?(user)
+        self.open do |access|
+          if access.allowed?(user)
+            access.update_permissions(user)
+            access.update_email(user)
+            user.last_credential_check_at = Time.now
+            user.save
+            true
+          else
+            false
+          end
+        end
+      end
+
       def initialize(adapter=nil)
         @adapter = adapter
       end

lib/gitlab/user_access.rb

@@ -3,13 +3,8 @@ module Gitlab
     def self.allowed?(user)
       return false if user.blocked?
 
-      if Gitlab.config.ldap.enabled
-        if user.ldap_user?
-          # Check if LDAP user exists and match LDAP user_filter
-          Gitlab::LDAP::Access.open do |adapter|
-            return false unless adapter.allowed?(user)
-          end
-        end
+      if user.requires_ldap_check?
+        return false unless Gitlab::LDAP::Access.allowed?(user)
       end
 
       true

spec/models/user_spec.rb

@@ -312,6 +312,40 @@ describe User do
     end
   end
 
+  describe :requires_ldap_check? do
+    let(:user) { User.new }
+
+    it 'is false when LDAP is disabled' do
+      # Create a condition which would otherwise cause 'true' to be returned
+      user.stub(ldap_user?: true)
+      user.last_credential_check_at = nil
+      expect(user.requires_ldap_check?).to be_false
+    end
+
+    context 'when LDAP is enabled' do
+      before { Gitlab.config.ldap.stub(enabled: true) }
+
+      it 'is false for non-LDAP users' do
+        user.stub(ldap_user?: false)
+        expect(user.requires_ldap_check?).to be_false
+      end
+
+      context 'and when the user is an LDAP user' do
+        before { user.stub(ldap_user?: true) }
+
+        it 'is true when the user has never had an LDAP check before' do
+          user.last_credential_check_at = nil
+          expect(user.requires_ldap_check?).to be_true
+        end
+
+        it 'is true when the last LDAP check happened over 1 hour ago' do
+          user.last_credential_check_at = 2.hours.ago
+          expect(user.requires_ldap_check?).to be_true
+        end
+      end
+    end
+  end
+
   describe '#full_website_url' do
     let(:user) { create(:user) }