diff --git a/.travis.yml b/.travis.yml index baef6da..e8fd1f0 100644 --- a/.travis.yml +++ b/.travis.yml @@ -2,5 +2,5 @@ branches: only: - 'master' rvm: - - 1.9.2 + - 2.0.0 script: "bundle exec rspec spec" diff --git a/Gemfile.lock b/Gemfile.lock index 3641e51..9bff2bd 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - gitlab_omniauth-ldap (1.0.2) + gitlab_omniauth-ldap (1.0.4) net-ldap (~> 0.3.1) omniauth (~> 1.0) pyu-ruby-sasl (~> 0.0.3.1) diff --git a/README.md b/README.md index cfd2270..420a960 100644 --- a/README.md +++ b/README.md @@ -11,6 +11,10 @@ Use the LDAP strategy as a middleware in your application: :method => :plain, :base => 'dc=intridea, dc=com', :uid => 'sAMAccountName', + :name_proc => Proc.new {|name| name.gsub(/@.*$/,'')}, + :bind_dn => 'default_bind_dn', + # Or, alternatively: + #:filter => '(&(uid=%{username})(memberOf=cn=myapp-users,ou=groups,dc=example,dc=com))' :name_proc => Proc.new {|name| name.gsub(/@.*$/,'')} :bind_dn => 'default_bind_dn' :password => 'password' @@ -29,6 +33,9 @@ Allowed values of :method are: :plain, :ssl, :tls. :uid is the LDAP attribute name for the user name in the login form. typically AD would be 'sAMAccountName' or 'UserPrincipalName', while OpenLDAP is 'uid'. +:filter is the LDAP filter used to search the user entry. It can be used in place of :uid for more flexibility. + `%{username}` will be replaced by the user name processed by :name_proc. + :name_proc allows you to match the user name entered with the format of the :uid attributes. For example, value of 'sAMAccountName' in AD contains only the windows user name. If your user prefers using email to login, a name_proc as above will trim the email string down to just the windows login name. diff --git a/gitlab_omniauth-ldap.gemspec b/gitlab_omniauth-ldap.gemspec index 589bb8a..b928ee3 100644 --- a/gitlab_omniauth-ldap.gemspec +++ b/gitlab_omniauth-ldap.gemspec @@ -7,6 +7,7 @@ Gem::Specification.new do |gem| gem.description = %q{A LDAP strategy for OmniAuth.} gem.summary = %q{A LDAP strategy for OmniAuth.} gem.homepage = "https://github.com/gitlabhq/omniauth-ldap" + gem.license = "MIT" gem.add_runtime_dependency 'omniauth', '~> 1.0' gem.add_runtime_dependency 'net-ldap', '~> 0.3.1' diff --git a/lib/omniauth-ldap/adaptor.rb b/lib/omniauth-ldap/adaptor.rb index 66459ad..68b4e14 100644 --- a/lib/omniauth-ldap/adaptor.rb +++ b/lib/omniauth-ldap/adaptor.rb @@ -3,7 +3,6 @@ require 'rack' require 'net/ldap' require 'net/ntlm' -require 'uri' require 'sasl' require 'kconv' module OmniAuth @@ -14,9 +13,10 @@ class ConfigurationError < StandardError; end class AuthenticationError < StandardError; end class ConnectionError < StandardError; end - VALID_ADAPTER_CONFIGURATION_KEYS = [:host, :port, :method, :bind_dn, :password, :try_sasl, :sasl_mechanisms, :uid, :base, :allow_anonymous] + VALID_ADAPTER_CONFIGURATION_KEYS = [:host, :port, :method, :bind_dn, :password, :try_sasl, :sasl_mechanisms, :uid, :base, :allow_anonymous, :filter] - MUST_HAVE_KEYS = [:host, :port, :method, :uid, :base] + # A list of needed keys. Possible alternatives are specified using sub-lists. + MUST_HAVE_KEYS = [:host, :port, :method, [:uid, :filter], :base] METHOD = { :ssl => :simple_tls, @@ -25,11 +25,15 @@ class ConnectionError < StandardError; end } attr_accessor :bind_dn, :password - attr_reader :connection, :uid, :base, :auth + attr_reader :connection, :uid, :base, :auth, :filter def self.validate(configuration={}) message = [] - MUST_HAVE_KEYS.each do |name| - message << name if configuration[name].nil? + MUST_HAVE_KEYS.each do |names| + names = [names].flatten + missing_keys = names.select{|name| configuration[name].nil?} + if missing_keys == names + message << names.join(' or ') + end end raise ArgumentError.new(message.join(",") +" MUST be provided") unless message.empty? end @@ -48,7 +52,6 @@ def initialize(configuration={}) :encryption => method, :base => @base } - @uri = construct_uri(@host, @port, @method != :plain) @bind_method = @try_sasl ? :sasl : (@allow_anonymous||!@bind_dn||!@password ? :anonymous : :simple) @@ -140,10 +143,6 @@ def sasl_bind_setup_gss_spnego(options) [Net::NTLM::Message::Type1.new.serialize, nego] end - def construct_uri(host, port, ssl) - protocol = ssl ? "ldaps" : "ldap" - URI.parse("#{protocol}://#{host}:#{port}").to_s - end end end end diff --git a/lib/omniauth-ldap/version.rb b/lib/omniauth-ldap/version.rb index 1531127..d5adc48 100644 --- a/lib/omniauth-ldap/version.rb +++ b/lib/omniauth-ldap/version.rb @@ -1,5 +1,5 @@ module OmniAuth module LDAP - VERSION = "1.0.3" + VERSION = "1.0.4" end end diff --git a/lib/omniauth/strategies/ldap.rb b/lib/omniauth/strategies/ldap.rb index c28628d..4b075e4 100644 --- a/lib/omniauth/strategies/ldap.rb +++ b/lib/omniauth/strategies/ldap.rb @@ -3,7 +3,6 @@ module OmniAuth module Strategies class LDAP - class MissingCredentialsError < StandardError; end include OmniAuth::Strategy @@config = { 'name' => 'cn', @@ -38,14 +37,10 @@ def request_phase def callback_phase @adaptor = OmniAuth::LDAP::Adaptor.new @options + return fail!(:missing_credentials) if missing_credentials? begin - # GITLAB security patch - # Dont allow blank password for ldap auth - if request['username'].nil? || request['username'].empty? || request['password'].nil? || request['password'].empty? - raise MissingCredentialsError.new("Missing login credentials") - end + @ldap_user_info = @adaptor.bind_as(:filter => filter(@adaptor), :size => 1, :password => request['password']) - @ldap_user_info = @adaptor.bind_as(:filter => Net::LDAP::Filter.eq(@adaptor.uid, @options[:name_proc].call(request['username'])),:size => 1, :password => request['password']) return fail!(:invalid_credentials) if !@ldap_user_info @user_info = self.class.map_user(@@config, @ldap_user_info) @@ -55,6 +50,14 @@ def callback_phase end end + def filter adaptor + if adaptor.filter and !adaptor.filter.empty? + Net::LDAP::Filter.construct(adaptor.filter % {username: @options[:name_proc].call(request['username'])}) + else + Net::LDAP::Filter.eq(adaptor.uid, @options[:name_proc].call(request['username'])) + end + end + uid { @user_info["uid"] } @@ -70,14 +73,14 @@ def self.map_user(mapper, object) mapper.each do |key, value| case value when String - user[key] = object[value.downcase.to_sym].first if object[value.downcase.to_sym] + user[key] = object[value.downcase.to_sym].first if object.respond_to? value.downcase.to_sym when Array - value.each {|v| (user[key] = object[v.downcase.to_sym].first; break;) if object[v.downcase.to_sym]} + value.each {|v| (user[key] = object[v.downcase.to_sym].first; break;) if object.respond_to? v.downcase.to_sym} when Hash value.map do |key1, value1| pattern = key1.dup value1.each_with_index do |v,i| - part = ''; v.collect(&:downcase).collect(&:to_sym).each {|v1| (part = object[v1].first; break;) if object[v1]} + part = ''; v.collect(&:downcase).collect(&:to_sym).each {|v1| (part = object[v1].first; break;) if object.respond_to? v1} pattern.gsub!("%#{i}",part||'') end user[key] = pattern @@ -86,6 +89,12 @@ def self.map_user(mapper, object) end user end + + protected + + def missing_credentials? + request['username'].nil? or request['username'].empty? or request['password'].nil? or request['password'].empty? + end # missing_credentials? end end end diff --git a/spec/omniauth-ldap/adaptor_spec.rb b/spec/omniauth-ldap/adaptor_spec.rb index b205c57..e6a304f 100644 --- a/spec/omniauth-ldap/adaptor_spec.rb +++ b/spec/omniauth-ldap/adaptor_spec.rb @@ -2,13 +2,13 @@ describe "OmniAuth::LDAP::Adaptor" do describe 'initialize' do - it 'should throw exception when must have field is not set' do #[:host, :port, :method, :bind_dn] - lambda { OmniAuth::LDAP::Adaptor.new({host: "192.168.1.145", method: 'plain'})}.should raise_error(ArgumentError) + lambda { OmniAuth::LDAP::Adaptor.new({host: "192.168.1.145", method: 'plain'})}.should raise_error(ArgumentError) end + it 'should throw exception when method is not supported' do - lambda { OmniAuth::LDAP::Adaptor.new({host: "192.168.1.145", method: 'myplain', uid: 'uid', port: 389, base: 'dc=com'})}.should raise_error(OmniAuth::LDAP::Adaptor::ConfigurationError) + lambda { OmniAuth::LDAP::Adaptor.new({host: "192.168.1.145", method: 'myplain', uid: 'uid', port: 389, base: 'dc=com'})}.should raise_error(OmniAuth::LDAP::Adaptor::ConfigurationError) end it 'should setup ldap connection with anonymous' do @@ -17,54 +17,59 @@ adaptor.connection.host.should == '192.168.1.145' adaptor.connection.port.should == 389 adaptor.connection.base.should == 'dc=intridea, dc=com' - adaptor.connection.instance_variable_get('@auth').should == {:method => :anonymous, :username => nil, :password => nil} + adaptor.connection.instance_variable_get('@auth').should == {:method => :anonymous, :username => nil, :password => nil} end + it 'should setup ldap connection with simple' do adaptor = OmniAuth::LDAP::Adaptor.new({host: "192.168.1.145", method: 'plain', base: 'dc=intridea, dc=com', port: 389, uid: 'sAMAccountName', bind_dn: 'bind_dn', password: 'password'}) adaptor.connection.should_not == nil adaptor.connection.host.should == '192.168.1.145' adaptor.connection.port.should == 389 adaptor.connection.base.should == 'dc=intridea, dc=com' - adaptor.connection.instance_variable_get('@auth').should == {:method => :simple, :username => 'bind_dn', :password => 'password'} - end + adaptor.connection.instance_variable_get('@auth').should == {:method => :simple, :username => 'bind_dn', :password => 'password'} + end + it 'should setup ldap connection with sasl-md5' do adaptor = OmniAuth::LDAP::Adaptor.new({host: "192.168.1.145", method: 'plain', base: 'dc=intridea, dc=com', port: 389, uid: 'sAMAccountName', try_sasl: true, sasl_mechanisms: ["DIGEST-MD5"], bind_dn: 'bind_dn', password: 'password'}) adaptor.connection.should_not == nil adaptor.connection.host.should == '192.168.1.145' adaptor.connection.port.should == 389 adaptor.connection.base.should == 'dc=intridea, dc=com' - adaptor.connection.instance_variable_get('@auth')[:method].should == :sasl - adaptor.connection.instance_variable_get('@auth')[:mechanism].should == 'DIGEST-MD5' - adaptor.connection.instance_variable_get('@auth')[:initial_credential].should == '' - adaptor.connection.instance_variable_get('@auth')[:challenge_response].should_not be_nil + adaptor.connection.instance_variable_get('@auth')[:method].should == :sasl + adaptor.connection.instance_variable_get('@auth')[:mechanism].should == 'DIGEST-MD5' + adaptor.connection.instance_variable_get('@auth')[:initial_credential].should == '' + adaptor.connection.instance_variable_get('@auth')[:challenge_response].should_not be_nil end + it 'should setup ldap connection with sasl-gss' do adaptor = OmniAuth::LDAP::Adaptor.new({host: "192.168.1.145", method: 'plain', base: 'dc=intridea, dc=com', port: 389, uid: 'sAMAccountName', try_sasl: true, sasl_mechanisms: ["GSS-SPNEGO"], bind_dn: 'bind_dn', password: 'password'}) adaptor.connection.should_not == nil adaptor.connection.host.should == '192.168.1.145' adaptor.connection.port.should == 389 adaptor.connection.base.should == 'dc=intridea, dc=com' - adaptor.connection.instance_variable_get('@auth')[:method].should == :sasl - adaptor.connection.instance_variable_get('@auth')[:mechanism].should == 'GSS-SPNEGO' - adaptor.connection.instance_variable_get('@auth')[:initial_credential].should =~ /^NTLMSSP/ - adaptor.connection.instance_variable_get('@auth')[:challenge_response].should_not be_nil + adaptor.connection.instance_variable_get('@auth')[:method].should == :sasl + adaptor.connection.instance_variable_get('@auth')[:mechanism].should == 'GSS-SPNEGO' + adaptor.connection.instance_variable_get('@auth')[:initial_credential].should =~ /^NTLMSSP/ + adaptor.connection.instance_variable_get('@auth')[:challenge_response].should_not be_nil end end - + describe 'bind_as' do let(:args) { {:filter => Net::LDAP::Filter.eq('sAMAccountName', 'username'), :password => 'password', :size => 1} } let(:rs) { Struct.new(:dn).new('new dn') } + it 'should bind simple' do adaptor = OmniAuth::LDAP::Adaptor.new({host: "192.168.1.126", method: 'plain', base: 'dc=score, dc=local', port: 389, uid: 'sAMAccountName', bind_dn: 'bind_dn', password: 'password'}) adaptor.connection.should_receive(:open).and_yield(adaptor.connection) - adaptor.connection.should_receive(:search).with(args).and_return([rs]) + adaptor.connection.should_receive(:search).with(args).and_return([rs]) adaptor.connection.should_receive(:bind).with({:username => 'new dn', :password => args[:password], :method => :simple}).and_return(true) adaptor.bind_as(args).should == rs end + it 'should bind sasl' do adaptor = OmniAuth::LDAP::Adaptor.new({host: "192.168.1.145", method: 'plain', base: 'dc=intridea, dc=com', port: 389, uid: 'sAMAccountName', try_sasl: true, sasl_mechanisms: ["GSS-SPNEGO"], bind_dn: 'bind_dn', password: 'password'}) adaptor.connection.should_receive(:open).and_yield(adaptor.connection) - adaptor.connection.should_receive(:search).with(args).and_return([rs]) + adaptor.connection.should_receive(:search).with(args).and_return([rs]) adaptor.connection.should_receive(:bind).and_return(true) adaptor.bind_as(args).should == rs end diff --git a/spec/omniauth/strategies/ldap_spec.rb b/spec/omniauth/strategies/ldap_spec.rb index 01a48bb..1c13329 100644 --- a/spec/omniauth/strategies/ldap_spec.rb +++ b/spec/omniauth/strategies/ldap_spec.rb @@ -12,7 +12,7 @@ # :password => 'password' class MyLdapProvider < OmniAuth::Strategies::LDAP; end - def app + let(:app) do Rack::Builder.new { use OmniAuth::Test::PhonySession use MyLdapProvider, :name => 'ldap', :title => 'MyLdap Form', :host => '192.168.1.145', :base => 'dc=score, dc=local', :name_proc => Proc.new {|name| name.gsub(/@.*$/,'')} @@ -20,7 +20,7 @@ def app }.to_app end - def session + let(:session) do last_request.env['rack.session'] end @@ -46,12 +46,12 @@ def session it 'should have a label of the form title' do last_response.body.scan('MyLdap Form').size.should > 1 end - end describe 'post /auth/ldap/callback' do before(:each) do - @adaptor = mock(OmniAuth::LDAP::Adaptor, {:uid => 'ping'}) + @adaptor = double(OmniAuth::LDAP::Adaptor, {:uid => 'ping'}) + @adaptor.stub(:filter) OmniAuth::LDAP::Adaptor.stub(:new).and_return(@adaptor) end @@ -60,10 +60,10 @@ def session @adaptor.stub(:bind_as).and_return(false) end - it 'should raise MissingCredentialsError' do + it 'should fail with missing_credentials' do post('/auth/ldap/callback', {}) last_response.should be_redirect - last_response.headers['Location'].should =~ %r{ldap_error} + last_response.headers['Location'].should =~ %r{missing_credentials} end it 'should redirect to error page' do @@ -78,22 +78,167 @@ def session last_response.should be_redirect last_response.headers['Location'].should =~ %r{ldap_error} end + + context "when username is not preset" do + it 'should redirect to error page' do + post('/auth/ldap/callback', {}) + + last_response.should be_redirect + last_response.headers['Location'].should =~ %r{missing_credentials} + end + end + + context "when username is empty" do + it 'should redirect to error page' do + post('/auth/ldap/callback', {:username => ""}) + + last_response.should be_redirect + last_response.headers['Location'].should =~ %r{missing_credentials} + end + end + + context "when username is present" do + context "and password is not preset" do + it 'should redirect to error page' do + post('/auth/ldap/callback', {:username => "ping"}) + + last_response.should be_redirect + last_response.headers['Location'].should =~ %r{missing_credentials} + end + end + + context "and password is empty" do + it 'should redirect to error page' do + post('/auth/ldap/callback', {:username => "ping", :password => ""}) + + last_response.should be_redirect + last_response.headers['Location'].should =~ %r{missing_credentials} + end + end + end + + context "when username and password are present" do + context "and bind on LDAP server failed" do + it 'should redirect to error page' do + post('/auth/ldap/callback', {:username => 'ping', :password => 'password'}) + + last_response.should be_redirect + last_response.headers['Location'].should =~ %r{invalid_credentials} + end + context 'and filter is set' do + it 'should bind with filter' do + @adaptor.stub(:filter).and_return('uid=%{username}') + Net::LDAP::Filter.should_receive(:construct).with('uid=ping') + post('/auth/ldap/callback', {:username => 'ping', :password => 'password'}) + + last_response.should be_redirect + last_response.headers['Location'].should =~ %r{invalid_credentials} + end + end + + end + + context "and communication with LDAP server caused an exception" do + before :each do + @adaptor.stub(:bind_as).and_throw(Exception.new('connection_error')) + end + + it 'should redirect to error page' do + post('/auth/ldap/callback', {:username => "ping", :password => "password"}) + + last_response.should be_redirect + last_response.headers['Location'].should =~ %r{ldap_error} + end + end + end end context 'success' do let(:auth_hash){ last_request.env['omniauth.auth'] } + before(:each) do - @adaptor.stub(:bind_as).and_return({:dn => ['cn=ping, dc=intridea, dc=com'], :mail => ['ping@intridea.com'], :givenname => ['Ping'], :sn => ['Yu'], - :telephonenumber => ['555-555-5555'], :mobile => ['444-444-4444'], :uid => ['ping'], :title => ['dev'], :address =>[ 'k street'], - :l => ['Washington'], :st => ['DC'], :co => ["U.S.A"], :postofficebox => ['20001'], :wwwhomepage => ['www.intridea.com'], - :jpegphoto => ['http://www.intridea.com/ping.jpg'], :description => ['omniauth-ldap']}) + @adaptor.stub(:filter) + @adaptor.stub(:bind_as).and_return(Net::LDAP::Entry.from_single_ldif_string( +%Q{dn: cn=ping, dc=intridea, dc=com +mail: ping@intridea.com +givenname: Ping +sn: Yu +telephonenumber: 555-555-5555 +mobile: 444-444-4444 +uid: ping +title: dev +address: k street +l: Washington +st: DC +co: U.S.A +postofficebox: 20001 +wwwhomepage: www.intridea.com +jpegphoto: http://www.intridea.com/ping.jpg +description: omniauth-ldap +} + )) + end + + it 'should not redirect to error page' do post('/auth/ldap/callback', {:username => 'ping', :password => 'password'}) + last_response.should_not be_redirect end - it 'should raise MissingCredentialsError' do - should_not raise_error OmniAuth::Strategies::LDAP::MissingCredentialsError + context 'and filter is set' do + it 'should bind with filter' do + @adaptor.stub(:filter).and_return('uid=%{username}') + Net::LDAP::Filter.should_receive(:construct).with('uid=ping') + post('/auth/ldap/callback', {:username => 'ping', :password => 'password'}) + + last_response.should_not be_redirect + end end - it 'should map user info' do + + it 'should map user info to Auth Hash' do + post('/auth/ldap/callback', {:username => 'ping', :password => 'password'}) + auth_hash.uid.should == 'cn=ping, dc=intridea, dc=com' + auth_hash.info.email.should == 'ping@intridea.com' + auth_hash.info.first_name.should == 'Ping' + auth_hash.info.last_name.should == 'Yu' + auth_hash.info.phone.should == '555-555-5555' + auth_hash.info.mobile.should == '444-444-4444' + auth_hash.info.nickname.should == 'ping' + auth_hash.info.title.should == 'dev' + auth_hash.info.location.should == 'k street, Washington, DC, U.S.A 20001' + auth_hash.info.url.should == 'www.intridea.com' + auth_hash.info.image.should == 'http://www.intridea.com/ping.jpg' + auth_hash.info.description.should == 'omniauth-ldap' + end + end + + context 'alternate fields' do + let(:auth_hash){ last_request.env['omniauth.auth'] } + + before(:each) do + @adaptor.stub(:filter) + @adaptor.stub(:bind_as).and_return(Net::LDAP::Entry.from_single_ldif_string( +%Q{dn: cn=ping, dc=intridea, dc=com +userprincipalname: ping@intridea.com +givenname: Ping +sn: Yu +telephonenumber: 555-555-5555 +mobile: 444-444-4444 +uid: ping +title: dev +address: k street +l: Washington +st: DC +co: U.S.A +postofficebox: 20001 +wwwhomepage: www.intridea.com +jpegphoto: http://www.intridea.com/ping.jpg +description: omniauth-ldap +} + )) + end + + it 'should map user info to Auth Hash' do + post('/auth/ldap/callback', {:username => 'ping', :password => 'password'}) auth_hash.uid.should == 'cn=ping, dc=intridea, dc=com' auth_hash.info.email.should == 'ping@intridea.com' auth_hash.info.first_name.should == 'Ping'