diff --git a/django_cryptolock/forms.py b/django_cryptolock/forms.py index 815fe7c..6d62382 100644 --- a/django_cryptolock/forms.py +++ b/django_cryptolock/forms.py @@ -1,3 +1,5 @@ +from urllib.parse import urlparse, parse_qs + from django import forms from django.contrib.auth import authenticate from django.contrib.auth import get_user_model @@ -7,7 +9,7 @@ from django.conf import settings from pybitid import bitid -from .models import Address +from .models import Address, Challenge from .validators import validate_monero_address, validate_bitcoin_address from .utils import generate_challenge @@ -21,17 +23,22 @@ class ChallengeMixin(forms.Form): challenge = forms.CharField() - def include_challange(self): - new_challenge = bitid.build_uri( - self.request.build_absolute_uri(), generate_challenge() - ) + def include_challenge(self): + """Created a new challenge only when no data is provided by user.""" if not self.data: - self.request.session["current_challenge"] = new_challenge + new_challenge = bitid.build_uri( + self.request.build_absolute_uri(), Challenge.objects.generate() + ) self.initial["challenge"] = new_challenge def clean_challenge(self): - challenge = self.cleaned_data.get("challenge") - if not challenge or challenge != self.request.session.get("current_challenge"): + challenge_uri = urlparse(self.cleaned_data.get("challenge")) + query = parse_qs(challenge_uri.query) + if not query.get("x"): + raise forms.ValidationError(_("Invalid or outdated challenge")) + + challenge = query["x"][0] + if not challenge or not Challenge.objects.is_active(challenge): raise forms.ValidationError(_("Invalid or outdated challenge")) return challenge @@ -54,7 +61,7 @@ class SimpleLoginForm(ChallengeMixin, forms.Form): super().__init__(*args, **kwargs) self.request = request self.user_cache = None - self.include_challange() + self.include_challenge() def clean(self): address = self.cleaned_data.get("address") @@ -99,7 +106,7 @@ class SimpleSignUpForm(ChallengeMixin, forms.Form): must be created.""" super().__init__(*args, **kwargs) self.request = request - self.include_challange() + self.include_challenge() self.network = None def clean_address(self): diff --git a/django_cryptolock/managers.py b/django_cryptolock/managers.py new file mode 100644 index 0000000..6b7914e --- /dev/null +++ b/django_cryptolock/managers.py @@ -0,0 +1,32 @@ +from datetime import timedelta + +from django.db.models.manager import Manager +from django.conf import settings +from django.utils import timezone + +from .utils import generate_challenge + + +class ChallengeManager(Manager): + """Provides methods to easily create and verify challenges.""" + + def generate(self): + token = generate_challenge() + age = getattr(settings, "DJCL_CHALLENGE_EXPIRATION", 10) + expiry_date = timezone.now() + timedelta(minutes=age) + return self.create(challenge=token, expires=expiry_date) + + def is_active(self, challenge): + """Returns True if the challenge can be used. Otherwise False.""" + now = timezone.now() + return self.filter(challenge=challenge, expires__gte=now).exists() + + def invalidate(self, challenge): + """Removes the provided challenge if it exists.""" + self.filter(challenge=challenge).delete() + + def clean_expired(self): + """Delete all expired challenges. Returns nº of entries removed.""" + now = timezone.now() + del_summary = self.filter(expires__lt=now).delete() + return del_summary[0] diff --git a/django_cryptolock/migrations/0003_challenge.py b/django_cryptolock/migrations/0003_challenge.py new file mode 100644 index 0000000..14db969 --- /dev/null +++ b/django_cryptolock/migrations/0003_challenge.py @@ -0,0 +1,29 @@ +# Generated by Django 2.2.5 on 2020-05-12 11:22 + +from django.db import migrations, models +import django.utils.timezone +import model_utils.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('django_cryptolock', '0002_auto_20200218_1312'), + ] + + operations = [ + migrations.CreateModel( + name='Challenge', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('created', model_utils.fields.AutoCreatedField(default=django.utils.timezone.now, editable=False, verbose_name='created')), + ('modified', model_utils.fields.AutoLastModifiedField(default=django.utils.timezone.now, editable=False, verbose_name='modified')), + ('challenge', models.CharField(max_length=150)), + ('expires', models.DateTimeField()), + ], + options={ + 'verbose_name': 'Challenge', + 'verbose_name_plural': 'Challenges', + }, + ), + ] diff --git a/django_cryptolock/models.py b/django_cryptolock/models.py index 6676408..fbe1213 100644 --- a/django_cryptolock/models.py +++ b/django_cryptolock/models.py @@ -7,6 +7,7 @@ from django.core.exceptions import ValidationError from model_utils.models import TimeStampedModel from .validators import validate_monero_address, validate_bitcoin_address +from .managers import ChallengeManager class Address(TimeStampedModel): @@ -39,3 +40,22 @@ class Address(TimeStampedModel): validate_bitcoin_address(self.address) except ValidationError: raise ValidationError(_("Invalid address for the given network")) + + +class Challenge(TimeStampedModel): + """Challenges provided to users for authentication purposes.""" + + challenge = models.CharField(max_length=150) + expires = models.DateTimeField(null=False) + + objects = ChallengeManager() + + class Meta: + """Meta definition for Challenge.""" + + verbose_name = _("Challenge") + verbose_name_plural = _("Challenges") + + def __str__(self): + """Unicode representation of Challenge.""" + return self.challenge diff --git a/django_cryptolock/views.py b/django_cryptolock/views.py index 0b4c461..d157f6c 100644 --- a/django_cryptolock/views.py +++ b/django_cryptolock/views.py @@ -10,13 +10,20 @@ from monerorpc.authproxy import JSONRPCException from .forms import SimpleSignUpForm, SimpleLoginForm from .utils import verify_monero_signature, verify_bitcoin_signature -from .models import Address +from .models import Address, Challenge class CryptoLockLoginView(LoginView): template_name = "django_cryptolock/login.html" form_class = SimpleLoginForm + def form_valid(self, form): + response = super().form_valid(form) + challenge = form.cleaned_data["challenge"] + Challenge.objects.invalidate(challenge) + Challenge.objects.clean_expired() + return response + class CryptoLockSignUpView(FormView): template_name = "django_cryptolock/signup.html" @@ -36,10 +43,12 @@ class CryptoLockSignUpView(FormView): username = form.cleaned_data["username"] address = form.cleaned_data["address"] + challenge = form.cleaned_data["challenge"] if valid_sig: user = get_user_model().objects.create(username=username) user.address_set.create(address=address, network=form.network) + Challenge.objects.invalidate(challenge) return super().form_valid(form) else: form._errors["signature"] = ErrorList([_("Invalid signature")]) diff --git a/docs/django_cryptolock.migrations.rst b/docs/django_cryptolock.migrations.rst index ad94b23..30d5e59 100644 --- a/docs/django_cryptolock.migrations.rst +++ b/docs/django_cryptolock.migrations.rst @@ -20,6 +20,14 @@ django\_cryptolock.migrations.0002\_auto\_20200218\_1312 module :undoc-members: :show-inheritance: +django\_cryptolock.migrations.0003\_challenge module +---------------------------------------------------- + +.. automodule:: django_cryptolock.migrations.0003_challenge + :members: + :undoc-members: + :show-inheritance: + Module contents --------------- diff --git a/docs/usage.rst b/docs/usage.rst index 6f1e4c1..66371ac 100644 --- a/docs/usage.rst +++ b/docs/usage.rst @@ -47,6 +47,8 @@ Optional Configuration default is ``16`` and you should avoid lower values unless you know what you are doing. +``DJCL_CHALLENGE_EXPIRATION`` can be used to control how long a challenge is +valid. The default value is `10` minutes. Using the default forms and views --------------------------------- diff --git a/tests/test_forms.py b/tests/test_forms.py index 3985270..60d00b4 100644 --- a/tests/test_forms.py +++ b/tests/test_forms.py @@ -1,12 +1,15 @@ from unittest.mock import MagicMock, patch +from datetime import timedelta from django.contrib.auth import get_user_model +from django.utils import timezone import pytest from model_mommy import mommy +from pybitid import bitid from django_cryptolock.forms import SimpleLoginForm, SimpleSignUpForm -from django_cryptolock.models import Address +from django_cryptolock.models import Address, Challenge from .helpers import set_monero_settings, set_bitcoin_settings @@ -14,77 +17,120 @@ pytestmark = pytest.mark.django_db VALID_MONERO_ADDRESS = "46fYuhPAdsxMbEeMg97LhSbFPamdiCw7C6b19VEcZSmV6xboWFZuZQ9MTbj1wLszhUExHi63CMtsWjDTrRDqegZiPVebgYq" VALID_BITCOIN_ADDRESS = "1N5attoW1FviYGnLmRu9xjaPMKTkWxtUCW" +FUTURE_TIME = timezone.now() + timedelta(minutes=15) User = get_user_model() +def gen_challenge(request, challenge): + return bitid.build_uri(request.build_absolute_uri(), challenge) + + def test_simpleloginform_generates_new_challenge(): request = MagicMock() - initial = {} - request.session.__setitem__.side_effect = initial.__setitem__ - request.session.__getitem__.side_effect = initial.__getitem__ request.build_absolute_uri.return_value = "http://something/" + assert not Challenge.objects.all().exists() + form = SimpleLoginForm(request=request) + challenge = Challenge.objects.first() assert form.initial.get("challenge") - assert initial["current_challenge"] == form.initial.get("challenge") + assert form.initial.get("challenge") == gen_challenge(request, challenge.challenge) assert form.initial.get("challenge").startswith("bitid://something") def test_simpleloginform_generates_no_new_challenge(): request = MagicMock() - initial = {} - request.session.__setitem__.side_effect = initial.__setitem__ - request.session.__getitem__.side_effect = initial.__getitem__ request.build_absolute_uri.return_value = "http://something/" + assert not Challenge.objects.all().exists() + form = SimpleLoginForm(request=request, data={"address": ""}) + assert not Challenge.objects.all().exists() assert not form.initial.get("challenge") - assert not initial.get("current_challenge") @pytest.mark.django_db def test_simpleloginform_valid_data(settings): settings.DJCL_MONERO_NETWORK = "mainnet" + mommy.make(Challenge, challenge="12345678", expires=FUTURE_TIME) + request = MagicMock() + request.build_absolute_uri.return_value = "http://something/" + + form = SimpleLoginForm( + request=request, + data={ + "address": VALID_MONERO_ADDRESS, + "challenge": gen_challenge(request, "12345678"), + "signature": "some valid signature", + }, + ) + with patch("django_cryptolock.forms.authenticate") as auth_mock: + auth_mock.return_value = mommy.make(User) + assert form.is_valid() + + +@pytest.mark.django_db +def test_simpleloginform_invalid_challenge(settings): + settings.DJCL_MONERO_NETWORK = "mainnet" + mommy.make(Challenge, challenge="12345678", expires=FUTURE_TIME) request = MagicMock() request.build_absolute_uri.return_value = "http://something/" form = SimpleLoginForm( request=request, data={ "address": VALID_MONERO_ADDRESS, - "challenge": "12345678", + "challenge": gen_challenge(request, "1234567"), "signature": "some valid signature", }, ) with patch("django_cryptolock.forms.authenticate") as auth_mock: auth_mock.return_value = mommy.make(User) - request.session.get.return_value = "12345678" - assert form.is_valid() + assert not form.is_valid() + + +@pytest.mark.django_db +def test_simpleloginform_expired_challenge(settings): + settings.DJCL_MONERO_NETWORK = "mainnet" + mommy.make(Challenge, challenge="12345678", expires=timezone.now()) + request = MagicMock() + request.build_absolute_uri.return_value = "http://something/" + form = SimpleLoginForm( + request=request, + data={ + "address": VALID_MONERO_ADDRESS, + "challenge": gen_challenge(request, "12345678"), + "signature": "some valid signature", + }, + ) + with patch("django_cryptolock.forms.authenticate") as auth_mock: + auth_mock.return_value = mommy.make(User) + assert not form.is_valid() def test_simplesignupform_generates_new_challenge(): request = MagicMock() - initial = {} - request.session.__setitem__.side_effect = initial.__setitem__ - request.session.__getitem__.side_effect = initial.__getitem__ request.build_absolute_uri.return_value = "http://something/" + assert not Challenge.objects.all().exists() + form = SimpleSignUpForm(request=request) + challenge = Challenge.objects.first() assert form.initial.get("challenge") - assert initial["current_challenge"] == form.initial.get("challenge") + assert form.initial.get("challenge") == gen_challenge(request, challenge.challenge) assert form.initial.get("challenge").startswith("bitid://something") def test_simplesignupform_generates_no_new_challenge(): request = MagicMock() - initial = {} - request.session.__setitem__.side_effect = initial.__setitem__ - request.session.__getitem__.side_effect = initial.__getitem__ request.build_absolute_uri.return_value = "http://something/" + assert not Challenge.objects.all().exists() + form = SimpleSignUpForm(request=request, data={"address": ""}) + assert not Challenge.objects.all().exists() assert not form.initial.get("challenge") - assert not initial.get("current_challenge") def test_validate_address_unique(settings): settings.DJCL_MONERO_NETWORK = "mainnet" mommy.make(Address, address=VALID_MONERO_ADDRESS) + mommy.make(Challenge, challenge="12345678", expires=FUTURE_TIME) request = MagicMock() request.build_absolute_uri.return_value = "http://something/" form = SimpleSignUpForm( @@ -92,7 +138,7 @@ def test_validate_address_unique(settings): data={ "username": "foo", "address": VALID_MONERO_ADDRESS, - "challenge": "12345678", + "challenge": gen_challenge(request, "12345678"), "signature": "some valid signature", }, ) @@ -100,17 +146,18 @@ def test_validate_address_unique(settings): assert "This address already exists" in form.errors["address"] -def test_simplesignupform_validate_bitcoin_addr(settings): +def test_simplesignupform_valid_bitcoin_addr(settings): set_bitcoin_settings(settings) + mommy.make(Challenge, challenge="12345678", expires=FUTURE_TIME) + request = MagicMock() request.build_absolute_uri.return_value = "http://something/" - request.session.get.return_value = "12345678" form = SimpleSignUpForm( request=request, data={ "username": "foo", "address": VALID_BITCOIN_ADDRESS, - "challenge": "12345678", + "challenge": gen_challenge(request, "12345678"), "signature": "some valid signature", }, ) @@ -119,23 +166,25 @@ def test_simplesignupform_validate_bitcoin_addr(settings): def test_simplesignupform_valid_monero_addr(settings): set_monero_settings(settings) + mommy.make(Challenge, challenge="12345678", expires=FUTURE_TIME) + settings.DJCL_MONERO_NETWORK = "mainnet" request = MagicMock() request.build_absolute_uri.return_value = "http://something/" - request.session.get.return_value = "12345678" form = SimpleSignUpForm( request=request, data={ "username": "foo", "address": VALID_MONERO_ADDRESS, - "challenge": "12345678", + "challenge": gen_challenge(request, "12345678"), "signature": "some valid signature", }, ) assert form.is_valid() -def test_simplesignupform_validate_invalid_addr(): +def test_simplesignupform_invalid_addr(): + mommy.make(Challenge, challenge="12345678", expires=FUTURE_TIME) request = MagicMock() request.build_absolute_uri.return_value = "http://something/" form = SimpleSignUpForm( @@ -143,9 +192,17 @@ def test_simplesignupform_validate_invalid_addr(): data={ "username": "foo", "address": "bad addr", - "challenge": "12345678", + "challenge": gen_challenge(request, "12345678"), "signature": "some valid signature", }, ) assert not form.is_valid() assert "Invalid address" in form.errors["address"] + + +# def test_simplesignupform_invalid_challenge(): +# pass + + +# def test_simple_signupform_expired_challenge(): +# pass diff --git a/tests/test_managers.py b/tests/test_managers.py new file mode 100644 index 0000000..14e25db --- /dev/null +++ b/tests/test_managers.py @@ -0,0 +1,54 @@ +from datetime import timedelta + +from django.utils import timezone + +import pytest +from model_mommy import mommy + +from django_cryptolock.models import Challenge + +pytestmark = pytest.mark.django_db + + +class TestChallengeManager: + @pytest.mark.parametrize("conf", (None, 5, 10, 15, 20, 30, 60)) + def test_generate_challenge_with_expiration(self, settings, conf): + if conf: + settings.DJCL_CHALLENGE_EXPIRATION = conf + test = timezone.now() + timedelta(minutes=conf + 1) + else: + test = timezone.now() + timedelta(minutes=11) + + assert not Challenge.objects.all().exists() + + challenge = Challenge.objects.generate() + assert challenge.challenge + assert challenge.expires < test + assert Challenge.objects.all().exists() + + def test_is_active_when_expired(self): + challenge = mommy.make(Challenge, challenge="1234", expires=timezone.now()) + assert not Challenge.objects.is_active(challenge=challenge.challenge) + + def test_is_active_when_inexistent(self): + assert not Challenge.objects.is_active(challenge="1234") + + def test_is_active(self): + challenge = Challenge.objects.generate() + assert Challenge.objects.is_active(challenge=challenge.challenge) + + def test_invalidate_existing_challenge(self): + challenge = Challenge.objects.generate() + Challenge.objects.invalidate(challenge.challenge) + assert not Challenge.objects.all().exists() + + def test_invalidate_inexistent_challenge(self): + Challenge.objects.invalidate("1234") + + @pytest.mark.parametrize("num", (2, 5, 10, 15)) + def test_clean_expired_challenges(self, num): + mommy.make(Challenge, num, expires=timezone.now()) + Challenge.objects.generate() + deleted = Challenge.objects.clean_expired() + assert deleted == num + assert Challenge.objects.count() == 1