Unit tests for searching by phone number

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

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);

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,

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"