From 32fd379b66fb12e8fa6043d69a9f1a7b1b6fc36a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Fri, 29 Jan 2021 20:32:41 +0100 Subject: [PATCH 01/11] Use constants from interface rather than class MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Calviño Sánchez --- .../lib/Controller/UsersController.php | 33 +++++++-------- .../tests/Controller/UsersControllerTest.php | 41 ++++++++++--------- 2 files changed, 38 insertions(+), 36 deletions(-) diff --git a/apps/settings/lib/Controller/UsersController.php b/apps/settings/lib/Controller/UsersController.php index a24a7c3ee2..98baed01dd 100644 --- a/apps/settings/lib/Controller/UsersController.php +++ b/apps/settings/lib/Controller/UsersController.php @@ -46,6 +46,7 @@ use OCA\FederatedFileSharing\FederatedShareProvider; use OCA\Settings\BackgroundJobs\VerifyUserData; use OCA\Settings\Events\BeforeTemplateRenderedEvent; use OCA\User_LDAP\User_Proxy; +use OCP\Accounts\IAccountManager; use OCP\App\IAppManager; use OCP\AppFramework\Controller; use OCP\AppFramework\Http\DataResponse; @@ -374,18 +375,18 @@ class UsersController extends Controller { } $user = $this->userSession->getUser(); $data = $this->accountManager->getUser($user); - $data[AccountManager::PROPERTY_AVATAR] = ['scope' => $avatarScope]; + $data[IAccountManager::PROPERTY_AVATAR] = ['scope' => $avatarScope]; if ($this->config->getSystemValue('allow_user_to_change_display_name', true) !== false) { - $data[AccountManager::PROPERTY_DISPLAYNAME] = ['value' => $displayname, 'scope' => $displaynameScope]; - $data[AccountManager::PROPERTY_EMAIL] = ['value' => $email, 'scope' => $emailScope]; + $data[IAccountManager::PROPERTY_DISPLAYNAME] = ['value' => $displayname, 'scope' => $displaynameScope]; + $data[IAccountManager::PROPERTY_EMAIL] = ['value' => $email, 'scope' => $emailScope]; } if ($this->appManager->isEnabledForUser('federatedfilesharing')) { $shareProvider = \OC::$server->query(FederatedShareProvider::class); if ($shareProvider->isLookupServerUploadEnabled()) { - $data[AccountManager::PROPERTY_WEBSITE] = ['value' => $website, 'scope' => $websiteScope]; - $data[AccountManager::PROPERTY_ADDRESS] = ['value' => $address, 'scope' => $addressScope]; - $data[AccountManager::PROPERTY_PHONE] = ['value' => $phone, 'scope' => $phoneScope]; - $data[AccountManager::PROPERTY_TWITTER] = ['value' => $twitter, 'scope' => $twitterScope]; + $data[IAccountManager::PROPERTY_WEBSITE] = ['value' => $website, 'scope' => $websiteScope]; + $data[IAccountManager::PROPERTY_ADDRESS] = ['value' => $address, 'scope' => $addressScope]; + $data[IAccountManager::PROPERTY_PHONE] = ['value' => $phone, 'scope' => $phoneScope]; + $data[IAccountManager::PROPERTY_TWITTER] = ['value' => $twitter, 'scope' => $twitterScope]; } } try { @@ -395,15 +396,15 @@ class UsersController extends Controller { 'status' => 'success', 'data' => [ 'userId' => $user->getUID(), - 'avatarScope' => $data[AccountManager::PROPERTY_AVATAR]['scope'], - 'displayname' => $data[AccountManager::PROPERTY_DISPLAYNAME]['value'], - 'displaynameScope' => $data[AccountManager::PROPERTY_DISPLAYNAME]['scope'], - 'email' => $data[AccountManager::PROPERTY_EMAIL]['value'], - 'emailScope' => $data[AccountManager::PROPERTY_EMAIL]['scope'], - 'website' => $data[AccountManager::PROPERTY_WEBSITE]['value'], - 'websiteScope' => $data[AccountManager::PROPERTY_WEBSITE]['scope'], - 'address' => $data[AccountManager::PROPERTY_ADDRESS]['value'], - 'addressScope' => $data[AccountManager::PROPERTY_ADDRESS]['scope'], + 'avatarScope' => $data[IAccountManager::PROPERTY_AVATAR]['scope'], + 'displayname' => $data[IAccountManager::PROPERTY_DISPLAYNAME]['value'], + 'displaynameScope' => $data[IAccountManager::PROPERTY_DISPLAYNAME]['scope'], + 'email' => $data[IAccountManager::PROPERTY_EMAIL]['value'], + 'emailScope' => $data[IAccountManager::PROPERTY_EMAIL]['scope'], + 'website' => $data[IAccountManager::PROPERTY_WEBSITE]['value'], + 'websiteScope' => $data[IAccountManager::PROPERTY_WEBSITE]['scope'], + 'address' => $data[IAccountManager::PROPERTY_ADDRESS]['value'], + 'addressScope' => $data[IAccountManager::PROPERTY_ADDRESS]['scope'], 'message' => $this->l10n->t('Settings saved') ] ], diff --git a/apps/settings/tests/Controller/UsersControllerTest.php b/apps/settings/tests/Controller/UsersControllerTest.php index 4679fd8f7b..ace0774090 100644 --- a/apps/settings/tests/Controller/UsersControllerTest.php +++ b/apps/settings/tests/Controller/UsersControllerTest.php @@ -31,6 +31,7 @@ use OC\Accounts\AccountManager; use OC\Encryption\Exceptions\ModuleDoesNotExistsException; use OC\Group\Manager; use OCA\Settings\Controller\UsersController; +use OCP\Accounts\IAccountManager; use OCP\App\IAppManager; use OCP\AppFramework\Http; use OCP\BackgroundJob\IJobList; @@ -196,45 +197,45 @@ class UsersControllerTest extends \Test\TestCase { ->method('getUser') ->with($user) ->willReturn([ - AccountManager::PROPERTY_DISPLAYNAME => + IAccountManager::PROPERTY_DISPLAYNAME => [ 'value' => 'Display name', - 'scope' => AccountManager::VISIBILITY_CONTACTS_ONLY, - 'verified' => AccountManager::NOT_VERIFIED, + 'scope' => IAccountManager::VISIBILITY_CONTACTS_ONLY, + 'verified' => IAccountManager::NOT_VERIFIED, ], - AccountManager::PROPERTY_ADDRESS => + IAccountManager::PROPERTY_ADDRESS => [ 'value' => '', - 'scope' => AccountManager::VISIBILITY_PRIVATE, - 'verified' => AccountManager::NOT_VERIFIED, + 'scope' => IAccountManager::VISIBILITY_PRIVATE, + 'verified' => IAccountManager::NOT_VERIFIED, ], - AccountManager::PROPERTY_WEBSITE => + IAccountManager::PROPERTY_WEBSITE => [ 'value' => '', - 'scope' => AccountManager::VISIBILITY_PRIVATE, - 'verified' => AccountManager::NOT_VERIFIED, + 'scope' => IAccountManager::VISIBILITY_PRIVATE, + 'verified' => IAccountManager::NOT_VERIFIED, ], - AccountManager::PROPERTY_EMAIL => + IAccountManager::PROPERTY_EMAIL => [ 'value' => '', - 'scope' => AccountManager::VISIBILITY_CONTACTS_ONLY, - 'verified' => AccountManager::NOT_VERIFIED, + 'scope' => IAccountManager::VISIBILITY_CONTACTS_ONLY, + 'verified' => IAccountManager::NOT_VERIFIED, ], - AccountManager::PROPERTY_AVATAR => + IAccountManager::PROPERTY_AVATAR => [ - 'scope' => AccountManager::VISIBILITY_CONTACTS_ONLY + 'scope' => IAccountManager::VISIBILITY_CONTACTS_ONLY ], - AccountManager::PROPERTY_PHONE => + IAccountManager::PROPERTY_PHONE => [ 'value' => '', - 'scope' => AccountManager::VISIBILITY_PRIVATE, - 'verified' => AccountManager::NOT_VERIFIED, + 'scope' => IAccountManager::VISIBILITY_PRIVATE, + 'verified' => IAccountManager::NOT_VERIFIED, ], - AccountManager::PROPERTY_TWITTER => + IAccountManager::PROPERTY_TWITTER => [ 'value' => '', - 'scope' => AccountManager::VISIBILITY_PRIVATE, - 'verified' => AccountManager::NOT_VERIFIED, + 'scope' => IAccountManager::VISIBILITY_PRIVATE, + 'verified' => IAccountManager::NOT_VERIFIED, ], ]); From 91e06bc08af767d9f5a1b4d4a6a76219c7825c01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Fri, 29 Jan 2021 20:34:10 +0100 Subject: [PATCH 02/11] Extract default test data to a helper getter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Calviño Sánchez --- .../tests/Controller/UsersControllerTest.php | 88 ++++++++++--------- 1 file changed, 46 insertions(+), 42 deletions(-) diff --git a/apps/settings/tests/Controller/UsersControllerTest.php b/apps/settings/tests/Controller/UsersControllerTest.php index ace0774090..763226dd0a 100644 --- a/apps/settings/tests/Controller/UsersControllerTest.php +++ b/apps/settings/tests/Controller/UsersControllerTest.php @@ -172,6 +172,51 @@ class UsersControllerTest extends \Test\TestCase { } } + protected function getDefaultAccountManagerUserData() { + return [ + IAccountManager::PROPERTY_DISPLAYNAME => + [ + 'value' => 'Display name', + 'scope' => IAccountManager::VISIBILITY_CONTACTS_ONLY, + 'verified' => IAccountManager::NOT_VERIFIED, + ], + IAccountManager::PROPERTY_ADDRESS => + [ + 'value' => '', + 'scope' => IAccountManager::VISIBILITY_PRIVATE, + 'verified' => IAccountManager::NOT_VERIFIED, + ], + IAccountManager::PROPERTY_WEBSITE => + [ + 'value' => '', + 'scope' => IAccountManager::VISIBILITY_PRIVATE, + 'verified' => IAccountManager::NOT_VERIFIED, + ], + IAccountManager::PROPERTY_EMAIL => + [ + 'value' => '', + 'scope' => IAccountManager::VISIBILITY_CONTACTS_ONLY, + 'verified' => IAccountManager::NOT_VERIFIED, + ], + IAccountManager::PROPERTY_AVATAR => + [ + 'scope' => IAccountManager::VISIBILITY_CONTACTS_ONLY + ], + IAccountManager::PROPERTY_PHONE => + [ + 'value' => '', + 'scope' => IAccountManager::VISIBILITY_PRIVATE, + 'verified' => IAccountManager::NOT_VERIFIED, + ], + IAccountManager::PROPERTY_TWITTER => + [ + 'value' => '', + 'scope' => IAccountManager::VISIBILITY_PRIVATE, + 'verified' => IAccountManager::NOT_VERIFIED, + ], + ]; + } + /** * @dataProvider dataTestSetUserSettings * @@ -196,48 +241,7 @@ class UsersControllerTest extends \Test\TestCase { $this->accountManager->expects($this->once()) ->method('getUser') ->with($user) - ->willReturn([ - IAccountManager::PROPERTY_DISPLAYNAME => - [ - 'value' => 'Display name', - 'scope' => IAccountManager::VISIBILITY_CONTACTS_ONLY, - 'verified' => IAccountManager::NOT_VERIFIED, - ], - IAccountManager::PROPERTY_ADDRESS => - [ - 'value' => '', - 'scope' => IAccountManager::VISIBILITY_PRIVATE, - 'verified' => IAccountManager::NOT_VERIFIED, - ], - IAccountManager::PROPERTY_WEBSITE => - [ - 'value' => '', - 'scope' => IAccountManager::VISIBILITY_PRIVATE, - 'verified' => IAccountManager::NOT_VERIFIED, - ], - IAccountManager::PROPERTY_EMAIL => - [ - 'value' => '', - 'scope' => IAccountManager::VISIBILITY_CONTACTS_ONLY, - 'verified' => IAccountManager::NOT_VERIFIED, - ], - IAccountManager::PROPERTY_AVATAR => - [ - 'scope' => IAccountManager::VISIBILITY_CONTACTS_ONLY - ], - IAccountManager::PROPERTY_PHONE => - [ - 'value' => '', - 'scope' => IAccountManager::VISIBILITY_PRIVATE, - 'verified' => IAccountManager::NOT_VERIFIED, - ], - IAccountManager::PROPERTY_TWITTER => - [ - 'value' => '', - 'scope' => IAccountManager::VISIBILITY_PRIVATE, - 'verified' => IAccountManager::NOT_VERIFIED, - ], - ]); + ->willReturn($this->getDefaultAccountManagerUserData()); $controller->expects($this->once())->method('saveUserSettings'); } else { From 1c8295610b54be0a5c1f6b241549dde6069d597b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Fri, 29 Jan 2021 20:37:14 +0100 Subject: [PATCH 03/11] Change default test data to values less similar to empty values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Right now it makes no difference, but this should make future tests clearer, specially in case of failure. Signed-off-by: Daniel Calviño Sánchez --- .../tests/Controller/UsersControllerTest.php | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/apps/settings/tests/Controller/UsersControllerTest.php b/apps/settings/tests/Controller/UsersControllerTest.php index 763226dd0a..4d262e1dfd 100644 --- a/apps/settings/tests/Controller/UsersControllerTest.php +++ b/apps/settings/tests/Controller/UsersControllerTest.php @@ -176,27 +176,27 @@ class UsersControllerTest extends \Test\TestCase { return [ IAccountManager::PROPERTY_DISPLAYNAME => [ - 'value' => 'Display name', + 'value' => 'Default display name', 'scope' => IAccountManager::VISIBILITY_CONTACTS_ONLY, - 'verified' => IAccountManager::NOT_VERIFIED, + 'verified' => IAccountManager::VERIFIED, ], IAccountManager::PROPERTY_ADDRESS => [ - 'value' => '', + 'value' => 'Default address', 'scope' => IAccountManager::VISIBILITY_PRIVATE, - 'verified' => IAccountManager::NOT_VERIFIED, + 'verified' => IAccountManager::VERIFIED, ], IAccountManager::PROPERTY_WEBSITE => [ - 'value' => '', + 'value' => 'Default website', 'scope' => IAccountManager::VISIBILITY_PRIVATE, - 'verified' => IAccountManager::NOT_VERIFIED, + 'verified' => IAccountManager::VERIFIED, ], IAccountManager::PROPERTY_EMAIL => [ - 'value' => '', + 'value' => 'Default email', 'scope' => IAccountManager::VISIBILITY_CONTACTS_ONLY, - 'verified' => IAccountManager::NOT_VERIFIED, + 'verified' => IAccountManager::VERIFIED, ], IAccountManager::PROPERTY_AVATAR => [ @@ -204,15 +204,15 @@ class UsersControllerTest extends \Test\TestCase { ], IAccountManager::PROPERTY_PHONE => [ - 'value' => '', + 'value' => 'Default phone', 'scope' => IAccountManager::VISIBILITY_PRIVATE, - 'verified' => IAccountManager::NOT_VERIFIED, + 'verified' => IAccountManager::VERIFIED, ], IAccountManager::PROPERTY_TWITTER => [ - 'value' => '', + 'value' => 'Default twitter', 'scope' => IAccountManager::VISIBILITY_PRIVATE, - 'verified' => IAccountManager::NOT_VERIFIED, + 'verified' => IAccountManager::VERIFIED, ], ]; } From bedb97fd57ca1c56b84c2b1056bbb28ea26035fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Fri, 29 Jan 2021 21:21:54 +0100 Subject: [PATCH 04/11] Add more unit tests for setting user settings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Calviño Sánchez --- .../tests/Controller/UsersControllerTest.php | 145 ++++++++++++++++++ 1 file changed, 145 insertions(+) diff --git a/apps/settings/tests/Controller/UsersControllerTest.php b/apps/settings/tests/Controller/UsersControllerTest.php index 4d262e1dfd..342452d1c0 100644 --- a/apps/settings/tests/Controller/UsersControllerTest.php +++ b/apps/settings/tests/Controller/UsersControllerTest.php @@ -276,6 +276,151 @@ class UsersControllerTest extends \Test\TestCase { ]; } + public function testSetUserSettingsWhenUserDisplayNameChangeNotAllowed() { + $controller = $this->getController(false, ['saveUserSettings']); + $user = $this->createMock(IUser::class); + + $this->userSession->method('getUser')->willReturn($user); + + $defaultProperties = $this->getDefaultAccountManagerUserData(); + + $this->accountManager->expects($this->once()) + ->method('getUser') + ->with($user) + ->willReturn($defaultProperties); + + $this->config->expects($this->once()) + ->method('getSystemValue') + ->with('allow_user_to_change_display_name') + ->willReturn(false); + + $this->appManager->expects($this->once()) + ->method('isEnabledForUser') + ->with('federatedfilesharing') + ->willReturn(true); + + $avatarScope = IAccountManager::VISIBILITY_PUBLIC; + $displayName = 'Display name'; + $displayNameScope = IAccountManager::VISIBILITY_PUBLIC; + $phone = '47658468'; + $phoneScope = IAccountManager::VISIBILITY_PUBLIC; + $email = 'john@example.com'; + $emailScope = IAccountManager::VISIBILITY_PUBLIC; + $website = 'nextcloud.com'; + $websiteScope = IAccountManager::VISIBILITY_PUBLIC; + $address = 'street and city'; + $addressScope = IAccountManager::VISIBILITY_PUBLIC; + $twitter = '@nextclouders'; + $twitterScope = IAccountManager::VISIBILITY_PUBLIC; + + // Display name and email are not changed. + $expectedProperties = $defaultProperties; + $expectedProperties[IAccountManager::PROPERTY_AVATAR]['scope'] = $avatarScope; + $expectedProperties[IAccountManager::PROPERTY_PHONE]['value'] = $phone; + $expectedProperties[IAccountManager::PROPERTY_PHONE]['scope'] = $phoneScope; + unset($expectedProperties[IAccountManager::PROPERTY_PHONE]['verified']); + $expectedProperties[IAccountManager::PROPERTY_WEBSITE]['value'] = $website; + $expectedProperties[IAccountManager::PROPERTY_WEBSITE]['scope'] = $websiteScope; + unset($expectedProperties[IAccountManager::PROPERTY_WEBSITE]['verified']); + $expectedProperties[IAccountManager::PROPERTY_ADDRESS]['value'] = $address; + $expectedProperties[IAccountManager::PROPERTY_ADDRESS]['scope'] = $addressScope; + unset($expectedProperties[IAccountManager::PROPERTY_ADDRESS]['verified']); + $expectedProperties[IAccountManager::PROPERTY_TWITTER]['value'] = $twitter; + $expectedProperties[IAccountManager::PROPERTY_TWITTER]['scope'] = $twitterScope; + unset($expectedProperties[IAccountManager::PROPERTY_TWITTER]['verified']); + + $this->mailer->expects($this->once())->method('validateMailAddress') + ->willReturn(true); + + $controller->expects($this->once()) + ->method('saveUserSettings') + ->with($user, $expectedProperties) + ->willReturnArgument(1); + + $result = $controller->setUserSettings( + $avatarScope, + $displayName, + $displayNameScope, + $phone, + $phoneScope, + $email, + $emailScope, + $website, + $websiteScope, + $address, + $addressScope, + $twitter, + $twitterScope + ); + } + + public function testSetUserSettingsWhenFederatedFilesharingNotEnabled() { + $controller = $this->getController(false, ['saveUserSettings']); + $user = $this->createMock(IUser::class); + + $this->userSession->method('getUser')->willReturn($user); + + $defaultProperties = $this->getDefaultAccountManagerUserData(); + + $this->accountManager->expects($this->once()) + ->method('getUser') + ->with($user) + ->willReturn($defaultProperties); + + $this->appManager->expects($this->once()) + ->method('isEnabledForUser') + ->with('federatedfilesharing') + ->willReturn(false); + + $avatarScope = IAccountManager::VISIBILITY_PUBLIC; + $displayName = 'Display name'; + $displayNameScope = IAccountManager::VISIBILITY_PUBLIC; + $phone = '47658468'; + $phoneScope = IAccountManager::VISIBILITY_PUBLIC; + $email = 'john@example.com'; + $emailScope = IAccountManager::VISIBILITY_PUBLIC; + $website = 'nextcloud.com'; + $websiteScope = IAccountManager::VISIBILITY_PUBLIC; + $address = 'street and city'; + $addressScope = IAccountManager::VISIBILITY_PUBLIC; + $twitter = '@nextclouders'; + $twitterScope = IAccountManager::VISIBILITY_PUBLIC; + + // Phone, website, address and twitter are not changed. + $expectedProperties = $defaultProperties; + $expectedProperties[IAccountManager::PROPERTY_AVATAR]['scope'] = $avatarScope; + $expectedProperties[IAccountManager::PROPERTY_DISPLAYNAME]['value'] = $displayName; + $expectedProperties[IAccountManager::PROPERTY_DISPLAYNAME]['scope'] = $displayNameScope; + unset($expectedProperties[IAccountManager::PROPERTY_DISPLAYNAME]['verified']); + $expectedProperties[IAccountManager::PROPERTY_EMAIL]['value'] = $email; + $expectedProperties[IAccountManager::PROPERTY_EMAIL]['scope'] = $emailScope; + unset($expectedProperties[IAccountManager::PROPERTY_EMAIL]['verified']); + + $this->mailer->expects($this->once())->method('validateMailAddress') + ->willReturn(true); + + $controller->expects($this->once()) + ->method('saveUserSettings') + ->with($user, $expectedProperties) + ->willReturnArgument(1); + + $result = $controller->setUserSettings( + $avatarScope, + $displayName, + $displayNameScope, + $phone, + $phoneScope, + $email, + $emailScope, + $website, + $websiteScope, + $address, + $addressScope, + $twitter, + $twitterScope + ); + } + /** * @dataProvider dataTestSaveUserSettings * From 0dfd0ec77a4acd11da16e47eaddfaf2fc9a52900 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Fri, 29 Jan 2021 22:11:16 +0100 Subject: [PATCH 05/11] Respect additional user settings not covered by the controller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "AccountManager::updateUser()" wipes previous user data with whichever user data is given (except for some adjustments, like resetting the verified status when needed). As the controller overrode the properties those properties would lose some of their attributes even if they are not affected by the changes made by the controller. Now the controller only modifies the attributes set ("value" and "scope") to prevent that. Note that with this change the controller no longer removes the "verified" status, but this is not a problem because, as mentioned, "AccountManager::updateUser()" resets them when needed (for example, when the value of the website property changes). This change is a previous step to fix overwritting properties with null values, and it will prevent the controller from making unexpected changes if more attributes are added in the future. Signed-off-by: Daniel Calviño Sánchez --- .../lib/Controller/UsersController.php | 20 ++++++++++++------- .../tests/Controller/UsersControllerTest.php | 6 ------ 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/apps/settings/lib/Controller/UsersController.php b/apps/settings/lib/Controller/UsersController.php index 98baed01dd..f7e5c35cb1 100644 --- a/apps/settings/lib/Controller/UsersController.php +++ b/apps/settings/lib/Controller/UsersController.php @@ -375,18 +375,24 @@ class UsersController extends Controller { } $user = $this->userSession->getUser(); $data = $this->accountManager->getUser($user); - $data[IAccountManager::PROPERTY_AVATAR] = ['scope' => $avatarScope]; + $data[IAccountManager::PROPERTY_AVATAR]['scope'] = $avatarScope; if ($this->config->getSystemValue('allow_user_to_change_display_name', true) !== false) { - $data[IAccountManager::PROPERTY_DISPLAYNAME] = ['value' => $displayname, 'scope' => $displaynameScope]; - $data[IAccountManager::PROPERTY_EMAIL] = ['value' => $email, 'scope' => $emailScope]; + $data[IAccountManager::PROPERTY_DISPLAYNAME]['value'] = $displayname; + $data[IAccountManager::PROPERTY_DISPLAYNAME]['scope'] = $displaynameScope; + $data[IAccountManager::PROPERTY_EMAIL]['value'] = $email; + $data[IAccountManager::PROPERTY_EMAIL]['scope'] = $emailScope; } if ($this->appManager->isEnabledForUser('federatedfilesharing')) { $shareProvider = \OC::$server->query(FederatedShareProvider::class); if ($shareProvider->isLookupServerUploadEnabled()) { - $data[IAccountManager::PROPERTY_WEBSITE] = ['value' => $website, 'scope' => $websiteScope]; - $data[IAccountManager::PROPERTY_ADDRESS] = ['value' => $address, 'scope' => $addressScope]; - $data[IAccountManager::PROPERTY_PHONE] = ['value' => $phone, 'scope' => $phoneScope]; - $data[IAccountManager::PROPERTY_TWITTER] = ['value' => $twitter, 'scope' => $twitterScope]; + $data[IAccountManager::PROPERTY_WEBSITE]['value'] = $website; + $data[IAccountManager::PROPERTY_WEBSITE]['scope'] = $websiteScope; + $data[IAccountManager::PROPERTY_ADDRESS]['value'] = $address; + $data[IAccountManager::PROPERTY_ADDRESS]['scope'] = $addressScope; + $data[IAccountManager::PROPERTY_PHONE]['value'] = $phone; + $data[IAccountManager::PROPERTY_PHONE]['scope'] = $phoneScope; + $data[IAccountManager::PROPERTY_TWITTER]['value'] = $twitter; + $data[IAccountManager::PROPERTY_TWITTER]['scope'] = $twitterScope; } } try { diff --git a/apps/settings/tests/Controller/UsersControllerTest.php b/apps/settings/tests/Controller/UsersControllerTest.php index 342452d1c0..78ddcde9e3 100644 --- a/apps/settings/tests/Controller/UsersControllerTest.php +++ b/apps/settings/tests/Controller/UsersControllerTest.php @@ -318,16 +318,12 @@ class UsersControllerTest extends \Test\TestCase { $expectedProperties[IAccountManager::PROPERTY_AVATAR]['scope'] = $avatarScope; $expectedProperties[IAccountManager::PROPERTY_PHONE]['value'] = $phone; $expectedProperties[IAccountManager::PROPERTY_PHONE]['scope'] = $phoneScope; - unset($expectedProperties[IAccountManager::PROPERTY_PHONE]['verified']); $expectedProperties[IAccountManager::PROPERTY_WEBSITE]['value'] = $website; $expectedProperties[IAccountManager::PROPERTY_WEBSITE]['scope'] = $websiteScope; - unset($expectedProperties[IAccountManager::PROPERTY_WEBSITE]['verified']); $expectedProperties[IAccountManager::PROPERTY_ADDRESS]['value'] = $address; $expectedProperties[IAccountManager::PROPERTY_ADDRESS]['scope'] = $addressScope; - unset($expectedProperties[IAccountManager::PROPERTY_ADDRESS]['verified']); $expectedProperties[IAccountManager::PROPERTY_TWITTER]['value'] = $twitter; $expectedProperties[IAccountManager::PROPERTY_TWITTER]['scope'] = $twitterScope; - unset($expectedProperties[IAccountManager::PROPERTY_TWITTER]['verified']); $this->mailer->expects($this->once())->method('validateMailAddress') ->willReturn(true); @@ -391,10 +387,8 @@ class UsersControllerTest extends \Test\TestCase { $expectedProperties[IAccountManager::PROPERTY_AVATAR]['scope'] = $avatarScope; $expectedProperties[IAccountManager::PROPERTY_DISPLAYNAME]['value'] = $displayName; $expectedProperties[IAccountManager::PROPERTY_DISPLAYNAME]['scope'] = $displayNameScope; - unset($expectedProperties[IAccountManager::PROPERTY_DISPLAYNAME]['verified']); $expectedProperties[IAccountManager::PROPERTY_EMAIL]['value'] = $email; $expectedProperties[IAccountManager::PROPERTY_EMAIL]['scope'] = $emailScope; - unset($expectedProperties[IAccountManager::PROPERTY_EMAIL]['verified']); $this->mailer->expects($this->once())->method('validateMailAddress') ->willReturn(true); From 0ddd89273663ff2449793f6091d4d0dfbdf9a082 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Sun, 31 Jan 2021 14:08:24 +0100 Subject: [PATCH 06/11] Fix TypeError when "email" is not given in the controller request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Calviño Sánchez --- apps/settings/lib/Controller/UsersController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/settings/lib/Controller/UsersController.php b/apps/settings/lib/Controller/UsersController.php index f7e5c35cb1..4dfc6e0d71 100644 --- a/apps/settings/lib/Controller/UsersController.php +++ b/apps/settings/lib/Controller/UsersController.php @@ -361,7 +361,7 @@ class UsersController extends Controller { $twitter, $twitterScope ) { - $email = strtolower($email); + $email = !is_null($email) ? strtolower($email) : $email; if (!empty($email) && !$this->mailer->validateMailAddress($email)) { return new DataResponse( [ From 77aba3df3c2693fc8c1f656096327fde365bd99f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Sun, 31 Jan 2021 22:53:48 +0100 Subject: [PATCH 07/11] Fix deleting properties of user settings when not given explicitly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The controller can receive an optional subset of the properties of the user settings; values not given are set to "null" by default. However, those null values overwrote the previously existing values, so in practice any value not given was deleted from the user settings. Now only non null values overwrite the previous values. Signed-off-by: Daniel Calviño Sánchez --- .../lib/Controller/UsersController.php | 52 ++++++-- .../tests/Controller/UsersControllerTest.php | 124 ++++++++++++++++++ 2 files changed, 163 insertions(+), 13 deletions(-) diff --git a/apps/settings/lib/Controller/UsersController.php b/apps/settings/lib/Controller/UsersController.php index 4dfc6e0d71..353db298c8 100644 --- a/apps/settings/lib/Controller/UsersController.php +++ b/apps/settings/lib/Controller/UsersController.php @@ -375,24 +375,50 @@ class UsersController extends Controller { } $user = $this->userSession->getUser(); $data = $this->accountManager->getUser($user); - $data[IAccountManager::PROPERTY_AVATAR]['scope'] = $avatarScope; + if (!is_null($avatarScope)) { + $data[IAccountManager::PROPERTY_AVATAR]['scope'] = $avatarScope; + } if ($this->config->getSystemValue('allow_user_to_change_display_name', true) !== false) { - $data[IAccountManager::PROPERTY_DISPLAYNAME]['value'] = $displayname; - $data[IAccountManager::PROPERTY_DISPLAYNAME]['scope'] = $displaynameScope; - $data[IAccountManager::PROPERTY_EMAIL]['value'] = $email; - $data[IAccountManager::PROPERTY_EMAIL]['scope'] = $emailScope; + if (!is_null($displayname)) { + $data[IAccountManager::PROPERTY_DISPLAYNAME]['value'] = $displayname; + } + if (!is_null($displaynameScope)) { + $data[IAccountManager::PROPERTY_DISPLAYNAME]['scope'] = $displaynameScope; + } + if (!is_null($email)) { + $data[IAccountManager::PROPERTY_EMAIL]['value'] = $email; + } + if (!is_null($emailScope)) { + $data[IAccountManager::PROPERTY_EMAIL]['scope'] = $emailScope; + } } if ($this->appManager->isEnabledForUser('federatedfilesharing')) { $shareProvider = \OC::$server->query(FederatedShareProvider::class); if ($shareProvider->isLookupServerUploadEnabled()) { - $data[IAccountManager::PROPERTY_WEBSITE]['value'] = $website; - $data[IAccountManager::PROPERTY_WEBSITE]['scope'] = $websiteScope; - $data[IAccountManager::PROPERTY_ADDRESS]['value'] = $address; - $data[IAccountManager::PROPERTY_ADDRESS]['scope'] = $addressScope; - $data[IAccountManager::PROPERTY_PHONE]['value'] = $phone; - $data[IAccountManager::PROPERTY_PHONE]['scope'] = $phoneScope; - $data[IAccountManager::PROPERTY_TWITTER]['value'] = $twitter; - $data[IAccountManager::PROPERTY_TWITTER]['scope'] = $twitterScope; + if (!is_null($website)) { + $data[IAccountManager::PROPERTY_WEBSITE]['value'] = $website; + } + if (!is_null($websiteScope)) { + $data[IAccountManager::PROPERTY_WEBSITE]['scope'] = $websiteScope; + } + if (!is_null($address)) { + $data[IAccountManager::PROPERTY_ADDRESS]['value'] = $address; + } + if (!is_null($addressScope)) { + $data[IAccountManager::PROPERTY_ADDRESS]['scope'] = $addressScope; + } + if (!is_null($phone)) { + $data[IAccountManager::PROPERTY_PHONE]['value'] = $phone; + } + if (!is_null($phoneScope)) { + $data[IAccountManager::PROPERTY_PHONE]['scope'] = $phoneScope; + } + if (!is_null($twitter)) { + $data[IAccountManager::PROPERTY_TWITTER]['value'] = $twitter; + } + if (!is_null($twitterScope)) { + $data[IAccountManager::PROPERTY_TWITTER]['scope'] = $twitterScope; + } } } try { diff --git a/apps/settings/tests/Controller/UsersControllerTest.php b/apps/settings/tests/Controller/UsersControllerTest.php index 78ddcde9e3..a43ff375fe 100644 --- a/apps/settings/tests/Controller/UsersControllerTest.php +++ b/apps/settings/tests/Controller/UsersControllerTest.php @@ -415,6 +415,130 @@ class UsersControllerTest extends \Test\TestCase { ); } + /** + * @dataProvider dataTestSetUserSettingsSubset + * + * @param string $property + * @param string $propertyValue + */ + public function testSetUserSettingsSubset($property, $propertyValue) { + $controller = $this->getController(false, ['saveUserSettings']); + $user = $this->createMock(IUser::class); + + $this->userSession->method('getUser')->willReturn($user); + + $defaultProperties = $this->getDefaultAccountManagerUserData(); + + $this->accountManager->expects($this->once()) + ->method('getUser') + ->with($user) + ->willReturn($defaultProperties); + + $this->appManager->expects($this->once()) + ->method('isEnabledForUser') + ->with('federatedfilesharing') + ->willReturn(true); + + $avatarScope = ($property === 'avatarScope') ? $propertyValue : null; + $displayName = ($property === 'displayName') ? $propertyValue : null; + $displayNameScope = ($property === 'displayNameScope') ? $propertyValue : null; + $phone = ($property === 'phone') ? $propertyValue : null; + $phoneScope = ($property === 'phoneScope') ? $propertyValue : null; + $email = ($property === 'email') ? $propertyValue : null; + $emailScope = ($property === 'emailScope') ? $propertyValue : null; + $website = ($property === 'website') ? $propertyValue : null; + $websiteScope = ($property === 'websiteScope') ? $propertyValue : null; + $address = ($property === 'address') ? $propertyValue : null; + $addressScope = ($property === 'addressScope') ? $propertyValue : null; + $twitter = ($property === 'twitter') ? $propertyValue : null; + $twitterScope = ($property === 'twitterScope') ? $propertyValue : null; + + $expectedProperties = $defaultProperties; + if ($property === 'avatarScope') { + $expectedProperties[IAccountManager::PROPERTY_AVATAR]['scope'] = $propertyValue; + } + if ($property === 'displayName') { + $expectedProperties[IAccountManager::PROPERTY_DISPLAYNAME]['value'] = $propertyValue; + } + if ($property === 'displayNameScope') { + $expectedProperties[IAccountManager::PROPERTY_DISPLAYNAME]['scope'] = $propertyValue; + } + if ($property === 'phone') { + $expectedProperties[IAccountManager::PROPERTY_PHONE]['value'] = $propertyValue; + } + if ($property === 'phoneScope') { + $expectedProperties[IAccountManager::PROPERTY_PHONE]['scope'] = $propertyValue; + } + if ($property === 'email') { + $expectedProperties[IAccountManager::PROPERTY_EMAIL]['value'] = $propertyValue; + } + if ($property === 'emailScope') { + $expectedProperties[IAccountManager::PROPERTY_EMAIL]['scope'] = $propertyValue; + } + if ($property === 'website') { + $expectedProperties[IAccountManager::PROPERTY_WEBSITE]['value'] = $propertyValue; + } + if ($property === 'websiteScope') { + $expectedProperties[IAccountManager::PROPERTY_WEBSITE]['scope'] = $propertyValue; + } + if ($property === 'address') { + $expectedProperties[IAccountManager::PROPERTY_ADDRESS]['value'] = $propertyValue; + } + if ($property === 'addressScope') { + $expectedProperties[IAccountManager::PROPERTY_ADDRESS]['scope'] = $propertyValue; + } + if ($property === 'twitter') { + $expectedProperties[IAccountManager::PROPERTY_TWITTER]['value'] = $propertyValue; + } + if ($property === 'twitterScope') { + $expectedProperties[IAccountManager::PROPERTY_TWITTER]['scope'] = $propertyValue; + } + + if (!empty($email)) { + $this->mailer->expects($this->once())->method('validateMailAddress') + ->willReturn(true); + } + + $controller->expects($this->once()) + ->method('saveUserSettings') + ->with($user, $expectedProperties) + ->willReturnArgument(1); + + $result = $controller->setUserSettings( + $avatarScope, + $displayName, + $displayNameScope, + $phone, + $phoneScope, + $email, + $emailScope, + $website, + $websiteScope, + $address, + $addressScope, + $twitter, + $twitterScope + ); + } + + public function dataTestSetUserSettingsSubset() { + return [ + ['avatarScope', IAccountManager::VISIBILITY_PUBLIC], + ['displayName', 'Display name'], + ['displayNameScope', IAccountManager::VISIBILITY_PUBLIC], + ['phone', '47658468'], + ['phoneScope', IAccountManager::VISIBILITY_PUBLIC], + ['email', 'john@example.com'], + ['emailScope', IAccountManager::VISIBILITY_PUBLIC], + ['website', 'nextcloud.com'], + ['websiteScope', IAccountManager::VISIBILITY_PUBLIC], + ['address', 'street and city'], + ['addressScope', IAccountManager::VISIBILITY_PUBLIC], + ['twitter', '@nextclouders'], + ['twitterScope', IAccountManager::VISIBILITY_PUBLIC], + ]; + } + /** * @dataProvider dataTestSaveUserSettings * From 67b49edac47eaabd81d672246879ae7c2fcdcb39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Thu, 3 Dec 2020 13:11:19 +0100 Subject: [PATCH 08/11] Make possible to set body in requesttoken requests in integration tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "sendingAToWithRequesttoken" needs to be used to test some non OCS endpoints which require the request token to be sent in the request. Now it is possible to specify the body (or, rather, additional contents beside the cookies and the request token) for those requests, as it will be needed for example to update the user profile. Signed-off-by: Daniel Calviño Sánchez --- .../features/bootstrap/BasicStructure.php | 24 +++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/build/integration/features/bootstrap/BasicStructure.php b/build/integration/features/bootstrap/BasicStructure.php index eed0f173ce..b92b85cf4d 100644 --- a/build/integration/features/bootstrap/BasicStructure.php +++ b/build/integration/features/bootstrap/BasicStructure.php @@ -307,21 +307,31 @@ trait BasicStructure { * @When Sending a :method to :url with requesttoken * @param string $method * @param string $url + * @param TableNode|array|null $body */ - public function sendingAToWithRequesttoken($method, $url) { + public function sendingAToWithRequesttoken($method, $url, $body = null) { $baseUrl = substr($this->baseUrl, 0, -5); + $options = [ + 'cookies' => $this->cookieJar, + 'headers' => [ + 'requesttoken' => $this->requestToken + ], + ]; + + if ($body instanceof TableNode) { + $fd = $body->getRowsHash(); + $options['form_params'] = $fd; + } elseif ($body) { + $options = array_merge($options, $body); + } + $client = new Client(); try { $this->response = $client->request( $method, $baseUrl . $url, - [ - 'cookies' => $this->cookieJar, - 'headers' => [ - 'requesttoken' => $this->requestToken - ] - ] + $options ); } catch (ClientException $e) { $this->response = $e->getResponse(); From 4e05a486a30d03fd6d44b0911f1c526d53438452 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Sun, 31 Jan 2021 23:10:55 +0100 Subject: [PATCH 09/11] Add integration tests for searching users in contacts menu MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Calviño Sánchez --- .drone.yml | 25 +++ .../features/bootstrap/ContactsMenu.php | 69 +++++++ .../features/bootstrap/FeatureContext.php | 1 + .../features/contacts-menu.feature | 188 ++++++++++++++++++ 4 files changed, 283 insertions(+) create mode 100644 build/integration/features/bootstrap/ContactsMenu.php create mode 100644 build/integration/features/contacts-menu.feature diff --git a/.drone.yml b/.drone.yml index feebf14f29..ee6c514469 100644 --- a/.drone.yml +++ b/.drone.yml @@ -1155,6 +1155,31 @@ trigger: - pull_request - push +--- +kind: pipeline +name: integration-contacts-menu + +steps: +- name: submodules + image: docker:git + commands: + - git submodule update --init +- name: integration-contacts-menu + image: nextcloudci/integration-php7.3:integration-php7.3-2 + commands: + - bash tests/drone-run-integration-tests.sh || exit 0 + - ./occ maintenance:install --admin-pass=admin --data-dir=/dev/shm/nc_int + - cd build/integration + - ./run.sh features/contacts-menu.feature + +trigger: + branch: + - master + - stable* + event: + - pull_request + - push + --- kind: pipeline name: integration-favorites diff --git a/build/integration/features/bootstrap/ContactsMenu.php b/build/integration/features/bootstrap/ContactsMenu.php new file mode 100644 index 0000000000..30e1dad625 --- /dev/null +++ b/build/integration/features/bootstrap/ContactsMenu.php @@ -0,0 +1,69 @@ + + * + * @author Daniel Calviño Sánchez + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +use PHPUnit\Framework\Assert; + +trait ContactsMenu { + + // BasicStructure trait is expected to be used in the class that uses this + // trait. + + /** + * @When /^searching for contacts matching with "([^"]*)"$/ + * + * @param string $filter + */ + public function searchingForContactsMatchingWith(string $filter) { + $url = '/index.php/contactsmenu/contacts'; + + $parameters[] = 'filter=' . $filter; + + $url .= '?' . implode('&', $parameters); + + $this->sendingAToWithRequesttoken('POST', $url); + } + + /** + * @Then /^the list of searched contacts has "(\d+)" contacts$/ + */ + public function theListOfSearchedContactsHasContacts(int $count) { + $this->theHTTPStatusCodeShouldBe(200); + + $searchedContacts = json_decode($this->response->getBody(), $asAssociativeArray = true)['contacts']; + + Assert::assertEquals($count, count($searchedContacts)); + } + + /** + * @Then /^searched contact "(\d+)" is named "([^"]*)"$/ + * + * @param int $index + * @param string $expectedName + */ + public function searchedContactXIsNamed(int $index, string $expectedName) { + $searchedContacts = json_decode($this->response->getBody(), $asAssociativeArray = true)['contacts']; + $searchedContact = $searchedContacts[$index]; + + Assert::assertEquals($expectedName, $searchedContact['fullName']); + } +} diff --git a/build/integration/features/bootstrap/FeatureContext.php b/build/integration/features/bootstrap/FeatureContext.php index 66e2282608..18e5eb3363 100644 --- a/build/integration/features/bootstrap/FeatureContext.php +++ b/build/integration/features/bootstrap/FeatureContext.php @@ -33,6 +33,7 @@ require __DIR__ . '/../../vendor/autoload.php'; * Features context. */ class FeatureContext implements Context, SnippetAcceptingContext { + use ContactsMenu; use Search; use WebDav; } diff --git a/build/integration/features/contacts-menu.feature b/build/integration/features/contacts-menu.feature new file mode 100644 index 0000000000..845d4d3592 --- /dev/null +++ b/build/integration/features/contacts-menu.feature @@ -0,0 +1,188 @@ +Feature: contacts-menu + + Scenario: users can be searched by display name + Given user "user0" exists + And user "user1" exists + And As an "admin" + And sending "PUT" to "/cloud/users/user1" with + | key | displayname | + | value | Test name | + When Logging in using web as "user0" + And searching for contacts matching with "test" + Then the list of searched contacts has "1" contacts + And searched contact "0" is named "Test name" + + Scenario: users can be searched by email + Given user "user0" exists + And user "user1" exists + And As an "admin" + And sending "PUT" to "/cloud/users/user1" with + | key | email | + | value | test@example.com | + When Logging in using web as "user0" + And searching for contacts matching with "test" + Then the list of searched contacts has "1" contacts + And searched contact "0" is named "user1" + + Scenario: users can not be searched by id + Given user "user0" exists + And user "user1" exists + And As an "admin" + And sending "PUT" to "/cloud/users/user1" with + | key | displayname | + | value | Test name | + When Logging in using web as "user0" + And searching for contacts matching with "user" + Then the list of searched contacts has "0" contacts + + Scenario: search several users + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And user "user3" exists + And user "user4" exists + And user "user5" exists + And As an "admin" + And sending "PUT" to "/cloud/users/user1" with + | key | displayname | + | value | Test name | + And sending "PUT" to "/cloud/users/user2" with + | key | email | + | value | test@example.com | + And sending "PUT" to "/cloud/users/user3" with + | key | displayname | + | value | Unmatched name | + And sending "PUT" to "/cloud/users/user4" with + | key | email | + | value | unmatched@example.com | + And sending "PUT" to "/cloud/users/user5" with + | key | displayname | + | value | Another test name | + And sending "PUT" to "/cloud/users/user5" with + | key | email | + | value | another_test@example.com | + When Logging in using web as "user0" + And searching for contacts matching with "test" + Then the list of searched contacts has "3" contacts + # Results are sorted alphabetically + And searched contact "0" is named "Another test name" + And searched contact "1" is named "Test name" + And searched contact "2" is named "user2" + + + + Scenario: users can not be found by display name if visibility is private + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And Logging in using web as "user1" + And Sending a "PUT" to "/settings/users/user1/settings" with requesttoken + | displayname | Test name | + | displaynameScope | private | + And Logging in using web as "user2" + And Sending a "PUT" to "/settings/users/user2/settings" with requesttoken + | displayname | Another test name | + | displaynameScope | contacts | + When Logging in using web as "user0" + And searching for contacts matching with "test" + Then the list of searched contacts has "1" contacts + And searched contact "0" is named "Another test name" + + Scenario: users can not be found by email if visibility is private + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And Logging in using web as "user1" + And Sending a "PUT" to "/settings/users/user1/settings" with requesttoken + | email | test@example.com | + | emailScope | private | + And Logging in using web as "user2" + And Sending a "PUT" to "/settings/users/user2/settings" with requesttoken + | email | another_test@example.com | + | emailScope | contacts | + When Logging in using web as "user0" + And searching for contacts matching with "test" + Then the list of searched contacts has "1" contacts + And searched contact "0" is named "user2" + + Scenario: users can be found by other properties if the visibility of one is private + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And Logging in using web as "user1" + And Sending a "PUT" to "/settings/users/user1/settings" with requesttoken + | displayname | Test name | + | displaynameScope | contacts | + | email | test@example.com | + | emailScope | private | + And Logging in using web as "user2" + And Sending a "PUT" to "/settings/users/user2/settings" with requesttoken + | displayname | Another test name | + | displaynameScope | private | + | email | another_test@example.com | + | emailScope | contacts | + When Logging in using web as "user0" + And searching for contacts matching with "test" + Then the list of searched contacts has "2" contacts + And searched contact "0" is named "" + And searched contact "1" is named "Test name" + + + + Scenario: users can be searched by display name if visibility is increased again + Given user "user0" exists + And user "user1" exists + And Logging in using web as "user1" + And Sending a "PUT" to "/settings/users/user1/settings" with requesttoken + | displayname | Test name | + | displaynameScope | private | + And Sending a "PUT" to "/settings/users/user1/settings" with requesttoken + | displaynameScope | contacts | + When Logging in using web as "user0" + And searching for contacts matching with "test" + Then the list of searched contacts has "1" contacts + And searched contact "0" is named "Test name" + + Scenario: users can be searched by email if visibility is increased again + Given user "user0" exists + And user "user1" exists + And Logging in using web as "user1" + And Sending a "PUT" to "/settings/users/user1/settings" with requesttoken + | email | test@example.com | + | emailScope | private | + And Sending a "PUT" to "/settings/users/user1/settings" with requesttoken + | emailScope | contacts | + When Logging in using web as "user0" + And searching for contacts matching with "test" + Then the list of searched contacts has "1" contacts + And searched contact "0" is named "user1" + + + + Scenario: users can not be searched by display name if visibility is private even if updated with provisioning + Given user "user0" exists + And user "user1" exists + And Logging in using web as "user1" + And Sending a "PUT" to "/settings/users/user1/settings" with requesttoken + | displaynameScope | private | + And As an "admin" + And sending "PUT" to "/cloud/users/user1" with + | key | displayname | + | value | Test name | + When Logging in using web as "user0" + And searching for contacts matching with "test" + Then the list of searched contacts has "0" contacts + + Scenario: users can not be searched by email if visibility is private even if updated with provisioning + Given user "user0" exists + And user "user1" exists + And Logging in using web as "user1" + And Sending a "PUT" to "/settings/users/user1/settings" with requesttoken + | emailScope | private | + And As an "admin" + And sending "PUT" to "/cloud/users/user1" with + | key | email | + | value | test@example.com | + When Logging in using web as "user0" + And searching for contacts matching with "test" + Then the list of searched contacts has "0" contacts From e929d16bbb1b5c8b4d8d45a811e993477d2eb06a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Mon, 1 Feb 2021 03:34:36 +0100 Subject: [PATCH 10/11] Fix settings scope menu hidden when lookup server upload is disabled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When upload to the lookup server is disabled the scope menu was hidden in the personal information settings. However, even if the lookup server upload is disabled the personal information is still accesible from the local server as well as trusted servers. Users should be able to still set if their information is available to other users or if it is private, so now the scope menu is always show (although the "Public" option is hidden if the lookup server upload is disabled). If the user set the information as public before the upload to the lookup server was disabled the menu will also show the "Public" option as active, although disabled. Setting the visibility to any of the other options will hide the "Public" option from the menu (until the lookup server upload is enabled again). Signed-off-by: Daniel Calviño Sánchez --- apps/settings/css/settings.scss | 10 +++ apps/settings/js/federationscopemenu.js | 11 +++ apps/settings/js/templates.js | 72 ++++++++++++++----- .../templates/federationscopemenu.handlebars | 10 +++ .../settings/personal/personal.info.php | 21 ++---- 5 files changed, 92 insertions(+), 32 deletions(-) diff --git a/apps/settings/css/settings.scss b/apps/settings/css/settings.scss index cc798868c5..4915dd7da7 100644 --- a/apps/settings/css/settings.scss +++ b/apps/settings/css/settings.scss @@ -403,6 +403,16 @@ select { font-weight: bold; } } + + &.disabled { + opacity: .5; + + cursor: default; + + span { + cursor: default; + } + } } } } diff --git a/apps/settings/js/federationscopemenu.js b/apps/settings/js/federationscopemenu.js index 170aec15a8..297cd006e5 100644 --- a/apps/settings/js/federationscopemenu.js +++ b/apps/settings/js/federationscopemenu.js @@ -117,6 +117,17 @@ break; } + var lookupServerUploadEnabled = $('#lookupServerUploadEnabled').val(); + if (!lookupServerUploadEnabled && !this._scopes[2].active) { + this._scopes[2].hidden = true + } else if (!lookupServerUploadEnabled && this._scopes[2].active) { + this._scopes[2].hidden = false + this._scopes[2].disabled = true + } else { + this._scopes[2].hidden = false + this._scopes[2].disabled = false + } + this.render(); this.$el.removeClass('hidden'); diff --git a/apps/settings/js/templates.js b/apps/settings/js/templates.js index 25c2e79004..2cd8179f3e 100644 --- a/apps/settings/js/templates.js +++ b/apps/settings/js/templates.js @@ -1,6 +1,15 @@ (function() { var template = Handlebars.template, templates = OC.Settings.Templates = OC.Settings.Templates || {}; templates['federationscopemenu'] = template({"1":function(container,depth0,helpers,partials,data) { + var stack1, lookupProperty = container.lookupProperty || function(parent, propertyName) { + if (Object.prototype.hasOwnProperty.call(parent, propertyName)) { + return parent[propertyName]; + } + return undefined + }; + + return ((stack1 = lookupProperty(helpers,"unless").call(depth0 != null ? depth0 : (container.nullContext || {}),(depth0 != null ? lookupProperty(depth0,"hidden") : depth0),{"name":"unless","hash":{},"fn":container.program(2, data, 0),"inverse":container.noop,"data":data,"loc":{"start":{"line":3,"column":2},"end":{"line":25,"column":13}}})) != null ? stack1 : ""); +},"2":function(container,depth0,helpers,partials,data) { var stack1, helper, alias1=depth0 != null ? depth0 : (container.nullContext || {}), alias2=container.hooks.helperMissing, alias3="function", alias4=container.escapeExpression, lookupProperty = container.lookupProperty || function(parent, propertyName) { if (Object.prototype.hasOwnProperty.call(parent, propertyName)) { return parent[propertyName]; @@ -8,22 +17,49 @@ templates['federationscopemenu'] = template({"1":function(container,depth0,helpe return undefined }; - return "
  • \n \n" - + ((stack1 = lookupProperty(helpers,"if").call(alias1,(depth0 != null ? lookupProperty(depth0,"iconClass") : depth0),{"name":"if","hash":{},"fn":container.program(4, data, 0),"inverse":container.program(6, data, 0),"data":data,"loc":{"start":{"line":5,"column":4},"end":{"line":9,"column":11}}})) != null ? stack1 : "") + return "
  • \n" + + ((stack1 = lookupProperty(helpers,"if").call(alias1,(depth0 != null ? lookupProperty(depth0,"disabled") : depth0),{"name":"if","hash":{},"fn":container.program(3, data, 0),"inverse":container.program(6, data, 0),"data":data,"loc":{"start":{"line":5,"column":3},"end":{"line":9,"column":10}}})) != null ? stack1 : "") + + ((stack1 = lookupProperty(helpers,"if").call(alias1,(depth0 != null ? lookupProperty(depth0,"iconClass") : depth0),{"name":"if","hash":{},"fn":container.program(8, data, 0),"inverse":container.program(10, data, 0),"data":data,"loc":{"start":{"line":10,"column":4},"end":{"line":14,"column":11}}})) != null ? stack1 : "") + "

    \n " - + alias4(((helper = (helper = lookupProperty(helpers,"displayName") || (depth0 != null ? lookupProperty(depth0,"displayName") : depth0)) != null ? helper : alias2),(typeof helper === alias3 ? helper.call(alias1,{"name":"displayName","hash":{},"data":data,"loc":{"start":{"line":11,"column":35},"end":{"line":11,"column":50}}}) : helper))) + + alias4(((helper = (helper = lookupProperty(helpers,"displayName") || (depth0 != null ? lookupProperty(depth0,"displayName") : depth0)) != null ? helper : alias2),(typeof helper === alias3 ? helper.call(alias1,{"name":"displayName","hash":{},"data":data,"loc":{"start":{"line":16,"column":35},"end":{"line":16,"column":50}}}) : helper))) + "
    \n " - + alias4(((helper = (helper = lookupProperty(helpers,"tooltip") || (depth0 != null ? lookupProperty(depth0,"tooltip") : depth0)) != null ? helper : alias2),(typeof helper === alias3 ? helper.call(alias1,{"name":"tooltip","hash":{},"data":data,"loc":{"start":{"line":12,"column":40},"end":{"line":12,"column":51}}}) : helper))) - + "\n

    \n     \n
    • \n"; -},"2":function(container,depth0,helpers,partials,data) { - return "active"; + + alias4(((helper = (helper = lookupProperty(helpers,"tooltip") || (depth0 != null ? lookupProperty(depth0,"tooltip") : depth0)) != null ? helper : alias2),(typeof helper === alias3 ? helper.call(alias1,{"name":"tooltip","hash":{},"data":data,"loc":{"start":{"line":17,"column":40},"end":{"line":17,"column":51}}}) : helper))) + + "\n

    \n" + + ((stack1 = lookupProperty(helpers,"if").call(alias1,(depth0 != null ? lookupProperty(depth0,"disabled") : depth0),{"name":"if","hash":{},"fn":container.program(12, data, 0),"inverse":container.program(14, data, 0),"data":data,"loc":{"start":{"line":19,"column":3},"end":{"line":23,"column":10}}})) != null ? stack1 : "") + + " \n"; +},"3":function(container,depth0,helpers,partials,data) { + var stack1, helper, alias1=depth0 != null ? depth0 : (container.nullContext || {}), alias2=container.hooks.helperMissing, alias3="function", alias4=container.escapeExpression, lookupProperty = container.lookupProperty || function(parent, propertyName) { + if (Object.prototype.hasOwnProperty.call(parent, propertyName)) { + return parent[propertyName]; + } + return undefined + }; + + return "
    \n"; },"4":function(container,depth0,helpers,partials,data) { + return "active"; +},"6":function(container,depth0,helpers,partials,data) { + var stack1, helper, alias1=depth0 != null ? depth0 : (container.nullContext || {}), alias2=container.hooks.helperMissing, alias3="function", alias4=container.escapeExpression, lookupProperty = container.lookupProperty || function(parent, propertyName) { + if (Object.prototype.hasOwnProperty.call(parent, propertyName)) { + return parent[propertyName]; + } + return undefined + }; + + return " \n"; +},"8":function(container,depth0,helpers,partials,data) { var helper, lookupProperty = container.lookupProperty || function(parent, propertyName) { if (Object.prototype.hasOwnProperty.call(parent, propertyName)) { return parent[propertyName]; @@ -32,10 +68,14 @@ templates['federationscopemenu'] = template({"1":function(container,depth0,helpe }; return " \n"; -},"6":function(container,depth0,helpers,partials,data) { +},"10":function(container,depth0,helpers,partials,data) { return " \n"; +},"12":function(container,depth0,helpers,partials,data) { + return "
    \n"; +},"14":function(container,depth0,helpers,partials,data) { + return " \n"; },"compiler":[8,">= 4.3.0"],"main":function(container,depth0,helpers,partials,data) { var stack1, lookupProperty = container.lookupProperty || function(parent, propertyName) { if (Object.prototype.hasOwnProperty.call(parent, propertyName)) { @@ -45,7 +85,7 @@ templates['federationscopemenu'] = template({"1":function(container,depth0,helpe }; return "
      \n" - + ((stack1 = lookupProperty(helpers,"each").call(depth0 != null ? depth0 : (container.nullContext || {}),(depth0 != null ? lookupProperty(depth0,"items") : depth0),{"name":"each","hash":{},"fn":container.program(1, data, 0),"inverse":container.noop,"data":data,"loc":{"start":{"line":2,"column":1},"end":{"line":16,"column":10}}})) != null ? stack1 : "") + + ((stack1 = lookupProperty(helpers,"each").call(depth0 != null ? depth0 : (container.nullContext || {}),(depth0 != null ? lookupProperty(depth0,"items") : depth0),{"name":"each","hash":{},"fn":container.program(1, data, 0),"inverse":container.noop,"data":data,"loc":{"start":{"line":2,"column":1},"end":{"line":26,"column":10}}})) != null ? stack1 : "") + "
    \n"; },"useData":true}); })(); \ No newline at end of file diff --git a/apps/settings/js/templates/federationscopemenu.handlebars b/apps/settings/js/templates/federationscopemenu.handlebars index 4bd945b094..c9d86c6314 100644 --- a/apps/settings/js/templates/federationscopemenu.handlebars +++ b/apps/settings/js/templates/federationscopemenu.handlebars @@ -1,7 +1,12 @@ diff --git a/apps/settings/templates/settings/personal/personal.info.php b/apps/settings/templates/settings/personal/personal.info.php index 184334b56e..1524403611 100644 --- a/apps/settings/templates/settings/personal/personal.info.php +++ b/apps/settings/templates/settings/personal/personal.info.php @@ -34,6 +34,9 @@ script('settings', [ ]); ?> + + +
    @@ -67,9 +70,7 @@ script('settings', [
    - -
    @@ -123,9 +124,7 @@ script('settings', [ - - - +
    @@ -173,9 +172,7 @@ script('settings', [ t('For password reset and notifications')); ?> - - - +
    @@ -196,9 +193,7 @@ script('settings', [ placeholder="t('Your phone number')); ?>" autocomplete="on" autocapitalize="none" autocorrect="off" /> - - @@ -220,9 +215,7 @@ script('settings', [ value="" autocomplete="on" autocapitalize="none" autocorrect="off" /> - - @@ -275,9 +268,7 @@ script('settings', [ } ?> /> - - @@ -330,9 +321,7 @@ script('settings', [ } ?> /> - - From 546be0ea633af858065624b2077b8d85b91ee245 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Mon, 1 Feb 2021 04:32:28 +0100 Subject: [PATCH 11/11] Handle corrupted scope values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Due to a bug (fixed some commits ago) in the UsersController of the settings app the scope of the properties can be null (for example, if lookup server upload was disabled and the user then changed the display name in the profile information). In that case now the scope menu icon shows an error to inform the user. The scope value will not change when other properties are modified until the user chooses an explicit value from the menu. Note that until a scope is explicitly set the property will behave as if it is private. Signed-off-by: Daniel Calviño Sánchez --- apps/settings/js/federationsettingsview.js | 15 ++++++++++++++- .../templates/settings/personal/personal.info.php | 14 +++++++------- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/apps/settings/js/federationsettingsview.js b/apps/settings/js/federationsettingsview.js index 9cefaf132f..2476c3f942 100644 --- a/apps/settings/js/federationsettingsview.js +++ b/apps/settings/js/federationsettingsview.js @@ -43,7 +43,14 @@ if (!scopeOnly) { self._config.set(field, $('#' + field).val()); } - self._config.set(field + 'Scope', $('#' + field + 'scope').val()); + // A scope could have been stored as null due to a previous bug. + // Null values should be kept as such until the user explicitly + // sets the right value, but they will be returned as empty + // strings in the template (which would overwrite the null value + // if sent). Due to this an extra class is needed to + // differentiate them. + var initialScopeValue = $('#' + field + 'scope').hasClass('corrupted-scope-value') ? undefined : $('#' + field + 'scope').val(); + self._config.set(field + 'Scope', initialScopeValue); // Set inputs whenever model values change if (!scopeOnly) { @@ -219,6 +226,12 @@ $icon.addClass('icon-link'); $icon.removeClass('hidden'); break; + case '': + case null: + case undefined: + $icon.addClass('icon-error'); + $icon.removeClass('hidden'); + break; } } }); diff --git a/apps/settings/templates/settings/personal/personal.info.php b/apps/settings/templates/settings/personal/personal.info.php index 1524403611..ea56b00bbd 100644 --- a/apps/settings/templates/settings/personal/personal.info.php +++ b/apps/settings/templates/settings/personal/personal.info.php @@ -70,7 +70,7 @@ script('settings', [ - + class="corrupted-scope-value">
    @@ -124,7 +124,7 @@ script('settings', [ - + class="corrupted-scope-value">
    @@ -172,7 +172,7 @@ script('settings', [ t('For password reset and notifications')); ?> - + class="corrupted-scope-value">
    @@ -193,7 +193,7 @@ script('settings', [ placeholder="t('Your phone number')); ?>" autocomplete="on" autocapitalize="none" autocorrect="off" /> - + class="corrupted-scope-value"> @@ -215,7 +215,7 @@ script('settings', [ value="" autocomplete="on" autocapitalize="none" autocorrect="off" /> - + class="corrupted-scope-value"> @@ -268,7 +268,7 @@ script('settings', [ } ?> /> - + class="corrupted-scope-value"> @@ -321,7 +321,7 @@ script('settings', [ } ?> /> - + class="corrupted-scope-value">