[SebastianKrupinski]

Summary

• Resolves: # Client Ticket
• Related: fix: remove wrongly used system address book flag #60307

• added logic to the system address book to only enumerate users from a users group

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)
• Labels added where applicable (ex: bug/enhancement, 3. to review, feature component)
• Milestone added for target branch/version (ex: 32.x for stable32)

AI (if applicable)

• The content of this PR was partly or fully generated using AI

[SebastianKrupinski]

/backport to stable34

[SebastianKrupinski]

/backport to stable33

[SebastianKrupinski]

/backport to stable32

[salmart-dev]

question: the logic here and a similar precedent one in ContactsController is applying the group exclusion in a completely different way than the setting itself advertises: in all other places groups are removed from the user's group list so that the checks about group membership only act on groups that are not in the exclude list.
I guess a common use-case is to have "everyone" groups, but exclude them from counting when checking for group membership and keep the filtering working. With the logic above and the configuration I mentioned, everyone would see everyone else.

Is this the desired outcome?

[SebastianKrupinski]

Hey! Sorry, just saw your comment.

You are correct, I copied the logic from calendar, and it was broken in calendar.

Fixing now

[DerDreschner]

/backport to stable35

[SebastianKrupinski]

@ChristophWurst @kesselb @hamza221 +1 please

[kesselb]

Looking at #63392 with #65812 in mind, I think we should extract the enumeration settings to a own helper in this PR.

class SystemAddressbookAccess {
	public function __construct(
		private IShareManager $shareManager,
		private IGroupManager $groupManager,
	) {
	}

	/** @return list<string>|null group IDs whose members $viewer may see, [] for own card only, null if unrestricted */
	private function getVisibleGroupIds(IUser $viewer): ?array {
		if ($viewer->getBackendClassName() === 'Guests'
			|| !$this->shareManager->allowEnumeration()
			|| ($this->shareManager->limitEnumerationToPhone() && !$this->shareManager->limitEnumerationToGroups())) {
			return [];
		}
		if (!$this->shareManager->limitEnumerationToGroups() && !$this->shareManager->shareWithGroupMembersOnly()) {
			return null;
		}
		return array_values(array_diff(
			$this->groupManager->getUserGroupIds($viewer),
			$this->shareManager->shareWithGroupMembersOnlyExcludeGroupsList(),
		));
	}

	public function canSee(IUser $viewer, IUser $target): bool { /* own → true, guest target → false, null → true, else group intersect */ }

	/** @return list<string>|null */
	public function getVisibleCardUris(IUser $viewer): ?array { /* own + non-guest members of getVisibleGroupIds() */ }
}

[kesselb]

Federation

• Trusted-server sync breaks with "only share with group members" (SystemAddressbook.php:117, :166)
  Trusted servers authenticate through FedAuth (apps/federation/lib/DAV/FedAuth.php), which sets no session user. The new || $restrictToOwnGroups branch runs before isFederation(). With a null user it throws Forbidden in getChild() and returns [] in getMultipleChildren(). getChanges() then moves every added or modified URI to deleted, so the remote server drops all federated contacts.
  The enumeration restrictions already behave like this, but this setting is far more common. Suggestion: handle the federation request before the restriction branches, and add a test for federation with the setting enabled.

Config

• Use the IShareManager helpers instead of reading the keys (SystemAddressbook.php:61-64, :100-103, :155-158, :276)
  IShareManager already has allowEnumeration(), limitEnumerationToGroups(), limitEnumerationToPhone(), shareWithGroupMembersOnly() and shareWithGroupMembersOnlyExcludeGroupsList(). The last one already returns [] when the restriction is off, so the $ignoreExcludedGroups parameter isn't needed.
  This also avoids an AppConfigTypeConflictException: getValueBool()/getValueString() throw if a key was stored with a different explicit type (e.g. occ config:app:set ... --type=string), and the system address book would return 500. These keys aren't in the core lexicon, UserPlugin reads them as strings, and the share manager reads them through getAppValue() without type checks.

Performance

• getChild() loads every member of every group to check one card (SystemAddressbook.php:171)
  The old loop returned on the first match. Now a user in a 20k-member group causes 20k IUser objects and getCardUri() calls on every single-card GET. Resolving the target from the URI (Backend:uid.vcf) and intersecting getUserGroupIds(), as ContactsStore does, costs O(groups) instead.
• getMultipleChildren() enumerates all group members to keep a few paths (SystemAddressbook.php:122)
  A multiget for 5 cards loads every member of every group the user is in. Checking only the requested paths makes the cost depend on the request size instead of the group size.

Structure

• Rule reads copied into three methods (SystemAddressbook.php:61, :100, :155)
  The same four reads appear in getChildren(), getMultipleChildren() and getChild(), and every test re-mocks exactly(4) calls. One decision method would remove the triplication. fix(contactsinteraction): skip recent contacts visible in system addr… #65812 needs the same rule in contactsinteraction, so this could become a small service in dav that both PRs use:
  • canSee(IUser $viewer, IUser $target): bool for getChild(), multiget and fix(contactsinteraction): skip recent contacts visible in system addr… #65812
  • getVisibleCardUris(IUser $viewer): ?array for getChildren() only
• Naming of getCardsForUsersGroups() (SystemAddressbook.php:273)
  $ignoreExcludedGroups = true actually turns the exclude list on, so it reads backwards. The method also returns card URIs, not cards.

Housekeeping

• Commit subject has no scope
  fix: system address book card restriction could be fix(dav): restrict system address book to own groups.