-
Notifications
You must be signed in to change notification settings - Fork 210
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
Use case-insensitive search for user lookup by external_auth_id #540
Conversation
cc @jeremiahsnapp @stephenlauck @chef/lob |
The migrate-on-login approach - to canonical name - could work if we keep a 'migrated' flag and use that as part of the query, as |
fetch(#chef_user{server_api_version = ?API_v0, username = UserName} = Record, CallbackFun) -> | ||
fetch_user(find_user_by_username_v0, Record, UserName, CallbackFun); | ||
fetch(#chef_user{username = UserName} = Record, CallbackFun) -> | ||
fetch_user(find_user_by_username, Record, UserName, CallbackFun). | ||
|
||
case_sensitivity() -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ldap_case_sensitivity
- it's a bit clearer since in context these fetches serve all the different lookup methods.
LGTM 👍 after customer confirmation and the function name change. |
Fixed the function name, thanks for the suggestion. Re the in-place migration. My biggest problem with any migration is just that we don't actually know whether that value should be case insensitive or not without checking the LDAP schema or having the user tell us. If we have the user tell us, I'd rather not do a migration based on that since the user might make a mistake when setting it. |
Almost all of the typical values of ldap['login_attribute'] are case insensitive according to the LDAP schema. However, we previously used a case-sensitive lookup when trying to find an existing user record. This commit changes the default lookup to be case insensitive but provides a user-controllable override for the rare case where users are setting a case-sensitive login_attribute. ChangeLog-Entry: Fix bug where logins via LDAP failed because of case sensitivity.
9159925
to
6116344
Compare
Use case-insensitive search for user lookup by external_auth_id
Almost all of the typical values of ldap['login_attribute'] are case
insensitive according to the LDAP schema. However, we previously used a
case-sensitive lookup when trying to find an existing user record.
This commit changes the default lookup to be case insensitive but
provides a user-controllable override for the rare case where users are
setting a case-sensitive login_attribute.
Review Notes
If we were doing this from scratch, I think the correct method of linking LDAP accounts to user records would be to store a canonicalized form of the distinguished name as returned by the LDAP server. Unfortunately, we have live installations with existing data.
I've made case-sensitivity a configurable with a default of false. Most fields that people will use for login_attribute are case insensitive; but it is possible that some users want to use a case sensitive field here. Unfortunately, because of the way chef_db/chef_sql/chef_object are tied together, changing queries based on config either requires adding a new field to the record, or querying the config all the way from chef_user. I opted for the latter as that change seemed to have the smaller surface area.
I looked at the prospect of doing something smarter here such as some sort of data migration combined with a canonicalization of what we store here are the problems I kept running into: