From 8234bfae4a0b9c9db73a9529bca3a837c37e0dd2 Mon Sep 17 00:00:00 2001 From: Joe VLcek Date: Thu, 6 Jul 2017 17:11:00 -0400 Subject: [PATCH 1/7] Converting userids to UPN format to avoid duplicate user records https://bugzilla.redhat.com/show_bug.cgi?id=1424618 --- app/models/authenticator/base.rb | 11 ++-- app/models/authenticator/httpd.rb | 47 +++++++++++-- spec/models/authenticator/httpd_spec.rb | 88 +++++++++++++++++++++++-- spec/models/authenticator_spec.rb | 4 +- 4 files changed, 131 insertions(+), 19 deletions(-) diff --git a/app/models/authenticator/base.rb b/app/models/authenticator/base.rb index 467af88c831..f829d1c0d1f 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..6ef4f53dc74 100644 --- a/app/models/authenticator/httpd.rb +++ b/app/models/authenticator/httpd.rb @@ -43,10 +43,10 @@ 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] + 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 +55,51 @@ 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(identity, username) 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_username(identity, username, upn_username) + user ||= find_userid_as_distinguished_name(user_attrs, upn_username) + user ||= User.new(:userid => upn_username) + + [upn_username, user] + end + private + def find_userid_as_upn(upn_username) + User.find_by_userid(upn_username) + end + + def find_userid_as_username(identity, username, upn_username) + userid = userid_for(identity, username) + user = User.find_by_userid(userid) + user ||= User.in_my_region.where('lower(userid) = ?', userid).order(:lastlogon).last + $audit_log.info("Updating userid from #{user.userid} to #{upn_username}") unless user.blank? + + user + end + + def find_userid_as_distinguished_name(user_attrs, upn_username) + 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 + $audit_log.info("Updating userid from #{user.userid} to #{upn_username}") unless user.blank? + + 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 +108,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 +117,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..97e9d4493ab 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,77 @@ 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 "leaves user record with userid in upn format unchanged" do + expect(-> { authenticate }).to_not change { sally_upn.reload.userid } + 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) { Classification.my_region_number } + let(:other_region) { Classification.my_region_number + 1 } + let(:other_region_id) { other_region * Classification.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 +326,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 +351,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 +391,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 +417,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) From a5be8625dab9cfc3a8c53db209e3b54c2cad9aee Mon Sep 17 00:00:00 2001 From: Joe VLcek Date: Thu, 10 Aug 2017 09:15:01 -0400 Subject: [PATCH 2/7] Fix rubocop ExtraSpacing warning --- app/models/authenticator/base.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/authenticator/base.rb b/app/models/authenticator/base.rb index f829d1c0d1f..787502484e8 100644 --- a/app/models/authenticator/base.rb +++ b/app/models/authenticator/base.rb @@ -151,7 +151,7 @@ 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 From e1cbe5cf3463f697f7da9cfaa0ecfcf5437433c5 Mon Sep 17 00:00:00 2001 From: Joe VLcek Date: Fri, 11 Aug 2017 08:49:42 -0400 Subject: [PATCH 3/7] Handle mixed case UPN userid format --- app/models/authenticator/httpd.rb | 5 +++-- spec/models/authenticator/httpd_spec.rb | 9 +++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/app/models/authenticator/httpd.rb b/app/models/authenticator/httpd.rb index 6ef4f53dc74..0a95b9afe89 100644 --- a/app/models/authenticator/httpd.rb +++ b/app/models/authenticator/httpd.rb @@ -62,8 +62,8 @@ def find_or_initialize_user(identity, username) upn_username = "#{user_attrs[:username]}@#{user_attrs[:domain].downcase}" user = find_userid_as_upn(upn_username) - user ||= find_userid_as_username(identity, username, upn_username) user ||= find_userid_as_distinguished_name(user_attrs, upn_username) + user ||= find_userid_as_username(identity, username, upn_username) user ||= User.new(:userid => upn_username) [upn_username, user] @@ -72,7 +72,8 @@ def find_or_initialize_user(identity, username) private def find_userid_as_upn(upn_username) - User.find_by_userid(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, upn_username) diff --git a/spec/models/authenticator/httpd_spec.rb b/spec/models/authenticator/httpd_spec.rb index 97e9d4493ab..1fde6c804e0 100644 --- a/spec/models/authenticator/httpd_spec.rb +++ b/spec/models/authenticator/httpd_spec.rb @@ -206,9 +206,9 @@ def authenticate end context "when user record with userid in upn format already exists" do - let!(:sally_username) { FactoryGirl.create(:user, :userid => 'sally') } + 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') } + 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 } @@ -218,8 +218,9 @@ def authenticate expect(-> { authenticate }).to_not change { sally_dn.reload.userid } end - it "leaves user record with userid in upn format unchanged" do - expect(-> { authenticate }).to_not change { sally_upn.reload.userid } + 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 From 629f083a9a69014e819cb37d693d9fbd05bd5678 Mon Sep 17 00:00:00 2001 From: Joe VLcek Date: Fri, 11 Aug 2017 09:33:30 -0400 Subject: [PATCH 4/7] Improve userid case testing --- spec/models/authenticator/httpd_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/authenticator/httpd_spec.rb b/spec/models/authenticator/httpd_spec.rb index 1fde6c804e0..272f5fd59a6 100644 --- a/spec/models/authenticator/httpd_spec.rb +++ b/spec/models/authenticator/httpd_spec.rb @@ -195,7 +195,7 @@ def authenticate let(:dn) { 'cn=sally,ou=people,ou=prod,dc=example,dc=com' } let(:config) { {:httpd_role => true} } - let(:username) { 'sally' } + let(:username) { 'saLLy' } let(:user_groups) { 'wibble@fqdn:bubble@fqdn' } let(:headers) do From 9e1877dcf302a612e9f7e57a4a9b1ab1a0079cae Mon Sep 17 00:00:00 2001 From: Joe VLcek Date: Mon, 21 Aug 2017 17:56:09 -0400 Subject: [PATCH 5/7] Remove uneeded super paramas and downcase entire usernane --- app/models/authenticator/httpd.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/authenticator/httpd.rb b/app/models/authenticator/httpd.rb index 0a95b9afe89..6fc2bd21d4b 100644 --- a/app/models/authenticator/httpd.rb +++ b/app/models/authenticator/httpd.rb @@ -57,9 +57,9 @@ def update_user_attributes(user, username, identity) def find_or_initialize_user(identity, username) user_attrs, _membership_list = identity - return super(identity, username) if user_attrs[:domain].nil? + return super if user_attrs[:domain].nil? - upn_username = "#{user_attrs[:username]}@#{user_attrs[:domain].downcase}" + upn_username = "#{user_attrs[:username]}@#{user_attrs[:domain]}".downcase user = find_userid_as_upn(upn_username) user ||= find_userid_as_distinguished_name(user_attrs, upn_username) From 309299c8813b5703bb10da71929dc9c0efaf650a Mon Sep 17 00:00:00 2001 From: Joe VLcek Date: Fri, 25 Aug 2017 16:22:24 -0400 Subject: [PATCH 6/7] Fix spec to get the current region from the ApplicationRecord --- spec/models/authenticator/httpd_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/models/authenticator/httpd_spec.rb b/spec/models/authenticator/httpd_spec.rb index 272f5fd59a6..7dbc1afedf3 100644 --- a/spec/models/authenticator/httpd_spec.rb +++ b/spec/models/authenticator/httpd_spec.rb @@ -242,9 +242,9 @@ def authenticate end context "when user record is for a different region" do - let(:my_region_number) { Classification.my_region_number } - let(:other_region) { Classification.my_region_number + 1 } - let(:other_region_id) { other_region * Classification.rails_sequence_factor + 1 } + 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) From 48ae7fb583cbec4d8f1ea676af27b0bc0460950a Mon Sep 17 00:00:00 2001 From: Joe VLcek Date: Sun, 27 Aug 2017 11:57:26 -0400 Subject: [PATCH 7/7] Refactor logging of updates to userid --- app/models/authenticator/httpd.rb | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/app/models/authenticator/httpd.rb b/app/models/authenticator/httpd.rb index 6fc2bd21d4b..ece32f75a2a 100644 --- a/app/models/authenticator/httpd.rb +++ b/app/models/authenticator/httpd.rb @@ -46,6 +46,7 @@ def groups_for(identity) def update_user_attributes(user, username, identity) user_attrs, _membership_list = identity + $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] @@ -62,8 +63,8 @@ def find_or_initialize_user(identity, username) upn_username = "#{user_attrs[:username]}@#{user_attrs[:domain]}".downcase user = find_userid_as_upn(upn_username) - user ||= find_userid_as_distinguished_name(user_attrs, upn_username) - user ||= find_userid_as_username(identity, username, 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] @@ -76,20 +77,16 @@ def find_userid_as_upn(upn_username) user || User.in_my_region.where('lower(userid) = ?', upn_username).order(:lastlogon).last end - def find_userid_as_username(identity, username, upn_username) + 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 - $audit_log.info("Updating userid from #{user.userid} to #{upn_username}") unless user.blank? - user end - def find_userid_as_distinguished_name(user_attrs, upn_username) + 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 - $audit_log.info("Updating userid from #{user.userid} to #{upn_username}") unless user.blank? - user end