From 3a3b835bc8efca391eea6c34960b743a62b0c2bb Mon Sep 17 00:00:00 2001
From: Alberto Bellotti <abellott@redhat.com>
Date: Wed, 30 Nov 2016 18:36:14 -0500
Subject: [PATCH 1/4] Enhance API to authorize users with system token
 authenticated requests.

This capability is needed for MiqLdap and External Authentication
Authentication Modes so users are authorized and auto-created
with system token authenticated requests.

- Added a User.admin?(userid) method
- Added a User.authorize_by_userid method
- Add support for authorize_user_by_userid for ldap, ldaps
- Add support for authorize_user_by_userid for httpd
- Add support to fetch user attrs for httpd

This solves the following BZ's:
https://bugzilla.redhat.com/show_bug.cgi?id=1400349
https://bugzilla.redhat.com/show_bug.cgi?id=1400350
---
 .../api/base_controller/authentication.rb     |  4 +-
 app/models/authenticator.rb                   | 20 ++++++--
 app/models/authenticator/amazon.rb            |  2 +-
 app/models/authenticator/httpd.rb             | 51 ++++++++++++++++---
 app/models/authenticator/ldap.rb              |  6 ++-
 app/models/user.rb                            |  9 ++++
 6 files changed, 78 insertions(+), 14 deletions(-)

diff --git a/app/controllers/api/base_controller/authentication.rb b/app/controllers/api/base_controller/authentication.rb
index 082b408b4aa..46ece63830b 100644
--- a/app/controllers/api/base_controller/authentication.rb
+++ b/app/controllers/api/base_controller/authentication.rb
@@ -61,7 +61,7 @@ def user_settings
       end
 
       def userid_to_userobj(userid)
-        User.find_by_userid(userid)
+        User.lookup_by_identity(userid)
       end
 
       def authorize_user_group(user_obj)
@@ -146,6 +146,8 @@ def authenticate_with_system_token(x_miq_token)
         validate_system_token_server(@miq_token_hash[:server_guid])
         validate_system_token_timestamp(@miq_token_hash[:timestamp])
 
+        User.authorize_by_userid(@miq_token_hash[:userid])
+
         @auth_user     = @miq_token_hash[:userid]
         @auth_user_obj = userid_to_userobj(@auth_user)
 
diff --git a/app/models/authenticator.rb b/app/models/authenticator.rb
index 05534d6c113..6c44fcb8d80 100644
--- a/app/models/authenticator.rb
+++ b/app/models/authenticator.rb
@@ -34,9 +34,23 @@ def uses_stored_password?
       false
     end
 
+    def authorize_user_by_userid?
+      false
+    end
+
+    def authorize_user_by_userid(userid)
+      return unless authorize_user_by_userid?
+      options = {
+        :require_user   => true,
+        :authorize_only => true
+      }
+      authenticate(userid, "", {}, options)
+    end
+
     def authenticate(username, password, request = nil, options = {})
       options = options.dup
       options[:require_user] ||= false
+      options[:authorize_only] ||= false
       fail_message = _("Authentication failed")
 
       user_or_taskid = nil
@@ -46,11 +60,11 @@ def authenticate(username, password, request = nil, options = {})
 
         username = normalize_username(username)
 
-        if _authenticate(username, password, request)
+        if options[:authorize_only] || _authenticate(username, password, request)
           AuditEvent.success(audit.merge(:message => "User #{username} successfully validated by #{self.class.proper_name}"))
 
           if authorize?
