Skip to content

Commit

Permalink
fix: require primary email before adding 2fa
Browse files Browse the repository at this point in the history
  • Loading branch information
mdtro committed Dec 11, 2024
1 parent ef9534e commit 8ecedc5
Show file tree
Hide file tree
Showing 6 changed files with 75 additions and 5 deletions.
16 changes: 15 additions & 1 deletion src/sentry/api/decorators.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@
from rest_framework.request import Request
from rest_framework.response import Response

from sentry.api.exceptions import EmailVerificationRequired, SudoRequired
from sentry.api.exceptions import (
EmailVerificationRequired,
PrimaryEmailVerificationRequired,
SudoRequired,
)
from sentry.models.apikey import is_api_key_auth
from sentry.models.apitoken import is_api_token_auth
from sentry.models.orgauthtoken import is_org_auth_token_auth
Expand Down Expand Up @@ -45,3 +49,13 @@ def wrapped(self, request: Request, *args, **kwargs) -> Response:
return func(self, request, *args, **kwargs)

return wrapped


def primary_email_verification_required(func):
@wraps(func)
def wrapped(self, request: Request, *args, **kwargs) -> Response:
if isinstance(request.user, AnonymousUser) or not request.user.has_verified_primary_email():
raise PrimaryEmailVerificationRequired(request.user)
return func(self, request, *args, **kwargs)

return wrapped
9 changes: 9 additions & 0 deletions src/sentry/api/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,15 @@ def __init__(self, user):
super().__init__(username=user.username)


class PrimaryEmailVerificationRequired(SentryAPIException):
status_code = status.HTTP_401_UNAUTHORIZED
code = "primary-email-verification-required"
message = "Primary email verification required."

def __init__(self, user):
super().__init__(username=user.username)


class TwoFactorRequired(SentryAPIException):
status_code = status.HTTP_401_UNAUTHORIZED
code = "2fa-required"
Expand Down
4 changes: 2 additions & 2 deletions src/sentry/users/api/endpoints/user_authenticator_enroll.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
from sentry.api.api_owners import ApiOwner
from sentry.api.api_publish_status import ApiPublishStatus
from sentry.api.base import control_silo_endpoint
from sentry.api.decorators import email_verification_required, sudo_required
from sentry.api.decorators import primary_email_verification_required, sudo_required
from sentry.api.invite_helper import ApiInviteHelper, remove_invite_details_from_session
from sentry.api.serializers import serialize
from sentry.auth.authenticators.base import EnrollmentStatus, NewEnrollmentDisallowed
Expand Down Expand Up @@ -175,7 +175,7 @@ def get(self, request: Request, user: User, interface_id: str) -> HttpResponse:
return Response(response)

@sudo_required
@email_verification_required
@primary_email_verification_required
def post(self, request: Request, user: User, interface_id: str) -> HttpResponse:
"""
Enroll in authenticator interface
Expand Down
3 changes: 3 additions & 0 deletions src/sentry/users/models/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,9 @@ def has_verified_emails(self) -> bool:
def has_unverified_emails(self) -> bool:
return self.get_unverified_emails().exists()

def has_verified_primary_email(self) -> bool:
return self.emails.filter(is_verified=True, email=self.email).exists()

def has_usable_password(self) -> bool:
if self.password == "" or self.password is None:
# This is the behavior we've been relying on from Django 1.6 - 2.0.
Expand Down
3 changes: 3 additions & 0 deletions src/sentry/users/services/user/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,9 @@ def has_unverified_emails(self) -> bool:
def has_verified_emails(self) -> bool:
return len(self.get_verified_emails()) > 0

def has_verified_primary_email(self) -> bool:
return bool([e for e in self.useremails if e.is_verified and e.email == self.email])

def get_unverified_emails(self) -> list[RpcUserEmail]:
return [e for e in self.useremails if not e.is_verified]

Expand Down
45 changes: 43 additions & 2 deletions tests/sentry/users/api/endpoints/test_user_authenticator_enroll.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,47 @@ def test_totp_can_enroll(self, validate_otp):
assert interface.secret == "secret56"
assert interface.config == {"secret": "secret56"}

@mock.patch("sentry.auth.authenticators.TotpInterface.validate_otp", return_value=True)
def test_totp_no_verified_primary_email(self, validate_otp):
from urllib.parse import quote

user = self.create_user()
UserEmail.objects.filter(user=user, email=user.email).update(is_verified=False)
self.login_as(user)

# XXX: Pretend an unbound function exists.
validate_otp.__func__ = None

with mock.patch(
"sentry.auth.authenticators.base.generate_secret_key", return_value="Z" * 32
):
resp = self.get_success_response("me", "totp")

assert resp.data["secret"] == "Z" * 32
assert (
resp.data["qrcode"]
== f"otpauth://totp/{quote(user.email)}?issuer=Sentry&secret=ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ"
)
assert resp.data["form"]
assert resp.data["secret"]

# try to enroll
with self.tasks():
resp = self.get_error_response(
"me",
"totp",
method="post",
status_code=401,
**{"secret": "secret12", "otp": "1234"},
)
assert resp.data == {
"detail": {
"code": "primary-email-verification-required",
"message": "Primary email verification required.",
"extra": {"username": user.email},
}
}

@override_options({"totp.disallow-new-enrollment": True})
def test_totp_disallow_new_enrollment(self):
self.get_error_response(
Expand Down Expand Up @@ -188,8 +229,8 @@ def test_sms_no_verified_email(self):
)
assert resp.data == {
"detail": {
"code": "email-verification-required",
"message": "Email verification required.",
"code": "primary-email-verification-required",
"message": "Primary email verification required.",
"extra": {"username": user.email},
}
}
Expand Down

0 comments on commit 8ecedc5

Please sign in to comment.