Merge pull request ManageIQ#15535 from jvlcek/bz1424618_dup_users

Converting userids to UPN format to avoid duplicate user records

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 = {})

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,15 +106,16 @@ 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
 
     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"]

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'     => '[email protected]',
+        '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'     => '[email protected]')
+      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 => '[email protected]') }
+
+        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("[email protected]").to("[email protected]")
+        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("[email protected]")
+        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("[email protected]")
+        end
+
+        it "does not modify userid if already in upn format" do
+          sally_upn = FactoryGirl.create(:user, :userid => '[email protected]')
+          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 => '[email protected]', :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'     => '[email protected]')
           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"        => ["[email protected]"],
                          "givenname"   => ["John"],
                          "sn"          => ["Doe"],
-                         "displayname" => ["John Doe"]}]
+                         "displayname" => ["John Doe"],
+                         "domainname"  => ["example.com"]}]
 
           expected_jdoe_attrs = {"mail"        => "[email protected]",
                                  "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)
 

spec/models/authenticator_spec.rb

@@ -22,15 +22,15 @@
 
     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
     end
 
     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)