From 6a57ab15402c17c09378fa1746339258afae9f0c Mon Sep 17 00:00:00 2001 From: Lukas Schaefer Date: Wed, 25 Mar 2026 10:11:40 -0400 Subject: [PATCH] Change api signature to require etag Signed-off-by: Lukas Schaefer --- lib/Controller/ApprovalController.php | 8 ++++---- lib/Service/ApprovalService.php | 12 ++++++------ tests/unit/Service/ApprovalServiceTest.php | 12 ++++-------- 3 files changed, 14 insertions(+), 18 deletions(-) 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 68237694..c264b0c6 100644 --- a/lib/Service/ApprovalService.php +++ b/lib/Service/ApprovalService.php @@ -353,14 +353,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 @@ -396,14 +396,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 3ef7f4e7..7ad15c1f 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 { @@ -365,24 +361,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']); }