Merge pull request #403 from nextcloud/change-api-signature

Change api signature to require etag

Author: lukasdotcom

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([]);

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

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']);
 	}