diff options
| -rw-r--r-- | app/models/user.rb | 4 | ||||
| -rw-r--r-- | lib/gitlab/ldap/user.rb | 74 | ||||
| -rw-r--r-- | lib/gitlab/oauth/auth_hash.rb | 54 | ||||
| -rw-r--r-- | lib/gitlab/oauth/user.rb | 134 | ||||
| -rw-r--r-- | spec/lib/gitlab/auth_spec.rb | 41 | ||||
| -rw-r--r-- | spec/lib/gitlab/ldap/user_spec.rb | 24 |
6 files changed, 190 insertions, 141 deletions
diff --git a/app/models/user.rb b/app/models/user.rb index f1ff76edd15..15e56a62a68 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -474,10 +474,6 @@ class User < ActiveRecord::Base email =~ /\Atemp-email-for-oauth/ end - def generate_tmp_oauth_email - self.email = "temp-email-for-oauth-#{username}@gitlab.localhost" - end - def public_profile? authorized_projects.public_only.any? end diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index e6aa3890992..25b5a702f9a 100644 --- a/lib/gitlab/ldap/user.rb +++ b/lib/gitlab/ldap/user.rb @@ -10,37 +10,20 @@ module Gitlab module LDAP class User < Gitlab::OAuth::User class << self - def find_or_create(auth) - @auth = auth - - if uid.blank? || email.blank? || username.blank? - raise_error("Account must provide a dn, uid and email address") - end - - user = find(auth) + 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 - unless user - # Look for user with same emails - # - # Possible cases: - # * When user already has account and need to link their LDAP account. - # * LDAP uid changed for user with same email and we need to update their uid - # - user = model.find_by(email: email) + 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: uid, provider: provider) - log.info("(LDAP) Updating legacy LDAP user #{email} with extern_uid => #{uid}") - else - # Create a new user inside GitLab database - # based on LDAP credentials - # - # - user = create(auth) - end + 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 - - user end def authenticate(login, password) @@ -48,17 +31,8 @@ module Gitlab # Only check with valid login and password to prevent anonymous bind results return nil unless ldap_conf.enabled && login.present? && password.present? - ldap = OmniAuth::LDAP::Adaptor.new(ldap_conf) - filter = Net::LDAP::Filter.eq(ldap.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 - - ldap_user = ldap.bind_as( - filter: filter, + ldap_user = adapter.bind_as( + filter: user_filter(login), size: 1, password: password ) @@ -66,10 +40,14 @@ module Gitlab find_by_uid(ldap_user.dn) if ldap_user end - private + def adapter + @adapter ||= OmniAuth::LDAP::Adaptor.new(ldap_conf) + end + + protected def find_by_uid_and_provider - find_by_uid(uid) + find_by_uid(auth_hash.uid) end def find_by_uid(uid) @@ -88,6 +66,20 @@ module Gitlab def ldap_conf Gitlab.config.ldap 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 + end + + def needs_blocking? + false end end end diff --git a/lib/gitlab/oauth/auth_hash.rb b/lib/gitlab/oauth/auth_hash.rb new file mode 100644 index 00000000000..0198f61f427 --- /dev/null +++ b/lib/gitlab/oauth/auth_hash.rb @@ -0,0 +1,54 @@ +# Class to parse and transform the info provided by omniauth +# +module Gitlab + module OAuth + class AuthHash + attr_reader :auth_hash + def initialize(auth_hash) + @auth_hash = auth_hash + end + + def uid + auth_hash.uid.to_s + end + + def provider + auth_hash.provider + end + + def info + auth_hash.info + end + + def name + (info.name || full_name).to_s.force_encoding('utf-8') + end + + def full_name + "#{info.first_name} #{info.last_name}" + end + + def username + (info.try(:nickname) || generate_username).to_s.force_encoding('utf-8') + end + + def email + (info.try(:email) || generate_temporarily_email).downcase + end + + def password + @password ||= Devise.friendly_token[0, 8].downcase + end + + # Get the first part of the email address (before @) + # In addtion in removes illegal characters + def generate_username + email.match(/^[^@]*/)[0].parameterize + end + + def generate_temporarily_email + "temp-email-for-oauth-#{username}@gitlab.localhost" + end + end + end +end diff --git a/lib/gitlab/oauth/user.rb b/lib/gitlab/oauth/user.rb index 9670aad2c5d..b768eda185f 100644 --- a/lib/gitlab/oauth/user.rb +++ b/lib/gitlab/oauth/user.rb @@ -7,106 +7,78 @@ module Gitlab module OAuth class User class << self - attr_reader :auth + attr_reader :auth_hash - def find(auth) - @auth = auth + def find(auth_hash) + self.auth_hash = auth_hash find_by_uid_and_provider end - def create(auth) - @auth = auth - password = Devise.friendly_token[0, 8].downcase - opts = { - extern_uid: uid, - provider: provider, - name: name, - username: username, - email: email, - password: password, - password_confirmation: password, - } - - user = model.build_user(opts) - user.skip_confirmation! - - # Services like twitter and github does not return email via oauth - # In this case we generate temporary email and force user to fill it later - if user.email.blank? - user.generate_tmp_oauth_email - elsif provider != "ldap" - # Google oauth returns email but dont return nickname - # So we use part of email as username for new user - # For LDAP, username is already set to the user's - # uid/userid/sAMAccountName. - email_username = email.match(/^[^@]*/)[0] - # Strip apostrophes since they are disallowed as part of username - user.username = email_username.gsub("'", "") - end - - begin - user.save! - rescue ActiveRecord::RecordInvalid => e - log.info "(OAuth) Email #{e.record.errors[:email]}. Username #{e.record.errors[:username]}" - return nil, e.record.errors - end - - log.info "(OAuth) Creating user #{email} from login with extern_uid => #{uid}" - - if Gitlab.config.omniauth['block_auto_created_users'] && !ldap? - user.block - end + def create(auth_hash) + user = new(auth_hash) + user.save_and_trigger_callbacks + end - user + def model + ::User end - private + def auth_hash=(auth_hash) + @auth_hash = AuthHash.new(auth_hash) + end + protected def find_by_uid_and_provider - model.where(provider: provider, extern_uid: uid).last + model.where(provider: auth_hash.provider, extern_uid: auth_hash.uid).last end + end - def uid - auth.uid.to_s - end + # Instance methods + attr_accessor :auth_hash, :user - def email - return unless auth.info.respond_to?(:email) - auth.info.email.downcase unless auth.info.email.nil? - end + def initialize(auth_hash) + self.auth_hash = auth_hash + self.user = self.class.model.new(user_attributes) + user.skip_confirmation! + end - def name - if auth.info.name.nil? - "#{auth.info.first_name} #{auth.info.last_name}".force_encoding('utf-8') - else - auth.info.name.to_s.force_encoding('utf-8') - end - end + def auth_hash=(auth_hash) + @auth_hash = AuthHash.new(auth_hash) + end - def username - return unless auth.info.respond_to?(:nickname) - auth.info.nickname.to_s.force_encoding("utf-8") - 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 provider - auth.provider - end + user + rescue ActiveRecord::RecordInvalid => e + log.info "(OAuth) Email #{e.record.errors[:email]}. Username #{e.record.errors[:username]}" + return nil, e.record.errors + end - def log - Gitlab::AppLogger - end + def user_attributes + { + extern_uid: auth_hash.uid, + provider: auth_hash.provider, + name: auth_hash.name, + username: auth_hash.username, + email: auth_hash.email, + password: auth_hash.password, + password_confirmation: auth_hash.password, + } + end - def model - ::User - end + def log + Gitlab::AppLogger + end - def raise_error(message) - raise OmniAuth::Error, "(OAuth) " + message - end + def raise_error(message) + raise OmniAuth::Error, "(OAuth) " + message + end - def ldap? - provider == 'ldap' - end + def needs_blocking? + Gitlab.config.omniauth['block_auto_created_users'] end end end diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index 073b811c3fb..551fb3fb5f6 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -4,25 +4,44 @@ describe Gitlab::Auth do let(:gl_auth) { Gitlab::Auth.new } describe :find do - before do - @user = create( - :user, - username: 'john', - password: '88877711', - password_confirmation: '88877711' - ) + let!(:user) do + create(:user, + username: username, + password: password, + password_confirmation: password) end + let(:username) { 'john' } + let(:password) { 'my-secret' } it "should find user by valid login/password" do - gl_auth.find('john', '88877711').should == @user + expect( gl_auth.find(username, password) ).to eql user end it "should not find user with invalid password" do - gl_auth.find('john', 'invalid11').should_not == @user + password = 'wrong' + expect( gl_auth.find(username, password) ).to_not eql user end - it "should not find user with invalid login and password" do - gl_auth.find('jon', 'invalid11').should_not == @user + it "should not find user with invalid login" do + user = 'wrong' + expect( gl_auth.find(username, password) ).to_not eql user + end + + context "with ldap enabled" do + before { Gitlab.config.ldap['enabled'] = true } + after { Gitlab.config.ldap['enabled'] = false } + + it "tries to autheticate with db before ldap" do + expect(Gitlab::LDAP::User).not_to receive(:authenticate) + + gl_auth.find(username, password) + end + + it "uses ldap as fallback to for authentication" do + expect(Gitlab::LDAP::User).to receive(:authenticate) + + gl_auth.find('ldap_user', 'password') + end end end end diff --git a/spec/lib/gitlab/ldap/user_spec.rb b/spec/lib/gitlab/ldap/user_spec.rb index 725338965be..d232cb20759 100644 --- a/spec/lib/gitlab/ldap/user_spec.rb +++ b/spec/lib/gitlab/ldap/user_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Gitlab::LDAP::User do - let(:gl_auth) { Gitlab::LDAP::User } + let(:gl_user) { Gitlab::LDAP::User } let(:info) do double( name: 'John', @@ -19,12 +19,12 @@ describe Gitlab::LDAP::User do it "finds the user if already existing" do existing_user = create(:user, extern_uid: 'my-uid', provider: 'ldap') - expect{ gl_auth.find_or_create(auth) }.to_not change{ User.count } + expect{ gl_user.find_or_create(auth) }.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_auth.find_or_create(auth) }.to_not change{ User.count } + expect{ gl_user.find_or_create(auth) }.to_not change{ User.count } existing_user.reload expect(existing_user.extern_uid).to eql 'my-uid' @@ -32,7 +32,23 @@ describe Gitlab::LDAP::User do end it "creates a new user if not found" do - expect{ gl_auth.find_or_create(auth) }.to change{ User.count }.by(1) + expect{ gl_user.find_or_create(auth) }.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 + expect( Gitlab::LDAP::User.adapter ).to receive(:bind_as) + Gitlab::LDAP::User.authenticate(login, password) end end end |