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.

Sign up for GitHub

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

Jump to bottom

Force user_type to UPN when username is a UPN #17690

Merged
merged 3 commits into from
Jul 17, 2018
Merged

Force user_type to UPN when username is a UPN #17690

merged 3 commits into from
Jul 17, 2018

Conversation

jvlcek
Copy link
Member

@jvlcek jvlcek commented Jul 10, 2018

When ordering a remote service the userid passed to the remote is taken from the database's
userid field, which can be either a UPN or a full DN. If it is a UPN and the authentication
configuration specifies samaccountname for User Type the domain prefix is erroneously
prepended to the username causing group lookup to fail.

This PR plugs this edge case by not prepending the domain prefix to the username if the username is already in UPN format.

Testing Done

In addition to the updated spec tests included in this PR I configured a service on a remote region on a live appliance and successfully ordered it from the global region while logged in as a:

  1. AD samaccount User Type user
  2. AD UPN User Type user
  3. OpenLdap Distinguished Name (CN=

Links

Steps for Testing/QA

  1. Integrate AD to authenticate Global and subordinate region on CFME appliance
    with the User Type of samaccountname
  2. Configure a service on the subordinate region
  3. Login on Global region via AD user and order a service.

jvlcek
jvlcek commented Jul 10, 2018
View reviewed changes
lib/miq_ldap.rb

return "#{username}@#{@user_suffix}"
when "mail"
username = "#{username}@#{@user_suffix}" unless @user_suffix.blank? || username =~ /^.+@.+$/
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is not correct. I'm fixing it now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fix has been completed.

@jvlcek jvlcek changed the title Force user_type to UPN when username is a UPN [WIP] Force user_type to UPN when username is a UPN Jul 10, 2018
@miq-bot miq-bot added the wip label Jul 10, 2018
@jvlcek jvlcek changed the title [WIP] Force user_type to UPN when username is a UPN Force user_type to UPN when username is a UPN Jul 10, 2018
@jvlcek
Copy link
Member Author

jvlcek commented Jul 10, 2018

@miq-bot add_label bug
@miq-bot add_label authentication

@jvlcek
Copy link
Member Author

jvlcek commented Jul 10, 2018

@miq-bot assign @abellotti

@jvlcek
Copy link
Member Author

jvlcek commented Jul 10, 2018

@abellotti Please review.

@miq-bot miq-bot removed the wip label Jul 10, 2018
abellotti
abellotti reviewed Jul 10, 2018
View reviewed changes
lib/miq_ldap.rb Outdated
@@ -303,7 +308,9 @@ def fqusername(username)

def get_user_object(username, user_type = nil)
user_type ||= @user_type.split("-").first
user_type = "dn" if self.is_dn?(username)
user_type = "dn" if self.dn?(username)
user_type = "upn" if self.upn?(username)
Copy link
Member

@abellotti abellotti Jul 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

this needs changing, I believe rdn's in LDAP can have the @ character. so if you have cn=joev@redhat,cn=users,.... the self.upn? call in 312 will return true but would actually be false so then incorrectly setting the user_type. You could use an if/else if structure for 311/312.

Copy link
Member Author

Choose a reason for hiding this comment

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

@abellotti Thank you. Will do!

lib/miq_ldap.rb Outdated

user_type = @user_type.split("-").first
return username if user_type != "mail" && self.upn?(username)
Copy link
Member

Choose a reason for hiding this comment

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

why was this needed ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was necessary because "mail" user_type can also have a "@". Similar to your comment regarding rdns in ldap.
Upon reviewing the old code, where there was logic to handle usernames with "@" when user_type was "mail" I realized this change was necessary.

@JPrause
Copy link
Member

JPrause commented Jul 11, 2018

@miq-bot add_label blocker

@JPrause
Copy link
Member

JPrause commented Jul 11, 2018

@jvlcek if this can be backported, can you add the gaprindashvili/yes label

@miq-bot
Copy link
Member

miq-bot commented Jul 11, 2018

Checked commits jvlcek/manageiq@2c9d3ef~...a45e59f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

lib/miq_ldap.rb

@jvlcek
Copy link
Member Author

jvlcek commented Jul 11, 2018

@miq-bot add_label gaprindashvili/yes

@jvlcek
Copy link
Member Author

jvlcek commented Jul 11, 2018

@gtanzillo Please take a look.

@abellotti
Copy link
Member

Hi @jvlcek code looks good, can you test against AD with user type DN ? (unless you did already) Thanks.

@jvlcek
Copy link
Member Author

jvlcek commented Jul 13, 2018

@abellotti Tests against AD with user type DN were successful! Wee HA!

@abellotti abellotti added this to the Sprint 91 Ending Jul 30, 2018 milestone Jul 17, 2018
@abellotti
Copy link
Member

LGTM!! Thanks @jvlcek for the additional testing. 👍

@abellotti abellotti merged commit c14bb2c into ManageIQ:master Jul 17, 2018
@simaishi
Copy link
Contributor

@jvlcek context '#get_user_object' does't exist in spec file for Gaprindashvili. What do we want do do for the new test added in that context in this PR? (it "searches for group membership when username is upn regardless of user_type" do

@jvlcek
Copy link
Member Author

jvlcek commented Jul 19, 2018

@simaishi

I am not sure why this PR lists a new test in contect #get_user_object
it "searches for group membership when username is upn regardless of user_type" do
Was added to the spec as part of the different commit:
f290134

Can you just add the `context "#fqusername" ?

JoeV

simaishi pushed a commit that referenced this pull request Jul 19, 2018
@abellotti @simaishi
Merge pull request #17690 from jvlcek/bz_1594641_ad_upn
5fa5d3c 
Force user_type to UPN when username is a UPN
(cherry picked from commit c14bb2c)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1603058
@simaishi
Copy link
Contributor

@jvlcek Added the fqusername only.

Gaprindashvili backport details:

$ git log -1
commit 5fa5d3c700ec00e2b96d45b5dc81f10f442abb68
Author: Alberto Bellotti <abellotti@users.noreply.github.com>
Date:   Tue Jul 17 13:50:30 2018 -0400

    Merge pull request #17690 from jvlcek/bz_1594641_ad_upn
    
    Force user_type to UPN when username is a UPN
    (cherry picked from commit c14bb2cd97ef584a70f34434245ec53c50b2418f)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1603058

@jvlcek jvlcek deleted the bz_1594641_ad_upn branch October 1, 2018 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abellotti abellotti abellotti approved these changes

@gtanzillo gtanzillo gtanzillo approved these changes

Assignees

@abellotti abellotti

Labels
blocker bug core/authentication gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jvlcek @JPrause @miq-bot @abellotti @simaishi @gtanzillo