From 847ed37afcc2b00f8dcf9ffc9f2f0671a3539bd4 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 20 May 2021 00:41:00 +0200 Subject: [PATCH] do not use private AccountManager in UsersController - extends IAccountProperty for verificationData getters and setters - implementation thereof ^ - and of course adaption of UsersController Signed-off-by: Arthur Schiwon --- .../lib/Controller/UsersController.php | 166 +++---- .../tests/Controller/UsersControllerTest.php | 404 +++++++++++------- lib/private/Accounts/Account.php | 4 +- lib/private/Accounts/AccountProperty.php | 17 +- lib/public/Accounts/IAccount.php | 5 +- lib/public/Accounts/IAccountProperty.php | 14 + tests/lib/Accounts/AccountManagerTest.php | 7 +- tests/lib/Accounts/AccountPropertyTest.php | 35 +- tests/lib/Accounts/AccountTest.php | 16 +- 9 files changed, 394 insertions(+), 274 deletions(-) diff --git a/apps/settings/lib/Controller/UsersController.php b/apps/settings/lib/Controller/UsersController.php index f4f364fe3e..3f8f05d422 100644 --- a/apps/settings/lib/Controller/UsersController.php +++ b/apps/settings/lib/Controller/UsersController.php @@ -37,7 +37,7 @@ declare(strict_types=1); namespace OCA\Settings\Controller; -use OC\Accounts\AccountManager; +use InvalidArgumentException; use OC\AppFramework\Http; use OC\Encryption\Exceptions\ModuleDoesNotExistsException; use OC\ForbiddenException; @@ -49,7 +49,9 @@ use OC\User\Manager as UserManager; use OCA\Settings\BackgroundJobs\VerifyUserData; use OCA\Settings\Events\BeforeTemplateRenderedEvent; use OCA\User_LDAP\User_Proxy; +use OCP\Accounts\IAccount; use OCP\Accounts\IAccountManager; +use OCP\Accounts\PropertyDoesNotExistException; use OCP\App\IAppManager; use OCP\AppFramework\Controller; use OCP\AppFramework\Http\DataResponse; @@ -88,7 +90,7 @@ class UsersController extends Controller { private $l10nFactory; /** @var IAppManager */ private $appManager; - /** @var AccountManager */ + /** @var IAccountManager */ private $accountManager; /** @var Manager */ private $keyManager; @@ -114,7 +116,7 @@ class UsersController extends Controller { IMailer $mailer, IFactory $l10nFactory, IAppManager $appManager, - AccountManager $accountManager, + IAccountManager $accountManager, Manager $keyManager, IJobList $jobList, IManager $encryptionManager, @@ -393,53 +395,36 @@ class UsersController extends Controller { ); } - $data = $this->accountManager->getUser($user); - $beforeData = $data; - if (!is_null($avatarScope)) { - $data[IAccountManager::PROPERTY_AVATAR]['scope'] = $avatarScope; - } - if ($this->config->getSystemValue('allow_user_to_change_display_name', true) !== false) { - if (!is_null($displayname)) { - $data[IAccountManager::PROPERTY_DISPLAYNAME]['value'] = $displayname; + $userAccount = $this->accountManager->getAccount($user); + $oldPhoneValue = $userAccount->getProperty(IAccountManager::PROPERTY_PHONE)->getValue(); + + $updatable = [ + IAccountManager::PROPERTY_AVATAR => ['value' => null, 'scope' => $avatarScope], + IAccountManager::PROPERTY_DISPLAYNAME => ['value' => $displayname, 'scope' => $displaynameScope], + IAccountManager::PROPERTY_EMAIL => ['value' => $email, 'scope' => $emailScope], + IAccountManager::PROPERTY_WEBSITE => ['value' => $website, 'scope' => $websiteScope], + IAccountManager::PROPERTY_ADDRESS => ['value' => $address, 'scope' => $addressScope], + IAccountManager::PROPERTY_PHONE => ['value' => $phone, 'scope' => $phoneScope], + IAccountManager::PROPERTY_TWITTER => ['value' => $twitter, 'scope' => $twitterScope], + ]; + $allowUserToChangeDisplayName = $this->config->getSystemValueBool('allow_user_to_change_display_name', true); + foreach ($updatable as $property => $data) { + if ($allowUserToChangeDisplayName === false + && in_array($property, [IAccountManager::PROPERTY_DISPLAYNAME, IAccountManager::PROPERTY_EMAIL], true)) { + continue; } - if (!is_null($displaynameScope)) { - $data[IAccountManager::PROPERTY_DISPLAYNAME]['scope'] = $displaynameScope; + $property = $userAccount->getProperty($property); + if (null !== $data['value']) { + $property->setValue($data['value']); } - if (!is_null($email)) { - $data[IAccountManager::PROPERTY_EMAIL]['value'] = $email; + if (null !== $data['scope']) { + $property->setScope($data['scope']); } - if (!is_null($emailScope)) { - $data[IAccountManager::PROPERTY_EMAIL]['scope'] = $emailScope; - } - } - if (!is_null($website)) { - $data[IAccountManager::PROPERTY_WEBSITE]['value'] = $website; - } - if (!is_null($websiteScope)) { - $data[IAccountManager::PROPERTY_WEBSITE]['scope'] = $websiteScope; - } - if (!is_null($address)) { - $data[IAccountManager::PROPERTY_ADDRESS]['value'] = $address; - } - if (!is_null($addressScope)) { - $data[IAccountManager::PROPERTY_ADDRESS]['scope'] = $addressScope; - } - if (!is_null($phone)) { - $data[IAccountManager::PROPERTY_PHONE]['value'] = $phone; - } - if (!is_null($phoneScope)) { - $data[IAccountManager::PROPERTY_PHONE]['scope'] = $phoneScope; - } - if (!is_null($twitter)) { - $data[IAccountManager::PROPERTY_TWITTER]['value'] = $twitter; - } - if (!is_null($twitterScope)) { - $data[IAccountManager::PROPERTY_TWITTER]['scope'] = $twitterScope; } try { - $data = $this->saveUserSettings($user, $data); - if ($beforeData[IAccountManager::PROPERTY_PHONE]['value'] !== $data[IAccountManager::PROPERTY_PHONE]['value']) { + $this->saveUserSettings($userAccount); + if ($oldPhoneValue !== $userAccount->getProperty(IAccountManager::PROPERTY_PHONE)->getValue()) { $this->knownUserService->deleteByContactUserId($user->getUID()); } return new DataResponse( @@ -447,32 +432,25 @@ class UsersController extends Controller { 'status' => 'success', 'data' => [ 'userId' => $user->getUID(), - 'avatarScope' => $data[IAccountManager::PROPERTY_AVATAR]['scope'], - 'displayname' => $data[IAccountManager::PROPERTY_DISPLAYNAME]['value'], - 'displaynameScope' => $data[IAccountManager::PROPERTY_DISPLAYNAME]['scope'], - 'phone' => $data[IAccountManager::PROPERTY_PHONE]['value'], - 'phoneScope' => $data[IAccountManager::PROPERTY_PHONE]['scope'], - 'email' => $data[IAccountManager::PROPERTY_EMAIL]['value'], - 'emailScope' => $data[IAccountManager::PROPERTY_EMAIL]['scope'], - 'website' => $data[IAccountManager::PROPERTY_WEBSITE]['value'], - 'websiteScope' => $data[IAccountManager::PROPERTY_WEBSITE]['scope'], - 'address' => $data[IAccountManager::PROPERTY_ADDRESS]['value'], - 'addressScope' => $data[IAccountManager::PROPERTY_ADDRESS]['scope'], - 'twitter' => $data[IAccountManager::PROPERTY_TWITTER]['value'], - 'twitterScope' => $data[IAccountManager::PROPERTY_TWITTER]['scope'], - 'message' => $this->l10n->t('Settings saved') - ] + 'avatarScope' => $userAccount->getProperty(IAccountManager::PROPERTY_AVATAR)->getScope(), + 'displayname' => $userAccount->getProperty(IAccountManager::PROPERTY_DISPLAYNAME)->getValue(), + 'displaynameScope' => $userAccount->getProperty(IAccountManager::PROPERTY_DISPLAYNAME)->getScope(), + 'phone' => $userAccount->getProperty(IAccountManager::PROPERTY_PHONE)->getValue(), + 'phoneScope' => $userAccount->getProperty(IAccountManager::PROPERTY_PHONE)->getScope(), + 'email' => $userAccount->getProperty(IAccountManager::PROPERTY_EMAIL)->getValue(), + 'emailScope' => $userAccount->getProperty(IAccountManager::PROPERTY_EMAIL)->getScope(), + 'website' => $userAccount->getProperty(IAccountManager::PROPERTY_WEBSITE)->getValue(), + 'websiteScope' => $userAccount->getProperty(IAccountManager::PROPERTY_WEBSITE)->getScope(), + 'address' => $userAccount->getProperty(IAccountManager::PROPERTY_ADDRESS)->getValue(), + 'addressScope' => $userAccount->getProperty(IAccountManager::PROPERTY_ADDRESS)->getScope(), + 'twitter' => $userAccount->getProperty(IAccountManager::PROPERTY_TWITTER)->getValue(), + 'twitterScope' => $userAccount->getProperty(IAccountManager::PROPERTY_TWITTER)->getScope(), + 'message' => $this->l10n->t('Settings saved'), + ], ], Http::STATUS_OK ); - } catch (ForbiddenException $e) { - return new DataResponse([ - 'status' => 'error', - 'data' => [ - 'message' => $e->getMessage() - ], - ]); - } catch (\InvalidArgumentException $e) { + } catch (ForbiddenException | InvalidArgumentException | PropertyDoesNotExistException $e) { return new DataResponse([ 'status' => 'error', 'data' => [ @@ -484,49 +462,41 @@ class UsersController extends Controller { /** * update account manager with new user data * - * @param IUser $user - * @param array $data - * @return array * @throws ForbiddenException - * @throws \InvalidArgumentException + * @throws InvalidArgumentException */ - protected function saveUserSettings(IUser $user, array $data): array { + protected function saveUserSettings(IAccount $userAccount): void { // keep the user back-end up-to-date with the latest display name and email // address - $oldDisplayName = $user->getDisplayName(); - $oldDisplayName = is_null($oldDisplayName) ? '' : $oldDisplayName; - if (isset($data[IAccountManager::PROPERTY_DISPLAYNAME]['value']) - && $oldDisplayName !== $data[IAccountManager::PROPERTY_DISPLAYNAME]['value'] - ) { - $result = $user->setDisplayName($data[IAccountManager::PROPERTY_DISPLAYNAME]['value']); + $oldDisplayName = $userAccount->getUser()->getDisplayName(); + if ($oldDisplayName !== $userAccount->getProperty(IAccountManager::PROPERTY_DISPLAYNAME)->getValue()) { + $result = $userAccount->getUser()->setDisplayName($userAccount->getProperty(IAccountManager::PROPERTY_DISPLAYNAME)->getValue()); if ($result === false) { throw new ForbiddenException($this->l10n->t('Unable to change full name')); } } - $oldEmailAddress = $user->getEMailAddress(); - $oldEmailAddress = is_null($oldEmailAddress) ? '' : strtolower($oldEmailAddress); - if (isset($data[IAccountManager::PROPERTY_EMAIL]['value']) - && $oldEmailAddress !== $data[IAccountManager::PROPERTY_EMAIL]['value'] - ) { + $oldEmailAddress = $userAccount->getUser()->getEMailAddress(); + $oldEmailAddress = strtolower((string)$oldEmailAddress); + if ($oldEmailAddress !== $userAccount->getProperty(IAccountManager::PROPERTY_EMAIL)->getValue()) { // this is the only permission a backend provides and is also used // for the permission of setting a email address - if (!$user->canChangeDisplayName()) { + if (!$userAccount->getUser()->canChangeDisplayName()) { throw new ForbiddenException($this->l10n->t('Unable to change email address')); } - $user->setEMailAddress($data[IAccountManager::PROPERTY_EMAIL]['value']); + $userAccount->getUser()->setEMailAddress($userAccount->getProperty(IAccountManager::PROPERTY_EMAIL)->getValue()); } try { - return $this->accountManager->updateUser($user, $data, true); - } catch (\InvalidArgumentException $e) { + $this->accountManager->updateAccount($userAccount); + } catch (InvalidArgumentException $e) { if ($e->getMessage() === IAccountManager::PROPERTY_PHONE) { - throw new \InvalidArgumentException($this->l10n->t('Unable to set invalid phone number')); + throw new InvalidArgumentException($this->l10n->t('Unable to set invalid phone number')); } if ($e->getMessage() === IAccountManager::PROPERTY_WEBSITE) { - throw new \InvalidArgumentException($this->l10n->t('Unable to set invalid website')); + throw new InvalidArgumentException($this->l10n->t('Unable to set invalid website')); } - throw new \InvalidArgumentException($this->l10n->t('Some account data was invalid')); + throw new InvalidArgumentException($this->l10n->t('Some account data was invalid')); } } @@ -548,7 +518,7 @@ class UsersController extends Controller { return new DataResponse([], Http::STATUS_BAD_REQUEST); } - $accountData = $this->accountManager->getUser($user); + $userAccount = $this->accountManager->getAccount($user); $cloudId = $user->getCloudId(); $message = 'Use my Federated Cloud ID to share with me: ' . $cloudId; $signature = $this->signMessage($user, $message); @@ -558,30 +528,30 @@ class UsersController extends Controller { switch ($account) { case 'verify-twitter': - $accountData[IAccountManager::PROPERTY_TWITTER]['verified'] = IAccountManager::VERIFICATION_IN_PROGRESS; $msg = $this->l10n->t('In order to verify your Twitter account, post the following tweet on Twitter (please make sure to post it without any line breaks):'); $code = $codeMd5; $type = IAccountManager::PROPERTY_TWITTER; - $accountData[IAccountManager::PROPERTY_TWITTER]['signature'] = $signature; break; case 'verify-website': - $accountData[IAccountManager::PROPERTY_WEBSITE]['verified'] = IAccountManager::VERIFICATION_IN_PROGRESS; $msg = $this->l10n->t('In order to verify your Website, store the following content in your web-root at \'.well-known/CloudIdVerificationCode.txt\' (please make sure that the complete text is in one line):'); $type = IAccountManager::PROPERTY_WEBSITE; - $accountData[IAccountManager::PROPERTY_WEBSITE]['signature'] = $signature; break; default: return new DataResponse([], Http::STATUS_BAD_REQUEST); } + $userProperty = $userAccount->getProperty($type); + $userProperty + ->setVerified(IAccountManager::VERIFICATION_IN_PROGRESS) + ->setVerificationData($signature); + if ($onlyVerificationCode === false) { - $accountData = $this->accountManager->updateUser($user, $accountData); - $data = $accountData[$type]['value']; + $this->accountManager->updateAccount($userAccount); $this->jobList->add(VerifyUserData::class, [ 'verificationCode' => $code, - 'data' => $data, + 'data' => $userProperty->getValue(), 'type' => $type, 'uid' => $user->getUID(), 'try' => 0, diff --git a/apps/settings/tests/Controller/UsersControllerTest.php b/apps/settings/tests/Controller/UsersControllerTest.php index c0dadd4c47..a163ef5b7e 100644 --- a/apps/settings/tests/Controller/UsersControllerTest.php +++ b/apps/settings/tests/Controller/UsersControllerTest.php @@ -31,10 +31,14 @@ namespace OCA\Settings\Tests\Controller; use OC\Accounts\AccountManager; use OC\Encryption\Exceptions\ModuleDoesNotExistsException; +use OC\ForbiddenException; use OC\Group\Manager; use OC\KnownUser\KnownUserService; use OCA\Settings\Controller\UsersController; +use OCP\Accounts\IAccount; use OCP\Accounts\IAccountManager; +use OCP\Accounts\IAccountProperty; +use OCP\Accounts\PropertyDoesNotExistException; use OCP\App\IAppManager; use OCP\AppFramework\Http; use OCP\BackgroundJob\IJobList; @@ -53,6 +57,7 @@ use OCP\IUserSession; use OCP\L10N\IFactory; use OCP\Mail\IMailer; use OCP\Security\ISecureRandom; +use PHPUnit\Framework\MockObject\MockObject; /** * @group DB @@ -180,49 +185,77 @@ class UsersControllerTest extends \Test\TestCase { } } - protected function getDefaultAccountManagerUserData() { - return [ - IAccountManager::PROPERTY_DISPLAYNAME => - [ - 'value' => 'Default display name', - 'scope' => IAccountManager::SCOPE_FEDERATED, - 'verified' => IAccountManager::VERIFIED, - ], - IAccountManager::PROPERTY_ADDRESS => - [ - 'value' => 'Default address', - 'scope' => IAccountManager::SCOPE_LOCAL, - 'verified' => IAccountManager::VERIFIED, - ], - IAccountManager::PROPERTY_WEBSITE => - [ - 'value' => 'Default website', - 'scope' => IAccountManager::SCOPE_LOCAL, - 'verified' => IAccountManager::VERIFIED, - ], - IAccountManager::PROPERTY_EMAIL => - [ - 'value' => 'Default email', - 'scope' => IAccountManager::SCOPE_FEDERATED, - 'verified' => IAccountManager::VERIFIED, - ], - IAccountManager::PROPERTY_AVATAR => - [ - 'scope' => IAccountManager::SCOPE_FEDERATED - ], - IAccountManager::PROPERTY_PHONE => - [ - 'value' => 'Default phone', - 'scope' => IAccountManager::SCOPE_LOCAL, - 'verified' => IAccountManager::VERIFIED, - ], - IAccountManager::PROPERTY_TWITTER => - [ - 'value' => 'Default twitter', - 'scope' => IAccountManager::SCOPE_LOCAL, - 'verified' => IAccountManager::VERIFIED, - ], + protected function buildPropertyMock(string $name, string $value, string $scope, string $verified = IAccountManager::VERIFIED): MockObject { + $property = $this->createMock(IAccountProperty::class); + $property->expects($this->any()) + ->method('getName') + ->willReturn($name); + $property->expects($this->any()) + ->method('getValue') + ->willReturn($value); + $property->expects($this->any()) + ->method('getScope') + ->willReturn($scope); + $property->expects($this->any()) + ->method('getVerified') + ->willReturn($verified); + + return $property; + } + + protected function getDefaultAccountMock(bool $useDefaultValues = true): MockObject { + $propertyMocks = [ + IAccountManager::PROPERTY_DISPLAYNAME => $this->buildPropertyMock( + IAccountManager::PROPERTY_DISPLAYNAME, + 'Default display name', + IAccountManager::SCOPE_FEDERATED, + ), + IAccountManager::PROPERTY_ADDRESS => $this->buildPropertyMock( + IAccountManager::PROPERTY_ADDRESS, + 'Default address', + IAccountManager::SCOPE_LOCAL, + ), + IAccountManager::PROPERTY_WEBSITE => $this->buildPropertyMock( + IAccountManager::PROPERTY_WEBSITE, + 'Default website', + IAccountManager::SCOPE_LOCAL, + ), + IAccountManager::PROPERTY_EMAIL => $this->buildPropertyMock( + IAccountManager::PROPERTY_EMAIL, + 'Default email', + IAccountManager::SCOPE_FEDERATED, + ), + IAccountManager::PROPERTY_AVATAR => $this->buildPropertyMock( + IAccountManager::PROPERTY_AVATAR, + '', + IAccountManager::SCOPE_FEDERATED, + ), + IAccountManager::PROPERTY_PHONE => $this->buildPropertyMock( + IAccountManager::PROPERTY_PHONE, + 'Default phone', + IAccountManager::SCOPE_LOCAL, + ), + IAccountManager::PROPERTY_TWITTER => $this->buildPropertyMock( + IAccountManager::PROPERTY_TWITTER, + 'Default twitter', + IAccountManager::SCOPE_LOCAL, + ), ]; + + $account = $this->createMock(IAccount::class); + $account->expects($this->any()) + ->method('getProperty') + ->willReturnCallback(function (string $propertyName) use ($propertyMocks) { + if (isset($propertyMocks[$propertyName])) { + return $propertyMocks[$propertyName]; + } + throw new PropertyDoesNotExistException($propertyName); + }); + $account->expects($this->any()) + ->method('getProperties') + ->willReturn($propertyMocks); + + return $account; } /** @@ -248,13 +281,12 @@ class UsersControllerTest extends \Test\TestCase { if ($saveData) { $this->accountManager->expects($this->once()) - ->method('getUser') + ->method('getAccount') ->with($user) - ->willReturn($this->getDefaultAccountManagerUserData()); + ->willReturn($this->getDefaultAccountMock()); $controller->expects($this->once()) - ->method('saveUserSettings') - ->willReturnArgument(1); + ->method('saveUserSettings'); } else { $controller->expects($this->never())->method('saveUserSettings'); } @@ -289,27 +321,6 @@ class UsersControllerTest extends \Test\TestCase { public function testSetUserSettingsWhenUserDisplayNameChangeNotAllowed() { $controller = $this->getController(false, ['saveUserSettings']); - $user = $this->createMock(IUser::class); - $user->method('getUID')->willReturn('johndoe'); - - $this->userSession->method('getUser')->willReturn($user); - - $defaultProperties = $this->getDefaultAccountManagerUserData(); - - $this->accountManager->expects($this->once()) - ->method('getUser') - ->with($user) - ->willReturn($defaultProperties); - - $this->config->expects($this->once()) - ->method('getSystemValue') - ->with('allow_user_to_change_display_name') - ->willReturn(false); - - $this->appManager->expects($this->any()) - ->method('isEnabledForUser') - ->with('federatedfilesharing') - ->willReturn(true); $avatarScope = IAccountManager::SCOPE_PUBLISHED; $displayName = 'Display name'; @@ -325,27 +336,67 @@ class UsersControllerTest extends \Test\TestCase { $twitter = '@nextclouders'; $twitterScope = IAccountManager::SCOPE_PUBLISHED; - // Display name and email are not changed. - $expectedProperties = $defaultProperties; - $expectedProperties[IAccountManager::PROPERTY_AVATAR]['scope'] = $avatarScope; - $expectedProperties[IAccountManager::PROPERTY_PHONE]['value'] = $phone; - $expectedProperties[IAccountManager::PROPERTY_PHONE]['scope'] = $phoneScope; - $expectedProperties[IAccountManager::PROPERTY_WEBSITE]['value'] = $website; - $expectedProperties[IAccountManager::PROPERTY_WEBSITE]['scope'] = $websiteScope; - $expectedProperties[IAccountManager::PROPERTY_ADDRESS]['value'] = $address; - $expectedProperties[IAccountManager::PROPERTY_ADDRESS]['scope'] = $addressScope; - $expectedProperties[IAccountManager::PROPERTY_TWITTER]['value'] = $twitter; - $expectedProperties[IAccountManager::PROPERTY_TWITTER]['scope'] = $twitterScope; + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('johndoe'); + + $this->userSession->method('getUser')->willReturn($user); + + /** @var MockObject|IAccount $userAccount */ + $userAccount = $this->getDefaultAccountMock(); + $this->accountManager->expects($this->once()) + ->method('getAccount') + ->with($user) + ->willReturn($userAccount); + + /** @var MockObject|IAccountProperty $avatarProperty */ + $avatarProperty = $userAccount->getProperty(IAccountManager::PROPERTY_AVATAR); + $avatarProperty->expects($this->atLeastOnce()) + ->method('setScope') + ->with($avatarScope) + ->willReturnSelf(); + + /** @var MockObject|IAccountProperty $avatarProperty */ + $avatarProperty = $userAccount->getProperty(IAccountManager::PROPERTY_ADDRESS); + $avatarProperty->expects($this->atLeastOnce()) + ->method('setScope') + ->with($addressScope) + ->willReturnSelf(); + $avatarProperty->expects($this->atLeastOnce()) + ->method('setValue') + ->with($address) + ->willReturnSelf(); + + /** @var MockObject|IAccountProperty $emailProperty */ + $emailProperty = $userAccount->getProperty(IAccountManager::PROPERTY_EMAIL); + $emailProperty->expects($this->never()) + ->method('setValue'); + $emailProperty->expects($this->never()) + ->method('setScope'); + + /** @var MockObject|IAccountProperty $emailProperty */ + $emailProperty = $userAccount->getProperty(IAccountManager::PROPERTY_DISPLAYNAME); + $emailProperty->expects($this->never()) + ->method('setValue'); + $emailProperty->expects($this->never()) + ->method('setScope'); + + $this->config->expects($this->once()) + ->method('getSystemValueBool') + ->with('allow_user_to_change_display_name') + ->willReturn(false); + + $this->appManager->expects($this->any()) + ->method('isEnabledForUser') + ->with('federatedfilesharing') + ->willReturn(true); $this->mailer->expects($this->once())->method('validateMailAddress') ->willReturn(true); $controller->expects($this->once()) - ->method('saveUserSettings') - ->with($user, $expectedProperties) - ->willReturnArgument(1); + ->method('saveUserSettings'); - $result = $controller->setUserSettings( + $controller->setUserSettings( $avatarScope, $displayName, $displayNameScope, @@ -369,12 +420,13 @@ class UsersControllerTest extends \Test\TestCase { $this->userSession->method('getUser')->willReturn($user); - $defaultProperties = $this->getDefaultAccountManagerUserData(); + $defaultProperties = []; //$this->getDefaultAccountMock(); + $userAccount = $this->getDefaultAccountMock(); $this->accountManager->expects($this->once()) - ->method('getUser') + ->method('getAccount') ->with($user) - ->willReturn($defaultProperties); + ->willReturn($userAccount); $this->appManager->expects($this->any()) ->method('isEnabledForUser') @@ -417,10 +469,9 @@ class UsersControllerTest extends \Test\TestCase { $controller->expects($this->once()) ->method('saveUserSettings') - ->with($user, $expectedProperties) - ->willReturnArgument(1); + ->with($userAccount); - $result = $controller->setUserSettings( + $controller->setUserSettings( $avatarScope, $displayName, $displayNameScope, @@ -450,12 +501,13 @@ class UsersControllerTest extends \Test\TestCase { $this->userSession->method('getUser')->willReturn($user); - $defaultProperties = $this->getDefaultAccountManagerUserData(); + /** @var IAccount|MockObject $userAccount */ + $userAccount = $this->getDefaultAccountMock(); $this->accountManager->expects($this->once()) - ->method('getUser') + ->method('getAccount') ->with($user) - ->willReturn($defaultProperties); + ->willReturn($userAccount); $avatarScope = ($property === 'avatarScope') ? $propertyValue : null; $displayName = ($property === 'displayName') ? $propertyValue : null; @@ -471,46 +523,43 @@ class UsersControllerTest extends \Test\TestCase { $twitter = ($property === 'twitter') ? $propertyValue : null; $twitterScope = ($property === 'twitterScope') ? $propertyValue : null; - $expectedProperties = $defaultProperties; - if ($property === 'avatarScope') { - $expectedProperties[IAccountManager::PROPERTY_AVATAR]['scope'] = $propertyValue; - } - if ($property === 'displayName') { - $expectedProperties[IAccountManager::PROPERTY_DISPLAYNAME]['value'] = $propertyValue; - } - if ($property === 'displayNameScope') { - $expectedProperties[IAccountManager::PROPERTY_DISPLAYNAME]['scope'] = $propertyValue; - } - if ($property === 'phone') { - $expectedProperties[IAccountManager::PROPERTY_PHONE]['value'] = $propertyValue; - } - if ($property === 'phoneScope') { - $expectedProperties[IAccountManager::PROPERTY_PHONE]['scope'] = $propertyValue; - } - if ($property === 'email') { - $expectedProperties[IAccountManager::PROPERTY_EMAIL]['value'] = $propertyValue; - } - if ($property === 'emailScope') { - $expectedProperties[IAccountManager::PROPERTY_EMAIL]['scope'] = $propertyValue; - } - if ($property === 'website') { - $expectedProperties[IAccountManager::PROPERTY_WEBSITE]['value'] = $propertyValue; - } - if ($property === 'websiteScope') { - $expectedProperties[IAccountManager::PROPERTY_WEBSITE]['scope'] = $propertyValue; - } - if ($property === 'address') { - $expectedProperties[IAccountManager::PROPERTY_ADDRESS]['value'] = $propertyValue; - } - if ($property === 'addressScope') { - $expectedProperties[IAccountManager::PROPERTY_ADDRESS]['scope'] = $propertyValue; - } - if ($property === 'twitter') { - $expectedProperties[IAccountManager::PROPERTY_TWITTER]['value'] = $propertyValue; - } - if ($property === 'twitterScope') { - $expectedProperties[IAccountManager::PROPERTY_TWITTER]['scope'] = $propertyValue; + /** @var IAccountProperty[]|MockObject[] $expectedProperties */ + $expectedProperties = $userAccount->getProperties(); + $isScope = strrpos($property, 'Scope') === strlen($property) - strlen(5); + switch ($property) { + case 'avatarScope': + $propertyId = IAccountManager::PROPERTY_AVATAR; + break; + case 'displayName': + case 'displayNameScope': + $propertyId = IAccountManager::PROPERTY_DISPLAYNAME; + break; + case 'phone': + case 'phoneScope': + $propertyId = IAccountManager::PROPERTY_PHONE; + break; + case 'email': + case 'emailScope': + $propertyId = IAccountManager::PROPERTY_EMAIL; + break; + case 'website': + case 'websiteScope': + $propertyId = IAccountManager::PROPERTY_WEBSITE; + break; + case 'address': + case 'addressScope': + $propertyId = IAccountManager::PROPERTY_ADDRESS; + break; + case 'twitter': + case 'twitterScope': + $propertyId = IAccountManager::PROPERTY_TWITTER; + break; + default: + $propertyId = '404'; } + $expectedProperties[$propertyId]->expects($this->any()) + ->method($isScope ? 'getScope' : 'getValue') + ->willReturn($propertyValue); if (!empty($email)) { $this->mailer->expects($this->once())->method('validateMailAddress') @@ -519,10 +568,9 @@ class UsersControllerTest extends \Test\TestCase { $controller->expects($this->once()) ->method('saveUserSettings') - ->with($user, $expectedProperties) - ->willReturnArgument(1); + ->with($userAccount); - $result = $controller->setUserSettings( + $controller->setUserSettings( $avatarScope, $displayName, $displayNameScope, @@ -593,10 +641,33 @@ class UsersControllerTest extends \Test\TestCase { ->willReturn(true); } - $this->accountManager->expects($this->once())->method('updateUser') - ->with($user, $data); + $properties = []; + foreach ($data as $propertyName => $propertyData) { + $properties[$propertyName] = $this->createMock(IAccountProperty::class); + $properties[$propertyName]->expects($this->any()) + ->method('getValue') + ->willReturn($propertyData['value']); + } - $this->invokePrivate($controller, 'saveUserSettings', [$user, $data]); + $account = $this->createMock(IAccount::class); + $account->expects($this->any()) + ->method('getUser') + ->willReturn($user); + $account->expects($this->any()) + ->method('getProperty') + ->willReturnCallback(function (string $propertyName) use ($properties) { + return $properties[$propertyName]; + }); + + $this->accountManager->expects($this->any()) + ->method('getAccount') + ->willReturn($account); + + $this->accountManager->expects($this->once()) + ->method('updateAccount') + ->with($account); + + $this->invokePrivate($controller, 'saveUserSettings', [$account]); } public function dataTestSaveUserSettings() { @@ -655,21 +726,15 @@ class UsersControllerTest extends \Test\TestCase { /** * @dataProvider dataTestSaveUserSettingsException - * - * @param array $data - * @param string $oldEmailAddress - * @param string $oldDisplayName - * @param bool $setDisplayNameResult - * @param bool $canChangeEmail - * */ - public function testSaveUserSettingsException($data, - $oldEmailAddress, - $oldDisplayName, - $setDisplayNameResult, - $canChangeEmail + public function testSaveUserSettingsException( + array $data, + string $oldEmailAddress, + string $oldDisplayName, + bool $setDisplayNameResult, + bool $canChangeEmail ) { - $this->expectException(\OC\ForbiddenException::class); + $this->expectException(ForbiddenException::class); $controller = $this->getController(); $user = $this->createMock(IUser::class); @@ -677,6 +742,22 @@ class UsersControllerTest extends \Test\TestCase { $user->method('getDisplayName')->willReturn($oldDisplayName); $user->method('getEMailAddress')->willReturn($oldEmailAddress); + /** @var MockObject|IAccount $userAccount */ + $userAccount = $this->createMock(IAccount::class); + $userAccount->expects($this->any()) + ->method('getUser') + ->willReturn($user); + $propertyMocks = []; + foreach ($data as $propertyName => $propertyData) { + /** @var MockObject|IAccountProperty $property */ + $propertyMocks[$propertyName] = $this->buildPropertyMock($propertyName, $propertyData['value'], ''); + } + $userAccount->expects($this->any()) + ->method('getProperty') + ->willReturnCallback(function (string $propertyName) use ($propertyMocks) { + return $propertyMocks[$propertyName]; + }); + if ($data[IAccountManager::PROPERTY_EMAIL]['value'] !== $oldEmailAddress) { $user->method('canChangeDisplayName') ->willReturn($canChangeEmail); @@ -688,7 +769,7 @@ class UsersControllerTest extends \Test\TestCase { ->willReturn($setDisplayNameResult); } - $this->invokePrivate($controller, 'saveUserSettings', [$user, $data]); + $this->invokePrivate($controller, 'saveUserSettings', [$userAccount]); } @@ -748,15 +829,34 @@ class UsersControllerTest extends \Test\TestCase { $controller = $this->getController(false, ['signMessage', 'getCurrentTime']); $user = $this->createMock(IUser::class); + + $property = $this->buildPropertyMock($type, $dataBefore[$type]['value'], '', IAccountManager::NOT_VERIFIED); + $property->expects($this->atLeastOnce()) + ->method('setVerified') + ->with(IAccountManager::VERIFICATION_IN_PROGRESS) + ->willReturnSelf(); + $property->expects($this->atLeastOnce()) + ->method('setVerificationData') + ->with($signature) + ->willReturnSelf(); + + $userAccount = $this->createMock(IAccount::class); + $userAccount->expects($this->any()) + ->method('getUser') + ->willReturn($user); + $userAccount->expects($this->any()) + ->method('getProperty') + ->willReturn($property); + $this->userSession->expects($this->once())->method('getUser')->willReturn($user); - $this->accountManager->expects($this->once())->method('getUser')->with($user)->willReturn($dataBefore); + $this->accountManager->expects($this->once())->method('getAccount')->with($user)->willReturn($userAccount); $user->expects($this->any())->method('getCloudId')->willReturn('user@nextcloud.com'); $user->expects($this->any())->method('getUID')->willReturn('uid'); $controller->expects($this->once())->method('signMessage')->with($user, $message)->willReturn($signature); $controller->expects($this->any())->method('getCurrentTime')->willReturn(1234567); if ($onlyVerificationCode === false) { - $this->accountManager->expects($this->once())->method('updateUser')->with($user, $expectedData)->willReturnArgument(1); + $this->accountManager->expects($this->once())->method('updateAccount')->with($userAccount)->willReturnArgument(1); $this->jobList->expects($this->once())->method('add') ->with('OCA\Settings\BackgroundJobs\VerifyUserData', [ diff --git a/lib/private/Accounts/Account.php b/lib/private/Accounts/Account.php index e7aeb6fa1e..109691641f 100644 --- a/lib/private/Accounts/Account.php +++ b/lib/private/Accounts/Account.php @@ -44,8 +44,8 @@ class Account implements IAccount { $this->user = $user; } - public function setProperty(string $property, string $value, string $scope, string $verified): IAccount { - $this->properties[$property] = new AccountProperty($property, $value, $scope, $verified); + public function setProperty(string $property, string $value, string $scope, string $verified, string $verificationData = ''): IAccount { + $this->properties[$property] = new AccountProperty($property, $value, $scope, $verified, $verificationData); return $this; } diff --git a/lib/private/Accounts/AccountProperty.php b/lib/private/Accounts/AccountProperty.php index 0152137f6d..cb60a7c694 100644 --- a/lib/private/Accounts/AccountProperty.php +++ b/lib/private/Accounts/AccountProperty.php @@ -39,12 +39,15 @@ class AccountProperty implements IAccountProperty { private $scope; /** @var string */ private $verified; + /** @var string */ + private $verificationData; - public function __construct(string $name, string $value, string $scope, string $verified) { + public function __construct(string $name, string $value, string $scope, string $verified, string $verificationData) { $this->name = $name; $this->value = $value; $this->setScope($scope); $this->verified = $verified; + $this->verificationData = $verificationData; } public function jsonSerialize() { @@ -52,7 +55,8 @@ class AccountProperty implements IAccountProperty { 'name' => $this->getName(), 'value' => $this->getValue(), 'scope' => $this->getScope(), - 'verified' => $this->getVerified() + 'verified' => $this->getVerified(), + 'verificationData' => $this->getVerificationData(), ]; } @@ -164,4 +168,13 @@ class AccountProperty implements IAccountProperty { public function getVerified(): string { return $this->verified; } + + public function setVerificationData(string $verificationData): IAccountProperty { + $this->verificationData = $verificationData; + return $this; + } + + public function getVerificationData(): string { + return $this->verificationData; + } } diff --git a/lib/public/Accounts/IAccount.php b/lib/public/Accounts/IAccount.php index 3f8c86cbc1..7397eb4fea 100644 --- a/lib/public/Accounts/IAccount.php +++ b/lib/public/Accounts/IAccount.php @@ -44,9 +44,10 @@ interface IAccount extends \JsonSerializable { * @param string $value * @param string $scope Must be one of the VISIBILITY_ prefixed constants of \OCP\Accounts\IAccountManager * @param string $verified \OCP\Accounts\IAccountManager::NOT_VERIFIED | \OCP\Accounts\IAccountManager::VERIFICATION_IN_PROGRESS | \OCP\Accounts\IAccountManager::VERIFIED + * @param string $verificationData Optional, defaults to empty string. Since @22.0.0. * @return IAccount */ - public function setProperty(string $property, string $value, string $scope, string $verified): IAccount; + public function setProperty(string $property, string $value, string $scope, string $verified, string $verificationData = ''): IAccount; /** * Get a property by its key @@ -60,7 +61,7 @@ interface IAccount extends \JsonSerializable { public function getProperty(string $property): IAccountProperty; /** - * Get all properties of an account + * Get all properties of an account. Array indices are property names. * * @since 15.0.0 * diff --git a/lib/public/Accounts/IAccountProperty.php b/lib/public/Accounts/IAccountProperty.php index 1366ddd954..dcc81f02ca 100644 --- a/lib/public/Accounts/IAccountProperty.php +++ b/lib/public/Accounts/IAccountProperty.php @@ -101,4 +101,18 @@ interface IAccountProperty extends \JsonSerializable { * @return string */ public function getVerified(): string; + + /** + * Sets data for verification purposes. + * + * @since 22.0.0 + */ + public function setVerificationData(string $verificationData): IAccountProperty; + + /** + * Retrieves data for verification purposes. + * + * @since 22.0.0 + */ + public function getVerificationData(): string; } diff --git a/tests/lib/Accounts/AccountManagerTest.php b/tests/lib/Accounts/AccountManagerTest.php index 1da128ae12..9bce92716a 100644 --- a/tests/lib/Accounts/AccountManagerTest.php +++ b/tests/lib/Accounts/AccountManagerTest.php @@ -106,6 +106,7 @@ class AccountManagerTest extends TestCase { /** @var IUser $user */ $user = $this->createMock(IUser::class); + // FIXME: should be an integration test instead of this abomination $accountManager->expects($this->once())->method('getUser')->with($user)->willReturn($oldData); if ($updateExisting) { @@ -147,9 +148,9 @@ class AccountManagerTest extends TestCase { public function dataTrueFalse() { return [ - [['newData'], ['oldData'], false, true], - [['newData'], [], true, false], - [['oldData'], ['oldData'], false, false] + [['myProperty' => ['value' => 'newData']], ['myProperty' => ['value' => 'oldData']], false, true], + [['myProperty' => ['value' => 'newData']], [], true, false], + [['myProperty' => ['value' => 'oldData']], ['myProperty' => ['value' => 'oldData']], false, false] ]; } diff --git a/tests/lib/Accounts/AccountPropertyTest.php b/tests/lib/Accounts/AccountPropertyTest.php index 84059bbc35..50c3b8f84a 100644 --- a/tests/lib/Accounts/AccountPropertyTest.php +++ b/tests/lib/Accounts/AccountPropertyTest.php @@ -38,7 +38,8 @@ class AccountPropertyTest extends TestCase { IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, - IAccountManager::VERIFIED + IAccountManager::VERIFIED, + '' ); $this->assertEquals(IAccountManager::PROPERTY_WEBSITE, $accountProperty->getName()); $this->assertEquals('https://example.com', $accountProperty->getValue()); @@ -51,7 +52,8 @@ class AccountPropertyTest extends TestCase { IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, - IAccountManager::VERIFIED + IAccountManager::VERIFIED, + '' ); $actualReturn = $accountProperty->setValue('https://example.org'); $this->assertEquals('https://example.org', $accountProperty->getValue()); @@ -63,7 +65,8 @@ class AccountPropertyTest extends TestCase { IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, - IAccountManager::VERIFIED + IAccountManager::VERIFIED, + '' ); $actualReturn = $accountProperty->setScope(IAccountManager::SCOPE_LOCAL); $this->assertEquals(IAccountManager::SCOPE_LOCAL, $accountProperty->getScope()); @@ -98,7 +101,8 @@ class AccountPropertyTest extends TestCase { IAccountManager::PROPERTY_WEBSITE, 'https://example.com', $storedScope, - IAccountManager::VERIFIED + IAccountManager::VERIFIED, + '' ); $this->assertEquals($returnedScope, $accountProperty->getScope()); } @@ -108,25 +112,42 @@ class AccountPropertyTest extends TestCase { IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, - IAccountManager::VERIFIED + IAccountManager::VERIFIED, + '' ); $actualReturn = $accountProperty->setVerified(IAccountManager::NOT_VERIFIED); $this->assertEquals(IAccountManager::NOT_VERIFIED, $accountProperty->getVerified()); $this->assertEquals(IAccountManager::NOT_VERIFIED, $actualReturn->getVerified()); } + public function testSetVerificationData() { + $accountProperty = new AccountProperty( + IAccountManager::PROPERTY_WEBSITE, + 'https://example.com', + IAccountManager::SCOPE_PUBLISHED, + IAccountManager::VERIFIED, + '' + ); + $token = uniqid(); + $actualReturn = $accountProperty->setVerificationData($token); + $this->assertEquals($token, $accountProperty->getVerificationData()); + $this->assertEquals($token, $actualReturn->getVerificationData()); + } + public function testJsonSerialize() { $accountProperty = new AccountProperty( IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, - IAccountManager::VERIFIED + IAccountManager::VERIFIED, + '60a7a633b74af', ); $this->assertEquals([ 'name' => IAccountManager::PROPERTY_WEBSITE, 'value' => 'https://example.com', 'scope' => IAccountManager::SCOPE_PUBLISHED, - 'verified' => IAccountManager::VERIFIED + 'verified' => IAccountManager::VERIFIED, + 'verificationData' => '60a7a633b74af' ], $accountProperty->jsonSerialize()); } } diff --git a/tests/lib/Accounts/AccountTest.php b/tests/lib/Accounts/AccountTest.php index 8afcc44afd..9c2a5333d2 100644 --- a/tests/lib/Accounts/AccountTest.php +++ b/tests/lib/Accounts/AccountTest.php @@ -43,7 +43,7 @@ class AccountTest extends TestCase { public function testSetProperty() { $user = $this->createMock(IUser::class); - $property = new AccountProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED); + $property = new AccountProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED, ''); $account = new Account($user); $account->setProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED); $this->assertEquals($property, $account->getProperty(IAccountManager::PROPERTY_WEBSITE)); @@ -52,8 +52,8 @@ class AccountTest extends TestCase { public function testGetProperties() { $user = $this->createMock(IUser::class); $properties = [ - IAccountManager::PROPERTY_WEBSITE => new AccountProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED), - IAccountManager::PROPERTY_EMAIL => new AccountProperty(IAccountManager::PROPERTY_EMAIL, 'user@example.com', IAccountManager::SCOPE_LOCAL, IAccountManager::VERIFIED) + IAccountManager::PROPERTY_WEBSITE => new AccountProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED, ''), + IAccountManager::PROPERTY_EMAIL => new AccountProperty(IAccountManager::PROPERTY_EMAIL, 'user@example.com', IAccountManager::SCOPE_LOCAL, IAccountManager::VERIFIED, '') ]; $account = new Account($user); $account->setProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED); @@ -65,9 +65,9 @@ class AccountTest extends TestCase { public function testGetFilteredProperties() { $user = $this->createMock(IUser::class); $properties = [ - IAccountManager::PROPERTY_WEBSITE => new AccountProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED), - IAccountManager::PROPERTY_EMAIL => new AccountProperty(IAccountManager::PROPERTY_EMAIL, 'user@example.com', IAccountManager::SCOPE_LOCAL, IAccountManager::VERIFIED), - IAccountManager::PROPERTY_PHONE => new AccountProperty(IAccountManager::PROPERTY_PHONE, '123456', IAccountManager::SCOPE_PUBLISHED, IAccountManager::VERIFIED), + IAccountManager::PROPERTY_WEBSITE => new AccountProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED, ''), + IAccountManager::PROPERTY_EMAIL => new AccountProperty(IAccountManager::PROPERTY_EMAIL, 'user@example.com', IAccountManager::SCOPE_LOCAL, IAccountManager::VERIFIED, ''), + IAccountManager::PROPERTY_PHONE => new AccountProperty(IAccountManager::PROPERTY_PHONE, '123456', IAccountManager::SCOPE_PUBLISHED, IAccountManager::VERIFIED, ''), ]; $account = new Account($user); $account->setProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED); @@ -98,8 +98,8 @@ class AccountTest extends TestCase { public function testJsonSerialize() { $user = $this->createMock(IUser::class); $properties = [ - IAccountManager::PROPERTY_WEBSITE => new AccountProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED), - IAccountManager::PROPERTY_EMAIL => new AccountProperty(IAccountManager::PROPERTY_EMAIL, 'user@example.com', IAccountManager::SCOPE_LOCAL, IAccountManager::VERIFIED) + IAccountManager::PROPERTY_WEBSITE => new AccountProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED, ''), + IAccountManager::PROPERTY_EMAIL => new AccountProperty(IAccountManager::PROPERTY_EMAIL, 'user@example.com', IAccountManager::SCOPE_LOCAL, IAccountManager::VERIFIED, '') ]; $account = new Account($user); $account->setProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED);