Skip to content

[stable34] fix: system address book card restriction #66530

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

Open
backportbot wants to merge 1 commit into
base: stable34
Choose a base branch
from
Open

[stable34] fix: system address book card restriction #66530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
237 changes: 152 additions & 85 deletions apps/dav/lib/CardDAV/SystemAddressbook.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,13 @@

use OCA\Federation\TrustedServers;
use OCP\Accounts\IAccountManager;
use OCP\IConfig;
use OCP\IGroup;
use OCP\IGroupManager;
use OCP\IL10N;
use OCP\IRequest;
use OCP\IUser;
use OCP\IUserSession;
use OCP\Share\IManager as IShareManager;
use Sabre\CardDAV\Backend\BackendInterface;
use Sabre\CardDAV\Backend\SyncSupport;
use Sabre\CardDAV\Card;
Expand All @@ -23,7 +25,6 @@
use Sabre\VObject\Component\VCard;
use Sabre\VObject\Reader;
use function array_filter;
use function array_intersect;
use function array_unique;
use function in_array;

Expand All @@ -34,7 +35,7 @@ public function __construct(
BackendInterface $carddavBackend,
array $addressBookInfo,
IL10N $l10n,
private IConfig $config,
private IShareManager $shareManager,
private IUserSession $userSession,
private ?IRequest $request = null,
private ?TrustedServers $trustedServers = null,
Expand All @@ -52,12 +53,14 @@ public function __construct(
* 'Allow username autocompletion in share dialog' + 'Allow username autocompletion to users within the same groups' -> show only users in intersecting groups
* 'Allow username autocompletion in share dialog' + 'Allow username autocompletion to users based on phone number integration' -> show only the same user
* 'Allow username autocompletion in share dialog' + 'Allow username autocompletion to users within the same groups' + 'Allow username autocompletion to users based on phone number integration' -> show only users in intersecting groups
* 'Restrict users to only share with users in their groups' -> show only users in intersecting groups, not counting the groups excluded from that check
*/
#[\Override]
public function getChildren() {
$shareEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes';
$shareEnumerationGroup = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes';
$shareEnumerationPhone = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_phone', 'no') === 'yes';
$shareEnumeration = $this->shareManager->allowEnumeration();
$shareEnumerationGroup = $this->shareManager->limitEnumerationToGroups();
$shareEnumerationPhone = $this->shareManager->limitEnumerationToPhone();
$restrictToOwnGroups = $this->shareManager->shareWithGroupMembersOnly();
$user = $this->userSession->getUser();
if (!$user) {
// Should never happen because we don't allow anonymous access
Expand All @@ -71,23 +74,12 @@ public function getChildren() {
return [];
}
}
if ($shareEnumerationGroup) {
if ($shareEnumerationGroup || $restrictToOwnGroups) {
if ($this->groupManager === null) {
// Group manager is not available, so we can't determine which data is safe
return [];
}
$groups = $this->groupManager->getUserGroups($user);
$names = [];
foreach ($groups as $group) {
$users = $group->getUsers();
foreach ($users as $groupUser) {
if ($groupUser->getBackendClassName() === 'Guests') {
continue;
}
$names[] = SyncService::getCardUri($groupUser);
}
}
return parent::getMultipleChildren(array_unique($names));
return parent::getMultipleChildren($this->getCardsForUsersGroups($user));
}

$children = parent::getChildren();
Expand All @@ -104,9 +96,33 @@ public function getChildren() {
*/
#[\Override]
public function getMultipleChildren($paths): array {
$shareEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes';
$shareEnumerationGroup = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes';
$shareEnumerationPhone = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_phone', 'no') === 'yes';
// Trusted servers have no session user, so they get no cards while group restrictions apply and bypass the others
if ($this->isFederation()) {
if ($this->shareManager->limitEnumerationToGroups() || $this->shareManager->shareWithGroupMembersOnly()) {
return [];
}
$objs = $this->carddavBackend->getMultipleCards($this->addressBookInfo['id'], $paths);
$children = [];
/** @var array $obj */
foreach ($objs as $obj) {
if (empty($obj)) {
continue;
}
$carddata = $this->extractCarddata($obj);
if (empty($carddata)) {
continue;
} else {
$obj['carddata'] = $carddata;
}
$children[] = new Card($this->carddavBackend, $this->addressBookInfo, $obj);
}
return $children;
}

$shareEnumeration = $this->shareManager->allowEnumeration();
$shareEnumerationGroup = $this->shareManager->limitEnumerationToGroups();
$shareEnumerationPhone = $this->shareManager->limitEnumerationToPhone();
$restrictToOwnGroups = $this->shareManager->shareWithGroupMembersOnly();
$user = $this->userSession->getUser();
if (($user !== null && $user->getBackendClassName() === 'Guests') || !$shareEnumeration || (!$shareEnumerationGroup && $shareEnumerationPhone)) {
// No user or cards with no access
Expand All @@ -120,44 +136,14 @@ public function getMultipleChildren($paths): array {
return [];
}
}
if ($shareEnumerationGroup) {
if ($shareEnumerationGroup || $restrictToOwnGroups) {
if ($this->groupManager === null || $user === null) {
// Group manager or user is not available, so we can't determine which data is safe
return [];
}
$groups = $this->groupManager->getUserGroups($user);
$allowedNames = [];
foreach ($groups as $group) {
$users = $group->getUsers();
foreach ($users as $groupUser) {
if ($groupUser->getBackendClassName() === 'Guests') {
continue;
}
$allowedNames[] = SyncService::getCardUri($groupUser);
}
}
return parent::getMultipleChildren(array_intersect($paths, $allowedNames));
}
if (!$this->isFederation()) {
return parent::getMultipleChildren($paths);
}

$objs = $this->carddavBackend->getMultipleCards($this->addressBookInfo['id'], $paths);
$children = [];
/** @var array $obj */
foreach ($objs as $obj) {
if (empty($obj)) {
continue;
}
$carddata = $this->extractCarddata($obj);
if (empty($carddata)) {
continue;
} else {
$obj['carddata'] = $carddata;
}
$children[] = new Card($this->carddavBackend, $this->addressBookInfo, $obj);
return parent::getMultipleChildren($this->filterAccessibleCardUris($user, $paths));
}
return $children;
return parent::getMultipleChildren($paths);
}

/**
Expand All @@ -168,51 +154,47 @@ public function getMultipleChildren($paths): array {
*/
#[\Override]
public function getChild($name): Card {
// Trusted servers have no session user, so they get no cards while group restrictions apply and bypass the others
if ($this->isFederation()) {
if ($this->shareManager->limitEnumerationToGroups() || $this->shareManager->shareWithGroupMembersOnly()) {
throw new Forbidden();
}
$obj = $this->carddavBackend->getCard($this->addressBookInfo['id'], $name);
if (!$obj) {
throw new NotFound('Card not found');
}
$carddata = $this->extractCarddata($obj);
if (empty($carddata)) {
throw new Forbidden();
} else {
$obj['carddata'] = $carddata;
}
return new Card($this->carddavBackend, $this->addressBookInfo, $obj);
}

$user = $this->userSession->getUser();
$shareEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes';
$shareEnumerationGroup = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes';
$shareEnumerationPhone = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_phone', 'no') === 'yes';
$shareEnumeration = $this->shareManager->allowEnumeration();
$shareEnumerationGroup = $this->shareManager->limitEnumerationToGroups();
$shareEnumerationPhone = $this->shareManager->limitEnumerationToPhone();
$restrictToOwnGroups = $this->shareManager->shareWithGroupMembersOnly();
if (($user !== null && $user->getBackendClassName() === 'Guests') || !$shareEnumeration || (!$shareEnumerationGroup && $shareEnumerationPhone)) {
$ownName = $user !== null ? SyncService::getCardUri($user) : null;
if ($ownName === $name) {
return parent::getChild($name);
}
throw new Forbidden();
}
if ($shareEnumerationGroup) {
if ($shareEnumerationGroup || $restrictToOwnGroups) {
if ($user === null || $this->groupManager === null) {
// Group manager is not available, so we can't determine which data is safe
throw new Forbidden();
}
$groups = $this->groupManager->getUserGroups($user);
foreach ($groups as $group) {
foreach ($group->getUsers() as $groupUser) {
if ($groupUser->getBackendClassName() === 'Guests') {
continue;
}
$otherName = SyncService::getCardUri($groupUser);
if ($otherName === $name) {
return parent::getChild($name);
}
}
if ($this->isCardAccessible($user, $name)) {
return parent::getChild($name);
}
throw new Forbidden();
}
if (!$this->isFederation()) {
return parent::getChild($name);
}

$obj = $this->carddavBackend->getCard($this->addressBookInfo['id'], $name);
if (!$obj) {
throw new NotFound('Card not found');
}
$carddata = $this->extractCarddata($obj);
if (empty($carddata)) {
throw new Forbidden();
} else {
$obj['carddata'] = $carddata;
}
return new Card($this->carddavBackend, $this->addressBookInfo, $obj);
return parent::getChild($name);
}
#[\Override]
public function getChanges($syncToken, $syncLevel, $limit = null) {
Expand Down Expand Up @@ -289,6 +271,91 @@ private function isFederation(): bool {
return true;
}

/**
* Groups excluded from the 'only share with group members' restriction are not considered.
*
* @return IGroup[] groups of $user whose members' cards $user can access
*/
private function getCardAccessGroups(IUser $user): array {
$excludedGroupIds = $this->shareManager->shareWithGroupMembersOnlyExcludeGroupsList();
$groups = [];
foreach ($this->groupManager->getUserGroups($user) as $group) {
if (!in_array($group->getGID(), $excludedGroupIds, true)) {
$groups[] = $group;
}
}
return $groups;
}

/**
* @return string[] card URIs of $user and of non-guest users sharing at least one group with $user
*/
private function getCardsForUsersGroups(IUser $user): array {
$names = [SyncService::getCardUri($user)];
foreach ($this->getCardAccessGroups($user) as $group) {
foreach ($group->getUsers() as $groupUser) {
if ($groupUser->getBackendClassName() === 'Guests') {
continue;
}
$names[] = SyncService::getCardUri($groupUser);
}
}
return array_values(array_unique($names));
}

private function isCardAccessible(IUser $user, string $cardUri): bool {
if ($cardUri === SyncService::getCardUri($user)) {
return true;
}
$uid = $this->extractUserId($cardUri);
if ($uid === null) {
return false;
}
foreach ($this->getCardAccessGroups($user) as $group) {
if ($this->groupManager->isInGroup($uid, $group->getGID())) {
return true;
}
}
return false;
}

/**
* Lists group members once, so the number of queries depends on neither the
* number of requested cards nor the size of the groups.
*
* @param string[] $cardUris
* @return string[] the card URIs $user can access
*/
private function filterAccessibleCardUris(IUser $user, array $cardUris): array {
$members = [];
foreach ($this->getCardAccessGroups($user) as $group) {
$members += $group->searchUsers('');
}
$ownCardUri = SyncService::getCardUri($user);
$accessibleCardUris = [];
foreach ($cardUris as $cardUri) {
$uid = $this->extractUserId($cardUri);
if ($cardUri === $ownCardUri || ($uid !== null && isset($members[$uid]))) {
$accessibleCardUris[] = $cardUri;
}
}
return $accessibleCardUris;
}

/**
* Reads the user id from a card URI as built by {@see SyncService::getCardUri()}.
* Only the URI is used, because loading the user would cost a lookup per card.
*
* @return string|null the user id, or null for guests and other URIs
*/
private function extractUserId(string $cardUri): ?string {
$separator = strpos($cardUri, ':');
if ($separator === false || !str_ends_with($cardUri, '.vcf') || str_starts_with($cardUri, 'Guests:')) {
return null;
}
return substr($cardUri, $separator + 1, -strlen('.vcf'));
}

/**
* If the validation doesn't work the card is "not found" so we
* return empty carddata even if the carddata might exist in the local backend.
Expand Down
6 changes: 2 additions & 4 deletions apps/dav/lib/CardDAV/UserAddressBooks.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,13 @@
use OCA\DAV\ConfigLexicon;
use OCA\Federation\TrustedServers;
use OCP\IAppConfig;
use OCP\IConfig;
use OCP\IGroupManager;
use OCP\IL10N;
use OCP\IRequest;
use OCP\IUser;
use OCP\IUserSession;
use OCP\Server;
use OCP\Share\IManager as IShareManager;
use OCP\Util;
use Psr\Container\ContainerExceptionInterface;
use Psr\Container\NotFoundExceptionInterface;
Expand All @@ -34,7 +34,6 @@

class UserAddressBooks extends \Sabre\CardDAV\AddressBookHome {
protected IL10N $l10n;
protected IConfig $config;
protected IAppConfig $appConfig;

public function __construct(
Expand All @@ -47,7 +46,6 @@ public function __construct(
parent::__construct($carddavBackend, $principalUri);

$this->l10n = Util::getL10N('dav');
$this->config = Server::get(IConfig::class);
$this->appConfig = Server::get(IAppConfig::class);
}

Expand Down Expand Up @@ -91,7 +89,7 @@ public function getChildren() {
$this->carddavBackend,
$addressBook,
$this->l10n,
$this->config,
Server::get(IShareManager::class),
Server::get(IUserSession::class),
$request,
$trustedServers,
Expand Down
Loading
Loading