Skip to content

Commit 7b4aca7

Browse files
elioschmutzelioschmutz
authored
Merge pull request #8165 from 4teamwork/es/TI-1935-delete-watchers
Allow admins to delete any watcher
2 parents cdeef4c + 2416f20 commit 7b4aca7

13 files changed

Lines changed: 215 additions & 23 deletions

File tree

‎changes/TI-1935.feature‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
A limited admin, an administrator and a manager can always remove a watcher from an object. [elioschmutz]

‎changes/TI-1935.other‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Extends the @watchers response with `watcher_properties` which tells the frontend if a specific watcher is deleteable by the current user. [elioschmutz]

‎docs/public/dev-manual/api/api_changelog.rst‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ Breaking Changes
2424
Other Changes
2525
^^^^^^^^^^^^^
2626
``@listing``: Filtering by inactive dossier responsible is now available.
27+
``@watchers``: Extends the @watchers response with ``watcher_properties``.
2728

2829
2025.6.0 (2025-04-07)
2930
---------------------

‎docs/public/dev-manual/api/watchers.rst‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,11 @@ Ein Beobachter kann verschiedene Rollen haben, beispielsweise die Rollen Auftrag
3030
"referenced_actors": [
3131
{
3232
"@id": "https://example.org/@actors/peter.mueller",
33-
"identifier": "peter.mueller",
33+
"identifier": "peter.mueller"
3434
},
3535
{
3636
"@id": "https://example.org/@actors/rolf.ziegler",
37-
"identifier": "rolf.ziegler",
37+
"identifier": "rolf.ziegler"
3838
}
3939
],
4040
"referenced_watcher_roles": [
@@ -49,7 +49,7 @@ Ein Beobachter kann verschiedene Rollen haben, beispielsweise die Rollen Auftrag
4949
{
5050
"id": "task_responsible",
5151
"title": "Auftragnehmer"
52-
},
52+
}
5353
],
5454
"watchers_and_roles": {
5555
"peter.mueller": [
@@ -59,6 +59,14 @@ Ein Beobachter kann verschiedene Rollen haben, beispielsweise die Rollen Auftrag
5959
"regular_watcher",
6060
"task_responsible"
6161
]
62+
},
63+
"watcher_properties": {
64+
"peter.mueller": {
65+
"can_delete_watcher": true
66+
},
67+
"rolf.ziegler": {
68+
"can_delete_watcher": false
69+
}
6270
}
6371
}
6472

@@ -89,7 +97,8 @@ Die Beobachter können als Komponente eines Inhalts direkt über den ``expand``-
8997
"@id": "https://example.org/ordnungssystem/fuehrung/dossier-23/task-1/@listing-stats",
9098
"referenced_actors": ["..."],
9199
"referenced_watcher_roles": ["..."],
92-
"watchers_and_roles": { "...": "..." }
100+
"watchers_and_roles": { "...": "..." },
101+
"watcher_properties": { "...": "..." }
93102
}
94103
},
95104
"...": "..."

‎opengever/api/permissions.zcml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,5 +8,6 @@
88
<permission id="opengever.api.ManageGroups" title="opengever.api: Manage Groups" />
99
<permission id="opengever.api.NotifyArbitraryUsers" title="opengever.api: Notify Arbitrary Users" />
1010
<permission id="opengever.api.AccessErrorLog" title="opengever.api: Access error log" />
11+
<permission id="opengever.api.RemoveAnyWatcher" title="opengever.api: Remove any watcher" />
1112

1213
</configure>

‎opengever/api/tests/test_watchers.py‎

Lines changed: 130 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,13 @@
22
from ftw.testbrowser import browsing
33
from opengever.activity import notification_center
44
from opengever.activity.roles import WATCHER_ROLE
5+
from opengever.api.watchers import WatcherDeleter
56
from opengever.base.model import create_session
67
from opengever.ogds.models.user import User
78
from opengever.testing import IntegrationTestCase
89
from opengever.testing import solr_data_for
910
from opengever.testing import SolrIntegrationTestCase
11+
from zExceptions import Forbidden
1012
import json
1113

1214

@@ -52,6 +54,10 @@ def test_get_watchers_for_tasks(self, browser):
5254
u'watchers_and_roles': {
5355
self.regular_user.id: [u'regular_watcher', u'task_responsible'],
5456
self.dossier_responsible.id: [u'task_issuer']
57+
},
58+
u'watcher_properties': {
59+
self.regular_user.id: {'can_delete_watcher': True },
60+
self.dossier_responsible.id: {'can_delete_watcher': False },
5561
}
5662
}
5763

@@ -104,6 +110,10 @@ def test_get_watchers_for_inbox_forwarding(self, browser):
104110
u'watchers_and_roles': {
105111
self.regular_user.id: [u'regular_watcher', u'task_responsible'],
106112
self.dossier_responsible.id: [u'task_issuer']
113+
},
114+
u'watcher_properties': {
115+
self.regular_user.id: {'can_delete_watcher': False },
116+
self.dossier_responsible.id: {'can_delete_watcher': False },
107117
}
108118
}
109119

@@ -141,6 +151,9 @@ def test_get_watchers_for_documents(self, browser):
141151
],
142152
u'watchers_and_roles': {
143153
self.dossier_responsible.id: [u'regular_watcher']
154+
},
155+
u'watcher_properties': {
156+
self.dossier_responsible.id: {'can_delete_watcher': False },
144157
}
145158
}
146159

@@ -177,6 +190,9 @@ def test_get_watchers_for_mails(self, browser):
177190
],
178191
u'watchers_and_roles': {
179192
self.dossier_responsible.id: [u'regular_watcher']
193+
},
194+
u'watcher_properties': {
195+
self.dossier_responsible.id: {'can_delete_watcher': False },
180196
}
181197
}
182198

@@ -217,6 +233,9 @@ def test_watchers_endpoint_supports_teams(self, browser):
217233
],
218234
u'watchers_and_roles': {
219235
u'team:1': [u'task_responsible'],
236+
},
237+
u'watcher_properties': {
238+
u'team:1': {'can_delete_watcher': False },
220239
}
221240
}
222241

@@ -255,9 +274,11 @@ def test_watchers_endpoint_supports_inboxes(self, browser):
255274
],
256275
u'watchers_and_roles': {
257276
u'inbox:fa': [u'task_responsible'],
277+
},
278+
u'watcher_properties': {
279+
u'inbox:fa': {'can_delete_watcher': False },
258280
}
259281
}
260-
261282
self.assertEqual(expected_json, browser.json)
262283

263284
browser.open(self.task.absolute_url() + '?expand=watchers',
@@ -287,7 +308,8 @@ def test_get_watchers_hides_inactive_users(self, browser):
287308
expected_json = {u'@id': self.document.absolute_url() + '/@watchers',
288309
u'referenced_actors': [],
289310
u'referenced_watcher_roles': [],
290-
u'watchers_and_roles': {}}
311+
u'watchers_and_roles': {},
312+
u'watcher_properties': {}}
291313
self.assertEqual(expected_json, browser.json['@components']['watchers'])
292314

293315

@@ -817,3 +839,109 @@ def test_possible_watchers_includes_users_groups_and_teams(self, browser):
817839
u'items_total': 10}
818840

