Skip to content

Commit

Permalink
Improve error messages when a pending Trusted Publisher's project nam…
Browse files Browse the repository at this point in the history 
…e already exists (#17405)

* Improve error messages when creating projects and publishers

This change improves the error messages when uploading a new project
or creating a pending Trusted Publisher, when the new project's name
already exists or is too similar to an existing project.

Signed-off-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>

* Fix test error

Signed-off-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>

* Fix name checking

Fix the check for projects with too-similar names when
there is more than one existing project with the same
ultranormalized name.

Signed-off-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>

* Remove similar name from error messages

Signed-off-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>

* Move errors back to interfaces.py

Signed-off-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>

* De-parametrize tests

Signed-off-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>

* Use exceptions instead of errors

Also change the TooSimilar error to store the conflicting
project name instead of a reference to the entire project.

Signed-off-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>

* Address review comments

Signed-off-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>

---------

Signed-off-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>
  • Loading branch information
@facutuesca
facutuesca authored Feb 6, 2025
1 parent 0e7e85f commit 4775901
Show file tree
Hide file tree
Showing 15 changed files with 384 additions and 150 deletions.
11 changes: 11 additions & 0 deletions tests/unit/accounts/test_views.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3358,6 +3358,7 @@ def find_service(iface, name=None, context=None):
find_service=pretend.call_recorder(find_service),
route_url=pretend.stub(),
POST=MultiDict(),
user=pretend.stub(id=pretend.stub()),
registry=pretend.stub(
settings={
"github.token": "fake-api-token",
Expand Down Expand Up @@ -3526,13 +3527,15 @@ def test_manage_publishing(self, metrics, monkeypatch):
api_token="fake-api-token",
route_url=route_url,
check_project_name=project_service.check_project_name,
user=request.user,
)
]
assert pending_gitlab_publisher_form_cls.calls == [
pretend.call(
request.POST,
route_url=route_url,
check_project_name=project_service.check_project_name,
user=request.user,
)
]

Expand Down Expand Up @@ -3621,13 +3624,15 @@ def test_manage_publishing_admin_disabled(self, monkeypatch, pyramid_request):
api_token="fake-api-token",
route_url=pyramid_request.route_url,
check_project_name=project_service.check_project_name,
user=pyramid_request.user,
)
]
assert pending_gitlab_publisher_form_cls.calls == [
pretend.call(
pyramid_request.POST,
route_url=pyramid_request.route_url,
check_project_name=project_service.check_project_name,
user=pyramid_request.user,
)
]

Expand Down Expand Up @@ -3753,13 +3758,15 @@ def test_add_pending_oidc_publisher_admin_disabled(
api_token="fake-api-token",
route_url=pyramid_request.route_url,
check_project_name=project_service.check_project_name,
user=pyramid_request.user,
)
]
assert pending_gitlab_publisher_form_cls.calls == [
pretend.call(
pyramid_request.POST,
route_url=pyramid_request.route_url,
check_project_name=project_service.check_project_name,
user=pyramid_request.user,
)
]

Expand Down Expand Up @@ -3897,13 +3904,15 @@ def test_add_pending_oidc_publisher_user_cannot_register(
api_token="fake-api-token",
route_url=pyramid_request.route_url,
check_project_name=project_service.check_project_name,
user=pyramid_request.user,
)
]
assert pending_gitlab_publisher_form_cls.calls == [
pretend.call(
pyramid_request.POST,
route_url=pyramid_request.route_url,
check_project_name=project_service.check_project_name,
user=pyramid_request.user,
)
]

Expand Down Expand Up @@ -4618,13 +4627,15 @@ def test_delete_pending_oidc_publisher_admin_disabled(
api_token="fake-api-token",
route_url=pyramid_request.route_url,
check_project_name=project_service.check_project_name,
user=pyramid_request.user,
)
]
assert pending_gitlab_publisher_form_cls.calls == [
pretend.call(
pyramid_request.POST,
route_url=pyramid_request.route_url,
check_project_name=project_service.check_project_name,
user=pyramid_request.user,
)
]

Expand Down
37 changes: 34 additions & 3 deletions tests/unit/oidc/forms/test_activestate.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
from webob.multidict import MultiDict

from warehouse.oidc.forms import activestate
from warehouse.packaging.interfaces import ProjectNameUnavailableReason
from warehouse.packaging.interfaces import ProjectNameUnavailableExistingError

