Fix a check for ancestor editor permissions at create by MyPyDavid · Pull Request #1629 · rdmorganiser/rdmo

MyPyDavid · 2026-05-29T10:19:52Z

Description

Related issue: #1628

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

…1628 Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

jochenklar

Please review and refactor before creating an PR, I will not review purely generated PRs. Also please provice an explanation how the change works and remove the placeholders.

jochenklar · 2026-05-29T11:28:20Z

            }
            response = client.post(url, data, content_type='application/json')
-            assert response.status_code == status_map['create'][username], response.json()
+            expected_status_code = status_map['create'][username]


Pleas do not autogenerate tests without proper review. This check should be based on the user as in every other test and not inferred from the request. This clutters out tests and hinders maintainability.

it looked so nice for a second response.wsgi_request.user.has_perm(...) 🤤
Now I've added a new test cases to the status maps, 'create-with-parent'

jochenklar · 2026-05-29T11:29:12Z

Please use a better name, do not autogenerate tests without proper review.

yes, this one is removed now and a specific test test_*_rejects_foreign_site_parent is added to each viewset test module

jochenklar · 2026-05-29T11:30:41Z

+        if request is None:
+            return
+
+        for field_name, _source_name, _target_name, _through_name in parent_fields:


Why is this not using get_parent_fields?

it needed to be refactored, and the permissions check should only check and not pop any fields, right?!
I'm now using self.get_parent_field_data(attrs) which is also re-used by self.get_parent_fields(validated_data)..

coveralls · 2026-05-29T11:37:06Z

Coverage is 95.102% — 2.4.5/fix/check-ancestor-editor-permissions into 2.4.5/release. No base build found for 2.4.5/release.

MyPyDavid · 2026-05-29T13:23:56Z

Thanks for the review! Yes, this PR was mostly AI-Engineered, it was still in Draft though.

jochenklar · 2026-05-29T14:08:09Z

Ah sorry, good point. I need to check that more carefully.

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

…uery counts Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

…parent test functions Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

MyPyDavid · 2026-06-01T14:49:11Z



-def get_obj_perms_status_code(instance, username, method, editors=None):
+def get_obj_perms_status_code(instance, username, action, editors=None):


this one was refactored just to make it clearer and more explicit what happens in there (since I could not follow it anymore) , this is actually a test "helper" and not a "util"

This whole file is much to complex for just setting up something to check against. Why not just prepare list or dicts or even inline the status_code in the tests?

yes, now, in the age (and with the help of) LLMs, I'll try to make it more explicit..

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

MyPyDavid · 2026-06-02T08:38:07Z

Did some human-in-the-loop and some necessary refactoring and now it's ready for review! 👓

MyPyDavid · 2026-06-02T09:39:05Z

From discussion: maybe the permission check should go into a new separate ParentFieldValidator class instead of in the .validate() calls of the mixin..?
And possibly a test for the ('create-with-parent') OptionSets should be added as well..

jochenklar

Nice, I think it works. I have some remarks about the tests, though.

jochenklar · 2026-06-02T14:29:36Z



-def get_obj_perms_status_code(instance, username, method, editors=None):
+def get_obj_perms_status_code(instance, username, action, editors=None):


This whole file is much to complex for just setting up something to check against. Why not just prepare list or dicts or even inline the status_code in the tests?

jochenklar · 2026-06-02T14:35:50Z

+    site_settings('bar.com')
+    client.login(username='bar-editor', password='bar-editor')
+
+    instance = Page.objects.get(uri_path='foo-page')


I think this is faulty. foo-page is read only for bar-editor anyway, so this does not test the implemented behaviour, right?

Also the section line should just be Section.objects.get(uri_path='foo-section')

via the UI it should seem read only but for the API, bar-editor could post a page to the foo-section. That was the whole point of this fix right?!

