Skip to content

fix(oidc): roll back aborted transaction after pending-publisher conflict #20074

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
Fyko wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(oidc): roll back aborted transaction after pending-publisher conflict #20074

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
160 changes: 143 additions & 17 deletions tests/unit/accounts/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import pretend
import pytest

from psycopg.errors import UniqueViolation
from pyramid.httpexceptions import (
HTTPBadRequest,
HTTPMovedPermanently,
Expand Down Expand Up @@ -4858,15 +4857,133 @@ def test_add_pending_oidc_publisher_already_exists(
)
]

def test_add_pending_oidc_publisher_uniqueviolation(self, monkeypatch, db_request):
"""A UniqueViolation on insert means another pending publisher already
exists for the same (repo, owner, workflow, environment) tuple but with
a different project_name. Surface this conflict to the user instead of
silently redirecting as if the registration succeeded.
@pytest.mark.parametrize(
(
"view_name",
"publisher_name",
"publisher_class",
"make_publisher",
"post_body",
),
[
(
"add_pending_github_oidc_publisher",
"GitHub",
PendingGitHubPublisher,
lambda user_id: PendingGitHubPublisher(
project_name="some-other-project-name",
repository_name="some-repository",
repository_owner="some-owner",
repository_owner_id="some-owner-id",
workflow_filename="some-workflow-filename.yml",
environment="some-environment",
added_by_id=user_id,
),
MultiDict(
{
"owner": "some-owner",
"repository": "some-repository",
"workflow_filename": "some-workflow-filename.yml",
"environment": "some-environment",
"project_name": "some-project-name",
}
),
),
(
"add_pending_gitlab_oidc_publisher",
"GitLab",
PendingGitLabPublisher,
lambda user_id: PendingGitLabPublisher(
project_name="some-other-project-name",
namespace="some-owner",
project="some-repository",
workflow_filepath="subfolder/some-workflow-filename.yml",
environment="some-environment",
issuer_url="https://gitlab.com",
added_by_id=user_id,
),
MultiDict(
{
"namespace": "some-owner",
"project": "some-repository",
"workflow_filepath": "subfolder/some-workflow-filename.yml",
"environment": "some-environment",
"project_name": "some-project-name",
"issuer_url": "https://gitlab.com",
}
),
),
(
"add_pending_google_oidc_publisher",
"Google",
PendingGooglePublisher,
lambda user_id: PendingGooglePublisher(
project_name="some-other-project-name",
email="some-email@example.com",
sub="some-sub",
added_by_id=user_id,
),
MultiDict(
{
"email": "some-email@example.com",
"sub": "some-sub",
"project_name": "some-project-name",
}
),
),
(
"add_pending_activestate_oidc_publisher",
"ActiveState",
PendingActiveStatePublisher,
lambda user_id: PendingActiveStatePublisher(
project_name="some-other-project-name",
added_by_id=user_id,
organization="some-org",
activestate_project_name="some-project",
actor="some-user",
actor_id="some-user-id",
),
MultiDict(
{
"organization": "some-org",
"project": "some-project",
"actor": "some-user",
"project_name": "some-project-name",
}
),
),
],
)
def test_add_pending_oidc_publisher_uniqueviolation(
self,
monkeypatch,
db_request,
view_name,
publisher_name,
publisher_class,
make_publisher,
post_body,
):
"""A UniqueViolation raised by the INSERT during ``flush()`` means
another pending publisher already exists for the same external
identity tuple but with a different ``project_name``. The early
duplicate-check query keys on ``project_name`` so it misses that row,
but the DB unique constraint does not, so the insert fails.

Surface the conflict to the user instead of silently redirecting as if
the registration succeeded -- and crucially, roll back the now-aborted
transaction so the session stays usable for the rest of the request
(template rendering, the end-of-request commit). Regression test for
GH-20006.
"""
db_request.user = UserFactory.create()
EmailFactory(user=db_request.user, verified=True, primary=True)
db_request.db.add = pretend.raiser(UniqueViolation("foo", "bar", "baz"))
# A pending publisher with the same external identity but a *different*
# project_name. flush()-ing it sends the INSERT to the DB so the next
# conflicting insert raises a real UniqueViolation.
existing_publisher = make_publisher(db_request.user.id)
db_request.db.add(existing_publisher)
db_request.db.flush()

db_request.registry = pretend.stub(
settings={
Expand All @@ -4879,15 +4996,7 @@ def test_add_pending_oidc_publisher_uniqueviolation(self, monkeypatch, db_reques
db_request.session = pretend.stub(
flash=pretend.call_recorder(lambda *a, **kw: None)
)
db_request.POST = MultiDict(
{
"owner": "some-owner",
"repository": "some-repository",
"workflow_filename": "some-workflow-filename.yml",
"environment": "some-environment",
"project_name": "some-project-name",
}
)
db_request.POST = post_body

view = views.ManageAccountPublishingViews(db_request)

Expand All @@ -4896,6 +5005,16 @@ def test_add_pending_oidc_publisher_uniqueviolation(self, monkeypatch, db_reques
"_lookup_owner",
lambda *a: {"login": "some-owner", "id": "some-owner-id"},
)
monkeypatch.setattr(
views.PendingActiveStatePublisherForm,
"_lookup_organization",
lambda *a: None,
)
monkeypatch.setattr(
views.PendingActiveStatePublisherForm,
"_lookup_actor",
lambda *a: {"user_id": "some-user-id"},
)

monkeypatch.setattr(
view, "_check_ratelimits", pretend.call_recorder(lambda: None)
Expand All @@ -4904,7 +5023,7 @@ def test_add_pending_oidc_publisher_uniqueviolation(self, monkeypatch, db_reques
view, "_hit_ratelimits", pretend.call_recorder(lambda: None)
)

assert view.add_pending_github_oidc_publisher() == view.default_response
assert getattr(view, view_name)() == view.default_response
assert db_request.session.flash.calls == [
pretend.call(
(
Expand All @@ -4915,6 +5034,13 @@ def test_add_pending_oidc_publisher_uniqueviolation(self, monkeypatch, db_reques
queue="error",
)
]
# The conflicting INSERT left the transaction aborted. Without an
# explicit rollback in the handler this query raises PendingRollbackError
# -- which is what surfaces to the user as a 500/503 once the template
# tries to render the user's existing pending publishers. The rollback
# also discards this request's uncommitted work, including the
# pre-existing publisher created above, so the count is 0.
assert db_request.db.query(publisher_class).count() == 0

@pytest.mark.parametrize(
("view_name", "publisher_name", "post_body", "publisher_class"),
Expand Down
31 changes: 27 additions & 4 deletions tests/unit/manage/views/test_organizations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

from freezegun import freeze_time
from paginate_sqlalchemy import SqlalchemyOrmPage as SQLAlchemyORMPage
from psycopg.errors import UniqueViolation
from pyramid.httpexceptions import HTTPBadRequest, HTTPNotFound, HTTPSeeOther
from webob.multidict import MultiDict

Expand Down Expand Up @@ -3730,16 +3729,34 @@ def test_add_pending_github_oidc_publisher_already_exists(
def test_add_pending_github_oidc_publisher_unique_violation(
self, db_request, monkeypatch
):
"""Test UniqueViolation exception handling during publisher creation"""
"""A UniqueViolation raised by the INSERT during ``flush()`` (another
pending publisher already targets the same external identity under a
different ``project_name``) must be handled as double-post protection:
roll back the now-aborted transaction so the session stays usable for
the end-of-request commit, then redirect. Regression test for GH-20006.
"""
organization = OrganizationFactory.create()
user = UserFactory.create(with_verified_primary_email=True)
db_request.POST = MultiDict()
db_request.path = "/fake/path"
db_request.route_url = pretend.call_recorder(lambda *a, **kw: "/fake/route")
db_request.user = user

# Mock db.add to raise UniqueViolation (simulates race condition)
db_request.db.add = pretend.raiser(UniqueViolation("foo", "bar", "baz"))
# A pending publisher with the same external identity but a *different*
# project_name. The early duplicate-check query keys on project_name so
# it misses this row, but the DB unique constraint does not -- so the
# next conflicting insert raises a real UniqueViolation during flush().
existing_publisher = PendingGitHubPublisherFactory.create(
project_name="test-project-existing",
repository_name="test-repo",
repository_owner="test-owner",
repository_owner_id="12345",
workflow_filename="release.yml",
environment="",
organization_id=organization.id,
)
db_request.db.add(existing_publisher)
db_request.db.flush()

# Mock form
form = pretend.stub(
Expand All @@ -3761,6 +3778,12 @@ def test_add_pending_github_oidc_publisher_unique_violation(
# Should return HTTPSeeOther redirect (double-post protection)
assert isinstance(result, HTTPSeeOther)
assert result.location == "/fake/path"
# The conflicting INSERT left the transaction aborted. Without an
# explicit rollback in the handler this query raises PendingRollbackError
# -- which is what surfaces to the user as a 500/503. The rollback also
# discards this request's uncommitted work, including the pre-existing
# publisher created above, so the count is 0.
assert db_request.db.query(PendingGitHubPublisher).count() == 0

def test_two_orgs_can_create_pending_publishers_for_same_project_name(
self, db_request, monkeypatch
Expand Down
5 changes: 5 additions & 0 deletions warehouse/accounts/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1899,6 +1899,11 @@ def _add_pending_oidc_publisher(
# pending publisher already targets the same external identity
# under a different project name. Surface that conflict instead
# of silently redirecting as if registration succeeded.
#
# The failed INSERT leaves the transaction in an aborted state, so
# roll back before doing anything else with the session -- otherwise
# rendering the response (or the end-of-request commit) blows up.
self.request.db.rollback()
self.request.session.flash(
self.request._(
"A pending trusted publisher matching this configuration "
Expand Down
5 changes: 4 additions & 1 deletion warehouse/manage/views/organizations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1886,7 +1886,10 @@ def _add_pending_oidc_publisher(
self.request.db.add(pending_publisher)
self.request.db.flush() # To get the new ID # ast-grep-ignore: db-flush
except UniqueViolation:
# Double-post protection
# Double-post protection. The failed INSERT leaves the transaction
# in an aborted state, so roll back before redirecting -- otherwise
# the end-of-request commit blows up.
self.request.db.rollback()
return HTTPSeeOther(self.request.path)

# Record event on organization
Expand Down
Loading