Merge pull request #404 from nextcloud/backport/403/stable28

[stable28] Change api signature to require etag

Author: lukasdotcom

lib/Controller/ApprovalController.php

@@ -81,13 +81,13 @@ public function getPendingNodes(?int $since = null): DataResponse {
 	 * Approve a file
 	 *
 	 * @param int $fileId
-	 * @param string|null $etag
+	 * @param string $etag
 	 * @return DataResponse
 	 */
 	#[NoAdminRequired]
-	public function approve(int $fileId, ?string $etag = ''): DataResponse {
+	public function approve(int $fileId, string $etag): DataResponse {
 		try {
-			if ($this->approvalService->approve($fileId, $this->userId, $etag ?? '')) {
+			if ($this->approvalService->approve($fileId, $this->userId, $etag)) {
 				return new DataResponse([]);
 			}
 			return new DataResponse([], Http::STATUS_BAD_REQUEST);
@@ -103,13 +103,13 @@ public function approve(int $fileId, ?string $etag = ''): DataResponse {
 	 * Reject a file
 	 *
 	 * @param int $fileId
-	 * @param string|null $etag
+	 * @param string $etag
 	 * @return DataResponse
 	 */
 	#[NoAdminRequired]
-	public function reject(int $fileId, ?string $etag = ''): DataResponse {
+	public function reject(int $fileId, string $etag): DataResponse {
 		try {
-			if ($this->approvalService->reject($fileId, $this->userId, $etag ?? '')) {
+			if ($this->approvalService->reject($fileId, $this->userId, $etag)) {
 				return new DataResponse([]);
 			}
 			return new DataResponse([], Http::STATUS_BAD_REQUEST);

lib/Service/ApprovalService.php

@@ -351,13 +351,13 @@ public function getApprovalState(int $fileId, ?string $userId, bool $userHasAcce
 	 *
 	 * @param int $fileId
 	 * @param string|null $userId
-	 * @param string $etag optional etag of the file to check if it has changed since approval was requested
+	 * @param string $etag
 	 * @return bool success
 	 * @throws OutdatedEtagException
 	 */
-	public function approve(int $fileId, ?string $userId, string $etag = ''): bool {
+	public function approve(int $fileId, ?string $userId, string $etag): 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
@@ -393,13 +393,13 @@ public function approve(int $fileId, ?string $userId, string $etag = ''): bool {
 	 *
 	 * @param int $fileId
 	 * @param string|null $userId
-	 * @param string $etag optional etag of the file to check if it has changed since approval was requested
+	 * @param string $etag
 	 * @return bool success
 	 * @throws OutdatedEtagException
 	 */
-	public function reject(int $fileId, ?string $userId, string $etag = ''): bool {
+	public function reject(int $fileId, ?string $userId, string $etag): 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

tests/unit/Service/ApprovalServiceTest.php

@@ -35,15 +35,11 @@
 use OCP\IL10N;
 use OCP\IUserManager;
 use OCP\Notification\IManager as INotificationManager;
-
 use OCP\Security\ICrypto;
 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 {
@@ -410,24 +406,24 @@ public function testApproval() {
 		// approve failures
 		// tag does not exist
 		$this->ruleService->saveRule($idRule3, $idTagPending3, -1, $idTagRejected3, $approvers, $requesters, $description);
-		$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);
 
 		// 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);
-		$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);
 
 		// 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']);
 	}