From 9cdb69250aa6669ecc416cb978ce47f62d0807fb Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Fri, 29 Jan 2016 17:08:59 -0600 Subject: [PATCH 1/5] Do not create GL user when LDAP user falls outside user_filter --- .../omniauth_callbacks_controller.rb | 23 +++++++++++-------- lib/gitlab/ldap/user.rb | 8 +++++-- lib/gitlab/o_auth/user.rb | 3 ++- 3 files changed, 21 insertions(+), 13 deletions(-) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 4c13228fce9f..a752bc06ccfe 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -22,19 +22,22 @@ def failure_message # if the authentication to LDAP was successful. def ldap ldap_user = Gitlab::LDAP::User.new(oauth) - ldap_user.save if ldap_user.changed? # will also save new users - @user = ldap_user.gl_user - @user.remember_me = params[:remember_me] if ldap_user.persisted? + if ldap_user.exist? + ldap_user.save if ldap_user.changed? # will also save new users - # Do additional LDAP checks for the user filter and EE features - if ldap_user.allowed? - log_audit_event(@user, with: :ldap) - sign_in_and_redirect(@user) - else - flash[:alert] = "Access denied for your LDAP account." - redirect_to new_user_session_path + @user = ldap_user.gl_user + @user.remember_me = params[:remember_me] if ldap_user.persisted? + + # Do additional LDAP checks for the user filter and EE features + if ldap_user.allowed? + log_audit_event(@user, with: :ldap) + sign_in_and_redirect(@user) and return + end end + + flash[:alert] = "Access denied for your LDAP account." + redirect_to new_user_session_path end def omniauth_error diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index e044f0ecc6d8..cfcfef60ce88 100644 --- a/lib/gitlab/ldap/user.rb +++ b/lib/gitlab/ldap/user.rb @@ -19,9 +19,13 @@ def find_by_uid_and_provider(uid, provider) end end - def initialize(auth_hash) - super + def save update_user_attributes + super + end + + def exist? + ldap_person.present? end # instance methods diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb index d87a72f7ba34..0df520d898e8 100644 --- a/lib/gitlab/o_auth/user.rb +++ b/lib/gitlab/o_auth/user.rb @@ -90,7 +90,8 @@ def ldap_person # Look for a corresponding person with same uid in any of the configured LDAP providers Gitlab::LDAP::Config.providers.each do |provider| adapter = Gitlab::LDAP::Adapter.new(provider) - @ldap_person = Gitlab::LDAP::Person.find_by_uid(auth_hash.uid, adapter) + # `find_by_dn` because LDAP `auth_hash.uid` is a full DN + @ldap_person = Gitlab::LDAP::Person.find_by_dn(auth_hash.uid, adapter) break if @ldap_person end @ldap_person -- GitLab From a719e24f76daad711cc421b64d325f3715724c9e Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Thu, 4 Feb 2016 13:31:45 -0600 Subject: [PATCH 2/5] Fixes --- app/controllers/omniauth_callbacks_controller.rb | 8 +++++--- lib/gitlab/ldap/user.rb | 3 ++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index a752bc06ccfe..c822b98ce114 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -18,18 +18,20 @@ def failure_message error.to_s.humanize if error end - # We only find ourselves here - # if the authentication to LDAP was successful. + # We only find ourselves here if the + # authentication to LDAP was successful. def ldap ldap_user = Gitlab::LDAP::User.new(oauth) + # By this time the user obviously exists in the LDAP server, but + # this check ensures the user is within any specified `user_filter`. if ldap_user.exist? ldap_user.save if ldap_user.changed? # will also save new users @user = ldap_user.gl_user @user.remember_me = params[:remember_me] if ldap_user.persisted? - # Do additional LDAP checks for the user filter and EE features + # Do additional LDAP checks and permission updates if ldap_user.allowed? log_audit_event(@user, with: :ldap) sign_in_and_redirect(@user) and return diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index cfcfef60ce88..71d266dabe15 100644 --- a/lib/gitlab/ldap/user.rb +++ b/lib/gitlab/ldap/user.rb @@ -25,7 +25,8 @@ def save end def exist? - ldap_person.present? + # TODO: Optimize this because we want to minimize number of ldap searches + Gitlab::LDAP::User.find_by_uid_and_provider(ldap_person.dn, ldap_person.provider) end # instance methods -- GitLab From 2b2ad815c2ad5aca98d130bfc4a274a327308cf7 Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Thu, 4 Feb 2016 15:36:32 -0600 Subject: [PATCH 3/5] Fix confusion --- lib/gitlab/o_auth/user.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb index 0df520d898e8..28b1e0c2a018 100644 --- a/lib/gitlab/o_auth/user.rb +++ b/lib/gitlab/o_auth/user.rb @@ -7,6 +7,10 @@ module Gitlab module OAuth class SignupDisabledError < StandardError; end + # NOTE: The LDAP related methods in this class are exclusively for linking + # an LDAP user to an OmniAuth user to pull more information about the user + # Do not confuse the methods here as having anything to do with LDAP auth + # directly. All of those methods are in the `Gitlab::LDAP::User` class. class User attr_accessor :auth_hash, :gl_user @@ -90,8 +94,7 @@ def ldap_person # Look for a corresponding person with same uid in any of the configured LDAP providers Gitlab::LDAP::Config.providers.each do |provider| adapter = Gitlab::LDAP::Adapter.new(provider) - # `find_by_dn` because LDAP `auth_hash.uid` is a full DN - @ldap_person = Gitlab::LDAP::Person.find_by_dn(auth_hash.uid, adapter) + @ldap_person = Gitlab::LDAP::Person.find_by_uid(auth_hash.uid, adapter) break if @ldap_person end @ldap_person -- GitLab From 0c0815af9230830c6cab739aabc58ffbc2b35bbd Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Thu, 4 Feb 2016 15:40:29 -0600 Subject: [PATCH 4/5] Fix confusion --- lib/gitlab/ldap/user.rb | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index 71d266dabe15..17fcf16fcc24 100644 --- a/lib/gitlab/ldap/user.rb +++ b/lib/gitlab/ldap/user.rb @@ -25,8 +25,15 @@ def save end def exist? - # TODO: Optimize this because we want to minimize number of ldap searches - Gitlab::LDAP::User.find_by_uid_and_provider(ldap_person.dn, ldap_person.provider) + ldap_user.present? + end + + def adapter + @adapter ||= Gitlab::LDAP::Adapter.new(auth_hash.provider) + end + + def ldap_user + @ldap_user ||= Gitlab::LDAP::Person.find_by_dn(auth_hash.uid, adapter) end # instance methods -- GitLab From 65ec99b85cd477219921865769c0a3f24c46a104 Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Thu, 4 Feb 2016 15:43:06 -0600 Subject: [PATCH 5/5] Fix confusion --- lib/gitlab/ldap/user.rb | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index 17fcf16fcc24..7b1196cd4607 100644 --- a/lib/gitlab/ldap/user.rb +++ b/lib/gitlab/ldap/user.rb @@ -36,7 +36,14 @@ def ldap_user @ldap_user ||= Gitlab::LDAP::Person.find_by_dn(auth_hash.uid, adapter) end - # instance methods + # Flow to find GitLab user. If any step proves fruitful, all of + # the next steps are skipped. + # + # 1. Find user with identity with matching uid (dn) and provider + # 2. Look for GL user by email. Existing identities not considered. This + # allows us to match up the user even if their dn has changed. + # 3. Build a new user with an LDAP identity + # def gl_user @gl_user ||= find_by_uid_and_provider || find_by_email || build_new_user end -- GitLab