Skip to content

feat: require 2fa for new user to upload as well #14505

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Sep 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions tests/unit/accounts/test_security_policy.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
)
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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"),
)
Expand All @@ -776,6 +781,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",
[
Expand Down
41 changes: 26 additions & 15 deletions warehouse/accounts/security_policy.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [
Expand All @@ -331,22 +332,32 @@ 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.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