diff options
| -rw-r--r-- | app/controllers/omniauth_callbacks_controller.rb | 35 | ||||
| -rw-r--r-- | lib/gitlab/ldap/user.rb | 75 | ||||
| -rw-r--r-- | lib/gitlab/oauth/auth_hash.rb | 2 | ||||
| -rw-r--r-- | lib/gitlab/oauth/user.rb | 71 | ||||
| -rw-r--r-- | spec/lib/gitlab/ldap/user_spec.rb | 20 | ||||
| -rw-r--r-- | spec/lib/gitlab/oauth/auth_hash_spec.rb | 55 | ||||
| -rw-r--r-- | spec/lib/gitlab/oauth/user_spec.rb | 88 |
7 files changed, 187 insertions, 159 deletions
diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 3ed6a69c2d8..fa5685938f0 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 25b5a702f9a..006ef170726 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_user_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_user_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/auth_hash.rb b/lib/gitlab/oauth/auth_hash.rb index 0198f61f427..ce52beec78e 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/lib/gitlab/oauth/user.rb b/lib/gitlab/oauth/user.rb index b768eda185f..699258baee4 100644 --- a/lib/gitlab/oauth/user.rb +++ b/lib/gitlab/oauth/user.rb @@ -6,55 +6,52 @@ module Gitlab module OAuth class User - class << self - attr_reader :auth_hash + attr_accessor :auth_hash, :gl_user - 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 initialize(auth_hash) + self.auth_hash = auth_hash + end - def model - ::User - end + def persisted? + gl_user.persisted? + end - def auth_hash=(auth_hash) - @auth_hash = AuthHash.new(auth_hash) - end + def new? + !gl_user.persisted? + end - protected - def find_by_uid_and_provider - model.where(provider: auth_hash.provider, extern_uid: auth_hash.uid).last - end + def valid? + gl_user.valid? end - # Instance methods - attr_accessor :auth_hash, :user + def save + gl_user.save! + log.info "(OAuth) saving user #{auth_hash.email} from login with extern_uid => #{auth_hash.uid}" + gl_user.block if needs_blocking? - def initialize(auth_hash) - self.auth_hash = auth_hash - self.user = self.class.model.new(user_attributes) - user.skip_confirmation! + gl_user + rescue ActiveRecord::RecordInvalid => e + log.info "(OAuth) Error saving user: #{gl_user.errors.full_messages}" + return self, e.record.errors + end + + def gl_user + @user ||= find_by_uid_and_provider || build_new_user end + protected 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 find_by_uid_and_provider + model.where(provider: auth_hash.provider, extern_uid: auth_hash.uid).last + end - user - rescue ActiveRecord::RecordInvalid => e - log.info "(OAuth) Email #{e.record.errors[:email]}. Username #{e.record.errors[:username]}" - return nil, e.record.errors + def build_new_user + model.new(user_attributes).tap do |user| + user.skip_confirmation! + end end def user_attributes @@ -80,6 +77,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/ldap/user_spec.rb b/spec/lib/gitlab/ldap/user_spec.rb index d232cb20759..a1aec0bb96f 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 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 00000000000..5eb77b492b2 --- /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 start_with('temp-email-for-oauth') + 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 diff --git a/spec/lib/gitlab/oauth/user_spec.rb b/spec/lib/gitlab/oauth/user_spec.rb index c241e198609..e4e96fd9f49 100644 --- a/spec/lib/gitlab/oauth/user_spec.rb +++ b/spec/lib/gitlab/oauth/user_spec.rb @@ -1,83 +1,55 @@ 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) - - user.should be_valid - user.extern_uid.should == auth.uid - user.provider.should == 'ldap' - end - - it "should create user from Omniauth" do - auth = double(info: info, uid: 'my-uid', provider: 'twitter') - 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 == 'twitter' + 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 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 + context "twitter" do + let(:provider) { 'twitter' } - 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') + it "creates a user from Omniauth" do + oauth_user.save - 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 |