Skip to content

fix: system address book card restriction #63392

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
SebastianKrupinski wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: system address book card restriction #63392

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
104 changes: 55 additions & 49 deletions apps/dav/lib/CardDAV/SystemAddressbook.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,11 @@

use OCA\Federation\TrustedServers;
use OCP\Accounts\IAccountManager;
use OCP\IConfig;
use OCP\IAppConfig;
use OCP\IGroupManager;
use OCP\IL10N;
use OCP\IRequest;
use OCP\IUser;
use OCP\IUserSession;
use Sabre\CardDAV\Backend\BackendInterface;
use Sabre\CardDAV\Backend\SyncSupport;
Expand All @@ -35,7 +36,7 @@ public function __construct(
BackendInterface $carddavBackend,
array $addressBookInfo,
IL10N $l10n,
private IConfig $config,
private IAppConfig $appConfig,
private IUserSession $userSession,
private ?IRequest $request = null,
private ?TrustedServers $trustedServers = null,
Expand All @@ -53,12 +54,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->appConfig->getValueBool('core', 'shareapi_allow_share_dialog_user_enumeration', true);
$shareEnumerationGroup = $this->appConfig->getValueBool('core', 'shareapi_restrict_user_enumeration_to_group');
$shareEnumerationPhone = $this->appConfig->getValueBool('core', 'shareapi_restrict_user_enumeration_to_phone');
$restrictToOwnGroups = $this->appConfig->getValueBool('core', 'shareapi_only_share_with_group_members');
$user = $this->userSession->getUser();
if (!$user) {
// Should never happen because we don't allow anonymous access
Expand All @@ -72,23 +75,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, $restrictToOwnGroups));
}

$children = parent::getChildren();
Expand All @@ -105,9 +97,10 @@ 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';
$shareEnumeration = $this->appConfig->getValueBool('core', 'shareapi_allow_share_dialog_user_enumeration', true);
$shareEnumerationGroup = $this->appConfig->getValueBool('core', 'shareapi_restrict_user_enumeration_to_group');
$shareEnumerationPhone = $this->appConfig->getValueBool('core', 'shareapi_restrict_user_enumeration_to_phone');
$restrictToOwnGroups = $this->appConfig->getValueBool('core', 'shareapi_only_share_with_group_members');
$user = $this->userSession->getUser();
if (($user !== null && $user->getBackendClassName() === 'Guests') || !$shareEnumeration || (!$shareEnumerationGroup && $shareEnumerationPhone)) {
// No user or cards with no access
Expand All @@ -121,23 +114,12 @@ 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));
return parent::getMultipleChildren(array_intersect($paths, $this->getCardsForUsersGroups($user, $restrictToOwnGroups)));
}
if (!$this->isFederation()) {
return parent::getMultipleChildren($paths);
Expand Down Expand Up @@ -170,32 +152,24 @@ public function getMultipleChildren($paths): array {
#[\Override]
public function getChild($name): Card {
$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->appConfig->getValueBool('core', 'shareapi_allow_share_dialog_user_enumeration', true);
$shareEnumerationGroup = $this->appConfig->getValueBool('core', 'shareapi_restrict_user_enumeration_to_group');
$shareEnumerationPhone = $this->appConfig->getValueBool('core', 'shareapi_restrict_user_enumeration_to_phone');
$restrictToOwnGroups = $this->appConfig->getValueBool('core', 'shareapi_only_share_with_group_members');
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 (in_array($name, $this->getCardsForUsersGroups($user, $restrictToOwnGroups), true)) {
return parent::getChild($name);
}
throw new Forbidden();
}
Expand Down Expand Up @@ -290,6 +264,38 @@ private function isFederation(): bool {
return true;
}

/**
* Groups listed in 'shareapi_only_share_with_group_members_exclude_group_list'
* are not considered when $ignoreExcludedGroups is set.
*
* @return string[] card URIs of $user and of non-guest users sharing at least one group with $user
*/
private function getCardsForUsersGroups(IUser $user, bool $ignoreExcludedGroups): array {
$excludedGroupIds = [];
if ($ignoreExcludedGroups) {
$excludedGroupIds = json_decode(
$this->appConfig->getValueString('core', 'shareapi_only_share_with_group_members_exclude_group_list', '[]'),
true
);
if (!is_array($excludedGroupIds)) {
$excludedGroupIds = [];
}
}
$names = [SyncService::getCardUri($user)];
foreach ($this->groupManager->getUserGroups($user) as $group) {
if (in_array($group->getGID(), $excludedGroupIds, true)) {
continue;
}
foreach ($group->getUsers() as $groupUser) {
if ($groupUser->getBackendClassName() === 'Guests') {
continue;
}
$names[] = SyncService::getCardUri($groupUser);
}
}
return array_values(array_unique($names));
}

/**
* 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
5 changes: 1 addition & 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 @@ -17,7 +17,6 @@
use OCA\DAV\ConfigLexicon;
use OCA\Federation\TrustedServers;
use OCP\IAppConfig;
use OCP\IConfig;
use OCP\IGroupManager;
use OCP\IL10N;
use OCP\IRequest;
Expand All @@ -35,7 +34,6 @@

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

public function __construct(
Expand All @@ -48,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 @@ -92,7 +89,7 @@ public function getChildren() {
$this->carddavBackend,
$addressBook,
$this->l10n,
$this->config,
$this->appConfig,
Server::get(IUserSession::class),
$request,
$trustedServers,
Expand Down
Loading
Loading