Merge pull request #23723 from jrafanie/fix-freeipa-userid-email-creating-different-users

Fix freeipa userid or email logins creating different users

Author: Fryguy

app/models/authenticator/httpd.rb

@@ -123,6 +123,8 @@ def find_userid_as_distinguished_name(user_attrs)
       User.in_my_region.where("userid LIKE ?", "%=#{user_attrs[:username]},%,#{dn_domain}").last
     end
 
+    # TODO: If we now have the principal name via krbPrincipalName, we can use that instead of this method.
+    # In order to remove this method, we'd need to make sure all identity providers using httpd expose the principal name in the same format, even if it's just the username.
     def username_to_upn_name(user_attrs)
       return user_attrs[:username] if user_attrs[:domain].nil?
 
@@ -133,7 +135,8 @@ def username_to_upn_name(user_attrs)
 
     def user_details_from_external_directory(username)
       ext_user_attrs = user_attrs_from_external_directory(username)
-      user_attrs = {:username  => username,
+      principal_username = ext_user_attrs["krbPrincipalName"] && normalize_username(ext_user_attrs["krbPrincipalName"])
+      user_attrs = {:username  => principal_username || username,
                     :fullname  => ext_user_attrs["displayname"],
                     :firstname => ext_user_attrs["givenname"],
                     :lastname  => ext_user_attrs["sn"],
@@ -153,7 +156,8 @@ 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)
-      user_attrs = {:username  => username,
+      principal_username = user_headers['X-REMOTE-USER-PRINCIPAL'] && 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'],
                     :lastname  => user_headers['X-REMOTE-USER-LASTNAME'],
@@ -164,6 +168,8 @@ def user_details_from_headers(username, request)
     end
 
     private def user_headers_from_request(request)
+      # NOTE: X-REMOTE-USER-PRINCIPAL must be set in the headers by the sssd/apache configuration
+      # with a value of the krbPrincipalName or some other reasonable value based on the identity provider.
       %w[
         X-REMOTE-USER
         X-REMOTE-USER-FULLNAME
@@ -173,6 +179,7 @@ def user_details_from_headers(username, request)
         X-REMOTE-USER-DOMAIN
         X-REMOTE-USER-GROUPS
         X-REMOTE-USER-GROUP-DELIMITER
+        X-REMOTE-USER-PRINCIPAL
       ].each_with_object({}) do |k, h|
         h[k] = request.headers[k]&.force_encoding("UTF-8")
       end.delete_nils
@@ -198,7 +205,7 @@ def user_attrs_from_external_directory(username)
       end
     end
 
-    ATTRS_NEEDED = %w[mail givenname sn displayname domainname].freeze
+    ATTRS_NEEDED = %w[mail givenname sn displayname domainname krbPrincipalName].freeze
 
     def user_attrs_from_external_directory_via_dbus(username)
       return unless username

spec/models/authenticator/httpd_spec.rb

@@ -93,14 +93,19 @@
       {
         'X-Remote-User'           => 'cheshire',
         'X-Remote-User-FullName'  => 'Cheshire Cat',
-        'X-Remote-User-FirstName' => 'Chechire',
+        'X-Remote-User-FirstName' => 'Cheshire',
         'X-Remote-User-LastName'  => 'Cat',
         'X-Remote-User-Email'     => 'cheshire@example.com',
         'X-Remote-User-Domain'    => 'example.com',
         'X-Remote-User-Groups'    => 'wibble@fqdn:bubble@fqdn',
       }
     end
 
+    it "finds the user based on the downcased X-Remote-User-Principal header" do
+      headers['X-Remote-User-Principal'] = 'cheshire@EXAMPLE.COM'
+      expect(subject.lookup_by_identity('baduser', request)).to eq(cheshire)
+    end
+
     it "Handles missing request parameter" do
       expect(subject.lookup_by_identity('alice')).to eq(alice)
     end
@@ -125,22 +130,22 @@
   describe '#find_or_initialize_user' do
     let(:user_attrs_simple) do
       {:username  => "sal",
-        :fullname  => "Test User Sal",
-        :firstname => "Salvadore",
-        :lastname  => "Bigs",
-        :email     => "sal_email@example.com",
-        :domain    => "example.com"}
+       :fullname  => "Test User Sal",
+       :firstname => "Salvadore",
+       :lastname  => "Bigs",
+       :email     => "sal_email@example.com",
+       :domain    => "example.com"}
     end
 
     let(:identity_simple) { [user_attrs_simple, %w[mumble bumble bee]] }
 
     let(:user_attrs_upn) do
       {:username  => "sal@example.com",
-        :fullname  => "Test User Sal",
-        :firstname => "Salvadore",
-        :lastname  => "Bigs",
-        :email     => "sal_email@example.com",
-        :domain    => "example.com"}
+       :fullname  => "Test User Sal",
+       :firstname => "Salvadore",
+       :lastname  => "Bigs",
+       :email     => "sal_email@example.com",
+       :domain    => "example.com"}
     end
 
     let(:identity_upn) { [user_attrs_upn, %w[mumble bumble bee]] }
