From cb1b262c9295f097a34af44917e23e9c9f62cef3 Mon Sep 17 00:00:00 2001 From: Oskar Hahn Date: Tue, 5 May 2015 10:42:31 +0200 Subject: [PATCH] Fix anonymous user for rest requests --- openslides/agenda/models.py | 2 +- openslides/global_settings.py | 5 +- openslides/users/auth.py | 22 ++++- openslides/users/models.py | 2 +- openslides/users/serializers.py | 63 ++++--------- openslides/users/views.py | 6 +- requirements_production.txt | 2 +- setup.cfg | 2 +- .../users/test_rest_anonymous_user.py | 25 +++++ tests/integration/users/test_viewset.py | 37 +++++++- tests/unit/users/test_auth.py | 4 +- tests/unit/users/test_serializers.py | 91 +++++-------------- 12 files changed, 129 insertions(+), 132 deletions(-) create mode 100644 tests/integration/users/test_rest_anonymous_user.py diff --git a/openslides/agenda/models.py b/openslides/agenda/models.py index 4f5e243de..edf383215 100644 --- a/openslides/agenda/models.py +++ b/openslides/agenda/models.py @@ -163,7 +163,7 @@ class Item(RESTModelMixin, SlideMixin, AbsoluteUrlMixin, MPTTModel): Return the title of this item. """ if not self.content_object: - agenda_title = self.title + agenda_title = self.title or "" else: try: agenda_title = self.content_object.get_agenda_title() diff --git a/openslides/global_settings.py b/openslides/global_settings.py index 446f9de6a..e28b9b52e 100644 --- a/openslides/global_settings.py +++ b/openslides/global_settings.py @@ -183,5 +183,8 @@ TEST_RUNNER = 'openslides.utils.test.OpenSlidesDiscoverRunner' # Config for the REST Framework REST_FRAMEWORK = { - 'UNAUTHENTICATED_USER': 'openslides.users.auth.AnonymousUser', + 'DEFAULT_AUTHENTICATION_CLASSES': ( + 'rest_framework.authentication.SessionAuthentication', + 'openslides.users.auth.AnonymousAuthentication', + ) } diff --git a/openslides/users/auth.py b/openslides/users/auth.py index 2dc039d43..286862020 100644 --- a/openslides/users/auth.py +++ b/openslides/users/auth.py @@ -1,15 +1,15 @@ +from django.contrib.auth import get_user as _get_user from django.contrib.auth import get_user_model from django.contrib.auth.backends import ModelBackend -from django.contrib.auth.models import AnonymousUser as DjangoAnonymousUser from django.contrib.auth.context_processors import auth as _auth -from django.contrib.auth import get_user as _get_user +from django.contrib.auth.models import AnonymousUser as DjangoAnonymousUser +from django.contrib.auth.models import Permission from django.db.models import Q from django.utils.functional import SimpleLazyObject +from rest_framework.authentication import BaseAuthentication from openslides.config.api import config -from .models import Permission - class AnonymousUser(DjangoAnonymousUser): """ @@ -94,6 +94,20 @@ class AuthenticationMiddleware(object): request.user = SimpleLazyObject(lambda: get_user(request)) +class AnonymousAuthentication(BaseAuthentication): + """ + Authentication class for the Django REST framework. + + Sets the user to the our AnonymousUser but only if system_enable_anonymous + is set to True in the config. + """ + + def authenticate(self, request): + if config['system_enable_anonymous']: + return (AnonymousUser(), None) + return None + + def get_user(request): """ Gets the user from the request. diff --git a/openslides/users/models.py b/openslides/users/models.py index cf3635172..563c70a27 100644 --- a/openslides/users/models.py +++ b/openslides/users/models.py @@ -99,7 +99,7 @@ class User(RESTModelMixin, SlideMixin, AbsoluteUrlMixin, PermissionsMixin, Abstr slide_callback_name = 'user' username = models.CharField( - ugettext_lazy('Username'), max_length=255, unique=True) + ugettext_lazy('Username'), max_length=255, unique=True, blank=True) first_name = models.CharField( ugettext_lazy('First name'), max_length=255, blank=True) diff --git a/openslides/users/serializers.py b/openslides/users/serializers.py index 828f104cf..dd3f7245f 100644 --- a/openslides/users/serializers.py +++ b/openslides/users/serializers.py @@ -1,4 +1,3 @@ -from django.core.exceptions import ImproperlyConfigured from django.contrib.auth.hashers import make_password from django.utils.translation import ugettext as _, ugettext_lazy @@ -31,32 +30,6 @@ class UserFullSerializer(ModelSerializer): Serializes all relevant fields. """ - class Meta: - model = User - fields = ( - 'id', - 'is_present', - 'username', - 'title', - 'first_name', - 'last_name', - 'structure_level', - 'about_me', - 'comment', - 'groups', - 'default_password', - 'last_login', - 'is_active',) - - -class UserCreateUpdateSerializer(ModelSerializer): - """ - Serializer for users.models.User objects. - - Serializes data to create new users or update users. - - Do not use this for list or retrieve requests. - """ groups = PrimaryKeyRelatedField( many=True, queryset=Group.objects.exclude(pk__in=(1, 2)), @@ -80,37 +53,33 @@ class UserCreateUpdateSerializer(ModelSerializer): 'default_password', 'is_active',) - def __init__(self, *args, **kwargs): - """ - Overridden to add read_only flag to username field in create requests. - """ - super().__init__(*args, **kwargs) - if self.context['view'].action == 'create': - self.fields['username'].read_only = True - elif self.context['view'].action == 'update': - # Everything is fine. Do nothing. - pass - else: # Other action than 'create' or 'update'. - raise ImproperlyConfigured('This serializer can only be used in create and update requests.') - def validate(self, data): """ Checks that first_name or last_name is given. + + Generates the username if it is empty. """ if not (data.get('username') or data.get('first_name') or data.get('last_name')): raise ValidationError(_('Username, first name and last name can not all be empty.')) + + # Generate username. But only if it is not set and the serializer is not + # called in a patch-context. + try: + action = self.context['view'].action + except (KeyError, AttributeError): + action = None + + if not data.get('username') and action != 'partial_update': + data['username'] = User.objects.generate_username( + data.get('first_name', ''), + data.get('last_name', '')) return data def create(self, validated_data): """ - Creates user with generated username and sets the default_password. - Adds the new user to the registered group. + Creates the user. Sets the default_password. Adds the new user to the + registered group. """ - # Generate username if neccessary. - if not validated_data.get('username'): - validated_data['username'] = User.objects.generate_username( - validated_data.get('first_name', ''), - validated_data.get('last_name', '')) # Prepare setup password. if not validated_data.get('default_password'): validated_data['default_password'] = User.objects.generate_password() diff --git a/openslides/users/views.py b/openslides/users/views.py index 687666ea3..5d3ff641a 100644 --- a/openslides/users/views.py +++ b/openslides/users/views.py @@ -22,7 +22,6 @@ from .models import Group, User from .pdf import users_passwords_to_pdf, users_to_pdf from .serializers import ( GroupSerializer, - UserCreateUpdateSerializer, UserFullSerializer, UserShortSerializer, ) @@ -88,9 +87,8 @@ class UserViewSet(ModelViewSet): Returns different serializer classes with respect to action and user's permissions. """ - if self.action in ('create', 'update'): - serializer_class = UserCreateUpdateSerializer - elif self.request.user.has_perm('users.can_see_extra_data'): + if (self.action in ('create', 'update') or + self.request.user.has_perm('users.can_see_extra_data')): serializer_class = UserFullSerializer else: serializer_class = UserShortSerializer diff --git a/requirements_production.txt b/requirements_production.txt index 8f12db506..eb0eccc06 100644 --- a/requirements_production.txt +++ b/requirements_production.txt @@ -5,7 +5,7 @@ bleach>=1.4,<1.5 django-ckeditor-updated>=4.2.3,<4.4 django-haystack>=2.1,<2.4 django-mptt>=0.6,<0.7 -djangorestframework>=3.0.5,<3.1.0 +djangorestframework>=3.0.5,<3.2.0 jsonfield>=0.9.19,<1.1 natsort>=3.2,<3.6 reportlab>=3.0,<3.2 diff --git a/setup.cfg b/setup.cfg index 245f0c1f7..cee566b4a 100644 --- a/setup.cfg +++ b/setup.cfg @@ -10,4 +10,4 @@ max_line_length = 150 [isort] include_trailing_comma = true multi_line_output = 3 -skip = main.py,signals.py,csv_import.py,global_settings.py,test_personal_info.py,tests.py,auth.py,search_indexes.py,test_list_of_speakers.py,chatbox.py,test_main_menu.py,test_config.py,runserver.py,rest_api.py,settings.py,test_pdf.py,__main__.py,test_serializers.py,autoupdate.py,pdf.py,widgets.py,models.py,test_csv_import.py,plugins.py,serializers.py,test_csv.py,test_overlays.py,backupdb.py,views.py,test_views.py,urls.py,forms.py,utils.py,test_auth.py +skip = main.py,signals.py,csv_import.py,global_settings.py,test_personal_info.py,tests.py,search_indexes.py,test_list_of_speakers.py,chatbox.py,test_main_menu.py,test_config.py,runserver.py,rest_api.py,settings.py,test_pdf.py,__main__.py,autoupdate.py,pdf.py,widgets.py,models.py,test_csv_import.py,plugins.py,serializers.py,test_csv.py,test_overlays.py,backupdb.py,views.py,test_views.py,urls.py,forms.py,utils.py diff --git a/tests/integration/users/test_rest_anonymous_user.py b/tests/integration/users/test_rest_anonymous_user.py new file mode 100644 index 000000000..fabb3f604 --- /dev/null +++ b/tests/integration/users/test_rest_anonymous_user.py @@ -0,0 +1,25 @@ +from unittest.mock import patch + +from openslides.utils.test import TestCase + + +class TestAnonymousRequests(TestCase): + """ + Test that a request with an user that is not logged in gets only the + requested data, if the anonymous user is activated in the config. + + Expects that the page '/rest/users/user/' needs a permission and the + anonymous user has this permission. + """ + + @patch('openslides.users.auth.config', {'system_enable_anonymous': True}) + def test_with_anonymous_user(self): + response = self.client.get('/rest/users/user/') + + self.assertEqual(response.status_code, 200) + + @patch('openslides.users.auth.config', {'system_enable_anonymous': False}) + def test_without_anonymous_user(self): + response = self.client.get('/rest/users/user/') + + self.assertEqual(response.status_code, 403) diff --git a/tests/integration/users/test_viewset.py b/tests/integration/users/test_viewset.py index 763c5be7d..9ab60dd90 100644 --- a/tests/integration/users/test_viewset.py +++ b/tests/integration/users/test_viewset.py @@ -6,6 +6,25 @@ from openslides.users.models import Group, User from openslides.utils.test import TestCase +class UserGetTest(TestCase): + """ + Tests to receive a users via REST API. + """ + def test_get_with_user_who_is_in_group_with_pk_1(self): + """ + It is invalid, that a user is in the group with the pk 1. But if the + database is invalid, the user should nevertheless be received. + """ + admin = User.objects.get(pk=1) + group1 = Group.objects.get(pk=1) + admin.groups.add(group1) + self.client.login(username='admin', password='admin') + + response = self.client.get('/rest/users/user/1/') + + self.assertEqual(response.status_code, 200) + + class UserCreate(TestCase): """ Tests creation of users via REST API. @@ -47,7 +66,7 @@ class UserCreate(TestCase): 'groups': group_pks}) self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.assertEqual(response.data, {'groups': ["Invalid pk '%d' - object does not exist." % group_pks[0]]}) + self.assertEqual(response.data, {'groups': ["Invalid pk \"%d\" - object does not exist." % group_pks[0]]}) class UserUpdate(TestCase): @@ -55,6 +74,11 @@ class UserUpdate(TestCase): Tests update of users via REST API. """ def test_simple_update_via_patch(self): + """ + Test to only update the last_name with a patch request. + + The field username *should not* be changed by the request. + """ admin_client = APIClient() admin_client.login(username='admin', password='admin') # This is the builtin user 'Administrator' with username 'admin'. The pk is valid. @@ -70,6 +94,11 @@ class UserUpdate(TestCase): self.assertEqual(user.username, 'admin') def test_simple_update_via_put(self): + """ + Test to only update the last_name with a put request. + + The field username *should* be changed by the request. + """ admin_client = APIClient() admin_client.login(username='admin', password='admin') # This is the builtin user 'Administrator'. The pk is valid. @@ -77,10 +106,10 @@ class UserUpdate(TestCase): response = admin_client.put( reverse('user-detail', args=[user_pk]), - {'last_name': 'New name Ohy4eeyei5Sahzah0Os2'}) + {'last_name': 'New name Ohy4eeyei5'}) - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.assertEqual(response.data, {'username': ['This field is required.']}) + self.assertEqual(response.status_code, 200) + self.assertEqual(User.objects.get(pk=1).username, 'New name Ohy4eeyei5') class UserDelete(TestCase): diff --git a/tests/unit/users/test_auth.py b/tests/unit/users/test_auth.py index 26a5d06a0..3f8420b4d 100644 --- a/tests/unit/users/test_auth.py +++ b/tests/unit/users/test_auth.py @@ -1,7 +1,7 @@ from unittest import TestCase -from unittest.mock import patch, MagicMock +from unittest.mock import MagicMock, patch -from openslides.users.auth import AnonymousUser, get_user, auth +from openslides.users.auth import AnonymousUser, auth, get_user class TestAnonymousUser(TestCase): diff --git a/tests/unit/users/test_serializers.py b/tests/unit/users/test_serializers.py index 7fd7ab658..9a9857f23 100644 --- a/tests/unit/users/test_serializers.py +++ b/tests/unit/users/test_serializers.py @@ -1,83 +1,42 @@ from unittest import TestCase from unittest.mock import MagicMock, patch -from django.core.exceptions import ImproperlyConfigured -from rest_framework import status -from rest_framework.viewsets import ModelViewSet -from rest_framework.test import APIRequestFactory +from openslides.users.serializers import UserFullSerializer +from openslides.utils.rest_api import ValidationError -class UserCreateUpdateSerializer(TestCase): - def test_improperly_configured_exception_list_request(self): +class UserCreateUpdateSerializerTest(TestCase): + def test_validate_no_data(self): """ - Tests that ImproperlyConfigured is raised if one tries to use the - UserCreateUpdateSerializer with a list request. + Tests, that the validator raises a ValidationError, if not data is given. """ - # Global import is not possible for some unknown magic. - from openslides.users.serializers import UserCreateUpdateSerializer - factory = APIRequestFactory() - request = factory.get('/') - view_class = ModelViewSet - view_class.queryset = MagicMock() - view_class.serializer_class = UserCreateUpdateSerializer - view = view_class.as_view({'get': 'list'}) + serializer = UserFullSerializer() + data = {} - with self.assertRaises(ImproperlyConfigured): - view(request) + with self.assertRaises(ValidationError): + serializer.validate(data) - @patch('rest_framework.generics.get_object_or_404') - def test_improperly_configured_exception_retrieve_request(self, mock_get_object_or_404): + @patch('openslides.users.serializers.User.objects.generate_username') + def test_validate_no_username(self, generate_username): """ - Tests that ImproperlyConfigured is raised if one tries to use the - UserCreateUpdateSerializer with a retrieve request. + Tests, that an empty username is generated. """ - # Global import is not possible for some unknown magic. - from openslides.users.serializers import UserCreateUpdateSerializer - factory = APIRequestFactory() - request = factory.get('/') - view_class = ModelViewSet - view_class.queryset = MagicMock() - view_class.serializer_class = UserCreateUpdateSerializer - view = view_class.as_view({'get': 'retrieve'}) - mock_get_object_or_404.return_value = MagicMock() + generate_username.return_value = 'test_value' + serializer = UserFullSerializer() + data = {'first_name': 'TestName'} - with self.assertRaises(ImproperlyConfigured): - view(request, pk='1') + new_data = serializer.validate(data) - def test_no_improperly_configured_exception_create_request(self): + self.assertEqual(new_data['username'], 'test_value') + + def test_validate_no_username_in_patch_request(self): """ - Tests that ImproperlyConfigured is not raised if one tries to use the - UserCreateUpdateSerializer with a create request. + Tests, that an empty username is not set in a patch request context. """ - # Global import is not possible for some unknown magic. - from openslides.users.serializers import UserCreateUpdateSerializer - factory = APIRequestFactory() - request = factory.get('/') - view_class = ModelViewSet - view_class.queryset = MagicMock() - view_class.serializer_class = UserCreateUpdateSerializer - view = view_class.as_view({'get': 'create'}) + view = MagicMock(action='partial_update') + serializer = UserFullSerializer(context={'view': view}) + data = {'first_name': 'TestName'} - response = view(request) + new_data = serializer.validate(data) - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - - @patch('rest_framework.generics.get_object_or_404') - def test_no_improperly_configured_exception_update_request(self, mock_get_object_or_404): - """ - Tests that ImproperlyConfigured is not raised if one tries to use the - UserCreateUpdateSerializer with a update request. - """ - # Global import is not possible for some unknown magic. - from openslides.users.serializers import UserCreateUpdateSerializer - factory = APIRequestFactory() - request = factory.get('/') - view_class = ModelViewSet - view_class.queryset = MagicMock() - view_class.serializer_class = UserCreateUpdateSerializer - view = view_class.as_view({'get': 'update'}) - mock_get_object_or_404.return_value = MagicMock() - - response = view(request, pk='1') - - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertIsNone(new_data.get('username'))