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

Force use of IPv4 for LDAP test. #367

Open
wants to merge 2 commits into
base: stable
Choose a base branch
from

Conversation

puck
Copy link
Contributor

@puck puck commented Sep 4, 2023

Net::LDAP::Server::Test binds to IPv6 by default if IPv6 is enabled, but Net::LDAP uses 'localhost' which resolves to an IPv4 address. Even when I switched the call to Net::LDAP->new() to use ip6-localhost it failed elsewhere due to RT using 127.0.0.1.

Net::LDAP::Server::Test binds to IPv6 by default if IPv6 is enabled, but
Net::LDAP uses 'localhost' which resolves to an IPv4 address.  Even when I
switched the call to Net::LDAP->new() to use ip6-localhost it failed
elsewhere due to RT using 127.0.0.1.
@cbrandtbuffalo
Copy link
Member

Hmm, this makes me wonder what else currently expects localhost to be '127.0.0.1' vs. '::1'. Assuming you ran RTs tests on your IPv6 enabled system, are the LDAP tests the only ones that failed?

@puck
Copy link
Contributor Author

puck commented Sep 5, 2023

I'm running the tests on a dual stack system. The LDAP tests are the only ones that fail for me without this patch.

A quick grep on 5.0-trunk shows this:

puck@dirk:~/personal/RT/rt$ grep -r "127\.0\.0\.1" * | grep -v "t/data/emails"
docs/web_deployment.pod:    spawn-fcgi -u www-data -g www-data -a 127.0.0.1 -p 9000 \
docs/web_deployment.pod:            fastcgi_pass 127.0.0.1:9000;
lib/RT/Interface/Web.pm:to handle common problems such as localhost vs 127.0.0.1
lib/RT/Interface/Web.pm:    $uri->host('127.0.0.1') if $uri->host eq 'localhost';
sbin/rt-setup-fulltext-index:    $url = 'sphinx://127.0.0.1:3312/rt'
sbin/rt-setup-fulltext-index.in:    $url = 'sphinx://127.0.0.1:3312/rt'
t/fts/indexed_sphinx.t:        url            => "sphinx://127.0.0.1:$port/rt",
t/externalauth/ldap_escaping.t:            'server'          => "127.0.0.1:$ldap_port",
t/externalauth/ldap_privileged.t:            'server'          => "127.0.0.1:$ldap_port",
t/externalauth/ldap_group.t:            'server'          => "127.0.0.1:$ldap_port",
t/externalauth/ldap.t:            'server'          => "127.0.0.1:$ldap_port",
t/externalauth/ldap.t:            'server'          => "127.0.0.1:$ldap_port",
t/externalauth/ldap_email_login.t:            'server'          => "127.0.0.1:$ldap_port",
puck@dirk:~/personal/RT/rt$ 

@puck
Copy link
Contributor Author

puck commented Sep 5, 2023

I can't remember what broke when I tried using ip6-localhost, but switching to that will cause breakage on hosts which don't have IPv6 enabled.

@puck
Copy link
Contributor Author

puck commented Sep 5, 2023

Oh, and given the amount of repeated code, standing up a test LDAP server feels like a good thing to abstract into a module. :)

@cbrandtbuffalo
Copy link
Member

Oh, and given the amount of repeated code, standing up a test LDAP server feels like a good thing to abstract into a module. :)

Yeah, that's a good thought. We have various helper modules for that reason, like lib/RT/Test.pm, lib/RT/Test/Email.pm, etc. Did you want to take a shot at updating your branch to move the that bit of LDAP server code to a module?

@puck
Copy link
Contributor Author

puck commented Sep 15, 2023

That's what I get for opening my mouth. ;) I'm working on a new lib/RT/Test/LDAP.pm file. Fortunately this is trivial to run tests for!

In the interests of not repeating code, let's centralise it.
@puck
Copy link
Contributor Author

puck commented Sep 22, 2023

I've pushed a commit which creates RT::Test::LDAP and changes all the tests for LDAP (ExternalAuth and LDAPImport) to use it.

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

Successfully merging this pull request may close these issues.

2 participants