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

Ensure user name is set even when common LDAP attributes are missing. #14142

Merged
merged 1 commit into from
Mar 2, 2017
Merged

Ensure user name is set even when common LDAP attributes are missing. #14142

merged 1 commit into from
Mar 2, 2017

Conversation

jvlcek
Copy link
Member

@jvlcek jvlcek commented Mar 2, 2017

Addresses:
https://bugzilla.redhat.com/show_bug.cgi?id=1400567

Description of problem:

Currently, if an administrator has configured users in LDAP without the common
but not required attribute displayName, login to MiQ fails for those users.

This PR adds functionality to set the user name to something other than displayName
when it is missing. The user name is used by MiQ to display a header with the name
of the logged in user.

Two attempts to set the user name are made.
The first attempt will try to use the LDAP attributes for first and last name. Which are readable but also optional. Failing that the second attempt will use the userid, which is less attractive
but more foolproof.

Description of solution implementation:

This could have been solved by adjusting for an empty user name in the single app/models/authenticator.rb. However this resulted in other authentication
mechanisms exercising code that is limited to ldap and httpd authenticators.

The solution presented solves the problem in both the ldap and httpd authenticators.
Because the ldap authenticator code is being EOLed the end result will, in the near
future, have the solution in a single place, the httpd authenticators.

Steps for Testing/QA

Test 1:
Attempt to log in to MiQ with an LDAP user who's displayName attribute has been deleted.

Test 2:
Attempt to log in to MiQ with an LDAP user who's displayName, sn and givenName attributes have been deleted.

These test should be repeated using both the MiQLdap client and External Auth

@jvlcek
Copy link
Member Author

jvlcek commented Mar 2, 2017

@abellotti and @kbrock Please review.

@abellotti After consideration I have decided that your initial idea of solving this in both the ldap and httpd authenticators is the better way to go. Please see my description above.

Thank you! JoeV

@miq-bot
Copy link
Member

miq-bot commented Mar 2, 2017

Checked commit jvlcek@52f02f5 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks good. 🏆

kbrock
kbrock approved these changes Mar 2, 2017
View reviewed changes
Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

Like where you put the code and how the tests turned out.

Like how each authenticator fully owns populating the user record for their own domain

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
Copy link
Member

Choose a reason for hiding this comment

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

I like how test tests turned out joe

Fryguy
Fryguy approved these changes Mar 2, 2017
View reviewed changes
Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

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

LGTM

@abellotti
Copy link
Member

Nice Enhancement @jvlcek LGTM!! 👍

@abellotti abellotti merged commit b502d51 into ManageIQ:master Mar 2, 2017
@abellotti abellotti added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 2, 2017
@gtanzillo
Copy link
Member

👍 Thanks @jvlcek!

@yada
Copy link

yada commented Mar 3, 2017

FYI : work for us, tested using ldap only.
Using an empty DisplayName, audit.log report 👍
[----] I, [2017-03-03T17:13:46.226748 #127921:112faec] INFO -- Success: MIQ(Authenticator.authenticate) userid: [TestRHELFixRN] - User testrhelfixrn@domain successfully validated by LDAP
[----] I, [2017-03-03T17:13:46.561999 #127921:112faec] INFO -- Success: MIQ(Authenticator.authenticate) userid: [TestRHELFixRN] - Authentication successful for user testrhelfixrn@domain
[----] I, [2017-03-03T17:13:46.866443 #127896:113bae0] INFO -- Success: MIQ(Authenticator.authenticate) userid: [TestRHELFixRN] - User testrhelfixrn@domain successfully validated by LDAP
[----] I, [2017-03-03T17:13:47.036887 #127896:113bae0] INFO -- Success: MIQ(Authenticator.authenticate) userid: [TestRHELFixRN] - Authentication successful for user testrhelfixrn@domain

@abellotti
Copy link
Member

Thanks @yada for testing it out.

@jvlcek
Copy link
Member Author

jvlcek commented Mar 3, 2017

Yes, Thank you @yada !

simaishi pushed a commit that referenced this pull request Mar 7, 2017
@abellotti @simaishi
Merge pull request #14142 from jvlcek/bz1400567_RFE_LDAP_no_displayname
38ee936 
Ensure user name is set even when common LDAP attributes are missing.
(cherry picked from commit b502d51)

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

simaishi commented Mar 7, 2017

Euwe backport details:

$ git log -1
commit 38ee936ca7613f501f0bb0d9283229a30ff2ab5f
Author: Alberto Bellotti <abellotti@users.noreply.github.com>
Date:   Thu Mar 2 17:34:53 2017 -0500

    Merge pull request #14142 from jvlcek/bz1400567_RFE_LDAP_no_displayname
    
    Ensure user name is set even when common LDAP attributes are missing.
    (cherry picked from commit b502d512dcb37ea6c34f8d0bcd84e5b489a6b311)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1428859

agrare pushed a commit to agrare/manageiq that referenced this pull request Apr 19, 2017
@abellotti @simaishi
Merge pull request ManageIQ#14142 from jvlcek/bz1400567_RFE_LDAP_no_d…
f75afe1 
…isplayname

Ensure user name is set even when common LDAP attributes are missing.
(cherry picked from commit b502d51)

https://bugzilla.redhat.com/show_bug.cgi?id=1428859
@jvlcek jvlcek deleted the bz1400567_RFE_LDAP_no_displayname branch July 6, 2017 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fryguy Fryguy Fryguy approved these changes

@kbrock kbrock kbrock approved these changes

Assignees

@abellotti abellotti

Labels
core/authentication enhancement euwe/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@jvlcek @miq-bot @abellotti @gtanzillo @yada @simaishi @kbrock @Fryguy @chessbyte