-            user_or_taskid = authorize_queue(username, request)
+            user_or_taskid = authorize_queue(username, request, 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
@@ -205,7 +219,7 @@ def encrypt_ldap_password(config)
       config[:bind_pwd] = MiqPassword.try_encrypt(config[:bind_pwd])
     end
 
-    def authorize_queue(username, _request, *args)
+    def authorize_queue(username, _request, _options, *args)
       task = MiqTask.create(:name => "#{self.class.proper_name} User Authorization of '#{username}'", :userid => username)
       if authorize_queue?
         encrypt_ldap_password(config) if MiqLdap.using_ldap?
diff --git a/app/models/authenticator/amazon.rb b/app/models/authenticator/amazon.rb
index e28286db6f8..c8eaab4366a 100644
--- a/app/models/authenticator/amazon.rb
+++ b/app/models/authenticator/amazon.rb
@@ -58,7 +58,7 @@ def _authenticate(username, password, _request)
       end
     end
 
-    def find_external_identity(username)
+    def find_external_identity(username, *_args)
       # Amazon IAM will be used for authentication and role assignment
       _log.info("AWS key: [#{config[:amazon_key]}]")
       _log.info(" User: [#{username}]")
diff --git a/app/models/authenticator/httpd.rb b/app/models/authenticator/httpd.rb
index b74554c9cac..cd533ebb7fa 100644
--- a/app/models/authenticator/httpd.rb
+++ b/app/models/authenticator/httpd.rb
@@ -4,15 +4,25 @@ def self.proper_name
       'External httpd'
     end
 
-    def authorize_queue(username, request)
-      user_attrs = {:username  => username,
-                    :fullname  => request.headers['X-REMOTE-USER-FULLNAME'],
-                    :firstname => request.headers['X-REMOTE-USER-FIRSTNAME'],
-                    :lastname  => request.headers['X-REMOTE-USER-LASTNAME'],
-                    :email     => request.headers['X-REMOTE-USER-EMAIL']}
-      membership_list = (request.headers['X-REMOTE-USER-GROUPS'] || '').split(/[;:]/)
+    def authorize_queue(username, request, options, *_args)
+      if options[:authorize_only] == true
+        ext_user_attrs = get_user_attrs(username)
+        user_attrs = {:username  => username,
+                      :fullname  => ext_user_attrs["displayname"],
+                      :firstname => ext_user_attrs["givenname"],
+                      :lastname  => ext_user_attrs["sn"],
+                      :email     => ext_user_attrs["mail"]}
+        membership_list = MiqGroup.get_httpd_groups_by_user(username)
+      else
+        user_attrs = {:username  => username,
+                      :fullname  => request.headers['X-REMOTE-USER-FULLNAME'],
+                      :firstname => request.headers['X-REMOTE-USER-FIRSTNAME'],
+                      :lastname  => request.headers['X-REMOTE-USER-LASTNAME'],
+                      :email     => request.headers['X-REMOTE-USER-EMAIL']}
+        membership_list = (request.headers['X-REMOTE-USER-GROUPS'] || '').split(/[;:]/)
+      end
 
-      super(username, request, user_attrs, membership_list)
+      super(username, request, {}, user_attrs, membership_list)
     end
 
     # We don't talk to an external system in #find_external_identity /
@@ -21,6 +31,31 @@ def authorize_queue?
       false
     end
 
+    def authorize_user_by_userid?
+      true
+    end
+
+    def get_user_attrs(username)
+      return unless username
+      require "dbus"
+
+      attrs_needed = %w(mail givenname sn displayname)
+
+      sysbus = DBus.system_bus
+      ifp_service   = sysbus["org.freedesktop.sssd.infopipe"]
+      ifp_object    = ifp_service.object "/org/freedesktop/sssd/infopipe"
+      ifp_object.introspect
+      ifp_interface = ifp_object["org.freedesktop.sssd.infopipe"]
+      begin
+        user_attrs = ifp_interface.GetUserAttr(username, attrs_needed).first
+      rescue => err
+        raise _("Unable to get attributes for external user %{user_name} - %{error}") %
+              {:user_name => username, :error => err}
+      end
+
+      attrs_needed.each_with_object({}) { |attr, hash| hash[attr] = Array(user_attrs[attr]).first }
+    end
+
     def _authenticate(_username, _password, request)
       request.present? &&
         request.headers['X-REMOTE-USER'].present?
diff --git a/app/models/authenticator/ldap.rb b/app/models/authenticator/ldap.rb
index 57c79d94ab0..de0514f0d7f 100644
--- a/app/models/authenticator/ldap.rb
+++ b/app/models/authenticator/ldap.rb
@@ -9,6 +9,10 @@ def lookup_by_identity(username)
         find_or_create_by_ldap(username)
     end
 
+    def authorize_user_by_userid?
+      true
+    end
+
     private
 
     def ldap
@@ -76,7 +80,7 @@ def _authenticate(username, password, _request)
         ldap_bind(username, password)
     end
 
-    def find_external_identity(username)
+    def find_external_identity(username, *_args)
       # Ldap will be used for authentication and role assignment
       _log.info("Bind DN: [#{config[:bind_dn]}]")
       _log.info(" User FQDN: [#{username}]")
diff --git a/app/models/user.rb b/app/models/user.rb
index 473b429a488..278588f3553 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -163,6 +163,11 @@ def self.lookup_by_identity(username)
     authenticator(username).lookup_by_identity(username)
   end
 
+  def self.authorize_by_userid(userid)
+    return if userid.blank? || admin?(userid)
+    authenticator(userid).authorize_user_by_userid(userid)
+  end
+
   def logoff
     self.lastlogoff = Time.now.utc
     save
@@ -194,6 +199,10 @@ def miq_groups=(groups)
   end
 
   def admin?
+    self.class.admin?(userid)
+  end
+
+  def self.admin?(userid)
     userid == "admin"
   end
 

From a5efeadb494e71ba42b00c2a81b204209927e836 Mon Sep 17 00:00:00 2001
From: Alberto Bellotti <abellott@redhat.com>
Date: Fri, 9 Dec 2016 09:53:02 -0500
Subject: [PATCH 2/4] PR Review updates, Adding rspecs

- Rename authorize_user_by_userid? to can_authorize_user_by_userid?
- Break out authorize_queue into smaller methods
- Added Rspecs
---
 app/models/authenticator.rb             |  4 +-
 app/models/authenticator/httpd.rb       | 85 ++++++++++++++-----------
 app/models/authenticator/ldap.rb        |  2 +-
 spec/models/authenticator/httpd_spec.rb | 44 +++++++++++++
 spec/models/authenticator/ldap_spec.rb  |  6 ++
 spec/models/user_spec.rb                | 20 ++++++
 6 files changed, 120 insertions(+), 41 deletions(-)

diff --git a/app/models/authenticator.rb b/app/models/authenticator.rb
index 6c44fcb8d80..9134153c93c 100644
--- a/app/models/authenticator.rb
+++ b/app/models/authenticator.rb
@@ -34,12 +34,12 @@ def uses_stored_password?
       false
     end
 
-    def authorize_user_by_userid?
+    def can_authorize_user_by_userid?
       false
     end
 
     def authorize_user_by_userid(userid)
-      return unless authorize_user_by_userid?
+      return unless can_authorize_user_by_userid?
       options = {
         :require_user   => true,
         :authorize_only => true
diff --git a/app/models/authenticator/httpd.rb b/app/models/authenticator/httpd.rb
index cd533ebb7fa..742317eeeff 100644
--- a/app/models/authenticator/httpd.rb
+++ b/app/models/authenticator/httpd.rb
@@ -5,22 +5,12 @@ def self.proper_name
     end
 
     def authorize_queue(username, request, options, *_args)
-      if options[:authorize_only] == true
-        ext_user_attrs = get_user_attrs(username)
-        user_attrs = {:username  => username,
-                      :fullname  => ext_user_attrs["displayname"],
-                      :firstname => ext_user_attrs["givenname"],
-                      :lastname  => ext_user_attrs["sn"],
-                      :email     => ext_user_attrs["mail"]}
-        membership_list = MiqGroup.get_httpd_groups_by_user(username)
-      else
-        user_attrs = {:username  => username,
-                      :fullname  => request.headers['X-REMOTE-USER-FULLNAME'],
-                      :firstname => request.headers['X-REMOTE-USER-FIRSTNAME'],
-                      :lastname  => request.headers['X-REMOTE-USER-LASTNAME'],
-                      :email     => request.headers['X-REMOTE-USER-EMAIL']}
-        membership_list = (request.headers['X-REMOTE-USER-GROUPS'] || '').split(/[;:]/)
-      end
+      user_attrs, membership_list =
+        if options[:authorize_only] == true
+          get_user_details_by_userid(username)
+        else
+          get_user_details_from_headers(username, request)
+        end
 
       super(username, request, {}, user_attrs, membership_list)
     end
@@ -31,31 +21,10 @@ def authorize_queue?
       false
     end
 
-    def authorize_user_by_userid?
+    def can_authorize_user_by_userid?
       true
     end
 
-    def get_user_attrs(username)
-      return unless username
-      require "dbus"
-
-      attrs_needed = %w(mail givenname sn displayname)
-
-      sysbus = DBus.system_bus
-      ifp_service   = sysbus["org.freedesktop.sssd.infopipe"]
-      ifp_object    = ifp_service.object "/org/freedesktop/sssd/infopipe"
-      ifp_object.introspect
-      ifp_interface = ifp_object["org.freedesktop.sssd.infopipe"]
-      begin
-        user_attrs = ifp_interface.GetUserAttr(username, attrs_needed).first
-      rescue => err
-        raise _("Unable to get attributes for external user %{user_name} - %{error}") %
-              {:user_name => username, :error => err}
-      end
-
-      attrs_needed.each_with_object({}) { |attr, hash| hash[attr] = Array(user_attrs[attr]).first }
-    end
-
     def _authenticate(_username, _password, request)
       request.present? &&
         request.headers['X-REMOTE-USER'].present?
@@ -83,5 +52,45 @@ def update_user_attributes(user, _username, identity)
       user.last_name  = user_attrs[:lastname]
       user.email      = user_attrs[:email] unless user_attrs[:email].blank?
     end
+
+    def get_user_details_by_userid(username)
+      ext_user_attrs = get_user_attrs(username)
+      user_attrs = {:username  => username,
+                    :fullname  => ext_user_attrs["displayname"],
+                    :firstname => ext_user_attrs["givenname"],
+                    :lastname  => ext_user_attrs["sn"],
+                    :email     => ext_user_attrs["mail"]}
+      [user_attrs, MiqGroup.get_httpd_groups_by_user(username)]
+    end
+
+    def get_user_details_from_headers(username, request)
+      user_attrs = {:username  => username,
+                    :fullname  => request.headers['X-REMOTE-USER-FULLNAME'],
+                    :firstname => request.headers['X-REMOTE-USER-FIRSTNAME'],
+                    :lastname  => request.headers['X-REMOTE-USER-LASTNAME'],
+                    :email     => request.headers['X-REMOTE-USER-EMAIL']}
+      [user_attrs, (request.headers['X-REMOTE-USER-GROUPS'] || '').split(/[;:]/)]
+    end
+
+    def get_user_attrs(username)
+      return unless username
+      require "dbus"
+
+      attrs_needed = %w(mail givenname sn displayname)
+
+      sysbus = DBus.system_bus
+      ifp_service   = sysbus["org.freedesktop.sssd.infopipe"]
+      ifp_object    = ifp_service.object "/org/freedesktop/sssd/infopipe"
+      ifp_object.introspect
+      ifp_interface = ifp_object["org.freedesktop.sssd.infopipe"]
+      begin
+        user_attrs = ifp_interface.GetUserAttr(username, attrs_needed).first
+      rescue => err
+        raise _("Unable to get attributes for external user %{user_name} - %{error}") %
+              {:user_name => username, :error => err}
+      end
+
+      attrs_needed.each_with_object({}) { |attr, hash| hash[attr] = Array(user_attrs[attr]).first }
+    end
   end
 end
diff --git a/app/models/authenticator/ldap.rb b/app/models/authenticator/ldap.rb
index de0514f0d7f..3f16eada8a6 100644
--- a/app/models/authenticator/ldap.rb
+++ b/app/models/authenticator/ldap.rb
@@ -9,7 +9,7 @@ def lookup_by_identity(username)
         find_or_create_by_ldap(username)
     end
 
-    def authorize_user_by_userid?
+    def can_authorize_user_by_userid?
       true
     end
 
diff --git a/spec/models/authenticator/httpd_spec.rb b/spec/models/authenticator/httpd_spec.rb
index 96c01f476e0..2e26b63d4d9 100644
--- a/spec/models/authenticator/httpd_spec.rb
+++ b/spec/models/authenticator/httpd_spec.rb
@@ -29,6 +29,12 @@
     end
   end
 
+  describe '.can_authorize_user_by_userid?' do
+    it "is true" do
+      expect(subject.can_authorize_user_by_userid?).to be_truthy
+    end
+  end
+
   describe '#lookup_by_identity' do
     it "finds existing users" do
       expect(subject.lookup_by_identity('alice')).to eq(alice)
@@ -290,6 +296,44 @@ def authenticate
           end
         end
       end
+
+      describe ".get_user_attrs" do
+        before do
+          require "dbus"
+          sysbus = double('sysbus')
+          ifp_service = double('ifp_service')
+          ifp_object  = double('ifp_object')
+          @ifp_interface = double('ifp_interface')
+
+          allow(DBus).to receive(:system_bus).and_return(sysbus)
+          allow(sysbus).to receive(:[]).with("org.freedesktop.sssd.infopipe").and_return(ifp_service)
+          allow(ifp_service).to receive(:object).with("/org/freedesktop/sssd/infopipe").and_return(ifp_object)
+          allow(ifp_object).to receive(:introspect)
+          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.get_user_attrs(nil)).to be_nil
+        end
+
+        it "should return user attributes hash for valid user" do
+          requested_attrs = %w(mail givenname sn displayname)
+
+          jdoe_attrs = [{"mail"        => ["jdoe@example.com"],
+                         "givenname"   => ["John"],
+                         "sn"          => ["Doe"],
+                         "displayname" => ["John Doe"]}]
+
+          expected_jdoe_attrs = {"mail"        => "jdoe@example.com",
+                                 "givenname"   => "John",
+                                 "sn"          => "Doe",
+                                 "displayname" => "John Doe"}
+
+          allow(@ifp_interface).to receive(:GetUserAttr).with('jdoe', requested_attrs).and_return(jdoe_attrs)
+
+          expect(subject.get_user_attrs('jdoe')).to eq(expected_jdoe_attrs)
+        end
+      end
     end
   end
 end
diff --git a/spec/models/authenticator/ldap_spec.rb b/spec/models/authenticator/ldap_spec.rb
index 6b3c6a66ddb..d4861097401 100644
--- a/spec/models/authenticator/ldap_spec.rb
+++ b/spec/models/authenticator/ldap_spec.rb
@@ -98,6 +98,12 @@ def normalize(dn)
     end
   end
 
+  describe ".can_authorize_user_by_userid?" do
+    it "is true" do
+      expect(subject.can_authorize_user_by_userid?).to be_truthy
+    end
+  end
+
   describe '#lookup_by_identity' do
     it "finds existing users" do
       expect(subject.lookup_by_identity('alice')).to eq(alice)
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 6ab3985ebf3..059a7410002 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -527,4 +527,24 @@
       expect(User.super_admin).to be_super_admin_user
     end
   end
+
+  context ".admin?" do
+    it "admin? succeeds with admin account" do
+      expect(User.admin?("admin")).to be_truthy
+    end
+
+    it "admin? fails with non-admin account" do
+      expect(User.admin?("regular_user")).to be_falsey
+    end
+  end
+
+  context ".authorize_by_userid" do
+    it "returns nil with blank userid" do
+      expect(User.authorize_by_userid("")).to be_nil
+    end
+
+    it "returns nil with admin userid" do
+      expect(User.authorize_by_userid("admin")).to be_nil
+    end
+  end
 end

From 8709e421f7b5e537d7ed8baaaa86030169d8d632 Mon Sep 17 00:00:00 2001
From: Alberto Bellotti <abellott@redhat.com>
Date: Mon, 12 Dec 2016 11:50:38 -0500
Subject: [PATCH 3/4] Fixed issue with group matching

- Fixed issue where users could not be created when groups are matched from other regions
---
 app/models/authenticator.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/authenticator.rb b/app/models/authenticator.rb
index 9134153c93c..d52b42c3e6f 100644
--- a/app/models/authenticator.rb
+++ b/app/models/authenticator.rb
@@ -268,7 +268,7 @@ def match_groups(external_group_names)
       return [] if external_group_names.empty?
       external_group_names = external_group_names.collect(&:downcase)
 
-      internal_groups = MiqGroup.order(:sequence).to_a
+      internal_groups = MiqGroup.in_my_region.order(:sequence).to_a
 
       external_group_names.each { |g| _log.debug("External Group: #{g}") }
       internal_groups.each      { |g| _log.debug("Internal Group: #{g.description.downcase}") }

From 68160e72680a5a43e6d79a6c965dd5f894c48d8a Mon Sep 17 00:00:00 2001
From: Alberto Bellotti <abellott@redhat.com>
Date: Mon, 12 Dec 2016 14:46:23 -0500
Subject: [PATCH 4/4] PR Review changes - clarifying method names

- Updated method names to reflect the what and not the how.
---
 .../api/base_controller/authentication.rb      |  2 +-
 app/models/authenticator.rb                    | 15 ++++++---------
 app/models/authenticator/httpd.rb              | 18 ++++++++++--------
 app/models/authenticator/ldap.rb               |  2 +-
 app/models/user.rb                             |  4 ++--
 spec/models/authenticator/httpd_spec.rb        | 10 +++++-----
 spec/models/authenticator/ldap_spec.rb         |  4 ++--
 spec/models/user_spec.rb                       |  6 +++---
 8 files changed, 30 insertions(+), 31 deletions(-)

diff --git a/app/controllers/api/base_controller/authentication.rb b/app/controllers/api/base_controller/authentication.rb
index 46ece63830b..0c3f989ae77 100644
--- a/app/controllers/api/base_controller/authentication.rb
+++ b/app/controllers/api/base_controller/authentication.rb
@@ -146,7 +146,7 @@ def authenticate_with_system_token(x_miq_token)
         validate_system_token_server(@miq_token_hash[:server_guid])
         validate_system_token_timestamp(@miq_token_hash[:timestamp])
 
-        User.authorize_by_userid(@miq_token_hash[:userid])
+        User.authorize_user(@miq_token_hash[:userid])
 
         @auth_user     = @miq_token_hash[:userid]
         @auth_user_obj = userid_to_userobj(@auth_user)
diff --git a/app/models/authenticator.rb b/app/models/authenticator.rb
index d52b42c3e6f..b3a6ed562c9 100644
--- a/app/models/authenticator.rb
+++ b/app/models/authenticator.rb
@@ -34,17 +34,13 @@ def uses_stored_password?
       false
     end
 
-    def can_authorize_user_by_userid?
+    def user_authorizable_without_authentication?
       false
     end
 
-    def authorize_user_by_userid(userid)
-      return unless can_authorize_user_by_userid?
-      options = {
-        :require_user   => true,
-        :authorize_only => true
-      }
-      authenticate(userid, "", {}, options)
+    def authorize_user(userid)
+      return unless user_authorizable_without_authentication?
+      authenticate(userid, "", {}, {:require_user => true, :authorize_only => true})
     end
 
     def authenticate(username, password, request = nil, options = {})
@@ -60,7 +56,8 @@ def authenticate(username, password, request = nil, options = {})
 
         username = normalize_username(username)
 
-        if options[:authorize_only] || _authenticate(username, password, request)
+        authenticated = options[:authorize_only] || _authenticate(username, password, request)
+        if authenticated
           AuditEvent.success(audit.merge(:message => "User #{username} successfully validated by #{self.class.proper_name}"))
 
           if authorize?
diff --git a/app/models/authenticator/httpd.rb b/app/models/authenticator/httpd.rb
index 742317eeeff..7dac5404a64 100644
--- a/app/models/authenticator/httpd.rb
+++ b/app/models/authenticator/httpd.rb
@@ -6,10 +6,10 @@ def self.proper_name
 
     def authorize_queue(username, request, options, *_args)
       user_attrs, membership_list =
-        if options[:authorize_only] == true
-          get_user_details_by_userid(username)
+        if options[:authorize_only]
+          user_details_from_external_directory(username)
         else
-          get_user_details_from_headers(username, request)
+          user_details_from_headers(username, request)
         end
 
       super(username, request, {}, user_attrs, membership_list)
@@ -21,7 +21,7 @@ def authorize_queue?
       false
     end
 
-    def can_authorize_user_by_userid?
+    def user_authorizable_without_authentication?
       true
     end
 
@@ -53,8 +53,10 @@ def update_user_attributes(user, _username, identity)
       user.email      = user_attrs[:email] unless user_attrs[:email].blank?
     end
 
-    def get_user_details_by_userid(username)
-      ext_user_attrs = get_user_attrs(username)
+    private
+
+    def user_details_from_external_directory(username)
+      ext_user_attrs = user_attrs_from_external_directory(username)
       user_attrs = {:username  => username,
                     :fullname  => ext_user_attrs["displayname"],
                     :firstname => ext_user_attrs["givenname"],
@@ -63,7 +65,7 @@ def get_user_details_by_userid(username)
       [user_attrs, MiqGroup.get_httpd_groups_by_user(username)]
     end
 
-    def get_user_details_from_headers(username, request)
+    def user_details_from_headers(username, request)
       user_attrs = {:username  => username,
                     :fullname  => request.headers['X-REMOTE-USER-FULLNAME'],
                     :firstname => request.headers['X-REMOTE-USER-FIRSTNAME'],
@@ -72,7 +74,7 @@ def get_user_details_from_headers(username, request)
       [user_attrs, (request.headers['X-REMOTE-USER-GROUPS'] || '').split(/[;:]/)]
     end
 
-    def get_user_attrs(username)
+    def user_attrs_from_external_directory(username)
       return unless username
       require "dbus"
 
diff --git a/app/models/authenticator/ldap.rb b/app/models/authenticator/ldap.rb
index 3f16eada8a6..350cce0758f 100644
--- a/app/models/authenticator/ldap.rb
+++ b/app/models/authenticator/ldap.rb
@@ -9,7 +9,7 @@ def lookup_by_identity(username)
         find_or_create_by_ldap(username)
     end
 
-    def can_authorize_user_by_userid?
+    def user_authorizable_without_authentication?
       true
     end
 
diff --git a/app/models/user.rb b/app/models/user.rb
index 278588f3553..68e0f62c952 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -163,9 +163,9 @@ def self.lookup_by_identity(username)
     authenticator(username).lookup_by_identity(username)
   end
 
-  def self.authorize_by_userid(userid)
+  def self.authorize_user(userid)
     return if userid.blank? || admin?(userid)
-    authenticator(userid).authorize_user_by_userid(userid)
+    authenticator(userid).authorize_user(userid)
   end
 
   def logoff
diff --git a/spec/models/authenticator/httpd_spec.rb b/spec/models/authenticator/httpd_spec.rb
index 2e26b63d4d9..5567ed1708f 100644
--- a/spec/models/authenticator/httpd_spec.rb
+++ b/spec/models/authenticator/httpd_spec.rb
@@ -29,9 +29,9 @@
     end
   end
 
-  describe '.can_authorize_user_by_userid?' do
+  describe '.user_authorizable_without_authentication?' do
     it "is true" do
-      expect(subject.can_authorize_user_by_userid?).to be_truthy
+      expect(subject.user_authorizable_without_authentication?).to be_truthy
     end
   end
 
@@ -297,7 +297,7 @@ def authenticate
         end
       end
 
-      describe ".get_user_attrs" do
+      describe ".user_attrs_from_external_directory" do
         before do
           require "dbus"
           sysbus = double('sysbus')
@@ -313,7 +313,7 @@ def authenticate
         end
 
         it "should return nil for unspecified user" do
-          expect(subject.get_user_attrs(nil)).to be_nil
+          expect(subject.send(:user_attrs_from_external_directory, nil)).to be_nil
         end
 
         it "should return user attributes hash for valid user" do
@@ -331,7 +331,7 @@ def authenticate
 
           allow(@ifp_interface).to receive(:GetUserAttr).with('jdoe', requested_attrs).and_return(jdoe_attrs)
 
-          expect(subject.get_user_attrs('jdoe')).to eq(expected_jdoe_attrs)
+          expect(subject.send(:user_attrs_from_external_directory, 'jdoe')).to eq(expected_jdoe_attrs)
         end
       end
     end
diff --git a/spec/models/authenticator/ldap_spec.rb b/spec/models/authenticator/ldap_spec.rb
index d4861097401..17ca1ce65be 100644
--- a/spec/models/authenticator/ldap_spec.rb
+++ b/spec/models/authenticator/ldap_spec.rb
@@ -98,9 +98,9 @@ def normalize(dn)
     end
   end
 
-  describe ".can_authorize_user_by_userid?" do
+  describe ".user_authorizable_without_authentication?" do
     it "is true" do
-      expect(subject.can_authorize_user_by_userid?).to be_truthy
+      expect(subject.user_authorizable_without_authentication?).to be_truthy
     end
   end
 
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 059a7410002..f8ac8936474 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -538,13 +538,13 @@
     end
   end
 
-  context ".authorize_by_userid" do
+  context ".authorize_user" do
     it "returns nil with blank userid" do
-      expect(User.authorize_by_userid("")).to be_nil
+      expect(User.authorize_user("")).to be_nil
     end
 
     it "returns nil with admin userid" do
-      expect(User.authorize_by_userid("admin")).to be_nil
+      expect(User.authorize_user("admin")).to be_nil
     end
   end
 end