diff --git a/css/app.scss b/css/app.scss index ebd46c0e3..ef9131739 100644 --- a/css/app.scss +++ b/css/app.scss @@ -1082,16 +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; - float: right; - font-size: 34px; -} -.package .details #remove-maintainer { - float: right; - font-size: 35px; - margin-top: 5px; +.package .details .maintainer-actions { + a { + font-size: 34px; + cursor: pointer; + } } .package .details .canonical, .package .funding p { @@ -1896,3 +1891,25 @@ body { } } } + +#transfer-package-form { + .maintainers-collection-wrapper { + margin-bottom: 15px; + + .maintainers-list { + list-style: none; + padding: 0; + + li { + display: flex; + gap: 10px; + margin-bottom: 10px; + align-items: center; + + .btn-danger { + padding: 5px 10px; + } + } + } + } +} 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/Command/TransferOwnershipCommand.php b/src/Command/TransferOwnershipCommand.php index c1cb01146..feb9ac50c 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(); @@ -87,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; @@ -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, false); } $this->doctrine->getManager()->flush(); 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 f7e18ac87..2f6e0ce8f 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; @@ -666,6 +668,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'] = []; @@ -801,7 +804,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 { @@ -853,7 +856,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); @@ -878,7 +881,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); @@ -916,7 +919,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); @@ -960,6 +963,48 @@ public function removeMaintainerAction(Request $req, #[MapEntity] Package $packa ]); } + + #[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); + + $form = $this->createTransferPackageForm($package); + $form->handleRequest($req); + + if (!$form->isSubmitted()) { + return $this->redirectToRoute('view_package', ['name' => $package->getName()]); + } + + 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()->maintainers->toArray(); + $result = $this->packageManager->transferPackage($package, $newMaintainers, true); + $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()]); + } + #[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 { @@ -1608,6 +1653,16 @@ private function createRemoveMaintainerForm(Package $package): FormInterface ]); } + /** + * @return FormInterface + */ + private function createTransferPackageForm(Package $package): FormInterface + { + $transferRequest = new TransferPackageRequest(clone $package->getMaintainers()); + + return $this->createForm(TransferPackageRequestType::class, $transferRequest); + } + /** * @return FormInterface */ diff --git a/src/Entity/Package.php b/src/Entity/Package.php index 29f68f57c..c96c27c0e 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/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/Model/TransferPackageRequest.php b/src/Form/Model/TransferPackageRequest.php new file mode 100644 index 000000000..71fb028ee --- /dev/null +++ b/src/Form/Model/TransferPackageRequest.php @@ -0,0 +1,28 @@ + + * 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 App\Validator\Constraints\TransferPackageValidMaintainersList; +use Doctrine\Common\Collections\ArrayCollection; +use Doctrine\Common\Collections\Collection; + +class TransferPackageRequest +{ + public function __construct( + /** @var Collection */ + #[TransferPackageValidMaintainersList] + public Collection $maintainers = new ArrayCollection(), + ) { + } +} diff --git a/src/Form/Type/MaintainerType.php b/src/Form/Type/MaintainerType.php new file mode 100644 index 000000000..9a7cbe279 --- /dev/null +++ b/src/Form/Type/MaintainerType.php @@ -0,0 +1,75 @@ + + * 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\CallbackTransformer; +use Symfony\Component\Form\Exception\TransformationFailedException; +use Symfony\Component\Form\Extension\Core\Type\TextType; +use Symfony\Component\Form\FormBuilderInterface; +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; + } + + $username = mb_strtolower($username); + $users = $this->em->getRepository(User::class)->findEnabledUsersByUsername([$username]); + + 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.', [ + '{{ value }}' => $username, + ]); + + throw $failure; + } + + return $users[$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'; + } +} diff --git a/src/Model/PackageManager.php b/src/Model/PackageManager.php index 57ca9e98c..5ebec63bf 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,35 @@ public function notifyNewMaintainer(User $user, Package $package): bool return true; } + + /** + * @param array $newMaintainers + */ + public function transferPackage(Package $package, array $newMaintainers, bool $notifyNewMaintainers): bool + { + $oldMaintainers = $package->getMaintainers()->toArray(); + $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) { + $isNewMaintainer = !in_array($maintainer->getId(), $normalizedOldMaintainers); + $package->addMaintainer($maintainer); + + if ($notifyNewMaintainers && $isNewMaintainer) { + $this->notifyNewMaintainer($maintainer, $package); + } + } + + $this->doctrine->getManager()->persist(AuditRecord::packageTransferred($package, null, $oldMaintainers, $newMaintainers)); + + return true; + } } 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'), }; 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(); + } +} 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; diff --git a/templates/package/view_package.html.twig b/templates/package/view_package.html.twig index 04868460e..92fa99d38 100644 --- a/templates/package/view_package.html.twig +++ b/templates/package/view_package.html.twig @@ -144,10 +144,17 @@ {% for maintainer in package.maintainers -%} {% endfor %} - {% if is_granted('remove_maintainer', package) %}{% endif %} - {% if is_granted('add_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 %}

    @@ -319,6 +326,36 @@ {% 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 %}
    diff --git a/tests/Command/TransferOwnershipCommandTest.php b/tests/Command/TransferOwnershipCommandTest.php index ef167fc6d..ea5d8db26 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,7 @@ 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']); + $this->assertEmailCount(0); } public function testExecuteSuccessForPackage(): void @@ -100,7 +96,6 @@ public function testExecuteSuccessForPackage(): void $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,44 +121,33 @@ public function testExecuteWithDryRunDoesNothing(): void $this->assertEqualsCanonicalizing(['john', 'alice'], array_map($callable, $package1->getMaintainers()->toArray())); } - public function testExecuteIgnoresIdenticalMaintainers(): void + public function testExecuteFailsWithUnknownMaintainers(): void { $this->commandTester->execute([ 'vendorOrPackage' => 'vendor1', - 'maintainers' => ['alice', 'john'], + 'maintainers' => ['unknown1', 'alice', 'unknown2'], ]); - $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())); + $this->assertSame(Command::FAILURE, $this->commandTester->getStatusCode()); - $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']); + $output = $this->commandTester->getDisplay(); + $this->assertStringContainsString('2 maintainers could not be found', $output); } - public function testExecuteFailsWithUnknownMaintainers(): void + public function testExecuteFailsWithDisabledMaintainers(): void { + $tom = self::createUser('tom', 'tom@example.org', enabled: false); + $this->store($tom); + $this->commandTester->execute([ 'vendorOrPackage' => 'vendor1', - 'maintainers' => ['unknown1', 'alice', 'unknown2'], + 'maintainers' => ['alice', 'tom'], ]); $this->assertSame(Command::FAILURE, $this->commandTester->getStatusCode()); $output = $this->commandTester->getDisplay(); - $this->assertStringContainsString('2 maintainers could not be found', $output); + $this->assertStringContainsString('1 maintainers could not be found', $output); } public function testExecuteFailsIfNoVendorPackagesFound(): void @@ -178,29 +162,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..17fc9c3a8 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,90 @@ 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', + ]); + + $this->client->enableProfiler(); // This is required in 7.3.4 to assert emails were sent, see https://github.com/symfony/symfony/issues/61873 + $this->client->submit($form); + + $this->assertResponseRedirects('/packages/test/pkg'); + + $this->assertEmailCount(1); + $email = $this->getMailerMessage(); + $this->assertNotNull($email); + $this->assertEmailHeaderSame($email, 'To', $bob->getEmail()); + + $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'])] + #[TestWith([null, 'at least one maintainer must be specified'])] + 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, $bob, $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/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); diff --git a/tests/Model/PackageManagerTest.php b/tests/Model/PackageManagerTest.php index acfa944e7..e4fd5cf43 100644 --- a/tests/Model/PackageManagerTest.php +++ b/tests/Model/PackageManagerTest.php @@ -12,11 +12,25 @@ 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; +use PHPUnit\Framework\Attributes\TestWith; -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 +60,75 @@ public function testNotifyFailure(): void $client->request('POST', '/api/github?username=test&apiToken=token', ['payload' => $payload]); $this->assertEquals(202, $client->getResponse()->getStatusCode()); } + + #[TestWith([false, 0])] + #[TestWith([true, 1])] + public function testTransferPackageReplacesAllMaintainers(bool $notifyNewMaintainers, int $expectedEmailCount): 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], $notifyNewMaintainers); + + $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']); + $this->assertEmailCount($expectedEmailCount); + } + + 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], false); + + $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 array $oldMaintainers + * @param array $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'])); + } }