diff --git a/webapp/config/packages/html_sanitizer.yaml b/webapp/config/packages/html_sanitizer.yaml new file mode 100644 index 0000000000..fea42ed40a --- /dev/null +++ b/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 diff --git a/webapp/public/js/domjudge.js b/webapp/public/js/domjudge.js index 891faf7abc..1cba40b466 100644 --- a/webapp/public/js/domjudge.js +++ b/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() { diff --git a/webapp/src/Controller/Jury/ClarificationController.php b/webapp/src/Controller/Jury/ClarificationController.php index df2a3708c6..dd73acbbf0 100644 --- a/webapp/src/Controller/Jury/ClarificationController.php +++ b/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, subjects: array>} - */ - 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) { diff --git a/webapp/src/Controller/Jury/TeamController.php b/webapp/src/Controller/Jury/TeamController.php index 60dba907c7..ccd2b7e55c 100644 --- a/webapp/src/Controller/Jury/TeamController.php +++ b/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(), ]) ]; diff --git a/webapp/src/Controller/RootController.php b/webapp/src/Controller/RootController.php index d9563beded..eb330a17f1 100644 --- a/webapp/src/Controller/RootController.php +++ b/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))]); } } diff --git a/webapp/src/Form/Type/JuryClarificationType.php b/webapp/src/Form/Type/JuryClarificationType.php new file mode 100644 index 0000000000..16e3c0bec9 --- /dev/null +++ b/webapp/src/Form/Type/JuryClarificationType.php @@ -0,0 +1,114 @@ + static::RECIPIENT_MUST_SELECT, + 'ALL' => '', + ]; + + $limitToTeam = $options['limit_to_team'] ?? null; + if ($limitToTeam) { + $recipientOptions[sprintf("%s (t%s)", $limitToTeam->getEffectiveName(), $limitToTeam->getTeamid())] = $limitToTeam->getTeamid(); + } else { + /** @var Team|null $limitToTeam */ + $teams = $this->em->getRepository(Team::class)->findAll(); + foreach ($teams as $team) { + $recipientOptions[sprintf("%s (t%s)", $team->getEffectiveName(), $team->getTeamid())] = $team->getTeamid(); + } + } + + $subjectOptions = []; + $subjectGroupBy = null; + + $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) { + $contestShortName = $cdata->getShortName(); + $namePrefix = ''; + if (!$hasCurrentContest) { + $namePrefix = $contestShortName . ' - '; + $subjectGroupBy = function (string $choice, string $key) { + return substr($key, 0, strpos($key, '-')); + }; + } + foreach ($categories as $name => $desc) { + $subjectOptions["$namePrefix $desc"] = "$cid-$name"; + } + + foreach ($contestproblems as $cp) { + if ($cp->getCid() != $cid) { + continue; + } + $subjectOptions[$namePrefix . $cp->getShortname() . ': ' . $cp->getProblem()->getName()] = "$cid-" . $cp->getProbid(); + } + } + + $builder->add('recipient', ChoiceType::class, [ + 'label' => 'Send to', + 'choices' => $recipientOptions, + 'constraints' => [ + new NotEqualTo('domjudge-must-select', message: 'You must select somewhere to send the clarification to.'), + ], + ]); + + $builder->add('subject', ChoiceType::class, [ + 'choices' => $subjectOptions, + 'group_by' => $subjectGroupBy, + ]); + + $builder->add('message', TextareaType::class, [ + 'label' => false, + 'attr' => [ + 'rows' => 5, + 'cols' => 85, + ], + ]); + } + + public function configureOptions(OptionsResolver $resolver): void + { + $resolver->setDefault('limit_to_team', null); + } +} diff --git a/webapp/src/Form/Type/TeamClarificationType.php b/webapp/src/Form/Type/TeamClarificationType.php index 0e7ae4d1a8..b21367a634 100644 --- a/webapp/src/Form/Type/TeamClarificationType.php +++ b/webapp/src/Form/Type/TeamClarificationType.php @@ -13,22 +13,23 @@ class TeamClarificationType extends AbstractType { - public function __construct(protected readonly DOMJudgeService $dj, protected readonly ConfigurationService $config) - { - } + public function __construct( + protected readonly DOMJudgeService $dj, + protected readonly ConfigurationService $config + ) {} public function buildForm(FormBuilderInterface $builder, array $options): void { $builder->add('recipient', TextType::class, [ - 'data' => 'Jury', - 'disabled' => true, + 'data' => 'Jury', + 'disabled' => true, ]); $subjects = []; /** @var string[] $categories */ $categories = $this->config->get('clar_categories'); - $user = $this->dj->getUser(); - $contest = $this->dj->getCurrentContest($user->getTeam()->getTeamid()); + $user = $this->dj->getUser(); + $contest = $this->dj->getCurrentContest($user->getTeam()->getTeamid()); if ($contest) { foreach ($categories as $categoryId => $categoryName) { $subjects[$categoryName] = sprintf('%d-%s', $contest->getCid(), $categoryId); @@ -37,8 +38,8 @@ public function buildForm(FormBuilderInterface $builder, array $options): void /** @var ContestProblem $problem */ foreach ($contest->getProblems() as $problem) { if ($problem->getAllowSubmit()) { - $problemName = sprintf('%s: %s', $problem->getShortname(), - $problem->getProblem()->getName()); + $problemName = sprintf('%s: %s', $problem->getShortname(), + $problem->getProblem()->getName()); $subjects[$problemName] = sprintf('%d-%d', $contest->getCid(), $problem->getProbid()); } } @@ -50,7 +51,6 @@ public function buildForm(FormBuilderInterface $builder, array $options): void ]); $builder->add('message', TextareaType::class, [ 'label' => false, - 'sanitize_html' => true, 'attr' => [ 'rows' => 5, 'cols' => 85, diff --git a/webapp/templates/jury/clarification.html.twig b/webapp/templates/jury/clarification.html.twig index a343545a1c..01b1ad80ab 100644 --- a/webapp/templates/jury/clarification.html.twig +++ b/webapp/templates/jury/clarification.html.twig @@ -61,7 +61,7 @@
- {% for qid,queue in clarform.queues %} + {% for qid,queue in queues %} {% endfor %} @@ -110,7 +110,7 @@
-
{{ clar.body | markdown_to_html }}
+
{{ clar.body | markdown_to_html | sanitize_html('app.clarification_sanitizer') }}
@@ -197,7 +197,7 @@ }); $('#clar_answers').change(clarificationAppendAnswer); - setupPreviewClarification($('#bodytext') , $('#messagepreview'), true); + setupPreviewClarification($('#jury_clarification_message') , $('#messagepreview'), true); }); {% endblock %} diff --git a/webapp/templates/jury/clarification_new.html.twig b/webapp/templates/jury/clarification_new.html.twig index 581c74ea5c..1682355d5d 100644 --- a/webapp/templates/jury/clarification_new.html.twig +++ b/webapp/templates/jury/clarification_new.html.twig @@ -18,7 +18,7 @@ {% block extrafooter %} {% endblock %} diff --git a/webapp/templates/jury/partials/clarification_form.html.twig b/webapp/templates/jury/partials/clarification_form.html.twig index 7184466bf3..34ae213ff1 100644 --- a/webapp/templates/jury/partials/clarification_form.html.twig +++ b/webapp/templates/jury/partials/clarification_form.html.twig @@ -1,74 +1,48 @@ - - -{% if origclar.clarid is defined %} - -{% endif %} - -
- - -
- -
- - -
- +{{ form_start(form) }} +{{ form_row(form.recipient) }} +{{ form_row(form.subject) }}
- +
- + {{ form_row(form.message) }}
- +
-
+
+ Start typing to see a preview of your message +
-
-
-
- -
-
+
+
+ +
+
-
-{% if clarform.answers is defined and clarform.answers|length > 0 %} -
- - -
-{% endif %} -
+
+ {% if answers is defined and answers|length > 0 %} +
+ + +
+ {% endif %} +
- -
+{{ form_end(form) }} diff --git a/webapp/templates/jury/team.html.twig b/webapp/templates/jury/team.html.twig index 063dc3c558..8c0916d13a 100644 --- a/webapp/templates/jury/team.html.twig +++ b/webapp/templates/jury/team.html.twig @@ -193,7 +193,7 @@ {{ button(path('jury_team_edit', {'teamId': team.teamid}), 'Edit', 'primary', 'edit') }} {{ button(path('jury_team_delete', {'teamId': team.teamid}), 'Delete', 'danger', 'trash-alt', true) }} {% endif %} - {{ button(path('jury_clarification_send', {'teamto': team.teamid}), 'Send message', 'secondary', 'envelope') }} + {{ button(path('jury_clarification_new', {'teamto': team.teamid}), 'Send message', 'secondary', 'envelope') }} {% include 'jury/partials/rejudge_form.html.twig' with {table: 'team', id: team.teamid, buttonClass: 'btn-secondary'} %} diff --git a/webapp/templates/team/partials/clarification.html.twig b/webapp/templates/team/partials/clarification.html.twig index acedc4cdcb..eda98ce293 100644 --- a/webapp/templates/team/partials/clarification.html.twig +++ b/webapp/templates/team/partials/clarification.html.twig @@ -40,7 +40,7 @@
- {{ clarification.body | markdown_to_html }} + {{ clarification.body | markdown_to_html | sanitize_html('app.clarification_sanitizer') }}
diff --git a/webapp/tests/Unit/Controller/Jury/ClarificationControllerTest.php b/webapp/tests/Unit/Controller/Jury/ClarificationControllerTest.php index e09121e267..a9e545f0df 100644 --- a/webapp/tests/Unit/Controller/Jury/ClarificationControllerTest.php +++ b/webapp/tests/Unit/Controller/Jury/ClarificationControllerTest.php @@ -101,14 +101,14 @@ public function testClarificationRequestComposeForm(): void self::assertEquals('Example teamname (t2)', $options[3]); $labels = $crawler->filter('label')->extract(['_text']); - self::assertEquals('Send to:', $labels[0]); - self::assertEquals('Subject:', $labels[1]); + self::assertEquals('Send to', $labels[0]); + self::assertEquals('Subject', $labels[1]); self::assertEquals('message', $labels[2]); $this->client->submitForm('Send', [ - 'sendto' => '', - 'problem' => '1-tech', - 'bodytext' => 'This is a clarification', + 'jury_clarification[recipient]' => '', + 'jury_clarification[subject]' => '1-tech', + 'jury_clarification[message]' => 'This is a clarification', ]); $this->client->followRedirect();