From c74678a1d239aaf91906039ccb3db940df4a0b41 Mon Sep 17 00:00:00 2001
From: Alex Arnaud <alex.arnaud@biblibre.com>
Date: Wed, 24 Feb 2016 10:13:40 +0100
Subject: [PATCH] Bug 15889: LDAP authentication: Only update mapped attributes

Test plan:

- Update your configuration file to use LDAP authentication and enable update
  (<update>1</update>) option,
- login with an existing user with extended attrbitutes that are not in
LDAP mapping,
- check that all attributes are still here.

Signed-off-by: Chris <chrisc@catalyst.net.nz>

Signed-off-by: Philippe Blouin <philippe.blouin@inlibro.com>

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
---
 C4/Auth_with_ldap.pm            | 18 +++++--------
 t/db_dependent/Auth_with_ldap.t | 48 +++++++++++++++++++++++++++++++--
 2 files changed, 52 insertions(+), 14 deletions(-)

diff --git a/C4/Auth_with_ldap.pm b/C4/Auth_with_ldap.pm
index 4a9c302417..14db437c72 100644
--- a/C4/Auth_with_ldap.pm
+++ b/C4/Auth_with_ldap.pm
@@ -206,25 +206,19 @@ sub checkpw_ldap {
         return 0;   # B2, D2
     }
     if (C4::Context->preference('ExtendedPatronAttributes') && $borrowernumber && ($config{update} ||$config{replicate})) {
-        my @extended_patron_attributes;
         foreach my $attribute_type ( C4::Members::AttributeTypes::GetAttributeTypes() ) {
             my $code = $attribute_type->{code};
-            if ( exists($borrower{$code}) && $borrower{$code} !~ m/^\s*$/ ) { # skip empty values
-                push @extended_patron_attributes, { code => $code, value => $borrower{$code} };
+            unless (exists($borrower{$code}) && $borrower{$code} !~ m/^\s*$/ ) {
+                next;
             }
-        }
-        #Check before add
-        my @unique_attr;
-        foreach my $attr ( @extended_patron_attributes ) {
-            if (C4::Members::Attributes::CheckUniqueness($attr->{code}, $attr->{value}, $borrowernumber)) {
-                push @unique_attr, $attr;
+            if (C4::Members::Attributes::CheckUniqueness($code, $borrower{$code}, $borrowernumber)) {
+                C4::Members::Attributes::UpdateBorrowerAttribute($borrowernumber, {code => $code, value => $borrower{$code}});
             } else {
-                warn "ERROR_extended_unique_id_failed $attr->{code} $attr->{value}";
+                warn "ERROR_extended_unique_id_failed $code $borrower{$code}";
             }
         }
-        C4::Members::Attributes::SetBorrowerAttributes($borrowernumber, \@unique_attr, 'no_branch_limit');
     }
-return(1, $cardnumber, $userid);
+    return(1, $cardnumber, $userid);
 }
 
 # Pass LDAP entry object and local cardnumber (userid).
diff --git a/t/db_dependent/Auth_with_ldap.t b/t/db_dependent/Auth_with_ldap.t
index 4982248c8e..9d68055aa3 100755
--- a/t/db_dependent/Auth_with_ldap.t
+++ b/t/db_dependent/Auth_with_ldap.t
@@ -20,6 +20,7 @@ use Modern::Perl;
 use Test::More tests => 4;
 use Test::MockModule;
 use Test::MockObject;
+use t::lib::TestBuilder;
 use Test::Warn;
 
 use C4::Context;
@@ -29,6 +30,8 @@ my $dbh = C4::Context->dbh;
 $dbh->{ AutoCommit } = 0;
 $dbh->{ RaiseError } = 1;
 
+my $builder = t::lib::TestBuilder->new();
+
 # Variables controlling LDAP server config
 my $update         = 0;
 my $replicate      = 0;
@@ -61,6 +64,32 @@ $ldap->mock( 'new',  sub {
     }
 });
 
+my $categorycode = $builder->build({ source => 'Category' })->{ categorycode };
+my $branchcode = $builder->build({ source => 'Branch' })->{ branchcode };
+my $attrType = $builder->build({
+    source => 'BorrowerAttributeType',
+    value => {
+        category_code => $categorycode
+    }
+});
+
+my $borrower = $builder->build({
+    source => 'Borrower',
+    value => {
+        userid => 'hola',
+        branchcode   => $branchcode,
+        categorycode => $categorycode
+    }
+});
+
+$builder->build({
+    source => 'BorrowerAttribute',
+    value => {
+        borrowernumber => $borrower->{borrowernumber},
+        code => $attrType->{code},
+        attribute => 'FOO'
+    }
+});
 
 # C4::Auth_with_ldap needs several stuff set first ^^^
 use_ok( 'C4::Auth_with_ldap' );
@@ -84,7 +113,7 @@ subtest "checkpw_ldap tests" => sub {
 
     subtest "auth_by_bind = 1 tests" => sub {
 
-        plan tests => 7;
+        plan tests => 8;
 
         $auth_by_bind          = 1;
 
@@ -105,8 +134,23 @@ subtest "checkpw_ldap tests" => sub {
         $anonymous_bind        = 1;
         $desired_bind_result   = 'success';
         $desired_search_result = 'success';
-        $desired_count_result  = 0; # user auth problem
+        $desired_count_result  = 1;
         $non_anonymous_bind_result = 'success';
+        $update = 1;
+        reload_ldap_module();
+
+        my $auth = new Test::MockModule('C4::Auth_with_ldap');
+        $auth->mock( 'update_local', sub {
+                return $borrower->{cardnumber};
+        });
+
+        C4::Auth_with_ldap::checkpw_ldap( $dbh, 'hola', password => 'hey');
+        ok(@{ C4::Members::Attributes::GetBorrowerAttributes($borrower->{borrowernumber}) }, 'Extended attributes are not deleted');
+        $auth->unmock('update_local');
+
+        $update = 0;
+        $desired_count_result = 0; # user auth problem
+        C4::Members::DelMember($borrower->{borrowernumber});
         reload_ldap_module();
         is ( C4::Auth_with_ldap::checkpw_ldap( $dbh, 'hola', password => 'hey' ),
             0, "checkpw_ldap returns 0 if user lookup returns 0");
-- 
2.39.5