From 0e26ba4c2adec21de3b5239a646bb4dbde44b2f4 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 2 Jun 2017 10:09:42 +0200 Subject: [PATCH 1/3] Don't allow the user to set fields they can't see Signed-off-by: Joas Schilling --- .../lib/Controller/UsersController.php | 31 ++++++++++++++----- .../tests/Controller/UsersControllerTest.php | 11 +++++-- 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index 6e34fe53eb..132727eecb 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -32,6 +32,7 @@ namespace OCA\Provisioning_API\Controller; use OC\Accounts\AccountManager; use OC\Settings\Mailer\NewUserMailHelper; use OC_Helper; +use OCP\App\IAppManager; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\OCS\OCSException; use OCP\AppFramework\OCS\OCSForbiddenException; @@ -52,6 +53,8 @@ class UsersController extends OCSController { private $userManager; /** @var IConfig */ private $config; + /** @var IAppManager */ + private $appManager; /** @var IGroupManager|\OC\Group\Manager */ // FIXME Requires a method that is not on the interface private $groupManager; /** @var IUserSession */ @@ -70,6 +73,7 @@ class UsersController extends OCSController { * @param IRequest $request * @param IUserManager $userManager * @param IConfig $config + * @param IAppManager $appManager * @param IGroupManager $groupManager * @param IUserSession $userSession * @param AccountManager $accountManager @@ -81,6 +85,7 @@ class UsersController extends OCSController { IRequest $request, IUserManager $userManager, IConfig $config, + IAppManager $appManager, IGroupManager $groupManager, IUserSession $userSession, AccountManager $accountManager, @@ -91,6 +96,7 @@ class UsersController extends OCSController { $this->userManager = $userManager; $this->config = $config; + $this->appManager = $appManager; $this->groupManager = $groupManager; $this->userSession = $userSession; $this->accountManager = $accountManager; @@ -309,14 +315,25 @@ class UsersController extends OCSController { $permittedFields = []; if($targetUser->getUID() === $currentLoggedInUser->getUID()) { // Editing self (display, email) - $permittedFields[] = 'display'; - $permittedFields[] = AccountManager::PROPERTY_DISPLAYNAME; - $permittedFields[] = AccountManager::PROPERTY_EMAIL; + if ($this->config->getSystemValue('allow_user_to_change_display_name', true) !== false) { + $permittedFields[] = 'display'; + $permittedFields[] = AccountManager::PROPERTY_DISPLAYNAME; + $permittedFields[] = AccountManager::PROPERTY_EMAIL; + } + $permittedFields[] = 'password'; - $permittedFields[] = AccountManager::PROPERTY_PHONE; - $permittedFields[] = AccountManager::PROPERTY_ADDRESS; - $permittedFields[] = AccountManager::PROPERTY_WEBSITE; - $permittedFields[] = AccountManager::PROPERTY_TWITTER; + + if ($this->appManager->isEnabledForUser('federatedfilesharing')) { + $federatedFileSharing = new \OCA\FederatedFileSharing\AppInfo\Application(); + $shareProvider = $federatedFileSharing->getFederatedShareProvider(); + if ($shareProvider->isLookupServerUploadEnabled()) { + $permittedFields[] = AccountManager::PROPERTY_PHONE; + $permittedFields[] = AccountManager::PROPERTY_ADDRESS; + $permittedFields[] = AccountManager::PROPERTY_WEBSITE; + $permittedFields[] = AccountManager::PROPERTY_TWITTER; + } + } + // If admin they can edit their own quota if($this->groupManager->isAdmin($currentLoggedInUser->getUID())) { $permittedFields[] = 'quota'; diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php index 57e1d2eac6..61205b4590 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -32,6 +32,7 @@ namespace OCA\Provisioning_API\Tests\Controller; use Exception; use OC\Accounts\AccountManager; use OC\Group\Manager; +use OCP\App\IAppManager; use OCP\Mail\IEMailTemplate; use OC\Settings\Mailer\NewUserMailHelper; use OC\SubAdmin; @@ -58,6 +59,8 @@ class UsersControllerTest extends TestCase { protected $userManager; /** @var IConfig|PHPUnit_Framework_MockObject_MockObject */ protected $config; + /** @var IAppManager|PHPUnit_Framework_MockObject_MockObject */ + protected $appManager; /** @var Manager|PHPUnit_Framework_MockObject_MockObject */ protected $groupManager; /** @var IUserSession|PHPUnit_Framework_MockObject_MockObject */ @@ -66,9 +69,9 @@ class UsersControllerTest extends TestCase { protected $logger; /** @var UsersController|PHPUnit_Framework_MockObject_MockObject */ protected $api; - /** @var AccountManager|PHPUnit_Framework_MockObject_MockObject */ + /** @var AccountManager|PHPUnit_Framework_MockObject_MockObject */ protected $accountManager; - /** @var IRequest|PHPUnit_Framework_MockObject_MockObject */ + /** @var IRequest|PHPUnit_Framework_MockObject_MockObject */ protected $request; /** @var IFactory|PHPUnit_Framework_MockObject_MockObject */ private $l10nFactory; @@ -80,6 +83,7 @@ class UsersControllerTest extends TestCase { $this->userManager = $this->createMock(IUserManager::class); $this->config = $this->createMock(IConfig::class); + $this->appManager = $this->createMock(IAppManager::class); $this->groupManager = $this->createMock(Manager::class); $this->userSession = $this->createMock(IUserSession::class); $this->logger = $this->createMock(ILogger::class); @@ -94,6 +98,7 @@ class UsersControllerTest extends TestCase { $this->request, $this->userManager, $this->config, + $this->appManager, $this->groupManager, $this->userSession, $this->accountManager, @@ -2647,6 +2652,7 @@ class UsersControllerTest extends TestCase { $this->request, $this->userManager, $this->config, + $this->appManager, $this->groupManager, $this->userSession, $this->accountManager, @@ -2707,6 +2713,7 @@ class UsersControllerTest extends TestCase { $this->request, $this->userManager, $this->config, + $this->appManager, $this->groupManager, $this->userSession, $this->accountManager, From 19041ed1a8a7a60570ae726445dd7535a304c322 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 2 Jun 2017 12:31:37 +0200 Subject: [PATCH 2/3] Also cover the settings endpoint Signed-off-by: Joas Schilling --- settings/Controller/UsersController.php | 50 +++++++++++++++---------- 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/settings/Controller/UsersController.php b/settings/Controller/UsersController.php index d4a5de93a3..e217e18946 100644 --- a/settings/Controller/UsersController.php +++ b/settings/Controller/UsersController.php @@ -78,6 +78,8 @@ class UsersController extends Controller { private $isEncryptionAppEnabled; /** @var bool contains the state of the admin recovery setting */ private $isRestoreEnabled = false; + /** @var IAppManager */ + private $appManager; /** @var IAvatarManager */ private $avatarManager; /** @var AccountManager */ @@ -146,6 +148,7 @@ class UsersController extends Controller { $this->l10n = $l10n; $this->log = $log; $this->mailer = $mailer; + $this->appManager = $appManager; $this->avatarManager = $avatarManager; $this->accountManager = $accountManager; $this->secureRandom = $secureRandom; @@ -718,18 +721,27 @@ class UsersController extends Controller { ); } - $data = [ - AccountManager::PROPERTY_AVATAR => ['scope' => $avatarScope], - AccountManager::PROPERTY_DISPLAYNAME => ['value' => $displayname, 'scope' => $displaynameScope], - AccountManager::PROPERTY_EMAIL=> ['value' => $email, 'scope' => $emailScope], - AccountManager::PROPERTY_WEBSITE => ['value' => $website, 'scope' => $websiteScope], - AccountManager::PROPERTY_ADDRESS => ['value' => $address, 'scope' => $addressScope], - AccountManager::PROPERTY_PHONE => ['value' => $phone, 'scope' => $phoneScope], - AccountManager::PROPERTY_TWITTER => ['value' => $twitter, 'scope' => $twitterScope] - ]; - $user = $this->userSession->getUser(); + $data = $this->accountManager->getUser($user); + + $data[AccountManager::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]; + } + + if ($this->appManager->isEnabledForUser('federatedfilesharing')) { + $federatedFileSharing = new \OCA\FederatedFileSharing\AppInfo\Application(); + $shareProvider = $federatedFileSharing->getFederatedShareProvider(); + 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]; + } + } + try { $this->saveUserSettings($user, $data); return new DataResponse( @@ -737,15 +749,15 @@ class UsersController extends Controller { 'status' => 'success', 'data' => [ 'userId' => $user->getUID(), - 'avatarScope' => $avatarScope, - 'displayname' => $displayname, - 'displaynameScope' => $displaynameScope, - 'email' => $email, - 'emailScope' => $emailScope, - 'website' => $website, - 'websiteScope' => $websiteScope, - 'address' => $address, - 'addressScope' => $addressScope, + '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'], 'message' => (string) $this->l10n->t('Settings saved') ] ], From f39fdaf46e6ab9b3840102bb657cce7d0b074e6f Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 6 Jun 2017 11:42:48 +0200 Subject: [PATCH 3/3] adjust the test Signed-off-by: Joas Schilling --- .../Controller/UsersControllerTest.php | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/tests/Settings/Controller/UsersControllerTest.php b/tests/Settings/Controller/UsersControllerTest.php index 5905023e96..7186ce7beb 100644 --- a/tests/Settings/Controller/UsersControllerTest.php +++ b/tests/Settings/Controller/UsersControllerTest.php @@ -2005,6 +2005,52 @@ class UsersControllerTest extends \Test\TestCase { $saveData = (!empty($email) && $validEmail) || empty($email); if ($saveData) { + $this->accountManager->expects($this->once()) + ->method('getUser') + ->with($user) + ->willReturn([ + AccountManager::PROPERTY_DISPLAYNAME => + [ + 'value' => 'Display name', + 'scope' => AccountManager::VISIBILITY_CONTACTS_ONLY, + 'verified' => AccountManager::NOT_VERIFIED, + ], + AccountManager::PROPERTY_ADDRESS => + [ + 'value' => '', + 'scope' => AccountManager::VISIBILITY_PRIVATE, + 'verified' => AccountManager::NOT_VERIFIED, + ], + AccountManager::PROPERTY_WEBSITE => + [ + 'value' => '', + 'scope' => AccountManager::VISIBILITY_PRIVATE, + 'verified' => AccountManager::NOT_VERIFIED, + ], + AccountManager::PROPERTY_EMAIL => + [ + 'value' => '', + 'scope' => AccountManager::VISIBILITY_CONTACTS_ONLY, + 'verified' => AccountManager::NOT_VERIFIED, + ], + AccountManager::PROPERTY_AVATAR => + [ + 'scope' => AccountManager::VISIBILITY_CONTACTS_ONLY + ], + AccountManager::PROPERTY_PHONE => + [ + 'value' => '', + 'scope' => AccountManager::VISIBILITY_PRIVATE, + 'verified' => AccountManager::NOT_VERIFIED, + ], + AccountManager::PROPERTY_TWITTER => + [ + 'value' => '', + 'scope' => AccountManager::VISIBILITY_PRIVATE, + 'verified' => AccountManager::NOT_VERIFIED, + ], + ]); + $controller->expects($this->once())->method('saveUserSettings'); } else { $controller->expects($this->never())->method('saveUserSettings');