fix: Respect permissions for indicator menus and version locking (#493)

fsbraun · web-flow · commit 29b395587167 · 2025-11-03T20:14:33.000+01:00
* fix: Non superusers do not need unlock permission to edit unlocked or own drafts

* fix: Indicator menu respects correct permissions

* fix tests

* fix premissions in test
diff --git a/djangocms_versioning/admin.py b/djangocms_versioning/admin.py
@@ -1,3 +1,5 @@
+from __future__ import annotations
+
 import json
 import warnings
 from collections import OrderedDict
@@ -1365,7 +1367,7 @@ def unlock_view(self, request, object_id):
             raise Http404
 
         # Check that the user has unlock permission
-        if not request.user.has_perm("djangocms_versioning.delete_versionlock"):
+        if not request.user.has_perm(f"{self.model._meta.app_label}.delete_versionlock"):
             return HttpResponseForbidden(force_str(_("You do not have permission to remove the version lock")))
 
         # Unlock the version
diff --git a/djangocms_versioning/cms_toolbars.py b/djangocms_versioning/cms_toolbars.py
@@ -136,7 +136,7 @@ def _add_unlock_button(self):
                     f"admin:{proxy_model._meta.app_label}_{proxy_model.__name__.lower()}_unlock",
                     args=(version.pk,),
                 )
