Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Properly throw errors on users management #16746

Closed
wants to merge 1 commit into from
Closed

Properly throw errors on users management #16746

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions apps/provisioning_api/lib/Controller/UsersController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
use OCP\App\IAppManager;
use OCP\AppFramework\Http\DataResponse;
use OCP\AppFramework\OCS\OCSException;
use OCP\AppFramework\OCS\OCSBadRequestException;
use OCP\AppFramework\OCS\OCSForbiddenException;
use OCP\IConfig;
use OCP\IGroup;
Expand Down Expand Up @@ -458,7 +459,7 @@ public function editUser(string $userId, string $key, string $value): DataRespon

$targetUser = $this->userManager->get($userId);
if ($targetUser === null) {
throw new OCSException('', \OCP\API::RESPOND_UNAUTHORISED);
throw new OCSException('Unknown user', 101);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be consistent with others methods on this page 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But now it reveals to anyone if a user exists, while before it required it to be your own user or you having subadmin/admin permissions

}

$permittedFields = [];
Expand Down Expand Up @@ -515,12 +516,12 @@ public function editUser(string $userId, string $key, string $value): DataRespon
$permittedFields[] = 'quota';
} else {
// No rights
throw new OCSException('', \OCP\API::RESPOND_UNAUTHORISED);
throw new OCSException('You must be admin or subadmin', \OCP\API::RESPOND_UNAUTHORISED);
}
}
// Check if permitted to edit this field
if (!in_array($key, $permittedFields)) {
throw new OCSException('', \OCP\API::RESPOND_UNAUTHORISED);
throw new OCSException('You`re not allowed to edit '.$key, \OCP\API::RESPOND_UNAUTHORISED);
}
// Process the edit
switch($key) {
Expand Down Expand Up @@ -574,7 +575,7 @@ public function editUser(string $userId, string $key, string $value): DataRespon
if (filter_var($value, FILTER_VALIDATE_EMAIL) || $value === '') {
$targetUser->setEMailAddress($value);
} else {
throw new OCSException('', 102);
throw new OCSException('Invalid email address', 102);
}
break;
case AccountManager::PROPERTY_PHONE:
Expand All @@ -588,7 +589,7 @@ public function editUser(string $userId, string $key, string $value): DataRespon
}
break;
default:
throw new OCSException('', 103);
throw new OCSException('Invalid property', 103);
}
return new DataResponse();
}
Expand Down Expand Up @@ -638,7 +639,7 @@ public function deleteUser(string $userId): DataResponse {
$targetUser = $this->userManager->get($userId);

if ($targetUser === null || $targetUser->getUID() === $currentLoggedInUser->getUID()) {
throw new OCSException('', 101);
throw new OCSException('Unknown user', 101);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unknown is not correct for the second part of the if condition

}

// If not permitted
Expand Down