From 422c49f90e36c4080e5dfb71ec4563a1edce723b Mon Sep 17 00:00:00 2001 From: nefrob Date: Tue, 31 Jan 2023 23:13:46 -0700 Subject: [PATCH 1/9] :sparkles: Adds HOTP 2fa code support --- ..._user_first_name_alter_user_id_and_more.py | 33 ++++++++++++++ testproject/tests/conftest.py | 14 ++++++ testproject/tests/test_add_mfa.py | 6 +-- testproject/tests/test_utils.py | 16 ++++++- trench/backends/base.py | 27 +++++++---- trench/command/create_mfa_method.py | 3 +- trench/command/create_otp.py | 15 +++++-- ...ry_is_active_mfamethod_counter_and_more.py | 45 +++++++++++++++++++ trench/models.py | 3 ++ trench/settings.py | 6 +++ trench/views/base.py | 10 +++-- 11 files changed, 158 insertions(+), 20 deletions(-) create mode 100644 testproject/testapp/migrations/0004_alter_user_first_name_alter_user_id_and_more.py create mode 100644 trench/migrations/0005_remove_mfamethod_primary_is_active_mfamethod_counter_and_more.py diff --git a/testproject/testapp/migrations/0004_alter_user_first_name_alter_user_id_and_more.py b/testproject/testapp/migrations/0004_alter_user_first_name_alter_user_id_and_more.py new file mode 100644 index 00000000..a18eccd7 --- /dev/null +++ b/testproject/testapp/migrations/0004_alter_user_first_name_alter_user_id_and_more.py @@ -0,0 +1,33 @@ +# Generated by Django 4.1.5 on 2023-02-01 05:43 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("testapp", "0003_remove_user_yubikey_id"), + ] + + operations = [ + migrations.AlterField( + model_name="user", + name="first_name", + field=models.CharField( + blank=True, max_length=150, verbose_name="first name" + ), + ), + migrations.AlterField( + model_name="user", + name="id", + field=models.BigAutoField( + auto_created=True, primary_key=True, serialize=False, verbose_name="ID" + ), + ), + migrations.AlterField( + model_name="user", + name="last_name", + field=models.CharField( + blank=True, max_length=150, verbose_name="last name" + ), + ), + ] diff --git a/testproject/tests/conftest.py b/testproject/tests/conftest.py index 81d16ce3..59b52e39 100644 --- a/testproject/tests/conftest.py +++ b/testproject/tests/conftest.py @@ -74,6 +74,20 @@ def active_user_with_email_otp() -> UserModel: return user +@pytest.fixture() +def active_user_with_email_hotp() -> UserModel: + user, created = User.objects.get_or_create( + username="imhotep", + email="imhotep@pyramids.eg", + ) + if created: + user.set_password("secretkey"), + user.is_active = True + user.save() + mfa_method_creator(user=user, method_name="email", is_totp=False) + return user + + @pytest.fixture() def deactivated_user_with_email_otp(active_user_with_email_otp) -> UserModel: active_user_with_email_otp.is_active = False diff --git a/testproject/tests/test_add_mfa.py b/testproject/tests/test_add_mfa.py index d8ecb574..1dc554cb 100644 --- a/testproject/tests/test_add_mfa.py +++ b/testproject/tests/test_add_mfa.py @@ -8,7 +8,7 @@ from rest_framework.status import HTTP_200_OK, HTTP_400_BAD_REQUEST from tests.utils import TrenchAPIClient -from trench.command.create_otp import create_otp_command +from trench.command.create_otp import create_totp_command from trench.command.create_secret import create_secret_command @@ -24,7 +24,7 @@ def test_add_user_mfa(active_user): path="/auth/email/activate/", data={ "secret": secret, - "code": create_otp_command(secret=secret, interval=60).now(), + "code": create_totp_command(secret=secret, interval=60).now(), "user": getattr(active_user, active_user.USERNAME_FIELD), }, format="json", @@ -43,7 +43,7 @@ def test_should_fail_on_add_user_mfa_with_invalid_source_field(active_user: User path="/auth/email/activate/", data={ "secret": secret, - "code": create_otp_command(secret=secret, interval=60).now(), + "code": create_totp_command(secret=secret, interval=60).now(), "user": getattr(active_user, active_user.USERNAME_FIELD), }, format="json", diff --git a/testproject/tests/test_utils.py b/testproject/tests/test_utils.py index f5807a5d..e94826d6 100644 --- a/testproject/tests/test_utils.py +++ b/testproject/tests/test_utils.py @@ -37,7 +37,7 @@ def test_innermost_object_test(active_user): @pytest.mark.django_db -def test_validate_code(active_user_with_email_otp): +def test_validate_code_totp(active_user_with_email_otp): email_method = active_user_with_email_otp.mfa_methods.get() handler = get_mfa_handler(mfa_method=email_method) valid_code = handler.create_code() @@ -46,6 +46,20 @@ def test_validate_code(active_user_with_email_otp): assert handler.validate_code(code=valid_code) is True +@pytest.mark.django_db +def test_validate_code_hotp(active_user_with_email_hotp): + email_method = active_user_with_email_hotp.mfa_methods.get() + handler = get_mfa_handler(mfa_method=email_method) + valid_code = handler.create_code() + + assert handler.validate_code(code="123456") is False + assert handler.validate_code(code=valid_code) is True + + new_valid_code = handler.create_code() + assert handler.validate_code(code=valid_code) is False + assert handler.validate_code(code=new_valid_code) is True + + @pytest.mark.django_db def test_validate_code_yubikey(active_user_with_many_otp_methods): active_user, _ = active_user_with_many_otp_methods diff --git a/trench/backends/base.py b/trench/backends/base.py index c32fe1a7..c7a0ab91 100644 --- a/trench/backends/base.py +++ b/trench/backends/base.py @@ -1,10 +1,10 @@ from django.db.models import Model from abc import ABC, abstractmethod -from pyotp import TOTP +from pyotp import TOTP, HOTP from typing import Any, Dict, Optional, Tuple -from trench.command.create_otp import create_otp_command +from trench.command.create_otp import create_totp_command, create_hotp_command from trench.exceptions import MissingConfigurationError from trench.models import MFAMethod from trench.responses import DispatchResponse @@ -64,7 +64,12 @@ def dispatch_message(self) -> DispatchResponse: raise NotImplementedError # pragma: no cover def create_code(self) -> str: - return self._get_otp().now() + if self._mfa_method.is_totp: + return self._get_otp().now() + else: + self._mfa_method.counter += 1 + self._mfa_method.save() + return self._get_otp().at(self._mfa_method.counter) def confirm_activation(self, code: str) -> None: pass @@ -73,12 +78,18 @@ def validate_confirmation_code(self, code: str) -> bool: return self.validate_code(code) def validate_code(self, code: str) -> bool: - return self._get_otp().verify(otp=code) + if self._mfa_method.is_totp: + return self._get_otp().verify(otp=code) + else: + return self._get_otp().verify(otp=code, counter=self._mfa_method.counter) - def _get_otp(self) -> TOTP: - return create_otp_command( - secret=self._mfa_method.secret, interval=self._get_valid_window() - ) + def _get_otp(self) -> TOTP | HOTP: + if self._mfa_method.is_totp: + return create_totp_command( + secret=self._mfa_method.secret, interval=self._get_valid_window() + ) + else: + return create_hotp_command(secret=self._mfa_method.secret) def _get_valid_window(self) -> int: return self._config.get( diff --git a/trench/command/create_mfa_method.py b/trench/command/create_mfa_method.py index cbe2efe8..d8981bec 100644 --- a/trench/command/create_mfa_method.py +++ b/trench/command/create_mfa_method.py @@ -11,10 +11,11 @@ def __init__(self, secret_generator: Callable, mfa_model: Type[MFAMethod]) -> No self._mfa_model = mfa_model self._create_secret = secret_generator - def execute(self, user_id: int, name: str) -> MFAMethod: + def execute(self, user_id: int, name: str, is_totp: bool = True) -> MFAMethod: mfa, created = self._mfa_model.objects.get_or_create( user_id=user_id, name=name, + is_totp=is_totp, defaults={ "secret": self._create_secret, "is_active": False, diff --git a/trench/command/create_otp.py b/trench/command/create_otp.py index 93ca4b7e..a3a1017d 100644 --- a/trench/command/create_otp.py +++ b/trench/command/create_otp.py @@ -1,10 +1,19 @@ -from pyotp import TOTP +from pyotp import TOTP, HOTP -class CreateOTPCommand: +class CreateTOTPCommand: @staticmethod def execute(secret: str, interval: int) -> TOTP: return TOTP(secret, interval=interval) -create_otp_command = CreateOTPCommand.execute +create_totp_command = CreateTOTPCommand.execute + + +class CreateHOTPCommand: + @staticmethod + def execute(secret: str) -> HOTP: + return HOTP(secret) + + +create_hotp_command = CreateHOTPCommand.execute diff --git a/trench/migrations/0005_remove_mfamethod_primary_is_active_mfamethod_counter_and_more.py b/trench/migrations/0005_remove_mfamethod_primary_is_active_mfamethod_counter_and_more.py new file mode 100644 index 00000000..dab200e1 --- /dev/null +++ b/trench/migrations/0005_remove_mfamethod_primary_is_active_mfamethod_counter_and_more.py @@ -0,0 +1,45 @@ +# Generated by Django 4.1.5 on 2023-02-01 05:51 +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("trench", "0004_alter_mfamethod_id_mfamethod_unique_user_is_primary_and_more"), + ] + + operations = [ + migrations.RemoveConstraint( + model_name="mfamethod", + name="primary_is_active", + ), + migrations.AddField( + model_name="mfamethod", + name="counter", + field=models.PositiveIntegerField(default=0, verbose_name="counter"), + ), + migrations.AddField( + model_name="mfamethod", + name="is_totp", + field=models.BooleanField(default=True, verbose_name="is totp"), + ), + migrations.AlterField( + model_name="mfamethod", + name="id", + field=models.BigAutoField( + auto_created=True, primary_key=True, serialize=False, verbose_name="ID" + ), + ), + migrations.AddConstraint( + model_name="mfamethod", + constraint=models.CheckConstraint( + check=models.Q( + models.Q(("is_primary", True), ("is_active", True)), + ("is_primary", False), + _connector="OR", + ), + name="primary_is_active", + ), + ), + ] diff --git a/trench/models.py b/trench/models.py index 0e741a29..363658f5 100644 --- a/trench/models.py +++ b/trench/models.py @@ -11,6 +11,7 @@ QuerySet, TextField, UniqueConstraint, + PositiveIntegerField, ) from django.utils.translation import gettext_lazy as _ @@ -70,6 +71,8 @@ class MFAMethod(Model): ) name = CharField(_("name"), max_length=255) secret = CharField(_("secret"), max_length=255) + counter = PositiveIntegerField(_("counter"), default=0) + is_totp = BooleanField(_("is totp"), default=True) is_primary = BooleanField(_("is primary"), default=False) is_active = BooleanField(_("is active"), default=False) _backup_codes = TextField(_("backup codes"), blank=True) diff --git a/trench/settings.py b/trench/settings.py index 885fad5a..a4021d12 100644 --- a/trench/settings.py +++ b/trench/settings.py @@ -44,6 +44,7 @@ def __getitem__(self, attr: str) -> Any: SOURCE_FIELD = "SOURCE_FIELD" HANDLER = "HANDLER" VALIDITY_PERIOD = "VALIDITY_PERIOD" +USE_TOPT = "USE_TOPT" VERBOSE_NAME = "VERBOSE_NAME" EMAIL_SUBJECT = "EMAIL_SUBJECT" EMAIL_PLAIN_TEMPLATE = "EMAIL_PLAIN_TEMPLATE" @@ -73,6 +74,7 @@ def __getitem__(self, attr: str) -> Any: "sms_twilio": { VERBOSE_NAME: _("sms_twilio"), VALIDITY_PERIOD: 30, + USE_TOPT: True, HANDLER: "trench.backends.twilio.TwilioMessageDispatcher", SOURCE_FIELD: "phone_number", TWILIO_VERIFIED_FROM_NUMBER: "YOUR TWILIO REGISTERED NUMBER", @@ -80,6 +82,7 @@ def __getitem__(self, attr: str) -> Any: "sms_api": { VERBOSE_NAME: _("sms_api"), VALIDITY_PERIOD: 30, + USE_TOPT: True, HANDLER: "trench.backends.sms_api.SMSAPIMessageDispatcher", SOURCE_FIELD: "phone_number", SMSAPI_ACCESS_TOKEN: "YOUR SMSAPI TOKEN", @@ -88,6 +91,7 @@ def __getitem__(self, attr: str) -> Any: "sms_aws": { VERBOSE_NAME: _("sms_aws"), VALIDITY_PERIOD: 30, + USE_TOPT: True, HANDLER: "trench.backends.aws.AWSMessageDispatcher", SOURCE_FIELD: "phone_number", AWS_ACCESS_KEY: "YOUR AWS ACCESS KEY", @@ -97,6 +101,7 @@ def __getitem__(self, attr: str) -> Any: "email": { VERBOSE_NAME: _("email"), VALIDITY_PERIOD: 30, + USE_TOPT: True, HANDLER: "trench.backends.basic_mail.SendMailMessageDispatcher", SOURCE_FIELD: "email", EMAIL_SUBJECT: _("Your verification code"), @@ -106,6 +111,7 @@ def __getitem__(self, attr: str) -> Any: "app": { VERBOSE_NAME: _("app"), VALIDITY_PERIOD: 30, + USE_TOPT: True, "USES_THIRD_PARTY_CLIENT": True, HANDLER: "trench.backends.application.ApplicationMessageDispatcher", }, diff --git a/trench/views/base.py b/trench/views/base.py index cd896413..5eef930c 100644 --- a/trench/views/base.py +++ b/trench/views/base.py @@ -43,7 +43,7 @@ MFAMethodDeactivationValidator, UserMFAMethodSerializer, ) -from trench.settings import SOURCE_FIELD, trench_settings +from trench.settings import SOURCE_FIELD, USE_TOPT, trench_settings from trench.utils import available_method_choices, get_mfa_model, user_token_generator @@ -104,20 +104,22 @@ class MFAMethodActivationView(APIView): @staticmethod def post(request: Request, method: str) -> Response: try: - source_field = get_mfa_config_by_name_query(name=method).get(SOURCE_FIELD) + mfa_config = get_mfa_config_by_name_query(name=method) + source_field = mfa_config.get(SOURCE_FIELD) + is_totp = mfa_config.get(USE_TOPT) except MFAMethodDoesNotExistError as cause: return ErrorResponse(error=cause) user = request.user try: if source_field is not None and not hasattr(user, source_field): raise MFASourceFieldDoesNotExistError( - source_field, - user.__class__.__name__ + source_field, user.__class__.__name__ ) mfa = create_mfa_method_command( user_id=user.id, name=method, + is_totp=is_totp, ) except MFAValidationError as cause: return ErrorResponse(error=cause) From ed3dfb90cee5dfd6d428f5c993cd5578cc87040c Mon Sep 17 00:00:00 2001 From: nefrob Date: Wed, 1 Feb 2023 14:51:36 -0700 Subject: [PATCH 2/9] :hammer: Fixes yubi issue and adds extra test check --- testproject/tests/test_utils.py | 6 ++++++ trench/views/base.py | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/testproject/tests/test_utils.py b/testproject/tests/test_utils.py index e94826d6..5d48c36e 100644 --- a/testproject/tests/test_utils.py +++ b/testproject/tests/test_utils.py @@ -44,6 +44,11 @@ def test_validate_code_totp(active_user_with_email_otp): assert handler.validate_code(code="123456") is False assert handler.validate_code(code=valid_code) is True + + # both codes are valid during the validitiy window + new_valid_code = handler.create_code() + assert handler.validate_code(code=valid_code) is True + assert handler.validate_code(code=new_valid_code) is True @pytest.mark.django_db @@ -55,6 +60,7 @@ def test_validate_code_hotp(active_user_with_email_hotp): assert handler.validate_code(code="123456") is False assert handler.validate_code(code=valid_code) is True + # creating a new code invalidates the previous one new_valid_code = handler.create_code() assert handler.validate_code(code=valid_code) is False assert handler.validate_code(code=new_valid_code) is True diff --git a/trench/views/base.py b/trench/views/base.py index 5eef930c..d53358ae 100644 --- a/trench/views/base.py +++ b/trench/views/base.py @@ -106,7 +106,7 @@ def post(request: Request, method: str) -> Response: try: mfa_config = get_mfa_config_by_name_query(name=method) source_field = mfa_config.get(SOURCE_FIELD) - is_totp = mfa_config.get(USE_TOPT) + is_totp = mfa_config.get(USE_TOPT, True) except MFAMethodDoesNotExistError as cause: return ErrorResponse(error=cause) user = request.user From 298764843bb8d71c40d3b1b544780c2d263cf63b Mon Sep 17 00:00:00 2001 From: nefrob Date: Thu, 2 Feb 2023 22:05:09 -0700 Subject: [PATCH 3/9] :recycle: Support hotp validity period --- ..._user_first_name_alter_user_id_and_more.py | 2 +- testproject/tests/conftest.py | 1 + testproject/tests/test_utils.py | 26 ++++++++++++- trench/backends/base.py | 39 +++++++++++++++---- ...e_mfamethod_primary_is_active_and_more.py} | 10 ++++- trench/models.py | 6 ++- trench/settings.py | 1 - 7 files changed, 70 insertions(+), 15 deletions(-) rename trench/migrations/{0005_remove_mfamethod_primary_is_active_mfamethod_counter_and_more.py => 0005_remove_mfamethod_primary_is_active_and_more.py} (82%) diff --git a/testproject/testapp/migrations/0004_alter_user_first_name_alter_user_id_and_more.py b/testproject/testapp/migrations/0004_alter_user_first_name_alter_user_id_and_more.py index a18eccd7..a358e0e8 100644 --- a/testproject/testapp/migrations/0004_alter_user_first_name_alter_user_id_and_more.py +++ b/testproject/testapp/migrations/0004_alter_user_first_name_alter_user_id_and_more.py @@ -1,4 +1,4 @@ -# Generated by Django 4.1.5 on 2023-02-01 05:43 +# Generated by Django 4.1.5 on 2023-02-03 04:55 from django.db import migrations, models diff --git a/testproject/tests/conftest.py b/testproject/tests/conftest.py index 59b52e39..e585cc6c 100644 --- a/testproject/tests/conftest.py +++ b/testproject/tests/conftest.py @@ -43,6 +43,7 @@ def mfa_method_creator( is_primary=is_primary, name=method_name, is_active=method_args.pop("is_active", True), + code_generated_at=None, **method_args, ) diff --git a/testproject/tests/test_utils.py b/testproject/tests/test_utils.py index 5d48c36e..8c1aec10 100644 --- a/testproject/tests/test_utils.py +++ b/testproject/tests/test_utils.py @@ -37,7 +37,7 @@ def test_innermost_object_test(active_user): @pytest.mark.django_db -def test_validate_code_totp(active_user_with_email_otp): +def test_create_code_hotp(active_user_with_email_otp): email_method = active_user_with_email_otp.mfa_methods.get() handler = get_mfa_handler(mfa_method=email_method) valid_code = handler.create_code() @@ -51,17 +51,41 @@ def test_validate_code_totp(active_user_with_email_otp): assert handler.validate_code(code=new_valid_code) is True +@pytest.mark.django_db +def test_validate_code_totp(active_user_with_email_otp): + email_method = active_user_with_email_otp.mfa_methods.get() + handler = get_mfa_handler(mfa_method=email_method) + valid_code = handler.create_code() + + assert handler.validate_code(code="123456") is False + assert handler.validate_code(code=valid_code) is True + + @pytest.mark.django_db def test_validate_code_hotp(active_user_with_email_hotp): email_method = active_user_with_email_hotp.mfa_methods.get() handler = get_mfa_handler(mfa_method=email_method) valid_code = handler.create_code() + email_method.refresh_from_db() + assert email_method.code_generated_at is not None + assert handler.validate_code(code="123456") is False + email_method.refresh_from_db() + assert email_method.code_generated_at is not None + + # successful validation clears code generation timestemp assert handler.validate_code(code=valid_code) is True + email_method.refresh_from_db() + assert email_method.code_generated_at is None + + # subsequently validating the same code twice fails + assert handler.validate_code(code=valid_code) is False # creating a new code invalidates the previous one + valid_code = handler.create_code() new_valid_code = handler.create_code() + assert new_valid_code != valid_code assert handler.validate_code(code=valid_code) is False assert handler.validate_code(code=new_valid_code) is True diff --git a/trench/backends/base.py b/trench/backends/base.py index c7a0ab91..9855c6e8 100644 --- a/trench/backends/base.py +++ b/trench/backends/base.py @@ -1,6 +1,8 @@ from django.db.models import Model +from django.utils import timezone from abc import ABC, abstractmethod +from datetime import timedelta from pyotp import TOTP, HOTP from typing import Any, Dict, Optional, Tuple @@ -64,12 +66,15 @@ def dispatch_message(self) -> DispatchResponse: raise NotImplementedError # pragma: no cover def create_code(self) -> str: + # totp if self._mfa_method.is_totp: return self._get_otp().now() - else: - self._mfa_method.counter += 1 - self._mfa_method.save() - return self._get_otp().at(self._mfa_method.counter) + + # hotp + self._mfa_method.counter += 1 + self._mfa_method.code_generated_at = timezone.now() + self._mfa_method.save() + return self._get_otp().at(self._mfa_method.counter) def confirm_activation(self, code: str) -> None: pass @@ -78,18 +83,36 @@ def validate_confirmation_code(self, code: str) -> bool: return self.validate_code(code) def validate_code(self, code: str) -> bool: + # totp if self._mfa_method.is_totp: return self._get_otp().verify(otp=code) - else: - return self._get_otp().verify(otp=code, counter=self._mfa_method.counter) + + # hotp + is_valid = self._get_otp().verify(otp=code, counter=self._mfa_method.counter) + if not is_valid or not self._mfa_method.code_generated_at: + return False + + min_time = self._mfa_method.code_generated_at + max_time = self._mfa_method.code_generated_at + timedelta( + seconds=self._get_valid_window() + ) + now = timezone.now() + if now < min_time or now > max_time: + return False + + self._mfa_method.code_generated_at = None + self._mfa_method.save() + return True def _get_otp(self) -> TOTP | HOTP: + # totp if self._mfa_method.is_totp: return create_totp_command( secret=self._mfa_method.secret, interval=self._get_valid_window() ) - else: - return create_hotp_command(secret=self._mfa_method.secret) + + # hotp + return create_hotp_command(secret=self._mfa_method.secret) def _get_valid_window(self) -> int: return self._config.get( diff --git a/trench/migrations/0005_remove_mfamethod_primary_is_active_mfamethod_counter_and_more.py b/trench/migrations/0005_remove_mfamethod_primary_is_active_and_more.py similarity index 82% rename from trench/migrations/0005_remove_mfamethod_primary_is_active_mfamethod_counter_and_more.py rename to trench/migrations/0005_remove_mfamethod_primary_is_active_and_more.py index dab200e1..c006002c 100644 --- a/trench/migrations/0005_remove_mfamethod_primary_is_active_mfamethod_counter_and_more.py +++ b/trench/migrations/0005_remove_mfamethod_primary_is_active_and_more.py @@ -1,5 +1,4 @@ -# Generated by Django 4.1.5 on 2023-02-01 05:51 -from __future__ import unicode_literals +# Generated by Django 4.1.5 on 2023-02-03 04:55 from django.db import migrations, models @@ -14,6 +13,13 @@ class Migration(migrations.Migration): model_name="mfamethod", name="primary_is_active", ), + migrations.AddField( + model_name="mfamethod", + name="code_generated_at", + field=models.DateTimeField( + blank=True, null=True, verbose_name="code generated at" + ), + ), migrations.AddField( model_name="mfamethod", name="counter", diff --git a/trench/models.py b/trench/models.py index 363658f5..29c42d20 100644 --- a/trench/models.py +++ b/trench/models.py @@ -1,17 +1,18 @@ from django.conf import settings from django.db.models import ( - CASCADE, BooleanField, + CASCADE, CharField, CheckConstraint, + DateTimeField, ForeignKey, Manager, Model, + PositiveIntegerField, Q, QuerySet, TextField, UniqueConstraint, - PositiveIntegerField, ) from django.utils.translation import gettext_lazy as _ @@ -72,6 +73,7 @@ class MFAMethod(Model): name = CharField(_("name"), max_length=255) secret = CharField(_("secret"), max_length=255) counter = PositiveIntegerField(_("counter"), default=0) + code_generated_at = DateTimeField(_("code generated at"), blank=True, null=True) is_totp = BooleanField(_("is totp"), default=True) is_primary = BooleanField(_("is primary"), default=False) is_active = BooleanField(_("is active"), default=False) diff --git a/trench/settings.py b/trench/settings.py index a4021d12..080956e3 100644 --- a/trench/settings.py +++ b/trench/settings.py @@ -111,7 +111,6 @@ def __getitem__(self, attr: str) -> Any: "app": { VERBOSE_NAME: _("app"), VALIDITY_PERIOD: 30, - USE_TOPT: True, "USES_THIRD_PARTY_CLIENT": True, HANDLER: "trench.backends.application.ApplicationMessageDispatcher", }, From 2febbc6b80addc609a161f6632868eda4e23b18d Mon Sep 17 00:00:00 2001 From: nefrob Date: Thu, 2 Feb 2023 22:09:24 -0700 Subject: [PATCH 4/9] =?UTF-8?q?=F0=9F=97=91=EF=B8=8F=20Remove=20unused=20t?= =?UTF-8?q?est?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- testproject/tests/test_utils.py | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/testproject/tests/test_utils.py b/testproject/tests/test_utils.py index 8c1aec10..2a904ecf 100644 --- a/testproject/tests/test_utils.py +++ b/testproject/tests/test_utils.py @@ -36,21 +36,6 @@ def test_innermost_object_test(active_user): AbstractMessageDispatcher._get_innermost_object(active_user, dotted_path="test") -@pytest.mark.django_db -def test_create_code_hotp(active_user_with_email_otp): - email_method = active_user_with_email_otp.mfa_methods.get() - handler = get_mfa_handler(mfa_method=email_method) - valid_code = handler.create_code() - - assert handler.validate_code(code="123456") is False - assert handler.validate_code(code=valid_code) is True - - # both codes are valid during the validitiy window - new_valid_code = handler.create_code() - assert handler.validate_code(code=valid_code) is True - assert handler.validate_code(code=new_valid_code) is True - - @pytest.mark.django_db def test_validate_code_totp(active_user_with_email_otp): email_method = active_user_with_email_otp.mfa_methods.get() From d2e93e06740e19046aefbe75fdc228993f59395e Mon Sep 17 00:00:00 2001 From: nefrob Date: Sat, 4 Feb 2023 09:06:59 -0700 Subject: [PATCH 5/9] :recycle: Use separate hotp dispatcher --- ..._user_first_name_alter_user_id_and_more.py | 2 +- testproject/tests/conftest.py | 14 ----- testproject/tests/test_utils.py | 50 +++++++++++++---- trench/backends/aws.py | 10 +++- trench/backends/base.py | 53 +++++++++---------- trench/backends/basic_mail.py | 9 +++- trench/backends/sms_api.py | 9 +++- trench/backends/twilio.py | 9 +++- trench/command/create_mfa_method.py | 3 +- ...ve_mfamethod_primary_is_active_and_more.py | 7 +-- trench/models.py | 1 - trench/settings.py | 5 -- trench/views/base.py | 10 ++-- 13 files changed, 105 insertions(+), 77 deletions(-) diff --git a/testproject/testapp/migrations/0004_alter_user_first_name_alter_user_id_and_more.py b/testproject/testapp/migrations/0004_alter_user_first_name_alter_user_id_and_more.py index a358e0e8..92c261fc 100644 --- a/testproject/testapp/migrations/0004_alter_user_first_name_alter_user_id_and_more.py +++ b/testproject/testapp/migrations/0004_alter_user_first_name_alter_user_id_and_more.py @@ -1,4 +1,4 @@ -# Generated by Django 4.1.5 on 2023-02-03 04:55 +# Generated by Django 4.1.5 on 2023-02-04 16:04 from django.db import migrations, models diff --git a/testproject/tests/conftest.py b/testproject/tests/conftest.py index e585cc6c..a16e94b0 100644 --- a/testproject/tests/conftest.py +++ b/testproject/tests/conftest.py @@ -75,20 +75,6 @@ def active_user_with_email_otp() -> UserModel: return user -@pytest.fixture() -def active_user_with_email_hotp() -> UserModel: - user, created = User.objects.get_or_create( - username="imhotep", - email="imhotep@pyramids.eg", - ) - if created: - user.set_password("secretkey"), - user.is_active = True - user.save() - mfa_method_creator(user=user, method_name="email", is_totp=False) - return user - - @pytest.fixture() def deactivated_user_with_email_otp(active_user_with_email_otp) -> UserModel: active_user_with_email_otp.is_active = False diff --git a/testproject/tests/test_utils.py b/testproject/tests/test_utils.py index 2a904ecf..6c1a2d77 100644 --- a/testproject/tests/test_utils.py +++ b/testproject/tests/test_utils.py @@ -2,8 +2,10 @@ from trench.backends.application import ApplicationMessageDispatcher from trench.backends.base import AbstractMessageDispatcher +from trench.backends.basic_mail import SendMailHotpMessageDispatcher from trench.backends.provider import get_mfa_handler from trench.models import MFAMethod +from trench.query.get_mfa_config_by_name import get_mfa_config_by_name_query from trench.utils import UserTokenGenerator @@ -37,39 +39,65 @@ def test_innermost_object_test(active_user): @pytest.mark.django_db -def test_validate_code_totp(active_user_with_email_otp): +def test_validate_code(active_user_with_email_otp): email_method = active_user_with_email_otp.mfa_methods.get() handler = get_mfa_handler(mfa_method=email_method) valid_code = handler.create_code() assert handler.validate_code(code="123456") is False assert handler.validate_code(code=valid_code) is True - + @pytest.mark.django_db -def test_validate_code_hotp(active_user_with_email_hotp): - email_method = active_user_with_email_hotp.mfa_methods.get() - handler = get_mfa_handler(mfa_method=email_method) - valid_code = handler.create_code() +def test_create_code_hotp(active_user_with_email_otp): + email_method = active_user_with_email_otp.mfa_methods.get() + conf = get_mfa_config_by_name_query(name=email_method.name) + handler = SendMailHotpMessageDispatcher(email_method, conf) + + email_method.counter = 0 + email_method.code_generated_at = None + email_method.save() + + handler.create_code() email_method.refresh_from_db() + assert email_method.counter == 1 assert email_method.code_generated_at is not None + previous_code_genererated_at = email_method.code_generated_at + + handler.create_code() + + email_method.refresh_from_db() + assert email_method.counter == 2 + assert email_method.code_generated_at > previous_code_genererated_at + + +@pytest.mark.django_db +def test_validate_code_hotp(active_user_with_email_otp): + email_method = active_user_with_email_otp.mfa_methods.get() + conf = get_mfa_config_by_name_query(name=email_method.name) + handler = SendMailHotpMessageDispatcher(email_method, conf) + + email_method.counter = 0 + email_method.code_generated_at = None + email_method.save() + + valid_code = handler.create_code() + assert handler.validate_code(code="123456") is False email_method.refresh_from_db() assert email_method.code_generated_at is not None - - # successful validation clears code generation timestemp + assert handler.validate_code(code=valid_code) is True email_method.refresh_from_db() assert email_method.code_generated_at is None - - # subsequently validating the same code twice fails + assert handler.validate_code(code=valid_code) is False - # creating a new code invalidates the previous one valid_code = handler.create_code() new_valid_code = handler.create_code() + assert new_valid_code != valid_code assert handler.validate_code(code=valid_code) is False assert handler.validate_code(code=new_valid_code) is True diff --git a/trench/backends/aws.py b/trench/backends/aws.py index 7d8cfaed..68a9a43c 100644 --- a/trench/backends/aws.py +++ b/trench/backends/aws.py @@ -4,7 +4,10 @@ import boto3 import botocore.exceptions -from trench.backends.base import AbstractMessageDispatcher +from trench.backends.base import ( + AbstractMessageDispatcher, + AbstractHotpMessageDispatcher, +) from trench.responses import ( DispatchResponse, FailedDispatchResponse, @@ -13,6 +16,7 @@ from trench.settings import AWS_ACCESS_KEY, AWS_SECRET_KEY, AWS_REGION from botocore.exceptions import ClientError, EndpointConnectionError + class AWSMessageDispatcher(AbstractMessageDispatcher): _SMS_BODY = _("Your verification code is: ") _SUCCESS_DETAILS = _("SMS message with MFA code has been sent.") @@ -36,3 +40,7 @@ def dispatch_message(self) -> DispatchResponse: except EndpointConnectionError as cause: logging.error(cause, exc_info=True) return FailedDispatchResponse(details=str(cause)) + + +class AWSHotpMessageDispatcher(AbstractHotpMessageDispatcher, AWSMessageDispatcher): + pass diff --git a/trench/backends/base.py b/trench/backends/base.py index 9855c6e8..640b63f9 100644 --- a/trench/backends/base.py +++ b/trench/backends/base.py @@ -66,15 +66,7 @@ def dispatch_message(self) -> DispatchResponse: raise NotImplementedError # pragma: no cover def create_code(self) -> str: - # totp - if self._mfa_method.is_totp: - return self._get_otp().now() - - # hotp - self._mfa_method.counter += 1 - self._mfa_method.code_generated_at = timezone.now() - self._mfa_method.save() - return self._get_otp().at(self._mfa_method.counter) + return self._get_otp().now() def confirm_activation(self, code: str) -> None: pass @@ -83,13 +75,32 @@ def validate_confirmation_code(self, code: str) -> bool: return self.validate_code(code) def validate_code(self, code: str) -> bool: - # totp - if self._mfa_method.is_totp: - return self._get_otp().verify(otp=code) + return self._get_otp().verify(otp=code) + + def _get_otp(self) -> TOTP: + return create_totp_command( + secret=self._mfa_method.secret, interval=self._get_valid_window() + ) + + def _get_valid_window(self) -> int: + return self._config.get( + VALIDITY_PERIOD, trench_settings.DEFAULT_VALIDITY_PERIOD + ) + + +class AbstractHotpMessageDispatcher(AbstractMessageDispatcher): + def create_code(self) -> str: + self._mfa_method.counter += 1 + self._mfa_method.code_generated_at = timezone.now() + self._mfa_method.save() + return self._get_otp().at(self._mfa_method.counter) + + def validate_code(self, code: str) -> bool: + if not self._mfa_method.code_generated_at: + return False - # hotp is_valid = self._get_otp().verify(otp=code, counter=self._mfa_method.counter) - if not is_valid or not self._mfa_method.code_generated_at: + if not is_valid: return False min_time = self._mfa_method.code_generated_at @@ -104,17 +115,5 @@ def validate_code(self, code: str) -> bool: self._mfa_method.save() return True - def _get_otp(self) -> TOTP | HOTP: - # totp - if self._mfa_method.is_totp: - return create_totp_command( - secret=self._mfa_method.secret, interval=self._get_valid_window() - ) - - # hotp + def _get_otp(self) -> HOTP: return create_hotp_command(secret=self._mfa_method.secret) - - def _get_valid_window(self) -> int: - return self._config.get( - VALIDITY_PERIOD, trench_settings.DEFAULT_VALIDITY_PERIOD - ) diff --git a/trench/backends/basic_mail.py b/trench/backends/basic_mail.py index 1f02e3eb..38c9531e 100644 --- a/trench/backends/basic_mail.py +++ b/trench/backends/basic_mail.py @@ -6,7 +6,10 @@ import logging from smtplib import SMTPException -from trench.backends.base import AbstractMessageDispatcher +from trench.backends.base import ( + AbstractMessageDispatcher, + AbstractHotpMessageDispatcher, +) from trench.responses import ( DispatchResponse, FailedDispatchResponse, @@ -39,3 +42,7 @@ def dispatch_message(self) -> DispatchResponse: except ConnectionRefusedError as cause: # pragma: nocover logging.error(cause, exc_info=True) # pragma: nocover return FailedDispatchResponse(details=str(cause)) # pragma: nocover + + +class SendMailHotpMessageDispatcher(AbstractHotpMessageDispatcher, SendMailMessageDispatcher): + pass diff --git a/trench/backends/sms_api.py b/trench/backends/sms_api.py index eba393f5..cc8875e2 100644 --- a/trench/backends/sms_api.py +++ b/trench/backends/sms_api.py @@ -4,7 +4,10 @@ from smsapi.client import SmsApiPlClient from smsapi.exception import SmsApiException -from trench.backends.base import AbstractMessageDispatcher +from trench.backends.base import ( + AbstractMessageDispatcher, + AbstractHotpMessageDispatcher, +) from trench.responses import ( DispatchResponse, FailedDispatchResponse, @@ -31,3 +34,7 @@ def dispatch_message(self) -> DispatchResponse: except SmsApiException as cause: logging.error(cause, exc_info=True) return FailedDispatchResponse(details=cause.message) + + +class SMSAPIHotpMessageDispatcher(AbstractHotpMessageDispatcher, SMSAPIMessageDispatcher): + pass diff --git a/trench/backends/twilio.py b/trench/backends/twilio.py index 897c4332..b979b1db 100644 --- a/trench/backends/twilio.py +++ b/trench/backends/twilio.py @@ -4,7 +4,10 @@ from twilio.base.exceptions import TwilioRestException from twilio.rest import Client -from trench.backends.base import AbstractMessageDispatcher +from trench.backends.base import ( + AbstractMessageDispatcher, + AbstractHotpMessageDispatcher, +) from trench.responses import ( DispatchResponse, FailedDispatchResponse, @@ -29,3 +32,7 @@ def dispatch_message(self) -> DispatchResponse: except TwilioRestException as cause: logging.error(cause, exc_info=True) return FailedDispatchResponse(details=cause.msg) + + +class TwilioHotpMessageDispatcher(AbstractHotpMessageDispatcher, TwilioMessageDispatcher): + pass diff --git a/trench/command/create_mfa_method.py b/trench/command/create_mfa_method.py index d8981bec..cbe2efe8 100644 --- a/trench/command/create_mfa_method.py +++ b/trench/command/create_mfa_method.py @@ -11,11 +11,10 @@ def __init__(self, secret_generator: Callable, mfa_model: Type[MFAMethod]) -> No self._mfa_model = mfa_model self._create_secret = secret_generator - def execute(self, user_id: int, name: str, is_totp: bool = True) -> MFAMethod: + def execute(self, user_id: int, name: str) -> MFAMethod: mfa, created = self._mfa_model.objects.get_or_create( user_id=user_id, name=name, - is_totp=is_totp, defaults={ "secret": self._create_secret, "is_active": False, diff --git a/trench/migrations/0005_remove_mfamethod_primary_is_active_and_more.py b/trench/migrations/0005_remove_mfamethod_primary_is_active_and_more.py index c006002c..c48bca8d 100644 --- a/trench/migrations/0005_remove_mfamethod_primary_is_active_and_more.py +++ b/trench/migrations/0005_remove_mfamethod_primary_is_active_and_more.py @@ -1,4 +1,4 @@ -# Generated by Django 4.1.5 on 2023-02-03 04:55 +# Generated by Django 4.1.5 on 2023-02-04 16:04 from django.db import migrations, models @@ -25,11 +25,6 @@ class Migration(migrations.Migration): name="counter", field=models.PositiveIntegerField(default=0, verbose_name="counter"), ), - migrations.AddField( - model_name="mfamethod", - name="is_totp", - field=models.BooleanField(default=True, verbose_name="is totp"), - ), migrations.AlterField( model_name="mfamethod", name="id", diff --git a/trench/models.py b/trench/models.py index 29c42d20..594fe8db 100644 --- a/trench/models.py +++ b/trench/models.py @@ -74,7 +74,6 @@ class MFAMethod(Model): secret = CharField(_("secret"), max_length=255) counter = PositiveIntegerField(_("counter"), default=0) code_generated_at = DateTimeField(_("code generated at"), blank=True, null=True) - is_totp = BooleanField(_("is totp"), default=True) is_primary = BooleanField(_("is primary"), default=False) is_active = BooleanField(_("is active"), default=False) _backup_codes = TextField(_("backup codes"), blank=True) diff --git a/trench/settings.py b/trench/settings.py index 080956e3..885fad5a 100644 --- a/trench/settings.py +++ b/trench/settings.py @@ -44,7 +44,6 @@ def __getitem__(self, attr: str) -> Any: SOURCE_FIELD = "SOURCE_FIELD" HANDLER = "HANDLER" VALIDITY_PERIOD = "VALIDITY_PERIOD" -USE_TOPT = "USE_TOPT" VERBOSE_NAME = "VERBOSE_NAME" EMAIL_SUBJECT = "EMAIL_SUBJECT" EMAIL_PLAIN_TEMPLATE = "EMAIL_PLAIN_TEMPLATE" @@ -74,7 +73,6 @@ def __getitem__(self, attr: str) -> Any: "sms_twilio": { VERBOSE_NAME: _("sms_twilio"), VALIDITY_PERIOD: 30, - USE_TOPT: True, HANDLER: "trench.backends.twilio.TwilioMessageDispatcher", SOURCE_FIELD: "phone_number", TWILIO_VERIFIED_FROM_NUMBER: "YOUR TWILIO REGISTERED NUMBER", @@ -82,7 +80,6 @@ def __getitem__(self, attr: str) -> Any: "sms_api": { VERBOSE_NAME: _("sms_api"), VALIDITY_PERIOD: 30, - USE_TOPT: True, HANDLER: "trench.backends.sms_api.SMSAPIMessageDispatcher", SOURCE_FIELD: "phone_number", SMSAPI_ACCESS_TOKEN: "YOUR SMSAPI TOKEN", @@ -91,7 +88,6 @@ def __getitem__(self, attr: str) -> Any: "sms_aws": { VERBOSE_NAME: _("sms_aws"), VALIDITY_PERIOD: 30, - USE_TOPT: True, HANDLER: "trench.backends.aws.AWSMessageDispatcher", SOURCE_FIELD: "phone_number", AWS_ACCESS_KEY: "YOUR AWS ACCESS KEY", @@ -101,7 +97,6 @@ def __getitem__(self, attr: str) -> Any: "email": { VERBOSE_NAME: _("email"), VALIDITY_PERIOD: 30, - USE_TOPT: True, HANDLER: "trench.backends.basic_mail.SendMailMessageDispatcher", SOURCE_FIELD: "email", EMAIL_SUBJECT: _("Your verification code"), diff --git a/trench/views/base.py b/trench/views/base.py index d53358ae..cd896413 100644 --- a/trench/views/base.py +++ b/trench/views/base.py @@ -43,7 +43,7 @@ MFAMethodDeactivationValidator, UserMFAMethodSerializer, ) -from trench.settings import SOURCE_FIELD, USE_TOPT, trench_settings +from trench.settings import SOURCE_FIELD, trench_settings from trench.utils import available_method_choices, get_mfa_model, user_token_generator @@ -104,22 +104,20 @@ class MFAMethodActivationView(APIView): @staticmethod def post(request: Request, method: str) -> Response: try: - mfa_config = get_mfa_config_by_name_query(name=method) - source_field = mfa_config.get(SOURCE_FIELD) - is_totp = mfa_config.get(USE_TOPT, True) + source_field = get_mfa_config_by_name_query(name=method).get(SOURCE_FIELD) except MFAMethodDoesNotExistError as cause: return ErrorResponse(error=cause) user = request.user try: if source_field is not None and not hasattr(user, source_field): raise MFASourceFieldDoesNotExistError( - source_field, user.__class__.__name__ + source_field, + user.__class__.__name__ ) mfa = create_mfa_method_command( user_id=user.id, name=method, - is_totp=is_totp, ) except MFAValidationError as cause: return ErrorResponse(error=cause) From 53f1e3fa579a1bd65cc240c2edecf4c4c5cf6648 Mon Sep 17 00:00:00 2001 From: nefrob Date: Sat, 4 Feb 2023 09:08:18 -0700 Subject: [PATCH 6/9] :pencil2: Lint --- trench/backends/basic_mail.py | 4 +++- trench/backends/sms_api.py | 4 +++- trench/backends/twilio.py | 4 +++- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/trench/backends/basic_mail.py b/trench/backends/basic_mail.py index 38c9531e..8dcc87ef 100644 --- a/trench/backends/basic_mail.py +++ b/trench/backends/basic_mail.py @@ -44,5 +44,7 @@ def dispatch_message(self) -> DispatchResponse: return FailedDispatchResponse(details=str(cause)) # pragma: nocover -class SendMailHotpMessageDispatcher(AbstractHotpMessageDispatcher, SendMailMessageDispatcher): +class SendMailHotpMessageDispatcher( + AbstractHotpMessageDispatcher, SendMailMessageDispatcher +): pass diff --git a/trench/backends/sms_api.py b/trench/backends/sms_api.py index cc8875e2..64382c2d 100644 --- a/trench/backends/sms_api.py +++ b/trench/backends/sms_api.py @@ -36,5 +36,7 @@ def dispatch_message(self) -> DispatchResponse: return FailedDispatchResponse(details=cause.message) -class SMSAPIHotpMessageDispatcher(AbstractHotpMessageDispatcher, SMSAPIMessageDispatcher): +class SMSAPIHotpMessageDispatcher( + AbstractHotpMessageDispatcher, SMSAPIMessageDispatcher +): pass diff --git a/trench/backends/twilio.py b/trench/backends/twilio.py index b979b1db..5b1f98ef 100644 --- a/trench/backends/twilio.py +++ b/trench/backends/twilio.py @@ -34,5 +34,7 @@ def dispatch_message(self) -> DispatchResponse: return FailedDispatchResponse(details=cause.msg) -class TwilioHotpMessageDispatcher(AbstractHotpMessageDispatcher, TwilioMessageDispatcher): +class TwilioHotpMessageDispatcher( + AbstractHotpMessageDispatcher, TwilioMessageDispatcher +): pass From 0f338fc56de4010616e5bd1ed2f55ac0d9918d29 Mon Sep 17 00:00:00 2001 From: nefrob Date: Fri, 17 Mar 2023 16:50:15 -0600 Subject: [PATCH 7/9] :recycle: Regenerate migrations post merge --- trench/trench | 1 + 1 file changed, 1 insertion(+) create mode 120000 trench/trench diff --git a/trench/trench b/trench/trench new file mode 120000 index 00000000..29326ae7 --- /dev/null +++ b/trench/trench @@ -0,0 +1 @@ +/Users/robertneff/Documents/django-trench/trench/ \ No newline at end of file From b44e78793f8fb0a3feb2a6b8fce42aef50e710b0 Mon Sep 17 00:00:00 2001 From: nefrob <25070989+nefrob@users.noreply.github.com> Date: Fri, 17 Mar 2023 16:54:24 -0600 Subject: [PATCH 8/9] =?UTF-8?q?=F0=9F=97=91=EF=B8=8F=20Delete=20accidental?= =?UTF-8?q?=20sym=20link=20commit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- trench/trench | 1 - 1 file changed, 1 deletion(-) delete mode 120000 trench/trench diff --git a/trench/trench b/trench/trench deleted file mode 120000 index 29326ae7..00000000 --- a/trench/trench +++ /dev/null @@ -1 +0,0 @@ -/Users/robertneff/Documents/django-trench/trench/ \ No newline at end of file From b5d7b7b73abbdcfc05528619fe53f638387be411 Mon Sep 17 00:00:00 2001 From: nefrob Date: Fri, 17 Mar 2023 16:57:15 -0600 Subject: [PATCH 9/9] :elephant: Adds missing migration --- ...hod_code_generated_at_mfamethod_counter.py | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 trench/migrations/0006_mfamethod_code_generated_at_mfamethod_counter.py diff --git a/trench/migrations/0006_mfamethod_code_generated_at_mfamethod_counter.py b/trench/migrations/0006_mfamethod_code_generated_at_mfamethod_counter.py new file mode 100644 index 00000000..cf23c721 --- /dev/null +++ b/trench/migrations/0006_mfamethod_code_generated_at_mfamethod_counter.py @@ -0,0 +1,24 @@ +# Generated by Django 4.1.5 on 2023-03-17 22:56 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("trench", "0005_remove_mfamethod_primary_is_active_and_more"), + ] + + operations = [ + migrations.AddField( + model_name="mfamethod", + name="code_generated_at", + field=models.DateTimeField( + blank=True, null=True, verbose_name="code generated at" + ), + ), + migrations.AddField( + model_name="mfamethod", + name="counter", + field=models.PositiveIntegerField(default=0, verbose_name="counter"), + ), + ]