Merge pull request #16069 from jvlcek/bz1496979_ext_auth_no_get_group…

…s_upn

Always check for userid as UPN

app/models/authenticator/base.rb

@@ -58,7 +58,7 @@ def authenticate(username, password, request = nil, options = {})
  else
  # If role_mode == database we will only use the external system for authentication. Also, the user must exist in our database
  # otherwise we will fail authentication
- user_or_taskid = lookup_by_identity(username)
+ user_or_taskid = lookup_by_identity(username, request)
  user_or_taskid ||= autocreate_user(username)
 
  unless user_or_taskid
@@ -166,7 +166,7 @@ def authenticate_with_http_basic(username, password, request = nil, options = {}
  [!!result, username]
  end
 
- def lookup_by_identity(username)
+ def lookup_by_identity(username, *_args)
  case_insensitive_find_by_userid(username)
  end
 

app/models/authenticator/httpd.rb

@@ -70,6 +70,15 @@ def find_or_initialize_user(identity, username)
  [upn_username, user]
  end
 
+ def lookup_by_identity(username, request)
+ user_attrs, _membership_list = user_details_from_headers(username, request)
+ upn_username = "#{user_attrs[:username]}@#{user_attrs[:domain]}".downcase
+
+ user = find_userid_as_upn(upn_username)
+ user ||= find_userid_as_distinguished_name(user_attrs)
+ user || case_insensitive_find_by_userid(username)
+ end
+
  private
 
  def find_userid_as_upn(upn_username)

spec/models/authenticator/httpd_spec.rb

@@ -1,7 +1,16 @@
 describe Authenticator::Httpd do
  subject { Authenticator::Httpd.new(config) }
  let!(:alice) { FactoryGirl.create(:user, :userid => 'alice') }
+ let!(:cheshire) { FactoryGirl.create(:user, :userid => 'cheshire@example.com') }
+ let(:user_groups) { 'wibble@fqdn:bubble@fqdn' }
  let(:config) { {:httpd_role => false} }
+ let(:request) do
+ env = {}
+ headers.each do |k, v|
+ env["HTTP_#{k.upcase.tr '-', '_'}"] = v if v
+ end
+ ActionDispatch::Request.new(Rack::MockRequest.env_for("/", env))
+ end
 
  before(:each) do
  # If anything goes looking for the currently configured
@@ -36,12 +45,37 @@
  end
 
  describe '#lookup_by_identity' do
- it "finds existing users" do
- expect(subject.lookup_by_identity('alice')).to eq(alice)
+ let(:dn) { 'cn=towmater,ou=people,ou=prod,dc=example,dc=com' }
+ let!(:towmater_dn) { FactoryGirl.create(:user, :userid => dn) }
+
+ let(:headers) do
+ {
+ 'X-Remote-User' => username,
+ 'X-Remote-User-FullName' => 'Cheshire Cat',
+ 'X-Remote-User-FirstName' => 'Chechire',
+ 'X-Remote-User-LastName' => 'Cat',
+ 'X-Remote-User-Email' => 'cheshire@example.com',
+ 'X-Remote-User-Domain' => 'example.com',
+ 'X-Remote-User-Groups' => user_groups,
+ }
+ end
+
+ let(:username) { 'cheshire' }
+
+ it "finds existing users as username" do
+ expect(subject.lookup_by_identity('alice', request)).to eq(alice)
+ end
+
+ it "finds existing users as UPN" do
+ expect(subject.lookup_by_identity('cheshire', request)).to eq(cheshire)
+ end
+
+ it "finds existing users as distinguished name" do
+ expect(subject.lookup_by_identity('towmater', request)).to eq(towmater_dn)
  end
 
  it "doesn't create new users" do
- expect(subject.lookup_by_identity('bob')).to be_nil
+ expect(subject.lookup_by_identity('bob', request)).to be_nil
  end
  end
 
@@ -50,14 +84,6 @@ def authenticate
  subject.authenticate(username, nil, request)
  end
 
- let(:request) do
- env = {}
- headers.each do |k, v|
- env["HTTP_#{k.upcase.tr '-', '_'}"] = v if v
- end
- ActionDispatch::Request.new(Rack::MockRequest.env_for("/", env))
- end
-
  let(:headers) do
  {
  'X-Remote-User' => username,
@@ -71,7 +97,6 @@ def authenticate
  end
 
  let(:username) { 'alice' }
- let(:user_groups) { 'wibble@fqdn:bubble@fqdn' }
 
  context "with user details" do
  context "using local authorization" do
@@ -196,8 +221,6 @@ def authenticate
  let(:config) { {:httpd_role => true} }
 
  let(:username) { 'saLLy' }
- let(:user_groups) { 'wibble@fqdn:bubble@fqdn' }
-
  let(:headers) do
  super().merge('X-Remote-User-FullName' => 'Sally Porsche',
  'X-Remote-User-FirstName' => 'Sally',