From 52f02f5177ab10e328eb9df31aa51c1ffa36c0d8 Mon Sep 17 00:00:00 2001
From: Joe VLcek <jvlcek@redhat.com>
Date: Thu, 2 Mar 2017 13:34:28 -0500
Subject: [PATCH] Ensure user name is set even when common LDAP attributes are
 missing.

Addresses:
https://bugzilla.redhat.com/show_bug.cgi?id=1400567
---
 app/models/authenticator/httpd.rb       |  4 ++-
 app/models/authenticator/ldap.rb        |  4 ++-
 spec/models/authenticator/httpd_spec.rb | 28 +++++++++++++++++
 spec/models/authenticator/ldap_spec.rb  | 40 +++++++++++++++++++++++++
 4 files changed, 74 insertions(+), 2 deletions(-)

diff --git a/app/models/authenticator/httpd.rb b/app/models/authenticator/httpd.rb
index 7dac5404a64..3bb6015df32 100644
--- a/app/models/authenticator/httpd.rb
+++ b/app/models/authenticator/httpd.rb
@@ -47,10 +47,12 @@ def update_user_attributes(user, _username, identity)
       user_attrs, _membership_list = identity
 
       user.userid     = user_attrs[:username]
-      user.name       = user_attrs[:fullname]
       user.first_name = user_attrs[:firstname]
       user.last_name  = user_attrs[:lastname]
       user.email      = user_attrs[:email] unless user_attrs[:email].blank?
+      user.name       = user_attrs[:fullname]
+      user.name       = "#{user.first_name} #{user.last_name}" if user.name.blank?
+      user.name       = user.userid if user.name.blank?
     end
 
     private
diff --git a/app/models/authenticator/ldap.rb b/app/models/authenticator/ldap.rb
index 350cce0758f..75d29b8efd7 100644
--- a/app/models/authenticator/ldap.rb
+++ b/app/models/authenticator/ldap.rb
@@ -131,11 +131,13 @@ def groups_for(obj)
 
     def update_user_attributes(user, _username, lobj)
       user.userid     = ldap.normalize(ldap.get_attr(lobj, :userprincipalname) || ldap.get_attr(lobj, :dn))
-      user.name       = ldap.get_attr(lobj, :displayname)
       user.first_name = ldap.get_attr(lobj, :givenname)
       user.last_name  = ldap.get_attr(lobj, :sn)
       email           = ldap.get_attr(lobj, :mail)
       user.email      = email unless email.blank?
+      user.name       = ldap.get_attr(lobj, :displayname)
+      user.name       = "#{user.first_name} #{user.last_name}" if user.name.blank?
+      user.name       = user.userid if user.name.blank?
     end
 
     REQUIRED_LDAP_USER_PROXY_KEYS = [:basedn, :bind_dn, :bind_pwd, :ldaphost, :ldapport, :mode]
diff --git a/spec/models/authenticator/httpd_spec.rb b/spec/models/authenticator/httpd_spec.rb
index 5567ed1708f..31fc64d8d4f 100644
--- a/spec/models/authenticator/httpd_spec.rb
+++ b/spec/models/authenticator/httpd_spec.rb
@@ -295,6 +295,34 @@ def authenticate
             expect(MiqTask.status_error?(task.status)).to be_truthy
           end
         end
+
+        context "when fullname is blank" do
+          let(:username) { 'betty' }
+          let(:headers) do
+            super().merge('X-Remote-User-FullName'  => '',
+                          'X-Remote-User-FirstName' => 'Betty',
+                          'X-Remote-User-LastName'  => 'Boop',
+                          'X-Remote-User-Email'     => 'betty@example.com')
+          end
+
+          it "creates a new User with name set to FirstName + LastName" do
+            expect(-> { authenticate }).to change { User.where(:name => 'Betty Boop').count }.from(0).to(1)
+          end
+        end
+
+        context "when fullname, firstname and lastname are blank" do
+          let(:username) { 'sam' }
+          let(:headers) do
+            super().merge('X-Remote-User-FullName'  => '',
+                          'X-Remote-User-FirstName' => '',
+                          'X-Remote-User-LastName'  => '',
+                          '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)
+          end
+        end
       end
 
       describe ".user_attrs_from_external_directory" do
diff --git a/spec/models/authenticator/ldap_spec.rb b/spec/models/authenticator/ldap_spec.rb
index 17ca1ce65be..4b6f70248ea 100644
--- a/spec/models/authenticator/ldap_spec.rb
+++ b/spec/models/authenticator/ldap_spec.rb
@@ -62,6 +62,8 @@ def normalize(dn)
       'rootdn' => {:password => 'verysecret'},
       'alice'  => alice_data,
       'bob'    => bob_data,
+      'betty'  => betty_data,
+      'sam'    => sam_data,
     }
   end
   let(:alice_data) do
@@ -86,6 +88,28 @@ def normalize(dn)
       :groups            => %w(wibble bubble),
     }
   end
+  let(:betty_data) do
+    {
+      :userprincipalname => 'betty',
+      :password          => 'secret',
+      :displayname       => nil,
+      :givenname         => 'Betty',
+      :sn                => 'Builderson',
+      :mail              => 'betty@example.com',
+      :groups            => %w(wibble bubble),
+    }
+  end
+  let(:sam_data) do
+    {
+      :userprincipalname => 'sam',
+      :password          => 'secret',
+      :displayname       => nil,
+      :givenname         => nil,
+      :sn                => nil,
+      :mail              => 'sam@example.com',
+      :groups            => %w(wibble bubble),
+    }
+  end
 
   before(:each) do
     allow(MiqLdap).to receive(:new) { FakeLdap.new(user_data) }
@@ -462,6 +486,22 @@ def authenticate
             expect(MiqTask.status_error?(task.status)).to be_truthy
           end
         end
+
+        context "when display name is blank" do
+          let(:username) { 'betty' }
+
+          it "creates a new User with name set to givenname + sn" do
+            expect(-> { authenticate }).to change { User.where(:name => 'Betty Builderson').count }.from(0).to(1)
+          end
+        end
+
+        context "when display name, givenname and sn are blank" do
+          let(:username) { 'sam' }
+
+          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)
+          end
+        end
       end
     end
   end