Skip to content

fix(search): search as the user the provider is given - #2936

Open
jospoortvliet wants to merge 1 commit into
nextcloud:mainfrom
jospoortvliet:fix/search-provider-honour-user
Open

fix(search): search as the user the provider is given#2936
jospoortvliet wants to merge 1 commit into
nextcloud:mainfrom
jospoortvliet:fix/search-provider-honour-user

Conversation

@jospoortvliet

Copy link
Copy Markdown
Member

Both services already take the parameter, so this passes the argument that already exists rather than changing any API.

The problem

SearchTablesProvider receives the account to search as, but calls TableService::search() and ViewService::search() without it. Both accept an optional $userId and fall back to PermissionsService::preCheckUserId(null), which resolves the session user so any caller searching on behalf of someone else silently received its own results instead.

I assume this is not intentional, just 'good enough' for unified search.

Some more details:

  • lib/Search/SearchTablesProvider.php:82 and :93 call TableService::search($term, $limit, $offset) and ViewService::search($term, $limit, $offset).
  • Both signatures end in ?string $userId = null and, when null, call PermissionsService::preCheckUserId(null)$this->userId, the session user.
  • The provider's own search(IUser $user, ISearchQuery $query) is handed the account to search as, and already uses it for the isEnabledForUser() check on the line above.

Through the unified search UI the session user and $user are always the same, so nothing is visibly broken today. But it breaks when you need to search on behalf of another account, like a background job, or a compliance tool that must search within a defined set of accounts.

this PR

The change in this PR is very simple:

Pass $user->getUID() to both calls. No API change; both services already accept it.

Tests

New tests/unit/Search/SearchTablesProviderTest.php:

  • asserts both services are called with the given uid
  • asserts neither is called when the app is disabled for that user

Verified the test fails against the unfixed provider and passes with it.

Checks run

  • phpunit -c tests/unit/phpunit.xml --filter SearchTablesProviderTest → 2 passed (run inside the dev container; the bootstrap needs a database)
  • php -l on both files
  • php-cs-fixer --dry-run on both files → no violations

🖼️ Screenshots

🏚️ Before 🏡 After
B A

🏁 Checklist

  • ⛑️ Tests (unit and/or integration) are included or not needed
  • 🔙 Backport requests are created or not needed: /backport to stableX.X
  • 📅 Milestone is set
  • 🌸 PR title is meaningful (if it should be in the changelog: is it meaningful to users?)

🤖 AI (if applicable)

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

The change and its test were produced with Claude Code (claude-opus-5).

SearchTablesProvider receives the account to search as, but called
TableService::search() and ViewService::search() without it. Both accept an
optional $userId and fall back to PermissionsService::preCheckUserId(null),
which resolves the session user — so any caller searching on behalf of someone
else silently received its own results instead.

This has no effect on unified search, where the two are always the same person.
It matters for callers that search on behalf of another account, such as
background jobs or compliance tooling, where returning the caller's own tables
is not a degraded result but a wrong one.

Both services already take the parameter, so this passes the argument that
already exists rather than changing any API.

Assisted-by: ClaudeCode:claude-opus-5

Signed-off-by: Jos Poortvliet <jospoortvliet@gmail.com>
@enjeck enjeck added AI assisted 3. to review Waiting for reviews labels Aug 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews AI assisted

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants