From 48ab12eb1033882f6726b5b3519c2889e9cad6bf Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 14 Feb 2022 15:39:58 -0500 Subject: [PATCH 01/78] warehouse/oidc: rough model skeleton --- warehouse/oidc/models.py | 47 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 warehouse/oidc/models.py diff --git a/warehouse/oidc/models.py b/warehouse/oidc/models.py new file mode 100644 index 000000000000..78679c9a24fe --- /dev/null +++ b/warehouse/oidc/models.py @@ -0,0 +1,47 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + + +from sqlalchemy import UUID, Column, ForeignKey, String + +from warehouse import db + + +class OIDCProvider(db.Model): + __tablename__ = "oidc_providers" + + discriminator = Column(String) + + __mapper_args__ = {"polymorphic_on": discriminator} + + def verify_claims(self, token): + return NotImplemented + + +class GitHubProvider(OIDCProvider): + __tablename__ = "github_oidc_providers" + __mapper_args__ = {"polymorphic_identity": "GitHubProvider"} + + id = Column(UUID(as_uuid=True), ForeignKey(OIDCProvider.id), primary_key=True) + repository_name = Column(String) + owner = Column(String) + owner_id = Column(String) + workflow_name = Column(String) + + def repository(self): + return f"{self.owner}/{self.repository_name}" + + def job_workflow_ref(self): + return f"{self.repository}/.github/workflows/{self.workflow_name}.yml" + + def verify_claims(self, token): + return NotImplemented From 7db2c2cd5fcf175d6f998cfff721329a1a439bd9 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 14 Feb 2022 15:46:07 -0500 Subject: [PATCH 02/78] warehouse/oidc: fix imports --- warehouse/oidc/models.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/warehouse/oidc/models.py b/warehouse/oidc/models.py index 78679c9a24fe..b83ab4fbd57b 100644 --- a/warehouse/oidc/models.py +++ b/warehouse/oidc/models.py @@ -11,7 +11,8 @@ # limitations under the License. -from sqlalchemy import UUID, Column, ForeignKey, String +from sqlalchemy import Column, ForeignKey, String +from sqlalchemy.dialects.postgresql import UUID from warehouse import db From eab5e707ae3056a61abe31a5843133aee4d9e3ce Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 14 Feb 2022 15:46:15 -0500 Subject: [PATCH 03/78] warehouse/migrations: add migration for OIDC models --- ...6d5769_add_initial_oidc_provider_models.py | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 warehouse/migrations/versions/47422b6d5769_add_initial_oidc_provider_models.py diff --git a/warehouse/migrations/versions/47422b6d5769_add_initial_oidc_provider_models.py b/warehouse/migrations/versions/47422b6d5769_add_initial_oidc_provider_models.py new file mode 100644 index 000000000000..3bb00d647758 --- /dev/null +++ b/warehouse/migrations/versions/47422b6d5769_add_initial_oidc_provider_models.py @@ -0,0 +1,59 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +""" +Add initial OIDC provider models + +Revision ID: 47422b6d5769 +Revises: 29a8901a4635 +Create Date: 2022-02-14 20:45:29.641650 +""" + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '47422b6d5769' +down_revision = '29a8901a4635' + +# Note: It is VERY important to ensure that a migration does not lock for a +# long period of time and to ensure that each individual migration does +# not break compatibility with the *previous* version of the code base. +# This is because the migrations will be ran automatically as part of the +# deployment process, but while the previous version of the code is still +# up and running. Thus backwards incompatible changes must be broken up +# over multiple migrations inside of multiple pull requests in order to +# phase them in over multiple deploys. + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.create_table('oidc_providers', + sa.Column('id', postgresql.UUID(as_uuid=True), server_default=sa.text('gen_random_uuid()'), nullable=False), + sa.Column('discriminator', sa.String(), nullable=True), + sa.PrimaryKeyConstraint('id') + ) + op.create_table('github_oidc_providers', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('repository_name', sa.String(), nullable=True), + sa.Column('owner', sa.String(), nullable=True), + sa.Column('owner_id', sa.String(), nullable=True), + sa.Column('workflow_name', sa.String(), nullable=True), + sa.ForeignKeyConstraint(['id'], ['oidc_providers.id'], ), + sa.PrimaryKeyConstraint('id') + ) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_table('github_oidc_providers') + op.drop_table('oidc_providers') + # ### end Alembic commands ### From 09d0966db447e9a6064235fe96232b5a99560718 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 14 Feb 2022 15:52:35 -0500 Subject: [PATCH 04/78] warehouse/migrations: reformat --- ...6d5769_add_initial_oidc_provider_models.py | 46 ++++++++++++------- 1 file changed, 29 insertions(+), 17 deletions(-) diff --git a/warehouse/migrations/versions/47422b6d5769_add_initial_oidc_provider_models.py b/warehouse/migrations/versions/47422b6d5769_add_initial_oidc_provider_models.py index 3bb00d647758..0fa9c2bebb09 100644 --- a/warehouse/migrations/versions/47422b6d5769_add_initial_oidc_provider_models.py +++ b/warehouse/migrations/versions/47422b6d5769_add_initial_oidc_provider_models.py @@ -17,12 +17,13 @@ Create Date: 2022-02-14 20:45:29.641650 """ -from alembic import op import sqlalchemy as sa + +from alembic import op from sqlalchemy.dialects import postgresql -revision = '47422b6d5769' -down_revision = '29a8901a4635' +revision = "47422b6d5769" +down_revision = "29a8901a4635" # Note: It is VERY important to ensure that a migration does not lock for a # long period of time and to ensure that each individual migration does @@ -33,27 +34,38 @@ # over multiple migrations inside of multiple pull requests in order to # phase them in over multiple deploys. + def upgrade(): # ### commands auto generated by Alembic - please adjust! ### - op.create_table('oidc_providers', - sa.Column('id', postgresql.UUID(as_uuid=True), server_default=sa.text('gen_random_uuid()'), nullable=False), - sa.Column('discriminator', sa.String(), nullable=True), - sa.PrimaryKeyConstraint('id') + op.create_table( + "oidc_providers", + sa.Column( + "id", + postgresql.UUID(as_uuid=True), + server_default=sa.text("gen_random_uuid()"), + nullable=False, + ), + sa.Column("discriminator", sa.String(), nullable=True), + sa.PrimaryKeyConstraint("id"), ) - op.create_table('github_oidc_providers', - sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), - sa.Column('repository_name', sa.String(), nullable=True), - sa.Column('owner', sa.String(), nullable=True), - sa.Column('owner_id', sa.String(), nullable=True), - sa.Column('workflow_name', sa.String(), nullable=True), - sa.ForeignKeyConstraint(['id'], ['oidc_providers.id'], ), - sa.PrimaryKeyConstraint('id') + op.create_table( + "github_oidc_providers", + sa.Column("id", postgresql.UUID(as_uuid=True), nullable=False), + sa.Column("repository_name", sa.String(), nullable=True), + sa.Column("owner", sa.String(), nullable=True), + sa.Column("owner_id", sa.String(), nullable=True), + sa.Column("workflow_name", sa.String(), nullable=True), + sa.ForeignKeyConstraint( + ["id"], + ["oidc_providers.id"], + ), + sa.PrimaryKeyConstraint("id"), ) # ### end Alembic commands ### def downgrade(): # ### commands auto generated by Alembic - please adjust! ### - op.drop_table('github_oidc_providers') - op.drop_table('oidc_providers') + op.drop_table("github_oidc_providers") + op.drop_table("oidc_providers") # ### end Alembic commands ### From 24b9eab3dc09d3206d560a87e3d4dc0ec15714e2 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 14 Feb 2022 16:52:12 -0500 Subject: [PATCH 05/78] warehouse/oidc: add basic verification logic --- warehouse/oidc/interfaces.py | 9 ++++++- warehouse/oidc/models.py | 9 +++++-- warehouse/oidc/services.py | 47 +++++++++++++++++++++++++++++++++--- 3 files changed, 59 insertions(+), 6 deletions(-) diff --git a/warehouse/oidc/interfaces.py b/warehouse/oidc/interfaces.py index 4c1ea13e9297..513d5119e9d3 100644 --- a/warehouse/oidc/interfaces.py +++ b/warehouse/oidc/interfaces.py @@ -28,5 +28,12 @@ def get_key(key_id): def verify(token): """ - Verify the given JWT. + Verify the given JWT's signature and basic claims, returning + a tuple of (valid, decoded) where `valid` indicates + the validity of the JWT and `decoded` is the decoded JWT, + or `None` if invalid. + + This function **does not** verify the token's suitability + for a particular action; subsequent checks on the decoded token's + third party claims must be done to ensure that. """ diff --git a/warehouse/oidc/models.py b/warehouse/oidc/models.py index b83ab4fbd57b..cb7a0406d1a2 100644 --- a/warehouse/oidc/models.py +++ b/warehouse/oidc/models.py @@ -24,7 +24,7 @@ class OIDCProvider(db.Model): __mapper_args__ = {"polymorphic_on": discriminator} - def verify_claims(self, token): + def verify_claims(self, signed_token): return NotImplemented @@ -44,5 +44,10 @@ def repository(self): def job_workflow_ref(self): return f"{self.repository}/.github/workflows/{self.workflow_name}.yml" - def verify_claims(self, token): + def verify_claims(self, signed_token): + """ + Given a JWT that has been successfully decoded (checked for a valid + signature and basic claims), verify it against the more specific + claims of this provider. + """ return NotImplemented diff --git a/warehouse/oidc/services.py b/warehouse/oidc/services.py index f84a116a6cf0..a9499c03e17b 100644 --- a/warehouse/oidc/services.py +++ b/warehouse/oidc/services.py @@ -12,11 +12,11 @@ import json +import jwt import redis import requests import sentry_sdk -from jwt import PyJWK from zope.interface import implementer from warehouse.metrics.interfaces import IMetricsService @@ -148,10 +148,51 @@ def get_key(self, key_id): tags=[f"provider:{self.provider}", f"key_id:{key_id}"], ) return None - return PyJWK(keyset[key_id]) + return jwt.PyJWK(keyset[key_id]) + + def _get_key_for_token(self, token): + """ + Return a JWK suitable for verifying the given JWT. + + The JWT is not verified at this point, and this step happens + prior to any verification. + """ + unverified_header = jwt.get_unverified_header(token) + return self.get_key(unverified_header["kid"]) def verify(self, token): - return NotImplemented + key = self._get_key_for_token(token) + + try: + # NOTE: Many of the keyword arguments here are defaults, but we + # set them explicitly to assert the intended verification behavior. + valid_token = jwt.decode( + token, + key=key, + algorithms=["RS256"], + verify_signature=True, + # "require" only checks for the presence of these claims, not + # their validity. Each has a corresponding "verify_" kwarg + # that enforces their actual validity. + require=["iss", "iat", "nbf", "exp", "aud"], + verify_iss=True, + verify_iat=True, + verify_nbf=True, + verify_exp=True, + verify_aud=True, + issuer=self.issuer_url, + audience="pypi", + leeway=60, + ) + return True, valid_token + except jwt.PyJWTError: + return False, None + except Exception as e: + # We expect pyjwt to only raise subclasses of PyJWTError, but + # we can't enforce this. Other exceptions indicate an abstraction + # leak, so we log them for upstream reporting. + sentry_sdk.capture_message(f"JWT verify raised generic error: {e}") + return False, None class OIDCProviderServiceFactory: From c56476f2d966791cc7c48b5df9c7d9b53e23dd11 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 14 Feb 2022 16:56:00 -0500 Subject: [PATCH 06/78] oidc/services: reduce clock skew leeway to 30s --- warehouse/oidc/services.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/warehouse/oidc/services.py b/warehouse/oidc/services.py index a9499c03e17b..cbe818f36789 100644 --- a/warehouse/oidc/services.py +++ b/warehouse/oidc/services.py @@ -182,7 +182,7 @@ def verify(self, token): verify_aud=True, issuer=self.issuer_url, audience="pypi", - leeway=60, + leeway=30, ) return True, valid_token except jwt.PyJWTError: From 41a1ca05e3eca51f47a9767c80ae734efe183250 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 15 Feb 2022 13:44:19 -0500 Subject: [PATCH 07/78] warehouse/oidc: refactor claim verification --- warehouse/oidc/interfaces.py | 2 +- warehouse/oidc/models.py | 83 +++++++++++++++++++++++++++++++----- warehouse/oidc/services.py | 2 +- 3 files changed, 75 insertions(+), 12 deletions(-) diff --git a/warehouse/oidc/interfaces.py b/warehouse/oidc/interfaces.py index 513d5119e9d3..33ffa0639d55 100644 --- a/warehouse/oidc/interfaces.py +++ b/warehouse/oidc/interfaces.py @@ -26,7 +26,7 @@ def get_key(key_id): """ pass - def verify(token): + def verify_signature_only(token): """ Verify the given JWT's signature and basic claims, returning a tuple of (valid, decoded) where `valid` indicates diff --git a/warehouse/oidc/models.py b/warehouse/oidc/models.py index cb7a0406d1a2..3b81aa8d4a7a 100644 --- a/warehouse/oidc/models.py +++ b/warehouse/oidc/models.py @@ -10,6 +10,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import sentry_sdk from sqlalchemy import Column, ForeignKey, String from sqlalchemy.dialects.postgresql import UUID @@ -24,8 +25,55 @@ class OIDCProvider(db.Model): __mapper_args__ = {"polymorphic_on": discriminator} - def verify_claims(self, signed_token): - return NotImplemented + # A map of claim names to "check" functions, each of which + # has the signature `check(ground-truth, signed-claim) -> bool`. + __verifiable_claims__ = dict() + + # Claims that have already been verified during the JWT signature + # verification phase. + __preverified_claims__ = { + "iss", + "iat", + "nbf", + "exp", + "aud", + } + + # Individual providers should explicitly override this set, + # indicating any custom claims that are known to be present but are + # not checked as part of verifying the JWT. + __unchecked_claims__ = set() + + def verify_claims(self, signed_claims): + """ + Given a JWT that has been successfully decoded (checked for a valid + signature and basic claims), verify it against the more specific + claims of this provider. + """ + + # Defensive programming: treat the absence of any claims to verify + # as a failure rather than trivially valid. + if not self.__verifiable_claims__: + return False + + # All claims should be accounted for. + # The presence of an unaccounted claim is not an error, only a warning + # that the JWT payload has changed. + known_claims = self.__verifiable_claims__.keys().union( + self.__preverified_claims__, self.__unchecked_claims__ + ) + unaccounted_claims = known_claims.difference(signed_claims.keys()) + if unaccounted_claims: + sentry_sdk.capture_message( + f"JWT for {self.__class__.__name__} has unaccounted claims: {unaccounted_claims}" + ) + + # Finally, perform the actual claim verification. + for claim_name, check in self.__verifiable_claims__.items(): + if not check(self.getattr(claim_name), signed_claims[claim_name]): + return False + + return True class GitHubProvider(OIDCProvider): @@ -38,16 +86,31 @@ class GitHubProvider(OIDCProvider): owner_id = Column(String) workflow_name = Column(String) + __verifiable_claims__ = { + "repository": str.__eq__, + "job_workflow_ref": str.startswith, + "actor": str.__eq__, + "workflow": str.__eq__, + } + + __unchecked_claims__ = { + "jti", + "sub", + "ref", + "sha", + "run_id", + "run_number", + "run_attempt", + "head_ref", + "base_ref", + "event_name", + "ref_type", + } + + @property def repository(self): return f"{self.owner}/{self.repository_name}" + @property def job_workflow_ref(self): return f"{self.repository}/.github/workflows/{self.workflow_name}.yml" - - def verify_claims(self, signed_token): - """ - Given a JWT that has been successfully decoded (checked for a valid - signature and basic claims), verify it against the more specific - claims of this provider. - """ - return NotImplemented diff --git a/warehouse/oidc/services.py b/warehouse/oidc/services.py index cbe818f36789..4b70193a2033 100644 --- a/warehouse/oidc/services.py +++ b/warehouse/oidc/services.py @@ -160,7 +160,7 @@ def _get_key_for_token(self, token): unverified_header = jwt.get_unverified_header(token) return self.get_key(unverified_header["kid"]) - def verify(self, token): + def verify_signature_only(self, token): key = self._get_key_for_token(token) try: From 63d16a2397aaaf1a6bebb47732dcf747d62fcc42 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 15 Feb 2022 13:46:45 -0500 Subject: [PATCH 08/78] oidc/models: fill in missing properties --- warehouse/oidc/models.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/warehouse/oidc/models.py b/warehouse/oidc/models.py index 3b81aa8d4a7a..7532bb29f872 100644 --- a/warehouse/oidc/models.py +++ b/warehouse/oidc/models.py @@ -114,3 +114,11 @@ def repository(self): @property def job_workflow_ref(self): return f"{self.repository}/.github/workflows/{self.workflow_name}.yml" + + @property + def actor(self): + return self.owner + + @property + def workflow(self): + return self.workflow_name From 50549ec7f1bd10f7d03fde2212cc5344b4accc4a Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 15 Feb 2022 13:48:34 -0500 Subject: [PATCH 09/78] warehouse/migrations: remove original OIDC migration Add many-many project-provider association. --- ...6d5769_add_initial_oidc_provider_models.py | 71 ------------------- warehouse/oidc/models.py | 20 +++++- 2 files changed, 19 insertions(+), 72 deletions(-) delete mode 100644 warehouse/migrations/versions/47422b6d5769_add_initial_oidc_provider_models.py diff --git a/warehouse/migrations/versions/47422b6d5769_add_initial_oidc_provider_models.py b/warehouse/migrations/versions/47422b6d5769_add_initial_oidc_provider_models.py deleted file mode 100644 index 0fa9c2bebb09..000000000000 --- a/warehouse/migrations/versions/47422b6d5769_add_initial_oidc_provider_models.py +++ /dev/null @@ -1,71 +0,0 @@ -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -""" -Add initial OIDC provider models - -Revision ID: 47422b6d5769 -Revises: 29a8901a4635 -Create Date: 2022-02-14 20:45:29.641650 -""" - -import sqlalchemy as sa - -from alembic import op -from sqlalchemy.dialects import postgresql - -revision = "47422b6d5769" -down_revision = "29a8901a4635" - -# Note: It is VERY important to ensure that a migration does not lock for a -# long period of time and to ensure that each individual migration does -# not break compatibility with the *previous* version of the code base. -# This is because the migrations will be ran automatically as part of the -# deployment process, but while the previous version of the code is still -# up and running. Thus backwards incompatible changes must be broken up -# over multiple migrations inside of multiple pull requests in order to -# phase them in over multiple deploys. - - -def upgrade(): - # ### commands auto generated by Alembic - please adjust! ### - op.create_table( - "oidc_providers", - sa.Column( - "id", - postgresql.UUID(as_uuid=True), - server_default=sa.text("gen_random_uuid()"), - nullable=False, - ), - sa.Column("discriminator", sa.String(), nullable=True), - sa.PrimaryKeyConstraint("id"), - ) - op.create_table( - "github_oidc_providers", - sa.Column("id", postgresql.UUID(as_uuid=True), nullable=False), - sa.Column("repository_name", sa.String(), nullable=True), - sa.Column("owner", sa.String(), nullable=True), - sa.Column("owner_id", sa.String(), nullable=True), - sa.Column("workflow_name", sa.String(), nullable=True), - sa.ForeignKeyConstraint( - ["id"], - ["oidc_providers.id"], - ), - sa.PrimaryKeyConstraint("id"), - ) - # ### end Alembic commands ### - - -def downgrade(): - # ### commands auto generated by Alembic - please adjust! ### - op.drop_table("github_oidc_providers") - op.drop_table("oidc_providers") - # ### end Alembic commands ### diff --git a/warehouse/oidc/models.py b/warehouse/oidc/models.py index 7532bb29f872..6158f6c2f855 100644 --- a/warehouse/oidc/models.py +++ b/warehouse/oidc/models.py @@ -12,16 +12,34 @@ import sentry_sdk -from sqlalchemy import Column, ForeignKey, String +from sqlalchemy import Column, ForeignKey, String, orm from sqlalchemy.dialects.postgresql import UUID from warehouse import db +from warehouse.packaging.models import Project + + +class OIDCProviderProjectAssociation(db.Model): + __tablename__ = "oidc_provider_project_association" + + oidc_provider_id = Column( + UUID(as_uuid=True), + ForeignKey("oidc_providers.id"), + nullable=False, + primary_key=True, + ) + project_id = Column( + UUID(as_uuid=True), ForeignKey("projects.id"), nullable=False, primary_key=True + ) class OIDCProvider(db.Model): __tablename__ = "oidc_providers" discriminator = Column(String) + projects = orm.relationship( + Project, secondary=OIDCProviderProjectAssociation, backref="oidc_providers" + ) __mapper_args__ = {"polymorphic_on": discriminator} From f1d7162fc2d97510ddc85cff2be450f43ddfba5b Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 15 Feb 2022 16:12:02 -0500 Subject: [PATCH 10/78] warehouse: add OIDC migration, fix association --- ...4c444f_add_initial_oidc_provider_models.py | 68 +++++++++++++++++++ warehouse/oidc/models.py | 2 +- 2 files changed, 69 insertions(+), 1 deletion(-) create mode 100644 warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py diff --git a/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py b/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py new file mode 100644 index 000000000000..5a51874140bc --- /dev/null +++ b/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py @@ -0,0 +1,68 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +""" +Add initial OIDC provider models + +Revision ID: f345394c444f +Revises: 29a8901a4635 +Create Date: 2022-02-15 21:11:41.693791 +""" + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = 'f345394c444f' +down_revision = '29a8901a4635' + +# Note: It is VERY important to ensure that a migration does not lock for a +# long period of time and to ensure that each individual migration does +# not break compatibility with the *previous* version of the code base. +# This is because the migrations will be ran automatically as part of the +# deployment process, but while the previous version of the code is still +# up and running. Thus backwards incompatible changes must be broken up +# over multiple migrations inside of multiple pull requests in order to +# phase them in over multiple deploys. + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.create_table('oidc_providers', + sa.Column('id', postgresql.UUID(as_uuid=True), server_default=sa.text('gen_random_uuid()'), nullable=False), + sa.Column('discriminator', sa.String(), nullable=True), + sa.PrimaryKeyConstraint('id') + ) + op.create_table('github_oidc_providers', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('repository_name', sa.String(), nullable=True), + sa.Column('owner', sa.String(), nullable=True), + sa.Column('owner_id', sa.String(), nullable=True), + sa.Column('workflow_name', sa.String(), nullable=True), + sa.ForeignKeyConstraint(['id'], ['oidc_providers.id'], ), + sa.PrimaryKeyConstraint('id') + ) + op.create_table('oidc_provider_project_association', + sa.Column('id', postgresql.UUID(as_uuid=True), server_default=sa.text('gen_random_uuid()'), nullable=False), + sa.Column('oidc_provider_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('project_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.ForeignKeyConstraint(['oidc_provider_id'], ['oidc_providers.id'], ), + sa.ForeignKeyConstraint(['project_id'], ['projects.id'], ), + sa.PrimaryKeyConstraint('id', 'oidc_provider_id', 'project_id') + ) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_table('oidc_provider_project_association') + op.drop_table('github_oidc_providers') + op.drop_table('oidc_providers') + # ### end Alembic commands ### diff --git a/warehouse/oidc/models.py b/warehouse/oidc/models.py index 6158f6c2f855..f098ba05c9d5 100644 --- a/warehouse/oidc/models.py +++ b/warehouse/oidc/models.py @@ -38,7 +38,7 @@ class OIDCProvider(db.Model): discriminator = Column(String) projects = orm.relationship( - Project, secondary=OIDCProviderProjectAssociation, backref="oidc_providers" + Project, secondary=OIDCProviderProjectAssociation.__table__, backref="oidc_providers" ) __mapper_args__ = {"polymorphic_on": discriminator} From 5479c8102926b6603560e2a97511b5a99ae2db19 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 15 Feb 2022 16:12:34 -0500 Subject: [PATCH 11/78] warehouse: reformat --- ...4c444f_add_initial_oidc_provider_models.py | 74 ++++++++++++------- warehouse/oidc/models.py | 4 +- 2 files changed, 52 insertions(+), 26 deletions(-) diff --git a/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py b/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py index 5a51874140bc..bbea7edfbb31 100644 --- a/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py +++ b/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py @@ -17,12 +17,13 @@ Create Date: 2022-02-15 21:11:41.693791 """ -from alembic import op import sqlalchemy as sa + +from alembic import op from sqlalchemy.dialects import postgresql -revision = 'f345394c444f' -down_revision = '29a8901a4635' +revision = "f345394c444f" +down_revision = "29a8901a4635" # Note: It is VERY important to ensure that a migration does not lock for a # long period of time and to ensure that each individual migration does @@ -33,36 +34,59 @@ # over multiple migrations inside of multiple pull requests in order to # phase them in over multiple deploys. + def upgrade(): # ### commands auto generated by Alembic - please adjust! ### - op.create_table('oidc_providers', - sa.Column('id', postgresql.UUID(as_uuid=True), server_default=sa.text('gen_random_uuid()'), nullable=False), - sa.Column('discriminator', sa.String(), nullable=True), - sa.PrimaryKeyConstraint('id') + op.create_table( + "oidc_providers", + sa.Column( + "id", + postgresql.UUID(as_uuid=True), + server_default=sa.text("gen_random_uuid()"), + nullable=False, + ), + sa.Column("discriminator", sa.String(), nullable=True), + sa.PrimaryKeyConstraint("id"), ) - op.create_table('github_oidc_providers', - sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), - sa.Column('repository_name', sa.String(), nullable=True), - sa.Column('owner', sa.String(), nullable=True), - sa.Column('owner_id', sa.String(), nullable=True), - sa.Column('workflow_name', sa.String(), nullable=True), - sa.ForeignKeyConstraint(['id'], ['oidc_providers.id'], ), - sa.PrimaryKeyConstraint('id') + op.create_table( + "github_oidc_providers", + sa.Column("id", postgresql.UUID(as_uuid=True), nullable=False), + sa.Column("repository_name", sa.String(), nullable=True), + sa.Column("owner", sa.String(), nullable=True), + sa.Column("owner_id", sa.String(), nullable=True), + sa.Column("workflow_name", sa.String(), nullable=True), + sa.ForeignKeyConstraint( + ["id"], + ["oidc_providers.id"], + ), + sa.PrimaryKeyConstraint("id"), ) - op.create_table('oidc_provider_project_association', - sa.Column('id', postgresql.UUID(as_uuid=True), server_default=sa.text('gen_random_uuid()'), nullable=False), - sa.Column('oidc_provider_id', postgresql.UUID(as_uuid=True), nullable=False), - sa.Column('project_id', postgresql.UUID(as_uuid=True), nullable=False), - sa.ForeignKeyConstraint(['oidc_provider_id'], ['oidc_providers.id'], ), - sa.ForeignKeyConstraint(['project_id'], ['projects.id'], ), - sa.PrimaryKeyConstraint('id', 'oidc_provider_id', 'project_id') + op.create_table( + "oidc_provider_project_association", + sa.Column( + "id", + postgresql.UUID(as_uuid=True), + server_default=sa.text("gen_random_uuid()"), + nullable=False, + ), + sa.Column("oidc_provider_id", postgresql.UUID(as_uuid=True), nullable=False), + sa.Column("project_id", postgresql.UUID(as_uuid=True), nullable=False), + sa.ForeignKeyConstraint( + ["oidc_provider_id"], + ["oidc_providers.id"], + ), + sa.ForeignKeyConstraint( + ["project_id"], + ["projects.id"], + ), + sa.PrimaryKeyConstraint("id", "oidc_provider_id", "project_id"), ) # ### end Alembic commands ### def downgrade(): # ### commands auto generated by Alembic - please adjust! ### - op.drop_table('oidc_provider_project_association') - op.drop_table('github_oidc_providers') - op.drop_table('oidc_providers') + op.drop_table("oidc_provider_project_association") + op.drop_table("github_oidc_providers") + op.drop_table("oidc_providers") # ### end Alembic commands ### diff --git a/warehouse/oidc/models.py b/warehouse/oidc/models.py index f098ba05c9d5..19a68fb20889 100644 --- a/warehouse/oidc/models.py +++ b/warehouse/oidc/models.py @@ -38,7 +38,9 @@ class OIDCProvider(db.Model): discriminator = Column(String) projects = orm.relationship( - Project, secondary=OIDCProviderProjectAssociation.__table__, backref="oidc_providers" + Project, + secondary=OIDCProviderProjectAssociation.__table__, + backref="oidc_providers", ) __mapper_args__ = {"polymorphic_on": discriminator} From 1e0f26c0f8dba319736ec16d380928b708cd918c Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Wed, 16 Feb 2022 14:09:34 -0500 Subject: [PATCH 12/78] warehouse: OIDC route/view skeleton work --- warehouse/manage/views.py | 13 ++++++++++++ warehouse/routes.py | 7 +++++++ warehouse/templates/manage/oidc.html | 14 +++++++++++++ warehouse/templates/manage/settings.html | 26 ++++++++++++++++++++++++ 4 files changed, 60 insertions(+) create mode 100644 warehouse/templates/manage/oidc.html diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index cdd564507eac..808fc2cf97bd 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -992,6 +992,19 @@ def manage_project_settings(project, request): } +@view_config( + route_name="manage.project.settings.oidc", + context=Project, + renderer="manage/oidc.html", + uses_session=True, + permission="manage:project", + has_translations=True, + require_reauth=True, +) +def manage_project_oidc(project, request): + return {"project": project} + + def get_user_role_in_project(project, user, request): return ( request.db.query(Role) diff --git a/warehouse/routes.py b/warehouse/routes.py index e9d6006af7ed..b0604d809b5e 100644 --- a/warehouse/routes.py +++ b/warehouse/routes.py @@ -229,6 +229,13 @@ def includeme(config): traverse="/{project_name}", domain=warehouse, ) + config.add_route( + "manage.project.settings.oidc", + "/manage/project/{project_name}/settings/oidc/", + factory="warehouse.packaging.models:ProjectFactory", + traverse="/{project_name}", + domain=warehouse, + ) config.add_route( "manage.project.delete_project", "/manage/project/{project_name}/delete_project/", diff --git a/warehouse/templates/manage/oidc.html b/warehouse/templates/manage/oidc.html new file mode 100644 index 000000000000..89780d9dc9ef --- /dev/null +++ b/warehouse/templates/manage/oidc.html @@ -0,0 +1,14 @@ +{# + # Licensed under the Apache License, Version 2.0 (the "License"); + # you may not use this file except in compliance with the License. + # You may obtain a copy of the License at + # + # http://www.apache.org/licenses/LICENSE-2.0 + # + # Unless required by applicable law or agreed to in writing, software + # distributed under the License is distributed on an "AS IS" BASIS, + # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + # See the License for the specific language governing permissions and + # limitations under the License. +-#} +{% extends "manage_project_base.html" %} diff --git a/warehouse/templates/manage/settings.html b/warehouse/templates/manage/settings.html index 218a4f17a6ae..6f3310fdd2cc 100644 --- a/warehouse/templates/manage/settings.html +++ b/warehouse/templates/manage/settings.html @@ -55,6 +55,32 @@

{% trans %}API tokens{% endtrans %}

{% endtrans %}

{% endif %} + +

{% trans %}OIDC providers{% endtrans %}

+

+ {% trans %} + OIDC providers provide a credential-free way to authenticate when uploading packages to PyPI. + {% endtrans %} +

+ +

+ {% if project.oidc_providers %} +

{% trans %}Current providers:{% endtrans %}

+
    + {% for provider in project.oidc_providers %} +
  • {{ provider }}
  • + {% endfor %} +
+ {% else %} +

{% trans %}No providers are currently configured.{% endtrans %}

+ {% endif %} + + {% trans href=request.route_path('manage.project.settings.oidc'), project_name=project.name %} + + Add an OIDC provider to {{ project_name }} + + {% endtrans %} +