819841
self.assertEqual(expected_json, browser.json)
842+
843+
class TestWatcherDeleter(IntegrationTestCase):
844+
845+
features = ('activity', )
846+
847+
def test_can_delete_watcher_from_context(self):
848+
self.login(self.regular_user)
849+
850+
center = notification_center()
851+
center.add_watcher_to_resource(self.task, self.regular_user.getId(), WATCHER_ROLE)
852+
center.add_watcher_to_resource(self.task, self.dossier_responsible.getId(), WATCHER_ROLE)
853+
854+
self.assertItemsEqual(
855+
[self.regular_user.getId(), self.dossier_responsible.getId()],
856+
[watcher.actorid for watcher in center.get_watchers(self.task, WATCHER_ROLE)]
857+
)
858+
859+
WatcherDeleter(self.task).delete(self.regular_user.getId())
860+
861+
self.assertItemsEqual(
862+
[self.dossier_responsible.getId()],
863+
[watcher.actorid for watcher in center.get_watchers(self.task, WATCHER_ROLE)]
864+
)
865+
866+
def test_delete_raises_forbidden_if_not_allowed(self):
867+
self.login(self.regular_user)
868+
869+
with self.assertRaises(Forbidden):
870+
WatcherDeleter(self.task).delete(self.dossier_responsible.getId())
871+
872+
def test_can_delete_returns_true_for_current_user(self):
873+
self.login(self.regular_user)
874+
875+
center = notification_center()
876+
center.add_watcher_to_resource(self.task, self.regular_user.getId(), WATCHER_ROLE)
877+
878+
self.assertTrue(WatcherDeleter(self.task).can_delete(self.regular_user.getId()))
879+
880+
def test_can_delete_returns_true_if_actor_has_watcher_role(self):
881+
self.login(self.regular_user)
882+
center = notification_center()
883+
884+
# No one is watching with the WATCHER_ROLE
885+
self.assertItemsEqual(
886+
[],
887+
[watcher.actorid for watcher in center.get_watchers(self.task, WATCHER_ROLE)]
888+
)
889+
890+
# But with other roles
891+
self.assertItemsEqual(
892+
[self.regular_user.getId(), self.dossier_responsible.getId()],
893+
[watcher.actorid for watcher in center.get_watchers(self.task)]
894+
)
895+
896+
# Delete should not be possible because the WATCHER_ROLE is missing for the user
897+
self.assertFalse(WatcherDeleter(self.task).can_delete(self.regular_user.getId()))
898+
899+
center.add_watcher_to_resource(self.task, self.regular_user.getId(), WATCHER_ROLE)
900+
901+
self.assertItemsEqual(
902+
[self.regular_user.getId()],
903+
[watcher.actorid for watcher in center.get_watchers(self.task, WATCHER_ROLE)]
904+
)
905+
906+
# Now it's possible because the user is watching with the required role
907+
self.assertTrue(WatcherDeleter(self.task).can_delete(self.regular_user.getId()))
908+
909+
def test_can_delete_returns_true_for_groups(self):
910+
self.login(self.regular_user)
911+
912+
center = notification_center()
913+
center.add_watcher_to_resource(self.task, 'fa_users', WATCHER_ROLE)
914+
915+
self.assertTrue(WatcherDeleter(self.task).can_delete('fa_users'))
916+
917+
def test_can_delete_returns_false_for_foreign_actors_as_editor(self):
918+
self.login(self.regular_user)
919+
920+
center = notification_center()
921+
center.add_watcher_to_resource(self.task, self.dossier_responsible.getId(), WATCHER_ROLE)
922+
923+
self.assertFalse(WatcherDeleter(self.task).can_delete(self.dossier_responsible.getId()))
924+
925+
def test_can_delete_returns_true_for_foreign_actors_as_limited_admin(self):
926+
self.login(self.limited_admin)
927+
928+
center = notification_center()
929+
center.add_watcher_to_resource(self.task, self.dossier_responsible.getId(), WATCHER_ROLE)
930+
931+
self.assertTrue(WatcherDeleter(self.task).can_delete(self.dossier_responsible.getId()))
932+
933+
def test_can_delete_returns_true_for_foreign_actors_as_administrator(self):
934+
self.login(self.administrator)
935+
936+
center = notification_center()
937+
center.add_watcher_to_resource(self.task, self.dossier_responsible.getId(), WATCHER_ROLE)
938+
939+
self.assertTrue(WatcherDeleter(self.task).can_delete(self.dossier_responsible.getId()))
940+
941+
def test_can_delete_returns_true_for_foreign_actors_as_manager(self):
942+
self.login(self.manager)
943+
944+
center = notification_center()
945+
center.add_watcher_to_resource(self.task, self.dossier_responsible.getId(), WATCHER_ROLE)
946+
947+
self.assertTrue(WatcherDeleter(self.task).can_delete(self.dossier_responsible.getId()))

‎opengever/api/watchers.py‎

Lines changed: 42 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -59,9 +59,15 @@ def __call__(self, expand=False):
5959
}
6060
for role in roles]
6161

