Skip to content
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

dav: Converter & SyncService shall not use private AccountManager #26922

Merged
merged 2 commits into from
May 12, 2021
Merged

dav: Converter & SyncService shall not use private AccountManager #26922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
04edd06
dav: do not use private AccountManager
blizzz May 7, 2021
665ffbd
remove private AccountManager from SyncService
blizzz May 11, 2021
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
61 changes: 20 additions & 41 deletions apps/dav/lib/CardDAV/Converter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@

namespace OCA\DAV\CardDAV;

use OC\Accounts\AccountManager;
use Exception;
use OCP\Accounts\IAccountManager;
use OCP\IImage;
use OCP\IUser;
Expand All @@ -36,24 +36,15 @@

class Converter {

/** @var AccountManager */
/** @var IAccountManager */
private $accountManager;

/**
* Converter constructor.
*
* @param AccountManager $accountManager
*/
public function __construct(AccountManager $accountManager) {
public function __construct(IAccountManager $accountManager) {
$this->accountManager = $accountManager;
}

/**
* @param IUser $user
* @return VCard|null
*/
public function createCardFromUser(IUser $user) {
$userData = $this->accountManager->getUser($user);
public function createCardFromUser(IUser $user): ?VCard {
$userProperties = $this->accountManager->getAccount($user)->getProperties();

$uid = $user->getUID();
$cloudId = $user->getCloudId();
Expand All @@ -65,43 +56,39 @@ public function createCardFromUser(IUser $user) {

$publish = false;

if ($image !== null && isset($userData[IAccountManager::PROPERTY_AVATAR])) {
Copy link
Member

Choose a reason for hiding this comment

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

where did this one go ? obsolete ?

Copy link
Member Author

Choose a reason for hiding this comment

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

it is obsolete, because we run over the set attributes, so no need to check for the presence anymore. Looked strange anyway.

$userData[IAccountManager::PROPERTY_AVATAR]['value'] = true;
}

foreach ($userData as $property => $value) {
foreach ($userProperties as $property) {
$shareWithTrustedServers =
$value['scope'] === AccountManager::SCOPE_FEDERATED ||
$value['scope'] === AccountManager::SCOPE_PUBLISHED;
$property->getScope() === IAccountManager::SCOPE_FEDERATED ||
$property->getScope() === IAccountManager::SCOPE_PUBLISHED;

$emptyValue = !isset($value['value']) || $value['value'] === '';
$emptyValue = $property->getValue() === '';

if ($shareWithTrustedServers && !$emptyValue) {
$publish = true;
switch ($property) {
switch ($property->getName()) {
case IAccountManager::PROPERTY_DISPLAYNAME:
$vCard->add(new Text($vCard, 'FN', $value['value']));
$vCard->add(new Text($vCard, 'N', $this->splitFullName($value['value'])));
$vCard->add(new Text($vCard, 'FN', $property->getValue()));
$vCard->add(new Text($vCard, 'N', $this->splitFullName($property->getValue())));
break;
case IAccountManager::PROPERTY_AVATAR:
if ($image !== null) {
$vCard->add('PHOTO', $image->data(), ['ENCODING' => 'b', 'TYPE' => $image->mimeType()]);
}
break;
case IAccountManager::PROPERTY_EMAIL:
$vCard->add(new Text($vCard, 'EMAIL', $value['value'], ['TYPE' => 'OTHER']));
$vCard->add(new Text($vCard, 'EMAIL', $property->getValue(), ['TYPE' => 'OTHER']));
break;
case IAccountManager::PROPERTY_WEBSITE:
$vCard->add(new Text($vCard, 'URL', $value['value']));
$vCard->add(new Text($vCard, 'URL', $property->getValue()));
break;
case IAccountManager::PROPERTY_PHONE:
$vCard->add(new Text($vCard, 'TEL', $value['value'], ['TYPE' => 'OTHER']));
$vCard->add(new Text($vCard, 'TEL', $property->getValue(), ['TYPE' => 'OTHER']));
break;
case IAccountManager::PROPERTY_ADDRESS:
$vCard->add(new Text($vCard, 'ADR', $value['value'], ['TYPE' => 'OTHER']));
$vCard->add(new Text($vCard, 'ADR', $property->getValue(), ['TYPE' => 'OTHER']));
break;
case IAccountManager::PROPERTY_TWITTER:
$vCard->add(new Text($vCard, 'X-SOCIALPROFILE', $value['value'], ['TYPE' => 'TWITTER']));
$vCard->add(new Text($vCard, 'X-SOCIALPROFILE', $property->getValue(), ['TYPE' => 'TWITTER']));
break;
}
}
Expand All @@ -116,11 +103,7 @@ public function createCardFromUser(IUser $user) {
return null;
}

/**
* @param string $fullName
* @return string[]
*/
public function splitFullName($fullName) {
public function splitFullName(string $fullName): array {
// Very basic western style parsing. I'm not gonna implement
// https://github.com/android/platform_packages_providers_contactsprovider/blob/master/src/com/android/providers/contacts/NameSplitter.java ;)

Expand All @@ -140,14 +123,10 @@ public function splitFullName($fullName) {
return $result;
}

/**
* @param IUser $user
* @return null|IImage
*/
private function getAvatarImage(IUser $user) {
private function getAvatarImage(IUser $user): ?IImage {
try {
return $user->getAvatarImage(-1);
} catch (\Exception $ex) {
} catch (Exception $ex) {
return null;
}
}
Expand Down
13 changes: 6 additions & 7 deletions apps/dav/lib/CardDAV/SyncService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ class SyncService {
/** @var array */
private $localSystemAddressBook;

/** @var AccountManager */
private $accountManager;
/** @var Converter */
private $converter;

/** @var string */
protected $certPath;
Expand All @@ -68,11 +68,11 @@ class SyncService {
* @param ILogger $logger
* @param AccountManager $accountManager
*/
public function __construct(CardDavBackend $backend, IUserManager $userManager, ILogger $logger, AccountManager $accountManager) {
public function __construct(CardDavBackend $backend, IUserManager $userManager, ILogger $logger, Converter $converter) {
$this->backend = $backend;
$this->userManager = $userManager;
$this->logger = $logger;
$this->accountManager = $accountManager;
$this->converter = $converter;
$this->certPath = '';
}

Expand Down Expand Up @@ -265,20 +265,19 @@ private function parseMultiStatus($body) {
public function updateUser(IUser $user) {
$systemAddressBook = $this->getLocalSystemAddressBook();
$addressBookId = $systemAddressBook['id'];
$converter = new Converter($this->accountManager);
$name = $user->getBackendClassName();
$userId = $user->getUID();

$cardId = "$name:$userId.vcf";
$card = $this->backend->getCard($addressBookId, $cardId);
if ($user->isEnabled()) {
if ($card === false) {
$vCard = $converter->createCardFromUser($user);
$vCard = $this->converter->createCardFromUser($user);
if ($vCard !== null) {
$this->backend->createCard($addressBookId, $cardId, $vCard->serialize());
}
} else {
$vCard = $converter->createCardFromUser($user);
$vCard = $this->converter->createCardFromUser($user);
if (is_null($vCard)) {
$this->backend->deleteCard($addressBookId, $cardId);
} else {
Expand Down
89 changes: 45 additions & 44 deletions apps/dav/tests/unit/CardDAV/ConverterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,65 +27,66 @@

namespace OCA\DAV\Tests\unit\CardDAV;

use OC\Accounts\AccountManager;
use OCA\DAV\CardDAV\Converter;
use OCP\Accounts\IAccount;
use OCP\Accounts\IAccountManager;
use OCP\Accounts\IAccountProperty;
use OCP\IImage;
use OCP\IUser;
use PHPUnit\Framework\MockObject\MockObject;
use Test\TestCase;

class ConverterTest extends TestCase {

/** @var AccountManager|\PHPUnit\Framework\MockObject\MockObject */
/** @var IAccountManager|\PHPUnit\Framework\MockObject\MockObject */
private $accountManager;

protected function setUp(): void {
parent::setUp();

$this->accountManager = $this->createMock(AccountManager::class);
$this->accountManager = $this->createMock(IAccountManager::class);
}

/**
* @return IAccountProperty|MockObject
*/
protected function getAccountPropertyMock(string $name, ?string $value, string $scope) {
$property = $this->createMock(IAccountProperty::class);
$property->expects($this->any())
->method('getName')
->willReturn($name);
$property->expects($this->any())
->method('getValue')
->willReturn((string)$value);
$property->expects($this->any())
->method('getScope')
->willReturn($scope);
$property->expects($this->any())
->method('getVerified')
->willReturn(IAccountManager::NOT_VERIFIED);
return $property;
}

public function getAccountManager(IUser $user) {
$accountManager = $this->getMockBuilder(AccountManager::class)
$account = $this->createMock(IAccount::class);
$account->expects($this->any())
->method('getProperties')
->willReturnCallback(function () use ($user) {
return [
$this->getAccountPropertyMock(IAccountManager::PROPERTY_DISPLAYNAME, $user->getDisplayName(), IAccountManager::SCOPE_FEDERATED),
$this->getAccountPropertyMock(IAccountManager::PROPERTY_ADDRESS, '', IAccountManager::SCOPE_LOCAL),
$this->getAccountPropertyMock(IAccountManager::PROPERTY_WEBSITE, '', IAccountManager::SCOPE_LOCAL),
$this->getAccountPropertyMock(IAccountManager::PROPERTY_EMAIL, $user->getEMailAddress(), IAccountManager::SCOPE_FEDERATED),
$this->getAccountPropertyMock(IAccountManager::PROPERTY_AVATAR, $user->getAvatarImage(-1)->data(), IAccountManager::SCOPE_FEDERATED),
$this->getAccountPropertyMock(IAccountManager::PROPERTY_PHONE, '', IAccountManager::SCOPE_LOCAL),
$this->getAccountPropertyMock(IAccountManager::PROPERTY_TWITTER, '', IAccountManager::SCOPE_LOCAL),
];
});

$accountManager = $this->getMockBuilder(IAccountManager::class)
->disableOriginalConstructor()->getMock();
$accountManager->expects($this->any())->method('getUser')->willReturn(
[
IAccountManager::PROPERTY_DISPLAYNAME =>
[
'value' => $user->getDisplayName(),
'scope' => AccountManager::SCOPE_FEDERATED,
],
IAccountManager::PROPERTY_ADDRESS =>
[
'value' => '',
'scope' => AccountManager::SCOPE_LOCAL,
],
IAccountManager::PROPERTY_WEBSITE =>
[
'value' => '',
'scope' => AccountManager::SCOPE_LOCAL,
],
IAccountManager::PROPERTY_EMAIL =>
[
'value' => $user->getEMailAddress(),
'scope' => AccountManager::SCOPE_FEDERATED,
],
IAccountManager::PROPERTY_AVATAR =>
[
'scope' => AccountManager::SCOPE_FEDERATED
],
IAccountManager::PROPERTY_PHONE =>
[
'value' => '',
'scope' => AccountManager::SCOPE_LOCAL,
],
IAccountManager::PROPERTY_TWITTER =>
[
'value' => '',
'scope' => AccountManager::SCOPE_LOCAL,
],
]
);

$accountManager->expects($this->any())->method('getAccount')->willReturn($account);

return $accountManager;
}
Expand All @@ -94,7 +95,7 @@ public function getAccountManager(IUser $user) {
* @dataProvider providesNewUsers
*/
public function testCreation($expectedVCard, $displayName = null, $eMailAddress = null, $cloudId = null) {
$user = $this->getUserMock($displayName, $eMailAddress, $cloudId);
$user = $this->getUserMock((string)$displayName, $eMailAddress, $cloudId);
$accountManager = $this->getAccountManager($user);

$converter = new Converter($accountManager);
Expand Down Expand Up @@ -203,7 +204,7 @@ public function providesNames() {
* @param $cloudId
* @return IUser | \PHPUnit\Framework\MockObject\MockObject
*/
protected function getUserMock($displayName, $eMailAddress, $cloudId) {
protected function getUserMock(string $displayName, ?string $eMailAddress, ?string $cloudId) {
$image0 = $this->getMockBuilder(IImage::class)->disableOriginalConstructor()->getMock();
$image0->method('mimeType')->willReturn('image/jpeg');
$image0->method('data')->willReturn('123456789');
Expand Down
58 changes: 13 additions & 45 deletions apps/dav/tests/unit/CardDAV/SyncServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,13 @@

use OC\Accounts\AccountManager;
use OCA\DAV\CardDAV\CardDavBackend;
use OCA\DAV\CardDAV\Converter;
use OCA\DAV\CardDAV\SyncService;
use OCP\Accounts\IAccountManager;
use OCP\ILogger;
use OCP\IUser;
use OCP\IUserManager;
use Sabre\VObject\Component\VCard;
use Test\TestCase;

class SyncServiceTest extends TestCase {
Expand Down Expand Up @@ -82,8 +84,9 @@ public function testEnsureSystemAddressBookExists() {
/** @var IUserManager $userManager */
$userManager = $this->getMockBuilder(IUserManager::class)->disableOriginalConstructor()->getMock();
$logger = $this->getMockBuilder(ILogger::class)->disableOriginalConstructor()->getMock();
$accountManager = $this->getMockBuilder(AccountManager::class)->disableOriginalConstructor()->getMock();
$ss = new SyncService($backend, $userManager, $logger, $accountManager);
$converter = $this->createMock(Converter::class);

$ss = new SyncService($backend, $userManager, $logger, $converter);
$ss->ensureSystemAddressBookExists('principals/users/adam', 'contacts', []);
}

Expand Down Expand Up @@ -130,47 +133,12 @@ public function testUpdateAndDeleteUser($activated, $createCalls, $updateCalls,
$user->method('getCloudId')->willReturn('cloudId');
$user->method('getDisplayName')->willReturn('test-user');
$user->method('isEnabled')->willReturn($activated);
$accountManager = $this->getMockBuilder(AccountManager::class)->disableOriginalConstructor()->getMock();
$accountManager->expects($this->any())->method('getUser')
->willReturn([
IAccountManager::PROPERTY_DISPLAYNAME =>
[
'value' => $user->getDisplayName(),
'scope' => AccountManager::SCOPE_FEDERATED,
],
IAccountManager::PROPERTY_ADDRESS =>
[
'value' => '',
'scope' => AccountManager::SCOPE_LOCAL,
],
IAccountManager::PROPERTY_WEBSITE =>
[
'value' => '',
'scope' => AccountManager::SCOPE_LOCAL,
],
IAccountManager::PROPERTY_EMAIL =>
[
'value' => $user->getEMailAddress(),
'scope' => AccountManager::SCOPE_FEDERATED,
],
IAccountManager::PROPERTY_AVATAR =>
[
'scope' => AccountManager::SCOPE_FEDERATED
],
IAccountManager::PROPERTY_PHONE =>
[
'value' => '',
'scope' => AccountManager::SCOPE_LOCAL,
],
IAccountManager::PROPERTY_TWITTER =>
[
'value' => '',
'scope' => AccountManager::SCOPE_LOCAL,
],
]
);

$ss = new SyncService($backend, $userManager, $logger, $accountManager);
$converter = $this->createMock(Converter::class);
$converter->expects($this->any())
->method('createCardFromUser')
->willReturn($this->createMock(VCard::class));

$ss = new SyncService($backend, $userManager, $logger, $converter);
$ss->updateUser($user);

$ss->updateUser($user);
Expand Down Expand Up @@ -202,11 +170,11 @@ private function getBackendMock($createCount, $updateCount, $deleteCount) {
private function getSyncServiceMock($backend, $response) {
$userManager = $this->getMockBuilder(IUserManager::class)->disableOriginalConstructor()->getMock();
$logger = $this->getMockBuilder(ILogger::class)->disableOriginalConstructor()->getMock();
$accountManager = $this->getMockBuilder(AccountManager::class)->disableOriginalConstructor()->getMock();
$converter = $this->createMock(Converter::class);
/** @var SyncService | \PHPUnit\Framework\MockObject\MockObject $ss */
$ss = $this->getMockBuilder(SyncService::class)
->setMethods(['ensureSystemAddressBookExists', 'requestSyncReport', 'download', 'getCertPath'])
->setConstructorArgs([$backend, $userManager, $logger, $accountManager])
->setConstructorArgs([$backend, $userManager, $logger, $converter])
->getMock();
$ss->method('requestSyncReport')->withAnyParameters()->willReturn(['response' => $response, 'token' => 'sync-token-1']);
$ss->method('ensureSystemAddressBookExists')->willReturn(['id' => 1]);
Expand Down