[Rt-commit] rt branch, 4.4/external-auth-additional-attrs, created. rt-4.4.4-60-g30296b231

[? sunnavy]

The branch, 4.4/external-auth-additional-attrs has been created
        at  30296b2317bfb6ad53c1a60bb988fa18504a7ecd (commit)

- Log -----------------------------------------------------------------
commit 30296b2317bfb6ad53c1a60bb988fa18504a7ecd
Author: sunnavy <sunnavy at bestpractical.com>
Date:   Fri Aug 30 03:19:29 2019 +0800

    Add additional_attrs support for external auth
    
    Previously subroutine references in attr_map had 2 modes, to get attr
    names and values, respectively, which is a bit confusing.
    
    This commit adds additional_attrs config, where people can put attr
    names used in subroutine references into. In this way subroutine
    references is only used to get values, which is clearer.

diff --git a/lib/RT/Authen/ExternalAuth.pm b/lib/RT/Authen/ExternalAuth.pm
index bf08f8e76..8e9c8e935 100644
--- a/lib/RT/Authen/ExternalAuth.pm
+++ b/lib/RT/Authen/ExternalAuth.pm
@@ -233,8 +233,6 @@ mapping. E.g.
     YYY => sub {
         my %args = @_;
 
-        return 'XXX' unless $args{external_entry};
-
         my @values = grep defined && length, $args{external_entry}->get_value('XXX');
         return @values;
     },
@@ -258,11 +256,6 @@ The currently processed key and value from the mapping.
 
 =back
 
-The subroutine is called in 2 modes: when called with external_entry
-specified, it should return value or list of values, otherwise, it should
-return the external field list it depends on, so RT could retrieve them at the
-beginning.
-
 =back
 
 The keys in the mapping (i.e. the RT fields, the left hand side) may be a user
@@ -280,6 +273,15 @@ etc).  You might use this to populate a ticket Location CF with all the
 locations of your users so that tickets can be associated with the locations
 in use.
 
+=item additional_attrs
+
+Additional attributes to query in the external source.  For example:
+
+    'additional_attrs' => [ 'foo', 'bar' ]
+
+Subroutine references in C<attr_mapping> might require more attributes,
+which could be added here.
+
 =back
 
 =back
diff --git a/lib/RT/Authen/ExternalAuth/DBI.pm b/lib/RT/Authen/ExternalAuth/DBI.pm
index 13f0bf08e..e866daff5 100644
--- a/lib/RT/Authen/ExternalAuth/DBI.pm
+++ b/lib/RT/Authen/ExternalAuth/DBI.pm
@@ -389,7 +389,8 @@ sub CanonicalizeUserInfo {
     my @attrs;
     for my $field ( values %{ $config->{'attr_map'} } ) {
         if ( ref $field eq 'CODE' ) {
-            push @attrs, $field->();
+            # extra attributes used in CODE should be put into 'additional_attrs'
+            next;
         }
         elsif ( ref $field eq 'ARRAY' ) {
             push @attrs, @$field;
@@ -398,6 +399,8 @@ sub CanonicalizeUserInfo {
             push @attrs, $field;
         }
     }
+    push @attrs, @{ $config->{additional_attrs} || [] };
+
     my $fields = join(',', uniq grep defined && length, @attrs);
     my $query = "SELECT $fields FROM $table WHERE $where_key=?";
     my @bind_params = ($where_value);
diff --git a/lib/RT/Authen/ExternalAuth/LDAP.pm b/lib/RT/Authen/ExternalAuth/LDAP.pm
index 2cc53f713..a69cd7b52 100644
--- a/lib/RT/Authen/ExternalAuth/LDAP.pm
+++ b/lib/RT/Authen/ExternalAuth/LDAP.pm
@@ -375,7 +375,8 @@ sub CanonicalizeUserInfo {
     my @attrs;
     for my $field ( values %{ $config->{'attr_map'} } ) {
         if ( ref $field eq 'CODE' ) {
-            push @attrs, $field->();
+            # extra attributes used in CODE should be put into 'additional_attrs'
+            next;
         }
         elsif ( ref $field eq 'ARRAY' ) {
             push @attrs, @$field;
@@ -384,6 +385,7 @@ sub CanonicalizeUserInfo {
             push @attrs, $field;
         }
     }
+    push @attrs, @{ $config->{additional_attrs} || [] };
 
     # This is a bit confusing and probably broken. Something to revisit..
     my $filter_addition = ($key && $value) ? "(". $key . "=". escape_filter_value($value) .")" : "";
diff --git a/t/externalauth/ldap.t b/t/externalauth/ldap.t
index 671d3fcd9..1922ba2b0 100644
--- a/t/externalauth/ldap.t
+++ b/t/externalauth/ldap.t
@@ -74,13 +74,13 @@ RT->Config->Set(
                 'UserCF.Employee Type' => 'employeeType',
                 'UserCF.Employee ID'   => sub {
                     my %args = @_;
-                    return ( 'employeeType', 'employeeID' ) unless $args{external_entry};
                     return (
                         $args{external_entry}->get_value('employeeType') // '',
                         $args{external_entry}->get_value('employeeID') // '',
                     );
                 },
-            }
+            },
+            additional_attrs => [ 'employeeID' ],
         },
     }
 );
diff --git a/t/externalauth/sqlite.t b/t/externalauth/sqlite.t
index a788de17e..327da50c2 100644
--- a/t/externalauth/sqlite.t
+++ b/t/externalauth/sqlite.t
@@ -81,10 +81,10 @@ RT->Config->Set(
                 'UserCF.Employee Type' => 'employee_type',
                 'UserCF.Employee ID'   => sub {
                     my %args = @_;
-                    return ( 'employee_type', 'employee_id' ) unless $args{external_entry};
                     return ( $args{external_entry}->{employee_type}, $args{external_entry}->{employee_id} );
                 },
-            }
+            },
+            additional_attrs => [ 'employee_id' ],
         },
     }
 );

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