[Rt-commit] rt branch, 4.4/add-ldap-email-authentication, created. rt-4.4.4-205-gf0a107b699

Blaine Motsinger blaine at bestpractical.com
Fri Jan 29 15:03:20 EST 2021


The branch, 4.4/add-ldap-email-authentication has been created
        at  f0a107b699e5f9eaf1abe87a9ace08cbe71655a0 (commit)

- Log -----------------------------------------------------------------
commit ed3ec68cfcebb55580a74cfb2838a2d24021e738
Author: Blaine Motsinger <blaine at bestpractical.com>
Date:   Thu Jan 21 17:44:30 2021 -0600

    Add LDAP email authentication
    
    This commit updates the LDAP authentication flow to allow users
    to authenticate using email address as well as username.
    
    The LDAP filter string was previously hardcoded to only query LDAP
    using the mapped Name field.  RT will now attempt to query LDAP
    using each of the defined keys in attr_match_list.
    
    After a match is found on the LDAP server, RT will now load the
    User object by either Name or EmailAddress, depending on what key
    returned the match.

diff --git a/lib/RT/Authen/ExternalAuth.pm b/lib/RT/Authen/ExternalAuth.pm
index bf08f8e761..4244db7be4 100644
--- a/lib/RT/Authen/ExternalAuth.pm
+++ b/lib/RT/Authen/ExternalAuth.pm
@@ -381,6 +381,7 @@ sub DoAuth {
         # safely bypass the password checking later on; primarily because
         # it's VERY unlikely we even have a password to check if an SSO succeeded.
         my $pass_bypass = 0;
+        my $exists;
         if(defined($username)) {
             $RT::Logger->debug("Pass not going to be checked, attempting SSO");
             $pass_bypass = 1;
@@ -403,7 +404,9 @@ sub DoAuth {
             # Don't continue unless the $username exists in the external service
 
             $RT::Logger->debug("Calling UserExists with \$username ($username) and \$service ($service)");
-            next unless RT::Authen::ExternalAuth::UserExists($username, $service);
+            $exists = RT::Authen::ExternalAuth::UserExists($username, $service) or next;
+
+            # TODO: we should last here if UserExists was a success
         }
 
         ####################################################################
@@ -415,7 +418,19 @@ sub DoAuth {
 
         # Does user already exist internally to RT?
         $session->{'CurrentUser'} = RT::CurrentUser->new();
-        $session->{'CurrentUser'}->Load($username);
+
+        if ( ref $exists && defined $exists->{'EmailAddress'} && $exists->{'EmailAddress'} eq $username ) {
+            $session->{'CurrentUser'}->LoadByEmail($username);
+        }
+        else {
+            $session->{'CurrentUser'}->Load($username);
+        }
+
+        # If LDAP search found the user, and Name was returned, ensure $username is set to Name.
+        # We want to try and ensure the autocreated user below has Name as name and not EmailAddress.
+        if ( ref $exists && $exists->{'Name'} ) {
+            $username = $exists->{'Name'};
+        }
 
         # Unless we have loaded a valid user with a UserID create one.
         unless ($session->{'CurrentUser'}->Id) {
diff --git a/lib/RT/Authen/ExternalAuth/LDAP.pm b/lib/RT/Authen/ExternalAuth/LDAP.pm
index 2cc53f713a..91e5ab0fdd 100644
--- a/lib/RT/Authen/ExternalAuth/LDAP.pm
+++ b/lib/RT/Authen/ExternalAuth/LDAP.pm
@@ -207,6 +207,12 @@ sub GetAuth {
     my $group_scope     = $config->{'group_scope'} || 'base';
     my $attr_map        = $config->{'attr_map'};
     my @attrs           = ('dn');
+    my $attr_match_list = $config->{'attr_match_list'};
+
+    foreach my $attr_match (@{$attr_match_list}) {
+        push @attrs, $attr_map->{$attr_match}
+            if exists $attr_map->{$attr_match} && defined $attr_map->{$attr_match};
+    }
 
     # Make sure we fetch the user attribute we'll need for the group check
     push @attrs, $group_attr_val
@@ -221,45 +227,62 @@ sub GetAuth {
     my $ldap = _GetBoundLdapObj($config);
     return 0 unless ($ldap);
 
-    $filter = Net::LDAP::Filter->new(   '(&(' .
-                                        $attr_map->{'Name'} .
-                                        '=' .
-                                        escape_filter_value($username) .
-                                        ')' .
-                                        $filter .
-                                        ')'
-                                    );
+    # loop over each of the attr_match_list members for LDAP search
+    my $ldap_msg;
+    foreach my $attr_match ( @{$attr_match_list} ) {
+        unless ( exists $attr_map->{$attr_match} && defined $attr_map->{$attr_match} ) {
+            $RT::Logger->error( "Invalid LDAP mapping for $attr_match, no defined fields in attr_map" );
+            next;
+        }
 
-    $RT::Logger->debug( "LDAP Search === ",
-                        "Base:",
-                        $base,
-                        "== Filter:",
-                        $filter->as_string,
-                        "== Attrs:",
-                        join(',', at attrs));
+        my $search_filter = Net::LDAP::Filter->new(
+            '(&' .
+            $filter .
+            '(' .
+            $attr_map->{$attr_match} .
+            '=' .
+            escape_filter_value($username) .
+            '))'
+        );
 
-    my $ldap_msg = $ldap->search(   base   => $base,
-                                    filter => $filter,
-                                    attrs  => \@attrs);
+        $RT::Logger->debug( "LDAP Search === ",
+                            "Base:",
+                            $base,
+                            "== Filter:",
+                            ($search_filter ? $search_filter->as_string : ''),
+                            "== Attrs:",
+                            join(',', at attrs) );
+
+        $ldap_msg = $ldap->search( base   => $base,
+                                   filter => $search_filter,
+                                   attrs  => \@attrs );
+
+        unless ( $ldap_msg->code == LDAP_SUCCESS || $ldap_msg->code == LDAP_PARTIAL_RESULTS ) {
+            $RT::Logger->critical( "search for",
+                                   $search_filter->as_string,
+                                   "failed:",
+                                   ldap_error_name($ldap_msg->code),
+                                   $ldap_msg->code );
+            # Didn't even get a partial result - jump straight to the next external auth service
+            return 0;
+        }
 
-    unless ($ldap_msg->code == LDAP_SUCCESS || $ldap_msg->code == LDAP_PARTIAL_RESULTS) {
-        $RT::Logger->debug( "search for",
-                            $filter->as_string,
-                            "failed:",
-                            ldap_error_name($ldap_msg->code),
-                            $ldap_msg->code);
-        # Didn't even get a partial result - jump straight to the next external auth service
-        return 0;
+        if ( $ldap_msg->count != 1 ) {
+            $RT::Logger->info( $service,
+                               "AUTH FAILED:",
+                               $username,
+                               "User not found or more than one user found" );
+            # We got no user, or too many users.. try the next attr_match_list field.
+            next;
+        }
+        else {
+            # User was found
+            last;
+        }
     }
 
-    unless ($ldap_msg->count == 1) {
-        $RT::Logger->info(  $service,
-                            "AUTH FAILED:",
-                            $username,
-                            "User not found or more than one user found");
-        # We got no user, or too many users.. jump straight to the next external auth service
-        return 0;
-    }
+    # if we didn't match anything, go to the next external auth service
+    return 0 unless $ldap_msg->first_entry;
 
     my $ldap_entry = $ldap_msg->first_entry;
     my $ldap_dn    = $ldap_entry->dn;
@@ -298,7 +321,7 @@ sub GetAuth {
 
         # We only need the dn for the actual group since all we care about is existence
         @attrs  = qw(dn);
-        $filter = Net::LDAP::Filter->new("(${group_attr}=" . escape_filter_value($group_val) . ")");
+        my $search_filter = Net::LDAP::Filter->new("(${group_attr}=" . escape_filter_value($group_val) . ")");
 
         $RT::Logger->debug( "LDAP Search === ",
                             "Base:",
@@ -306,12 +329,12 @@ sub GetAuth {
                             "== Scope:",
                             $group_scope,
                             "== Filter:",
-                            $filter->as_string,
+                            $search_filter->as_string,
                             "== Attrs:",
                             join(',', at attrs));
 
         $ldap_msg = $ldap->search(  base   => $group,
-                                    filter => $filter,
+                                    filter => $search_filter,
                                     attrs  => \@attrs,
                                     scope  => $group_scope);
 
@@ -319,7 +342,7 @@ sub GetAuth {
         unless ($ldap_msg->code == LDAP_SUCCESS ||
                 $ldap_msg->code == LDAP_PARTIAL_RESULTS) {
             $RT::Logger->critical(  "Search for",
-                                    $filter->as_string,
+                                    $search_filter->as_string,
                                     "failed:",
                                     ldap_error_name($ldap_msg->code),
                                     $ldap_msg->code);
@@ -536,57 +559,103 @@ sub UserExists {
     # but there's no harm in doing this to be sure
     undef $filter if defined $filter and $filter eq "()";
 
-    if (defined($config->{'attr_map'}->{'Name'})) {
-        # Construct the complex filter
-        $filter = Net::LDAP::Filter->new(           '(&' .
-                                                    $filter .
-                                                    '(' .
-                                                    $config->{'attr_map'}->{'Name'} .
-                                                    '=' .
-                                                    escape_filter_value($username) .
-                                                    '))'
-                                        );
+    my $attr_map        = $config->{'attr_map'};
+    my $attr_match_list = $config->{'attr_match_list'};
+
+    my @attrs;
+    foreach my $attr_match (@{$attr_match_list}) {
+        push @attrs, $attr_map->{$attr_match}
+            if exists $attr_map->{$attr_match} && defined $attr_map->{$attr_match};
     }
 
-    my $ldap = _GetBoundLdapObj($config);
-    return unless $ldap;
+    # Ensure we try to get back a Name value from LDAP on the initial LDAP search.
+    my $name_attr;
+    if ( defined $attr_map->{'Name'} ) {
+        push @attrs, $attr_map->{'Name'};
+        $name_attr = $attr_map->{'Name'};
+    }
 
-    my @attrs = values(%{$config->{'attr_map'}});
+    # loop over each of the attr_match_list members for the initial lookup
+    foreach my $attr_match ( @{$attr_match_list} ) {
+        unless ( exists $attr_map->{$attr_match} && defined $attr_map->{$attr_match} ) {
+            $RT::Logger->error( "Invalid LDAP mapping for $attr_match, no defined fields in attr_map" );
+            next;
+        }
 
-    # Check that the user exists in the LDAP service
-    $RT::Logger->debug( "LDAP Search === ",
-                        "Base:",
-                        $base,
-                        "== Filter:",
-                        ($filter ? $filter->as_string : ''),
-                        "== Attrs:",
-                        join(',', at attrs));
+        my $search_filter = Net::LDAP::Filter->new(
+            '(&' .
+            $filter .
+            '(' .
+            $attr_map->{$attr_match} .
+            '=' .
+            escape_filter_value($username) .
+            '))'
+        );
 
-    my $user_found = $ldap->search( base    => $base,
-                                    filter  => $filter,
-                                    attrs   => \@attrs);
+        my $ldap = _GetBoundLdapObj($config);
+        return unless $ldap;
 
-    if($user_found->count < 1) {
-        # If 0 or negative integer, no user found or major failure
-        $RT::Logger->debug( "User Check Failed :: (",
-                            $service,
-                            ")",
-                            $username,
-                            "User not found");
-        return 0;
-    } elsif ($user_found->count > 1) {
-        # If more than one result returned, die because we the username field should be unique!
-        $RT::Logger->debug( "User Check Failed :: (",
-                            $service,
-                            ")",
-                            $username,
-                            "More than one user with that username!");
-        return 0;
+        # Check that the user exists in the LDAP service
+        $RT::Logger->debug( "LDAP Search === ",
+                            "Base:",
+                            $base,
+                            "== Filter:",
+                            ($search_filter ? $search_filter->as_string : ''),
+                            "== Attrs:",
+                            join(',', at attrs) );
+
+        my $user_found = $ldap->search( base   => $base,
+                                        filter => $search_filter,
+                                        attrs  => \@attrs );
+
+        unless ( $user_found->code == LDAP_SUCCESS || $user_found->code == LDAP_PARTIAL_RESULTS ) {
+            $RT::Logger->debug( "search for",
+                                $filter->as_string,
+                                "failed:",
+                                ldap_error_name($user_found->code),
+                                $user_found->code );
+            # Didn't even get a partial result - jump straight to the next external auth service
+            return 0;
+        }
+
+        if ( $user_found->count < 1 ) {
+            # If 0 or negative integer, no user found or major failure
+            $RT::Logger->debug( "User Check Failed :: (",
+                                $service,
+                                ")",
+                                $username,
+                                "User not found" );
+            next;
+        }
+        elsif ( $user_found->count > 1 ) {
+            # If more than one result returned, jump to the next attr because the username field should be unique!
+            $RT::Logger->debug( "User Check Failed :: (",
+                                $service,
+                                ")",
+                                $username,
+                                "More than one user with that username!" );
+            next;
+        }
+        else {
+            # User was found
+            my $match = {
+                $attr_match => $username,
+            };
+
+            if ( $attr_match ne 'Name' && $name_attr ) {
+                my $ldap_entry = $user_found->first_entry;
+                my $name_value = $ldap_entry->get_value($name_attr);
+
+                $match->{'Name'} = $name_value;
+            }
+
+            return $match;
+        }
     }
-    undef $user_found;
 
-    # If we havent returned now, there must be a valid user.
-    return 1;
+    # No valid user was found using each of the search filters.
+    # go to the next external auth service.
+    return 0;
 }
 
 sub UserDisabled {

commit f0a107b699e5f9eaf1abe87a9ace08cbe71655a0
Author: Blaine Motsinger <blaine at bestpractical.com>
Date:   Tue Jan 19 18:46:43 2021 -0600

    Add test for LDAP attr search and match
    
    This test verifies LDAP authentication using a second entry in
    attr_match_list.  In this case EmailAddress instead of the
    previously hardcoded Name key.

diff --git a/t/externalauth/ldap_attr.t b/t/externalauth/ldap_attr.t
new file mode 100644
index 0000000000..5c3a07eca6
--- /dev/null
+++ b/t/externalauth/ldap_attr.t
@@ -0,0 +1,94 @@
+use strict;
+use warnings;
+
+use RT::Test tests => undef;
+
+eval { require RT::Authen::ExternalAuth; require Net::LDAP::Server::Test; 1; } or do {
+    plan skip_all => 'Unable to test without Net::LDAP and Net::LDAP::Server::Test';
+};
+
+my $ldap_port = RT::Test->find_idle_port;
+ok( my $server = Net::LDAP::Server::Test->new( $ldap_port, auto_schema => 1 ),
+    "spawned test LDAP server on port $ldap_port" );
+
+my $ldap = Net::LDAP->new( "localhost:$ldap_port" );
+$ldap->bind();
+
+my $username = 'testldapuser';
+my $email    = "$username\@example.com";
+my $password = 'password';
+my $base     = 'dc=bestpractical,dc=com';
+my $dn       = "uid=$username,$base";
+my $entry    = {
+    cn           => $username,
+    mail         => $email,
+    uid          => $username,
+    objectClass  => 'User',
+    userPassword => $password,
+};
+
+$ldap->add( $base );
+$ldap->add( $dn, attr => [%$entry] );
+
+RT->Config->Set( ExternalAuthPriority        => ['My_LDAP'] );
+RT->Config->Set( ExternalInfoPriority        => ['My_LDAP'] );
+RT->Config->Set( AutoCreateNonExternalUsers  => 0 );
+RT->Config->Set( AutoCreate  => undef );
+RT->Config->Set(
+    ExternalSettings => {
+        'My_LDAP' => {
+            'type'            => 'ldap',
+            'server'          => "127.0.0.1:$ldap_port",
+            'base'            => $base,
+            'filter'          => '(objectClass=*)',
+            'd_filter'        => '()',
+            'tls'             => 0,
+            'net_ldap_args'   => [ version => 3 ],
+            'attr_match_list' => [ 'Name', 'EmailAddress' ],
+            'attr_map'        => {
+                'Name'         => 'uid',
+                'EmailAddress' => 'mail',
+            }
+        },
+    }
+);
+RT->Config->PostLoadCheck;
+
+my ( $baseurl, $m ) = RT::Test->started_ok();
+
+# create user, but don't set the email address.
+# after authentication, email address will be retrieved from LDAP and set for
+# the user by CanonicalizeUserInfo.
+my $testuser = RT::User->new( $RT::SystemUser );
+my ( $uid, $msg ) = $testuser->Create(
+    Name => $username,
+);
+ok( $uid, "created $username" );
+$testuser->SetEmailAddress('');
+ok( !$testuser->EmailAddress, "$username email address is not set" );
+
+diag 'test login with Name';
+$m->get_ok( $baseurl, 'base url' );
+$m->submit_form(
+    form_number => 1,
+    fields      => { user => $username, pass => 'password', },
+);
+$m->text_contains( 'Logout', 'logged in via form' );
+is( $m->uri, $baseurl . '/SelfService/' , 'selfservice page is displayed' );
+
+my $verifyuser = RT::User->new( $RT::SystemUser );
+$verifyuser->Load( $username );
+is( $verifyuser->EmailAddress, $email, "$username email address was retrieved from LDAP after authentication" );
+
+diag 'test login with EmailAddress';
+$m->logout;
+$m->get_ok( $baseurl, 'base url' );
+$m->submit_form(
+    form_number => 1,
+    fields      => { user => $email, pass => 'password', },
+);
+is( $m->uri, $baseurl . '/SelfService/' , 'selfservice page is displayed' );
+
+$ldap->unbind();
+
+done_testing;

-----------------------------------------------------------------------


More information about the rt-commit mailing list