From d0750df20c3ca69ad250731b8c1e6db1c647c43d Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 3 Dec 2020 20:44:29 +0100 Subject: [PATCH] Unit tests for searching by phone number Signed-off-by: Joas Schilling --- .../lib/Controller/UsersController.php | 21 +++++--- .../tests/Controller/UsersControllerTest.php | 49 ++++++++++++++++--- .../features/provisioning-v1.feature | 2 +- 3 files changed, 56 insertions(+), 16 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index 18baa07a39..735d394796 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -52,10 +52,10 @@ use OCA\Provisioning_API\FederatedShareProviderFactory; use OCA\Settings\Mailer\NewUserMailHelper; use OCP\Accounts\IAccountManager; use OCP\App\IAppManager; +use OCP\AppFramework\Http; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\OCS\OCSException; use OCP\AppFramework\OCS\OCSForbiddenException; -use OCP\Federation\ICloudIdManager; use OCP\IConfig; use OCP\IGroup; use OCP\IGroupManager; @@ -74,8 +74,6 @@ class UsersController extends AUserData { /** @var IAppManager */ private $appManager; - /** @var ICloudIdManager */ - protected $cloudIdManager; /** @var IURLGenerator */ protected $urlGenerator; /** @var ILogger */ @@ -101,7 +99,6 @@ class UsersController extends AUserData { IGroupManager $groupManager, IUserSession $userSession, AccountManager $accountManager, - ICloudIdManager $cloudIdManager, IURLGenerator $urlGenerator, ILogger $logger, IFactory $l10nFactory, @@ -120,7 +117,6 @@ class UsersController extends AUserData { $l10nFactory); $this->appManager = $appManager; - $this->cloudIdManager = $cloudIdManager; $this->urlGenerator = $urlGenerator; $this->logger = $logger; $this->l10nFactory = $l10nFactory; @@ -228,6 +224,11 @@ class UsersController extends AUserData { public function searchByPhoneNumbers(string $location, array $search): DataResponse { $phoneUtil = PhoneNumberUtil::getInstance(); + if ($phoneUtil->getCountryCodeForRegion($location) === 0) { + // Not a valid region code + return new DataResponse([], Http::STATUS_BAD_REQUEST); + } + $normalizedNumberToKey = []; foreach ($search as $key => $phoneNumbers) { foreach ($phoneNumbers as $phone) { @@ -254,11 +255,17 @@ class UsersController extends AUserData { return new DataResponse(); } - $cloudUrl = $this->urlGenerator->getAbsoluteURL('/'); + $cloudUrl = rtrim($this->urlGenerator->getAbsoluteURL('/'), '/'); + if (strpos($cloudUrl, 'http://') === 0) { + $cloudUrl = substr($cloudUrl, strlen('http://')); + } elseif (strpos($cloudUrl, 'https://') === 0) { + $cloudUrl = substr($cloudUrl, strlen('https://')); + } $matches = []; foreach ($userMatches as $phone => $userId) { - $matches[$normalizedNumberToKey[$phone]] = $this->cloudIdManager->getCloudId($userId, $cloudUrl)->getId(); + // Not using the ICloudIdManager as that would run a search for each contact to find the display name in the address book + $matches[$normalizedNumberToKey[$phone]] = $userId . '@' . $cloudUrl; } return new DataResponse($matches); diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php index db00963626..b6f28cc4a0 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -52,7 +52,6 @@ use OCP\Accounts\IAccountManager; use OCP\App\IAppManager; use OCP\AppFramework\Http\DataResponse; use OCP\EventDispatcher\IEventDispatcher; -use OCP\Federation\ICloudIdManager; use OCP\IConfig; use OCP\IGroup; use OCP\IL10N; @@ -88,8 +87,6 @@ class UsersControllerTest extends TestCase { protected $api; /** @var AccountManager|MockObject */ protected $accountManager; - /** @var ICloudIdManager|MockObject */ - protected $cloudIdManager; /** @var IURLGenerator|MockObject */ protected $urlGenerator; /** @var IRequest|MockObject */ @@ -118,7 +115,6 @@ class UsersControllerTest extends TestCase { $this->logger = $this->createMock(ILogger::class); $this->request = $this->createMock(IRequest::class); $this->accountManager = $this->createMock(AccountManager::class); - $this->cloudIdManager = $this->createMock(ICloudIdManager::class); $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->l10nFactory = $this->createMock(IFactory::class); $this->newUserMailHelper = $this->createMock(NewUserMailHelper::class); @@ -137,7 +133,6 @@ class UsersControllerTest extends TestCase { $this->groupManager, $this->userSession, $this->accountManager, - $this->cloudIdManager, $this->urlGenerator, $this->logger, $this->l10nFactory, @@ -402,7 +397,6 @@ class UsersControllerTest extends TestCase { $this->groupManager, $this->userSession, $this->accountManager, - $this->cloudIdManager, $this->urlGenerator, $this->logger, $this->l10nFactory, @@ -1383,6 +1377,47 @@ class UsersControllerTest extends TestCase { $this->assertEquals($expected, $this->invokePrivate($this->api, 'getUserData', ['UID'])); } + public function dataSearchByPhoneNumbers(): array { + return [ + 'Invalid country' => ['Not a country code', ['12345' => ['NaN']], 400, null, null, []], + 'No number to search' => ['DE', ['12345' => ['NaN']], 200, null, null, []], + 'Valid number but no match' => ['DE', ['12345' => ['0711 / 25 24 28-90']], 200, ['+4971125242890'], [], []], + 'Invalid number' => ['FR', ['12345' => ['0711 / 25 24 28-90']], 200, null, null, []], + 'Invalid and valid number' => ['DE', ['12345' => ['NaN', '0711 / 25 24 28-90']], 200, ['+4971125242890'], [], []], + 'Valid and invalid number' => ['DE', ['12345' => ['0711 / 25 24 28-90', 'NaN']], 200, ['+4971125242890'], [], []], + 'Valid number and a match' => ['DE', ['12345' => ['0711 / 25 24 28-90']], 200, ['+4971125242890'], ['+4971125242890' => 'admin'], ['12345' => 'admin@localhost']], + 'Same number twice, later hits' => ['DE', ['12345' => ['0711 / 25 24 28-90'], '23456' => ['0711 / 25 24 28-90']], 200, ['+4971125242890'], ['+4971125242890' => 'admin'], ['23456' => 'admin@localhost']], + ]; + } + + /** + * @dataProvider dataSearchByPhoneNumbers + * @param string $location + * @param array $search + * @param int $status + * @param array $expected + */ + public function testSearchByPhoneNumbers(string $location, array $search, int $status, ?array $searchUsers, ?array $userMatches, array $expected) { + if ($searchUsers === null) { + $this->accountManager->expects($this->never()) + ->method('searchUsers'); + } else { + $this->accountManager->expects($this->once()) + ->method('searchUsers') + ->with(IAccountManager::PROPERTY_PHONE, $searchUsers) + ->willReturn($userMatches); + } + + $this->urlGenerator->method('getAbsoluteURL') + ->with('/') + ->willReturn('https://localhost/'); + + $response = $this->api->searchByPhoneNumbers($location, $search); + + self::assertEquals($status, $response->getStatus()); + self::assertEquals($expected, $response->getData()); + } + public function testEditUserRegularUserSelfEditChangeDisplayName() { $loggedInUser = $this->getMockBuilder(IUser::class) ->disableOriginalConstructor() @@ -3185,7 +3220,6 @@ class UsersControllerTest extends TestCase { $this->groupManager, $this->userSession, $this->accountManager, - $this->cloudIdManager, $this->urlGenerator, $this->logger, $this->l10nFactory, @@ -3252,7 +3286,6 @@ class UsersControllerTest extends TestCase { $this->groupManager, $this->userSession, $this->accountManager, - $this->cloudIdManager, $this->urlGenerator, $this->logger, $this->l10nFactory, diff --git a/build/integration/features/provisioning-v1.feature b/build/integration/features/provisioning-v1.feature index 123e58a869..717aa04e4b 100644 --- a/build/integration/features/provisioning-v1.feature +++ b/build/integration/features/provisioning-v1.feature @@ -118,7 +118,7 @@ Feature: provisioning And the OCS status code should be "100" And the HTTP status code should be "200" Then phone matches returned are - | random-string1 | phone-user@http://localhost:8080/ | + | random-string1 | phone-user@localhost:8080 | Scenario: Create a group Given As an "admin"