Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always check for userid as UPN #16069

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 => '[email protected]') }
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' => '[email protected]',
'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