Skip to content

always query the lookup server in a global scale setup #11714

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
schiessle merged 4 commits into from
Oct 15, 2018
Merged

always query the lookup server in a global scale setup #11714

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3fa13e7
always query the lookup server in a global scale setup and have a nic…
schiessle Oct 9, 2018
9e76d40
adjust tests
schiessle Oct 9, 2018
fd5fc24
some small fixes and improvements
schiessle Oct 10, 2018
1b0b159
add more tests
schiessle Oct 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 35 additions & 3 deletions lib/private/Collaboration/Collaborators/LookupPlugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,11 @@
use OCP\Collaboration\Collaborators\ISearchPlugin;
use OCP\Collaboration\Collaborators\ISearchResult;
use OCP\Collaboration\Collaborators\SearchResultType;
use OCP\Federation\ICloudIdManager;
use OCP\Http\Client\IClientService;
use OCP\IConfig;
use OCP\ILogger;
use OCP\IUserSession;
use OCP\Share;

class LookupPlugin implements ISearchPlugin {
Expand All @@ -37,14 +40,31 @@ class LookupPlugin implements ISearchPlugin {
private $config;
/** @var IClientService */
private $clientService;
/** @var string remote part of the current user's cloud id */
private $currentUserRemote;
/** @var ICloudIdManager */
private $cloudIdManager;
/** @var ILogger */
private $logger;

public function __construct(IConfig $config, IClientService $clientService) {
public function __construct(IConfig $config,
IClientService $clientService,
IUserSession $userSession,
ICloudIdManager $cloudIdManager,
ILogger $logger) {
$this->config = $config;
$this->clientService = $clientService;
$this->cloudIdManager = $cloudIdManager;
$currentUserCloudId = $userSession->getUser()->getCloudId();
$this->currentUserRemote = $cloudIdManager->resolveCloudId($currentUserCloudId)->getRemote();
$this->logger = $logger;
}

public function search($search, $limit, $offset, ISearchResult $searchResult) {
if ($this->config->getAppValue('files_sharing', 'lookupServerEnabled', 'no') !== 'yes') {
$isGlobalScaleEnabled = $this->config->getSystemValue('gs.enabled', false);
$isLookupServerEnabled = $this->config->getAppValue('files_sharing', 'lookupServerEnabled', 'no') === 'yes';
// if case of Global Scale we always search the lookup server
if (!$isLookupServerEnabled && !$isGlobalScaleEnabled) {
return false;
}

Expand All @@ -65,8 +85,20 @@ public function search($search, $limit, $offset, ISearchResult $searchResult) {
$body = json_decode($response->getBody(), true);

foreach ($body as $lookup) {
try {
$remote = $this->cloudIdManager->resolveCloudId($lookup['federationId'])->getRemote();
} catch (\Exception $e) {
$this->logger->error('Can not parse federated cloud ID "' . $lookup['federationId'] . '"');
$this->logger->error($e->getMessage());
continue;
}
if ($this->currentUserRemote === $remote) {
continue;
}
$name = isset($lookup['name']['value']) ? $lookup['name']['value'] : '';
Copy link
Member

@nickvergessen nickvergessen Oct 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use ?? operator, see backport

Copy link
Member Author

@schiessle schiessle Oct 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but honestly I consider the other one more readable so I will stick to it for now. All other points are addressed... Thanks for all the feedback!

$label = empty($name) ? $lookup['federationId'] : $name . ' (' . $lookup['federationId'] . ')';
$result[] = [
'label' => $lookup['federationId'],
'label' => $label,
'value' => [
'shareType' => Share::SHARE_TYPE_REMOTE,
'shareWith' => $lookup['federationId'],
Expand Down
43 changes: 41 additions & 2 deletions tests/lib/Collaboration/Collaborators/LookupPluginTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,18 @@


use OC\Collaboration\Collaborators\LookupPlugin;
use OC\Federation\CloudId;
use OCP\Collaboration\Collaborators\ISearchResult;
use OCP\Collaboration\Collaborators\SearchResultType;
use OCP\Federation\ICloudId;
use OCP\Federation\ICloudIdManager;
use OCP\Http\Client\IClient;
use OCP\Http\Client\IClientService;
use OCP\Http\Client\IResponse;
use OCP\IConfig;
use OCP\ILogger;
use OCP\IUser;
use OCP\IUserSession;
use OCP\Share;
use Test\TestCase;

Expand All @@ -40,16 +46,45 @@ class LookupPluginTest extends TestCase {
protected $config;
/** @var IClientService|\PHPUnit_Framework_MockObject_MockObject */
protected $clientService;
/** @var IUserSession|\PHPUnit_Framework_MockObject_MockObject */
protected $userSession;
/** @var ICloudIdManager|\PHPUnit_Framework_MockObject_MockObject */
protected $cloudIdManager;
/** @var LookupPlugin */
protected $plugin;
/** @var ILogger|\PHPUnit_Framework_MockObject_MockObject */
protected $logger;

public function setUp() {
parent::setUp();

$this->userSession = $this->createMock(IUserSession::class);
$this->cloudIdManager = $this->createMock(ICloudIdManager::class);
$this->config = $this->createMock(IConfig::class);
$this->logger = $this->createMock(ILogger::class);
$this->clientService = $this->createMock(IClientService::class);
$cloudId = $this->createMock(ICloudId::class);
$cloudId->expects($this->any())->method('getRemote')->willReturn('myNextcloud.net');
$user = $this->createMock(IUser::class);
$user->expects($this->any())->method('getCloudId')->willReturn('[email protected]');
$this->userSession->expects($this->any())->method('getUser')
->willReturn($user);
$this->cloudIdManager->expects($this->any())->method('resolveCloudId')
->willReturnCallback(function($cloudId) {
if ($cloudId === '[email protected]') {
return new CloudId('[email protected]', 'user', 'myNextcloud.net');
}
return new CloudId('[email protected]', 'user', 'someNextcloud.net');
});


$this->plugin = new LookupPlugin($this->config, $this->clientService);
$this->plugin = new LookupPlugin(
$this->config,
$this->clientService,
$this->userSession,
$this->cloudIdManager,
$this->logger
);
}

/**
Expand All @@ -69,7 +104,11 @@ public function testSearch(array $searchParams) {
->method('getAppValue')
->with('files_sharing', 'lookupServerEnabled', 'no')
->willReturn('yes');
$this->config->expects($this->once())
$this->config->expects($this->at(0))
->method('getSystemValue')
->with('gs.enabled', false)
Copy link
Member

@blizzz blizzz Oct 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have tests with gs.enabled = true, too, please?

Copy link
Member Author

@schiessle schiessle Oct 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will produce the exact same results, because the lookup server is enabled in all test cases 😉 But I added another test (see last commit) which might do what you meant...

Copy link
Member

@blizzz blizzz Oct 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks :)

Copy link
Member Author

@schiessle schiessle Oct 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blizzz does this count as 👍 😉

->willReturn(false);
$this->config->expects($this->at(2))
->method('getSystemValue')
->with('lookup_server', 'https://lookup.nextcloud.com')
->willReturn($searchParams['server']);
Expand Down