Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User login scoping (API) #20506

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions app/models/authenticator/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ def authenticate(username, password, request = nil, options = {})
audit = {:event => audit_event, :userid => username}

# The fail_message might or might not come from the _authenticate method
authenticated, fail_message = options[:authorize_only] || _authenticate(username, password, request)
authenticated, fail_message = options[:authorize_only] || _authenticate(username, password, request: request, lookup_scope: options[:lookup_scope])
fail_message ||= _("Authentication failed") # Fall back to the default fail_message

if authenticated
Expand All @@ -76,7 +76,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, request)
user_or_taskid = lookup_by_identity(username, request: request, lookup_scope: options[:lookup_scope])
user_or_taskid ||= autocreate_user(username)

unless user_or_taskid
Expand Down Expand Up @@ -200,8 +200,8 @@ def authenticate_with_http_basic(username, password, request = nil, options = {}
[!!result, username]
end

def lookup_by_identity(username, *_args)
case_insensitive_find_by_userid(username)
def lookup_by_identity(username, request: nil, lookup_scope: nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure the advantage of this method.
do both this class and case_insensitive_find use the same caching lookup?
Think they can be merged? (I'm probably not seeing a character change or something in there)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I was kinda confused about this as well. I decided not to address that in this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me try again.
you added a cache here
you added a cache in the case_insensitive_find_by_userid

can we get away with making this method just the call into case_insensitive_find_by_userid ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol. ^ idiot. totally didn't process your response.

LGTM

case_insensitive_find_by_userid(username, lookup_scope: lookup_scope)
end

# FIXME: LDAP
Expand Down Expand Up @@ -242,9 +242,15 @@ def failure_reason(_username, _request)
nil
end

def case_insensitive_find_by_userid(username)
user = User.lookup_by_userid(username)
user || User.in_my_region.where(:lower_userid => username.downcase).order(:lastlogon).last
def case_insensitive_find_by_userid(username, lookup_scope: nil)
user_key = username.downcase
@lookup ||= {}
@lookup[user_key] ||= {}
return @lookup[user_key][lookup_scope] if @lookup[username][lookup_scope]

base = User.send(User::LOOKUP_SCOPES[lookup_scope])
user = base.lookup_by_userid(username)
@lookup[user_key][lookup_scope] = user || base.in_my_region.where(:lower_userid => user_key).order(:lastlogon).last
end

def userid_for(_identity, username)
Expand Down
4 changes: 2 additions & 2 deletions app/models/authenticator/database.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ def uses_stored_password?

private

def _authenticate(username, password, _request)
user = case_insensitive_find_by_userid(username)
def _authenticate(username, password, request: nil, lookup_scope: nil)
user = case_insensitive_find_by_userid(username, lookup_scope: lookup_scope)

return [false, _('Your account has been locked due to too many failed login attempts, please contact the administrator.')] if user&.locked?

Expand Down
19 changes: 10 additions & 9 deletions app/models/authenticator/httpd.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ def user_authorizable_with_system_token?
ext_auth_is_oidc? || ext_auth_is_saml?
end

def _authenticate(_username, _password, request)
def _authenticate(_username, _password, request: nil, lookup_scope: nil)
request.present? &&
request.headers['X-REMOTE-USER'].present?
end
Expand Down Expand Up @@ -85,29 +85,30 @@ def find_or_initialize_user(identity, username)
[upn_username, user]
end

def lookup_by_identity(username, request = nil)
def lookup_by_identity(username, request: nil, lookup_scope: nil)
if request
user_attrs, _membership_list = user_details_from_headers(username, request)
upn_username = username_to_upn_name(user_attrs)
user = find_userid_as_upn(upn_username)
user ||= find_userid_as_distinguished_name(user_attrs)
user = find_userid_as_upn(upn_username, lookup_scope: lookup_scope)
user ||= find_userid_as_distinguished_name(user_attrs, lookup_scope: lookup_scope)
end
user || case_insensitive_find_by_userid(username)
user || case_insensitive_find_by_userid(username, lookup_scope: lookup_scope)
end

private

def find_userid_as_upn(upn_username)
case_insensitive_find_by_userid(upn_username)
def find_userid_as_upn(upn_username, lookup_scope: nil)
case_insensitive_find_by_userid(upn_username, lookup_scope: lookup_scope)
end

def find_userid_as_username(identity, username)
case_insensitive_find_by_userid(userid_for(identity, username))
end

def find_userid_as_distinguished_name(user_attrs)
def find_userid_as_distinguished_name(user_attrs, lookup_scope: nil)
dn_domain = user_attrs[:domain].downcase.split(".").map { |s| "dc=#{s}" }.join(",")
user = User.in_my_region.where("userid LIKE ?", "%=#{user_attrs[:username]},%,#{dn_domain}").last
base = User.send(User::LOOKUP_SCOPES[lookup_scope])
user = base.in_my_region.where("userid LIKE ?", "%=#{user_attrs[:username]},%,#{dn_domain}").last
user
end

Expand Down
2 changes: 1 addition & 1 deletion app/models/authenticator/ldap.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def normalize_username(username)
miq_ldap.normalize(miq_ldap.fqusername(username))
end

def _authenticate(username, password, _request)
def _authenticate(username, password, request: nil, lookup_scope: nil)
password.present? &&
ldap_bind(username, password)
end
Expand Down
23 changes: 21 additions & 2 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,25 @@ class User < ApplicationRecord

scope :with_same_userid, ->(id) { where(:userid => User.where(:id => id).pluck(:userid)) }

# This just creates the following Hash:
#
# {
# :api_includes => :api_includes
# }
#
# Where :default_scoped is the value for anything that doesn't match a key in
# the original array. The "lookup hash" was chossen since it is much faster
# to do a hash table lookup than a `Array.includes?(KEY)`.
LOOKUP_SCOPES = Hash.new(:default_scoped).merge({:api_includes => :api_includes}).freeze

scope :api_includes, -> {
kbrock marked this conversation as resolved.
Show resolved Hide resolved
eager_load(:current_group => [
:miq_user_role,
:tenant,
:entitlement
])
}

def self.with_roles_excluding(identifier)
where.not(:id => User.unscope(:select).joins(:miq_groups => :miq_product_features)
.where(:miq_product_features => {:identifier => identifier})
Expand Down Expand Up @@ -263,8 +282,8 @@ def self.authenticate_with_http_basic(username, password, request = nil, options
authenticator(username).authenticate_with_http_basic(username, password, request, options)
end

def self.lookup_by_identity(username)
authenticator(username).lookup_by_identity(username)
def self.lookup_by_identity(username, lookup_scope: :none)
authenticator(username).lookup_by_identity(username, lookup_scope: lookup_scope)
end

def self.authorize_user(userid)
Expand Down
8 changes: 4 additions & 4 deletions spec/models/authenticator/httpd_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -107,19 +107,19 @@
end

it "finds existing users as username" do
expect(subject.lookup_by_identity('alice', request)).to eq(alice)
expect(subject.lookup_by_identity('alice', request: request)).to eq(alice)
end

it "finds existing users as UPN" do
expect(subject.lookup_by_identity('cheshire', request)).to eq(cheshire)
expect(subject.lookup_by_identity('cheshire', request: request)).to eq(cheshire)
end

it "finds existing users as distinguished name" do
expect(subject.lookup_by_identity('towmater', request)).to eq(towmater_dn)
expect(subject.lookup_by_identity('towmater', request: request)).to eq(towmater_dn)
end

it "doesn't create new users" do
expect(subject.lookup_by_identity('bob', request)).to be_nil
expect(subject.lookup_by_identity('bob', request: request)).to be_nil
end
end

Expand Down