Merge pull request #23139 from kbrock/CP4AIOPS-3113

CP4AIOPS-3113 Introduce configurable delimiter for LDAP group names

Gemfile

@@ -290,7 +290,7 @@ end
 
 group :appliance, :optional => true do
   gem "irb",                            "=1.4.1",            :require => false # Locked to same version as the installed RPM rubygem-irb-1.4.1-142.module_el9+787+b20bfeee.noarch so that we don't bundle our own
-  gem "manageiq-appliance_console",     "~>9.1", ">=9.1.1",  :require => false
+  gem "manageiq-appliance_console",     "~>10.0",            :require => false
   gem "rdoc",                                                :require => false # Needed for rails console
 end
 

app/models/authenticator/httpd.rb

@@ -4,6 +4,11 @@ def self.proper_name
       'External httpd'
     end
 
+    # @return group delimiter defined in settings
+    def self.group_delimiter
+      ::Settings.authentication.group_delimiter.presence
+    end
+
     def authorize_queue(username, request, options, *_args)
       log_auth_debug("authorize_queue(username=#{username}, options=#{options})")
 
@@ -146,7 +151,8 @@ def user_details_from_headers(username, request)
         user_headers.each { |k, v| log_auth_debug("  %-24{key} = \"%{val}\"" % {:key => k, :val => v}) }
       end
 
-      groups     = CGI.unescape(user_headers['X-REMOTE-USER-GROUPS'] || '').split(/[;:,]/)
+      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,
                     :fullname  => user_headers['X-REMOTE-USER-FULLNAME'],
                     :firstname => user_headers['X-REMOTE-USER-FIRSTNAME'],
@@ -166,6 +172,7 @@ def user_details_from_headers(username, request)
         X-REMOTE-USER-EMAIL
         X-REMOTE-USER-DOMAIN
         X-REMOTE-USER-GROUPS
+        X-REMOTE-USER-GROUP-DELIMITER
       ].each_with_object({}) do |k, h|
         h[k] = request.headers[k]&.force_encoding("UTF-8")
       end.delete_nils

config/settings.yml

@@ -16,6 +16,7 @@
   :ldaphost:
   :ldapport: '389'
   :mode: database
+  :group_delimiter:
   :max_failed_login_attempts: 3
   :locked_account_timeout: 2.minutes
   :search_timeout: 30

spec/models/authenticator/httpd_spec.rb

@@ -1,5 +1,5 @@
 RSpec.describe Authenticator::Httpd do
-  subject { Authenticator::Httpd.new(config) }
+  subject { Authenticator::Httpd.new(Settings.authentication.to_hash) }
 
   let(:config) { {:httpd_role => false} }
   let(:request) do
@@ -21,6 +21,7 @@
     # dummy_password_for_external_auth hook runs, and it needs to ask
     # Authenticator#uses_stored_password? whether it's allowed to do anything.
 
+    stub_settings_merge(:authentication => config)
     allow(User).to receive(:authenticator).and_return(subject)
 
     EvmSpecHelper.local_miq_server
@@ -172,6 +173,17 @@ def authenticate
       }
     end
 
+    let(:user_attrs) do
+      {
+        :username  => "testuser",
+        :fullname  => "Test User",
+        :firstname => "Alice",
+        :lastname  => "Aardvark",
+        :email     => "[email protected]",
+        :domain    => "example.com"
+      }
+    end
+
     context "with user details" do
       let!(:alice) { FactoryBot.create(:user, :userid => 'alice') }
 
@@ -702,38 +714,78 @@ def authenticate
         end
       end
 
-      context "using a comma separated group list" do
+      context "using default delimiters" do
         let(:config) { {:httpd_role => true} }
         let(:headers) do
           super().merge('X-Remote-User-Groups' => 'wibble@fqdn,bubble@fqdn')
         end
