Skip to content

feat: add validators to check values in services #4059

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

Merged
juliusknorr merged 2 commits into from
Oct 31, 2022
Merged

feat: add validators to check values in services #4059

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1217b37
feat: add validators to check values in services
luka-nextcloud Sep 13, 2022
bfec6f5
Add some tests for parameter validation
juliusknorr Oct 31, 2022
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
2 changes: 1 addition & 1 deletion cypress/e2e/boardFeatures.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ describe('Board', function() {
})

it('Can create a board', function() {
const board = 'Test'
const board = 'TestBoard'

cy.intercept({
method: 'POST',
Expand Down
27 changes: 12 additions & 15 deletions lib/Service/AssignmentService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
use OCA\Deck\NoPermissionException;
use OCA\Deck\NotFoundException;
use OCA\Deck\Notification\NotificationHelper;
use OCA\Deck\Validators\AssignmentServiceValidator;
use OCP\AppFramework\Db\DoesNotExistException;
use OCP\AppFramework\Db\Entity;
use OCP\AppFramework\Db\MultipleObjectsReturnedException;
Expand Down Expand Up @@ -76,6 +77,11 @@ class AssignmentService {
private $eventDispatcher;
/** @var string|null */
private $currentUser;
/**
* @var AssignmentServiceValidator
*/
private $assignmentServiceValidator;


public function __construct(
PermissionService $permissionService,
Expand All @@ -86,8 +92,10 @@ public function __construct(
ActivityManager $activityManager,
ChangeHelper $changeHelper,
IEventDispatcher $eventDispatcher,
AssignmentServiceValidator $assignmentServiceValidator,
$userId
) {
$this->assignmentServiceValidator = $assignmentServiceValidator;
$this->permissionService = $permissionService;
$this->cardMapper = $cardMapper;
$this->assignedUsersMapper = $assignedUsersMapper;
Expand All @@ -96,6 +104,8 @@ public function __construct(
$this->changeHelper = $changeHelper;
$this->activityManager = $activityManager;
$this->eventDispatcher = $eventDispatcher;

$this->assignmentServiceValidator->check(compact('userId'));
$this->currentUser = $userId;
}

Expand All @@ -109,13 +119,7 @@ public function __construct(
* @throws DoesNotExistException
*/
public function assignUser($cardId, $userId, int $type = Assignment::TYPE_USER) {
if (is_numeric($cardId) === false) {
throw new BadRequestException('card id must be a number');
}

if ($userId === false || $userId === null) {
throw new BadRequestException('user id must be provided');
}
$this->assignmentServiceValidator->check(compact('cardId', 'userId'));

if ($type !== Assignment::TYPE_USER && $type !== Assignment::TYPE_GROUP) {
throw new BadRequestException('Invalid type provided for assignemnt');
Expand Down Expand Up @@ -168,16 +172,9 @@ public function assignUser($cardId, $userId, int $type = Assignment::TYPE_USER)
* @throws MultipleObjectsReturnedException
*/
public function unassignUser($cardId, $userId, $type = 0) {
$this->assignmentServiceValidator->check(compact('cardId', 'userId'));
$this->permissionService->checkPermission($this->cardMapper, $cardId, Acl::PERMISSION_EDIT);

if (is_numeric($cardId) === false) {
throw new BadRequestException('card id must be a number');
}

if ($userId === false || $userId === null) {
throw new BadRequestException('user must be provided');
}

$assignments = $this->assignedUsersMapper->findAll($cardId);
foreach ($assignments as $assignment) {
if ($assignment->getParticipant() === $userId && $assignment->getType() === $type) {
Expand Down
46 changes: 21 additions & 25 deletions lib/Service/AttachmentService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
use OCA\Deck\NotFoundException;
use OCA\Deck\Cache\AttachmentCacheHelper;
use OCA\Deck\StatusException;
use OCA\Deck\Validators\AttachmentServiceValidator;
use OCP\AppFramework\Db\IMapperException;
use OCP\AppFramework\Http\Response;
use OCP\IL10N;
Expand All @@ -60,17 +61,22 @@ class AttachmentService {
/** @var ChangeHelper */
private $changeHelper;
private IUserManager $userManager;

public function __construct(AttachmentMapper $attachmentMapper,
CardMapper $cardMapper,
IUserManager $userManager,
ChangeHelper $changeHelper,
PermissionService $permissionService,
Application $application,
AttachmentCacheHelper $attachmentCacheHelper,
$userId,
IL10N $l10n,
ActivityManager $activityManager) {
/** @var AttachmentServiceValidator */
private AttachmentServiceValidator $attachmentServiceValidator;

public function __construct(
AttachmentMapper $attachmentMapper,
CardMapper $cardMapper,
IUserManager $userManager,
ChangeHelper $changeHelper,
PermissionService $permissionService,
Application $application,
AttachmentCacheHelper $attachmentCacheHelper,
$userId,
IL10N $l10n,
ActivityManager $activityManager,
AttachmentServiceValidator $attachmentServiceValidator
) {
$this->attachmentMapper = $attachmentMapper;
$this->cardMapper = $cardMapper;
$this->permissionService = $permissionService;
Expand All @@ -81,6 +87,7 @@ public function __construct(AttachmentMapper $attachmentMapper,
$this->activityManager = $activityManager;
$this->changeHelper = $changeHelper;
$this->userManager = $userManager;
$this->attachmentServiceValidator = $attachmentServiceValidator;

// Register shipped attachment services
// TODO: move this to a plugin based approach once we have different types of attachments
Expand Down Expand Up @@ -187,17 +194,7 @@ public function count($cardId) {
* @throws BadRequestException
*/
public function create($cardId, $type, $data) {
if (is_numeric($cardId) === false) {
throw new BadRequestException('card id must be a number');
}

if ($type === false || $type === null) {
throw new BadRequestException('type must be provided');
}

if ($data === false || $data === null) {
//throw new BadRequestException('data must be provided');
}
$this->attachmentServiceValidator->check(compact('cardId', 'type', 'data'));

$this->permissionService->checkPermission($this->cardMapper, $cardId, Acl::PERMISSION_EDIT);

Expand Down Expand Up @@ -283,6 +280,8 @@ public function display($cardId, $attachmentId, $type = 'deck_file') {
* @throws NoPermissionException
*/
public function update($cardId, $attachmentId, $data, $type = 'deck_file') {
$this->attachmentServiceValidator->check(compact('cardId', 'type', 'data'));

try {
$service = $this->getService($type);
} catch (InvalidAttachmentType $e) {
Expand All @@ -304,9 +303,6 @@ public function update($cardId, $attachmentId, $data, $type = 'deck_file') {
}
}

if ($data === false || $data === null) {
//throw new BadRequestException('data must be provided');
}
try {
$attachment = $this->attachmentMapper->find($attachmentId);
} catch (\Exception $e) {
Expand Down
101 changes: 15 additions & 86 deletions lib/Service/BoardService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
use OCA\Deck\Db\LabelMapper;
use OCP\IUserManager;
use OCA\Deck\BadRequestException;
use OCA\Deck\Validators\BoardServiceValidator;
use OCP\IURLGenerator;
use OCP\Server;
use Psr\Container\ContainerExceptionInterface;
Expand All @@ -79,6 +80,7 @@ class BoardService {
private ?array $boardsCache = null;
private IURLGenerator $urlGenerator;
private IDBConnection $connection;
private BoardServiceValidator $boardServiceValidator;

public function __construct(
BoardMapper $boardMapper,
Expand All @@ -98,6 +100,7 @@ public function __construct(
ChangeHelper $changeHelper,
IURLGenerator $urlGenerator,
IDBConnection $connection,
BoardServiceValidator $boardServiceValidator,
?string $userId
) {
$this->boardMapper = $boardMapper;
Expand All @@ -118,6 +121,7 @@ public function __construct(
$this->urlGenerator = $urlGenerator;
$this->cardMapper = $cardMapper;
$this->connection = $connection;
$this->boardServiceValidator = $boardServiceValidator;
}

/**
Expand Down Expand Up @@ -182,6 +186,7 @@ public function findAll($since = -1, $details = null, $includeArchived = true) {
* @throws BadRequestException
*/
public function find($boardId) {
$this->boardServiceValidator->check(compact('boardId'));
if ($this->boardsCache && isset($this->boardsCache[$boardId])) {
return $this->boardsCache[$boardId];
}
Expand Down Expand Up @@ -236,9 +241,7 @@ private function getBoardPrerequisites() {
* @throws BadRequestException
*/
public function isArchived($mapper, $id) {
if (is_numeric($id) === false) {
throw new BadRequestException('id must be a number');
}
$this->boardServiceValidator->check(compact('id'));

try {
$boardId = $id;
Expand All @@ -265,13 +268,7 @@ public function isArchived($mapper, $id) {
* @throws BadRequestException
*/
public function isDeleted($mapper, $id) {
if ($mapper === false || $mapper === null) {
throw new BadRequestException('mapper must be provided');
}

if (is_numeric($id) === false) {
throw new BadRequestException('id must be a number');
}
$this->boardServiceValidator->check(compact('mapper', 'id'));

try {
$boardId = $id;
Expand All @@ -297,17 +294,7 @@ public function isDeleted($mapper, $id) {
* @throws BadRequestException
*/
public function create($title, $userId, $color) {
if ($title === false || $title === null) {
throw new BadRequestException('title must be provided');
}

if ($userId === false || $userId === null) {
throw new BadRequestException('userId must be provided');
}

if ($color === false || $color === null) {
throw new BadRequestException('color must be provided');
}
$this->boardServiceValidator->check(compact('title', 'userId', 'color'));

if (!$this->permissionService->canCreate()) {
throw new NoPermissionException('Creating boards has been disabled for your account.');
Expand Down Expand Up @@ -358,9 +345,7 @@ public function create($title, $userId, $color) {
* @throws BadRequestException
*/
public function delete($id) {
if (is_numeric($id) === false) {
throw new BadRequestException('board id must be a number');
}
$this->boardServiceValidator->check(compact('id'));

$this->permissionService->checkPermission($this->boardMapper, $id, Acl::PERMISSION_MANAGE);
$board = $this->find($id);
Expand All @@ -383,9 +368,7 @@ public function delete($id) {
* @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException
*/
public function deleteUndo($id) {
if (is_numeric($id) === false) {
throw new BadRequestException('board id must be a number');
}
$this->boardServiceValidator->check(compact('id'));

$this->permissionService->checkPermission($this->boardMapper, $id, Acl::PERMISSION_MANAGE);
$board = $this->find($id);
Expand All @@ -406,9 +389,7 @@ public function deleteUndo($id) {
* @throws BadRequestException
*/
public function deleteForce($id) {
if (is_numeric($id) === false) {
throw new BadRequestException('id must be a number');
}
$this->boardServiceValidator->check(compact('id'));

$this->permissionService->checkPermission($this->boardMapper, $id, Acl::PERMISSION_MANAGE);
$board = $this->find($id);
Expand All @@ -429,21 +410,7 @@ public function deleteForce($id) {
* @throws BadRequestException
*/
public function update($id, $title, $color, $archived) {
if (is_numeric($id) === false) {
throw new BadRequestException('board id must be a number');
}

if ($title === false || $title === null) {
throw new BadRequestException('title must be provided');
}

if ($color === false || $color === null) {
throw new BadRequestException('color must be provided');
}

if (is_bool($archived) === false) {
throw new BadRequestException('archived must be a boolean');
}
$this->boardServiceValidator->check(compact('id', 'title', 'color', 'archived'));

$this->permissionService->checkPermission($this->boardMapper, $id, Acl::PERMISSION_MANAGE);
$board = $this->find($id);
Expand Down Expand Up @@ -493,29 +460,7 @@ public function enrichWithBoardSettings(Board $board) {
* @throws \OCA\Deck\NoPermissionException
*/
public function addAcl($boardId, $type, $participant, $edit, $share, $manage) {
if (is_numeric($boardId) === false) {
throw new BadRequestException('board id must be a number');
}

if ($type === false || $type === null) {
throw new BadRequestException('type must be provided');
}

if ($participant === false || $participant === null) {
throw new BadRequestException('participant must be provided');
}

if ($edit === null) {
throw new BadRequestException('edit must be provided');
}

if ($share === null) {
throw new BadRequestException('share must be provided');
}

if ($manage === null) {
throw new BadRequestException('manage must be provided');
}
$this->boardServiceValidator->check(compact('boardId', 'type', 'participant', 'edit', 'share', 'manage'));

$this->permissionService->checkPermission($this->boardMapper, $boardId, Acl::PERMISSION_SHARE);
[$edit, $share, $manage] = $this->applyPermissions($boardId, $edit, $share, $manage);
Expand Down Expand Up @@ -561,21 +506,7 @@ public function addAcl($boardId, $type, $participant, $edit, $share, $manage) {
* @throws BadRequestException
*/
public function updateAcl($id, $edit, $share, $manage) {
if (is_numeric($id) === false) {
throw new BadRequestException('id must be a number');
}

if ($edit === null) {
throw new BadRequestException('edit must be provided');
}

if ($share === null) {
throw new BadRequestException('share must be provided');
}

if ($manage === null) {
throw new BadRequestException('manage must be provided');
}
$this->boardServiceValidator->check(compact('id', 'edit', 'share', 'manage'));

$this->permissionService->checkPermission($this->aclMapper, $id, Acl::PERMISSION_SHARE);

Expand Down Expand Up @@ -643,9 +574,7 @@ public function deleteAcl(int $id): ?Acl {
* @throws BadRequestException
*/
public function clone($id, $userId) {
if (is_numeric($id) === false) {
throw new BadRequestException('board id must be a number');
}
$this->boardServiceValidator->check(compact('id', 'userId'));

$this->permissionService->checkPermission($this->boardMapper, $id, Acl::PERMISSION_READ);

Expand Down
Loading