diff --git a/lib/Controller/ApprovalController.php b/lib/Controller/ApprovalController.php index 944d4fed..c702c983 100644 --- a/lib/Controller/ApprovalController.php +++ b/lib/Controller/ApprovalController.php @@ -78,12 +78,12 @@ public function getPendingNodes(?int $since = null): DataResponse { * Approve a file * * @param int $fileId + * @param string $etag * @param string|null $message - * @param string|null $etag * @return DataResponse */ #[NoAdminRequired] - public function approve(int $fileId, ?string $message = '', ?string $etag = ''): DataResponse { + public function approve(int $fileId, string $etag, ?string $message = ''): DataResponse { try { if ($this->approvalService->approve($fileId, $this->userId, $message, $etag)) { return new DataResponse([]); @@ -98,12 +98,12 @@ public function approve(int $fileId, ?string $message = '', ?string $etag = ''): * Reject a file * * @param int $fileId + * @param string $etag * @param string|null $message - * @param string|null $etag * @return DataResponse */ #[NoAdminRequired] - public function reject(int $fileId, ?string $message = '', ?string $etag = ''): DataResponse { + public function reject(int $fileId, string $etag, ?string $message = ''): DataResponse { try { if ($this->approvalService->reject($fileId, $this->userId, $message, $etag)) { return new DataResponse([]); diff --git a/lib/Service/ApprovalService.php b/lib/Service/ApprovalService.php index e70599c8..aa3dc80d 100644 --- a/lib/Service/ApprovalService.php +++ b/lib/Service/ApprovalService.php @@ -351,14 +351,14 @@ public function getApprovalState(int $fileId, ?string $userId, bool $userHasAcce * * @param int $fileId * @param string|null $userId + * @param string $etag * @param string $message - * @param string $etag optional etag of the file to check if it has changed since approval was requested * @return bool success * @throws OutdatedEtagException */ - public function approve(int $fileId, ?string $userId, string $message = '', string $etag = ''): bool { + public function approve(int $fileId, ?string $userId, string $etag, string $message = ''): bool { $fileState = $this->getApprovalState($fileId, $userId); - if ($etag !== '' && $etag !== $this->getEtag($fileId)) { + if ($etag !== $this->getEtag($fileId)) { throw new OutdatedEtagException(); } // if file has pending tag and user is authorized to approve it @@ -394,14 +394,14 @@ public function approve(int $fileId, ?string $userId, string $message = '', stri * * @param int $fileId * @param string|null $userId + * @param string $etag * @param string $message - * @param string $etag optional etag of the file to check if it has changed since approval was requested * @return bool success * @throws OutdatedEtagException */ - public function reject(int $fileId, ?string $userId, string $message = '', string $etag = ''): bool { + public function reject(int $fileId, ?string $userId, string $etag, string $message = ''): bool { $fileState = $this->getApprovalState($fileId, $userId); - if ($etag !== '' && $etag !== $this->getEtag($fileId)) { + if ($etag !== $this->getEtag($fileId)) { throw new OutdatedEtagException(); } // if file has pending tag and user is authorized to approve it diff --git a/tests/unit/Service/ApprovalServiceTest.php b/tests/unit/Service/ApprovalServiceTest.php index 44608584..266636c7 100644 --- a/tests/unit/Service/ApprovalServiceTest.php +++ b/tests/unit/Service/ApprovalServiceTest.php @@ -20,14 +20,10 @@ use OCP\IL10N; use OCP\IUserManager; use OCP\Notification\IManager as INotificationManager; - use OCP\Share\IManager as IShareManager; - use OCP\Share\IShare; - use OCP\SystemTag\ISystemTagManager; use OCP\SystemTag\ISystemTagObjectMapper; - use Psr\Log\LoggerInterface; class ApprovalServiceTest extends TestCase { @@ -395,24 +391,24 @@ public function testApproval() { // approve failures // tag does not exist $this->ruleService->saveRule($idRule3, $idTagPending3, -1, $idTagRejected3, $approvers, $requesters, $description, $unapproveWhenModified); - $result = $this->approvalService->approve($fileToReject->getId(), 'user1'); + $result = $this->approvalService->approve($fileToReject->getId(), 'user1', $fileToReject->getEtag()); $this->assertFalse($result); $this->ruleService->saveRule($idRule3, $idTagPending3, $idTagApproved3, $idTagRejected3, $approvers, $requesters, $description, $unapproveWhenModified); // approve - $this->approvalService->approve($fileToApprove->getId(), 'user1'); + $this->approvalService->approve($fileToApprove->getId(), 'user1', $fileToApprove->getEtag()); $stateForUser1 = $this->approvalService->getApprovalState($fileToApprove->getId(), 'user1'); $this->assertEquals(Application::STATE_APPROVED, $stateForUser1['state']); // reject failures // tag does not exist $this->ruleService->saveRule($idRule3, $idTagPending3, $idTagApproved3, -1, $approvers, $requesters, $description, $unapproveWhenModified); - $result = $this->approvalService->reject($fileToReject->getId(), 'user1'); + $result = $this->approvalService->reject($fileToReject->getId(), 'user1', $fileToReject->getEtag()); $this->assertFalse($result); $this->ruleService->saveRule($idRule3, $idTagPending3, $idTagApproved3, $idTagRejected3, $approvers, $requesters, $description, $unapproveWhenModified); // reject - $this->approvalService->reject($fileToReject->getId(), 'user1'); + $this->approvalService->reject($fileToReject->getId(), 'user1', $fileToReject->getEtag()); $stateForUser1 = $this->approvalService->getApprovalState($fileToReject->getId(), 'user1'); $this->assertEquals(Application::STATE_REJECTED, $stateForUser1['state']); }