Merge pull request #4521 from normanjaeckel/RefactorStateAccessLevel

Refactored state access level by renaming state field to restriction.

CHANGELOG.rst

@@ -35,8 +35,9 @@ Motions:
    follow recommendation, manage submitters and supporters, change motion
    category, motion block and origin and manage motion polls [#3913].
  - Added new permission to create amendments [#4128].
- - Added new flag to motion state to control access for different users. Added
+ - Added new flag to motion state to control access restrictions for different
-   new permission to see motions in some internal state [#4235, #4518].
+   users. Added new permission to see motions in some internal state
+   [#4235, #4518, #4521].
  - Allowed submitters to set state of new motions in complex and customized
    workflow [#4236].
  - Added multi select action to manage submitters, tags, states and

client/src/app/shared/models/motions/workflow-state.ts

@@ -21,7 +21,7 @@ export class WorkflowState extends Deserializer {
     public name: string;
     public recommendation_label: string;
     public css_class: string;
-    public access_level: number;
+    public restriction: string[];
     public allow_support: boolean;
     public allow_create_poll: boolean;
     public allow_submitter_edit: boolean;

client/src/app/site/motions/modules/motion-workflow/components/workflow-detail/workflow-detail.component.html

@@ -103,13 +103,24 @@
                         </div>
                         <div
                             class="clickable-cell"
-                            *ngIf="perm.type === 'accessLevel'"
+                            *ngIf="perm.type === 'restriction'"
-                            [matMenuTriggerFor]="accessLevelMenu"
+                            [matMenuTriggerFor]="restrictionMenu"
                             [matMenuTriggerData]="{ state: state }"
-                            matTooltip="{{ accessLevels[state.access_level].label | translate }}"
                         >
                             <div class="inner-table">
-                                {{ accessLevels[state.access_level].label | translate }}
+                                <div *ngIf="!state.restriction.length">
+                                    -
+                                </div>
+                                <div *ngIf="state.restriction.length">
+                                    <span
+                                        *ngFor="
+                                            let restriction of state.restriction;
+                                            let last = last
+                                        "
+                                    >
+                                        {{ getRestrictionLabel(restriction) | translate }}<span *ngIf="!last">,&nbsp;</span>
+                                    </span>
+                                </div>
                             </div>
                         </div>
                     </mat-cell>
@@ -179,12 +190,12 @@
     </ng-template>
 </mat-menu>
 
-<!-- Select access level menu -->
+<!-- Select restriction menu -->
-<mat-menu matMenuContent #accessLevelMenu="matMenu">
+<mat-menu matMenuContent #restrictionMenu="matMenu">
     <ng-template let-state="state" matMenuContent>
-        <button mat-menu-item *ngFor="let level of accessLevels" (click)="onSetAccesLevel(level.level, state)">
+        <button mat-menu-item *ngFor="let restriction of restrictions" (click)="onSetRestriction(restriction.key, state)">
-            <mat-icon *ngIf="state.access_level === level.level">check</mat-icon>
+            <mat-icon *ngIf="state.restriction.includes(restriction.key)">check</mat-icon>
-            <span>{{ level.label | translate }}</span>
+            <span>{{ restriction.label | translate }}</span>
         </button>
     </ng-template>
 </mat-menu>

client/src/app/site/motions/modules/motion-workflow/components/workflow-detail/workflow-detail.component.ts

@@ -41,18 +41,18 @@ interface StatePerm {
 }
 
 /**
- * Defines the structure of access levels
+ * Defines the structure of AmendmentIntoFinal
  */
-interface AccessLevel {
+interface AmendmentIntoFinal {
-    level: number;
+    merge: MergeAmendment;
     label: string;
 }
 
 /**
- * Defines the structure of access levels
+ * Defines the structre of restrictions
  */
-interface AmendmentIntoFinal {
+interface Restriction {
-    merge: MergeAmendment;
+    key: string;
     label: string;
 }
 
@@ -108,20 +108,20 @@ export class WorkflowDetailComponent extends BaseViewComponent implements OnInit
             type: 'check'
         },
         { name: 'Show amendment in parent motoin', selector: 'merge_amendment_into_final', type: 'amendment' },
-        { name: 'Access level', selector: 'access_level', type: 'accessLevel' },
+        { name: 'Restrictions', selector: 'restriction', type: 'restriction' },
         { name: 'Label color', selector: 'css_class', type: 'color' },
         { name: 'Next states', selector: 'next_states_id', type: 'state' }
     ] as StatePerm[];
 
     /**
-     * Determines possible access levels
+     * Determines possible restrictions
      */
-    public accessLevels = [
+    public restrictions = [
-        { level: 0, label: '0: All users' },
+        { key: 'managers_only', label: 'Managers only' },
-        { level: 1, label: '1: Submitters, authorized users and managers' },
+        { key: 'motions.can_see_internal', label: 'Can see internal (perm)' },
-        { level: 2, label: '2: Authorized users and managers for motions and metadata' },
+        { key: 'motions.can_manage_metadata', label: 'Can manage metadata (perm)' },
-        { level: 3, label: '3: Only managers for motions' }
+        { key: 'is_submitter', label: 'Is submitter' }
-    ] as AccessLevel[];
+    ] as Restriction[];
 
     /**
      * Determines possible "Merge amendments into final"
@@ -279,11 +279,27 @@ export class WorkflowDetailComponent extends BaseViewComponent implements OnInit
     /**
      * Sets an access level to the given workflow state
      *
-     * @param level The new access level
+     * @param restrictions The new restrictions
      * @param state the state to change
      */
-    public onSetAccesLevel(level: number, state: WorkflowState): void {
+    public onSetRestriction(restriction: string, state: WorkflowState): void {
-        this.workflowRepo.updateState({ access_level: level }, state).then(() => {}, this.raiseError);
+        const restrictions = state.restriction.map(r => r);
+        const restrictionIndex = restrictions.findIndex(r => r === restriction);
+
+        if (restrictionIndex < 0) {
+            restrictions.push(restriction);
+        } else {
+            restrictions.splice(restrictionIndex, 1);
+        }
+        this.workflowRepo.updateState({ restriction: restrictions }, state).then(() => {}, this.raiseError);
+    }
+
+    /**
+     * @returns the restriction label for the given restriction
+     */
+    public getRestrictionLabel(restriction: string): string {
+        const entry = this.restrictions.find(r => r.key === restriction);
+        return entry ? entry.label : '';
     }
 
     /**

client/src/app/site/motions/services/motion-filter-list.service.ts

@@ -246,7 +246,7 @@ export class MotionFilterListService extends BaseFilterListService<Motion, ViewM
                     // filter out restricted states for unpriviledged users
                     if (
                         this.operator.hasPerms('motions.can_manage', 'motions.can_manage_metadata') ||
-                        state.access_level === 0
+                        state.restriction.length === 0
                     ) {
                         if (state.isFinalState) {
                             finalStates.push(state.id);

openslides/motions/access_permissions.py

@@ -37,19 +37,30 @@ class MotionAccessPermissions(BaseAccessPermissions):
                     is_submitter = False
 
                 # Check see permission for this motion.
-                from .models import State
+                restriction = full["state_restriction"]
 
-                if await async_has_perm(user_id, "motions.can_manage"):
+                # Managers can see all motions.
-                    level = State.MANAGERS_ONLY
+                permission = await async_has_perm(user_id, "motions.can_manage")
-                elif await async_has_perm(
+                # If restriction field is an empty list, everybody can see the motion.
-                    user_id, "motions.can_manage_metadata"
+                permission = permission or not restriction
-                ) or await async_has_perm(user_id, "motions.can_see_internal"):
+
-                    level = State.EXTENDED_MANAGERS
+                if not permission:
-                elif is_submitter:
+                    # Parse values of restriction field.
-                    level = State.EXTENDED_MANAGERS_AND_SUBMITTER
+                    for value in restriction:
-                else:
+                        if value == "managers_only":
-                    level = State.ALL
+                            # permission remains false
-                permission = level >= full["state_access_level"]
+                            break
+                        elif value in (
+                            "motions.can_see_internal",
+                            "motions.can_manage_metadata",
+                        ):
+                            if await async_has_perm(user_id, value):
+                                permission = True
+                                break
+                        elif value == "is_submitter":
+                            if is_submitter:
+                                permission = True
+                                break
 
                 # Parse single motion.
                 if permission:

openslides/motions/migrations/0024_state_restriction_1.py

@@ -0,0 +1,25 @@
+# Generated by Django 2.1.7 on 2019-03-20 15:49
+
+import jsonfield.encoder
+import jsonfield.fields
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [("motions", "0023_remove_motion_log")]
+
+    operations = [
+        migrations.AddField(
+            model_name="state",
+            name="restriction",
+            field=jsonfield.fields.JSONField(
+                default=list,
+                dump_kwargs={
+                    "cls": jsonfield.encoder.JSONEncoder,
+                    "separators": (",", ":"),
+                },
+                load_kwargs={},
+            ),
+        )
+    ]

openslides/motions/migrations/0024_state_restriction_2.py

@@ -0,0 +1,34 @@
+# Generated by Django 2.1.7 on 2019-03-20 15:49
+
+from django.db import migrations
+
+
+def copy_access_level(apps, schema_editor):
+    """
+    Sets new restriction field of states according to access_level.
+    """
+    # We get the model from the versioned app registry;
+    # if we directly import it, it will be the wrong version.
+    State = apps.get_model("motions", "State")
+    for state in State.objects.all():
+        if state.access_level == 3:
+            state.restriction = ["managers_only"]
+        elif state.access_level == 2:
+            state.restriction = [
+                "motions.can_see_internal",
+                "motions.can_manage_metadata",
+            ]
+        elif state.access_level == 1:
+            state.restriction = [
+                "motions.can_see_internal",
+                "motions.can_manage_metadata",
+                "is_submitter",
+            ]
+        state.save(skip_autoupdate=True)
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [("motions", "0024_state_restriction_1")]
+
+    operations = [migrations.RunPython(copy_access_level)]

openslides/motions/migrations/0024_state_restriction_3.py

@@ -0,0 +1,10 @@
+# Generated by Django 2.1.7 on 2019-03-20 15:55
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [("motions", "0024_state_restriction_2")]
+
+    operations = [migrations.RemoveField(model_name="state", name="access_level")]

openslides/motions/models.py

@@ -985,24 +985,6 @@ class State(RESTModelMixin, models.Model):
     state.
     """
 
-    ALL = 0
-    EXTENDED_MANAGERS_AND_SUBMITTER = 1
-    EXTENDED_MANAGERS = 2
-    MANAGERS_ONLY = 3
-
-    ACCESS_LEVELS = (
-        (ALL, "All users with permission to see motions"),
-        (
-            EXTENDED_MANAGERS_AND_SUBMITTER,
-            "Submitters, authorized users (with permission to see internal motions), managers and users with permission to manage metadata",
-        ),
-        (
-            EXTENDED_MANAGERS,
-            "Only authorized users (with permission to see internal motions), managers and users with permission to manage metadata",
-        ),
-        (MANAGERS_ONLY, "Only managers"),
-    )
-
     name = models.CharField(max_length=255)
     """A string representing the state."""
 
@@ -1024,11 +1006,22 @@ class State(RESTModelMixin, models.Model):
     Default value is 'primary' (blue).
     """
 
-    access_level = models.IntegerField(choices=ACCESS_LEVELS, default=0)
+    restriction = JSONField(default=list)
     """
-    Defines which users may see motions in this state e. g. only managers,
+    Defines which users may see motions in this state:
-    authorized users with permission to see internal motiosn, users with permission
+
-    to manage metadata and submitters.
+    Contains a list of one or more of the following strings:
+     * motions.can_see_internal
+     * motions.can_manage_metadata
+     * is_submitter
+     * managers_only
+
+    If the list is empty, everybody with the general permission to see motions
+    can see this motion. If the list contains 'managers_only', only managers with
+    motions.can_manage permission may see this motion. In all other cases the user
+    shall have one of the given permissions respectivly is submitter of the motion.
+
+    Default: Empty list so everybody can see the motion.
     """
 
     allow_support = models.BooleanField(default=False)

openslides/motions/serializers.py

@@ -1,5 +1,6 @@
 from typing import Dict, Optional
 
+import jsonschema
 from django.db import transaction
 
 from ..core.config import config
@@ -13,6 +14,7 @@ from ..utils.rest_api import (
     Field,
     IdPrimaryKeyRelatedField,
     IntegerField,
+    JSONField,
     ModelSerializer,
     SerializerMethodField,
     ValidationError,
@@ -94,6 +96,8 @@ class StateSerializer(ModelSerializer):
     Serializer for motion.models.State objects.
     """
 
+    restriction = JSONField()
+
     class Meta:
         model = State
         fields = (
@@ -101,7 +105,7 @@ class StateSerializer(ModelSerializer):
             "name",
             "recommendation_label",
             "css_class",
-            "access_level",
+            "restriction",
             "allow_support",
             "allow_create_poll",
             "allow_submitter_edit",
@@ -113,6 +117,33 @@ class StateSerializer(ModelSerializer):
             "workflow",
         )
 
+    def validate_restriction(self, value):
+        """
+        Ensures that the value is a list and only contains valid values.
+        """
+        schema = {
+            "$schema": "http://json-schema.org/draft-07/schema#",
+            "title": "Motion workflow state restriction field schema",
+            "description": "An array containing one or more explicit strings to control restriction for motions in this state.",
+            "type": "array",
+            "items": {
+                "type": "string",
+                "enum": [
+                    "motions.can_see_internal",
+                    "motions.can_manage_metadata",
+                    "is_submitter",
+                    "managers_only",
+                ],
+            },
+        }
+
+        # Validate value.
+        try:
+            jsonschema.validate(value, schema)
+        except jsonschema.ValidationError as err:
+            raise ValidationError({"detail": str(err)})
+        return value
+
 
 class WorkflowSerializer(ModelSerializer):
     """
@@ -369,7 +400,7 @@ class MotionSerializer(ModelSerializer):
     polls = MotionPollSerializer(many=True, read_only=True)
     modified_final_version = CharField(allow_blank=True, required=False)
     reason = CharField(allow_blank=True, required=False)
-    state_access_level = SerializerMethodField()
+    state_restriction = SerializerMethodField()
     text = CharField(allow_blank=True)
     title = CharField(max_length=255)
     amendment_paragraphs = AmendmentParagraphsJSONSerializerField(required=False)
@@ -404,7 +435,7 @@ class MotionSerializer(ModelSerializer):
             "supporters",
             "state",
             "state_extension",
-            "state_access_level",
+            "state_restriction",
             "statute_paragraph",
             "workflow_id",
             "recommendation",
@@ -509,9 +540,9 @@ class MotionSerializer(ModelSerializer):
 
         return result
 
-    def get_state_access_level(self, motion):
+    def get_state_restriction(self, motion):
         """
-        Returns the access level of this state. The default is 0 so everybody
+        Returns the restriction of this state. The default is an empty list so everybody
         with permission to see motions can see this motion.
         """
-        return motion.state.access_level
+        return motion.state.restriction

tests/integration/motions/test_viewset.py

@@ -477,11 +477,11 @@ class RetrieveMotion(TestCase):
                 username=f"user_{index}", password="password"
             )
 
-    def test_guest_state_with_access_level(self):
+    def test_guest_state_with_restriction(self):
         config["general_system_enable_anonymous"] = True
         guest_client = APIClient()
         state = self.motion.state
-        state.access_level = State.MANAGERS_ONLY
+        state.restriction = ["managers_only"]
         state.save()
         # The cache has to be cleared, see:
         # https://github.com/OpenSlides/OpenSlides/issues/3396
@@ -490,17 +490,16 @@ class RetrieveMotion(TestCase):
         response = guest_client.get(reverse("motion-detail", args=[self.motion.pk]))
         self.assertEqual(response.status_code, 404)
 
-    def test_admin_state_with_access_level(self):
+    def test_admin_state_with_restriction(self):
         state = self.motion.state
-        state.access_level = State.MANAGERS_ONLY
+        state.restriction = ["managers_only"]
-
         state.save()
         response = self.client.get(reverse("motion-detail", args=[self.motion.pk]))
         self.assertEqual(response.status_code, status.HTTP_200_OK)
 
-    def test_submitter_state_with_access_level(self):
+    def test_submitter_state_with_restriction(self):
         state = self.motion.state
-        state.access_level = State.EXTENDED_MANAGERS_AND_SUBMITTER
+        state.restriction = ["is_submitter"]
         state.save()
         user = get_user_model().objects.create_user(
             username="username_ohS2opheikaSa5theijo",

tests/unit/motions/test_projector.py

@@ -29,7 +29,7 @@ def all_data():
             "supporters_id": [],
             "state_id": 1,
             "state_extension": None,
-            "state_access_level": 0,
+            "state_restriction": [],
             "statute_paragraph_id": None,
             "workflow_id": 1,
             "recommendation_id": None,
@@ -118,7 +118,7 @@ def all_data():
             "supporters_id": [],
             "state_id": 1,
             "state_extension": None,
-            "state_access_level": 0,
+            "state_restriction": [],
             "statute_paragraph_id": None,
             "workflow_id": 1,
             "recommendation_id": None,
@@ -151,7 +151,7 @@ def all_data():
             "supporters_id": [],
             "state_id": 1,
             "state_extension": None,
-            "state_access_level": 0,
+            "state_restriction": [],
             "statute_paragraph_id": 1,
             "workflow_id": 1,
             "recommendation_id": None,
@@ -178,7 +178,7 @@ def all_data():
                     "name": "submitted",
                     "recommendation_label": None,
                     "css_class": "primary",
-                    "access_level": 0,
+                    "restriction": [],
                     "allow_support": True,
                     "allow_create_poll": True,
                     "allow_submitter_edit": True,
@@ -194,7 +194,7 @@ def all_data():
                     "name": "accepted",
                     "recommendation_label": "Acceptance",
                     "css_class": "success",
-                    "access_level": 0,
+                    "restriction": [],
                     "allow_support": False,
                     "allow_create_poll": False,
                     "allow_submitter_edit": False,
@@ -210,7 +210,7 @@ def all_data():
                     "name": "rejected",
                     "recommendation_label": "Rejection",
                     "css_class": "danger",
-                    "access_level": 0,
+                    "restriction": [],
                     "allow_support": False,
                     "allow_create_poll": False,
                     "allow_submitter_edit": False,
@@ -226,7 +226,7 @@ def all_data():
                     "name": "not decided",
                     "recommendation_label": "No decision",
                     "css_class": "default",
-                    "access_level": 0,
+                    "restriction": [],
                     "allow_support": False,
                     "allow_create_poll": False,
                     "allow_submitter_edit": False,