From 3d6ffd3da223b035557d595b4e99ea4f38f60787 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Tue, 22 Jun 2021 12:56:00 +0200 Subject: [PATCH] adjust verification state updater method - also fixes scope of internal methods Signed-off-by: Arthur Schiwon --- lib/private/Accounts/AccountManager.php | 103 +++++++--------------- tests/lib/Accounts/AccountManagerTest.php | 15 ++-- 2 files changed, 38 insertions(+), 80 deletions(-) diff --git a/lib/private/Accounts/AccountManager.php b/lib/private/Accounts/AccountManager.php index 89d4ac965ae..ffa3e2a9a75 100644 --- a/lib/private/Accounts/AccountManager.php +++ b/lib/private/Accounts/AccountManager.php @@ -223,24 +223,12 @@ class AccountManager implements IAccountManager { } } - /** - * update user record - * - * @param IUser $user - * @param array $data - * @param bool $throwOnData Set to true if you can inform the user about invalid data - * @return array The potentially modified data (e.g. phone numbers are converted to E.164 format) - * @throws InvalidArgumentException Message is the property that was invalid - */ - public function updateUser(IUser $user, array $data, bool $throwOnData = false): array { - $userData = $this->getUser($user); + protected function updateUser(IUser $user, array $data, bool $throwOnData = false): array { + $userData = $this->getUser($user, false); $updated = true; - if (empty($userData)) { - $this->insertNewUser($user, $data); - } elseif ($userData !== $data) { + if ($userData !== $data) { $data = $this->checkEmailVerification($userData, $data, $user); - $data = $this->updateVerifyStatus($userData, $data); $this->updateExistingUser($user, $data); } else { // nothing needs to be done if new and old data set are the same @@ -287,10 +275,8 @@ class AccountManager implements IAccountManager { /** * get stored data from a given user - * - * @deprecated use getAccount instead to make sure migrated properties work correctly */ - public function getUser(IUser $user, bool $insertIfNotExists = true): array { + protected function getUser(IUser $user, bool $insertIfNotExists = true): array { $uid = $user->getUID(); $query = $this->connection->getQueryBuilder(); $query->select('data') @@ -362,6 +348,7 @@ class AccountManager implements IAccountManager { */ protected function checkEmailVerification($oldData, $newData, IUser $user): array { if ($oldData[self::PROPERTY_EMAIL]['value'] !== $newData[self::PROPERTY_EMAIL]['value']) { + $this->jobList->add(VerifyUserData::class, [ 'verificationCode' => '', @@ -373,6 +360,7 @@ class AccountManager implements IAccountManager { ] ); $newData[self::PROPERTY_EMAIL]['verified'] = self::VERIFICATION_IN_PROGRESS; + } return $newData; @@ -404,59 +392,31 @@ class AccountManager implements IAccountManager { return $userData; } - /** - * reset verification status if personal data changed - * - * @param array $oldData - * @param array $newData - * @return array - */ - protected function updateVerifyStatus(array $oldData, array $newData): array { - - // which account was already verified successfully? - $twitterVerified = isset($oldData[self::PROPERTY_TWITTER]['verified']) && $oldData[self::PROPERTY_TWITTER]['verified'] === self::VERIFIED; - $websiteVerified = isset($oldData[self::PROPERTY_WEBSITE]['verified']) && $oldData[self::PROPERTY_WEBSITE]['verified'] === self::VERIFIED; - $emailVerified = isset($oldData[self::PROPERTY_EMAIL]['verified']) && $oldData[self::PROPERTY_EMAIL]['verified'] === self::VERIFIED; - - // keep old verification status if we don't have a new one - if (!isset($newData[self::PROPERTY_TWITTER]['verified'])) { - // keep old verification status if value didn't changed and an old value exists - $keepOldStatus = $newData[self::PROPERTY_TWITTER]['value'] === $oldData[self::PROPERTY_TWITTER]['value'] && isset($oldData[self::PROPERTY_TWITTER]['verified']); - $newData[self::PROPERTY_TWITTER]['verified'] = $keepOldStatus ? $oldData[self::PROPERTY_TWITTER]['verified'] : self::NOT_VERIFIED; - } - - if (!isset($newData[self::PROPERTY_WEBSITE]['verified'])) { - // keep old verification status if value didn't changed and an old value exists - $keepOldStatus = $newData[self::PROPERTY_WEBSITE]['value'] === $oldData[self::PROPERTY_WEBSITE]['value'] && isset($oldData[self::PROPERTY_WEBSITE]['verified']); - $newData[self::PROPERTY_WEBSITE]['verified'] = $keepOldStatus ? $oldData[self::PROPERTY_WEBSITE]['verified'] : self::NOT_VERIFIED; - } - - if (!isset($newData[self::PROPERTY_EMAIL]['verified'])) { - // keep old verification status if value didn't changed and an old value exists - $keepOldStatus = $newData[self::PROPERTY_EMAIL]['value'] === $oldData[self::PROPERTY_EMAIL]['value'] && isset($oldData[self::PROPERTY_EMAIL]['verified']); - $newData[self::PROPERTY_EMAIL]['verified'] = $keepOldStatus ? $oldData[self::PROPERTY_EMAIL]['verified'] : self::VERIFICATION_IN_PROGRESS; - } - - // reset verification status if a value from a previously verified data was changed - if ($twitterVerified && - $oldData[self::PROPERTY_TWITTER]['value'] !== $newData[self::PROPERTY_TWITTER]['value'] - ) { - $newData[self::PROPERTY_TWITTER]['verified'] = self::NOT_VERIFIED; - } - - if ($websiteVerified && - $oldData[self::PROPERTY_WEBSITE]['value'] !== $newData[self::PROPERTY_WEBSITE]['value'] - ) { - $newData[self::PROPERTY_WEBSITE]['verified'] = self::NOT_VERIFIED; - } + protected function updateVerificationStatus(IAccount $updatedAccount, array $oldData): void { + static $propertiesVerifiableByLookupServer = [ + self::PROPERTY_TWITTER, + self::PROPERTY_WEBSITE, + self::PROPERTY_EMAIL, + ]; - if ($emailVerified && - $oldData[self::PROPERTY_EMAIL]['value'] !== $newData[self::PROPERTY_EMAIL]['value'] - ) { - $newData[self::PROPERTY_EMAIL]['verified'] = self::NOT_VERIFIED; + foreach ($propertiesVerifiableByLookupServer as $propertyName) { + try { + $property = $updatedAccount->getProperty($propertyName); + } catch (PropertyDoesNotExistException $e) { + continue; + } + $wasVerified = isset($oldData[$propertyName]) + && isset($oldData[$propertyName]['verified']) + && $oldData[$propertyName]['verified'] === self::VERIFIED; + if ((!isset($oldData[$propertyName]) + || !isset($oldData[$propertyName]['value']) + || $property->getValue() !== $oldData[$propertyName]['value']) + && ($property->getVerified() !== self::NOT_VERIFIED + || $wasVerified) + ) { + $property->setVerified(self::NOT_VERIFIED); + } } - - return $newData; } protected function dataArrayToJson(array $accountData): string { @@ -679,6 +639,9 @@ class AccountManager implements IAccountManager { $this->testPropertyScope($property, $allowedScopes, true); } + $this->updateVerificationStatus($account, $this->getUser($account->getUser(), false)); + + $this->updateUser($account->getUser(), $data, true); } -} +} \ No newline at end of file diff --git a/tests/lib/Accounts/AccountManagerTest.php b/tests/lib/Accounts/AccountManagerTest.php index d9a55825122..d70d453c248 100644 --- a/tests/lib/Accounts/AccountManagerTest.php +++ b/tests/lib/Accounts/AccountManagerTest.php @@ -246,7 +246,7 @@ class AccountManagerTest extends TestCase { ], ]; foreach ($users as $userInfo) { - $this->accountManager->updateUser($userInfo['user'], $userInfo['data'], false); + $this->invokePrivate($this->accountManager, 'updateUser', [$userInfo['user'], $userInfo['data'], false]); } } @@ -278,7 +278,7 @@ class AccountManagerTest extends TestCase { * @param bool $updateExisting */ public function testUpdateUser($newData, $oldData, $insertNew, $updateExisting) { - $accountManager = $this->getInstance(['getUser', 'insertNewUser', 'updateExistingUser', 'updateVerifyStatus', 'checkEmailVerification']); + $accountManager = $this->getInstance(['getUser', 'insertNewUser', 'updateExistingUser', 'checkEmailVerification']); /** @var IUser $user */ $user = $this->createMock(IUser::class); @@ -288,8 +288,6 @@ class AccountManagerTest extends TestCase { if ($updateExisting) { $accountManager->expects($this->once())->method('checkEmailVerification') ->with($oldData, $newData, $user)->willReturn($newData); - $accountManager->expects($this->once())->method('updateVerifyStatus') - ->with($oldData, $newData)->willReturn($newData); $accountManager->expects($this->once())->method('updateExistingUser') ->with($user, $newData); $accountManager->expects($this->never())->method('insertNewUser'); @@ -303,7 +301,6 @@ class AccountManagerTest extends TestCase { if (!$insertNew && !$updateExisting) { $accountManager->expects($this->never())->method('updateExistingUser'); $accountManager->expects($this->never())->method('checkEmailVerification'); - $accountManager->expects($this->never())->method('updateVerifyStatus'); $accountManager->expects($this->never())->method('insertNewUser'); $this->eventDispatcher->expects($this->never())->method('dispatch'); } else { @@ -319,13 +316,13 @@ class AccountManagerTest extends TestCase { ); } - $accountManager->updateUser($user, $newData); + $this->invokePrivate($accountManager, 'updateUser', [$user, $newData]); } public function dataTrueFalse() { return [ + #$newData | $oldData | $insertNew | $updateExisting [['myProperty' => ['value' => 'newData']], ['myProperty' => ['value' => 'oldData']], false, true], - [['myProperty' => ['value' => 'newData']], [], true, false], [['myProperty' => ['value' => 'oldData']], ['myProperty' => ['value' => 'oldData']], false, false] ]; } @@ -356,9 +353,7 @@ class AccountManagerTest extends TestCase { } $this->addDummyValuesToTable($setUser, $setData); - $this->assertEquals($expectedData, - $accountManager->getUser($askUser) - ); + $this->assertEquals($expectedData, $this->invokePrivate($accountManager, 'getUser', [$askUser])); } public function dataTestGetUser() {