mbk-lab
/
nextcloud
2
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Fix unauthorized OCS status in provisioning 

Signed-off-by: Joas Schilling <coding@schilljs.com>
This commit is contained in:
Joas Schilling 2021-04-21 08:58:35 +02:00
parent 0599a8060c
commit b6c6527705
No known key found for this signature in database
GPG Key ID: 7076EA9751AACDDA
12 changed files with 120 additions and 104 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
apps/provisioning_api/lib/Controller/GroupsController.php
View File

@ -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);
}
}

32
apps/provisioning_api/lib/Controller/UsersController.php
View File

@ -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();

4
apps/provisioning_api/lib/Middleware/ProvisioningApiMiddleware.php
View File

@ -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;

16
apps/provisioning_api/tests/Controller/UsersControllerTest.php
View File

@ -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()

4
apps/provisioning_api/tests/Middleware/ProvisioningApiMiddlewareTest.php
View File

@ -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);

27
build/integration/features/provisioning-v1.feature
View File

@ -176,8 +176,8 @@ Feature: provisioning
When sending "PUT" to "/cloud/users/brand-new-user" with
| key | phoneScope |
| value | v2-private |
Then the OCS status code should be "997"
And the HTTP status code should be "401"
Then the OCS status code should be "103"
And the HTTP status code should be "200"
Scenario: Search by phone number
Given As an "admin"
@ -234,11 +234,12 @@ Feature: provisioning
And the HTTP status code should be "200"
Scenario: adding user to a group without privileges
Given As an "brand-new-user"
Given user "brand-new-user" exists
And As an "brand-new-user"
When sending "POST" to "/cloud/users/brand-new-user/groups" with
| groupid | new-group |
Then the OCS status code should be "997"
And the HTTP status code should be "401"
Then the OCS status code should be "403"
And the HTTP status code should be "200"
Scenario: adding user to a group
Given As an "admin"
@ -523,8 +524,8 @@ Feature: provisioning
And Assure user "subadmin" is subadmin of group "new-group"
And As an "subadmin"
When sending "PUT" to "/cloud/users/user1/disable"
Then the OCS status code should be "997"
Then the HTTP status code should be "401"
Then the OCS status code should be "998"
Then the HTTP status code should be "200"
And As an "admin"
And user "user1" is enabled
@ -539,8 +540,8 @@ Feature: provisioning
And Assure user "subadmin" is subadmin of group "new-group"
And As an "subadmin"
When sending "PUT" to "/cloud/users/another-admin/disable"
Then the OCS status code should be "997"
Then the HTTP status code should be "401"
Then the OCS status code should be "998"
Then the HTTP status code should be "200"
And As an "admin"
And user "another-admin" is enabled
@ -615,8 +616,8 @@ Feature: provisioning
And user "user2" exists
And As an "user1"
When sending "PUT" to "/cloud/users/user2/disable"
Then the OCS status code should be "997"
And the HTTP status code should be "401"
Then the OCS status code should be "403"
And the HTTP status code should be "200"
And As an "admin"
And user "user2" is enabled
@ -627,8 +628,8 @@ Feature: provisioning
And assure user "user2" is disabled
And As an "user1"
When sending "PUT" to "/cloud/users/user2/enable"
Then the OCS status code should be "997"
And the HTTP status code should be "401"
Then the OCS status code should be "403"
And the HTTP status code should be "200"
And As an "admin"
And user "user2" is disabled

2
build/integration/sharing_features/sharing-v1-part2.feature
View File

@ -919,7 +919,7 @@ Feature: sharing
And As an "user1"
When Deleting last share
Then the OCS status code should be "403"
And the HTTP status code should be "401"
And the HTTP status code should be "200"
Scenario: Keep usergroup shares (#22143)
Given As an "admin"

4
build/integration/sharing_features/sharing-v1-part3.feature
View File

@ -403,7 +403,7 @@ Feature: sharing
And Updating last share with
| permissions | 19 |
Then the OCS status code should be "403"
And the HTTP status code should be "401"
And the HTTP status code should be "200"
Scenario: do not allow to increase permissions on non received share with user with resharing rights
Given As an "admin"
@ -427,7 +427,7 @@ Feature: sharing
And Updating last share with
| permissions | 19 |
Then the OCS status code should be "403"
And the HTTP status code should be "401"
And the HTTP status code should be "200"
Scenario: do not allow to increase link share permissions on reshare
Given As an "admin"

2
build/integration/sharing_features/sharing-v1.feature
View File

@ -54,7 +54,7 @@ Feature: sharing
| shareWith | a-room-token |
| shareType | 10 |
Then the OCS status code should be "403"
And the HTTP status code should be "401"
And the HTTP status code should be "200"
Scenario: Creating a new mail share
Given dummy mail server is listening

12
lib/private/AppFramework/Middleware/OCSMiddleware.php
View File

@ -100,8 +100,7 @@ class OCSMiddleware extends Middleware {
* we need to catch the response and convert it to a proper OCS response.
*/
if ($controller instanceof OCSController && !($response instanceof BaseResponse)) {
if ($response->getStatus() === Http::STATUS_UNAUTHORIZED ||
$response->getStatus() === Http::STATUS_FORBIDDEN) {
if ($response->getStatus() === Http::STATUS_UNAUTHORIZED) {
$message = '';
if ($response instanceof JSONResponse) {
/** @var DataResponse $response */
@ -110,6 +109,15 @@ class OCSMiddleware extends Middleware {
return $this->buildNewResponse($controller, OCSController::RESPOND_UNAUTHORISED, $message);
}
if ($response->getStatus() === Http::STATUS_FORBIDDEN) {
$message = '';
if ($response instanceof JSONResponse) {
/** @var DataResponse $response */
$message = $response->getData()['message'];
}
return $this->buildNewResponse($controller, Http::STATUS_FORBIDDEN, $message);
}
}
return $response;

2
lib/private/AppFramework/OCS/V1Response.php
View File

@ -37,7 +37,7 @@ class V1Response extends BaseResponse {
*/
public function getStatus() {
$status = parent::getStatus();
if ($status === Http::STATUS_FORBIDDEN || $status === OCSController::RESPOND_UNAUTHORISED) {
if ($status === OCSController::RESPOND_UNAUTHORISED) {
return Http::STATUS_UNAUTHORIZED;
}

51
tests/lib/AppFramework/Middleware/OCSMiddlewareTest.php
View File

@ -98,9 +98,12 @@ class OCSMiddlewareTest extends \Test\TestCase {
$OCSMiddleware = new OCSMiddleware($this->request);
$OCSMiddleware->beforeController($controller, 'method');
try {
if ($forward) {
$this->expectException(get_class($exception));
$this->expectExceptionMessage($exception->getMessage());
}
$result = $OCSMiddleware->afterException($controller, 'method', $exception);
$this->assertFalse($forward);
$this->assertInstanceOf(V1Response::class, $result);
@ -112,16 +115,8 @@ class OCSMiddlewareTest extends \Test\TestCase {
$this->assertSame($code, $result->getOCSStatus());
}
if ($exception instanceof OCSForbiddenException) {
$this->assertSame(Http::STATUS_UNAUTHORIZED, $result->getStatus());
} else {
$this->assertSame(Http::STATUS_OK, $result->getStatus());
}
} catch (\Exception $e) {
$this->assertTrue($forward);
$this->assertEquals($exception, $e);
}
}
/**
* @dataProvider dataAfterException
@ -139,9 +134,12 @@ class OCSMiddlewareTest extends \Test\TestCase {
$OCSMiddleware = new OCSMiddleware($this->request);
$OCSMiddleware->beforeController($controller, 'method');
try {
if ($forward) {
$this->expectException(get_class($exception));
$this->expectExceptionMessage($exception->getMessage());
}
$result = $OCSMiddleware->afterException($controller, 'method', $exception);
$this->assertFalse($forward);
$this->assertInstanceOf(V2Response::class, $result);
@ -152,10 +150,6 @@ class OCSMiddlewareTest extends \Test\TestCase {
$this->assertSame($code, $result->getOCSStatus());
}
$this->assertSame($code, $result->getStatus());
} catch (\Exception $e) {
$this->assertTrue($forward);
$this->assertEquals($exception, $e);
}
}
/**
@ -174,9 +168,12 @@ class OCSMiddlewareTest extends \Test\TestCase {
$OCSMiddleware = new OCSMiddleware($this->request);
$OCSMiddleware->beforeController($controller, 'method');
try {
if ($forward) {
$this->expectException(get_class($exception));
$this->expectExceptionMessage($exception->getMessage());
}
$result = $OCSMiddleware->afterException($controller, 'method', $exception);
$this->assertFalse($forward);
$this->assertInstanceOf(V2Response::class, $result);
@ -187,10 +184,6 @@ class OCSMiddlewareTest extends \Test\TestCase {
$this->assertSame($code, $result->getOCSStatus());
}
$this->assertSame($code, $result->getStatus());
} catch (\Exception $e) {
$this->assertTrue($forward);
$this->assertEquals($exception, $e);
}
}
public function dataAfterController() {
@ -205,7 +198,7 @@ class OCSMiddlewareTest extends \Test\TestCase {
[$OCSController, new Http\Response(), false],
[$OCSController, new Http\JSONResponse(), false],
[$OCSController, new Http\JSONResponse(['message' => 'foo']), false],
[$OCSController, new Http\JSONResponse(['message' => 'foo'], Http::STATUS_UNAUTHORIZED), true],
[$OCSController, new Http\JSONResponse(['message' => 'foo'], Http::STATUS_UNAUTHORIZED), true, OCSController::RESPOND_UNAUTHORISED],
[$OCSController, new Http\JSONResponse(['message' => 'foo'], Http::STATUS_FORBIDDEN), true],
[$controller, new Http\Response(), false],
@ -223,8 +216,9 @@ class OCSMiddlewareTest extends \Test\TestCase {
* @param Controller $controller
* @param Http\Response $response
* @param bool $converted
* @param int $convertedOCSStatus
*/
public function testAfterController($controller, $response, $converted) {
public function testAfterController($controller, $response, $converted, $convertedOCSStatus = 0) {
$OCSMiddleware = new OCSMiddleware($this->request);
$newResponse = $OCSMiddleware->afterController($controller, 'foo', $response);
@ -233,8 +227,13 @@ class OCSMiddlewareTest extends \Test\TestCase {
} else {
$this->assertInstanceOf(BaseResponse::class, $newResponse);
$this->assertSame($response->getData()['message'], $this->invokePrivate($newResponse, 'statusMessage'));
$this->assertSame(\OCP\AppFramework\OCSController::RESPOND_UNAUTHORISED, $newResponse->getOCSStatus());
$this->assertSame(Http::STATUS_UNAUTHORIZED, $newResponse->getStatus());
if ($convertedOCSStatus) {
$this->assertSame($convertedOCSStatus, $newResponse->getOCSStatus());
} else {
$this->assertSame($response->getStatus(), $newResponse->getOCSStatus());
}
$this->assertSame($response->getStatus(), $newResponse->getStatus());
}
}
}