Ckan upgrade 2.8.0a - permit alternative DistinguishedName (dn) #49
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Adds to the description the alternative base Distinguished Name (DN) in case the forest has two Organizational Units (OU) to look for. The way the python-ldap package is built, it needs the DN to have an OU. The original ldap library in C does not need it. Although, as a corner case solution, a new parameter of the alternative DN was created.
Include the alternative Distinguished Name (dn) parameter. It seems (needs confirmation) like the way the python-ldap package was built, it does not accept a DN wit only the Domain Component (DC), it must have a OU. So the parameter ckanext.ldap.base_dn_alt will be useful in case the Active Directory (AD) forest has two Organizational Units. Although, the best solution would be changing the python-ldap package so that the search functions worked with a DN without an OU.
This modification permits the use of two base Distinguished Names that will be searched for. It works along with the ckanext.ldap.search_alt, using the same logic. It there was already a way to use two Organizational Units in the cnakext.ldap.base_dn this change would be dull.
jrdh
requested changes
Feb 5, 2019
| except ldap.SERVER_DOWN: | ||
| log.error(u'LDAP server is not reachable') | ||
| return None | ||
| except ldap.OPERATIONS_ERROR as e: | ||
| log.error( | ||
| u'LDAP query failed. Maybe you need auth credentials for performing searches? Error returned by the server: ' + e.info) | ||
| log.error(u'LDAP query failed. Maybe you need auth credentials for performing searches?') |
There was a problem hiding this comment.
Can the error info be added back in? Or another way or getting more information about the error for the logs?
There was a problem hiding this comment.
The e.info does not exist. I was getting a Notice in the error log, that says the info atribute is not available when it did enter in that except.
I don't know how I could get more information to de logs.
There was a problem hiding this comment.
The ldap library's doc isn't all that clear but I think from my quick look the info attribute is optional and dependant on whether the server returned an extra info or not. Perhaps we could check if there is an info attribute available or not and then log it or not based on that?
Removes commented like as indicated by @jrdh. Corrects the case when alternative filter search is changed and distinguished name alternative must be tested.
|
I committed to my respository branch. I think it automatically brings the changes. |
jrdh
reviewed
Feb 15, 2019
| @@ -287,6 +287,8 @@ def _find_ldap_user(login): | |||
|
|
|||
| filter_str = config[u'ckanext.ldap.search.filter'].format( | |||
| login=ldap.filter.escape_filter_chars(login)) | |||
| filter_str_alt = config[u'ckanext.ldap.search.alt'].format( | |||
| login=ldap.filter.escape_filter_chars(login)) | |||
There was a problem hiding this comment.
ckanext.ldap.search.alt is optional so I'm pretty sure this will raise an exception if it's not defined in the config.
alycejenni
reviewed
Nov 26, 2019
| @@ -106,7 +106,7 @@ def _login_success(self, user_name, came_from): | |||
| ''' | |||
| session[u'ckanext-ldap-user'] = user_name | |||
| session.save() | |||
| toolkit.redirect_to(u'user.logged_in', came_from=came_from) | |||
| toolkit.redirect_to(controller=u'user', action=u'logged_in', came_from=came_from) | |||
There was a problem hiding this comment.
This reverts back to the old pylons syntax.
Suggested change
| toolkit.redirect_to(controller=u'user', action=u'logged_in', came_from=came_from) | |
| toolkit.redirect_to(u'user.logged_in', came_from=came_from) |
alycejenni
reviewed
Nov 26, 2019
| except ldap.SERVER_DOWN: | ||
| log.error(u'LDAP server is not reachable') | ||
| return None | ||
| except ldap.OPERATIONS_ERROR as e: | ||
| log.error( | ||
| u'LDAP query failed. Maybe you need auth credentials for performing searches? Error returned by the server: ' + e.info) | ||
| log.error(u'LDAP query failed. Maybe you need auth credentials for performing searches?') |
There was a problem hiding this comment.
Suggested change
| log.error(u'LDAP query failed. Maybe you need auth credentials for performing searches?') | |
| error_msg = u'LDAP query failed. Maybe you need auth credentials for performing searches?' | |
| if hasattr(e, u'info'): | |
| error_msg += u' Error returned by the server: ' + e.info | |
| log.error(error_msg) |
alycejenni
reviewed
Nov 26, 2019
| ret = _ldap_search(cnx, filter_str, attributes, non_unique=u'raise') | ||
| ret = _ldap_search(cnx, filter_str_alt, attributes, config[u'ckanext.ldap.base_dn'], non_unique=u'raise') | ||
| if ret is None and u'ckanext.ldap.base_dn_alt' in config: | ||
| ret = _ldap_search(cnx, filter_str, attributes, config[u'ckanext.ldap.base_dn_alt'], non_unique=u'log |
There was a problem hiding this comment.
Suggested change
| ret = _ldap_search(cnx, filter_str, attributes, config[u'ckanext.ldap.base_dn_alt'], non_unique=u'log | |
| ret = _ldap_search(cnx, filter_str, attributes, config[u'ckanext.ldap.base_dn_alt'], non_unique=u'log') |
alycejenni
reviewed
Nov 26, 2019
| if ret is None and u'ckanext.ldap.base_dn_alt' in config: | ||
| ret = _ldap_search(cnx, filter_str, attributes, config[u'ckanext.ldap.base_dn_alt'], non_unique=u'log | ||
| if ret is None and u'ckanext.ldap.base_dn_alt' in config and u'ckanext.ldap.search.alt' in config: | ||
| ret = _ldap_search(cnx, filter_str_alt, attributes, config[u'ckanext.ldap.base_dn_alt'], non_unique=u'raise |
There was a problem hiding this comment.
Suggested change
| ret = _ldap_search(cnx, filter_str_alt, attributes, config[u'ckanext.ldap.base_dn_alt'], non_unique=u'raise | |
| ret = _ldap_search(cnx, filter_str_alt, attributes, config[u'ckanext.ldap.base_dn_alt'], non_unique=u'raise') |
alycejenni
reviewed
Nov 26, 2019
| @@ -287,23 +287,29 @@ def _find_ldap_user(login): | |||
|
|
|||
| filter_str = config[u'ckanext.ldap.search.filter'].format( | |||
| login=ldap.filter.escape_filter_chars(login)) | |||
| filter_str_alt = config[u'ckanext.ldap.search.alt'].format( | |||
There was a problem hiding this comment.
Suggested change
| filter_str_alt = config[u'ckanext.ldap.search.alt'].format( | |
| filter_str_alt = config.get(u'ckanext.ldap.search.alt', u'').format( |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Includes a new parameter, called ckanext.ldap.base_dn_alt with permits a new Distinguished Name to be searched when a login operation is performed.
It uses the same logic implemented in the parameter ckanext.ldap.search_alt that already existed.
This was implemented because the forest of my organization has two organizational units (OU) and the CKAN must be acessed from users from both OUs.