From 80204297d39ec25f455c82bfb2df2ed323718ccd Mon Sep 17 00:00:00 2001 From: provokateurin Date: Thu, 27 Mar 2025 09:31:01 +0100 Subject: [PATCH] fix(settings): Handle email change restriction separately from display name change restriction Co-authored-by: provokateurin Co-authored-by: Ferdinand Thiessen Co-authored-by: Louis Signed-off-by: Ferdinand Thiessen --- .../lib/Controller/UsersController.php | 38 ++++---- .../tests/Controller/UsersControllerTest.php | 92 +++++++++++++++++-- .../lib/Settings/Personal/PersonalInfo.php | 1 + .../EmailSection/EmailSection.vue | 6 +- build/psalm-baseline.xml | 10 ++ lib/private/User/LazyUser.php | 4 + lib/private/User/User.php | 5 + 7 files changed, 126 insertions(+), 30 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index a0578f7d5c4..39c6df4160e 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -708,14 +708,16 @@ class UsersController extends AUserData { $targetUser = $currentLoggedInUser; } - // Editing self (display, email) - if ($this->config->getSystemValue('allow_user_to_change_display_name', true) !== false) { - if ( - $targetUser->getBackend() instanceof ISetDisplayNameBackend - || $targetUser->getBackend()->implementsActions(Backend::SET_DISPLAYNAME) - ) { - $permittedFields[] = IAccountManager::PROPERTY_DISPLAYNAME; - } + $allowDisplayNameChange = $this->config->getSystemValue('allow_user_to_change_display_name', true); + if ($allowDisplayNameChange === true && ( + $targetUser->getBackend() instanceof ISetDisplayNameBackend + || $targetUser->getBackend()->implementsActions(Backend::SET_DISPLAYNAME) + )) { + $permittedFields[] = IAccountManager::PROPERTY_DISPLAYNAME; + } + + // Fallback to display name value to avoid changing behavior with the new option. + if ($this->config->getSystemValue('allow_user_to_change_email', $allowDisplayNameChange)) { $permittedFields[] = IAccountManager::PROPERTY_EMAIL; } @@ -862,15 +864,17 @@ class UsersController extends AUserData { $permittedFields = []; if ($targetUser->getUID() === $currentLoggedInUser->getUID()) { - // Editing self (display, email) - if ($this->config->getSystemValue('allow_user_to_change_display_name', true) !== false) { - if ( - $targetUser->getBackend() instanceof ISetDisplayNameBackend - || $targetUser->getBackend()->implementsActions(Backend::SET_DISPLAYNAME) - ) { - $permittedFields[] = self::USER_FIELD_DISPLAYNAME; - $permittedFields[] = IAccountManager::PROPERTY_DISPLAYNAME; - } + $allowDisplayNameChange = $this->config->getSystemValue('allow_user_to_change_display_name', true); + if ($allowDisplayNameChange !== false && ( + $targetUser->getBackend() instanceof ISetDisplayNameBackend + || $targetUser->getBackend()->implementsActions(Backend::SET_DISPLAYNAME) + )) { + $permittedFields[] = self::USER_FIELD_DISPLAYNAME; + $permittedFields[] = IAccountManager::PROPERTY_DISPLAYNAME; + } + + // Fallback to display name value to avoid changing behavior with the new option. + if ($this->config->getSystemValue('allow_user_to_change_email', $allowDisplayNameChange)) { $permittedFields[] = IAccountManager::PROPERTY_EMAIL; } diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php index 3bfea4a1194..8f91c118716 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -74,6 +74,7 @@ use OCP\User\Backend\ISetDisplayNameBackend; use OCP\UserInterface; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; +use RuntimeException; use Test\TestCase; class UsersControllerTest extends TestCase { @@ -1679,6 +1680,8 @@ class UsersControllerTest extends TestCase { ->method('getBackend') ->willReturn($backend); + $this->config->method('getSystemValue')->willReturnCallback(fn (string $key, mixed $default) => $default); + $this->assertEquals([], $this->api->editUser('UserToEdit', 'email', 'demo@nextcloud.com')->getData()); } @@ -1873,6 +1876,8 @@ class UsersControllerTest extends TestCase { ->method('getBackend') ->willReturn($backend); + $this->config->method('getSystemValue')->willReturnCallback(fn (string $key, mixed $default) => $default); + $this->api->editUser('UserToEdit', 'email', 'demo.org'); } @@ -4244,7 +4249,8 @@ class UsersControllerTest extends TestCase { public function dataGetEditableFields() { return [ - [false, ISetDisplayNameBackend::class, [ + [false, true, ISetDisplayNameBackend::class, [ + IAccountManager::PROPERTY_EMAIL, IAccountManager::COLLECTION_EMAIL, IAccountManager::PROPERTY_PHONE, IAccountManager::PROPERTY_ADDRESS, @@ -4257,8 +4263,49 @@ class UsersControllerTest extends TestCase { IAccountManager::PROPERTY_BIOGRAPHY, IAccountManager::PROPERTY_PROFILE_ENABLED, ]], - [true, ISetDisplayNameBackend::class, [ + [true, false, ISetDisplayNameBackend::class, [ IAccountManager::PROPERTY_DISPLAYNAME, + IAccountManager::COLLECTION_EMAIL, + IAccountManager::PROPERTY_PHONE, + IAccountManager::PROPERTY_ADDRESS, + IAccountManager::PROPERTY_WEBSITE, + IAccountManager::PROPERTY_TWITTER, + IAccountManager::PROPERTY_FEDIVERSE, + IAccountManager::PROPERTY_ORGANISATION, + IAccountManager::PROPERTY_ROLE, + IAccountManager::PROPERTY_HEADLINE, + IAccountManager::PROPERTY_BIOGRAPHY, + IAccountManager::PROPERTY_PROFILE_ENABLED, + ]], + [true, true, ISetDisplayNameBackend::class, [ + IAccountManager::PROPERTY_DISPLAYNAME, + IAccountManager::PROPERTY_EMAIL, + IAccountManager::COLLECTION_EMAIL, + IAccountManager::PROPERTY_PHONE, + IAccountManager::PROPERTY_ADDRESS, + IAccountManager::PROPERTY_WEBSITE, + IAccountManager::PROPERTY_TWITTER, + IAccountManager::PROPERTY_FEDIVERSE, + IAccountManager::PROPERTY_ORGANISATION, + IAccountManager::PROPERTY_ROLE, + IAccountManager::PROPERTY_HEADLINE, + IAccountManager::PROPERTY_BIOGRAPHY, + IAccountManager::PROPERTY_PROFILE_ENABLED, + ]], + [false, false, ISetDisplayNameBackend::class, [ + IAccountManager::COLLECTION_EMAIL, + IAccountManager::PROPERTY_PHONE, + IAccountManager::PROPERTY_ADDRESS, + IAccountManager::PROPERTY_WEBSITE, + IAccountManager::PROPERTY_TWITTER, + IAccountManager::PROPERTY_FEDIVERSE, + IAccountManager::PROPERTY_ORGANISATION, + IAccountManager::PROPERTY_ROLE, + IAccountManager::PROPERTY_HEADLINE, + IAccountManager::PROPERTY_BIOGRAPHY, + IAccountManager::PROPERTY_PROFILE_ENABLED, + ]], + [false, true, UserInterface::class, [ IAccountManager::PROPERTY_EMAIL, IAccountManager::COLLECTION_EMAIL, IAccountManager::PROPERTY_PHONE, @@ -4272,7 +4319,20 @@ class UsersControllerTest extends TestCase { IAccountManager::PROPERTY_BIOGRAPHY, IAccountManager::PROPERTY_PROFILE_ENABLED, ]], - [true, UserInterface::class, [ + [true, false, UserInterface::class, [ + IAccountManager::COLLECTION_EMAIL, + IAccountManager::PROPERTY_PHONE, + IAccountManager::PROPERTY_ADDRESS, + IAccountManager::PROPERTY_WEBSITE, + IAccountManager::PROPERTY_TWITTER, + IAccountManager::PROPERTY_FEDIVERSE, + IAccountManager::PROPERTY_ORGANISATION, + IAccountManager::PROPERTY_ROLE, + IAccountManager::PROPERTY_HEADLINE, + IAccountManager::PROPERTY_BIOGRAPHY, + IAccountManager::PROPERTY_PROFILE_ENABLED, + ]], + [true, true, UserInterface::class, [ IAccountManager::PROPERTY_EMAIL, IAccountManager::COLLECTION_EMAIL, IAccountManager::PROPERTY_PHONE, @@ -4286,6 +4346,19 @@ class UsersControllerTest extends TestCase { IAccountManager::PROPERTY_BIOGRAPHY, IAccountManager::PROPERTY_PROFILE_ENABLED, ]], + [false, false, UserInterface::class, [ + IAccountManager::COLLECTION_EMAIL, + IAccountManager::PROPERTY_PHONE, + IAccountManager::PROPERTY_ADDRESS, + IAccountManager::PROPERTY_WEBSITE, + IAccountManager::PROPERTY_TWITTER, + IAccountManager::PROPERTY_FEDIVERSE, + IAccountManager::PROPERTY_ORGANISATION, + IAccountManager::PROPERTY_ROLE, + IAccountManager::PROPERTY_HEADLINE, + IAccountManager::PROPERTY_BIOGRAPHY, + IAccountManager::PROPERTY_PROFILE_ENABLED, + ]], ]; } @@ -4296,13 +4369,12 @@ class UsersControllerTest extends TestCase { * @param string $userBackend * @param array $expected */ - public function testGetEditableFields(bool $allowedToChangeDisplayName, string $userBackend, array $expected) { - $this->config - ->method('getSystemValue') - ->with( - $this->equalTo('allow_user_to_change_display_name'), - $this->anything() - )->willReturn($allowedToChangeDisplayName); + public function testGetEditableFields(bool $allowedToChangeDisplayName, bool $allowedToChangeEmail, string $userBackend, array $expected): void { + $this->config->method('getSystemValue')->willReturnCallback(fn (string $key, mixed $default) => match ($key) { + 'allow_user_to_change_display_name' => $allowedToChangeDisplayName, + 'allow_user_to_change_email' => $allowedToChangeEmail, + default => throw new RuntimeException('Unexpected system config key: ' . $key), + }); $user = $this->createMock(IUser::class); $this->userSession->method('getUser') diff --git a/apps/settings/lib/Settings/Personal/PersonalInfo.php b/apps/settings/lib/Settings/Personal/PersonalInfo.php index 7d16c9645a9..e0d241497b2 100644 --- a/apps/settings/lib/Settings/Personal/PersonalInfo.php +++ b/apps/settings/lib/Settings/Personal/PersonalInfo.php @@ -172,6 +172,7 @@ class PersonalInfo implements ISettings { $accountParameters = [ 'avatarChangeSupported' => $user->canChangeAvatar(), 'displayNameChangeSupported' => $user->canChangeDisplayName(), + 'emailChangeSupported' => $user->canChangeEmail(), 'federationEnabled' => $federationEnabled, 'lookupServerUploadEnabled' => $lookupServerUploadEnabled, ]; diff --git a/apps/settings/src/components/PersonalInfo/EmailSection/EmailSection.vue b/apps/settings/src/components/PersonalInfo/EmailSection/EmailSection.vue index 55b68fa5933..4870969cf6d 100644 --- a/apps/settings/src/components/PersonalInfo/EmailSection/EmailSection.vue +++ b/apps/settings/src/components/PersonalInfo/EmailSection/EmailSection.vue @@ -31,7 +31,7 @@ :scope.sync="primaryEmail.scope" @add-additional="onAddAdditionalEmail" /> -