diff --git a/.travis.yml b/.travis.yml index c615d13be..249826f5e 100644 --- a/.travis.yml +++ b/.travis.yml @@ -8,8 +8,6 @@ matrix: pip: true python: - "3.6" - env: - - TRAVIS_NODE_VERSION="10.5" install: - python --version - pip install --upgrade setuptools pip @@ -20,15 +18,13 @@ matrix: - flake8 openslides tests - isort --check-only --diff --recursive openslides tests - python -m mypy openslides/ - - pytest tests/old/ tests/integration/ tests/unit/ --cov --cov-fail-under=75 + - pytest tests/old/ tests/integration/ tests/unit/ --cov --cov-fail-under=76 - language: python cache: pip: true python: - "3.7" - env: - - TRAVIS_NODE_VERSION="10.5" install: - python --version - pip install --upgrade setuptools pip @@ -39,7 +35,7 @@ matrix: - flake8 openslides tests - isort --check-only --diff --recursive openslides tests - python -m mypy openslides/ - - pytest tests/old/ tests/integration/ tests/unit/ --cov --cov-fail-under=75 + - pytest tests/old/ tests/integration/ tests/unit/ --cov --cov-fail-under=76 - language: node_js node_js: diff --git a/CHANGELOG.rst b/CHANGELOG.rst index e4050a27a..25344e1a6 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -21,6 +21,10 @@ Motions: - Option to customly sort motions [#3894]. - Added support for adding a statute [#3894]. +User: + - Added new admin group which grants all permissions. Users of existing group + 'Admin' or 'Staff' are move to the new group during migration [#3859]. + Version 2.3 (unreleased) ======================== diff --git a/client/src/app/core/services/operator.service.ts b/client/src/app/core/services/operator.service.ts index ba81fb38c..ebc356184 100644 --- a/client/src/app/core/services/operator.service.ts +++ b/client/src/app/core/services/operator.service.ts @@ -124,6 +124,9 @@ export class OperatorService extends OpenSlidesComponent { * @param checkPerms The permissions to check, if at least one matches. */ public hasPerms(...checkPerms: Permission[]): boolean { + if (this._user && this._user.groups_id.includes(2)) { + return true; + } return checkPerms.some(permission => { return this.permissions.includes(permission); }); diff --git a/client/src/app/shared/components/head-bar/head-bar.component.html b/client/src/app/shared/components/head-bar/head-bar.component.html index 4b787d3f9..f28d22228 100644 --- a/client/src/app/shared/components/head-bar/head-bar.component.html +++ b/client/src/app/shared/components/head-bar/head-bar.component.html @@ -18,7 +18,7 @@ - diff --git a/client/src/app/shared/components/head-bar/head-bar.component.ts b/client/src/app/shared/components/head-bar/head-bar.component.ts index d5189a266..539f4e90f 100644 --- a/client/src/app/shared/components/head-bar/head-bar.component.ts +++ b/client/src/app/shared/components/head-bar/head-bar.component.ts @@ -1,5 +1,4 @@ import { Component, OnInit, Input, Output, EventEmitter } from '@angular/core'; -import { OperatorService } from '../../../core/services/operator.service'; /** * Reusable head bar component for Apps. @@ -87,7 +86,7 @@ export class HeadBarComponent implements OnInit { /** * Empty constructor */ - public constructor(private op: OperatorService) {} + public constructor() {} /** * empty onInit @@ -108,22 +107,4 @@ export class HeadBarComponent implements OnInit { public clickPlusButton(): void { this.plusButtonClicked.emit(true); } - - /** - * Determine if the operator has the correct permission to use a button in the menu - * @param perm - */ - public opHasPerm(perm: string): boolean { - // return false if the operator is not yet loaded - if (this.op) { - // if no permission was required, return true - if (!perm) { - return true; - } else { - return this.op.hasPerms(perm); - } - } else { - return false; - } - } } diff --git a/client/src/app/site/users/components/group-list/group-list.component.html b/client/src/app/site/users/components/group-list/group-list.component.html index 367d4fa9d..e7e500c3c 100644 --- a/client/src/app/site/users/components/group-list/group-list.component.html +++ b/client/src/app/site/users/components/group-list/group-list.component.html @@ -74,7 +74,9 @@
- + +
diff --git a/client/src/app/site/users/components/user-detail/user-detail.component.ts b/client/src/app/site/users/components/user-detail/user-detail.component.ts index 386b642e9..c26a28717 100644 --- a/client/src/app/site/users/components/user-detail/user-detail.component.ts +++ b/client/src/app/site/users/components/user-detail/user-detail.component.ts @@ -89,11 +89,7 @@ export class UserDetailComponent implements OnInit { * sets the ownPage variable if the operator owns the page */ public opOwnsPage(userId: number): boolean { - if (this.op.user && this.op.user.id === userId) { - return true; - } else { - return false; - } + return this.op.user && this.op.user.id === userId; } /** @@ -326,9 +322,9 @@ export class UserDetailComponent implements OnInit { */ public ngOnInit(): void { this.makeFormEditable(this.editUser); - this.groups = this.DS.getAll(Group); + this.groups = this.DS.filter(Group, group => group.id !== 1); this.DS.changeObservable.subscribe(model => { - if (model instanceof Group) { + if (model instanceof Group && model.id !== 1) { this.groups.push(model as Group); } }); diff --git a/openslides/users/migrations/0007_superadmin.py b/openslides/users/migrations/0007_superadmin.py new file mode 100644 index 000000000..57a83a168 --- /dev/null +++ b/openslides/users/migrations/0007_superadmin.py @@ -0,0 +1,70 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations + + +def create_superadmin_group(apps, schema_editor): + """ + Migrates the groups to create an admin group with all permissions + granted. Replaces the delegate group to get pk=2. + + - Create new delegate group. Move users and permissions to it. + - Rename the old delegate group to Admin and remove all permissions. + - If a group with the name 'Admin' (probably with pk = 4) exists, move all + users from it to the new superadmin group and delete it. If not, check for + the staff group and assign all users to the superadmin group. + """ + Group = apps.get_model('users', 'Group') + + # If no groups exists at all, skip this migration + if Group.objects.count() == 0: + return + + # Get the new superadmin group (or the old delegates) + superadmin, created_superadmin_group = Group.objects.get_or_create(pk=2, defaults={'name': '__temp__'}) + + if not created_superadmin_group: + new_delegate = Group.objects.create(name='Delegates2') + new_delegate.permissions.set(superadmin.permissions.all()) + superadmin.permissions.set([]) + + for user in superadmin.user_set.all(): + user.groups.add(new_delegate) + user.groups.remove(superadmin) + + finished_moving_users = False + try: + admin = Group.objects.get(name='Admin') + for user in admin.user_set.all(): + user.groups.add(superadmin) + user.groups.remove(admin) + admin.delete(skip_autoupdate=True) + finished_moving_users = True + except Group.DoesNotExist: + pass + + if not finished_moving_users: + try: + staff = Group.objects.get(name='Staff') + for user in staff.user_set.all(): + user.groups.add(superadmin) + except Group.DoesNotExist: + pass + + superadmin.name = 'Admin' + superadmin.save(skip_autoupdate=True) + if not created_superadmin_group: + new_delegate.name = 'Delegates' + new_delegate.save(skip_autoupdate=True) + + +class Migration(migrations.Migration): + + dependencies = [ + ('users', '0006_user_email'), + ] + + operations = [ + migrations.RunPython(create_superadmin_group), + ] diff --git a/openslides/users/models.py b/openslides/users/models.py index 07ba04d60..b7e68eaf7 100644 --- a/openslides/users/models.py +++ b/openslides/users/models.py @@ -13,12 +13,13 @@ from django.contrib.auth.models import ( from django.core import mail from django.core.exceptions import ValidationError from django.db import models -from django.db.models import Prefetch, Q +from django.db.models import Prefetch from django.utils import timezone from jsonfield import JSONField from ..core.config import config from ..core.models import Projector +from ..utils.auth import GROUP_ADMIN_PK from ..utils.collection import CollectionElement from ..utils.models import RESTModelMixin from .access_permissions import ( @@ -60,24 +61,15 @@ class UserManager(BaseUserManager): """ Creates an user with the username 'admin'. If such a user already exists, resets it. The password is (re)set to 'admin'. The user - becomes member of the group 'Staff'. The two important permissions - 'users.can_see_name' and 'users.can_manage' are added to this group, - so that the admin can manage all other permissions. + becomes member of the group 'Admin'. """ - query_can_see_name = Q(content_type__app_label='users') & Q(codename='can_see_name') - query_can_manage = Q(content_type__app_label='users') & Q(codename='can_manage') - - admin_group, _ = Group.objects.get_or_create(name='Admin') - admin_group.permissions.add(Permission.objects.get(query_can_see_name)) - admin_group.permissions.add(Permission.objects.get(query_can_manage)) - admin, created = self.get_or_create( username='admin', defaults={'last_name': 'Administrator'}) admin.default_password = 'admin' admin.password = make_password(admin.default_password) admin.save() - admin.groups.add(admin_group) + admin.groups.add(GROUP_ADMIN_PK) return created def generate_username(self, first_name, last_name): diff --git a/openslides/users/signals.py b/openslides/users/signals.py index ea2447c92..ac762f391 100644 --- a/openslides/users/signals.py +++ b/openslides/users/signals.py @@ -2,6 +2,7 @@ from django.apps import apps from django.contrib.auth.models import Permission from django.db.models import Q +from ..utils.auth import GROUP_ADMIN_PK, GROUP_DEFAULT_PK from ..utils.autoupdate import inform_changed_data from .models import Group, User @@ -69,7 +70,7 @@ def create_builtin_groups_and_admin(**kwargs): permission_string = '.'.join((permission.content_type.app_label, permission.codename)) permission_dict[permission_string] = permission - # Default (pk 1) + # Default (pk 1 == GROUP_DEFAULT_PK) base_permissions = ( permission_dict['agenda.can_see'], permission_dict['agenda.can_see_internal_items'], @@ -79,10 +80,13 @@ def create_builtin_groups_and_admin(**kwargs): permission_dict['mediafiles.can_see'], permission_dict['motions.can_see'], permission_dict['users.can_see_name'], ) - group_default = Group.objects.create(pk=1, name='Default') + group_default = Group.objects.create(pk=GROUP_DEFAULT_PK, name='Default') group_default.permissions.add(*base_permissions) - # Delegates (pk 2) + # Admin (pk 2 == GROUP_ADMIN_PK) + group_admin = Group.objects.create(pk=GROUP_ADMIN_PK, name='Admin') + + # Delegates (pk 3) delegates_permissions = ( permission_dict['agenda.can_see'], permission_dict['agenda.can_see_internal_items'], @@ -97,10 +101,10 @@ def create_builtin_groups_and_admin(**kwargs): permission_dict['motions.can_create'], permission_dict['motions.can_support'], permission_dict['users.can_see_name'], ) - group_delegates = Group.objects.create(pk=2, name='Delegates') + group_delegates = Group.objects.create(pk=3, name='Delegates') group_delegates.permissions.add(*delegates_permissions) - # Staff (pk 3) + # Staff (pk 4) staff_permissions = ( permission_dict['agenda.can_see'], permission_dict['agenda.can_see_internal_items'], @@ -126,41 +130,9 @@ def create_builtin_groups_and_admin(**kwargs): permission_dict['users.can_manage'], permission_dict['users.can_see_extra_data'], permission_dict['mediafiles.can_see_hidden'],) - group_staff = Group.objects.create(pk=3, name='Staff') + group_staff = Group.objects.create(pk=4, name='Staff') group_staff.permissions.add(*staff_permissions) - # Admin (pk 4) - admin_permissions = ( - permission_dict['agenda.can_see'], - permission_dict['agenda.can_see_internal_items'], - permission_dict['agenda.can_be_speaker'], - permission_dict['agenda.can_manage'], - permission_dict['agenda.can_manage_list_of_speakers'], - permission_dict['assignments.can_see'], - permission_dict['assignments.can_manage'], - permission_dict['assignments.can_nominate_other'], - permission_dict['assignments.can_nominate_self'], - permission_dict['core.can_see_frontpage'], - permission_dict['core.can_see_projector'], - permission_dict['core.can_manage_config'], - permission_dict['core.can_manage_logos_and_fonts'], - permission_dict['core.can_manage_projector'], - permission_dict['core.can_manage_tags'], - permission_dict['core.can_use_chat'], - permission_dict['core.can_manage_chat'], - permission_dict['mediafiles.can_see'], - permission_dict['mediafiles.can_manage'], - permission_dict['mediafiles.can_upload'], - permission_dict['motions.can_see'], - permission_dict['motions.can_create'], - permission_dict['motions.can_manage'], - permission_dict['users.can_see_name'], - permission_dict['users.can_manage'], - permission_dict['users.can_see_extra_data'], - permission_dict['mediafiles.can_see_hidden'],) - group_admin = Group.objects.create(pk=4, name='Admin') - group_admin.permissions.add(*admin_permissions) - # Add users.can_see_name permission to staff/admin # group to ensure proper management possibilities # TODO: Remove this redundancy after cleanup of the permission system. @@ -181,7 +153,7 @@ def create_builtin_groups_and_admin(**kwargs): permission_dict['motions.can_create'], permission_dict['motions.can_support'], permission_dict['users.can_see_name'], ) - group_committee = Group.objects.create(name='Committees') + group_committee = Group.objects.create(pk=5, name='Committees') group_committee.permissions.add(*committees_permissions) # Create or reset admin user @@ -190,4 +162,4 @@ def create_builtin_groups_and_admin(**kwargs): # After each group was created, the permissions (many to many fields) where # added to the group. So we have to update the cache by calling # inform_changed_data(). - inform_changed_data((group_default, group_delegates, group_staff, group_admin, group_committee)) + inform_changed_data((group_default, group_admin, group_delegates, group_staff, group_committee)) diff --git a/openslides/users/views.py b/openslides/users/views.py index 337e4801c..84fa779bf 100644 --- a/openslides/users/views.py +++ b/openslides/users/views.py @@ -19,6 +19,8 @@ from django.utils.translation import ugettext as _ from ..core.config import config from ..core.signals import permission_change from ..utils.auth import ( + GROUP_ADMIN_PK, + GROUP_DEFAULT_PK, anonymous_is_enabled, has_perm, user_to_collection_user, @@ -338,10 +340,10 @@ class GroupViewSet(ModelViewSet): def destroy(self, request, *args, **kwargs): """ - Protects builtin groups 'Default' (pk=1) from being deleted. + Protects builtin groups 'Default' (pk=1) and 'Admin' (pk=2) from being deleted. """ instance = self.get_object() - if instance.pk == 1: + if instance.pk in (GROUP_DEFAULT_PK, GROUP_ADMIN_PK): self.permission_denied(request) # The list() is required to evaluate the query affected_users_ids = list(instance.user_set.values_list('pk', flat=True)) diff --git a/openslides/utils/auth.py b/openslides/utils/auth.py index 3a5914c0e..a957907e4 100644 --- a/openslides/utils/auth.py +++ b/openslides/utils/auth.py @@ -12,6 +12,7 @@ from .collection import CollectionElement GROUP_DEFAULT_PK = 1 # This is the hard coded pk for the default group. +GROUP_ADMIN_PK = 2 # This is the hard coded pk for the admin group. def get_group_model() -> Model: @@ -45,6 +46,9 @@ def has_perm(user: Optional[CollectionElement], perm: str) -> bool: # Use the permissions from the default group. default_group = CollectionElement.from_values(group_collection_string, GROUP_DEFAULT_PK) has_perm = perm in default_group.get_full_data()['permissions'] + elif GROUP_ADMIN_PK in user.get_full_data()['groups_id']: + # User in admin group (pk 2) grants all permissions. + has_perm = True else: # Get all groups of the user and then see, if one group has the required # permission. If the user has no groups, then use the default group. @@ -62,7 +66,8 @@ def has_perm(user: Optional[CollectionElement], perm: str) -> bool: def in_some_groups(user: Optional[CollectionElement], groups: List[int]) -> bool: """ Checks that user is in at least one given group. Groups can be given as a list - of ids or group instances. + of ids or group instances. If the user is in the admin group (pk = 2) the result + is always true. User can be a CollectionElement of a user or None. """ @@ -78,6 +83,9 @@ def in_some_groups(user: Optional[CollectionElement], groups: List[int]) -> bool elif user is None: # Use the permissions from the default group. in_some_groups = GROUP_DEFAULT_PK in groups + elif GROUP_ADMIN_PK in user.get_full_data()['groups_id']: + # User in admin group (pk 2) grants all permissions. + in_some_groups = True else: # Get all groups of the user and then see, if one group has the required # permission. If the user has no groups, then use the default group. diff --git a/tests/integration/motions/test_viewset.py b/tests/integration/motions/test_viewset.py index e77e94a7c..d2437209b 100644 --- a/tests/integration/motions/test_viewset.py +++ b/tests/integration/motions/test_viewset.py @@ -27,6 +27,12 @@ from openslides.utils.test import TestCase from ..helpers import count_queries +GROUP_DEFAULT_PK = 1 +GROUP_ADMIN_PK = 2 +GROUP_DELEGATE_PK = 3 +GROUP_STAFF_PK = 4 + + @pytest.mark.django_db(transaction=False) def test_motion_db_queries(): """ @@ -138,8 +144,8 @@ class TestStatuteParagraphs(TestCase): def test_create_non_admin(self): self.admin = get_user_model().objects.get(username='admin') - self.admin.groups.add(2) - self.admin.groups.remove(4) + self.admin.groups.add(GROUP_DELEGATE_PK) + self.admin.groups.remove(GROUP_ADMIN_PK) inform_changed_data(self.admin) response = self.client.post( @@ -171,8 +177,8 @@ class TestStatuteParagraphs(TestCase): def test_update_non_admin(self): self.admin = get_user_model().objects.get(username='admin') - self.admin.groups.add(2) - self.admin.groups.remove(4) + self.admin.groups.add(GROUP_DELEGATE_PK) + self.admin.groups.remove(GROUP_ADMIN_PK) inform_changed_data(self.admin) self.create_statute_paragraph() @@ -191,8 +197,8 @@ class TestStatuteParagraphs(TestCase): def test_delete_non_admin(self): self.admin = get_user_model().objects.get(username='admin') - self.admin.groups.add(2) - self.admin.groups.remove(4) + self.admin.groups.add(GROUP_DELEGATE_PK) + self.admin.groups.remove(GROUP_ADMIN_PK) inform_changed_data(self.admin) self.create_statute_paragraph() @@ -315,8 +321,8 @@ class CreateMotion(TestCase): Test to create a motion by a delegate, non staff user. """ self.admin = get_user_model().objects.get(username='admin') - self.admin.groups.add(2) - self.admin.groups.remove(4) + self.admin.groups.add(GROUP_DELEGATE_PK) + self.admin.groups.remove(GROUP_ADMIN_PK) inform_changed_data(self.admin) response = self.client.post( @@ -367,8 +373,8 @@ class CreateMotion(TestCase): parent_motion.save() self.admin = get_user_model().objects.get(username='admin') - self.admin.groups.add(2) - self.admin.groups.remove(4) + self.admin.groups.add(GROUP_DELEGATE_PK) + self.admin.groups.remove(GROUP_ADMIN_PK) inform_changed_data(self.admin) response = self.client.post( @@ -446,7 +452,7 @@ class RetrieveMotion(TestCase): def test_user_without_can_see_user_permission_to_see_motion_and_submitter_data(self): admin = get_user_model().objects.get(username='admin') Submitter.objects.add(admin, self.motion) - group = get_group_model().objects.get(pk=1) # Group with pk 1 is for anonymous and default users. + group = get_group_model().objects.get(pk=GROUP_DEFAULT_PK) # Group with pk 1 is for anonymous and default users. permission_string = 'users.can_see_name' app_label, codename = permission_string.split('.') permission = group.permissions.get(content_type__app_label=app_label, codename=codename) @@ -578,10 +584,8 @@ class DeleteMotion(TestCase): self.assertEqual(motions, 0) def make_admin_delegate(self): - group_admin = self.admin.groups.get(name='Admin') - group_delegates = get_group_model().objects.get(name='Delegates') - self.admin.groups.remove(group_admin) - self.admin.groups.add(group_delegates) + self.admin.groups.remove(GROUP_ADMIN_PK) + self.admin.groups.add(GROUP_DELEGATE_PK) inform_changed_data(self.admin) def put_motion_in_complex_workflow(self): @@ -662,10 +666,8 @@ class ManageSubmitters(TestCase): def test_add_without_permission(self): admin = get_user_model().objects.get(username='admin') - group_admin = admin.groups.get(name='Admin') - group_delegates = type(group_admin).objects.get(name='Delegates') - admin.groups.add(group_delegates) - admin.groups.remove(group_admin) + admin.groups.add(GROUP_DELEGATE_PK) + admin.groups.remove(GROUP_ADMIN_PK) inform_changed_data(admin) response = self.client.post( @@ -732,8 +734,14 @@ class ManageComments(TestCase): self.client.login(username='admin', password='admin') self.admin = get_user_model().objects.get() - self.group_in = get_group_model().objects.get(pk=4) - self.group_out = get_group_model().objects.get(pk=2) # The admin should not be in this group + self.group_out = get_group_model().objects.get(pk=GROUP_DELEGATE_PK) # The admin should not be in this group + + # Put the admin into the staff group, becaust in the admin group, he has all permissions for + # every single comment section. + self.admin.groups.add(GROUP_STAFF_PK) + self.admin.groups.remove(GROUP_ADMIN_PK) + inform_changed_data(self.admin) + self.group_in = get_group_model().objects.get(pk=GROUP_STAFF_PK) self.motion = Motion( title='test_title_SlqfMw(waso0saWMPqcZ', @@ -960,8 +968,11 @@ class TestMotionCommentSection(TestCase): self.client.login(username='admin', password='admin') self.admin = get_user_model().objects.get() - self.group_in = get_group_model().objects.get(pk=4) - self.group_out = get_group_model().objects.get(pk=2) # The admin should not be in this group + self.admin.groups.add(GROUP_STAFF_PK) # Put the admin in a groiup with limited permissions for testing. + self.admin.groups.remove(GROUP_ADMIN_PK) + inform_changed_data(self.admin) + self.group_in = get_group_model().objects.get(pk=GROUP_STAFF_PK) + self.group_out = get_group_model().objects.get(pk=GROUP_DELEGATE_PK) # The admin should not be in this group def test_retrieve(self): """ @@ -1307,7 +1318,7 @@ class SupportMotion(TestCase): """ def setUp(self): self.admin = get_user_model().objects.get(username='admin') - self.admin.groups.add(2) + self.admin.groups.add(GROUP_DELEGATE_PK) inform_changed_data(self.admin) self.client.login(username='admin', password='admin') self.motion = Motion( diff --git a/tests/unit/users/test_models.py b/tests/unit/users/test_models.py index 0d640fec5..5f20a1872 100644 --- a/tests/unit/users/test_models.py +++ b/tests/unit/users/test_models.py @@ -123,19 +123,15 @@ class UserManagerGeneratePassword(TestCase): class UserManagerCreateOrResetAdminUser(TestCase): def test_add_admin_group(self, mock_group, mock_permission): """ - Tests that the Group with name='Staff' is added to the admin. + Tests that the Group with pk=2 (Admin group) is added to the admin. """ admin_user = MagicMock() manager = UserManager() manager.get_or_create = MagicMock(return_value=(admin_user, False)) - staff_group = MagicMock(name="Staff") - mock_group.objects.get_or_create = MagicMock(return_value=(staff_group, True)) - mock_permission.get = MagicMock() - manager.create_or_reset_admin_user() - admin_user.groups.add.assert_called_once_with(staff_group) + admin_user.groups.add.assert_called_once_with(2) # the admin should be added to the admin group with pk=2 def test_password_set_to_admin(self, mock_group, mock_permission): """ @@ -200,24 +196,3 @@ class UserManagerCreateOrResetAdminUser(TestCase): admin_user.last_name, 'Administrator', "The last_name of a new created admin should be 'Administrator'.") - - def test_get_permissions(self, mock_group, mock_permission): - """ - Tests if two permissions are get - """ - admin_user = MagicMock() - manager = UserManager() - manager.get_or_create = MagicMock(return_value=(admin_user, True)) - - staff_group = MagicMock(name="Staff") - mock_group.objects.get_or_create = MagicMock(return_value=(staff_group, True)) - - permission_mock = MagicMock(name="test permission") - mock_permission.objects.get = MagicMock(return_value=permission_mock) - - manager.create_or_reset_admin_user() - - self.assertEqual( - mock_permission.objects.get.call_count, - 2, - "Permission.get should be called two times")