Merge pull request ManageIQ#23727 from jrafanie/freeipa-userid-email-cleanup

Freeipa userid email cleanup

Author: Fryguy

app/models/authenticator/httpd.rb

@@ -135,7 +135,7 @@ def username_to_upn_name(user_attrs)
 
     def user_details_from_external_directory(username)
       ext_user_attrs = user_attrs_from_external_directory(username)
-      principal_username = ext_user_attrs["krbPrincipalName"] && normalize_username(ext_user_attrs["krbPrincipalName"])
+      principal_username = ext_user_attrs["krbPrincipalName"].presence && normalize_username(ext_user_attrs["krbPrincipalName"])
       user_attrs = {:username  => principal_username || username,
                     :fullname  => ext_user_attrs["displayname"],
                     :firstname => ext_user_attrs["givenname"],
@@ -156,7 +156,7 @@ def user_details_from_headers(username, request)
 
       delimiter  = self.class.group_delimiter || user_headers['X-REMOTE-USER-GROUP-DELIMITER'].presence || /[;:,]/
       groups     = CGI.unescape(user_headers['X-REMOTE-USER-GROUPS'] || '').split(delimiter)
-      principal_username = user_headers['X-REMOTE-USER-PRINCIPAL'] && normalize_username(user_headers['X-REMOTE-USER-PRINCIPAL'])
+      principal_username = user_headers['X-REMOTE-USER-PRINCIPAL'].presence && normalize_username(user_headers['X-REMOTE-USER-PRINCIPAL'])
       user_attrs = {:username  => principal_username || username,
                     :fullname  => user_headers['X-REMOTE-USER-FULLNAME'],
                     :firstname => user_headers['X-REMOTE-USER-FIRSTNAME'],

spec/models/authenticator/httpd_spec.rb

@@ -12,25 +12,13 @@
   let(:username) { request.headers["X-Remote-User"] }
 
   before do
-    # If anything goes looking for the currently configured
-    # Authenticator during any of these tests, we'd really rather they
-    # found the one we're working on.
-    #
-    # This specifically comes up when we auto-create a new user from an
-    # external auth system: they get saved without a password, so User's
-    # dummy_password_for_external_auth hook runs, and it needs to ask
-    # Authenticator#uses_stored_password? whether it's allowed to do anything.
-
+    # Stub authenticator to prevent User.dummy_password_for_external_auth from calling wrong authenticator
     stub_settings_merge(:authentication => config)
     allow(User).to receive(:authenticator).and_return(subject)
-
     EvmSpecHelper.local_miq_server
-  end
 
-  before do
     FactoryBot.create(:miq_group, :description => 'wibble')
     FactoryBot.create(:miq_group, :description => 'wobble')
-
     allow(MiqLdap).to receive(:using_ldap?) { false }
   end
 
@@ -106,6 +94,16 @@
       expect(subject.lookup_by_identity('baduser', request)).to eq(cheshire)
     end
 
+    it "falls back to X-Remote-User when X-Remote-User-Principal is empty string" do
+      headers['X-Remote-User-Principal'] = ''
+      expect(subject.lookup_by_identity('cheshire', request)).to eq(cheshire)
+    end
+
+    it "falls back to X-Remote-User when X-Remote-User-Principal is whitespace" do
+      headers['X-Remote-User-Principal'] = '   '
+      expect(subject.lookup_by_identity('cheshire', request)).to eq(cheshire)
+    end
+
     it "Handles missing request parameter" do
       expect(subject.lookup_by_identity('alice')).to eq(alice)
     end
@@ -731,6 +729,53 @@ def authenticate
             user_attrs, _groups = subject.send(:user_details_from_external_directory, 'jdoe')
             expect(user_attrs[:username]).to eq("jdoe@ipa.test")
           end
+
+          it "should fall back to provided username when krbPrincipalName is empty string" do
+            requested_attrs = %w[mail givenname sn displayname domainname krbPrincipalName]
+
+            jdoe_attrs = [{"mail"             => ["jdoe@example.com"],
+                           "givenname"        => ["John"],
+                           "sn"               => ["Doe"],
+                           "displayname"      => ["John Doe"],
+                           "domainname"       => ["ipa.test"],
+                           "krbPrincipalName" => [""]}]
+
+            allow(ifp_interface).to receive(:GetUserAttr).with('jdoe', requested_attrs).and_return(jdoe_attrs)
+            allow(MiqGroup).to receive(:get_httpd_groups_by_user).with('jdoe').and_return([])
+
+            user_attrs, _groups = subject.send(:user_details_from_external_directory, 'jdoe')
+            expect(user_attrs[:username]).to eq("jdoe")
+          end
+
+          context "integration test: multiple login formats produce same normalized username" do
+            let(:expected_normalized_username) { "flast@ipa.test" }
+            let(:requested_attrs) { %w[mail givenname sn displayname domainname krbPrincipalName] }
+            let(:user_attrs_response) do
+              [{"mail"             => ["first.last@example.com"],
+                "givenname"        => ["First"],
+                "sn"               => ["Last"],
+                "displayname"      => ["First Last"],
+                "domainname"       => ["ipa.test"],
+                "krbPrincipalName" => ["flast@IPA.TEST"]}]
+            end
+
+            before do
+              allow(MiqGroup).to receive(:get_httpd_groups_by_user).and_return([])
+            end
+
+            shared_examples "normalizes to principal name" do |login_format|
+              it "normalizes #{login_format}" do
+                allow(ifp_interface).to receive(:GetUserAttr).with(login_format, requested_attrs).and_return(user_attrs_response)
+                user_attrs, _groups = subject.send(:user_details_from_external_directory, login_format)
+                expect(user_attrs[:username]).to eq(expected_normalized_username)
+              end
+            end
+
+            include_examples "normalizes to principal name", "first.last@example.com"
+            include_examples "normalizes to principal name", "flast@IPA.TEST"
+            include_examples "normalizes to principal name", "flast@ipa.test"
+            include_examples "normalizes to principal name", "flast"
+          end
         end
       end
     end