-                can_unlock = self.request.user.has_perm("djangocms_versioning.delete_versionlock")
+                can_unlock = self.request.user.has_perm(f"{version._meta.app_label}.delete_versionlock")
                 if can_unlock:
                     extra_classes = [
                         "cms-btn-action",
diff --git a/djangocms_versioning/conditions.py b/djangocms_versioning/conditions.py
@@ -80,7 +80,12 @@ def inner(version, user):
 
 def user_can_unlock(message: str) -> callable:
     def inner(version, user):
-        if not user.has_perm("djangocms_versioning.delete_versionlock"):
+        if conf.LOCK_VERSIONS:
+            if user.has_perm(f"{version._meta.app_label}.delete_versionlock"):
+                return
+            draft_version = get_latest_draft_version(version)
+            if draft_version and (draft_version.locked_by == user or draft_version.locked_by is  None):
+                return
             raise ConditionFailed(message)
     return inner
 
diff --git a/djangocms_versioning/indicators.py b/djangocms_versioning/indicators.py
@@ -1,7 +1,6 @@
 from __future__ import annotations
 
 from cms.utils.urlutils import admin_reverse
-from django.contrib.auth import get_permission_codename
 from django.db import models
 from django.utils.http import urlencode
 from django.utils.translation import gettext_lazy as _
@@ -21,10 +20,13 @@ def _reverse_action(version, action, back=None):
 def content_indicator_menu(request, status, versions, back=""):
     from djangocms_versioning.helpers import version_list_url
 
+    has_change_perm =versions[0].has_change_permission(request.user)
+    delete_versionlock_perm = f"{versions[0]._meta.app_label}.delete_versionlock"
+
     menu = []
-    if request.user.has_perm(f"cms.{get_permission_codename('change', versions[0]._meta)}"):
+    if has_change_perm:
         if versions[0].check_unlock.as_bool(request.user):
-            can_unlock = request.user.has_perm("djangocms_versioning.delete_versionlock")
+            can_unlock = request.user.has_perm(delete_versionlock_perm)
             # disable if permissions are insufficient
             additional_class = "" if can_unlock else " cms-pagetree-dropdown-item-disabled"
             menu.append((
diff --git a/tests/test_indicators.py b/tests/test_indicators.py
@@ -1,6 +1,8 @@
+from cms.models import GlobalPagePermission, Site
 from cms.test_utils.testcases import CMSTestCase
 from cms.utils.urlutils import admin_reverse
 
+from djangocms_versioning.constants import ARCHIVED, DRAFT, PUBLISHED
 from djangocms_versioning.helpers import get_latest_admin_viewable_content
 from djangocms_versioning.models import Version
 from djangocms_versioning.test_utils.blogpost.admin import BlogContentAdmin
@@ -88,26 +90,50 @@ class TestVersionState(CMSTestCase):
     def test_page_indicators(self):
         """The page content indicators render correctly"""
         page = PageFactory(node__depth=1) if TreeNode else PageFactory(depth=1)
+        user = self._create_user(
+            "albert",
+            is_staff=True,
+            is_superuser=False,
+            permissions=["change_version", "change_page", "publish_page"]
+        )
+        gpp = GlobalPagePermission.objects.create(
+            user=user,
+            can_add=True,
+            can_change=True,
+            can_delete=True,
+            can_publish=True,
+            can_move_page=True,
+            can_change_permissions=True,
+            can_view=True,
+        )
+        gpp.sites.set([Site.objects.get_current()])
         version1 = PageVersionFactory(
             content__page=page,
             content__language="en",
+            locked_by=None,
         )
         pk = version1.pk
 
         page_tree = admin_reverse("cms_pagecontent_get_tree")
-        with self.login_user_context(self.get_superuser()):
+        with self.login_user_context(user):
             # New page has draft version, nothing else
             response = self.client.get(page_tree, {"language": "en"})
+            # Check markers
             self.assertNotContains(response, "cms-pagetree-node-state-empty")
             self.assertContains(response, "cms-pagetree-node-state-draft")
             self.assertNotContains(response, "cms-pagetree-node-state-published")
             self.assertNotContains(response, "cms-pagetree-node-state-dirty")
             self.assertNotContains(response, "cms-pagetree-node-state-unpublished")
+            # Check actions
+            self.assertContains(response, "Manage Versions...")
+            self.assertContains(response, "Publish")
 
             # Now archive
             response = self.client.post(admin_reverse("djangocms_versioning_pagecontentversion_archive",
                                                       args=(pk,)))
             self.assertEqual(response.status_code, 302)  # Sends a redirect
+            self.assertEqual(Version.objects.get(pk=pk).state, ARCHIVED)
+
             # Is archived indicator? No draft indicator
             response = self.client.get(page_tree, {"language": "en"})
             self.assertContains(response, "cms-pagetree-node-state-archived")
@@ -117,21 +143,26 @@ def test_page_indicators(self):
             response = self.client.post(admin_reverse("djangocms_versioning_pagecontentversion_revert",
                                         args=(pk,)))
             self.assertEqual(response.status_code, 302)  # Sends a redirect
+            pk = Version.objects.filter_by_content_grouping_values(version1.content).order_by("-pk")[0].pk
+            self.assertEqual(Version.objects.get(pk=pk).state, DRAFT)
+
             # Is draft indicator? No archived indicator
             response = self.client.get(page_tree, {"language": "en"})
             self.assertContains(response, "cms-pagetree-node-state-draft")
             self.assertNotContains(response, "cms-pagetree-node-state-archived")
             # New draft was created, get new pk
-            pk = Version.objects.filter_by_content_grouping_values(version1.content).order_by("-pk")[0].pk
 
             # Now publish
             response = self.client.post(admin_reverse("djangocms_versioning_pagecontentversion_publish",
                                         args=(pk,)))
             self.assertEqual(response.status_code, 302)  # Sends a redirect
+            self.assertEqual(Version.objects.get(pk=pk).state, PUBLISHED)
             # Is published indicator? No draft indicator
             response = self.client.get(page_tree, {"language": "en"})
             self.assertContains(response, "cms-pagetree-node-state-published")
             self.assertNotContains(response, "cms-pagetree-node-state-draft")
+            # Check actions
+            self.assertContains(response, "Unpublish")
 
             # Now unpublish
             response = self.client.post(admin_reverse("djangocms_versioning_pagecontentversion_unpublish",
diff --git a/tests/test_locking.py b/tests/test_locking.py
@@ -3,7 +3,7 @@
 from cms.models import PlaceholderRelationField
 from cms.test_utils.testcases import CMSTestCase
 from cms.toolbar.items import TemplateItem
-from cms.toolbar.utils import get_object_preview_url
+from cms.toolbar.utils import get_object_edit_url, get_object_preview_url
 from cms.utils import get_current_site
 from django.contrib import admin
 from django.contrib.auth.models import Permission
@@ -130,6 +130,131 @@ def test_user_does_not_have_change_permission(self):
         self.assertIsNotNone(version.locked_by)  # Was locked
         self.assertEqual(response.status_code, 403)
 
+    def test_editor_without_delete_versionlock_permission_can_edit_unlocked_content(self):
+        """
+        Non-superuser editors with delete_versionlock permission can access edit mode
+        of unlocked content. This test verifies the fix for the issue where editors
+        without delete_versionlock permission were unable to access edit mode even
+        with appropriate change permissions.
+        """
+        # Create an editor user with change permissions and no delete_versionlock permission
+        editor = self._create_user(
+            "editor_without_unlock",
+            is_staff=True,
+            is_superuser=False,
+            permissions=["change_page"],
+        )
+
+        # Create a version without a lock (unlocked)
+        version = factories.PageVersionFactory(state=DRAFT, locked_by=None)
+
+        # Editor should be able to access the edit view
+        url = get_object_edit_url(version.content)
+
+        with self.login_user_context(editor):
+            response = self.client.get(url)
+
+        # Should succeed with 200 status
+        self.assertEqual(response.status_code, 200)
+
+    def test_editor_without_delete_versionlock_permission_can_edit_their_locked_content(self):
+        """
+        Non-superuser editors with delete_versionlock permission can access edit mode
+        of unlocked content. This test verifies the fix for the issue where editors
+        without delete_versionlock permission were unable to access edit mode even
+        with appropriate change permissions.
+        """
+        # Create an editor user with change permissions and no delete_versionlock permission
+        editor = self._create_user(
+            "editor_without_unlock",
+            is_staff=True,
+            is_superuser=False,
+            permissions=["change_page"],
+        )
+
+        # Create a version without a lock (unlocked)
+        version = factories.PageVersionFactory(state=DRAFT, locked_by=editor)
+
+        # Editor should be able to access the edit view
+        url = get_object_edit_url(version.content)
+
+        with self.login_user_context(editor):
+            response = self.client.get(url)
+
+        # Should succeed with 200 status - this is the key test
+        # Without delete_versionlock permission, this would return 403
+        self.assertEqual(response.status_code, 200)
+
+    def test_editor_without_delete_versionlock_permission_cannot_edit_others_content(self):
+        """
+        Non-superuser editors without delete_versionlock permission cannot access
+        edit mode of content locked by another user. This is the expected behavior
+        that was causing issues when users didn't have the delete_versionlock permission.
+        """
+        # Create an editor user without delete_versionlock permission
+        editor_without_unlock = self._create_user(
+            "editor_without_unlock",
+            is_staff=True,
+            is_superuser=False,
+            permissions=["change_page"],
+        )
+
+        # Create a version locked by another user
+        author = factories.UserFactory(is_staff=True)
+        version = factories.PageVersionFactory(
+            state=DRAFT,
+            created_by=author,
+            locked_by=author
+        )
+
+        # Editor without unlock permission should not be able to access edit view
+        url = get_object_edit_url(version.content)
+
+        with self.login_user_context(editor_without_unlock):
+            response = self.client.get(url)
+
+        # Should be denied with 302 status -> redirect to preview
+        self.assertEqual(response.status_code, 302)
+
+        # Version should still be locked by the original author
+        updated_version = Version.objects.get(pk=version.pk)
+        self.assertEqual(updated_version.locked_by, author)
+
+    def test_editor_with_delete_versionlock_permission_can_edit_others_content(self):
+        """
+        Non-superuser editors without delete_versionlock permission cannot access
+        edit mode of content locked by another user. This is the expected behavior
+        that was causing issues when users didn't have the delete_versionlock permission.
+        """
+        # Create an editor user without delete_versionlock permission
+        editor_with_unlock = self._create_user(
+            "editor_with_unlock",
+            is_staff=True,
+            is_superuser=False,
+            permissions=["delete_versionlock", "change_page"],
+        )
+
+        # Create a version locked by another user
+        author = factories.UserFactory(is_staff=True)
+        version = factories.PageVersionFactory(
+            state=DRAFT,
+            created_by=author,
+            locked_by=author
+        )
+
+        # Editor without unlock permission should not be able to access edit view
+        url = get_object_edit_url(version.content)
+
+        with self.login_user_context(editor_with_unlock):
+            response = self.client.get(url)
+
+        # Should be redirected with 302 status since user first must unlock
+        self.assertEqual(response.status_code, 302)
+
+        # Version should still be locked by the original author
+        updated_version = Version.objects.get(pk=version.pk)
+        self.assertEqual(updated_version.locked_by, author)
+
 
 @override_settings(DJANGOCMS_VERSIONING_LOCK_VERSIONS=True)
 class VersionLockUnlockTestCase(CMSTestCase):

Original file line number	Diff line number	Diff line change
`@@ -136,7 +136,7 @@ def _add_unlock_button(self):`
`136`	`136`	`f"admin:{proxy_model._meta.app_label}_{proxy_model.__name__.lower()}_unlock",`
`137`	`137`	`args=(version.pk,),`
`138`	`138`	`)`
`139`		`- can_unlock = self.request.user.has_perm("djangocms_versioning.delete_versionlock")`
	`139`	`+ can_unlock = self.request.user.has_perm(f"{version._meta.app_label}.delete_versionlock")`
`140`	`140`	`if can_unlock:`
`141`	`141`	`extra_classes = [`
`142`	`142`	`"cms-btn-action",`