Commit 45ab20dc authored by Michael Kozono's avatar Michael Kozono

Switch to new DN class

for normalizing and parsing DNs
parent fe46c11d
...@@ -4,7 +4,7 @@ module Gitlab ...@@ -4,7 +4,7 @@ module Gitlab
module LDAP module LDAP
class AuthHash < Gitlab::OAuth::AuthHash class AuthHash < Gitlab::OAuth::AuthHash
def uid def uid
Gitlab::LDAP::Person.normalize_dn(super) Gitlab::LDAP::DN.new(super).to_s_normalized
end end
private private
......
...@@ -49,21 +49,6 @@ module Gitlab ...@@ -49,21 +49,6 @@ module Gitlab
uid uid
end end
# Returns the DN in a normalized form.
#
# 1. Excess spaces around attribute names and values are stripped
# 2. The string is downcased (for case-insensitivity)
def self.normalize_dn(dn)
dn.split(/(?<!\\)([,+=])/).map do |part|
normalize_dn_part(part)
end.join('')
rescue StandardError => e
Rails.logger.info("Returning original DN \"#{dn}\" due to error during normalization attempt: #{e.message}")
Rails.logger.info(e.backtrace.join("\n"))
dn
end
def initialize(entry, provider) def initialize(entry, provider)
Rails.logger.debug { "Instantiating #{self.class.name} with LDIF:\n#{entry.to_ldif}" } Rails.logger.debug { "Instantiating #{self.class.name} with LDIF:\n#{entry.to_ldif}" }
@entry = entry @entry = entry
...@@ -87,7 +72,7 @@ module Gitlab ...@@ -87,7 +72,7 @@ module Gitlab
end end
def dn def dn
self.class.normalize_dn(entry.dn) DN.new(entry.dn).to_s_normalized
end end
private private
......
...@@ -17,41 +17,6 @@ describe Gitlab::LDAP::Person do ...@@ -17,41 +17,6 @@ describe Gitlab::LDAP::Person do
) )
end end
shared_examples_for 'normalizes the DN' do
# Regarding the telephoneNumber test:
#
# I am not sure whether a space after the telephoneNumber plus sign is valid,
# and I am not sure if this is "proper" behavior under these conditions, and
# I am not sure if it matters to us or anyone else, so rather than dig
# through RFCs, I am only documenting the behavior here.
where(:test_description, :given, :expected) do
'strips extraneous whitespace' | 'uid =John Smith , ou = People, dc= example,dc =com' | 'uid=john smith,ou=people,dc=example,dc=com'
'strips extraneous whitespace for a DN with a single RDN' | 'uid = John Smith' | 'uid=john smith'
'strips extraneous whitespace without changing escaped characters' | 'uid = Sebasti\\c3\\a1n\\ C.\\20Smith\\ , ou=People (aka. \\22humans\\") ,dc=example, dc=com' | 'uid=sebasti\\c3\\a1n\\ c.\\20smith\\ ,ou=people (aka. \\22humans\\"),dc=example,dc=com'
'strips extraneous whitespace without modifying the multivalued RDN' | 'uid = John Smith + telephoneNumber = +1 555-555-5555 , ou = People,dc=example,dc=com' | 'uid=john smith+telephonenumber=+1 555-555-5555,ou=people,dc=example,dc=com'
'strips the space after the plus sign in the telephoneNumber' | 'uid = John Smith + telephoneNumber = + 1 555-555-5555 , ou = People,dc=example,dc=com' | 'uid=john smith+telephonenumber=+1 555-555-5555,ou=people,dc=example,dc=com'
'downcases the whole string' | 'UID=John Smith,ou=People,dc=example,dc=com' | 'uid=john smith,ou=people,dc=example,dc=com'
'for a null DN (empty string), returns empty string and does not error' | '' | ''
'does not strip an escaped leading space in an attribute value (and does not error like Net::LDAP::DN.new does)' | 'uid=\\ John Smith,ou=People,dc=example,dc=com' | 'uid=\\ john smith,ou=people,dc=example,dc=com'
'does not strip an escaped trailing space in an attribute value' | 'uid=John Smith\\ ,ou=People,dc=example,dc=com' | 'uid=john smith\\ ,ou=people,dc=example,dc=com'
'does not strip an escaped leading newline in an attribute value' | 'uid=\\\nJohn Smith,ou=People,dc=example,dc=com' | 'uid=\\\njohn smith,ou=people,dc=example,dc=com'
'does not strip an escaped trailing newline in an attribute value' | 'uid=John Smith\\\n,ou=People,dc=example,dc=com' | 'uid=john smith\\\n,ou=people,dc=example,dc=com'
'does not strip an unescaped leading newline (actually an invalid DN)' | 'uid=\nJohn Smith,ou=People,dc=example,dc=com' | 'uid=\njohn smith,ou=people,dc=example,dc=com'
'does not strip an unescaped trailing newline (actually an invalid DN)' | 'uid=John Smith\n ,ou=People,dc=example,dc=com' | 'uid=john smith\n,ou=people,dc=example,dc=com'
'does not strip non whitespace' | 'uid=John Smith,ou=People,dc=example,dc=com' | 'uid=john smith,ou=people,dc=example,dc=com'
'does not treat escaped equal signs as attribute delimiters' | 'uid= foo \\= bar' | 'uid=foo \\= bar'
'does not treat escaped hex equal signs as attribute delimiters' | 'uid= foo \\3D bar' | 'uid=foo \\3d bar'
'does not treat escaped commas as attribute delimiters' | 'uid= John C. Smith, ou=San Francisco\\, CA' | 'uid=john c. smith,ou=san francisco\\, ca'
'does not treat escaped hex commas as attribute delimiters' | 'uid= John C. Smith, ou=San Francisco\\2C CA' | 'uid=john c. smith,ou=san francisco\\2c ca'
end
with_them do
it 'normalizes the DN' do
assert_generic_test(test_description, subject, expected)
end
end
end
shared_examples_for 'normalizes the UID' do shared_examples_for 'normalizes the UID' do
where(:test_description, :given, :expected) do where(:test_description, :given, :expected) do
'strips extraneous whitespace' | ' John C. Smith ' | 'john c. smith' 'strips extraneous whitespace' | ' John C. Smith ' | 'john c. smith'
...@@ -91,20 +56,6 @@ describe Gitlab::LDAP::Person do ...@@ -91,20 +56,6 @@ describe Gitlab::LDAP::Person do
end end
end end
describe '.normalize_dn' do
subject { described_class.normalize_dn(given) }
it_behaves_like 'normalizes the DN'
context 'with an exception during normalization' do
let(:given) { described_class } # just something that will cause an exception
it 'returns the given DN unmodified' do
expect(subject).to eq(given)
end
end
end
describe '#name' do describe '#name' do
it 'uses the configured name attribute and handles values as an array' do it 'uses the configured name attribute and handles values as an array' do
name = 'John Doe' name = 'John Doe'
......
...@@ -11,7 +11,7 @@ describe Gitlab::LDAP::User do ...@@ -11,7 +11,7 @@ describe Gitlab::LDAP::User do
} }
end end
let(:auth_hash) do let(:auth_hash) do
OmniAuth::AuthHash.new(uid: 'my-uid', provider: 'ldapmain', info: info) OmniAuth::AuthHash.new(uid: 'uid=John Smith,ou=People,dc=example,dc=com', provider: 'ldapmain', info: info)
end end
let(:ldap_user_upper_case) { described_class.new(auth_hash_upper_case) } let(:ldap_user_upper_case) { described_class.new(auth_hash_upper_case) }
let(:info_upper_case) do let(:info_upper_case) do
...@@ -22,12 +22,12 @@ describe Gitlab::LDAP::User do ...@@ -22,12 +22,12 @@ describe Gitlab::LDAP::User do
} }
end end
let(:auth_hash_upper_case) do let(:auth_hash_upper_case) do
OmniAuth::AuthHash.new(uid: 'my-uid', provider: 'ldapmain', info: info_upper_case) OmniAuth::AuthHash.new(uid: 'uid=John Smith,ou=People,dc=example,dc=com', provider: 'ldapmain', info: info_upper_case)
end end
describe '#changed?' do describe '#changed?' do
it "marks existing ldap user as changed" do it "marks existing ldap user as changed" do
create(:omniauth_user, extern_uid: 'my-uid', provider: 'ldapmain') create(:omniauth_user, extern_uid: 'uid=John Smith,ou=People,dc=example,dc=com', provider: 'ldapmain')
expect(ldap_user.changed?).to be_truthy expect(ldap_user.changed?).to be_truthy
end end
...@@ -37,7 +37,7 @@ describe Gitlab::LDAP::User do ...@@ -37,7 +37,7 @@ describe Gitlab::LDAP::User do
end end
it "does not mark existing ldap user as changed" do it "does not mark existing ldap user as changed" do
create(:omniauth_user, email: 'john@example.com', extern_uid: 'my-uid', provider: 'ldapmain') create(:omniauth_user, email: 'john@example.com', extern_uid: 'uid=john smith,ou=people,dc=example,dc=com', provider: 'ldapmain')
ldap_user.gl_user.user_synced_attributes_metadata(provider: 'ldapmain', email: true) ldap_user.gl_user.user_synced_attributes_metadata(provider: 'ldapmain', email: true)
expect(ldap_user.changed?).to be_falsey expect(ldap_user.changed?).to be_falsey
end end
...@@ -60,7 +60,7 @@ describe Gitlab::LDAP::User do ...@@ -60,7 +60,7 @@ describe Gitlab::LDAP::User do
describe 'find or create' do describe 'find or create' do
it "finds the user if already existing" do it "finds the user if already existing" do
create(:omniauth_user, extern_uid: 'my-uid', provider: 'ldapmain') create(:omniauth_user, extern_uid: 'uid=John Smith,ou=People,dc=example,dc=com', provider: 'ldapmain')
expect { ldap_user.save }.not_to change { User.count } expect { ldap_user.save }.not_to change { User.count }
end end
...@@ -70,7 +70,7 @@ describe Gitlab::LDAP::User do ...@@ -70,7 +70,7 @@ describe Gitlab::LDAP::User do
expect { ldap_user.save }.not_to change { User.count } expect { ldap_user.save }.not_to change { User.count }
existing_user.reload existing_user.reload
expect(existing_user.ldap_identity.extern_uid).to eql 'my-uid' expect(existing_user.ldap_identity.extern_uid).to eql 'uid=john smith,ou=people,dc=example,dc=com'
expect(existing_user.ldap_identity.provider).to eql 'ldapmain' expect(existing_user.ldap_identity.provider).to eql 'ldapmain'
end end
...@@ -79,7 +79,7 @@ describe Gitlab::LDAP::User do ...@@ -79,7 +79,7 @@ describe Gitlab::LDAP::User do
expect { ldap_user.save }.not_to change { User.count } expect { ldap_user.save }.not_to change { User.count }
existing_user.reload existing_user.reload
expect(existing_user.ldap_identity.extern_uid).to eql 'my-uid' expect(existing_user.ldap_identity.extern_uid).to eql 'uid=john smith,ou=people,dc=example,dc=com'
expect(existing_user.ldap_identity.provider).to eql 'ldapmain' expect(existing_user.ldap_identity.provider).to eql 'ldapmain'
expect(existing_user.id).to eql ldap_user.gl_user.id expect(existing_user.id).to eql ldap_user.gl_user.id
end end
...@@ -89,7 +89,7 @@ describe Gitlab::LDAP::User do ...@@ -89,7 +89,7 @@ describe Gitlab::LDAP::User do
expect { ldap_user_upper_case.save }.not_to change { User.count } expect { ldap_user_upper_case.save }.not_to change { User.count }
existing_user.reload existing_user.reload
expect(existing_user.ldap_identity.extern_uid).to eql 'my-uid' expect(existing_user.ldap_identity.extern_uid).to eql 'uid=john smith,ou=people,dc=example,dc=com'
expect(existing_user.ldap_identity.provider).to eql 'ldapmain' expect(existing_user.ldap_identity.provider).to eql 'ldapmain'
expect(existing_user.id).to eql ldap_user.gl_user.id expect(existing_user.id).to eql ldap_user.gl_user.id
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment