0011844: decodeSid fails for some encoded SIDs

* encoded Sids might contain dashes -.-
* adds a test
* improves logging

https://forge.tine20.org/view.php?id=11844

Change-Id: Ief81024e3d1c4cd862a587e93529717af88c7f79
Reviewed-on: http://gerrit.tine20.com/customers/3139
Tested-by: Jenkins CI (http://ci.tine20.com/)
Reviewed-by: Philipp Schüle <p.schuele@metaways.de>

diff --git a/tests/tine20/Tinebase/AllTests.php b/tests/tine20/Tinebase/AllTests.php
index 7231529..1354f87 100644 (file)
--- a/tests/tine20/Tinebase/AllTests.php
+++ b/tests/tine20/Tinebase/AllTests.php
@@ -35,13 +35,14 @@ class Tinebase_AllTests
     {
         $suite = new PHPUnit_Framework_TestSuite('Tine 2.0 Tinebase All Tests');
         $suite->addTestSuite('Tinebase_AccountTest');
+        $suite->addTestSuite('Tinebase_AuthTest');
         $suite->addTestSuite('Tinebase_CoreTest');
-        $suite->addTestSuite('Tinebase_ModelConfigurationTest');
         $suite->addTestSuite('Tinebase_DateTimeTest');
         $suite->addTestSuite('Tinebase_ExceptionTest');
+        $suite->addTestSuite('Tinebase_LdapTest');
+        $suite->addTestSuite('Tinebase_ModelConfigurationTest');
         $suite->addTestSuite('Tinebase_Record_RecordTest');
         $suite->addTestSuite('Tinebase_Record_RecordSetTest');
-        $suite->addTestSuite('Tinebase_AuthTest');
         $suite->addTestSuite('Tinebase_UserTest');
         $suite->addTestSuite('Tinebase_GroupTest');
         $suite->addTestSuite('Tinebase_ZendFilterTest');

diff --git a/tests/tine20/Tinebase/LdapTest.php b/tests/tine20/Tinebase/LdapTest.php
new file mode 100644 (file)
index 0000000..541180d
--- /dev/null
+++ b/tests/tine20/Tinebase/LdapTest.php
@@ -0,0 +1,35 @@
+<?php
+/**
+ * Tine 2.0 - http://www.tine20.org
+ * 
+ * @package     Tinebase
+ * @subpackage  Ldap
+ * @license     http://www.gnu.org/licenses/agpl.html
+ * @copyright   Copyright (c) 2016 Metaways Infosystems GmbH (http://www.metaways.de)
+ * @author      Philipp Schüle <p.schuele@metaways.de>
+ */
+
+/**
+ * Test class for Tinebase_User_Ldap
+ */
+class Tinebase_LdapTest extends TestCase
+{
+    /**
+     * @see 0011844: decodeSid fails for some encoded SIDs
+     *
+     * TODO write a test for a decoded sid, like this: ^A^E^@^@^@^@^@^E^U^@^@^@^A.z<F4>^W<B0>Ot^^<DC>^O^V<DE>-^@^@
+     *
+     * we could dump the encoded sid like this:
+     *  $str = pack('c*', $data);
+     *  for ($i=0; $i < strlen($str); ++$i) {
+     *    echo '\x' . ord($str[$i]);
+     *  }
+     */
+    public function testDecodeAlreadyDecodedSid()
+    {
+        $sid = "S-1-5-21-2127521184-1604012920-1887927527";
+
+        $decodedSid = Tinebase_Ldap::decodeSid($sid);
+        $this->assertEquals($decodedSid, $sid);
+    }
+}

diff --git a/tine20/Admin/Controller/User.php b/tine20/Admin/Controller/User.php
index 2a6cb75..d50f20c 100644 (file)
--- a/tine20/Admin/Controller/User.php
+++ b/tine20/Admin/Controller/User.php
@@ -284,8 +284,7 @@ class Admin_Controller_User extends Tinebase_Controller_Abstract
 
         } catch (Exception $e) {
             Tinebase_TransactionManager::getInstance()->rollBack();
-            Tinebase_Core::getLogger()->err(__METHOD__ . '::' . __LINE__ . ' ' . $e->getMessage());
-            if (Tinebase_Core::isLogLevel(Zend_Log::DEBUG)) Tinebase_Core::getLogger()->debug(__METHOD__ . '::' . __LINE__ . ' ' . $e->getTraceAsString());
+            Tinebase_Exception::log($e);
             throw $e;
         }
         

diff --git a/tine20/Tinebase/Ldap.php b/tine20/Tinebase/Ldap.php
index 167ffb4..7b27415 100644 (file)
--- a/tine20/Tinebase/Ldap.php
+++ b/tine20/Tinebase/Ldap.php
@@ -250,13 +250,18 @@ class Tinebase_Ldap extends Zend_Ldap
     
     /**
      * decode ActiveDirectory SID
-     * 
+     *
+     * @see https://msdn.microsoft.com/en-us/library/ff632068.aspx
+     *
      * @param  string  $binarySid  the binary encoded SID
      * @return string
+     *
+     * TODO should be moved to AD trait/abstract
      */
     public static function decodeSid($binarySid) 
     {
-        if (strpos($binarySid, '-') !== false) {
+        if (preg_match('/^S\-1/', $binarySid)) {
+            // already decoded
             return $binarySid;
         }
         

diff --git a/tine20/Tinebase/User/ActiveDirectory.php b/tine20/Tinebase/User/ActiveDirectory.php
index e2855b9..15db70f 100644 (file)
--- a/tine20/Tinebase/User/ActiveDirectory.php
+++ b/tine20/Tinebase/User/ActiveDirectory.php
@@ -161,7 +161,7 @@ class Tinebase_User_ActiveDirectory extends Tinebase_User_Ldap
         // add user to primary group and set primary group
         Tinebase_Group::getInstance()->addGroupMemberInSyncBackend($_user->accountPrimaryGroup, $userId);
         
-        // set primary goup id
+        // set primary group id
         $this->_ldap->updateProperty($dn, array('primarygroupid' => $primaryGroupId));