From bd55bb7eff0db424c6e39543f9532eb0c25d53d9 Mon Sep 17 00:00:00 2001 From: Carter Date: Thu, 14 May 2026 10:38:11 -0500 Subject: [PATCH] fix(oidc): roll back aborted transaction after pending-publisher conflict The `except UniqueViolation` handler for pending trusted publishers never rolled back the aborted transaction, so the next DB access raised PendingRollbackError and the user got a 500/503 instead of the intended error message. Roll back inside the handler, in both the account- and organization-scoped views. Fixes #20006 --- tests/unit/accounts/test_views.py | 160 ++++++++++++++++-- tests/unit/manage/views/test_organizations.py | 31 +++- warehouse/accounts/views.py | 5 + warehouse/manage/views/organizations.py | 5 +- 4 files changed, 179 insertions(+), 22 deletions(-) diff --git a/tests/unit/accounts/test_views.py b/tests/unit/accounts/test_views.py index e4933afbddf7..10c5d06ef766 100644 --- a/tests/unit/accounts/test_views.py +++ b/tests/unit/accounts/test_views.py @@ -8,7 +8,6 @@ import pretend import pytest -from psycopg.errors import UniqueViolation from pyramid.httpexceptions import ( HTTPBadRequest, HTTPMovedPermanently, @@ -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={ @@ -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) @@ -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) @@ -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( ( @@ -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"), diff --git a/tests/unit/manage/views/test_organizations.py b/tests/unit/manage/views/test_organizations.py index 469e1f5c45e4..dfcd30bd98a8 100644 --- a/tests/unit/manage/views/test_organizations.py +++ b/tests/unit/manage/views/test_organizations.py @@ -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 @@ -3730,7 +3729,12 @@ 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() @@ -3738,8 +3742,21 @@ def test_add_pending_github_oidc_publisher_unique_violation( 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( @@ -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 diff --git a/warehouse/accounts/views.py b/warehouse/accounts/views.py index dfe314500333..518153148857 100644 --- a/warehouse/accounts/views.py +++ b/warehouse/accounts/views.py @@ -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 " diff --git a/warehouse/manage/views/organizations.py b/warehouse/manage/views/organizations.py index 3903b401fdb9..c7c6fdd4c920 100644 --- a/warehouse/manage/views/organizations.py +++ b/warehouse/manage/views/organizations.py @@ -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