From 884a24c1d99558880590595a2a3c2c9c9058974e Mon Sep 17 00:00:00 2001 From: Steven Rombauts Date: Thu, 13 Nov 2025 17:13:12 +0100 Subject: [PATCH 01/15] Add `PACKAGE_NAME_REGEX` constant --- src/Controller/FeedController.php | 2 +- src/Controller/PackageController.php | 8 ++++---- src/Entity/Package.php | 2 ++ src/Validator/ValidPackageRepositoryValidator.php | 2 +- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/Controller/FeedController.php b/src/Controller/FeedController.php index 1c4e9c82d..016d1fbae 100644 --- a/src/Controller/FeedController.php +++ b/src/Controller/FeedController.php @@ -130,7 +130,7 @@ public function extensionReleasesAction(Request $req): Response return $this->buildResponse($req, $feed); } - #[Route(path: '/package.{package}.{_format}', name: 'feed_package', requirements: ['_format' => '(rss|atom)', 'package' => '[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+'], methods: ['GET'])] + #[Route(path: '/package.{package}.{_format}', name: 'feed_package', requirements: ['_format' => '(rss|atom)', 'package' => Package::PACKAGE_NAME_REGEX], methods: ['GET'])] public function packageAction(Request $req, string $package): Response { $repo = $this->doctrine->getRepository(Version::class); diff --git a/src/Controller/PackageController.php b/src/Controller/PackageController.php index e870a9158..194ce6600 100644 --- a/src/Controller/PackageController.php +++ b/src/Controller/PackageController.php @@ -827,7 +827,7 @@ public function deletePackageVersionAction(Request $req, int $versionId): Respon return new Response('', 204); } - #[Route(path: '/packages/{name}', name: 'update_package', requirements: ['name' => '[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+'], defaults: ['_format' => 'json'], methods: ['PUT'])] + #[Route(path: '/packages/{name}', name: 'update_package', requirements: ['name' => Package::PACKAGE_NAME_REGEX], defaults: ['_format' => 'json'], methods: ['PUT'])] public function updatePackageAction(Request $req, string $name, #[CurrentUser] User $user): Response { try { @@ -879,7 +879,7 @@ public function updatePackageAction(Request $req, string $name, #[CurrentUser] U return new JsonResponse(['status' => 'error', 'message' => 'Package was already updated in the last 24 hours'], 404); } - #[Route(path: '/packages/{name}', name: 'delete_package', requirements: ['name' => '[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+'], methods: ['DELETE'])] + #[Route(path: '/packages/{name}', name: 'delete_package', requirements: ['name' => Package::PACKAGE_NAME_REGEX], methods: ['DELETE'])] public function deletePackageAction(Request $req, string $name): Response { $package = $this->getPartialPackageWithVersions($req, $name); @@ -904,7 +904,7 @@ public function deletePackageAction(Request $req, string $name): Response return new Response('Invalid form input', 400); } - #[Route(path: '/packages/{name:package}/maintainers/', name: 'add_maintainer', requirements: ['name' => '[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+'])] + #[Route(path: '/packages/{name:package}/maintainers/', name: 'add_maintainer', requirements: ['name' => Package::PACKAGE_NAME_REGEX])] public function createMaintainerAction(Request $req, #[MapEntity] Package $package, #[CurrentUser] User $user, LoggerInterface $logger): RedirectResponse { $this->denyAccessUnlessGranted(PackageActions::AddMaintainer->value, $package); @@ -942,7 +942,7 @@ public function createMaintainerAction(Request $req, #[MapEntity] Package $packa return $this->redirectToRoute('view_package', ['name' => $package->getName()]); } - #[Route(path: '/packages/{name:package}/maintainers/delete', name: 'remove_maintainer', requirements: ['name' => '[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+'])] + #[Route(path: '/packages/{name:package}/maintainers/delete', name: 'remove_maintainer', requirements: ['name' => Package::PACKAGE_NAME_REGEX])] public function removeMaintainerAction(Request $req, #[MapEntity] Package $package, #[CurrentUser] User $user, LoggerInterface $logger): Response { $this->denyAccessUnlessGranted(PackageActions::RemoveMaintainer->value, $package); diff --git a/src/Entity/Package.php b/src/Entity/Package.php index 29f68f57c..d4a5c78b4 100644 --- a/src/Entity/Package.php +++ b/src/Entity/Package.php @@ -90,6 +90,8 @@ class Package public const AUTO_MANUAL_HOOK = 1; public const AUTO_GITHUB_HOOK = 2; + public const string PACKAGE_NAME_REGEX = '[a-zA-Z0-9](?:[_.-]?[a-zA-Z0-9]+)*/[a-zA-Z0-9](?:[_.-]?[a-zA-Z0-9]+)*'; + #[ORM\Id] #[ORM\Column(type: 'integer')] #[ORM\GeneratedValue(strategy: 'AUTO')] diff --git a/src/Validator/ValidPackageRepositoryValidator.php b/src/Validator/ValidPackageRepositoryValidator.php index 5f918baa3..f5484d57b 100644 --- a/src/Validator/ValidPackageRepositoryValidator.php +++ b/src/Validator/ValidPackageRepositoryValidator.php @@ -90,7 +90,7 @@ public function validate(mixed $value, Constraint $constraint): void } $name = $value->getName(); - if (!Preg::isMatch('{^[a-z0-9]([_.-]?[a-z0-9]+)*/[a-z0-9]([_.-]?[a-z0-9]+)*$}iD', $name)) { + if (!Preg::isMatch(sprintf('{^%s$}D', Package::PACKAGE_NAME_REGEX), $name)) { $this->addViolation('The package name '.htmlentities($name, \ENT_COMPAT, 'utf-8').' is invalid, it should have a vendor name, a forward slash, and a package name. The vendor and package name can be words separated by -, . or _. The complete name should match "[a-z0-9]([_.-]?[a-z0-9]+)*/[a-z0-9]([_.-]?[a-z0-9]+)*".'); return; From 4729629c6fb438702000e1e2b958e7de9cc01f4c Mon Sep 17 00:00:00 2001 From: Steven Rombauts Date: Thu, 13 Nov 2025 21:12:02 +0100 Subject: [PATCH 02/15] Add `TransferPackage` action to `PackageVoter` --- src/Security/Voter/PackageActions.php | 1 + src/Security/Voter/PackageVoter.php | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Security/Voter/PackageActions.php b/src/Security/Voter/PackageActions.php index 71d4c3d24..f21ea67d0 100644 --- a/src/Security/Voter/PackageActions.php +++ b/src/Security/Voter/PackageActions.php @@ -17,6 +17,7 @@ enum PackageActions: string case Edit = 'edit'; case AddMaintainer = 'add_maintainer'; case RemoveMaintainer = 'remove_maintainer'; + case TransferPackage = 'transfer_package'; case Abandon = 'abandon'; case Delete = 'delete'; case DeleteVersion = 'delete_version'; diff --git a/src/Security/Voter/PackageVoter.php b/src/Security/Voter/PackageVoter.php index 7ee873554..d970b9fed 100644 --- a/src/Security/Voter/PackageVoter.php +++ b/src/Security/Voter/PackageVoter.php @@ -54,7 +54,7 @@ protected function voteOnAttribute(string $attribute, mixed $subject, TokenInter PackageActions::Delete => $this->canDelete($package, $user), PackageActions::DeleteVersion => $this->canDeleteVersion($package, $user), PackageActions::Edit => $this->canEdit($package, $user), - PackageActions::AddMaintainer => $this->canAddMaintainers($package, $user), + PackageActions::AddMaintainer, PackageActions::TransferPackage => $this->canAddMaintainers($package, $user), PackageActions::RemoveMaintainer => $this->canRemoveMaintainers($package, $user), PackageActions::Update => $package->isMaintainer($user) || $this->security->isGranted('ROLE_UPDATE_PACKAGES'), }; From 99eee3b36978c89701add0b183f6fb5710548142 Mon Sep 17 00:00:00 2001 From: Steven Rombauts Date: Thu, 13 Nov 2025 21:13:20 +0100 Subject: [PATCH 03/15] Create the `TransferPackageRequestType` form --- src/Form/Model/TransferPackageRequest.php | 44 ++++++++++++++ src/Form/Type/MaintainerType.php | 62 ++++++++++++++++++++ src/Form/Type/TransferPackageRequestType.php | 56 ++++++++++++++++++ 3 files changed, 162 insertions(+) create mode 100644 src/Form/Model/TransferPackageRequest.php create mode 100644 src/Form/Type/MaintainerType.php create mode 100644 src/Form/Type/TransferPackageRequestType.php diff --git a/src/Form/Model/TransferPackageRequest.php b/src/Form/Model/TransferPackageRequest.php new file mode 100644 index 000000000..afc957e67 --- /dev/null +++ b/src/Form/Model/TransferPackageRequest.php @@ -0,0 +1,44 @@ + + * Nils Adermann + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace App\Form\Model; + +use App\Entity\User; +use Doctrine\Common\Collections\ArrayCollection; +use Doctrine\Common\Collections\Collection; + +class TransferPackageRequest +{ + /** @var Collection */ + private Collection $maintainers; + + public function __construct() + { + $this->maintainers = new ArrayCollection(); + } + + /** + * @return Collection + */ + public function getMaintainers(): Collection + { + return $this->maintainers; + } + + /** + * @param Collection $maintainers + */ + public function setMaintainers(Collection $maintainers): void + { + $this->maintainers = $maintainers; + } +} diff --git a/src/Form/Type/MaintainerType.php b/src/Form/Type/MaintainerType.php new file mode 100644 index 000000000..c453db5a0 --- /dev/null +++ b/src/Form/Type/MaintainerType.php @@ -0,0 +1,62 @@ + + * Nils Adermann + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace App\Form\Type; + +use App\Entity\User; +use Doctrine\ORM\EntityManagerInterface; +use Symfony\Component\Form\AbstractType; +use Symfony\Component\Form\Extension\Core\Type\TextType; +use Symfony\Component\Form\FormBuilderInterface; +use Symfony\Component\Form\CallbackTransformer; +use Symfony\Component\OptionsResolver\OptionsResolver; + +/** + * @extends AbstractType + */ +class MaintainerType extends AbstractType +{ + public function __construct( + private readonly EntityManagerInterface $em, + ) { + } + + public function buildForm(FormBuilderInterface $builder, array $options): void + { + $builder->addModelTransformer(new CallbackTransformer( + function (?User $user): string { + return $user?->getUsername() ?? ''; + }, + function (?string $username): ?User { + if (!$username) { + return null; + } + + return $this->em->getRepository(User::class)->findOneByUsernameOrEmail($username); + } + )); + } + + public function getParent(): string + { + return TextType::class; + } + + public function configureOptions(OptionsResolver $resolver): void + { + $resolver->setDefaults([ + 'attr' => [ + 'placeholder' => 'Username or email', + ], + ]); + } +} diff --git a/src/Form/Type/TransferPackageRequestType.php b/src/Form/Type/TransferPackageRequestType.php new file mode 100644 index 000000000..173cc934a --- /dev/null +++ b/src/Form/Type/TransferPackageRequestType.php @@ -0,0 +1,56 @@ + + * Nils Adermann + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace App\Form\Type; + +use App\Form\Model\TransferPackageRequest; +use Symfony\Component\Form\AbstractType; +use Symfony\Component\Form\Extension\Core\Type\CollectionType; +use Symfony\Component\Form\FormBuilderInterface; +use Symfony\Component\OptionsResolver\OptionsResolver; + +/** + * @extends AbstractType + */ +class TransferPackageRequestType extends AbstractType +{ + public function buildForm(FormBuilderInterface $builder, array $options): void + { + $builder->add('maintainers', CollectionType::class, [ + 'entry_type' => MaintainerType::class, + 'entry_options' => [ + 'label' => false, + ], + 'allow_add' => true, + 'allow_delete' => true, + 'delete_empty' => true, + 'prototype' => true, + 'prototype_name' => '__name__', + 'label' => 'New Maintainers', + 'attr' => [ + 'class' => 'maintainers-collection', + ], + ]); + } + + public function configureOptions(OptionsResolver $resolver): void + { + $resolver->setDefaults([ + 'data_class' => TransferPackageRequest::class, + ]); + } + + public function getBlockPrefix(): string + { + return 'transfer_package_form'; + } +} From 62d6807fe56b0066ca2a723370c071981c8be679 Mon Sep 17 00:00:00 2001 From: Steven Rombauts Date: Thu, 13 Nov 2025 21:26:42 +0100 Subject: [PATCH 04/15] Create the form to transfer package ownership --- css/app.scss | 49 +++++++++++++++++++++++- js/view.js | 49 ++++++++++++++++++++++-- src/Controller/PackageController.php | 49 ++++++++++++++++++++++++ src/Model/PackageManager.php | 27 +++++++++++++ templates/package/view_package.html.twig | 28 ++++++++++++++ 5 files changed, 197 insertions(+), 5 deletions(-) diff --git a/css/app.scss b/css/app.scss index 32e1d7930..91070a7f9 100644 --- a/css/app.scss +++ b/css/app.scss @@ -1082,12 +1082,19 @@ input:focus:invalid:focus, textarea:focus:invalid:focus, select:focus:invalid:fo margin-bottom: 4px; margin-right: 4px; } -.package .details #add-maintainer { +.package .details #add-maintainer{ margin-top: 3px; margin-bottom: 7px; float: right; font-size: 34px; } +.package .details #transfer-package { + margin-top: 7px; + margin-bottom: 7px; + margin-right: 4px; + float: right; + font-size: 34px; +} .package .details #remove-maintainer { float: right; font-size: 35px; @@ -1884,3 +1891,43 @@ body { } } } + +#transfer-package-form { + .maintainers-collection-wrapper { + margin-bottom: 15px; + + label { + font-weight: bold; + margin-bottom: 10px; + } + + .maintainers-list { + list-style: none; + padding: 0; + margin-bottom: 10px; + + li { + display: flex; + gap: 10px; + margin-bottom: 10px; + align-items: center; + + input[type="text"] { + flex: 1; + padding: 6px 12px; + font-size: 14px; + border: 1px solid #ccc; + border-radius: 4px; + } + + .btn-danger { + flex-shrink: 0; + } + } + } + + .add-maintainer-item { + margin-top: 5px; + } + } +} diff --git a/js/view.js b/js/view.js index d411a4ec9..558916774 100644 --- a/js/view.js +++ b/js/view.js @@ -7,14 +7,23 @@ const init = function ($) { var versionCache = {}, ongoingRequest = false; - $('#add-maintainer').on('click', function (e) { + const togglePackageForm = function (selector) { $('#remove-maintainer-form').addClass('hidden'); - $('#add-maintainer-form').removeClass('hidden'); + $('#add-maintainer-form').addClass('hidden'); + $('#transfer-package-form').addClass('hidden'); + $(selector).removeClass('hidden'); + } + + $('#add-maintainer').on('click', function (e) { + togglePackageForm('#add-maintainer-form'); e.preventDefault(); }); $('#remove-maintainer').on('click', function (e) { - $('#add-maintainer-form').addClass('hidden'); - $('#remove-maintainer-form').removeClass('hidden'); + togglePackageForm('#remove-maintainer-form'); + e.preventDefault(); + }); + $('#transfer-package').on('click', function (e) { + togglePackageForm('#transfer-package-form'); e.preventDefault(); }); @@ -227,6 +236,38 @@ const init = function ($) { $(versionsList).css('max-height', 'inherit'); }); } + + // Handle add/remove buttons for transfer package form + $('.add-maintainer-item').on('click', function (e) { + e.preventDefault(); + + var list = $('.maintainers-list'); + var prototype = list.data('prototype'); + var index = list.find('li').length + 1; + + var newForm = prototype.replace(/__name__/g, index); + var newItem = $('
  • ').append(newForm); + addMaintainerRemoveButton(newItem); + list.append(newItem); + }); + + $('.maintainers-list').find('li').each(function(index) { + addMaintainerRemoveButton($(this)); + }); + + function addMaintainerRemoveButton(item) { + var removeButton = $(''); + removeButton.on('click', function(e) { + e.preventDefault(); + + if ($('.maintainers-list').find('li').length === 1) { + return; + } + + item.remove(); + }); + item.append(removeButton); + } }; if (document.querySelector('#view-package-page')) { diff --git a/src/Controller/PackageController.php b/src/Controller/PackageController.php index 194ce6600..11370cbeb 100644 --- a/src/Controller/PackageController.php +++ b/src/Controller/PackageController.php @@ -27,10 +27,12 @@ use App\Entity\Vendor; use App\Entity\Version; use App\Form\Model\MaintainerRequest; +use App\Form\Model\TransferPackageRequest; use App\Form\Type\AbandonedType; use App\Form\Type\AddMaintainerRequestType; use App\Form\Type\PackageType; use App\Form\Type\RemoveMaintainerRequestType; +use App\Form\Type\TransferPackageRequestType; use App\Model\DownloadManager; use App\Model\FavoriteManager; use App\Model\PackageManager; @@ -692,6 +694,7 @@ public function viewPackageAction(Request $req, string $name, CsrfTokenManagerIn $data['addMaintainerForm'] = $this->createAddMaintainerForm($package)->createView(); $data['removeMaintainerForm'] = $this->createRemoveMaintainerForm($package)->createView(); + $data['transferPackageForm'] = $this->createTransferPackageForm($package)->createView(); $data['deleteForm'] = $this->createDeletePackageForm($package)->createView(); } else { $data['hasVersionSecurityAdvisories'] = []; @@ -986,6 +989,41 @@ public function removeMaintainerAction(Request $req, #[MapEntity] Package $packa ]); } + + #[Route(path: '/packages/{name:package}/transfer/', name: 'transfer_package', requirements: ['name' => Package::PACKAGE_NAME_REGEX])] + public function transferPackageAction(Request $req, #[MapEntity] Package $package, #[CurrentUser] User $user, LoggerInterface $logger): RedirectResponse + { + $this->denyAccessUnlessGranted(PackageActions::TransferPackage->value, $package); + + $oldMaintainers = $package->getMaintainers()->toArray(); + + $form = $this->createTransferPackageForm($package); + $form->handleRequest($req); + + if ($form->isSubmitted() && $form->isValid()) { + try { + $em = $this->getEM(); + $newMaintainers = $form->getData()->getMaintainers()->toArray(); + $result = $this->packageManager->transferPackage($package, $oldMaintainers, $newMaintainers); + $em->flush(); + + if ($result) { + $usernames = array_map(fn (User $user) => $user->getUsername(), $newMaintainers); + $this->addFlash('success', sprintf('Package has been transferred to %s', implode(', ', $usernames))); + } else { + $this->addFlash('warning', 'Package maintainers are identical and have not been changed'); + } + + return $this->redirectToRoute('view_package', ['name' => $package->getName()]); + } catch (\Exception $e) { + $logger->critical($e->getMessage(), ['exception', $e]); + $this->addFlash('error', 'The package could not be transferred.'); + } + } + + return $this->redirectToRoute('view_package', ['name' => $package->getName()]); + } + #[Route(path: '/packages/{name:package}/edit', name: 'edit_package', requirements: ['name' => '[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+?'])] public function editAction(Request $req, #[MapEntity] Package $package, #[CurrentUser] ?User $user = null): Response { @@ -1623,6 +1661,17 @@ private function createRemoveMaintainerForm(Package $package): FormInterface ]); } + /** + * @return FormInterface + */ + private function createTransferPackageForm(Package $package): FormInterface + { + $transferRequest = new TransferPackageRequest(); + $transferRequest->setMaintainers($package->getMaintainers()); + + return $this->createForm(TransferPackageRequestType::class, $transferRequest); + } + /** * @return FormInterface */ diff --git a/src/Model/PackageManager.php b/src/Model/PackageManager.php index b1a171c38..b967b0d7c 100644 --- a/src/Model/PackageManager.php +++ b/src/Model/PackageManager.php @@ -14,6 +14,7 @@ use Algolia\AlgoliaSearch\Exceptions\AlgoliaException; use Algolia\AlgoliaSearch\SearchClient; +use App\Entity\AuditRecord; use App\Entity\Dependent; use App\Entity\Download; use App\Entity\EmptyReferenceCache; @@ -239,4 +240,30 @@ public function notifyNewMaintainer(User $user, Package $package): bool return true; } + + /* + * @param User[] $oldMaintainers + * @param User[] $newMaintainers + */ + public function transferPackage(Package $package, array $oldMaintainers, array $newMaintainers): bool + { + $normalizedOldMaintainers = array_values(array_map(fn (User $user) => $user->getId(), $oldMaintainers)); + sort($normalizedOldMaintainers, SORT_NUMERIC); + + $normalizedMaintainers = array_values(array_map(fn (User $user) => $user->getId(), $newMaintainers)); + sort($normalizedMaintainers, SORT_NUMERIC); + + if ($normalizedMaintainers === $normalizedOldMaintainers) { + return false; + } + + $package->getMaintainers()->clear(); + foreach ($newMaintainers as $maintainer) { + $package->addMaintainer($maintainer); + } + + $this->doctrine->getManager()->persist(AuditRecord::packageTransferred($package, null, $oldMaintainers, $newMaintainers)); + + return true; + } } diff --git a/templates/package/view_package.html.twig b/templates/package/view_package.html.twig index 04868460e..7a99b0a31 100644 --- a/templates/package/view_package.html.twig +++ b/templates/package/view_package.html.twig @@ -146,6 +146,7 @@ {% endfor %} {% if is_granted('remove_maintainer', package) %}{% endif %} {% if is_granted('add_maintainer', package) %}{% endif %} + {% if is_granted('transfer_package', package) %}{% endif %}

    Details
    @@ -319,6 +320,33 @@ {% endif %} + {% if is_granted('transfer_package', package) %} +
    + {{ form_start(transferPackageForm, { + attr: { id: 'transfer-package-form', class: 'col-sm-6 col-md-3 col-md-offset-9 ' ~ (show_transfer_package_form|default(false) ? '': 'hidden') }, + action: path('transfer_package', { 'name': package.name }) + }) }} +
    +

    Transfer Ownership

    +
    + {{ form_errors(transferPackageForm.maintainers) }} +
      + {% for maintainerField in transferPackageForm.maintainers %} +
    • + {{ form_errors(maintainerField) }} + {{ form_widget(maintainerField) }} +
      • + {% endfor %} +
    + +
    + {{ form_rest(transferPackageForm) }} + +
    + {{ form_end(transferPackageForm) }} +
    + {% endif %} + {% if versions|length %}
    From 9fc3165ba53380f7dfd1cfc191827ceeee427abf Mon Sep 17 00:00:00 2001 From: Steven Rombauts Date: Thu, 13 Nov 2025 22:09:59 +0100 Subject: [PATCH 05/15] Add form validation to `transferPackageAction()` --- src/Controller/PackageController.php | 4 ++ src/Form/Model/TransferPackageRequest.php | 2 + src/Form/Type/MaintainerType.php | 16 ++++++- .../TransferPackageValidMaintainersList.php | 21 +++++++++ ...erPackageValidMaintainersListValidator.php | 46 +++++++++++++++++++ 5 files changed, 87 insertions(+), 2 deletions(-) create mode 100644 src/Validator/Constraints/TransferPackageValidMaintainersList.php create mode 100644 src/Validator/Constraints/TransferPackageValidMaintainersListValidator.php diff --git a/src/Controller/PackageController.php b/src/Controller/PackageController.php index 11370cbeb..180a321d6 100644 --- a/src/Controller/PackageController.php +++ b/src/Controller/PackageController.php @@ -1019,6 +1019,10 @@ public function transferPackageAction(Request $req, #[MapEntity] Package $packag $logger->critical($e->getMessage(), ['exception', $e]); $this->addFlash('error', 'The package could not be transferred.'); } + } elseif (!$form->isValid()) { + foreach ($form->getErrors(true, true) as $error) { + $this->addFlash('error', $error->getMessage()); + } } return $this->redirectToRoute('view_package', ['name' => $package->getName()]); diff --git a/src/Form/Model/TransferPackageRequest.php b/src/Form/Model/TransferPackageRequest.php index afc957e67..747debb94 100644 --- a/src/Form/Model/TransferPackageRequest.php +++ b/src/Form/Model/TransferPackageRequest.php @@ -13,12 +13,14 @@ namespace App\Form\Model; use App\Entity\User; +use App\Validator\Constraints\TransferPackageValidMaintainersList; use Doctrine\Common\Collections\ArrayCollection; use Doctrine\Common\Collections\Collection; class TransferPackageRequest { /** @var Collection */ + #[TransferPackageValidMaintainersList] private Collection $maintainers; public function __construct() diff --git a/src/Form/Type/MaintainerType.php b/src/Form/Type/MaintainerType.php index c453db5a0..930c9457f 100644 --- a/src/Form/Type/MaintainerType.php +++ b/src/Form/Type/MaintainerType.php @@ -15,9 +15,10 @@ use App\Entity\User; use Doctrine\ORM\EntityManagerInterface; use Symfony\Component\Form\AbstractType; +use Symfony\Component\Form\CallbackTransformer; +use Symfony\Component\Form\Exception\TransformationFailedException; use Symfony\Component\Form\Extension\Core\Type\TextType; use Symfony\Component\Form\FormBuilderInterface; -use Symfony\Component\Form\CallbackTransformer; use Symfony\Component\OptionsResolver\OptionsResolver; /** @@ -41,7 +42,18 @@ function (?string $username): ?User { return null; } - return $this->em->getRepository(User::class)->findOneByUsernameOrEmail($username); + $user = $this->em->getRepository(User::class)->findOneByUsernameOrEmail($username); + + if ($user === null) { + $failure = new TransformationFailedException(sprintf('User "%s" does not exist.', $username)); + $failure->setInvalidMessage('The given "{{ value }}" value is not a valid username or email.', [ + '{{ value }}' => $username, + ]); + + throw $failure; + } + + return $user; } )); } diff --git a/src/Validator/Constraints/TransferPackageValidMaintainersList.php b/src/Validator/Constraints/TransferPackageValidMaintainersList.php new file mode 100644 index 000000000..f423e9d57 --- /dev/null +++ b/src/Validator/Constraints/TransferPackageValidMaintainersList.php @@ -0,0 +1,21 @@ + + * Nils Adermann + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace App\Validator\Constraints; + +use Symfony\Component\Validator\Constraint; + +#[\Attribute] +class TransferPackageValidMaintainersList extends Constraint +{ + public string $emptyMessage = 'At least one maintainer must be specified.'; +} diff --git a/src/Validator/Constraints/TransferPackageValidMaintainersListValidator.php b/src/Validator/Constraints/TransferPackageValidMaintainersListValidator.php new file mode 100644 index 000000000..3036b697b --- /dev/null +++ b/src/Validator/Constraints/TransferPackageValidMaintainersListValidator.php @@ -0,0 +1,46 @@ + + * Nils Adermann + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace App\Validator\Constraints; + +use App\Entity\User; +use App\Form\Model\InvalidMaintainer; +use Doctrine\Common\Collections\Collection; +use Symfony\Component\Validator\Constraint; +use Symfony\Component\Validator\ConstraintValidator; +use Symfony\Component\Validator\Exception\UnexpectedTypeException; +use Symfony\Component\Validator\Exception\UnexpectedValueException; + +class TransferPackageValidMaintainersListValidator extends ConstraintValidator +{ + public function validate(mixed $value, Constraint $constraint): void + { + if (!$constraint instanceof TransferPackageValidMaintainersList) { + throw new UnexpectedTypeException($constraint, TransferPackageValidMaintainersList::class); + } + + if (null === $value) { + return; + } + + if (!$value instanceof Collection) { + throw new UnexpectedValueException($value, Collection::class); + } + + if (!$value->isEmpty()) { + return; + } + + $this->context->buildViolation($constraint->emptyMessage) + ->addViolation(); + } +} From e679bfbb21be5837bb893d5182c714dbb12e874d Mon Sep 17 00:00:00 2001 From: Steven Rombauts Date: Fri, 14 Nov 2025 08:38:45 +0100 Subject: [PATCH 06/15] Move maintainer actions below the list of maintainers --- css/app.scss | 3 --- templates/package/view_package.html.twig | 10 +++++++--- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/css/app.scss b/css/app.scss index 91070a7f9..02fddc488 100644 --- a/css/app.scss +++ b/css/app.scss @@ -1085,18 +1085,15 @@ input:focus:invalid:focus, textarea:focus:invalid:focus, select:focus:invalid:fo .package .details #add-maintainer{ margin-top: 3px; margin-bottom: 7px; - float: right; font-size: 34px; } .package .details #transfer-package { margin-top: 7px; margin-bottom: 7px; margin-right: 4px; - float: right; font-size: 34px; } .package .details #remove-maintainer { - float: right; font-size: 35px; margin-top: 5px; } diff --git a/templates/package/view_package.html.twig b/templates/package/view_package.html.twig index 7a99b0a31..470d2e6e6 100644 --- a/templates/package/view_package.html.twig +++ b/templates/package/view_package.html.twig @@ -144,10 +144,14 @@ {% for maintainer in package.maintainers -%} {% endfor %} - {% if is_granted('remove_maintainer', package) %}{% endif %} +

    + + {% if is_granted('remove_maintainer', package) or is_granted('add_maintainer', package) or is_granted('transfer_package', package) %} +
    Maintainer actions
    {% if is_granted('add_maintainer', package) %}{% endif %} + {% if is_granted('remove_maintainer', package) %}{% endif %} {% if is_granted('transfer_package', package) %}{% endif %} -

    + {% endif %}
    Details
    {% set repoUrl = package.browsableRepository %} @@ -338,7 +342,7 @@ {% endfor %} - +
    {{ form_rest(transferPackageForm) }} From 5cb5b9307c6c7245fe78fa1d9cb474eabe184bd2 Mon Sep 17 00:00:00 2001 From: Steven Rombauts Date: Fri, 14 Nov 2025 08:46:26 +0100 Subject: [PATCH 07/15] Add tests for `PackageController::transferPackageAction()` --- src/Command/TransferOwnershipCommand.php | 20 +---- src/Controller/PackageController.php | 9 +- src/Model/PackageManager.php | 3 +- .../Command/TransferOwnershipCommandTest.php | 63 +------------- tests/Controller/PackageControllerTest.php | 79 +++++++++++++++++ tests/Model/PackageManagerTest.php | 85 ++++++++++++++++++- 6 files changed, 172 insertions(+), 87 deletions(-) diff --git a/src/Command/TransferOwnershipCommand.php b/src/Command/TransferOwnershipCommand.php index c1cb01146..b31125ce0 100644 --- a/src/Command/TransferOwnershipCommand.php +++ b/src/Command/TransferOwnershipCommand.php @@ -15,6 +15,7 @@ use App\Entity\AuditRecord; use App\Entity\Package; use App\Entity\User; +use App\Model\PackageManager; use App\Util\DoctrineTrait; use Composer\Console\Input\InputOption; use Doctrine\Persistence\ManagerRegistry; @@ -30,6 +31,7 @@ class TransferOwnershipCommand extends Command public function __construct( private readonly ManagerRegistry $doctrine, + private readonly PackageManager $packageManager, ) { parent::__construct(); @@ -165,24 +167,8 @@ private function outputPackageTable(OutputInterface $output, array $packages, ar */ private function transferOwnership(array $packages, array $maintainers): void { - $normalizedMaintainers = array_values(array_map(fn (User $user) => $user->getId(), $maintainers)); - sort($normalizedMaintainers, SORT_NUMERIC); - foreach ($packages as $package) { - $oldMaintainers = $package->getMaintainers()->toArray(); - - $normalizedOldMaintainers = array_values(array_map(fn (User $user) => $user->getId(), $oldMaintainers)); - sort($normalizedOldMaintainers, SORT_NUMERIC); - if ($normalizedMaintainers === $normalizedOldMaintainers) { - continue; - } - - $package->getMaintainers()->clear(); - foreach ($maintainers as $maintainer) { - $package->addMaintainer($maintainer); - } - - $this->doctrine->getManager()->persist(AuditRecord::packageTransferred($package, null, $oldMaintainers, array_values($maintainers))); + $this->packageManager->transferPackage($package, $maintainers); } $this->doctrine->getManager()->flush(); diff --git a/src/Controller/PackageController.php b/src/Controller/PackageController.php index 180a321d6..85e9ed79d 100644 --- a/src/Controller/PackageController.php +++ b/src/Controller/PackageController.php @@ -995,17 +995,14 @@ public function transferPackageAction(Request $req, #[MapEntity] Package $packag { $this->denyAccessUnlessGranted(PackageActions::TransferPackage->value, $package); - $oldMaintainers = $package->getMaintainers()->toArray(); - $form = $this->createTransferPackageForm($package); $form->handleRequest($req); if ($form->isSubmitted() && $form->isValid()) { try { - $em = $this->getEM(); $newMaintainers = $form->getData()->getMaintainers()->toArray(); - $result = $this->packageManager->transferPackage($package, $oldMaintainers, $newMaintainers); - $em->flush(); + $result = $this->packageManager->transferPackage($package, $newMaintainers); + $this->getEM()->flush(); if ($result) { $usernames = array_map(fn (User $user) => $user->getUsername(), $newMaintainers); @@ -1671,7 +1668,7 @@ private function createRemoveMaintainerForm(Package $package): FormInterface private function createTransferPackageForm(Package $package): FormInterface { $transferRequest = new TransferPackageRequest(); - $transferRequest->setMaintainers($package->getMaintainers()); + $transferRequest->setMaintainers(clone $package->getMaintainers()); return $this->createForm(TransferPackageRequestType::class, $transferRequest); } diff --git a/src/Model/PackageManager.php b/src/Model/PackageManager.php index b967b0d7c..4f5db1abd 100644 --- a/src/Model/PackageManager.php +++ b/src/Model/PackageManager.php @@ -245,8 +245,9 @@ public function notifyNewMaintainer(User $user, Package $package): bool * @param User[] $oldMaintainers * @param User[] $newMaintainers */ - public function transferPackage(Package $package, array $oldMaintainers, array $newMaintainers): bool + public function transferPackage(Package $package, array $newMaintainers): bool { + $oldMaintainers = $package->getMaintainers()->toArray(); $normalizedOldMaintainers = array_values(array_map(fn (User $user) => $user->getId(), $oldMaintainers)); sort($normalizedOldMaintainers, SORT_NUMERIC); diff --git a/tests/Command/TransferOwnershipCommandTest.php b/tests/Command/TransferOwnershipCommandTest.php index ef167fc6d..d21946a80 100644 --- a/tests/Command/TransferOwnershipCommandTest.php +++ b/tests/Command/TransferOwnershipCommandTest.php @@ -12,14 +12,12 @@ namespace App\Tests\Command; -use App\Audit\AuditRecordType; use App\Command\TransferOwnershipCommand; -use App\Entity\AuditRecord; use App\Entity\Package; use App\Entity\User; +use App\Model\PackageManager; use App\Tests\IntegrationTestCase; use Doctrine\Persistence\ManagerRegistry; -use PHPUnit\Framework\Attributes\TestWith; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Tester\CommandTester; @@ -45,7 +43,7 @@ protected function setUp(): void $this->package3 = self::createPackage('vendor2/package1', 'https://github.com/vendor2/package1',maintainers: [$john]); $this->store($this->package1, $this->package2, $this->package3); - $command = new TransferOwnershipCommand(self::getContainer()->get(ManagerRegistry::class)); + $command = new TransferOwnershipCommand(self::getContainer()->get(ManagerRegistry::class), self::getContainer()->get(PackageManager::class)); $this->commandTester = new CommandTester($command); } @@ -73,9 +71,6 @@ public function testExecuteSuccessForVendor(): void $this->assertEqualsCanonicalizing(['alice', 'bob'], array_map($callable, $package1->getMaintainers()->toArray())); $this->assertEqualsCanonicalizing(['alice', 'bob'], array_map($callable, $package2->getMaintainers()->toArray())); $this->assertEqualsCanonicalizing(['john'], array_map($callable, $package3->getMaintainers()->toArray()), 'vendor2 packages should not be changed'); - - $this->assertAuditLogWasCreated($package1, ['john', 'alice'], ['alice', 'bob']); - $this->assertAuditLogWasCreated($package2, ['john', 'bob'], ['alice', 'bob']); } public function testExecuteSuccessForPackage(): void @@ -99,8 +94,6 @@ public function testExecuteSuccessForPackage(): void $callable = fn (User $user) => $user->getUsernameCanonical(); $this->assertEqualsCanonicalizing(['bob', 'john'], array_map($callable, $package2->getMaintainers()->toArray()), 'vendor1 packages should not be changed'); $this->assertEqualsCanonicalizing(['alice', 'john'], array_map($callable, $package3->getMaintainers()->toArray())); - - $this->assertAuditLogWasCreated($package3, ['john'], ['alice', 'john']); } public function testExecuteWithDryRunDoesNothing(): void @@ -126,33 +119,6 @@ public function testExecuteWithDryRunDoesNothing(): void $this->assertEqualsCanonicalizing(['john', 'alice'], array_map($callable, $package1->getMaintainers()->toArray())); } - public function testExecuteIgnoresIdenticalMaintainers(): void - { - $this->commandTester->execute([ - 'vendorOrPackage' => 'vendor1', - 'maintainers' => ['alice', 'john'], - ]); - - $this->commandTester->assertCommandIsSuccessful(); - - $em = self::getEM(); - $em->clear(); - - $package1 = $em->find(Package::class, $this->package1->getId()); - $package2 = $em->find(Package::class, $this->package2->getId()); - - $this->assertNotNull($package1); - $this->assertNotNull($package2); - - $callable = fn (User $user) => $user->getUsernameCanonical(); - $this->assertEqualsCanonicalizing(['alice', 'john'], array_map($callable, $package1->getMaintainers()->toArray())); - $this->assertEqualsCanonicalizing(['alice', 'john'], array_map($callable, $package2->getMaintainers()->toArray())); - - $record = $this->retrieveAuditRecordForPackage($package1); - $this->assertNull($record, 'No audit log should be created if package maintainers are identical'); - $this->assertAuditLogWasCreated($package2, ['john', 'bob'], ['alice', 'john']); - } - public function testExecuteFailsWithUnknownMaintainers(): void { $this->commandTester->execute([ @@ -178,29 +144,4 @@ public function testExecuteFailsIfNoVendorPackagesFound(): void $output = $this->commandTester->getDisplay(); $this->assertStringContainsString('No packages found for foobar', $output); } - - /** - * @param string[] $oldMaintainers - * @param string[] $newMaintainers - */ - private function assertAuditLogWasCreated(Package $package, array $oldMaintainers, array $newMaintainers): void - { - $record = $this->retrieveAuditRecordForPackage($package); - $this->assertNotNull($record); - $this->assertSame('admin', $record->attributes['actor']); - $this->assertSame($package->getId(), $record->packageId); - - $callable = fn (array $user) => $user['username']; - $this->assertEqualsCanonicalizing($oldMaintainers, array_map($callable, $record->attributes['previous_maintainers'])); - $this->assertEqualsCanonicalizing($newMaintainers, array_map($callable, $record->attributes['current_maintainers'])); - } - - private function retrieveAuditRecordForPackage(Package $package): ?AuditRecord - { - return $this->getEM()->getRepository(AuditRecord::class)->findOneBy([ - 'type' => AuditRecordType::PackageTransferred->value, - 'packageId' => $package->getId(), - 'actorId' => null, - ]); - } } diff --git a/tests/Controller/PackageControllerTest.php b/tests/Controller/PackageControllerTest.php index 6c5214185..b44d064d2 100644 --- a/tests/Controller/PackageControllerTest.php +++ b/tests/Controller/PackageControllerTest.php @@ -16,6 +16,7 @@ use App\Entity\Package; use App\Entity\User; use App\Tests\IntegrationTestCase; +use PHPUnit\Framework\Attributes\TestWith; class PackageControllerTest extends IntegrationTestCase { @@ -144,4 +145,82 @@ public function testRemoveMaintainer(): void $this->assertNotNull($auditRecord); } + + public function testTransferPackage(): void + { + $john = self::createUser('john', 'john@example.org'); + $alice = self::createUser('alice', 'alice@example.org'); + $bob = self::createUser('bob', 'bob@example.org'); + $package = self::createPackage('test/pkg', 'https://example.com/test/pkg', maintainers: [$john, $alice]); + + $this->store($john, $alice, $bob, $package); + + $this->client->loginUser($john); + + $this->assertTrue($package->isMaintainer($john)); + $this->assertTrue($package->isMaintainer($alice)); + $this->assertFalse($package->isMaintainer($bob)); + + $crawler = $this->client->request('GET', '/packages/test/pkg'); + + $form = $crawler->filter('[name="transfer_package_form"]')->form(); + $form->setValues([ + 'transfer_package_form[maintainers][0]' => 'alice', + 'transfer_package_form[maintainers][1]' => 'bob@example.org', + ]); + + $this->client->submit($form); + + $this->assertResponseRedirects('/packages/test/pkg'); + $this->client->followRedirect(); + $this->assertResponseIsSuccessful(); + + $em = self::getEM(); + $em->clear(); + + $package = $em->getRepository(Package::class)->find($package->getId()); + $this->assertNotNull($package); + + $maintainerIds = array_map(fn (User $user) => $user->getId(), $package->getMaintainers()->toArray()); + $this->assertContains($alice->getId(), $maintainerIds); + $this->assertContains($bob->getId(), $maintainerIds); + $this->assertNotContains($john->getId(), $maintainerIds); + + $auditRecord = $em->getRepository(\App\Entity\AuditRecord::class)->findOneBy([ + 'type' => AuditRecordType::PackageTransferred->value, + 'packageId' => $package->getId(), + ]); + + $this->assertNotNull($auditRecord, 'Audit record not found'); + } + + #[TestWith(['does_not_exist', 'value is not a valid username or email'])] + #[TestWith([null, 'at least one maintainer must be specified'])] + public function testTransferPackageReturnsValidationError(?string $value, string $message): void + { + $alice = self::createUser('alice', 'alice@example.org'); + $package = self::createPackage('test/pkg', 'https://example.com/test/pkg', maintainers: [$alice]); + + $this->store($alice, $package); + + $this->client->loginUser($alice); + + $crawler = $this->client->request('GET', '/packages/test/pkg'); + + $form = $crawler->filter('[name="transfer_package_form"]')->form(); + $form->setValues([ + 'transfer_package_form[maintainers][0]' => $value, + ]); + + $this->client->submit($form); + + $this->assertResponseRedirects('/packages/test/pkg'); + $crawler = $this->client->followRedirect(); + $this->assertResponseIsSuccessful(); + + $elements = $crawler->filter('.flash-container .alert-error'); + $this->assertCount(1, $elements); + $text = $elements->text(); + $this->assertStringContainsStringIgnoringCase($message, $text); + } } diff --git a/tests/Model/PackageManagerTest.php b/tests/Model/PackageManagerTest.php index acfa944e7..fd5f56eae 100644 --- a/tests/Model/PackageManagerTest.php +++ b/tests/Model/PackageManagerTest.php @@ -12,11 +12,24 @@ namespace App\Tests\Model; +use App\Audit\AuditRecordType; +use App\Entity\AuditRecord; use App\Entity\Package; -use PHPUnit\Framework\TestCase; +use App\Entity\User; +use App\Model\PackageManager; +use App\Tests\IntegrationTestCase; -class PackageManagerTest extends TestCase +class PackageManagerTest extends IntegrationTestCase { + private PackageManager $packageManager; + + protected function setUp(): void + { + parent::setUp(); + + $this->packageManager = self::getContainer()->get(PackageManager::class); + } + public function testNotifyFailure(): void { $this->markTestSkipped('Do it!'); @@ -46,4 +59,72 @@ public function testNotifyFailure(): void $client->request('POST', '/api/github?username=test&apiToken=token', ['payload' => $payload]); $this->assertEquals(202, $client->getResponse()->getStatusCode()); } + + public function testTransferPackageReplacesAllMaintainers(): void + { + $alice = self::createUser('alice', 'alice@example.org'); + $bob = self::createUser('bob', 'bob@example.org'); + $john = self::createUser('john', 'john@example.org'); + $this->store($alice, $bob, $john); + + $package = self::createPackage('vendor/package', 'https://github.com/vendor/package', maintainers: [$john, $alice]); + $this->store($package); + + $result = $this->packageManager->transferPackage($package, [$bob, $alice]); + + $em = self::getEM(); + $em->flush(); + $em->clear(); + + $this->assertTrue($result); + + $callable = fn (User $user) => $user->getUsernameCanonical(); + $this->assertEqualsCanonicalizing(['alice', 'bob'], array_map($callable, $package->getMaintainers()->toArray())); + $this->assertAuditLogWasCreated($package, ['john', 'alice'], ['bob', 'alice']); + } + + public function testTransferPackageWithSameMaintainersDoesNothing(): void + { + $alice = self::createUser('alice', 'alice@example.org'); + $bob = self::createUser('bob', 'bob@example.org'); + $this->store($alice, $bob); + + $package = self::createPackage('vendor/package', 'https://github.com/vendor/package', maintainers: [$bob, $alice]); + $this->store($package); + + $result = $this->packageManager->transferPackage($package, [$alice, $bob]); + + $em = self::getEM(); + $em->flush(); + $em->clear(); + + $this->assertFalse($result); + + $record = $em->getRepository(AuditRecord::class)->findOneBy([ + 'type' => AuditRecordType::PackageTransferred->value, + 'packageId' => $package->getId(), + ]); + + $this->assertNull($record, 'No audit record should be created when maintainers are the same'); + } + + /** + * @param string[] $oldMaintainers + * @param string[] $newMaintainers + */ + private function assertAuditLogWasCreated(Package $package, array $oldMaintainers, array $newMaintainers): void + { + $record = self::getEM()->getRepository(AuditRecord::class)->findOneBy([ + 'type' => AuditRecordType::PackageTransferred->value, + 'packageId' => $package->getId(), + 'actorId' => null, + ]); + + $this->assertNotNull($record, 'Audit record should be created for package transfer'); + $this->assertSame($package->getId(), $record->packageId); + + $callable = fn (array $user) => $user['username']; + $this->assertEqualsCanonicalizing($oldMaintainers, array_map($callable, $record->attributes['previous_maintainers'])); + $this->assertEqualsCanonicalizing($newMaintainers, array_map($callable, $record->attributes['current_maintainers'])); + } } From 85c911b7b57377149c88138ae7a75d10928e52f6 Mon Sep 17 00:00:00 2001 From: Steven Rombauts Date: Fri, 14 Nov 2025 09:16:56 +0100 Subject: [PATCH 08/15] Clean-up #transfer-package-form CSS --- css/app.scss | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-) diff --git a/css/app.scss b/css/app.scss index 02fddc488..fabc67e96 100644 --- a/css/app.scss +++ b/css/app.scss @@ -1893,15 +1893,9 @@ body { .maintainers-collection-wrapper { margin-bottom: 15px; - label { - font-weight: bold; - margin-bottom: 10px; - } - .maintainers-list { list-style: none; padding: 0; - margin-bottom: 10px; li { display: flex; @@ -1909,22 +1903,10 @@ body { margin-bottom: 10px; align-items: center; - input[type="text"] { - flex: 1; - padding: 6px 12px; - font-size: 14px; - border: 1px solid #ccc; - border-radius: 4px; - } - .btn-danger { - flex-shrink: 0; + padding: 5px 10px; } } } - - .add-maintainer-item { - margin-top: 5px; - } } } From 3ba81348288bfa68e76956debed951643edab5c9 Mon Sep 17 00:00:00 2001 From: Steven Rombauts Date: Fri, 14 Nov 2025 09:54:54 +0100 Subject: [PATCH 09/15] Define array types with the `array` notation --- src/Model/PackageManager.php | 5 ++--- tests/Model/PackageManagerTest.php | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/Model/PackageManager.php b/src/Model/PackageManager.php index 4f5db1abd..457859452 100644 --- a/src/Model/PackageManager.php +++ b/src/Model/PackageManager.php @@ -241,9 +241,8 @@ public function notifyNewMaintainer(User $user, Package $package): bool return true; } - /* - * @param User[] $oldMaintainers - * @param User[] $newMaintainers + /** + * @param array $newMaintainers */ public function transferPackage(Package $package, array $newMaintainers): bool { diff --git a/tests/Model/PackageManagerTest.php b/tests/Model/PackageManagerTest.php index fd5f56eae..3b00c8b35 100644 --- a/tests/Model/PackageManagerTest.php +++ b/tests/Model/PackageManagerTest.php @@ -109,8 +109,8 @@ public function testTransferPackageWithSameMaintainersDoesNothing(): void } /** - * @param string[] $oldMaintainers - * @param string[] $newMaintainers + * @param array $oldMaintainers + * @param array $newMaintainers */ private function assertAuditLogWasCreated(Package $package, array $oldMaintainers, array $newMaintainers): void { From 046caa0f6104c40caf0586fd6711d112fd19efd9 Mon Sep 17 00:00:00 2001 From: Steven Rombauts Date: Fri, 14 Nov 2025 17:44:06 +0100 Subject: [PATCH 10/15] Remove `href` attributes and fix form processing --- src/Controller/PackageController.php | 42 ++++++++++++++---------- templates/package/view_package.html.twig | 8 ++--- 2 files changed, 28 insertions(+), 22 deletions(-) diff --git a/src/Controller/PackageController.php b/src/Controller/PackageController.php index 85e9ed79d..8bdf50fed 100644 --- a/src/Controller/PackageController.php +++ b/src/Controller/PackageController.php @@ -998,28 +998,34 @@ public function transferPackageAction(Request $req, #[MapEntity] Package $packag $form = $this->createTransferPackageForm($package); $form->handleRequest($req); - if ($form->isSubmitted() && $form->isValid()) { - try { - $newMaintainers = $form->getData()->getMaintainers()->toArray(); - $result = $this->packageManager->transferPackage($package, $newMaintainers); - $this->getEM()->flush(); - - if ($result) { - $usernames = array_map(fn (User $user) => $user->getUsername(), $newMaintainers); - $this->addFlash('success', sprintf('Package has been transferred to %s', implode(', ', $usernames))); - } else { - $this->addFlash('warning', 'Package maintainers are identical and have not been changed'); - } + if (!$form->isSubmitted()) { + return $this->redirectToRoute('view_package', ['name' => $package->getName()]); + } - return $this->redirectToRoute('view_package', ['name' => $package->getName()]); - } catch (\Exception $e) { - $logger->critical($e->getMessage(), ['exception', $e]); - $this->addFlash('error', 'The package could not be transferred.'); - } - } elseif (!$form->isValid()) { + if (!$form->isValid()) { foreach ($form->getErrors(true, true) as $error) { $this->addFlash('error', $error->getMessage()); } + + return $this->redirectToRoute('view_package', ['name' => $package->getName()]); + } + + try { + $newMaintainers = $form->getData()->getMaintainers()->toArray(); + $result = $this->packageManager->transferPackage($package, $newMaintainers); + $this->getEM()->flush(); + + if ($result) { + $usernames = array_map(fn (User $user) => $user->getUsername(), $newMaintainers); + $this->addFlash('success', sprintf('Package has been transferred to %s', implode(', ', $usernames))); + } else { + $this->addFlash('warning', 'Package maintainers are identical and have not been changed'); + } + + return $this->redirectToRoute('view_package', ['name' => $package->getName()]); + } catch (\Exception $e) { + $logger->critical($e->getMessage(), ['exception', $e]); + $this->addFlash('error', 'The package could not be transferred.'); } return $this->redirectToRoute('view_package', ['name' => $package->getName()]); diff --git a/templates/package/view_package.html.twig b/templates/package/view_package.html.twig index 470d2e6e6..efa031020 100644 --- a/templates/package/view_package.html.twig +++ b/templates/package/view_package.html.twig @@ -145,12 +145,12 @@ {% endfor %}

    - + {% if is_granted('remove_maintainer', package) or is_granted('add_maintainer', package) or is_granted('transfer_package', package) %}
    Maintainer actions
    - {% if is_granted('add_maintainer', package) %}{% endif %} - {% if is_granted('remove_maintainer', package) %}{% endif %} - {% if is_granted('transfer_package', package) %}{% endif %} + {% if is_granted('add_maintainer', package) %}{% endif %} + {% if is_granted('remove_maintainer', package) %}{% endif %} + {% if is_granted('transfer_package', package) %}{% endif %} {% endif %}
    Details
    From bc1baed0ba20a13d5c070749fd77c412fcf33521 Mon Sep 17 00:00:00 2001 From: Steven Rombauts Date: Tue, 9 Dec 2025 13:54:32 +0100 Subject: [PATCH 11/15] Clean-up CSS --- css/app.scss | 19 +++++-------------- templates/package/view_package.html.twig | 13 +++++++++---- 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/css/app.scss b/css/app.scss index fabc67e96..c144fcdc1 100644 --- a/css/app.scss +++ b/css/app.scss @@ -1082,20 +1082,11 @@ input:focus:invalid:focus, textarea:focus:invalid:focus, select:focus:invalid:fo margin-bottom: 4px; margin-right: 4px; } -.package .details #add-maintainer{ - margin-top: 3px; - margin-bottom: 7px; - font-size: 34px; -} -.package .details #transfer-package { - margin-top: 7px; - margin-bottom: 7px; - margin-right: 4px; - font-size: 34px; -} -.package .details #remove-maintainer { - font-size: 35px; - margin-top: 5px; +.package .details .maintainer-actions { + a { + font-size: 34px; + cursor: pointer; + } } .package .details .canonical, .package .funding p { diff --git a/templates/package/view_package.html.twig b/templates/package/view_package.html.twig index efa031020..92fa99d38 100644 --- a/templates/package/view_package.html.twig +++ b/templates/package/view_package.html.twig @@ -148,9 +148,11 @@ {% if is_granted('remove_maintainer', package) or is_granted('add_maintainer', package) or is_granted('transfer_package', package) %}
    Maintainer actions
    - {% if is_granted('add_maintainer', package) %}{% endif %} - {% if is_granted('remove_maintainer', package) %}{% endif %} - {% if is_granted('transfer_package', package) %}{% endif %} +

    + {% if is_granted('add_maintainer', package) %}{% endif %} + {% if is_granted('remove_maintainer', package) %}{% endif %} + {% if is_granted('transfer_package', package) %}{% endif %} +

    {% endif %}
    Details
    @@ -327,7 +329,10 @@ {% if is_granted('transfer_package', package) %}
    {{ form_start(transferPackageForm, { - attr: { id: 'transfer-package-form', class: 'col-sm-6 col-md-3 col-md-offset-9 ' ~ (show_transfer_package_form|default(false) ? '': 'hidden') }, + attr: { + id: 'transfer-package-form', + class: 'col-sm-6 col-md-3 col-md-offset-9 ' ~ (show_transfer_package_form|default(false) ? '': 'hidden'), + }, action: path('transfer_package', { 'name': package.name }) }) }}
    From 67d982431c97b548e634b5d8feab2526ffca0cb2 Mon Sep 17 00:00:00 2001 From: Steven Rombauts Date: Tue, 9 Dec 2025 14:17:44 +0100 Subject: [PATCH 12/15] Exclude users that are disabled --- src/Command/TransferOwnershipCommand.php | 2 +- src/Entity/UserRepository.php | 7 +++++-- src/Form/Type/MaintainerType.php | 9 +++++++++ tests/Command/TransferOwnershipCommandTest.php | 16 ++++++++++++++++ tests/Controller/PackageControllerTest.php | 4 +++- tests/Entity/UserRepositoryTest.php | 5 +++-- tests/IntegrationTestCase.php | 2 +- 7 files changed, 38 insertions(+), 7 deletions(-) diff --git a/src/Command/TransferOwnershipCommand.php b/src/Command/TransferOwnershipCommand.php index b31125ce0..b2632097e 100644 --- a/src/Command/TransferOwnershipCommand.php +++ b/src/Command/TransferOwnershipCommand.php @@ -89,7 +89,7 @@ private function queryAndValidateMaintainers(InputInterface $input, OutputInterf $usernames = array_map('mb_strtolower', $input->getArgument('maintainers')); sort($usernames); - $maintainers = $this->getEM()->getRepository(User::class)->findUsersByUsername($usernames, ['usernameCanonical' => 'ASC']); + $maintainers = $this->getEM()->getRepository(User::class)->findEnabledUsersByUsername($usernames, ['usernameCanonical' => 'ASC']); if (array_keys($maintainers) === $usernames) { return $maintainers; diff --git a/src/Entity/UserRepository.php b/src/Entity/UserRepository.php index 028d8a525..352d851c9 100644 --- a/src/Entity/UserRepository.php +++ b/src/Entity/UserRepository.php @@ -46,9 +46,12 @@ public function findOneByUsernameOrEmail(string $usernameOrEmail): ?User * @param ?array $orderBy * @return array */ - public function findUsersByUsername(array $usernames, ?array $orderBy = null): array + public function findEnabledUsersByUsername(array $usernames, ?array $orderBy = null): array { - $matches = $this->findBy(['usernameCanonical' => $usernames], $orderBy); + $matches = $this->findBy([ + 'usernameCanonical' => $usernames, + 'enabled' => true, + ], $orderBy); $users = []; foreach ($matches as $match) { diff --git a/src/Form/Type/MaintainerType.php b/src/Form/Type/MaintainerType.php index 930c9457f..db1bd8dda 100644 --- a/src/Form/Type/MaintainerType.php +++ b/src/Form/Type/MaintainerType.php @@ -53,6 +53,15 @@ function (?string $username): ?User { throw $failure; } + if (!$user->isEnabled()) { + $failure = new TransformationFailedException(sprintf('User "%s" is disabled.', $username)); + $failure->setInvalidMessage('The user "{{ value }}" is disabled.', [ + '{{ value }}' => $username, + ]); + + throw $failure; + } + return $user; } )); diff --git a/tests/Command/TransferOwnershipCommandTest.php b/tests/Command/TransferOwnershipCommandTest.php index d21946a80..deaefa774 100644 --- a/tests/Command/TransferOwnershipCommandTest.php +++ b/tests/Command/TransferOwnershipCommandTest.php @@ -132,6 +132,22 @@ public function testExecuteFailsWithUnknownMaintainers(): void $this->assertStringContainsString('2 maintainers could not be found', $output); } + public function testExecuteFailsWithDisabledMaintainers(): void + { + $tom = self::createUser('tom', 'tom@example.org', enabled: false); + $this->store($tom); + + $this->commandTester->execute([ + 'vendorOrPackage' => 'vendor1', + 'maintainers' => ['alice', 'tom'], + ]); + + $this->assertSame(Command::FAILURE, $this->commandTester->getStatusCode()); + + $output = $this->commandTester->getDisplay(); + $this->assertStringContainsString('1 maintainers could not be found', $output); + } + public function testExecuteFailsIfNoVendorPackagesFound(): void { $this->commandTester->execute([ diff --git a/tests/Controller/PackageControllerTest.php b/tests/Controller/PackageControllerTest.php index b44d064d2..7e78c1c40 100644 --- a/tests/Controller/PackageControllerTest.php +++ b/tests/Controller/PackageControllerTest.php @@ -196,12 +196,14 @@ public function testTransferPackage(): void #[TestWith(['does_not_exist', 'value is not a valid username or email'])] #[TestWith([null, 'at least one maintainer must be specified'])] + #[TestWith(['bob', 'The user "bob" is disabled'])] public function testTransferPackageReturnsValidationError(?string $value, string $message): void { $alice = self::createUser('alice', 'alice@example.org'); + $bob = self::createUser('bob', 'bob@example.org', enabled: false); $package = self::createPackage('test/pkg', 'https://example.com/test/pkg', maintainers: [$alice]); - $this->store($alice, $package); + $this->store($alice, $bob, $package); $this->client->loginUser($alice); diff --git a/tests/Entity/UserRepositoryTest.php b/tests/Entity/UserRepositoryTest.php index 2e32eee7d..32119d6b2 100644 --- a/tests/Entity/UserRepositoryTest.php +++ b/tests/Entity/UserRepositoryTest.php @@ -31,9 +31,10 @@ public function testFindUsersByUsernameWithMultipleValidUsernames(): void $alice = self::createUser('Alice', 'alice@example.org'); $bob = self::createUser('Bob', 'bob@example.org'); $charlie = self::createUser('Charlie', 'charlie@example.org'); - $this->store($alice, $bob, $charlie); + $john = self::createUser('John', 'john@example.org', enabled: false); + $this->store($alice, $bob, $charlie, $john); - $result = $this->userRepository->findUsersByUsername(['alice', 'bob']); + $result = $this->userRepository->findEnabledUsersByUsername(['alice', 'bob', 'john']); $this->assertCount(2, $result); diff --git a/tests/IntegrationTestCase.php b/tests/IntegrationTestCase.php index 05fd5eb1d..e80bfe9eb 100644 --- a/tests/IntegrationTestCase.php +++ b/tests/IntegrationTestCase.php @@ -104,7 +104,7 @@ protected static function createPackage(string $name, string $repository, ?strin protected static function createUser(string $username = 'test', string $email = 'test@example.org', string $password = 'testtest', string $apiToken = 'api-token', string $safeApiToken = 'safe-api-token', string $githubId = '12345', bool $enabled = true, array $roles = []): User { $user = new User(); - $user->setEnabled(true); + $user->setEnabled($enabled); $user->setUsername($username); $user->setEmail($email); $user->setPassword($password); From f3694d767bfd76fc41ff25ece69cc6eb8311d7d0 Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Wed, 10 Dec 2025 16:40:40 +0100 Subject: [PATCH 13/15] Minor tweaks/cleanups --- src/Controller/PackageController.php | 5 ++-- src/Form/Model/TransferPackageRequest.php | 28 ++++------------------- 2 files changed, 7 insertions(+), 26 deletions(-) diff --git a/src/Controller/PackageController.php b/src/Controller/PackageController.php index 8bdf50fed..d6f382379 100644 --- a/src/Controller/PackageController.php +++ b/src/Controller/PackageController.php @@ -990,7 +990,7 @@ public function removeMaintainerAction(Request $req, #[MapEntity] Package $packa } - #[Route(path: '/packages/{name:package}/transfer/', name: 'transfer_package', requirements: ['name' => Package::PACKAGE_NAME_REGEX])] + #[Route(path: '/packages/{name:package}/transfer/', name: 'transfer_package', requirements: ['name' => Package::PACKAGE_NAME_REGEX], methods: ['GET', 'POST'])] public function transferPackageAction(Request $req, #[MapEntity] Package $package, #[CurrentUser] User $user, LoggerInterface $logger): RedirectResponse { $this->denyAccessUnlessGranted(PackageActions::TransferPackage->value, $package); @@ -1673,8 +1673,7 @@ private function createRemoveMaintainerForm(Package $package): FormInterface */ private function createTransferPackageForm(Package $package): FormInterface { - $transferRequest = new TransferPackageRequest(); - $transferRequest->setMaintainers(clone $package->getMaintainers()); + $transferRequest = new TransferPackageRequest(clone $package->getMaintainers()); return $this->createForm(TransferPackageRequestType::class, $transferRequest); } diff --git a/src/Form/Model/TransferPackageRequest.php b/src/Form/Model/TransferPackageRequest.php index 747debb94..71fb028ee 100644 --- a/src/Form/Model/TransferPackageRequest.php +++ b/src/Form/Model/TransferPackageRequest.php @@ -19,28 +19,10 @@ class TransferPackageRequest { - /** @var Collection */ - #[TransferPackageValidMaintainersList] - private Collection $maintainers; - - public function __construct() - { - $this->maintainers = new ArrayCollection(); - } - - /** - * @return Collection - */ - public function getMaintainers(): Collection - { - return $this->maintainers; - } - - /** - * @param Collection $maintainers - */ - public function setMaintainers(Collection $maintainers): void - { - $this->maintainers = $maintainers; + public function __construct( + /** @var Collection */ + #[TransferPackageValidMaintainersList] + public Collection $maintainers = new ArrayCollection(), + ) { } } From 844ad18a5010fb9fa0ace5697ef32f515f775021 Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Wed, 10 Dec 2025 17:01:25 +0100 Subject: [PATCH 14/15] Fix usage, make regex more possessive to avoid router issues with package names containing underscores --- src/Controller/PackageController.php | 2 +- src/Entity/Package.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Controller/PackageController.php b/src/Controller/PackageController.php index d6f382379..db9cc3993 100644 --- a/src/Controller/PackageController.php +++ b/src/Controller/PackageController.php @@ -1011,7 +1011,7 @@ public function transferPackageAction(Request $req, #[MapEntity] Package $packag } try { - $newMaintainers = $form->getData()->getMaintainers()->toArray(); + $newMaintainers = $form->getData()->maintainers->toArray(); $result = $this->packageManager->transferPackage($package, $newMaintainers); $this->getEM()->flush(); diff --git a/src/Entity/Package.php b/src/Entity/Package.php index d4a5c78b4..c96c27c0e 100644 --- a/src/Entity/Package.php +++ b/src/Entity/Package.php @@ -90,7 +90,7 @@ class Package public const AUTO_MANUAL_HOOK = 1; public const AUTO_GITHUB_HOOK = 2; - public const string PACKAGE_NAME_REGEX = '[a-zA-Z0-9](?:[_.-]?[a-zA-Z0-9]+)*/[a-zA-Z0-9](?:[_.-]?[a-zA-Z0-9]+)*'; + public const string PACKAGE_NAME_REGEX = '[a-zA-Z0-9]++(?:[_.-]?[a-zA-Z0-9]++)*+/[a-zA-Z0-9]++(?:[_.-]?[a-zA-Z0-9]++)*+'; #[ORM\Id] #[ORM\Column(type: 'integer')] From 36229601d69d61da98267596a558c3c60b4f89df Mon Sep 17 00:00:00 2001 From: Steven Rombauts Date: Wed, 10 Dec 2025 17:41:23 +0100 Subject: [PATCH 15/15] Reuse `findEnabledUsersByUsername()` in `MaintainerType` too --- src/Form/Type/MaintainerType.php | 18 +++++------------- tests/Controller/PackageControllerTest.php | 5 ++--- 2 files changed, 7 insertions(+), 16 deletions(-) diff --git a/src/Form/Type/MaintainerType.php b/src/Form/Type/MaintainerType.php index db1bd8dda..9a7cbe279 100644 --- a/src/Form/Type/MaintainerType.php +++ b/src/Form/Type/MaintainerType.php @@ -42,27 +42,19 @@ function (?string $username): ?User { return null; } - $user = $this->em->getRepository(User::class)->findOneByUsernameOrEmail($username); + $username = mb_strtolower($username); + $users = $this->em->getRepository(User::class)->findEnabledUsersByUsername([$username]); - if ($user === null) { + if (!count($users) || !array_key_exists($username, $users)) { $failure = new TransformationFailedException(sprintf('User "%s" does not exist.', $username)); - $failure->setInvalidMessage('The given "{{ value }}" value is not a valid username or email.', [ + $failure->setInvalidMessage('The given "{{ value }}" value is not a valid username.', [ '{{ value }}' => $username, ]); throw $failure; } - if (!$user->isEnabled()) { - $failure = new TransformationFailedException(sprintf('User "%s" is disabled.', $username)); - $failure->setInvalidMessage('The user "{{ value }}" is disabled.', [ - '{{ value }}' => $username, - ]); - - throw $failure; - } - - return $user; + return $users[$username]; } )); } diff --git a/tests/Controller/PackageControllerTest.php b/tests/Controller/PackageControllerTest.php index 7e78c1c40..ae44aa960 100644 --- a/tests/Controller/PackageControllerTest.php +++ b/tests/Controller/PackageControllerTest.php @@ -166,7 +166,7 @@ public function testTransferPackage(): void $form = $crawler->filter('[name="transfer_package_form"]')->form(); $form->setValues([ 'transfer_package_form[maintainers][0]' => 'alice', - 'transfer_package_form[maintainers][1]' => 'bob@example.org', + 'transfer_package_form[maintainers][1]' => 'bob', ]); $this->client->submit($form); @@ -194,9 +194,8 @@ public function testTransferPackage(): void $this->assertNotNull($auditRecord, 'Audit record not found'); } - #[TestWith(['does_not_exist', 'value is not a valid username or email'])] + #[TestWith(['does_not_exist', 'value is not a valid username'])] #[TestWith([null, 'at least one maintainer must be specified'])] - #[TestWith(['bob', 'The user "bob" is disabled'])] public function testTransferPackageReturnsValidationError(?string $value, string $message): void { $alice = self::createUser('alice', 'alice@example.org');