Use a Symfony Form for jury clarifications.

Also sanitize only after converting to markdown. Fixes #2311

Author: nickygerritsen

webapp/config/packages/html_sanitizer.yaml

@@ -0,0 +1,6 @@
+framework:
+    html_sanitizer:
+        sanitizers:
+            app.clarification_sanitizer:
+                allow_safe_elements: true
+                allow_relative_medias: true

webapp/public/js/domjudge.js

@@ -420,10 +420,10 @@ function toggleExpand(event)
 function clarificationAppendAnswer() {
     if ( $('#clar_answers').val() == '_default' ) { return; }
     var selected = $("#clar_answers option:selected").text();
-    var textbox = $('#bodytext');
+    var textbox = $('#jury_clarification_message');
     textbox.val(textbox.val().replace(/\n$/, "") + '\n' + selected);
     textbox.scrollTop(textbox[0].scrollHeight);
-    previewClarification($('#bodytext') , $('#messagepreview'));
+    previewClarification($('#jury_clarification_message') , $('#messagepreview'));
 }
 
 function confirmLogout() {

webapp/src/Controller/Jury/ClarificationController.php

@@ -8,13 +8,14 @@
 use App\Entity\Problem;
 use App\Entity\Team;
 use App\Entity\User;
+use App\Form\Type\JuryClarificationType;
 use App\Service\ConfigurationService;
 use App\Service\DOMJudgeService;
 use App\Service\EventLogService;
 use App\Utils\Utils;
 use Doctrine\ORM\EntityManagerInterface;
 use Doctrine\ORM\Query\Expr\Join;
-use Symfony\Component\HtmlSanitizer\HtmlSanitizerInterface;
+use Symfony\Component\Form\FormInterface;
 use Symfony\Component\HttpKernel\Attribute\MapQueryParameter;
 use Symfony\Component\Security\Http\Attribute\IsGranted;
 use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
@@ -122,32 +123,62 @@ public function indexAction(
     }
 
     #[Route(path: '/{id<\d+>}', name: 'jury_clarification')]
-    public function viewAction(int $id): Response
+    public function viewAction(Request $request, int $id): Response
     {
         $clarification = $this->em->getRepository(Clarification::class)->find($id);
         if (!$clarification) {
             throw new NotFoundHttpException(sprintf('Clarification with ID %s not found', $id));
         }
 
-        $clardata = ['list'=>[]];
-        $clardata['clarform'] = $this->getClarificationFormData($clarification->getSender());
-        $clardata['showExternalId'] = $this->eventLogService->externalIdFieldForEntity(Clarification::class);
-
-        $categories = $clardata['clarform']['subjects'];
-        $queues     = $this->config->get('clar_queues');
-        $clar_answers = $this->config->get('clar_answers');
-
         if ($inReplyTo = $clarification->getInReplyTo()) {
             $clarification = $inReplyTo;
         }
-        $clarlist = [$clarification];
+        $clarificationList = [$clarification];
         $replies = $clarification->getReplies();
-        foreach ($replies as $clar_reply) {
-            $clarlist[] = $clar_reply;
+        foreach ($replies as $reply) {
+            $clarificationList[] = $reply;
+        }
+
+        $parameters = ['list' => []];
+        $parameters['showExternalId'] = $this->eventLogService->externalIdFieldForEntity(Clarification::class);
+
+        $formData = [
+            'recipient' => JuryClarificationType::RECIPIENT_MUST_SELECT,
+            'subject' => sprintf('%s-%s', $clarification->getContest()->getCid(), $clarification->getProblem()?->getProbid() ?? $clarification->getCategory()),
+        ];
+        if ($clarification->getRecipient()) {
+            $formData['recipient'] = $clarification->getRecipient()->getTeamid();
+        }
+
+        /** @var Clarification $lastClarification */
+        $lastClarification = end($clarificationList);
+        $formData['message'] = "> " . str_replace("\n", "\n> ", Utils::wrapUnquoted($lastClarification->getBody())) . "\n\n";
+
+        $form = $this->createForm(JuryClarificationType::class, $formData, ['limit_to_team' => $clarification->getSender()]);
+
+        $form->handleRequest($request);
+
+        if ($form->isSubmitted() && $form->isValid()) {
+            return $this->processSubmittedClarification($form, $clarification);
+        }
+
+        $parameters['form'] = $form->createView();
+
+        $categories = array_flip($form->get('subject')->getConfig()->getOptions()['choices']);
+        $groupedCategories = [];
+        foreach ($categories as $key => $value) {
+            if ($this->dj->getCurrentContest()) {
+                $groupedCategories[$this->dj->getCurrentContest()->getShortname()][$key] = $value;
+            } else {
+                [$group] = explode(' - ', $value, 2);
+                $groupedCategories[$group][$key] = $value;
+            }
         }
+        $parameters['subjects'] = $groupedCategories;
+        $queues = $this->config->get('clar_queues');
+        $clarificationAnswers = $this->config->get('clar_answers');
 
-        $concernsteam = null;
-        foreach ($clarlist as $clar) {
+        foreach ($clarificationList as $clar) {
             $data = ['clarid' => $clar->getClarid(), 'externalid' => $clar->getExternalid()];
             $data['time'] = $clar->getSubmittime();
 
@@ -161,7 +192,6 @@ public function viewAction(int $id): Response
             if ($fromteam = $clar->getSender()) {
                 $data['from_teamname'] = $fromteam->getEffectiveName();
                 $data['from_team'] = $fromteam;
-                $concernsteam = $fromteam->getTeamid();
             }
             if ($toteam = $clar->getRecipient()) {
                 $data['to_teamname'] = $toteam->getEffectiveName();
@@ -181,7 +211,7 @@ public function viewAction(int $id): Response
                 $concernssubject = "";
             }
             if ($concernssubject !== "") {
-                $data['subject'] = $categories[$clarcontest][$concernssubject];
+                $data['subject'] = $categories[$concernssubject];
             } else {
                 $data['subject'] = $clarcontest;
             }
@@ -192,104 +222,36 @@ public function viewAction(int $id): Response
             $data['answered'] = $clar->getAnswered();
 
             $data['body'] = $clar->getBody();
-            $clardata['list'][] = $data;
-        }
-
-        if ($concernsteam) {
-            $clardata['clarform']['toteam'] = $concernsteam;
-        }
-        if ($concernssubject) {
-            $clardata['clarform']['onsubject'] = $concernssubject;
-        }
-
-        $clardata['clarform']['quotedtext'] = "> " . str_replace("\n", "\n> ", Utils::wrapUnquoted($data['body'])) . "\n\n";
-        $clardata['clarform']['queues'] = $queues;
-        $clardata['clarform']['answers'] = $clar_answers;
-
-        return $this->render('jury/clarification.html.twig',
-            $clardata
-        );
-    }
-
-    /**
-     * @return array{teams: array<string|int, string>, subjects: array<string, array<string, string>>}
-     */
-    protected function getClarificationFormData(?Team $team = null): array
-    {
-        $teamlist = [];
-        $em = $this->em;
-        if ($team !== null) {
-            $teamlist[$team->getTeamid()] = sprintf("%s (t%s)", $team->getEffectiveName(), $team->getTeamid());
-        } else {
-            $teams = $em->getRepository(Team::class)->findAll();
-            foreach ($teams as $team) {
-                $teamlist[$team->getTeamid()] = sprintf("%s (t%s)", $team->getEffectiveName(), $team->getTeamid());
-            }
-        }
-        asort($teamlist, SORT_STRING | SORT_FLAG_CASE);
-        $teamlist = ['domjudge-must-select' => '(select...)', '' => 'ALL'] + $teamlist;
-
-        $data= ['teams' => $teamlist ];
-
-        $subject_options = [];
-
-        $categories = $this->config->get('clar_categories');
-        $contest = $this->dj->getCurrentContest();
-        $hasCurrentContest = $contest !== null;
-        if ($hasCurrentContest) {
-            $contests = [$contest->getCid() => $contest];
-        } else {
-            $contests = $this->dj->getCurrentContests();
-        }
-
-        /** @var ContestProblem[] $contestproblems */
-        $contestproblems = $this->em->createQueryBuilder()
-            ->from(ContestProblem::class, 'cp')
-            ->select('cp, p')
-            ->innerJoin('cp.problem', 'p')
-            ->where('cp.contest IN (:contests)')
-            ->setParameter('contests', $contests)
-            ->orderBy('cp.shortname')
-            ->getQuery()->getResult();
-
-        foreach ($contests as $cid => $cdata) {
-            $cshort = $cdata->getShortName();
-            $namePrefix = '';
-            if (!$hasCurrentContest) {
-                $namePrefix = $cshort . ' - ';
-            }
-            foreach ($categories as $name => $desc) {
-                $subject_options[$cshort]["$cid-$name"] = "$namePrefix $desc";
-            }
-
-            foreach ($contestproblems as $cp) {
-                if ($cp->getCid()!=$cid) {
-                    continue;
-                }
-                $subject_options[$cshort]["$cid-" . $cp->getProbid()] =
-                    $namePrefix . $cp->getShortname() . ': ' . $cp->getProblem()->getName();
-            }
+            $parameters['list'][] = $data;
         }
 
-        $data['subjects'] = $subject_options;
+        $parameters['queues'] = $queues;
+        $parameters['answers'] = $clarificationAnswers;
 
-        return $data;
+        return $this->render('jury/clarification.html.twig', $parameters);
     }
 
-    #[Route(path: '/send', methods: ['GET'], name: 'jury_clarification_new')]
+    #[Route(path: '/send', name: 'jury_clarification_new')]
     public function composeClarificationAction(
+        Request $request,
         #[MapQueryParameter]
-        ?string $teamto = null
+        ?string $teamto = null,
     ): Response {
-        // TODO: Use a proper Symfony form for this.
-
-        $data = $this->getClarificationFormData();
+        $formData = ['recipient' => JuryClarificationType::RECIPIENT_MUST_SELECT];
 
         if ($teamto !== null) {
-            $data['toteam'] = $teamto;
+            $formData['recipient'] = $teamto;
+        }
+
+        $form = $this->createForm(JuryClarificationType::class, $formData);
+
+        $form->handleRequest($request);
+
+        if ($form->isSubmitted() && $form->isValid()) {
+            return $this->processSubmittedClarification($form);
         }
 
-        return $this->render('jury/clarification_new.html.twig', ['clarform' => $data]);
+        return $this->render('jury/clarification_new.html.twig', ['form' => $form->createView()]);
     }
 
     #[Route(path: '/{clarId<\d+>}/claim', name: 'jury_clarification_claim')]
@@ -387,30 +349,25 @@ public function changeQueueAction(Request $request, int $clarId): Response
         return $this->redirectToRoute('jury_clarification', ['id' => $clarId]);
     }
 
-    #[Route(path: '/send', methods: ['POST'], name: 'jury_clarification_send')]
-    public function sendAction(Request $request, HtmlSanitizerInterface $htmlSanitizer): Response
-    {
+    protected function processSubmittedClarification(
+        FormInterface $form,
+        ?Clarification $inReplTo = null
+    ): Response {
+        $formData = $form->getData();
         $clarification = new Clarification();
+        $clarification->setInReplyTo($inReplTo);
 
-        if ($respid = $request->request->get('id')) {
-            $respclar = $this->em->getRepository(Clarification::class)->find($respid);
-            $clarification->setInReplyTo($respclar);
-        }
-
-        $sendto = $request->request->get('sendto');
-        if (empty($sendto)) {
-            $sendto = null;
-        } elseif ($sendto === 'domjudge-must-select') {
-            $message = 'You must select somewhere to send the clarification to.';
-            $this->addFlash('danger', $message);
-            return $this->redirectToRoute('jury_clarification_send');
+        $recipient = $formData['recipient'];
+        if (empty($recipient)) {
+            $recipient = null;
         } else {
-            $team = $this->em->getReference(Team::class, $sendto);
+            $team = $this->em->getReference(Team::class, $recipient);
             $clarification->setRecipient($team);
         }
 
-        $problem = $request->request->get('problem');
-        [$cid, $probid] = explode('-', $problem);
+
+        $subject = $formData['subject'];
+        [$cid, $probid] = explode('-', $subject);
 
         $contest = $this->em->getReference(Contest::class, $cid);
         $clarification->setContest($contest);
@@ -428,8 +385,8 @@ public function sendAction(Request $request, HtmlSanitizerInterface $htmlSanitiz
             }
         }
 
-        if ($respid) {
-            $queue = $respclar->getQueue();
+        if ($inReplTo) {
+            $queue = $inReplTo->getQueue();
         } else {
             $queue = $this->config->get('clar_default_problem_queue');
             if ($queue === "") {
@@ -440,14 +397,13 @@ public function sendAction(Request $request, HtmlSanitizerInterface $htmlSanitiz
 
         $clarification->setJuryMember($this->getUser()->getUserIdentifier());
         $clarification->setAnswered(true);
-        $clarification->setBody($htmlSanitizer->sanitize($request->request->get('bodytext')));
+        $clarification->setBody($formData['message']);
         $clarification->setSubmittime(Utils::now());
 
         $this->em->persist($clarification);
-        if ($respid) {
-            $respclar->setAnswered(true);
-            $respclar->setJuryMember($this->getUser()->getUserIdentifier());
-            $this->em->persist($respclar);
+        if ($inReplTo) {
+            $inReplTo->setAnswered(true);
+            $inReplTo->setJuryMember($this->getUser()->getUserIdentifier());
         }
         $this->em->flush();
 
@@ -457,9 +413,8 @@ public function sendAction(Request $request, HtmlSanitizerInterface $htmlSanitiz
         // Reload clarification to make sure we have a fresh one after calling the event log service.
         $clarification = $this->em->getRepository(Clarification::class)->find($clarId);
 
-        if ($sendto) {
-            $team = $this->em->getRepository(Team::class)->find($sendto);
-            $team->addUnreadClarification($clarification);
+        if ($clarification->getRecipient()) {
+            $clarification->getRecipient()->addUnreadClarification($clarification);
         } else {
             $teams = $this->em->getRepository(Team::class)->findAll();
             foreach ($teams as $team) {

webapp/src/Controller/Jury/TeamController.php

@@ -167,7 +167,7 @@ public function indexAction(): Response
             $teamactions[] = [
                 'icon' => 'envelope',
                 'title' => 'send clarification to this team',
-                'link' => $this->generateUrl('jury_clarification_send', [
+                'link' => $this->generateUrl('jury_clarification_new', [
                     'teamto' => $t->getTeamId(),
                 ])
             ];

webapp/src/Controller/RootController.php

@@ -45,12 +45,12 @@ public function markdownPreview(
         Request $request,
         #[Autowire(service: 'twig.runtime.markdown')]
         MarkdownRuntime $markdownRuntime,
-        HtmlSanitizerInterface $htmlSanitizer
+        HtmlSanitizerInterface $appClarificationSanitizer,
     ): JsonResponse {
         $message = $request->request->get('message');
         if ($message === null) {
             throw new BadRequestHttpException('A message is required');
         }
-        return new JsonResponse(['html' => $markdownRuntime->convert($htmlSanitizer->sanitize($message))]);
+        return new JsonResponse(['html' => $appClarificationSanitizer->sanitize($markdownRuntime->convert($message))]);
     }
 }