Merge pull request #521 from rhenanbartels/fix/auth-backend

Fix/auth backend

.github/workflows/django.yml

@@ -28,22 +28,18 @@ jobs:
         ENV_FILE: .env
         PROJECT_NAME: brasil.io
         DOCKER_COMPOSE_FILE: docker-compose.yml
-    - name: Install Dependencies
-      run: |
-        python -m pip install --upgrade pip
-        pip install -I -r requirements-development.txt
     - name: Migrate and Update Data
       run: |
-        python manage.py migrate --no-input
+        docker-compose run web python manage.py migrate --no-input
     - name: Collect static staticfiles
       run: |
-        python manage.py collectstatic --no-input
+        docker-compose run web python manage.py collectstatic --no-input
     - name: Run Tests
       run: |
-        pytest
+        docker-compose run web pytest
     - name: Run black
       run: |
-        black . --exclude "docker" -l 120 --check
+        docker-compose run web black . --exclude "docker" -l 120 --check
     - name: Run flake8
       run: |
-        flake8 --config setup.cfg
+        docker-compose run web flake8 --config setup.cfg

Makefile

@@ -2,7 +2,7 @@ lint:
 	autoflake --in-place --recursive --remove-unused-variables --remove-all-unused-imports .
 	isort --skip migrations --skip brasilio/wsgi.py -rc .
 	black . --exclude "docker" -l 120
-	flake8
+	flake8 --config setup.cfg
 
 test:
 	pytest

api/admin.py

@@ -1,10 +1,7 @@
 from django.contrib import admin
-from django.contrib.auth import get_user_model
 
 from api.models import Token
 
-User = get_user_model()
-
 
 class TokenAdmin(admin.ModelAdmin):
     list_display = ("key", "user", "created")

api/tests/test_views.py

@@ -153,7 +153,11 @@ def test_redirects(self):
         assert "/api/v1/datasets/" == path_assertions[0][1]
 
     def test_redirects_from_api_host(self):
-        self.client.force_login(baker.make(User))
+        settings.ALLOWED_HOSTS.append(settings.BRASILIO_API_HOST)
+        user = baker.make(User, is_active=True)
+        self.client.force_login(user)
+        token = baker.make("api.Token", user=user)
+        auth_header = {"HTTP_AUTHORIZATION": f"Token {token.key}"}
 
         urlconf = settings.API_ROOT_URLCONF
         path_assertions = [
@@ -170,7 +174,7 @@ def test_redirects_from_api_host(self):
         ]
 
         for url, redirect_url in path_assertions:
-            response = self.client.get(url, HTTP_HOST=settings.BRASILIO_API_HOST)
+            response = self.client.get(url, HTTP_HOST=settings.BRASILIO_API_HOST, **auth_header,)
             self.assertRedirects(response, redirect_url, msg_prefix=url, fetch_redirect_response=False, status_code=301)
 
         assert "/datasets/" == path_assertions[0][0]

brasilio/settings.py

@@ -1,5 +1,3 @@
-from urllib.parse import urlparse
-
 import environ
 import sentry_sdk
 from django.urls import reverse_lazy

brasilio_auth/auth_backend.py

@@ -10,10 +10,9 @@ def authenticate(self, request, username=None, password=None, *args, **kwargs):
         if username is None:
             return None
         elif "@" in username:
-            # Won't allow anybody having "@" in username to log in
-            query = Q(email=username)
+            query = Q(email__iexact=username.strip())
         else:
-            query = Q(username=username)
+            query = Q(username__iexact=username.strip())
 
         try:
             user = User.objects.get(query)

brasilio_auth/forms.py

@@ -1,10 +1,20 @@
+import re
+
 from django import forms
 from django.contrib.auth import get_user_model
 from django.utils.translation import gettext_lazy as _
 from django_registration.forms import RegistrationFormUniqueEmail
 
 from utils.forms import FlagedReCaptchaField as ReCaptchaField
 
+USERNAME_REGEXP = re.compile(r"[^A-Za-z0-9_]")
+PUNCT_REGEXP = re.compile("[-/ .]")
+User = get_user_model()
+
+
+def is_valid_username(username):
+    return not (PUNCT_REGEXP.sub("", username).isdigit() or USERNAME_REGEXP.search(username))
+
 
 class UserCreationForm(RegistrationFormUniqueEmail):
     username = forms.CharField(widget=forms.TextInput(attrs={"style": "text-transform: lowercase"}),)
@@ -23,14 +33,19 @@ class Meta:
         fields = ("username", "email")
 
     def clean_username(self):
-        username = self.cleaned_data.get("username", "")
-        return username.lower()
+        username = self.cleaned_data.get("username", "").strip()
+        if not is_valid_username(username):
+            raise forms.ValidationError(
+                "Nome de usuário pode conter apenas letras, números e '_' e não deve ser um documento"
+            )
+        elif username and User.objects.filter(username__iexact=username).exists():
+            raise forms.ValidationError("Nome de usuário já existente (escolha um diferente).")
+        return username
 
     def clean_email(self):
         email = self.cleaned_data.get("email")
