From fd9c7c996287c570812e0e61b2f39d6ec044664b Mon Sep 17 00:00:00 2001 From: Mike Fiedler Date: Thu, 7 Sep 2023 18:53:37 -0400 Subject: [PATCH 1/3] feat: require 2fa for new user to upload as well Expand the policy to include file upload actions. Follows #14294 Refs #13762 Signed-off-by: Mike Fiedler --- tests/unit/accounts/test_security_policy.py | 17 +++++++++++++++++ warehouse/accounts/security_policy.py | 3 ++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/tests/unit/accounts/test_security_policy.py b/tests/unit/accounts/test_security_policy.py index 32e778dee7dd..90c5ac464719 100644 --- a/tests/unit/accounts/test_security_policy.py +++ b/tests/unit/accounts/test_security_policy.py @@ -776,6 +776,23 @@ def test_deny_manage_projects_without_2fa(self, monkeypatch, policy_class): policy = policy_class() assert not policy.permits(request, context, "myperm") + def test_deny_forklift_file_upload_without_2fa(self, monkeypatch, policy_class): + monkeypatch.setattr(security_policy, "User", pretend.stub) + + request = pretend.stub( + identity=pretend.stub( + __principals__=lambda: ["user:5"], + has_primary_verified_email=True, + has_two_factor=False, + date_joined=datetime(2023, 8, 9), + ), + matched_route=pretend.stub(name="forklift.legacy.file_upload"), + ) + context = pretend.stub(__acl__=[(Allow, "user:5", "myperm")]) + + policy = policy_class() + assert not policy.permits(request, context, "myperm") + @pytest.mark.parametrize( "matched_route", [ diff --git a/warehouse/accounts/security_policy.py b/warehouse/accounts/security_policy.py index 5717a767a576..55d119c7aa83 100644 --- a/warehouse/accounts/security_policy.py +++ b/warehouse/accounts/security_policy.py @@ -332,7 +332,8 @@ def _check_for_mfa(request, context) -> WarehouseDenied | None: ] if ( - request.matched_route.name.startswith("manage") + request.matched_route.name == "forklift.legacy.file_upload" + or request.matched_route.name.startswith("manage") and request.matched_route.name != "manage.account" and not any( request.matched_route.name.startswith(route) for route in _exempt_routes From 4d1c36a3464110f341ca6ef0f9af0007b7e7984c Mon Sep 17 00:00:00 2001 From: Ee Durbin Date: Fri, 8 Sep 2023 09:03:13 -0400 Subject: [PATCH 2/3] refactor #14505 to improve error message on upload --- tests/unit/accounts/test_security_policy.py | 5 +++ warehouse/accounts/security_policy.py | 35 ++++++++++++--------- 2 files changed, 26 insertions(+), 14 deletions(-) diff --git a/tests/unit/accounts/test_security_policy.py b/tests/unit/accounts/test_security_policy.py index 90c5ac464719..8dc8f2694907 100644 --- a/tests/unit/accounts/test_security_policy.py +++ b/tests/unit/accounts/test_security_policy.py @@ -575,6 +575,7 @@ def test_acl(self, monkeypatch, policy_class, principals, expected): __principals__=lambda: principals, has_primary_verified_email=True, has_two_factor=False, + date_joined=datetime(2022, 8, 1), ), matched_route=pretend.stub(name="random.route"), ) @@ -603,6 +604,7 @@ def test_2fa_owner_requires( __principals__=lambda: ["user:5"], has_primary_verified_email=True, has_two_factor=has_mfa, + date_joined=datetime(2022, 8, 1), ), matched_route=pretend.stub(name="random.route"), registry=pretend.stub( @@ -640,6 +642,7 @@ def test_2fa_pypi_mandates_2fa( __principals__=lambda: ["user:5"], has_primary_verified_email=True, has_two_factor=has_mfa, + date_joined=datetime(2022, 8, 1), ), matched_route=pretend.stub(name="random.route"), registry=pretend.stub( @@ -677,6 +680,7 @@ def test_2fa_pypi_mandates_2fa_with_warning( __principals__=lambda: ["user:5"], has_primary_verified_email=True, has_two_factor=has_mfa, + date_joined=datetime(2022, 8, 1), ), matched_route=pretend.stub(name="random.route"), registry=pretend.stub( @@ -751,6 +755,7 @@ def test_permits_manage_projects_with_2fa(self, monkeypatch, policy_class): __principals__=lambda: ["user:5"], has_primary_verified_email=True, has_two_factor=True, + date_joined=datetime(2022, 8, 1), ), matched_route=pretend.stub(name="manage.projects"), ) diff --git a/warehouse/accounts/security_policy.py b/warehouse/accounts/security_policy.py index 55d119c7aa83..e72198d9b749 100644 --- a/warehouse/accounts/security_policy.py +++ b/warehouse/accounts/security_policy.py @@ -332,22 +332,29 @@ def _check_for_mfa(request, context) -> WarehouseDenied | None: ] if ( - request.matched_route.name == "forklift.legacy.file_upload" - or request.matched_route.name.startswith("manage") - and request.matched_route.name != "manage.account" - and not any( - request.matched_route.name.startswith(route) for route in _exempt_routes - ) - and not request.identity.has_two_factor - ) and ( - # Start enforcement from 2023-08-08, but we should remove this check - # at the end of 2023. request.identity.date_joined and request.identity.date_joined > datetime.datetime(2023, 8, 8) ): - return WarehouseDenied( - "You must enable two factor authentication to manage other settings", - reason="manage_2fa_required", - ) + if ( + request.matched_route.name.startswith("manage") + and request.matched_route.name != "manage.account" + and not any( + request.matched_route.name.startswith(route) for route in _exempt_routes + ) + and not request.identity.has_two_factor + ): + return WarehouseDenied( + "You must enable two factor authentication to manage other settings", + reason="manage_2fa_required", + ) + + if ( + request.matched_route.name == "forklift.legacy.file_upload" + and not request.identity.has_two_factor + ): + return WarehouseDenied( + "You must enable two factor authentication to upload", + reason="upload_2fa_required", + ) return None From 8e45163457fd8c6e91ac35b2481100f175ba5981 Mon Sep 17 00:00:00 2001 From: Ee Durbin Date: Fri, 8 Sep 2023 09:18:22 -0400 Subject: [PATCH 3/3] fix comments --- warehouse/accounts/security_policy.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/warehouse/accounts/security_policy.py b/warehouse/accounts/security_policy.py index e72198d9b749..90ab04471133 100644 --- a/warehouse/accounts/security_policy.py +++ b/warehouse/accounts/security_policy.py @@ -320,8 +320,9 @@ def _check_for_mfa(request, context) -> WarehouseDenied | None: queue="warning", ) - # Regardless of TwoFactorRequireable, if we're in the manage namespace, we'll - # check if the user has 2FA enabled, and if they don't we'll deny them. + # Regardless of TwoFactorRequireable, if we're in the manage namespace or file + # uploads, we'll check if the user has 2FA enabled, and if they don't we'll deny + # them. # Management routes that don't require 2FA, mostly to set up 2FA. _exempt_routes = [ @@ -331,6 +332,8 @@ def _check_for_mfa(request, context) -> WarehouseDenied | None: "manage.account.webauthn-provision", ] + # Start enforcement from 2023-08-08, but we should remove this check + # at the end of 2023. if ( request.identity.date_joined and request.identity.date_joined > datetime.datetime(2023, 8, 8)