Skip to content

feat: require 2fa for new users #14294

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
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
125 changes: 109 additions & 16 deletions tests/unit/accounts/test_security_policy.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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",
Expand All @@ -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")])

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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")
49 changes: 39 additions & 10 deletions warehouse/accounts/security_policy.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
4 changes: 2 additions & 2 deletions warehouse/locale/messages.pot
Original file line number Diff line number Diff line change
Expand Up @@ -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 ""

Expand Down
6 changes: 5 additions & 1 deletion warehouse/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 "
Expand Down