diff options
author | blizzz <blizzz@arthur-schiwon.de> | 2021-05-12 14:17:50 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-05-12 14:17:50 +0200 |
commit | 7e1c842d96cde01ebb6854b3e28fb998006575b2 (patch) | |
tree | 0da4f083e0db25a76c59c9d93d869fcf08809c23 | |
parent | 6c741724fbdfc71762407298738c330e5ceb9b88 (diff) | |
parent | 8413ed947577a37c79ceebfc827acd37494d1a37 (diff) | |
download | nextcloud-server-7e1c842d96cde01ebb6854b3e28fb998006575b2.tar.gz nextcloud-server-7e1c842d96cde01ebb6854b3e28fb998006575b2.zip |
Merge pull request #26923 from nextcloud/techdebt/noid/provapi-no-private-accountmanager
provisioning API to use only public API of IAccountManager
-rw-r--r-- | apps/provisioning_api/lib/Controller/AUserData.php | 54 | ||||
-rw-r--r-- | apps/provisioning_api/lib/Controller/GroupsController.php | 4 | ||||
-rw-r--r-- | apps/provisioning_api/lib/Controller/UsersController.php | 23 | ||||
-rw-r--r-- | apps/provisioning_api/tests/Controller/GroupsControllerTest.php | 20 | ||||
-rw-r--r-- | apps/provisioning_api/tests/Controller/UsersControllerTest.php | 67 | ||||
-rw-r--r-- | lib/private/Accounts/AccountProperty.php | 31 | ||||
-rw-r--r-- | lib/public/Accounts/IAccountProperty.php | 3 | ||||
-rw-r--r-- | tests/lib/Accounts/AccountManagerTest.php | 20 | ||||
-rw-r--r-- | tests/lib/Accounts/AccountPropertyTest.php | 12 |
9 files changed, 148 insertions, 86 deletions
diff --git a/apps/provisioning_api/lib/Controller/AUserData.php b/apps/provisioning_api/lib/Controller/AUserData.php index c26c4f9e2d0..6f682b41f28 100644 --- a/apps/provisioning_api/lib/Controller/AUserData.php +++ b/apps/provisioning_api/lib/Controller/AUserData.php @@ -31,12 +31,13 @@ declare(strict_types=1); namespace OCA\Provisioning_API\Controller; -use OC\Accounts\AccountManager; use OC\Group\Manager; use OC\User\Backend; use OC\User\NoUserException; use OC_Helper; use OCP\Accounts\IAccountManager; +use OCP\Accounts\PropertyDoesNotExistException; +use OCP\AppFramework\Http; use OCP\AppFramework\OCS\OCSException; use OCP\AppFramework\OCS\OCSNotFoundException; use OCP\AppFramework\OCSController; @@ -61,7 +62,7 @@ abstract class AUserData extends OCSController { protected $groupManager; /** @var IUserSession */ protected $userSession; - /** @var AccountManager */ + /** @var IAccountManager */ protected $accountManager; /** @var IFactory */ protected $l10nFactory; @@ -72,7 +73,7 @@ abstract class AUserData extends OCSController { IConfig $config, IGroupManager $groupManager, IUserSession $userSession, - AccountManager $accountManager, + IAccountManager $accountManager, IFactory $l10nFactory) { parent::__construct($appName, $request); @@ -140,30 +141,35 @@ abstract class AUserData extends OCSController { $data['subadmin'] = $this->getUserSubAdminGroupsData($targetUserObject->getUID()); $data['quota'] = $this->fillStorageInfo($targetUserObject->getUID()); - if ($includeScopes) { - $data[IAccountManager::PROPERTY_AVATAR . self::SCOPE_SUFFIX] = $userAccount->getProperty(IAccountManager::PROPERTY_AVATAR)->getScope(); - } - - $data[IAccountManager::PROPERTY_EMAIL] = $targetUserObject->getEMailAddress(); - if ($includeScopes) { - $data[IAccountManager::PROPERTY_EMAIL . self::SCOPE_SUFFIX] = $userAccount->getProperty(IAccountManager::PROPERTY_EMAIL)->getScope(); - } - $data[IAccountManager::PROPERTY_DISPLAYNAME] = $targetUserObject->getDisplayName(); - if ($includeScopes) { - $data[IAccountManager::PROPERTY_DISPLAYNAME . self::SCOPE_SUFFIX] = $userAccount->getProperty(IAccountManager::PROPERTY_DISPLAYNAME)->getScope(); - } + try { + if ($includeScopes) { + $data[IAccountManager::PROPERTY_AVATAR . self::SCOPE_SUFFIX] = $userAccount->getProperty(IAccountManager::PROPERTY_AVATAR)->getScope(); + } - foreach ([ - IAccountManager::PROPERTY_PHONE, - IAccountManager::PROPERTY_ADDRESS, - IAccountManager::PROPERTY_WEBSITE, - IAccountManager::PROPERTY_TWITTER, - ] as $propertyName) { - $property = $userAccount->getProperty($propertyName); - $data[$propertyName] = $property->getValue(); + $data[IAccountManager::PROPERTY_EMAIL] = $targetUserObject->getEMailAddress(); + if ($includeScopes) { + $data[IAccountManager::PROPERTY_EMAIL . self::SCOPE_SUFFIX] = $userAccount->getProperty(IAccountManager::PROPERTY_EMAIL)->getScope(); + } + $data[IAccountManager::PROPERTY_DISPLAYNAME] = $targetUserObject->getDisplayName(); if ($includeScopes) { - $data[$propertyName . self::SCOPE_SUFFIX] = $property->getScope(); + $data[IAccountManager::PROPERTY_DISPLAYNAME . self::SCOPE_SUFFIX] = $userAccount->getProperty(IAccountManager::PROPERTY_DISPLAYNAME)->getScope(); + } + + foreach ([ + IAccountManager::PROPERTY_PHONE, + IAccountManager::PROPERTY_ADDRESS, + IAccountManager::PROPERTY_WEBSITE, + IAccountManager::PROPERTY_TWITTER, + ] as $propertyName) { + $property = $userAccount->getProperty($propertyName); + $data[$propertyName] = $property->getValue(); + if ($includeScopes) { + $data[$propertyName . self::SCOPE_SUFFIX] = $property->getScope(); + } } + } catch (PropertyDoesNotExistException $e) { + // hard coded properties should exist + throw new OCSException($e->getMessage(), Http::STATUS_INTERNAL_SERVER_ERROR, $e); } $data['groups'] = $gids; diff --git a/apps/provisioning_api/lib/Controller/GroupsController.php b/apps/provisioning_api/lib/Controller/GroupsController.php index b031c405046..583af8454f6 100644 --- a/apps/provisioning_api/lib/Controller/GroupsController.php +++ b/apps/provisioning_api/lib/Controller/GroupsController.php @@ -34,7 +34,7 @@ declare(strict_types=1); namespace OCA\Provisioning_API\Controller; -use OC\Accounts\AccountManager; +use OCP\Accounts\IAccountManager; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\OCS\OCSException; use OCP\AppFramework\OCS\OCSForbiddenException; @@ -61,7 +61,7 @@ class GroupsController extends AUserData { IConfig $config, IGroupManager $groupManager, IUserSession $userSession, - AccountManager $accountManager, + IAccountManager $accountManager, IFactory $l10nFactory, LoggerInterface $logger) { parent::__construct($appName, diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index 115b955354b..f254620f330 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -46,7 +46,6 @@ use libphonenumber\NumberParseException; use libphonenumber\PhoneNumber; use libphonenumber\PhoneNumberFormat; use libphonenumber\PhoneNumberUtil; -use OC\Accounts\AccountManager; use OC\Authentication\Token\RemoteWipe; use OC\HintException; use OC\KnownUser\KnownUserService; @@ -102,7 +101,7 @@ class UsersController extends AUserData { IAppManager $appManager, IGroupManager $groupManager, IUserSession $userSession, - AccountManager $accountManager, + IAccountManager $accountManager, IURLGenerator $urlGenerator, LoggerInterface $logger, IFactory $l10nFactory, @@ -734,13 +733,14 @@ class UsersController extends AUserData { case IAccountManager::PROPERTY_ADDRESS: case IAccountManager::PROPERTY_WEBSITE: case IAccountManager::PROPERTY_TWITTER: - $userAccount = $this->accountManager->getUser($targetUser); - if ($userAccount[$key]['value'] !== $value) { - $userAccount[$key]['value'] = $value; + $userAccount = $this->accountManager->getAccount($targetUser); + $userProperty = $userAccount->getProperty($key); + if ($userProperty->getValue() !== $value) { try { - $this->accountManager->updateUser($targetUser, $userAccount, true); + $userProperty->setValue($value); + $this->accountManager->updateAccount($userAccount); - if ($key === IAccountManager::PROPERTY_PHONE) { + if ($userProperty->getName() === IAccountManager::PROPERTY_PHONE) { $this->knownUserService->deleteByContactUserId($targetUser->getUID()); } } catch (\InvalidArgumentException $e) { @@ -756,11 +756,12 @@ class UsersController extends AUserData { case IAccountManager::PROPERTY_TWITTER . self::SCOPE_SUFFIX: case IAccountManager::PROPERTY_AVATAR . self::SCOPE_SUFFIX: $propertyName = substr($key, 0, strlen($key) - strlen(self::SCOPE_SUFFIX)); - $userAccount = $this->accountManager->getUser($targetUser); - if ($userAccount[$propertyName]['scope'] !== $value) { - $userAccount[$propertyName]['scope'] = $value; + $userAccount = $this->accountManager->getAccount($targetUser); + $userProperty = $userAccount->getProperty($propertyName); + if ($userProperty->getScope() !== $value) { try { - $this->accountManager->updateUser($targetUser, $userAccount, true); + $userProperty->setScope($value); + $this->accountManager->updateAccount($userAccount); } catch (\InvalidArgumentException $e) { throw new OCSException('Invalid ' . $e->getMessage(), 102); } diff --git a/apps/provisioning_api/tests/Controller/GroupsControllerTest.php b/apps/provisioning_api/tests/Controller/GroupsControllerTest.php index 0624c9e0b75..b32b6e4d89b 100644 --- a/apps/provisioning_api/tests/Controller/GroupsControllerTest.php +++ b/apps/provisioning_api/tests/Controller/GroupsControllerTest.php @@ -30,7 +30,6 @@ namespace OCA\Provisioning_API\Tests\Controller; -use OC\Accounts\AccountManager; use OC\Group\Manager; use OC\SubAdmin; use OC\User\NoUserException; @@ -57,7 +56,7 @@ class GroupsControllerTest extends \Test\TestCase { protected $groupManager; /** @var IUserSession|\PHPUnit\Framework\MockObject\MockObject */ protected $userSession; - /** @var AccountManager|\PHPUnit\Framework\MockObject\MockObject */ + /** @var IAccountManager|\PHPUnit\Framework\MockObject\MockObject */ protected $accountManager; /** @var LoggerInterface|\PHPUnit\Framework\MockObject\MockObject */ protected $logger; @@ -76,7 +75,7 @@ class GroupsControllerTest extends \Test\TestCase { $this->config = $this->createMock(IConfig::class); $this->groupManager = $this->createMock(Manager::class); $this->userSession = $this->createMock(IUserSession::class); - $this->accountManager = $this->createMock(AccountManager::class); + $this->accountManager = $this->createMock(IAccountManager::class); $this->l10nFactory = $this->createMock(IFactory::class); $this->logger = $this->createMock(LoggerInterface::class); @@ -181,19 +180,6 @@ class GroupsControllerTest extends \Test\TestCase { }); } - private function useAccountManager() { - $this->accountManager->expects($this->any()) - ->method('getUser') - ->willReturnCallback(function (IUser $user) { - return [ - IAccountManager::PROPERTY_PHONE => ['value' => '0800-call-' . $user->getUID()], - IAccountManager::PROPERTY_ADDRESS => ['value' => 'Holzweg 99, 0601 Herrera, Panama'], - IAccountManager::PROPERTY_WEBSITE => ['value' => 'https://' . $user->getUid() . '.pa'], - IAccountManager::PROPERTY_TWITTER => ['value' => '@' . $user->getUID()], - ]; - }); - } - public function dataGetGroups() { return [ [null, 0, 0], @@ -505,7 +491,6 @@ class GroupsControllerTest extends \Test\TestCase { $gid = 'ncg1'; $this->asAdmin(); - $this->useAccountManager(); $users = [ 'ncu1' => $this->createUser('ncu1'), # regular @@ -551,7 +536,6 @@ class GroupsControllerTest extends \Test\TestCase { $gid = 'Department A/B C/D'; $this->asAdmin(); - $this->useAccountManager(); $users = [ 'ncu1' => $this->createUser('ncu1'), # regular diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php index 1afe9be4319..ed644f58cd8 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -41,7 +41,6 @@ namespace OCA\Provisioning_API\Tests\Controller; use Exception; -use OC\Accounts\AccountManager; use OC\Authentication\Token\RemoteWipe; use OC\Group\Manager; use OC\KnownUser\KnownUserService; @@ -88,7 +87,7 @@ class UsersControllerTest extends TestCase { protected $logger; /** @var UsersController|MockObject */ protected $api; - /** @var AccountManager|MockObject */ + /** @var IAccountManager|MockObject */ protected $accountManager; /** @var IURLGenerator|MockObject */ protected $urlGenerator; @@ -117,7 +116,7 @@ class UsersControllerTest extends TestCase { $this->userSession = $this->createMock(IUserSession::class); $this->logger = $this->createMock(LoggerInterface::class); $this->request = $this->createMock(IRequest::class); - $this->accountManager = $this->createMock(AccountManager::class); + $this->accountManager = $this->createMock(IAccountManager::class); $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->l10nFactory = $this->createMock(IFactory::class); $this->newUserMailHelper = $this->createMock(NewUserMailHelper::class); @@ -1574,13 +1573,34 @@ class UsersControllerTest extends TestCase { ->method('getBackend') ->willReturn($backend); - $this->accountManager->expects($this->once()) - ->method('getUser') + $propertyMock = $this->createMock(IAccountProperty::class); + $propertyMock->expects($this->any()) + ->method('getName') + ->willReturn($propertyName); + $propertyMock->expects($this->any()) + ->method('getValue') + ->willReturn($oldValue); + $propertyMock->expects($this->once()) + ->method('setValue') + ->with($newValue) + ->willReturnSelf(); + $propertyMock->expects($this->any()) + ->method('getScope') + ->willReturn(IAccountManager::SCOPE_LOCAL); + + $accountMock = $this->createMock(IAccount::class); + $accountMock->expects($this->any()) + ->method('getProperty') + ->with($propertyName) + ->willReturn($propertyMock); + + $this->accountManager->expects($this->atLeastOnce()) + ->method('getAccount') ->with($loggedInUser) - ->willReturn([$propertyName => ['value' => $oldValue, 'scope' => IAccountManager::SCOPE_LOCAL]]); + ->willReturn($accountMock); $this->accountManager->expects($this->once()) - ->method('updateUser') - ->with($loggedInUser, [$propertyName => ['value' => $newValue, 'scope' => IAccountManager::SCOPE_LOCAL]], true); + ->method('updateAccount') + ->with($accountMock); $this->assertEquals([], $this->api->editUser('UserToEdit', $propertyName, $newValue)->getData()); } @@ -1624,13 +1644,34 @@ class UsersControllerTest extends TestCase { ->method('getBackend') ->willReturn($backend); - $this->accountManager->expects($this->once()) - ->method('getUser') + $propertyMock = $this->createMock(IAccountProperty::class); + $propertyMock->expects($this->any()) + ->method('getName') + ->willReturn($propertyName); + $propertyMock->expects($this->any()) + ->method('getValue') + ->willReturn('somevalue'); + $propertyMock->expects($this->any()) + ->method('getScope') + ->willReturn($oldScope); + $propertyMock->expects($this->atLeastOnce()) + ->method('setScope') + ->with($newScope) + ->willReturnSelf(); + + $accountMock = $this->createMock(IAccount::class); + $accountMock->expects($this->any()) + ->method('getProperty') + ->with($propertyName) + ->willReturn($propertyMock); + + $this->accountManager->expects($this->atLeastOnce()) + ->method('getAccount') ->with($loggedInUser) - ->willReturn([$propertyName => ['value' => 'somevalue', 'scope' => $oldScope]]); + ->willReturn($accountMock); $this->accountManager->expects($this->once()) - ->method('updateUser') - ->with($loggedInUser, [$propertyName => ['value' => 'somevalue', 'scope' => $newScope]], true); + ->method('updateAccount') + ->with($accountMock); $this->assertEquals([], $this->api->editUser('UserToEdit', $propertyName . 'Scope', $newScope)->getData()); } diff --git a/lib/private/Accounts/AccountProperty.php b/lib/private/Accounts/AccountProperty.php index 850f39df9e3..0152137f6de 100644 --- a/lib/private/Accounts/AccountProperty.php +++ b/lib/private/Accounts/AccountProperty.php @@ -43,7 +43,7 @@ class AccountProperty implements IAccountProperty { public function __construct(string $name, string $value, string $scope, string $verified) { $this->name = $name; $this->value = $value; - $this->scope = $this->mapScopeToV2($scope); + $this->setScope($scope); $this->verified = $verified; } @@ -78,7 +78,16 @@ class AccountProperty implements IAccountProperty { * @return IAccountProperty */ public function setScope(string $scope): IAccountProperty { - $this->scope = $this->mapScopeToV2($scope); + $newScope = $this->mapScopeToV2($scope); + if (!in_array($newScope, [ + IAccountManager::SCOPE_LOCAL, + IAccountManager::SCOPE_FEDERATED, + IAccountManager::SCOPE_PRIVATE, + IAccountManager::SCOPE_PUBLISHED + ])) { + throw new \InvalidArgumentException('Invalid scope'); + } + $this->scope = $newScope; return $this; } @@ -128,21 +137,21 @@ class AccountProperty implements IAccountProperty { return $this->scope; } - public static function mapScopeToV2($scope) { + public static function mapScopeToV2(string $scope): string { if (strpos($scope, 'v2-') === 0) { return $scope; } switch ($scope) { - case IAccountManager::VISIBILITY_PRIVATE: - return IAccountManager::SCOPE_LOCAL; - case IAccountManager::VISIBILITY_CONTACTS_ONLY: - return IAccountManager::SCOPE_FEDERATED; - case IAccountManager::VISIBILITY_PUBLIC: - return IAccountManager::SCOPE_PUBLISHED; + case IAccountManager::VISIBILITY_PRIVATE: + return IAccountManager::SCOPE_LOCAL; + case IAccountManager::VISIBILITY_CONTACTS_ONLY: + return IAccountManager::SCOPE_FEDERATED; + case IAccountManager::VISIBILITY_PUBLIC: + return IAccountManager::SCOPE_PUBLISHED; + default: + return $scope; } - - return IAccountManager::SCOPE_LOCAL; } /** diff --git a/lib/public/Accounts/IAccountProperty.php b/lib/public/Accounts/IAccountProperty.php index 657121a27e8..1366ddd9543 100644 --- a/lib/public/Accounts/IAccountProperty.php +++ b/lib/public/Accounts/IAccountProperty.php @@ -26,6 +26,8 @@ declare(strict_types=1); namespace OCP\Accounts; +use InvalidArgumentException; + /** * Interface IAccountProperty * @@ -50,6 +52,7 @@ interface IAccountProperty extends \JsonSerializable { * * @param string $scope * @return IAccountProperty + * @throws InvalidArgumentException (since 22.0.0) */ public function setScope(string $scope): IAccountProperty; diff --git a/tests/lib/Accounts/AccountManagerTest.php b/tests/lib/Accounts/AccountManagerTest.php index 687ae29ff7b..1da128ae124 100644 --- a/tests/lib/Accounts/AccountManagerTest.php +++ b/tests/lib/Accounts/AccountManagerTest.php @@ -213,8 +213,8 @@ class AccountManagerTest extends TestCase { // SCOPE_PRIVATE is not allowed for display name and email IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'Display Name', 'scope' => IAccountManager::SCOPE_PRIVATE], IAccountManager::PROPERTY_EMAIL => ['value' => 'test@example.org', 'scope' => IAccountManager::SCOPE_PRIVATE], - IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => 'invalid'], - IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => ''], + IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => IAccountManager::SCOPE_LOCAL], + IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => IAccountManager::SCOPE_LOCAL], ], [ IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'Display Name', 'scope' => IAccountManager::SCOPE_LOCAL], @@ -222,9 +222,23 @@ class AccountManagerTest extends TestCase { IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => IAccountManager::SCOPE_LOCAL], IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => IAccountManager::SCOPE_LOCAL], ], - // don't throw but fall back false, false, ], + // illegal scope values + [ + [ + IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => IAccountManager::SCOPE_FEDERATED], + IAccountManager::PROPERTY_ADDRESS => ['value' => 'some street', 'scope' => IAccountManager::SCOPE_LOCAL], + IAccountManager::PROPERTY_WEBSITE => ['value' => 'https://example.org', 'scope' => IAccountManager::SCOPE_PRIVATE], + ], + [ + IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => ''], + IAccountManager::PROPERTY_ADDRESS => ['value' => 'some street', 'scope' => 'v2-invalid'], + IAccountManager::PROPERTY_WEBSITE => ['value' => 'https://example.org', 'scope' => 'invalid'], + ], + [], + true, true + ], // invalid or unsupported scope values throw an exception when passing $throwOnData=true [ [IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'Display Name', 'scope' => IAccountManager::SCOPE_PUBLISHED]], diff --git a/tests/lib/Accounts/AccountPropertyTest.php b/tests/lib/Accounts/AccountPropertyTest.php index f99abc21f83..84059bbc35d 100644 --- a/tests/lib/Accounts/AccountPropertyTest.php +++ b/tests/lib/Accounts/AccountPropertyTest.php @@ -80,16 +80,20 @@ class AccountPropertyTest extends TestCase { [IAccountManager::VISIBILITY_PRIVATE, IAccountManager::SCOPE_LOCAL], [IAccountManager::VISIBILITY_CONTACTS_ONLY, IAccountManager::SCOPE_FEDERATED], [IAccountManager::VISIBILITY_PUBLIC, IAccountManager::SCOPE_PUBLISHED], - // fallback - ['', IAccountManager::SCOPE_LOCAL], - ['unknown', IAccountManager::SCOPE_LOCAL], + // invalid values + ['', null], + ['unknown', null], + ['v2-unknown', null], ]; } /** * @dataProvider scopesProvider */ - public function testSetScopeMapping($storedScope, $returnedScope) { + public function testSetScopeMapping(string $storedScope, ?string $returnedScope) { + if ($returnedScope === null) { + $this->expectException(\InvalidArgumentException::class); + } $accountProperty = new AccountProperty( IAccountManager::PROPERTY_WEBSITE, 'https://example.com', |