From 3345a72e7e4a5d11b1140d17b46567b65055f2e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Pablo=20Villaf=C3=A1=C3=B1ez?= Date: Tue, 14 Mar 2017 14:07:33 +0100 Subject: [PATCH 1/6] Correctly apply quota Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/User/User.php | 39 +++++++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/apps/user_ldap/lib/User/User.php b/apps/user_ldap/lib/User/User.php index e29b10616c..7a840de87f 100644 --- a/apps/user_ldap/lib/User/User.php +++ b/apps/user_ldap/lib/User/User.php @@ -169,6 +169,10 @@ class User { $attr = strtolower($this->connection->ldapQuotaAttribute); if(isset($ldapEntry[$attr])) { $this->updateQuota($ldapEntry[$attr][0]); + } else { + if ($this->connection->ldapQuotaDefault !== '') { + $this->updateQuota(); + } } unset($attr); @@ -464,25 +468,48 @@ class User { if($this->wasRefreshed('quota')) { return; } - //can be null - $quotaDefault = $this->connection->ldapQuotaDefault; - $quota = $quotaDefault !== '' ? $quotaDefault : null; - $quota = !is_null($valueFromLDAP) ? $valueFromLDAP : $quota; + $quota = false; if(is_null($valueFromLDAP)) { $quotaAttribute = $this->connection->ldapQuotaAttribute; if ($quotaAttribute !== '') { $aQuota = $this->access->readAttribute($this->dn, $quotaAttribute); if($aQuota && (count($aQuota) > 0)) { - $quota = $aQuota[0]; + if ($this->verifyQuotaValue($aQuota[0])) { + $quota = $aQuota[0]; + } else { + $this->log->log('not suitable LDAP quota found for user ' . $this->uid . ': [' . $aQuota[0] . ']', \OCP\Util::WARN); + } } } + } else { + if ($this->verifyQuotaValue($valueFromLDAP)) { + $quota = $valueFromLDAP; + } else { + $this->log->log('not suitable LDAP quota found for user ' . $this->uid . ': [' . $valueFromLDAP . ']', \OCP\Util::WARN); + } } - if(!is_null($quota)) { + + if ($quota === false) { + // quota not found using the LDAP attribute (or not parseable). Try the default quota + $defaultQuota = $this->connection->ldapQuotaDefault; + if ($this->verifyQuotaValue($defaultQuota)) { + $quota = $defaultQuota; + } + } + + if($quota !== false) { $this->userManager->get($this->uid)->setQuota($quota); + } else { + $this->log->log('not suitable default quota found for user ' . $this->uid . ': [' . $defaultQuota . ']', \OCP\Util::WARN); + $this->userManager->get($this->uid)->setQuota('default'); } } + private function verifyQuotaValue($quotaValue) { + return $quotaValue === 'none' || $quotaValue === 'default' || \OC_Helper::computerFileSize($quotaValue) !== false; + } + /** * called by a post_login hook to save the avatar picture * From f9832ff3475a1feec9d8b8d2d42bbf56d72c0118 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Pablo=20Villaf=C3=A1=C3=B1ez?= Date: Tue, 14 Mar 2017 17:08:05 +0100 Subject: [PATCH 2/6] Check if the user exists before trying to set the quota Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/User/User.php | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/apps/user_ldap/lib/User/User.php b/apps/user_ldap/lib/User/User.php index 7a840de87f..392cb2e51b 100644 --- a/apps/user_ldap/lib/User/User.php +++ b/apps/user_ldap/lib/User/User.php @@ -498,11 +498,16 @@ class User { } } - if($quota !== false) { - $this->userManager->get($this->uid)->setQuota($quota); + $targetUser = $this->userManager->get($this->uid); + if ($targetUser) { + if($quota !== false) { + $targetUser->setQuota($quota); + } else { + $this->log->log('not suitable default quota found for user ' . $this->uid . ': [' . $defaultQuota . ']', \OCP\Util::WARN); + $targetUser->setQuota('default'); + } } else { - $this->log->log('not suitable default quota found for user ' . $this->uid . ': [' . $defaultQuota . ']', \OCP\Util::WARN); - $this->userManager->get($this->uid)->setQuota('default'); + $this->log->log('trying to set a quota for user ' . $this->uid . ' but the user is missing', \OCP\Util::ERROR); } } From e9b96062e68d1e09055f4f2e7dac18161c062609 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Pablo=20Villaf=C3=A1=C3=B1ez?= Date: Wed, 15 Mar 2017 08:55:14 +0100 Subject: [PATCH 3/6] Adjust current tests to match the expectations Signed-off-by: Arthur Schiwon --- apps/user_ldap/tests/User/UserTest.php | 58 +++++++++++++------------- 1 file changed, 28 insertions(+), 30 deletions(-) diff --git a/apps/user_ldap/tests/User/UserTest.php b/apps/user_ldap/tests/User/UserTest.php index c261a2abaa..7a0e40b856 100644 --- a/apps/user_ldap/tests/User/UserTest.php +++ b/apps/user_ldap/tests/User/UserTest.php @@ -210,16 +210,18 @@ class UserTest extends \Test\TestCase { $this->getAdvancedMocks($config, $filesys, $log, $avaMgr, $dbc); $connection->expects($this->at(0)) - ->method('__get') - ->with($this->equalTo('ldapQuotaDefault')) - ->will($this->returnValue('23 GB')); - - $connection->expects($this->at(1)) ->method('__get') ->with($this->equalTo('ldapQuotaAttribute')) ->will($this->returnValue('myquota')); - $connection->expects($this->exactly(2)) + /* Having a quota defined, the ldapQuotaDefault won't be used + $connection->expects($this->at(1)) + ->method('__get') + ->with($this->equalTo('ldapQuotaDefault')) + ->will($this->returnValue('23 GB')); + */ + + $connection->expects($this->exactly(1)) ->method('__get'); $access->expects($this->once()) @@ -256,13 +258,13 @@ class UserTest extends \Test\TestCase { $connection->expects($this->at(0)) ->method('__get') - ->with($this->equalTo('ldapQuotaDefault')) - ->will($this->returnValue('25 GB')); + ->with($this->equalTo('ldapQuotaAttribute')) + ->will($this->returnValue('myquota')); $connection->expects($this->at(1)) ->method('__get') - ->with($this->equalTo('ldapQuotaAttribute')) - ->will($this->returnValue('myquota')); + ->with($this->equalTo('ldapQuotaDefault')) + ->will($this->returnValue('25 GB')); $connection->expects($this->exactly(2)) ->method('__get'); @@ -300,16 +302,18 @@ class UserTest extends \Test\TestCase { $this->getAdvancedMocks($config, $filesys, $log, $avaMgr, $dbc); $connection->expects($this->at(0)) - ->method('__get') - ->with($this->equalTo('ldapQuotaDefault')) - ->will($this->returnValue('')); - - $connection->expects($this->at(1)) ->method('__get') ->with($this->equalTo('ldapQuotaAttribute')) ->will($this->returnValue('myquota')); - $connection->expects($this->exactly(2)) + /* Having a quota set this won't be used + $connection->expects($this->at(1)) + ->method('__get') + ->with($this->equalTo('ldapQuotaDefault')) + ->will($this->returnValue('')); + */ + + $connection->expects($this->exactly(1)) ->method('__get'); $access->expects($this->once()) @@ -346,13 +350,13 @@ class UserTest extends \Test\TestCase { $connection->expects($this->at(0)) ->method('__get') - ->with($this->equalTo('ldapQuotaDefault')) - ->will($this->returnValue('')); + ->with($this->equalTo('ldapQuotaAttribute')) + ->will($this->returnValue('myquota')); $connection->expects($this->at(1)) ->method('__get') - ->with($this->equalTo('ldapQuotaAttribute')) - ->will($this->returnValue('myquota')); + ->with($this->equalTo('ldapQuotaDefault')) + ->will($this->returnValue('')); $connection->expects($this->exactly(2)) ->method('__get'); @@ -384,12 +388,12 @@ class UserTest extends \Test\TestCase { $connection->expects($this->at(0)) ->method('__get') - ->with($this->equalTo('ldapQuotaDefault')) + ->with($this->equalTo('ldapQuotaAttribute')) ->will($this->returnValue('')); $connection->expects($this->at(1)) ->method('__get') - ->with($this->equalTo('ldapQuotaAttribute')) + ->with($this->equalTo('ldapQuotaDefault')) ->will($this->returnValue('')); $connection->expects($this->exactly(2)) @@ -419,15 +423,9 @@ class UserTest extends \Test\TestCase { $readQuota = '19 GB'; - $connection->expects($this->at(0)) + $connection->expects($this->never()) ->method('__get') - ->with($this->equalTo('ldapQuotaDefault')) - ->will($this->returnValue('')); - - $connection->expects($this->once(1)) - ->method('__get') - ->with($this->equalTo('ldapQuotaDefault')) - ->will($this->returnValue(null)); + ->with($this->equalTo('ldapQuotaDefault')); $access->expects($this->never()) ->method('readAttribute'); From ebd1a7d4b2fca32edb9fd17ebfeaaa646cbbe495 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Pablo=20Villaf=C3=A1=C3=B1ez?= Date: Thu, 16 Mar 2017 12:22:02 +0100 Subject: [PATCH 4/6] Add new testes to cover more cases Signed-off-by: Arthur Schiwon --- apps/user_ldap/tests/User/UserTest.php | 210 +++++++++++++++++++++++++ 1 file changed, 210 insertions(+) diff --git a/apps/user_ldap/tests/User/UserTest.php b/apps/user_ldap/tests/User/UserTest.php index 7a0e40b856..b918285963 100644 --- a/apps/user_ldap/tests/User/UserTest.php +++ b/apps/user_ldap/tests/User/UserTest.php @@ -367,6 +367,16 @@ class UserTest extends \Test\TestCase { $this->equalTo('myquota')) ->will($this->returnValue(false)); + $user = $this->createMock('\OCP\IUser'); + $user->expects($this->once()) + ->method('setQuota') + ->with('default'); + + $userMgr->expects($this->once()) + ->method('get') + ->with('alice') + ->will($this->returnValue($user)); + $config->expects($this->never()) ->method('setUserValue'); @@ -399,6 +409,17 @@ class UserTest extends \Test\TestCase { $connection->expects($this->exactly(2)) ->method('__get'); + $user = $this->createMock('\OCP\IUser'); + $user->expects($this->once()) + ->method('setQuota') + ->with('default'); + + $userMgr->expects($this->once()) + ->method('get') + ->with('alice') + ->will($this->returnValue($user)); + + $access->expects($this->never()) ->method('readAttribute'); @@ -449,6 +470,195 @@ class UserTest extends \Test\TestCase { $user->updateQuota($readQuota); } + /** + * Unparseable quota will fallback to use the LDAP default + */ + public function testUpdateWrongQuotaAllProvided() { + list($access, $config, $filesys, $image, $log, $avaMgr, $dbc, $userMgr) = + $this->getTestInstances(); + + list($access, $connection) = + $this->getAdvancedMocks($config, $filesys, $log, $avaMgr, $dbc); + + $connection->expects($this->at(0)) + ->method('__get') + ->with($this->equalTo('ldapQuotaAttribute')) + ->will($this->returnValue('myquota')); + + $connection->expects($this->at(1)) + ->method('__get') + ->with($this->equalTo('ldapQuotaDefault')) + ->will($this->returnValue('23 GB')); + + $connection->expects($this->exactly(2)) + ->method('__get'); + + $access->expects($this->once()) + ->method('readAttribute') + ->with($this->equalTo('uid=alice,dc=foo,dc=bar'), + $this->equalTo('myquota')) + ->will($this->returnValue(array('42 GBwos'))); + + $user = $this->createMock('\OCP\IUser'); + $user->expects($this->once()) + ->method('setQuota') + ->with('23 GB'); + + $userMgr->expects($this->once()) + ->method('get') + ->with('alice') + ->will($this->returnValue($user)); + + $uid = 'alice'; + $dn = 'uid=alice,dc=foo,dc=bar'; + + $user = new User( + $uid, $dn, $access, $config, $filesys, $image, $log, $avaMgr, $userMgr); + + $user->updateQuota(); + } + + /** + * No user quota and wrong default will set 'default' as quota + */ + public function testUpdateWrongDefaultQuotaProvided() { + list($access, $config, $filesys, $image, $log, $avaMgr, $dbc, $userMgr) = + $this->getTestInstances(); + + list($access, $connection) = + $this->getAdvancedMocks($config, $filesys, $log, $avaMgr, $dbc); + + $connection->expects($this->at(0)) + ->method('__get') + ->with($this->equalTo('ldapQuotaAttribute')) + ->will($this->returnValue('myquota')); + + $connection->expects($this->at(1)) + ->method('__get') + ->with($this->equalTo('ldapQuotaDefault')) + ->will($this->returnValue('23 GBwowowo')); + + $connection->expects($this->exactly(2)) + ->method('__get'); + + $access->expects($this->once()) + ->method('readAttribute') + ->with($this->equalTo('uid=alice,dc=foo,dc=bar'), + $this->equalTo('myquota')) + ->will($this->returnValue(false)); + + $user = $this->createMock('\OCP\IUser'); + $user->expects($this->once()) + ->method('setQuota') + ->with('default'); + + $userMgr->expects($this->once()) + ->method('get') + ->with('alice') + ->will($this->returnValue($user)); + + $uid = 'alice'; + $dn = 'uid=alice,dc=foo,dc=bar'; + + $user = new User( + $uid, $dn, $access, $config, $filesys, $image, $log, $avaMgr, $userMgr); + + $user->updateQuota(); + } + + /** + * Wrong user quota and wrong default will set 'default' as quota + */ + public function testUpdateWrongQuotaAndDefaultAllProvided() { + list($access, $config, $filesys, $image, $log, $avaMgr, $dbc, $userMgr) = + $this->getTestInstances(); + + list($access, $connection) = + $this->getAdvancedMocks($config, $filesys, $log, $avaMgr, $dbc); + + $connection->expects($this->at(0)) + ->method('__get') + ->with($this->equalTo('ldapQuotaAttribute')) + ->will($this->returnValue('myquota')); + + $connection->expects($this->at(1)) + ->method('__get') + ->with($this->equalTo('ldapQuotaDefault')) + ->will($this->returnValue('23 GBwowowo')); + + $connection->expects($this->exactly(2)) + ->method('__get'); + + $access->expects($this->once()) + ->method('readAttribute') + ->with($this->equalTo('uid=alice,dc=foo,dc=bar'), + $this->equalTo('myquota')) + ->will($this->returnValue(array('23 flush'))); + + $user = $this->createMock('\OCP\IUser'); + $user->expects($this->once()) + ->method('setQuota') + ->with('default'); + + $userMgr->expects($this->once()) + ->method('get') + ->with('alice') + ->will($this->returnValue($user)); + + $uid = 'alice'; + $dn = 'uid=alice,dc=foo,dc=bar'; + + $user = new User( + $uid, $dn, $access, $config, $filesys, $image, $log, $avaMgr, $userMgr); + + $user->updateQuota(); + } + + /** + * No quota attribute set and wrong default will set 'default' as quota + */ + public function testUpdateWrongDefaultQuotaNotProvided() { + list($access, $config, $filesys, $image, $log, $avaMgr, $dbc, $userMgr) = + $this->getTestInstances(); + + list($access, $connection) = + $this->getAdvancedMocks($config, $filesys, $log, $avaMgr, $dbc); + + $connection->expects($this->at(0)) + ->method('__get') + ->with($this->equalTo('ldapQuotaAttribute')) + ->will($this->returnValue('')); + + $connection->expects($this->at(1)) + ->method('__get') + ->with($this->equalTo('ldapQuotaDefault')) + ->will($this->returnValue('23 GBwowowo')); + + $connection->expects($this->exactly(2)) + ->method('__get'); + + $access->expects($this->never()) + ->method('readAttribute'); + + $user = $this->createMock('\OCP\IUser'); + $user->expects($this->once()) + ->method('setQuota') + ->with('default'); + + $userMgr->expects($this->once()) + ->method('get') + ->with('alice') + ->will($this->returnValue($user)); + + $uid = 'alice'; + $dn = 'uid=alice,dc=foo,dc=bar'; + + $user = new User( + $uid, $dn, $access, $config, $filesys, $image, $log, $avaMgr, $userMgr); + + $user->updateQuota(); + } + //the testUpdateAvatar series also implicitely tests getAvatarImage public function testUpdateAvatarJpegPhotoProvided() { list($access, $config, $filesys, $image, $log, $avaMgr, $dbc, $userMgr) = From 3676189e05d3b51e3162c4410b30ea7268048c48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Pablo=20Villaf=C3=A1=C3=B1ez?= Date: Fri, 17 Mar 2017 12:56:27 +0100 Subject: [PATCH 5/6] Add comments in the updateQuota method to explain the behavior Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/User/User.php | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/apps/user_ldap/lib/User/User.php b/apps/user_ldap/lib/User/User.php index 392cb2e51b..5d4af1fd09 100644 --- a/apps/user_ldap/lib/User/User.php +++ b/apps/user_ldap/lib/User/User.php @@ -459,6 +459,20 @@ class User { } /** + * Overall process goes as follow: + * 1. fetch the quota from LDAP and check if it's parseable with the "verifyQuotaValue" function + * 2. if the value can't be fetched, is empty or not parseable, use the default LDAP quota + * 3. if the default LDAP quota can't be parsed, use the ownCloud's default quota (use 'default') + * 4. check if the target user exists and set the quota for the user. + * + * In order to improve performance and prevent an unwanted extra LDAP call, the $valueFromLDAP + * parameter can be passed with the value of the attribute. This value will be considered as the + * quota for the user coming from the LDAP server (step 1 of the process) It can be useful to + * fetch all the user's attributes in one call and use the fetched values in this function. + * The expected value for that parameter is a string describing the quota for the user. Valid + * values are 'none' (unlimited), 'default' (the ownCloud's default quota), '1234' (quota in + * bytes), '1234 MB' (quota in MB - check the \OC_Helper::computerFileSize method for more info) + * * fetches the quota from LDAP and stores it as ownCloud user value * @param string $valueFromLDAP the quota attribute's value can be passed, * to save the readAttribute request From 4622bebdedf277ff19bdf7b261f2b4c6e78873b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Pablo=20Villaf=C3=A1=C3=B1ez?= Date: Fri, 17 Mar 2017 13:02:08 +0100 Subject: [PATCH 6/6] Include tests for "default" and "none" quotas Signed-off-by: Arthur Schiwon --- apps/user_ldap/tests/User/UserTest.php | 80 ++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/apps/user_ldap/tests/User/UserTest.php b/apps/user_ldap/tests/User/UserTest.php index b918285963..6961e39d9f 100644 --- a/apps/user_ldap/tests/User/UserTest.php +++ b/apps/user_ldap/tests/User/UserTest.php @@ -249,6 +249,86 @@ class UserTest extends \Test\TestCase { $user->updateQuota(); } + public function testUpdateQuotaToDefaultAllProvided() { + list($access, $config, $filesys, $image, $log, $avaMgr, $dbc, $userMgr) = + $this->getTestInstances(); + + list($access, $connection) = + $this->getAdvancedMocks($config, $filesys, $log, $avaMgr, $dbc); + + $connection->expects($this->at(0)) + ->method('__get') + ->with($this->equalTo('ldapQuotaAttribute')) + ->will($this->returnValue('myquota')); + + $connection->expects($this->exactly(1)) + ->method('__get'); + + $access->expects($this->once()) + ->method('readAttribute') + ->with($this->equalTo('uid=alice,dc=foo,dc=bar'), + $this->equalTo('myquota')) + ->will($this->returnValue(array('default'))); + + $user = $this->createMock('\OCP\IUser'); + $user->expects($this->once()) + ->method('setQuota') + ->with('default'); + + $userMgr->expects($this->once()) + ->method('get') + ->with('alice') + ->will($this->returnValue($user)); + + $uid = 'alice'; + $dn = 'uid=alice,dc=foo,dc=bar'; + + $user = new User( + $uid, $dn, $access, $config, $filesys, $image, $log, $avaMgr, $userMgr); + + $user->updateQuota(); + } + + public function testUpdateQuotaToNoneAllProvided() { + list($access, $config, $filesys, $image, $log, $avaMgr, $dbc, $userMgr) = + $this->getTestInstances(); + + list($access, $connection) = + $this->getAdvancedMocks($config, $filesys, $log, $avaMgr, $dbc); + + $connection->expects($this->at(0)) + ->method('__get') + ->with($this->equalTo('ldapQuotaAttribute')) + ->will($this->returnValue('myquota')); + + $connection->expects($this->exactly(1)) + ->method('__get'); + + $access->expects($this->once()) + ->method('readAttribute') + ->with($this->equalTo('uid=alice,dc=foo,dc=bar'), + $this->equalTo('myquota')) + ->will($this->returnValue(array('none'))); + + $user = $this->createMock('\OCP\IUser'); + $user->expects($this->once()) + ->method('setQuota') + ->with('none'); + + $userMgr->expects($this->once()) + ->method('get') + ->with('alice') + ->will($this->returnValue($user)); + + $uid = 'alice'; + $dn = 'uid=alice,dc=foo,dc=bar'; + + $user = new User( + $uid, $dn, $access, $config, $filesys, $image, $log, $avaMgr, $userMgr); + + $user->updateQuota(); + } + public function testUpdateQuotaDefaultProvided() { list($access, $config, $filesys, $image, $log, $avaMgr, $dbc, $userMgr) = $this->getTestInstances();