From 4365c32703a13423895fa6a4578d045096f72f08 Mon Sep 17 00:00:00 2001 From: rafalp Date: Sat, 9 Dec 2023 22:15:56 +0100 Subject: [PATCH 1/2] WIP replace old oauth2 hooks with new ones --- misago/hooks.py | 3 - misago/oauth2/hooks/__init__.py | 2 + misago/oauth2/hooks/filter_user_data.py | 47 ++++++ misago/oauth2/hooks/validate_user_data.py | 52 +++++++ misago/oauth2/tests/conftest.py | 6 + misago/oauth2/tests/test_user_data_filter.py | 118 --------------- .../oauth2/tests/test_user_data_validation.py | 137 +++++------------- misago/oauth2/validation.py | 40 ++--- misago/plugins/outlets.py | 2 + 9 files changed, 163 insertions(+), 244 deletions(-) create mode 100644 misago/oauth2/hooks/__init__.py create mode 100644 misago/oauth2/hooks/filter_user_data.py create mode 100644 misago/oauth2/hooks/validate_user_data.py create mode 100644 misago/oauth2/tests/conftest.py diff --git a/misago/hooks.py b/misago/hooks.py index 9a32027982..e1905c0ed9 100644 --- a/misago/hooks.py +++ b/misago/hooks.py @@ -4,9 +4,6 @@ new_registrations_validators = [] -oauth2_user_data_filters = [] -oauth2_validators = [] - post_search_filters = [] post_validators = [] diff --git a/misago/oauth2/hooks/__init__.py b/misago/oauth2/hooks/__init__.py new file mode 100644 index 0000000000..fa3dae88d9 --- /dev/null +++ b/misago/oauth2/hooks/__init__.py @@ -0,0 +1,2 @@ +from .filter_user_data import filter_user_data_hook +from .validate_user_data import validate_user_data_hook diff --git a/misago/oauth2/hooks/filter_user_data.py b/misago/oauth2/hooks/filter_user_data.py new file mode 100644 index 0000000000..efe508d7bf --- /dev/null +++ b/misago/oauth2/hooks/filter_user_data.py @@ -0,0 +1,47 @@ +from typing import Optional, Protocol + +from django.http import HttpRequest + +from ...plugins.hooks import FilterHook +from ...users.models import User + + +class FilterUserDataHookAction(Protocol): + def __call__( + self, + request: HttpRequest, + user: Optional[User], + user_data: dict, + ) -> dict: + pass + + +class FilterUserDataHookFilter(Protocol): + def __call__( + self, + action: FilterUserDataHookAction, + request: HttpRequest, + user: Optional[User], + user_data: dict, + ) -> dict: + pass + + +class FilterUserDataHook( + FilterHook[FilterUserDataHookAction, FilterUserDataHookFilter] +): + __slots__ = FilterHook.__slots__ + + def __call__( + self, + action: FilterUserDataHookAction, + request: HttpRequest, + user: Optional[User], + user_data: dict, + *args, + **kwargs, + ) -> dict: + return super().__call__(action, request, user, user_data, *args, **kwargs) + + +filter_user_data_hook = FilterUserDataHook() diff --git a/misago/oauth2/hooks/validate_user_data.py b/misago/oauth2/hooks/validate_user_data.py new file mode 100644 index 0000000000..52e951d335 --- /dev/null +++ b/misago/oauth2/hooks/validate_user_data.py @@ -0,0 +1,52 @@ +from typing import Optional, Protocol + +from django.http import HttpRequest + +from ...plugins.hooks import FilterHook +from ...users.models import User + + +class ValidateUserDataHookAction(Protocol): + def __call__( + self, + request: HttpRequest, + user: Optional[User], + user_data: dict, + response_json: dict, + ) -> dict: + pass + + +class ValidateUserDataHookFilter(Protocol): + def __call__( + self, + action: ValidateUserDataHookAction, + request: HttpRequest, + user: Optional[User], + user_data: dict, + response_json: dict, + ) -> dict: + pass + + +class ValidateUserDataHook( + FilterHook[ValidateUserDataHookAction, ValidateUserDataHookFilter] +): + __slots__ = FilterHook.__slots__ + + def __call__( + self, + action: ValidateUserDataHookAction, + request: HttpRequest, + user: Optional[User], + user_data: dict, + response_json: dict, + *args, + **kwargs, + ) -> dict: + return super().__call__( + action, request, user, user_data, response_json, *args, **kwargs + ) + + +validate_user_data_hook = ValidateUserDataHook() diff --git a/misago/oauth2/tests/conftest.py b/misago/oauth2/tests/conftest.py new file mode 100644 index 0000000000..772b84df2f --- /dev/null +++ b/misago/oauth2/tests/conftest.py @@ -0,0 +1,6 @@ +import pytest + + +@pytest.fixture +def disable_user_data_filters(): + pass diff --git a/misago/oauth2/tests/test_user_data_filter.py b/misago/oauth2/tests/test_user_data_filter.py index 2b38cf1800..c7a25a2b3e 100644 --- a/misago/oauth2/tests/test_user_data_filter.py +++ b/misago/oauth2/tests/test_user_data_filter.py @@ -1,5 +1,3 @@ -from unittest.mock import patch - from ..validation import filter_user_data @@ -204,78 +202,6 @@ def user_email_filter(request, user, user_data): } -def test_new_user_data_is_filtered_using_custom_filters(db, request): - def user_is_none_filter(request, user, user_data): - assert user is None - - with patch( - "misago.oauth2.validation.oauth2_user_data_filters", - [ - user_request_filter, - user_is_none_filter, - user_id_filter, - user_name_filter, - user_email_filter, - ], - ): - filtered_data = filter_user_data( - request, - None, - { - "id": "1234", - "name": "New User", - "email": "oauth2@example.com", - "avatar": None, - }, - ) - - assert filtered_data == { - "id": "4321", - "name": "resU weN", - "email": "filtered_oauth2@example.com", - "avatar": None, - } - - -def test_existing_user_data_is_filtered_using_custom_filters(user, request): - def user_is_set_filter(request, user_obj, user_data): - assert user_obj == user - return { - "id": str(user_obj.id), - "name": user_data["name"], - "email": user_data["email"], - "avatar": user_data["avatar"], - } - - with patch( - "misago.oauth2.validation.oauth2_user_data_filters", - [ - user_request_filter, - user_is_set_filter, - user_id_filter, - user_name_filter, - user_email_filter, - ], - ): - filtered_data = filter_user_data( - request, - user, - { - "id": "1234", - "name": "New User", - "email": "oauth2@example.com", - "avatar": None, - }, - ) - - assert filtered_data == { - "id": "".join(reversed(str(user.id))), - "name": "resU weN", - "email": "filtered_oauth2@example.com", - "avatar": None, - } - - def test_original_user_data_is_not_mutated_by_default_filters(user, request): user_data = { "id": "1234", @@ -298,47 +224,3 @@ def test_original_user_data_is_not_mutated_by_default_filters(user, request): "email": "oauth2@example.com", "avatar": None, } - - -def test_original_user_data_is_not_mutated_by_custom_filters(user, request): - def user_is_set_filter(request, user_obj, user_data): - assert user_obj == user - return { - "id": str(user_obj.id), - "name": user_data["name"], - "email": user_data["email"], - "avatar": user_data["avatar"], - } - - user_data = { - "id": "1234", - "name": "New User", - "email": "oauth2@example.com", - "avatar": None, - } - - with patch( - "misago.oauth2.validation.oauth2_user_data_filters", - [ - user_request_filter, - user_is_set_filter, - user_id_filter, - user_name_filter, - user_email_filter, - ], - ): - filtered_data = filter_user_data(request, user, user_data) - - assert filtered_data == { - "id": "".join(reversed(str(user.id))), - "name": "resU weN", - "email": "filtered_oauth2@example.com", - "avatar": None, - } - - assert user_data == { - "id": "1234", - "name": "New User", - "email": "oauth2@example.com", - "avatar": None, - } diff --git a/misago/oauth2/tests/test_user_data_validation.py b/misago/oauth2/tests/test_user_data_validation.py index 2b5d77cd11..0531e52543 100644 --- a/misago/oauth2/tests/test_user_data_validation.py +++ b/misago/oauth2/tests/test_user_data_validation.py @@ -1,7 +1,6 @@ -from unittest.mock import Mock, patch +from unittest.mock import Mock import pytest -from django.core.exceptions import ValidationError from ..exceptions import OAuth2UserDataValidationError from ..validation import validate_user_data @@ -49,28 +48,19 @@ def test_existing_user_valid_data_is_validated(user, dynamic_settings): } -def user_noop_filter(*args): - return None - - def test_error_was_raised_for_user_data_with_without_name(db, dynamic_settings): with pytest.raises(OAuth2UserDataValidationError) as excinfo: - # Custom filters disable build in filters - with patch( - "misago.oauth2.validation.oauth2_user_data_filters", - [user_noop_filter], - ): - validate_user_data( - Mock(settings=dynamic_settings), - None, - { - "id": "1234", - "name": "", - "email": "user@example.com", - "avatar": None, - }, - {}, - ) + validate_user_data( + Mock(settings=dynamic_settings), + None, + { + "id": "1234", + "name": "", + "email": "user@example.com", + "avatar": None, + }, + {}, + ) assert excinfo.value.error_list == [ ( @@ -82,22 +72,17 @@ def test_error_was_raised_for_user_data_with_without_name(db, dynamic_settings): def test_error_was_raised_for_user_data_with_invalid_name(db, dynamic_settings): with pytest.raises(OAuth2UserDataValidationError) as excinfo: - # Custom filters disable build in filters - with patch( - "misago.oauth2.validation.oauth2_user_data_filters", - [user_noop_filter], - ): - validate_user_data( - Mock(settings=dynamic_settings), - None, - { - "id": "1234", - "name": "Invalid!", - "email": "user@example.com", - "avatar": None, - }, - {}, - ) + validate_user_data( + Mock(settings=dynamic_settings), + None, + { + "id": "1234", + "name": "Invalid!", + "email": "user@example.com", + "avatar": None, + }, + {}, + ) assert excinfo.value.error_list == [ ( @@ -109,22 +94,17 @@ def test_error_was_raised_for_user_data_with_invalid_name(db, dynamic_settings): def test_error_was_raised_for_user_data_with_too_long_name(db, dynamic_settings): with pytest.raises(OAuth2UserDataValidationError) as excinfo: - # Custom filters disable build in filters - with patch( - "misago.oauth2.validation.oauth2_user_data_filters", - [user_noop_filter], - ): - validate_user_data( - Mock(settings=dynamic_settings), - None, - { - "id": "1234", - "name": "UserName" * 100, - "email": "user@example.com", - "avatar": None, - }, - {}, - ) + validate_user_data( + Mock(settings=dynamic_settings), + None, + { + "id": "1234", + "name": "UserName" * 100, + "email": "user@example.com", + "avatar": None, + }, + {}, + ) assert excinfo.value.error_list == [ "Username cannot be longer than 200 characters." @@ -163,52 +143,3 @@ def test_error_was_raised_for_user_data_with_invalid_email(db, dynamic_settings) ) assert excinfo.value.error_list == ["Enter a valid e-mail address."] - - -def custom_oauth2_validator(request, user, user_data, raw_data): - if "bad" in user_data["name"].lower(): - raise ValidationError("Custom validation error!") - - -def test_custom_oauth2_validator_passes_valid_data(db, dynamic_settings): - user_data = { - "id": "1234", - "name": "UserName", - "email": "user@example.com", - "avatar": None, - } - - with patch( - "misago.oauth2.validation.oauth2_validators", - [custom_oauth2_validator], - ): - assert ( - validate_user_data( - Mock(settings=dynamic_settings), - None, - user_data, - {}, - ) - == user_data - ) - - -def test_custom_oauth2_validator_raises_error_for_invalid_data(db, dynamic_settings): - with pytest.raises(OAuth2UserDataValidationError) as excinfo: - with patch( - "misago.oauth2.validation.oauth2_validators", - [custom_oauth2_validator], - ): - validate_user_data( - Mock(settings=dynamic_settings), - None, - { - "id": "1234", - "name": "UserNameBad", - "email": "user@example.com", - "avatar": None, - }, - {}, - ) - - assert excinfo.value.error_list == ["Custom validation error!"] diff --git a/misago/oauth2/validation.py b/misago/oauth2/validation.py index ca23aa5775..b4a14ff9f8 100644 --- a/misago/oauth2/validation.py +++ b/misago/oauth2/validation.py @@ -5,13 +5,13 @@ from django.utils.crypto import get_random_string from unidecode import unidecode -from ..hooks import oauth2_validators, oauth2_user_data_filters from ..users.validators import ( dj_validate_email, validate_username_content, validate_username_length, ) from .exceptions import OAuth2UserDataValidationError +from .hooks import filter_user_data_hook, validate_user_data_hook User = get_user_model() @@ -22,38 +22,38 @@ class UsernameSettings: def validate_user_data(request, user, user_data, response_json): - filtered_data = filter_user_data(request, user, user_data) - try: - validate_username_content(filtered_data["name"]) - validate_username_length(UsernameSettings, filtered_data["name"]) - dj_validate_email(filtered_data["email"]) - - for plugin_oauth2_validator in oauth2_validators: - plugin_oauth2_validator(request, user, user_data, response_json) + return validate_user_data_hook( + validate_user_data_action, + request, + user, + user_data, + response_json, + ) except ValidationError as exc: raise OAuth2UserDataValidationError(error_list=[str(exc.message)]) + +def validate_user_data_action(request, user, user_data, response_json): + filtered_data = filter_user_data(request, user, user_data) + validate_username_content(filtered_data["name"]) + validate_username_length(UsernameSettings, filtered_data["name"]) + dj_validate_email(filtered_data["email"]) return filtered_data def filter_user_data(request, user, user_data): - filtered_data = { + return filter_user_data_hook(filter_user_data_action, request, user, user_data) + + +def filter_user_data_action(request, user, user_data): + return { "id": user_data["id"], - "name": str(user_data["name"] or "").strip(), + "name": filter_name(user, str(user_data["name"] or "").strip()), "email": str(user_data["email"] or "").strip(), "avatar": filter_user_avatar(user_data["avatar"]), } - if oauth2_user_data_filters: - return filter_user_data_with_filters( - request, user, filtered_data, oauth2_user_data_filters - ) - else: - filtered_data["name"] = filter_name(user, filtered_data["name"]) - - return filtered_data - def filter_user_avatar(user_avatar): if user_avatar: diff --git a/misago/plugins/outlets.py b/misago/plugins/outlets.py index 1aba318e85..d06074b901 100644 --- a/misago/plugins/outlets.py +++ b/misago/plugins/outlets.py @@ -23,6 +23,8 @@ def __call__(self, context: dict | Context) -> str | SafeString | None: class PluginOutletHook(ActionHook[PluginOutletHookAction]): + __slots__ = ActionHook.__slots__ + def __call__(self, context: dict | Context) -> List[str | SafeString | None]: return super().__call__(context) From 8aabc8ecceb24c6d00c8398a7051dcc840169879 Mon Sep 17 00:00:00 2001 From: rafalp Date: Sat, 9 Dec 2023 22:38:49 +0100 Subject: [PATCH 2/2] Fix tests --- misago/oauth2/tests/conftest.py | 9 ++++- .../tests/test_user_creation_from_data.py | 33 +++++++------------ .../oauth2/tests/test_user_data_validation.py | 8 +++-- .../tests/test_user_update_with_data.py | 33 +++++++------------ 4 files changed, 38 insertions(+), 45 deletions(-) diff --git a/misago/oauth2/tests/conftest.py b/misago/oauth2/tests/conftest.py index 772b84df2f..18cd0d48d0 100644 --- a/misago/oauth2/tests/conftest.py +++ b/misago/oauth2/tests/conftest.py @@ -1,6 +1,13 @@ +from unittest.mock import patch + import pytest +def noop_filter_user_data(request, user, user_data): + return user_data + + @pytest.fixture def disable_user_data_filters(): - pass + with patch("misago.oauth2.validation.filter_user_data", noop_filter_user_data): + yield diff --git a/misago/oauth2/tests/test_user_creation_from_data.py b/misago/oauth2/tests/test_user_creation_from_data.py index 2ad6d4f2a9..2698adb848 100644 --- a/misago/oauth2/tests/test_user_creation_from_data.py +++ b/misago/oauth2/tests/test_user_creation_from_data.py @@ -1,4 +1,4 @@ -from unittest.mock import Mock, patch +from unittest.mock import Mock import pytest from django.contrib.auth import get_user_model @@ -92,29 +92,20 @@ def test_user_is_created_with_admin_activation_from_valid_data(db, dynamic_setti assert user.requires_activation == User.ACTIVATION_ADMIN -def user_noop_filter(*args): - pass - - def test_user_name_conflict_during_creation_from_valid_data_is_handled( - user, dynamic_settings + user, dynamic_settings, disable_user_data_filters ): with pytest.raises(OAuth2UserDataValidationError) as excinfo: - # Custom filters disable build in filters - with patch( - "misago.oauth2.validation.oauth2_user_data_filters", - [user_noop_filter], - ): - get_user_from_data( - Mock(settings=dynamic_settings, user_ip="83.0.0.1"), - { - "id": "1234", - "name": user.username, - "email": "test@example.com", - "avatar": None, - }, - {}, - ) + get_user_from_data( + Mock(settings=dynamic_settings, user_ip="83.0.0.1"), + { + "id": "1234", + "name": user.username, + "email": "test@example.com", + "avatar": None, + }, + {}, + ) assert excinfo.value.error_list == [ "Your username returned by the provider is not available for use on this site." diff --git a/misago/oauth2/tests/test_user_data_validation.py b/misago/oauth2/tests/test_user_data_validation.py index 0531e52543..14ed16f4fe 100644 --- a/misago/oauth2/tests/test_user_data_validation.py +++ b/misago/oauth2/tests/test_user_data_validation.py @@ -48,7 +48,9 @@ def test_existing_user_valid_data_is_validated(user, dynamic_settings): } -def test_error_was_raised_for_user_data_with_without_name(db, dynamic_settings): +def test_error_was_raised_for_user_data_with_without_name( + db, dynamic_settings, disable_user_data_filters +): with pytest.raises(OAuth2UserDataValidationError) as excinfo: validate_user_data( Mock(settings=dynamic_settings), @@ -70,7 +72,9 @@ def test_error_was_raised_for_user_data_with_without_name(db, dynamic_settings): ] -def test_error_was_raised_for_user_data_with_invalid_name(db, dynamic_settings): +def test_error_was_raised_for_user_data_with_invalid_name( + db, dynamic_settings, disable_user_data_filters +): with pytest.raises(OAuth2UserDataValidationError) as excinfo: validate_user_data( Mock(settings=dynamic_settings), diff --git a/misago/oauth2/tests/test_user_update_with_data.py b/misago/oauth2/tests/test_user_update_with_data.py index a522de62a8..23259ae124 100644 --- a/misago/oauth2/tests/test_user_update_with_data.py +++ b/misago/oauth2/tests/test_user_update_with_data.py @@ -1,4 +1,4 @@ -from unittest.mock import Mock, patch +from unittest.mock import Mock import pytest from django.contrib.auth import get_user_model @@ -72,31 +72,22 @@ def test_user_is_not_updated_with_unchanged_valid_data(user, dynamic_settings): assert user_by_email.id == user.id -def user_noop_filter(*args): - pass - - def test_user_name_conflict_during_update_with_valid_data_is_handled( - user, other_user, dynamic_settings + user, other_user, dynamic_settings, disable_user_data_filters ): Subject.objects.create(sub="1234", user=user) with pytest.raises(OAuth2UserDataValidationError) as excinfo: - # Custom filters disable build in filters - with patch( - "misago.oauth2.validation.oauth2_user_data_filters", - [user_noop_filter], - ): - get_user_from_data( - Mock(settings=dynamic_settings, user_ip="83.0.0.1"), - { - "id": "1234", - "name": other_user.username, - "email": "test@example.com", - "avatar": None, - }, - {}, - ) + get_user_from_data( + Mock(settings=dynamic_settings, user_ip="83.0.0.1"), + { + "id": "1234", + "name": other_user.username, + "email": "test@example.com", + "avatar": None, + }, + {}, + ) assert excinfo.value.error_list == [ "Your username returned by the provider is not available "