@@ -644,7 +649,7 @@ def authenticate
         end
       end
 
-      describe ".user_attrs_from_external_directory_via_dbus" do
+      context "with dbus mocks" do
         let(:ifp_interface) { double('ifp_interface') }
         before do
           require "dbus"
@@ -659,28 +664,73 @@ def authenticate
           allow(ifp_object).to receive(:[]).with("org.freedesktop.sssd.infopipe").and_return(ifp_interface)
         end
 
-        it "should return nil for unspecified user" do
-          expect(subject.send(:user_attrs_from_external_directory_via_dbus, nil)).to be_nil
-        end
+        describe ".user_attrs_from_external_directory_via_dbus" do
+          it "should return nil for unspecified user" do
+            expect(subject.send(:user_attrs_from_external_directory_via_dbus, nil)).to be_nil
+          end
+
+          it "should return user attributes hash for valid user" do
+            requested_attrs = %w[mail givenname sn displayname domainname krbPrincipalName]
 
-        it "should return user attributes hash for valid user" do
-          requested_attrs = %w[mail givenname sn displayname domainname]
+            jdoe_attrs = [{"mail"             => ["jdoe@example.com"],
+                           "givenname"        => ["John"],
+                           "sn"               => ["Doe"],
+                           "displayname"      => ["John Doe"],
+                           "domainname"       => ["ipa.test"],
+                           "krbPrincipalName" => ["jdoe@IPA.TEST"]}]
+
+            expected_jdoe_attrs = {"mail"             => "jdoe@example.com",
+                                   "givenname"        => "John",
+                                   "sn"               => "Doe",
+                                   "displayname"      => "John Doe",
+                                   "domainname"       => "ipa.test",
+                                   "krbPrincipalName" => "jdoe@IPA.TEST"}
+
+            allow(ifp_interface).to receive(:GetUserAttr).with('jdoe', requested_attrs).and_return(jdoe_attrs)
+
+            result = subject.send(:user_attrs_from_external_directory_via_dbus, 'jdoe')
+            expect(result).to eq(expected_jdoe_attrs)
+          end
 
-          jdoe_attrs = [{"mail"        => ["jdoe@example.com"],
-                         "givenname"   => ["John"],
-                         "sn"          => ["Doe"],
-                         "displayname" => ["John Doe"],
-                         "domainname"  => ["example.com"]}]
+          it "should handle missing krbPrincipalName gracefully" do
+            requested_attrs = %w[mail givenname sn displayname domainname krbPrincipalName]
 
-          expected_jdoe_attrs = {"mail"        => "jdoe@example.com",
-                                 "givenname"   => "John",
-                                 "sn"          => "Doe",
-                                 "displayname" => "John Doe",
-                                 "domainname"  => "example.com"}
+            jdoe_attrs = [{"mail"        => ["jdoe@example.com"],
+                           "givenname"   => ["John"],
+                           "sn"          => ["Doe"],
+                           "displayname" => ["John Doe"],
+                           "domainname"  => ["ipa.test"]}]
 
-          allow(ifp_interface).to receive(:GetUserAttr).with('jdoe', requested_attrs).and_return(jdoe_attrs)
+            expected_jdoe_attrs = {"mail"             => "jdoe@example.com",
+                                   "givenname"        => "John",
+                                   "sn"               => "Doe",
+                                   "displayname"      => "John Doe",
+                                   "domainname"       => "ipa.test",
+                                   "krbPrincipalName" => nil}
 
-          expect(subject.send(:user_attrs_from_external_directory_via_dbus, 'jdoe')).to eq(expected_jdoe_attrs)
+            allow(ifp_interface).to receive(:GetUserAttr).with('jdoe', requested_attrs).and_return(jdoe_attrs)
+
+            expect(subject.send(:user_attrs_from_external_directory_via_dbus, 'jdoe')).to eq(expected_jdoe_attrs)
+          end
+        end
+
+        describe "#user_details_from_external_directory" do
+          it "should normalize krbPrincipalName when present" 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" => ["jdoe@IPA.TEST"]}]
+
+            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@ipa.test")
+          end
         end
       end
     end