diff --git a/tests/unit/accounts/test_security_policy.py b/tests/unit/accounts/test_security_policy.py index ccfcbea037f8..c087296bd0a5 100644 --- a/tests/unit/accounts/test_security_policy.py +++ b/tests/unit/accounts/test_security_policy.py @@ -10,6 +10,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +from datetime import datetime import pretend import pytest @@ -502,20 +503,6 @@ def test_identity_ip_banned(self, monkeypatch): assert add_vary_cb.calls == [pretend.call("Cookie")] assert request.add_response_callback.calls == [pretend.call(vary_cb)] - def test_permits_with_unverified_email(self, monkeypatch): - monkeypatch.setattr(security_policy, "User", pretend.stub) - - request = pretend.stub( - identity=pretend.stub( - __principals__=lambda: ["user:5"], has_primary_verified_email=False - ), - matched_route=pretend.stub(name="manage.projects"), - ) - context = pretend.stub(__acl__=[(Allow, "user:5", "myperm")]) - - policy = security_policy.SessionSecurityPolicy() - assert not policy.permits(request, context, "myperm") - @pytest.mark.parametrize( "policy_class", @@ -530,8 +517,11 @@ def test_acl(self, monkeypatch, policy_class, principals, expected): request = pretend.stub( identity=pretend.stub( - __principals__=lambda: principals, has_primary_verified_email=True - ) + __principals__=lambda: principals, + has_primary_verified_email=True, + has_two_factor=False, + ), + matched_route=pretend.stub(name="random.route"), ) context = pretend.stub(__acl__=[(Allow, "user:5", "myperm")]) @@ -559,6 +549,7 @@ def test_2fa_owner_requires( has_primary_verified_email=True, has_two_factor=has_mfa, ), + matched_route=pretend.stub(name="random.route"), registry=pretend.stub( settings={ "warehouse.two_factor_requirement.enabled": True, @@ -595,6 +586,7 @@ def test_2fa_pypi_mandates_2fa( has_primary_verified_email=True, has_two_factor=has_mfa, ), + matched_route=pretend.stub(name="random.route"), registry=pretend.stub( settings={ "warehouse.two_factor_requirement.enabled": False, @@ -631,6 +623,7 @@ def test_2fa_pypi_mandates_2fa_with_warning( has_primary_verified_email=True, has_two_factor=has_mfa, ), + matched_route=pretend.stub(name="random.route"), registry=pretend.stub( settings={ "warehouse.two_factor_requirement.enabled": False, @@ -658,3 +651,103 @@ def test_2fa_pypi_mandates_2fa_with_warning( ] else: assert request.session.flash.calls == [] + + def test_permits_with_unverified_email(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=False, + has_two_factor=False, + ), + matched_route=pretend.stub(name="manage.projects"), + ) + context = pretend.stub(__acl__=[(Allow, "user:5", "myperm")]) + + policy = policy_class() + assert not policy.permits(request, context, "myperm") + + # TODO: remove this test when we remove the conditional + def test_permits_manage_projects_without_2fa_for_older_users( + 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(2019, 1, 1), + ), + matched_route=pretend.stub(name="manage.projects"), + ) + context = pretend.stub(__acl__=[(Allow, "user:5", "myperm")]) + + policy = policy_class() + assert policy.permits(request, context, "myperm") + + def test_permits_manage_projects_with_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=True, + ), + matched_route=pretend.stub(name="manage.projects"), + ) + context = pretend.stub(__acl__=[(Allow, "user:5", "myperm")]) + + policy = policy_class() + assert policy.permits(request, context, "myperm") + + def test_deny_manage_projects_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="manage.projects"), + ) + context = pretend.stub(__acl__=[(Allow, "user:5", "myperm")]) + + policy = policy_class() + assert not policy.permits(request, context, "myperm") + + @pytest.mark.parametrize( + "matched_route", + [ + "manage.account", + "manage.account.recovery-codes", + "manage.account.totp-provision", + "manage.account.two-factor", + "manage.account.webauthn-provision", + "manage.account.webauthn-provision.validate", + ], + ) + def test_permits_2fa_routes_without_2fa( + self, monkeypatch, policy_class, matched_route + ): + 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.now(), + ), + matched_route=pretend.stub(name=matched_route), + ) + + context = pretend.stub(__acl__=[(Allow, "user:5", "myperm")]) + + policy = policy_class() + assert policy.permits(request, context, "myperm") diff --git a/warehouse/accounts/security_policy.py b/warehouse/accounts/security_policy.py index 3f894ec3d88c..0dac26b4f1af 100644 --- a/warehouse/accounts/security_policy.py +++ b/warehouse/accounts/security_policy.py @@ -184,16 +184,7 @@ def authenticated_userid(self, request): raise NotImplementedError def permits(self, request, context, permission): - res = _permits_for_user_policy(self._acl, request, context, permission) - # Verify email before you can manage account/projects. - if ( - isinstance(res, Allowed) - and not request.identity.has_primary_verified_email - and request.matched_route.name.startswith("manage") - and request.matched_route.name != "manage.account" - ): - return WarehouseDenied("unverified", reason="unverified_email") - return res + return _permits_for_user_policy(self._acl, request, context, permission) @implementer(ISecurityPolicy) @@ -248,6 +239,15 @@ def _permits_for_user_policy(acl, request, context, permission): # NOTE: These parameters are in a different order than the signature of this method. res = acl.permits(context, principals_for(request.identity), permission) + # Verify email before you can manage account/projects. + if ( + isinstance(res, Allowed) + and not request.identity.has_primary_verified_email + and request.matched_route.name.startswith("manage") + and request.matched_route.name != "manage.account" + ): + return WarehouseDenied("unverified", reason="unverified_email") + # If our underlying permits allowed this, we will check our 2FA status, # that might possibly return a reason to deny the request anyways, and if # it does we'll return that. @@ -309,4 +309,33 @@ 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. + + # Management routes that don't require 2FA, mostly to set up 2FA. + _exempt_routes = [ + "manage.account.recovery-codes", + "manage.account.totp-provision", + "manage.account.two-factor", + "manage.account.webauthn-provision", + ] + + 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 + ) 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", + ) + return None diff --git a/warehouse/locale/messages.pot b/warehouse/locale/messages.pot index cdf08668069c..14b26ac7d2ee 100644 --- a/warehouse/locale/messages.pot +++ b/warehouse/locale/messages.pot @@ -4,13 +4,13 @@ msgid "" "this action." msgstr "" -#: warehouse/views.py:157 +#: warehouse/views.py:161 msgid "" "Two-factor authentication must be enabled on your account to perform this" " action." msgstr "" -#: warehouse/views.py:282 +#: warehouse/views.py:286 msgid "Locale updated" msgstr "" diff --git a/warehouse/views.py b/warehouse/views.py index 61554e33bc10..0a2b5ac9d5db 100644 --- a/warehouse/views.py +++ b/warehouse/views.py @@ -152,7 +152,11 @@ def forbidden(exc, request): # If the forbidden error is because the user doesn't have 2FA enabled, we'll # redirect them to the 2FA page - if exc.result.reason in {"owners_require_2fa", "pypi_mandates_2fa"}: + if exc.result.reason in { + "owners_require_2fa", + "pypi_mandates_2fa", + "manage_2fa_required", + }: request.session.flash( request._( "Two-factor authentication must be enabled on your account to "