+
+        it "parses group names" do
+          expect(subject).to receive(:find_external_identity).with(username, user_attrs, ["wibble@fqdn", "bubble@fqdn"])
+          authenticate
+        end
+      end
+
+      context "using custom delimiter in settings" do
+        let(:config) { {:httpd_role => true, :group_delimiter => ","} }
+        let(:headers) do
+          super().merge('X-Remote-User-Groups' => 'wibble:wobble@fqdn,hobble;bubble@fqdn')
+        end
+
+        it "parses group names that contain characters from the default delimiters (:)" do
+          expect(subject).to receive(:find_external_identity).with(username, user_attrs, ["wibble:wobble@fqdn", "hobble;bubble@fqdn"])
+          authenticate
+        end
+      end
+
+      context "using custom delimiter in settings (despite a header)" do
+        let(:config) { {:httpd_role => true, :group_delimiter => "|"} }
+        let(:headers) do
+          super().merge('X-Remote-User-Groups' => 'wibble,wobble@fqdn|bubble@fqdn', 'X-Remote-User-Group-Delimiter' => ",")
+        end
         let(:user_attrs) do
-          {:username  => "testuser",
+          {
+            :username  => "testuser",
             :fullname  => "Test User",
             :firstname => "Alice",
             :lastname  => "Aardvark",
             :email     => "[email protected]",
-            :domain    => "example.com"}
+            :domain    => "example.com"
+          }
         end
 
-        it "handles a comma separated grouplist" do
-          expect(subject).to receive(:find_external_identity).with(username, user_attrs, ["wibble@fqdn", "bubble@fqdn"])
+        it "parses group names that contain characters from header" do
+          expect(subject).to receive(:find_external_identity).with(username, user_attrs, ["wibble,wobble@fqdn", "bubble@fqdn"])
           authenticate
         end
       end
 
-      context "when group names have escaped special characters" do
+      context "using header delimiter" do
         let(:config) { {:httpd_role => true} }
         let(:headers) do
-          super().merge('X-Remote-User-Groups' => CGI.escape('spécial_char@fqdn:moré@fqdn'))
+          super().merge('X-Remote-User-Groups' => 'wibble:wobble@fqdn,bubble@fqdn', 'X-Remote-User-Group-Delimiter' => ",")
         end
         let(:user_attrs) do
-          {:username  => "testuser",
+          {
+            :username  => "testuser",
             :fullname  => "Test User",
             :firstname => "Alice",
             :lastname  => "Aardvark",
             :email     => "[email protected]",
-            :domain    => "example.com"}
+            :domain    => "example.com"
+          }
+        end
+
+        it "parses group names that contain characters from default delimiter" do
+          expect(subject).to receive(:find_external_identity).with(username, user_attrs, ["wibble:wobble@fqdn", "bubble@fqdn"])
+          authenticate
+        end
+      end
+
+      context "when group names have escaped special characters" do
+        let(:config) { {:httpd_role => true} }
+        let(:headers) do
+          super().merge('X-Remote-User-Groups' => CGI.escape('spécial_char@fqdn:moré@fqdn'))
         end
 
         it "handles group names with escaped special characters" do
@@ -755,13 +807,26 @@ def authenticate
             'X-Remote-User-Groups'    => nil,
           }
         end
-        let(:user_attrs) do
-          {:username  => "testuser",
-            :fullname  => "Test User",
-            :firstname => "Alice",
-            :lastname  => "Aardvark",
-            :email     => "[email protected]",
-            :domain    => "example.com"}
+
+        it "handles nil group names" do
+          expect(subject).to receive(:find_external_identity).with(username, user_attrs, [])
+          authenticate
+        end
+      end
+
+      context "when there are no group names (but a group header)" do
+        let(:config) { {:httpd_role => true} }
+        let(:headers) do
+          {
+            'X-Remote-User'                 => 'testuser',
+            'X-Remote-User-FullName'        => 'Test User',
+            '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'          => nil,
+            'X-Remote-User-Group-Delimiter' => nil
+          }
         end
 
         it "handles nil group names" do