Welcome to mirror list, hosted at ThFree Co, Russian Federation.

gitlab.com/gitlab-org/gitlab-foss.git - Unnamed repository; edit this file 'description' to name the repository.
summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDouwe Maan <douwe@gitlab.com>2017-10-02 18:24:48 +0300
committerDouwe Maan <douwe@gitlab.com>2017-10-02 18:24:48 +0300
commitb40192a9464503bf4b141f8cf6133d7ba0f893fe (patch)
treeb88f2ecfa2a6d0eca0e00dc09d30dcc16af4d32b
parent4716e81f9dc73b95e45df26fd339b75aaf6b48f6 (diff)
parent011c168bff7174ce4b2defe239aa8d5031aa8269 (diff)
Merge branch '33493-attempt-to-link-saml-users-to-ldap-by-email' into 'master'
Attempt to link saml users to ldap by email Closes #33493 See merge request gitlab-org/gitlab-ce!14216
-rw-r--r--app/models/user.rb6
-rw-r--r--changelogs/unreleased/33493-attempt-to-link-saml-users-to-ldap-by-email.yml5
-rw-r--r--lib/gitlab/ldap/adapter.rb22
-rw-r--r--lib/gitlab/ldap/person.rb6
-rw-r--r--lib/gitlab/ldap/user.rb26
-rw-r--r--lib/gitlab/o_auth/user.rb68
-rw-r--r--lib/gitlab/saml/user.rb37
-rw-r--r--spec/lib/gitlab/o_auth/user_spec.rb17
-rw-r--r--spec/lib/gitlab/saml/user_spec.rb97
9 files changed, 173 insertions, 111 deletions
diff --git a/app/models/user.rb b/app/models/user.rb
index 4d523aa983f..e5f345e93ae 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -692,7 +692,11 @@ class User < ActiveRecord::Base
end
def ldap_user?
- identities.exists?(["provider LIKE ? AND extern_uid IS NOT NULL", "ldap%"])
+ if identities.loaded?
+ identities.find { |identity| identity.provider.start_with?('ldap') && !identity.extern_uid.nil? }
+ else
+ identities.exists?(["provider LIKE ? AND extern_uid IS NOT NULL", "ldap%"])
+ end
end
def ldap_identity
diff --git a/changelogs/unreleased/33493-attempt-to-link-saml-users-to-ldap-by-email.yml b/changelogs/unreleased/33493-attempt-to-link-saml-users-to-ldap-by-email.yml
new file mode 100644
index 00000000000..727f3cecd52
--- /dev/null
+++ b/changelogs/unreleased/33493-attempt-to-link-saml-users-to-ldap-by-email.yml
@@ -0,0 +1,5 @@
+---
+title: Link SAML users to LDAP by email.
+merge_request: 14216
+author:
+type: changed
diff --git a/lib/gitlab/ldap/adapter.rb b/lib/gitlab/ldap/adapter.rb
index cd7e4ca7b7e..0afaa2306b5 100644
--- a/lib/gitlab/ldap/adapter.rb
+++ b/lib/gitlab/ldap/adapter.rb
@@ -22,8 +22,8 @@ module Gitlab
Gitlab::LDAP::Config.new(provider)
end
- def users(field, value, limit = nil)
- options = user_options(field, value, limit)
+ def users(fields, value, limit = nil)
+ options = user_options(Array(fields), value, limit)
entries = ldap_search(options).select do |entry|
entry.respond_to? config.uid
@@ -72,20 +72,24 @@ module Gitlab
private
- def user_options(field, value, limit)
- options = { attributes: Gitlab::LDAP::Person.ldap_attributes(config).compact.uniq }
+ def user_options(fields, value, limit)
+ options = {
+ attributes: Gitlab::LDAP::Person.ldap_attributes(config).compact.uniq,
+ base: config.base
+ }
+
options[:size] = limit if limit
- if field.to_sym == :dn
+ if fields.include?('dn')
+ raise ArgumentError, 'It is not currently possible to search the DN and other fields at the same time.' if fields.size > 1
+
options[:base] = value
options[:scope] = Net::LDAP::SearchScope_BaseObject
- options[:filter] = user_filter
else
- options[:base] = config.base
- options[:filter] = user_filter(Net::LDAP::Filter.eq(field, value))
+ filter = fields.map { |field| Net::LDAP::Filter.eq(field, value) }.inject(:|)
end
- options
+ options.merge(filter: user_filter(filter))
end
def user_filter(filter = nil)
diff --git a/lib/gitlab/ldap/person.rb b/lib/gitlab/ldap/person.rb
index 4d6f8ac79de..9a6f7827b16 100644
--- a/lib/gitlab/ldap/person.rb
+++ b/lib/gitlab/ldap/person.rb
@@ -17,6 +17,12 @@ module Gitlab
adapter.user('dn', dn)
end
+ def self.find_by_email(email, adapter)
+ email_fields = adapter.config.attributes['email']
+
+ adapter.user(email_fields, email)
+ end
+
def self.disabled_via_active_directory?(dn, adapter)
adapter.dn_matches_filter?(dn, AD_USER_DISABLED)
end
diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb
index 3bf27b37ae6..1793097363e 100644
--- a/lib/gitlab/ldap/user.rb
+++ b/lib/gitlab/ldap/user.rb
@@ -17,41 +17,19 @@ module Gitlab
end
end
- def initialize(auth_hash)
- super
- update_user_attributes
- end
-
def save
super('LDAP')
end
# instance methods
- def gl_user
- @gl_user ||= find_by_uid_and_provider || find_by_email || build_new_user
+ def find_user
+ find_by_uid_and_provider || find_by_email || build_new_user
end
def find_by_uid_and_provider
self.class.find_by_uid_and_provider(auth_hash.uid, auth_hash.provider)
end
- def find_by_email
- ::User.find_by(email: auth_hash.email.downcase) if auth_hash.has_attribute?(:email)
- end
-
- def update_user_attributes
- if persisted?
- # find_or_initialize_by doesn't update `gl_user.identities`, and isn't autosaved.
- identity = gl_user.identities.find { |identity| identity.provider == auth_hash.provider }
- identity ||= gl_user.identities.build(provider: auth_hash.provider)
-
- # For a new identity set extern_uid to the LDAP DN
- # For an existing identity with matching email but changed DN, update the DN.
- # For an existing identity with no change in DN, this line changes nothing.
- identity.extern_uid = auth_hash.uid
- end
- end
-
def changed?
gl_user.changed? || gl_user.identities.any?(&:changed?)
end
diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb
index e06d4dc45f7..68815be4d13 100644
--- a/lib/gitlab/o_auth/user.rb
+++ b/lib/gitlab/o_auth/user.rb
@@ -13,6 +13,7 @@ module Gitlab
def initialize(auth_hash)
self.auth_hash = auth_hash
update_profile if sync_profile_from_provider?
+ add_or_update_user_identities
end
def persisted?
@@ -44,47 +45,54 @@ module Gitlab
end
def gl_user
- @user ||= find_by_uid_and_provider
+ return @gl_user if defined?(@gl_user)
- if auto_link_ldap_user?
- @user ||= find_or_create_ldap_user
- end
+ @gl_user = find_user
+ end
- if signup_enabled?
- @user ||= build_new_user
- end
+ def find_user
+ user = find_by_uid_and_provider
- if external_provider? && @user
- @user.external = true
- end
+ user ||= find_or_build_ldap_user if auto_link_ldap_user?
+ user ||= build_new_user if signup_enabled?
+
+ user.external = true if external_provider? && user
- @user
+ user
end
protected
- def find_or_create_ldap_user
+ def add_or_update_user_identities
+ # find_or_initialize_by doesn't update `gl_user.identities`, and isn't autosaved.
+ identity = gl_user.identities.find { |identity| identity.provider == auth_hash.provider }
+
+ identity ||= gl_user.identities.build(provider: auth_hash.provider)
+ identity.extern_uid = auth_hash.uid
+
+ if auto_link_ldap_user? && !gl_user.ldap_user? && ldap_person
+ log.info "Correct LDAP account has been found. identity to user: #{gl_user.username}."
+ gl_user.identities.build(provider: ldap_person.provider, extern_uid: ldap_person.dn)
+ end
+ end
+
+ def find_or_build_ldap_user
return unless ldap_person
- # If a corresponding person exists with same uid in a LDAP server,
- # check if the user already has a GitLab account.
user = Gitlab::LDAP::User.find_by_uid_and_provider(ldap_person.dn, ldap_person.provider)
if user
- # Case when a LDAP user already exists in Gitlab. Add the OAuth identity to existing account.
log.info "LDAP account found for user #{user.username}. Building new #{auth_hash.provider} identity."
- user.identities.find_or_initialize_by(extern_uid: auth_hash.uid, provider: auth_hash.provider)
- else
- log.info "No existing LDAP account was found in GitLab. Checking for #{auth_hash.provider} account."
- user = find_by_uid_and_provider
- if user.nil?
- log.info "No user found using #{auth_hash.provider} provider. Creating a new one."
- user = build_new_user
- end
- log.info "Correct account has been found. Adding LDAP identity to user: #{user.username}."
- user.identities.new(provider: ldap_person.provider, extern_uid: ldap_person.dn)
+ return user
end
- user
+ log.info "No user found using #{auth_hash.provider} provider. Creating a new one."
+ build_new_user
+ end
+
+ def find_by_email
+ return unless auth_hash.has_attribute?(:email)
+
+ ::User.find_by(email: auth_hash.email.downcase)
end
def auto_link_ldap_user?
@@ -108,9 +116,9 @@ module Gitlab
end
def find_ldap_person(auth_hash, adapter)
- by_uid = Gitlab::LDAP::Person.find_by_uid(auth_hash.uid, adapter)
- # The `uid` might actually be a DN. Try it next.
- by_uid || Gitlab::LDAP::Person.find_by_dn(auth_hash.uid, adapter)
+ Gitlab::LDAP::Person.find_by_uid(auth_hash.uid, adapter) ||
+ Gitlab::LDAP::Person.find_by_email(auth_hash.uid, adapter) ||
+ Gitlab::LDAP::Person.find_by_dn(auth_hash.uid, adapter)
end
def ldap_config
@@ -152,7 +160,7 @@ module Gitlab
end
def build_new_user
- user_params = user_attributes.merge(extern_uid: auth_hash.uid, provider: auth_hash.provider, skip_confirmation: true)
+ user_params = user_attributes.merge(skip_confirmation: true)
Users::BuildService.new(nil, user_params).execute(skip_authorization: true)
end
diff --git a/lib/gitlab/saml/user.rb b/lib/gitlab/saml/user.rb
index 0f323a9e8b2..e0a9d1dee77 100644
--- a/lib/gitlab/saml/user.rb
+++ b/lib/gitlab/saml/user.rb
@@ -10,41 +10,20 @@ module Gitlab
super('SAML')
end
- def gl_user
- if auto_link_ldap_user?
- @user ||= find_or_create_ldap_user
- end
-
- @user ||= find_by_uid_and_provider
-
- if auto_link_saml_user?
- @user ||= find_by_email
- end
+ def find_user
+ user = find_by_uid_and_provider
- if signup_enabled?
- @user ||= build_new_user
- end
+ user ||= find_by_email if auto_link_saml_user?
+ user ||= find_or_build_ldap_user if auto_link_ldap_user?
+ user ||= build_new_user if signup_enabled?
- if external_users_enabled? && @user
+ if external_users_enabled? && user
# Check if there is overlap between the user's groups and the external groups
# setting then set user as external or internal.
- @user.external =
- if (auth_hash.groups & Gitlab::Saml::Config.external_groups).empty?
- false
- else
- true
- end
+ user.external = !(auth_hash.groups & Gitlab::Saml::Config.external_groups).empty?
end
- @user
- end
-
- def find_by_email
- if auth_hash.has_attribute?(:email)
- user = ::User.find_by(email: auth_hash.email.downcase)
- user.identities.new(extern_uid: auth_hash.uid, provider: auth_hash.provider) if user
- user
- end
+ user
end
def changed?
diff --git a/spec/lib/gitlab/o_auth/user_spec.rb b/spec/lib/gitlab/o_auth/user_spec.rb
index 8aaf320cbf5..db26e16e3b2 100644
--- a/spec/lib/gitlab/o_auth/user_spec.rb
+++ b/spec/lib/gitlab/o_auth/user_spec.rb
@@ -4,6 +4,7 @@ describe Gitlab::OAuth::User do
let(:oauth_user) { described_class.new(auth_hash) }
let(:gl_user) { oauth_user.gl_user }
let(:uid) { 'my-uid' }
+ let(:dn) { 'uid=user1,ou=People,dc=example' }
let(:provider) { 'my-provider' }
let(:auth_hash) { OmniAuth::AuthHash.new(uid: uid, provider: provider, info: info_hash) }
let(:info_hash) do
@@ -197,7 +198,7 @@ describe Gitlab::OAuth::User do
allow(ldap_user).to receive(:uid) { uid }
allow(ldap_user).to receive(:username) { uid }
allow(ldap_user).to receive(:email) { ['johndoe@example.com', 'john2@example.com'] }
- allow(ldap_user).to receive(:dn) { 'uid=user1,ou=People,dc=example' }
+ allow(ldap_user).to receive(:dn) { dn }
end
context "and no account for the LDAP user" do
@@ -213,7 +214,7 @@ describe Gitlab::OAuth::User do
identities_as_hash = gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }
expect(identities_as_hash).to match_array(
[
- { provider: 'ldapmain', extern_uid: 'uid=user1,ou=People,dc=example' },
+ { provider: 'ldapmain', extern_uid: dn },
{ provider: 'twitter', extern_uid: uid }
]
)
@@ -221,7 +222,7 @@ describe Gitlab::OAuth::User do
end
context "and LDAP user has an account already" do
- let!(:existing_user) { create(:omniauth_user, email: 'john@example.com', extern_uid: 'uid=user1,ou=People,dc=example', provider: 'ldapmain', username: 'john') }
+ let!(:existing_user) { create(:omniauth_user, email: 'john@example.com', extern_uid: dn, provider: 'ldapmain', username: 'john') }
it "adds the omniauth identity to the LDAP account" do
allow(Gitlab::LDAP::Person).to receive(:find_by_uid).and_return(ldap_user)
@@ -234,7 +235,7 @@ describe Gitlab::OAuth::User do
identities_as_hash = gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }
expect(identities_as_hash).to match_array(
[
- { provider: 'ldapmain', extern_uid: 'uid=user1,ou=People,dc=example' },
+ { provider: 'ldapmain', extern_uid: dn },
{ provider: 'twitter', extern_uid: uid }
]
)
@@ -252,7 +253,7 @@ describe Gitlab::OAuth::User do
expect(identities_as_hash)
.to match_array(
[
- { provider: 'ldapmain', extern_uid: 'uid=user1,ou=People,dc=example' },
+ { provider: 'ldapmain', extern_uid: dn },
{ provider: 'twitter', extern_uid: uid }
]
)
@@ -310,8 +311,8 @@ describe Gitlab::OAuth::User do
allow(ldap_user).to receive(:uid) { uid }
allow(ldap_user).to receive(:username) { uid }
allow(ldap_user).to receive(:email) { ['johndoe@example.com', 'john2@example.com'] }
- allow(ldap_user).to receive(:dn) { 'uid=user1,ou=People,dc=example' }
- allow(oauth_user).to receive(:ldap_person).and_return(ldap_user)
+ allow(ldap_user).to receive(:dn) { dn }
+ allow(Gitlab::LDAP::Person).to receive(:find_by_uid).and_return(ldap_user)
end
context "and no account for the LDAP user" do
@@ -341,7 +342,7 @@ describe Gitlab::OAuth::User do
end
context 'and LDAP user has an account already' do
- let!(:existing_user) { create(:omniauth_user, email: 'john@example.com', extern_uid: 'uid=user1,ou=People,dc=example', provider: 'ldapmain', username: 'john') }
+ let!(:existing_user) { create(:omniauth_user, email: 'john@example.com', extern_uid: dn, provider: 'ldapmain', username: 'john') }
context 'dont block on create (LDAP)' do
before do
diff --git a/spec/lib/gitlab/saml/user_spec.rb b/spec/lib/gitlab/saml/user_spec.rb
index 19710029224..59923bfb14d 100644
--- a/spec/lib/gitlab/saml/user_spec.rb
+++ b/spec/lib/gitlab/saml/user_spec.rb
@@ -1,9 +1,12 @@
require 'spec_helper'
describe Gitlab::Saml::User do
+ include LdapHelpers
+
let(:saml_user) { described_class.new(auth_hash) }
let(:gl_user) { saml_user.gl_user }
let(:uid) { 'my-uid' }
+ let(:dn) { 'uid=user1,ou=People,dc=example' }
let(:provider) { 'saml' }
let(:auth_hash) { OmniAuth::AuthHash.new(uid: uid, provider: provider, info: info_hash, extra: { raw_info: OneLogin::RubySaml::Attributes.new({ 'groups' => %w(Developers Freelancers Designers) }) }) }
let(:info_hash) do
@@ -163,13 +166,17 @@ describe Gitlab::Saml::User do
end
context 'and a corresponding LDAP person' do
+ let(:adapter) { ldap_adapter('ldapmain') }
+
before do
allow(ldap_user).to receive(:uid) { uid }
allow(ldap_user).to receive(:username) { uid }
allow(ldap_user).to receive(:email) { %w(john@mail.com john2@example.com) }
- allow(ldap_user).to receive(:dn) { 'uid=user1,ou=People,dc=example' }
- allow(Gitlab::LDAP::Person).to receive(:find_by_uid).and_return(ldap_user)
- allow(Gitlab::LDAP::Person).to receive(:find_by_dn).and_return(ldap_user)
+ allow(ldap_user).to receive(:dn) { dn }
+ allow(Gitlab::LDAP::Adapter).to receive(:new).and_return(adapter)
+ allow(Gitlab::LDAP::Person).to receive(:find_by_uid).with(uid, adapter).and_return(ldap_user)
+ allow(Gitlab::LDAP::Person).to receive(:find_by_dn).with(dn, adapter).and_return(ldap_user)
+ allow(Gitlab::LDAP::Person).to receive(:find_by_email).with('john@mail.com', adapter).and_return(ldap_user)
end
context 'and no account for the LDAP user' do
@@ -181,20 +188,86 @@ describe Gitlab::Saml::User do
expect(gl_user.email).to eql 'john@mail.com'
expect(gl_user.identities.length).to be 2
identities_as_hash = gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }
- expect(identities_as_hash).to match_array([{ provider: 'ldapmain', extern_uid: 'uid=user1,ou=People,dc=example' },
+ expect(identities_as_hash).to match_array([{ provider: 'ldapmain', extern_uid: dn },
{ provider: 'saml', extern_uid: uid }])
end
end
context 'and LDAP user has an account already' do
+ let(:auth_hash_base_attributes) do
+ {
+ uid: uid,
+ provider: provider,
+ info: info_hash,
+ extra: {
+ raw_info: OneLogin::RubySaml::Attributes.new(
+ { 'groups' => %w(Developers Freelancers Designers) }
+ )
+ }
+ }
+ end
+ let(:auth_hash) { OmniAuth::AuthHash.new(auth_hash_base_attributes) }
+ let(:uid_types) { %w(uid dn email) }
+
before do
create(:omniauth_user,
email: 'john@mail.com',
- extern_uid: 'uid=user1,ou=People,dc=example',
+ extern_uid: dn,
provider: 'ldapmain',
username: 'john')
end
+ shared_examples 'find LDAP person' do |uid_type, uid|
+ let(:auth_hash) { OmniAuth::AuthHash.new(auth_hash_base_attributes.merge(uid: extern_uid)) }
+
+ before do
+ nil_types = uid_types - [uid_type]
+
+ nil_types.each do |type|
+ allow(Gitlab::LDAP::Person).to receive(:"find_by_#{type}").and_return(nil)
+ end
+
+ allow(Gitlab::LDAP::Person).to receive(:"find_by_#{uid_type}").and_return(ldap_user)
+ end
+
+ it 'adds the omniauth identity to the LDAP account' do
+ identities = [
+ { provider: 'ldapmain', extern_uid: dn },
+ { provider: 'saml', extern_uid: extern_uid }
+ ]
+
+ identities_as_hash = gl_user.identities.map do |id|
+ { provider: id.provider, extern_uid: id.extern_uid }
+ end
+
+ saml_user.save
+
+ expect(gl_user).to be_valid
+ expect(gl_user.username).to eql 'john'
+ expect(gl_user.email).to eql 'john@mail.com'
+ expect(gl_user.identities.length).to be 2
+ expect(identities_as_hash).to match_array(identities)
+ end
+ end
+
+ context 'when uid is an uid' do
+ it_behaves_like 'find LDAP person', 'uid' do
+ let(:extern_uid) { uid }
+ end
+ end
+
+ context 'when uid is a dn' do
+ it_behaves_like 'find LDAP person', 'dn' do
+ let(:extern_uid) { dn }
+ end
+ end
+
+ context 'when uid is an email' do
+ it_behaves_like 'find LDAP person', 'email' do
+ let(:extern_uid) { 'john@mail.com' }
+ end
+ end
+
it 'adds the omniauth identity to the LDAP account' do
saml_user.save
@@ -203,7 +276,7 @@ describe Gitlab::Saml::User do
expect(gl_user.email).to eql 'john@mail.com'
expect(gl_user.identities.length).to be 2
identities_as_hash = gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }
- expect(identities_as_hash).to match_array([{ provider: 'ldapmain', extern_uid: 'uid=user1,ou=People,dc=example' },
+ expect(identities_as_hash).to match_array([{ provider: 'ldapmain', extern_uid: dn },
{ provider: 'saml', extern_uid: uid }])
end
@@ -219,17 +292,21 @@ describe Gitlab::Saml::User do
context 'user has SAML user, and wants to add their LDAP identity' do
it 'adds the LDAP identity to the existing SAML user' do
- create(:omniauth_user, email: 'john@mail.com', extern_uid: 'uid=user1,ou=People,dc=example', provider: 'saml', username: 'john')
- local_hash = OmniAuth::AuthHash.new(uid: 'uid=user1,ou=People,dc=example', provider: provider, info: info_hash)
+ create(:omniauth_user, email: 'john@mail.com', extern_uid: dn, provider: 'saml', username: 'john')
+
+ allow(Gitlab::LDAP::Person).to receive(:find_by_uid).with(dn, adapter).and_return(ldap_user)
+
+ local_hash = OmniAuth::AuthHash.new(uid: dn, provider: provider, info: info_hash)
local_saml_user = described_class.new(local_hash)
+
local_saml_user.save
local_gl_user = local_saml_user.gl_user
expect(local_gl_user).to be_valid
expect(local_gl_user.identities.length).to be 2
identities_as_hash = local_gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }
- expect(identities_as_hash).to match_array([{ provider: 'ldapmain', extern_uid: 'uid=user1,ou=People,dc=example' },
- { provider: 'saml', extern_uid: 'uid=user1,ou=People,dc=example' }])
+ expect(identities_as_hash).to match_array([{ provider: 'ldapmain', extern_uid: dn },
+ { provider: 'saml', extern_uid: dn }])
end
end
end