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] 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