From 2daafa8db91dfd92eaa71b0078752128b9e5099e Mon Sep 17 00:00:00 2001 From: Oskar Hahn Date: Thu, 26 Jan 2017 15:34:24 +0100 Subject: [PATCH] Created a function to convert anything possible to a user-collectoin-element or None Changed user.has_perm(...) to has_perm(user, ...) at any place. Removed old code --- CHANGELOG | 1 + openslides/agenda/models.py | 4 +- openslides/agenda/views.py | 19 +-- openslides/assignments/views.py | 25 ++-- openslides/core/access_permissions.py | 8 +- openslides/core/views.py | 22 +-- openslides/global_settings.py | 15 -- openslides/mediafiles/views.py | 19 +-- openslides/motions/views.py | 55 +++---- openslides/topics/views.py | 3 +- openslides/users/access_permissions.py | 13 +- openslides/users/views.py | 20 +-- openslides/utils/auth.py | 185 +++++++----------------- openslides/utils/autoupdate.py | 14 +- openslides/utils/rest_api.py | 7 +- tests/integration/core/test_viewset.py | 16 +- tests/integration/users/test_viewset.py | 12 +- tests/unit/agenda/test_views.py | 24 ++- tests/unit/motions/test_views.py | 16 +- tests/unit/utils/test_auth.py | 53 ------- 20 files changed, 199 insertions(+), 332 deletions(-) delete mode 100644 tests/unit/utils/test_auth.py diff --git a/CHANGELOG b/CHANGELOG index 2b71f0a29..7229bcca5 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -102,6 +102,7 @@ Other: - Accelerated startup process (send all data to the client after login). - Added function utils.auth.anonymous_is_enabled which returns true, if it is. - Changed has_perm to support an user id or None (for anyonmous) as first argument. +- Removed our AnonymousUser. Please make sure not to use user.has_perm() anymore. Version 2.0 (2016-04-18) diff --git a/openslides/agenda/models.py b/openslides/agenda/models.py index 0ada88492..6896afe20 100644 --- a/openslides/agenda/models.py +++ b/openslides/agenda/models.py @@ -1,6 +1,7 @@ from collections import defaultdict from django.conf import settings +from django.contrib.auth.models import AnonymousUser from django.contrib.contenttypes.fields import GenericForeignKey from django.contrib.contenttypes.models import ContentType from django.db import models, transaction @@ -14,7 +15,6 @@ from openslides.utils.exceptions import OpenSlidesError from openslides.utils.models import RESTModelMixin from openslides.utils.utils import to_roman -from ..utils.auth import DjangoAnonymousUser from .access_permissions import ItemAccessPermissions @@ -340,7 +340,7 @@ class SpeakerManager(models.Manager): if self.filter(user=user, item=item, begin_time=None).exists(): raise OpenSlidesError( _('{user} is already on the list of speakers.').format(user=user)) - if isinstance(user, DjangoAnonymousUser): + if isinstance(user, AnonymousUser): raise OpenSlidesError( _('An anonymous user can not be on lists of speakers.')) weight = (self.filter(item=item).aggregate( diff --git a/openslides/agenda/views.py b/openslides/agenda/views.py index 0b98490d4..e2a9d9307 100644 --- a/openslides/agenda/views.py +++ b/openslides/agenda/views.py @@ -16,6 +16,7 @@ from openslides.utils.rest_api import ( list_route, ) +from ..utils.auth import has_perm from .access_permissions import ItemAccessPermissions from .models import Item, Speaker @@ -39,16 +40,16 @@ class ItemViewSet(ListModelMixin, RetrieveModelMixin, UpdateModelMixin, GenericV if self.action in ('list', 'retrieve'): result = self.get_access_permissions().check_permissions(self.request.user) elif self.action in ('metadata', 'manage_speaker', 'tree'): - result = self.request.user.has_perm('agenda.can_see') + result = has_perm(self.request.user, 'agenda.can_see') # For manage_speaker and tree requests the rest of the check is # done in the specific method. See below. elif self.action in ('partial_update', 'update'): - result = (self.request.user.has_perm('agenda.can_see') and - self.request.user.has_perm('agenda.can_see_hidden_items') and - self.request.user.has_perm('agenda.can_manage')) + result = (has_perm(self.request.user, 'agenda.can_see') and + has_perm(self.request.user, 'agenda.can_see_hidden_items') and + has_perm(self.request.user, 'agenda.can_manage')) elif self.action in ('speak', 'sort_speakers', 'numbering'): - result = (self.request.user.has_perm('agenda.can_see') and - self.request.user.has_perm('agenda.can_manage')) + result = (has_perm(self.request.user, 'agenda.can_see') and + has_perm(self.request.user, 'agenda.can_manage')) else: result = False return result @@ -80,14 +81,14 @@ class ItemViewSet(ListModelMixin, RetrieveModelMixin, UpdateModelMixin, GenericV # Check permissions and other conditions. Get user instance. if user_id is None: # Add oneself - if not self.request.user.has_perm('agenda.can_be_speaker'): + if not has_perm(self.request.user, 'agenda.can_be_speaker'): self.permission_denied(request) if item.speaker_list_closed: raise ValidationError({'detail': _('The list of speakers is closed.')}) user = self.request.user else: # Add someone else. - if not self.request.user.has_perm('agenda.can_manage'): + if not has_perm(self.request.user, 'agenda.can_manage'): self.permission_denied(request) try: user = get_user_model().objects.get(pk=int(user_id)) @@ -123,7 +124,7 @@ class ItemViewSet(ListModelMixin, RetrieveModelMixin, UpdateModelMixin, GenericV message = _('You are successfully removed from the list of speakers.') else: # Remove someone else. - if not self.request.user.has_perm('agenda.can_manage'): + if not has_perm(self.request.user, 'agenda.can_manage'): self.permission_denied(request) if type(speaker_ids) is int: speaker_ids = [speaker_ids] diff --git a/openslides/assignments/views.py b/openslides/assignments/views.py index 0af667017..3b30c0d18 100644 --- a/openslides/assignments/views.py +++ b/openslides/assignments/views.py @@ -13,6 +13,7 @@ from openslides.utils.rest_api import ( detail_route, ) +from ..utils.auth import has_perm from .access_permissions import AssignmentAccessPermissions from .models import Assignment, AssignmentPoll, AssignmentRelatedUser from .serializers import AssignmentAllPollSerializer @@ -42,14 +43,14 @@ class AssignmentViewSet(ModelViewSet): result = True elif self.action in ('create', 'partial_update', 'update', 'destroy', 'mark_elected', 'create_poll', 'sort_related_users'): - result = (self.request.user.has_perm('assignments.can_see') and - self.request.user.has_perm('assignments.can_manage')) + result = (has_perm(self.request.user, 'assignments.can_see') and + has_perm(self.request.user, 'assignments.can_manage')) elif self.action == 'candidature_self': - result = (self.request.user.has_perm('assignments.can_see') and - self.request.user.has_perm('assignments.can_nominate_self')) + result = (has_perm(self.request.user, 'assignments.can_see') and + has_perm(self.request.user, 'assignments.can_nominate_self')) elif self.action == 'candidature_other': - result = (self.request.user.has_perm('assignments.can_see') and - self.request.user.has_perm('assignments.can_nominate_other')) + result = (has_perm(self.request.user, 'assignments.can_see') and + has_perm(self.request.user, 'assignments.can_nominate_other')) else: result = False return result @@ -73,7 +74,7 @@ class AssignmentViewSet(ModelViewSet): def nominate_self(self, request, assignment): if assignment.phase == assignment.PHASE_FINISHED: raise ValidationError({'detail': _('You can not candidate to this election because it is finished.')}) - if assignment.phase == assignment.PHASE_VOTING and not request.user.has_perm('assignments.can_manage'): + if assignment.phase == assignment.PHASE_VOTING and not has_perm(request.user, 'assignments.can_manage'): # To nominate self during voting you have to be a manager. self.permission_denied(request) # If the request.user is already a candidate he can nominate himself nevertheless. @@ -84,7 +85,7 @@ class AssignmentViewSet(ModelViewSet): # Withdraw candidature. if assignment.phase == assignment.PHASE_FINISHED: raise ValidationError({'detail': _('You can not withdraw your candidature to this election because it is finished.')}) - if assignment.phase == assignment.PHASE_VOTING and not request.user.has_perm('assignments.can_manage'): + if assignment.phase == assignment.PHASE_VOTING and not has_perm(request.user, 'assignments.can_manage'): # To withdraw self during voting you have to be a manager. self.permission_denied(request) if not assignment.is_candidate(request.user): @@ -133,7 +134,7 @@ class AssignmentViewSet(ModelViewSet): if assignment.phase == assignment.PHASE_FINISHED: detail = _('You can not nominate someone to this election because it is finished.') raise ValidationError({'detail': detail}) - if assignment.phase == assignment.PHASE_VOTING and not request.user.has_perm('assignments.can_manage'): + if assignment.phase == assignment.PHASE_VOTING and not has_perm(request.user, 'assignments.can_manage'): # To nominate another user during voting you have to be a manager. self.permission_denied(request) if assignment.is_candidate(user): @@ -143,7 +144,7 @@ class AssignmentViewSet(ModelViewSet): def delete_other(self, request, user, assignment): # To delete candidature status you have to be a manager. - if not request.user.has_perm('assignments.can_manage'): + if not has_perm(request.user, 'assignments.can_manage'): self.permission_denied(request) if assignment.phase == assignment.PHASE_FINISHED: detail = _("You can not delete someone's candidature to this election because it is finished.") @@ -243,5 +244,5 @@ class AssignmentPollViewSet(UpdateModelMixin, DestroyModelMixin, GenericViewSet) """ Returns True if the user has required permissions. """ - return (self.request.user.has_perm('assignments.can_see') and - self.request.user.has_perm('assignments.can_manage')) + return (has_perm(self.request.user, 'assignments.can_see') and + has_perm(self.request.user, 'assignments.can_manage')) diff --git a/openslides/core/access_permissions.py b/openslides/core/access_permissions.py index 685979d1f..cd60616e6 100644 --- a/openslides/core/access_permissions.py +++ b/openslides/core/access_permissions.py @@ -1,5 +1,7 @@ +from django.contrib.auth.models import AnonymousUser + from ..utils.access_permissions import BaseAccessPermissions -from ..utils.auth import DjangoAnonymousUser, anonymous_is_enabled, has_perm +from ..utils.auth import anonymous_is_enabled, has_perm class ProjectorAccessPermissions(BaseAccessPermissions): @@ -31,7 +33,7 @@ class TagAccessPermissions(BaseAccessPermissions): """ # Every authenticated user can retrieve tags. Anonymous users can do # so if they are enabled. - return not isinstance(user, DjangoAnonymousUser) or anonymous_is_enabled() + return not isinstance(user, AnonymousUser) or anonymous_is_enabled() def get_serializer_class(self, user=None): """ @@ -112,7 +114,7 @@ class ConfigAccessPermissions(BaseAccessPermissions): """ # Every authenticated user can see the metadata and list or retrieve # the config. Anonymous users can do so if they are enabled. - return not isinstance(user, DjangoAnonymousUser) or anonymous_is_enabled() + return not isinstance(user, AnonymousUser) or anonymous_is_enabled() def get_full_data(self, instance): """ diff --git a/openslides/core/views.py b/openslides/core/views.py index 9c27645cf..df4976111 100644 --- a/openslides/core/views.py +++ b/openslides/core/views.py @@ -17,7 +17,7 @@ from django.utils.translation import ugettext as _ from .. import __version__ as version from ..utils import views as utils_views -from ..utils.auth import anonymous_is_enabled +from ..utils.auth import anonymous_is_enabled, has_perm from ..utils.autoupdate import inform_changed_data, inform_deleted_data from ..utils.collection import Collection, CollectionElement from ..utils.plugins import ( @@ -213,15 +213,15 @@ class ProjectorViewSet(ModelViewSet): if self.action in ('list', 'retrieve'): result = self.get_access_permissions().check_permissions(self.request.user) elif self.action == 'metadata': - result = self.request.user.has_perm('core.can_see_projector') + result = has_perm(self.request.user, 'core.can_see_projector') elif self.action in ( 'create', 'update', 'partial_update', 'destroy', 'activate_elements', 'prune_elements', 'update_elements', 'deactivate_elements', 'clear_elements', 'control_view', 'set_resolution', 'set_scroll', 'control_blank', 'broadcast', 'set_projectiondefault', ): - result = (self.request.user.has_perm('core.can_see_projector') and - self.request.user.has_perm('core.can_manage_projector')) + result = (has_perm(self.request.user, 'core.can_see_projector') and + has_perm(self.request.user, 'core.can_manage_projector')) else: result = False return result @@ -576,7 +576,7 @@ class TagViewSet(ModelViewSet): # Anonymous users can do so if they are enabled. result = self.request.user.is_authenticated() or anonymous_is_enabled() elif self.action in ('create', 'update', 'destroy'): - result = self.request.user.has_perm('core.can_manage_tags') + result = has_perm(self.request.user, 'core.can_manage_tags') else: result = False return result @@ -633,7 +633,7 @@ class ConfigViewSet(ViewSet): # enabled. result = self.request.user.is_authenticated() or anonymous_is_enabled() elif self.action == 'update': - result = self.request.user.has_perm('core.can_manage_config') + result = has_perm(self.request.user, 'core.can_manage_config') else: result = False return result @@ -705,11 +705,11 @@ class ChatMessageViewSet(ModelViewSet): # group has the permission core.can_use_chat. result = ( self.request.user.is_authenticated() and - self.request.user.has_perm('core.can_use_chat')) + has_perm(self.request.user, 'core.can_use_chat')) elif self.action == 'clear': result = ( - self.request.user.has_perm('core.can_use_chat') and - self.request.user.has_perm('core.can_manage_chat')) + has_perm(self.request.user, 'core.can_use_chat') and + has_perm(self.request.user, 'core.can_manage_chat')) else: result = False return result @@ -754,7 +754,7 @@ class ProjectorMessageViewSet(ModelViewSet): if self.action in ('list', 'retrieve'): result = self.get_access_permissions().check_permissions(self.request.user) elif self.action in ('create', 'update', 'destroy'): - result = self.request.user.has_perm('core.can_manage_projector') + result = has_perm(self.request.user, 'core.can_manage_projector') else: result = False return result @@ -776,7 +776,7 @@ class CountdownViewSet(ModelViewSet): if self.action in ('list', 'retrieve'): result = self.get_access_permissions().check_permissions(self.request.user) elif self.action in ('create', 'update', 'destroy'): - result = self.request.user.has_perm('core.can_manage_projector') + result = has_perm(self.request.user, 'core.can_manage_projector') else: result = False return result diff --git a/openslides/global_settings.py b/openslides/global_settings.py index ffcd23083..523f21ece 100644 --- a/openslides/global_settings.py +++ b/openslides/global_settings.py @@ -93,10 +93,6 @@ STATICFILES_DIRS = [ AUTH_USER_MODEL = 'users.User' -AUTHENTICATION_BACKENDS = [ - 'openslides.utils.auth.CustomizedModelBackend', -] - SESSION_COOKIE_NAME = 'OpenSlidesSessionID' SESSION_EXPIRE_AT_BROWSER_CLOSE = True @@ -135,17 +131,6 @@ CACHES = { } -# Django REST framework -# http://www.django-rest-framework.org/api-guide/settings/ - -REST_FRAMEWORK = { - 'DEFAULT_AUTHENTICATION_CLASSES': ( - 'rest_framework.authentication.SessionAuthentication', - 'openslides.utils.auth.RESTFrameworkAnonymousAuthentication', - ) -} - - # Django Channels # http://channels.readthedocs.io/en/latest/ diff --git a/openslides/mediafiles/views.py b/openslides/mediafiles/views.py index 92170899f..cdc8113f2 100644 --- a/openslides/mediafiles/views.py +++ b/openslides/mediafiles/views.py @@ -1,3 +1,4 @@ +from ..utils.auth import has_perm from ..utils.rest_api import ModelViewSet, ValidationError from .access_permissions import MediafileAccessPermissions from .models import Mediafile @@ -22,17 +23,17 @@ class MediafileViewSet(ModelViewSet): if self.action in ('list', 'retrieve'): result = self.get_access_permissions().check_permissions(self.request.user) elif self.action == 'metadata': - result = self.request.user.has_perm('mediafiles.can_see') + result = has_perm(self.request.user, 'mediafiles.can_see') elif self.action == 'create': - result = (self.request.user.has_perm('mediafiles.can_see') and - self.request.user.has_perm('mediafiles.can_upload')) + result = (has_perm(self.request.user, 'mediafiles.can_see') and + has_perm(self.request.user, 'mediafiles.can_upload')) elif self.action in ('partial_update', 'update'): - result = (self.request.user.has_perm('mediafiles.can_see') and - self.request.user.has_perm('mediafiles.can_upload') and - self.request.user.has_perm('mediafiles.can_manage')) + result = (has_perm(self.request.user, 'mediafiles.can_see') and + has_perm(self.request.user, 'mediafiles.can_upload') and + has_perm(self.request.user, 'mediafiles.can_manage')) elif self.action == 'destroy': - result = (self.request.user.has_perm('mediafiles.can_see') and - self.request.user.has_perm('mediafiles.can_manage')) + result = (has_perm(self.request.user, 'mediafiles.can_see') and + has_perm(self.request.user, 'mediafiles.can_manage')) else: result = False return result @@ -44,7 +45,7 @@ class MediafileViewSet(ModelViewSet): # Check permission to check if the uploader has to be changed. uploader_id = self.request.data.get('uploader_id') if (uploader_id and - not request.user.has_perm('mediafiles.can_manage') and + not has_perm(request.user, 'mediafiles.can_manage') and str(self.request.user.pk) != str(uploader_id)): self.permission_denied(request) if not self.request.data.get('mediafile'): diff --git a/openslides/motions/views.py b/openslides/motions/views.py index 9c94efe95..f4989d09f 100644 --- a/openslides/motions/views.py +++ b/openslides/motions/views.py @@ -9,6 +9,7 @@ from django.utils.translation import ugettext_noop from rest_framework import status from ..core.config import config +from ..utils.auth import has_perm from ..utils.autoupdate import inform_changed_data from ..utils.rest_api import ( DestroyModelMixin, @@ -61,20 +62,20 @@ class MotionViewSet(ModelViewSet): if self.action in ('list', 'retrieve'): result = self.get_access_permissions().check_permissions(self.request.user) elif self.action in ('metadata', 'partial_update', 'update'): - result = self.request.user.has_perm('motions.can_see') + result = has_perm(self.request.user, 'motions.can_see') # For partial_update and update requests the rest of the check is # done in the update method. See below. elif self.action == 'create': - result = (self.request.user.has_perm('motions.can_see') and - self.request.user.has_perm('motions.can_create') and + result = (has_perm(self.request.user, 'motions.can_see') and + has_perm(self.request.user, 'motions.can_create') and (not config['motions_stop_submitting'] or - self.request.user.has_perm('motions.can_manage'))) + has_perm(self.request.user, 'motions.can_manage'))) elif self.action in ('destroy', 'manage_version', 'set_state', 'set_recommendation', 'create_poll'): - result = (self.request.user.has_perm('motions.can_see') and - self.request.user.has_perm('motions.can_manage')) + result = (has_perm(self.request.user, 'motions.can_see') and + has_perm(self.request.user, 'motions.can_manage')) elif self.action == 'support': - result = (self.request.user.has_perm('motions.can_see') and - self.request.user.has_perm('motions.can_support')) + result = (has_perm(self.request.user, 'motions.can_see') and + has_perm(self.request.user, 'motions.can_support')) else: result = False return result @@ -84,7 +85,7 @@ class MotionViewSet(ModelViewSet): Customized view endpoint to create a new motion. """ # Check permission to send some data. - if not request.user.has_perm('motions.can_manage'): + if not has_perm(request.user, 'motions.can_manage'): whitelist = ( 'title', 'text', @@ -97,7 +98,7 @@ class MotionViewSet(ModelViewSet): self.permission_denied(request) # Check permission to send comment data. - if not request.user.has_perm('motions.can_see_and_manage_comments'): + if not has_perm(request.user, 'motions.can_see_and_manage_comments'): try: # Ignore comments data if user is not allowed to send comments. del request.data['comments'] @@ -128,13 +129,13 @@ class MotionViewSet(ModelViewSet): motion = self.get_object() # Check permissions. - if (not request.user.has_perm('motions.can_manage') and + if (not has_perm(request.user, 'motions.can_manage') and not (motion.is_submitter(request.user) and motion.state.allow_submitter_edit)): self.permission_denied(request) # Check permission to send only some data. - if not request.user.has_perm('motions.can_manage'): + if not has_perm(request.user, 'motions.can_manage'): whitelist = ( 'title', 'text', @@ -144,7 +145,7 @@ class MotionViewSet(ModelViewSet): if key not in whitelist: # Non-staff users are allowed to send only some data. Ignore other data. del request.data[key] - if not request.user.has_perm('motions.can_see_and_manage_comments'): + if not has_perm(request.user, 'motions.can_see_and_manage_comments'): try: del request.data['comments'] except KeyError: @@ -163,7 +164,7 @@ class MotionViewSet(ModelViewSet): # TODO: Log if a version was updated. updated_motion.write_log([ugettext_noop('Motion updated')], request.user) if (config['motions_remove_supporters'] and updated_motion.state.allow_support and - not request.user.has_perm('motions.can_manage')): + not has_perm(request.user, 'motions.can_manage')): updated_motion.supporters.clear() updated_motion.write_log([ugettext_noop('All supporters removed')], request.user) return Response(serializer.data) @@ -352,8 +353,8 @@ class MotionPollViewSet(UpdateModelMixin, DestroyModelMixin, GenericViewSet): """ Returns True if the user has required permissions. """ - return (self.request.user.has_perm('motions.can_see') and - self.request.user.has_perm('motions.can_manage')) + return (has_perm(self.request.user, 'motions.can_see') and + has_perm(self.request.user, 'motions.can_manage')) class MotionChangeRecommendationViewSet(ModelViewSet): @@ -373,9 +374,9 @@ class MotionChangeRecommendationViewSet(ModelViewSet): if self.action in ('list', 'retrieve'): result = self.get_access_permissions().check_permissions(self.request.user) elif self.action == 'metadata': - result = self.request.user.has_perm('motions.can_see') + result = has_perm(self.request.user, 'motions.can_see') elif self.action in ('create', 'destroy', 'partial_update', 'update'): - result = self.request.user.has_perm('motions.can_manage') + result = has_perm(self.request.user, 'motions.can_manage') else: result = False return result @@ -398,10 +399,10 @@ class CategoryViewSet(ModelViewSet): if self.action in ('list', 'retrieve'): result = self.get_access_permissions().check_permissions(self.request.user) elif self.action == 'metadata': - result = self.request.user.has_perm('motions.can_see') + result = has_perm(self.request.user, 'motions.can_see') elif self.action in ('create', 'partial_update', 'update', 'destroy', 'numbering'): - result = (self.request.user.has_perm('motions.can_see') and - self.request.user.has_perm('motions.can_manage')) + result = (has_perm(self.request.user, 'motions.can_see') and + has_perm(self.request.user, 'motions.can_manage')) else: result = False return result @@ -511,10 +512,10 @@ class MotionBlockViewSet(ModelViewSet): if self.action in ('list', 'retrieve'): result = self.get_access_permissions().check_permissions(self.request.user) elif self.action == 'metadata': - result = self.request.user.has_perm('motions.can_see') + result = has_perm(self.request.user, 'motions.can_see') elif self.action in ('create', 'partial_update', 'update', 'destroy', 'follow_recommendations'): - result = (self.request.user.has_perm('motions.can_see') and - self.request.user.has_perm('motions.can_manage')) + result = (has_perm(self.request.user, 'motions.can_see') and + has_perm(self.request.user, 'motions.can_manage')) else: result = False return result @@ -559,10 +560,10 @@ class WorkflowViewSet(ModelViewSet): if self.action in ('list', 'retrieve'): result = self.get_access_permissions().check_permissions(self.request.user) elif self.action == 'metadata': - result = self.request.user.has_perm('motions.can_see') + result = has_perm(self.request.user, 'motions.can_see') elif self.action in ('create', 'partial_update', 'update', 'destroy'): - result = (self.request.user.has_perm('motions.can_see') and - self.request.user.has_perm('motions.can_manage')) + result = (has_perm(self.request.user, 'motions.can_see') and + has_perm(self.request.user, 'motions.can_manage')) else: result = False return result diff --git a/openslides/topics/views.py b/openslides/topics/views.py index e4515037e..858c14ee4 100644 --- a/openslides/topics/views.py +++ b/openslides/topics/views.py @@ -1,5 +1,6 @@ from openslides.utils.rest_api import ModelViewSet +from ..utils.auth import has_perm from .access_permissions import TopicAccessPermissions from .models import Topic @@ -21,5 +22,5 @@ class TopicViewSet(ModelViewSet): if self.action in ('list', 'retrieve'): result = self.get_access_permissions().check_permissions(self.request.user) else: - result = self.request.user.has_perm('agenda.can_manage') + result = has_perm(self.request.user, 'agenda.can_manage') return result diff --git a/openslides/users/access_permissions.py b/openslides/users/access_permissions.py index 373b9f356..f1e5d8d33 100644 --- a/openslides/users/access_permissions.py +++ b/openslides/users/access_permissions.py @@ -1,5 +1,7 @@ +from django.contrib.auth.models import AnonymousUser + from ..utils.access_permissions import BaseAccessPermissions -from ..utils.auth import DjangoAnonymousUser, anonymous_is_enabled, has_perm +from ..utils.auth import anonymous_is_enabled, has_perm class UserAccessPermissions(BaseAccessPermissions): @@ -42,7 +44,9 @@ class UserAccessPermissions(BaseAccessPermissions): case = MANY_DATA else: case = LITTLE_DATA - elif user.pk == full_data.get('id'): + elif user is not None and user.id == full_data.get('id'): + # An authenticated user without the permission to see users tries + # to see himself. case = LITTLE_DATA else: case = NO_DATA @@ -92,10 +96,7 @@ class GroupAccessPermissions(BaseAccessPermissions): """ # Every authenticated user can retrieve groups. Anonymous users can do # so if they are enabled. - # Our AnonymousUser is a subclass of the DjangoAnonymousUser. Normaly, a - # DjangoAnonymousUser means, that AnonymousUser is disabled. But this is - # no garanty. send_data uses the AnonymousUser in any case. - return not isinstance(user, DjangoAnonymousUser) or anonymous_is_enabled() + return not isinstance(user, AnonymousUser) or anonymous_is_enabled() def get_serializer_class(self, user=None): """ diff --git a/openslides/users/views.py b/openslides/users/views.py index 9194acf2b..2b7de2c74 100644 --- a/openslides/users/views.py +++ b/openslides/users/views.py @@ -5,7 +5,7 @@ from django.utils.encoding import force_text from django.utils.translation import ugettext as _ from ..core.config import config -from ..utils.auth import anonymous_is_enabled +from ..utils.auth import anonymous_is_enabled, has_perm from ..utils.collection import CollectionElement from ..utils.rest_api import ( ModelViewSet, @@ -40,11 +40,11 @@ class UserViewSet(ModelViewSet): if self.action in ('list', 'retrieve'): result = self.get_access_permissions().check_permissions(self.request.user) elif self.action in ('metadata', 'update', 'partial_update'): - result = self.request.user.has_perm('users.can_see_name') + result = has_perm(self.request.user, 'users.can_see_name') elif self.action in ('create', 'destroy', 'reset_password'): - result = (self.request.user.has_perm('users.can_see_name') and - self.request.user.has_perm('users.can_see_extra_data') and - self.request.user.has_perm('users.can_manage')) + result = (has_perm(self.request.user, 'users.can_see_name') and + has_perm(self.request.user, 'users.can_see_extra_data') and + has_perm(self.request.user, 'users.can_manage')) else: result = False return result @@ -59,8 +59,8 @@ class UserViewSet(ModelViewSet): wants to update himself or is manager. """ # Check manager perms - if (request.user.has_perm('users.can_see_extra_data') and - request.user.has_perm('users.can_manage')): + if (has_perm(request.user, 'users.can_see_extra_data') and + has_perm(request.user, 'users.can_manage')): if request.data.get('is_active') is False and self.get_object() == request.user: # A user can not deactivate himself. raise ValidationError({'detail': _('You can not deactivate yourself.')}) @@ -141,9 +141,9 @@ class GroupViewSet(ModelViewSet): result = self.request.user.is_authenticated() or anonymous_is_enabled() elif self.action in ('create', 'partial_update', 'update', 'destroy'): # Users with all app permissions can edit groups. - result = (self.request.user.has_perm('users.can_see_name') and - self.request.user.has_perm('users.can_see_extra_data') and - self.request.user.has_perm('users.can_manage')) + result = (has_perm(self.request.user, 'users.can_see_name') and + has_perm(self.request.user, 'users.can_see_extra_data') and + has_perm(self.request.user, 'users.can_manage')) else: # Deny request in any other case. result = False diff --git a/openslides/utils/auth.py b/openslides/utils/auth.py index 16a017ae9..73de6dde4 100644 --- a/openslides/utils/auth.py +++ b/openslides/utils/auth.py @@ -1,145 +1,24 @@ -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.models import Permission -from django.utils.functional import SimpleLazyObject -from rest_framework.authentication import BaseAuthentication +from django.contrib.auth.models import AnonymousUser from .collection import CollectionElement -# Registered users - -class CustomizedModelBackend(ModelBackend): - """ - Customized backend for authentication. Ensures that all users - without a group have the permissions of the group 'Default' (pk=1). - See AUTHENTICATION_BACKENDS settings. - """ - def get_group_permissions(self, user_obj, obj=None): - """ - Returns a set of permission strings that this user has through his/her - groups. - """ - # TODO: Refactor this after Django 1.8 is minimum requirement. Add - # also anonymous permission check to this backend. - if user_obj.is_anonymous() or obj is not None: - return set() - if not hasattr(user_obj, '_group_perm_cache'): - if user_obj.is_superuser: - perms = Permission.objects.all() - else: - if user_obj.groups.all().count() == 0: # user is in no group - perms = Permission.objects.filter(group__pk=1) # group 'default' (pk=1) - else: - user_groups_field = get_user_model()._meta.get_field('groups') - user_groups_query = 'group__%s' % user_groups_field.related_query_name() - perms = Permission.objects.filter(**{user_groups_query: user_obj}) - perms = perms.values_list('content_type__app_label', 'codename').order_by() - user_obj._group_perm_cache = set("%s.%s" % (ct, name) for ct, name in perms) - return user_obj._group_perm_cache - - -# Anonymous users - -class AnonymousUser(DjangoAnonymousUser): - """ - Class for anonymous user instances which have the permissions from the - group 'Anonymous' (pk=1). - """ - - def has_perm(self, perm, obj=None): - """ - Checks if the user has a specific permission. - """ - default_group = CollectionElement.from_values('users/group', 1) - return perm in default_group.get_full_data()['permissions'] - - -class RESTFrameworkAnonymousAuthentication(BaseAuthentication): - """ - Authentication class for the Django REST framework. - - Sets the user to the our AnonymousUser but only if - anonymous user is enabled in the config. - """ - - def authenticate(self, request): - if anonymous_is_enabled(): - return (AnonymousUser(), None) - return None - - -class AuthenticationMiddleware: - """ - Middleware to get the logged in user in users. - - Uses AnonymousUser instead of Django's anonymous user. - """ - def process_request(self, request): - """ - Like django.contrib.auth.middleware.AuthenticationMiddleware, but uses - our own get_user function. - """ - assert hasattr(request, 'session'), ( - "The authentication middleware requires session middleware " - "to be installed. Edit your MIDDLEWARE_CLASSES setting to insert " - "'django.contrib.sessions.middleware.SessionMiddleware' before " - "'openslides.utils.auth.AuthenticationMiddleware'." - ) - request.user = SimpleLazyObject(lambda: get_user(request)) - - -def get_user(request): - """ - Gets the user from the request. - - This is a mix of django.contrib.auth.get_user and - django.contrib.auth.middleware.get_user which uses our anonymous user. - """ - try: - return_user = request._cached_user - except AttributeError: - # Get the user. If it is a DjangoAnonymousUser, then use our AnonymousUser - return_user = _get_user(request) - if anonymous_is_enabled() and isinstance(return_user, DjangoAnonymousUser): - return_user = AnonymousUser() - request._cached_user = return_user - return return_user - - def has_perm(user, perm): """ Checks that user has a specific permission. - User can be an user object, an user id None (for anonymous) or a - CollectionElement for a user. + User can be an a CollectionElement for a user or None. """ - # First, convert a user id or None to an anonymous user or an CollectionElement - if user is None and anonymous_is_enabled(): - user = AnonymousUser() - elif user is None: - user = DjangoAnonymousUser() - elif isinstance(user, int): - user = CollectionElement.from_values('users/user', user) - - if isinstance(user, AnonymousUser): - # Our anonymous user has a has_perm-method that works with the cache - # system. So we can use it here. - has_perm = user.has_perm(perm) - elif isinstance(user, DjangoAnonymousUser): - # The django anonymous user is only used when anonymous user is disabled - # So he never has permissions to see anything. + # Convert user to right type + user = user_to_collection_user(user) + if user is None and not anonymous_is_enabled(): has_perm = False + elif user is None: + # Use the permissions from the default group. + default_group = CollectionElement.from_values('users/group', 1) + has_perm = perm in default_group.get_full_data()['permissions'] else: - if isinstance(user, get_user_model()): - # Converts a user object to a collection element. - # from_instance can not be used because the user serializer loads - # the group from the db. So each call to from_instance(user) consts - # one db query. - user = CollectionElement.from_values('users/user', user.id) - # Get all groups of the user and then see, if one group has the required # permission. If the user has no groups, then use group 1. group_ids = user.get_full_data()['groups_id'] or [1] @@ -154,5 +33,47 @@ def has_perm(user, perm): def anonymous_is_enabled(): - from ..core.config import config - return config['general_system_enable_anonymous'] + """ + Returns true, when the anonymous user is enabled in the settings. + """ + return (CollectionElement.from_values('core/config', 'general_system_enable_anonymous') + .get_full_data()['value']) + + +def user_to_collection_user(user): + """ + Taks an object, that represents a user an converts it to a collection_element + or None, if it is an anonymous user. + + User can be + * a user object, + * a collection_element for an user + * an user id + * an anonymous user. + + Raises an TypeError, if the given user object can not be converted + """ + if user is None: + # Nothing to do + pass + elif isinstance(user, CollectionElement) and user.collection_string == 'users/user': + # Nothing to do + pass + elif isinstance(user, CollectionElement): + raise TypeError( + "Unsupported type for user. Only CollectionElements for users can be" + "used. Not {}".format(user.collection_string)) + elif isinstance(user, int): + user = CollectionElement.from_values('users/user', user) + elif isinstance(user, AnonymousUser): + user = None + elif isinstance(user, get_user_model()): + # Converts a user object to a collection element. + # from_instance can not be used because the user serializer loads + # the group from the db. So each call to from_instance(user) consts + # one db query. + user = CollectionElement.from_values('users/user', user.id) + else: + raise TypeError( + "Unsupported type for user. User {} has type {}.".format(user, type(user))) + return user diff --git a/openslides/utils/autoupdate.py b/openslides/utils/autoupdate.py index 9a6aea9bd..979bb842c 100644 --- a/openslides/utils/autoupdate.py +++ b/openslides/utils/autoupdate.py @@ -11,7 +11,7 @@ from django.db import transaction from ..core.config import config from ..core.models import Projector -from .auth import AnonymousUser, anonymous_is_enabled +from .auth import has_perm, user_to_collection_user from .cache import websocket_user_cache from .collection import Collection, CollectionElement, CollectionElementList @@ -67,7 +67,8 @@ def ws_add_site(message): # Skip apps that do not implement get_startup_elements continue for collection in get_startup_elements(): - output.extend(collection.as_autoupdate_for_user(message.user.id)) + user = user_to_collection_user(message.user.id) + output.extend(collection.as_autoupdate_for_user(user)) # Send all data. If there is no data, then only accept the connection if output: @@ -91,12 +92,9 @@ def ws_add_projector(message, projector_id): Adds the websocket connection to a group specific to the projector with the given id. Also sends all data that are shown on the projector. """ - user = message.user - # user is the django anonymous user. We have our own. - if user.is_anonymous and anonymous_is_enabled(): - user = AnonymousUser() + user = message.user.id - if not user.has_perm('core.can_see_projector'): + if not has_perm(user, 'core.can_see_projector'): send_or_wait(message.reply_channel.send, {'text': 'No permissions to see this projector.'}) else: try: @@ -150,7 +148,7 @@ def send_data(message): for user_id, channel_names in websocket_user_cache.get_all().items(): if not user_id: # Anonymous user - user = AnonymousUser() + user = None else: user = CollectionElement.from_values('users/user', user_id) output = collection_elements.as_autoupdate_for_user(user) diff --git a/openslides/utils/rest_api.py b/openslides/utils/rest_api.py index 66a49ea13..e9487c65f 100644 --- a/openslides/utils/rest_api.py +++ b/openslides/utils/rest_api.py @@ -29,6 +29,7 @@ from rest_framework.viewsets import GenericViewSet as _GenericViewSet # noqa from rest_framework.viewsets import ModelViewSet as _ModelViewSet # noqa from rest_framework.viewsets import ViewSet as _ViewSet # noqa +from .auth import user_to_collection_user from .collection import Collection, CollectionElement router = DefaultRouter() @@ -182,7 +183,8 @@ class ListModelMixin(_ListModelMixin): response = super().list(request, *args, **kwargs) else: collection = Collection(collection_string) - response = Response(collection.as_list_for_user(request.user)) + user = user_to_collection_user(request.user) + response = Response(collection.as_list_for_user(user)) return response @@ -206,8 +208,9 @@ class RetrieveModelMixin(_RetrieveModelMixin): lookup_url_kwarg = self.lookup_url_kwarg or self.lookup_field collection_element = CollectionElement.from_values( collection_string, self.kwargs[lookup_url_kwarg]) + user = user_to_collection_user(request.user) try: - content = collection_element.as_dict_for_user(request.user) + content = collection_element.as_dict_for_user(user) except collection_element.get_model().DoesNotExist: raise Http404 if content is None: diff --git a/tests/integration/core/test_viewset.py b/tests/integration/core/test_viewset.py index 5bb9bb981..6524b51a6 100644 --- a/tests/integration/core/test_viewset.py +++ b/tests/integration/core/test_viewset.py @@ -102,14 +102,10 @@ class TestTagDBQueries(TestCase): def test_anonymous(self): """ Tests that only the following db queries are done: - * 2 requests to get the permission for anonymous (config and permissions) + * 1 requests to see if anonyomus is enabled * 2 requests to get the list of all projectors, - - * 10 requests for to config - - The last 10 requests are a bug. """ - with self.assertNumQueries(14): + with self.assertNumQueries(3): self.client.get(reverse('tag-list')) @@ -140,14 +136,10 @@ class TestConfigDBQueries(TestCase): def test_anonymous(self): """ Tests that only the following db queries are done: - * 2 requests to get the permission for anonymous (config and permissions), + * 1 requests to see if anonymous is enabled * 1 to get all config value and - - * 57 requests to find out if anonymous is enabled. - - TODO: The last 57 requests are a bug. """ - with self.assertNumQueries(63): + with self.assertNumQueries(2): self.client.get(reverse('config-list')) diff --git a/tests/integration/users/test_viewset.py b/tests/integration/users/test_viewset.py index 80f1c68d5..0d5f79c62 100644 --- a/tests/integration/users/test_viewset.py +++ b/tests/integration/users/test_viewset.py @@ -78,18 +78,10 @@ class TestGroupDBQueries(TestCase): def test_anonymous(self): """ Tests that only the following db queries are done: - * 2 requests to find out if anonymous is enabled + * 1 requests to find out if anonymous is enabled * 3 request to get the list of all groups and - - * 14 Requests to find out if anonymous is enabled. - - TODO: There should be only one request to find out if anonymous is enabled. - The reason for the last 14 requests is the base get_restricted_data() - method that is used by the group access_permissions. It calls check_permissions(). - But this is important for the autoupdate-case and can only be fixt by - caching the config object. """ - with self.assertNumQueries(19): + with self.assertNumQueries(4): self.client.get(reverse('group-list')) diff --git a/tests/unit/agenda/test_views.py b/tests/unit/agenda/test_views.py index 9f9ccc778..76d6a4d6c 100644 --- a/tests/unit/agenda/test_views.py +++ b/tests/unit/agenda/test_views.py @@ -15,24 +15,32 @@ class ItemViewSetManageSpeaker(TestCase): self.view_instance.get_object = get_object_mock = MagicMock() get_object_mock.return_value = self.mock_item = MagicMock() + @patch('openslides.agenda.views.has_perm') @patch('openslides.agenda.views.Speaker') - def test_add_oneself_as_speaker(self, mock_speaker): + def test_add_oneself_as_speaker(self, mock_speaker, mock_has_perm): self.request.method = 'POST' - self.request.user.has_perm.return_value = True + self.request.user = 1 + mock_has_perm.return_value = True self.request.data = {} self.mock_item.speaker_list_closed = False + self.view_instance.manage_speaker(self.request) + mock_speaker.objects.add.assert_called_with(self.request.user, self.mock_item) + @patch('openslides.agenda.views.has_perm') @patch('openslides.agenda.views.get_user_model') @patch('openslides.agenda.views.Speaker') - def test_add_someone_else_as_speaker(self, mock_speaker, mock_get_user_model): + def test_add_someone_else_as_speaker(self, mock_speaker, mock_get_user_model, mock_has_perm): self.request.method = 'POST' - self.request.user.has_perm.return_value = True + self.request.user = 1 self.request.data = {'user': '2'} # It is assumed that the request user has pk!=2. mock_get_user_model.return_value = MockUser = MagicMock() MockUser.objects.get.return_value = mock_user = MagicMock() + mock_has_perm.return_value = True + self.view_instance.manage_speaker(self.request) + MockUser.objects.get.assert_called_with(pk=2) mock_speaker.objects.add.assert_called_with(mock_user, self.mock_item) @@ -44,12 +52,16 @@ class ItemViewSetManageSpeaker(TestCase): mock_queryset = mock_speaker.objects.filter.return_value.exclude.return_value mock_queryset.get.return_value.delete.assert_called_with() + @patch('openslides.agenda.views.has_perm') @patch('openslides.agenda.views.Speaker') - def test_remove_someone_else(self, mock_speaker): + def test_remove_someone_else(self, mock_speaker, mock_has_perm): self.request.method = 'DELETE' - self.request.user.has_perm.return_value = True + self.request.user = 1 self.request.data = {'speaker': '1'} + mock_has_perm.return_value = True + self.view_instance.manage_speaker(self.request) + mock_speaker.objects.get.assert_called_with(pk=1) mock_speaker.objects.get.return_value.delete.assert_called_with() diff --git a/tests/unit/motions/test_views.py b/tests/unit/motions/test_views.py index dd3af2608..5835f8916 100644 --- a/tests/unit/motions/test_views.py +++ b/tests/unit/motions/test_views.py @@ -16,10 +16,14 @@ class MotionViewSetCreate(TestCase): self.view_instance.get_serializer = get_serializer_mock = MagicMock() get_serializer_mock.return_value = self.mock_serializer = MagicMock() + @patch('openslides.motions.views.has_perm') @patch('openslides.motions.views.config') - def test_simple_create(self, mock_config): - self.request.user.has_perm.return_value = True + def test_simple_create(self, mock_config, mock_has_perm): + self.request.user = 1 + mock_has_perm.return_value = True + self.view_instance.create(self.request) + self.mock_serializer.save.assert_called_with(request_user=self.request.user) @@ -36,11 +40,15 @@ class MotionViewSetUpdate(TestCase): self.view_instance.get_serializer = get_serializer_mock = MagicMock() get_serializer_mock.return_value = self.mock_serializer = MagicMock() + @patch('openslides.motions.views.has_perm') @patch('openslides.motions.views.config') - def test_simple_update(self, mock_config): - self.request.user.has_perm.return_value = True + def test_simple_update(self, mock_config, mock_has_perm): + self.request.user = 1 self.request.data.get.return_value = versioning_mock = MagicMock() + mock_has_perm.return_value = True + self.view_instance.update(self.request) + self.mock_serializer.save.assert_called_with(disable_versioning=versioning_mock) diff --git a/tests/unit/utils/test_auth.py b/tests/unit/utils/test_auth.py deleted file mode 100644 index 6c4dc53f7..000000000 --- a/tests/unit/utils/test_auth.py +++ /dev/null @@ -1,53 +0,0 @@ -from unittest import TestCase, skip -from unittest.mock import MagicMock, patch - -from openslides.utils.auth import AnonymousUser, get_user - - -@skip # I don't know how to patch the config if it is not imported in the global space -@patch('openslides.utils.auth.config') -@patch('openslides.utils.auth._get_user') -class TestGetUser(TestCase): - def test_not_in_cache(self, mock_get_user, mock_config): - mock_config.__getitem__.return_value = True - mock_get_user.return_value = AnonymousUser() - request = MagicMock() - del request._cached_user - - user = get_user(request) - - mock_get_user.assert_called_once_with(request) - self.assertEqual(user, AnonymousUser()) - self.assertEqual(request._cached_user, AnonymousUser()) - - def test_in_cache(self, mock_get_user, mock_config): - request = MagicMock() - request._cached_user = 'my_user' - - user = get_user(request) - - self.assertFalse( - mock_get_user.called, - "_get_user should not have been called when the user object is in cache") - self.assertEqual( - user, - 'my_user', - "The user in cache should be returned") - - def test_disabled_anonymous_user(self, mock_get_user, mock_config): - mock_config.__getitem__.return_value = False - mock_get_user.return_value = 'django_anonymous_user' - request = MagicMock() - del request._cached_user - - user = get_user(request) - - mock_get_user.assert_called_once_with(request) - self.assertEqual( - user, - 'django_anonymous_user', - "The django user should be returned") - self.assertEqual( - request._cached_user, - 'django_anonymous_user', - "The django user should be cached")