From bf5859bb3b33d521f72a0ad0f528774cb7f49530 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Thu, 17 Feb 2022 18:13:30 -0500 Subject: [PATCH 13/78] warehouse: form, view logic for adding OIDC providers --- warehouse/manage/views.py | 58 ++++++++++++++-- warehouse/oidc/forms.py | 80 ++++++++++++++++++++++ warehouse/oidc/models.py | 19 +++++- warehouse/templates/manage/oidc.html | 84 +++++++++++++++++++++++- warehouse/templates/manage/settings.html | 20 ++---- 5 files changed, 238 insertions(+), 23 deletions(-) create mode 100644 warehouse/oidc/forms.py diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index 808fc2cf97bd..414fb7ef3fda 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -71,6 +71,8 @@ ProvisionWebAuthnForm, SaveAccountForm, ) +from warehouse.oidc.forms import GitHubProviderForm +from warehouse.oidc.models import GitHubProvider from warehouse.packaging.models import ( File, JournalEntry, @@ -992,17 +994,65 @@ def manage_project_settings(project, request): } -@view_config( - route_name="manage.project.settings.oidc", +@view_defaults( context=Project, + route_name="manage.project.settings.oidc", renderer="manage/oidc.html", uses_session=True, + require_csrf=True, + require_methods=False, permission="manage:project", has_translations=True, require_reauth=True, ) -def manage_project_oidc(project, request): - return {"project": project} +class ManageOIDCProviderViews: + def __init__(self, project, request): + self.request = request + self.project = project + + @property + def default_response(self): + return { + "project": self.project, + "github_provider_form": GitHubProviderForm(), + } + + @view_config(request_method="GET") + def manage_project_oidc_providers(self): + return self.default_response + + @view_config(request_method="POST") + def add_github_oidc_provider(self, _form_class=GitHubProviderForm): + form = _form_class(self.request.POST) + + if form.validate(): + provider = GitHubProvider( + repository_name=form.repository, + owner=form.owner, + owner_id=form.owner_id, + workflow_name=form.workflow_name.data, + ) + + self.request.db.add(provider) + self.project.oidc_providers.append(provider) + + self.project.record_event( + tag="project:oidc:provider-added", + ip_address=self.request.remote_addr, + additional={ + "provider": "github", + "repository": form.repository_slug.data, + "workflow": form.workflow_name.data, + }, + ) + + self.request.session.flash( + f"Added {form.workflow_name.data} on {form.repository_slug.data} " + f"to {self.project.name}", + queue="success", + ) + + return {**self.default_response, "github_provider_form": form} def get_user_role_in_project(project, user, request): diff --git a/warehouse/oidc/forms.py b/warehouse/oidc/forms.py new file mode 100644 index 000000000000..9890bf23c1b7 --- /dev/null +++ b/warehouse/oidc/forms.py @@ -0,0 +1,80 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import re + +import requests +import wtforms + +from warehouse import forms +from warehouse.i18n import localize as _ + +# This roughly matches the "owner/repo" convention used by GitHub. +_VALID_GITHUB_OWNER_REPO_SLUG = re.compile( + r"^[a-zA-Z0-9][a-zA-Z0-9-]*/[a-zA-Z0-9-_.]+$" +) + + +class GitHubProviderForm(forms.Form): + repository_slug = wtforms.StringField( + validators=[ + wtforms.validators.DataRequired(message="Specify repository slug"), + ] + ) + + workflow_name = wtforms.StringField( + validators=[wtforms.validators.DataRequired(message="Specify workflow name")] + ) + + def validate_repository_slug(self, field): + repository_slug = field.data + if not _VALID_GITHUB_OWNER_REPO_SLUG.fullmatch(repository_slug): + raise wtforms.validators.ValidationError( + _( + "The specified repository is invalid. Repositories must be " + "specified in owner/repo format." + ) + ) + + owner, repository = repository_slug.split("/", 1) + + # To actually validate the owner, we ask GitHub's API about them. + # We can't do this for the repository, since it might be private. + response = requests.get( + f"https://api.github.com/users/{owner}", + headers={"Accept": "application/vnd.github.v3+json"}, + allow_redirects=True, + ) + + if response.status_code == 404: + raise wtforms.validators.ValidationError( + _("Unknown GitHub user or organization.") + ) + elif not response.ok: + raise wtforms.validators.ValidationError( + _("Unexpected error from GitHub. Try again.") + ) + + owner_info = response.json() + + # NOTE: Use the normalized owner name as provided by GitHub. + self.owner = owner_info["login"] + self.owner_id = owner_info["id"] + self.repository = repository + + def validate_workflow_name(self, field): + workflow_name = field.data + + if not (workflow_name.endswith(".yml") or workflow_name.endswith(".yaml")): + raise wtforms.validators.ValidationError( + _("Workflow name must end with .yml or .yaml") + ) diff --git a/warehouse/oidc/models.py b/warehouse/oidc/models.py index 19a68fb20889..e3e3ba5418f8 100644 --- a/warehouse/oidc/models.py +++ b/warehouse/oidc/models.py @@ -43,7 +43,10 @@ class OIDCProvider(db.Model): backref="oidc_providers", ) - __mapper_args__ = {"polymorphic_on": discriminator} + __mapper_args__ = { + "polymorphic_identity": "oidc_providers", + "polymorphic_on": discriminator, + } # A map of claim names to "check" functions, each of which # has the signature `check(ground-truth, signed-claim) -> bool`. @@ -85,7 +88,8 @@ def verify_claims(self, signed_claims): unaccounted_claims = known_claims.difference(signed_claims.keys()) if unaccounted_claims: sentry_sdk.capture_message( - f"JWT for {self.__class__.__name__} has unaccounted claims: {unaccounted_claims}" + f"JWT for {self.__class__.__name__} has unaccounted claims: " + f"{unaccounted_claims}" ) # Finally, perform the actual claim verification. @@ -95,10 +99,15 @@ def verify_claims(self, signed_claims): return True + @property + def provider_name(): + # Only concrete subclasses of OIDCProvider are constructed. + return NotImplemented + class GitHubProvider(OIDCProvider): __tablename__ = "github_oidc_providers" - __mapper_args__ = {"polymorphic_identity": "GitHubProvider"} + __mapper_args__ = {"polymorphic_identity": "github_oidc_providers"} id = Column(UUID(as_uuid=True), ForeignKey(OIDCProvider.id), primary_key=True) repository_name = Column(String) @@ -127,6 +136,10 @@ class GitHubProvider(OIDCProvider): "ref_type", } + @property + def provider_name(): + return "github" + @property def repository(self): return f"{self.owner}/{self.repository_name}" diff --git a/warehouse/templates/manage/oidc.html b/warehouse/templates/manage/oidc.html index 89780d9dc9ef..28b94cec2c5f 100644 --- a/warehouse/templates/manage/oidc.html +++ b/warehouse/templates/manage/oidc.html @@ -11,4 +11,86 @@ # See the License for the specific language governing permissions and # limitations under the License. -#} -{% extends "manage_project_base.html" %} + +{% extends "manage_base.html" %} + +{% block title %} + {% trans %}OIDC provider management{% endtrans %} +{% endblock %} + +{% block content %} +{% if testPyPI %} +{% set title = "TestPyPI" %} +{% else %} +{% set title = "PyPI" %} +{% endif %} + +
+
+

{% trans %}OIDC provider management{% endtrans %}

+ +

+ {% trans trimmed %} + OIDC provides a flexible, credential-free mechanism for delegating + publishing authority for a PyPI package to a third party service, + like GitHub Actions. + {% endtrans %} +

+ +

+ {% trans trimmed %} + PyPI projects can use trusted OIDC providers to automate their release + processes, without having to explicitly provision or manage API tokens. + {% endtrans %} +

+ +

{% trans %}Add a new provider{% endtrans %}

+ +

GitHub

+ +
+
+ + + {{ github_provider_form.repository_slug(placeholder=gettext("owner/repo"), autocomplete="off", autocapitalize="off", spellcheck="false", class_="form-group__field", **{"aria-describedby":"repository-slug-errors"}) }} +
+ {{ field_errors(github_provider_form.repository_slug) }} +
+
+
+ + {{ github_provider_form.workflow_name(placeholder=gettext("workflow.yml"), class_="form-group__field", autocomplete="off", **{"aria-describedby":"workflow-name-errors"}) }} +
+ {{ field_errors(github_provider_form.workflow_name) }} +
+
+
+ +
+
+ +

{% trans %}Manage current providers{% endtrans %}

+ {% if project.oidc_providers %} +
    + {% for provider in project.oidc_providers %} +
  • {{ provider }}
  • + {% endfor %} +
+ {% else %} +

{% trans %}No providers are currently configured.{% endtrans %}

+ {% endif %} +
+
+ +{% endblock %} + diff --git a/warehouse/templates/manage/settings.html b/warehouse/templates/manage/settings.html index 6f3310fdd2cc..a2a0f21e1e47 100644 --- a/warehouse/templates/manage/settings.html +++ b/warehouse/templates/manage/settings.html @@ -62,25 +62,15 @@

{% trans %}OIDC providers{% endtrans %}

OIDC providers provide a credential-free way to authenticate when uploading packages to PyPI. {% endtrans %}

- -

- {% if project.oidc_providers %} -

{% trans %}Current providers:{% endtrans %}

-
    - {% for provider in project.oidc_providers %} -
  • {{ provider }}
  • - {% endfor %} -
- {% else %} -

{% trans %}No providers are currently configured.{% endtrans %}

- {% endif %} - - {% trans href=request.route_path('manage.project.settings.oidc'), project_name=project.name %} + +

+ {% trans href=request.route_path('manage.project.settings.oidc', project_name=project.name), project_name=project.name %} - Add an OIDC provider to {{ project_name }} + Manage OIDC providers for {{ project_name }} {% endtrans %}

+
From a62a9170fa69a87f9d46f803091bd066ea83c6a3 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Thu, 17 Feb 2022 18:33:03 -0500 Subject: [PATCH 14/78] manage/views: disable HTTP cache, add TODO --- warehouse/manage/views.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index 414fb7ef3fda..e60e644027db 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -1004,6 +1004,7 @@ def manage_project_settings(project, request): permission="manage:project", has_translations=True, require_reauth=True, + http_cache=0, ) class ManageOIDCProviderViews: def __init__(self, project, request): @@ -1054,6 +1055,8 @@ def add_github_oidc_provider(self, _form_class=GitHubProviderForm): return {**self.default_response, "github_provider_form": form} + # TODO: DELETE handler for removing OIDC providers + def get_user_role_in_project(project, user, request): return ( From ed1559c8598a52e3332ea632fcdfc89edb8f9abb Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 18 Feb 2022 14:38:24 -0500 Subject: [PATCH 15/78] warehouse: move oidc views to "publishing" ...and make it a sub-page for project management. --- warehouse/manage/views.py | 2 +- .../templates/includes/manage/manage-project-menu.html | 6 ++++++ warehouse/templates/manage/{oidc.html => publishing.html} | 6 ++++-- 3 files changed, 11 insertions(+), 3 deletions(-) rename warehouse/templates/manage/{oidc.html => publishing.html} (97%) diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index e60e644027db..e0372d5f9625 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -997,7 +997,7 @@ def manage_project_settings(project, request): @view_defaults( context=Project, route_name="manage.project.settings.oidc", - renderer="manage/oidc.html", + renderer="manage/publishing.html", uses_session=True, require_csrf=True, require_methods=False, diff --git a/warehouse/templates/includes/manage/manage-project-menu.html b/warehouse/templates/includes/manage/manage-project-menu.html index 62350ee3a4d7..3172ce89ee43 100644 --- a/warehouse/templates/includes/manage/manage-project-menu.html +++ b/warehouse/templates/includes/manage/manage-project-menu.html @@ -45,6 +45,12 @@ {% endif %} +
  • + + + {% trans %}Publishing{% endtrans %} + +
  • diff --git a/warehouse/templates/manage/oidc.html b/warehouse/templates/manage/publishing.html similarity index 97% rename from warehouse/templates/manage/oidc.html rename to warehouse/templates/manage/publishing.html index 28b94cec2c5f..22f863a248ae 100644 --- a/warehouse/templates/manage/oidc.html +++ b/warehouse/templates/manage/publishing.html @@ -12,13 +12,15 @@ # limitations under the License. -#} -{% extends "manage_base.html" %} +{% extends "manage_project_base.html" %} + +{% set active_tab = 'publishing' %} {% block title %} {% trans %}OIDC provider management{% endtrans %} {% endblock %} -{% block content %} +{% block main %} {% if testPyPI %} {% set title = "TestPyPI" %} {% else %} From 971ccf2f5a86cd209440949b197c685a4f5a4254 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 18 Feb 2022 16:18:19 -0500 Subject: [PATCH 16/78] warehouse: provider deletion routing --- warehouse/manage/views.py | 26 +++++++++----- warehouse/oidc/forms.py | 21 +++++++++-- warehouse/oidc/models.py | 13 +++++-- warehouse/templates/manage/account.html | 4 +-- warehouse/templates/manage/publishing.html | 41 +++++++++++++++++++--- 5 files changed, 85 insertions(+), 20 deletions(-) diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index e0372d5f9625..a129e9e7f420 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -71,8 +71,8 @@ ProvisionWebAuthnForm, SaveAccountForm, ) -from warehouse.oidc.forms import GitHubProviderForm -from warehouse.oidc.models import GitHubProvider +from warehouse.oidc.forms import DeleteProviderForm, GitHubProviderForm +from warehouse.oidc.models import GitHubProvider, OIDCProvider from warehouse.packaging.models import ( File, JournalEntry, @@ -1022,9 +1022,9 @@ def default_response(self): def manage_project_oidc_providers(self): return self.default_response - @view_config(request_method="POST") - def add_github_oidc_provider(self, _form_class=GitHubProviderForm): - form = _form_class(self.request.POST) + @view_config(request_method="POST", request_param=GitHubProviderForm.__params__) + def add_github_oidc_provider(self): + form = GitHubProviderForm(self.request.POST) if form.validate(): provider = GitHubProvider( @@ -1048,14 +1048,24 @@ def add_github_oidc_provider(self, _form_class=GitHubProviderForm): ) self.request.session.flash( - f"Added {form.workflow_name.data} on {form.repository_slug.data} " - f"to {self.project.name}", + f"Added {provider} to {self.project.name}", queue="success", ) return {**self.default_response, "github_provider_form": form} - # TODO: DELETE handler for removing OIDC providers + @view_config(request_method="POST", request_param=DeleteProviderForm.__params__) + def delete_oidc_provider(self): + form = DeleteProviderForm(self.request.POST) + + if form.validate(): + provider = self.request.db.query(OIDCProvider).get(form.provider_id.data) + # TODO: Check that provider is not None here? + self.project.oidc_providers.remove(provider) + + self.request.session.flash(f"Removed {provider} from {self.project.name}") + + return self.default_response def get_user_role_in_project(project, user, request): diff --git a/warehouse/oidc/forms.py b/warehouse/oidc/forms.py index 9890bf23c1b7..ba8bfa0f0c26 100644 --- a/warehouse/oidc/forms.py +++ b/warehouse/oidc/forms.py @@ -25,14 +25,16 @@ class GitHubProviderForm(forms.Form): + __params__ = ["repository_slug", "workflow_name"] + repository_slug = wtforms.StringField( validators=[ - wtforms.validators.DataRequired(message="Specify repository slug"), + wtforms.validators.DataRequired(message=_("Specify repository slug")), ] ) workflow_name = wtforms.StringField( - validators=[wtforms.validators.DataRequired(message="Specify workflow name")] + validators=[wtforms.validators.DataRequired(message=_("Specify workflow name"))] ) def validate_repository_slug(self, field): @@ -78,3 +80,18 @@ def validate_workflow_name(self, field): raise wtforms.validators.ValidationError( _("Workflow name must end with .yml or .yaml") ) + + if "/" in workflow_name: + raise wtforms.validators.ValidationError( + _("Workflow name must be a basename, without directories") + ) + + +class DeleteProviderForm(forms.Form): + __params__ = ["provider_id"] + + provider_id = wtforms.StringField( + validators=[ + wtforms.validators.UUID(message=_("Provider must be specified by ID")) + ] + ) diff --git a/warehouse/oidc/models.py b/warehouse/oidc/models.py index e3e3ba5418f8..50e22e9e3067 100644 --- a/warehouse/oidc/models.py +++ b/warehouse/oidc/models.py @@ -100,7 +100,7 @@ def verify_claims(self, signed_claims): return True @property - def provider_name(): + def provider_name(self): # Only concrete subclasses of OIDCProvider are constructed. return NotImplemented @@ -109,6 +109,10 @@ class GitHubProvider(OIDCProvider): __tablename__ = "github_oidc_providers" __mapper_args__ = {"polymorphic_identity": "github_oidc_providers"} + # TODO: We could make the constraints here smarter, e.g. make every + # GitHubProvider unique on (repository_name, owner_id, workflow_name). + # Is it worth it? + id = Column(UUID(as_uuid=True), ForeignKey(OIDCProvider.id), primary_key=True) repository_name = Column(String) owner = Column(String) @@ -137,8 +141,8 @@ class GitHubProvider(OIDCProvider): } @property - def provider_name(): - return "github" + def provider_name(self): + return "GitHub" @property def repository(self): @@ -155,3 +159,6 @@ def actor(self): @property def workflow(self): return self.workflow_name + + def __str__(self): + return f"{self.workflow_name} @ {self.repository}" diff --git a/warehouse/templates/manage/account.html b/warehouse/templates/manage/account.html index ab386455972a..d4be82cae493 100644 --- a/warehouse/templates/manage/account.html +++ b/warehouse/templates/manage/account.html @@ -69,9 +69,9 @@ {% macro email_row(email) -%} - + {{ email.email }} - + {% if email.primary %} diff --git a/warehouse/templates/manage/publishing.html b/warehouse/templates/manage/publishing.html index 22f863a248ae..f26b74636fd9 100644 --- a/warehouse/templates/manage/publishing.html +++ b/warehouse/templates/manage/publishing.html @@ -27,6 +27,25 @@ {% set title = "PyPI" %} {% endif %} +{% macro provider_row(provider) -%} +
    + + +
    + + + + {{ provider.provider_name }} + + + {{ provider|string }} + + + + + +{%- endmacro %} +

    {% trans %}OIDC provider management{% endtrans %}

    @@ -83,11 +102,23 @@

    GitHub

    {% trans %}Manage current providers{% endtrans %}

    {% if project.oidc_providers %} -
      - {% for provider in project.oidc_providers %} -
    • {{ provider }}
    • - {% endfor %} -
    + + + + + + + + + + + {% for provider in project.oidc_providers %} + {{ provider_row(provider) }} + {% endfor %} + +
    + {% trans project_name=project.name %}OIDC providers associated with {{ project_name }}{% endtrans %} +
    {% trans %}Provider{% endtrans %}{% trans %}Specification{% endtrans %}
    {% else %}

    {% trans %}No providers are currently configured.{% endtrans %}

    {% endif %} From 4aad8eeff22d9a03c875d5f1060394dad1ae9858 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 18 Feb 2022 16:49:59 -0500 Subject: [PATCH 17/78] warehouse: shore up constraints, better error flashes --- warehouse/manage/views.py | 59 +++++++++++++++---- ...4c444f_add_initial_oidc_provider_models.py | 3 + warehouse/oidc/models.py | 14 +++-- 3 files changed, 61 insertions(+), 15 deletions(-) diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index a129e9e7f420..75367fdc2df1 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -1027,21 +1027,45 @@ def add_github_oidc_provider(self): form = GitHubProviderForm(self.request.POST) if form.validate(): - provider = GitHubProvider( - repository_name=form.repository, - owner=form.owner, - owner_id=form.owner_id, - workflow_name=form.workflow_name.data, - ) + # GitHub OIDC providers are unique on the tuple of + # (repository_name, owner, workflow_name), so we check for + # an already registered one before creating. + provider = ( + self.request.db.query(GitHubProvider) + .filter( + GitHubProvider.repository_name == form.repository, + GitHubProvider.owner == form.owner, + GitHubProvider.workflow_name == form.workflow_name.data, + ) + .one_or_none() + ) + if provider is None: + provider = GitHubProvider( + repository_name=form.repository, + owner=form.owner, + owner_id=form.owner_id, + workflow_name=form.workflow_name.data, + ) + + self.request.db.add(provider) + + # Each project has a unique registration of OIDC providers, + # even though the relationship is many-many. + if provider in self.project.oidc_providers: + self.request.session.flash( + f"{provider} is already registered with {self.project.name}", + queue="error", + ) + return {**self.default_response, "github_provider_form": form} - self.request.db.add(provider) self.project.oidc_providers.append(provider) self.project.record_event( tag="project:oidc:provider-added", ip_address=self.request.remote_addr, additional={ - "provider": "github", + "provider": provider.provider_name, + "id": str(provider.id), "repository": form.repository_slug.data, "workflow": form.workflow_name.data, }, @@ -1052,7 +1076,7 @@ def add_github_oidc_provider(self): queue="success", ) - return {**self.default_response, "github_provider_form": form} + return self.default_response @view_config(request_method="POST", request_param=DeleteProviderForm.__params__) def delete_oidc_provider(self): @@ -1060,9 +1084,24 @@ def delete_oidc_provider(self): if form.validate(): provider = self.request.db.query(OIDCProvider).get(form.provider_id.data) - # TODO: Check that provider is not None here? + if provider not in self.project.oidc_providers: + self.request.session.flash( + f"{provider} is not registered to {self.project.name}", + queue="error", + ) + return self.default_response + self.project.oidc_providers.remove(provider) + self.project.record_event( + tag="project:oidc:provider-removed", + ip_address=self.request.remote_addr, + additional={ + "provider": provider.provider_name, + "id": str(provider.id), + }, + ) + self.request.session.flash(f"Removed {provider} from {self.project.name}") return self.default_response diff --git a/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py b/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py index bbea7edfbb31..35ca6701110e 100644 --- a/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py +++ b/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py @@ -60,6 +60,9 @@ def upgrade(): ["oidc_providers.id"], ), sa.PrimaryKeyConstraint("id"), + sa.UniqueConstraint( + "repository_name", "owner", "workflow_name", name="_github_oidc_provider_uc" + ), ) op.create_table( "oidc_provider_project_association", diff --git a/warehouse/oidc/models.py b/warehouse/oidc/models.py index 50e22e9e3067..9a8b2a7fc4a0 100644 --- a/warehouse/oidc/models.py +++ b/warehouse/oidc/models.py @@ -12,7 +12,7 @@ import sentry_sdk -from sqlalchemy import Column, ForeignKey, String, orm +from sqlalchemy import Column, ForeignKey, String, UniqueConstraint, orm from sqlalchemy.dialects.postgresql import UUID from warehouse import db @@ -108,10 +108,14 @@ def provider_name(self): class GitHubProvider(OIDCProvider): __tablename__ = "github_oidc_providers" __mapper_args__ = {"polymorphic_identity": "github_oidc_providers"} - - # TODO: We could make the constraints here smarter, e.g. make every - # GitHubProvider unique on (repository_name, owner_id, workflow_name). - # Is it worth it? + __table_args__ = ( + UniqueConstraint( + "repository_name", + "owner", + "workflow_name", + name="_github_oidc_provider_uc", + ), + ) id = Column(UUID(as_uuid=True), ForeignKey(OIDCProvider.id), primary_key=True) repository_name = Column(String) From 100120c2d8304a1badb049167184b1f755ef44dd Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 25 Feb 2022 15:09:57 -0500 Subject: [PATCH 18/78] warehouse/migrations: rebase revision --- .../versions/f345394c444f_add_initial_oidc_provider_models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py b/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py index 35ca6701110e..7bfc717a3ea0 100644 --- a/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py +++ b/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py @@ -13,7 +13,7 @@ Add initial OIDC provider models Revision ID: f345394c444f -Revises: 29a8901a4635 +Revises: 19cf76d2d459 Create Date: 2022-02-15 21:11:41.693791 """ @@ -23,7 +23,7 @@ from sqlalchemy.dialects import postgresql revision = "f345394c444f" -down_revision = "29a8901a4635" +down_revision = "19cf76d2d459" # Note: It is VERY important to ensure that a migration does not lock for a # long period of time and to ensure that each individual migration does From 1ca025abe8804b3d865e120d3da1cb2e71e643f6 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 25 Feb 2022 16:28:41 -0500 Subject: [PATCH 19/78] warehouse/templates: update OIDC language Refer to OIDC providers as "OpenID Connect publishers" --- warehouse/templates/manage/publishing.html | 14 +++++++------- warehouse/templates/manage/settings.html | 6 +++--- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/warehouse/templates/manage/publishing.html b/warehouse/templates/manage/publishing.html index f26b74636fd9..7d1d83ca4155 100644 --- a/warehouse/templates/manage/publishing.html +++ b/warehouse/templates/manage/publishing.html @@ -17,7 +17,7 @@ {% set active_tab = 'publishing' %} {% block title %} - {% trans %}OIDC provider management{% endtrans %} + {% trans %}OpenID Connect publisher management{% endtrans %} {% endblock %} {% block main %} @@ -48,11 +48,11 @@
    -

    {% trans %}OIDC provider management{% endtrans %}

    +

    {% trans %}OpenID Connect publisher management{% endtrans %}

    {% trans trimmed %} - OIDC provides a flexible, credential-free mechanism for delegating + OpenID Connect provides a flexible, credential-free mechanism for delegating publishing authority for a PyPI package to a third party service, like GitHub Actions. {% endtrans %} @@ -60,7 +60,7 @@

    {% trans %}OIDC provider management{% endtrans %}

    {% trans trimmed %} - PyPI projects can use trusted OIDC providers to automate their release + PyPI projects can use trusted OpenID Connect publishers to automate their release processes, without having to explicitly provision or manage API tokens. {% endtrans %}

    @@ -104,11 +104,11 @@

    {% trans %}Manage current providers{% endtrans %}

    {% if project.oidc_providers %} - + @@ -120,7 +120,7 @@

    {% trans %}Manage current providers{% endtrans %}

    - {% trans project_name=project.name %}OIDC providers associated with {{ project_name }}{% endtrans %} + {% trans project_name=project.name %}OpenID Connect publishers associated with {{ project_name }}{% endtrans %}
    {% trans %}Provider{% endtrans %}{% trans %}Publisher{% endtrans %} {% trans %}Specification{% endtrans %}
    {% else %} -

    {% trans %}No providers are currently configured.{% endtrans %}

    +

    {% trans %}No publishers are currently configured.{% endtrans %}

    {% endif %}
    diff --git a/warehouse/templates/manage/settings.html b/warehouse/templates/manage/settings.html index 8904af1470aa..2ca91174dc85 100644 --- a/warehouse/templates/manage/settings.html +++ b/warehouse/templates/manage/settings.html @@ -98,17 +98,17 @@

    {% trans %}2FA requirement{% {% endif %} -

    {% trans %}OIDC providers{% endtrans %}

    +

    {% trans %}OpenID Connect publishers{% endtrans %}

    {% trans %} - OIDC providers provide a credential-free way to authenticate when uploading packages to PyPI. + OpenID Connect publishers provide a credential-free way to authenticate when uploading packages to PyPI. {% endtrans %}

    {% trans href=request.route_path('manage.project.settings.oidc', project_name=project.name), project_name=project.name %} - Manage OIDC providers for {{ project_name }} + Manage OpenID Connect publishers for {{ project_name }} {% endtrans %}

    From d54dd38a644e06a0c1806a722706a5eaa9d73f85 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 25 Feb 2022 16:48:13 -0500 Subject: [PATCH 20/78] warehouse: OIDC rate limiting groundwork --- warehouse/config.py | 6 ++++++ warehouse/manage/__init__.py | 10 ++++++++++ 2 files changed, 16 insertions(+) diff --git a/warehouse/config.py b/warehouse/config.py index a758cb0895d5..3ac2c35efbcb 100644 --- a/warehouse/config.py +++ b/warehouse/config.py @@ -270,6 +270,12 @@ def configure(settings=None): "PASSWORD_RESET_RATELIMIT_STRING", default="5 per day", ) + maybe_set( + settings, + "warehouse.manage.oidc.provider_registration_string", + "OIDC_PROVIDER_REGISTRATION_RATELIMIT_STRING", + default="10 per day", + ) # 2FA feature flags maybe_set( diff --git a/warehouse/manage/__init__.py b/warehouse/manage/__init__.py index 7c5d6962193b..2346aa9c07c9 100644 --- a/warehouse/manage/__init__.py +++ b/warehouse/manage/__init__.py @@ -17,6 +17,7 @@ from warehouse.accounts.forms import ReAuthenticateForm from warehouse.accounts.interfaces import IUserService +from warehouse.rate_limiting import IRateLimiter, RateLimit DEFAULT_TIME_TO_REAUTH = 30 * 60 # 30 minutes @@ -62,3 +63,12 @@ def wrapped(context, request): def includeme(config): config.add_view_deriver(reauth_view, over="rendered_view", under="decorated_view") + + oidc_provider_registration_ratelimit_string = config.registry.settings.get( + "warehouse.manage.oidc.provider_registration_ratelimit_string" + ) + config.register_service_factory( + RateLimit(oidc_provider_registration_ratelimit_string), + IRateLimiter, + name="oidc.provider.register", + ) From c700c92e0b42abd7c87183ffa1eddb9b23afa224 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 25 Feb 2022 17:08:41 -0500 Subject: [PATCH 21/78] manage/views: clean up OIDC events --- warehouse/manage/views.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index ca4ba642a3e3..7ee304ce3d37 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -1121,8 +1121,7 @@ def add_github_oidc_provider(self): additional={ "provider": provider.provider_name, "id": str(provider.id), - "repository": form.repository_slug.data, - "workflow": form.workflow_name.data, + "specifier": str(provider), }, ) @@ -1154,6 +1153,7 @@ def delete_oidc_provider(self): additional={ "provider": provider.provider_name, "id": str(provider.id), + "specifier": str(provider), }, ) From d2159312460e1a492c3f85c8559ed81c4b3ddc2e Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 25 Feb 2022 17:27:11 -0500 Subject: [PATCH 22/78] warehouse: use GitHub token for API requests, when available --- warehouse/manage/views.py | 4 +++- warehouse/oidc/forms.py | 14 +++++++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index 7ee304ce3d37..f34d6c38892c 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -1070,7 +1070,9 @@ def __init__(self, project, request): def default_response(self): return { "project": self.project, - "github_provider_form": GitHubProviderForm(), + "github_provider_form": GitHubProviderForm( + api_token=self.request.registry.settings.get("github.token") + ), } @view_config(request_method="GET") diff --git a/warehouse/oidc/forms.py b/warehouse/oidc/forms.py index ba8bfa0f0c26..a7fd47269260 100644 --- a/warehouse/oidc/forms.py +++ b/warehouse/oidc/forms.py @@ -37,6 +37,15 @@ class GitHubProviderForm(forms.Form): validators=[wtforms.validators.DataRequired(message=_("Specify workflow name"))] ) + def __init__(self, *args, api_token, **kwargs): + super().__init__(*args, **kwargs) + self._api_token = api_token + + def _headers_auth(self): + if not self._api_token: + return {} + return {"Authorization": f"token {self._api_token}"} + def validate_repository_slug(self, field): repository_slug = field.data if not _VALID_GITHUB_OWNER_REPO_SLUG.fullmatch(repository_slug): @@ -53,7 +62,10 @@ def validate_repository_slug(self, field): # We can't do this for the repository, since it might be private. response = requests.get( f"https://api.github.com/users/{owner}", - headers={"Accept": "application/vnd.github.v3+json"}, + headers={ + "Accept": "application/vnd.github.v3+json", + **self._headers_auth(), + }, allow_redirects=True, ) From 232ae6f09d35c6e302931a66a61879a13f9868bf Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 25 Feb 2022 17:41:40 -0500 Subject: [PATCH 23/78] oidc/forms: special casing for rate limiting Record errors with Sentry. --- warehouse/oidc/forms.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/warehouse/oidc/forms.py b/warehouse/oidc/forms.py index a7fd47269260..90e86bbf7707 100644 --- a/warehouse/oidc/forms.py +++ b/warehouse/oidc/forms.py @@ -13,6 +13,7 @@ import re import requests +import sentry_sdk import wtforms from warehouse import forms @@ -73,7 +74,20 @@ def validate_repository_slug(self, field): raise wtforms.validators.ValidationError( _("Unknown GitHub user or organization.") ) + if response.status_code == 403: + # GitHub's API uses 403 to signal rate limiting, and returns a JSON + # blob explaining the reason. + sentry_sdk.capture_message( + "Exceeded GitHub rate limit for user lookups. " + f"Reason: {response.json()}" + ) + raise wtforms.validators.ValidationError( + _("GitHub has rate-limited this action. Try again in a few minutes.") + ) elif not response.ok: + sentry_sdk.capture_message( + f"Unexpected error from GitHub user lookup: {response.content=}" + ) raise wtforms.validators.ValidationError( _("Unexpected error from GitHub. Try again.") ) From 62a795ffb1e88b28155aeea8c20f87db5a970b94 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 25 Feb 2022 18:05:19 -0500 Subject: [PATCH 24/78] warehouse: split user/repo form inputs apart --- warehouse/manage/views.py | 13 +++++--- warehouse/oidc/forms.py | 36 ++++++++++------------ warehouse/templates/manage/publishing.html | 20 +++++++++--- 3 files changed, 41 insertions(+), 28 deletions(-) diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index f34d6c38892c..7d02b00d0bb7 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -1081,7 +1081,10 @@ def manage_project_oidc_providers(self): @view_config(request_method="POST", request_param=GitHubProviderForm.__params__) def add_github_oidc_provider(self): - form = GitHubProviderForm(self.request.POST) + form = GitHubProviderForm( + self.request.POST, + api_token=self.request.registry.settings.get("github.token"), + ) if form.validate(): # GitHub OIDC providers are unique on the tuple of @@ -1090,16 +1093,16 @@ def add_github_oidc_provider(self): provider = ( self.request.db.query(GitHubProvider) .filter( - GitHubProvider.repository_name == form.repository, - GitHubProvider.owner == form.owner, + GitHubProvider.repository_name == form.repository.data, + GitHubProvider.owner == form.normalized_owner, GitHubProvider.workflow_name == form.workflow_name.data, ) .one_or_none() ) if provider is None: provider = GitHubProvider( - repository_name=form.repository, - owner=form.owner, + repository_name=form.repository.data, + owner=form.normalized_owner, owner_id=form.owner_id, workflow_name=form.workflow_name.data, ) diff --git a/warehouse/oidc/forms.py b/warehouse/oidc/forms.py index 90e86bbf7707..c7c1b34ec01a 100644 --- a/warehouse/oidc/forms.py +++ b/warehouse/oidc/forms.py @@ -19,18 +19,26 @@ from warehouse import forms from warehouse.i18n import localize as _ -# This roughly matches the "owner/repo" convention used by GitHub. -_VALID_GITHUB_OWNER_REPO_SLUG = re.compile( - r"^[a-zA-Z0-9][a-zA-Z0-9-]*/[a-zA-Z0-9-_.]+$" -) +_VALID_GITHUB_REPO = re.compile(r"^[a-zA-Z0-9-_.]+$") class GitHubProviderForm(forms.Form): - __params__ = ["repository_slug", "workflow_name"] + __params__ = ["owner", "repository", "workflow_name"] - repository_slug = wtforms.StringField( + owner = wtforms.StringField( + validators=[ + wtforms.validators.DataRequired( + message=_("Specify GitHub owner (username or organization)") + ), + ] + ) + + repository = wtforms.StringField( validators=[ wtforms.validators.DataRequired(message=_("Specify repository slug")), + wtforms.validators.Regexp( + _VALID_GITHUB_REPO, message=_("Invalid repository name") + ), ] ) @@ -47,17 +55,8 @@ def _headers_auth(self): return {} return {"Authorization": f"token {self._api_token}"} - def validate_repository_slug(self, field): - repository_slug = field.data - if not _VALID_GITHUB_OWNER_REPO_SLUG.fullmatch(repository_slug): - raise wtforms.validators.ValidationError( - _( - "The specified repository is invalid. Repositories must be " - "specified in owner/repo format." - ) - ) - - owner, repository = repository_slug.split("/", 1) + def validate_owner(self, field): + owner = field.data # To actually validate the owner, we ask GitHub's API about them. # We can't do this for the repository, since it might be private. @@ -95,9 +94,8 @@ def validate_repository_slug(self, field): owner_info = response.json() # NOTE: Use the normalized owner name as provided by GitHub. - self.owner = owner_info["login"] + self.normalized_owner = owner_info["login"] self.owner_id = owner_info["id"] - self.repository = repository def validate_workflow_name(self, field): workflow_name = field.data diff --git a/warehouse/templates/manage/publishing.html b/warehouse/templates/manage/publishing.html index 7d1d83ca4155..c9bd85bc1e4d 100644 --- a/warehouse/templates/manage/publishing.html +++ b/warehouse/templates/manage/publishing.html @@ -72,15 +72,27 @@

    GitHub

    + + {{ github_provider_form.owner(placeholder=gettext("owner"), autocomplete="off", autocapitalize="off", spellcheck="false", class_="form-group__field", **{"aria-describedby":"owner-slug-errors"}) }} +
    + {{ field_errors(github_provider_form.owner) }} +
    +
    +
    - {{ github_provider_form.repository_slug(placeholder=gettext("owner/repo"), autocomplete="off", autocapitalize="off", spellcheck="false", class_="form-group__field", **{"aria-describedby":"repository-slug-errors"}) }} + {{ github_provider_form.repository(placeholder=gettext("repository"), autocomplete="off", autocapitalize="off", spellcheck="false", class_="form-group__field", **{"aria-describedby":"repository-slug-errors"}) }}
    - {{ field_errors(github_provider_form.repository_slug) }} + {{ field_errors(github_provider_form.repository) }}
    From 5ed70bc4e93d37428b96400a8d9fd57c88f0ddcc Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 25 Feb 2022 18:08:36 -0500 Subject: [PATCH 25/78] warehouse/templates: link to GitHub's OIDC docs --- warehouse/templates/manage/publishing.html | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/warehouse/templates/manage/publishing.html b/warehouse/templates/manage/publishing.html index c9bd85bc1e4d..5f935948a8f6 100644 --- a/warehouse/templates/manage/publishing.html +++ b/warehouse/templates/manage/publishing.html @@ -69,6 +69,12 @@

    {% trans %}Add a new provider{% endtrans %}

    GitHub

    +

    + {% trans trimmed href="https://docs.github.com/en/actions/deployment/security-hardening-your-deployments/about-security-hardening-with-openid-connect" %} + Read more about GitHub's OpenID Connect provider here. + {% endtrans %} +

    +
    From 2c9722d4b780fb2266187eab26a8ab8f07110834 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 25 Feb 2022 18:22:35 -0500 Subject: [PATCH 26/78] oidc/models: remove actor from checked claims --- warehouse/oidc/models.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/warehouse/oidc/models.py b/warehouse/oidc/models.py index 9a8b2a7fc4a0..5333eed9b18a 100644 --- a/warehouse/oidc/models.py +++ b/warehouse/oidc/models.py @@ -126,11 +126,11 @@ class GitHubProvider(OIDCProvider): __verifiable_claims__ = { "repository": str.__eq__, "job_workflow_ref": str.startswith, - "actor": str.__eq__, "workflow": str.__eq__, } __unchecked_claims__ = { + "actor", "jti", "sub", "ref", @@ -156,10 +156,6 @@ def repository(self): def job_workflow_ref(self): return f"{self.repository}/.github/workflows/{self.workflow_name}.yml" - @property - def actor(self): - return self.owner - @property def workflow(self): return self.workflow_name From be82d1b860da1130f44feb61b438a31e23ce710c Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 28 Feb 2022 10:46:36 -0500 Subject: [PATCH 27/78] templates/email: add OIDC email templates --- .../email/oidc-provider-added/body.html | 40 +++++++++++++++++++ .../email/oidc-provider-added/body.txt | 34 ++++++++++++++++ .../email/oidc-provider-added/subject.txt | 17 ++++++++ 3 files changed, 91 insertions(+) create mode 100644 warehouse/templates/email/oidc-provider-added/body.html create mode 100644 warehouse/templates/email/oidc-provider-added/body.txt create mode 100644 warehouse/templates/email/oidc-provider-added/subject.txt diff --git a/warehouse/templates/email/oidc-provider-added/body.html b/warehouse/templates/email/oidc-provider-added/body.html new file mode 100644 index 000000000000..f6ced02a2c5e --- /dev/null +++ b/warehouse/templates/email/oidc-provider-added/body.html @@ -0,0 +1,40 @@ +{# + # Licensed under the Apache License, Version 2.0 (the "License"); + # you may not use this file except in compliance with the License. + # You may obtain a copy of the License at + # + # http://www.apache.org/licenses/LICENSE-2.0 + # + # Unless required by applicable law or agreed to in writing, software + # distributed under the License is distributed on an "AS IS" BASIS, + # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + # See the License for the specific language governing permissions and + # limitations under the License. +-#} +{% extends "email/_base/body.html" %} + + +{% block content %} +

    + {% trans username=username, project=project %} + PyPI user {{ username }} has added a new OpenID Connect + publisher to a project ({{project}}) that you manage. + OpenID Connect publishers act as trusted users and can create project releases + automatically. + {% endtrans %} +

    + +

    + {% trans %} + If you did not make this change and you think it was made maliciously, you can + remove it from the project via the "Publishing" tab on the project's page. + {% endtrans %} +

    + +

    + {% trans href='mailto:admin@pypi.org', email_address='admin@pypi.org' %} + If you are unable to revert the change and need to do so, you can email + {{ email_address }} to communicate with the PyPI + administrators. + {% endtrans %} +

    diff --git a/warehouse/templates/email/oidc-provider-added/body.txt b/warehouse/templates/email/oidc-provider-added/body.txt new file mode 100644 index 000000000000..eb4f69cfbb3f --- /dev/null +++ b/warehouse/templates/email/oidc-provider-added/body.txt @@ -0,0 +1,34 @@ +{# + # Licensed under the Apache License, Version 2.0 (the "License"); + # you may not use this file except in compliance with the License. + # You may obtain a copy of the License at + # + # http://www.apache.org/licenses/LICENSE-2.0 + # + # Unless required by applicable law or agreed to in writing, software + # distributed under the License is distributed on an "AS IS" BASIS, + # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + # See the License for the specific language governing permissions and + # limitations under the License. +-#} +{% extends "email/_base/body.txt" %} + +{% block content %} +{% trans username=username, project=project %} +PyPI user {{ username }} has added a new OpenID Connect publisher to a project +({{project}}) that you manage. OpenID Connect publishers act as trusted +users and can create project releases automatically. +{% endtrans %} + +{% trans %} +If you did not make this change and you think it was made maliciously, you can +remove it from the project via the "Publishing" tab on the project's page. +{% endtrans %} + +{% trans email_address='admin@pypi.org' %} +If you are unable to revert the change and need to do so, you can email +{{ email_address }} to communicate with the PyPI administrators. +{% endtrans %} + +{% endblock %} + diff --git a/warehouse/templates/email/oidc-provider-added/subject.txt b/warehouse/templates/email/oidc-provider-added/subject.txt new file mode 100644 index 000000000000..1d14c91ebc07 --- /dev/null +++ b/warehouse/templates/email/oidc-provider-added/subject.txt @@ -0,0 +1,17 @@ +{# + # Licensed under the Apache License, Version 2.0 (the "License"); + # you may not use this file except in compliance with the License. + # You may obtain a copy of the License at + # + # http://www.apache.org/licenses/LICENSE-2.0 + # + # Unless required by applicable law or agreed to in writing, software + # distributed under the License is distributed on an "AS IS" BASIS, + # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + # See the License for the specific language governing permissions and + # limitations under the License. +-#} + +{% extends "email/_base/subject.txt" %} + +{% block subject %}{% trans %}OpenID Connect publisher added{% endtrans %}{% endblock %} From 6c004872d58c0c67f5a7a807c45649b72a7639f4 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 28 Feb 2022 11:14:02 -0500 Subject: [PATCH 28/78] warehouse: fix templates, add email sending logic --- warehouse/email/__init__.py | 6 ++++++ warehouse/manage/views.py | 14 ++++++++++++++ .../templates/email/oidc-provider-added/body.html | 5 +++-- .../templates/email/oidc-provider-added/body.txt | 4 ++-- .../email/oidc-provider-added/subject.txt | 6 +++++- 5 files changed, 30 insertions(+), 5 deletions(-) diff --git a/warehouse/email/__init__.py b/warehouse/email/__init__.py index 8e38cbb77d64..2896e6d2e9e9 100644 --- a/warehouse/email/__init__.py +++ b/warehouse/email/__init__.py @@ -426,6 +426,12 @@ def send_recovery_code_reminder_email(request, user): return {"username": user.username} +@_email("oidc-provider-added") +def send_oidc_provider_added_email(request, user, project_name): + # We use the request's user, since they're the one triggering the action. + return {"username": request.user.username, "project_name": project_name} + + def includeme(config): email_sending_class = config.maybe_dotted(config.registry.settings["mail.backend"]) config.register_service_factory(email_sending_class.create_service, IEmailSender) diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index 7d02b00d0bb7..491cc78316f7 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -41,6 +41,7 @@ send_collaborator_removed_email, send_collaborator_role_changed_email, send_email_verification_email, + send_oidc_provider_added_email, send_password_change_email, send_primary_email_change_email, send_project_role_verification_email, @@ -1118,6 +1119,19 @@ def add_github_oidc_provider(self): ) return {**self.default_response, "github_provider_form": form} + # We only email project owners, since only owners can manage OIDC providers. + owner_roles = ( + self.request.db.query(Role) + .filter(Role.project == self.project) + .filter(Role.role_name == "Owner") + .all() + ) + owner_users = {owner.user for owner in owner_roles} + for user in owner_users: + send_oidc_provider_added_email( + self.request, user, project_name=self.project.name + ) + self.project.oidc_providers.append(provider) self.project.record_event( diff --git a/warehouse/templates/email/oidc-provider-added/body.html b/warehouse/templates/email/oidc-provider-added/body.html index f6ced02a2c5e..b873f860abd9 100644 --- a/warehouse/templates/email/oidc-provider-added/body.html +++ b/warehouse/templates/email/oidc-provider-added/body.html @@ -16,9 +16,9 @@ {% block content %}

    - {% trans username=username, project=project %} + {% trans username=username, project_name=project_name %} PyPI user {{ username }} has added a new OpenID Connect - publisher to a project ({{project}}) that you manage. + publisher to a project ({{project_name}}) that you manage. OpenID Connect publishers act as trusted users and can create project releases automatically. {% endtrans %} @@ -38,3 +38,4 @@ administrators. {% endtrans %}

    +{% endblock %} diff --git a/warehouse/templates/email/oidc-provider-added/body.txt b/warehouse/templates/email/oidc-provider-added/body.txt index eb4f69cfbb3f..e1e3f23c64ee 100644 --- a/warehouse/templates/email/oidc-provider-added/body.txt +++ b/warehouse/templates/email/oidc-provider-added/body.txt @@ -14,9 +14,9 @@ {% extends "email/_base/body.txt" %} {% block content %} -{% trans username=username, project=project %} +{% trans username=username, project_name=project_name %} PyPI user {{ username }} has added a new OpenID Connect publisher to a project -({{project}}) that you manage. OpenID Connect publishers act as trusted +({{ project_name }}) that you manage. OpenID Connect publishers act as trusted users and can create project releases automatically. {% endtrans %} diff --git a/warehouse/templates/email/oidc-provider-added/subject.txt b/warehouse/templates/email/oidc-provider-added/subject.txt index 1d14c91ebc07..178d873e16fd 100644 --- a/warehouse/templates/email/oidc-provider-added/subject.txt +++ b/warehouse/templates/email/oidc-provider-added/subject.txt @@ -14,4 +14,8 @@ {% extends "email/_base/subject.txt" %} -{% block subject %}{% trans %}OpenID Connect publisher added{% endtrans %}{% endblock %} +{% block subject %} +{% trans project_name=project_name %} +OpenID Connect publisher added to {{ project_name }} +{% endtrans %} +{% endblock %} From a6aa4a07c099a9b572e634c471120555d3755fe1 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 28 Feb 2022 11:26:48 -0500 Subject: [PATCH 29/78] warehouse: add an AdminFlag for OIDC control --- warehouse/admin/flags.py | 1 + warehouse/manage/views.py | 29 +++++++++++++++++++ ...4c444f_add_initial_oidc_provider_models.py | 16 +++++++--- 3 files changed, 42 insertions(+), 4 deletions(-) diff --git a/warehouse/admin/flags.py b/warehouse/admin/flags.py index 356a0ba4247a..03082747c611 100644 --- a/warehouse/admin/flags.py +++ b/warehouse/admin/flags.py @@ -22,6 +22,7 @@ class AdminFlagValue(enum.Enum): DISALLOW_NEW_PROJECT_REGISTRATION = "disallow-new-project-registration" DISALLOW_NEW_UPLOAD = "disallow-new-upload" DISALLOW_NEW_USER_REGISTRATION = "disallow-new-user-registration" + DISALLOW_OIDC = "disallow-oidc" READ_ONLY = "read-only" diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index 491cc78316f7..7866f7f1b025 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -1078,10 +1078,29 @@ def default_response(self): @view_config(request_method="GET") def manage_project_oidc_providers(self): + if self.request.flags.enabled(AdminFlagValue.DISALLOW_OIDC): + self.request.session.flash( + ( + "OpenID Connect is temporarily disabled. " + "See https://pypi.org/help#admin-intervention for details." + ), + queue="error", + ) + return self.default_response @view_config(request_method="POST", request_param=GitHubProviderForm.__params__) def add_github_oidc_provider(self): + if self.request.flags.enabled(AdminFlagValue.DISALLOW_OIDC): + self.request.session.flash( + ( + "OpenID Connect is temporarily disabled. " + "See https://pypi.org/help#admin-intervention for details." + ), + queue="error", + ) + return self.default_response + form = GitHubProviderForm( self.request.POST, api_token=self.request.registry.settings.get("github.token"), @@ -1153,6 +1172,16 @@ def add_github_oidc_provider(self): @view_config(request_method="POST", request_param=DeleteProviderForm.__params__) def delete_oidc_provider(self): + if self.request.flags.enabled(AdminFlagValue.DISALLOW_OIDC): + self.request.session.flash( + ( + "OpenID Connect is temporarily disabled. " + "See https://pypi.org/help#admin-intervention for details." + ), + queue="error", + ) + return self.default_response + form = DeleteProviderForm(self.request.POST) if form.validate(): diff --git a/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py b/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py index 7bfc717a3ea0..138ebe9e97f8 100644 --- a/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py +++ b/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py @@ -36,7 +36,6 @@ def upgrade(): - # ### commands auto generated by Alembic - please adjust! ### op.create_table( "oidc_providers", sa.Column( @@ -84,12 +83,21 @@ def upgrade(): ), sa.PrimaryKeyConstraint("id", "oidc_provider_id", "project_id"), ) - # ### end Alembic commands ### + op.execute( + """ + INSERT INTO admin_flags(id, description, enabled, notify) + VALUES ( + 'disallow-oidc', + 'Disallow ALL OpenID Connect behavior, including authentication', + FALSE, + FALSE + ) + """ + ) def downgrade(): - # ### commands auto generated by Alembic - please adjust! ### op.drop_table("oidc_provider_project_association") op.drop_table("github_oidc_providers") op.drop_table("oidc_providers") - # ### end Alembic commands ### + op.execute("DELETE FROM admin_flags WHERE id = 'disallow-oidc'") From e259dc81d3a4d01e3627dfaaacf738db325b695b Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 28 Feb 2022 11:28:56 -0500 Subject: [PATCH 30/78] oidc/models: use set operators --- warehouse/oidc/models.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/warehouse/oidc/models.py b/warehouse/oidc/models.py index 5333eed9b18a..e2b14c6a5591 100644 --- a/warehouse/oidc/models.py +++ b/warehouse/oidc/models.py @@ -82,10 +82,12 @@ def verify_claims(self, signed_claims): # All claims should be accounted for. # The presence of an unaccounted claim is not an error, only a warning # that the JWT payload has changed. - known_claims = self.__verifiable_claims__.keys().union( - self.__preverified_claims__, self.__unchecked_claims__ + known_claims = ( + self.__verifiable_claims__.keys() + | self.__preverified_claims__ + | self.__unchecked_claims__ ) - unaccounted_claims = known_claims.difference(signed_claims.keys()) + unaccounted_claims = known_claims - signed_claims.keys() if unaccounted_claims: sentry_sdk.capture_message( f"JWT for {self.__class__.__name__} has unaccounted claims: " From d0b37d2565b0638ac7f344f50ed2ac27ff998c32 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 28 Feb 2022 11:36:44 -0500 Subject: [PATCH 31/78] oidc/forms: exception driven handling for GitHub API errors --- warehouse/oidc/forms.py | 61 ++++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 25 deletions(-) diff --git a/warehouse/oidc/forms.py b/warehouse/oidc/forms.py index c7c1b34ec01a..e35900b208a0 100644 --- a/warehouse/oidc/forms.py +++ b/warehouse/oidc/forms.py @@ -60,35 +60,46 @@ def validate_owner(self, field): # To actually validate the owner, we ask GitHub's API about them. # We can't do this for the repository, since it might be private. - response = requests.get( - f"https://api.github.com/users/{owner}", - headers={ - "Accept": "application/vnd.github.v3+json", - **self._headers_auth(), - }, - allow_redirects=True, - ) - - if response.status_code == 404: - raise wtforms.validators.ValidationError( - _("Unknown GitHub user or organization.") - ) - if response.status_code == 403: - # GitHub's API uses 403 to signal rate limiting, and returns a JSON - # blob explaining the reason. - sentry_sdk.capture_message( - "Exceeded GitHub rate limit for user lookups. " - f"Reason: {response.json()}" - ) - raise wtforms.validators.ValidationError( - _("GitHub has rate-limited this action. Try again in a few minutes.") + try: + response = requests.get( + f"https://api.github.com/users/{owner}", + headers={ + "Accept": "application/vnd.github.v3+json", + **self._headers_auth(), + }, + allow_redirects=True, ) - elif not response.ok: + response.raise_for_status() + except requests.HTTPError as exc: + if exc.status_code == 404: + raise wtforms.validators.ValidationError( + _("Unknown GitHub user or organization.") + ) + if exc.status_code == 403: + # GitHub's API uses 403 to signal rate limiting, and returns a JSON + # blob explaining the reason. + sentry_sdk.capture_message( + "Exceeded GitHub rate limit for user lookups. " + f"Reason: {response.json()}" + ) + raise wtforms.validators.ValidationError( + _( + "GitHub has rate-limited this action. Try again in a few minutes." + ) + ) + else: + sentry_sdk.capture_message( + f"Unexpected error from GitHub user lookup: {response.content=}" + ) + raise wtforms.validators.ValidationError( + _("Unexpected error from GitHub. Try again.") + ) + except requests.Timeout: sentry_sdk.capture_message( - f"Unexpected error from GitHub user lookup: {response.content=}" + "Timeout from GitHub user lookup API (possibly offline)" ) raise wtforms.validators.ValidationError( - _("Unexpected error from GitHub. Try again.") + _("Unexpected timeout from GitHub. Try again in a few minutes.") ) owner_info = response.json() From 1fc826a89e296274fcd57d2691b2d5aaba56ac73 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Wed, 2 Mar 2022 13:42:29 -0500 Subject: [PATCH 32/78] warehouse: OIDC ratelimiting logic Also some small HTML fixes. --- warehouse/accounts/interfaces.py | 7 +-- warehouse/config.py | 10 ++++- warehouse/manage/__init__.py | 17 ++++++-- warehouse/manage/views.py | 51 +++++++++++++++++++++- warehouse/oidc/forms.py | 8 ++-- warehouse/oidc/interfaces.py | 6 +++ warehouse/rate_limiting/interfaces.py | 7 +++ warehouse/templates/manage/publishing.html | 12 ++--- 8 files changed, 94 insertions(+), 24 deletions(-) diff --git a/warehouse/accounts/interfaces.py b/warehouse/accounts/interfaces.py index 6ff29e708923..515cc0e2b7eb 100644 --- a/warehouse/accounts/interfaces.py +++ b/warehouse/accounts/interfaces.py @@ -12,12 +12,7 @@ from zope.interface import Attribute, Interface - -class RateLimiterException(Exception): - def __init__(self, *args, resets_in, **kwargs): - self.resets_in = resets_in - - return super().__init__(*args, **kwargs) +from warehouse.rate_limiting.interfaces import RateLimiterException class TooManyFailedLogins(RateLimiterException): diff --git a/warehouse/config.py b/warehouse/config.py index 3ac2c35efbcb..114ab640c4f9 100644 --- a/warehouse/config.py +++ b/warehouse/config.py @@ -272,8 +272,14 @@ def configure(settings=None): ) maybe_set( settings, - "warehouse.manage.oidc.provider_registration_string", - "OIDC_PROVIDER_REGISTRATION_RATELIMIT_STRING", + "warehouse.manage.oidc.user_provider_registration_ratelimit_string", + "USER_OIDC_PROVIDER_REGISTRATION_RATELIMIT_STRING", + default="10 per day", + ) + maybe_set( + settings, + "warehouse.manage.oidc.ip_provider_registration_ratelimit_string", + "IP_OIDC_PROVIDER_REGISTRATION_RATELIMIT_STRING", default="10 per day", ) diff --git a/warehouse/manage/__init__.py b/warehouse/manage/__init__.py index 2346aa9c07c9..9c2c73a0ba7a 100644 --- a/warehouse/manage/__init__.py +++ b/warehouse/manage/__init__.py @@ -64,11 +64,20 @@ def wrapped(context, request): def includeme(config): config.add_view_deriver(reauth_view, over="rendered_view", under="decorated_view") - oidc_provider_registration_ratelimit_string = config.registry.settings.get( - "warehouse.manage.oidc.provider_registration_ratelimit_string" + user_oidc_provider_registration_ratelimit_string = config.registry.settings.get( + "warehouse.manage.oidc.user_provider_registration_ratelimit_string" ) config.register_service_factory( - RateLimit(oidc_provider_registration_ratelimit_string), + RateLimit(user_oidc_provider_registration_ratelimit_string), IRateLimiter, - name="oidc.provider.register", + name="user_oidc.provider.register", + ) + + ip_oidc_provider_registration_ratelimit_string = config.registry.settings.get( + "warehouse.manage.oidc.ip_provider_registration_ratelimit_string" + ) + config.register_service_factory( + RateLimit(ip_oidc_provider_registration_ratelimit_string), + IRateLimiter, + name="ip_oidc.provider.register", ) diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index 7866f7f1b025..279f9315a833 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -16,7 +16,12 @@ import pyqrcode from paginate_sqlalchemy import SqlalchemyOrmPage as SQLAlchemyORMPage -from pyramid.httpexceptions import HTTPBadRequest, HTTPNotFound, HTTPSeeOther +from pyramid.httpexceptions import ( + HTTPBadRequest, + HTTPNotFound, + HTTPSeeOther, + HTTPTooManyRequests, +) from pyramid.response import Response from pyramid.view import view_config, view_defaults from sqlalchemy import func @@ -74,6 +79,7 @@ Toggle2FARequirementForm, ) from warehouse.oidc.forms import DeleteProviderForm, GitHubProviderForm +from warehouse.oidc.interfaces import TooManyOIDCRegistrations from warehouse.oidc.models import GitHubProvider, OIDCProvider from warehouse.packaging.models import ( File, @@ -85,6 +91,7 @@ RoleInvitation, RoleInvitationStatus, ) +from warehouse.rate_limiting import IRateLimiter from warehouse.utils.http import is_safe_url from warehouse.utils.paginate import paginate_url_factory from warehouse.utils.project import confirm_project, destroy_docs, remove_project @@ -1067,12 +1074,40 @@ def __init__(self, project, request): self.request = request self.project = project + @property + def _ratelimiters(self): + return { + "user.oidc": self.request.find_service( + IRateLimiter, name="user_oidc.provider.register" + ), + "ip.oidc": self.request.find_service( + IRateLimiter, name="ip_oidc.provider.register" + ), + } + + def _hit_ratelimits(self): + self._ratelimiters["user.oidc"].hit(self.request.user.id) + self._ratelimiters["ip.oidc"].hit(self.request.remote_addr) + + def _check_ratelimits(self): + if not self._ratelimiters["user.oidc"].test(self.request.user.id): + raise TooManyOIDCRegistrations( + resets_in=self._ratelimiters["user.oidc"].resets_in( + self.request.user.id + ) + ) + + if not self._ratelimiters["ip.oidc"].test(self.request.remote_addr): + raise TooManyOIDCRegistrations( + resets_in=self._ratelimiters["ip.oidc"].resets_in(self.remote_addr) + ) + @property def default_response(self): return { "project": self.project, "github_provider_form": GitHubProviderForm( - api_token=self.request.registry.settings.get("github.token") + api_token=self.request.registry.settings.get("github.token"), ), } @@ -1101,12 +1136,24 @@ def add_github_oidc_provider(self): ) return self.default_response + try: + self._check_ratelimits() + except TooManyOIDCRegistrations as exc: + return HTTPTooManyRequests( + self.request._( + "There have been too many attempted OpenID Connect registrations. " + "Try again later." + ), + retry_after=exc.resets_in.total_seconds(), + ) + form = GitHubProviderForm( self.request.POST, api_token=self.request.registry.settings.get("github.token"), ) if form.validate(): + self._hit_ratelimits() # GitHub OIDC providers are unique on the tuple of # (repository_name, owner, workflow_name), so we check for # an already registered one before creating. diff --git a/warehouse/oidc/forms.py b/warehouse/oidc/forms.py index e35900b208a0..f60a67ba66bb 100644 --- a/warehouse/oidc/forms.py +++ b/warehouse/oidc/forms.py @@ -71,16 +71,16 @@ def validate_owner(self, field): ) response.raise_for_status() except requests.HTTPError as exc: - if exc.status_code == 404: + if exc.response.status_code == 404: raise wtforms.validators.ValidationError( _("Unknown GitHub user or organization.") ) - if exc.status_code == 403: + if exc.response.status_code == 403: # GitHub's API uses 403 to signal rate limiting, and returns a JSON # blob explaining the reason. sentry_sdk.capture_message( "Exceeded GitHub rate limit for user lookups. " - f"Reason: {response.json()}" + f"Reason: {exc.response.json()}" ) raise wtforms.validators.ValidationError( _( @@ -89,7 +89,7 @@ def validate_owner(self, field): ) else: sentry_sdk.capture_message( - f"Unexpected error from GitHub user lookup: {response.content=}" + f"Unexpected error from GitHub user lookup: {exc.response.content=}" ) raise wtforms.validators.ValidationError( _("Unexpected error from GitHub. Try again.") diff --git a/warehouse/oidc/interfaces.py b/warehouse/oidc/interfaces.py index 33ffa0639d55..9c7e21bab889 100644 --- a/warehouse/oidc/interfaces.py +++ b/warehouse/oidc/interfaces.py @@ -13,6 +13,8 @@ from zope.interface import Interface +from warehouse.rate_limiting.interfaces import RateLimiterException + class IOIDCProviderService(Interface): def get_key(key_id): @@ -37,3 +39,7 @@ def verify_signature_only(token): for a particular action; subsequent checks on the decoded token's third party claims must be done to ensure that. """ + + +class TooManyOIDCRegistrations(RateLimiterException): + pass diff --git a/warehouse/rate_limiting/interfaces.py b/warehouse/rate_limiting/interfaces.py index 876122751b70..aac632e621fa 100644 --- a/warehouse/rate_limiting/interfaces.py +++ b/warehouse/rate_limiting/interfaces.py @@ -38,3 +38,10 @@ def clear(*identifiers): """ Clears the rate limiter identified by the identifiers. """ + + +class RateLimiterException(Exception): + def __init__(self, *args, resets_in, **kwargs): + self.resets_in = resets_in + + return super().__init__(*args, **kwargs) diff --git a/warehouse/templates/manage/publishing.html b/warehouse/templates/manage/publishing.html index 5f935948a8f6..2d4b1dafa25b 100644 --- a/warehouse/templates/manage/publishing.html +++ b/warehouse/templates/manage/publishing.html @@ -84,8 +84,8 @@

    GitHub

    {% trans %}(required){% endtrans %} {% endif %} - {{ github_provider_form.owner(placeholder=gettext("owner"), autocomplete="off", autocapitalize="off", spellcheck="false", class_="form-group__field", **{"aria-describedby":"owner-slug-errors"}) }} -
    + {{ github_provider_form.owner(placeholder=gettext("owner"), autocomplete="off", autocapitalize="off", spellcheck="false", class_="form-group__field", **{"aria-describedby":"owner-errors"}) }} +
    {{ field_errors(github_provider_form.owner) }}
    @@ -96,8 +96,8 @@

    GitHub

    {% trans %}(required){% endtrans %} {% endif %} - {{ github_provider_form.repository(placeholder=gettext("repository"), autocomplete="off", autocapitalize="off", spellcheck="false", class_="form-group__field", **{"aria-describedby":"repository-slug-errors"}) }} -
    + {{ github_provider_form.repository(placeholder=gettext("repository"), autocomplete="off", autocapitalize="off", spellcheck="false", class_="form-group__field", **{"aria-describedby":"repository-errors"}) }} +
    {{ field_errors(github_provider_form.repository) }}
    @@ -108,8 +108,8 @@

    GitHub

    {% trans %}(required){% endtrans %} {% endif %} - {{ github_provider_form.workflow_name(placeholder=gettext("workflow.yml"), class_="form-group__field", autocomplete="off", **{"aria-describedby":"workflow-name-errors"}) }} -
    + {{ github_provider_form.workflow_name(placeholder=gettext("workflow.yml"), class_="form-group__field", autocomplete="off", **{"aria-describedby":"workflow_name-errors"}) }} +
    {{ field_errors(github_provider_form.workflow_name) }}
    From f307de72f712bd8651283766af87a636f4774280 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Wed, 2 Mar 2022 14:19:48 -0500 Subject: [PATCH 33/78] warehouse/locale: update translations --- warehouse/locale/messages.pot | 251 ++++++++++++++++++++++++++++++---- 1 file changed, 221 insertions(+), 30 deletions(-) diff --git a/warehouse/locale/messages.pot b/warehouse/locale/messages.pot index fce353ed1a22..8247f1b3e5c3 100644 --- a/warehouse/locale/messages.pot +++ b/warehouse/locale/messages.pot @@ -116,7 +116,7 @@ msgstr "" msgid "Successful WebAuthn assertion" msgstr "" -#: warehouse/accounts/views.py:439 warehouse/manage/views.py:786 +#: warehouse/accounts/views.py:439 warehouse/manage/views.py:796 msgid "Recovery code accepted. The supplied code cannot be used again." msgstr "" @@ -225,48 +225,98 @@ msgstr "" msgid "Banner Preview" msgstr "" -#: warehouse/manage/views.py:222 +#: warehouse/manage/views.py:232 msgid "Email ${email_address} added - check your email for a verification link" msgstr "" -#: warehouse/manage/views.py:734 +#: warehouse/manage/views.py:744 msgid "Recovery codes already generated" msgstr "" -#: warehouse/manage/views.py:735 +#: warehouse/manage/views.py:745 msgid "Generating new recovery codes will invalidate your existing codes." msgstr "" -#: warehouse/manage/views.py:1598 +#: warehouse/manage/views.py:1150 +msgid "" +"There have been too many attempted OpenID Connect registrations. Try " +"again later." +msgstr "" + +#: warehouse/manage/views.py:1808 msgid "User '${username}' already has ${role_name} role for project" msgstr "" -#: warehouse/manage/views.py:1609 +#: warehouse/manage/views.py:1819 msgid "" "User '${username}' does not have a verified primary email address and " "cannot be added as a ${role_name} for project" msgstr "" -#: warehouse/manage/views.py:1622 +#: warehouse/manage/views.py:1832 msgid "User '${username}' already has an active invite. Please try again later." msgstr "" -#: warehouse/manage/views.py:1680 +#: warehouse/manage/views.py:1890 msgid "Invitation sent to '${username}'" msgstr "" -#: warehouse/manage/views.py:1727 +#: warehouse/manage/views.py:1937 msgid "Could not find role invitation." msgstr "" -#: warehouse/manage/views.py:1738 +#: warehouse/manage/views.py:1948 msgid "Invitation already expired." msgstr "" -#: warehouse/manage/views.py:1762 +#: warehouse/manage/views.py:1972 msgid "Invitation revoked from '${username}'." msgstr "" +#: warehouse/oidc/forms.py:31 +msgid "Specify GitHub owner (username or organization)" +msgstr "" + +#: warehouse/oidc/forms.py:38 +msgid "Specify repository slug" +msgstr "" + +#: warehouse/oidc/forms.py:40 +msgid "Invalid repository name" +msgstr "" + +#: warehouse/oidc/forms.py:46 +msgid "Specify workflow name" +msgstr "" + +#: warehouse/oidc/forms.py:76 +msgid "Unknown GitHub user or organization." +msgstr "" + +#: warehouse/oidc/forms.py:86 +msgid "GitHub has rate-limited this action. Try again in a few minutes." +msgstr "" + +#: warehouse/oidc/forms.py:95 +msgid "Unexpected error from GitHub. Try again." +msgstr "" + +#: warehouse/oidc/forms.py:102 +msgid "Unexpected timeout from GitHub. Try again in a few minutes." +msgstr "" + +#: warehouse/oidc/forms.py:116 +msgid "Workflow name must end with .yml or .yaml" +msgstr "" + +#: warehouse/oidc/forms.py:121 +msgid "Workflow name must be a basename, without directories" +msgstr "" + +#: warehouse/oidc/forms.py:130 +msgid "Provider must be specified by ID" +msgstr "" + #: warehouse/templates/403.html:16 msgid "Access Denied / Forbidden (403)" msgstr "" @@ -536,7 +586,7 @@ msgstr "" #: warehouse/templates/manage/manage_base.html:278 #: warehouse/templates/manage/manage_base.html:330 #: warehouse/templates/manage/release.html:163 -#: warehouse/templates/manage/settings.html:119 +#: warehouse/templates/manage/settings.html:134 msgid "Warning" msgstr "" @@ -842,6 +892,9 @@ msgstr "" #: warehouse/templates/manage/account/recovery_codes-burn.html:70 #: warehouse/templates/manage/account/totp-provision.html:69 #: warehouse/templates/manage/account/webauthn-provision.html:44 +#: warehouse/templates/manage/publishing.html:84 +#: warehouse/templates/manage/publishing.html:96 +#: warehouse/templates/manage/publishing.html:108 #: warehouse/templates/manage/roles.html:170 #: warehouse/templates/manage/roles.html:182 #: warehouse/templates/manage/token.html:136 @@ -1272,6 +1325,41 @@ msgid "" "to publish." msgstr "" +#: warehouse/templates/email/oidc-provider-added/body.html:19 +#, python-format +msgid "" +"\n" +" PyPI user %(username)s has added a new OpenID Connect\n" +" publisher to a project (%(project_name)s) that you " +"manage.\n" +" OpenID Connect publishers act as trusted users and can create project " +"releases\n" +" automatically.\n" +" " +msgstr "" + +#: warehouse/templates/email/oidc-provider-added/body.html:28 +msgid "" +"\n" +" If you did not make this change and you think it was made maliciously, " +"you can\n" +" remove it from the project via the \"Publishing\" tab on the project's " +"page.\n" +" " +msgstr "" + +#: warehouse/templates/email/oidc-provider-added/body.html:35 +#, python-format +msgid "" +"\n" +" If you are unable to revert the change and need to do so, you can email" +"\n" +" %(email_address)s to communicate with the PyPI" +"\n" +" administrators.\n" +" " +msgstr "" + #: warehouse/templates/email/password-change/body.html:18 #, python-format msgid "" @@ -1534,7 +1622,7 @@ msgstr "" #: warehouse/templates/manage/release.html:118 #: warehouse/templates/manage/releases.html:172 #: warehouse/templates/manage/roles.html:38 -#: warehouse/templates/manage/settings.html:113 +#: warehouse/templates/manage/settings.html:128 #: warehouse/templates/search/results.html:199 msgid "Close" msgstr "" @@ -1703,6 +1791,10 @@ msgid "Documentation" msgstr "" #: warehouse/templates/includes/manage/manage-project-menu.html:51 +msgid "Publishing" +msgstr "" + +#: warehouse/templates/includes/manage/manage-project-menu.html:57 msgid "Settings" msgstr "" @@ -2600,6 +2692,7 @@ msgstr "" #: warehouse/templates/manage/manage_base.html:64 #: warehouse/templates/manage/manage_base.html:78 +#: warehouse/templates/manage/publishing.html:44 msgid "Remove" msgstr "" @@ -2854,6 +2947,88 @@ msgid "" "rel=\"noopener\">Python Packaging User Guide" msgstr "" +#: warehouse/templates/manage/publishing.html:20 +#: warehouse/templates/manage/publishing.html:51 +msgid "OpenID Connect publisher management" +msgstr "" + +#: warehouse/templates/manage/publishing.html:54 +msgid "" +"OpenID Connect provides a flexible, credential-free mechanism for " +"delegating publishing authority for a PyPI package to a third party " +"service, like GitHub Actions." +msgstr "" + +#: warehouse/templates/manage/publishing.html:62 +msgid "" +"PyPI projects can use trusted OpenID Connect publishers to automate their" +" release processes, without having to explicitly provision or manage API " +"tokens." +msgstr "" + +#: warehouse/templates/manage/publishing.html:68 +msgid "Add a new provider" +msgstr "" + +#: warehouse/templates/manage/publishing.html:73 +#, python-format +msgid "" +"Read more about GitHub's OpenID Connect provider here." +msgstr "" + +#: warehouse/templates/manage/publishing.html:82 +#: warehouse/templates/manage/roles.html:43 +#: warehouse/templates/manage/roles.html:77 +#: warehouse/templates/manage/roles.html:88 +msgid "Owner" +msgstr "" + +#: warehouse/templates/manage/publishing.html:87 +msgid "owner" +msgstr "" + +#: warehouse/templates/manage/publishing.html:94 +msgid "Repository name" +msgstr "" + +#: warehouse/templates/manage/publishing.html:99 +msgid "repository" +msgstr "" + +#: warehouse/templates/manage/publishing.html:106 +msgid "Workflow name" +msgstr "" + +#: warehouse/templates/manage/publishing.html:111 +msgid "workflow.yml" +msgstr "" + +#: warehouse/templates/manage/publishing.html:117 +msgid "Add" +msgstr "" + +#: warehouse/templates/manage/publishing.html:121 +msgid "Manage current providers" +msgstr "" + +#: warehouse/templates/manage/publishing.html:125 +#, python-format +msgid "OpenID Connect publishers associated with %(project_name)s" +msgstr "" + +#: warehouse/templates/manage/publishing.html:129 +msgid "Publisher" +msgstr "" + +#: warehouse/templates/manage/publishing.html:130 +msgid "Specification" +msgstr "" + +#: warehouse/templates/manage/publishing.html:141 +msgid "No publishers are currently configured." +msgstr "" + #: warehouse/templates/manage/release.html:18 #, python-format msgid "Manage '%(project_name)s' – release version %(version)s" @@ -2943,7 +3118,7 @@ msgstr "" #: warehouse/templates/manage/release.html:118 #: warehouse/templates/manage/releases.html:172 #: warehouse/templates/manage/roles.html:38 -#: warehouse/templates/manage/settings.html:113 +#: warehouse/templates/manage/settings.html:128 msgid "Dismiss" msgstr "" @@ -3234,12 +3409,6 @@ msgid "" "delete files, releases, or the project." msgstr "" -#: warehouse/templates/manage/roles.html:43 -#: warehouse/templates/manage/roles.html:77 -#: warehouse/templates/manage/roles.html:88 -msgid "Owner" -msgstr "" - #: warehouse/templates/manage/roles.html:44 msgid "" "Can upload releases. Can invite other collaborators. Can delete files, " @@ -3373,11 +3542,33 @@ msgstr "" msgid "Enable 2FA requirement for %(project_name)s" msgstr "" -#: warehouse/templates/manage/settings.html:104 +#: warehouse/templates/manage/settings.html:101 +msgid "OpenID Connect publishers" +msgstr "" + +#: warehouse/templates/manage/settings.html:103 +msgid "" +"\n" +" OpenID Connect publishers provide a credential-free way to " +"authenticate when uploading packages to PyPI.\n" +" " +msgstr "" + +#: warehouse/templates/manage/settings.html:109 +#, python-format +msgid "" +"\n" +" \n" +" Manage OpenID Connect publishers for %(project_name)s\n" +" \n" +" " +msgstr "" + +#: warehouse/templates/manage/settings.html:119 msgid "Project description and sidebar" msgstr "" -#: warehouse/templates/manage/settings.html:106 +#: warehouse/templates/manage/settings.html:121 #, python-format msgid "" "To set the '%(project_name)s' description, author, links, classifiers, " @@ -3393,16 +3584,16 @@ msgid "" "for more help." msgstr "" -#: warehouse/templates/manage/settings.html:117 -#: warehouse/templates/manage/settings.html:148 +#: warehouse/templates/manage/settings.html:132 +#: warehouse/templates/manage/settings.html:163 msgid "Delete project" msgstr "" -#: warehouse/templates/manage/settings.html:120 +#: warehouse/templates/manage/settings.html:135 msgid "Deleting this project will:" msgstr "" -#: warehouse/templates/manage/settings.html:125 +#: warehouse/templates/manage/settings.html:140 #, python-format msgid "" "Irreversibly delete the project along with %(count)s" @@ -3413,15 +3604,15 @@ msgid_plural "" msgstr[0] "" msgstr[1] "" -#: warehouse/templates/manage/settings.html:131 +#: warehouse/templates/manage/settings.html:146 msgid "Irreversibly delete the project" msgstr "" -#: warehouse/templates/manage/settings.html:135 +#: warehouse/templates/manage/settings.html:150 msgid "Make the project name available to any other PyPI user" msgstr "" -#: warehouse/templates/manage/settings.html:137 +#: warehouse/templates/manage/settings.html:152 msgid "" "This user will be able to make new releases under this project name, so " "long as the distribution filenames do not match filenames from a " @@ -3430,7 +3621,7 @@ msgid "" "number + distribution type)" msgstr "" -#: warehouse/templates/manage/settings.html:148 +#: warehouse/templates/manage/settings.html:163 msgid "Project Name" msgstr "" From 67fb78c837bbd6facc4e36f30db3839c231f8584 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 7 Mar 2022 17:50:25 -0500 Subject: [PATCH 34/78] warehouse: lintage --- warehouse/oidc/forms.py | 3 ++- warehouse/templates/manage/publishing.html | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/warehouse/oidc/forms.py b/warehouse/oidc/forms.py index f60a67ba66bb..15c22be38210 100644 --- a/warehouse/oidc/forms.py +++ b/warehouse/oidc/forms.py @@ -84,7 +84,8 @@ def validate_owner(self, field): ) raise wtforms.validators.ValidationError( _( - "GitHub has rate-limited this action. Try again in a few minutes." + "GitHub has rate-limited this action. " + "Try again in a few minutes." ) ) else: diff --git a/warehouse/templates/manage/publishing.html b/warehouse/templates/manage/publishing.html index 2d4b1dafa25b..3e2013cff81b 100644 --- a/warehouse/templates/manage/publishing.html +++ b/warehouse/templates/manage/publishing.html @@ -41,7 +41,7 @@ {{ provider|string }} - + {%- endmacro %} From d249c6b01a8c8b29225308c62f21f8bf45e5ff5a Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Wed, 9 Mar 2022 10:04:12 -0500 Subject: [PATCH 35/78] templates/manage/settings: remove vestigial HTML --- warehouse/templates/manage/settings.html | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/warehouse/templates/manage/settings.html b/warehouse/templates/manage/settings.html index 2ca91174dc85..5ac32b564b0c 100644 --- a/warehouse/templates/manage/settings.html +++ b/warehouse/templates/manage/settings.html @@ -98,21 +98,6 @@

    {% trans %}2FA requirement{% {% endif %} -

    {% trans %}OpenID Connect publishers{% endtrans %}

    -

    - {% trans %} - OpenID Connect publishers provide a credential-free way to authenticate when uploading packages to PyPI. - {% endtrans %} -

    - -

    - {% trans href=request.route_path('manage.project.settings.oidc', project_name=project.name), project_name=project.name %} - - Manage OpenID Connect publishers for {{ project_name }} - - {% endtrans %} -

    -
    From 07a7119c2d2caccc0504a7bc843a8d2365fdd5f3 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Wed, 9 Mar 2022 11:10:55 -0500 Subject: [PATCH 36/78] warehouse: address feedback * Simplify form handling * Validate GitHub usernames against a regex * Fix form error presentation --- warehouse/manage/views.py | 21 ++++++++++++--------- warehouse/oidc/forms.py | 7 ++++++- warehouse/templates/manage/publishing.html | 4 +++- 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index d556dc73b9e5..123a8adeb959 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -1114,13 +1114,18 @@ def _check_ratelimits(self): resets_in=self._ratelimiters["ip.oidc"].resets_in(self.remote_addr) ) + @property + def github_provider_form(self): + return GitHubProviderForm( + self.request.POST, + api_token=self.request.registry.settings.get("github.token"), + ) + @property def default_response(self): return { "project": self.project, - "github_provider_form": GitHubProviderForm( - api_token=self.request.registry.settings.get("github.token"), - ), + "github_provider_form": self.github_provider_form, } @view_config(request_method="GET") @@ -1159,10 +1164,8 @@ def add_github_oidc_provider(self): retry_after=exc.resets_in.total_seconds(), ) - form = GitHubProviderForm( - self.request.POST, - api_token=self.request.registry.settings.get("github.token"), - ) + response = self.default_response + form = response["github_provider_form"] if form.validate(): self._hit_ratelimits() @@ -1195,7 +1198,7 @@ def add_github_oidc_provider(self): f"{provider} is already registered with {self.project.name}", queue="error", ) - return {**self.default_response, "github_provider_form": form} + return response # We only email project owners, since only owners can manage OIDC providers. owner_roles = ( @@ -1227,7 +1230,7 @@ def add_github_oidc_provider(self): queue="success", ) - return self.default_response + return response @view_config(request_method="POST", request_param=DeleteProviderForm.__params__) def delete_oidc_provider(self): diff --git a/warehouse/oidc/forms.py b/warehouse/oidc/forms.py index 15c22be38210..c495d16a2aa5 100644 --- a/warehouse/oidc/forms.py +++ b/warehouse/oidc/forms.py @@ -20,6 +20,7 @@ from warehouse.i18n import localize as _ _VALID_GITHUB_REPO = re.compile(r"^[a-zA-Z0-9-_.]+$") +_VALID_GITHUB_OWNER = re.compile(r"^[a-zA-Z0-9][a-zA-Z0-9-]*$") class GitHubProviderForm(forms.Form): @@ -28,7 +29,11 @@ class GitHubProviderForm(forms.Form): owner = wtforms.StringField( validators=[ wtforms.validators.DataRequired( - message=_("Specify GitHub owner (username or organization)") + message=_("Specify GitHub owner (username or organization)"), + ), + wtforms.validators.Regexp( + _VALID_GITHUB_OWNER, + message=_("Invalid GitHub user or organization name"), ), ] ) diff --git a/warehouse/templates/manage/publishing.html b/warehouse/templates/manage/publishing.html index 3e2013cff81b..437ad59f8775 100644 --- a/warehouse/templates/manage/publishing.html +++ b/warehouse/templates/manage/publishing.html @@ -75,9 +75,11 @@

    GitHub

    {% endtrans %}

    + {{ form_error_anchor(github_provider_form) }}
    + + {{ form_errors(github_provider_form) }}
    -
  • {% endif %}
  • - + {% trans %}Publishing{% endtrans %} diff --git a/warehouse/templates/manage/publishing.html b/warehouse/templates/manage/publishing.html index 437ad59f8775..0ca347afa997 100644 --- a/warehouse/templates/manage/publishing.html +++ b/warehouse/templates/manage/publishing.html @@ -28,7 +28,7 @@ {% endif %} {% macro provider_row(provider) -%} - + @@ -76,7 +76,7 @@

    GitHub

    {{ form_error_anchor(github_provider_form) }} -
    + {{ form_errors(github_provider_form) }}
    From c72357bcc1f787bcaa570ea9678ed0182b02f1e9 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Wed, 9 Mar 2022 17:38:09 -0500 Subject: [PATCH 44/78] More tests, restructure for testing --- tests/unit/email/test_init.py | 81 ++++++++++++++ tests/unit/oidc/test_forms.py | 201 ++++++++++++++++++++++++++++++++++ warehouse/oidc/forms.py | 35 +++--- 3 files changed, 301 insertions(+), 16 deletions(-) create mode 100644 tests/unit/oidc/test_forms.py diff --git a/tests/unit/email/test_init.py b/tests/unit/email/test_init.py index deb1cb0a7740..d2ab08e5f37e 100644 --- a/tests/unit/email/test_init.py +++ b/tests/unit/email/test_init.py @@ -3575,3 +3575,84 @@ def test_recovery_code_emails( }, ) ] + + +class TestOIDCProviderEmails: + @pytest.mark.parametrize( + "fn, template_name", + [ + (email.send_oidc_provider_added_email, "oidc-provider-added"), + (email.send_oidc_provider_removed_email, "oidc-provider-removed"), + ], + ) + def test_oidc_provider_emails( + self, pyramid_request, pyramid_config, monkeypatch, fn, template_name + ): + stub_user = pretend.stub( + id="id", + username="username", + name="", + email="email@example.com", + primary_email=pretend.stub(email="email@example.com", verified=True), + ) + subject_renderer = pyramid_config.testing_add_renderer( + f"email/{ template_name }/subject.txt" + ) + subject_renderer.string_response = "Email Subject" + body_renderer = pyramid_config.testing_add_renderer( + f"email/{ template_name }/body.txt" + ) + body_renderer.string_response = "Email Body" + html_renderer = pyramid_config.testing_add_renderer( + f"email/{ template_name }/body.html" + ) + html_renderer.string_response = "Email HTML Body" + + send_email = pretend.stub( + delay=pretend.call_recorder(lambda *args, **kwargs: None) + ) + pyramid_request.task = pretend.call_recorder(lambda *args, **kwargs: send_email) + monkeypatch.setattr(email, "send_email", send_email) + + pyramid_request.db = pretend.stub( + query=lambda a: pretend.stub( + filter=lambda *a: pretend.stub( + one=lambda: pretend.stub(user_id=stub_user.id) + ) + ), + ) + pyramid_request.user = stub_user + pyramid_request.registry.settings = {"mail.sender": "noreply@example.com"} + + project_name = "test_project" + + result = fn(pyramid_request, stub_user, project_name=project_name) + + assert result == {"username": stub_user.username, "project_name": project_name} + subject_renderer.assert_() + body_renderer.assert_(username=stub_user.username, project_name=project_name) + html_renderer.assert_(username=stub_user.username, project_name=project_name) + assert pyramid_request.task.calls == [pretend.call(send_email)] + assert send_email.delay.calls == [ + pretend.call( + f"{stub_user.username} <{stub_user.email}>", + { + "subject": "Email Subject", + "body_text": "Email Body", + "body_html": ( + "\n\n" + "

    Email HTML Body

    \n\n" + ), + }, + { + "tag": "account:email:sent", + "user_id": stub_user.id, + "additional": { + "from_": "noreply@example.com", + "to": stub_user.email, + "subject": "Email Subject", + "redact_ip": False, + }, + }, + ) + ] diff --git a/tests/unit/oidc/test_forms.py b/tests/unit/oidc/test_forms.py new file mode 100644 index 000000000000..c5a0a8617d59 --- /dev/null +++ b/tests/unit/oidc/test_forms.py @@ -0,0 +1,201 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import pretend +import pytest +import wtforms + +from requests import HTTPError, Timeout +from webob.multidict import MultiDict + +from warehouse.oidc import forms + + +class TestGitHubProviderForm: + def test_creation(self): + api_token = "fake token" + form = forms.GitHubProviderForm(api_token=api_token) + + assert form._api_token == "fake token" + assert form._headers_auth() == {"Authorization": "token fake token"} + + def test_lookup_owner_404(self, monkeypatch): + response = pretend.stub( + status_code=404, raise_for_status=pretend.raiser(HTTPError) + ) + requests = pretend.stub( + get=pretend.call_recorder(lambda o, **kw: response), HTTPError=HTTPError + ) + monkeypatch.setattr(forms, "requests", requests) + + # sentry_sdk = pretend.stub( + # capture_message=pretend.call_recorder(lambda s: None) + # ) + # monkeypatch.setattr(forms, "sentry_sdk", sentry_sdk) + + form = forms.GitHubProviderForm(api_token="fake-token") + with pytest.raises(wtforms.validators.ValidationError): + form._lookup_owner("some-owner") + + assert requests.get.calls == [ + pretend.call( + "https://api.github.com/users/some-owner", + headers={ + "Accept": "application/vnd.github.v3+json", + "Authorization": "token fake-token", + }, + allow_redirects=True, + ) + ] + + def test_lookup_owner_403(self, monkeypatch): + response = pretend.stub( + status_code=403, + raise_for_status=pretend.raiser(HTTPError), + json=lambda: {"message": "fake-message"}, + ) + requests = pretend.stub( + get=pretend.call_recorder(lambda o, **kw: response), HTTPError=HTTPError + ) + monkeypatch.setattr(forms, "requests", requests) + + sentry_sdk = pretend.stub(capture_message=pretend.call_recorder(lambda s: None)) + monkeypatch.setattr(forms, "sentry_sdk", sentry_sdk) + + form = forms.GitHubProviderForm(api_token="fake-token") + with pytest.raises(wtforms.validators.ValidationError): + form._lookup_owner("some-owner") + + assert requests.get.calls == [ + pretend.call( + "https://api.github.com/users/some-owner", + headers={ + "Accept": "application/vnd.github.v3+json", + "Authorization": "token fake-token", + }, + allow_redirects=True, + ) + ] + assert sentry_sdk.capture_message.calls == [ + pretend.call( + "Exceeded GitHub rate limit for user lookups. " + "Reason: {'message': 'fake-message'}" + ) + ] + + def test_lookup_owner_other_http_error(self, monkeypatch): + response = pretend.stub( + # anything that isn't 404 or 403 + status_code=422, + raise_for_status=pretend.raiser(HTTPError), + content=b"fake-content", + ) + requests = pretend.stub( + get=pretend.call_recorder(lambda o, **kw: response), HTTPError=HTTPError + ) + monkeypatch.setattr(forms, "requests", requests) + + sentry_sdk = pretend.stub(capture_message=pretend.call_recorder(lambda s: None)) + monkeypatch.setattr(forms, "sentry_sdk", sentry_sdk) + + form = forms.GitHubProviderForm(api_token="fake-token") + with pytest.raises(wtforms.validators.ValidationError): + form._lookup_owner("some-owner") + + assert requests.get.calls == [ + pretend.call( + "https://api.github.com/users/some-owner", + headers={ + "Accept": "application/vnd.github.v3+json", + "Authorization": "token fake-token", + }, + allow_redirects=True, + ) + ] + + assert sentry_sdk.capture_message.calls == [ + pretend.call( + "Unexpected error from GitHub user lookup: " + "response.content=b'fake-content'" + ) + ] + + def test_lookup_owner_http_timeout(self, monkeypatch): + requests = pretend.stub( + get=pretend.raiser(Timeout), + Timeout=Timeout, + HTTPError=HTTPError, + ) + monkeypatch.setattr(forms, "requests", requests) + + sentry_sdk = pretend.stub(capture_message=pretend.call_recorder(lambda s: None)) + monkeypatch.setattr(forms, "sentry_sdk", sentry_sdk) + + form = forms.GitHubProviderForm(api_token="fake-token") + with pytest.raises(wtforms.validators.ValidationError): + form._lookup_owner("some-owner") + + assert sentry_sdk.capture_message.calls == [ + pretend.call("Timeout from GitHub user lookup API (possibly offline)") + ] + + @pytest.mark.parametrize( + "data", + [ + {"owner": None, "repository": "some", "workflow_name": "some"}, + {"owner": "", "repository": "some", "workflow_name": "some"}, + { + "owner": "invalid_characters@", + "repository": "some", + "workflow_name": "some", + }, + {"repository": None, "owner": "some", "workflow_name": "some"}, + {"repository": "", "owner": "some", "workflow_name": "some"}, + { + "repository": "$invalid#characters", + "owner": "some", + "workflow_name": "some", + }, + {"repository": "some", "owner": "some", "workflow_name": None}, + {"repository": "some", "owner": "some", "workflow_name": ""}, + ], + ) + def test_validate_basic_invalid_fields(self, monkeypatch, data): + form = forms.GitHubProviderForm(MultiDict(data), api_token=pretend.stub()) + + # We're testing only the basic validation here. + owner_info = {"login": "fake-username", "id": "1234"} + monkeypatch.setattr(form, "_lookup_owner", lambda o: owner_info) + + assert not form.validate() + + def test_validate_owner(self, monkeypatch): + form = forms.GitHubProviderForm(api_token=pretend.stub()) + + owner_info = {"login": "some-username", "id": "1234"} + monkeypatch.setattr(form, "_lookup_owner", lambda o: owner_info) + + field = pretend.stub(data="SOME-USERNAME") + form.validate_owner(field) + + assert form.normalized_owner == "some-username" + assert form.owner_id == "1234" + + @pytest.mark.parametrize( + "workflow_name", ["missing_suffix", "/slash", "/many/slashes", "/slash.yml"] + ) + def test_validate_workflow_name(self, workflow_name): + form = forms.GitHubProviderForm(api_token=pretend.stub()) + field = pretend.stub(data=workflow_name) + + with pytest.raises(wtforms.validators.ValidationError): + form.validate_workflow_name(field) diff --git a/warehouse/oidc/forms.py b/warehouse/oidc/forms.py index 584bb9f50881..1ad9a7ef48b1 100644 --- a/warehouse/oidc/forms.py +++ b/warehouse/oidc/forms.py @@ -56,16 +56,7 @@ def _headers_auth(self): return {} return {"Authorization": f"token {self._api_token}"} - def validate_owner(self, field): - owner = field.data - - # We pre-filter owners with a regex, to avoid loading GitHub's API - # with usernames/org names that will never be valid. - if not _VALID_GITHUB_OWNER.match(owner): - raise wtforms.validators.ValidationError( - _("Invalid GitHub user or organization name.") - ) - + def _lookup_owner(self, owner): # To actually validate the owner, we ask GitHub's API about them. # We can't do this for the repository, since it might be private. try: @@ -78,17 +69,17 @@ def validate_owner(self, field): allow_redirects=True, ) response.raise_for_status() - except requests.HTTPError as exc: - if exc.response.status_code == 404: + except requests.HTTPError: + if response.status_code == 404: raise wtforms.validators.ValidationError( _("Unknown GitHub user or organization.") ) - if exc.response.status_code == 403: + if response.status_code == 403: # GitHub's API uses 403 to signal rate limiting, and returns a JSON # blob explaining the reason. sentry_sdk.capture_message( "Exceeded GitHub rate limit for user lookups. " - f"Reason: {exc.response.json()}" + f"Reason: {response.json()}" ) raise wtforms.validators.ValidationError( _( @@ -98,7 +89,7 @@ def validate_owner(self, field): ) else: sentry_sdk.capture_message( - f"Unexpected error from GitHub user lookup: {exc.response.content=}" + f"Unexpected error from GitHub user lookup: {response.content=}" ) raise wtforms.validators.ValidationError( _("Unexpected error from GitHub. Try again.") @@ -111,7 +102,19 @@ def validate_owner(self, field): _("Unexpected timeout from GitHub. Try again in a few minutes.") ) - owner_info = response.json() + return response.json() + + def validate_owner(self, field): + owner = field.data + + # We pre-filter owners with a regex, to avoid loading GitHub's API + # with usernames/org names that will never be valid. + if not _VALID_GITHUB_OWNER.match(owner): + raise wtforms.validators.ValidationError( + _("Invalid GitHub user or organization name.") + ) + + owner_info = self._lookup_owner(owner) # NOTE: Use the normalized owner name as provided by GitHub. self.normalized_owner = owner_info["login"] From 45e272155ba14d1c0bdfa54d9ab947bf2eea0f66 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Wed, 9 Mar 2022 17:57:04 -0500 Subject: [PATCH 45/78] tests: fill in GitHubProviderForm tests --- tests/unit/oidc/test_forms.py | 68 +++++++++++++++++++++++++++++------ 1 file changed, 58 insertions(+), 10 deletions(-) diff --git a/tests/unit/oidc/test_forms.py b/tests/unit/oidc/test_forms.py index c5a0a8617d59..129357f8c984 100644 --- a/tests/unit/oidc/test_forms.py +++ b/tests/unit/oidc/test_forms.py @@ -21,12 +21,21 @@ class TestGitHubProviderForm: - def test_creation(self): - api_token = "fake token" - form = forms.GitHubProviderForm(api_token=api_token) + @pytest.mark.parametrize( + "token, headers", + [ + ( + None, + {}, + ), + ("fake-token", {"Authorization": "token fake-token"}), + ], + ) + def test_creation(self, token, headers): + form = forms.GitHubProviderForm(api_token=token) - assert form._api_token == "fake token" - assert form._headers_auth() == {"Authorization": "token fake token"} + assert form._api_token == token + assert form._headers_auth() == headers def test_lookup_owner_404(self, monkeypatch): response = pretend.stub( @@ -37,11 +46,6 @@ def test_lookup_owner_404(self, monkeypatch): ) monkeypatch.setattr(forms, "requests", requests) - # sentry_sdk = pretend.stub( - # capture_message=pretend.call_recorder(lambda s: None) - # ) - # monkeypatch.setattr(forms, "sentry_sdk", sentry_sdk) - form = forms.GitHubProviderForm(api_token="fake-token") with pytest.raises(wtforms.validators.ValidationError): form._lookup_owner("some-owner") @@ -148,6 +152,34 @@ def test_lookup_owner_http_timeout(self, monkeypatch): pretend.call("Timeout from GitHub user lookup API (possibly offline)") ] + def test_lookup_owner_succeeds(self, monkeypatch): + fake_owner_info = pretend.stub() + response = pretend.stub( + status_code=200, + raise_for_status=pretend.call_recorder(lambda: None), + json=lambda: fake_owner_info, + ) + requests = pretend.stub( + get=pretend.call_recorder(lambda o, **kw: response), HTTPError=HTTPError + ) + monkeypatch.setattr(forms, "requests", requests) + + form = forms.GitHubProviderForm(api_token="fake-token") + info = form._lookup_owner("some-owner") + + assert requests.get.calls == [ + pretend.call( + "https://api.github.com/users/some-owner", + headers={ + "Accept": "application/vnd.github.v3+json", + "Authorization": "token fake-token", + }, + allow_redirects=True, + ) + ] + assert response.raise_for_status.calls == [pretend.call()] + assert info == fake_owner_info + @pytest.mark.parametrize( "data", [ @@ -178,6 +210,22 @@ def test_validate_basic_invalid_fields(self, monkeypatch, data): assert not form.validate() + def test_validate(self, monkeypatch): + data = MultiDict( + { + "owner": "some-owner", + "repository": "some-repo", + "workflow_name": "some-workflow.yml", + } + ) + form = forms.GitHubProviderForm(MultiDict(data), api_token=pretend.stub()) + + # We're testing only the basic validation here. + owner_info = {"login": "fake-username", "id": "1234"} + monkeypatch.setattr(form, "_lookup_owner", lambda o: owner_info) + + assert form.validate() + def test_validate_owner(self, monkeypatch): form = forms.GitHubProviderForm(api_token=pretend.stub()) From f6fde8dae90852fec01462e7646dc61522f50d49 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Thu, 10 Mar 2022 15:59:16 -0500 Subject: [PATCH 46/78] tests, warehouse: more tests, adaptations for testing --- tests/unit/manage/test_views.py | 247 ++++++++++++++++++++++++++++++++ warehouse/manage/views.py | 35 ++--- warehouse/oidc/models.py | 5 +- 3 files changed, 264 insertions(+), 23 deletions(-) diff --git a/tests/unit/manage/test_views.py b/tests/unit/manage/test_views.py index 8f0798715f5c..dec73955a515 100644 --- a/tests/unit/manage/test_views.py +++ b/tests/unit/manage/test_views.py @@ -4682,3 +4682,250 @@ def test_raises_404_with_out_of_range_page(self, db_request): with pytest.raises(HTTPNotFound): assert views.manage_project_journal(project, db_request) + + +class TestManageOIDCProviderViews: + def test_initializes(self): + project = pretend.stub() + request = pretend.stub() + view = views.ManageOIDCProviderViews(project, request) + + assert view.project is project + assert view.request is request + + def test_manage_project_oidc_providers(self, monkeypatch): + project = pretend.stub() + request = pretend.stub( + flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), + POST=pretend.stub(), + registry=pretend.stub( + settings=pretend.stub( + get=pretend.call_recorder(lambda k: "fake-api-token") + ) + ), + ) + + github_provider_form_obj = pretend.stub() + github_provider_form_cls = pretend.call_recorder( + lambda *a, **kw: github_provider_form_obj + ) + monkeypatch.setattr(views, "GitHubProviderForm", github_provider_form_cls) + + view = views.ManageOIDCProviderViews(project, request) + assert view.manage_project_oidc_providers() == { + "project": project, + "github_provider_form": github_provider_form_obj, + } + + assert request.flags.enabled.calls == [ + pretend.call(AdminFlagValue.DISALLOW_OIDC) + ] + assert request.registry.settings.get.calls == [pretend.call("github.token")] + assert github_provider_form_cls.calls == [ + pretend.call(request.POST, api_token="fake-api-token") + ] + + def test_manage_project_oidc_providers_admin_disabled(self, monkeypatch): + project = pretend.stub() + request = pretend.stub( + flags=pretend.stub(enabled=pretend.call_recorder(lambda f: True)), + session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), + POST=pretend.stub(), + registry=pretend.stub( + settings=pretend.stub( + get=pretend.call_recorder(lambda k: "fake-api-token") + ) + ), + ) + + view = views.ManageOIDCProviderViews(project, request) + github_provider_form_obj = pretend.stub() + github_provider_form_cls = pretend.call_recorder( + lambda *a, **kw: github_provider_form_obj + ) + monkeypatch.setattr(views, "GitHubProviderForm", github_provider_form_cls) + + view = views.ManageOIDCProviderViews(project, request) + assert view.manage_project_oidc_providers() == { + "project": project, + "github_provider_form": github_provider_form_obj, + } + + assert request.flags.enabled.calls == [ + pretend.call(AdminFlagValue.DISALLOW_OIDC) + ] + assert request.session.flash.calls == [ + pretend.call( + ( + "OpenID Connect is temporarily disabled. " + "See https://pypi.org/help#admin-intervention for details." + ), + queue="error", + ) + ] + assert request.registry.settings.get.calls == [pretend.call("github.token")] + assert github_provider_form_cls.calls == [ + pretend.call(request.POST, api_token="fake-api-token") + ] + + def test_add_github_oidc_provider_preexisting(self, monkeypatch): + provider = pretend.stub( + id="fakeid", + provider_name="GitHub", + repository_name="fakerepo", + owner="fakeowner", + owner_id="1234", + workflow_name="fakeworkflow.yml", + ) + # NOTE: Can't set __str__ using pretend.stub() + monkeypatch.setattr(provider.__class__, "__str__", lambda s: "fakespecifier") + + project = pretend.stub( + name="fakeproject", + oidc_providers=[], + record_event=pretend.call_recorder(lambda *a, **kw: None), + ) + + request = pretend.stub( + flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), + session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), + POST=pretend.stub(), + db=pretend.stub( + query=lambda *a: pretend.stub( + filter=lambda *a: pretend.stub(one_or_none=lambda: provider) + ), + add=pretend.call_recorder(lambda o: None), + ), + registry=pretend.stub( + settings=pretend.stub( + get=pretend.call_recorder(lambda k: "fake-api-token") + ) + ), + remote_addr="0.0.0.0", + ) + + github_provider_form_obj = pretend.stub( + validate=pretend.call_recorder(lambda: True), + repository=pretend.stub(data=provider.repository_name), + normalized_owner=provider.owner, + workflow_name=pretend.stub(data=provider.workflow_name), + ) + github_provider_form_cls = pretend.call_recorder( + lambda *a, **kw: github_provider_form_obj + ) + monkeypatch.setattr(views, "GitHubProviderForm", github_provider_form_cls) + monkeypatch.setattr( + views, "project_owners", pretend.call_recorder(lambda *a: []) + ) + + view = views.ManageOIDCProviderViews(project, request) + monkeypatch.setattr( + view, "_hit_ratelimits", pretend.call_recorder(lambda: None) + ) + monkeypatch.setattr( + view, "_check_ratelimits", pretend.call_recorder(lambda: None) + ) + + assert view.add_github_oidc_provider() == { + "project": project, + "github_provider_form": github_provider_form_obj, + } + assert project.record_event.calls == [ + pretend.call( + tag="project:oidc:provider-added", + ip_address=request.remote_addr, + additional={ + "provider": "GitHub", + "id": "fakeid", + "specifier": "fakespecifier", + }, + ) + ] + assert request.session.flash.calls == [ + pretend.call( + "Added fakespecifier to fakeproject", + queue="success", + ) + ] + assert request.db.add.calls == [] + assert github_provider_form_obj.validate.calls == [pretend.call()] + assert views.project_owners.calls == [pretend.call(request, project)] + assert view._hit_ratelimits.calls == [pretend.call()] + assert view._check_ratelimits.calls == [pretend.call()] + assert project.oidc_providers == [provider] + + def test_add_github_oidc_provider_created(self, monkeypatch): + project = pretend.stub( + name="fakeproject", + oidc_providers=[], + record_event=pretend.call_recorder(lambda *a, **kw: None), + ) + + request = pretend.stub( + flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), + session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), + POST=pretend.stub(), + db=pretend.stub( + query=lambda *a: pretend.stub( + filter=lambda *a: pretend.stub(one_or_none=lambda: None) + ), + add=pretend.call_recorder(lambda o: setattr(o, "id", "fakeid")), + ), + registry=pretend.stub( + settings=pretend.stub( + get=pretend.call_recorder(lambda k: "fake-api-token") + ) + ), + remote_addr="0.0.0.0", + ) + + github_provider_form_obj = pretend.stub( + validate=pretend.call_recorder(lambda: True), + repository=pretend.stub(data="fakerepo"), + normalized_owner="fakeowner", + owner_id="1234", + workflow_name=pretend.stub(data="fakeworkflow.yml"), + ) + github_provider_form_cls = pretend.call_recorder( + lambda *a, **kw: github_provider_form_obj + ) + monkeypatch.setattr(views, "GitHubProviderForm", github_provider_form_cls) + monkeypatch.setattr( + views, "project_owners", pretend.call_recorder(lambda *a: []) + ) + + view = views.ManageOIDCProviderViews(project, request) + monkeypatch.setattr( + view, "_hit_ratelimits", pretend.call_recorder(lambda: None) + ) + monkeypatch.setattr( + view, "_check_ratelimits", pretend.call_recorder(lambda: None) + ) + + assert view.add_github_oidc_provider() == { + "project": project, + "github_provider_form": github_provider_form_obj, + } + assert project.record_event.calls == [ + pretend.call( + tag="project:oidc:provider-added", + ip_address=request.remote_addr, + additional={ + "provider": "GitHub", + "id": "fakeid", + "specifier": "fakeworkflow.yml @ fakeowner/fakerepo", + }, + ) + ] + assert request.session.flash.calls == [ + pretend.call( + "Added fakeworkflow.yml @ fakeowner/fakerepo to fakeproject", + queue="success", + ) + ] + assert request.db.add.calls == [pretend.call(project.oidc_providers[0])] + assert github_provider_form_obj.validate.calls == [pretend.call()] + assert views.project_owners.calls == [pretend.call(request, project)] + assert view._hit_ratelimits.calls == [pretend.call()] + assert view._check_ratelimits.calls == [pretend.call()] + assert len(project.oidc_providers) == 1 diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index b2dea4f2b643..1dc1a5d7e474 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -143,6 +143,17 @@ def user_projects(request): } +def project_owners(request, project): + """Return all users who are owners of the project.""" + owner_roles = ( + request.db.query(User.id) + .join(Role.user) + .filter(Role.role_name == "Owner", Role.project == project) + .subquery() + ) + return request.db.query(User).join(owner_roles, User.id == owner_roles.c.id).all() + + @view_defaults( route_name="manage.account", renderer="manage/account.html", @@ -1202,17 +1213,7 @@ def add_github_oidc_provider(self): return response # We only email project owners, since only owners can manage OIDC providers. - owner_roles = ( - self.request.db.query(User.id) - .join(Role.user) - .filter(Role.role_name == "Owner", Role.project == self.project) - .subquery() - ) - owner_users = ( - self.request.db.query(User) - .join(owner_roles, User.id == owner_roles.c.id) - .all() - ) + owner_users = project_owners(self.request, self.project) for user in owner_users: send_oidc_provider_added_email( self.request, user, project_name=self.project.name @@ -1263,17 +1264,7 @@ def delete_oidc_provider(self): return self.default_response # We only email project owners, since only owners can manage OIDC providers. - owner_roles = ( - self.request.db.query(User.id) - .join(Role.user) - .filter(Role.role_name == "Owner", Role.project == self.project) - .subquery() - ) - owner_users = ( - self.request.db.query(User) - .join(owner_roles, User.id == owner_roles.c.id) - .all() - ) + owner_users = project_owners(self.request, self.project) for user in owner_users: send_oidc_provider_removed_email( self.request, user, project_name=self.project.name diff --git a/warehouse/oidc/models.py b/warehouse/oidc/models.py index e2b14c6a5591..23deec3a82cc 100644 --- a/warehouse/oidc/models.py +++ b/warehouse/oidc/models.py @@ -156,7 +156,7 @@ def repository(self): @property def job_workflow_ref(self): - return f"{self.repository}/.github/workflows/{self.workflow_name}.yml" + return f"{self.repository}/.github/workflows/{self.workflow_name}@" @property def workflow(self): @@ -164,3 +164,6 @@ def workflow(self): def __str__(self): return f"{self.workflow_name} @ {self.repository}" + + def __eq__(self, other): + pass From 17b2473bb8a99da9f5a498594b2796b18a1cec34 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Thu, 10 Mar 2022 16:50:46 -0500 Subject: [PATCH 47/78] tests: more manage/view tests --- tests/unit/manage/test_views.py | 134 +++++++++++++++++++++++++++++++- 1 file changed, 132 insertions(+), 2 deletions(-) diff --git a/tests/unit/manage/test_views.py b/tests/unit/manage/test_views.py index dec73955a515..3a39d5606ca2 100644 --- a/tests/unit/manage/test_views.py +++ b/tests/unit/manage/test_views.py @@ -18,7 +18,12 @@ import pytest from paginate_sqlalchemy import SqlalchemyOrmPage as SQLAlchemyORMPage -from pyramid.httpexceptions import HTTPBadRequest, HTTPNotFound, HTTPSeeOther +from pyramid.httpexceptions import ( + HTTPBadRequest, + HTTPNotFound, + HTTPSeeOther, + HTTPTooManyRequests, +) from pyramid.response import Response from sqlalchemy.orm import joinedload from sqlalchemy.orm.exc import NoResultFound @@ -37,6 +42,7 @@ from warehouse.forklift.legacy import MAX_FILESIZE, MAX_PROJECT_SIZE from warehouse.macaroons.interfaces import IMacaroonService from warehouse.manage import views +from warehouse.oidc.interfaces import TooManyOIDCRegistrations from warehouse.packaging.models import ( File, JournalEntry, @@ -4890,8 +4896,14 @@ def test_add_github_oidc_provider_created(self, monkeypatch): lambda *a, **kw: github_provider_form_obj ) monkeypatch.setattr(views, "GitHubProviderForm", github_provider_form_cls) + fakeowners = [pretend.stub(), pretend.stub(), pretend.stub()] monkeypatch.setattr( - views, "project_owners", pretend.call_recorder(lambda *a: []) + views, "project_owners", pretend.call_recorder(lambda *a: fakeowners) + ) + monkeypatch.setattr( + views, + "send_oidc_provider_added_email", + pretend.call_recorder(lambda *a, **kw: None), ) view = views.ManageOIDCProviderViews(project, request) @@ -4926,6 +4938,124 @@ def test_add_github_oidc_provider_created(self, monkeypatch): assert request.db.add.calls == [pretend.call(project.oidc_providers[0])] assert github_provider_form_obj.validate.calls == [pretend.call()] assert views.project_owners.calls == [pretend.call(request, project)] + assert views.send_oidc_provider_added_email.calls == [ + pretend.call(request, fakeowner, project_name="fakeproject") + for fakeowner in fakeowners + ] assert view._hit_ratelimits.calls == [pretend.call()] assert view._check_ratelimits.calls == [pretend.call()] assert len(project.oidc_providers) == 1 + + def test_add_github_oidc_provider_already_registered_with_project( + self, monkeypatch + ): + provider = pretend.stub( + id="fakeid", + provider_name="GitHub", + repository_name="fakerepo", + owner="fakeowner", + owner_id="1234", + workflow_name="fakeworkflow.yml", + ) + # NOTE: Can't set __str__ using pretend.stub() + monkeypatch.setattr(provider.__class__, "__str__", lambda s: "fakespecifier") + + project = pretend.stub( + name="fakeproject", + oidc_providers=[provider], + record_event=pretend.call_recorder(lambda *a, **kw: None), + ) + + request = pretend.stub( + flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), + session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), + POST=pretend.stub(), + db=pretend.stub( + query=lambda *a: pretend.stub( + filter=lambda *a: pretend.stub(one_or_none=lambda: provider) + ), + ), + registry=pretend.stub( + settings=pretend.stub( + get=pretend.call_recorder(lambda k: "fake-api-token") + ) + ), + ) + + github_provider_form_obj = pretend.stub( + validate=pretend.call_recorder(lambda: True), + repository=pretend.stub(data=provider.repository_name), + normalized_owner=provider.owner, + workflow_name=pretend.stub(data=provider.workflow_name), + ) + github_provider_form_cls = pretend.call_recorder( + lambda *a, **kw: github_provider_form_obj + ) + monkeypatch.setattr(views, "GitHubProviderForm", github_provider_form_cls) + + view = views.ManageOIDCProviderViews(project, request) + monkeypatch.setattr( + view, "_hit_ratelimits", pretend.call_recorder(lambda: None) + ) + monkeypatch.setattr( + view, "_check_ratelimits", pretend.call_recorder(lambda: None) + ) + + assert view.add_github_oidc_provider() == { + "project": project, + "github_provider_form": github_provider_form_obj, + } + assert project.record_event.calls == [] + assert request.session.flash.calls == [ + pretend.call( + "fakespecifier is already registered with fakeproject", + queue="error", + ) + ] + + def test_add_github_oidc_provider_ratelimited(self, monkeypatch): + project = pretend.stub() + request = pretend.stub( + flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), + _=lambda s: s, + ) + + view = views.ManageOIDCProviderViews(project, request) + monkeypatch.setattr( + view, + "_check_ratelimits", + pretend.call_recorder( + pretend.raiser( + TooManyOIDCRegistrations( + resets_in=pretend.stub(total_seconds=lambda: 60) + ) + ) + ), + ) + + assert view.add_github_oidc_provider().__class__ == HTTPTooManyRequests + + def test_add_github_oidc_provider_admin_disabled(self, monkeypatch): + project = pretend.stub() + request = pretend.stub( + flags=pretend.stub(enabled=pretend.call_recorder(lambda f: True)), + session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), + _=lambda s: s, + ) + + view = views.ManageOIDCProviderViews(project, request) + default_response = {"_": pretend.stub()} + monkeypatch.setattr( + views.ManageOIDCProviderViews, "default_response", default_response + ) + + assert view.add_github_oidc_provider() == default_response + assert request.session.flash.calls == [ + pretend.call( + ( + "OpenID Connect is temporarily disabled. " + "See https://pypi.org/help#admin-intervention for details." + ), + queue="error", + ) + ] From 6d3130b65a233ff65c9c0e821f4c985d06ba4564 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Thu, 10 Mar 2022 17:21:07 -0500 Subject: [PATCH 48/78] tests, warehouse: ratelimit tests, fix bug --- tests/unit/manage/test_views.py | 59 +++++++++++++++++++++++++++++++++ warehouse/manage/views.py | 4 ++- 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/tests/unit/manage/test_views.py b/tests/unit/manage/test_views.py index 3a39d5606ca2..2dcd54722366 100644 --- a/tests/unit/manage/test_views.py +++ b/tests/unit/manage/test_views.py @@ -52,6 +52,7 @@ RoleInvitation, User, ) +from warehouse.rate_limiting import IRateLimiter from warehouse.utils.paginate import paginate_url_factory from warehouse.utils.project import remove_documentation @@ -4699,6 +4700,64 @@ def test_initializes(self): assert view.project is project assert view.request is request + @pytest.mark.parametrize( + "ip_exceeded, user_exceeded", + [ + (False, False), + (False, True), + (True, False), + ], + ) + def test_ratelimiting(self, ip_exceeded, user_exceeded): + project = pretend.stub() + + user_rate_limiter = pretend.stub( + hit=pretend.call_recorder(lambda *a, **kw: None), + test=pretend.call_recorder(lambda uid: not user_exceeded), + resets_in=pretend.call_recorder(lambda uid: pretend.stub()), + ) + ip_rate_limiter = pretend.stub( + hit=pretend.call_recorder(lambda *a, **kw: None), + test=pretend.call_recorder(lambda ip: not ip_exceeded), + resets_in=pretend.call_recorder(lambda uid: pretend.stub()), + ) + + def find_service(iface, name): + if name == "user_oidc.provider.register": + return user_rate_limiter + else: + return ip_rate_limiter + + request = pretend.stub( + find_service=pretend.call_recorder(find_service), + user=pretend.stub(id=pretend.stub()), + remote_addr=pretend.stub(), + ) + + view = views.ManageOIDCProviderViews(project, request) + + assert view._ratelimiters == { + "user.oidc": user_rate_limiter, + "ip.oidc": ip_rate_limiter, + } + assert request.find_service.calls == [ + pretend.call(IRateLimiter, name="user_oidc.provider.register"), + pretend.call(IRateLimiter, name="ip_oidc.provider.register"), + ] + + view._hit_ratelimits() + + assert user_rate_limiter.hit.calls == [ + pretend.call(request.user.id), + ] + assert ip_rate_limiter.hit.calls == [pretend.call(request.remote_addr)] + + if user_exceeded or ip_exceeded: + with pytest.raises(TooManyOIDCRegistrations): + view._check_ratelimits() + else: + view._check_ratelimits() + def test_manage_project_oidc_providers(self, monkeypatch): project = pretend.stub() request = pretend.stub( diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index 1dc1a5d7e474..571a5b3c6160 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -1123,7 +1123,9 @@ def _check_ratelimits(self): if not self._ratelimiters["ip.oidc"].test(self.request.remote_addr): raise TooManyOIDCRegistrations( - resets_in=self._ratelimiters["ip.oidc"].resets_in(self.remote_addr) + resets_in=self._ratelimiters["ip.oidc"].resets_in( + self.request.remote_addr + ) ) @property From 8bd4d044e17b9c996a1d4050c6f6a34e4fd43a92 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Thu, 10 Mar 2022 17:30:37 -0500 Subject: [PATCH 49/78] tests: round out ratelimiting --- tests/unit/manage/test_views.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/unit/manage/test_views.py b/tests/unit/manage/test_views.py index 2dcd54722366..9c45f6f8ff7e 100644 --- a/tests/unit/manage/test_views.py +++ b/tests/unit/manage/test_views.py @@ -5118,3 +5118,31 @@ def test_add_github_oidc_provider_admin_disabled(self, monkeypatch): queue="error", ) ] + + def test_add_github_oidc_provider_invalid_form(self, monkeypatch): + project = pretend.stub() + request = pretend.stub( + flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), + session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), + _=lambda s: s, + ) + + github_provider_form_obj = pretend.stub( + validate=pretend.call_recorder(lambda: False), + ) + github_provider_form_cls = pretend.call_recorder( + lambda *a, **kw: github_provider_form_obj + ) + monkeypatch.setattr(views, "GitHubProviderForm", github_provider_form_cls) + + view = views.ManageOIDCProviderViews(project, request) + default_response = {"github_provider_form": github_provider_form_obj} + monkeypatch.setattr( + views.ManageOIDCProviderViews, "default_response", default_response + ) + monkeypatch.setattr( + view, "_check_ratelimits", pretend.call_recorder(lambda: None) + ) + + assert view.add_github_oidc_provider() == default_response + assert github_provider_form_obj.validate.calls == [pretend.call()] From e8f1a8dfff7dab4587b6389468ef4c72e0093f42 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Thu, 10 Mar 2022 17:42:08 -0500 Subject: [PATCH 50/78] tests: more tests --- tests/unit/manage/test_views.py | 2 ++ tests/unit/oidc/test_services.py | 22 ++++++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/tests/unit/manage/test_views.py b/tests/unit/manage/test_views.py index 9c45f6f8ff7e..877a8d53a20b 100644 --- a/tests/unit/manage/test_views.py +++ b/tests/unit/manage/test_views.py @@ -5146,3 +5146,5 @@ def test_add_github_oidc_provider_invalid_form(self, monkeypatch): assert view.add_github_oidc_provider() == default_response assert github_provider_form_obj.validate.calls == [pretend.call()] + + # TODO: OIDC deletion tests diff --git a/tests/unit/oidc/test_services.py b/tests/unit/oidc/test_services.py index 1488b3821435..8f02053cfeea 100644 --- a/tests/unit/oidc/test_services.py +++ b/tests/unit/oidc/test_services.py @@ -461,3 +461,25 @@ def test_get_key_refresh_fails(self, monkeypatch): tags=["provider:example", "key_id:fake-key-id"], ) ] + + def test_get_key_for_token(self, monkeypatch): + token = pretend.stub() + key = pretend.stub() + + service = services.OIDCProviderService( + provider="example", + issuer_url="https://example.com", + cache_url="rediss://fake.example.com", + metrics=pretend.stub(), + ) + monkeypatch.setattr(service, "get_key", pretend.call_recorder(lambda kid: key)) + + monkeypatch.setattr( + services.jwt, + "get_unverified_header", + pretend.call_recorder(lambda token: {"kid": "fake-key-id"}), + ) + + assert service._get_key_for_token(token) == key + assert service.get_key.calls == [pretend.call("fake-key-id")] + assert services.jwt.get_unverified_header.calls == [pretend.call(token)] From ce65932ffeadfbb75ce342f6964004196567dc2e Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 11 Mar 2022 13:45:46 -0500 Subject: [PATCH 51/78] tests, warehouse: OIDC deletion tests Also, gets some coverage for free by reusing a helper. --- tests/unit/manage/test_views.py | 184 +++++++++++++++++++++++++++++++- warehouse/manage/views.py | 22 ++-- 2 files changed, 189 insertions(+), 17 deletions(-) diff --git a/tests/unit/manage/test_views.py b/tests/unit/manage/test_views.py index 877a8d53a20b..7d99d5950cd7 100644 --- a/tests/unit/manage/test_views.py +++ b/tests/unit/manage/test_views.py @@ -5147,4 +5147,186 @@ def test_add_github_oidc_provider_invalid_form(self, monkeypatch): assert view.add_github_oidc_provider() == default_response assert github_provider_form_obj.validate.calls == [pretend.call()] - # TODO: OIDC deletion tests + def test_delete_oidc_provider(self, monkeypatch): + provider = pretend.stub( + provider_name="fakeprovider", + id="fakeid", + ) + # NOTE: Can't set __str__ using pretend.stub() + monkeypatch.setattr(provider.__class__, "__str__", lambda s: "fakespecifier") + + project = pretend.stub( + oidc_providers=[provider], + name="fakeproject", + record_event=pretend.call_recorder(lambda *a, **kw: None), + ) + request = pretend.stub( + flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), + session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), + POST=pretend.stub(), + db=pretend.stub( + query=lambda *a: pretend.stub(get=lambda id: provider), + ), + remote_addr="0.0.0.0", + ) + + delete_provider_form_obj = pretend.stub( + validate=pretend.call_recorder(lambda: True), + provider_id=pretend.stub(data="fakeid"), + ) + delete_provider_form_cls = pretend.call_recorder( + lambda *a, **kw: delete_provider_form_obj + ) + monkeypatch.setattr(views, "DeleteProviderForm", delete_provider_form_cls) + + fakeowners = [pretend.stub(), pretend.stub(), pretend.stub()] + monkeypatch.setattr( + views, "project_owners", pretend.call_recorder(lambda *a: fakeowners) + ) + monkeypatch.setattr( + views, + "send_oidc_provider_removed_email", + pretend.call_recorder(lambda *a, **kw: None), + ) + + view = views.ManageOIDCProviderViews(project, request) + default_response = {"_": pretend.stub()} + monkeypatch.setattr( + views.ManageOIDCProviderViews, "default_response", default_response + ) + + assert view.delete_oidc_provider() == default_response + assert provider not in project.oidc_providers + + assert project.record_event.calls == [ + pretend.call( + tag="project:oidc:provider-removed", + ip_address=request.remote_addr, + additional={ + "provider": "fakeprovider", + "id": "fakeid", + "specifier": "fakespecifier", + }, + ) + ] + + assert request.flags.enabled.calls == [ + pretend.call(AdminFlagValue.DISALLOW_OIDC) + ] + assert request.session.flash.calls == [ + pretend.call("Removed fakespecifier from fakeproject", queue="success") + ] + + assert delete_provider_form_cls.calls == [pretend.call(request.POST)] + assert delete_provider_form_obj.validate.calls == [pretend.call()] + + assert views.project_owners.calls == [pretend.call(request, project)] + assert views.send_oidc_provider_removed_email.calls == [ + pretend.call(request, fakeowner, project_name="fakeproject") + for fakeowner in fakeowners + ] + + def test_delete_oidc_provider_invalid_form(self, monkeypatch): + provider = pretend.stub() + project = pretend.stub(oidc_providers=[provider]) + request = pretend.stub( + flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), + POST=pretend.stub(), + ) + + delete_provider_form_obj = pretend.stub( + validate=pretend.call_recorder(lambda: False), + ) + delete_provider_form_cls = pretend.call_recorder( + lambda *a, **kw: delete_provider_form_obj + ) + monkeypatch.setattr(views, "DeleteProviderForm", delete_provider_form_cls) + + view = views.ManageOIDCProviderViews(project, request) + default_response = {"_": pretend.stub()} + monkeypatch.setattr( + views.ManageOIDCProviderViews, "default_response", default_response + ) + + assert view.delete_oidc_provider() == default_response + assert len(project.oidc_providers) == 1 + + assert delete_provider_form_cls.calls == [pretend.call(request.POST)] + assert delete_provider_form_obj.validate.calls == [pretend.call()] + + @pytest.mark.parametrize( + "other_provider", [None, pretend.stub(id="different-fakeid")] + ) + def test_delete_oidc_provider_not_found(self, monkeypatch, other_provider): + provider = pretend.stub( + provider_name="fakeprovider", + id="fakeid", + ) + # NOTE: Can't set __str__ using pretend.stub() + monkeypatch.setattr(provider.__class__, "__str__", lambda s: "fakespecifier") + + project = pretend.stub( + oidc_providers=[provider], + name="fakeproject", + record_event=pretend.call_recorder(lambda *a, **kw: None), + ) + request = pretend.stub( + flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), + session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), + POST=pretend.stub(), + db=pretend.stub( + query=lambda *a: pretend.stub(get=lambda id: other_provider), + ), + remote_addr="0.0.0.0", + ) + + delete_provider_form_obj = pretend.stub( + validate=pretend.call_recorder(lambda: True), + provider_id=pretend.stub(data="different-fakeid"), + ) + delete_provider_form_cls = pretend.call_recorder( + lambda *a, **kw: delete_provider_form_obj + ) + monkeypatch.setattr(views, "DeleteProviderForm", delete_provider_form_cls) + + view = views.ManageOIDCProviderViews(project, request) + default_response = {"_": pretend.stub()} + monkeypatch.setattr( + views.ManageOIDCProviderViews, "default_response", default_response + ) + + assert view.delete_oidc_provider() == default_response + assert provider in project.oidc_providers # not deleted + assert other_provider not in project.oidc_providers + + assert project.record_event.calls == [] + assert request.session.flash.calls == [ + pretend.call("Invalid publisher for project", queue="error") + ] + + assert delete_provider_form_cls.calls == [pretend.call(request.POST)] + assert delete_provider_form_obj.validate.calls == [pretend.call()] + + def test_delete_oidc_provider_admin_disabled(self, monkeypatch): + project = pretend.stub() + request = pretend.stub( + flags=pretend.stub(enabled=pretend.call_recorder(lambda f: True)), + session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), + ) + + view = views.ManageOIDCProviderViews(project, request) + default_response = {"_": pretend.stub()} + monkeypatch.setattr( + views.ManageOIDCProviderViews, "default_response", default_response + ) + + assert view.delete_oidc_provider() == default_response + assert request.session.flash.calls == [ + pretend.call( + ( + "OpenID Connect is temporarily disabled. " + "See https://pypi.org/help#admin-intervention for details." + ), + queue="error", + ) + ] diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index 571a5b3c6160..836f52cecaeb 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -1260,7 +1260,7 @@ def delete_oidc_provider(self): # provider will be `None` here if someone manually futzes with the form. if provider is None or provider not in self.project.oidc_providers: self.request.session.flash( - f"Unknown publisher: {provider}", + "Invalid publisher for project", queue="error", ) return self.default_response @@ -1284,7 +1284,9 @@ def delete_oidc_provider(self): }, ) - self.request.session.flash(f"Removed {provider} from {self.project.name}") + self.request.session.flash( + f"Removed {provider} from {self.project.name}", queue="success" + ) return self.default_response @@ -2051,13 +2053,7 @@ def change_project_role(project, request, _form_class=ChangeRoleForm): }, ) - owner_roles = ( - request.db.query(Role) - .filter(Role.project == project) - .filter(Role.role_name == "Owner") - .all() - ) - owner_users = {owner.user for owner in owner_roles} + owner_users = set(project_owners(request, project)) # Don't send owner notification email to new user # if they are now an owner owner_users.discard(role.user) @@ -2128,13 +2124,7 @@ def delete_project_role(project, request): }, ) - owner_roles = ( - request.db.query(Role) - .filter(Role.project == project) - .filter(Role.role_name == "Owner") - .all() - ) - owner_users = {owner.user for owner in owner_roles} + owner_users = set(project_owners(request, project)) # Don't send owner notification email to new user # if they are now an owner owner_users.discard(role.user) From 792b30616c5f2c69b524c45179491081912b09e4 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 11 Mar 2022 15:07:09 -0500 Subject: [PATCH 52/78] tests, warehouse: fill in model checks Accommodations for testing. --- tests/unit/oidc/test_models.py | 130 +++++++++++++++++++++++++++++++++ warehouse/oidc/models.py | 35 ++++++--- 2 files changed, 154 insertions(+), 11 deletions(-) create mode 100644 tests/unit/oidc/test_models.py diff --git a/tests/unit/oidc/test_models.py b/tests/unit/oidc/test_models.py new file mode 100644 index 000000000000..6daac4130744 --- /dev/null +++ b/tests/unit/oidc/test_models.py @@ -0,0 +1,130 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import pretend + +from warehouse.oidc import models + + +class TestOIDCProvider: + def test_oidc_provider_not_default_verifiable(self): + provider = models.OIDCProvider(projects=[]) + + assert not provider.verify_claims(signed_claims={}) + + +class TestGitHubProvider: + def test_github_provider_all_known_claims(self): + assert models.GitHubProvider.all_known_claims() == { + # verifiable claims + "repository", + "job_workflow_ref", + "workflow", + # preverified claims + "iss", + "iat", + "nbf", + "exp", + "aud", + # unchecked claims + "actor", + "jti", + "sub", + "ref", + "sha", + "run_id", + "run_number", + "run_attempt", + "head_ref", + "base_ref", + "event_name", + "ref_type", + } + + def test_github_provider_computed_properties(self): + provider = models.GitHubProvider( + repository_name="fakerepo", + owner="fakeowner", + owner_id="fakeid", + workflow_name="fakeworkflow.yml", + ) + + for claim_name in provider.__verifiable_claims__.keys(): + assert getattr(provider, claim_name) is not None + + assert str(provider) == "fakeworkflow.yml @ fakeowner/fakerepo" + + def test_github_provider_unaccounted_claims(self, monkeypatch): + provider = models.GitHubProvider( + repository_name="fakerepo", + owner="fakeowner", + owner_id="fakeid", + workflow_name="fakeworkflow.yml", + ) + + sentry_sdk = pretend.stub(capture_message=pretend.call_recorder(lambda s: None)) + monkeypatch.setattr(models, "sentry_sdk", sentry_sdk) + + # We don't care if these actually verify, only that they're present. + signed_claims = { + claim_name: "fake" + for claim_name in models.GitHubProvider.all_known_claims() + } + signed_claims["fake-claim"] = "fake" + assert not provider.verify_claims(signed_claims=signed_claims) + assert sentry_sdk.capture_message.calls == [ + pretend.call( + "JWT for GitHubProvider has unaccounted claims: {'fake-claim'}" + ) + ] + + def test_github_provider_missing_claims(self, monkeypatch): + provider = models.GitHubProvider( + repository_name="fakerepo", + owner="fakeowner", + owner_id="fakeid", + workflow_name="fakeworkflow.yml", + ) + + sentry_sdk = pretend.stub(capture_message=pretend.call_recorder(lambda s: None)) + monkeypatch.setattr(models, "sentry_sdk", sentry_sdk) + + signed_claims = { + claim_name: "fake" + for claim_name in models.GitHubProvider.all_known_claims() + } + signed_claims.pop("repository") + assert not provider.verify_claims(signed_claims=signed_claims) + assert sentry_sdk.capture_message.calls == [ + pretend.call("JWT for GitHubProvider is missing claim: repository") + ] + + def test_github_provider_verifies(self, monkeypatch): + provider = models.GitHubProvider( + repository_name="fakerepo", + owner="fakeowner", + owner_id="fakeid", + workflow_name="fakeworkflow.yml", + ) + + noop_check = pretend.call_recorder(lambda l, r: True) + verifiable_claims = { + claim_name: noop_check for claim_name in provider.__verifiable_claims__ + } + monkeypatch.setattr(provider, "__verifiable_claims__", verifiable_claims) + + signed_claims = { + claim_name: "fake" + for claim_name in models.GitHubProvider.all_known_claims() + } + assert provider.verify_claims(signed_claims=signed_claims) + assert len(noop_check.calls) == len(verifiable_claims) diff --git a/warehouse/oidc/models.py b/warehouse/oidc/models.py index 23deec3a82cc..002addb81e6c 100644 --- a/warehouse/oidc/models.py +++ b/warehouse/oidc/models.py @@ -67,6 +67,17 @@ class OIDCProvider(db.Model): # not checked as part of verifying the JWT. __unchecked_claims__ = set() + @classmethod + def all_known_claims(cls): + """ + Returns all claims "known" to this provider. + """ + return ( + cls.__verifiable_claims__.keys() + | cls.__preverified_claims__ + | cls.__unchecked_claims__ + ) + def verify_claims(self, signed_claims): """ Given a JWT that has been successfully decoded (checked for a valid @@ -82,12 +93,7 @@ def verify_claims(self, signed_claims): # All claims should be accounted for. # The presence of an unaccounted claim is not an error, only a warning # that the JWT payload has changed. - known_claims = ( - self.__verifiable_claims__.keys() - | self.__preverified_claims__ - | self.__unchecked_claims__ - ) - unaccounted_claims = known_claims - signed_claims.keys() + unaccounted_claims = signed_claims.keys() - self.all_known_claims() if unaccounted_claims: sentry_sdk.capture_message( f"JWT for {self.__class__.__name__} has unaccounted claims: " @@ -96,13 +102,23 @@ def verify_claims(self, signed_claims): # Finally, perform the actual claim verification. for claim_name, check in self.__verifiable_claims__.items(): - if not check(self.getattr(claim_name), signed_claims[claim_name]): + # All verifiable claims are mandatory. The absence of a missing + # claim *is* an error, since it indicates a breaking change in the + # JWT's payload. + signed_claim = signed_claims.get(claim_name) + if signed_claim is None: + sentry_sdk.capture_message( + f"JWT for {self.__class__.__name__} is missing claim: {claim_name}" + ) + return False + + if not check(getattr(self, claim_name), signed_claim): return False return True @property - def provider_name(self): + def provider_name(self): # pragma: no cover # Only concrete subclasses of OIDCProvider are constructed. return NotImplemented @@ -164,6 +180,3 @@ def workflow(self): def __str__(self): return f"{self.workflow_name} @ {self.repository}" - - def __eq__(self, other): - pass From 2215f397edbace5b35b88f1dff5946bc52c114c5 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 11 Mar 2022 15:23:26 -0500 Subject: [PATCH 53/78] oidc/models: type hints --- warehouse/oidc/models.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/warehouse/oidc/models.py b/warehouse/oidc/models.py index 002addb81e6c..ccc58a7c4580 100644 --- a/warehouse/oidc/models.py +++ b/warehouse/oidc/models.py @@ -10,6 +10,9 @@ # See the License for the specific language governing permissions and # limitations under the License. + +from typing import Any, Callable, Dict, Set + import sentry_sdk from sqlalchemy import Column, ForeignKey, String, UniqueConstraint, orm @@ -39,7 +42,7 @@ class OIDCProvider(db.Model): discriminator = Column(String) projects = orm.relationship( Project, - secondary=OIDCProviderProjectAssociation.__table__, + secondary=OIDCProviderProjectAssociation.__table__, # type: ignore backref="oidc_providers", ) @@ -50,7 +53,7 @@ class OIDCProvider(db.Model): # A map of claim names to "check" functions, each of which # has the signature `check(ground-truth, signed-claim) -> bool`. - __verifiable_claims__ = dict() + __verifiable_claims__: Dict[str, Callable[[Any, Any], bool]] = dict() # Claims that have already been verified during the JWT signature # verification phase. @@ -65,7 +68,7 @@ class OIDCProvider(db.Model): # Individual providers should explicitly override this set, # indicating any custom claims that are known to be present but are # not checked as part of verifying the JWT. - __unchecked_claims__ = set() + __unchecked_claims__: Set[str] = set() @classmethod def all_known_claims(cls): From 1a21b4f1f621847bc38100840906a8d5affe712d Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 11 Mar 2022 15:23:42 -0500 Subject: [PATCH 54/78] warehouse/locale: `make translations` --- warehouse/locale/messages.pot | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/warehouse/locale/messages.pot b/warehouse/locale/messages.pot index e862be71e634..7204e49090eb 100644 --- a/warehouse/locale/messages.pot +++ b/warehouse/locale/messages.pot @@ -247,33 +247,33 @@ msgid "" "again later." msgstr "" -#: warehouse/manage/views.py:1833 +#: warehouse/manage/views.py:1835 msgid "User '${username}' already has ${role_name} role for project" msgstr "" -#: warehouse/manage/views.py:1844 +#: warehouse/manage/views.py:1846 msgid "" "User '${username}' does not have a verified primary email address and " "cannot be added as a ${role_name} for project" msgstr "" -#: warehouse/manage/views.py:1857 +#: warehouse/manage/views.py:1859 msgid "User '${username}' already has an active invite. Please try again later." msgstr "" -#: warehouse/manage/views.py:1915 +#: warehouse/manage/views.py:1917 msgid "Invitation sent to '${username}'" msgstr "" -#: warehouse/manage/views.py:1962 +#: warehouse/manage/views.py:1964 msgid "Could not find role invitation." msgstr "" -#: warehouse/manage/views.py:1973 +#: warehouse/manage/views.py:1975 msgid "Invitation already expired." msgstr "" -#: warehouse/manage/views.py:1997 +#: warehouse/manage/views.py:1999 msgid "Invitation revoked from '${username}'." msgstr "" From cc0c21a2cae85dac6afd51985d87731803f076c6 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 14 Mar 2022 12:52:22 -0400 Subject: [PATCH 55/78] tests, warehouse: site-wide OIDC feature flag --- tests/unit/manage/test_views.py | 107 +++++++++++++++------ tests/unit/test_config.py | 1 + warehouse/config.py | 9 ++ warehouse/manage/views.py | 11 +++ warehouse/templates/manage/publishing.html | 9 +- 5 files changed, 108 insertions(+), 29 deletions(-) diff --git a/tests/unit/manage/test_views.py b/tests/unit/manage/test_views.py index 7d99d5950cd7..1053d3bebf73 100644 --- a/tests/unit/manage/test_views.py +++ b/tests/unit/manage/test_views.py @@ -4694,11 +4694,14 @@ def test_raises_404_with_out_of_range_page(self, db_request): class TestManageOIDCProviderViews: def test_initializes(self): project = pretend.stub() - request = pretend.stub() + request = pretend.stub( + registry=pretend.stub(settings={"warehouse.oidc.enabled": True}) + ) view = views.ManageOIDCProviderViews(project, request) assert view.project is project assert view.request is request + assert view.oidc_enabled @pytest.mark.parametrize( "ip_exceeded, user_exceeded", @@ -4729,6 +4732,7 @@ def find_service(iface, name): return ip_rate_limiter request = pretend.stub( + registry=pretend.stub(settings={"warehouse.oidc.enabled": True}), find_service=pretend.call_recorder(find_service), user=pretend.stub(id=pretend.stub()), remote_addr=pretend.stub(), @@ -4761,13 +4765,14 @@ def find_service(iface, name): def test_manage_project_oidc_providers(self, monkeypatch): project = pretend.stub() request = pretend.stub( - flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), - POST=pretend.stub(), registry=pretend.stub( - settings=pretend.stub( - get=pretend.call_recorder(lambda k: "fake-api-token") - ) + settings={ + "warehouse.oidc.enabled": True, + "github.token": "fake-api-token", + } ), + flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), + POST=pretend.stub(), ) github_provider_form_obj = pretend.stub() @@ -4778,6 +4783,7 @@ def test_manage_project_oidc_providers(self, monkeypatch): view = views.ManageOIDCProviderViews(project, request) assert view.manage_project_oidc_providers() == { + "oidc_enabled": True, "project": project, "github_provider_form": github_provider_form_obj, } @@ -4785,7 +4791,6 @@ def test_manage_project_oidc_providers(self, monkeypatch): assert request.flags.enabled.calls == [ pretend.call(AdminFlagValue.DISALLOW_OIDC) ] - assert request.registry.settings.get.calls == [pretend.call("github.token")] assert github_provider_form_cls.calls == [ pretend.call(request.POST, api_token="fake-api-token") ] @@ -4793,14 +4798,15 @@ def test_manage_project_oidc_providers(self, monkeypatch): def test_manage_project_oidc_providers_admin_disabled(self, monkeypatch): project = pretend.stub() request = pretend.stub( + registry=pretend.stub( + settings={ + "warehouse.oidc.enabled": True, + "github.token": "fake-api-token", + } + ), flags=pretend.stub(enabled=pretend.call_recorder(lambda f: True)), session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), POST=pretend.stub(), - registry=pretend.stub( - settings=pretend.stub( - get=pretend.call_recorder(lambda k: "fake-api-token") - ) - ), ) view = views.ManageOIDCProviderViews(project, request) @@ -4812,6 +4818,7 @@ def test_manage_project_oidc_providers_admin_disabled(self, monkeypatch): view = views.ManageOIDCProviderViews(project, request) assert view.manage_project_oidc_providers() == { + "oidc_enabled": True, "project": project, "github_provider_form": github_provider_form_obj, } @@ -4828,7 +4835,6 @@ def test_manage_project_oidc_providers_admin_disabled(self, monkeypatch): queue="error", ) ] - assert request.registry.settings.get.calls == [pretend.call("github.token")] assert github_provider_form_cls.calls == [ pretend.call(request.POST, api_token="fake-api-token") ] @@ -4852,6 +4858,12 @@ def test_add_github_oidc_provider_preexisting(self, monkeypatch): ) request = pretend.stub( + registry=pretend.stub( + settings={ + "warehouse.oidc.enabled": True, + "github.token": "fake-api-token", + } + ), flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), POST=pretend.stub(), @@ -4861,11 +4873,6 @@ def test_add_github_oidc_provider_preexisting(self, monkeypatch): ), add=pretend.call_recorder(lambda o: None), ), - registry=pretend.stub( - settings=pretend.stub( - get=pretend.call_recorder(lambda k: "fake-api-token") - ) - ), remote_addr="0.0.0.0", ) @@ -4892,6 +4899,7 @@ def test_add_github_oidc_provider_preexisting(self, monkeypatch): ) assert view.add_github_oidc_provider() == { + "oidc_enabled": True, "project": project, "github_provider_form": github_provider_form_obj, } @@ -4927,6 +4935,12 @@ def test_add_github_oidc_provider_created(self, monkeypatch): ) request = pretend.stub( + registry=pretend.stub( + settings={ + "warehouse.oidc.enabled": True, + "github.token": "fake-api-token", + } + ), flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), POST=pretend.stub(), @@ -4936,11 +4950,6 @@ def test_add_github_oidc_provider_created(self, monkeypatch): ), add=pretend.call_recorder(lambda o: setattr(o, "id", "fakeid")), ), - registry=pretend.stub( - settings=pretend.stub( - get=pretend.call_recorder(lambda k: "fake-api-token") - ) - ), remote_addr="0.0.0.0", ) @@ -4974,6 +4983,7 @@ def test_add_github_oidc_provider_created(self, monkeypatch): ) assert view.add_github_oidc_provider() == { + "oidc_enabled": True, "project": project, "github_provider_form": github_provider_form_obj, } @@ -5026,6 +5036,12 @@ def test_add_github_oidc_provider_already_registered_with_project( ) request = pretend.stub( + registry=pretend.stub( + settings={ + "warehouse.oidc.enabled": True, + "github.token": "fake-api-token", + } + ), flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), POST=pretend.stub(), @@ -5034,11 +5050,6 @@ def test_add_github_oidc_provider_already_registered_with_project( filter=lambda *a: pretend.stub(one_or_none=lambda: provider) ), ), - registry=pretend.stub( - settings=pretend.stub( - get=pretend.call_recorder(lambda k: "fake-api-token") - ) - ), ) github_provider_form_obj = pretend.stub( @@ -5061,6 +5072,7 @@ def test_add_github_oidc_provider_already_registered_with_project( ) assert view.add_github_oidc_provider() == { + "oidc_enabled": True, "project": project, "github_provider_form": github_provider_form_obj, } @@ -5075,6 +5087,11 @@ def test_add_github_oidc_provider_already_registered_with_project( def test_add_github_oidc_provider_ratelimited(self, monkeypatch): project = pretend.stub() request = pretend.stub( + registry=pretend.stub( + settings={ + "warehouse.oidc.enabled": True, + } + ), flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), _=lambda s: s, ) @@ -5094,9 +5111,24 @@ def test_add_github_oidc_provider_ratelimited(self, monkeypatch): assert view.add_github_oidc_provider().__class__ == HTTPTooManyRequests + def test_add_github_oidc_provider_oidc_not_enabled(self, monkeypatch): + project = pretend.stub() + request = pretend.stub( + registry=pretend.stub(settings={"warehouse.oidc.enabled": False}), + ) + + view = views.ManageOIDCProviderViews(project, request) + default_response = {"_": pretend.stub()} + monkeypatch.setattr( + views.ManageOIDCProviderViews, "default_response", default_response + ) + + assert view.add_github_oidc_provider() == default_response + def test_add_github_oidc_provider_admin_disabled(self, monkeypatch): project = pretend.stub() request = pretend.stub( + registry=pretend.stub(settings={"warehouse.oidc.enabled": True}), flags=pretend.stub(enabled=pretend.call_recorder(lambda f: True)), session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), _=lambda s: s, @@ -5122,6 +5154,7 @@ def test_add_github_oidc_provider_admin_disabled(self, monkeypatch): def test_add_github_oidc_provider_invalid_form(self, monkeypatch): project = pretend.stub() request = pretend.stub( + registry=pretend.stub(settings={"warehouse.oidc.enabled": True}), flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), _=lambda s: s, @@ -5161,6 +5194,7 @@ def test_delete_oidc_provider(self, monkeypatch): record_event=pretend.call_recorder(lambda *a, **kw: None), ) request = pretend.stub( + registry=pretend.stub(settings={"warehouse.oidc.enabled": True}), flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), POST=pretend.stub(), @@ -5230,6 +5264,7 @@ def test_delete_oidc_provider_invalid_form(self, monkeypatch): provider = pretend.stub() project = pretend.stub(oidc_providers=[provider]) request = pretend.stub( + registry=pretend.stub(settings={"warehouse.oidc.enabled": True}), flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), POST=pretend.stub(), ) @@ -5271,6 +5306,7 @@ def test_delete_oidc_provider_not_found(self, monkeypatch, other_provider): record_event=pretend.call_recorder(lambda *a, **kw: None), ) request = pretend.stub( + registry=pretend.stub(settings={"warehouse.oidc.enabled": True}), flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), POST=pretend.stub(), @@ -5307,9 +5343,24 @@ def test_delete_oidc_provider_not_found(self, monkeypatch, other_provider): assert delete_provider_form_cls.calls == [pretend.call(request.POST)] assert delete_provider_form_obj.validate.calls == [pretend.call()] + def test_delete_oidc_provider_oidc_not_enabled(self, monkeypatch): + project = pretend.stub() + request = pretend.stub( + registry=pretend.stub(settings={"warehouse.oidc.enabled": False}), + ) + + view = views.ManageOIDCProviderViews(project, request) + default_response = {"_": pretend.stub()} + monkeypatch.setattr( + views.ManageOIDCProviderViews, "default_response", default_response + ) + + assert view.delete_oidc_provider() == default_response + def test_delete_oidc_provider_admin_disabled(self, monkeypatch): project = pretend.stub() request = pretend.stub( + registry=pretend.stub(settings={"warehouse.oidc.enabled": True}), flags=pretend.stub(enabled=pretend.call_recorder(lambda f: True)), session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), ) diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py index edcd836c786f..3b0f87276a3b 100644 --- a/tests/unit/test_config.py +++ b/tests/unit/test_config.py @@ -260,6 +260,7 @@ def __init__(self): "warehouse.two_factor_requirement.enabled": False, "warehouse.two_factor_mandate.available": False, "warehouse.two_factor_mandate.enabled": False, + "warehouse.oidc.enabled": False, } if environment == config.Environment.development: expected_settings.update( diff --git a/warehouse/config.py b/warehouse/config.py index 7036ae68f6cf..4b469a968457 100644 --- a/warehouse/config.py +++ b/warehouse/config.py @@ -306,6 +306,15 @@ def configure(settings=None): default=False, ) + # OIDC feature flags + maybe_set( + settings, + "warehouse.oidc.enabled", + "OIDC_ENABLED", + coercer=distutils.util.strtobool, + default=False, + ) + # Add the settings we use when the environment is set to development. if settings["warehouse.env"] == Environment.development: settings.setdefault("enforce_https", False) diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index 836f52cecaeb..e554a676d840 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -1097,6 +1097,7 @@ class ManageOIDCProviderViews: def __init__(self, project, request): self.request = request self.project = project + self.oidc_enabled = self.request.registry.settings["warehouse.oidc.enabled"] @property def _ratelimiters(self): @@ -1138,12 +1139,16 @@ def github_provider_form(self): @property def default_response(self): return { + "oidc_enabled": self.oidc_enabled, "project": self.project, "github_provider_form": self.github_provider_form, } @view_config(request_method="GET") def manage_project_oidc_providers(self): + # NOTE: We don't need a special "OIDC enabled" check here, since + # this view handler has no special logic. + if self.request.flags.enabled(AdminFlagValue.DISALLOW_OIDC): self.request.session.flash( ( @@ -1157,6 +1162,9 @@ def manage_project_oidc_providers(self): @view_config(request_method="POST", request_param=GitHubProviderForm.__params__) def add_github_oidc_provider(self): + if not self.oidc_enabled: + return self.default_response + if self.request.flags.enabled(AdminFlagValue.DISALLOW_OIDC): self.request.session.flash( ( @@ -1242,6 +1250,9 @@ def add_github_oidc_provider(self): @view_config(request_method="POST", request_param=DeleteProviderForm.__params__) def delete_oidc_provider(self): + if not self.oidc_enabled: + return self.default_response + if self.request.flags.enabled(AdminFlagValue.DISALLOW_OIDC): self.request.session.flash( ( diff --git a/warehouse/templates/manage/publishing.html b/warehouse/templates/manage/publishing.html index 0ca347afa997..aa46a0db0339 100644 --- a/warehouse/templates/manage/publishing.html +++ b/warehouse/templates/manage/publishing.html @@ -66,7 +66,7 @@

    {% trans %}OpenID Connect publisher management{% endtrans

    {% trans %}Add a new provider{% endtrans %}

    - + {% if oidc_enabled %}

    GitHub

    @@ -142,6 +142,13 @@

    {% trans %}Manage current providers{% endtrans %}

    {% else %}

    {% trans %}No publishers are currently configured.{% endtrans %}

    {% endif %} + {% else %} +

    + {% trans %} + OpenID Connect publishing isn't enabled yet! Come back soon. + {% endtrans %} +

    + {% endif %}
  • From 3fa705e38aba965fbc7070fa38ab708ca0add7db Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 14 Mar 2022 13:03:30 -0400 Subject: [PATCH 56/78] warehouse: `make translations` --- warehouse/locale/messages.pot | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/warehouse/locale/messages.pot b/warehouse/locale/messages.pot index 5231a7b8bd78..5f84dc9c7258 100644 --- a/warehouse/locale/messages.pot +++ b/warehouse/locale/messages.pot @@ -241,39 +241,39 @@ msgstr "" msgid "Generating new recovery codes will invalidate your existing codes." msgstr "" -#: warehouse/manage/views.py:1174 +#: warehouse/manage/views.py:1182 msgid "" "There have been too many attempted OpenID Connect registrations. Try " "again later." msgstr "" -#: warehouse/manage/views.py:1835 +#: warehouse/manage/views.py:1846 msgid "User '${username}' already has ${role_name} role for project" msgstr "" -#: warehouse/manage/views.py:1846 +#: warehouse/manage/views.py:1857 msgid "" "User '${username}' does not have a verified primary email address and " "cannot be added as a ${role_name} for project" msgstr "" -#: warehouse/manage/views.py:1859 +#: warehouse/manage/views.py:1870 msgid "User '${username}' already has an active invite. Please try again later." msgstr "" -#: warehouse/manage/views.py:1917 +#: warehouse/manage/views.py:1928 msgid "Invitation sent to '${username}'" msgstr "" -#: warehouse/manage/views.py:1964 +#: warehouse/manage/views.py:1975 msgid "Could not find role invitation." msgstr "" -#: warehouse/manage/views.py:1975 +#: warehouse/manage/views.py:1986 msgid "Invitation already expired." msgstr "" -#: warehouse/manage/views.py:1999 +#: warehouse/manage/views.py:2010 msgid "Invitation revoked from '${username}'." msgstr "" @@ -3057,6 +3057,13 @@ msgstr "" msgid "No publishers are currently configured." msgstr "" +#: warehouse/templates/manage/publishing.html:147 +msgid "" +"\n" +" OpenID Connect publishing isn't enabled yet! Come back soon.\n" +" " +msgstr "" + #: warehouse/templates/manage/release.html:18 #, python-format msgid "Manage '%(project_name)s' – release version %(version)s" From c2ca980fc4ae3551631d5434212b4b7a0d2f1085 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Thu, 17 Mar 2022 14:30:32 -0400 Subject: [PATCH 57/78] treewide: route to 404 when OIDC is disabled Enable OIDC by default for development environments; update tests. --- dev/environment | 1 + tests/unit/manage/test_views.py | 29 +++++++++++-------- warehouse/manage/views.py | 11 ++++--- .../includes/manage/manage-project-menu.html | 2 ++ 4 files changed, 27 insertions(+), 16 deletions(-) diff --git a/dev/environment b/dev/environment index 04c22c4d14c4..dcffbd5621b1 100644 --- a/dev/environment +++ b/dev/environment @@ -49,3 +49,4 @@ GITHUB_TOKEN_SCANNING_META_API_URL="http://notgithub:8000/meta/public_keys/token TWOFACTORREQUIREMENT_ENABLED=true TWOFACTORMANDATE_AVAILABLE=true TWOFACTORMANDATE_ENABLED=true +OIDC_ENABLED=true diff --git a/tests/unit/manage/test_views.py b/tests/unit/manage/test_views.py index 1053d3bebf73..28438b843f87 100644 --- a/tests/unit/manage/test_views.py +++ b/tests/unit/manage/test_views.py @@ -4839,6 +4839,17 @@ def test_manage_project_oidc_providers_admin_disabled(self, monkeypatch): pretend.call(request.POST, api_token="fake-api-token") ] + def test_manage_project_oidc_providers_oidc_not_enabled(self): + project = pretend.stub() + request = pretend.stub( + registry=pretend.stub(settings={"warehouse.oidc.enabled": False}), + ) + + view = views.ManageOIDCProviderViews(project, request) + + with pytest.raises(HTTPNotFound): + view.manage_project_oidc_providers() + def test_add_github_oidc_provider_preexisting(self, monkeypatch): provider = pretend.stub( id="fakeid", @@ -5111,19 +5122,16 @@ def test_add_github_oidc_provider_ratelimited(self, monkeypatch): assert view.add_github_oidc_provider().__class__ == HTTPTooManyRequests - def test_add_github_oidc_provider_oidc_not_enabled(self, monkeypatch): + def test_add_github_oidc_provider_oidc_not_enabled(self): project = pretend.stub() request = pretend.stub( registry=pretend.stub(settings={"warehouse.oidc.enabled": False}), ) view = views.ManageOIDCProviderViews(project, request) - default_response = {"_": pretend.stub()} - monkeypatch.setattr( - views.ManageOIDCProviderViews, "default_response", default_response - ) - assert view.add_github_oidc_provider() == default_response + with pytest.raises(HTTPNotFound): + view.add_github_oidc_provider() def test_add_github_oidc_provider_admin_disabled(self, monkeypatch): project = pretend.stub() @@ -5343,19 +5351,16 @@ def test_delete_oidc_provider_not_found(self, monkeypatch, other_provider): assert delete_provider_form_cls.calls == [pretend.call(request.POST)] assert delete_provider_form_obj.validate.calls == [pretend.call()] - def test_delete_oidc_provider_oidc_not_enabled(self, monkeypatch): + def test_delete_oidc_provider_oidc_not_enabled(self): project = pretend.stub() request = pretend.stub( registry=pretend.stub(settings={"warehouse.oidc.enabled": False}), ) view = views.ManageOIDCProviderViews(project, request) - default_response = {"_": pretend.stub()} - monkeypatch.setattr( - views.ManageOIDCProviderViews, "default_response", default_response - ) - assert view.delete_oidc_provider() == default_response + with pytest.raises(HTTPNotFound): + view.delete_oidc_provider() def test_delete_oidc_provider_admin_disabled(self, monkeypatch): project = pretend.stub() diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index e554a676d840..f20131b21bf3 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -1146,8 +1146,8 @@ def default_response(self): @view_config(request_method="GET") def manage_project_oidc_providers(self): - # NOTE: We don't need a special "OIDC enabled" check here, since - # this view handler has no special logic. + if not self.oidc_enabled: + raise HTTPNotFound if self.request.flags.enabled(AdminFlagValue.DISALLOW_OIDC): self.request.session.flash( @@ -1163,7 +1163,7 @@ def manage_project_oidc_providers(self): @view_config(request_method="POST", request_param=GitHubProviderForm.__params__) def add_github_oidc_provider(self): if not self.oidc_enabled: - return self.default_response + raise HTTPNotFound if self.request.flags.enabled(AdminFlagValue.DISALLOW_OIDC): self.request.session.flash( @@ -1251,7 +1251,7 @@ def add_github_oidc_provider(self): @view_config(request_method="POST", request_param=DeleteProviderForm.__params__) def delete_oidc_provider(self): if not self.oidc_enabled: - return self.default_response + raise HTTPNotFound if self.request.flags.enabled(AdminFlagValue.DISALLOW_OIDC): self.request.session.flash( @@ -1283,6 +1283,9 @@ def delete_oidc_provider(self): self.request, user, project_name=self.project.name ) + # NOTE: We remove the provider from the project, but we don't actually + # delete the provider model itself (since it might be associated + # with other projects). self.project.oidc_providers.remove(provider) self.project.record_event( diff --git a/warehouse/templates/includes/manage/manage-project-menu.html b/warehouse/templates/includes/manage/manage-project-menu.html index 982b9ed3cc1d..d8dfb5b72eb0 100644 --- a/warehouse/templates/includes/manage/manage-project-menu.html +++ b/warehouse/templates/includes/manage/manage-project-menu.html @@ -45,12 +45,14 @@ {% endif %} + {% if request.registry.settings["warehouse.oidc.enabled"] %}
  • {% trans %}Publishing{% endtrans %}
  • + {% endif %}
  • From f6b2cf0cc319bf849d6deeccb2437bc47e1efe80 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Thu, 17 Mar 2022 14:41:55 -0400 Subject: [PATCH 58/78] warehouse: `make translations` --- warehouse/locale/messages.pot | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/warehouse/locale/messages.pot b/warehouse/locale/messages.pot index 871e875478aa..9d569f9cf413 100644 --- a/warehouse/locale/messages.pot +++ b/warehouse/locale/messages.pot @@ -243,33 +243,33 @@ msgid "" "again later." msgstr "" -#: warehouse/manage/views.py:1846 +#: warehouse/manage/views.py:1849 msgid "User '${username}' already has ${role_name} role for project" msgstr "" -#: warehouse/manage/views.py:1857 +#: warehouse/manage/views.py:1860 msgid "" "User '${username}' does not have a verified primary email address and " "cannot be added as a ${role_name} for project" msgstr "" -#: warehouse/manage/views.py:1870 +#: warehouse/manage/views.py:1873 msgid "User '${username}' already has an active invite. Please try again later." msgstr "" -#: warehouse/manage/views.py:1928 +#: warehouse/manage/views.py:1931 msgid "Invitation sent to '${username}'" msgstr "" -#: warehouse/manage/views.py:1975 +#: warehouse/manage/views.py:1978 msgid "Could not find role invitation." msgstr "" -#: warehouse/manage/views.py:1986 +#: warehouse/manage/views.py:1989 msgid "Invitation already expired." msgstr "" -#: warehouse/manage/views.py:2010 +#: warehouse/manage/views.py:2013 msgid "Invitation revoked from '${username}'." msgstr "" @@ -1814,11 +1814,11 @@ msgstr "" msgid "Documentation" msgstr "" -#: warehouse/templates/includes/manage/manage-project-menu.html:51 +#: warehouse/templates/includes/manage/manage-project-menu.html:52 msgid "Publishing" msgstr "" -#: warehouse/templates/includes/manage/manage-project-menu.html:57 +#: warehouse/templates/includes/manage/manage-project-menu.html:59 msgid "Settings" msgstr "" From 8912a405655c98802a05d4da0016b5ef1a47f8ff Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 21 Mar 2022 16:46:42 -0700 Subject: [PATCH 59/78] Update warehouse/templates/manage/publishing.html Co-authored-by: Joachim Jablon --- warehouse/templates/manage/publishing.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/warehouse/templates/manage/publishing.html b/warehouse/templates/manage/publishing.html index aa46a0db0339..5187785d6063 100644 --- a/warehouse/templates/manage/publishing.html +++ b/warehouse/templates/manage/publishing.html @@ -86,7 +86,7 @@

    GitHub

    {% trans %}(required){% endtrans %} {% endif %} - {{ github_provider_form.owner(placeholder=gettext("owner"), autocomplete="off", autocapitalize="off", spellcheck="false", class_="form-group__field", **{"aria-describedby":"owner-errors"}) }} + {{ github_provider_form.owner(placeholder=gettext("owner"), autocomplete="off", autocapitalize="off", spellcheck="false", class_="form-group__field", aria_describedby="owner-errors") }}
    {{ field_errors(github_provider_form.owner) }}
    From 7482d789a946c418e64fa1c362c5aa211a5f8862 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 21 Mar 2022 16:56:22 -0700 Subject: [PATCH 60/78] oidc/{interfaces,services}: simplify API --- warehouse/oidc/interfaces.py | 4 +--- warehouse/oidc/services.py | 6 +++--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/warehouse/oidc/interfaces.py b/warehouse/oidc/interfaces.py index 9c7e21bab889..4e0171c7fc66 100644 --- a/warehouse/oidc/interfaces.py +++ b/warehouse/oidc/interfaces.py @@ -31,9 +31,7 @@ def get_key(key_id): def verify_signature_only(token): """ Verify the given JWT's signature and basic claims, returning - a tuple of (valid, decoded) where `valid` indicates - the validity of the JWT and `decoded` is the decoded JWT, - or `None` if invalid. + the decoded JWT, or `None` if invalid. This function **does not** verify the token's suitability for a particular action; subsequent checks on the decoded token's diff --git a/warehouse/oidc/services.py b/warehouse/oidc/services.py index 4b70193a2033..5b5f949b5fbc 100644 --- a/warehouse/oidc/services.py +++ b/warehouse/oidc/services.py @@ -184,15 +184,15 @@ def verify_signature_only(self, token): audience="pypi", leeway=30, ) - return True, valid_token + return valid_token except jwt.PyJWTError: - return False, None + return None except Exception as e: # We expect pyjwt to only raise subclasses of PyJWTError, but # we can't enforce this. Other exceptions indicate an abstraction # leak, so we log them for upstream reporting. sentry_sdk.capture_message(f"JWT verify raised generic error: {e}") - return False, None + return None class OIDCProviderServiceFactory: From 17eae838595dd452d7ecfb520210dbe21d571a17 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 21 Mar 2022 17:00:41 -0700 Subject: [PATCH 61/78] tests: update --- tests/unit/oidc/test_services.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/oidc/test_services.py b/tests/unit/oidc/test_services.py index 8f02053cfeea..24b3bb5e99ab 100644 --- a/tests/unit/oidc/test_services.py +++ b/tests/unit/oidc/test_services.py @@ -69,7 +69,7 @@ def test_verify_signature_only(self, monkeypatch): ) monkeypatch.setattr(services, "jwt", jwt) - assert service.verify_signature_only(token) == (True, decoded) + assert service.verify_signature_only(token) == decoded assert jwt.decode.calls == [ pretend.call( token, @@ -104,7 +104,7 @@ def test_verify_signature_only_fails(self, monkeypatch, exc): ) monkeypatch.setattr(services, "jwt", jwt) - assert service.verify_signature_only(token) == (False, None) + assert service.verify_signature_only(token) is None def test_get_keyset_not_cached(self, monkeypatch): service = services.OIDCProviderService( From c94b2b8aa4f962c3452f13c844d4fe4755ace594 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 21 Mar 2022 17:00:49 -0700 Subject: [PATCH 62/78] warehouse/migrations: rebase --- .../versions/f345394c444f_add_initial_oidc_provider_models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py b/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py index 138ebe9e97f8..cb874b05a0cc 100644 --- a/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py +++ b/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py @@ -13,7 +13,7 @@ Add initial OIDC provider models Revision ID: f345394c444f -Revises: 19cf76d2d459 +Revises: fdf9e337538a Create Date: 2022-02-15 21:11:41.693791 """ @@ -23,7 +23,7 @@ from sqlalchemy.dialects import postgresql revision = "f345394c444f" -down_revision = "19cf76d2d459" +down_revision = "fdf9e337538a" # Note: It is VERY important to ensure that a migration does not lock for a # long period of time and to ensure that each individual migration does From 1d4e12b7b0b4c140b07b6b7efee78fbd19cd54d6 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 21 Mar 2022 17:20:31 -0700 Subject: [PATCH 63/78] tests, warehouse: move ratelimit hit up --- tests/unit/manage/test_views.py | 5 +++++ warehouse/manage/views.py | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/unit/manage/test_views.py b/tests/unit/manage/test_views.py index 28438b843f87..88e23c6eb6aa 100644 --- a/tests/unit/manage/test_views.py +++ b/tests/unit/manage/test_views.py @@ -5184,8 +5184,13 @@ def test_add_github_oidc_provider_invalid_form(self, monkeypatch): monkeypatch.setattr( view, "_check_ratelimits", pretend.call_recorder(lambda: None) ) + monkeypatch.setattr( + view, "_hit_ratelimits", pretend.call_recorder(lambda: None) + ) assert view.add_github_oidc_provider() == default_response + assert view._hit_ratelimits.calls == [pretend.call()] + assert view._check_ratelimits.calls == [pretend.call()] assert github_provider_form_obj.validate.calls == [pretend.call()] def test_delete_oidc_provider(self, monkeypatch): diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index f20131b21bf3..87d9fd5d93d4 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -1186,11 +1186,12 @@ def add_github_oidc_provider(self): retry_after=exc.resets_in.total_seconds(), ) + self._hit_ratelimits() + response = self.default_response form = response["github_provider_form"] if form.validate(): - self._hit_ratelimits() # GitHub OIDC providers are unique on the tuple of # (repository_name, owner, workflow_name), so we check for # an already registered one before creating. From 7e380c948a18a68a9cad60ec3fd15c793af9fb3c Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 28 Mar 2022 15:42:35 -0400 Subject: [PATCH 64/78] warehouse: `make translations` --- warehouse/locale/messages.pot | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/warehouse/locale/messages.pot b/warehouse/locale/messages.pot index d02c6965e852..02d5b511de7e 100644 --- a/warehouse/locale/messages.pot +++ b/warehouse/locale/messages.pot @@ -243,33 +243,33 @@ msgid "" "again later." msgstr "" -#: warehouse/manage/views.py:1849 +#: warehouse/manage/views.py:1850 msgid "User '${username}' already has ${role_name} role for project" msgstr "" -#: warehouse/manage/views.py:1860 +#: warehouse/manage/views.py:1861 msgid "" "User '${username}' does not have a verified primary email address and " "cannot be added as a ${role_name} for project" msgstr "" -#: warehouse/manage/views.py:1873 +#: warehouse/manage/views.py:1874 msgid "User '${username}' already has an active invite. Please try again later." msgstr "" -#: warehouse/manage/views.py:1931 +#: warehouse/manage/views.py:1932 msgid "Invitation sent to '${username}'" msgstr "" -#: warehouse/manage/views.py:1978 +#: warehouse/manage/views.py:1979 msgid "Could not find role invitation." msgstr "" -#: warehouse/manage/views.py:1989 +#: warehouse/manage/views.py:1990 msgid "Invitation already expired." msgstr "" -#: warehouse/manage/views.py:2013 +#: warehouse/manage/views.py:2014 msgid "Invitation revoked from '${username}'." msgstr "" From 5dafd988d994d3477246ab289db3e088ffc5532f Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 29 Mar 2022 17:24:01 -0400 Subject: [PATCH 65/78] warehouse: plug in more OIDC metrics Adds additional metrics on: * Publisher configuration (attempt + ok) * Publisher removal (attempt + ok) * JWT signature verification (attempt + ok) --- warehouse/manage/views.py | 11 +++++++++++ warehouse/oidc/services.py | 2 ++ 2 files changed, 13 insertions(+) diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index 87d9fd5d93d4..bd1cd2bccfca 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -79,6 +79,7 @@ SaveAccountForm, Toggle2FARequirementForm, ) +from warehouse.metrics.interfaces import IMetricsService from warehouse.oidc.forms import DeleteProviderForm, GitHubProviderForm from warehouse.oidc.interfaces import TooManyOIDCRegistrations from warehouse.oidc.models import GitHubProvider, OIDCProvider @@ -1098,6 +1099,7 @@ def __init__(self, project, request): self.request = request self.project = project self.oidc_enabled = self.request.registry.settings["warehouse.oidc.enabled"] + self.metrics = self.request.find_service(IMetricsService, context=None) @property def _ratelimiters(self): @@ -1165,6 +1167,8 @@ def add_github_oidc_provider(self): if not self.oidc_enabled: raise HTTPNotFound + self.metrics.increment("warehouse.oidc.add_github_provider.attempt") + if self.request.flags.enabled(AdminFlagValue.DISALLOW_OIDC): self.request.session.flash( ( @@ -1178,6 +1182,7 @@ def add_github_oidc_provider(self): try: self._check_ratelimits() except TooManyOIDCRegistrations as exc: + self.metrics.increment("warehouse.oidc.add_github_provider.ratelimited") return HTTPTooManyRequests( self.request._( "There have been too many attempted OpenID Connect registrations. " @@ -1247,6 +1252,8 @@ def add_github_oidc_provider(self): queue="success", ) + self.metrics.increment("warehouse.oidc.add_github_provider.ok") + return response @view_config(request_method="POST", request_param=DeleteProviderForm.__params__) @@ -1254,6 +1261,8 @@ def delete_oidc_provider(self): if not self.oidc_enabled: raise HTTPNotFound + self.metrics.increment("warehouse.oidc.delete_provider.attempt") + if self.request.flags.enabled(AdminFlagValue.DISALLOW_OIDC): self.request.session.flash( ( @@ -1303,6 +1312,8 @@ def delete_oidc_provider(self): f"Removed {provider} from {self.project.name}", queue="success" ) + self.metrics.increment("warehouse.oidc.delete_provider.ok") + return self.default_response diff --git a/warehouse/oidc/services.py b/warehouse/oidc/services.py index 5b5f949b5fbc..fb2405451387 100644 --- a/warehouse/oidc/services.py +++ b/warehouse/oidc/services.py @@ -161,6 +161,7 @@ def _get_key_for_token(self, token): return self.get_key(unverified_header["kid"]) def verify_signature_only(self, token): + self.metrics.increment("warehouse.oidc.verify_signature_only.attempt") key = self._get_key_for_token(token) try: @@ -184,6 +185,7 @@ def verify_signature_only(self, token): audience="pypi", leeway=30, ) + self.metrics.increment("warehouse.oidc.verify_signature_only.ok") return valid_token except jwt.PyJWTError: return None From 0ff3f012ae4a681fd3f0e58add66187b10f13d31 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 29 Mar 2022 17:44:15 -0400 Subject: [PATCH 66/78] warehouse/oidc: add a `verify_for_helper` iface method This encapsulates the entire JWT verification process. It isn't hooked up to anything yet, but just to get something down. --- warehouse/oidc/interfaces.py | 8 ++++++++ warehouse/oidc/services.py | 29 +++++++++++++++++++++++++++-- 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/warehouse/oidc/interfaces.py b/warehouse/oidc/interfaces.py index 4e0171c7fc66..74899faa26af 100644 --- a/warehouse/oidc/interfaces.py +++ b/warehouse/oidc/interfaces.py @@ -38,6 +38,14 @@ def verify_signature_only(token): third party claims must be done to ensure that. """ + def verify_for_project(token, project): + """ + Verify the given JWT's signature and basic claims in the same + manner as `verify_signature_only`, but *also* verify that the JWT's + claims are consistent with at least one of the project's registered + OIDC providers. + """ + class TooManyOIDCRegistrations(RateLimiterException): pass diff --git a/warehouse/oidc/services.py b/warehouse/oidc/services.py index fb2405451387..58e9671cb397 100644 --- a/warehouse/oidc/services.py +++ b/warehouse/oidc/services.py @@ -167,7 +167,7 @@ def verify_signature_only(self, token): try: # NOTE: Many of the keyword arguments here are defaults, but we # set them explicitly to assert the intended verification behavior. - valid_token = jwt.decode( + signed_payload = jwt.decode( token, key=key, algorithms=["RS256"], @@ -186,7 +186,7 @@ def verify_signature_only(self, token): leeway=30, ) self.metrics.increment("warehouse.oidc.verify_signature_only.ok") - return valid_token + return signed_payload except jwt.PyJWTError: return None except Exception as e: @@ -196,6 +196,31 @@ def verify_signature_only(self, token): sentry_sdk.capture_message(f"JWT verify raised generic error: {e}") return None + def verify_for_project(self, token, project): + signed_payload = self.verify_signature_only(token) + + self.metrics.increment("warehouse.oidc.verify_for_project.attempt") + + if signed_payload is None: + self.metrics.increment( + "warehouse.oidc.verify_for_project.invalid_signature" + ) + return False + + # In order for a signed JWT to be valid for a particular PyPI project, + # it must match at least one of the OIDC providers registered to + # the project. + verified = any( + provider.verify_claims(signed_payload) + for provider in project.oidc_providers + ) + if not verified: + self.metrics.increment("warehouse.oidc.verify_for_project.invalid_claims") + else: + self.metrics.increment("warehouse.oidc.verify_for_project.ok") + + return verified + class OIDCProviderServiceFactory: def __init__(self, provider, issuer_url, service_class=OIDCProviderService): From c945ee7d969e04faec99a818f66dade6fb54fb5d Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 29 Mar 2022 17:53:28 -0400 Subject: [PATCH 67/78] manage/views: add provider names to metrics --- warehouse/manage/views.py | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index bd1cd2bccfca..b0a3a62e99a9 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -1167,7 +1167,9 @@ def add_github_oidc_provider(self): if not self.oidc_enabled: raise HTTPNotFound - self.metrics.increment("warehouse.oidc.add_github_provider.attempt") + self.metrics.increment( + "warehouse.oidc.add_provider.attempt", tags=["provider:GitHub"] + ) if self.request.flags.enabled(AdminFlagValue.DISALLOW_OIDC): self.request.session.flash( @@ -1182,7 +1184,9 @@ def add_github_oidc_provider(self): try: self._check_ratelimits() except TooManyOIDCRegistrations as exc: - self.metrics.increment("warehouse.oidc.add_github_provider.ratelimited") + self.metrics.increment( + "warehouse.oidc.add_provider.ratelimited", tags=["provider:GitHub"] + ) return HTTPTooManyRequests( self.request._( "There have been too many attempted OpenID Connect registrations. " @@ -1252,7 +1256,9 @@ def add_github_oidc_provider(self): queue="success", ) - self.metrics.increment("warehouse.oidc.add_github_provider.ok") + self.metrics.increment( + "warehouse.oidc.add_provider.ok", tags=["provider:GitHub"] + ) return response @@ -1312,7 +1318,10 @@ def delete_oidc_provider(self): f"Removed {provider} from {self.project.name}", queue="success" ) - self.metrics.increment("warehouse.oidc.delete_provider.ok") + self.metrics.increment( + "warehouse.oidc.delete_provider.ok", + tags=[f"provider:{provider.provider_name}"], + ) return self.default_response From 32cfd38bb43493cba21b23c133fe2617035baa40 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 29 Mar 2022 17:57:53 -0400 Subject: [PATCH 68/78] oidc/services: add project tag to metrics during JWT verification --- warehouse/oidc/services.py | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/warehouse/oidc/services.py b/warehouse/oidc/services.py index 58e9671cb397..55679b15b3a0 100644 --- a/warehouse/oidc/services.py +++ b/warehouse/oidc/services.py @@ -161,7 +161,6 @@ def _get_key_for_token(self, token): return self.get_key(unverified_header["kid"]) def verify_signature_only(self, token): - self.metrics.increment("warehouse.oidc.verify_signature_only.attempt") key = self._get_key_for_token(token) try: @@ -185,7 +184,6 @@ def verify_signature_only(self, token): audience="pypi", leeway=30, ) - self.metrics.increment("warehouse.oidc.verify_signature_only.ok") return signed_payload except jwt.PyJWTError: return None @@ -199,11 +197,15 @@ def verify_signature_only(self, token): def verify_for_project(self, token, project): signed_payload = self.verify_signature_only(token) - self.metrics.increment("warehouse.oidc.verify_for_project.attempt") + self.metrics.increment( + "warehouse.oidc.verify_for_project.attempt", + tags=[f"project:{project.name}"], + ) if signed_payload is None: self.metrics.increment( - "warehouse.oidc.verify_for_project.invalid_signature" + "warehouse.oidc.verify_for_project.invalid_signature", + tags=[f"project:{project.name}"], ) return False @@ -215,9 +217,15 @@ def verify_for_project(self, token, project): for provider in project.oidc_providers ) if not verified: - self.metrics.increment("warehouse.oidc.verify_for_project.invalid_claims") + self.metrics.increment( + "warehouse.oidc.verify_for_project.invalid_claims", + tags=[f"project:{project.name}"], + ) else: - self.metrics.increment("warehouse.oidc.verify_for_project.ok") + self.metrics.increment( + "warehouse.oidc.verify_for_project.ok", + tags=[f"project:{project.name}"], + ) return verified From 63bce661e202f53ef704b21f353ebdb0d7e47689 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 29 Mar 2022 18:00:44 -0400 Subject: [PATCH 69/78] oidc/services: include provider name in metrics too --- warehouse/oidc/services.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/warehouse/oidc/services.py b/warehouse/oidc/services.py index 55679b15b3a0..9c679ed095ec 100644 --- a/warehouse/oidc/services.py +++ b/warehouse/oidc/services.py @@ -197,15 +197,16 @@ def verify_signature_only(self, token): def verify_for_project(self, token, project): signed_payload = self.verify_signature_only(token) + metrics_tags = [f"project:{project.name}", f"provider:{self.provider}"] self.metrics.increment( "warehouse.oidc.verify_for_project.attempt", - tags=[f"project:{project.name}"], + tags=metrics_tags, ) if signed_payload is None: self.metrics.increment( "warehouse.oidc.verify_for_project.invalid_signature", - tags=[f"project:{project.name}"], + tags=metrics_tags, ) return False @@ -219,12 +220,12 @@ def verify_for_project(self, token, project): if not verified: self.metrics.increment( "warehouse.oidc.verify_for_project.invalid_claims", - tags=[f"project:{project.name}"], + tags=metrics_tags, ) else: self.metrics.increment( "warehouse.oidc.verify_for_project.ok", - tags=[f"project:{project.name}"], + tags=metrics_tags, ) return verified From b677a8df6ea005976b307fe390af602c864f4660 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Wed, 30 Mar 2022 11:07:00 -0400 Subject: [PATCH 70/78] tests/unit: plumb metrics through OIDC unit tests --- tests/unit/manage/test_views.py | 112 ++++++++++++++++++++++++++++++-- 1 file changed, 108 insertions(+), 4 deletions(-) diff --git a/tests/unit/manage/test_views.py b/tests/unit/manage/test_views.py index 88e23c6eb6aa..03568584dcc7 100644 --- a/tests/unit/manage/test_views.py +++ b/tests/unit/manage/test_views.py @@ -42,6 +42,7 @@ from warehouse.forklift.legacy import MAX_FILESIZE, MAX_PROJECT_SIZE from warehouse.macaroons.interfaces import IMacaroonService from warehouse.manage import views +from warehouse.metrics.interfaces import IMetricsService from warehouse.oidc.interfaces import TooManyOIDCRegistrations from warehouse.packaging.models import ( File, @@ -4693,15 +4694,22 @@ def test_raises_404_with_out_of_range_page(self, db_request): class TestManageOIDCProviderViews: def test_initializes(self): + metrics = pretend.stub() project = pretend.stub() request = pretend.stub( - registry=pretend.stub(settings={"warehouse.oidc.enabled": True}) + registry=pretend.stub(settings={"warehouse.oidc.enabled": True}), + find_service=pretend.call_recorder(lambda *a, **kw: metrics), ) view = views.ManageOIDCProviderViews(project, request) assert view.project is project assert view.request is request assert view.oidc_enabled + assert view.metrics is metrics + + assert view.request.find_service.calls == [ + pretend.call(IMetricsService, context=None) + ] @pytest.mark.parametrize( "ip_exceeded, user_exceeded", @@ -4714,6 +4722,7 @@ def test_initializes(self): def test_ratelimiting(self, ip_exceeded, user_exceeded): project = pretend.stub() + metrics = pretend.stub() user_rate_limiter = pretend.stub( hit=pretend.call_recorder(lambda *a, **kw: None), test=pretend.call_recorder(lambda uid: not user_exceeded), @@ -4725,7 +4734,10 @@ def test_ratelimiting(self, ip_exceeded, user_exceeded): resets_in=pretend.call_recorder(lambda uid: pretend.stub()), ) - def find_service(iface, name): + def find_service(iface, name=None, context=None): + if iface is IMetricsService: + return metrics + if name == "user_oidc.provider.register": return user_rate_limiter else: @@ -4745,6 +4757,7 @@ def find_service(iface, name): "ip.oidc": ip_rate_limiter, } assert request.find_service.calls == [ + pretend.call(IMetricsService, context=None), pretend.call(IRateLimiter, name="user_oidc.provider.register"), pretend.call(IRateLimiter, name="ip_oidc.provider.register"), ] @@ -4769,8 +4782,9 @@ def test_manage_project_oidc_providers(self, monkeypatch): settings={ "warehouse.oidc.enabled": True, "github.token": "fake-api-token", - } + }, ), + find_service=lambda *a, **kw: None, flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), POST=pretend.stub(), ) @@ -4802,8 +4816,9 @@ def test_manage_project_oidc_providers_admin_disabled(self, monkeypatch): settings={ "warehouse.oidc.enabled": True, "github.token": "fake-api-token", - } + }, ), + find_service=lambda *a, **kw: None, flags=pretend.stub(enabled=pretend.call_recorder(lambda f: True)), session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), POST=pretend.stub(), @@ -4843,6 +4858,7 @@ def test_manage_project_oidc_providers_oidc_not_enabled(self): project = pretend.stub() request = pretend.stub( registry=pretend.stub(settings={"warehouse.oidc.enabled": False}), + find_service=lambda *a, **kw: None, ) view = views.ManageOIDCProviderViews(project, request) @@ -4868,6 +4884,8 @@ def test_add_github_oidc_provider_preexisting(self, monkeypatch): record_event=pretend.call_recorder(lambda *a, **kw: None), ) + metrics = pretend.stub(increment=pretend.call_recorder(lambda *a, **kw: None)) + request = pretend.stub( registry=pretend.stub( settings={ @@ -4875,6 +4893,7 @@ def test_add_github_oidc_provider_preexisting(self, monkeypatch): "github.token": "fake-api-token", } ), + find_service=lambda *a, **kw: metrics, flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), POST=pretend.stub(), @@ -4914,6 +4933,12 @@ def test_add_github_oidc_provider_preexisting(self, monkeypatch): "project": project, "github_provider_form": github_provider_form_obj, } + assert view.metrics.increment.calls == [ + pretend.call( + "warehouse.oidc.add_provider.attempt", tags=["provider:GitHub"] + ), + pretend.call("warehouse.oidc.add_provider.ok", tags=["provider:GitHub"]), + ] assert project.record_event.calls == [ pretend.call( tag="project:oidc:provider-added", @@ -4945,6 +4970,8 @@ def test_add_github_oidc_provider_created(self, monkeypatch): record_event=pretend.call_recorder(lambda *a, **kw: None), ) + metrics = pretend.stub(increment=pretend.call_recorder(lambda *a, **kw: None)) + request = pretend.stub( registry=pretend.stub( settings={ @@ -4952,6 +4979,7 @@ def test_add_github_oidc_provider_created(self, monkeypatch): "github.token": "fake-api-token", } ), + find_service=lambda *a, **kw: metrics, flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), POST=pretend.stub(), @@ -4998,6 +5026,12 @@ def test_add_github_oidc_provider_created(self, monkeypatch): "project": project, "github_provider_form": github_provider_form_obj, } + assert view.metrics.increment.calls == [ + pretend.call( + "warehouse.oidc.add_provider.attempt", tags=["provider:GitHub"] + ), + pretend.call("warehouse.oidc.add_provider.ok", tags=["provider:GitHub"]), + ] assert project.record_event.calls == [ pretend.call( tag="project:oidc:provider-added", @@ -5040,6 +5074,8 @@ def test_add_github_oidc_provider_already_registered_with_project( # NOTE: Can't set __str__ using pretend.stub() monkeypatch.setattr(provider.__class__, "__str__", lambda s: "fakespecifier") + metrics = pretend.stub(increment=pretend.call_recorder(lambda *a, **kw: None)) + project = pretend.stub( name="fakeproject", oidc_providers=[provider], @@ -5053,6 +5089,7 @@ def test_add_github_oidc_provider_already_registered_with_project( "github.token": "fake-api-token", } ), + find_service=lambda *a, **kw: metrics, flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), POST=pretend.stub(), @@ -5087,6 +5124,11 @@ def test_add_github_oidc_provider_already_registered_with_project( "project": project, "github_provider_form": github_provider_form_obj, } + assert view.metrics.increment.calls == [ + pretend.call( + "warehouse.oidc.add_provider.attempt", tags=["provider:GitHub"] + ), + ] assert project.record_event.calls == [] assert request.session.flash.calls == [ pretend.call( @@ -5097,12 +5139,16 @@ def test_add_github_oidc_provider_already_registered_with_project( def test_add_github_oidc_provider_ratelimited(self, monkeypatch): project = pretend.stub() + + metrics = pretend.stub(increment=pretend.call_recorder(lambda *a, **kw: None)) + request = pretend.stub( registry=pretend.stub( settings={ "warehouse.oidc.enabled": True, } ), + find_service=lambda *a, **kw: metrics, flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), _=lambda s: s, ) @@ -5121,11 +5167,20 @@ def test_add_github_oidc_provider_ratelimited(self, monkeypatch): ) assert view.add_github_oidc_provider().__class__ == HTTPTooManyRequests + assert view.metrics.increment.calls == [ + pretend.call( + "warehouse.oidc.add_provider.attempt", tags=["provider:GitHub"] + ), + pretend.call( + "warehouse.oidc.add_provider.ratelimited", tags=["provider:GitHub"] + ), + ] def test_add_github_oidc_provider_oidc_not_enabled(self): project = pretend.stub() request = pretend.stub( registry=pretend.stub(settings={"warehouse.oidc.enabled": False}), + find_service=lambda *a, **kw: None, ) view = views.ManageOIDCProviderViews(project, request) @@ -5135,8 +5190,10 @@ def test_add_github_oidc_provider_oidc_not_enabled(self): def test_add_github_oidc_provider_admin_disabled(self, monkeypatch): project = pretend.stub() + metrics = pretend.stub(increment=pretend.call_recorder(lambda *a, **kw: None)) request = pretend.stub( registry=pretend.stub(settings={"warehouse.oidc.enabled": True}), + find_service=lambda *a, **kw: metrics, flags=pretend.stub(enabled=pretend.call_recorder(lambda f: True)), session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), _=lambda s: s, @@ -5149,6 +5206,11 @@ def test_add_github_oidc_provider_admin_disabled(self, monkeypatch): ) assert view.add_github_oidc_provider() == default_response + assert view.metrics.increment.calls == [ + pretend.call( + "warehouse.oidc.add_provider.attempt", tags=["provider:GitHub"] + ), + ] assert request.session.flash.calls == [ pretend.call( ( @@ -5161,8 +5223,10 @@ def test_add_github_oidc_provider_admin_disabled(self, monkeypatch): def test_add_github_oidc_provider_invalid_form(self, monkeypatch): project = pretend.stub() + metrics = pretend.stub(increment=pretend.call_recorder(lambda *a, **kw: None)) request = pretend.stub( registry=pretend.stub(settings={"warehouse.oidc.enabled": True}), + find_service=lambda *a, **kw: metrics, flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), _=lambda s: s, @@ -5189,6 +5253,11 @@ def test_add_github_oidc_provider_invalid_form(self, monkeypatch): ) assert view.add_github_oidc_provider() == default_response + assert view.metrics.increment.calls == [ + pretend.call( + "warehouse.oidc.add_provider.attempt", tags=["provider:GitHub"] + ), + ] assert view._hit_ratelimits.calls == [pretend.call()] assert view._check_ratelimits.calls == [pretend.call()] assert github_provider_form_obj.validate.calls == [pretend.call()] @@ -5206,8 +5275,10 @@ def test_delete_oidc_provider(self, monkeypatch): name="fakeproject", record_event=pretend.call_recorder(lambda *a, **kw: None), ) + metrics = pretend.stub(increment=pretend.call_recorder(lambda *a, **kw: None)) request = pretend.stub( registry=pretend.stub(settings={"warehouse.oidc.enabled": True}), + find_service=lambda *a, **kw: metrics, flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), POST=pretend.stub(), @@ -5245,6 +5316,15 @@ def test_delete_oidc_provider(self, monkeypatch): assert view.delete_oidc_provider() == default_response assert provider not in project.oidc_providers + assert view.metrics.increment.calls == [ + pretend.call( + "warehouse.oidc.delete_provider.attempt", + ), + pretend.call( + "warehouse.oidc.delete_provider.ok", tags=["provider:fakeprovider"] + ), + ] + assert project.record_event.calls == [ pretend.call( tag="project:oidc:provider-removed", @@ -5276,8 +5356,10 @@ def test_delete_oidc_provider(self, monkeypatch): def test_delete_oidc_provider_invalid_form(self, monkeypatch): provider = pretend.stub() project = pretend.stub(oidc_providers=[provider]) + metrics = pretend.stub(increment=pretend.call_recorder(lambda *a, **kw: None)) request = pretend.stub( registry=pretend.stub(settings={"warehouse.oidc.enabled": True}), + find_service=lambda *a, **kw: metrics, flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), POST=pretend.stub(), ) @@ -5299,6 +5381,12 @@ def test_delete_oidc_provider_invalid_form(self, monkeypatch): assert view.delete_oidc_provider() == default_response assert len(project.oidc_providers) == 1 + assert view.metrics.increment.calls == [ + pretend.call( + "warehouse.oidc.delete_provider.attempt", + ), + ] + assert delete_provider_form_cls.calls == [pretend.call(request.POST)] assert delete_provider_form_obj.validate.calls == [pretend.call()] @@ -5318,8 +5406,10 @@ def test_delete_oidc_provider_not_found(self, monkeypatch, other_provider): name="fakeproject", record_event=pretend.call_recorder(lambda *a, **kw: None), ) + metrics = pretend.stub(increment=pretend.call_recorder(lambda *a, **kw: None)) request = pretend.stub( registry=pretend.stub(settings={"warehouse.oidc.enabled": True}), + find_service=lambda *a, **kw: metrics, flags=pretend.stub(enabled=pretend.call_recorder(lambda f: False)), session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), POST=pretend.stub(), @@ -5348,6 +5438,12 @@ def test_delete_oidc_provider_not_found(self, monkeypatch, other_provider): assert provider in project.oidc_providers # not deleted assert other_provider not in project.oidc_providers + assert view.metrics.increment.calls == [ + pretend.call( + "warehouse.oidc.delete_provider.attempt", + ), + ] + assert project.record_event.calls == [] assert request.session.flash.calls == [ pretend.call("Invalid publisher for project", queue="error") @@ -5360,6 +5456,7 @@ def test_delete_oidc_provider_oidc_not_enabled(self): project = pretend.stub() request = pretend.stub( registry=pretend.stub(settings={"warehouse.oidc.enabled": False}), + find_service=lambda *a, **kw: None, ) view = views.ManageOIDCProviderViews(project, request) @@ -5369,8 +5466,10 @@ def test_delete_oidc_provider_oidc_not_enabled(self): def test_delete_oidc_provider_admin_disabled(self, monkeypatch): project = pretend.stub() + metrics = pretend.stub(increment=pretend.call_recorder(lambda *a, **kw: None)) request = pretend.stub( registry=pretend.stub(settings={"warehouse.oidc.enabled": True}), + find_service=lambda *a, **kw: metrics, flags=pretend.stub(enabled=pretend.call_recorder(lambda f: True)), session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), ) @@ -5382,6 +5481,11 @@ def test_delete_oidc_provider_admin_disabled(self, monkeypatch): ) assert view.delete_oidc_provider() == default_response + assert view.metrics.increment.calls == [ + pretend.call( + "warehouse.oidc.delete_provider.attempt", + ), + ] assert request.session.flash.calls == [ pretend.call( ( From f9813ad01f568230e2da01ba1e98604533388130 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Wed, 30 Mar 2022 11:29:28 -0400 Subject: [PATCH 71/78] tests/unit: fill in coverage --- tests/unit/oidc/test_services.py | 93 ++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/tests/unit/oidc/test_services.py b/tests/unit/oidc/test_services.py index 6e615a2e7560..46644c85e8f8 100644 --- a/tests/unit/oidc/test_services.py +++ b/tests/unit/oidc/test_services.py @@ -103,6 +103,99 @@ def test_verify_signature_only_fails(self, monkeypatch, exc): assert service.verify_signature_only(token) is None + def test_verify_for_project(self, monkeypatch): + service = services.OIDCProviderService( + provider="fakeprovider", + issuer_url=pretend.stub(), + cache_url=pretend.stub(), + metrics=pretend.stub( + increment=pretend.call_recorder(lambda *a, **kw: None) + ), + ) + + token = pretend.stub() + claims = pretend.stub() + monkeypatch.setattr( + service, "verify_signature_only", pretend.call_recorder(lambda t: claims) + ) + + provider = pretend.stub(verify_claims=pretend.call_recorder(lambda c: True)) + project = pretend.stub(name="fakeproject", oidc_providers=[provider]) + + assert service.verify_for_project(token, project) + assert service.metrics.increment.calls == [ + pretend.call( + "warehouse.oidc.verify_for_project.attempt", + tags=["project:fakeproject", "provider:fakeprovider"], + ), + pretend.call( + "warehouse.oidc.verify_for_project.ok", + tags=["project:fakeproject", "provider:fakeprovider"], + ), + ] + assert service.verify_signature_only.calls == [pretend.call(token)] + assert provider.verify_claims.calls == [pretend.call(claims)] + + def test_verify_for_project_invalid_signature(self, monkeypatch): + service = services.OIDCProviderService( + provider="fakeprovider", + issuer_url=pretend.stub(), + cache_url=pretend.stub(), + metrics=pretend.stub( + increment=pretend.call_recorder(lambda *a, **kw: None) + ), + ) + + token = pretend.stub() + monkeypatch.setattr(service, "verify_signature_only", lambda t: None) + + project = pretend.stub(name="fakeproject") + + assert not service.verify_for_project(token, project) + assert service.metrics.increment.calls == [ + pretend.call( + "warehouse.oidc.verify_for_project.attempt", + tags=["project:fakeproject", "provider:fakeprovider"], + ), + pretend.call( + "warehouse.oidc.verify_for_project.invalid_signature", + tags=["project:fakeproject", "provider:fakeprovider"], + ), + ] + + def test_verify_for_project_invalid_claims(self, monkeypatch): + service = services.OIDCProviderService( + provider="fakeprovider", + issuer_url=pretend.stub(), + cache_url=pretend.stub(), + metrics=pretend.stub( + increment=pretend.call_recorder(lambda *a, **kw: None) + ), + ) + + token = pretend.stub() + claims = pretend.stub() + monkeypatch.setattr( + service, "verify_signature_only", pretend.call_recorder(lambda t: claims) + ) + + provider = pretend.stub(verify_claims=pretend.call_recorder(lambda c: False)) + project = pretend.stub(name="fakeproject", oidc_providers=[provider]) + + assert not service.verify_for_project(token, project) + assert service.metrics.increment.calls == [ + pretend.call( + "warehouse.oidc.verify_for_project.attempt", + tags=["project:fakeproject", "provider:fakeprovider"], + ), + pretend.call( + "warehouse.oidc.verify_for_project.invalid_claims", + tags=["project:fakeproject", "provider:fakeprovider"], + ), + ] + assert service.verify_signature_only.calls == [pretend.call(token)] + assert provider.verify_claims.calls == [pretend.call(claims)] + def test_get_keyset_not_cached(self, monkeypatch, mockredis): service = services.OIDCProviderService( provider="example", From dd955004e8c80a9b19099b0674aefcea00bece42 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Wed, 30 Mar 2022 11:44:08 -0400 Subject: [PATCH 72/78] warehouse: `make translations` --- warehouse/locale/messages.pot | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/warehouse/locale/messages.pot b/warehouse/locale/messages.pot index 02d5b511de7e..2d8d099d9fb7 100644 --- a/warehouse/locale/messages.pot +++ b/warehouse/locale/messages.pot @@ -116,7 +116,7 @@ msgstr "" msgid "Successful WebAuthn assertion" msgstr "" -#: warehouse/accounts/views.py:441 warehouse/manage/views.py:813 +#: warehouse/accounts/views.py:441 warehouse/manage/views.py:814 msgid "Recovery code accepted. The supplied code cannot be used again." msgstr "" @@ -225,51 +225,51 @@ msgstr "" msgid "Banner Preview" msgstr "" -#: warehouse/manage/views.py:244 +#: warehouse/manage/views.py:245 msgid "Email ${email_address} added - check your email for a verification link" msgstr "" -#: warehouse/manage/views.py:761 +#: warehouse/manage/views.py:762 msgid "Recovery codes already generated" msgstr "" -#: warehouse/manage/views.py:762 +#: warehouse/manage/views.py:763 msgid "Generating new recovery codes will invalidate your existing codes." msgstr "" -#: warehouse/manage/views.py:1182 +#: warehouse/manage/views.py:1191 msgid "" "There have been too many attempted OpenID Connect registrations. Try " "again later." msgstr "" -#: warehouse/manage/views.py:1850 +#: warehouse/manage/views.py:1870 msgid "User '${username}' already has ${role_name} role for project" msgstr "" -#: warehouse/manage/views.py:1861 +#: warehouse/manage/views.py:1881 msgid "" "User '${username}' does not have a verified primary email address and " "cannot be added as a ${role_name} for project" msgstr "" -#: warehouse/manage/views.py:1874 +#: warehouse/manage/views.py:1894 msgid "User '${username}' already has an active invite. Please try again later." msgstr "" -#: warehouse/manage/views.py:1932 +#: warehouse/manage/views.py:1952 msgid "Invitation sent to '${username}'" msgstr "" -#: warehouse/manage/views.py:1979 +#: warehouse/manage/views.py:1999 msgid "Could not find role invitation." msgstr "" -#: warehouse/manage/views.py:1990 +#: warehouse/manage/views.py:2010 msgid "Invitation already expired." msgstr "" -#: warehouse/manage/views.py:2014 +#: warehouse/manage/views.py:2034 msgid "Invitation revoked from '${username}'." msgstr "" From 04c7261191b9138e09391d22e9d63bf129713ee2 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 1 Apr 2022 15:15:24 -0400 Subject: [PATCH 73/78] tests, warehouse: disable `job_workflow_ref` For now. --- tests/unit/oidc/test_models.py | 2 +- warehouse/oidc/models.py | 7 ++----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/tests/unit/oidc/test_models.py b/tests/unit/oidc/test_models.py index 6daac4130744..7e97dd30c623 100644 --- a/tests/unit/oidc/test_models.py +++ b/tests/unit/oidc/test_models.py @@ -27,7 +27,6 @@ def test_github_provider_all_known_claims(self): assert models.GitHubProvider.all_known_claims() == { # verifiable claims "repository", - "job_workflow_ref", "workflow", # preverified claims "iss", @@ -48,6 +47,7 @@ def test_github_provider_all_known_claims(self): "base_ref", "event_name", "ref_type", + "job_workflow_ref", } def test_github_provider_computed_properties(self): diff --git a/warehouse/oidc/models.py b/warehouse/oidc/models.py index ccc58a7c4580..5ce99eeb7910 100644 --- a/warehouse/oidc/models.py +++ b/warehouse/oidc/models.py @@ -146,7 +146,6 @@ class GitHubProvider(OIDCProvider): __verifiable_claims__ = { "repository": str.__eq__, - "job_workflow_ref": str.startswith, "workflow": str.__eq__, } @@ -163,6 +162,8 @@ class GitHubProvider(OIDCProvider): "base_ref", "event_name", "ref_type", + # TODO(#11096): Support reusable workflows. + "job_workflow_ref", } @property @@ -173,10 +174,6 @@ def provider_name(self): def repository(self): return f"{self.owner}/{self.repository_name}" - @property - def job_workflow_ref(self): - return f"{self.repository}/.github/workflows/{self.workflow_name}@" - @property def workflow(self): return self.workflow_name From 460955955d5a9af8468c309b46a392967cba7f79 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 5 Apr 2022 09:13:08 -0400 Subject: [PATCH 74/78] Apply suggestions from code review Co-authored-by: Dustin Ingram --- warehouse/config.py | 4 ++-- warehouse/oidc/forms.py | 10 +++++----- .../templates/email/oidc-provider-removed/subject.txt | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/warehouse/config.py b/warehouse/config.py index 6bb686bb0905..2087c1b195b3 100644 --- a/warehouse/config.py +++ b/warehouse/config.py @@ -274,13 +274,13 @@ def configure(settings=None): settings, "warehouse.manage.oidc.user_registration_ratelimit_string", "USER_OIDC_REGISTRATION_RATELIMIT_STRING", - default="10 per day", + default="20 per day", ) maybe_set( settings, "warehouse.manage.oidc.ip_registration_ratelimit_string", "IP_OIDC_REGISTRATION_RATELIMIT_STRING", - default="10 per day", + default="20 per day", ) # 2FA feature flags diff --git a/warehouse/oidc/forms.py b/warehouse/oidc/forms.py index 1ad9a7ef48b1..e79c0ee57d4d 100644 --- a/warehouse/oidc/forms.py +++ b/warehouse/oidc/forms.py @@ -29,22 +29,22 @@ class GitHubProviderForm(forms.Form): owner = wtforms.StringField( validators=[ wtforms.validators.DataRequired( - message=_("Specify GitHub owner (username or organization)"), + message=_("Specify GitHub repository owner (username or organization)"), ), ] ) repository = wtforms.StringField( validators=[ - wtforms.validators.DataRequired(message=_("Specify repository slug")), + wtforms.validators.DataRequired(message=_("Specify repository name")), wtforms.validators.Regexp( _VALID_GITHUB_REPO, message=_("Invalid repository name") ), ] ) - workflow_name = wtforms.StringField( - validators=[wtforms.validators.DataRequired(message=_("Specify workflow name"))] + workflow_filename = wtforms.StringField( + validators=[wtforms.validators.DataRequired(message=_("Specify workflow filename"))] ) def __init__(self, *args, api_token, **kwargs): @@ -130,7 +130,7 @@ def validate_workflow_name(self, field): if "/" in workflow_name: raise wtforms.validators.ValidationError( - _("Workflow name must be a basename, without directories") + _("Workflow filename must be a filename only, without directories") ) diff --git a/warehouse/templates/email/oidc-provider-removed/subject.txt b/warehouse/templates/email/oidc-provider-removed/subject.txt index 688e4af64264..8e76331c6d35 100644 --- a/warehouse/templates/email/oidc-provider-removed/subject.txt +++ b/warehouse/templates/email/oidc-provider-removed/subject.txt @@ -16,6 +16,6 @@ {% block subject %} {% trans project_name=project_name %} -OpenID Connect removed from {{ project_name }} +OpenID Connect publisher removed from {{ project_name }} {% endtrans %} {% endblock %} From 52c4e15255a4131293229a076995335852e389b2 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 5 Apr 2022 10:29:30 -0400 Subject: [PATCH 75/78] tests, warehouse: update tests for changes Also use `workflow_filename` consistently. --- tests/unit/manage/test_views.py | 10 +++---- tests/unit/oidc/test_forms.py | 26 +++++++++---------- tests/unit/oidc/test_models.py | 8 +++--- tests/unit/test_config.py | 4 +-- warehouse/manage/views.py | 6 ++--- ...4c444f_add_initial_oidc_provider_models.py | 7 +++-- warehouse/oidc/forms.py | 16 +++++++----- warehouse/oidc/models.py | 8 +++--- warehouse/templates/manage/publishing.html | 18 ++++--------- 9 files changed, 51 insertions(+), 52 deletions(-) diff --git a/tests/unit/manage/test_views.py b/tests/unit/manage/test_views.py index 03568584dcc7..ca0dd441d983 100644 --- a/tests/unit/manage/test_views.py +++ b/tests/unit/manage/test_views.py @@ -4873,7 +4873,7 @@ def test_add_github_oidc_provider_preexisting(self, monkeypatch): repository_name="fakerepo", owner="fakeowner", owner_id="1234", - workflow_name="fakeworkflow.yml", + workflow_filename="fakeworkflow.yml", ) # NOTE: Can't set __str__ using pretend.stub() monkeypatch.setattr(provider.__class__, "__str__", lambda s: "fakespecifier") @@ -4910,7 +4910,7 @@ def test_add_github_oidc_provider_preexisting(self, monkeypatch): validate=pretend.call_recorder(lambda: True), repository=pretend.stub(data=provider.repository_name), normalized_owner=provider.owner, - workflow_name=pretend.stub(data=provider.workflow_name), + workflow_filename=pretend.stub(data=provider.workflow_filename), ) github_provider_form_cls = pretend.call_recorder( lambda *a, **kw: github_provider_form_obj @@ -4997,7 +4997,7 @@ def test_add_github_oidc_provider_created(self, monkeypatch): repository=pretend.stub(data="fakerepo"), normalized_owner="fakeowner", owner_id="1234", - workflow_name=pretend.stub(data="fakeworkflow.yml"), + workflow_filename=pretend.stub(data="fakeworkflow.yml"), ) github_provider_form_cls = pretend.call_recorder( lambda *a, **kw: github_provider_form_obj @@ -5069,7 +5069,7 @@ def test_add_github_oidc_provider_already_registered_with_project( repository_name="fakerepo", owner="fakeowner", owner_id="1234", - workflow_name="fakeworkflow.yml", + workflow_filename="fakeworkflow.yml", ) # NOTE: Can't set __str__ using pretend.stub() monkeypatch.setattr(provider.__class__, "__str__", lambda s: "fakespecifier") @@ -5104,7 +5104,7 @@ def test_add_github_oidc_provider_already_registered_with_project( validate=pretend.call_recorder(lambda: True), repository=pretend.stub(data=provider.repository_name), normalized_owner=provider.owner, - workflow_name=pretend.stub(data=provider.workflow_name), + workflow_filename=pretend.stub(data=provider.workflow_filename), ) github_provider_form_cls = pretend.call_recorder( lambda *a, **kw: github_provider_form_obj diff --git a/tests/unit/oidc/test_forms.py b/tests/unit/oidc/test_forms.py index 129357f8c984..8558c9543a73 100644 --- a/tests/unit/oidc/test_forms.py +++ b/tests/unit/oidc/test_forms.py @@ -183,22 +183,22 @@ def test_lookup_owner_succeeds(self, monkeypatch): @pytest.mark.parametrize( "data", [ - {"owner": None, "repository": "some", "workflow_name": "some"}, - {"owner": "", "repository": "some", "workflow_name": "some"}, + {"owner": None, "repository": "some", "workflow_filename": "some"}, + {"owner": "", "repository": "some", "workflow_filename": "some"}, { "owner": "invalid_characters@", "repository": "some", - "workflow_name": "some", + "workflow_filename": "some", }, - {"repository": None, "owner": "some", "workflow_name": "some"}, - {"repository": "", "owner": "some", "workflow_name": "some"}, + {"repository": None, "owner": "some", "workflow_filename": "some"}, + {"repository": "", "owner": "some", "workflow_filename": "some"}, { "repository": "$invalid#characters", "owner": "some", - "workflow_name": "some", + "workflow_filename": "some", }, - {"repository": "some", "owner": "some", "workflow_name": None}, - {"repository": "some", "owner": "some", "workflow_name": ""}, + {"repository": "some", "owner": "some", "workflow_filename": None}, + {"repository": "some", "owner": "some", "workflow_filename": ""}, ], ) def test_validate_basic_invalid_fields(self, monkeypatch, data): @@ -215,7 +215,7 @@ def test_validate(self, monkeypatch): { "owner": "some-owner", "repository": "some-repo", - "workflow_name": "some-workflow.yml", + "workflow_filename": "some-workflow.yml", } ) form = forms.GitHubProviderForm(MultiDict(data), api_token=pretend.stub()) @@ -239,11 +239,11 @@ def test_validate_owner(self, monkeypatch): assert form.owner_id == "1234" @pytest.mark.parametrize( - "workflow_name", ["missing_suffix", "/slash", "/many/slashes", "/slash.yml"] + "workflow_filename", ["missing_suffix", "/slash", "/many/slashes", "/slash.yml"] ) - def test_validate_workflow_name(self, workflow_name): + def test_validate_workflow_filename(self, workflow_filename): form = forms.GitHubProviderForm(api_token=pretend.stub()) - field = pretend.stub(data=workflow_name) + field = pretend.stub(data=workflow_filename) with pytest.raises(wtforms.validators.ValidationError): - form.validate_workflow_name(field) + form.validate_workflow_filename(field) diff --git a/tests/unit/oidc/test_models.py b/tests/unit/oidc/test_models.py index 7e97dd30c623..9ffd87bef676 100644 --- a/tests/unit/oidc/test_models.py +++ b/tests/unit/oidc/test_models.py @@ -55,7 +55,7 @@ def test_github_provider_computed_properties(self): repository_name="fakerepo", owner="fakeowner", owner_id="fakeid", - workflow_name="fakeworkflow.yml", + workflow_filename="fakeworkflow.yml", ) for claim_name in provider.__verifiable_claims__.keys(): @@ -68,7 +68,7 @@ def test_github_provider_unaccounted_claims(self, monkeypatch): repository_name="fakerepo", owner="fakeowner", owner_id="fakeid", - workflow_name="fakeworkflow.yml", + workflow_filename="fakeworkflow.yml", ) sentry_sdk = pretend.stub(capture_message=pretend.call_recorder(lambda s: None)) @@ -92,7 +92,7 @@ def test_github_provider_missing_claims(self, monkeypatch): repository_name="fakerepo", owner="fakeowner", owner_id="fakeid", - workflow_name="fakeworkflow.yml", + workflow_filename="fakeworkflow.yml", ) sentry_sdk = pretend.stub(capture_message=pretend.call_recorder(lambda s: None)) @@ -113,7 +113,7 @@ def test_github_provider_verifies(self, monkeypatch): repository_name="fakerepo", owner="fakeowner", owner_id="fakeid", - workflow_name="fakeworkflow.yml", + workflow_filename="fakeworkflow.yml", ) noop_check = pretend.call_recorder(lambda l, r: True) diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py index 9953ac4a1bc2..e194c2becfe6 100644 --- a/tests/unit/test_config.py +++ b/tests/unit/test_config.py @@ -255,8 +255,8 @@ def __init__(self): "warehouse.account.global_login_ratelimit_string": "1000 per 5 minutes", "warehouse.account.email_add_ratelimit_string": "2 per day", "warehouse.account.password_reset_ratelimit_string": "5 per day", - "warehouse.manage.oidc.user_registration_ratelimit_string": "10 per day", - "warehouse.manage.oidc.ip_registration_ratelimit_string": "10 per day", + "warehouse.manage.oidc.user_registration_ratelimit_string": "20 per day", + "warehouse.manage.oidc.ip_registration_ratelimit_string": "20 per day", "warehouse.two_factor_requirement.enabled": False, "warehouse.two_factor_mandate.available": False, "warehouse.two_factor_mandate.enabled": False, diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index b0a3a62e99a9..13adea90258e 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -1202,14 +1202,14 @@ def add_github_oidc_provider(self): if form.validate(): # GitHub OIDC providers are unique on the tuple of - # (repository_name, owner, workflow_name), so we check for + # (repository_name, owner, workflow_filename), so we check for # an already registered one before creating. provider = ( self.request.db.query(GitHubProvider) .filter( GitHubProvider.repository_name == form.repository.data, GitHubProvider.owner == form.normalized_owner, - GitHubProvider.workflow_name == form.workflow_name.data, + GitHubProvider.workflow_filename == form.workflow_filename.data, ) .one_or_none() ) @@ -1218,7 +1218,7 @@ def add_github_oidc_provider(self): repository_name=form.repository.data, owner=form.normalized_owner, owner_id=form.owner_id, - workflow_name=form.workflow_name.data, + workflow_filename=form.workflow_filename.data, ) self.request.db.add(provider) diff --git a/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py b/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py index cb874b05a0cc..07a04c6e7573 100644 --- a/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py +++ b/warehouse/migrations/versions/f345394c444f_add_initial_oidc_provider_models.py @@ -53,14 +53,17 @@ def upgrade(): sa.Column("repository_name", sa.String(), nullable=True), sa.Column("owner", sa.String(), nullable=True), sa.Column("owner_id", sa.String(), nullable=True), - sa.Column("workflow_name", sa.String(), nullable=True), + sa.Column("workflow_filename", sa.String(), nullable=True), sa.ForeignKeyConstraint( ["id"], ["oidc_providers.id"], ), sa.PrimaryKeyConstraint("id"), sa.UniqueConstraint( - "repository_name", "owner", "workflow_name", name="_github_oidc_provider_uc" + "repository_name", + "owner", + "workflow_filename", + name="_github_oidc_provider_uc", ), ) op.create_table( diff --git a/warehouse/oidc/forms.py b/warehouse/oidc/forms.py index e79c0ee57d4d..beb77b76474d 100644 --- a/warehouse/oidc/forms.py +++ b/warehouse/oidc/forms.py @@ -24,7 +24,7 @@ class GitHubProviderForm(forms.Form): - __params__ = ["owner", "repository", "workflow_name"] + __params__ = ["owner", "repository", "workflow_filename"] owner = wtforms.StringField( validators=[ @@ -44,7 +44,9 @@ class GitHubProviderForm(forms.Form): ) workflow_filename = wtforms.StringField( - validators=[wtforms.validators.DataRequired(message=_("Specify workflow filename"))] + validators=[ + wtforms.validators.DataRequired(message=_("Specify workflow filename")) + ] ) def __init__(self, *args, api_token, **kwargs): @@ -120,15 +122,17 @@ def validate_owner(self, field): self.normalized_owner = owner_info["login"] self.owner_id = owner_info["id"] - def validate_workflow_name(self, field): - workflow_name = field.data + def validate_workflow_filename(self, field): + workflow_filename = field.data - if not (workflow_name.endswith(".yml") or workflow_name.endswith(".yaml")): + if not ( + workflow_filename.endswith(".yml") or workflow_filename.endswith(".yaml") + ): raise wtforms.validators.ValidationError( _("Workflow name must end with .yml or .yaml") ) - if "/" in workflow_name: + if "/" in workflow_filename: raise wtforms.validators.ValidationError( _("Workflow filename must be a filename only, without directories") ) diff --git a/warehouse/oidc/models.py b/warehouse/oidc/models.py index 5ce99eeb7910..f2f780671890 100644 --- a/warehouse/oidc/models.py +++ b/warehouse/oidc/models.py @@ -133,7 +133,7 @@ class GitHubProvider(OIDCProvider): UniqueConstraint( "repository_name", "owner", - "workflow_name", + "workflow_filename", name="_github_oidc_provider_uc", ), ) @@ -142,7 +142,7 @@ class GitHubProvider(OIDCProvider): repository_name = Column(String) owner = Column(String) owner_id = Column(String) - workflow_name = Column(String) + workflow_filename = Column(String) __verifiable_claims__ = { "repository": str.__eq__, @@ -176,7 +176,7 @@ def repository(self): @property def workflow(self): - return self.workflow_name + return self.workflow_filename def __str__(self): - return f"{self.workflow_name} @ {self.repository}" + return f"{self.workflow_filename} @ {self.repository}" diff --git a/warehouse/templates/manage/publishing.html b/warehouse/templates/manage/publishing.html index 5187785d6063..5bc91e1f5a30 100644 --- a/warehouse/templates/manage/publishing.html +++ b/warehouse/templates/manage/publishing.html @@ -66,7 +66,6 @@

    {% trans %}OpenID Connect publisher management{% endtrans

    {% trans %}Add a new provider{% endtrans %}

    - {% if oidc_enabled %}

    GitHub

    @@ -104,15 +103,15 @@

    GitHub

  • -
    From 73eef397563377f12b6237e6bbdda01bf48336aa Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 5 Apr 2022 10:36:59 -0400 Subject: [PATCH 76/78] warehouse, tests: email all users on OIDC changes Instead of just owners. --- tests/unit/manage/test_views.py | 28 +++++++++------------------- warehouse/manage/views.py | 8 ++------ 2 files changed, 11 insertions(+), 25 deletions(-) diff --git a/tests/unit/manage/test_views.py b/tests/unit/manage/test_views.py index ca0dd441d983..57298a21a8f3 100644 --- a/tests/unit/manage/test_views.py +++ b/tests/unit/manage/test_views.py @@ -4882,6 +4882,7 @@ def test_add_github_oidc_provider_preexisting(self, monkeypatch): name="fakeproject", oidc_providers=[], record_event=pretend.call_recorder(lambda *a, **kw: None), + users=[], ) metrics = pretend.stub(increment=pretend.call_recorder(lambda *a, **kw: None)) @@ -4916,9 +4917,6 @@ def test_add_github_oidc_provider_preexisting(self, monkeypatch): lambda *a, **kw: github_provider_form_obj ) monkeypatch.setattr(views, "GitHubProviderForm", github_provider_form_cls) - monkeypatch.setattr( - views, "project_owners", pretend.call_recorder(lambda *a: []) - ) view = views.ManageOIDCProviderViews(project, request) monkeypatch.setattr( @@ -4958,16 +4956,17 @@ def test_add_github_oidc_provider_preexisting(self, monkeypatch): ] assert request.db.add.calls == [] assert github_provider_form_obj.validate.calls == [pretend.call()] - assert views.project_owners.calls == [pretend.call(request, project)] assert view._hit_ratelimits.calls == [pretend.call()] assert view._check_ratelimits.calls == [pretend.call()] assert project.oidc_providers == [provider] def test_add_github_oidc_provider_created(self, monkeypatch): + fakeusers = [pretend.stub(), pretend.stub(), pretend.stub()] project = pretend.stub( name="fakeproject", oidc_providers=[], record_event=pretend.call_recorder(lambda *a, **kw: None), + users=fakeusers, ) metrics = pretend.stub(increment=pretend.call_recorder(lambda *a, **kw: None)) @@ -5003,10 +5002,6 @@ def test_add_github_oidc_provider_created(self, monkeypatch): lambda *a, **kw: github_provider_form_obj ) monkeypatch.setattr(views, "GitHubProviderForm", github_provider_form_cls) - fakeowners = [pretend.stub(), pretend.stub(), pretend.stub()] - monkeypatch.setattr( - views, "project_owners", pretend.call_recorder(lambda *a: fakeowners) - ) monkeypatch.setattr( views, "send_oidc_provider_added_email", @@ -5051,10 +5046,9 @@ def test_add_github_oidc_provider_created(self, monkeypatch): ] assert request.db.add.calls == [pretend.call(project.oidc_providers[0])] assert github_provider_form_obj.validate.calls == [pretend.call()] - assert views.project_owners.calls == [pretend.call(request, project)] assert views.send_oidc_provider_added_email.calls == [ - pretend.call(request, fakeowner, project_name="fakeproject") - for fakeowner in fakeowners + pretend.call(request, fakeuser, project_name="fakeproject") + for fakeuser in fakeusers ] assert view._hit_ratelimits.calls == [pretend.call()] assert view._check_ratelimits.calls == [pretend.call()] @@ -5270,10 +5264,12 @@ def test_delete_oidc_provider(self, monkeypatch): # NOTE: Can't set __str__ using pretend.stub() monkeypatch.setattr(provider.__class__, "__str__", lambda s: "fakespecifier") + fakeusers = [pretend.stub(), pretend.stub(), pretend.stub()] project = pretend.stub( oidc_providers=[provider], name="fakeproject", record_event=pretend.call_recorder(lambda *a, **kw: None), + users=fakeusers, ) metrics = pretend.stub(increment=pretend.call_recorder(lambda *a, **kw: None)) request = pretend.stub( @@ -5296,11 +5292,6 @@ def test_delete_oidc_provider(self, monkeypatch): lambda *a, **kw: delete_provider_form_obj ) monkeypatch.setattr(views, "DeleteProviderForm", delete_provider_form_cls) - - fakeowners = [pretend.stub(), pretend.stub(), pretend.stub()] - monkeypatch.setattr( - views, "project_owners", pretend.call_recorder(lambda *a: fakeowners) - ) monkeypatch.setattr( views, "send_oidc_provider_removed_email", @@ -5347,10 +5338,9 @@ def test_delete_oidc_provider(self, monkeypatch): assert delete_provider_form_cls.calls == [pretend.call(request.POST)] assert delete_provider_form_obj.validate.calls == [pretend.call()] - assert views.project_owners.calls == [pretend.call(request, project)] assert views.send_oidc_provider_removed_email.calls == [ - pretend.call(request, fakeowner, project_name="fakeproject") - for fakeowner in fakeowners + pretend.call(request, fakeuser, project_name="fakeproject") + for fakeuser in fakeusers ] def test_delete_oidc_provider_invalid_form(self, monkeypatch): diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index 13adea90258e..85d7bb4c32a9 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -1232,9 +1232,7 @@ def add_github_oidc_provider(self): ) return response - # We only email project owners, since only owners can manage OIDC providers. - owner_users = project_owners(self.request, self.project) - for user in owner_users: + for user in self.project.users: send_oidc_provider_added_email( self.request, user, project_name=self.project.name ) @@ -1292,9 +1290,7 @@ def delete_oidc_provider(self): ) return self.default_response - # We only email project owners, since only owners can manage OIDC providers. - owner_users = project_owners(self.request, self.project) - for user in owner_users: + for user in self.project.users: send_oidc_provider_removed_email( self.request, user, project_name=self.project.name ) From 4651ce87cf98946211196b3fa339fad0777ad4ff Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 5 Apr 2022 11:19:04 -0400 Subject: [PATCH 77/78] warehouse, tests: include publisher info in OIDC emails --- tests/unit/email/test_init.py | 16 ++++++++++++++-- tests/unit/manage/test_views.py | 11 +++++++++-- warehouse/email/__init__.py | 18 ++++++++++++++---- warehouse/manage/views.py | 10 ++++++++-- .../email/oidc-provider-added/body.html | 8 ++++++++ .../email/oidc-provider-added/body.txt | 5 +++++ .../email/oidc-provider-removed/body.html | 8 ++++++++ .../email/oidc-provider-removed/body.txt | 5 +++++ 8 files changed, 71 insertions(+), 10 deletions(-) diff --git a/tests/unit/email/test_init.py b/tests/unit/email/test_init.py index d2ab08e5f37e..5ae27303bb8e 100644 --- a/tests/unit/email/test_init.py +++ b/tests/unit/email/test_init.py @@ -3625,10 +3625,22 @@ def test_oidc_provider_emails( pyramid_request.registry.settings = {"mail.sender": "noreply@example.com"} project_name = "test_project" + fakeprovider = pretend.stub(provider_name="fakeprovider") + # NOTE: Can't set __str__ using pretend.stub() + monkeypatch.setattr( + fakeprovider.__class__, "__str__", lambda s: "fakespecifier" + ) - result = fn(pyramid_request, stub_user, project_name=project_name) + result = fn( + pyramid_request, stub_user, project_name=project_name, provider=fakeprovider + ) - assert result == {"username": stub_user.username, "project_name": project_name} + assert result == { + "username": stub_user.username, + "project_name": project_name, + "provider_name": "fakeprovider", + "provider_spec": "fakespecifier", + } subject_renderer.assert_() body_renderer.assert_(username=stub_user.username, project_name=project_name) html_renderer.assert_(username=stub_user.username, project_name=project_name) diff --git a/tests/unit/manage/test_views.py b/tests/unit/manage/test_views.py index 57298a21a8f3..4b9483c25a01 100644 --- a/tests/unit/manage/test_views.py +++ b/tests/unit/manage/test_views.py @@ -5047,7 +5047,12 @@ def test_add_github_oidc_provider_created(self, monkeypatch): assert request.db.add.calls == [pretend.call(project.oidc_providers[0])] assert github_provider_form_obj.validate.calls == [pretend.call()] assert views.send_oidc_provider_added_email.calls == [ - pretend.call(request, fakeuser, project_name="fakeproject") + pretend.call( + request, + fakeuser, + project_name="fakeproject", + provider=project.oidc_providers[0], + ) for fakeuser in fakeusers ] assert view._hit_ratelimits.calls == [pretend.call()] @@ -5339,7 +5344,9 @@ def test_delete_oidc_provider(self, monkeypatch): assert delete_provider_form_obj.validate.calls == [pretend.call()] assert views.send_oidc_provider_removed_email.calls == [ - pretend.call(request, fakeuser, project_name="fakeproject") + pretend.call( + request, fakeuser, project_name="fakeproject", provider=provider + ) for fakeuser in fakeusers ] diff --git a/warehouse/email/__init__.py b/warehouse/email/__init__.py index 49236b5ae153..752dc4392887 100644 --- a/warehouse/email/__init__.py +++ b/warehouse/email/__init__.py @@ -467,15 +467,25 @@ def send_recovery_code_reminder_email(request, user): @_email("oidc-provider-added") -def send_oidc_provider_added_email(request, user, project_name): +def send_oidc_provider_added_email(request, user, project_name, provider): # We use the request's user, since they're the one triggering the action. - return {"username": request.user.username, "project_name": project_name} + return { + "username": request.user.username, + "project_name": project_name, + "provider_name": provider.provider_name, + "provider_spec": str(provider), + } @_email("oidc-provider-removed") -def send_oidc_provider_removed_email(request, user, project_name): +def send_oidc_provider_removed_email(request, user, project_name, provider): # We use the request's user, since they're the one triggering the action. - return {"username": request.user.username, "project_name": project_name} + return { + "username": request.user.username, + "project_name": project_name, + "provider_name": provider.provider_name, + "provider_spec": str(provider), + } def includeme(config): diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index 85d7bb4c32a9..0adc541fb6f0 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -1234,7 +1234,10 @@ def add_github_oidc_provider(self): for user in self.project.users: send_oidc_provider_added_email( - self.request, user, project_name=self.project.name + self.request, + user, + project_name=self.project.name, + provider=provider, ) self.project.oidc_providers.append(provider) @@ -1292,7 +1295,10 @@ def delete_oidc_provider(self): for user in self.project.users: send_oidc_provider_removed_email( - self.request, user, project_name=self.project.name + self.request, + user, + project_name=self.project.name, + provider=provider, ) # NOTE: We remove the provider from the project, but we don't actually diff --git a/warehouse/templates/email/oidc-provider-added/body.html b/warehouse/templates/email/oidc-provider-added/body.html index b873f860abd9..01544af9ce06 100644 --- a/warehouse/templates/email/oidc-provider-added/body.html +++ b/warehouse/templates/email/oidc-provider-added/body.html @@ -24,6 +24,14 @@ {% endtrans %}

    +

    + {% trans %}Publisher information{% endtrans %}: +

      +
    • {% trans %}Publisher name{% endtrans %}: {{ provider_name }}
    • +
    • {% trans %}Publisher specification{% endtrans %}: {{ provider_spec }}
    • +
    +

    +

    {% trans %} If you did not make this change and you think it was made maliciously, you can diff --git a/warehouse/templates/email/oidc-provider-added/body.txt b/warehouse/templates/email/oidc-provider-added/body.txt index e1e3f23c64ee..bdf0c8ac4b0e 100644 --- a/warehouse/templates/email/oidc-provider-added/body.txt +++ b/warehouse/templates/email/oidc-provider-added/body.txt @@ -20,6 +20,11 @@ PyPI user {{ username }} has added a new OpenID Connect publisher to a project users and can create project releases automatically. {% endtrans %} +{% trans %}Publisher information{% endtrans %}: + +* {% trans %}Publisher name{% endtrans %}: {{ provider_name }} +* {% trans %}Publisher specification{% endtrans %}: {{ provider_spec }} + {% trans %} If you did not make this change and you think it was made maliciously, you can remove it from the project via the "Publishing" tab on the project's page. diff --git a/warehouse/templates/email/oidc-provider-removed/body.html b/warehouse/templates/email/oidc-provider-removed/body.html index e33044d5cc2a..d90cedcb152e 100644 --- a/warehouse/templates/email/oidc-provider-removed/body.html +++ b/warehouse/templates/email/oidc-provider-removed/body.html @@ -22,6 +22,14 @@ {% endtrans %}

    +

    + {% trans %}Publisher information{% endtrans %}: +

      +
    • {% trans %}Publisher name{% endtrans %}: {{ provider_name }}
    • +
    • {% trans %}Publisher specification{% endtrans %}: {{ provider_spec }}
    • +
    +

    +

    {% trans %} If you did not make this change and you think it was made maliciously, you can diff --git a/warehouse/templates/email/oidc-provider-removed/body.txt b/warehouse/templates/email/oidc-provider-removed/body.txt index e586473788f2..93e1606824c0 100644 --- a/warehouse/templates/email/oidc-provider-removed/body.txt +++ b/warehouse/templates/email/oidc-provider-removed/body.txt @@ -19,6 +19,11 @@ PyPI user {{ username }} has removed an OpenID Connect publisher from a project ({{ project_name }}) that you manage. {% endtrans %} +{% trans %}Publisher information{% endtrans %}: + +* {% trans %}Publisher name{% endtrans %}: {{ provider_name }} +* {% trans %}Publisher specification{% endtrans %}: {{ provider_spec }} + {% trans %} If you did not make this change and you think it was made maliciously, you can check the "Security history" tab on the project's page. From e090ad347abc05697788ec416182333dfe332313 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 5 Apr 2022 11:44:37 -0400 Subject: [PATCH 78/78] warehouse: `make translations` --- warehouse/locale/messages.pot | 100 ++++++++++++++++++---------------- 1 file changed, 54 insertions(+), 46 deletions(-) diff --git a/warehouse/locale/messages.pot b/warehouse/locale/messages.pot index 845fa4d13d89..06b3b0ff8733 100644 --- a/warehouse/locale/messages.pot +++ b/warehouse/locale/messages.pot @@ -243,81 +243,81 @@ msgid "" "again later." msgstr "" -#: warehouse/manage/views.py:1870 +#: warehouse/manage/views.py:1872 msgid "User '${username}' already has ${role_name} role for project" msgstr "" -#: warehouse/manage/views.py:1881 +#: warehouse/manage/views.py:1883 msgid "" "User '${username}' does not have a verified primary email address and " "cannot be added as a ${role_name} for project" msgstr "" -#: warehouse/manage/views.py:1894 +#: warehouse/manage/views.py:1896 msgid "User '${username}' already has an active invite. Please try again later." msgstr "" -#: warehouse/manage/views.py:1952 +#: warehouse/manage/views.py:1954 msgid "Invitation sent to '${username}'" msgstr "" -#: warehouse/manage/views.py:1999 +#: warehouse/manage/views.py:2001 msgid "Could not find role invitation." msgstr "" -#: warehouse/manage/views.py:2010 +#: warehouse/manage/views.py:2012 msgid "Invitation already expired." msgstr "" -#: warehouse/manage/views.py:2034 +#: warehouse/manage/views.py:2036 msgid "Invitation revoked from '${username}'." msgstr "" #: warehouse/oidc/forms.py:32 -msgid "Specify GitHub owner (username or organization)" +msgid "Specify GitHub repository owner (username or organization)" msgstr "" #: warehouse/oidc/forms.py:39 -msgid "Specify repository slug" +msgid "Specify repository name" msgstr "" #: warehouse/oidc/forms.py:41 msgid "Invalid repository name" msgstr "" -#: warehouse/oidc/forms.py:47 -msgid "Specify workflow name" +#: warehouse/oidc/forms.py:48 +msgid "Specify workflow filename" msgstr "" -#: warehouse/oidc/forms.py:75 +#: warehouse/oidc/forms.py:77 msgid "Unknown GitHub user or organization." msgstr "" -#: warehouse/oidc/forms.py:85 +#: warehouse/oidc/forms.py:87 msgid "GitHub has rate-limited this action. Try again in a few minutes." msgstr "" -#: warehouse/oidc/forms.py:95 +#: warehouse/oidc/forms.py:97 msgid "Unexpected error from GitHub. Try again." msgstr "" -#: warehouse/oidc/forms.py:102 +#: warehouse/oidc/forms.py:104 msgid "Unexpected timeout from GitHub. Try again in a few minutes." msgstr "" -#: warehouse/oidc/forms.py:114 +#: warehouse/oidc/forms.py:116 msgid "Invalid GitHub user or organization name." msgstr "" -#: warehouse/oidc/forms.py:128 +#: warehouse/oidc/forms.py:132 msgid "Workflow name must end with .yml or .yaml" msgstr "" -#: warehouse/oidc/forms.py:133 -msgid "Workflow name must be a basename, without directories" +#: warehouse/oidc/forms.py:137 +msgid "Workflow filename must be a filename only, without directories" msgstr "" -#: warehouse/oidc/forms.py:142 +#: warehouse/oidc/forms.py:146 msgid "Provider must be specified by ID" msgstr "" @@ -896,9 +896,9 @@ msgstr "" #: warehouse/templates/manage/account/recovery_codes-burn.html:70 #: warehouse/templates/manage/account/totp-provision.html:69 #: warehouse/templates/manage/account/webauthn-provision.html:44 -#: warehouse/templates/manage/publishing.html:86 -#: warehouse/templates/manage/publishing.html:98 -#: warehouse/templates/manage/publishing.html:110 +#: warehouse/templates/manage/publishing.html:85 +#: warehouse/templates/manage/publishing.html:97 +#: warehouse/templates/manage/publishing.html:109 #: warehouse/templates/manage/roles.html:170 #: warehouse/templates/manage/roles.html:182 #: warehouse/templates/manage/token.html:136 @@ -1343,6 +1343,21 @@ msgid "" msgstr "" #: warehouse/templates/email/oidc-provider-added/body.html:28 +#: warehouse/templates/email/oidc-provider-removed/body.html:26 +msgid "Publisher information" +msgstr "" + +#: warehouse/templates/email/oidc-provider-added/body.html:30 +#: warehouse/templates/email/oidc-provider-removed/body.html:28 +msgid "Publisher name" +msgstr "" + +#: warehouse/templates/email/oidc-provider-added/body.html:31 +#: warehouse/templates/email/oidc-provider-removed/body.html:29 +msgid "Publisher specification" +msgstr "" + +#: warehouse/templates/email/oidc-provider-added/body.html:36 msgid "" "\n" " If you did not make this change and you think it was made maliciously, " @@ -1352,8 +1367,8 @@ msgid "" " " msgstr "" -#: warehouse/templates/email/oidc-provider-added/body.html:35 -#: warehouse/templates/email/oidc-provider-removed/body.html:33 +#: warehouse/templates/email/oidc-provider-added/body.html:43 +#: warehouse/templates/email/oidc-provider-removed/body.html:41 #, python-format msgid "" "\n" @@ -1375,7 +1390,7 @@ msgid "" " " msgstr "" -#: warehouse/templates/email/oidc-provider-removed/body.html:26 +#: warehouse/templates/email/oidc-provider-removed/body.html:34 msgid "" "\n" " If you did not make this change and you think it was made maliciously, " @@ -2994,72 +3009,65 @@ msgstr "" msgid "Add a new provider" msgstr "" -#: warehouse/templates/manage/publishing.html:73 +#: warehouse/templates/manage/publishing.html:72 #, python-format msgid "" "Read more about GitHub's OpenID Connect provider here." msgstr "" -#: warehouse/templates/manage/publishing.html:84 +#: warehouse/templates/manage/publishing.html:83 #: warehouse/templates/manage/roles.html:43 #: warehouse/templates/manage/roles.html:77 #: warehouse/templates/manage/roles.html:88 msgid "Owner" msgstr "" -#: warehouse/templates/manage/publishing.html:89 +#: warehouse/templates/manage/publishing.html:88 msgid "owner" msgstr "" -#: warehouse/templates/manage/publishing.html:96 +#: warehouse/templates/manage/publishing.html:95 msgid "Repository name" msgstr "" -#: warehouse/templates/manage/publishing.html:101 +#: warehouse/templates/manage/publishing.html:100 msgid "repository" msgstr "" -#: warehouse/templates/manage/publishing.html:108 +#: warehouse/templates/manage/publishing.html:107 msgid "Workflow name" msgstr "" -#: warehouse/templates/manage/publishing.html:113 +#: warehouse/templates/manage/publishing.html:112 msgid "workflow.yml" msgstr "" -#: warehouse/templates/manage/publishing.html:119 +#: warehouse/templates/manage/publishing.html:118 msgid "Add" msgstr "" -#: warehouse/templates/manage/publishing.html:123 +#: warehouse/templates/manage/publishing.html:122 msgid "Manage current providers" msgstr "" -#: warehouse/templates/manage/publishing.html:127 +#: warehouse/templates/manage/publishing.html:126 #, python-format msgid "OpenID Connect publishers associated with %(project_name)s" msgstr "" -#: warehouse/templates/manage/publishing.html:131 +#: warehouse/templates/manage/publishing.html:130 msgid "Publisher" msgstr "" -#: warehouse/templates/manage/publishing.html:132 +#: warehouse/templates/manage/publishing.html:131 msgid "Specification" msgstr "" -#: warehouse/templates/manage/publishing.html:143 +#: warehouse/templates/manage/publishing.html:142 msgid "No publishers are currently configured." msgstr "" -#: warehouse/templates/manage/publishing.html:147 -msgid "" -"\n" -" OpenID Connect publishing isn't enabled yet! Come back soon.\n" -" " -msgstr "" - #: warehouse/templates/manage/release.html:18 #, python-format msgid "Manage '%(project_name)s' – release version %(version)s"