Skip to content

Commit

Permalink
Always check for userid as UPN
Browse files Browse the repository at this point in the history
Always check for userid in UPN format, even when "Get User Groups
from External Authentication (httpd)" is not checked

https://bugzilla.redhat.com/show_bug.cgi?id=1496979
  • Loading branch information
jvlcek committed Sep 29, 2017
1 parent cbc2859 commit 2ba8f0c
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 16 deletions.
4 changes: 2 additions & 2 deletions app/models/authenticator/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
9 changes: 9 additions & 0 deletions app/models/authenticator/httpd.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
51 changes: 37 additions & 14 deletions spec/models/authenticator/httpd_spec.rb
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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

Expand All @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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',
Expand Down

0 comments on commit 2ba8f0c

Please sign in to comment.