From 0956b493b6394a6ea3c748fa5dd0910bc697ba8a Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 25 Sep 2023 15:33:40 +0200 Subject: [PATCH] fix(phonenumber): Use the newly introduced API to limit 3rdparty lib usage Signed-off-by: Joas Schilling --- .../lib/Controller/UsersController.php | 33 +++++++------------ .../tests/Controller/UsersControllerTest.php | 19 ++++++++--- lib/private/Accounts/AccountManager.php | 15 +++------ tests/lib/Accounts/AccountManagerTest.php | 11 +++++-- 4 files changed, 39 insertions(+), 39 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index 07f651c74fa4e..c3e9d50267555 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -45,9 +45,6 @@ namespace OCA\Provisioning_API\Controller; use InvalidArgumentException; -use libphonenumber\NumberParseException; -use libphonenumber\PhoneNumberFormat; -use libphonenumber\PhoneNumberUtil; use OC\Authentication\Token\RemoteWipe; use OC\KnownUser\KnownUserService; use OC\User\Backend; @@ -66,6 +63,7 @@ use OCP\IConfig; use OCP\IGroup; use OCP\IGroupManager; +use OCP\IPhoneNumberUtil; use OCP\IRequest; use OCP\IURLGenerator; use OCP\IUser; @@ -113,7 +111,8 @@ public function __construct( ISecureRandom $secureRandom, RemoteWipe $remoteWipe, KnownUserService $knownUserService, - IEventDispatcher $eventDispatcher + IEventDispatcher $eventDispatcher, + private IPhoneNumberUtil $phoneNumberUtil, ) { parent::__construct( $appName, @@ -243,9 +242,7 @@ public function getUsersDetails(string $search = '', int $limit = null, int $off * 400: Invalid location */ public function searchByPhoneNumbers(string $location, array $search): DataResponse { - $phoneUtil = PhoneNumberUtil::getInstance(); - - if ($phoneUtil->getCountryCodeForRegion($location) === 0) { + if ($this->phoneNumberUtil->getCountryCodeForRegion($location) === null) { // Not a valid region code return new DataResponse([], Http::STATUS_BAD_REQUEST); } @@ -258,26 +255,18 @@ public function searchByPhoneNumbers(string $location, array $search): DataRespo $normalizedNumberToKey = []; foreach ($search as $key => $phoneNumbers) { foreach ($phoneNumbers as $phone) { - try { - $phoneNumber = $phoneUtil->parse($phone, $location); - if ($phoneUtil->isValidNumber($phoneNumber)) { - $normalizedNumber = $phoneUtil->format($phoneNumber, PhoneNumberFormat::E164); - $normalizedNumberToKey[$normalizedNumber] = (string) $key; - } - } catch (NumberParseException $e) { + $normalizedNumber = $this->phoneNumberUtil->convertToStandardFormat($phone, $location); + if ($normalizedNumber !== null) { + $normalizedNumberToKey[$normalizedNumber] = (string) $key; } - if ($defaultPhoneRegion !== '' && $defaultPhoneRegion !== $location && strpos($phone, '0') === 0) { + if ($defaultPhoneRegion !== '' && $defaultPhoneRegion !== $location && str_starts_with($phone, '0')) { // If the number has a leading zero (no country code), // we also check the default phone region of the instance, // when it's different to the user's given region. - try { - $phoneNumber = $phoneUtil->parse($phone, $defaultPhoneRegion); - if ($phoneUtil->isValidNumber($phoneNumber)) { - $normalizedNumber = $phoneUtil->format($phoneNumber, PhoneNumberFormat::E164); - $normalizedNumberToKey[$normalizedNumber] = (string) $key; - } - } catch (NumberParseException $e) { + $normalizedNumber = $this->phoneNumberUtil->convertToStandardFormat($phone, $defaultPhoneRegion); + if ($normalizedNumber !== null) { + $normalizedNumberToKey[$normalizedNumber] = (string) $key; } } } diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php index 56a47f23a5cc4..a48461c0a27b5 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -46,6 +46,7 @@ use OC\Authentication\Token\RemoteWipe; use OC\Group\Manager; use OC\KnownUser\KnownUserService; +use OC\PhoneNumberUtil; use OC\SubAdmin; use OCA\Provisioning_API\Controller\UsersController; use OCA\Settings\Mailer\NewUserMailHelper; @@ -59,6 +60,7 @@ use OCP\IConfig; use OCP\IGroup; use OCP\IL10N; +use OCP\IPhoneNumberUtil; use OCP\IRequest; use OCP\IURLGenerator; use OCP\IUser; @@ -103,8 +105,10 @@ class UsersControllerTest extends TestCase { private $remoteWipe; /** @var KnownUserService|MockObject */ private $knownUserService; - /** @var IEventDispatcher */ + /** @var IEventDispatcher|MockObject */ private $eventDispatcher; + /** @var IPhoneNumberUtil */ + private $phoneNumberUtil; protected function setUp(): void { parent::setUp(); @@ -123,6 +127,7 @@ protected function setUp(): void { $this->remoteWipe = $this->createMock(RemoteWipe::class); $this->knownUserService = $this->createMock(KnownUserService::class); $this->eventDispatcher = $this->createMock(IEventDispatcher::class); + $this->phoneNumberUtil = new PhoneNumberUtil(); $this->api = $this->getMockBuilder(UsersController::class) ->setConstructorArgs([ @@ -141,8 +146,9 @@ protected function setUp(): void { $this->remoteWipe, $this->knownUserService, $this->eventDispatcher, + $this->phoneNumberUtil, ]) - ->setMethods(['fillStorageInfo']) + ->onlyMethods(['fillStorageInfo']) ->getMock(); } @@ -411,8 +417,9 @@ public function testAddUserSuccessfulWithDisplayName() { $this->remoteWipe, $this->knownUserService, $this->eventDispatcher, + $this->phoneNumberUtil, ]) - ->setMethods(['editUser']) + ->onlyMethods(['editUser']) ->getMock(); $this->userManager @@ -3693,8 +3700,9 @@ public function testGetCurrentUserLoggedIn() { $this->remoteWipe, $this->knownUserService, $this->eventDispatcher, + $this->phoneNumberUtil, ]) - ->setMethods(['getUserData']) + ->onlyMethods(['getUserData']) ->getMock(); $api->expects($this->once())->method('getUserData')->with('UID', true) @@ -3779,8 +3787,9 @@ public function testGetUser() { $this->remoteWipe, $this->knownUserService, $this->eventDispatcher, + $this->phoneNumberUtil, ]) - ->setMethods(['getUserData']) + ->onlyMethods(['getUserData']) ->getMock(); $expected = [ diff --git a/lib/private/Accounts/AccountManager.php b/lib/private/Accounts/AccountManager.php index 9865438161b20..3e33e783635fe 100644 --- a/lib/private/Accounts/AccountManager.php +++ b/lib/private/Accounts/AccountManager.php @@ -37,9 +37,6 @@ use Exception; use InvalidArgumentException; -use libphonenumber\NumberParseException; -use libphonenumber\PhoneNumberFormat; -use libphonenumber\PhoneNumberUtil; use OC\Profile\TProfileHelper; use OCP\Accounts\UserUpdatedEvent; use OCP\Cache\CappedMemoryCache; @@ -56,6 +53,7 @@ use OCP\IConfig; use OCP\IDBConnection; use OCP\IL10N; +use OCP\IPhoneNumberUtil; use OCP\IURLGenerator; use OCP\IUser; use OCP\L10N\IFactory; @@ -119,6 +117,7 @@ public function __construct( private IFactory $l10nFactory, private IURLGenerator $urlGenerator, private ICrypto $crypto, + private IPhoneNumberUtil $phoneNumberUtil, ) { $this->internalCache = new CappedMemoryCache(); } @@ -139,13 +138,9 @@ protected function parsePhoneNumber(string $input): string { $defaultRegion = 'EN'; } - $phoneUtil = PhoneNumberUtil::getInstance(); - try { - $phoneNumber = $phoneUtil->parse($input, $defaultRegion); - if ($phoneUtil->isValidNumber($phoneNumber)) { - return $phoneUtil->format($phoneNumber, PhoneNumberFormat::E164); - } - } catch (NumberParseException $e) { + $phoneNumber = $this->phoneNumberUtil->convertToStandardFormat($input, $defaultRegion); + if ($phoneNumber !== null) { + return $phoneNumber; } throw new InvalidArgumentException(self::PROPERTY_PHONE); diff --git a/tests/lib/Accounts/AccountManagerTest.php b/tests/lib/Accounts/AccountManagerTest.php index d12dfbfaceae4..3d0bee5902f9c 100644 --- a/tests/lib/Accounts/AccountManagerTest.php +++ b/tests/lib/Accounts/AccountManagerTest.php @@ -26,6 +26,7 @@ use OC\Accounts\Account; use OC\Accounts\AccountManager; +use OC\PhoneNumberUtil; use OCA\Settings\BackgroundJobs\VerifyUserData; use OCP\Accounts\IAccountManager; use OCP\Accounts\UserUpdatedEvent; @@ -34,6 +35,7 @@ use OCP\EventDispatcher\IEventDispatcher; use OCP\IConfig; use OCP\IDBConnection; +use OCP\IPhoneNumberUtil; use OCP\IURLGenerator; use OCP\IUser; use OCP\L10N\IFactory; @@ -75,6 +77,8 @@ class AccountManagerTest extends TestCase { /** @var IJobList|MockObject */ private $jobList; + /** @var IPhoneNumberUtil */ + private $phoneNumberUtil; /** accounts table name */ private string $table = 'accounts'; @@ -97,6 +101,7 @@ protected function setUp(): void { $this->l10nFactory = $this->createMock(IFactory::class); $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->crypto = $this->createMock(ICrypto::class); + $this->phoneNumberUtil = new PhoneNumberUtil(); $this->accountManager = new AccountManager( $this->connection, @@ -109,7 +114,8 @@ protected function setUp(): void { $this->defaults, $this->l10nFactory, $this->urlGenerator, - $this->crypto + $this->crypto, + $this->phoneNumberUtil, ); } @@ -473,7 +479,8 @@ public function getInstance(?array $mockedMethods = null) { $this->defaults, $this->l10nFactory, $this->urlGenerator, - $this->crypto + $this->crypto, + $this->phoneNumberUtil, ]) ->onlyMethods($mockedMethods) ->getMock();