From 3a11d68dd800500d9d687c0e79b1851fedd5482a Mon Sep 17 00:00:00 2001 From: Stefan Lender Date: Fri, 9 Oct 2026 23:50:31 +0200 Subject: [PATCH 1/2] feat(teams): add Team activity stream Assisted-by: GitHubCopilot:GPT-5.6-Terra Signed-off-by: Stefan Lender --- appinfo/routes.php | 1 + docs/endpoint-v2.md | 29 +- lib/Controller/APIv2Controller.php | 69 +++-- lib/Data.php | 225 +++++++++++++++- lib/TeamActivityResolver.php | 151 +++++++++++ lib/TeamActivityScope.php | 41 +++ tests/Controller/APIv2ControllerTest.php | 37 +++ tests/Controller/TeamActivityResponseTest.php | 63 +++++ tests/DataTest.php | 68 +++++ tests/TeamActivityDataTest.php | 184 +++++++++++++ tests/TeamActivityResolverTest.php | 251 ++++++++++++++++++ 11 files changed, 1099 insertions(+), 20 deletions(-) create mode 100644 lib/TeamActivityResolver.php create mode 100644 lib/TeamActivityScope.php create mode 100644 tests/Controller/TeamActivityResponseTest.php create mode 100644 tests/TeamActivityDataTest.php create mode 100644 tests/TeamActivityResolverTest.php diff --git a/appinfo/routes.php b/appinfo/routes.php index ba8fb9864..12ece2217 100644 --- a/appinfo/routes.php +++ b/appinfo/routes.php @@ -14,6 +14,7 @@ ['name' => 'APIv2#getDownloadCount', 'url' => '/api/v2/activity/downloads/count', 'verb' => 'GET'], // Before the catch-all below, and a sub-path of it so `{filter}` cannot // swallow it either way + ['name' => 'APIv2#getTeam', 'url' => '/api/v2/activity/team/{teamId}', 'verb' => 'GET'], ['name' => 'APIv2#getHistogram', 'url' => '/api/v2/activity/{filter}/histogram', 'verb' => 'GET'], ['name' => 'APIv2#getFilter', 'url' => '/api/v2/activity/{filter}', 'verb' => 'GET'], ], diff --git a/docs/endpoint-v2.md b/docs/endpoint-v2.md index 2fd6c2b43..aa1e9755e 100644 --- a/docs/endpoint-v2.md +++ b/docs/endpoint-v2.md @@ -32,6 +32,31 @@ With type filter GET /ocs/v2.php/apps/activity/api/v2/activity/{filter} ``` +With a Team's roster and resource activity + +``` +GET /ocs/v2.php/apps/activity/api/v2/activity/team/{teamId} +``` + +The Team endpoint is available only to current Team members. It accepts the +same pagination, ordering, search, date, and actor parameters as the default +stream. It includes roster activity from before the current member's join date. +File activity is limited to the Team's current `files` resources that are +readable through the member's own mounts and ACLs, including older events for +those files. A file resource is included directly; a folder resource includes +its full readable subtree. Deleted files, files no longer reachable through any +Team resource, and nested external mounts are not part of this scope. + +Other Team resource providers can opt in to the stream by resolving their +resources into Activity object scopes. Activity then filters on those object +types and IDs without inferring scope from the Team's member roster. A resource +may be shared with other Teams or accounts; it only needs to be currently +reachable through this Team's resources. + +Recipient copies are normalized to the viewer's file paths and deduplicated +before pagination. An empty Team page returns `200 OK`; a matching ETag returns +`304 Not Modified`. The existing personal stream retains its empty-page `304`. + Supported type filters can be obtained from: ``` @@ -93,11 +118,11 @@ Status Code | Description ----------- | ----------- `200 OK` | Activities `204 No Content` | The user has selected no activities to be listed in the stream -`304 Not Modified` | ETag/If-None-Match are the same or the end of the activity list was reached +`304 Not Modified` | ETag/If-None-Match are the same, or the personal stream is empty `400 Bad Request` | The search term is too short or too long, or `from` is after `to` `403 Forbidden` | The offset activity belongs to a different user `403 Forbidden` | The user is not logged in -`404 Not Found` | The filter is unknown +`404 Not Found` | The filter is unknown, or the Team does not exist or the user is not one of its members ## Headers diff --git a/lib/Controller/APIv2Controller.php b/lib/Controller/APIv2Controller.php index a584ddba4..3fc3239b6 100644 --- a/lib/Controller/APIv2Controller.php +++ b/lib/Controller/APIv2Controller.php @@ -14,6 +14,8 @@ use OCA\Activity\Exception\InvalidSearchCriteriaException; use OCA\Activity\GroupHelper; use OCA\Activity\SearchCriteria; +use OCA\Activity\TeamActivityResolver; +use OCA\Activity\TeamActivityScope; use OCA\Activity\UserSettings; use OCA\Activity\ViewInfoCache; use OCP\Activity\IFilter; @@ -70,6 +72,7 @@ public function __construct( protected ViewInfoCache $infoCache, protected INotificationManager $notificationManager, protected IDateTimeZone $dateTimeZone, + protected TeamActivityResolver $teamActivityResolver, ) { parent::__construct($appName, $request); $this->activityManager = $activityManager; @@ -131,6 +134,27 @@ public function getFilter(string $filter, int $since = 0, int $limit = 50, bool return $this->get($filter, $since, $limit, $previews, $object_type, $object_id, $sort, $search, $from, $to, $actor); } + /** + * @param string $search Only return activities whose file path contains this substring + * @param int $from Only return activities at or after this Unix timestamp + * @param int $to Only return activities at or before this Unix timestamp + * @param string $actor Only return activities authored by this account + */ + #[NoAdminRequired] + public function getTeam(string $teamId, int $since = 0, int $limit = 50, bool $previews = false, string $sort = 'desc', string $search = '', int $from = 0, int $to = 0, string $actor = ''): DataResponse { + $user = $this->userSession->getUser(); + if (!$user instanceof IUser) { + return new DataResponse([], Http::STATUS_FORBIDDEN); + } + + $scope = $this->teamActivityResolver->getScope($teamId, $user->getUID()); + if ($scope === null) { + return new DataResponse([], Http::STATUS_NOT_FOUND); + } + + return $this->get('all', $since, $limit, $previews, '', 0, $sort, $search, $from, $to, $actor, $scope); + } + /** * @param string $object_type Object type to count downloads for (must be 'files') * @param int $object_id File ID @@ -250,7 +274,7 @@ public function listFilters(): DataResponse { return new DataResponse($filters); } - protected function get(string $filter, int $since, int $limit, bool $previews, string $filterObjectType, int $filterObjectId, string $sort, string $search = '', int $from = 0, int $to = 0, string $actor = ''): DataResponse { + protected function get(string $filter, int $since, int $limit, bool $previews, string $filterObjectType, int $filterObjectId, string $sort, string $search = '', int $from = 0, int $to = 0, string $actor = '', ?TeamActivityScope $teamScope = null): DataResponse { try { $this->validateParameters($filter, $since, $limit, $previews, $filterObjectType, $filterObjectId, $sort, $search, $from, $to, $actor); } catch (InvalidFilterException) { @@ -263,21 +287,32 @@ protected function get(string $filter, int $since, int $limit, bool $previews, s $this->activityManager->setRequirePNG(false); try { - $response = $this->data->get( - $this->helper, - $this->settings, - $this->user, - - $this->since, - $this->limit, - $this->sort, - - $this->filter, - $this->objectType, - $this->objectId, - false, - $this->searchCriteria - ); + $response = $teamScope === null + ? $this->data->get( + $this->helper, + $this->settings, + $this->user, + + $this->since, + $this->limit, + $this->sort, + + $this->filter, + $this->objectType, + $this->objectId, + false, + $this->searchCriteria + ) + : $this->data->getTeam( + $this->helper, + $this->settings, + $this->user, + $this->since, + $this->limit, + $this->sort, + $teamScope, + $this->searchCriteria + ); } catch (\OutOfBoundsException) { // Invalid since argument return new DataResponse([], Http::STATUS_FORBIDDEN); @@ -288,7 +323,7 @@ protected function get(string $filter, int $since, int $limit, bool $previews, s $this->activityManager->setRequirePNG(false); $headers = $this->generateHeaders($response['headers'], $response['has_more'], $response['data']); - if (empty($response['data']) || $this->request->getHeader('If-None-Match') === $headers['ETag']) { + if (($teamScope === null && empty($response['data'])) || $this->request->getHeader('If-None-Match') === $headers['ETag']) { return new DataResponse([], Http::STATUS_NOT_MODIFIED, $headers); } diff --git a/lib/Data.php b/lib/Data.php index 96b9876f5..b7b60e837 100644 --- a/lib/Data.php +++ b/lib/Data.php @@ -34,6 +34,7 @@ class Data { * chart into an unbounded fetch. Reaching it is reported rather than hidden. */ public const MAX_HISTOGRAM_ROWS = 100000; + private const TEAM_RESOURCE_OBJECT_CHUNK_SIZE = 500; /** @var */ protected ?IQueryBuilder $insertActivity = null; @@ -314,6 +315,189 @@ public function get(GroupHelper $groupHelper, UserSettings $userSettings, string } } + /** + * Read activities for a Team and its current resources. + * + * @return array{data: array, has_more: bool, headers: array} + */ + public function getTeam(GroupHelper $groupHelper, UserSettings $userSettings, string $user, int $since, int $limit, string $sort, TeamActivityScope $scope, ?SearchCriteria $search = null): array { + if ($user === '') { + throw new \OutOfBoundsException('Invalid user', 1); + } + + $limit = max(1, min(200, $limit)); + $activeFilter = null; + try { + $activeFilter = $this->activityManager->getFilterById('all'); + } catch (FilterNotFoundException) { + // Unknown filter => ignore app filtering, as get() does. + } + + $scopes = [['circles', [$scope->circleObjectId]]]; + foreach ($scope->resourceScopes as $resourceScope) { + foreach (array_chunk($resourceScope->getObjectIds(), self::TEAM_RESOURCE_OBJECT_CHUNK_SIZE) as $objectIds) { + if ($objectIds !== []) { + $scopes[] = [$resourceScope->getObjectType(), $objectIds]; + } + } + } + + $cursorTimestamp = null; + if ($since !== 0) { + $cursorQuery = $this->connection->getQueryBuilder(); + $cursorQuery->select('timestamp')->from('activity') + ->where($cursorQuery->expr()->eq('activity_id', $cursorQuery->createNamedParameter($since, IQueryBuilder::PARAM_INT))); + $cursorTimestamp = $cursorQuery->executeQuery()->fetchOne(); + if ($cursorTimestamp === false) { + throw new \OutOfBoundsException('Unknown Team Activity cursor'); + } + $cursorTimestamp = (int)$cursorTimestamp; + } + + $sqlSort = ($sort === 'asc') ? 'ASC' : 'DESC'; + $queries = []; + $timestamps = []; + foreach ($scopes as [$objectType, $objectIds]) { + $createQuery = function () use ($userSettings, $user, $activeFilter, $search, $objectType, $objectIds, $cursorTimestamp, $sqlSort): IQueryBuilder { + $query = $this->connection->getQueryBuilder(); + $query->select('*')->from('activity', 'a'); + $this->applyStreamConditions($query, $userSettings, $user, 'all', $activeFilter, '', 0, null, false); + $this->applyTeamSearchCriteria($query, $search ?? SearchCriteria::empty(), $objectType === 'circles'); + $query->andWhere($query->expr()->eq('object_type', $query->createNamedParameter($objectType))); + $query->andWhere($query->expr()->in('object_id', $query->createNamedParameter($objectIds, IQueryBuilder::PARAM_INT_ARRAY))); + if ($cursorTimestamp !== null) { + $parameter = $query->createNamedParameter($cursorTimestamp, IQueryBuilder::PARAM_INT); + $query->andWhere($sqlSort === 'DESC' ? $query->expr()->lte('timestamp', $parameter) : $query->expr()->gte('timestamp', $parameter)); + } + return $query; + }; + $queries[] = $createQuery; + $timestampQuery = $createQuery(); + $timestampQuery->select('timestamp')->groupBy('timestamp')->orderBy('timestamp', $sqlSort)->setMaxResults($limit + 2); + $result = $timestampQuery->executeQuery(); + foreach ($result->fetchAllAssociative() as $row) { + $timestamps[(int)$row['timestamp']] = (int)$row['timestamp']; + } + $result->closeCursor(); + } + $sqlSort === 'DESC' ? rsort($timestamps) : sort($timestamps); + $rows = []; + foreach ($timestamps as $timestamp) { + $bucket = []; + foreach ($queries as $createQuery) { + $bucketQuery = $createQuery(); + $bucketQuery->andWhere($bucketQuery->expr()->eq('timestamp', $bucketQuery->createNamedParameter($timestamp, IQueryBuilder::PARAM_INT))); + $result = $bucketQuery->executeQuery(); + while ($row = $result->fetch()) { + $bucket[] = $this->normaliseTeamFileActivity($row, $scope->filePaths); + } + $result->closeCursor(); + } + $bucket = $this->deduplicateTeamActivities($bucket); + usort($bucket, static fn (array $left, array $right): int => $sqlSort === 'DESC' + ? (int)$right['activity_id'] <=> (int)$left['activity_id'] + : (int)$left['activity_id'] <=> (int)$right['activity_id']); + foreach ($bucket as $row) { + if ($timestamp === $cursorTimestamp && ($sqlSort === 'DESC' ? (int)$row['activity_id'] >= $since : (int)$row['activity_id'] <= $since)) { + continue; + } + $rows[] = $row; + if (count($rows) > $limit) { + break 2; + } + } + } + + $hasMore = count($rows) > $limit; + $rows = array_slice($rows, 0, $limit); + $headers = []; + foreach ($rows as $row) { + if ($row['object_type'] === 'files') { + $row['affecteduser'] = $user; + } + $headers['X-Activity-Last-Given'] = (int)$row['activity_id']; + $groupHelper->addActivity($row); + } + + return ['data' => $groupHelper->getActivities(), 'has_more' => $hasMore, 'headers' => $headers]; + } + + /** + * Files publishes one recipient-specific row per user. A Team stream has no + * single recipient, so turn the self-only wording into its actor form before + * collapsing otherwise identical rows. + */ + private function normaliseTeamFileActivity(array $row, array $filePaths = []): array { + if ($row['app'] !== 'files' || $row['object_type'] !== 'files' || !preg_match('/^(created|changed|deleted|restored|renamed|moved)_(self|by)(_enc)?$/', (string)$row['subject'], $matches)) { + return $row; + } + + $parameters = json_decode((string)$row['subjectparams'], true); + if (!is_array($parameters)) { + return $row; + } + + $action = $matches[1]; + if ($matches[2] === 'self') { + $parameters = in_array($action, ['renamed', 'moved'], true) + ? [$parameters[0] ?? [], $row['user'], $parameters[1] ?? []] + : [$parameters[0] ?? [], $row['user']]; + } + $sourcePath = is_array($parameters[0] ?? null) ? reset($parameters[0]) : ($parameters[0] ?? ''); + foreach ($parameters as &$parameter) { + if (!is_array($parameter)) { + continue; + } + foreach ($parameter as $fileId => &$path) { + if (is_string($path) && is_numeric($fileId)) { + $path = isset($filePaths[$fileId]) && is_string($sourcePath) + ? $this->normaliseTeamFilePath($path, $sourcePath, $filePaths[$fileId]) + : basename($path); + } + } + unset($path); + } + unset($parameter); + $row['file'] = $filePaths[$row['object_id']] ?? basename($row['file']); + $row['link'] = ''; + $row['subject'] = $action . '_by' . ($matches[3] ?? ''); + $row['subjectparams'] = json_encode($parameters, JSON_THROW_ON_ERROR); + + return $row; + } + + private function normaliseTeamFilePath(string $path, string $sourcePath, string $viewerPath): string { + $source = explode('/', trim(dirname($sourcePath), '/')); + $target = explode('/', trim(dirname($viewerPath), '/')); + $relative = explode('/', trim($path, '/')); + while ($source !== [] && $relative !== [] && $source[0] === $relative[0]) { + array_shift($source); + array_shift($relative); + } + $target = array_slice($target, 0, max(0, count($target) - count($source))); + return '/' . implode('/', array_merge($target, $relative)); + } + + /** + * @param list> $rows + * @return list> + */ + private function deduplicateTeamActivities(array $rows): array { + $deduplicated = []; + foreach ($rows as $row) { + $key = implode("\0", [ + $row['app'], $row['subject'], $row['subjectparams'], $row['message'], $row['messageparams'], + $row['file'], $row['link'], $row['user'], $row['timestamp'], $row['priority'], $row['type'], + $row['object_type'], $row['object_id'], + ]); + if (!isset($deduplicated[$key]) || (int)$row['activity_id'] < (int)$deduplicated[$key]['activity_id']) { + $deduplicated[$key] = $row; + } + } + + return array_values($deduplicated); + } + /** * Restrict a query to the activities a user may see under a given filter. * @@ -332,8 +516,11 @@ private function applyStreamConditions( string $objectType, int $objectId, ?SearchCriteria $search, + bool $restrictToAffectedUser = true, ): void { - $query->where($query->expr()->eq('affecteduser', $query->createNamedParameter($user))); + if ($restrictToAffectedUser) { + $query->where($query->expr()->eq('affecteduser', $query->createNamedParameter($user))); + } if ($activeFilter instanceof IFilter && !($activeFilter instanceof AllFilter)) { $notificationTypes = $userSettings->getNotificationTypes(); @@ -511,6 +698,42 @@ private function applySearchCriteria(IQueryBuilder $query, SearchCriteria $crite } } + /** + * Apply Team stream restrictions. Unlike the personal stream, Team Activity + * includes roster events without a file path, so a text search also covers + * their subject identifier and serialized rich-subject parameters. + */ + private function applyTeamSearchCriteria(IQueryBuilder $query, SearchCriteria $criteria, bool $includeHistoricalInitiator): void { + if ($criteria->from !== null) { + $query->andWhere($query->expr()->gte('timestamp', $query->createNamedParameter($criteria->from, IQueryBuilder::PARAM_INT))); + } + + if ($criteria->to !== null) { + $query->andWhere($query->expr()->lte('timestamp', $query->createNamedParameter($criteria->to, IQueryBuilder::PARAM_INT))); + } + + if ($criteria->term !== null) { + $pattern = '%' . $this->connection->escapeLikeParameter($criteria->term) . '%'; + $query->andWhere($query->expr()->orX( + $query->expr()->iLike('file', $query->createNamedParameter($pattern)), + $query->expr()->iLike('subject', $query->createNamedParameter($pattern)), + $query->expr()->iLike('subjectparams', $query->createNamedParameter($pattern)), + )); + } + + if ($criteria->actor !== null) { + $authorCondition = $query->expr()->eq('user', $query->createNamedParameter($criteria->actor)); + if ($includeHistoricalInitiator) { + $initiatorPattern = '%"initiator":{"userId":"' . $this->connection->escapeLikeParameter($criteria->actor) . '"%'; + $authorCondition = $query->expr()->orX( + $authorCondition, + $query->expr()->iLike('subjectparams', $query->createNamedParameter($initiatorPattern)), + ); + } + $query->andWhere($authorCondition); + } + } + /** * * @return array Headers that should be set on the response diff --git a/lib/TeamActivityResolver.php b/lib/TeamActivityResolver.php new file mode 100644 index 000000000..dc32ae3ba --- /dev/null +++ b/lib/TeamActivityResolver.php @@ -0,0 +1,151 @@ +isMember($teamId, $userId)) { + return null; + } + + $circleObjectId = $this->getCircleObjectId($teamId); + if ($circleObjectId === null) { + return null; + } + + $fileResourceIds = []; + $providerResources = []; + foreach ($this->teamManager->getSharedWith($teamId, $userId) as $resource) { + $provider = $resource->getProvider(); + if ($provider->getId() === 'files' && ctype_digit($resource->getId())) { + $fileResourceIds[] = (int)$resource->getId(); + continue; + } + if ($provider instanceof ITeamActivityResourceProvider) { + $providerResources[spl_object_id($provider)]['provider'] = $provider; + $providerResources[spl_object_id($provider)]['resources'][] = $resource; + } + } + + $resourceScopes = []; + foreach ($providerResources as $entry) { + try { + foreach ($entry['provider']->getActivityScopes($entry['resources'], $userId) as $resourceScope) { + if (!$resourceScope instanceof ResourceActivityScope) { + continue; + } + if ($resourceScope->getObjectType() === 'files') { + array_push($fileResourceIds, ...$resourceScope->getObjectIds()); + } else { + $resourceScopes[] = $resourceScope; + } + } + } catch (\Throwable $e) { + $this->logger->warning('Team resource provider failed to resolve its activity scopes', [ + 'exception' => $e, + 'app' => 'activity', + ]); + } + } + $filePaths = $this->getResourceObjects(array_values(array_unique($fileResourceIds)), $userId); + if ($filePaths !== []) { + array_unshift($resourceScopes, new ResourceActivityScope('files', array_keys($filePaths))); + } + + return new TeamActivityScope($circleObjectId, array_keys($filePaths), $filePaths, $resourceScopes); + } + + private function isMember(string $teamId, string $userId): bool { + $members = $this->teamManager->getMembersOfTeam($teamId, $userId); + return array_key_exists($userId, $members); + } + + private function getCircleObjectId(string $teamId): ?int { + $query = $this->connection->getQueryBuilder(); + $query->select('id') + ->from('circles_circle') + ->where($query->expr()->eq('unique_id', $query->createNamedParameter($teamId))); + + $objectId = $query->executeQuery()->fetchOne(); + return $objectId === false ? null : (int)$objectId; + } + + /** + * @param list $resourceIds + * @return array + */ + private function getResourceObjects(array $resourceIds, string $userId): array { + try { + $userFolder = $this->rootFolder->getUserFolder($userId); + } catch (NotFoundException|NotPermittedException) { + return []; + } + + $paths = []; + foreach ($resourceIds as $resourceId) { + try { + $roots = $userFolder->getById($resourceId); + } catch (NotFoundException|NotPermittedException) { + continue; + } + foreach ($roots as $root) { + if (!$root instanceof Node) { + continue; + } + $visited = []; + $pending = [$root]; + while ($pending !== []) { + $node = array_pop($pending); + try { + $id = $node->getId(); + if ($id === null || isset($visited[$id]) || !$node->isReadable() + || $node->getMountPoint()->getMountPoint() !== $root->getMountPoint()->getMountPoint()) { + continue; + } + $visited[$id] = true; + $path = $userFolder->getRelativePath($node->getPath()); + if ($path === null) { + continue; + } + $paths[$id] ??= $path; + if ($node instanceof Folder) { + array_push($pending, ...$node->getDirectoryListing()); + } + } catch (NotFoundException|NotPermittedException) { + continue; + } + } + } + } + + return $paths; + } +} diff --git a/lib/TeamActivityScope.php b/lib/TeamActivityScope.php new file mode 100644 index 000000000..831e43ac3 --- /dev/null +++ b/lib/TeamActivityScope.php @@ -0,0 +1,41 @@ + + */ +class TeamActivityScope { + /** + * @param FileObjectIds $fileObjectIds Filecache IDs reachable through Team resources + * @param array $filePaths Readable paths relative to the viewer's files + * @param list<\OCP\Teams\TeamActivityScope> $resourceScopes Activity objects reachable through Team resources + */ + public function __construct( + public readonly int $circleObjectId, + public readonly array $fileObjectIds = [], + public readonly array $filePaths = [], + array $resourceScopes = [], + ) { + foreach ($resourceScopes as $resourceScope) { + if (!$resourceScope instanceof \OCP\Teams\TeamActivityScope) { + throw new \InvalidArgumentException('Team resource scopes must be Teams Activity scopes'); + } + } + $this->resourceScopes = $resourceScopes !== [] + ? $resourceScopes + : ($this->fileObjectIds === [] ? [] : [new \OCP\Teams\TeamActivityScope('files', $this->fileObjectIds)]); + } + + /** @var list<\OCP\Teams\TeamActivityScope> */ + public readonly array $resourceScopes; +} diff --git a/tests/Controller/APIv2ControllerTest.php b/tests/Controller/APIv2ControllerTest.php index 0f4e142fe..9ef768993 100644 --- a/tests/Controller/APIv2ControllerTest.php +++ b/tests/Controller/APIv2ControllerTest.php @@ -31,6 +31,8 @@ use OCA\Activity\Exception\InvalidSearchCriteriaException; use OCA\Activity\GroupHelper; use OCA\Activity\SearchCriteria; +use OCA\Activity\TeamActivityResolver; +use OCA\Activity\TeamActivityScope; use OCA\Activity\Tests\TestCase; use OCA\Activity\UserSettings; use OCA\Activity\ViewInfoCache; @@ -70,6 +72,7 @@ class APIv2ControllerTest extends TestCase { protected ViewInfoCache&MockObject $infoCache; protected INotificationManager&MockObject $notificationManager; protected IDateTimeZone&MockObject $dateTimeZone; + protected TeamActivityResolver&MockObject $teamActivityResolver; protected IL10N $l10n; protected APIv2Controller $controller; @@ -92,6 +95,7 @@ protected function setUp(): void { $this->request = $this->createMock(IRequest::class); $this->dateTimeZone = $this->createMock(IDateTimeZone::class); $this->dateTimeZone->method('getTimeZone')->willReturn(new \DateTimeZone('UTC')); + $this->teamActivityResolver = $this->createMock(TeamActivityResolver::class); $this->controller = $this->getController(); } @@ -112,6 +116,7 @@ protected function getController(array $methods = []): APIv2Controller|MockObjec $this->infoCache, $this->notificationManager, $this->dateTimeZone, + $this->teamActivityResolver, ); } @@ -130,6 +135,7 @@ protected function getController(array $methods = []): APIv2Controller|MockObjec $this->infoCache, $this->notificationManager, $this->dateTimeZone, + $this->teamActivityResolver, ]) ->onlyMethods($methods) ->getMock(); @@ -363,6 +369,37 @@ public function testGetFilterForwardsSearchCriteria(): void { $controller->getFilter('files', 0, 50, false, '', 0, 'desc', 'report', 1000, 2000, 'alice'); } + public function testGetTeamForwardsTheResolvedScope(): void { + $controller = $this->getController(['get']); + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('alice'); + $this->userSession->method('getUser')->willReturn($user); + $scope = new TeamActivityScope(42, [7, 8]); + $this->teamActivityResolver->expects($this->once()) + ->method('getScope') + ->with('team-single-id', 'alice') + ->willReturn($scope); + $controller->expects($this->once()) + ->method('get') + ->with('all', 0, 50, false, '', 0, 'desc', 'report', 1000, 2000, 'bob', $scope); + + $controller->getTeam('team-single-id', 0, 50, false, 'desc', 'report', 1000, 2000, 'bob'); + } + + public function testGetTeamHidesAnInaccessibleTeam(): void { + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('alice'); + $this->userSession->method('getUser')->willReturn($user); + $this->teamActivityResolver->expects($this->once()) + ->method('getScope') + ->with('team-single-id', 'alice') + ->willReturn(null); + + $result = $this->controller->getTeam('team-single-id'); + + $this->assertSame(Http::STATUS_NOT_FOUND, $result->getStatus()); + } + public function testGetPassesSearchCriteriaToData(): void { $controller = $this->getController(['validateParameters', 'generateHeaders']); $controller->method('validateParameters'); diff --git a/tests/Controller/TeamActivityResponseTest.php b/tests/Controller/TeamActivityResponseTest.php new file mode 100644 index 000000000..009c30dfa --- /dev/null +++ b/tests/Controller/TeamActivityResponseTest.php @@ -0,0 +1,63 @@ + [false, [], '', 304], + 'personal ETag match' => [false, [['activity_id' => 1]], 'etag', 304], + 'empty Team feed' => [true, [], '', 200], + 'Team ETag match' => [true, [], 'etag', 304], + ]; + } + + #[DataProvider('responses')] + public function testResponseContract(bool $team, array $activities, string $etag, int $status): void { + $controller = $this->getMockBuilder(APIv2Controller::class) + ->disableOriginalConstructor() + ->onlyMethods(['validateParameters', 'generateHeaders']) + ->getMock(); + $controller->method('generateHeaders')->willReturn(['ETag' => 'etag']); + $request = $this->createMock(IRequest::class); + $request->expects(!$team && $activities === [] ? $this->never() : $this->once()) + ->method('getHeader')->with('If-None-Match')->willReturn($etag); + $data = $this->createMock(Data::class); + $data->expects($this->once())->method($team ? 'getTeam' : 'get')->willReturn([ + 'data' => $activities, 'headers' => [], 'has_more' => false, + ]); + foreach ([ + 'request' => $request, + 'activityManager' => $this->createMock(IManager::class), + 'data' => $data, + 'helper' => $this->createMock(GroupHelper::class), + 'settings' => $this->createMock(UserSettings::class), + 'searchCriteria' => SearchCriteria::empty(), + ] as $property => $value) { + (new \ReflectionProperty(APIv2Controller::class, $property))->setValue($controller, $value); + } + $response = (new \ReflectionMethod(APIv2Controller::class, 'get'))->invoke( + $controller, 'all', 0, 50, false, '', 0, 'desc', '', 0, 0, '', $team ? new TeamActivityScope(42, []) : null, + ); + $this->assertSame($status, $response->getStatus()); + $this->assertSame([], $response->getData()); + } +} diff --git a/tests/DataTest.php b/tests/DataTest.php index 275e4cc03..48c5d9468 100644 --- a/tests/DataTest.php +++ b/tests/DataTest.php @@ -26,6 +26,9 @@ use OCA\Activity\AppInfo\Application; use OCA\Activity\Data; +use OCA\Activity\GroupHelper; +use OCA\Activity\SearchCriteria; +use OCA\Activity\TeamActivityScope; use OCA\Activity\UserSettings; use OCP\Activity\Exceptions\FilterNotFoundException; use OCP\Activity\IManager; @@ -125,6 +128,71 @@ public function testSend(string $actionUser, string $affectedUser, string $expec $this->deleteTestActivities(); } + public function testTeamStreamIncludesEventsPublishedBeforeTheViewerJoined(): void { + $this->deleteTestActivities(); + + $event = $this->realActivityManager->generateEvent(); + $event->setApp('test') + ->setType('type') + ->setSubject('subject') + ->setAuthor('bob') + ->setAffectedUser('bob') + ->setObject('circles', 42, 'Activity 3'); + $this->data->send($event); + + $helper = $this->createMock(GroupHelper::class); + $helper->expects($this->once()) + ->method('addActivity') + ->with($this->callback(static fn (array $activity): bool => $activity['affecteduser'] === 'bob')); + $helper->expects($this->once()) + ->method('getActivities') + ->willReturn([]); + + $result = $this->data->getTeam( + $helper, + $this->createMock(UserSettings::class), + 'alice', + 0, + 50, + 'desc', + new TeamActivityScope(42, []), + ); + + $this->assertFalse($result['has_more']); + $this->deleteTestActivities(); + } + + public function testTeamStreamSearchesRosterSubjectsAndFiltersByAuthor(): void { + $this->deleteTestActivities(); + + $event = $this->realActivityManager->generateEvent(); + $event->setApp('test') + ->setType('member_added') + ->setSubject('member_added', ['initiator' => ['userId' => 'admin'], 'member' => 'Alice']) + ->setAffectedUser('bob') + ->setObject('circles', 42, 'Activity 3'); + $this->data->send($event); + + $helper = $this->createMock(GroupHelper::class); + $helper->expects($this->once()) + ->method('addActivity') + ->with($this->callback(static fn (array $activity): bool => $activity['subject'] === 'member_added' && $activity['user'] === '')); + $helper->expects($this->once())->method('getActivities')->willReturn([]); + + $this->data->getTeam( + $helper, + $this->createMock(UserSettings::class), + 'alice', + 0, + 50, + 'desc', + new TeamActivityScope(42, []), + SearchCriteria::create('member', 0, 0, 'admin'), + ); + + $this->deleteTestActivities(); + } + #[DataProvider('dataSend')] public function testStoreMail(string $actionUser, string $affectedUser, string $expectedAuthor, string $expectedAffected, bool $expectedActivity): void { $this->deleteTestMails(); diff --git a/tests/TeamActivityDataTest.php b/tests/TeamActivityDataTest.php new file mode 100644 index 000000000..b414cf663 --- /dev/null +++ b/tests/TeamActivityDataTest.php @@ -0,0 +1,184 @@ +markTestSkipped('The isolated Team Activity tests require pdo_sqlite'); + } + $this->database = DriverManager::getConnection(['driver' => 'pdo_sqlite', 'memory' => true]); + $this->database->executeStatement('CREATE TABLE activity (activity_id INTEGER PRIMARY KEY, app TEXT, subject TEXT, subjectparams TEXT, message TEXT, messageparams TEXT, file TEXT, link TEXT, user TEXT, affecteduser TEXT, timestamp INTEGER, priority INTEGER, type TEXT, object_type TEXT, object_id INTEGER)'); + $inner = $this->createMock(Connection::class); + $inner->method('getDatabasePlatform')->willReturn($this->database->getDatabasePlatform()); + $connection = $this->createMock(ConnectionAdapter::class); + $connection->method('getInner')->willReturn($inner); + $connection->method('getDatabaseProvider')->willReturn(IDBConnection::PLATFORM_SQLITE); + $connection->method('executeQuery')->willReturnCallback(fn ($sql, $parameters, $types) => new ResultAdapter($this->database->executeQuery($sql, $parameters, $types))); + $systemConfig = $this->createMock(SystemConfig::class); + $connection->method('getQueryBuilder')->willReturnCallback(static function () use ($connection, $systemConfig): QueryBuilder { + $query = new QueryBuilder($connection, $systemConfig, new NullLogger()); + $query->automaticTablePrefix(false); + return $query; + }); + $manager = $this->createMock(IManager::class); + $manager->method('getFilterById')->willThrowException(new FilterNotFoundException('all')); + $this->data = new Data($manager, $connection, new NullLogger(), $this->createMock(IConfig::class), $this->createMock(IRootFolder::class)); + } + + public static function pages(): array { + return [ + [1, 'desc', [90, 85, 80]], + [2, 'desc', [90, 85, 80]], + [1, 'asc', [80, 85, 90]], + [2, 'asc', [80, 85, 90]], + ]; + } + + #[DataProvider('pages')] + public function testPaginationDeduplicatesBeforeApplyingCursorAndLimit(int $limit, string $sort, array $expected): void { + $this->insertFileEvent(100, 100, 7, 'created_self', 'alice', '/Owner/a.txt'); + $this->insertFileEvent(90, 100, 7, 'created_by', 'bob', '/Recipient/a.txt'); + $this->insertFileEvent(85, 100, 8, 'created_by', 'bob', '/Recipient/b.txt'); + $this->database->insert('activity', [ + 'activity_id' => 80, 'timestamp' => 99, 'app' => 'circles', + 'subject' => 'member_added', 'subjectparams' => '[]', 'message' => '', 'messageparams' => '[]', + 'file' => '', 'link' => '', 'user' => 'alice', 'affecteduser' => 'bob', + 'priority' => 30, 'type' => 'member_added', 'object_type' => 'circles', 'object_id' => 42, + ]); + $scope = new TeamActivityScope(42, [7, 8], [7 => '/Viewer/a.txt', 8 => '/Viewer/b.txt']); + $seen = []; + $since = 0; + $page = 0; + do { + $result = $this->page($scope, $since, $limit, $sort); + foreach ($result['data'] as $row) { + $seen[] = (int)$row['activity_id']; + if ($row['object_type'] === 'files') { + $this->assertSame('viewer', $row['affecteduser']); + $this->assertStringStartsWith('/Viewer/', $row['file']); + $this->assertStringNotContainsString('Recipient', $row['subjectparams']); + $this->assertStringNotContainsString('Owner', $row['subjectparams']); + } + } + if (!$result['has_more']) { + break; + } + $this->assertNotEmpty($result['data']); + $since = $result['headers']['X-Activity-Last-Given']; + $page++; + } while ($page < 5); + $this->assertSame($expected, $seen); + $this->assertFalse($result['has_more']); + $empty = $this->page($scope, $result['headers']['X-Activity-Last-Given'], $limit, $sort); + $this->assertSame([], $empty['data']); + $this->assertFalse($empty['has_more']); + } + + public function testMovePathsAreRecipientIndependentButDistinctMovesRemain(): void { + $this->insertFileEvent(10, 100, 7, 'moved_self', 'alice', '/Owner/New/a.txt', '/Owner/Old/a.txt'); + $this->insertFileEvent(20, 100, 7, 'moved_by', 'bob', '/Recipient/New/a.txt', '/Recipient/Old/a.txt'); + $this->insertFileEvent(30, 100, 7, 'moved_by', 'bob', '/Recipient/New/a.txt', '/Recipient/Other/a.txt'); + $scope = new TeamActivityScope(42, [7], [7 => '/Viewer/New/a.txt']); + $result = $this->page($scope, 0, 10, 'asc'); + $this->assertSame([10, 30], array_map('intval', array_column($result['data'], 'activity_id'))); + $this->assertSame([[7 => '/Viewer/New/a.txt'], 'alice', [7 => '/Viewer/Old/a.txt']], json_decode($result['data'][0]['subjectparams'], true)); + $this->assertSame([[7 => '/Viewer/New/a.txt'], 'alice', [7 => '/Viewer/Other/a.txt']], json_decode($result['data'][1]['subjectparams'], true)); + } + + public function testPaginationMergesMultipleFileIdChunks(): void { + $this->insertFileEvent(10, 100, 1, 'created_self', 'alice', '/Owner/a.txt'); + $this->insertFileEvent(20, 100, 1, 'created_by', 'bob', '/Recipient/a.txt'); + $this->insertFileEvent(30, 100, 501, 'created_by', 'bob', '/Recipient/b.txt'); + $scope = new TeamActivityScope(42, range(1, 501), [1 => '/Viewer/a.txt', 501 => '/Viewer/b.txt']); + $first = $this->page($scope, 0, 1, 'desc'); + $this->assertSame(30, $first['headers']['X-Activity-Last-Given']); + $this->assertTrue($first['has_more']); + $second = $this->page($scope, 30, 1, 'desc'); + $this->assertSame(10, $second['headers']['X-Activity-Last-Given']); + $this->assertFalse($second['has_more']); + } + + private function insertFileEvent(int $id, int $timestamp, int $fileId, string $subject, string $recipient, string $path, ?string $oldPath = null): void { + $parameters = [[$fileId => $path]]; + if (str_ends_with($subject, '_by')) { + $parameters[] = 'alice'; + } + if ($oldPath !== null) { + $parameters[] = [$fileId => $oldPath]; + } + $this->database->insert('activity', [ + 'activity_id' => $id, 'timestamp' => $timestamp, 'app' => 'files', + 'subject' => $subject, 'subjectparams' => json_encode($parameters), 'message' => '', 'messageparams' => '[]', + 'file' => $path, 'link' => '/recipient/' . $recipient, 'user' => 'alice', 'affecteduser' => $recipient, + 'priority' => 30, 'type' => 'file_created', 'object_type' => 'files', 'object_id' => $fileId, + ]); + } + + private function page(TeamActivityScope $scope, int $since, int $limit, string $sort): array { + $activities = []; + $helper = $this->createMock(GroupHelper::class); + $helper->method('addActivity')->willReturnCallback(static function (array $row) use (&$activities): void { + $activities[] = $row; + }); + $helper->method('getActivities')->willReturnCallback(static function () use (&$activities): array { + return $activities; + }); + return $this->data->getTeam($helper, $this->createMock(UserSettings::class), 'viewer', $since, $limit, $sort, $scope); + } + + public function testNonFileResourceScopesKeepTheirActivityPayload(): void { + $this->database->insert('activity', [ + 'activity_id' => 20, 'timestamp' => 100, 'app' => 'deck', + 'subject' => 'card_create', 'subjectparams' => '{"card":{"id":70,"title":"Team card"}}', 'message' => '', 'messageparams' => '[]', + 'file' => '', 'link' => '/apps/deck/#/board/7/card/70', 'user' => 'alice', 'affecteduser' => 'alice', + 'priority' => 30, 'type' => 'deck', 'object_type' => 'deck_card', 'object_id' => 70, + ]); + $this->database->insert('activity', [ + 'activity_id' => 21, 'timestamp' => 101, 'app' => 'deck', + 'subject' => 'card_create', 'subjectparams' => '{"card":{"id":71,"title":"Other card"}}', 'message' => '', 'messageparams' => '[]', + 'file' => '', 'link' => '/apps/deck/#/board/8/card/71', 'user' => 'alice', 'affecteduser' => 'alice', + 'priority' => 30, 'type' => 'deck', 'object_type' => 'deck_card', 'object_id' => 71, + ]); + $scope = new TeamActivityScope(42, [], [], [new ResourceActivityScope('deck_card', [70])]); + + $result = $this->page($scope, 0, 10, 'desc'); + + self::assertSame([20], array_map('intval', array_column($result['data'], 'activity_id'))); + self::assertSame('/apps/deck/#/board/7/card/70', $result['data'][0]['link']); + self::assertSame('{"card":{"id":70,"title":"Team card"}}', $result['data'][0]['subjectparams']); + } + +} diff --git a/tests/TeamActivityResolverTest.php b/tests/TeamActivityResolverTest.php new file mode 100644 index 000000000..b626d0739 --- /dev/null +++ b/tests/TeamActivityResolverTest.php @@ -0,0 +1,251 @@ +createMock(ITeamManager::class); + $connection = $this->createMock(IDBConnection::class); + $teamManager->expects($this->once()) + ->method('getMembersOfTeam') + ->with('team-1', 'alice') + ->willReturn(['bob' => 'Bob']); + $connection->expects($this->never())->method('getQueryBuilder'); + + $resolver = new TeamActivityResolver($teamManager, $connection, $this->createMock(IRootFolder::class), $this->createMock(LoggerInterface::class)); + + self::assertNull($resolver->getScope('team-1', 'alice')); + } + + public function testFolderScopeExcludesUnreadableSubtrees(): void { + $rootFolder = $this->createMock(IRootFolder::class); + $userFolder = $this->createMock(IUserFolder::class); + $folder = $this->createMock(Folder::class); + $denied = $this->createMock(Folder::class); + $mount = $this->createMock(IMountPoint::class); + $mount->method('getMountPoint')->willReturn('/alice/files/Team/'); + $rootFolder->method('getUserFolder')->with('alice')->willReturn($userFolder); + $userFolder->method('getById')->with(200)->willReturn([$folder]); + $userFolder->method('getRelativePath')->willReturn('/Team'); + $folder->method('getId')->willReturn(200); + $folder->method('isReadable')->willReturn(true); + $folder->method('getMountPoint')->willReturn($mount); + $folder->method('getDirectoryListing')->willReturn([$denied]); + $denied->method('getId')->willReturn(201); + $denied->method('isReadable')->willReturn(false); + $denied->expects($this->never())->method('getDirectoryListing'); + $resolver = new TeamActivityResolver($this->createMock(ITeamManager::class), $this->createMock(IDBConnection::class), $rootFolder, $this->createMock(LoggerInterface::class)); + + $method = new \ReflectionMethod($resolver, 'getResourceObjects'); + self::assertSame([200 => '/Team'], $method->invoke($resolver, [200], 'alice')); + } + + public function testEachMountIsCheckedAndNestedMountsAreExcluded(): void { + $rootFolder = $this->createMock(IRootFolder::class); + $userFolder = $this->createMock(IUserFolder::class); + $rootFolder->method('getUserFolder')->willReturn($userFolder); + $userFolder->method('getRelativePath')->willReturnArgument(0); + $roots = []; + foreach (['/First', '/Second'] as $path) { + $mount = $this->createMock(IMountPoint::class); + $mount->method('getMountPoint')->willReturn($path); + $root = $this->createMock(Folder::class); + $root->method('getId')->willReturn(200); + $root->method('getPath')->willReturn($path); + $root->method('isReadable')->willReturn(true); + $root->method('getMountPoint')->willReturn($mount); + $child = $this->createMock(Folder::class); + $child->method('getId')->willReturn(201); + $child->method('getPath')->willReturn($path . '/Child'); + $child->method('isReadable')->willReturn($path === '/Second'); + $child->method('getMountPoint')->willReturn($mount); + $foreign = $this->createMock(Folder::class); + $foreign->method('getId')->willReturn(300); + $foreign->method('isReadable')->willReturn(true); + $foreignMount = $this->createMock(IMountPoint::class); + $foreignMount->method('getMountPoint')->willReturn($path . '/Shared'); + $foreign->method('getMountPoint')->willReturn($foreignMount); + $foreign->expects($this->never())->method('getDirectoryListing'); + $root->method('getDirectoryListing')->willReturn([$child, $foreign]); + $roots[] = $root; + } + $userFolder->method('getById')->willReturn($roots); + $resolver = new TeamActivityResolver($this->createMock(ITeamManager::class), $this->createMock(IDBConnection::class), $rootFolder, $this->createMock(LoggerInterface::class)); + $method = new \ReflectionMethod($resolver, 'getResourceObjects'); + self::assertSame([200 => '/First', 201 => '/Second/Child'], $method->invoke($resolver, [200], 'alice')); + } + + public function testMissingUserFolderHasNoFileScope(): void { + $rootFolder = $this->createMock(IRootFolder::class); + $rootFolder->method('getUserFolder')->willThrowException(new NotFoundException()); + $resolver = new TeamActivityResolver($this->createMock(ITeamManager::class), $this->createMock(IDBConnection::class), $rootFolder, $this->createMock(LoggerInterface::class)); + $method = new \ReflectionMethod($resolver, 'getResourceObjects'); + self::assertSame([], $method->invoke($resolver, [200], 'alice')); + } + + public function testResourceScopeIncludesFileRootsAndFolderSubtrees(): void { + $rootFolder = $this->createMock(IRootFolder::class); + $userFolder = $this->createMock(IUserFolder::class); + $folder = $this->createMock(Folder::class); + $file = $this->createMock(File::class); + $child = $this->createMock(File::class); + $mount = $this->createMock(IMountPoint::class); + $mount->method('getMountPoint')->willReturn('/alice/files/'); + $rootFolder->method('getUserFolder')->with('alice')->willReturn($userFolder); + $userFolder->method('getById')->willReturnMap([[100, [$file]], [200, [$folder]]]); + $userFolder->method('getRelativePath')->willReturnMap([ + ['/alice/files/readme.md', '/readme.md'], + ['/alice/files/Shared', '/Shared'], + ['/alice/files/Shared/note.md', '/Shared/note.md'], + ]); + foreach ([[$file, 100, '/alice/files/readme.md'], [$folder, 200, '/alice/files/Shared'], [$child, 201, '/alice/files/Shared/note.md']] as [$node, $id, $path]) { + $node->method('getId')->willReturn($id); + $node->method('getPath')->willReturn($path); + $node->method('isReadable')->willReturn(true); + $node->method('getMountPoint')->willReturn($mount); + } + $folder->method('getDirectoryListing')->willReturn([$child]); + $resolver = new TeamActivityResolver($this->createMock(ITeamManager::class), $this->createMock(IDBConnection::class), $rootFolder, $this->createMock(LoggerInterface::class)); + + $method = new \ReflectionMethod($resolver, 'getResourceObjects'); + self::assertSame([100 => '/readme.md', 200 => '/Shared', 201 => '/Shared/note.md'], $method->invoke($resolver, [100, 200], 'alice')); + } + + public function testScopeUsesOnlyVisibleFileResources(): void { + $teamManager = $this->createMock(ITeamManager::class); + $connection = $this->createMock(IDBConnection::class); + $rootFolder = $this->createMock(IRootFolder::class); + $userFolder = $this->createMock(IUserFolder::class); + $file = $this->createMock(File::class); + $groupFolderFile = $this->createMock(File::class); + $mount = $this->createMock(IMountPoint::class); + $mount->method('getMountPoint')->willReturn('/alice/files/'); + $file->method('getId')->willReturn(100); + $file->method('getPath')->willReturn('/alice/files/readme.md'); + $file->method('isReadable')->willReturn(true); + $file->method('getMountPoint')->willReturn($mount); + $groupFolderFile->method('getId')->willReturn(200); + $groupFolderFile->method('getPath')->willReturn('/alice/files/Team/readme.md'); + $groupFolderFile->method('isReadable')->willReturn(true); + $groupFolderFile->method('getMountPoint')->willReturn($mount); + $rootFolder->expects($this->once())->method('getUserFolder')->with('alice')->willReturn($userFolder); + $userFolder->expects($this->exactly(2))->method('getById')->willReturnMap([[100, [$file]], [200, [$groupFolderFile]]]); + $userFolder->method('getRelativePath')->willReturnMap([ + ['/alice/files/readme.md', '/readme.md'], + ['/alice/files/Team/readme.md', '/Team/readme.md'], + ]); + $filesProvider = $this->createMock(ITeamResourceProvider::class); + $filesProvider->method('getId')->willReturn('files'); + $deckProvider = $this->createMock(ITeamActivityResourceProvider::class); + $deckProvider->method('getId')->willReturn('deck'); + $deckResource = new TeamResource($deckProvider, '200', 'Board', '/deck/200'); + $deckProvider->expects($this->once()) + ->method('getActivityScopes') + ->with([$deckResource], 'alice') + ->willReturn([new ResourceActivityScope('deck_board', [200]), new ResourceActivityScope('deck_card', [300, 301])]); + $groupFoldersProvider = $this->createMock(ITeamActivityResourceProvider::class); + $groupFoldersProvider->method('getId')->willReturn('groupfolders'); + $groupFolderResource = new TeamResource($groupFoldersProvider, '42', 'Team folder', '/groupfolders/42'); + $groupFoldersProvider->expects($this->once()) + ->method('getActivityScopes') + ->with([$groupFolderResource], 'alice') + ->willReturn([new ResourceActivityScope('files', [200])]); + $unsupportedProvider = $this->createMock(ITeamResourceProvider::class); + $unsupportedProvider->method('getId')->willReturn('calendar'); + $teamManager->expects($this->once())->method('getMembersOfTeam')->with('team-1', 'alice')->willReturn(['alice' => 'Alice']); + $teamManager->expects($this->once())->method('getSharedWith')->with('team-1', 'alice')->willReturn([ + new TeamResource($filesProvider, '100', 'Readme', '/f/100'), + $groupFolderResource, + $deckResource, + new TeamResource($unsupportedProvider, '400', 'Calendar', '/calendar/400'), + new TeamResource($filesProvider, 'not-a-file-id', 'Invalid', '/f/invalid'), + ]); + $query = $this->createMock(IQueryBuilder::class); + $expression = $this->createMock(IExpressionBuilder::class); + $result = $this->createMock(IResult::class); + $connection->expects($this->once())->method('getQueryBuilder')->willReturn($query); + $query->method('select')->willReturnSelf(); + $query->method('from')->willReturnSelf(); + $query->method('where')->willReturnSelf(); + $query->method('createNamedParameter')->willReturn(':team'); + $query->method('expr')->willReturn($expression); + $expression->method('eq')->willReturn('unique_id = :team'); + $query->method('executeQuery')->willReturn($result); + $result->method('fetchOne')->willReturn(42); + + $scope = (new TeamActivityResolver($teamManager, $connection, $rootFolder, $this->createMock(LoggerInterface::class)))->getScope('team-1', 'alice'); + + self::assertNotNull($scope); + self::assertSame(42, $scope->circleObjectId); + self::assertSame([100, 200], $scope->fileObjectIds); + self::assertSame([100 => '/readme.md', 200 => '/Team/readme.md'], $scope->filePaths); + self::assertSame('files', $scope->resourceScopes[0]->getObjectType()); + self::assertSame([100, 200], $scope->resourceScopes[0]->getObjectIds()); + self::assertSame('deck_board', $scope->resourceScopes[1]->getObjectType()); + self::assertSame([200], $scope->resourceScopes[1]->getObjectIds()); + self::assertSame('deck_card', $scope->resourceScopes[2]->getObjectType()); + self::assertSame([300, 301], $scope->resourceScopes[2]->getObjectIds()); + } + + public function testProviderFailureIsLoggedAndOtherScopesAreKept(): void { + $teamManager = $this->createMock(ITeamManager::class); + $failingProvider = $this->createMock(ITeamActivityResourceProvider::class); + $failingProvider->method('getId')->willReturn('broken'); + $failingProvider->method('getActivityScopes')->willThrowException(new \RuntimeException('provider down')); + $deckProvider = $this->createMock(ITeamActivityResourceProvider::class); + $deckProvider->method('getId')->willReturn('deck'); + $deckProvider->method('getActivityScopes')->willReturn([new ResourceActivityScope('deck_card', [300])]); + $teamManager->method('getMembersOfTeam')->willReturn(['alice' => 'Alice']); + $teamManager->method('getSharedWith')->willReturn([ + new TeamResource($failingProvider, '1', 'Broken', '/broken/1'), + new TeamResource($deckProvider, '2', 'Board', '/deck/2'), + ]); + $query = $this->createMock(IQueryBuilder::class); + $result = $this->createMock(IResult::class); + $connection = $this->createMock(IDBConnection::class); + $connection->method('getQueryBuilder')->willReturn($query); + $query->method('select')->willReturnSelf(); + $query->method('from')->willReturnSelf(); + $query->method('where')->willReturnSelf(); + $query->method('expr')->willReturn($this->createMock(IExpressionBuilder::class)); + $query->method('executeQuery')->willReturn($result); + $result->method('fetchOne')->willReturn(42); + $logger = $this->createMock(LoggerInterface::class); + $logger->expects($this->once())->method('warning')->with( + $this->stringContains('provider'), + $this->callback(static fn (array $context): bool => $context['exception'] instanceof \RuntimeException), + ); + + $scope = (new TeamActivityResolver($teamManager, $connection, $this->createMock(IRootFolder::class), $logger))->getScope('team-1', 'alice'); + + self::assertNotNull($scope); + self::assertCount(1, $scope->resourceScopes); + self::assertSame('deck_card', $scope->resourceScopes[0]->getObjectType()); + } +} From 60850321072f635a4501b02b7a5de40d87028abd Mon Sep 17 00:00:00 2001 From: Stefan Lender Date: Mon, 19 Oct 2026 14:19:47 +0200 Subject: [PATCH 2/2] perf(teams): use filecache join for Team activity stream Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Stefan Lender --- docs/endpoint-v2.md | 8 + lib/Data.php | 232 ++++++++++++++--- lib/TeamActivityFileScope.php | 72 ++++++ lib/TeamActivityResolver.php | 47 ++-- lib/TeamActivityScope.php | 16 +- tests/Controller/APIv2ControllerTest.php | 2 +- tests/TeamActivityDataTest.php | 310 ++++++++++++++++++++++- tests/TeamActivityFileScopeTest.php | 101 ++++++++ tests/TeamActivityResolverTest.php | 69 ++--- 9 files changed, 729 insertions(+), 128 deletions(-) create mode 100644 lib/TeamActivityFileScope.php create mode 100644 tests/TeamActivityFileScopeTest.php diff --git a/docs/endpoint-v2.md b/docs/endpoint-v2.md index aa1e9755e..77db29d5e 100644 --- a/docs/endpoint-v2.md +++ b/docs/endpoint-v2.md @@ -47,6 +47,14 @@ those files. A file resource is included directly; a folder resource includes its full readable subtree. Deleted files, files no longer reachable through any Team resource, and nested external mounts are not part of this scope. +File scopes are resolved from their roots without recursively listing all +descendants. Activity queries join `filecache` and restrict candidates to the +resource's storage and subtree. Roots are queried in bounded chunks, so Teams +with many file resources do not produce unbounded queries. Candidates are then +checked through the member's mounts, including read access to intermediate +folders. Pagination continues past inaccessible candidates until another +visible activity is found or the matching activities are exhausted. + Other Team resource providers can opt in to the stream by resolving their resources into Activity object scopes. Activity then filters on those object types and IDs without inferring scope from the Team's member roster. A resource diff --git a/lib/Data.php b/lib/Data.php index b7b60e837..a7f3e2856 100644 --- a/lib/Data.php +++ b/lib/Data.php @@ -17,7 +17,11 @@ use OCP\Activity\IManager; use OCP\DB\Exception; use OCP\DB\QueryBuilder\IQueryBuilder; +use OCP\Files\Folder; +use OCP\Files\IHomeStorage; use OCP\Files\IRootFolder; +use OCP\Files\Node; +use OCP\Files\NotFoundException; use OCP\IConfig; use OCP\IDBConnection; use Psr\Log\LoggerInterface; @@ -35,6 +39,8 @@ class Data { */ public const MAX_HISTOGRAM_ROWS = 100000; private const TEAM_RESOURCE_OBJECT_CHUNK_SIZE = 500; + private const TEAM_FILE_ROOT_CHUNK_SIZE = 100; + private const TEAM_TIMESTAMP_BATCH_SIZE = 50; /** @var */ protected ?IQueryBuilder $insertActivity = null; @@ -333,14 +339,18 @@ public function getTeam(GroupHelper $groupHelper, UserSettings $userSettings, st // Unknown filter => ignore app filtering, as get() does. } - $scopes = [['circles', [$scope->circleObjectId]]]; + $scopes = [['circles', [$scope->circleObjectId], []]]; foreach ($scope->resourceScopes as $resourceScope) { foreach (array_chunk($resourceScope->getObjectIds(), self::TEAM_RESOURCE_OBJECT_CHUNK_SIZE) as $objectIds) { - if ($objectIds !== []) { - $scopes[] = [$resourceScope->getObjectType(), $objectIds]; - } + $scopes[] = [$resourceScope->getObjectType(), $objectIds, []]; } } + $fileRoots = $this->getTeamFileRoots($scope); + $singleFiles = array_values(array_filter($fileRoots, static fn (array $root): bool => !$root['folder'])); + $folders = array_values(array_filter($fileRoots, static fn (array $root): bool => $root['folder'])); + foreach ([...array_chunk($singleFiles, self::TEAM_RESOURCE_OBJECT_CHUNK_SIZE), ...array_chunk($folders, self::TEAM_FILE_ROOT_CHUNK_SIZE)] as $chunk) { + $scopes[] = ['files', [], $chunk]; + } $cursorTimestamp = null; if ($since !== 0) { @@ -356,15 +366,18 @@ public function getTeam(GroupHelper $groupHelper, UserSettings $userSettings, st $sqlSort = ($sort === 'asc') ? 'ASC' : 'DESC'; $queries = []; - $timestamps = []; - foreach ($scopes as [$objectType, $objectIds]) { - $createQuery = function () use ($userSettings, $user, $activeFilter, $search, $objectType, $objectIds, $cursorTimestamp, $sqlSort): IQueryBuilder { + foreach ($scopes as [$objectType, $objectIds, $chunkRoots]) { + $createQuery = function () use ($userSettings, $user, $activeFilter, $search, $objectType, $objectIds, $chunkRoots, $cursorTimestamp, $sqlSort): IQueryBuilder { $query = $this->connection->getQueryBuilder(); - $query->select('*')->from('activity', 'a'); + $query->select('a.*')->from('activity', 'a'); $this->applyStreamConditions($query, $userSettings, $user, 'all', $activeFilter, '', 0, null, false); $this->applyTeamSearchCriteria($query, $search ?? SearchCriteria::empty(), $objectType === 'circles'); $query->andWhere($query->expr()->eq('object_type', $query->createNamedParameter($objectType))); - $query->andWhere($query->expr()->in('object_id', $query->createNamedParameter($objectIds, IQueryBuilder::PARAM_INT_ARRAY))); + if ($objectType === 'files') { + $this->applyTeamFileRoots($query, $chunkRoots); + } else { + $query->andWhere($query->expr()->in('object_id', $query->createNamedParameter($objectIds, IQueryBuilder::PARAM_INT_ARRAY))); + } if ($cursorTimestamp !== null) { $parameter = $query->createNamedParameter($cursorTimestamp, IQueryBuilder::PARAM_INT); $query->andWhere($sqlSort === 'DESC' ? $query->expr()->lte('timestamp', $parameter) : $query->expr()->gte('timestamp', $parameter)); @@ -372,40 +385,77 @@ public function getTeam(GroupHelper $groupHelper, UserSettings $userSettings, st return $query; }; $queries[] = $createQuery; - $timestampQuery = $createQuery(); - $timestampQuery->select('timestamp')->groupBy('timestamp')->orderBy('timestamp', $sqlSort)->setMaxResults($limit + 2); - $result = $timestampQuery->executeQuery(); - foreach ($result->fetchAllAssociative() as $row) { - $timestamps[(int)$row['timestamp']] = (int)$row['timestamp']; - } - $result->closeCursor(); } - $sqlSort === 'DESC' ? rsort($timestamps) : sort($timestamps); $rows = []; - foreach ($timestamps as $timestamp) { - $bucket = []; + $filePaths = []; + $scannedTimestamp = null; + while (count($rows) <= $limit) { + $timestamps = []; + $mayHaveMore = false; foreach ($queries as $createQuery) { - $bucketQuery = $createQuery(); - $bucketQuery->andWhere($bucketQuery->expr()->eq('timestamp', $bucketQuery->createNamedParameter($timestamp, IQueryBuilder::PARAM_INT))); - $result = $bucketQuery->executeQuery(); + $timestampQuery = $createQuery(); + if ($scannedTimestamp !== null) { + $parameter = $timestampQuery->createNamedParameter($scannedTimestamp, IQueryBuilder::PARAM_INT); + $timestampQuery->andWhere($sqlSort === 'DESC' + ? $timestampQuery->expr()->lt('timestamp', $parameter) + : $timestampQuery->expr()->gt('timestamp', $parameter)); + } + $timestampQuery->select('timestamp')->groupBy('timestamp')->orderBy('timestamp', $sqlSort)->setMaxResults(self::TEAM_TIMESTAMP_BATCH_SIZE); + $result = $timestampQuery->executeQuery(); + $found = $result->fetchAllAssociative(); + $result->closeCursor(); + $mayHaveMore = $mayHaveMore || count($found) === self::TEAM_TIMESTAMP_BATCH_SIZE; + foreach ($found as $row) { + $timestamps[(int)$row['timestamp']] = (int)$row['timestamp']; + } + } + if ($timestamps === []) { + break; + } + $sqlSort === 'DESC' ? rsort($timestamps) : sort($timestamps); + $batch = array_slice($timestamps, 0, self::TEAM_TIMESTAMP_BATCH_SIZE); + // No scope filled its page and the union fits one batch: nothing is left after this batch. + $exhausted = !$mayHaveMore && count($timestamps) <= self::TEAM_TIMESTAMP_BATCH_SIZE; + // Every timestamp of any scope inside the batch range is part of the batch. + $buckets = array_fill_keys($batch, []); + foreach ($queries as $createQuery) { + $rangeQuery = $createQuery(); + $rangeQuery->andWhere( + $rangeQuery->expr()->gte('timestamp', $rangeQuery->createNamedParameter(min($batch), IQueryBuilder::PARAM_INT)), + $rangeQuery->expr()->lte('timestamp', $rangeQuery->createNamedParameter(max($batch), IQueryBuilder::PARAM_INT)), + ); + $result = $rangeQuery->executeQuery(); while ($row = $result->fetch()) { - $bucket[] = $this->normaliseTeamFileActivity($row, $scope->filePaths); + $buckets[(int)$row['timestamp']][] = $row; } $result->closeCursor(); } - $bucket = $this->deduplicateTeamActivities($bucket); - usort($bucket, static fn (array $left, array $right): int => $sqlSort === 'DESC' - ? (int)$right['activity_id'] <=> (int)$left['activity_id'] - : (int)$left['activity_id'] <=> (int)$right['activity_id']); - foreach ($bucket as $row) { - if ($timestamp === $cursorTimestamp && ($sqlSort === 'DESC' ? (int)$row['activity_id'] >= $since : (int)$row['activity_id'] <= $since)) { - continue; + foreach ($batch as $timestamp) { + $scannedTimestamp = $timestamp; + $bucket = []; + foreach ($buckets[$timestamp] as $row) { + $row = $this->prepareTeamActivity($scope, $row, $filePaths); + if ($row !== null) { + $bucket[] = $row; + } } - $rows[] = $row; - if (count($rows) > $limit) { - break 2; + $bucket = $this->deduplicateTeamActivities($bucket); + usort($bucket, static fn (array $left, array $right): int => $sqlSort === 'DESC' + ? (int)$right['activity_id'] <=> (int)$left['activity_id'] + : (int)$left['activity_id'] <=> (int)$right['activity_id']); + foreach ($bucket as $row) { + if ($timestamp === $cursorTimestamp && ($sqlSort === 'DESC' ? (int)$row['activity_id'] >= $since : (int)$row['activity_id'] <= $since)) { + continue; + } + $rows[] = $row; + if (count($rows) > $limit) { + break 3; + } } } + if ($exhausted) { + break; + } } $hasMore = count($rows) > $limit; @@ -422,6 +472,122 @@ public function getTeam(GroupHelper $groupHelper, UserSettings $userSettings, st return ['data' => $groupHelper->getActivities(), 'has_more' => $hasMore, 'headers' => $headers]; } + /** + * Read raw cache paths rather than mount-relative paths, which may be jailed. + * Single files need no cache lookup. + * + * @return list + */ + private function getTeamFileRoots(TeamActivityScope $scope): array { + $nodes = []; + foreach ($scope->fileScopes as $fileScope) { + $id = $fileScope->root->getId(); + if ($id !== null) { + $nodes[$id] = $fileScope->root; + } + } + $roots = []; + $folderIds = []; + foreach ($nodes as $id => $node) { + if ($node instanceof Folder) { + $folderIds[] = $id; + } else { + $roots[] = ['fileid' => $id, 'storage' => 0, 'path' => '', 'folder' => false]; + } + } + foreach (array_chunk($folderIds, self::TEAM_RESOURCE_OBJECT_CHUNK_SIZE) as $ids) { + $query = $this->connection->getQueryBuilder(); + $query->select('fileid', 'storage', 'path')->from('filecache') + ->where($query->expr()->in('fileid', $query->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))); + $result = $query->executeQuery(); + while ($row = $result->fetch()) { + $id = (int)$row['fileid']; + $path = (string)$row['path']; + if ($path === '' && $this->isHomeStorage($nodes[$id])) { + // Keep trash, versions and caches of a home storage out of the candidate set. + $path = 'files'; + } + $roots[] = ['fileid' => $id, 'storage' => (int)$row['storage'], 'path' => $path, 'folder' => true]; + } + $result->closeCursor(); + } + return $roots; + } + + private function isHomeStorage(Node $node): bool { + try { + return $node->getStorage()->instanceOfStorage(IHomeStorage::class); + } catch (NotFoundException) { + return false; + } + } + + /** @param non-empty-list $roots */ + private function applyTeamFileRoots(IQueryBuilder $query, array $roots): void { + $fileIds = []; + $conditions = []; + foreach ($roots as $root) { + if (!$root['folder']) { + $fileIds[] = $root['fileid']; + continue; + } + $storageCondition = $query->expr()->eq('f.storage', $query->createNamedParameter($root['storage'], IQueryBuilder::PARAM_INT)); + $conditions[] = $root['path'] === '' ? $storageCondition : $query->expr()->andX( + $storageCondition, + $query->expr()->orX( + $query->expr()->eq('f.fileid', $query->createNamedParameter($root['fileid'], IQueryBuilder::PARAM_INT)), + $query->expr()->like('f.path', $query->createNamedParameter($this->connection->escapeLikeParameter($root['path'] . '/') . '%')), + ), + ); + } + if ($conditions !== []) { + $query->innerJoin('a', 'filecache', 'f', $query->expr()->eq('a.object_id', 'f.fileid')); + } + if ($fileIds !== []) { + $conditions[] = $query->expr()->in('a.object_id', $query->createNamedParameter($fileIds, IQueryBuilder::PARAM_INT_ARRAY)); + } + $query->andWhere($query->expr()->orX(...$conditions)); + } + + /** + * Drop file activities the viewer cannot see and resolve the viewer's path + * for the file and every file referenced in its parameters. + * + * @param array $filePaths + */ + private function prepareTeamActivity(TeamActivityScope $scope, array $row, array &$filePaths): ?array { + if ($row['object_type'] === 'files') { + if ($this->getTeamFilePath($scope, (int)$row['object_id'], $filePaths) === null) { + return null; + } + $parameters = json_decode((string)$row['subjectparams'], true); + foreach (is_array($parameters) ? $parameters : [] as $parameter) { + foreach (is_array($parameter) ? array_keys($parameter) : [] as $id) { + if (is_numeric($id)) { + $this->getTeamFilePath($scope, (int)$id, $filePaths); + } + } + } + } + + return $this->normaliseTeamFileActivity($row, $filePaths); + } + + /** @param array $paths */ + private function getTeamFilePath(TeamActivityScope $scope, int $fileId, array &$paths): ?string { + if (!array_key_exists($fileId, $paths)) { + $paths[$fileId] = null; + foreach ($scope->fileScopes as $fileScope) { + $path = $fileScope->getVisiblePath($fileId); + if ($path !== null) { + $paths[$fileId] = $path; + break; + } + } + } + return $paths[$fileId]; + } + /** * Files publishes one recipient-specific row per user. A Team stream has no * single recipient, so turn the self-only wording into its actor form before diff --git a/lib/TeamActivityFileScope.php b/lib/TeamActivityFileScope.php new file mode 100644 index 000000000..c431302d0 --- /dev/null +++ b/lib/TeamActivityFileScope.php @@ -0,0 +1,72 @@ + */ + private array $paths = []; + /** @var array */ + private array $readable = []; + + public function __construct( + public readonly Node $root, + public readonly string $viewerPath, + ) { + } + + public function getVisiblePath(int $fileId): ?string { + if (array_key_exists($fileId, $this->paths)) { + return $this->paths[$fileId]; + } + $this->paths[$fileId] = null; + + try { + $nodes = $fileId === $this->root->getId() + ? [$this->root] + : ($this->root instanceof Folder ? $this->root->getById($fileId) : []); + $rootPath = rtrim($this->root->getPath(), '/'); + $mountPoint = $this->root->getMountPoint()->getMountPoint(); + foreach ($nodes as $node) { + $path = $node->getPath(); + if ($path !== $rootPath && !str_starts_with($path, $rootPath . '/')) { + continue; + } + $current = $node; + while (true) { + $currentPath = $current->getPath(); + if (!array_key_exists($currentPath, $this->readable)) { + $this->readable[$currentPath] = $current->getMountPoint()->getMountPoint() === $mountPoint + && $current->isReadable(); + } + if (!$this->readable[$currentPath]) { + continue 2; + } + if ($currentPath === $rootPath) { + $this->paths[$fileId] = rtrim($this->viewerPath, '/') . substr($path, strlen($rootPath)); + return $this->paths[$fileId]; + } + $current = $current->getParent(); + if ($current->getPath() !== $rootPath && !str_starts_with($current->getPath(), $rootPath . '/')) { + continue 2; + } + } + } + } catch (NotFoundException|NotPermittedException) { + // A file that disappeared or lost access is not part of this stream. + } + + return null; + } +} diff --git a/lib/TeamActivityResolver.php b/lib/TeamActivityResolver.php index dc32ae3ba..fb97ed9d3 100644 --- a/lib/TeamActivityResolver.php +++ b/lib/TeamActivityResolver.php @@ -9,7 +9,6 @@ namespace OCA\Activity; -use OCP\Files\Folder; use OCP\Files\IRootFolder; use OCP\Files\Node; use OCP\Files\NotFoundException; @@ -18,7 +17,6 @@ use OCP\Teams\ITeamActivityResourceProvider; use OCP\Teams\ITeamManager; use OCP\Teams\TeamActivityScope as ResourceActivityScope; -use OCP\Teams\TeamResource; use Psr\Log\LoggerInterface; class TeamActivityResolver { @@ -75,12 +73,8 @@ public function getScope(string $teamId, string $userId): ?TeamActivityScope { ]); } } - $filePaths = $this->getResourceObjects(array_values(array_unique($fileResourceIds)), $userId); - if ($filePaths !== []) { - array_unshift($resourceScopes, new ResourceActivityScope('files', array_keys($filePaths))); - } - - return new TeamActivityScope($circleObjectId, array_keys($filePaths), $filePaths, $resourceScopes); + $fileScopes = $this->getResourceObjects(array_values(array_unique($fileResourceIds)), $userId); + return new TeamActivityScope($circleObjectId, $fileScopes, $resourceScopes); } private function isMember(string $teamId, string $userId): bool { @@ -100,16 +94,19 @@ private function getCircleObjectId(string $teamId): ?int { /** * @param list $resourceIds - * @return array + * @return list */ private function getResourceObjects(array $resourceIds, string $userId): array { + if ($resourceIds === []) { + return []; + } try { $userFolder = $this->rootFolder->getUserFolder($userId); } catch (NotFoundException|NotPermittedException) { return []; } - $paths = []; + $scopes = []; foreach ($resourceIds as $resourceId) { try { $roots = $userFolder->getById($resourceId); @@ -120,32 +117,20 @@ private function getResourceObjects(array $resourceIds, string $userId): array { if (!$root instanceof Node) { continue; } - $visited = []; - $pending = [$root]; - while ($pending !== []) { - $node = array_pop($pending); - try { - $id = $node->getId(); - if ($id === null || isset($visited[$id]) || !$node->isReadable() - || $node->getMountPoint()->getMountPoint() !== $root->getMountPoint()->getMountPoint()) { - continue; - } - $visited[$id] = true; - $path = $userFolder->getRelativePath($node->getPath()); - if ($path === null) { - continue; - } - $paths[$id] ??= $path; - if ($node instanceof Folder) { - array_push($pending, ...$node->getDirectoryListing()); - } - } catch (NotFoundException|NotPermittedException) { + try { + if ($root->getId() === null || !$root->isReadable()) { continue; } + $path = $userFolder->getRelativePath($root->getPath()); + if ($path !== null) { + $scopes[] = new TeamActivityFileScope($root, $path); + } + } catch (NotFoundException|NotPermittedException) { + continue; } } } - return $paths; + return $scopes; } } diff --git a/lib/TeamActivityScope.php b/lib/TeamActivityScope.php index 831e43ac3..3ee257a26 100644 --- a/lib/TeamActivityScope.php +++ b/lib/TeamActivityScope.php @@ -11,31 +11,21 @@ /** * The internal Activity objects visible in a Team's stream. - * - * @psalm-type FileObjectIds = list */ class TeamActivityScope { /** - * @param FileObjectIds $fileObjectIds Filecache IDs reachable through Team resources - * @param array $filePaths Readable paths relative to the viewer's files + * @param list $fileScopes Accessible file roots, without enumerating their descendants * @param list<\OCP\Teams\TeamActivityScope> $resourceScopes Activity objects reachable through Team resources */ public function __construct( public readonly int $circleObjectId, - public readonly array $fileObjectIds = [], - public readonly array $filePaths = [], - array $resourceScopes = [], + public readonly array $fileScopes = [], + public readonly array $resourceScopes = [], ) { foreach ($resourceScopes as $resourceScope) { if (!$resourceScope instanceof \OCP\Teams\TeamActivityScope) { throw new \InvalidArgumentException('Team resource scopes must be Teams Activity scopes'); } } - $this->resourceScopes = $resourceScopes !== [] - ? $resourceScopes - : ($this->fileObjectIds === [] ? [] : [new \OCP\Teams\TeamActivityScope('files', $this->fileObjectIds)]); } - - /** @var list<\OCP\Teams\TeamActivityScope> */ - public readonly array $resourceScopes; } diff --git a/tests/Controller/APIv2ControllerTest.php b/tests/Controller/APIv2ControllerTest.php index 9ef768993..17ecc8f67 100644 --- a/tests/Controller/APIv2ControllerTest.php +++ b/tests/Controller/APIv2ControllerTest.php @@ -374,7 +374,7 @@ public function testGetTeamForwardsTheResolvedScope(): void { $user = $this->createMock(IUser::class); $user->method('getUID')->willReturn('alice'); $this->userSession->method('getUser')->willReturn($user); - $scope = new TeamActivityScope(42, [7, 8]); + $scope = new TeamActivityScope(42); $this->teamActivityResolver->expects($this->once()) ->method('getScope') ->with('team-single-id', 'alice') diff --git a/tests/TeamActivityDataTest.php b/tests/TeamActivityDataTest.php index b414cf663..e55366b99 100644 --- a/tests/TeamActivityDataTest.php +++ b/tests/TeamActivityDataTest.php @@ -18,14 +18,21 @@ use OC\SystemConfig; use OCA\Activity\Data; use OCA\Activity\GroupHelper; +use OCA\Activity\SearchCriteria; +use OCA\Activity\TeamActivityFileScope; use OCA\Activity\TeamActivityScope; use OCA\Activity\UserSettings; use OCP\Activity\Exceptions\FilterNotFoundException; use OCP\Activity\IManager; +use OCP\Files\File; +use OCP\Files\Folder; +use OCP\Files\IHomeStorage; use OCP\Files\IRootFolder; +use OCP\Files\Mount\IMountPoint; +use OCP\Files\Storage\IStorage; use OCP\IConfig; use OCP\IDBConnection; - use OCP\Teams\TeamActivityScope as ResourceActivityScope; +use OCP\Teams\TeamActivityScope as ResourceActivityScope; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; use Psr\Log\NullLogger; @@ -33,6 +40,10 @@ class TeamActivityDataTest extends TestCase { private DbalConnection $database; private Data $data; + /** @var list */ + private array $fileLookups = []; + /** @var list */ + private array $queries = []; protected function setUp(): void { parent::setUp(); @@ -41,12 +52,19 @@ protected function setUp(): void { } $this->database = DriverManager::getConnection(['driver' => 'pdo_sqlite', 'memory' => true]); $this->database->executeStatement('CREATE TABLE activity (activity_id INTEGER PRIMARY KEY, app TEXT, subject TEXT, subjectparams TEXT, message TEXT, messageparams TEXT, file TEXT, link TEXT, user TEXT, affecteduser TEXT, timestamp INTEGER, priority INTEGER, type TEXT, object_type TEXT, object_id INTEGER)'); + $this->database->executeStatement('CREATE INDEX activity_object ON activity (object_type, object_id)'); + $this->database->executeStatement('CREATE TABLE filecache (fileid INTEGER PRIMARY KEY, storage INTEGER, path TEXT)'); + $this->database->executeStatement('CREATE INDEX filecache_storage_path ON filecache (storage, path)'); $inner = $this->createMock(Connection::class); $inner->method('getDatabasePlatform')->willReturn($this->database->getDatabasePlatform()); $connection = $this->createMock(ConnectionAdapter::class); $connection->method('getInner')->willReturn($inner); $connection->method('getDatabaseProvider')->willReturn(IDBConnection::PLATFORM_SQLITE); - $connection->method('executeQuery')->willReturnCallback(fn ($sql, $parameters, $types) => new ResultAdapter($this->database->executeQuery($sql, $parameters, $types))); + $connection->method('escapeLikeParameter')->willReturnCallback(static fn (string $value): string => addcslashes($value, '\\_%')); + $connection->method('executeQuery')->willReturnCallback(function ($sql, $parameters, $types): ResultAdapter { + $this->queries[] = ['sql' => $sql, 'parameters' => $parameters, 'types' => $types]; + return new ResultAdapter($this->database->executeQuery($sql, $parameters, $types)); + }); $systemConfig = $this->createMock(SystemConfig::class); $connection->method('getQueryBuilder')->willReturnCallback(static function () use ($connection, $systemConfig): QueryBuilder { $query = new QueryBuilder($connection, $systemConfig, new NullLogger()); @@ -78,7 +96,7 @@ public function testPaginationDeduplicatesBeforeApplyingCursorAndLimit(int $limi 'file' => '', 'link' => '', 'user' => 'alice', 'affecteduser' => 'bob', 'priority' => 30, 'type' => 'member_added', 'object_type' => 'circles', 'object_id' => 42, ]); - $scope = new TeamActivityScope(42, [7, 8], [7 => '/Viewer/a.txt', 8 => '/Viewer/b.txt']); + $scope = $this->fileScope([7 => '/a.txt', 8 => '/b.txt']); $seen = []; $since = 0; $page = 0; @@ -111,18 +129,19 @@ public function testMovePathsAreRecipientIndependentButDistinctMovesRemain(): vo $this->insertFileEvent(10, 100, 7, 'moved_self', 'alice', '/Owner/New/a.txt', '/Owner/Old/a.txt'); $this->insertFileEvent(20, 100, 7, 'moved_by', 'bob', '/Recipient/New/a.txt', '/Recipient/Old/a.txt'); $this->insertFileEvent(30, 100, 7, 'moved_by', 'bob', '/Recipient/New/a.txt', '/Recipient/Other/a.txt'); - $scope = new TeamActivityScope(42, [7], [7 => '/Viewer/New/a.txt']); + $scope = $this->fileScope([7 => '/New/a.txt']); $result = $this->page($scope, 0, 10, 'asc'); $this->assertSame([10, 30], array_map('intval', array_column($result['data'], 'activity_id'))); $this->assertSame([[7 => '/Viewer/New/a.txt'], 'alice', [7 => '/Viewer/Old/a.txt']], json_decode($result['data'][0]['subjectparams'], true)); $this->assertSame([[7 => '/Viewer/New/a.txt'], 'alice', [7 => '/Viewer/Other/a.txt']], json_decode($result['data'][1]['subjectparams'], true)); } - public function testPaginationMergesMultipleFileIdChunks(): void { + public function testPaginationDeduplicatesOverlappingFileRoots(): void { $this->insertFileEvent(10, 100, 1, 'created_self', 'alice', '/Owner/a.txt'); $this->insertFileEvent(20, 100, 1, 'created_by', 'bob', '/Recipient/a.txt'); $this->insertFileEvent(30, 100, 501, 'created_by', 'bob', '/Recipient/b.txt'); - $scope = new TeamActivityScope(42, range(1, 501), [1 => '/Viewer/a.txt', 501 => '/Viewer/b.txt']); + $scope = $this->fileScope([1 => '/a.txt', 501 => '/b.txt']); + $scope = new TeamActivityScope(42, [$scope->fileScopes[0], $scope->fileScopes[0]]); $first = $this->page($scope, 0, 1, 'desc'); $this->assertSame(30, $first['headers']['X-Activity-Last-Given']); $this->assertTrue($first['has_more']); @@ -147,7 +166,7 @@ private function insertFileEvent(int $id, int $timestamp, int $fileId, string $s ]); } - private function page(TeamActivityScope $scope, int $since, int $limit, string $sort): array { + private function page(TeamActivityScope $scope, int $since, int $limit, string $sort, ?SearchCriteria $search = null): array { $activities = []; $helper = $this->createMock(GroupHelper::class); $helper->method('addActivity')->willReturnCallback(static function (array $row) use (&$activities): void { @@ -156,7 +175,7 @@ private function page(TeamActivityScope $scope, int $since, int $limit, string $ $helper->method('getActivities')->willReturnCallback(static function () use (&$activities): array { return $activities; }); - return $this->data->getTeam($helper, $this->createMock(UserSettings::class), 'viewer', $since, $limit, $sort, $scope); + return $this->data->getTeam($helper, $this->createMock(UserSettings::class), 'viewer', $since, $limit, $sort, $scope, $search); } public function testNonFileResourceScopesKeepTheirActivityPayload(): void { @@ -172,7 +191,7 @@ public function testNonFileResourceScopesKeepTheirActivityPayload(): void { 'file' => '', 'link' => '/apps/deck/#/board/8/card/71', 'user' => 'alice', 'affecteduser' => 'alice', 'priority' => 30, 'type' => 'deck', 'object_type' => 'deck_card', 'object_id' => 71, ]); - $scope = new TeamActivityScope(42, [], [], [new ResourceActivityScope('deck_card', [70])]); + $scope = new TeamActivityScope(42, [], [new ResourceActivityScope('deck_card', [70])]); $result = $this->page($scope, 0, 10, 'desc'); @@ -181,4 +200,277 @@ public function testNonFileResourceScopesKeepTheirActivityPayload(): void { self::assertSame('{"card":{"id":70,"title":"Team card"}}', $result['data'][0]['subjectparams']); } + /** + * @param array $paths Paths relative to the shared root + * @param list $denied + */ + private function fileScope(array $paths, array $denied = [], string $cacheRoot = 'files/Team', int $storage = 1, bool $homeStorage = false, int $rootId = 10000): TeamActivityScope { + $this->database->insert('filecache', ['fileid' => $rootId, 'storage' => $storage, 'path' => $cacheRoot]); + $mount = $this->createMock(IMountPoint::class); + $mount->method('getMountPoint')->willReturn('/viewer/files/Viewer/'); + $root = $this->createMock(Folder::class); + $root->method('getId')->willReturn($rootId); + $root->method('getPath')->willReturn('/viewer/files/Viewer'); + $root->method('getMountPoint')->willReturn($mount); + $root->method('isReadable')->willReturn(true); + $rootStorage = $this->createMock(IStorage::class); + $rootStorage->method('instanceOfStorage')->willReturnCallback(static fn (string $class): bool => $homeStorage && $class === IHomeStorage::class); + $root->method('getStorage')->willReturn($rootStorage); + $root->expects($this->never())->method('getDirectoryListing'); + $folders = ['' => $root]; + $nodes = []; + foreach ($paths as $id => $path) { + $this->database->insert('filecache', ['fileid' => $id, 'storage' => $storage, 'path' => ltrim($cacheRoot . $path, '/')]); + $parentPath = ''; + foreach (array_slice(explode('/', trim($path, '/')), 0, -1) as $segment) { + $folderPath = $parentPath . '/' . $segment; + if (!isset($folders[$folderPath])) { + $folder = $this->createMock(Folder::class); + $folder->method('getPath')->willReturn('/viewer/files/Viewer' . $folderPath); + $folder->method('getMountPoint')->willReturn($mount); + $folder->method('isReadable')->willReturn(true); + $folder->method('getParent')->willReturn($folders[$parentPath]); + $folders[$folderPath] = $folder; + } + $parentPath = $folderPath; + } + $node = $this->createMock(File::class); + $node->method('getId')->willReturn($id); + $node->method('getPath')->willReturn('/viewer/files/Viewer' . $path); + $node->method('getMountPoint')->willReturn($mount); + $node->method('isReadable')->willReturn(!in_array($id, $denied, true)); + $node->method('getParent')->willReturn($folders[$parentPath]); + $nodes[$id] = [$node]; + } + $root->method('getById')->willReturnCallback(function (int $id) use ($nodes): array { + $this->fileLookups[] = $id; + return $nodes[$id] ?? []; + }); + return new TeamActivityScope(42, [new TeamActivityFileScope($root, '/Viewer')]); + } + + #[DataProvider('pages')] + public function testPaginationRefillsAfterMoreThanOneBatchOfDeniedTimestamps(int $limit, string $sort): void { + $paths = []; + $denied = []; + for ($id = 1; $id <= 125; $id++) { + $paths[$id] = '/' . $id . '.txt'; + if (!in_array($id, [1, 63, 125], true)) { + $denied[] = $id; + } + $this->insertFileEvent($id, $id, $id, 'created_by', 'alice', '/Owner/' . $id . '.txt'); + } + $scope = $this->fileScope($paths, $denied); + $seen = []; + $since = 0; + do { + $result = $this->page($scope, $since, $limit, $sort); + foreach ($result['data'] as $row) { + $seen[] = (int)$row['activity_id']; + } + if ($result['has_more']) { + self::assertNotEmpty($result['data']); + $since = $result['headers']['X-Activity-Last-Given']; + } + } while ($result['has_more'] && count($seen) < 5); + self::assertSame($sort === 'asc' ? [1, 63, 125] : [125, 63, 1], $seen); + self::assertFalse($result['has_more']); + } + + public static function cacheRoots(): array { + return [ + 'shared folder' => ['files/Team'], + 'literal wildcards' => ['files/T_e%am'], + 'literal escape' => ['files/T\\eam'], + 'storage root' => [''], + ]; + } + + #[DataProvider('cacheRoots')] + public function testFilecacheRestrictsStorageAndSubtree(string $cacheRoot): void { + $scope = $this->fileScope([7 => '/a.txt'], [], $cacheRoot); + $this->insertFileEvent(1, 100, 7, 'created_by', 'alice', '/Owner/a.txt'); + foreach ([ + [8, 1, $cacheRoot . '2/a.txt'], + [9, 2, ltrim($cacheRoot . '/b.txt', '/')], + [10, 1, 'files/Txexxam/a.txt'], + ] as [$id, $storage, $path]) { + $this->database->insert('filecache', ['fileid' => $id, 'storage' => $storage, 'path' => $path]); + $this->insertFileEvent($id, 100, $id, 'created_by', 'alice', '/Other/a.txt'); + } + $this->insertFileEvent(11, 100, 11, 'deleted_by', 'alice', '/Deleted/a.txt'); + $result = $this->page($scope, 0, 10, 'desc'); + self::assertSame([1], array_map('intval', array_column($result['data'], 'activity_id'))); + sort($this->fileLookups); + self::assertSame($cacheRoot === '' ? [7, 8, 10] : [7], $this->fileLookups); + } + + public function testAllDeniedCandidatesProduceAnExhaustedEmptyPage(): void { + $scope = $this->fileScope([7 => '/a.txt'], [7]); + $this->insertFileEvent(1, 100, 7, 'created_by', 'alice', '/Owner/a.txt'); + $result = $this->page($scope, 0, 1, 'desc'); + self::assertSame([], $result['data']); + self::assertFalse($result['has_more']); + self::assertSame([], $result['headers']); + } + + public function testLargeSubtreeOnlyResolvesActivityCandidates(): void { + $scope = $this->fileScope([7 => '/a.txt']); + $this->database->beginTransaction(); + for ($id = 20000; $id < 30000; $id++) { + $this->database->insert('filecache', ['fileid' => $id, 'storage' => 1, 'path' => 'files/Team/unused/' . $id]); + } + $this->database->commit(); + $this->insertFileEvent(1, 100, 7, 'created_self', 'alice', '/Owner/a.txt'); + $this->insertFileEvent(2, 100, 7, 'created_by', 'bob', '/Recipient/a.txt'); + + $result = $this->page($scope, 0, 1, 'desc'); + + self::assertSame([1], array_map('intval', array_column($result['data'], 'activity_id'))); + self::assertFalse($result['has_more']); + self::assertSame([7], $this->fileLookups); + self::assertCount(5, $this->queries); + foreach ($this->queries as $query) { + self::assertLessThan(1000, strlen($query['sql'])); + } + } + + public function testIndividualFileScopeDoesNotIncludeSiblingFiles(): void { + $folderScope = $this->fileScope([7 => '/a.txt', 8 => '/b.txt']); + $file = $folderScope->fileScopes[0]->root->getById(7)[0]; + $scope = new TeamActivityScope(42, [new TeamActivityFileScope($file, '/Viewer/a.txt')]); + $this->insertFileEvent(1, 100, 7, 'created_by', 'alice', '/Owner/a.txt'); + $this->insertFileEvent(2, 100, 8, 'created_by', 'alice', '/Owner/b.txt'); + + $result = $this->page($scope, 0, 10, 'desc'); + + self::assertSame([1], array_map('intval', array_column($result['data'], 'activity_id'))); + } + + public function testFileSearchDateAndActorRestrictionsArePreserved(): void { + $scope = $this->fileScope([7 => '/a.txt', 8 => '/b.txt']); + $this->insertFileEvent(1, 100, 7, 'created_by', 'alice', '/Owner/a.txt'); + $this->insertFileEvent(2, 101, 8, 'created_by', 'alice', '/Owner/b.txt'); + $this->insertFileEvent(3, 102, 7, 'created_by', 'alice', '/Owner/a.txt'); + $result = $this->page($scope, 0, 10, 'desc', SearchCriteria::create('a.txt', 100, 101, 'alice')); + self::assertSame([1], array_map('intval', array_column($result['data'], 'activity_id'))); + } + + public function testNonFileResourceChunksStillMergeBeforePagination(): void { + foreach ([1 => 1, 2 => 501] as $activityId => $objectId) { + $this->insertFileEvent($activityId, 100, $objectId, 'created_by', 'alice', '/Unused'); + $this->database->update('activity', ['app' => 'deck', 'object_type' => 'deck_card', 'subject' => 'card_create'], ['activity_id' => $activityId]); + } + $scope = new TeamActivityScope(42, [], [new ResourceActivityScope('deck_card', range(1, 501))]); + $first = $this->page($scope, 0, 1, 'desc'); + self::assertSame(2, $first['headers']['X-Activity-Last-Given']); + self::assertTrue($first['has_more']); + $second = $this->page($scope, 2, 1, 'desc'); + self::assertSame(1, $second['headers']['X-Activity-Last-Given']); + self::assertFalse($second['has_more']); + } + + public function testTimestampBucketsAreNotTruncatedBeforeDeduplication(): void { + $scope = $this->fileScope([7 => '/a.txt', 8 => '/b.txt']); + for ($id = 1; $id <= 125; $id++) { + $this->insertFileEvent($id, 100, 7, 'created_by', 'recipient-' . $id, '/Recipient/a.txt'); + } + $this->insertFileEvent(126, 100, 8, 'created_by', 'alice', '/Owner/b.txt'); + $first = $this->page($scope, 0, 1, 'asc'); + self::assertSame(1, $first['headers']['X-Activity-Last-Given']); + self::assertTrue($first['has_more']); + $second = $this->page($scope, 1, 1, 'asc'); + self::assertSame(126, $second['headers']['X-Activity-Last-Given']); + self::assertFalse($second['has_more']); + } + + public function testHomeStorageRootExcludesTrashVersionsAndCaches(): void { + $scope = $this->fileScope([], [], '', 1, true); + foreach ([[20, 'files/a.txt'], [21, 'files_trashbin/files/b.txt'], [22, 'files_versions/a.txt.v1'], [23, 'cache/c']] as [$id, $path]) { + $this->database->insert('filecache', ['fileid' => $id, 'storage' => 1, 'path' => $path]); + $this->insertFileEvent($id, 100, $id, 'created_by', 'alice', '/Owner/' . $id); + } + + $this->page($scope, 0, 10, 'desc'); + + self::assertSame([20], $this->fileLookups); + } + + public function testManyFileRootsAreSplitIntoBoundedQueries(): void { + $scope = $this->fileScope([7 => '/a.txt']); + $fileScopes = $scope->fileScopes; + for ($i = 0; $i < 250; $i++) { + $this->database->insert('filecache', ['fileid' => 20000 + $i, 'storage' => 1, 'path' => 'files/Filler' . $i]); + $fileScopes[] = new TeamActivityFileScope($this->filler(Folder::class, 20000 + $i), '/Filler' . $i); + $fileScopes[] = new TeamActivityFileScope($this->filler(File::class, 40000 + $i), '/Single' . $i); + } + $this->insertFileEvent(1, 100, 7, 'created_by', 'alice', '/Owner/a.txt'); + + $result = $this->page(new TeamActivityScope(42, $fileScopes), 0, 10, 'desc'); + + self::assertSame([1], array_map('intval', array_column($result['data'], 'activity_id'))); + foreach ($this->queries as $query) { + $count = 0; + array_walk_recursive($query['parameters'], static function () use (&$count): void { + $count++; + }); + self::assertLessThan(400, $count); + } + } + + public function testQueryCountDoesNotGrowWithTheNumberOfTimestamps(): void { + $paths = []; + for ($id = 1; $id <= 30; $id++) { + $paths[$id] = '/' . $id . '.txt'; + $this->insertFileEvent($id, $id, $id, 'created_by', 'alice', '/Owner/' . $id . '.txt'); + } + $scope = $this->fileScope($paths); + + $result = $this->page($scope, 0, 200, 'desc'); + + self::assertCount(30, $result['data']); + self::assertCount(5, $this->queries); + } + + public function testRootsOnDifferentStoragesOnlyMatchTheirOwnSubtree(): void { + $first = $this->fileScope([7 => '/a.txt'], [], 'files/Team', 1); + $second = $this->fileScope([8 => '/b.txt'], [], 'files/Other', 2, false, 10001); + $scope = new TeamActivityScope(42, [$first->fileScopes[0], $second->fileScopes[0]]); + $this->insertFileEvent(1, 100, 7, 'created_by', 'alice', '/Owner/a.txt'); + $this->insertFileEvent(2, 101, 8, 'created_by', 'alice', '/Owner/b.txt'); + foreach ([[9, 2, 'files/Team/x.txt'], [10, 1, 'files/Other/y.txt']] as [$id, $storage, $path]) { + $this->database->insert('filecache', ['fileid' => $id, 'storage' => $storage, 'path' => $path]); + $this->insertFileEvent($id + 10, 102, $id, 'created_by', 'alice', '/Owner/' . $id); + } + + $result = $this->page($scope, 0, 10, 'asc'); + + self::assertSame([1, 2], array_map('intval', array_column($result['data'], 'activity_id'))); + self::assertNotContains(9, $this->fileLookups); + self::assertNotContains(10, $this->fileLookups); + } + + public function testExactlyOneBatchOfTimestampsIsNotTruncated(): void { + $paths = []; + for ($id = 1; $id <= 50; $id++) { + $paths[$id] = '/' . $id . '.txt'; + $this->insertFileEvent($id, $id, $id, 'created_by', 'alice', '/Owner/' . $id . '.txt'); + } + $scope = $this->fileScope($paths); + + $result = $this->page($scope, 0, 200, 'desc'); + + self::assertCount(50, $result['data']); + self::assertFalse($result['has_more']); + } + + /** + * @param class-string<\OCP\Files\Node> $class + */ + private function filler(string $class, int $id): \OCP\Files\Node { + $node = $this->createMock($class); + $node->method('getId')->willReturn($id); + $node->method('getPath')->willReturn('/viewer/files/Filler' . $id); + return $node; + } } diff --git a/tests/TeamActivityFileScopeTest.php b/tests/TeamActivityFileScopeTest.php new file mode 100644 index 000000000..eb0207488 --- /dev/null +++ b/tests/TeamActivityFileScopeTest.php @@ -0,0 +1,101 @@ + [true, true, '/viewer/files/Team/Child/a.txt', '/viewer/files/Team/', '/Team/Child/a.txt'], + 'denied file' => [false, true, '/viewer/files/Team/Child/a.txt', '/viewer/files/Team/', null], + 'denied ancestor' => [true, false, '/viewer/files/Team/Child/a.txt', '/viewer/files/Team/', null], + 'nested mount' => [true, true, '/viewer/files/Team/Child/a.txt', '/viewer/files/Team/Child/', null], + 'other resource' => [true, true, '/viewer/files/Team2/a.txt', '/viewer/files/Team/', null], + ]; + } + + #[DataProvider('visibility')] + public function testVisibilityChecksTheWholePathAndMount(bool $readable, bool $parentReadable, string $path, string $mountPoint, ?string $expected): void { + $mount = $this->createMock(IMountPoint::class); + $mount->method('getMountPoint')->willReturn('/viewer/files/Team/'); + $root = $this->createMock(Folder::class); + $root->method('getId')->willReturn(100); + $root->method('getPath')->willReturn('/viewer/files/Team'); + $root->method('getMountPoint')->willReturn($mount); + $root->method('isReadable')->willReturn(true); + $root->expects($this->never())->method('getDirectoryListing'); + $parent = $this->createMock(Folder::class); + $parent->method('getPath')->willReturn('/viewer/files/Team/Child'); + $parent->method('getMountPoint')->willReturn($mount); + $parent->method('isReadable')->willReturn($parentReadable); + $parent->method('getParent')->willReturn($root); + $parent->expects($this->never())->method('getDirectoryListing'); + $fileMount = $this->createMock(IMountPoint::class); + $fileMount->method('getMountPoint')->willReturn($mountPoint); + $file = $this->createMock(File::class); + $file->method('getPath')->willReturn($path); + $file->method('getMountPoint')->willReturn($fileMount); + $file->method('isReadable')->willReturn($readable); + $file->method('getParent')->willReturn($parent); + $root->expects($this->once())->method('getById')->with(7)->willReturn([$file]); + $scope = new TeamActivityFileScope($root, '/Team'); + + self::assertSame($expected, $scope->getVisiblePath(7)); + self::assertSame($expected, $scope->getVisiblePath(7)); + } + + public function testAnIndividualFileDoesNotIncludeOtherFiles(): void { + $mount = $this->createMock(IMountPoint::class); + $mount->method('getMountPoint')->willReturn('/viewer/files/'); + $file = $this->createMock(File::class); + $file->method('getId')->willReturn(7); + $file->method('getPath')->willReturn('/viewer/files/a.txt'); + $file->method('getMountPoint')->willReturn($mount); + $file->method('isReadable')->willReturn(true); + $scope = new TeamActivityFileScope($file, '/a.txt'); + + self::assertSame('/a.txt', $scope->getVisiblePath(7)); + self::assertNull($scope->getVisiblePath(8)); + } + + public function testMissingOrInaccessibleCandidatesAreExcluded(): void { + foreach ([new NotFoundException(), new NotPermittedException()] as $exception) { + $root = $this->createMock(Folder::class); + $root->method('getId')->willReturn(100); + $root->method('getById')->willThrowException($exception); + $scope = new TeamActivityFileScope($root, '/Team'); + self::assertNull($scope->getVisiblePath(7)); + } + } + + public function testOtherMountsAreTriedAfterAnUnreadableMount(): void { + $scopes = []; + foreach ([false, true] as $readable) { + $mount = $this->createMock(IMountPoint::class); + $mount->method('getMountPoint')->willReturn($readable ? '/Second/' : '/First/'); + $root = $this->createMock(File::class); + $root->method('getId')->willReturn(7); + $root->method('getPath')->willReturn($readable ? '/Second/a.txt' : '/First/a.txt'); + $root->method('getMountPoint')->willReturn($mount); + $root->method('isReadable')->willReturn($readable); + $scopes[] = new TeamActivityFileScope($root, $readable ? '/Second/a.txt' : '/First/a.txt'); + } + self::assertNull($scopes[0]->getVisiblePath(7)); + self::assertSame('/Second/a.txt', $scopes[1]->getVisiblePath(7)); + } +} diff --git a/tests/TeamActivityResolverTest.php b/tests/TeamActivityResolverTest.php index b626d0739..40919a4f2 100644 --- a/tests/TeamActivityResolverTest.php +++ b/tests/TeamActivityResolverTest.php @@ -10,16 +10,16 @@ namespace OCA\Activity\Tests; use OCA\Activity\TeamActivityResolver; -use OCP\Files\Folder; +use OCP\DB\IResult; +use OCP\DB\QueryBuilder\IExpressionBuilder; +use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\Files\File; +use OCP\Files\Folder; use OCP\Files\IRootFolder; use OCP\Files\IUserFolder; use OCP\Files\Mount\IMountPoint; use OCP\Files\NotFoundException; use OCP\IDBConnection; -use OCP\DB\IResult; -use OCP\DB\QueryBuilder\IExpressionBuilder; -use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\Teams\ITeamActivityResourceProvider; use OCP\Teams\ITeamManager; use OCP\Teams\ITeamResourceProvider; @@ -43,11 +43,10 @@ public function testNonMemberCannotResolveActivityObjects(): void { self::assertNull($resolver->getScope('team-1', 'alice')); } - public function testFolderScopeExcludesUnreadableSubtrees(): void { + public function testFolderScopeDoesNotEnumerateDescendants(): void { $rootFolder = $this->createMock(IRootFolder::class); $userFolder = $this->createMock(IUserFolder::class); $folder = $this->createMock(Folder::class); - $denied = $this->createMock(Folder::class); $mount = $this->createMock(IMountPoint::class); $mount->method('getMountPoint')->willReturn('/alice/files/Team/'); $rootFolder->method('getUserFolder')->with('alice')->willReturn($userFolder); @@ -56,17 +55,17 @@ public function testFolderScopeExcludesUnreadableSubtrees(): void { $folder->method('getId')->willReturn(200); $folder->method('isReadable')->willReturn(true); $folder->method('getMountPoint')->willReturn($mount); - $folder->method('getDirectoryListing')->willReturn([$denied]); - $denied->method('getId')->willReturn(201); - $denied->method('isReadable')->willReturn(false); - $denied->expects($this->never())->method('getDirectoryListing'); + $folder->expects($this->never())->method('getDirectoryListing'); $resolver = new TeamActivityResolver($this->createMock(ITeamManager::class), $this->createMock(IDBConnection::class), $rootFolder, $this->createMock(LoggerInterface::class)); $method = new \ReflectionMethod($resolver, 'getResourceObjects'); - self::assertSame([200 => '/Team'], $method->invoke($resolver, [200], 'alice')); + $scopes = $method->invoke($resolver, [200], 'alice'); + self::assertCount(1, $scopes); + self::assertSame($folder, $scopes[0]->root); + self::assertSame('/Team', $scopes[0]->viewerPath); } - public function testEachMountIsCheckedAndNestedMountsAreExcluded(): void { + public function testEachMountIsKeptWithoutEnumeratingDescendants(): void { $rootFolder = $this->createMock(IRootFolder::class); $userFolder = $this->createMock(IUserFolder::class); $rootFolder->method('getUserFolder')->willReturn($userFolder); @@ -80,25 +79,15 @@ public function testEachMountIsCheckedAndNestedMountsAreExcluded(): void { $root->method('getPath')->willReturn($path); $root->method('isReadable')->willReturn(true); $root->method('getMountPoint')->willReturn($mount); - $child = $this->createMock(Folder::class); - $child->method('getId')->willReturn(201); - $child->method('getPath')->willReturn($path . '/Child'); - $child->method('isReadable')->willReturn($path === '/Second'); - $child->method('getMountPoint')->willReturn($mount); - $foreign = $this->createMock(Folder::class); - $foreign->method('getId')->willReturn(300); - $foreign->method('isReadable')->willReturn(true); - $foreignMount = $this->createMock(IMountPoint::class); - $foreignMount->method('getMountPoint')->willReturn($path . '/Shared'); - $foreign->method('getMountPoint')->willReturn($foreignMount); - $foreign->expects($this->never())->method('getDirectoryListing'); - $root->method('getDirectoryListing')->willReturn([$child, $foreign]); + $root->expects($this->never())->method('getDirectoryListing'); $roots[] = $root; } $userFolder->method('getById')->willReturn($roots); $resolver = new TeamActivityResolver($this->createMock(ITeamManager::class), $this->createMock(IDBConnection::class), $rootFolder, $this->createMock(LoggerInterface::class)); $method = new \ReflectionMethod($resolver, 'getResourceObjects'); - self::assertSame([200 => '/First', 201 => '/Second/Child'], $method->invoke($resolver, [200], 'alice')); + $scopes = $method->invoke($resolver, [200], 'alice'); + self::assertSame($roots, array_column($scopes, 'root')); + self::assertSame(['/First', '/Second'], array_column($scopes, 'viewerPath')); } public function testMissingUserFolderHasNoFileScope(): void { @@ -109,12 +98,11 @@ public function testMissingUserFolderHasNoFileScope(): void { self::assertSame([], $method->invoke($resolver, [200], 'alice')); } - public function testResourceScopeIncludesFileRootsAndFolderSubtrees(): void { + public function testResourceScopeIncludesOnlyReadableRoots(): void { $rootFolder = $this->createMock(IRootFolder::class); $userFolder = $this->createMock(IUserFolder::class); $folder = $this->createMock(Folder::class); $file = $this->createMock(File::class); - $child = $this->createMock(File::class); $mount = $this->createMock(IMountPoint::class); $mount->method('getMountPoint')->willReturn('/alice/files/'); $rootFolder->method('getUserFolder')->with('alice')->willReturn($userFolder); @@ -122,19 +110,20 @@ public function testResourceScopeIncludesFileRootsAndFolderSubtrees(): void { $userFolder->method('getRelativePath')->willReturnMap([ ['/alice/files/readme.md', '/readme.md'], ['/alice/files/Shared', '/Shared'], - ['/alice/files/Shared/note.md', '/Shared/note.md'], ]); - foreach ([[$file, 100, '/alice/files/readme.md'], [$folder, 200, '/alice/files/Shared'], [$child, 201, '/alice/files/Shared/note.md']] as [$node, $id, $path]) { + foreach ([[$file, 100, '/alice/files/readme.md'], [$folder, 200, '/alice/files/Shared']] as [$node, $id, $path]) { $node->method('getId')->willReturn($id); $node->method('getPath')->willReturn($path); - $node->method('isReadable')->willReturn(true); + $node->method('isReadable')->willReturn($node !== $folder); $node->method('getMountPoint')->willReturn($mount); } - $folder->method('getDirectoryListing')->willReturn([$child]); + $folder->expects($this->never())->method('getDirectoryListing'); $resolver = new TeamActivityResolver($this->createMock(ITeamManager::class), $this->createMock(IDBConnection::class), $rootFolder, $this->createMock(LoggerInterface::class)); $method = new \ReflectionMethod($resolver, 'getResourceObjects'); - self::assertSame([100 => '/readme.md', 200 => '/Shared', 201 => '/Shared/note.md'], $method->invoke($resolver, [100, 200], 'alice')); + $scopes = $method->invoke($resolver, [100, 200], 'alice'); + self::assertCount(1, $scopes); + self::assertSame($file, $scopes[0]->root); } public function testScopeUsesOnlyVisibleFileResources(): void { @@ -203,14 +192,12 @@ public function testScopeUsesOnlyVisibleFileResources(): void { self::assertNotNull($scope); self::assertSame(42, $scope->circleObjectId); - self::assertSame([100, 200], $scope->fileObjectIds); - self::assertSame([100 => '/readme.md', 200 => '/Team/readme.md'], $scope->filePaths); - self::assertSame('files', $scope->resourceScopes[0]->getObjectType()); - self::assertSame([100, 200], $scope->resourceScopes[0]->getObjectIds()); - self::assertSame('deck_board', $scope->resourceScopes[1]->getObjectType()); - self::assertSame([200], $scope->resourceScopes[1]->getObjectIds()); - self::assertSame('deck_card', $scope->resourceScopes[2]->getObjectType()); - self::assertSame([300, 301], $scope->resourceScopes[2]->getObjectIds()); + self::assertSame([$file, $groupFolderFile], array_column($scope->fileScopes, 'root')); + self::assertSame(['/readme.md', '/Team/readme.md'], array_column($scope->fileScopes, 'viewerPath')); + self::assertSame('deck_board', $scope->resourceScopes[0]->getObjectType()); + self::assertSame([200], $scope->resourceScopes[0]->getObjectIds()); + self::assertSame('deck_card', $scope->resourceScopes[1]->getObjectType()); + self::assertSame([300, 301], $scope->resourceScopes[1]->getObjectIds()); } public function testProviderFailureIsLoggedAndOtherScopesAreKept(): void {