fake_username = "some-username"
fake_org_name = "some-org"
Expand Down Expand Up @@ -50,6 +50,7 @@ def check_project_name(name):
MultiDict(data),
route_url=route_url,
check_project_name=check_project_name,
user=pretend.stub(),
)

# Test built-in validations
Expand All @@ -61,20 +62,28 @@ def check_project_name(name):
assert form._route_url == route_url
assert form.validate()

def test_validate_project_name_already_in_use(self, pyramid_config):
def test_validate_project_name_already_in_use_owner(self, pyramid_config):
route_url = pretend.call_recorder(lambda *args, **kwargs: "")
user = pretend.stub()
owners = [user]

def check_project_name(name):
return ProjectNameUnavailableReason.AlreadyExists
return ProjectNameUnavailableExistingError(
existing_project=pretend.stub(owners=owners)
)

form = activestate.PendingActiveStatePublisherForm(
route_url=route_url,
check_project_name=check_project_name,
user=user,
)

field = pretend.stub(data="some-project")
with pytest.raises(wtforms.validators.ValidationError):
form.validate_project_name(field)

# The project settings URL is only shown in the error message if
# the user is the owner of the project
assert route_url.calls == [
pretend.call(
"manage.project.settings.publishing",
Expand All @@ -83,6 +92,28 @@ def check_project_name(name):
)
]

def test_validate_project_name_already_in_use_not_owner(self, pyramid_config):
route_url = pretend.call_recorder(lambda *args, **kwargs: "")
user = pretend.stub()
owners = []

def check_project_name(name):
return ProjectNameUnavailableExistingError(
existing_project=pretend.stub(owners=owners)
)

form = activestate.PendingActiveStatePublisherForm(
route_url=route_url,
check_project_name=check_project_name,
user=user,
)

field = pretend.stub(data="some-project")
with pytest.raises(wtforms.validators.ValidationError):
form.validate_project_name(field)

assert route_url.calls == []


class TestActiveStatePublisherForm:
def test_validate(self, monkeypatch):
Expand Down
55 changes: 51 additions & 4 deletions tests/unit/oidc/forms/test_github.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,19 @@
from webob.multidict import MultiDict

from warehouse.oidc.forms import github
from warehouse.packaging.interfaces import ProjectNameUnavailableReason
from warehouse.packaging.interfaces import (
ProjectNameUnavailableExistingError,
ProjectNameUnavailableInvalidError,
ProjectNameUnavailableProhibitedError,
ProjectNameUnavailableSimilarError,
ProjectNameUnavailableStdlibError,
)


class TestPendingGitHubPublisherForm:
def test_validate(self, monkeypatch):
route_url = pretend.stub()
user = pretend.stub()

def check_project_name(name):
return None # Name is available.
Expand All @@ -41,6 +48,7 @@ def check_project_name(name):
api_token=pretend.stub(),
route_url=route_url,
check_project_name=check_project_name,
user=user,
)

# We're testing only the basic validation here.
Expand All @@ -49,20 +57,29 @@ def check_project_name(name):

assert form._check_project_name == check_project_name
assert form._route_url == route_url
assert form._user == user
assert form.validate()

def test_validate_project_name_already_in_use(self, pyramid_config):
def test_validate_project_name_already_in_use_owner(self, pyramid_config):
route_url = pretend.call_recorder(lambda *args, **kwargs: "")
user = pretend.stub()
owners = [user]

form = github.PendingGitHubPublisherForm(
api_token="fake-token",
route_url=route_url,
check_project_name=lambda name: ProjectNameUnavailableReason.AlreadyExists,
check_project_name=lambda name: ProjectNameUnavailableExistingError(
existing_project=pretend.stub(owners=owners)
),
user=user,
)

field = pretend.stub(data="some-project")
with pytest.raises(wtforms.validators.ValidationError):
form.validate_project_name(field)

# The project settings URL is only shown in the error message if
# the user is the owner of the project
assert route_url.calls == [
pretend.call(
"manage.project.settings.publishing",
Expand All @@ -71,12 +88,42 @@ def test_validate_project_name_already_in_use(self, pyramid_config):
)
]

@pytest.mark.parametrize("reason", list(ProjectNameUnavailableReason))
def test_validate_project_name_already_in_use_not_owner(self, pyramid_config):
route_url = pretend.call_recorder(lambda *args, **kwargs: "")
user = pretend.stub()
owners = []

