diff --git a/changelog/unreleased/41574 b/changelog/unreleased/41574 new file mode 100644 index 00000000000..39b6397e0b2 --- /dev/null +++ b/changelog/unreleased/41574 @@ -0,0 +1,8 @@ +Bugfix: Fix subadmin email change updating caller's address instead of target's + +The verification token and confirmation link in the subadmin path of +setMailAddress were associated with the caller's account instead of the +target user's account. Clicking the confirmation link changed the +subadmin's email rather than the intended target's email. + +https://github.com/owncloud/core/pull/41574 diff --git a/settings/Controller/UsersController.php b/settings/Controller/UsersController.php index b688e0fd554..06382de4444 100644 --- a/settings/Controller/UsersController.php +++ b/settings/Controller/UsersController.php @@ -985,7 +985,7 @@ public function setMailAddress($id, $mailAddress) { } try { - if ($this->sendEmail($userId, $mailAddress)) { + if ($this->sendEmail($id, $mailAddress)) { return new DataResponse( [ 'status' => 'success', diff --git a/tests/Settings/Controller/UsersControllerTest.php b/tests/Settings/Controller/UsersControllerTest.php index aff872116e9..12ea792b58e 100644 --- a/tests/Settings/Controller/UsersControllerTest.php +++ b/tests/Settings/Controller/UsersControllerTest.php @@ -2276,6 +2276,81 @@ public function testSetEmailAddress($mailAddress, $isValid, $expectsUpdate, $can $this->assertSame($responseCode, $response->getStatus()); } + /** + * When a subadmin calls setMailAddress for a different user, the verification + * email and token must be stored under the target user's ID, not the caller's. + * Regression test for GHSA-5945-5fp8-4283 (subadmin path). + */ + public function testSetMailAddressSubadminSendsEmailToTargetNotCaller(): void { + $this->container['IsAdmin'] = false; + + $callerUser = $this->createMock(User::class); + $callerUser->method('getUID')->willReturn('subadmin'); + + $targetUser = $this->createMock(User::class); + $targetUser->method('getUID')->willReturn('targetuser'); + $targetUser->method('canChangeMailAddress')->willReturn(true); + + $subAdmin = $this->createMock(SubAdmin::class); + $subAdmin->method('isUserAccessible')->willReturn(true); + + $this->container['GroupManager'] + ->method('getSubAdmin') + ->willReturn($subAdmin); + + $this->container['UserSession'] + ->method('getUser') + ->willReturn($callerUser); + + $this->container['UserManager'] + ->method('get') + ->willReturnCallback(function ($id) use ($callerUser, $targetUser) { + $userMap = ['subadmin' => $callerUser, 'targetuser' => $targetUser]; + return $userMap[$id] ?? null; + }); + + $this->container['Mailer'] + ->method('validateMailAddress') + ->willReturn(true); + + // Token must be read from and stored under 'targetuser', not 'subadmin' + $this->container['Config'] + ->method('getUserValue') + ->with('targetuser', 'owncloud', 'changeMail') + ->willReturn(''); + + $this->container['Config'] + ->expects($this->once()) + ->method('setUserValue') + ->with('targetuser', 'owncloud', 'changeMail', $this->anything()); + + $this->container['TimeFactory'] + ->method('getTime') + ->willReturn(12345); + + $this->container['SecureRandom'] + ->method('generate') + ->willReturn('SomeToken'); + + // Confirmation link must carry 'targetuser', not 'subadmin' + $this->container['URLGenerator'] + ->expects($this->once()) + ->method('linkToRouteAbsolute') + ->with('settings.Users.changeMail', $this->callback(function ($params) { + return isset($params['userId']) && $params['userId'] === 'targetuser'; + })) + ->willReturn('https://example.com/changeMail'); + + $message = $this->createMock(Message::class); + $this->container['Mailer'] + ->method('createMessage') + ->willReturn($message); + + $response = $this->container['UsersController']->setMailAddress('targetuser', 'new@example.com'); + $this->assertSame(Http::STATUS_OK, $response->getStatus()); + $this->assertSame('success', $response->getData()['status']); + } + public function testStatsAdmin(): void { $this->container['IsAdmin'] = true;