From 5092278b0f4bf1905fe08d51ad329682b6e8f410 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 25 Sep 2023 15:25:01 +0200 Subject: [PATCH 1/3] feat(phonenumber): Add OCP wrapper for PhoneNumber utils library Signed-off-by: Joas Schilling --- lib/composer/composer/autoload_classmap.php | 2 + lib/composer/composer/autoload_static.php | 2 + lib/private/PhoneNumberUtil.php | 72 +++++++++++++++++++++ lib/private/Server.php | 3 + lib/public/IPhoneNumberUtil.php | 52 +++++++++++++++ 5 files changed, 131 insertions(+) create mode 100644 lib/private/PhoneNumberUtil.php create mode 100644 lib/public/IPhoneNumberUtil.php diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 8c4d822942215..6e5bf85efcf93 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -485,6 +485,7 @@ 'OCP\\IMemcache' => $baseDir . '/lib/public/IMemcache.php', 'OCP\\IMemcacheTTL' => $baseDir . '/lib/public/IMemcacheTTL.php', 'OCP\\INavigationManager' => $baseDir . '/lib/public/INavigationManager.php', + 'OCP\\IPhoneNumberUtil' => $baseDir . '/lib/public/IPhoneNumberUtil.php', 'OCP\\IPreview' => $baseDir . '/lib/public/IPreview.php', 'OCP\\IRequest' => $baseDir . '/lib/public/IRequest.php', 'OCP\\IRequestId' => $baseDir . '/lib/public/IRequestId.php', @@ -1480,6 +1481,7 @@ 'OC\\OCS\\Exception' => $baseDir . '/lib/private/OCS/Exception.php', 'OC\\OCS\\Provider' => $baseDir . '/lib/private/OCS/Provider.php', 'OC\\OCS\\Result' => $baseDir . '/lib/private/OCS/Result.php', + 'OC\\PhoneNumberUtil' => $baseDir . '/lib/private/PhoneNumberUtil.php', 'OC\\PreviewManager' => $baseDir . '/lib/private/PreviewManager.php', 'OC\\PreviewNotAvailableException' => $baseDir . '/lib/private/PreviewNotAvailableException.php', 'OC\\Preview\\BMP' => $baseDir . '/lib/private/Preview/BMP.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 6ed91a7d67fab..ef88bcd2a9b19 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -518,6 +518,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OCP\\IMemcache' => __DIR__ . '/../../..' . '/lib/public/IMemcache.php', 'OCP\\IMemcacheTTL' => __DIR__ . '/../../..' . '/lib/public/IMemcacheTTL.php', 'OCP\\INavigationManager' => __DIR__ . '/../../..' . '/lib/public/INavigationManager.php', + 'OCP\\IPhoneNumberUtil' => __DIR__ . '/../../..' . '/lib/public/IPhoneNumberUtil.php', 'OCP\\IPreview' => __DIR__ . '/../../..' . '/lib/public/IPreview.php', 'OCP\\IRequest' => __DIR__ . '/../../..' . '/lib/public/IRequest.php', 'OCP\\IRequestId' => __DIR__ . '/../../..' . '/lib/public/IRequestId.php', @@ -1513,6 +1514,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\OCS\\Exception' => __DIR__ . '/../../..' . '/lib/private/OCS/Exception.php', 'OC\\OCS\\Provider' => __DIR__ . '/../../..' . '/lib/private/OCS/Provider.php', 'OC\\OCS\\Result' => __DIR__ . '/../../..' . '/lib/private/OCS/Result.php', + 'OC\\PhoneNumberUtil' => __DIR__ . '/../../..' . '/lib/private/PhoneNumberUtil.php', 'OC\\PreviewManager' => __DIR__ . '/../../..' . '/lib/private/PreviewManager.php', 'OC\\PreviewNotAvailableException' => __DIR__ . '/../../..' . '/lib/private/PreviewNotAvailableException.php', 'OC\\Preview\\BMP' => __DIR__ . '/../../..' . '/lib/private/Preview/BMP.php', diff --git a/lib/private/PhoneNumberUtil.php b/lib/private/PhoneNumberUtil.php new file mode 100644 index 0000000000000..197ccaba0354a --- /dev/null +++ b/lib/private/PhoneNumberUtil.php @@ -0,0 +1,72 @@ + + * + * @author Joas Schilling + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OC; + +use libphonenumber\NumberParseException; +use libphonenumber\PhoneNumberFormat; +use OCP\IPhoneNumberUtil; + +/** + * @since 28.0.0 + */ +class PhoneNumberUtil implements IPhoneNumberUtil { + /** + * Returns the country code for a specific region + * + * For example, this would be `41` for Switzerland and `49` for Germany. + * Returns null when the region is invalid. + * + * @since 28.0.0 + */ + public function getCountryCodeForRegion(string $regionCode): ?int { + $phoneUtil = \libphonenumber\PhoneNumberUtil::getInstance(); + $countryCode = $phoneUtil->getCountryCodeForRegion($regionCode); + return $countryCode === 0 ? null : $countryCode; + } + + /** + * Converts a given input into an E164 formatted phone number + * + * E164 is the international format without any formatting characters or spaces. + * E.g. +41446681800 where +41 is the region code. + * Returns null when the input is invalid for the given region. + * + * @since 28.0.0 + */ + public function convertToStandardFormat(string $input, ?string $defaultRegion = null): ?string { + $phoneUtil = \libphonenumber\PhoneNumberUtil::getInstance(); + try { + $phoneNumber = $phoneUtil->parse($input, $defaultRegion); + if ($phoneUtil->isValidNumber($phoneNumber)) { + return $phoneUtil->format($phoneNumber, PhoneNumberFormat::E164); + } + } catch (NumberParseException) { + } + + return null; + } +} diff --git a/lib/private/Server.php b/lib/private/Server.php index 7ab57478f44ed..d2a1d890ccd25 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -212,6 +212,7 @@ use OCP\IL10N; use OCP\ILogger; use OCP\INavigationManager; +use OCP\IPhoneNumberUtil; use OCP\IPreview; use OCP\IRequest; use OCP\IRequestId; @@ -1423,6 +1424,8 @@ public function __construct($webRoot, \OC\Config $config) { $this->registerAlias(ILimiter::class, Limiter::class); + $this->registerAlias(IPhoneNumberUtil::class, PhoneNumberUtil::class); + $this->connectDispatcher(); } diff --git a/lib/public/IPhoneNumberUtil.php b/lib/public/IPhoneNumberUtil.php new file mode 100644 index 0000000000000..06f8c77706edd --- /dev/null +++ b/lib/public/IPhoneNumberUtil.php @@ -0,0 +1,52 @@ + + * + * @author Joas Schilling + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OCP; + +/** + * @since 28.0.0 + */ +interface IPhoneNumberUtil { + /** + * Returns the country code for a specific region + * + * For example, this would be `41` for Switzerland and `49` for Germany. + * Returns null when there is no country code for the region. + * + * @since 28.0.0 + */ + public function getCountryCodeForRegion(string $regionCode): ?int; + + /** + * Converts a given input into an E164 formatted phone number + * + * E164 is the international format without any formatting characters or spaces. + * E.g. +41446681800 where +41 is the country code + * + * @since 28.0.0 + */ + public function convertToStandardFormat(string $input, ?string $defaultRegion = null): ?string; +} From 0956b493b6394a6ea3c748fa5dd0910bc697ba8a Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 25 Sep 2023 15:33:40 +0200 Subject: [PATCH 2/3] 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(); From 3b4c3068606249f1912bfe45df57027619f2a165 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 25 Sep 2023 17:32:47 +0200 Subject: [PATCH 3/3] fix(phonenumber): Improve docs by adding input details Signed-off-by: Joas Schilling --- lib/private/PhoneNumberUtil.php | 15 ++------------- lib/public/IPhoneNumberUtil.php | 9 +++++++-- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/lib/private/PhoneNumberUtil.php b/lib/private/PhoneNumberUtil.php index 197ccaba0354a..a1eb2f13233af 100644 --- a/lib/private/PhoneNumberUtil.php +++ b/lib/private/PhoneNumberUtil.php @@ -35,12 +35,7 @@ */ class PhoneNumberUtil implements IPhoneNumberUtil { /** - * Returns the country code for a specific region - * - * For example, this would be `41` for Switzerland and `49` for Germany. - * Returns null when the region is invalid. - * - * @since 28.0.0 + * {@inheritDoc} */ public function getCountryCodeForRegion(string $regionCode): ?int { $phoneUtil = \libphonenumber\PhoneNumberUtil::getInstance(); @@ -49,13 +44,7 @@ public function getCountryCodeForRegion(string $regionCode): ?int { } /** - * Converts a given input into an E164 formatted phone number - * - * E164 is the international format without any formatting characters or spaces. - * E.g. +41446681800 where +41 is the region code. - * Returns null when the input is invalid for the given region. - * - * @since 28.0.0 + * {@inheritDoc} */ public function convertToStandardFormat(string $input, ?string $defaultRegion = null): ?string { $phoneUtil = \libphonenumber\PhoneNumberUtil::getInstance(); diff --git a/lib/public/IPhoneNumberUtil.php b/lib/public/IPhoneNumberUtil.php index 06f8c77706edd..733de0e35a62e 100644 --- a/lib/public/IPhoneNumberUtil.php +++ b/lib/public/IPhoneNumberUtil.php @@ -34,8 +34,10 @@ interface IPhoneNumberUtil { * Returns the country code for a specific region * * For example, this would be `41` for Switzerland and `49` for Germany. - * Returns null when there is no country code for the region. + * Returns null when the region is invalid. * + * @param string $regionCode Two-letter region code of ISO 3166-1 + * @return int|null Null when invalid/unsupported, the phone country code otherwise * @since 28.0.0 */ public function getCountryCodeForRegion(string $regionCode): ?int; @@ -44,8 +46,11 @@ public function getCountryCodeForRegion(string $regionCode): ?int; * Converts a given input into an E164 formatted phone number * * E164 is the international format without any formatting characters or spaces. - * E.g. +41446681800 where +41 is the country code + * E.g. +41446681800 where +41 is the region code. * + * @param string $input Input phone number can contain formatting spaces, slashes and dashes + * @param string|null $defaultRegion Two-letter region code of ISO 3166-1 + * @return string|null Null when the input is invalid for the given region or requires a region. * @since 28.0.0 */ public function convertToStandardFormat(string $input, ?string $defaultRegion = null): ?string;