Converting userids to UPN format to avoid duplicate user records #15535

jvlcek · 2017-07-10T19:46:17Z

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

The Issue:

The issue this PR will address is that the various mechanism available for MiQ customers to use for authentication and authorization do not create user records with a common userid.

Brief Definitions:

user principal name (upn) format

This format is: username@domain name

e.g.: [email protected]

distinguished name (dn) format

This format exposes the directory layout and is of the form:
cn|uid="username",ou="level1", ou="level2"...,dc="domain",dc="domain"

e.g.: cn=sally,ou=people,ou=prod,dc=example,dc=com

username

This format is simply the username.

e.g.: sally

Issue Details:

The matrix of possible configurations creating userids in different formats is large. They can be either UPN, DN, or username.

One example is the likely scenario where a customer manually migrated, not using the automated conversion tool currently under development, from using our MiqLdap client to External Auth.

The MiqLdap client would have created a user record with the userid in UPN format

e.g.: [email protected]_

or, depending on configuration in DN format

e.g.: cn=sally,ou=people,ou=prod,dc=example,dc=com

Using external auth, when the same user credentials are specified, a user record is created with a userid of simply the username

e.g.: _sally_

Resulting in duplicate user records for the same user.

The Solution:

Ultimately a UUID should be made available from the underlying directory infrastructure that could be used as the userid but this is not yet available. Working with the IdM team and Alberto Bellotti it was decided the best solution at the moment is to standardize the userid to be user principal name (upn) format.

We could provide a migration that would update every user record to have a userid in UPN format. However it is possible to avoid the pitfalls associated with such a migration by simply updating the user record to have a userid in UPN format at user login.

Once a user record in UPN format is found user records with a related userid would be ignored.
I had considered destroying any user records with related userids once the UPN formatted one is found but have decided against it. Although in the normal flow of usage this should never happen. However in the event of the unpredictable, ignoring such records instead of destroying them would allow customers the ability to address such inconsistencies.

The Related PRs:

The following associated PRs in different repos are required for this PR and must be merged at the same #time.

PR 127 in repo ManageIQ/manageiq-appliance
PR 424 in repo ManageIQ/manageiq_docs
PR 250 in repo ManageIQ/manageie-gems-pending

This and the above PRs should all be merged at the same time.

Note:

