fix(ban): Simplify validation and document known errors

Signed-off-by: Joas Schilling <coding@schilljs.com>

6 files changed, 30 insertions, 31 deletions

diff --git a/lib/Controller/BanController.php b/lib/Controller/BanController.php
index bc709824a..8c246ca14 100644
--- a/lib/Controller/BanController.php
+++ b/lib/Controller/BanController.php
@@ -16,6 +16,7 @@ use OCA\Talk\Service\BanService;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\Attribute\PublicPage;
 use OCP\AppFramework\Http\DataResponse;
+use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\IRequest;
 
 /**
@@ -25,7 +26,8 @@ class BanController extends AEnvironmentAwareController {
 	public function __construct(
 		string $appName,
 		IRequest $request,
-		protected BanService $banService
+		protected BanService $banService,
+		protected ITimeFactory $timeFactory,
 	) {
 		parent::__construct($appName, $request);
 	}
@@ -39,7 +41,7 @@ class BanController extends AEnvironmentAwareController {
 	 * @psalm-param Attendee::ACTOR_*|'ip' $actorType Type of actor to ban, or `ip` when banning a clients remote address
 	 * @param string $actorId Actor ID or the IP address or range in case of type `ip`
 	 * @param string $internalNote Optional internal note
-	 * @return DataResponse<Http::STATUS_OK, TalkBan, array{}>|DataResponse<Http::STATUS_BAD_REQUEST, array{error: string}, array{}>
+	 * @return DataResponse<Http::STATUS_OK, TalkBan, array{}>|DataResponse<Http::STATUS_BAD_REQUEST, array{error: 'bannedActor'|'internalNote'}, array{}>
 	 *
 	 * 200: Ban successfully
 	 * 400: Actor information is invalid
@@ -58,14 +60,16 @@ class BanController extends AEnvironmentAwareController {
 				$this->room->getId(),
 				$actorId,
 				$actorType,
-				null,
+				$this->timeFactory->getDateTime(),
 				$internalNote
 			);
 
 			return new DataResponse($ban->jsonSerialize(), Http::STATUS_OK);
 		} catch (\InvalidArgumentException $e) {
+			/** @var 'bannedActor'|'internalNote' $message */
+			$message = $e->getMessage();
 			return new DataResponse([
-				'error' => $e->getMessage(),
+				'error' => $message,
 			], Http::STATUS_BAD_REQUEST);
 		}
 	}
diff --git a/lib/Service/BanService.php b/lib/Service/BanService.php
index 96f67b97c..d176f620b 100644
--- a/lib/Service/BanService.php
+++ b/lib/Service/BanService.php
@@ -12,42 +12,27 @@ use DateTime;
 use OCA\Talk\Model\Ban;
 use OCA\Talk\Model\BanMapper;
 use OCP\AppFramework\Db\DoesNotExistException;
-use OCP\AppFramework\Utility\ITimeFactory;
 
 class BanService {
 
 	public function __construct(
 		protected BanMapper $banMapper,
-		protected ITimeFactory $timeFactory,
 	) {
 	}
 
 	/**
-	 * Validate the ban data.
+	 * Create a new ban
+	 *
+	 * @throws \InvalidArgumentException
 	 */
-	private function validateBanData(string $actorId, string $actorType, int $roomId, string $bannedId, string $bannedType, ?DateTime $bannedTime, ?string $internalNote): void {
-		if (empty($bannedId)) {
-			throw new \InvalidArgumentException("invalid_bannedId.");
-		}
-
-		if (empty($bannedType)) {
-			throw new \InvalidArgumentException("invalid_bannedType.");
+	public function createBan(string $actorId, string $actorType, int $roomId, string $bannedId, string $bannedType, DateTime $bannedTime, string $internalNote): Ban {
+		if (empty($bannedId) || empty($bannedType)) {
+			throw new \InvalidArgumentException('bannedActor');
 		}
 
 		if (empty($internalNote)) {
-			throw new \InvalidArgumentException("invalid_internalNote.");
-		}
-
-		if ($bannedTime !== null && !$bannedTime instanceof DateTime) {
-			throw new \InvalidArgumentException("invalid_bannedTime.");
+			throw new \InvalidArgumentException('internalNote');
 		}
-	}
-
-	/**
-	 * Create a new ban
-	 */
-	public function createBan(string $actorId, string $actorType, int $roomId, string $bannedId, string $bannedType, ?DateTime $bannedTime, ?string $internalNote): Ban {
-		$this->validateBanData($actorId, $actorType, $roomId, $bannedId, $bannedType, $bannedTime, $internalNote);
 
 		$ban = new Ban();
 		$ban->setActorId($actorId);
@@ -55,7 +40,7 @@ class BanService {
 		$ban->setRoomId($roomId);
 		$ban->setBannedId($bannedId);
 		$ban->setBannedType($bannedType);
-		$ban->setBannedTime($bannedTime ?? new \DateTime());
+		$ban->setBannedTime($bannedTime);
 		$ban->setInternalNote($internalNote);
 
 		return $this->banMapper->insert($ban);
diff --git a/openapi-full.json b/openapi-full.json
index 9b1bb467b..f9f21adc8 100644
--- a/openapi-full.json
+++ b/openapi-full.json
@@ -2099,7 +2099,11 @@
                                                     ],
                                                     "properties": {
                                                         "error": {
-                                                            "type": "string"
+                                                            "type": "string",
+                                                            "enum": [
+                                                                "bannedActor",
+                                                                "internalNote"
+                                                            ]
                                                         }
                                                     }
                                                 }
diff --git a/openapi.json b/openapi.json
index 19bb7a71d..d70cb5769 100644
--- a/openapi.json
+++ b/openapi.json
@@ -1986,7 +1986,11 @@
                                                     ],
                                                     "properties": {
                                                         "error": {
-                                                            "type": "string"
+                                                            "type": "string",
+                                                            "enum": [
+                                                                "bannedActor",
+                                                                "internalNote"
+                                                            ]
                                                         }
                                                     }
                                                 }
diff --git a/src/types/openapi/openapi-full.ts b/src/types/openapi/openapi-full.ts
index a1d38bd4d..43e07580a 100644
--- a/src/types/openapi/openapi-full.ts
+++ b/src/types/openapi/openapi-full.ts
@@ -2428,7 +2428,8 @@ export interface operations {
                         ocs: {
                             meta: components["schemas"]["OCSMeta"];
                             data: {
-                                error: string;
+                                /** @enum {string} */
+                                error: "bannedActor" | "internalNote";
                             };
                         };
                     };
diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts
index f521d639b..1487919d2 100644
--- a/src/types/openapi/openapi.ts
+++ b/src/types/openapi/openapi.ts
@@ -1913,7 +1913,8 @@ export interface operations {
                         ocs: {
                             meta: components["schemas"]["OCSMeta"];
                             data: {
-                                error: string;
+                                /** @enum {string} */
+                                error: "bannedActor" | "internalNote";
                             };
                         };
                     };