Merge pull request #26923 from nextcloud/techdebt/noid/provapi-no-private-accountmanager

provisioning API to use only public API of IAccountManager
2021-05-12 14:17:50 +02:00 · 2021-05-12 14:17:50 +02:00 · 7e1c842d96
parent 6c741724fb 8413ed9475
commit 7e1c842d96
9 changed files with 149 additions and 87 deletions
--- 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();
-		}
-
-		foreach ([
-			IAccountManager::PROPERTY_PHONE,
-			IAccountManager::PROPERTY_ADDRESS,
-			IAccountManager::PROPERTY_WEBSITE,
-			IAccountManager::PROPERTY_TWITTER,
-		] as $propertyName) {
-			$property = $userAccount->getProperty($propertyName);
-			$data[$propertyName] = $property->getValue();
+		try {
 			if ($includeScopes) {
-				$data[$propertyName . self::SCOPE_SUFFIX] = $property->getScope();
+				$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();
+			}
+
+			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;
--- 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,
--- 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);
 					}
--- 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
--- 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());
 	}
--- 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;
 	}

 	/**
--- 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;

--- 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]],
--- 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',