-        if email:
-            if get_user_model().objects.filter(email__iexact=email).exists():
-                raise forms.ValidationError(f"Usuário com o email {email} já cadastrado.")
+        if email and User.objects.filter(email__iexact=email).exists():
+            raise forms.ValidationError(f"Usuário com o email {email} já cadastrado.")
         return email
 
 

brasilio_auth/scripts/migrate_wrong_usernames.py

@@ -0,0 +1,75 @@
+"""
+Anteriormente, os usuários podiam cadastrar seu username utilizando o caractere
+inválido '@'.
+Este script corrige esse problema alterando os usernames que contem '@' por
+variações únicas baseadas no nome de usuário e/ou no e-mail de cada registro.
+
+Exemplo:
+    old_username = 'name@'
+    new_username = 'name'
+"""
+import csv
+import string
+from typing import Tuple
+
+from django.contrib.auth import get_user_model
+from rows.utils import slug
+
+from brasilio_auth.forms import is_valid_username
+
+User = get_user_model()
+possible_chars = string.ascii_letters + string.digits + "_"
+
+
+def possible_usernames(username: str, email: str, n_suffix: int = 10) -> Tuple[str, ...]:
+    username = username.strip()
+    email = email.strip()
+
+    if username.lower() == email.lower():
+        username = username.split("@")[0]
+    else:
+        if username.startswith("@"):
+            username = username[1:]
+        if username.endswith("@"):
+            username = username[:-1]
+        if "@" in username:
+            stop = username.find("@")
+            before, after = username[:stop], username[stop + 1 :]
+            if "." in after:
+                username = before
+            else:
+                username = "_".join(username.split("@"))
+
+    email_parts = email.split("@")
+    possible_2 = email_parts[0]
+    possible_3 = email_parts[0] + "_" + email_parts[1].split(".")[0]
+    possible_with_suffix = [f"{username}_{i}" for i in range(1, n_suffix)]
+    return (username, possible_2, possible_3, *possible_with_suffix)
+
+
+def migrate_usernames(filepath):
+    with open(filepath, mode="w") as fobj:
+        writer = csv.DictWriter(fobj, fieldnames=["old_username", "new_username", "email"])
+        writer.writeheader()
+        for user in User.objects.all():
+            if is_valid_username(user.username):
+                continue
+
+            # Define possible usernames based on current and remove any
+            # non-allowed chars
+            possible = [
+                slug(username, permitted_chars=possible_chars)
+                for username in possible_usernames(user.username, user.email)
+            ]
+            for username in possible:
+                if not User.objects.filter(username=username).exists():
+                    writer.writerow({"old_username": user.username, "new_username": username, "email": user.email})
+                    user.username = username
+                    user.save()
+                    break
+            print(f"ERROR: could not migrate {user} (tried: {', '.join(possible)})")
+
+
+def run():
+    filepath = "/data/fixed-usernames.csv"
+    migrate_usernames(filepath)

brasilio_auth/templates/brasilio_auth/login.html

@@ -7,9 +7,18 @@
       <h4>Login</h4>
 
       {% if form.errors %}
-        <p class="red-text text-lighten-3">
-          Senha inválida. Por favor tente novamente.
-        </p>
+          {% for field in form %}
+              {% for error in field.errors %}
+                  <p class="red-text text-lighten-3">
+                      {{ error|escape }}
+                  </p>
+              {% endfor %}
+          {% endfor %}
+          {% for error in form.non_field_errors %}
+              <p class="red-text text-lighten-3">
+                  {{ error|escape }}
+              </p>
+          {% endfor %}
       {% endif %}
 
       {% if next %}

brasilio_auth/tests/test_forms.py

@@ -11,6 +11,10 @@
 
 
 class UserCreationFormTests(TestCase):
+    username_invalid_error = "Nome de usuário pode conter apenas letras, números e '_' e não deve ser um documento"
+    username_max_length_error = "Certifique-se de que o valor tenha no máximo 150 caracteres (ele possui 151)."
+    username_exists_error = "Nome de usuário já existente (escolha um diferente)."
+
     def test_required_fields(self):
         required_fields = ["username", "email", "password1", "password2", "captcha"]
 
@@ -42,30 +46,12 @@ def test_create_user(self):
         assert user.check_password(passwd) is True
         assert not NewsletterSubscriber.objects.filter(user=user).exists()
 
