Merge pull request #13790 from owncloud/fix-subadmin-group

Fix subadmin listing of group

1 files changed, 43 insertions, 24 deletions

diff --git a/settings/controller/userscontroller.php b/settings/controller/userscontroller.php
index be1b26f86ad..80fb81600df 100644
--- a/settings/controller/userscontroller.php
+++ b/settings/controller/userscontroller.php
@@ -1,7 +1,7 @@
 <?php
 /**
  * @author Lukas Reschke
- * @copyright 2014 Lukas Reschke lukas@owncloud.com
+ * @copyright 2014-2015 Lukas Reschke lukas@owncloud.com
  *
  * This file is licensed under the Affero General Public License version 3 or
  * later.
@@ -11,6 +11,7 @@
 namespace OC\Settings\Controller;
 
 use OC\AppFramework\Http;
+use OC\Settings\Factory\SubAdminFactory;
 use OC\User\User;
 use OCP\App\IAppManager;
 use OCP\AppFramework\Controller;
@@ -56,6 +57,8 @@ class UsersController extends Controller {
 	private $isEncryptionAppEnabled;
 	/** @var bool contains the state of the admin recovery setting */
 	private $isRestoreEnabled = false;
+	/** @var SubAdminFactory */
+	private $subAdminFactory;
 
 	/**
 	 * @param string $appName
@@ -70,7 +73,9 @@ class UsersController extends Controller {
 	 * @param \OC_Defaults $defaults
 	 * @param \OC_Mail $mail
 	 * @param string $fromMailAddress
+	 * @param IURLGenerator $urlGenerator
 	 * @param IAppManager $appManager
+	 * @param SubAdminFactory $subAdminFactory
 	 */
 	public function __construct($appName,
 								IRequest $request,
@@ -85,7 +90,8 @@ class UsersController extends Controller {
 								\OC_Mail $mail,
 								$fromMailAddress,
 								IURLGenerator $urlGenerator,
-								IAppManager $appManager) {
+								IAppManager $appManager,
+								SubAdminFactory $subAdminFactory) {
 		parent::__construct($appName, $request);
 		$this->userManager = $userManager;
 		$this->groupManager = $groupManager;
@@ -98,6 +104,7 @@ class UsersController extends Controller {
 		$this->mail = $mail;
 		$this->fromMailAddress = $fromMailAddress;
 		$this->urlGenerator = $urlGenerator;
+		$this->subAdminFactory = $subAdminFactory;
 
 		// check for encryption state - TODO see formatUserForIndex
 		$this->isEncryptionAppEnabled = $appManager->isEnabledForUser('files_encryption');
@@ -161,7 +168,7 @@ class UsersController extends Controller {
 	private function getUsersForUID(array $userIDs) {
 		$users = [];
 		foreach ($userIDs as $uid => $displayName) {
-			$users[] = $this->userManager->get($uid);
+			$users[$uid] = $this->userManager->get($uid);
 		}
 		return $users;
 	}
@@ -196,7 +203,7 @@ class UsersController extends Controller {
 			}
 		}
 
-		$users = array();
+		$users = [];
 		if ($this->isAdmin) {
 
 			if($gid !== '') {
@@ -210,16 +217,34 @@ class UsersController extends Controller {
 			}
 
 		} else {
+			$subAdminOfGroups = $this->subAdminFactory->getSubAdminsOfGroups(
+				$this->userSession->getUser()->getUID()
+			);
 			// Set the $gid parameter to an empty value if the subadmin has no rights to access a specific group
-			if($gid !== '' && !in_array($gid, \OC_SubAdmin::getSubAdminsGroups($this->userSession->getUser()->getUID()))) {
+			if($gid !== '' && !in_array($gid, $subAdminOfGroups)) {
 				$gid = '';
 			}
 
-			$batch = $this->getUsersForUID($this->groupManager->displayNamesInGroup($gid, $pattern, $limit, $offset));
+			// Batch all groups the user is subadmin of when a group is specified
+			$batch = [];
+			if($gid === '') {
+				foreach($subAdminOfGroups as $group) {
+					$groupUsers = $this->groupManager->displayNamesInGroup($group, $pattern, $limit, $offset);
+					foreach($groupUsers as $uid => $displayName) {
+						$batch[$uid] = $displayName;
+					}
+				}
+			} else {
+				$batch = $this->groupManager->displayNamesInGroup($gid, $pattern, $limit, $offset);
+			}
+			$batch = $this->getUsersForUID($batch);
+
 			foreach ($batch as $user) {
 				// Only add the groups, this user is a subadmin of
-				$userGroups = array_intersect($this->groupManager->getUserGroupIds($user),
-					\OC_SubAdmin::getSubAdminsGroups($this->userSession->getUser()->getUID()));
+				$userGroups = array_values(array_intersect(
+					$this->groupManager->getUserGroupIds($user),
+					$subAdminOfGroups
+				));
 				$users[] = $this->formatUserForIndex($user, $userGroups);
 			}
 		}
@@ -235,8 +260,6 @@ class UsersController extends Controller {
 	 * @param array $groups
 	 * @param string $email
 	 * @return DataResponse
-	 *
-	 * TODO: Tidy up and write unit tests - code is mainly static method calls
 	 */
 	public function create($username, $password, array $groups=array(), $email='') {
 
@@ -249,17 +272,17 @@ class UsersController extends Controller {
 			);
 		}
 
-		// TODO FIXME get rid of the static calls to OC_Subadmin
 		if (!$this->isAdmin) {
+			$userId = $this->userSession->getUser()->getUID();
 			if (!empty($groups)) {
 				foreach ($groups as $key => $group) {
-					if (!\OC_SubAdmin::isGroupAccessible($this->userSession->getUser()->getUID(), $group)) {
+					if (!$this->subAdminFactory->isGroupAccessible($userId, $group)) {
 						unset($groups[$key]);
 					}
 				}
 			}
 			if (empty($groups)) {
-				$groups = \OC_SubAdmin::getSubAdminsGroups($this->userSession->getUser()->getUID());
+				$groups = $this->subAdminFactory->getSubAdminsOfGroups($userId);
 			}
 		}
 
@@ -276,7 +299,7 @@ class UsersController extends Controller {
 
 		if($user instanceof User) {
 			if($groups !== null) {
-				foreach( $groups as $groupName ) {
+				foreach($groups as $groupName) {
 					$group = $this->groupManager->get($groupName);
 
 					if(empty($group)) {
@@ -342,11 +365,10 @@ class UsersController extends Controller {
 	 *
 	 * @param string $id
 	 * @return DataResponse
-	 *
-	 * TODO: Tidy up and write unit tests - code is mainly static method calls
 	 */
 	public function destroy($id) {
-		if($this->userSession->getUser()->getUID() === $id) {
+		$userId = $this->userSession->getUser()->getUID();
+		if($userId === $id) {
 			return new DataResponse(
 				array(
 					'status' => 'error',
@@ -358,8 +380,7 @@ class UsersController extends Controller {
 			);
 		}
 
-		// FIXME: Remove this static function call at some point…
-		if(!$this->isAdmin && !\OC_SubAdmin::isUserAccessible($this->userSession->getUser()->getUID(), $id)) {
+		if(!$this->isAdmin && !$this->subAdminFactory->isUserAccessible($userId, $id)) {
 			return new DataResponse(
 				array(
 					'status' => 'error',
@@ -406,14 +427,12 @@ class UsersController extends Controller {
 	 * @param string $id
 	 * @param string $mailAddress
 	 * @return DataResponse
-	 *
-	 * TODO: Tidy up and write unit tests - code is mainly static method calls
 	 */
 	public function setMailAddress($id, $mailAddress) {
-		// FIXME: Remove this static function call at some point…
-		if($this->userSession->getUser()->getUID() !== $id
+		$userId = $this->userSession->getUser()->getUID();
+		if($userId !== $id
 			&& !$this->isAdmin
-			&& !\OC_SubAdmin::isUserAccessible($this->userSession->getUser()->getUID(), $id)) {
+			&& !$this->subAdminFactory->isUserAccessible($userId, $id)) {
 			return new DataResponse(
 				array(
 					'status' => 'error',