From 669682686ea32a787aa9ef950388f780cfc00146 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 30 Jul 2014 09:50:50 +0200 Subject: Move LDAP timeout code to Gitlab::LDAP::Access --- app/controllers/application_controller.rb | 13 ++++--------- app/controllers/omniauth_callbacks_controller.rb | 13 ++++++------- lib/gitlab/ldap/access.rb | 13 +++++++++++++ 3 files changed, 23 insertions(+), 16 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index d0546a441e1..5ffec7f75bf 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -201,15 +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) - 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 diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index ef2afec52dc..3ed6a69c2d8 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -21,13 +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) - 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 diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index 4e48ff11871..62709a12942 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -9,6 +9,19 @@ module Gitlab end end + def self.allowed?(user) + self.open do |access| + if access.allowed?(user) + # GitLab EE LDAP code goes here + user.last_credential_check_at = Time.now + user.save + true + else + false + end + end + end + def initialize(adapter=nil) @adapter = adapter end -- cgit v1.2.1 From 82dc40936a233edee59b4be45e0458883446ca9e Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 30 Jul 2014 09:51:12 +0200 Subject: Cache LDAP check in Gitlab::UserAccess This changes the number of LDAP calls when users access GitLab via Git-over-SSH or the API. LDAP check results are cached for 1 hour. --- lib/gitlab/user_access.rb | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index 16df21b49ba..4885baf9526 100644 --- a/lib/gitlab/user_access.rb +++ b/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 -- cgit v1.2.1 From 1118a6fd342f2b5cfe438b63a829368782ff1919 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 6 Aug 2014 15:16:45 +0200 Subject: Add a spec for User#requires_ldap_check? --- spec/models/user_spec.rb | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index a36b57a95de..c4bd80cc96a 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -312,6 +312,29 @@ describe User do end end + describe :requires_ldap_check? do + let(:user) { User.new } + + it 'is false for non-LDAP users' do + user.stub(ldap_user?: false) + expect(user.requires_ldap_check?).to be_false + end + + context '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 + describe '#full_website_url' do let(:user) { create(:user) } -- cgit v1.2.1 From e0fea696c6e4eb007c945d63faca594a70dd45e7 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 6 Aug 2014 15:17:12 +0200 Subject: Skip LDAP cache check when LDAP is disabled --- app/models/user.rb | 4 +++- spec/models/user_spec.rb | 31 +++++++++++++++++++++---------- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 7e3a7262afc..8f9f319aa9e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -403,7 +403,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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index c4bd80cc96a..e18e76cd69e 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -315,22 +315,33 @@ describe User do describe :requires_ldap_check? do let(:user) { User.new } - it 'is false for non-LDAP users' do - user.stub(ldap_user?: false) + 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 the user is an LDAP user' do - before { user.stub(ldap_user?: true) } + context 'when LDAP is enabled' do + before { Gitlab.config.ldap.stub(enabled: 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 + it 'is false for non-LDAP users' do + user.stub(ldap_user?: false) + expect(user.requires_ldap_check?).to be_false 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 + 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 -- cgit v1.2.1