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

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


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

- Log -----------------------------------------------------------------
commit 57b60e5c774d01004df68a5280595198fcce9a0e
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..75480ca078 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 a user was found during the LDAP search in UserExists, we need to ensure
+        # $username is Name instead of EmailAddress, if the user used that to auth.
+        if ( ref $exists eq 'RT::User' ) {
+            $username = $exists->Name;
+        }
+        # This check is strange, but we need to also allow for other ExternalAuth types to return 1
+        # for UserExists, while still checking for a valid username from LDAP.
+        elsif ( $exists !~ /^1$/ ) {
+            $username = $exists;
+        }
+
+        $session->{CurrentUser}->Load($username);
 
         # 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..c705ecd450 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 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 ( 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,104 @@ 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 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.
+    push @attrs, $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 ( 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 in LDAP
+            my $match = RT::User->new($RT::SystemUser);
+            if ( $attr_match eq 'EmailAddress' ) {
+                $match->LoadByEmail($username);
+            }
+            else {
+                $match->Load($username);
+            }
+
+            # If the user doesn't exist in RT, return the Name value we got from LDAP
+            # incase we need to create the user in RT.
+            unless ( $match->Id ) {
+                my $ldap_entry = $user_found->first_entry;
+                my $name_value = $ldap_entry->get_value($attr_map->{'Name'});
+                $match         = $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 64f64c1521c0abddf5bc88cc6a195d9654dc9fcc
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