Skip to content

Commit

Permalink
[bandit] Use the secrets module for activation keys
Browse files Browse the repository at this point in the history
Issue: [B311:blacklist] Standard pseudo-random generators are not
       suitable for security/cryptographic purposes.

Fix: instead of using random.random() use secrets.token_hex() and
     avoid using hashing the salt then hashing salt+username!

NEW DB MIGRATION:
    - increases activation_key field size to 64 chars!
  • Loading branch information
atodorov committed Apr 13, 2018
1 parent e17c071 commit e45dde2
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 27 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Generated by Django 2.0.4 on 2018-04-12 11:56

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('tcms.core.contrib.auth', '0001_initial'),
]

operations = [
migrations.AlterField(
model_name='useractivatekey',
name='activation_key',
field=models.CharField(blank=True, max_length=64, null=True),
),
]
9 changes: 3 additions & 6 deletions tcms/core/contrib/auth/models.py
Original file line number Diff line number Diff line change
@@ -1,26 +1,23 @@
# -*- coding: utf-8 -*-

import datetime
import random
import secrets

from django.db import models
from django.conf import settings

from tcms.core.utils.checksum import checksum


class UserActivateKey(models.Model):
user = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.CASCADE)
activation_key = models.CharField(max_length=40, null=True, blank=True)
activation_key = models.CharField(max_length=64, null=True, blank=True)
key_expires = models.DateTimeField(null=True, blank=True)

class Meta:
db_table = u'tcms_user_activate_keys'

@classmethod
def set_random_key_for_user(cls, user, force=False):
salt = checksum(str(random.random()))[:5]
activation_key = checksum(salt + user.username)
activation_key = secrets.token_hex()

# Create and save their profile
user_activation_key, created = cls.objects.get_or_create(user=user)
Expand Down
35 changes: 14 additions & 21 deletions tcms/core/contrib/auth/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
from django.test import TestCase, override_settings

from tcms import signals
from tcms.core.utils.checksum import checksum
from .models import UserActivateKey


Expand All @@ -29,23 +28,17 @@ def setUpTestData(cls):
password='password')

@patch('tcms.core.contrib.auth.models.datetime')
@patch('tcms.core.contrib.auth.models.random')
def test_set_random_key(self, random, mock_datetime):
mock_datetime.datetime.today.return_value = datetime.datetime(2017, 5, 10)
mock_datetime.timedelta.return_value = datetime.timedelta(7)
fake_random = 0.12345678
random.random.return_value = fake_random
def test_set_random_key(self, mock_datetime):
now = datetime.datetime.now()
in_7_days = datetime.timedelta(7)

activation_key = UserActivateKey.set_random_key_for_user(self.new_user)
mock_datetime.datetime.today.return_value = now
mock_datetime.timedelta.return_value = in_7_days

activation_key = UserActivateKey.set_random_key_for_user(self.new_user)
self.assertEqual(self.new_user, activation_key.user)

s_random = checksum(str(fake_random))[:5]
expected_key = checksum(s_random + self.new_user.username)
self.assertEqual(expected_key, activation_key.activation_key)

self.assertEqual(datetime.datetime(2017, 5, 17),
activation_key.key_expires)
self.assertNotEqual('', activation_key.activation_key)
self.assertEqual(now + in_7_days, activation_key.key_expires)


class TestForceToSetRandomKey(TestCase):
Expand Down Expand Up @@ -113,8 +106,8 @@ def test_open_registration_page(self):

def assert_user_registration(self, username, follow=False):

with patch('tcms.core.contrib.auth.models.checksum') as _checksum:
_checksum.return_value = self.fake_activate_key
with patch('tcms.core.contrib.auth.models.secrets') as _secrets:
_secrets.token_hex.return_value = self.fake_activate_key

response = self.client.post(self.register_url,
{'username': username,
Expand Down Expand Up @@ -228,8 +221,8 @@ def test_fail_if_activation_key_does_not_exist(self):
def test_fail_if_activation_key_expired(self):
fake_activation_key = 'secret-activation-key'

with patch('tcms.core.contrib.auth.models.checksum') as _checksum:
_checksum.return_value = fake_activation_key
with patch('tcms.core.contrib.auth.models.secrets') as _secrets:
_secrets.token_hex.return_value = fake_activation_key
key = UserActivateKey.set_random_key_for_user(self.new_user)
key.key_expires = datetime.datetime.now() - datetime.timedelta(days=10)
key.save()
Expand All @@ -246,8 +239,8 @@ def test_fail_if_activation_key_expired(self):
def test_confirm(self):
fake_activate_key = 'secret-activate-key'

with patch('tcms.core.contrib.auth.models.checksum') as _checksum:
_checksum.return_value = fake_activate_key
with patch('tcms.core.contrib.auth.models.secrets') as _secrets:
_secrets.token_hex.return_value = fake_activate_key
UserActivateKey.set_random_key_for_user(self.new_user)

confirm_url = reverse('tcms-confirm',
Expand Down

0 comments on commit e45dde2

Please sign in to comment.