fix(settings): pass target user ID to sendEmail in setMailAddress subadmin path

[DeepDiver1975]

Summary

• Fixes the subadmin path of UsersController::setMailAddress which was passing $userId (the caller's session UID) to sendEmail() instead of $id (the target user's UID)
• The verification token and confirmation link were being stored/generated under the caller's account, causing the caller's email to be changed instead of the target's when the link was clicked
• The admin path was already corrected in commit 3ff2884; this fixes the remaining subadmin path
• Adds a regression test testSetMailAddressSubadminSendsEmailToTargetNotCaller that asserts the token is stored and the confirmation link is generated with the target user's ID

Fixes GHSA-5945-5fp8-4283 (subadmin path).

Test plan

• Run tests/Settings/Controller/UsersControllerTest.php::testSetMailAddressSubadminSendsEmailToTargetNotCaller — verifies setUserValue and linkToRouteAbsolute are called with the target user's ID, not the caller's
• Run existing testSetEmailAddress suite to confirm no regression in admin path
• Manual: as a subadmin, change a group member's email address and confirm the verification email is sent to the new address with a link that changes the member's email, not the subadmin's

🤖 Generated with Claude Code

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[DeepDiver1975]

Code Review

Security fix for GHSA-5945-5fp8-4283 (subadmin path). +84/−1, 3 files.

Overview

UsersController::setMailAddress($id, ...) carries two identities:

• $userId = caller's session UID ($this->userSession->getUser()->getUID())
• $id = the target user whose email is being changed

The non-admin verification branch was calling sendEmail($userId, ...), so the change-mail token and confirmation link were keyed to the caller, not the target. The one-line fix switches it to sendEmail($id, ...).

Correctness — fix is right ✅

Traced the full chain (setMailAddress → sendEmail → changeMail):

• sendEmail($userId) stores the token via setUserValue($userId, 'owncloud', 'changeMail', ...) and builds the link with ['userId' => $userId, ...].
• changeMail($token, $userId) guards with if ($user->getUID() !== $sessionUser->getUID()) and finally calls setEmailAddress($userId, $mailAddress).

In the buggy subadmin path the token + link were stored under the subadmin, so the confirmation email went to the new address but the link carried userId=subadmin — the only way it completes is the subadmin clicking it, which then changes the subadmin's own email. The legitimate target change could never complete. The fix correctly keys everything to $id.

Sanity checks that confirm it's safe and complete:

• Self-service unaffected: when a user edits their own email, $id === $userId, so behavior is identical.
• $userId still used in the access-control check (if ($userId !== $id && !$this->isAdmin)) — no dead variable introduced.
• Admin path short-circuits to setEmailAddress and never reaches sendEmail, consistent with the admin path already being fixed in 3ff2884.

Test quality — catches the regression ✅

The test fails pre-fix, not just passes post-fix: getUserValue, setUserValue (->expects($this->once())), and linkToRouteAbsolute are all constrained to 'targetuser' / a callback asserting userId === 'targetuser'. Pre-fix the code passes 'subadmin', violating those constraints → failure. Correct way to pin the behavior.

Suggestions (optional, non-blocking)

• Rate-limit path: the test mocks getUserValue → '' (no prior token). Consider a case asserting the rate-limit branch also reads from the target (recent token → false / "already sent recently"). Pre-fix this read the caller's token, so a subadmin's own recent change could suppress a target's email.
• Assert recipient: add a $message->setTo([$mailAddress => 'targetuser']) expectation to document the recipient label follows the target too.
• Changelog: format/content matches conventions 👍. Since this is a tracked GHSA, consider whether a Security: line / advisory reference belongs here per the project's security-disclosure convention.

Risk

Low. One-character logic change on a narrow branch, covered by a targeted regression test, with self-service and admin paths provably unaffected.

Verdict

Correct, minimal, well-tested security fix. Ready to merge — suggestions above are optional hardening.

🤖 Generated with Claude Code