-    @patch.object(ReCaptchaField, "validate", Mock(return_value=True))
-    def test_force_lower_for_username(self):
-        passwd = "verygoodpassword"
-        data = {
-            "username": "FOO",
-            "email": "[email protected]",
-            "password1": passwd,
-            "password2": passwd,
-            "captcha": "captcha-validation",
-        }
-
-        form = UserCreationForm(data)
-        assert form.is_valid() is True
-        user = form.save()
-        user.refresh_from_db()
-
-        assert "foo" == user.username
-
     @patch.object(ReCaptchaField, "validate", Mock(return_value=True))
     def test_respect_abstract_user_max_length_for_username(self):
         passwd = "verygoodpassword"
         data = {
             "username": "a" * 150,
-            "email": "foo@bar.com",
+            "email": "another_foo@bar.com",
             "password1": passwd,
             "password2": passwd,
             "captcha": "captcha-validation",
@@ -78,14 +64,16 @@ def test_respect_abstract_user_max_length_for_username(self):
         form = UserCreationForm(data)
         assert not form.is_valid()
         assert "username" in form.errors
+        assert "email" not in form.errors
+        assert form.errors["username"] == [self.username_max_length_error]
 
     @patch.object(ReCaptchaField, "validate", Mock(return_value=True))
     def test_invalid_username_if_already_exists(self):
         baker.make(settings.AUTH_USER_MODEL, username="foo")
         passwd = "verygoodpassword"
         data = {
             "username": "foo",
-            "email": "foo@bar.com",
+            "email": "another_foo@bar.com",
             "password1": passwd,
             "password2": passwd,
             "captcha": "captcha-validation",
@@ -94,13 +82,15 @@ def test_invalid_username_if_already_exists(self):
         form = UserCreationForm(data)
         assert form.is_valid() is False
         assert "username" in form.errors
+        assert "email" not in form.errors
+        assert form.errors["username"] == [self.username_exists_error]
 
     @patch.object(ReCaptchaField, "validate", Mock(return_value=True))
     def test_invalid_email_if_user_already_exists(self):
-        baker.make(settings.AUTH_USER_MODEL, email="[email protected]")
+        baker.make(settings.AUTH_USER_MODEL, email="[email protected]", username="foo")
         passwd = "verygoodpassword"
         data = {
-            "username": "foo",
+            "username": "another_foo",
             "email": "[email protected]",
             "password1": passwd,
             "password2": passwd,
@@ -110,13 +100,14 @@ def test_invalid_email_if_user_already_exists(self):
         form = UserCreationForm(data)
         assert not form.is_valid()
         assert "email" in form.errors
+        assert "username" not in form.errors
 
     @patch.object(ReCaptchaField, "validate", Mock(return_value=True))
     def test_email_validation_does_not_break_if_different_letter_case(self):
-        baker.make(settings.AUTH_USER_MODEL, email="[email protected]")
+        baker.make(settings.AUTH_USER_MODEL, email="[email protected]", username="foo")
         passwd = "verygoodpassword"
         data = {
-            "username": "foo",
+            "username": "another_foo",
             "email": "[email protected]",
             "password1": passwd,
             "password2": passwd,
@@ -126,6 +117,7 @@ def test_email_validation_does_not_break_if_different_letter_case(self):
         form = UserCreationForm(data)
         assert not form.is_valid()
         assert "email" in form.errors
+        assert "username" not in form.errors
 
     def test_do_not_validate_if_bad_captcha(self):
         passwd = "verygoodpassword"
@@ -142,6 +134,54 @@ def test_do_not_validate_if_bad_captcha(self):
         assert not form.is_valid()
         assert "captcha" in form.errors
 
+    @patch.object(ReCaptchaField, "validate", Mock(return_value=True))
+    def test_invalid_username_characters(self):
+        passwd = "verygoodpassword"
+        data = {
+            "username": "foo@",
+            "email": "[email protected]",
+            "password1": passwd,
+            "password2": passwd,
+            "captcha": "captcha-validation",
+        }
+
+        form = UserCreationForm(data)
+        assert not form.is_valid()
+        assert "username" in form.errors
+        assert form.errors["username"] == [self.username_invalid_error]
+
+    @patch.object(ReCaptchaField, "validate", Mock(return_value=True))
+    def test_invalid_username_digits(self):
+        passwd = "verygoodpassword"
+        data = {
+            "username": "123.456.789-01",
+            "email": "[email protected]",
+            "password1": passwd,
+            "password2": passwd,
+            "captcha": "captcha-validation",
+        }
+
+        form = UserCreationForm(data)
+        assert not form.is_valid()
+        assert "username" in form.errors
+        assert form.errors["username"] == [self.username_invalid_error]
+
+    @patch.object(ReCaptchaField, "validate", Mock(return_value=True))
+    def test_do_not_set_username_to_lowercase(self):
+        passwd = "verygoodpassword"
+        username = "Foo"
+        data = {
+            "username": username,
+            "email": "[email protected]",
+            "password1": passwd,
+            "password2": passwd,
+            "captcha": "captcha-validation",
+        }
+
+        form = UserCreationForm(data)
+        assert form.is_valid()
+        assert form.cleaned_data["username"] == username
+
 
 class TestTokenApiManagementForm(TestCase):
     def test_required_fields(self):