From c96a1e6931562b6d4a70923acdd57437e61a98b2 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Thu, 11 Sep 2014 12:00:46 +0200 Subject: [PATCH 01/51] Add backwards compatibility for old ldap server definition --- config/initializers/1_settings.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index cacf2a3dc8..a450f2729a 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -59,6 +59,14 @@ Settings.ldap['enabled'] = false if Settings.ldap['enabled'].nil? Settings.ldap['allow_username_or_email_login'] = false if Settings.ldap['allow_username_or_email_login'].nil? Settings.ldap['sync_time'] = 3600 if Settings.ldap['sync_time'].nil? +# backwards compatibility, we only have one host +if Settings.ldap['enabled'] && Settings.ldap['host'].present? + per_server_keys = %w(host port uid method base user_filter group_base admin_group) + server = Settings.ldap.slice(per_server_keys) + server['primary'] = true + server['name'] = 'LDAP' + Settings.ldap['servers'] = [server] +end Settings['omniauth'] ||= Settingslogic.new({}) Settings.omniauth['enabled'] = false if Settings.omniauth['enabled'].nil? From cd1d5b202664c846aace240554ae1bda4b4da616 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Thu, 11 Sep 2014 15:42:08 +0200 Subject: [PATCH 02/51] Adapt LDAP settings by excluding non-host-specific settings --- config/initializers/1_settings.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index a450f2729a..398eef7fd7 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -61,8 +61,8 @@ Settings.ldap['sync_time'] = 3600 if Settings.ldap['sync_time'].nil? # backwards compatibility, we only have one host if Settings.ldap['enabled'] && Settings.ldap['host'].present? - per_server_keys = %w(host port uid method base user_filter group_base admin_group) - server = Settings.ldap.slice(per_server_keys) + excluded_per_server_settings = %w(sync_time allow_username_or_email_login) + server = Settings.ldap.except(excluded_per_server_settings) server['primary'] = true server['name'] = 'LDAP' Settings.ldap['servers'] = [server] From 8772c2a1133de4b52938894de10dd3f634a5826d Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Thu, 11 Sep 2014 15:43:28 +0200 Subject: [PATCH 03/51] Add support for multiple ldap omniauth strategies --- config/initializers/7_omniauth.rb | 8 ++++++++ config/initializers/devise.rb | 20 +++++++++++--------- 2 files changed, 19 insertions(+), 9 deletions(-) create mode 100644 config/initializers/7_omniauth.rb diff --git a/config/initializers/7_omniauth.rb b/config/initializers/7_omniauth.rb new file mode 100644 index 0000000000..6a71c0b565 --- /dev/null +++ b/config/initializers/7_omniauth.rb @@ -0,0 +1,8 @@ +module OmniAuth::Strategies + class Ldap0 < LDAP; end + class Ldap1 < LDAP; end + class Ldap2 < LDAP; end + class Ldap3 < LDAP; end + class Ldap4 < LDAP; end + class Ldap5 < LDAP; end +end \ No newline at end of file diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 34f4f38698..9e0ee7c548 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -211,15 +211,17 @@ Devise.setup do |config| email_stripping_proc = ->(name) {name} end - config.omniauth :ldap, - host: Gitlab.config.ldap['host'], - base: Gitlab.config.ldap['base'], - uid: Gitlab.config.ldap['uid'], - port: Gitlab.config.ldap['port'], - method: Gitlab.config.ldap['method'], - bind_dn: Gitlab.config.ldap['bind_dn'], - password: Gitlab.config.ldap['password'], - name_proc: email_stripping_proc + Gitlab.config.ldap.servers.each_with_index do |server, i| + config.omniauth :"ldap#{i}", + host: server['host'], + base: server['base'], + uid: server['uid'], + port: server['port'], + method: server['method'], + bind_dn: server['bind_dn'], + password: server['password'], + name_proc: email_stripping_proc + end end Gitlab.config.omniauth.providers.each do |provider| From e45861d7d9a54eac8e6b50621d465aedda3dfa0a Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Thu, 11 Sep 2014 16:14:54 +0200 Subject: [PATCH 04/51] Display multiple LDAP servers on login page --- app/controllers/omniauth_callbacks_controller.rb | 7 +++++++ app/controllers/sessions_controller.rb | 4 ++++ app/helpers/application_helper.rb | 4 ---- app/helpers/oauth_helper.rb | 2 +- app/views/devise/sessions/_new_ldap.html.haml | 2 +- app/views/devise/sessions/new.html.haml | 16 +++++++++------- config/initializers/7_omniauth.rb | 2 +- 7 files changed, 23 insertions(+), 14 deletions(-) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 3ed6a69c2d..e1788982ad 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -30,6 +30,13 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController end end + alias_method :ldap0, :ldap + alias_method :ldap1, :ldap + alias_method :ldap2, :ldap + alias_method :ldap3, :ldap + alias_method :ldap4, :ldap + alias_method :ldap5, :ldap + def omniauth_error @provider = params[:provider] @error = params[:error] diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 1bdba75c5e..e918f46bb3 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -18,6 +18,10 @@ class SessionsController < Devise::SessionsController store_location_for(:redirect, redirect_path) end + if Gitlab.config.ldap.enabled + @ldap_servers = Gitlab.config.ldap.servers + end + super end diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index fb8d318ec0..e6d50bea4d 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -244,10 +244,6 @@ module ApplicationHelper end end - def ldap_enabled? - Gitlab.config.ldap.enabled - end - def link_to(name = nil, options = nil, html_options = nil, &block) begin uri = URI(options) diff --git a/app/helpers/oauth_helper.rb b/app/helpers/oauth_helper.rb index c0177dacbf..7024483b8b 100644 --- a/app/helpers/oauth_helper.rb +++ b/app/helpers/oauth_helper.rb @@ -1,6 +1,6 @@ module OauthHelper def ldap_enabled? - Devise.omniauth_providers.include?(:ldap) + Gitlab.config.ldap.enabled end def default_providers diff --git a/app/views/devise/sessions/_new_ldap.html.haml b/app/views/devise/sessions/_new_ldap.html.haml index 6c5a878e90..0158461149 100644 --- a/app/views/devise/sessions/_new_ldap.html.haml +++ b/app/views/devise/sessions/_new_ldap.html.haml @@ -1,4 +1,4 @@ -= form_tag(user_omniauth_callback_path(:ldap), id: 'new_ldap_user' ) do += form_tag(user_omniauth_callback_path(provider), id: 'new_ldap_user' ) do = text_field_tag :username, nil, {class: "form-control top", placeholder: "LDAP Login", autofocus: "autofocus"} = password_field_tag :password, nil, {class: "form-control bottom", placeholder: "Password"} %br/ diff --git a/app/views/devise/sessions/new.html.haml b/app/views/devise/sessions/new.html.haml index f53d6f09da..8db732d6c4 100644 --- a/app/views/devise/sessions/new.html.haml +++ b/app/views/devise/sessions/new.html.haml @@ -4,20 +4,22 @@ .panel-body - if ldap_enabled? && gitlab_config.signin_enabled %ul.nav.nav-tabs - %li.active - = link_to 'LDAP', '#tab-ldap', 'data-toggle' => 'tab' + - @ldap_servers.each_with_index do |server, i| + %li{class: (:active if server['primary'])} + = link_to server['name'], "#tab-ldap#{i}", 'data-toggle' => 'tab' %li = link_to 'Standard', '#tab-signin', 'data-toggle' => 'tab' .tab-content - %div#tab-ldap.tab-pane.active - = render partial: 'devise/sessions/new_ldap' + - @ldap_servers.each_with_index do |server, i| + %div.tab-pane{id: "tab-ldap#{i}", class: (:active if server['primary'])} + = render 'devise/sessions/new_ldap', provider: "ldap#{i}" %div#tab-signin.tab-pane - = render partial: 'devise/sessions/new_base' + = render 'devise/sessions/new_base' - elsif ldap_enabled? - = render partial: 'devise/sessions/new_ldap' + = render 'devise/sessions/new_ldap', ldap_servers: @ldap_servers - elsif gitlab_config.signin_enabled - = render partial: 'devise/sessions/new_base' + = render 'devise/sessions/new_base' - else %div No authentication methods configured. diff --git a/config/initializers/7_omniauth.rb b/config/initializers/7_omniauth.rb index 6a71c0b565..b3c5260764 100644 --- a/config/initializers/7_omniauth.rb +++ b/config/initializers/7_omniauth.rb @@ -5,4 +5,4 @@ module OmniAuth::Strategies class Ldap3 < LDAP; end class Ldap4 < LDAP; end class Ldap5 < LDAP; end -end \ No newline at end of file +end From 9edf6f2f9b88f81c920bfa763acaf8e335767210 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Tue, 30 Sep 2014 16:29:32 +0200 Subject: [PATCH 05/51] Add tests for auth hash --- lib/gitlab/oauth/auth_hash.rb | 2 +- spec/lib/gitlab/oauth/auth_hash_spec.rb | 55 +++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 spec/lib/gitlab/oauth/auth_hash_spec.rb diff --git a/lib/gitlab/oauth/auth_hash.rb b/lib/gitlab/oauth/auth_hash.rb index 0198f61f42..ce52beec78 100644 --- a/lib/gitlab/oauth/auth_hash.rb +++ b/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 diff --git a/spec/lib/gitlab/oauth/auth_hash_spec.rb b/spec/lib/gitlab/oauth/auth_hash_spec.rb new file mode 100644 index 0000000000..68b87dbd4b --- /dev/null +++ b/spec/lib/gitlab/oauth/auth_hash_spec.rb @@ -0,0 +1,55 @@ +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_not be_empty + 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 From 16013bebfea51af378beb35616c9503c361a7d58 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Tue, 30 Sep 2014 18:08:05 +0200 Subject: [PATCH 06/51] Refactor: Make all Oauth::User methods instance methods --- lib/gitlab/oauth/user.rb | 74 +++++++++++++------------- spec/lib/gitlab/oauth/user_spec.rb | 84 +++++++++++------------------- 2 files changed, 67 insertions(+), 91 deletions(-) diff --git a/lib/gitlab/oauth/user.rb b/lib/gitlab/oauth/user.rb index b768eda185..6ad81c3d17 100644 --- a/lib/gitlab/oauth/user.rb +++ b/lib/gitlab/oauth/user.rb @@ -6,55 +6,51 @@ module Gitlab module OAuth class User - class << self - attr_reader :auth_hash - - def find(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 - end - - def model - ::User - end - - def auth_hash=(auth_hash) - @auth_hash = AuthHash.new(auth_hash) - end - - protected - def find_by_uid_and_provider - model.where(provider: auth_hash.provider, extern_uid: auth_hash.uid).last - end - end - - # Instance methods - attr_accessor :auth_hash, :user + attr_accessor :auth_hash, :gl_user def initialize(auth_hash) self.auth_hash = auth_hash - self.user = self.class.model.new(user_attributes) - user.skip_confirmation! end 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 persisted? + gl_user.persisted? + end - user + def new? + !gl_user.persisted? + end + + def valid? + gl_user.valid? + end + + def save + gl_user.save! + log.info "(OAuth) Creating user #{auth_hash.email} from login with extern_uid => #{auth_hash.uid}" + gl_user.block if needs_blocking? + + gl_user rescue ActiveRecord::RecordInvalid => e log.info "(OAuth) Email #{e.record.errors[:email]}. Username #{e.record.errors[:username]}" - return nil, e.record.errors + return self, e.record.errors + end + + def gl_user + @user ||= find_by_uid_and_provider || build_new_user + end + + def find_by_uid_and_provider + model.where(provider: auth_hash.provider, extern_uid: auth_hash.uid).last + end + + def build_new_user + model.new(user_attributes).tap do |user| + user.skip_confirmation! + end end def user_attributes @@ -80,6 +76,10 @@ module Gitlab def needs_blocking? Gitlab.config.omniauth['block_auto_created_users'] end + + def model + ::User + end end end end diff --git a/spec/lib/gitlab/oauth/user_spec.rb b/spec/lib/gitlab/oauth/user_spec.rb index c241e19860..e0341c47fc 100644 --- a/spec/lib/gitlab/oauth/user_spec.rb +++ b/spec/lib/gitlab/oauth/user_spec.rb @@ -1,83 +1,59 @@ 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 end - it "should create user from Omniauth" do - auth = double(info: info, uid: 'my-uid', provider: 'twitter') - user = gl_auth.create(auth) + context "twitter" do + let(:provider) { 'twitter' } - user.should be_valid - user.extern_uid.should == auth.uid - user.provider.should == 'twitter' - end + it "creates a user from Omniauth" do + oauth_user.save - 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 - - 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') - - user = gl_auth.create(auth) - expect(user.email).to_not be_empty - 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' + expect(gl_user).to be_valid + expect(gl_user.extern_uid).to eql uid + expect(gl_user.provider).to eql 'twitter' + end end end end From 69c17a767c36b3c78f7571e143d8f4ab6e1b70d6 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Tue, 30 Sep 2014 18:10:09 +0200 Subject: [PATCH 07/51] Protect non public methods --- lib/gitlab/oauth/user.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/gitlab/oauth/user.rb b/lib/gitlab/oauth/user.rb index 6ad81c3d17..3e23f3ad1a 100644 --- a/lib/gitlab/oauth/user.rb +++ b/lib/gitlab/oauth/user.rb @@ -12,10 +12,6 @@ module Gitlab self.auth_hash = auth_hash end - def auth_hash=(auth_hash) - @auth_hash = AuthHash.new(auth_hash) - end - def persisted? gl_user.persisted? end @@ -43,6 +39,11 @@ module Gitlab @user ||= find_by_uid_and_provider || build_new_user end + protected + def auth_hash=(auth_hash) + @auth_hash = AuthHash.new(auth_hash) + end + def find_by_uid_and_provider model.where(provider: auth_hash.provider, extern_uid: auth_hash.uid).last end From 37e68cf86b4548b5d67e7e8f044135b01beb3deb Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Tue, 30 Sep 2014 18:21:41 +0200 Subject: [PATCH 08/51] remove stub --- spec/lib/gitlab/oauth/user_spec.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/spec/lib/gitlab/oauth/user_spec.rb b/spec/lib/gitlab/oauth/user_spec.rb index e0341c47fc..e4e96fd9f4 100644 --- a/spec/lib/gitlab/oauth/user_spec.rb +++ b/spec/lib/gitlab/oauth/user_spec.rb @@ -14,10 +14,6 @@ describe Gitlab::OAuth::User do } end - before do - Gitlab.config.stub(omniauth: {}) - end - describe :persisted? do let!(:existing_user) { create(:user, extern_uid: 'my-uid', provider: 'my-provider') } From ad1c2f79d97d224ee60be9b52e5cdb7cd0bd1d83 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Wed, 1 Oct 2014 14:11:50 +0200 Subject: [PATCH 09/51] Refactor Gitlab::LDAP::User to instance methods --- .../omniauth_callbacks_controller.rb | 35 ++++----- lib/gitlab/ldap/user.rb | 75 ++++++++++--------- lib/gitlab/oauth/user.rb | 2 +- spec/lib/gitlab/ldap/user_spec.rb | 20 +++-- 4 files changed, 66 insertions(+), 66 deletions(-) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 3ed6a69c2d..fa5685938f 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -15,15 +15,17 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController error.to_s.humanize if error end + # We only find ourselves here + # if the authentication to LDAP was successful. def ldap - # We only find ourselves here - # if the authentication to LDAP was successful. - @user = Gitlab::LDAP::User.find_or_create(oauth) - @user.remember_me = true if @user.persisted? + @user = Gitlab::LDAP::User.new(oauth) + @user.save if @user.changed? # will also save new users + gl_user = @user.gl_user + gl_user.remember_me = true if @user.persisted? # Do additional LDAP checks for the user filter and EE features - if Gitlab::LDAP::Access.allowed?(@user) - sign_in_and_redirect(@user) + if Gitlab::LDAP::Access.allowed?(gl_user) + sign_in_and_redirect(gl_user) else flash[:alert] = "Access denied for your LDAP account." redirect_to new_user_session_path @@ -46,24 +48,17 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController current_user.save redirect_to profile_path else - @user = Gitlab::OAuth::User.find(oauth) + @user = Gitlab::OAuth::User.new(oauth) - # Create user if does not exist - # and allow_single_sign_on is true - if Gitlab.config.omniauth['allow_single_sign_on'] && !@user - @user, errors = Gitlab::OAuth::User.create(oauth) + if Gitlab.config.omniauth['allow_single_sign_on'] && @user.new? + @user.save end - if @user && !errors - sign_in_and_redirect(@user) + if @user.valid? + sign_in_and_redirect(@user.gl_user) else - if errors - error_message = errors.map{ |attribute, message| "#{attribute} #{message}" }.join(", ") - redirect_to omniauth_error_path(oauth['provider'], error: error_message) and return - else - flash[:notice] = "There's no such user!" - end - redirect_to new_user_session_path + error_message = @user.gl_user.errors.map{ |attribute, message| "#{attribute} #{message}" }.join(", ") + redirect_to omniauth_error_path(oauth['provider'], error: error_message) and return end end end diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index 25b5a702f9..c8e5d11447 100644 --- a/lib/gitlab/ldap/user.rb +++ b/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 - end + def initialize(auth_hash) + super + update_attributes + end - def ldap_conf - Gitlab.config.ldap - end + # 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) - end - filter - end + 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 + + def find_by_email + model.find_by(email: auth_hash.email) + end + + def update_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? diff --git a/lib/gitlab/oauth/user.rb b/lib/gitlab/oauth/user.rb index 3e23f3ad1a..8231886144 100644 --- a/lib/gitlab/oauth/user.rb +++ b/lib/gitlab/oauth/user.rb @@ -26,7 +26,7 @@ module Gitlab def save gl_user.save! - log.info "(OAuth) Creating user #{auth_hash.email} from login with extern_uid => #{auth_hash.uid}" + log.info "(OAuth) saving user #{auth_hash.email} from login with extern_uid => #{auth_hash.uid}" gl_user.block if needs_blocking? gl_user diff --git a/spec/lib/gitlab/ldap/user_spec.rb b/spec/lib/gitlab/ldap/user_spec.rb index d232cb2075..a1aec0bb96 100644 --- a/spec/lib/gitlab/ldap/user_spec.rb +++ b/spec/lib/gitlab/ldap/user_spec.rb @@ -1,30 +1,28 @@ 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 + let(:auth_hash) do + double(uid: 'my-uid', provider: 'ldap', info: double(info)) end - before { Gitlab.config.stub(omniauth: {}) } describe :find_or_create do - let(:auth) do - double(info: info, provider: 'ldap', uid: 'my-uid') - end - 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 From 97aa12fcc835f9b02a6e86b421c962fedf7a9377 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Thu, 2 Oct 2014 12:45:53 +0200 Subject: [PATCH 10/51] Dynamically initiate LDAP strategies --- .../omniauth_callbacks_controller.rb | 9 +- app/views/devise/sessions/new.html.haml | 11 +- config/gitlab.yml.example | 106 +++++++++--------- config/initializers/1_settings.rb | 2 +- config/initializers/7_omniauth.rb | 10 +- config/initializers/devise.rb | 4 +- 6 files changed, 71 insertions(+), 71 deletions(-) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 13855a9058..ccec22340e 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -32,12 +32,9 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController end end - alias_method :ldap0, :ldap - alias_method :ldap1, :ldap - alias_method :ldap2, :ldap - alias_method :ldap3, :ldap - alias_method :ldap4, :ldap - alias_method :ldap5, :ldap + Gitlab.config.ldap.servers.each do |server| + alias_method "ldap#{server.index}", :ldap + end def omniauth_error @provider = params[:provider] diff --git a/app/views/devise/sessions/new.html.haml b/app/views/devise/sessions/new.html.haml index 6bd2bcb57f..08e757bbcf 100644 --- a/app/views/devise/sessions/new.html.haml +++ b/app/views/devise/sessions/new.html.haml @@ -4,15 +4,15 @@ .login-body - if ldap_enabled? && gitlab_config.signin_enabled %ul.nav.nav-tabs - - @ldap_servers.each_with_index do |server, i| + - @ldap_servers.each do |server| %li{class: (:active if server['primary'])} - = link_to server['name'], "#tab-ldap#{i}", 'data-toggle' => 'tab' + = link_to server['label'], "#tab-ldap#{server.index}", 'data-toggle' => 'tab' %li = link_to 'Standard', '#tab-signin', 'data-toggle' => 'tab' .tab-content - - @ldap_servers.each_with_index do |server, i| - %div.tab-pane{id: "tab-ldap#{i}", class: (:active if server['primary'])} - = render 'devise/sessions/new_ldap', provider: "ldap#{i}" + - @ldap_servers.each do |server| + %div.tab-pane{id: "tab-ldap#{server.index}", class: (:active if server['primary'])} + = render 'devise/sessions/new_ldap', provider: "ldap#{server.index}" %div#tab-signin.tab-pane = render 'devise/sessions/new_base' @@ -38,7 +38,6 @@ %span.light Did not receive confirmation email? = link_to "Send again", new_confirmation_path(resource_name) - - if extra_config.has_key?('sign_in_text') %hr = markdown(extra_config.sign_in_text) diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index d42ebf144d..d5fdc44b63 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -134,63 +134,69 @@ production: &base # bundle exec rake gitlab:ldap:check RAILS_ENV=production ldap: enabled: false - host: '_your_ldap_server' - port: 636 - uid: 'sAMAccountName' - method: 'ssl' # "tls" or "ssl" or "plain" - bind_dn: '_the_full_dn_of_the_user_you_will_bind_with' - password: '_the_password_of_the_bind_user' + servers: + - + host: '_your_ldap_server' + port: 636 + uid: 'sAMAccountName' + method: 'ssl' # "tls" or "ssl" or "plain" + bind_dn: '_the_full_dn_of_the_user_you_will_bind_with' + password: '_the_password_of_the_bind_user' - # This setting controls the amount of time between LDAP permission checks for each user. - # After this time has expired for a given user, their next interaction with GitLab (a click in the web UI, a git pull etc.) will be slower because the LDAP permission check is being performed. - # How much slower depends on your LDAP setup, but it is not uncommon for this check to add seconds of waiting time. - # The default value is to have a 'slow click' once every 3600 seconds, i.e. once per hour. - # - # Warning: if you set this value too low, every click in GitLab will be a 'slow click' for all of your LDAP users. - # sync_time: 3600 + # When authenticating against an ldap server, this will provide a unique identifier + # Only use uniq numbers here + index: 1 - # If allow_username_or_email_login is enabled, GitLab will ignore everything - # after the first '@' in the LDAP username submitted by the user on login. - # - # Example: - # - the user enters 'jane.doe@example.com' and 'p@ssw0rd' as LDAP credentials; - # - GitLab queries the LDAP server with 'jane.doe' and 'p@ssw0rd'. - # - # If you are using "uid: 'userPrincipalName'" on ActiveDirectory you need to - # disable this setting, because the userPrincipalName contains an '@'. - allow_username_or_email_login: false + # This setting controls the amount of time between LDAP permission checks for each user. + # After this time has expired for a given user, their next interaction with GitLab (a click in the web UI, a git pull etc.) will be slower because the LDAP permission check is being performed. + # How much slower depends on your LDAP setup, but it is not uncommon for this check to add seconds of waiting time. + # The default value is to have a 'slow click' once every 3600 seconds, i.e. once per hour. + # + # Warning: if you set this value too low, every click in GitLab will be a 'slow click' for all of your LDAP users. + # sync_time: 3600 - # Base where we can search for users - # - # Ex. ou=People,dc=gitlab,dc=example - # - base: '' + # If allow_username_or_email_login is enabled, GitLab will ignore everything + # after the first '@' in the LDAP username submitted by the user on login. + # + # Example: + # - the user enters 'jane.doe@example.com' and 'p@ssw0rd' as LDAP credentials; + # - GitLab queries the LDAP server with 'jane.doe' and 'p@ssw0rd'. + # + # If you are using "uid: 'userPrincipalName'" on ActiveDirectory you need to + # disable this setting, because the userPrincipalName contains an '@'. + allow_username_or_email_login: false - # Filter LDAP users - # - # Format: RFC 4515 - # Ex. (employeeType=developer) - # - user_filter: '' + # Base where we can search for users + # + # Ex. ou=People,dc=gitlab,dc=example + # + base: '' - # Base where we can search for groups - # - # Ex. ou=Groups,dc=gitlab,dc=example - # - group_base: '' + # Filter LDAP users + # + # Format: RFC 4515 + # Ex. (employeeType=developer) + # + user_filter: '' - # LDAP group of users who should be admins in GitLab - # - # Ex. GLAdmins - # - admin_group: '' + # Base where we can search for groups + # + # Ex. ou=Groups,dc=gitlab,dc=example + # + group_base: '' - # Name of attribute which holds a ssh public key of the user object. - # If false or nil, SSH key syncronisation will be disabled. - # - # Ex. sshpublickey - # - sync_ssh_keys: false + # LDAP group of users who should be admins in GitLab + # + # Ex. GLAdmins + # + admin_group: '' + + # Name of attribute which holds a ssh public key of the user object. + # If false or nil, SSH key syncronisation will be disabled. + # + # Ex. sshpublickey + # + sync_ssh_keys: false ## OmniAuth settings omniauth: diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 2e4e431560..d0a9fc801e 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -64,7 +64,7 @@ if Settings.ldap['enabled'] && Settings.ldap['host'].present? excluded_per_server_settings = %w(sync_time allow_username_or_email_login) server = Settings.ldap.except(excluded_per_server_settings) server['primary'] = true - server['name'] = 'LDAP' + server['label'] = 'LDAP' Settings.ldap['servers'] = [server] end diff --git a/config/initializers/7_omniauth.rb b/config/initializers/7_omniauth.rb index b3c5260764..d2b890da4d 100644 --- a/config/initializers/7_omniauth.rb +++ b/config/initializers/7_omniauth.rb @@ -1,8 +1,6 @@ module OmniAuth::Strategies - class Ldap0 < LDAP; end - class Ldap1 < LDAP; end - class Ldap2 < LDAP; end - class Ldap3 < LDAP; end - class Ldap4 < LDAP; end - class Ldap5 < LDAP; end + Gitlab.config.ldap.servers.each do |server| + class_name = "Ldap#{server.index}" + const_set(class_name, Class.new(LDAP)) + end end diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 9e0ee7c548..07f0937f84 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -211,8 +211,8 @@ Devise.setup do |config| email_stripping_proc = ->(name) {name} end - Gitlab.config.ldap.servers.each_with_index do |server, i| - config.omniauth :"ldap#{i}", + Gitlab.config.ldap.servers.each do |server| + config.omniauth :"ldap#{server.index}", host: server['host'], base: server['base'], uid: server['uid'], From d9355ea59052b9c58b62325c0ad3af5b6d0c71fe Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 3 Oct 2014 13:59:20 +0200 Subject: [PATCH 11/51] Allow proper provider_name for ldap provider --- .../omniauth_callbacks_controller.rb | 2 +- app/views/devise/sessions/new.html.haml | 6 ++--- config/initializers/1_settings.rb | 22 ++++++++++++------- config/initializers/7_omniauth.rb | 5 ++--- config/initializers/devise.rb | 2 +- 5 files changed, 21 insertions(+), 16 deletions(-) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index ccec22340e..c9621b939a 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -33,7 +33,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController end Gitlab.config.ldap.servers.each do |server| - alias_method "ldap#{server.index}", :ldap + alias_method server.provider_name, :ldap end def omniauth_error diff --git a/app/views/devise/sessions/new.html.haml b/app/views/devise/sessions/new.html.haml index 08e757bbcf..328a2d8f04 100644 --- a/app/views/devise/sessions/new.html.haml +++ b/app/views/devise/sessions/new.html.haml @@ -6,13 +6,13 @@ %ul.nav.nav-tabs - @ldap_servers.each do |server| %li{class: (:active if server['primary'])} - = link_to server['label'], "#tab-ldap#{server.index}", 'data-toggle' => 'tab' + = link_to server['label'], "#tab-#{server.provider_name}", 'data-toggle' => 'tab' %li = link_to 'Standard', '#tab-signin', 'data-toggle' => 'tab' .tab-content - @ldap_servers.each do |server| - %div.tab-pane{id: "tab-ldap#{server.index}", class: (:active if server['primary'])} - = render 'devise/sessions/new_ldap', provider: "ldap#{server.index}" + %div.tab-pane{id: "tab-#{server.provider_name}", class: (:active if server['primary'])} + = render 'devise/sessions/new_ldap', provider: server.provider_name %div#tab-signin.tab-pane = render 'devise/sessions/new_base' diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index d0a9fc801e..617434385d 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -52,7 +52,6 @@ class Settings < Settingslogic end end - # Default settings Settings['ldap'] ||= Settingslogic.new({}) Settings.ldap['enabled'] = false if Settings.ldap['enabled'].nil? @@ -60,14 +59,21 @@ Settings.ldap['allow_username_or_email_login'] = false if Settings.ldap['allow_u Settings.ldap['sync_time'] = 3600 if Settings.ldap['sync_time'].nil? # backwards compatibility, we only have one host -if Settings.ldap['enabled'] && Settings.ldap['host'].present? - excluded_per_server_settings = %w(sync_time allow_username_or_email_login) - server = Settings.ldap.except(excluded_per_server_settings) - server['primary'] = true - server['label'] = 'LDAP' - Settings.ldap['servers'] = [server] -end +if Settings.ldap['enabled'] + if Settings.ldap['host'].present? + excluded_per_server_settings = %w(sync_time allow_username_or_email_login) + server = Settings.ldap.except(excluded_per_server_settings) + server['primary'] = true + server['label'] = 'LDAP' + server['provider_index'] = '' #providername will be ldap + Settings.ldap['servers'] = [server] + end + Settings.ldap['servers'].each do |server| + server['provider_name'] = "ldap#{server['provider_id']}".downcase + server['provider_class'] = OmniAuth::Utils.camelize(server['provider_name']) + end +end Settings['omniauth'] ||= Settingslogic.new({}) Settings.omniauth['enabled'] = false if Settings.omniauth['enabled'].nil? Settings.omniauth['providers'] ||= [] diff --git a/config/initializers/7_omniauth.rb b/config/initializers/7_omniauth.rb index d2b890da4d..4477597016 100644 --- a/config/initializers/7_omniauth.rb +++ b/config/initializers/7_omniauth.rb @@ -1,6 +1,5 @@ module OmniAuth::Strategies - Gitlab.config.ldap.servers.each do |server| - class_name = "Ldap#{server.index}" - const_set(class_name, Class.new(LDAP)) + Gitlab.config.ldap.servers.each_with_index do |server| + const_set(server.provider_class, Class.new(LDAP)) end end diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 07f0937f84..530cf26af5 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -212,7 +212,7 @@ Devise.setup do |config| end Gitlab.config.ldap.servers.each do |server| - config.omniauth :"ldap#{server.index}", + config.omniauth server.provider_name, host: server['host'], base: server['base'], uid: server['uid'], From a6422e271ef5c13bdb7bf46c574e5d7cee239ee4 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 3 Oct 2014 14:01:06 +0200 Subject: [PATCH 12/51] Add TODO for further refactoring --- lib/gitlab/ldap/user.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index c8e5d11447..078941b9de 100644 --- a/lib/gitlab/ldap/user.rb +++ b/lib/gitlab/ldap/user.rb @@ -10,6 +10,7 @@ module Gitlab module LDAP class User < Gitlab::OAuth::User class << self + # TODO: Look through LDAP servers until valid credentials are found? 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 From 63132b75134a5b07251a01cb38aced9db1b6e451 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 3 Oct 2014 17:16:12 +0200 Subject: [PATCH 13/51] Enable provider based LDAP adapter --- lib/gitlab/ldap/adapter.rb | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/lib/gitlab/ldap/adapter.rb b/lib/gitlab/ldap/adapter.rb index e6ab0cbfa2..16a296da29 100644 --- a/lib/gitlab/ldap/adapter.rb +++ b/lib/gitlab/ldap/adapter.rb @@ -1,11 +1,11 @@ module Gitlab module LDAP class Adapter - attr_reader :ldap + attr_reader :provider, :ldap - def self.open(&block) - Net::LDAP.open(adapter_options) do |ldap| - block.call(self.new(ldap)) + def self.open(provider, &block) + Net::LDAP.open(adapter_options(provider)) do |ldap| + block.call(self.new(provider, ldap)) end end @@ -13,7 +13,12 @@ module Gitlab Gitlab.config.ldap end - def self.adapter_options + def self.config_for(provider) + config.servers.find { |server| server.provider_name == provider } + end + + def self.adapter_options(provider) + config = config_for(provider) encryption = case config['method'].to_s when 'ssl' @@ -45,7 +50,8 @@ module Gitlab end - def initialize(ldap=nil) + def initialize(provider, ldap=nil) + @provider = provider @ldap = ldap || Net::LDAP.new(self.class.adapter_options) end @@ -133,7 +139,7 @@ module Gitlab private def config - @config ||= self.class.config + @config ||= self.class.config_for(provider) end end end From e08c14c0f0d4ba83cf7af1dd00028f1978f2cc03 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 3 Oct 2014 17:18:25 +0200 Subject: [PATCH 14/51] Use provider based ldap config --- lib/gitlab/ldap/access.rb | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index f7ef82bf46..4c18c6e6f0 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -42,17 +42,17 @@ module Gitlab end def update_permissions(user) - if Gitlab.config.ldap['sync_ssh_keys'] + if ldap_config['sync_ssh_keys'] update_ssh_keys(user) end # Skip updating group permissions # if instance does not use group_base setting - return true unless Gitlab.config.ldap['group_base'].present? + return true unless ldap_config['group_base'].present? update_ldap_group_links(user) - if Gitlab.config.ldap['admin_group'].present? + if ldap_config['admin_group'].present? update_admin_status(user) end end @@ -71,7 +71,7 @@ module Gitlab (ldap_user.ssh_keys - user.keys.ldap.pluck(:key)).each do |key| Rails.logger.info "#{self.class.name}: adding LDAP SSH key #{key.inspect} to #{user.name} (#{user.id})" - new_key = LDAPKey.new(title: "LDAP - #{Gitlab.config.ldap['sync_ssh_keys']}", key: key) + new_key = LDAPKey.new(title: "LDAP - #{ldap_config['sync_ssh_keys']}", key: key) new_key.user = user unless new_key.save Rails.logger.error "#{self.class.name}: failed to add LDAP SSH key #{key.inspect} to #{user.name} (#{user.id})\n"\ @@ -100,7 +100,7 @@ module Gitlab end def update_admin_status(user) - admin_group = Gitlab::LDAP::Group.find_by_cn(Gitlab.config.ldap['admin_group'], adapter) + admin_group = Gitlab::LDAP::Group.find_by_cn(ldap_config['admin_group'], adapter) if admin_group.has_member?(Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter)) unless user.admin? user.admin = true @@ -140,6 +140,10 @@ module Gitlab end.map(&:cn) end + def ldap_config + Gitlab::LDAP::Adapter.config_for(provider) + end + private def gitlab_groups_with_ldap_link ::Group.includes(:ldap_group_links).references(:ldap_group_links). From ce22ffbc7f18d4fa4d249c5db66ba10942f02ff3 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 3 Oct 2014 17:21:38 +0200 Subject: [PATCH 15/51] Reuse the ldap adapter --- lib/gitlab/ldap/access.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index 4c18c6e6f0..adf12f5abd 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -38,7 +38,7 @@ module Gitlab end def get_ldap_user(user) - @ldap_user ||= Gitlab::LDAP::Person.find_by_dn(user.extern_uid) + @ldap_user ||= Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter) end def update_permissions(user) From 0e276f9cbf902553fd2dd05ae7843f17479503c9 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 3 Oct 2014 17:27:14 +0200 Subject: [PATCH 16/51] Move allowed call to LDAP::User --- app/controllers/omniauth_callbacks_controller.rb | 2 +- lib/gitlab/ldap/user.rb | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index c9621b939a..0f364a48ea 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -24,7 +24,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController gl_user.remember_me = true if @user.persisted? # Do additional LDAP checks for the user filter and EE features - if Gitlab::LDAP::Access.allowed?(gl_user) + if @user.allowed? sign_in_and_redirect(gl_user) else flash[:alert] = "Access denied for your LDAP account." diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index 078941b9de..a50a30d6bd 100644 --- a/lib/gitlab/ldap/user.rb +++ b/lib/gitlab/ldap/user.rb @@ -89,6 +89,10 @@ module Gitlab def needs_blocking? false end + + def allowed? + Gitlab::LDAP::Access.allowed?(gl_user) + end end end end From e23a26aea02ebef4aefa1463ebf589c7c5c4bff0 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 3 Oct 2014 17:27:35 +0200 Subject: [PATCH 17/51] Add provider context to Ldap::Access --- lib/gitlab/ldap/access.rb | 18 ++++++++++++------ lib/gitlab/ldap/user.rb | 2 +- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index adf12f5abd..901acb6305 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -1,16 +1,21 @@ +# LDAP authorization model +# +# * Check if we are allowed access (not blocked) +# * Update authorizations and associations +# module Gitlab module LDAP class Access - attr_reader :adapter + attr_reader :adapter, :provider - def self.open(&block) - Gitlab::LDAP::Adapter.open do |adapter| - block.call(self.new(adapter)) + def self.open(provider, &block) + Gitlab::LDAP::Adapter.open(provider) do |adapter| + block.call(self.new(provider, adapter)) end end def self.allowed?(user) - self.open do |access| + self.open(user.provider) do |access| if access.allowed?(user) access.update_permissions(user) access.update_email(user) @@ -23,7 +28,8 @@ module Gitlab end end - def initialize(adapter=nil) + def initialize(provider, adapter=nil) + @provider = provider @adapter = adapter end diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index a50a30d6bd..28a48671d8 100644 --- a/lib/gitlab/ldap/user.rb +++ b/lib/gitlab/ldap/user.rb @@ -66,7 +66,7 @@ module Gitlab def find_by_uid_and_provider # LDAP distinguished name is case-insensitive model. - where(provider: auth_hash.provider). + where(provider: [auth_hash.provider, :ldap]). where('lower(extern_uid) = ?', auth_hash.uid.downcase).last end From e151bbbb285c8eafe4fe8aff06d225fef71e520e Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Tue, 7 Oct 2014 12:22:16 +0200 Subject: [PATCH 18/51] Extract LDAP config to seperate class --- lib/gitlab/ldap/config.rb | 81 +++++++++++++++++++++++++++++ spec/lib/gitlab/ldap/config_spec.rb | 16 ++++++ 2 files changed, 97 insertions(+) create mode 100644 lib/gitlab/ldap/config.rb create mode 100644 spec/lib/gitlab/ldap/config_spec.rb diff --git a/lib/gitlab/ldap/config.rb b/lib/gitlab/ldap/config.rb new file mode 100644 index 0000000000..37316f41a9 --- /dev/null +++ b/lib/gitlab/ldap/config.rb @@ -0,0 +1,81 @@ +# Load a specific server configuration +module Gitlab + module LDAP + class Config + attr_accessor :provider, :options + + def initialize(provider) + @provider = provider + @options = config_for(provider) + end + + def enabled? + base_config.enabled + end + + def adapter_options + { + host: options['host'], + port: options['port'], + encryption: encryption + }.tap do |options| + options.merge!(auth_options) if has_auth? + end + end + + def sync_ssh_keys? + ssh_sync_key.present? + end + + def ssh_sync_key + options['sync_ssh_keys'] + end + + def user_filter + options['user_filter'] + end + + def group_base + options['group_base'] + end + + def admin_group + options['admin_group'] + end + + protected + def base_config + Gitlab.config.ldap + end + + def config_for(provider) + base_config.servers.find { |server| server.provider_name == provider } + end + + def encryption + case options['method'].to_s + when 'ssl' + :simple_tls + when 'tls' + :start_tls + else + nil + end + end + + def auth_options + { + auth: { + method: :simple, + username: options['bind_dn'], + password: options['password'] + } + } + end + + def has_auth? + options['password'] || options['bind_dn'] + end + end + end +end diff --git a/spec/lib/gitlab/ldap/config_spec.rb b/spec/lib/gitlab/ldap/config_spec.rb new file mode 100644 index 0000000000..a01166b264 --- /dev/null +++ b/spec/lib/gitlab/ldap/config_spec.rb @@ -0,0 +1,16 @@ +require 'spec_helper' + +describe Gitlab::LDAP::Config do + let(:config) { Gitlab::LDAP::Config.new provider } + let(:provider) { 'ldapmain' } + + describe :initalize do + it 'requires a provider' do + expect{ Gitlab::LDAP::Config.new }.to raise_error ArgumentError + end + + it "works" do + expect(config).to be_a described_class + end + end +end \ No newline at end of file From e60da677f2dca811f616cca1028f079be06a3a56 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Tue, 7 Oct 2014 12:28:02 +0200 Subject: [PATCH 19/51] Add example LDAP config for tests --- config/gitlab.yml.example | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index d5fdc44b63..e14bc4907b 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -315,6 +315,21 @@ test: project_url: "http://redmine/projects/:issues_tracker_id" issues_url: "http://redmine/:project_id/:issues_tracker_id/:id" new_issue_url: "http://redmine/projects/:issues_tracker_id/issues/new" - + ldap: + enabled: false + servers: + - + provider_id: main + label: ldap + primary: true + host: 127.0.0.1 + port: 3890 + uid: 'uid' + method: 'plain' # "tls" or "ssl" or "plain" + base: 'dc=example,dc=com' + user_filter: '' + group_base: 'ou=groups,dc=example,dc=com' + admin_group: '' + sync_ssh_keys staging: <<: *base From 4ef74844c3295124d87b5b11518f0bb482fb83ce Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Tue, 7 Oct 2014 12:31:30 +0200 Subject: [PATCH 20/51] Adapt to new LDAP::Config class --- config/initializers/1_settings.rb | 4 +- lib/gitlab/ldap/access.rb | 24 ++++++++++-- lib/gitlab/ldap/adapter.rb | 57 +++++----------------------- lib/gitlab/ldap/user.rb | 8 ++-- spec/lib/gitlab/ldap/access_spec.rb | 3 +- spec/lib/gitlab/ldap/adapter_spec.rb | 2 +- spec/lib/gitlab/ldap/user_spec.rb | 17 +++++---- 7 files changed, 46 insertions(+), 69 deletions(-) diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 617434385d..17123a3049 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -59,13 +59,13 @@ Settings.ldap['allow_username_or_email_login'] = false if Settings.ldap['allow_u Settings.ldap['sync_time'] = 3600 if Settings.ldap['sync_time'].nil? # backwards compatibility, we only have one host -if Settings.ldap['enabled'] +if Settings.ldap['enabled'] || Rails.env.test? if Settings.ldap['host'].present? excluded_per_server_settings = %w(sync_time allow_username_or_email_login) server = Settings.ldap.except(excluded_per_server_settings) server['primary'] = true server['label'] = 'LDAP' - server['provider_index'] = '' #providername will be ldap + server['provider_id'] = '' #providername will be ldap Settings.ldap['servers'] = [server] end diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index 901acb6305..262d41d56a 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -43,22 +43,26 @@ module Gitlab false end + def adapter + @adapter ||= Gitlab::LDAP::Adapter.new(provider) + end + def get_ldap_user(user) @ldap_user ||= Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter) end def update_permissions(user) - if ldap_config['sync_ssh_keys'] + if sync_ssh_keys? update_ssh_keys(user) end # Skip updating group permissions # if instance does not use group_base setting - return true unless ldap_config['group_base'].present? + return true unless group_base.present? update_ldap_group_links(user) - if ldap_config['admin_group'].present? + if admin_group.present? update_admin_status(user) end end @@ -147,7 +151,19 @@ module Gitlab end def ldap_config - Gitlab::LDAP::Adapter.config_for(provider) + Gitlab::LDAP::Config.new(provider) + end + + def sync_ssh_keys? + ldap_config.sync_ssh_keys? + end + + def group_base + ldap_config.group_base + end + + def admin_group + ldap_config.admin_group end private diff --git a/lib/gitlab/ldap/adapter.rb b/lib/gitlab/ldap/adapter.rb index 16a296da29..1e58bd7f8e 100644 --- a/lib/gitlab/ldap/adapter.rb +++ b/lib/gitlab/ldap/adapter.rb @@ -4,55 +4,22 @@ module Gitlab attr_reader :provider, :ldap def self.open(provider, &block) - Net::LDAP.open(adapter_options(provider)) do |ldap| + Net::LDAP.open(config(provider).adapter_options) do |ldap| block.call(self.new(provider, ldap)) end end - def self.config - Gitlab.config.ldap + def self.config(provider) + Gitlab::LDAP::Config.new(provider) end - def self.config_for(provider) - config.servers.find { |server| server.provider_name == provider } - end - - def self.adapter_options(provider) - config = config_for(provider) - encryption = - case config['method'].to_s - when 'ssl' - :simple_tls - when 'tls' - :start_tls - else - nil - end - - options = { - host: config['host'], - port: config['port'], - encryption: encryption - } - - auth_options = { - auth: { - method: :simple, - username: config['bind_dn'], - password: config['password'] - } - } - - if config['password'] || config['bind_dn'] - options.merge!(auth_options) - end - options - end - - def initialize(provider, ldap=nil) @provider = provider - @ldap = ldap || Net::LDAP.new(self.class.adapter_options) + @ldap = ldap || Net::LDAP.new(config.adapter_options) + end + + def config + Gitlab::LDAP::Config.new(provider) end # Get LDAP groups from ou=Groups @@ -107,7 +74,7 @@ module Gitlab end entries.map do |entry| - Gitlab::LDAP::Person.new(entry) + Gitlab::LDAP::Person.new(entry, provider) end end @@ -135,12 +102,6 @@ module Gitlab results end end - - private - - def config - @config ||= self.class.config_for(provider) - end end end end diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index 28a48671d8..5bca12f8f0 100644 --- a/lib/gitlab/ldap/user.rb +++ b/lib/gitlab/ldap/user.rb @@ -26,21 +26,21 @@ module Gitlab end def adapter - @adapter ||= OmniAuth::LDAP::Adaptor.new(ldap_conf) + @adapter ||= OmniAuth::LDAP::Adaptor.new(ldap_conf.options) 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']) + 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 ldap_conf - Gitlab.config.ldap + Gitlab::LDAP::Config.new(provider) end def find_by_uid(uid) diff --git a/spec/lib/gitlab/ldap/access_spec.rb b/spec/lib/gitlab/ldap/access_spec.rb index 92c823fcb0..4c24abf12d 100644 --- a/spec/lib/gitlab/ldap/access_spec.rb +++ b/spec/lib/gitlab/ldap/access_spec.rb @@ -1,10 +1,9 @@ require 'spec_helper' describe Gitlab::LDAP::Access do - let(:access) { Gitlab::LDAP::Access.new } + let(:access) { Gitlab::LDAP::Access.new 'ldapmain' } let(:user) { create(:user) } - describe :allowed? do subject { access.allowed?(user) } diff --git a/spec/lib/gitlab/ldap/adapter_spec.rb b/spec/lib/gitlab/ldap/adapter_spec.rb index c3f0733443..19347e4737 100644 --- a/spec/lib/gitlab/ldap/adapter_spec.rb +++ b/spec/lib/gitlab/ldap/adapter_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Gitlab::LDAP::Adapter do - let(:adapter) { Gitlab::LDAP::Adapter.new } + let(:adapter) { Gitlab::LDAP::Adapter.new 'ldapmain' } describe :dn_matches_filter? do let(:ldap) { double(:ldap) } diff --git a/spec/lib/gitlab/ldap/user_spec.rb b/spec/lib/gitlab/ldap/user_spec.rb index a1aec0bb96..9e0334adc6 100644 --- a/spec/lib/gitlab/ldap/user_spec.rb +++ b/spec/lib/gitlab/ldap/user_spec.rb @@ -10,12 +10,12 @@ describe Gitlab::LDAP::User do } end let(:auth_hash) do - double(uid: 'my-uid', provider: 'ldap', info: double(info)) + double(uid: 'my-uid', provider: 'ldapmain', 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') + existing_user = create(:user, extern_uid: 'my-uid', provider: 'ldapmain') expect{ gl_user.save }.to_not change{ User.count } end @@ -26,7 +26,7 @@ describe Gitlab::LDAP::User do existing_user.reload expect(existing_user.extern_uid).to eql 'my-uid' - expect(existing_user.provider).to eql 'ldap' + expect(existing_user.provider).to eql 'ldapmain' end it "creates a new user if not found" do @@ -38,13 +38,14 @@ describe Gitlab::LDAP::User do let(:login) { 'john' } let(:password) { 'my-secret' } - before { - Gitlab.config.ldap['enabled'] = true - Gitlab.config.ldap['user_filter'] = 'employeeType=developer' - } - after { Gitlab.config.ldap['enabled'] = false } + # before { + # Gitlab.config.ldap['enabled'] = true + # Gitlab.config.ldap['user_filter'] = 'employeeType=developer' + # } + # after { Gitlab.config.ldap['enabled'] = false } it "send an authentication request to ldap" do + pending('needs refactoring') expect( Gitlab::LDAP::User.adapter ).to receive(:bind_as) Gitlab::LDAP::User.authenticate(login, password) end From 1bc99369dc63fa8c556a679cd4f1686b12f41222 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Tue, 7 Oct 2014 17:25:13 +0200 Subject: [PATCH 21/51] Adapt tests to refactoring - Use smarter instance methods - Support multiple LDAP servers --- lib/gitlab/ldap/access.rb | 62 +++++---- lib/gitlab/ldap/person.rb | 27 ++-- lib/gitlab/ldap/user.rb | 3 +- spec/factories.rb | 5 + spec/lib/gitlab/ldap/access_spec.rb | 189 ++++++++++++---------------- 5 files changed, 129 insertions(+), 157 deletions(-) diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index 262d41d56a..0160430a56 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -6,19 +6,19 @@ module Gitlab module LDAP class Access - attr_reader :adapter, :provider + attr_reader :adapter, :provider, :user, :ldap_user - def self.open(provider, &block) - Gitlab::LDAP::Adapter.open(provider) do |adapter| - block.call(self.new(provider, adapter)) + def self.open(user, &block) + Gitlab::LDAP::Adapter.open(user.provider) do |adapter| + block.call(self.new(user, adapter)) end end def self.allowed?(user) - self.open(user.provider) do |access| - if access.allowed?(user) - access.update_permissions(user) - access.update_email(user) + self.open(user) do |access| + if access.allowed? + access.update_permissions + access.update_email user.last_credential_check_at = Time.now user.save true @@ -28,12 +28,13 @@ module Gitlab end end - def initialize(provider, adapter=nil) - @provider = provider + def initialize(user, adapter=nil) @adapter = adapter + @user = user + @provider = user.provider end - def allowed?(user) + def allowed? if Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter) !Gitlab::LDAP::Person.disabled_via_active_directory?(user.extern_uid, adapter) else @@ -47,31 +48,28 @@ module Gitlab @adapter ||= Gitlab::LDAP::Adapter.new(provider) end - def get_ldap_user(user) + def ldap_user @ldap_user ||= Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter) end - def update_permissions(user) + def update_permissions if sync_ssh_keys? - update_ssh_keys(user) + update_ssh_keys end # Skip updating group permissions # if instance does not use group_base setting return true unless group_base.present? - update_ldap_group_links(user) + update_ldap_group_links if admin_group.present? - update_admin_status(user) + update_admin_status end end # Update user ssh keys if they changed in LDAP - def update_ssh_keys(user) - # Get LDAP user entry - ldap_user = get_ldap_user(user) - + def update_ssh_keys user.keys.ldap.where.not(key: ldap_user.ssh_keys).each do |deleted_key| Rails.logger.info "#{self.class.name}: removing LDAP SSH key #{deleted_key.key} from #{user.name} (#{user.id})" unless deleted_key.destroy @@ -81,7 +79,7 @@ module Gitlab (ldap_user.ssh_keys - user.keys.ldap.pluck(:key)).each do |key| Rails.logger.info "#{self.class.name}: adding LDAP SSH key #{key.inspect} to #{user.name} (#{user.id})" - new_key = LDAPKey.new(title: "LDAP - #{ldap_config['sync_ssh_keys']}", key: key) + new_key = LDAPKey.new(title: "LDAP - #{ldap_config.ssh_sync_key}", key: key) new_key.user = user unless new_key.save Rails.logger.error "#{self.class.name}: failed to add LDAP SSH key #{key.inspect} to #{user.name} (#{user.id})\n"\ @@ -91,16 +89,12 @@ module Gitlab end # Update user email if it changed in LDAP - def update_email(user) - uid = user.extern_uid - ldap_user = get_ldap_user(user) - gitlab_user = ::User.where(provider: 'ldap', extern_uid: uid).last - - if gitlab_user && ldap_user && ldap_user.email + def update_email + if ldap_user.try(:email) ldap_email = ldap_user.email.last.to_s.downcase - if (gitlab_user.email != ldap_email) - gitlab_user.update(email: ldap_email) + if (user.email != ldap_email) + user.update(email: ldap_email) else false end @@ -109,8 +103,8 @@ module Gitlab end end - def update_admin_status(user) - admin_group = Gitlab::LDAP::Group.find_by_cn(ldap_config['admin_group'], adapter) + def update_admin_status + admin_group = Gitlab::LDAP::Group.find_by_cn(ldap_config.admin_group, adapter) if admin_group.has_member?(Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter)) unless user.admin? user.admin = true @@ -125,9 +119,9 @@ module Gitlab end # Loop throug all ldap conneted groups, and update the users link with it - def update_ldap_group_links(user) + def update_ldap_group_links gitlab_groups_with_ldap_link.each do |group| - active_group_links = group.ldap_group_links.where(cn: cns_with_access(get_ldap_user(user))) + active_group_links = group.ldap_group_links.where(cn: cns_with_access) if active_group_links.any? group.add_users([user.id], fetch_group_access(group, user, active_group_links)) @@ -144,7 +138,7 @@ module Gitlab end # returns a collection of cn strings to which the user has access - def cns_with_access(ldap_user) + def cns_with_access @ldap_groups_with_access ||= ldap_groups.select do |ldap_group| ldap_group.has_member?(ldap_user) end.map(&:cn) diff --git a/lib/gitlab/ldap/person.rb b/lib/gitlab/ldap/person.rb index d9d89ea16d..125673a5df 100644 --- a/lib/gitlab/ldap/person.rb +++ b/lib/gitlab/ldap/person.rb @@ -6,24 +6,24 @@ module Gitlab # Source: http://ctogonewild.com/2009/09/03/bitmask-searches-in-ldap/ AD_USER_DISABLED = Net::LDAP::Filter.ex("userAccountControl:1.2.840.113556.1.4.803", "2") - def self.find_by_uid(uid, adapter=nil) - adapter ||= Gitlab::LDAP::Adapter.new + attr_accessor :entry, :provider + + def self.find_by_uid(uid, adapter) adapter.user(Gitlab.config.ldap.uid, uid) end - def self.find_by_dn(dn, adapter=nil) - adapter ||= Gitlab::LDAP::Adapter.new + def self.find_by_dn(dn, adapter) adapter.user('dn', dn) end - def self.disabled_via_active_directory?(dn, adapter=nil) - adapter ||= Gitlab::LDAP::Adapter.new + def self.disabled_via_active_directory?(dn, adapter) adapter.dn_matches_filter?(dn, AD_USER_DISABLED) end - def initialize(entry) + def initialize(entry, provider) Rails.logger.debug { "Instantiating #{self.class.name} with LDIF:\n#{entry.to_ldif}" } @entry = entry + @provider = provider end def name @@ -47,9 +47,8 @@ module Gitlab end def ssh_keys - ssh_keys_attribute = Gitlab.config.ldap['sync_ssh_keys'].to_sym - if entry.respond_to?(ssh_keys_attribute) - entry[ssh_keys_attribute] + if config.sync_ssh_keys? && entry.respond_to?(config.ssh_sync_key) + entry[config.ssh_sync_key.to_sym] else [] end @@ -61,12 +60,12 @@ module Gitlab @entry end - def adapter - @adapter ||= Gitlab::LDAP::Adapter.new - end + # def adapter + # @adapter ||= Gitlab::LDAP::Adapter.new + # end def config - @config ||= Gitlab.config.ldap + @config ||= Gitlab::LDAP::Config.new(provider) end end end diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index 5bca12f8f0..300c12339d 100644 --- a/lib/gitlab/ldap/user.rb +++ b/lib/gitlab/ldap/user.rb @@ -14,8 +14,9 @@ module Gitlab 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 - return nil unless ldap_conf.enabled && login.present? && password.present? + return nil unless ldap_conf.enabled? && login.present? && password.present? + binding.pry ldap_user = adapter.bind_as( filter: user_filter(login), size: 1, diff --git a/spec/factories.rb b/spec/factories.rb index f7f65bffb8..f15763adcb 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -24,6 +24,11 @@ FactoryGirl.define do admin true end + trait :ldap do + provider 'ldapmain' + extern_uid 'my-ldap-id' + end + factory :admin, traits: [:admin] end diff --git a/spec/lib/gitlab/ldap/access_spec.rb b/spec/lib/gitlab/ldap/access_spec.rb index 4c24abf12d..07e8ab4093 100644 --- a/spec/lib/gitlab/ldap/access_spec.rb +++ b/spec/lib/gitlab/ldap/access_spec.rb @@ -1,11 +1,11 @@ require 'spec_helper' describe Gitlab::LDAP::Access do - let(:access) { Gitlab::LDAP::Access.new 'ldapmain' } - let(:user) { create(:user) } + let(:access) { Gitlab::LDAP::Access.new user } + let(:user) { create(:user, :ldap) } describe :allowed? do - subject { access.allowed?(user) } + subject { access.allowed? } context 'when the user cannot be found' do before { Gitlab::LDAP::Person.stub(find_by_dn: nil) } @@ -31,161 +31,131 @@ describe Gitlab::LDAP::Access do end describe :update_permissions do - subject { access.update_permissions(user) } - - before do - Gitlab.config.ldap['enabled'] = true - Gitlab.config.ldap['sync_ssh_keys'] = false - Gitlab.config.ldap['group_base'] = 'something' - Gitlab.config.ldap['admin_group'] = '' - end - - after do - Gitlab.config.ldap['enabled'] = false - end + subject { access.update_permissions } it "syncs ssh keys if enabled by configuration" do - Gitlab.config.ldap['sync_ssh_keys'] = true - expect(access).to receive(:update_ssh_keys).with(user).once + access.stub sync_ssh_keys?: true + expect(access).to receive(:update_ssh_keys).once + + subject + end + + it "does update group permissions with a group base configured" do + access.stub group_base: 'my-group-base' + expect(access).to receive(:update_ldap_group_links) subject end it "does not update group permissions without a group base configured" do - Gitlab.config.ldap['group_base'] = '' - expect(access).not_to receive(:update_ldap_group_links).with(user) + access.stub group_base: '' + expect(access).not_to receive(:update_ldap_group_links) subject end it "does update admin group permissions if admin group is configured" do - Gitlab.config.ldap['admin_group'] = 'NSA' - - access.stub(:update_ldap_group_links) - expect(access).to receive(:update_admin_status).with(user) + access.stub admin_group: 'my-admin-group' + access.stub :update_ldap_group_links + expect(access).to receive(:update_admin_status) subject end end describe :update_ssh_keys do - let(:user_ldap) { create(:user, provider: 'ldap', extern_uid: "66049")} let(:ssh_key) { 'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCrSQHff6a1rMqBdHFt+FwIbytMZ+hJKN3KLkTtOWtSvNIriGhnTdn4rs+tjD/w+z+revytyWnMDM9dS7J8vQi006B16+hc9Xf82crqRoPRDnBytgAFFQY1G/55ql2zdfsC5yvpDOFzuwIJq5dNGsojS82t6HNmmKPq130fzsenFnj5v1pl3OJvk513oduUyKiZBGTroWTn7H/eOPtu7s9MD7pAdEjqYKFLeaKmyidiLmLqQlCRj3Tl2U9oyFg4PYNc0bL5FZJ/Z6t0Ds3i/a2RanQiKxrvgu3GSnUKMx7WIX373baL4jeM7cprRGiOY/1NcS+1cAjfJ8oaxQF/1dYj' } - let(:key_ldap) { LDAPKey.new(title: 'used to be a ldap key', key: ssh_key) } + let(:ssh_key_attribute_name) { 'sshpublickey' } + let(:entry) { + Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com\n#{ssh_key_attribute_name}: #{ssh_key}") } before do - @old_value = Gitlab.config.ldap['sync_ssh_keys'] - key_attribute_name = 'sshpublickey' - Gitlab.config.ldap['sync_ssh_keys'] = key_attribute_name - end - - after do - Gitlab.config.ldap['sync_ssh_keys'] = @old_value + Gitlab::LDAP::Config.any_instance.stub(ssh_sync_key: ssh_key_attribute_name) + access.stub sync_ssh_keys?: true end it "should add a SSH key if it is in LDAP but not in gitlab" do - entry = Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com\n#{Gitlab.config.ldap['sync_ssh_keys']}: #{ssh_key}") - Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry) } + entry = Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com\n#{ssh_key_attribute_name}: #{ssh_key}") + Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry, 'ldapmain') } - expect(user_ldap.keys.size).to be(0) - access.update_ssh_keys(user_ldap) - user_ldap.reload - expect(user_ldap.keys.size).to be(1) + expect{ access.update_ssh_keys }.to change(user.keys, :count).from(0).to(1) end it "should add a SSH key and give it a proper name" do - entry = Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com\n#{Gitlab.config.ldap['sync_ssh_keys']}: #{ssh_key}") - Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry) } + entry = Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com\n#{ssh_key_attribute_name}: #{ssh_key}") + Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry, 'ldapmain') } - access.update_ssh_keys(user_ldap) - expect(user_ldap.keys.last.title).to match(/LDAP/) - expect(user_ldap.keys.last.title).to match(/#{Gitlab.config.ldap['sync_ssh_keys']}/) + access.update_ssh_keys + expect(user.keys.last.title).to match(/LDAP/) + expect(user.keys.last.title).to match(/#{access.ldap_config.ssh_sync_key}/) end it "should not add a SSH key if it is invalid" do - entry = Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com\n#{Gitlab.config.ldap['sync_ssh_keys']}: I am not a valid key") - Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry) } + entry = Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com\n#{ssh_key_attribute_name}: I am not a valid key") + Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry, 'ldapmain') } - expect(user_ldap.keys.size).to be(0) - access.update_ssh_keys(user_ldap) - expect(user_ldap.keys.size).to be(0) + expect{ access.update_ssh_keys }.to_not change(user.keys, :count) end context 'user has at least one LDAPKey' do - it "should remove a SSH key if it is no longer in LDAP" do - entry = Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com\n#{Gitlab.config.ldap['sync_ssh_keys']}:\n") - Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry) } - key_ldap.save - user_ldap.keys << key_ldap + before { user.keys.ldap.create key: ssh_key, title: 'to be removed' } - expect(user_ldap.keys.size).to be(1) - access.update_ssh_keys(user_ldap) - expect(user_ldap.keys.size).to be(0) + it "should remove a SSH key if it is no longer in LDAP" do + entry = Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com\n#{ssh_key_attribute_name}:\n") + Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry, 'ldapmain') } + + expect{ access.update_ssh_keys }.to change(user.keys, :count).from(1).to(0) end - it "should remove a SSH key if the ldap attribute was removes" do + it "should remove a SSH key if the ldap attribute was removed" do entry = Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com") - Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry) } - key_ldap.save - user_ldap.keys << key_ldap - expect(user_ldap.keys.size).to be(1) - access.update_ssh_keys(user_ldap) - expect(user_ldap.keys.size).to be(0) + Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry, 'ldapmain') } + + expect{ access.update_ssh_keys }.to change(user.keys, :count).from(1).to(0) end end end describe :update_user_email do - let(:user_ldap) { create(:user, provider: 'ldap', extern_uid: "66048")} + let(:entry) { Net::LDAP::Entry.new } + + before do + access.stub ldap_user: Gitlab::LDAP::Person.new(entry, user.provider) + end it "should not update email if email attribute is not set" do - entry = Net::LDAP::Entry.new - Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry) } - updated = access.update_email(user_ldap) - updated.should == false + expect{ access.update_email }.to_not change(user, :unconfirmed_email) end it "should not update the email if the user has the same email in GitLab and in LDAP" do - entry = Net::LDAP::Entry.new - entry['mail'] = [user_ldap.email] - Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry) } - updated = access.update_email(user_ldap) - updated.should == false + entry['mail'] = [user.email] + expect{ access.update_email }.to_not change(user, :unconfirmed_email) end it "should not update the email if the user has the same email GitLab and in LDAP, but with upper case in LDAP" do - entry = Net::LDAP::Entry.new - entry['mail'] = [user_ldap.email.upcase] - Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry) } - updated = access.update_email(user_ldap) - updated.should == false + entry['mail'] = [user.email.upcase] + expect{ access.update_email }.to_not change(user, :unconfirmed_email) end it "should update the email if the user email is different" do - entry = Net::LDAP::Entry.new entry['mail'] = ["new_email@example.com"] - Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry) } - updated = access.update_email(user_ldap) - updated.should == true + expect{ access.update_email }.to change(user, :unconfirmed_email) end end describe :update_admin_status do - let(:gitlab_user) { create(:user, provider: 'ldap', extern_uid: "admin2")} - let(:gitlab_admin) { create(:admin, provider: 'ldap', extern_uid: "admin2")} - before do - Gitlab.config.ldap['admin_group'] = "GLAdmins" + access.stub(admin_group: "GLAdmins") ldap_user_entry = Net::LDAP::Entry.new - Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(ldap_user_entry) } + Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(ldap_user_entry, user.provider) } Gitlab::LDAP::Person.any_instance.stub(:uid) { 'admin2' } end it "should give admin privileges to an User" do admin_group = Net::LDAP::Entry.from_single_ldif_string( -%Q{dn: cn=#{Gitlab.config.ldap['admin_group']},ou=groups,dc=bar,dc=com -cn: #{Gitlab.config.ldap['admin_group']} +%Q{dn: cn=#{access.admin_group},ou=groups,dc=bar,dc=com +cn: #{access.admin_group} description: GitLab admins gidnumber: 42 memberuid: admin1 @@ -195,15 +165,15 @@ objectclass: top objectclass: posixGroup }) Gitlab::LDAP::Adapter.any_instance.stub(:group) { Gitlab::LDAP::Group.new(admin_group) } - expect(gitlab_user.admin?).to be false - access.update_admin_status(gitlab_user) - expect(gitlab_user.admin?).to be true + + expect{ access.update_admin_status }.to change(user, :admin?).to(true) end it "should remove admin privileges from an User" do + user.update_attribute(:admin, true) admin_group = Net::LDAP::Entry.from_single_ldif_string( -%Q{dn: cn=#{Gitlab.config.ldap['admin_group']},ou=groups,dc=bar,dc=com -cn: #{Gitlab.config.ldap['admin_group']} +%Q{dn: cn=#{access.admin_group},ou=groups,dc=bar,dc=com +cn: #{access.admin_group} description: GitLab admins gidnumber: 42 memberuid: admin1 @@ -212,9 +182,7 @@ objectclass: top objectclass: posixGroup }) Gitlab::LDAP::Adapter.any_instance.stub(:group) { Gitlab::LDAP::Group.new(admin_group) } - expect(gitlab_admin.admin?).to be true - access.update_admin_status(gitlab_admin) - expect(gitlab_admin.admin?).to be false + expect{ access.update_admin_status }.to change(user, :admin?).to(false) end end @@ -225,17 +193,17 @@ objectclass: posixGroup let(:gitlab_group_2) { create :group } before do - access.stub(:get_ldap_user) access.stub(cns_with_access: cns_with_access) end context "non existing access for group-1, allowed via ldap-group1 as MASTER" do before do - gitlab_group_1.ldap_group_links.create cn: 'ldap-group1', group_access: Gitlab::Access::MASTER + gitlab_group_1.ldap_group_links.create({ + cn: 'ldap-group1', group_access: Gitlab::Access::MASTER }) end it "gives the user master access for group 1" do - access.update_ldap_group_links(user) + access.update_ldap_group_links expect( gitlab_group_1.has_master?(user) ).to be_true end end @@ -243,11 +211,12 @@ objectclass: posixGroup context "existing access as guest for group-1, allowed via ldap-group1 as DEVELOPER" do before do gitlab_group_1.users_groups.guests.create(user_id: user.id) - gitlab_group_1.ldap_group_links.create cn: 'ldap-group1', group_access: Gitlab::Access::MASTER + gitlab_group_1.ldap_group_links.create({ + cn: 'ldap-group1', group_access: Gitlab::Access::MASTER }) end it "upgrades the users access to master for group 1" do - expect { access.update_ldap_group_links(user) }.to \ + expect { access.update_ldap_group_links }.to \ change{ gitlab_group_1.has_master?(user) }.from(false).to(true) end end @@ -255,11 +224,12 @@ objectclass: posixGroup context "existing access as MASTER for group-1, allowed via ldap-group1 as DEVELOPER" do before do gitlab_group_1.users_groups.masters.create(user_id: user.id) - gitlab_group_1.ldap_group_links.create cn: 'ldap-group1', group_access: Gitlab::Access::DEVELOPER + gitlab_group_1.ldap_group_links.create({ + cn: 'ldap-group1', group_access: Gitlab::Access::DEVELOPER }) end it "keeps the users master access for group 1" do - expect { access.update_ldap_group_links(user) }.not_to \ + expect { access.update_ldap_group_links }.not_to \ change{ gitlab_group_1.has_master?(user) } end end @@ -272,7 +242,7 @@ objectclass: posixGroup end it "removes user from gitlab_group_1" do - expect { access.update_ldap_group_links(user) }.to \ + expect { access.update_ldap_group_links }.to \ change{ gitlab_group_1.members.where(user_id: user).any? }.from(true).to(false) end end @@ -338,13 +308,16 @@ objectclass: posixGroup Gitlab::LDAP::Group.new(ldap_group_response_2) ] end - let(:ldap_user) { Gitlab::LDAP::Person.new(Net::LDAP::Entry.new) } + let(:ldap_user) { Gitlab::LDAP::Person.new(Net::LDAP::Entry.new, user.provider) } - before { ldap_user.stub(:uid) { 'user42' } } + before do + access.stub(ldap_user: ldap_user) + ldap_user.stub(:uid) { 'user42' } + end it "only returns ldap cns to which the user has access" do access.stub(ldap_groups: ldap_groups) - expect(access.cns_with_access(ldap_user)).to eql ['group1'] + expect(access.cns_with_access).to eql ['group1'] end end end From e972ac691be7f600d21c2d563924c039335d4ef8 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Tue, 7 Oct 2014 17:27:56 +0200 Subject: [PATCH 22/51] Remove binding.pry --- lib/gitlab/ldap/user.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index 300c12339d..c8e40a6448 100644 --- a/lib/gitlab/ldap/user.rb +++ b/lib/gitlab/ldap/user.rb @@ -16,7 +16,6 @@ module Gitlab # Only check with valid login and password to prevent anonymous bind results return nil unless ldap_conf.enabled? && login.present? && password.present? - binding.pry ldap_user = adapter.bind_as( filter: user_filter(login), size: 1, From ee0154cd0c64f5582212ddfc6d44ad862a028101 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Tue, 7 Oct 2014 17:39:53 +0200 Subject: [PATCH 23/51] Add proper ssh sync attribute key for tests --- config/gitlab.yml.example | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index e14bc4907b..35ebc5b65f 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -330,6 +330,6 @@ test: user_filter: '' group_base: 'ou=groups,dc=example,dc=com' admin_group: '' - sync_ssh_keys + sync_ssh_keys: sshpublickey staging: <<: *base From f488fd598ef56d94e4a4f2438096604ecc421abb Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Wed, 8 Oct 2014 13:56:13 +0200 Subject: [PATCH 24/51] Add hint for refactoring --- spec/requests/api/ldap_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/requests/api/ldap_spec.rb b/spec/requests/api/ldap_spec.rb index 4d24fef143..8d98dc4d02 100644 --- a/spec/requests/api/ldap_spec.rb +++ b/spec/requests/api/ldap_spec.rb @@ -25,6 +25,7 @@ describe API::API do context "when authenticated as user" do it "should return an array of ldap groups" do + pending('Needs refactoring') get api("/ldap/groups", user) response.status.should == 200 json_response.should be_an Array From 212b406f4986a9c01a3a2b2a25d684ffb67fd3b1 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Wed, 8 Oct 2014 15:23:06 +0200 Subject: [PATCH 25/51] Rename ssh_sync_key to old convention to prevent confusion --- lib/gitlab/ldap/access.rb | 2 +- lib/gitlab/ldap/config.rb | 5 +++-- lib/gitlab/ldap/person.rb | 4 ++-- spec/lib/gitlab/ldap/access_spec.rb | 4 ++-- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index 0160430a56..d11ce648d4 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -79,7 +79,7 @@ module Gitlab (ldap_user.ssh_keys - user.keys.ldap.pluck(:key)).each do |key| Rails.logger.info "#{self.class.name}: adding LDAP SSH key #{key.inspect} to #{user.name} (#{user.id})" - new_key = LDAPKey.new(title: "LDAP - #{ldap_config.ssh_sync_key}", key: key) + new_key = LDAPKey.new(title: "LDAP - #{ldap_config.sync_ssh_keys}", key: key) new_key.user = user unless new_key.save Rails.logger.error "#{self.class.name}: failed to add LDAP SSH key #{key.inspect} to #{user.name} (#{user.id})\n"\ diff --git a/lib/gitlab/ldap/config.rb b/lib/gitlab/ldap/config.rb index 37316f41a9..c4324ac420 100644 --- a/lib/gitlab/ldap/config.rb +++ b/lib/gitlab/ldap/config.rb @@ -24,10 +24,11 @@ module Gitlab end def sync_ssh_keys? - ssh_sync_key.present? + sync_ssh_keys.present? end - def ssh_sync_key + # The LDAP attribute in which the ssh keys are stored + def sync_ssh_keys options['sync_ssh_keys'] end diff --git a/lib/gitlab/ldap/person.rb b/lib/gitlab/ldap/person.rb index 125673a5df..f4ee22476d 100644 --- a/lib/gitlab/ldap/person.rb +++ b/lib/gitlab/ldap/person.rb @@ -47,8 +47,8 @@ module Gitlab end def ssh_keys - if config.sync_ssh_keys? && entry.respond_to?(config.ssh_sync_key) - entry[config.ssh_sync_key.to_sym] + if config.sync_ssh_keys? && entry.respond_to?(config.sync_ssh_keys) + entry[config.sync_ssh_keys.to_sym] else [] end diff --git a/spec/lib/gitlab/ldap/access_spec.rb b/spec/lib/gitlab/ldap/access_spec.rb index 07e8ab4093..fe2841111a 100644 --- a/spec/lib/gitlab/ldap/access_spec.rb +++ b/spec/lib/gitlab/ldap/access_spec.rb @@ -70,7 +70,7 @@ describe Gitlab::LDAP::Access do Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com\n#{ssh_key_attribute_name}: #{ssh_key}") } before do - Gitlab::LDAP::Config.any_instance.stub(ssh_sync_key: ssh_key_attribute_name) + Gitlab::LDAP::Config.any_instance.stub(sync_ssh_keys: ssh_key_attribute_name) access.stub sync_ssh_keys?: true end @@ -87,7 +87,7 @@ describe Gitlab::LDAP::Access do access.update_ssh_keys expect(user.keys.last.title).to match(/LDAP/) - expect(user.keys.last.title).to match(/#{access.ldap_config.ssh_sync_key}/) + expect(user.keys.last.title).to match(/#{access.ldap_config.sync_ssh_keys}/) end it "should not add a SSH key if it is invalid" do From f8c8ae29a20627af5cffabf7fd5cd51484433652 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Wed, 8 Oct 2014 15:33:50 +0200 Subject: [PATCH 26/51] Use LDAP::Config in all of the adapter methods --- lib/gitlab/ldap/adapter.rb | 8 ++++---- lib/gitlab/ldap/config.rb | 8 ++++++++ spec/lib/gitlab/ldap/access_spec.rb | 2 +- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/lib/gitlab/ldap/adapter.rb b/lib/gitlab/ldap/adapter.rb index 1e58bd7f8e..3d1e05bd64 100644 --- a/lib/gitlab/ldap/adapter.rb +++ b/lib/gitlab/ldap/adapter.rb @@ -31,7 +31,7 @@ module Gitlab # def groups(cn = "*", size = nil) options = { - base: config['group_base'], + base: config.group_base, filter: Net::LDAP::Filter.eq("cn", cn) } @@ -54,13 +54,13 @@ module Gitlab } else options = { - base: config['base'], + base: config.base, filter: Net::LDAP::Filter.eq(field, value) } end - if config['user_filter'].present? - user_filter = Net::LDAP::Filter.construct(config['user_filter']) + if config.user_filter.present? + user_filter = Net::LDAP::Filter.construct(config.user_filter) options[:filter] = if options[:filter] Net::LDAP::Filter.join(options[:filter], user_filter) diff --git a/lib/gitlab/ldap/config.rb b/lib/gitlab/ldap/config.rb index c4324ac420..4c88d4bcaf 100644 --- a/lib/gitlab/ldap/config.rb +++ b/lib/gitlab/ldap/config.rb @@ -23,6 +23,14 @@ module Gitlab end end + def base + options['base'] + end + + def uid + options['uid'] + end + def sync_ssh_keys? sync_ssh_keys.present? end diff --git a/spec/lib/gitlab/ldap/access_spec.rb b/spec/lib/gitlab/ldap/access_spec.rb index fe2841111a..01af43514a 100644 --- a/spec/lib/gitlab/ldap/access_spec.rb +++ b/spec/lib/gitlab/ldap/access_spec.rb @@ -34,7 +34,7 @@ describe Gitlab::LDAP::Access do subject { access.update_permissions } it "syncs ssh keys if enabled by configuration" do - access.stub sync_ssh_keys?: true + access.stub sync_ssh_keys?: 'sshpublickey' expect(access).to receive(:update_ssh_keys).once subject From 6059e0683a8c776131f518a577744b4eda73530b Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Wed, 8 Oct 2014 16:03:11 +0200 Subject: [PATCH 27/51] Make ldap/groups API call backwards compatible It now just returns the results of the first server. A new call will replace the existing to get results for a specific provider --- config/gitlab.yml.example | 2 +- lib/api/ldap.rb | 5 ++++- lib/gitlab/ldap/config.rb | 8 ++++++++ 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index 35ebc5b65f..1dc6b5ae0d 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -330,6 +330,6 @@ test: user_filter: '' group_base: 'ou=groups,dc=example,dc=com' admin_group: '' - sync_ssh_keys: sshpublickey + sync_ssh_keys: false staging: <<: *base diff --git a/lib/api/ldap.rb b/lib/api/ldap.rb index 8e92622ecc..84d67ab4f9 100644 --- a/lib/api/ldap.rb +++ b/lib/api/ldap.rb @@ -10,7 +10,10 @@ module API # Example Request: # GET /ldap/groups get 'groups' do - @groups = Gitlab::LDAP::Adapter.new.groups("#{params[:search]}*", 20) + # NOTE: this should be deprecated in favour of /ldap/PROVIDER_NAME/groups + # for now we just select the first LDAP server + provider = Gitlab::LDAP::Config.servers.first.provider_name + @groups = Gitlab::LDAP::Adapter.new(provider).groups("#{params[:search]}*", 20) present @groups, with: Entities::LdapGroup end end diff --git a/lib/gitlab/ldap/config.rb b/lib/gitlab/ldap/config.rb index 4c88d4bcaf..bb9b03f370 100644 --- a/lib/gitlab/ldap/config.rb +++ b/lib/gitlab/ldap/config.rb @@ -4,6 +4,14 @@ module Gitlab class Config attr_accessor :provider, :options + def self.enabled? + Gitlab.config.ldap.enabled + end + + def servers + Gitlab.config.ldap.servers + end + def initialize(provider) @provider = provider @options = config_for(provider) From c0229e46a38d772397803301fe89a40ff85388bf Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Wed, 8 Oct 2014 16:07:49 +0200 Subject: [PATCH 28/51] Adapt ldap_user? to multiple ldap providers --- app/models/user.rb | 4 ++-- spec/models/user_spec.rb | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 5c0f1096d9..c786ee0405 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -176,7 +176,7 @@ class User < ActiveRecord::Base scope :not_in_team, ->(team){ where('users.id NOT IN (:ids)', ids: team.member_ids) } scope :not_in_project, ->(project) { project.users.present? ? where("id not in (:ids)", ids: project.users.map(&:id) ) : all } scope :without_projects, -> { where('id NOT IN (SELECT DISTINCT(user_id) FROM users_projects)') } - scope :ldap, -> { where(provider: 'ldap') } + scope :ldap, -> { where('provider LIKE ?', 'ldap%') } scope :subscribed_for_admin_email, -> { where(admin_email_unsubscribed_at: nil) } scope :potential_team_members, ->(team) { team.members.any? ? active.not_in_team(team) : active } @@ -397,7 +397,7 @@ class User < ActiveRecord::Base end def ldap_user? - extern_uid && provider == 'ldap' + extern_uid && provider.start_with?('ldap') end def accessible_deploy_keys diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 7221328a45..056b306b9e 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -346,6 +346,25 @@ describe User do end end + describe :ldap_user? do + let(:user) { build(:user, :ldap) } + + it "is true if provider name starts with ldap" do + user.provider = 'ldapmain' + expect( user.ldap_user? ).to be_true + end + + it "is false for other providers" do + user.provider = 'other-provider' + expect( user.ldap_user? ).to be_false + end + + it "is false if no extern_uid is provided" do + user.extern_uid = nil + expect( user.ldap_user? ).to be_false + end + end + describe '#full_website_url' do let(:user) { create(:user) } From ddb46f48f36c0c16e1a9f85fc8a1f30cba8fd673 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Wed, 8 Oct 2014 16:09:29 +0200 Subject: [PATCH 29/51] Explain auth logic some more --- lib/gitlab/auth.rb | 4 +++- spec/lib/gitlab/auth_spec.rb | 3 +-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 955abc1bed..4bc8227b0c 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -3,9 +3,11 @@ module Gitlab def find(login, password) user = User.find_by(email: login) || User.find_by(username: login) + # If no user is found, or it's an LDAP server, try LDAP. + # LDAP users are only authenticated via LDAP if user.nil? || user.ldap_user? # Second chance - try LDAP authentication - return nil unless ldap_conf.enabled + return nil unless Gitlab::LDAP::Config.enabled? Gitlab::LDAP::User.authenticate(login, password) else diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index 551fb3fb5f..1822e60fa9 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -28,8 +28,7 @@ describe Gitlab::Auth do end context "with ldap enabled" do - before { Gitlab.config.ldap['enabled'] = true } - after { Gitlab.config.ldap['enabled'] = false } + before { Gitlab::LDAP::Config.enabled.stub(enabled?: true) } it "tries to autheticate with db before ldap" do expect(Gitlab::LDAP::User).not_to receive(:authenticate) From 797ac2d60ffbce1506c27e460ed334667d326c64 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Thu, 9 Oct 2014 08:54:55 +0200 Subject: [PATCH 30/51] Make authentication backwards compatible with multiple LDAP servers Note: authentication agains an other server, other then the first still does not work. This will be completed in further MR's --- lib/gitlab/ldap/user.rb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index c8e40a6448..be9392a8d8 100644 --- a/lib/gitlab/ldap/user.rb +++ b/lib/gitlab/ldap/user.rb @@ -45,11 +45,16 @@ module Gitlab def find_by_uid(uid) # LDAP distinguished name is case-insensitive - model.where("provider = ? and lower(extern_uid) = ?", provider, uid.downcase).last + model. + where(provider: [provider, :ldap]). + where('lower(extern_uid) = ?', uid.downcase).last end def provider - 'ldap' + # Note: for backwards compatibility we just get the first provider + # Later on, we should loop through all servers until a successful + # authentication + Gitlab::LDAP::Config.servers.first.provider_name end end From 429f9dfac3347d4bf104046979e716d8f01bc014 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Thu, 9 Oct 2014 10:36:10 +0200 Subject: [PATCH 31/51] Removed duplicated `enabled` --- spec/lib/gitlab/auth_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index 1822e60fa9..206a92f837 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -28,7 +28,7 @@ describe Gitlab::Auth do end context "with ldap enabled" do - before { Gitlab::LDAP::Config.enabled.stub(enabled?: true) } + before { Gitlab::LDAP::Config.stub(enabled?: true) } it "tries to autheticate with db before ldap" do expect(Gitlab::LDAP::User).not_to receive(:authenticate) From d6cabd2732555d48f6d4cd1e8f742c43eda8f89d Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Thu, 9 Oct 2014 12:13:28 +0200 Subject: [PATCH 32/51] Add explanation about new gitlab.yml attributes --- config/gitlab.yml.example | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index 1dc6b5ae0d..3918141c90 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -144,8 +144,11 @@ production: &base password: '_the_password_of_the_bind_user' # When authenticating against an ldap server, this will provide a unique identifier - # Only use uniq numbers here - index: 1 + # Use one uniq word, no non-word charcters are allowed + provider_id: main + + # The UI use this to make a distinction between your LDAP servers + label: 'LDAP' # This setting controls the amount of time between LDAP permission checks for each user. # After this time has expired for a given user, their next interaction with GitLab (a click in the web UI, a git pull etc.) will be slower because the LDAP permission check is being performed. @@ -321,7 +324,6 @@ test: - provider_id: main label: ldap - primary: true host: 127.0.0.1 port: 3890 uid: 'uid' From 19d811e6bf3f4a6b490fedad716686ce0ecafc51 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Thu, 9 Oct 2014 18:57:56 +0200 Subject: [PATCH 33/51] Make LDAP authentication work with multiple LDAP servers --- lib/gitlab/ldap/authentication.rb | 68 +++++++++++++++++++++ lib/gitlab/ldap/config.rb | 6 +- lib/gitlab/ldap/user.rb | 44 +------------ spec/lib/gitlab/auth_spec.rb | 4 +- spec/lib/gitlab/ldap/authentication_spec.rb | 53 ++++++++++++++++ spec/lib/gitlab/ldap/user_spec.rb | 17 ------ 6 files changed, 130 insertions(+), 62 deletions(-) create mode 100644 lib/gitlab/ldap/authentication.rb create mode 100644 spec/lib/gitlab/ldap/authentication_spec.rb diff --git a/lib/gitlab/ldap/authentication.rb b/lib/gitlab/ldap/authentication.rb new file mode 100644 index 0000000000..0eca9b2613 --- /dev/null +++ b/lib/gitlab/ldap/authentication.rb @@ -0,0 +1,68 @@ +# This calls helps to authenticate to LDAP by providing username and password +# +# Since multiple LDAP servers are supported, it will loop through all of them +# until a valid bind is found +# + +module Gitlab + module LDAP + class Authentication + def self.login(login, password) + return unless Gitlab::LDAP::Config.enabled? + return unless login.present? && password.present? + + auth = nil + # loop through providers until valid bind + providers.find do |provider| + auth = new(provider) + auth.login(login, password) # true will exit the loop + end + + auth.user + end + + def self.providers + Gitlab::LDAP::Config.providers + end + + attr_accessor :provider, :ldap_user + + def initialize(provider) + @provider = provider + end + + def login(login, password) + @ldap_user = adapter.bind_as( + filter: user_filter(login), + size: 1, + password: password + ) + end + + def adapter + OmniAuth::LDAP::Adaptor.new(config.options) + end + + def config + Gitlab::LDAP::Config.new(provider) + end + + def user_filter(login) + Net::LDAP::Filter.eq(config.uid, login).tap do |filter| + # Apply LDAP user filter if present + if config.user_filter.present? + Net::LDAP::Filter.join( + filter, + Net::LDAP::Filter.construct(config.user_filter) + ) + end + end + end + + def user + return nil unless ldap_user + Gitlab::LDAP::User.find_by_uid_and_provider(ldap_user.dn, provider) + end + end + end +end \ No newline at end of file diff --git a/lib/gitlab/ldap/config.rb b/lib/gitlab/ldap/config.rb index bb9b03f370..ee09b43089 100644 --- a/lib/gitlab/ldap/config.rb +++ b/lib/gitlab/ldap/config.rb @@ -8,10 +8,14 @@ module Gitlab Gitlab.config.ldap.enabled end - def servers + def self.servers Gitlab.config.ldap.servers end + def self.providers + servers.map &:provider_name + end + def initialize(provider) @provider = provider @options = config_for(provider) diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index be9392a8d8..8326714888 100644 --- a/lib/gitlab/ldap/user.rb +++ b/lib/gitlab/ldap/user.rb @@ -10,52 +10,12 @@ module Gitlab module LDAP class User < Gitlab::OAuth::User class << self - # TODO: Look through LDAP servers until valid credentials are found? - 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 - return nil unless ldap_conf.enabled? && login.present? && password.present? - - ldap_user = adapter.bind_as( - filter: user_filter(login), - size: 1, - password: password - ) - - find_by_uid(ldap_user.dn) if ldap_user - end - - def adapter - @adapter ||= OmniAuth::LDAP::Adaptor.new(ldap_conf.options) - 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) - end - filter - end - - def ldap_conf - Gitlab::LDAP::Config.new(provider) - end - - def find_by_uid(uid) + def find_by_uid_and_provider(uid, provider) # LDAP distinguished name is case-insensitive - model. + ::User. where(provider: [provider, :ldap]). where('lower(extern_uid) = ?', uid.downcase).last end - - def provider - # Note: for backwards compatibility we just get the first provider - # Later on, we should loop through all servers until a successful - # authentication - Gitlab::LDAP::Config.servers.first.provider_name - end end def initialize(auth_hash) diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index 206a92f837..1f3e1a4a3c 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -31,13 +31,13 @@ describe Gitlab::Auth do before { Gitlab::LDAP::Config.stub(enabled?: true) } it "tries to autheticate with db before ldap" do - expect(Gitlab::LDAP::User).not_to receive(:authenticate) + expect(Gitlab::LDAP::Authentication).not_to receive(:login) gl_auth.find(username, password) end it "uses ldap as fallback to for authentication" do - expect(Gitlab::LDAP::User).to receive(:authenticate) + expect(Gitlab::LDAP::Authentication).to receive(:login) gl_auth.find('ldap_user', 'password') end diff --git a/spec/lib/gitlab/ldap/authentication_spec.rb b/spec/lib/gitlab/ldap/authentication_spec.rb new file mode 100644 index 0000000000..0eb7c443b8 --- /dev/null +++ b/spec/lib/gitlab/ldap/authentication_spec.rb @@ -0,0 +1,53 @@ +require 'spec_helper' + +describe Gitlab::LDAP::Authentication do + let(:klass) { Gitlab::LDAP::Authentication } + let(:user) { create(:user, :ldap, extern_uid: dn) } + let(:dn) { 'uid=john,ou=people,dc=example,dc=com' } + let(:login) { 'john' } + let(:password) { 'password' } + + describe :login do + let(:adapter) { double :adapter } + before do + Gitlab::LDAP::Config.stub(enabled?: true) + end + + it "finds the user if authentication is successful" do + user + # try only to fake the LDAP call + klass.any_instance.stub(adapter: double(:adapter, + bind_as: double(:ldap_user, dn: dn) + )) + expect(klass.login(login, password)).to be_true + end + + it "is false if the user does not exist" do + # try only to fake the LDAP call + klass.any_instance.stub(adapter: double(:adapter, + bind_as: double(:ldap_user, dn: dn) + )) + expect(klass.login(login, password)).to be_false + end + + it "is false if authentication fails" do + user + # try only to fake the LDAP call + klass.any_instance.stub(adapter: double(:adapter, bind_as: nil)) + expect(klass.login(login, password)).to be_false + end + + it "fails if ldap is disabled" do + Gitlab::LDAP::Config.stub(enabled?: false) + expect(klass.login(login, password)).to be_false + end + + it "fails if no login is supplied" do + expect(klass.login('', password)).to be_false + end + + it "fails if no password is supplied" do + expect(klass.login(login, '')).to be_false + end + end +end \ No newline at end of file diff --git a/spec/lib/gitlab/ldap/user_spec.rb b/spec/lib/gitlab/ldap/user_spec.rb index 9e0334adc6..726c9764e3 100644 --- a/spec/lib/gitlab/ldap/user_spec.rb +++ b/spec/lib/gitlab/ldap/user_spec.rb @@ -33,21 +33,4 @@ describe Gitlab::LDAP::User do expect{ gl_user.save }.to change{ User.count }.by(1) end end - - describe "authenticate" do - let(:login) { 'john' } - let(:password) { 'my-secret' } - - # before { - # Gitlab.config.ldap['enabled'] = true - # Gitlab.config.ldap['user_filter'] = 'employeeType=developer' - # } - # after { Gitlab.config.ldap['enabled'] = false } - - it "send an authentication request to ldap" do - pending('needs refactoring') - expect( Gitlab::LDAP::User.adapter ).to receive(:bind_as) - Gitlab::LDAP::User.authenticate(login, password) - end - end end From 5b3818686784304b1224bd1a620d9aafe5bbf30c Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 09:12:16 +0200 Subject: [PATCH 34/51] Add note for refactoring --- config/initializers/devise.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 530cf26af5..22d4bf68dd 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -205,6 +205,7 @@ Devise.setup do |config| # end if Gitlab.config.ldap.enabled + # TODO: make server specific if Gitlab.config.ldap.allow_username_or_email_login email_stripping_proc = ->(name) {name.gsub(/@.*$/,'')} else From c832c50c8702c710671cf2e8644393d2151926fa Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 09:36:54 +0200 Subject: [PATCH 35/51] Do not use primary flag for LDAP servers --- app/views/devise/sessions/new.html.haml | 8 ++++---- config/initializers/1_settings.rb | 1 - 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/app/views/devise/sessions/new.html.haml b/app/views/devise/sessions/new.html.haml index 328a2d8f04..04e998f8be 100644 --- a/app/views/devise/sessions/new.html.haml +++ b/app/views/devise/sessions/new.html.haml @@ -4,14 +4,14 @@ .login-body - if ldap_enabled? && gitlab_config.signin_enabled %ul.nav.nav-tabs - - @ldap_servers.each do |server| - %li{class: (:active if server['primary'])} + - @ldap_servers.each_with_index do |server, i| + %li{class: (:active if i==0)} = link_to server['label'], "#tab-#{server.provider_name}", 'data-toggle' => 'tab' %li = link_to 'Standard', '#tab-signin', 'data-toggle' => 'tab' .tab-content - - @ldap_servers.each do |server| - %div.tab-pane{id: "tab-#{server.provider_name}", class: (:active if server['primary'])} + - @ldap_servers.each_with_index do |server,i| + %div.tab-pane{id: "tab-#{server.provider_name}", class: (:active if i==0)} = render 'devise/sessions/new_ldap', provider: server.provider_name %div#tab-signin.tab-pane = render 'devise/sessions/new_base' diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 856afafb7e..4ac53c26ba 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -64,7 +64,6 @@ if Settings.ldap['enabled'] || Rails.env.test? if Settings.ldap['host'].present? excluded_per_server_settings = %w(sync_time allow_username_or_email_login) server = Settings.ldap.except(excluded_per_server_settings) - server['primary'] = true server['label'] = 'LDAP' server['provider_id'] = '' #providername will be ldap Settings.ldap['servers'] = [server] From 1835fceb4bc9d548ace7ab0683a6b1ffd014285a Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 09:40:31 +0200 Subject: [PATCH 36/51] Make allow_username_or_email_login server specific --- config/initializers/1_settings.rb | 3 +-- config/initializers/devise.rb | 13 ++++++------- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 4ac53c26ba..abab32265d 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -62,8 +62,7 @@ Settings.ldap['active_directory'] = true if Settings.ldap['active_directory'].ni # backwards compatibility, we only have one host if Settings.ldap['enabled'] || Rails.env.test? if Settings.ldap['host'].present? - excluded_per_server_settings = %w(sync_time allow_username_or_email_login) - server = Settings.ldap.except(excluded_per_server_settings) + server = Settings.ldap.except('sync_time') server['label'] = 'LDAP' server['provider_id'] = '' #providername will be ldap Settings.ldap['servers'] = [server] diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 22d4bf68dd..7770f018a1 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -205,14 +205,13 @@ Devise.setup do |config| # end if Gitlab.config.ldap.enabled - # TODO: make server specific - if Gitlab.config.ldap.allow_username_or_email_login - email_stripping_proc = ->(name) {name.gsub(/@.*$/,'')} - else - email_stripping_proc = ->(name) {name} - end - Gitlab.config.ldap.servers.each do |server| + if server['allow_username_or_email_login'] + email_stripping_proc = ->(name) {name.gsub(/@.*$/,'')} + else + email_stripping_proc = ->(name) {name} + end + config.omniauth server.provider_name, host: server['host'], base: server['base'], From 53816ef8f36d8e8920da1c198aa4e298797847fa Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 10:07:37 +0200 Subject: [PATCH 37/51] Make AD check server specific --- config/initializers/1_settings.rb | 4 ++-- lib/gitlab/ldap/access.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index abab32265d..76a87a5bf8 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -55,9 +55,7 @@ end # Default settings Settings['ldap'] ||= Settingslogic.new({}) Settings.ldap['enabled'] = false if Settings.ldap['enabled'].nil? -Settings.ldap['allow_username_or_email_login'] = false if Settings.ldap['allow_username_or_email_login'].nil? Settings.ldap['sync_time'] = 3600 if Settings.ldap['sync_time'].nil? -Settings.ldap['active_directory'] = true if Settings.ldap['active_directory'].nil? # backwards compatibility, we only have one host if Settings.ldap['enabled'] || Rails.env.test? @@ -69,6 +67,8 @@ if Settings.ldap['enabled'] || Rails.env.test? end Settings.ldap['servers'].each do |server| + server['allow_username_or_email_login'] = false if server['allow_username_or_email_login'].nil? + server['active_directory'] = true if server['active_directory'].nil? server['provider_name'] = "ldap#{server['provider_id']}".downcase server['provider_class'] = OmniAuth::Utils.camelize(server['provider_name']) end diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index ca1dcadfc5..191884e03f 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -36,7 +36,7 @@ module Gitlab def allowed? if Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter) - if Gitlab.config.ldap.active_directory + if ldap_config.active_directory !Gitlab::LDAP::Person.disabled_via_active_directory?(user.extern_uid, adapter) end else From adee309127374d84c6840b1c2fbb528b782467b8 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 10:10:39 +0200 Subject: [PATCH 38/51] Better description for new LDAP options --- config/gitlab.yml.example | 28 +++++++++++++++++++++------- lib/gitlab/ldap/person.rb | 4 ---- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index 6bcdecf0bc..272463121f 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -136,6 +136,27 @@ production: &base enabled: false servers: - + ## provider_id + # + # This identifier is used by GitLab to keep track of which LDAP server each + # GitLab user belongs to. Each LDAP server known to GitLab should have a unique + # provider_id. This identifier cannot be changed once users from the LDAP server + # have started logging in to GitLab. + # + # Format: one word, using a-z (lower case) and 0-9 + # Example: 'paris' or 'uswest2' + + provider_id: main + + ## label + # + # A human-friendly name for your LDAP server. It is OK to change the label later, + # for instance if you find out it is too large to fit on the web page. + # + # Example: 'Paris' or 'Acme, Ltd.' + + label: 'LDAP' + host: '_your_ldap_server' port: 636 uid: 'sAMAccountName' @@ -143,13 +164,6 @@ production: &base bind_dn: '_the_full_dn_of_the_user_you_will_bind_with' password: '_the_password_of_the_bind_user' - # When authenticating against an ldap server, this will provide a unique identifier - # Use one uniq word, no non-word charcters are allowed - provider_id: main - - # The UI use this to make a distinction between your LDAP servers - label: 'LDAP' - # This setting controls the amount of time between LDAP permission checks for each user. # After this time has expired for a given user, their next interaction with GitLab (a click in the web UI, a git pull etc.) will be slower because the LDAP permission check is being performed. # How much slower depends on your LDAP setup, but it is not uncommon for this check to add seconds of waiting time. diff --git a/lib/gitlab/ldap/person.rb b/lib/gitlab/ldap/person.rb index f4ee22476d..a35fd22073 100644 --- a/lib/gitlab/ldap/person.rb +++ b/lib/gitlab/ldap/person.rb @@ -60,10 +60,6 @@ module Gitlab @entry end - # def adapter - # @adapter ||= Gitlab::LDAP::Adapter.new - # end - def config @config ||= Gitlab::LDAP::Config.new(provider) end From 8271fac0a97ffbb6cbf5b8cf0ebf618f1ed1b6b7 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 10:39:46 +0200 Subject: [PATCH 39/51] Make AD check for access work properly --- lib/gitlab/ldap/access.rb | 5 ++--- lib/gitlab/ldap/config.rb | 4 ++++ spec/lib/gitlab/ldap/access_spec.rb | 18 ++++++------------ 3 files changed, 12 insertions(+), 15 deletions(-) diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index 191884e03f..e3d2cc065f 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -36,9 +36,8 @@ module Gitlab def allowed? if Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter) - if ldap_config.active_directory - !Gitlab::LDAP::Person.disabled_via_active_directory?(user.extern_uid, adapter) - end + return true unless ldap_config.active_directory + !Gitlab::LDAP::Person.disabled_via_active_directory?(user.extern_uid, adapter) else false end diff --git a/lib/gitlab/ldap/config.rb b/lib/gitlab/ldap/config.rb index bb9b03f370..d0dfbacc32 100644 --- a/lib/gitlab/ldap/config.rb +++ b/lib/gitlab/ldap/config.rb @@ -60,6 +60,10 @@ module Gitlab options['admin_group'] end + def active_directory + options['active_directory'] + end + protected def base_config Gitlab.config.ldap diff --git a/spec/lib/gitlab/ldap/access_spec.rb b/spec/lib/gitlab/ldap/access_spec.rb index 564f4c04a0..4656442a6f 100644 --- a/spec/lib/gitlab/ldap/access_spec.rb +++ b/spec/lib/gitlab/ldap/access_spec.rb @@ -28,19 +28,13 @@ describe Gitlab::LDAP::Access do it { should be_true } end - context 'and has no disabled flag in active diretory' do - before { - Gitlab::LDAP::Person.stub(disabled_via_active_directory?: false) - Gitlab.config.ldap['enabled'] = true - Gitlab.config.ldap['active_directory'] = false - } + context 'withoud ActiveDirectory enabled' do + before do + Gitlab::LDAP::Config.stub(enabled?: true) + Gitlab::LDAP::Config.any_instance.stub(active_directory: false) + end - after { - Gitlab.config.ldap['enabled'] = false - Gitlab.config.ldap['active_directory'] = true - } - - it { should be_false } + it { should be_true } end end end From faf54d525c01aa8d2d2dcd0790edb622a91a13d5 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 10:57:09 +0200 Subject: [PATCH 40/51] Name the default provider main, to avoid name collisions --- config/initializers/1_settings.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 76a87a5bf8..8961246ac7 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -62,7 +62,7 @@ if Settings.ldap['enabled'] || Rails.env.test? if Settings.ldap['host'].present? server = Settings.ldap.except('sync_time') server['label'] = 'LDAP' - server['provider_id'] = '' #providername will be ldap + server['provider_id'] = 'main' Settings.ldap['servers'] = [server] end From 824aeacdd7a85d9c5992609e524b36b311a929e5 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 11:25:35 +0200 Subject: [PATCH 41/51] Only allow valid LDAP::Config instances. When trying to instantiate a new, but unknown provider, guidance is provided to get the correct provider RuntimeError: Unknown provider (henk). Available providers: ["ldapmain"] --- lib/gitlab/ldap/config.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/ldap/config.rb b/lib/gitlab/ldap/config.rb index d0dfbacc32..af0d743f4d 100644 --- a/lib/gitlab/ldap/config.rb +++ b/lib/gitlab/ldap/config.rb @@ -8,11 +8,16 @@ module Gitlab Gitlab.config.ldap.enabled end - def servers + def self.servers Gitlab.config.ldap.servers end + def self.providers + servers.map &:provider_name + end + def initialize(provider) + raise "Unknown provider (#{provider}). Available providers: #{self.class.providers}" @provider = provider @options = config_for(provider) end From 74e79105cde3b9470dc83f8705fa73e452aab75f Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 11:31:33 +0200 Subject: [PATCH 42/51] Cleanup provider validation --- lib/gitlab/ldap/config.rb | 10 +++++++++- spec/lib/gitlab/ldap/config_spec.rb | 4 ++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/ldap/config.rb b/lib/gitlab/ldap/config.rb index af0d743f4d..697b66dcda 100644 --- a/lib/gitlab/ldap/config.rb +++ b/lib/gitlab/ldap/config.rb @@ -17,8 +17,8 @@ module Gitlab end def initialize(provider) - raise "Unknown provider (#{provider}). Available providers: #{self.class.providers}" @provider = provider + invalid_provider unless valid_provider? @options = config_for(provider) end @@ -89,6 +89,14 @@ module Gitlab end end + def valid_provider? + self.class.providers.include?(provider) + end + + def invalid_provider + raise "Unknown provider (#{provider}). Available providers: #{self.class.providers}" + end + def auth_options { auth: { diff --git a/spec/lib/gitlab/ldap/config_spec.rb b/spec/lib/gitlab/ldap/config_spec.rb index a01166b264..76cc7f95c4 100644 --- a/spec/lib/gitlab/ldap/config_spec.rb +++ b/spec/lib/gitlab/ldap/config_spec.rb @@ -12,5 +12,9 @@ describe Gitlab::LDAP::Config do it "works" do expect(config).to be_a described_class end + + it "raises an error if a unknow provider is used" do + expect{ Gitlab::LDAP::Config.new 'unknown' }.to raise_error + end end end \ No newline at end of file From 049ae5b331b7a09e23980404aa006cf21f8d8081 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 11:42:49 +0200 Subject: [PATCH 43/51] Clearly distance ourselfs from AR methods --- lib/gitlab/ldap/user.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index be9392a8d8..ba0f5844eb 100644 --- a/lib/gitlab/ldap/user.rb +++ b/lib/gitlab/ldap/user.rb @@ -60,7 +60,7 @@ module Gitlab def initialize(auth_hash) super - update_attributes + update_user_attributes end # instance methods @@ -79,7 +79,7 @@ module Gitlab model.find_by(email: auth_hash.email) end - def update_attributes + def update_user_attributes gl_user.attributes = { extern_uid: auth_hash.uid, provider: auth_hash.provider, From 492398a6c6c86d84644df86e5b6bd2edf6c693a5 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 11:42:49 +0200 Subject: [PATCH 44/51] Clearly distance ourselfs from AR methods --- lib/gitlab/ldap/user.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index c8e5d11447..006ef17072 100644 --- a/lib/gitlab/ldap/user.rb +++ b/lib/gitlab/ldap/user.rb @@ -54,7 +54,7 @@ module Gitlab def initialize(auth_hash) super - update_attributes + update_user_attributes end # instance methods @@ -73,7 +73,7 @@ module Gitlab model.find_by(email: auth_hash.email) end - def update_attributes + def update_user_attributes gl_user.attributes = { extern_uid: auth_hash.uid, provider: auth_hash.provider, From 9a1573b2dc2ed443638a5115542baa71b42e21d7 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 11:56:20 +0200 Subject: [PATCH 45/51] More usefull errors if saving fails --- lib/gitlab/oauth/user.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/oauth/user.rb b/lib/gitlab/oauth/user.rb index 8231886144..699258baee 100644 --- a/lib/gitlab/oauth/user.rb +++ b/lib/gitlab/oauth/user.rb @@ -31,7 +31,7 @@ module Gitlab gl_user rescue ActiveRecord::RecordInvalid => e - log.info "(OAuth) Email #{e.record.errors[:email]}. Username #{e.record.errors[:username]}" + log.info "(OAuth) Error saving user: #{gl_user.errors.full_messages}" return self, e.record.errors end From 70efa16a0c5dc4f031075ffaaec682f4f69fcb44 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 12:00:37 +0200 Subject: [PATCH 46/51] More specific check for generated email --- spec/lib/gitlab/oauth/auth_hash_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/lib/gitlab/oauth/auth_hash_spec.rb b/spec/lib/gitlab/oauth/auth_hash_spec.rb index 68b87dbd4b..5eb77b492b 100644 --- a/spec/lib/gitlab/oauth/auth_hash_spec.rb +++ b/spec/lib/gitlab/oauth/auth_hash_spec.rb @@ -33,7 +33,7 @@ describe Gitlab::OAuth::AuthHash do context "email not provided" do before { info_hash.delete(:email) } it "generates a temp email" do - expect( auth_hash.email).to_not be_empty + expect( auth_hash.email).to start_with('temp-email-for-oauth') end end From 757d9aacb5318878e169cbf0ce3bf035ff714b01 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 15:02:31 +0200 Subject: [PATCH 47/51] Call the correct authentication method --- lib/gitlab/auth.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 4bc8227b0c..f97c0247b6 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -9,7 +9,7 @@ module Gitlab # Second chance - try LDAP authentication return nil unless Gitlab::LDAP::Config.enabled? - Gitlab::LDAP::User.authenticate(login, password) + Gitlab::LDAP::Authentication.login(login, password) else user if user.valid_password?(password) end From 948a7a12b5fbe4ce70cf74911ac73c65f27fd5a1 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Mon, 13 Oct 2014 10:38:02 +0200 Subject: [PATCH 48/51] Don't change the providername without migrating --- config/initializers/1_settings.rb | 2 +- config/initializers/7_omniauth.rb | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 8961246ac7..58ff3e4996 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -62,7 +62,7 @@ if Settings.ldap['enabled'] || Rails.env.test? if Settings.ldap['host'].present? server = Settings.ldap.except('sync_time') server['label'] = 'LDAP' - server['provider_id'] = 'main' + server['provider_id'] = '' Settings.ldap['servers'] = [server] end diff --git a/config/initializers/7_omniauth.rb b/config/initializers/7_omniauth.rb index 4477597016..90b6ff579b 100644 --- a/config/initializers/7_omniauth.rb +++ b/config/initializers/7_omniauth.rb @@ -1,5 +1,6 @@ module OmniAuth::Strategies Gitlab.config.ldap.servers.each_with_index do |server| + next unless server.provider_id.present? const_set(server.provider_class, Class.new(LDAP)) end end From 103419be96f7bc9d8314a789d117b9516f33f2af Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Tue, 14 Oct 2014 17:26:42 +0200 Subject: [PATCH 49/51] Add ldap callback definitions to initializer --- app/controllers/omniauth_callbacks_controller.rb | 4 ---- config/initializers/7_omniauth.rb | 6 ++++++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 0f364a48ea..f46b36568f 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -32,10 +32,6 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController end end - Gitlab.config.ldap.servers.each do |server| - alias_method server.provider_name, :ldap - end - def omniauth_error @provider = params[:provider] @error = params[:error] diff --git a/config/initializers/7_omniauth.rb b/config/initializers/7_omniauth.rb index 90b6ff579b..ef375cc8bc 100644 --- a/config/initializers/7_omniauth.rb +++ b/config/initializers/7_omniauth.rb @@ -4,3 +4,9 @@ module OmniAuth::Strategies const_set(server.provider_class, Class.new(LDAP)) end end + +OmniauthCallbacksController.class_eval do + Gitlab.config.ldap.servers.each do |server| + alias_method server.provider_name, :ldap + end +end From b01e4a1c905087b65499ca370cc806bc7a7c05b6 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Tue, 14 Oct 2014 17:27:13 +0200 Subject: [PATCH 50/51] Move method to match with CE --- lib/gitlab/ldap/access.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index e3d2cc065f..ac0e7ec8cf 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -49,6 +49,10 @@ module Gitlab @adapter ||= Gitlab::LDAP::Adapter.new(provider) end + def ldap_config + Gitlab::LDAP::Config.new(provider) + end + def ldap_user @ldap_user ||= Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter) end @@ -145,10 +149,6 @@ module Gitlab end.map(&:cn) end - def ldap_config - Gitlab::LDAP::Config.new(provider) - end - def sync_ssh_keys? ldap_config.sync_ssh_keys? end From 435b6f9135db61440ebf9bd54e741ebd9a385fdb Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Wed, 15 Oct 2014 14:25:06 +0200 Subject: [PATCH 51/51] Remove pending status from test --- spec/requests/api/ldap_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/requests/api/ldap_spec.rb b/spec/requests/api/ldap_spec.rb index 8d98dc4d02..4d24fef143 100644 --- a/spec/requests/api/ldap_spec.rb +++ b/spec/requests/api/ldap_spec.rb @@ -25,7 +25,6 @@ describe API::API do context "when authenticated as user" do it "should return an array of ldap groups" do - pending('Needs refactoring') get api("/ldap/groups", user) response.status.should == 200 json_response.should be_an Array