Also translate the phone number when coming in via the accounts manager API directly

Signed-off-by: Joas Schilling <coding@schilljs.com>

4 files changed, 43 insertions, 28 deletions

diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php
index 08c541ffa6c..edb1fc5121a 100644
--- a/apps/provisioning_api/lib/Controller/UsersController.php
+++ b/apps/provisioning_api/lib/Controller/UsersController.php
@@ -654,7 +654,11 @@ class UsersController extends AUserData {
 				$userAccount = $this->accountManager->getUser($targetUser);
 				if ($userAccount[$key]['value'] !== $value) {
 					$userAccount[$key]['value'] = $value;
-					$this->accountManager->updateUser($targetUser, $userAccount);
+					try {
+						$this->accountManager->updateUser($targetUser, $userAccount);
+					} catch (\InvalidArgumentException $e) {
+						throw new OCSException('Invalid ' . $e->getMessage(), 102);
+					}
 				}
 				break;
 			default:
diff --git a/apps/settings/lib/Controller/UsersController.php b/apps/settings/lib/Controller/UsersController.php
index 4267b8be4c4..5b3c1574f96 100644
--- a/apps/settings/lib/Controller/UsersController.php
+++ b/apps/settings/lib/Controller/UsersController.php
@@ -35,10 +35,6 @@ declare(strict_types=1);
 
 namespace OCA\Settings\Controller;
 
-use libphonenumber\NumberParseException;
-use libphonenumber\PhoneNumber;
-use libphonenumber\PhoneNumberFormat;
-use libphonenumber\PhoneNumberUtil;
 use OC\Accounts\AccountManager;
 use OC\AppFramework\Http;
 use OC\Encryption\Exceptions\ModuleDoesNotExistsException;
@@ -469,24 +465,14 @@ class UsersController extends Controller {
 			$user->setEMailAddress($data[IAccountManager::PROPERTY_EMAIL]['value']);
 		}
 
-		if (isset($data[AccountManager::PROPERTY_PHONE])) {
-			$phoneUtil = PhoneNumberUtil::getInstance();
-			try {
-				$phoneValue = $data[AccountManager::PROPERTY_PHONE]['value'];
-				$phoneNumber = $phoneUtil->parse($phoneValue, 'DE'); // FIXME need a reasonable default
-				if ($phoneNumber instanceof PhoneNumber && $phoneUtil->isValidNumber($phoneNumber)) {
-					$data[AccountManager::PROPERTY_PHONE]['value'] = $phoneUtil->format($phoneNumber, PhoneNumberFormat::E164);
-				} else {
-					throw new \InvalidArgumentException($this->l10n->t('Unable to set invalid phone number'));
-				}
-			} catch (NumberParseException $e) {
+		try {
+			return $this->accountManager->updateUser($user, $data);
+		} 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('Some account data was invalid'));
 		}
-
-		$this->accountManager->updateUser($user, $data);
-
-		return $data;
 	}
 
 	/**
@@ -521,14 +507,12 @@ class UsersController extends Controller {
 				$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;
-				$data = $accountData[IAccountManager::PROPERTY_TWITTER]['value'];
 				$accountData[IAccountManager::PROPERTY_TWITTER]['signature'] = $signature;
 				break;
 			case 'verify-website':
 				$accountData[IAccountManager::PROPERTY_WEBSITE]['verified'] = AccountManager::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;
-				$data = $accountData[IAccountManager::PROPERTY_WEBSITE]['value'];
 				$accountData[IAccountManager::PROPERTY_WEBSITE]['signature'] = $signature;
 				break;
 			default:
@@ -536,7 +520,8 @@ class UsersController extends Controller {
 		}
 
 		if ($onlyVerificationCode === false) {
-			$this->accountManager->updateUser($user, $accountData);
+			$accountData = $this->accountManager->updateUser($user, $accountData);
+			$data = $accountData[$type]['value'];
 
 			$this->jobList->add(VerifyUserData::class,
 				[
diff --git a/apps/settings/tests/Controller/UsersControllerTest.php b/apps/settings/tests/Controller/UsersControllerTest.php
index 75e71435b0f..23e3ef5ec01 100644
--- a/apps/settings/tests/Controller/UsersControllerTest.php
+++ b/apps/settings/tests/Controller/UsersControllerTest.php
@@ -239,12 +239,14 @@ class UsersControllerTest extends \Test\TestCase {
 						],
 				]);
 
-			$controller->expects($this->once())->method('saveUserSettings');
+			$controller->expects($this->once())
+				->method('saveUserSettings')
+				->willReturnArgument(1);
 		} else {
 			$controller->expects($this->never())->method('saveUserSettings');
 		}
 
-		$result = $controller->setUserSettings(
+		$result = $controller->setUserSettings(//
 			AccountManager::VISIBILITY_CONTACTS_ONLY,
 			'displayName',
 			AccountManager::VISIBILITY_CONTACTS_ONLY,
@@ -471,7 +473,7 @@ class UsersControllerTest extends \Test\TestCase {
 		$controller->expects($this->any())->method('getCurrentTime')->willReturn(1234567);
 
 		if ($onlyVerificationCode === false) {
-			$this->accountManager->expects($this->once())->method('updateUser')->with($user, $expectedData);
+			$this->accountManager->expects($this->once())->method('updateUser')->with($user, $expectedData)->willReturnArgument(1);
 			$this->jobList->expects($this->once())->method('add')
 				->with('OCA\Settings\BackgroundJobs\VerifyUserData',
 					[
diff --git a/lib/private/Accounts/AccountManager.php b/lib/private/Accounts/AccountManager.php
index 39921dae9db..4f4a146bdd9 100644
--- a/lib/private/Accounts/AccountManager.php
+++ b/lib/private/Accounts/AccountManager.php
@@ -30,6 +30,10 @@
 
 namespace OC\Accounts;
 
+use libphonenumber\NumberParseException;
+use libphonenumber\PhoneNumber;
+use libphonenumber\PhoneNumberFormat;
+use libphonenumber\PhoneNumberUtil;
 use OCA\Settings\BackgroundJobs\VerifyUserData;
 use OCP\Accounts\IAccount;
 use OCP\Accounts\IAccountManager;
@@ -85,11 +89,29 @@ class AccountManager implements IAccountManager {
 	 * update user record
 	 *
 	 * @param IUser $user
-	 * @param $data
+	 * @param array $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, $data) {
+	public function updateUser(IUser $user, array $data): array {
 		$userData = $this->getUser($user);
 		$updated = true;
+
+		if (isset($data[self::PROPERTY_PHONE])) {
+			$phoneUtil = PhoneNumberUtil::getInstance();
+			try {
+				$phoneValue = $data[self::PROPERTY_PHONE]['value'];
+				$phoneNumber = $phoneUtil->parse($phoneValue, 'DE'); // FIXME need a reasonable default
+				if ($phoneNumber instanceof PhoneNumber && $phoneUtil->isValidNumber($phoneNumber)) {
+					$data[self::PROPERTY_PHONE]['value'] = $phoneUtil->format($phoneNumber, PhoneNumberFormat::E164);
+				} else {
+					throw new \InvalidArgumentException(self::PROPERTY_PHONE);
+				}
+			} catch (NumberParseException $e) {
+				throw new \InvalidArgumentException(self::PROPERTY_PHONE);
+			}
+		}
+
 		if (empty($userData)) {
 			$this->insertNewUser($user, $data);
 		} elseif ($userData !== $data) {
@@ -107,6 +129,8 @@ class AccountManager implements IAccountManager {
 				new GenericEvent($user, $data)
 			);
 		}
+
+		return $data;
 	}
 
 	/**