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

tests: Add LDAP URI to ldappasswd explicitly #404

Closed
wants to merge 1 commit into from
Closed

tests: Add LDAP URI to ldappasswd explicitly #404

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 19, 2017

Test should always respect api.env.* values.

https://fedorahosted.org/freeipa/ticket/6622

@pvomacka pvomacka self-requested a review January 19, 2017 09:13
@tiran
Copy link
Member

tiran commented Jan 19, 2017

ipatests/test_integration/util.py calls ldappasswd without -H option, too. Related to the issue at hand, ipaserver/install/service.py has a similar issue with ldapmodify.

@MartinBasti MartinBasti changed the title tests: Add LDAP URI to ldappasswd explicitelly tests: Add LDAP URI to ldappasswd explicitly Jan 19, 2017
@ghost
Copy link
Author

ghost commented Jan 25, 2017

@tiran Nice catch, I've added it to ipatests/test_integration/util.py, too. but I would rather not extend this outside of tests.

Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

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

LGTM

Can you either create a ticket or address the ldapmodify call in another PR? I would hate to forget about the other minor issue.

@ghost
Copy link
Author

ghost commented Jan 26, 2017

@tiran After a closer look I believe that it's handled correctly in ipaserver/install/service.py
https://github.com/freeipa/freeipa/blob/master/ipaserver/install/service.py#L205:L208

@tiran
Copy link
Member

tiran commented Jan 26, 2017

I stand corrected! Thanks David.

@@ -86,5 +87,5 @@ def ldappasswd_user_change(user, oldpw, newpw, master):
userdn = "uid={},{},{}".format(user, container_user, basedn)

args = [paths.LDAPPASSWD, '-D', userdn, '-w', oldpw, '-a', oldpw,
'-s', newpw, '-x']
'-s', newpw, '-x', '-H', api.env.ldap_uri]
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tested this?

This code is executed on controller machine, thus api.env may be undefined, because there is no installed IPA.
I'm not sure if ldap_uri may be diferent on each (I assume than ldapi URI is the same on all machines, but not ldaps)

Copy link
Contributor

Choose a reason for hiding this comment

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

@MartinBasti I agree with your concerns. I guess constructed LDAP URI from passed in master parameter sounds as a safer bet.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants