Skip to content

feat: add package version delete api #1533

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add package version delete api #1533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 33 additions & 2 deletions src/Controller/ApiController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use App\Entity\SecurityAdvisory;
use App\Entity\User;
use App\Entity\Vendor;
use App\Entity\Version;
use App\Model\DownloadManager;
use App\Model\ProviderManager;
use App\Model\VersionIdCache;
Expand All @@ -24,6 +25,7 @@
use App\Service\Scheduler;
use App\Util\UserAgentParser;
use Composer\Pcre\Preg;
use Symfony\Bridge\Doctrine\Attribute\MapEntity;
use Symfony\Component\HttpFoundation\BinaryFileResponse;
use Symfony\Component\HttpFoundation\JsonResponse;
use Symfony\Component\HttpFoundation\Request;
Expand Down Expand Up @@ -183,14 +185,14 @@ public function updatePackageAction(Request $request, string $githubWebhookSecre
}

#[Route(path: '/api/packages/{package}', name: 'api_edit_package', requirements: ['package' => '[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+?'], defaults: ['_format' => 'json'], methods: ['PUT'])]
public function editPackageAction(Request $request, Package $package, ValidatorInterface $validator, StatsDClient $statsd): JsonResponse
public function editPackageAction(Request $request, #[MapEntity(mapping: ['package' => 'name'])] Package $package, ValidatorInterface $validator, StatsDClient $statsd): JsonResponse
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks like a different change than adding a new API endpoint.

{
$user = $this->findUser($request);
if (!$user) {
return new JsonResponse(['status' => 'error', 'message' => 'Missing or invalid username/apiToken in request'], 406);
}
if (!$package->getMaintainers()->contains($user)) {
throw new AccessDeniedException;
return new JsonResponse(['status' => 'error', 'message' => 'You are not allowed to edit this package'], 403);
}

$statsd->increment('edit_package_api');
Expand Down Expand Up @@ -225,6 +227,35 @@ public function editPackageAction(Request $request, Package $package, ValidatorI
return new JsonResponse(['status' => 'success'], 200);
}

#[Route(path: '/api/packages/{package}/delete-version/{version}', name: 'api_delete_package_version', requirements: ['package' => '[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+?'], defaults: ['_format' => 'json'], methods: ['POST'])]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think should should be a path /api/packages/{package}/versions/{version} instead, using the DELETE method

public function deletePackageVersionAction(Request $request, #[MapEntity(mapping: ['package' => 'name'])] Package $package, string $version, StatsDClient $statsd): JsonResponse
{
$user = $this->findUser($request);
if (!$user) {
return new JsonResponse(['status' => 'error', 'message' => 'Missing or invalid username/apiToken in request'], 406);
}
if (!$package->getMaintainers()->contains($user)) {
return new JsonResponse(['status' => 'error', 'message' => 'You are not allowed to delete this package version'], 403);
}

$statsd->increment('delete_package_version_api');

$packageVersion = $package->getVersions()->findFirst(static function (int $index, Version $innerVersion) use ($version) {
return $innerVersion->getVersion() === $version;
});

if (!$packageVersion) {
return new JsonResponse(['status' => 'error', 'message' => 'Version not found'], 404);
}

$repo = $this->getEM()->getRepository(Version::class);
$repo->remove($packageVersion);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be wrapped in $em->transactional() (which calls flush() automatically at the end) or $em->getConnection()->transactional() (keeping an explicit flush of the entity manager inside the closure) to ensure that the removal is done in a transactional way.

$this->getEM()->flush();
$this->getEM()->clear();

return new JsonResponse(['status' => 'success'], 200);
}

#[Route(path: '/jobs/{id}', name: 'get_job', requirements: ['id' => '[a-f0-9]+'], defaults: ['_format' => 'json'], methods: ['GET'])]
public function getJobAction(string $id, StatsDClient $statsd): JsonResponse
{
Expand Down
25 changes: 22 additions & 3 deletions templates/api_doc/index.html.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -523,16 +523,35 @@ PUT https://{{ packagist_host }}/api/packages/[package name]?username=[username]
<p>This endpoint is considered SAFE and allows either your main or safe <a href="{{ path('my_profile') }}">API token</a> to be used.</p>

<pre>
POST https://{{ packagist_host }}/api/update-package?username=[username]&amp;apiToken=[apiToken] {"repository":"[url]"}
PUT https://{{ packagist_host }}/api/update-package?username=[username]&amp;apiToken=[apiToken] {"repository":"[url]"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong. This endpoints receiving a webhook to trigger an update of the method of this package expects the POST method, not PUT

<code>
{
"status": "success",
"jobs": ["job-id", ".."]
}
</code></pre>
<p>Working examples:<br/>
<code>curl -X POST -H'Content-Type:application/json' 'https://{{ packagist_host }}/api/update-package?username={{ app.user.username|default('USERNAME') }}&amp;apiToken=********' -d '{"repository":"https://github.com/Seldaek/monolog"}'</code><br/>
<code>curl -X POST -H'Content-Type:application/json' 'https://{{ packagist_host }}/api/update-package?username={{ app.user.username|default('USERNAME') }}&amp;apiToken=********' -d '{"repository":"https://packagist.org/monolog/monolog"}'</code>
<code>curl -X PUT -H'Content-Type:application/json' 'https://{{ packagist_host }}/api/update-package?username={{ app.user.username|default('USERNAME') }}&amp;apiToken=********' -d '{"repository":"https://github.com/Seldaek/monolog"}'</code><br/>
<code>curl -X PUT -H'Content-Type:application/json' 'https://{{ packagist_host }}/api/update-package?username={{ app.user.username|default('USERNAME') }}&amp;apiToken=********' -d '{"repository":"https://packagist.org/monolog/monolog"}'</code>
</p>

</section>

<section class="col-d-12">
<h3 id="delete-package-version">Delete a package version</h3>

<p>This endpoint deletes a package version by package name and version. Parameters <code>username</code> and <code>apiToken</code> are required. Only the <code>POST</code> method is allowed. The <code>content-type: application/json</code> header is required.</p>
<p>This endpoint is considered SAFE and allows either your main or safe <a href="{{ path('my_profile') }}">API token</a> to be used.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be considered safe ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No definitely not.


<pre>
POST https://{{ packagist_host }}/api/packages/[package name]/delete-version/[version]?username=[username]&amp;apiToken=[apiToken]
<code>
{
"status": "success"
}
</code></pre>
<p>Working examples:<br/>
<code>curl -X POST 'https://{{ packagist_host }}/api/packages/monolog/monolog/delete-version/1.0.0?username={{ app.user.username|default('USERNAME') }}&amp;apiToken=********'</code><br/>
</p>

</section>
Expand Down
28 changes: 28 additions & 0 deletions tests/Controller/ApiControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use App\Entity\Package;
use App\Entity\SecurityAdvisory;
use App\Entity\User;
use App\Entity\Version;
use App\SecurityAdvisory\GitHubSecurityAdvisoriesSource;
use App\SecurityAdvisory\RemoteSecurityAdvisory;
use App\SecurityAdvisory\Severity;
Expand Down Expand Up @@ -181,4 +182,31 @@ public function testSecurityAdvisories(): void
$this->assertArrayHasKey('acme/package', $content['advisories']);
$this->assertCount(1, $content['advisories']['acme/package']);
}

public function testDeletePackageVersion(): void
{
$url = 'https://github.com/composer/composer';
$user = self::createUser();
$package = self::createPackage('test/'.bin2hex(random_bytes(10)), $url, maintainers: [$user]);

$packageVersion = new Version();
$packageVersion->setName($package->getName());
$packageVersion->setVersion('1.0.0');
$packageVersion->setNormalizedVersion('1.0.0.0');
$packageVersion->setDevelopment(false);
$packageVersion->setLicense(['MIT']);
$packageVersion->setAutoload([]);
$packageVersion->setSource([]);
$packageVersion->setDist([]);
$packageVersion->setPackage($package);

$package->addVersion($packageVersion);
$this->store($user, $package, $packageVersion);

$this->client->request('POST', '/api/packages/'.$package->getName().'/delete-version/1.0.0', ['username' => $user->getUsername(), 'apiToken' => 'api-token']);
$this->assertEquals(200, $this->client->getResponse()->getStatusCode(), $this->client->getResponse()->getContent());

$this->client->request('POST', '/api/packages/'.$package->getName().'/delete-version/1.0.0', ['username' => $user->getUsername(), 'apiToken' => 'api-token']);
$this->assertEquals(404, $this->client->getResponse()->getStatusCode(), $this->client->getResponse()->getContent());
}
}