This change requires new support in the underlying SSSD code. Updates were
required to provide the domain name when MiQ is configured to use External Authentication (Mode: External (httpd)

The BZs that track this work are:

The fixes for these BZs are targeted for RHEL 7.4 GA and CentOS 7.4

Therefor this change should not be merged until MiQ appliance builds migrate to RHEL 7.4 GA and CentOS 7.4

Steps for Testing/QA

The way to test this is to configure MiQ to use the MiqLdap client (Mode: LDAP) for authentication.
Log in with a valid user/group
Then manually reconfigure the appliance to use External Authentication (Mode: External (httpd)) for authentication.
Log in with the same valid user/group from step 2.
Confirm there is only a single user created.

jvlcek · 2017-07-10T19:46:55Z

@abellotti and @gtanzillo Please review

jvlcek · 2017-07-10T19:49:39Z

@miq-bot add_labels authentication, bug

miq-bot · 2017-08-07T18:46:35Z

This pull request is not mergeable. Please rebase and repush.

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

jvlcek · 2017-08-23T19:02:49Z

@gtanzillo and @abellotti All requested code and doc changes have been made and all retesting, with the additional testing of the realm join and IPA/AD Trust configurations, have been successfully completed. Please take a look and merge this and the associated PR if appropriate.

Associated PRs

PR 127 in repo ManageIQ/manageiq-appliance
PR 424 in repo ManageIQ/manageiq_docs
PR 250 in repo ManageIQ/manageiq-gems-pending

jvlcek · 2017-08-23T21:12:02Z

@gtanzillo and @abellotti I also added this Pivotal Tracker story to track the SAML research:
https://www.pivotaltracker.com/n/projects/1610127/stories/150498547

Let me know when this is good to merge and I will squash the commits.

gtanzillo

@abellotti and I reviewed this together. It looks good 👍. Just a few minor changes requested.

gtanzillo · 2017-08-25T16:01:22Z

app/models/authenticator/httpd.rb

+    def find_userid_as_distinguished_name(user_attrs, upn_username)
+      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
+      $audit_log.info("Updating userid from #{user.userid} to #{upn_username}") unless user.blank?


Is this really "Updating userid..."? It's not clear where it's setting it and saving it.

gtanzillo · 2017-08-25T16:04:35Z

app/models/authenticator/httpd.rb

+      userid = userid_for(identity, username)
+      user   = User.find_by_userid(userid)
+      user ||= User.in_my_region.where('lower(userid) = ?', userid).order(:lastlogon).last
+      $audit_log.info("Updating userid from #{user.userid} to #{upn_username}") unless user.blank?


It seems that you are not using upn_username in this method other than the log message. Like the comment below, it doesn't seem like it's actually updating. So can upn_username be dropped from this method?

gtanzillo · 2017-08-25T16:09:01Z

app/models/authenticator/httpd.rb

+    def find_userid_as_username(identity, username, upn_username)
+      userid = userid_for(identity, username)
+      user   = User.find_by_userid(userid)
+      user ||= User.in_my_region.where('lower(userid) = ?', userid).order(:lastlogon).last


This could be DRY'd up by putting it in its own method.

gtanzillo · 2017-08-25T16:18:48Z

spec/models/authenticator/httpd_spec.rb

+      end
+
+      context "when user record is for a different region" do
+        let(:my_region_number) { Classification.my_region_number }


Just curious, why get the current region from Classification and not the one in ApplicationRecord since Classification is not related to any of this?

Same for the next 2 lines

jvlcek · 2017-08-25T20:43:24Z

@gtanzillo and @abellotti Thank you for the review.
Sorry but I don't see how I can reply to the individual comments. So I'll try to address them all here:

Good catch on spec/models/authenticator/httpd_spec.rb

I had used spec/models/classification_spec.rb as an example spec that checks for .my_region_number

Regarding your comments for app/models/authenticator/httpd.rb

When the lower level method detects upn_username will be changed it logs that.
Then then new upn_username is returned to the caller, which passes it to update_user_attributes, which is updates the attributes that will be passed to user.save

Not sure what you want me to DRY. Need to discuss

jvlcek · 2017-08-25T20:49:39Z

@gtanzillo I think the User.in_my_region.where is cleaner the way it is.

jvlcek · 2017-08-27T16:01:31Z

@gtanzillo and @abellotti I refactored the logging up updates the the userid to avoid having to pass upn_username just to use in in a log message. I think this change will address the points you have made. Please let me know if you want me to squash the commits.

Thank you for all the help!
JoeV

miq-bot · 2017-08-27T16:02:56Z

Checked commits jvlcek/manageiq@8234bfa~...48ae7fb with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 2 offenses detected

app/models/authenticator/httpd.rb

❗ - Line 76, Col 14 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_userid.
❗ - Line 82, Col 16 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_userid.

gtanzillo

Looks good! 👍

jvlcek · 2017-08-29T10:09:12Z

@gtanzillo

Thank you for all the help and for merging this. It's important that the 3 associated PRs be merged now too.

The Related PRs:

The following associated PRs in different repos are required for this PR and must be merged at the same #time.

PR 127 in repo ManageIQ/manageiq-appliance

PR 424 in repo ManageIQ/manageiq_docs

PR 250 in repo ManageIQ/manageie-gems-pending

simaishi · 2017-09-01T15:40:27Z

Marking as fine/conflict as this will not cherry-pick cleanly.

Manually cherry-picked out of ManageIQ#15535 (cherry picked from commit 53c1704) Merge pull request ManageIQ#15535 from jvlcek/bz1424618_dup_users https://bugzilla.redhat.com/show_bug.cgi?id=1487689

simaishi · 2017-09-05T15:23:12Z

Backported to Fine via #15927

Manually cherry-picked out of ManageIQ#15535 (cherry picked from commit 53c1704) Merge pull request ManageIQ#15535 from jvlcek/bz1424618_dup_users https://bugzilla.redhat.com/show_bug.cgi?id=1487689

miq-bot added core/authentication bug labels Jul 10, 2017

This was referenced Jul 10, 2017

Add support for the domain user attribute ManageIQ/manageiq-appliance#127

Merged

Document support for the domain user attribute ManageIQ/manageiq-documentation#424

Merged

chessbyte assigned gtanzillo Jul 11, 2017

jvlcek force-pushed the bz1424618_dup_users branch from 9da9490 to 16a90f9 Compare July 17, 2017 18:50

This was referenced Jul 24, 2017

A tool for converting miqldap auth to external auth with sssd #15640

Merged

Add support for the external auth domain user attribute ManageIQ/manageiq-gems-pending#250

Merged

jvlcek force-pushed the bz1424618_dup_users branch from 16a90f9 to 0b23e1f Compare August 7, 2017 17:51

miq-bot added the unmergeable label Aug 7, 2017

jvlcek force-pushed the bz1424618_dup_users branch from 0b23e1f to d9b7d68 Compare August 9, 2017 22:21

miq-bot removed the unmergeable label Aug 9, 2017

jvlcek added 4 commits August 21, 2017 17:21

Converting userids to UPN format to avoid duplicate user records

8234bfa

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

Fix rubocop ExtraSpacing warning

a5be862

Handle mixed case UPN userid format

e1cbe5c

Improve userid case testing

629f083

jvlcek force-pushed the bz1424618_dup_users branch from bef5276 to 629f083 Compare August 21, 2017 21:21

Remove uneeded super paramas and downcase entire usernane

9e1877d

gtanzillo reviewed Aug 25, 2017

View reviewed changes

Fix spec to get the current region from the ApplicationRecord

309299c

Refactor logging of updates to userid

48ae7fb

gtanzillo approved these changes Aug 28, 2017

View reviewed changes

gtanzillo added this to the Sprint 68 Ending Sep 4, 2017 milestone Aug 28, 2017

gtanzillo merged commit 53c1704 into ManageIQ:master Aug 28, 2017

simaishi added fine/conflict labels Sep 1, 2017

jvlcek mentioned this pull request Sep 1, 2017

[FINE] Converting userids to UPN format to avoid duplicate user records #15927

Merged

simaishi added fine/backported and removed fine/conflict labels Sep 5, 2017

jvlcek deleted the bz1424618_dup_users branch November 10, 2017 19:47

jvlcek mentioned this pull request Jan 18, 2018

Allow users already in UPN format #16849

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Converting userids to UPN format to avoid duplicate user records #15535

Converting userids to UPN format to avoid duplicate user records #15535

jvlcek commented Jul 10, 2017 •

edited

Loading

jvlcek commented Jul 10, 2017

jvlcek commented Jul 10, 2017

miq-bot commented Aug 7, 2017

jvlcek commented Aug 23, 2017 •

edited by gtanzillo

Loading

jvlcek commented Aug 23, 2017

gtanzillo left a comment

gtanzillo Aug 25, 2017

gtanzillo Aug 25, 2017

gtanzillo Aug 25, 2017

gtanzillo Aug 25, 2017

jvlcek commented Aug 25, 2017

jvlcek commented Aug 25, 2017

jvlcek commented Aug 27, 2017

miq-bot commented Aug 27, 2017

gtanzillo left a comment

jvlcek commented Aug 29, 2017

simaishi commented Sep 1, 2017

simaishi commented Sep 5, 2017

Converting userids to UPN format to avoid duplicate user records #15535

Converting userids to UPN format to avoid duplicate user records #15535

Conversation

jvlcek commented Jul 10, 2017 • edited Loading

The Issue:

Brief Definitions:

user principal name (upn) format

distinguished name (dn) format

username

Issue Details:

The Solution:

The Related PRs:

Note:

Steps for Testing/QA

jvlcek commented Jul 10, 2017

jvlcek commented Jul 10, 2017

miq-bot commented Aug 7, 2017

jvlcek commented Aug 23, 2017 • edited by gtanzillo Loading

Associated PRs

jvlcek commented Aug 23, 2017

gtanzillo left a comment

Choose a reason for hiding this comment

gtanzillo Aug 25, 2017

Choose a reason for hiding this comment

gtanzillo Aug 25, 2017

Choose a reason for hiding this comment

gtanzillo Aug 25, 2017

Choose a reason for hiding this comment

gtanzillo Aug 25, 2017

Choose a reason for hiding this comment

jvlcek commented Aug 25, 2017

jvlcek commented Aug 25, 2017

jvlcek commented Aug 27, 2017

miq-bot commented Aug 27, 2017

gtanzillo left a comment

Choose a reason for hiding this comment

jvlcek commented Aug 29, 2017

The Related PRs:

simaishi commented Sep 1, 2017

simaishi commented Sep 5, 2017

jvlcek commented Jul 10, 2017 •

edited

Loading

jvlcek commented Aug 23, 2017 •

edited by gtanzillo

Loading