Merge pull request owncloud#366 from owncloud/bugfix/wrong-ingroup-check

Make sure to check IUser directly for IGroup->inGroup
eszkadev · Dec 3, 2020 · d652bba · d652bba
2 parents d954fee + 01bdb29
commit d652bba
Show file tree

Hide file tree

Showing 3 changed files with 45 additions and 10 deletions.
diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php
@@ -50,7 +50,9 @@ private function registerServices() {
 				$c->query('ICacheFactory'),
 				$c->query('Logger'),
 				$storage,
-				$c->query('OCP\App\IAppManager')
+				$c->query('OCP\App\IAppManager'),
+				$c->query('ServerContainer')->getGroupManager(),
+				$c->query('ServerContainer')->getUserManager()
 			);
 		});
 		$container->registerService('SettingsController', function ($c) {

diff --git a/lib/Controller/DocumentController.php b/lib/Controller/DocumentController.php
@@ -17,6 +17,7 @@
 use \OCP\AppFramework\Controller;
 use \OCP\Constants;
 use OCP\Files\File;
+use OCP\IGroupManager;
 use \OCP\IRequest;
 use \OCP\IConfig;
 use \OCP\IL10N;
@@ -33,6 +34,7 @@
 use \OCA\Richdocuments\Storage;
 use \OCA\Richdocuments\Http\DownloadResponse;
 use \OCA\Richdocuments\Http\ResponseException;
+use OCP\IUserManager;
 
 class DocumentController extends Controller {
 	private $uid;
@@ -43,6 +45,14 @@ class DocumentController extends Controller {
 	private $logger;
 	private $storage;
 	private $appManager;
+	/**
+	 * @var IGroupManager
+	 */
+	private $groupManager;
+	/**
+	 * @var IUserManager
+	 */
+	private $userManager;
 	const ODT_TEMPLATE_PATH = '/assets/odttemplate.odt';
 
 	// Signifies LOOL that document has been changed externally in this storage
@@ -57,7 +67,9 @@ public function __construct($appName,
 								ICacheFactory $cache,
 								ILogger $logger,
 								Storage $storage,
-								IAppManager $appManager) {
+								IAppManager $appManager,
+								IGroupManager $groupManager,
+								IUserManager $userManager) {
 		parent::__construct($appName, $request);
 		$this->uid = $uid;
 		$this->l10n = $l10n;
@@ -67,6 +79,8 @@ public function __construct($appName,
 		$this->logger = $logger;
 		$this->storage = $storage;
 		$this->appManager = $appManager;
+		$this->groupManager = $groupManager;
+		$this->userManager = $userManager;
 	}
 
 	/**
@@ -127,7 +141,7 @@ private function isTester() {
 		$testgroups = \array_filter(\explode('|', $this->appConfig->getAppValue('test_server_groups')));
 		$this->logger->debug('Testgroups are {testgroups}', [ 'app' => $this->appName, 'testgroups' => $testgroups ]);
 		foreach ($testgroups as $testgroup) {
-			$test = \OC::$server->getGroupManager()->get($testgroup);
+			$test = $this->groupManager->get($testgroup);
 			if ($test !== null && \sizeof($test->searchUsers($uid)) > 0) {
 				$this->logger->debug('User {user} found in {group}', ['app' => $this->appName, 'user' => $uid, 'group' => $testgroup ]);
 				$tester = true;
@@ -550,10 +564,15 @@ private function isAllowedEditor($editorUid) {
 		$editGroups = \array_filter(\explode('|', $this->appConfig->getAppValue('edit_groups')));
 		$isAllowed = true;
 		if (\count($editGroups) > 0) {
+			$editor = $this->userManager->get($editorUid);
+			if (!$editor) {
+				return false;
+			}
+
 			$isAllowed = false;
 			foreach ($editGroups as $editGroup) {
-				$editorGroup = \OC::$server->getGroupManager()->get($editGroup);
-				if ($editorGroup !== null && \sizeof($editorGroup->searchUsers($editorUid)) > 0) {
+				$editorGroup = $this->groupManager->get($editGroup);
+				if ($editorGroup !== null && $editorGroup->inGroup($editor)) {
 					$this->logger->debug("Editor {editor} is in edit group {group}", [
 						'app' => $this->appName,
 						'editor' => $editorUid,
@@ -812,7 +831,7 @@ public function wopiCheckFileInfo($documentId) {
 		}
 
 		if ($res['editor'] && $res['editor'] != '') {
-			$editor = \OC::$server->getUserManager()->get($res['editor']);
+			$editor = $this->userManager->get($res['editor']);
 			$editorId = $editor->getUID();
 			$editorDisplayName = $editor->getDisplayName();
 			$editorEmail = $editor->getEMailAddress();
@@ -1119,7 +1138,7 @@ private function putRelative($fileId, $owner, $editor, $suggested) {
 	 */
 	private function getFileHandle($fileId, $owner, $editor) {
 		if ($editor && $editor != '') {
-			$user = \OC::$server->getUserManager()->get($editor);
+			$user = $this->userManager->get($editor);
 			if (!$user) {
 				$this->logger->warning('wopiPutFile(): No such user', ['app' => $this->appName]);
 				return null;

diff --git a/tests/unit/Controller/DocumentControllerTest.php b/tests/unit/Controller/DocumentControllerTest.php
@@ -12,13 +12,15 @@
 
 use \OCA\Richdocuments\Controller\DocumentController;
 use OCP\App\IAppManager;
+use OCP\IGroupManager;
 use \OCP\IRequest;
 use \OCP\IConfig;
 use \OCA\Richdocuments\AppConfig;
 use \OCP\IL10N;
 use \OCP\ICacheFactory;
 use \OCP\ILogger;
 use \OCA\Richdocuments\Storage;
+use OCP\IUserManager;
 use phpDocumentor\Reflection\Types\This;
 
 /**
@@ -59,7 +61,15 @@ class DocumentControllerTest extends \Test\TestCase {
 	/**
 	 * @var IAppManager
 	 */
-	private $appMAnager;
+	private $appManager;
+	/**
+	 * @var IGroupManager
+	 */
+	private $groupManager;
+	/**
+	 * @var IUserManager
+	 */
+	private $userManager;
 	/**
 	 * @var DocumentController
 	 */
@@ -74,7 +84,9 @@ public function setUp(): void {
 		$this->cache = $this->createMock(ICacheFactory::class);
 		$this->logger = $this->createMock(ILogger::class);
 		$this->storage = $this->createMock(Storage::class);
-		$this->appMAnager = $this->createMock(IAppManager::class);
+		$this->appManager = $this->createMock(IAppManager::class);
+		$this->groupManager = $this->createMock(IGroupManager::class);
+		$this->userManager = $this->createMock(IUserManager::class);
 		$this->documentController = new DocumentController(
 			'richdocuments',
 			$this->request,
@@ -85,7 +97,9 @@ public function setUp(): void {
 			$this->cache,
 			$this->logger,
 			$this->storage,
-			$this->appMAnager
+			$this->appManager,
+			$this->groupManager,
+			$this->userManager
 		);
 	}