diff --git a/CHANGELOG.md b/CHANGELOG.md index 29ce446d..b371a81f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +- [#169](https://github.com/itk-dev/devops_itksites/pull/169) + Mark advisories as not affected per git repo, so they stop counting towards + a site's advisories, and sort sites by their open advisories - [#168](https://github.com/itk-dev/devops_itksites/pull/168) Rewrite the fixtures to cover all content with made-up data shaped like production diff --git a/assets/admin.js b/assets/admin.js index 7bd14d4e..5f1ac6c2 100644 --- a/assets/admin.js +++ b/assets/admin.js @@ -21,6 +21,16 @@ const tomSelectHandler = () => { window.addEventListener("DOMContentLoaded", tomSelectHandler); document.addEventListener("ea.collection.item-added", tomSelectHandler); +// Disable the buttons of a [data-submit-once] form once it submits, so a +// slow page load does not invite a second click. +document.addEventListener("submit", (event) => { + if (event.target.matches("[data-submit-once]")) { + event.target + .querySelectorAll("button") + .forEach((button) => (button.disabled = true)); + } +}); + // Copy the text of a [data-copy-text] button to the clipboard, and show a // check mark for a moment, like GitHub's copy buttons. document.addEventListener("click", async (event) => { diff --git a/fixtures/package.yaml b/fixtures/package.yaml index 0adca1db..8a589cf1 100644 --- a/fixtures/package.yaml +++ b/fixtures/package.yaml @@ -6,6 +6,7 @@ # - drupal/core has a drupal.org and a Packagist advisory for the same CVE. # - league/commonmark and drupal/webform have advisories without a CVE. # - guzzle has no advisories. +# - example-org/intranet has the commonmark advisory marked as not affected. App\Entity\Package: package/symfony-http-foundation: @@ -106,6 +107,15 @@ App\Entity\Advisory: - name: "Drupal" remoteId: "SA-CONTRIB-2099-001" +# Advisories a developer has marked as not affected, per git repo. +App\Entity\AdvisoryAssessment: + advisory_assessment/intranet-commonmark: + advisoryId: "PKSA-m3r8-5tyv-1h6p" + reason: "Markdown is only rendered from editor input" + assessedBy: "user@example.com" + __calls: + - setGitRepo: ["@git_repo/intranet"] + App\Entity\PackageVersion: package_version/symfony-http-foundation-6.4.10: package: "@package/symfony-http-foundation" diff --git a/migrations/Version20261021140107.php b/migrations/Version20261021140107.php new file mode 100644 index 00000000..93981a7e --- /dev/null +++ b/migrations/Version20261021140107.php @@ -0,0 +1,26 @@ +addSql('CREATE TABLE advisory_assessment (id BINARY(16) NOT NULL, created_at DATETIME NOT NULL, modified_at DATETIME NOT NULL, created_by VARCHAR(255) DEFAULT \'\' NOT NULL, modified_by VARCHAR(255) DEFAULT \'\' NOT NULL, repo_provider VARCHAR(255) NOT NULL, repo_organization VARCHAR(255) NOT NULL, repo_name VARCHAR(255) NOT NULL, advisory_id VARCHAR(255) NOT NULL, reason LONGTEXT NOT NULL, assessed_by VARCHAR(180) NOT NULL, UNIQUE INDEX repo_advisory (repo_provider, repo_organization, repo_name, advisory_id), PRIMARY KEY (id)) DEFAULT CHARACTER SET utf8mb4 COLLATE `utf8mb4_unicode_ci`'); + } + + public function down(Schema $schema): void + { + $this->addSql('DROP TABLE advisory_assessment'); + } +} diff --git a/src/Admin/AdvisoryCountSort.php b/src/Admin/AdvisoryCountSort.php index 5812d5e5..b4692443 100644 --- a/src/Admin/AdvisoryCountSort.php +++ b/src/Admin/AdvisoryCountSort.php @@ -5,6 +5,8 @@ namespace App\Admin; use App\Entity\Advisory; +use App\Entity\AdvisoryAssessment; +use App\Entity\GitRepo; use Doctrine\ORM\QueryBuilder; use EasyCorp\Bundle\EasyAdminBundle\Config\Option\SortOrder; use EasyCorp\Bundle\EasyAdminBundle\Dto\SearchDto; @@ -15,32 +17,66 @@ final class AdvisoryCountSort /** * Order the index by Site::getAdvisoryCount(), which is computed in PHP and - * so has no column to sort on. EasyAdmin drops the sort before it reaches - * the query — isValidCustomSort() only accepts mapped fields and - * associations — so read it off the SearchDto and order by a scalar sub - * query instead. Intended for use from a CRUD controller's - * createIndexQueryBuilder() after the parent has built the QB. + * so has no column to sort on. EasyAdmin drops the sort from the URL before + * it reaches the query — isValidCustomSort() only accepts mapped fields and + * associations — and orders by a non-existent entity.advisoryCount column + * when it is the default sort. So read it off the SearchDto, drop that + * column, and order by a scalar sub query instead. Intended for use from a + * CRUD controller's createIndexQueryBuilder() after the parent has built + * the QB. */ public static function apply(QueryBuilder $qb, SearchDto $searchDto): void { + $rootAlias = (string) current($qb->getRootAliases()); + self::removeColumnOrder($qb, $rootAlias); + // The property name is ours, but the direction comes from the URL and // reaches DQL as a string, so it is whitelisted rather than trusted. - $direction = strtoupper((string) ($searchDto->getCustomSort()[self::PROPERTY] ?? '')); + $direction = strtoupper((string) ($searchDto->getSort()[self::PROPERTY] ?? '')); if (SortOrder::ASC !== $direction && SortOrder::DESC !== $direction) { return; } - $rootAlias = (string) current($qb->getRootAliases()); + $qb->addSelect(sprintf('(%s) AS HIDDEN %s', self::subQuery($rootAlias), self::PROPERTY)); + $qb->addOrderBy(self::PROPERTY, SortOrder::DESC === $direction ? \SortDirection::Descending : \SortDirection::Ascending); + } - // An advisory belongs to a single package and an installation holds a - // single version of it, so the advisory cannot be counted twice. - $qb->addSelect(sprintf( - '(SELECT COUNT(adv.id) FROM %s adv JOIN adv.packageVersions advPackageVersion JOIN advPackageVersion.installations advInstallation WHERE advInstallation.id = IDENTITY(%s.installation)) AS HIDDEN %s', + /** + * DQL counting the open advisories of the site aliased $siteAlias. + * + * An advisory belongs to a single package and an installation holds a + * single version of it, so the advisory cannot be counted twice. + * Advisories marked as not affected for the installation's git repo do + * not count, as in AdvisoryAssessor::openCount(). + */ + public static function subQuery(string $siteAlias): string + { + return sprintf( + 'SELECT COUNT(adv.id) FROM %1$s adv JOIN adv.packageVersions advPackageVersion JOIN advPackageVersion.installations advInstallation WHERE advInstallation.id = IDENTITY(%2$s.installation) AND NOT EXISTS (SELECT advAssessment.id FROM %3$s advAssessment JOIN %4$s advRepo WITH advRepo.provider = advAssessment.repoProvider AND advRepo.organization = advAssessment.repoOrganization AND advRepo.repo = advAssessment.repoName JOIN advRepo.gitTags advTag JOIN advTag.installations advTagInstallation WHERE advTagInstallation.id = advInstallation.id AND advAssessment.advisoryId = adv.advisoryId)', Advisory::class, - $rootAlias, - self::PROPERTY - )); + $siteAlias, + AdvisoryAssessment::class, + GitRepo::class, + ); + } - $qb->addOrderBy(self::PROPERTY, SortOrder::DESC === $direction ? \SortDirection::Descending : \SortDirection::Ascending); + private static function removeColumnOrder(QueryBuilder $qb, string $rootAlias): void + { + $column = $rootAlias.'.'.self::PROPERTY.' '; + $parts = []; + foreach ($qb->getDQLPart('orderBy') as $orderBy) { + foreach ($orderBy->getParts() as $part) { + if (!str_starts_with($part, $column)) { + $parts[] = $part; + } + } + } + + // A part reads " ", and addOrderBy() wants them apart. + $qb->resetDQLPart('orderBy'); + foreach ($parts as $part) { + $at = (int) strrpos($part, ' '); + $qb->addOrderBy(substr($part, 0, $at), substr($part, $at + 1)); + } } } diff --git a/src/Controller/Admin/AdvisoryAssessmentController.php b/src/Controller/Admin/AdvisoryAssessmentController.php new file mode 100644 index 00000000..01f2ca77 --- /dev/null +++ b/src/Controller/Admin/AdvisoryAssessmentController.php @@ -0,0 +1,83 @@ +getId(); + } + + #[AdminRoute(path: '/site/{id}/advisories/not-affected', name: 'site_advisories_not_affected', options: ['methods' => ['POST']])] + public function markNotAffected(#[MapEntity(id: 'id')] Site $site, Request $request): RedirectResponse + { + $this->denyUnlessTokenIsValid($site, $request); + + try { + $marked = $this->assessor->markNotAffected( + $site, + array_values(array_map(strval(...), $request->request->all('advisoryIds'))), + $request->request->getString('reason'), + (string) $this->getUser()?->getUserIdentifier(), + ); + if (0 === $marked) { + $this->addFlash('info', 'These advisories are already marked as not affected.'); + } else { + $this->addFlash('success', sprintf( + 'Marked %d %s as not affected for all checkouts of %s.', + $marked, + 1 === $marked ? 'advisory' : 'advisories', + $this->assessor->gitRepo($site), + )); + } + } catch (\InvalidArgumentException $exception) { + $this->addFlash('danger', $exception->getMessage()); + } + + return $this->redirectToSite($site); + } + + #[AdminRoute(path: '/site/{id}/advisories/remove-mark', name: 'site_advisories_remove_mark', options: ['methods' => ['POST']])] + public function removeMark(#[MapEntity(id: 'id')] Site $site, Request $request): RedirectResponse + { + $this->denyUnlessTokenIsValid($site, $request); + + $advisoryId = $request->request->getString('advisoryId'); + $this->assessor->removeMark($site, $advisoryId); + $this->addFlash('success', sprintf('Removed the mark on %s, so it counts again.', $advisoryId)); + + return $this->redirectToSite($site); + } + + private function denyUnlessTokenIsValid(Site $site, Request $request): void + { + if (!$this->isCsrfTokenValid(self::csrfIntent($site), $request->request->getString('_token'))) { + throw $this->createAccessDeniedException('Invalid CSRF token.'); + } + } + + private function redirectToSite(Site $site): RedirectResponse + { + return $this->redirectToRoute('admin_site_detail', ['entityId' => $site->getId()]); + } +} diff --git a/src/Controller/Admin/DashboardController.php b/src/Controller/Admin/DashboardController.php index 2a107170..2b172c90 100644 --- a/src/Controller/Admin/DashboardController.php +++ b/src/Controller/Admin/DashboardController.php @@ -5,6 +5,7 @@ namespace App\Controller\Admin; use App\Repository\AdvisoryRepository; +use App\Repository\SiteRepository; use EasyCorp\Bundle\EasyAdminBundle\Attribute\AdminDashboard; use EasyCorp\Bundle\EasyAdminBundle\Config\Assets; use EasyCorp\Bundle\EasyAdminBundle\Config\Crud; @@ -22,6 +23,7 @@ class DashboardController extends AbstractDashboardController public function __construct( private readonly AdminUrlGenerator $adminUrlGenerator, private readonly AdvisoryRepository $advisoryRepository, + private readonly SiteRepository $siteRepository, ) { } @@ -57,7 +59,7 @@ public function configureMenuItems(): iterable yield MenuItem::linkToDashboard('Dashboard', 'fa fa-home'); yield MenuItem::linkTo(ServerCrudController::class, 'Servers', 'fas fa-server'); yield MenuItem::linkTo(InstallationCrudController::class, 'Installations', 'fas fa-folder'); - yield MenuItem::linkTo(SiteCrudController::class, 'Sites', 'fas fa-bookmark'); + yield MenuItem::linkTo(SiteCrudController::class, 'Sites', 'fas fa-bookmark')->setBadge($this->siteRepository->countWithOpenAdvisories() ?: null, 'danger'); yield MenuItem::linkTo(DomainCrudController::class, 'Domains', 'fas fa-link'); // OIDC and Service certificates are deprecated. The controllers and // entities are kept so existing rows stay reachable by URL, but the diff --git a/src/Controller/Admin/SiteCrudController.php b/src/Controller/Admin/SiteCrudController.php index 5c333cb4..79c8165e 100644 --- a/src/Controller/Admin/SiteCrudController.php +++ b/src/Controller/Admin/SiteCrudController.php @@ -16,6 +16,7 @@ use App\Form\Type\Admin\HostingProviderFilter; use App\Form\Type\Admin\SemverFilter; use App\Form\Type\Admin\ServerTypeFilter; +use App\Service\AdvisoryAssessor; use App\Trait\ExportCrudControllerTrait; use App\Trait\SemverSortableCrudControllerTrait; use Doctrine\ORM\QueryBuilder; @@ -43,8 +44,9 @@ class SiteCrudController extends AbstractCrudController createIndexQueryBuilder as private semverIndexQueryBuilder; } - public function __construct() - { + public function __construct( + private readonly AdvisoryAssessor $assessor, + ) { } public static function getEntityFqcn(): string @@ -55,7 +57,9 @@ public static function getEntityFqcn(): string #[\Override] public function configureCrud(Crud $crud): Crud { - return $crud->showEntityActionsInlined(); + return $crud + ->showEntityActionsInlined() + ->setDefaultSort([AdvisoryCountSort::PROPERTY => 'DESC']); } #[\Override] @@ -71,7 +75,13 @@ public function configureActions(Actions $actions): Actions public function configureFields(string $pageName): iterable { yield DomainField::new('primaryDomain')->setColumns(12); - yield AdvisoryCountField::new('advisoryCount')->setLabel('Adv.')->setSortable(true); + // Advisories marked as not affected do not count. The detail page + // lists them all, with their marks. + yield AdvisoryCountField::new('advisoryCount')->setLabel('Adv.')->setSortable(true) + ->setTemplatePath('EasyAdminBundle/Fields/site_advisories.html.twig') + ->formatValue(Crud::PAGE_DETAIL === $pageName + ? fn (mixed $value, Site $site): array => ['gitRepo' => $this->assessor->gitRepo($site), 'advisories' => $this->assessor->advisories($site)] + : fn (mixed $value, Site $site): int => $this->assessor->openCount($site)); yield AssociationField::new('domains')->hideOnIndex(); yield SiteTypeField::new('type')->setLabel('Stack'); yield ConfigFilePathField::new('configFilePath')->setColumns(12)->hideOnIndex(); diff --git a/src/Entity/AdvisoryAssessment.php b/src/Entity/AdvisoryAssessment.php new file mode 100644 index 00000000..8ce0be40 --- /dev/null +++ b/src/Entity/AdvisoryAssessment.php @@ -0,0 +1,106 @@ +repoOrganization.'/'.$this->repoName.': '.$this->advisoryId; + } + + public function setGitRepo(GitRepo $gitRepo): self + { + $this->repoProvider = (string) $gitRepo->getProvider(); + $this->repoOrganization = (string) $gitRepo->getOrganization(); + $this->repoName = (string) $gitRepo->getRepo(); + + return $this; + } + + public function getRepoProvider(): string + { + return $this->repoProvider; + } + + public function getRepoOrganization(): string + { + return $this->repoOrganization; + } + + public function getRepoName(): string + { + return $this->repoName; + } + + public function getAdvisoryId(): string + { + return $this->advisoryId; + } + + public function setAdvisoryId(string $advisoryId): self + { + $this->advisoryId = $advisoryId; + + return $this; + } + + public function getReason(): string + { + return $this->reason; + } + + public function setReason(string $reason): self + { + $this->reason = $reason; + + return $this; + } + + public function getAssessedBy(): string + { + return $this->assessedBy; + } + + public function setAssessedBy(string $assessedBy): self + { + $this->assessedBy = $assessedBy; + + return $this; + } +} diff --git a/src/Repository/AdvisoryAssessmentRepository.php b/src/Repository/AdvisoryAssessmentRepository.php new file mode 100644 index 00000000..76cf0804 --- /dev/null +++ b/src/Repository/AdvisoryAssessmentRepository.php @@ -0,0 +1,45 @@ + + * + * @method AdvisoryAssessment|null find($id, $lockMode = null, $lockVersion = null) + * @method AdvisoryAssessment|null findOneBy(array $criteria, array $orderBy = null) + * @method AdvisoryAssessment[] findAll() + * @method AdvisoryAssessment[] findBy(array $criteria, array $orderBy = null, $limit = null, $offset = null) + */ +class AdvisoryAssessmentRepository extends ServiceEntityRepository +{ + public function __construct(ManagerRegistry $registry) + { + parent::__construct($registry, AdvisoryAssessment::class); + } + + /** + * The repo's assessments, keyed by advisory id. + * + * @return array + */ + public function findByGitRepo(GitRepo $gitRepo): array + { + $assessments = []; + foreach ($this->findBy([ + 'repoProvider' => $gitRepo->getProvider(), + 'repoOrganization' => $gitRepo->getOrganization(), + 'repoName' => $gitRepo->getRepo(), + ]) as $assessment) { + $assessments[$assessment->getAdvisoryId()] = $assessment; + } + + return $assessments; + } +} diff --git a/src/Repository/GitRepoRepository.php b/src/Repository/GitRepoRepository.php index d1dae337..eba1bb9e 100644 --- a/src/Repository/GitRepoRepository.php +++ b/src/Repository/GitRepoRepository.php @@ -4,8 +4,10 @@ namespace App\Repository; +use App\Entity\AdvisoryAssessment; use App\Entity\GitRepo; use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository; +use Doctrine\ORM\Query\Expr\Join; use Doctrine\Persistence\ManagerRegistry; use Symfony\Component\Uid\Ulid; @@ -25,7 +27,8 @@ public function __construct(ManagerRegistry $registry) } /** - * Repos reachable from any package-version advisory, with their advisory count. + * Repos reachable from any package-version advisory, with their count of + * advisories not marked as not affected for the repo. * * Path: GitRepo → GitTag → Installation → PackageVersion → Advisory. * @@ -40,6 +43,8 @@ public function findReposWithAdvisoryCount(): array ->innerJoin('gt.installations', 'i') ->innerJoin('i.packageVersions', 'pv') ->innerJoin('pv.advisories', 'a') + ->leftJoin(AdvisoryAssessment::class, 'aa', Join::WITH, 'aa.advisoryId = a.advisoryId AND aa.repoProvider = r.provider AND aa.repoOrganization = r.organization AND aa.repoName = r.repo') + ->andWhere('aa.id IS NULL') ->groupBy('r.id') ->having('COUNT(DISTINCT a.id) > 0') ->orderBy('r.organization', \SortDirection::Ascending) diff --git a/src/Repository/SiteRepository.php b/src/Repository/SiteRepository.php index f8ed961e..725f0865 100644 --- a/src/Repository/SiteRepository.php +++ b/src/Repository/SiteRepository.php @@ -4,6 +4,7 @@ namespace App\Repository; +use App\Admin\AdvisoryCountSort; use App\Entity\Site; use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository; use Doctrine\ORM\AbstractQuery; @@ -36,4 +37,16 @@ public function getPrimaryDomains(): array ->getQuery() ->getResult(AbstractQuery::HYDRATE_SCALAR_COLUMN); } + + /** + * The number of sites with advisories not marked as not affected. + */ + public function countWithOpenAdvisories(): int + { + return (int) $this->createQueryBuilder('s') + ->select('COUNT(s.id)') + ->where(sprintf('(%s) > 0', AdvisoryCountSort::subQuery('s'))) + ->getQuery() + ->getSingleScalarResult(); + } } diff --git a/src/Service/AdvisoryAssessor.php b/src/Service/AdvisoryAssessor.php new file mode 100644 index 00000000..e9943b65 --- /dev/null +++ b/src/Service/AdvisoryAssessor.php @@ -0,0 +1,99 @@ +getInstallation()->getGitTag()?->getRepo(); + } + + /** + * The site's advisories, each with its assessment if it is marked. + * + * @return list + */ + public function advisories(Site $site): array + { + $gitRepo = $this->gitRepo($site); + $assessments = null === $gitRepo ? [] : $this->repository->findByGitRepo($gitRepo); + + return array_values(array_map( + static fn (Advisory $advisory): array => ['advisory' => $advisory, 'assessment' => $assessments[$advisory->getAdvisoryId()] ?? null], + $site->getAdvisories()->toArray(), + )); + } + + /** + * Advisories that are not marked as not affected. + */ + public function openCount(Site $site): int + { + return count(array_filter($this->advisories($site), static fn (array $row): bool => null === $row['assessment'])); + } + + /** + * Mark those of the advisories that are open on the site. + * + * @param list $advisoryIds + * + * @return int the number of advisories marked + * + * @throws \InvalidArgumentException when the site has no git repo or the reason is empty + */ + public function markNotAffected(Site $site, array $advisoryIds, string $reason, string $assessedBy): int + { + $gitRepo = $this->gitRepo($site) ?? throw new \InvalidArgumentException('No git repo linked, so advisories cannot be marked as not affected.'); + $reason = trim($reason); + if ('' === $reason) { + throw new \InvalidArgumentException('A reason is required.'); + } + + $marked = 0; + foreach ($this->advisories($site) as ['advisory' => $advisory, 'assessment' => $assessment]) { + if (null !== $assessment || !in_array($advisory->getAdvisoryId(), $advisoryIds, true)) { + continue; + } + $this->entityManager->persist(new AdvisoryAssessment() + ->setGitRepo($gitRepo) + ->setAdvisoryId($advisory->getAdvisoryId()) + ->setReason($reason) + ->setAssessedBy($assessedBy)); + ++$marked; + } + $this->entityManager->flush(); + + return $marked; + } + + public function removeMark(Site $site, string $advisoryId): void + { + $gitRepo = $this->gitRepo($site); + $assessment = null === $gitRepo ? null : ($this->repository->findByGitRepo($gitRepo)[$advisoryId] ?? null); + if (null !== $assessment) { + $this->entityManager->remove($assessment); + $this->entityManager->flush(); + } + } +} diff --git a/templates/EasyAdminBundle/Fields/_advisory_reason_modal.html.twig b/templates/EasyAdminBundle/Fields/_advisory_reason_modal.html.twig new file mode 100644 index 00000000..58d7b101 --- /dev/null +++ b/templates/EasyAdminBundle/Fields/_advisory_reason_modal.html.twig @@ -0,0 +1,16 @@ +{# A button opening a modal that asks for the reason and submits the enclosing form. #} +{{ label|default('Mark') }} +{# text-wrap: the modal sits in a text-nowrap table cell and would inherit it. #} + + + {# Blocks do not see the include's variables, so the label wraps the field instead of naming its id. #} + + + + Cancel + Mark as not affected + + diff --git a/templates/EasyAdminBundle/Fields/site_advisories.html.twig b/templates/EasyAdminBundle/Fields/site_advisories.html.twig new file mode 100644 index 00000000..d6b8cdb7 --- /dev/null +++ b/templates/EasyAdminBundle/Fields/site_advisories.html.twig @@ -0,0 +1,80 @@ +{# @var ea \EasyCorp\Bundle\EasyAdminBundle\Context\AdminContext #} +{# @var field \EasyCorp\Bundle\EasyAdminBundle\Dto\FieldDto #} +{# @var entity \EasyCorp\Bundle\EasyAdminBundle\Dto\EntityDto #} +{% if ea().crud.currentAction == 'detail' %} + {% set git_repo = field.formattedValue.gitRepo %} + {% set advisories = field.formattedValue.advisories %} + {% set open = advisories|filter(row => row.assessment is null) %} + {% set site_id = entity.instance.id %} + {% set token = csrf_token('site_advisories_' ~ site_id) %} +
+ {% if git_repo %} +

Marks apply to all checkouts of {{ git_repo }}.

+ {% else %} +

No git repo linked Advisories cannot be marked as not affected.

+ {% endif %} + {% if advisories|length > 0 %} + + + + + + + + + + + {% for row in advisories %} + {% set advisory = row.advisory %} + {% set assessment = row.assessment %} + {% set url = ea_url() + .unsetAll() + .setController('App\\Controller\\Admin\\AdvisoryCrudController') + .setAction('detail') + .setEntityId(advisory.id) %} + {# Fades the cells, not the row, so the Unmark button stays at full strength. #} + {% set faded = assessment ? 'text-body-secondary opacity-50' %} + + + + + + + {% endfor %} + +
PackageCVETitleAction
{{ advisory.package }}{{ advisory.cve ?? advisory.advisoryId }} + {{ advisory.title }} + {% if assessment %} +
Not affected{{ assessment.reason }} ({{ assessment.assessedBy }}, {{ assessment.createdAt|date('Y-m-d') }})
+ {% endif %} +
+ {% if assessment %} + {% if git_repo %} +
+ + + Unmark +
+ {% endif %} + {% elseif git_repo %} +
+ + + {{ include('EasyAdminBundle/Fields/_advisory_reason_modal.html.twig', {id: 'advisory-mark-' ~ loop.index, title: 'Mark ' ~ (advisory.cve ?? advisory.advisoryId) ~ ' as not affected?'}) }} +
+ {% endif %} +
+ {% endif %} + {% if git_repo and open|length > 0 %} +
+ + {% for row in open %} + + {% endfor %} + {{ include('EasyAdminBundle/Fields/_advisory_reason_modal.html.twig', {id: 'advisory-accept-all', title: 'Mark all ' ~ open|length ~ ' open advisories as not affected?', label: 'Mark all'}) }} +
+ {% endif %} +
+{% elseif field.formattedValue != 0 %} + {{ field.formattedValue }} +{% endif %} diff --git a/tests/Admin/AdvisoryCountSortTest.php b/tests/Admin/AdvisoryCountSortTest.php index 3753e8d9..5eaa319d 100644 --- a/tests/Admin/AdvisoryCountSortTest.php +++ b/tests/Admin/AdvisoryCountSortTest.php @@ -28,6 +28,33 @@ public function testItOrdersByTheCountSubQuery(): void $this->assertNotEmpty($qb->getQuery()->getSQL()); } + /** + * As the default sort, EasyAdmin orders by the column, which does not + * exist, so the sub query has to replace it. + */ + public function testItReplacesTheColumnOrderOfTheDefaultSort(): void + { + $qb = $this->queryBuilder() + ->addOrderBy('entity.primaryDomain', 'ASC') + ->addOrderBy('entity.advisoryCount', 'DESC'); + + AdvisoryCountSort::apply($qb, $this->searchDto([], ['advisoryCount' => 'DESC'])); + + $dql = $qb->getQuery()->getDQL(); + $this->assertStringContainsString('ORDER BY entity.primaryDomain ASC, advisoryCount DESC', $dql); + $this->assertStringNotContainsString('entity.advisoryCount', $dql); + $this->assertNotEmpty($qb->getQuery()->getSQL()); + } + + public function testTheUrlSortWinsOverTheDefaultSort(): void + { + $qb = $this->queryBuilder()->addOrderBy('entity.advisoryCount', 'DESC'); + + AdvisoryCountSort::apply($qb, $this->searchDto(['advisoryCount' => 'ASC'], ['advisoryCount' => 'DESC'])); + + $this->assertStringContainsString('ORDER BY advisoryCount ASC', $qb->getQuery()->getDQL()); + } + /** * The direction reaches DQL as a string, so anything but ASC or DESC is * ignored rather than passed through. @@ -58,8 +85,8 @@ private function queryBuilder(): QueryBuilder ->createQueryBuilder('entity'); } - private function searchDto(array $customSort): SearchDto + private function searchDto(array $customSort, array $defaultSort = []): SearchDto { - return new SearchDto(new Request(), null, null, [], $customSort, null); + return new SearchDto(new Request(), null, null, $defaultSort, $customSort, null); } } diff --git a/tests/Controller/Admin/AdvisoryAssessmentControllerTest.php b/tests/Controller/Admin/AdvisoryAssessmentControllerTest.php new file mode 100644 index 00000000..b32c9426 --- /dev/null +++ b/tests/Controller/Admin/AdvisoryAssessmentControllerTest.php @@ -0,0 +1,255 @@ +loggedInClient(); + + $crawler = $client->request('GET', $this->detailUrl('shop.example.com')); + + $this->assertResponseIsSuccessful(); + $this->assertSelectorTextContains('.advisory-assessments', 'Marks apply to all checkouts of example-org/shop'); + $this->assertCount(2, $crawler->filter('form.advisory-mark'), 'One per open advisory'); + $this->assertSelectorExists('form.advisory-accept-all'); + } + + public function testMarkingAnAdvisoryAsNotAffected(): void + { + $client = $this->loggedInClient(); + $crawler = $client->request('GET', $this->detailUrl('shop.example.com')); + + $client->submit($this->markForm($crawler, self::TWIG), ['reason' => 'Templates are not user editable']); + + $this->assertResponseRedirects($this->detailUrl('shop.example.com')); + $crawler = $client->followRedirect(); + $this->assertSelectorTextContains('#flash-messages', 'Marked 1 advisory as not affected for all checkouts of example-org/shop'); + $row = $this->row($crawler, self::TWIG); + $this->assertStringContainsString('Not affected', $row->text()); + $this->assertStringContainsString('Templates are not user editable', $row->text()); + $this->assertStringContainsString('admin@example.com', $row->text()); + $this->assertCount(1, $row->filter('form.advisory-remove')); + + $assessment = $this->entityManager()->getRepository(AdvisoryAssessment::class)->findOneBy(['advisoryId' => self::TWIG]); + $this->assertSame('admin@example.com', $assessment?->getAssessedBy()); + } + + public function testResubmittingAMarkSaysItIsAlreadyMarked(): void + { + $client = $this->loggedInClient(); + $crawler = $client->request('GET', $this->detailUrl('shop.example.com')); + $form = $this->markForm($crawler, self::TWIG); + $client->submit($form, ['reason' => 'Reviewed']); + + $client->submit($form, ['reason' => 'Reviewed']); + + $client->followRedirect(); + $this->assertSelectorTextContains('#flash-messages', 'already marked as not affected'); + $this->assertSelectorTextNotContains('#flash-messages', 'Marked 0'); + } + + public function testAcceptingAllCurrentAdvisories(): void + { + $client = $this->loggedInClient(); + $crawler = $client->request('GET', $this->detailUrl('shop.example.com')); + + $client->submit($crawler->filter('form.advisory-accept-all')->form(), ['reason' => 'Reviewed with the client']); + + $crawler = $client->followRedirect(); + $this->assertSelectorTextContains('#flash-messages', 'Marked 2 advisories as not affected'); + $this->assertCount(0, $crawler->filter('form.advisory-mark')); + $this->assertCount(0, $crawler->filter('form.advisory-accept-all'), 'Nothing left to accept'); + } + + public function testAReasonIsRequired(): void + { + $client = $this->loggedInClient(); + $crawler = $client->request('GET', $this->detailUrl('shop.example.com')); + + $client->submit($this->markForm($crawler, self::TWIG), ['reason' => ' ']); + + $client->followRedirect(); + $this->assertSelectorTextContains('#flash-messages', 'A reason is required'); + $this->assertSame(0, $this->entityManager()->getRepository(AdvisoryAssessment::class)->count(['advisoryId' => self::TWIG])); + } + + public function testRemovingAMark(): void + { + $client = $this->loggedInClient(); + $crawler = $client->request('GET', $this->detailUrl('intranet.example.com')); + + $client->submit($this->row($crawler, 'PKSA-m3r8-5tyv-1h6p')->filter('form.advisory-remove')->form()); + + $crawler = $client->followRedirect(); + $this->assertSelectorTextContains('#flash-messages', 'Removed the mark'); + $this->assertCount(1, $this->row($crawler, 'PKSA-m3r8-5tyv-1h6p')->filter('form.advisory-mark')); + } + + public function testAnInvalidTokenIsRejected(): void + { + $client = $this->loggedInClient(); + $crawler = $client->request('GET', $this->detailUrl('shop.example.com')); + $form = $this->markForm($crawler, self::TWIG); + + $client->request('POST', $form->getUri(), ['advisoryIds' => [self::TWIG], 'reason' => 'Reviewed', '_token' => 'invalid']); + + $this->assertResponseStatusCodeSame(403); + } + + public function testASiteWithoutAGitRepoExplainsWhyItCannotBeMarked(): void + { + $client = $this->loggedInClient(); + + $crawler = $client->request('GET', $this->detailUrl('release.example.com')); + + $this->assertResponseIsSuccessful(); + $this->assertSelectorTextContains('.advisory-assessments', 'No git repo linked'); + $this->assertCount(0, $crawler->filter('form.advisory-mark, form.advisory-accept-all')); + $this->assertGreaterThan(0, $crawler->filter('.advisory-assessments tbody tr')->count(), 'The advisories are still listed'); + } + + public function testTheIndexCountsOnlyOpenAdvisories(): void + { + $client = $this->loggedInClient(); + $crawler = $client->request('GET', $this->detailUrl('intranet.example.com')); + $this->assertSame('3', $this->indexBadge($client, 'intranet.example.com'), 'Four advisories, one marked in the fixtures'); + + // With all four marked, intranet has the most advisories but none open. + $client->submit($crawler->filter('form.advisory-accept-all')->form(), ['reason' => 'Reviewed']); + + $this->assertSame('', $this->indexBadge($client, 'intranet.example.com')); + $url = static::getContainer()->get(AdminUrlGenerator::class) + ->setController(SiteCrudController::class) + ->setAction(Crud::PAGE_INDEX) + ->set('sort', ['advisoryCount' => 'DESC']) + ->generateUrl(); + + $crawler = $client->request('GET', $url); + + $this->assertResponseIsSuccessful(); + $counts = $crawler->filter('tbody tr[data-id]')->each(static fn (Crawler $row): int => (int) ($row->filter(self::BADGE)->count() > 0 ? $row->filter(self::BADGE)->text() : 0)); + $sorted = $counts; + rsort($sorted); + $this->assertSame($sorted, $counts, 'Sorted by the open count'); + } + + public function testTheIndexSortsByTheOpenCountByDefault(): void + { + $client = $this->loggedInClient(); + $url = static::getContainer()->get(AdminUrlGenerator::class) + ->setController(SiteCrudController::class) + ->setAction(Crud::PAGE_INDEX) + ->generateUrl(); + + $crawler = $client->request('GET', $url); + + $this->assertResponseIsSuccessful(); + $counts = $crawler->filter('tbody tr[data-id]')->each(static fn (Crawler $row): int => (int) ($row->filter(self::BADGE)->count() > 0 ? $row->filter(self::BADGE)->text() : 0)); + $sorted = $counts; + rsort($sorted); + $this->assertSame($sorted, $counts); + $this->assertGreaterThan(0, $counts[0], 'A site with open advisories comes first'); + } + + public function testTheSitesMenuItemCountsSitesWithOpenAdvisories(): void + { + $client = $this->loggedInClient(); + $url = static::getContainer()->get(AdminUrlGenerator::class) + ->setController(SiteCrudController::class) + ->setAction(Crud::PAGE_INDEX) + ->generateUrl(); + $crawler = $client->request('GET', $url); + $withOpen = $crawler->filter('tbody tr[data-id]')->reduce(static fn (Crawler $row): bool => $row->filter(self::BADGE)->count() > 0)->count(); + + $badge = $crawler->filter('a.ea-sidebar-item-link') + ->reduce(static fn (Crawler $link): bool => 'Sites' === $link->filter('.ea-sidebar-item-label')->text()) + ->filter('.ea-sidebar-badge.badge-danger'); + $this->assertSame((string) $withOpen, $badge->text()); + } + + public function testTheRepoAdvisoriesPageCountsOnlyOpenAdvisories(): void + { + $client = $this->loggedInClient(); + + $crawler = $client->request('GET', static::getContainer()->get('router')->generate('admin_repo_advisories')); + + $this->assertResponseIsSuccessful(); + $row = $crawler->filter('tr')->reduce(static fn (Crawler $row): bool => str_contains($row->text(), 'example-org/intranet')); + $this->assertSame('3', $row->filter('.badge-danger')->text()); + } + + private function loggedInClient(): KernelBrowser + { + $client = static::createClient(); + // Keep the rows written by one request for the next. + $client->disableReboot(); + $client->loginUser($this->entityManager()->getRepository(User::class)->findOneBy(['email' => 'admin@example.com'])); + + return $client; + } + + private function entityManager(): EntityManagerInterface + { + return static::getContainer()->get(EntityManagerInterface::class); + } + + private function detailUrl(string $primaryDomain): string + { + $site = $this->entityManager()->getRepository(Site::class)->findOneBy(['primaryDomain' => $primaryDomain]); + + return static::getContainer()->get(AdminUrlGenerator::class) + ->setController(SiteCrudController::class) + ->setAction(Crud::PAGE_DETAIL) + ->setEntityId($site?->getId()) + ->generateUrl(); + } + + private function row(Crawler $crawler, string $advisoryId): Crawler + { + return $crawler->filter('.advisory-assessments tbody tr[data-advisory-id="'.$advisoryId.'"]'); + } + + private function markForm(Crawler $crawler, string $advisoryId): Form + { + return $this->row($crawler, $advisoryId)->filter('form.advisory-mark')->form(); + } + + /** + * The site's advisory badge on the index page, or '' without one. + */ + private function indexBadge(KernelBrowser $client, string $primaryDomain): string + { + $url = static::getContainer()->get(AdminUrlGenerator::class) + ->setController(SiteCrudController::class) + ->setAction(Crud::PAGE_INDEX) + ->generateUrl(); + $badge = $client->request('GET', $url)->filter('tbody tr[data-id]') + ->reduce(static fn (Crawler $row): bool => str_contains($row->text(), $primaryDomain)) + ->filter(self::BADGE); + + return $badge->count() > 0 ? $badge->text() : ''; + } +} diff --git a/tests/DataFixtures/FixturesTest.php b/tests/DataFixtures/FixturesTest.php index 99b7f635..b2991513 100644 --- a/tests/DataFixtures/FixturesTest.php +++ b/tests/DataFixtures/FixturesTest.php @@ -5,6 +5,7 @@ namespace App\Tests\DataFixtures; use App\Entity\Advisory; +use App\Entity\AdvisoryAssessment; use App\Entity\CodeOwner; use App\Entity\DetectionResult; use App\Entity\DockerImage; @@ -199,6 +200,7 @@ public function testFixturesUseExampleDataOnly(): void ...array_map(static fn (User $user): string => (string) $user->getEmail(), $this->entityManager->getRepository(User::class)->findAll()), ...array_map(static fn (CodeOwner $owner): string => $owner->getEmail(), $this->entityManager->getRepository(CodeOwner::class)->findAll()), ...array_filter(array_map(static fn (SecurityContract $contract): ?string => $contract->getClientContactEmail(), $this->entityManager->getRepository(SecurityContract::class)->findAll())), + ...array_map(static fn (AdvisoryAssessment $assessment): string => $assessment->getAssessedBy(), $this->entityManager->getRepository(AdvisoryAssessment::class)->findAll()), ]; foreach ($emails as $email) { $this->assertMatchesRegularExpression('/@example\.com$/', $email); diff --git a/tests/Service/AdvisoryAssessorTest.php b/tests/Service/AdvisoryAssessorTest.php new file mode 100644 index 00000000..6aad741d --- /dev/null +++ b/tests/Service/AdvisoryAssessorTest.php @@ -0,0 +1,113 @@ +entityManager = self::getContainer()->get(EntityManagerInterface::class); + $this->assessor = self::getContainer()->get(AdvisoryAssessor::class); + } + + public function testAMarkAppliesToEveryCheckoutOfTheRepo(): void + { + $this->assertSame(2, $this->assessor->openCount($this->site('shop.example.com'))); + $this->assertSame(1, $this->assessor->openCount($this->site('stg.shop.example.com'))); + + $this->assessor->markNotAffected($this->site('shop.example.com'), [self::TWIG], 'Templates are not user editable', 'user@example.com'); + + // A new advisory, here http-foundation, still counts. + $this->assertSame(1, $this->assessor->openCount($this->site('shop.example.com'))); + $this->assertSame(0, $this->assessor->openCount($this->site('stg.shop.example.com'))); + } + + public function testOnlyTheSitesOpenAdvisoriesAreMarked(): void + { + $marked = $this->assessor->markNotAffected($this->site('stg.shop.example.com'), [self::TWIG, self::HTTP_FOUNDATION], 'Reviewed', 'user@example.com'); + + $this->assertSame(1, $marked); + $this->assertSame(1, $this->assessor->openCount($this->site('shop.example.com')), 'http-foundation is not on stg'); + } + + public function testAMarkRecordsTheReasonAndWhoMadeIt(): void + { + $this->assessor->markNotAffected($this->site('shop.example.com'), [self::TWIG], ' Templates are not user editable ', 'user@example.com'); + + $assessment = $this->entityManager->getRepository(AdvisoryAssessment::class)->findOneBy(['advisoryId' => self::TWIG]); + $this->assertSame(['github.com', 'example-org', 'shop'], [$assessment->getRepoProvider(), $assessment->getRepoOrganization(), $assessment->getRepoName()]); + $this->assertSame('Templates are not user editable', $assessment->getReason()); + $this->assertSame('user@example.com', $assessment->getAssessedBy()); + } + + public function testAReasonIsRequired(): void + { + $this->expectException(\InvalidArgumentException::class); + + $this->assessor->markNotAffected($this->site('shop.example.com'), [self::TWIG], ' ', 'user@example.com'); + } + + public function testASiteWithoutAGitRepoCannotBeMarked(): void + { + $site = $this->site('release.example.com'); + $this->assertNull($this->assessor->gitRepo($site)); + + $this->expectException(\InvalidArgumentException::class); + + $this->assessor->markNotAffected($site, ['SA-CORE-2099-001'], 'Reviewed', 'user@example.com'); + } + + public function testRemovingAMarkCountsTheAdvisoryAgain(): void + { + $site = $this->site('intranet.example.com'); + $this->assertSame(3, $this->assessor->openCount($site), 'The fixtures mark commonmark'); + + $this->assessor->removeMark($site, 'PKSA-m3r8-5tyv-1h6p'); + + $this->assertSame(4, $this->assessor->openCount($site)); + } + + public function testAdvisoriesListTheirMark(): void + { + $marks = []; + foreach ($this->assessor->advisories($this->site('intranet.example.com')) as ['advisory' => $advisory, 'assessment' => $assessment]) { + $marks[$advisory->getAdvisoryId()] = $assessment?->getReason(); + } + ksort($marks); + + $this->assertSame([ + 'PKSA-d8k2-6vnp-4j9t' => null, + 'PKSA-m3r8-5tyv-1h6p' => 'Markdown is only rendered from editor input', + 'SA-CONTRIB-2099-001' => null, + 'SA-CORE-2099-001' => null, + ], $marks); + } + + private function site(string $primaryDomain): Site + { + return $this->entityManager->getRepository(Site::class)->findOneBy(['primaryDomain' => $primaryDomain]); + } +}