diff options
| -rw-r--r-- | app/controllers/application_controller.rb | 13 | ||||
| -rw-r--r-- | app/controllers/omniauth_callbacks_controller.rb | 13 | ||||
| -rw-r--r-- | app/models/user.rb | 4 | ||||
| -rw-r--r-- | lib/gitlab/ldap/access.rb | 13 | ||||
| -rw-r--r-- | lib/gitlab/user_access.rb | 9 | ||||
| -rw-r--r-- | spec/models/user_spec.rb | 34 |
6 files changed, 62 insertions, 24 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/app/models/user.rb b/app/models/user.rb index 9ab3ea025c3..f1ff76edd15 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -414,7 +414,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/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 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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index ef6b8a94502..7221328a45f 100644 --- a/spec/models/user_spec.rb +++ b/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) } |