form = github.PendingGitHubPublisherForm(
api_token="fake-token",
route_url=route_url,
check_project_name=lambda name: ProjectNameUnavailableExistingError(
existing_project=pretend.stub(owners=owners)
),
user=user,
)

field = pretend.stub(data="some-project")
with pytest.raises(wtforms.validators.ValidationError):
form.validate_project_name(field)

assert route_url.calls == []

@pytest.mark.parametrize(
"reason",
[
ProjectNameUnavailableExistingError(pretend.stub(owners=[pretend.stub()])),
ProjectNameUnavailableInvalidError(),
ProjectNameUnavailableStdlibError(),
ProjectNameUnavailableProhibitedError(),
ProjectNameUnavailableSimilarError(similar_project_name="pkg_name"),
],
)
def test_validate_project_name_unavailable(self, reason, pyramid_config):
form = github.PendingGitHubPublisherForm(
api_token="fake-token",
route_url=pretend.call_recorder(lambda *args, **kwargs: ""),
check_project_name=lambda name: reason,
user=pretend.stub(),
)

field = pretend.stub(data="some-project")
Expand Down
42 changes: 36 additions & 6 deletions tests/unit/oidc/forms/test_gitlab.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,13 @@
from webob.multidict import MultiDict

from warehouse.oidc.forms import gitlab
from warehouse.packaging.interfaces import ProjectNameUnavailableReason
from warehouse.packaging.interfaces import ProjectNameUnavailableExistingError


class TestPendingGitLabPublisherForm:
def test_validate(self, monkeypatch):
route_url = pretend.stub()
user = pretend.stub()

def check_project_name(name):
return None # Name is available.
Expand All @@ -36,25 +37,37 @@ def check_project_name(name):
}
)
form = gitlab.PendingGitLabPublisherForm(
MultiDict(data), route_url=route_url, check_project_name=check_project_name
MultiDict(data),
route_url=route_url,
check_project_name=check_project_name,
user=user,
)

assert form._route_url == route_url
assert form._check_project_name == check_project_name
assert form._user == user
# We're testing only the basic validation here.
assert form.validate()

def test_validate_project_name_already_in_use(self, pyramid_config):
def test_validate_project_name_already_in_use_owner(self, pyramid_config):
user = pretend.stub()
owners = [user]
route_url = pretend.call_recorder(lambda *args, **kwargs: "my_url")

form = gitlab.PendingGitLabPublisherForm(
route_url=route_url,
check_project_name=lambda name: ProjectNameUnavailableReason.AlreadyExists,
check_project_name=lambda name: ProjectNameUnavailableExistingError(
existing_project=pretend.stub(owners=owners)
),
user=user,
)

field = pretend.stub(data="some-project")
with pytest.raises(wtforms.validators.ValidationError):
form.validate_project_name(field)

# The project settings URL is only shown in the error message if
# the user is the owner of the project
assert route_url.calls == [
pretend.call(
"manage.project.settings.publishing",
Expand All @@ -63,6 +76,25 @@ def test_validate_project_name_already_in_use(self, pyramid_config):
)
]

def test_validate_project_name_already_in_use_not_owner(self, pyramid_config):
user = pretend.stub()
owners = []
route_url = pretend.call_recorder(lambda *args, **kwargs: "my_url")

form = gitlab.PendingGitLabPublisherForm(
route_url=route_url,
check_project_name=lambda name: ProjectNameUnavailableExistingError(
existing_project=pretend.stub(owners=owners)
),
user=user,
)

field = pretend.stub(data="some-project")
with pytest.raises(wtforms.validators.ValidationError):
form.validate_project_name(field)

assert route_url.calls == []


class TestGitLabPublisherForm:
@pytest.mark.parametrize(
Expand Down Expand Up @@ -138,7 +170,6 @@ def test_validate_basic_invalid_fields(self, monkeypatch, data):
["invalid.git", "invalid.atom", "invalid--project"],
)
def test_reserved_project_names(self, project_name):

data = MultiDict(
{
"namespace": "some",
Expand All @@ -160,7 +191,6 @@ def test_reserved_project_names(self, project_name):
],
)
def test_reserved_organization_names(self, namespace):

data = MultiDict(
{
"namespace": namespace,
Expand Down
Loading

0 comments on commit 4775901

Please sign in to comment.