Fix unauthorized OCS status in provisioning

Signed-off-by: Joas Schilling <coding@schilljs.com>

5 files changed, 34 insertions, 26 deletions

diff --git a/apps/provisioning_api/lib/Controller/GroupsController.php b/apps/provisioning_api/lib/Controller/GroupsController.php
index b031c405046..e9b74a2723f 100644
--- a/apps/provisioning_api/lib/Controller/GroupsController.php
+++ b/apps/provisioning_api/lib/Controller/GroupsController.php
@@ -225,7 +225,7 @@ class GroupsController extends AUserData {
 			return new DataResponse(['users' => $usersDetails]);
 		}
 
-		throw new OCSException('User does not have access to specified group', OCSController::RESPOND_UNAUTHORISED);
+		throw new OCSException('The requested group could not be found', OCSController::RESPOND_NOT_FOUND);
 	}
 
 	/**
@@ -271,7 +271,7 @@ class GroupsController extends AUserData {
 
 			throw new OCSException('Not supported by backend', 101);
 		} else {
-			throw new OCSException('', OCSController::RESPOND_UNAUTHORISED);
+			throw new OCSException('', OCSController::RESPOND_UNKNOWN_ERROR);
 		}
 	}
 
diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php
index 115b955354b..0bc9f25eeb1 100644
--- a/apps/provisioning_api/lib/Controller/UsersController.php
+++ b/apps/provisioning_api/lib/Controller/UsersController.php
@@ -509,7 +509,7 @@ class UsersController extends AUserData {
 		$data = $this->getUserData($userId, $includeScopes);
 		// getUserData returns empty array if not enough permissions
 		if (empty($data)) {
-			throw new OCSException('', OCSController::RESPOND_UNAUTHORISED);
+			throw new OCSException('', OCSController::RESPOND_NOT_FOUND);
 		}
 		return new DataResponse($data);
 	}
@@ -602,7 +602,7 @@ class UsersController extends AUserData {
 
 		$targetUser = $this->userManager->get($userId);
 		if ($targetUser === null) {
-			throw new OCSException('', OCSController::RESPOND_UNAUTHORISED);
+			throw new OCSException('', OCSController::RESPOND_NOT_FOUND);
 		}
 
 		$permittedFields = [];
@@ -668,12 +668,12 @@ class UsersController extends AUserData {
 				$permittedFields[] = 'quota';
 			} else {
 				// No rights
-				throw new OCSException('', OCSController::RESPOND_UNAUTHORISED);
+				throw new OCSException('', OCSController::RESPOND_NOT_FOUND);
 			}
 		}
 		// Check if permitted to edit this field
 		if (!in_array($key, $permittedFields)) {
-			throw new OCSException('', OCSController::RESPOND_UNAUTHORISED);
+			throw new OCSException('', 103);
 		}
 		// Process the edit
 		switch ($key) {
@@ -690,7 +690,7 @@ class UsersController extends AUserData {
 						$quota = \OCP\Util::computerFileSize($quota);
 					}
 					if ($quota === false) {
-						throw new OCSException('Invalid quota value '.$value, 103);
+						throw new OCSException('Invalid quota value '.$value, 102);
 					}
 					if ($quota === -1) {
 						$quota = 'none';
@@ -788,14 +788,18 @@ class UsersController extends AUserData {
 
 		$targetUser = $this->userManager->get($userId);
 
-		if ($targetUser === null || $targetUser->getUID() === $currentLoggedInUser->getUID()) {
+		if ($targetUser === null) {
+			throw new OCSException('', OCSController::RESPOND_NOT_FOUND);
+		}
+
+		if ($targetUser->getUID() === $currentLoggedInUser->getUID()) {
 			throw new OCSException('', 101);
 		}
 
 		// If not permitted
 		$subAdminManager = $this->groupManager->getSubAdmin();
 		if (!$this->groupManager->isAdmin($currentLoggedInUser->getUID()) && !$subAdminManager->isUserAccessible($currentLoggedInUser, $targetUser)) {
-			throw new OCSException('', OCSController::RESPOND_UNAUTHORISED);
+			throw new OCSException('', OCSController::RESPOND_NOT_FOUND);
 		}
 
 		$this->remoteWipe->markAllTokensForWipe($targetUser);
@@ -816,14 +820,18 @@ class UsersController extends AUserData {
 
 		$targetUser = $this->userManager->get($userId);
 
-		if ($targetUser === null || $targetUser->getUID() === $currentLoggedInUser->getUID()) {
+		if ($targetUser === null) {
+			throw new OCSException('', OCSController::RESPOND_NOT_FOUND);
+		}
+
+		if ($targetUser->getUID() === $currentLoggedInUser->getUID()) {
 			throw new OCSException('', 101);
 		}
 
 		// If not permitted
 		$subAdminManager = $this->groupManager->getSubAdmin();
 		if (!$this->groupManager->isAdmin($currentLoggedInUser->getUID()) && !$subAdminManager->isUserAccessible($currentLoggedInUser, $targetUser)) {
-			throw new OCSException('', OCSController::RESPOND_UNAUTHORISED);
+			throw new OCSException('', OCSController::RESPOND_NOT_FOUND);
 		}
 
 		// Go ahead with the delete
@@ -877,7 +885,7 @@ class UsersController extends AUserData {
 		// If not permitted
 		$subAdminManager = $this->groupManager->getSubAdmin();
 		if (!$this->groupManager->isAdmin($currentLoggedInUser->getUID()) && !$subAdminManager->isUserAccessible($currentLoggedInUser, $targetUser)) {
-			throw new OCSException('', OCSController::RESPOND_UNAUTHORISED);
+			throw new OCSException('', OCSController::RESPOND_NOT_FOUND);
 		}
 
 		// enable/disable the user now
@@ -924,7 +932,7 @@ class UsersController extends AUserData {
 				return new DataResponse(['groups' => $groups]);
 			} else {
 				// Not permitted
-				throw new OCSException('', OCSController::RESPOND_UNAUTHORISED);
+				throw new OCSException('', OCSController::RESPOND_NOT_FOUND);
 			}
 		}
 	}
@@ -1132,7 +1140,7 @@ class UsersController extends AUserData {
 		if (!$subAdminManager->isUserAccessible($currentLoggedInUser, $targetUser)
 			&& !$this->groupManager->isAdmin($currentLoggedInUser->getUID())) {
 			// No rights
-			throw new OCSException('', OCSController::RESPOND_UNAUTHORISED);
+			throw new OCSException('', OCSController::RESPOND_NOT_FOUND);
 		}
 
 		$email = $targetUser->getEMailAddress();
diff --git a/apps/provisioning_api/lib/Middleware/ProvisioningApiMiddleware.php b/apps/provisioning_api/lib/Middleware/ProvisioningApiMiddleware.php
index a8bb8140060..cf51a148bba 100644
--- a/apps/provisioning_api/lib/Middleware/ProvisioningApiMiddleware.php
+++ b/apps/provisioning_api/lib/Middleware/ProvisioningApiMiddleware.php
@@ -30,10 +30,10 @@ namespace OCA\Provisioning_API\Middleware;
 
 use OCA\Provisioning_API\Middleware\Exceptions\NotSubAdminException;
 use OCP\AppFramework\Controller;
+use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\Response;
 use OCP\AppFramework\Middleware;
 use OCP\AppFramework\OCS\OCSException;
-use OCP\AppFramework\OCSController;
 use OCP\AppFramework\Utility\IControllerMethodReflector;
 
 class ProvisioningApiMiddleware extends Middleware {
@@ -84,7 +84,7 @@ class ProvisioningApiMiddleware extends Middleware {
 	 */
 	public function afterException($controller, $methodName, \Exception $exception) {
 		if ($exception instanceof NotSubAdminException) {
-			throw new OCSException($exception->getMessage(), OCSController::RESPOND_UNAUTHORISED);
+			throw new OCSException($exception->getMessage(), Http::STATUS_FORBIDDEN);
 		}
 
 		throw $exception;
diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php
index 1afe9be4319..d95edff8625 100644
--- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php
+++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php
@@ -1208,7 +1208,7 @@ class UsersControllerTest extends TestCase {
 
 	public function testGetUserDataAsSubAdminAndUserIsNotAccessible() {
 		$this->expectException(\OCP\AppFramework\OCS\OCSException::class);
-		$this->expectExceptionCode(997);
+		$this->expectExceptionCode(998);
 
 		$loggedInUser = $this->getMockBuilder(IUser::class)
 			->disableOriginalConstructor()
@@ -1681,7 +1681,7 @@ class UsersControllerTest extends TestCase {
 
 	public function testEditUserRegularUserSelfEditChangeQuota() {
 		$this->expectException(\OCP\AppFramework\OCS\OCSException::class);
-		$this->expectExceptionCode(997);
+		$this->expectExceptionCode(103);
 
 		$loggedInUser = $this->getMockBuilder(IUser::class)
 			->disableOriginalConstructor()
@@ -1759,7 +1759,7 @@ class UsersControllerTest extends TestCase {
 	public function testEditUserAdminUserSelfEditChangeInvalidQuota() {
 		$this->expectException(\OCP\AppFramework\OCS\OCSException::class);
 		$this->expectExceptionMessage('Invalid quota value ABC');
-		$this->expectExceptionCode(103);
+		$this->expectExceptionCode(102);
 
 		$loggedInUser = $this->getMockBuilder(IUser::class)->disableOriginalConstructor()->getMock();
 		$loggedInUser
@@ -2091,7 +2091,7 @@ class UsersControllerTest extends TestCase {
 
 	public function testEditUserSubadminUserInaccessible() {
 		$this->expectException(\OCP\AppFramework\OCS\OCSException::class);
-		$this->expectExceptionCode(997);
+		$this->expectExceptionCode(998);
 
 		$loggedInUser = $this->getMockBuilder(IUser::class)->disableOriginalConstructor()->getMock();
 		$loggedInUser
@@ -2131,7 +2131,7 @@ class UsersControllerTest extends TestCase {
 
 	public function testDeleteUserNotExistingUser() {
 		$this->expectException(\OCP\AppFramework\OCS\OCSException::class);
-		$this->expectExceptionCode(101);
+		$this->expectExceptionCode(998);
 
 		$loggedInUser = $this->getMockBuilder(IUser::class)->disableOriginalConstructor()->getMock();
 		$loggedInUser
@@ -2344,7 +2344,7 @@ class UsersControllerTest extends TestCase {
 
 	public function testDeleteUserAsSubAdminAndUserIsNotAccessible() {
 		$this->expectException(\OCP\AppFramework\OCS\OCSException::class);
-		$this->expectExceptionCode(997);
+		$this->expectExceptionCode(998);
 
 		$loggedInUser = $this->getMockBuilder(IUser::class)->disableOriginalConstructor()->getMock();
 		$loggedInUser
@@ -2525,7 +2525,7 @@ class UsersControllerTest extends TestCase {
 
 	public function testGetUsersGroupsForSubAdminUserAndUserIsInaccessible() {
 		$this->expectException(\OCP\AppFramework\OCS\OCSException::class);
-		$this->expectExceptionCode(997);
+		$this->expectExceptionCode(998);
 
 		$loggedInUser = $this->getMockBuilder(IUser::class)->disableOriginalConstructor()->getMock();
 		$loggedInUser
@@ -3526,7 +3526,7 @@ class UsersControllerTest extends TestCase {
 
 	public function testResendWelcomeMessageAsSubAdminAndUserIsNotAccessible() {
 		$this->expectException(\OCP\AppFramework\OCS\OCSException::class);
-		$this->expectExceptionCode(997);
+		$this->expectExceptionCode(998);
 
 		$loggedInUser = $this->getMockBuilder(IUser::class)
 			->disableOriginalConstructor()
diff --git a/apps/provisioning_api/tests/Middleware/ProvisioningApiMiddlewareTest.php b/apps/provisioning_api/tests/Middleware/ProvisioningApiMiddlewareTest.php
index 4565bde42a2..1977a4aa0f9 100644
--- a/apps/provisioning_api/tests/Middleware/ProvisioningApiMiddlewareTest.php
+++ b/apps/provisioning_api/tests/Middleware/ProvisioningApiMiddlewareTest.php
@@ -27,8 +27,8 @@ namespace OCA\Provisioning_API\Tests\Middleware;
 use OCA\Provisioning_API\Middleware\Exceptions\NotSubAdminException;
 use OCA\Provisioning_API\Middleware\ProvisioningApiMiddleware;
 use OCP\AppFramework\Controller;
+use OCP\AppFramework\Http;
 use OCP\AppFramework\OCS\OCSException;
-use OCP\AppFramework\OCSController;
 use OCP\AppFramework\Utility\IControllerMethodReflector;
 use Test\TestCase;
 
@@ -115,7 +115,7 @@ class ProvisioningApiMiddlewareTest extends TestCase {
 		} catch (OCSException $e) {
 			$this->assertFalse($forwared);
 			$this->assertSame($exception->getMessage(), $e->getMessage());
-			$this->assertSame(OCSController::RESPOND_UNAUTHORISED, $e->getCode());
+			$this->assertSame(Http::STATUS_FORBIDDEN, $e->getCode());
 		} catch (\Exception $e) {
 			$this->assertTrue($forwared);
 			$this->assertSame($exception, $e);