diff --git a/.github/workflows/django.yml b/.github/workflows/django.yml index f1bd9f69..1c2bd783 100644 --- a/.github/workflows/django.yml +++ b/.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 diff --git a/Makefile b/Makefile index 93588be9..9c28a5f3 100644 --- a/Makefile +++ b/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 diff --git a/api/admin.py b/api/admin.py index a849b138..44e92cbc 100644 --- a/api/admin.py +++ b/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") diff --git a/api/tests/test_views.py b/api/tests/test_views.py index cb428d85..5a99e8b1 100644 --- a/api/tests/test_views.py +++ b/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] diff --git a/brasilio/settings.py b/brasilio/settings.py index e2afa60f..c5983b6f 100644 --- a/brasilio/settings.py +++ b/brasilio/settings.py @@ -1,5 +1,3 @@ -from urllib.parse import urlparse - import environ import sentry_sdk from django.urls import reverse_lazy diff --git a/brasilio_auth/auth_backend.py b/brasilio_auth/auth_backend.py index 54929547..12bcd3d0 100644 --- a/brasilio_auth/auth_backend.py +++ b/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) diff --git a/brasilio_auth/forms.py b/brasilio_auth/forms.py index af6b20ea..ebd02cc0 100644 --- a/brasilio_auth/forms.py +++ b/brasilio_auth/forms.py @@ -1,3 +1,5 @@ +import re + from django import forms from django.contrib.auth import get_user_model from django.utils.translation import gettext_lazy as _ @@ -5,6 +7,14 @@ 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 diff --git a/brasilio_auth/scripts/migrate_wrong_usernames.py b/brasilio_auth/scripts/migrate_wrong_usernames.py new file mode 100644 index 00000000..234ca000 --- /dev/null +++ b/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) diff --git a/brasilio_auth/templates/brasilio_auth/login.html b/brasilio_auth/templates/brasilio_auth/login.html index d180a719..c61cffea 100644 --- a/brasilio_auth/templates/brasilio_auth/login.html +++ b/brasilio_auth/templates/brasilio_auth/login.html @@ -7,9 +7,18 @@
- Senha inválida. Por favor tente novamente. -
+ {% for field in form %} + {% for error in field.errors %} ++ {{ error|escape }} +
+ {% endfor %} + {% endfor %} + {% for error in form.non_field_errors %} ++ {{ error|escape }} +
+ {% endfor %} {% endif %} {% if next %} diff --git a/brasilio_auth/tests/test_forms.py b/brasilio_auth/tests/test_forms.py index 7a4225b6..717d59eb 100644 --- a/brasilio_auth/tests/test_forms.py +++ b/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": "foo@bar.com", - "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,6 +64,8 @@ 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): @@ -85,7 +73,7 @@ def test_invalid_username_if_already_exists(self): 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="foo@bar.com") + baker.make(settings.AUTH_USER_MODEL, email="foo@bar.com", username="foo") passwd = "verygoodpassword" data = { - "username": "foo", + "username": "another_foo", "email": "foo@bar.com", "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="foo@bar.com") + baker.make(settings.AUTH_USER_MODEL, email="foo@bar.com", username="foo") passwd = "verygoodpassword" data = { - "username": "foo", + "username": "another_foo", "email": "FOO@bar.com", "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": "foo@bar.com", + "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": "foo@bar.com", + "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": "foo@bar.com", + "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): diff --git a/brasilio_auth/tests/test_scripts.py b/brasilio_auth/tests/test_scripts.py new file mode 100644 index 00000000..42541cf4 --- /dev/null +++ b/brasilio_auth/tests/test_scripts.py @@ -0,0 +1,111 @@ +from tempfile import NamedTemporaryFile + +from django.contrib.auth import get_user_model +from django.test import TestCase +from model_bakery import baker + +from brasilio_auth.scripts.migrate_wrong_usernames import migrate_usernames, possible_usernames + +User = get_user_model() + + +class TestPossibleUsernames(TestCase): + def test_username_equal_to_email(self): + username = "test@example.com" + email = "test@example.com" + + possible_values = possible_usernames(username, email, n_suffix=2) + expected = ("test", "test", "test_example", "test_1") + + assert possible_values == expected + + def test_username_with_invalid_char_at_the_beginning(self): + username = "@test" + email = "test@example.com" + + possible_values = possible_usernames(username, email, n_suffix=2) + expected = ("test", "test", "test_example", "test_1") + + assert possible_values == expected + + def test_username_with_invalid_char_at_the_end(self): + username = "test@" + email = "test@example.com" + + possible_values = possible_usernames(username, email, n_suffix=2) + expected = ("test", "test", "test_example", "test_1") + + assert possible_values == expected + + def test_username_with_invalid_char_at_the_both_ends(self): + username = "@test@" + email = "test@example.com" + + possible_values = possible_usernames(username, email, n_suffix=2) + expected = ("test", "test", "test_example", "test_1") + + assert possible_values == expected + + def test_replace_invalid_char_with_undescore(self): + username = "test@123" + email = "test@example.com" + + possible_values = possible_usernames(username, email, n_suffix=2) + expected = ("test_123", "test", "test_example", "test_123_1") + + assert possible_values == expected + + def test_combine_email_with_domain_name(self): + username = "test@exampl.com" + email = "test@example.com" + + possible_values = possible_usernames(username, email, n_suffix=2) + expected = ("test", "test", "test_example", "test_1") + + assert possible_values == expected + + def test_create_more_possibilities_with_suffixes(self): + username = "test@exampl.com" + email = "test@example.com" + + possible_values = possible_usernames(username, email, n_suffix=3) + expected = ("test", "test", "test_example", "test_1", "test_2") + + assert possible_values == expected + + +class TestReplaceUsernameWithSuggestions(TestCase): + def setUp(self): + self.user_1 = baker.make(User, username="test@", email="test@example.com",) + self.expected_username_1 = "test" + + self.user_2 = baker.make(User, username="@test@", email="name@example.com") + # teste would already exists because of self.username_1 + self.expected_username_2 = "name" + + self.temp_file = NamedTemporaryFile(mode="r") + self.expected_csv = ( + "old_username,new_username,email\n" "test@,test,test@example.com\n" "@test@,name,name@example.com\n" + ) + + def test_happy_path_wrong_usernames_replacement(self): + migrate_usernames(filepath=self.temp_file.name) + + self.user_1.refresh_from_db() + self.user_2.refresh_from_db() + self.temp_file.seek(0) + + assert self.user_1.username == self.expected_username_1 + assert self.user_2.username == self.expected_username_2 + assert self.temp_file.read() == self.expected_csv + + def test_all_combinations_of_user_1_already_exists(self): + baker.make(User, username="test") + baker.make(User, username="test_example") + + migrate_usernames(self.temp_file.name) + self.expected_username_1 = "test_1" + + self.user_1.refresh_from_db() + + assert self.user_1.username == self.expected_username_1 diff --git a/brasilio_auth/tests/test_views.py b/brasilio_auth/tests/test_views.py index 650f0fe3..13088c79 100644 --- a/brasilio_auth/tests/test_views.py +++ b/brasilio_auth/tests/test_views.py @@ -76,11 +76,26 @@ def test_form_error_if_trying_to_create_user_with_existing_username(self): response = self.client.post(self.url, data=self.data) assert User.objects.filter(username="foo").exists() - self.data["username"] = "FOO" + new_username = "foo" + assert new_username == self.data["username"] # same capitalization + self.data["username"] = new_username + self.data["email"] = "new@foo.com" + response = self.client.post(self.url, data=self.data) + self.assertTemplateUsed(response, "brasilio_auth/user_creation_form.html") + assert bool(response.context["form"].errors) is True + + def test_form_error_if_trying_to_create_user_with_existing_username_uppercase(self): + response = self.client.post(self.url, data=self.data) + assert User.objects.filter(username="foo").exists() + + new_username = "FOO" + # same username, different capitalization + assert new_username.lower() == self.data["username"].lower() + assert new_username != self.data["username"] + self.data["username"] = new_username self.data["email"] = "new@foo.com" response = self.client.post(self.url, data=self.data) self.assertTemplateUsed(response, "brasilio_auth/user_creation_form.html") - print(response.context["form"].errors) assert bool(response.context["form"].errors) is True diff --git a/core/forms.py b/core/forms.py index bb485f51..b6fc79d4 100644 --- a/core/forms.py +++ b/core/forms.py @@ -44,55 +44,6 @@ def _get_name(obj, person_type): return obj.name -class TracePathForm(forms.Form): - TYPE_CHOICES = [("pessoa-fisica", "Pessoa Física (nome completo)"), ("pessoa-juridica", "Pessoa Jurídica (CNPJ)")] - - origin_type = forms.ChoiceField(choices=TYPE_CHOICES, required=True) - origin_identifier = forms.CharField(required=True) - - destination_type = forms.ChoiceField(choices=TYPE_CHOICES, required=True) - destination_identifier = forms.CharField(required=True) - - def clean(self): - cleaned_data = super().clean() - origin_type = cleaned_data.get("origin_type") - origin_identifier = cleaned_data.get("origin_identifier") - destination_type = cleaned_data.get("destination_type") - destination_identifier = cleaned_data.get("destination_identifier") - - origin_field = _resolve_field_by_type(origin_type) - destination_field = _resolve_field_by_type(destination_type) - origin = _get_obj(origin_field, origin_identifier, origin_type) - destination = _get_obj(destination_field, destination_identifier, destination_type) - - if origin is None: - self.add_error("origin_identifier", "Name/document not found") - else: - cleaned_data["origin_name"] = _get_name(origin, origin_type) - if destination is None: - self.add_error("destination_identifier", "Name/document not found") - else: - cleaned_data["destination_name"] = _get_name(destination, destination_type) - - return cleaned_data - - -class CompanyGroupsForm(forms.Form): - identifier = forms.CharField(required=True, label="CNPJ") - - def clean(self): - cleaned_data = super().clean() - identifier = cleaned_data["identifier"] - for char in "./-": - identifier = identifier.replace(char, "") - company = _get_obj("", identifier, "pessoa-juridica") - if not company: - self.add_error("identifier", "Document not found") - else: - cleaned_data["company"] = company - return cleaned_data - - class ContactForm(forms.Form): name = forms.CharField(required=True, label="Nome") email = forms.EmailField(required=True, label="E-mail") diff --git a/core/tests/test_views.py b/core/tests/test_views.py index 0a9bf0c7..1c9375dc 100644 --- a/core/tests/test_views.py +++ b/core/tests/test_views.py @@ -100,6 +100,7 @@ def test_enforce_rate_limit_if_flagged(self, mocked_ratelimit): @override_settings(RATELIMIT_RATE="0/s") @patch("traffic_control.decorators.ratelimit") def test_enforce_rate_limit_if_flagged_for_api(self, mocked_ratelimit): + settings.ALLOWED_HOSTS.append(settings.BRASILIO_API_HOST) urlconf = settings.API_ROOT_URLCONF self.url = reverse("v1:dataset-table-data", args=["sample", "sample_table"], urlconf=urlconf) response = self.client.get(self.url, HTTP_HOST=settings.BRASILIO_API_HOST) diff --git a/core/views_special.py b/core/views_special.py index 1aa35cba..63d73b56 100644 --- a/core/views_special.py +++ b/core/views_special.py @@ -8,7 +8,6 @@ from django.urls import reverse from core.data_models import EmpresaTableConfig -from core.forms import CompanyGroupsForm, TracePathForm from core.models import get_table, get_table_model cipher_suite = Fernet(settings.FERNET_KEY) diff --git a/env.example b/env.example index 7efdc033..8163e6e2 100644 --- a/env.example +++ b/env.example @@ -6,7 +6,7 @@ POSTGRES_PASSWORD=postgres POSTGRES_DB=brasilio # Django settings -ALLOWED_HOSTS=localhost +ALLOWED_HOSTS=localhost,api.brasil.io APP_HOST="localhost:8000" BLOCKED_AGENTS="Wget,curl,python-requests,Python-urllib" DATABASE_URL=postgres://postgres:postgres@db:5432/brasilio diff --git a/setup.cfg b/setup.cfg index f004f728..23876f59 100644 --- a/setup.cfg +++ b/setup.cfg @@ -1,4 +1,4 @@ [flake8] max-line-length = 120 exclude = .tox,.git,*/migrations/*,*/static/CACHE/*,docs,node_modules,docker/postgresql/data -ignore=I001,I003,I004,E231,E501 +ignore=I001,I003,I004,E231,E501,E203 diff --git a/static/css/base.css b/static/css/base.css index 8f6b56af..7c9fbf6e 100644 --- a/static/css/base.css +++ b/static/css/base.css @@ -202,3 +202,10 @@ a { color: #ee6e73; font-size: 1rem; } + +#id_username { + background: url('data:image/svg+xml;utf8,') no-repeat; + padding-left: 1.4rem !important; + width: 100%; + box-sizing: border-box !important; +} diff --git a/utils/rocketchat.py b/utils/rocketchat.py index d8f999f7..c97c4e05 100644 --- a/utils/rocketchat.py +++ b/utils/rocketchat.py @@ -27,7 +27,8 @@ def make_request(self, method, *args, **kwargs): response = getattr(requests, method)(*args, **kwargs) if response.status_code >= 400: raise RuntimeError( - f"HTTP {response.status_code} error when processing request to {response.url} (body: {response.content})" + f"HTTP {response.status_code} error when processing request " + f"to {response.url} (body: {response.content})" ) return response