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

Normalize the username entered at login to lowercase #15716

Merged
merged 2 commits into from
Aug 7, 2017

Conversation

jvlcek
Copy link
Member

@jvlcek jvlcek commented Aug 2, 2017

LDAP does a case sensitive match of the user name but AD will
do a case insensitive match. By normalizing the userid to
lowercase when using external auth both backed to either
an LDAP directory or AD both will authenticate but only one DB
record, in all lowercase, will be created, even if the user
attempted to login with a mixed case username when backed to AD.

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

Steps for Testing/QA

Test the AD case:

  1. Configure an appliance to use authentication Mode: External (httpd) to an AD directory
  2. Attempt to login to an appliance with a valid username in mixed case, e.g.: tEsTuSeR1
  3. Confirm one user record, with the userid in lowercase, was created.
  4. Attempt to login again with a valid username in mixed case but different from step 1. e.g. TestUser1
  5. Repeat step 3 with the same username but varying which letters in the username are upper and which are lowercase
  6. Confirm the user can log in but only one DB record, with the userid in lowercase, is created.

Test the LDAP case:

  1. Configure an appliance to use authentication Mode: External (httpd) to an LDAP directory
  2. Attempt to login to an appliance with a valid username, the case must match what is in the LDAP directory because LDAP does a case sensitive lookup
  3. Confirm one user record, with the userid in lowercase, was created.
  4. Confirm attempts to login to the appliance with the case of the username not matching what is in LDAP fail

LDAP does a case sensitive match of the user name but AD will
do a case insensitive match. By normalizing the userid to
lowercase when using external auth both backed to either
an LDAP directory or AD both will authenticate but only one DB
record, in all lowercase, will be created, even if the user
attempted to login with a mixed case username when backed to AD.

https://bugzilla.redhat.com/show_bug.cgi?id=1448787
@jvlcek
Copy link
Member Author

jvlcek commented Aug 2, 2017

@abellotti Please review

@jvlcek
Copy link
Member Author

jvlcek commented Aug 2, 2017

@miq-bot add_label authentication, bug

@miq-bot
Copy link
Member

miq-bot commented Aug 4, 2017

Checked commits jvlcek/manageiq@88a312c~...ce0513c with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 1 offense detected

app/models/authenticator/base.rb

@Fryguy
Copy link
Member

Fryguy commented Aug 4, 2017

Is it possible for two users to have the same name but different case? e.g. bob and Bob ?

@jvlcek
Copy link
Member Author

jvlcek commented Aug 4, 2017

@Fryguy that would be a bug. In fact that is the bug this PR is trying to address. :) But to your point, No, it is not possible for the same user to exist in the directory (Active Directory or LDAP) with the same name but with mismatched case.

So there are 2 perspectives:
1 what is valid in the directory
mismatch case is not
2 what ends up being created in the MiQ DB. An existing bug will create multiple user with mismatched case.

This PR addresses #2 with the understanding that #1 is a tautology :)

Thanks for the help!

Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

👍 Nice!

@gtanzillo gtanzillo added this to the Sprint 66 Ending Aug 7, 2017 milestone Aug 7, 2017
@gtanzillo gtanzillo merged commit 731174c into ManageIQ:master Aug 7, 2017
@simaishi
Copy link
Contributor

@jvlcek @gtanzillo there is a request to backport this to Fine branch. Is that ok?

@jvlcek
Copy link
Member Author

jvlcek commented Aug 11, 2017

@simaishi I will put together the Fine backport PR.

@simaishi
Copy link
Contributor

@jvlcek thank you. For tracking purpose, I've added fine flags.

@simaishi
Copy link
Contributor

Backported to Fine via #15796

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants