Skip to content

Commit c6dd5af

Browse files
facutuescadi
facutuesca
and
di
authored
Check for existing Trusted Publishers before constraining existing one (#17576)
* Add check before creating constrained Trusted Publisher Signed-off-by: Facundo Tuesca <[email protected]> * Add extra information to OIDCPublisherAdded event Signed-off-by: Facundo Tuesca <[email protected]> * Move exists() check to Trusted Publisher classes Signed-off-by: Facundo Tuesca <[email protected]> --------- Signed-off-by: Facundo Tuesca <[email protected]> Co-authored-by: Dustin Ingram <[email protected]>
1 parent 310f89e commit c6dd5af

File tree

14 files changed

+313
-41
lines changed

14 files changed

+313
-41
lines changed

‎tests/unit/manage/test_views.py

Lines changed: 84 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6573,6 +6573,8 @@ def test_manage_project_oidc_publishers_constrain_environment(
65736573
"specifier": str(constrained_publisher),
65746574
"url": publisher.publisher_url(),
65756575
"submitted_by": db_request.user.username,
6576+
"reified_from_pending_publisher": False,
6577+
"constrained_from_existing_publisher": True,
65766578
},
65776579
),
65786580
pretend.call(
@@ -6672,6 +6674,8 @@ def test_manage_project_oidc_publishers_constrain_environment_shared_publisher(
66726674
"specifier": str(constrained_publisher),
66736675
"url": publisher.publisher_url(),
66746676
"submitted_by": db_request.user.username,
6677+
"reified_from_pending_publisher": False,
6678+
"constrained_from_existing_publisher": True,
66756679
},
66766680
),
66776681
pretend.call(
@@ -6923,7 +6927,7 @@ def test_constrain_unsupported_publisher(
69236927
)
69246928
]
69256929

6926-
def test_constrain_publisher_already_constrained(
6930+
def test_constrain_publisher_with_nonempty_environment(
69276931
self, monkeypatch, metrics, db_request
69286932
):
69296933
owner = UserFactory.create()
@@ -6978,6 +6982,81 @@ def test_constrain_publisher_already_constrained(
69786982
)
69796983
]
69806984

6985+
@pytest.mark.parametrize(
6986+
("publisher_class", "publisher_kwargs"),
6987+
[
6988+
(
6989+
GitHubPublisher,
6990+
{
6991+
"repository_name": "some-repository",
6992+
"repository_owner": "some-owner",
6993+
"repository_owner_id": "666",
6994+
"workflow_filename": "some-workflow-filename.yml",
6995+
},
6996+
),
6997+
(
6998+
GitLabPublisher,
6999+
{
7000+
"namespace": "some-namespace",
7001+
"project": "some-project",
7002+
"workflow_filepath": "some-workflow-filename.yml",
7003+
},
7004+
),
7005+
],
7006+
)
7007+
def test_constrain_environment_publisher_already_exists(
7008+
self, monkeypatch, metrics, db_request, publisher_class, publisher_kwargs
7009+
):
7010+
owner = UserFactory.create()
7011+
db_request.user = owner
7012+
7013+
# Create unconstrained and constrained versions of the publisher
7014+
unconstrained = publisher_class(environment="", **publisher_kwargs)
7015+
constrained = publisher_class(environment="fakeenv", **publisher_kwargs)
7016+
7017+
project = ProjectFactory.create(oidc_publishers=[unconstrained, constrained])
7018+
project.record_event = pretend.call_recorder(lambda *a, **kw: None)
7019+
RoleFactory.create(user=owner, project=project, role_name="Owner")
7020+
7021+
db_request.db.add_all([unconstrained, constrained])
7022+
db_request.db.flush() # To get the ids
7023+
7024+
db_request.method = "POST"
7025+
db_request.POST = MultiDict(
7026+
{
7027+
"constrained_publisher_id": str(unconstrained.id),
7028+
"constrained_environment_name": "fakeenv",
7029+
}
7030+
)
7031+
db_request.find_service = lambda *a, **kw: metrics
7032+
db_request.session = pretend.stub(
7033+
flash=pretend.call_recorder(lambda *a, **kw: None)
7034+
)
7035+
db_request.flags = pretend.stub(
7036+
disallow_oidc=pretend.call_recorder(lambda f=None: False)
7037+
)
7038+
db_request._ = lambda s: s
7039+
7040+
view = views.ManageOIDCPublisherViews(project, db_request)
7041+
default_response = {"_": pretend.stub()}
7042+
monkeypatch.setattr(
7043+
views.ManageOIDCPublisherViews, "default_response", default_response
7044+
)
7045+
7046+
assert view.constrain_environment() == default_response
7047+
assert view.metrics.increment.calls == [
7048+
pretend.call(
7049+
"warehouse.oidc.constrain_publisher_environment.attempt",
7050+
),
7051+
]
7052+
assert project.record_event.calls == []
7053+
assert db_request.session.flash.calls == [
7054+
pretend.call(
7055+
f"{unconstrained} is already registered with {project.name}",
7056+
queue="error",
7057+
)
7058+
]
7059+
69817060
@pytest.mark.parametrize(
69827061
("view_name", "publisher", "make_form"),
69837062
[
@@ -7135,6 +7214,8 @@ def test_add_oidc_publisher_preexisting(
71357214
"specifier": "fakespecifier",
71367215
"url": publisher.publisher_url(),
71377216
"submitted_by": "some-user",
7217+
"reified_from_pending_publisher": False,
7218+
"constrained_from_existing_publisher": False,
71387219
},
71397220
)
71407221
]
@@ -7286,6 +7367,8 @@ def test_add_oidc_publisher_created(
72867367
"specifier": str(publisher),
72877368
"url": publisher.publisher_url(),
72887369
"submitted_by": "some-user",
7370+
"reified_from_pending_publisher": False,
7371+
"constrained_from_existing_publisher": False,
72897372
},
72907373
)
72917374
]

‎tests/unit/oidc/models/test_activestate.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -366,6 +366,21 @@ def test_activestate_publisher_verify_url(self, url, expected):
366366
)
367367
assert publisher.verify_url(url) == expected
368368

369+
@pytest.mark.parametrize("exists_in_db", [True, False])
370+
def test_exists(self, db_request, exists_in_db):
371+
publisher = ActiveStatePublisher(
372+
organization="repository_name",
373+
activestate_project_name="project_name",
374+
actor_id=ACTOR_ID,
375+
actor=ACTOR,
376+
)
377+
378+
if exists_in_db:
379+
db_request.db.add(publisher)
380+
db_request.db.flush()
381+
382+
assert publisher.exists(db_request.db) == exists_in_db
383+
369384

370385
class TestPendingActiveStatePublisher:
371386
def test_reify_does_not_exist_yet(self, db_request):

‎tests/unit/oidc/models/test_github.py

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -685,6 +685,22 @@ def test_github_publisher_attestation_identity(self, environment):
685685
else:
686686
assert identity.environment == publisher.environment
687687

688+
@pytest.mark.parametrize("exists_in_db", [True, False])
689+
def test_exists(self, db_request, exists_in_db):
690+
publisher = github.GitHubPublisher(
691+
repository_name="repository_name",
692+
repository_owner="repository_owner",
693+
repository_owner_id="666",
694+
workflow_filename="workflow_filename.yml",
695+
environment="",
696+
)
697+
698+
if exists_in_db:
699+
db_request.db.add(publisher)
700+
db_request.db.flush()
701+
702+
assert publisher.exists(db_request.db) == exists_in_db
703+
688704

689705
class TestPendingGitHubPublisher:
690706
def test_reify_does_not_exist_yet(self, db_request):

‎tests/unit/oidc/models/test_gitlab.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -741,6 +741,21 @@ def test_gitlab_publisher_attestation_identity(self, environment):
741741
else:
742742
assert identity.environment == publisher.environment
743743

744+
@pytest.mark.parametrize("exists_in_db", [True, False])
745+
def test_exists(self, db_request, exists_in_db):
746+
publisher = gitlab.GitLabPublisher(
747+
project="repository_name",
748+
namespace="repository_owner",
749+
workflow_filepath="subfolder/worflow_filename.yml",
750+
environment="",
751+
)
752+
753+
if exists_in_db:
754+
db_request.db.add(publisher)
755+
db_request.db.flush()
756+
757+
assert publisher.exists(db_request.db) == exists_in_db
758+
744759

745760
class TestPendingGitLabPublisher:
746761
def test_reify_does_not_exist_yet(self, db_request):

‎tests/unit/oidc/models/test_google.py

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,19 @@ def test_google_publisher_sub_is_optional(self, expected_sub, actual_sub, valid)
196196
)
197197
assert str(e.value) == "Check failed for optional claim 'sub'"
198198

199+
@pytest.mark.parametrize("exists_in_db", [True, False])
200+
def test_exists(self, db_request, exists_in_db):
201+
publisher = google.GooglePublisher(
202+
email="[email protected]",
203+
sub="fakesubject",
204+
)
205+
206+
if exists_in_db:
207+
db_request.db.add(publisher)
208+
db_request.db.flush()
209+
210+
assert publisher.exists(db_request.db) == exists_in_db
211+
199212

200213
class TestPendingGooglePublisher:
201214
@pytest.mark.parametrize("sub", ["fakesubject", None])

‎tests/unit/oidc/test_views.py

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,11 +32,13 @@
3232
from warehouse.metrics import IMetricsService
3333
from warehouse.oidc import errors, views
3434
from warehouse.oidc.interfaces import IOIDCPublisherService, SignedClaims
35+
from warehouse.oidc.models import GitHubPublisher
3536
from warehouse.oidc.views import (
3637
is_from_reusable_workflow,
3738
should_send_environment_warning_email,
3839
)
3940
from warehouse.packaging import services
41+
from warehouse.packaging.models import Project
4042
from warehouse.rate_limiting.interfaces import IRateLimiter
4143

4244

@@ -446,6 +448,7 @@ def test_mint_token_from_oidc_pending_publisher_ok(
446448
dummy_github_oidc_jwt,
447449
):
448450
user = UserFactory.create()
451+
449452
pending_publisher = PendingGitHubPublisherFactory.create(
450453
project_name="does-not-exist",
451454
added_by=user,
@@ -476,6 +479,25 @@ def test_mint_token_from_oidc_pending_publisher_ok(
476479
pretend.call(db_request.remote_addr),
477480
]
478481

482+
project = (
483+
db_request.db.query(Project)
484+
.filter(Project.name == pending_publisher.project_name)
485+
.one()
486+
)
487+
publisher = db_request.db.query(GitHubPublisher).one()
488+
event = project.events.where(
489+
Project.Event.tag == EventTag.Project.OIDCPublisherAdded
490+
).one()
491+
assert event.additional == {
492+
"publisher": publisher.publisher_name,
493+
"id": str(publisher.id),
494+
"specifier": str(publisher),
495+
"url": publisher.publisher_url(),
496+
"submitted_by": "OpenID created token",
497+
"reified_from_pending_publisher": True,
498+
"constrained_from_existing_publisher": False,
499+
}
500+
479501

480502
def test_mint_token_from_pending_trusted_publisher_invalidates_others(
481503
monkeypatch, db_request, dummy_github_oidc_jwt
@@ -543,6 +565,25 @@ def test_mint_token_from_pending_trusted_publisher_invalidates_others(
543565
pretend.call(db_request.remote_addr),
544566
]
545567

568+
project = (
569+
db_request.db.query(Project)
570+
.filter(Project.name == pending_publisher.project_name)
571+
.one()
572+
)
573+
publisher = db_request.db.query(GitHubPublisher).one()
574+
event = project.events.where(
575+
Project.Event.tag == EventTag.Project.OIDCPublisherAdded
576+
).one()
577+
assert event.additional == {
578+
"publisher": publisher.publisher_name,
579+
"id": str(publisher.id),
580+
"specifier": str(publisher),
581+
"url": publisher.publisher_url(),
582+
"submitted_by": "OpenID created token",
583+
"reified_from_pending_publisher": True,
584+
"constrained_from_existing_publisher": False,
585+
}
586+
546587

547588
@pytest.mark.parametrize(
548589
("claims_in_token", "claims_input"),

0 commit comments

Comments
 (0)