diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index 62709a12942..c054b6f5865 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -28,7 +28,7 @@ module Gitlab def allowed?(user) if Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter) - !Gitlab::LDAP::Person.active_directory_disabled?(user.extern_uid, adapter) + !Gitlab::LDAP::Person.disabled_via_active_directory?(user.extern_uid, adapter) else false end diff --git a/lib/gitlab/ldap/person.rb b/lib/gitlab/ldap/person.rb index 9ad6618bd46..87c3d711db4 100644 --- a/lib/gitlab/ldap/person.rb +++ b/lib/gitlab/ldap/person.rb @@ -16,7 +16,7 @@ module Gitlab adapter.user('dn', dn) end - def self.active_directory_disabled?(dn, adapter=nil) + def self.disabled_via_active_directory?(dn, adapter=nil) adapter ||= Gitlab::LDAP::Adapter.new adapter.dn_matches_filter?(dn, AD_USER_DISABLED) end diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index 79aa145d871..e6aa3890992 100644 --- a/lib/gitlab/ldap/user.rb +++ b/lib/gitlab/ldap/user.rb @@ -77,10 +77,6 @@ module Gitlab model.where("provider = ? and lower(extern_uid) = ?", provider, uid.downcase).last end - def username - auth.info.nickname.to_s.force_encoding("utf-8") - end - def provider 'ldap' end diff --git a/lib/gitlab/oauth/user.rb b/lib/gitlab/oauth/user.rb index a252895493b..9670aad2c5d 100644 --- a/lib/gitlab/oauth/user.rb +++ b/lib/gitlab/oauth/user.rb @@ -67,9 +67,7 @@ module Gitlab end def uid - uid = auth.info.uid || auth.uid - uid = uid.to_s unless uid.nil? - uid + auth.uid.to_s end def email @@ -86,6 +84,7 @@ module Gitlab end def username + return unless auth.info.respond_to?(:nickname) auth.info.nickname.to_s.force_encoding("utf-8") end diff --git a/spec/lib/gitlab/ldap/access_spec.rb b/spec/lib/gitlab/ldap/access_spec.rb index d8c107502ba..2307a03f656 100644 --- a/spec/lib/gitlab/ldap/access_spec.rb +++ b/spec/lib/gitlab/ldap/access_spec.rb @@ -16,14 +16,14 @@ describe Gitlab::LDAP::Access do context 'when the user is found' do before { Gitlab::LDAP::Person.stub(find_by_dn: :ldap_user) } - context 'and the Active Directory disabled flag is set' do - before { Gitlab::LDAP::Person.stub(active_directory_disabled?: true) } + context 'and the user is diabled via active directory' do + before { Gitlab::LDAP::Person.stub(disabled_via_active_directory?: true) } it { should be_false } end - context 'and the Active Directory disabled flag is not set' do - before { Gitlab::LDAP::Person.stub(active_directory_disabled?: false) } + context 'and has no disabled flag in active diretory' do + before { Gitlab::LDAP::Person.stub(disabled_via_active_directory?: false) } it { should be_true } end diff --git a/spec/lib/gitlab/ldap/user_spec.rb b/spec/lib/gitlab/ldap/user_spec.rb index de5717417f1..725338965be 100644 --- a/spec/lib/gitlab/ldap/user_spec.rb +++ b/spec/lib/gitlab/ldap/user_spec.rb @@ -2,45 +2,37 @@ require 'spec_helper' describe Gitlab::LDAP::User do let(:gl_auth) { Gitlab::LDAP::User } - - before do - Gitlab.config.stub(omniauth: {}) - - @info = double( - uid: '12djsak321', + let(:info) do + double( name: 'John', - email: 'john@mail.com', + email: 'john@example.com', nickname: 'john' ) end + before { Gitlab.config.stub(omniauth: {}) } - describe :find_for_ldap_auth do - before do - @auth = double( - uid: '12djsak321', - info: @info, - provider: 'ldap' - ) + describe :find_or_create do + let(:auth) do + double(info: info, provider: 'ldap', uid: 'my-uid') end - it "should update credentials by email if missing uid" do - user = double('User') - User.stub find_by_extern_uid_and_provider: nil - User.stub(:find_by).with(hash_including(email: anything())) { user } - user.should_receive :update_attributes - gl_auth.find_or_create(@auth) + 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 } end - it "should not update credentials by username if missing uid and Gitlab.config.ldap.allow_username_or_email_login is false" do - user = double('User') - value = Gitlab.config.ldap.allow_username_or_email_login - Gitlab.config.ldap['allow_username_or_email_login'] = false - User.stub find_by_extern_uid_and_provider: nil - User.stub(:find_by).with(hash_including(email: anything())) { nil } - User.stub(:find_by).with(hash_including(username: anything())) { user } - user.should_not_receive :update_attributes - gl_auth.find_or_create(@auth) - Gitlab.config.ldap['allow_username_or_email_login'] = value + 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 } + + existing_user.reload + expect(existing_user.extern_uid).to eql 'my-uid' + expect(existing_user.provider).to eql 'ldap' + end + + it "creates a new user if not found" do + expect{ gl_auth.find_or_create(auth) }.to change{ User.count }.by(1) end end end diff --git a/spec/lib/gitlab/oauth/user_spec.rb b/spec/lib/gitlab/oauth/user_spec.rb index 60d9c4f8a9b..c241e198609 100644 --- a/spec/lib/gitlab/oauth/user_spec.rb +++ b/spec/lib/gitlab/oauth/user_spec.rb @@ -2,40 +2,54 @@ require 'spec_helper' describe Gitlab::OAuth::User do let(:gl_auth) { Gitlab::OAuth::User } - - before do - Gitlab.config.stub(omniauth: {}) - - @info = double( - uid: '12djsak321', + let(:info) do + double( nickname: 'john', name: 'John', email: 'john@mail.com' ) end + before do + Gitlab.config.stub(omniauth: {}) + end + + describe :find 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) + 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) + end + end + describe :create do it "should create user from LDAP" do - @auth = double(info: @info, provider: 'ldap') - user = gl_auth.create(@auth) + auth = double(info: info, uid: 'my-uid', provider: 'ldap') + user = gl_auth.create(auth) user.should be_valid - user.extern_uid.should == @info.uid + user.extern_uid.should == auth.uid user.provider.should == 'ldap' end it "should create user from Omniauth" do - @auth = double(info: @info, provider: 'twitter') - user = gl_auth.create(@auth) + auth = double(info: info, uid: 'my-uid', provider: 'twitter') + user = gl_auth.create(auth) user.should be_valid - user.extern_uid.should == @info.uid + user.extern_uid.should == auth.uid user.provider.should == 'twitter' end it "should apply defaults to user" do - @auth = double(info: @info, provider: 'ldap') - user = gl_auth.create(@auth) + 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 @@ -48,10 +62,22 @@ describe Gitlab::OAuth::User do nickname: 'john', name: 'John' ) - auth = double(info: info, provider: 'my-provider') + 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' + end end end