diff --git a/app/models/authenticator/base.rb b/app/models/authenticator/base.rb index 467af88c831..787502484e8 100644 --- a/app/models/authenticator/base.rb +++ b/app/models/authenticator/base.rb @@ -118,9 +118,8 @@ def authorize(taskid, username, *args) end matching_groups = match_groups(groups_for(identity)) - userid = userid_for(identity, username) - user = find_or_initialize_user(userid) - update_user_attributes(user, username, identity) + userid, user = find_or_initialize_user(identity, username) + update_user_attributes(user, userid, identity) user.miq_groups = matching_groups if matching_groups.empty? @@ -148,10 +147,12 @@ def authorize(taskid, username, *args) end end - def find_or_initialize_user(userid) + def find_or_initialize_user(identity, username) + userid = userid_for(identity, username) user = User.find_by_userid(userid) user ||= User.in_my_region.where('lower(userid) = ?', userid).order(:lastlogon).last - user || User.new(:userid => userid) + user ||= User.new(:userid => userid) + [userid, user] end def authenticate_with_http_basic(username, password, request = nil, options = {}) diff --git a/app/models/authenticator/httpd.rb b/app/models/authenticator/httpd.rb index 3bb6015df32..ece32f75a2a 100644 --- a/app/models/authenticator/httpd.rb +++ b/app/models/authenticator/httpd.rb @@ -43,10 +43,11 @@ def groups_for(identity) MiqGroup.strip_group_domains(membership_list) end - def update_user_attributes(user, _username, identity) + def update_user_attributes(user, username, identity) user_attrs, _membership_list = identity - user.userid = user_attrs[:username] + $audit_log.info("Updating userid from #{user.userid} to #{username}") if user.userid != username + user.userid = username user.first_name = user_attrs[:firstname] user.last_name = user_attrs[:lastname] user.email = user_attrs[:email] unless user_attrs[:email].blank? @@ -55,15 +56,48 @@ def update_user_attributes(user, _username, identity) user.name = user.userid if user.name.blank? end + def find_or_initialize_user(identity, username) + user_attrs, _membership_list = identity + return super if user_attrs[:domain].nil? + + 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 ||= find_userid_as_username(identity, username) + user ||= User.new(:userid => upn_username) + + [upn_username, user] + end + private + def find_userid_as_upn(upn_username) + user = User.find_by_userid(upn_username) + user || User.in_my_region.where('lower(userid) = ?', upn_username).order(:lastlogon).last + end + + def find_userid_as_username(identity, username) + userid = userid_for(identity, username) + user = User.find_by_userid(userid) + user ||= User.in_my_region.where('lower(userid) = ?', userid).order(:lastlogon).last + user + end + + def find_userid_as_distinguished_name(user_attrs) + dn_domain = user_attrs[:domain].downcase.split(".").map { |s| "dc=#{s}" }.join(",") + user = User.in_my_region.where("userid LIKE ?", "%=#{user_attrs[:username]},%,#{dn_domain}").last + user + end + def user_details_from_external_directory(username) ext_user_attrs = user_attrs_from_external_directory(username) user_attrs = {:username => username, :fullname => ext_user_attrs["displayname"], :firstname => ext_user_attrs["givenname"], :lastname => ext_user_attrs["sn"], - :email => ext_user_attrs["mail"]} + :email => ext_user_attrs["mail"], + :domain => ext_user_attrs["domainname"]} [user_attrs, MiqGroup.get_httpd_groups_by_user(username)] end @@ -72,7 +106,8 @@ def user_details_from_headers(username, request) :fullname => request.headers['X-REMOTE-USER-FULLNAME'], :firstname => request.headers['X-REMOTE-USER-FIRSTNAME'], :lastname => request.headers['X-REMOTE-USER-LASTNAME'], - :email => request.headers['X-REMOTE-USER-EMAIL']} + :email => request.headers['X-REMOTE-USER-EMAIL'], + :domain => request.headers['X-REMOTE-USER-DOMAIN']} [user_attrs, (request.headers['X-REMOTE-USER-GROUPS'] || '').split(/[;:]/)] end @@ -80,7 +115,7 @@ def user_attrs_from_external_directory(username) return unless username require "dbus" - attrs_needed = %w(mail givenname sn displayname) + attrs_needed = %w(mail givenname sn displayname domainname) sysbus = DBus.system_bus ifp_service = sysbus["org.freedesktop.sssd.infopipe"] diff --git a/spec/models/authenticator/httpd_spec.rb b/spec/models/authenticator/httpd_spec.rb index 0809c8bb1a1..7dbc1afedf3 100644 --- a/spec/models/authenticator/httpd_spec.rb +++ b/spec/models/authenticator/httpd_spec.rb @@ -65,6 +65,7 @@ def authenticate 'X-Remote-User-FirstName' => 'Alice', 'X-Remote-User-LastName' => 'Aardvark', 'X-Remote-User-Email' => 'alice@example.com', + 'X-Remote-User-Domain' => 'example.com', 'X-Remote-User-Groups' => user_groups, } end @@ -190,6 +191,78 @@ def authenticate end end + context "with potential for multiple user records" do + let(:dn) { 'cn=sally,ou=people,ou=prod,dc=example,dc=com' } + 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', + 'X-Remote-User-LastName' => 'Porsche', + 'X-Remote-User-Email' => 'Sally@example.com') + end + + context "when user record with userid in upn format already exists" do + let!(:sally_username) { FactoryGirl.create(:user, :userid => 'sAlly') } + let!(:sally_dn) { FactoryGirl.create(:user, :userid => dn) } + let!(:sally_upn) { FactoryGirl.create(:user, :userid => 'sAlly@example.com') } + + it "leaves user record with userid in username format unchanged" do + expect(-> { authenticate }).to_not change { sally_username.reload.userid } + end + + it "leaves user record with userid in distinguished name format unchanged" do + expect(-> { authenticate }).to_not change { sally_dn.reload.userid } + end + + it "downcases user record with userid in upn format" do + expect(-> { authenticate }) + .to change { sally_upn.reload.userid }.from("sAlly@example.com").to("sally@example.com") + end + end + + context "when user record with userid in upn format does not already exists" do + it "updates userid from username format to upn format" do + sally_username = FactoryGirl.create(:user, :userid => 'sally') + expect(-> { authenticate }).to change { sally_username.reload.userid }.from("sally").to("sally@example.com") + end + + it "updates userid from distinguished name format to upn format" do + sally_dn = FactoryGirl.create(:user, :userid => dn) + expect(-> { authenticate }).to change { sally_dn.reload.userid }.from(dn).to("sally@example.com") + end + + it "does not modify userid if already in upn format" do + sally_upn = FactoryGirl.create(:user, :userid => 'sally@example.com') + expect(-> { authenticate }).to_not change { sally_upn.reload.userid } + end + end + + context "when user record is for a different region" do + let(:my_region_number) { ApplicationRecord.my_region_number } + let(:other_region) { ApplicationRecord.my_region_number + 1 } + let(:other_region_id) { other_region * ApplicationRecord.rails_sequence_factor + 1 } + + it "does not modify the user record when userid is in username format" do + sally_username = FactoryGirl.create(:user, :userid => 'sally', :id => other_region_id) + expect(-> { authenticate }).to_not change { sally_username.reload.userid } + end + + it "does not modify the user record when userid is in distinguished name format" do + sally_dn = FactoryGirl.create(:user, :userid => dn, :id => other_region_id) + expect(-> { authenticate }).to_not change { sally_dn.reload.userid } + end + + it "does not modify the user record when userid is in already upn format" do + sally_upn = FactoryGirl.create(:user, :userid => 'sally@example.com', :id => other_region_id) + expect(-> { authenticate }).to_not change { sally_upn.reload.userid } + end + end + end + context "with unknown username in mixed case" do let(:username) { 'bOb' } let(:headers) do @@ -254,7 +327,7 @@ def authenticate end it "creates a new User" do - expect(-> { authenticate }).to change { User.where(:userid => 'bob').count }.from(0).to(1) + expect(-> { authenticate }).to change { User.where(:userid => 'bob@example.com').count }.from(0).to(1) end context "with no matching groups" do @@ -279,7 +352,7 @@ def authenticate expect(AuditEvent).to receive(:failure).with( :event => 'authorize', :userid => 'bob', - :message => "Authentication failed for userid bob, unable to match user's group membership to an EVM role", + :message => "Authentication failed for userid bob@example.com, unable to match user's group membership to an EVM role", ) authenticate end @@ -319,8 +392,8 @@ def authenticate 'X-Remote-User-Email' => 'sam@example.com') end - it "creates a new User with name set to the userid" do - expect(-> { authenticate }).to change { User.where(:name => 'sam').count }.from(0).to(1) + it "creates a new User with the userid set to the UPN" do + expect(-> { authenticate }).to change { User.where(:name => 'sam@example.com').count }.from(0).to(1) end end end @@ -345,17 +418,19 @@ def authenticate end it "should return user attributes hash for valid user" do - requested_attrs = %w(mail givenname sn displayname) + requested_attrs = %w(mail givenname sn displayname domainname) jdoe_attrs = [{"mail" => ["jdoe@example.com"], "givenname" => ["John"], "sn" => ["Doe"], - "displayname" => ["John Doe"]}] + "displayname" => ["John Doe"], + "domainname" => ["example.com"]}] expected_jdoe_attrs = {"mail" => "jdoe@example.com", "givenname" => "John", "sn" => "Doe", - "displayname" => "John Doe"} + "displayname" => "John Doe", + "domainname" => "example.com"} allow(@ifp_interface).to receive(:GetUserAttr).with('jdoe', requested_attrs).and_return(jdoe_attrs) diff --git a/spec/models/authenticator_spec.rb b/spec/models/authenticator_spec.rb index 82be7267016..b972d2521c9 100644 --- a/spec/models/authenticator_spec.rb +++ b/spec/models/authenticator_spec.rb @@ -22,7 +22,7 @@ it 'Updates the user groups when no matching groups' do expect(authenticator).to receive(:find_external_identity) - .and_return([{:username => user.userid, :fullname => user.name}, []]) + .and_return([{:username => user.userid, :fullname => user.name, :domain => "example.com"}, []]) authenticator.authorize(task.id, user.userid) expect(user.reload.miq_groups).to be_empty @@ -30,7 +30,7 @@ it 'Updates the user groups' do expect(authenticator).to receive(:find_external_identity) - .and_return([{:username => user.userid, :fullname => user.name}, groups.collect(&:name)]) + .and_return([{:username => user.userid, :fullname => user.name, :domain => "example.com"}, groups.collect(&:name)]) authenticator.authorize(task.id, user.userid) expect(user.reload.miq_groups).to match_array(groups)