Merge pull request #5223 from nextcloud/do-not-allow-to-set-invisible-fields

Don't allow the user to set fields they can't see
9 years ago · 15314b6f5b
4 changed files with 110 additions and 28 deletions
--- 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';
--- 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,
--- 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')
 					]
 				],
--- 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');