From 2ba8f0c7617e8446ab87b7d9514772845b1824db Mon Sep 17 00:00:00 2001
From: Joe VLcek <jvlcek@redhat.com>
Date: Thu, 28 Sep 2017 18:03:13 -0400
Subject: [PATCH] Always check for userid as UPN

Always check for userid in UPN format, even when "Get User Groups
from External Authentication (httpd)" is not checked

https://bugzilla.redhat.com/show_bug.cgi?id=1496979
---
 app/models/authenticator/base.rb        |  4 +-
 app/models/authenticator/httpd.rb       |  9 +++++
 spec/models/authenticator/httpd_spec.rb | 51 ++++++++++++++++++-------
 3 files changed, 48 insertions(+), 16 deletions(-)

diff --git a/app/models/authenticator/base.rb b/app/models/authenticator/base.rb
index 6012738edc0..69b4b097b45 100644
--- a/app/models/authenticator/base.rb
+++ b/app/models/authenticator/base.rb
@@ -58,7 +58,7 @@ def authenticate(username, password, request = nil, options = {})
           else
             # If role_mode == database we will only use the external system for authentication. Also, the user must exist in our database
             # otherwise we will fail authentication
-            user_or_taskid = lookup_by_identity(username)
+            user_or_taskid = lookup_by_identity(username, request)
             user_or_taskid ||= autocreate_user(username)
 
             unless user_or_taskid
@@ -166,7 +166,7 @@ def authenticate_with_http_basic(username, password, request = nil, options = {}
       [!!result, username]
     end
 
-    def lookup_by_identity(username)
+    def lookup_by_identity(username, *_args)
       case_insensitive_find_by_userid(username)
     end
 
diff --git a/app/models/authenticator/httpd.rb b/app/models/authenticator/httpd.rb
index 9144ce92508..04155950bc4 100644
--- a/app/models/authenticator/httpd.rb
+++ b/app/models/authenticator/httpd.rb
@@ -70,6 +70,15 @@ def find_or_initialize_user(identity, username)
       [upn_username, user]
     end
 
+    def lookup_by_identity(username, request)
+      user_attrs, _membership_list = user_details_from_headers(username, request)
+      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 ||  case_insensitive_find_by_userid(username)
+    end
+
     private
 
     def find_userid_as_upn(upn_username)
diff --git a/spec/models/authenticator/httpd_spec.rb b/spec/models/authenticator/httpd_spec.rb
index 1c2bc667a97..42559f311d4 100644
--- a/spec/models/authenticator/httpd_spec.rb
+++ b/spec/models/authenticator/httpd_spec.rb
@@ -1,7 +1,16 @@
 describe Authenticator::Httpd do
   subject { Authenticator::Httpd.new(config) }
   let!(:alice) { FactoryGirl.create(:user, :userid => 'alice') }
+  let!(:cheshire) { FactoryGirl.create(:user, :userid => 'cheshire@example.com') }
+  let(:user_groups) { 'wibble@fqdn:bubble@fqdn' }
   let(:config) { {:httpd_role => false} }
+  let(:request) do
+    env = {}
+    headers.each do |k, v|
+      env["HTTP_#{k.upcase.tr '-', '_'}"] = v if v
+    end
+    ActionDispatch::Request.new(Rack::MockRequest.env_for("/", env))
+  end
 
   before(:each) do
     # If anything goes looking for the currently configured
@@ -36,12 +45,37 @@
   end
 
   describe '#lookup_by_identity' do
-    it "finds existing users" do
-      expect(subject.lookup_by_identity('alice')).to eq(alice)
+    let(:dn) { 'cn=towmater,ou=people,ou=prod,dc=example,dc=com' }
+    let!(:towmater_dn) { FactoryGirl.create(:user, :userid => dn) }
+
+    let(:headers) do
+      {
+        'X-Remote-User'           => username,
+        'X-Remote-User-FullName'  => 'Cheshire Cat',
+        'X-Remote-User-FirstName' => 'Chechire',
+        'X-Remote-User-LastName'  => 'Cat',
+        'X-Remote-User-Email'     => 'cheshire@example.com',
+        'X-Remote-User-Domain'    => 'example.com',
+        'X-Remote-User-Groups'    => user_groups,
+      }
+    end
+
+    let(:username) { 'cheshire' }
+
+    it "finds existing users as username" do
+      expect(subject.lookup_by_identity('alice', request)).to eq(alice)
+    end
+
+    it "finds existing users as UPN" do
+      expect(subject.lookup_by_identity('cheshire', request)).to eq(cheshire)
+    end
+
+    it "finds existing users as distinguished name" do
+      expect(subject.lookup_by_identity('towmater', request)).to eq(towmater_dn)
     end
 
     it "doesn't create new users" do
-      expect(subject.lookup_by_identity('bob')).to be_nil
+      expect(subject.lookup_by_identity('bob', request)).to be_nil
     end
   end
 
@@ -50,14 +84,6 @@ def authenticate
       subject.authenticate(username, nil, request)
     end
 
-    let(:request) do
-      env = {}
-      headers.each do |k, v|
-        env["HTTP_#{k.upcase.tr '-', '_'}"] = v if v
-      end
-      ActionDispatch::Request.new(Rack::MockRequest.env_for("/", env))
-    end
-
     let(:headers) do
       {
         'X-Remote-User'           => username,
@@ -71,7 +97,6 @@ def authenticate
     end
 
     let(:username) { 'alice' }
-    let(:user_groups) { 'wibble@fqdn:bubble@fqdn' }
 
     context "with user details" do
       context "using local authorization" do
@@ -196,8 +221,6 @@ def authenticate
       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',