62+
watcher_properties = defaultdict(dict)
63+
deleter = WatcherDeleter(self.context)
64+
for actor_id in watchers_and_roles:
65+
watcher_properties[actor_id]['can_delete_watcher'] = deleter.can_delete(actor_id)
66+
6267
result['watchers']['watchers_and_roles'] = watchers_and_roles
6368
result['watchers']['referenced_watcher_roles'] = referenced_watcher_roles
6469
result['watchers']['referenced_actors'] = referenced_actors
70+
result['watchers']['watcher_properties'] = watcher_properties
6571
return result
6672

6773

@@ -94,6 +100,41 @@ def extract_data(self):
94100
raise BadRequest(u"Actor '{}' does not exist".format(self.actor_id))
95101

96102

103+
class WatcherDeleter(object):
104+
105+
def __init__(self, context):
106+
self.context = context
107+
self.center = notification_center()
108+
109+
def can_delete(self, actor_id):
110+
watchers = self.center.get_watchers(self.context, WATCHER_ROLE)
111+
112+
# Delete watcher is only possible for the WATCHER_ROLE
113+
if actor_id not in [watcher.actorid for watcher in watchers]:
114+
return False
115+
116+
# Always allow depending on a permission
117+
if api.user.has_permission('opengever.api: Remove any watcher', obj=self.context):
118+
return True
119+
120+
# The user can always remove itself
121+
if actor_id == api.user.get_current().getId():
122+
return True
123+
124+
# The user can never remove another user
125+
if isinstance(ActorLookup(actor_id).lookup(), OGDSUserActor):
126+
return False
127+
128+
# the user can remove groups and teams
129+
return True
130+
131+
def delete(self, actor_id):
132+
if not self.can_delete(actor_id):
133+
raise Forbidden()
134+
135+
notification_center().remove_watcher_from_resource(self.context, actor_id, WATCHER_ROLE)
136+
137+
97138
@implementer(IPublishTraverse)
98139
class WatchersDelete(Service):
99140
def __init__(self, context, request):
@@ -110,11 +151,7 @@ def reply(self):
110151

111152
self.extract_data()
112153
actor_id = self.read_params()
113-
if not self.can_delete_actor(actor_id):
114-
raise Forbidden()
115-
116-
self.center = notification_center()
117-
self.center.remove_watcher_from_resource(self.context, actor_id, WATCHER_ROLE)
154+
WatcherDeleter(self.context).delete(actor_id)
118155

119156
self.request.response.setStatus(204)
120157
return None
@@ -124,18 +161,6 @@ def extract_data(self):
124161
if data:
125162
raise BadRequest("DELETE does not take any data")
126163

127-
def can_delete_actor(self, actor_id):
128-
# The user can always remove itslef
129-
if actor_id == api.user.get_current().getId():
130-
return True
131-
132-
# The user can never remove another user
133-
if isinstance(ActorLookup(actor_id).lookup(), OGDSUserActor):
134-
return False
135-
136-
# the user can remove groups and teams
137-
return True
138-
139164
def read_params(self):
140165
if len(self.params) == 0:
141166
raise BadRequest("Must supply an actor ID as URL path parameter.")

‎opengever/base/monkey/patches/readonly.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,7 @@ def getRolesInContext(self, context):
230230
'opengever.api: Manage Role Assignment Reports',
231231
'opengever.api: Notify Arbitrary Users',
232232
'opengever.api: Transfer Assignment',
233+
'opengever.api: Remove any watcher',
233234
'opengever.contact: Edit team',
234235
'opengever.disposition: Edit transfer number',
235236
'opengever.document: Cancel',

‎opengever/core/lawgiver.zcml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,7 @@
233233
opengever.api: Manage Groups,
234234
opengever.api: Manage Role Assignment Reports,
235235
opengever.api: Notify Arbitrary Users,
236+
opengever.api: Remove any watcher,
236237
opengever.api: Transfer Assignment,
237238
opengever.api: View AllowedRolesAndPrincipals,
238239
opengever.bumblebee: Revive Preview,

‎opengever/core/profiles/default/rolemap.xml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -454,6 +454,12 @@
454454
<role name="Reviewer" />
455455
</permission>
456456

457+
<permission name="opengever.api: Remove any watcher" acquire="True">
458+
<role name="Manager" />
459+
<role name="Administrator" />
460+
<role name="LimitedAdmin" />
461+
</permission>
462+
457463
</permissions>
458464

459465
</rolemap>

0 commit comments

Comments
 (0)