I though that the problem was that the bar-editor could post a bar-question to a foo-page (which they don't have permissions on). The bar-editor should not be able to post anything to the foo-question, right? Or maybe I am missing something.

yes exactly! Im trying to make the explicit multisite object permissions status code maps like so :

STATUS_CODES = { 'detail': { 'http://example.com/terms/questions/catalog': status_map['detail'], 'http://example.com/terms/questions/catalog2': status_map['detail'], 'https://foo.com/terms/questions/foo-catalog': { 'user': 404, 'reviewer': 200, 'editor': 200, 'example-reviewer': 200, 'example-editor': 200, 'foo-user': 404, 'foo-reviewer': 200, 'foo-editor': 200, 'bar-user': 404, 'bar-reviewer': 404, 'bar-editor': 404, 'anonymous': 401 }, 'https://bar.com/terms/questions/bar-catalog': { 'user': 404, 'reviewer': 200, 'editor': 200, 'example-reviewer': 200, 'example-editor': 200, 'foo-user': 404, 'foo-reviewer': 404, 'foo-editor': 404, 'bar-user': 404, 'bar-reviewer': 200, 'bar-editor': 200, 'anonymous': 401 }, }, .... }

Yes, I think this is much better. But then the test was faulty before right? Because bar-question/foo-page was not tested (or I missed it).

the test before was more like bar-editor posting a bar-page that contained the foo-section

jochenklar · 2026-06-02T15:13:39Z

And yes, we should ad a check for Option as well.

The validator is probably not worth it since the whole validation is strongly coupled to the serializer.

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

MyPyDavid · 2026-06-03T11:14:05Z

I've refactored/improved the tests and added test for the options as well. Would you approve these changes?

jochenklar

Thanks! I think I will check again tomorrow and then merge.

MyPyDavid self-assigned this May 29, 2026

Add test for checking permissions on ancestor elements #1628

0e33c71

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

MyPyDavid force-pushed the 2.4.5/fix/check-ancestor-editor-permissions branch from d6b4bd6 to 0e33c71 Compare May 29, 2026 10:35

MyPyDavid changed the base branch from main to 2.4.5/release May 29, 2026 10:35

MyPyDavid linked an issue May 29, 2026 that may be closed by this pull request

multisite: editor permissions do not check if the ancestor elements are editable or not #1628

Open

Fix permission checks on parent field in ThroughModelSerializerMixin #…

eea8c8a

…1628 Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

jochenklar requested changes May 29, 2026

View reviewed changes

MyPyDavid changed the title ~~2.4.5/fix/check ancestor editor permissions~~ Draft: 2.4.5/fix/check ancestor editor permissions Jun 1, 2026

MyPyDavid added 6 commits June 1, 2026 13:18

Add and use create-with-parent from status map for test assertions

42700bb

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

Refactor get_parent_fields in ThroughModelSerializerMixin for re-use

6030bd0

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

Add tests for checking foreign-site parent questions elements at create

f31d4a2

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

Add foo and bar parent questions elements fixtures and increase max q…

e546132

…uery counts Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

Refactor test helper for object permission status code

fa413f0

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

Remove redundant test module, refactored into *_rejects_foreign_site_…

dac4720

…parent test functions Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

MyPyDavid commented Jun 1, 2026

View reviewed changes

Refactor parent field methods and get_object_permission helper function

5bd560a

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

MyPyDavid marked this pull request as ready for review June 2, 2026 08:36

MyPyDavid changed the title ~~Draft: 2.4.5/fix/check ancestor editor permissions~~ Fix a check for ancestor editor permissions at create Jun 2, 2026

MyPyDavid requested a review from jochenklar June 2, 2026 08:38

MyPyDavid added this to the RDMO 2.4.5 milestone Jun 2, 2026

MyPyDavid commented Jun 2, 2026

View reviewed changes

Comment thread rdmo/core/permissions.py

MyPyDavid added the status:work-in-progress label Jun 2, 2026

jochenklar added 2 commits June 2, 2026 16:22

Rename and move sites fixture

1a3268c

Slightly refactor ThroughModelSerializerMixin

bbb5e45

jochenklar requested changes Jun 2, 2026

View reviewed changes

MyPyDavid added 3 commits June 3, 2026 11:14

Use status code mappings in multisite tests

cbb853c

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

Add multisite tests for options and optionsets

0789c66

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

Simplify multisite create-with-parent tests

47f8377

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

MyPyDavid requested a review from jochenklar June 3, 2026 11:14

jochenklar reviewed Jun 3, 2026

View reviewed changes

jochenklar merged commit 09daae7 into 2.4.5/release Jun 8, 2026
19 checks passed

jochenklar deleted the 2.4.5/fix/check-ancestor-editor-permissions branch June 8, 2026 07:53



		def get_obj_perms_status_code(instance, username, method, editors=None):
		def get_obj_perms_status_code(instance, username, action, editors=None):

Conversation

MyPyDavid commented May 29, 2026

Description

Uh oh!

jochenklar left a comment

Choose a reason for hiding this comment

Uh oh!

jochenklar May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MyPyDavid Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MyPyDavid commented May 29, 2026

Uh oh!

jochenklar commented May 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MyPyDavid commented Jun 2, 2026

Uh oh!

Uh oh!

MyPyDavid commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jochenklar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jochenklar commented Jun 2, 2026

Uh oh!

MyPyDavid commented Jun 3, 2026

Uh oh!

jochenklar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

jochenklar May 29, 2026 •

edited

Loading

MyPyDavid Jun 2, 2026 •

edited

Loading

coveralls commented May 29, 2026 •

edited

Loading

MyPyDavid commented Jun 2, 2026 •

edited

Loading